Skip to content
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

Update the payment intent when save is unselected. #4291

Merged
merged 55 commits into from
Oct 18, 2021

Conversation

michelleb-stripe
Copy link
Contributor

Summary

There are three states that a New PaymentSelection can be in: RequestSave, RequestNoSave, or NoRequest.

  • NoRequest indicates the save checkbox is not presented to the user. setup_future_usage will not be set on the PI when confirming
  • RequestNoSave indicates the save checkbox is presented to the user and it is not checked. setup_future_usage will be set "" on the PI when confirming
  • RequestSave indicates the save checkbox is presented to the user and it is check. setup_future_usage will be set "off_session" on the PI when confirming

Motivation

Reproduction steps:

  1. Enter card details requiring card authentication and have the save for future use checkbox selected
  2. Fail the authentication
  3. Uncheck the save for future use checkbox
  4. Complete authentication

Actual: Card is saved to the customer.
Expected: Card is not saved

Testing

  • Added tests
  • Modified tests
  • Manually verified

Base automatically changed from michelleb/oophs-take2 to master October 15, 2021 21:27
yuki-stripe
yuki-stripe previously approved these changes Oct 18, 2021
sealed class New : PaymentSelection() {
abstract val paymentMethodCreateParams: PaymentMethodCreateParams
abstract val shouldSavePaymentMethod: Boolean
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

iOS models this is as Boolean?, where null means .NoRequest

Comment on lines 20 to 23
enum class UserReuseRequest {
RequestReuse,
RequestNoReuse,
NoRequest
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"User" can refer to either the merchant's users (i.e. customer) or Stripe's users (i.e. merchants). This should be renamed to "Customer" to avoid confusion.

nit: Rename to CustomerRequestedSave? "Reuse" implies the customer is trying to reuse the payment method, but they're only saving it at this point for reuse. They may never actually reuse it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

got it

skyler-stripe
skyler-stripe previously approved these changes Oct 18, 2021
brnunes-stripe
brnunes-stripe previously approved these changes Oct 18, 2021
Comment on lines -138 to -142
is CharSequence -> {
if (value.isEmpty()) {
compactParams.remove(key)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I worry if this will break something

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did look at annotations and didn't see a particular reason why this is true. I am curious at @yuki-stripe If you have this similar functionality on iOS where you globally remove all spaces and null parameters from all requests?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't have the "if 'empty' parameter -> don't send" logic. Not sure about spaces.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is technically a breaking change but probably zero impact in practice. We can call it out in the CHANGELOG?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, added to the changelog already.

@michelleb-stripe michelleb-stripe merged commit 36555cc into master Oct 18, 2021
@michelleb-stripe michelleb-stripe deleted the michelleb/unselect-save branch October 18, 2021 18:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants