Skip to content

Conversation

@aitorvs
Copy link
Collaborator

@aitorvs aitorvs commented Aug 4, 2021

Task/Issue URL: https://app.asana.com/0/414730916066338/1200714585958657/f
Tech Design URL:
CC:

Description:
All our pixels currently send the ATB as a parameter but some pixels should not send such information, for instance EMAIL_TOOLTIP_DISMISSED, EMAIL_USE_ALIAS, , EMAIL_USE_ADDRESS.

This PR:

  • Adds a PixelAtbRemovalInterceptor that is added to the pixel okHttp instance that will redact the atb parameter from the pixel if necessary
  • Adds an inline interceptor to the pixel okHttp instance that just logs the final request url that is sent so that we can easily check in the console in debug builds
  • we already had some timbers that logged the pixel params etc, but as we are modifying the request using an interceptor, those timber may not contain the request params that are actually going out
  • Adds PixelAtbRemovalInterceptorTest that verifies that for all pixels in AppPixelName only contain the atb param those which are supposse to

Steps to test this PR:

  1. install from this branch
  2. filter logcat by Pixel url request:
  3. add your personal duck address in settings -> Email Protection
  4. navigate to github.com
  5. in the email address input field dax should appear
  6. click on dax
  7. dismiss the dialog that appears
  8. verify that Pixel url request: https://improving.duckduckgo.com/t/m_e_t_d_android_phone?appVersion=5.92.1&cohort=internal_beta&test=1 appears in logcat
  • the important thing is that pixel should be m_e_t_d and atb param is not in url
  1. click on dax
  2. click on User yourEmail@duck.com
  3. verify that Pixel url request: https://improving.duckduckgo.com/t/m_e_uad_android_phone?appVersion=5.92.1&cohort=internal_beta&test=1 appears in logcat
  • the important thing is that pixel should be m_e_uad and atb param is not in url
  1. click on dax
  2. click on Generate private address
  3. verify that Pixel url request: https://improving.duckduckgo.com/t/m_e_ua_android_phone?appVersion=5.92.1&cohort=internal_beta&test=1 appears in logcat
  • the important thing is that pixel should be m_e_ua and atb param is not in url

If you remove the PixelAtbRemovalInterceptor form the okHttp instance and perform the test steps above, all three pixels should have the atb param

The PixelAtbRemovalInterceptorTest should ensure that no other pixel is affected by this change


Internal references:

Software Engineering Expectations
Technical Design Template

@marcosholgado marcosholgado self-requested a review August 5, 2021 08:48
@marcosholgado marcosholgado self-assigned this Aug 5, 2021
// list of pixels for which we'll remove the ATB information
@VisibleForTesting
val pixels = listOf(
"m_e_t_d",
Copy link
Contributor

@marcosholgado marcosholgado Aug 5, 2021

Choose a reason for hiding this comment

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

Can we use here AppPixelName.EMAIL_TOOLTIP_DISMISSED.pixelName, etc instead of the actual values?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think it makes sense. Will change

Copy link
Contributor

@marcosholgado marcosholgado left a comment

Choose a reason for hiding this comment

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

Looks good, just left a comment for better readability to use AppPixelName.EMAIL_TOOLTIP_DISMISSED.pixelName instead the string directly.

@aitorvs aitorvs merged commit c18c3a6 into develop Aug 5, 2021
@aitorvs aitorvs deleted the lhf/aitor/stop_sending_atb_in_some_pixels branch August 5, 2021 11: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.

2 participants