JavaScript tracker exclude query parameters #17887
Conversation
|
Thanks for this @bx80 👍 It's not been reviewed yet but noticed that this PR may include changes from your other PR? |
bx80
force-pushed
the
11627-tracker-ignore-url-params
branch
from
40ddd4b
to
0f4ca5e
Compare
|
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. |
There was a problem hiding this comment.
It would be awesome to also have some javascript tests for that. They could be added after this:
matomo/tests/javascript/index.php
Line 2510 in da4df21
Not sure how familiar you are with those tests, but it could be something like:
tracker.setExcludedQueryParams(['sid', 'test']);
equal( tracker.hook.test._purify('http://example.com/?sid=12345&test5=1'), 'http://example.com/?test5=1', 'http://example.com/?sid=12345&test5=1');
equal( tracker.hook.test._purify('http://example.com/?asid=12345&test=1'), 'http://example.com/?asid=12345', 'http://example.com/?asid=12345&test=1');
equal( tracker.hook.test._purify('http://example.com/?sid=test#hash'), 'http://example.com/', 'http://example.com/?sid=test#hash');
|
This issue is in "needs review" but there has been no activity for 7 days. ping @matomo-org/core-reviewers |
github-actions
bot
added
the
Stale
|
build js |
github-actions
bot
removed
the
Stale
|
@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. |
|
@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? |
|
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 |
|
This issue is in "needs review" but there has been no activity for 7 days. ping @matomo-org/core-reviewers |
github-actions
bot
added
the
Stale
|
Closed due to git branch merge issues and replaced by PR #18031 |
Description:
Fixes #11627
Adds a new option to the JavaScript tracker to exclude query parameters from the tracked URL.
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