@diosmosis opened this Pull Request on September 2nd 2017 Member

Fixes #11942

Problem

The WidgetConfig's category/subcategory is used in three different contexts. It's used to place the widget in a page, used to display which widgets users can add to a dashboard and used to differentiate widgets in a 'ByDimension' layout. The last of these uses conflicts with the others.

The 'Sales by' widgets are added to a 'ByDimension' container. This container is displayed as the Ecommerce > Sales page. These widgets have a custom category/subcategory set to 'Sales by ...'. Since they don't have a recognized widget category/subcategory, they do not appear in the dashboard manager (or by themselves in a page).

We could set the category/subcategory of these widgets to Ecommerce/Sales so they appear in the dashboard manager, but this causes two problems. First, the 'ByDimension' report renders incorrectly (instead of Sales by ... categories, there will be just one, 'Sales'). Second, the name of the widget in dashboard manager becomes the dimension name (so 'City' instead of 'Sales by City').

We could add new WidgetConfig instances for the Sales by ... widgets, eg, by using the Widget.addWidgetConfigs event, but this causes another problem. Since API.getReportPagesMetadata automatically adds these reports to the Ecommerce > Sales page, there ends up being two of each report on that page. It seems currently impossible to add a widget to the dashboard manager, but exclude it from pages.

Solution

This PR has a quick solution to the above problem. Instead of relying on the category/subcategory/report name for 'ByDimension' reports, it introduces two new widget config fields: dimensionCategory and dimensionName.

The piwikWidgetByDimensionContainer directive uses these fields if present (falling back on normal subcategory/name). This allows us to widgetize reports that are placed within a 'ByDimension' layout, keeps the API BC (mostly), and allows us to display the report in the dashboard manager while excluding it from pages.

There's only one issue I can think of: consumers of the API might need to change if they use 'ByDimension' layouts (eg, maybe the mobile app?). They would have to start using the new categories.

A long term solution would require separating the conflicting concepts and implementing something more domain driven. An entry that appears in the dashboard manager is different from a widget displayed in a page is different from a widget contained in a 'ByDimension' layout. The code should reflect that. (If this is the preferred approach, I can do that instead of this approach, but it would be a larger change w/ potential BC breaks).

@diosmosis commented on September 2nd 2017 Member

@tsteur @sgiehl @mattab please read & review this PR. Not done yet, but should be reviewed before moving on.

@tsteur commented on September 2nd 2017 Owner

Would it be maybe possible to iterate recursive in https://github.com/piwik/piwik/blob/3.0.5-b2/plugins/API/WidgetMetadata.php#L40-L41 and mark it here as widgetizable or so? https://github.com/piwik/piwik/blob/3.0.5-b2/plugins/Goals/Pages.php#L305

If you then made this here recursive to include categories and subcategories of widgets in containers the naming issue might be resolved as well: https://github.com/piwik/piwik/blob/3.0.5-b2/plugins/API/WidgetMetadata.php#L31 (to be tested). It might still somewhat have the same name as in the reports so probably won't fix 100% the problem just yet.

I would not really want to add a dimensionName etc to those widgets as they are not related in any way to a dimension etc. A widget is often just a plain widget that shows a help info but not a report etc. It also complicated things with specifying different names etc. If it is needed to add this, then it should be somehow only added to a ByDimensionWidgetContainerConfig class if somehow possible.

I would try to fix the above mentioned issues and then it might show it correctly as they have basically a subcategory set like "Goals by XYZ", it would only need to be added to the right category. Currently they are not shown there since we do not recursively search for all widgets in containers (and not yet create categories dynamically)

@diosmosis commented on September 3rd 2017 Member

Would it be maybe possible to iterate recursive in https://github.com/piwik/piwik/blob/3.0.5-b2/plugins/API/WidgetMetadata.php#L40-L41 and mark it here as widgetizable or so? https://github.com/piwik/piwik/blob/3.0.5-b2/plugins/Goals/Pages.php#L305
If you then made this here recursive to include categories and subcategories of widgets in containers the naming issue might be resolved as well: https://github.com/piwik/piwik/blob/3.0.5-b2/plugins/API/WidgetMetadata.php#L31 (to be tested). It might still somewhat have the same name as in the reports so probably won't fix 100% the problem just yet.

So is this your suggested solution?

  • Add categories recursively from createMissingCategoriesAndSubcategories(), so categories like 'Sales by Referrers' will show up in the widget metadata.
  • Then in the dashboard manager, instead of showing them as:
    Ecommerce - Products
    ...
    Sales by Referrers
    - Referrer Type
    - ...
    Sales by ...

    show them as

    Ecommerce - Sales
    Sales by Referrer Type
    Sales by ...
    ...

    ? (I haven't tested if this is how it would appear as w/o modifications, but I'm pretty sure.)

I'm not sure if I understand completely, seems like it won't work out of the box.

A ByDimensionWidgetContainerConfig class might work, it would let us separate dimension name/dimension category from widget name/category. Let me know if this is an acceptable solution.

@tsteur commented on September 3rd 2017 Owner

Check out https://github.com/piwik/piwik/compare/11942?expand=1 there it seems to work for me.
image

The only downside I see is that an added widget with only the title of eg "Device Type" it is not clear that it is "Ecomerce Sales by ... Device Type". We could change the widget title with some hack dynamically (eg when API.getWidgetMetadata is requested) but would make things just complicated. Maybe always adding the subcategory to the widget title shown in the dashboard but would result in long widget names. Would make things possibly much more clear though.

Here you can see you would know that those device types are only for sales:
image

@tsteur commented on September 3rd 2017 Owner

The fix for the widget title would be actually that when there are widgets in a widget container with a "by dimension" layout, that we automatically prepend the subcategory of the widget to the widget title like Sales by user attribute: Device type in WidgetMetadata.php but only when getting getWidgetMetadata(). This would not even by a hack but a proper solution.

@diosmosis commented on September 3rd 2017 Member

The issue linked specifies that there should be one new widget category "Ecommerce - Sales", not multiple "Ecommerce - Sales by ..." categories. @mattab can you confirm having multiple categories is ok?

@tsteur commented on September 3rd 2017 Owner

We could fix this as well in WidgetMetadata.php that we define to always apply the first level subcategory for WidgetContainers and prepend the subcategory of the actual Widget within a WidgetContainer to the widget title. I wouldn't even consider this a hack since it makes sure we keep showing our reporting menu structure again in the dashboard selector and we have clear widget title names.

There may be a hack needed to do this only for WidgetContainers when the layout is ByDimension but not sure if it would change existing structure if we did this in general.

@diosmosis commented on September 3rd 2017 Member

Maybe I'm not understanding, but if we use the widget category of the container (in this case 'Sales') for the widget category of the actual widget (ie, 'Sales by Referrer'), then the widget will appear in the dashboard manager AND in the Ecommerce > Sales page, right? Then in that page, there will be the widget + the widget container with the ByDimension layout.

This was one of the problems I encountered, setting the right category on the WidgetConfig makes it appear in the dashboard manager, but it also shows it twice in the actual page.

EDIT: Nevermind, I think getPagesMetadata doesn't recurse, while getWidgetsMetadata does.

@diosmosis commented on September 3rd 2017 Member

Ok, I think I understand the system now, thanks for your patience @tsteur ;)

@tsteur commented on September 3rd 2017 Owner

Oki :)

Just to roughly explain it: We basically would need to change only the logic in WidgetMetadata.php in my other branch and only when calling API.getWidgetMetadata (not API.getPagesMetadata).

When we iterate over the widgets, we would basically somehow remember the first level subcategory in WidgetMetadata::getWidgetMetadata() maybe like this and forward it to all calls:

diff --git a/plugins/API/WidgetMetadata.php b/plugins/API/WidgetMetadata.php
index 2d77e76..453a6ec 100644
--- a/plugins/API/WidgetMetadata.php
+++ b/plugins/API/WidgetMetadata.php
@@ -36,6 +36,8 @@ class WidgetMetadata

             /** <a class='mention' href='https://github.com/var'>@var</a> WidgetConfig[] $widgets */
             $widgets = array($widgetConfig);
+            $subcategoryId = $widgetConfig->getSubcategoryId();
+
             if ($widgetConfig instanceof WidgetContainerConfig) {
                 // so far we go only one level down, in theory these widgetConfigs could have again containers containing configs
                 $widgets = array_merge($widgets, $widgetConfig->getWidgetConfigs());
@@ -47,7 +49,7 @@ class WidgetMetadata
                     continue;
                 }

-                $flat[] = $this->buildWidgetMetadata($widget, $categoryList);
+                $flat[] = $this->buildWidgetMetadata($widget, $subcategoryId, $categoryList);
             }
         }

it is just a rough idea on how to implement this but haven't tested if it would work like this

@diosmosis commented on September 5th 2017 Member

Ready for review, though the UI tests might not pass (there were some failures in the last run I couldn't explain, they might be random).

@diosmosis commented on September 5th 2017 Member

One thing to note: this adds widgets like 'Sales by Referrers: Referrer Type', but it also changes the name of some existing widgets, eg, 'Actions: Content Name'. Let me know if that is not desired.

@tsteur commented on September 5th 2017 Owner

I think @mattab can probably say more about 'Actions: Content Name' whether it is desired or not.

@mattab commented on September 18th 2017 Owner

it also changes the name of some existing widgets, eg, 'Actions: Content Name'. Let me know if that is not desired.

It should be fine to prefix the name, if it becomes a problem we could adjust later on

This Pull Request was closed on September 25th 2017
Powered by GitHub Issue Mirror