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

Multicurrency: Do not modify deposit percentage for display. #7247

Merged
merged 3 commits into from
Oct 23, 2023
Merged

Multicurrency: Do not modify deposit percentage for display. #7247

merged 3 commits into from
Oct 23, 2023

Conversation

peterwilsoncc
Copy link
Contributor

@peterwilsoncc peterwilsoncc commented Sep 21, 2023

Fixes: WooCommerce Deposits #\506
Supersedes PR #6930

Changes proposed in this Pull Request

As reported in WooCommerce Deposits #\506, enabling multi-currency modifies the display of percentage deposits.

The issue comes from WooCommerceDeposits::modify_cart_item_deposit_amount_meta() attempting to convert a percentage number although it were currency.

A store with a USD base selling a product with a 20% deposit results in the following displaying for select currencies:

AUD: 31%
CAD: 27%
INR: 1656%

I haven't written any tests as there are existing tests for multicurrency that are passing but as the plugin is using mocks the tests are passing as they're failing to account for the various filters running in this and deposits.

This is a sibling PR for https://github.com/woocommerce/woocommerce-deposits/pull/544

Product page
Screen Shot 2023-09-21 at 2 18 54 pm

Cart page

Screen Shot 2023-09-21 at 2 19 31 pm

Checkout page

Screen Shot 2023-09-21 at 2 20 21 pm

Account > Order page

Screen Shot 2023-09-21 at 2 20 58 pm

Testing instructions

  1. Configure Multi-currency Payments: using USD (default) and INR will best demonstrate the bug
  2. Check out the Deposits extension branch in https://github.com/woocommerce/woocommerce-deposits/pull/544
  3. Build the Deposits extension following the instructions in the readme.
  4. Configure and publish product to accept deposits:
    • Enable Deposits: Yes - Deposits are optional
    • Deposit Type:: percentage
    • Deposit Amount: 20
    • Default Deposit Selected Type: Pay Deposit
  5. View the product on the front end
  6. Switch to USD if required, observe the deposit amount is displayed as 20%
  7. Switch the currency from USD to INR
  8. On this branch the deposit amount should remain 20%, on trunk the amount is significantly over 20%

  • Run npm run changelog to add a changelog file, choose patch to leave it empty if the change is not significant. You can add multiple changelog files in one PR by running this command a few times.
  • Covered with tests (or have a good reason not to test in description ☝️)
  • Tested on mobile (or does not apply)

Post merge

@peterwilsoncc peterwilsoncc marked this pull request as draft September 21, 2023 03:52
@peterwilsoncc peterwilsoncc marked this pull request as ready for review September 21, 2023 04:28
@Mayisha Mayisha self-requested a review September 28, 2023 14:13
@vikrampm1
Copy link

@Mayisha checking in here to see if you had a chance to review.

@Mayisha
Copy link
Contributor

Mayisha commented Oct 10, 2023

@vikrampm1 I have reviewed both the PRs and the issue I previously found is not happening anymore. I will go ahead and approve the PRs.

Copy link
Contributor

@Mayisha Mayisha left a comment

Choose a reason for hiding this comment

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

Along with the fix made in the Deposits plugin, the changes here seem to fix the deposit percentage issue for a percentage deposit set in individual products.

@jeffpaul
Copy link

@Mayisha please let us know if this gets milestoned into a plugin release so we can coordinate accordingly in the Deposits extension... thanks!

@Mayisha
Copy link
Contributor

Mayisha commented Oct 20, 2023

@jeffpaul We are planning to ship this in the next release which is scheduled on Nov 01.

@Mayisha Mayisha added this pull request to the merge queue Oct 23, 2023
Merged via the queue into Automattic:develop with commit dec5477 Oct 23, 2023
20 of 27 checks passed
@peterwilsoncc peterwilsoncc deleted the deposits/506-percentage-display branch November 28, 2023 01:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants