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

Pass hasUserLocationConsent to DataCollector #472

Conversation

tdchow
Copy link
Contributor

@tdchow tdchow commented Apr 17, 2024

Summary of changes

  • Add javadocs for hasUserLocationConsent param
  • Pass hasUserLocationConsent to DataCollector.collectDeviceData()

Checklist

  • Added a changelog entry

Authors

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

@tdchow tdchow requested a review from a team as a code owner April 17, 2024 14:49
}
// TODO: set hasUserLocationConsent on paypalRequest
dropInRequest.hasUserLocationConsent();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

hasUserLocationConsent is set on the PayPalRequest directly. No need to set it here.

Copy link
Contributor

Choose a reason for hiding this comment

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

If the merchant has set hasUserLocationConsent on their dropInRequest already - can we pass that value in when creating the default PayPalVaultRequest?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, great suggestion! i'll update this.

unrelated question - if this function gets called with a null PayPalRequest and a default one is created, are there scenarios where payPalClient.tokenizePayPalAccount() will ever succeed? are there required fields on the request?

Copy link
Contributor

Choose a reason for hiding this comment

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

Before this change, there were no required fields for a PayPalVaultRequest (checkout request requires amount). So if a merchant doesn't pass a custom PayPalRequest to their DropInRequest, drop-ins default behavior is the PayPal billing agreement/vault flow - it should succeed when we create the default vault request.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was seeing some errors thrown when returnUrl is not set on the request. I'm not sure if this was just an issue in the PayPal Native module. I'll do some testing outside the scope of this PR/story.

I just pushed the change to pass hasUserLocationConsent from the dropInRequest.

@sarahkoop
Copy link
Contributor

Is there going to be a separate PR to pass this flag to all other instances of collectDeviceData throughout drop-in?

@tdchow
Copy link
Contributor Author

tdchow commented Apr 17, 2024

Is there going to be a separate PR to pass this flag to all other instances of collectDeviceData throughout drop-in?

I was intending this to be the final PR for drop-in. I wasn't able to find any other instances where we'd need to pass the flag when searching for "collectDeviceData" in the repo. Are there other callsites that I'm currently missing?

@tdchow tdchow merged commit 8fc5ee9 into user-location-consent-feature Apr 17, 2024
4 checks passed
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.

None yet

3 participants