Skip to content

Conversation

@nsingh-branch
Copy link
Contributor

Reference

SDK-2459 -- [Android] Implement Consumer Protection Preferences

Description

Added a new method for setting the consumer protection preference, setConsumerProtectionPreference. This value can be set and changed at any time, but persists across sessions. This change also deprecated the disableTracking() method since setting the preference to TRACKING_DISABLED will perform the same logic.

Testing Instructions

Use the new method or the new testbed "Consumer Protection Preference" button to change the preference and observe the protection_preference field change in each request.

Risk Assessment [MEDIUM]

It deprecated a frequently used method and replaces it with a new one with some shared logic.

  • I, the PR creator, have tested — integration, unit, or otherwise — this code.

Reviewer Checklist (To be checked off by the reviewer only)

  • JIRA Ticket is referenced in PR title.
  • Correctness & Style
    • Conforms to AOSP Style Guides
    • Mission critical pieces are documented in code and out of code as needed.
  • Unit Tests reviewed and test issue sufficiently.
  • Functionality was reviewed in QA independently by another engineer on the team.

cc @BranchMetrics/saas-sdk-devs for visibility.

@gdeluna-branch
Copy link
Contributor

Automation is faling

@gdeluna-branch
Copy link
Contributor

Please reopen when ready.

@nsingh-branch nsingh-branch reopened this Nov 1, 2024
* ({@code false}).
* @deprecated Use {@link #setConsumerProtectionAttributionLevel(Defines.BranchAttributionLevel)} instead.
*/
public void disableTracking(boolean disableTracking) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you test this with the delayed init plus disable tracking by default?

updateAdvertisingIdsObject(gaid);
// gaid is put in the request body below, calling to remove hardware id from request now
replaceHardwareIdOnValidAdvertisingId();
if (prefHelper_.getConsumerProtectionAttributionLevel() == Defines.BranchAttributionLevel.FULL || !prefHelper_.isAttributionLevelInitialized()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I think it would be slightly easier to read && prefHelper_.isAttributionLevelInitialized()

I would also suggest that this logical check just be made into its own function since it's repeated several times.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So i'm actually checking for either of them to be true, not both. More details in the comment below, but basically the CPP level not being set should be treated the same as it being set to full.

utility.parseReferringURL(externalIntentUri);

JSONObject urlQueryParams = utility.getURLQueryParamsForRequest(this);
if (prefHelper_.getConsumerProtectionAttributionLevel() == Defines.BranchAttributionLevel.FULL || !prefHelper_.isAttributionLevelInitialized()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible for getConsumerProtectionAttributionLevel to have a value and isAttributionLevelInitialized not be true?

In other words, if prefHelper_.getConsumerProtectionAttributionLevel() == Defines.BranchAttributionLevel.FULL how can prefHelper_.isAttributionLevelInitialized() be false? Correct me if I'm wrong but it seems redundant

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Basically we want this if statement to be true if the CPP level is FULL or if it hasn't been set, since that's treated like FULL. However, we don't want to always add the cpp_level field to requests, so i didn't just set the default for getConsumerProtectionAttributionLevel() to be FULL.

So thats why im checking for either the CPP level to be full OR for it to not be set here.

@nsingh-branch nsingh-branch merged commit 5903ba9 into master Nov 18, 2024
6 of 9 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.

2 participants