Send bank statement descriptors to payment intents #3643
Conversation
cesarcosta99
added
status: blocked
|
Waiting for #3625 getting merged before requesting review for this, since the work here depends and it's based on it. |
deepakpathania
added
category: core
cesarcosta99
force-pushed
the
update/send-statement-descriptors-on-pi
branch
from
2bfa68f
to
ca779b6
Compare
cesarcosta99
changed the base branch from
develop
to
update/statement-descriptors
|
Rebased |
cesarcosta99
force-pushed
the
update/send-statement-descriptors-on-pi
branch
2 times, most recently
from
84ffc42
to
86b05a7
Compare
cesarcosta99
removed
the
status: blocked
|
Rebased |
There was a problem hiding this comment.
Now that I understand better how the full statement descriptor works, I believe we should make the short statement descriptor work the same way, and not try to save it under the plugin settings, but use the Stripe account for that, or at least explore this possibility, otherwise it can be hard to update this later and make it backwards compatible.
If we end up not having time to merge this with the statement descriptor prefix coming from the Stripe account, we could probably hide #3625 and merge this in the next WCPay version IMO (please check with others as well).
With the current implementation this tests well, despite one test detailed below. This is what I tested:
- Short statement descriptor:
- non-UPE:
- ✅ Blocks checkout.
- ✅ Classic checkout.
- ✅ Pay for order page.
- UPE:
- ✅ Blocks checkout.
- ✅ Classic checkout.
- ❌ Pay for order page.
- ✅ Payment Request Button checkout.
- ✅ Checkout with saved payment method.
- non-UPE:
- Full statement descriptor:
- non-UPE:
- ✅ Blocks checkout.
- ✅ Classic checkout.
- ✅ Pay for order page.
- UPE:
- ✅ Blocks checkout.
- ✅ Classic checkout.
- ✅ Pay for order page.
- ✅ Payment Request Button checkout.
- ✅ Checkout with saved payment method.
- non-UPE:
Reproduction steps for ❌ Pay for order page:
- Make sure UPE is enabled and short statement descriptor is enabled.
- Go to WooCommerce > Orders > Add order.
- Add an item to the order, e.g Album.
- Click Create.
- Click "Customer payment page →".
- Checkout with a new credit card.
- Notice the full statement descriptor is used, instead of the short one.
| public function get_statement_descriptor() { | ||
| return $this->statement_descriptor; | ||
| } |
There was a problem hiding this comment.
Do we plan to use this in the future? I didn't find it being used.
There was a problem hiding this comment.
No, removed it in 81e755a.
| @@ -1111,6 +1111,8 @@ public function process_payment_for_order( $cart, $payment_information, $additio | |||
| // phpcs:ignore WordPress.Security.NonceVerification.Missing | |||
| $platform_checkout_intent_id = sanitize_user( wp_unslash( $_POST['platform-checkout-intent'] ?? '' ), true ); | |||
|
|
|||
| $statement_descriptor = $this->get_statement_descriptor( $order->get_payment_method(), $order ); | |||
There was a problem hiding this comment.
Now that you confirmed that the full statement descriptor comes directly from Stripe, perhaps we don't want to pass it to the payment intent, unless it needs to be changed.
The full statement descriptor should always be the same from the Stripe dashboard, correct?https://dashboard.stripe.com/test/connect/accounts/{acct_id}/identity
I suppose what differs from the Stripe plugin now is that we don't (or can't), set the full statement descriptor for the standard Stripe account directly, so that's why we allow a custom statement descriptor from the plugin settings (I didn't check this assumption).
Since we can set the express account statement descriptor in WCPay, we may not want to pass it along with the request, since it will always default to its full form anyway. We also may not want to do that to not risk providing an invalid statement descriptor somehow.
Now that I look at the Stripe docs more carefully, we may actually want to save the short statement descriptor in the account instead: https://stripe.com/docs/api/accounts/object#account_object-settings-card_payments-statement_descriptor_prefix
And then pass a suffix.
So in the end, this could be refactored like so:
$statement_descriptor_suffix = $this->maybe_use_statement_descriptor_suffix( $order->get_payment_method(), $order );public function maybe_use_statement_descriptor_suffix( $payment_method, $order ) {
$statement_descriptor_prefix = $this->get_account_statement_descriptor_prefix();
$is_short_statement_descriptor_enabled = 'yes' === $this->get_option( 'is_short_statement_descriptor_enabled' );
if ( in_array( $payment_method, [ 'card', 'woocommerce_payments' ], true ) && $is_short_statement_descriptor_enabled && ! empty( $statement_descriptor_prefix ) ) {
// TODO: Refactor this:
return WC_Payments_Utils::get_dynamic_statement_descriptor( $order );
}
return null;
}get_account_statement_descriptor_prefix would grab the statement descriptor prefix from the account, similar to get_account_statement_descriptor, but probably without logging errors.
Then this function should only return the * #123 part, which is the suffix.
And then we need to rename the additional parameter statement_descriptor to statement_descriptor_suffix.
Note: I didn't test the code above.
There was a problem hiding this comment.
perhaps we don't want to pass it to the payment intent, unless it needs to be changed.
Agreed. I tweaked the logic here to only return it when payment method is not a card, which doesn't set a statement descriptor even if it's set on Stripe – that's why I also recommended testing a non-card payment method, such as SEPA.
we may actually want to save the short statement descriptor in the account instead
I've tried that approach and it didn't play well with the regular and suffix statement descriptors, not sure why. I've tried to follow Stripe docs and sent different combinations as well as setting all of them together and the payment intent always ended up having the full statement descriptor set in it.
There was a problem hiding this comment.
@cesarcosta99 I did some testing and was able to set the short statement descriptor (aka statement_descriptor_prefix) in the Stripe account level and create a PaymentIntent using that.
- Changed the server to also accept a "statement_descriptor_suffix" as an argument here and here.
- Manually updated my account using
curlto have a card statement descriptor prefix:
curl https://api.stripe.com/v1/accounts/{WCPay_Dev_CONNECTED_ACCOUNT_ID} \
-u "{WCPay_Dev_SECRET_KEY}:" \ # Remember the ":" at the end of the secret key
-d "settings[card_payments][statement_descriptor_prefix]"="WOO.COM"- Add the
statement_descriptor_suffixto the WCPay client intentions request to something like "ID: 1234". (Like what you did with$request->set_statement_descriptor().
The only issue I encountered was that I needed to put a Latin character on the statement descriptor suffix, otherwise Stripe would throw an error:
invalid_request_error - statement_descriptor_suffix
The statement descriptor must contain at least one Latin character.
|
Just commenting to say I've got back to this and it's in progress. |
cesarcosta99
force-pushed
the
update/send-statement-descriptors-on-pi
branch
from
9d75d4b
to
462ef88
Compare
|
Size Change: +2.56 kB (0%) Total Size: 1.26 MB
ℹ️ View Unchanged
|
cesarcosta99
force-pushed
the
update/send-statement-descriptors-on-pi
branch
from
3b00e98
to
0168ec5
Compare
|
Rebased @ricardo, I'm requesting another review from you since you've got context from previous reviews. Please consider my last comment as well as my last replies from previous reviews. You might want to go through the testing instructions from #3625 too, since those changes got introduced in this PR. |
|
@cesarcosta99 I didn't fully review the code yet, but considering it's possible to set the statement descriptor prefix at the account level, I guess that would be the best way to do it, given the full statement descriptor being already stored at the account level. I think this would also simplify the implementation a little bit. We wouldn't need to store the statement descriptor prefix in the plugin settings nor handle the logic on whether to use the full statement or the short statement. We'd pass a Of course, this would require a server change as well, where we enable saving the
I personally don't see this as a problem, since the statement descriptor prefix can be seen from the WCPay dashboard and account cache, but others might disagree. Maybe we should ask the architects team on their thoughts about saving it under the plugin settings, or the Stripe account.
Regarding this error. It might be worth asking Stripe about what's the "ideal" suffix there, if not an order ID, and how to prefix the order ID with a Latin character so that this requirement can be fulfilled. |
|
Not sure if the input width used to be smaller at the time this PR was created, but maybe we should double-check this with design now, in order to avoid regression. Also checking if the labels are accurate now.
|
cesarcosta99
added
pr: in progress
status: on hold
|
Thanks for reviewing, @ricardo. I'm putting the PR on hold since I can't prioritize these changes at the moment. I'll address your questions and concerns once I get back to this PR. |
|
Closing this pull request due to changes in Fractal priorities. More details in the original issue. |
cesarcosta99
removed
pr: in progress
status: on hold
Fixed #4329
Changes proposed in this Pull Request
The usage of the statement descriptor in transactions is being addressed in this PR. This is the final part of the #3625's work.
For some portion of the transactions, the regular statement descriptor is being added to the payment intents already. Here we're making sure the descriptor is passed to the payment intent and the shortened descriptor is being included to take place on card payments, if enabled.
It also includes a small fix for the Blocks checkout when using UPE, where the
wcpay_selected_upe_payment_typewas not being sent.Testing instructions
Test the descriptors set are sent to the payment intent.
There's a vast amount of combinations for this test, please try to test as much as you can:
Add customer order number to the bank statementenabled and disabled.Also consider the following:
Full bank statementandShortened customer bank statementare provided, the payment intent should either contain no statement descriptor or have theFull bank statementdescriptor set.Full bank statementis provided andAdd customer order number to the bank statementis disabled, the payment intent should contain theFull bank statement's value in the statement descriptor regardless the payment method chosen.Add customer order number to the bank statementis enabled andShortened customer bank statementis provided, the payment intent should contain theShortened customer bank statement's value concatenated with the order number in the statement descriptor.The test:
Full bank statementandShortened customer bank statement.readme.txtandchangelog.txt(or does not apply)Post merge