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

Collect Device Data #339

Merged
merged 5 commits into from
Jul 20, 2021
Merged

Collect Device Data #339

merged 5 commits into from
Jul 20, 2021

Conversation

scannillo
Copy link
Contributor

@scannillo scannillo commented Jul 8, 2021

Changes

  • Expose deviceData on BTDropInResult

Motivation

  • Android & Web DropIn offer device data collection capabilities.
  • In Android, you have to set collectDeviceData to true on your DropInRequest here. If you do, only then will you get back device data on your DropInResult here.
    • For iOS, I decided not to require merchants set the collectDeviceData boolean property on their request, and instead always include deviceData on the DropInResult.
    • I think since this is an additive (non-breaking) change, this is an easier integration pattern for merchants. They don't have to worry about requesting the data. They will always get it and can decide to ignore or use it.

Concerns

I had a concern about performance, but used XCTest's measure function to time this test. I get roughly the same time to initialize the DropInResult with and without the call to PPDataCollector. I think it is OK to add.

Checklist

  • Added a changelog entry

Authors

@scannillo

Comment on lines +38 to +40
// For testing
static Class PayPalDataCollectorClass;
static NSString *PayPalDataCollectorClassString = @"PPDataCollector";
Copy link
Contributor Author

@scannillo scannillo Jul 8, 2021

Choose a reason for hiding this comment

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

I think we can do this with OCMock instead (see docs under the 3.2 Stubs and verification section).

This project doesn't use OCMock, so I tried to import it. OCMock has an SPM bug, which I reported on their GitHub. So rather than use a 2nd package manager for the project to fetch OCMock, I went for this approach.

We can always change this testing situation in the future. Let me know what ya'll think.

Copy link
Contributor

Choose a reason for hiding this comment

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

This approach seems reasonable to me. No need to introduce another package manager for testing.

@scannillo scannillo marked this pull request as ready for review July 8, 2021 20:18
@scannillo scannillo requested a review from a team as a code owner July 8, 2021 20:18
Copy link
Contributor

@sarahkoop sarahkoop left a comment

Choose a reason for hiding this comment

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

This looks good to me! Should we mark collectDeviceData on BTDropInRequest as deprecated if it's always going to be requested and exposed now?

@scannillo
Copy link
Contributor Author

Should we mark collectDeviceData on BTDropInRequest as deprecated if it's always going to be requested and exposed now?

@sarahkoop There is no collectDeviceData on BTDropInRequest, so we should be good!

@scannillo scannillo merged commit 8511233 into master Jul 20, 2021
@scannillo scannillo deleted the collect-device-data branch July 20, 2021 14:18
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