Ca 1456 Remove fieldsrelatedfields from codebase #3101
Conversation
📝 WalkthroughWalkthroughA large-scale refactoring renames Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant DetailView as DetailView Component
participant RelatedNames as Related-Names Utils
participant API as Backend API
participant Cache as relatedNames Cache
User->>DetailView: Mount component with related IDs
DetailView->>RelatedNames: Call refreshRelatedNames()
RelatedNames->>RelatedNames: Extract unique IDs from data<br/>Group by URL model
RelatedNames->>Cache: Check cached results
Note over RelatedNames,Cache: Deduplicate, filter to UUIDs
RelatedNames->>API: Fetch missing names<br/>(batch request, ≤100 IDs)
API-->>RelatedNames: Return ID→name mappings
RelatedNames->>Cache: Merge & store results
DetailView->>Cache: Query cached labels
Cache-->>DetailView: Return display names
DetailView->>User: Render linked labels<br/>for related objects
User->>DetailView: Data change triggers effect
DetailView->>RelatedNames: Refresh related names
Note over RelatedNames: Repeat fetch cycle
RelatedNames->>Cache: Update cache
DetailView->>User: Re-render with updated labels
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
frontend/src/routes/(app)/(third-party)/requirement-assessments/[id=uuid]/edit/+page.server.ts (1)
92-107: Critical: Undefined variable reference causes ReferenceError.Line 103 references an undefined
responsevariable. This will throw a ReferenceError when the else branch executes (i.e., whenfetchJsonreturns null due to a failed fetch).Since
fetchJsonalready logs errors and returns null on failure, this additional error logging is redundant.🔎 Proposed fix
Remove the redundant error logging since
fetchJsonalready handles it:measureModel.selectFields.map(async (selectField) => { const url = `${baseUrl}/applied-controls/${selectField.field}/`; const data = await fetchJson(url); if (data) { measureSelectOptions[selectField.field] = Object.entries(data).map(([key, value]) => ({ label: value, value: selectField.valueType === 'number' ? parseInt(key) : key })); - } else { - console.error(`Failed to fetch data for ${selectField.field}: ${response.statusText}`); } })frontend/src/routes/(app)/(third-party)/compliance-assessments/[id=uuid]/table-mode/+page.server.ts (1)
100-108: Remove unusedrequirementAssessmentsByIdvariable.The
requirementAssessmentsByIdlookup object (lines 100-106) is created but no longer used after line 108 was changed to directly assigntableMode.requirements. This appears to be dead code from the refactoring.🔎 Proposed fix
-const requirementAssessmentsById = requirement_assessments.reduce( - (acc, requirementAssessment) => { - acc[requirementAssessment.requirement] = requirementAssessment; - return acc; - }, - {} -); - const requirements = tableMode.requirements;backend/core/views.py (1)
631-654: Fix UUID path parsing inget_querysetto avoid 500s on detail GETsThe regex branch in
get_querysetis currently broken:
q.group(1)refers to the optional([\w-]+/)?segment, not the UUID segment; for standard detail URLs (/api/<model>/<uuid>/) this group isNone, soUUID(q.group(1))raisesTypeError.- Even when it’s non‑
None, it will be something like"threats/", which is not a valid UUID and raisesValueError.- Because the exception is not caught here, any GET on a detail endpoint backed by
BaseModelViewSetwill 500 once this branch matches.You should instead pull the UUIDs from the second capturing group, guard the conversion, and (optionally) restrict to the first UUID when multiple are present.
Suggested fix for UUID extraction and error‑safety
@@ - object_ids_view = None - if self.request.method == "GET": - if q := re.match( - r"/api/[\w-]+/([\w-]+/)?([0-9a-fA-F]{8}(-[0-9a-fA-F]{4}){3}-[0-9a-fA-F]{12}(,[0-9a-fA-F]{8}(-[0-9a-fA-F]{4}){3}-[0-9a-fA-F]{12})+)", - self.request.path, - ): - """"get_queryset is called by Django even for an individual object via get_object - https://stackoverflow.com/questions/74048193/why-does-a-retrieve-request-end-up-calling-get-queryset""" - id = UUID(q.group(1)) - if RoleAssignment.is_object_readable(self.request.user, self.model, id): - object_ids_view = [id] + object_ids_view = None + if self.request.method == "GET": + m = re.match( + r"/api/[\w-]+/([\w-]+/)?([0-9a-fA-F]{8}(-[0-9a-fA-F]{4}){3}-[0-9a-fA-F]{12}" + r"(,[0-9a-fA-F]{8}(-[0-9a-fA-F]{4}){3}-[0-9a-fA-F]{12})+)", + self.request.path, + ) + if m: + # get_queryset is called by Django even for an individual object via get_object + # https://stackoverflow.com/questions/74048193/why-does-a-retrieve-request-end-up-calling-get-queryset + ids_segment = m.group(2) + first_id_str = ids_segment.split(",", 1)[0] + try: + obj_id = UUID(first_id_str) + except (TypeError, ValueError): + pass + else: + if RoleAssignment.is_object_readable( + self.request.user, self.model, obj_id + ): + object_ids_view = [obj_id] @@ - if not object_ids_view: - scope_folder = Folder.get_root_folder() - object_ids_view = RoleAssignment.get_accessible_object_ids( - scope_folder, self.request.user, self.model - )[0] + if not object_ids_view: + scope_folder = Folder.get_root_folder() + object_ids_view = RoleAssignment.get_accessible_object_ids( + scope_folder, self.request.user, self.model + )[0]backend/library/serializers.py (1)
13-13: Replace undefinedFieldsRelatedFieldwithIdRelatedFieldand add missing import.Line 13 uses
FieldsRelatedField, which no longer exists in the codebase—it was refactored toIdRelatedField. The import statement also does not includeFieldsRelatedField, so this will cause aNameErrorat runtime. Change line 13 to useIdRelatedFieldinstead, which is already designed to accept the legacyfieldsargument for backward compatibility.backend/core/serializers.py (1)
2184-2190: Fix undefinedFieldsRelatedFieldcausing pipeline failures.
FieldsRelatedFieldis used on line 2186 but is not imported—the import was replaced withIdRelatedField. This is causing all three CI pipeline failures.🔎 Proposed fix
class LibraryFilteringLabelReadSerializer(BaseModelSerializer): path = PathField(read_only=True) - folder = FieldsRelatedField() + folder = IdRelatedField() class Meta: model = LibraryFilteringLabel fields = "__all__"
🧹 Nitpick comments (7)
frontend/src/params/filters.ts (1)
5-6: Add a brief comment explaining the "names" parameter special case.The early return for "names" lacks documentation for future maintainers. The endpoint is properly implemented in the route handler and used by the related-names utility. Consider adding a comment explaining its purpose:
+ // Allow "names" parameter for related-names endpoint (maps IDs to display labels) if (param.toLowerCase() === 'names') return true;frontend/src/routes/(app)/(third-party)/requirement-assessments/[id=uuid]/edit/+page.server.ts (1)
32-47: Consider parallelizing independent fetches for better performance.The current implementation fetches related entities sequentially. While the defensive null-checking is excellent, some fetches could be parallelized to improve load times.
🔎 Potential optimization
const requirementAssessment = await fetchJson(endpoint); const complianceAssessmentId = getId(requirementAssessment?.compliance_assessment); const requirementId = getId(requirementAssessment?.requirement); const folderId = getId(requirementAssessment?.folder); -const complianceAssessment = complianceAssessmentId - ? await fetchJson(`${baseUrl}/compliance-assessments/${complianceAssessmentId}/`) - : null; +const [complianceAssessment, requirement] = await Promise.all([ + complianceAssessmentId + ? fetchJson(`${baseUrl}/compliance-assessments/${complianceAssessmentId}/`) + : null, + requirementId ? fetchJson(`${baseUrl}/requirements/${requirementId}/`) : null +]); + const frameworkId = getId(complianceAssessment?.framework); -const framework = frameworkId ? await fetchJson(`${baseUrl}/frameworks/${frameworkId}/`) : null; -const requirement = requirementId - ? await fetchJson(`${baseUrl}/requirements/${requirementId}/`) - : null; -const compliance_assessment_score = complianceAssessmentId - ? await fetchJson(`${baseUrl}/compliance-assessments/${complianceAssessmentId}/global_score/`) - : null; + +const [framework, compliance_assessment_score] = await Promise.all([ + frameworkId ? fetchJson(`${baseUrl}/frameworks/${frameworkId}/`) : null, + complianceAssessmentId + ? fetchJson(`${baseUrl}/compliance-assessments/${complianceAssessmentId}/global_score/`) + : null +]);frontend/src/routes/(app)/(third-party)/compliance-assessments/[id=uuid]/table-mode/+page.server.ts (2)
16-18: Consider extracting these helpers to a shared utility module.The
getIdandnormalizeIdsfunctions implement a useful pattern for handling ID-based relations that aligns with the backend changes. Since this pattern may be needed across multiple route files, consider moving these helpers to a shared utility module (e.g.,$lib/utils/helpers.ts) for consistency and reusability.
26-31: Add error handling for framework fetch.The framework fetch lacks error handling. If the fetch fails or returns a non-OK status, the
.json()call could throw, or the framework assignment could fail silently. Consider wrapping in a try-catch block and checking the response status.🔎 Proposed error handling
const frameworkId = getId(compliance_assessment.framework); if (frameworkId) { const frameworkEndpoint = `${BASE_API_URL}/frameworks/${frameworkId}/`; - const framework = await fetch(frameworkEndpoint).then((res) => res.json()); - compliance_assessment.framework = framework; + try { + const res = await fetch(frameworkEndpoint); + if (res.ok) { + compliance_assessment.framework = await res.json(); + } else { + console.warn(`Failed to fetch framework ${frameworkId}: ${res.status}`); + } + } catch (error) { + console.error(`Error fetching framework ${frameworkId}:`, error); + } }backend/library/serializers.py (1)
82-82: Consider removing ignoredfieldsparameter.
IdRelatedFieldignores thefieldsparameter per its implementation (backend/core/serializer_fields.py lines 30-31). While kept for backward compatibility, removingfields=["urn", "str", "name"]here would improve clarity since it has no effect.🔎 Proposed simplification
- dependencies = IdRelatedField(many=True, fields=["urn", "str", "name"]) + dependencies = IdRelatedField(many=True)frontend/src/lib/components/DetailView/DetailView.svelte (1)
127-154: Helper functions duplicated across components.The functions
getUrlModelForField,extractId,extractIds, andisIdOnlyArrayare duplicated betweenDetailView.svelteandModelTable.svelte. Consider extracting these to a shared utility module (e.g., alongsiderelated-names.ts) to reduce duplication and ensure consistent behavior.frontend/src/lib/components/ModelTable/ModelTable.svelte (1)
208-268: Same helper functions duplicated from DetailView.As noted in the DetailView review, these helper functions (
getUrlModelForField,extractId,extractIds,isIdOnlyArray, andrefreshRelatedNameslogic) are nearly identical. Extracting them to a shared module would improve maintainability.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
backend/core/serializer_fields.pybackend/core/serializers.pybackend/core/views.pybackend/crq/serializers.pybackend/ebios_rm/serializers.pybackend/iam/serializers.pybackend/library/serializers.pybackend/pmbok/serializers.pybackend/privacy/serializers.pybackend/resilience/serializers.pybackend/tprm/serializers.pyenterprise/backend/enterprise_core/serializers.pyfrontend/src/lib/components/DetailView/DetailView.sveltefrontend/src/lib/components/ModelTable/ModelTable.sveltefrontend/src/lib/utils/related-names.tsfrontend/src/params/filters.tsfrontend/src/routes/(app)/(internal)/[model=urlmodel]/[filter=filters]/+server.tsfrontend/src/routes/(app)/(third-party)/compliance-assessments/[id=uuid]/+page.server.tsfrontend/src/routes/(app)/(third-party)/compliance-assessments/[id=uuid]/+page.sveltefrontend/src/routes/(app)/(third-party)/compliance-assessments/[id=uuid]/table-mode/+page.server.tsfrontend/src/routes/(app)/(third-party)/compliance-assessments/[id=uuid]/table-mode/+page.sveltefrontend/src/routes/(app)/(third-party)/requirement-assessments/[id=uuid]/edit/+page.server.tsfrontend/src/routes/(app)/(third-party)/requirement-assessments/[id=uuid]/edit/+page.svelte
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-11-17T11:09:18.488Z
Learnt from: monsieurswag
Repo: intuitem/ciso-assistant-community PR: 2860
File: frontend/src/routes/(app)/(internal)/[model=urlmodel]/[id=uuid]/+page.server.ts:0-0
Timestamp: 2025-11-17T11:09:18.488Z
Learning: In frontend/src/routes/(app)/(internal)/[model=urlmodel]/[id=uuid]/+page.server.ts, the select action performs batch PATCH operations in Promise.all without transaction semantics. Partial failures (e.g., one object fails to update due to race conditions) are acceptable and should not trigger a fail() return, as most objects will still be updated successfully. Console warnings are sufficient for logging failures.
Applied to files:
frontend/src/lib/utils/related-names.tsfrontend/src/routes/(app)/(third-party)/compliance-assessments/[id=uuid]/table-mode/+page.server.tsfrontend/src/routes/(app)/(internal)/[model=urlmodel]/[filter=filters]/+server.tsfrontend/src/routes/(app)/(third-party)/requirement-assessments/[id=uuid]/edit/+page.server.ts
📚 Learning: 2025-09-19T09:08:52.306Z
Learnt from: Mohamed-Hacene
Repo: intuitem/ciso-assistant-community PR: 2550
File: backend/core/migrations/0098_evidence_lifecycle_data_migration.py:21-28
Timestamp: 2025-09-19T09:08:52.306Z
Learning: Evidence model inherits from FolderMixin making folder a mandatory field, and the lifecycle management handles folder propagation to revisions automatically.
Applied to files:
frontend/src/routes/(app)/(third-party)/requirement-assessments/[id=uuid]/edit/+page.server.tsbackend/core/serializers.py
📚 Learning: 2025-09-19T08:46:34.495Z
Learnt from: Mohamed-Hacene
Repo: intuitem/ciso-assistant-community PR: 2550
File: backend/core/serializers.py:1277-1292
Timestamp: 2025-09-19T08:46:34.495Z
Learning: In EvidenceWriteSerializer, the link and attachment fields are read-only, so updates to these fields should be done through EvidenceRevisionWriteSerializer to maintain proper versioning.
Applied to files:
backend/tprm/serializers.pybackend/core/serializers.py
📚 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
📚 Learning: 2025-10-12T21:53:47.742Z
Learnt from: ab-smith
Repo: intuitem/ciso-assistant-community PR: 2699
File: backend/core/serializers.py:414-422
Timestamp: 2025-10-12T21:53:47.742Z
Learning: In the asset hierarchy for CISO Assistant (backend/core/serializers.py), both PRIMARY and SUPPORT assets can have child_assets (supporting assets). PRIMARY assets are at the top of the hierarchy and cannot have parent_assets, while SUPPORT assets can have both parents and children.
Applied to files:
backend/core/serializers.py
🧬 Code graph analysis (13)
frontend/src/routes/(app)/(third-party)/compliance-assessments/[id=uuid]/table-mode/+page.server.ts (1)
frontend/src/lib/utils/constants.ts (1)
BASE_API_URL(4-8)
frontend/src/routes/(app)/(third-party)/requirement-assessments/[id=uuid]/edit/+page.server.ts (2)
frontend/src/lib/utils/crud.ts (1)
getModelInfo(2451-2457)frontend/src/lib/utils/schemas.ts (1)
modelSchema(1482-1484)
backend/tprm/serializers.py (1)
backend/core/serializer_fields.py (2)
IdRelatedField(24-45)HashSlugRelatedField(10-21)
enterprise/backend/enterprise_core/serializers.py (1)
backend/core/serializer_fields.py (1)
IdRelatedField(24-45)
frontend/src/routes/(app)/(third-party)/compliance-assessments/[id=uuid]/+page.server.ts (1)
frontend/src/lib/utils/constants.ts (1)
BASE_API_URL(4-8)
backend/library/serializers.py (1)
backend/core/serializer_fields.py (2)
IdRelatedField(24-45)HashSlugRelatedField(10-21)
backend/crq/serializers.py (1)
backend/core/serializer_fields.py (1)
IdRelatedField(24-45)
backend/resilience/serializers.py (3)
backend/core/serializer_fields.py (1)
IdRelatedField(24-45)backend/resilience/models.py (1)
risk_matrix(214-215)backend/core/models.py (1)
children_assets(2693-2696)
backend/core/views.py (1)
backend/iam/models.py (4)
Folder(80-328)get_root_folder(87-89)RoleAssignment(823-1116)get_accessible_object_ids(933-1037)
backend/privacy/serializers.py (1)
backend/core/serializer_fields.py (1)
IdRelatedField(24-45)
backend/iam/serializers.py (1)
backend/core/serializer_fields.py (1)
IdRelatedField(24-45)
backend/ebios_rm/serializers.py (1)
backend/core/serializer_fields.py (5)
IdRelatedField(24-45)HashSlugRelatedField(10-21)to_representation(15-21)to_representation(40-45)to_representation(61-95)
backend/core/serializers.py (1)
backend/core/serializer_fields.py (1)
IdRelatedField(24-45)
🪛 GitHub Actions: API Tests
backend/core/serializers.py
[error] 2186-2186: NameError: name 'FieldsRelatedField' is not defined. Did you mean: 'IdRelatedField'?
🪛 GitHub Actions: Backend code coverage
backend/core/serializers.py
[error] 2186-2186: NameError: name 'FieldsRelatedField' is not defined. Did you mean: 'IdRelatedField'?
🪛 GitHub Actions: Backend migrations check
backend/core/serializers.py
[error] 2186-2186: NameError: name 'FieldsRelatedField' is not defined. Did you mean: 'IdRelatedField'? (raised during makemigrations check while loading serializers)
⏰ 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). (7)
- GitHub Check: build_enterprise_frontend
- GitHub Check: build_community_frontend
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: Analyze (python)
🔇 Additional comments (46)
frontend/src/routes/(app)/(third-party)/requirement-assessments/[id=uuid]/edit/+page.server.ts (7)
21-21: LGTM: Clean ID normalization helper.The
getIdhelper provides a simple, defensive pattern for extracting IDs from values that may be strings or objects.
23-30: LGTM: Robust fetch wrapper with error handling.The
fetchJsonhelper provides consistent error handling across all fetch operations and enables graceful degradation when requests fail.
53-64: LGTM: Clean ID array normalization pattern.The
normalizeIdsutility and its application to related fields provide consistent ID-based handling across evidences, applied_controls, and security_exceptions.
85-87: LGTM: Consistent use of folderId in form initialization.The change to pass
folderIdinstead of a nested folder object aligns with the ID-based pattern used throughout this refactor.
127-127: LGTM: Consistent folderId usage in evidence form.The evidence form initialization correctly uses
folderIdconsistent with the ID-based pattern.
152-152: LGTM: Consistent folderId usage in security exception form.The security exception form initialization correctly uses
folderIdconsistent with the ID-based pattern.
180-181: LGTM: Enhanced return payload with related entities.Returning
complianceAssessmentandframeworkprovides valuable context to the frontend for rendering and validation.frontend/src/routes/(app)/(third-party)/compliance-assessments/[id=uuid]/table-mode/+page.server.ts (2)
45-84: LGTM! Consistent ID normalization pattern.The use of
getIdandnormalizeIdsthroughout the requirement assessment processing is consistent and defensive. This properly handles cases where relations might be either ID strings or populated objects, aligning well with the backend changes mentioned in the PR summary.
108-108: No changes needed. The direct assignmentconst requirements = tableMode.requirements;is correct and properly handles the requirements data structure. ThetableModeobject is fetched directly from the API endpointrequirements_list/and contains the complete, correctly formatted requirements list. Downstream Svelte components consume this data appropriately by building a hashmap for efficient lookups. The previousrequirementAssessmentsByIdcreation appears to be unused dead code, so the current simpler approach is the right choice.frontend/src/routes/(app)/(third-party)/compliance-assessments/[id=uuid]/+page.svelte (3)
51-57: LGTM! Defensive derivation of IDs and framework data.The derived values properly handle multiple data shapes:
folderIdhandles both string IDs and objects with anidpropertyframeworkfalls back tocompliance_assessment.frameworkif not provided in dataframeworkReferenceControlsandimplementationGroupsDefinitionsafely default to empty arraysThis defensive approach prevents runtime errors when the backend returns ID-only relations and aligns well with the ID-based pattern described in the PR summary.
61-75: LGTM! Consistent use of derivedfolderIdin permission checks.Using the derived
folderIdinstead ofcompliance_assessment.folder.idensures permission checks work correctly regardless of whetherfolderis an ID string or a populated object. This is a safer pattern that prevents potential undefined access errors.
362-394: LGTM! Consistent use of derived framework values throughout UI.The replacement of direct nested path access (e.g.,
data.compliance_assessment.framework.reference_controls) with the derivedframeworkReferenceControlsandimplementationGroupsDefinitionvalues is consistent throughout the component. This improves:
- Safety: Prevents undefined access errors when framework data is null/undefined
- Consistency: Single source of truth for these values
- Readability: Clearer variable names than nested paths
Also applies to: 452-452, 732-732
frontend/src/routes/(app)/(internal)/[model=urlmodel]/[filter=filters]/+server.ts (3)
13-13: LGTM!The fetch call correctly converts the URL object to a string.
6-11: The URL construction change is sound.The refactoring to use a URL object is a good improvement.
BASE_API_URLis properly configured as an absolute URL (default:http://localhost:8000/api), so thenew URL()constructor call on line 8 is safe. The query parameter forwarding in line 11 is intentional design for this internal route's filtering functionality.No changes needed.
18-24: The early return for thenamesfilter is correct.The backend
/names/endpoint returns a dictionary mapping UUIDs to object names ({uuid: "name", ...}), which matches the format expected by the frontend'sfetchNamesfunction. The early return correctly bypasses the transformation logic at lines 26–32, which is intended for other filters that return different response structures.backend/core/views.py (5)
661-682: Genericnamesaction is correct and RBAC‑safeThe new
namesaction:
- Validates
self.modeland theid[]query param, safely ignoring malformed UUIDs.- Intersects the requested IDs with
get_accessible_object_ids(Folder.get_root_folder(), …)[0], so it never reveals names for objects the user cannot view.- Returns a simple
{id: str(obj)}mapping that lines up well with the new Id‑based related‑field pattern.This looks sound and should serve the frontend’s generic ID→label resolution use case without introducing extra permission surface.
1283-1295: Threat queryset prefetches match IdRelatedField usageThe added
select_related("library")andprefetch_related("filtering_labels__folder")align with the new Id‑only related serialization (where labels and libraries are resolved separately) and reduce N+1s for table and detail views. AssumingThreat.libraryandFilteringLabel.folderexist as declared in models, this is a good optimization.
3806-3825: AppliedControl queryset prefetches correctly cover new Id‑based relationsThe extended
prefetch_related:
- Adds
"filtering_labels__folder","evidences", and"security_exceptions", which are exactly the relationships now serialized via IdRelatedField / counted in views.- Keeps
select_related("folder", "reference_control"), so row rendering and exports remain efficient.This is an appropriate performance improvement for a hot table API.
7580-7599: ComplianceAssessment queryset optimizations are consistent with new relation shapeIn
ComplianceAssessmentViewSet.get_querysetthe new:
select_related("perimeter__folder", "campaign")tracks the fact that serializers/actions now exposeperimeterandcampaignthrough IdRelatedField with folder context.prefetch_related("assets", "evidences")matches the ManyToMany fields now sent as ID lists.These additions should significantly cut query counts for list and export endpoints without changing behavior.
8956-8974: RequirementAssessment queryset prefetches support Id‑only related fieldsThe updated queryset:
- Adds
select_related("compliance_assessment__perimeter__folder"), which matches the nested IdRelatedField usage for perimeter/folder in requirement‑assessment tables.- Prefetches
"evidences"and"security_exceptions"so the new Id‑based serializers don’t trigger N+1 behavior.This is a good, targeted optimization for a frequently queried model.
frontend/src/routes/(app)/(third-party)/compliance-assessments/[id=uuid]/+page.server.ts (3)
21-37: LGTM! Robust handling of ID-based relations.The pattern of deriving IDs with type guards (checking
typeof === 'string'vs object) and the conditional framework fetch with error handling (res.ok ? res.json() : null) correctly handles the transition to ID-based serialization.
61-62: LGTM! Consistent use of derived IDs.Using
frameworkIdandperimeterId ?? nullcorrectly aligns the clone data with the new ID-based approach.
99-110: LGTM! Proper propagation of derived data.Passing
folderIdtoloadValidationFlowFormDataand exposing the fetchedframeworkin the page data correctly implements the new data-fetching pattern.frontend/src/routes/(app)/(third-party)/compliance-assessments/[id=uuid]/table-mode/+page.svelte (2)
73-81: LGTM! Well-structured helper for requirement resolution.The
getRequirementhelper correctly handles both ID-based (string) and nested object forms ofrequirementAssessment.requirement, with appropriate hashmap lookup and fallbacks.
88-92: LGTM! Consistent application of requirement resolution.The
getRequirementhelper is applied systematically throughout the component with appropriate optional chaining (requirement?.field), ensuring safe access to requirement metadata regardless of whether it's stored as an ID or object.Also applies to: 404-476, 609-622
frontend/src/routes/(app)/(third-party)/requirement-assessments/[id=uuid]/edit/+page.svelte (4)
49-58: LGTM! Safe extraction of optional related data.Using optional chaining with array fallbacks (
?? []) forassociated_threats,associated_reference_controls, andimplementation_groups_definitioncorrectly handles cases where related data might be absent or ID-only.
71-77: LGTM! Consistent ID derivation pattern.The type guard approach to derive
complianceAssessmentIdwith a fallback URL mirrors the pattern used in the server file, ensuring consistent handling of ID-based relations.
281-320: LGTM! Consistent optional chaining for safe access.The use of
data.complianceAssessment?.is_lockedanddata.requirement?.{field}throughout ensures the component gracefully handles missing or partial data from the ID-based serialization.
532-533: No action needed. The code is correct.Lines 532 and 567 properly pass
page.data.requirementAssessment.folderdirectly tooptionsDetailedUrlParametersasscope_folder_id. TheRequirementAssessmentReadSerializerdefinesfolder = IdRelatedField(), which serializes the folder as a primary key ID string, so it can be used directly without.idextraction.enterprise/backend/enterprise_core/serializers.py (1)
7-7: LGTM! Clean migration to IdRelatedField.The import update and field change in
RoleReadSerializerare consistent with the broader serialization refactor.Also applies to: 46-46
backend/resilience/serializers.py (1)
2-2: LGTM! Systematic migration to IdRelatedField.All related fields in BusinessImpactAnalysis, AssetAssessment, and EscalationThreshold read serializers have been consistently updated to use
IdRelatedField. Thevalidation_flowsfield retains its nested field specification via the (ignored)fieldsparameter, which maintains API documentation clarity even though the parameter is not used.Also applies to: 18-30, 43-55, 91-95
backend/iam/serializers.py (1)
5-5: LGTM! Consistent migration to IdRelatedField.The update to
PersonalAccessTokenReadSerializer.useraligns with the broader ID-based serialization pattern.Also applies to: 125-125
backend/tprm/serializers.py (1)
6-6: LGTM! Comprehensive migration to IdRelatedField.All TPRM read serializers (Entity, EntityAssessment, Representative, Solution, Contract) have been systematically updated to use
IdRelatedFieldfor related object references, maintaining consistency with the broader serialization refactor.Also applies to: 22-30, 113-131, 266-268, 337-342, 376-383
frontend/src/lib/components/DetailView/DetailView.svelte (2)
205-209: LGTM!The reactive effect correctly tracks dependencies and refreshes related names when the underlying data changes.
512-530: LGTM!The library field rendering correctly handles all cases: resolved labels with links, labels without links, raw IDs during loading, and visibility notices for inaccessible items.
backend/core/serializer_fields.py (1)
24-45: Breaking API change - related fields now return IDs only.This is a significant API behavior change. Previously,
FieldsRelatedFieldreturned nested object data; nowIdRelatedFieldreturns only the primary key. The backward-compatible__init__signature is well-designed to avoid call-site changes, but API consumers will receive different response shapes.Ensure this change is documented in release notes or API migration guides for external consumers.
frontend/src/lib/components/ModelTable/ModelTable.svelte (2)
329-333: LGTM!The reactive effect correctly tracks row and model changes to refresh related names.
668-740: LGTM!The array rendering logic properly handles ID-only arrays, resolved names with links, and visibility notices for hidden items. The implementation is consistent with DetailView.
backend/pmbok/serializers.py (1)
4-42: LGTM!The migration from
FieldsRelatedFieldtoIdRelatedFieldis consistent. The legacy field arguments are preserved for documentation/compatibility purposes but are correctly ignored byIdRelatedField.__init__.backend/crq/serializers.py (2)
85-101: LGTM!The migration is consistent with the pattern. Note that the
fieldsargument onvalidation_flows(lines 94-99) is preserved for documentation but is ignored byIdRelatedField- only IDs will be returned.
130-149: LGTM!All related fields in
QuantitativeRiskScenarioReadSerializerandQuantitativeRiskHypothesisReadSerializerare correctly migrated toIdRelatedField.backend/privacy/serializers.py (1)
1-181: LGTM!All privacy serializers are consistently migrated to
IdRelatedField. The legacy field specifications (e.g.,["name", "type", "folder"]onassets) are preserved but will be ignored, as only IDs are returned.backend/ebios_rm/serializers.py (2)
394-399: LGTM!Using
IdRelatedField().to_representation()directly inget_strategic_scenariois a valid approach for consistent serialization of the nested relationship.
475-561: LGTM!The remaining read serializers (
ElementaryActionReadSerializer,OperatingModeReadSerializer,KillChainReadSerializer) are correctly migrated to useIdRelatedField.frontend/src/lib/utils/related-names.ts (1)
1-42: Well-structured utility for client-side related name resolution.The implementation correctly handles batching, deduplication, and caching. A few observations:
- The module-level cache persists for the entire session. If related entities are renamed, users would need to refresh the page to see updated names.
- Silent failure on non-OK responses (line 33) is appropriate for resilience, but consider logging for debugging purposes in development.
backend/core/serializers.py (1)
10-14: LGTM!The import changes correctly bring in
IdRelatedFieldwhich is consistently used throughout the file (except for the bug at line 2186 noted separately).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/core/serializers.py (1)
2041-2042: Remove or uncomment the dead code.Line 2041 contains a commented-out field definition. If it's intentionally removed, delete the comment. If it's needed, uncomment it.
Proposed fix
- # library = IdRelatedField(["name", "id"]) folder = IdRelatedField()
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/core/serializers.pybackend/library/serializers.pybackend/metrology/serializers.py
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/library/serializers.py
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-10-12T21:53:47.742Z
Learnt from: ab-smith
Repo: intuitem/ciso-assistant-community PR: 2699
File: backend/core/serializers.py:414-422
Timestamp: 2025-10-12T21:53:47.742Z
Learning: In the asset hierarchy for CISO Assistant (backend/core/serializers.py), both PRIMARY and SUPPORT assets can have child_assets (supporting assets). PRIMARY assets are at the top of the hierarchy and cannot have parent_assets, while SUPPORT assets can have both parents and children.
Applied to files:
backend/core/serializers.py
📚 Learning: 2025-09-19T08:46:34.495Z
Learnt from: Mohamed-Hacene
Repo: intuitem/ciso-assistant-community PR: 2550
File: backend/core/serializers.py:1277-1292
Timestamp: 2025-09-19T08:46:34.495Z
Learning: In EvidenceWriteSerializer, the link and attachment fields are read-only, so updates to these fields should be done through EvidenceRevisionWriteSerializer to maintain proper versioning.
Applied to files:
backend/core/serializers.py
📚 Learning: 2025-09-19T09:08:52.306Z
Learnt from: Mohamed-Hacene
Repo: intuitem/ciso-assistant-community PR: 2550
File: backend/core/migrations/0098_evidence_lifecycle_data_migration.py:21-28
Timestamp: 2025-09-19T09:08:52.306Z
Learning: Evidence model inherits from FolderMixin making folder a mandatory field, and the lifecycle management handles folder propagation to revisions automatically.
Applied to files:
backend/core/serializers.py
🧬 Code graph analysis (2)
backend/metrology/serializers.py (1)
backend/core/serializer_fields.py (2)
IdRelatedField(24-45)PathField(48-95)
backend/core/serializers.py (1)
backend/core/serializer_fields.py (1)
IdRelatedField(24-45)
⏰ 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 (3.12)
- GitHub Check: build_community_frontend
- GitHub Check: build_enterprise_frontend
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: Analyze (python)
🔇 Additional comments (8)
backend/metrology/serializers.py (3)
5-5: Verify the intentional breaking API change: nested fields are now ignored.Based on the
IdRelatedFieldimplementation inbackend/core/serializer_fields.py, it explicitly ignores all field arguments (like["name", "id"],["folder"], or nested specs) and only returns primary keys. This is a breaking change for API consumers expecting nested data.The field specifications throughout this file (e.g.,
IdRelatedField(["name", "id"])) are preserved for call-site compatibility but have no effect on serialization output.Ensure the frontend's "dynamic fetch of related fields" pattern (mentioned in PR objectives) is fully implemented to compensate for the missing nested data.
46-56: Complex nested field spec is ignored by IdRelatedField.This detailed field specification with nested objects (
{"unit": ["name", "id"]},"choices_definition", etc.) will be completely ignored. Themetric_definitionfield will only return the UUID/primary key.If API consumers rely on these nested fields (category, unit, choices_definition, higher_is_better), they will need to make additional API calls to fetch this data.
320-337: Deeply nested field specification has no effect.The
metric_instancefield has an elaborate nested structure includingmetric_definitionwith its own nestedunit. All of this is ignored byIdRelatedField, which only returns the primary key.This is consistent with the pattern, but verify that dashboard widget functionality doesn't break due to missing nested metric definition data.
backend/core/serializers.py (5)
10-14: Import change aligns with project-wide IdRelatedField migration.The switch from
FieldsRelatedFieldtoIdRelatedFieldis consistent with the PR's objective. All field arguments passed toIdRelatedFieldthroughout this file will be ignored (only IDs returned).
337-346: Complex nested field spec for validation_flows will be ignored.The
validation_flowsfield specifies nested approver details (["id", "email", "first_name", "last_name"]), butIdRelatedFieldonly returns primary keys. If the frontend needs approver names/emails for validation flow display, ensure the dynamic fetch pattern handles this.
143-148: AssessmentReadSerializer base class fields updated consistently.The base serializer fields (
perimeter,authors,reviewers,folder) are updated toIdRelatedField. Child serializers inheriting from this will also be affected.
757-789: RiskScenarioReadSerializer has extensive IdRelatedField usage.Multiple related fields updated:
risk_assessment,risk_matrix,perimeter,operational_scenario,threats,assets,qualifications,risk_origin,antecedent_scenarios,applied_controls,existing_applied_controls,owner,security_exceptions,filtering_labels.All field specifications (e.g.,
["id", "name", "is_locked"]for risk_assessment) will be ignored. The API response for risk scenarios will now contain only IDs for all these relationships.
2686-2701: TaskNodeReadSerializer preserves field specs for documentation purposes.Fields like
task_template,evidences,expected_evidence, etc. useIdRelatedField(["folder", "id"], many=True). While the["folder", "id"]spec is ignored, it serves as documentation for what the frontend may need to fetch dynamically.
tchoumi313
changed the title
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (7)
frontend/messages/hi.json (1)
1036-1036: Consider using ICU MessageFormat for proper Hindi pluralization.The new translation uses a singular form "वस्तु" (object) which will be grammatically incorrect when
countis plural. Other count-based translations in this file (e.g.,hourCount,minuteCount,dayCountat lines 900-928) use ICU MessageFormat with plural rules for Hindi.🔎 Suggested pluralization pattern
- "objectsNotVisible": "{count} वस्तु दिखाई नहीं देती।" + "objectsNotVisible": [ + { + "declarations": ["input count", "local countPlural = count: plural"], + "selectors": ["countPlural"], + "match": { + "countPlural=one": "{count} वस्तु दिखाई नहीं देती।", + "countPlural=other": "{count} वस्तुएं दिखाई नहीं देतीं।" + } + } + ]frontend/messages/ur.json (1)
1035-1035: Consider adding pluralization for better UX.The new locale key uses a simple
{count}placeholder with singular form "شے" (object). For grammatically correct Urdu and better user experience, consider using a pluralization structure similar to other count-based messages in this file (e.g.,cascadeAffectedNoticeat lines 1022-1032).🔎 Example pluralization structure
- "objectsNotVisible": "{count} شے نظر نہیں آتی۔" + "objectsNotVisible": [ + { + "declarations": ["input count", "local countPlural = count: plural"], + "selectors": ["count", "countPlural"], + "match": { + "countPlural=one": "{count} شے نظر نہیں آتی۔", + "countPlural=other": "{count} اشیاء نظر نہیں آتیں۔" + } + } + ]Note: This assumes the i18n framework supports this format. Verify the translation with a native Urdu speaker for grammatical accuracy.
frontend/src/lib/components/ModelTable/ModelTable.svelte (3)
242-270: Parallelize the fetchNames calls for better performance.The current implementation awaits
fetchNamessequentially in a loop, which can cause noticeable delays when multiple URL models need name resolution. Consider fetching all names concurrently usingPromise.all.🔎 Proposed refactor using Promise.all
const refreshRelatedNames = async (rowsToInspect: Record<string, any>[] = tableSource.body) => { if (!browser || !model?.foreignKeyFields) return; if (!Array.isArray(rowsToInspect)) return; const idsByModel = new Map<string, Set<string>>(); for (const fieldConfig of model.foreignKeyFields) { const urlModel = fieldConfig.urlModel ?? fieldConfig.field?.replace(/_/g, '-'); if (!urlModel) continue; for (const row of rowsToInspect) { const ids = extractIds(row[fieldConfig.field]); if (ids.length === 0) continue; const bucket = idsByModel.get(urlModel) ?? new Set<string>(); ids.forEach((id) => bucket.add(id)); idsByModel.set(urlModel, bucket); } } const key = JSON.stringify( Array.from(idsByModel.entries()).map(([urlModel, ids]) => [urlModel, Array.from(ids).sort()]) ); if (!key || key === relatedNamesKey) return; relatedNamesKey = key; - const updates: Record<string, Record<string, string>> = {}; - for (const [urlModel, ids] of idsByModel.entries()) { - updates[urlModel] = await fetchNames(urlModel, Array.from(ids)); - } + const fetchPromises = Array.from(idsByModel.entries()).map(async ([urlModel, ids]) => { + const names = await fetchNames(urlModel, Array.from(ids)); + return [urlModel, names] as const; + }); + const results = await Promise.all(fetchPromises); + const updates: Record<string, Record<string, string>> = Object.fromEntries(results); + if (Object.keys(updates).length > 0) { relatedNames = { ...relatedNames, ...updates }; } };
688-763: Consider extracting the array rendering logic into helper functions.The array rendering block has grown quite complex with nested conditionals and duplicated patterns (e.g., the
ul > lirendering appears multiple times). Extracting this into separate helper functions or components would improve readability and maintainability.For example, you could create helpers like:
renderIdArray(ids, urlModel, resolvedNames)for ID-only arraysrenderMixedArray(values, key, model)for arrays with display labelsThis would reduce cognitive complexity and make the rendering logic easier to test and modify.
210-211: Optional: Consider implementing cache size limits.The
relatedNamescache grows indefinitely and never clears (line 268 spreads new updates on top of existing cache). While this is unlikely to cause issues in typical sessions, very long-running sessions viewing many different models could accumulate significant memory.Consider implementing a simple LRU cache or setting a maximum cache size (e.g., clear cache when it exceeds 1000 entries or when navigating to a different model). This is a nice-to-have improvement rather than a critical issue.
Also applies to: 268-268
backend/core/views.py (2)
631-654: Clarify/remove unused UUID path detection logic inget_querysetThe
re.matchblock that tries to parse UUIDs fromself.request.pathlooks effectively unused with the current routing (list/detail routes don’t place comma‑separated UUIDs at the end of the path, and the newnamesaction uses query params). It adds complexity and potential confusion around how visibility is scoped, without changing behavior in practice.Consider either:
- Removing this block entirely, and always using the
get_accessible_object_ids(root_folder, …)path, or- Refactoring it to match the actual URL patterns still in use, with a small comment explaining the supported paths.
6955-6969: MakeFrameworkViewSet.namesmore defensive aroundget_name_translatedThe current implementation assumes
framework.get_name_translatedis either a string or a zero‑arg callable:str(framework.id): framework.get_name_translated if isinstance(framework.get_name_translated, str) else framework.get_name_translated()If
get_name_translatedis ever a lazy translation proxy or a property returning a non‑str/non‑callable, this could raise at runtime.Consider a slightly more defensive pattern that also falls back to
str(framework)when the attribute is missing or of an unexpected type:Suggested refactor
- return Response( - { - str(framework.id): framework.get_name_translated - if isinstance(framework.get_name_translated, str) - else framework.get_name_translated() - for framework in queryset - } - ) + data = {} + for framework in queryset: + translated = getattr(framework, "get_name_translated", None) + if callable(translated): + label = translated() + elif isinstance(translated, str): + label = translated + else: + label = str(framework) + data[str(framework.id)] = label + return Response(data)This keeps the intended behavior but is more robust to future model changes.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
backend/core/views.pyfrontend/messages/ar.jsonfrontend/messages/cs.jsonfrontend/messages/da.jsonfrontend/messages/de.jsonfrontend/messages/el.jsonfrontend/messages/en.jsonfrontend/messages/es.jsonfrontend/messages/fr.jsonfrontend/messages/hi.jsonfrontend/messages/hr.jsonfrontend/messages/hu.jsonfrontend/messages/id.jsonfrontend/messages/it.jsonfrontend/messages/nl.jsonfrontend/messages/pl.jsonfrontend/messages/pt.jsonfrontend/messages/ro.jsonfrontend/messages/sv.jsonfrontend/messages/tr.jsonfrontend/messages/uk.jsonfrontend/messages/ur.jsonfrontend/src/lib/components/DetailView/DetailView.sveltefrontend/src/lib/components/ModelTable/ModelTable.svelte
✅ Files skipped from review due to trivial changes (2)
- frontend/messages/da.json
- frontend/src/lib/components/DetailView/DetailView.svelte
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-11-17T11:09:18.488Z
Learnt from: monsieurswag
Repo: intuitem/ciso-assistant-community PR: 2860
File: frontend/src/routes/(app)/(internal)/[model=urlmodel]/[id=uuid]/+page.server.ts:0-0
Timestamp: 2025-11-17T11:09:18.488Z
Learning: In frontend/src/routes/(app)/(internal)/[model=urlmodel]/[id=uuid]/+page.server.ts, the select action performs batch PATCH operations in Promise.all without transaction semantics. Partial failures (e.g., one object fails to update due to race conditions) are acceptable and should not trigger a fail() return, as most objects will still be updated successfully. Console warnings are sufficient for logging failures.
Applied to files:
frontend/src/lib/components/ModelTable/ModelTable.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/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). (1)
- GitHub Check: Analyze (python)
🔇 Additional comments (18)
frontend/messages/hu.json (1)
1066-1067: LGTM!The addition of the
objectsNotVisibletranslation key is correctly implemented with proper JSON syntax and aligns with the PR's objective to alert users about non-visible objects. The Hungarian translation follows the existing pattern for count-based messages.frontend/messages/id.json (1)
1251-1252: LGTM!The trailing comma addition and new translation entry are correctly implemented. The Indonesian translation "objek tidak terlihat" accurately conveys "objects not visible," and the placeholder syntax is consistent with similar entries throughout the file.
frontend/messages/de.json (1)
2932-2932: LGTM: Trailing comma added for consistency.The trailing comma added to the
libraryFilteringLabelsentry is valid JSON and follows best practices for easier maintenance and cleaner diffs when adding new entries.frontend/messages/ur.json (1)
1034-1034: LGTM! JSON syntax fix is correct.The trailing comma addition is necessary and correct for valid JSON syntax when followed by another entry.
frontend/messages/hr.json (1)
2031-2031: LGTM! Trailing comma correctly added.The trailing comma after the
libraryFilteringLabelsentry is properly placed to accommodate the new subsequent key, maintaining valid JSON syntax.frontend/messages/cs.json (1)
1054-1054: LGTM!The trailing comma is correctly added to accommodate the new key below.
frontend/messages/sv.json (1)
1057-1057: LGTM: Trailing comma correctly added.The trailing comma is properly added to accommodate the new sibling entry below.
frontend/messages/uk.json (1)
1581-1581: LGTM!The trailing comma is correctly added to accommodate the new translation key that follows.
frontend/messages/pt.json (1)
1066-1066: LGTM: Trailing comma correctly added.The trailing comma addition is necessary and correct for valid JSON structure.
frontend/messages/pl.json (1)
1806-1806: No action needed. The comma at the end of line 1806 is a standard JSON separator between key-value pairs, not a trailing comma. The JSON file is valid and parses correctly with jq. The"libraryFilteringLabels"entry is properly formatted.Likely an incorrect or invalid review comment.
frontend/messages/en.json (1)
3024-3025: LGTM! Clean addition of translation key for non-visible objects notification.The JSON syntax is valid, and the new
objectsNotVisibletranslation follows the existing simple pluralization pattern used elsewhere in the file (e.g.,untreatedRiskScenarios,acceptedRiskScenarios). The trailing comma on line 3024 correctly enables the new entry.frontend/messages/tr.json (1)
1799-1800: LGTM! Turkish translation is grammatically correct.The translation
"{count} nesne görünür değil."correctly maintains singular noun form after the count placeholder, which is proper Turkish grammar. The JSON syntax is valid and mirrors the English locale changes appropriately.frontend/messages/fr.json (1)
3023-3024: New French key is consistent with existing pattern.
objectsNotVisiblecorrectly mirrors the existing{count} … {s}style already used in this file, and the JSON structure (comma afterlibraryFilteringLabels) is valid.backend/core/views.py (5)
661-682: Genericnamesaction looks correct and permission‑safeThe new
namesaction correctly:
- Validates/normalizes UUIDs from
id[]query params.- Intersects requested IDs with
get_accessible_object_ids(Folder.get_root_folder(), …)to enforce RBAC.- Returns a simple
{id: str(obj)}map that matches the new Id‑based frontend pattern.No issues from a correctness or security perspective.
1283-1295: Threat queryset optimizations align with IdRelatedField usageAdding:
select_related("library")prefetch_related("filtering_labels__folder")on top of the existing folder prefetches is consistent with moving to Id‑based related fields and should reduce N+1s for table/list views. The relationships referenced all exist and are correctly shaped for these ORM hints.
3806-3825: AppliedControl queryset prefetch additions are appropriateThe extended prefetch:
filtering_labels__folderevidencessecurity_exceptionsmatches the new IdRelatedField pattern (including nested
folderwhere needed) and will avoid extra queries in serializers and tables. The mix ofselect_relatedandprefetch_relatedis idiomatic for the referenced FKs/M2Ms.
7583-7601: ComplianceAssessment queryset: good balance of select/prefetch and annotationsThe added
select_relatedtargets (perimeter__folder,campaign) andprefetch_relatedforassetsandevidencesare well‑chosen for Id‑based relations and table columns. They compose cleanly with the subsequentannotateblock (no joins that would break the counts), so this is a solid performance/UX improvement.
8958-8976: RequirementAssessment queryset optimizations are consistentThe new:
select_related("compliance_assessment__perimeter__folder")prefetch_related("evidences", "security_exceptions")fit the read pattern (table + IdRelatedFields) and reuse the same folder‑path optimization as other viewsets. No correctness issues; this should significantly reduce query chatter when listing requirement assessments.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
backend/ebios_rm/serializers.py (1)
37-37: Duplicate field declaration:reference_entityis defined twice.
reference_entityis declared on both line 35 and line 37. The second declaration will override the first. Remove the duplicate.🔎 Proposed fix
class EbiosRMStudyReadSerializer(BaseModelSerializer): str = serializers.CharField(source="__str__") perimeter = IdRelatedField(["id", "folder"]) folder = IdRelatedField() reference_entity = IdRelatedField() risk_matrix = IdRelatedField() - reference_entity = IdRelatedField() assets = IdRelatedField(["id", "type", {"folder": ["id"]}], many=True)
🧹 Nitpick comments (1)
backend/ebios_rm/serializers.py (1)
33-59: Consider removing ignored field specifications.
IdRelatedFieldaccepts field arrays and nested field specifications for backwards compatibility but ignores them, returning only the primary key. The field specifications on lines 33, 38, and 50-58 have no effect and may confuse future maintainers.Example cleanup
- perimeter = IdRelatedField(["id", "folder"]) + perimeter = IdRelatedField() - assets = IdRelatedField(["id", "type", {"folder": ["id"]}], many=True) + assets = IdRelatedField(many=True) - validation_flows = IdRelatedField( - many=True, - fields=[ - "id", - "ref_id", - "status", - {"approver": ["id", "email", "first_name", "last_name"]}, - ], - source="validationflow_set", - ) + validation_flows = IdRelatedField(many=True, source="validationflow_set")
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/ebios_rm/serializers.pyfrontend/messages/en.jsonfrontend/messages/fr.json
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/messages/en.json
- frontend/messages/fr.json
🧰 Additional context used
🧬 Code graph analysis (1)
backend/ebios_rm/serializers.py (1)
backend/core/serializer_fields.py (5)
IdRelatedField(24-45)HashSlugRelatedField(10-21)to_representation(15-21)to_representation(40-45)to_representation(61-95)
⏰ 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: build (3.12)
- 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: Analyze (python)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
frontend/messages/hr.json (1)
2031-2041: Croatian pluralization now structured; consider addingfewform for correctness.The new
objectsNotVisibleentry follows the existinghourCount/dayCountpattern and will work technically, but Croatian grammar would be more accurate with a separatecountPlural=fewbranch and adjustedothertext for 5+. This is optional, but keeps it closer to proper Croatian plurals.Optional diff to refine Croatian plural forms
- "objectsNotVisible": [ - { - "declarations": ["input count", "local countPlural = count: plural"], - "selectors": ["count", "countPlural"], - "match": { - "countPlural=one": "{count} objekt nije vidljiv.", - "countPlural=other": "{count} objekti nisu vidljivi." - } - } - ] + "objectsNotVisible": [ + { + "declarations": ["input count", "local countPlural = count: plural"], + "selectors": ["count", "countPlural"], + "match": { + "countPlural=one": "{count} objekt nije vidljiv.", + "countPlural=few": "{count} objekta nisu vidljiva.", + "countPlural=other": "{count} objekata nije vidljivo." + } + } + ]
🧹 Nitpick comments (2)
frontend/messages/ar.json (1)
1034-1044: Arabic pluralization forobjectsNotVisiblelooks correct; optional tweak for label wording.The new
objectsNotVisibleentry is structured consistently with other pluralized messages and the Arabic text is fine. If you want slightly more natural wording, you might renamelibraryFilteringLabelsto refer explicitly to “library labels” rather than “office labels”.Optional wording tweak for `libraryFilteringLabels`
- "libraryFilteringLabels": "العلامات المكتبية", + "libraryFilteringLabels": "علامات المكتبة",backend/ebios_rm/serializers.py (1)
424-429: Unconventional pattern: Consider definingstrategic_scenariodirectly as anIdRelatedFieldif the optional relationship permits.The dynamic
IdRelatedField().to_representation()pattern inget_strategic_scenario()is unique in the codebase—no other serializers use this approach. While it correctly handles the optional nested relationship, all otherIdRelatedFielddeclarations are defined directly on the class. If the relationship can be safely declared as a direct field (accounting for the potential null case), that would be more consistent with the rest of the codebase.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
backend/ebios_rm/serializers.pyfrontend/messages/ar.jsonfrontend/messages/cs.jsonfrontend/messages/de.jsonfrontend/messages/el.jsonfrontend/messages/en.jsonfrontend/messages/es.jsonfrontend/messages/hr.jsonfrontend/messages/it.jsonfrontend/messages/nl.jsonfrontend/messages/pl.jsonfrontend/messages/pt.jsonfrontend/messages/ro.jsonfrontend/messages/sv.jsonfrontend/messages/uk.json
🚧 Files skipped from review as they are similar to previous changes (6)
- frontend/messages/pt.json
- frontend/messages/en.json
- frontend/messages/cs.json
- frontend/messages/nl.json
- frontend/messages/pl.json
- frontend/messages/de.json
⏰ 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: build (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: Analyze (python)
🔇 Additional comments (11)
frontend/messages/ro.json (1)
1063-1073: LGTM! Pluralization correctly implemented.The
objectsNotVisibleentry now properly handles Romanian pluralization using the ICU MessageFormat structure, matching the established pattern in this file. Both the noun and adjective correctly change form between singular ("obiect nevizibil") and plural ("obiecte nevizibile"), which addresses the previous review feedback.frontend/messages/uk.json (2)
1581-1581: LGTM: Simple translation added correctly.The translation for "libraryFilteringLabels" is grammatically correct and follows the file's format consistently.
1582-1592: Excellent implementation of Ukrainian pluralization!The ICU MessageFormat structure correctly addresses the previous review concern about incomplete pluralization. The three plural forms (one/few/other) are grammatically accurate for Ukrainian:
- one: "об'єкт невидимий" (singular nominative)
- few: "об'єкти невидимі" (2-4 objects, nominative plural)
- other: "об'єктів невидимих" (5+ objects, genitive plural)
This implementation matches the pattern used elsewhere in the file and is actually more complete than some existing entries.
frontend/messages/sv.json (1)
1057-1067: LGTM! Plural handling correctly implemented.The ICU MessageFormat structure properly addresses the previous review concerns about Swedish plural adjective agreement. The adjective correctly changes from "synligt" (singular) to "synliga" (plural) based on the count, and the pattern matches existing plural entries in the file (lines 922-951).
frontend/messages/el.json (1)
1763-1773: Greek label and pluralization for non‑visible objects look correct
libraryFilteringLabelsis a clear translation, and the newobjectsNotVisibleICU block correctly handles singular/plural ("αντικείμενο/αντικείμενα", "ορατό/ορατά") and matches existing pluralization patterns (e.g.,cascadeAffectedNotice).frontend/messages/es.json (1)
3024-3034: Spanish width label and objects‑visibility pluralization are correct
width→ "Anchura" is appropriate, and the newobjectsNotVisibleICU entry cleanly fixes the earlier pluralization issue, with correct singular/plural agreement ("objeto no visible" / "objetos no visibles") and consistent declaration/selector structure.frontend/messages/it.json (1)
2056-2066: ItalianobjectsNotVisiblepluralization andyearkey are consistent and correct.The new
yearentry and the pluralizedobjectsNotVisiblemessage follow the existing inlang pluralization pattern and read naturally in Italian; no further changes needed.backend/ebios_rm/serializers.py (4)
33-58: LGTM! Field conversions to IdRelatedField are consistent.The conversion of multiple related fields to
IdRelatedFieldis correct and follows the PR's objective. The nested field specifications, including the complexvalidation_flowsfield with nested approver details, appear properly structured.
525-533: LGTM! Clean conversion to IdRelatedField.All field conversions in
OperatingModeReadSerializerfollow the consistent pattern established throughout the file.
582-591: LGTM! Clean conversion to IdRelatedField.All field conversions in
KillChainReadSerializerare consistent with the PR's objectives.
509-509: Theserializerparameter and fields list passed toIdRelatedFieldare intentionally ignored.The
IdRelatedFieldclass is designed to accept legacyFieldsRelatedFieldarguments (fields,serializer) for backward compatibility but explicitly ignores them (seebackend/core/serializer_fields.py). The["id", "name"]andserializer=ThreatReadSerializerparameters on line 509 have no functional effect and can be removed for cleaner code.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
backend/app_tests/api/test_utils.py (1)
38-43: Verify that list ordering should be ignored for non-dict lists.The comparison sorts lists when they don't contain dicts, which ignores the original order. While this is likely intentional for comparing ID collections (given the PR's context of replacing FieldsRelatedField with IdRelatedField), it could mask bugs in cases where list order is semantically significant.
Confirm this behavior aligns with your testing requirements across all use cases.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/app_tests/api/test_utils.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). (1)
- GitHub Check: Analyze (python)
🔇 Additional comments (1)
backend/app_tests/api/test_utils.py (1)
528-530: LGTM - Appropriate usage of the new comparison utilities.The replacements of direct equality assertions with
EndpointTestsUtils.values_equal()are correctly applied across the test methods. This pattern properly handles the variance in how related fields are represented (IDs vs. nested objects) following the PR's changes to use IdRelatedField.Once the missing
@staticmethoddecorators are added to the utility methods, these comparisons will function correctly.Also applies to: 683-687, 725-729, 834-838, 883-887
| def normalize_value(value): | ||
| if isinstance(value, str): | ||
| try: | ||
| return json.loads(value) | ||
| except (TypeError, json.JSONDecodeError): | ||
| return value | ||
| if isinstance(value, models.Model): | ||
| return str(value.id) | ||
| if isinstance(value, dict): | ||
| if "id" in value: | ||
| return str(value["id"]) | ||
| return value | ||
| if isinstance(value, (list, tuple, set)): | ||
| return [EndpointTestsUtils.normalize_value(item) for item in value] | ||
| return value |
There was a problem hiding this comment.
Missing @staticmethod decorator causes runtime error.
This method is called as EndpointTestsUtils.normalize_value(value) throughout the codebase (lines 32, 36, 528, 683, 725, 834, 883) but lacks the @staticmethod decorator. Without it, Python will raise a TypeError when the method is invoked.
🔎 Proposed fix
+ @staticmethod
def normalize_value(value):
if isinstance(value, str):
try:🤖 Prompt for AI Agents
In backend/app_tests/api/test_utils.py around lines 19 to 33, the
normalize_value method is defined as an instance method but is invoked as
EndpointTestsUtils.normalize_value(value) throughout the codebase, causing a
TypeError at runtime; fix it by marking the method as a staticmethod (add the
@staticmethod decorator directly above the def normalize_value line), ensure
imports remain correct, and run the test suite to verify all calls succeed.
| def values_equal(actual, expected): | ||
| actual_value = EndpointTestsUtils.normalize_value(actual) | ||
| expected_value = EndpointTestsUtils.normalize_value(expected) | ||
| if isinstance(actual_value, list) and isinstance(expected_value, list): | ||
| if all( | ||
| not isinstance(item, dict) for item in actual_value + expected_value | ||
| ): | ||
| return sorted(actual_value) == sorted(expected_value) | ||
| return actual_value == expected_value | ||
| return actual_value == expected_value |
There was a problem hiding this comment.
Missing @staticmethod decorator causes runtime error.
This method is called as EndpointTestsUtils.values_equal(actual, expected) throughout the codebase (lines 528, 683, 725, 834, 883) but lacks the @staticmethod decorator. Without it, Python will raise a TypeError when the method is invoked.
🔎 Proposed fix
+ @staticmethod
def values_equal(actual, expected):
actual_value = EndpointTestsUtils.normalize_value(actual)
expected_value = EndpointTestsUtils.normalize_value(expected)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def values_equal(actual, expected): | |
| actual_value = EndpointTestsUtils.normalize_value(actual) | |
| expected_value = EndpointTestsUtils.normalize_value(expected) | |
| if isinstance(actual_value, list) and isinstance(expected_value, list): | |
| if all( | |
| not isinstance(item, dict) for item in actual_value + expected_value | |
| ): | |
| return sorted(actual_value) == sorted(expected_value) | |
| return actual_value == expected_value | |
| return actual_value == expected_value | |
| @staticmethod | |
| def values_equal(actual, expected): | |
| actual_value = EndpointTestsUtils.normalize_value(actual) | |
| expected_value = EndpointTestsUtils.normalize_value(expected) | |
| if isinstance(actual_value, list) and isinstance(expected_value, list): | |
| if all( | |
| not isinstance(item, dict) for item in actual_value + expected_value | |
| ): | |
| return sorted(actual_value) == sorted(expected_value) | |
| return actual_value == expected_value | |
| return actual_value == expected_value |
🤖 Prompt for AI Agents
In backend/app_tests/api/test_utils.py around lines 35 to 44, the values_equal
method is defined as an instance method but is invoked as
EndpointTestsUtils.values_equal(...) across the codebase; add the @staticmethod
decorator above the def values_equal(...) declaration so it can be called on the
class without a self parameter and update imports/formatting if needed to keep
style consistent.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/messages/en.jsonfrontend/messages/fr.json
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/messages/fr.json
🧰 Additional context used
🪛 Biome (2.1.2)
frontend/messages/en.json
[error] 3031-3032: The key libraryFilteringLabels was already declared.
This where a duplicated key was declared again.
If a key is defined multiple times, only the last definition takes effect. Previous definitions are ignored.
(lint/suspicious/noDuplicateObjectKeys)
⏰ 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 (3.12)
- GitHub Check: build_enterprise_frontend
- GitHub Check: build_community_frontend
- GitHub Check: startup-docker-compose-test
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: Analyze (python)
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/messages/fr.json
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (python)
🔇 Additional comments (1)
frontend/messages/fr.json (1)
3030-3039: LGTM: Proper plural handling structure.The
objectsNotVisibletranslation correctly follows the established i18n message format pattern used elsewhere in the file (e.g.,hourCount,minuteCount,dayCount). The plural forms and French grammar are appropriate.
| "metricDefinitionsDescription": "Définir des métriques personnalisées pour suivre et mesurer les indicateurs de performance", | ||
| "metricInstancesDescription": "Suivre les valeurs de métriques pour les objets de votre organisation", | ||
| "dashboardsDescription": "Tableaux de bord personnalisés pour visualiser les métriques et indicateurs clés de performance", | ||
| "libraryFilteringLabels": "Étiquettes de bibliothèque", |
There was a problem hiding this comment.
Remove duplicate translation key.
The key libraryFilteringLabels is defined twice (lines 3029 and 3041) with identical values. JSON does not allow duplicate keys—the parser will only retain the last occurrence, making the first definition at line 3029 ineffective.
🔎 Proposed fix
Remove the duplicate entry at line 3041:
],
- "secretCopiedToClipboard": "Secret copié dans le presse-papier avec succès",
- "libraryFilteringLabels": "Étiquettes de bibliothèque"
+ "secretCopiedToClipboard": "Secret copié dans le presse-papier avec succès"
}Also applies to: 3041-3041
🤖 Prompt for AI Agents
In frontend/messages/fr.json around lines 3029 and 3041, the key
"libraryFilteringLabels" is duplicated; remove the duplicate entry at line 3041
so only one "libraryFilteringLabels" definition remains, then validate the JSON
(run the linter/JSON parser) to ensure no other duplicate keys or syntax errors.
There was a problem hiding this comment.
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 (3)
backend/core/serializers.py (1)
337-346: Nested field specifications in IdRelatedField are also ignored.Several
IdRelatedFielddeclarations use nested dictionary specifications for related fields (e.g.,{"approver": ["id", "email", "first_name", "last_name"]}). Like the simplefieldsparameter, these nested specifications are also ignored byIdRelatedFieldand only serve to make the code misleading.Examples:
- Lines 337-346:
validation_flows = IdRelatedField(many=True, fields=["id", "ref_id", "status", {"approver": [...]}])- Lines 1189-1198:
validation_flows = IdRelatedField(many=True, fields=["id", "ref_id", "status", {"approver": [...]}])- Lines 2971-3023: Multiple fields with nested dict specifications in
ValidationFlowReadSerializerThese complex field specifications suggest rich nested serialization but will only return a list of IDs.
Also applies to: 1189-1198, 1771-1780, 2971-3023
backend/core/views.py (2)
631-652: Fix UUID extraction logic inBaseModelViewSet.get_querysetThe regex group usage on line 643 will consistently crash with
TypeErrororValueError:
- The pattern has an optional prefix in group 1 and the UUID in group 2, but
UUID(q.group(1))uses the prefix (eitherNoneor a string like"names/"), not the UUID.- When
q.group(1)isNone(no prefix),UUID(None)raisesTypeError.- When
q.group(1)is"names/"or similar,UUID("names/")raisesValueError: invalid UUID.- Only
q.group(2)contains the actual UUID value and will successfully convert.Switch to
q.group(2)and optionally simplify the regex if multi-ID support isn't needed:- id = UUID(q.group(1)) + obj_id = UUID(q.group(2)) - if RoleAssignment.is_object_readable(self.request.user, self.model, id): - object_ids_view = [id] + if RoleAssignment.is_object_readable( + self.request.user, self.model, obj_id + ): + object_ids_view = [obj_id]If multi-ID support is needed, parse the comma-separated list explicitly rather than passing it directly to
UUID().
6955-6969: Fix RBAC bypass inFrameworkViewSet.namesand simplifyget_name_translatedhandlingTwo issues in this override:
RBAC is bypassed compared to
BaseModelViewSet.namesHere you do:
uuid_list = request.query_params.getlist("id[]", []) queryset = Framework.objects.filter(id__in=uuid_list)without going through
RoleAssignment.get_accessible_object_ids. This allowsframeworks/namesto return names for any IDs the caller guesses, even if the user has no view access to those frameworks, breaking the hidden object detection pattern implemented by the base class.Mirror the base implementation:
def names(self, request): uuid_list = request.query_params.getlist("id[]", [])
if not uuid_list:return Response({})parsed_ids = []for value in uuid_list:try:parsed_ids.append(UUID(value))except (TypeError, ValueError):continueif not parsed_ids:return Response({})viewable_ids = RoleAssignment.get_accessible_object_ids(Folder.get_root_folder(), request.user, Framework)[0]
queryset = Framework.objects.filter(id__in=uuid_list)
queryset = Framework.objects.filter(id__in=parsed_ids, id__in=viewable_ids)
get_name_translatedcheck is confusing and incorrectThe property always returns
str, making theisinstancecheck alwaysTrueand the else clause unreachable:str(framework.id): framework.get_name_translated if isinstance(framework.get_name_translated, str) else framework.get_name_translated() # ← dead codeSince
get_name_translatedis a property (not callable), simplify to:
return Response({str(framework.id): framework.get_name_translatedif isinstance(framework.get_name_translated, str)else framework.get_name_translated()for framework in queryset})
return Response({str(f.id): f.get_name_translated for f in queryset})
🧹 Nitpick comments (6)
backend/metrology/serializers.py (5)
27-30: Consider removing unused field specifications.The field specifications (e.g.,
["name", "id"]) passed toIdRelatedFieldare ignored per its implementation—it only returns primary key IDs. While these arguments are accepted for backward compatibility, they're misleading in new code.🔎 Proposed refactor to remove unused arguments
- folder = IdRelatedField() - library = IdRelatedField(["name", "id"]) - unit = IdRelatedField(["name", "id"]) - filtering_labels = IdRelatedField(["folder"], many=True) + folder = IdRelatedField() + library = IdRelatedField() + unit = IdRelatedField() + filtering_labels = IdRelatedField(many=True)
53-63: Complex field specification is unused and misleading.This nested structure including
{"unit": ["name", "id"]}is ignored byIdRelatedField—only the primary key ID is returned. While accepted for backward compatibility, this complex specification is particularly misleading.🔎 Proposed refactor
- metric_definition = IdRelatedField( - [ - "name", - "ref_id", - "id", - "category", - {"unit": ["name", "id"]}, - "choices_definition", - "higher_is_better", - ] - ) + metric_definition = IdRelatedField()
116-117: Field specification is unused.The
["name", "ref_id", "id"]specification is ignored byIdRelatedField. Consider simplifying toIdRelatedField()for clarity.🔎 Proposed refactor
folder = IdRelatedField() - metric_instance = IdRelatedField(["name", "ref_id", "id"]) + metric_instance = IdRelatedField()
143-144: Field specification is unused.The
["folder"]specification is ignored byIdRelatedField. Consider simplifying for clarity.🔎 Proposed refactor
folder = IdRelatedField() - filtering_labels = IdRelatedField(["folder"], many=True) + filtering_labels = IdRelatedField(many=True)
339-356: Highly complex field specification is unused and misleading.This 17-line nested structure is completely ignored by
IdRelatedField—only the ID is returned. This is the most misleading example in the file.🔎 Proposed refactor
- metric_instance = IdRelatedField( - [ - "name", - "ref_id", - "id", - "target_value", - { - "metric_definition": [ - "name", - "id", - "category", - {"unit": ["name", "id"]}, - "choices_definition", - "higher_is_better", - ] - }, - ] - ) + metric_instance = IdRelatedField()backend/core/serializers.py (1)
145-148: Remove misleading legacy parameters - they are intentionally ignored for backward compatibility.The
IdRelatedFieldclass acceptsfieldsandserializerparameters (lines 145, 157, 240, 333, etc.) but explicitly ignores them (seebackend/core/serializer_fields.py:28-36). This is intentional: the code comment states "Accepts legacy FieldsRelatedField arguments... and ignores them to keep call sites compatible."While the implementation is correct, the dead parameters should be removed throughout the file to improve code clarity and avoid confusion about what data is actually returned (always just the primary key).
# Current (confusing): perimeter = IdRelatedField(["id", "folder"]) # fields parameter ignored # Should be: perimeter = IdRelatedField() # Only returns pkThis applies to all occurrences where fields are passed to
IdRelatedField.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
backend/core/serializers.pybackend/core/views.pybackend/metrology/serializers.pyfrontend/messages/en.jsonfrontend/messages/fr.jsonfrontend/src/routes/(app)/(third-party)/compliance-assessments/[id=uuid]/+page.sveltefrontend/src/routes/(app)/(third-party)/requirement-assessments/[id=uuid]/edit/+page.svelte
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/messages/fr.json
- frontend/src/routes/(app)/(third-party)/requirement-assessments/[id=uuid]/edit/+page.svelte
🧰 Additional context used
🧠 Learnings (6)
📚 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/src/routes/(app)/(third-party)/compliance-assessments/[id=uuid]/+page.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/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
📚 Learning: 2025-10-12T21:53:47.742Z
Learnt from: ab-smith
Repo: intuitem/ciso-assistant-community PR: 2699
File: backend/core/serializers.py:414-422
Timestamp: 2025-10-12T21:53:47.742Z
Learning: In the asset hierarchy for CISO Assistant (backend/core/serializers.py), both PRIMARY and SUPPORT assets can have child_assets (supporting assets). PRIMARY assets are at the top of the hierarchy and cannot have parent_assets, while SUPPORT assets can have both parents and children.
Applied to files:
backend/core/serializers.py
📚 Learning: 2025-09-19T08:46:34.495Z
Learnt from: Mohamed-Hacene
Repo: intuitem/ciso-assistant-community PR: 2550
File: backend/core/serializers.py:1277-1292
Timestamp: 2025-09-19T08:46:34.495Z
Learning: In EvidenceWriteSerializer, the link and attachment fields are read-only, so updates to these fields should be done through EvidenceRevisionWriteSerializer to maintain proper versioning.
Applied to files:
backend/core/serializers.py
📚 Learning: 2025-09-19T09:08:52.306Z
Learnt from: Mohamed-Hacene
Repo: intuitem/ciso-assistant-community PR: 2550
File: backend/core/migrations/0098_evidence_lifecycle_data_migration.py:21-28
Timestamp: 2025-09-19T09:08:52.306Z
Learning: Evidence model inherits from FolderMixin making folder a mandatory field, and the lifecycle management handles folder propagation to revisions automatically.
Applied to files:
backend/core/serializers.py
🧬 Code graph analysis (3)
backend/core/views.py (2)
backend/iam/models.py (4)
Folder(80-328)get_root_folder(87-89)RoleAssignment(823-1116)get_accessible_object_ids(933-1037)backend/core/models.py (2)
get_name_translated(118-121)get_name_translated(1764-1773)
backend/metrology/serializers.py (1)
backend/core/serializer_fields.py (2)
IdRelatedField(24-45)PathField(48-95)
backend/core/serializers.py (1)
backend/core/serializer_fields.py (1)
IdRelatedField(24-45)
⏰ 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: startup-functional-test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: build (3.12)
- GitHub Check: test (3.12)
- GitHub Check: build_community_frontend
- GitHub Check: build_enterprise_frontend
- GitHub Check: Analyze (python)
🔇 Additional comments (12)
frontend/messages/en.json (1)
3057-3066: LGTM! Well-structured pluralization entry.The new
objectsNotVisibletranslation key is correctly implemented using ICU MessageFormat. The pluralization logic properly handles both singular and plural cases, and the structure is consistent with other pluralized entries in the file (e.g.,hourCount,dayCount,cascadeAffectedNotice). This addition aligns with the PR objective of implementing a pattern to detect and notify users about non-visible objects.frontend/src/routes/(app)/(third-party)/compliance-assessments/[id=uuid]/+page.svelte (3)
59-61: LGTM: Safe framework derivations.Good use of nullish coalescing with safe defaults (
[]) to prevent potential runtime errors when accessing nested framework properties.
379-379: LGTM: Consistent use of derived frameworkReferenceControls.Replacing direct nested property access with the derived
frameworkReferenceControlsimproves consistency and readability. Safe because it defaults to an empty array.Also applies to: 400-400, 411-411, 769-769
470-470: LGTM: Safe implementation groups check.Using the derived
implementationGroupsDefinitionwith proper array and length checks is a good defensive pattern and maintains consistency with other framework-related changes.backend/metrology/serializers.py (3)
6-6: LGTM! Import updated correctly.The import change from
FieldsRelatedFieldtoIdRelatedFieldaligns with the PR objective of simplifying related field handling.
88-95: Verify consistency: method returns nested data while related fields return IDs.The
get_unitmethod returns a nested dict{"id": ..., "name": ...}, whereas related fields now return only IDs. Confirm this mixed approach is intentional for the API contract.
25-456: Migration to IdRelatedField completed successfully.The serializers have been successfully updated to use
IdRelatedFieldthroughout, aligning with the PR objective. While the unused field specifications are misleading, they maintain backward compatibility during the transition. The implementation is correct and functional.backend/core/serializers.py (1)
11-11: Import change aligns with the architectural refactoring.The import of
IdRelatedFieldinstead ofFieldsRelatedFieldis correct and supports the shift to ID-based serialization throughout this file. However, be aware that this represents a significant API contract change (see detailed comments below on specific serializer fields).backend/core/views.py (4)
661-682:BaseModelViewSet.namesendpoint looks correct but is UUID‑onlyThe new
namesaction follows the intended pattern (parseid[]query params → validate as UUIDs → intersect with RBAC-filtered IDs fromRoleAssignment.get_accessible_object_ids→ return{id: str(obj)}), which is exactly what the frontend needs to detect hidden objects.Two minor points to be aware of:
- It silently ignores invalid UUID strings (they’re just skipped). That’s probably fine, but if debugging becomes painful, you may want to surface invalid inputs (e.g. in logs).
- It only works for UUID primary keys; for any model with an integer PK, all IDs will be dropped by the
UUID()parsing and the endpoint always returns{}. If you ever plug a non‑UUID model intoBaseModelViewSet, you’ll probably want to branch here on PK type.Otherwise, the logic is sound and respects RBAC.
1283-1295: ThreatViewSet queryset optimizations align with IdRelatedField usageAdding:
.select_related("library").prefetch_related("filtering_labels__folder")on top of the existing folder-related selects is consistent with the IdRelatedField refactor: both
libraryandfiltering_labelsnow carry extra metadata (including folder) that the serializers can use without N+1 queries.No functional or RBAC changes here; this is a straightforward performance improvement.
3806-3825: AppliedControlViewSet queryset prefetch additions are appropriateThe extra prefetches:
"filtering_labels__folder""evidences""security_exceptions"are all relations now exposed through IdRelatedField (or otherwise used in table views). Prefetching them on the base queryset is the right trade‑off to avoid N+1s on high‑traffic list endpoints, especially given AppliedControl’s central role.
No correctness or permission implications; just improved query behavior.
8963-8981: RequirementAssessmentViewSet queryset optimizations match new related-field usageThe added:
select_related("compliance_assessment__perimeter__folder")prefetch_related("evidences", "applied_controls", "security_exceptions")line up with how requirement assessments are now serialized (several of these relations are exposed as IdRelatedField or used in table columns). This should significantly reduce query churn on the RA list/detail views.
The changes do not alter filtering or permissions; they just front‑load necessary joins/prefetches.
| folder = IdRelatedField() | ||
| json_definition = serializers.JSONField(source="get_json_translated") | ||
| library = FieldsRelatedField(["name", "id"]) | ||
| library = IdRelatedField(["name", "id"]) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
File-wide: Remove ignored fields parameters for code clarity.
Throughout this file, IdRelatedField is called with a fields parameter (e.g., ["id", "folder"], ["name", "id"], ["id", "name", "is_locked"]), but this parameter is ignored by IdRelatedField.__init__(). These parameters create misleading code that suggests nested objects will be returned when only IDs are actually serialized.
Examples of misleading parameters:
- Line 157:
library = IdRelatedField(["name", "id"])→ only returns library ID - Line 331:
perimeter = IdRelatedField(["id", "folder"])→ only returns perimeter ID - Line 757:
risk_assessment = IdRelatedField(["id", "name", "is_locked"])→ only returns risk_assessment ID - Line 2960:
event_actor = IdRelatedField(["id", "email", "first_name", "last_name"])→ only returns actor ID
🔎 Recommended cleanup
Remove all fields parameters from IdRelatedField declarations to make the code honest about its behavior:
- folder = IdRelatedField() # already correct
- library = IdRelatedField(["name", "id"])
+ library = IdRelatedField()
- perimeter = IdRelatedField(["id", "folder"])
+ perimeter = IdRelatedField()This cleanup would affect approximately 50+ field declarations across all Read serializers in this file.
Also applies to: 193-197, 238-240, 266-268, 331-337, 483-494, 619-621, 676-678, 757-789, 948-972, 1025-1044, 1189-1198, 1412-1413, 1441-1443, 1479-1480, 1497-1502, 1617-1620, 1676-1683, 1720-1735, 1752-1780, 1946-1964, 2208-2208, 2254-2259, 2323-2336, 2370-2380, 2504-2507, 2522-2530, 2543-2550, 2725-2738, 2960-2960, 2970-3023, 3035-3037
🤖 Prompt for AI Agents
In backend/core/serializers.py around lines 155-157 (and many other locations
listed in the comment), IdRelatedField is being called with a fields parameter
that is ignored by its __init__; remove the misleading fields list arguments so
the declarations reflect that only the related ID is returned. Find all
occurrences of IdRelatedField([...]) throughout this file (examples: lines
193-197, 238-240, 266-268, 331-337, 483-494, 619-621, 676-678, 757-789, 948-972,
1025-1044, 1189-1198, 1412-1413, 1441-1443, 1479-1480, 1497-1502, 1617-1620,
1676-1683, 1720-1735, 1752-1780, 1946-1964, 2208, 2254-2259, 2323-2336,
2370-2380, 2504-2507, 2522-2530, 2543-2550, 2725-2738, 2960, 2970-3023,
3035-3037) and remove the list argument so calls become simply IdRelatedField()
(or keep any other valid positional/keyword args intact); run tests/lint to
ensure no breakage and update any inline comments if they implied nested fields.
frontend/src/routes/(app)/(third-party)/compliance-assessments/[id=uuid]/+page.svelte
Outdated
Show resolved
Hide resolved
|
The review process for this one will require extra care, let's not rush it. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/tests/utils/form-content.ts (1)
7-34: ReDoS warnings are acceptable in test context, but consider simplifying patterns.The static analysis tool flags potential ReDoS vulnerabilities due to dynamic regex construction. While
escapeRegExpsanitizes special characters, the.*prefix patterns (lines 26, 33) could theoretically cause backtracking on adversarial input.Risk assessment: Since this is test utility code operating on controlled test data (not user input), the practical risk is negligible. However, for completeness:
Return type inconsistency:
buildOptionMatcherdeclaresstring | RegExpbut always returnsRegExp. Consider updating the signature toRegExpfor clarity.Optional hardening: If you want to eliminate the static analysis warnings entirely, you could add a length limit on the input or use a non-backtracking regex library, but this is likely overkill for test utilities.
🔎 Optional: Fix return type annotation
-const buildOptionMatcher = (value: string): string | RegExp => { +const buildOptionMatcher = (value: string): RegExp => {
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/tests/utils/form-content.ts
🧰 Additional context used
🪛 ast-grep (0.40.3)
frontend/tests/utils/form-content.ts
[warning] 11-11: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(\\s*/?${escaped}\\s*)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 15-15: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(\\s*/?${escaped}\\s*)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 17-17: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(\\s*(?:/?${escaped}|/?${escapeRegExp(suffix)})\\s*)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 25-25: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^\\s*(?:/?${escaped}|.*${escapeRegExp(raw)})\\s*$)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 29-29: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^\\s*/?${escaped}\\s*$)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 32-32: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^\\s*(?:.*${escapeRegExp(suffix)})\\s*$)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
⏰ 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 (3.12)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: build_community_frontend
- GitHub Check: build_enterprise_frontend
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: startup-docker-compose-test
- GitHub Check: Analyze (python)
🔇 Additional comments (4)
frontend/tests/utils/form-content.ts (4)
3-5: LGTM on the escape utilities.Both
escapeRegExpandstripLeadingSlashare correctly implemented and provide the necessary sanitization for safe regex construction.
123-124: LGTM on searchbox matcher usage.The transition from exact text containment to
buildSearchboxMatchercorrectly handles the new dynamic label formats with path prefixes.
132-152: LGTM on option matcher integration.The refactored option selection logic correctly uses
buildOptionMatcherto handle both exact matches and UUID-prefixed suffixes. The.first()calls appropriately handle potential multiple matches.
159-165: LGTM on multi-select autocomplete handling.The matcher-based approach correctly iterates through values and clicks matching options. The pattern is consistent with the single-select implementation above.
Bumps [filelock](https://github.com/tox-dev/py-filelock) from 3.17.0 to 3.20.1. - [Release notes](https://github.com/tox-dev/py-filelock/releases) - [Changelog](https://github.com/tox-dev/filelock/blob/main/docs/changelog.rst) - [Commits](tox-dev/filelock@3.17.0...3.20.1) --- updated-dependencies: - dependency-name: filelock dependency-version: 3.20.1 dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…ibility of progress (#3103) * feat: support extended audit result * adapt frontend * include donut and better styling * flag to control the progress status as well * donut styling * update extended result choices * exta validations for consistency * fix tests * fix missing strings * fix typos * minor donut adjustment
* feat: link ISO objectives to custom metrics * fix migration order * format * update illustration
* starting point * backend first try * frontend first try * light rpc * backend tidying * add filters to configs viewset * passthrough for rpc * pain * filter out some of the noise in tables endpoint * some l10n * remove redundant field * use actual form instead of temporary mock object * fix choices display in autocomplete * better schema * i18n + simplifications * add some logging for outbound sync * fix stack trace information exposure * defensive dict attribute access * tweak timeouts * fix error logging * use get_object_or_404 for consistent 404 handling * fix typo * fix form value path * chore: format * move webhook authentication logic to orchestrator layer * fix snow allowed fields * remove debug prop * value mapping field name type safety * use webhook secret generator component in snow integration * allow dynamic fetching for snow tables * chore: format * remove except pass * fix stack trace exposure through response body * more resilient schema * add null check for remote_id before processing webhook events * add timeouts and improve logging * guard against missing keys in API response * better service account credentials protection * some l10n * fix: remote field mappings
There was a problem hiding this comment.
Actionable comments posted: 7
Fix all issues with AI Agents 🤖
In @backend/resilience/serializers.py:
- Around line 43-57: The issue is that bia = IdRelatedField(["id", "name",
"is_locked"]) and associated_controls = IdRelatedField(["id", "folder", "name",
"status", "eta"], many=True) currently return only IDs because IdRelatedField
explicitly ignores the legacy fields argument; to fix, change those declarations
to use a serializer-producing field (e.g., replace IdRelatedField(...) with the
existing nested serializer that returns the dict shape the frontend expects,
such as BiaSerializer or ControlSerializer with many=True for
associated_controls) or switch to FieldsRelatedField/RelatedSerializer that
accepts the fields list; alternatively, update IdRelatedField to honor the
fields/serializer argument so bia and associated_controls serialize as objects
(keep the same attribute names: bia and associated_controls) so frontend code
accessing .is_locked, .name and control.id continues to work.
In @frontend/messages/fr.json:
- Line 27: The French locale introduced lowercase compound keys
("addAppliedcontrol", "addComplianceassessment") while most locales (and the
code) still use camelCase ("addAppliedControl", "addComplianceAssessment"),
causing missing translations; either revert the two keys in fr.json to the
camelCase names or update all other locale files to use the new lowercase
compound convention consistently. Locate the translation keys
"addAppliedcontrol" and "addComplianceassessment" in the changed fr.json and
rename them back to "addAppliedControl" and "addComplianceAssessment" (or
perform the inverse rename across every other locale file to match the new
lowercase style), ensuring the key names match exactly what the frontend
references so the translation lookup and any toCamelCase fallback work
correctly.
In @frontend/src/lib/utils/crud.ts:
- Around line 1065-1066: The ProcessingReadSerializer is missing the evidences
relation causing API responses to omit evidences; in
backend/privacy/serializers.py add an evidences field to the
ProcessingReadSerializer by defining evidences = IdRelatedField(many=True)
(using the same IdRelatedField pattern as associated_controls) so the serializer
exposes the ManyToMany evidences relationship to the frontend.
In
@frontend/src/routes/(app)/(third-party)/compliance-assessments/[id=uuid]/+page.svelte:
- Around line 57-66: The ternary expression that computes folderId has a
formatting typo: the colon is immediately followed by "typeof" without a space;
update the expression in the folderId assignment (the code that checks
compliance_assessment.folder and uses typeof compliance_assessment.folder) to
insert a space after the colon so it reads ": typeof" to satisfy Prettier and
remove the pipeline formatting warning.
- Around line 598-621: The lookup for foreignKeyFields uses data.URLModel but
should use the already-defined model constant; update the itemHref computation
inside the each block so it references model['foreignKeyFields'] (or
model.foreignKeyFields) instead of URL_MODEL_MAP[data.URLModel], keeping the
rest of the logic (find by item.field === key and .urlModel / val.id) unchanged
and ensuring itemHref generation and anchor rendering still behave the same.
- Around line 583-590: The call to the message template objectsNotVisible passes
an unnecessary s parameter; remove the s key and invoke objectsNotVisible with
only the count (use hiddenCount) so pluralization is handled automatically by
the template, i.e., replace m.objectsNotVisible({ count: hiddenCount, s: ... })
with m.objectsNotVisible({ count: hiddenCount }).
In @frontend/tests/utils/form-content.ts:
- Line 1: The import statement "import { expect, type Locator, type Page } from
'./test-utils.js';" is failing Prettier checks; run the project's Prettier
formatter to auto-fix the file (e.g., run the project's Prettier command: pnpm
exec prettier --write <target file>), stage the formatted changes and commit
them so the pipeline formatting check passes.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/resilience/serializers.py (1)
18-30: Breaking API change: ValidationFlowsSection will receive only IDs instead of nested validation flow objects.The
IdRelatedFieldimplementation intentionally ignores thefieldsargument (lines 23-28), sovalidation_flowswill return only primary keys instead of the nested structures containingref_id,status, andapproverdetails that the component expects (ValidationFlowsSection.svelte lines 20-77).The frontend component accesses
validation_flow.ref_id,validation_flow.status, andvalidation_flow.approver.email/first_name/last_name, which will be undefined, causing the UI to fail at runtime.Verify:
- The approach aligns with the "dynamic fetch of related fields" design goal by adding a separate data-fetching mechanism to ValidationFlowsSection
- An API endpoint or data-loading function exists to fetch full validation flow details by ID
- Tests confirm the expected behavior with ID-only responses
- The
BusinessImpactAnalysisSchemaincludesvalidation_flowsto validate the response structure
🧹 Nitpick comments (3)
frontend/src/lib/utils/crud.ts (1)
1067-1067: Clarify or remove commented code.The commented field for
naturecreates ambiguity. Sincenatureappears in theselectFieldsarray (line 1059), it's likely a select field rather than a foreign key relationship. Consider either:
- Removing the commented line if it's no longer needed
- Uncommenting and completing the implementation if it's intended functionality
- Adding a TODO comment explaining why it's deferred
🔎 Proposed fix to remove commented code
{ field: 'associated_controls', urlModel: 'applied-controls' }, { field: 'evidences', urlModel: 'evidences' }, - // { field: 'nature', urlModel: 'processing-natures' }, { field: 'filtering_labels', urlModel: 'filtering-labels' }frontend/src/routes/(app)/(third-party)/compliance-assessments/[id=uuid]/+page.svelte (2)
538-538: Optional: Simplify redundant Array.isArray check.Line 66 ensures
implementationGroupsDefinitionis always an array (defaults to[]), so theArray.isArray(implementationGroupsDefinition)check on line 538 is redundant. You could simplify to:-if (key === 'selected_implementation_groups' && (!implementationGroupsDefinition || !Array.isArray(implementationGroupsDefinition) || implementationGroupsDefinition.length === 0)) return false; +if (key === 'selected_implementation_groups' && implementationGroupsDefinition.length === 0) return false;However, the defensive check doesn't cause issues and may protect against unexpected type changes.
554-637: Consider extracting rendering logic to helper functions.The rendering logic for arrays and single UUID values (lines 554-637) is deeply nested and complex. Consider extracting this into helper functions or a separate component to improve readability and maintainability:
renderIdOnlyArray(idValues, urlModel, resolvedNames, key)renderObjectArray(value, key)renderSingleUuid(value, urlModel, resolvedNames)This would make the template more declarative and easier to test.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
backend/resilience/serializers.pyfrontend/messages/en.jsonfrontend/messages/fr.jsonfrontend/src/lib/utils/crud.tsfrontend/src/routes/(app)/(third-party)/compliance-assessments/[id=uuid]/+page.sveltefrontend/tests/utils/form-content.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/messages/en.json
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: nas-tabchiche
Repo: intuitem/ciso-assistant-community PR: 1446
File: backend/core/migrations/0051_rename_project_perimeter_alter_perimeter_options_and_more.py:0-0
Timestamp: 2025-02-04T08:29:42.810Z
Learning: Using `migrations.RenameField` is preferred over add-migrate-remove pattern when renaming fields in Django as it preserves data and relationships at the database level without requiring explicit data migration.
📚 Learning: 2025-12-29T15:52:14.696Z
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:14.696Z
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/src/routes/(app)/(third-party)/compliance-assessments/[id=uuid]/+page.svelte
📚 Learning: 2025-10-12T21:53:47.742Z
Learnt from: ab-smith
Repo: intuitem/ciso-assistant-community PR: 2699
File: backend/core/serializers.py:414-422
Timestamp: 2025-10-12T21:53:47.742Z
Learning: In the asset hierarchy for CISO Assistant (backend/core/serializers.py), both PRIMARY and SUPPORT assets can have child_assets (supporting assets). PRIMARY assets are at the top of the hierarchy and cannot have parent_assets, while SUPPORT assets can have both parents and children.
Applied to files:
backend/resilience/serializers.py
🧬 Code graph analysis (2)
frontend/tests/utils/form-content.ts (1)
frontend/tests/utils/test-utils.ts (1)
expect(470-515)
backend/resilience/serializers.py (3)
backend/core/serializer_fields.py (1)
IdRelatedField(24-45)backend/resilience/models.py (1)
risk_matrix(214-215)backend/core/models.py (1)
children_assets(2693-2696)
🪛 ast-grep (0.40.3)
frontend/tests/utils/form-content.ts
[warning] 21-21: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(\\s*/?${escaped}\\s*)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 25-25: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(\\s*/?${escaped}\\s*)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 27-27: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(\\s*(?:/?${escaped}|/?${suffix})\\s*)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 40-40: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^\\s*(?:\\d+-)?.*${escaped}.*$, 'i')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🪛 GitHub Actions: Frontend Linters
frontend/tests/utils/form-content.ts
[warning] 1-1: Prettier formatting issues detected. Run 'pnpm exec prettier --write' to fix in this file.
frontend/src/routes/(app)/(third-party)/compliance-assessments/[id=uuid]/+page.svelte
[warning] 1-1: Prettier formatting issues detected. Run 'pnpm exec prettier --write' to fix in this file.
⏰ 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). (10)
- GitHub Check: build (22)
- GitHub Check: startup-docker-compose-test
- GitHub Check: test (3.12)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: build_community_frontend
- GitHub Check: build_enterprise_frontend
- GitHub Check: build (3.12)
- GitHub Check: Analyze (python)
🔇 Additional comments (14)
frontend/tests/utils/form-content.ts (5)
3-15: LGTM! Well-designed utility functions.The helper functions are straightforward and handle edge cases appropriately (empty segments, trimming, etc.).
17-29: Note the ReDoS warnings from static analysis.Static analysis flagged potential ReDoS vulnerabilities when constructing RegExp from variables. While the input is escaped via
escapeRegExp, the patterns themselves (especially\\s*quantifiers) could theoretically cause backtracking issues.However, in this test context where input comes from controlled test data rather than untrusted user input, the risk is minimal and acceptable.
As per static analysis hints, consider this a low-priority observation for test code.
31-42: Flexible matcher logic handles dynamic option labels well.The regex pattern correctly accommodates various label formats (index-prefixed, UUID-suffixed, breadcrumb paths). The same ReDoS consideration applies here as mentioned earlier, but remains acceptable in the test context.
124-163: Matcher-based approach improves test resilience to dynamic labels.The switch from exact string matching to regex matchers accommodates the dynamic option labels introduced by the IdRelatedField refactor. Using
.first()is appropriate here to handle potential multiple matches while maintaining test stability.
165-182: Consistent matcher usage for multiple selection.The implementation correctly applies the matcher pattern to each value in the multiple-select case, maintaining consistency with the single-select approach.
frontend/messages/fr.json (3)
2434-2974: New permission translation keys added - verify completeness.A large set of permission-related translation keys has been added (approximately 540 new entries). These follow a systematic pattern with prefixes:
add*- for creation actionschange*- for modification actionsdelete*- for deletion actionsview*- for viewing actionsThe keys use lowercase compound naming (e.g.,
addAssetcapability,changeBusinessimpactanalysis). Please verify:
- All permission keys are present and correctly spelled
- The translations accurately reflect the intended actions
- The naming convention is consistent with the backend permission system
3264-3273: LGTM! Pluralization structure is well-formed.The
objectsNotVisibleentry correctly implements ICU MessageFormat pluralization for French:
- Singular form: "{count} objet non visible."
- Plural form: "{count} objets non visible."
The structure properly declares input variables, selectors, and match cases for handling different count scenarios. This will provide proper localization for object visibility messages.
3263-3263: The duplicatelibraryFilteringLabelskey from the previous review has been resolved. The file now contains only a single occurrence at line 3263, and no duplicate keys exist in the JSON structure.frontend/src/routes/(app)/(third-party)/compliance-assessments/[id=uuid]/+page.svelte (4)
5-5: LGTM! Proper imports for new related-names functionality.The
browserimport is correctly used to gate client-side-only operations, and the related-names utilities are appropriately imported for the new dynamic name resolution feature.Also applies to: 43-43
70-84: LGTM! Permission checks correctly use derived folderId.The permission checks now use the safely-derived
folderIdinstead of accessingcompliance_assessment.folder.iddirectly, which properly handles null/undefined folder values.
447-447: LGTM! Consistent use of derived frameworkReferenceControls.The replacement of direct
framework.reference_controlsaccess with the safely-derivedframeworkReferenceControls(which defaults to an empty array) ensures consistent behavior across all usages and prevents potential null reference errors.Also applies to: 468-468, 479-479, 900-900
86-147: The code is correct as written. No changes are needed.$effect reactive dependencies: Svelte 5 automatically tracks reactive dependencies, so explicit dependencies are unnecessary. The key-based caching (lines 130-134) with the early return condition prevents redundant fetches when data hasn't changed, making the current implementation correct.
compliance_assessment null safety:
compliance_assessmentis guaranteed to exist because it comes from the server-side load function, which would throw if the API call fails. The component accesses it directly throughout without null checks. The optional chaining at line 124 correctly protects the field access, notcompliance_assessmentitself, which is the appropriate defensive pattern.backend/resilience/serializers.py (2)
2-2: Import change is correct, but note the semantic shift.The import has been correctly updated from
FieldsRelatedFieldtoIdRelatedField. However, be aware thatIdRelatedFieldonly returns primary keys (IDs), whereasFieldsRelatedFieldreturned nested field data. This is an intentional breaking change per the PR objectives, but ensure all API consumers are prepared for ID-only responses.
93-97: No action needed. TheIdRelatedFieldclass is designed to always return only primary keys, regardless of whether nested field lists are specified. The frontend schema already expects ID-only responses for these fields (asset_assessment: z.string()andqualifications: z.string().uuid().array()), and the backend validation code correctly uses them as references.
| { field: 'associated_controls', urlModel: 'applied-controls' }, | ||
| { field: 'evidences', urlModel: 'evidences' }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find all Python serializer files, focusing on Processing model
find . -type f -name "*.py" | xargs grep -l "class.*Processing.*Serializer" 2>/dev/null | head -20Repository: intuitem/ciso-assistant-community
Length of output: 109
🏁 Script executed:
# Also search for any serializer files in common backend locations
fd -type f -name "serializers.py" 2>/dev/nullRepository: intuitem/ciso-assistant-community
Length of output: 59
🏁 Script executed:
# Search for the specific fields in serializer context
rg "associated_controls|evidences" -t py --max-count 20Repository: intuitem/ciso-assistant-community
Length of output: 16134
🏁 Script executed:
# Search for Processing model and serializer definition
rg "class Processing" -t py -A 3 -B 1Repository: intuitem/ciso-assistant-community
Length of output: 2381
🏁 Script executed:
# Check the privacy serializers file for Processing class
cat backend/privacy/serializers.pyRepository: intuitem/ciso-assistant-community
Length of output: 4438
Add missing evidences field to ProcessingReadSerializer.
The backend Processing serializer is incomplete. While associated_controls is correctly exposed via IdRelatedField(["name"], many=True), the evidences field is missing. The Processing model defines both fields as ManyToManyFields, but only associated_controls is currently exposed in ProcessingReadSerializer. Add the following to backend/privacy/serializers.py in the ProcessingReadSerializer class:
evidences = IdRelatedField(many=True)
Without this, the frontend code expecting to fetch related evidences through the API will fail.
🤖 Prompt for AI Agents
In @frontend/src/lib/utils/crud.ts around lines 1065-1066, The
ProcessingReadSerializer is missing the evidences relation causing API responses
to omit evidences; in backend/privacy/serializers.py add an evidences field to
the ProcessingReadSerializer by defining evidences = IdRelatedField(many=True)
(using the same IdRelatedField pattern as associated_controls) so the serializer
exposes the ManyToMany evidences relationship to the frontend.
frontend/src/routes/(app)/(third-party)/compliance-assessments/[id=uuid]/+page.svelte
Outdated
Show resolved
Hide resolved
| {:else} | ||
| <ul> | ||
| {#each value as val} | ||
| <li> | ||
| {#if val?.str && val?.id} | ||
| {@const itemHref = `/${ | ||
| URL_MODEL_MAP[data.URLModel]['foreignKeyFields']?.find( | ||
| (item) => item.field === key | ||
| )?.urlModel | ||
| }/${val.id}`} | ||
| {#if !page.data.user.is_third_party} | ||
| <Anchor href={itemHref} class="anchor">{val.str}</Anchor> | ||
| {:else} | ||
| {val.str} | ||
| {/if} | ||
| {:else if val?.str} | ||
| {safeTranslate(val.str)} | ||
| {:else} | ||
| {val.str} | ||
| {safeTranslate(val)} | ||
| {/if} | ||
| {:else} | ||
| {val} | ||
| {/if} | ||
| </li> | ||
| {/each} | ||
| </ul> | ||
| </li> | ||
| {/each} | ||
| </ul> | ||
| {/if} |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use the model constant instead of data.URLModel for consistency.
Lines 604-606 look up foreignKeyFields using data.URLModel, but the model constant is already defined on line 69 as URL_MODEL_MAP['compliance-assessments']. For consistency with the rest of the file (e.g., lines 90, 119), use the model constant:
🔎 Proposed fix
{@const itemHref = `/${
- URL_MODEL_MAP[data.URLModel]['foreignKeyFields']?.find(
+ model.foreignKeyFields?.find(
(item) => item.field === key
)?.urlModel
}/${val.id}`}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {:else} | |
| <ul> | |
| {#each value as val} | |
| <li> | |
| {#if val?.str && val?.id} | |
| {@const itemHref = `/${ | |
| URL_MODEL_MAP[data.URLModel]['foreignKeyFields']?.find( | |
| (item) => item.field === key | |
| )?.urlModel | |
| }/${val.id}`} | |
| {#if !page.data.user.is_third_party} | |
| <Anchor href={itemHref} class="anchor">{val.str}</Anchor> | |
| {:else} | |
| {val.str} | |
| {/if} | |
| {:else if val?.str} | |
| {safeTranslate(val.str)} | |
| {:else} | |
| {val.str} | |
| {safeTranslate(val)} | |
| {/if} | |
| {:else} | |
| {val} | |
| {/if} | |
| </li> | |
| {/each} | |
| </ul> | |
| </li> | |
| {/each} | |
| </ul> | |
| {/if} | |
| {:else} | |
| <ul> | |
| {#each value as val} | |
| <li> | |
| {#if val?.str && val?.id} | |
| {@const itemHref = `/${ | |
| model.foreignKeyFields?.find( | |
| (item) => item.field === key | |
| )?.urlModel | |
| }/${val.id}`} | |
| {#if !page.data.user.is_third_party} | |
| <Anchor href={itemHref} class="anchor">{val.str}</Anchor> | |
| {:else} | |
| {val.str} | |
| {/if} | |
| {:else if val?.str} | |
| {safeTranslate(val.str)} | |
| {:else} | |
| {safeTranslate(val)} | |
| {/if} | |
| </li> | |
| {/each} | |
| </ul> | |
| {/if} |
🤖 Prompt for AI Agents
In
@frontend/src/routes/(app)/(third-party)/compliance-assessments/[id=uuid]/+page.svelte
around lines 598-621, The lookup for foreignKeyFields uses data.URLModel but
should use the already-defined model constant; update the itemHref computation
inside the each block so it references model['foreignKeyFields'] (or
model.foreignKeyFields) instead of URL_MODEL_MAP[data.URLModel], keeping the
rest of the logic (find by item.field === key and .urlModel / val.id) unchanged
and ensuring itemHref generation and anchor rendering still behave the same.
| @@ -1,5 +1,46 @@ | |||
| import { expect, type Locator, type Page } from './test-utils.js'; | |||
There was a problem hiding this comment.
Fix Prettier formatting before merge.
The pipeline detected formatting issues. Please run:
pnpm exec prettier --write frontend/tests/utils/form-content.ts🧰 Tools
🪛 GitHub Actions: Frontend Linters
[warning] 1-1: Prettier formatting issues detected. Run 'pnpm exec prettier --write' to fix in this file.
🤖 Prompt for AI Agents
In @frontend/tests/utils/form-content.ts around line 1, The import statement
"import { expect, type Locator, type Page } from './test-utils.js';" is failing
Prettier checks; run the project's Prettier formatter to auto-fix the file
(e.g., run the project's Prettier command: pnpm exec prettier --write <target
file>), stage the formatted changes and commit them so the pipeline formatting
check passes.
14cf75d to
3c28304
Compare
* feat: Unify library model tables * Apply missing translations * Attempt to fix codefactor * Remove debug line * Fix wrong error message * Complete missing fields * Fix migration conflict * Formatter * Add missing is_published value set * Add LibraryFilteringLabel + some filters * Add library list view related dead code * Formatter * Simplest fix attempt * Real good fix for library loading * Better UI and make quick filters compatible with filter persistance * Fix functional tests * Fix function tests * Improve spanish translation * Naive test fix attempt * Remove mapping_suggested filter * Rename YAML filtering_labels key to labels * Remove wrong dead code * Add labels in detail view * Delete fake stored libraries on unload * Remove useless * Fix wrong snippet typing * Better exception handling
* feat(epic): custom metrics * first step * fixup * fix migrations order * component for ordered list * fixup * implementing orderedlist on metric definition component * simplify model * fixup * wip * wip * ok * fixup * better management of embedded tables * fixup * add dashboard object * fix migrations * fix migrations * first steps for dashboard * good progress * wip * wip * fixup * snapping more consistent and different buttons colors * handle higher is better on quant metrics * fixup * better contrast for dashboards * add/delete rows * stable point * fixup * cleanup * simplify routing * fixup * fix deletion issue * fix new row issue * cleanup * fix unit * fixup * allow editing choices for qualitative metrics * fixup * a step with builtin metrics * support breakdown on tables and donut * simplifications * error management * fixup * more simplifications * format * support default_target for definition * support metric definition through library * simplify migrations * fixes * first sample of metrics * update migrations * add metrology app to EE * fixup to be able to edit the widget of built-in metrics * fixup * fixup * fixup * fixup * fix migrations * fix text widget issues * add backfill to startup.py * uniqueness and import lib capability * add staleness check * format * add assignments * fixup * better colors and strings * format * fixup * fixup * missing navdata for EE * fix api tests * tests fixup * fixup * wip * fix migrations * side change for asset ref_id on BIA * add metrics to library * fixup * fix test regression * increase timeout for domain import test
…trategic scenario (#3099) * feat: ebios rm - allow users to focus on a specific feared event on strategic scenario * coderabbit feedback * restrict graph when a focused on a feared event
enhance and fix webhook form
* feat: link ISO objectives to custom metrics * fix migration order * format * update illustration
* starting point * backend first try * frontend first try * light rpc * backend tidying * add filters to configs viewset * passthrough for rpc * pain * filter out some of the noise in tables endpoint * some l10n * remove redundant field * use actual form instead of temporary mock object * fix choices display in autocomplete * better schema * i18n + simplifications * add some logging for outbound sync * fix stack trace information exposure * defensive dict attribute access * tweak timeouts * fix error logging * use get_object_or_404 for consistent 404 handling * fix typo * fix form value path * chore: format * move webhook authentication logic to orchestrator layer * fix snow allowed fields * remove debug prop * value mapping field name type safety * use webhook secret generator component in snow integration * allow dynamic fetching for snow tables * chore: format * remove except pass * fix stack trace exposure through response body * more resilient schema * add null check for remote_id before processing webhook events * add timeouts and improve logging * guard against missing keys in API response * better service account credentials protection * some l10n * fix: remote field mappings
…ntuitem/ciso-assistant-community into CA-1456-make-domain-changeable
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
frontend/src/lib/components/Forms/WebhookSecretGenerator.svelte (1)
35-56: Remove duplicate function declaration.The
copySecretfunction is declared twice with identical implementations. This causes a syntax error as confirmed by the pipeline failure.🔎 Proposed fix
Remove one of the duplicate declarations. Keep only the first one:
function copySecret() { if (!generatedSecret) return; navigator.clipboard.writeText(generatedSecret).then(() => { flash.set({ type: 'success', message: m.secretCopiedToClipboard() }); }); - } - - function copySecret() { - if (!generatedSecret) return; - - navigator.clipboard.writeText(generatedSecret).then(() => { - flash.set({ - type: 'success', - message: m.secretCopiedToClipboard() - }); - }); - }frontend/messages/fr.json (1)
2434-2979: Bulk addition of ~200+ translation keys uses inconsistent lowercase compound naming.The newly added translation keys from lines 2434-2979 follow a lowercase compound pattern (e.g.,
addAssetassessment,changeAppliedcontrol,viewBusinessimpactanalysis) instead of the camelCase convention used throughout the rest of this file and other locale files.Examples of inconsistency:
- Line 2929:
addAssetassessment→ should beaddAssetAssessment- Line 2975:
changeAppliedcontrol→ should bechangeAppliedControl- Line 3026:
deleteAppliedcontrol→ should bedeleteAppliedControl- Line 3080:
viewAttackpath→ should beviewAttackPathThese keys represent CRUD operations (add, change, delete, view) for various models and should follow the same camelCase pattern as existing keys like
addAsset(line 29),addReferenceControl(line 26), andchangePassword(line 61).Apply camelCase naming consistently to all newly added keys to maintain translation file integrity across all locales.
backend/core/models.py (1)
5085-5117: Bug: key type mismatch prevents existing-control checks in RiskAssessment.quality_check
scenario_objectsis keyed bystr(s.id), but lookups use the integerri["id"]from the JSON‐decoded scenarios:scenario_objects = { str(s.id): s for s in self.risk_scenarios.prefetch_related( "existing_applied_controls", "applied_controls" ).all() } for ri in scenarios: scenario_obj = scenario_objects.get(ri["id"]) if not scenario_obj: continueBecause
ri["id"]is anint,scenario_objects.get(ri["id"])will always returnNone, so:
- The duplicate check between
existing_applied_controlsandapplied_controls.- The “existing control not active” validation.
never actually run.
Aligning key types fixes this:
Proposed fix: key by integer IDs
- scenario_objects = { - str(s.id): s - for s in self.risk_scenarios.prefetch_related( - "existing_applied_controls", "applied_controls" - ).all() - } + scenario_objects = { + s.id: s + for s in self.risk_scenarios.prefetch_related( + "existing_applied_controls", "applied_controls" + ).all() + } @@ - for ri in scenarios: - scenario_obj = scenario_objects.get(ri["id"]) + for ri in scenarios: + scenario_obj = scenario_objects.get(ri["id"])frontend/src/lib/utils/table.ts (1)
1059-1066: Duplicate label with IS_LOADED_FILTER.Both
IS_UPDATE_FILTER(line 1062) andIS_LOADED_FILTER(line 1053) now have the label'loadedLibraries'. This creates ambiguity since these filters serve different purposes. Consider using a distinct label like'updateAvailable'(the original label) or'hasUpdate'for theIS_UPDATE_FILTERto clearly differentiate it fromIS_LOADED_FILTER.
🤖 Fix all issues with AI Agents
In @frontend/messages/en.json:
- Line 27: Restore the inconsistent lowercase translation keys to camelCase
(e.g., rename "addAppliedcontrol" back to "addAppliedControl") and do the same
for the other affected keys noted in the comment (the ones accidentally
lowercased at the other locations), ensuring they match the existing convention
used by keys like "addEbiosRmStudy"; update any code references/usages to the
corrected camelCase keys so lookups remain valid and run a quick grep across the
project to catch any remaining lowercase variants.
- Around line 3037-3241: The translation keys in this diff use inconsistent
lowercase naming (e.g., addAssetclass, addAttackpath, changeAppliedcontrol) and
must be converted to camelCase to match the rest of the file and JS/TS
conventions; update each key in this block to camelCase (e.g., addAssetclass ->
addAssetClass, addAttackpath -> addAttackPath, changeAppliedcontrol ->
changeAppliedControl) while keeping the corresponding string values unchanged,
and apply the same transformation to all similarly named keys in this range
(such as addEbiosrmstudy -> addEbiosRmStudy and changeQuantitativeriskstudy ->
changeQuantitativeRiskStudy) so naming is consistent across the file.
In @frontend/messages/fr.json:
- Line 27: The French locale contains two keys with incorrect casing—rename the
keys addAppliedcontrol and addComplianceassessment back to the standard
camelCase names addAppliedControl and addComplianceAssessment to match the other
19 locales; update the key names (not the values) so translation lookups use the
same identifiers as the frontend, and verify the same two keys are corrected
wherever they were changed inconsistently.
In @frontend/src/lib/components/ModelTable/ModelTable.svelte:
- Around line 242-270: The refreshRelatedNames function currently awaits
fetchNames for each urlModel without error handling, so a failing network/API
call can abort the whole update; wrap the per-model fetch loop (the for (const
[urlModel, ids] of idsByModel.entries()) block) in a try-catch, on success set
updates[urlModel] = await fetchNames(urlModel, Array.from(ids)), on failure log
the error (e.g., console.error or existing logger) including urlModel and
continue so other models can still resolve; keep the final merge into
relatedNames and preserve relatedNamesKey behavior so partial results are
applied while failures are non-fatal.
🧹 Nitpick comments (10)
backend/core/models.py (2)
4925-4941: Domain mismatch checks for RiskAssessment are coherent; consider centralizing helperThe added root/assessment folder context and
is_domain_mismatchlogic correctly:
- Short‑circuit when folder info is missing.
- Treat the root folder as domain‑neutral on either side.
- Compare folder ids consistently as strings.
The downstream usages for applied controls, risk acceptances, assets, and evidences use the appropriate folder/source (serialized
folderfield orfolder_id) and avoid duplicate messages per applied control viadomain_checked_controls.Given the identical helper and patterns are also introduced in
ComplianceAssessment.quality_check, it would be cleaner to extract a shared utility (e.g. a small function orAssessment-level method takingassessment_folder_idandother_folder_id) to avoid divergence on future changes and make the “domain mismatch” definition explicit in one place.Also applies to: 5145-5163, 5237-5248, 5275-5315
6381-6398: ComplianceAssessment domain mismatch pattern matches RA and looks correct; helper duplication remainsThe new
is_domain_mismatchhelper and its usages here are consistent with the RiskAssessment implementation:
- Root folder is treated as neutral (no mismatch if either side is root).
- Folder ids are normalized via
str(...)to avoid type issues.- Requirement assessments, applied controls (via
applied_control_qs), evidences (via both applied controls and direct M2M to the assessment), and assets all use the right folder source (folder_idor serializedfolder) for checks.domain_checked_controlsavoids duplicate “applied control different branch” info entries.Functionally this aligns well with the stated goal of surfacing cross‑branch links.
As with the RA version, the helper and patterns are now duplicated across two methods; centralizing the folder‑comparison logic into a small reusable function (or
Assessmentmixin method) would reduce drift and make the domain semantics easier to reason about in one place.Also applies to: 6433-6445, 6484-6486, 6489-6489, 6495-6495, 6497-6514, 6531-6532, 6560-6582, 6584-6598
backend/core/tests/test_models.py (3)
395-404: Consider aligning root folder retrieval with existing test patterns.The defensive approach using
Folder.get_root_folder()with fallback creation differs from the pattern used elsewhere in this file (e.g., lines 782, 991, 1665), where tests directly callFolder.objects.get(content_type=Folder.ContentType.ROOT). While your approach is more defensive, consistency across the test suite would improve maintainability.🔎 Consider this pattern used elsewhere
def _get_root_folder(self): - root_folder = Folder.get_root_folder() - if not root_folder: - root_folder = Folder.objects.create( - name="root", - description="root folder", - content_type=Folder.ContentType.ROOT, - parent_folder=None, - ) - return root_folder + return Folder.objects.get(content_type=Folder.ContentType.ROOT)
406-467: Well-structured test for cross-domain risk assessment quality checks.The test correctly sets up a cross-domain scenario and verifies that the quality check detects and reports the expected informational messages for applied controls, risk acceptances, assets, and evidences that belong to different domain branches.
Consider adding a complementary negative test case that verifies no domain mismatch warnings are generated when all related objects are within the same domain. This would provide more confidence that the quality check logic correctly distinguishes between same-domain and cross-domain scenarios.
469-531: Good coverage of compliance assessment cross-domain quality checks.The test appropriately validates that cross-domain links in compliance assessments are detected and reported, covering requirements, applied controls, evidences, and assets.
The setup code for root and domain folders (lines 470-480) duplicates the pattern from the previous test method. Consider extracting this common setup into a pytest fixture or a
setUpmethod to reduce duplication and improve maintainability.🔎 Example fixture approach
@pytest.fixture def cross_domain_folders(self): root_folder = self._get_root_folder() folder_a = Folder.objects.create( name="domain-a", description="domain a", parent_folder=root_folder, ) folder_b = Folder.objects.create( name="domain-b", description="domain b", parent_folder=root_folder, ) return root_folder, folder_a, folder_bThen use it in your tests:
def test_risk_assessment_quality_check_warns_on_cross_domain_links(self, cross_domain_folders): root_folder, folder_a, folder_b = cross_domain_folders # rest of test...frontend/src/lib/components/Forms/ModelForm/EvidenceRevisionForm.svelte (1)
77-92: Consider removing commented-out code.The commented-out block shows the old implementation logic but adds clutter to the codebase. Since the new implementation is working and the PR is finalizing this change, consider removing these commented lines for cleaner code maintenance.
backend/core/serializers.py (4)
76-88: Verify folder change prevention logic for builtin objects.The validation correctly prevents folder changes for builtin objects, but the string comparison of UUID folder IDs (line 85) is unconventional. Django typically compares UUID fields directly without string conversion. Consider simplifying:
- if new_folder_id and str(new_folder_id) != str(instance.folder_id): + if new_folder_id is not None and new_folder_id != instance.folder_id:This is more idiomatic and avoids unnecessary string conversions. Also note that checking
if new_folder_idcould evaluate toFalsefor a falsy non-None value, though unlikely with UUIDs.
2678-2695: Folder cascading is correct; consider explicittransaction.on_commit()for consistency.The folder cascade to TaskNode objects (lines 2684-2687) is correctly implemented within the atomic transaction.
Notifications are sent immediately after the atomic block (lines 2690-2695), which works correctly since the transaction has committed. However, for consistency with
ComplianceAssessmentWriteSerializer.update()(lines 1894-1899), consider usingtransaction.on_commit()to make the intent more explicit:🔎 Optional refactor for consistency
+ # Schedule notifications to run **after transaction commits** if newly_assigned_ids: - self._send_assignment_notifications(instance, list(newly_assigned_ids)) + transaction.on_commit( + lambda: self._send_assignment_notifications( + instance, list(newly_assigned_ids) + ) + )
2323-2347: Consider if FindingsAssessment should cascade folder changes to Finding objects.
FindingWriteSerializer.create()(lines 2392-2398) sets the folder fromfindings_assessment.folder, establishing a parent-child folder relationship. However,FindingsAssessmentWriteSerializer.update()does not cascade folder changes to relatedFindingobjects, unlike the pattern used for Evidence, ComplianceAssessment, Incident, TaskTemplate, and ValidationFlow.If findings should follow their assessment's folder, consider adding similar cascading logic:
🔎 Example folder cascade for FindingsAssessment
def update(self, instance, validated_data): + old_folder_id = instance.folder_id + # Check if status is changing to deprecated old_status = instance.status new_status = validated_data.get("status", old_status) # Auto-lock when status changes to deprecated if old_status != "deprecated" and new_status == "deprecated": validated_data["is_locked"] = True - return super().update(instance, validated_data) + with transaction.atomic(): + updated_instance = super().update(instance, validated_data) + + # Cascade folder change to findings + if old_folder_id != updated_instance.folder_id: + Finding.objects.filter(findings_assessment=updated_instance).update( + folder=updated_instance.folder + ) + + return updated_instance
2943-2977: Extract folder cascade logic to reduce duplication.The folder cascade to
FlowEventobjects is duplicated at lines 2962–2965 and 2972–2975. While functionally correct, extracting this into a helper method improves maintainability and aligns with the DRY principle.The
transaction.atomic()blocks are necessary and intentional—they ensure atomicity ofsuper().update()combined with cascading updates. This pattern is used consistently across other serializers in the codebase (e.g., Evidence, Incident, TaskTemplate).Consider extracting the cascade logic:
Suggested refactor
+ def _cascade_folder_to_events(self, instance, old_folder_id): + """Cascade folder changes to related FlowEvent objects.""" + if old_folder_id != instance.folder_id: + FlowEvent.objects.filter(validation_flow=instance).update( + folder=instance.folder + ) + def update(self, instance: ValidationFlow, validated_data: dict) -> ValidationFlow: ... if "status" in validated_data: ... with transaction.atomic(): updated_instance = super().update(instance, validated_data) FlowEvent.objects.create(...) self._manage_associated_objects_lock(...) - if old_folder_id != updated_instance.folder_id: - FlowEvent.objects.filter(validation_flow=updated_instance).update( - folder=updated_instance.folder - ) + self._cascade_folder_to_events(updated_instance, old_folder_id) return updated_instance with transaction.atomic(): updated_instance = super().update(instance, validated_data) - if old_folder_id != updated_instance.folder_id: - FlowEvent.objects.filter(validation_flow=updated_instance).update( - folder=updated_instance.folder - ) + self._cascade_folder_to_events(updated_instance, old_folder_id) return updated_instance
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (48)
backend/app_tests/api/test_api_perimeters.pybackend/core/models.pybackend/core/serializers.pybackend/core/tests/test_models.pybackend/crq/serializers.pybackend/ebios_rm/serializers.pybackend/privacy/serializers.pybackend/resilience/serializers.pyfrontend/messages/en.jsonfrontend/messages/fr.jsonfrontend/src/lib/components/Forms/ModelForm/AccreditationForm.sveltefrontend/src/lib/components/Forms/ModelForm/AssetForm.sveltefrontend/src/lib/components/Forms/ModelForm/BusinessImpactAnalysisForm.sveltefrontend/src/lib/components/Forms/ModelForm/CampaignForm.sveltefrontend/src/lib/components/Forms/ModelForm/ComplianceAssessmentForm.sveltefrontend/src/lib/components/Forms/ModelForm/ContractForm.sveltefrontend/src/lib/components/Forms/ModelForm/CustomMetricSampleForm.sveltefrontend/src/lib/components/Forms/ModelForm/DashboardBuiltinWidgetForm.sveltefrontend/src/lib/components/Forms/ModelForm/DashboardTextWidgetForm.sveltefrontend/src/lib/components/Forms/ModelForm/DashboardWidgetForm.sveltefrontend/src/lib/components/Forms/ModelForm/DataBreachForm.sveltefrontend/src/lib/components/Forms/ModelForm/EbiosRmForm.sveltefrontend/src/lib/components/Forms/ModelForm/ElementaryActionForm.sveltefrontend/src/lib/components/Forms/ModelForm/EntityAssessmentForm.sveltefrontend/src/lib/components/Forms/ModelForm/EntityForm.sveltefrontend/src/lib/components/Forms/ModelForm/EvidenceRevisionForm.sveltefrontend/src/lib/components/Forms/ModelForm/FindingsAssessmentForm.sveltefrontend/src/lib/components/Forms/ModelForm/GenericCollectionForm.sveltefrontend/src/lib/components/Forms/ModelForm/IncidentForm.sveltefrontend/src/lib/components/Forms/ModelForm/MetricDefinitionForm.sveltefrontend/src/lib/components/Forms/ModelForm/OrganisationIssueForm.sveltefrontend/src/lib/components/Forms/ModelForm/OrganisationObjectiveForm.sveltefrontend/src/lib/components/Forms/ModelForm/PerimeterForm.sveltefrontend/src/lib/components/Forms/ModelForm/ProcessingForm.sveltefrontend/src/lib/components/Forms/ModelForm/QuantitativeRiskStudyForm.sveltefrontend/src/lib/components/Forms/ModelForm/ReferenceControlForm.sveltefrontend/src/lib/components/Forms/ModelForm/RequirementAssessmentForm.sveltefrontend/src/lib/components/Forms/ModelForm/RightRequestForm.sveltefrontend/src/lib/components/Forms/ModelForm/RiskAcceptanceForm.sveltefrontend/src/lib/components/Forms/ModelForm/RiskAssessmentForm.sveltefrontend/src/lib/components/Forms/ModelForm/SecurityExceptionForm.sveltefrontend/src/lib/components/Forms/ModelForm/ThreatForm.sveltefrontend/src/lib/components/Forms/ModelForm/TimelineEntryForm.sveltefrontend/src/lib/components/Forms/ModelForm/ValidationFlowForm.sveltefrontend/src/lib/components/Forms/ModelForm/VulnerabilitiesForm.sveltefrontend/src/lib/components/Forms/WebhookSecretGenerator.sveltefrontend/src/lib/components/ModelTable/ModelTable.sveltefrontend/src/lib/utils/table.ts
💤 Files with no reviewable changes (30)
- frontend/src/lib/components/Forms/ModelForm/RiskAssessmentForm.svelte
- frontend/src/lib/components/Forms/ModelForm/DashboardWidgetForm.svelte
- frontend/src/lib/components/Forms/ModelForm/DashboardBuiltinWidgetForm.svelte
- frontend/src/lib/components/Forms/ModelForm/QuantitativeRiskStudyForm.svelte
- frontend/src/lib/components/Forms/ModelForm/RightRequestForm.svelte
- frontend/src/lib/components/Forms/ModelForm/OrganisationObjectiveForm.svelte
- frontend/src/lib/components/Forms/ModelForm/IncidentForm.svelte
- frontend/src/lib/components/Forms/ModelForm/AssetForm.svelte
- frontend/src/lib/components/Forms/ModelForm/ValidationFlowForm.svelte
- frontend/src/lib/components/Forms/ModelForm/VulnerabilitiesForm.svelte
- frontend/src/lib/components/Forms/ModelForm/GenericCollectionForm.svelte
- frontend/src/lib/components/Forms/ModelForm/ContractForm.svelte
- frontend/src/lib/components/Forms/ModelForm/DashboardTextWidgetForm.svelte
- frontend/src/lib/components/Forms/ModelForm/DataBreachForm.svelte
- frontend/src/lib/components/Forms/ModelForm/CampaignForm.svelte
- frontend/src/lib/components/Forms/ModelForm/FindingsAssessmentForm.svelte
- frontend/src/lib/components/Forms/ModelForm/ProcessingForm.svelte
- frontend/src/lib/components/Forms/ModelForm/ThreatForm.svelte
- frontend/src/lib/components/Forms/ModelForm/ElementaryActionForm.svelte
- frontend/src/lib/components/Forms/ModelForm/EbiosRmForm.svelte
- frontend/src/lib/components/Forms/ModelForm/OrganisationIssueForm.svelte
- frontend/src/lib/components/Forms/ModelForm/EntityAssessmentForm.svelte
- frontend/src/lib/components/Forms/ModelForm/AccreditationForm.svelte
- frontend/src/lib/components/Forms/ModelForm/CustomMetricSampleForm.svelte
- frontend/src/lib/components/Forms/ModelForm/ComplianceAssessmentForm.svelte
- frontend/src/lib/components/Forms/ModelForm/BusinessImpactAnalysisForm.svelte
- frontend/src/lib/components/Forms/ModelForm/MetricDefinitionForm.svelte
- frontend/src/lib/components/Forms/ModelForm/ReferenceControlForm.svelte
- frontend/src/lib/components/Forms/ModelForm/SecurityExceptionForm.svelte
- frontend/src/lib/components/Forms/ModelForm/RiskAcceptanceForm.svelte
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: nas-tabchiche
Repo: intuitem/ciso-assistant-community PR: 1446
File: backend/core/migrations/0051_rename_project_perimeter_alter_perimeter_options_and_more.py:0-0
Timestamp: 2025-02-04T08:29:42.810Z
Learning: Using `migrations.RenameField` is preferred over add-migrate-remove pattern when renaming fields in Django as it preserves data and relationships at the database level without requiring explicit data migration.
📚 Learning: 2025-12-29T15:52:14.696Z
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:14.696Z
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/src/lib/components/Forms/ModelForm/RequirementAssessmentForm.sveltefrontend/src/lib/components/Forms/ModelForm/PerimeterForm.sveltefrontend/src/lib/components/Forms/ModelForm/EntityForm.svelte
📚 Learning: 2025-11-17T11:09:18.488Z
Learnt from: monsieurswag
Repo: intuitem/ciso-assistant-community PR: 2860
File: frontend/src/routes/(app)/(internal)/[model=urlmodel]/[id=uuid]/+page.server.ts:0-0
Timestamp: 2025-11-17T11:09:18.488Z
Learning: In frontend/src/routes/(app)/(internal)/[model=urlmodel]/[id=uuid]/+page.server.ts, the select action performs batch PATCH operations in Promise.all without transaction semantics. Partial failures (e.g., one object fails to update due to race conditions) are acceptable and should not trigger a fail() return, as most objects will still be updated successfully. Console warnings are sufficient for logging failures.
Applied to files:
frontend/src/lib/components/ModelTable/ModelTable.svelte
📚 Learning: 2025-10-12T21:53:47.742Z
Learnt from: ab-smith
Repo: intuitem/ciso-assistant-community PR: 2699
File: backend/core/serializers.py:414-422
Timestamp: 2025-10-12T21:53:47.742Z
Learning: In the asset hierarchy for CISO Assistant (backend/core/serializers.py), both PRIMARY and SUPPORT assets can have child_assets (supporting assets). PRIMARY assets are at the top of the hierarchy and cannot have parent_assets, while SUPPORT assets can have both parents and children.
Applied to files:
backend/resilience/serializers.pybackend/core/serializers.py
📚 Learning: 2025-09-19T08:46:34.495Z
Learnt from: Mohamed-Hacene
Repo: intuitem/ciso-assistant-community PR: 2550
File: backend/core/serializers.py:1277-1292
Timestamp: 2025-09-19T08:46:34.495Z
Learning: In EvidenceWriteSerializer, the link and attachment fields are read-only, so updates to these fields should be done through EvidenceRevisionWriteSerializer to maintain proper versioning.
Applied to files:
backend/core/serializers.py
📚 Learning: 2025-09-19T09:08:52.306Z
Learnt from: Mohamed-Hacene
Repo: intuitem/ciso-assistant-community PR: 2550
File: backend/core/migrations/0098_evidence_lifecycle_data_migration.py:21-28
Timestamp: 2025-09-19T09:08:52.306Z
Learning: EvidenceRevision.save() method automatically sets is_published from the parent Evidence object and handles folder assignment from the evidence's folder with proper safety checks.
Applied to files:
backend/core/serializers.py
📚 Learning: 2025-09-19T09:08:52.306Z
Learnt from: Mohamed-Hacene
Repo: intuitem/ciso-assistant-community PR: 2550
File: backend/core/migrations/0098_evidence_lifecycle_data_migration.py:21-28
Timestamp: 2025-09-19T09:08:52.306Z
Learning: Evidence model inherits from FolderMixin making folder a mandatory field, and the lifecycle management handles folder propagation to revisions automatically.
Applied to files:
backend/core/serializers.py
🧬 Code graph analysis (6)
backend/app_tests/api/test_api_perimeters.py (1)
backend/app_tests/api/test_utils.py (2)
EndpointTestsUtils(16-142)get_endpoint_url(46-51)
backend/ebios_rm/serializers.py (2)
backend/core/serializer_fields.py (4)
IdRelatedField(24-45)to_representation(15-21)to_representation(40-45)to_representation(61-95)backend/core/serializers.py (18)
update(75-96)update(272-280)update(329-338)update(486-501)update(898-914)update(1326-1340)update(1585-1602)update(1868-1901)update(2080-2103)update(2338-2347)update(2552-2560)update(2668-2697)update(2885-2977)BaseModelSerializer(65-139)to_representation(916-940)to_representation(1017-1044)to_representation(1604-1619)to_representation(2638-2652)
backend/resilience/serializers.py (4)
backend/core/serializer_fields.py (1)
IdRelatedField(24-45)backend/resilience/views.py (2)
risk_matrix(91-93)risk_matrix(196-198)backend/resilience/models.py (1)
risk_matrix(214-215)backend/core/models.py (1)
children_assets(2693-2696)
backend/crq/serializers.py (2)
backend/core/serializer_fields.py (1)
IdRelatedField(24-45)backend/crq/models.py (1)
QuantitativeRiskScenario(365-526)
backend/core/tests/test_models.py (1)
backend/core/models.py (8)
RiskAssessment(4788-5326)AppliedControl(4056-4496)risk_scenarios(4333-4334)assets(7596-7597)Evidence(3727-3814)quality_check(4918-5323)quality_check(6372-6605)ComplianceAssessment(5798-6752)
backend/core/serializers.py (1)
backend/core/serializer_fields.py (2)
IdRelatedField(24-45)PathField(48-95)
🪛 GitHub Actions: Frontend Linters
frontend/src/lib/components/Forms/WebhookSecretGenerator.svelte
[warning] 1-1: Prettier formatting issues detected. Run 'prettier --write' to fix.
🪛 GitHub Actions: Functional Tests
frontend/src/lib/components/Forms/WebhookSecretGenerator.svelte
[error] 47-47: Identifier 'copySecret' has already been declared in this scope.
⏰ 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). (7)
- GitHub Check: build (3.12)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: Analyze (python)
🔇 Additional comments (26)
frontend/src/lib/components/Forms/ModelForm/EntityForm.svelte (1)
68-89: Verify the behavioral change and visibility strategy inconsistency.This change modifies when the folder field is editable:
Previous behavior: The folder field was hidden when
initialData.folderexisted (i.e., when editing an entity that already has a folder assigned).New behavior: The folder field is always visible but disabled only for builtin objects. This means non-builtin entities being edited can now change their folder/domain, whereas previously the field would have been hidden.
Additionally, there's an inconsistent visibility pattern:
- Folder field (lines 68-77): Always rendered but conditionally disabled via
disabled={object.builtin}- Relationship field (lines 79-89): Conditionally rendered (completely hidden) via
{#if !object.builtin}While this inconsistency may be intentional (showing the folder/domain for context even when not editable, while completely hiding the relationship selector), please verify:
- Is allowing folder/domain changes for non-builtin existing entities the intended behavior?
- Is the mixed approach (disabled vs. hidden) deliberate, or should both fields use the same visibility strategy?
Given the PR description mentions "working on a pattern to detect non visible objects," this appears intentional, but confirming the expected UX would help ensure consistency across the codebase.
frontend/messages/en.json (3)
2484-2703: LGTM! New translation keys added correctly.The new translation keys for EBIOS RM studies, flow events, asset capabilities, and various CRUD operations follow proper camelCase naming convention and provide clear, descriptive labels.
3273-3277: LGTM! New branch validation messages added.These new translation keys provide clear validation messages for detecting when objects belong to different branches than their parent assessments. The keys follow proper camelCase naming and provide user-friendly error messages.
3291-3300: LGTM! New visibility filter with proper pluralization.The new
objectsNotVisibletranslation key correctly implements plural handling using ICU MessageFormat, which aligns with the PR objective of "working on a pattern to detect non visible objects and alert the user."The implementation properly differentiates between singular and plural forms, following the established pattern used elsewhere in this file.
frontend/messages/fr.json (2)
3269-3279: LGTM! Properly structured plural message and success notification.The
objectsNotVisibleentry (lines 3269-3278) correctly implements ICU MessageFormat plural handling with appropriate French singular/plural forms. The structure properly declares input variables, derives plural selectors, and provides correct grammatical agreement ("objet non visible" vs "objets non visible").The
secretCopiedToClipboardentry (line 3279) follows the existing pattern for success messages and the translation is appropriate.
3263-3263: No action needed — duplicate keys have been properly removed.The JSON file contains no duplicate keys. The previous concern about duplicate
libraryFilteringLabelsentries has been resolved.backend/app_tests/api/test_api_perimeters.py (2)
150-150: This comment is incorrect. Bothtest.folder.name(line 98) andstr(test.folder)(line 150) are functionally equivalent and not inconsistent.The Folder model's
__str__method returnsself.name.__str__()(backend/iam/models.py, line 134-135), so callingstr(test.folder)produces the same value as directly accessingtest.folder.name. Using different approaches to access the same value is a stylistic choice, not a bug.Likely an incorrect or invalid review comment.
153-155: Clarify the purpose of test_params.The
test_paramsparameter specifies the expected values in the API response after the update operation. Since API responses serialize object references as stringified UUIDs whilebuild_paramscontains model instances,test_paramsprovides the response format values for verification. In this case, it ensures the folder field in the response contains the stringified folder ID.frontend/src/lib/components/Forms/ModelForm/PerimeterForm.svelte (1)
34-34: LGTM! Improved UX by disabling instead of hiding.Changing from
hiddentodisabledimproves the user experience by keeping the folder field visible (but non-editable) wheninitialData.folderexists. This allows users to see the current domain value even when they can't change it.frontend/src/lib/components/Forms/ModelForm/RequirementAssessmentForm.svelte (1)
84-92: LGTM! Domain field is now user-editable.The replacement of
HiddenInputwithAutocompleteSelectsuccessfully makes the folder/domain field interactive and editable, aligning with the PR objective to make domains changeable. The implementation follows the established pattern with proper caching and endpoint configuration.frontend/src/lib/components/Forms/ModelForm/TimelineEntryForm.svelte (1)
34-42: LGTM! Consistent pattern for domain editability.The addition of
AutocompleteSelectfor the folder field follows the same pattern as other forms in this PR, enabling users to select domains for timeline entries. The implementation is properly configured with caching, endpoint, and label.frontend/src/lib/components/Forms/ModelForm/EvidenceRevisionForm.svelte (1)
68-76: LGTM! Domain field is now consistently editable.The replacement with
AutocompleteSelectmakes the folder/domain field interactive and follows the consistent pattern established across other forms in this PR. The implementation is properly configured with caching, endpoint, and label.backend/ebios_rm/serializers.py (3)
31-48: LGTM! Transactional cascade pattern correctly implemented.The update method properly captures the old folder_id, wraps changes in an atomic transaction, and cascades folder updates to all related child models. This ensures referential integrity when folder changes occur.
127-605: Consistent IdRelatedField migration across all read serializers.All read serializers (FearedEvent, RoTo, Stakeholder, StrategicScenario, AttackPath, OperationalScenario, etc.) have been consistently updated to use IdRelatedField for related fields. The migration is uniform and aligns with the PR objectives.
As with lines 53-77, verify that the frontend correctly handles ID-only responses for these serializers and uses the related-name resolution utilities where needed.
53-77: IdRelatedField ignores field arguments—verify frontend compatibility.The field specifications passed to
IdRelatedField(e.g.,["id", "folder"]on line 53,["id", "type", {"folder": ["id"]}]on line 57) are silently ignored per the implementation inbackend/core/serializer_fields.py. These fields will serialize as plain IDs only. Confirm that frontend consumers have been updated to handle ID-only responses, and if nested data is still needed, ensure the frontend uses the available utilities likefetchNamesfrom$lib/utils/related-names.tsto resolve related object details.frontend/src/lib/components/ModelTable/ModelTable.svelte (3)
339-346: Hidden count calculation looks correct.The
$effectproperly calculates the difference betweenexpectedCountand the actual row count, handling undefined cases and settinghiddenCountaccordingly. This provides useful feedback to users about non-visible items.
689-764: Array rendering logic correctly handles ID resolution.The rendering logic properly distinguishes between ID-only arrays and arrays with display labels. It resolves IDs to names using
relatedNames, renders links when available, and shows "not visible" messages for unresolved IDs. The fallback handling is comprehensive.
782-796: UUID field rendering with fallback is well-implemented.Single UUID fields are correctly resolved to labels using
relatedNames. The logic handles all cases: rendering links when both label and URL exist, displaying labels without links, and falling back to the raw UUID when resolution hasn't completed. This provides a good user experience.backend/privacy/serializers.py (2)
123-140: Transactional folder cascade correctly implemented.The update method follows the same safe pattern as other serializers in this PR: capturing the old folder_id, wrapping changes in a transaction, and cascading folder updates to all related models (Purpose, PersonalData, DataSubject, DataRecipient, DataContractor, DataTransfer).
26-201: IdRelatedField migration is consistent across all serializers.All read serializers have been updated to use IdRelatedField. The migration follows the same pattern as other modules in this PR. As with other files, verify that consumers of these APIs (particularly ProcessingReadSerializer lines 144-149 with nested field specifications) handle ID-only responses correctly.
backend/crq/serializers.py (2)
84-95: Transactional cascade pattern correctly handles nested relationships.Both update methods properly implement the cascade pattern:
QuantitativeRiskStudyWriteSerializer.updatecascades folder changes to both scenarios and their hypotheses (lines 89-94)QuantitativeRiskScenarioWriteSerializer.updatecascades to hypotheses (lines 148-150)The use of transactions ensures atomicity across the entire hierarchy, maintaining referential integrity.
Also applies to: 143-151
99-341: IdRelatedField migration is consistent across CRQ serializers.All read serializers (QuantitativeRiskStudy, QuantitativeRiskScenario, QuantitativeRiskHypothesis) have been consistently updated to use IdRelatedField. The pattern aligns with the rest of the PR.
As with other files, verify that API consumers handle ID-only responses correctly, especially for fields like
validation_flows(lines 106-115) where nested field specifications are provided but ignored by IdRelatedField.backend/core/serializers.py (4)
1869-1901: Well implemented transactional folder cascading and notification scheduling.The implementation correctly:
- Wraps updates in
transaction.atomic()for consistency- Cascades folder changes to related
RequirementAssessmentobjects (lines 1884-1887)- Schedules notifications to run after transaction commits using
transaction.on_commit()(lines 1894-1899)This is the proper pattern for combining database updates with external side effects (notifications).
2552-2560: Folder cascading to TimelineEntry is correctly implemented.The transactional folder cascade from Incident to TimelineEntry (lines 2556-2559) follows the correct pattern:
- Atomic transaction ensures consistency
- Related timeline entries follow the incident's folder
No notifications are scheduled, which appears intentional for this update scenario.
314-342: The review comment is based on an incorrect assumption.RiskScenariodoes not have afolderfield. While other parent-child relationships (Evidence→EvidenceRevision, ComplianceAssessment→RequirementAssessment, Incident→TimelineEntry, TaskTemplate→TaskNode, ValidationFlow→FlowEvent) require folder cascading because the child models have folder fields,RiskScenarioonly has arisk_assessmentforeign key and no folder field of its own. Therefore, folder cascading toRiskScenariois not applicable and no changes are needed toRiskAssessmentWriteSerializer.Likely an incorrect or invalid review comment.
1591-1600: Implementation is correct — no issues found.The explicit EvidenceRevision folder cascade (lines 1597-1600) is necessary. While EvidenceRevision.save() does set folder automatically when called directly, Evidence.save() does not cascade folder changes to existing revisions. The explicit bulk update is required because existing revisions won't have their save() method invoked during the Evidence update, and the transaction wrapper ensures atomicity.
| def test_update_perimeters_folder_forbidden(self, authenticated_client): | ||
| """test to ensure perimeter folder cannot be updated with the API""" | ||
|
|
||
| folder = Folder.objects.create(name="test") | ||
| perimeter = Perimeter.objects.create( | ||
| name=PERIMETER_NAME, | ||
| description=PERIMETER_DESCRIPTION, | ||
| folder=folder, | ||
| ref_id=PERIMETER_REFERENCE, | ||
| lc_status=PERIMETER_STATUS[0], | ||
| ) | ||
| new_folder = Folder.objects.create(name="test2") | ||
| url = ( | ||
| EndpointTestsUtils.get_endpoint_url("Perimeters") + str(perimeter.id) + "/" | ||
| ) | ||
| response = authenticated_client.patch( | ||
| url, {"folder": str(new_folder.id)}, format="json" | ||
| ) | ||
| assert response.status_code == status.HTTP_403_FORBIDDEN | ||
| perimeter.refresh_from_db() | ||
| assert perimeter.folder_id == folder.id |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Consider using the test fixture for proper permission context.
This test uses authenticated_client directly while other tests in the class use the test fixture that provides proper user_group and folder scope setup. Without this context, the 403 response might be due to lack of permissions to access the perimeter rather than the folder update restriction you're testing.
Consider refactoring to use the test fixture pattern:
🔎 Suggested refactor
- def test_update_perimeters_folder_forbidden(self, authenticated_client):
+ def test_update_perimeters_folder_forbidden(self, test):
"""test to ensure perimeter folder cannot be updated with the API"""
- folder = Folder.objects.create(name="test")
- perimeter = Perimeter.objects.create(
+ perimeter = Perimeter.objects.create(
name=PERIMETER_NAME,
description=PERIMETER_DESCRIPTION,
- folder=folder,
+ folder=test.folder,
ref_id=PERIMETER_REFERENCE,
lc_status=PERIMETER_STATUS[0],
)
new_folder = Folder.objects.create(name="test2")
url = (
EndpointTestsUtils.get_endpoint_url("Perimeters") + str(perimeter.id) + "/"
)
- response = authenticated_client.patch(
+ response = test.client.patch(
url, {"folder": str(new_folder.id)}, format="json"
)
assert response.status_code == status.HTTP_403_FORBIDDEN
perimeter.refresh_from_db()
- assert perimeter.folder_id == folder.id
+ assert perimeter.folder_id == test.folder.idThis ensures the test verifies folder update restrictions specifically, not general permission denial.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_update_perimeters_folder_forbidden(self, authenticated_client): | |
| """test to ensure perimeter folder cannot be updated with the API""" | |
| folder = Folder.objects.create(name="test") | |
| perimeter = Perimeter.objects.create( | |
| name=PERIMETER_NAME, | |
| description=PERIMETER_DESCRIPTION, | |
| folder=folder, | |
| ref_id=PERIMETER_REFERENCE, | |
| lc_status=PERIMETER_STATUS[0], | |
| ) | |
| new_folder = Folder.objects.create(name="test2") | |
| url = ( | |
| EndpointTestsUtils.get_endpoint_url("Perimeters") + str(perimeter.id) + "/" | |
| ) | |
| response = authenticated_client.patch( | |
| url, {"folder": str(new_folder.id)}, format="json" | |
| ) | |
| assert response.status_code == status.HTTP_403_FORBIDDEN | |
| perimeter.refresh_from_db() | |
| assert perimeter.folder_id == folder.id | |
| def test_update_perimeters_folder_forbidden(self, test): | |
| """test to ensure perimeter folder cannot be updated with the API""" | |
| perimeter = Perimeter.objects.create( | |
| name=PERIMETER_NAME, | |
| description=PERIMETER_DESCRIPTION, | |
| folder=test.folder, | |
| ref_id=PERIMETER_REFERENCE, | |
| lc_status=PERIMETER_STATUS[0], | |
| ) | |
| new_folder = Folder.objects.create(name="test2") | |
| url = ( | |
| EndpointTestsUtils.get_endpoint_url("Perimeters") + str(perimeter.id) + "/" | |
| ) | |
| response = test.client.patch( | |
| url, {"folder": str(new_folder.id)}, format="json" | |
| ) | |
| assert response.status_code == status.HTTP_403_FORBIDDEN | |
| perimeter.refresh_from_db() | |
| assert perimeter.folder_id == test.folder.id |
| def update(self, instance, validated_data): | ||
| new_folder = validated_data.get("folder", None) | ||
| if new_folder is not None: | ||
| new_folder_id = ( | ||
| new_folder.id if isinstance(new_folder, models.Model) else new_folder | ||
| ) | ||
| if new_folder_id and str(new_folder_id) != str(instance.folder_id): | ||
| raise PermissionDenied({"folder": "Perimeter domain cannot be changed"}) | ||
| return super().update(instance, validated_data) |
There was a problem hiding this comment.
Clarify intent: folder validation prevents changes but PR suggests enabling them.
The PR title references "CA-1456-make-domain-changeable" but this validation (line 279) explicitly prevents Perimeter folder changes with the error "Perimeter domain cannot be changed." This appears contradictory.
Additionally, the same string comparison issue noted in BaseModelSerializer.update() applies here. Consider using direct UUID comparison:
- if new_folder_id and str(new_folder_id) != str(instance.folder_id):
+ if new_folder_id is not None and new_folder_id != instance.folder_id:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def update(self, instance, validated_data): | |
| new_folder = validated_data.get("folder", None) | |
| if new_folder is not None: | |
| new_folder_id = ( | |
| new_folder.id if isinstance(new_folder, models.Model) else new_folder | |
| ) | |
| if new_folder_id and str(new_folder_id) != str(instance.folder_id): | |
| raise PermissionDenied({"folder": "Perimeter domain cannot be changed"}) | |
| return super().update(instance, validated_data) | |
| def update(self, instance, validated_data): | |
| new_folder = validated_data.get("folder", None) | |
| if new_folder is not None: | |
| new_folder_id = ( | |
| new_folder.id if isinstance(new_folder, models.Model) else new_folder | |
| ) | |
| if new_folder_id is not None and new_folder_id != instance.folder_id: | |
| raise PermissionDenied({"folder": "Perimeter domain cannot be changed"}) | |
| return super().update(instance, validated_data) |
| bia = IdRelatedField(["id", "name", "is_locked"]) | ||
| asset = IdRelatedField() | ||
| asset_folder = IdRelatedField(source="asset.folder") | ||
| children_assets = IdRelatedField(source="asset.children_assets", many=True) | ||
| folder = IdRelatedField() | ||
|
|
||
| asset_ref_id = serializers.CharField( | ||
| source="asset.ref_id", read_only=True, allow_null=True | ||
| ) | ||
| asset_folder = FieldsRelatedField(source="asset.folder") | ||
| children_assets = FieldsRelatedField(source="asset.children_assets", many=True) | ||
| folder = FieldsRelatedField() | ||
|
|
||
| dependencies = FieldsRelatedField(many=True) | ||
| evidences = FieldsRelatedField(many=True) | ||
| associated_controls = FieldsRelatedField( | ||
| dependencies = IdRelatedField(many=True) | ||
| evidences = IdRelatedField(many=True) | ||
| associated_controls = IdRelatedField( | ||
| ["id", "folder", "name", "status", "eta"], many=True | ||
| ) |
There was a problem hiding this comment.
Critical: Breaking API change will cause frontend runtime failures.
Lines 43 and 55-57 declare bia = IdRelatedField(["id", "name", "is_locked"]) and associated_controls = IdRelatedField(["id", "folder", "name", "status", "eta"], many=True), but IdRelatedField explicitly ignores the fields argument and returns only IDs.
Frontend breakage locations:
frontend/src/routes/(app)/(internal)/asset-assessments/[id=uuid]/+page.svelteline 16 accessesasset_assessment.bia?.is_locked(will fail—bia is now an integer)- Same file line 27 accesses
asset_assessment.bia.name(will fail) frontend/src/routes/(app)/(internal)/business-impact-analysis/[id=uuid]/report/+page.server.tsline 49 accessescontrol.idfrom associated_controls (will fail—items are integers, not objects)
Either:
- Use a proper nested serializer for these fields (e.g., a lightweight BiaSerializer), or
- Update the frontend to fetch these related objects separately using the new related-name utilities
Based on learnings and past review comments.
#!/bin/bash
# Verify the exact frontend code that will break
echo "=== Confirming bia field access in asset-assessments detail page ==="
sed -n '14,30p' frontend/src/routes/\(app\)/\(internal\)/asset-assessments/\[id=uuid\]/+page.svelte
echo ""
echo "=== Confirming associated_controls access in BIA report ==="
sed -n '45,53p' frontend/src/routes/\(app\)/\(internal\)/business-impact-analysis/\[id=uuid\]/report/+page.server.ts| "addVulnerability": "Add vulnerability", | ||
| "addReferenceControl": "Add reference control", | ||
| "addAppliedControl": "Add applied control", | ||
| "addAppliedcontrol": "Add applied control", |
There was a problem hiding this comment.
Inconsistent key naming: camelCase changed to lowercase.
These translation keys have been changed from camelCase to lowercase (e.g., addAppliedControl → addAppliedcontrol). However, this creates inconsistency because:
- Other new keys in this file still use camelCase (e.g., line 2484:
addEbiosRmStudy) - This breaks the established naming convention used throughout the file
Ensure a consistent naming convention across all translation keys.
Also applies to: 33-33, 473-473, 482-482
🤖 Prompt for AI Agents
In @frontend/messages/en.json at line 27, Restore the inconsistent lowercase
translation keys to camelCase (e.g., rename "addAppliedcontrol" back to
"addAppliedControl") and do the same for the other affected keys noted in the
comment (the ones accidentally lowercased at the other locations), ensuring they
match the existing convention used by keys like "addEbiosRmStudy"; update any
code references/usages to the corrected camelCase keys so lookups remain valid
and run a quick grep across the project to catch any remaining lowercase
variants.
| "addAssetclass": "Add asset class", | ||
| "addAttackpath": "Add attack path", | ||
| "addDatabreach": "Register data breach", | ||
| "addDatacontractor": "Add data contractor", | ||
| "addDatarecipient": "Add data recipient", | ||
| "addDatasubject": "Add data subject", | ||
| "addDatatransfer": "Add data transfer", | ||
| "addEbiosrmstudy": "Add EBIOS RM study", | ||
| "addElementaryaction": "Add elementary action", | ||
| "addEntityassessment": "Add entity assessment", | ||
| "addEscalationthreshold": "Add threshold", | ||
| "addEvent": "Add event", | ||
| "addEvidencerevision": "New revision", | ||
| "addFearedevent": "Add feared event", | ||
| "addFilteringlabel": "Add filtering label", | ||
| "addFindingsassessment": "Add findings tracking", | ||
| "addGenericcollection": "Create collection", | ||
| "addIntegrationconfiguration": "Add integration configuration", | ||
| "addKillchain": "Add kill chain step", | ||
| "addLoadedlibrary": "Add loaded library", | ||
| "addOperatingmode": "Add operating mode", | ||
| "addOperationalscenario": "Add operational scenario", | ||
| "addOrganisationissue": "Add organisation issue", | ||
| "addOrganisationobjective": "Add organisation objective", | ||
| "addPersonaldata": "Add personal data", | ||
| "addQuantitativeriskhypothesis": "Add hypothesis", | ||
| "addQuantitativeriskscenario": "Add scenario", | ||
| "addQuantitativeriskstudy": "Start risk quantification study", | ||
| "addReferencecontrol": "Add reference control", | ||
| "addRequirementmappingset": "Add requirement mapping set", | ||
| "addRightrequest": "Track request", | ||
| "addRiskacceptance": "Add risk acceptance", | ||
| "addRiskassessment": "Add risk assessment", | ||
| "addRiskmatrix": "Add risk matrix", | ||
| "addRiskscenario": "Add risk scenario", | ||
| "addSecurityexception": "Add exception", | ||
| "addStoredlibrary": "Add stored library", | ||
| "addStrategicscenario": "Add strategic scenario", | ||
| "addSyncmapping": "Add sync mapping", | ||
| "addTasktemplate": "Add task", | ||
| "addTimelineentry": "Add timeline entry", | ||
| "addValidationflow": "Add validation flow", | ||
| "addWebhookendpoint": "Add webhook endpoint", | ||
| "approveRiskacceptance": "Approve risk acceptance", | ||
| "changeAppliedcontrol": "Change applied control", | ||
| "changeAssetassessment": "Change asset assessment", | ||
| "changeAssetclass": "Change asset class", | ||
| "changeAttackpath": "Change attack path", | ||
| "changeBusinessimpactanalysis": "Change business impact analysis", | ||
| "changeClientsettings": "Change client settings", | ||
| "changeComplianceassessment": "Change compliance assessment", | ||
| "changeDatabreach": "Change data breach", | ||
| "changeDatacontractor": "Change data contractor", | ||
| "changeDatarecipient": "Change data recipient", | ||
| "changeDatasubject": "Change data subject", | ||
| "changeDatatransfer": "Change data transfer", | ||
| "changeEbiosrmstudy": "Change EBIOS RM study", | ||
| "changeElementaryaction": "Change elementary action", | ||
| "changeEntityassessment": "Change entity assessment", | ||
| "changeEscalationthreshold": "Change escalation threshold", | ||
| "changeEvent": "Change event", | ||
| "changeEvidencerevision": "Change evidence revision", | ||
| "changeFearedevent": "Change feared event", | ||
| "changeFilteringlabel": "Change filtering label", | ||
| "changeFindingsassessment": "Change findings assessment", | ||
| "changeGenericcollection": "Change generic collection", | ||
| "changeGlobalsettings": "Change global settings", | ||
| "changeIntegrationconfiguration": "Change integration configuration", | ||
| "changeKillchain": "Change kill chain", | ||
| "changeOperatingmode": "Change operating mode", | ||
| "changeOperationalscenario": "Change operational scenario", | ||
| "changeOrganisationissue": "Change organisation issue", | ||
| "changeOrganisationobjective": "Change organisation objective", | ||
| "changePersonaldata": "Change personal data", | ||
| "changeQuantitativeriskhypothesis": "Change quantitative risk hypothesis", | ||
| "changeQuantitativeriskscenario": "Change quantitative risk scenario", | ||
| "changeQuantitativeriskstudy": "Change quantitative risk study", | ||
| "changeReferencecontrol": "Change reference control", | ||
| "changeRequirementassessment": "Change requirement assessment", | ||
| "changeRightrequest": "Change right request", | ||
| "changeRiskacceptance": "Change risk acceptance", | ||
| "changeRiskassessment": "Change risk assessment", | ||
| "changeRiskmatrix": "Change risk matrix", | ||
| "changeRiskscenario": "Change risk scenario", | ||
| "changeRoto": "Change RoTO", | ||
| "changeSecurityexception": "Change security exception", | ||
| "changeSsosettings": "Change SSO settings", | ||
| "changeStrategicscenario": "Change strategic scenario", | ||
| "changeSyncmapping": "Change sync mapping", | ||
| "changeTasknode": "Change task node", | ||
| "changeTasktemplate": "Change task template", | ||
| "changeTimelineentry": "Change timeline entry", | ||
| "changeUsergroup": "Change user group", | ||
| "changeValidationflow": "Change validation flow", | ||
| "changeWebhookendpoint": "Change webhook endpoint", | ||
| "deleteAppliedcontrol": "Delete applied control", | ||
| "deleteAssetassessment": "Delete asset assessment", | ||
| "deleteAssetclass": "Delete asset class", | ||
| "deleteAttackpath": "Delete attack path", | ||
| "deleteBusinessimpactanalysis": "Delete business impact analysis", | ||
| "deleteComplianceassessment": "Delete compliance assessment", | ||
| "deleteDatabreach": "Delete data breach", | ||
| "deleteDatacontractor": "Delete data contractor", | ||
| "deleteDatarecipient": "Delete data recipient", | ||
| "deleteDatasubject": "Delete data subject", | ||
| "deleteDatatransfer": "Delete data transfer", | ||
| "deleteEbiosrmstudy": "Delete EBIOS RM study", | ||
| "deleteElementaryaction": "Delete elementary action", | ||
| "deleteEntityassessment": "Delete entity assessment", | ||
| "deleteEscalationthreshold": "Delete escalation threshold", | ||
| "deleteEvent": "Delete event", | ||
| "deleteEvidencerevision": "Delete evidence revision", | ||
| "deleteFearedevent": "Delete feared event", | ||
| "deleteFilteringlabel": "Delete filtering label", | ||
| "deleteFindingsassessment": "Delete findings assessment", | ||
| "deleteGenericcollection": "Delete generic collection", | ||
| "deleteIntegrationconfiguration": "Delete integration configuration", | ||
| "deleteKillchain": "Delete kill chain", | ||
| "deleteLoadedlibrary": "Delete loaded library", | ||
| "deleteOperatingmode": "Delete operating mode", | ||
| "deleteOperationalscenario": "Delete operational scenario", | ||
| "deleteOrganisationissue": "Delete organisation issue", | ||
| "deleteOrganisationobjective": "Delete organisation objective", | ||
| "deletePersonaldata": "Delete personal data", | ||
| "deleteQuantitativeriskhypothesis": "Delete quantitative risk hypothesis", | ||
| "deleteQuantitativeriskscenario": "Delete quantitative risk scenario", | ||
| "deleteQuantitativeriskstudy": "Delete quantitative risk study", | ||
| "deleteReferencecontrol": "Delete reference control", | ||
| "deleteRequirementmappingset": "Delete requirement mapping set", | ||
| "deleteRightrequest": "Delete right request", | ||
| "deleteRiskacceptance": "Delete risk acceptance", | ||
| "deleteRiskassessment": "Delete risk assessment", | ||
| "deleteRiskmatrix": "Delete risk matrix", | ||
| "deleteRiskscenario": "Delete risk scenario", | ||
| "deleteRoto": "Delete RoTO", | ||
| "deleteSecurityexception": "Delete security exception", | ||
| "deleteStoredlibrary": "Delete stored library", | ||
| "deleteStrategicscenario": "Delete strategic scenario", | ||
| "deleteSyncmapping": "Delete sync mapping", | ||
| "deleteTasknode": "Delete task node", | ||
| "deleteTasktemplate": "Delete task template", | ||
| "deleteTimelineentry": "Delete timeline entry", | ||
| "deleteUsergroup": "Delete user group", | ||
| "deleteValidationflow": "Delete validation flow", | ||
| "deleteWebhookendpoint": "Delete webhook endpoint", | ||
| "viewAppliedcontrol": "View applied control", | ||
| "viewAssetassessment": "View asset assessment", | ||
| "viewAssetcapability": "View asset capability", | ||
| "viewAssetclass": "View asset class", | ||
| "viewAttackpath": "View attack path", | ||
| "viewBusinessimpactanalysis": "View business impact analysis", | ||
| "viewClientsettings": "View client settings", | ||
| "viewComplianceassessment": "View compliance assessment", | ||
| "viewDatabreach": "View data breach", | ||
| "viewDatacontractor": "View data contractor", | ||
| "viewDatarecipient": "View data recipient", | ||
| "viewDatasubject": "View data subject", | ||
| "viewDatatransfer": "View data transfer", | ||
| "viewEbiosrmstudy": "View EBIOS RM study", | ||
| "viewElementaryaction": "View elementary action", | ||
| "viewEntityassessment": "View entity assessment", | ||
| "viewEscalationthreshold": "View escalation threshold", | ||
| "viewEvent": "View event", | ||
| "viewEvidencerevision": "View evidence revision", | ||
| "viewFearedevent": "View feared event", | ||
| "viewFilteringlabel": "View filtering label", | ||
| "viewFindingsassessment": "View findings assessment", | ||
| "viewGenericcollection": "View generic collection", | ||
| "viewGlobalsettings": "View global settings", | ||
| "viewIntegrationconfiguration": "View integration configuration", | ||
| "viewKillchain": "View kill chain", | ||
| "viewLoadedlibrary": "View loaded library", | ||
| "viewLogentry": "View log entry", | ||
| "viewOperatingmode": "View operating mode", | ||
| "viewOperationalscenario": "View operational scenario", | ||
| "viewOrganisationissue": "View organisation issue", | ||
| "viewOrganisationobjective": "View organisation objective", | ||
| "viewPermission": "View permission", | ||
| "viewPersonaldata": "View personal data", | ||
| "viewProcessingnature": "View processing nature", | ||
| "viewQuantitativeriskhypothesis": "View quantitative risk hypothesis", | ||
| "viewQuantitativeriskscenario": "View quantitative risk scenario", | ||
| "viewQuantitativeriskstudy": "View quantitative risk study", | ||
| "viewReferencecontrol": "View reference control", | ||
| "viewRequirementassessment": "View requirement assessment", | ||
| "viewRequirementmapping": "View requirement mapping", | ||
| "viewRequirementmappingset": "View requirement mapping set", | ||
| "viewRequirementnode": "View requirement node", | ||
| "viewRightrequest": "View right request", | ||
| "viewRiskacceptance": "View risk acceptance", | ||
| "viewRiskassessment": "View risk assessment", | ||
| "viewRiskmatrix": "View risk matrix", | ||
| "viewRiskscenario": "View risk scenario", | ||
| "viewRoto": "View RoTO", | ||
| "viewSecurityexception": "View security exception", | ||
| "viewSsosettings": "View SSO settings", | ||
| "viewStoredlibrary": "View stored library", | ||
| "viewStrategicscenario": "View strategic scenario", | ||
| "viewSyncmapping": "View sync mapping", | ||
| "viewTasknode": "View task node", | ||
| "viewTasktemplate": "View task template", | ||
| "viewTimelineentry": "View timeline entry", | ||
| "viewUsergroup": "View user group", | ||
| "viewValidationflow": "View validation flow", | ||
| "viewWebhookendpoint": "View webhook endpoint", |
There was a problem hiding this comment.
Critical naming inconsistency: lowercase keys instead of camelCase.
These translation keys use lowercase without camelCase (e.g., addAssetclass, addAttackpath, changeAppliedcontrol), which is inconsistent with:
- Earlier additions in this same file (lines 2484-2703) that use proper camelCase:
addEbiosRmStudy,addFlowEvent - Standard JavaScript/TypeScript naming conventions
- Most other keys in this translation file
This creates a confusing mix of naming styles within the same file. All translation keys should follow a consistent naming convention—preferably camelCase to match JavaScript conventions.
Example keys that need correction
- "addAssetclass": "Add asset class",
+ "addAssetClass": "Add asset class",
- "addAttackpath": "Add attack path",
+ "addAttackPath": "Add attack path",
- "changeAppliedcontrol": "Change applied control",
+ "changeAppliedControl": "Change applied control",Apply similar corrections to all lowercase keys in this range.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @frontend/messages/en.json around lines 3037 - 3241, The translation keys in
this diff use inconsistent lowercase naming (e.g., addAssetclass, addAttackpath,
changeAppliedcontrol) and must be converted to camelCase to match the rest of
the file and JS/TS conventions; update each key in this block to camelCase
(e.g., addAssetclass -> addAssetClass, addAttackpath -> addAttackPath,
changeAppliedcontrol -> changeAppliedControl) while keeping the corresponding
string values unchanged, and apply the same transformation to all similarly
named keys in this range (such as addEbiosrmstudy -> addEbiosRmStudy and
changeQuantitativeriskstudy -> changeQuantitativeRiskStudy) so naming is
consistent across the file.
| "addVulnerability": "Ajouter une vulnérabilité", | ||
| "addReferenceControl": "Ajouter une mesure de référence", | ||
| "addAppliedControl": "Ajouter une mesure appliquée", | ||
| "addAppliedcontrol": "Ajouter une mesure appliquée", |
There was a problem hiding this comment.
Translation key naming remains inconsistent across locales—only 2 of 21 files updated.
This issue was previously flagged but remains unresolved. The lowercase compound keys addAppliedcontrol (line 27) and addComplianceassessment (line 33) differ from the camelCase convention (addAppliedControl, addComplianceAssessment) used in the other 19 translation files.
This inconsistency will cause translation lookup failures when the frontend references the standard camelCase keys, as the toCamelCase fallback cannot convert these without separators.
Revert these two keys to match the camelCase naming used across all other locale files.
Also applies to: 33-33
🤖 Prompt for AI Agents
In @frontend/messages/fr.json at line 27, The French locale contains two keys
with incorrect casing—rename the keys addAppliedcontrol and
addComplianceassessment back to the standard camelCase names addAppliedControl
and addComplianceAssessment to match the other 19 locales; update the key names
(not the values) so translation lookups use the same identifiers as the
frontend, and verify the same two keys are corrected wherever they were changed
inconsistently.
There was a problem hiding this comment.
Actionable comments posted: 4
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)
6947-6959: FixFrameworkViewSet.nameshandling ofget_name_translatedto support lazy/attribute usageThe new logic:
str(framework.id): framework.get_name_translated if isinstance(framework.get_name_translated, str) else framework.get_name_translated()assumes
framework.get_name_translatedis either a plain string or a callable. In this codebase,get_name_translatedis often used as an attribute/property that returns a DjangoPromise(lazy translation) or similar, not a function. In those cases:
isinstance(framework.get_name_translated, str)will beFalse.framework.get_name_translated()will raiseTypeError: 'Promise' object is not callable.Even when it is callable and returns a lazy object, the result should still be coerced to
strfor JSON serialization.A safer pattern is: grab the attribute once, call it only if it’s callable, then cast to
str:Proposed fix for `names` action
@action(detail=False, methods=["get"]) def names(self, request): uuid_list = request.query_params.getlist("id[]", []) queryset = Framework.objects.filter(id__in=uuid_list) - return Response( - { - str(framework.id): framework.get_name_translated - if isinstance(framework.get_name_translated, str) - else framework.get_name_translated() - for framework in queryset - } - ) + results = {} + for framework in queryset: + value = framework.get_name_translated + if callable(value): + value = value() + # Ensure lazy translations or other proxies are converted to plain strings + results[str(framework.id)] = str(value) if value is not None else "" + + return Response(results)This will work whether
get_name_translatedis a property returning a lazy object, a plain string, or a callable that returns either.
🤖 Fix all issues with AI Agents
In @frontend/src/lib/components/ModelTable/ModelTable.svelte:
- Around line 242-270: refreshRelatedNames currently awaits fetchNames for each
urlModel without error handling, so a single network/API failure can throw and
break the component; update refreshRelatedNames to wrap each call to
fetchNames(urlModel, Array.from(ids)) in a try-catch, handle errors per-urlModel
(e.g., log the error or call an error handler and set updates[urlModel] = {} or
skip merging for that model) and continue fetching remaining urlModels, ensuring
relatedNamesKey and the merge into relatedNames still occur only for successful
results. Use the function name refreshRelatedNames and the
fetchNames/relatedNames variables to locate where to add the try-catch.
In
@frontend/src/routes/(app)/(third-party)/compliance-assessments/[id=uuid]/+page.svelte:
- Around line 583-590: The call to the i18n/message helper objectsNotVisible
currently passes an unnecessary s parameter; update the invocation (where
objectsNotVisible is called with hiddenCount) to remove the s argument and pass
only { count: hiddenCount } so the template uses built-in pluralization (refer
to objectsNotVisible and hiddenCount in the +page.svelte component).
- Around line 603-606: The lookup for foreignKeyFields in the itemHref
computation uses data.URLModel but should use the already-defined model constant
for consistency; update the expression inside the template that builds itemHref
to replace data.URLModel with model (so the code uses
URL_MODEL_MAP[model]['foreignKeyFields']?.find((item) => item.field ===
key)?.urlModel) leaving the rest of the logic (itemHref, key, URL_MODEL_MAP, and
foreignKeyFields access) unchanged.
In
@frontend/src/routes/(app)/(third-party)/requirement-assessments/[id=uuid]/edit/+page.svelte:
- Around line 224-236: Remove the two debug console.log statements that
reference data.requirementAssessment and securityExceptionsExpected in the
+page.svelte file (they appear immediately after the
appliedControlsExpected/evidencesExpected/securityExceptionsExpected
calculations); simply delete those console.log(...) calls so no debug output is
shipped to production, or replace them with a proper logger call if runtime
diagnostics are required.
🧹 Nitpick comments (6)
frontend/src/lib/components/ModelTable/ModelTable.svelte (1)
707-709: Variable shadowing:hiddenCountreused in template scope.The local
hiddenCounton line 707 shadows the component-levelhiddenCountstate (line 303). While functionally correct due to different scopes, this can cause confusion during maintenance.🔎 Suggested rename
-{@const hiddenCount = hasResolvedNames +{@const itemHiddenCount = hasResolvedNames ? idValues.length - visibleIds.length : 0} ... -{#if hiddenCount > 0} +{#if itemHiddenCount > 0} <p class="text-xs text-yellow-700"> {m.objectsNotVisible({ - count: hiddenCount, - s: hiddenCount === 1 ? '' : 's' + count: itemHiddenCount, + s: itemHiddenCount === 1 ? '' : 's' })} </p> {/if}frontend/src/routes/(app)/(third-party)/compliance-assessments/[id=uuid]/+page.svelte (5)
87-90: Fallback urlModel derivation may produce incorrect values.The fallback
modelField?.field?.replace(/_/g, '-')converts field names likereference_controlstoreference-controls, but this may not always match the actual URL model. IfforeignKeyFieldsdoesn't define aurlModelfor a field, the derived value could be incorrect.Consider verifying that all foreign key fields have explicit
urlModelvalues defined, or adding validation to handle cases where the derived urlModel doesn't exist.
143-145: Unhandled promise rejection in$effect.Using
void refreshRelatedNames()suppresses the promise result but also swallows any errors. IffetchNamesfails (e.g., network error), the error will be silently ignored.🔎 Proposed fix
$effect(() => { - void refreshRelatedNames(); + refreshRelatedNames().catch((err) => { + console.error('Failed to refresh related names:', err); + }); });
536-536: Redundant array check.Since
implementationGroupsDefinitionis initialized withframework?.implementation_groups_definition ?? []on line 64, it's always an array. The!Array.isArray(implementationGroupsDefinition)check is redundant.🔎 Proposed simplification
-if (key === 'selected_implementation_groups' && (!implementationGroupsDefinition || !Array.isArray(implementationGroupsDefinition) || implementationGroupsDefinition.length === 0)) return false; +if (key === 'selected_implementation_groups' && implementationGroupsDefinition.length === 0) return false;
444-446: Address the TODO comment: verify the correct count for sync message.The inline comment
//change thissuggests this count value needs verification. IsframeworkReferenceControls.lengththe correct count for the sync-to-applied-controls confirmation message, or should it reflect the actual number of changes being applied?Would you like me to help determine the correct count value based on the sync action's expected behavior?
492-507: Consider consolidating reactivity patterns.The file mixes Svelte 5 runes (
$effect,$state,$derived) with the legacyrun()function fromsvelte/legacy. This is acceptable during migration, but consider converting theserun()blocks to$effect()for consistency when time permits.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
backend/core/views.pyfrontend/src/lib/components/ModelTable/ModelTable.sveltefrontend/src/lib/utils/related-names.tsfrontend/src/routes/(app)/(third-party)/compliance-assessments/[id=uuid]/+page.sveltefrontend/src/routes/(app)/(third-party)/requirement-assessments/[id=uuid]/+page.sveltefrontend/src/routes/(app)/(third-party)/requirement-assessments/[id=uuid]/edit/+page.sveltefrontend/tests/utils/form-content.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/lib/utils/related-names.ts
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-12-29T15:52:14.696Z
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:14.696Z
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/src/routes/(app)/(third-party)/compliance-assessments/[id=uuid]/+page.sveltefrontend/src/routes/(app)/(third-party)/requirement-assessments/[id=uuid]/+page.sveltefrontend/src/routes/(app)/(third-party)/requirement-assessments/[id=uuid]/edit/+page.svelte
📚 Learning: 2025-11-17T11:09:18.488Z
Learnt from: monsieurswag
Repo: intuitem/ciso-assistant-community PR: 2860
File: frontend/src/routes/(app)/(internal)/[model=urlmodel]/[id=uuid]/+page.server.ts:0-0
Timestamp: 2025-11-17T11:09:18.488Z
Learning: In frontend/src/routes/(app)/(internal)/[model=urlmodel]/[id=uuid]/+page.server.ts, the select action performs batch PATCH operations in Promise.all without transaction semantics. Partial failures (e.g., one object fails to update due to race conditions) are acceptable and should not trigger a fail() return, as most objects will still be updated successfully. Console warnings are sufficient for logging failures.
Applied to files:
frontend/src/lib/components/ModelTable/ModelTable.svelte
📚 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
📚 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
🧬 Code graph analysis (1)
frontend/tests/utils/form-content.ts (1)
frontend/tests/utils/test-utils.ts (1)
expect(470-515)
🪛 ast-grep (0.40.3)
frontend/tests/utils/form-content.ts
[warning] 24-24: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(\\s*/?${escaped}\\s*)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 28-28: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(\\s*/?${escaped}\\s*)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 30-30: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(\\s*(?:/?${escaped}|/?${suffix})\\s*)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 43-43: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^\\s*(?:\\d+-)?.*${escaped}.*$, 'i')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
⏰ 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: startup-functional-test (3.12)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: build_enterprise_frontend
- GitHub Check: build_community_frontend
- GitHub Check: build (3.12)
- GitHub Check: Analyze (python)
🔇 Additional comments (29)
frontend/tests/utils/form-content.ts (6)
3-5: LGTM on helper utilities.The
escapeRegExpfunction correctly escapes all regex meta-characters, andstripLeadingSlashis straightforward.
7-18: LGTM on segment extraction.The
extractLastSegmenthelper correctly handles path-like strings and edge cases (empty segments, trimming).
20-45: Regex patterns are safe due to escaping, butbuildOptionMatchercould be more precise.The static analysis flagged potential ReDoS, but since
escapeRegExpescapes all meta-characters, the constructed patterns won't cause catastrophic backtracking from user input.However,
buildOptionMatcheruses.*${escaped}.*which is intentionally broad. Consider using a more constrained pattern if option labels have a predictable structure, to reduce false matches in tests.
134-135: LGTM on searchbox matcher usage.Switching from direct text containment to regex-based matching aligns with the dynamic name resolution approach.
143-163: LGTM on option matcher usage in SELECT_AUTOCOMPLETE.The matcher-based selection correctly handles both request-based and direct value paths with
.first()to avoid ambiguity.
170-176: LGTM on SELECT_MULTIPLE_AUTOCOMPLETE matcher usage.The iteration with per-value matchers correctly handles multi-select scenarios.
frontend/src/routes/(app)/(third-party)/requirement-assessments/[id=uuid]/edit/+page.svelte (6)
49-52: LGTM on optional chaining for requirement fields.Safe access patterns with fallback defaults prevent runtime errors when data is partially available.
71-77: LGTM on compliance assessment ID extraction.The type guard correctly handles both string and object forms, with a sensible fallback URL.
295-295: LGTM on optional chaining for is_locked.Using
data.complianceAssessment?.is_lockedsafely handles cases where the compliance assessment might not be fully loaded.
317-319: Safe nullish coalescing on implementation_groups iteration.The
?? []fallback ensures the#eachblock won't fail ifimplementation_groupsis undefined.
546-546: Verify folder ID is always a string.The change from
page.data.requirementAssessment.folder.idtopage.data.requirementAssessment.folderassumesfolderis now an ID string rather than an object. Confirm this aligns with the backend serializer changes (IdRelatedField pattern).
633-637: LGTM on optional questions handling.The conditional check and optional chaining prevent errors when questions data is unavailable.
frontend/src/routes/(app)/(third-party)/requirement-assessments/[id=uuid]/+page.svelte (3)
77-82: LGTM on expected count derivations.Consistent pattern with the edit page, using
Array.isArrayguards for safe length access.
299-299: LGTM on expectedCount prop for applied-controls table.Correctly passes the derived count to enable hidden item detection.
316-316: LGTM on expectedCount prop for evidences table.Consistent with the applied-controls table pattern.
frontend/src/lib/components/ModelTable/ModelTable.svelte (8)
13-13: LGTM on imports.The
fetchNamesandisUuidutilities are correctly imported for related name resolution.
88-88: LGTM on expectedCount prop addition.Correctly typed as optional number for backward compatibility.
210-240: LGTM on helper functions for ID extraction.The
extractId,extractIds, andisIdOnlyArrayhelpers correctly handle various input formats (strings, objects, arrays).
303-303: LGTM on hiddenCount state initialization.Correctly initialized to 0.
337-356: LGTM on effects for related names and hidden count.The effects correctly track dependencies and handle edge cases with fallback calculations.
618-624: LGTM on hidden items banner.Clear visual feedback when items are not visible due to permissions.
698-774: LGTM on array rendering logic with ID resolution.The branching correctly handles ID-only arrays vs. object arrays with display labels, with appropriate fallbacks for unresolved names.
792-806: LGTM on single UUID field rendering.Correctly resolves and links UUIDs when labels are available, with fallback display.
backend/core/views.py (2)
631-668: Root-scoped queryset + explicitidfiltering look correctThe updated
get_querysetcorrectly:
- Falls back to root-folder visibility (
Folder.get_root_folder()) when no specific object view is detected.- Layers an explicit
id/id[]query-parameter filter on top of the RBAC-filtered queryset, parsing only valid UUIDs and returning an empty queryset when all supplied IDs are invalid.This keeps permission checks intact and makes the
idfilter predictable without changing existing access semantics. No issues spotted here.
1274-1286: IdRelatedField prefetch/select additions are consistent and safeAcross
ThreatViewSet,AppliedControlViewSet,ComplianceAssessmentViewSet, andRequirementAssessmentViewSet, the addedselect_related/prefetch_relatedcalls for things likelibrary,filtering_labels__folder,perimeter__folder,campaign,assets,evidences, andsecurity_exceptionsalign with the switch toIdRelatedFieldand will cut down on N+1 queries at the serializer layer. The relationships and traversal paths used are valid for the referenced models. No correctness or obvious performance regressions here.Also applies to: 3797-3815, 7575-7592, 8955-8972
frontend/src/routes/(app)/(third-party)/compliance-assessments/[id=uuid]/+page.svelte (4)
57-64: LGTM! Proper null handling and fallback defaults.The
folderIdderivation correctly handles thenullcase before thetypeofcheck. The fallback patterns forframework,frameworkReferenceControls, andimplementationGroupsDefinitionare well-structured with safe defaults.
622-637: LGTM! Proper fallback handling for single UUID values.The rendering logic correctly handles all cases: resolved name with link, resolved name without link (for third-party users), and fallback to raw UUID when name resolution fails.
116-141: Solid implementation of related names caching.The
refreshRelatedNamesfunction efficiently batches ID lookups by model, uses a cache key to prevent redundant fetches, and properly merges updates with existing state. The browser guard prevents SSR issues.
68-82: Thefolderfield is guaranteed to exist in the backend model (FolderMixinusesdefault=Folder.get_root_folder_id), so the defensive check for a falsyfolderis unnecessary. WhilecanPerformActiongracefully handlesundefinedby denying access, the type signature should be corrected to reflect thatdomaincan bestring | undefinedto match what the frontend code can pass.Likely an incorrect or invalid review comment.
| const refreshRelatedNames = async (rowsToInspect: Record<string, any>[] = tableSource.body) => { | ||
| if (!browser || !model?.foreignKeyFields) return; | ||
| if (!Array.isArray(rowsToInspect)) return; | ||
| const idsByModel = new Map<string, Set<string>>(); | ||
| for (const fieldConfig of model.foreignKeyFields) { | ||
| const urlModel = fieldConfig.urlModel ?? fieldConfig.field?.replace(/_/g, '-'); | ||
| if (!urlModel) continue; | ||
| for (const row of rowsToInspect) { | ||
| const ids = extractIds(row[fieldConfig.field]); | ||
| if (ids.length === 0) continue; | ||
| const bucket = idsByModel.get(urlModel) ?? new Set<string>(); | ||
| ids.forEach((id) => bucket.add(id)); | ||
| idsByModel.set(urlModel, bucket); | ||
| } | ||
| } | ||
| const key = JSON.stringify( | ||
| Array.from(idsByModel.entries()).map(([urlModel, ids]) => [urlModel, Array.from(ids).sort()]) | ||
| ); | ||
| if (!key || key === relatedNamesKey) return; | ||
| relatedNamesKey = key; | ||
| const updates: Record<string, Record<string, string>> = {}; | ||
| for (const [urlModel, ids] of idsByModel.entries()) { | ||
| updates[urlModel] = await fetchNames(urlModel, Array.from(ids)); | ||
| } | ||
| if (Object.keys(updates).length > 0) { | ||
| relatedNames = { ...relatedNames, ...updates }; | ||
| } | ||
| }; |
There was a problem hiding this comment.
Error handling still missing in refreshRelatedNames.
Network or API failures in fetchNames will propagate and potentially break the component. Wrap individual fetches in try-catch for resilience.
🔎 Proposed fix
const updates: Record<string, Record<string, string>> = {};
for (const [urlModel, ids] of idsByModel.entries()) {
+ try {
updates[urlModel] = await fetchNames(urlModel, Array.from(ids));
+ } catch (error) {
+ console.warn(`Failed to fetch names for ${urlModel}:`, error);
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const refreshRelatedNames = async (rowsToInspect: Record<string, any>[] = tableSource.body) => { | |
| if (!browser || !model?.foreignKeyFields) return; | |
| if (!Array.isArray(rowsToInspect)) return; | |
| const idsByModel = new Map<string, Set<string>>(); | |
| for (const fieldConfig of model.foreignKeyFields) { | |
| const urlModel = fieldConfig.urlModel ?? fieldConfig.field?.replace(/_/g, '-'); | |
| if (!urlModel) continue; | |
| for (const row of rowsToInspect) { | |
| const ids = extractIds(row[fieldConfig.field]); | |
| if (ids.length === 0) continue; | |
| const bucket = idsByModel.get(urlModel) ?? new Set<string>(); | |
| ids.forEach((id) => bucket.add(id)); | |
| idsByModel.set(urlModel, bucket); | |
| } | |
| } | |
| const key = JSON.stringify( | |
| Array.from(idsByModel.entries()).map(([urlModel, ids]) => [urlModel, Array.from(ids).sort()]) | |
| ); | |
| if (!key || key === relatedNamesKey) return; | |
| relatedNamesKey = key; | |
| const updates: Record<string, Record<string, string>> = {}; | |
| for (const [urlModel, ids] of idsByModel.entries()) { | |
| updates[urlModel] = await fetchNames(urlModel, Array.from(ids)); | |
| } | |
| if (Object.keys(updates).length > 0) { | |
| relatedNames = { ...relatedNames, ...updates }; | |
| } | |
| }; | |
| const refreshRelatedNames = async (rowsToInspect: Record<string, any>[] = tableSource.body) => { | |
| if (!browser || !model?.foreignKeyFields) return; | |
| if (!Array.isArray(rowsToInspect)) return; | |
| const idsByModel = new Map<string, Set<string>>(); | |
| for (const fieldConfig of model.foreignKeyFields) { | |
| const urlModel = fieldConfig.urlModel ?? fieldConfig.field?.replace(/_/g, '-'); | |
| if (!urlModel) continue; | |
| for (const row of rowsToInspect) { | |
| const ids = extractIds(row[fieldConfig.field]); | |
| if (ids.length === 0) continue; | |
| const bucket = idsByModel.get(urlModel) ?? new Set<string>(); | |
| ids.forEach((id) => bucket.add(id)); | |
| idsByModel.set(urlModel, bucket); | |
| } | |
| } | |
| const key = JSON.stringify( | |
| Array.from(idsByModel.entries()).map(([urlModel, ids]) => [urlModel, Array.from(ids).sort()]) | |
| ); | |
| if (!key || key === relatedNamesKey) return; | |
| relatedNamesKey = key; | |
| const updates: Record<string, Record<string, string>> = {}; | |
| for (const [urlModel, ids] of idsByModel.entries()) { | |
| try { | |
| updates[urlModel] = await fetchNames(urlModel, Array.from(ids)); | |
| } catch (error) { | |
| console.warn(`Failed to fetch names for ${urlModel}:`, error); | |
| } | |
| } | |
| if (Object.keys(updates).length > 0) { | |
| relatedNames = { ...relatedNames, ...updates }; | |
| } | |
| }; |
🤖 Prompt for AI Agents
In @frontend/src/lib/components/ModelTable/ModelTable.svelte around lines 242 -
270, refreshRelatedNames currently awaits fetchNames for each urlModel without
error handling, so a single network/API failure can throw and break the
component; update refreshRelatedNames to wrap each call to fetchNames(urlModel,
Array.from(ids)) in a try-catch, handle errors per-urlModel (e.g., log the error
or call an error handler and set updates[urlModel] = {} or skip merging for that
model) and continue fetching remaining urlModels, ensuring relatedNamesKey and
the merge into relatedNames still occur only for successful results. Use the
function name refreshRelatedNames and the fetchNames/relatedNames variables to
locate where to add the try-catch.
| {#if hiddenCount > 0} | ||
| <p class="text-xs text-yellow-700"> | ||
| {m.objectsNotVisible({ | ||
| count: hiddenCount, | ||
| s: hiddenCount === 1 ? '' : 's' | ||
| })} | ||
| </p> | ||
| {/if} |
There was a problem hiding this comment.
Remove the unnecessary s parameter from the objectsNotVisible call.
The message template uses built-in pluralization via count: plural and does not use the s parameter. The correct call should be:
m.objectsNotVisible({ count: hiddenCount })🤖 Prompt for AI Agents
In
@frontend/src/routes/(app)/(third-party)/compliance-assessments/[id=uuid]/+page.svelte
around lines 583 - 590, The call to the i18n/message helper objectsNotVisible
currently passes an unnecessary s parameter; update the invocation (where
objectsNotVisible is called with hiddenCount) to remove the s argument and pass
only { count: hiddenCount } so the template uses built-in pluralization (refer
to objectsNotVisible and hiddenCount in the +page.svelte component).
| {@const itemHref = `/${ | ||
| URL_MODEL_MAP[data.URLModel]['foreignKeyFields']?.find( | ||
| (item) => item.field === key | ||
| )?.urlModel |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use the model constant instead of data.URLModel for consistency.
Lines 604-606 look up foreignKeyFields using data.URLModel, but the model constant is already defined on line 67 as URL_MODEL_MAP['compliance-assessments']. For consistency with the rest of the file, use the model constant.
🔎 Proposed fix
{@const itemHref = `/${
- URL_MODEL_MAP[data.URLModel]['foreignKeyFields']?.find(
+ model.foreignKeyFields?.find(
(item) => item.field === key
)?.urlModel
}/${val.id}`}🤖 Prompt for AI Agents
In
@frontend/src/routes/(app)/(third-party)/compliance-assessments/[id=uuid]/+page.svelte
around lines 603 - 606, The lookup for foreignKeyFields in the itemHref
computation uses data.URLModel but should use the already-defined model constant
for consistency; update the expression inside the template that builds itemHref
to replace data.URLModel with model (so the code uses
URL_MODEL_MAP[model]['foreignKeyFields']?.find((item) => item.field ===
key)?.urlModel) leaving the rest of the logic (itemHref, key, URL_MODEL_MAP, and
foreignKeyFields access) unchanged.
| const appliedControlsExpected = Array.isArray(data.requirementAssessment.applied_controls) | ||
| ? data.requirementAssessment.applied_controls.length | ||
| : 0; | ||
| const evidencesExpected = Array.isArray(data.requirementAssessment.evidences) | ||
| ? data.requirementAssessment.evidences.length | ||
| : 0; | ||
| console.log('data.requirementAssessment.security_exceptions', data.requirementAssessment); | ||
| const securityExceptionsExpected = Array.isArray(data.requirementAssessment.security_exceptions) | ||
| ? data.requirementAssessment.security_exceptions.length | ||
| : 0; | ||
| console.log('securityExceptionsExpected', securityExceptionsExpected); |
There was a problem hiding this comment.
Remove debug console.log statements before merging.
Lines 231 and 236 contain debug logging that should not be shipped to production.
🔎 Proposed fix
const evidencesExpected = Array.isArray(data.requirementAssessment.evidences)
? data.requirementAssessment.evidences.length
: 0;
-console.log('data.requirementAssessment.security_exceptions', data.requirementAssessment);
const securityExceptionsExpected = Array.isArray(data.requirementAssessment.security_exceptions)
? data.requirementAssessment.security_exceptions.length
: 0;
-
-console.log('securityExceptionsExpected', securityExceptionsExpected);🤖 Prompt for AI Agents
In
@frontend/src/routes/(app)/(third-party)/requirement-assessments/[id=uuid]/edit/+page.svelte
around lines 224 - 236, Remove the two debug console.log statements that
reference data.requirementAssessment and securityExceptionsExpected in the
+page.svelte file (they appear immediately after the
appliedControlsExpected/evidencesExpected/securityExceptionsExpected
calculations); simply delete those console.log(...) calls so no debug output is
shipped to production, or replace them with a proper logger call if runtime
diagnostics are required.
Summary by CodeRabbit
Release Notes
New Features
Improvements
Internationalization
✏️ Tip: You can customize this high-level summary in your review settings.