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

Fix missing lft rgt nested tree rebuild in usergroups table when deleting usergroup(s) #28684

Conversation

richard67
Copy link
Member

@richard67 richard67 commented Apr 14, 2020

Pull Request for Issue #28522 (comment).

Summary of Changes

When deleting one or more usergroups, the lftand rgt values in the #__usergroups table are not recalculated, i.e. the nested set tree is not rebuilt.

This PR adds the missing function call for that purpose to the delete routine of the Usergroup table class.

Testing Instructions

  1. On a clean, new installation, check with an SQL client (e.g. phpMyAdmin when using MySQL or MariaDB) the content of the #__usergroups as follows:
    j3_usergroup-delete-manager_table-before

  2. In backend as a super user and not being member of one of these groups, delete the usergroup "Manager", which will also delete its child group "Administrator". (You can also delete others but then it's on you to calculate the right lftand rgt values to check if this PR is right.)

  3. Check again with an SQL client the content of the #__usergroups with the same SQL query as before.

Result: lftand rgt values in the #__usergroups table haven't changed for the remaining records:
j3_usergroup-delete-manager_table-after-wrong

  1. If you want to delete in the next test the same user group as in the test before: Make a new installation.

  2. Apply the patch.

  3. Repeat steps 2 and 3.

Result: lftand rgt values in the #__usergroups table have been recalculated. If you have deleted the group as described above, it should look as follows:
j3_usergroup-delete-manager_table-after-right

The calculation scheme is as follows for that example:
usergroup-tree-calculation

Expected result

The lftand rgt values in the #__usergroups table are correctly recalculated.

Actual result

The lftand rgt values in the #__usergroups table are not recalculated.

Documentation Changes Required

None.

Additional information

When inserting new user groups or moving some around between parent groups, the lftand rgt values in the #__usergroups table are recalculated without this patch using the same function, so I assume the lftand rgt values are still relevant.

@richard67 richard67 marked this pull request as ready for review April 14, 2020 17:05
@chmst
Copy link
Contributor

chmst commented Apr 14, 2020

I have tested this item ✅ successfully on 299f4f7


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

1 similar comment
@jwaisner
Copy link
Member

I have tested this item ✅ successfully on 299f4f7


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

@jwaisner
Copy link
Member

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Apr 14, 2020
@richard67
Copy link
Member Author

I can’t believe it, but when looking through the history of the file changed by this PR, it seems to me that what is added by this PR always has been missing.

@richard67
Copy link
Member Author

Maybe it should have been done the other way, remove it completely because for the groups hierarchy it is enough to know the parent group?

@HLeithner HLeithner merged commit 90be80c into joomla:staging Apr 15, 2020
@HLeithner
Copy link
Member

Thanks

@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Apr 15, 2020
@HLeithner HLeithner added this to the Joomla! 3.9.17 milestone Apr 15, 2020
@richard67 richard67 deleted the staging-fix-missing-tree-rebuild-when-deleting-usergroup branch April 15, 2020 08:48
@richard67
Copy link
Member Author

Thanks to testers and mergers ;-)

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

Successfully merging this pull request may close these issues.

None yet

5 participants