Add role restrictions

This is a very good idea. We need to think about a little how to implement this. It could be a simple setting in the module page that would allow you to select which roles can see the embedded scripts but that would be mostly custom code.

There are also other contrib solutions like https://www.drupal.org/project/eca → that allows conditions but it seems a bit heavy sided for this functionality.

Other ideas and ways to implement this are welcomed.

This patch contains a snapshot of the current MR

I've discussed this with rhezios, a collegae of mine. We think that ECA is a bit overkill for a simple feature like this. We could possibly look into a plugin-based solution so it could be used for more situations.

I did a functional test and it works as aspected.

Thanks for the MR. This looks very good to us and was about the same functionality I was also thinking.

The ECA idea same mostly so that it would be possible to add a more powerful rules engine to the module but agreed that it is a bit overkill.

This does look like a new feature so I think it would justify a 1.1.0 release. For that it would make sense to create a 1.1.x branch and make that as the default branch and target branch for this MR.

I will try to handle that as soon as possible.

But in the mean while the module also includes tests that should be passing: https://git.drupalcode.org/project/piwik_pro/-/blob/1.0.x/tests/src/Func.... Currently the pipeline is failing: https://git.drupalcode.org/issue/piwik_pro-3480154/-/pipelines/315963/te....

The tests should be updated and we should also add information about role visibility to the project readme: https://git.drupalcode.org/project/piwik_pro/-/blob/1.0.x/README.txt. A bit out of scope but that file should be also renamed as readme.md.

Marking as Needs work still because of the failing tests and readme changes.

I've updated the issue summary, fixed the phpcs warnings and updated the readme. I'll try to write some test later this week. I've also created 📌 Replace README.txt to Readme.md file Active , I'll work on that later this week.

Excellent, thanks.

A couple of things came to my mind related to testing
1. What happens if the system roles change? New roles might not cause issues but what happens if a role is removed?
2. Do we need to take caches into account? Does the current implementation take those into account if a user with a role to see the snippet first visits the page and then right after a second user without the same role visits the page. Currently the code does not seem to implement any cache contexts when loading the snippet. Not sure if this can be handled with unit tests so any ideas are welcome.

I've added some tests. It's actually the first time that I've written tests for a module, so let me know if somethings needs to be handled differently.

heikkiy → credited dimas11 → .

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