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 fatal error if store currency is changed after enabled currencies are set #2078

Conversation

jessepearson
Copy link
Contributor

Fixes #2068 #2004

Changes proposed in this Pull Request

  • Removed loop that created enabled currencies from available currencies.
  • Add store currency into enabled currencies.
  • Added option query to get stored currencies with a fallback to mock currencies.
  • Updated tests to use new option, and to match what was available/enabled.

Testing instructions

  • Navigate to WooCommerce > Settings > General.
  • Set your store currency to USD.
  • Navigate to WooCommerce > Settings > Multi-Currency.
  • Use Add Currencies and enable CAD and EUR.
  • Navigate to WooCommerce > Settings > General.
  • Set your store currency to EUR.
  • Navigate to WooCommerce > Settings > Multi-Currency.
  • Everything should still work.

  • 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)

@jessepearson jessepearson added the component: customer multi-currency Issues related to customer multi-currency project label Jun 8, 2021
@jessepearson jessepearson self-assigned this Jun 8, 2021
@jessepearson jessepearson requested a review from a team June 8, 2021 15:34
@luizreis luizreis mentioned this pull request Jun 8, 2021
3 tasks
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.

Looks great! I've added a couple of comments regarding some tests, but I'm pre-approving this. Feel free to :shipit: once they're addressed 🚀

Comment on lines +265 to 267
if ( ! isset( $available_currencies[ $code ] ) ) {
continue;
}
Copy link
Contributor

@luizreis luizreis Jun 8, 2021

Choose a reason for hiding this comment

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

Would you mind adding a test for this change? Something like this would be enough:

public function test_enabled_but_unavailable_currencies_are_skipped() {
	update_option( 'wcpay_multi_currency_enabled_currencies', [ 'RANDOM_CURRENCY', 'USD' ] );

	// Recreate Multi_Currency instance to use the recently set currencies.
	$this->reset_multi_currency_instance();
	$this->multi_currency = WCPay\Multi_Currency\Multi_Currency::instance();

	$this->assertSame( [ 'USD' ], array_keys( $this->multi_currency->get_enabled_currencies() ) );
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in b9a7c71 , thanks for the example, I applied it directly 😁

@@ -87,8 +92,6 @@ function () {
}

public function test_get_enabled_currencies_returns_correctly() {
update_option( 'wcpay_multi_currency_enabled_currencies', $this->mock_enabled_currencies );
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that the options are defined during the test's setUp, could we remove the Multi_Currency instance reset from this test?

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 catch, I missed that. Updated in b9a7c71

@jessepearson jessepearson merged commit 4dbb6e9 into update/multi-currency-phpunit-tests Jun 8, 2021
@jessepearson jessepearson deleted the fix/2068-fatal-on-store-currency-change branch June 8, 2021 17:56
jessepearson added a commit that referenced this pull request Jun 8, 2021
* Added phpunit tests for Customer Multi-Currency.

* Fix fatal error if store currency is changed after enabled currencies are set (#2078)

* Fix fatal error if store currency is changed after enabled currencies set, update tests.

* Fix fatal error if store currency is changed after enabled currencies set, update tests.

* Additional test for multi-currency, clean up of other tests.
moon0326 pushed a commit that referenced this pull request Jun 16, 2021
* Added phpunit tests for Customer Multi-Currency.

* Fix fatal error if store currency is changed after enabled currencies are set (#2078)

* Fix fatal error if store currency is changed after enabled currencies set, update tests.

* Fix fatal error if store currency is changed after enabled currencies set, update tests.

* Additional test for multi-currency, clean up of other tests.
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
Development

Successfully merging this pull request may close these issues.

2 participants