make domain changeable on all objects forms #3106
Conversation
📝 WalkthroughWalkthroughAdds domain-branch mismatch checks to assessment quality checks; wraps many serializer updates in transactions that cascade folder changes and enqueue post-commit notifications; adds translation keys; and makes folder/perimeter fields visible/selectable across many frontend forms. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client (UI/API)
participant API as API View
participant Serializer as WriteSerializer.update
participant DB as Database
participant Related as Related Models
participant Notif as Notification/Worker
Note over Client,API: Update request (PATCH/PUT)
Client->>API: send update payload
API->>Serializer: call .update(instance, validated_data)
Serializer->>DB: BEGIN transaction (transaction.atomic)
Serializer->>DB: update instance record
alt folder_id changed
Serializer->>Related: bulk update related objects' folder (cascade)
Related-->>DB: apply bulk updates
end
Serializer->>DB: COMMIT transaction
DB-->>Serializer: transaction.on_commit callbacks registered
Note over Serializer,Notif: post-commit actions
Serializer->>Notif: schedule assignment/notifications via transaction.on_commit
Notif->>Notif: dispatch notifications/tasks (async worker)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)backend/app_tests/api/test_api_perimeters.py (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
🔇 Additional comments (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
frontend/messages/en.json (1)
3027-3031: Consider more idiomatic phrasing for domain mismatch messages.The new translation keys are clear and consistent, which is good. However, the phrasing "belongs to another domain than" could be slightly more idiomatic in English. Consider using "belongs to a different domain from" or "is in a different domain than" for more natural-sounding messages.
Example:
- Current: "Requirement assessment belongs to another domain than the audit"
- Alternative: "Requirement assessment belongs to a different domain from the audit"
This is purely stylistic—the current phrasing is grammatically acceptable and will be understood by users.
🔎 Proposed refinement
- "requirementAssessmentDifferentDomain": "Requirement assessment belongs to another domain than the audit", - "appliedControlDifferentDomain": "Applied control belongs to another domain than the assessment", - "riskAcceptanceDifferentDomain": "Risk acceptance belongs to another domain than the risk assessment", - "evidenceDifferentDomain": "Evidence belongs to another domain than the assessment", - "assetDifferentDomain": "Asset belongs to another domain than the assessment" + "requirementAssessmentDifferentDomain": "Requirement assessment belongs to a different domain from the audit", + "appliedControlDifferentDomain": "Applied control belongs to a different domain from the assessment", + "riskAcceptanceDifferentDomain": "Risk acceptance belongs to a different domain from the risk assessment", + "evidenceDifferentDomain": "Evidence belongs to a different domain from the assessment", + "assetDifferentDomain": "Asset belongs to a different domain from the assessment"backend/core/models.py (2)
4919-4935: Sharedis_domain_mismatchhelper is correct but duplicated and mildly type-unsafe at the root helper boundaryThe domain comparison logic (null guard, root-folder bypass, then folder-id inequality) looks consistent and matches the intended “root is global, others must match the assessment folder” semantics. However, the same closure is duplicated in both
RiskAssessment.quality_checkandComplianceAssessment.quality_check, and both depend onFolder.get_root_folder_id()which (periam.models.get_root_folder_id) can theoretically return either an id or aFolderinstance on its fallback path, relying onstr(...)coercion for comparison.Extracting a small shared utility (e.g. a module-level
def is_domain_mismatch(folder_id, assessment_folder_id)) that internally normalizes the root folder strictly to an id would:
- Remove the duplication between the two quality checks.
- Guarantee consistent typing regardless of how
get_root_folder_idis implemented or evolves.Also applies to: 6334-6351
5139-5156: Applied-control domain dedupe works; consider upstream distinct() if you ever want to dedupe other warnings tooUsing
domain_checked_controls = set()to avoid emitting multiple domain-mismatch warnings for the sameAppliedControlid is a good way to keep findings readable, given that the serialized lists can contain duplicates when a control is linked to multiple scenarios/requirements.If, in the future, you also want to avoid duplicated other warnings for such controls (
appliedControlNoETA,appliedControlNoEffort, etc.), you might additionally apply.distinct()on theAppliedControlquerysets before serialization. That would centralize deduplication at the queryset level instead of per-check, without changing current behavior for these newly-added domain warnings.Also applies to: 6447-6467
backend/core/tests/test_models.py (2)
395-404: Consider using a pytest fixture for root folder setup.The
_get_root_foldermethod creates side effects by potentially creating a root folder during tests. Consider using a pytest fixture (e.g., similar to other fixtures in the file) to ensure consistent test isolation and clearer dependency management.Alternative approach using a fixture
@pytest.fixture def root_folder(): """Ensure a root folder exists for domain mismatch tests.""" 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_folderThen use it as a parameter in test methods:
def test_risk_assessment_quality_check_warns_on_cross_domain_links(self, root_folder): folder_a = Folder.objects.create( name="domain-a", description="domain a", parent_folder=root_folder, ) # ... rest of test
406-467: Consider adding a complementary positive test case.The test correctly validates that cross-domain entity links trigger warnings. Consider adding a test that verifies entities within the same domain do NOT produce these warnings, ensuring the domain mismatch logic works bidirectionally.
Example complementary test
def test_risk_assessment_quality_check_no_warnings_for_same_domain(self): root_folder = self._get_root_folder() folder_a = Folder.objects.create( name="domain-a", description="domain a", parent_folder=root_folder, ) risk_matrix = RiskMatrix.objects.create( name="risk matrix", description="risk matrix description", json_definition=RISK_MATRIX_JSON_DEFINITION, folder=folder_a, ) perimeter = Perimeter.objects.create(name="perimeter", folder=folder_a) risk_assessment = RiskAssessment.objects.create( name="risk assessment", description="risk assessment description", perimeter=perimeter, risk_matrix=risk_matrix, folder=folder_a, ) scenario = RiskScenario.objects.create( name="scenario", description="scenario description", risk_assessment=risk_assessment, ) # All entities in folder_a (same domain) applied_control = AppliedControl.objects.create( name="applied control", description="applied control description", folder=folder_a, ) scenario.applied_controls.add(applied_control) findings = risk_assessment.quality_check() warning_ids = {item["msgid"] for item in findings["warnings"]} # No domain mismatch warnings should be present assert "appliedControlDifferentDomain" not in warning_ids assert "riskAcceptanceDifferentDomain" not in warning_ids assert "assetDifferentDomain" not in warning_ids assert "evidenceDifferentDomain" not in warning_idsfrontend/src/lib/components/Forms/ModelForm/EvidenceRevisionForm.svelte (1)
77-92: Consider removing commented-out code.The commented block shows the previous conditional logic for the folder field. Since the new implementation is complete and aligned with PR objectives, this commented code can be removed to improve maintainability and reduce clutter.
🔎 Suggested cleanup
-<!-- {#if !(initialData.applied_controls || initialData.requirement_assessments || initialData.evidence)} - <AutocompleteSelect - {form} - optionsEndpoint="folders?content_type=DO&content_type=GL" - field="folder" - pathField="path" - cacheLock={cacheLocks['folder']} - bind:cachedValue={formDataCache['folder']} - label={m.domain()} - hidden={initialData.applied_controls || - initialData.requirement_assessments || - initialData.folder} - /> -{:else} - <HiddenInput {form} field="folder" /> -{/if} -->
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (34)
backend/core/models.pybackend/core/tests/test_models.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/CampaignForm.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/EntityForm.sveltefrontend/src/lib/components/Forms/ModelForm/EvidenceRevisionForm.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/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.svelte
💤 Files with no reviewable changes (27)
- frontend/src/lib/components/Forms/ModelForm/DataBreachForm.svelte
- frontend/src/lib/components/Forms/ModelForm/RightRequestForm.svelte
- frontend/src/lib/components/Forms/ModelForm/EntityForm.svelte
- frontend/src/lib/components/Forms/ModelForm/ReferenceControlForm.svelte
- frontend/src/lib/components/Forms/ModelForm/OrganisationObjectiveForm.svelte
- frontend/src/lib/components/Forms/ModelForm/SecurityExceptionForm.svelte
- frontend/src/lib/components/Forms/ModelForm/AccreditationForm.svelte
- frontend/src/lib/components/Forms/ModelForm/RiskAcceptanceForm.svelte
- frontend/src/lib/components/Forms/ModelForm/GenericCollectionForm.svelte
- frontend/src/lib/components/Forms/ModelForm/EbiosRmForm.svelte
- frontend/src/lib/components/Forms/ModelForm/PerimeterForm.svelte
- frontend/src/lib/components/Forms/ModelForm/QuantitativeRiskStudyForm.svelte
- frontend/src/lib/components/Forms/ModelForm/ValidationFlowForm.svelte
- frontend/src/lib/components/Forms/ModelForm/VulnerabilitiesForm.svelte
- frontend/src/lib/components/Forms/ModelForm/ElementaryActionForm.svelte
- frontend/src/lib/components/Forms/ModelForm/OrganisationIssueForm.svelte
- frontend/src/lib/components/Forms/ModelForm/CustomMetricSampleForm.svelte
- frontend/src/lib/components/Forms/ModelForm/ContractForm.svelte
- frontend/src/lib/components/Forms/ModelForm/ThreatForm.svelte
- frontend/src/lib/components/Forms/ModelForm/ProcessingForm.svelte
- frontend/src/lib/components/Forms/ModelForm/IncidentForm.svelte
- frontend/src/lib/components/Forms/ModelForm/AssetForm.svelte
- frontend/src/lib/components/Forms/ModelForm/DashboardTextWidgetForm.svelte
- frontend/src/lib/components/Forms/ModelForm/DashboardWidgetForm.svelte
- frontend/src/lib/components/Forms/ModelForm/MetricDefinitionForm.svelte
- frontend/src/lib/components/Forms/ModelForm/CampaignForm.svelte
- frontend/src/lib/components/Forms/ModelForm/DashboardBuiltinWidgetForm.svelte
🧰 Additional context used
🧠 Learnings (1)
📚 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/lib/components/Forms/ModelForm/EvidenceRevisionForm.svelte
🧬 Code graph analysis (2)
backend/core/tests/test_models.py (1)
backend/core/models.py (12)
create(4626-4628)RiskAssessment(4782-5320)AppliedControl(4056-4496)applied_controls(7536-7537)RiskAcceptance(7200-7288)risk_scenarios(4333-4334)Asset(2417-3334)assets(7532-7533)Evidence(3727-3814)quality_check(4912-5317)quality_check(6325-6558)RequirementAssessment(6708-7000)
backend/core/models.py (1)
backend/iam/models.py (1)
get_root_folder_id(92-97)
⏰ 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-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: build_enterprise_frontend
- GitHub Check: build_community_frontend
- GitHub Check: build (3.12)
- GitHub Check: test (3.12)
- GitHub Check: Analyze (python)
🔇 Additional comments (6)
frontend/src/lib/components/Forms/ModelForm/TimelineEntryForm.svelte (1)
34-42: All verification concerns confirmed as correct and intentionally designed.
Content type parameters: The
content_type=DO&content_type=GLquery parameters are correct. Migration 0069 confirms these values (DO for Domain, GL for Global), and this pattern is used consistently across 30+ forms in the codebase for multi-domain objects.Context-independent editability: The folder field being always visible (not conditionally rendered based on context) is an intentional design pattern. All examined forms consistently show the folder field as always editable, unlike the incident field which has explicit context checks. This pattern is used for forms where domain/folder should be changeable.
Backend domain validation: The backend enforces domain consistency automatically. TimelineEntry's
save()method includesself.folder = self.incident.folder(line 4043), which synchronizes the folder to always match its parent incident's folder. This prevents cross-domain relationship inconsistencies at the model level.The implementation correctly aligns with the PR objective to make domains changeable on forms, and all concerns are properly addressed by the existing design and backend constraints.
frontend/messages/fr.json (1)
3026-3030: These translation keys are defined but not used in the codebase.The English and French translations exist in both message files and are grammatically correct. However, a search for references to these keys across TypeScript, JavaScript, and Vue components returned no results, indicating these translations are not currently utilized in the UI.
Either:
- The implementation to use these keys in domain-mismatch validation warnings is missing, or
- These translations are orphaned and should be removed
Verify whether the backend validation logic that should emit these warnings has been implemented, and update the component code to reference these keys accordingly.
frontend/src/lib/components/Forms/ModelForm/RequirementAssessmentForm.svelte (1)
74-82: The folders endpoint correctly supports content_type filtering throughFolderFilter.content_type, and the backend properly handlesDOandGLparameters. Domain change validation is implemented viaComplianceAssessment.is_domain_mismatch(), which generates warnings (not blocks) for cross-domain relationships and uses the translation keys correctly. The AutocompleteSelect implementation is correct and follows established patterns. No issues identified.backend/core/models.py (1)
5231-5242: Domain checks for risk acceptances, assets and evidences look consistent and efficiently scopedThe new quality checks for:
RiskAcceptancevsRiskAssessmentfolder (riskAcceptanceDifferentDomain),- assets linked to risk scenarios (
assetDifferentDomain),- evidences linked via applied controls in risk assessments (
evidenceDifferentDomain),- evidences linked either via applied controls or directly to a compliance assessment, and
- assets linked to a compliance assessment
all consistently reuse the same
is_domain_mismatchlogic and limit queries to onlyid,name, andfolder_idwith appropriate.distinct()where needed. This keeps the additional checks cheap while broadening coverage of cross-domain inconsistencies exactly in the places the PR description calls out.Also applies to: 5269-5310, 6484-6485, 6513-6551
backend/core/tests/test_models.py (1)
469-531: LGTM! Comprehensive compliance assessment domain mismatch test.The test thoroughly validates domain mismatch warnings for compliance assessments, covering requirement assessments, applied controls, evidence, and assets. The setup correctly places entities across different domains and verifies the expected warnings are emitted.
frontend/src/lib/components/Forms/ModelForm/EvidenceRevisionForm.svelte (1)
4-4: LGTM!The AutocompleteSelect import is necessary for the new folder field implementation.
tchoumi313
requested review from
nas-tabchiche
and removed request for
nas-tabchiche
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
backend/core/models.py (3)
4919-4936: Domain-mismatch helper and applied-control deduplication are sound, but duplicatedThe
is_domain_mismatchhelper correctly:
- Treats missing folders and root-folder participation as “no mismatch”.
- Compares folder IDs via string to be robust to different PK types.
And thedomain_checked_controlsset avoids spamming duplicate findings when the sameAppliedControlis linked to multiple scenarios.You now have essentially identical
is_domain_mismatchimplementations (and very similar control‑dedup logic) in bothRiskAssessment.quality_checkandComplianceAssessment.quality_check. Consider extracting a small shared utility (e.g. a staticmethod onAssessmentor a helper in this module) that:
- Builds the assessment “domain context” (root vs assessment folder).
- Performs the mismatch test and optionally handles deduplication on an ID set.
This would reduce drift if the domain rules evolve again.
Also applies to: 5139-5156
6334-6351: ComplianceAssessment domain helper and requirement-assessment checks align with RiskAssessmentThe
ComplianceAssessment.quality_checkdomain helper mirrors the RiskAssessment one and is used immediately to flagRequirementAssessments whosefolder_iddiffers from the assessment (excluding the root case). Usingra.folder_id(model field, not serialized) keeps this cheap, and serializing tora_dictonly once per RA is fine.Given this logic is duplicated with the risk side, it would benefit from the same shared helper abstraction mentioned earlier, but functionally it looks correct.
Also applies to: 6386-6397
6437-6439: Applied-control domain checks in compliance quality_check behave as intendedThe introduction of
applied_control_qsanddomain_checked_controls:
- Prevents repeated “different branch” findings when the same
AppliedControlis linked to multiple requirement assessments.- Reuses the same folder-based mismatch criteria as elsewhere.
- Leaves the existing
appliedControlNoReferenceControlcheck unchanged.If you ever run into performance issues with very large assessments, a small further optimization would be to short‑circuit on
id in domain_checked_controlsbefore callingis_domain_mismatch, but that’s purely micro‑level.Also applies to: 6442-6442, 6447-6467
backend/core/tests/test_models.py (1)
395-404: Consider relying on existing fixtures for root folder.The
_get_root_folder()helper creates a root folder if it doesn't exist, but fixtures used in other tests (e.g.,domain_perimeter_fixtureat line 1312) likely already handle root folder creation. Creating it conditionally here could mask fixture issues or create inconsistent test state.Suggested simplification
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.get_root_folder()If
Folder.get_root_folder()can return None, consider adding a fixture or using@pytest.mark.usefixturesto ensure the root folder exists for all tests.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
backend/core/models.pybackend/core/serializers.pybackend/core/tests/test_models.pyfrontend/messages/en.jsonfrontend/messages/fr.jsonfrontend/src/lib/components/Forms/ModelForm/BusinessImpactAnalysisForm.sveltefrontend/src/lib/components/Forms/ModelForm/ComplianceAssessmentForm.sveltefrontend/src/lib/components/Forms/ModelForm/EntityAssessmentForm.sveltefrontend/src/lib/components/Forms/ModelForm/FindingsAssessmentForm.sveltefrontend/src/lib/components/Forms/ModelForm/RiskAssessmentForm.svelte
💤 Files with no reviewable changes (5)
- frontend/src/lib/components/Forms/ModelForm/ComplianceAssessmentForm.svelte
- frontend/src/lib/components/Forms/ModelForm/FindingsAssessmentForm.svelte
- frontend/src/lib/components/Forms/ModelForm/EntityAssessmentForm.svelte
- frontend/src/lib/components/Forms/ModelForm/BusinessImpactAnalysisForm.svelte
- frontend/src/lib/components/Forms/ModelForm/RiskAssessmentForm.svelte
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/messages/fr.json
- frontend/messages/en.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 (5)
backend/core/models.py (2)
5231-5242: RiskAcceptance / Asset / Evidence domain checks are consistent and efficientThe new checks for:
RiskAcceptancelinked to scenarios,Assetlinked viarisk_scenarios,Evidencelinked viaapplied_controls,all:
- Use relational filters (
distinct()+only('id', 'name', 'folder_id')) to avoid N+1 queries.- Reuse the same
is_domain_mismatchsemantics and emit clearly scopedinfofindings with stablemsgids.This looks correct and fits the intended “cross‑branch” detection behavior.
Also applies to: 5269-5288, 5289-5309
6513-6535: Evidence and asset domain checks for compliance assessments look correctThe new blocks that:
- Build
domain_evidence_objectsfrom bothapplied_control_qsandself.evidences, and- Iterate over
self.assets,use
.distinct().only('id', 'name', 'folder_id')and the sharedis_domain_mismatchhelper, yielding a single info finding per evidence/asset that is outside the assessment’s folder (except when either side is root).This should cover both direct and control‑linked evidences and assets without duplicates, and the message payloads (
msgid,link,obj_type,object) are consistent with the risk‑assessment side.Also applies to: 6537-6551
backend/core/tests/test_models.py (2)
406-467: Comprehensive test coverage for RiskAssessment domain-mismatch detection.This test properly validates that the quality_check method detects cross-domain references for:
- Applied controls in different domains
- Risk acceptances in different domains
- Assets in different domains
- Evidence in different domains (via applied control)
The test correctly sets up a scenario where folder_a contains the assessment while folder_b contains all related objects, ensuring domain-mismatch warnings are triggered.
469-531: Good test coverage for ComplianceAssessment domain-mismatch detection.This test validates that quality_check detects cross-domain references for requirement assessments, applied controls, evidence, and assets. The test correctly creates objects in
folder_bwhile the compliance assessment is infolder_a, ensuring domain-mismatch warnings are triggered.Note: The test bypasses the serializer's folder cascade logic (lines 1870-1874 in serializers.py) by creating objects directly via ORM. This is correct—quality_check should detect inconsistencies from any source (manual creation, imports, legacy data).
backend/core/serializers.py (1)
1848-1848: Folder cascade logic is correct and requires no additional cascading.The cascade from ComplianceAssessment to RequirementAssessment correctly mirrors the established pattern used for Evidence→EvidenceRevision (lines 1574-1578) and TaskTemplate→TaskNode (lines 2610-2614).
RequirementAssessment's relationships to Evidence and AppliedControl do not require folder cascading. Unlike EvidenceRevision and TaskNode (which are child objects owned by their parents), Evidence and AppliedControl are independent domain objects with their own folder associations. The many-to-many relationships are simple association links, not ownership relationships, and Evidence/AppliedControl maintain their folder context independently across multiple assessments.
| requirement = RequirementNode.objects.create( | ||
| name="requirement", | ||
| description="requirement description", | ||
| framework=framework, | ||
| assessable=True, | ||
| folder=folder_b, | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for RequirementNode model definition and folder field
rg -n -A 10 'class RequirementNode' backend/core/models.py | head -50Repository: intuitem/ciso-assistant-community
Length of output: 210
🏁 Script executed:
#!/bin/bash
# Search for quality_check method and requirementNodeDifferentBranch references
rg -n 'requirementNodeDifferentBranch|quality_check' backend/core/models.py | head -30Repository: intuitem/ciso-assistant-community
Length of output: 160
🏁 Script executed:
#!/bin/bash
# Search for framework folder relationship in RequirementNode
rg -n -B 3 -A 3 'framework.*folder|RequirementNode.*folder' backend/core/models.py | head -100Repository: intuitem/ciso-assistant-community
Length of output: 59
🏁 Script executed:
#!/bin/bash
# Check serializers for framework folder cascade logic
rg -n -B 5 -A 5 'requirement.*folder\|framework.*folder' backend/core/serializers.py | head -100Repository: intuitem/ciso-assistant-community
Length of output: 59
🏁 Script executed:
#!/bin/bash
# Find RequirementNode model definition
rg -n 'class RequirementNode' backend/core/models.py -A 20 | head -60Repository: intuitem/ciso-assistant-community
Length of output: 889
🏁 Script executed:
#!/bin/bash
# Look at quality_check method around line 4912
sed -n '4900,4950p' backend/core/models.pyRepository: intuitem/ciso-assistant-community
Length of output: 2025
🏁 Script executed:
#!/bin/bash
# Search for folder field in RequirementNode and related validations
rg -n 'folder.*=' backend/core/models.py | grep -i 'requirement\|framework' | head -20Repository: intuitem/ciso-assistant-community
Length of output: 59
🏁 Script executed:
#!/bin/bash
# Check the test file to understand what's being tested
sed -n '469,531p' backend/core/tests/test_models.pyRepository: intuitem/ciso-assistant-community
Length of output: 2545
🏁 Script executed:
#!/bin/bash
# Find ComplianceAssessment quality_check method (likely around line 6325)
sed -n '6320,6450p' backend/core/models.pyRepository: intuitem/ciso-assistant-community
Length of output: 5533
🏁 Script executed:
#!/bin/bash
# Search for any requirementNodeDifferentBranch or similar checks
rg -n 'requirementNodeDifferentBranch|requirement.*DifferentBranch' backend/core/models.pyRepository: intuitem/ciso-assistant-community
Length of output: 154
🏁 Script executed:
#!/bin/bash
# Look at RequirementNode model more completely to see folder field definition
sed -n '2080,2150p' backend/core/models.pyRepository: intuitem/ciso-assistant-community
Length of output: 2597
🏁 Script executed:
#!/bin/bash
# Check if there are any folder validation constraints on RequirementNode
rg -n -B 5 -A 5 'class Meta' backend/core/models.py | grep -A 10 'RequirementNode' | head -30Repository: intuitem/ciso-assistant-community
Length of output: 400
Add assertion for requirementNodeDifferentBranch or update quality_check to warn about framework-requirement folder mismatch.
The test creates a RequirementNode in folder_b (line 499) while its parent framework is in folder_a (line 484). The quality_check() method already detects and warns about similar cross-domain mismatches for RequirementAssessment, AppliedControl, Evidence, and Asset, but does not check if a RequirementNode's folder differs from its Framework's folder. Either add an assertion confirming this scenario is detected, or update quality_check() to include the missing check for consistency.
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)
backend/core/serializers.py
🧰 Additional context used
🧬 Code graph analysis (1)
backend/core/serializers.py (1)
backend/core/models.py (2)
update(1209-1221)RequirementAssessment(6708-7000)
⏰ 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 (2)
backend/core/serializers.py (2)
1860-1864: LGTM — folder cascade logic is correct.The cascade update to
RequirementAssessmentobjects is appropriately guarded by the folder change check, uses an efficient bulk update, and is consistent with similar patterns elsewhere in this file (e.g.,EvidenceWriteSerializer,TaskTemplateWriteSerializer).
1871-1876: Good pattern: deferring notifications to post-commit.Using
transaction.on_commit()ensures notifications are only dispatched after the database changes are successfully committed, preventing notifications for failed updates. This is a good improvement over the immediate notification pattern used in other serializers likeAppliedControlWriteSerializer.
eric-intuitem
left a comment
eric-intuitem
left a comment
There was a problem hiding this comment.
- I've added transactions and missing cascading effects
- allow domain change for ebios-rm study
- the main entity should be stuck to Global domain (the domain should be read-only for that)
- For perimeter, the domain should be visible in read-only mode (better than invisible)
- We need to do the other changes mentioned in CA-1456:
1/ suppress the filtering mechanism based on SCOPE_FOLDER_ID
2/ remove fieldsrelatedfields everywhere
3/ fix all broken pages by using dynamic fetch of related fields
4/ check with non-regression tests that we have fixed all pages
the other changes of CA-1456 where done here #3101 (still working on fixing the tests). Okay, i am going to adjust main entity and perimeter folder view |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
backend/core/models.py (2)
4925-4942: Validate folder‑equality logic for “domain branch” and confirm it matches business semanticsThe new
is_domain_mismatchhelper plus the checks for applied controls, risk acceptances, assets, and evidences are coherent and the root-folder special‑case is sensible (treating the global folder as compatible with everything). The deduplication viadomain_checked_controlsalso avoids spamming multiple findings for the sameAppliedControl.However, the current implementation assumes that “same domain/branch” ⇔
folder_idequality (excluding root):
- Any object whose
folder_iddiffers fromself.folder_idis treated as a different branch, even if both folders might live under the same top‑level “domain” node.- If domains in your data model are actually “folder subtrees” (e.g., domain root folder with nested children), this will flag mismatches between siblings/descendants in the same domain.
If your intention is strict “same folder” constraints, this is correct. If “domain branch” really means “same top‑level domain folder (ancestor)”, you may want to compare on a normalized domain identifier (e.g., top‑level ancestor or
folder.domain_id) instead of rawfolder_id.I’d recommend double‑checking the domain semantics and, if needed, centralizing the logic in a reusable helper on
Assessment/FolderMixin(e.g.,_is_domain_mismatch(other_folder_id)) to keep RiskAssessment and ComplianceAssessment in sync.Also applies to: 5145-5162, 5237-5315
6381-6398: DRY up domain‑mismatch helper and confirm evidence/asset coverage is what you expectThe ComplianceAssessment quality check mirrors the RiskAssessment changes:
- Same
is_domain_mismatchlogic (root folder special‑casing, simplefolder_idinequality).- Domain warnings for requirement assessments, applied controls (with
domain_checked_controlsto avoid duplicates), evidences (via both applied controls and directcompliance_assessmentslink), and assets.Functionally this looks consistent and the reuse of
applied_control_qsboth for serialization and evidence queries is a nice improvement.Two follow‑ups worth considering:
Shared helper instead of duplication
The
is_domain_mismatchimplementation is now duplicated between RiskAssessment and ComplianceAssessment. To avoid drift later, you could extract it into:
- a protected method on
Assessment(e.g.,_is_domain_mismatch(self, folder_id)), or- a small utility on
Folder/FolderMixinthat takes both folder IDs.Confirm strict folder‑equality vs. “same branch” semantics
As with RiskAssessment, this logic treats any non‑root
folder_iddifferent fromself.folder_idas a different branch. If “domain” is actually modeled as a subtree under a domain root folder, you may want to compare using a normalized domain anchor rather than directfolder_id.If strict folder‑level matching is indeed the intended contract for all these relations, then the current implementation is fine; otherwise, a shared, domain‑aware helper will make it easier to adjust the behavior in one place later.
Also applies to: 6433-6444, 6484-6486, 6494-6515, 6531-6532, 6560-6585
backend/core/serializers.py (1)
2657-2676: Consider usingtransaction.on_commit()for notification consistency.The transaction handling and folder cascade are correct. However, for consistency with
ComplianceAssessmentWriteSerializer, consider wrapping the notification call intransaction.on_commit(). This is more robust in nested transaction scenarios.🔎 Suggested change for consistency
with transaction.atomic(): instance = super().update(instance, validated_data) now_recurrent = instance.is_recurrent self._sync_task_node(instance, tasknode_data, was_recurrent, now_recurrent) # Update all TaskNodes' folder if the TaskTemplate's folder changed if old_folder_id != instance.folder_id: TaskNode.objects.filter(task_template=instance).update( folder=instance.folder ) - # Get new assigned users after update - new_assigned_ids = set(instance.assigned_to.values_list("id", flat=True)) - - # Send notifications only to newly assigned users - newly_assigned_ids = new_assigned_ids - old_assigned_ids - if newly_assigned_ids: - self._send_assignment_notifications(instance, list(newly_assigned_ids)) + # Get new assigned users after update + new_assigned_ids = set(instance.assigned_to.values_list("id", flat=True)) + newly_assigned_ids = new_assigned_ids - old_assigned_ids + + # Schedule notifications to run after transaction commits + if newly_assigned_ids: + transaction.on_commit( + lambda: self._send_assignment_notifications( + instance, list(newly_assigned_ids) + ) + ) return instance
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
backend/core/models.pybackend/core/serializers.pyfrontend/messages/en.jsonfrontend/messages/fr.jsonfrontend/src/lib/components/Forms/ModelForm/ComplianceAssessmentForm.sveltefrontend/src/lib/components/Forms/ModelForm/MetricDefinitionForm.sveltefrontend/src/lib/components/Forms/ModelForm/OrganisationObjectiveForm.sveltefrontend/src/lib/components/Forms/ModelForm/RequirementAssessmentForm.sveltefrontend/src/lib/components/Forms/ModelForm/SecurityExceptionForm.sveltefrontend/src/lib/components/Forms/ModelForm/ValidationFlowForm.svelte
💤 Files with no reviewable changes (5)
- frontend/src/lib/components/Forms/ModelForm/SecurityExceptionForm.svelte
- frontend/src/lib/components/Forms/ModelForm/MetricDefinitionForm.svelte
- frontend/src/lib/components/Forms/ModelForm/ComplianceAssessmentForm.svelte
- frontend/src/lib/components/Forms/ModelForm/ValidationFlowForm.svelte
- frontend/src/lib/components/Forms/ModelForm/OrganisationObjectiveForm.svelte
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/messages/fr.json
- frontend/src/lib/components/Forms/ModelForm/RequirementAssessmentForm.svelte
🧰 Additional context used
🧠 Learnings (2)
📚 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
🧬 Code graph analysis (1)
backend/core/models.py (1)
backend/iam/models.py (1)
get_root_folder_id(92-97)
🪛 Biome (2.1.2)
frontend/messages/en.json
[error] 3057-3058: 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: build_enterprise_frontend
- GitHub Check: build_community_frontend
- GitHub Check: startup-docker-compose-test
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: test (3.12)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: build (3.12)
- GitHub Check: Analyze (python)
🔇 Additional comments (5)
backend/core/serializers.py (5)
5-5: LGTM! Import fix addresses the critical issue.The
transactionimport is now correctly added to support thetransaction.atomic()andtransaction.on_commit()calls used throughout the serializers.
1564-1581: LGTM! Transaction handling and folder cascade are correctly implemented.The
transaction.atomic()block ensures atomicity between the Evidence update and the cascading folder update to relatedEvidenceRevisionrecords. This aligns with the learned behavior that EvidenceRevision handles folder assignment from its parent Evidence.
1847-1880: Well-structured transaction handling with proper notification deferral.Good implementation:
transaction.atomic()ensures the folder cascade toRequirementAssessmentis atomic with the parent updatetransaction.on_commit()correctly defers email notifications until after successful commit, preventing notifications for rolled-back updates- Auto-lock on status deprecation is preserved from before
2531-2540: LGTM! Clean implementation following the established pattern.The new
updatemethod correctly cascades folder changes to relatedTimelineEntryrecords within a transaction, ensuring data consistency.
2864-2956: LGTM! Both update paths handle transactions and folder cascades correctly.Good implementation details:
- Both branches (status change vs non-status change) properly wrap updates in
transaction.atomic()- Folder changes are cascaded to
FlowEventrecords in both paths- The
FlowEventcreation in the status-change branch already usesupdated_instance.folder, so the subsequent bulk update is idempotent (harmless redundancy)- Permission checks are correctly performed before entering the transaction block
| "libraryFilteringLabels": "Library labels", | ||
| "requirementAssessmentDifferentBranch": "Requirement assessment belongs to another branch than the audit", | ||
| "appliedControlDifferentBranch": "Applied control belongs to another branch than the assessment", | ||
| "riskAcceptanceDifferentBranch": "Risk acceptance belongs to another branch than the risk assessment", | ||
| "evidenceDifferentBranch": "Evidence belongs to another branch than the assessment", | ||
| "assetDifferentBranch": "Asset belongs to another branch than the assessment", |
There was a problem hiding this comment.
Critical: Duplicate JSON key libraryFilteringLabels detected.
The key libraryFilteringLabels is defined at both line 3057 and line 3073. In JSON, only the last definition is used, causing line 3057's value to be ignored at runtime. This duplicate should be removed.
🔎 Proposed fix
- "libraryFilteringLabels": "Library labels",
"requirementAssessmentDifferentBranch": "Requirement assessment belongs to another branch than the audit",
"appliedControlDifferentBranch": "Applied control belongs to another branch than the assessment",
"riskAcceptanceDifferentBranch": "Risk acceptance belongs to another branch than the risk assessment",
"evidenceDifferentBranch": "Evidence belongs to another branch than the assessment",
"assetDifferentBranch": "Asset belongs to another branch than the assessment",📝 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.
| "libraryFilteringLabels": "Library labels", | |
| "requirementAssessmentDifferentBranch": "Requirement assessment belongs to another branch than the audit", | |
| "appliedControlDifferentBranch": "Applied control belongs to another branch than the assessment", | |
| "riskAcceptanceDifferentBranch": "Risk acceptance belongs to another branch than the risk assessment", | |
| "evidenceDifferentBranch": "Evidence belongs to another branch than the assessment", | |
| "assetDifferentBranch": "Asset belongs to another branch than the assessment", | |
| "requirementAssessmentDifferentBranch": "Requirement assessment belongs to another branch than the audit", | |
| "appliedControlDifferentBranch": "Applied control belongs to another branch than the assessment", | |
| "riskAcceptanceDifferentBranch": "Risk acceptance belongs to another branch than the risk assessment", | |
| "evidenceDifferentBranch": "Evidence belongs to another branch than the assessment", | |
| "assetDifferentBranch": "Asset belongs to another branch than the assessment", |
🧰 Tools
🪛 Biome (2.1.2)
[error] 3057-3058: 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)
🤖 Prompt for AI Agents
In frontend/messages/en.json around lines 3057 to 3062 (duplicate key also
appears at ~3073), the JSON key "libraryFilteringLabels" is defined twice which
causes the earlier value (line 3057) to be ignored at runtime; remove the
duplicate entry so the key is only defined once (keep the intended/accurate
value — typically the later one at ~3073) or merge/update the values if needed,
then validate the JSON to ensure no other duplicate keys remain.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
backend/core/serializers.py (1)
2887-2979: Transaction wrapping and folder cascade are correctly implemented.The two transaction blocks handle different scenarios appropriately:
- Status changes require additional logic (FlowEvent creation, lock management)
- Non-status changes only need folder cascade
Both paths maintain atomicity and properly cascade folder changes to FlowEvent records.
💡 Optional: Consider extracting the folder cascade logic to reduce duplication
The folder cascade logic appears in both transaction blocks (lines 2964-2967 and 2974-2977). While this is minor, you could extract it to a helper method:
def _cascade_folder_to_events(self, validation_flow, old_folder_id): """Cascade folder changes to related FlowEvent records""" if old_folder_id != validation_flow.folder_id: FlowEvent.objects.filter(validation_flow=validation_flow).update( folder=validation_flow.folder )Then call it in both places:
self._cascade_folder_to_events(updated_instance, old_folder_id)This is entirely optional and not blocking.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/core/serializers.pyfrontend/src/lib/components/Forms/ModelForm/EntityForm.sveltefrontend/src/lib/components/Forms/ModelForm/PerimeterForm.sveltefrontend/src/lib/components/ModelTable/ModelTable.svelte
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/lib/components/Forms/ModelForm/EntityForm.svelte
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-12-29T15:52:09.213Z
Learnt from: melinoix
Repo: intuitem/ciso-assistant-community PR: 3107
File: frontend/src/routes/(app)/(internal)/recap/+page.svelte:49-51
Timestamp: 2025-12-29T15:52:09.213Z
Learning: In the recap page (frontend/src/routes/(app)/(internal)/recap/+page.svelte), perimeter.folder is guaranteed to be present when perimeter.compliance_assessments exist, so null safety checks are not necessary for perimeter.folder.str in that context.
Applied to files:
frontend/src/lib/components/Forms/ModelForm/PerimeterForm.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-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
🧬 Code graph analysis (1)
backend/core/serializers.py (2)
backend/privacy/serializers.py (1)
update(123-140)backend/core/models.py (4)
update(1209-1221)EvidenceRevision(3817-3886)TimelineEntry(3985-4044)TaskNode(7553-7617)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: test (3.12)
- GitHub Check: build_community_frontend
- GitHub Check: build_enterprise_frontend
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: build (3.12)
- GitHub Check: Analyze (python)
🔇 Additional comments (10)
frontend/src/lib/components/Forms/ModelForm/PerimeterForm.svelte (1)
26-35: LGTM! Domain field now visible in read-only mode for existing perimeters.The change from
hiddentodisabledcorrectly implements the PR requirement that perimeter objects should have the domain field visible in read-only mode rather than hidden. When editing an existing perimeter (initialData.folderis truthy), the field is now disabled (read-only), and when creating a new perimeter, it remains enabled.This improves UX by always showing the domain context while preventing unintended modifications to existing perimeters.
frontend/src/lib/components/ModelTable/ModelTable.svelte (2)
271-271: LGTM!The updated preventDelete logic correctly prevents deletion of builtin/urn items except for terminologies, which aligns with the domain-change functionality introduced in this PR.
724-726: Confirm intentionality of edit/delete permission asymmetry for entities.Entities with
builtinorurnmetadata can be edited (lines 724-726:editURLis enabled whenURLModel === 'entities'overrides the builtin/urn check), but cannot be deleted (line 271:preventDeleteblocks deletion forbuiltin || urnwhenURLModel !== 'terminologies'). SinceURLModel === 'entities'satisfies this condition, deletion is prevented. This asymmetry appears intentional, but please confirm it aligns with the domain-change requirements in the PR objectives.backend/core/serializers.py (7)
5-5: Transaction import successfully added! ✅This addresses the critical issue from the previous review where
transaction.atomic()andtransaction.on_commit()calls would have raisedNameError.
76-88: Good defensive check for builtin objects.This validation correctly prevents builtin objects from changing folders, which maintains referential integrity for system-provided objects.
272-280: Perimeter folder-change prevention is correctly implemented.This enforces the constraint that a Perimeter's domain (folder) should not be changed after creation, which aligns with the PR objectives stating that perimeter domain should be read-only.
1587-1604: Transaction wrapping and folder cascade are correctly implemented.The atomic transaction ensures that Evidence updates and EvidenceRevision folder propagation happen together or not at all. The folder cascade logic correctly maintains consistency between Evidence and its revisions.
1870-1903: Excellent use of transactional updates with deferred notifications! ✨This implementation correctly:
- Wraps the update and folder cascade in an atomic transaction
- Uses
transaction.on_commit()to defer notifications until after the transaction successfully commits- Prevents notification spam if the transaction rolls back
The pattern of deferring side effects (like email notifications) until after transaction commit is a best practice for maintaining data consistency and avoiding misleading notifications.
2554-2562: Transaction wrapping and folder cascade are correctly implemented.The atomic transaction ensures that Incident updates and TimelineEntry folder propagation happen atomically, maintaining consistency between the incident and its timeline entries.
2670-2689: Transaction wrapping and folder cascade are correctly implemented.The atomic transaction ensures that TaskTemplate updates, TaskNode synchronization, and folder propagation to TaskNode records happen atomically. Notifications are correctly sent after the transaction commits.
| editURL={!(row.meta.builtin || row.meta.urn) || | ||
| URLModel === 'terminologies' || | ||
| URLModel === 'entities' |
There was a problem hiding this comment.
Add optional chaining to prevent potential runtime errors.
The editURL calculation accesses row.meta.builtin and row.meta.urn without optional chaining, but Line 271 uses row?.meta?.builtin. If row.meta is undefined, this could cause a runtime exception.
🔎 Proposed fix
-editURL={!(row.meta.builtin || row.meta.urn) ||
+editURL={!(row.meta?.builtin || row.meta?.urn) ||
URLModel === 'terminologies' ||
URLModel === 'entities'🤖 Prompt for AI Agents
In frontend/src/lib/components/ModelTable/ModelTable.svelte around lines 724 to
726, the expression accesses row.meta.builtin and row.meta.urn directly which
can throw if row.meta is undefined; update the expression to use optional
chaining (e.g. row?.meta?.builtin and row?.meta?.urn) so it mirrors the safe
check used elsewhere, ensuring the logical conditions and parentheses remain the
same.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for entity form definitions and domain field configurations
find frontend/src/lib/components -name "*.svelte" | xargs grep -l "entities.*edit\|entity.*form\|domain.*field" 2>/dev/null | head -20Repository: intuitem/ciso-assistant-community
Length of output: 217
🏁 Script executed:
#!/bin/bash
# Search for domain field configurations in forms, particularly for entities
rg -n "domain" frontend/src/lib/components -t svelte -B3 -A3 | grep -i "entity\|entities\|edit" | head -40Repository: intuitem/ciso-assistant-community
Length of output: 107
🏁 Script executed:
#!/bin/bash
# Find any edit form components for entities or models
fd -e svelte "edit|form" frontend/src/lib/components --type f | head -20Repository: intuitem/ciso-assistant-community
Length of output: 1450
🏁 Script executed:
#!/bin/bash
# Look for model/entity form or editor components
fd -e svelte . frontend/src/lib/components --type f | xargs grep -l "readOnly.*domain\|domain.*readOnly\|disabled.*domain\|domain.*disabled" 2>/dev/nullRepository: intuitem/ciso-assistant-community
Length of output: 61
🏁 Script executed:
#!/bin/bash
# Find EntityForm component
fd -e svelte EntityForm frontend/src/lib/components --type fRepository: intuitem/ciso-assistant-community
Length of output: 138
🏁 Script executed:
#!/bin/bash
# Search for domain field in form components - use grep instead of rg type
grep -r "domain" frontend/src/lib/components/Forms --include="*.svelte" | grep -i "entity\|readOnly\|disabled" | head -30Repository: intuitem/ciso-assistant-community
Length of output: 158
🏁 Script executed:
#!/bin/bash
# Examine EntityAssessmentForm to see if it has domain field handling
cat frontend/src/lib/components/Forms/ModelForm/EntityAssessmentForm.svelte | head -100Repository: intuitem/ciso-assistant-community
Length of output: 2881
🏁 Script executed:
#!/bin/bash
# Search for any domain field definitions in forms
grep -r "domain" frontend/src/lib/components/Forms/ModelForm --include="*.svelte" -B2 -A2 | head -60Repository: intuitem/ciso-assistant-community
Length of output: 4651
🏁 Script executed:
#!/bin/bash
# View the complete EntityForm.svelte to inspect domain field configuration
cat frontend/src/lib/components/Forms/ModelForm/EntityForm.svelteRepository: intuitem/ciso-assistant-community
Length of output: 8199
🏁 Script executed:
#!/bin/bash
# Search for domain field with context in EntityForm
grep -n "domain" frontend/src/lib/components/Forms/ModelForm/EntityForm.svelte -B5 -A5Repository: intuitem/ciso-assistant-community
Length of output: 382
Add urn check to domain field restriction in EntityForm.svelte.
The domain field (folder) in EntityForm.svelte is only disabled for builtin entities (disabled={object.builtin}), but the PR allows editing of both builtin and urn entities. To ensure the domain remains read-only for main entities as stated in the PR objectives, the domain field must also be disabled for urn entities. Update line 76 to: disabled={object.builtin || object.urn}
🤖 Prompt for AI Agents
In frontend/src/lib/components/EntityForm.svelte around line 76, the domain
(folder) field is currently only disabled for builtin entities; update the
disabled prop to also check object.urn so the field is read-only for both
builtin and urn entities (i.e., change the condition to include object.urn).
Ensure the expression uses logical OR between object.builtin and object.urn and
that tests/consumers still treat urn entities as non-editable for the domain
field.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
Localization
✏️ Tip: You can customize this high-level summary in your review settings.