@bx80 opened this Pull Request on August 14th 2021 Contributor

Description:

Fixes #11627

Adds a new option to the JavaScript tracker to exclude query parameters from the tracked URL.

_paq.push(["setExcludedQueryParams", ["uid", "sid"]]);

Existing purify() and removeUrlParameter() functions were reused.

The tracker code generator has been updated to automatically add this option to the generated javascript tracking code using the excluded query parameters configured under Websites > Settings.

Tracker code generator unit tests have also been updated to test for valid tracker code and string escaping with the option enabled.

(Initially used "setIgnoreUrlParams" as the option name as per issue report, but though it would be more consistent to use "setExcludedQueryParams" to match the existing config option and UI terminology.)

Review

  • [ ] Functional review done
  • [ ] Potential edge cases thought about (behavior of the code with strange input, with strange internal state or possible interactions with other Matomo subsystems)
  • [ ] Usability review done (is anything maybe unclear or think about anything that would cause people to reach out to support)
  • [ ] Security review done see checklist
  • [ ] Code review done
  • [X] Tests were added if useful/possible
  • [ ] Reviewed for breaking changes
  • [ ] Developer changelog updated if needed
  • [ ] Documentation added if needed
  • [ ] Existing documentation updated if needed
@tsteur commented on August 15th 2021 Member

Thanks for this @bx80 👍 It's not been reviewed yet but noticed that this PR may include changes from your other PR?

@bx80 commented on August 16th 2021 Contributor

Sorry about that, I unintentionally created a sub-branch of the other PR. I've now rebased the branch so it only has the relevant commits.

@github-actions[bot] commented on September 3rd 2021 Contributor

This issue is in "needs review" but there has been no activity for 7 days. ping @matomo-org/core-reviewers

@justinvelluppillai commented on September 5th 2021 Contributor

build js

@sgiehl commented on September 6th 2021 Member

@justinvelluppillai building js doesn't work for pull requests coming from forks. I guess in theory it would be possible to enable that for our action (if the issuer allows edits by us), but not sure how much effort it would be.

Did you test the changes locally. I had only done a code review yet. If you can confirm that it works as expected, I guess it could be merged (once the build assets are updated)

btw. I have also added another missing JS test.

@justinvelluppillai commented on September 6th 2021 Contributor

@sgiehl yeah I realised that after I attempted it hence my laughing emoji at my comment! How do we normally add the built assets, build them locally and commit to the PR?

@sgiehl commented on September 6th 2021 Member

To do that, guess you would need to check out the fork, or at least add it as remote. Alternatively you could merge the PR into a new branch in our repo. Build the assets there and recreate the PR from that branch.

Btw. Feel free to create a new issue to adjust our action to also build assets for forks when possible

@github-actions[bot] commented on September 14th 2021 Contributor

This issue is in "needs review" but there has been no activity for 7 days. ping @matomo-org/core-reviewers

@bx80 commented on September 20th 2021 Contributor

Closed due to git branch merge issues and replaced by PR #18031

This Pull Request was closed on September 20th 2021
Powered by GitHub Issue Mirror