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

Update StripeConnect's FinancialConnections integration to work with public key override. #4379

Merged
merged 5 commits into from
Dec 21, 2024

Conversation

nschris-stripe
Copy link
Contributor

Summary

  • Ensures the financial connection integration uses the public key override.
  • Updates implementation to make it more testable.

Copy link

github-actions bot commented Dec 19, 2024

🚨 New dead code detected in this PR:

FinancialConnectionsPresenter.swift:21 warning: Parameter 'presentingViewController' 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.

@nschris-stripe nschris-stripe force-pushed the nschris/fixFinancialConnectionsDashboard branch from 3e17301 to a3e8b57 Compare December 19, 2024 12:27
Copy link
Collaborator

@mludowise-stripe mludowise-stripe left a comment

Choose a reason for hiding this comment

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

lgtm -- not sure if you wanted to add an xctassertequal on the client secret. If so, lmk and I'll reshipit

@mludowise-stripe
Copy link
Collaborator

Looks like testOpenFinancialConnections tests need to get updated ConnectComponentWebViewControllerTests. Looks like they're failing because the method signature changed for overridePresentForToken

@nschris-stripe nschris-stripe force-pushed the nschris/fixFinancialConnectionsDashboard branch from a3e8b57 to 2cf8a48 Compare December 20, 2024 17:25
@nschris-stripe nschris-stripe changed the title Update stripe connect integration to work with public key override. Update stripe connect Financial Connections integration to work with public key override. Dec 20, 2024
@nschris-stripe nschris-stripe changed the title Update stripe connect Financial Connections integration to work with public key override. Update StripeConnect's Financial Connections integration to work with public key override. Dec 20, 2024
@nschris-stripe nschris-stripe changed the title Update StripeConnect's Financial Connections integration to work with public key override. Update StripeConnect's FinancialConnections integration to work with public key override. Dec 20, 2024
@nschris-stripe nschris-stripe force-pushed the nschris/fixFinancialConnectionsDashboard branch from 2cf8a48 to b514231 Compare December 20, 2024 17:51
Comment on lines +467 to +468
XCTAssert(compManager.apiClient == componentManager.apiClient)
XCTAssert(compManager.publicKeyOverride == componentManager.publicKeyOverride)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Doublechecking – STPAPIClient subclasses NSObject and doesn't override isEqual, so I think this should just do pointer-comparison, right? Just want to make sure the equality comparison does what we think it should.

Non-blocking: Also, if we're using == anyway, this could be XCTAssertEqual which would provide a better failure message if the test fails.

Suggested change
XCTAssert(compManager.apiClient == componentManager.apiClient)
XCTAssert(compManager.publicKeyOverride == componentManager.publicKeyOverride)
XCTAssertEqual(compManager.apiClient, componentManager.apiClient)
XCTAssertEqual(compManager.publicKeyOverride, componentManager.publicKeyOverride)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah this should be doing pointer comparison.

Copy link

emerge-tools bot commented Dec 20, 2024

📸 Snapshot Test

No snapshots generated

Name Added Removed Modified Unchanged Errored Approval
StripePaymentsUISize
com.stripe.StripePaymentsUISize
0 0 0 0 0 N/A
StripeApplePaySize
com.stripe.StripeApplePaySize
0 0 0 0 0 N/A
StripeSize
com.stripe.StripeSize
0 0 0 0 0 N/A
StripePaymentSheetSize
com.stripe.StripePaymentSheetSize
0 0 0 0 0 N/A
StripeIdentitySize
com.stripe.StripeIdentitySize
0 0 0 0 0 N/A
StripePaymentsSize
com.stripe.StripePaymentsSize
0 0 0 0 0 N/A
StripeFinancialConnectionsSize
com.stripe.StripeFinancialConnectionsSize
0 0 0 0 0 N/A
StripeConnectSize
com.stripe.StripeConnectSize
0 0 0 0 0 N/A

🛸 Powered by Emerge Tools

@nschris-stripe nschris-stripe merged commit e6546d7 into master Dec 21, 2024
6 checks passed
@nschris-stripe nschris-stripe deleted the nschris/fixFinancialConnectionsDashboard branch December 21, 2024 19:52
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