feat: Choose the domain of applied controls created from an audit #2784
Conversation
WalkthroughAdds a folder "subdomains" API action, enforces keyword-only parameters for folder path/subfolder methods, and updates frontend forms to use the new subdomains endpoint and propagate an optional Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as FolderViewSet
participant RBAC
participant Folder as FolderModel
participant Serializer
Client->>API: GET /folders/{pk}/subdomains/
API->>API: retrieve Folder(pk)
API->>RBAC: check view_folder permission
alt permission denied
RBAC-->>Client: 403 Forbidden
else permission granted
RBAC-->>API: OK
API->>Folder: get_sub_folders(include_self=True)
Folder-->>API: yields folders (generator)
API->>Serializer: serialize folder list
Serializer-->>Client: 200 OK (JSON)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code graph analysis (1)backend/core/models.py (1)
⏰ 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)
🔇 Additional comments (5)
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
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
Summary by CodeRabbit
New Features
Refactor