feat: contracts management and additional concepts for DORA reports #2854
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughAdds a Contract model and related migrations, extends Asset with Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Frontend
participant API
participant ContractModel
participant RelatedModels
rect rgba(76,175,80,0.06)
User->>Frontend: Open Contracts list / apply filters
Frontend->>API: GET /contracts/?...
API->>ContractModel: Query contracts (with prefetch related)
ContractModel->>RelatedModels: Load entities, evidences, solutions, owner
RelatedModels-->>ContractModel: Return related data
ContractModel-->>API: Return serialized contracts
API-->>Frontend: 200 OK (contracts)
Frontend-->>User: Render contracts table
end
sequenceDiagram
participant User
participant Frontend
participant API
participant AssetModel
participant SolutionModel
rect rgba(33,150,243,0.06)
User->>Frontend: Edit Asset (is_critical, solutions)
Frontend->>API: PATCH /assets/{id}/ { is_critical, solutions }
API->>AssetModel: Update is_critical and M2M
API->>SolutionModel: Sync Asset↔Solution links
AssetModel-->>API: Return updated Asset
API-->>Frontend: 200 OK (updated Asset)
Frontend-->>User: Show updated asset
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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: 2
🧹 Nitpick comments (4)
backend/core/models.py (1)
2152-2153: Use translated verbose_name (and optional help_text); consider a filtered index if heavily queried.Prefer i18n like other fields and add concise help text. Indexing can wait unless you filter on it a lot.
Apply this change:
- is_critical = models.BooleanField("is_critical", default=False) + is_critical = models.BooleanField( + verbose_name=_("Is critical"), + default=False, + help_text=_("Marks assets that are critical to operations"), + )Optionally (PostgreSQL): add a partial index in a follow-up migration if
WHERE is_critical = TRUEis common.frontend/src/lib/components/SideBar/navData.ts (1)
284-288: Add explicit permissions to the contracts nav item for consistency.Most gated items specify permissions; suggest
['view_contract']here to align behavior.{ name: 'contracts', fa_icon: 'fa-solid fa-file-contract', - href: '/contracts' + href: '/contracts', + permissions: ['view_contract'] },frontend/src/lib/utils/schemas.ts (1)
626-629: Tighten contract relation validation to UUIDsOther schemas in this file validate relationship IDs with
z.string().uuid().optional().array().optional(). Here we only enforce “string”, so a malformed ID (or even an empty string injected via dev tools) will pass client-side checks and fail later against the API. Please align these arrays with the established UUID pattern so the form catches bad inputs early.- owner: z.array(z.string().optional()).optional(), - entities: z.array(z.string().optional()).optional(), - evidences: z.array(z.string().optional()).optional(), - solutions: z.array(z.string().optional()).optional(), + owner: z.string().uuid().optional().array().optional(), + entities: z.string().uuid().optional().array().optional(), + evidences: z.string().uuid().optional().array().optional(), + solutions: z.string().uuid().optional().array().optional(),backend/core/views.py (1)
662-679: Asset boolean filter addition looks goodAdding is_critical to AssetFilter enables straightforward boolean filtering; no RBAC impact. Optionally, if this field will be queried often, consider adding it to asset CSV exports and/or a partial index (WHERE is_critical = TRUE) in a follow-up migration for large datasets.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
backend/core/migrations/0112_asset_is_critical.py(1 hunks)backend/core/models.py(1 hunks)backend/core/serializers.py(3 hunks)backend/core/startup.py(10 hunks)backend/core/urls.py(2 hunks)backend/core/views.py(2 hunks)backend/tprm/migrations/0008_entity_legal_identifiers.py(1 hunks)backend/tprm/migrations/0009_contract.py(1 hunks)backend/tprm/models.py(4 hunks)backend/tprm/serializers.py(4 hunks)backend/tprm/views.py(3 hunks)frontend/messages/en.json(4 hunks)frontend/src/lib/components/Forms/LegalIdentifierField.svelte(1 hunks)frontend/src/lib/components/Forms/ModelForm.svelte(2 hunks)frontend/src/lib/components/Forms/ModelForm/AssetForm.svelte(2 hunks)frontend/src/lib/components/Forms/ModelForm/ContractForm.svelte(1 hunks)frontend/src/lib/components/Forms/ModelForm/EntityForm.svelte(2 hunks)frontend/src/lib/components/SideBar/navData.ts(2 hunks)frontend/src/lib/utils/crud.ts(5 hunks)frontend/src/lib/utils/schemas.ts(4 hunks)frontend/src/lib/utils/table.ts(4 hunks)frontend/src/lib/utils/types.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 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 (7)
backend/tprm/views.py (2)
backend/tprm/models.py (4)
Entity(18-63)Solution(136-170)Contract(173-238)Status(178-182)backend/core/views.py (17)
get(440-447)get(5255-5256)BaseModelViewSet(233-430)status(1234-1235)status(1503-1504)status(2439-2440)status(5846-5847)status(5967-5968)status(6001-6002)status(6114-6115)status(7444-7445)status(7941-7942)status(8110-8111)status(8570-8571)status(8708-8709)status(9480-9481)status(9491-9492)
frontend/src/lib/utils/schemas.ts (1)
backend/core/base_models.py (1)
NameDescriptionMixin(119-132)
backend/tprm/migrations/0009_contract.py (1)
backend/iam/models.py (2)
Folder(68-316)get_root_folder_id(80-85)
backend/tprm/serializers.py (2)
backend/tprm/models.py (3)
Entity(18-63)Solution(136-170)Contract(173-238)backend/core/serializer_fields.py (1)
FieldsRelatedField(26-84)
backend/core/serializers.py (2)
backend/tprm/models.py (1)
Solution(136-170)backend/core/serializer_fields.py (1)
FieldsRelatedField(26-84)
backend/core/urls.py (1)
backend/tprm/views.py (1)
ContractViewSet(162-172)
backend/tprm/models.py (2)
backend/core/models.py (6)
FilteringLabelMixin(191-197)Meta(113-114)Meta(168-169)Meta(196-197)Meta(201-203)Meta(1368-1370)backend/iam/models.py (1)
FolderMixin(319-338)
🪛 Biome (2.1.2)
frontend/messages/en.json
[error] 57-57: The key associatedEntities 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)
[error] 212-212: The key startDate 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)
[error] 213-213: The key endDate 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 (3.12)
- GitHub Check: build_enterprise_frontend
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: build_community_frontend
- GitHub Check: startup-docker-compose-test
- GitHub Check: test (3.12)
- GitHub Check: Analyze (python)
🔇 Additional comments (7)
frontend/src/lib/components/SideBar/navData.ts (1)
267-267: Now let me verify the Contract model's Meta class to ensure it uses Django's default permissions:Verified: 'view_contract' permission codename is correct.
Django automatically creates add, delete, change, and view permissions for each model when
django.contrib.authis inINSTALLED_APPS. For a model, Django creates permissions with codenames like 'add_post', 'change_post', 'delete_post', 'view_post' following the pattern<action>_<modelname_lowercase>.The Contract model exists at
backend/tprm/models.py:173, so Django will automatically generateview_contractas the codename. Your frontend addition of'view_contract'to the permissions array is correct and aligns with Django's default naming convention.frontend/src/lib/utils/types.ts (1)
68-69: All infrastructure confirmed — 'contracts' model is fully integrated.✓ Schema (contractSchema)
✓ CRUD config with foreign/reverse-foreign keys
✓ Table configuration
✓ Form component (ContractForm.svelte)
✓ Backend model (Contract class)
✓ Permissions (view_contract)
✓ Routes via dynamic [model=urlmodel] matcherfrontend/src/lib/components/Forms/ModelForm/EntityForm.svelte (1)
5-5: LGTM! Clean integration of the legal identifiers field.The
LegalIdentifierFieldcomponent is properly imported and integrated following the same patterns as other form fields. The props are correctly passed and the two-way binding withcachedValueis consistent with the form's caching mechanism.Also applies to: 61-66
backend/core/serializers.py (2)
463-463: LGTM! Consistent with other many-to-many read fields.The
solutionsfield inAssetReadSerializerfollows the established pattern for many-to-many relationships, usingFieldsRelatedField(many=True)just like other related fields in this serializer.
394-398: Solutions field is correctly handled by DRF's automatic many-to-many processing.After verification, the
solutionsfield requires no explicit handling increate()andupdate()methods. Unlikesupport_assets(which hassource="child_assets"and therefore needs explicit pop+set logic), thesolutionsfield has no source override, meaning its field name directly matches the Asset model's many-to-many attribute. Django REST Framework automatically manages such many-to-many fields during object creation and updates without requiring manual intervention.frontend/src/lib/components/Forms/ModelForm/ContractForm.svelte (1)
1-113: LGTM! Well-structured contract form following established patterns.The
ContractFormcomponent is cleanly implemented with:
- Consistent prop handling and caching mechanism
- Appropriate field types for each data type (AutocompleteSelect for relations, TextField for dates/text, Select for status)
- Proper multiple selection for relationship fields (owner, entities, solutions, evidences, filtering_labels)
- Extra fields displayed for context (provider_entity for solutions, folder for evidences)
- Consistent i18n label usage
- Standard form binding and cache management
The implementation aligns well with the backend Contract model and follows the same patterns as other form components in the codebase.
backend/core/views.py (1)
5786-5801: Thecontractsfield exists on Evidence as a valid reverse relation—no action neededThe Contract model in
backend/tprm/models.pydefines a ManyToManyField to Evidence withrelated_name="contracts", which automatically creates a reverse relation on Evidence. This allows django-filter to correctly exposecontractsin filterset_fields without runtime errors.Likely an incorrect or invalid review comment.
| "associatedRepresentatives": "Associated representatives", | ||
| "associatedSolutions": "Associated solutions", | ||
| "associatedContracts": "Associated contracts", | ||
| "associatedEntities": "Associated entities", |
There was a problem hiding this comment.
Remove duplicate translation keys
associatedEntities, startDate, and endDate are already defined later in this file. Re-declaring them here triggers Biome’s noDuplicateObjectKeys error and, because JSON parsers keep the last value only, it makes the earlier definitions meaningless. Please drop the newly added duplicates and rely on the existing entries.
"associatedContracts": "Associated contracts",
- "associatedEntities": "Associated entities",
@@
- "startDate": "Start date",
- "endDate": "End date",Also applies to: 212-213
🧰 Tools
🪛 Biome (2.1.2)
[error] 57-57: The key associatedEntities 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 line 57 (and also lines 212-213), you added
duplicate translation keys (associatedEntities, startDate, endDate) that
conflict with existing entries later in the file; remove the duplicate entries
at these locations so only the original definitions remain, ensuring the JSON
object has unique keys and passes Biome's noDuplicateObjectKeys check and JSON
validation.
| function updateIdentifierType(oldType: string, newType: string) { | ||
| if (oldType === newType) return; | ||
| const copy = { ...legalIdentifiers }; | ||
| const value = copy[oldType] || ''; | ||
| // Remove old key | ||
| delete copy[oldType]; | ||
| // Add with new key if it doesn't already exist | ||
| if (!copy[newType]) { | ||
| copy[newType] = value; | ||
| } | ||
| legalIdentifiers = copy; | ||
| } |
There was a problem hiding this comment.
Prevent data loss when switching identifier types
If a user switches an identifier to a type that is already present, the old entry is deleted and never re-added, so the identifier value disappears silently. Please guard against clashes before deleting the old key, or surface a validation message, so data is not lost.
function updateIdentifierType(oldType: string, newType: string) {
- if (oldType === newType) return;
-
- const copy = { ...legalIdentifiers };
- const value = copy[oldType] || '';
-
- // Remove old key
- delete copy[oldType];
-
- // Add with new key if it doesn't already exist
- if (!copy[newType]) {
- copy[newType] = value;
- }
-
- legalIdentifiers = copy;
+ if (oldType === newType) return;
+
+ if (legalIdentifiers[newType] !== undefined) {
+ return;
+ }
+
+ const copy = { ...legalIdentifiers };
+ const value = copy[oldType] ?? '';
+
+ delete copy[oldType];
+ copy[newType] = value;
+
+ legalIdentifiers = copy;
}📝 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.
| function updateIdentifierType(oldType: string, newType: string) { | |
| if (oldType === newType) return; | |
| const copy = { ...legalIdentifiers }; | |
| const value = copy[oldType] || ''; | |
| // Remove old key | |
| delete copy[oldType]; | |
| // Add with new key if it doesn't already exist | |
| if (!copy[newType]) { | |
| copy[newType] = value; | |
| } | |
| legalIdentifiers = copy; | |
| } | |
| function updateIdentifierType(oldType: string, newType: string) { | |
| if (oldType === newType) return; | |
| if (legalIdentifiers[newType] !== undefined) { | |
| return; | |
| } | |
| const copy = { ...legalIdentifiers }; | |
| const value = copy[oldType] ?? ''; | |
| delete copy[oldType]; | |
| copy[newType] = value; | |
| legalIdentifiers = copy; | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/src/lib/components/Forms/ModelForm/EntityForm.svelte (1)
54-64: Guard against nullableobjectvalues.If a parent ever passes
object = null, accessingobject.builtinwill throw before the condition runs. Optional chaining keeps the guard null-safe without changing behavior.(developer.mozilla.org)-{#if !object.builtin} +{#if !object?.builtin}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frontend/src/lib/components/DetailView/DetailView.svelte(1 hunks)frontend/src/lib/components/Forms/ModelForm.svelte(3 hunks)frontend/src/lib/components/Forms/ModelForm/EntityForm.svelte(3 hunks)frontend/src/lib/utils/table.ts(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: build (3.12)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: build_enterprise_frontend
- GitHub Check: build_community_frontend
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: startup-docker-compose-test
- GitHub Check: test (3.12)
- GitHub Check: Analyze (python)
🔇 Additional comments (5)
frontend/src/lib/components/DetailView/DetailView.svelte (1)
266-267: Now I need to search the codebase for backend permission handling on these models:Ensure backend permission checks are enforced for terminologies and entities models.
The frontend change bypasses UI-level permission validation (
canEditObject) for these models, allowing the edit button to display regardless of user authorization. While frontend UI control is cosmetic, permission checks must be enforced at the backend view level—the API endpoints must validate that users have proper authorization before allowing PUT/PATCH/DELETE operations.Verify that the backend views for
terminologiesandentitiesmodels properly enforce permission validation and do not implicitly grant edit access. DRF generic views should explicitly callcheck_object_permissions(request, obj)to enforce object-level permissions; confirm this is implemented in your backend views for these models.frontend/src/lib/utils/table.ts (4)
837-844: LGTM! Well-structured filter definition.The
ASSET_IS_CRITICAL_FILTERfollows the established pattern for boolean filters and correctly usesYES_NO_OPTIONS.
1310-1311: LGTM! Proper filter integration.The
is_criticalfilter is correctly added to the assets configuration alongside the existingasset_classfilter.
1511-1518: Backend model and serializer verified—all required fields are present.The Contract model (
backend/tprm/models.py:173) correctly exposes all fields expected by the frontend table:
ref_id,name,description(inherited/defined in model)status(TextField with Status choices enum)start_date,end_date(DateField instances)entities(ManyToManyField to Entity)The
ContractReadSerializerproperly exposes these fields viaexclude = [], ensuring no serialization issues. Field naming and types align with frontend expectations.
1019-1029: Backend endpoint verified and properly implemented.The
contracts/statusendpoint exists as a DRF action on theContractViewSet(backend/tprm/views.py:170-172) and correctly returns status choices via the model'sStatus.choices. The frontend configuration will work as expected.
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (1)
frontend/messages/en.json (1)
57-57: Remove duplicate translation keys (previously flagged)These duplicate keys were already identified in a previous review but remain unfixed.
associatedEntities(line 57),startDate(line 212), andendDate(line 213) are declared again later in the file (lines 936, 927, 1686 respectively). JSON parsers keep only the last value, making these earlier definitions meaningless and triggering linter errors.Apply this diff to remove the duplicates:
"associatedContracts": "Associated contracts", - "associatedEntities": "Associated entities", "associatedTaskTemplates": "Associated tasks","dueDate": "Due date", - "startDate": "Start date", - "endDate": "End date", "attachment": "Attachment",Also applies to: 212-213
🧹 Nitpick comments (5)
frontend/src/routes/(app)/(internal)/reports/dora-roi/+server.ts (1)
19-24: Consider streaming for large files.The current implementation loads the entire ZIP file into memory with
await res.blob(). For large reports, this could cause memory pressure.If DORA ROI reports can be large, consider streaming the response:
- return new Response(await res.blob(), { + return new Response(res.body, { headers: { 'Content-Type': 'application/zip', 'Content-Disposition': `attachment; filename="${fileName}"` } });This passes the stream directly without buffering the entire file.
backend/tprm/views.py (2)
112-112: Use timezone-aware datetime for consistency.Line 112 uses
datetime.now()which is timezone-naive. Django typically uses timezone-aware datetimes whenUSE_TZ = True.Apply this diff:
+from django.utils import timezone + # Prepare response zip_buffer.seek(0) - timestamp = datetime.now().strftime("%Y%m%d_%H%M%S") + timestamp = timezone.now().strftime("%Y%m%d_%H%M%S") filename = f"DORA_ROI_{timestamp}.zip"
74-100: Consider memory efficiency for large datasets.The current implementation loads all entities into memory at once. For organizations with thousands of entities, this could consume significant memory and slow down the response.
Consider implementing pagination or batch processing if you expect large numbers of entities:
- Use
.iterator(chunk_size=500)for the queryset to fetch entities in batches- Or add a query parameter to limit results (e.g., by folder or relationship type)
frontend/src/routes/(app)/(internal)/reports/ReportTile.svelte (1)
40-91: Refactor to eliminate code duplicationThe anchor and button branches contain nearly identical markup (lines 45-63 vs 71-89). Consider extracting the shared content into a separate component or using a wrapper component pattern to avoid duplication.
Example refactor:
<script lang="ts"> // ... existing props ... const Tag = href ? 'a' : 'button'; const tagProps = href ? { href } : { type: 'button', onclick }; </script> <svelte:element this={Tag} {...tagProps} class="{baseClasses} {getCategoryColor(category)}" > <div class="flex items-start gap-4"> <div class="flex-shrink-0"> <div class="w-12 h-12 rounded-lg bg-white flex items-center justify-center shadow-sm"> <i class="{icon} text-2xl {getCategoryIconColor(category)}"></i> </div> </div> <div class="flex-1 min-w-0"> <h3 class="text-lg font-semibold text-gray-900 mb-2"> {title} </h3> <p class="text-sm text-gray-600 leading-relaxed"> {description} </p> <div class="mt-4 flex items-center text-sm font-medium {getCategoryIconColor(category)}"> {m.generateReport ? m.generateReport() : 'Generate Report'} <i class="fas fa-arrow-right ml-2 text-xs"></i> </div> </div> </div> </svelte:element>frontend/src/lib/components/SideBar/navData.ts (1)
295-299: Consider adding permissions to gate access to contracts.While other items in
thirdPartyCategorylack explicit permissions, thetprmOverviewitem (line 278) requiresview_contractpermission. For consistency and proper access control, consider addingpermissions: ['view_contract']to this navigation item to prevent users without the appropriate permission from accessing the contracts route.Apply this diff to add the permission:
{ name: 'contracts', fa_icon: 'fa-solid fa-file-contract', - href: '/contracts' + href: '/contracts', + permissions: ['view_contract'] },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
backend/tprm/views.py(3 hunks)frontend/messages/en.json(5 hunks)frontend/src/lib/components/SideBar/navData.ts(3 hunks)frontend/src/routes/(app)/(internal)/reports/+page.server.ts(1 hunks)frontend/src/routes/(app)/(internal)/reports/+page.svelte(1 hunks)frontend/src/routes/(app)/(internal)/reports/ReportTile.svelte(1 hunks)frontend/src/routes/(app)/(internal)/reports/dora-roi/+server.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
frontend/src/routes/(app)/(internal)/reports/dora-roi/+server.ts (1)
frontend/src/lib/utils/constants.ts (1)
BASE_API_URL(4-8)
backend/tprm/views.py (3)
backend/tprm/models.py (4)
Entity(18-63)Solution(136-170)Contract(173-238)Status(178-182)backend/iam/models.py (4)
RoleAssignment(807-1094)get_accessible_object_ids(917-1015)Folder(68-316)get_root_folder(75-77)backend/core/views.py (17)
get(440-447)get(5255-5256)BaseModelViewSet(233-430)status(1234-1235)status(1503-1504)status(2439-2440)status(5846-5847)status(5967-5968)status(6001-6002)status(6114-6115)status(7444-7445)status(7941-7942)status(8110-8111)status(8570-8571)status(8708-8709)status(9480-9481)status(9491-9492)
🪛 Biome (2.1.2)
frontend/messages/en.json
[error] 57-57: The key associatedEntities 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)
[error] 212-212: The key startDate 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)
[error] 213-213: The key endDate 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)
🪛 GitHub Actions: Frontend Linters
frontend/src/routes/(app)/(internal)/reports/ReportTile.svelte
[warning] 1-1: Code style issues found. Run 'pnpm exec prettier --write .' to fix.
frontend/src/routes/(app)/(internal)/reports/+page.svelte
[warning] 1-1: Code style issues found. Run 'pnpm exec prettier --write .' to fix.
⏰ 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: 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: build (3.12)
- GitHub Check: Analyze (python)
🔇 Additional comments (8)
frontend/src/routes/(app)/(internal)/reports/dora-roi/+server.ts (1)
1-3: LGTM!The imports are appropriate and minimal.
backend/tprm/views.py (4)
4-4: LGTM!The imports are clean and appropriate for the CSV/ZIP generation functionality.
Also applies to: 12-17
33-39: LGTM!The addition of "contracts" to filterset_fields is consistent with the Entity-Contract relationship.
238-238: LGTM!Adding "contracts" to the filterset enables filtering solutions by their associated contracts, which aligns with the Solution-Contract relationship.
247-257: LGTM!The ContractViewSet implementation follows the established patterns in the codebase and includes appropriate filterset fields and status choices endpoint.
frontend/src/routes/(app)/(internal)/reports/+page.server.ts (1)
1-9: LGTM!The server load function is correctly implemented with proper typing and localization. It cleanly provides the user context and localized title to the reports page.
frontend/src/lib/components/SideBar/navData.ts (2)
278-278: LGTM!Adding
view_contractpermission to the TPRM overview is appropriate and aligns with the PR's contract management feature.
29-39: Reports permissions are correct as configured.Based on code review, the reports page only accesses entity data (e.g.,
/entities/generate_dora_roi/) and does not fetch assessment data. The analytics page, which requiresview_assessment, accesses compliance-assessments and risk-assessments endpoints. The permission sets appropriately reflect each page's actual data access patterns. No changes needed.
| @action(detail=False, methods=["get"], name="Generate DORA ROI") | ||
| def generate_dora_roi(self, request): | ||
| """ | ||
| Generate DORA Register of Information (ROI) as a zip file containing CSV data. | ||
| """ | ||
| # Get viewable entities for the current user | ||
| (viewable_items, _, _) = RoleAssignment.get_accessible_object_ids( | ||
| folder=Folder.get_root_folder(), | ||
| user=request.user, | ||
| object_type=Entity, | ||
| ) | ||
|
|
||
| # Create CSV in memory | ||
| csv_buffer = io.StringIO() | ||
| csv_writer = csv.writer(csv_buffer) | ||
|
|
||
| # Write CSV headers | ||
| csv_writer.writerow( | ||
| [ | ||
| "Entity ID", | ||
| "Name", | ||
| "Description", | ||
| "Mission", | ||
| "Reference Link", | ||
| "Relationship Types", | ||
| "Legal Identifiers", | ||
| "Folder", | ||
| "Created At", | ||
| "Updated At", | ||
| ] | ||
| ) | ||
|
|
||
| # Write entity data | ||
| for entity in Entity.objects.filter(id__in=viewable_items).select_related( | ||
| "folder" | ||
| ): | ||
| # Get relationship types as comma-separated string | ||
| relationships = ", ".join([str(rel) for rel in entity.relationship.all()]) | ||
|
|
||
| # Format legal identifiers as string | ||
| legal_ids = ( | ||
| ", ".join([f"{k}: {v}" for k, v in entity.legal_identifiers.items()]) | ||
| if entity.legal_identifiers | ||
| else "" | ||
| ) | ||
|
|
||
| csv_writer.writerow( | ||
| [ | ||
| str(entity.id), | ||
| entity.name, | ||
| entity.description or "", | ||
| entity.mission or "", | ||
| entity.reference_link or "", | ||
| relationships, | ||
| legal_ids, | ||
| entity.folder.name if entity.folder else "", | ||
| entity.created_at.strftime("%Y-%m-%d %H:%M:%S"), | ||
| entity.updated_at.strftime("%Y-%m-%d %H:%M:%S"), | ||
| ] | ||
| ) | ||
|
|
||
| # Create zip file in memory | ||
| zip_buffer = io.BytesIO() | ||
| with zipfile.ZipFile(zip_buffer, "w", zipfile.ZIP_DEFLATED) as zip_file: | ||
| # Add CSV to zip | ||
| zip_file.writestr( | ||
| "entities_register.csv", csv_buffer.getvalue().encode("utf-8") | ||
| ) | ||
|
|
||
| # Prepare response | ||
| zip_buffer.seek(0) | ||
| timestamp = datetime.now().strftime("%Y%m%d_%H%M%S") | ||
| filename = f"DORA_ROI_{timestamp}.zip" | ||
|
|
||
| response = HttpResponse(zip_buffer.getvalue(), content_type="application/zip") | ||
| response["Content-Disposition"] = f'attachment; filename="{filename}"' | ||
|
|
||
| return response |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add error handling for CSV/ZIP generation.
The CSV and ZIP file generation lacks error handling. If any step fails (e.g., invalid data, I/O errors), the user will see a generic 500 error instead of a meaningful message.
Consider wrapping the generation logic in a try-except block:
@action(detail=False, methods=["get"], name="Generate DORA ROI")
def generate_dora_roi(self, request):
"""
Generate DORA Register of Information (ROI) as a zip file containing CSV data.
"""
try:
# Get viewable entities for the current user
(viewable_items, _, _) = RoleAssignment.get_accessible_object_ids(
folder=Folder.get_root_folder(),
user=request.user,
object_type=Entity,
)
# ... rest of the implementation ...
return response
except Exception as e:
logger.error("Failed to generate DORA ROI", error=str(e), user=request.user.id)
return Response(
{"error": "Failed to generate DORA ROI report"},
status=500
)🤖 Prompt for AI Agents
backend/tprm/views.py around lines 41 to 118: the CSV/ZIP generation is
unprotected and will surface a generic 500 on any failure; wrap the entire
generation and response creation in a try/except block that catches Exception,
logs the error with context (include exception text and request.user.id), and
returns a DRF Response with a clear error message and HTTP 500; ensure any
opened buffers are properly closed (use existing context managers or a finally
to close csv_buffer and zip_buffer) and add necessary imports for logging and
Response if not present.
backend/tprm/views.py
Outdated
| for entity in Entity.objects.filter(id__in=viewable_items).select_related( | ||
| "folder" | ||
| ): | ||
| # Get relationship types as comma-separated string | ||
| relationships = ", ".join([str(rel) for rel in entity.relationship.all()]) |
There was a problem hiding this comment.
N+1 query: prefetch relationships to avoid multiple database hits.
Line 78 calls entity.relationship.all() inside the loop, triggering one query per entity. Add prefetch_related("relationship") to the query on line 74.
Apply this diff:
# Write entity data
- for entity in Entity.objects.filter(id__in=viewable_items).select_related(
- "folder"
- ):
+ for entity in Entity.objects.filter(id__in=viewable_items).select_related(
+ "folder"
+ ).prefetch_related("relationship"):
# Get relationship types as comma-separated string
relationships = ", ".join([str(rel) for rel in entity.relationship.all()])📝 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.
| for entity in Entity.objects.filter(id__in=viewable_items).select_related( | |
| "folder" | |
| ): | |
| # Get relationship types as comma-separated string | |
| relationships = ", ".join([str(rel) for rel in entity.relationship.all()]) | |
| for entity in Entity.objects.filter(id__in=viewable_items).select_related( | |
| "folder" | |
| ).prefetch_related("relationship"): | |
| # Get relationship types as comma-separated string | |
| relationships = ", ".join([str(rel) for rel in entity.relationship.all()]) |
🤖 Prompt for AI Agents
In backend/tprm/views.py around lines 74 to 78, the loop calls
entity.relationship.all() which triggers an N+1 query; update the queryset to
prefetch the relationship relation (e.g., chain
.prefetch_related("relationship") alongside the existing
.select_related("folder")) so relationships for all entities are fetched in a
single query and the loop no longer issues per-entity database hits.
| title: m.complianceSummaryReport ? m.complianceSummaryReport() : 'Compliance Summary Report', | ||
| description: m.complianceSummaryReportDesc ? m.complianceSummaryReportDesc() : 'Generate comprehensive compliance status across all frameworks', | ||
| icon: 'fa-solid fa-clipboard-check', | ||
| category: 'compliance' | ||
| }, | ||
| { | ||
| id: 'incident-analysis', | ||
| title: m.incidentAnalysisReport ? m.incidentAnalysisReport() : 'Incident Analysis Report', | ||
| description: m.incidentAnalysisReportDesc ? m.incidentAnalysisReportDesc() : 'Comprehensive analysis of security incidents and trends', | ||
| icon: 'fa-solid fa-bug', | ||
| category: 'operations' | ||
| }, | ||
| { | ||
| id: 'asset-inventory', | ||
| title: m.assetInventoryReport ? m.assetInventoryReport() : 'Asset Inventory Report', | ||
| description: m.assetInventoryReportDesc ? m.assetInventoryReportDesc() : 'Complete inventory of assets and their risk profiles', | ||
| icon: 'fa-solid fa-gem', | ||
| category: 'assets' | ||
| } |
There was a problem hiding this comment.
Add missing translation keys to complete i18n
The code uses several translation functions (m.complianceSummaryReport, m.complianceSummaryReportDesc, m.incidentAnalysisReport, m.incidentAnalysisReportDesc, m.assetInventoryReport, m.assetInventoryReportDesc) with fallback strings. These keys should be added to frontend/messages/en.json to complete the internationalization implementation.
Add these keys to frontend/messages/en.json:
"complianceSummaryReport": "Compliance Summary Report",
"complianceSummaryReportDesc": "Generate comprehensive compliance status across all frameworks",
"incidentAnalysisReport": "Incident Analysis Report",
"incidentAnalysisReportDesc": "Comprehensive analysis of security incidents and trends",
"assetInventoryReport": "Asset Inventory Report",
"assetInventoryReportDesc": "Complete inventory of assets and their risk profiles"🤖 Prompt for AI Agents
In frontend/src/routes/(app)/(internal)/reports/+page.svelte around lines 34 to
52, several translation keys used (complianceSummaryReport,
complianceSummaryReportDesc, incidentAnalysisReport, incidentAnalysisReportDesc,
assetInventoryReport, assetInventoryReportDesc) are missing from the English
messages file; add these six keys with the provided English strings to
frontend/messages/en.json so the m.* lookups resolve without falling back to
hardcoded defaults and ensure JSON is valid (comma placement, quoting).
| if (tile.onClick) { | ||
| tile.onClick(); | ||
| } else { | ||
| // Default action - will be implemented with backend |
There was a problem hiding this comment.
Add missing translation keys for UI text
The m.generateReport, m.aboutReports, and m.aboutReportsDescription functions are used but their keys are missing from frontend/messages/en.json. Add these to complete the i18n coverage.
Add these keys to frontend/messages/en.json:
"generateReport": "Generate Report",
"aboutReports": "About Reports",
"aboutReportsDescription": "Reports provide simple tools to generate specialized reports useful for key insights or required by authorities for specific standards. More specialized capabilities will be added as we identify specific cases."Also applies to: 93-96
🤖 Prompt for AI Agents
In frontend/src/routes/(app)/(internal)/reports/+page.svelte around lines 59 and
again 93-96, the view references translation keys m.generateReport,
m.aboutReports, and m.aboutReportsDescription that are missing from
frontend/messages/en.json; add these keys to that file with the following
values: "generateReport": "Generate Report", "aboutReports": "About Reports",
and "aboutReportsDescription": "Reports provide simple tools to generate
specialized reports useful for key insights or required by authorities for
specific standards. More specialized capabilities will be added as we identify
specific cases."
| const res = await fetch(endpoint); | ||
| if (!res.ok) { | ||
| error(400, 'Error generating DORA ROI file'); | ||
| } |
There was a problem hiding this comment.
Preserve backend error status and details.
The current error handling always returns a 400 status regardless of the actual backend error. This loses important context about whether the issue is a client error (4xx) or server error (5xx), and doesn't include any error details from the backend response.
Apply this diff to preserve the backend status and error details:
const res = await fetch(endpoint);
if (!res.ok) {
- error(400, 'Error generating DORA ROI file');
+ const errorMessage = await res.text().catch(() => 'Error generating DORA ROI file');
+ error(res.status, errorMessage);
}📝 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 res = await fetch(endpoint); | |
| if (!res.ok) { | |
| error(400, 'Error generating DORA ROI file'); | |
| } | |
| const res = await fetch(endpoint); | |
| if (!res.ok) { | |
| const errorMessage = await res.text().catch(() => 'Error generating DORA ROI file'); | |
| error(res.status, errorMessage); | |
| } |
🤖 Prompt for AI Agents
In frontend/src/routes/(app)/(internal)/reports/dora-roi/+server.ts around lines
8–11, the code unconditionally throws a 400 error on any non-OK backend
response, losing the original HTTP status and backend error details; change the
branch to preserve and rethrow the backend status and message by reading the
response body (try JSON then fallback to text), extract a useful error message
from that body (e.g., message or error fields or raw text), and call
error(res.status, extractedMessage) so the original status code and backend
error details are propagated.
| // Extract filename from Content-Disposition header if available | ||
| const contentDisposition = res.headers.get('Content-Disposition'); | ||
| const fileName = contentDisposition | ||
| ? contentDisposition.split('filename=')[1]?.replace(/"/g, '') | ||
| : `DORA_ROI_${new Date().toISOString().split('T')[0]}.zip`; |
There was a problem hiding this comment.
Improve Content-Disposition parsing and sanitize filename.
The current filename extraction has two concerns:
-
Fragile parsing: The
split('filename=')[1]approach doesn't handle all valid Content-Disposition formats (e.g.,filename*, single quotes, or parameters before filename). -
Security: The filename from the backend response should be sanitized to prevent potential path traversal attacks, even though browsers typically handle
attachmentsafely.
Consider this more robust approach:
// Extract filename from Content-Disposition header if available
const contentDisposition = res.headers.get('Content-Disposition');
- const fileName = contentDisposition
- ? contentDisposition.split('filename=')[1]?.replace(/"/g, '')
- : `DORA_ROI_${new Date().toISOString().split('T')[0]}.zip`;
+ let fileName = `DORA_ROI_${new Date().toISOString().split('T')[0]}.zip`;
+ if (contentDisposition) {
+ const match = contentDisposition.match(/filename[^;=\n]*=["]?([^";\n]*)["]?/);
+ if (match?.[1]) {
+ // Sanitize filename to prevent path traversal
+ fileName = match[1].replace(/[/\\]/g, '_').trim();
+ }
+ }📝 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.
| // Extract filename from Content-Disposition header if available | |
| const contentDisposition = res.headers.get('Content-Disposition'); | |
| const fileName = contentDisposition | |
| ? contentDisposition.split('filename=')[1]?.replace(/"/g, '') | |
| : `DORA_ROI_${new Date().toISOString().split('T')[0]}.zip`; | |
| // Extract filename from Content-Disposition header if available | |
| const contentDisposition = res.headers.get('Content-Disposition'); | |
| let fileName = `DORA_ROI_${new Date().toISOString().split('T')[0]}.zip`; | |
| if (contentDisposition) { | |
| const match = contentDisposition.match(/filename[^;=\n]*=["]?([^";\n]*)["]?/); | |
| if (match?.[1]) { | |
| // Sanitize filename to prevent path traversal | |
| fileName = match[1].replace(/[/\\]/g, '_').trim(); | |
| } | |
| } |
🤖 Prompt for AI Agents
In frontend/src/routes/(app)/(internal)/reports/dora-roi/+server.ts around lines
13 to 17, the current Content-Disposition parsing is fragile and the extracted
filename isn't sanitized; update the code to robustly parse Content-Disposition
by first checking for a filename* parameter (RFC 5987) and decoding
percent-encoding, then falling back to filename with support for double or
single quotes and parameters in any order using a regex that captures either
form; after extraction, sanitize the resulting filename by stripping any
directory components or path separators, removing/control-normalizing dangerous
characters (e.g., .., slashes, nulls, control chars), limiting length to a safe
maximum, and falling back to the existing DORA_ROI_YYYY-MM-DD.zip default if
parsing yields nothing safe.
| {description} | ||
| </p> | ||
| <div class="mt-4 flex items-center text-sm font-medium {getCategoryIconColor(category)}"> | ||
| {m.generateReport ? m.generateReport() : 'Generate Report'} |
There was a problem hiding this comment.
Add missing translation key
The m.generateReport function is used but the key is missing from frontend/messages/en.json. This is also needed by the parent +page.svelte file.
Add this key to frontend/messages/en.json:
"generateReport": "Generate Report"Also applies to: 85-85
🤖 Prompt for AI Agents
In frontend/src/routes/(app)/(internal)/reports/ReportTile.svelte around lines
59 and 85, the template calls m.generateReport() but the translation key is
missing from frontend/messages/en.json; add the key "generateReport": "Generate
Report" to frontend/messages/en.json (ensuring valid JSON comma placement) so
both this component and the parent +page.svelte can resolve the translation.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (6)
frontend/src/routes/(app)/(internal)/reports/ReportTile.svelte (1)
59-62: Missing i18n key generateReport (already flagged)
m.generateReport()is used but the key is absent infrontend/messages/en.json. Add"generateReport": "Generate Report". See prior bot comment.Also applies to: 96-99
backend/tprm/views.py (2)
93-106: N+1 on entity.relationship (already flagged)Prefetch
relationshipon the queryset to avoid per-entity queries in the ROI export loop.- for entity in Entity.objects.filter(id__in=viewable_items).select_related( - "folder" - ): + for entity in ( + Entity.objects.filter(id__in=viewable_items) + .select_related("folder") + .prefetch_related("relationship") + ):
55-151: Wrap ROI generation in error handling (already flagged)Add a try/except, log the error, and return a clear 500 response to avoid exposing generic errors to users.
frontend/messages/en.json (3)
2626-2640: Add missing translation for ReportTile CTAAdd the
generateReportkey used by ReportTile. Ensure valid comma placement."impactRequirements": "All values must be provided and greater than 0. Values must increase from top to bottom (e.g., Minor: 5,000 → Major: 50,000 → Critical: 500,000).", + "generateReport": "Generate Report", "reports": "Reports",
56-58: Remove duplicate key
associatedEntitiesis already defined later; remove this earlier duplicate to satisfy Biome’snoDuplicateObjectKeys."associatedSolutions": "Associated solutions", - "associatedContracts": "Associated contracts", - "associatedEntities": "Associated entities", + "associatedContracts": "Associated contracts",
212-213: Remove duplicate keys
startDateandendDateare already defined later; drop these duplicates here.- "startDate": "Start date", - "endDate": "End date",
🧹 Nitpick comments (5)
backend/tprm/views.py (2)
1-15: Clean up imports
Responseis imported twice anddate_formatis unused. Remove the duplicate and unused import.-from rest_framework.response import Response +from rest_framework.response import Response @@ -from rest_framework.response import Response - -from django.utils.formats import date_format +from rest_framework.response import Response
208-260: Reduce N+1 queries in metricsPrefetch
solutionsandreviewers, andselect_related("entity", "compliance_assessment")to avoid repeated queries inside the loop.- for ea in EntityAssessment.objects.filter(id__in=viewable_items): + qs = ( + EntityAssessment.objects.filter(id__in=viewable_items) + .select_related("entity", "compliance_assessment") + .prefetch_related("solutions", "reviewers") + ) + for ea in qs:backend/core/constants.py (1)
255-422: Consider documenting legacy currencies.The CURRENCY_CHOICES list includes both current and legacy currencies (e.g., BYR "Belarussian Ruble (2000 Series)" at line 280, LTL "Lithuanian Litas" at line 343, LVL "Latvian Lats" at line 344, ZMK at line 416). While this may be intentional for historical data support, consider adding a comment at the top of the list documenting which currencies are legacy/deprecated to help future maintainers.
backend/tprm/models.py (2)
51-96: Consider DecimalField for financial data.Line 85-90: The
dora_assets_valueusesFloatFieldfor financial data. Consider usingDecimalFieldinstead to avoid floating-point precision issues with monetary values. This is especially important for DORA compliance reporting where accuracy is critical.Apply this diff:
- dora_assets_value = models.FloatField( + dora_assets_value = models.DecimalField( + max_digits=20, + decimal_places=2, blank=True, null=True, verbose_name=_("DORA assets value"), help_text=_("Total assets value for DORA reporting"), )Otherwise, the Entity model enhancements look well-structured with appropriate field types, proper help text, and good use of choices constraints.
302-307: Consider DecimalField for financial data.Line 302-307: The
annual_expensefield usesFloatFieldfor monetary values. For consistency with financial reporting standards and to avoid floating-point precision issues, consider usingDecimalFieldinstead:- annual_expense = models.FloatField( + annual_expense = models.DecimalField( + max_digits=15, + decimal_places=2, blank=True, null=True, verbose_name=_("Annual expense"), help_text=_("Annual expense amount for this contract"), )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
backend/core/constants.py(5 hunks)backend/core/dora.py(1 hunks)backend/privacy/migrations/0010_alter_datacontractor_country_and_more.py(1 hunks)backend/tprm/migrations/0010_contract_annual_expense_contract_currency_and_more.py(1 hunks)backend/tprm/models.py(5 hunks)backend/tprm/serializers.py(5 hunks)backend/tprm/views.py(3 hunks)frontend/messages/en.json(5 hunks)frontend/src/lib/components/Forms/ModelForm/ContractForm.svelte(1 hunks)frontend/src/lib/components/Forms/ModelForm/EntityForm.svelte(3 hunks)frontend/src/lib/components/Forms/ModelForm/SolutionForm.svelte(2 hunks)frontend/src/lib/utils/crud.ts(6 hunks)frontend/src/lib/utils/schemas.ts(5 hunks)frontend/src/routes/(app)/(internal)/reports/+page.svelte(1 hunks)frontend/src/routes/(app)/(internal)/reports/ReportTile.svelte(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- backend/tprm/migrations/0010_contract_annual_expense_contract_currency_and_more.py
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/src/routes/(app)/(internal)/reports/+page.svelte
- frontend/src/lib/components/Forms/ModelForm/ContractForm.svelte
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-18T10:42:17.597Z
Learnt from: ab-smith
Repo: intuitem/ciso-assistant-community PR: 2548
File: frontend/src/lib/components/ModelTable/LibraryOverview.svelte:1-22
Timestamp: 2025-09-18T10:42:17.597Z
Learning: The frontend of the ciso-assistant-community project is based on Svelte 5, so Svelte 5 runes like $props(), $derived, $state, etc. are expected and should not be flagged as issues.
Applied to files:
frontend/src/routes/(app)/(internal)/reports/ReportTile.svelte
📚 Learning: 2025-08-12T12:36:17.078Z
Learnt from: nas-tabchiche
Repo: intuitem/ciso-assistant-community PR: 2397
File: backend/privacy/migrations/0005_alter_datacontractor_country_and_more.py:12-214
Timestamp: 2025-08-12T12:36:17.078Z
Learning: In Django migrations, country choices should be hardcoded as static lists rather than referencing dynamic constants like COUNTRY_CHOICES from backend.core.constants, to ensure migrations remain stable over time and don't break if the referenced constants change.
Applied to files:
backend/core/constants.pybackend/privacy/migrations/0010_alter_datacontractor_country_and_more.py
🧬 Code graph analysis (5)
backend/tprm/serializers.py (2)
backend/tprm/models.py (7)
Entity(26-111)Solution(184-225)Contract(228-341)Meta(100-102)Meta(156-158)Meta(223-225)Meta(339-341)backend/core/serializer_fields.py (1)
FieldsRelatedField(26-84)
backend/privacy/migrations/0010_alter_datacontractor_country_and_more.py (2)
backend/tprm/migrations/0010_contract_annual_expense_contract_currency_and_more.py (1)
Migration(7-1091)backend/privacy/migrations/0005_alter_datacontractor_country_and_more.py (1)
Migration(6-418)
frontend/src/lib/utils/schemas.ts (1)
backend/core/base_models.py (1)
NameDescriptionMixin(119-132)
backend/tprm/models.py (3)
backend/core/models.py (16)
FilteringLabelMixin(191-197)Status(1896-1902)Status(3264-3270)Status(3403-3408)Status(3572-3578)Status(4074-4079)Status(4160-4167)Status(4236-4241)Status(5054-5059)Status(5925-5929)Status(6309-6319)Meta(113-114)Meta(168-169)Meta(196-197)Meta(201-203)Meta(1368-1370)backend/tprm/views.py (11)
country(153-154)currency(157-158)currency(331-332)dora_entity_type(161-162)dora_entity_hierarchy(165-166)dora_ict_service_type(302-303)status(201-202)status(327-328)dora_contractual_arrangement(335-336)termination_reason(339-340)governing_law_country(343-344)backend/tprm/serializers.py (11)
Meta(36-38)Meta(42-44)Meta(51-68)Meta(82-84)Meta(209-211)Meta(218-220)Meta(277-279)Meta(288-290)Meta(294-296)Meta(307-309)Meta(313-315)
backend/tprm/views.py (2)
backend/tprm/models.py (6)
Entity(26-111)Representative(161-181)Solution(184-225)EntityAssessment(114-158)Contract(228-341)Status(233-237)backend/iam/models.py (4)
RoleAssignment(807-1094)get_accessible_object_ids(917-1015)Folder(68-316)get_root_folder(75-77)
🪛 Biome (2.1.2)
frontend/messages/en.json
[error] 57-57: The key associatedEntities 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)
[error] 212-212: The key startDate 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)
[error] 213-213: The key endDate 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: startup-functional-test (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: build (3.12)
- GitHub Check: build_enterprise_frontend
- GitHub Check: build_community_frontend
- GitHub Check: test (3.12)
- GitHub Check: Analyze (python)
🔇 Additional comments (18)
backend/privacy/migrations/0010_alter_datacontractor_country_and_more.py (2)
12-271: Static choices in migration: goodHardcoding the country choices inside the migration avoids drift and runtime import issues. This aligns with prior guidance. Based on learnings.
12-531: No changes needed—blank/null parity is preserved.Both
datacontractor.countryanddatatransfer.countryremain as requiredCharFieldfields in migration 0010, matching their state in migration 0005. Neither migration specifiesblankornullparameters, meaning both adhere to Django's defaults (blank=False,null=False), maintaining consistent field semantics across the alteration.frontend/src/lib/utils/schemas.ts (1)
307-310: Schemas align with backend additions
AssetSchema.is_critical,AssetSchema.solutions, entity DORA fields, solution DORA ICT, andcontractSchemalook consistent with models and view filters.Also applies to: 573-581, 629-648, 1294-1295
frontend/src/lib/components/Forms/ModelForm/SolutionForm.svelte (1)
71-85: LGTM — DORA ICT service type fieldDropdown + AutocompleteSelect wiring matches backend choices endpoint and i18n keys.
frontend/src/routes/(app)/(internal)/reports/ReportTile.svelte (1)
80-80: Bind click handler using Svelte syntaxUse
on:click={onclick}instead of passing{onclick}to ensure reliable event binding.- <button type="button" class="{baseClasses} {getCategoryColor(category)}" {onclick}> + <button type="button" class="{baseClasses} {getCategoryColor(category)}" on:click={onclick}>⛔ Skipped due to learnings
Learnt from: ab-smith Repo: intuitem/ciso-assistant-community PR: 2610 File: frontend/src/routes/(app)/(internal)/applied-controls/flash-mode/+page.svelte:175-175 Timestamp: 2025-09-29T20:07:40.257Z Learning: In Svelte 5, event handlers use DOM-style property syntax (onkeydown, onclick, oninput, etc.) instead of the Svelte 4 directive syntax (on:keydown, on:click, on:input, etc.). This applies to all event handlers including those on svelte:window and svelte:document.backend/core/dora.py (1)
5-90: I can see the search returned references to the EBA's XBRL taxonomy but not the specific label mappings. Let me search for more targeted information about the official DORA labels.Let me search for more specific information about accessing the official DORA taxonomy.
Let me search for the regulatory text that officially defines these codes and labels.
Based on my investigation, I found that the EBA publishes official DORA XBRL taxonomy packages and sample files containing the taxonomy mappings, and the technical package includes XBRL taxonomies addressing various reporting requirements, including cross-sectoral technical standards for ICT third-party service providers under DORA.
However, I was unable to retrieve the specific official label mappings for entity types (eba_CT codes) and ICT service types (eba_TA codes) through available web sources. The official taxonomy data is published primarily in XBRL format files rather than human-readable documentation.
One potential discrepancy identified:
- The review mentions eba_CT:x304 as "Central securities depository" (plural), but the code snippet shows "Central security depository" (singular)
Without access to the complete official EBA/EIOPA DORA taxonomy definitions, I cannot definitively verify whether all labels in the code match the official taxonomy exactly. The review's suggestion is marked as optional and requesting verification—indicating inherent uncertainty about the extent of any required changes.
Verify the DORA taxonomy labels against the official EBA technical package (available at eba.europa.eu/activities/direct-supervision-and-oversight/digital-operational-resilience-act/preparation-dora-application) by downloading the current XBRL taxonomy and data point model, then comparing the eba_CT and eba_TA code-to-label mappings against the lists in backend/core/dora.py. Pay particular attention to capitalization and singular/plural variations (e.g., eba_CT:x304).
backend/core/constants.py (1)
1-253: LGTM! Comprehensive country list expansion.The expanded COUNTRY_CHOICES list is well-structured with proper Unicode handling for special characters (Åland, Curaçao, Réunion, etc.) and includes a useful sentinel entry for "Other Countries".
frontend/src/lib/components/Forms/ModelForm/EntityForm.svelte (4)
5-7: LGTM! Component imports aligned with new form fields.The new imports (NumberField, LegalIdentifierField, Dropdown) are properly added to support the enhanced entity form functionality.
48-58: LGTM! Proper conditional rendering for builtin entities.The relationship field is correctly hidden for builtin entities, preventing unintended modifications to system entities.
59-83: LGTM! Clean organization of optional fields.The LegalIdentifierField is properly integrated, and grouping reference_link and country in a collapsible "more" dropdown improves form UX by keeping optional fields accessible but not cluttering the main view.
85-129: LGTM! DORA compliance fields well-organized.The DORA-specific fields are logically grouped in a separate dropdown with appropriate field types. The scale-balanced icon effectively represents the regulatory nature of these fields, and starting collapsed keeps the form manageable while ensuring compliance data is accessible.
frontend/src/lib/utils/crud.ts (4)
485-492: LGTM! Asset-solution relationships properly configured.The bidirectional asset-solution relationship is correctly configured with
disableCreateanddisableDeleteflags on the reverse relationship (line 503), ensuring link management is controlled from the assets side. The addition ofis_criticalandsolutionsto detailViewFields provides proper visibility of these important fields.Also applies to: 503-517
763-793: LGTM! Entity model properly configured for DORA compliance.The detailViewFields provide a focused view of core entity information, and the new selectFields (country, currency, dora_entity_type, dora_entity_hierarchy) align with the backend Entity model enhancements. The addition of contracts to reverseForeignKeyFields enables proper relationship tracking.
829-855: LGTM! Comprehensive contracts model configuration.The new contracts model is well-structured with:
- Proper relationship controls via disableCreate/disableDelete flags ensuring link management happens from the appropriate side
- Self-referential overarching_contract field enabling hierarchical contract structures
- Complete selectFields covering contract status, currency, DORA compliance fields, and governing law
This aligns well with the backend Contract model and supports the DORA reporting objectives.
821-827: LGTM! Solutions model updated for contract integration.The addition of contracts to reverseForeignKeyFields and the dora_ict_service_type selectField properly extends the solutions model to support contract relationships and DORA ICT service classification.
backend/tprm/models.py (3)
213-219: LGTM! Solution model enhanced for DORA ICT classification.The
dora_ict_service_typefield is properly configured with appropriate constraints and help text to support DORA ICT service classification requirements.
228-341: LGTM! Comprehensive Contract model for DORA compliance.The Contract model is well-designed with:
- Clear Status enum for contract lifecycle management
- Proper many-to-many relationships to entities, solutions, and evidences with descriptive help text
- Self-referential
overarching_contractfield enabling hierarchical contract structures- Complete DORA-specific fields for regulatory reporting
- Appropriate use of mixins (FolderMixin, FilteringLabelMixin) for standard functionality
The model effectively supports the DORA reporting requirements outlined in the PR objectives. The only suggestion is to consider DecimalField for monetary values as noted separately.
362-365: LGTM! Audit logging properly configured.The Contract model is correctly registered with auditlog using the common exclusion pattern, ensuring proper change tracking for compliance and audit purposes.
| legal_identifiers = serializers.SerializerMethodField() | ||
|
|
||
| def get_legal_identifiers(self, obj): | ||
| """Format legal identifiers as a readable string for display""" | ||
| if not obj.legal_identifiers: | ||
| return "" | ||
| return "\n".join( | ||
| [f"{key}: {value}" for key, value in obj.legal_identifiers.items()] | ||
| ) |
There was a problem hiding this comment.
Don’t override JSON field with a string in read serializer
legal_identifiers is a JSONField on the model, but the read serializer replaces it with a string via SerializerMethodField. This changes the API shape and can break clients expecting an object. Expose the pretty string under a different name and keep the raw JSON.
- legal_identifiers = serializers.SerializerMethodField()
+ legal_identifiers_display = serializers.SerializerMethodField()
@@
- def get_legal_identifiers(self, obj):
+ def get_legal_identifiers_display(self, obj):
"""Format legal identifiers as a readable string for display"""
if not obj.legal_identifiers:
return ""
return "\n".join(
[f"{key}: {value}" for key, value in obj.legal_identifiers.items()]
)📝 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.
| legal_identifiers = serializers.SerializerMethodField() | |
| def get_legal_identifiers(self, obj): | |
| """Format legal identifiers as a readable string for display""" | |
| if not obj.legal_identifiers: | |
| return "" | |
| return "\n".join( | |
| [f"{key}: {value}" for key, value in obj.legal_identifiers.items()] | |
| ) | |
| legal_identifiers_display = serializers.SerializerMethodField() | |
| def get_legal_identifiers_display(self, obj): | |
| """Format legal identifiers as a readable string for display""" | |
| if not obj.legal_identifiers: | |
| return "" | |
| return "\n".join( | |
| [f"{key}: {value}" for key, value in obj.legal_identifiers.items()] | |
| ) |
🤖 Prompt for AI Agents
In backend/tprm/serializers.py around lines 26 to 34, the serializer currently
overrides the model's JSONField legal_identifiers with a SerializerMethodField
that returns a string; instead keep the original JSON object in the serialized
output and expose the formatted string under a new field name (e.g.,
legal_identifiers_pretty). Remove or rename the existing SerializerMethodField
so it no longer replaces legal_identifiers, implement
get_legal_identifiers_pretty to return the "\n".join formatted string (or empty
string if none), and update the serializer's fields list to include the new
pretty field while leaving legal_identifiers as the raw JSON.
| class ContractReadSerializer(BaseModelSerializer): | ||
| folder = FieldsRelatedField() | ||
| owner = FieldsRelatedField(many=True) | ||
| entities = FieldsRelatedField(many=True) | ||
| evidences = FieldsRelatedField(many=True) | ||
| solutions = FieldsRelatedField(many=True) | ||
| overarching_contract = FieldsRelatedField() | ||
|
|
||
| class Meta: | ||
| model = Contract | ||
| exclude = [] |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Nullable relation should allow null representation
overarching_contract is optional; FieldsRelatedField() doesn’t handle None unless allowed. Set allow_null=True to avoid serialization errors when unset.
-class ContractReadSerializer(BaseModelSerializer):
+class ContractReadSerializer(BaseModelSerializer):
folder = FieldsRelatedField()
owner = FieldsRelatedField(many=True)
entities = FieldsRelatedField(many=True)
evidences = FieldsRelatedField(many=True)
solutions = FieldsRelatedField(many=True)
- overarching_contract = FieldsRelatedField()
+ overarching_contract = FieldsRelatedField(allow_null=True)📝 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.
| class ContractReadSerializer(BaseModelSerializer): | |
| folder = FieldsRelatedField() | |
| owner = FieldsRelatedField(many=True) | |
| entities = FieldsRelatedField(many=True) | |
| evidences = FieldsRelatedField(many=True) | |
| solutions = FieldsRelatedField(many=True) | |
| overarching_contract = FieldsRelatedField() | |
| class Meta: | |
| model = Contract | |
| exclude = [] | |
| class ContractReadSerializer(BaseModelSerializer): | |
| folder = FieldsRelatedField() | |
| owner = FieldsRelatedField(many=True) | |
| entities = FieldsRelatedField(many=True) | |
| evidences = FieldsRelatedField(many=True) | |
| solutions = FieldsRelatedField(many=True) | |
| overarching_contract = FieldsRelatedField(allow_null=True) | |
| class Meta: | |
| model = Contract | |
| exclude = [] |
🤖 Prompt for AI Agents
In backend/tprm/serializers.py around lines 299 to 309, the
ContractReadSerializer's overarching_contract field is optional but currently
declared as FieldsRelatedField() which will raise on None; update that field to
allow nulls by passing allow_null=True (e.g.,
FieldsRelatedField(allow_null=True)) so the serializer can represent unset
overarching_contract without error.
ab-smith
changed the title
Summary by CodeRabbit
New Features
Permissions