Add Unknown payment method type support in Link wallet - #13140
Conversation
| editCardDetailsInteractor = interactor.editCardDetailsInteractor, | ||
| ) | ||
| } | ||
| is LinkPaymentDetails.Unknown -> {} |
There was a problem hiding this comment.
Leaving empty for now to handle the Unknown case but will be adding UI here in a future PR.
|
Diffuse output: APKDEXARSC |
a567484 to
e2c6312
Compare
|
|
||
| @RestrictTo(RestrictTo.Scope.LIBRARY_GROUP) | ||
| @Parcelize | ||
| data class Unknown( |
There was a problem hiding this comment.
Would it be a huge lift to find all and replace this to Generic instead of Unknown? I'm going to change iOS to Generic and it'd be nice to get that change in ahead of merging this one.
There was a problem hiding this comment.
Do we want to update just LinkPaymentDetails.Unknown or ConsumerPaymentDetails.Unknown as well?
| get() = when (this) { | ||
| is Card -> makeCardDisplayName(nickname, funding, brand) | ||
| is BankAccount -> bankName?.resolvableString ?: "••••$last4".resolvableString | ||
| is LinkPaymentDetails.Unknown -> label?.resolvableString ?: sublabel?.resolvableString ?: "".resolvableString |
There was a problem hiding this comment.
[nit] do we need the LinkPaymentDetails prefix here? Same with line 31
| sublabel = "0x••••22Dd", | ||
| icon = ConsentUi.Icon(default = "https://example.com/crypto-icon.png") | ||
| ), | ||
| nextActionTypes = listOf("STRIPE_REDIRECT") |
There was a problem hiding this comment.
[nit] can you update both usages of STRIPE_REDIRECT -> redirect_to_url? Just to make it closer to what is sent by the server.
| val nickname: String?, | ||
| val label: String?, | ||
| val sublabel: String?, | ||
| val icon: ConsentUi.Icon?, |
There was a problem hiding this comment.
[nit] Should this be a ConsentUi icon? I'm not sure about the context of ConsentUi so maybe it's used in most places we parse URLs -> Images in stripe-android, but it seems out of place
There was a problem hiding this comment.
As suggested by Till, added a new Icon data class instead of reusing ContentUi.Icon
| private const val FIELD_LABEL = "label" | ||
| private const val FIELD_SUBLABEL = "sublabel" | ||
| private const val FIELD_ICON = "icon" | ||
| private const val FIELD_DEFAULT = "default" |
There was a problem hiding this comment.
[nit] A rename may be helpful here, since a reader could confuse this with FIELD_IS_DEFAULT. Maybe FIELD_DEFAULT_ICON?
| open val nickname: String?, | ||
| open val billingAddress: BillingAddress?, | ||
| open val billingEmailAddress: String?, | ||
| open val display: DisplayMetadata? = null |
There was a problem hiding this comment.
I wonder if we can make this non-null? Since we're filtering out payment method types that don't have a display, we shouldn't be setting this field to null at any point, right?
There was a problem hiding this comment.
It has to be nullable in PaymentDetails since we eventually want to add display to Card, Bank, and Passthrough (right? or am I misremembering this?) but aren't parsing display for those types right now. I'll update Unknown/Generic to non-null display to reduce null checks.
| } ?: false | ||
| is ConsumerPaymentDetails.BankAccount -> allowedCountries.contains(CountryCode.US.value) | ||
| is ConsumerPaymentDetails.Unknown -> billingAddress?.countryCode?.let { | ||
| allowedCountries.contains(it.value.uppercase()) && display != null && !display?.label.isNullOrBlank() |
There was a problem hiding this comment.
Why include the display conditions here?
| is ConsumerPaymentDetails.Passthrough -> ConsumerPaymentDetails.Card.TYPE | ||
| is ConsumerPaymentDetails.Card, | ||
| is ConsumerPaymentDetails.Passthrough, | ||
| is ConsumerPaymentDetails.Unknown -> ConsumerPaymentDetails.Card.TYPE |
There was a problem hiding this comment.
This is probably correct, but @tillh-stripe does this seem ok?
There was a problem hiding this comment.
Updated expectedPaymentMethodType to be nullable which matches how iOS handles generics: Made expectedPaymentMethodType nullable to match iOS
| LinkController.PaymentMethodType.BankAccount | ||
| } | ||
| is ConsumerPaymentDetails.Unknown -> { | ||
| LinkController.PaymentMethodType.Card |
There was a problem hiding this comment.
This feels off, but we may not have another option? How does this LinkController.PaymentMethodPreview get used?
There was a problem hiding this comment.
PaymentMethodPreview.Type is only being used for settlement speed related logic in the OnrampViewModel. If we add a Generic type to LinkController.PaymentMethodType then the user will be able to filter using it if LinkController is made public #13145 so I'm not too sure how else to handle this case. Card and GooglePay have instant settlement speed in the OnrampViewModel and I assumed Generics were going to be instant as well.
There was a problem hiding this comment.
Returning a type of Card for non-card payment methods doesn't seem right. It feels to me like we need to introduce a separate type if we want to:
a) return a realistic type as part of PaymentMethodPreview, and
b) allow filtering for cards and bank accounts in the upcoming Presenter.present method in #13145.
@jeans-stripe Can you incorporate this idea in the API review?
There was a problem hiding this comment.
Done! Added a Generic type to LinkController.PaymentMethodType. I've updated present() to accept a nullable list of PaymentMethodType in #13145 so I'll need to rebase this PR once that is merged fix merge conflicts and enable filtering for Generic
| shouldShowDefaultBadge = true, | ||
| ), | ||
| ), | ||
| PaymentOptionsItem.SavedPaymentMethod( |
There was a problem hiding this comment.
Do we need to another preview payment option here?
There was a problem hiding this comment.
No, sorry this was left in from some testing. Removed!
tillh-stripe
left a comment
tillh-stripe
left a comment
There was a problem hiding this comment.
Added a few comments about the code.
The main thing we still need to do is update SupportedPaymentMethodTypes.kt so that we no longer send "types": ["card", "bank_account"], which effectively excludes all unknown payment method types from being included in the response.
| val nickname: String?, | ||
| val label: String?, | ||
| val sublabel: String?, | ||
| val icon: ConsentUi.Icon?, |
There was a problem hiding this comment.
Let's introduce an Icon class inside DisplayMetadata (which we should just rename to Display or DisplayInfo) instead of something from the OAuth flow.
| val rawType: String, | ||
| override val display: DisplayMetadata?, | ||
| val nextActionTypes: List<String>, | ||
| ) : PaymentDetails( |
There was a problem hiding this comment.
| ) : PaymentDetails( | |
| ) : PaymentDetails( |
| modifier = modifier, | ||
| title = paymentDetails.displayName.resolve(), | ||
| subtitle = paymentDetails.display?.sublabel, | ||
| icon = { UnknownIcon(iconUrl = paymentDetails.display?.icon?.default?.takeIf { it.isNotBlank() }) } |
There was a problem hiding this comment.
Nit: If we're already checking for a blank URL during parsing, we don't need to do it at the call-sites.
| get() = when (this) { | ||
| is Card -> makeCardDisplayName(nickname, funding, brand) | ||
| is BankAccount -> bankName?.resolvableString ?: "••••$last4".resolvableString | ||
| is LinkPaymentDetails.Unknown -> label?.resolvableString ?: sublabel?.resolvableString ?: "".resolvableString |
There was a problem hiding this comment.
label being nullable makes this tricky. Inside PaymentMethodWithLinkDetailsJsonParser, can we just reject unknown payment method types that don't have a display hash?
| is ConsumerPaymentDetails.Passthrough -> { | ||
| "•••• $last4".resolvableString | ||
| } | ||
| is ConsumerPaymentDetails.Unknown -> display?.label?.resolvableString ?: "".resolvableString |
There was a problem hiding this comment.
This would also be solved that way.
| import com.stripe.android.uicore.image.StripeImage | ||
|
|
||
| @Composable | ||
| fun UnknownIcon( |
There was a problem hiding this comment.
Nit: We can remove the Unknown prefix here.
There was a problem hiding this comment.
Should we call it LoadableIcon or StripeIcon to make it distinct from material Icon?
| val items = remember(paymentDetails) { | ||
| buildList { | ||
| if (!paymentDetails.isDefault) { | ||
| if (!paymentDetails.isDefault && paymentDetails !is ConsumerPaymentDetails.Unknown) { |
There was a problem hiding this comment.
Let's make this a canBeSetAsDefault on ConsumerPaymentDetails with exhaustive matching.
Yes, the filtering is removed in #13141. The link sheet closing flow isn't properly animated yet and I also update that to make the full flow functional. |
Support displaying and interacting with payment methods the SDK doesn't explicitly recognize by introducing ConsumerPaymentDetails.Unknown and LinkPaymentDetails.Unknown. Previously, unrecognized types were silently dropped — now they render using server-provided display metadata (label, sublabel, icon URL). Key changes: - Add Unknown data class with DisplayMetadata for server-driven rendering - Parse unrecognized payment_details types as Unknown instead of null - Thread Unknown through wallet UI, menus, confirmation, and billing utils - Add UnknownIcon composable for loading remote icon URLs - Make listPaymentDetails types filter nullable to retrieve all types - Add screenshot and unit tests for Unknown in wallet flows
- Renaming Unknown to Generic - Renaming DisplayMetadata to Display - Removing unused code
- Generic payment methods now map to null expectedPaymentMethodType which is passed to backend which already accepts null. This matches the iOS logic for unknown/generic payment methods.
- Added `Generic` LinkPaymentMethodFilter - `PaymentMethodPreview` now returns `Generic` for `Generic` types
2fc7505 to
33ede93
Compare
| PaymentMethodDisplayData.Type.BankAccount | ||
|
|
||
| LinkController.PaymentMethodType.Generic -> | ||
| PaymentMethodDisplayData.Type.Card |
There was a problem hiding this comment.
Added to this ticket to update when settlement times are server-driven
| import com.stripe.android.paymentsheet.DisplayableSavedPaymentMethod | ||
| import com.stripe.android.paymentsheet.PaymentOptionsItem | ||
| import com.stripe.android.paymentsheet.R | ||
| import com.stripe.android.paymentsheet.SavedPaymentMethod |
There was a problem hiding this comment.
Do we need this import?
3 participants
Summary
Support displaying and interacting with payment methods the SDK doesn't explicitly recognize by introducing
ConsumerPaymentDetails.UnknownandLinkPaymentDetails.Unknown. Previously, unrecognized types were silently dropped, now they can be rendered using server-provided display metadata (label, sublabel, icon URL). The unknown types are still being filtered out bySupportedPaymentTypesin this PR but that will be updated in the next PR.Key changes:
Motivation
We'd like forward compatibility for Link payment methods. When the backend adds PMs, they should be able to be rendered and used for payment in prior versions of the SDK. This PR is a first step towards this goal. This proposal provides more context.
Testing
Screenshots
New screenshots have been captured