Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#1403 closed New feature (fixed)

Refactoring the menu classes and adding top menu hookable

Reported by: halfdan Owned by:
Priority: normal Milestone: Piwik 0.6.4
Component: Plugins Wishlist Keywords:
Cc: matt, vipsoft Sensitive: no

Description

I'm currently writing a new plugin for Piwik. It will implement a functionality similar to Google Analytics Alerts. Users can create custom alerts "Goal revenue increased by 10%", "Unique visitors greater than 1000", etc. of which they will then get notified of if they occur. An Alert can be set for 1:n websites.

ToDo:

  • Piwik allows adding Menu entries to Admin and normal Menu but not to the TopMenu (handled via Smarty plugin). As first step I'll refactor the AdminMenu/Menu classes, and introduce TopMenu (as well as the appropriate hook).
abstract Piwik_Abstract_Menu
->  Piwik_Menu
->  Piwik_AdminMenu
->  Piwik_TopMenu 

The abstract class would contain the basic logic of a menu. Piwik_Menu e.g. overrides applyOrdering() to position the Overview and Evolution entries at the first position. (seperate patch)

  • Alerts uses a new hook "ArchiveProcessing.postCompute" which is triggered after all Archiving is finished.
  • 3 new database tables (alert, alert_site, alert_log)
  • Add Widget to show log of Alerts (probably later replaced by a graph)

I opened this ticket for discussion. As soon as I'm having a first preview I'll upload it here for review and comments.

Attachments (2)

Piwik › Web Analytics Reports.png (52.6 KB) - added by matt 4 years ago.
menus in random orders, top menu missing except API
MenuRestructure.patch (39.9 KB) - added by halfdan 4 years ago.
Patch against latest

Download all attachments as: .zip

Change History (28)

comment:1 Changed 4 years ago by vipsoft (robocoder)

Suggestion: focus on the core logic of the plugin, and defer the Menu refactoring since there are several UI tickets (#1048, #1154, and #1358).

comment:2 Changed 4 years ago by matt (mattab)

The menu refactoring is pure 'php' refactoring to ensure all menus (main analytics menu, top bar menu and admin menu) provide the same features (ordering, etc.). I think this can be done independently of the UI refactoring and other menu improvements.

comment:3 Changed 4 years ago by vipsoft (robocoder)

Ok. Very good.

Then wrtnaming convention, please use Piwik_Menu_Abstract (instead of Piwik_Abstract_Menu).

At the same time I think we should move core/PluginsFunctions/Menu.php and core/PluginsFunctions/AdminMenu.php into core/Menu/

comment:4 Changed 4 years ago by halfdan

Agreed. For consistency I should also rename Piwik_Menu to Piwik_Menu_Menu and Piwik_AdminMenu to Piwik_Menu_AdminMenu, does that sound reasonable?

Another thing to consider is renaming Piwik_Menu to Piwik_Menu_MainMenu (as we have TopMenu and AdminMenu), but this would definetely break backwards compatibility with 3rd party plugins (because of the hook Menu.add -> MainMenu.add and Piwik_AddMenu -> Piwik_AddMainMenu). I guess this should be left untouched.

comment:5 Changed 4 years ago by matt (mattab)

I would say Piwik_Menu_Main, Piwik_Menu_Admin and Piwik_Menu_Top, and of course leaving the existing functions unchanged as we need to keep backward compatibility.

comment:6 follow-up: Changed 4 years ago by halfdan

Alright. I'd like a code review of my menu restructuring:

  • Code moved to core/Menu/
  • Abstract class provides core functionality for all three menus (Main, Top, Admin)
  • Abstract class provides submenus which are only being used by the MainMenu. I think it's good to provide this functionality for all menu types for later extension.

Note:

  • This is only a snapshot, nothing complete!
  • Ordering/Renaming/Editing does not work yet, easy to implement though.
  • LanguageManager is not viewed yet.

I need suggestions for:

  • The Feedback plugin uses "#id=topbar-feedback", instead of providing a link to call. Piwik_AddTopMenu nor the Piwik_Menu_Abstract::add() allow to define a #fragment. I think of modifying urlRewriteWithParameters() or Piwik_Url::getCurrentQueryStringWithParametersModified to produce a fragment when the $url array contains e.g. '_frag'.
$url = array('module' => 'A', 'action' => 'B', '_frag' => 'C=Foo');

would produce ?module=A&action=B#C=Foo

  • I had to add $view->topBar = Piwik_GetTopMenu() to a lot of modules. Should the TopMenu be shown by default and possibly be deactivated similar to the normal Menu:
{assign var=showTopMenu value=false}
  • For the LanguageManager I've to allow HTML as menu entry. Idea:

-> Piwik_Abstract::addHtml(..) which allow to add HTML as menu entry OR
-> Extend Piwik_Abstract::add() with a parameter $isHTML (default=false).

Snapshot of Alerts plugin coming soon.

comment:7 in reply to: ↑ 6 Changed 4 years ago by matt (mattab)

  • The Feedback plugin uses "#id=topbar-feedback", instead of providing a link to call. Piwik_AddTopMenu nor the Piwik_Menu_Abstract::add() allow to define a #fragment. I think of modifying urlRewriteWithParameters() or Piwik_Url::getCurrentQueryStringWithParametersModified to produce a fragment when the $url array contains e.g. '_frag'.

It seems the feedback link doesn't add a fragment, but a HTML id attribute:
id=topbar-feedback

We shouldnt add complicated logic just to handle this specific use case. I think the generated HTML for the top bar would contain some sort of ID that you could use and select instead of topbar-feedback in http://dev.piwik.org/trac/browser/trunk/plugins/Feedback/templates/feedback.js#L2

  • I had to add $view->topBar = Piwik_GetTopMenu() to a lot of modules. Should the TopMenu be shown by default and possibly be deactivated similar to the normal Menu:
{assign var=showTopMenu value=false}

Yes that sounds good (the $view->topBar = Piwik_GetTopMenu() could then be refactored in setGeneralVariablesView maybe?

  • For the LanguageManager I've to allow HTML as menu entry. Idea:

-> Piwik_Abstract::addHtml(..) which allow to add HTML as menu entry OR
-> Extend Piwik_Abstract::add() with a parameter $isHTML (default=false).

because it is currently only used in the top bar, you can maybe overwrite add() in the Top Menu class to allow for HTML as a menu entry.

comment:8 Changed 4 years ago by halfdan

Patch updated.

  • Editing/Renaming works.
  • Ordering works.
  • TopMenu automatically generates IDs on the form "topmenu-{$module}", adjusted feedback.js (topbar -> topmenu).
  • TopMenu adds function addHtml. Overwriting add() would mean to reimplement the same code from Abstract + extra handling of HTML. I extended Piwik_AddTopMenu instead to take a $isHTML parameter (default: false) which determines if the second parameter is a $url or $html.
  • Views can use
{assign var=showTopMenu value=false} 

to disable the TopMenu which is now set by default in setGeneralVariablesView().

comment:9 Changed 4 years ago by matt (mattab)

looks good! Few questions

  • I see following lines seem the same, is the first one needed?
     	88	                Piwik_AddMenu('Actions_Actions', '', array('module' => 'Actions', 'action' => 'getPageUrls'), true, 15); 
     	89	                Piwik_AddMenu('Actions_Actions', 'Actions_SubmenuPages', array('module' => 'Actions', 'action' => 'getPageUrls'), true, 1); 
    
    I also see the same call twice in plugins/Goals/Goals.php and Referers.php
  • patch should delete file core/SmartyPlugins/function.assignTopBar.php
  • in plugins/Dashboard/Dashboard.php the comment could be removed and the call to the function Piwik_AddMenu at the bottom removed too?
  • unnecessary brackets in
    if(($language = Piwik_LanguagesManager_API::getInstance()->getLanguageForSession()) != null) 
    

Appart from these minor things, it looks good - please confirm that you have tested the code, submit last patch and I will commit it, thanks!

comment:10 Changed 4 years ago by halfdan

  • Yes the line is needed due to the ordering. The last argument differs everytime. The first line adds the first level menu with order 15, the second line adds the submenu "Pages" with order 1.
  • Patch now deletes the SmartyPlugin
  • Regarding the Dashboard: That was my intention, forgot to review that file.
  • Removed the brackets, it works without them. My IDE (Netbeans) complains about an "Possible unwanted assignment in line x" - that was the reason I added the != null in the first place..

I tested all code intensively! - However, I just discovered a nasty bug for which I have no good idea how to fix it. For the "Visits" page the submenus are added by different plugins. As a result the first level menu "Visits" gets added indirectly (as a consequence of adding a submenu). This leads to the URL of UserSettings.index being the one that is associated with the first level menu. A later call to

Piwik_AddMenu('General_Visitors', '', array('module' => 'VisitsSummary', 'action' => 'index'), 10);

gets ignored, as the menu has already been added. The quick-fix in this case would be a call to

Piwik_EditMenuUrl($mainMenuToEdit, $subMenuToEdit, $newUrl)

this however would mean, that we'd need to do this for every first level menu entry, as new plugins could add submenus to them before the first level menu has been added.

Alternative: Allow to directly overwrite menu entries (would make Piwik_Edit*Url) obsolete.

Give me a short response / idea and I adjust it accordingly.

comment:11 Changed 4 years ago by matt (mattab)

  • in Piwik_AddMenu, can you make it work so that it caches calls and then builds the menu once all calls to addMenu have been done (ie. when you request the menu)?
  • sorry for the brackets, you were right that they are important, I didn't see the first half of expression

Let me know when bug is fixed, and I'll commit

comment:12 Changed 4 years ago by halfdan

All fixed. Ready to commit :)

comment:13 Changed 4 years ago by matt (mattab)

I applied the patch but the top menu didn't display, and the menu was displaying in random orders (see screenshot). is it working for you?

Changed 4 years ago by matt (mattab)

menus in random orders, top menu missing except API

comment:14 Changed 4 years ago by matt (mattab)

Also you can put the Piwik header files to the new files (eg. see core/Piwik.php header).

Also can you can remove empty comments (or write comments ;) from core/Menu/Abstract.php and other files.
cheers

comment:15 Changed 4 years ago by halfdan

The ordering works perfectly for me, did you maybe had conflicts in the files? When I updated I had several conflicts that I had to resolve. I upload a new patch containing comments, correct header and diffed against the latest trunk revision (r2400).

comment:16 Changed 4 years ago by matt (mattab)

the patch is missing Actions/Actions.php menu calls with weights it seems.

Otherwise top menu displays OK for me, just "Actions" and "UI Framework" (plugin ExampleUI) menu is in wrong order.

cheers

comment:17 Changed 4 years ago by matt (mattab)

Also when I visit the tracker after applying patch, I get the error:
: Class 'Piwik_Menu_Abstract' not found in core\Menu\Main.php on line 16

The menu files should not be loaded in the tracker as it slightly impacts performance.

comment:18 Changed 4 years ago by halfdan

Actions.php - Yeah :( My IDE screwed up the SVN states and therefore did not include the file in the patch.

ExampleUI - Added ordering.

Added comments to the Piwik_Add*Menu functions.

I agree that the menu should not be loaded when running the tracker - This is a pre-existing condition though as the menu is/was being loaded in core/PluginManager.php. The error you get is due to the autoloader being unavailable in tracking mode. To avoid the error I manually included the Abstract.php in PluginManager.php for now.

I applied the patch on a fresh checkout and it worked for me - Please recheck it though my IDE might've missed some files again.

Changed 4 years ago by halfdan

Patch against latest

comment:19 Changed 4 years ago by matt (mattab)

  • Resolution set to fixed
  • Status changed from new to closed

(In [2409]) Fixes #1403 Patch by halfdan: great stuff! Menu code is now clean and refactored.

comment:20 Changed 4 years ago by matt (mattab)

  • Resolution fixed deleted
  • Status changed from closed to reopened

The "Alerts plugin" is not fixed yet of course, reopening. halfdan, do you know when a beta will be available for Alerts? cheers

comment:21 Changed 4 years ago by matt (mattab)

Regression: It seems the feedback link doesn't work anymore in trunk. It loads the content instead of opening a popover.

comment:22 Changed 4 years ago by SteveG (sgiehl)

(In [2412]) refs #1403 feedback form is working again

comment:23 Changed 4 years ago by halfdan

SteveG: Thanks, missed that file in the patch.
matt: Yes, I'm running a little late due to exams/packing up my stuff before returning to Europe, but I'll be having a beta in approx. 3-4 days.

Thanks for commiting the refactored menu!

comment:24 Changed 4 years ago by matt (mattab)

(In [2514]) Refs #1403
Added support for JSON/Serialized PHP export of Multi dimensional arrays in API responses, used by Alert plugin in the UI (json)

comment:25 Changed 4 years ago by matt (mattab)

  • Resolution set to fixed
  • Status changed from reopened to closed
  • Summary changed from Plugin: Alerts to Refactoring the menu classes and adding top menu hookable

comment:26 Changed 4 years ago by vipsoft (robocoder)

  • Milestone changed from 2 - Piwik 0.8 - A Web Analytics platform to 0 - Piwik 0.6.4
Note: See TracTickets for help on using tickets.