From 27496b81f1590bcfccbbd5b8eba9e9d82bb3fdf1 Mon Sep 17 00:00:00 2001 From: cagache Date: Wed, 21 Aug 2019 18:19:18 +0300 Subject: [PATCH 1/4] code review comments --- .../hold/HoldServiceImpl.java | 2 +- .../model/rma/aspect/FrozenAspect.java | 5 +- .../script/hold/BaseHold.java | 2 +- .../rma/aspect/FrozenAspectUnitTest.java | 3 +- .../util/NodeTypeUtilityUnitTest.java | 91 +++++++++++++++++++ 5 files changed, 97 insertions(+), 6 deletions(-) create mode 100644 rm-community/rm-community-repo/unit-test/java/org/alfresco/module/org_alfresco_module_rm/util/NodeTypeUtilityUnitTest.java 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 a4e7c868af..ffbcf8e0c2 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 @@ -616,7 +616,7 @@ public class HoldServiceImpl extends ServiceBaseImpl */ private void checkNodeCanBeAddedToHold(NodeRef nodeRef) { - if (!isRecord(nodeRef) && !isRecordFolder(nodeRef) && !instanceOf(nodeRef, ContentModel.TYPE_CONTENT)) + if (!isRecordFolder(nodeRef) && !instanceOf(nodeRef, ContentModel.TYPE_CONTENT)) { 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 28b61a2d3f..7f4648fb6c 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 @@ -140,7 +140,7 @@ public class FrozenAspect extends BaseBehaviourBean public void onAddAspect(final NodeRef nodeRef, final QName aspectTypeQName) { AuthenticationUtil.runAsSystem((RunAsWork) () -> { - if (nodeService.exists(nodeRef) && (isRecord(nodeRef) || instanceOf(nodeRef, TYPE_CONTENT))) + if (nodeService.exists(nodeRef) && instanceOf(nodeRef, TYPE_CONTENT)) { // get the owning folder final NodeRef parentRef = nodeService.getPrimaryParent(nodeRef).getParentRef(); @@ -177,8 +177,7 @@ public class FrozenAspect extends BaseBehaviourBean { AuthenticationUtil.runAsSystem((RunAsWork) () -> { - if (nodeService.exists(nodeRef) && - (isRecord(nodeRef) || instanceOf(nodeRef, TYPE_CONTENT))) + if (nodeService.exists(nodeRef) && instanceOf(nodeRef, TYPE_CONTENT)) { // get the owning folder final NodeRef owningFolder = nodeService.getPrimaryParent(nodeRef).getParentRef(); diff --git a/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/script/hold/BaseHold.java b/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/script/hold/BaseHold.java index 7a46004e3a..d855cead9d 100644 --- a/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/script/hold/BaseHold.java +++ b/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/script/hold/BaseHold.java @@ -219,7 +219,7 @@ public abstract class BaseHold extends DeclarativeWebScript } // ensure that the node we are adding to the hold is a record or record folder or active content - if (!recordService.isRecord(nodeRef) && !recordFolderService.isRecordFolder(nodeRef) && + if (!recordFolderService.isRecordFolder(nodeRef) && !nodeTypeUtility.instanceOf(nodeService.getType(nodeRef), ContentModel.TYPE_CONTENT)) { throw new WebScriptException(Status.STATUS_BAD_REQUEST, "Items added to a hold must be either a record, a record folder or active content."); diff --git a/rm-community/rm-community-repo/unit-test/java/org/alfresco/module/org_alfresco_module_rm/model/rma/aspect/FrozenAspectUnitTest.java b/rm-community/rm-community-repo/unit-test/java/org/alfresco/module/org_alfresco_module_rm/model/rma/aspect/FrozenAspectUnitTest.java index 4559ecd4be..78a0b7b38d 100644 --- a/rm-community/rm-community-repo/unit-test/java/org/alfresco/module/org_alfresco_module_rm/model/rma/aspect/FrozenAspectUnitTest.java +++ b/rm-community/rm-community-repo/unit-test/java/org/alfresco/module/org_alfresco_module_rm/model/rma/aspect/FrozenAspectUnitTest.java @@ -108,6 +108,8 @@ public class FrozenAspectUnitTest { MockitoAnnotations.initMocks(this); when(mockNodeService.exists(record)).thenReturn(true); + when(mockNodeService.getType(record)).thenReturn(ContentModel.TYPE_CONTENT); + when(mockedNodeTypeUtility.instanceOf(mockNodeService.getType(record), ContentModel.TYPE_CONTENT)).thenReturn(true); when(mockNodeService.exists(content)).thenReturn(true); when(mockNodeService.hasAspect(folder, ASPECT_HELD_CHILDREN)).thenReturn(true); when(mockNodeService.getProperty(folder, PROP_HELD_CHILDREN_COUNT)).thenReturn(1); @@ -116,7 +118,6 @@ public class FrozenAspectUnitTest children.add(mockChildRef); when(mockNodeService.getChildAssocs(content)).thenReturn(children); when(mockChildRef.isPrimary()).thenReturn(true); - when(mockNodeService.hasAspect(record, ASPECT_RECORD)).thenReturn(true); } /** diff --git a/rm-community/rm-community-repo/unit-test/java/org/alfresco/module/org_alfresco_module_rm/util/NodeTypeUtilityUnitTest.java b/rm-community/rm-community-repo/unit-test/java/org/alfresco/module/org_alfresco_module_rm/util/NodeTypeUtilityUnitTest.java new file mode 100644 index 0000000000..eceac23478 --- /dev/null +++ b/rm-community/rm-community-repo/unit-test/java/org/alfresco/module/org_alfresco_module_rm/util/NodeTypeUtilityUnitTest.java @@ -0,0 +1,91 @@ +/* + * #%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.util; + +import static org.mockito.Matchers.any; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import org.alfresco.module.org_alfresco_module_rm.test.util.AlfMock; +import org.alfresco.service.cmr.dictionary.DictionaryService; +import org.alfresco.service.namespace.QName; +import org.junit.Before; +import org.junit.Test; +import org.mockito.InjectMocks; +import org.mockito.Mock; +import org.mockito.MockitoAnnotations; + +/** + * Unit test class for NodeTypeUtility + * + * @author Claudia Agache + * @since 3.2 + */ +public class NodeTypeUtilityUnitTest +{ + @InjectMocks + private NodeTypeUtility nodeTypeUtility; + + @Mock + private DictionaryService mockedDictionaryService; + + private QName type, ofType; + + @Before + public void setUp() + { + MockitoAnnotations.initMocks(this); + type = AlfMock.generateQName(); + ofType = AlfMock.generateQName(); + } + + /** test that instanceOf returns false if verified type is not subtype of the other */ + @Test + public void testNotInstanceOf() + { + when(mockedDictionaryService.isSubClass(type, ofType)).thenReturn(false); + when(nodeTypeUtility.instanceOf(type, ofType)).thenReturn(false); + } + + /** test that instanceOf returns true if verified type is subtype of the other */ + @Test + public void testIsInstanceOf() + { + when(mockedDictionaryService.isSubClass(type, ofType)).thenReturn(true); + when(nodeTypeUtility.instanceOf(type, ofType)).thenReturn(true); + } + + /** test that instanceOf checks the cache when verifying the same type twice */ + @Test + public void testInstanceOfCacheSameTypes() + { + nodeTypeUtility.instanceOf(type, ofType); + nodeTypeUtility.instanceOf(type, ofType); + verify(mockedDictionaryService, times(1)).isSubClass(any(), any()); + } +} From f169e79599eb5402ce93bb36da1cbe2ef3d0e8c4 Mon Sep 17 00:00:00 2001 From: cagache Date: Wed, 21 Aug 2019 18:38:37 +0300 Subject: [PATCH 2/4] fix tests --- .../util/NodeTypeUtilityUnitTest.java | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/rm-community/rm-community-repo/unit-test/java/org/alfresco/module/org_alfresco_module_rm/util/NodeTypeUtilityUnitTest.java b/rm-community/rm-community-repo/unit-test/java/org/alfresco/module/org_alfresco_module_rm/util/NodeTypeUtilityUnitTest.java index eceac23478..7b6d0eef6d 100644 --- a/rm-community/rm-community-repo/unit-test/java/org/alfresco/module/org_alfresco_module_rm/util/NodeTypeUtilityUnitTest.java +++ b/rm-community/rm-community-repo/unit-test/java/org/alfresco/module/org_alfresco_module_rm/util/NodeTypeUtilityUnitTest.java @@ -26,6 +26,8 @@ */ package org.alfresco.module.org_alfresco_module_rm.util; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; import static org.mockito.Matchers.any; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; @@ -69,7 +71,7 @@ public class NodeTypeUtilityUnitTest public void testNotInstanceOf() { when(mockedDictionaryService.isSubClass(type, ofType)).thenReturn(false); - when(nodeTypeUtility.instanceOf(type, ofType)).thenReturn(false); + assertFalse(nodeTypeUtility.instanceOf(type, ofType)); } /** test that instanceOf returns true if verified type is subtype of the other */ @@ -77,7 +79,7 @@ public class NodeTypeUtilityUnitTest public void testIsInstanceOf() { when(mockedDictionaryService.isSubClass(type, ofType)).thenReturn(true); - when(nodeTypeUtility.instanceOf(type, ofType)).thenReturn(true); + assertTrue(nodeTypeUtility.instanceOf(type, ofType)); } /** test that instanceOf checks the cache when verifying the same type twice */ @@ -85,7 +87,19 @@ public class NodeTypeUtilityUnitTest public void testInstanceOfCacheSameTypes() { nodeTypeUtility.instanceOf(type, ofType); + verify(mockedDictionaryService, times(1)).isSubClass(any(), any()); nodeTypeUtility.instanceOf(type, ofType); verify(mockedDictionaryService, times(1)).isSubClass(any(), any()); } + + /** test the invocations when verifying different types */ + @Test + public void testInstanceOfDifferentTypes() + { + QName anotherType = AlfMock.generateQName(); + nodeTypeUtility.instanceOf(type, ofType); + verify(mockedDictionaryService, times(1)).isSubClass(any(), any()); + nodeTypeUtility.instanceOf(anotherType, ofType); + verify(mockedDictionaryService, times(2)).isSubClass(any(), any()); + } } From 903791e1dbf5c3ea1b9d30afbaf4b3b32060325a Mon Sep 17 00:00:00 2001 From: cagache Date: Wed, 21 Aug 2019 18:56:27 +0300 Subject: [PATCH 3/4] code review comments --- .../util/NodeTypeUtilityUnitTest.java | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/rm-community/rm-community-repo/unit-test/java/org/alfresco/module/org_alfresco_module_rm/util/NodeTypeUtilityUnitTest.java b/rm-community/rm-community-repo/unit-test/java/org/alfresco/module/org_alfresco_module_rm/util/NodeTypeUtilityUnitTest.java index 7b6d0eef6d..a0b73aa9f7 100644 --- a/rm-community/rm-community-repo/unit-test/java/org/alfresco/module/org_alfresco_module_rm/util/NodeTypeUtilityUnitTest.java +++ b/rm-community/rm-community-repo/unit-test/java/org/alfresco/module/org_alfresco_module_rm/util/NodeTypeUtilityUnitTest.java @@ -102,4 +102,12 @@ public class NodeTypeUtilityUnitTest nodeTypeUtility.instanceOf(anotherType, ofType); verify(mockedDictionaryService, times(2)).isSubClass(any(), any()); } + + /** test that instanceOf returns true if verified type is equal to the other */ + @Test + public void testTypesAreEqual() + { + assertTrue(nodeTypeUtility.instanceOf(type, type)); + verify(mockedDictionaryService, times(0)).isSubClass(any(), any()); + } } From becbf296df9d0855e53d4852f42451a2f5672f78 Mon Sep 17 00:00:00 2001 From: cagache Date: Thu, 22 Aug 2019 08:30:18 +0300 Subject: [PATCH 4/4] code review comments --- .../org_alfresco_module_rm/util/NodeTypeUtilityUnitTest.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/rm-community/rm-community-repo/unit-test/java/org/alfresco/module/org_alfresco_module_rm/util/NodeTypeUtilityUnitTest.java b/rm-community/rm-community-repo/unit-test/java/org/alfresco/module/org_alfresco_module_rm/util/NodeTypeUtilityUnitTest.java index a0b73aa9f7..2e8be8b789 100644 --- a/rm-community/rm-community-repo/unit-test/java/org/alfresco/module/org_alfresco_module_rm/util/NodeTypeUtilityUnitTest.java +++ b/rm-community/rm-community-repo/unit-test/java/org/alfresco/module/org_alfresco_module_rm/util/NodeTypeUtilityUnitTest.java @@ -87,7 +87,6 @@ public class NodeTypeUtilityUnitTest public void testInstanceOfCacheSameTypes() { nodeTypeUtility.instanceOf(type, ofType); - verify(mockedDictionaryService, times(1)).isSubClass(any(), any()); nodeTypeUtility.instanceOf(type, ofType); verify(mockedDictionaryService, times(1)).isSubClass(any(), any()); } @@ -98,7 +97,6 @@ public class NodeTypeUtilityUnitTest { QName anotherType = AlfMock.generateQName(); nodeTypeUtility.instanceOf(type, ofType); - verify(mockedDictionaryService, times(1)).isSubClass(any(), any()); nodeTypeUtility.instanceOf(anotherType, ofType); verify(mockedDictionaryService, times(2)).isSubClass(any(), any()); }