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

Introduce IntegrationConfigurable #4081

Merged
merged 7 commits into from
Oct 3, 2024
Merged

Conversation

porter-stripe
Copy link
Collaborator

@porter-stripe porter-stripe commented Oct 1, 2024

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

Testing

  • Compiler
  • CI

Changelog

N/A

Comment on lines 373 to 374
payload["form_sheet_action"] = formSheetAction.analyticValue
payload["hide_mandate_text"] = hidesMandateText
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note these new embedded only params.

/// 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 {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Open to naming suggestions

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link

github-actions bot commented Oct 1, 2024

🚨 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
Copy link
Collaborator Author

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:

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

@porter-stripe porter-stripe marked this pull request as ready for review October 1, 2024 18:41
@porter-stripe porter-stripe requested review from a team as code owners October 1, 2024 18:41
@@ -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.
Copy link
Collaborator

Choose a reason for hiding this comment

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

/// 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 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

let configuration: PaymentSheet.Configuration
let configuration: IntegrationConfigurable
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

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 looking at the class more, it'll be pretty hard. We'll want some embedded specific one.

/// 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 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

@porter-stripe porter-stripe merged commit 10bf64a into master Oct 3, 2024
6 checks passed
@porter-stripe porter-stripe deleted the porter/MOBILESDK-2533 branch October 3, 2024 15:26
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