Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Refactoring the menu classes and adding top menu hookable #1403

Closed
halfdan opened this Issue · 26 comments

4 participants

@halfdan
Collaborator

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.

@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).

@mattab
Owner

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.

@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/

@halfdan
Collaborator

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.

@mattab
Owner

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.

@halfdan
Collaborator

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.

@mattab
Owner
  • 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 https://github.com/piwik/piwik/blob/master/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.

@halfdan
Collaborator

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().

@mattab
Owner

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!

@halfdan
Collaborator
  • 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.

@mattab
Owner
  • 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

@halfdan
Collaborator

All fixed. Ready to commit :)

@mattab
Owner

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?

@mattab
Owner

Attachment: menus in random orders, top menu missing except API
Piwik Web Analytics Reports.png

@mattab
Owner

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

@halfdan
Collaborator

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).

@mattab
Owner

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

@mattab
Owner

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.

@halfdan
Collaborator

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.

@halfdan
Collaborator

Attachment: Patch against latest
MenuRestructure.patch

@mattab
Owner

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

@mattab
Owner

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

@mattab
Owner

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

@sgiehl
Collaborator

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

@halfdan
Collaborator

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!

@mattab
Owner

(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)

@halfdan halfdan added this to the Piwik 0.6.4 milestone
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.