Skip to content

[Woo POS] Coupons: Sync from remote #15423

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Apr 1, 2025
Merged

[Woo POS] Coupons: Sync from remote #15423

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
99109bc
service to fetch coupons from remote
iamgabrielma Mar 26, 2025
d47f648
make storage fallback fetch when remote fails
iamgabrielma Mar 26, 2025
0f236e7
restore storage-first load, and sync from remote
iamgabrielma Mar 31, 2025
3f240a2
Move coupon sync to service
iamgabrielma Mar 31, 2025
1a23559
Handle empty storage by waiting for remote
iamgabrielma Mar 31, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions Networking/Networking/Remote/CouponsRemote.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -372,6 +372,23 @@ public final class CouponsRemote: Remote, CouponsRemoteProtocol {
}
}

extension CouponsRemote {
public func loadAllCoupons(for siteID: Int64,
Copy link
Contributor

@staskus staskus Mar 31, 2025

Choose a reason for hiding this comment

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

super nit; not loading all coupons here

Suggested change
public func loadAllCoupons(for siteID: Int64,
public func loadCoupons(for siteID: Int64,

Copy link
Contributor Author

@iamgabrielma iamgabrielma Mar 31, 2025

Choose a reason for hiding this comment

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

I'm not sure to understand: loadAllCoupons loads all while loadCoupons loads a subset which we know the couponIDs of, right? "makes a single request for a list of coupons" but we don't have that list so we have to fetch them all.

Copy link
Contributor

@staskus staskus Mar 31, 2025

Choose a reason for hiding this comment

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

I just meant from a readability point of view. loadAllCoupons sounds like we're loading ALL coupons, rather than specific page. But I see now that CouponsRemoteProtocol has loadAllCoupons and loadCoupons which is different. 👍 So we can keep it as it is. 👍

Copy link
Contributor Author

@iamgabrielma iamgabrielma Mar 31, 2025

Choose a reason for hiding this comment

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

Ah gotcha, I agree the names are a bit confusing thought, I'll see if I can sneak a task separately to rename these better.

Copy link
Contributor

@staskus staskus Mar 31, 2025

Choose a reason for hiding this comment

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

It's fine, not necessary 👍

pageNumber: Int = CouponsRemote.Default.pageNumber,
pageSize: Int = CouponsRemote.Default.pageSize) async throws -> [Coupon] {
try await withCheckedThrowingContinuation { continuation in
loadAllCoupons(for: siteID, pageNumber: pageNumber, pageSize: pageSize) { result in
switch result {
case .success(let coupons):
continuation.resume(returning: coupons)
case .failure(let error):
continuation.resume(throwing: error)
}
}
}
}
}

// MARK: - Constants
//
public extension CouponsRemote {
Expand Down
1 change: 1 addition & 0 deletions WooCommerce/Classes/POS/Controllers/PointOfSaleCouponsController.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import Observation
import enum Yosemite.POSItem
import enum Yosemite.CouponAction
import protocol Yosemite.PointOfSaleItemServiceProtocol

@available(iOS 17.0, *)
Expand Down
2 changes: 2 additions & 0 deletions WooCommerce/Classes/ViewRelated/Hub Menu/HubMenuViewModel.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,10 +108,12 @@ final class HubMenuViewModel: ObservableObject {
private(set) lazy var posCouponProvider: PointOfSaleItemServiceProtocol = {
let storage = ServiceLocator.storageManager
let currencySettings = ServiceLocator.currencySettings
let stores = ServiceLocator.stores

return PointOfSaleCouponService(siteID: siteID,
currencySettings: currencySettings,
credentials: credentials,
stores: stores,
storage: storage)
}()

Expand Down
63 changes: 49 additions & 14 deletions Yosemite/Yosemite/PointOfSale/PointOfSaleCouponService.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import protocol Networking.Network
import protocol Networking.ProductVariationsRemoteProtocol
import class Networking.ProductsRemote
import class Networking.ProductVariationsRemote
import class Networking.CouponsRemote
import class Networking.AlamofireNetwork
import class WooFoundation.CurrencyFormatter
import class WooFoundation.CurrencySettings
Expand All @@ -10,52 +9,56 @@ import Storage
public final class PointOfSaleCouponService: PointOfSaleItemServiceProtocol {
Copy link
Contributor

@staskus staskus Mar 31, 2025

Choose a reason for hiding this comment

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

Theoretically, PointOfSaleCouponService doesn't need or perhaps even shouldn't be PointOfSaleItemServiceProtocol. It's an independent service with its own logic. It doesn't have to fit the same protocol defined for items.

Having a separate protocol allows us to define an interface that would support our needs. E.g., notifying a controller when remote updates.

public func providePointOfSaleItems(pageNumber: Int) could maybe even be AsyncSequence that returns first round of items from storage and another round when storage is updated with remote. I don't know if we have used it anywhere else yet but maybe could be interesting to try out.

Copy link
Contributor Author

@iamgabrielma iamgabrielma Mar 31, 2025

Choose a reason for hiding this comment

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

That's a very good point, I initially stuck with conforming to the PointOfSaleItemServiceProtocol to follow the same design as products, but its making things more rigid.

As long as we keep the same protocol for the controller, we can switch them in the dashboard to switch between products and coupons (currentController), but the service that feeds it can be independent, which should give us more flexibility. I'll do that as a next step after merging this one just to keep changes separate, but as you mention, this should clarify/simplify the path for the rest of changes.

private var siteID: Int64
private let currencyFormatter: CurrencyFormatter
private let productsRemote: ProductsRemote
private let variationRemote: ProductVariationsRemoteProtocol
private let couponsRemote: CouponsRemote
Copy link
Contributor

@staskus staskus Mar 31, 2025

Choose a reason for hiding this comment

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

Should we use a CouponsRemoteProtocol here to support mocking the remote in unit tests?

Or is it preferred to mock Network instead in the project?

Suggested change
private let couponsRemote: CouponsRemote
private let couponsRemote: CouponsRemoteProtocol

Copy link
Contributor Author

@iamgabrielma iamgabrielma Apr 1, 2025

Choose a reason for hiding this comment

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

Good question, I'm not sure what we'd want to test better right now: Passing a predefined response from mocking the network, or mocking the remote logic, since a lot of it should be covered by pre-existing tests. Perhaps both! I'll take a look into it asap separately with some unit tests.

private let stores: StoresManager?
private let storage: StorageManagerType?

public init(siteID: Int64,
currencySettings: CurrencySettings,
network: Network,
stores: StoresManager? = nil,
storage: StorageManagerType? = nil) {
self.siteID = siteID
self.currencyFormatter = CurrencyFormatter(currencySettings: currencySettings)
self.productsRemote = ProductsRemote(network: network)
self.variationRemote = ProductVariationsRemote(network: network)
self.couponsRemote = CouponsRemote(network: network)
self.stores = stores
self.storage = storage
}

public convenience init(siteID: Int64,
currencySettings: CurrencySettings,
credentials: Credentials?,
stores: StoresManager,
storage: StorageManagerType) {
self.init(siteID: siteID,
currencySettings: currencySettings,
network: AlamofireNetwork(credentials: credentials),
stores: stores,
storage: storage)
}

// TODO:
// gh-15326 - Return PagedItems<POSItem> instead.
@MainActor
public func providePointOfSaleCoupons() -> [POSItem] {
public func providePointOfSaleCoupons() async -> [POSItem] {
guard let storage = storage else {
return []
}

let predicate = NSPredicate(format: "siteID == %lld", siteID)
let descriptor = NSSortDescriptor(keyPath: \StorageCoupon.dateCreated,
ascending: false)

let resultsController = ResultsController<StorageCoupon>(storageManager: storage,
matching: predicate,
sortedBy: [descriptor])
matching: predicate,
sortedBy: [descriptor])

do {
try resultsController.performFetch()
let storeCoupons = resultsController.fetchedObjects
return mapCouponsToPOSItems(coupons: storeCoupons)
let storageCoupons = resultsController.fetchedObjects
return mapCouponsToPOSItems(coupons: storageCoupons)
} catch {
debugPrint(error)
debugPrint("Failed to load coupons from storage:", error)
return []
}
}
Expand All @@ -73,7 +76,39 @@ public final class PointOfSaleCouponService: PointOfSaleItemServiceProtocol {

@MainActor
public func providePointOfSaleItems(pageNumber: Int) async throws -> PagedItems<POSItem> {
let coupons = providePointOfSaleCoupons()
return .init(items: coupons, hasMorePages: false)
let coupons = await providePointOfSaleCoupons()
Copy link
Contributor

@staskus staskus Mar 31, 2025

Choose a reason for hiding this comment

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

Idea: If coupons is empty, we could wait for syncCouponsFromRemote to complete before returning PagedItems.

Copy link
Contributor Author

@iamgabrielma iamgabrielma Mar 31, 2025

Choose a reason for hiding this comment

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

That sounds good, updated here: 1a23559 . I was a bit wary of using a detached Task attached to a dispatcher action, but from testing seems to work fine, let me know if raises any red flag:

  • If non empty, we return storage and fire a sync task which will complete at some point
  • If empty, we wait for the sync both remote and storage, then return that

Copy link
Contributor

@staskus staskus Mar 31, 2025

Choose a reason for hiding this comment

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

I was a bit wary of using a detached Task attached to a dispatcher action, but from testing seems to work fine

I think it's a right usage of a detached Task. We want the storage to be updated with the newest coupons independent from the providePointOfSaleItems.


if !coupons.isEmpty {
// Fire-and-forget sync
Task.detached {
await self.syncCouponsFromRemote(pageNumber: pageNumber)
}
return .init(items: coupons, hasMorePages: false)
} else {
// Wait for the sync to complete
await syncCouponsFromRemote(pageNumber: pageNumber)
let refreshedCoupons = await providePointOfSaleCoupons()
return .init(items: refreshedCoupons, hasMorePages: false)
}
}

private func syncCouponsFromRemote(pageNumber: Int) async {
guard let stores = stores else {
return
}

await withCheckedContinuation { continuation in
let action = CouponAction.synchronizeCoupons(
siteID: siteID,
pageNumber: pageNumber,
pageSize: 25,
onCompletion: { _ in
continuation.resume()
}
)
Task { @MainActor in
stores.dispatch(action)
}
}
}
}
Loading