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

Admin Menu custom presets, import/export a preset into/from database driven menu #16451

Merged
merged 14 commits into from Jul 31, 2017

Conversation

Projects
None yet
10 participants
@izharaazmi
Contributor

izharaazmi commented Jun 2, 2017

Summary of Changes

As we know that with the release of Joomla 3.7 we have the Admin Menu Manager that lets you customise the administrator menu according to your needs. The backend menu comes in two flavours: Custom Menu and Preset Menu. You can have as many custom menu each configured in different way as suitable. Or if you do not want to customise then you use the Preset menu.

This PR is indented to extend the features for this Admin Menu Manager as promised in the initial strategy.

Changes

  1. Custom Presets:
    If you copy (and modify) one of the existing preset files from /administrator/com_menus/presets to /administrator/templates/{your_template}/html/com_menus/presets then you can use it as your own custom preset.

  2. Import Preset Menu:
    If you would like to modify merely one or two things and keep rest of the menu structure the same way as in a preset menu you still need to create all of those links one by one yourself manually. After this PR Joomla will allow you to directly import an existing preset (core or custom) into your custom admin menu directly and then you can customise it very quickly. When creating/editing a Menu (menutype) you can optionally select a preset that you would like to import.

  3. Export Menu as Preset:
    You can also export your database driven custom admin menu as an a Preset. Just select a Menu in the list and click Download As Preset button in the toolbar.

  4. Menu Module Layout Override:
    The current implementation of the Admin Menu Module has the HTML rendering from within the PHP class JAdminCssMenu which does not let you customise the HTML in any way. This PR puts all the HTML output logic into the layout files under tmpl/ folder as commonly expected. This way you can easily override the layout in your template.

  5. Menu Structure Override:
    Prior to Joomla 3.7 we could override the menu links using default_enabled.php and default_disabled.php. After the Menu Manager was introduced this ability was removed. Most likely you would really never need this anymore. It is always better to import a preset in a custom menu and customise it in the Menu Manager instead of messing with any PHP code. However, if you still insist, you can use the Custom Preset as discussed above. The naming convention is like my-custom-menu.xml becomes My Custom Menu (we can improve this later if needed). It is really easy to understand and work with. Give it a try.

Testing Instructions

  1. Create a custom admin menu and set it as your active admin menu in the module settings.

    • It should work correct and show the links based on the appropriate permissions.
    • Notice about missing important menu links (if any) should be displayed as before.
    • Clicking on the Menu Recovery link should enable the recovery mode and you should be able to exit the recovery mode too.
    • Components container should also work correctly like hiding/showing choices as configured.
    • Installing a new component should make the link under the components container menu.
  2. Try to choose another preset in the module settings. The displayed menu should be updated according the selected preset.

  3. Create a custom preset by copying (as discussed above) and make some changes. Set this custom preset as your active menu. The menu should now load you custom preset.

  4. Create new custom admin menu and choose a preset to import. This should load the menu links and hierarchy from the preset.

  5. From the list view of Menu, select one of your Admin Menus and click on "Download as Preset" button. The XML file should be downloaded correctly and should contain the selected menu links with correct settings.

Documentation Changes Required

I'd like to write the detailed documentation of the entire Admin Menu Manager. Please advise where should I head towards.


PS: As I am on a vacation this PR was created in hurry. Pardon me for any mistakes and please ask for any kind of clarifications or suggestions. I'll try to respond as quickly as possible.

{
$menuItems = array();
JFactory::getApplication()->enqueueMessage(JText::_('JERROR_AN_ERROR_HAS_OCCURRED'), 'error');

This comment has been minimized.

@andrepereiradasilva

andrepereiradasilva Jun 2, 2017

Contributor

i didn't review the rest of the PR, but just one comment to this line

Why catch the default JDatabase (or Registry) exception and replace it with something called JERROR_AN_ERROR_HAS_OCCURRED?

This is more or less the same as saying: "Joomla knows the system is not working properly but doesn't have idea what happened! All it can say is .. an error has occurred ..." 😄

Maybe use $e->getMessage() ?

@andrepereiradasilva

andrepereiradasilva Jun 2, 2017

Contributor

i didn't review the rest of the PR, but just one comment to this line

Why catch the default JDatabase (or Registry) exception and replace it with something called JERROR_AN_ERROR_HAS_OCCURRED?

This is more or less the same as saying: "Joomla knows the system is not working properly but doesn't have idea what happened! All it can say is .. an error has occurred ..." 😄

Maybe use $e->getMessage() ?

This comment has been minimized.

@izharaazmi

izharaazmi Jun 2, 2017

Contributor

You're right. I actually reused the old code as is. I'll fix when I get to steal few minutes. Please review further so I can fix them all at once. Thanks.

@izharaazmi

izharaazmi Jun 2, 2017

Contributor

You're right. I actually reused the old code as is. I'll fix when I get to steal few minutes. Please review further so I can fix them all at once. Thanks.

@izharaazmi

This comment has been minimized.

Show comment
Hide comment
@izharaazmi

izharaazmi Jun 2, 2017

Contributor

@sanderpotjer It would be really great if you could look at this in the light of our disscussion at #13036 (comment). Thanks.

Contributor

izharaazmi commented Jun 2, 2017

@sanderpotjer It would be really great if you could look at this in the light of our disscussion at #13036 (comment). Thanks.

@StefanSTS

This comment has been minimized.

Show comment
Hide comment
@StefanSTS

StefanSTS Jun 3, 2017

Contributor

Hello Izhar,
nice work done with a lot of effort. Thanks.
I am testing and will do some more testing.
I copied the full https://github.com/joomla/joomla-cms/tree/c787b79e6ae7a198087f9c72434856d3b6f63ee3 over an existing J 3.7.2 for testing.
I found an issue if you add a menu item to a new menu that is already existent maybe through automatically added menu or another custom menu. BE ends up white with error 1242.
The error message is: Subquery returns more than 1 row

To avoid this error there might be options like:

  • compare the menu items in db before adding a new link and do not allow a second entry or
  • query the db in a way that only the corresponding item to the menu is found (maybe by id instead of com_something/menu type).

I got two other error messages after creating a new menu:
Notice: Undefined property: MenusViewMenus::$filterForm in /opt/lampp/htdocs/j36vm32/administrator/components/com_menus/layouts/joomla/searchtools/default/bar.php on line 37
Fatal error: Call to a member function getField() on null in /opt/lampp/htdocs/j36vm32/administrator/components/com_menus/layouts/joomla/searchtools/default/bar.php on line 37
I was distracted by some other work. After reloading the page, I guess, the menu appeared and I could not reproduce this. I will test this further in a while.

Contributor

StefanSTS commented Jun 3, 2017

Hello Izhar,
nice work done with a lot of effort. Thanks.
I am testing and will do some more testing.
I copied the full https://github.com/joomla/joomla-cms/tree/c787b79e6ae7a198087f9c72434856d3b6f63ee3 over an existing J 3.7.2 for testing.
I found an issue if you add a menu item to a new menu that is already existent maybe through automatically added menu or another custom menu. BE ends up white with error 1242.
The error message is: Subquery returns more than 1 row

To avoid this error there might be options like:

  • compare the menu items in db before adding a new link and do not allow a second entry or
  • query the db in a way that only the corresponding item to the menu is found (maybe by id instead of com_something/menu type).

I got two other error messages after creating a new menu:
Notice: Undefined property: MenusViewMenus::$filterForm in /opt/lampp/htdocs/j36vm32/administrator/components/com_menus/layouts/joomla/searchtools/default/bar.php on line 37
Fatal error: Call to a member function getField() on null in /opt/lampp/htdocs/j36vm32/administrator/components/com_menus/layouts/joomla/searchtools/default/bar.php on line 37
I was distracted by some other work. After reloading the page, I guess, the menu appeared and I could not reproduce this. I will test this further in a while.

@StefanSTS

This comment has been minimized.

Show comment
Hide comment
@StefanSTS

StefanSTS Jun 3, 2017

Contributor

Method of import
On making a new menu "menunew" I can import the joomla.xml or modern.xml.
Example:
I import joomla.xml.
I check the menu, I don't like it.
I import modern.xml.
Now I have all the menu items from both imports.

Iit would be helpful to have an option to either add the new preset to the existing items or replace all existing items. So there is the possibility to add either a preset for only a component or a full menu.

New presets not showing
I placed a new XML file into administrator/components/com_menus/presets/, called menunew.xml.
This file is not showing up in my importable xml files in the menu edit. Placed it into the templates override folder like given above, and it also does not show.

File structure xml vs. php
One more thing about the new xml file structure. Before it was a php file (default_enabled.php), so template parameters could be imported and used as custom settings.
Template settings like minimal, normal, advanced. The user could choose his preferred way of displaying the menu. Lets say, have 2 menu items, 10 or 24, depending on what he needs for daily work or if he uses a small screen device or a desktop computer.
I guess with an xml file we could loose this simple way of changing the menu view.

Suggestion for mobile view
With an additional parameter viewlevel in the xml this could be done if the menu edit gets this too.

This would be a good feature for people who do a lot of their work with mobile devices. And it would kind of restore a possibility you had with php files before.

Contributor

StefanSTS commented Jun 3, 2017

Method of import
On making a new menu "menunew" I can import the joomla.xml or modern.xml.
Example:
I import joomla.xml.
I check the menu, I don't like it.
I import modern.xml.
Now I have all the menu items from both imports.

Iit would be helpful to have an option to either add the new preset to the existing items or replace all existing items. So there is the possibility to add either a preset for only a component or a full menu.

New presets not showing
I placed a new XML file into administrator/components/com_menus/presets/, called menunew.xml.
This file is not showing up in my importable xml files in the menu edit. Placed it into the templates override folder like given above, and it also does not show.

File structure xml vs. php
One more thing about the new xml file structure. Before it was a php file (default_enabled.php), so template parameters could be imported and used as custom settings.
Template settings like minimal, normal, advanced. The user could choose his preferred way of displaying the menu. Lets say, have 2 menu items, 10 or 24, depending on what he needs for daily work or if he uses a small screen device or a desktop computer.
I guess with an xml file we could loose this simple way of changing the menu view.

Suggestion for mobile view
With an additional parameter viewlevel in the xml this could be done if the menu edit gets this too.

This would be a good feature for people who do a lot of their work with mobile devices. And it would kind of restore a possibility you had with php files before.

@izharaazmi

This comment has been minimized.

Show comment
Hide comment
@izharaazmi

izharaazmi Jun 6, 2017

Contributor

@StefanSTS Below are my responses as per my understanding to your inputs:

It would be helpful to have an option to either add the new preset to the existing items or replace all existing items...

If we replace all the existing menu items during a single click import process it would be an easy destructive action. Therefore, I omitted that feature on purpose. The user must "select all" and "delete" all existing menu items if so intended.

I placed a new XML file into ... not showing up in my importable xml files in the menu edit. Placed it into the templates override folder like given above, and it also does not show.

I tested this and it is working here as expected. I may be missing something, please let me know if you still get this issue.

File structure xml vs. php

The preset XML files are to represent the menu item definition and hierarchy. Their rendering is still controlled via the layout files in the mod_menu layout files/overrides. Layout files are PHP files as usual and the module options are accessible in the code there.

Therefore, you still have all the controls over the rendering while keeping the preset definition as simple and general.

Additionally, I'd like to mention that those default_enable and default_disabled.php did not give you much control over rendering rather only on the hierarchy. The HTML output was hard-coded within the class method which could not be overridden. With this PR you get all that flexibility. Your mobile rendering idea too can work even better by just overriding the layout files appropriately and not modifying the preset XML files at all.

Contributor

izharaazmi commented Jun 6, 2017

@StefanSTS Below are my responses as per my understanding to your inputs:

It would be helpful to have an option to either add the new preset to the existing items or replace all existing items...

If we replace all the existing menu items during a single click import process it would be an easy destructive action. Therefore, I omitted that feature on purpose. The user must "select all" and "delete" all existing menu items if so intended.

I placed a new XML file into ... not showing up in my importable xml files in the menu edit. Placed it into the templates override folder like given above, and it also does not show.

I tested this and it is working here as expected. I may be missing something, please let me know if you still get this issue.

File structure xml vs. php

The preset XML files are to represent the menu item definition and hierarchy. Their rendering is still controlled via the layout files in the mod_menu layout files/overrides. Layout files are PHP files as usual and the module options are accessible in the code there.

Therefore, you still have all the controls over the rendering while keeping the preset definition as simple and general.

Additionally, I'd like to mention that those default_enable and default_disabled.php did not give you much control over rendering rather only on the hierarchy. The HTML output was hard-coded within the class method which could not be overridden. With this PR you get all that flexibility. Your mobile rendering idea too can work even better by just overriding the layout files appropriately and not modifying the preset XML files at all.

@StefanSTS

This comment has been minimized.

Show comment
Hide comment
@StefanSTS

StefanSTS Jun 7, 2017

Contributor

Thanks for your reply, Izhar. I will be testing again when time permits on a fresh installation. This week is a little tight though, not sure if it will fit in.

Contributor

StefanSTS commented Jun 7, 2017

Thanks for your reply, Izhar. I will be testing again when time permits on a fresh installation. This week is a little tight though, not sure if it will fit in.

@brianteeman

This comment has been minimized.

Show comment
Hide comment
@brianteeman

brianteeman Jul 24, 2017

Contributor

@izharaazmi can you look at the merge conflicts please - then i can test it :)

Contributor

brianteeman commented Jul 24, 2017

@izharaazmi can you look at the merge conflicts please - then i can test it :)

@izharaazmi

This comment has been minimized.

Show comment
Hide comment
@izharaazmi

izharaazmi Jul 24, 2017

Contributor

Sure

Contributor

izharaazmi commented Jul 24, 2017

Sure

@izharaazmi

This comment has been minimized.

Show comment
Hide comment
@izharaazmi

izharaazmi Jul 24, 2017

Contributor

@brianteeman I have resolved the conflicts. Travis failure is due to some internal error and not on code.

Contributor

izharaazmi commented Jul 24, 2017

@brianteeman I have resolved the conflicts. Travis failure is due to some internal error and not on code.

@izharaazmi

This comment has been minimized.

Show comment
Hide comment
@izharaazmi

izharaazmi Jul 26, 2017

Contributor

@brianteeman @andrepereiradasilva It'd be great if we could get this before the 3.8 feature freeze.

Contributor

izharaazmi commented Jul 26, 2017

@brianteeman @andrepereiradasilva It'd be great if we could get this before the 3.8 feature freeze.

@brianteeman

This comment has been minimized.

Show comment
Hide comment
@brianteeman

brianteeman Jul 26, 2017

Contributor

Error displaying the error page: Class 'Joomla\CMS\Menu\Tree' not found: Class 'Joomla\CMS\Menu\Tree' not found

Contributor

brianteeman commented Jul 26, 2017

Error displaying the error page: Class 'Joomla\CMS\Menu\Tree' not found: Class 'Joomla\CMS\Menu\Tree' not found

@izharaazmi

This comment has been minimized.

Show comment
Hide comment
@izharaazmi

izharaazmi Jul 26, 2017

Contributor

@brianteeman Could not reproduce this anywhere. May be I'm missing somewhere. Can you please guide me the steps through it?

Contributor

izharaazmi commented Jul 26, 2017

@brianteeman Could not reproduce this anywhere. May be I'm missing somewhere. Can you please guide me the steps through it?

@brianteeman

This comment has been minimized.

Show comment
Hide comment
@brianteeman

brianteeman Jul 26, 2017

Contributor

I got it immediately after applying the patch will try again later

Contributor

brianteeman commented Jul 26, 2017

I got it immediately after applying the patch will try again later

@franz-wohlkoenig

This comment has been minimized.

Show comment
Hide comment
@franz-wohlkoenig

franz-wohlkoenig Jul 26, 2017

Got same Error as @brianteeman wrote.

franz-wohlkoenig commented Jul 26, 2017

Got same Error as @brianteeman wrote.

@izharaazmi

This comment has been minimized.

Show comment
Hide comment
@izharaazmi

izharaazmi Jul 27, 2017

Contributor

@brianteeman @franz-wohlkoenig There seems to be some issue with the patch tester applying this patch. Please use the source branch izharaazmi/joomla-cms/menu-preset-features.zip for testing.

I have updated the branch with the latest staging so there should be no conflicts.

Contributor

izharaazmi commented Jul 27, 2017

@brianteeman @franz-wohlkoenig There seems to be some issue with the patch tester applying this patch. Please use the source branch izharaazmi/joomla-cms/menu-preset-features.zip for testing.

I have updated the branch with the latest staging so there should be no conflicts.

@infograf768

This comment has been minimized.

Show comment
Hide comment
@infograf768

infograf768 Jul 27, 2017

Member

I have tested this item successfully on 39b96e1

Test:
Created a custom preset (mypreset.xml) in Isis by copying the Modern Preset.
Set a custom menu to use this custom preset.
Set the menu module to also load that preset (both the module and menutype should be set to use the preset).
I deleted in that custom preset all related to User.

Works fine here. 😃


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/16451.
Member

infograf768 commented Jul 27, 2017

I have tested this item successfully on 39b96e1

Test:
Created a custom preset (mypreset.xml) in Isis by copying the Modern Preset.
Set a custom menu to use this custom preset.
Set the menu module to also load that preset (both the module and menutype should be set to use the preset).
I deleted in that custom preset all related to User.

Works fine here. 😃


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/16451.
@izharaazmi

This comment has been minimized.

Show comment
Hide comment
@izharaazmi

izharaazmi Jul 28, 2017

Contributor

@brianteeman @franz-wohlkoenig Can you try again please! Running out of time for feature freeze.

Contributor

izharaazmi commented Jul 28, 2017

@brianteeman @franz-wohlkoenig Can you try again please! Running out of time for feature freeze.

@zafarnayab

This comment has been minimized.

Show comment
Hide comment
@zafarnayab

zafarnayab Jul 28, 2017

I have tested this item successfully on 39b96e1

Works as described.
Tested custom preset, custom menu, preset import. All good. 👍


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/16451.

zafarnayab commented Jul 28, 2017

I have tested this item successfully on 39b96e1

Works as described.
Tested custom preset, custom menu, preset import. All good. 👍


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/16451.

@franz-wohlkoenig

This comment has been minimized.

Show comment
Hide comment
@franz-wohlkoenig

franz-wohlkoenig Jul 28, 2017

RTC after two successful tests.

franz-wohlkoenig commented Jul 28, 2017

RTC after two successful tests.

@joomla-cms-bot joomla-cms-bot added the RTC label Jul 28, 2017

@izharaazmi

This comment has been minimized.

Show comment
Hide comment
@izharaazmi

izharaazmi Jul 29, 2017

Contributor
Contributor

izharaazmi commented Jul 29, 2017

@infograf768 infograf768 added this to the Joomla 3.8.0 milestone Jul 29, 2017

@infograf768

This comment has been minimized.

Show comment
Hide comment
@infograf768

infograf768 Jul 29, 2017

Member

@mbabker
Can we get this in for 3.8.0? Only a few hours late... ;)

Member

infograf768 commented Jul 29, 2017

@mbabker
Can we get this in for 3.8.0? Only a few hours late... ;)

@mbabker

Several file permissions are changed from 0644 to 0755. Those need to be fixed.

@izharaazmi

This comment has been minimized.

Show comment
Hide comment
@izharaazmi

izharaazmi Jul 29, 2017

Contributor
Contributor

izharaazmi commented Jul 29, 2017

hans2103 and others added some commits Jul 29, 2017

Add filter by Tag to mod_article_category (#16945)
* add filteroption to select tag to filter

* all php logic to get selected filter option

* add nice lines to separate fitler by tag from the rest.

* fix code style

* conditional load tags. Only load when filter is used
Merge branch 'staging' into menu-preset-features
* staging: (35 commits)
  Add filter by Tag to mod_article_category (#16945)
  Change links
  Implement component params for fieldgroups (#17317)
  Moved JLanguageMultilang::isAdminEnabled() to JModuleHelper::isAdminMultilang() (#17314)
  Fix tests
  Namespace sodium cipher, use compat API
  Fix namespace use and class casing
  Various doc block fixes and removing unneeded imports
  Class mappings for internal classes that apparently aren't internal
  Don't load unexisting paths
  Cleanup and optimization in FinderIndexerDrivers (#13511)
  Fix deleted files listing
  Deleted files updated
  Rename the document renderer base class
  Split feed data object classes to separate files
  Correct class name logic
  Deleted files for #17278
  Namespace feed (#17278)
  [CS] Required=true (#17313)
  required = false is nnot required (#17309)
  ...

@mbabker mbabker merged commit 2da3914 into joomla:staging Jul 31, 2017

3 of 5 checks passed

continuous-integration/jenkins/pr-merge This commit cannot be built
Details
continuous-integration/drone/pr this build is pending
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
hound No violations found. Woof!

@joomla-cms-bot joomla-cms-bot removed the RTC label Jul 31, 2017

@izharaazmi izharaazmi deleted the izharaazmi:menu-preset-features branch Aug 31, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment