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,5 @@
 import logging
+from datetime import datetime
 from unittest import mock
 
 import pytest
@@ -34,14 +35,19 @@ class TestGetMemberships:
     def test_it(self, group_members_service, db_session, factories):
         group, other_group = factories.Group.build_batch(size=2)
         users = factories.User.build_batch(size=2)
-        memberships = [GroupMembership(group=group, user=user) for user in users]
+        memberships = [
+            GroupMembership(
+                group=group,
+                user=user,
+                created=datetime(1970, 1, 1, 0, 0, second),
+            )
+            for second, user in enumerate(users)
+        ]
         db_session.add_all(
             [*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 list(group_members_service.get_memberships(group)) == memberships
 
     def test_roles(self, group_members_service, db_session, factories):
         group = factories.Group.build()
@@ -73,10 +79,16 @@ def test_multiple_roles(self, group_members_service, db_session, factories):
         member = factories.User.build()
         memberships = [
             GroupMembership(
-                group=group, user=admin, roles=[GroupMembershipRoles.ADMIN]
+                group=group,
+                user=admin,
+                roles=[GroupMembershipRoles.ADMIN],
+                created=datetime(1970, 1, 1, 0, 0, 0),
             ),
             GroupMembership(
-                group=group, user=moderator, roles=[GroupMembershipRoles.MODERATOR]
+                group=group,
+                user=moderator,
+                roles=[GroupMembershipRoles.MODERATOR],
+                created=datetime(1970, 1, 1, 0, 0, 1),
             ),
         ]
         db_session.add_all(
@@ -88,27 +100,83 @@ 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],
+                )
             )
-        ) == sorted(memberships, key=lambda membership: membership.user.username)
+            == memberships
+        )
 
     def test_offset_and_limit(self, group_members_service, db_session, factories):
         group = factories.Group()
         users = factories.User.build_batch(size=4)
-        memberships = [GroupMembership(group=group, user=user) for user in users]
+        memberships = [
+            GroupMembership(
+                group=group,
+                user=user,
+                created=datetime(1970, 1, 1, 0, 0, second),
+            )
+            for second, user in enumerate(users)
+        ]
         db_session.add_all(memberships)
 
         returned_memberships = group_members_service.get_memberships(
             group, offset=1, limit=2
         )
 
-        assert (
-            list(returned_memberships)
-            == sorted(memberships, key=lambda membership: membership.user.username)[1:3]
-        )
+        assert list(returned_memberships) == memberships[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: