[MGNLUI-3062] Deleting not empty folders in groups and roles does not work Created: 15/Jul/14  Updated: 05/Dec/14  Resolved: 11/Aug/14

Status: Closed
Project: Magnolia UI
Component/s: security app
Affects Version/s: 5.3.1
Fix Version/s: 5.2.9, 5.3.2

Type: Bug Priority: Major
Reporter: Karel de Witte Assignee: Evzen Fochr
Resolution: Fixed Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Cloners
is cloned by MGNLUI-3093 Improve UI for deleting folders, grou... Closed
causality
is causing MGNLUI-3286 Deleting folder of groups or roles do... Closed
relation
is related to MGNLUI-2635 Deleting user/group/role on author do... Closed
Template:
Acceptance criteria:
Empty
Task DoD:
[ ]* Doc/release notes changes? Comment present?
[ ]* Downstream builds green?
[ ]* Solution information and context easily available?
[ ]* Tests
[ ]* FixVersion filled and not yet released
[ ]  Architecture Decision Record (ADR)
Bug DoR:
[ ]* Steps to reproduce, expected, and actual results filled
[ ]* Affected version filled
Date of First Response:

 Description   

Go to Security -> Groups
1. Select non empty folder
2. "Delete Folder"
3. Confirmation dialog: "Delete this folder? The folder and all its sub-items will be marked for deletion."
4. Press "YES - delete"
5. Error: Deletion has failed. /wcmstestkopie: info.magnolia.ui.api.action.ActionExecutionException: Folder is not empty

Please see MGNLUI-2635 and SUPPORT-3049



 Comments   
Comment by Mikaël Geljić [ 29/Jul/14 ]

Simply disabling the action makes it hard for the user to find out, when exactly he/she can delete an item. We should rather notify the user, say why we deny the delete.

For single groups (resp. roles), if I attempt to delete a group which is still in use, I get a warning alert:
"This group still contains users. Please empty the group first. [ OK ]"

Likewise for group folders (resp. role folders). I get a warning alert:
"This folder contains groups which are still in use. Please make sure no users are assigned to these first. [ OK ]"

If the folder contains only unused items, then we get the confirmation dialog as usual.

Comment by Milan Divilek [ 08/Aug/14 ]

Reopen: It looks good, just few thinks to test info.magnolia.security.app.action.DeleteFolderActionTest.

1. #testFolderIsNotDeletedIfGroupInUse - line 164

actionGroups = new DeleteFolderAction(definition, itemRoles, commandsManager, eventBus, uiContext, i18n, securitySupport);

itemGroups should be used instead of itemRoles

2. In #setUp() you are setting two different info.magnolia.cms.security.SecuritySupport instances.

securitySupport = mock(SecuritySupport.class);
ComponentsTestUtil.setImplementation(SecuritySupport.class, SecuritySupportImpl.class);

This then can easily caused problems during tests. For example try run this test. TestFolder will not be removed, but it should.

    @Test
    public void testFolderIsDeletedWhenContainsGroupAndGroupIsNotUsed() throws Exception {
        // GIVEN
        MgnlGroupManager mgnlGroupManager = new MgnlGroupManager() {
            @Override
            protected void validateGroupName(String name) throws AccessDeniedException {
                //not needed for test
            }
        };

        mgnlGroupManager.createGroup("/testFolder", "testGroup1");

        ((SecuritySupportImpl) Security.getSecuritySupport()).setGroupManager(mgnlGroupManager);

        ((SecuritySupportImpl) Security.getSecuritySupport()).setUserManagers(new HashMap<String, UserManager>());

        actionGroups = new DeleteFolderAction(definition, itemGroups, commandsManager, eventBus, uiContext, i18n, securitySupport);

        // WHEN
        actionGroups.execute();

        // THEN
        assertTrue(!sessionGroups.itemExists("/testFolder"));
    }

Fix this just simply by doing this in #setup()

ComponentsTestUtil.setImplementation(SecuritySupport.class, SecuritySupportImpl.class);
securitySupport = Components.getComponent(SecuritySupport.class);

3. in tests you are using deprecated Security class for getting SecuritySuppor instance. Use global securitySupport property(which you have already defined private SecuritySupport securitySupport) instead of it.

Comment by Evzen Fochr [ 11/Aug/14 ]

http://git.magnolia-cms.com/gitweb/?p=magnolia_ui.pub.git;a=commit;h=28586755ac7cd4a11f1becbc1170dc62315abff9

Comment by Aleksandr Pchelintcev [ 11/Aug/14 ]
  • In DeleteFolderActionTest#setUp, cumbersome structure like
        Node exportModuleDef = 
    	configSession.getRootNode().addNode("modules", NodeTypes.ContentNode.NAME).addNode("commands", NodeTypes.ContentNode.NAME)
                    .addNode("default", NodeTypes.ContentNode.NAME).addNode("delete", NodeTypes.ContentNode.NAME);
    

    Can be substituted with much simpler

    Node exportModuleDef = NodeUtil.createPath(configSession.getRootNode(), "modules/commands/default/delete", NodeTypes.ContentNode.NAME);
    
  • In DeleteFolderActionTest#testFolderIsNotDeletedIfGroupInUse - un-used variable g1
  • In DeleteFolderActionTest#testFolderIsNotDeletedIfRoleInUse - un-used variable r
  • I've noticed you're creating the overriden MgnlGroupManager several times the same way:
     MgnlGroupManager mgnlGroupManager = new MgnlGroupManager() {
                @Override
                protected void validateGroupName(String name) throws AccessDeniedException {
                    //not needed for test
                }
            };
    

    Wouldn't it make sense to create a static class and save some lines?

Comment by Evzen Fochr [ 11/Aug/14 ]

http://git.magnolia-cms.com/gitweb/?p=magnolia_ui.pub.git;a=commit;h=1fccd8c7ae7f85b9655f8cf6e4d4b62f9061b504

Generated at Mon Feb 12 09:02:51 CET 2024 using Jira 9.4.2#940002-sha1:46d1a51de284217efdcb32434eab47a99af2938b.