RM-1424 (Holds the user has read-only permission for are displayed in Add to Hold dialog)

git-svn-id: https://svn.alfresco.com/repos/alfresco-enterprise/modules/recordsmanagement/HEAD@72241 c4b6b30b-aa2e-2d43-bbcb-ca4b014f7261

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

@@ -218,7 +218,8 @@ public class HoldServiceImpl extends ServiceBaseImpl
         for (ChildAssociationRef holdAssoc : holdsAssocs)
         {
             NodeRef hold = holdAssoc.getChildRef();
-            if (isHold(hold))
+            boolean hasPermissionOnHold = (permissionService.hasPermission(hold, RMPermissionModel.FILING) == AccessStatus.ALLOWED);
+            if (isHold(hold) && hasPermissionOnHold)
             {
                 // add to list of holds
                 holds.add(hold);

rm-server/test/java/org/alfresco/module/org_alfresco_module_rm/test/service/HoldServiceImplTest.java

@@ -18,10 +18,12 @@
  */
 package org.alfresco.module.org_alfresco_module_rm.test.service;
 
+import java.util.ArrayList;
 import java.util.List;
 
 import org.alfresco.error.AlfrescoRuntimeException;
 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.service.cmr.repository.NodeRef;
 
@@ -33,6 +35,14 @@ import org.alfresco.service.cmr.repository.NodeRef;
  */
 public class HoldServiceImplTest extends BaseRMTestCase
 {
+    /** Constants for the holds */
+    private static final String HOLD1_NAME = "hold one";
+    private static final String HOLD2_NAME = "hold two";
+    private static final String HOLD1_REASON = "I have my reasons";
+    private static final String HOLD2_REASON = "secrets are everything";
+    private static final String HOLD1_DESC = "but I'll not describe them here!";
+    private static final String HOLD2_DESC = "no then! that's just not on!";
+
     @Override
     protected boolean isRecordTest()
     {
@@ -45,6 +55,34 @@ public class HoldServiceImplTest extends BaseRMTestCase
         return true;
     }
 
+    /**
+     * Creates a hold and checks if the hold is null or not
+     *
+     * @return {@link NodeRef} Node reference of the hold.
+     */
+    private NodeRef createAndCheckHold()
+    {
+        NodeRef hold = holdService.createHold(filePlan, HOLD1_NAME, HOLD1_REASON, HOLD1_DESC);
+        assertNotNull(hold);
+        return hold;
+    }
+
+    /**
+     * Creates two holds and checks them if they are null or not
+     *
+     * @return List of {@link NodeRef}s of the holds.
+     */
+    private List<NodeRef> createAndCheckHolds()
+    {
+        List<NodeRef> holds = new ArrayList<NodeRef>(2);
+        holds.add(createAndCheckHold());
+        NodeRef hold2 = holdService.createHold(filePlan, HOLD2_NAME, HOLD2_REASON, HOLD2_DESC);
+        assertNotNull(hold2);
+        holds.add(hold2);
+        assertEquals(2, holds.size());
+        return holds;
+    }
+
     public void testDeleteHoldBehaviourForRecordFolder()
     {
         doTestInTransaction(new Test<Void>()
@@ -53,8 +91,7 @@ public class HoldServiceImplTest extends BaseRMTestCase
            public Void run() throws Exception
            {
                // create test holds
-               NodeRef hold1 = holdService.createHold(filePlan, "hold one", "I have my reasons", "but I'll not describe them here!");
+               NodeRef hold1 = createAndCheckHold();
-               assertNotNull(hold1);
 
                // add the record folder to hold1
                holdService.addToHold(hold1, rmFolder);
@@ -94,10 +131,9 @@ public class HoldServiceImplTest extends BaseRMTestCase
            public Void run() throws Exception
            {
                // create test holds
-               NodeRef hold1 = holdService.createHold(filePlan, "hold one", "I have my reasons", "but I'll not describe them here!");
+               List<NodeRef> holds = createAndCheckHolds();
-               assertNotNull(hold1);
+               NodeRef hold1 = holds.get(0);
-               NodeRef hold2 = holdService.createHold(filePlan, "hold two", "secrets are everything", "no then! that's just not on!");
+               NodeRef hold2 = holds.get(1);
-               assertNotNull(hold2);
 
                // add the record folder to hold1
                holdService.addToHold(hold1, rmFolder);
@@ -150,8 +186,7 @@ public class HoldServiceImplTest extends BaseRMTestCase
     public void testAddRecordFolderToHoldWithoutFilingPermissionOnRecordFolder()
     {
         // Create hold
-        final NodeRef hold = holdService.createHold(filePlan, "hold one", "I have my reasons", "but I'll not describe them here!");
+        final NodeRef hold = createAndCheckHold();
-        assertNotNull(hold);
 
         doTestInTransaction(new Test<Void>()
         {
@@ -169,7 +204,7 @@ public class HoldServiceImplTest extends BaseRMTestCase
 
                return null;
            }
-        }, "admin");
+        });
 
         doTestInTransaction(new FailureTest(AlfrescoRuntimeException.class)
         {
@@ -184,8 +219,7 @@ public class HoldServiceImplTest extends BaseRMTestCase
     public void testAddRecordFolderToHoldWithoutFilingPermissionOnHold()
     {
         // Create hold
-        final NodeRef hold = holdService.createHold(filePlan, "hold one", "I have my reasons", "but I'll not describe them here!");
+        final NodeRef hold = createAndCheckHold();
-        assertNotNull(hold);
 
         doTestInTransaction(new Test<Void>()
         {
@@ -203,7 +237,7 @@ public class HoldServiceImplTest extends BaseRMTestCase
 
                return null;
            }
-        }, "admin");
+        });
 
         doTestInTransaction(new FailureTest(AlfrescoRuntimeException.class)
         {
@@ -215,4 +249,115 @@ public class HoldServiceImplTest extends BaseRMTestCase
         }, userName);
     }
 
+    public void testGettingHolds()
+    {
+        final List<NodeRef> listWithTwoHolds = new ArrayList<NodeRef>(2);
+
+        doTestInTransaction(new Test<Void>()
+        {
+           @Override
+           public Void run() throws Exception
+           {
+               // No holds
+               List<NodeRef> emptyHoldList = holdService.getHolds(filePlan);
+               assertNotNull(emptyHoldList);
+               assertTrue(emptyHoldList.isEmpty());
+
+               // Create 2 holds
+               createAndCheckHolds();
+
+               // Check the list of holds
+               listWithTwoHolds.addAll(holdService.getHolds(filePlan));
+               assertNotNull(listWithTwoHolds);
+               assertEquals(2, listWithTwoHolds.size());
+
+               // Check the first hold
+               NodeRef hold1 = listWithTwoHolds.get(0);
+               assertEquals(RecordsManagementModel.TYPE_HOLD, nodeService.getType(hold1));
+               assertEquals(HOLD1_NAME, (String) nodeService.getProperty(hold1, PROP_NAME));
+               assertEquals(HOLD1_REASON, (String) nodeService.getProperty(hold1, PROP_HOLD_REASON));
+               assertEquals(HOLD1_DESC, (String) nodeService.getProperty(hold1, PROP_DESCRIPTION));
+
+               // Add the user to the RM Manager role
+               filePlanRoleService.assignRoleToAuthority(filePlan, ROLE_NAME_RECORDS_MANAGER, userName);
+
+               return null;
+           }
+        });
+
+        doTestInTransaction(new Test<Void>()
+        {
+            @Override
+            public Void run() throws Exception
+            {
+                // Get the holds the test user without having any permissions on the holds
+                List<NodeRef> holds = holdService.getHolds(filePlan);
+                assertNotNull(holds);
+                assertEquals(0, holds.size());
+
+                return null;
+            }
+        }, userName);
+
+        final NodeRef hold2 = listWithTwoHolds.get(1);
+        doTestInTransaction(new Test<Void>()
+        {
+           @Override
+           public Void run() throws Exception
+           {
+               // Give the user read permissions on the hold
+               permissionService.setPermission(hold2, userName, RMPermissionModel.FILING, true);
+
+               return null;
+           }
+        });
+
+        doTestInTransaction(new Test<Void>()
+        {
+            @Override
+            public Void run() throws Exception
+            {
+                List<NodeRef> holds = holdService.getHolds(filePlan);
+                assertNotNull(holds);
+                assertEquals(1, holds.size());
+                assertEquals(RecordsManagementModel.TYPE_HOLD, nodeService.getType(hold2));
+                assertEquals(HOLD2_NAME, (String) nodeService.getProperty(hold2, PROP_NAME));
+                assertEquals(HOLD2_REASON, (String) nodeService.getProperty(hold2, PROP_HOLD_REASON));
+                assertEquals(HOLD2_DESC, (String) nodeService.getProperty(hold2, PROP_DESCRIPTION));
+
+                return null;
+            }
+        }, userName);
+    }
+
+    public void testHeldByNothing()
+    {
+        doTestInTransaction(new Test<Void>()
+        {
+           @Override
+           public Void run() throws Exception
+           {
+               // Create the test holds
+               createAndCheckHolds();
+
+               // Check that the record folder isn't held by anything
+               List<NodeRef> holds = new ArrayList<NodeRef>();
+               holds.addAll(holdService.heldBy(rmFolder, true));
+               assertTrue(holds.isEmpty());
+               holds.clear();
+               holds.addAll(holdService.heldBy(rmFolder, false));
+               assertEquals(2, holds.size());
+
+               // Check that record isn't held by anything (recordOne is a child of the rmFolder)
+               holds.clear();
+               holds.addAll(holdService.heldBy(recordOne, true));
+               assertTrue(holds.isEmpty());
+               holds.clear();
+               holds.addAll(holdService.heldBy(recordOne, false));
+               assertEquals(2, holds.size());
+
+               return null;
+           }
+        });
+    }
 }

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

@@ -71,7 +71,7 @@ public class HoldServiceImplUnitTest extends BaseUnitTest
 
     protected NodeRef holdContainer;
     protected NodeRef hold;
-    protected NodeRef hold2;    
+    protected NodeRef hold2;
 
     @Spy @InjectMocks HoldServiceImpl holdService;
 
@@ -88,8 +88,6 @@ public class HoldServiceImplUnitTest extends BaseUnitTest
 
         // setup interactions
         doReturn(holdContainer).when(mockedFilePlanService).getHoldContainer(filePlan);
-
-        
     }
 
     @Test
@@ -99,56 +97,6 @@ public class HoldServiceImplUnitTest extends BaseUnitTest
         assertFalse(holdService.isHold(recordFolder));
     }
 
-    @Test
-    public void getHolds()
-    {
-        // with no holds
-        List<NodeRef> emptyHoldList = holdService.getHolds(filePlan);
-        verify(mockedNodeService).getChildAssocs(holdContainer, ContentModel.ASSOC_CONTAINS, RegexQNamePattern.MATCH_ALL);
-        assertNotNull(emptyHoldList);
-        assertTrue(emptyHoldList.isEmpty());
-
-        // setup holds (hold1 and hold 2)
-        setupHolds();
-
-        // with 2 holds
-        List<NodeRef> holdsList = holdService.getHolds(filePlan);
-        verify(mockedNodeService, times(2)).getChildAssocs(holdContainer, ContentModel.ASSOC_CONTAINS, RegexQNamePattern.MATCH_ALL);
-        assertNotNull(holdsList);
-        assertEquals(2, holdsList.size());
-
-        // check one of the holds
-        NodeRef holdFromList = holdsList.get(0);
-        assertEquals(TYPE_HOLD, mockedNodeService.getType(holdFromList));
-    }
-
-    private void setupHolds()
-    {
-        // set up list of two holds
-        List<ChildAssociationRef> list = new ArrayList<ChildAssociationRef>(2);
-        list.add(new ChildAssociationRef(generateQName(), holdContainer, generateQName(), hold, true, 1));
-        list.add(new ChildAssociationRef(generateQName(), holdContainer, generateQName(), hold2, true, 2));
-        when(mockedNodeService.getChildAssocs(holdContainer, ContentModel.ASSOC_CONTAINS, RegexQNamePattern.MATCH_ALL)).thenReturn(list);
-    }
-
-    @Test
-    public void heldByNothing()
-    {
-        // setup interactions
-        doReturn(new ArrayList<ChildAssociationRef>()).when(mockedNodeService).getParentAssocs(recordFolder, ASSOC_FROZEN_RECORDS, ASSOC_FROZEN_RECORDS);
-        setupHolds();
-
-        // check that the record folder isn't held by anything
-        assertTrue(holdService.heldBy(recordFolder, true).isEmpty());
-        List<NodeRef> holds = holdService.heldBy(recordFolder, false);
-        assertEquals(2, holds.size());
-
-        // check that the record isn't held by anything (record is a child of the record folder)
-        assertTrue(holdService.heldBy(record, true).isEmpty());
-        holds = holdService.heldBy(record, false);
-        assertEquals(2, holds.size());
-    }
-
     @Test
     public void heldByMultipleResults()
     {
@@ -357,7 +305,6 @@ public class HoldServiceImplUnitTest extends BaseUnitTest
         verify(mockedNodeService, never()).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));
-
     }
 
     @SuppressWarnings("unchecked")