@diosmosis opened this Pull Request on October 26th 2020 Member

Fixes #15435

@tsteur commented on October 28th 2020 Member

@diosmosis haven't had a detailed look yet or tried it yet but do you think it is possible to remove a report when a dimension is disabled? Say there is a report "City" with a "City" dimension but this dimension is disabled. I reckon this might be possible to do in Piwik\Plugins\Report::isEnabled which would then check if a dimension is set and if so check if that dimension was maybe disabled.

Then ideally the widget and report would disappear from the UI. Or maybe better we would render a simple standard view saying "This report was disabled by a super user. To enable it and start tracking this data, please ask a super user to enable this report in ..."

@diosmosis commented on October 29th 2020 Member

@tsteur added report disabling + some dimension names + only showing tracked dimensions

@diosmosis commented on October 30th 2020 Member

Btw... when selecting a dimension, it always closes directly after just selecting one dimension. Not sure if it can stay open?

This is caused by the following code: https://github.com/matomo-org/matomo/blob/4.x-dev/plugins/CorePluginsAdmin/angularjs/form-field/form-field.directive.js#L102-L108

I'm not sure if it's really needed or even what it's intended purpose was.

@diosmosis commented on October 30th 2020 Member

Wonder if we should move this setting to Privacy -> Anonymise data page or alternatively at least under the Privacy Manager system settings maybe?

I thought privacy might be a good place as well.

Don't really have an opinion on the other UX items, mostly those seem to be about the scope of the issue.

@mattab commented on November 1st 2020 Member

@mattab if you have any thoughts on this let us know ASAP

My main thought is about find any possible issue before people find it themselves (it would save us lots of time if we found any bug proactively). for example: could we create some test to disable all the dimensions using this feature, and track some various data (pageview, event, download, outlinks, with most other dimensions set) and test if tracking+archiving+some APIs still work as expected? (at least one manual test and also some automated tests).

@diosmosis commented on November 2nd 2020 Member

for example: could we create some test to disable all the dimensions using this feature, and track some various data (pageview, event, download, outlinks, with most other dimensions set) and test if tracking+archiving+some APIs still work as expected? (at least one manual test and also some automated tests).

I can add a system test like this.

@tsteur commented on November 2nd 2020 Member

Be good to move it into the privacy manager since it is mostly interesting for privacy reasons to track less data.

@diosmosis let me know when all the changes are done and I'll review again

@diosmosis commented on November 4th 2020 Member

@tsteur still fixing the tests, but this should be ready for another review

@tsteur commented on November 8th 2020 Member

@diosmosis is this ready for another review? or any questions open?

@diosmosis commented on November 9th 2020 Member

@tsteur yes it's ready for a review

@diosmosis commented on November 9th 2020 Member

@tsteur yes ready for another review

@tsteur commented on November 9th 2020 Member

Thanks @diosmosis.

I started reviewing again and started having these points

  • Can we move this to the privacy manager -> anonymise data page?
  • When there is no data for a report, and it has a dimension that is disabled, can we show a message that it's showing maybe no data because the dimension is disabled?
  • Can we hide dimensions that store a numeric or enum value since they likely won't store any personal data from a privacy perspective? I reckon eg "number of interactions" etc would then remove from the list. This would I think solve two problems: Too many dimensions in the list make it hard to use and some dimensions could break something maybe like "Visit ecommerce status at the end of the visit".

But then noticed the feature is not all that useful as I was hoping if we don't also remove the reports from the UI that are disabled. However, removing these reports might cause issues that people don't understand why certain reports are not visible etc. And then these dimensions would still appear in segments etc and for some dimensions we would need to do more hard coding to disable the related reports (like removing real time map if longitude or latitude dimension is disabled). Disabling some dimensions could cause problems eg with fingerprint (when disabling plugins). Disabling referrer url or referrer type could break the correctness of quite a few reports as well and everything is overall not that predictable what would happen when disabling certain dimensions. I should have thought it more through from the beginning as it turns out quite complicated. I reckon overall the feature might not be used that much but could cause more issues (especially in support and troubleshooting bugs etc) that it is not worth proceeding with this for now. As a workaround on On-Premise people still have the possibility to disable certain dimensions by disabling plugins. We might need to roll this feature out for Cloud and On-Premise as well although Matomo isn't really tested much when certain plugins are disabled and it doesn't always work. I'm thinking though overall disabling entire plugins might be more of a solution then this maybe.

Thanks for your work on this @diosmosis. For now we will close this PR and also close the related issue as a wontfix.

@diosmosis in a new PR can we still keep this change to add a default value to visits count? https://github.com/matomo-org/matomo/pull/16609/files#diff-4de94fc1a73792e176363f13ba9a47f47f0d69bef9e180f6b11e11b828a14c2cR21

And maybe we could also keep the changes for adding the dimension names like for the plugin dimensions as eg in https://github.com/matomo-org/matomo/pull/16609/files#diff-c025d6a1afc9d0b6ed34a29a2fe6f050da06b16821d05cbb8445781a684ee8f5R23 ?
This would make them actually available in custom reports

@diosmosis commented on November 9th 2020 Member

@tsteur I guess we'd want to go through and list every dimension that someone might want to disable and see if it is already in its own plugin.

I can move the other changes into a new PR.

Also I guess I misread the privacymanager change, I moved it to the plugin's settings, not the anonymise data page.

@tsteur commented on November 9th 2020 Member

Thanks for creating the other PR @diosmosis 👍

This Pull Request was closed on November 9th 2020
Powered by GitHub Issue Mirror