feat: Choose the domain of applied controls created from an audit #2784
Conversation
WalkthroughThe changes enforce keyword-only parameter usage for Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
backend/core/views.py (1)
3628-3647: Optimize subdomains for N+1 avoidance and deterministic ordering
- Serializer likely triggers extra queries (path/parent_folder/labels). Prefetch and order for stable output.
- Optional: use get_object_or_404 for brevity without changing RBAC behavior.
Apply:
@action(detail=True, methods=["get"]) def subdomains(self, request, pk): """ Returns a list composed of the given domain and all its subdomains """ - instance = Folder.objects.filter(pk=pk).first() - if not instance: - return Response(status=status.HTTP_404_NOT_FOUND) + instance = get_object_or_404(Folder, pk=pk) if not RoleAssignment.is_access_allowed( user=request.user, perm=Permission.objects.get(codename="view_folder"), folder=instance, ): return Response(status=status.HTTP_403_FORBIDDEN) - subfolders = list(instance.get_sub_folders(include_self=True)) - serializer = FolderReadSerializer(subfolders, many=True) + subfolder_ids = [f.id for f in instance.get_sub_folders(include_self=True)] + subfolders_qs = ( + Folder.objects + .filter(id__in=subfolder_ids) + .select_related("parent_folder") + .prefetch_related("filtering_labels") + .order_by(Lower("name")) + ) + serializer = FolderReadSerializer(subfolders_qs, many=True) return Response(serializer.data)backend/iam/models.py (2)
336-341: FolderMixin.get_folder_full_path parity and return shapeLGTM and consistent with Folder.get_folder_full_path. Minor: consider mirroring the docstring here for clarity, and add a small test covering include_root True/False.
Do you want a tiny pytest to lock behavior for include_root?
125-133: Clarify generator behavior and include_self semanticsDocstring says "list" but the method yields a generator in depth‑first (pre‑order). Update the docstring to state it yields and to document include_self semantics; callers in the repo iterate/convert to list and do not rely on list indexing/len.
File: backend/iam/models.py (around lines 125–139)
- def get_sub_folders( - self, *, include_self: bool = False - ) -> Generator[Self, None, None]: - """Return the list of subfolders""" + def get_sub_folders( + self, *, include_self: bool = False + ) -> Generator[Self, None, None]: + """Yield subfolders in depth-first order (pre-order). + Set include_self=True to yield the folder itself first. + """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/core/models.py(1 hunks)backend/core/views.py(1 hunks)backend/iam/models.py(3 hunks)frontend/src/lib/components/Forms/ModelForm/AppliedControlPolicyForm.svelte(1 hunks)frontend/src/params/fields.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
backend/core/views.py (2)
backend/iam/models.py (4)
Folder(68-321)RoleAssignment(812-1099)is_access_allowed(842-862)get_sub_folders(125-138)backend/core/serializers.py (1)
FolderReadSerializer(1253-1262)
backend/core/models.py (1)
backend/iam/models.py (3)
get_folder_full_path(147-155)get_folder_full_path(336-340)Folder(68-321)
backend/iam/models.py (1)
backend/core/models.py (1)
get_folder_full_path(4792-4793)
🔇 Additional comments (4)
frontend/src/params/fields.ts (1)
15-17: LGTM! Clear initialization for subdomain route support.The Set initialization with 'subdomains' properly supports the new endpoint pattern. The inline comment effectively explains the purpose.
frontend/src/lib/components/Forms/ModelForm/AppliedControlPolicyForm.svelte (1)
346-346: Confirm removal of content_type filters is intentional by verifying against feature requirements.The current
subdomainsendpoint returns all subfolder types (DOMAIN, ENCLAVE, ROOT) without filtering. The previous implementation filtered to only DOMAIN (DO) and ROOT (GL) types. Verify this change aligns with the intended behavior for the Applied Control Policy form—specifically, whether all subfolder types should be selectable or only specific ones.backend/iam/models.py (1)
147-156: No compatibility issue found—all existing calls already use keyword arguments or defaults.Verification confirmed that all four actual calls to
get_folder_full_path()in the codebase either passinclude_rootas a keyword argument or rely on the default. There are zero positional callers likeget_folder_full_path(True)that would break with this signature.backend/core/models.py (1)
4792-4793: Keyword-only alignment verified—no positional calls foundSearch across the codebase confirmed all existing calls to
get_folder_full_path()use either no arguments or keyword arguments. The delegation inRiskScenario.get_folder_full_path()correctly forwards the parameter as a keyword argument (include_root=include_root). No runtime breakage risk.
frontend/src/lib/components/Forms/ModelForm/AppliedControlPolicyForm.svelte
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frontend/src/lib/components/Forms/ModelForm.svelte(3 hunks)frontend/src/lib/components/Forms/ModelForm/AppliedControlPolicyForm.svelte(4 hunks)frontend/src/lib/components/Modals/CreateModal.svelte(3 hunks)frontend/src/routes/(app)/(third-party)/requirement-assessments/[id=uuid]/edit/+page.svelte(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). (8)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: test (3.12)
- GitHub Check: build_community_frontend
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: build_enterprise_frontend
- GitHub Check: startup-docker-compose-test
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: build (3.12)
🔇 Additional comments (7)
frontend/src/routes/(app)/(third-party)/requirement-assessments/[id=uuid]/edit/+page.svelte (1)
70-90: LGTM! Origin parameter correctly identifies the requirement-assessments context.The addition of
origin: 'requirement-assessments'properly propagates context to the modal form, enabling origin-specific endpoint selection downstream.frontend/src/lib/components/Modals/CreateModal.svelte (2)
18-53: LGTM! Origin prop correctly defined and defaulted.The optional
originprop with a sensible null default enables context-aware form behavior without breaking existing usage.
81-99: LGTM! Origin properly forwarded to ModelForm.The prop is correctly threaded to the child component for downstream consumption.
frontend/src/lib/components/Forms/ModelForm/AppliedControlPolicyForm.svelte (2)
19-39: LGTM! Origin prop correctly defined.The prop signature and default value are consistent with the parent components.
352-360: LGTM! Dynamic endpoint correctly wired to AutocompleteSelect.The
domainOptionEndpointproperly replaces the static endpoint, enabling context-aware domain selection.frontend/src/lib/components/Forms/ModelForm.svelte (2)
90-130: LGTM! Origin prop correctly integrated.The prop is properly typed, defaulted, and consistent with the component hierarchy.
364-375: LGTM! Origin correctly forwarded to AppliedControlsPoliciesForm.The prop propagation completes the flow from the requirement-assessments page to the nested form component.
frontend/src/lib/components/Forms/ModelForm/AppliedControlPolicyForm.svelte
Outdated
Show resolved
Hide resolved
nas-tabchiche
left a comment
nas-tabchiche
left a comment
There was a problem hiding this comment.
The scope_folder_id url parameter should be enough to address this.
I am quite confident something like
<AutocompleteSelect
{form}
optionsEndpoint="folders?content_type=DO&content_type=GL"
optionsDetailedUrlParameters={[
origin === 'requirement-assessments' ? ['scope_folder_id', initialData.folder] : ['', undefined]
]}
field="folder"
pathField="path"
cacheLock={cacheLocks['folder']}
bind:cachedValue={formDataCache['folder']}
label={m.domain()}
/>would work the same as the proposed implementation
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/iam/models.py(2 hunks)frontend/src/lib/components/Forms/ModelForm/AppliedControlPolicyForm.svelte(3 hunks)
🧰 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 (1)
backend/iam/models.py (1)
backend/core/models.py (1)
get_folder_full_path(4890-4891)
⏰ 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_enterprise_frontend
- GitHub Check: build_community_frontend
- GitHub Check: test (3.12)
- GitHub Check: build (3.12)
- 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: Analyze (python)
🔇 Additional comments (3)
frontend/src/lib/components/Forms/ModelForm/AppliedControlPolicyForm.svelte (1)
28-28: LGTM! Origin prop correctly defined.The
originprop is properly typed and initialized with a sensible default value.Also applies to: 41-41
backend/iam/models.py (2)
331-331: Keyword-only enforcement verified—all callers compliant.All four callsites of
get_folder_full_path()use keyword arguments or default values:
serializers.py:121: no arguments (default)models.py:4891:include_root=include_rootviews.py:4009:include_root=Falsecustom_middleware.py:43: no arguments (default)The signature change at line 331 mirrors
Folder.get_folder_full_path()at line 142 and maintains consistency across the mixin hierarchy. No breaking changes detected.
142-142: Good refactor for API clarity – verification confirms all callers are compatible.Making
include_rootkeyword-only improves call-site readability and prevents positional argument misuse. This aligns with the pattern established inRiskScenario.get_folder_full_path.Verification shows all existing callers are compatible with the change:
backend/core/serializers.pyandbackend/core/custom_middleware.pycall with no argumentsbackend/core/views.pyandbackend/core/models.pyexplicitly use keyword argumentsNo positional argument usage detected in the codebase.
frontend/src/lib/components/Forms/ModelForm/AppliedControlPolicyForm.svelte
Show resolved
Hide resolved
nas-tabchiche
deleted the
feat/choose_domain_for_controls_created_from_audits
branch
Summary by CodeRabbit
Bug Fixes
New Features
Refactor