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

Jump to bottom

Expose isDeviceToken on BTApplePayCardNonce #1516

Merged
merged 5 commits into from
Feb 10, 2025
Merged

Expose isDeviceToken on BTApplePayCardNonce #1516

merged 5 commits into from
Feb 10, 2025

Conversation

stechiu
Copy link
Collaborator

@stechiu stechiu commented Feb 6, 2025 •
edited
Loading

With this PR, Merchants will have the ability to see if their ApplePay payment was tokenized as an MPAN or DPAN.

Summary of changes

  • Exposed isDeviceToken
  • false == DPAN
  • true == MPAN
Screenshot 2025-02-05 at 4 34 16 PM

Checklist

  • Added a changelog entry
  • Tested and confirmed payment flows affected by this change are functioning as expected

Authors

List GitHub usernames for everyone who contributed to this pull request.

@stechiu stechiu requested a review from a team as a code owner February 6, 2025 01:05
stechiu
stechiu commented Feb 6, 2025
View reviewed changes
scannillo
scannillo approved these changes Feb 6, 2025
View reviewed changes
Copy link
Contributor

@scannillo scannillo left a comment

Choose a reason for hiding this comment

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

👏 Thanks for also including details on testing / seeing it working E2E

@scannillo
Copy link
Contributor

scannillo commented Feb 6, 2025 •
edited
Loading

❓ Actually, is the false you're seeing just an SDK default - or is the API giving back false? We were expecting the API to always give back true until the Demo app gets updated to specifically request an MPAN.

We will have to decide what value to use if there is NONE returned back from API - either true (is DPAN) or nil.

@scannillo scannillo self-requested a review February 6, 2025 15:22
@warmkesselj warmkesselj self-assigned this Feb 6, 2025
@stechiu
Copy link
Collaborator Author

stechiu commented Feb 6, 2025

❓ Actually, is the false you're seeing just an SDK default - or is the API giving back false? We were expecting the API to always give back true until the Demo app gets updated to specifically request an MPAN.

We will have to decide what value to use if there is NONE returned back from API - either true (is DPAN) or nil.

I believe the value is coming from the SDK. I'm not sure if I exposed the key properly for the value to come from API...

richherrera
richherrera approved these changes Feb 6, 2025
View reviewed changes
Copy link
Contributor

@richherrera richherrera left a comment

Choose a reason for hiding this comment

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

LGTM!!

jaxdesmarais
jaxdesmarais approved these changes Feb 7, 2025
View reviewed changes
Copy link
Contributor

@jaxdesmarais jaxdesmarais left a comment

Choose a reason for hiding this comment

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

Couple of very minor comments but otherwise looks great!

CHANGELOG.md Outdated
@@ -1,5 +1,9 @@
# Braintree iOS SDK Release Notes

## unreleased
* BraintreeApplePay
* Add BTApplePayCardNonce.isDeviceToken for MPAN identification
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Add BTApplePayCardNonce.isDeviceToken for MPAN identification
* Add `BTApplePayCardNonce.isDeviceToken` for MPAN identification

Sources/BraintreeApplePay/BTApplePayCardNonce.swift Outdated
Comment on lines 22 to 24
let isDeviceToken = json["details"]["isDeviceToken"].asBool() ?? true

self.isDeviceToken = isDeviceToken
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we just extract the properties above because it'd make the super init a mess. Here I think we can do:

Suggested change
let isDeviceToken = json["details"]["isDeviceToken"].asBool() ?? true
self.isDeviceToken = isDeviceToken
self.isDeviceToken = json["details"]["isDeviceToken"].asBool() ?? true

@stechiu
Copy link
Collaborator Author

stechiu commented Feb 7, 2025

Couple of very minor comments but otherwise looks great!

Great thank you!

agedd
agedd approved these changes Feb 10, 2025
View reviewed changes
Copy link
Contributor

@agedd agedd left a comment •
edited
Loading

Choose a reason for hiding this comment

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

lgtm! 🚀

@stechiu stechiu merged commit d48d6f8 into mpan Feb 10, 2025
7 checks passed
@stechiu stechiu deleted the mpan-feature-expose-isDeviceToken branch February 10, 2025 21:41
@stechiu stechiu restored the mpan-feature-expose-isDeviceToken branch February 12, 2025 22:31
@stechiu stechiu mentioned this pull request Feb 12, 2025
Merged
2 tasks
@stechiu stechiu deleted the mpan-feature-expose-isDeviceToken branch February 12, 2025 22:41
@stechiu stechiu mentioned this pull request Feb 12, 2025
Merged
2 tasks
stechiu added a commit that referenced this pull request Feb 13, 2025
@stechiu
Merge mpan feature branch into main (#1521)
05f1b03 
* Exposed `isDeviceToken`

* Added unit tests

* Changed default to `true`

* Updated demo app

* Expose `isDeviceToken` on BTApplePayCardNonce (#1516)

* Addressed PR comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@scannillo scannillo scannillo approved these changes

@jaxdesmarais jaxdesmarais jaxdesmarais approved these changes

@agedd agedd agedd approved these changes

@richherrera richherrera richherrera approved these changes

@warmkesselj warmkesselj warmkesselj approved these changes

Assignees

@warmkesselj warmkesselj

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@stechiu @scannillo @jaxdesmarais @agedd @richherrera @warmkesselj