From de2310bf6803f46e0e1c56ffbbc298ba87745766 Mon Sep 17 00:00:00 2001 From: Huong Do Date: Tue, 26 Nov 2024 11:16:21 +0700 Subject: [PATCH 01/22] Replace deprecated use of writerDerivedStorage --- Yosemite/Yosemite/Stores/ProductTagStore.swift | 15 +++------------ 1 file changed, 3 insertions(+), 12 deletions(-) diff --git a/Yosemite/Yosemite/Stores/ProductTagStore.swift b/Yosemite/Yosemite/Stores/ProductTagStore.swift index 3638e149ef1..c465d6e0682 100644 --- a/Yosemite/Yosemite/Stores/ProductTagStore.swift +++ b/Yosemite/Yosemite/Stores/ProductTagStore.swift @@ -7,10 +7,6 @@ import Storage public final class ProductTagStore: Store { private let remote: ProductTagsRemote - private lazy var sharedDerivedStorage: StorageType = { - return storageManager.writerDerivedStorage - }() - public override init(dispatcher: Dispatcher, storageManager: StorageManagerType, network: Network) { self.remote = ProductTagsRemote(network: network) super.init(dispatcher: dispatcher, storageManager: storageManager, network: network) @@ -132,14 +128,9 @@ private extension ProductTagStore { func upsertStoredProductTagsInBackground(_ readOnlyProductTags: [Networking.ProductTag], siteID: Int64, onCompletion: @escaping () -> Void) { - let derivedStorage = sharedDerivedStorage - derivedStorage.perform { [weak self] in - self?.upsertStoredProductTags(readOnlyProductTags, in: derivedStorage, siteID: siteID) - } - - storageManager.saveDerivedType(derivedStorage: derivedStorage) { - DispatchQueue.main.async(execute: onCompletion) - } + storageManager.performAndSave({ [weak self] storage in + self?.upsertStoredProductTags(readOnlyProductTags, in: storage, siteID: siteID) + }, completion: onCompletion, on: .main) } /// Deletes any Storage.ProductTag that is not associated to a product on the specified `siteID` From 149e08fbbbc550de1ad0ced166d1fc032a74c6e4 Mon Sep 17 00:00:00 2001 From: Huong Do Date: Tue, 26 Nov 2024 11:20:03 +0700 Subject: [PATCH 02/22] Remove unused tags as part of the upsertion --- .../Yosemite/Stores/ProductTagStore.swift | 26 ++++++++++--------- 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/Yosemite/Yosemite/Stores/ProductTagStore.swift b/Yosemite/Yosemite/Stores/ProductTagStore.swift index c465d6e0682..cde3fd2d687 100644 --- a/Yosemite/Yosemite/Stores/ProductTagStore.swift +++ b/Yosemite/Yosemite/Stores/ProductTagStore.swift @@ -76,11 +76,10 @@ private extension ProductTagStore { switch result { case .success(let productTags): - if pageNumber == Default.firstPageNumber { - self?.deleteUnusedStoredProductTags(siteID: siteID) - } - - self?.upsertStoredProductTagsInBackground(productTags, siteID: siteID) { + let shouldRemoveUnusedTags = pageNumber == Default.firstPageNumber + self?.upsertStoredProductTagsInBackground(productTags, + siteID: siteID, + shouldRemoveUnusedTags: shouldRemoveUnusedTags) { onCompletion(.success(productTags)) } case .failure(let error): @@ -95,7 +94,7 @@ private extension ProductTagStore { remote.createProductTags(for: siteID, names: tags) { [weak self] (result) in switch result { case .success(let productTags): - self?.upsertStoredProductTagsInBackground(productTags, siteID: siteID) { + self?.upsertStoredProductTagsInBackground(productTags, siteID: siteID, shouldRemoveUnusedTags: false) { onCompletion(.success(productTags)) } case .failure(let error): @@ -126,17 +125,20 @@ private extension ProductTagStore { /// onCompletion will be called on the main thread! /// func upsertStoredProductTagsInBackground(_ readOnlyProductTags: [Networking.ProductTag], - siteID: Int64, - onCompletion: @escaping () -> Void) { + siteID: Int64, + shouldRemoveUnusedTags: Bool, + onCompletion: @escaping () -> Void) { storageManager.performAndSave({ [weak self] storage in + if shouldRemoveUnusedTags { + self?.deleteUnusedStoredProductTags(siteID: siteID, in: storage) + } self?.upsertStoredProductTags(readOnlyProductTags, in: storage, siteID: siteID) }, completion: onCompletion, on: .main) } /// Deletes any Storage.ProductTag that is not associated to a product on the specified `siteID` /// - func deleteUnusedStoredProductTags(siteID: Int64) { - let storage = storageManager.viewStorage + func deleteUnusedStoredProductTags(siteID: Int64, in storage: StorageType) { storage.deleteUnusedProductTags(siteID: siteID) storage.saveIfNeeded() } @@ -160,8 +162,8 @@ private extension ProductTagStore { /// - siteID: site ID for looking up the ProductTag. /// func upsertStoredProductTags(_ readOnlyProductTags: [Networking.ProductTag], - in storage: StorageType, - siteID: Int64) { + in storage: StorageType, + siteID: Int64) { // Upserts the ProductTag models from the read-only version for readOnlyProductTag in readOnlyProductTags { let storageProductTag: Storage.ProductTag = { From c646ba1c1b6b8b6de76a8d4bb90a9ced2a817b75 Mon Sep 17 00:00:00 2001 From: Huong Do Date: Tue, 26 Nov 2024 11:37:50 +0700 Subject: [PATCH 03/22] Handle deleteStoredProductTags in background --- Yosemite/Yosemite/Stores/ProductTagStore.swift | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/Yosemite/Yosemite/Stores/ProductTagStore.swift b/Yosemite/Yosemite/Stores/ProductTagStore.swift index cde3fd2d687..0e0561d0ee3 100644 --- a/Yosemite/Yosemite/Stores/ProductTagStore.swift +++ b/Yosemite/Yosemite/Stores/ProductTagStore.swift @@ -109,8 +109,9 @@ private extension ProductTagStore { remote.deleteProductTags(for: siteID, ids: ids) { [weak self] (result) in switch result { case .success(let productTags): - self?.deleteStoredProductTags(siteID: siteID, ids: ids) - onCompletion(.success(productTags)) + self?.deleteStoredProductTags(siteID: siteID, ids: ids) { + onCompletion(.success(productTags)) + } case .failure(let error): onCompletion(.failure(error)) } @@ -145,10 +146,10 @@ private extension ProductTagStore { /// Deletes any Storage.ProductTag with the specified `siteID` and `productID` /// - func deleteStoredProductTags(siteID: Int64, ids: [Int64]) { - let storage = storageManager.viewStorage - storage.deleteProductTags(siteID: siteID, ids: ids) - storage.saveIfNeeded() + func deleteStoredProductTags(siteID: Int64, ids: [Int64], onCompletion: @escaping () -> Void) { + storageManager.performAndSave({ storage in + storage.deleteProductTags(siteID: siteID, ids: ids) + }, completion: onCompletion, on: .main) } } From 7450cef2dbe9c08505c81ab06e3aa383f62a167e Mon Sep 17 00:00:00 2001 From: Huong Do Date: Tue, 26 Nov 2024 11:51:05 +0700 Subject: [PATCH 04/22] Remove unused method deleteUnusedStoredProductTags and move the logic to be as part of upsertion --- Yosemite/Yosemite/Stores/ProductTagStore.swift | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/Yosemite/Yosemite/Stores/ProductTagStore.swift b/Yosemite/Yosemite/Stores/ProductTagStore.swift index 0e0561d0ee3..f766ff43436 100644 --- a/Yosemite/Yosemite/Stores/ProductTagStore.swift +++ b/Yosemite/Yosemite/Stores/ProductTagStore.swift @@ -131,19 +131,12 @@ private extension ProductTagStore { onCompletion: @escaping () -> Void) { storageManager.performAndSave({ [weak self] storage in if shouldRemoveUnusedTags { - self?.deleteUnusedStoredProductTags(siteID: siteID, in: storage) + storage.deleteUnusedProductTags(siteID: siteID) } self?.upsertStoredProductTags(readOnlyProductTags, in: storage, siteID: siteID) }, completion: onCompletion, on: .main) } - /// Deletes any Storage.ProductTag that is not associated to a product on the specified `siteID` - /// - func deleteUnusedStoredProductTags(siteID: Int64, in storage: StorageType) { - storage.deleteUnusedProductTags(siteID: siteID) - storage.saveIfNeeded() - } - /// Deletes any Storage.ProductTag with the specified `siteID` and `productID` /// func deleteStoredProductTags(siteID: Int64, ids: [Int64], onCompletion: @escaping () -> Void) { From 5c6a7f4a8a1cdd7077175123c55797ed80085fda Mon Sep 17 00:00:00 2001 From: Huong Do Date: Tue, 26 Nov 2024 13:42:48 +0700 Subject: [PATCH 05/22] Add new field count for ProductTag --- Fakes/Fakes/Networking.generated.swift | 3 ++- Fakes/Fakes/Products/ProductFactory.swift | 2 +- .../Mapper/ProductTagListMapper.swift | 2 +- .../Copiable/Models+Copiable.generated.swift | 7 +++++-- .../Networking/Model/Product/ProductTag.swift | 10 ++++++++-- .../Mapper/ProductTagListMapperTests.swift | 2 ++ .../Remote/GenerativeContentRemoteTests.swift | 8 ++++---- .../Remote/ProductsRemoteTests.swift | 18 +++++++++--------- .../product-tags-all-without-data.json | 8 ++++---- .../Responses/product-tags-all.json | 8 ++++---- .../ProductDetailPreviewViewModel.swift | 2 +- .../ProductTag+ReadOnlyConvertible.swift | 3 ++- 12 files changed, 43 insertions(+), 30 deletions(-) diff --git a/Fakes/Fakes/Networking.generated.swift b/Fakes/Fakes/Networking.generated.swift index a7a2d3b8b25..4d19e8a913b 100644 --- a/Fakes/Fakes/Networking.generated.swift +++ b/Fakes/Fakes/Networking.generated.swift @@ -1628,7 +1628,8 @@ extension Networking.ProductTag { siteID: .fake(), tagID: .fake(), name: .fake(), - slug: .fake() + slug: .fake(), + count: .fake() ) } } diff --git a/Fakes/Fakes/Products/ProductFactory.swift b/Fakes/Fakes/Products/ProductFactory.swift index 51ee86ddd1c..e039aaa2379 100644 --- a/Fakes/Fakes/Products/ProductFactory.swift +++ b/Fakes/Fakes/Products/ProductFactory.swift @@ -66,6 +66,6 @@ public enum ProductFactory { upsellIDs: [1, 2], crossSellIDs: [3, 4], categories: [ProductCategory(categoryID: 1, siteID: 2, parentID: 6, name: "", slug: "")], - tags: [ProductTag(siteID: 123, tagID: 1, name: "", slug: "")]) + tags: [ProductTag(siteID: 123, tagID: 1, name: "", slug: "", count: 0)]) } } diff --git a/Networking/Networking/Mapper/ProductTagListMapper.swift b/Networking/Networking/Mapper/ProductTagListMapper.swift index 359f164c4d2..243f5c1588b 100644 --- a/Networking/Networking/Mapper/ProductTagListMapper.swift +++ b/Networking/Networking/Mapper/ProductTagListMapper.swift @@ -39,7 +39,7 @@ struct ProductTagListMapper: Mapper { .filter { $0.error == nil } .compactMap { (tagCreated) -> ProductTag? in if let name = tagCreated.name, let slug = tagCreated.slug { - return ProductTag(siteID: tagCreated.siteID, tagID: tagCreated.tagID, name: name, slug: slug) + return ProductTag(siteID: tagCreated.siteID, tagID: tagCreated.tagID, name: name, slug: slug, count: 0) } return nil } diff --git a/Networking/Networking/Model/Copiable/Models+Copiable.generated.swift b/Networking/Networking/Model/Copiable/Models+Copiable.generated.swift index c7ed46b6838..930b35d7acb 100644 --- a/Networking/Networking/Model/Copiable/Models+Copiable.generated.swift +++ b/Networking/Networking/Model/Copiable/Models+Copiable.generated.swift @@ -2634,18 +2634,21 @@ extension Networking.ProductTag { siteID: CopiableProp = .copy, tagID: CopiableProp = .copy, name: CopiableProp = .copy, - slug: CopiableProp = .copy + slug: CopiableProp = .copy, + count: CopiableProp = .copy ) -> Networking.ProductTag { let siteID = siteID ?? self.siteID let tagID = tagID ?? self.tagID let name = name ?? self.name let slug = slug ?? self.slug + let count = count ?? self.count return Networking.ProductTag( siteID: siteID, tagID: tagID, name: name, - slug: slug + slug: slug, + count: count ) } } diff --git a/Networking/Networking/Model/Product/ProductTag.swift b/Networking/Networking/Model/Product/ProductTag.swift index 2b0bea1aa1a..79d87cc099b 100644 --- a/Networking/Networking/Model/Product/ProductTag.swift +++ b/Networking/Networking/Model/Product/ProductTag.swift @@ -8,17 +8,20 @@ public struct ProductTag: Codable, Equatable, GeneratedFakeable, GeneratedCopiab public let tagID: Int64 public let name: String public let slug: String + public let count: Int /// ProductTag initializer. /// public init(siteID: Int64, tagID: Int64, name: String, - slug: String) { + slug: String, + count: Int) { self.siteID = siteID self.tagID = tagID self.name = name self.slug = slug + self.count = count } /// Public initializer for ProductTag. @@ -33,8 +36,10 @@ public struct ProductTag: Codable, Equatable, GeneratedFakeable, GeneratedCopiab let tagID = try container.decode(Int64.self, forKey: .tagID) let name = try container.decode(String.self, forKey: .name) let slug = try container.decode(String.self, forKey: .slug) + /// `count` is available when fetching from `wc/v3/products/tags` but not from `wc/v3/products` + let count = (try container.decodeIfPresent(Int.self, forKey: .count)) ?? 0 - self.init(siteID: siteID, tagID: tagID, name: name, slug: slug) + self.init(siteID: siteID, tagID: tagID, name: name, slug: slug, count: count) } public func encode(to encoder: Encoder) throws { @@ -54,6 +59,7 @@ private extension ProductTag { case tagID = "id" case name = "name" case slug = "slug" + case count = "count" } } diff --git a/Networking/NetworkingTests/Mapper/ProductTagListMapperTests.swift b/Networking/NetworkingTests/Mapper/ProductTagListMapperTests.swift index 825f442c98b..419ceb89895 100644 --- a/Networking/NetworkingTests/Mapper/ProductTagListMapperTests.swift +++ b/Networking/NetworkingTests/Mapper/ProductTagListMapperTests.swift @@ -16,6 +16,7 @@ final class ProductTagListMapperTests: XCTestCase { XCTAssertEqual(secondTag.tagID, 35) XCTAssertEqual(secondTag.name, "Oxford Shoes") XCTAssertEqual(secondTag.slug, "oxford-shoes") + XCTAssertEqual(secondTag.count, 2) } /// Verifies that all of the ProductTag Fields are parsed correctly. @@ -28,6 +29,7 @@ final class ProductTagListMapperTests: XCTestCase { XCTAssertEqual(secondTag.tagID, 35) XCTAssertEqual(secondTag.name, "Oxford Shoes") XCTAssertEqual(secondTag.slug, "oxford-shoes") + XCTAssertEqual(secondTag.count, 2) } /// Verifies that all of the ProductTag Fields under `create` field are parsed correctly. diff --git a/Networking/NetworkingTests/Remote/GenerativeContentRemoteTests.swift b/Networking/NetworkingTests/Remote/GenerativeContentRemoteTests.swift index 682f826df8a..2c6c8cc0730 100644 --- a/Networking/NetworkingTests/Remote/GenerativeContentRemoteTests.swift +++ b/Networking/NetworkingTests/Remote/GenerativeContentRemoteTests.swift @@ -421,8 +421,8 @@ final class GenerativeContentRemoteTests: XCTestCase { dimensionUnit: "cm", weightUnit: "kg", categories: [], - tags: [.init(siteID: sampleSiteID, tagID: 1, name: "Food", slug: ""), - .init(siteID: sampleSiteID, tagID: 2, name: "Grocery", slug: "")]) + tags: [.init(siteID: sampleSiteID, tagID: 1, name: "Food", slug: "", count: 0), + .init(siteID: sampleSiteID, tagID: 2, name: "Grocery", slug: "", count: 0)]) // Then let request = try XCTUnwrap(network.requestsForResponseData.last as? DotcomRequest) @@ -446,8 +446,8 @@ final class GenerativeContentRemoteTests: XCTestCase { dimensionUnit: "cm", weightUnit: "kg", categories: [], - tags: [.init(siteID: sampleSiteID, tagID: 1, name: "Food", slug: ""), - .init(siteID: sampleSiteID, tagID: 2, name: "Grocery", slug: "")]) + tags: [.init(siteID: sampleSiteID, tagID: 1, name: "Food", slug: "", count: 0), + .init(siteID: sampleSiteID, tagID: 2, name: "Grocery", slug: "", count: 0)]) // Then let request = try XCTUnwrap(network.requestsForResponseData.last as? DotcomRequest) diff --git a/Networking/NetworkingTests/Remote/ProductsRemoteTests.swift b/Networking/NetworkingTests/Remote/ProductsRemoteTests.swift index f34a77786e2..cfebafeea25 100644 --- a/Networking/NetworkingTests/Remote/ProductsRemoteTests.swift +++ b/Networking/NetworkingTests/Remote/ProductsRemoteTests.swift @@ -1090,15 +1090,15 @@ private extension ProductsRemoteTests { } func sampleTags() -> [Networking.ProductTag] { - let tag1 = ProductTag(siteID: sampleSiteID, tagID: 37, name: "room", slug: "room") - let tag2 = ProductTag(siteID: sampleSiteID, tagID: 38, name: "party room", slug: "party-room") - let tag3 = ProductTag(siteID: sampleSiteID, tagID: 39, name: "30", slug: "30") - let tag4 = ProductTag(siteID: sampleSiteID, tagID: 40, name: "20+", slug: "20") - let tag5 = ProductTag(siteID: sampleSiteID, tagID: 41, name: "meeting room", slug: "meeting-room") - let tag6 = ProductTag(siteID: sampleSiteID, tagID: 42, name: "meetings", slug: "meetings") - let tag7 = ProductTag(siteID: sampleSiteID, tagID: 43, name: "parties", slug: "parties") - let tag8 = ProductTag(siteID: sampleSiteID, tagID: 44, name: "graduation", slug: "graduation") - let tag9 = ProductTag(siteID: sampleSiteID, tagID: 45, name: "birthday party", slug: "birthday-party") + let tag1 = ProductTag(siteID: sampleSiteID, tagID: 37, name: "room", slug: "room", count: 0) + let tag2 = ProductTag(siteID: sampleSiteID, tagID: 38, name: "party room", slug: "party-room", count: 0) + let tag3 = ProductTag(siteID: sampleSiteID, tagID: 39, name: "30", slug: "30", count: 0) + let tag4 = ProductTag(siteID: sampleSiteID, tagID: 40, name: "20+", slug: "20", count: 0) + let tag5 = ProductTag(siteID: sampleSiteID, tagID: 41, name: "meeting room", slug: "meeting-room", count: 0) + let tag6 = ProductTag(siteID: sampleSiteID, tagID: 42, name: "meetings", slug: "meetings", count: 0) + let tag7 = ProductTag(siteID: sampleSiteID, tagID: 43, name: "parties", slug: "parties", count: 0) + let tag8 = ProductTag(siteID: sampleSiteID, tagID: 44, name: "graduation", slug: "graduation", count: 0) + let tag9 = ProductTag(siteID: sampleSiteID, tagID: 45, name: "birthday party", slug: "birthday-party", count: 0) return [tag1, tag2, tag3, tag4, tag5, tag6, tag7, tag8, tag9] } diff --git a/Networking/NetworkingTests/Responses/product-tags-all-without-data.json b/Networking/NetworkingTests/Responses/product-tags-all-without-data.json index 6728510d597..62ae6ff32cb 100644 --- a/Networking/NetworkingTests/Responses/product-tags-all-without-data.json +++ b/Networking/NetworkingTests/Responses/product-tags-all-without-data.json @@ -4,7 +4,7 @@ "name": "Leather Shoes", "slug": "leather-shoes", "description": "", - "count": 0, + "count": 1, "_links": { "self": [ { @@ -23,7 +23,7 @@ "name": "Oxford Shoes", "slug": "oxford-shoes", "description": "", - "count": 0, + "count": 2, "_links": { "self": [ { @@ -42,7 +42,7 @@ "name": "Leather Boots", "slug": "leather-boots", "description": "", - "count": 0, + "count": 3, "_links": { "self": [ { @@ -61,7 +61,7 @@ "name": "Sneakers", "slug": "sneakers", "description": "A shoe suitable for playing sports", - "count": 0, + "count": 4, "_links": { "self": [ { diff --git a/Networking/NetworkingTests/Responses/product-tags-all.json b/Networking/NetworkingTests/Responses/product-tags-all.json index f361d2b7307..c7921f09afc 100644 --- a/Networking/NetworkingTests/Responses/product-tags-all.json +++ b/Networking/NetworkingTests/Responses/product-tags-all.json @@ -5,7 +5,7 @@ "name": "Leather Shoes", "slug": "leather-shoes", "description": "", - "count": 0, + "count": 1, "_links": { "self": [ { @@ -24,7 +24,7 @@ "name": "Oxford Shoes", "slug": "oxford-shoes", "description": "", - "count": 0, + "count": 2, "_links": { "self": [ { @@ -43,7 +43,7 @@ "name": "Leather Boots", "slug": "leather-boots", "description": "", - "count": 0, + "count": 3, "_links": { "self": [ { @@ -62,7 +62,7 @@ "name": "Sneakers", "slug": "sneakers", "description": "A shoe suitable for playing sports", - "count": 0, + "count": 4, "_links": { "self": [ { diff --git a/WooCommerce/Classes/ViewRelated/Products/Add Product/AddProductWithAI/Preview/ProductDetailPreviewViewModel.swift b/WooCommerce/Classes/ViewRelated/Products/Add Product/AddProductWithAI/Preview/ProductDetailPreviewViewModel.swift index 0a592fde6ee..d0797fa3b57 100644 --- a/WooCommerce/Classes/ViewRelated/Products/Add Product/AddProductWithAI/Preview/ProductDetailPreviewViewModel.swift +++ b/WooCommerce/Classes/ViewRelated/Products/Add Product/AddProductWithAI/Preview/ProductDetailPreviewViewModel.swift @@ -603,7 +603,7 @@ private extension ProductDetailPreviewViewModel { /// /// We will later upload the local tag using `saveLocalCategoriesAndTags` method /// - tags.append(ProductTag(siteID: siteID, tagID: 0, name: aiTag, slug: "")) + tags.append(ProductTag(siteID: siteID, tagID: 0, name: aiTag, slug: "", count: 0)) } } diff --git a/Yosemite/Yosemite/Model/Storage/ProductTag+ReadOnlyConvertible.swift b/Yosemite/Yosemite/Model/Storage/ProductTag+ReadOnlyConvertible.swift index e7e33c3be17..ae71469161b 100644 --- a/Yosemite/Yosemite/Model/Storage/ProductTag+ReadOnlyConvertible.swift +++ b/Yosemite/Yosemite/Model/Storage/ProductTag+ReadOnlyConvertible.swift @@ -21,6 +21,7 @@ extension Storage.ProductTag: ReadOnlyConvertible { return ProductTag(siteID: siteID, tagID: tagID, name: name, - slug: slug) + slug: slug, + count: products?.count ?? 0) } } From 9ee30afd0409484320589db8dd308f3552d6cc4e Mon Sep 17 00:00:00 2001 From: Huong Do Date: Tue, 26 Nov 2024 13:43:15 +0700 Subject: [PATCH 06/22] Only upsert product tags with associated products --- Yosemite/Yosemite/Stores/ProductTagStore.swift | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Yosemite/Yosemite/Stores/ProductTagStore.swift b/Yosemite/Yosemite/Stores/ProductTagStore.swift index f766ff43436..291a7783b0c 100644 --- a/Yosemite/Yosemite/Stores/ProductTagStore.swift +++ b/Yosemite/Yosemite/Stores/ProductTagStore.swift @@ -77,7 +77,9 @@ private extension ProductTagStore { switch result { case .success(let productTags): let shouldRemoveUnusedTags = pageNumber == Default.firstPageNumber - self?.upsertStoredProductTagsInBackground(productTags, + /// Only upsert tags that are associated with at least 1 product. + let filteredTags = productTags.filter { $0.count > 0 } + self?.upsertStoredProductTagsInBackground(filteredTags, siteID: siteID, shouldRemoveUnusedTags: shouldRemoveUnusedTags) { onCompletion(.success(productTags)) From 5f103afd9534a8b1d65b6a97e4cda183bbf33b61 Mon Sep 17 00:00:00 2001 From: Huong Do Date: Tue, 26 Nov 2024 15:17:29 +0700 Subject: [PATCH 07/22] Fix test build failure --- Yosemite/YosemiteTests/Stores/ProductTagStoreTests.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Yosemite/YosemiteTests/Stores/ProductTagStoreTests.swift b/Yosemite/YosemiteTests/Stores/ProductTagStoreTests.swift index 8714a544fc1..bedac8cda17 100644 --- a/Yosemite/YosemiteTests/Stores/ProductTagStoreTests.swift +++ b/Yosemite/YosemiteTests/Stores/ProductTagStoreTests.swift @@ -351,6 +351,6 @@ final class ProductTagStoreTests: XCTestCase { private extension ProductTagStoreTests { func sampleTag(tagID: Int64) -> Networking.ProductTag { - return Networking.ProductTag(siteID: sampleSiteID, tagID: tagID, name: "Sample", slug: "sample") + return Networking.ProductTag(siteID: sampleSiteID, tagID: tagID, name: "Sample", slug: "sample", count: 0) } } From d1c2ba452e3fb58a321f4d88f8f198c3c97158ce Mon Sep 17 00:00:00 2001 From: Huong Do Date: Tue, 26 Nov 2024 16:25:32 +0700 Subject: [PATCH 08/22] Add test for addProductTags to check when tag list is empty --- .../Yosemite/Stores/ProductTagStore.swift | 3 +++ .../Stores/ProductTagStoreTests.swift | 19 +++++++++++++++++++ 2 files changed, 22 insertions(+) diff --git a/Yosemite/Yosemite/Stores/ProductTagStore.swift b/Yosemite/Yosemite/Stores/ProductTagStore.swift index 291a7783b0c..20164a795a5 100644 --- a/Yosemite/Yosemite/Stores/ProductTagStore.swift +++ b/Yosemite/Yosemite/Stores/ProductTagStore.swift @@ -93,6 +93,9 @@ private extension ProductTagStore { /// Create new product tags associated with a given Site ID. /// func addProductTags(siteID: Int64, tags: [String], onCompletion: @escaping (Result<[ProductTag], Error>) -> Void) { + guard tags.isEmpty == false else { + return onCompletion(.success([])) + } remote.createProductTags(for: siteID, names: tags) { [weak self] (result) in switch result { case .success(let productTags): diff --git a/Yosemite/YosemiteTests/Stores/ProductTagStoreTests.swift b/Yosemite/YosemiteTests/Stores/ProductTagStoreTests.swift index bedac8cda17..1feae44bb7c 100644 --- a/Yosemite/YosemiteTests/Stores/ProductTagStoreTests.swift +++ b/Yosemite/YosemiteTests/Stores/ProductTagStoreTests.swift @@ -238,6 +238,25 @@ final class ProductTagStoreTests: XCTestCase { XCTAssertNotNil(result?.failure) } + func test_addProductTags_returns_success_for_empty_tag_list() throws { + // When + var result: Result<[Networking.ProductTag], Error>? + waitForExpectation { (exp) in + let action = ProductTagAction.addProductTags(siteID: sampleSiteID, tags: []) { (aResult) in + result = aResult + exp.fulfill() + } + store.onAction(action) + } + + // Then + let receivedResult = try XCTUnwrap(result) + XCTAssertTrue(receivedResult.isSuccess) + + let addedTags = try receivedResult.get() + XCTAssertTrue(addedTags.isEmpty) + } + func testAddProductTagReturnsErrorUponEmptyResponse() { // Given an empty network response XCTAssertEqual(storedProductTagsCount, 0) From c1ddb994094b47b65bc8b7dd2aba1e7a30735b17 Mon Sep 17 00:00:00 2001 From: Huong Do Date: Tue, 26 Nov 2024 17:03:08 +0700 Subject: [PATCH 09/22] Set default value for count to avoid unnecessary changes --- .../Networking/Model/Product/ProductTag.swift | 2 +- .../Remote/GenerativeContentRemoteTests.swift | 8 ++++---- .../Remote/ProductsRemoteTests.swift | 18 +++++++++--------- .../ProductDetailPreviewViewModel.swift | 2 +- .../ProductTag+ReadOnlyConvertible.swift | 2 +- 5 files changed, 16 insertions(+), 16 deletions(-) diff --git a/Networking/Networking/Model/Product/ProductTag.swift b/Networking/Networking/Model/Product/ProductTag.swift index 79d87cc099b..85f492e8046 100644 --- a/Networking/Networking/Model/Product/ProductTag.swift +++ b/Networking/Networking/Model/Product/ProductTag.swift @@ -16,7 +16,7 @@ public struct ProductTag: Codable, Equatable, GeneratedFakeable, GeneratedCopiab tagID: Int64, name: String, slug: String, - count: Int) { + count: Int = 0) { self.siteID = siteID self.tagID = tagID self.name = name diff --git a/Networking/NetworkingTests/Remote/GenerativeContentRemoteTests.swift b/Networking/NetworkingTests/Remote/GenerativeContentRemoteTests.swift index 2c6c8cc0730..682f826df8a 100644 --- a/Networking/NetworkingTests/Remote/GenerativeContentRemoteTests.swift +++ b/Networking/NetworkingTests/Remote/GenerativeContentRemoteTests.swift @@ -421,8 +421,8 @@ final class GenerativeContentRemoteTests: XCTestCase { dimensionUnit: "cm", weightUnit: "kg", categories: [], - tags: [.init(siteID: sampleSiteID, tagID: 1, name: "Food", slug: "", count: 0), - .init(siteID: sampleSiteID, tagID: 2, name: "Grocery", slug: "", count: 0)]) + tags: [.init(siteID: sampleSiteID, tagID: 1, name: "Food", slug: ""), + .init(siteID: sampleSiteID, tagID: 2, name: "Grocery", slug: "")]) // Then let request = try XCTUnwrap(network.requestsForResponseData.last as? DotcomRequest) @@ -446,8 +446,8 @@ final class GenerativeContentRemoteTests: XCTestCase { dimensionUnit: "cm", weightUnit: "kg", categories: [], - tags: [.init(siteID: sampleSiteID, tagID: 1, name: "Food", slug: "", count: 0), - .init(siteID: sampleSiteID, tagID: 2, name: "Grocery", slug: "", count: 0)]) + tags: [.init(siteID: sampleSiteID, tagID: 1, name: "Food", slug: ""), + .init(siteID: sampleSiteID, tagID: 2, name: "Grocery", slug: "")]) // Then let request = try XCTUnwrap(network.requestsForResponseData.last as? DotcomRequest) diff --git a/Networking/NetworkingTests/Remote/ProductsRemoteTests.swift b/Networking/NetworkingTests/Remote/ProductsRemoteTests.swift index cfebafeea25..f34a77786e2 100644 --- a/Networking/NetworkingTests/Remote/ProductsRemoteTests.swift +++ b/Networking/NetworkingTests/Remote/ProductsRemoteTests.swift @@ -1090,15 +1090,15 @@ private extension ProductsRemoteTests { } func sampleTags() -> [Networking.ProductTag] { - let tag1 = ProductTag(siteID: sampleSiteID, tagID: 37, name: "room", slug: "room", count: 0) - let tag2 = ProductTag(siteID: sampleSiteID, tagID: 38, name: "party room", slug: "party-room", count: 0) - let tag3 = ProductTag(siteID: sampleSiteID, tagID: 39, name: "30", slug: "30", count: 0) - let tag4 = ProductTag(siteID: sampleSiteID, tagID: 40, name: "20+", slug: "20", count: 0) - let tag5 = ProductTag(siteID: sampleSiteID, tagID: 41, name: "meeting room", slug: "meeting-room", count: 0) - let tag6 = ProductTag(siteID: sampleSiteID, tagID: 42, name: "meetings", slug: "meetings", count: 0) - let tag7 = ProductTag(siteID: sampleSiteID, tagID: 43, name: "parties", slug: "parties", count: 0) - let tag8 = ProductTag(siteID: sampleSiteID, tagID: 44, name: "graduation", slug: "graduation", count: 0) - let tag9 = ProductTag(siteID: sampleSiteID, tagID: 45, name: "birthday party", slug: "birthday-party", count: 0) + let tag1 = ProductTag(siteID: sampleSiteID, tagID: 37, name: "room", slug: "room") + let tag2 = ProductTag(siteID: sampleSiteID, tagID: 38, name: "party room", slug: "party-room") + let tag3 = ProductTag(siteID: sampleSiteID, tagID: 39, name: "30", slug: "30") + let tag4 = ProductTag(siteID: sampleSiteID, tagID: 40, name: "20+", slug: "20") + let tag5 = ProductTag(siteID: sampleSiteID, tagID: 41, name: "meeting room", slug: "meeting-room") + let tag6 = ProductTag(siteID: sampleSiteID, tagID: 42, name: "meetings", slug: "meetings") + let tag7 = ProductTag(siteID: sampleSiteID, tagID: 43, name: "parties", slug: "parties") + let tag8 = ProductTag(siteID: sampleSiteID, tagID: 44, name: "graduation", slug: "graduation") + let tag9 = ProductTag(siteID: sampleSiteID, tagID: 45, name: "birthday party", slug: "birthday-party") return [tag1, tag2, tag3, tag4, tag5, tag6, tag7, tag8, tag9] } diff --git a/WooCommerce/Classes/ViewRelated/Products/Add Product/AddProductWithAI/Preview/ProductDetailPreviewViewModel.swift b/WooCommerce/Classes/ViewRelated/Products/Add Product/AddProductWithAI/Preview/ProductDetailPreviewViewModel.swift index d0797fa3b57..0a592fde6ee 100644 --- a/WooCommerce/Classes/ViewRelated/Products/Add Product/AddProductWithAI/Preview/ProductDetailPreviewViewModel.swift +++ b/WooCommerce/Classes/ViewRelated/Products/Add Product/AddProductWithAI/Preview/ProductDetailPreviewViewModel.swift @@ -603,7 +603,7 @@ private extension ProductDetailPreviewViewModel { /// /// We will later upload the local tag using `saveLocalCategoriesAndTags` method /// - tags.append(ProductTag(siteID: siteID, tagID: 0, name: aiTag, slug: "", count: 0)) + tags.append(ProductTag(siteID: siteID, tagID: 0, name: aiTag, slug: "")) } } diff --git a/Yosemite/Yosemite/Model/Storage/ProductTag+ReadOnlyConvertible.swift b/Yosemite/Yosemite/Model/Storage/ProductTag+ReadOnlyConvertible.swift index ae71469161b..ee3911b3aa8 100644 --- a/Yosemite/Yosemite/Model/Storage/ProductTag+ReadOnlyConvertible.swift +++ b/Yosemite/Yosemite/Model/Storage/ProductTag+ReadOnlyConvertible.swift @@ -22,6 +22,6 @@ extension Storage.ProductTag: ReadOnlyConvertible { tagID: tagID, name: name, slug: slug, - count: products?.count ?? 0) + count: 0) } } From 56b23b39602b71c86f2dd4fad43ff51b8feefabd Mon Sep 17 00:00:00 2001 From: Huong Do Date: Tue, 26 Nov 2024 17:04:06 +0700 Subject: [PATCH 10/22] Fix test failure for ProductTagStore --- Networking/NetworkingTests/Responses/product-tags-extra.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Networking/NetworkingTests/Responses/product-tags-extra.json b/Networking/NetworkingTests/Responses/product-tags-extra.json index 2e83a0e2572..3ca60a45343 100644 --- a/Networking/NetworkingTests/Responses/product-tags-extra.json +++ b/Networking/NetworkingTests/Responses/product-tags-extra.json @@ -5,7 +5,7 @@ "name": "Leather Shoes 2", "slug": "leather-shoes-2", "description": "", - "count": 0, + "count": 1, "_links": { "self": [ { From ad897f44abb4408fcde922f4a680508f17bbc8d5 Mon Sep 17 00:00:00 2001 From: Huong Do Date: Tue, 26 Nov 2024 17:05:59 +0700 Subject: [PATCH 11/22] Add comment explaining the `count` property for ProductTag --- Networking/Networking/Model/Product/ProductTag.swift | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Networking/Networking/Model/Product/ProductTag.swift b/Networking/Networking/Model/Product/ProductTag.swift index 85f492e8046..fece46cbb80 100644 --- a/Networking/Networking/Model/Product/ProductTag.swift +++ b/Networking/Networking/Model/Product/ProductTag.swift @@ -8,6 +8,9 @@ public struct ProductTag: Codable, Equatable, GeneratedFakeable, GeneratedCopiab public let tagID: Int64 public let name: String public let slug: String + + /// The number of products associated with this tag - available only when syncing tags from `wc/v3/products/tags`. + /// This is default to 0 and ignored when fetched from `wc/v3/products` public let count: Int /// ProductTag initializer. From 9a4c9adf35a9f43ef97d09d33a1749c45d7cf26b Mon Sep 17 00:00:00 2001 From: Huong Do Date: Tue, 26 Nov 2024 17:12:25 +0700 Subject: [PATCH 12/22] Update unit tests for ProductTagStore --- .../Responses/product-tags-all-without-data.json | 2 +- .../Responses/product-tags-all.json | 2 +- .../Stores/ProductTagStoreTests.swift | 14 +++++++------- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/Networking/NetworkingTests/Responses/product-tags-all-without-data.json b/Networking/NetworkingTests/Responses/product-tags-all-without-data.json index 62ae6ff32cb..b9e2f08714c 100644 --- a/Networking/NetworkingTests/Responses/product-tags-all-without-data.json +++ b/Networking/NetworkingTests/Responses/product-tags-all-without-data.json @@ -4,7 +4,7 @@ "name": "Leather Shoes", "slug": "leather-shoes", "description": "", - "count": 1, + "count": 0, "_links": { "self": [ { diff --git a/Networking/NetworkingTests/Responses/product-tags-all.json b/Networking/NetworkingTests/Responses/product-tags-all.json index c7921f09afc..50c270e2826 100644 --- a/Networking/NetworkingTests/Responses/product-tags-all.json +++ b/Networking/NetworkingTests/Responses/product-tags-all.json @@ -5,7 +5,7 @@ "name": "Leather Shoes", "slug": "leather-shoes", "description": "", - "count": 1, + "count": 0, "_links": { "self": [ { diff --git a/Yosemite/YosemiteTests/Stores/ProductTagStoreTests.swift b/Yosemite/YosemiteTests/Stores/ProductTagStoreTests.swift index 1feae44bb7c..c7f07f85288 100644 --- a/Yosemite/YosemiteTests/Stores/ProductTagStoreTests.swift +++ b/Yosemite/YosemiteTests/Stores/ProductTagStoreTests.swift @@ -58,7 +58,7 @@ final class ProductTagStoreTests: XCTestCase { super.tearDown() } - func testSynchronizeProductTagsReturnsTagsUponSuccessfulResponse() throws { + func test_synchronizeAllProductTags_saves_all_tags_with_count_larger_than_0_to_storage() throws { // Given a stubed product-tags network response network.simulateResponse(requestUrlSuffix: "products/tags", filename: "product-tags-all") network.simulateResponse(requestUrlSuffix: "products/tags", filename: "product-tags-empty") @@ -74,8 +74,8 @@ final class ProductTagStoreTests: XCTestCase { store.onAction(action) } - // Then a valid set of tags should be stored - XCTAssertEqual(storedProductTagsCount, 4) + // Then a valid set of tags (count > 0) should be stored + XCTAssertEqual(storedProductTagsCount, 3) XCTAssertNil(errorResponse) } @@ -97,13 +97,13 @@ final class ProductTagStoreTests: XCTestCase { } // Then the combined set of tags should be stored - XCTAssertEqual(storedProductTagsCount, 5) + XCTAssertEqual(storedProductTagsCount, 4) XCTAssertNil(errorResponse) } func testSynchronizeProductTagsUpdatesStoredTagsSuccessfulResponse() { // Given an initial stored tag and a stubed product-tags network response - let initialTag = sampleTag(tagID: 34) + let initialTag = sampleTag(tagID: 35) storageManager.insertSampleProductTag(readOnlyProductTag: initialTag) network.simulateResponse(requestUrlSuffix: "products/tags", filename: "product-tags-all") network.simulateResponse(requestUrlSuffix: "products/tags", filename: "product-tags-empty") @@ -144,7 +144,7 @@ final class ProductTagStoreTests: XCTestCase { } // Then first page of tags should be stored - XCTAssertEqual(storedProductTagsCount, 4) + XCTAssertEqual(storedProductTagsCount, 3) // And error should contain correct fromPageNumber switch errorResponse { @@ -300,7 +300,7 @@ final class ProductTagStoreTests: XCTestCase { } // Then new tag should be stored and old tags should be deleted - XCTAssertEqual(storedProductTagsCount, 4) + XCTAssertEqual(storedProductTagsCount, 3) XCTAssertNil(errorResponse) } From 7c6fd0a7d1a59f97e0f581d8353165d5b614f900 Mon Sep 17 00:00:00 2001 From: Huong Do Date: Tue, 26 Nov 2024 17:17:13 +0700 Subject: [PATCH 13/22] Remove count property from tag initializers --- Fakes/Fakes/Products/ProductFactory.swift | 2 +- Networking/Networking/Mapper/ProductTagListMapper.swift | 2 +- .../Model/Storage/ProductTag+ReadOnlyConvertible.swift | 3 +-- Yosemite/YosemiteTests/Stores/ProductTagStoreTests.swift | 2 +- 4 files changed, 4 insertions(+), 5 deletions(-) diff --git a/Fakes/Fakes/Products/ProductFactory.swift b/Fakes/Fakes/Products/ProductFactory.swift index e039aaa2379..51ee86ddd1c 100644 --- a/Fakes/Fakes/Products/ProductFactory.swift +++ b/Fakes/Fakes/Products/ProductFactory.swift @@ -66,6 +66,6 @@ public enum ProductFactory { upsellIDs: [1, 2], crossSellIDs: [3, 4], categories: [ProductCategory(categoryID: 1, siteID: 2, parentID: 6, name: "", slug: "")], - tags: [ProductTag(siteID: 123, tagID: 1, name: "", slug: "", count: 0)]) + tags: [ProductTag(siteID: 123, tagID: 1, name: "", slug: "")]) } } diff --git a/Networking/Networking/Mapper/ProductTagListMapper.swift b/Networking/Networking/Mapper/ProductTagListMapper.swift index 243f5c1588b..359f164c4d2 100644 --- a/Networking/Networking/Mapper/ProductTagListMapper.swift +++ b/Networking/Networking/Mapper/ProductTagListMapper.swift @@ -39,7 +39,7 @@ struct ProductTagListMapper: Mapper { .filter { $0.error == nil } .compactMap { (tagCreated) -> ProductTag? in if let name = tagCreated.name, let slug = tagCreated.slug { - return ProductTag(siteID: tagCreated.siteID, tagID: tagCreated.tagID, name: name, slug: slug, count: 0) + return ProductTag(siteID: tagCreated.siteID, tagID: tagCreated.tagID, name: name, slug: slug) } return nil } diff --git a/Yosemite/Yosemite/Model/Storage/ProductTag+ReadOnlyConvertible.swift b/Yosemite/Yosemite/Model/Storage/ProductTag+ReadOnlyConvertible.swift index ee3911b3aa8..e7e33c3be17 100644 --- a/Yosemite/Yosemite/Model/Storage/ProductTag+ReadOnlyConvertible.swift +++ b/Yosemite/Yosemite/Model/Storage/ProductTag+ReadOnlyConvertible.swift @@ -21,7 +21,6 @@ extension Storage.ProductTag: ReadOnlyConvertible { return ProductTag(siteID: siteID, tagID: tagID, name: name, - slug: slug, - count: 0) + slug: slug) } } diff --git a/Yosemite/YosemiteTests/Stores/ProductTagStoreTests.swift b/Yosemite/YosemiteTests/Stores/ProductTagStoreTests.swift index c7f07f85288..38cc780fce3 100644 --- a/Yosemite/YosemiteTests/Stores/ProductTagStoreTests.swift +++ b/Yosemite/YosemiteTests/Stores/ProductTagStoreTests.swift @@ -370,6 +370,6 @@ final class ProductTagStoreTests: XCTestCase { private extension ProductTagStoreTests { func sampleTag(tagID: Int64) -> Networking.ProductTag { - return Networking.ProductTag(siteID: sampleSiteID, tagID: tagID, name: "Sample", slug: "sample", count: 0) + return Networking.ProductTag(siteID: sampleSiteID, tagID: tagID, name: "Sample", slug: "sample") } } From 2afea6914ec51b7d9a47320ab193390f25283280 Mon Sep 17 00:00:00 2001 From: Huong Do Date: Tue, 26 Nov 2024 17:23:06 +0700 Subject: [PATCH 14/22] Update release notes --- RELEASE-NOTES.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/RELEASE-NOTES.txt b/RELEASE-NOTES.txt index e3724ee6f82..a9c2eb1b43d 100644 --- a/RELEASE-NOTES.txt +++ b/RELEASE-NOTES.txt @@ -4,6 +4,7 @@ ----- - [*] Jetpack Setup: Fixed an issue where the WordPress.com authentication fails when using a passwordless account that's already connected to Jetpack [https://github.com/woocommerce/woocommerce-ios/pull/14501] - [*] Jetpack Setup: Fixed an issue with magic link handling when the app is cold started. [https://github.com/woocommerce/woocommerce-ios/pull/14502] +- [*] Product Tags: Fixed issue clearing all tags and hid unused tags from suggested list. [https://github.com/woocommerce/woocommerce-ios/pull/14511] 21.2 ----- From c6b7febfceb16f5fe427069e30e07bd60d5f729d Mon Sep 17 00:00:00 2001 From: Huong Do Date: Tue, 26 Nov 2024 17:38:26 +0700 Subject: [PATCH 15/22] Fetch all items in one go to avoid multiple fetches --- Yosemite/Yosemite/Stores/ProductTagStore.swift | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Yosemite/Yosemite/Stores/ProductTagStore.swift b/Yosemite/Yosemite/Stores/ProductTagStore.swift index 20164a795a5..c83395870ff 100644 --- a/Yosemite/Yosemite/Stores/ProductTagStore.swift +++ b/Yosemite/Yosemite/Stores/ProductTagStore.swift @@ -163,10 +163,11 @@ private extension ProductTagStore { func upsertStoredProductTags(_ readOnlyProductTags: [Networking.ProductTag], in storage: StorageType, siteID: Int64) { + let storedItems = storage.loadProductTags(siteID: siteID) // Upserts the ProductTag models from the read-only version for readOnlyProductTag in readOnlyProductTags { let storageProductTag: Storage.ProductTag = { - if let storedTag = storage.loadProductTag(siteID: siteID, tagID: readOnlyProductTag.tagID) { + if let storedTag = storedItems.first(where: { $0.tagID == readOnlyProductTag.tagID }) { return storedTag } return storage.insertNewObject(ofType: Storage.ProductTag.self) From e5f3ab2631994efcceca36ebf00397bd71e33e8a Mon Sep 17 00:00:00 2001 From: Huong Do Date: Thu, 28 Nov 2024 17:25:17 +0700 Subject: [PATCH 16/22] Revert change to synchronizeProductTags method --- .../Yosemite/Stores/ProductTagStore.swift | 13 ++----- .../Stores/ProductTagStoreTests.swift | 34 ++----------------- 2 files changed, 5 insertions(+), 42 deletions(-) diff --git a/Yosemite/Yosemite/Stores/ProductTagStore.swift b/Yosemite/Yosemite/Stores/ProductTagStore.swift index c83395870ff..b3356dce8ee 100644 --- a/Yosemite/Yosemite/Stores/ProductTagStore.swift +++ b/Yosemite/Yosemite/Stores/ProductTagStore.swift @@ -76,12 +76,7 @@ private extension ProductTagStore { switch result { case .success(let productTags): - let shouldRemoveUnusedTags = pageNumber == Default.firstPageNumber - /// Only upsert tags that are associated with at least 1 product. - let filteredTags = productTags.filter { $0.count > 0 } - self?.upsertStoredProductTagsInBackground(filteredTags, - siteID: siteID, - shouldRemoveUnusedTags: shouldRemoveUnusedTags) { + self?.upsertStoredProductTagsInBackground(productTags, siteID: siteID) { onCompletion(.success(productTags)) } case .failure(let error): @@ -99,7 +94,7 @@ private extension ProductTagStore { remote.createProductTags(for: siteID, names: tags) { [weak self] (result) in switch result { case .success(let productTags): - self?.upsertStoredProductTagsInBackground(productTags, siteID: siteID, shouldRemoveUnusedTags: false) { + self?.upsertStoredProductTagsInBackground(productTags, siteID: siteID) { onCompletion(.success(productTags)) } case .failure(let error): @@ -132,12 +127,8 @@ private extension ProductTagStore { /// func upsertStoredProductTagsInBackground(_ readOnlyProductTags: [Networking.ProductTag], siteID: Int64, - shouldRemoveUnusedTags: Bool, onCompletion: @escaping () -> Void) { storageManager.performAndSave({ [weak self] storage in - if shouldRemoveUnusedTags { - storage.deleteUnusedProductTags(siteID: siteID) - } self?.upsertStoredProductTags(readOnlyProductTags, in: storage, siteID: siteID) }, completion: onCompletion, on: .main) } diff --git a/Yosemite/YosemiteTests/Stores/ProductTagStoreTests.swift b/Yosemite/YosemiteTests/Stores/ProductTagStoreTests.swift index 38cc780fce3..0fa812c1a82 100644 --- a/Yosemite/YosemiteTests/Stores/ProductTagStoreTests.swift +++ b/Yosemite/YosemiteTests/Stores/ProductTagStoreTests.swift @@ -75,7 +75,7 @@ final class ProductTagStoreTests: XCTestCase { } // Then a valid set of tags (count > 0) should be stored - XCTAssertEqual(storedProductTagsCount, 3) + XCTAssertEqual(storedProductTagsCount, 4) XCTAssertNil(errorResponse) } @@ -97,7 +97,7 @@ final class ProductTagStoreTests: XCTestCase { } // Then the combined set of tags should be stored - XCTAssertEqual(storedProductTagsCount, 4) + XCTAssertEqual(storedProductTagsCount, 5) XCTAssertNil(errorResponse) } @@ -144,7 +144,7 @@ final class ProductTagStoreTests: XCTestCase { } // Then first page of tags should be stored - XCTAssertEqual(storedProductTagsCount, 3) + XCTAssertEqual(storedProductTagsCount, 4) // And error should contain correct fromPageNumber switch errorResponse { @@ -276,34 +276,6 @@ final class ProductTagStoreTests: XCTestCase { XCTAssertNotNil(result?.failure) } - func testSynchronizeProductTagsDeletesUnusedTags() { - // Given some stored product tags without product relationships - let sampleTags = (1...5).map { id in - return sampleTag(tagID: id) - } - sampleTags.forEach { tag in - storageManager.insertSampleProductTag(readOnlyProductTag: tag) - } - XCTAssertEqual(storedProductTagsCount, sampleTags.count) - - // When dispatching a `synchronizeAllProductTags` action - network.simulateResponse(requestUrlSuffix: "products/tags", filename: "product-tags-all") - network.simulateResponse(requestUrlSuffix: "products/tags", filename: "product-tags-empty") - - var errorResponse: ProductTagActionError? - waitForExpectation { (exp) in - let action = ProductTagAction.synchronizeAllProductTags(siteID: sampleSiteID) { error in - errorResponse = error - exp.fulfill() - } - store.onAction(action) - } - - // Then new tag should be stored and old tags should be deleted - XCTAssertEqual(storedProductTagsCount, 3) - XCTAssertNil(errorResponse) - } - func testDeleteProductTagDeleteStoredTagSuccessfulResponse() { // Given a stubed product tag network response and a product tag stored locally network.simulateResponse(requestUrlSuffix: "products/tags/batch", filename: "product-tags-deleted") From c1010ccb15ff45a286e12e51b7d6d3f767c2e6b3 Mon Sep 17 00:00:00 2001 From: Huong Do Date: Thu, 28 Nov 2024 17:31:06 +0700 Subject: [PATCH 17/22] Revert addition of count property --- Fakes/Fakes/Networking.generated.swift | 3 +-- .../Model/Copiable/Models+Copiable.generated.swift | 7 ++----- .../Networking/Model/Product/ProductTag.swift | 13 ++----------- .../Mapper/ProductTagListMapperTests.swift | 2 -- .../Responses/product-tags-all-without-data.json | 6 +++--- .../NetworkingTests/Responses/product-tags-all.json | 6 +++--- .../Responses/product-tags-extra.json | 2 +- RELEASE-NOTES.txt | 2 +- .../YosemiteTests/Stores/ProductTagStoreTests.swift | 2 +- 9 files changed, 14 insertions(+), 29 deletions(-) diff --git a/Fakes/Fakes/Networking.generated.swift b/Fakes/Fakes/Networking.generated.swift index 4d19e8a913b..a7a2d3b8b25 100644 --- a/Fakes/Fakes/Networking.generated.swift +++ b/Fakes/Fakes/Networking.generated.swift @@ -1628,8 +1628,7 @@ extension Networking.ProductTag { siteID: .fake(), tagID: .fake(), name: .fake(), - slug: .fake(), - count: .fake() + slug: .fake() ) } } diff --git a/Networking/Networking/Model/Copiable/Models+Copiable.generated.swift b/Networking/Networking/Model/Copiable/Models+Copiable.generated.swift index 930b35d7acb..c7ed46b6838 100644 --- a/Networking/Networking/Model/Copiable/Models+Copiable.generated.swift +++ b/Networking/Networking/Model/Copiable/Models+Copiable.generated.swift @@ -2634,21 +2634,18 @@ extension Networking.ProductTag { siteID: CopiableProp = .copy, tagID: CopiableProp = .copy, name: CopiableProp = .copy, - slug: CopiableProp = .copy, - count: CopiableProp = .copy + slug: CopiableProp = .copy ) -> Networking.ProductTag { let siteID = siteID ?? self.siteID let tagID = tagID ?? self.tagID let name = name ?? self.name let slug = slug ?? self.slug - let count = count ?? self.count return Networking.ProductTag( siteID: siteID, tagID: tagID, name: name, - slug: slug, - count: count + slug: slug ) } } diff --git a/Networking/Networking/Model/Product/ProductTag.swift b/Networking/Networking/Model/Product/ProductTag.swift index fece46cbb80..2b0bea1aa1a 100644 --- a/Networking/Networking/Model/Product/ProductTag.swift +++ b/Networking/Networking/Model/Product/ProductTag.swift @@ -9,22 +9,16 @@ public struct ProductTag: Codable, Equatable, GeneratedFakeable, GeneratedCopiab public let name: String public let slug: String - /// The number of products associated with this tag - available only when syncing tags from `wc/v3/products/tags`. - /// This is default to 0 and ignored when fetched from `wc/v3/products` - public let count: Int - /// ProductTag initializer. /// public init(siteID: Int64, tagID: Int64, name: String, - slug: String, - count: Int = 0) { + slug: String) { self.siteID = siteID self.tagID = tagID self.name = name self.slug = slug - self.count = count } /// Public initializer for ProductTag. @@ -39,10 +33,8 @@ public struct ProductTag: Codable, Equatable, GeneratedFakeable, GeneratedCopiab let tagID = try container.decode(Int64.self, forKey: .tagID) let name = try container.decode(String.self, forKey: .name) let slug = try container.decode(String.self, forKey: .slug) - /// `count` is available when fetching from `wc/v3/products/tags` but not from `wc/v3/products` - let count = (try container.decodeIfPresent(Int.self, forKey: .count)) ?? 0 - self.init(siteID: siteID, tagID: tagID, name: name, slug: slug, count: count) + self.init(siteID: siteID, tagID: tagID, name: name, slug: slug) } public func encode(to encoder: Encoder) throws { @@ -62,7 +54,6 @@ private extension ProductTag { case tagID = "id" case name = "name" case slug = "slug" - case count = "count" } } diff --git a/Networking/NetworkingTests/Mapper/ProductTagListMapperTests.swift b/Networking/NetworkingTests/Mapper/ProductTagListMapperTests.swift index 419ceb89895..825f442c98b 100644 --- a/Networking/NetworkingTests/Mapper/ProductTagListMapperTests.swift +++ b/Networking/NetworkingTests/Mapper/ProductTagListMapperTests.swift @@ -16,7 +16,6 @@ final class ProductTagListMapperTests: XCTestCase { XCTAssertEqual(secondTag.tagID, 35) XCTAssertEqual(secondTag.name, "Oxford Shoes") XCTAssertEqual(secondTag.slug, "oxford-shoes") - XCTAssertEqual(secondTag.count, 2) } /// Verifies that all of the ProductTag Fields are parsed correctly. @@ -29,7 +28,6 @@ final class ProductTagListMapperTests: XCTestCase { XCTAssertEqual(secondTag.tagID, 35) XCTAssertEqual(secondTag.name, "Oxford Shoes") XCTAssertEqual(secondTag.slug, "oxford-shoes") - XCTAssertEqual(secondTag.count, 2) } /// Verifies that all of the ProductTag Fields under `create` field are parsed correctly. diff --git a/Networking/NetworkingTests/Responses/product-tags-all-without-data.json b/Networking/NetworkingTests/Responses/product-tags-all-without-data.json index b9e2f08714c..6728510d597 100644 --- a/Networking/NetworkingTests/Responses/product-tags-all-without-data.json +++ b/Networking/NetworkingTests/Responses/product-tags-all-without-data.json @@ -23,7 +23,7 @@ "name": "Oxford Shoes", "slug": "oxford-shoes", "description": "", - "count": 2, + "count": 0, "_links": { "self": [ { @@ -42,7 +42,7 @@ "name": "Leather Boots", "slug": "leather-boots", "description": "", - "count": 3, + "count": 0, "_links": { "self": [ { @@ -61,7 +61,7 @@ "name": "Sneakers", "slug": "sneakers", "description": "A shoe suitable for playing sports", - "count": 4, + "count": 0, "_links": { "self": [ { diff --git a/Networking/NetworkingTests/Responses/product-tags-all.json b/Networking/NetworkingTests/Responses/product-tags-all.json index 50c270e2826..f361d2b7307 100644 --- a/Networking/NetworkingTests/Responses/product-tags-all.json +++ b/Networking/NetworkingTests/Responses/product-tags-all.json @@ -24,7 +24,7 @@ "name": "Oxford Shoes", "slug": "oxford-shoes", "description": "", - "count": 2, + "count": 0, "_links": { "self": [ { @@ -43,7 +43,7 @@ "name": "Leather Boots", "slug": "leather-boots", "description": "", - "count": 3, + "count": 0, "_links": { "self": [ { @@ -62,7 +62,7 @@ "name": "Sneakers", "slug": "sneakers", "description": "A shoe suitable for playing sports", - "count": 4, + "count": 0, "_links": { "self": [ { diff --git a/Networking/NetworkingTests/Responses/product-tags-extra.json b/Networking/NetworkingTests/Responses/product-tags-extra.json index 3ca60a45343..2e83a0e2572 100644 --- a/Networking/NetworkingTests/Responses/product-tags-extra.json +++ b/Networking/NetworkingTests/Responses/product-tags-extra.json @@ -5,7 +5,7 @@ "name": "Leather Shoes 2", "slug": "leather-shoes-2", "description": "", - "count": 1, + "count": 0, "_links": { "self": [ { diff --git a/RELEASE-NOTES.txt b/RELEASE-NOTES.txt index e2c37ab0a49..0e340aa7d30 100644 --- a/RELEASE-NOTES.txt +++ b/RELEASE-NOTES.txt @@ -4,7 +4,7 @@ ----- - [*] Jetpack Setup: Fixed an issue where the WordPress.com authentication fails when using a passwordless account that's already connected to Jetpack [https://github.com/woocommerce/woocommerce-ios/pull/14501] - [*] Jetpack Setup: Fixed an issue with magic link handling when the app is cold started. [https://github.com/woocommerce/woocommerce-ios/pull/14502] -- [*] Product Tags: Fixed issue clearing all tags and hid unused tags from suggested list. [https://github.com/woocommerce/woocommerce-ios/pull/14511] +- [*] Product Tags: Fixed issue clearing all tags [https://github.com/woocommerce/woocommerce-ios/pull/14511] - [**] Media Library: On sites logged in with application password, when picking image from WordPress Media Library, all images will now load correctly. [https://github.com/woocommerce/woocommerce-ios/pull/14444] 21.2 diff --git a/Yosemite/YosemiteTests/Stores/ProductTagStoreTests.swift b/Yosemite/YosemiteTests/Stores/ProductTagStoreTests.swift index 0fa812c1a82..e33ee07eff9 100644 --- a/Yosemite/YosemiteTests/Stores/ProductTagStoreTests.swift +++ b/Yosemite/YosemiteTests/Stores/ProductTagStoreTests.swift @@ -74,7 +74,7 @@ final class ProductTagStoreTests: XCTestCase { store.onAction(action) } - // Then a valid set of tags (count > 0) should be stored + // Then a valid set of tags should be stored XCTAssertEqual(storedProductTagsCount, 4) XCTAssertNil(errorResponse) } From 2b06eb87ec0589744c35ab799f4313e7e9d0fea2 Mon Sep 17 00:00:00 2001 From: Huong Do Date: Thu, 28 Nov 2024 17:35:01 +0700 Subject: [PATCH 18/22] Revert changes to unit tests --- Yosemite/YosemiteTests/Stores/ProductTagStoreTests.swift | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Yosemite/YosemiteTests/Stores/ProductTagStoreTests.swift b/Yosemite/YosemiteTests/Stores/ProductTagStoreTests.swift index e33ee07eff9..d04981c9f1b 100644 --- a/Yosemite/YosemiteTests/Stores/ProductTagStoreTests.swift +++ b/Yosemite/YosemiteTests/Stores/ProductTagStoreTests.swift @@ -58,7 +58,7 @@ final class ProductTagStoreTests: XCTestCase { super.tearDown() } - func test_synchronizeAllProductTags_saves_all_tags_with_count_larger_than_0_to_storage() throws { + func test_synchronizeAllProductTags_saves_all_tags_to_storage() throws { // Given a stubed product-tags network response network.simulateResponse(requestUrlSuffix: "products/tags", filename: "product-tags-all") network.simulateResponse(requestUrlSuffix: "products/tags", filename: "product-tags-empty") @@ -103,7 +103,7 @@ final class ProductTagStoreTests: XCTestCase { func testSynchronizeProductTagsUpdatesStoredTagsSuccessfulResponse() { // Given an initial stored tag and a stubed product-tags network response - let initialTag = sampleTag(tagID: 35) + let initialTag = sampleTag(tagID: 34) storageManager.insertSampleProductTag(readOnlyProductTag: initialTag) network.simulateResponse(requestUrlSuffix: "products/tags", filename: "product-tags-all") network.simulateResponse(requestUrlSuffix: "products/tags", filename: "product-tags-empty") From 31008d341b40ecd3dafd29b89584edc328c035f9 Mon Sep 17 00:00:00 2001 From: Huong Do Date: Fri, 29 Nov 2024 10:29:55 +0700 Subject: [PATCH 19/22] Update comment on deleteStoredProductTags --- Yosemite/Yosemite/Stores/ProductTagStore.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Yosemite/Yosemite/Stores/ProductTagStore.swift b/Yosemite/Yosemite/Stores/ProductTagStore.swift index b3356dce8ee..87a076d1c93 100644 --- a/Yosemite/Yosemite/Stores/ProductTagStore.swift +++ b/Yosemite/Yosemite/Stores/ProductTagStore.swift @@ -133,7 +133,7 @@ private extension ProductTagStore { }, completion: onCompletion, on: .main) } - /// Deletes any Storage.ProductTag with the specified `siteID` and `productID` + /// Deletes any Storage.ProductTag with the specified `siteID` and `tagID`'s /// func deleteStoredProductTags(siteID: Int64, ids: [Int64], onCompletion: @escaping () -> Void) { storageManager.performAndSave({ storage in From cd4946a3efcf76f846f6c7cb782120a1179d4dbe Mon Sep 17 00:00:00 2001 From: Huong Do Date: Fri, 29 Nov 2024 10:48:25 +0700 Subject: [PATCH 20/22] Display cached tags immediately upon loading tags --- .../Edit Tags/ProductTagsViewController.swift | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/WooCommerce/Classes/ViewRelated/Products/Edit Product/Edit Tags/ProductTagsViewController.swift b/WooCommerce/Classes/ViewRelated/Products/Edit Product/Edit Tags/ProductTagsViewController.swift index 9cc27f67fe1..a34149ea258 100644 --- a/WooCommerce/Classes/ViewRelated/Products/Edit Product/Edit Tags/ProductTagsViewController.swift +++ b/WooCommerce/Classes/ViewRelated/Products/Edit Product/Edit Tags/ProductTagsViewController.swift @@ -207,7 +207,14 @@ extension ProductTagsViewController: KeyboardScrollable { // private extension ProductTagsViewController { func loadTags() { - displayGhostContent(over: tableView) + let cachedTags = self.fetchedTags + if cachedTags.isEmpty { + /// Display the loading state only cached items are unavailable. + displayGhostContent(over: tableView) + } else { + /// Otherwise, display cached items right away. + tagsLoaded(tags: cachedTags.map { $0.name } ) + } let action = ProductTagAction.synchronizeAllProductTags(siteID: product.siteID) { [weak self] error in if let error = error { From c77d166c09054466c27e2937c0b708024be077d2 Mon Sep 17 00:00:00 2001 From: Huong Do Date: Fri, 29 Nov 2024 10:49:00 +0700 Subject: [PATCH 21/22] Only trigger tag creation if there are any tags confirmed --- .../Edit Tags/ProductTagsViewController.swift | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/WooCommerce/Classes/ViewRelated/Products/Edit Product/Edit Tags/ProductTagsViewController.swift b/WooCommerce/Classes/ViewRelated/Products/Edit Product/Edit Tags/ProductTagsViewController.swift index a34149ea258..8e14513ea41 100644 --- a/WooCommerce/Classes/ViewRelated/Products/Edit Product/Edit Tags/ProductTagsViewController.swift +++ b/WooCommerce/Classes/ViewRelated/Products/Edit Product/Edit Tags/ProductTagsViewController.swift @@ -105,7 +105,7 @@ private extension ProductTagsViewController { navigationItem.setRightBarButton(UIBarButtonItem(title: Strings.saveButton, style: .done, target: self, - action: #selector(addTagsRemotely)), + action: #selector(confirmTags)), animated: true) navigationItem.rightBarButtonItem?.isEnabled = true } @@ -239,12 +239,21 @@ private extension ProductTagsViewController { searchQuery: partialTag) } - @objc func addTagsRemotely() { + @objc func confirmTags() { ServiceLocator.analytics.track(.productTagSettingsDoneButtonTapped, withProperties: [ "has_changed_data": hasUnsavedChanges() ]) textView.resignFirstResponder() + + guard allTags.isNotEmpty else { + return onCompletion([]) + } + + createNewTagsRemotely() + } + + func createNewTagsRemotely() { configureRightBarButtonItemAsSpinner() let action = ProductTagAction.addProductTags(siteID: product.siteID, tags: allTags) { [weak self] (result) in From 8cd948a7fa0d09a4a5713d5fb30584a1e366d043 Mon Sep 17 00:00:00 2001 From: Huong Do Date: Fri, 29 Nov 2024 10:53:43 +0700 Subject: [PATCH 22/22] Update release notes --- RELEASE-NOTES.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/RELEASE-NOTES.txt b/RELEASE-NOTES.txt index 0e340aa7d30..b0aea31671d 100644 --- a/RELEASE-NOTES.txt +++ b/RELEASE-NOTES.txt @@ -4,7 +4,7 @@ ----- - [*] Jetpack Setup: Fixed an issue where the WordPress.com authentication fails when using a passwordless account that's already connected to Jetpack [https://github.com/woocommerce/woocommerce-ios/pull/14501] - [*] Jetpack Setup: Fixed an issue with magic link handling when the app is cold started. [https://github.com/woocommerce/woocommerce-ios/pull/14502] -- [*] Product Tags: Fixed issue clearing all tags [https://github.com/woocommerce/woocommerce-ios/pull/14511] +- [*] Product Tags: Improved the tag list screen by displaying the cached items on first load and fixed issue clearing all tags [https://github.com/woocommerce/woocommerce-ios/pull/14511] - [**] Media Library: On sites logged in with application password, when picking image from WordPress Media Library, all images will now load correctly. [https://github.com/woocommerce/woocommerce-ios/pull/14444] 21.2