ACE-4823: Fixed "list of a person's site memberships" REST API which only returned one of the two sites with identical titles.

- Also, fixed code standards.

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

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

@@ -22,6 +22,7 @@ import java.io.Serializable;
 import java.text.Collator;
 import java.util.AbstractList;
 import java.util.ArrayList;
+import java.util.Collections;
 import java.util.Comparator;
 import java.util.HashMap;
 import java.util.Iterator;
@@ -447,19 +448,18 @@ public class SitesImpl implements Sites
                     SortOrder.ASCENDING ));
         }
 
-        final Set<SiteMembership> sortedSiteMembers = new TreeSet<SiteMembership>(
-                new SiteMembershipComparator(
-                        sortPairs, 
-                        SiteMembershipComparator.Type.SITES));
-        
         // get the unsorted list of site memberships
         List<SiteMembership> siteMembers = siteService.listSiteMemberships (personId, 0);
 
         // sort the list of site memberships
         int totalSize = siteMembers.size();
-        sortedSiteMembers.addAll(siteMembers);
+        final List<SiteMembership> sortedSiteMembers = new ArrayList<>(siteMembers);
+        Collections.sort(sortedSiteMembers, new SiteMembershipComparator(
+                    sortPairs,
+                    SiteMembershipComparator.Type.SITES));
+
         PageDetails pageDetails = PageDetails.getPageDetails(pagingRequest, totalSize);
-        List<MemberOfSite> ret = new ArrayList<MemberOfSite>(totalSize);
+        List<MemberOfSite> ret = new ArrayList<>(totalSize);
         
         Iterator<SiteMembership> it = sortedSiteMembers.iterator();
         for(int counter = 0; counter < pageDetails.getEnd() && it.hasNext(); counter++)

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

@@ -1,10 +1,31 @@
+/*
+ * Copyright (C) 2005-2015 Alfresco Software Limited.
+ *
+ * This file is part of Alfresco
+ *
+ * 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/>.
+ */
+
 package org.alfresco.rest.api.tests;
 
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
 
 import java.util.ArrayList;
+import java.util.Collections;
 import java.util.HashMap;
 import java.util.Iterator;
 import java.util.LinkedList;
@@ -41,7 +62,7 @@ public class TestPersonSites extends EnterpriseTestApi
     private TestPerson person12;
     private TestPerson person21;
 
-	private List<TestSite> sites = new ArrayList<TestSite>(10);
+    private List<TestSite> sites = new ArrayList<>(10);
 
     /* Sites and users used to test the site sorting */
     private TestPerson person31;
@@ -49,21 +70,19 @@ public class TestPersonSites extends EnterpriseTestApi
     private TestSite site1;
     private TestSite site2;
     private TestSite site3;
-        private static String site1_name = "a_" + GUID.generate();;
+    private String site1_name = "a_" + GUID.generate();
-        private static String site1_title = "c_" + GUID.generate();
+    private String site1_title = "c_" + GUID.generate();
-        private static SiteRole site1_role = SiteRole.SiteContributor;
+    private SiteRole site1_role = SiteRole.SiteContributor;
-        private static String site2_name = "b_" + GUID.generate();;
+    private String site2_name = "b_" + GUID.generate();
-        private static String site2_title = "a_" + GUID.generate();
+    private String site2_title = "a_" + GUID.generate();
-        private static SiteRole site2_role = SiteRole.SiteManager;
+    private SiteRole site2_role = SiteRole.SiteManager;
-        private static String site3_name = "c_" + GUID.generate();;
+    private String site3_name = "c_" + GUID.generate();
-        private static String site3_title = "b_" + GUID.generate();
+    private String site3_title = "b_" + GUID.generate();
-        private static SiteRole site3_role = SiteRole.SiteConsumer;
+    private SiteRole site3_role = SiteRole.SiteConsumer;
 
 
     public void initializeSites() throws Exception
     {
-            
-            
         /*
          * Create data for testing the site sorting. We create the sites as
          * person31 and assign roles to person32. The list requests will be
@@ -204,8 +223,8 @@ public class TestPersonSites extends EnterpriseTestApi
         Set<MemberOfSite> personSites = new TreeSet<MemberOfSite>();
 
         //Get last site for use with personRemoveSite
-		TestSite personRemoveSite = sites.get(sites.size()-1);
+        TestSite personRemoveSite = sites.get(sites.size() - 1);
-		sites.remove(sites.size()-1);
+        sites.remove(sites.size() - 1);
 
         personSites.addAll(network1.getSiteMemberships(person11.getId()));
 
@@ -266,7 +285,7 @@ public class TestPersonSites extends EnterpriseTestApi
             sitesProxy.getPersonSites(GUID.generate(), null);
             fail("");
         }
-		catch(PublicApiException e)
+        catch (PublicApiException e)
         {
             assertEquals(HttpStatus.SC_NOT_FOUND, e.getHttpResponse().getStatusCode());
         }
@@ -312,7 +331,7 @@ public class TestPersonSites extends EnterpriseTestApi
             sitesProxy.getPersonSites(person11.getId(), createParams(paging, null));
             fail("");
         }
-		catch(PublicApiException e)
+        catch (PublicApiException e)
         {
             assertEquals(HttpStatus.SC_UNAUTHORIZED, e.getHttpResponse().getStatusCode());
         }
@@ -334,7 +353,7 @@ public class TestPersonSites extends EnterpriseTestApi
             sitesProxy.getPersonSite(GUID.generate(), memberOfSite.getSiteId());
             fail();
         }
-		catch(PublicApiException e)
+        catch (PublicApiException e)
         {
             assertEquals(HttpStatus.SC_NOT_FOUND, e.getHttpResponse().getStatusCode());
         }
@@ -345,7 +364,7 @@ public class TestPersonSites extends EnterpriseTestApi
             sitesProxy.getPersonSite(person11.getId(), GUID.generate());
             fail();
         }
-		catch(PublicApiException e)
+        catch (PublicApiException e)
         {
             assertEquals(HttpStatus.SC_NOT_FOUND, e.getHttpResponse().getStatusCode());
         }
@@ -360,7 +379,7 @@ public class TestPersonSites extends EnterpriseTestApi
             sitesProxy.getPersonSite(GUID.generate(), memberOfSite.getSiteId());
             fail();
         }
-		catch(PublicApiException e)
+        catch (PublicApiException e)
         {
             assertEquals(HttpStatus.SC_NOT_FOUND, e.getHttpResponse().getStatusCode());
         }
@@ -371,7 +390,7 @@ public class TestPersonSites extends EnterpriseTestApi
             sitesProxy.getPersonSite(person11.getId(), GUID.generate());
             fail();
         }
-		catch(PublicApiException e)
+        catch (PublicApiException e)
         {
             assertEquals(HttpStatus.SC_NOT_FOUND, e.getHttpResponse().getStatusCode());
         }
@@ -385,7 +404,7 @@ public class TestPersonSites extends EnterpriseTestApi
             {
                 sitesProxy.getPersonSite(person11.getId(), personRemoveSite.getSiteId());
             }
-			catch(PublicApiException e)
+            catch (PublicApiException e)
             {
                 assertEquals(HttpStatus.SC_NOT_FOUND, e.getHttpResponse().getStatusCode());
             }
@@ -404,7 +423,7 @@ public class TestPersonSites extends EnterpriseTestApi
             sitesProxy.create("people", person11.getId(), "sites", memberOfSite.getSiteId(), null, "Unable to POST to a person site");
             fail();
         }
-		catch(PublicApiException e)
+        catch (PublicApiException e)
         {
             assertEquals(HttpStatus.SC_METHOD_NOT_ALLOWED, e.getHttpResponse().getStatusCode());
         }
@@ -415,7 +434,7 @@ public class TestPersonSites extends EnterpriseTestApi
             sitesProxy.create("people", person11.getId(), "sites", null, null, "Unable to POST to person sites");
             fail();
         }
-		catch(PublicApiException e)
+        catch (PublicApiException e)
         {
             assertEquals(HttpStatus.SC_METHOD_NOT_ALLOWED, e.getHttpResponse().getStatusCode());
         }
@@ -428,7 +447,7 @@ public class TestPersonSites extends EnterpriseTestApi
             sitesProxy.update("people", person11.getId(), "sites", memberOfSite.getSiteId(), null, "Unable to PUT a person site");
             fail();
         }
-		catch(PublicApiException e)
+        catch (PublicApiException e)
         {
             assertEquals(HttpStatus.SC_METHOD_NOT_ALLOWED, e.getHttpResponse().getStatusCode());
         }
@@ -439,7 +458,7 @@ public class TestPersonSites extends EnterpriseTestApi
             sitesProxy.update("people", person11.getId(), "sites", null, null, "Unable to PUT person sites");
             fail();
         }
-		catch(PublicApiException e)
+        catch (PublicApiException e)
         {
             assertEquals(HttpStatus.SC_METHOD_NOT_ALLOWED, e.getHttpResponse().getStatusCode());
         }
@@ -450,7 +469,7 @@ public class TestPersonSites extends EnterpriseTestApi
             sitesProxy.remove("people", person11.getId(), "sites", null, "Unable to DELETE person sites");
             fail();
         }
-		catch(PublicApiException e)
+        catch (PublicApiException e)
         {
             assertEquals(HttpStatus.SC_METHOD_NOT_ALLOWED, e.getHttpResponse().getStatusCode());
         }
@@ -459,8 +478,6 @@ public class TestPersonSites extends EnterpriseTestApi
     /**
      * Retrieves the site memberships associated to a user
      *
-         * @param skipCount
-         * @param maxItems
      * @param sortColumn
      * @param asc
      * @return
@@ -473,9 +490,9 @@ public class TestPersonSites extends EnterpriseTestApi
 
         // sort params
         final Map<String, String> params = new HashMap<String, String>();
-            if(sortColumn != null)
+        if (sortColumn != null)
         {
-                params.put("orderBy", sortColumn + " " + (asc?"ASC":"DESC"));
+            params.put("orderBy", sortColumn + " " + (asc ? "ASC" : "DESC"));
         }
 
         // get memberships
@@ -511,11 +528,12 @@ public class TestPersonSites extends EnterpriseTestApi
         ListResponse<MemberOfSite> resp = getSiteMembershipsForPerson32(paging, "SiteTitle", true);
 
         // check results
-            List<MemberOfSite> expectedList = new LinkedList<MemberOfSite>();
+        List<MemberOfSite> expectedList = new LinkedList<>();
         expectedList.add(new MemberOfSite(site3, site3_role));
         expectedList.add(new MemberOfSite(site1, site1_role));
 
         checkList(expectedList, paging.getExpectedPaging(), resp);
+
     }
 
     /**
@@ -536,7 +554,7 @@ public class TestPersonSites extends EnterpriseTestApi
         ListResponse<MemberOfSite> resp = getSiteMembershipsForPerson32(paging, "SiteTitle", false);
 
         // check results
-            List<MemberOfSite> expectedList = new LinkedList<MemberOfSite>();
+        List<MemberOfSite> expectedList = new LinkedList<>();
         expectedList.add(new MemberOfSite(site3, site3_role));
         expectedList.add(new MemberOfSite(site2, site2_role));
 
@@ -561,7 +579,7 @@ public class TestPersonSites extends EnterpriseTestApi
         ListResponse<MemberOfSite> resp = getSiteMembershipsForPerson32(paging, "Role", true);
 
         // check results
-            List<MemberOfSite> expectedList = new LinkedList<MemberOfSite>();
+        List<MemberOfSite> expectedList = new LinkedList<>();
         expectedList.add(new MemberOfSite(site1, site1_role));
         expectedList.add(new MemberOfSite(site2, site2_role));
 
@@ -586,7 +604,7 @@ public class TestPersonSites extends EnterpriseTestApi
         ListResponse<MemberOfSite> resp = getSiteMembershipsForPerson32(paging, "Role", false);
 
         // check results
-            List<MemberOfSite> expectedList = new LinkedList<MemberOfSite>();
+        List<MemberOfSite> expectedList = new LinkedList<>();
         expectedList.add(new MemberOfSite(site1, site1_role));
         expectedList.add(new MemberOfSite(site3, site3_role));
 
@@ -611,7 +629,7 @@ public class TestPersonSites extends EnterpriseTestApi
         ListResponse<MemberOfSite> resp = getSiteMembershipsForPerson32(paging, "SiteShortName", true);
 
         // check results
-            List<MemberOfSite> expectedList = new LinkedList<MemberOfSite>();
+        List<MemberOfSite> expectedList = new LinkedList<>();
         expectedList.add(new MemberOfSite(site2, site2_role));
         expectedList.add(new MemberOfSite(site3, site3_role));
 
@@ -636,7 +654,7 @@ public class TestPersonSites extends EnterpriseTestApi
         ListResponse<MemberOfSite> resp = getSiteMembershipsForPerson32(paging, "SiteShortName", false);
 
         // check results
-            List<MemberOfSite> expectedList = new LinkedList<MemberOfSite>();
+        List<MemberOfSite> expectedList = new LinkedList<>();
         expectedList.add(new MemberOfSite(site2, site2_role));
         expectedList.add(new MemberOfSite(site1, site1_role));
 
@@ -659,12 +677,13 @@ public class TestPersonSites extends EnterpriseTestApi
         ListResponse<MemberOfSite> resp = getSiteMembershipsForPerson32(null, null, false);
 
         // check results
-            List<MemberOfSite> expectedList = new LinkedList<MemberOfSite>();
+        List<MemberOfSite> expectedList = new LinkedList<>();
         expectedList.add(new MemberOfSite(site2, site2_role));
         expectedList.add(new MemberOfSite(site3, site3_role));
         expectedList.add(new MemberOfSite(site1, site1_role));
 
         checkList(expectedList, paging.getExpectedPaging(), resp);
+
     }
 
     @Test
@@ -680,4 +699,56 @@ public class TestPersonSites extends EnterpriseTestApi
         testSortingAndPagingBySiteNameDesc();
         testSortingAndPagingDefault();
     }
+
+    // ACE-4823
+    @Test
+    public void testSitesWithSameTitles() throws Exception
+    {
+        // Creates 3 sites
+        initializeSites();
+
+        final String site4_name = "d_" + GUID.generate();
+        final String site4_title = site3_title; // Same title as site3
+        final SiteRole site4_role = SiteRole.SiteCollaborator;
+
+        TestSite site4 = TenantUtil.runAsUserTenant(new TenantRunAsWork<TestSite>()
+        {
+            @Override
+            public TestSite doWork() throws Exception
+            {
+                SiteInformation siteInfo = new SiteInformation(site4_name, site4_title, site4_title, SiteVisibility.PRIVATE);
+                TestSite site = network1.createSite(siteInfo);
+                site.inviteToSite(person32.getId(), site4_role);
+                return site;
+            }
+        }, person31.getId(), network1.getId());
+        assertNotNull(site4);
+
+        // paging
+        int totalResults = 4;
+        Paging paging = getPaging(null, null, totalResults, null);
+
+        // get memberships
+        ListResponse<MemberOfSite> resp = getSiteMembershipsForPerson32(null, null, false);
+
+        // check results
+        List<MemberOfSite> expectedList = new LinkedList<>();
+        expectedList.add(new MemberOfSite(site2, site2_role));
+        expectedList.add(new MemberOfSite(site3, site3_role));
+        expectedList.add(new MemberOfSite(site4, site4_role));
+        expectedList.add(new MemberOfSite(site1, site1_role));
+
+        try
+        {
+            checkList(expectedList, paging.getExpectedPaging(), resp);
+        }
+        catch (AssertionError error)
+        {
+            // Site3 and Site4 have a same title, and as we are sorting on titles (default sorting),
+            // we can't guarantee the order in which the sites will
+            // return, hence swap the sites and compare again.
+            Collections.swap(expectedList, 1, 2);
+            checkList(expectedList, paging.getExpectedPaging(), resp);
+        }
+    }
 }