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

Async init for embedded and initial selection #4068

Merged
merged 15 commits into from
Sep 30, 2024

Conversation

porter-stripe
Copy link
Collaborator

@porter-stripe porter-stripe commented Sep 27, 2024

Summary

  • Updates the embedded playground to take in the configuration from the PlaygroundController, and use the actual embedded APIs.
  • Implements the create for the EmbeddedPaymentElement. This now uses the PaymentSheetLoader to load the intent and elements/session. This function also determines with the load result if we should show Link or Apple Pay, and the first selection in the embedded view.
  • Updates EmbeddedPaymentMethodsView to take in an initial selection and select it if present
  • Adds an IntegrationShape to the PaymentSheetLoader to determine if we can default to Apple Pay/Link and when to start checkout timer
  • Some updated snapshot tests

Motivation

Embedded

Testing

  • Manual

Changelog

N/A

@porter-stripe porter-stripe changed the title [DRAFT] Start on embedded init/create Async init for embedded and initial selection Sep 30, 2024
@porter-stripe porter-stripe marked this pull request as ready for review September 30, 2024 17:11
@porter-stripe porter-stripe requested review from a team as code owners September 30, 2024 17:11
/// - paymentSheetConfig: The PaymentSheet.Configuration instance to convert from.
/// - formSheetAction: The FormSheetAction specific to EmbeddedPaymentElement.Configuration.
/// - hidesMandateText: Determines whether to hide mandate text. Defaults to `false`.
public init(
Copy link
Collaborator

Choose a reason for hiding this comment

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

How will we add Embedded-specific stuff like hidesMandateText or formSheetAction if we go from playground settings -> PaymentSheet.Configuration -> EmbeddedConfiguration?

Maybe we should make the "playground settings -> EmbeddedConfiguration" logic happen in the PlaygroundController or something?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah I think creating the EmbeddedConfiguration in the PlaygroundController sounds good, I was thinking we could just inject those two properties into this view controller but that doesn't scale well if we add more. Will update this in this PR.

appearance: .default,
shouldShowApplePay: true,
shouldShowLink: true
let paymentSheetConfiguration = configuration.makePaymentSheetConfiguration()
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 a temporary hack right? If so, let's document it

appearance: rowButtonAppearance,
rightAccessoryView: accessoryButton,
isEmbedded: true,
didTap: handleRowSelection(selectedRowButton:)))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah this makes me wonder, do the other RowButtons have a retain cycle if they pass a selector directly to didTap? Like on line 44

@@ -180,3 +220,41 @@ extension EmbeddedPaymentElement {
public typealias BillingDetailsCollectionConfiguration = PaymentSheet.BillingDetailsCollectionConfiguration
public typealias ExternalPaymentMethodConfiguration = PaymentSheet.ExternalPaymentMethodConfiguration
}

// TODO(porter) Create a protocol for the commonalities between PaymentSheet.Configuration <> EmbeddedPaymentElement.Configuration
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are you gonna follow up on this immediately or should we make a ticket

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah will do later, just created: https://jira.corp.stripe.com/browse/MOBILESDK-2533

Copy link

github-actions bot commented Sep 30, 2024

🚨 New dead code detected in this PR:

EmbeddedPaymentMethodsView.swift: warning: Property 'selection' is assigned, but never used

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.

@@ -472,6 +501,44 @@ class EmbeddedPaymentMethodsViewSnapshotTests: STPSnapshotTestCase {
verify(embeddedView)
}

// MARK: Initial selection tests
Copy link
Collaborator

Choose a reason for hiding this comment

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

Might as well add an assert on selection, too.

EmbeddedPaymentMethodsView.swift: warning: Property 'selection' is assigned, but never used

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good idea!

@@ -177,6 +177,75 @@ class PlaygroundController: ObservableObject {
return configuration
}

var embeddedConfiguration: EmbeddedPaymentElement.Configuration {
var configuration = EmbeddedPaymentElement.Configuration(formSheetAction: .confirm(completion: { [weak self] result in
// TODO(porter) Handle two step confirm
Copy link
Collaborator

Choose a reason for hiding this comment

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

ticket or immediate followup? We should probably make an embedded-only formSheetAction toggle in the playground

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can make a ticket, in my head I was thinking the week of Oct. 7 it would be added but I can add sooner

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll make a follow up right now and add the hide mandate API too.

@porter-stripe porter-stripe merged commit c147b50 into master Sep 30, 2024
6 checks passed
@porter-stripe porter-stripe deleted the porter/MOBILESDK-2525 branch September 30, 2024 21:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants