From ae38657eac6758e85fe8e468cc9d9794110d5402 Mon Sep 17 00:00:00 2001 From: cagache Date: Tue, 23 Jul 2019 15:45:23 +0300 Subject: [PATCH 01/13] Allow active content to be added to a hold --- .../rm-service-context.xml | 2 +- .../hold/HoldServiceImpl.java | 6 ++- .../hold/HoldServiceImplUnitTest.java | 54 +++++++++++++++---- 3 files changed, 50 insertions(+), 12 deletions(-) diff --git a/rm-community/rm-community-repo/config/alfresco/module/org_alfresco_module_rm/rm-service-context.xml b/rm-community/rm-community-repo/config/alfresco/module/org_alfresco_module_rm/rm-service-context.xml index 7a903fc7b8..9377be06c2 100644 --- a/rm-community/rm-community-repo/config/alfresco/module/org_alfresco_module_rm/rm-service-context.xml +++ b/rm-community/rm-community-repo/config/alfresco/module/org_alfresco_module_rm/rm-service-context.xml @@ -1569,7 +1569,7 @@ org.alfresco.module.org_alfresco_module_rm.hold.HoldService.getHoldReason=RM.Read.0 org.alfresco.module.org_alfresco_module_rm.hold.HoldService.setHoldReason=RM_CAP.0.rma:filePlanComponent.EditHold org.alfresco.module.org_alfresco_module_rm.hold.HoldService.deleteHold=RM_CAP.0.rma:filePlanComponent.DeleteHold - org.alfresco.module.org_alfresco_module_rm.hold.HoldService.addToHold=RM_CAP.0.rma:filePlanComponent.AddToHold + org.alfresco.module.org_alfresco_module_rm.hold.HoldService.addToHold=RM_CAP.0.rma:filePlanComponent.AddToHold, ACL_NODE.1.sys:base.Write org.alfresco.module.org_alfresco_module_rm.hold.HoldService.addToHolds=RM_ALLOW org.alfresco.module.org_alfresco_module_rm.hold.HoldService.removeFromHold=RM_CAP.0.rma:filePlanComponent.RemoveFromHold org.alfresco.module.org_alfresco_module_rm.hold.HoldService.removeFromHolds=RM_ALLOW diff --git a/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/hold/HoldServiceImpl.java b/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/hold/HoldServiceImpl.java index a44f3c996b..046a0ca8d2 100644 --- a/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/hold/HoldServiceImpl.java +++ b/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/hold/HoldServiceImpl.java @@ -539,13 +539,15 @@ public class HoldServiceImpl extends ServiceBaseImpl ParameterCheck.mandatoryCollection("holds", holds); ParameterCheck.mandatory("nodeRef", nodeRef); - if (!isRecord(nodeRef) && !isRecordFolder(nodeRef)) + if (!isRecord(nodeRef) && !isRecordFolder(nodeRef) && !instanceOf(nodeRef, ContentModel.TYPE_CONTENT)) { String nodeName = (String) nodeService.getProperty(nodeRef, ContentModel.PROP_NAME); + // TODO error message needs to be changed 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) + if ((isRecord(nodeRef) || isRecordFolder(nodeRef)) && + 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."); diff --git a/rm-community/rm-community-repo/unit-test/java/org/alfresco/module/org_alfresco_module_rm/hold/HoldServiceImplUnitTest.java b/rm-community/rm-community-repo/unit-test/java/org/alfresco/module/org_alfresco_module_rm/hold/HoldServiceImplUnitTest.java index 78c34fde09..72faf25a86 100644 --- a/rm-community/rm-community-repo/unit-test/java/org/alfresco/module/org_alfresco_module_rm/hold/HoldServiceImplUnitTest.java +++ b/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,11 @@ 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.test.util.BaseUnitTest; import org.alfresco.service.cmr.repository.ChildAssociationRef; import org.alfresco.service.cmr.repository.NodeRef; +import org.alfresco.service.cmr.security.AccessStatus; import org.alfresco.service.namespace.NamespaceService; import org.alfresco.service.namespace.QName; import org.alfresco.service.namespace.RegexQNamePattern; @@ -81,6 +85,7 @@ public class HoldServiceImplUnitTest extends BaseUnitTest protected NodeRef holdContainer; protected NodeRef hold; protected NodeRef hold2; + protected NodeRef activeContent; @Spy @InjectMocks HoldServiceImpl holdService; @@ -95,6 +100,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); } @@ -165,13 +175,15 @@ public class HoldServiceImplUnitTest extends BaseUnitTest public void getHeldWithResults() { // setup record folder in hold - List holds = new ArrayList<>(1); + List 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 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" }) @@ -282,7 +294,7 @@ public class HoldServiceImplUnitTest extends BaseUnitTest } @Test (expected=AlfrescoRuntimeException.class) - public void addToHoldNotARecordFolderOrRecord() + public void addToHoldNotARecordFolderOrRecordOrActiveContent() { NodeRef anotherThing = generateNodeRef(TYPE_RECORD_CATEGORY); holdService.addToHold(hold, anotherThing); @@ -297,20 +309,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 +335,40 @@ 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 = AlfrescoRuntimeException.class) + public void addActiveContentToHoldNoPermissionsOnHold() + { + when(mockedPermissionService.hasPermission(hold, RMPermissionModel.FILING)).thenReturn(AccessStatus.DENIED); + holdService.addToHold(hold, activeContent); } @SuppressWarnings("unchecked") From 9578df5f1e8779b461e04d13c1b23dcd9988eb0b Mon Sep 17 00:00:00 2001 From: cagache Date: Wed, 24 Jul 2019 17:30:15 +0300 Subject: [PATCH 02/13] Add ASPECT_HELD_CHILDREN on active content parent --- .../rm-service-context.xml | 2 +- .../freeze/FreezeServiceImpl.java | 12 ++++++++---- .../hold/HoldServiceImpl.java | 13 ++++++++----- .../model/rma/aspect/FrozenAspect.java | 17 +++++++++-------- 4 files changed, 26 insertions(+), 18 deletions(-) diff --git a/rm-community/rm-community-repo/config/alfresco/module/org_alfresco_module_rm/rm-service-context.xml b/rm-community/rm-community-repo/config/alfresco/module/org_alfresco_module_rm/rm-service-context.xml index 9377be06c2..7a903fc7b8 100644 --- a/rm-community/rm-community-repo/config/alfresco/module/org_alfresco_module_rm/rm-service-context.xml +++ b/rm-community/rm-community-repo/config/alfresco/module/org_alfresco_module_rm/rm-service-context.xml @@ -1569,7 +1569,7 @@ org.alfresco.module.org_alfresco_module_rm.hold.HoldService.getHoldReason=RM.Read.0 org.alfresco.module.org_alfresco_module_rm.hold.HoldService.setHoldReason=RM_CAP.0.rma:filePlanComponent.EditHold org.alfresco.module.org_alfresco_module_rm.hold.HoldService.deleteHold=RM_CAP.0.rma:filePlanComponent.DeleteHold - org.alfresco.module.org_alfresco_module_rm.hold.HoldService.addToHold=RM_CAP.0.rma:filePlanComponent.AddToHold, ACL_NODE.1.sys:base.Write + org.alfresco.module.org_alfresco_module_rm.hold.HoldService.addToHold=RM_CAP.0.rma:filePlanComponent.AddToHold org.alfresco.module.org_alfresco_module_rm.hold.HoldService.addToHolds=RM_ALLOW org.alfresco.module.org_alfresco_module_rm.hold.HoldService.removeFromHold=RM_CAP.0.rma:filePlanComponent.RemoveFromHold org.alfresco.module.org_alfresco_module_rm.hold.HoldService.removeFromHolds=RM_ALLOW diff --git a/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/freeze/FreezeServiceImpl.java b/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/freeze/FreezeServiceImpl.java index 809094fc34..63a6c9cdb1 100644 --- a/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/freeze/FreezeServiceImpl.java +++ b/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; @@ -273,8 +276,9 @@ public class FreezeServiceImpl extends ServiceBaseImpl boolean result = false; - // check that we are dealing with a record folder - if (isRecordFolder(nodeRef)) + // check that we are dealing with a record folder or a collaboration folder + 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(); - if (childAssociationRef.isPrimary() && isRecord(record) && isFrozen(record)) + NodeRef childRef = childAssociationRef.getChildRef(); + if (childAssociationRef.isPrimary() && isFrozen(childRef)) { heldCount ++; } diff --git a/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/hold/HoldServiceImpl.java b/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/hold/HoldServiceImpl.java index 046a0ca8d2..7e769ec9b2 100644 --- a/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/hold/HoldServiceImpl.java +++ b/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/hold/HoldServiceImpl.java @@ -539,19 +539,23 @@ public class HoldServiceImpl extends ServiceBaseImpl ParameterCheck.mandatoryCollection("holds", holds); ParameterCheck.mandatory("nodeRef", nodeRef); + String nodeName = (String) nodeService.getProperty(nodeRef, ContentModel.PROP_NAME); if (!isRecord(nodeRef) && !isRecordFolder(nodeRef) && !instanceOf(nodeRef, ContentModel.TYPE_CONTENT)) { - String nodeName = (String) nodeService.getProperty(nodeRef, ContentModel.PROP_NAME); - // TODO error message needs to be changed - throw new AlfrescoRuntimeException("'" + nodeName + "' is neither a record nor a record folder. Only records or record folders can be added to a hold."); + throw new AlfrescoRuntimeException("'" + nodeName + "' is neither a record nor a record folder nor a document. " + + "Only records, record folders or active content can be added to a hold."); } if ((isRecord(nodeRef) || isRecordFolder(nodeRef)) && 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."); } + else if (instanceOf(nodeRef, ContentModel.TYPE_CONTENT) && + permissionService.hasPermission(nodeRef, PermissionService.WRITE) == AccessStatus.DENIED) + { + throw new AlfrescoRuntimeException("Write permission on '" + nodeName + "' is needed."); + } for (final NodeRef hold : holds) { @@ -563,7 +567,6 @@ public class HoldServiceImpl extends ServiceBaseImpl if (permissionService.hasPermission(hold, RMPermissionModel.FILING) == AccessStatus.DENIED) { - String nodeName = (String) nodeService.getProperty(nodeRef, ContentModel.PROP_NAME); 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."); } diff --git a/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/model/rma/aspect/FrozenAspect.java b/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/model/rma/aspect/FrozenAspect.java index ea933e5185..958b808000 100644 --- a/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/model/rma/aspect/FrozenAspect.java +++ b/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/model/rma/aspect/FrozenAspect.java @@ -27,6 +27,8 @@ package org.alfresco.module.org_alfresco_module_rm.model.rma.aspect; +import static org.alfresco.model.ContentModel.TYPE_CONTENT; + import java.util.List; import org.alfresco.module.org_alfresco_module_rm.fileplan.FilePlanService; @@ -148,25 +150,24 @@ 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() { @Override public Void doWork() { - if (nodeService.exists(record) && - isRecord(record)) + if (nodeService.exists(nodeRef) && (isRecord(nodeRef) || instanceOf(nodeRef, TYPE_CONTENT))) { - // get the owning record folder - NodeRef recordFolder = nodeService.getPrimaryParent(record).getParentRef(); + // get the owning nodeRef folder + NodeRef parentRef = nodeService.getPrimaryParent(nodeRef).getParentRef(); // check that the aspect has been added - if (nodeService.hasAspect(recordFolder, ASPECT_HELD_CHILDREN)) + if (nodeService.hasAspect(parentRef, ASPECT_HELD_CHILDREN)) { // increment current count - int currentCount = (Integer)nodeService.getProperty(recordFolder, PROP_HELD_CHILDREN_COUNT); + int currentCount = (Integer) nodeService.getProperty(parentRef, PROP_HELD_CHILDREN_COUNT); currentCount = currentCount + 1; - nodeService.setProperty(recordFolder, PROP_HELD_CHILDREN_COUNT, currentCount); + nodeService.setProperty(parentRef, PROP_HELD_CHILDREN_COUNT, currentCount); } } return null; From 3cc0f925ba6c4c96b54181737aafd26c8fe5b544 Mon Sep 17 00:00:00 2001 From: cagache Date: Thu, 25 Jul 2019 11:07:36 +0300 Subject: [PATCH 03/13] Add IT to check that active content can be added to a hold by admin user --- .../rm-service-context.xml | 2 +- .../hold/AddRemoveFromHoldTest.java | 60 ++++++++++++++++++- 2 files changed, 60 insertions(+), 2 deletions(-) diff --git a/rm-community/rm-community-repo/config/alfresco/module/org_alfresco_module_rm/rm-service-context.xml b/rm-community/rm-community-repo/config/alfresco/module/org_alfresco_module_rm/rm-service-context.xml index 7a903fc7b8..b463de52b8 100644 --- a/rm-community/rm-community-repo/config/alfresco/module/org_alfresco_module_rm/rm-service-context.xml +++ b/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 diff --git a/rm-community/rm-community-repo/test/java/org/alfresco/module/org_alfresco_module_rm/test/integration/hold/AddRemoveFromHoldTest.java b/rm-community/rm-community-repo/test/java/org/alfresco/module/org_alfresco_module_rm/test/integration/hold/AddRemoveFromHoldTest.java index 9d1d6d685b..581a6c922c 100644 --- a/rm-community/rm-community-repo/test/java/org/alfresco/module/org_alfresco_module_rm/test/integration/hold/AddRemoveFromHoldTest.java +++ b/rm-community/rm-community-repo/test/java/org/alfresco/module/org_alfresco_module_rm/test/integration/hold/AddRemoveFromHoldTest.java @@ -44,7 +44,65 @@ import org.springframework.extensions.webscripts.GUID; public class AddRemoveFromHoldTest extends BaseRMTestCase { private static final int RECORD_COUNT = 10; - + + @Override + protected boolean isCollaborationSiteTest() + { + return true; + } + + public void testAddActiveContentToHold() + { + doBehaviourDrivenTest(new BehaviourDrivenTest() + { + private NodeRef hold; + + public void given() + { + // Check that the document is not a record + assertFalse("The document should not be a record", recordService.isRecord(dmDocument)); + + // create a hold + hold = holdService.createHold(filePlan, GUID.generate(), GUID.generate(), GUID.generate()); + + // assert current states + assertFalse(freezeService.isFrozen(dmDocument)); + assertFalse(freezeService.hasFrozenChildren(dmFolder)); + + // additional check for child held caching + assertFalse(nodeService.hasAspect(dmFolder, ASPECT_HELD_CHILDREN)); + } + + public void when() throws Exception + { + // add the active content to hold + holdService.addToHold(hold, dmDocument); + } + + public void then() + { + // active content is held + assertTrue(freezeService.isFrozen(dmDocument)); + + // collaboration folder has frozen children + assertFalse(freezeService.isFrozen(dmFolder)); + assertTrue(freezeService.hasFrozenChildren(dmFolder)); + + // collaboration folder is not held + assertFalse(holdService.getHeld(hold).contains(dmFolder)); + assertFalse(holdService.heldBy(dmFolder, true).contains(hold)); + + // hold contains active content + assertTrue(holdService.getHeld(hold).contains(dmDocument)); + assertTrue(holdService.heldBy(dmDocument, true).contains(hold)); + + // additional check for child held caching + assertTrue(nodeService.hasAspect(dmFolder, ASPECT_HELD_CHILDREN)); + assertEquals(1, nodeService.getProperty(dmFolder, PROP_HELD_CHILDREN_COUNT)); + } + }); + } + public void testAddRecordToHold() { doBehaviourDrivenTest(new BehaviourDrivenTest() From 3d7b5df3c9078b2ca81e468d9d1a6437034dca24 Mon Sep 17 00:00:00 2001 From: cagache Date: Thu, 25 Jul 2019 17:51:17 +0300 Subject: [PATCH 04/13] Added IT and unit tests for add file to hold --- .../hold/HoldService.java | 12 +- .../hold/HoldServiceImpl.java | 2 +- .../hold/AddActiveContentToHoldTest.java | 242 ++++++++++++++++++ .../hold/AddRemoveFromHoldTest.java | 58 ----- .../test/util/BaseRMTestCase.java | 9 +- .../hold/HoldServiceImplUnitTest.java | 19 ++ 6 files changed, 276 insertions(+), 66 deletions(-) create mode 100644 rm-community/rm-community-repo/test/java/org/alfresco/module/org_alfresco_module_rm/test/integration/hold/AddActiveContentToHoldTest.java diff --git a/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/hold/HoldService.java b/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/hold/HoldService.java index db3550a13a..9940e17007 100644 --- a/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/hold/HoldService.java +++ b/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 true to retrieve the list of hold node references which will include the node reference * false to get a list of node references which will not have the given node reference * @return List of hold node references @@ -119,10 +119,10 @@ public interface HoldService void deleteHold(NodeRef hold); /** - * Adds the record to the given hold + * Adds the item to the given hold * - * @param hold The {@link NodeRef} of the hold - * @param nodeRef The {@link NodeRef} of the record / record folder which will be added to the given hold + * @param hold The {@link NodeRef} of the 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 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 holds, NodeRef nodeRef); diff --git a/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/hold/HoldServiceImpl.java b/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/hold/HoldServiceImpl.java index 7e769ec9b2..51020bf8a5 100644 --- a/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/hold/HoldServiceImpl.java +++ b/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/hold/HoldServiceImpl.java @@ -303,7 +303,7 @@ public class HoldServiceImpl extends ServiceBaseImpl if (!includedInHold) { // invert list to get list of holds that do not contain this node - NodeRef filePlan = filePlanService.getFilePlan(nodeRef); + NodeRef filePlan = isFilePlanComponent(nodeRef) ? filePlanService.getFilePlan(nodeRef) : filePlanService.getFilePlanBySiteId(FilePlanService.DEFAULT_RM_SITE_ID); List allHolds = getHolds(filePlan); result = ListUtils.subtract(allHolds, new ArrayList<>(holdsNotIncludingNodeRef)); } diff --git a/rm-community/rm-community-repo/test/java/org/alfresco/module/org_alfresco_module_rm/test/integration/hold/AddActiveContentToHoldTest.java b/rm-community/rm-community-repo/test/java/org/alfresco/module/org_alfresco_module_rm/test/integration/hold/AddActiveContentToHoldTest.java new file mode 100644 index 0000000000..75d8dc2541 --- /dev/null +++ b/rm-community/rm-community-repo/test/java/org/alfresco/module/org_alfresco_module_rm/test/integration/hold/AddActiveContentToHoldTest.java @@ -0,0 +1,242 @@ +/* + * #%L + * Alfresco Records Management Module + * %% + * Copyright (C) 2005 - 2019 Alfresco Software Limited + * %% + * This file is part of the Alfresco software. + * - + * If the software was purchased under a paid Alfresco license, the terms of + * the paid license agreement will prevail. Otherwise, the software is + * provided under the following open source license terms: + * - + * Alfresco is free software: you can redistribute it and/or modify + * it under the terms of the GNU Lesser General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * - + * Alfresco is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Lesser General Public License for more details. + * - + * You should have received a copy of the GNU Lesser General Public License + * along with Alfresco. If not, see . + * #L% + */ +package org.alfresco.module.org_alfresco_module_rm.test.integration.hold; + +import org.alfresco.error.AlfrescoRuntimeException; +import org.alfresco.module.org_alfresco_module_rm.test.util.BaseRMTestCase; +import org.alfresco.repo.security.authentication.AuthenticationUtil; +import org.alfresco.repo.security.authentication.AuthenticationUtil.RunAsWork; +import org.alfresco.repo.security.permissions.AccessDeniedException; +import org.alfresco.repo.site.SiteModel; +import org.alfresco.service.cmr.repository.NodeRef; +import org.springframework.extensions.webscripts.GUID; + +/** + * Add Active Content To Hold Integration Tests + * + * @author Claudia Agache + * @since 3.2 + */ +public class AddActiveContentToHoldTest extends BaseRMTestCase +{ + @Override + protected boolean isCollaborationSiteTest() + { + return true; + } + + @Override + protected boolean isUserTest() + { + return true; + } + + public void testAddDocumentToHold() + { + doBehaviourDrivenTest(new BehaviourDrivenTest() + { + private NodeRef hold; + + public void given() + { + // Check that the document is not a record + assertFalse("The document should not be a record", recordService.isRecord(dmDocument)); + + // create a hold + hold = holdService.createHold(filePlan, GUID.generate(), GUID.generate(), GUID.generate()); + + // assert current states + assertFalse(freezeService.isFrozen(dmDocument)); + assertFalse(freezeService.hasFrozenChildren(dmFolder)); + + // additional check for child held caching + assertFalse(nodeService.hasAspect(dmFolder, ASPECT_HELD_CHILDREN)); + } + + public void when() + { + // add the active content to hold + holdService.addToHold(hold, dmDocument); + } + + public void then() + { + // active content is held + assertTrue(freezeService.isFrozen(dmDocument)); + + // collaboration folder has frozen children + assertFalse(freezeService.isFrozen(dmFolder)); + assertTrue(freezeService.hasFrozenChildren(dmFolder)); + + // collaboration folder is not held + assertFalse(holdService.getHeld(hold).contains(dmFolder)); + assertFalse(holdService.heldBy(dmFolder, true).contains(hold)); + + // hold contains active content + assertTrue(holdService.getHeld(hold).contains(dmDocument)); + assertTrue(holdService.heldBy(dmDocument, true).contains(hold)); + + // additional check for child held caching + assertTrue(nodeService.hasAspect(dmFolder, ASPECT_HELD_CHILDREN)); + assertEquals(1, nodeService.getProperty(dmFolder, PROP_HELD_CHILDREN_COUNT)); + } + }); + } + + public void testAddDocumentToHoldAsNonRMUser() + { + doBehaviourDrivenTest(new BehaviourDrivenTest(AccessDeniedException.class) + { + private NodeRef hold; + + public void given() + { + // Check that the document is not a record + assertFalse("The document should not be a record", recordService.isRecord(dmDocument)); + + // create a hold + hold = holdService.createHold(filePlan, GUID.generate(), GUID.generate(), GUID.generate()); + + // assert current states + assertFalse(freezeService.isFrozen(dmDocument)); + } + + public void when() + { + // add the active content to hold as a non RM user + AuthenticationUtil.runAs( + (RunAsWork) () -> { + holdService.addToHold(hold, dmDocument); + return null; + }, dmCollaborator); + } + }); + } + + public void testAddDocumentToHoldNoWritePermissionOnDoc() + { + doBehaviourDrivenTest(new BehaviourDrivenTest(AlfrescoRuntimeException.class) + { + private NodeRef hold; + + public void given() + { + // Check that the document is not a record + assertFalse("The document should not be a record", recordService.isRecord(dmDocument)); + + // create a hold + hold = holdService.createHold(filePlan, GUID.generate(), GUID.generate(), GUID.generate()); + + // assert current states + assertFalse(freezeService.isFrozen(dmDocument)); + } + + public void when() + { + // add the active content to hold as a RM admin who has Add to Hold Capability and filing permission on + // hold, but no Write permissions on doc + AuthenticationUtil.runAs( + (RunAsWork) () -> { + holdService.addToHold(hold, dmDocument); + return null; + }, rmAdminName); + } + }); + } + + public void testAddDocumentToHoldNoFilingPermissionOnHold() + { + doBehaviourDrivenTest(new BehaviourDrivenTest(AlfrescoRuntimeException.class) + { + private NodeRef hold; + + public void given() + { + // Check that the document is not a record + assertFalse("The document should not be a record", recordService.isRecord(dmDocument)); + + // create a hold + hold = holdService.createHold(filePlan, GUID.generate(), GUID.generate(), GUID.generate()); + + // assert current states + assertFalse(freezeService.isFrozen(dmDocument)); + + //add recordsManagerPerson as manager in collaboration site to have write permissions on dmDocument + siteService.setMembership(collabSiteId, recordsManagerName, SiteModel.SITE_MANAGER); + } + + public void when() + { + // add the active content to hold as a RM manager who has Add to Hold Capability and write permission on + // doc, but no filing permission on hold + AuthenticationUtil.runAs( + (RunAsWork) () -> { + holdService.addToHold(hold, dmDocument); + return null; + }, recordsManagerName); + } + }); + } + + + public void testAddDocumentToHoldNoCapability() + { + doBehaviourDrivenTest(new BehaviourDrivenTest(AlfrescoRuntimeException.class) + { + private NodeRef hold; + + public void given() + { + // Check that the document is not a record + assertFalse("The document should not be a record", recordService.isRecord(dmDocument)); + + // create a hold + hold = holdService.createHold(filePlan, GUID.generate(), GUID.generate(), GUID.generate()); + + // assert current states + assertFalse(freezeService.isFrozen(dmDocument)); + + //add powerUserPerson as manager in collaboration site to have write permissions on dmDocument + siteService.setMembership(collabSiteId, powerUserName, SiteModel.SITE_MANAGER); + + //assign powerUserPerson filing permission on hold + filePlanPermissionService.setPermission(hold, powerUserName, FILING); + } + + public void when() + { + // add the active content to hold as a RM power user who has write permission on doc and filing + // permission on hold, but no Add To Hold capability + AuthenticationUtil.runAs( + (RunAsWork) () -> { + holdService.addToHold(hold, dmDocument); + return null; + }, powerUserName); + } + }); + } +} diff --git a/rm-community/rm-community-repo/test/java/org/alfresco/module/org_alfresco_module_rm/test/integration/hold/AddRemoveFromHoldTest.java b/rm-community/rm-community-repo/test/java/org/alfresco/module/org_alfresco_module_rm/test/integration/hold/AddRemoveFromHoldTest.java index 581a6c922c..0a31e20fa5 100644 --- a/rm-community/rm-community-repo/test/java/org/alfresco/module/org_alfresco_module_rm/test/integration/hold/AddRemoveFromHoldTest.java +++ b/rm-community/rm-community-repo/test/java/org/alfresco/module/org_alfresco_module_rm/test/integration/hold/AddRemoveFromHoldTest.java @@ -45,64 +45,6 @@ public class AddRemoveFromHoldTest extends BaseRMTestCase { private static final int RECORD_COUNT = 10; - @Override - protected boolean isCollaborationSiteTest() - { - return true; - } - - public void testAddActiveContentToHold() - { - doBehaviourDrivenTest(new BehaviourDrivenTest() - { - private NodeRef hold; - - public void given() - { - // Check that the document is not a record - assertFalse("The document should not be a record", recordService.isRecord(dmDocument)); - - // create a hold - hold = holdService.createHold(filePlan, GUID.generate(), GUID.generate(), GUID.generate()); - - // assert current states - assertFalse(freezeService.isFrozen(dmDocument)); - assertFalse(freezeService.hasFrozenChildren(dmFolder)); - - // additional check for child held caching - assertFalse(nodeService.hasAspect(dmFolder, ASPECT_HELD_CHILDREN)); - } - - public void when() throws Exception - { - // add the active content to hold - holdService.addToHold(hold, dmDocument); - } - - public void then() - { - // active content is held - assertTrue(freezeService.isFrozen(dmDocument)); - - // collaboration folder has frozen children - assertFalse(freezeService.isFrozen(dmFolder)); - assertTrue(freezeService.hasFrozenChildren(dmFolder)); - - // collaboration folder is not held - assertFalse(holdService.getHeld(hold).contains(dmFolder)); - assertFalse(holdService.heldBy(dmFolder, true).contains(hold)); - - // hold contains active content - assertTrue(holdService.getHeld(hold).contains(dmDocument)); - assertTrue(holdService.heldBy(dmDocument, true).contains(hold)); - - // additional check for child held caching - assertTrue(nodeService.hasAspect(dmFolder, ASPECT_HELD_CHILDREN)); - assertEquals(1, nodeService.getProperty(dmFolder, PROP_HELD_CHILDREN_COUNT)); - } - }); - } - public void testAddRecordToHold() { doBehaviourDrivenTest(new BehaviourDrivenTest() diff --git a/rm-community/rm-community-repo/test/java/org/alfresco/module/org_alfresco_module_rm/test/util/BaseRMTestCase.java b/rm-community/rm-community-repo/test/java/org/alfresco/module/org_alfresco_module_rm/test/util/BaseRMTestCase.java index 57f2151c23..204ca4c8e8 100644 --- a/rm-community/rm-community-repo/test/java/org/alfresco/module/org_alfresco_module_rm/test/util/BaseRMTestCase.java +++ b/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()) diff --git a/rm-community/rm-community-repo/unit-test/java/org/alfresco/module/org_alfresco_module_rm/hold/HoldServiceImplUnitTest.java b/rm-community/rm-community-repo/unit-test/java/org/alfresco/module/org_alfresco_module_rm/hold/HoldServiceImplUnitTest.java index 72faf25a86..19fa820027 100644 --- a/rm-community/rm-community-repo/unit-test/java/org/alfresco/module/org_alfresco_module_rm/hold/HoldServiceImplUnitTest.java +++ b/rm-community/rm-community-repo/unit-test/java/org/alfresco/module/org_alfresco_module_rm/hold/HoldServiceImplUnitTest.java @@ -54,10 +54,12 @@ import java.util.Map; import org.alfresco.error.AlfrescoRuntimeException; import org.alfresco.model.ContentModel; import org.alfresco.module.org_alfresco_module_rm.capability.RMPermissionModel; +import org.alfresco.module.org_alfresco_module_rm.fileplan.FilePlanService; import org.alfresco.module.org_alfresco_module_rm.test.util.BaseUnitTest; import org.alfresco.service.cmr.repository.ChildAssociationRef; import org.alfresco.service.cmr.repository.NodeRef; import org.alfresco.service.cmr.security.AccessStatus; +import org.alfresco.service.cmr.security.PermissionService; import org.alfresco.service.namespace.NamespaceService; import org.alfresco.service.namespace.QName; import org.alfresco.service.namespace.RegexQNamePattern; @@ -107,6 +109,7 @@ public class HoldServiceImplUnitTest extends BaseUnitTest // setup interactions doReturn(holdContainer).when(mockedFilePlanService).getHoldContainer(filePlan); + doReturn(filePlan).when(mockedFilePlanService).getFilePlanBySiteId(FilePlanService.DEFAULT_RM_SITE_ID); } @Test @@ -125,6 +128,9 @@ public class HoldServiceImplUnitTest extends BaseUnitTest holds.add(new ChildAssociationRef(ASSOC_FROZEN_RECORDS, hold2, ASSOC_FROZEN_RECORDS, recordFolder, true, 2)); doReturn(holds).when(mockedNodeService).getParentAssocs(recordFolder, ASSOC_FROZEN_RECORDS, ASSOC_FROZEN_RECORDS); + //setup active content in multiple holds + doReturn(holds).when(mockedNodeService).getParentAssocs(activeContent, ASSOC_FROZEN_RECORDS, ASSOC_FROZEN_RECORDS); + // check that both holds are found for record folder List heldByHolds = holdService.heldBy(recordFolder, true); assertNotNull(heldByHolds); @@ -136,6 +142,12 @@ public class HoldServiceImplUnitTest extends BaseUnitTest assertNotNull(heldByHolds); assertEquals(2, heldByHolds.size()); assertTrue(holdService.heldBy(record, false).isEmpty()); + + // check that both holds are found for active content + heldByHolds = holdService.heldBy(activeContent, true); + assertNotNull(heldByHolds); + assertEquals(2, heldByHolds.size()); + assertTrue(holdService.heldBy(activeContent, false).isEmpty()); } @Test (expected=AlfrescoRuntimeException.class) @@ -371,6 +383,13 @@ public class HoldServiceImplUnitTest extends BaseUnitTest holdService.addToHold(hold, activeContent); } + @Test (expected = AlfrescoRuntimeException.class) + public void addActiveContentToHoldNoPermissionsOnContent() + { + when(mockedPermissionService.hasPermission(activeContent, PermissionService.WRITE)).thenReturn(AccessStatus.DENIED); + holdService.addToHold(hold, activeContent); + } + @SuppressWarnings("unchecked") @Test public void addToHolds() From 78e49e77f11a89a7427da0b7a804a6030069fedc Mon Sep 17 00:00:00 2001 From: cagache Date: Mon, 29 Jul 2019 15:35:35 +0300 Subject: [PATCH 05/13] Add ASPECT_HELD_CHILDREN on active content parent --- .../model/rma/aspect/FrozenAspect.java | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/model/rma/aspect/FrozenAspect.java b/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/model/rma/aspect/FrozenAspect.java index 958b808000..8bf26d1375 100644 --- a/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/model/rma/aspect/FrozenAspect.java +++ b/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/model/rma/aspect/FrozenAspect.java @@ -28,8 +28,13 @@ 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; @@ -169,6 +174,16 @@ public class FrozenAspect extends BaseBehaviourBean currentCount = currentCount + 1; nodeService.setProperty(parentRef, PROP_HELD_CHILDREN_COUNT, currentCount); } + else + { + if(instanceOf(parentRef, TYPE_FOLDER) && !nodeService.hasAspect(parentRef, ASPECT_SITE_CONTAINER)) + { + // add aspect and set count to 1 + Map props = new HashMap<>(1); + props.put(PROP_HELD_CHILDREN_COUNT, 1); + getInternalNodeService().addAspect(parentRef, ASPECT_HELD_CHILDREN, props); + } + } } return null; } From f62235bd01c3847e565dfb615651aba85cf39b6b Mon Sep 17 00:00:00 2001 From: cagache Date: Thu, 1 Aug 2019 16:02:55 +0300 Subject: [PATCH 06/13] code review comments --- .../hold/HoldServiceImpl.java | 2 +- .../model/rma/aspect/FrozenAspect.java | 2 +- .../hold/AddActiveContentToHoldTest.java | 96 +++++++++++++------ 3 files changed, 71 insertions(+), 29 deletions(-) diff --git a/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/hold/HoldServiceImpl.java b/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/hold/HoldServiceImpl.java index 51020bf8a5..e3398a45ec 100644 --- a/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/hold/HoldServiceImpl.java +++ b/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/hold/HoldServiceImpl.java @@ -303,7 +303,7 @@ public class HoldServiceImpl extends ServiceBaseImpl if (!includedInHold) { // invert list to get list of holds that do not contain this node - NodeRef filePlan = isFilePlanComponent(nodeRef) ? filePlanService.getFilePlan(nodeRef) : filePlanService.getFilePlanBySiteId(FilePlanService.DEFAULT_RM_SITE_ID); + NodeRef filePlan = filePlanService.getFilePlanBySiteId(FilePlanService.DEFAULT_RM_SITE_ID); List allHolds = getHolds(filePlan); result = ListUtils.subtract(allHolds, new ArrayList<>(holdsNotIncludingNodeRef)); } diff --git a/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/model/rma/aspect/FrozenAspect.java b/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/model/rma/aspect/FrozenAspect.java index 8bf26d1375..20f9256ec3 100644 --- a/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/model/rma/aspect/FrozenAspect.java +++ b/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/model/rma/aspect/FrozenAspect.java @@ -164,7 +164,7 @@ public class FrozenAspect extends BaseBehaviourBean { if (nodeService.exists(nodeRef) && (isRecord(nodeRef) || instanceOf(nodeRef, TYPE_CONTENT))) { - // get the owning nodeRef folder + // get the owning folder NodeRef parentRef = nodeService.getPrimaryParent(nodeRef).getParentRef(); // check that the aspect has been added if (nodeService.hasAspect(parentRef, ASPECT_HELD_CHILDREN)) diff --git a/rm-community/rm-community-repo/test/java/org/alfresco/module/org_alfresco_module_rm/test/integration/hold/AddActiveContentToHoldTest.java b/rm-community/rm-community-repo/test/java/org/alfresco/module/org_alfresco_module_rm/test/integration/hold/AddActiveContentToHoldTest.java index 75d8dc2541..ce47d7ad3f 100644 --- a/rm-community/rm-community-repo/test/java/org/alfresco/module/org_alfresco_module_rm/test/integration/hold/AddActiveContentToHoldTest.java +++ b/rm-community/rm-community-repo/test/java/org/alfresco/module/org_alfresco_module_rm/test/integration/hold/AddActiveContentToHoldTest.java @@ -26,7 +26,11 @@ */ 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.error.AlfrescoRuntimeException; +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; @@ -139,7 +143,7 @@ public class AddActiveContentToHoldTest extends BaseRMTestCase public void testAddDocumentToHoldNoWritePermissionOnDoc() { - doBehaviourDrivenTest(new BehaviourDrivenTest(AlfrescoRuntimeException.class) + doBehaviourDrivenTest(new BehaviourDrivenTest() { private NodeRef hold; @@ -153,6 +157,9 @@ public class AddActiveContentToHoldTest extends BaseRMTestCase // assert current states assertFalse(freezeService.isFrozen(dmDocument)); + + //add rm Admin as consumer in collaboration site to have read permissions on dmDocument + siteService.setMembership(collabSiteId, rmAdminName, SiteModel.SITE_CONSUMER); } public void when() @@ -161,7 +168,16 @@ public class AddActiveContentToHoldTest extends BaseRMTestCase // hold, but no Write permissions on doc AuthenticationUtil.runAs( (RunAsWork) () -> { - holdService.addToHold(hold, dmDocument); + try + { + holdService.addToHold(hold, dmDocument); + fail("Expected AlfrescoRuntimeException to be thrown."); + } + catch (AlfrescoRuntimeException e) + { + System.out.println(e); + assertTrue(e.getMessage().endsWith("Write permission on '" + NAME_DM_DOCUMENT + "' is needed.")); + } return null; }, rmAdminName); } @@ -170,61 +186,79 @@ public class AddActiveContentToHoldTest extends BaseRMTestCase public void testAddDocumentToHoldNoFilingPermissionOnHold() { - doBehaviourDrivenTest(new BehaviourDrivenTest(AlfrescoRuntimeException.class) + doBehaviourDrivenTest(new BehaviourDrivenTest(recordsManagerName, false) { private NodeRef hold; public void given() { - // Check that the document is not a record - assertFalse("The document should not be a record", recordService.isRecord(dmDocument)); + AuthenticationUtil.runAs( + (RunAsWork) () -> { + // Check that the document is not a record + assertFalse("The document should not be a record", recordService.isRecord(dmDocument)); - // create a hold - hold = holdService.createHold(filePlan, GUID.generate(), GUID.generate(), GUID.generate()); + // create a hold + hold = holdService.createHold(filePlan, GUID.generate(), GUID.generate(), GUID.generate()); - // assert current states - assertFalse(freezeService.isFrozen(dmDocument)); + //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, SiteModel.SITE_MANAGER); + // assert current states + assertFalse(freezeService.isFrozen(dmDocument)); + + //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 AuthenticationUtil.runAs( (RunAsWork) () -> { - holdService.addToHold(hold, dmDocument); + // 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 + try + { + holdService.addToHold(hold, dmDocument); + fail("Expected AccessDeniedException to be thrown."); + } + catch (AccessDeniedException e) + { + //expected + } return null; }, recordsManagerName); } }); } - public void testAddDocumentToHoldNoCapability() { - doBehaviourDrivenTest(new BehaviourDrivenTest(AlfrescoRuntimeException.class) + doBehaviourDrivenTest(new BehaviourDrivenTest(powerUserName, false) { private NodeRef hold; public void given() { - // Check that the document is not a record - assertFalse("The document should not be a record", recordService.isRecord(dmDocument)); + AuthenticationUtil.runAs( + (RunAsWork) () -> { + // Check that the document is not a record + assertFalse("The document should not be a record", recordService.isRecord(dmDocument)); - // create a hold - hold = holdService.createHold(filePlan, GUID.generate(), GUID.generate(), GUID.generate()); + // create a hold + hold = holdService.createHold(filePlan, GUID.generate(), GUID.generate(), GUID.generate()); - // assert current states - assertFalse(freezeService.isFrozen(dmDocument)); + // assert current states + assertFalse(freezeService.isFrozen(dmDocument)); - //add powerUserPerson as manager in collaboration site to have write permissions on dmDocument - siteService.setMembership(collabSiteId, powerUserName, SiteModel.SITE_MANAGER); + //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); + //assign powerUserPerson filing permission on hold + filePlanPermissionService.setPermission(hold, powerUserName, FILING); + return null; + }, getAdminUserName()); } public void when() @@ -233,7 +267,15 @@ public class AddActiveContentToHoldTest extends BaseRMTestCase // permission on hold, but no Add To Hold capability AuthenticationUtil.runAs( (RunAsWork) () -> { - holdService.addToHold(hold, dmDocument); + try + { + holdService.addToHold(hold, dmDocument); + fail("Expected AccessDeniedException to be thrown."); + } + catch (AccessDeniedException e) + { + //expected + } return null; }, powerUserName); } From e61cb93cfda3f97ae004c7940ec3a3c29801186e Mon Sep 17 00:00:00 2001 From: cagache Date: Fri, 2 Aug 2019 09:42:09 +0300 Subject: [PATCH 07/13] get default file plan only if node is not a file plan component --- .../hold/HoldServiceImpl.java | 94 +++++++++---------- 1 file changed, 43 insertions(+), 51 deletions(-) diff --git a/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/hold/HoldServiceImpl.java b/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/hold/HoldServiceImpl.java index e3398a45ec..657e8d8e7c 100644 --- a/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/hold/HoldServiceImpl.java +++ b/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/hold/HoldServiceImpl.java @@ -303,7 +303,7 @@ public class HoldServiceImpl extends ServiceBaseImpl if (!includedInHold) { // invert list to get list of holds that do not contain this node - NodeRef filePlan = filePlanService.getFilePlanBySiteId(FilePlanService.DEFAULT_RM_SITE_ID); + NodeRef filePlan = isFilePlanComponent(nodeRef) ? filePlanService.getFilePlan(nodeRef) : filePlanService.getFilePlanBySiteId(FilePlanService.DEFAULT_RM_SITE_ID); List allHolds = getHolds(filePlan); result = ListUtils.subtract(allHolds, new ArrayList<>(holdsNotIncludingNodeRef)); } @@ -575,63 +575,55 @@ public class HoldServiceImpl extends ServiceBaseImpl 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() - { - @Override - public Void doWork() + authenticationUtil.runAsSystem((RunAsWork) () -> { + // gather freeze properties + Map props = new HashMap<>(2); + 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 - Map props = new HashMap<>(2); - 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 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; + List records = recordService.getRecords(nodeRef); + records.forEach(record -> addFrozenAspect(record, props)); } + + return 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 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) */ From 970330b2c5c5c241e29c45c107809aa6f2fc6bfa Mon Sep 17 00:00:00 2001 From: cagache Date: Fri, 2 Aug 2019 15:38:54 +0300 Subject: [PATCH 08/13] Removed printed exception --- .../test/integration/hold/AddActiveContentToHoldTest.java | 1 - 1 file changed, 1 deletion(-) diff --git a/rm-community/rm-community-repo/test/java/org/alfresco/module/org_alfresco_module_rm/test/integration/hold/AddActiveContentToHoldTest.java b/rm-community/rm-community-repo/test/java/org/alfresco/module/org_alfresco_module_rm/test/integration/hold/AddActiveContentToHoldTest.java index ce47d7ad3f..77dd7322c3 100644 --- a/rm-community/rm-community-repo/test/java/org/alfresco/module/org_alfresco_module_rm/test/integration/hold/AddActiveContentToHoldTest.java +++ b/rm-community/rm-community-repo/test/java/org/alfresco/module/org_alfresco_module_rm/test/integration/hold/AddActiveContentToHoldTest.java @@ -175,7 +175,6 @@ public class AddActiveContentToHoldTest extends BaseRMTestCase } catch (AlfrescoRuntimeException e) { - System.out.println(e); assertTrue(e.getMessage().endsWith("Write permission on '" + NAME_DM_DOCUMENT + "' is needed.")); } return null; From 0a1962d0b72368b50cd8390153cb9795a9ff107f Mon Sep 17 00:00:00 2001 From: cagache Date: Fri, 2 Aug 2019 17:31:02 +0300 Subject: [PATCH 09/13] code review comments --- .../hold/HoldServiceImpl.java | 2 + .../hold/AddActiveContentToHoldTest.java | 68 +++++++++++-------- .../test/util/BaseRMTestCase.java | 7 ++ .../hold/HoldServiceImplUnitTest.java | 19 +++++- 4 files changed, 64 insertions(+), 32 deletions(-) diff --git a/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/hold/HoldServiceImpl.java b/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/hold/HoldServiceImpl.java index 657e8d8e7c..e1dd5c0e07 100644 --- a/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/hold/HoldServiceImpl.java +++ b/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 allHolds = getHolds(filePlan); result = ListUtils.subtract(allHolds, new ArrayList<>(holdsNotIncludingNodeRef)); diff --git a/rm-community/rm-community-repo/test/java/org/alfresco/module/org_alfresco_module_rm/test/integration/hold/AddActiveContentToHoldTest.java b/rm-community/rm-community-repo/test/java/org/alfresco/module/org_alfresco_module_rm/test/integration/hold/AddActiveContentToHoldTest.java index 77dd7322c3..8755328eea 100644 --- a/rm-community/rm-community-repo/test/java/org/alfresco/module/org_alfresco_module_rm/test/integration/hold/AddActiveContentToHoldTest.java +++ b/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( - (RunAsWork) () -> { - // 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 - try - { - holdService.addToHold(hold, dmDocument); - fail("Expected AccessDeniedException to be thrown."); - } - catch (AccessDeniedException e) - { - //expected - } - return null; - }, recordsManagerName); + // 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(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( - (RunAsWork) () -> { - try - { - holdService.addToHold(hold, dmDocument); - fail("Expected AccessDeniedException to be thrown."); - } - catch (AccessDeniedException e) - { - //expected - } - return null; - }, powerUserName); + holdService.addToHold(hold, dmDocument); } }); } diff --git a/rm-community/rm-community-repo/test/java/org/alfresco/module/org_alfresco_module_rm/test/util/BaseRMTestCase.java b/rm-community/rm-community-repo/test/java/org/alfresco/module/org_alfresco_module_rm/test/util/BaseRMTestCase.java index 204ca4c8e8..21cb4ce5d8 100644 --- a/rm-community/rm-community-repo/test/java/org/alfresco/module/org_alfresco_module_rm/test/util/BaseRMTestCase.java +++ b/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 */ } diff --git a/rm-community/rm-community-repo/unit-test/java/org/alfresco/module/org_alfresco_module_rm/hold/HoldServiceImplUnitTest.java b/rm-community/rm-community-repo/unit-test/java/org/alfresco/module/org_alfresco_module_rm/hold/HoldServiceImplUnitTest.java index 19fa820027..8bcd8980fc 100644 --- a/rm-community/rm-community-repo/unit-test/java/org/alfresco/module/org_alfresco_module_rm/hold/HoldServiceImplUnitTest.java +++ b/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); } From 2d1753830a53fcb204f3b75497d09625f2a6eba7 Mon Sep 17 00:00:00 2001 From: cagache Date: Mon, 5 Aug 2019 16:40:26 +0300 Subject: [PATCH 10/13] Added check for archived nodes --- .../hold/HoldServiceImpl.java | 73 ++++++++++++------- .../hold/HoldServiceImplUnitTest.java | 21 +++++- 2 files changed, 63 insertions(+), 31 deletions(-) diff --git a/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/hold/HoldServiceImpl.java b/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/hold/HoldServiceImpl.java index e1dd5c0e07..8a5990ea76 100644 --- a/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/hold/HoldServiceImpl.java +++ b/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/hold/HoldServiceImpl.java @@ -285,33 +285,34 @@ public class HoldServiceImpl extends ServiceBaseImpl { ParameterCheck.mandatory("nodeRef", nodeRef); - List result = null; + List result = new ArrayList<>(); // get all the immediate parent holds - Set holdsNotIncludingNodeRef = getParentHolds(nodeRef); + Set 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 recordFolders = recordFolderService.getRecordFolders(nodeRef); for (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 - //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 allHolds = getHolds(filePlan); - result = ListUtils.subtract(allHolds, new ArrayList<>(holdsNotIncludingNodeRef)); + if (!holdsIncludingNodeRef.isEmpty()) + { + // invert list to get list of holds that do not contain this node + NodeRef filePlan = filePlanService.getFilePlan(holdsIncludingNodeRef.iterator().next()); + List allHolds = getHolds(filePlan); + result = ListUtils.subtract(allHolds, new ArrayList<>(holdsIncludingNodeRef)); + } } else { - result = new ArrayList<>(holdsNotIncludingNodeRef); + result = new ArrayList<>(holdsIncludingNodeRef); } return result; @@ -541,23 +542,7 @@ public class HoldServiceImpl extends ServiceBaseImpl ParameterCheck.mandatoryCollection("holds", holds); ParameterCheck.mandatory("nodeRef", nodeRef); - String nodeName = (String) nodeService.getProperty(nodeRef, ContentModel.PROP_NAME); - if (!isRecord(nodeRef) && !isRecordFolder(nodeRef) && !instanceOf(nodeRef, ContentModel.TYPE_CONTENT)) - { - throw new AlfrescoRuntimeException("'" + nodeName + "' is neither a record nor a record folder nor a document. " + - "Only records, record folders or active content can be added to a hold."); - } - - if ((isRecord(nodeRef) || isRecordFolder(nodeRef)) && - permissionService.hasPermission(nodeRef, RMPermissionModel.FILING) == AccessStatus.DENIED) - { - throw new AlfrescoRuntimeException("Filing permission on '" + nodeName + "' is needed."); - } - else if (instanceOf(nodeRef, ContentModel.TYPE_CONTENT) && - permissionService.hasPermission(nodeRef, PermissionService.WRITE) == AccessStatus.DENIED) - { - throw new AlfrescoRuntimeException("Write permission on '" + nodeName + "' is needed."); - } + checkNodeCanBeAddedToHold(nodeRef); for (final NodeRef hold : holds) { @@ -569,6 +554,7 @@ public class HoldServiceImpl extends ServiceBaseImpl if (permissionService.hasPermission(hold, RMPermissionModel.FILING) == AccessStatus.DENIED) { + String nodeName = (String) nodeService.getProperty(nodeRef, ContentModel.PROP_NAME); 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."); } @@ -604,6 +590,37 @@ public class HoldServiceImpl extends ServiceBaseImpl } } + /** + * 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) + { + String nodeName = (String) nodeService.getProperty(nodeRef, ContentModel.PROP_NAME); + if (!isRecord(nodeRef) && !isRecordFolder(nodeRef) && !instanceOf(nodeRef, ContentModel.TYPE_CONTENT)) + { + throw new AlfrescoRuntimeException("'" + nodeName + "' is neither a record nor a record folder nor a document. " + + "Only records, record folders or active content can be added to a hold."); + } + + if ((isRecord(nodeRef) || isRecordFolder(nodeRef)) && + permissionService.hasPermission(nodeRef, RMPermissionModel.FILING) == AccessStatus.DENIED) + { + throw new AlfrescoRuntimeException("Filing permission on '" + nodeName + "' is needed."); + } + else if (instanceOf(nodeRef, ContentModel.TYPE_CONTENT) && + permissionService.hasPermission(nodeRef, PermissionService.WRITE) == AccessStatus.DENIED) + { + throw new AlfrescoRuntimeException("Write permission on '" + nodeName + "' is needed."); + } + + if (nodeService.hasAspect(nodeRef, ASPECT_ARCHIVED)) + { + throw new AlfrescoRuntimeException("Archived nodes can't be added to hold."); + } + } + /** * Add Frozen aspect only if node isn't already frozen * diff --git a/rm-community/rm-community-repo/unit-test/java/org/alfresco/module/org_alfresco_module_rm/hold/HoldServiceImplUnitTest.java b/rm-community/rm-community-repo/unit-test/java/org/alfresco/module/org_alfresco_module_rm/hold/HoldServiceImplUnitTest.java index 8bcd8980fc..f5ba65377f 100644 --- a/rm-community/rm-community-repo/unit-test/java/org/alfresco/module/org_alfresco_module_rm/hold/HoldServiceImplUnitTest.java +++ b/rm-community/rm-community-repo/unit-test/java/org/alfresco/module/org_alfresco_module_rm/hold/HoldServiceImplUnitTest.java @@ -54,7 +54,7 @@ import java.util.Map; import org.alfresco.error.AlfrescoRuntimeException; import org.alfresco.model.ContentModel; import org.alfresco.module.org_alfresco_module_rm.capability.RMPermissionModel; -import org.alfresco.module.org_alfresco_module_rm.fileplan.FilePlanService; +import org.alfresco.module.org_alfresco_module_rm.model.RecordsManagementModel; import org.alfresco.module.org_alfresco_module_rm.test.util.BaseUnitTest; import org.alfresco.service.cmr.repository.ChildAssociationRef; import org.alfresco.service.cmr.repository.NodeRef; @@ -115,7 +115,6 @@ public class HoldServiceImplUnitTest extends BaseUnitTest // setup interactions doReturn(holdContainer).when(mockedFilePlanService).getHoldContainer(filePlan); - doReturn(filePlan).when(mockedFilePlanService).getFilePlanBySiteId(FilePlanService.DEFAULT_RM_SITE_ID); } @Test @@ -129,14 +128,19 @@ public class HoldServiceImplUnitTest extends BaseUnitTest public void heldByMultipleResults() { // setup record folder in multiple holds - List holds = new ArrayList<>(2); + List 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(filePlan).when(mockedFilePlanService).getFilePlan(hold); + doReturn(filePlan).when(mockedFilePlanService).getFilePlan(hold2); + // check that both holds are found for record folder List heldByHolds = holdService.heldBy(recordFolder, true); assertNotNull(heldByHolds); @@ -405,6 +409,17 @@ public class HoldServiceImplUnitTest extends BaseUnitTest holdService.addToHold(hold, activeContent); } + @Test + public void addArchivedContentToHold() + { + expectedEx.expect(AlfrescoRuntimeException.class); + expectedEx.expectMessage("Archived nodes can't be added to hold."); + + when(mockedNodeService.getProperty(activeContent, ContentModel.PROP_NAME)).thenReturn(ACTIVE_CONTENT_NAME); + when(mockedNodeService.hasAspect(activeContent, RecordsManagementModel.ASPECT_ARCHIVED)).thenReturn(true); + holdService.addToHold(hold, activeContent); + } + @SuppressWarnings("unchecked") @Test public void addToHolds() From 71b50078cc3b77d47ee3ff4344ef914548f794ba Mon Sep 17 00:00:00 2001 From: cagache Date: Tue, 6 Aug 2019 13:45:53 +0300 Subject: [PATCH 11/13] code review comments --- .../messages/hold-service.properties | 4 ++ .../org_alfresco_module_rm/module-context.xml | 1 + .../hold/HoldServiceImpl.java | 35 ++++++++------ .../hold/AddActiveContentToHoldTest.java | 47 +------------------ .../hold/HoldServiceImplUnitTest.java | 34 ++++---------- 5 files changed, 35 insertions(+), 86 deletions(-) create mode 100644 rm-community/rm-community-repo/config/alfresco/module/org_alfresco_module_rm/messages/hold-service.properties diff --git a/rm-community/rm-community-repo/config/alfresco/module/org_alfresco_module_rm/messages/hold-service.properties b/rm-community/rm-community-repo/config/alfresco/module/org_alfresco_module_rm/messages/hold-service.properties new file mode 100644 index 0000000000..ae21a7ddb8 --- /dev/null +++ b/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. \ No newline at end of file diff --git a/rm-community/rm-community-repo/config/alfresco/module/org_alfresco_module_rm/module-context.xml b/rm-community/rm-community-repo/config/alfresco/module/org_alfresco_module_rm/module-context.xml index 5618e90a80..04b7a32d05 100644 --- a/rm-community/rm-community-repo/config/alfresco/module/org_alfresco_module_rm/module-context.xml +++ b/rm-community/rm-community-repo/config/alfresco/module/org_alfresco_module_rm/module-context.xml @@ -70,6 +70,7 @@ alfresco.module.org_alfresco_module_rm.messages.dataset-service alfresco.module.org_alfresco_module_rm.messages.rm-system alfresco.module.org_alfresco_module_rm.messages.template + alfresco.module.org_alfresco_module_rm.messages.hold-service diff --git a/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/hold/HoldServiceImpl.java b/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/hold/HoldServiceImpl.java index 8a5990ea76..167645696f 100644 --- a/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/hold/HoldServiceImpl.java +++ b/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 @@ -302,12 +305,17 @@ public class HoldServiceImpl extends ServiceBaseImpl if (!includedInHold) { - if (!holdsIncludingNodeRef.isEmpty()) + Set filePlans = filePlanService.getFilePlans(); + if (!CollectionUtils.isEmpty(filePlans)) { - // invert list to get list of holds that do not contain this node - NodeRef filePlan = filePlanService.getFilePlan(holdsIncludingNodeRef.iterator().next()); - List allHolds = getHolds(filePlan); - result = ListUtils.subtract(allHolds, new ArrayList<>(holdsIncludingNodeRef)); + List holdsNotIncludingNodeRef = new ArrayList<>(); + filePlans.forEach(filePlan -> + { + // invert list to get list of holds that do not contain this node + List allHolds = getHolds(filePlan); + holdsNotIncludingNodeRef.addAll(ListUtils.subtract(allHolds, new ArrayList<>(holdsIncludingNodeRef))); + }); + result = holdsNotIncludingNodeRef; } } else @@ -549,14 +557,12 @@ public class HoldServiceImpl extends ServiceBaseImpl if (!isHold(hold)) { 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); - 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."); + throw new AccessDeniedException(I18NUtil.getMessage("permissions.err_access_denied")); } // check that the node isn't already in the hold @@ -597,27 +603,26 @@ public class HoldServiceImpl extends ServiceBaseImpl */ private void checkNodeCanBeAddedToHold(NodeRef nodeRef) { - String nodeName = (String) nodeService.getProperty(nodeRef, ContentModel.PROP_NAME); if (!isRecord(nodeRef) && !isRecordFolder(nodeRef) && !instanceOf(nodeRef, ContentModel.TYPE_CONTENT)) { - throw new AlfrescoRuntimeException("'" + nodeName + "' is neither a record nor a record folder nor a document. " + - "Only records, record folders or active content can be added to a hold."); + 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) { - throw new AlfrescoRuntimeException("Filing permission on '" + nodeName + "' is needed."); + throw new AccessDeniedException(I18NUtil.getMessage("permissions.err_access_denied")); } else if (instanceOf(nodeRef, ContentModel.TYPE_CONTENT) && permissionService.hasPermission(nodeRef, PermissionService.WRITE) == AccessStatus.DENIED) { - throw new AlfrescoRuntimeException("Write permission on '" + nodeName + "' is needed."); + throw new AccessDeniedException(I18NUtil.getMessage("permissions.err_access_denied")); } if (nodeService.hasAspect(nodeRef, ASPECT_ARCHIVED)) { - throw new AlfrescoRuntimeException("Archived nodes can't be added to hold."); + throw new IntegrityException(I18NUtil.getMessage("rm.hold.add-to-hold-archived-node"), null); } } diff --git a/rm-community/rm-community-repo/test/java/org/alfresco/module/org_alfresco_module_rm/test/integration/hold/AddActiveContentToHoldTest.java b/rm-community/rm-community-repo/test/java/org/alfresco/module/org_alfresco_module_rm/test/integration/hold/AddActiveContentToHoldTest.java index 8755328eea..191b170d59 100644 --- a/rm-community/rm-community-repo/test/java/org/alfresco/module/org_alfresco_module_rm/test/integration/hold/AddActiveContentToHoldTest.java +++ b/rm-community/rm-community-repo/test/java/org/alfresco/module/org_alfresco_module_rm/test/integration/hold/AddActiveContentToHoldTest.java @@ -29,7 +29,6 @@ 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.error.AlfrescoRuntimeException; 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; @@ -75,18 +74,8 @@ public class AddActiveContentToHoldTest extends BaseRMTestCase public void given() { - // Check that the document is not a record - assertFalse("The document should not be a record", recordService.isRecord(dmDocument)); - // create a hold hold = holdService.createHold(filePlan, GUID.generate(), GUID.generate(), GUID.generate()); - - // assert current states - assertFalse(freezeService.isFrozen(dmDocument)); - assertFalse(freezeService.hasFrozenChildren(dmFolder)); - - // additional check for child held caching - assertFalse(nodeService.hasAspect(dmFolder, ASPECT_HELD_CHILDREN)); } public void when() @@ -133,14 +122,8 @@ public class AddActiveContentToHoldTest extends BaseRMTestCase public void given() { - // Check that the document is not a record - assertFalse("The document should not be a record", recordService.isRecord(dmDocument)); - // create a hold hold = holdService.createHold(filePlan, GUID.generate(), GUID.generate(), GUID.generate()); - - // assert current states - assertFalse(freezeService.isFrozen(dmDocument)); } public void when() @@ -163,21 +146,15 @@ public class AddActiveContentToHoldTest extends BaseRMTestCase */ public void testAddDocumentToHoldNoWritePermissionOnDoc() { - doBehaviourDrivenTest(new BehaviourDrivenTest() + doBehaviourDrivenTest(new BehaviourDrivenTest(AccessDeniedException.class) { private NodeRef hold; public void given() { - // Check that the document is not a record - assertFalse("The document should not be a record", recordService.isRecord(dmDocument)); - // create a hold hold = holdService.createHold(filePlan, GUID.generate(), GUID.generate(), GUID.generate()); - // assert current states - assertFalse(freezeService.isFrozen(dmDocument)); - //add rm Admin as consumer in collaboration site to have read permissions on dmDocument siteService.setMembership(collabSiteId, rmAdminName, SiteModel.SITE_CONSUMER); } @@ -188,15 +165,7 @@ public class AddActiveContentToHoldTest extends BaseRMTestCase // hold, but no Write permissions on doc AuthenticationUtil.runAs( (RunAsWork) () -> { - try - { - holdService.addToHold(hold, dmDocument); - fail("Expected AlfrescoRuntimeException to be thrown."); - } - catch (AlfrescoRuntimeException e) - { - assertTrue(e.getMessage().endsWith("Write permission on '" + NAME_DM_DOCUMENT + "' is needed.")); - } + holdService.addToHold(hold, dmDocument); return null; }, rmAdminName); } @@ -219,18 +188,12 @@ public class AddActiveContentToHoldTest extends BaseRMTestCase { AuthenticationUtil.runAs( (RunAsWork) () -> { - // Check that the document is not a record - assertFalse("The document should not be a record", recordService.isRecord(dmDocument)); - // create a hold hold = holdService.createHold(filePlan, GUID.generate(), GUID.generate(), GUID.generate()); //add Read permission on hold filePlanPermissionService.setPermission(hold, recordsManagerName, RMPermissionModel.READ_RECORDS); - // assert current states - assertFalse(freezeService.isFrozen(dmDocument)); - //add recordsManagerPerson as manager in collaboration site to have write permissions on dmDocument siteService.setMembership(collabSiteId, recordsManagerName, SITE_MANAGER); return null; @@ -262,15 +225,9 @@ public class AddActiveContentToHoldTest extends BaseRMTestCase { AuthenticationUtil.runAs( (RunAsWork) () -> { - // Check that the document is not a record - assertFalse("The document should not be a record", recordService.isRecord(dmDocument)); - // create a hold hold = holdService.createHold(filePlan, GUID.generate(), GUID.generate(), GUID.generate()); - // assert current states - assertFalse(freezeService.isFrozen(dmDocument)); - //add powerUserPerson as manager in collaboration site to have write permissions on dmDocument siteService.setMembership(collabSiteId, powerUserName, SiteModel.SITE_MANAGER); diff --git a/rm-community/rm-community-repo/unit-test/java/org/alfresco/module/org_alfresco_module_rm/hold/HoldServiceImplUnitTest.java b/rm-community/rm-community-repo/unit-test/java/org/alfresco/module/org_alfresco_module_rm/hold/HoldServiceImplUnitTest.java index f5ba65377f..1bdbe63d9e 100644 --- a/rm-community/rm-community-repo/unit-test/java/org/alfresco/module/org_alfresco_module_rm/hold/HoldServiceImplUnitTest.java +++ b/rm-community/rm-community-repo/unit-test/java/org/alfresco/module/org_alfresco_module_rm/hold/HoldServiceImplUnitTest.java @@ -56,6 +56,8 @@ 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; @@ -64,9 +66,7 @@ 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; @@ -85,7 +85,6 @@ 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; @@ -94,9 +93,6 @@ public class HoldServiceImplUnitTest extends BaseUnitTest @Spy @InjectMocks HoldServiceImpl holdService; - @Rule - public ExpectedException expectedEx = ExpectedException.none(); - @Before @Override public void before() throws Exception @@ -138,8 +134,7 @@ public class HoldServiceImplUnitTest extends BaseUnitTest //setup active content in multiple holds doReturn(holds).when(mockedNodeService).getParentAssocs(activeContent, ASSOC_FROZEN_RECORDS, ASSOC_FROZEN_RECORDS); - doReturn(filePlan).when(mockedFilePlanService).getFilePlan(hold); - doReturn(filePlan).when(mockedFilePlanService).getFilePlan(hold2); + doReturn(Collections.singleton(filePlan)).when(mockedFilePlanService).getFilePlans(); // check that both holds are found for record folder List heldByHolds = holdService.heldBy(recordFolder, true); @@ -309,13 +304,13 @@ 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 addToHoldNotARecordFolderOrRecordOrActiveContent() { NodeRef anotherThing = generateNodeRef(TYPE_RECORD_CATEGORY); @@ -386,36 +381,23 @@ public class HoldServiceImplUnitTest extends BaseUnitTest verify(mockedRecordsManagementAuditService).auditEvent(eq(activeContent), anyString()); } - @Test + @Test (expected = AccessDeniedException.class) 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 + @Test (expected = AccessDeniedException.class) 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); } - @Test + @Test (expected = IntegrityException.class) public void addArchivedContentToHold() { - expectedEx.expect(AlfrescoRuntimeException.class); - expectedEx.expectMessage("Archived nodes can't be added to hold."); - - when(mockedNodeService.getProperty(activeContent, ContentModel.PROP_NAME)).thenReturn(ACTIVE_CONTENT_NAME); when(mockedNodeService.hasAspect(activeContent, RecordsManagementModel.ASPECT_ARCHIVED)).thenReturn(true); holdService.addToHold(hold, activeContent); } From c12e300919037eaa53fa6f0304ff56404159c2e4 Mon Sep 17 00:00:00 2001 From: cagache Date: Tue, 6 Aug 2019 16:25:08 +0300 Subject: [PATCH 12/13] fix sonar issues --- .../freeze/FreezeServiceImpl.java | 4 ++-- .../hold/HoldServiceImpl.java | 22 +++++++++---------- .../model/rma/aspect/FrozenAspect.java | 2 +- 3 files changed, 14 insertions(+), 14 deletions(-) diff --git a/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/freeze/FreezeServiceImpl.java b/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/freeze/FreezeServiceImpl.java index 63a6c9cdb1..762e3188fb 100644 --- a/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/freeze/FreezeServiceImpl.java +++ b/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/freeze/FreezeServiceImpl.java @@ -171,7 +171,7 @@ public class FreezeServiceImpl extends ServiceBaseImpl NodeRef hold = null; if (!nodeRefs.isEmpty()) { - List list = new ArrayList<>(nodeRefs); + final List list = new ArrayList<>(nodeRefs); hold = createHold(list.get(0), reason); getHoldService().addToHold(hold, list); } @@ -307,7 +307,7 @@ public class FreezeServiceImpl extends ServiceBaseImpl { for (ChildAssociationRef childAssociationRef : childAssocs) { - NodeRef childRef = childAssociationRef.getChildRef(); + final NodeRef childRef = childAssociationRef.getChildRef(); if (childAssociationRef.isPrimary() && isFrozen(childRef)) { heldCount ++; diff --git a/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/hold/HoldServiceImpl.java b/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/hold/HoldServiceImpl.java index 167645696f..9045c1144b 100644 --- a/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/hold/HoldServiceImpl.java +++ b/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/hold/HoldServiceImpl.java @@ -90,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; @@ -296,7 +299,7 @@ public class HoldServiceImpl extends ServiceBaseImpl // check whether the record is held by virtue of it's record folder if (isRecord(nodeRef)) { - List recordFolders = recordFolderService.getRecordFolders(nodeRef); + final List recordFolders = recordFolderService.getRecordFolders(nodeRef); for (NodeRef recordFolder : recordFolders) { holdsIncludingNodeRef.addAll(getParentHolds(recordFolder)); @@ -305,7 +308,7 @@ public class HoldServiceImpl extends ServiceBaseImpl if (!includedInHold) { - Set filePlans = filePlanService.getFilePlans(); + final Set filePlans = filePlanService.getFilePlans(); if (!CollectionUtils.isEmpty(filePlans)) { List holdsNotIncludingNodeRef = new ArrayList<>(); @@ -562,7 +565,7 @@ public class HoldServiceImpl extends ServiceBaseImpl if (permissionService.hasPermission(hold, RMPermissionModel.FILING) == AccessStatus.DENIED) { - throw new AccessDeniedException(I18NUtil.getMessage("permissions.err_access_denied")); + throw new AccessDeniedException(I18NUtil.getMessage(MSG_ERR_ACCESS_DENIED)); } // check that the node isn't already in the hold @@ -609,15 +612,12 @@ public class HoldServiceImpl extends ServiceBaseImpl 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) + 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("permissions.err_access_denied")); - } - else if (instanceOf(nodeRef, ContentModel.TYPE_CONTENT) && - permissionService.hasPermission(nodeRef, PermissionService.WRITE) == AccessStatus.DENIED) - { - throw new AccessDeniedException(I18NUtil.getMessage("permissions.err_access_denied")); + throw new AccessDeniedException(I18NUtil.getMessage(MSG_ERR_ACCESS_DENIED)); } if (nodeService.hasAspect(nodeRef, ASPECT_ARCHIVED)) diff --git a/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/model/rma/aspect/FrozenAspect.java b/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/model/rma/aspect/FrozenAspect.java index 20f9256ec3..deadb823e2 100644 --- a/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/model/rma/aspect/FrozenAspect.java +++ b/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/model/rma/aspect/FrozenAspect.java @@ -165,7 +165,7 @@ public class FrozenAspect extends BaseBehaviourBean if (nodeService.exists(nodeRef) && (isRecord(nodeRef) || instanceOf(nodeRef, TYPE_CONTENT))) { // get the owning folder - NodeRef parentRef = nodeService.getPrimaryParent(nodeRef).getParentRef(); + final NodeRef parentRef = nodeService.getPrimaryParent(nodeRef).getParentRef(); // check that the aspect has been added if (nodeService.hasAspect(parentRef, ASPECT_HELD_CHILDREN)) { From cc07996323b6cb141b3ab512d8d22d68c2d1bcf9 Mon Sep 17 00:00:00 2001 From: cagache Date: Wed, 7 Aug 2019 12:38:18 +0300 Subject: [PATCH 13/13] fix sonar issues --- .../hold/HoldServiceImpl.java | 16 +++---- .../model/rma/aspect/FrozenAspect.java | 44 ++++++++----------- 2 files changed, 27 insertions(+), 33 deletions(-) diff --git a/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/hold/HoldServiceImpl.java b/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/hold/HoldServiceImpl.java index 9045c1144b..93ca463d99 100644 --- a/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/hold/HoldServiceImpl.java +++ b/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/hold/HoldServiceImpl.java @@ -294,13 +294,13 @@ public class HoldServiceImpl extends ServiceBaseImpl List result = new ArrayList<>(); // get all the immediate parent holds - Set holdsIncludingNodeRef = getParentHolds(nodeRef); + final Set holdsIncludingNodeRef = getParentHolds(nodeRef); // check whether the record is held by virtue of it's record folder if (isRecord(nodeRef)) { final List recordFolders = recordFolderService.getRecordFolders(nodeRef); - for (NodeRef recordFolder : recordFolders) + for (final NodeRef recordFolder : recordFolders) { holdsIncludingNodeRef.addAll(getParentHolds(recordFolder)); } @@ -311,11 +311,11 @@ public class HoldServiceImpl extends ServiceBaseImpl final Set filePlans = filePlanService.getFilePlans(); if (!CollectionUtils.isEmpty(filePlans)) { - List holdsNotIncludingNodeRef = new ArrayList<>(); + final List holdsNotIncludingNodeRef = new ArrayList<>(); filePlans.forEach(filePlan -> { // invert list to get list of holds that do not contain this node - List allHolds = getHolds(filePlan); + final List allHolds = getHolds(filePlan); holdsNotIncludingNodeRef.addAll(ListUtils.subtract(allHolds, new ArrayList<>(holdsIncludingNodeRef))); }); result = holdsNotIncludingNodeRef; @@ -559,7 +559,7 @@ public class HoldServiceImpl extends ServiceBaseImpl { if (!isHold(hold)) { - String holdName = (String) nodeService.getProperty(hold, ContentModel.PROP_NAME); + final String holdName = (String) nodeService.getProperty(hold, ContentModel.PROP_NAME); throw new IntegrityException(I18NUtil.getMessage("rm.hold.not-hold", holdName), null); } @@ -574,7 +574,7 @@ public class HoldServiceImpl extends ServiceBaseImpl // run as system to ensure we have all the appropriate permissions to perform the manipulations we require authenticationUtil.runAsSystem((RunAsWork) () -> { // gather freeze properties - Map props = new HashMap<>(2); + final Map props = new HashMap<>(2); props.put(PROP_FROZEN_AT, new Date()); props.put(PROP_FROZEN_BY, AuthenticationUtil.getFullyAuthenticatedUser()); @@ -589,7 +589,7 @@ public class HoldServiceImpl extends ServiceBaseImpl // Mark all the folders contents as frozen if (isRecordFolder(nodeRef)) { - List records = recordService.getRecords(nodeRef); + final List records = recordService.getRecords(nodeRef); records.forEach(record -> addFrozenAspect(record, props)); } @@ -608,7 +608,7 @@ public class HoldServiceImpl extends ServiceBaseImpl { if (!isRecord(nodeRef) && !isRecordFolder(nodeRef) && !instanceOf(nodeRef, ContentModel.TYPE_CONTENT)) { - String nodeName = (String) nodeService.getProperty(nodeRef, ContentModel.PROP_NAME); + final String nodeName = (String) nodeService.getProperty(nodeRef, ContentModel.PROP_NAME); throw new IntegrityException(I18NUtil.getMessage("rm.hold.add-to-hold-invalid-type", nodeName), null); } diff --git a/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/model/rma/aspect/FrozenAspect.java b/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/model/rma/aspect/FrozenAspect.java index deadb823e2..c3dcc1f4fb 100644 --- a/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/model/rma/aspect/FrozenAspect.java +++ b/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/model/rma/aspect/FrozenAspect.java @@ -157,37 +157,31 @@ public class FrozenAspect extends BaseBehaviourBean ) public void onAddAspect(final NodeRef nodeRef, final QName aspectTypeQName) { - AuthenticationUtil.runAsSystem(new RunAsWork() - { - @Override - public Void doWork() + AuthenticationUtil.runAsSystem((RunAsWork) () -> { + if (nodeService.exists(nodeRef) && (isRecord(nodeRef) || instanceOf(nodeRef, TYPE_CONTENT))) { - if (nodeService.exists(nodeRef) && (isRecord(nodeRef) || instanceOf(nodeRef, TYPE_CONTENT))) + // get the owning folder + final NodeRef parentRef = nodeService.getPrimaryParent(nodeRef).getParentRef(); + // check that the aspect has been added + if (nodeService.hasAspect(parentRef, ASPECT_HELD_CHILDREN)) { - // get the owning folder - final NodeRef parentRef = nodeService.getPrimaryParent(nodeRef).getParentRef(); - // check that the aspect has been added - if (nodeService.hasAspect(parentRef, ASPECT_HELD_CHILDREN)) + // increment current count + int currentCount = (Integer) nodeService.getProperty(parentRef, PROP_HELD_CHILDREN_COUNT); + currentCount = currentCount + 1; + nodeService.setProperty(parentRef, PROP_HELD_CHILDREN_COUNT, currentCount); + } else + { + if (instanceOf(parentRef, TYPE_FOLDER) && !nodeService.hasAspect(parentRef, ASPECT_SITE_CONTAINER)) { - // increment current count - int currentCount = (Integer) nodeService.getProperty(parentRef, PROP_HELD_CHILDREN_COUNT); - currentCount = currentCount + 1; - nodeService.setProperty(parentRef, PROP_HELD_CHILDREN_COUNT, currentCount); - } - else - { - if(instanceOf(parentRef, TYPE_FOLDER) && !nodeService.hasAspect(parentRef, ASPECT_SITE_CONTAINER)) - { - // add aspect and set count to 1 - Map props = new HashMap<>(1); - props.put(PROP_HELD_CHILDREN_COUNT, 1); - getInternalNodeService().addAspect(parentRef, ASPECT_HELD_CHILDREN, props); - } + // add aspect and set count to 1 + final Map props = new HashMap<>(1); + props.put(PROP_HELD_CHILDREN_COUNT, 1); + getInternalNodeService().addAspect(parentRef, ASPECT_HELD_CHILDREN, props); } } - return null; } - }); + return null; + }); } @Override