diff --git a/h/services/group_members.py b/h/services/group_members.py index 754d6875261..f9f0a2f0278 100644 --- a/h/services/group_members.py +++ b/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: diff --git a/h/views/api/group_members.py b/h/views/api/group_members.py index 6b1e26a490c..e04b0059871 100644 --- a/h/views/api/group_members.py +++ b/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) diff --git a/tests/functional/api/groups/members_test.py b/tests/functional/api/groups/members_test.py index 9ab30642a73..39a10306917 100644 --- a/tests/functional/api/groups/members_test.py +++ b/tests/functional/api/groups/members_test.py @@ -17,10 +17,10 @@ def test_it_returns_list_of_members_for_restricted_group_without_authn( memberships=[ GroupMembership( user=user, - created=datetime(1970, 1, 1, 0, 0, 0), - updated=datetime(1970, 1, 1, 0, 0, 1), + created=datetime(1970, 1, 1, 0, 0, second), + updated=datetime(1970, 1, 2, 0, 0, second), ) - for user in factories.User.create_batch(size=3) + for second, user in enumerate(factories.User.create_batch(size=3)) ] ) db_session.commit() @@ -42,12 +42,10 @@ def test_it_returns_list_of_members_for_restricted_group_without_authn( "display_name": membership.user.display_name, "roles": membership.roles, "actions": [], - "created": "1970-01-01T00:00:00.000000+00:00", - "updated": "1970-01-01T00:00:01.000000+00:00", + "created": f"1970-01-01T00:00:{second:02}.000000+00:00", + "updated": f"1970-01-02T00:00:{second:02}.000000+00:00", } - for membership in sorted( - group.memberships, key=lambda membership: membership.user.username - ) + for second, membership in enumerate(group.memberships) ] def test_it_returns_list_of_members_if_user_has_access_to_private_group( @@ -65,8 +63,8 @@ def test_it_returns_list_of_members_if_user_has_access_to_private_group( ), GroupMembership( user=other_user, - created=datetime(1971, 1, 1, 0, 0, 0), - updated=datetime(1971, 1, 1, 0, 0, 1), + created=datetime(1971, 1, 2, 0, 0, 0), + updated=datetime(1971, 1, 2, 0, 0, 1), ), ] ) @@ -97,8 +95,8 @@ def test_it_returns_list_of_members_if_user_has_access_to_private_group( "display_name": other_user.display_name, "roles": [GroupMembershipRoles.MEMBER], "actions": [], - "created": "1971-01-01T00:00:00.000000+00:00", - "updated": "1971-01-01T00:00:01.000000+00:00", + "created": "1971-01-02T00:00:00.000000+00:00", + "updated": "1971-01-02T00:00:01.000000+00:00", }, ], key=lambda membership: membership["username"], @@ -132,10 +130,10 @@ def test_it_returns_list_of_members_for_restricted_group_without_auth( memberships=[ GroupMembership( user=user, - created=datetime(1970, 1, 1, 0, 0, 0), - updated=datetime(1970, 1, 1, 0, 0, 1), + created=datetime(1970, 1, 1, 0, 0, second), + updated=datetime(1970, 1, 2, 0, 0, second), ) - for user in factories.User.create_batch(size=4) + for second, user in enumerate(factories.User.create_batch(size=4)) ] ) db_session.commit() @@ -157,12 +155,10 @@ def test_it_returns_list_of_members_for_restricted_group_without_auth( "display_name": membership.user.display_name, "roles": membership.roles, "actions": [], - "created": "1970-01-01T00:00:00.000000+00:00", - "updated": "1970-01-01T00:00:01.000000+00:00", + "created": f"1970-01-01T00:00:{second:02}.000000+00:00", + "updated": f"1970-01-02T00:00:{second:02}.000000+00:00", } - for membership in sorted( - group.memberships, key=lambda membership: membership.user.username - )[1:3] + for second, membership in list(enumerate(group.memberships))[1:3] ], } @@ -181,8 +177,8 @@ def test_it_returns_list_of_members_if_user_has_access_to_private_group( ), GroupMembership( user=other_user, - created=datetime(1971, 1, 1, 0, 0, 0), - updated=datetime(1971, 1, 1, 0, 0, 1), + created=datetime(1971, 1, 2, 0, 0, 0), + updated=datetime(1971, 1, 2, 0, 0, 1), ), ] ) @@ -197,31 +193,28 @@ def test_it_returns_list_of_members_if_user_has_access_to_private_group( assert res.status_code == 200 assert res.json == { "meta": {"page": {"total": 2}}, - "data": sorted( - [ - { - "authority": group.authority, - "userid": user.userid, - "username": user.username, - "display_name": user.display_name, - "roles": [GroupMembershipRoles.MEMBER], - "actions": ["delete"], - "created": "1970-01-01T00:00:00.000000+00:00", - "updated": "1970-01-01T00:00:01.000000+00:00", - }, - { - "authority": group.authority, - "userid": other_user.userid, - "username": other_user.username, - "display_name": other_user.display_name, - "roles": [GroupMembershipRoles.MEMBER], - "actions": [], - "created": "1971-01-01T00:00:00.000000+00:00", - "updated": "1971-01-01T00:00:01.000000+00:00", - }, - ], - key=lambda membership: membership["username"], - ), + "data": [ + { + "authority": group.authority, + "userid": user.userid, + "username": user.username, + "display_name": user.display_name, + "roles": [GroupMembershipRoles.MEMBER], + "actions": ["delete"], + "created": "1970-01-01T00:00:00.000000+00:00", + "updated": "1970-01-01T00:00:01.000000+00:00", + }, + { + "authority": group.authority, + "userid": other_user.userid, + "username": other_user.username, + "display_name": other_user.display_name, + "roles": [GroupMembershipRoles.MEMBER], + "actions": [], + "created": "1971-01-02T00:00:00.000000+00:00", + "updated": "1971-01-02T00:00:01.000000+00:00", + }, + ], } def test_it_returns_404_if_user_does_not_have_read_access_to_group( diff --git a/tests/unit/h/services/group_members_test.py b/tests/unit/h/services/group_members_test.py index 2c48949128e..ccb42f0a6d8 100644 --- a/tests/unit/h/services/group_members_test.py +++ b/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):