From b8f1665b830ef2695cb49c0b4979874bd8090c35 Mon Sep 17 00:00:00 2001 From: Josh Heald Date: Thu, 7 Nov 2024 10:47:19 +0000 Subject: [PATCH 1/3] 14304 Non-default currency order edit notice As a workaround, we disable editing orders with a non-default currency. If we allowed this, items on these orders would be shown in the wrong currency. See peaMlT-XF-p2 for context. Multi-currency is not supported in the app at present, but when we add support properly, this workaround should be removed and the edit screen fixed. --- .../Classes/Analytics/WooAnalyticsEvent.swift | 4 ++ .../Classes/Analytics/WooAnalyticsStat.swift | 1 + .../OrderDetailsSyncStateController.swift | 16 +++++ .../Order Details/OrderDetailsViewModel.swift | 65 +++++++++++++------ .../OrderDetailsViewController.swift | 18 ++++- .../WooCommerce.xcodeproj/project.pbxproj | 4 ++ .../OrderDetailsViewModelTests.swift | 43 +++++++++++- 7 files changed, 128 insertions(+), 23 deletions(-) create mode 100644 WooCommerce/Classes/ViewModels/Order Details/OrderDetailsSyncStateController.swift diff --git a/WooCommerce/Classes/Analytics/WooAnalyticsEvent.swift b/WooCommerce/Classes/Analytics/WooAnalyticsEvent.swift index c03107a1133..24d01c07921 100644 --- a/WooCommerce/Classes/Analytics/WooAnalyticsEvent.swift +++ b/WooCommerce/Classes/Analytics/WooAnalyticsEvent.swift @@ -654,6 +654,10 @@ extension WooAnalyticsEvent { ]) } + static func orderEditButtonTappedWhileDisabledForCurrencyConflict() -> WooAnalyticsEvent { + WooAnalyticsEvent(statName: .orderEditButtonTappedWhileDisabledForCurrencyConflict, properties: [:]) + } + static func orderProductAdd(flow: Flow, source: BarcodeScanning.Source, addedVia: OrderProductAdditionVia, diff --git a/WooCommerce/Classes/Analytics/WooAnalyticsStat.swift b/WooCommerce/Classes/Analytics/WooAnalyticsStat.swift index bcbc346b718..cdd92777abb 100644 --- a/WooCommerce/Classes/Analytics/WooAnalyticsStat.swift +++ b/WooCommerce/Classes/Analytics/WooAnalyticsStat.swift @@ -474,6 +474,7 @@ enum WooAnalyticsStat: String { case orderContactAction = "order_contact_action" case orderCustomerAdd = "order_customer_add" case orderEditButtonTapped = "order_edit_button_tapped" + case orderEditButtonTappedWhileDisabledForCurrencyConflict = "order_edit_button_tapped_while_disabled_for_currency_conflict" case ordersListFilter = "orders_list_filter" case ordersListSearch = "orders_list_search" case ordersListLoaded = "orders_list_loaded" diff --git a/WooCommerce/Classes/ViewModels/Order Details/OrderDetailsSyncStateController.swift b/WooCommerce/Classes/ViewModels/Order Details/OrderDetailsSyncStateController.swift new file mode 100644 index 00000000000..44538d6692e --- /dev/null +++ b/WooCommerce/Classes/ViewModels/Order Details/OrderDetailsSyncStateController.swift @@ -0,0 +1,16 @@ +import Foundation + +protocol OrderDetailsSyncStateControlling { + var syncState: OrderDetailsSyncState { get set } +} + +struct OrderDetailsSyncStateController: OrderDetailsSyncStateControlling { + var syncState: OrderDetailsSyncState +} + +/// Defines the possible sync states of the view model data. +/// +enum OrderDetailsSyncState { + case notSynced + case synced +} diff --git a/WooCommerce/Classes/ViewModels/Order Details/OrderDetailsViewModel.swift b/WooCommerce/Classes/ViewModels/Order Details/OrderDetailsViewModel.swift index 2eba41cf431..c14cfe466b5 100644 --- a/WooCommerce/Classes/ViewModels/Order Details/OrderDetailsViewModel.swift +++ b/WooCommerce/Classes/ViewModels/Order Details/OrderDetailsViewModel.swift @@ -21,7 +21,7 @@ final class OrderDetailsViewModel { /// Defines the current sync states of the view model data. /// - private var syncState: SyncState = .notSynced + private var syncStateController: OrderDetailsSyncStateControlling var orderStatus: OrderStatus? { return lookUpOrderStatus(for: order) @@ -31,12 +31,14 @@ final class OrderDetailsViewModel { stores: StoresManager = ServiceLocator.stores, storageManager: StorageManagerType = ServiceLocator.storageManager, currencyFormatter: CurrencyFormatter = CurrencyFormatter(currencySettings: ServiceLocator.currencySettings), - featureFlagService: FeatureFlagService = ServiceLocator.featureFlagService) { + featureFlagService: FeatureFlagService = ServiceLocator.featureFlagService, + syncStateController: OrderDetailsSyncStateControlling = OrderDetailsSyncStateController(syncState: .notSynced)) { self.order = order self.stores = stores self.storageManager = storageManager self.currencyFormatter = currencyFormatter self.featureFlagService = featureFlagService + self.syncStateController = syncStateController self.configurationLoader = CardPresentConfigurationLoader(stores: stores) self.dataSource = OrderDetailsDataSource(order: order, cardPresentPaymentsConfiguration: configurationLoader.configuration) @@ -168,8 +170,23 @@ final class OrderDetailsViewModel { /// Returns edit action availability given the internal state. /// - var editButtonIsEnabled: Bool { - syncState == .synced + var editButtonBehaviour: EditButtonBehaviour { + guard syncStateController.syncState == .synced else { + return .disabledForSyncing + } + + guard let orderCurrency = CurrencyCode(caseInsensitiveRawValue: order.currency), + orderCurrency == ServiceLocator.currencySettings.currencyCode else { + return .showNoticeForCurrencyConflict + } + + return .enabled + } + + enum EditButtonBehaviour { + case enabled + case disabledForSyncing + case showNoticeForCurrencyConflict } var paymentMethodsViewModel: PaymentMethodsViewModel { @@ -297,7 +314,7 @@ extension OrderDetailsViewModel { /// Update state to synced /// - self?.syncState = .synced + self?.syncStateController.syncState = .synced onReloadSections?() onCompletion?() @@ -908,21 +925,11 @@ private extension OrderDetailsViewModel { } } -// MARK: Definitions -private extension OrderDetailsViewModel { - /// Defines the possible sync states of the view model data. - /// - enum SyncState { - case notSynced - case synced - } -} - // MARK: - Notices -private extension OrderDetailsViewModel { - func displayReceiptRetrievalErrorNotice(for order: Order, - with error: Error?, - in viewController: UIViewController) { +extension OrderDetailsViewModel { + private func displayReceiptRetrievalErrorNotice(for order: Order, + with error: Error?, + in viewController: UIViewController) { let noticePresenter = DefaultNoticePresenter() let notice = Notice(title: Localization.failedReceiptRetrievalNoticeText, feedbackType: .error) @@ -932,11 +939,31 @@ private extension OrderDetailsViewModel { DDLogError("Failed to retrieve receipt for order: \(order.orderID). Site \(order.siteID). Error: \(String(describing: error))") } + func showNoticeForEditingWithCurrencyConflict(in viewController: UIViewController) { + let siteCurrency = ServiceLocator.currencySettings.currencyCode.rawValue + let noticePresenter = DefaultNoticePresenter() + let title = String(format: Localization.editingOrderWithCurrencyConflictNoticeTitle, order.currency, siteCurrency) + let notice = Notice(title: title, + feedbackType: .error) + noticePresenter.presentingViewController = viewController + noticePresenter.enqueue(notice: notice) + + DDLogError("Attempt to edit order \(order.orderID) with currency \(order.currency), but did not match site's currency \(siteCurrency).") + } + enum Localization { static let failedReceiptRetrievalNoticeText = NSLocalizedString( "OrderDetailsViewModel.displayReceiptRetrievalErrorNotice.notice", value: "Unable to retrieve receipt.", comment: "Notice that appears when no receipt can be retrieved upon tapping on 'See receipt' in the Order Details view.") + + static let editingOrderWithCurrencyConflictNoticeTitle = NSLocalizedString( + "OrderDetailsViewModel.editingOrderWithCurrencyConflictNotice.title", + value: "Sorry, you can only edit this order on the web, as it uses %1$@, and your site's currency is %2$@.", + comment: "Title for notice that's shown when trying to edit an order that's in a different currency. " + + "This action isn't supported in the app. Placeholders: %1$@ is the order currency code (e.g. USD), " + + "%2$@ is the site currency code (e.g. GBP.)" + ) } } diff --git a/WooCommerce/Classes/ViewRelated/Orders/Order Details/OrderDetailsViewController.swift b/WooCommerce/Classes/ViewRelated/Orders/Order Details/OrderDetailsViewController.swift index 82a28ad1d9f..014aadd7bde 100644 --- a/WooCommerce/Classes/ViewRelated/Orders/Order Details/OrderDetailsViewController.swift +++ b/WooCommerce/Classes/ViewRelated/Orders/Order Details/OrderDetailsViewController.swift @@ -159,9 +159,9 @@ private extension OrderDetailsViewController { let editButton = UIBarButtonItem(title: Localization.NavBar.editOrder, style: .plain, target: self, - action: #selector(editOrder)) + action: #selector(editButtonTapped)) editButton.accessibilityIdentifier = "order-details-edit-button" - editButton.isEnabled = viewModel.editButtonIsEnabled + editButton.isEnabled = viewModel.editButtonBehaviour != .disabledForSyncing navigationItem.rightBarButtonItems = [editButton] + orderNavigationRightBarButtonItems() navigationItem.largeTitleDisplayMode = .never @@ -388,9 +388,21 @@ private extension OrderDetailsViewController { } } + @objc private func editButtonTapped() { + switch viewModel.editButtonBehaviour { + case .enabled: + editOrder() + case .showNoticeForCurrencyConflict: + viewModel.showNoticeForEditingWithCurrencyConflict(in: self) + ServiceLocator.analytics.track(event: WooAnalyticsEvent.Orders.orderEditButtonTappedWhileDisabledForCurrencyConflict()) + case .disabledForSyncing: + return + } + } + /// Presents the order edit form /// - @objc private func editOrder() { + private func editOrder() { let viewModel = EditableOrderViewModel(siteID: viewModel.order.siteID, flow: .editing(initialOrder: viewModel.order)) let viewController = OrderFormHostingController(viewModel: viewModel) if ServiceLocator.featureFlagService.isFeatureFlagEnabled(.sideBySideViewForOrderForm) { diff --git a/WooCommerce/WooCommerce.xcodeproj/project.pbxproj b/WooCommerce/WooCommerce.xcodeproj/project.pbxproj index 07b1e0f217b..e424a833231 100644 --- a/WooCommerce/WooCommerce.xcodeproj/project.pbxproj +++ b/WooCommerce/WooCommerce.xcodeproj/project.pbxproj @@ -853,6 +853,7 @@ 20D5CB532AFCF8E7009A39C3 /* PaymentsToggleRow.swift in Sources */ = {isa = PBXBuildFile; fileRef = 20D5CB522AFCF8E7009A39C3 /* PaymentsToggleRow.swift */; }; 20DA6DDB2B681175002AA0FB /* AdaptiveModalContainer.swift in Sources */ = {isa = PBXBuildFile; fileRef = 20DA6DDA2B681175002AA0FB /* AdaptiveModalContainer.swift */; }; 20E188842AD059A50053E945 /* AboutTapToPayView.swift in Sources */ = {isa = PBXBuildFile; fileRef = 20E188832AD059A50053E945 /* AboutTapToPayView.swift */; }; + 20FA73882CDCC3A900554BE3 /* OrderDetailsSyncStateController.swift in Sources */ = {isa = PBXBuildFile; fileRef = 20FA73872CDCC3A900554BE3 /* OrderDetailsSyncStateController.swift */; }; 247CE89C2583402A00F9D9D1 /* Embassy in Frameworks */ = {isa = PBXBuildFile; productRef = 247CE89B2583402A00F9D9D1 /* Embassy */; }; 247CE8A6258340E600F9D9D1 /* ScreenshotImages.xcassets in Resources */ = {isa = PBXBuildFile; fileRef = 247CE8A5258340E600F9D9D1 /* ScreenshotImages.xcassets */; }; 24C5AC7625A53021008FD769 /* Embassy in Frameworks */ = {isa = PBXBuildFile; productRef = 247CE89B2583402A00F9D9D1 /* Embassy */; }; @@ -3961,6 +3962,7 @@ 20D5CB522AFCF8E7009A39C3 /* PaymentsToggleRow.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = PaymentsToggleRow.swift; sourceTree = ""; }; 20DA6DDA2B681175002AA0FB /* AdaptiveModalContainer.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = AdaptiveModalContainer.swift; sourceTree = ""; }; 20E188832AD059A50053E945 /* AboutTapToPayView.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = AboutTapToPayView.swift; sourceTree = ""; }; + 20FA73872CDCC3A900554BE3 /* OrderDetailsSyncStateController.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = OrderDetailsSyncStateController.swift; sourceTree = ""; }; 247CE8A5258340E600F9D9D1 /* ScreenshotImages.xcassets */ = {isa = PBXFileReference; lastKnownFileType = folder.assetcatalog; path = ScreenshotImages.xcassets; sourceTree = ""; }; 24C579D124F476300076E1B4 /* Woo-Alpha.entitlements */ = {isa = PBXFileReference; lastKnownFileType = text.plist.entitlements; path = "Woo-Alpha.entitlements"; sourceTree = ""; }; 24F98C4F2502AEE200F49B68 /* EventLogging.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = EventLogging.swift; sourceTree = ""; }; @@ -12349,6 +12351,7 @@ CECC758D23D2260E00486676 /* Refunded Products */, 02619856256B539400E321E9 /* Shipping Labels */, CE85535C209B5BB700938BDC /* OrderDetailsViewModel.swift */, + 20FA73872CDCC3A900554BE3 /* OrderDetailsSyncStateController.swift */, 0375799828201F750083F2E1 /* CardPresentPaymentsReadinessUseCase.swift */, D817585D22BB5E8700289CFE /* OrderEmailComposer.swift */, D817586122BB64C300289CFE /* OrderDetailsNotices.swift */, @@ -16262,6 +16265,7 @@ DA0DBE2F2C4FC61D00DF14C0 /* POSFloatingControlView.swift in Sources */, DE02ABAD2B55288D008E0AC4 /* BlazeTargetLocationSearchView.swift in Sources */, 45BBFBC3274FDA6400213001 /* HubMenuViewController.swift in Sources */, + 20FA73882CDCC3A900554BE3 /* OrderDetailsSyncStateController.swift in Sources */, 451B1747258BD7B600836277 /* AddAttributeOptionsViewModel.swift in Sources */, B958640A2A657F44002C4C6E /* EnhancedCouponListView.swift in Sources */, B9E4364E287589E200883CFA /* BadgeView.swift in Sources */, diff --git a/WooCommerce/WooCommerceTests/ViewRelated/OrderDetailsViewModelTests.swift b/WooCommerce/WooCommerceTests/ViewRelated/OrderDetailsViewModelTests.swift index 48f9e61bcaf..e70907f4621 100644 --- a/WooCommerce/WooCommerceTests/ViewRelated/OrderDetailsViewModelTests.swift +++ b/WooCommerce/WooCommerceTests/ViewRelated/OrderDetailsViewModelTests.swift @@ -213,7 +213,48 @@ final class OrderDetailsViewModelTests: XCTestCase { let viewModel = OrderDetailsViewModel(order: order) // Then - XCTAssertFalse(viewModel.editButtonIsEnabled) + XCTAssertEqual(viewModel.editButtonBehaviour, OrderDetailsViewModel.EditButtonBehaviour.disabledForSyncing) + } + + // Context: https://github.com/woocommerce/woocommerce-ios/issues/14304 + func test_there_should_not_be_an_edit_order_action_if_order_currency_doesnt_match_site_currency() { + // Given + let gbp = CurrencySettings(currencyCode: .GBP, + currencyPosition: .left, + thousandSeparator: "", + decimalSeparator: ".", + numberOfDecimals: 2) + ServiceLocator.setCurrencySettings(gbp) + + let usdOrder = Order.fake().copy(currency: "usd", total: "10.0") + + let syncStateController = OrderDetailsSyncStateController(syncState: .synced) + + // When + let viewModel = OrderDetailsViewModel(order: usdOrder, syncStateController: syncStateController) + + // Then + XCTAssertEqual(viewModel.editButtonBehaviour, OrderDetailsViewModel.EditButtonBehaviour.showNoticeForCurrencyConflict) + } + + func test_the_edit_order_action_should_be_enabled_when_the_order_is_synced_and_matches_site_currency() { + // Given + let usd = CurrencySettings(currencyCode: .USD, + currencyPosition: .left, + thousandSeparator: "", + decimalSeparator: ".", + numberOfDecimals: 2) + ServiceLocator.setCurrencySettings(usd) + + let usdOrder = Order.fake().copy(currency: "usd", total: "10.0") + + let syncStateController = OrderDetailsSyncStateController(syncState: .synced) + + // When + let viewModel = OrderDetailsViewModel(order: usdOrder, syncStateController: syncStateController) + + // Then + XCTAssertEqual(viewModel.editButtonBehaviour, OrderDetailsViewModel.EditButtonBehaviour.enabled) } func test_paymentMethodsViewModel_title_contains_formatted_order_amount() { From 8f6359e396c5cca98fe1886273b35cbec95ad10b Mon Sep 17 00:00:00 2001 From: Josh Heald Date: Thu, 7 Nov 2024 10:59:58 +0000 Subject: [PATCH 2/3] 14304 Add order edit notice to 21.1 release notes --- RELEASE-NOTES.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/RELEASE-NOTES.txt b/RELEASE-NOTES.txt index 68dc4b3fb5b..c3eabb0d392 100644 --- a/RELEASE-NOTES.txt +++ b/RELEASE-NOTES.txt @@ -6,6 +6,7 @@ - [internal] Updated Xcode version to 16.1 [https://github.com/woocommerce/woocommerce-ios/pull/14225] - [**] Fixed: properly open and show order details in Watch app [https://github.com/woocommerce/woocommerce-ios/pull/14306] +- [*] Order editing: disable editing orders in other currencies, due to risk of showing incorrect information [https://github.com/woocommerce/woocommerce-ios/pull/14345] 21.0 ----- From d7d69c5beda5bc275c4cc72e936f3b30abcc6484 Mon Sep 17 00:00:00 2001 From: Josh Heald Date: Thu, 7 Nov 2024 13:43:21 +0000 Subject: [PATCH 3/3] 14304 update test to ensure order action works --- .../Order Details/OrderDetailsViewControllerTests.swift | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/WooCommerce/WooCommerceTests/ViewRelated/Orders/Order Details/OrderDetailsViewControllerTests.swift b/WooCommerce/WooCommerceTests/ViewRelated/Orders/Order Details/OrderDetailsViewControllerTests.swift index 374e2f91b73..badb5730fac 100644 --- a/WooCommerce/WooCommerceTests/ViewRelated/Orders/Order Details/OrderDetailsViewControllerTests.swift +++ b/WooCommerce/WooCommerceTests/ViewRelated/Orders/Order Details/OrderDetailsViewControllerTests.swift @@ -77,14 +77,17 @@ final class OrderDetailsViewControllerTests: XCTestCase { } @MainActor - func test_edit_order_button_is_visible_and_navigates_to_edit_order_screen() throws { + func test_edit_order_button_is_visible_and_navigates_to_edit_order_screen_when_order_synced() throws { // Given let presentationVerifier = PresentationVerifier() let storageManager = MockStorageManager() let order = MockOrders().sampleOrder() let storesManager = OrderDetailStoreManagerFactory.createManager(order: order) - let viewModel = OrderDetailsViewModel(order: order, stores: storesManager, storageManager: storageManager) + let viewModel = OrderDetailsViewModel(order: order, + stores: storesManager, + storageManager: storageManager, + syncStateController: OrderDetailsSyncStateController(syncState: .synced)) let viewController = OrderDetailsViewController(viewModel: viewModel) // When