-
Notifications
You must be signed in to change notification settings - Fork 15
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/167: Add WooPayments compatibility #201
Conversation
if ( isset( $_GET['currency'] ) ) { // phpcs:ignore WordPress.Security.NonceVerification.Recommended | ||
$currency_code = array( | ||
sanitize_text_field( | ||
wp_unslash( $_GET['currency'] ) // phpcs:ignore WordPress.Security.NonceVerification.Recommended | ||
), | ||
); | ||
// Check if the currency is set in the session (for logged-out users). | ||
} elseif ( 0 === $user_id && WC()->session ) { | ||
$currency_code = WC()->session->get( \WCPay\MultiCurrency\MultiCurrency::CURRENCY_SESSION_KEY ); | ||
// Check if the currency is set in the user meta (for logged-in users). | ||
} elseif ( $user_id ) { | ||
$currency_code = get_user_meta( $user_id, \WCPay\MultiCurrency\MultiCurrency::CURRENCY_META_KEY ); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not familiar with how multi-currency works so this feedback may not make sense. But wondering if there's a scenario where the user session or user meta hasn't been set and thus the currency code ends up being empty here?
I guess wondering if we should separate this code into two if
statements. So we check for the currency value from the URL first and store that in the $currency_code
variable. And then we overwrite that value from session or user meta, if that value exists in either. Something like:
$currency_code = '';
if ( isset( $_GET['currency'] ) ) { // phpcs:ignore WordPress.Security.NonceVerification.Recommended
$currency_code = array(
sanitize_text_field(
wp_unslash( $_GET['currency'] ) // phpcs:ignore WordPress.Security.NonceVerification.Recommended
),
);
}
// Check if the currency is set in the session (for logged-out users).
if ( 0 === $user_id && WC()->session ) {
$stored_code = WC()->session->get( \WCPay\MultiCurrency\MultiCurrency::CURRENCY_SESSION_KEY );
// Not sure the best check on this value? Should it always be an array and we can check for that? Can it sometimes be a string?
if ( $stored_code ) {
$currency_code = $stored_code;
}
// Check if the currency is set in the user meta (for logged-in users).
} elseif ( $user_id ) {
$stored_code = get_user_meta( $user_id, \WCPay\MultiCurrency\MultiCurrency::CURRENCY_META_KEY );
// Not sure the best check on this value? Should it always be an array and we can check for that? Can it sometimes be a string?
if ( $stored_code ) {
$currency_code = $stored_code;
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dkotter I've fixed the confusion between the return value types array
vs string
. There was a small bug, thanks for pointing that out.
Regarding separating if statements into 2, the problem is that in Block Checkout, the user meta is saved after the block is rendered. So the block checkout page requires an additional reload for Payfast to appear in the list of gateways. We give precedence to the GET parameter, if that is set, then we use it, because ultimately that gets saved to the session and user meta.
QA/Test Report- ✅Testing Environment -
Test Results - Functional Demo / Screencast - PayFast payment gateway is available on checkout when customers use 'Rand' as a currency from WooPayment currency selection option. Recording.577.mp4Next Step- Ready to UAT Testing Documentation Status- Cases related to this PR/Issue, are added to Critical Flow Wiki Pages:
|
Regression+Smoke Test Report- ✅Testing Environment -
Tested with Archive File created via
Please note that this plugin has been tested with the build created by the specified versions ☝🏼 of Composer, Node, and NPM.Status- Working as expected. Ready to merge 🚀 |
All Submissions:
Changes proposed in this Pull Request:
Closes #167 .
Steps to test the changes in this Pull Request:
ZAR
to WooPayments multi-currency.ZAR
, add a product to cart and visit checkout page.Changelog entry