Merge branch 'feature/RM-6872_AddActiveContentToHold' into 'master'

RM-6872 Add active content to hold

See merge request records-management/records-management!1209

rm-community/rm-community-repo/config/alfresco/module/org_alfresco_module_rm/messages/hold-service.properties

@@ -0,0 +1,4 @@
+rm.hold.not-hold=The node {0} is not a hold.
+rm.hold.add-to-hold-invalid-type={0} is neither a record nor a record folder nor active content. Only records, record \
+  folders or active content can be added to a hold.
+rm.hold.add-to-hold-archived-node=Archived nodes can't be added to hold.

rm-community/rm-community-repo/config/alfresco/module/org_alfresco_module_rm/module-context.xml

@@ -70,6 +70,7 @@
             <value>alfresco.module.org_alfresco_module_rm.messages.dataset-service</value>
             <value>alfresco.module.org_alfresco_module_rm.messages.rm-system</value>
             <value>alfresco.module.org_alfresco_module_rm.messages.template</value>
+            <value>alfresco.module.org_alfresco_module_rm.messages.hold-service</value>
          </list>
       </property>
    </bean>

rm-community/rm-community-repo/config/alfresco/module/org_alfresco_module_rm/rm-service-context.xml

@@ -1563,7 +1563,7 @@
             org.alfresco.module.org_alfresco_module_rm.hold.HoldService.isHold=RM_ALLOW
             org.alfresco.module.org_alfresco_module_rm.hold.HoldService.getHolds=RM.Read.0,AFTER_RM.FilterNode
             org.alfresco.module.org_alfresco_module_rm.hold.HoldService.getHold=RM.Read.0,AFTER_RM.FilterNode
-            org.alfresco.module.org_alfresco_module_rm.hold.HoldService.heldBy=RM.Read.0,AFTER_RM.FilterNode
+            org.alfresco.module.org_alfresco_module_rm.hold.HoldService.heldBy=ACL_NODE.0.sys:base.Read,RM.Read.0,AFTER_RM.FilterNode
             org.alfresco.module.org_alfresco_module_rm.hold.HoldService.getHeld=RM.Read.0,AFTER_RM.FilterNode
             org.alfresco.module.org_alfresco_module_rm.hold.HoldService.createHold=RM_CAP.0.rma:filePlanComponent.CreateHold
             org.alfresco.module.org_alfresco_module_rm.hold.HoldService.getHoldReason=RM.Read.0

rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/freeze/FreezeServiceImpl.java

@@ -28,6 +28,9 @@
 
 package org.alfresco.module.org_alfresco_module_rm.freeze;
 
+import static org.alfresco.model.ContentModel.TYPE_FOLDER;
+import static org.alfresco.repo.site.SiteModel.ASPECT_SITE_CONTAINER;
+
 import java.io.Serializable;
 import java.util.ArrayList;
 import java.util.Date;
@@ -168,7 +171,7 @@ public class FreezeServiceImpl extends    ServiceBaseImpl
         NodeRef hold = null;
         if (!nodeRefs.isEmpty())
         {
-            List<NodeRef> list = new ArrayList<>(nodeRefs);
+            final List<NodeRef> list = new ArrayList<>(nodeRefs);
             hold = createHold(list.get(0), reason);
             getHoldService().addToHold(hold, list);
         }
@@ -273,8 +276,9 @@ public class FreezeServiceImpl extends    ServiceBaseImpl
 
         boolean result = false;
 
-        // check that we are dealing with a record folder
+        // check that we are dealing with a record folder or a collaboration folder
-        if (isRecordFolder(nodeRef))
+        if (isRecordFolder(nodeRef) ||
+                (instanceOf(nodeRef, TYPE_FOLDER) && !nodeService.hasAspect(nodeRef, ASPECT_SITE_CONTAINER)))
         {
             int heldCount = 0;
 
@@ -303,8 +307,8 @@ public class FreezeServiceImpl extends    ServiceBaseImpl
                                 {
                                     for (ChildAssociationRef childAssociationRef : childAssocs)
                                     {
-                                        NodeRef record = childAssociationRef.getChildRef();
+                                        final NodeRef childRef = childAssociationRef.getChildRef();
-                                        if (childAssociationRef.isPrimary() && isRecord(record) && isFrozen(record))
+                                        if (childAssociationRef.isPrimary() && isFrozen(childRef))
                                         {
                                             heldCount ++;
                                         }

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 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

@@ -48,6 +48,7 @@ import org.alfresco.module.org_alfresco_module_rm.record.RecordService;
 import org.alfresco.module.org_alfresco_module_rm.recordfolder.RecordFolderService;
 import org.alfresco.module.org_alfresco_module_rm.util.ServiceBaseImpl;
 import org.alfresco.repo.node.NodeServicePolicies;
+import org.alfresco.repo.node.integrity.IntegrityException;
 import org.alfresco.repo.policy.Behaviour.NotificationFrequency;
 import org.alfresco.repo.policy.annotation.Behaviour;
 import org.alfresco.repo.policy.annotation.BehaviourBean;
@@ -64,9 +65,11 @@ import org.alfresco.service.namespace.NamespaceService;
 import org.alfresco.service.namespace.QName;
 import org.alfresco.service.namespace.RegexQNamePattern;
 import org.alfresco.util.ParameterCheck;
+import org.apache.commons.collections.CollectionUtils;
 import org.apache.commons.collections.ListUtils;
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
+import org.springframework.extensions.surf.util.I18NUtil;
 
 /**
  * Hold service implementation
@@ -87,6 +90,9 @@ public class HoldServiceImpl extends ServiceBaseImpl
     private static final String AUDIT_ADD_TO_HOLD = "addToHold";
     private static final String AUDIT_REMOVE_FROM_HOLD = "removeFromHold";
 
+    /** I18N */
+    private static final String MSG_ERR_ACCESS_DENIED = "permissions.err_access_denied";
+
     /** File Plan Service */
     private FilePlanService filePlanService;
 
@@ -285,31 +291,39 @@ public class HoldServiceImpl extends ServiceBaseImpl
     {
         ParameterCheck.mandatory("nodeRef", nodeRef);
 
-        List<NodeRef> result = null;
+        List<NodeRef> result = new ArrayList<>();
 
         // get all the immediate parent holds
-        Set<NodeRef> holdsNotIncludingNodeRef = getParentHolds(nodeRef);
+        final Set<NodeRef> holdsIncludingNodeRef = getParentHolds(nodeRef);
 
-        // check whether the record is held by vitue of it's record folder
+        // check whether the record is held by virtue of it's record folder
         if (isRecord(nodeRef))
         {
-            List<NodeRef> recordFolders = recordFolderService.getRecordFolders(nodeRef);
+            final List<NodeRef> recordFolders = recordFolderService.getRecordFolders(nodeRef);
-            for (NodeRef recordFolder : recordFolders)
+            for (final NodeRef recordFolder : recordFolders)
             {
-                holdsNotIncludingNodeRef.addAll(getParentHolds(recordFolder));
+                holdsIncludingNodeRef.addAll(getParentHolds(recordFolder));
             }
         }
 
         if (!includedInHold)
         {
-            // invert list to get list of holds that do not contain this node
+            final Set<NodeRef> filePlans = filePlanService.getFilePlans();
-            NodeRef filePlan = filePlanService.getFilePlan(nodeRef);
+            if (!CollectionUtils.isEmpty(filePlans))
-            List<NodeRef> allHolds = getHolds(filePlan);
+            {
-            result = ListUtils.subtract(allHolds, new ArrayList<>(holdsNotIncludingNodeRef));
+                final List<NodeRef> holdsNotIncludingNodeRef = new ArrayList<>();
+                filePlans.forEach(filePlan ->
+                {
+                    // invert list to get list of holds that do not contain this node
+                    final List<NodeRef> allHolds = getHolds(filePlan);
+                    holdsNotIncludingNodeRef.addAll(ListUtils.subtract(allHolds, new ArrayList<>(holdsIncludingNodeRef)));
+                });
+                result = holdsNotIncludingNodeRef;
+            }
         }
         else
         {
-            result = new ArrayList<>(holdsNotIncludingNodeRef);
+            result = new ArrayList<>(holdsIncludingNodeRef);
         }
 
         return result;
@@ -539,94 +553,101 @@ public class HoldServiceImpl extends ServiceBaseImpl
         ParameterCheck.mandatoryCollection("holds", holds);
         ParameterCheck.mandatory("nodeRef", nodeRef);
 
-        if (!isRecord(nodeRef) && !isRecordFolder(nodeRef))
+        checkNodeCanBeAddedToHold(nodeRef);
-        {
-            String nodeName = (String) nodeService.getProperty(nodeRef, ContentModel.PROP_NAME);
-            throw new AlfrescoRuntimeException("'" + nodeName + "' is neither a record nor a record folder. Only records or record folders can be added to a hold.");
-        }
-
-        if (permissionService.hasPermission(nodeRef, RMPermissionModel.FILING) == AccessStatus.DENIED)
-        {
-            String nodeName = (String) nodeService.getProperty(nodeRef, ContentModel.PROP_NAME);
-            throw new AlfrescoRuntimeException("Filing permission on '" + nodeName + "' is needed.");
-        }
 
         for (final NodeRef hold : holds)
         {
             if (!isHold(hold))
             {
-                String holdName = (String) nodeService.getProperty(hold, ContentModel.PROP_NAME);
+                final String holdName = (String) nodeService.getProperty(hold, ContentModel.PROP_NAME);
-                throw new AlfrescoRuntimeException("'" + holdName + "' is not a hold so record folders/records cannot be added.");
+                throw new IntegrityException(I18NUtil.getMessage("rm.hold.not-hold", holdName), null);
             }
 
             if (permissionService.hasPermission(hold, RMPermissionModel.FILING) == AccessStatus.DENIED)
             {
-                String nodeName = (String) nodeService.getProperty(nodeRef, ContentModel.PROP_NAME);
+                throw new AccessDeniedException(I18NUtil.getMessage(MSG_ERR_ACCESS_DENIED));
-                String holdName = (String) nodeService.getProperty(hold, ContentModel.PROP_NAME);
-                throw new AlfrescoRuntimeException("'" + nodeName + "' can't be added to the hold container as filing permission for '" + holdName + "' is needed.");
             }
 
             // check that the node isn't already in the hold
             if (!getHeld(hold).contains(nodeRef))
             {
                 // run as system to ensure we have all the appropriate permissions to perform the manipulations we require
-                authenticationUtil.runAsSystem(new RunAsWork<Void>()
+                authenticationUtil.runAsSystem((RunAsWork<Void>) () -> {
-                {
+                    // gather freeze properties
-                    @Override
+                    final Map<QName, Serializable> props = new HashMap<>(2);
-                    public Void doWork()
+                    props.put(PROP_FROZEN_AT, new Date());
+                    props.put(PROP_FROZEN_BY, AuthenticationUtil.getFullyAuthenticatedUser());
+
+                    addFrozenAspect(nodeRef, props);
+
+                    // Link the record to the hold
+                    nodeService.addChild(hold, nodeRef, ASSOC_FROZEN_RECORDS, ASSOC_FROZEN_RECORDS);
+
+                    // audit item being added to the hold
+                    recordsManagementAuditService.auditEvent(nodeRef, AUDIT_ADD_TO_HOLD);
+
+                    // Mark all the folders contents as frozen
+                    if (isRecordFolder(nodeRef))
                     {
-                        // gather freeze properties
+                        final List<NodeRef> records = recordService.getRecords(nodeRef);
-                        Map<QName, Serializable> props = new HashMap<>(2);
+                        records.forEach(record -> addFrozenAspect(record, props));
-                        props.put(PROP_FROZEN_AT, new Date());
-                        props.put(PROP_FROZEN_BY, AuthenticationUtil.getFullyAuthenticatedUser());
-
-                        if (!nodeService.hasAspect(nodeRef, ASPECT_FROZEN))
-                        {
-                            // add freeze aspect
-                            nodeService.addAspect(nodeRef, ASPECT_FROZEN, props);
-
-                            if (logger.isDebugEnabled())
-                            {
-                                StringBuilder msg = new StringBuilder();
-                                msg.append("Frozen aspect applied to '").append(nodeRef).append("'.");
-                                logger.debug(msg.toString());
-                            }
-                        }
-
-                        // Link the record to the hold
-                        nodeService.addChild(hold, nodeRef, ASSOC_FROZEN_RECORDS, ASSOC_FROZEN_RECORDS);
-
-                        // audit item being added to the hold
-                        recordsManagementAuditService.auditEvent(nodeRef, AUDIT_ADD_TO_HOLD);
-
-                        // Mark all the folders contents as frozen
-                        if (isRecordFolder(nodeRef))
-                        {
-                            List<NodeRef> records = recordService.getRecords(nodeRef);
-                            for (NodeRef record : records)
-                            {
-                                // no need to freeze if already frozen!
-                                if (!nodeService.hasAspect(record, ASPECT_FROZEN))
-                                {
-                                    nodeService.addAspect(record, ASPECT_FROZEN, props);
-
-                                    if (logger.isDebugEnabled())
-                                    {
-                                        StringBuilder msg = new StringBuilder();
-                                        msg.append("Frozen aspect applied to '").append(record).append("'.");
-                                        logger.debug(msg.toString());
-                                    }
-                                }
-                            }
-                        }
-
-                        return null;
                     }
+
+                    return null;
                 });
             }
         }
     }
 
+    /**
+     * Check if the given node is eligible to be added into a hold
+     *
+     * @param nodeRef the node to be checked
+     */
+    private void checkNodeCanBeAddedToHold(NodeRef nodeRef)
+    {
+        if (!isRecord(nodeRef) && !isRecordFolder(nodeRef) && !instanceOf(nodeRef, ContentModel.TYPE_CONTENT))
+        {
+            final String nodeName = (String) nodeService.getProperty(nodeRef, ContentModel.PROP_NAME);
+            throw new IntegrityException(I18NUtil.getMessage("rm.hold.add-to-hold-invalid-type", nodeName), null);
+        }
+
+        if (((isRecord(nodeRef) || isRecordFolder(nodeRef)) &&
+                permissionService.hasPermission(nodeRef, RMPermissionModel.FILING) == AccessStatus.DENIED) ||
+                (instanceOf(nodeRef, ContentModel.TYPE_CONTENT) &&
+                        permissionService.hasPermission(nodeRef, PermissionService.WRITE) == AccessStatus.DENIED))
+        {
+            throw new AccessDeniedException(I18NUtil.getMessage(MSG_ERR_ACCESS_DENIED));
+        }
+
+        if (nodeService.hasAspect(nodeRef, ASPECT_ARCHIVED))
+        {
+            throw new IntegrityException(I18NUtil.getMessage("rm.hold.add-to-hold-archived-node"), null);
+        }
+    }
+
+    /**
+     * Add Frozen aspect only if node isn't already frozen
+     *
+     * @param nodeRef node on which aspect will be added
+     * @param props aspect properties map
+     */
+    private void addFrozenAspect(NodeRef nodeRef, Map<QName, Serializable> props)
+    {
+        if (!nodeService.hasAspect(nodeRef, ASPECT_FROZEN))
+        {
+            // add freeze aspect
+            nodeService.addAspect(nodeRef, ASPECT_FROZEN, props);
+
+            if (logger.isDebugEnabled())
+            {
+                StringBuilder msg = new StringBuilder();
+                msg.append("Frozen aspect applied to '").append(nodeRef).append("'.");
+                logger.debug(msg.toString());
+            }
+        }
+    }
+
     /**
      * @see org.alfresco.module.org_alfresco_module_rm.hold.HoldService#addToHolds(java.util.List, java.util.List)
      */

rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/model/rma/aspect/FrozenAspect.java

@@ -27,7 +27,14 @@
 
 package org.alfresco.module.org_alfresco_module_rm.model.rma.aspect;
 
+import static org.alfresco.model.ContentModel.TYPE_CONTENT;
+import static org.alfresco.model.ContentModel.TYPE_FOLDER;
+import static org.alfresco.repo.site.SiteModel.ASPECT_SITE_CONTAINER;
+
+import java.io.Serializable;
+import java.util.HashMap;
 import java.util.List;
+import java.util.Map;
 
 import org.alfresco.module.org_alfresco_module_rm.fileplan.FilePlanService;
 import org.alfresco.module.org_alfresco_module_rm.freeze.FreezeService;
@@ -148,30 +155,33 @@ public class FrozenAspect extends    BaseBehaviourBean
             kind = BehaviourKind.CLASS,
             notificationFrequency = NotificationFrequency.TRANSACTION_COMMIT
     )
-    public void onAddAspect(final NodeRef record, final QName aspectTypeQName)
+    public void onAddAspect(final NodeRef nodeRef, final QName aspectTypeQName)
     {
-        AuthenticationUtil.runAsSystem(new RunAsWork<Void>()
+        AuthenticationUtil.runAsSystem((RunAsWork<Void>) () -> {
-        {
+            if (nodeService.exists(nodeRef) && (isRecord(nodeRef) || instanceOf(nodeRef, TYPE_CONTENT)))
-            @Override
-            public Void doWork()
             {
-                if (nodeService.exists(record) &&
+                // get the owning folder
-                    isRecord(record))
+                final NodeRef parentRef = nodeService.getPrimaryParent(nodeRef).getParentRef();
+                // check that the aspect has been added
+                if (nodeService.hasAspect(parentRef, ASPECT_HELD_CHILDREN))
                 {
-                    // get the owning record folder
+                    // increment current count
-                    NodeRef recordFolder = nodeService.getPrimaryParent(record).getParentRef();
+                    int currentCount = (Integer) nodeService.getProperty(parentRef, PROP_HELD_CHILDREN_COUNT);
-                    // check that the aspect has been added
+                    currentCount = currentCount + 1;
-                    if (nodeService.hasAspect(recordFolder, ASPECT_HELD_CHILDREN))
+                    nodeService.setProperty(parentRef, PROP_HELD_CHILDREN_COUNT, currentCount);
+                } else
+                {
+                    if (instanceOf(parentRef, TYPE_FOLDER) && !nodeService.hasAspect(parentRef, ASPECT_SITE_CONTAINER))
                     {
-                        // increment current count
+                        // add aspect and set count to 1
-                        int currentCount = (Integer)nodeService.getProperty(recordFolder, PROP_HELD_CHILDREN_COUNT);
+                        final Map<QName, Serializable> props = new HashMap<>(1);
-                        currentCount = currentCount + 1;
+                        props.put(PROP_HELD_CHILDREN_COUNT, 1);
-                        nodeService.setProperty(recordFolder, PROP_HELD_CHILDREN_COUNT, currentCount);
+                        getInternalNodeService().addAspect(parentRef, ASPECT_HELD_CHILDREN, props);
                     }
                 }
-                return null;
             }
-        });        
+            return null;
+        });
     }
 
     @Override

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

@@ -0,0 +1,248 @@
+/*
+ * #%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 static org.alfresco.repo.security.authentication.AuthenticationUtil.getAdminUserName;
+import static org.alfresco.repo.site.SiteModel.SITE_MANAGER;
+
+import org.alfresco.module.org_alfresco_module_rm.capability.RMPermissionModel;
+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;
+    }
+
+    /**
+     * Given active content
+     * And file permission on the hold
+     * And the appropriate capability to add to hold
+     * When I try to add the active content to the hold
+     * Then the active content is frozen
+     * And the active content is contained within the hold
+     */
+    public void testAddDocumentToHold()
+    {
+        doBehaviourDrivenTest(new BehaviourDrivenTest()
+        {
+            private NodeRef hold;
+
+            public void given()
+            {
+                // create a hold
+                hold = holdService.createHold(filePlan, GUID.generate(), GUID.generate(), GUID.generate());
+            }
+
+            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));
+            }
+        });
+    }
+
+    /**
+     * Given active content
+     * And a non rm user with write permission on active content
+     * When user tries to add the active content to hold
+     * Then AccessDeniedException is thrown
+     */
+    public void testAddDocumentToHoldAsNonRMUser()
+    {
+        doBehaviourDrivenTest(new BehaviourDrivenTest(AccessDeniedException.class)
+        {
+            private NodeRef hold;
+
+            public void given()
+            {
+                // create a hold
+                hold = holdService.createHold(filePlan, GUID.generate(), GUID.generate(), GUID.generate());
+            }
+
+            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);
+            }
+        });
+    }
+
+    /**
+     * Given active content
+     * And a rm user with Filing permission on hold and Add to Hold Capability, but only read permission on active content
+     * When user tries to add the active content to hold
+     * Then an exception is thrown
+     */
+    public void testAddDocumentToHoldNoWritePermissionOnDoc()
+    {
+        doBehaviourDrivenTest(new BehaviourDrivenTest(AccessDeniedException.class)
+        {
+            private NodeRef hold;
+
+            public void given()
+            {
+                // create a hold
+                hold = holdService.createHold(filePlan, GUID.generate(), GUID.generate(), GUID.generate());
+
+                //add rm Admin as consumer in collaboration site to have read permissions on dmDocument
+                siteService.setMembership(collabSiteId, rmAdminName, SiteModel.SITE_CONSUMER);
+            }
+
+            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);
+            }
+        });
+    }
+
+    /**
+     * Given active content
+     * And a rm user with Add to Hold Capability, write permission on active content and only Read permission on hold
+     * When user tries to add the active content to hold
+     * Then AccessDeniedException is thrown
+     */
+    public void testAddDocumentToHoldNoFilingPermissionOnHold()
+    {
+        doBehaviourDrivenTest(new BehaviourDrivenTest(AccessDeniedException.class, recordsManagerName, false)
+        {
+            private NodeRef hold;
+
+            public void given()
+            {
+                AuthenticationUtil.runAs(
+                        (RunAsWork<Void>) () -> {
+                            // create a hold
+                            hold = holdService.createHold(filePlan, GUID.generate(), GUID.generate(), GUID.generate());
+
+                            //add Read permission on hold
+                            filePlanPermissionService.setPermission(hold, recordsManagerName, RMPermissionModel.READ_RECORDS);
+
+                            //add recordsManagerPerson as manager in collaboration site to have write permissions on dmDocument
+                            siteService.setMembership(collabSiteId, recordsManagerName, SITE_MANAGER);
+                            return null;
+                        }, getAdminUserName());
+            }
+
+            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
+                holdService.addToHold(hold, dmDocument);
+            }
+        });
+    }
+
+    /**
+     * Given active content
+     * And a rm user with write permission on active content and Filing permission on hold, but no Add to Hold Capability
+     * When user tries to add the active content to hold
+     * Then AccessDeniedException is thrown
+     */
+    public void testAddDocumentToHoldNoCapability()
+    {
+        doBehaviourDrivenTest(new BehaviourDrivenTest(AccessDeniedException.class, powerUserName, false)
+        {
+            private NodeRef hold;
+
+            public void given()
+            {
+                AuthenticationUtil.runAs(
+                        (RunAsWork<Void>) () -> {
+                            // create a hold
+                            hold = holdService.createHold(filePlan, GUID.generate(), GUID.generate(), GUID.generate());
+
+                            //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);
+                            return null;
+                        }, getAdminUserName());
+            }
+
+            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
+                holdService.addToHold(hold, dmDocument);
+            }
+        });
+    }
+}

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

@@ -44,7 +44,7 @@ import org.springframework.extensions.webscripts.GUID;
 public class AddRemoveFromHoldTest extends BaseRMTestCase
 {
     private static final int RECORD_COUNT = 10;
-    
+
     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())
@@ -915,6 +922,13 @@ public abstract class BaseRMTestCase extends RetryingTransactionHelperTestCase
             }
         }
 
+        public BehaviourDrivenTest(Class<?> expectedException, String runAsUser, boolean runInTransactionTests)
+        {
+            this.expectedException = expectedException;
+            this.runAsUser = runAsUser;
+            this.runInTransactionTests = runInTransactionTests;
+        }
+
         public void given() throws Exception { /** empty implementation */ }
 
         public void when() throws Exception  { /** empty implementation */ }

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

@@ -27,6 +27,8 @@
 
 package org.alfresco.module.org_alfresco_module_rm.hold;
 
+import static java.util.Arrays.asList;
+
 import static org.alfresco.module.org_alfresco_module_rm.test.util.AlfMock.generateQName;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
@@ -51,9 +53,15 @@ 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.model.RecordsManagementModel;
 import org.alfresco.module.org_alfresco_module_rm.test.util.BaseUnitTest;
+import org.alfresco.repo.node.integrity.IntegrityException;
+import org.alfresco.repo.security.permissions.AccessDeniedException;
 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;
@@ -81,6 +89,7 @@ public class HoldServiceImplUnitTest extends BaseUnitTest
     protected NodeRef holdContainer;
     protected NodeRef hold;
     protected NodeRef hold2;
+    protected NodeRef activeContent;
 
     @Spy @InjectMocks HoldServiceImpl holdService;
 
@@ -95,6 +104,11 @@ public class HoldServiceImplUnitTest extends BaseUnitTest
         hold = generateNodeRef(TYPE_HOLD);
         hold2 = generateNodeRef(TYPE_HOLD);
 
+        activeContent = generateNodeRef();
+        QName contentSubtype = QName.createQName("contentSubtype", "contentSubtype");
+        when(mockedNodeService.getType(activeContent)).thenReturn(contentSubtype);
+        when(mockedDictionaryService.isSubClass(contentSubtype, ContentModel.TYPE_CONTENT)).thenReturn(true);
+
         // setup interactions
         doReturn(holdContainer).when(mockedFilePlanService).getHoldContainer(filePlan);
     }
@@ -110,11 +124,18 @@ public class HoldServiceImplUnitTest extends BaseUnitTest
     public void heldByMultipleResults()
     {
         // setup record folder in multiple holds
-        List<ChildAssociationRef> holds = new ArrayList<>(2);
+        List<ChildAssociationRef> holds = new ArrayList<>(4);
         holds.add(new ChildAssociationRef(ASSOC_FROZEN_RECORDS, hold, ASSOC_FROZEN_RECORDS, recordFolder, true, 1));
         holds.add(new ChildAssociationRef(ASSOC_FROZEN_RECORDS, hold2, ASSOC_FROZEN_RECORDS, recordFolder, true, 2));
+        holds.add(new ChildAssociationRef(ASSOC_FROZEN_RECORDS, hold, ASSOC_FROZEN_RECORDS, activeContent, true, 1));
+        holds.add(new ChildAssociationRef(ASSOC_FROZEN_RECORDS, hold2, ASSOC_FROZEN_RECORDS, activeContent, 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);
+
+        doReturn(Collections.singleton(filePlan)).when(mockedFilePlanService).getFilePlans();
+
         // check that both holds are found for record folder
         List<NodeRef> heldByHolds = holdService.heldBy(recordFolder, true);
         assertNotNull(heldByHolds);
@@ -126,6 +147,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)
@@ -165,13 +192,15 @@ public class HoldServiceImplUnitTest extends BaseUnitTest
     public void getHeldWithResults()
     {
         // setup record folder in hold
-        List<ChildAssociationRef> holds = new ArrayList<>(1);
+        List<ChildAssociationRef> holds = new ArrayList<>(2);
         holds.add(new ChildAssociationRef(ASSOC_FROZEN_RECORDS, hold, ASSOC_FROZEN_RECORDS, recordFolder, true, 1));
+        holds.add(new ChildAssociationRef(ASSOC_FROZEN_RECORDS, hold, ASSOC_FROZEN_RECORDS, activeContent, true, 1));
         doReturn(holds).when(mockedNodeService).getChildAssocs(hold, ASSOC_FROZEN_RECORDS, RegexQNamePattern.MATCH_ALL);
 
         List<NodeRef> list = holdService.getHeld(hold);
-        assertEquals(1, list.size());
+        assertEquals(2, list.size());
         assertEquals(recordFolder, list.get(0));
+        assertEquals(activeContent, list.get(1));
     }
 
     @SuppressWarnings({ "unchecked", "rawtypes" })
@@ -275,14 +304,14 @@ public class HoldServiceImplUnitTest extends BaseUnitTest
         // TODO check interactions with policy component!!!
     }
 
-    @Test (expected=AlfrescoRuntimeException.class)
+    @Test (expected = IntegrityException.class)
     public void addToHoldNotAHold()
     {
         holdService.addToHold(recordFolder, recordFolder);
     }
 
-    @Test (expected=AlfrescoRuntimeException.class)
+    @Test (expected = IntegrityException.class)
-    public void addToHoldNotARecordFolderOrRecord()
+    public void addToHoldNotARecordFolderOrRecordOrActiveContent()
     {
         NodeRef anotherThing = generateNodeRef(TYPE_RECORD_CATEGORY);
         holdService.addToHold(hold, anotherThing);
@@ -297,20 +326,25 @@ public class HoldServiceImplUnitTest extends BaseUnitTest
         verify(mockedNodeService).addChild(hold, recordFolder, ASSOC_FROZEN_RECORDS, ASSOC_FROZEN_RECORDS);
         verify(mockedNodeService).addAspect(eq(recordFolder), eq(ASPECT_FROZEN), any(Map.class));
         verify(mockedNodeService).addAspect(eq(record), eq(ASPECT_FROZEN), any(Map.class));
-        verify(mockedRecordsManagementAuditService, times(1)).auditEvent(eq(recordFolder), anyString());
+        verify(mockedRecordsManagementAuditService).auditEvent(eq(recordFolder), anyString());
 
         holdService.addToHold(hold, record);
         verify(mockedNodeService).addChild(hold, record, ASSOC_FROZEN_RECORDS, ASSOC_FROZEN_RECORDS);
         verify(mockedNodeService).addAspect(eq(recordFolder), eq(ASPECT_FROZEN), any(Map.class));
         verify(mockedNodeService, times(2)).addAspect(eq(record), eq(ASPECT_FROZEN), any(Map.class));
-        verify(mockedRecordsManagementAuditService, times(1)).auditEvent(eq(record), anyString());
+        verify(mockedRecordsManagementAuditService).auditEvent(eq(record), anyString());
+
+        holdService.addToHold(hold, activeContent);
+        verify(mockedNodeService).addChild(hold, activeContent, ASSOC_FROZEN_RECORDS, ASSOC_FROZEN_RECORDS);
+        verify(mockedNodeService).addAspect(eq(activeContent), eq(ASPECT_FROZEN), any(Map.class));
+        verify(mockedRecordsManagementAuditService).auditEvent(eq(activeContent), anyString());
     }
 
     @SuppressWarnings("unchecked")
     @Test
     public void addToHoldAlreadyInHold()
     {
-        doReturn(Collections.singletonList(recordFolder)).when(holdService).getHeld(hold);
+        doReturn(asList(recordFolder, activeContent)).when(holdService).getHeld(hold);
 
         holdService.addToHold(hold, recordFolder);
 
@@ -318,21 +352,54 @@ public class HoldServiceImplUnitTest extends BaseUnitTest
         verify(mockedNodeService, never()).addAspect(eq(recordFolder), eq(ASPECT_FROZEN), any(Map.class));
         verify(mockedNodeService, never()).addAspect(eq(record), eq(ASPECT_FROZEN), any(Map.class));
         verify(mockedRecordsManagementAuditService, never()).auditEvent(eq(recordFolder), anyString());
+
+        holdService.addToHold(hold, activeContent);
+
+        verify(mockedNodeService, never()).addChild(hold, activeContent, ASSOC_FROZEN_RECORDS, ASSOC_FROZEN_RECORDS);
+        verify(mockedNodeService, never()).addAspect(eq(activeContent), eq(ASPECT_FROZEN), any(Map.class));
+        verify(mockedRecordsManagementAuditService, never()).auditEvent(eq(activeContent), anyString());
     }
 
     @SuppressWarnings("unchecked")
     @Test
-    public void addToHoldAldeadyFrozen()
+    public void addToHoldAlreadyFrozen()
     {
         doReturn(true).when(mockedNodeService).hasAspect(recordFolder, ASPECT_FROZEN);
         doReturn(true).when(mockedNodeService).hasAspect(record, ASPECT_FROZEN);
+        doReturn(true).when(mockedNodeService).hasAspect(activeContent, ASPECT_FROZEN);
 
         holdService.addToHold(hold, recordFolder);
 
-        verify(mockedNodeService, times(1)).addChild(hold, recordFolder, ASSOC_FROZEN_RECORDS, ASSOC_FROZEN_RECORDS);
+        verify(mockedNodeService).addChild(hold, recordFolder, ASSOC_FROZEN_RECORDS, ASSOC_FROZEN_RECORDS);
         verify(mockedNodeService, never()).addAspect(eq(recordFolder), eq(ASPECT_FROZEN), any(Map.class));
         verify(mockedNodeService, never()).addAspect(eq(record), eq(ASPECT_FROZEN), any(Map.class));
-        verify(mockedRecordsManagementAuditService, times(1)).auditEvent(eq(recordFolder), anyString());
+        verify(mockedRecordsManagementAuditService).auditEvent(eq(recordFolder), anyString());
+
+        holdService.addToHold(hold, activeContent);
+        verify(mockedNodeService).addChild(hold, activeContent, ASSOC_FROZEN_RECORDS, ASSOC_FROZEN_RECORDS);
+        verify(mockedNodeService, never()).addAspect(eq(activeContent), eq(ASPECT_FROZEN), any(Map.class));
+        verify(mockedRecordsManagementAuditService).auditEvent(eq(activeContent), anyString());
+    }
+
+    @Test (expected = AccessDeniedException.class)
+    public void addActiveContentToHoldNoPermissionsOnHold()
+    {
+        when(mockedPermissionService.hasPermission(hold, RMPermissionModel.FILING)).thenReturn(AccessStatus.DENIED);
+        holdService.addToHold(hold, activeContent);
+    }
+
+    @Test (expected = AccessDeniedException.class)
+    public void addActiveContentToHoldNoPermissionsOnContent()
+    {
+        when(mockedPermissionService.hasPermission(activeContent, PermissionService.WRITE)).thenReturn(AccessStatus.DENIED);
+        holdService.addToHold(hold, activeContent);
+    }
+
+    @Test (expected = IntegrityException.class)
+    public void addArchivedContentToHold()
+    {
+        when(mockedNodeService.hasAspect(activeContent, RecordsManagementModel.ASPECT_ARCHIVED)).thenReturn(true);
+        holdService.addToHold(hold, activeContent);
     }
 
     @SuppressWarnings("unchecked")