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.

Sign up for GitHub

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

Fix Bug where some BTThreeDSecureRequest Default Values Returned Errors Unexpectedly #1132

Merged
merged 4 commits into from
Nov 8, 2023
Merged

Fix Bug where some BTThreeDSecureRequest Default Values Returned Errors Unexpectedly #1132

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1cb8391
make any optional in 3ds requestParameters to resolve bug with defaul…
jaxdesmarais Nov 8, 2023
850f094
add UnitTest for building request with nil values as expected
jaxdesmarais Nov 8, 2023
97af8da
add CHANGELOG entry
jaxdesmarais Nov 8, 2023
412696b
update casting to resolve cocopods warning
jaxdesmarais Nov 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
* BraintreeThreeDSecure
* Add `cardAddChallengeRequested` to `BTThreeDSecureRequest`
* Deprecate `BTThreeDSecureRequest.cardAddChallenge`
* Fix bug where defaults for `BTThreeDSecureRequest.accountType`, `BTThreeDSecureRequest.requestedExemptionType`, and `BTThreeDSecureRequest.dfReferenceID` were improperly returning an error if not passed into the request
* BraintreeCard
* Deprecate unused `BTCardRequest` class

Expand Down
12 changes: 6 additions & 6 deletions Sources/BraintreeThreeDSecure/BTThreeDSecureClient.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -339,15 +339,15 @@ import BraintreeCore

let customer: [String: String] = [:]

var requestParameters: [String: Any] = [
"amount": request.amount ?? 0,
var requestParameters: [String: Any?] = [
"amount": request.amount,
"customer": customer,
"requestedThreeDSecureVersion": "2",
"dfReferenceId": request.dfReferenceID ?? "",
"accountType": request.accountType.stringValue ?? "",
"dfReferenceId": request.dfReferenceID,
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious why we want to pass nil rather than just not including the param in the request if it's null? Is the behavior the same either way?

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 the behavior would be the same - we could do either technically, seems like v5 did a nil check then passed the key/value if not nil (code here). I'm fine with either option and this is slightly cleaner than a bunch of if/else below but if folks felt strongly we could extract these out.

It's also worth noting when we do compactMapValues below we strip out any nil key/values so either option has the same effect. From a playground:

let myDictionary: [String: Any?] = [
    "string1": "hasValue",
    "string2": nil,
    "int1": 0
]

print(myDictionary)
print(myDictionary.compactMapValues { $0 })

The above would return with compactMapValues stripping out nil values:

["string2": nil, "string1": Optional("hasValue"), "int1": Optional(0)] // print(myDictionary)
["int1": 0, "string1": "hasValue"] // print(myDictionary.compactMapValues { $0 })

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah cool thanks for the explanation!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem! compactMapValues is pretty cool and was definitely a benefit of the Swift move. 🥳

"accountType": request.accountType.stringValue,
"challengeRequested": request.challengeRequested,
"exemptionRequested": request.exemptionRequested,
"requestedExemptionType": request.requestedExemptionType.stringValue ?? "",
"requestedExemptionType": request.requestedExemptionType.stringValue,
"dataOnlyRequested": request.dataOnlyRequested
]

Expand Down Expand Up @@ -380,7 +380,7 @@ import BraintreeCore

self.apiClient.post(
"v1/payment_methods/\(urlSafeNonce)/three_d_secure/lookup",
parameters: requestParameters
parameters: requestParameters as [String: Any]
) { body, _, error in
if let error = error as NSError? {
// Provide more context for card validation error when status code 422
Expand Down
19 changes: 19 additions & 0 deletions UnitTests/BraintreeThreeDSecureTests/BTThreeDSecureClient_Tests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,25 @@ class BTThreeDSecureClient_Tests: XCTestCase {
waitForExpectations(timeout: 1, handler: nil)
}

func testPerformThreeDSecureLookup_whenDefaultsArePassed_buildsRequestWithNilValues() {
let expectation = expectation(description: "willCallCompletion")

threeDSecureRequest.nonce = "fake-card-nonce"
threeDSecureRequest.amount = 9.99

client.performThreeDSecureLookup(threeDSecureRequest) { _, _ in
XCTAssertEqual(self.mockAPIClient.lastPOSTParameters!["amount"] as! NSDecimalNumber, 9.99)
XCTAssertEqual(self.mockAPIClient.lastPOSTParameters!["requestedThreeDSecureVersion"] as! String, "2")
XCTAssertNil(self.mockAPIClient.lastPOSTParameters!["dfReferenceId"] as? String)
XCTAssertNil(self.mockAPIClient.lastPOSTParameters!["accountType"] as? String)
XCTAssertNil(self.mockAPIClient.lastPOSTParameters!["requestedExemptionType"] as? String)

expectation.fulfill()
}

waitForExpectations(timeout: 1)
}

func testPerformThreeDSecureLookup_whenCardAddChallengeNotRequested_sendsCardAddFalse() {
let expectation = self.expectation(description: "willCallCompletion")

Expand Down