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
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,36 +5,41 @@ 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(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: AsyncPaginationTracker
private let itemProvider: PointOfSaleItemServiceProtocol

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

@MainActor
func loadInitialItems() async {
itemListStateSubject.send(.initialLoading)
itemsViewState = ItemsViewState(containerState: .loading, itemsStack: ItemsStackState(root: .loading([])))
do {
try await paginationTracker.syncFirstPage { [weak self] pageNumber in
guard let self else { return true }
return try await fetchItems(pageNumber: pageNumber)
}
updateItemListStateAfterLoadAttempt()
} catch {
itemListStateSubject.send(.error(PointOfSaleErrorState.errorOnLoadingProducts()))
itemsViewState = ItemsViewState(containerState: .error(PointOfSaleErrorState.errorOnLoadingProducts()),
itemsStack: ItemsStackState(root: .loaded([])))
}
}

Expand All @@ -43,58 +48,67 @@ class PointOfSaleItemsController: PointOfSaleItemsControllerProtocol {
guard paginationTracker.hasNextPage else {
return
}
itemListStateSubject.send(.loading(allItems))
let currentItems = itemsViewState.itemsStack.root.items
itemsViewState = ItemsViewState(containerState: .content, itemsStack: ItemsStackState(root: .loading(currentItems)))
do {
let nextPageState = try await paginationTracker.ensureNextPageIsSynced { [weak self] pageNumber in
_ = try await paginationTracker.ensureNextPageIsSynced { [weak self] pageNumber in
guard let self else { return true }
return try await fetchItems(pageNumber: pageNumber)
}
switch nextPageState {
case .noNextPage, .synced:
updateItemListStateAfterLoadAttempt()
case .syncing:
break
}
} catch {
// TODO: 14694 - Handle error from loading the next page, like showing an error UI at the end or as an overlay.
itemListStateSubject.send(.error(PointOfSaleErrorState.errorOnLoadingProducts()))
itemsViewState = ItemsViewState(containerState: .error(PointOfSaleErrorState.errorOnLoadingProducts()),
itemsStack: ItemsStackState(root: .loaded(currentItems)))
}
}

@MainActor
func reload() async {
allItems.removeAll()
do {
try await paginationTracker.resync { [weak self] pageNumber in
guard let self else { return true }
return try await fetchItems(pageNumber: pageNumber)
return try await fetchItems(pageNumber: pageNumber, appendToExistingItems: false)
}
updateItemListStateAfterLoadAttempt()
} catch {
// TODO: 14694 - Handle error from pull-to-refresh, like showing an error UI at the beginning or as an overlay.
itemListStateSubject.send(.error(PointOfSaleErrorState.errorOnLoadingProducts()))
itemsViewState = ItemsViewState(containerState: .error(PointOfSaleErrorState.errorOnLoadingProducts()),
itemsStack: ItemsStackState(root: .loaded([])))
}
}

/// Fetches items given a page number and appends new unique items to the `allItems` array.
/// - Parameter pageNumber: Page number to fetch items from.
/// - Parameter appendToExistingItems: Default true – set this to false when refreshing to make the new page the only page.
/// - Returns: A boolean that indicates whether there is next page for the paginated items.
@MainActor
private func fetchItems(pageNumber: Int) async throws -> Bool {
private func fetchItems(pageNumber: Int, appendToExistingItems: Bool = true) async throws -> Bool {
let pagedItems = try await itemProvider.providePointOfSaleItems(pageNumber: pageNumber)
let newItems = pagedItems.items
var allItems = appendToExistingItems ? 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)
if allItems.isEmpty {
itemsViewState = ItemsViewState(containerState: .empty,
itemsStack: ItemsStackState(root: .loaded([])))
} else {
itemsViewState = ItemsViewState(containerState: .content,
itemsStack: ItemsStackState(root: .loaded(allItems)))
}
return pagedItems.hasMorePages
}
}

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 []
}
}
}
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 {}
8 changes: 8 additions & 0 deletions WooCommerce/Classes/POS/Models/ItemsStackState.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
import Foundation
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
26 changes: 14 additions & 12 deletions WooCommerce/Classes/POS/Presentation/ItemListView.swift
Original file line number Diff line number Diff line change
Expand Up @@ -10,20 +10,24 @@ struct ItemListView: View {

@State private var lastScrollPosition: CGFloat = 0
@State private var showSimpleProductsModal: Bool = false
var itemListState: ItemListState {
jaclync marked this conversation as resolved.
Show resolved Hide resolved
posModel.itemsViewState.itemsStack.root
}

@AppStorage(BannerState.isSimpleProductsOnlyBannerDismissedKey)
private var isHeaderBannerDismissed: Bool = false

var body: some View {
VStack {
headerView
switch posModel.itemListState {
case .initialLoading, .empty, .error:
// These cases are handled directly in the dashboard, we do not render
// a specific view within the ItemListView to handle them
EmptyView()
case .loading(let items), .loaded(let items):
switch itemListState {
case .loading(let items),
.loaded(let items):
listView(items)
case .error:
// Currently unused, but this will show errors that are displayed inline with previously
// loaded items, e.g. when loading a new page or refreshing.
EmptyView()
}
}
.refreshable {
Expand Down Expand Up @@ -134,15 +138,15 @@ private extension ItemListView {
listRow(item: item)
}
GhostItemCardView()
.renderedIf(posModel.itemListState.isLoadingAfterInitialLoad)
.renderedIf(itemListState.isLoading)
}
.frame(maxWidth: .infinity)
.padding(.bottom, floatingControlAreaSize.height)
.padding(.horizontal, Constants.itemListPadding)
.background(GeometryReader { proxy in
Color.clear
.onChange(of: proxy.frame(in: .global).maxY) { maxY in
if posModel.itemListState.isLoadingAfterInitialLoad {
if itemListState.isLoading {
return
}
let threshold = Constants.viewHeight * Constants.scrollThresholdMultiplier
Expand Down Expand Up @@ -172,7 +176,7 @@ private extension ItemListView {

private extension ItemListView {
var shouldShowHeaderBanner: Bool {
posModel.itemListState.eligibleToShowSimpleProductsBanner && !isHeaderBannerDismissed
itemListState.eligibleToShowSimpleProductsBanner && !isHeaderBannerDismissed
}
}

Expand All @@ -182,9 +186,7 @@ private extension ItemListState {
case .loading,
.loaded:
return true
case .empty,
.initialLoading,
.error:
case .error:
return false
}
}
Expand Down
Loading
Loading