Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Woo POS] Split container and list states with new pagination #14727

Merged
7 changes: 7 additions & 0 deletions Fakes/Fakes/Fake.swift
Original file line number Diff line number Diff line change
Expand Up @@ -119,3 +119,10 @@ extension NSRange {
.init()
}
}

extension UUID {
/// Returns a default UUID
static func fake() -> Self {
.init()
}
}
16 changes: 16 additions & 0 deletions Fakes/Fakes/Yosemite.generated.swift
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,22 @@ extension Yosemite.JustInTimeMessageTemplate {
.banner
}
}
extension Yosemite.POSSimpleProduct {
/// Returns a "ready to use" type filled with fake values.
///
public static func fake() -> Yosemite.POSSimpleProduct {
.init(
id: .fake(),
name: .fake(),
formattedPrice: .fake(),
productImageSource: .fake(),
productID: .fake(),
price: .fake(),
productType: .fake(),
bundledItems: .fake()
)
}
}
extension Yosemite.ProductReviewFromNoteParcel {
/// Returns a "ready to use" type filled with fake values.
///
Expand Down
2 changes: 1 addition & 1 deletion Networking/Networking/Remote/ProductsRemote.swift
Original file line number Diff line number Diff line change
Expand Up @@ -645,7 +645,7 @@ public extension ProductsRemote {

private extension ProductsRemote {
enum POSConstants {
static let productsPerPage = "100"
static let productsPerPage = "5"
jaclync marked this conversation as resolved.
Show resolved Hide resolved
static let productType = "simple"
static let productStatus = "publish"
}
Expand Down
6 changes: 3 additions & 3 deletions Networking/NetworkingTests/Remote/ProductsRemoteTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -940,7 +940,7 @@ final class ProductsRemoteTests: XCTestCase {
// When
network.simulateResponse(requestUrlSuffix: "products", filename: "products-load-all-type-simple")

let products = try await remote.loadProductsForPointOfSale(for: sampleSiteID)
let (products, _) = try await remote.loadProductsForPointOfSale(for: sampleSiteID)

// Then
XCTAssertEqual(products.count, expectedProductsFromResponse)
Expand Down Expand Up @@ -970,7 +970,7 @@ final class ProductsRemoteTests: XCTestCase {
// When
network.simulateResponse(requestUrlSuffix: "products", filename: "products-load-all-type-simple")

let products = try await remote.loadProductsForPointOfSale(for: sampleSiteID, pageNumber: initialPageNumber)
let (products, _) = try await remote.loadProductsForPointOfSale(for: sampleSiteID, pageNumber: initialPageNumber)

// Then
XCTAssertEqual(products.count, expectedProductsFromResponse)
Expand All @@ -988,7 +988,7 @@ final class ProductsRemoteTests: XCTestCase {
// When
network.simulateResponse(requestUrlSuffix: "products", filename: "empty-data-array")

let products = try await remote.loadProductsForPointOfSale(for: sampleSiteID, pageNumber: pageNumber)
let (products, _) = try await remote.loadProductsForPointOfSale(for: sampleSiteID, pageNumber: pageNumber)

// Then
XCTAssertEqual(products.count, expectedProductsFromResponse)
Expand Down
27 changes: 27 additions & 0 deletions WooCommerce/Classes/Copiable/Models+Copiable.generated.swift
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,33 @@ extension WooCommerce.AggregateOrderItem {
}
}

extension WooCommerce.ItemsStackState {
func copy(
root: CopiableProp<ItemListState> = .copy
) -> WooCommerce.ItemsStackState {
let root = root ?? self.root

return WooCommerce.ItemsStackState(
root: root
)
}
}

extension WooCommerce.ItemsViewState {
func copy(
containerState: CopiableProp<ItemsContainerState> = .copy,
itemsStack: CopiableProp<ItemsStackState> = .copy
) -> WooCommerce.ItemsViewState {
let containerState = containerState ?? self.containerState
let itemsStack = itemsStack ?? self.itemsStack

return WooCommerce.ItemsViewState(
containerState: containerState,
itemsStack: itemsStack
)
}
}

extension WooCommerce.ShippingLabelSelectedRate {
func copy(
packageID: CopiableProp<String> = .copy,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,88 +5,100 @@ import protocol Yosemite.PointOfSaleItemServiceProtocol
import enum Yosemite.PointOfSaleProductServiceError

protocol PointOfSaleItemsControllerProtocol {
var itemListStatePublisher: any Publisher<ItemListState, Never> { get }
var itemsViewStatePublisher: any Publisher<ItemsViewState, Never> { get }
func loadInitialItems() async
func loadNextItems() async
func reload() async
}

class PointOfSaleItemsController: PointOfSaleItemsControllerProtocol {
private(set) var itemListStatePublisher: any Publisher<ItemListState, Never>
private var itemListStateSubject: PassthroughSubject<ItemListState, Never> = .init()
private var allItems: [POSItem] = []
private var isInitialLoading: Bool = true
private let paginationTracker: PaginationTracker
private(set) var itemsViewStatePublisher: any Publisher<ItemsViewState, Never>
private var itemsViewStateSubject: PassthroughSubject<ItemsViewState, Never> = .init()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reasons not having this as a CurrentValueSubject and use its value when needed, or a @Published var?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a published var because it's not an observable object (and I wanted to keep POSModel as the only one, partly for possibly easier moves to Observable.

It could be a CurrentValueSubject, I just haven't tried it. No time left today but feel free to make that change if you think it'd be an improvement...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, I think it's an improvement not having to need a separate private variable with didSet to sync with the subject. Updated in 00fc51f and it seems to work fine from testing.

private var itemsViewState: ItemsViewState = ItemsViewState(containerState: .loading,
itemsStack: ItemsStackState(root: .loading([]))) {
didSet {
itemsViewStateSubject.send(itemsViewState)
}
}
private let paginationTracker: PaginationTracker = PaginationTracker()
private let itemProvider: PointOfSaleItemServiceProtocol

init(itemProvider: PointOfSaleItemServiceProtocol) {
self.itemProvider = itemProvider
self.paginationTracker = .init(pageFirstIndex: Constants.initialPage)
itemListStatePublisher = itemListStateSubject.eraseToAnyPublisher()
itemsViewStatePublisher = itemsViewStateSubject.eraseToAnyPublisher()

paginationTracker.delegate = self
}

@MainActor
func loadInitialItems() async {
paginationTracker.syncFirstPage()
itemsViewState = ItemsViewState(containerState: .loading, itemsStack: ItemsStackState(root: .loading([])))
await withCheckedContinuation { continuation in
paginationTracker.syncFirstPage {
continuation.resume()
}
}
}

@MainActor
func loadNextItems() async {
paginationTracker.ensureNextPageIsSynced()
let currentItems = itemsViewState.itemsStack.root.items
itemsViewState = ItemsViewState(containerState: .content, itemsStack: ItemsStackState(root: .loading(currentItems)))
await withCheckedContinuation { continuation in
paginationTracker.ensureNextPageIsSynced {
continuation.resume()
}
}
let updatedItems = itemsViewState.itemsStack.root.items
itemsViewState = ItemsViewState(containerState: .content, itemsStack: ItemsStackState(root: .loaded(updatedItems)))
}

@MainActor
func reload() async {
allItems.removeAll()
paginationTracker.resync()
itemsViewState = ItemsViewState(containerState: .content, itemsStack: ItemsStackState(root: .loading([])))
await withCheckedContinuation { continuation in
paginationTracker.resync {
continuation.resume()
}
}
}

/// <#Description#>
/// - Parameter pageNumber: <#pageNumber description#>
/// - Returns: A boolean that indicates whether there is next page for the paginated items.
@MainActor
private func fetchItems(pageNumber: Int) async throws -> Bool {
let (newItems, hasNextPage) = try await itemProvider.providePointOfSaleItems(pageNumber: pageNumber)
var allItems = itemsViewState.itemsStack.root.items
let uniqueNewItems = newItems.filter { newItem in
// Note that this uniquing won't currently work, as POSItem has a UUID.
!allItems.contains(newItem)
}
allItems.append(contentsOf: uniqueNewItems)
itemsViewState = ItemsViewState(containerState: .content,
itemsStack: ItemsStackState(root: .loaded(allItems)))
return hasNextPage
}
}

private func updateItemListStateAfterLoadAttempt() {
if allItems.isEmpty {
itemListStateSubject.send(.empty)
} else {
itemListStateSubject.send(.loaded(allItems))
private extension ItemListState {
var items: [POSItem] {
switch self {
case .loading(let items),
.loaded(let items):
return items
case .error:
return []
}
}

private enum Constants {
static let initialPage: Int = 1
}
}

extension PointOfSaleItemsController: PaginationTrackerDelegate {
func sync(pageNumber: Int, pageSize: Int, reason: String?, onCompletion: SyncCompletion?) {
if isInitialLoading {
isInitialLoading = false
itemListStateSubject.send(.initialLoading)
} else {
itemListStateSubject.send(.loading(allItems))
}
Task { @MainActor in
do {
let hasNextPage = try await fetchItems(pageNumber: pageNumber)
updateItemListStateAfterLoadAttempt()
onCompletion?(.success(hasNextPage))
} catch PointOfSaleProductServiceError.pageOutOfRange {
updateItemListStateAfterLoadAttempt()
onCompletion?(.failure(PointOfSaleProductServiceError.pageOutOfRange))
} catch {
itemListStateSubject.send(.error(PointOfSaleErrorState.errorOnLoadingProducts()))
itemsViewState = ItemsViewState(containerState: .error(PointOfSaleErrorState.errorOnLoadingProducts()),
itemsStack: ItemsStackState(root: .loading([])))
onCompletion?(.failure(error))
}
}
Expand Down
10 changes: 5 additions & 5 deletions WooCommerce/Classes/POS/Models/ItemListState.swift
Original file line number Diff line number Diff line change
@@ -1,14 +1,12 @@
import enum Yosemite.POSItem
import protocol Yosemite.POSOrderableItem
import Codegen

enum ItemListState: Equatable {
case empty
case initialLoading
enum ItemListState {
case loading(_ currentItems: [POSItem])
case loaded(_ items: [POSItem])
case error(PointOfSaleErrorState)

var isLoadingAfterInitialLoad: Bool {
var isLoading: Bool {
switch self {
case .loading:
return true
Expand All @@ -17,3 +15,5 @@ enum ItemListState: Equatable {
}
}
}

extension ItemListState: Equatable, GeneratedCopiable {}
10 changes: 10 additions & 0 deletions WooCommerce/Classes/POS/Models/ItemsContainerState.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
import Foundation

enum ItemsContainerState {
case loading
case empty
case error(PointOfSaleErrorState)
case content
}

extension ItemsContainerState: Equatable {}
9 changes: 9 additions & 0 deletions WooCommerce/Classes/POS/Models/ItemsStackState.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
import Foundation
import enum Yosemite.POSItem
jaclync marked this conversation as resolved.
Show resolved Hide resolved
import Codegen

struct ItemsStackState {
let root: ItemListState
}

extension ItemsStackState: Equatable, GeneratedCopiable {}
9 changes: 9 additions & 0 deletions WooCommerce/Classes/POS/Models/ItemsViewState.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
import Foundation
import Codegen

struct ItemsViewState {
let containerState: ItemsContainerState
let itemsStack: ItemsStackState
}

extension ItemsViewState: GeneratedCopiable, Equatable {}
11 changes: 6 additions & 5 deletions WooCommerce/Classes/POS/Models/PointOfSaleAggregateModel.swift
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ protocol PointOfSaleAggregateModelProtocol {
func cancelCardPaymentsOnboarding()
func trackCardPaymentsOnboardingShown()

var itemListState: ItemListState { get }
var itemsViewState: ItemsViewState { get }
func loadInitialItems() async
func loadNextItems() async
func reload() async
Expand All @@ -47,7 +47,8 @@ class PointOfSaleAggregateModel: ObservableObject, PointOfSaleAggregateModelProt
@Published var cardPresentPaymentOnboardingViewModel: CardPresentPaymentsOnboardingViewModel?
private var onOnboardingCancellation: (() -> Void)?

@Published private(set) var itemListState: ItemListState = .initialLoading
@Published private(set) var itemsViewState: ItemsViewState = ItemsViewState(containerState: .loading,
itemsStack: ItemsStackState(root: .loading([])))

@Published private(set) var cart: [CartItem] = []

Expand All @@ -74,7 +75,7 @@ class PointOfSaleAggregateModel: ObservableObject, PointOfSaleAggregateModelProt
self.orderController = orderController
self.analytics = analytics
self.paymentState = paymentState
publishItemListState()
publishItemsViewState()
publishCardReaderConnectionStatus()
publishPaymentMessages()
publishOrderState()
Expand All @@ -84,8 +85,8 @@ class PointOfSaleAggregateModel: ObservableObject, PointOfSaleAggregateModelProt

// MARK: - ItemList
extension PointOfSaleAggregateModel {
private func publishItemListState() {
itemsController.itemListStatePublisher.assign(to: &$itemListState)
private func publishItemsViewState() {
itemsController.itemsViewStatePublisher.assign(to: &$itemsViewState)
}

@MainActor
Expand Down
Loading