refactor: SDK existing file modifications for CheckoutComponents#1632
refactor: SDK existing file modifications for CheckoutComponents#1632bthebladeprimer wants to merge 7 commits into
Conversation
Generated by 🚫 Danger Swift against 9c8d8ab |
| ) | ||
| ) | ||
|
|
||
| // Note: v3.0 breaking change — Mastercard formatting changed from [4, 10] |
There was a problem hiding this comment.
Curious where this change came from - product-led or something uncovered during development?
| /// ``` | ||
| /// - Note: **v3.0 breaking change**: This enum is now `public`. All cases are part of the | ||
| /// public API contract — no cases can be removed or renamed without a breaking change. | ||
| public enum PrimerPaymentMethodType: String, Codable, CaseIterable, Equatable, Hashable { |
There was a problem hiding this comment.
What is the rationale behind making this public?
I dont believe this is an exhaustive list due to how WEB_REDIRECT payment methods work, payment methods can be added and will work with the SDK without additions here. cc @henry-cooper-primer for a sense-check in case my worldview is out of date.
If it is not an exhaustive list of supported payment methods, it may be confusing to a merchant if exposed
There was a problem hiding this comment.
I would also add that allowing the posibility for this to be extended by the merchant could cause unknown headaches
| extension URLSession: URLSessionProtocol {} | ||
|
|
||
| final class DefaultRequestDispatcher: RequestDispatcher, LogReporter { | ||
| final class DefaultRequestDispatcher: RequestDispatcher, @unchecked Sendable, LogReporter { |
There was a problem hiding this comment.
@unchecked here makes me nervous. Why is it needed? I would like to see unit tests which test multi-threaded behaviour if we need it, I worry about races/crashes if this runs on multiple threads.
| return text.contains("@") && text.contains(".") | ||
| default: | ||
| return true | ||
| return false |
There was a problem hiding this comment.
A latent bug, or was this required because the cases were not exhaustive before?
| DispatchQueue.main.async { | ||
| rawDataManager.delegate?.primerRawDataManager?( | ||
| rawDataManager, | ||
| metadataDidChange: ["cardNetwork": self.cardNetwork.rawValue] |
There was a problem hiding this comment.
metadataDidChange is deprecated, but here we stop sending data there. I would say either remove it, or send to both until removed
nquinn-primer
left a comment
There was a problem hiding this comment.
Nice work, added some comments
| @@ -298,70 +336,55 @@ extension MerchantHeadlessCheckoutRawDataViewController: PrimerHeadlessUniversal | |||
| let printableNetworks = metadata.detectedCardNetworks.items.map(\.network.rawValue).joined(separator: ", ") | |||
| print("[MerchantHeadlessCheckoutRawDataViewController] didReceiveCardMetadata: \(printableNetworks) forCardValidationState: \(cardState.cardNumber)") | |||
There was a problem hiding this comment.
We shouldnt be exposing the unexposed cardnumber for PCI reasons. Can you check if we ever did this before with PrimerValidationState - is it something new in PrimerCardNumberEntryState?
| .cvv, | ||
| .otp, | ||
| .phoneNumber: | ||
| case .cardNumber, .expiryDate, .cvv, .otp, .phoneNumber, .postalCode: |
There was a problem hiding this comment.
.postalCode has moved from .alphabet to .numberPad. This will cause issues in many countries where postalCode is not only numeric
| return true | ||
| } | ||
|
|
||
| func application(_ app: UIApplication, open url: URL, options: [UIApplication.OpenURLOptionsKey: Any] = [:]) -> Bool { |
There was a problem hiding this comment.
DeepLink support removed? Is this intended?
There was a problem hiding this comment.
If it is handled elsewhere, then please make sure to test all redirect methods and E2E tests which rely on this
| /// | ||
| /// You can enable multiple dismissal mechanisms by passing an array to `PrimerUIOptions`. | ||
| /// For example, `[.gestures, .closeButton]` allows both swipe gestures and a close button. | ||
| public enum DismissalMechanism: String, Codable { |
There was a problem hiding this comment.
A breaking change - fine since we are going to 3.0 but we will need to keep track of these public breaking changes for the future CHANGELOG
| case .firstName, .lastName: | ||
| return text.isValidNonDecimalString | ||
| case .email: | ||
| return text.contains("@") && text.contains(".") |
There was a problem hiding this comment.
Perhaps we can use a more robust email regex - I would suggest aligning across platforms
|
|
||
| /// Human-readable field name for display | ||
| var displayName: String { | ||
| switch self { |
There was a problem hiding this comment.
Should these display names be pulling from Localized strings?
| return imgView | ||
| } | ||
|
|
||
| if Thread.isMainThread { |
There was a problem hiding this comment.
is this required since the function is @mainactor?
| XCTAssertEqual(report.recommendations.count, 2) | ||
| } | ||
|
|
||
| // MARK: - printDetailedReport / printReport Tests |
There was a problem hiding this comment.
These tests are deleted but these code paths still exist in production. Intended?
637b7c4 to
01baa63
Compare
This reverts commit d2f62bf.
Restore call to deprecated primerHeadlessUniveraslCheckoutUIDidDismissPaymentMethod in both PrimerDelegateProxy dismiss overloads to maintain backward compatibility with merchants still using the typo method name.
2e87ec4 to
9c8d8ab
Compare
| (internalText ?? "").isEmpty | ||
| } | ||
|
|
||
| func wipe() { |
There was a problem hiding this comment.
This leaves data behind in memory since utf8CString returns a copy - I wonder is now a good time to resolve this and work towards killing this VULN ticket at the same time cc @marjaimate
Summary
Context
PR 3 of 15 in the CheckoutComponents feature split. This is the highest-risk PR as it touches existing SDK files across the codebase. Depends on PR 2 (#1631).
Tracker: https://www.notion.so/32fca65dc30e81dabf66f33e2b58c3a1
Test plan
xcodebuild -workspace PrimerSDK.xcworkspace -scheme PrimerSDKTests -destination "platform=iOS Simulator,name=iPhone 16" test