Move JS snippets to separate script files

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.

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
testSettingsForm

All 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 ?

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.

I will mark this task as postponed until we have solved 📌 Deprecate sync tags Active .

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.

I changed the target version to 1.2.x-dev. No need to assign the ticket to yourself @anish.ir. It's enough that you comment that you are working on it.

Hey @heikkiy,

The current issue fork does not contain the 1.2.x branch. I tried updating the fork and creating a new branch, and received this error : Failed to create branch '3482342-move-snippets-js': invalid reference name '1.2.x'.

I pushed https://git.drupalcode.org/issue/piwik_pro-3482342/-/tree/1.2.x there now.

You can continue from that. Most likely it makes sense to start fresh and move the changes from the 1.x feature branch there because 1.2.x has a lot of new features.

anish.ir → changed the visibility of the branch 3482342-move-js-snippets to hidden.

Hey,

I have reverted the previous changes and added the mentioned functionality.

Changes made:

• New Setting: Added a boolean field to the module settings page, Load Piwik PRO snippet from a library, as per your suggestion. The default value is set to false, ensuring backward compatibility with existing implementations.
• Load Snippet from Library: Updated the module to check the new setting value. If the setting is TRUE, it will load the JS file from the external library. If FALSE, it will continue using the old method by injecting the Piwik PRO snippet directly into the DOM.
• Installation Hook: Added an installation hook to set the default value of the new setting (FALSE) for sites that already have the module installed.
• ESLint and PHPUnit Tests: The current ESLint and PHPUnit test failures are acknowledged and are still in progress. I am working on updating the tests to reflect the new script loading mechanism:
• When the setting is FALSE, the test will check if the Piwik PRO domain is present in the DOM.
• When the setting is TRUE, the test will validate if the new piwik_pro/piwik_pro_snippet library is properly loaded.

I am still working on finalizing the test validations. Please let me know if there are any other changes or adjustments that need to be made, and I will add them.

I am attaching the screen shot for the config form page.

Thanks for the guidance and let me know if you have any further feedback!

Excellent progress, thank you.

This looks really good. I think the pipeline linter and test validation updates are the only thing before we can move this to Needs review.

Thank you, @heikkiy.

The PHPCS and ESLint errors have been resolved as well. I am currently working on finalizing the PHPUnit test cases.

Hey,

I think the mentioned changes have been made, the tests are also running fine now.
Please have a look.
Thanks!

Hi @anish.ir and thank you for your contributions with this!

I tested your changes and noticed that the new library mode doesn't take into account the excluded pages and other exclusion settings when it's enabled.

So I mean that the method isVisible() should be used somewhere in the code, for example like this:

In the function:

function piwik_pro_page_top(array &$page) {

You could use isVisible() for example in here

if ($load_from_library && \Drupal::service('piwik_pro.snippet')->isVisible()) {

Normally it's used when getSnippet() is called, so when that gets skipped with the new library mode also isVisible() will be skipped.

Can you look into this a bit more? Thanks!

In addition to this, I was thinking would there be a better way to describe this setting in the admin UI? Some users might think that "external library" is something that is loaded from outside Drupal, like from another server entirely. Maybe the description text could be changed into "Check this to load the Piwik PRO snippet from a JavaScript library shipped with the module. "
What do you think? Any better ideas for the description text?

Changed status to needs work.

Hi,

Thanks for the feedback! I’ve made the following updates based on your suggestions:

• Ensured isVisible() is checked before adding the library in piwik_pro_page_top(), so the new script loading method respects exclusion settings.
• Improved the admin UI description to avoid confusion about "external libraries."

I've also cleared the cache and tested the changes, ensuring they work as expected. Let me know if there's anything else you'd like to adjust!

Thanks again!

Thank you. I am still hesitant to approve this because all the unit tests were passing here https://git.drupalcode.org/issue/piwik_pro-3482342/-/jobs/4235376 even though the isVisible() check was not there. So something was wrong with the unit tests that they didn't catch the problem.

Thanks @anish.ir!

Otherwise it looks good but I'm with @heikkiy here that there should be some new tests for this new setting, maybe?

I think the current tests assume the new setting "piwik_pro_load_from_library" is NULL and that's why isVisible() doesn't have any effect.

I would suggest adding maybe a new testing function to PiwikProSnippetTest.php
I'd say maybe one new testing function would be enough to test if the script configs get added to source when piwik_pro_load_from_library is set to TRUE.
Basically even the same kind of assertion should work when using the library mode, because the piwik_domain should exist in the html source code within the drupal-settings-json

    $this->assertSession()->responseContains((string) $this->config->get('piwik_domain'));

"piwik_pro":{"piwik_domain":"https:\/\/yourname.containers.piwik.pro/\/"

Hi,

I've added the test case for the Piwik PRO snippet when loaded via the library. The test ensures that:

• The configuration is set correctly.
• The snippet is loaded from the library.
• The expected piwik_domain appears in the drupal-settings-json script.

Please review the changes and let me know if any modifications are needed. Thanks for your guidance!

Marking as RTBC. Thanks!

Merged to 1.2.x now and I resolved all threads from GitLab.

Hey,

Since the MR have been merged and the changes have already been verified and reviewed, I think we may transition this to Fixed. Please let me know If I am wrong here.
Thanks !

Indeed. Was already supposed to do that in the previous comment.

Moved new features to 1.3.x-dev and kept 1.2.x. as the supported version with only bug fixes.

hartsak → closed merge request !8

Automatically closed - issue fixed for 2 weeks with no activity.