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

Remove parameterName and parameterValue from ad-attribution feature to simplify code #1343

Merged
merged 1 commit into from
Sep 20, 2022

Conversation

jonathanKingston
Copy link
Collaborator

@jonathanKingston jonathanKingston commented Aug 17, 2022

Reviewer: @englehardt

Description:

Marking draft as CI should fail if working correcly; testCases still need amending.

This is to simplify the parsing logic as we already verify an ad click based on the adDomainParameterName parameter name in the link click.

https://app.asana.com/0/0/1202808108652811/f

Corresponding config change should probably land first so we can just run bundle-config instead of hand hacking it: duckduckgo/privacy-configuration#345

Steps to test this PR:

Automated tests:

  • Unit tests
  • Integration tests
Reviewer Checklist:
  • Ensure the PR solves the problem
  • Review every line of code
  • Ensure the PR does no harm by testing the changes thoroughly
  • Get help if you're uncomfortable with any of the above!
  • Determine if there are any quick wins that improve the implementation
PR Author Checklist:
  • Get advice or leverage existing code
  • Agree on technical approach with reviewer (if the changes are nuanced)
  • Ensure that there is a testing strategy (and documented non-automated tests)
  • Ensure there is a documented monitoring strategy (if necessary)
  • Consider systems implications

@englehardt englehardt marked this pull request as ready for review September 19, 2022 21:25
@englehardt
Copy link
Contributor

englehardt commented Sep 19, 2022

@jonathanKingston let's rebase this on the newest develop and on #1410, which fixes one regression I noticed when testing this.

I also saw another regression present in develop related to bat.js, though it isn't caused by these changes. It looks like some changes to the UI have caused the exempt domain warning to no longer show up. Here's what I see when an exemption is active:

Screen Shot 2022-09-19 at 3 33 11 PM

Here's what it looks like when inactive (which is correct):

Screen Shot 2022-09-19 at 3 33 47 PM

So the UI partially detecting the change as the description is correct, but the exemption isn't listed and the icon is incorrect.

Copy link
Contributor

@englehardt englehardt left a comment

Choose a reason for hiding this comment

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

The UI issues I pointed out were related to an outdated local instance of privacy grade. I can no longer repro after updating.

Just retested and it LGTM.

@jonathanKingston jonathanKingston merged commit e3fd8a1 into develop Sep 20, 2022
@jonathanKingston jonathanKingston deleted the jkt/remove-parameter-name branch September 20, 2022 23:00
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

2 participants