Skip to content
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

Merged
merged 20 commits into from Dec 1, 2021

Conversation

bx80
Copy link
Contributor

@bx80 bx80 commented Nov 23, 2021

Description:

Fixes #18332

Adds an option to specify the max allowed period for transitions, either globally or per site

[Transitions]
max_period_allowed = "month"
[Transitions_1]
max_period_allowed = "week"

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

@bx80 bx80 added not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. Needs Review PRs that need a code review labels Nov 23, 2021
@bx80 bx80 added this to the 4.7.0 milestone Nov 23, 2021
@bx80 bx80 self-assigned this Nov 23, 2021
Copy link
Member

@sgiehl sgiehl left a 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:

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;
},

plugins/Transitions/API.php Outdated Show resolved Hide resolved
plugins/Transitions/API.php Outdated Show resolved Hide resolved
@sgiehl sgiehl removed the Needs Review PRs that need a code review label Nov 23, 2021
@bx80
Copy link
Contributor Author

bx80 commented Nov 24, 2021

@sgiehl Thanks, Template.jsGlobalVariables was the pointer I needed. I've added code to disable the transitions row action if the period isn't allowed. Also added a system test to make sure the config setting results in the correct exception.

plugins/Transitions/Transitions.php Outdated Show resolved Hide resolved
@bx80 bx80 added the Needs Review PRs that need a code review label Nov 26, 2021
Copy link
Member

@sgiehl sgiehl left a 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/CoreHome/vue/src/Periods/Range.ts Outdated Show resolved Hide resolved
core/Period/Range.php Outdated Show resolved Hide resolved
plugins/Transitions/API.php Outdated Show resolved Hide resolved
plugins/Transitions/API.php Outdated Show resolved Hide resolved
plugins/Transitions/API.php Outdated Show resolved Hide resolved
Copy link
Member

@sgiehl sgiehl left a 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.

@bx80 bx80 merged commit 74a6bb0 into 4.x-dev Dec 1, 2021
@bx80 bx80 deleted the m-18332-disable-transition-periods branch December 1, 2021 02:27
@bx80 bx80 mentioned this pull request Dec 1, 2021
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add new feature to disable Transitions for specific periods
4 participants