@diosmosis opened this Pull Request on September 14th 2018 Member

More than a few changes to get it to work:

  • Use segment metadata when available to support complex segments like the one for UserCountry.City dimension.
  • Pass the pivot by dimension report's parameters when fetching so it will support pivoting by parameterized dimensions (like dimension for specific custom dimension or goal).
  • Allow Report to be passed by instance to PivotByDimension (not required but think it's helpful).
  • In Report:: getForDimension() use ReportsProvider.
  • Small tweak to exception message in PivotByDimension.

Fixes #13317

@tsteur commented on September 16th 2018 Member

FYI because I saw custom dimension changes in another PR... changing the ID of a custom dimension may break all existing custom reports that use such a dimension or a metric https://github.com/matomo-org/plugin-CustomDimensions/compare/pivot-by

@diosmosis commented on September 18th 2018 Member

Ready for review.

@tsteur commented on September 19th 2018 Member

Looks pretty much fine, mostly concerned with findReportMatchingParameters and making sure to not cause any problems with reports that cannot be found anymore. Eg around funnels, etc. As mentioned in the comment, not sure if that is needed?

@diosmosis commented on September 19th 2018 Member

Updated.

@tsteur commented on September 19th 2018 Member

Just tested it again and #13317 is fixed for me. 👍 (tested it with https://github.com/matomo-org/plugin-CustomDimensions/pull/100/files)
Before merging maybe also merge https://github.com/matomo-org/plugin-CustomDimensions/pull/100 and then update the submodule and fix the tests?

This Pull Request was closed on September 20th 2018
Powered by GitHub Issue Mirror