Sort memberships by created time then username

Fixes #9139

h/services/group_members.py

@@ -1,7 +1,7 @@
 import logging
 from functools import partial
 
-from sqlalchemy import func, or_, select
+from sqlalchemy import func, nulls_first, or_, select
 
 from h import session
 from h.models import Group, GroupMembership, GroupMembershipRoles, User
@@ -53,7 +53,7 @@ def get_memberships(
             select(GroupMembership)
             .join(User)
             .where(GroupMembership.group == group)
-            .order_by(User.username)
+            .order_by(nulls_first(GroupMembership.created), User.username)
         )
 
         if roles:

h/views/api/group_members.py

@@ -38,8 +38,8 @@ def list_members_legacy(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.
+    # memberships explictly sorted by creation date then username whereas
+    # `context.memberships` is unsorted.
     group_members_service = request.find_service(name="group_members")
     memberships = group_members_service.get_memberships(context.group)
 

tests/unit/h/services/group_members_test.py

@@ -1,4 +1,6 @@
 import logging
+from datetime import datetime
+from operator import attrgetter
 from unittest import mock
 
 import pytest
@@ -39,9 +41,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)) == sorted(
-            memberships, key=lambda membership: membership.user.username
-        )
+        assert sorted(
+            group_members_service.get_memberships(group), key=attrgetter("id")
+        ) == sorted(memberships, key=attrgetter("id"))
 
     def test_roles(self, group_members_service, db_session, factories):
         group = factories.Group.build()
@@ -60,11 +62,12 @@ def test_roles(self, group_members_service, db_session, factories):
             ]
         )
 
-        assert list(
+        assert sorted(
             group_members_service.get_memberships(
                 group, roles=[GroupMembershipRoles.ADMIN]
-            )
-        ) == sorted(memberships, key=lambda membership: membership.user.username)
+            ),
+            key=attrgetter("id"),
+        ) == sorted(memberships, key=attrgetter("id"))
 
     def test_multiple_roles(self, group_members_service, db_session, factories):
         group = factories.Group.build()
@@ -88,12 +91,13 @@ def test_multiple_roles(self, group_members_service, db_session, factories):
             ]
         )
 
-        assert list(
+        assert sorted(
             group_members_service.get_memberships(
                 group,
                 roles=[GroupMembershipRoles.ADMIN, GroupMembershipRoles.MODERATOR],
-            )
-        ) == sorted(memberships, key=lambda membership: membership.user.username)
+            ),
+            key=attrgetter("id"),
+        ) == sorted(memberships, key=attrgetter("id"))
 
     def test_offset_and_limit(self, group_members_service, db_session, factories):
         group = factories.Group()
@@ -106,10 +110,59 @@ def test_offset_and_limit(self, group_members_service, db_session, factories):
         )
 
         assert (
-            list(returned_memberships)
-            == sorted(memberships, key=lambda membership: membership.user.username)[1:3]
+            sorted(returned_memberships, key=attrgetter("id"))
+            == sorted(memberships, key=attrgetter("id"))[1:3]
         )
 
+    def test_order(self, group_members_service, db_session, factories):
+        group = factories.Group.build()
+        users = [
+            factories.User.build(username="user_0"),
+            factories.User.build(username="user_1"),
+            factories.User.build(username="user_2"),
+            factories.User.build(username="user_3"),
+        ]
+
+        def membership_with_no_created_time(**kwargs):
+            # The `default` in GroupMembership makes it hard to create a
+            # membership with created=None: it gets overridden with the current
+            # time when the DB session is flushed. So we have to do this...
+            membership = GroupMembership(**kwargs)
+            db_session.add(membership)
+            db_session.flush()
+            membership.created = None
+            return membership
+
+        memberships = [
+            GroupMembership(
+                group=group,
+                user=users[0],
+                created=datetime(1970, 1, 1, 0, 0, 1),
+            ),
+            GroupMembership(
+                group=group,
+                user=users[1],
+                created=datetime(1970, 1, 1, 0, 0, 0),
+            ),
+            membership_with_no_created_time(group=group, user=users[3]),
+            membership_with_no_created_time(group=group, user=users[2]),
+        ]
+        db_session.add_all(memberships)
+
+        returned_memberships = group_members_service.get_memberships(group)
+
+        assert [
+            (membership.created, membership.user.username)
+            for membership in returned_memberships
+        ] == [
+            # NULLs are first, ordered by username.
+            (None, "user_2"),
+            (None, "user_3"),
+            # Non-NULLs are ordered by created time, oldest first.
+            (datetime(1970, 1, 1, 0, 0, 0), "user_1"),
+            (datetime(1970, 1, 1, 0, 0, 1), "user_0"),
+        ]
+
 
 class TestCountMemberships:
     def test_it(self, group_members_service, factories):