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

Fix/issue 270 estimated order amount multicurrency #213

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

alexiglesias31
Copy link

@alexiglesias31 alexiglesias31 commented Sep 18, 2023

All Submissions:

  • Does your code follow the Extendables standards?
  • Have you written new tests for your changes, as applicable?
  • Have you successfully run tests with your changes locally?

Changes proposed in this Pull Request:

Disable the estimated order amount if the currency is not the default currency for the region.

How to test the changes in this Pull Request:

  1. Install a built version of this branch and setup the amazon plugin to Europe region.
  2. Install the plugin FOX - Currency Switcher Professional for WooCommerce.
  3. Estimated order amount should be set when the currency is EUR but should not be set when the currency is USD.
  4. The checkout can be completed without issues.

Other information:

  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?

Changelog entry

Fix - Issue with estimated order amount and multi-currency.

@alexiglesias31 alexiglesias31 self-assigned this Sep 18, 2023
Copy link

@dpanta94 dpanta94 left a comment

Choose a reason for hiding this comment

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

Small comment and additonally im not sure if we should completely unset the estimateOrderAmount property from the button's payload on frontend (insatead of just providing an empty value). I remember the docs stating that in staging mode, having an error in estimated order amount property should produce a non blocking console error.

@@ -51,4 +51,24 @@ public static function get_create_checkout_classic_session_config( array $payloa
'signature' => $signature,
);
}

public static function is_region_supports_shop_currency( $region, $currency = false ) : bool {

Choose a reason for hiding this comment

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

Why here we take as param the region instead of figuring it out within the method like done in production code ?

@alexiglesias31
Copy link
Author

alexiglesias31 commented Sep 20, 2023

I didn't know about that, yes, I tested with an empty string and works as well 👍

P.D: 🤔, giving a second thought to this, it doesn't feel correct to me that the estimated order amount is empty when the currency is different. I would find more intuitive to pass the converted amount rather than an empty string.

@alexiglesias31
Copy link
Author

Providing a new approach, instead of set the total as empty, moving back to return an empty string on the estimated_order_amount, but now adding some checks in the JS code so the parameter is not set when is empty.

Copy link

@dpanta94 dpanta94 left a comment

Choose a reason for hiding this comment

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

LGTM!

@alexiglesias31
Copy link
Author

@dpanta94 asking CR again, since the EOA was still being calculated on blocks.

Copy link

@dpanta94 dpanta94 left a comment

Choose a reason for hiding this comment

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

Looks good @alexiglesias31. The latest change about blocks is alerady considered on classic with blocks ?

asking mostly cause i see you are adding a variable in both classic and express from the backend but on frontend there are only changes for express.

@alexiglesias31
Copy link
Author

I think so 😄, EOA is not used on the JS related to classics

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.

2 participants