chore: cleanup of PRB implementation in favor of ECE #9698
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Test the buildOption 1. Jetpack Beta
Option 2. Jurassic Ninja - available for logged-in A12s🚀 Launch a JN site with this branch 🚀 ℹ️ Install this Tampermonkey script to get more options. Build info:
Note: the build is updated when a new commit is pushed to this PR. |
|
Size Change: -16.8 kB (-1%) Total Size: 1.34 MB
ℹ️ View Unchanged
|
asumaran
reviewed
Nov 12, 2024
| @@ -207,9 +205,9 @@ export default class WCPayAPI { | |||
| result.error.setup_intent.id ); | |||
|
|
|||
| // In case this is being called via payment request button from a product page, | |||
There was a problem hiding this comment.
In case this is being called via payment request button from a product page
If this line is meant for cases where the Payment Request Button is called from a product page, it might no longer be relevant since you’re removing the PRB implementation, right?
Additionally, to align with the comment, shouldn’t the code try to use getConfig first, like this?
const ajaxUrl = getConfig( 'ajaxUrl' ) ?? getExpressCheckoutConfig( 'ajax_url' );There was a problem hiding this comment.
Those are good questions, and I honestly don't know the answers to them :D
I couldn't find anything in the PRB implementation that would trigger these functions, so I'm not sure when or the reasons why this was introduced.
The comment says:
the getConfig function won't work
So it would make sense to have getPaymentRequestData be called first.
It could be that the implementation over the years changed so much that this would have no longer been relevant anymore in either case.
When a payment is being attempted, the backend could return a redirect_url with a #wcpay-confirm-....hash prefix, used to confirm the PI. In turn, the JS will call this function. The backend has the ability to return such URL for PRBs, ECE, and any other payment method. So I'd be wary of removing it or changing it, since theconfirmIntent()` function could be called from the ECE as well.
frosso
changed the title
frosso
requested review from
a team and
timur27
and removed request for
a team
frosso
commented
Nov 12, 2024
client/settings/express-checkout-settings/general-payment-request-button-settings.js
Show resolved
Hide resolved
client/settings/express-checkout-settings/payment-request-button-preview.js
Show resolved
Hide resolved
timur27
reviewed
Nov 12, 2024
client/settings/express-checkout-settings/general-payment-request-button-settings.js
Show resolved
Hide resolved
timur27
reviewed
Nov 13, 2024
timur27
reviewed
Nov 13, 2024
There was a problem hiding this comment.
@frosso I reviewed the code which looks good, I left a few questions and one suggestion to remove the ajax method if it's not used anymore.
No more code comments from me at the moment, I will start testing.
PS I also see that tests started to fail after the most recent commit.
includes/express-checkout/class-wc-payments-express-checkout-ajax-handler.php
Outdated
Show resolved
Hide resolved
includes/express-checkout/class-wc-payments-express-checkout-button-display-handler.php
Show resolved
Hide resolved
timur27
approved these changes
Nov 13, 2024
There was a problem hiding this comment.
I went through the testing steps outlined in the description as well as tested the subscriptions, incl. lack of ECE for free trial physical subscription as per instructions here. Things work well.
The comments I've left are non-blocking so I'm approving the PR while you're responding to them and fixing the failing e2e tests.
|
@timur27 I see similar failures from that suite also on other PRs. I asked for assistance here: p1731518139549869-slack-CGGCLBN58 I'll hold off until we get clarity, but it looks like the failures are not related to these changes. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #8950
Fixes #8529
Changes proposed in this Pull Request
Cleaning up the PRB implementation in favor of the ECE implementation.
Maintaining the tokenized payment request implementation behind a feature flag for future reference.
Within this PR:
client/checkout/api/index.jswith thepaymentRequest*prefix to have theexpressCheckoutECEprefix insteadclient/checkout/api/index.jswith thepaymentRequestprefix that were no longer usedece_*prefixisExpressCheckoutElementEnabled,isStripeEceEnabled,PaymentRequestButtonElement,WC_Payments_Features::is_stripe_ece_enabled(),_wcpay_feature_stripe_eceincludes/class-wc-payments-payment-request-button-handler.php, but kept some of its logic for the tokenized carts. Keep in mind that some of those handlers are already duplicated inincludes/express-checkout/class-wc-payments-express-checkout-ajax-handler.phpTesting instructions
developand this branchnpm run changelogto add a changelog file, choosepatchto leave it empty if the change is not significant. You can add multiple changelog files in one PR by running this command a few times.Post merge