Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 34 additions & 1 deletion baseapp-profiles/baseapp_profiles/graphql/filters.py
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
import django_filters
import swapper
from django.db.models import Q
from django.db.models import Case, IntegerField, Q, Value, When

Profile = swapper.load_model("baseapp_profiles", "Profile")
ProfileUserRole = swapper.load_model("baseapp_profiles", "ProfileUserRole")


class ProfileFilter(django_filters.FilterSet):
Expand All @@ -22,3 +23,35 @@ class Meta:

def filter_q(self, queryset, name, value):
return queryset.filter(Q(name__icontains=value) | Q(url_paths__path__icontains=value))


class MemberOrderingFilter(django_filters.OrderingFilter):
def __init__(self, *args, **kwargs):
super().__init__(*args, **kwargs)
self.extra["choices"] += [
("status", "Status"),
]

def filter(self, qs, value):
if value is None:
return qs

if any(v == "status" for v in value):
status_order = Case(
When(status=ProfileUserRole.ProfileRoleStatus.PENDING.value, then=Value(1)),
When(status=ProfileUserRole.ProfileRoleStatus.INACTIVE.value, then=Value(2)),
When(status=ProfileUserRole.ProfileRoleStatus.ACTIVE.value, then=Value(3)),
default=Value(4),
output_field=IntegerField(),
)
return qs.order_by(status_order)

return super().filter(qs, value)


class MemberFilter(django_filters.FilterSet):
order_by = MemberOrderingFilter()

class Meta:
model = ProfileUserRole
fields = ["role", "order_by"]
9 changes: 6 additions & 3 deletions baseapp-profiles/baseapp_profiles/graphql/object_types.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
from graphene import relay
from graphene_django.filter import DjangoFilterConnectionField

from .filters import ProfileFilter
from .filters import MemberFilter, ProfileFilter

Profile = swapper.load_model("baseapp_profiles", "Profile")
ProfileUserRole = swapper.load_model("baseapp_profiles", "ProfileUserRole")
Expand All @@ -30,7 +30,7 @@ class Meta:
model = ProfileUserRole
interfaces = [relay.Node]
fields = ["id", "pk", "user", "role", "created", "modified", "status"]
filter_fields = ["role"]
filterset_class = MemberFilter


class ProfileUserRoleObjectType(DjangoObjectType, BaseProfileUserRoleObjectType):
Expand Down Expand Up @@ -89,7 +89,9 @@ class BaseProfileObjectType:
target = graphene.Field(lambda: ProfileInterface)
image = ThumbnailField(required=False)
banner_image = ThumbnailField(required=False)
members = DjangoFilterConnectionField(get_object_type_for_model(ProfileUserRole))
members = DjangoFilterConnectionField(
get_object_type_for_model(ProfileUserRole),
)

class Meta:
interfaces = interfaces
Expand Down Expand Up @@ -124,6 +126,7 @@ def resolve_metadata(cls, instance, info):
def resolve_members(cls, instance, info, **kwargs):
if not info.context.user.has_perm("baseapp_profiles.view_profile_members", instance):
return instance.members.none()

return instance.members.all()


Expand Down
95 changes: 94 additions & 1 deletion baseapp-profiles/baseapp_profiles/tests/test_get_queries.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,8 @@
from baseapp_pages.tests.factories import URLPathFactory
from django.contrib.contenttypes.models import ContentType

from .factories import ProfileFactory
from ..models import ProfileUserRole
from .factories import ProfileFactory, ProfileUserRoleFactory

pytestmark = pytest.mark.django_db

Expand Down Expand Up @@ -144,6 +145,98 @@ def test_another_user_cant_view_members(graphql_user_client):
assert content["data"]["profile"]["members"]


def test_members_ordered_by_status(django_user_client, graphql_user_client):
user = django_user_client.user
profile = ProfileFactory(owner=user)
ProfileUserRoleFactory(
profile=profile,
status=ProfileUserRole.ProfileRoleStatus.ACTIVE,
)
ProfileUserRoleFactory(
profile=profile,
status=ProfileUserRole.ProfileRoleStatus.PENDING,
)
ProfileUserRoleFactory(
profile=profile,
status=ProfileUserRole.ProfileRoleStatus.ACTIVE,
)
ProfileUserRoleFactory(
profile=profile,
status=ProfileUserRole.ProfileRoleStatus.INACTIVE,
)

response = graphql_user_client(
query="""
query Profile($id: ID!, $orderBy: String) {
profile(id: $id) {
members(orderBy: $orderBy) {
edges {
node {
id
status
}
}
}
}
}
""",
variables={"id": profile.relay_id, "orderBy": "status"},
)

content = response.json()

members = content["data"]["profile"]["members"]["edges"]
statuses = [member["node"]["status"] for member in members]

assert statuses == ["PENDING", "INACTIVE", "ACTIVE", "ACTIVE"]


def test_members_not_ordered_by_status(django_user_client, graphql_user_client):
user = django_user_client.user
profile = ProfileFactory(owner=user)
ProfileUserRoleFactory(
profile=profile,
status=ProfileUserRole.ProfileRoleStatus.ACTIVE,
)
ProfileUserRoleFactory(
profile=profile,
status=ProfileUserRole.ProfileRoleStatus.PENDING,
)
ProfileUserRoleFactory(
profile=profile,
status=ProfileUserRole.ProfileRoleStatus.ACTIVE,
)
ProfileUserRoleFactory(
profile=profile,
status=ProfileUserRole.ProfileRoleStatus.INACTIVE,
)

response = graphql_user_client(
query="""
query Profile($id: ID!) {
profile(id: $id) {
members {
edges {
node {
id
status
}
}
}
}
}
""",
variables={"id": profile.relay_id},
)

content = response.json()

members = content["data"]["profile"]["members"]["edges"]
statuses = [member["node"]["status"] for member in members]

assert statuses == ["ACTIVE", "PENDING", "ACTIVE", "INACTIVE"]

Comment on lines +194 to +238
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Reduce code duplication and improve test clarity.

The test duplicates setup code from the previous test and doesn't clearly explain the expected default ordering behavior.

Consider refactoring to:

  1. Extract common setup into a fixture
  2. Add assertions to verify the ordering matches creation order
  3. Add documentation about the expected behavior

Here's a suggested refactor:

+@pytest.fixture
+def profile_with_members(django_user_client):
+    """Create a profile with members having different statuses."""
+    user = django_user_client.user
+    profile = ProfileFactory(owner=user)
+    test_statuses = [
+        ProfileUserRole.ProfileRoleStatus.ACTIVE,
+        ProfileUserRole.ProfileRoleStatus.PENDING,
+        ProfileUserRole.ProfileRoleStatus.ACTIVE,
+        ProfileUserRole.ProfileRoleStatus.INACTIVE,
+    ]
+    members = []
+    for status in test_statuses:
+        members.append(ProfileUserRoleFactory(profile=profile, status=status))
+    return profile, members

-def test_members_not_ordered_by_status(django_user_client, graphql_user_client):
+def test_members_not_ordered_by_status(django_user_client, graphql_user_client, profile_with_members):
+    """
+    Test that members are returned in creation order when no ordering is specified.
+    This verifies the default ordering behavior of the members query.
+    """
-    user = django_user_client.user
-    profile = ProfileFactory(owner=user)
-    ProfileUserRoleFactory(
-        profile=profile,
-        status=ProfileUserRole.ProfileRoleStatus.ACTIVE,
-    )
-    ProfileUserRoleFactory(
-        profile=profile,
-        status=ProfileUserRole.ProfileRoleStatus.PENDING,
-    )
-    ProfileUserRoleFactory(
-        profile=profile,
-        status=ProfileUserRole.ProfileRoleStatus.ACTIVE,
-    )
-    ProfileUserRoleFactory(
-        profile=profile,
-        status=ProfileUserRole.ProfileRoleStatus.INACTIVE,
-    )
+    profile, expected_members = profile_with_members

     response = graphql_user_client(
         query="""
             query Profile($id: ID!) {
                 profile(id: $id) {
                     members {
                         edges {
                             node {
                                 id
                                 status
+                                createdAt
                             }
                         }
                     }
                 }
             }
         """,
         variables={"id": profile.relay_id},
     )

     content = response.json()
     members = content["data"]["profile"]["members"]["edges"]
     statuses = [member["node"]["status"] for member in members]
+    created_dates = [member["node"]["createdAt"] for member in members]

-    assert statuses == ["ACTIVE", "PENDING", "ACTIVE", "INACTIVE"]
+    # Verify members are returned in creation order
+    assert created_dates == sorted(created_dates)
+    assert statuses == [m.status for m in expected_members]
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
def test_members_not_ordered_by_status(django_user_client, graphql_user_client):
user = django_user_client.user
profile = ProfileFactory(owner=user)
ProfileUserRoleFactory(
profile=profile,
status=ProfileUserRole.ProfileRoleStatus.ACTIVE,
)
ProfileUserRoleFactory(
profile=profile,
status=ProfileUserRole.ProfileRoleStatus.PENDING,
)
ProfileUserRoleFactory(
profile=profile,
status=ProfileUserRole.ProfileRoleStatus.ACTIVE,
)
ProfileUserRoleFactory(
profile=profile,
status=ProfileUserRole.ProfileRoleStatus.INACTIVE,
)
response = graphql_user_client(
query="""
query Profile($id: ID!) {
profile(id: $id) {
members {
edges {
node {
id
status
}
}
}
}
}
""",
variables={"id": profile.relay_id},
)
content = response.json()
members = content["data"]["profile"]["members"]["edges"]
statuses = [member["node"]["status"] for member in members]
assert statuses == ["ACTIVE", "PENDING", "ACTIVE", "INACTIVE"]
@pytest.fixture
def profile_with_members(django_user_client):
"""Create a profile with members having different statuses."""
user = django_user_client.user
profile = ProfileFactory(owner=user)
test_statuses = [
ProfileUserRole.ProfileRoleStatus.ACTIVE,
ProfileUserRole.ProfileRoleStatus.PENDING,
ProfileUserRole.ProfileRoleStatus.ACTIVE,
ProfileUserRole.ProfileRoleStatus.INACTIVE,
]
members = []
for status in test_statuses:
members.append(ProfileUserRoleFactory(profile=profile, status=status))
return profile, members
def test_members_not_ordered_by_status(django_user_client, graphql_user_client, profile_with_members):
"""
Test that members are returned in creation order when no ordering is specified.
This verifies the default ordering behavior of the members query.
"""
profile, expected_members = profile_with_members
response = graphql_user_client(
query="""
query Profile($id: ID!) {
profile(id: $id) {
members {
edges {
node {
id
status
createdAt
}
}
}
}
}
""",
variables={"id": profile.relay_id},
)
content = response.json()
members = content["data"]["profile"]["members"]["edges"]
statuses = [member["node"]["status"] for member in members]
created_dates = [member["node"]["createdAt"] for member in members]
# Verify members are returned in creation order
assert created_dates == sorted(created_dates)
assert statuses == [m.status for m in expected_members]


def test_search_profiles(graphql_user_client):
profile1 = ProfileFactory(name="David")
profile2 = ProfileFactory(name="Daniel")
Expand Down
2 changes: 1 addition & 1 deletion baseapp-profiles/setup.cfg
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[metadata]
name = baseapp_profiles
version = 0.3.1
version = 0.3.2
description = BaseApp Profiles
long_description = file: README.md
long_description_content_type = text/markdown
Expand Down
Loading