Update promo in MPE after bank flow #4370

tillh-stripe · 2024-12-18T17:50:36Z

Summary

This pull request adds code to refresh the MPE’s promo information after the bank auth flow completes. If no incentive is available, the promo badge in the carousel and bank form should be updated accordingly.

Motivation

CONSUMERBANK-571

Testing

Changelog

github-actions · 2024-12-18T18:06:30Z

🚨 New dead code detected in this PR:

StripeAttestBackend.swift:24 warning: Parameter 'appId' is unused
StripeAttestBackend.swift:24 warning: Parameter 'deviceId' is unused
StripeAttestBackend.swift:24 warning: Parameter 'keyId' is unused
StripeAttestBackend.swift:20 warning: Parameter 'attestation' is unused
StripeAttestBackend.swift:14 warning: Property 'apiClient' is assigned, but never used
SavedPaymentMethodRowButton.swift:9 warning: Imported module 'StripeCore' 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.

...Sheet/Source/PaymentSheet/Vertical Main Screen/VerticalPaymentMethodListViewController.swift

mats-stripe · 2025-01-14T14:04:57Z

...StripePaymentSheet/Source/PaymentSheet/USBankAccount/InstantDebitsPaymentMethodElement.swift

@@ -164,6 +169,11 @@ final class InstantDebitsPaymentMethodElement: ContainerElement {

        return nameValid && emailValid && phoneValid && addressValid
    }
+
+    var displayableIncentive: PaymentMethodIncentive? {
+        let canShowIncentive = linkedBank?.incentiveEligible ?? true


Any reason we default to true here? I'd expect the opposite

If we don’t have a linked bank yet (and therefore no info if the session is eligible), we assume that the link_consumer_incentive as coming from the backend is valid. Otherwise, we would never show the incentive before the user completes the flow 🙈

...StripePaymentSheet/Source/PaymentSheet/USBankAccount/InstantDebitsPaymentMethodElement.swift

...Sheet/Source/PaymentSheet/Vertical Main Screen/VerticalPaymentMethodListViewController.swift

github-actions · 2025-01-24T20:25:12Z

⚠️ Public API changes detected:

StripePaymentSheet

- case integrationError(nonPIIDebugDescription: Swift.String)

If you are adding a new public API consider the following:

Do these APIs need to be public or can they be protected with @_spi(STP)?
If these APIs need to be public, assess whether they require an API review.

If you are modifying or removing a public API:

Does this require a breaking version change?
Do these changes require API review?

If you confirm these APIs need to be added/updated and have undergone necessary review, add the label modifies public API to this PR to acknowledge and bypass this check.

ℹ️ If this comment appears to be left in error, make sure your branch is up-to-date with master.

- Rename `updateLinkedBank` to `renderLinkedBank` for greater clarity - Add explanatory comment to `canShowIncentive` - Make `refreshContent` a method instead of a property

tillh-stripe · 2025-01-24T20:57:04Z

StripePaymentSheet/StripePaymentSheet/Source/PaymentSheet/USBankAccount/IncentiveOwner.swift

+
+import Foundation
+
+protocol IncentiveOwner {


Ideas for better names are welcome!

Maybe IncentiveDisplayable or IncentiveBadgeDisplayer - none are really great though 🤷

I'm also curious your thoughts on making a protocol for this. Seems to only be one class conforming to it at the moment. I guess I'm wondering if showIncentiveInHeader could just be a concrete property on InstantDebitsPaymentMethodElement?

mats-stripe

LGTM - some minor comments / questions

mats-stripe · 2025-01-24T22:55:43Z

StripePaymentSheet/StripePaymentSheet/Source/PaymentSheet/USBankAccount/IncentiveOwner.swift

+
+import Foundation
+
+protocol IncentiveOwner {


Maybe IncentiveDisplayable or IncentiveBadgeDisplayer - none are really great though 🤷

mats-stripe · 2025-01-24T22:56:21Z

StripePaymentSheet/StripePaymentSheet/Source/PaymentSheet/Vertical Main Screen/RowButton.swift

@@ -186,6 +186,8 @@ class RowButton: UIView {
                }
            }()
            promoBadge.translatesAutoresizingMaskIntoConstraints = false
+            promoBadge.isUserInteractionEnabled = false
+            promoBadge.isAccessibilityElement = false


What does this mean? We don't want it to be picked up by screen readers?

mats-stripe · 2025-01-24T23:00:15Z

...Sheet/Source/PaymentSheet/Vertical Main Screen/VerticalPaymentMethodListViewController.swift

@@ -119,7 +148,7 @@ class VerticalPaymentMethodListViewController: UIViewController {
                promoText: incentive?.takeIfAppliesTo(paymentMethodType)?.displayText,
                appearance: appearance,
                // Enable press animation if tapping this transitions the screen to a form instead of becoming selected
-                shouldAnimateOnPress: !delegate.shouldSelectPaymentMethod(selection)
+                shouldAnimateOnPress: delegate?.shouldSelectPaymentMethod(selection) == true


I'm a bit confused about this change - it now does the inverse? Is the comment above still accurate?

mats-stripe · 2025-01-24T23:02:51Z

StripePaymentSheet/StripePaymentSheet/Source/PaymentSheet/USBankAccount/IncentiveOwner.swift

+
+import Foundation
+
+protocol IncentiveOwner {


I'm also curious your thoughts on making a protocol for this. Seems to only be one class conforming to it at the moment. I guess I'm wondering if showIncentiveInHeader could just be a concrete property on InstantDebitsPaymentMethodElement?

tillh-stripe force-pushed the tillh/ibp-incentives-fetch-available-incentives branch 2 times, most recently from 5ad3d19 to b83e20a Compare December 20, 2024 14:31

tillh-stripe force-pushed the tillh/ibp-incentives-update-mpe-after-flow branch 3 times, most recently from 2ed15ab to 242910b Compare December 20, 2024 19:43

tillh-stripe force-pushed the tillh/ibp-incentives-fetch-available-incentives branch from b83e20a to e564e9e Compare December 20, 2024 19:44

tillh-stripe force-pushed the tillh/ibp-incentives-fetch-available-incentives branch 5 times, most recently from cabfd94 to 98c8d0d Compare January 13, 2025 14:52

Base automatically changed from tillh/ibp-incentives-fetch-available-incentives to master January 13, 2025 16:18

tillh-stripe force-pushed the tillh/ibp-incentives-update-mpe-after-flow branch 2 times, most recently from 1e979a0 to d9df82d Compare January 13, 2025 21:25

tillh-stripe commented Jan 13, 2025

View reviewed changes

...Sheet/Source/PaymentSheet/Vertical Main Screen/VerticalPaymentMethodListViewController.swift Outdated Show resolved Hide resolved

tillh-stripe force-pushed the tillh/ibp-incentives-update-mpe-after-flow branch 3 times, most recently from 2bba9f1 to 8d2951b Compare January 14, 2025 16:40

mats-stripe previously approved these changes Jan 14, 2025

View reviewed changes

tillh-stripe dismissed mats-stripe’s stale review via 9bd2ef9 January 14, 2025 19:40

tillh-stripe force-pushed the tillh/ibp-incentives-update-mpe-after-flow branch 2 times, most recently from 19ff93c to 8ae81af Compare January 16, 2025 20:15

tillh-stripe changed the base branch from master to tillh/ibp-incentives-header-badge-again January 16, 2025 20:15

tillh-stripe force-pushed the tillh/ibp-incentives-header-badge-again branch from 7700e8a to 4314eaf Compare January 16, 2025 21:12

tillh-stripe force-pushed the tillh/ibp-incentives-update-mpe-after-flow branch from 8ae81af to 10b949a Compare January 16, 2025 21:13

Base automatically changed from tillh/ibp-incentives-header-badge-again to master January 17, 2025 20:08

tillh-stripe force-pushed the tillh/ibp-incentives-update-mpe-after-flow branch from 2076970 to d965bd7 Compare January 22, 2025 19:02

tillh-stripe changed the base branch from master to tillh/ibp-incentives-api-tweaks January 22, 2025 19:02

tillh-stripe force-pushed the tillh/ibp-incentives-api-tweaks branch from 07039d1 to a5db3ad Compare January 22, 2025 23:45

Base automatically changed from tillh/ibp-incentives-api-tweaks to master January 23, 2025 00:12

tillh-stripe force-pushed the tillh/ibp-incentives-update-mpe-after-flow branch 3 times, most recently from ca06961 to 96174cc Compare January 24, 2025 13:51

tillh-stripe added 7 commits January 24, 2025 15:34

Update promo in MPE after bank flow

976fefa

Address code review feedback

5e21510

- Rename `updateLinkedBank` to `renderLinkedBank` for greater clarity - Add explanatory comment to `canShowIncentive` - Make `refreshContent` a method instead of a property

Fix issue where header badge didn't update correctly

874db39

Fix issue where tapping the badge didn’t select the row

2f8f67a

Avoid unnecessary layout work

95de7a3

Fix rebase issue

246835c

Make minor code tweaks

3f88f40

tillh-stripe force-pushed the tillh/ibp-incentives-update-mpe-after-flow branch from 8a63335 to 3f88f40 Compare January 24, 2025 20:34

tillh-stripe marked this pull request as ready for review January 24, 2025 20:56

tillh-stripe requested review from a team as code owners January 24, 2025 20:56

tillh-stripe requested a review from mats-stripe January 24, 2025 20:56

tillh-stripe commented Jan 24, 2025

View reviewed changes

mats-stripe approved these changes Jan 24, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update promo in MPE after bank flow #4370

Update promo in MPE after bank flow #4370

tillh-stripe commented Dec 18, 2024 •

edited

Loading

github-actions bot commented Dec 18, 2024 •

edited

Loading

mats-stripe Jan 14, 2025

tillh-stripe Jan 14, 2025

github-actions bot commented Jan 24, 2025

tillh-stripe Jan 24, 2025

mats-stripe Jan 24, 2025

mats-stripe Jan 24, 2025

mats-stripe left a comment

mats-stripe Jan 24, 2025

mats-stripe Jan 24, 2025

mats-stripe Jan 24, 2025

mats-stripe Jan 24, 2025

Update promo in MPE after bank flow #4370

Are you sure you want to change the base?

Update promo in MPE after bank flow #4370

Conversation

tillh-stripe commented Dec 18, 2024 • edited Loading

Summary

Motivation

Testing

Changelog

github-actions bot commented Dec 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jan 24, 2025

StripePaymentSheet

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mats-stripe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tillh-stripe commented Dec 18, 2024 •

edited

Loading

github-actions bot commented Dec 18, 2024 •

edited

Loading