REPO-1646: V1 REST API - cannot unset optional fields (eg. when updating person / site details ...) - minor fixes with tests (update person)

- ensure enabled & emailNotificationsEnabled cannot be null
- null/empty company object should unset all fields (fix for empty case)

git-svn-id: https://svn.alfresco.com/repos/alfresco-enterprise/alfresco/BRANCHES/DEV/5.2.N/root@133351 c4b6b30b-aa2e-2d43-bbcb-ca4b014f7261

source/java/org/alfresco/rest/api/impl/PeopleImpl.java

@@ -526,10 +526,18 @@ public class PeopleImpl implements People
 		{
 			throw new InvalidArgumentException("Field '"+fieldName+"' is null, but is required.");
 		}
+
+        // belts-and-braces - note: should not see empty string (since converted to null via custom json deserializer)
+        if ((fieldValue instanceof String) && ((String)fieldValue).isEmpty())
+        {
+            throw new InvalidArgumentException("Field '"+fieldName+"' is empty, but is required.");
+        }
 	}
 
     public Person update(String personId, final Person person)
     {
+        validateUpdatePersonData(person);
+
         boolean isAdmin = isAdminAuthority();
 
         String currentUserId = AuthenticationUtil.getFullyAuthenticatedUser();
@@ -545,6 +553,18 @@ public class PeopleImpl implements People
         // if requested, update password
         updatePassword(isAdmin, personIdToUpdate, person);
 
+        if (person.isEnabled() != null)
+        {
+            if (isAdminAuthority(personIdToUpdate))
+            {
+                throw new PermissionDeniedException("Admin authority cannot be disabled.");
+            }
+
+            // note: if current user is not an admin then permission denied exception is thrown
+            MutableAuthenticationService mutableAuthenticationService = (MutableAuthenticationService) authenticationService;
+            mutableAuthenticationService.setAuthenticationEnabled(personIdToUpdate, person.isEnabled());
+        }
+
 		NodeRef personNodeRef = personService.getPerson(personIdToUpdate, false);
 		if (person.wasSet(Person.PROP_PERSON_DESCRIPTION))
         {
@@ -579,6 +599,29 @@ public class PeopleImpl implements People
         return getPerson(personId);
     }
 
+    private void validateUpdatePersonData(Person person)
+    {
+        if (person.wasSet(ContentModel.PROP_FIRSTNAME))
+        {
+            checkRequiredField("firstName", person.getFirstName());
+        }
+
+        if (person.wasSet(ContentModel.PROP_EMAIL))
+        {
+            checkRequiredField("email", person.getEmail());
+        }
+
+        if (person.wasSet(ContentModel.PROP_ENABLED) && (person.isEnabled() == null))
+        {
+            throw new IllegalArgumentException("'enabled' field cannot be empty.");
+        }
+
+        if (person.wasSet(ContentModel.PROP_EMAIL_FEED_DISABLED) && (person.isEmailNotificationsEnabled() == null))
+        {
+            throw new IllegalArgumentException("'emailNotificationsEnabled' field cannot be empty.");
+        }
+    }
+
     private void updatePassword(boolean isAdmin, String personIdToUpdate, Person person)
     {
         MutableAuthenticationService mutableAuthenticationService = (MutableAuthenticationService) authenticationService;
@@ -626,17 +669,6 @@ public class PeopleImpl implements People
                 mutableAuthenticationService.setAuthentication(personIdToUpdate, newPassword);
             }
         }
-
-        if (person.isEnabled() != null)
-        {
-            if (isAdminAuthority(personIdToUpdate))
-            {
-                throw new PermissionDeniedException("Admin authority cannot be disabled.");
-            }
-
-            mutableAuthenticationService.setAuthenticationEnabled(personIdToUpdate, person.isEnabled());
-        }
-
     }
 
     private boolean isAdminAuthority()

source/java/org/alfresco/rest/api/model/Person.java

@@ -493,50 +493,62 @@ public class Person
 		if (wasSet(PROP_PERSON_COMPANY))
 		{
 			Company company = getCompany();
+
+			int setCount = 0;
 			if (company != null)
 			{
 				if (company.wasSet(ContentModel.PROP_ORGANIZATION))
 				{
+					setCount++;
 					properties.put(ContentModel.PROP_ORGANIZATION, company.getOrganization());
 				}
-				
+
 				if (company.wasSet(ContentModel.PROP_COMPANYADDRESS1))
 				{
+					setCount++;
 					properties.put(ContentModel.PROP_COMPANYADDRESS1, company.getAddress1());
 				}
-				
+
 				if (company.wasSet(ContentModel.PROP_COMPANYADDRESS2))
 				{
+					setCount++;
 					properties.put(ContentModel.PROP_COMPANYADDRESS2, company.getAddress2());
 				}
-				
+
 				if (company.wasSet(ContentModel.PROP_COMPANYADDRESS3))
 				{
+					setCount++;
 					properties.put(ContentModel.PROP_COMPANYADDRESS3, company.getAddress3());
 				}
 
 				if (company.wasSet(ContentModel.PROP_COMPANYPOSTCODE))
 				{
+					setCount++;
 					properties.put(ContentModel.PROP_COMPANYPOSTCODE, company.getPostcode());
 				}
 
 				if (company.wasSet(ContentModel.PROP_COMPANYTELEPHONE))
 				{
+					setCount++;
 					properties.put(ContentModel.PROP_COMPANYTELEPHONE, company.getTelephone());
 				}
-				
+
 				if (company.wasSet(ContentModel.PROP_COMPANYFAX))
 				{
+					setCount++;
 					properties.put(ContentModel.PROP_COMPANYFAX, company.getFax());
 				}
 
 				if (company.wasSet(ContentModel.PROP_COMPANYEMAIL))
 				{
+					setCount++;
 					properties.put(ContentModel.PROP_COMPANYEMAIL, company.getEmail());
 				}
 			}
-			else
+
+			if (setCount == 0)
 			{
+				// company was null or {} (no individual properties set)
 				properties.put(ContentModel.PROP_ORGANIZATION, null);
 				properties.put(ContentModel.PROP_COMPANYADDRESS1, null);
 				properties.put(ContentModel.PROP_COMPANYADDRESS2, null);

source/test-java/org/alfresco/rest/api/tests/TestPeople.java

@@ -852,6 +852,11 @@ public class TestPeople extends EnterpriseTestApi
         // TODO: temp fix, set back to orig firstName
         publicApiClient.setRequestContext(new RequestContext(account1.getId(), account1Admin, "admin"));
         people.update(personId, qjson("{ `firstName`:`Bill` }"), 200);
+
+        // -ve test: check that required/mandatory/non-null fields cannot be unset (or empty string)
+        people.update("people", personId, null, null, qjson("{ `firstName`:`` }"), null, "Expected 400 response when updating " + personId, 400);
+        people.update("people", personId, null, null, qjson("{ `email`:`` }"), null, "Expected 400 response when updating " + personId, 400);
+        people.update("people", personId, null, null, qjson("{ `emailNotificationsEnabled`:`` }"), null, "Expected 400 response when updating " + personId, 400);
     }
 
     @Test
@@ -996,7 +1001,6 @@ public class TestPeople extends EnterpriseTestApi
                         + "  \"location\":null,\n"
 
                         + "  \"company\": {\n"
-                        + "    \"organization\":null,\n" 
                         + "    \"address1\":null,\n"
                         + "    \"address2\":null,\n"
                         + "    \"address3\":null,\n"
@@ -1024,7 +1028,8 @@ public class TestPeople extends EnterpriseTestApi
         assertNull(updatedPerson.getLocation());
 
         assertNotNull(updatedPerson.getCompany());
-        assertNull(updatedPerson.getCompany().getOrganization());
+        assertNotNull(updatedPerson.getCompany().getOrganization());
+
         assertNull(updatedPerson.getCompany().getAddress1());
         assertNull(updatedPerson.getCompany().getAddress2());
         assertNull(updatedPerson.getCompany().getAddress3());
@@ -1037,6 +1042,19 @@ public class TestPeople extends EnterpriseTestApi
         assertNull(updatedPerson.getTelephone());
         assertNull(updatedPerson.getUserStatus());
 
+        // test ability to unset company fields as a whole
+        response = people.update("people", personId, null, null,
+                "{\n"
+                        + "  \"company\": {} \n"
+                        + "}", params,
+                "Expected 200 response when updating " + personId, 200);
+
+        updatedPerson = Person.parsePerson((JSONObject) response.getJsonResponse().get("entry"));
+
+        // note: empty company object is returned for backwards compatibility (with pre-existing getPerson API <= 5.1)
+        assertNotNull(updatedPerson.getCompany());
+        assertNull(updatedPerson.getCompany().getOrganization());
+
         // set at least one company field
         String updatedOrgName = "another org";
 
@@ -1064,7 +1082,6 @@ public class TestPeople extends EnterpriseTestApi
 
         // note: empty company object is returned for backwards compatibility (with pre-existing getPerson API <= 5.1)
         assertNotNull(updatedPerson.getCompany());
-
         assertNull(updatedPerson.getCompany().getOrganization());
     }
 
@@ -1104,6 +1121,10 @@ public class TestPeople extends EnterpriseTestApi
             assertEquals(enabled, updatedPerson.isEnabled());
         }
 
+        // -ve test: enabled flag cannot be null/empty
+        people.update("people", personId, null, null, qjson("{ `enabled`: null }"), null, "Expected 400 response when updating " + personId, 400);
+        people.update("people", personId, null, null, qjson("{ `enabled`: `` }"), null, "Expected 400 response when updating " + personId, 400);
+
         // Use non-admin user's own credentials
         publicApiClient.setRequestContext(new RequestContext(account3.getId(), personId, "password"));