Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Order editing] Non-default currency order edit notice #14345

Open
wants to merge 4 commits into
base: trunk
Choose a base branch
from

Conversation

joshheald
Copy link
Contributor

@joshheald joshheald commented Nov 7, 2024

Closes: #14304

Description

This PR disables order editing for orders with a different currency compared to the site's default.

This is a workaround. 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.

Steps to reproduce

  1. Launch the app and switch to a multi currency store
  2. Go to the orders tab and select an order in a different currency from the store's default
  3. Tap edit
  4. Observe that you're shown a notice explaining that you can't edit in the app

Repeat for an order in your store's default currency – observe that you can edit it as normal.

Testing information

I added some unit tests for this, but the reliance on whether the order is synced or not was tricky. I effectively exposed that property to be mockable in tests, which is a little strange but it would have made for very brittle tests to actually allow the syncEverything function to work via lots of mocking.

I tested on an iPhone running iOS 18.1, and an iPad running iOS 17.7

Tracks event registered.

Note that I also see issues when creating an order – the order creation flow shows my site's default currency (GBP), but the order is then created in USD. I'm not really sure why that is, and it seems more drastic to disable order creation than order editing... so I've treated it as out of scope for this ticket, but will find out why and create another issue if needed.

Screenshots

edit-order-notice.mp4

  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

Reviewer (or Author, in the case of optional code reviews):

Please make sure these conditions are met before approving the PR, or request changes if the PR needs improvement:

  • The PR is small and has a clear, single focus, or a valid explanation is provided in the description. If needed, please request to split it into smaller PRs.
  • Ensure Adequate Unit Test Coverage: The changes are reasonably covered by unit tests or an explanation is provided in the PR description.
  • Manual Testing: The author listed all the tests they ran, including smoke tests when needed (e.g., for refactorings). The reviewer confirmed that the PR works as expected on all devices (phone/tablet) and no regressions are added.

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.
@joshheald joshheald added type: bug A confirmed bug. feature: order creation All tasks related to creating an order labels Nov 7, 2024
@joshheald joshheald added this to the 21.1 milestone Nov 7, 2024
@dangermattic
Copy link
Collaborator

dangermattic commented Nov 7, 2024

1 Warning
⚠️ This PR is assigned to the milestone 21.1. This milestone is due in less than 2 days.
Please make sure to get it merged by then or assign it to a milestone with a later deadline.
1 Message
📖

This PR contains changes to Tracks-related logic. Please ensure (author and reviewer) the following are completed:

  • The tracks events must be validated in the Tracks system.
  • Verify the internal Tracks spreadsheet has also been updated.
  • Please consider registering any new events.
  • The PR must be assigned the category: tracks label.

Generated by 🚫 Danger

@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Nov 7, 2024

WooCommerce iOS📲 You can test the changes from this Pull Request in WooCommerce iOS by scanning the QR code below to install the corresponding build.

App NameWooCommerce iOS WooCommerce iOS
Build Numberpr14345-d7d69c5
Version21.0
Bundle IDcom.automattic.alpha.woocommerce
Commitd7d69c5
App Center BuildWooCommerce - Prototype Builds #11482
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@joshheald joshheald marked this pull request as ready for review November 7, 2024 11:25
@joshheald joshheald added the category: tracks Related to analytics, including Tracks Events. label Nov 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: tracks Related to analytics, including Tracks Events. feature: order creation All tasks related to creating an order type: bug A confirmed bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Editing Order Details - Incorrect Currency Displayed When Order Currency Differs from Store Default
3 participants