Add a new field custom URL Referrer on pageview tag #566
Conversation
Being able to set a custom Url Referrer for pageviews is handy when you are working with SPA tracking. It can also be handy of referrers contain PII data to be able to clan these values before they are posted to Matomo.
Adding new setting for urlRef + tests + en translation
tomper00
changed the title
There was a problem hiding this comment.
Thank you for taking the time to create this PR, @tomper00 . I think it looks really good. The only request I have is that you create a migration similar to the one you created in your other PR. I guess another option would be to wait until your other PR is merged, merge those changes into this branch, and then simply add a single $migrator->addField('customUrlRef'); line to the migration your created as part of your other PR.
Oh. One other side note. Please remember to add the Needs Review label to any PRs that you create in the future. It just makes it more visible to our team and lets us know that it's ready for us to provide feedback.
|
Ok thanks again for the feedback @snake14 I will go with the option to merge my branches, that sounds like an easy way forward. |
@tomper00 I just left a comment on your other PR, but I realised that I had you use the wrong class for the migration. Since this one is for a tag, you would use the So, if you merge the two branches, you would run |
Translation updates based on comments from @AltamashShaikh Co-authored-by: Altamash Shaikh <altu9594@gmail.com>
Update test to match new translation
Updates/4.12.4-b2.php
Outdated
| $migrator = new NewTagParameterMigrator(MatomoTag::ID, 'customUrlRef'); | ||
| $migrator->migrate(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Since you merged the changes from your other branch into this one, you don't have to create a new migration script. You should be able to add the two lines from the doUpdate method in this one to the end of the doUpdate in the existing migration script. That way both your variable and tag changes get migrated in the same script. I guess it doesn't hurt to have two separate scripts, but it makes the release process a little more complicated.
| /** | ||
| * Update for version 4.12.4-b1. | ||
| */ | ||
| class Updates_4_12_4_b3 extends PiwikUpdates |
There was a problem hiding this comment.
I believe that this will error out if the class name doesn't match the file name. Could you please update it?
There was a problem hiding this comment.
@tomper00 would you please be able to update the class name to match the file name?
Description:
This is often needed when you work with SPA tracking since the Referrer often is not updated on virtual pageviews.
It is also nice to be able to create a custom JS Variable some times to be able to filter out PII etc.
Review