Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provide a way for a type to rename any report and to disable/enable reports #7823

Open
tsteur opened this issue May 4, 2015 · 3 comments
Open
Labels
c: Platform For Matomo platform changes that aren't impacting any of our APIs but improve the core itself. Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc.

Comments

@tsteur
Copy link
Member

tsteur commented May 4, 2015

Needed for #7131 and #4734. Requires #7821 and #7822.

A "type" could be a plugin that defines a type "Website" or "MobileApp". Different types need different reports and different naming of reports.

Requirements

  • A type needs to be able to rename the name of a report
  • A type needs to be able to rename the widget title of a report
  • A type needs to be able to rename the menu title of a report
  • A type needs to be able to disable a report that would be usually enabled (eg a report that is not needed for MobileApp type)
  • A type needs to be able to enable a report that would be usually disabled (eg a new report specifically for MobileApp type)
  • In some cases we need the possibility to access all reports (eg when creating a new report via console generate:report)
  • Does it actually make more sense in general to let types enable/disable dimensions instead of reports? Or both needed long term? Eg in theory there could be two different reports based on one dimension. Disabling only one dimension would make sure all later added reports (eg by another plugin) would be disabled as well. This can be wanted, but maybe not always.

To be considered

  • Currently, all report instances are cached as it is quite expensive to create new report instances (currently). So probably a solution should allow us to still cache reporting instances (or make creating report instances fast)
  • Some reports contain titles like "Visits by days since last visit". Here we should find a solution to use %s eg %s by days since last %s if possible. This would lower the amount of reports having to rename a lot. This won't be easy and needs to be considered when working on this. I couldn't think of a nice solution so far. If it is not possible then okay. Maybe it gets more clear once we resolved the possibility to rename metrics in Add possibility to rename metrics based on the type of the site #7824
  • From this point on we will allow only reports that are defined via a Report class.
  • In the future we want to allow types to disable a report based on a disabled dimension, just FYI. Eg a type could disable a dimension "Page URL" and then the report "getPageUrls" would be disabled.
  • Sometimes we handle in the context of multiple idSites and therefore different types. Eg getReportMetadata($idSites). In this case we might need to fallback to generic wording and do not apply the rename (or we apply a generic rename on top). We might deprecate $idSites in the future.
  • Currently, eg the WidgetsList class uses Reports::getAllReports() to build a list of widgets. To do this it needs eg the name of the report which can be renamed by a type. WidgetsList is in core. If we rename the Reports only in API.getProcessedReport/getReportMetadata we could never get the renamed report name in WidgetsList as the core does not know anything about APIs and other plugins. Therefore we might need to apply some renaming in Report::getAllReports() and some in getProcessedReport (eg only metrics). Another solution would be to move WidgetsList to a plugin so it would know about APIs and would be able to access the metadata but Widgets are not only reports, it can be anything and it is kinda a core feature. Another reason why Widgets are Core: A report (which is a core feature) currently can add a widget. If Widgets were entirely done via a plugin, report would not be able to know anything about it. So moving widgets to a plugin also effects reports etc.
  • A type might want to change related reports
  • How will performance be when we have like 200 or 300 reports (in case someone has installed many "types" / custom reports / etc)
  • How will one later know which reports to disable? Requesting getReportMetadata and manually decide which reports are relevant is possible but quite a bit of work. Maybe we can make this somehow easier to get module/action for the reports to disable. Maybe a UI builder in a next step?
  • Will it be actually easy to change the order of a report and their widgets? Eg in Mobile apps other reports might be more important and one wants to list different reports on a page earlier. This is not important for now but will be at some point and maybe it helps to keep this in mind when looking for a solution.
  • We need to make sure Reports do not translate anything directly. Currently they do eg $this->name = Piwik::translate('MyKEy'). They should not do that. Otherwise renaming can become difficult and we need the actual translation key for other things (eg it will allow us to cache report instances, maybe even serialize them). This will be a must do when working on reports. Instead we should translate where needed. This will also improve performance.
  • Similar to a new WigdetConfig we might want to introduce a ReportConfig and a static configure(ReportConfig $config) method.

Questions / TBD

  • Can we use the same name for "Report Name", "Report Widget Title" and "Report Menu Title"? I know we cannot really but maybe we can somehow

Possible implementations
Here are some of my initial thoughts re possible implementations.

  1. Post an event Piwik::postEvent('Reports.getAllReports', array($idSite)) when initialising the report instances, iterate over all instances and rename the report if needed. We'd need to post the idSite as well, otherwise we cannot find the correct type. This solution would work but might be a bit slow (as we need to post many events under circumstances and we will be able to cache report instances only per site (a "Decorator" could fix that))
  2. In base report method getMenuTitle, getName, ... we could check whether there is something to rename but we do not have the current idSite there. So we do not know with what to replace. Also plugins could overwrite that method and that could lever out that behaviour. Also wouldn't work when $this->menuTitle or $this->name is used directly within the class (easily fixable).
  3. Whenever someone uses eg $report->getName() or $report->getMenuTitle() , rename the title if needed. That could work as we usually have the idSite at this point, but we'd have to do it whenever we use those methods no matter where. Error-prone... A "decorator" could help but still error-prone. Another problem: In Report::getAllReports() the reports are sorted by category, this can lead to issues if we allow to rename categories as well as they would not be sorted by there actual renamed names but by their original names.
  4. Pass $idSite to Report::getAllReports() and initialise menu title based on that. This might work. We'd also have to do this for Report::factory(). This is similar as 1) just that we do not post an event and couple report and title a bit more. Not 100% sure whether we always have the $idSite but probably.
  5. Whenever we output something (UI, Scheduled Reports, ...) always use API.getProcessedReport(), hook to that event, and rename any report name if needed. This might work, but it'll work only there. If something doesn't use getProcessedReport we have a problem. Also we have again a problem if reports are eg sorted by category. Renaming them afterwards would lead to a wrong sort order (same problem as in 4.). Edit: I think it is wrong that reports are sorted in getAllReports so this could be an acceptable solution.
  6. We will have a Type class which uses generic words by default. Eg "Property" instead of "Website". When creating a report instance we require a ReportBuilder eg via DI and set the current type. From there we create a report. A ReportRepository might handle multiple reports. A report can create more Metrics by using eg a MetricsRepository and MetricsBuilder. A report can access the current type and do eg $this->name = Piwik::translate('MyKey', $this->type->getNamePlural()); or $this->name = Piwik::translate('MyKey', $this->visitsMetric->getNamePlural());. As a report can currently define its metrics like $this->metrics = array('nb_visits', 'nb_visitoors') we could maybe keep it this way and resolve the metrics into classes in the background (maybe).
  • Solution 1 and 4 seems to work, an implementation of 4) is available in https://github.com/piwik/piwik/compare/poc_manage_apps (not finished). A specific report is always based on module, action and idSite. Should also work for removing report(s) and for Widgets::getAllWidgets.
  • We will go most likely with 6.
  • Ideally, we would maybe use solution 5) as we always have an idsite there etc. I need to implement a POC. I think it will be problematic when rendering a report as it requires a report instance and not an array as returned by processed report / report metadata. I think we might actually need to render reports in the browser and only consume data via API.getProcessedReport etc.
  • If we would render reports entirely in the browser we could go with 5) I think (ScheduledReports etc already uses metadata API)
  • In theory the report should not know anything about a site / idsite. It should rather get the $type passed.
  • In theory it is easy. We have a ReportBuilder that creates a report instance, configures it or passes a $type to it and everything is easy. Problem are translations if we use report names like %s (Visits) by days since last %s (visit)

What a report is or what a report can do, what they are used for, ...

  • A report has a name
  • A report belongs to a report category
  • A report consists of dimensions and widgets
  • A report can define a new widget
  • A report can be added/displayed on a page
  • A report defines how the report should be displayed (eg defaultSortOrder, whether search is enabled, which columns should be displayed, ...)
  • A report has related reports
@tsteur tsteur added Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc. c: Platform For Matomo platform changes that aren't impacting any of our APIs but improve the core itself. labels May 4, 2015
@mattab
Copy link
Member

mattab commented Jul 15, 2015

Hi @tsteur

Do you reckon this will be done for 3.0.0 or later?

@mattab mattab added this to the 3.0.0 milestone Jul 15, 2015
@tsteur
Copy link
Member Author

tsteur commented Jul 15, 2015

If we have generic wording everywhere then it is ok to do it after 3.0.0 I'd say. Although it might be still needed to rename same reports. I'd say let's move it to Mid term and if needed move it to 3.0.0

@diosmosis
Copy link
Member

  • For mobile app measurables, we can rename "Page Titles" => "Screens" and hide page reports

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: Platform For Matomo platform changes that aren't impacting any of our APIs but improve the core itself. Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc.
Projects
None yet
Development

No branches or pull requests

3 participants