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

Egeniesse/add sunbit #3724

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Egeniesse/add sunbit #3724

wants to merge 2 commits into from

Conversation

egeniesse-stripe
Copy link

@egeniesse-stripe egeniesse-stripe commented Jun 27, 2024

Summary

Adding the Sunbit payment method to PaymentSheets and the API bindings. For information about Sunbit and why we're integrating it, check out its product brief and API review doc.

Motivation

Testing

  • Added tests
  • Manually verified

Changelog

Screen.Recording.2024-06-28.at.9.18.43.AM.mov

@egeniesse-stripe egeniesse-stripe requested review from a team as code owners June 27, 2024 21:43
Copy link

github-actions bot commented Jun 27, 2024

⚠️ Public API changes detected:

StripePayments

+ get
+ }
+ @objc public var sunbit: StripePayments.STPPaymentMethodSunbit? {
+ case sunbit
+ @objc public var sunbit: StripePayments.STPPaymentMethodSunbitParams?
+ @objc convenience public init(sunbit: StripePayments.STPPaymentMethodSunbitParams, billingDetails: StripePayments.STPPaymentMethodBillingDetails?, metadata: [Swift.String : Swift.String]?)
+ @objc public var allResponseFields: [Swift.AnyHashable : Any] {
+ get
+ }
+ @objc override dynamic public var description: Swift.String {
+ @objc get
+ }
+ @objc public class func decodedObject(fromAPIResponse response: [Swift.AnyHashable : Any]?) -> Self?
+ @objc deinit
+ @objc public var additionalAPIParameters: [Swift.AnyHashable : Any]
+ @objc public class func rootObjectName() -> Swift.String?
+ @objc public class func propertyNamesToFormFieldNamesMapping() -> [Swift.String : Swift.String]
+ @objc override dynamic public init()
+ @objc deinit

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.

@egeniesse-stripe egeniesse-stripe force-pushed the egeniesse/add-sunbit branch 6 times, most recently from a18fbac to 89018fb Compare July 1, 2024 16:17
@@ -2,6 +2,10 @@

### Payments
* [Fixed] An issue where the correct card brand was not being displayed for card brand choice in STPPaymentOptionsViewController and STPPaymentContext.
* [Added] Support for Sunbit bindings.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we add (Private Beta) too or no

Comment on lines +1079 to +1118

func testLpm_Sunbit_AutomaticFields() throws {
var settings = PaymentSheetTestPlaygroundSettings.defaultValues()
settings.customerMode = .guest
settings.currency = .usd
settings.merchantCountryCode = .US
settings.applePayEnabled = .off
settings.apmsEnabled = .off
settings.linkEnabled = .off
settings.attachDefaults = .off
settings.collectName = .automatic
settings.collectEmail = .automatic
settings.collectPhone = .automatic
settings.collectAddress = .automatic
loadPlayground(
app,
settings
)

checkoutButton.tap()

let cell = try XCTUnwrap(scroll(collectionView: app.collectionViews.firstMatch, toFindCellWithId: "sunbit"))
cell.tap()

XCTAssertFalse(emailField.exists)
XCTAssertFalse(fullNameField.exists)
XCTAssertFalse(phoneField.exists)
XCTAssertFalse(phoneField.exists)
XCTAssertFalse(billingAddressField.exists)
XCTAssertFalse(app.textFields["Country"].exists)
XCTAssertFalse(line1Field.exists)
XCTAssertFalse(line2Field.exists)
XCTAssertFalse(cityField.exists)
XCTAssertFalse(stateField.exists)
XCTAssertFalse(zipField.exists)

// Just check the button is enabled
var payButton: XCUIElement { app.buttons["Pay $50.99"] }
XCTAssertTrue(payButton.isEnabled)
}
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 remove these, not sure why we started this test pattern. There's nothing sunbit-specific about automatic billing detail collection, we can probably craft good unit tests instead of all of these UI tests (cc @davidme-stripe ).

Comment on lines +628 to +650
func testSunbitPaymentMethod() throws {
var settings = PaymentSheetTestPlaygroundSettings.defaultValues()
settings.currency = .usd
settings.merchantCountryCode = .US
settings.customerMode = .new
settings.apmsEnabled = .off
loadPlayground(app, settings)
app.buttons["Present PaymentSheet"].tap()
let payButton = app.buttons["Pay $50.99"]

// Select Sunbit
guard let sunbit = scroll(collectionView: app.collectionViews.firstMatch, toFindCellWithId: "Sunbit") else {
XCTFail()
return
}
sunbit.tap()

XCTAssertTrue(payButton.isEnabled)

// Attempt payment, should succeed
payButton.tap()
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

I understand you're just copying the existing test pattern here, but I think these tests need to complete payment or they're not testing enough to be useful. You can leave it as-is in this PR, I can't find an example to copy that completes the payment.

cc @davidme-stripe I think we should make all of these complete payment, there's nothing else that can test this final part of the checkout flow of 'next action handled -> paymenthandler completes payment'.

XCTAssertNil(error)
XCTAssertNotNil(paymentMethod, "Payment method should be populated")
XCTAssertEqual(paymentMethod?.type, .sunbit, "Incorrect PaymentMethod type")
XCTAssertNotNil(paymentMethod?.sunbit, "The `sunbit` property must be populated")
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks entirely redundant with the test in STPPaymentMethodFunctionalTest except for this line. Why did we start doing this..

Let's delete this file and add this line to STPPaymentMethodFunctionalTest.

Not a huge win but we should probably do the same for all the other "Params" tests that actually just create PMs :/ @davidme-stripe

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants