From 5d63ab4262d2f26bf776d74129271b0653dc12c3 Mon Sep 17 00:00:00 2001 From: Hafiz Rahman Date: Tue, 24 Sep 2024 21:38:11 +0700 Subject: [PATCH 01/10] Add view model for custom fields list. Contains logic for locally editing custom fields while keeping track of two main arrays, editedFields and addedFields. --- .../CustomFieldsListViewModel.swift | 92 +++++++++++++++++++ .../WooCommerce.xcodeproj/project.pbxproj | 4 + 2 files changed, 96 insertions(+) create mode 100644 WooCommerce/Classes/ViewRelated/Custom Fields/CustomFieldsListViewModel.swift diff --git a/WooCommerce/Classes/ViewRelated/Custom Fields/CustomFieldsListViewModel.swift b/WooCommerce/Classes/ViewRelated/Custom Fields/CustomFieldsListViewModel.swift new file mode 100644 index 00000000000..0a4bfcdb33b --- /dev/null +++ b/WooCommerce/Classes/ViewRelated/Custom Fields/CustomFieldsListViewModel.swift @@ -0,0 +1,92 @@ +import Foundation + +final class CustomFieldsListViewModel: ObservableObject { + private let originalCustomFields: [CustomFieldViewModel] + + var shouldShowErrorState: Bool { + savingError != nil + } + + @Published private(set) var savingError: Error? + @Published private(set) var combinedList: [CustomFieldUI] = [] + + @Published private var editedFields: [CustomFieldUI] = [] + @Published private var addedFields: [CustomFieldUI] = [] + var hasChanges: Bool { + !editedFields.isEmpty || !addedFields.isEmpty + } + + init(customFields: [CustomFieldViewModel]) { + self.originalCustomFields = customFields + updateCombinedList() + } +} + +// MARK: - Items actions +extension CustomFieldsListViewModel { + func editField(at index: Int, newField: CustomFieldUI) { + let oldField = combinedList[index] + + if newField.id == nil { + // If there's no id, it means we're now editing a newly added field. + if let addedIndex = addedFields.firstIndex(where: { $0.key == oldField.key }) { + if newField.key == oldField.key { + // If the key hasn't changed, update the existing added field + addedFields[addedIndex] = newField + } else { + // If the key has changed, remove the old field and add the new one + addedFields.remove(at: addedIndex) + addedFields.append(newField) + } + } else { + // This case should not happen in normal flow + DDLogError("⛔️ Error: Editing a newly updated field that doesn't exist in combinedList") + + } + } else { + // For when editing an already edited field + if let editedIndex = editedFields.firstIndex(where: { $0.id == oldField.id }) { + editedFields[editedIndex] = newField + } else { + // For the first time a field is edited + editedFields.append(newField) + } + } + + updateCombinedList() + } + + func addField(_ field: CustomFieldUI) { + addedFields.append(field) + updateCombinedList() + } +} + +private extension CustomFieldsListViewModel { + func updateCombinedList() { + let editedList = originalCustomFields.map { field in + editedFields.first { $0.id == field.id } ?? CustomFieldUI(customField: field) + } + combinedList = editedList + addedFields + } +} + +extension CustomFieldsListViewModel { + struct CustomFieldUI: Identifiable { + let key: String + let value: String + let id: Int64? + + init(key: String, value: String, id: Int64? = nil) { + self.key = key + self.value = value + self.id = id + } + + init(customField: CustomFieldViewModel) { + self.key = customField.title + self.value = customField.content + self.id = customField.id + } + } +} diff --git a/WooCommerce/WooCommerce.xcodeproj/project.pbxproj b/WooCommerce/WooCommerce.xcodeproj/project.pbxproj index b6553d9232f..db0554df894 100644 --- a/WooCommerce/WooCommerce.xcodeproj/project.pbxproj +++ b/WooCommerce/WooCommerce.xcodeproj/project.pbxproj @@ -1712,6 +1712,7 @@ 86DE68822B4BA47A00B437A6 /* BlazeAdDestinationSettingViewModel.swift in Sources */ = {isa = PBXBuildFile; fileRef = 86DE68812B4BA47900B437A6 /* BlazeAdDestinationSettingViewModel.swift */; }; 86E40AED2B597DEC00990365 /* BlazeCampaignCreationCoordinatorTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 86E40AEC2B597DEC00990365 /* BlazeCampaignCreationCoordinatorTests.swift */; }; 86F0896F2B307D7E00D668A1 /* ThemesPreviewViewModelTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 86F0896E2B307D7E00D668A1 /* ThemesPreviewViewModelTests.swift */; }; + 86F5FFE22CA302B300C767C4 /* CustomFieldsListViewModel.swift in Sources */ = {isa = PBXBuildFile; fileRef = 86F5FFE12CA302B300C767C4 /* CustomFieldsListViewModel.swift */; }; 86F9D3642C897FFE00B1835B /* CustomFieldEditorView.swift in Sources */ = {isa = PBXBuildFile; fileRef = 86F9D3632C897FFE00B1835B /* CustomFieldEditorView.swift */; }; 8CD41D4A21F8A7E300CF3C2B /* RELEASE-NOTES.txt in Resources */ = {isa = PBXBuildFile; fileRef = 8CD41D4921F8A7E300CF3C2B /* RELEASE-NOTES.txt */; }; 933A27372222354600C2143A /* Logging.swift in Sources */ = {isa = PBXBuildFile; fileRef = 933A27362222354600C2143A /* Logging.swift */; }; @@ -4736,6 +4737,7 @@ 86DE68812B4BA47900B437A6 /* BlazeAdDestinationSettingViewModel.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = BlazeAdDestinationSettingViewModel.swift; sourceTree = ""; }; 86E40AEC2B597DEC00990365 /* BlazeCampaignCreationCoordinatorTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = BlazeCampaignCreationCoordinatorTests.swift; sourceTree = ""; }; 86F0896E2B307D7E00D668A1 /* ThemesPreviewViewModelTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = ThemesPreviewViewModelTests.swift; sourceTree = ""; }; + 86F5FFE12CA302B300C767C4 /* CustomFieldsListViewModel.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = CustomFieldsListViewModel.swift; sourceTree = ""; }; 86F9D3632C897FFE00B1835B /* CustomFieldEditorView.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = CustomFieldEditorView.swift; sourceTree = ""; }; 8A659E65308A3D9DD79A95F9 /* Pods-WooCommerceTests.release.xcconfig */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = text.xcconfig; name = "Pods-WooCommerceTests.release.xcconfig"; path = "../Pods/Target Support Files/Pods-WooCommerceTests/Pods-WooCommerceTests.release.xcconfig"; sourceTree = ""; }; 8CA4F6DD220B257000A47B5D /* WooCommerce.debug.xcconfig */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text.xcconfig; name = WooCommerce.debug.xcconfig; path = ../config/WooCommerce.debug.xcconfig; sourceTree = ""; }; @@ -10572,6 +10574,7 @@ B626C71A287659D60083820C /* CustomFieldsListView.swift */, B6C838DD28793B3A003AB786 /* CustomFieldViewModel.swift */, 86F9D3632C897FFE00B1835B /* CustomFieldEditorView.swift */, + 86F5FFE12CA302B300C767C4 /* CustomFieldsListViewModel.swift */, ); path = "Custom Fields"; sourceTree = ""; @@ -16095,6 +16098,7 @@ B99B87A72AEFCF0A006B8AB1 /* Order+Empty.swift in Sources */, CE6A8FB62B725A690063564D /* AnalyticsReportLinkViewModel.swift in Sources */, 684AB83C2873DF04003DFDD1 /* CardReaderManualsViewModel.swift in Sources */, + 86F5FFE22CA302B300C767C4 /* CustomFieldsListViewModel.swift in Sources */, 575472812452185300A94C3C /* PushNotification.swift in Sources */, 0396CFAD2981476900E91436 /* CardPresentModalBuiltInConnectingFailed.swift in Sources */, 02C1853B27FF0D9C00ABD764 /* RefundSubmissionUseCase.swift in Sources */, From abb6f230819e78558e683196e7bc352b67ccbead Mon Sep 17 00:00:00 2001 From: Hafiz Rahman Date: Tue, 24 Sep 2024 22:08:05 +0700 Subject: [PATCH 02/10] Update CustomFieldsListView to make use of CustomFieldsListViewModel --- .../Order Details/OrderDetailsViewModel.swift | 2 +- .../Custom Fields/CustomFieldsListView.swift | 106 +++++++++--------- 2 files changed, 55 insertions(+), 53 deletions(-) diff --git a/WooCommerce/Classes/ViewModels/Order Details/OrderDetailsViewModel.swift b/WooCommerce/Classes/ViewModels/Order Details/OrderDetailsViewModel.swift index 1389a5fef2d..c09635fca3b 100644 --- a/WooCommerce/Classes/ViewModels/Order Details/OrderDetailsViewModel.swift +++ b/WooCommerce/Classes/ViewModels/Order Details/OrderDetailsViewModel.swift @@ -488,7 +488,7 @@ extension OrderDetailsViewModel { let customFieldsView = UIHostingController( rootView: CustomFieldsListView( isEditable: featureFlagService.isFeatureFlagEnabled(.viewEditCustomFieldsInProductsAndOrders), - customFields: customFields)) + viewModel: CustomFieldsListViewModel(customFields: customFields))) viewController.present(customFieldsView, animated: true) case .seeReceipt: let countryCode = configurationLoader.configuration.countryCode diff --git a/WooCommerce/Classes/ViewRelated/Custom Fields/CustomFieldsListView.swift b/WooCommerce/Classes/ViewRelated/Custom Fields/CustomFieldsListView.swift index 11b0c8f6523..a156e46ba30 100644 --- a/WooCommerce/Classes/ViewRelated/Custom Fields/CustomFieldsListView.swift +++ b/WooCommerce/Classes/ViewRelated/Custom Fields/CustomFieldsListView.swift @@ -2,53 +2,62 @@ import SwiftUI struct CustomFieldsListView: View { @Environment(\.presentationMode) var presentationMode + @ObservedObject private var viewModel: CustomFieldsListViewModel let isEditable: Bool - let customFields: [CustomFieldViewModel] + + init(isEditable: Bool, + viewModel: CustomFieldsListViewModel) { + self.isEditable = isEditable + self.viewModel = viewModel + } var body: some View { NavigationView { - GeometryReader { geometry in - ScrollView { - VStack(alignment: .leading) { - ForEach(customFields) { customField in - if isEditable { - NavigationLink(destination: CustomFieldEditorView(key: customField.title, - value: customField.content) - ) { - CustomFieldRow(isEditable: true, - title: customField.title, - content: customField.content.removedHTMLTags, - contentURL: customField.contentURL) - } - } - else { - CustomFieldRow(isEditable: false, - title: customField.title, - content: customField.content.removedHTMLTags, - contentURL: customField.contentURL) - } - - Divider() - .padding(.leading) - } + List(viewModel.combinedList) { customField in + if isEditable { + NavigationLink(destination: CustomFieldEditorView(key: customField.key, value: customField.value)) { + CustomFieldRow(isEditable: true, + title: customField.key, + content: customField.value.removedHTMLTags, + contentURL: nil) } - .padding(.horizontal, insets: geometry.safeAreaInsets) - .background(Color(.listForeground(modal: false))) + } else { + CustomFieldRow(isEditable: false, + title: customField.key, + content: customField.value.removedHTMLTags, + contentURL: nil) } - .background(Color(.listBackground)) - .ignoresSafeArea(edges: .horizontal) - .toolbar { - ToolbarItem(placement: .cancellationAction) { - Button(action: { - presentationMode.wrappedValue.dismiss() - }, label: { - Image(uiImage: .closeButton) - }) + } + .navigationTitle(Localization.title) + .navigationBarTitleDisplayMode(.inline) + .toolbar { + ToolbarItem(placement: .cancellationAction) { + Button(action: { + presentationMode.wrappedValue.dismiss() + }, label: { + Image(uiImage: .closeButton) + }) + } + + ToolbarItem(placement: .navigationBarTrailing) { + if isEditable { + HStack { + Button { + // todo-13493: add save handling + } label: { + Text("Save") // todo-13493: set String to be translatable + } + .disabled(!viewModel.hasChanges) + Button(action: { + // todo-13493: add addition handling + }, label: { + Image(systemName: "plus") + .renderingMode(.template) + }) + } } } - .navigationTitle(Localization.title) - .navigationBarTitleDisplayMode(.inline) } } .wooNavigationBarStyle() @@ -105,15 +114,6 @@ private struct CustomFieldRow: View { .footnoteStyle() .lineLimit(isEditable ? 2 : nil) } - }.padding([.leading, .trailing], Constants.vStackPadding) - - Spacer() - - if isEditable { - // Chevron icon - Image(uiImage: .chevronImage) - .flipsForRightToLeftLayoutDirection(true) - .foregroundStyle(Color(.textTertiary)) } } .padding(Constants.hStackPadding) @@ -144,11 +144,13 @@ private extension CustomFieldRow { struct OrderCustomFieldsDetails_Previews: PreviewProvider { static var previews: some View { CustomFieldsListView( - isEditable: false, - customFields: [ - CustomFieldViewModel(id: 0, title: "First Title", content: "First Content"), - CustomFieldViewModel(id: 1, title: "Second Title", content: "Second Content", contentURL: URL(string: "https://woocommerce.com/")) - ]) + isEditable: true, + viewModel: CustomFieldsListViewModel( + customFields: [ + CustomFieldViewModel(id: 0, title: "First Title", content: "First Content"), + CustomFieldViewModel(id: 1, title: "Second Title", content: "Second Content", contentURL: URL(string: "https://woocommerce.com/")) + ]) + ) } } From 4ff48580c782799b504ab7f67f38a937783a6631 Mon Sep 17 00:00:00 2001 From: Hafiz Rahman Date: Tue, 24 Sep 2024 22:10:46 +0700 Subject: [PATCH 03/10] Add unit tests for CustomFieldsListViewModel --- .../WooCommerce.xcodeproj/project.pbxproj | 4 + .../CustomFieldsListViewModelTests.swift | 98 +++++++++++++++++++ 2 files changed, 102 insertions(+) create mode 100644 WooCommerce/WooCommerceTests/ViewRelated/Custom Fields/CustomFieldsListViewModelTests.swift diff --git a/WooCommerce/WooCommerce.xcodeproj/project.pbxproj b/WooCommerce/WooCommerce.xcodeproj/project.pbxproj index db0554df894..49bcf88e6e7 100644 --- a/WooCommerce/WooCommerce.xcodeproj/project.pbxproj +++ b/WooCommerce/WooCommerce.xcodeproj/project.pbxproj @@ -1713,6 +1713,7 @@ 86E40AED2B597DEC00990365 /* BlazeCampaignCreationCoordinatorTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 86E40AEC2B597DEC00990365 /* BlazeCampaignCreationCoordinatorTests.swift */; }; 86F0896F2B307D7E00D668A1 /* ThemesPreviewViewModelTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 86F0896E2B307D7E00D668A1 /* ThemesPreviewViewModelTests.swift */; }; 86F5FFE22CA302B300C767C4 /* CustomFieldsListViewModel.swift in Sources */ = {isa = PBXBuildFile; fileRef = 86F5FFE12CA302B300C767C4 /* CustomFieldsListViewModel.swift */; }; + 86F5FFE42CA30D9200C767C4 /* CustomFieldsListViewModelTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 86F5FFE32CA30D9200C767C4 /* CustomFieldsListViewModelTests.swift */; }; 86F9D3642C897FFE00B1835B /* CustomFieldEditorView.swift in Sources */ = {isa = PBXBuildFile; fileRef = 86F9D3632C897FFE00B1835B /* CustomFieldEditorView.swift */; }; 8CD41D4A21F8A7E300CF3C2B /* RELEASE-NOTES.txt in Resources */ = {isa = PBXBuildFile; fileRef = 8CD41D4921F8A7E300CF3C2B /* RELEASE-NOTES.txt */; }; 933A27372222354600C2143A /* Logging.swift in Sources */ = {isa = PBXBuildFile; fileRef = 933A27362222354600C2143A /* Logging.swift */; }; @@ -4738,6 +4739,7 @@ 86E40AEC2B597DEC00990365 /* BlazeCampaignCreationCoordinatorTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = BlazeCampaignCreationCoordinatorTests.swift; sourceTree = ""; }; 86F0896E2B307D7E00D668A1 /* ThemesPreviewViewModelTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = ThemesPreviewViewModelTests.swift; sourceTree = ""; }; 86F5FFE12CA302B300C767C4 /* CustomFieldsListViewModel.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = CustomFieldsListViewModel.swift; sourceTree = ""; }; + 86F5FFE32CA30D9200C767C4 /* CustomFieldsListViewModelTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = CustomFieldsListViewModelTests.swift; sourceTree = ""; }; 86F9D3632C897FFE00B1835B /* CustomFieldEditorView.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = CustomFieldEditorView.swift; sourceTree = ""; }; 8A659E65308A3D9DD79A95F9 /* Pods-WooCommerceTests.release.xcconfig */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = text.xcconfig; name = "Pods-WooCommerceTests.release.xcconfig"; path = "../Pods/Target Support Files/Pods-WooCommerceTests/Pods-WooCommerceTests.release.xcconfig"; sourceTree = ""; }; 8CA4F6DD220B257000A47B5D /* WooCommerce.debug.xcconfig */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text.xcconfig; name = WooCommerce.debug.xcconfig; path = ../config/WooCommerce.debug.xcconfig; sourceTree = ""; }; @@ -9771,6 +9773,7 @@ isa = PBXGroup; children = ( CC5BA5F4287EDC900072F307 /* CustomFieldViewModelTests.swift */, + 86F5FFE32CA30D9200C767C4 /* CustomFieldsListViewModelTests.swift */, ); path = "Custom Fields"; sourceTree = ""; @@ -16421,6 +16424,7 @@ 4552085B25829091001CF873 /* AddAttributeViewModelTests.swift in Sources */, 02F1E6BD2A39805C00C3E4C7 /* ProductDescriptionAICoordinatorTests.swift in Sources */, CC33238C29CDF67D00CA9709 /* ComponentSettingsViewModelTests.swift in Sources */, + 86F5FFE42CA30D9200C767C4 /* CustomFieldsListViewModelTests.swift in Sources */, 0261F5A728D454CF00B7AC72 /* ProductSearchUICommandTests.swift in Sources */, 098FFA1727AD7F5D002EBEE4 /* OrderStatusListDataSourceTests.swift in Sources */, DE19BB1D26C6911900AB70D9 /* ShippingLabelCustomsFormListViewModelTests.swift in Sources */, diff --git a/WooCommerce/WooCommerceTests/ViewRelated/Custom Fields/CustomFieldsListViewModelTests.swift b/WooCommerce/WooCommerceTests/ViewRelated/Custom Fields/CustomFieldsListViewModelTests.swift new file mode 100644 index 00000000000..ef1bdee4fe4 --- /dev/null +++ b/WooCommerce/WooCommerceTests/ViewRelated/Custom Fields/CustomFieldsListViewModelTests.swift @@ -0,0 +1,98 @@ +import XCTest +@testable import WooCommerce + +final class CustomFieldsListViewModelTests: XCTestCase { + + private var viewModel: CustomFieldsListViewModel! + + override func setUp() { + super.setUp() + let customFields = [ + CustomFieldViewModel(id: 1, title: "Key1", content: "Value1"), + CustomFieldViewModel(id: 2, title: "Key2", content: "Value2") + ] + viewModel = CustomFieldsListViewModel(customFields: customFields) + } + + override func tearDown() { + viewModel = nil + super.tearDown() + } + + func test_given_initializedViewModel_then_displayedItemsMatchInitialCustomFields() { + // Given: The viewModel is initialized with two custom fields (in setUp) + + // When: No additional action needed, we're testing the initial state + + // Then: The displayed items should match the initial custom fields + XCTAssertEqual(viewModel.combinedList.count, 2) + XCTAssertEqual(viewModel.combinedList[0].key, "Key1") + XCTAssertEqual(viewModel.combinedList[0].value, "Value1") + XCTAssertEqual(viewModel.combinedList[0].id, 1) + XCTAssertEqual(viewModel.combinedList[1].key, "Key2") + XCTAssertEqual(viewModel.combinedList[1].value, "Value2") + XCTAssertEqual(viewModel.combinedList[1].id, 2) + } + + func test_given_existingField_when_editFieldCalled_then_displayedItemsAndPendingChangesAreUpdated() { + // Given: A custom field UI to edit an existing field + let editedField = CustomFieldsListViewModel.CustomFieldUI(key: "EditedKey1", value: "EditedValue1", id: 1) + + // When: Editing the field + viewModel.editField(at: 0, newField: editedField) + + // Then: The number of displayed items remains the same as before and the value is edited correctly + XCTAssertEqual(viewModel.combinedList.count, 2) + XCTAssertEqual(viewModel.combinedList[0].key, "EditedKey1") + XCTAssertEqual(viewModel.combinedList[0].value, "EditedValue1") + } + + func test_given_newField_when_addFieldCalled_then_displayedItemsAndPendingChangesAreUpdated() { + // Given: A new custom field UI to add + let newField = CustomFieldsListViewModel.CustomFieldUI(key: "NewKey", value: "NewValue") + + // When: Adding the new field + viewModel.addField(newField) + + // Then: The pending changes and displayed items should be updated + XCTAssertEqual(viewModel.combinedList.count, 3) + XCTAssertEqual(viewModel.combinedList.last?.key, "NewKey") + XCTAssertEqual(viewModel.combinedList.last?.value, "NewValue") + } + + func test_given_editedAndNewFields_when_updatingDisplayedItems_then_changesAreReflected() { + // Given: An edited field and a new field + let editedField = CustomFieldsListViewModel.CustomFieldUI(key: "EditedKey1", value: "EditedValue1", id: 1) + let newField = CustomFieldsListViewModel.CustomFieldUI(key: "NewKey", value: "NewValue") + + // When: Editing and adding fields + viewModel.editField(at: 0, newField: editedField) + viewModel.addField(newField) + + // Then: The displayed items should reflect both the edited and added fields + XCTAssertEqual(viewModel.combinedList.count, 3) + XCTAssertEqual(viewModel.combinedList[0].key, "EditedKey1") + XCTAssertEqual(viewModel.combinedList[0].value, "EditedValue1") + XCTAssertEqual(viewModel.combinedList[2].key, "NewKey") + XCTAssertEqual(viewModel.combinedList[2].value, "NewValue") + } + + func test_given_variousChanges_when_pendingChangesUpdated_then_hasChangesReflectsCorrectState() { + // Given: Initial state with no changes + XCTAssertFalse(viewModel.hasChanges) + + // When: Editing a field + let editedField = CustomFieldsListViewModel.CustomFieldUI(key: "EditedKey1", value: "EditedValue1", id: 1) + viewModel.editField(at: 0, newField: editedField) + + // Then: hasChanges should be true + XCTAssertTrue(viewModel.hasChanges) + + // When: Adding a new field + let newField = CustomFieldsListViewModel.CustomFieldUI(key: "NewKey", value: "NewValue") + viewModel.addField(newField) + + // Then: hasChanges should be true + XCTAssertTrue(viewModel.hasChanges) + } +} From f229605bbe6201284f4c24e279f4529ac8d358b9 Mon Sep 17 00:00:00 2001 From: Hafiz Rahman Date: Tue, 24 Sep 2024 22:24:47 +0700 Subject: [PATCH 04/10] Add error handling for invalid index --- .../Custom Fields/CustomFieldsListViewModel.swift | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/WooCommerce/Classes/ViewRelated/Custom Fields/CustomFieldsListViewModel.swift b/WooCommerce/Classes/ViewRelated/Custom Fields/CustomFieldsListViewModel.swift index 0a4bfcdb33b..eb343d3b8bf 100644 --- a/WooCommerce/Classes/ViewRelated/Custom Fields/CustomFieldsListViewModel.swift +++ b/WooCommerce/Classes/ViewRelated/Custom Fields/CustomFieldsListViewModel.swift @@ -25,6 +25,11 @@ final class CustomFieldsListViewModel: ObservableObject { // MARK: - Items actions extension CustomFieldsListViewModel { func editField(at index: Int, newField: CustomFieldUI) { + guard index >= 0 && index < combinedList.count else { + DDLogError("⛔️ Error: Invalid index for editing a custom field") + return + } + let oldField = combinedList[index] if newField.id == nil { From 98cec53a245a4d41c67c566e7ecab91ad485c229 Mon Sep 17 00:00:00 2001 From: Hafiz Rahman Date: Thu, 26 Sep 2024 09:16:58 +0700 Subject: [PATCH 05/10] Refactor editing locally added or existing field to make it clearer. --- .../CustomFieldsListViewModel.swift | 50 +++++++++++-------- 1 file changed, 29 insertions(+), 21 deletions(-) diff --git a/WooCommerce/Classes/ViewRelated/Custom Fields/CustomFieldsListViewModel.swift b/WooCommerce/Classes/ViewRelated/Custom Fields/CustomFieldsListViewModel.swift index eb343d3b8bf..44ae144466e 100644 --- a/WooCommerce/Classes/ViewRelated/Custom Fields/CustomFieldsListViewModel.swift +++ b/WooCommerce/Classes/ViewRelated/Custom Fields/CustomFieldsListViewModel.swift @@ -31,30 +31,13 @@ extension CustomFieldsListViewModel { } let oldField = combinedList[index] - if newField.id == nil { - // If there's no id, it means we're now editing a newly added field. - if let addedIndex = addedFields.firstIndex(where: { $0.key == oldField.key }) { - if newField.key == oldField.key { - // If the key hasn't changed, update the existing added field - addedFields[addedIndex] = newField - } else { - // If the key has changed, remove the old field and add the new one - addedFields.remove(at: addedIndex) - addedFields.append(newField) - } - } else { - // This case should not happen in normal flow - DDLogError("⛔️ Error: Editing a newly updated field that doesn't exist in combinedList") - - } + editLocallyAddedField(oldField: oldField, newField: newField) } else { - // For when editing an already edited field - if let editedIndex = editedFields.firstIndex(where: { $0.id == oldField.id }) { - editedFields[editedIndex] = newField + if let existingId = oldField.id { + editExistingField(idToEdit: existingId, newField: newField) } else { - // For the first time a field is edited - editedFields.append(newField) + DDLogError("⛔️ Error: Trying to edit existing field that has no id.") } } @@ -68,6 +51,31 @@ extension CustomFieldsListViewModel { } private extension CustomFieldsListViewModel { + func editLocallyAddedField(oldField: CustomFieldUI, newField: CustomFieldUI) { + if let index = addedFields.firstIndex(where: { $0.key == oldField.key }) { + // Found the matching field, update it + addedFields[index] = newField + } else { + // This shouldn't happen in normal flow, but log an error just in case + DDLogError("⛔️ Error: Trying to edit a locally added field that doesn't exist in addedFields") + } + } + + func editExistingField(idToEdit: Int64, newField: CustomFieldUI) { + guard idToEdit == newField.id else { + DDLogError("⛔️ Error: Trying to edit existing field but supplied new id is different.") + return + } + + if let index = editedFields.firstIndex(where: { $0.id == idToEdit }) { + // This field has been locally edited, let's update it again + editedFields[index] = newField + } else { + // First time the field is locally edited + editedFields.append(newField) + } + } + func updateCombinedList() { let editedList = originalCustomFields.map { field in editedFields.first { $0.id == field.id } ?? CustomFieldUI(customField: field) From 2dde854c52fe672192f5e54cefdcfb0198847a24 Mon Sep 17 00:00:00 2001 From: Hafiz Rahman Date: Thu, 26 Sep 2024 09:17:29 +0700 Subject: [PATCH 06/10] Fix indentation --- .../Custom Fields/CustomFieldsListViewModel.swift | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/WooCommerce/Classes/ViewRelated/Custom Fields/CustomFieldsListViewModel.swift b/WooCommerce/Classes/ViewRelated/Custom Fields/CustomFieldsListViewModel.swift index 44ae144466e..bc356a65cc2 100644 --- a/WooCommerce/Classes/ViewRelated/Custom Fields/CustomFieldsListViewModel.swift +++ b/WooCommerce/Classes/ViewRelated/Custom Fields/CustomFieldsListViewModel.swift @@ -77,11 +77,11 @@ private extension CustomFieldsListViewModel { } func updateCombinedList() { - let editedList = originalCustomFields.map { field in - editedFields.first { $0.id == field.id } ?? CustomFieldUI(customField: field) - } - combinedList = editedList + addedFields + let editedList = originalCustomFields.map { field in + editedFields.first { $0.id == field.id } ?? CustomFieldUI(customField: field) } + combinedList = editedList + addedFields + } } extension CustomFieldsListViewModel { From 66f425b9f64f1761418e8f9e9afa364d7018c7d5 Mon Sep 17 00:00:00 2001 From: Hafiz Rahman Date: Thu, 26 Sep 2024 09:49:51 +0700 Subject: [PATCH 07/10] Update comments to make logic clearer. --- .../Custom Fields/CustomFieldsListViewModel.swift | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/WooCommerce/Classes/ViewRelated/Custom Fields/CustomFieldsListViewModel.swift b/WooCommerce/Classes/ViewRelated/Custom Fields/CustomFieldsListViewModel.swift index bc356a65cc2..882f86243dd 100644 --- a/WooCommerce/Classes/ViewRelated/Custom Fields/CustomFieldsListViewModel.swift +++ b/WooCommerce/Classes/ViewRelated/Custom Fields/CustomFieldsListViewModel.swift @@ -24,6 +24,9 @@ final class CustomFieldsListViewModel: ObservableObject { // MARK: - Items actions extension CustomFieldsListViewModel { + /// Params: + /// - index: The index of field to be edited, taken from the `combinedList` array + /// - newField: The new content for the custom field in question func editField(at index: Int, newField: CustomFieldUI) { guard index >= 0 && index < combinedList.count else { DDLogError("⛔️ Error: Invalid index for editing a custom field") @@ -32,12 +35,13 @@ extension CustomFieldsListViewModel { let oldField = combinedList[index] if newField.id == nil { + // When editing a field that has no id yet, it means the field has only been added locally. editLocallyAddedField(oldField: oldField, newField: newField) } else { if let existingId = oldField.id { editExistingField(idToEdit: existingId, newField: newField) } else { - DDLogError("⛔️ Error: Trying to edit existing field that has no id.") + DDLogError("⛔️ Error: Trying to edit an existing field but it has no id. It might be the wrong field to edit.") } } @@ -53,14 +57,14 @@ extension CustomFieldsListViewModel { private extension CustomFieldsListViewModel { func editLocallyAddedField(oldField: CustomFieldUI, newField: CustomFieldUI) { if let index = addedFields.firstIndex(where: { $0.key == oldField.key }) { - // Found the matching field, update it addedFields[index] = newField } else { - // This shouldn't happen in normal flow, but log an error just in case + // This shouldn't happen in normal flow, but logging just in case DDLogError("⛔️ Error: Trying to edit a locally added field that doesn't exist in addedFields") } } + /// Checking by id when editing an existing field since existing fields will always have them. func editExistingField(idToEdit: Int64, newField: CustomFieldUI) { guard idToEdit == newField.id else { DDLogError("⛔️ Error: Trying to edit existing field but supplied new id is different.") @@ -68,7 +72,7 @@ private extension CustomFieldsListViewModel { } if let index = editedFields.firstIndex(where: { $0.id == idToEdit }) { - // This field has been locally edited, let's update it again + // Existing field has been locally edited, let's update it again editedFields[index] = newField } else { // First time the field is locally edited From 547e190e911088399ac736c79b3727af527f4a13 Mon Sep 17 00:00:00 2001 From: Hafiz Rahman Date: Thu, 26 Sep 2024 10:00:02 +0700 Subject: [PATCH 08/10] Add unit tests for adding duplicate key and editing invalid index Co-authored-by: Paolo Musolino --- .../CustomFieldsListViewModelTests.swift | 27 +++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/WooCommerce/WooCommerceTests/ViewRelated/Custom Fields/CustomFieldsListViewModelTests.swift b/WooCommerce/WooCommerceTests/ViewRelated/Custom Fields/CustomFieldsListViewModelTests.swift index ef1bdee4fe4..44a4ec8f192 100644 --- a/WooCommerce/WooCommerceTests/ViewRelated/Custom Fields/CustomFieldsListViewModelTests.swift +++ b/WooCommerce/WooCommerceTests/ViewRelated/Custom Fields/CustomFieldsListViewModelTests.swift @@ -94,5 +94,32 @@ final class CustomFieldsListViewModelTests: XCTestCase { // Then: hasChanges should be true XCTAssertTrue(viewModel.hasChanges) + + func test_given_invalidIndex_when_editFieldCalled_then_noChangesAreMade() { + // Given: An invalid index and a custom field UI + let editedField = CustomFieldsListViewModel.CustomFieldUI(key: "EditedKey1", value: "EditedValue1", id: 1) + + // When: Trying to edit a field at an invalid index + viewModel.editField(at: -1, newField: editedField) + + // Then: No changes should be made + XCTAssertEqual(viewModel.combinedList.count, 2) + XCTAssertEqual(viewModel.combinedList[0].key, "Key1") + XCTAssertEqual(viewModel.combinedList[0].value, "Value1") + XCTAssertEqual(viewModel.combinedList[1].key, "Key2") + XCTAssertEqual(viewModel.combinedList[1].value, "Value2") + } + + func test_given_duplicateKey_when_addFieldCalled_then_fieldIsAdded() { + // Given: A new custom field UI with a duplicate key + let newField = CustomFieldsListViewModel.CustomFieldUI(key: "Key1", value: "NewValue") + + // When: Adding the new field + viewModel.addField(newField) + + // Then: The field should be added to the list + XCTAssertEqual(viewModel.combinedList.count, 3) + XCTAssertEqual(viewModel.combinedList.last?.key, "Key1") + XCTAssertEqual(viewModel.combinedList.last?.value, "NewValue") } } From 0e2906795fc85a16160d8ce46275d43550fd1456 Mon Sep 17 00:00:00 2001 From: Hafiz Rahman Date: Thu, 26 Sep 2024 10:16:19 +0700 Subject: [PATCH 09/10] Add UUID for CustomFIeldUI and refactor previous id as fieldId. --- .../CustomFieldsListViewModel.swift | 19 ++++--- .../CustomFieldsListViewModelTests.swift | 57 ++++++++++--------- 2 files changed, 39 insertions(+), 37 deletions(-) diff --git a/WooCommerce/Classes/ViewRelated/Custom Fields/CustomFieldsListViewModel.swift b/WooCommerce/Classes/ViewRelated/Custom Fields/CustomFieldsListViewModel.swift index 882f86243dd..be1aae61922 100644 --- a/WooCommerce/Classes/ViewRelated/Custom Fields/CustomFieldsListViewModel.swift +++ b/WooCommerce/Classes/ViewRelated/Custom Fields/CustomFieldsListViewModel.swift @@ -34,11 +34,11 @@ extension CustomFieldsListViewModel { } let oldField = combinedList[index] - if newField.id == nil { + if newField.fieldId == nil { // When editing a field that has no id yet, it means the field has only been added locally. editLocallyAddedField(oldField: oldField, newField: newField) } else { - if let existingId = oldField.id { + if let existingId = oldField.fieldId { editExistingField(idToEdit: existingId, newField: newField) } else { DDLogError("⛔️ Error: Trying to edit an existing field but it has no id. It might be the wrong field to edit.") @@ -66,12 +66,12 @@ private extension CustomFieldsListViewModel { /// Checking by id when editing an existing field since existing fields will always have them. func editExistingField(idToEdit: Int64, newField: CustomFieldUI) { - guard idToEdit == newField.id else { + guard idToEdit == newField.fieldId else { DDLogError("⛔️ Error: Trying to edit existing field but supplied new id is different.") return } - if let index = editedFields.firstIndex(where: { $0.id == idToEdit }) { + if let index = editedFields.firstIndex(where: { $0.fieldId == idToEdit }) { // Existing field has been locally edited, let's update it again editedFields[index] = newField } else { @@ -82,7 +82,7 @@ private extension CustomFieldsListViewModel { func updateCombinedList() { let editedList = originalCustomFields.map { field in - editedFields.first { $0.id == field.id } ?? CustomFieldUI(customField: field) + editedFields.first { $0.fieldId == field.id } ?? CustomFieldUI(customField: field) } combinedList = editedList + addedFields } @@ -90,20 +90,21 @@ private extension CustomFieldsListViewModel { extension CustomFieldsListViewModel { struct CustomFieldUI: Identifiable { + let id = UUID() let key: String let value: String - let id: Int64? + let fieldId: Int64? - init(key: String, value: String, id: Int64? = nil) { + init(key: String, value: String, fieldId: Int64? = nil) { self.key = key self.value = value - self.id = id + self.fieldId = fieldId } init(customField: CustomFieldViewModel) { self.key = customField.title self.value = customField.content - self.id = customField.id + self.fieldId = customField.id } } } diff --git a/WooCommerce/WooCommerceTests/ViewRelated/Custom Fields/CustomFieldsListViewModelTests.swift b/WooCommerce/WooCommerceTests/ViewRelated/Custom Fields/CustomFieldsListViewModelTests.swift index 44a4ec8f192..150519aa064 100644 --- a/WooCommerce/WooCommerceTests/ViewRelated/Custom Fields/CustomFieldsListViewModelTests.swift +++ b/WooCommerce/WooCommerceTests/ViewRelated/Custom Fields/CustomFieldsListViewModelTests.swift @@ -28,10 +28,10 @@ final class CustomFieldsListViewModelTests: XCTestCase { XCTAssertEqual(viewModel.combinedList.count, 2) XCTAssertEqual(viewModel.combinedList[0].key, "Key1") XCTAssertEqual(viewModel.combinedList[0].value, "Value1") - XCTAssertEqual(viewModel.combinedList[0].id, 1) + XCTAssertEqual(viewModel.combinedList[0].fieldId, 1) XCTAssertEqual(viewModel.combinedList[1].key, "Key2") XCTAssertEqual(viewModel.combinedList[1].value, "Value2") - XCTAssertEqual(viewModel.combinedList[1].id, 2) + XCTAssertEqual(viewModel.combinedList[1].fieldId, 2) } func test_given_existingField_when_editFieldCalled_then_displayedItemsAndPendingChangesAreUpdated() { @@ -95,31 +95,32 @@ final class CustomFieldsListViewModelTests: XCTestCase { // Then: hasChanges should be true XCTAssertTrue(viewModel.hasChanges) - func test_given_invalidIndex_when_editFieldCalled_then_noChangesAreMade() { - // Given: An invalid index and a custom field UI - let editedField = CustomFieldsListViewModel.CustomFieldUI(key: "EditedKey1", value: "EditedValue1", id: 1) - - // When: Trying to edit a field at an invalid index - viewModel.editField(at: -1, newField: editedField) - - // Then: No changes should be made - XCTAssertEqual(viewModel.combinedList.count, 2) - XCTAssertEqual(viewModel.combinedList[0].key, "Key1") - XCTAssertEqual(viewModel.combinedList[0].value, "Value1") - XCTAssertEqual(viewModel.combinedList[1].key, "Key2") - XCTAssertEqual(viewModel.combinedList[1].value, "Value2") - } - - func test_given_duplicateKey_when_addFieldCalled_then_fieldIsAdded() { - // Given: A new custom field UI with a duplicate key - let newField = CustomFieldsListViewModel.CustomFieldUI(key: "Key1", value: "NewValue") - - // When: Adding the new field - viewModel.addField(newField) - - // Then: The field should be added to the list - XCTAssertEqual(viewModel.combinedList.count, 3) - XCTAssertEqual(viewModel.combinedList.last?.key, "Key1") - XCTAssertEqual(viewModel.combinedList.last?.value, "NewValue") + func test_given_invalidIndex_when_editFieldCalled_then_noChangesAreMade() { + // Given: An invalid index and a custom field UI + let editedField = CustomFieldsListViewModel.CustomFieldUI(key: "EditedKey1", value: "EditedValue1", id: 1) + + // When: Trying to edit a field at an invalid index + viewModel.editField(at: -1, newField: editedField) + + // Then: No changes should be made + XCTAssertEqual(viewModel.combinedList.count, 2) + XCTAssertEqual(viewModel.combinedList[0].key, "Key1") + XCTAssertEqual(viewModel.combinedList[0].value, "Value1") + XCTAssertEqual(viewModel.combinedList[1].key, "Key2") + XCTAssertEqual(viewModel.combinedList[1].value, "Value2") + } + + func test_given_duplicateKey_when_addFieldCalled_then_fieldIsAdded() { + // Given: A new custom field UI with a duplicate key + let newField = CustomFieldsListViewModel.CustomFieldUI(key: "Key1", value: "NewValue") + + // When: Adding the new field + viewModel.addField(newField) + + // Then: The field should be added to the list + XCTAssertEqual(viewModel.combinedList.count, 3) + XCTAssertEqual(viewModel.combinedList.last?.key, "Key1") + XCTAssertEqual(viewModel.combinedList.last?.value, "NewValue") + } } } From f280d615bad62b7a9ae6667dc35a9c21c27a12c8 Mon Sep 17 00:00:00 2001 From: Hafiz Rahman Date: Fri, 27 Sep 2024 19:21:18 +0700 Subject: [PATCH 10/10] Update unit tests. --- .../Custom Fields/CustomFieldsListViewModelTests.swift | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/WooCommerce/WooCommerceTests/ViewRelated/Custom Fields/CustomFieldsListViewModelTests.swift b/WooCommerce/WooCommerceTests/ViewRelated/Custom Fields/CustomFieldsListViewModelTests.swift index 150519aa064..b4f8b76f09e 100644 --- a/WooCommerce/WooCommerceTests/ViewRelated/Custom Fields/CustomFieldsListViewModelTests.swift +++ b/WooCommerce/WooCommerceTests/ViewRelated/Custom Fields/CustomFieldsListViewModelTests.swift @@ -36,7 +36,7 @@ final class CustomFieldsListViewModelTests: XCTestCase { func test_given_existingField_when_editFieldCalled_then_displayedItemsAndPendingChangesAreUpdated() { // Given: A custom field UI to edit an existing field - let editedField = CustomFieldsListViewModel.CustomFieldUI(key: "EditedKey1", value: "EditedValue1", id: 1) + let editedField = CustomFieldsListViewModel.CustomFieldUI(key: "EditedKey1", value: "EditedValue1", fieldId: 1) // When: Editing the field viewModel.editField(at: 0, newField: editedField) @@ -62,7 +62,7 @@ final class CustomFieldsListViewModelTests: XCTestCase { func test_given_editedAndNewFields_when_updatingDisplayedItems_then_changesAreReflected() { // Given: An edited field and a new field - let editedField = CustomFieldsListViewModel.CustomFieldUI(key: "EditedKey1", value: "EditedValue1", id: 1) + let editedField = CustomFieldsListViewModel.CustomFieldUI(key: "EditedKey1", value: "EditedValue1", fieldId: 1) let newField = CustomFieldsListViewModel.CustomFieldUI(key: "NewKey", value: "NewValue") // When: Editing and adding fields @@ -82,7 +82,7 @@ final class CustomFieldsListViewModelTests: XCTestCase { XCTAssertFalse(viewModel.hasChanges) // When: Editing a field - let editedField = CustomFieldsListViewModel.CustomFieldUI(key: "EditedKey1", value: "EditedValue1", id: 1) + let editedField = CustomFieldsListViewModel.CustomFieldUI(key: "EditedKey1", value: "EditedValue1", fieldId: 1) viewModel.editField(at: 0, newField: editedField) // Then: hasChanges should be true @@ -97,7 +97,7 @@ final class CustomFieldsListViewModelTests: XCTestCase { func test_given_invalidIndex_when_editFieldCalled_then_noChangesAreMade() { // Given: An invalid index and a custom field UI - let editedField = CustomFieldsListViewModel.CustomFieldUI(key: "EditedKey1", value: "EditedValue1", id: 1) + let editedField = CustomFieldsListViewModel.CustomFieldUI(key: "EditedKey1", value: "EditedValue1", fieldId: 1) // When: Trying to edit a field at an invalid index viewModel.editField(at: -1, newField: editedField)