From bc5c5bd51d5c0100de8ebffc5dba88d00b985e8c Mon Sep 17 00:00:00 2001 From: Chris Shields Date: Tue, 19 Nov 2019 21:11:50 +0000 Subject: [PATCH 1/2] Code review changes --- .../audit/event/RemoveFromHoldAuditEvent.java | 6 ++- ...RecordsManagementAuditServiceImplTest.java | 42 +++++++------------ .../test/util/CommonRMTestUtils.java | 2 +- .../RemoveFromHoldAuditEventUnitTest.java | 4 +- 4 files changed, 21 insertions(+), 33 deletions(-) diff --git a/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/audit/event/RemoveFromHoldAuditEvent.java b/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/audit/event/RemoveFromHoldAuditEvent.java index 48f419f701..2141f524c6 100644 --- a/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/audit/event/RemoveFromHoldAuditEvent.java +++ b/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/audit/event/RemoveFromHoldAuditEvent.java @@ -49,7 +49,9 @@ import org.alfresco.service.namespace.QName; @BehaviourBean public class RemoveFromHoldAuditEvent extends AuditEvent implements HoldServicePolicies.OnRemoveFromHoldPolicy { - /** Node Service */ + /** + * Node Service + */ private NodeService nodeService; /** @@ -63,7 +65,7 @@ public class RemoveFromHoldAuditEvent extends AuditEvent implements HoldServiceP } /** - * @see @see org.alfresco.module.org_alfresco_module_rm.hold.HoldServicePolicies.OnRemoveFromHoldPolicy#onRemoveFromHold(org.alfresco.service.cmr.repository.NodeRef, org.alfresco.service.cmr.repository.NodeRef) + * @see org.alfresco.module.org_alfresco_module_rm.hold.HoldServicePolicies.OnRemoveFromHoldPolicy#onRemoveFromHold(org.alfresco.service.cmr.repository.NodeRef, org.alfresco.service.cmr.repository.NodeRef) */ @Override @Behaviour diff --git a/rm-community/rm-community-repo/test/java/org/alfresco/module/org_alfresco_module_rm/test/legacy/service/RecordsManagementAuditServiceImplTest.java b/rm-community/rm-community-repo/test/java/org/alfresco/module/org_alfresco_module_rm/test/legacy/service/RecordsManagementAuditServiceImplTest.java index 3d246fea3c..6aee148602 100644 --- a/rm-community/rm-community-repo/test/java/org/alfresco/module/org_alfresco_module_rm/test/legacy/service/RecordsManagementAuditServiceImplTest.java +++ b/rm-community/rm-community-repo/test/java/org/alfresco/module/org_alfresco_module_rm/test/legacy/service/RecordsManagementAuditServiceImplTest.java @@ -72,6 +72,12 @@ public class RecordsManagementAuditServiceImplTest extends BaseRMTestCase /** Test start time */ private Date testStartTime; + /** + * Remove from hold audit event name. + */ + private static final String REMOVE_FROM_HOLD_AUDIT_EVENT = "Remove From Hold"; + + /** * @see org.alfresco.module.org_alfresco_module_rm.test.util.BaseRMTestCase#setUp() */ @@ -748,13 +754,9 @@ public class RecordsManagementAuditServiceImplTest extends BaseRMTestCase { doBehaviourDrivenTest(new BehaviourDrivenTest() { - final static String REMOVE_FROM_HOLD_AUDIT_EVENT = "Remove From Hold"; - String holdName = "Hold " + GUID.generate(); NodeRef hold; - Map auditEventProperties; - @Override public void given() { @@ -772,7 +774,7 @@ public class RecordsManagementAuditServiceImplTest extends BaseRMTestCase @Override public void then() { - auditEventProperties = getAuditEntry(REMOVE_FROM_HOLD_AUDIT_EVENT).getBeforeProperties(); + Map auditEventProperties = getAuditEntry(REMOVE_FROM_HOLD_AUDIT_EVENT).getBeforeProperties(); // check remove from hold audit event includes the hold name assertEquals("Remove From Hold event does not include hold name.", holdName, @@ -806,14 +808,10 @@ public class RecordsManagementAuditServiceImplTest extends BaseRMTestCase { doBehaviourDrivenTest(new BehaviourDrivenTest() { - final static String REMOVE_FROM_HOLD_AUDIT_EVENT = "Remove From Hold"; - String holdName1 = "Hold " + GUID.generate(); String holdName2 = "Hold " + GUID.generate(); NodeRef hold1, hold2; - List> auditEventPropertiesList; - @Override public void given() { @@ -823,26 +821,21 @@ public class RecordsManagementAuditServiceImplTest extends BaseRMTestCase hold2 = utils.createHold(filePlan, holdName2, "Reason " + GUID.generate()); utils.addItemToHold(hold1, dmDocument); utils.addItemToHold(hold2, dmDocument); - } @Override public void when() { - utils.removeItemFromHold(hold1, dmDocument); - utils.removeItemFromHold(hold2, dmDocument); + utils.removeItemsFromHolds(Arrays.asList(hold1, hold2), Arrays.asList(dmDocument)); } @Override public void then() { - auditEventPropertiesList = getAuditEntries(REMOVE_FROM_HOLD_AUDIT_EVENT) - .stream() - .map(RecordsManagementAuditEntry::getBeforeProperties) - .collect(Collectors.toList()); + List auditEntries = getAuditEntries(REMOVE_FROM_HOLD_AUDIT_EVENT); - // check remove from hold audit event exists for both records - assertEquals(2, auditEventPropertiesList.size()); + // check remove from hold audit event exists for both holds + assertEquals(2, auditEntries.size()); } @Override @@ -867,13 +860,9 @@ public class RecordsManagementAuditServiceImplTest extends BaseRMTestCase { doBehaviourDrivenTest(new BehaviourDrivenTest() { - final static String REMOVE_FROM_HOLD_AUDIT_EVENT = "Remove From Hold"; - String holdName = "Hold " + GUID.generate(); NodeRef hold; - List> auditEventPropertiesList; - @Override public void given() { @@ -893,13 +882,10 @@ public class RecordsManagementAuditServiceImplTest extends BaseRMTestCase @Override public void then() { - auditEventPropertiesList = getAuditEntries(REMOVE_FROM_HOLD_AUDIT_EVENT) - .stream() - .map(RecordsManagementAuditEntry::getBeforeProperties) - .collect(Collectors.toList()); + List auditEntries = getAuditEntries(REMOVE_FROM_HOLD_AUDIT_EVENT); - // check remove from hold audit event exists for both records - assertEquals(2, auditEventPropertiesList.size()); + // check remove from hold audit event exists for both documents + assertEquals(2, auditEntries.size()); } @Override diff --git a/rm-community/rm-community-repo/test/java/org/alfresco/module/org_alfresco_module_rm/test/util/CommonRMTestUtils.java b/rm-community/rm-community-repo/test/java/org/alfresco/module/org_alfresco_module_rm/test/util/CommonRMTestUtils.java index 63721187e4..692ac87edc 100644 --- a/rm-community/rm-community-repo/test/java/org/alfresco/module/org_alfresco_module_rm/test/util/CommonRMTestUtils.java +++ b/rm-community/rm-community-repo/test/java/org/alfresco/module/org_alfresco_module_rm/test/util/CommonRMTestUtils.java @@ -456,7 +456,7 @@ public class CommonRMTestUtils implements RecordsManagementModel } /** - * Util method to remove items from a holds. + * Util method to remove items from holds. * * @param holdNodeRefs the list {@link NodeRef}s of the holds * @param contentNodeRef the list of items which will be removed from the given holds diff --git a/rm-community/rm-community-repo/unit-test/java/org/alfresco/module/org_alfresco_module_rm/audit/event/RemoveFromHoldAuditEventUnitTest.java b/rm-community/rm-community-repo/unit-test/java/org/alfresco/module/org_alfresco_module_rm/audit/event/RemoveFromHoldAuditEventUnitTest.java index 8c84469428..57fcaad37f 100644 --- a/rm-community/rm-community-repo/unit-test/java/org/alfresco/module/org_alfresco_module_rm/audit/event/RemoveFromHoldAuditEventUnitTest.java +++ b/rm-community/rm-community-repo/unit-test/java/org/alfresco/module/org_alfresco_module_rm/audit/event/RemoveFromHoldAuditEventUnitTest.java @@ -48,7 +48,7 @@ import org.mockito.InjectMocks; import org.mockito.Mock; /** - * Unit tests for {@link CreateHoldAuditEvent}. + * Unit tests for {@link RemoveFromHoldAuditEvent}. * * @author Chris Shields * @since 3.3 @@ -86,7 +86,7 @@ public class RemoveFromHoldAuditEventUnitTest extends BaseUnitTest * Check that the remove from hold event calls an audit event. */ @Test - public void testAddToHoldCausesAuditEvent() + public void testRemoveFromHoldCausesAuditEvent() { removeFromHoldAuditEvent.onRemoveFromHold(holdNodeRef, contentNodeRef); verify(mockedRecordsManagementAuditService, times(1)).auditEvent(eq(contentNodeRef), any(String.class), any(Map.class), isNull(Map.class), eq(true)); From 0b3f1c793db92787f7fe11725217435606f00173 Mon Sep 17 00:00:00 2001 From: Chris Shields Date: Wed, 20 Nov 2019 11:16:04 +0000 Subject: [PATCH 2/2] Code review changes --- ...RecordsManagementAuditServiceImplTest.java | 44 ++++++++++++------- .../hold/HoldServiceImplUnitTest.java | 1 - 2 files changed, 29 insertions(+), 16 deletions(-) diff --git a/rm-community/rm-community-repo/test/java/org/alfresco/module/org_alfresco_module_rm/test/legacy/service/RecordsManagementAuditServiceImplTest.java b/rm-community/rm-community-repo/test/java/org/alfresco/module/org_alfresco_module_rm/test/legacy/service/RecordsManagementAuditServiceImplTest.java index 6aee148602..6212c45b3f 100644 --- a/rm-community/rm-community-repo/test/java/org/alfresco/module/org_alfresco_module_rm/test/legacy/service/RecordsManagementAuditServiceImplTest.java +++ b/rm-community/rm-community-repo/test/java/org/alfresco/module/org_alfresco_module_rm/test/legacy/service/RecordsManagementAuditServiceImplTest.java @@ -34,7 +34,6 @@ import java.util.HashMap; import java.util.List; import java.util.Locale; import java.util.Map; -import java.util.stream.Collectors; import org.alfresco.model.ContentModel; import org.alfresco.module.org_alfresco_module_rm.audit.RecordsManagementAuditEntry; @@ -510,8 +509,7 @@ public class RecordsManagementAuditServiceImplTest extends BaseRMTestCase public void when() throws Exception { // set the audit wuery param - RecordsManagementAuditQueryParameters params = new RecordsManagementAuditQueryParameters(); - params.setEvent(DELETE_USER_AUDIT_EVENT); + RecordsManagementAuditQueryParameters params = createAuditQueryParameters(DELETE_USER_AUDIT_EVENT); // get the audit events for "Delete Person" entry = getAuditTrail(params, 1, ADMIN_USER); @@ -566,8 +564,7 @@ public class RecordsManagementAuditServiceImplTest extends BaseRMTestCase public void when() throws Exception { // set the audit query param - RecordsManagementAuditQueryParameters params = new RecordsManagementAuditQueryParameters(); - params.setEvent(CREATE_USER_AUDIT_EVENT); + RecordsManagementAuditQueryParameters params = createAuditQueryParameters(CREATE_USER_AUDIT_EVENT); // get the audit events for "Create Person" entry = getAuditTrail(params, 1, ADMIN_USER); @@ -950,13 +947,11 @@ public class RecordsManagementAuditServiceImplTest extends BaseRMTestCase private RecordsManagementAuditEntry getAuditEntry(String auditEvent) { - // set the audit query param for the given event - RecordsManagementAuditQueryParameters params = new RecordsManagementAuditQueryParameters(); - params.setEvent(auditEvent); + // create the audit query parameters for the given event + RecordsManagementAuditQueryParameters params = createAuditQueryParameters(auditEvent); // get the audit entries for the given event - List auditEntries; - auditEntries = getAuditTrail(params, 1, ADMIN_USER); + List auditEntries = getAuditEntryAssertOnlyOne(params); // verify we have the expected audit event RecordsManagementAuditEntry auditEntry = auditEntries.get(0); @@ -966,17 +961,36 @@ public class RecordsManagementAuditServiceImplTest extends BaseRMTestCase return auditEntry; } + private List getAuditEntryAssertOnlyOne(RecordsManagementAuditQueryParameters params) + { + List auditEntries; + auditEntries = getAuditTrail(params, 1, ADMIN_USER); + return auditEntries; + } + private List getAuditEntries(String auditEvent) { - // set the audit query param for the given event - RecordsManagementAuditQueryParameters params = new RecordsManagementAuditQueryParameters(); - params.setEvent(auditEvent); + // create the audit query parameters for the given event + RecordsManagementAuditQueryParameters params = createAuditQueryParameters(auditEvent); // get the audit entries for the given event - List auditEntries; - auditEntries = getAuditTrail(params, -1, ADMIN_USER); + List auditEntries = getAllAuditEntries(params); return auditEntries; } + private List getAllAuditEntries(RecordsManagementAuditQueryParameters params) + { + List auditEntries; + auditEntries = getAuditTrail(params, -1, ADMIN_USER); + return auditEntries; + } + + private RecordsManagementAuditQueryParameters createAuditQueryParameters(String auditEvent) + { + RecordsManagementAuditQueryParameters params = new RecordsManagementAuditQueryParameters(); + params.setEvent(auditEvent); + return params; + } + } 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 f86b3efb90..9543a3a3c5 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 @@ -477,7 +477,6 @@ public class HoldServiceImplUnitTest extends BaseUnitTest verify(mockedNodeService, never()).removeChild(hold, recordFolder); verify(mockedNodeService, never()).removeAspect(recordFolder, ASPECT_FROZEN); verify(mockedNodeService, never()).removeAspect(record, ASPECT_FROZEN); - verify(mockedRecordsManagementAuditService, never()).auditEvent(eq(recordFolder), anyString()); } @Test