Add event to alter delete website confirmation #22484
Conversation
michalkleiner
changed the title
There was a problem hiding this comment.
Hey @AltamashShaikh,
I've discussed the solution again in core team. As mentioned before the "perfect" solution would be using a javascript event, that plugins like the TagManager could easily use to add content to the confirmation. But as we currently do not have something like a global javascript event registry it is not really possible to solve it that way.
Such a javascript event registry would be beneficial for a lot other use cases as well, but core team would need to plan and work on that separately.
To be able to allow altering the removal confirmation already we might indeed need to go with a similar workaround like you implemented. But from our point of view the implementation - especially in TagManager - is too unflexible.
You are currently fully replacing the text of the confirmation. That for sure works, but if another plugin would aim to do the same the result is completely unpredictable. The plugin, that would be loaded last, would most likely "win the race" and place it's content, while the content of other plugins is simply overwritten.
When introducing new events you should always consider that other plugins might want to use those events as well.
My approach here would have been to have an event that only provides sort of a list of things to warn that the will be removed as well and keep a more general basic message as introduction.
So there would be an event like getThingsToWarnOnSiteRemoval, which receives the idSite and a variable, prefilled with an empty array. TagManager can then add the items that will be removed to that array. If the list contains items after calling the event, core would adjust the message to add them accordingly.
@sgiehl I changed the event to pass an array with a default message that as the first key and plugins can add into the array of update the first key as required, will that work for you guys ?? |
tests/UI/expected-screenshots/UIIntegrationTest_api_listing.png
Outdated
Show resolved
Hide resolved
|
@sgiehl I have updated the code as per your suggestion if all okay please don't merge as I need to get the text checked from product once to ensure if we need to change anything in core. |
|
@sgiehl All good from product team to merge this 👍 |
…omo-org/matomo into PG-3574-delete-website-message-event
plugins/SitesManager/API.php
Outdated
| { | ||
| $messages = []; | ||
| Piwik::checkUserHasSuperUserAccess($idSite); | ||
| Piwik::postEvent('SitesManager.getMessagesToWarnOnSiteRemoval', [$idSite, &$messages]); |
There was a problem hiding this comment.
We should add a comment above the event to document what it is good for. Like it'S e.g. the case here:
matomo/core/Scheduler/Scheduler.php
Lines 157 to 166 in 9a3ef94
And when looking at other events it seems we use to pass the referenced parameter first, and then the other. So you may switch idSite and messages
There was a problem hiding this comment.
@sgiehl Added as suggested 👍
sgiehl
changed the title
sgiehl
added
the
Enhancement
Description:
When a website is about to be deleted Matomo currently shows a confirmation.
As users might not be aware which additional plugin data might be removed with that, plugins should have the possibility to add some additional information/warning to that confirmation.
This PR introduces a new event
SitesManager.getMessagesToWarnOnSiteRemoval, which allows plugins to provide additional text blocks that should be displayed within the modal.Fixes: #PG-3574
Review