launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/team-members-ordering into lp:launchpad.

Commit message:
Sort team members collections so that webservice batching works more reliably.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1839132 in Launchpad itself: "The API's people[groupname].members method can return duplicates"
  https://bugs.launchpad.net/launchpad/+bug/1839132

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/team-members-ordering/+merge/371035
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/team-members-ordering into lp:launchpad.

=== modified file 'lib/lp/registry/browser/tests/test_person_webservice.py'
--- lib/lp/registry/browser/tests/test_person_webservice.py	2019-05-22 14:57:45 +0000
+++ lib/lp/registry/browser/tests/test_person_webservice.py	2019-08-07 08:38:58 +0000
@@ -1,10 +1,12 @@
-# Copyright 2009-2018 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2019 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
 
+from operator import attrgetter
 import textwrap
 
+from lazr.uri import URI
 from storm.store import Store
 from zope.component import getUtility
 from zope.security.management import endInteraction
@@ -189,6 +191,37 @@
             get_members, create_member, 2)
         self.assertThat(recorder2, HasQueryCount.byEquality(recorder1))
 
+    def test_members_ordering(self):
+        # Entries in the various members collections are sorted by
+        # (Person.display_name, Person.name).
+        with admin_logged_in():
+            team = self.factory.makeTeam()
+            owner = team.teamowner
+            name = team.name
+        members = [owner]
+        with admin_logged_in():
+            for member_suffix in (
+                    'a1', 'b1', 'a2', 'b2', 'a3', 'b4', 'a4', 'b3',
+                    'a5', 'b5'):
+                person = self.factory.makePerson(
+                    name='member-' + member_suffix)
+                team.addMember(person, owner)
+                members.append(person)
+        expected_member_names = [
+            member.name for member in sorted(
+                members, key=attrgetter('display_name', 'name'))]
+        ws = webservice_for_person(owner)
+        observed_member_names = []
+        batch = ws.get('/~%s/members' % name).jsonBody()
+        while True:
+            for entry in batch['entries']:
+                observed_member_names.append(entry['name'])
+            next_link = batch.get('next_collection_link')
+            if next_link is None:
+                break
+            batch = ws.get(URI(next_link)).jsonBody()
+        self.assertEqual(expected_member_names, observed_member_names)
+
     def test_many_ppas(self):
         # POSTing to a person with many PPAs doesn't OOPS.
         with admin_logged_in():

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2018-11-13 03:48:13 +0000
+++ lib/lp/registry/model/person.py	2019-08-07 08:38:58 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2018 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2019 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Implementation classes for a Person."""
@@ -4040,6 +4040,12 @@
         # Adapt the result into a cached Person.
         columns = tuple(columns)
         raw_result = store.using(*origin).find(columns, conditions)
+        if need_api:
+            # Collections exported on the API need to be sorted in order to
+            # provide stable batching.  In some ways Person.name might make
+            # more sense, but we use the same ordering as in web UI views to
+            # minimise confusion.
+            raw_result = raw_result.order_by(Person.sortingColumns)
 
         def preload_for_people(rows):
             if need_teamowner or need_api: