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

remove alternateUpdatePaymentMethodNavigation flag and updated tests #4337

Conversation

joyceqin-stripe
Copy link
Collaborator

@joyceqin-stripe joyceqin-stripe commented Dec 6, 2024

Summary

Removed the alternateUpdatePaymentMethodNavigation flag that gated the new update payment method screen. Now, by default, any payment method that can be removed or edited (card, US bank account, SEPA debit) will display an edit icon or chevron (horizontal vs vertical) that will lead to the new UpdatePaymentMethod screen.

Motivation

Testing

Changelog

[Changed] Changed the edit and remove saved payment method flow so that tapping 'Edit' displays an icon that leads to a new update payment method screen

Copy link

github-actions bot commented Dec 6, 2024

🚨 New dead code detected in this PR:

String+Localized.swift:352 warning: Property 'remove_payment_method' is unused
CustomerSavedPaymentMethodsCollectionViewController.swift:43 warning: Enum case 'didSelectRemoveOnInvalidItem' is unused

Please remove the dead code before merging.

If this is intentional, you can bypass this check by adding the label skip dead code check to this PR.

ℹ️ If this comment appears to be left in error, double check that the flagged code is actually used and/or make sure your branch is up-to-date with master.

@joyceqin-stripe joyceqin-stripe marked this pull request as ready for review December 6, 2024 16:32
@joyceqin-stripe joyceqin-stripe requested review from a team as code owners December 6, 2024 16:32
@porter-stripe
Copy link
Collaborator

Have we fixed the bug called out here? https://stripe.slack.com/archives/C07RF1UM08M/p1733430979980769

@joyceqin-stripe
Copy link
Collaborator Author

Have we fixed the bug called out here? https://stripe.slack.com/archives/C07RF1UM08M/p1733430979980769

I believe so, yes

Copy link
Collaborator

@porter-stripe porter-stripe left a comment

Choose a reason for hiding this comment

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

We should add a CHANGELOG entry for this, cc @amk-stripe

}
return allowsPaymentMethodRemoval || (viewModel?.savedPaymentMethod?.isCoBrandedCard ?? false && cbcEligible)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since all cards are now editable, do we need " (viewModel?.savedPaymentMethod?.isCoBrandedCard ?? false && cbcEligible)"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If payment method removal is not allowed, then you can only edit your payment methods if it's a cbc card (and in the future, if it allows set as default), right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yep that sounds right.

porter-stripe
porter-stripe previously approved these changes Dec 6, 2024
CHANGELOG.md Outdated
@@ -1,3 +1,5 @@
### PaymentSheet, CustomerSheet
* [Changed] Changed the edit and remove saved payment method flow so that tapping 'Edit' displays an icon that leads to a new update payment method screen
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit:

  • We're missing a new line under this
  • Need a period to end the sentence (see other entries)
  • We should explain what this new payment method screen does e.g. displays card cvc, expiry, bank acct details, sepa, etc.

…flag' of github.com:stripe/stripe-ios into joyceqin-remove-alternateupdatepaymentmethodnavigation-flag
porter-stripe
porter-stripe previously approved these changes Dec 6, 2024
@joyceqin-stripe joyceqin-stripe enabled auto-merge (squash) December 6, 2024 22:26
@joyceqin-stripe joyceqin-stripe merged commit 6a12711 into master Dec 6, 2024
7 checks passed
@joyceqin-stripe joyceqin-stripe deleted the joyceqin-remove-alternateupdatepaymentmethodnavigation-flag branch December 6, 2024 23:09
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.

2 participants