tag-manager
tag-manager copied to clipboard
matomo-org
Improvements to make MTM and _paq work better together
Description:
We're trying to make the MTM tracking code changes from a few months ago work. This re-applies them, making some improvements and adjusting for recent changes. Re-applying PRs 594 and 599.
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
- [ ] Wording review done
- [ ] Code review done
- [ ] Tests were added if useful/possible
- [ ] Reviewed for breaking changes
- [ ] Developer changelog updated if needed
- [ ] Documentation added if needed
- [ ] Existing documentation updated if needed
@snake14 I have tested this changes and looks good 🎉
I just noticed 1 thing is that the order of goals was not consistent
Thank you @AltamashShaikh I think that we're probably alright. This was the case even when I tested with the plain JS tracker by itself. I loaded the page multiple times and the goal conversion order changed each time.
@tsteur Can you please do an additional review as it created data loss issues last time when we implemented this
@tsteur Would you be able to review this? If not, we can merge it after the holidays and have QA test it.
@AltamashShaikh @snake14 Is there a way to see the diff in comparison to the previous solution? And do we have somewhere the information which use case it broke last time? It'll be hard for me to test all the different possible scenarios as it would take quite a long time. I wonder if it otherwise makes more sense to get help from support or so.
@tsteur I don't think that there's a clean diff, but here's the PR that reverted the original changes. A diff wouldn't be terribly helpful as there have been a fair number of new configs added to plugins/TagManager/Template/Tag/MatomoTag.web.js since the original ticket. The main issue that I found and fixed was only visible when there were a good number of _paq.push calls for configs mixed in with other tracking requests so that at least one tracking request was dropped. It stemmed from me trying to remove the configs from the array and then processing the remaining requests. I changed it to leave the configs in the array, but simply keep track of their indexes and so that we could skip them while processing the tracking requests. I created a few example HTML files as part of this PR to help show some ways to test and confirm the issue has been resolved.
We were planning on doing QA/support testing after your review, but we can move forward to that if you prefer.
@snake14 it be great if others tested first and if needed I could check last 👍
@achakko Do you have an environment where you can test this without me merging it into 5.x-dev?
@matomo-org/core-team I received the OK from Product to merge this PR. I merged in the most recent changes from 5.x-dev and everything looks good. Can we please tag this change for the Matomo 5.1.0 release?