From f62235bd01c3847e565dfb615651aba85cf39b6b Mon Sep 17 00:00:00 2001 From: cagache Date: Thu, 1 Aug 2019 16:02:55 +0300 Subject: [PATCH] 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); }