code review comments

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

@@ -303,6 +303,8 @@ public class HoldServiceImpl extends ServiceBaseImpl
         if (!includedInHold)
         {
             // invert list to get list of holds that do not contain this node
+            //TODO Find a way to get rid of the isFilePlanComponent(nodeRef) check. Currently it is used because
+            // integration tests create multiple rm sites with generated ids
             NodeRef filePlan = isFilePlanComponent(nodeRef) ? filePlanService.getFilePlan(nodeRef) : filePlanService.getFilePlanBySiteId(FilePlanService.DEFAULT_RM_SITE_ID);
             List<NodeRef> allHolds = getHolds(filePlan);
             result = ListUtils.subtract(allHolds, new ArrayList<>(holdsNotIncludingNodeRef));

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

@@ -59,6 +59,14 @@ public class AddActiveContentToHoldTest extends BaseRMTestCase
         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()
@@ -111,6 +119,12 @@ public class AddActiveContentToHoldTest extends BaseRMTestCase
         });
     }
 
+    /**
+     * 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)
@@ -141,6 +155,12 @@ public class AddActiveContentToHoldTest extends BaseRMTestCase
         });
     }
 
+    /**
+     * 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()
@@ -183,9 +203,15 @@ public class AddActiveContentToHoldTest extends BaseRMTestCase
         });
     }
 
+    /**
+     * 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(recordsManagerName, false)
+        doBehaviourDrivenTest(new BehaviourDrivenTest(AccessDeniedException.class, recordsManagerName, false)
         {
             private NodeRef hold;
 
@@ -213,28 +239,22 @@ public class AddActiveContentToHoldTest extends BaseRMTestCase
 
             public void when()
             {
-                AuthenticationUtil.runAs(
+                // add the active content to hold as a RM manager who has Add to Hold Capability and write permission on
-                        (RunAsWork<Void>) () -> {
+                // doc, but no filing permission on hold
-                            // add the active content to hold as a RM manager who has Add to Hold Capability and write permission on
+                holdService.addToHold(hold, dmDocument);
-                            // doc, but no filing permission on hold
-                            try
-                            {
-                                holdService.addToHold(hold, dmDocument);
-                                fail("Expected AccessDeniedException to be thrown.");
-                            }
-                            catch (AccessDeniedException e)
-                            {
-                                //expected
-                            }
-                            return null;
-                        }, recordsManagerName);
             }
         });
     }
 
+    /**
+     * 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(powerUserName, false)
+        doBehaviourDrivenTest(new BehaviourDrivenTest(AccessDeniedException.class, powerUserName, false)
         {
             private NodeRef hold;
 
@@ -264,19 +284,7 @@ public class AddActiveContentToHoldTest extends BaseRMTestCase
             {
                 // add the active content to hold as a RM power user who has write permission on doc and filing
                 // permission on hold, but no Add To Hold capability
-                AuthenticationUtil.runAs(
+                holdService.addToHold(hold, dmDocument);
-                        (RunAsWork<Void>) () -> {
-                            try
-                            {
-                                holdService.addToHold(hold, dmDocument);
-                                fail("Expected AccessDeniedException to be thrown.");
-                            }
-                            catch (AccessDeniedException e)
-                            {
-                                //expected
-                            }
-                            return null;
-                        }, powerUserName);
             }
         });
     }

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

@@ -922,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

@@ -64,7 +64,9 @@ import org.alfresco.service.namespace.NamespaceService;
 import org.alfresco.service.namespace.QName;
 import org.alfresco.service.namespace.RegexQNamePattern;
 import org.junit.Before;
+import org.junit.Rule;
 import org.junit.Test;
+import org.junit.rules.ExpectedException;
 import org.mockito.ArgumentCaptor;
 import org.mockito.InjectMocks;
 import org.mockito.Spy;
@@ -83,6 +85,7 @@ public class HoldServiceImplUnitTest extends BaseUnitTest
     private static final String HOLD_NAME = "holdname";
     private static final String HOLD_REASON = "holdreason";
     private static final String HOLD_DESCRIPTION = "holddescription";
+    private static final String ACTIVE_CONTENT_NAME = "activeContentName";
 
     protected NodeRef holdContainer;
     protected NodeRef hold;
@@ -91,6 +94,9 @@ public class HoldServiceImplUnitTest extends BaseUnitTest
 
     @Spy @InjectMocks HoldServiceImpl holdService;
 
+    @Rule
+    public ExpectedException expectedEx = ExpectedException.none();
+
     @Before
     @Override
     public void before() throws Exception
@@ -376,16 +382,25 @@ public class HoldServiceImplUnitTest extends BaseUnitTest
         verify(mockedRecordsManagementAuditService).auditEvent(eq(activeContent), anyString());
     }
 
-    @Test (expected = AlfrescoRuntimeException.class)
+    @Test
     public void addActiveContentToHoldNoPermissionsOnHold()
     {
+        expectedEx.expect(AlfrescoRuntimeException.class);
+        expectedEx.expectMessage("'" + ACTIVE_CONTENT_NAME + "' can't be added to the hold container as filing permission for '" + HOLD_NAME + "' is needed.");
+
+        when(mockedNodeService.getProperty(activeContent, ContentModel.PROP_NAME)).thenReturn(ACTIVE_CONTENT_NAME);
+        when(mockedNodeService.getProperty(hold, ContentModel.PROP_NAME)).thenReturn(HOLD_NAME);
         when(mockedPermissionService.hasPermission(hold, RMPermissionModel.FILING)).thenReturn(AccessStatus.DENIED);
         holdService.addToHold(hold, activeContent);
     }
 
-    @Test (expected = AlfrescoRuntimeException.class)
+    @Test
     public void addActiveContentToHoldNoPermissionsOnContent()
     {
+        expectedEx.expect(AlfrescoRuntimeException.class);
+        expectedEx.expectMessage("Write permission on '" + ACTIVE_CONTENT_NAME + "' is needed.");
+
+        when(mockedNodeService.getProperty(activeContent, ContentModel.PROP_NAME)).thenReturn(ACTIVE_CONTENT_NAME);
         when(mockedPermissionService.hasPermission(activeContent, PermissionService.WRITE)).thenReturn(AccessStatus.DENIED);
         holdService.addToHold(hold, activeContent);
     }