Skip to content

Conversation

@eric-intuitem
Copy link
Collaborator

@eric-intuitem eric-intuitem commented Dec 28, 2025

Show domains with full path
Fix sorting for domains, perimeters, roles

Summary by CodeRabbit

  • New Features

    • Lists (folders, perimeters, roles, user groups) can be ordered by full display paths; parent-domain autocomplete filter added.
  • UI/Display Updates

    • List views show full-path strings for folders and perimeters and have adjusted columns.
    • Detail view renders full path for folder-related items.
    • Risk assessments list no longer displays the numeric "risk_tolerance" field.
  • Tests

    • Test data includes path strings for perimeters; a short 3s wait added in a functional test.

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

fix domain list view
filter on parent folder shows full path
Show full path in detailview for folder
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 28, 2025

📝 Walkthrough

Walkthrough

Models now produce hierarchical display paths via updated str implementations; serializers expose a read-only str field. Views introduce a PathAwareOrderingFilter and enable path-based ordering for folders/perimeters. Frontend list/detail views, table configs, and tests updated to use str; one functional test adds a 3s wait.

Changes

Cohort / File(s) Summary
Model string changes
backend/core/models.py, backend/iam/models.py
Perimeter.__str__ now returns f"{self.folder}/{self.name}". Folder.__str__ builds a joined folder path via get_folder_full_path(...). UserGroup.__str__ and get_localization_dict() use str(self.folder) instead of folder.name.
Serializer additions
backend/core/serializers.py
Added read-only str fields to PerimeterReadSerializer and FolderReadSerializer mapped to __str__.
Path-aware ordering & view updates
backend/core/views.py
Added PathAwareOrderingFilter (Python-side ordering for path fields). PerimeterViewSet, FolderViewSet, and UserGroupOrderingFilter switched to use it; "str" added to ordering/path fields and relevant search_fields updated.
Enterprise role ordering
enterprise/backend/enterprise_core/views.py
Added RoleOrderingFilter (Python-side, case-insensitive/locale-aware name sorting) and included it in RoleViewSet filter_backends; ordering_fields = ["name"] exposed.
Detail view rendering
frontend/src/lib/components/DetailView/DetailView.svelte
When key is name for folder models/URLs and data.data.str exists, render data.data.str directly; otherwise behavior unchanged.
CRUD config change
frontend/src/lib/utils/crud.ts
Removed risk_tolerance from URL_MODEL_MAP['risk-assessments'].selectFields; only status remains.
Table and filters
frontend/src/lib/utils/table.ts
Added exported PARENT_DOMAIN_FILTER. listViewFields['folders'] body now uses str (replacing name), removed parentDomain header, added parent_folder filter. listViewFields['perimeters'] body uses str (replacing name) and removed domain column.
Frontend tests
frontend/tests/utils/test-utils.ts, frontend/tests/functional/detailed/ebios-rm.test.ts
Test builder adds str to perimetersPage.build. Functional test inserts a 3s wait after clicking "Generate the risk" before filling the form.

Sequence Diagram(s)

sequenceDiagram
    autonumber
    actor Client as Client
    participant ViewSet as "ViewSet\n(Perimeter/Folder)"
    participant PathFilter as "PathAwareOrderingFilter"
    participant DB as "QuerySet / DB"
    participant Serializer as "Serializer\n(PerimeterRead/FolderRead)"

    Note over PathFilter: Path-aware ordering triggers Python-side sort\n(using obj.__str__ / related FK traversal)
    Client->>ViewSet: GET /perimeters/?ordering=str
    activate ViewSet
    ViewSet->>PathFilter: filter_queryset(request, queryset, view)
    activate PathFilter
    alt ordering field is path-based ("str")
        PathFilter->>DB: fetch full result set (no DB ORDER BY)
        activate DB
        DB-->>PathFilter: unsorted results
        deactivate DB
        PathFilter->>PathFilter: compute keys via obj.__str__ (and related fields)
        PathFilter->>PathFilter: Python-side stable sort
    else standard SQL ordering
        PathFilter->>DB: apply order_by(...)
        activate DB
        DB-->>PathFilter: ordered results
        deactivate DB
    end
    PathFilter-->>ViewSet: sorted_queryset
    deactivate PathFilter
    ViewSet->>Serializer: serialize(sorted_queryset)
    activate Serializer
    Serializer->>Serializer: include `str` field via __str__()
    Serializer-->>ViewSet: JSON payload
    deactivate Serializer
    ViewSet-->>Client: HTTP 200 + JSON
    deactivate ViewSet
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐇
I hop through folders, stitch names end to end,
I glue folder and perimeter — a tidy bend.
Lists show my trail, details hum the chord,
A rabbit's small cheer for paths now adored! 🎉

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 18.75% 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 describes the main objectives: showing domain full paths and fixing sorting for domains/perimeters/roles.
✨ Finishing touches
  • 📝 Generate docstrings

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.

@eric-intuitem eric-intuitem marked this pull request as ready for review December 29, 2025 01:36
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: 4

🧹 Nitpick comments (7)
backend/core/views.py (1)

5615-5618: Minor: duplicated view-level path_fields is unused given path_ordering_fields

In FolderViewSet:

ordering_fields = ["name", "str", "parent_folder", "description", "content_type"]
batch_size = 100
path_ordering_fields = {"str"}
path_fields = {"str"}

PathAwareOrderingFilter only consults self.path_fields (on the filter) or view.path_ordering_fields; view.path_fields is never read. Same pattern exists in PerimeterViewSet.

Not a bug, but you can drop path_fields on the view classes to avoid confusion around which attribute is actually used:

-    ordering_fields = ["name", "str", "parent_folder", "description", "content_type"]
-    batch_size = 100  # Configurable batch size for processing domain import
-    path_ordering_fields = {"str"}
-    path_fields = {"str"}
+    ordering_fields = ["name", "str", "parent_folder", "description", "content_type"]
+    batch_size = 100  # Configurable batch size for processing domain import
+    path_ordering_fields = {"str"}

Same simplification applies to PerimeterViewSet if you like.

backend/core/models.py (1)

2324-2325: Path-aware Perimeter.__str__ looks good; consider simplifying downstream usage

Using f"{self.folder}/{self.name}" is a clean way to leverage Folder.__str__ for full paths and aligns with how other path-like models work.

One non-blocking follow-up to consider: RiskAssessment.path_display currently does:

return f"{self.perimeter.folder}/{self.perimeter}/{self.name} - {self.version}"

Now that Perimeter.__str__ already includes the folder path, this results in the folder segment being repeated in that string. You could simplify it to something like:

return f"{self.perimeter}/{self.name} - {self.version}"

to avoid duplication and keep the path shorter. This is purely cosmetic and can be done in a separate cleanup.

frontend/tests/utils/test-utils.ts (1)

551-561: Aligns test data with new Perimeter path string

Adding str: \${vars.folderName}/${vars.perimeterName}`keeps the test data in sync with the new Perimeterstr/strrepresentation and matches the existing pattern used for other models’strfields. Just be aware this hard-codes the expected format, so any future change toPerimeter.str` will require updating this test helper as well.

backend/iam/models.py (2)

134-140: Path-aware Folder.__str__ implementation looks correct

The new __str__ builds a path from get_folder_full_path(include_root=False), filters out empty names, and joins with /, falling back to self.name/"" when needed. This is safe for root and shallow folders and gives a stable, human-readable path for use in serializers and UI sorting.


386-400: UserGroup display now uses folder path instead of bare folder name

Switching UserGroup.__str__ and get_localization_dict()['folder'] to use self.folder/str(self.folder) (rather than folder.name) correctly propagates the new folder path representation into group labels and the user-groups list view. This improves disambiguation for similarly named domains without affecting permission logic.

frontend/src/lib/utils/table.ts (2)

121-129: Parent-domain filter correctly uses path-aware Folder labels

PARENT_DOMAIN_FILTER reuses the existing folders endpoint and switches the label field to str, which matches the new FolderReadSerializer and ensures the parent-domain selector shows full paths. This is consistent with other filters using optionsLabelField.


1205-1221: Folder & Perimeter tables now sort on path-aware str values

  • folders list now displays/sorts the first column using body: ['str', ...] while keeping the header key as 'name'. That gives users a full domain path for the “name” column and fixes sorting to be path-based.
  • perimeters list similarly switches the second body field from name to str, so sorting on the “name” column becomes path-aware too.
  • Adding parent_folder: PARENT_DOMAIN_FILTER to folders.filters wires the new filter into the list view with appropriate backend field naming.

All of this lines up with the new str fields in the backend and the PR goal to show and sort by full paths. If you also want domain filters elsewhere to show paths instead of raw names, you could optionally reuse PARENT_DOMAIN_FILTER (or at least its optionsLabelField: 'str') for DOMAIN_FILTER as well.

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f92bd23 and 01547c6.

📒 Files selected for processing (9)
  • backend/core/models.py
  • backend/core/serializers.py
  • backend/core/views.py
  • backend/iam/models.py
  • enterprise/backend/enterprise_core/views.py
  • frontend/src/lib/components/DetailView/DetailView.svelte
  • frontend/src/lib/utils/crud.ts
  • frontend/src/lib/utils/table.ts
  • frontend/tests/utils/test-utils.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-05T09:18:16.194Z
Learnt from: Mohamed-Hacene
Repo: intuitem/ciso-assistant-community PR: 2472
File: backend/iam/models.py:906-907
Timestamp: 2025-09-05T09:18:16.194Z
Learning: In the get_accessible_object_ids method in backend/iam/models.py, for objects with parent_folder attribute (like Folder objects), the folder association logic treats each folder as being associated with itself, so the mapping is folder_id → folder_id. The code `[(fid, fid) for fid in folder_permissions.keys()]` correctly implements this self-referential mapping rather than querying for child folders.

Applied to files:

  • backend/iam/models.py
🧬 Code graph analysis (3)
backend/iam/models.py (1)
backend/core/models.py (1)
  • get_folder_full_path (5454-5455)
backend/core/views.py (2)
backend/core/models.py (2)
  • full_path (3343-3347)
  • get_folder_full_path (5454-5455)
backend/iam/models.py (3)
  • Folder (80-332)
  • get_folder_full_path (158-166)
  • get_folder_full_path (347-351)
enterprise/backend/enterprise_core/views.py (2)
backend/core/views.py (2)
  • filter_queryset (1147-1205)
  • BaseModelViewSet (606-1103)
backend/iam/models.py (1)
  • Role (809-824)
🪛 GitHub Actions: Backend Linters
enterprise/backend/enterprise_core/views.py

[error] 1-1: ruff format --check . reported that 1 file would be reformatted. Run 'ruff format' to fix code style issues in enterprise_core/views.py.

⏰ 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-functional-test (3.12)
  • GitHub Check: startup-functional-test (3.12)
  • GitHub Check: startup-docker-compose-test
  • GitHub Check: enterprise-startup-docker-compose-test
  • GitHub Check: build (3.12)
  • GitHub Check: build_enterprise_frontend
  • GitHub Check: build_community_frontend
  • GitHub Check: test (3.12)
  • GitHub Check: Analyze (python)
🔇 Additional comments (4)
enterprise/backend/enterprise_core/views.py (1)

279-280: LGTM!

The RoleOrderingFilter integration looks correct. The filter backends are properly ordered, and restricting ordering_fields to ["name"] ensures only the field with special sorting logic is exposed for client-side ordering requests.

frontend/src/lib/components/DetailView/DetailView.svelte (1)

438-440: Folder name override to use data.data.str is correct and safely guarded

The new branch for key === 'name' on folder detail views cleanly prefers the backend-provided str (full path) and falls back to the existing value rendering otherwise. The additional checks on data.model?.name / data.urlModel and data.data?.str make this safe and scoped to folders only.

backend/core/serializers.py (2)

264-274: Expose Perimeter path string via str

Adding str = serializers.CharField(source="__str__", read_only=True) cleanly exposes the Perimeter display path for list/detail APIs without impacting writes. This matches the existing str pattern used elsewhere and allows the frontend to sort/filter on the full path.


1413-1423: Expose Folder path string via str

The new str field sourced from __str__ on Folder is consistent with other serializers and gives the frontend a single, canonical path string to use (e.g., in filters and tables) while keeping the write serializer unchanged.

workaround for flaky test
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)
frontend/tests/functional/detailed/ebios-rm.test.ts (1)

592-597: Replace fixed timeout with conditional wait.

The fixed 3-second waitForTimeout is a test smell that makes tests slower and potentially still flaky. The rest of this test file uses proper waiting mechanisms like expect().toPass() and waiting for specific elements.

🔎 Suggested approaches consistent with patterns in this file

Option 1 (Preferred): Wait for modal to be visible before filling the form

 await page.getByRole('button', { name: ' Step 1 Generate the risk' }).click();
-await page.waitForTimeout(3000);
+await expect(page.getByTestId('modal-title')).toBeVisible({ timeout: 10_000 });
 await riskAssessmentsPage.form.fill({
   name: 'test-risk-assessment-ebios-rm',
   perimeter: `${vars.folderName}/${vars.perimeterName}`
 });

Option 2: Wait for loading spinners to disappear (pattern used in lines 307-311, 338-342, etc.)

 await page.getByRole('button', { name: ' Step 1 Generate the risk' }).click();
-await page.waitForTimeout(3000);
+for (const spinner of await page.locator('.loading-spinner').all()) {
+  await expect(spinner).not.toBeVisible({ timeout: 10_000 });
+}
 await riskAssessmentsPage.form.fill({
   name: 'test-risk-assessment-ebios-rm',
   perimeter: `${vars.folderName}/${vars.perimeterName}`
 });

Option 3: Use toPass() retry pattern (used in lines 150-154, 172-176, etc.)

 await page.getByRole('button', { name: ' Step 1 Generate the risk' }).click();
-await page.waitForTimeout(3000);
-await riskAssessmentsPage.form.fill({
-  name: 'test-risk-assessment-ebios-rm',
-  perimeter: `${vars.folderName}/${vars.perimeterName}`
-});
-await page.getByTestId('save-button').click();
+await expect(async () => {
+  await riskAssessmentsPage.form.fill({
+    name: 'test-risk-assessment-ebios-rm',
+    perimeter: `${vars.folderName}/${vars.perimeterName}`
+  });
+  await page.getByTestId('save-button').click();
+}).toPass({ timeout: 80_000, intervals: [500, 1000, 2000] });
📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ef95425 and d445a7b.

📒 Files selected for processing (1)
  • frontend/tests/functional/detailed/ebios-rm.test.ts
⏰ 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: build (3.12)
  • GitHub Check: build_enterprise_frontend
  • GitHub Check: build_community_frontend
  • GitHub Check: enterprise-startup-docker-compose-test
  • GitHub Check: startup-docker-compose-test
  • GitHub Check: startup-functional-test (3.12)
  • GitHub Check: enterprise-startup-functional-test (3.12)
  • GitHub Check: test (3.12)
  • GitHub Check: Analyze (python)

@eric-intuitem eric-intuitem marked this pull request as draft December 29, 2025 18:34
duplicate code
@eric-intuitem eric-intuitem marked this pull request as ready for review December 30, 2025 12:33
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

♻️ Duplicate comments (1)
enterprise/backend/enterprise_core/views.py (1)

253-270: Pagination fix from previous review is missing.

The pagination issue raised in the previous review was marked as "confirmed as addressed," but the fix is not present in the current code. Line 267 still returns the plain data list without checking for pagination support, which will break DRF pagination when sorting roles by name.

🔎 Apply the previously suggested pagination fix
 class RoleOrderingFilter(filters.OrderingFilter):
     """
     Order roles by their display name (sensitive to builtin translations).
     """

     def filter_queryset(self, request, queryset, view):
         ordering = self.get_ordering(request, queryset, view)
         if not ordering:
             return queryset

         if len(ordering) == 1 and ordering[0].lstrip("-") == "name":
             reverse = ordering[0].startswith("-")
             data = list(queryset)
             data.sort(key=lambda role: str(role).casefold(), reverse=reverse)
+
+            # DRF pagination support
+            if getattr(view, "paginator", None):
+                return view.paginator.paginate_queryset(data, request, view=view)
             return data

         return super().filter_queryset(request, queryset, view)
🧹 Nitpick comments (2)
enterprise/backend/enterprise_core/views.py (1)

280-280: Consider reordering filter backends for conventional ordering.

The typical DRF pattern places SearchFilter before OrderingFilter, resulting in the sequence: filter → search → order. The current order (filter → order → search) is functional but unconventional.

🔎 Conventional ordering
-    filter_backends = [DjangoFilterBackend, RoleOrderingFilter, filters.SearchFilter]
+    filter_backends = [DjangoFilterBackend, filters.SearchFilter, RoleOrderingFilter]
frontend/src/lib/utils/table.ts (1)

112-119: Consider adding optionsLabelField: 'str' to DOMAIN_FILTER for consistency.

The newly added PARENT_DOMAIN_FILTER explicitly specifies optionsLabelField: 'str' to display hierarchical paths, while the existing DOMAIN_FILTER doesn't specify this property. If the intent is to show full paths consistently across all domain filters, consider updating DOMAIN_FILTER similarly.

🔎 Proposed consistency update
 export const DOMAIN_FILTER: ListViewFilterConfig = {
 	component: AutocompleteSelect,
 	props: {
 		optionsEndpoint: 'folders?content_type=DO&content_type=GL',
+		optionsLabelField: 'str',
 		label: 'domain',
 		multiple: true
 	}
 };

Note: This change would affect all usages of DOMAIN_FILTER throughout the file. Verify this is the desired behavior before applying.

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d445a7b and 1f8353a.

📒 Files selected for processing (3)
  • backend/core/views.py
  • enterprise/backend/enterprise_core/views.py
  • frontend/src/lib/utils/table.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • backend/core/views.py
🧰 Additional context used
🧬 Code graph analysis (1)
enterprise/backend/enterprise_core/views.py (2)
backend/core/views.py (1)
  • filter_queryset (1147-1201)
backend/iam/models.py (1)
  • Role (809-824)
⏰ 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). (8)
  • GitHub Check: startup-docker-compose-test
  • GitHub Check: enterprise-startup-functional-test (3.12)
  • GitHub Check: startup-functional-test (3.12)
  • GitHub Check: enterprise-startup-docker-compose-test
  • GitHub Check: build_community_frontend
  • GitHub Check: build (3.12)
  • GitHub Check: build_enterprise_frontend
  • GitHub Check: test (3.12)
🔇 Additional comments (3)
frontend/src/lib/utils/table.ts (3)

121-129: LGTM - Well-structured filter configuration.

The new PARENT_DOMAIN_FILTER properly configures the AutocompleteSelect component to display hierarchical paths using the str field, which aligns with the PR's objective to show domains with full paths.


1206-1212: Backend already exposes the str field for folders

The changes correctly implement hierarchical path display using the str field. The backend's FolderReadSerializer (line 1419) explicitly exposes str = serializers.CharField(source="__str__", read_only=True), which sources from the Folder model's __str__() method that generates the full hierarchical path. The frontend change is properly supported and safe.


1215-1216: The backend correctly supports the str field for perimeters.

The PerimeterReadSerializer exposes the field via str = serializers.CharField(source="__str__", read_only=True) at line 269 of backend/core/serializers.py. The Perimeter model's __str__ method (line 2324-2325 of backend/core/models.py) returns the hierarchical path format f"{self.folder}/{self.name}", enabling the list view to display perimeter hierarchy correctly.

@nas-tabchiche
Copy link
Collaborator

@coderabbitai full review

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 31, 2025

✅ Actions performed

Full review triggered.

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

♻️ Duplicate comments (5)
enterprise/backend/enterprise_core/views.py (1)

253-270: Ensure RoleOrderingFilter runs last to avoid SearchFilter operating on a list

RoleOrderingFilter.filter_queryset converts the queryset to a Python list when ordering by name. With:

filter_backends = [DjangoFilterBackend, RoleOrderingFilter, filters.SearchFilter]

any request that uses both ordering=name and search=... will feed a list into SearchFilter, which expects a QuerySet and can blow up at runtime.

You can avoid this by making the ordering backend run last:

Proposed change
 class RoleViewSet(BaseModelViewSet):
@@
-    ordering = ["builtin", "name"]
-    ordering_fields = ["name"]
-    filter_backends = [DjangoFilterBackend, RoleOrderingFilter, filters.SearchFilter]
+    ordering = ["builtin", "name"]
+    ordering_fields = ["name"]
+    filter_backends = [DjangoFilterBackend, filters.SearchFilter, RoleOrderingFilter]

This keeps all filtering/search on QuerySets and only converts to a list at the very end when applying the display-name sort.

Also applies to: 277-281

backend/core/views.py (4)

5532-5557: UserGroupOrderingFilter + UserGroupViewSet: align filter_backends order with new path-aware behavior

Good call reusing PathAwareOrderingFilter via UserGroupOrderingFilter(path_fields = {"localization_dict"}).

However, UserGroupViewSet.filter_backends still runs UserGroupOrderingFilter before SearchFilter:

filter_backends = [
    DjangoFilterBackend,
    UserGroupOrderingFilter,
    filters.SearchFilter,
]

With the new path-aware implementation, this has the same issue as Perimeter/Folder: combining ?ordering=localization_dict with ?search=… will cause SearchFilter to receive a list instead of a QuerySet, which can break search and pagination.

Please reorder so that SearchFilter runs before the path-aware ordering filter.

Suggested filter_backends change for UserGroupViewSet
-    filter_backends = [
-        DjangoFilterBackend,
-        UserGroupOrderingFilter,
-        filters.SearchFilter,
-    ]
+    filter_backends = [
+        DjangoFilterBackend,
+        filters.SearchFilter,
+        UserGroupOrderingFilter,
+    ]
#!/bin/bash
# Sanity check combined search + path ordering on user groups
curl -sS 'http://localhost:8000/api/user-groups/?search=test&ordering=localization_dict' | head -c 200 || true

1136-1201: PathAwareOrderingFilter: normalize non-path sort keys to avoid TypeError; related_field attr is unused

The path-aware logic and use of get_folder_full_path look good, but:

  • For non-path fields, key_for_field returns the raw attribute value. If that attribute is a model instance (e.g. folder, perimeter, etc.), Python’s sort will attempt to compare model instances and can raise TypeError for multi-field orderings like ?ordering=str,folder or any ordering where a non-string FK is involved.
  • The class attribute related_field is not actually used; filter_queryset always derives related_field from view.path_related_field or defaults to "folder". Keeping the unused attribute is confusing.

I’d harden key_for_field so it always returns a comparable scalar for non-path fields (string-like), and optionally drop or repurpose the class-level related_field attribute to match the runtime behavior.

Suggested update for key_for_field
-        def key_for_field(obj, field):
-            if field not in path_fields:
-                value = getattr(obj, field, "")
-                return value.casefold() if isinstance(value, str) else value
+        def key_for_field(obj, field):
+            if field not in path_fields:
+                value = getattr(obj, field, "")
+                if isinstance(value, str):
+                    return value.casefold()
+                if value is None:
+                    return ""
+                # Handle date/datetime and similar objects
+                if hasattr(value, "isoformat"):
+                    try:
+                        return value.isoformat()
+                    except Exception:
+                        pass
+                # Fallback to string representation
+                return str(value).casefold()
             # Path-aware field
             path_key = full_path(obj).casefold()
             name_key = (getattr(obj, "name", "") or "").casefold()
             return (path_key, name_key)

Optionally, either remove the class attribute:

-    related_field: str | None = None  # override per view if needed

or change filter_queryset to honor it (e.g. getattr(view, "path_related_field", self.related_field or "folder")) so the attribute has an effect.

To be safe, please also run a couple of queries with mixed ordering to confirm no TypeError is raised at runtime:

#!/bin/bash
# Example smoke checks (adjust URLs as needed)
curl -sS 'http://localhost:8000/api/perimeters/?ordering=str,folder' | head -c 200 || true
curl -sS 'http://localhost:8000/api/perimeters/?ordering=folder,str' | head -c 200 || true

1209-1220: PerimeterViewSet: run SearchFilter before PathAwareOrderingFilter to keep search working with path-based ordering

With the current filter_backends order:

filter_backends = [
    DjangoFilterBackend,
    PathAwareOrderingFilter,
    filters.SearchFilter,
]

when a request uses both search and path ordering (e.g. ?search=foo&ordering=str), PathAwareOrderingFilter materializes the queryset to a list and returns it, so SearchFilter receives a list instead of a QuerySet. That breaks ORM-based filtering and can lead to 500s and double-pagination.

PathAwareOrderingFilter should be the last backend so that all ORM filters (including search) run while the data is still a QuerySet.

Suggested filter_backends reorder for PerimeterViewSet
-    filter_backends = [
-        DjangoFilterBackend,
-        PathAwareOrderingFilter,
-        filters.SearchFilter,
-    ]
+    filter_backends = [
+        DjangoFilterBackend,
+        filters.SearchFilter,
+        PathAwareOrderingFilter,
+    ]

This keeps search + ordering composable while still enabling Python-side path ordering on "str".

#!/bin/bash
# After reordering, these should both work without 500s and return path-ordered results:
curl -sS 'http://localhost:8000/api/perimeters/?search=test&ordering=str' | head -c 200 || true
curl -sS 'http://localhost:8000/api/perimeters/?ordering=-str' | head -c 200 || true

5606-5615: FolderViewSet: ensure SearchFilter runs before PathAwareOrderingFilter; fix or drop unused path_related_fields

Two points here:

  1. Filter backend order
    As with PerimeterViewSet and UserGroupViewSet, FolderViewSet currently runs PathAwareOrderingFilter before SearchFilter:

    filter_backends = [
        DjangoFilterBackend,
        PathAwareOrderingFilter,
        filters.SearchFilter,
    ]

    This can break ?search=…&ordering=str because SearchFilter may receive a list instead of a QuerySet. PathAwareOrderingFilter should be last.

  2. path_related_fields typo / dead config
    The view defines:

    path_related_fields = "parent_folder"

    but PathAwareOrderingFilter looks for view.path_related_field (singular). As written, this attribute has no effect and is potentially confusing. Since full_path() short-circuits for Folder instances by using str(obj), you probably don’t need any path_related_field override here at all.

Suggested tweaks for FolderViewSet
-    filter_backends = [
-        DjangoFilterBackend,
-        PathAwareOrderingFilter,
-        filters.SearchFilter,
-    ]
+    filter_backends = [
+        DjangoFilterBackend,
+        filters.SearchFilter,
+        PathAwareOrderingFilter,
+    ]
@@
-    path_ordering_fields = {"str"}
-    path_fields = {"str"}
-    path_related_fields = "parent_folder"
+    path_ordering_fields = {"str"}
+    path_fields = {"str"}
+    # No explicit path_related_field needed: Folder instances use their __str__ as the full path.
+    # If you ever want to override, use `path_related_field = "parent_folder"`.
#!/bin/bash
# Check that folder search + path ordering works and no 500 is raised
curl -sS 'http://localhost:8000/api/folders/?search=test&ordering=str' | head -c 200 || true
🧹 Nitpick comments (2)
frontend/tests/functional/detailed/ebios-rm.test.ts (1)

591-605: Replace fixed waitForTimeout with a condition-based wait

Using a hard-coded waitForTimeout(3000) adds 3s to every run and can still be flaky if the form is slower or faster. It’s more robust to wait on the form being usable.

Consider wrapping the fill in a retrying expectation instead of sleeping:

Proposed change
	await test.step('workshop 5', async () => {
		await page.getByRole('button', { name: ' Step 1 Generate the risk' }).click();
-		await page.waitForTimeout(3000);
-		await riskAssessmentsPage.form.fill({
-			name: 'test-risk-assessment-ebios-rm',
-			perimeter: `${vars.folderName}/${vars.perimeterName}`
-		});
+		await expect(async () => {
+			await riskAssessmentsPage.form.fill({
+				name: 'test-risk-assessment-ebios-rm',
+				perimeter: `${vars.folderName}/${vars.perimeterName}`
+			});
+		}).toPass({ timeout: 80_000, intervals: [500, 1000, 2000] });
backend/core/models.py (1)

2324-2325: Perimeter string now uses folder full path; consider deduping in RiskAssessment.path_display

Using f"{self.folder}/{self.name}" is consistent with Folder’s new hierarchical __str__ and matches the “show full path” goal. No correctness issues.

One side effect: RiskAssessment.path_display currently does:

return f"{self.perimeter.folder}/{self.perimeter}/{self.name} - {self.version}"

With the new Folder.__str__ and Perimeter.__str__, this now repeats the folder path twice (folder path, then folder path/perimeter). You may want to simplify it to avoid duplication, e.g. drop the explicit self.perimeter.folder and rely on self.perimeter alone, or build the path from a single canonical helper.

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f92bd23 and 1f8353a.

📒 Files selected for processing (10)
  • backend/core/models.py
  • backend/core/serializers.py
  • backend/core/views.py
  • backend/iam/models.py
  • enterprise/backend/enterprise_core/views.py
  • frontend/src/lib/components/DetailView/DetailView.svelte
  • frontend/src/lib/utils/crud.ts
  • frontend/src/lib/utils/table.ts
  • frontend/tests/functional/detailed/ebios-rm.test.ts
  • frontend/tests/utils/test-utils.ts
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-12-29T15:52:09.213Z
Learnt from: melinoix
Repo: intuitem/ciso-assistant-community PR: 3107
File: frontend/src/routes/(app)/(internal)/recap/+page.svelte:49-51
Timestamp: 2025-12-29T15:52:09.213Z
Learning: In the recap page (frontend/src/routes/(app)/(internal)/recap/+page.svelte), perimeter.folder is guaranteed to be present when perimeter.compliance_assessments exist, so null safety checks are not necessary for perimeter.folder.str in that context.

Applied to files:

  • frontend/tests/utils/test-utils.ts
  • frontend/src/lib/components/DetailView/DetailView.svelte
📚 Learning: 2025-09-05T09:18:16.194Z
Learnt from: Mohamed-Hacene
Repo: intuitem/ciso-assistant-community PR: 2472
File: backend/iam/models.py:906-907
Timestamp: 2025-09-05T09:18:16.194Z
Learning: In the get_accessible_object_ids method in backend/iam/models.py, for objects with parent_folder attribute (like Folder objects), the folder association logic treats each folder as being associated with itself, so the mapping is folder_id → folder_id. The code `[(fid, fid) for fid in folder_permissions.keys()]` correctly implements this self-referential mapping rather than querying for child folders.

Applied to files:

  • backend/iam/models.py
  • backend/core/views.py
📚 Learning: 2025-08-25T08:51:15.404Z
Learnt from: Mohamed-Hacene
Repo: intuitem/ciso-assistant-community PR: 2422
File: backend/core/serializers.py:1018-1030
Timestamp: 2025-08-25T08:51:15.404Z
Learning: The CISO Assistant project uses a custom permission system where RoleAssignment.get_accessible_object_ids() provides special handling for Permission objects by filtering them by content_type app_label rather than folder hierarchy, since Permission objects don't belong to folders. This allows safe CRUD operations on permissions while preventing privilege escalation by restricting access to only application-specific permissions from allowed apps: "core", "ebios_rm", "tprm", "privacy", "resilience", and "cal".

Applied to files:

  • backend/core/views.py
🧬 Code graph analysis (3)
enterprise/backend/enterprise_core/views.py (1)
backend/iam/models.py (1)
  • Role (809-824)
backend/iam/models.py (1)
backend/core/models.py (1)
  • get_folder_full_path (5454-5455)
backend/core/views.py (1)
backend/iam/models.py (3)
  • Folder (80-332)
  • get_folder_full_path (158-166)
  • get_folder_full_path (347-351)
🔇 Additional comments (7)
frontend/tests/utils/test-utils.ts (1)

551-561: Perimeter str test data aligns with new path-based display

Adding str: \${vars.folderName}/${vars.perimeterName}`toperimetersPage.buildmatches the new backendPerimeter.str/str` field and existing patterns (e.g., risk assessments, asset assessments). This should help tests assert full paths without affecting form filling.

frontend/src/lib/utils/crud.ts (1)

214-232: Simplifying risk-assessments selectFields to status only looks fine

Dropping risk_tolerance from selectFields and keeping just { field: 'status' } matches the UI filters (which only use status here) and avoids fetching unused select metadata. Just ensure no remaining frontend code expects risk_tolerance to be pre-populated from this map.

backend/iam/models.py (1)

134-139: Path-based __str__ for Folder/UserGroup is correct and consistent

  • Folder.__str__ builds a /-joined path from get_folder_full_path(include_root=False), falling back cleanly to self.name/"" if needed, without recursive __str__ calls.
  • UserGroup.__str__ and get_localization_dict now rely on self.folder’s string form, so group labels/localization will show the full domain path, matching the new UI expectations.

Also applies to: 386-400

frontend/src/lib/utils/table.ts (1)

121-129: Using str and PARENT_DOMAIN_FILTER for domains/perimeters matches new path display

  • PARENT_DOMAIN_FILTER correctly hits the folders endpoint, displaying options via the str path while keeping values as IDs.
  • folders and perimeters list configs now use str in body so the “name” column shows the full path, while filters (parent_folder, folder) still work off IDs.

This lines up with the backend exposing str for folders/perimeters.

Also applies to: 1205-1217

backend/core/serializers.py (2)

269-269: LGTM! Exposing hierarchical path for perimeters.

The addition of the str field correctly exposes the Perimeter model's __str__ method, which now returns the hierarchical display path (folder path + perimeter name). This supports the PR objective to show domains with full path and enables path-based sorting in the frontend.


1419-1419: LGTM! Exposing hierarchical path for folders.

The addition of the str field correctly exposes the Folder model's __str__ method to return the hierarchical display path. This change is consistent with the PerimeterReadSerializer (line 269) and aligns with the PR objective to show full paths for domains and support proper sorting.

frontend/src/lib/components/DetailView/DetailView.svelte (1)

438-440: LGTM! Hierarchical path rendering implemented correctly.

The conditional shortcut appropriately prioritizes data.data.str (the full hierarchical path from the backend) over the simple value for folder name fields. The null-safety check with optional chaining and the fallback to existing rendering logic are both correct.

This aligns with the PR objective to show domains with their full path.

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: 2

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)

1068-1080: Import deque before using it in _get_optimized_object_data

_get_optimized_object_data uses deque but only defaultdict is imported from collections at line 40. This will raise NameError: name 'deque' is not defined when this code path runs.

Suggested import fix
-from collections import defaultdict
+from collections import defaultdict, deque
♻️ Duplicate comments (2)
backend/core/views.py (2)

1136-1201: Normalize non‑path sort keys to comparable scalars to avoid TypeError in multi‑field ordering

In key_for_field, non‑path fields currently return the raw attribute value. If a client uses multi‑field ordering that mixes a path field with a non‑path FK (e.g. ?ordering=str,folder for PerimeterViewSet or similar patterns elsewhere), Python will try to compare model instances during the secondary sort and can raise TypeError: '<' not supported between instances ....

Normalizing non‑path values to scalars (strings / ISO timestamps / empty for None) makes the sort stable and future‑proof without changing semantics for existing simple cases.

Suggested fix for key_for_field
     # Key function for sorting
     def key_for_field(obj, field):
         if field not in path_fields:
-            value = getattr(obj, field, "")
-            return value.casefold() if isinstance(value, str) else value
+            value = getattr(obj, field, "")
+            if isinstance(value, str):
+                return value.casefold()
+            if value is None:
+                return ""
+            # Handle date/datetime and similar objects
+            if hasattr(value, "isoformat"):
+                try:
+                    return value.isoformat()
+                except Exception:
+                    pass
+            # Fallback to string representation
+            return str(value).casefold()
         # Path-aware field
         path_key = full_path(obj).casefold()
         name_key = (getattr(obj, "name", "") or "").casefold()
         return (path_key, name_key)

You might also consider either wiring the related_field attribute into filter_queryset or removing it from PathAwareOrderingFilter to avoid confusion, since the implementation now exclusively uses view.path_related_field (with a default of "folder").


1211-1220: Ensure SearchFilter runs before path‑aware ordering to keep search + ordering working together

PathAwareOrderingFilter.filter_queryset materializes the queryset to a Python list and returns it. With the current filter_backends order:

  • PerimeterViewSet: [DjangoFilterBackend, PathAwareOrderingFilter, filters.SearchFilter]
  • UserGroupViewSet: [DjangoFilterBackend, UserGroupOrderingFilter, filters.SearchFilter]
  • FolderViewSet: [DjangoFilterBackend, PathAwareOrderingFilter, filters.SearchFilter]

any request that combines search with a path field ordering (e.g. ?ordering=str&search=foo) will pass a list into SearchFilter.filter_queryset, which then calls .filter() and will fail at runtime. Path‑aware ordering should be the last backend so that all ORM‑based filters operate on a real QuerySet.

Suggested filter_backends reordering
 class PerimeterViewSet(BaseModelViewSet):
@@
-    filter_backends = [
-        DjangoFilterBackend,
-        PathAwareOrderingFilter,
-        filters.SearchFilter,
-    ]
+    filter_backends = [
+        DjangoFilterBackend,
+        filters.SearchFilter,
+        PathAwareOrderingFilter,
+    ]
@@
 class UserGroupViewSet(BaseModelViewSet):
@@
-    filter_backends = [
-        DjangoFilterBackend,
-        UserGroupOrderingFilter,
-        filters.SearchFilter,
-    ]
+    filter_backends = [
+        DjangoFilterBackend,
+        filters.SearchFilter,
+        UserGroupOrderingFilter,
+    ]
@@
 class FolderViewSet(BaseModelViewSet):
@@
-    filter_backends = [
-        DjangoFilterBackend,
-        PathAwareOrderingFilter,
-        filters.SearchFilter,
-    ]
+    filter_backends = [
+        DjangoFilterBackend,
+        filters.SearchFilter,
+        PathAwareOrderingFilter,
+    ]

This keeps behavior unchanged for pure ordering, but avoids search failures and keeps pagination semantics intact because only the final backend returns a list.

Also applies to: 5532-5557, 5606-5610

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

5613-5615: Fix unused/misnamed path_related_fields on FolderViewSet

PathAwareOrderingFilter reads view.path_related_field (singular), but FolderViewSet defines path_related_fields = "parent_folder". As written, this attribute is ignored; Folder ordering still works only because full_path() special‑cases Folder instances.

Either rename or drop it for clarity:

  • If you want to hint the filter to follow parent_folder for select‑related:
-    path_related_fields = "parent_folder"
+    path_related_field = "parent_folder"
  • Or remove the attribute entirely if it’s not needed.
📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1f8353a and f4169df.

📒 Files selected for processing (5)
  • backend/core/models.py
  • backend/core/serializers.py
  • backend/core/views.py
  • frontend/src/lib/utils/crud.ts
  • frontend/src/lib/utils/table.ts
🚧 Files skipped from review as they are similar to previous changes (2)
  • frontend/src/lib/utils/table.ts
  • backend/core/models.py
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-05T09:18:16.194Z
Learnt from: Mohamed-Hacene
Repo: intuitem/ciso-assistant-community PR: 2472
File: backend/iam/models.py:906-907
Timestamp: 2025-09-05T09:18:16.194Z
Learning: In the get_accessible_object_ids method in backend/iam/models.py, for objects with parent_folder attribute (like Folder objects), the folder association logic treats each folder as being associated with itself, so the mapping is folder_id → folder_id. The code `[(fid, fid) for fid in folder_permissions.keys()]` correctly implements this self-referential mapping rather than querying for child folders.

Applied to files:

  • backend/core/views.py
📚 Learning: 2025-08-25T08:51:15.404Z
Learnt from: Mohamed-Hacene
Repo: intuitem/ciso-assistant-community PR: 2422
File: backend/core/serializers.py:1018-1030
Timestamp: 2025-08-25T08:51:15.404Z
Learning: The CISO Assistant project uses a custom permission system where RoleAssignment.get_accessible_object_ids() provides special handling for Permission objects by filtering them by content_type app_label rather than folder hierarchy, since Permission objects don't belong to folders. This allows safe CRUD operations on permissions while preventing privilege escalation by restricting access to only application-specific permissions from allowed apps: "core", "ebios_rm", "tprm", "privacy", "resilience", and "cal".

Applied to files:

  • backend/core/views.py
⏰ 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: test (3.12)
  • GitHub Check: build_community_frontend
  • GitHub Check: build_enterprise_frontend
  • GitHub Check: enterprise-startup-docker-compose-test
  • GitHub Check: startup-docker-compose-test
  • GitHub Check: startup-functional-test (3.12)
  • GitHub Check: enterprise-startup-functional-test (3.12)
  • GitHub Check: build (3.12)
  • GitHub Check: Analyze (python)
🔇 Additional comments (1)
backend/core/serializers.py (1)

269-269: LGTM! The str field is properly optimized.

The addition of the str field to expose the Perimeter's string representation follows the established pattern in the codebase and is safe from query optimization issues. Perimeter's __str__() method (inherited from NameDescriptionMixin) simply returns self.name without traversing relationships. Additionally, the PathAwareOrderingFilter already handles optimization by calling select_related("folder") when the "str" field is used for ordering, which aligns with the view's path_fields = {"str"} configuration.

filtering_labels = FieldsRelatedField(many=True)

content_type = serializers.CharField(source="get_content_type_display")
str = serializers.CharField(source="__str__", read_only=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# First, let's verify the FolderReadSerializer change and context
cat -n backend/core/serializers.py | sed -n '1410,1430p'

Repository: intuitem/ciso-assistant-community

Length of output: 809


🏁 Script executed:

# Check the Folder model and its __str__ method
rg -A 15 "class Folder" backend/core/models.py | head -40

Repository: intuitem/ciso-assistant-community

Length of output: 59


🏁 Script executed:

# Check the FolderViewSet implementation
rg -A 30 "class FolderViewSet" backend/core/views.py

Repository: intuitem/ciso-assistant-community

Length of output: 1080


🏁 Script executed:

# Also check PerimeterReadSerializer for comparison
rg -B 5 -A 10 "class PerimeterReadSerializer" backend/core/serializers.py

Repository: intuitem/ciso-assistant-community

Length of output: 536


🏁 Script executed:

# Check Folder model __str__ method
rg -A 10 "class Folder" backend/core/models.py | grep -A 20 "def __str__"

Repository: intuitem/ciso-assistant-community

Length of output: 59


🏁 Script executed:

# Check FolderViewSet.get_queryset() for query optimization
rg -A 40 "class FolderViewSet" backend/core/views.py | grep -A 20 "get_queryset"

Repository: intuitem/ciso-assistant-community

Length of output: 59


🏁 Script executed:

# Check PathAwareOrderingFilter implementation
rg -B 5 -A 30 "class PathAwareOrderingFilter" backend/core/

Repository: intuitem/ciso-assistant-community

Length of output: 2298


🏁 Script executed:

# Also check if there's any prefetch_related in the base viewset
rg -A 20 "class BaseModelViewSet" backend/core/views.py | head -40

Repository: intuitem/ciso-assistant-community

Length of output: 714


🏁 Script executed:

# Find the Folder model file location
find backend -name "*.py" -type f | xargs grep -l "^class Folder"

Repository: intuitem/ciso-assistant-community

Length of output: 148


🏁 Script executed:

# Get Folder model definition with __str__
rg "class Folder\(" backend/core/models.py -A 100 | head -120

Repository: intuitem/ciso-assistant-community

Length of output: 59


🏁 Script executed:

# Check if FolderViewSet overrides get_queryset
rg "class FolderViewSet" backend/core/views.py -A 100 | grep -A 15 "def get_queryset"

Repository: intuitem/ciso-assistant-community

Length of output: 59


🏁 Script executed:

# Search for Folder model definition more broadly
rg "^class Folder" backend/

Repository: intuitem/ciso-assistant-community

Length of output: 550


🏁 Script executed:

# Check what's in backend/core/models.py for Folder
rg "class Folder" backend/core/models.py -A 80

Repository: intuitem/ciso-assistant-community

Length of output: 59


🏁 Script executed:

# Check the import structure
head -50 backend/core/models.py

Repository: intuitem/ciso-assistant-community

Length of output: 1640


🏁 Script executed:

# Get the Folder model from iam/models.py
rg "class Folder\(" backend/iam/models.py -A 50

Repository: intuitem/ciso-assistant-community

Length of output: 1456


🏁 Script executed:

# Check if Folder has __str__ method
rg "class Folder\(" backend/iam/models.py -A 100 | grep -A 5 "def __str__"

Repository: intuitem/ciso-assistant-community

Length of output: 388


🏁 Script executed:

# Also check NameDescriptionMixin to see if __str__ is there
rg "class NameDescriptionMixin" backend/ -A 20

Repository: intuitem/ciso-assistant-community

Length of output: 1191


🏁 Script executed:

# Check the get_folder_full_path method
rg "def get_folder_full_path" backend/iam/models.py -A 20

Repository: intuitem/ciso-assistant-community

Length of output: 1671


🏁 Script executed:

# Check if there's a property or method that might be caching the path
rg "get_folder_full_path\|folder_path\|get_path" backend/iam/models.py

Repository: intuitem/ciso-assistant-community

Length of output: 59


🏁 Script executed:

# Check what the full Folder model looks like, especially any properties
rg "class Folder\(" backend/iam/models.py -A 150 | tail -100

Repository: intuitem/ciso-assistant-community

Length of output: 4107


🏁 Script executed:

# Let's check if FolderViewSet has a custom get_queryset method
rg "class FolderViewSet" backend/core/views.py -A 200 | grep -B 5 -A 20 "def get_queryset"

Repository: intuitem/ciso-assistant-community

Length of output: 59


🏁 Script executed:

# Check if there's any prefetch_related or select_related in the base class or viewset
rg "class FolderViewSet" backend/core/views.py -A 200 | grep -E "prefetch|select_related"

Repository: intuitem/ciso-assistant-community

Length of output: 59


🏁 Script executed:

# Verify the exact attribute name used in PathAwareOrderingFilter
rg "path_related_field" backend/core/views.py

Repository: intuitem/ciso-assistant-community

Length of output: 188


🏁 Script executed:

# Let's check the full FolderViewSet implementation to see if there's any queryset optimization we missed
cat -n backend/core/views.py | grep -A 80 "class FolderViewSet"

Repository: intuitem/ciso-assistant-community

Length of output: 3472


🏁 Script executed:

# Check if BaseModelViewSet has any get_queryset override
cat -n backend/core/views.py | grep -B 5 -A 30 "class BaseModelViewSet"

Repository: intuitem/ciso-assistant-community

Length of output: 1354


🏁 Script executed:

# Get the full get_queryset method from BaseModelViewSet
cat -n backend/core/views.py | sed -n '631,700p'

Repository: intuitem/ciso-assistant-community

Length of output: 3951


🏁 Script executed:

# Check if there's any queryset optimization specific to the Folder model
rg "Folder.objects" backend/core/views.py | head -20

Repository: intuitem/ciso-assistant-community

Length of output: 1398


🏁 Script executed:

# Let me verify the attribute name discrepancy more carefully
rg "path_related_field" backend/core/views.py -B 2 -A 2

Repository: intuitem/ciso-assistant-community

Length of output: 483


🏁 Script executed:

# Check if the FolderViewSet's attribute name matches what PathAwareOrderingFilter expects
rg "path_related_field\|path_related_fields" backend/core/views.py

Repository: intuitem/ciso-assistant-community

Length of output: 59


🏁 Script executed:

# Let's also check if there are any other viewsets that use this pattern correctly
rg "path_related_field" backend/core/views.py -B 10 | grep -E "class \w+ViewSet|path_related"

Repository: intuitem/ciso-assistant-community

Length of output: 188


🏁 Script executed:

# Check the Folder model fields to see if it has a "folder" field
rg "class Folder\(" backend/iam/models.py -A 80 | grep -E "^\s+(parent_folder|folder)\s*="

Repository: intuitem/ciso-assistant-community

Length of output: 115


🏁 Script executed:

# Verify that Folder model doesn't have a "folder" field
rg "^\s+folder\s*=" backend/iam/models.py

Repository: intuitem/ciso-assistant-community

Length of output: 573


🏁 Script executed:

# Let me look at the complete PathAwareOrderingFilter implementation
cat -n backend/core/views.py | sed -n '555,600p'

Repository: intuitem/ciso-assistant-community

Length of output: 1936


🏁 Script executed:

# Find the complete PathAwareOrderingFilter implementation
rg "class PathAwareOrderingFilter" backend/core/views.py -A 50

Repository: intuitem/ciso-assistant-community

Length of output: 2245


LGTM! Verify and fix query optimization for folder list views.

The addition of the str field to expose the Folder's string representation is consistent with the codebase. However, there is a genuine N+1 query concern:

The Folder.__str__() method traverses parent_folder relationships to build a hierarchical path. When serializing folder lists, each folder will trigger separate queries to fetch its parent folders. Additionally, there's an attribute name mismatch: FolderViewSet declares path_related_fields = "parent_folder" (plural), but PathAwareOrderingFilter looks for path_related_field (singular), preventing the filter's select_related() optimization from being applied.

To fix this, either:

  1. Rename path_related_fields to path_related_field in FolderViewSet, or
  2. Add explicit prefetch_related("parent_folder") to FolderViewSet.get_queryset()
🤖 Prompt for AI Agents
In backend/core/serializers.py around line 1419, adding the str field exposes
Folder.__str__ which walks parent_folder and causes N+1 queries when serializing
lists; fix FolderViewSet so the controller-side query optimization applies by
either renaming the attribute path_related_fields to path_related_field
(singular) to match PathAwareOrderingFilter so it can call select_related(...),
or explicitly add prefetch_related("parent_folder") (or select_related if single
FK) inside FolderViewSet.get_queryset() to eagerly load parents and avoid
per-object DB hits.

],
reverseForeignKeyFields: [{ field: 'risk_assessment', urlModel: 'risk-scenarios' }],
selectFields: [{ field: 'status' }, { field: 'risk_tolerance', valueType: 'number' }],
selectFields: [{ field: 'status' }],
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

# First, let's examine the actual file and the selectFields definition for risk-assessments
cat -n frontend/src/lib/utils/crud.ts | sed -n '220,240p'

Repository: intuitem/ciso-assistant-community

Length of output: 1191


🏁 Script executed:

# Search for all references to risk_tolerance in the entire codebase
echo "=== All risk_tolerance references ==="
rg -n 'risk_tolerance' --type=ts --type=js --type=svelte

Repository: intuitem/ciso-assistant-community

Length of output: 162


🏁 Script executed:

# Search for risk-assessments model definition to understand context
echo "=== Risk assessments model definition ==="
rg -B5 -A10 "'risk-assessments'" frontend/src/lib/utils/crud.ts

Repository: intuitem/ciso-assistant-community

Length of output: 5671


🏁 Script executed:

# Check if risk_tolerance is used in any components or forms
echo "=== Searching for risk_tolerance in component code ==="
fd -e svelte -e ts -e js . frontend/src -x grep -l 'risk_tolerance' {} \;

Repository: intuitem/ciso-assistant-community

Length of output: 432


🏁 Script executed:

# Check how risk_tolerance is used in RiskAssessmentForm.svelte
echo "=== RiskAssessmentForm.svelte usage ==="
grep -n 'risk_tolerance' frontend/src/lib/components/Forms/ModelForm/RiskAssessmentForm.svelte -C3

Repository: intuitem/ciso-assistant-community

Length of output: 433


🏁 Script executed:

# Check how risk_tolerance is defined in schemas.ts
echo "=== risk_tolerance in schemas.ts ==="
grep -n 'risk_tolerance' frontend/src/lib/utils/schemas.ts -C3

Repository: intuitem/ciso-assistant-community

Length of output: 763


🏁 Script executed:

# Check the crud.ts file to see if there are other selectFields definitions or if this was previously set
echo "=== Check if risk_tolerance appears elsewhere in crud.ts ==="
grep -n 'risk_tolerance' frontend/src/lib/utils/crud.ts -C3

Repository: intuitem/ciso-assistant-community

Length of output: 444


🏁 Script executed:

# Check QuantitativeRiskStudyForm for context
echo "=== QuantitativeRiskStudyForm.svelte usage ==="
grep -n 'risk_tolerance' frontend/src/lib/components/Forms/ModelForm/QuantitativeRiskStudyForm.svelte -C3

Repository: intuitem/ciso-assistant-community

Length of output: 2536


Restore risk_tolerance to selectFields for risk-assessments—this removal breaks the form.

The risk_tolerance field has been removed from the selectFields array at line 231, but it is actively used in RiskAssessmentForm.svelte (line 125) to display and edit risk tolerance data. The selectFields array controls which fields are included in API SELECT queries; removing it means the field will not be fetched, leaving the form with undefined data and breaking risk assessment edits.

The field is defined in the schema as risk_tolerance: z.number().optional().default(-1) and is essential for the risk assessment form. Restore it to:

selectFields: [{ field: 'status' }, { field: 'risk_tolerance' }],
🤖 Prompt for AI Agents
In frontend/src/lib/utils/crud.ts around line 231, the selectFields array no
longer includes risk_tolerance which causes RiskAssessmentForm.svelte (line 125)
to receive undefined for that field; restore risk_tolerance to the SELECT list
so the API returns it. Update the selectFields entry to include both status and
risk_tolerance (i.e., ensure the array contains { field: 'status' } and { field:
'risk_tolerance' }) so the form can read and edit the value.

@eric-intuitem eric-intuitem marked this pull request as draft December 31, 2025 16:47
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.

4 participants