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 for fatal error when saving changes on Multi-Currency page #2182

Merged
merged 5 commits into from
Jun 16, 2021

Conversation

dmallory42
Copy link
Contributor

@dmallory42 dmallory42 commented Jun 15, 2021

Fixes #2180

Changes proposed in this Pull Request

  • Fixes a fatal error that occurs when clicking Save Changes on the main Multi-Currency page (with the currency list) by hiding the save changes button on this page.

Testing instructions

  • Checkout this branch. Go to the main Settings -> Multi Currency page.
  • Verify the Save Changes button is no longer there.
  • Verify the Save Changes button is present on the "edit" page for a single currency.
  • Verify the code changes make sense.

  • Added changelog entry (or does not apply)
  • Covered with tests (or have a good reason not to test in description ☝️)
  • Tested on mobile (or does not apply)

@dmallory42 dmallory42 requested a review from a team June 15, 2021 09:48
@dmallory42 dmallory42 added component: customer multi-currency Issues related to customer multi-currency project pr: needs review labels Jun 15, 2021
Copy link
Contributor

@luizreis luizreis left a comment

Choose a reason for hiding this comment

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

LGTM! I left a comment regarding a future section, but I'm pre-approving this. :shipit:

$available_currencies = $this->multi_currency->get_available_currencies();
update_option( $this->id . '_manual_rate_' . $current_section, $available_currencies[ strtoupper( $current_section ) ]->get_rate() );
// If we are saving the settings for an individual currency, we have some additional logic.
if ( '' !== $current_section ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Soon we'll also introduce the store section. Do you think we should also include a check for it here in case we forget to do it when enabling it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea! Updated in ccbeab3.

Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome! :shipit:

@dmallory42 dmallory42 merged commit 284d119 into develop Jun 16, 2021
@dmallory42 dmallory42 deleted the fix/2180-fatal-error branch June 16, 2021 10:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: customer multi-currency Issues related to customer multi-currency project pr: ready to merge
Projects
None yet
2 participants