Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ba 1779 be multiple profiles member list #182

Merged
merged 8 commits into from
Dec 4, 2024

Conversation

hpmoreira05
Copy link
Contributor

@hpmoreira05 hpmoreira05 commented Nov 13, 2024

As a user, on the BaseApp Profile,I would like to I want to have a page where I can see the members of a profile, In order to be able to manage the members.

Original Story:
Acceptance Criteria
Context
This story will be about creating the option to access the members page, creating the structure of the members page and also creating the members modal and the relationship with the profiles

Business Rules - Members List - 5

Given a user has navigated to the Main Profile Settings Page for any profile, then they should see the "Members" option in the side menu.

After clicking the Members option, the user should be able to see a list of the members for the active profile with the following properties

Show a count of the total members (active and pending invite)

Pending invite members should be displayed in a light gray font with a pending status.

The members list will have its scrolling behavior inside its component, this means the page will not scroll if you do it inside the member list component.

Current behavior

Expected behavior

Code Snippet

Design Link: https://www.figma.com/design/XRD6wSl1m8Kz6XUcAy5CLp/BaseApp---WEB?node-id=3660-64822&node-type=frame&t=MBvMfObBLgXLqFQv-0
Template: https://bitbucket.org/silverlogic/baseapp-frontend-template/pull-requests/113
BA FE Package: silverlogic/baseapp-frontend#135

Summary by CodeRabbit

  • New Features

    • Introduced a new filtering mechanism for user roles with enhanced control over member visibility.
    • Added ordering options based on member status for improved sorting capabilities.
  • Bug Fixes

    • Implemented permission checks to ensure only authorized users can access profile members.
  • Tests

    • Added new tests to verify member ordering by status and default ordering behavior.
  • Chores

    • Updated version number to 0.3.2 in the configuration file.

Copy link

coderabbitai bot commented Nov 13, 2024

Walkthrough

The changes in this pull request involve modifications to the baseapp_profiles module, specifically enhancing the filtering logic for user roles and member visibility. A new MemberFilter is introduced, replacing the existing ProfileFilter in the BaseProfileUserRoleObjectType. The resolve_members method in BaseProfileObjectType is updated to include permission checks. Additionally, a new MemberOrderingFilter and MemberFilter classes are added in the filters module. Corresponding tests are introduced to validate the new ordering functionality based on member status, and the version number in the configuration file is incremented.

Changes

File Path Change Summary
baseapp_profiles/graphql/object_types.py Introduced MemberFilter, updated BaseProfileUserRoleObjectType to use MemberFilter, reformatted members field, and added permission checks in resolve_members.
baseapp_profiles/graphql/filters.py Loaded new model ProfileUserRole, added MemberOrderingFilter and MemberFilter classes to enhance filtering capabilities.
baseapp_profiles/tests/test_get_queries.py Added tests for member ordering by status and default ordering, imported ProfileUserRole, and added ProfileUserRoleFactory.
baseapp-profiles/setup.cfg Updated version number from 0.3.1 to 0.3.2.

Possibly related PRs

Suggested reviewers

  • nossila
  • vitorguima
  • Ronan-Fernandes
  • anicioalexandre

🐰 In the meadow where we play,
New filters hop in, bright as day.
Members ordered, roles defined,
Permissions checked, all aligned.
With a version bump, we take our leap,
In code we trust, our changes deep! 🌼


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between a02e473 and 2f78436.

📒 Files selected for processing (1)
  • baseapp-profiles/baseapp_profiles/graphql/object_types.py (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • baseapp-profiles/baseapp_profiles/graphql/object_types.py

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (1)
baseapp-profiles/baseapp_profiles/graphql/object_types.py (1)

97-99: Consider adding input validation for order_by_status

While the implementation works, consider adding an enum for valid ordering values to prevent invalid inputs and improve schema documentation.

+OrderByStatusEnum = graphene.Enum.from_enum('OrderByStatus', [('CUSTOM', 'custom')])

 class BaseProfileObjectType:
     members = DjangoFilterConnectionField(
         get_object_type_for_model(ProfileUserRole),
-        order_by_status=graphene.String()
+        order_by_status=OrderByStatusEnum()
     )
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between f5c11f4 and ae2fea9.

📒 Files selected for processing (1)
  • baseapp-profiles/baseapp_profiles/graphql/object_types.py (3 hunks)
🔇 Additional comments (3)
baseapp-profiles/baseapp_profiles/graphql/object_types.py (3)

12-12: LGTM: Import changes are appropriate

The new imports are correctly organized and necessary for implementing the custom status-based ordering functionality.


131-147: Implementation aligns well with PR objectives

The changes successfully implement the member list functionality with:

  • Proper permission checks
  • Custom ordering that prioritizes pending invites
  • Clean separation of concerns

The code is well-structured and maintainable.


137-145: 🛠️ Refactor suggestion

Use enum values for status ordering

Instead of hardcoding status values, use the ProfileRoleStatus enum to make the code more maintainable and type-safe.

 if order_by_status == "custom":
     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)),
+        When(status=ProfileUserRole.ProfileRoleStatus.PENDING, then=Value(1)),
+        When(status=ProfileUserRole.ProfileRoleStatus.INACTIVE, then=Value(2)),
+        When(status=ProfileUserRole.ProfileRoleStatus.ACTIVE, then=Value(3)),
         default=Value(4),
         output_field=IntegerField(),
     )

Likely invalid or redundant comment.

Copy link
Member

@nossila nossila left a comment

Choose a reason for hiding this comment

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

Please bump the version on the setup.cfg file


members_queryset = instance.members.all()

if order_by_status == "custom":
Copy link
Member

Choose a reason for hiding this comment

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

Ideally the filters should be on a FilterSet class that the ObjectType uses. The ProfileObjectType have an example. By doing that any DjangoFilterConnection that points to that ObjectType will have those filters without the need to define them on every connection

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (1)
baseapp-profiles/baseapp_profiles/graphql/object_types.py (1)

132-141: Improve code maintainability and documentation

  1. The custom ordering logic needs documentation to explain the business rules.
  2. Status values should be defined as constants for better maintainability.

Consider this refactor:

 if order_by_status == "custom":
+    # Order members by status priority:
+    # 1. Pending invitations
+    # 2. Inactive members
+    # 3. Active members
+    # 4. Others (fallback)
+    STATUS_PRIORITY = {
+        ProfileUserRole.ProfileRoleStatus.PENDING.value: 1,
+        ProfileUserRole.ProfileRoleStatus.INACTIVE.value: 2,
+        ProfileUserRole.ProfileRoleStatus.ACTIVE.value: 3,
+    }
     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)),
+        *[When(status=status, then=Value(priority)) 
+          for status, priority in STATUS_PRIORITY.items()],
         default=Value(4),
         output_field=IntegerField(),
     )
     members_queryset = members_queryset.order_by(status_order)
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between ae2fea9 and 53b1c0a.

📒 Files selected for processing (1)
  • baseapp-profiles/baseapp_profiles/graphql/object_types.py (3 hunks)
🔇 Additional comments (3)
baseapp-profiles/baseapp_profiles/graphql/object_types.py (3)

11-11: LGTM: Import changes are appropriate

The new imports from django.db.models are correctly placed and necessary for the status-based ordering functionality.


92-94: Consider moving filters to a dedicated FilterSet class

As mentioned in a previous review, filters should ideally be defined in a FilterSet class that the ObjectType uses. This promotes better code organization and reusability across different connections.

Consider creating a dedicated FilterSet class:

class ProfileUserRoleFilterSet(django_filters.FilterSet):
    order_by_status = django_filters.CharFilter(method='filter_order_by_status')
    
    class Meta:
        model = ProfileUserRole
        fields = ['order_by_status']

    def filter_order_by_status(self, queryset, name, value):
        if value == "custom":
            return queryset.order_by(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 queryset

126-129: ⚠️ Potential issue

Add input validation for order_by_status parameter

The method should validate the order_by_status parameter to prevent unexpected behavior.

Add validation:

 @classmethod
 def resolve_members(cls, instance, info, order_by_status=None, **kwargs):
     if not info.context.user.has_perm("baseapp_profiles.view_profile_members", instance):
         return instance.members.none()
+    
+    valid_orders = ["custom"]
+    if order_by_status and order_by_status not in valid_orders:
+        raise graphene.GraphQLError(f"Invalid order_by_status value. Allowed values: {valid_orders}")

Likely invalid or redundant comment.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (2)
baseapp-profiles/baseapp_profiles/graphql/object_types.py (2)

28-28: Remove unnecessary empty line

Consider removing this extra empty line to maintain consistent spacing.


136-144: Extract status ordering logic to a separate method

Consider extracting the status ordering logic to improve readability and maintainability.

 class BaseProfileObjectType:
+    @classmethod
+    def _get_status_order_case(cls):
+        return 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(),
+        )
+
     @classmethod
     def resolve_members(cls, instance, info, order_by_status=None, **kwargs):
         if not info.context.user.has_perm("baseapp_profiles.view_profile_members", instance):
             return instance.members.none()
 
         members_queryset = instance.members.all()
 
         if order_by_status == "custom":
-            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(),
-            )
+            status_order = cls._get_status_order_case()
             members_queryset = members_queryset.order_by(status_order)
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 53b1c0a and bf8598e.

📒 Files selected for processing (2)
  • baseapp-profiles/baseapp_profiles/graphql/__init__.py (1 hunks)
  • baseapp-profiles/baseapp_profiles/graphql/object_types.py (4 hunks)
✅ Files skipped from review due to trivial changes (1)
  • baseapp-profiles/baseapp_profiles/graphql/init.py
🔇 Additional comments (4)
baseapp-profiles/baseapp_profiles/graphql/object_types.py (4)

11-11: LGTM: Import changes are appropriate

The added imports are necessary for implementing the custom status-based ordering functionality.


25-27: LGTM: Well-structured interface implementation

The MemberInterface provides a clean abstraction for member access, and its implementation in BaseProfileUserRoleObjectType is correct.

Also applies to: 35-35


130-133: Add error handling for invalid order_by_status values

The method should handle invalid order_by_status values gracefully. Consider raising a GraphQL error for unsupported values.


96-98: 🛠️ Refactor suggestion

Add validation for order_by_status values

Consider adding an Enum for valid order_by_status values instead of using a plain String. This would provide better type safety and documentation.

+OrderByStatusEnum = graphene.Enum.from_enum('OrderByStatus', [('CUSTOM', 'custom')])
+
 class BaseProfileObjectType:
     members = DjangoFilterConnectionField(
-        get_object_type_for_model(ProfileUserRole), order_by_status=graphene.String()
+        get_object_type_for_model(ProfileUserRole), order_by_status=OrderByStatusEnum()
     )

Likely invalid or redundant comment.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (5)
baseapp-profiles/baseapp_profiles/graphql/filters.py (1)

28-49: Consider improving maintainability of status ordering values

The implementation is solid, but consider making the status ordering values more maintainable.

Consider this refactoring to improve maintainability:

 class MemberOrderingFilter(django_filters.OrderingFilter):
+    STATUS_ORDER = {
+        ProfileUserRole.ProfileRoleStatus.PENDING.value: 1,
+        ProfileUserRole.ProfileRoleStatus.INACTIVE.value: 2,
+        ProfileUserRole.ProfileRoleStatus.ACTIVE.value: 3,
+        'default': 4
+    }
     
     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),
+                *[When(status=status, then=Value(order))
+                  for status, order in self.STATUS_ORDER.items()
+                  if status != 'default'],
+                default=Value(self.STATUS_ORDER['default']),
                 output_field=IntegerField(),
             )
             return qs.order_by(status_order)
baseapp-profiles/baseapp_profiles/graphql/object_types.py (1)

28-28: Remove extra blank line

There's an unnecessary blank line that can be removed to maintain consistent spacing.

baseapp-profiles/baseapp_profiles/tests/test_get_queries.py (3)

149-149: Translate comment to English for consistency.

Replace the Portuguese comment with English.

-    # Cria um perfil com membros com diferentes status
+    # Create a profile with members having different statuses

148-193: Enhance test clarity and efficiency.

The test could be improved in several ways:

  1. Add a comment explaining the expected ordering logic (why PENDING comes before INACTIVE and ACTIVE)
  2. Consider using a more concise setup for creating test data
  3. Add assertion to verify relative ordering of members with the same status

Here's a suggested refactor:

 def test_members_ordered_by_status(django_user_client, graphql_user_client):
+    # Test member ordering by status
+    # Expected order: PENDING -> INACTIVE -> ACTIVE
+    # Members with same status maintain their creation order
     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,
-    )
+    # Create members with different statuses
+    test_statuses = [
+        ProfileUserRole.ProfileRoleStatus.ACTIVE,
+        ProfileUserRole.ProfileRoleStatus.PENDING,
+        ProfileUserRole.ProfileRoleStatus.ACTIVE,
+        ProfileUserRole.ProfileRoleStatus.INACTIVE,
+    ]
+    for status in test_statuses:
+        ProfileUserRoleFactory(profile=profile, status=status)

     response = graphql_user_client(
         query="""
             query Profile($id: ID!, $orderBy: String) {
                 profile(id: $id) {
                     members(orderBy: $orderBy) {
                         edges {
                             node {
                                 id
                                 status
+                                createdAt
                             }
                         }
                     }
                 }
             }
         """,
         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"]
+    
+    # Verify members with same status maintain creation order
+    active_members = [m for m in members if m["node"]["status"] == "ACTIVE"]
+    active_created_dates = [m["node"]["createdAt"] for m in active_members]
+    assert active_created_dates == sorted(active_created_dates)

Line range hint 95-142: Fix incorrect assertion in test_another_user_cant_view_members.

The test name suggests it should verify that another user cannot view members, but the assertion suggests the opposite behavior. This appears to be a bug in the test.

 def test_another_user_cant_view_members(graphql_user_client):
     profile = ProfileFactory()

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

     content = response.json()

-    assert content["data"]["profile"]["members"]
+    assert content["data"]["profile"]["members"] is None
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between bf8598e and 01d35bd.

📒 Files selected for processing (4)
  • baseapp-profiles/baseapp_profiles/graphql/filters.py (2 hunks)
  • baseapp-profiles/baseapp_profiles/graphql/object_types.py (4 hunks)
  • baseapp-profiles/baseapp_profiles/tests/test_get_queries.py (2 hunks)
  • baseapp-profiles/setup.cfg (1 hunks)
✅ Files skipped from review due to trivial changes (1)
  • baseapp-profiles/setup.cfg
🔇 Additional comments (9)
baseapp-profiles/baseapp_profiles/graphql/filters.py (2)

3-3: LGTM! Necessary imports and model loading

The added imports and model loading are appropriate for implementing the new member filtering functionality.

Also applies to: 6-6


52-57: Verify role field filtering implementation

The MemberFilter implementation looks good, using the custom ordering filter and appropriate model.

Let's verify the role field implementation:

✅ Verification successful

Based on the shell script results, I can now verify the role field implementation in the MemberFilter. The results show that:

  1. In the ProfileUserRole model, the role field is defined as an IntegerField with choices from ProfileRoles enum and default value of MANAGER
  2. The field is properly mapped in GraphQL schema through object_types.py
  3. The field is included in the initial migration

Role field filtering implementation is correctly configured

The MemberFilter implementation is valid as:

  • It correctly references the ProfileUserRole model which has a properly defined role field
  • The role field is properly included in the filter's fields list
  • The field type in the model (IntegerField with choices) is compatible with django-filter's default filtering behavior
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check role field definition in ProfileUserRole model and its usage

# Check role field in model
ast-grep --pattern 'class ProfileUserRole($_):
  $$$
  role = $$$
  $$$'

# Check if there are any role-related filters or validators
rg -A 5 'role.*=|role.*Field' 

Length of output: 39717

baseapp-profiles/baseapp_profiles/graphql/object_types.py (6)

15-15: LGTM: Import statement is correctly placed

The addition of MemberFilter import aligns with the module's organization and is used appropriately in the code.


25-27: LGTM: Well-structured interface implementation

The MemberInterface is correctly implemented with relay.Node and provides the necessary member field for ProfileUserRole relationships.


35-37: Consider moving filters to a dedicated FilterSet class

While the change to use MemberFilter is correct, consider reorganizing the filters following the pattern used in ProfileObjectType, where filters are defined in a dedicated FilterSet class. This would make the filters reusable across different DjangoFilterConnections.


96-98: LGTM: Properly configured members field

The DjangoFilterConnectionField is correctly configured with the appropriate model type.


132-133: 🛠️ Refactor suggestion

Add error handling for order_by_status parameter

While the permission check is good, the method should handle the order_by_status parameter as suggested in the previous review. Also, there's an unnecessary blank line that can be removed.

@classmethod
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()

25-37: Verify test coverage and interface implementation

Let's verify that all components using ProfileUserRole are updated for the new MemberInterface and that we have adequate test coverage.

baseapp-profiles/baseapp_profiles/tests/test_get_queries.py (1)

6-7: LGTM: Imports are correctly placed and necessary for the new tests.

Comment on lines +195 to +239
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"]

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]

@@ -144,6 +145,99 @@ 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):
# Cria um perfil com membros com diferentes status
Copy link
Contributor

Choose a reason for hiding this comment

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

remove comment

Copy link

sonarqubecloud bot commented Dec 4, 2024

Quality Gate Failed Quality Gate failed for 'silverlogic_baseapp-backend_baseapp-profiles'

Failed conditions
0.0% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

@nossila nossila merged commit d68df4e into master Dec 4, 2024
64 of 65 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants