Introduce IntegrationConfigurable #4081

porter-stripe · 2024-10-01T16:46:04Z

Summary

Adds IntegrationConfigurable which is a protocol that defines similarities between PaymentSheet.Configuration and EmbeddedPaymentElement.Configuration.
Injects a IntegrationConfigurable where currently needed
Moves some extensions on PaymentSheet.Configuration to IntegrationConfigurable
None of these changes broke CustomerSheet, so I did not conform CustomerSheet.Configuration to IntegrationConfigurable as it's not currently needed. It might be in the future however.

Motivation

Cleaner code
https://jira.corp.stripe.com/browse/MOBILESDK-2533

Testing

Compiler
CI

Changelog

N/A

porter-stripe · 2024-10-01T16:48:38Z

StripePaymentSheet/StripePaymentSheet/Source/Analytics/PaymentSheetAnalyticsHelper.swift

+        payload["form_sheet_action"] = formSheetAction.analyticValue
+        payload["hide_mandate_text"] = hidesMandateText


Note these new embedded only params.

porter-stripe · 2024-10-01T16:49:17Z

StripePaymentSheet/StripePaymentSheet/Source/PaymentSheet/IntegrationConfigurable.swift

+/// Represents shared configuration properties shared between integration surfaces in mobile payment element.
+/// - Note: See the concrete implementations of `IntegrationConfigurable` for detailed doc comments.
+/// - Note: Not currently used by CustomerSheet.
+protocol IntegrationConfigurable: PaymentMethodRequirementProvider {


Open to naming suggestions

Let's call it PaymentElementConfiguration? I think that's the most accurate name, since this is Configuration for all Mobile Payment Element integration variants. We just need to start thinking of PaymentSheet, FlowController, EmbeddedPaymentElement as Payment Element variants.

This means we should probably rename things like PaymentSheetLoader to be PaymentElementLoader.

Agree on renaming other PaymentSheet prefixed classes, maybe something we should tackle when we rename the module.

github-actions · 2024-10-01T17:02:51Z

🚨 New dead code detected in this PR:

PaymentElementConfiguration.swift: warning: Property 'primaryButtonColor' is unused
PaymentElementConfiguration.swift: warning: Property 'primaryButtonLabel' is unused
PaymentElementConfiguration.swift: warning: Property 'style' is unused
PaymentElementConfiguration.swift: warning: Property 'merchantDisplayName' is unused
PaymentElementConfiguration.swift: warning: Property 'savePaymentMethodOptInBehavior' is unused
PaymentElementConfiguration.swift: warning: Property 'appearance' is unused
PaymentElementConfiguration.swift: warning: Property 'shippingDetails' is unused
PaymentElementConfiguration.swift: warning: Property 'preferredNetworks' is unused
PaymentElementConfiguration.swift: warning: Property 'userOverrideCountry' is unused
PaymentElementConfiguration.swift: warning: Property 'removeSavedPaymentMethodMessage' is unused
PaymentElementConfiguration.swift: warning: Property 'allowsRemovalOfLastSavedPaymentMethod' 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.

porter-stripe · 2024-10-01T18:21:51Z

I initially thought the dead code check was confused, but it's not, it's correct! I can comment out all those properties on IntegrationConfigurable and the project still compiles. This is b/c no where in the project where we inject a IntegrationConfigurable do we use any of those properties (yet).

I added the skip dead code check b/c I feel the spirit of the protocol is to define the commonalities between config objects and even though those properties are not used yet they probably will be as we build out embedded.

Options:

Skip dead code check, add all commonalities to IntegrationConfigurable as we have in the PR
Remove the flagged code, add properties to IntegrationConfigurable as needed in future PRs.

yuki-stripe · 2024-10-02T20:19:27Z

...ePaymentSheet/Source/PaymentSheet/CustomerSheet/CustomerAddPaymentMethodViewController.swift

@@ -244,7 +244,7 @@ class CustomerAddPaymentMethodViewController: UIViewController {
            isSettingUp: true,
            countryCode: nil,
            savePaymentMethodConsentBehavior: savePaymentMethodConsentBehavior,
-            analyticsHelper: .init(isCustom: false, configuration: .init()) // Just use a dummy analytics helper; we don't look at these analytics.
+            analyticsHelper: .init(isCustom: false, configuration: PaymentSheet.Configuration.init()) // Just use a dummy analytics helper; we don't look at these analytics.


Can you add https://jira.corp.stripe.com/browse/MOBILESDK-2548 to this comment

yuki-stripe · 2024-10-02T20:22:14Z

StripePaymentSheet/StripePaymentSheet/Source/PaymentSheet/IntegrationConfigurable.swift

+/// Represents shared configuration properties shared between integration surfaces in mobile payment element.
+/// - Note: See the concrete implementations of `IntegrationConfigurable` for detailed doc comments.
+/// - Note: Not currently used by CustomerSheet.
+protocol IntegrationConfigurable: PaymentMethodRequirementProvider {


Let's call it PaymentElementConfiguration? I think that's the most accurate name, since this is Configuration for all Mobile Payment Element integration variants. We just need to start thinking of PaymentSheet, FlowController, EmbeddedPaymentElement as Payment Element variants.

yuki-stripe · 2024-10-02T20:24:05Z

StripePaymentSheet/StripePaymentSheet/Source/Analytics/PaymentSheetAnalyticsHelper.swift

-    let configuration: PaymentSheet.Configuration
+    let configuration: IntegrationConfigurable


I wonder if we should keep this PaymentSheet.Configuration? I'm not sure this class can reasonably also handle analytics for EMPE

Yeah looking at the class more, it'll be pretty hard. We'll want some embedded specific one.

yuki-stripe · 2024-10-02T20:25:26Z

StripePaymentSheet/StripePaymentSheet/Source/PaymentSheet/IntegrationConfigurable.swift

+/// Represents shared configuration properties shared between integration surfaces in mobile payment element.
+/// - Note: See the concrete implementations of `IntegrationConfigurable` for detailed doc comments.
+/// - Note: Not currently used by CustomerSheet.
+protocol IntegrationConfigurable: PaymentMethodRequirementProvider {


This means we should probably rename things like PaymentSheetLoader to be PaymentElementLoader.

Introduce IntegrationConfigurable

f12ff59

porter-stripe commented Oct 1, 2024

View reviewed changes

update doc comment

8d7f39a

porter-stripe added skip dead code check and removed skip dead code check labels Oct 1, 2024

porter-stripe added 3 commits October 1, 2024 11:21

Fix tests

a4ba6b6

Don't touch StripeConnect

d119ee2

One more StripeConnect failure

bae9e15

porter-stripe added the skip dead code check label Oct 1, 2024

porter-stripe requested a review from yuki-stripe October 1, 2024 18:39

porter-stripe marked this pull request as ready for review October 1, 2024 18:41

porter-stripe requested review from a team as code owners October 1, 2024 18:41

porter-stripe added the embedded label Oct 1, 2024

yuki-stripe reviewed Oct 2, 2024

View reviewed changes

porter-stripe added 2 commits October 2, 2024 15:05

PR feedback

d2e1d58

lint

d86a5e1

porter-stripe requested a review from yuki-stripe October 3, 2024 14:34

yuki-stripe approved these changes Oct 3, 2024

View reviewed changes

porter-stripe merged commit 10bf64a into master Oct 3, 2024
6 checks passed

porter-stripe deleted the porter/MOBILESDK-2533 branch October 3, 2024 15:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce IntegrationConfigurable #4081

Introduce IntegrationConfigurable #4081

porter-stripe commented Oct 1, 2024 •

edited

Loading

porter-stripe Oct 1, 2024

porter-stripe Oct 1, 2024

yuki-stripe Oct 2, 2024

yuki-stripe Oct 2, 2024

porter-stripe Oct 2, 2024

github-actions bot commented Oct 1, 2024 •

edited

Loading

porter-stripe commented Oct 1, 2024

yuki-stripe Oct 2, 2024

yuki-stripe Oct 2, 2024

yuki-stripe Oct 2, 2024

porter-stripe Oct 2, 2024

yuki-stripe Oct 2, 2024

		payload["form_sheet_action"] = formSheetAction.analyticValue
		payload["hide_mandate_text"] = hidesMandateText

		let configuration: PaymentSheet.Configuration
		let configuration: IntegrationConfigurable

Introduce IntegrationConfigurable #4081

Introduce IntegrationConfigurable #4081

Conversation

porter-stripe commented Oct 1, 2024 • edited Loading

Summary

Motivation

Testing

Changelog

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

github-actions bot commented Oct 1, 2024 • edited Loading

porter-stripe commented Oct 1, 2024

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

porter-stripe commented Oct 1, 2024 •

edited

Loading

github-actions bot commented Oct 1, 2024 •

edited

Loading