Enable deferred intent creation UPE for SEPA #7232
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
…into feature/enable-sepa-for-deferred-intent-upe
timur27
force-pushed
the
feature/enable-sepa-for-deferred-intent-upe
branch
from
01a8998 to
df9e9bf
Compare
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: 0 B Total Size: 1.41 MB ℹ️ View Unchanged
|
timur27
changed the title
timur27
requested review from
a team and
frosso
and removed request for
a team
frosso
approved these changes
Sep 20, 2023
| * @return boolean True if mandate must be shown and acknowledged by customer before deferred intent UPE payment can be processed, false otherwise. | ||
| */ | ||
| public function does_payment_method_require_mandate_data() { | ||
| $is_link_payment = Payment_Method::CARD === $this->get_selected_stripe_payment_type_id() && in_array( Payment_Method::LINK, $this->get_upe_enabled_payment_method_ids(), true ); |
There was a problem hiding this comment.
Is it accurate to say that $is_link_payment could be true if the payment method was CC and the merchant enabled Stripe Link? (i.e.: does this check accurately tell us if the customer paid with Stripe Link, and not just with CC?)
And would it matter?
There was a problem hiding this comment.
Is it accurate to say that $is_link_payment could be true if the payment method was CC and the merchant enabled Stripe Link?
Yes. But it'd be inaccurate to say that $is_link_payment could be true if and only if the payment method was CC and the merchant chose Stripe Link. Because for the use-case, when someone's paying with CC w/o choosing Stripe Link (but having it enabled), $is_link_payment will be true as well.
does this check accurately tell us if the customer paid with Stripe Link, and not just with CC?
Unfortunately, no.
The check will be true for Stripe Link enabled, even if it's not used. Consequently, the mandate_data will be sent for plain CC payments as well. This logic is being followed to support the process. I think that the Stripe Link initialisation logic on the back-end has a room for improvement and should be further improved/refined.
And would it matter?
mandate_data is added to the payment intent request only in case we're confirming the intent, and Stripe uses it to validate that we've shown the terms to a user before the order placement. I think Stripe takes this parameter only if it's needed (e.g. when SEPA payment is used) and checks if we've sent it. For card payments, this parameter will be redundant and in my opinion won't be checked by Stripe.
I think I need to change $is_link_payment name to something like $is_link_enabled to better reflect the reality. (done in 361cef7) Other than that, I'm happy to refine this existing Stripe Link logic further if needed. Let me know your thoughts!
There was a problem hiding this comment.
Thank you for the explanation! I like the rename you've done, I think it'll provide better clarity for future wanderers and adventurers 👍
| * | ||
| * @return boolean False since card payment does not require mandate. | ||
| */ | ||
| protected function does_payment_method_require_mandate_data() { |
There was a problem hiding this comment.
(minor) just as an alternative with the is_/has_ prefixes convention that is present in most of the codebase, what are your thoughts on naming this new method to is_mandate_data_required?
There was a problem hiding this comment.
Sure, makes total sense, thank you! 765db74
timur27
force-pushed
the
feature/enable-sepa-for-deferred-intent-upe
branch
from
21e814b to
765db74
Compare
frosso
approved these changes
Sep 20, 2023
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 #7113
Changes proposed in this Pull Request
After SEPA has been disabled as a reusable payment method (including Subscriptions) in #7107, this PR removes the fallback from the deferred intent creation UPE to legacy UPE so that SEPA can be seamlessly used with the deferred intent creation UPE.
Testing instructions
Legacy UPE
Payments -> SettingsDeferred intent creation UPE
Payments -> Settingsnpm 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