New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Configuration option to disable transition periods #18366
Conversation
This makes it possible to limit the process lifetime to avoid excessive memory usage (possibly due to PHP bugs such as 79519).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some comments. Besides that, according to the original issue it would be good to hide the transitions row action when it's disabled for the current period.
Guess to achieve that, the simples solution would be to pass the config setting as a global javascript variable using the even Template.jsGlobalVariables
, and then you can check that variable in the row action method that returns it's availability:
matomo/plugins/Transitions/javascripts/transitions.js
Lines 125 to 137 in c83fe5a
isAvailableOnReport: function (dataTableParams) { | |
var i = 0; | |
for (i; i < DataTable_RowActions_Transitions.registeredReports.length; i++) { | |
var report = DataTable_RowActions_Transitions.registeredReports[i]; | |
if (report | |
&& report.isAvailableOnReport | |
&& report.isAvailableOnReport(dataTableParams)) { | |
return true; | |
} | |
} | |
return false; | |
}, |
@sgiehl Thanks, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a couple of comments.
plugins/Transitions/tests/System/TransitionsMaxAllowedPeriodTest.php
Outdated
Show resolved
Hide resolved
plugins/Transitions/tests/System/TransitionsMaxAllowedPeriodTest.php
Outdated
Show resolved
Hide resolved
plugins/Transitions/tests/System/TransitionsMaxAllowedPeriodTest.php
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looked through the code and tested everything locally. Works as expected 👍
You could add an additional UI test with the period that should be shown disabled, but that's a nice to have. Besides that, feel free to merge.
…ds' into m-18332-disable-transition-periods
Description:
Fixes #18332
Adds an option to specify the max allowed period for transitions, either globally or per site
Site level settings override the general settings.
All period smaller than the max period are allowed, so month will also allow week and day.
Ranges with more days than the max period will not be allowed, eg. an 8 day range isn't allowed if the
max_period_allowed = "week"
, but a 7 day range would be ok.Row transitions obey the same rules, but I haven't hidden the row transition action icon if the period is not allowed as it might be confusing to have to icon disappear entirely without any explanation. It's also a bit awkward having the datatable row action registration methods make an async API call each time to check period availability - open to suggestions on how to do this efficiently if we really want to go ahead and hide the row action 🙂
Review