Merge remote-tracking branch 'remotes/origin/master' into feature/RM-7033_AuditHoldTests_no

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

@@ -178,6 +178,11 @@
 
    <util:constant id="propThumbnailModification" static-field="org.alfresco.model.ContentModel.PROP_LAST_THUMBNAIL_MODIFICATION_DATA" />
    
+   <!-- Defines a list of namespace URIs for properties, which should be always editable for a frozen node-->
+   <util:list id="frozen_alwaysEditURIs" value-type="java.lang.String">
+      <value>http://www.alfresco.org/model/system/1.0</value>
+   </util:list>
+
    <bean name="updateFrozenPropertyCheck"
          class="org.alfresco.module.org_alfresco_module_rm.util.PropertyModificationAllowedCheck">
       <property name="whiteList">
@@ -185,6 +190,7 @@
             <ref bean="propThumbnailModification" />
          </list>
       </property>
+      <property name="editableURIs" ref="frozen_alwaysEditURIs" />
    </bean>
 
    <bean id="rma.vitalRecordDefinition" class="org.alfresco.module.org_alfresco_module_rm.model.rma.aspect.VitalRecordDefinitionAspect" parent="rm.baseBehaviour">

rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/audit/event/HoldUtils.java

@@ -28,6 +28,7 @@
 package org.alfresco.module.org_alfresco_module_rm.audit.event;
 
 import org.alfresco.model.ContentModel;
+import org.alfresco.module.org_alfresco_module_rm.model.RecordsManagementModel;
 import org.alfresco.service.cmr.repository.NodeRef;
 import org.alfresco.service.cmr.repository.NodeService;
 import org.alfresco.service.namespace.QName;
@@ -44,6 +45,8 @@ import java.util.Map;
  */
 class HoldUtils
 {
+    /** A QName to display for the hold name. */
+    public static final QName HOLD_NAME = QName.createQName(RecordsManagementModel.RM_URI, "Hold Name");
     /**
      * Create a properties map containing the hold name for the given hold.
      *
@@ -55,7 +58,7 @@ class HoldUtils
     {
         Map<QName, Serializable> auditProperties = new HashMap<>();
 
-        auditProperties.put(ContentModel.PROP_NAME, nodeService.getProperty(nodeRef, ContentModel.PROP_NAME));
+        auditProperties.put(HOLD_NAME, nodeService.getProperty(nodeRef, ContentModel.PROP_NAME));
 
         return auditProperties;
     }

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

@@ -47,10 +47,14 @@ import org.alfresco.module.org_alfresco_module_rm.audit.event.AuditEvent;
 import org.alfresco.module.org_alfresco_module_rm.capability.CapabilityService;
 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.hold.HoldServicePolicies.BeforeAddToHoldPolicy;
 import org.alfresco.module.org_alfresco_module_rm.hold.HoldServicePolicies.BeforeCreateHoldPolicy;
 import org.alfresco.module.org_alfresco_module_rm.hold.HoldServicePolicies.BeforeDeleteHoldPolicy;
+import org.alfresco.module.org_alfresco_module_rm.hold.HoldServicePolicies.BeforeRemoveFromHoldPolicy;
+import org.alfresco.module.org_alfresco_module_rm.hold.HoldServicePolicies.OnAddToHoldPolicy;
 import org.alfresco.module.org_alfresco_module_rm.hold.HoldServicePolicies.OnCreateHoldPolicy;
 import org.alfresco.module.org_alfresco_module_rm.hold.HoldServicePolicies.OnDeleteHoldPolicy;
+import org.alfresco.module.org_alfresco_module_rm.hold.HoldServicePolicies.OnRemoveFromHoldPolicy;
 import org.alfresco.module.org_alfresco_module_rm.model.RecordsManagementModel;
 import org.alfresco.module.org_alfresco_module_rm.record.RecordService;
 import org.alfresco.module.org_alfresco_module_rm.recordfolder.RecordFolderService;
@@ -206,6 +210,10 @@ public class HoldServiceImpl extends ServiceBaseImpl
     private ClassPolicyDelegate<OnCreateHoldPolicy> onCreateHoldPolicyDelegate;
     private ClassPolicyDelegate<BeforeDeleteHoldPolicy> beforeDeleteHoldPolicyDelegate;
     private ClassPolicyDelegate<OnDeleteHoldPolicy> onDeleteHoldPolicyDelegate;
+    private ClassPolicyDelegate<BeforeAddToHoldPolicy> beforeAddToHoldPolicyDelegate;
+    private ClassPolicyDelegate<OnAddToHoldPolicy> onAddToHoldPolicyDelegate;
+    private ClassPolicyDelegate<BeforeRemoveFromHoldPolicy> beforeRemoveFromHoldPolicyDelegate;
+    private ClassPolicyDelegate<OnRemoveFromHoldPolicy> onRemoveFromHoldPolicyDelegate;
 
     /**
      * Initialise hold service
@@ -228,6 +236,11 @@ public class HoldServiceImpl extends ServiceBaseImpl
         onCreateHoldPolicyDelegate = getPolicyComponent().registerClassPolicy(OnCreateHoldPolicy.class);
         beforeDeleteHoldPolicyDelegate = getPolicyComponent().registerClassPolicy(BeforeDeleteHoldPolicy.class);
         onDeleteHoldPolicyDelegate = getPolicyComponent().registerClassPolicy(OnDeleteHoldPolicy.class);
+        beforeAddToHoldPolicyDelegate = getPolicyComponent().registerClassPolicy(BeforeAddToHoldPolicy.class);
+        onAddToHoldPolicyDelegate = getPolicyComponent().registerClassPolicy(OnAddToHoldPolicy.class);
+        beforeRemoveFromHoldPolicyDelegate = getPolicyComponent().registerClassPolicy(BeforeRemoveFromHoldPolicy.class);
+        onRemoveFromHoldPolicyDelegate = getPolicyComponent().registerClassPolicy(OnRemoveFromHoldPolicy.class);
+
     }
 
     /**
@@ -637,6 +650,8 @@ public class HoldServiceImpl extends ServiceBaseImpl
             // check that the node isn't already in the hold
             if (!getHeld(hold).contains(nodeRef))
             {
+                // fire before add to hold policy
+                invokeBeforeAddToHold(hold, nodeRef);
                 // run as system to ensure we have all the appropriate permissions to perform the manipulations we require
                 authenticationUtil.runAsSystem((RunAsWork<Void>) () -> {
                     // gather freeze properties
@@ -661,6 +676,9 @@ public class HoldServiceImpl extends ServiceBaseImpl
                         records.forEach(record -> addFrozenAspect(record, props));
                     }
 
+                    // fire on add to hold policy
+                    invokeOnAddToHold(hold, nodeRef);
+
                     return null;
                 });
             }
@@ -797,6 +815,8 @@ public class HoldServiceImpl extends ServiceBaseImpl
                     // run as system so we don't run into further permission issues
                     // we already know we have to have the correct capability to get here
                     authenticationUtil.runAsSystem((RunAsWork<Void>) () -> {
+                        // fire before remove from hold policy
+                        invokeBeforeRemoveFromHold(hold, nodeRef);
                         // remove from hold
                         //set in transaction cache in order not to trigger update policy when removing the child association
                         transactionalResourceHelper.getSet("frozen").add(nodeRef);
@@ -806,6 +826,9 @@ public class HoldServiceImpl extends ServiceBaseImpl
                         // TODO add details of the hold that the node was removed from
                         recordsManagementAuditService.auditEvent(nodeRef, AUDIT_REMOVE_FROM_HOLD);
 
+                        // fire on remove from hold policy
+                        invokeOnRemoveFromHold(hold, nodeRef);
+
                         return null;
 
                     });
@@ -916,4 +939,52 @@ public class HoldServiceImpl extends ServiceBaseImpl
 
     }
 
+    /**
+     * Invoke beforeAddToHold policy
+     *
+     * @param hold           hold node reference
+     * @param contentNodeRef content node reference
+     */
+    protected void invokeBeforeAddToHold(NodeRef hold, NodeRef contentNodeRef)
+    {
+        BeforeAddToHoldPolicy policy = beforeAddToHoldPolicyDelegate.get(getTypeAndApsects(hold));
+        policy.beforeAddToHold(hold, contentNodeRef);
+    }
+
+    /**
+     * Invoke onAddToHold policy
+     *
+     * @param hold           hold node reference
+     * @param contentNodeRef content node reference
+     */
+    protected void invokeOnAddToHold(NodeRef hold, NodeRef contentNodeRef)
+    {
+        OnAddToHoldPolicy policy = onAddToHoldPolicyDelegate.get(getTypeAndApsects(hold));
+        policy.onAddToHold(hold, contentNodeRef);
+    }
+
+    /**
+     * Invoke beforeRemoveFromHold policy
+     *
+     * @param hold           hold node reference
+     * @param contentNodeRef content node reference
+     */
+    protected void invokeBeforeRemoveFromHold(NodeRef hold, NodeRef contentNodeRef)
+    {
+        BeforeRemoveFromHoldPolicy policy = beforeRemoveFromHoldPolicyDelegate.get(getTypeAndApsects(hold));
+        policy.beforeRemoveFromHold(hold, contentNodeRef);
+    }
+
+    /**
+     * Invoke onRemoveFromHold policy
+     *
+     * @param hold           hold node reference
+     * @param contentNodeRef content node reference
+     */
+    protected void invokeOnRemoveFromHold(NodeRef hold, NodeRef contentNodeRef)
+    {
+        OnRemoveFromHoldPolicy policy = onRemoveFromHoldPolicyDelegate.get(getTypeAndApsects(hold));
+        policy.onRemoveFromHold(hold, contentNodeRef);
+
+    }
 }

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

@@ -36,6 +36,7 @@ import org.alfresco.service.namespace.QName;
  * Hold Service Policies
  *
  * @author Ramona Popa
+ * @author Roxana Lucanu
  * @since 3.3
  */
 
@@ -86,4 +87,57 @@ public interface HoldServicePolicies
          */
         void onDeleteHold(String holdname);
     }
+
+    interface BeforeAddToHoldPolicy extends ClassPolicy
+    {
+        QName QNAME = QName.createQName(NamespaceService.ALFRESCO_URI, "beforeAddToHold");
+
+        /**
+         * Called before adding content to hold.
+         *
+         * @param hold           the hold to be added into
+         * @param contentNodeRef the item to be added to hold
+         */
+        void beforeAddToHold(NodeRef hold, NodeRef contentNodeRef);
+    }
+
+    interface OnAddToHoldPolicy extends ClassPolicy
+    {
+        QName QNAME = QName.createQName(NamespaceService.ALFRESCO_URI, "onAddToHold");
+
+        /**
+         * Called when content is added to hold.
+         *
+         * @param hold           the hold to be added into
+         * @param contentNodeRef the item to be added to hold
+         */
+        void onAddToHold(NodeRef hold, NodeRef contentNodeRef);
+    }
+
+    interface BeforeRemoveFromHoldPolicy extends ClassPolicy
+    {
+        QName QNAME = QName.createQName(NamespaceService.ALFRESCO_URI, "beforeRemoveFromHold");
+
+        /**
+         * Called before removing content from hold.
+         *
+         * @param hold           the hold to be removed from
+         * @param contentNodeRef the item to be removed from hold
+         */
+        void beforeRemoveFromHold(NodeRef hold, NodeRef contentNodeRef);
+    }
+
+    interface OnRemoveFromHoldPolicy extends ClassPolicy
+    {
+        QName QNAME = QName.createQName(NamespaceService.ALFRESCO_URI, "onRemoveFromHold");
+
+        /**
+         * Called when removing content from hold.
+         *
+         * @param hold           the hold to be removed from
+         * @param contentNodeRef the item to be removed from hold
+         */
+        void onRemoveFromHold(NodeRef hold, NodeRef contentNodeRef);
+    }
+
 }

rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/util/PropertyModificationAllowedCheck.java

@@ -26,10 +26,13 @@
  */
 package org.alfresco.module.org_alfresco_module_rm.util;
 
+import static java.util.Collections.unmodifiableList;
+
 import java.io.Serializable;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 
 import org.alfresco.service.namespace.QName;
 
@@ -46,61 +49,98 @@ public class PropertyModificationAllowedCheck
      */
     private List<QName> whiteList;
 
+    /**
+     * List of model URI's for which the properties can be updated
+     */
+    private List<String> editableURIs;
+
+    /**
+     * Getter for list of model URI's
+     *
+     * @return return the list of model URI's
+     */
+    private List<String> getEditableURIs()
+    {
+        return unmodifiableList(editableURIs);
+    }
+
+    /**
+     * Setter for list of model URI's
+     *
+     * @param editableURIs List<String>
+     */
+    public void setEditableURIs(List<String> editableURIs)
+    {
+        this.editableURIs = unmodifiableList(editableURIs);
+    }
+
     /**
      * Setter for list of qnames
+     *
      * @param whiteList List<QName>
      */
     public void setWhiteList(List<QName> whiteList)
     {
-        this.whiteList = whiteList;
+        this.whiteList = unmodifiableList(whiteList);
+
     }
 
     /**
      * Compares the node properties with the requested update to make sure all potential updates are permitted
+     *
      * @param before current node properties
      * @param after  updated properties for the node
-     * @return true -  if all modified property keys are in the whitelist
+     * @return true -  if all modified property keys are in the whitelist or
+     *                    in the list of model URI's for which the properties can be modified
      */
     public boolean check(Map<QName, Serializable> before, Map<QName, Serializable> after)
     {
         boolean proceed = true;
-        HashSet<QName> unionKeys = new HashSet<>(before.keySet());
+        // Initially check for changes to existing keys and values.
-        unionKeys.addAll(after.keySet());
+        for (final Map.Entry<QName, Serializable> entry : before.entrySet())
-        for (QName key : unionKeys)
         {
-            //Check if property has been added or removed
+            final QName key = entry.getKey();
-            if (!before.containsKey(key)  || !after.containsKey(key))
+            final Serializable beforeValue = entry.getValue();
+            //check if property has been updated
+            final boolean modified = after.containsKey(key) && after.get(key) != null
+                    && !after.get(key).equals(beforeValue);
+
+            //check if the property has been emptied or removed
+            final boolean propertyRemovedEmptied = (after.get(key) == null && beforeValue != null)
+                                            || !after.containsKey(key);
+            if (modified || propertyRemovedEmptied)
             {
-                //Property modified check to see if allowed
+                proceed = allowPropertyUpdate(key);
-                proceed = whiteList.contains(key);
+            }
+            if (!proceed)
+            {
+                return proceed;
+            }
+        }
+
+        // Check for new values. Record individual values and group as a single map.
+        final Set<QName> newKeys = new HashSet<>(after.keySet());
+        newKeys.removeAll(before.keySet());
+        for (final QName key : newKeys)
+        {
+            proceed = allowPropertyUpdate(key);
             if (!proceed)
             {
                 break;
             }
         }
-            //Check if property emptied or empty property filled
-            if  ((before.get(key) == null && after.get(key) != null) ||
-                    (after.get(key) == null && before.get(key) != null))
-            {
-                //Property modified check to see if allowed
-                proceed = whiteList.contains(key);
-                if (!proceed)
-                {
-                    break;
-                }
-            }
-            //If properties aren't missing or empty check equality
-            if (before.get(key) != null && after.get(key) != null && !(after.get(key).equals(before.get(key))))
-            {
-                //Property modified check to see if allowed
-                proceed = whiteList.contains(key);
-                if (!proceed)
-                {
-                    break;
-                }
-            }
-        }
         return proceed;
     }
 
+    /**
+     * Determines whether the property should be allowed to be updated or not.
+     *
+     * @param key property
+     * @return true if property update is allowed
+     */
+    private boolean allowPropertyUpdate(QName key)
+    {
+        return whiteList.contains(key) || getEditableURIs().contains(key.getNamespaceURI());
+    }
+
 }

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

@@ -24,27 +24,37 @@
  * along with Alfresco. If not, see <http://www.gnu.org/licenses/>.
  * #L%
  */
-
 package org.alfresco.module.org_alfresco_module_rm.test.integration.hold;
 
 import java.util.ArrayList;
 import java.util.List;
 
 import org.alfresco.model.ContentModel;
+import org.alfresco.module.org_alfresco_module_rm.hold.HoldServicePolicies.BeforeAddToHoldPolicy;
+import org.alfresco.module.org_alfresco_module_rm.hold.HoldServicePolicies.OnAddToHoldPolicy;
+import org.alfresco.module.org_alfresco_module_rm.model.RecordsManagementModel;
 import org.alfresco.module.org_alfresco_module_rm.test.util.BaseRMTestCase;
+import org.alfresco.repo.policy.Behaviour.NotificationFrequency;
+import org.alfresco.repo.policy.BehaviourDefinition;
+import org.alfresco.repo.policy.ClassBehaviourBinding;
+import org.alfresco.repo.policy.JavaBehaviour;
 import org.alfresco.service.cmr.repository.NodeRef;
 import org.springframework.extensions.webscripts.GUID;
 
 /**
- * Hold service integration test.
+ * Add To Hold Integration Tests
  *
  * @author Roy Wetherall
  * @since 2.2
  */
-public class AddRemoveFromHoldTest extends BaseRMTestCase
+
+public class AddToHoldTest extends BaseRMTestCase implements BeforeAddToHoldPolicy, OnAddToHoldPolicy
 {
     private static final int RECORD_COUNT = 10;
 
+    private boolean beforeAddToHoldFlag = false;
+    private boolean onAddToHoldFlag = false;
+
     public void testAddRecordToHold()
     {
         doBehaviourDrivenTest(new BehaviourDrivenTest()
@@ -243,182 +253,63 @@ public class AddRemoveFromHoldTest extends BaseRMTestCase
 
     }
 
-    public void testRemoveRecordsFromHold()
+    public void testPolicyNotificationForAddToHold()
     {
         doBehaviourDrivenTest(new BehaviourDrivenTest()
         {
             private NodeRef hold;
             private NodeRef recordCategory;
             private NodeRef recordFolder;
-            private List<NodeRef> records = new ArrayList<>(RECORD_COUNT);
+            BehaviourDefinition<ClassBehaviourBinding> beforeAddToHoldBehaviour;
+            BehaviourDefinition<ClassBehaviourBinding> onAddToHoldBehaviour;
 
             public void given()
             {
                 // create a hold
                 hold = holdService.createHold(filePlan, GUID.generate(), GUID.generate(), GUID.generate());
-                
+                // create a record category -> record folder
-                // create a record folder that contains records  
                 recordCategory = filePlanService.createRecordCategory(filePlan, GUID.generate());
                 recordFolder = recordFolderService.createRecordFolder(recordCategory, GUID.generate());
-                for (int i = 0; i < RECORD_COUNT; i++)
-                {
-                    records.add(recordService.createRecordFromContent(recordFolder, GUID.generate(), ContentModel.TYPE_CONTENT, null, null));
-                }
 
-                // add records to hold
+                beforeAddToHoldBehaviour = policyComponent.bindClassBehaviour(BeforeAddToHoldPolicy.QNAME,
-                holdService.addToHold(hold, records);
+                        RecordsManagementModel.TYPE_HOLD, new JavaBehaviour(AddToHoldTest.this, "beforeAddToHold", NotificationFrequency.EVERY_EVENT));
+
+                onAddToHoldBehaviour = policyComponent.bindClassBehaviour(OnAddToHoldPolicy.QNAME,
+                        RecordsManagementModel.TYPE_HOLD, new JavaBehaviour(AddToHoldTest.this, "onAddToHold", NotificationFrequency.EVERY_EVENT));
+
+                assertFalse(beforeAddToHoldFlag);
+                assertFalse(onAddToHoldFlag);
             }
 
             public void when() throws Exception
             {
-                // remove *some* of the records
+                // add the record folder to hold
-                holdService.removeFromHold(hold, records.subList(0, 5));
-            }
-            
-            public void then()
-            {
-                // check record state (no longer held)   
-                for (NodeRef record : records.subList(0, 5))
-                {                
-                    assertFalse(freezeService.isFrozen(record));
-                    assertFalse(holdService.getHeld(hold).contains(record));
-                    assertFalse(holdService.heldBy(record, true).contains(hold));
-                }
-                
-                // check record state (still held)   
-                for (NodeRef record : records.subList(5, 10))
-                {                
-                    assertTrue(freezeService.isFrozen(record));
-                    assertTrue(holdService.getHeld(hold).contains(record));
-                    assertTrue(holdService.heldBy(record, true).contains(hold));
-                }
-                
-                // record folder has frozen children
-                assertFalse(freezeService.isFrozen(recordFolder));
-                assertTrue(freezeService.hasFrozenChildren(recordFolder));
-                
-                // record folder is not held
-                assertFalse(holdService.getHeld(hold).contains(recordFolder));
-                assertFalse(holdService.heldBy(recordFolder, true).contains(hold));
-                
-                // additional check for child held caching
-                assertTrue(nodeService.hasAspect(recordFolder, ASPECT_HELD_CHILDREN));
-                assertEquals(5, nodeService.getProperty(recordFolder, PROP_HELD_CHILDREN_COUNT));
-            }            
-        });         
-    }
-    
-    public void testRemoveAllRecordsFromHold()
-    {
-        doBehaviourDrivenTest(new BehaviourDrivenTest()
-        {   
-            private NodeRef hold;
-            private NodeRef recordCategory;
-            private NodeRef recordFolder;
-            private List<NodeRef> records = new ArrayList<>(RECORD_COUNT);
-            
-            public void given()
-            {   
-                // create a hold
-                hold = holdService.createHold(filePlan, GUID.generate(), GUID.generate(), GUID.generate());
-                
-                // create a record folder that contains records  
-                recordCategory = filePlanService.createRecordCategory(filePlan, GUID.generate());
-                recordFolder = recordFolderService.createRecordFolder(recordCategory, GUID.generate());
-                for (int i = 0; i < RECORD_COUNT; i++)
-                {
-                    records.add(recordService.createRecordFromContent(recordFolder, GUID.generate(), ContentModel.TYPE_CONTENT, null, null));
-                }
-                
-                // add records to hold
-                holdService.addToHold(hold, records);
-            }
-            
-            public void when() throws Exception
-            {      
-                // remove all of the records
-                holdService.removeFromHold(hold, records);
-            }
-            
-            public void then()
-            {
-                // check record state (no longer held)   
-                for (NodeRef record : records)
-                {                
-                    assertFalse(freezeService.isFrozen(record));
-                    assertFalse(holdService.getHeld(hold).contains(record));
-                    assertFalse(holdService.heldBy(record, true).contains(hold));
-                }
-                
-                // record folder has frozen children
-                assertFalse(freezeService.isFrozen(recordFolder));
-                assertFalse(freezeService.hasFrozenChildren(recordFolder));
-                
-                // record folder is not held
-                assertFalse(holdService.getHeld(hold).contains(recordFolder));
-                assertFalse(holdService.heldBy(recordFolder, true).contains(hold));
-                
-                // additional check for child held caching
-                assertTrue(nodeService.hasAspect(recordFolder, ASPECT_HELD_CHILDREN));
-                assertEquals(0, nodeService.getProperty(recordFolder, PROP_HELD_CHILDREN_COUNT));
-            }            
-        });         
-    }
-    
-    public void testRemoveRecordFolderFromHold()
-    {
-        doBehaviourDrivenTest(new BehaviourDrivenTest()
-        {   
-            private NodeRef hold;
-            private NodeRef recordCategory;
-            private NodeRef recordFolder;
-            private List<NodeRef> records = new ArrayList<>(RECORD_COUNT);
-            
-            public void given()
-            {   
-                // create a hold
-                hold = holdService.createHold(filePlan, GUID.generate(), GUID.generate(), GUID.generate());
-                
-                // create a record folder that contains records  
-                recordCategory = filePlanService.createRecordCategory(filePlan, GUID.generate());
-                recordFolder = recordFolderService.createRecordFolder(recordCategory, GUID.generate());
-                for (int i = 0; i < RECORD_COUNT; i++)
-                {
-                    records.add(recordService.createRecordFromContent(recordFolder, GUID.generate(), ContentModel.TYPE_CONTENT, null, null));
-                }
-                
-                // add record folder to hold
                 holdService.addToHold(hold, recordFolder);
             }
 
-            public void when() throws Exception
-            {      
-                // remove record folder from hold
-                holdService.removeFromHold(hold, recordFolder);
-            }
-            
             public void then()
             {
-                // check record states   
+                assertTrue(beforeAddToHoldFlag);
-                for (NodeRef record : records)
+                assertTrue(onAddToHoldFlag);
-                {                
-                    assertFalse(freezeService.isFrozen(record));
-                    assertFalse(holdService.getHeld(hold).contains(record));
-                    assertFalse(holdService.heldBy(record, true).contains(hold));
             }
 
-                // record folder has frozen children
+            public void after()
-                assertFalse(freezeService.isFrozen(recordFolder));
+            {
-                assertFalse(freezeService.hasFrozenChildren(recordFolder));
+                policyComponent.removeClassDefinition(beforeAddToHoldBehaviour);
-                
+                policyComponent.removeClassDefinition(onAddToHoldBehaviour);
-                // record folder is not held
-                assertFalse(holdService.getHeld(hold).contains(recordFolder));
-                assertFalse(holdService.heldBy(recordFolder, true).contains(hold));
-                
-                // additional check for child held caching
-                assertTrue(nodeService.hasAspect(recordFolder, ASPECT_HELD_CHILDREN));
-                assertEquals(0, nodeService.getProperty(recordFolder, PROP_HELD_CHILDREN_COUNT));
             }
         });
     }
+
+    @Override
+    public void beforeAddToHold(NodeRef hold, NodeRef contentNodeRef)
+    {
+        beforeAddToHoldFlag = true;
+    }
+
+    @Override
+    public void onAddToHold(NodeRef hold, NodeRef contentNodeRef)
+    {
+        onAddToHoldFlag = true;
+    }
 }

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

@@ -0,0 +1,298 @@
+/*
+ * #%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 java.util.ArrayList;
+import java.util.List;
+
+import org.alfresco.model.ContentModel;
+import org.alfresco.module.org_alfresco_module_rm.hold.HoldServicePolicies.BeforeRemoveFromHoldPolicy;
+import org.alfresco.module.org_alfresco_module_rm.hold.HoldServicePolicies.OnRemoveFromHoldPolicy;
+import org.alfresco.module.org_alfresco_module_rm.model.RecordsManagementModel;
+import org.alfresco.module.org_alfresco_module_rm.test.util.BaseRMTestCase;
+import org.alfresco.repo.policy.Behaviour.NotificationFrequency;
+import org.alfresco.repo.policy.BehaviourDefinition;
+import org.alfresco.repo.policy.ClassBehaviourBinding;
+import org.alfresco.repo.policy.JavaBehaviour;
+import org.alfresco.service.cmr.repository.NodeRef;
+import org.springframework.extensions.webscripts.GUID;
+
+/**
+ * Remove From Hold integration tests.
+ *
+ * @author Roy Wetherall
+ * @since 2.2
+ */
+public class RemoveFromHoldTest extends BaseRMTestCase implements BeforeRemoveFromHoldPolicy, OnRemoveFromHoldPolicy
+{
+    private static final int RECORD_COUNT = 10;
+
+    private boolean beforeRemoveFromHoldFlag = false;
+    private boolean onRemoveFromHoldFlag = false;
+
+    public void testRemoveRecordsFromHold()
+    {
+        doBehaviourDrivenTest(new BehaviourDrivenTest()
+        {
+            private NodeRef hold;
+            private NodeRef recordCategory;
+            private NodeRef recordFolder;
+            private List<NodeRef> records = new ArrayList<>(RECORD_COUNT);
+
+            public void given()
+            {
+                // create a hold
+                hold = holdService.createHold(filePlan, GUID.generate(), GUID.generate(), GUID.generate());
+
+                // create a record folder that contains records  
+                recordCategory = filePlanService.createRecordCategory(filePlan, GUID.generate());
+                recordFolder = recordFolderService.createRecordFolder(recordCategory, GUID.generate());
+                for (int i = 0; i < RECORD_COUNT; i++)
+                {
+                    records.add(recordService.createRecordFromContent(recordFolder, GUID.generate(), ContentModel.TYPE_CONTENT, null, null));
+                }
+
+                // add records to hold
+                holdService.addToHold(hold, records);
+            }
+
+            public void when() throws Exception
+            {
+                // remove *some* of the records
+                holdService.removeFromHold(hold, records.subList(0, 5));
+            }
+
+            public void then()
+            {
+                // check record state (no longer held)   
+                for (NodeRef record : records.subList(0, 5))
+                {
+                    assertFalse(freezeService.isFrozen(record));
+                    assertFalse(holdService.getHeld(hold).contains(record));
+                    assertFalse(holdService.heldBy(record, true).contains(hold));
+                }
+
+                // check record state (still held)   
+                for (NodeRef record : records.subList(5, 10))
+                {
+                    assertTrue(freezeService.isFrozen(record));
+                    assertTrue(holdService.getHeld(hold).contains(record));
+                    assertTrue(holdService.heldBy(record, true).contains(hold));
+                }
+
+                // record folder has frozen children
+                assertFalse(freezeService.isFrozen(recordFolder));
+                assertTrue(freezeService.hasFrozenChildren(recordFolder));
+
+                // record folder is not held
+                assertFalse(holdService.getHeld(hold).contains(recordFolder));
+                assertFalse(holdService.heldBy(recordFolder, true).contains(hold));
+
+                // additional check for child held caching
+                assertTrue(nodeService.hasAspect(recordFolder, ASPECT_HELD_CHILDREN));
+                assertEquals(5, nodeService.getProperty(recordFolder, PROP_HELD_CHILDREN_COUNT));
+            }
+        });
+    }
+
+    public void testRemoveAllRecordsFromHold()
+    {
+        doBehaviourDrivenTest(new BehaviourDrivenTest()
+        {
+            private NodeRef hold;
+            private NodeRef recordCategory;
+            private NodeRef recordFolder;
+            private List<NodeRef> records = new ArrayList<>(RECORD_COUNT);
+
+            public void given()
+            {
+                // create a hold
+                hold = holdService.createHold(filePlan, GUID.generate(), GUID.generate(), GUID.generate());
+
+                // create a record folder that contains records  
+                recordCategory = filePlanService.createRecordCategory(filePlan, GUID.generate());
+                recordFolder = recordFolderService.createRecordFolder(recordCategory, GUID.generate());
+                for (int i = 0; i < RECORD_COUNT; i++)
+                {
+                    records.add(recordService.createRecordFromContent(recordFolder, GUID.generate(), ContentModel.TYPE_CONTENT, null, null));
+                }
+
+                // add records to hold
+                holdService.addToHold(hold, records);
+            }
+
+            public void when() throws Exception
+            {
+                // remove all of the records
+                holdService.removeFromHold(hold, records);
+            }
+
+            public void then()
+            {
+                // check record state (no longer held)   
+                for (NodeRef record : records)
+                {
+                    assertFalse(freezeService.isFrozen(record));
+                    assertFalse(holdService.getHeld(hold).contains(record));
+                    assertFalse(holdService.heldBy(record, true).contains(hold));
+                }
+
+                // record folder has frozen children
+                assertFalse(freezeService.isFrozen(recordFolder));
+                assertFalse(freezeService.hasFrozenChildren(recordFolder));
+
+                // record folder is not held
+                assertFalse(holdService.getHeld(hold).contains(recordFolder));
+                assertFalse(holdService.heldBy(recordFolder, true).contains(hold));
+
+                // additional check for child held caching
+                assertTrue(nodeService.hasAspect(recordFolder, ASPECT_HELD_CHILDREN));
+                assertEquals(0, nodeService.getProperty(recordFolder, PROP_HELD_CHILDREN_COUNT));
+            }
+        });
+    }
+
+    public void testRemoveRecordFolderFromHold()
+    {
+        doBehaviourDrivenTest(new BehaviourDrivenTest()
+        {
+            private NodeRef hold;
+            private NodeRef recordCategory;
+            private NodeRef recordFolder;
+            private List<NodeRef> records = new ArrayList<>(RECORD_COUNT);
+
+            public void given()
+            {
+                // create a hold
+                hold = holdService.createHold(filePlan, GUID.generate(), GUID.generate(), GUID.generate());
+
+                // create a record folder that contains records  
+                recordCategory = filePlanService.createRecordCategory(filePlan, GUID.generate());
+                recordFolder = recordFolderService.createRecordFolder(recordCategory, GUID.generate());
+                for (int i = 0; i < RECORD_COUNT; i++)
+                {
+                    records.add(recordService.createRecordFromContent(recordFolder, GUID.generate(), ContentModel.TYPE_CONTENT, null, null));
+                }
+
+                // add record folder to hold
+                holdService.addToHold(hold, recordFolder);
+            }
+
+            public void when() throws Exception
+            {
+                // remove record folder from hold
+                holdService.removeFromHold(hold, recordFolder);
+            }
+
+            public void then()
+            {
+                // check record states   
+                for (NodeRef record : records)
+                {
+                    assertFalse(freezeService.isFrozen(record));
+                    assertFalse(holdService.getHeld(hold).contains(record));
+                    assertFalse(holdService.heldBy(record, true).contains(hold));
+                }
+
+                // record folder has frozen children
+                assertFalse(freezeService.isFrozen(recordFolder));
+                assertFalse(freezeService.hasFrozenChildren(recordFolder));
+
+                // record folder is not held
+                assertFalse(holdService.getHeld(hold).contains(recordFolder));
+                assertFalse(holdService.heldBy(recordFolder, true).contains(hold));
+
+                // additional check for child held caching
+                assertTrue(nodeService.hasAspect(recordFolder, ASPECT_HELD_CHILDREN));
+                assertEquals(0, nodeService.getProperty(recordFolder, PROP_HELD_CHILDREN_COUNT));
+            }
+        });
+    }
+
+    public void testPolicyNotificationForRemoveFromHold()
+    {
+        doBehaviourDrivenTest(new BehaviourDrivenTest()
+        {
+            private NodeRef hold;
+            private NodeRef recordCategory;
+            private NodeRef recordFolder;
+            BehaviourDefinition<ClassBehaviourBinding> beforeRemoveFromHoldBehaviour;
+            BehaviourDefinition<ClassBehaviourBinding> onRemoveFromHoldBehaviour;
+
+            public void given()
+            {
+                // create a hold
+                hold = holdService.createHold(filePlan, GUID.generate(), GUID.generate(), GUID.generate());
+                // create a record category -> record folder
+                recordCategory = filePlanService.createRecordCategory(filePlan, GUID.generate());
+                recordFolder = recordFolderService.createRecordFolder(recordCategory, GUID.generate());
+                // add the record folder to hold
+                holdService.addToHold(hold, recordFolder);
+
+                beforeRemoveFromHoldBehaviour = policyComponent.bindClassBehaviour(BeforeRemoveFromHoldPolicy.QNAME,
+                        RecordsManagementModel.TYPE_HOLD, new JavaBehaviour(RemoveFromHoldTest.this, "beforeRemoveFromHold", NotificationFrequency.EVERY_EVENT));
+
+                onRemoveFromHoldBehaviour = policyComponent.bindClassBehaviour(OnRemoveFromHoldPolicy.QNAME,
+                        RecordsManagementModel.TYPE_HOLD, new JavaBehaviour(RemoveFromHoldTest.this, "onRemoveFromHold", NotificationFrequency.EVERY_EVENT));
+
+                assertFalse(beforeRemoveFromHoldFlag);
+                assertFalse(onRemoveFromHoldFlag);
+            }
+
+            public void when() throws Exception
+            {
+                // remove the record folder from the hold
+                holdService.removeFromHold(hold, recordFolder);
+            }
+
+            public void then()
+            {
+                assertTrue(beforeRemoveFromHoldFlag);
+                assertTrue(onRemoveFromHoldFlag);
+            }
+
+            public void after()
+            {
+                policyComponent.removeClassDefinition(beforeRemoveFromHoldBehaviour);
+                policyComponent.removeClassDefinition(onRemoveFromHoldBehaviour);
+            }
+        });
+    }
+
+    @Override
+    public void beforeRemoveFromHold(NodeRef hold, NodeRef contentNodeRef)
+    {
+        beforeRemoveFromHoldFlag = true;
+    }
+
+    @Override
+    public void onRemoveFromHold(NodeRef hold, NodeRef contentNodeRef)
+    {
+        onRemoveFromHoldFlag = true;
+    }
+}

rm-community/rm-community-repo/test/java/org/alfresco/module/org_alfresco_module_rm/test/legacy/service/RecordsManagementAuditServiceImplTest.java

@@ -40,6 +40,7 @@ import org.alfresco.module.org_alfresco_module_rm.audit.RecordsManagementAuditQu
 import org.alfresco.module.org_alfresco_module_rm.audit.RecordsManagementAuditService;
 import org.alfresco.module.org_alfresco_module_rm.audit.event.AuditEvent;
 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.BaseRMTestCase;
 import org.alfresco.repo.security.authentication.AuthenticationException;
 import org.alfresco.repo.security.authentication.AuthenticationUtil;
@@ -60,6 +61,9 @@ import org.alfresco.util.Pair;
 public class RecordsManagementAuditServiceImplTest extends BaseRMTestCase
                                                    implements RMPermissionModel
 {
+    /** A QName to display for the hold name. */
+    public static final QName HOLD_NAME = QName.createQName(RecordsManagementModel.RM_URI, "Hold Name");
+
     /** Test record */
     private NodeRef record;
 
@@ -606,7 +610,7 @@ public class RecordsManagementAuditServiceImplTest extends BaseRMTestCase
             {
                 // check create hold audit event includes the hold name
                 assertEquals("Create Hold event does not include hold name.", holdName,
-                    auditEventProperties.get(PROP_NAME));
+                    auditEventProperties.get(HOLD_NAME));
 
                 // check create hold audit event includes the hold reason
                 assertEquals("Create Hold event does not include hold reason.", holdReason,

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

@@ -71,7 +71,6 @@ import org.junit.Before;
 import org.junit.Test;
 import org.mockito.ArgumentCaptor;
 import org.mockito.InjectMocks;
-import org.mockito.Matchers;
 import org.mockito.Mock;
 import org.mockito.Spy;
 import org.mockito.invocation.InvocationOnMock;
@@ -340,6 +339,8 @@ public class HoldServiceImplUnitTest extends BaseUnitTest
     @Test
     public void addToHoldNotInHold()
     {
+        mockPoliciesForAddToHold();
+
         holdService.addToHold(hold, recordFolder);
 
         verify(mockedNodeService).addChild(hold, recordFolder, ASSOC_FROZEN_CONTENT, ASSOC_FROZEN_CONTENT);
@@ -387,6 +388,8 @@ public class HoldServiceImplUnitTest extends BaseUnitTest
         doReturn(true).when(mockedNodeService).hasAspect(record, ASPECT_FROZEN);
         doReturn(true).when(mockedNodeService).hasAspect(activeContent, ASPECT_FROZEN);
 
+        mockPoliciesForAddToHold();
+
         holdService.addToHold(hold, recordFolder);
 
         verify(mockedNodeService).addChild(hold, recordFolder, ASSOC_FROZEN_CONTENT, ASSOC_FROZEN_CONTENT);
@@ -448,6 +451,8 @@ public class HoldServiceImplUnitTest extends BaseUnitTest
 
         }).when(mockedNodeService).addAspect(any(NodeRef.class), eq(ASPECT_FROZEN), any(Map.class));
 
+        mockPoliciesForAddToHold();
+
         // build a list of holds
         List<NodeRef> holds = new ArrayList<>(2);
         holds.add(hold);
@@ -473,6 +478,8 @@ public class HoldServiceImplUnitTest extends BaseUnitTest
     @Test
     public void removeFromHoldNotInHold()
     {
+        mockPoliciesForRemoveFromHold();
+
         holdService.removeFromHold(hold, recordFolder);
 
         verify(mockedNodeService, never()).removeChild(hold, recordFolder);
@@ -488,6 +495,8 @@ public class HoldServiceImplUnitTest extends BaseUnitTest
         doReturn(true).when(mockedNodeService).hasAspect(recordFolder, ASPECT_FROZEN);
         doReturn(true).when(mockedNodeService).hasAspect(record, ASPECT_FROZEN);
 
+        mockPoliciesForRemoveFromHold();
+
         holdService.removeFromHold(hold, recordFolder);
 
         verify(mockedNodeService, times(1)).removeChild(hold, recordFolder);
@@ -504,6 +513,8 @@ public class HoldServiceImplUnitTest extends BaseUnitTest
         doReturn(true).when(mockedNodeService).hasAspect(recordFolder, ASPECT_FROZEN);
         doReturn(true).when(mockedNodeService).hasAspect(record, ASPECT_FROZEN);
 
+        mockPoliciesForRemoveFromHold();
+
         // build a list of holds
         List<NodeRef> holds = new ArrayList<>(2);
         holds.add(hold);
@@ -536,6 +547,8 @@ public class HoldServiceImplUnitTest extends BaseUnitTest
 
         }).when(mockedNodeService).removeChild(hold, recordFolder);
 
+        mockPoliciesForRemoveFromHold();
+
         doAnswer(new Answer<Void>()
         {
             public Void answer(InvocationOnMock invocation)
@@ -576,4 +589,22 @@ public class HoldServiceImplUnitTest extends BaseUnitTest
         holds.add(hold2);
         holdService.removeFromHolds(holds, activeContent);
     }
+
+    /**
+     * mocks policies for add to hold
+     */
+    private void mockPoliciesForAddToHold()
+    {
+        doNothing().when(holdService).invokeBeforeAddToHold(any(), any());
+        doNothing().when(holdService).invokeOnAddToHold(any(), any());
+    }
+
+    /**
+     * mocks policies for remove from hold
+     */
+    private void mockPoliciesForRemoveFromHold()
+    {
+        doNothing().when(holdService).invokeBeforeRemoveFromHold(any(), any());
+        doNothing().when(holdService).invokeOnRemoveFromHold(any(), any());
+    }
 }

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

@@ -58,6 +58,7 @@ public class PropertyModificationAllowedCheckUnitTest
     private QName qName, qName2;
 
     private List<QName> list;
+    private List<String> editableURIs;
 
     @Mock
     private Serializable serializable, serializable2;
@@ -74,6 +75,9 @@ public class PropertyModificationAllowedCheckUnitTest
         before.put(qName, serializable);
         after.put(qName, serializable2);
         list = new ArrayList();
+        editableURIs = new ArrayList<>();
+        propertyModificationAllowedCheck.setWhiteList(list);
+        propertyModificationAllowedCheck.setEditableURIs(editableURIs);
     }
 
     /**
@@ -232,4 +236,26 @@ public class PropertyModificationAllowedCheckUnitTest
         after.put(qName, null);
         assertTrue(propertyModificationAllowedCheck.check(before, after));
     }
+
+    /**
+     * Test update of a property from the model URI for which properties can be updated
+     */
+    @Test
+    public void testUpdatePropertyFromAllowedModelURI()
+    {
+        editableURIs.add("foo");
+        propertyModificationAllowedCheck.setEditableURIs(editableURIs);
+        assertTrue(propertyModificationAllowedCheck.check(before, after));
+    }
+
+    /**
+     * Test update of a property that is not in the model URI for which properties can be updated
+     */
+    @Test
+    public void testUpdatePropertyFromNotAllowedModelURI()
+    {
+        editableURIs.add("bar");
+        propertyModificationAllowedCheck.setEditableURIs(editableURIs);
+        assertFalse(propertyModificationAllowedCheck.check(before, after));
+    }
 }