Removes default action name for flattened action urls #12669

sgiehl · 2018-03-28T18:29:05Z

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

Left a couple questions.

Also @mattab, @sgiehl's question on the original issue is unanswered. Can you give a 👍 if removing the index from all flattened reports is ok?

diosmosis · 2018-03-28T19:04:10Z

plugins/Actions/DataTable/Filter/Actions.php

+                        $label = substr($label, 0, -strlen($defaultActionName));
+                        if (substr($label, -1) == '/') {
+                            $label = rtrim($label, '/') . '/';
+                        }


I think you can just have $label = rtrim($label, '/') . '/'; here instead of the if(), right? It should have the same effect even if the string doesn't end w/ '/'.

Also does this code need to take into account the [General] action_url_category_delimiter option? I don't know what the use case would be for it not to be /, but you never know I guess.

using the config should make sense

diosmosis · 2018-03-28T19:12:49Z

plugins/Actions/DataTable/Filter/Actions.php

+                // remove the default action name 'index' in the end of flattened urls
+                if (Common::getRequestVar('flat', 0)) {
+                    $label = $row->getColumn('label');
+                    if (substr($label, -strlen($defaultActionName)) == $defaultActionName) {


If a row has a label like /path/to/filenamewithindex, will this still work?

hm... will change it so that won't match

diosmosis · 2018-03-28T19:21:41Z

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.

mattab

Currently in global.ini.php we have action_default_name = index

So maybe the solution could be to make action_default_name = in global.ini.php so that this setting still does what it used to ?

mattab · 2018-03-28T21:33:00Z

...HPUnit/System/expected/test_BackwardsCompatibility1XTest_flat__Actions.getPageUrls_range.xml

@@ -1,7 +1,7 @@
 <?xml version="1.0" encoding="utf-8" ?>
 <result>
 	<row>
-		<label>index</label>
+		<label></label>


here it's expected to be / ie. the root URL

In flattened url reports currently all leading slashes are removed. Should all other labels have leading slashes as well? Otherwise it would be inconsistent

👍 sounds good that all pages have leading slashes

sgiehl · 2018-03-29T07:37:32Z

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

mattab · 2018-04-24T01:42:55Z

@sgiehl fyi tests result show some possible regression

sgiehl · 2018-04-24T07:42:46Z

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

sgiehl · 2018-04-27T16:32:03Z

@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 · 2018-04-30T06:03:23Z

@sgiehl the tests results look good now 👍

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 · 2018-04-30T06:08:33Z

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

sgiehl · 2018-04-30T06:16:22Z

@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 · 2018-04-30T08:31:12Z

@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 · 2018-04-30T08:35:25Z

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 · 2018-05-02T08:17:07Z

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

sgiehl · 2018-05-02T08:38:10Z

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

mattab · 2018-05-02T10:07:12Z

@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

* remove default action name for flattened action urls * improve code * update system tests * always prepend action delimiter to flattened action urls * update ui files * update test files * updates CHANGELOG * submodule update

sgiehl 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 Mar 28, 2018

sgiehl added this to the 3.4.1 milestone Mar 28, 2018

sgiehl force-pushed the flattenactions branch from 2b319e7 to 1f9624d Compare March 28, 2018 18:31

diosmosis requested changes Mar 28, 2018

View reviewed changes

sgiehl force-pushed the flattenactions branch from ccf0c45 to 03a15f5 Compare March 28, 2018 20:17

mattab requested changes Mar 28, 2018

View reviewed changes

sgiehl force-pushed the flattenactions branch 2 times, most recently from 67d1a0d to 6c50548 Compare April 1, 2018 16:39

sgiehl force-pushed the flattenactions branch 3 times, most recently from c4561c5 to ccf0124 Compare April 23, 2018 16:28

sgiehl self-assigned this Apr 24, 2018

sgiehl added Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. and removed Needs Review PRs that need a code review labels Apr 24, 2018

sgiehl force-pushed the flattenactions branch 4 times, most recently from d79717b to edd92cb Compare April 27, 2018 15:47

sgiehl force-pushed the flattenactions branch 3 times, most recently from e6fcbc2 to 9bcbb5e Compare April 27, 2018 19:57

sgiehl added Needs Review PRs that need a code review and removed Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. labels Apr 27, 2018

sgiehl removed their assignment Apr 28, 2018

mattab mentioned this pull request May 2, 2018

Remove the feature to set a custom action name "action_default_name" #12809

Open

mattab approved these changes May 2, 2018

View reviewed changes

sgiehl added a commit to matomo-org/plugin-CustomAlerts that referenced this pull request May 3, 2018

update tests for matomo-org/matomo#12669

455c9e0

sgiehl added 8 commits May 3, 2018 22:13

remove default action name for flattened action urls

71c4bc0

improve code

9bf4636

update system tests

744eddb

always prepend action delimiter to flattened action urls

c4fc95e

update ui files

53e5509

update test files

3fae81b

updates CHANGELOG

7dec0e8

submodule update

adfc0de

sgiehl force-pushed the flattenactions branch from 68350b0 to adfc0de Compare May 3, 2018 20:13

sgiehl merged commit 83ae917 into 3.x-dev May 3, 2018

sgiehl deleted the flattenactions branch May 3, 2018 21:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removes default action name for flattened action urls #12669

Removes default action name for flattened action urls #12669

sgiehl commented Mar 28, 2018

diosmosis left a comment

diosmosis Mar 28, 2018

diosmosis Mar 28, 2018

sgiehl Mar 28, 2018

diosmosis Mar 28, 2018

sgiehl Mar 28, 2018

diosmosis commented Mar 28, 2018

mattab left a comment •

edited

mattab Mar 28, 2018

sgiehl Mar 29, 2018

mattab Apr 23, 2018

sgiehl commented Mar 29, 2018

mattab commented Apr 24, 2018

sgiehl commented Apr 24, 2018

sgiehl commented Apr 27, 2018

mattab commented Apr 30, 2018

mattab commented Apr 30, 2018

sgiehl commented Apr 30, 2018

mattab commented Apr 30, 2018

sgiehl commented Apr 30, 2018

mattab commented May 2, 2018

sgiehl commented May 2, 2018

mattab commented May 2, 2018

Removes default action name for flattened action urls #12669

Removes default action name for flattened action urls #12669

Conversation

sgiehl commented Mar 28, 2018

diosmosis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diosmosis commented Mar 28, 2018

mattab left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sgiehl commented Mar 29, 2018

mattab commented Apr 24, 2018

sgiehl commented Apr 24, 2018

sgiehl commented Apr 27, 2018

mattab commented Apr 30, 2018

mattab commented Apr 30, 2018

sgiehl commented Apr 30, 2018

mattab commented Apr 30, 2018

sgiehl commented Apr 30, 2018

mattab commented May 2, 2018

sgiehl commented May 2, 2018

mattab commented May 2, 2018

mattab left a comment •

edited