Added IT and unit tests for add file to hold

rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/hold/HoldService.java

@@ -69,7 +69,7 @@ public interface HoldService
     /**
      * Gets the list of all the holds within the holds container for the given node reference
      *
-     * @param nodeRef The {@link NodeRef} of the record / record folder
+     * @param nodeRef The {@link NodeRef} of the record / record folder /active content
      * @param includedInHold <code>true</code> to retrieve the list of hold node references which will include the node reference
      * <code>false</code> to get a list of node references which will not have the given node reference
      * @return List of hold node references
@@ -119,10 +119,10 @@ public interface HoldService
     void deleteHold(NodeRef hold);
 
     /**
-     * Adds the record to the given hold
+     * Adds the item to the given hold
      *
      * @param hold    The {@link NodeRef} of the hold
-     * @param nodeRef The {@link NodeRef} of the record / record folder which will be added to the given hold
+     * @param nodeRef The {@link NodeRef} of the record / record folder / active content which will be added to the given hold
      */
     void addToHold(NodeRef hold, NodeRef nodeRef);
 
@@ -135,10 +135,10 @@ public interface HoldService
     void addToHold(NodeRef hold, List<NodeRef> nodeRefs);
 
     /**
-     * Adds the record to the given list of holds
+     * Adds the item to the given list of holds
      *
      * @param holds The list of {@link NodeRef}s of the holds
-     * @param nodeRef The {@link NodeRef} of the record / record folder which will be added to the given holds
+     * @param nodeRef The {@link NodeRef} of the record / record folder / active content which will be added to the given holds
      */
     void addToHolds(List<NodeRef> holds, NodeRef nodeRef);
 

rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/hold/HoldServiceImpl.java

@@ -303,7 +303,7 @@ public class HoldServiceImpl extends ServiceBaseImpl
         if (!includedInHold)
         {
             // invert list to get list of holds that do not contain this node
-            NodeRef filePlan = filePlanService.getFilePlan(nodeRef);
+            NodeRef filePlan = isFilePlanComponent(nodeRef) ? filePlanService.getFilePlan(nodeRef) : filePlanService.getFilePlanBySiteId(FilePlanService.DEFAULT_RM_SITE_ID);
             List<NodeRef> allHolds = getHolds(filePlan);
             result = ListUtils.subtract(allHolds, new ArrayList<>(holdsNotIncludingNodeRef));
         }

rm-community/rm-community-repo/test/java/org/alfresco/module/org_alfresco_module_rm/test/integration/hold/AddActiveContentToHoldTest.java

@@ -0,0 +1,242 @@
+/*
+ * #%L
+ * Alfresco Records Management Module
+ * %%
+ * Copyright (C) 2005 - 2019 Alfresco Software Limited
+ * %%
+ * This file is part of the Alfresco software.
+ * -
+ * If the software was purchased under a paid Alfresco license, the terms of
+ * the paid license agreement will prevail.  Otherwise, the software is
+ * provided under the following open source license terms:
+ * -
+ * Alfresco is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Lesser General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ * -
+ * Alfresco is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Lesser General Public License for more details.
+ * -
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with Alfresco. If not, see <http://www.gnu.org/licenses/>.
+ * #L%
+ */
+package org.alfresco.module.org_alfresco_module_rm.test.integration.hold;
+
+import org.alfresco.error.AlfrescoRuntimeException;
+import org.alfresco.module.org_alfresco_module_rm.test.util.BaseRMTestCase;
+import org.alfresco.repo.security.authentication.AuthenticationUtil;
+import org.alfresco.repo.security.authentication.AuthenticationUtil.RunAsWork;
+import org.alfresco.repo.security.permissions.AccessDeniedException;
+import org.alfresco.repo.site.SiteModel;
+import org.alfresco.service.cmr.repository.NodeRef;
+import org.springframework.extensions.webscripts.GUID;
+
+/**
+ * Add Active Content To Hold Integration Tests
+ *
+ * @author Claudia Agache
+ * @since 3.2
+ */
+public class AddActiveContentToHoldTest extends BaseRMTestCase
+{
+    @Override
+    protected boolean isCollaborationSiteTest()
+    {
+        return true;
+    }
+
+    @Override
+    protected boolean isUserTest()
+    {
+        return true;
+    }
+
+    public void testAddDocumentToHold()
+    {
+        doBehaviourDrivenTest(new BehaviourDrivenTest()
+        {
+            private NodeRef hold;
+
+            public void given()
+            {
+                // Check that the document is not a record
+                assertFalse("The document should not be a record", recordService.isRecord(dmDocument));
+
+                // create a hold
+                hold = holdService.createHold(filePlan, GUID.generate(), GUID.generate(), GUID.generate());
+
+                // assert current states
+                assertFalse(freezeService.isFrozen(dmDocument));
+                assertFalse(freezeService.hasFrozenChildren(dmFolder));
+
+                // additional check for child held caching
+                assertFalse(nodeService.hasAspect(dmFolder, ASPECT_HELD_CHILDREN));
+            }
+
+            public void when()
+            {
+                // add the active content to hold
+                holdService.addToHold(hold, dmDocument);
+            }
+
+            public void then()
+            {
+                // active content is held
+                assertTrue(freezeService.isFrozen(dmDocument));
+
+                // collaboration folder has frozen children
+                assertFalse(freezeService.isFrozen(dmFolder));
+                assertTrue(freezeService.hasFrozenChildren(dmFolder));
+
+                // collaboration folder is not held
+                assertFalse(holdService.getHeld(hold).contains(dmFolder));
+                assertFalse(holdService.heldBy(dmFolder, true).contains(hold));
+
+                // hold contains active content
+                assertTrue(holdService.getHeld(hold).contains(dmDocument));
+                assertTrue(holdService.heldBy(dmDocument, true).contains(hold));
+
+                // additional check for child held caching
+                assertTrue(nodeService.hasAspect(dmFolder, ASPECT_HELD_CHILDREN));
+                assertEquals(1, nodeService.getProperty(dmFolder, PROP_HELD_CHILDREN_COUNT));
+            }
+        });
+    }
+
+    public void testAddDocumentToHoldAsNonRMUser()
+    {
+        doBehaviourDrivenTest(new BehaviourDrivenTest(AccessDeniedException.class)
+        {
+            private NodeRef hold;
+
+            public void given()
+            {
+                // Check that the document is not a record
+                assertFalse("The document should not be a record", recordService.isRecord(dmDocument));
+
+                // create a hold
+                hold = holdService.createHold(filePlan, GUID.generate(), GUID.generate(), GUID.generate());
+
+                // assert current states
+                assertFalse(freezeService.isFrozen(dmDocument));
+            }
+
+            public void when()
+            {
+                // add the active content to hold as a non RM user
+                AuthenticationUtil.runAs(
+                        (RunAsWork<Void>) () -> {
+                            holdService.addToHold(hold, dmDocument);
+                            return null;
+                        }, dmCollaborator);
+            }
+        });
+    }
+
+    public void testAddDocumentToHoldNoWritePermissionOnDoc()
+    {
+        doBehaviourDrivenTest(new BehaviourDrivenTest(AlfrescoRuntimeException.class)
+        {
+            private NodeRef hold;
+
+            public void given()
+            {
+                // Check that the document is not a record
+                assertFalse("The document should not be a record", recordService.isRecord(dmDocument));
+
+                // create a hold
+                hold = holdService.createHold(filePlan, GUID.generate(), GUID.generate(), GUID.generate());
+
+                // assert current states
+                assertFalse(freezeService.isFrozen(dmDocument));
+            }
+
+            public void when()
+            {
+                // add the active content to hold as a RM admin who has Add to Hold Capability and filing permission on
+                // hold, but no Write permissions on doc
+                AuthenticationUtil.runAs(
+                        (RunAsWork<Void>) () -> {
+                            holdService.addToHold(hold, dmDocument);
+                            return null;
+                        }, rmAdminName);
+            }
+        });
+    }
+
+    public void testAddDocumentToHoldNoFilingPermissionOnHold()
+    {
+        doBehaviourDrivenTest(new BehaviourDrivenTest(AlfrescoRuntimeException.class)
+        {
+            private NodeRef hold;
+
+            public void given()
+            {
+                // Check that the document is not a record
+                assertFalse("The document should not be a record", recordService.isRecord(dmDocument));
+
+                // create a hold
+                hold = holdService.createHold(filePlan, GUID.generate(), GUID.generate(), GUID.generate());
+
+                // assert current states
+                assertFalse(freezeService.isFrozen(dmDocument));
+
+                //add recordsManagerPerson as manager in collaboration site to have write permissions on dmDocument
+                siteService.setMembership(collabSiteId, recordsManagerName, SiteModel.SITE_MANAGER);
+            }
+
+            public void when()
+            {
+                // add the active content to hold as a RM manager who has Add to Hold Capability and write permission on
+                // doc, but no filing permission on hold
+                AuthenticationUtil.runAs(
+                        (RunAsWork<Void>) () -> {
+                            holdService.addToHold(hold, dmDocument);
+                            return null;
+                        }, recordsManagerName);
+            }
+        });
+    }
+
+
+    public void testAddDocumentToHoldNoCapability()
+    {
+        doBehaviourDrivenTest(new BehaviourDrivenTest(AlfrescoRuntimeException.class)
+        {
+            private NodeRef hold;
+
+            public void given()
+            {
+                // Check that the document is not a record
+                assertFalse("The document should not be a record", recordService.isRecord(dmDocument));
+
+                // create a hold
+                hold = holdService.createHold(filePlan, GUID.generate(), GUID.generate(), GUID.generate());
+
+                // assert current states
+                assertFalse(freezeService.isFrozen(dmDocument));
+
+                //add powerUserPerson as manager in collaboration site to have write permissions on dmDocument
+                siteService.setMembership(collabSiteId, powerUserName, SiteModel.SITE_MANAGER);
+
+                //assign powerUserPerson filing permission on hold
+                filePlanPermissionService.setPermission(hold, powerUserName, FILING);
+            }
+
+            public void when()
+            {
+                // add the active content to hold as a RM power user who has write permission on doc and filing
+                // permission on hold, but no Add To Hold capability
+                AuthenticationUtil.runAs(
+                        (RunAsWork<Void>) () -> {
+                            holdService.addToHold(hold, dmDocument);
+                            return null;
+                        }, powerUserName);
+            }
+        });
+    }
+}

rm-community/rm-community-repo/test/java/org/alfresco/module/org_alfresco_module_rm/test/integration/hold/AddRemoveFromHoldTest.java

@@ -45,64 +45,6 @@ public class AddRemoveFromHoldTest extends BaseRMTestCase
 {
     private static final int RECORD_COUNT = 10;
 
-    @Override
-    protected boolean isCollaborationSiteTest()
-    {
-        return true;
-    }
-
-    public void testAddActiveContentToHold()
-    {
-        doBehaviourDrivenTest(new BehaviourDrivenTest()
-        {
-            private NodeRef hold;
-
-            public void given()
-            {
-                // Check that the document is not a record
-                assertFalse("The document should not be a record", recordService.isRecord(dmDocument));
-
-                // create a hold
-                hold = holdService.createHold(filePlan, GUID.generate(), GUID.generate(), GUID.generate());
-
-                // assert current states
-                assertFalse(freezeService.isFrozen(dmDocument));
-                assertFalse(freezeService.hasFrozenChildren(dmFolder));
-
-                // additional check for child held caching
-                assertFalse(nodeService.hasAspect(dmFolder, ASPECT_HELD_CHILDREN));
-            }
-
-            public void when() throws Exception
-            {
-                // add the active content to hold
-                holdService.addToHold(hold, dmDocument);
-            }
-
-            public void then()
-            {
-                // active content is held
-                assertTrue(freezeService.isFrozen(dmDocument));
-
-                // collaboration folder has frozen children
-                assertFalse(freezeService.isFrozen(dmFolder));
-                assertTrue(freezeService.hasFrozenChildren(dmFolder));
-
-                // collaboration folder is not held
-                assertFalse(holdService.getHeld(hold).contains(dmFolder));
-                assertFalse(holdService.heldBy(dmFolder, true).contains(hold));
-
-                // hold contains active content
-                assertTrue(holdService.getHeld(hold).contains(dmDocument));
-                assertTrue(holdService.heldBy(dmDocument, true).contains(hold));
-
-                // additional check for child held caching
-                assertTrue(nodeService.hasAspect(dmFolder, ASPECT_HELD_CHILDREN));
-                assertEquals(1, nodeService.getProperty(dmFolder, PROP_HELD_CHILDREN_COUNT));
-            }
-        });
-    }
-
     public void testAddRecordToHold()
     { 
         doBehaviourDrivenTest(new BehaviourDrivenTest()

rm-community/rm-community-repo/test/java/org/alfresco/module/org_alfresco_module_rm/test/util/BaseRMTestCase.java

@@ -253,6 +253,7 @@ public abstract class BaseRMTestCase extends RetryingTransactionHelperTestCase
     protected String powerUserName;
     protected String securityOfficerName;
     protected String recordsManagerName;
+    protected String rmAdminName;
 
     /** test people */
     protected NodeRef userPerson;
@@ -260,6 +261,7 @@ public abstract class BaseRMTestCase extends RetryingTransactionHelperTestCase
     protected NodeRef powerUserPerson;
     protected NodeRef securityOfficerPerson;
     protected NodeRef recordsManagerPerson;
+    protected NodeRef rmAdminPerson;
 
     /** test records */
     protected NodeRef recordOne;
@@ -656,13 +658,18 @@ public abstract class BaseRMTestCase extends RetryingTransactionHelperTestCase
         recordsManagerPerson = createPerson(recordsManagerName);
         filePlanRoleService.assignRoleToAuthority(filePlan, FilePlanRoleService.ROLE_RECORDS_MANAGER, recordsManagerName);
 
+        rmAdminName = GUID.generate();
+        rmAdminPerson = createPerson(rmAdminName);
+        filePlanRoleService.assignRoleToAuthority(filePlan, FilePlanRoleService.ROLE_ADMIN, rmAdminName);
+
         testUsers = new String[]
         {
                 userName,
                 rmUserName,
                 powerUserName,
                 securityOfficerName,
-                recordsManagerName
+                recordsManagerName,
+                rmAdminName
         };
 
         if (isFillingForAllUsers())

rm-community/rm-community-repo/unit-test/java/org/alfresco/module/org_alfresco_module_rm/hold/HoldServiceImplUnitTest.java

@@ -54,10 +54,12 @@ import java.util.Map;
 import org.alfresco.error.AlfrescoRuntimeException;
 import org.alfresco.model.ContentModel;
 import org.alfresco.module.org_alfresco_module_rm.capability.RMPermissionModel;
+import org.alfresco.module.org_alfresco_module_rm.fileplan.FilePlanService;
 import org.alfresco.module.org_alfresco_module_rm.test.util.BaseUnitTest;
 import org.alfresco.service.cmr.repository.ChildAssociationRef;
 import org.alfresco.service.cmr.repository.NodeRef;
 import org.alfresco.service.cmr.security.AccessStatus;
+import org.alfresco.service.cmr.security.PermissionService;
 import org.alfresco.service.namespace.NamespaceService;
 import org.alfresco.service.namespace.QName;
 import org.alfresco.service.namespace.RegexQNamePattern;
@@ -107,6 +109,7 @@ public class HoldServiceImplUnitTest extends BaseUnitTest
 
         // setup interactions
         doReturn(holdContainer).when(mockedFilePlanService).getHoldContainer(filePlan);
+        doReturn(filePlan).when(mockedFilePlanService).getFilePlanBySiteId(FilePlanService.DEFAULT_RM_SITE_ID);
     }
 
     @Test
@@ -125,6 +128,9 @@ public class HoldServiceImplUnitTest extends BaseUnitTest
         holds.add(new ChildAssociationRef(ASSOC_FROZEN_RECORDS, hold2, ASSOC_FROZEN_RECORDS, recordFolder, true, 2));
         doReturn(holds).when(mockedNodeService).getParentAssocs(recordFolder, ASSOC_FROZEN_RECORDS, ASSOC_FROZEN_RECORDS);
 
+        //setup active content in multiple holds
+        doReturn(holds).when(mockedNodeService).getParentAssocs(activeContent, ASSOC_FROZEN_RECORDS, ASSOC_FROZEN_RECORDS);
+
         // check that both holds are found for record folder
         List<NodeRef> heldByHolds = holdService.heldBy(recordFolder, true);
         assertNotNull(heldByHolds);
@@ -136,6 +142,12 @@ public class HoldServiceImplUnitTest extends BaseUnitTest
         assertNotNull(heldByHolds);
         assertEquals(2, heldByHolds.size());
         assertTrue(holdService.heldBy(record, false).isEmpty());
+
+        // check that both holds are found for active content
+        heldByHolds = holdService.heldBy(activeContent, true);
+        assertNotNull(heldByHolds);
+        assertEquals(2, heldByHolds.size());
+        assertTrue(holdService.heldBy(activeContent, false).isEmpty());
     }
 
     @Test (expected=AlfrescoRuntimeException.class)
@@ -371,6 +383,13 @@ public class HoldServiceImplUnitTest extends BaseUnitTest
         holdService.addToHold(hold, activeContent);
     }
 
+    @Test (expected = AlfrescoRuntimeException.class)
+    public void addActiveContentToHoldNoPermissionsOnContent()
+    {
+        when(mockedPermissionService.hasPermission(activeContent, PermissionService.WRITE)).thenReturn(AccessStatus.DENIED);
+        holdService.addToHold(hold, activeContent);
+    }
+
     @SuppressWarnings("unchecked")
     @Test
     public void addToHolds()