changes following review

rm-automation/rm-automation-community-rest-api/src/test/java/org/alfresco/rest/rm/community/records/CompleteRecordTests.java

@@ -61,7 +61,7 @@ public class CompleteRecordTests extends BaseRMRestTest
 {
     private static final Boolean COMPLETE = true;
     private static final Boolean INCOMPLETE = false;
-    private static final String parameters = "include=isCompleted";
+    private static final String PARAMETERS = "include=isCompleted";
 
     /**
      * Incomplete records with mandatory meta-data missing
@@ -106,7 +106,7 @@ public class CompleteRecordTests extends BaseRMRestTest
     /**
      * <pre>
      * Given the repository is configured to check mandatory data before completing a record
-     * And an incomplete record with missing mandatory meta-data
+     * And an incomplete record with its mandatory meta-data missing
      * When I complete the record
      * Then I receive an error indicating that I can't complete the operation,
      * because some of the mandatory meta-data of the record is missing
@@ -183,7 +183,7 @@ public class CompleteRecordTests extends BaseRMRestTest
     {
         // Get the recordsAPI
         RecordsAPI recordsAPI = getRestAPIFactory().getRecordsAPI();
-        recordsAPI.completeRecord(nonRecordId, parameters);
+        recordsAPI.completeRecord(nonRecordId, PARAMETERS);
         assertStatusCode(BAD_REQUEST);
     }
 
@@ -277,7 +277,7 @@ public class CompleteRecordTests extends BaseRMRestTest
     private void verifyRecordCompletionStatus(Record record, Boolean completionStatus)
     {
         RecordsAPI recordsAPI = getRestAPIFactory().getRecordsAPI();
-        Record recordModel = recordsAPI.getRecord(record.getId(), parameters);
+        Record recordModel = recordsAPI.getRecord(record.getId(), PARAMETERS);
         assertEquals(recordModel.getIsCompleted(), completionStatus);
     }
 
@@ -287,6 +287,6 @@ public class CompleteRecordTests extends BaseRMRestTest
     private void completeRecord(Record record) throws Exception
     {
         RecordsAPI recordsAPI = getRestAPIFactory().getRecordsAPI();
-        recordsAPI.completeRecord(record.getId(), parameters);
+        recordsAPI.completeRecord(record.getId(), PARAMETERS);
     }
 }

rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/action/impl/DeclareRecordAction.java

@@ -27,6 +27,9 @@
 
 package org.alfresco.module.org_alfresco_module_rm.action.impl;
 
+import java.util.HashMap;
+import java.util.Map;
+
 import org.alfresco.module.org_alfresco_module_rm.action.RMActionExecuterAbstractBase;
 import org.alfresco.module.org_alfresco_module_rm.record.RecordService;
 import org.alfresco.repo.action.executer.ActionExecuterAbstractBase;
@@ -42,6 +45,8 @@ import org.alfresco.util.ParameterCheck;
  */
 public class DeclareRecordAction extends RMActionExecuterAbstractBase
 {
+    private static final String MISSING_PROPERTIES = "missingProperties";
+    
     /** action name */
     public static final String NAME = "declareRecord";
 
@@ -71,11 +76,25 @@ public class DeclareRecordAction extends RMActionExecuterAbstractBase
         }
         catch (IntegrityException e)
         {
-            if (e.getMessage().contains("missing"))
+            Map<String, String> errorMap = getErrorMessageMap();
+            if (e.getMsgId().equals(errorMap.get(MISSING_PROPERTIES)))
             {
-                action.setParameterValue(ActionExecuterAbstractBase.PARAM_RESULT, "missingProperties");
+                action.setParameterValue(ActionExecuterAbstractBase.PARAM_RESULT, MISSING_PROPERTIES);
             }
         }
 
     }
+
+    /**
+     * TODO: needs to be properties file
+     * Temporary Helper method to get the map of complete record error message keys with associated message text
+     * @return errorMap
+     */
+    private Map<String, String> getErrorMessageMap() {
+        Map<String, String> errorMap = new HashMap<>();
+        errorMap.put("missingProperties", "The record has missing mandatory properties.");
+        errorMap.put("alreadyCompleted", "The record is already completed.");
+        errorMap.put("unsuitableNode", "The node is not a record or the record does not exist or is frozen.");
+        return errorMap;
+    }
 }

rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/record/RecordServiceImpl.java

@@ -1972,6 +1972,39 @@ public class RecordServiceImpl extends BaseBehaviourBean
     @Override
     public void complete(NodeRef nodeRef)
     {
+        validateForCompletion(nodeRef);
+        disablePropertyEditableCheck();
+        try
+        {
+            // Add the declared aspect
+            Map<QName, Serializable> declaredProps = new HashMap<>(2);
+            declaredProps.put(PROP_DECLARED_AT, new Date());
+            declaredProps.put(PROP_DECLARED_BY, AuthenticationUtil.getRunAsUser());
+            nodeService.addAspect(nodeRef, ASPECT_DECLARED_RECORD, declaredProps);
+
+            AuthenticationUtil.runAsSystem(new RunAsWork<Void>()
+            {
+                @Override
+                public Void doWork()
+                {
+                    // remove all owner related rights
+                    ownableService.setOwner(nodeRef, OwnableService.NO_OWNER);
+                    return null;
+                }
+            });
+        } finally
+        {
+            enablePropertyEditableCheck();
+        }
+    }
+
+    /**
+     * Helper method to validate whether the node is in a state suitable for completion
+     *
+     * @param nodeRef node reference
+     * @throws Exception if node not valid for completion
+     */
+    private void validateForCompletion(NodeRef nodeRef) {
         if (nodeService.exists(nodeRef) && isRecord(nodeRef) && !freezeService.isFrozen(nodeRef))
         {
             if (!isDeclared(nodeRef))
@@ -1985,32 +2018,7 @@ public class RecordServiceImpl extends BaseBehaviourBean
 
                 List<String> missingProperties = new ArrayList<>(5);
                 // Aspect not already defined - check mandatory properties then add
-                if (!checkMandatoryPropertiesEnabled || mandatoryPropertiesSet(nodeRef, missingProperties))
+                if (checkMandatoryPropertiesEnabled && !mandatoryPropertiesSet(nodeRef, missingProperties))
-                {
-                    disablePropertyEditableCheck();
-                    try
-                    {
-                        // Add the declared aspect
-                        Map<QName, Serializable> declaredProps = new HashMap<>(2);
-                        declaredProps.put(PROP_DECLARED_AT, new Date());
-                        declaredProps.put(PROP_DECLARED_BY, AuthenticationUtil.getRunAsUser());
-                        nodeService.addAspect(nodeRef, ASPECT_DECLARED_RECORD, declaredProps);
-
-                        AuthenticationUtil.runAsSystem(new RunAsWork<Void>()
-                        {
-                            @Override
-                            public Void doWork()
-                            {
-                                // remove all owner related rights
-                                ownableService.setOwner(nodeRef, OwnableService.NO_OWNER);
-                                return null;
-                            }
-                        });
-                    } finally
-                    {
-                        enablePropertyEditableCheck();
-                    }
-                } else
                 {
                     LOGGER.debug(buildMissingPropertiesErrorString(missingProperties));
                     throw new IntegrityException("The record has missing mandatory properties.", null);
@@ -2025,10 +2033,9 @@ public class RecordServiceImpl extends BaseBehaviourBean
             {
                 LOGGER.warn(I18NUtil.getMessage(MSG_UNDECLARED_ONLY_RECORDS, nodeRef.toString()));
             }
-            throw new IntegrityException("The record does not exist or is frozen.", null);
+            throw new IntegrityException("The node is not a record or the record does not exist or is frozen.", null);
         }
     }
-
     private String buildMissingPropertiesErrorString(List<String> missingProperties)
     {
         StringBuilder builder = new StringBuilder(255);
@@ -2087,29 +2094,17 @@ public class RecordServiceImpl extends BaseBehaviourBean
         {
             if (propDef.isMandatory() && nodeRefProps.get(propDef.getName()) == null)
             {
-                logMissingProperty(propDef, missingProperties);
+                if (LOGGER.isWarnEnabled())
-                ;
+                {
+                    StringBuilder msg = new StringBuilder();
+                    msg.append("Mandatory property missing: ").append(propDef.getName());
+                    LOGGER.warn(msg.toString());
+                }
+                missingProperties.add(propDef.getName().toString());
             }
         }
     }
 
-    /**
-     * Log information about missing properties.
-     *
-     * @param propDef           property definition
-     * @param missingProperties missing properties
-     */
-    private void logMissingProperty(PropertyDefinition propDef, List<String> missingProperties)
-    {
-        if (LOGGER.isWarnEnabled())
-        {
-            StringBuilder msg = new StringBuilder();
-            msg.append("Mandatory property missing: ").append(propDef.getName());
-            LOGGER.warn(msg.toString());
-        }
-        missingProperties.add(propDef.getName().toString());
-    }
-
     /**
      * Helper method to get the custom aspect for a given nodeRef type
      *