-
Notifications
You must be signed in to change notification settings - Fork 69
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
Integrate product and cart pages with Payment Request Button #1175
Conversation
7fef928
to
aacc28a
Compare
27d6715
to
f7df576
Compare
@dechov Until your last commit, everything looks great to me. Now we need to get this PR merged, and then the child PR #1239 without a release in between. I've removed myself from assignees, so other people can jump in as reviewers without thinking that I'm reviewing this PR. Feel free to review my changes as well, or edit the PR description. |
@ricardo Haven't tested your commits but the code changes look good to me. |
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.
This has been reviewed and tested enough, so let's merge and ship! 🚀
0d90131
to
9a115ba
Compare
includes/class-wc-payments.php
Outdated
@@ -150,6 +151,8 @@ public static function init() { | |||
|
|||
self::$gateway = new $gateway_class( self::$api_client, self::$account, self::$customer_service, self::$token_service, self::$action_scheduler_service ); | |||
|
|||
new WC_Payments_Payment_Request( self::$account ); |
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.
As mentioned in the Slack thread, this is a bit confusing and implies there are side effects in the constructor (which is an antipattern we've been falling into in other parts of the plugin, so that's probably ok here).
But could we consider the following:
- Rename it to something like
WC_Payments_Payment_Request_Button_Handler
or similar? The double "payment" and the word "request" could imply at first that this is making a regular HTTP payment request to the API - Assign it to a variable, similar to the other classes instantiated here
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 agree. This was adapted from a previous implementation in Stripe. We should probably consider refactoring it somehow, but given the deadline for this feature, I've renamed and assigned it to a variable (even though we do nothing with that variable).
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.
Hey @marcinbot I had to rebase and adapt the child PR #1239. Could you please take a look at the last commit where I do the same thing with the Apple_Pay_Registration class, assigning it to a variable? I just didn't rename the class.
$this->gateway_settings = array_merge( self::get_default_settings(), get_option( 'woocommerce_woocommerce_payments_settings', [] ) ); | ||
$this->account = $account; | ||
$this->testmode = ( ! empty( $this->gateway_settings['test_mode'] ) && 'yes' === $this->gateway_settings['test_mode'] ) ? true : false; | ||
$this->total_label = ! empty( $this->account->get_statement_descriptor() ) ? $this->account->get_statement_descriptor() : ''; |
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.
It's possible this is the bit that's responsible for the early request that causes the e2e errors. The account cache is missing, so it tries to make an API request before the rest of this and other plugins have finished loading.
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.
Thanks for digging into this 🙏
I've taken that out of the constructor and put it into a function. I also refactored the code a little bit, because this if ! empty( $this->account->get_statement_descriptor() ) ?
is not necessary since get_statement_descriptor()
already returns an empty string in case the value is not set.
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.
Looks better. I also noticed two things that could use a refactor, but I also see you mentioned the deadline. So I'll leave it up to you to decide whether to implement them. If you decide not to do that in this PR, then maybe we should open a follow-up one right after shipping this one. Or at least add a comment to refactor the new class.
I also haven't had a chance to test it, so will trust that you and @reykjalin handled that :)
// Add default settings in case the payment request options are missing. | ||
$this->gateway_settings = array_merge( self::get_default_settings(), get_option( 'woocommerce_woocommerce_payments_settings', [] ) ); | ||
$this->account = $account; | ||
$this->testmode = ( ! empty( $this->gateway_settings['test_mode'] ) && 'yes' === $this->gateway_settings['test_mode'] ) ? true : false; |
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.
This should be camelcase snake case test_mode
but could also be retrieved on the fly to respect the filters/dev mode that the gateway class implements, i.e. $this->gateway->is_in_test_mode()
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.
@marcinbot Do you mean $this->testmode
should be slug snake case $this->test_mode
?
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.
Yep, sorry, my bad. I wrote one thing while thinking of the other :) But if we do inject the gateway as I suggested in the previous comment, then there's probably no need to have that field at all - we could just check the return value of is_in_test_mode
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 see that there's a get_gateway
function too. We should probably use that instead - WC_Payments::get_gateway()-> is_in_test_mode()
.
As I mentioned in the comment above, I'll open a follow-up PR.
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 see that there's a get_gateway function too. We should probably use that instead - WC_Payments::get_gateway()-> is_in_test_mode().
This could work, if passing around the gateway instance doesn't for any reason. It's fine to open a followup PR.
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.
Honestly, I think using the WC_Payments::get_gateway()
function is the way to go. It removes some of the noise of adding a class variable just for a single function.
I also realize we can do the same for the Tracks events; I added a class variable for the gateway just to check for dev/test mode. Should've used this instead; it feels muuuuch cleaner.
@marcinbot @ricardo I just unresolved one of the conversations I think will be relevant for future work on this. I agree with @marcinbot's assessment that we should open issue/s or PR/s ASAP to address things in these PRs we know we want to improve. Things like how we pass around the |
* Copy payment request module directly from Stripe gateway extension * Load payment request module * Replace key with account * Use unique DOM identifiers * Make global JS variable name unique * Rename JS container object * Omit suffix from script path * Use WCPay gateway for processing payment * Pass payment method instead of source * Use WCPay gateway settings * Use WCPay utils * Update textdomain for i18n to WCPay * Make AJAX actions unique * Automatically fix sniff violations with phpcbf Automatically fix 73 of these code sniffing violations: - Short array syntax must be used to define arrays - Array double arrow not aligned correctly - Usage of ELSE IF is discouraged; use ELSEIF instead - Line indented incorrectly - Equals sign not aligned with surrounding assignments - Concat operator must be surrounded by a single space * Adjust module header * Fix 'Unexpected var, use let or const instead' lint errors * Fix 'Identifier … is not in camel case' lint errors * Fix JSDoc lint errors * Fix remaining eslint issues * Fix various PHPCS errors * Fix more PHPCS issues * Make nonce actions unique to WCPay * Pass publishable key for current mode * Initialize payment request module after gateway is set to avoid fatal * Add remaining Payment Request Button settings * Use new WCPay-specific filters * Fix remaining PHPCS issues * Remove @SInCE and @Version from comments * Fix remaining ESLint issues * Remove @SInCE and @Version from JSDoc * Fix custom payment request button not showing * Add payment request styles * Fix internal dependency docblock * Fix stylelint errors * Add JS to toggle payment request options settings * Fix default settings and missing settings mismatch * Small fix - same event getting triggered error * Add changelog entry * Copy tests from Stripe * Modify tests to suite WCPay * Use prepare_amount function * Sync payment request default values * Fix early account request * Rename payment_request class and assign to var * Rename files Co-authored-by: Ricardo Metring <ricardometring@gmail.com>
Fixes #1072
Changes proposed in this Pull Request
Copies (ee5a56e) and adapts the Payment Request integration code from the Stripe extension [PHP, JS] to the context of WCPay – the goal is to provide identical functionality to the existing battle-hardened integration.
All available tests were copied and adapted from the Stripe Gateway extension. The client doesn't have tests yet, but we plan to sync here once we add new tests in the Stripe repository.
Testing instructions
For the purpose of this test, you can test either Google Pay, using Chrome, or Microsoft Pay, if you use Windows.
For Apple Pay, the domain needs to be registered with Apple first. #1239 will take care of that.
Screenshots
Chrome - Branded - Text and logo
Chrome - Branded - Logo only
Default - Buy - Dark
Default - Donate - Dark
Default - Buy - Light
Default - Buy - Light-Outline
Custom
On Microsoft Edge it defaults to Stripe's default button style.