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

Inheritable menu item property (take 2) #13817

Closed
wants to merge 20 commits into from
Closed

Inheritable menu item property (take 2) #13817

wants to merge 20 commits into from

Conversation

Napoleon-BlownApart
Copy link

I have implemented an Inheritable property for menu items. The property determines if higher access levels inherit the menu item. An inheritable menu item will display in the menu for all user groups who inherit visibility of the menu item's set access level. A menu item set to un-inheritable will only be seen by users who belong to the access level group that is set in the menu item.

For example:
With a "Register" menu link (whose access is set to Public with Inheritable NO), a public visitor to the site will be able to register, but once they have activated their account and been upgraded to the Registered user group, they will not see the "Register" link while they remain logged in (assuming the user is not assigned to the Public User Group on the User:Edit-Assigned User Groups screen).
Had the Inheritable property been left ON (default), the menu item's behaviour would have been standard Joomla behaviour, and the Registered user would still be able to see the "Register" link (unless a separte menu was created for registered users or some other hack was implemented).

This is a continuation of PR #13766

… Administrator management at the Menus:Edit Item level.
… Administrator management at the Menus:Items level.
@joomla-cms-bot joomla-cms-bot added Language Change This is for Translators PR-staging labels Jan 31, 2017
@Napoleon-BlownApart
Copy link
Author

Wow! Travis has had a change of mind.

@Napoleon-BlownApart
Copy link
Author

Testing Instructions

Setup

Install latest staging (that includes my Inheritable branch)
Enable users to register (or manually create the users; one public, one registered).
Set "Guest User Group" to Public in Global Configuration -> Users: Options -> User Options.
Create a menu with 3 items, Home, Register, Edit Profile.
Set the Home menu item to Public Access with _Inheritable_ YES (default).
Set the Register menu item to Public Access but with _Inheritable_ NO (either on the Menu:Item Edit or Menu:Items screens)
Set the Edit Profile menu item to Registered Access with Inheritable YES (default).

Test 1: Visibility of the Menu Item to users.

Test 1.1

Open a browser and visit the site (make sure you are not logged in).
In the menu, you will see menu items "Home" and "Register".

Test 1.2

Register on the site (or login with a registered user)
In the menu, you will only see menu items "Home" and "Edit Profile" ("Register" will not be shown).
Log out.
In the menu, you will see menu items "Home" and "Register".

Test 2: Managing the Inheritable as an Administrator

Test 2.1

On the Menu:Item Edit screen, change the Inheritable property then "Save & Close"
You will see in the Menu:Items list that the menu item reports the state of Inheritable according to your setting.

Test 2.2

On the Menu:Items screen, attempt to change the inheritable property of the DEFAULT page.
You should be presented with an error stating you cannot do this for the default page.

Test 2.3

Choose a non-Default menu item, set  it to un-published AND set it to un-inheritable.
Now choose that same item, on the Menu:Items screen, and make it the Default page.
You should see that this action makes the item both Published as well as Inheritable.

Note I have not implemented this check at the Menu:Item Edit screen level. If necessary, I will do that.

Test 3: Language settings

Change the text of the following language dependent variables according to your requirements. You will need to add these for languages other than en-GB.
Revisit the administrator pages to see your settings.

In administrator/language/en-GB/en-GB.com_menus.ini

COM_MENUS_ITEM_FIELD_INHERITABLE_DESC
COM_MENUS_ITEM_FIELD_INHERITABLE_LABEL
COM_MENUS_HTML_SETINHERITABLE_ITEM
COM_MENUS_HTML_UNSETINHERITABLE_ITEM
COM_MENUS_ITEMS_SET_INHERITABLE_0
COM_MENUS_ITEMS_SET_INHERITABLE_1
COM_MENUS_ITEMS_SET_INHERITABLE_MORE
COM_MENUS_ITEMS_UNSET_INHERITABLE

In _ administrator/language/en-GB/en-GB.ini_

JGRID_HEADING_INHERITABLE_ASC
JGRID_HEADING_INHERITABLE_DESC

In _ administrator/language/en-GB/en-GB.lib_joomla.ini_

JLIB_DATABASE_ERROR_MENU_UNINHETITABLE_DEFAULT_HOME

In _ language/en-GB/en-GB.lib_joomla.ini_

JLIB_DATABASE_ERROR_MENU_UNINHETITABLE_DEFAULT_HOME

@Napoleon-BlownApart
Copy link
Author

Napoleon-BlownApart commented Jan 31, 2017

@infograf768: I have changed the wording of COM_MENUS_ITEM_FIELD_INHERITABLE_DESC to:

"Determines if view access level for this menu item can be inherited."

Feedback on improvements are welcome.

@ghost
Copy link

ghost commented Jan 31, 2017

@Napoleon-BlownApart is your PR a Solution for #12010?

@Bakual
Copy link
Contributor

Bakual commented Jan 31, 2017

With a "Register" menu link (whose access is set to Public with Inheritable NO), a public visitor to the site will be able to register, but once they have activated their account and been upgraded to the Registered user group, they will not see the "Register" link while they remain logged in (assuming the user is not assigned to the Public User Group on the User:Edit-Assigned User Groups screen).

The example is a bit badly chosen as there is a "Guest" user group exactly for that purpose. So you can show things for visitors that are not logged in and hide it for logged in users.

Maybe you should choose another example which can't be solved with the current ACL possibilities already.

@Napoleon-BlownApart
Copy link
Author

@franz-wohlkoenig: no, it is not.
@Bakual:
The whole idea behind the Inheritable property is that you do not have to create Leaf Nodes in your ACL structure. Thus, with this feature, the Guest group becomes redundant for some, perhaps most, use cases. (Personally I see the need for a Guest group as a hack.) With the Inheritable proeprty, you will need to define far fewer View Access Levels and it will be much easier to understand the consequences of mappings between User Groups and View Access Levels and at the same time, easier to setup.
It also makes it much easier for people to setup their sites with proper/clean menu access.

I came up with this idea while I was trying to figure out the current ACL, which I consider very hard to learn to understand and inefficient for the kind of use cases I had in mind.

The main ideas are far easier learning curve, improved likelyhood that the configured ACL will reflect the intended ACL, easier to modify settings. Without sacrificing any of Joomla's ACL features.

I have an open thread in the Joomla Extension forum which I need to wrap up, where, thanks to WebDongle, Per Yngve Berg, and toivo, I finally understood what was happening under the ACL hood. In that thread, I will draw a comparison between the standard configuration and how it would be achieved using the Inheritable property. I will link to it from here.

@ghost
Copy link

ghost commented Jan 31, 2017

thanks for Info, @Napoleon-BlownApart

@Bakual
Copy link
Contributor

Bakual commented Jan 31, 2017

Thus, with this feature, the Guest group becomes redundant for some, perhaps most, use cases.

Only for menus as I understand it, all other cases (item visibility, module visibility, ACL in general) would still need it.
I don't see the guest group as a hack. It's a special setting to detect users that aren't logged in yes, but not a hack. It's quite a special case from an ACL point.

Personally I wouldn't allow to break up ACL inheritance. But that's a personal opinion. Looking forward for a more helpful example which doesn't involve the guest group.

@Napoleon-BlownApart
Copy link
Author

I am preparing a scenario to draw comparisons. I'm considering using Akeeba backup to provide demos, as well as screenshots and explanatory text. Any suggestions on how to make the presentation are welcome.

@Napoleon-BlownApart
Copy link
Author

(I will fix up the merge conficts next.)

I've prepared two implementations of the same scenario. Before I continue, I need to qualify that I am not an expert in Joomla ACL, let alone Joomla in general. I just cleared a path through a dense jungle. So I may not be the best person to argue the benefits of this feature, but here goes....

Feedback concerning my scenario set up are most welcome.

Justification

1 The current technique to provide non-inheritable User Group specific content (I will use the group extension -exclusive for this) requires the creation of additional child User Groups. Thus, up to twice as many User Groups need to be set up to facilitate this. In addition, up to twice the number of View Access Level needs to be set up to allow exclusive access.

2 Content assigned to the -exclusive is outside the normal hierarchical structure of the User Group inheritance, and thus needs to be configured separately. This is particularly relevant to Category access and permission granting. Category Lists do not show content that is separated out for exclusive access.

3 Since ACL is so important, the Inheritable attribute makes the task of organising and checking access and permissions much easier. The small and relatively simple scenario I set up was very complicated to achieve (and I would like feedback from those more familiar with Joomla ACL configuration) because of the dual User Group/Access Level problem. Using the Inheritable property, the task was greatly simplified, which ensured the quality of the outcome.

4 With fewer User Groups and Access Levels, there is a significant saving in the size of the database. You will notice that the Inheritable version of the Akeeba backup is more than 10MB smaller than the standard version. And this is for a very simple scenario.

5 The feature is transparent and has no effect on the Joomla configuration when the default setting of YES is used. Thus you can export/import the sql from one site to another and it will work after you add the Inheritable column to #__menu table. I used this property to simplify the populate of the Inheritable scenario, inside a basic Joomla installation.

6 I believe that Wordpress has won the website publishing (blog) race. I have been a supporter, advocate, and user of Joomla since around 2006. I have never used Wordpress but I see it in use all over the place when I google. I have studied the architecture of Drupal but never implemented a site with it. I think Joomla is a terrific CMS, but I think it's time Joomla moves beyond the author/editor/publisher paradigm. I believe Joomla filling the gap between Wordpress and Drupal. Joomla has a nice framework and is a lot easier to work with than Drupal, but much more sophisticated than Wordpress, as demonstrated by the large number of off-the-shelf applications that can be installed into Joomla (eg Virtumart). An object molded into shape is stronger than an object beaten into shape.

Scenario Details

I have prepared two Akeeba backups, one that implements the scenario using standard Joomla, and the other with the Inheritable modification. I have used Joomla 3.6.5 for this as I had some issues with the staging version.

The scenario is of a company with the following user types: public, registered customers, general employees, a sales department, and a warehousing department. Each of these groups has access to appropriate content (which sometimes overlaps). Image follow at the end below.

A detailed scenario description (text file) can be downloaded from https://www.abms.net.au/public/inheritable_comparison_scenario.txt
The scenario implemented in a vanilla 3.6.5 install using the standard Joomla approach can be downloaded from https://www.abms.net.au/public/site-jmfull.int-20170208-070014.jpa
The scenario implemented in a modified 3.6.5 install (to enable Inheritance) can be downloaded from https://www.abms.net.au/public/site-jminherit.int-20170208-070257.jpa

Sample images comparing the scenarios:
LHS/Above is standard, RHS/Below is with Inheritable.

  • Screen showing Employee Coordinator logged in and viewing the Employees link:
    Front End
  • Screen showing Menus: Items (Main Menu):
    Menus: Items (Main Menu)
  • Screen showing Users: Groups:
    Users: Groups
  • Screen showing User: View Access Levels:
    User: View Access Levels
  • Screen showing Users: (All passwords are test.)
    Users

@Bakual
Copy link
Contributor

Bakual commented Feb 8, 2017

@sanderpotjer Can you have a look at this proposal? You know the ACL system better than I do 😄

@Napoleon-BlownApart
Copy link
Author

@Bakual: I fixed the merge conflicts, but it's not working. When I pull the staging branch in my local repo it tells me I'm already up to date. However, I'm not getting the code that either caused the merge conflict, nor my resolution. How can I update my local staging branch so I can merge the changes and debug the resolution? Cheers.

@Bakual
Copy link
Contributor

Bakual commented Feb 9, 2017

What I usually do is that I have added the joomla/joomla-cms as a "Upstream" remote repo. Then I can merge the upstream staging into my local staging (fast forwarding) and then push to that to my origin.

This way your own origin is in sync with the one from here.

@Napoleon-BlownApart
Copy link
Author

Napoleon-BlownApart commented Feb 10, 2017

Travis and I are, again, in disagreement. Travis reports the following error, which I cannot replicate.

.....S.............................FPHP Fatal error:  Call to a member function getActive() on a non-object in /home/travis/build/joomla/joomla-cms/libraries/cms/application/site.php on line 323

Could someone please help me with this? Not a fix, but an indication on how to replicate the problem or otherwise. Cheers.

PS.. I have no idea how to interpret Appveyor's results, they don't seem to tell my anything.

@Napoleon-BlownApart
Copy link
Author

I think it is failing some test, but I can't workout which one, nor where the tests are.

@Napoleon-BlownApart
Copy link
Author

Looking at the output of Travis Job #33609 and expanding line 189, reveals that the inheritable field in jos_menu is not defined in line 522, where it should be (immediately after access). I don't understand how this could happen because the field is defined in all the the ./installation/sql/.../joomla.sql scripts for mysql, postgresql, and sqlazure. The update scripts in ./administrator/components/com_admin/sql/updates/.../3.7.0-2017-01-27.sql are also set up for mysql, postgresql, and sqlazure.

@infograf768
Copy link
Member

Many errors here:
Filtering by site or admin menus is lost.
There is a new filter for Published/Unpublished which has no effect.

We get
screen shot 2017-02-11 at 07 38 46

While we have now:
screen shot 2017-02-11 at 07 43 06

@Napoleon-BlownApart
Copy link
Author

@infograf768 : I have a feeling we are working with different builds. Here is the We Get screen that I see in my browser:
Inheritable_w_Site_Search_Selector.png

I merged upstream/staging into my staging and then into the inheritable branch with these commits:

commit f040f12a15a022414fac29813ebcd69b1567d354
Merge: e95df2d 0cc1876
Author: Dimitri Joukoff <Dimitri.Joukoff@GriffithUni.edu.au>
Date:   Fri Feb 10 15:23:16 2017 +1000

    Merge branch 'staging' into inheritable

commit 0cc18764e000a3192b044cd64a9cb263369c2deb
Merge: 639b8d7 88f80b7
Author: Dimitri Joukoff <Dimitri.Joukoff@GriffithUni.edu.au>
Date:   Fri Feb 10 15:11:55 2017 +1000

    Merge remote-tracking branch 'upstream/staging' into staging

Then I made the required changes and pushed back to origin/inheritable. This is where I'm up to with Travis.
I have placed the Full Inheritable package build (zip format), generated from my local repo, onto my server, which can be retrieved from: https://www.abms.net.au/public/Joomla_3.7.0-beta3-dev-Full_Inheritable_Package.zip And an Akeeba backup of the install, if that is more convenient, can be retrieved from: https://www.abms.net.au/public/site-jmrepo.int-20170211-091014.jpa

@Napoleon-BlownApart
Copy link
Author

@infograf768: When I merged upstream/staging into mine, somehow I ended up being 136 commits ahead of origin/staging. I expected the merge commit to handle this. (I did not use fast-forward when I did the merge.) I just pushed my local staging to origin/staging.

With the Search Tools selected, the "Unpublished" filter drop-down should be for filtering Inheritable/Un-inheritable menu items. I will fix that. Cheers.

@Napoleon-BlownApart
Copy link
Author

Napoleon-BlownApart commented Feb 11, 2017

@infograf768: I have fixed the erroneous Search Tools -> Unpublished drop-down. It now shows three options '- Select Inheritable -', 'Un-Inheritable', and 'Inheritable' using language settings, and the filter now works.

@Napoleon-BlownApart
Copy link
Author

TRAVIS..... You're FIRED!

@Napoleon-BlownApart
Copy link
Author

Napoleon-BlownApart commented Feb 13, 2017

But seriously, could someone please help me identify the problem Travis is complaining about. I believe it may have something to do with my local repo being messed up? Otherwise, how could the tests be creating the database without the required field. All the SQL files have been updated.

@zero-24
Copy link
Contributor

zero-24 commented Feb 13, 2017

@Napoleon-BlownApart it looks like there is a fatal error:

Fatal error: Call to a member function getActive() on a non-object in /home/travis/build/joomla/joomla-cms/libraries/cms/application/site.php on line 323

After this change?

@Napoleon-BlownApart
Copy link
Author

@zero-24 fair enough, and I could see that in the Travis report. But on my install here, I don't get that error, and others who've tested my proposal have not had that problem either. How do I fix a problem I can't replicate? Which test is failing? What is the call stack that lead to the error?
What I did notice in the Travis report (I explained above) is that the schema for the #__menu table is incorrect. There should be an additional column "inheritable". I don't understand how this can happen when both the Full package SQL and the Update SQL specify that column.

build/build.php Outdated
@@ -55,7 +55,8 @@

echo "Copy the files from the git repository.\n";
chdir($repo);
system($systemGit . ' archive ' . $fullVersion . ' | tar -x -C ' . $fullpath);
//system($systemGit . ' archive ' . $fullVersion . ' | tar -x -C ' . $fullpath);
system($systemGit . ' archive inheritable | tar -x -C ' . $fullpath);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason you pushed your changes in the build.php?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yvesh: because without it I cannot build the inheritable branch. and it is polluting my git status output. I take it from your question that this is not the right thing to do, so I will revert it back.

$this->inheritable = $state;
}

$this->setError('');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you set it empty here? Shouldn't be filled? :)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yvesh: I used function publish(), which is just above function setInheritable(), as a template and it too sets it to an empty string.

…anged excpet file ownership on some from root to my user id. I have installed composer and phpunit since the last commit and attempted to run the command 'phpunit . --debug --testdox'. I think git got confused somehow.
@joomla-cms-bot joomla-cms-bot added Unit/System Tests and removed Language Change This is for Translators labels Feb 16, 2017
@Napoleon-BlownApart
Copy link
Author

Napoleon-BlownApart commented Feb 16, 2017

This is all getting too complicated for someone new to the process and now I'm responsible for fixing merge conflicts in shockwave files (I'm blown away). I'm going to delete my fork and start again. I've learned a lot going through these steps and I think this will lead to a much cleaner proposal that will make it a lot easier for those interested/responsible to evaluate.

@infograf768
Copy link
Member

If you delete your fork, it may be a good idea to do a fully new PR and close this one.

@Napoleon-BlownApart
Copy link
Author

@infograf768 : Cheers. That's what I had in mind, but wasn't sure if that was a good idea. I have already backed up everything here.

@Napoleon-BlownApart
Copy link
Author

@infograf768 : Cheers. That's what I had in mind, but wasn't sure if that was a good idea. I have already backed up everything here.

I have created a new fork now, so I'm closing this PR. I'll create a new PR once I fix all the Travis issues.

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

Successfully merging this pull request may close these issues.

None yet

7 participants