code review comments

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);

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<Void>) () -> {
-            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<Void>) () -> {
 
-            if (nodeService.exists(nodeRef) &&
+            if (nodeService.exists(nodeRef) && instanceOf(nodeRef, TYPE_CONTENT))
-                    (isRecord(nodeRef) || instanceOf(nodeRef, TYPE_CONTENT)))
             {
                 // get the owning folder
                 final NodeRef owningFolder = nodeService.getPrimaryParent(nodeRef).getParentRef();

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.");

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);
     }
 
     /**

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 <http://www.gnu.org/licenses/>.
+ * #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());
+    }
+}