Use explicit sort order in get-group-members API

Currently the get-group-members API just returns the list of memberships
in whatever order the SQLAlchemy relationship `Group.memberships`
returns them, which is undefined.

Ultimately we want this API to return memberships sorted by the creation
time of the membership itself, oldest first. But we don't currently have
a `created` column on the `user_group` table.

So for now sort memberships by username instead.

I couldn't figure out how to set the sort order of the
`Group.memberships` relationship itself to `User.username`: however I
try to use the `order_by` argument to this relationship I'm running into
various SQLAlchemy errors and I can't find an example in the docs or on
Google. GitHub Copilot has no idea ;)

So change the view to call `GroupMembersService.get_memberships()`
instead of just reading `context.group.memberships`, and add the sort
ordering to the query that `GroupMembersService.get_memberships()` does.

h/services/group_members.py

@@ -4,7 +4,7 @@
 from sqlalchemy import or_, select
 
 from h import session
-from h.models import Group, GroupMembership, GroupMembershipRoles
+from h.models import Group, GroupMembership, GroupMembershipRoles, User
 
 log = logging.getLogger(__name__)
 
@@ -45,7 +45,12 @@ def get_memberships(
         If multiple roles are given return all memberships matching *any* of
         the given roles.
         """
-        query = select(GroupMembership).where(GroupMembership.group == group)
+        query = (
+            select(GroupMembership)
+            .join(User)
+            .where(GroupMembership.group == group)
+            .order_by(User.username)
+        )
 
         if roles:
             query = query.where(

h/views/api/group_members.py

@@ -21,9 +21,17 @@
     permission=Permission.Group.READ,
 )
 def list_members(context: GroupContext, request):
+
+    # Get the list of memberships from GroupMembersService instead of just
+    # accessing `context.memberships` because GroupMembersService returns the
+    # memberships explictly sorted by username whereas `context.memberships` is
+    # unsorted.
+    group_members_service = request.find_service(name="group_members")
+    memberships = group_members_service.get_memberships(context.group)
+
     return [
         GroupMembershipJSONPresenter(request, membership).asdict()
-        for membership in context.group.memberships
+        for membership in memberships
     ]
 
 

tests/functional/api/groups/members_test.py

@@ -31,7 +31,9 @@ def test_it_returns_list_of_members_for_restricted_group_without_authn(
                 "roles": membership.roles,
                 "actions": [],
             }
-            for membership in group.memberships
+            for membership in sorted(
+                group.memberships, key=lambda membership: membership.user.username
+            )
         ]
 
     def test_it_returns_list_of_members_if_user_has_access_to_private_group(

tests/unit/h/services/group_members_test.py

@@ -39,7 +39,9 @@ def test_it(self, group_members_service, db_session, factories):
             [*memberships, GroupMembership(group=other_group, user=users[0])]
         )
 
-        assert list(group_members_service.get_memberships(group)) == memberships
+        assert list(group_members_service.get_memberships(group)) == sorted(
+            memberships, key=lambda membership: membership.user.username
+        )
 
     def test_roles(self, group_members_service, db_session, factories):
         group = factories.Group.build()
@@ -58,14 +60,11 @@ def test_roles(self, group_members_service, db_session, factories):
             ]
         )
 
-        assert (
-            list(
-                group_members_service.get_memberships(
-                    group, roles=[GroupMembershipRoles.ADMIN]
-                )
+        assert list(
+            group_members_service.get_memberships(
+                group, roles=[GroupMembershipRoles.ADMIN]
             )
-            == memberships
-        )
+        ) == sorted(memberships, key=lambda membership: membership.user.username)
 
     def test_multiple_roles(self, group_members_service, db_session, factories):
         group = factories.Group.build()
@@ -89,15 +88,12 @@ def test_multiple_roles(self, group_members_service, db_session, factories):
             ]
         )
 
-        assert (
-            list(
-                group_members_service.get_memberships(
-                    group,
-                    roles=[GroupMembershipRoles.ADMIN, GroupMembershipRoles.MODERATOR],
-                )
+        assert list(
+            group_members_service.get_memberships(
+                group,
+                roles=[GroupMembershipRoles.ADMIN, GroupMembershipRoles.MODERATOR],
             )
-            == memberships
-        )
+        ) == sorted(memberships, key=lambda membership: membership.user.username)
 
 
 class TestMemberJoin:

tests/unit/h/views/api/group_members_test.py

@@ -14,8 +14,18 @@
 
 
 class TestListMembers:
-    def test_it(self, context, pyramid_request, GroupMembershipJSONPresenter):
-        context.group.memberships = [sentinel.membership_1, sentinel.membership_2]
+    def test_it(
+        self,
+        context,
+        pyramid_request,
+        GroupMembershipJSONPresenter,
+        group_members_service,
+    ):
+        group_members_service.get_memberships.return_value = [
+            sentinel.membership_1,
+            sentinel.membership_2,
+        ]
+
         presenter_instances = GroupMembershipJSONPresenter.side_effect = [
             create_autospec(
                 presenters.GroupMembershipJSONPresenter, instance=True, spec_set=True
@@ -27,6 +37,7 @@ def test_it(self, context, pyramid_request, GroupMembershipJSONPresenter):
 
         response = views.list_members(context, pyramid_request)
 
+        group_members_service.get_memberships.assert_called_once_with(context.group)
         assert GroupMembershipJSONPresenter.call_args_list == [
             call(pyramid_request, sentinel.membership_1),
             call(pyramid_request, sentinel.membership_2),