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

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

Merged
merged 4 commits into from
Nov 8, 2023

Conversation

jaxdesmarais
Copy link
Contributor

Summary of changes

  • Fix bug where defaults for BTThreeDSecureRequest.accountType, BTThreeDSecureRequest.requestedExemptionType, and BTThreeDSecureRequest.dfReferenceID were improperly returning an error if not passed into the request
  • The Any in these cases for the dictionary should be optional to allow for nil values (behavior in v5) - currently we were defaulting to an empty string in the nil case which is invalid
  • Add UnitTest for this case in the future

Checklist

  • Added a changelog entry

Authors

@jaxdesmarais jaxdesmarais requested a review from a team as a code owner November 8, 2023 14:42
@jaxdesmarais jaxdesmarais changed the title Fix Bug where some BTThreeDSecureRequest Default Values Returned Errors Unexpectedly Fix Bug where some BTThreeDSecureRequest Default Values Returned Errors Unexpectedly Nov 8, 2023
"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. 🥳

@jaxdesmarais jaxdesmarais merged commit 74f056a into main Nov 8, 2023
6 checks passed
@jaxdesmarais jaxdesmarais deleted the fix-3ds-request-bug branch November 8, 2023 18:39
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.

3 participants