Skip to content

Commit

Permalink
Sort memberships by created time then username
Browse files Browse the repository at this point in the history 
Fixes #9139
  • Loading branch information
@seanh
seanh committed Dec 5, 2024
1 parent 25ba3cb commit b7912ac
Show file tree
Hide file tree
Showing 4 changed files with 108 additions and 62 deletions.
4 changes: 2 additions & 2 deletions h/services/group_members.py
View file
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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:
Expand Down
4 changes: 2 additions & 2 deletions h/views/api/group_members.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down
87 changes: 40 additions & 47 deletions tests/functional/api/groups/members_test.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand All @@ -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(
Expand All @@ -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),
),
]
)
Expand Down Expand Up @@ -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"],
Expand Down Expand Up @@ -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()
Expand All @@ -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]
],
}

Expand All @@ -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),
),
]
)
Expand All @@ -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(
Expand Down
75 changes: 64 additions & 11 deletions tests/unit/h/services/group_members_test.py
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
import logging
from datetime import datetime
from operator import attrgetter
from unittest import mock

import pytest
Expand Down Expand Up @@ -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()
Expand All @@ -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()
Expand All @@ -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()
Expand All @@ -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):
Expand Down

0 comments on commit b7912ac

Please sign in to comment.