Skip to content

Conversation

pablohashescobar
Copy link
Member

@pablohashescobar pablohashescobar commented Sep 11, 2025

Description

fix: correct member_ids in modules list to filter out deleted_at module members from the response

Type of Change

  • Bug fix (non-breaking change which fixes an issue)

Test Scenarios

  • remove a member from the module refresh and the member should be removed from the module

References

WEB-3528

Summary by CodeRabbit

  • Bug Fixes
    • Module member lists now include only active members, preventing deleted or inactive entries from appearing.
    • Improves accuracy of member counts and displays across module views.
    • Enhances reliability of member-related actions (e.g., filtering and exports) by excluding inactive members from results.

@Copilot Copilot AI review requested due to automatic review settings September 11, 2025 12:21
Copy link
Contributor

coderabbitai bot commented Sep 11, 2025

Walkthrough

The queryset in ModuleViewSet was modified by removing select_related on project, workspace, and lead. The ArrayAgg for member_ids now filters to include only non-null, non-deleted member IDs via modulemember__deleted_at__isnull=True.

Changes

Cohort / File(s) Summary
API viewset queryset adjustments
apps/api/plane/app/views/module/base.py
Removed select_related for project, workspace, lead; refined ArrayAgg filter to include only active, non-null member IDs using modulemember__deleted_at__isnull=True.

Sequence Diagram(s)

sequenceDiagram
  actor Client
  participant API as ModuleViewSet
  participant DB as Database

  Client->>API: GET /modules
  API->>API: build queryset (no select_related on project/workspace/lead)
  API->>DB: Query modules + ArrayAgg(member_ids where not null and not deleted)
  DB-->>API: Result set
  API-->>Client: JSON response
  Note over API: member_ids only for active members
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Suggested labels

🐛bug, requires approval

Suggested reviewers

  • sriramveeraghanta
  • vamsikrishnamathala

Pre-merge checks (2 passed, 1 warning)

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The title "[WEB-3528] fix: correct member id in modules list showing deleted_at members" clearly references the tracked ticket and describes the primary intent of the change—preventing deleted module members from appearing in the modules list—which matches the ArrayAgg filter change in the diff. It is directly related to the main change and communicates that this is a bug fix. The phrasing is slightly wordy and could be tightened for clarity.
Description Check ✅ Passed The PR description follows the repository template by providing a concise Description, marking the Type of Change as a bug fix, and including a Test Scenario and References, which makes it mostly complete. The Screenshots/Media section is omitted (acceptable if not applicable), but the References entry has a mismatch where the link text shows WEB-3528 while the URL points to WEB-3258 and should be corrected. Consider also expanding the Test Scenario with explicit steps or noting any automated tests added for verification.

Poem

I thump my paws on fields of code,
Pruned eager paths, a lighter load—
Only active burrow-mates remain,
No ghostly IDs in our domain.
Query hops are neat and tight,
Carrots cached, results just right. 🥕🐇

✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix-module-member-delete

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

makeplane bot commented Sep 11, 2025

Pull Request Linked with Plane Work Items

References

Comment Automatically Generated by Plane

Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes a bug where deleted module members were still appearing in the modules list response. The change filters out module members with a deleted_at timestamp to ensure only active members are returned.

  • Modified the member_ids annotation to exclude deleted module members
  • Removed unnecessary select_related calls for optimization

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Copy link
Contributor

@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

🧹 Nitpick comments (1)
apps/api/plane/app/views/module/base.py (1)

315-318: Make member_ids ordering deterministic.

Without an explicit ordering, Postgres can return arrays in arbitrary order, causing flaky diffs/tests. Consider ordering by member ID.

Apply:

 ArrayAgg(
   "members__id",
   distinct=True,
   filter=Q(
     members__id__isnull=False,
     modulemember__deleted_at__isnull=True,
   ),
+  ordering=("members__id",),
 )
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 85f23b4 and 9259694.

📒 Files selected for processing (1)
  • apps/api/plane/app/views/module/base.py (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Analyze (javascript)
🔇 Additional comments (2)
apps/api/plane/app/views/module/base.py (2)

315-318: Fix aligns with WEB-3528: soft-deleted memberships are excluded from member_ids.

Using ArrayAgg with a filtered join on modulemember__deleted_at__isnull=True correctly omits removed members; Coalesce to a typed empty UUID array is also right. Distinct avoids dupes. LGTM.


315-318: Confirm through model & field names for members lookup

                        filter=Q(
                            members__id__isnull=False,
                            modulemember__deleted_at__isnull=True,
                        ),

Search didn't find model files (plane/db/models missing) — only this usage at apps/api/plane/app/views/module/base.py:317. Confirm Module.members uses ModuleMember as the through model and that ModuleMember defines deleted_at (or that the through model's reverse related_name matches "modulemember"); if not, update the lookup to the actual related_name/field.

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.

2 participants