Can you add some tests for this? And ideally if possible also contain the code in one module or class. It looks otherwise bit fragile/hacked.
I've also tried to use it and saw eg
Not sure if it is normal to see a dash before others in Acquisition => Websites report? (maybe isn't related to this PR)
Also my understanding was we would not only always show it in two columns but maybe have an option or two different "flattening" features whether you want it grouped or not? Personally in most cases I wouldn't like this view and prefer it in one column.
changed the implementation. There's now an additional option as soon as flattening is active.
Tested locally and noticed some oddities w/ the UI, for example the keywords report:
the first column is left aligned w/ a lot of space, and the second column is right aligned. Happens in other reports as well.
Otherwise, works well and code looks good to merge.
@diosmosis I've seen the UI glitch as well. Tried to fix it, but actually it seems to be a lot work to change the code so there can be more than one
@tsteur I already changed the code, so by default the columns are shown as one (like it is at the moment), but there is another table option to split the rows
Good start! Here is my feedback:
Aggregate rows are hidden -> Show them Dimensions are shown combined -> Show dimensions separately The report is flat -> Make it hierarchical (default) The report is not showing the totals row -> Show totals row
Could we change the order
The report is flat -> Make it hierarchical (default) Dimensions are shown combined -> Show dimensions separately Aggregate rows are hidden -> Show them The report is not showing the totals row -> Show totals row
Fixed a couple of things.
When the report is hierarchical, below "Make it flat", add the new option "Dimensions are combined -> Show dimensions separately" so that the feature is more accessible and from both views, not just flattened.
That won't be possible with the current implementation. I'm adding an additional metadata
dimensions when flattening the datatable. It's possible to determine if the subtables have different dimensions at this point. In normal view we would need to load at least one subtable to check if showing the option makes sense or not, also we would need to add the dimensions metadata to all datatables (even unflattened), which is otherwise useless.
The open segmented visitor log isn't currently visible in the view like it is in the "hierarchical" view, but expected to see it.
That is not the case in any flattened report, so we should move that in a new issue if needed
Row evolution icon would ideally appear in each column and have a different behavior. When the 1st column row evolution is clicked, it opens row evolution for this clicked label (without the sub-table/second dimension). When the second dimension's column row evolution is clicked, then row evolution shows the evolution for this particular row (Same behavior as "This report is hierarchical" view)
Not sure if that is worth the effort. Row actions currently only work for rows (but are placed in the first column). Making that available for multiple columns might be a bit more work, as we would need to change the whole stuff. That might also break custom row actions of other plugins.
If that's something we really need, I would suggest to put that in a new issue as well, as everything also works without that.
@mattab could you have a look at my comments and the changes I've made. I think it should look much better now.
Thanks for your answers, all makes sense.
- Othersin the second column. Expected
Othersand on hover
Others (learn more)with the link to FAQ (same behavior as when
Othersis shown in reports):
When flattening the data and showing dimensions separately, we get the columns (Provider, Provider, City):
Expected instead (Country, City, Provider).
Country - City - Providerbut Got instead label as
When exporting the flattened data and dimensions are shown separately, in the export data popover we can see the "Flatten report". Expected also to see an option "Show dimensions separately" pre-selected (because i had dimensions shown separately in my report). Got no such option. So it's not possible to export the data with the dimensions separate, but it is valuable feature we need. refs #12163 #12199
The dimensions are included in the export as additional dimensions as defined in #12163 #12199. So when the report is flat, the label will contain the concatenated dimensions, but there are also values for each dimensions separately. Not sure what we should improve here?
@mattab maybe you could have another look. All your points should now be fixed/adjusted, except of the one I've commented above.
Also need to write some more (ui) tests for that, so some more edge cases are tested as well. Will try to add them as soon as I have some time for it.
The dimensions are included in the export as additional dimensions as defined in <a href='/12163'>#12163</a> <a href='/12199'>#12199</a>. So when the report is flat, the label will contain the concatenated dimensions, but there are also values for each dimensions separately. Not sure what we should improve here?
Had forgotten this. Looks good :+1:
All other things look good.
@diosmosis before merge, just need to confirm that the feature is fully covered by system tests and UI tests?
@sgiehl can you check the 2 points left in https://github.com/matomo-org/matomo/pull/12524#issuecomment-493941140 ? or should they be fixed in a separate PR? If so can you create separate issue? Looking forward to merging this valuable feature! :+1:
in this report, dimensions are set as Country > City > Provider, but the report once flattened shows a different order: Country > Provider > City
should be fixed with the last commit. The display order of additional dimensions was reverse.
Could the feature be enabled for Behavior > Downloads report? And for Behavior > Outlinks? Not required though.
It is enabled for all reports where the dimension for the subtable differs. Otherwise it doesn't actually make much sense to split it up. Same applies to all pages reports.
@diosmosis can you please review this quickly and merge if it looks good to you?