Skip to content
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ extension PersistedProductAttribute: FetchableRecord, MutablePersistableRecord {

public enum Columns {
static let id = Column(CodingKeys.id)
static let productID = Column(CodingKeys.productID)
public static let productID = Column(CodingKeys.productID)
static let name = Column(CodingKeys.name)
static let position = Column(CodingKeys.position)
static let visible = Column(CodingKeys.visible)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ extension PersistedProductImage: FetchableRecord, PersistableRecord {

public enum Columns {
static let id = Column(CodingKeys.id)
static let productID = Column(CodingKeys.productID)
public static let productID = Column(CodingKeys.productID)
static let dateCreated = Column(CodingKeys.dateCreated)
static let dateModified = Column(CodingKeys.dateModified)
static let src = Column(CodingKeys.src)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ extension PersistedProductVariationAttribute: FetchableRecord, MutablePersistabl

public enum Columns {
static let id = Column(CodingKeys.id)
static let productVariationID = Column(CodingKeys.productVariationID)
public static let productVariationID = Column(CodingKeys.productVariationID)
static let name = Column(CodingKeys.name)
static let option = Column(CodingKeys.option)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ extension PersistedProductVariationImage: FetchableRecord, PersistableRecord {

public enum Columns {
static let id = Column(CodingKeys.id)
static let productVariationID = Column(CodingKeys.productVariationID)
public static let productVariationID = Column(CodingKeys.productVariationID)
Copy link
Contributor

Choose a reason for hiding this comment

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

We can make all these public but I'll do it in a future PR.

I'm sure that periphery would moan about it, but actually it's part of the public interface for the persisted classes. Since the error messages are unclear, it's a better experience to make them public ahead of use.

static let dateCreated = Column(CodingKeys.dateCreated)
static let dateModified = Column(CodingKeys.dateModified)
static let src = Column(CodingKeys.src)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,22 +20,25 @@ public protocol POSCatalogIncrementalSyncServiceProtocol {
public final class POSCatalogIncrementalSyncService: POSCatalogIncrementalSyncServiceProtocol {
private let syncRemote: POSCatalogSyncRemoteProtocol
private let batchSize: Int
private let persistenceService: POSCatalogPersistenceServiceProtocol
private var lastIncrementalSyncDates: [Int64: Date] = [:]
private let batchedLoader: BatchedRequestLoader

public convenience init?(credentials: Credentials?, batchSize: Int = 1) {
public convenience init?(credentials: Credentials?, batchSize: Int = 1, grdbManager: GRDBManagerProtocol) {
guard let credentials else {
DDLogError("⛔️ Could not create POSCatalogIncrementalSyncService due missing credentials")
return nil
}
let network = AlamofireNetwork(credentials: credentials, ensuresSessionManagerIsInitialized: true)
let syncRemote = POSCatalogSyncRemote(network: network)
self.init(syncRemote: syncRemote, batchSize: batchSize)
let persistenceService = POSCatalogPersistenceService(grdbManager: grdbManager)
self.init(syncRemote: syncRemote, batchSize: batchSize, persistenceService: persistenceService)
}

init(syncRemote: POSCatalogSyncRemoteProtocol, batchSize: Int) {
init(syncRemote: POSCatalogSyncRemoteProtocol, batchSize: Int, persistenceService: POSCatalogPersistenceServiceProtocol) {
self.syncRemote = syncRemote
self.batchSize = batchSize
self.persistenceService = persistenceService
self.batchedLoader = BatchedRequestLoader(batchSize: batchSize)
}

Expand All @@ -51,7 +54,8 @@ public final class POSCatalogIncrementalSyncService: POSCatalogIncrementalSyncSe
let catalog = try await loadCatalog(for: siteID, modifiedAfter: modifiedAfter, syncRemote: syncRemote)
DDLogInfo("✅ Loaded \(catalog.products.count) products and \(catalog.variations.count) variations for siteID \(siteID)")

// TODO: WOOMOB-1298 - persist to database
try await persistenceService.persistIncrementalCatalogData(catalog, siteID: siteID)
DDLogInfo("✅ Persisted \(catalog.products.count) products and \(catalog.variations.count) variations to database for siteID \(siteID)")

// TODO: WOOMOB-1289 - replace with store settings persistence
lastIncrementalSyncDates[siteID] = syncStartDate
Expand Down
Original file line number Diff line number Diff line change
@@ -1,13 +1,20 @@
// periphery:ignore:all
import Foundation
import Storage
import GRDB

protocol POSCatalogPersistenceServiceProtocol {
/// Clears existing data and persists new catalog data
/// - Parameters:
/// - catalog: The catalog to persist
/// - siteID: The site ID to associate the catalog with
func replaceAllCatalogData(_ catalog: POSCatalog, siteID: Int64) async throws

/// Persists incremental catalog data (insert/update)
/// - Parameters:
/// - catalog: The catalog difference to persist
/// - siteID: The site ID to associate the catalog with
func persistIncrementalCatalogData(_ catalog: POSCatalog, siteID: Int64) async throws
}

final class POSCatalogPersistenceService: POSCatalogPersistenceServiceProtocol {
Expand All @@ -29,23 +36,23 @@ final class POSCatalogPersistenceService: POSCatalogPersistenceServiceProtocol {
try site.insert(db)

for product in catalog.productsToPersist {
try product.insert(db, onConflict: .ignore)
try product.insert(db, onConflict: .replace)
}

for image in catalog.productImagesToPersist {
try image.insert(db, onConflict: .ignore)
try image.insert(db, onConflict: .replace)
}

for var attribute in catalog.productAttributesToPersist {
try attribute.insert(db)
}

for variation in catalog.variationsToPersist {
try variation.insert(db, onConflict: .ignore)
try variation.insert(db, onConflict: .replace)
}

for image in catalog.variationImagesToPersist {
try image.insert(db, onConflict: .ignore)
try image.insert(db, onConflict: .replace)
}

for var attribute in catalog.variationAttributesToPersist {
Expand All @@ -68,6 +75,67 @@ final class POSCatalogPersistenceService: POSCatalogPersistenceServiceProtocol {
"\(variationImageCount) variation images, \(variationAttributeCount) variation attributes")
}
}

func persistIncrementalCatalogData(_ catalog: POSCatalog, siteID: Int64) async throws {
DDLogInfo("💾 Persisting incremental catalog with \(catalog.products.count) products and \(catalog.variations.count) variations")

try await grdbManager.databaseConnection.write { db in
for product in catalog.productsToPersist {
try product.insert(db, onConflict: .replace)

try PersistedProductImage
.filter { $0.productID == product.id }
.deleteAll(db)

try PersistedProductAttribute
.filter { $0.productID == product.id }
.deleteAll(db)
}

for image in catalog.productImagesToPersist {
try image.insert(db, onConflict: .replace)
}

for var attribute in catalog.productAttributesToPersist {
try attribute.insert(db, onConflict: .replace)
}

for variation in catalog.variationsToPersist {
try variation.insert(db, onConflict: .replace)

try PersistedProductVariationImage
.filter { $0.productVariationID == variation.id }
.deleteAll(db)

try PersistedProductVariationAttribute
.filter { $0.productVariationID == variation.id }
.deleteAll(db)
}

for image in catalog.variationImagesToPersist {
try image.insert(db, onConflict: .replace)
Copy link
Contributor

Choose a reason for hiding this comment

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

For the full sync, I used onConflict: ignore. We should be consistent – it doesn't matter which we go with for images or attributes (when we support global attributes in future.) Either first wins, or last wins.

For variations on an incremental sync, it does matter because we'll definitely want to replace... so perhaps we should use replace everywhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea, incremental sync requires relace on conflict. For full sync, like you said, either works as we delete all the data at the beginning. It only matters if there are duplicate products/variations in the catalog (which is not impossible from the "Large fun testing" test site) with different values, but there's no way to know whether the first or last is correct in this edge case.

I replaced the ignore with replace on conflict strategy in full sync in
7162e20.

}

for var attribute in catalog.variationAttributesToPersist {
try attribute.insert(db, onConflict: .replace)
}
}

DDLogInfo("✅ Incremental catalog persistence complete")

try await grdbManager.databaseConnection.read { db in
let productCount = try PersistedProduct.fetchCount(db)
let productImageCount = try PersistedProductImage.fetchCount(db)
let productAttributeCount = try PersistedProductAttribute.fetchCount(db)
let variationCount = try PersistedProductVariation.fetchCount(db)
let variationImageCount = try PersistedProductVariationImage.fetchCount(db)
let variationAttributeCount = try PersistedProductVariationAttribute.fetchCount(db)

DDLogInfo("Total after incremental update: \(productCount) products, \(productImageCount) product images, " +
"\(productAttributeCount) product attributes, \(variationCount) variations, " +
"\(variationImageCount) variation images, \(variationAttributeCount) variation attributes")
}
}
}

private extension POSCatalog {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,18 @@ import Foundation
import Testing
@testable import Networking
@testable import Yosemite
@testable import Storage

struct POSCatalogIncrementalSyncServiceTests {
private let sut: POSCatalogIncrementalSyncService
private let mockSyncRemote: MockPOSCatalogSyncRemote
private let mockPersistenceService: MockPOSCatalogPersistenceService
private let sampleSiteID: Int64 = 134

init() {
self.mockSyncRemote = MockPOSCatalogSyncRemote()
self.sut = POSCatalogIncrementalSyncService(syncRemote: mockSyncRemote, batchSize: 2)
self.mockPersistenceService = MockPOSCatalogPersistenceService()
self.sut = POSCatalogIncrementalSyncService(syncRemote: mockSyncRemote, batchSize: 2, persistenceService: mockPersistenceService)
}

// MARK: - Basic Incremental Sync Tests
Expand All @@ -32,6 +35,7 @@ struct POSCatalogIncrementalSyncServiceTests {
#expect(mockSyncRemote.loadIncrementalProductVariationsCallCount == 2)
#expect(mockSyncRemote.lastIncrementalProductsModifiedAfter == lastFullSyncDate)
#expect(mockSyncRemote.lastIncrementalVariationsModifiedAfter == lastFullSyncDate)
#expect(mockPersistenceService.persistIncrementalCatalogDataCallCount == 1)
}

@Test func startIncrementalSync_uses_last_incremental_sync_date_as_modifiedAfter_date_when_available() async throws {
Expand Down Expand Up @@ -73,6 +77,8 @@ struct POSCatalogIncrementalSyncServiceTests {

// Then
#expect(mockSyncRemote.loadIncrementalProductsCallCount == 4)
let persistedCatalog = try #require(mockPersistenceService.persistIncrementalCatalogDataLastPersistedCatalog)
#expect(persistedCatalog.products.count == 3)
}

@Test func startIncrementalSync_handles_paginated_variations_correctly() async throws {
Expand All @@ -92,6 +98,8 @@ struct POSCatalogIncrementalSyncServiceTests {

// Then
#expect(mockSyncRemote.loadIncrementalProductVariationsCallCount == 2)
let persistedCatalog = try #require(mockPersistenceService.persistIncrementalCatalogDataLastPersistedCatalog)
#expect(persistedCatalog.variations.count == 2)
}

// MARK: - Error Handling Tests
Expand All @@ -108,13 +116,39 @@ struct POSCatalogIncrementalSyncServiceTests {
await #expect(throws: expectedError) {
try await sut.startIncrementalSync(for: sampleSiteID, lastFullSyncDate: lastFullSyncDate)
}
#expect(mockPersistenceService.persistIncrementalCatalogDataCallCount == 0)

// When attempting a second sync
mockSyncRemote.setIncrementalProductResult(pageNumber: 1, result: .success(PagedItems(items: [], hasMorePages: false, totalItems: 0)))
try await sut.startIncrementalSync(for: sampleSiteID, lastFullSyncDate: lastFullSyncDate)

// Then it uses lastFullSyncDate since no incremental date was stored due to previous failure
#expect(mockSyncRemote.lastIncrementalProductsModifiedAfter == lastFullSyncDate)
#expect(mockPersistenceService.persistIncrementalCatalogDataCallCount == 1)
}

@Test func startIncrementalSync_throws_error_when_persistence_fails() async throws {
// Given
let lastFullSyncDate = Date(timeIntervalSince1970: 1000)
let expectedError = NSError(domain: "persistence", code: 500, userInfo: nil)

mockSyncRemote.setIncrementalProductResult(pageNumber: 1, result: .success(PagedItems(items: [], hasMorePages: false, totalItems: 0)))
mockSyncRemote.setIncrementalVariationResult(pageNumber: 1, result: .success(PagedItems(items: [], hasMorePages: false, totalItems: 0)))
mockPersistenceService.persistIncrementalCatalogDataError = expectedError

// When/Then
await #expect(throws: Error.self) {
try await sut.startIncrementalSync(for: sampleSiteID, lastFullSyncDate: lastFullSyncDate)
}
#expect(mockPersistenceService.persistIncrementalCatalogDataCallCount == 1)

// When attempting a second sync
mockPersistenceService.persistIncrementalCatalogDataError = nil // Clear the error
try await sut.startIncrementalSync(for: sampleSiteID, lastFullSyncDate: lastFullSyncDate)

// Then it uses lastFullSyncDate since no incremental date was stored due to previous persistence failure
#expect(mockSyncRemote.lastIncrementalProductsModifiedAfter == lastFullSyncDate)
#expect(mockPersistenceService.persistIncrementalCatalogDataCallCount == 2)
}

// MARK: - Per-Site Behavior Tests
Expand All @@ -140,3 +174,23 @@ struct POSCatalogIncrementalSyncServiceTests {
#expect(site2ModifiedAfter == lastFullSyncDate)
}
}

// MARK: - Mock Classes

private final class MockPOSCatalogPersistenceService: POSCatalogPersistenceServiceProtocol {
private(set) var persistIncrementalCatalogDataCallCount = 0
private(set) var persistIncrementalCatalogDataLastPersistedCatalog: POSCatalog?
private(set) var persistIncrementalCatalogDataLastPersistedSiteID: Int64?
var persistIncrementalCatalogDataError: Error?

func replaceAllCatalogData(_ catalog: POSCatalog, siteID: Int64) async throws {}

func persistIncrementalCatalogData(_ catalog: POSCatalog, siteID: Int64) async throws {
persistIncrementalCatalogDataCallCount += 1
persistIncrementalCatalogDataLastPersistedSiteID = siteID
persistIncrementalCatalogDataLastPersistedCatalog = catalog
if let error = persistIncrementalCatalogDataError {
throw error
}
}
}
Loading