@sgiehl opened this Pull Request on January 31st 2018 Member
@tsteur commented on January 2nd 2019 Member

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
image

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.

@diosmosis commented on January 22nd 2019 Member

@sgiehl will you be able to update this PR?

@sgiehl commented on January 28th 2019 Member

changed the implementation. There's now an additional option as soon as flattening is active.

@diosmosis commented on March 11th 2019 Member

Tested locally and noticed some oddities w/ the UI, for example the keywords report:

image

image

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.

@tsteur commented on March 11th 2019 Member

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.

What is the plan here @sgiehl @mattab

@sgiehl commented on March 11th 2019 Member

@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 label column.

@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

@mattab commented on March 18th 2019 Member

Good start! Here is my feedback:

  • [x] make Dimension columns left-aligned (currently the second one is right aligned).
  • [ ] 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)
  • [ ] ~The open segmented visitor log isn't currently visible in the view like it is in the "hierarchical" view, but expected to see it.~
  • [x] in some cases there is a lot of empty space and looks like this (as reported by @diosmosis). Maybe there's a way to make the first label column less wide in this case?

Screenshot from 2019-03-18 15-31-00

  • [x] Sorting (alphabetical) by the second column doesn't work for me (example url), see examples:

2
1

  • [x] Currently when the report is flattened, options are in this order:
    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
  • [x] Maybe rename "Dimensions are shown combined" to "Dimensions are combined"
  • [ ] 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.
@sgiehl commented on April 1st 2019 Member

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.

@sgiehl commented on April 1st 2019 Member

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

@sgiehl commented on April 1st 2019 Member

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.

@sgiehl commented on April 2nd 2019 Member

@mattab could you have a look at my comments and the changes I've made. I think it should look much better now.

@diosmosis commented on April 7th 2019 Member

@mattab can you take another look at this PR?

@mattab commented on April 12th 2019 Member

Thanks for your answers, all makes sense.
Feedback:

  • :heavy_check_mark: Got - Others in the second column. Expected Others and on hover Others (learn more) with the link to FAQ (same behavior as when Others is shown in reports):
    image
  • :question: 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 https://github.com/matomo-org/matomo/issues/12163 https://github.com/matomo-org/matomo/pull/12199
  • :heavy_check_mark: Custom reports: created a custom report with 3 dimensions (Country, City, Provider). It looks like this:
    3 levels

When flattening the data and showing dimensions separately, we get the columns (Provider, Provider, City):
Screenshot from 2019-04-12 12-09-29

Expected instead (Country, City, Provider).

  • :heavy_check_mark: Note this is likely not an issue in this code but in core or custom reports, as the issue is also present in the flattened report (Expected to see label as Country - City - Provider but Got instead label as Provider:

Screenshot from 2019-04-12 12-10-23

  • :heavy_check_mark: Expected that searching in the report also searches recursively in all sub tables columns. Got instead: searching only find matching results in the first dimension.
  • Is the feature well covered by tests? be important not to regress this in the future
  • Note: didn't check the code
@sgiehl commented on April 22nd 2019 Member

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?

@sgiehl commented on April 23rd 2019 Member

@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.

@mattab commented on May 20th 2019 Member
 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?

@mattab commented on May 20th 2019 Member

@sgiehl found another bug:

  • in this report, dimensions are set as Country > City > Provider, but the report once flattened shows a different order: Country > Provider > City

Screenshot from 2019-05-20 22-59-10

  • Could the feature be enabled for Behavior > Downloads report? And for Behavior > Outlinks? Not required though.
Powered by GitHub Issue Mirror