@sgiehl opened this Pull Request on March 28th 2018 Member

When flattening url action reports the default_action_name (default: index) is still contained in the labels.
This changes will remove that.

fixes #12651

@diosmosis commented on March 28th 2018 Member

Also, I think there's probably test coverage for flattened actions reports, but if not, would be good to cover this change in a test.

@sgiehl commented on March 29th 2018 Member

The default action name will still be displayed for non flattened reports. Changing it also won't change any old reports...

@mattab commented on April 24th 2018 Member

@sgiehl fyi tests result show some possible regression

@sgiehl commented on April 24th 2018 Member

I have seen that. I'm not yet sure how to solve that best without regressions... Need to investigate that further...

@sgiehl commented on April 27th 2018 Member

@mattab guess I now found a way how to always have a leading slash. But one thing needs to be clear here: Changing that might break some custom alerts defined by users. CustomAlerts plugin uses flatten reports for comparing actions. As the returned actions changes (leading slash and removed default action name) this might change results of defined alerts...

@mattab commented on April 30th 2018 Member

@sgiehl the tests results look good now :+1:

Feedback:

  • Since this is an API change, and possibly API break for some edge case like custom alerts, could you please add a mention in the CHANGELOG.md ?
@mattab commented on April 30th 2018 Member

@sgiehl i was expecting that the PR includes the change in global.ini.php: action_default_name = or action_default_name = / , why not?

@sgiehl commented on April 30th 2018 Member

@mattab As the title says it currently only changes flattened reports.
Changing action_default_name would change all reports. And it would only affect new data, all already tracked data and generated reports wouldn't change.

@mattab commented on April 30th 2018 Member

@sgiehl Makes sense. And what do you think about the idea of changing the setting to / or empty string, in a separate PR, to make it consistent with the flattened reports? Would there be any issue with this approach maybe?

@sgiehl commented on April 30th 2018 Member

We maybe should only do that for new installs as otherwise we might create inconsistency between old and new tracked data & reports.
Also setting it to / might be logically incorrect. We never store a leading slash for any action. So an empty string might be more correct...

@mattab commented on May 2nd 2018 Member

@sgiehl noticed there's an integration test failing in CustomAlerts, could you update the submodule so the test passes?

@sgiehl commented on May 2nd 2018 Member

Sure. Would do that directly before merging. Is everything else here good to merge?

@mattab commented on May 2nd 2018 Member

@sgiehl LGTM. I reckon we should completely remove the action_default_name feature since it's not useful IMO, but let's do it later and keep this PR this PR as is.

Created another issue: Remove the feature to set a custom action name "action_default_name" #12809

This Pull Request was closed on May 3rd 2018
Powered by GitHub Issue Mirror