Merged 5.2.N (5.2.1) to HEAD (5.2)

131862 mward: Merged 131680:131794 from DEV/mward/5.2.n-createperson to 5.2.N
     REPO-892: make sure not all fields need to be supplied during create.
     REPO-892: throws error if fields exclusively belonging to Person (that are not part of PersonUpdate) are sent in request.
     REPO-892: cleaned up PersonUpdateJSONSerializer a little, by removing unnecessary 'fullVisibility' switch.
     REPO-892: improved test for optional fields; added test for too few fields.
     REPO-892: added tests (and impl where needed) for -ve response codes as given in the open api spec for create person.
     REPO-892: fixed broken test due to reuse of username.


git-svn-id: https://svn.alfresco.com/repos/alfresco-enterprise/alfresco/HEAD/root@132308 c4b6b30b-aa2e-2d43-bbcb-ca4b014f7261

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

@@ -39,6 +39,7 @@ import org.alfresco.rest.api.Sites;
 import org.alfresco.rest.api.model.Company;
 import org.alfresco.rest.api.model.Person;
 import org.alfresco.rest.api.model.PersonUpdate;
+import org.alfresco.rest.framework.core.exceptions.ConstraintViolatedException;
 import org.alfresco.rest.framework.core.exceptions.EntityNotFoundException;
 import org.alfresco.rest.framework.core.exceptions.InvalidArgumentException;
 import org.alfresco.service.cmr.repository.AssociationRef;
@@ -285,6 +286,19 @@ public class PeopleImpl implements People
 	@Override
 	public Person create(PersonUpdate person)
 	{
+		if (person.getUserName() == null)
+		{
+			throw new InvalidArgumentException("Field 'id' is null, but is required.");
+		}
+
+		// TODO: check, is this transaction safe?
+		// Unfortunately PersonService.createPerson(...) only throws an AlfrescoRuntimeException
+		// rather than a more specific exception and does not use a message ID either, so there's
+		// no sensible way to know that it was thrown due to the user already existing - hence this check here.
+		if (personService.personExists(person.getUserName()))
+		{
+			throw new ConstraintViolatedException("Person '"+person.getUserName()+"' already exists.");
+		}
 		Map<QName, Serializable> props = person.toProperties();
 		NodeRef nodeRef = personService.createPerson(props);
 

source/java/org/alfresco/rest/api/people/PeopleEntityResource.java

@@ -31,6 +31,7 @@ import org.alfresco.rest.api.model.PersonUpdate;
 import org.alfresco.rest.framework.WebApiDescription;
 import org.alfresco.rest.framework.WebApiParam;
 import org.alfresco.rest.framework.core.ResourceParameter;
+import org.alfresco.rest.framework.core.exceptions.InvalidArgumentException;
 import org.alfresco.rest.framework.resource.EntityResource;
 import org.alfresco.rest.framework.resource.actions.interfaces.EntityResourceAction;
 import org.alfresco.rest.framework.resource.parameters.Parameters;
@@ -86,18 +87,29 @@ public class PeopleEntityResource implements EntityResourceAction.ReadById<Perso
             kind= ResourceParameter.KIND.HTTP_BODY_OBJECT, allowMultiple=false)
     public List<Person> create(List<Person> persons, Parameters parameters)
     {
+        Person p = persons.get(0);
+
         // Until REPO-110 is solved, we need to explicitly test for the presence of fields
         // that are present on Person but not PersonUpdate
         // see also, SiteEntityResource.update(String, Site, Parameters)
-
-        // TODO: these are the extras:
-        // avatarId
-        // statusUpdatedAt
-        // quota
-        // quotaUsed
+        if (p.getStatusUpdatedAt() != null)
+        {
+            throw new InvalidArgumentException("Unsupported field: statusUpdatedAt");
+        }
+        if (p.getAvatarId() != null)
+        {
+            throw new InvalidArgumentException("Unsupported field: avatarId");
+        }
+        if (p.getQuota() != null)
+        {
+            throw new InvalidArgumentException("Unsupported field: quota");
+        }
+        if (p.getQuotaUsed() != null)
+        {
+            throw new InvalidArgumentException("Unsupported field: quotaUsed");
+        }
 
         List<Person> result = new ArrayList<>(1);
-        Person p = persons.get(0);
         PersonUpdate person = new PersonUpdate.Builder()
                 .id(p.getUserName())
                 .firstName(p.getFirstName())

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

@@ -33,6 +33,7 @@ import org.alfresco.rest.api.tests.client.RequestContext;
 import org.alfresco.rest.api.tests.client.data.Company;
 import org.alfresco.rest.api.tests.client.data.JSONAble;
 import org.alfresco.rest.api.tests.client.data.Person;
+import org.alfresco.util.GUID;
 import org.apache.commons.httpclient.HttpStatus;
 import org.json.simple.JSONObject;
 import org.junit.Before;
@@ -46,25 +47,33 @@ import static org.junit.Assert.fail;
 public class TestPeople extends EnterpriseTestApi
 {
 	private People people;
+	private Iterator<TestNetwork> accountsIt;
+	private TestNetwork account1;
+	private TestNetwork account2;
+	private Iterator<String> account1PersonIt;
+	private Iterator<String> account2PersonIt;
+	private String account1Admin;
+	private String account2Admin;
 
 	@Before
 	public void setUp() throws Exception
 	{
 		people = publicApiClient.people();
+		accountsIt = getTestFixture().getNetworksIt();
+		account1 = accountsIt.next();
+		account2 = accountsIt.next();
+		account1Admin = "admin@"+account1.getId();
+		account2Admin = "admin@"+account2.getId();
+		account1PersonIt = account1.getPersonIds().iterator();
+		account2PersonIt = account2.getPersonIds().iterator();
 	}
 
 	@Test
 	public void testPeople() throws Exception
 	{
-		Iterator<TestNetwork> accountsIt = getTestFixture().getNetworksIt();
-		final TestNetwork account1 = accountsIt.next();
-		Iterator<String> personIt1 = account1.getPersonIds().iterator();
-		final String person1 = personIt1.next();
-    	final String person2 = personIt1.next();
-
-		final TestNetwork account2 = accountsIt.next();
-		Iterator<String> personIt2 = account2.getPersonIds().iterator();
-    	final String person3 = personIt2.next();
+		final String person1 = account1PersonIt.next();
+    	final String person2 = account1PersonIt.next();
+		final String person3 = account2PersonIt.next();
 
     	// Test Case cloud-2192
     	// should be able to see oneself
@@ -116,13 +125,10 @@ public class TestPeople extends EnterpriseTestApi
 	@Test
 	public void testCreatePerson() throws Exception
 	{
-		Iterator<TestNetwork> accountsIt = getTestFixture().getNetworksIt();
-		final TestNetwork account1 = accountsIt.next();
-		final String networkAdmin = "admin@"+account1.getId();
-		publicApiClient.setRequestContext(new RequestContext(account1.getId(), networkAdmin, "admin"));
+		publicApiClient.setRequestContext(new RequestContext(account1.getId(), account1Admin, "admin"));
 
 		PersonUpdate person = new PersonUpdate.Builder().
-				id("myUserName@"+account1.getId()).
+				id("myUserName00@"+account1.getId()).
 				firstName("Firstname").
 				lastName("Lastname").
 				description("my description").
@@ -140,10 +146,9 @@ public class TestPeople extends EnterpriseTestApi
 				emailNotificationsEnabled(true).
 			build();
 
-		// true -> use extra fields such as company
-		Person p = people.create(person, true);
+		Person p = people.create(person);
 
-		assertEquals("myUserName@"+account1.getId(), p.getId());
+		assertEquals("myUserName00@"+account1.getId(), p.getId());
 		assertEquals("Firstname", p.getFirstName());
 		assertEquals("Lastname", p.getLastName());
 
@@ -181,13 +186,12 @@ public class TestPeople extends EnterpriseTestApi
 	@Test
 	public void testCreatePerson_notAllFieldsRequired() throws Exception
 	{
-		Iterator<TestNetwork> accountsIt = getTestFixture().getNetworksIt();
-		final TestNetwork account1 = accountsIt.next();
-		final String networkAdmin = "admin@"+account1.getId();
-		publicApiClient.setRequestContext(new RequestContext(account1.getId(), networkAdmin, "admin"));
+		publicApiClient.setRequestContext(new RequestContext(account1.getId(), account1Admin, "admin"));
 
+		// +ve: a random subset of fields should succeed.
+		{
 			PersonUpdate person = new PersonUpdate.Builder().
-				id("joe.bloggs@"+account1.getId()).
+					id("joe.bloggs@" + account1.getId()).
 					firstName("Joe").
 					lastName("Bloggs").
 					email("joe.bloggs@example.com").
@@ -197,10 +201,9 @@ public class TestPeople extends EnterpriseTestApi
 					emailNotificationsEnabled(false).
 					build();
 
-		// true -> use extra fields such as company
-		Person p = people.create(person, true);
+			Person p = people.create(person);
 
-		assertEquals("joe.bloggs@"+account1.getId(), p.getId());
+			assertEquals("joe.bloggs@" + account1.getId(), p.getId());
 			assertEquals("Joe", p.getFirstName());
 			assertEquals("Bloggs", p.getLastName());
 			assertEquals(null, p.getDescription());
@@ -218,6 +221,108 @@ public class TestPeople extends EnterpriseTestApi
 			assertEquals(false, p.isEmailNotificationsEnabled());
 		}
 
+		// +ve: absolute minimum
+		{
+			PersonUpdate person = new PersonUpdate.Builder().
+					id("joe.bloggs.2@" + account1.getId()).
+					build();
+
+			Person p = people.create(person);
+
+			assertEquals("joe.bloggs.2@" + account1.getId(), p.getId());
+			assertEquals(null, p.getFirstName());
+			assertEquals(null, p.getLastName());
+			assertEquals(null, p.getDescription());
+			assertEquals(null, p.getEmail());
+			assertEquals(null, p.getSkypeId());
+			assertEquals(null, p.getGoogleId());
+			assertEquals(null, p.getInstantMessageId());
+			assertEquals(null, p.getJobTitle());
+			assertEquals(null, p.getLocation());
+			assertEquals(null, p.getCompany());
+			assertEquals(null, p.getMobile());
+			assertEquals(null, p.getTelephone());
+			assertEquals(null, p.getUserStatus());
+			assertEquals(true, p.isEnabled());
+			assertEquals(true, p.isEmailNotificationsEnabled());
+		}
+
+		// -ve: not enough fields!
+		{
+			// Create a person with no fields set.
+			PersonUpdate person = new PersonUpdate.Builder().build();
+			people.create(person, 400);
+		}
+	}
+
+	@Test
+	public void testCreatePerson_extraFieldsCauseError() throws Exception {
+		publicApiClient.setRequestContext(new RequestContext(account1.getId(), account1Admin, "admin"));
+
+		String username = "joe.bloggs@"+account1.getId();
+
+		String[] illegalFields = new String[] {
+				"\"avatarId\": \"workspace://SpacesStore/\"",
+				"\"statusUpdatedAt\": \"2016-10-25T09:12:58.621Z\"",
+				"\"quota\": \"123\"",
+				"\"quotaUsed\": \"80\""
+		};
+
+		for (String badField : illegalFields)
+		{
+			String json =
+					"{\n" +
+					"  \"id\": \"" + username + "\",\n" +
+					"  \"firstName\": \"Joe\",\n" +
+					"  \"lastName\": \"Bloggs\",\n" +
+					badField +
+					"}";
+			people.create("people", null, null, null, json, "Illegal field test:"+badField, 400);
+		}
+	}
+
+	/**
+	 * General error conditions not covered by other "create person" tests.
+	 */
+	@Test
+	public void testCreatePerson_errorResponses() throws Exception {
+		// -ve: authorisation required
+		{
+			// Invalid auth details
+			publicApiClient.setRequestContext(new RequestContext(account1.getId(), GUID.generate(), "password"));
+			PersonUpdate person = new PersonUpdate.Builder().
+					id("myUserName01@"+account1.getId()).
+					build();
+			people.create(person, 401);
+		}
+
+		// -ve: API user does not have permission to create user.
+		{
+			String apiUser = account2PersonIt.next();
+			publicApiClient.setRequestContext(new RequestContext(account2.getId(), apiUser));
+			PersonUpdate person = new PersonUpdate.Builder().
+					id("myUserName02@"+account2.getId()).
+					build();
+			people.create(person, 403);
+
+			publicApiClient.setRequestContext(new RequestContext(account2.getId(), account2Admin, "admin"));
+			// Should succeed this time.
+			people.create(person, 201);
+		}
+
+		// -ve: person already exists
+		{
+			publicApiClient.setRequestContext(new RequestContext(account1.getId(), account1Admin, "admin"));
+			PersonUpdate person = new PersonUpdate.Builder().
+					id("myUserName03@"+account1.getId()).
+					build();
+			people.create(person);
+
+			// Attempt to create the person a second time.
+			people.create(person, 409);
+		}
+	}
+
 	public static class PersonUpdateJSONSerializer implements JSONAble
 	{
 		private final PersonUpdate personUpdate;
@@ -229,11 +334,6 @@ public class TestPeople extends EnterpriseTestApi
 
 		@Override
 		public JSONObject toJSON()
-		{
-			return toJSON(true);
-		}
-
-		public JSONObject toJSON(boolean fullVisibility)
 		{
 			JSONObject personJson = new JSONObject();
 
@@ -241,8 +341,6 @@ public class TestPeople extends EnterpriseTestApi
 			personJson.put("firstName", personUpdate.getFirstName());
 			personJson.put("lastName", personUpdate.getLastName());
 
-			if (fullVisibility)
-			{
 			personJson.put("description", personUpdate.getDescription());
 			personJson.put("email", personUpdate.getEmail());
 			personJson.put("skypeId", personUpdate.getSkypeId());
@@ -261,7 +359,7 @@ public class TestPeople extends EnterpriseTestApi
 			personJson.put("userStatus", personUpdate.getUserStatus());
 			personJson.put("enabled", personUpdate.isEnabled());
 			personJson.put("emailNotificationsEnabled", personUpdate.isEmailNotificationsEnabled());
-			}
+
 			return personJson;
 		}
 	}

source/test-java/org/alfresco/rest/api/tests/client/PublicApiClient.java

@@ -1072,12 +1072,25 @@ public class PublicApiClient
 			return retSite;
 		}
 
-		public Person create(PersonUpdate person, boolean fullVisibility) throws PublicApiException
+		public Person create(PersonUpdate person) throws PublicApiException
+		{
+			return create(person, 201);
+		}
+
+		public Person create(PersonUpdate person, int expectedStatus) throws PublicApiException
 		{
 			TestPeople.PersonUpdateJSONSerializer jsonizer = new TestPeople.PersonUpdateJSONSerializer(person) ;
-			HttpResponse response = create("people", null, null, null, jsonizer.toJSON(fullVisibility).toString(), "Failed to create person");
-			Person retSite = Person.parsePerson((JSONObject)response.getJsonResponse().get("entry"));
-			return retSite;
+			HttpResponse response = create("people", null, null, null, jsonizer.toJSON().toString(), "Failed to create person", expectedStatus);
+			if ((response != null) && (response.getJsonResponse() != null))
+			{
+				Object entry = response.getJsonResponse().get("entry");
+				// entry will be null if there is an "error" data structure returned instead.
+				if (entry != null)
+				{
+					return Person.parsePerson((JSONObject) entry);
+				}
+			}
+			return null;
 		}
 
 		public void remove(Person person) throws PublicApiException