Detect GA3, GA4 or GTM during installation and suggest migration guides #19957
Conversation
…guration guide link
…mail these instuctions' button, add consent manager link to instructions email, add link to the privacy 'asking for consent' page
…ect-consent-manager
…or site content test data, added UI tests for consent manager detection
…ect-consent-manager
… instructions email
…dded new fixture to installation UI test to prevent site content detection request
|
The installation test was failing because the site content detection values required by the instructions email and tracking code templates were not being populated for fresh installs. I've updated the installation controller to do a site content detection request as part of the install wizard and provide the required values. I've also added a new fixture which will mock the site content detector finding nothing on the site and added this to the installation UI test so that the test doesn't make live requests to the test site. |
|
@bx80 installation works now, once the tests are passed, will do another review :) |
There was a problem hiding this comment.
Reviewed functionality and code, working as expected.
The only I am wondering about in the SiteContentDetector class, there is a set of test data for the fixture, which works for me, not sure if there is a different approach.
- 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
|
@peterhashair If you mean the data in the |
|
@bx80 sorry I mean, those functions |
|
@peterhashair The site content detector gets called as part of the UI tests and while it's possible to mock class methods using PHPUnit for unit tests, I haven't seen a good way to do this with UI tests which will interact with the class method by way of an API call. The nearest existing pattern I found in the codebase was the location provider where a mock location provider class is set as the default provider via an option setting. The approach here is broadly similar, where the test fixture can set test data as an option and the class will check for it and return the test data values if they exist. Not the most elegant solution, but it works and is flexible. I'm open to better ideas though, @sgiehl Do you have a recommended approach for this scenario? |
I guess we have this solved in various ways. I haven't had a look at the detailed solution here. But I guess I would have tried to somehow solve it using DI. So the class responsible for the detection is loaded using DI. And for ui tests, you could then overwrite the used class with some sort of test class, that returns fake response. |
|
Thanks @sgiehl, I've reworked the UI tests to use DI 👍 |
| {% endif %} | ||
|
|
||
| {% if ga4Used %} | ||
| {{ 'SitesManager_GA4DetectedEmail'|translate('Google Analytics 4', 'GA', 'https://matomo.org/faq/how-to/migrate-from-google-analytics-4-to-matomo/')|raw }} |
There was a problem hiding this comment.
@bx80 @sgiehl I cannot seem to find the translation for GA4DetectedEmail and GTMDetectedEmail in the translation file is this a bug ?
There was a problem hiding this comment.
Its a bug, have fixed it in my upcoming Pr for cloudflare
Description:
Checks for GA3, GA4 or GTM usage on a site when showing the tracking code / getting started screen and if present, will show a link to the appropriate guide on how to migrate to Matomo.
Ref: DEV-3005
Review