- Issue created by @heikkiy
- First commit to issue fork.
- Merge request !8Issue-piwik_pro-3482342: Moved JS snippets to separate script files. → (Open) created by anish.ir
Hey @HeikkiY
I solved all the eslint and phpcs errors, they are all green now.
The only pipeline thats failing now is phpunit, and it is giving errors which I think, are not related to the JS files which I have added.
Some of the errors of strings not found, like
"The string "https://yourname.containers.piwik.pro/" was not found anywhere in the HTML response of the current page."
Which, I think is to be provided in the module configurations, please take a look into this.
- 🇫🇮Finland heikkiy Oulu
This change is so radical from the unit test perspective so we need to think about the unit tests a bit.
Basically the unit tests are now expecting a lot of the JS snippets are in the DOM and it's trying to find the string from the it and test that everything gets injected correctly.
You can find the current php unit tests from https://git.drupalcode.org/project/piwik_pro/-/blob/1.1.x/tests/src/Func... and I think we need to think about how they should be updated to support the external JS files.
The failing tests are
testPiwikProConfiguration
testSnippetVisibility
testSnippetVisibilityPagesInverted
testSnippetVisibilitySyncSnippet
testSnippetVisibilityChangedDataLayer
testSettingsFormAll the other errors seem to be related to the JS snippet not being visible in the DOM but there is one error that seems to indicate a larger problem:
7) Drupal\Tests\piwik_pro\Functional\PiwikProSnippetTest::testSettingsForm Behat\Mink\Exception\ExpectationException: Current response status code is 500, but 200 expected.
It seems like the module settings page is now giving an error 500.
I will add the needs tests tag for now. These should be possible to also run the unit tests in local for easier debugging with https://github.com/ddev/ddev-drupal-contrib.
Hey @ heikkiy → ,
So should I proceed with making changes in the test file "PiwikProSnippetTest.php", as there are many errors stating that "String not found" which could be possibly due to the external Js files are not being supported.
Also is this issue ✨ Support to load the script using an external file also Active related to the same we are discussing ?
- 🇫🇮Finland heikkiy Oulu
Issue |#3354840] is closed and doesn't need work. We should progress with 📌 Deprecate sync tags Active first because that will remove a bunch of code from the module and that will make this task a lot easier to manage.
And yes, after #3486733 is done we need to update the tests to pass again.
Related to the ESLint warnings earlier. The script is coming from Piwik PRO directly. I think we should keep the changes to those files to minimum if Piwik PRO in the future makes changes to the code it will be easier to diff the changes. Because of this I would put the original JS code format back with the linter errors and put the files into .eslintignore file. I would keep them formatted as they were earlier but just replace the Piwik domain etc.
- 🇫🇮Finland heikkiy Oulu
I will mark this task as postponed until we have solved 📌 Deprecate sync tags Active .
- 🇫🇮Finland heikkiy Oulu
This task can be now continued after 📌 Deprecate sync tags Active is merged and 1.1.1 released. It should make this task easier because all the sync tag code was removed.