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

Set 'Change >' and sublabel to selected embedded form rows #4538

Merged
merged 10 commits into from
Feb 7, 2025

Conversation

yuki-stripe
Copy link
Collaborator

Summary

Adds logic to set and unset the "Change >" + sublabel.

Motivation

https://jira.corp.stripe.com/browse/MOBILESDK-3057

Testing

See new tests

Changelog

See changelog

Copy link

github-actions bot commented Feb 4, 2025

🚨 New dead code detected in this PR:

FormSpecProvider.swift:60 warning: Function 'load()' 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.

Comment on lines 379 to 380
let brand = STPCardValidator.brand(forNumber: cardNumber)
let brandString = STPCardBrandUtilities.stringFrom(brand)
Copy link
Collaborator

@porter-stripe porter-stripe Feb 4, 2025

Choose a reason for hiding this comment

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

This doesn't seem to work with co-branded cards, when I enter a co-branded (Visa/CB) card and choose CB, the copy says "Visa ...". we need to also check the preferred network (if it exists) on the card params. Can we add a test for this too?

params.paymentMethodParams.card?.networks?.preferred

There's also a weird edge case where you can fill out a form for a co-branded card and don't select a brand since it's optional, so the card brand gets decided on the backend. Not exactly sure what we show there.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for catching this! Fixed it up. Are there things we can do to make it harder to make this mistake?

Copy link
Collaborator

Choose a reason for hiding this comment

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

We did something similar to what you did with func brand(for card: STPPaymentMethodCardParams?) -> STPCardBrand on STPPaymentMethodCard. But this is the first instance AFAIK where we have needed to read it off the STPPaymentMethodCardParams. Maybe we "internally" deprecate STPCardBrandUtilities.stringFrom(brand) somehow, since the card number is not the single source of truth for the card brand for a number.

@porter-stripe
Copy link
Collaborator

You can probably ignore the dead code flag, it's flagging it b/c that function is only used in tests. But maybe that function should be in a test file?

porter-stripe
porter-stripe previously approved these changes Feb 7, 2025
porter-stripe
porter-stripe previously approved these changes Feb 7, 2025
porter-stripe
porter-stripe previously approved these changes Feb 7, 2025
@yuki-stripe
Copy link
Collaborator Author

I approved a bunch of snapshot tests that are not quite right; the labels can get squished a bit w/ the hidden "Change" button. Debugging & fixing this is too toilsome with the current RowButton architecture relative to the tiny UI problem here, so I'm going to wait to fix this until we refactor RowButton as a GA fast-follow (likely after we submit for Implementation Review & finish the next major version) @porter-stripe

@yuki-stripe yuki-stripe merged commit b7b11b7 into master Feb 7, 2025
6 checks passed
@yuki-stripe yuki-stripe deleted the yuki/embedded-update-form-pm-row-pt-3 branch February 7, 2025 23:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants