Skip to content

Conversation

@tchoumi313
Copy link
Contributor

@tchoumi313 tchoumi313 commented Nov 28, 2025

Summary by CodeRabbit

  • New Features
    • Expanded filtering across the app: users can now filter items by folder hierarchy, asset class, perimeter, risk matrix/study, reference controls, risk assessments, approvers/requesters, and parent folders for finer-grained searches and reporting.
    • Framework/provider view simplified to focus on provider-based filtering for clearer results.

✏️ Tip: You can customize this high-level summary in your review settings.

@tchoumi313 tchoumi313 self-assigned this Nov 28, 2025
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 28, 2025

Walkthrough

Added explicit ModelMultipleChoiceFilter declarations for many relationship fields in backend filter classes and removed those fields from their Meta.fields to tighten the public filter surface and avoid duplicate/implicit filters.

Changes

Cohort / File(s) Summary
Filter field declarations (primary)
backend/core/views.py
Added explicit ModelMultipleChoiceFilter fields across multiple filter classes (examples: folder, asset_class, perimeter, risk_matrix, ebios_rm_study, reference_control, risk_assessment, approver, requester, parent_folder) and removed those same fields from the respective Meta.fields declarations to prevent implicit auto-generated filters.
Related filter classes (grouped in same file)
backend/core/views.py (e.g., AssetFilter, RiskAssessmentFilterSet, AppliedControlFilterSet, RiskScenarioFilter, RiskAcceptanceFilterSet, ValidationFlowFilterSet, FolderFilter, FrameworkFilter, TaskTemplateFilter, ...)
Public surface adjustments only: explicit filters added to class bodies; corresponding Meta.fields entries pruned. No internal wiring or behavior changes indicated beyond filter exposure.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10–20 minutes

  • Areas to check:
    • Correct queryset and (if present) field_name for each new ModelMultipleChoiceFilter
    • That every removed Meta.fields entry corresponds to an explicit class filter
    • API schema/serialization to ensure no accidental breaking changes in filter names

Possibly related PRs

Suggested labels

High Value

Suggested reviewers

  • eric-intuitem
  • nas-tabchiche
  • ab-smith

Poem

🐰 I hopped through code with careful paws and care,

Filters once hidden now stand bold and fair.
Folder and approver, asset class in sight,
Queries dance neatly in morning light.
A little crunchy carrot for CI’s delight 🥕

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.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
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: fixing inconsistent filtering on many-to-many fields across multiple filter sets.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch CA-1420-the-filters-on-applied-controls-break-and-eventually-other-object-filters-do-as-well

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
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 (2)
backend/core/views.py (2)

3151-3154: AppliedControl filters: good, but verify query param format from frontend

The new folder and reference_control ModelMultipleChoiceFilters plus explicit Meta.fields entries for category/csf_function/priority/effort/control_impact work correctly with the existing NullableChoiceFilters and should fix multi‑select behavior on these relations.

One integration detail to double‑check: ModelMultipleChoiceFilter expects multiple values as repeated query params (?folder=id1&folder=id2), not a single comma‑separated string. If the frontend is still sending CSVs, you may want to normalize GET parameters similarly to _process_request_data or switch relevant lookups to an in-style filter class.

Also applies to: 3248-3254


5136-5160: Parent folder filter on FolderFilter is correct but could be surfaced in Meta

parent_folder = ModelMultipleChoiceFilter(queryset=Folder.objects.all()) is appropriate for hierarchical navigation. If you rely on Meta.fields for documentation or schema generation, consider adding "parent_folder" there as well; otherwise the explicit filter alone is fine.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 71819da and 2d1c641.

📒 Files selected for processing (1)
  • backend/core/views.py (11 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
backend/core/views.py (4)
backend/iam/models.py (1)
  • Folder (68-316)
backend/core/models.py (4)
  • Perimeter (1861-1909)
  • RiskMatrix (1478-1567)
  • ReferenceControl (1405-1475)
  • RiskAssessment (4338-4742)
backend/ebios_rm/models.py (5)
  • risk_matrix (352-353)
  • risk_matrix (951-952)
  • risk_matrix (1030-1031)
  • ebios_rm_study (947-948)
  • EbiosRMStudy (57-296)
backend/resilience/models.py (1)
  • risk_matrix (214-215)
⏰ 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). (9)
  • GitHub Check: enterprise-startup-docker-compose-test
  • GitHub Check: enterprise-startup-functional-test (3.12)
  • GitHub Check: startup-functional-test (3.12)
  • GitHub Check: startup-docker-compose-test
  • GitHub Check: build_community_frontend
  • GitHub Check: build_enterprise_frontend
  • GitHub Check: test (3.12)
  • GitHub Check: build (3.12)
  • GitHub Check: Analyze (python)
🔇 Additional comments (7)
backend/core/views.py (7)

941-983: Explicit folder/asset_class filters in AssetFilter look correct

Defining folder and asset_class as ModelMultipleChoiceFilters is consistent with how other relationship filters are modeled and avoids relying on implicit Meta.fields behavior. No issues spotted.


1900-1918: RiskAssessment relationship filters are well‑scoped

Adding folder, perimeter, risk_matrix, and ebios_rm_study as explicit ModelMultipleChoiceFilters aligns the API with typical usage (multi‑select on these relations) and keeps Meta.fields restricted to scalar fields. Implementation looks sound.


4361-4419: New risk_assessment filter on RiskScenario is consistent and non‑breaking

The added risk_assessment = ModelMultipleChoiceFilter(queryset=RiskAssessment.objects.all()) provides a direct multi‑select filter while keeping the existing folder/perimeter aliases intact. No logic or RBAC issues evident.


4714-4735: RiskAcceptance filters match common use cases

Adding folder and approver as ModelMultipleChoiceFilters mirrors how the viewset already exposes these relations (e.g., waiting and state transitions). Filtering semantics are clear and should behave as expected.


4914-4952: ValidationFlowFilterSet gains useful relational filters

The new folder, requester, and approver ModelMultipleChoiceFilters integrate cleanly with the existing linked_models filter and ValidationFlowViewSet. This should make complex filtering significantly easier without changing existing query behavior.


6469-6492: FrameworkFilter: explicit folder filter + tighter Meta fields

Adding folder = ModelMultipleChoiceFilter(queryset=Folder.objects.all()) and narrowing Meta.fields to ["provider"] is a clean separation: relations are handled explicitly, simple scalars via Meta. This should help avoid ambiguous auto‑generated filters.


10401-10420: TaskTemplateFilter: folder filter aligns with task RBAC and calendar logic

The new folder ModelMultipleChoiceFilter matches how TaskTemplateViewSet already scopes and reports tasks. Using it via filterset_class = TaskTemplateFilter is consistent, and the implementation doesn’t conflict with the existing filterset_fields list.

@nas-tabchiche nas-tabchiche changed the title fix: filters on applied controls break and eventually other object filters do as well fix: inconsistent filtering on many to many fields Nov 28, 2025
Copy link
Collaborator

@nas-tabchiche nas-tabchiche left a comment

Choose a reason for hiding this comment

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

Nice catch! LGTM. To merge once CI is green

…k-and-eventually-other-object-filters-do-as-well
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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
backend/core/views.py (1)

1671-1688: Bug: RiskMatrixViewSet.probability will crash due to list–union misuse

The code initializes options as an empty list and then attempts to use it with the | operator:

options = []
...
options = options | choices.items()

In Python, list | ... raises TypeError because the bitwise OR operator is not defined for lists. This endpoint will crash at runtime.

The suggested fix changes the accumulation to use dicts, matching the pattern in the risk() endpoint:

-        undefined = {-1: "--"}
-        options = []
+        undefined = {-1: "--"}
+        options = undefined
         for matrix in RiskMatrix.objects.filter(id__in=viewable_matrices):
             _choices = {
                 i: name
                 for i, name in enumerate(
                     x["name"] for x in matrix.json_definition["probability"]
                 )
             }
-            choices = undefined | _choices
-            options = options | choices.items()
-        return Response(
-            [{k: v for k, v in zip(("value", "label"), o)} for o in options]
-        )
+            options = options | _choices
+        res = [{"value": k, "label": v} for k, v in options.items()]
+        return Response(res)

This resolves the TypeError and aligns with the working risk() method implementation.

🧹 Nitpick comments (2)
backend/core/views.py (2)

3172-3202: AppliedControlFilterSet: folder/reference_control filters are correct; consider trimming Meta for choice fields

The explicit folder and reference_control ModelMultipleChoiceFilters are appropriate for these FKs and match the pattern used elsewhere; excluding them from Meta.fields prevents extra implicit lookup variants from being generated.

category, csf_function, priority, effort, and control_impact are now handled via NullableChoiceFilter but also remain in Meta.fields with only "exact". That’s harmless, but you could optionally drop those keys from Meta.fields and rely solely on the explicit filters to make the filter surface easier to reason about.

Also applies to: 3269-3276


10423-10443: TaskTemplateFilter: folder filter is fine; filterset_fields on the ViewSet is now redundant

folder = ModelMultipleChoiceFilter(queryset=Folder.objects.all()) is correct for the FK and matches the pattern used for other resources.

Note that TaskTemplateViewSet sets both filterset_class = TaskTemplateFilter and filterset_fields = [...]. In DRF, the explicit filterset_class wins, so filterset_fields is effectively unused for filter generation and can be confusing to future readers. Consider dropping filterset_fields there (or the explicit filterset_class) to have a single source of truth.

Also applies to: 10470-10478

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2d1c641 and e9dcaa9.

📒 Files selected for processing (1)
  • backend/core/views.py (11 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
backend/core/views.py (4)
backend/iam/models.py (2)
  • Folder (68-316)
  • User (496-784)
backend/core/models.py (4)
  • Perimeter (1861-1909)
  • RiskMatrix (1478-1567)
  • ReferenceControl (1405-1475)
  • RiskAssessment (4346-4750)
backend/ebios_rm/models.py (5)
  • risk_matrix (352-353)
  • risk_matrix (951-952)
  • risk_matrix (1030-1031)
  • ebios_rm_study (947-948)
  • EbiosRMStudy (57-296)
backend/resilience/models.py (1)
  • risk_matrix (214-215)
⏰ 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). (9)
  • GitHub Check: enterprise-startup-docker-compose-test
  • GitHub Check: enterprise-startup-functional-test (3.12)
  • GitHub Check: startup-docker-compose-test
  • GitHub Check: build_community_frontend
  • GitHub Check: startup-functional-test (3.12)
  • GitHub Check: build_enterprise_frontend
  • GitHub Check: build (3.12)
  • GitHub Check: test (3.12)
  • GitHub Check: Analyze (python)
🔇 Additional comments (7)
backend/core/views.py (7)

963-1004: AssetFilter: explicit multi-select filters for folder and asset_class look good

Defining folder and asset_class as ModelMultipleChoiceFilters while omitting them from Meta.fields aligns with the goal of consistent, explicit multi-select behaviour and avoids extra implicit lookups. Just ensure the frontend is sending these as multi-valued params in the way django-filter expects (repeated params vs comma-separated), since that drives how ModelMultipleChoiceFilter parses values.


1921-1939: RiskAssessmentFilterSet: new foreign-key multi-select filters are consistent and scoped

The added folder, perimeter, risk_matrix, and ebios_rm_study ModelMultipleChoiceFilters are correct for the underlying FK fields, and removing these keys from Meta.fields keeps the exposed lookups to this explicit surface (no extra __in/range variants). Please double-check that existing clients are not relying on any of the removed implicit lookups (e.g. risk_matrix__in), and update API docs accordingly if they were.


4382-4442: RiskScenarioFilter: adding risk_assessment multi-select filter is coherent with aliases

The new risk_assessment = ModelMultipleChoiceFilter(...) is appropriate for the FK and complements the existing folder/perimeter alias filters. With risk_assessment removed from Meta.fields, only this explicit filter and your alias UUID filters are exposed, which should help avoid confusing implicit lookups. Verify that UI code is updated to use risk_assessment= rather than any older risk_assessment__in patterns.


4735-4757: RiskAcceptanceFilterSet: explicit folder/approver filters are correct

Using ModelMultipleChoiceFilter for folder and approver matches the underlying relations and brings RiskAcceptance filtering in line with other resources. Since these fields are no longer in Meta.fields, only the intended exact multi-select behaviour is available. Just confirm that any clients relying on the previous single-value behaviour (if any) have been adjusted.


4935-4975: ValidationFlowFilterSet: multi-select filters for folder/requester/approver are consistent

The new folder, requester, and approver ModelMultipleChoiceFilters correctly reflect their FK types and make the filtering surface explicit. Keeping these out of Meta.fields avoids extra implicit lookups that might not be supported by the UI. This is consistent with the rest of the PR.


5157-5182: FolderFilter: parent_folder multi-select filter is appropriate

Adding parent_folder = ModelMultipleChoiceFilter(Folder.objects.all()) is the right way to expose hierarchical filtering on folders and pairs well with the existing owned and content_type filters. Excluding parent_folder from Meta.fields keeps the API surface limited to exactly this filter. You may later want a RBAC-scoped queryset here, but that matches existing patterns elsewhere.


6491-6514: FrameworkFilter: explicit folder filter plus Meta.fields reduction matches intent

Defining folder as a ModelMultipleChoiceFilter while trimming Meta.fields down to ["provider"] avoids implicit framework-folder lookups and makes the two supported filters (folder, provider) very clear. This should also keep the behaviour consistent with FrameworkViewSet.filterset_class = FrameworkFilter.

@ab-smith ab-smith changed the title fix: inconsistent filtering on many to many fields fix: inconsistent filtering on 'many to many' fields Dec 3, 2025
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