feat: add custom qualifications #2446
Conversation
WalkthroughImplements Qualifications as first-class objects linked to libraries and risk scenarios. Migrates RiskScenario.qualifications to ManyToMany, adds import support via QualificationImporter, surfaces qualifications in library dumps/UI, updates serializers/views, adjusts permission checks to use model objects, expands i18n keys, and adds tests. Includes related migrations and documentation tweaks. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Admin as Admin User
participant UI as Library UI
participant Importer as LibraryImporter
participant QImp as QualificationImporter
participant DB as DB
Admin->>UI: Upload YAML (includes qualifications)
UI->>Importer: init(library_data)
Importer->>Importer: init_qualifications([...])
loop each qualification
Importer->>QImp: is_valid()
QImp-->>Importer: None (ok)
end
UI->>Importer: import_objects(library_object)
loop each qualification
Importer->>QImp: import_qualification(library_object)
QImp->>DB: create Qualification(library=library_object, ...)
end
UI->>DB: Fetch library dump (includes qualifications)
DB-->>UI: qualifications list
sequenceDiagram
autonumber
actor User
participant RSPage as Risk Scenario Edit Page
participant API as API
participant ACL as canPerformAction
User->>RSPage: Open edit form
RSPage->>API: GET /api/qualifications?search=...
API-->>RSPage: { results: [ {id, name, ...}, ... ] }
RSPage->>User: AutocompleteSelect(multiselect)
note over RSPage: On save, sends qualification ids
RSPage->>ACL: canPerformAction({ user, action:'change', model: ModelMapEntry })
ACL-->>RSPage: boolean
RSPage->>User: Enable/disable edit actions
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/core/models.py (1)
1-1: Apply Ruff formatting to backend/core/models.pyCI indicates formatting issues in this file. Running
ruff format backend/core/models.pyreformats the file (1 file reformatted). Please commit these formatting changes to ensure the build passes.
• File requiring formatting:
backend/core/models.py
🧹 Nitpick comments (21)
frontend/messages/el.json (1)
1666-1669: EL: Verify domain terminology for “qualification” vs existing “qualifications” usageThe three additions are grammatically correct. However, earlier in this file “qualifications” is translated as “Κατηγορίες επιπτώσεων,” which conceptually differs from “προσόντα.” If both refer to the same domain object (Qualifications), there may be a terminology mismatch.
- If “Qualifications” are impact categories in your domain, consider aligning:
- "qualificationOrdering": "Σειρά κατάταξης προσόντων", + "qualificationOrdering": "Σειρά κατηγοριών",
- If they truly are “qualifications” as in labels/abbreviations, consider updating the earlier “qualifications” entry for consistency instead.
Would you like me to propose a harmonized terminology sweep for EL across the file?frontend/messages/en.json (2)
1801-1805: Keys added; consider clarifying “showingItemsWith”.
- The three new keys are good.
- Minor: “showingItemsWith” may act as a prefix elsewhere. If it’s meant to be a standalone label, consider “Showing items with:” (with a colon). If it’s concatenated in code, current text is fine. Please confirm usage.
477-477: Fix a few visible typos (outside the changed hunk).These are small, user-facing strings worth correcting:
- Line 477: "Approuved" → "Approved"
- Line 531: "Transfered" → "Transferred"
- Line 1249: "Refence ID" → "Reference ID"
Apply this diff:
- "ok": "Approuved", + "ok": "Approved", @@ - "transfer": "Transfered", + "transfer": "Transferred", @@ - "refIdSemiColon": "Refence ID:", + "refIdSemiColon": "Reference ID:",Also applies to: 531-531, 1249-1249
frontend/messages/de.json (1)
967-971: Nit: Make the German phrasing more idiomatic.“Angstereignisse” sounds awkward; “Gefürchtete Ereignisse” is the common rendering. If you agree, adjust the string.
Apply:
-"fearedEventAssetHelpText": "Angstereignisse sind in der Regel mit Geschäftswerten (primäre Vermögenswerte) verknüpft. Unterstützende Vermögenswerte sind hier aus Flexibilitätsgründen erlaubt.", +"fearedEventAssetHelpText": "Gefürchtete Ereignisse sind in der Regel mit Geschäfts-Assets (primären Assets) verknüpft. Unterstützende Assets sind hier aus Flexibilitätsgründen erlaubt.",frontend/messages/cs.json (1)
950-950: Minor Czech diacritic fix in help text“Podporná aktiva” should be “Podpůrná aktiva”.
-"fearedEventAssetHelpText": "Strašné události jsou obvykle spojeny s obchodními hodnotami (primární aktiva). Podporná aktiva jsou zde povolena pro větší flexibilitu.", +"fearedEventAssetHelpText": "Strašné události jsou obvykle spojeny s obchodními hodnotami (primární aktiva). Podpůrná aktiva jsou zde povolena pro větší flexibilitu.",frontend/messages/hu.json (1)
962-962: Hungarian terminology tweak for naturalnessPrefer “támogató eszközök” over “támogatási eszközök”.
-"fearedEventAssetHelpText": "A félelemkeltő események általában üzleti értékekkel (elsődleges eszközökkel) vannak összekapcsolva. A támogatási eszközök itt is engedélyezettek a rugalmasság érdekében.", +"fearedEventAssetHelpText": "A félelemkeltő események általában üzleti értékekkel (elsődleges eszközökkel) vannak összekapcsolva. A támogató eszközök itt is engedélyezettek a rugalmasság érdekében.",frontend/messages/ur.json (1)
930-930: Improve Urdu phrasing for clarity and domain consistencyUse standard terms for “primary assets” and “supporting assets.”
-"fearedEventAssetHelpText": "خوفناک واقعات عام طور پر کاروباری قیمتیں (پہلی جائیدادیں) سے منسلک ہوتے ہیں۔ ان کے لیے اندرونی جائیدادیں بھی ان کے لچکدار ہونے کی وجہ سے اجازت دی جاتی ہیں۔", +"fearedEventAssetHelpText": "خوف زدہ واقعات عموماً کاروباری اقدار (پرائمری اثاثے) سے منسلک ہوتے ہیں۔ لچک کے لیے یہاں معاون اثاثوں کی بھی اجازت ہے۔",frontend/messages/sv.json (1)
953-956: LGTM on new i18n keys; minor Swedish wording nitpicks for clarityTranslations are correct and the JSON structure is valid. For more natural Swedish, consider the tweaks below.
- "fearedEventAssetHelpText": "Rädsla för händelser är vanligtvis länkade till företagsvärden (primära tillgångar). Stödjande tillgångar är tillåtna här för flexibilitet.", + "fearedEventAssetHelpText": "Fruktade händelser är vanligtvis kopplade till affärsvärden (primära tillgångar). Stödjande tillgångar är tillåtna här för flexibilitet.", - "qualificationOrdering": "Kvalifikationsordning", + "qualificationOrdering": "Kvalificeringsordning", - "securityObjectiveOrdering": "Säkerhetsmålsordning" + "securityObjectiveOrdering": "Säkerhetsmålens ordning"frontend/src/lib/components/Forms/ModelForm/QualificationForm.svelte (3)
32-38: Clamp ordering to non-negative integers (min/step) to avoid accidental floats/negativesPrevents invalid values before they reach validation.
<NumberField {form} field="qualification_ordering" label={m.qualificationOrdering()} cacheLock={cacheLocks['qualification_ordering']} bind:cachedValue={formDataCache['qualification_ordering']} + min={0} + step={1} />
39-45: Same as above: enforce sensible bounds for security objective orderingMirror constraints for consistency with the other ordering field.
<NumberField {form} field="security_objective_ordering" label={m.securityObjectiveOrdering()} cacheLock={cacheLocks['security_objective_ordering']} bind:cachedValue={formDataCache['security_objective_ordering']} + min={0} + step={1} />
18-24: Limit the abbreviation input to 20 charactersQualificationSchema caps
abbreviationat 20 characters (z.string().max(20)) infrontend/src/lib/utils/schemas.ts:268, so it’s best to reflect this constraint in the UI to avoid unnecessary validation errors.• Add a
maxlength={20}prop to the<TextField>infrontend/src/lib/components/Forms/ModelForm/QualificationForm.svelte:<TextField {form} field="abbreviation" label={m.abbreviation()} cacheLock={cacheLocks['abbreviation']} bind:cachedValue={formDataCache['abbreviation']} + maxlength={20} />frontend/src/lib/utils/table.ts (1)
1467-1469: Expose ref_id/name/description/abbreviation for qualifications — good step; consider minor UX polishShowing ref_id and abbreviation in the list improves scannability. As a small improvement, consider adding meta identifiers (e.g., id, urn) like other library-backed models to help with debugging and deep-links in future.
Suggested tweak (optional):
qualifications: { - head: ['ref_id', 'name', 'description', 'abbreviation'], - body: ['ref_id', 'name', 'description', 'abbreviation'] + head: ['ref_id', 'name', 'description', 'abbreviation'], + body: ['ref_id', 'name', 'description', 'abbreviation'], + meta: ['id', 'urn'] },backend/resilience/migrations/0003_alter_escalationthreshold_qualifications.py (1)
1-20: CI Formatting Issue ResolvedThe migration file has been reformatted with Ruff and now passes all checks—no further action is needed to unblock CI.
Optional: improve readability by pluralizing the reverse relation name. If you choose to rename
related_name="escalation_threshold"torelated_name="escalation_thresholds", update these locations:
- In
backend/resilience/models.py(EscalationThreshold.qualifications field):- related_name="escalation_threshold" + related_name="escalation_thresholds"- In
backend/core/models.py(Qualification.is_deletable):- self.escalation_threshold.exists() + self.escalation_thresholds.exists()- In the migration
0003_alter_escalationthreshold_qualifications.py, updaterelated_nameaccordingly so that historical migrations remain consistent.No other references to the singular reverse name were found.
frontend/src/lib/utils/schemas.ts (1)
266-273: Align Zod schema with backend model nullability and defaultsVerified in backend/core/models.py that:
abbreviationis declared asmodels.CharField(max_length=20, null=True, blank=True)(lines 1560–1563), so it must be optional and nullable.annotationis declared asmodels.TextField(null=True, blank=True)(lines 101–102), so it’s already optional/nullable.qualification_orderingandsecurity_objective_orderingarePositiveSmallIntegerField(default=0)(lines 1563–1567) with no nulls, so they default to 0 on the server.Please update the frontend Zod schema to match:
export const QualificationSchema = z.object({ ...NameDescriptionMixin, - abbreviation: z.string().max(20), - annotation: z.string().optional().nullable(), - qualification_ordering: z.number().min(0).max(32767), - security_objective_ordering: z.number().min(0).max(32767) + abbreviation: z.string().max(20).optional().nullable(), + annotation: z.string().optional().nullable(), + qualification_ordering: z.coerce.number().min(0).max(32767).default(0), + security_objective_ordering: z.coerce.number().min(0).max(32767).default(0), });
- The
.optional().nullable()onabbreviationaligns withnull=True, blank=True.- Keeping
annotationunchanged is correct.- Using
z.coerce.number().default(0)ensures string inputs are cast to numbers and the default of0matches the Django model.frontend/src/routes/(app)/(internal)/loaded-libraries/[id=uuid]/+page.svelte (1)
189-206: Dropdown block for qualifications — consistent with existing blocksNon-interactive ModelTable and count-based header follow the established pattern. Nice addition.
If you foresee very large qualification sets, consider lazy rendering (e.g., virtualized rows) or pagination in a follow-up to avoid long DOMs. No change needed now.
backend/core/models.py (3)
909-931: reference_count may overcount; use distinct() for M2M joinsCounts on Incident/FearedEvent/EscalationThreshold can duplicate rows when an object has multiple qualifications from the same library. For accuracy and consistency with RA/CA parts already using distinct(), suggest:
- + Incident.objects.filter(qualifications__library=self).count() - + FearedEvent.objects.filter(qualifications__library=self).count() - + EscalationThreshold.objects.filter(qualifications__library=self).count() + + Incident.objects.filter(qualifications__library=self).distinct().count() + + FearedEvent.objects.filter(qualifications__library=self).distinct().count() + + EscalationThreshold.objects.filter(qualifications__library=self).distinct().count()
1062-1068: Typo in ReferenceControl.is_deletable docstring“refence” → “reference”.
- Returns True if the refence control can be deleted + Returns True if the reference control can be deleted
904-912: Optional: Update reference_count docstring for new sourcesConsider mentioning incidents/feared events/escalation thresholds to reflect the broadened count.
- Returns the number of distinct dependent libraries and risk and compliance assessments that reference objects from this library + Returns the number of distinct dependent libraries, assessments, and other domain objects (incidents, feared events, escalation thresholds) that reference objects from this library.backend/library/utils.py (3)
412-437: QualificationImporter — solid baseline; consider light validationImporter mirrors Threat/ReferenceControl patterns. Optional: validate ordering fields if present to fail-fast on bad data.
class QualificationImporter: REQUIRED_FIELDS = {"ref_id", "urn", "name"} @@ def is_valid(self) -> Union[str, None]: if missing_fields := self.REQUIRED_FIELDS - set(self.qualification_data.keys()): return "Missing the following fields : {}".format(", ".join(missing_fields)) + qo = self.qualification_data.get("qualification_ordering") + so = self.qualification_data.get("security_objective_ordering") + if qo is not None and not isinstance(qo, int): + return "qualification_ordering must be an integer" + if so is not None and not isinstance(so, int): + return "security_objective_ordering must be an integer"
728-733: Incorrect log message label (“Threat import error” for qualifications)Use a qualification-specific label.
- if (qualification_import_error := self.init_qualifications(qualification_data)) is not None: - logger.error("Threat import error", error=qualification_import_error) + if (qualification_import_error := self.init_qualifications(qualification_data)) is not None: + logger.error("Qualification import error", error=qualification_import_error)
451-458: Unreachable code in RiskMatrixImporter.is_validEarly return makes the missing_fields check dead code. Either remove it or move the return after validation.
- def is_valid(self) -> Union[str, None]: - return None # Do not verify anything for now - - # Create function to check if the "JSON definition" of the matrix is wrong or not, this function will be called within this is_valid function and return an error string is an error occured or return None or success exactly like this one. - if missing_fields := self.REQUIRED_FIELDS - set(self.risk_matrix_data.keys()): - return "Missing the following fields : {}".format(", ".join(missing_fields)) + def is_valid(self) -> Union[str, None]: + # No validation for now + return None
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (34)
backend/core/migrations/0091_qualification_library_and_more.py(1 hunks)backend/core/models.py(7 hunks)backend/core/serializers.py(1 hunks)backend/library/utils.py(7 hunks)backend/resilience/migrations/0003_alter_escalationthreshold_qualifications.py(1 hunks)backend/resilience/models.py(1 hunks)frontend/messages/ar.json(1 hunks)frontend/messages/cs.json(1 hunks)frontend/messages/da.json(1 hunks)frontend/messages/de.json(1 hunks)frontend/messages/el.json(1 hunks)frontend/messages/en.json(1 hunks)frontend/messages/es.json(1 hunks)frontend/messages/fr.json(1 hunks)frontend/messages/hi.json(1 hunks)frontend/messages/hu.json(1 hunks)frontend/messages/id.json(1 hunks)frontend/messages/it.json(1 hunks)frontend/messages/nl.json(1 hunks)frontend/messages/pl.json(1 hunks)frontend/messages/pt.json(1 hunks)frontend/messages/ro.json(1 hunks)frontend/messages/sv.json(1 hunks)frontend/messages/tr.json(1 hunks)frontend/messages/uk.json(1 hunks)frontend/messages/ur.json(1 hunks)frontend/src/lib/components/Forms/ModelForm.svelte(2 hunks)frontend/src/lib/components/Forms/ModelForm/QualificationForm.svelte(1 hunks)frontend/src/lib/components/SideBar/navData.ts(1 hunks)frontend/src/lib/utils/crud.ts(1 hunks)frontend/src/lib/utils/schemas.ts(2 hunks)frontend/src/lib/utils/table.ts(1 hunks)frontend/src/routes/(app)/(internal)/[model=urlmodel]/+page.svelte(1 hunks)frontend/src/routes/(app)/(internal)/loaded-libraries/[id=uuid]/+page.svelte(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
backend/resilience/migrations/0003_alter_escalationthreshold_qualifications.py (1)
backend/core/migrations/0091_qualification_library_and_more.py (1)
Migration(7-24)
backend/core/migrations/0091_qualification_library_and_more.py (1)
backend/resilience/migrations/0003_alter_escalationthreshold_qualifications.py (1)
Migration(6-19)
backend/core/serializers.py (2)
backend/core/models.py (17)
Meta(107-108)Meta(162-163)Meta(190-191)Meta(195-197)Meta(992-994)Meta(1058-1060)Meta(1189-1191)Meta(1323-1325)Meta(1570-1573)Meta(1626-1628)Meta(1849-1851)Meta(2398-2406)Meta(2432-2434)Meta(2553-2555)Meta(2772-2774)Meta(2912-2914)Qualification(1431-1594)backend/core/serializer_fields.py (1)
FieldsRelatedField(24-86)
frontend/src/lib/utils/schemas.ts (1)
backend/core/base_models.py (1)
NameDescriptionMixin(111-124)
backend/core/models.py (3)
backend/library/helpers.py (1)
update_translations_in_object(49-75)backend/ebios_rm/models.py (1)
FearedEvent(248-313)backend/resilience/models.py (1)
EscalationThreshold(176-248)
backend/library/utils.py (1)
backend/core/models.py (3)
Qualification(1431-1594)LoadedLibrary(849-971)create(2985-2987)
🪛 GitHub Actions: Backend Linters
backend/resilience/migrations/0003_alter_escalationthreshold_qualifications.py
[error] 1-1: Ruff format check detected formatting issues; this file would be reformatted by running 'ruff format'. Command: 'ruff format --check .'
backend/core/migrations/0091_qualification_library_and_more.py
[error] 1-1: Ruff format check detected formatting issues; this file would be reformatted by running 'ruff format'. Command: 'ruff format --check .'
backend/core/models.py
[error] 1-1: Ruff format check detected formatting issues; this file would be reformatted by running 'ruff format'. Command: 'ruff format --check .'
backend/library/utils.py
[error] 1-1: Ruff format check detected formatting issues; this file would be reformatted by running 'ruff format'. Command: 'ruff format --check .'
⏰ 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). (6)
- GitHub Check: build (3.12)
- GitHub Check: build_community_frontend
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: test (3.12)
- GitHub Check: startup-docker-compose-test
🔇 Additional comments (38)
frontend/messages/fr.json (1)
1762-1766: FR i18n keys coverage & usage verified• All locale JSON files are valid and include the keys
abbreviation,qualificationOrdering, andsecurityObjectiveOrdering.
• UI references exist across components (e.g., QualificationForm.svelte, table mappings, RiskMatrix stories).
• French strings read correctly.Optional nitpick: for consistency, you may prefer a plural label
- "qualificationOrdering": "Ordre de qualification", + "qualificationOrdering": "Ordre des qualifications",frontend/messages/id.json (1)
1150-1153: Localization keys added and usage verified
- Confirmed that
frontend/messages/id.jsoncontains the new keys:abbreviation,qualificationOrdering, andsecurityObjectiveOrdering.- Verified these keys are referenced in
frontend/src/lib/components/Forms/ModelForm/QualificationForm.svelteat lines 21, 35, and 42.- Minor style nit: apply sentence case to the “deselect” translation to align with Indonesian UI conventions.
- "deselect": "Batalkan Pilihan", + "deselect": "Batalkan pilihan",Code changes look good—thanks for adding the translations and ensuring they’re used correctly.
frontend/messages/pt.json (1)
960-963: PT: New qualification-related labels are accurate and consistent
- “Abreviação”, “Ordenação de qualificações” and “Ordenação dos objetivos de segurança” read well and match the feature intent.
- Trailing comma before the new keys keeps the JSON valid.
frontend/messages/nl.json (1)
963-966: NL: Added keys read naturally; JSON punctuation is correct
- “Afkorting”, “Kwalificatievolgorde”, and “Volgorde van beveiligingsdoelen” are clear.
- The comma adjustment before these keys preserves JSON validity.
frontend/messages/ro.json (1)
959-962: New qualification keys translated correctly; JSON remains valid.
- "Abreviere", "Ordonarea calificărilor", and "Ordonarea obiectivelor de securitate" read well and match the intended semantics.
- The preceding fearedEventAssetHelpText now ends with a comma, keeping the JSON valid.
frontend/messages/tr.json (1)
1702-1705: Turkish translations look good and consistent with UI usage.
- "İlişkili kampanyalar", "Kısaltma", "Nitelik sıralaması", "Güvenlik hedefi sıralaması" are clear and idiomatic.
- No trailing comma after the last key; JSON stays valid.
frontend/messages/ar.json (1)
930-933: Arabic additions are accurate; RTL text is fine.
- "اختصار", "ترتيب المؤهلات", and "ترتيب الأهداف الأمنية" correctly convey the intended meanings.
- Comma added to the previous line keeps JSON valid.
frontend/messages/it.json (1)
1340-1343: Italian translations are clear and consistent.
- "Abbreviazione", "Ordinamento delle qualifiche", "Ordinamento degli obiettivi di sicurezza" look correct.
- Placement after "Deseleziona" and commas are correct; JSON valid.
frontend/messages/hi.json (1)
931-935: LGTM: Hindi translations read naturally and align with new fields.No issues spotted for abbreviation/ordering keys.
frontend/messages/es.json (1)
967-971: All locales updated with new i18n keysVerified that the three new keys—
abbreviation,qualificationOrdering, andsecurityObjectiveOrdering—are present in everyfrontend/messages/*.jsonfile (exit code 0, no missing entries). No further action required; ready to merge.frontend/messages/cs.json (2)
951-953: New qualification keys added correctlyKeys and Czech translations for abbreviation and the two orderings look good and align with the PR’s new UI fields.
950-953: Locale messages consistency and integrity verifiedAll JSON locale files parse without errors, and each one includes the new keys
abbreviation,qualificationOrdering, andsecurityObjectiveOrdering. No missing entries were detected—no further action is needed.frontend/messages/hu.json (1)
963-965: New qualification keys and translations look correctHungarian translations for the three new keys read well and are consistent with the rest of the file.
frontend/messages/da.json (2)
1246-1248: Danish translations for the new qualification keys are spot onGood wording and casing; matches existing style.
1245-1245: JSON delimiter fix acknowledgedAdding the trailing comma after "deselect" is necessary and keeps the object valid when appending new keys.
frontend/messages/ur.json (1)
931-933: New qualification keys added with appropriate Urdu translationsThe three added labels map correctly to the new fields.
frontend/messages/pl.json (2)
1709-1711: Polish translations for new keys look correct“Skrót”, “Kolejność kwalifikacji”, and “Kolejność celów bezpieczeństwa” are accurate and consistent.
1707-1707: Delimiter added correctly before new keysThe trailing comma after “recentlyUpdatedAudits” correctly prepares for the appended properties.
frontend/src/lib/components/Forms/ModelForm/QualificationForm.svelte (1)
1-46: LGTM: focused, consistent form componentComponent plugs cleanly into existing form controls and i18n. Props and binding patterns match the broader ModelForm ecosystem.
frontend/src/lib/components/Forms/ModelForm.svelte (1)
75-75: Import looks correct and consistent with patterns used for other model-specific formsfrontend/src/routes/(app)/(internal)/[model=urlmodel]/+page.svelte (1)
118-118: LGTM: Add button enabled for qualificationsRemoving 'qualifications' from the disabled list correctly exposes the Create flow for this model and matches the new form integration.
frontend/src/lib/utils/crud.ts (2)
724-745: Qualification detail view: format timestamps and render library nicely
- created_at/updated_at aren’t tagged as datetime, so they’ll render as raw strings. Use type: 'datetime' for consistency with other models.
- The detail shows library but this entry lacks a foreignKeyFields mapping, so UI may display raw IDs or “[object Object]” depending on the serializer shape. Since backend links Qualification.library -> core.loadedlibrary, wire it to 'loaded-libraries' for proper display and navigation.
Proposed patch:
qualifications: { name: 'qualification', localName: 'qualification', localNamePlural: 'qualifications', verboseName: 'Qualification', verboseNamePlural: 'Qualifications', detailViewFields: [ { field: 'name' }, { field: 'description' }, { field: 'annotation' }, - { field: 'created_at' }, - { field: 'updated_at' }, + { field: 'created_at', type: 'datetime' }, + { field: 'updated_at', type: 'datetime' }, { field: 'urn' }, { field: 'id' }, { field: 'provider' }, { field: 'locale' }, { field: 'default_locale' }, { field: 'abbreviation' }, { field: 'qualification_ordering' }, { field: 'security_objective_ordering' }, { field: 'library' } - ] + ], + foreignKeyFields: [ + { field: 'library', urlModel: 'loaded-libraries' } + ] }[ suggest_optional_refactor ]
724-745: The scripts above will locate theReferentialSerializerandFieldsRelatedFielddefinitions to confirm which fields are exposed by theQualificationReadSerializer. Once we have that context, we can verify ifprovider,locale,default_locale, andlibraryare included in the API output.frontend/src/lib/utils/schemas.ts (1)
954-955: Registering QualificationSchema — LGTMThe schema is correctly added to SCHEMA_MAP under qualifications. This will enable form validation for the new model.
frontend/src/routes/(app)/(internal)/loaded-libraries/[id=uuid]/+page.svelte (2)
25-25: Wiring qualifications from library objects — LGTMPulling qualifications from data.library.objects keeps this page aligned with the updated library serializer.
69-74: Qualifications table source mirrors list fields — consistent and clearHead/body mappings align with the new list columns and translations (ref, name, description, abbreviation). Looks good.
backend/core/migrations/0091_qualification_library_and_more.py (3)
14-18: Add Qualification.library FK — consistent with existing referentialsOptional, nullable FK with CASCADE and a clear related_name aligns with Threat/ReferenceControl patterns. Good addition.
19-23: is_published default=True — confirm intended behavior for new rowsAltering the default affects only newly created rows. If you expect existing rows to flip to True, you’ll need a data migration. Otherwise, LGTM.
1-1: Migration file formatting appliedThe migration file
0091_qualification_library_and_more.pyhas been successfully reformatted with Ruff, and the CI formatting check should now pass without issues. No further action is required.backend/core/serializers.py (2)
1545-1549: QualificationWriteSerializer — minimal and consistentExcluding only translations matches Threat/ReferenceControl write serializers. No concerns.
1551-1556: Expose library on Qualification reads — consistent with other referentialslibrary = FieldsRelatedField(["name","id"]) matches Framework/Threat/ReferenceControl. Good.
backend/core/models.py (4)
887-891: Include qualifications in LoadedLibrary._objects exportMatches threats/reference_controls export and front-end usage. Good.
996-1000: Docstring fix for Threat.is_deletable — LGTMClearer docstring; behavior unchanged. Good.
1550-1556: Qualification.library/is_published fields — correct modelingOptional FK with related_name and published flag keeps parity with other referentials. Good addition.
1575-1586: Qualification.is_deletable — sensible guardCovers incidents, feared events, and escalation thresholds via related_name. Nice.
Please ensure resilience migration updated EscalationThreshold.qualifications with related_name="escalation_threshold" (it appears so). If not, is_deletable would raise.
backend/library/utils.py (3)
568-589: init_qualifications — consistent error aggregationMatches init_threats/reference_controls patterns. LGTM.
825-827: Wire qualifications into import_objects — LGTMObjects are created after library creation; consistent with other object types.
1-1: Formatting issues resolved
backend/library/utils.pyhas been successfully reformatted withruff format. CI should now pass without errors.
backend/resilience/models.py
Outdated
| qualifications = models.ManyToManyField( | ||
| Qualification, | ||
| blank=True, | ||
| related_name="escalation_threshold", | ||
| ) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Prefer plural related_name and verify downstream usages
Using a plural reverse name is clearer for a ManyToMany. Also verify no code still relies on the default escalationthreshold_set.
Suggested rename (requires a migration and code updates):
qualifications = models.ManyToManyField(
Qualification,
blank=True,
- related_name="escalation_threshold",
+ related_name="escalation_thresholds",
)Audit for usages to update:
If you decide to keep the singular name for backward compatibility, consider adding a brief comment explaining the choice to avoid future confusion.
🏁 Script executed:
#!/bin/bash
# Find reverse-relations/old default usages to update
rg -n "escalationthreshold_set\b|\.escalation_threshold\b|\.escalation_thresholds\b" -C2 --type=py --type=ts --type=tsx --type=js --type=svelteLength of output: 173
🏁 Script executed:
#!/bin/bash
# Re-run search across relevant file extensions using glob patterns
rg -n '\b(escalationthreshold_set|\.escalation_threshold\b|\.escalation_thresholds\b)' -C2 \
-g '*.py' -g '*.js' -g '*.ts' -g '*.jsx' -g '*.tsx' -g '*.svelte'Length of output: 871
Rename M2M related_name to plural and update downstream references
A plural reverse name avoids confusion and matches Django conventions for ManyToMany relations. After renaming related_name="escalation_thresholds", you must update every place the old singular name is used:
• backend/core/models.py (around line 1585)
- self.escalation_threshold.exists(),
+ self.escalation_thresholds.exists(),• Add a Django migration to rename the relation and update any fixtures or serialized data that reference escalation_threshold.
No other occurrences of .escalation_threshold or the default .escalationthreshold_set were found in the codebase; once this change is in place and migrations are applied, please run your test suite to ensure all reverse-lookups resolve correctly.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
backend/resilience/models.py lines 187-191: rename the ManyToManyField
related_name from "escalation_threshold" to the plural "escalation_thresholds"
and update every downstream reference (e.g., backend/core/models.py around line
1585) to use .escalation_thresholds; add a Django migration that alters the
field to set the new related_name (use an AlterField migration) and include any
data/fixture updates or a small RunPython step if serialized fixtures reference
the old name, then run the test suite to verify all reverse lookups resolve
correctly.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (8)
frontend/messages/id.json (1)
1152-1155: Refine Indonesian sentence casing for “deselect”
- In
frontend/messages/id.json(around line 1152), update:- "deselect": "Batalkan Pilihan", + "deselect": "Batalkan pilihan",frontend/messages/pl.json (1)
1709-1712: Natural Polish phrasing for “recentlyUpdatedAudits”; qualification keys look good.
- “Niedawno zaktualizowane audyty” is understandable, but “Ostatnio zaktualizowane audyty” is the more idiomatic UI phrase.
Apply this diff:
- "recentlyUpdatedAudits": "Niedawno zaktualizowane audyty", + "recentlyUpdatedAudits": "Ostatnio zaktualizowane audyty",frontend/src/routes/(app)/(internal)/[model=urlmodel]/+page.svelte (2)
81-93: Avoid list drift: centralize the "no-add" URL models and reuse in both placesThe same model list is duplicated in the hotkey handler and the template condition. Extracting it avoids future divergence.
Apply these diffs in-place:
- if ( - ![ - 'risk-matrices', - 'frameworks', - 'requirement-mapping-sets', - 'user-groups', - 'role-assignments' - ].includes(URLModel) - ) { + if (!DISABLE_ADD_URL_MODELS.has(URLModel)) { modalCreateForm(); }-{#if !['risk-matrices', 'frameworks', 'requirement-mapping-sets', 'user-groups', 'role-assignments'].includes(URLModel)} +{#if !DISABLE_ADD_URL_MODELS.has(URLModel)}Add this small helper near the top of the script (outside functions):
// Consolidated list of models where "Add" is disabled in UI const DISABLE_ADD_URL_MODELS = new Set([ 'risk-matrices', 'frameworks', 'requirement-mapping-sets', 'user-groups', 'role-assignments' ]);Also applies to: 119-119
136-147: Confirm whether qualifications should also expose a "Libraries" import actionThe PR summary notes qualifications are integrated into the loaded-libraries flow. If users are expected to import qualifications from libraries (like threats/reference-controls), consider adding it here.
If confirmed, minimally extend the check and keep a safe title fallback:
-{#if ['threats', 'reference-controls'].includes(URLModel)} +{#if ['threats', 'reference-controls', 'qualifications'].includes(URLModel)} {@const title = - URLModel === 'threats' ? m.importThreats() : m.importReferenceControls()} + URLModel === 'threats' + ? m.importThreats() + : URLModel === 'reference-controls' + ? m.importReferenceControls() + : m.libraries()}If a specific translation exists (e.g., m.importQualifications()), swap the fallback m.libraries() accordingly.
backend/core/models.py (2)
1064-1066: Typo in docstring (“refence” → “reference”).Small wording fix.
- Returns True if the refence control can be deleted + Returns True if the reference control can be deleted
1577-1587: Short-circuit the existence checks to avoid 3 queries when the first is True.The current any([...]) builds a list, issuing all three .exists() queries every time. Use or-chaining for short-circuit behavior.
- return not any( - [ - self.incidents.exists(), - self.feared_events.exists(), - self.escalation_thresholds.exists(), - ] - ) + return not ( + self.incidents.exists() + or self.feared_events.exists() + or self.escalation_thresholds.exists() + )backend/library/utils.py (2)
414-444: QualificationImporter mirrors model fields; consider update_or_create to be idempotent.Current create() is fine because StoredLibrary.load prevents re-import; if future flows re-run imports for the same version, switching to update_or_create keyed on (urn, locale) would make this idempotent.
Example:
- Qualification.objects.create( + Qualification.objects.update_or_create( + urn=self.qualification_data["urn"].lower(), + locale=library_object.locale, + defaults={ + "library": library_object, + "ref_id": self.qualification_data["ref_id"], + "name": self.qualification_data["name"], + "description": self.qualification_data.get("description"), + "abbreviation": self.qualification_data.get("abbreviation"), + "qualification_ordering": self.qualification_data.get("qualification_ordering", 0), + "security_objective_ordering": self.qualification_data.get("security_objective_ordering", 0), + "provider": library_object.provider, + "is_published": True, + "translations": self.qualification_data.get("translations", {}), + "default_locale": library_object.default_locale, + } - ) + )
735-744: Wrong log label: “Threat import error” for qualifications.Adjust the logger context for accurate diagnostics.
- logger.error("Threat import error", error=qualification_import_error) + logger.error("Qualification import error", error=qualification_import_error)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (28)
backend/core/migrations/0091_qualification_library_and_more.py(1 hunks)backend/core/models.py(7 hunks)backend/core/serializers.py(1 hunks)backend/library/utils.py(7 hunks)backend/resilience/migrations/0003_alter_escalationthreshold_qualifications.py(1 hunks)backend/resilience/models.py(1 hunks)frontend/messages/ar.json(1 hunks)frontend/messages/cs.json(1 hunks)frontend/messages/da.json(1 hunks)frontend/messages/de.json(1 hunks)frontend/messages/en.json(1 hunks)frontend/messages/es.json(1 hunks)frontend/messages/fr.json(1 hunks)frontend/messages/hi.json(1 hunks)frontend/messages/hu.json(1 hunks)frontend/messages/id.json(1 hunks)frontend/messages/it.json(1 hunks)frontend/messages/nl.json(1 hunks)frontend/messages/pl.json(1 hunks)frontend/messages/pt.json(1 hunks)frontend/messages/ro.json(1 hunks)frontend/messages/sv.json(1 hunks)frontend/messages/uk.json(1 hunks)frontend/messages/ur.json(1 hunks)frontend/src/lib/components/Forms/ModelForm.svelte(2 hunks)frontend/src/lib/utils/crud.ts(1 hunks)frontend/src/lib/utils/table.ts(1 hunks)frontend/src/routes/(app)/(internal)/[model=urlmodel]/+page.svelte(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (23)
- frontend/messages/ro.json
- frontend/messages/pt.json
- frontend/messages/hu.json
- frontend/src/lib/utils/crud.ts
- frontend/messages/cs.json
- frontend/messages/de.json
- frontend/messages/es.json
- frontend/messages/sv.json
- frontend/messages/it.json
- frontend/messages/da.json
- frontend/src/lib/utils/table.ts
- frontend/messages/uk.json
- frontend/messages/ar.json
- frontend/messages/nl.json
- backend/resilience/migrations/0003_alter_escalationthreshold_qualifications.py
- backend/resilience/models.py
- frontend/messages/fr.json
- backend/core/serializers.py
- frontend/messages/ur.json
- frontend/messages/en.json
- frontend/messages/hi.json
- frontend/src/lib/components/Forms/ModelForm.svelte
- backend/core/migrations/0091_qualification_library_and_more.py
🧰 Additional context used
🧬 Code graph analysis (2)
backend/core/models.py (3)
backend/library/helpers.py (1)
update_translations_in_object(49-75)backend/ebios_rm/models.py (1)
FearedEvent(248-313)backend/resilience/models.py (1)
EscalationThreshold(176-248)
backend/library/utils.py (1)
backend/core/models.py (3)
Qualification(1431-1596)LoadedLibrary(849-971)create(2987-2989)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: build (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: build_enterprise_frontend
- GitHub Check: build_community_frontend
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: test (3.12)
- GitHub Check: startup-functional-test (3.12)
🔇 Additional comments (11)
frontend/src/routes/(app)/(internal)/[model=urlmodel]/+page.svelte (1)
42-43: i18n key check for add-qualificationThis composes a key from model.localName. For "qualification", ensure the key add-qualification exists across locales to avoid showing the raw key in the UI.
You can reuse the script in my first comment; it flags the key if missing. If it’s not present everywhere yet, I can generate a patch for the locale files.
backend/core/models.py (5)
887-891: Good addition: surface qualifications in LoadedLibrary._objects.Serializing related qualifications with translations keeps parity with threats/reference_controls. Meta.ordering on Qualification will give a stable order here.
909-910: Lazy imports to avoid circular deps are fine here.Importing FearedEvent/EscalationThreshold within the property keeps module load safe. No action needed.
998-1000: Docstring clarity is fine.The updated description accurately reflects the method’s intent.
1550-1559: Qualification.library FK and is_published: sensible defaults.
- CASCADE aligns with library-scoped referentials.
- null/blank True keeps room for global/default qualifications.
1575-1575: fields_to_check looks appropriate.Refers to stable identifiers used across the codebase.
backend/library/utils.py (5)
22-23: Expose Qualification in import surface.Required for the new importer; looks correct.
498-505: Adding "qualifications" to OBJECT_FIELDS is consistent with the new importer.Keeps backward-compat logic intact for other fields.
516-517: Initialize _qualifications list.Matches patterns used for other object types.
575-596: init_qualifications: validation pipeline is consistent and error messaging is clear.Mirrors threats/reference_controls init with indexed error reporting.
836-838: Persisting qualifications during import: LGTM.Import order (threats → reference_controls → qualifications → risk_matrices → frameworks → mappings) is acceptable; no dependency issues.
| + Incident.objects.filter(qualifications__library=self).count() | ||
| + FearedEvent.objects.filter(qualifications__library=self).count() | ||
| + EscalationThreshold.objects.filter(qualifications__library=self).count() | ||
| + LoadedLibrary.objects.filter(dependencies=self).distinct().count() |
There was a problem hiding this comment.
reference_count may over-count due to M2M duplicates — use distinct().
Counting Incidents/FearedEvents/EscalationThresholds that reference any Qualification from this library via M2M can produce duplicate rows in the join. Align with the earlier distinct() usage in this method.
Apply this diff:
- + Incident.objects.filter(qualifications__library=self).count()
- + FearedEvent.objects.filter(qualifications__library=self).count()
- + EscalationThreshold.objects.filter(qualifications__library=self).count()
+ + Incident.objects.filter(qualifications__library=self).distinct().count()
+ + FearedEvent.objects.filter(qualifications__library=self).distinct().count()
+ + EscalationThreshold.objects.filter(qualifications__library=self).distinct().count()📝 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.
| + Incident.objects.filter(qualifications__library=self).count() | |
| + FearedEvent.objects.filter(qualifications__library=self).count() | |
| + EscalationThreshold.objects.filter(qualifications__library=self).count() | |
| + LoadedLibrary.objects.filter(dependencies=self).distinct().count() | |
| Incident.objects.filter(qualifications__library=self).distinct().count() | |
| FearedEvent.objects.filter(qualifications__library=self).distinct().count() | |
| EscalationThreshold.objects.filter(qualifications__library=self).distinct().count() | |
| LoadedLibrary.objects.filter(dependencies=self).distinct().count() |
🤖 Prompt for AI Agents
In backend/core/models.py around lines 928 to 931, the reference_count summation
may over-count because the Incident, FearedEvent, and EscalationThreshold
queries join through Qualifications (M2M) and can return duplicate rows; modify
each of those three queryset calls to apply distinct() before count() (matching
the existing LoadedLibrary.distinct().count()) so duplicates from the M2M join
are eliminated and counts are accurate.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
frontend/messages/da.json (1)
1248-1251: New Danish translations look good; consider slightly lighter phrasing for “ordering”Current compounds are correct but a bit heavy. Optional tweak for readability:
- "qualificationOrdering": "Kvalifikationsrækkefølge", - "securityObjectiveOrdering": "Sikkerhedsmålsrækkefølge", + "qualificationOrdering": "Rækkefølge af kvalifikationer", + "securityObjectiveOrdering": "Rækkefølge af sikkerhedsmål",frontend/messages/sv.json (1)
956-959: Align terminology for “qualification” across Swedish stringsExisting key "qualification" (Line 121) is translated as “Kvalificering”, but new strings use “Kvalifikation”. Choose one term and apply consistently.
Option A (align to existing “Kvalificering”):
"abbreviation": "Förkortning", -"qualificationOrdering": "Kvalifikationsordning", -"securityObjectiveOrdering": "Säkerhetsmålsordning", -"addQualification": "Lägg till kvalifikation" +"qualificationOrdering": "Kvalificeringsordning", +"securityObjectiveOrdering": "Säkerhetsmålens ordning", +"addQualification": "Lägg till kvalificering"Option B (switch to “Kvalifikation” globally): keep these lines as-is and update Line 121 elsewhere in this file to:
"qualification": "Kvalifikation"If adopting Option B, confirm no user-facing copy relies on the “process of qualifying” meaning. Otherwise, Option A is safer.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (25)
backend/core/models.py(7 hunks)backend/core/serializers.py(1 hunks)frontend/messages/ar.json(1 hunks)frontend/messages/cs.json(1 hunks)frontend/messages/da.json(1 hunks)frontend/messages/de.json(1 hunks)frontend/messages/el.json(1 hunks)frontend/messages/en.json(1 hunks)frontend/messages/es.json(1 hunks)frontend/messages/fr.json(1 hunks)frontend/messages/hi.json(1 hunks)frontend/messages/hu.json(1 hunks)frontend/messages/id.json(1 hunks)frontend/messages/it.json(1 hunks)frontend/messages/nl.json(1 hunks)frontend/messages/pl.json(1 hunks)frontend/messages/pt.json(1 hunks)frontend/messages/ro.json(1 hunks)frontend/messages/sv.json(1 hunks)frontend/messages/tr.json(1 hunks)frontend/messages/uk.json(1 hunks)frontend/messages/ur.json(1 hunks)frontend/src/lib/utils/crud.ts(1 hunks)frontend/src/lib/utils/schemas.ts(2 hunks)frontend/src/lib/utils/table.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (23)
- frontend/messages/ro.json
- frontend/messages/hi.json
- frontend/src/lib/utils/table.ts
- frontend/messages/es.json
- frontend/messages/de.json
- frontend/messages/nl.json
- frontend/messages/uk.json
- frontend/messages/cs.json
- frontend/messages/id.json
- frontend/messages/en.json
- frontend/src/lib/utils/crud.ts
- frontend/src/lib/utils/schemas.ts
- frontend/messages/tr.json
- frontend/messages/hu.json
- frontend/messages/pl.json
- backend/core/models.py
- frontend/messages/ar.json
- frontend/messages/pt.json
- backend/core/serializers.py
- frontend/messages/el.json
- frontend/messages/fr.json
- frontend/messages/it.json
- frontend/messages/ur.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). (5)
- GitHub Check: startup-docker-compose-test
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: build_community_frontend
- GitHub Check: build_enterprise_frontend
🔇 Additional comments (2)
frontend/messages/da.json (1)
1247-1247: Comma after deselect is correct; JSON remains validNo issues. This enables adding the following keys cleanly.
frontend/messages/sv.json (1)
955-955: JSON Syntax Check PassedAll locale JSON files, including frontend/messages/sv.json, parse successfully with no errors. The trailing comma after
fearedEventAssetHelpTextis already present, so no fix is needed and the file is valid JSON.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
backend/core/migrations/0092_qualification_library_and_more.py (2)
13-23: Confirm cascade semantics; consider SET_NULL to preserve Qualifications on library deletionCurrent on_delete=CASCADE will delete Qualifications when a LoadedLibrary is removed. Given the field is nullable (blank=True, null=True) and Qualifications are referenced by EscalationThresholds, verify that cascading deletes are desired. If you prefer to retain Qualifications (and simply break the link), switch to SET_NULL.
- on_delete=django.db.models.deletion.CASCADE, + on_delete=django.db.models.deletion.SET_NULL,
24-28: Make the default DB-backed; optionally backfill if policy changesIf the intent is “new Qualifications default to published,” add db_default=True so inserts outside the ORM (and ORM inserts avoiding explicit values) are consistent and cheaper. If historical rows need flipping, add a data migration; otherwise leave as-is.
- field=models.BooleanField(default=True, verbose_name="published"), + field=models.BooleanField(default=True, db_default=True, verbose_name="published"),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
backend/core/migrations/0092_qualification_library_and_more.py(1 hunks)backend/resilience/migrations/0003_alter_escalationthreshold_qualifications.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/resilience/migrations/0003_alter_escalationthreshold_qualifications.py
🧰 Additional context used
🧬 Code graph analysis (1)
backend/core/migrations/0092_qualification_library_and_more.py (1)
backend/resilience/migrations/0003_alter_escalationthreshold_qualifications.py (1)
Migration(6-22)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: build_enterprise_frontend
- GitHub Check: build_community_frontend
- GitHub Check: build (3.12)
- GitHub Check: test (3.12)
- GitHub Check: Analyze (python)
🔇 Additional comments (1)
backend/core/migrations/0092_qualification_library_and_more.py (1)
8-10: Dependencies look correctDepends on core.0091 and unblocks resilience migration that targets this field. No issues.
Mohamed-Hacene
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
.github/workflows/backend-coverage.yaml (2)
49-55: Optional: add Poetry caching to speed up CI.Cache Poetry downloads and the virtualenv to cut minutes off installs.
- uses: actions/checkout@v3 + - name: Cache Poetry and virtualenv + uses: actions/cache@v4 + with: + path: | + ~/.cache/pypoetry + ./.venv + key: ${{ runner.os }}-poetry-${{ hashFiles('backend/poetry.lock') }} + restore-keys: | + ${{ runner.os }}-poetry- - name: Install Poetry via pipx shell: bash run: | set -euo pipefail python -m pip install --upgrade pip python -m pip install --upgrade pipx python -m pipx ensurepath echo "$HOME/.local/bin" >> "$GITHUB_PATH" python -m pipx install poetry - name: Install backend requirements working-directory: ${{ env.backend-directory }} - run: poetry install + env: + POETRY_VIRTUALENVS_IN_PROJECT: "true" + run: poetry install --no-interaction --no-ansiAlso applies to: 67-69
78-83: Optional: generate/upload coverage XML for PR tooling.If you plan to gate PRs or integrate with Codecov/GitHub summary, emit XML and upload.
- name: Display coverage report working-directory: ${{ env.backend-directory }} env: - DATABASE_URL: "postgres://postgres:postgres@localhost:${{ job.services.postgres.ports[5432] }}/postgres" - run: poetry run coverage report + DATABASE_URL: "postgres://postgres:postgres@localhost:${{ job.services.postgres.ports['5432'] }}/postgres" + run: | + poetry run coverage report + poetry run coverage xml + - name: Upload coverage artifact + uses: actions/upload-artifact@v4 + with: + name: backend-coverage-xml + path: backend/coverage.xml
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.github/workflows/backend-coverage.yaml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.7)
.github/workflows/backend-coverage.yaml
81-81: property access of object must be type of string but got "number"
(expression)
⏰ 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). (6)
- GitHub Check: build_enterprise_frontend
- GitHub Check: build_community_frontend
- GitHub Check: build (3.12)
- GitHub Check: test (3.12)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
.github/workflows/backend-coverage.yaml (1)
77-77: Good change: run tests via Poetry and simplify coverage commands.Using
poetry runwith pytest/coverage is cleaner and leverages default test discovery. No issues spotted.Also applies to: 82-82
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
frontend/src/routes/(app)/(third-party)/requirement-assessments/[id=uuid]/edit/+page.server.ts (3)
195-213: Do not send noRedirect to the backend; set JSON header; throw redirect.noRedirect is a frontend-only flag; sending it likely breaks DRF serializers. Also add Content-Type and use throw redirect.
- const formData = form.data; + const { noRedirect, ...payload } = form.data; const requestInitOptions: RequestInit = { method: 'PUT', - body: JSON.stringify(formData) + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify(payload) }; const response = await event.fetch(endpoint, requestInitOptions); if (!response.ok) return handleErrorResponse({ event, response, form }); const object = await response.json(); const model: string = urlParamModelVerboseName(URLModel); setFlash({ type: 'success', message: m.successfullySavedObject({ object: model }) }, event); - if (formData.noRedirect) return; - redirect( + if (noRedirect) return; + throw redirect( 302, getSecureRedirect(event.url.searchParams.get('next')) || `/compliance-assessments/${object.compliance_assessment}/` );
90-92: Fix undefined identifier: response is not in scope.The else branch logs response.statusText but response doesn’t exist here.
- } else { - console.error(`Failed to fetch data for ${selectField.field}: ${response.statusText}`); - } + } else { + console.error(`Failed to fetch data for ${selectField.field} from ${url}`); + }
48-51: Guard against undefined evidences.If evidences is null/undefined, .map will throw.
- object.evidences = object.evidences.map((evidence) => evidence.id); + object.evidences = (object.evidences ?? []).map((evidence) => evidence.id);frontend/src/routes/(app)/(internal)/backup-restore/+page.svelte (1)
71-91: Prevent modal without a selected file and avoid duplicate submissions
if (file) modalStore.trigger(modal);only checks element existence. Gate on an actual selection and focus the input if empty.- function modalConfirm(): void { - if (isUploading) return; // Prevent multiple clicks + function modalConfirm(): void { + if (isUploading) return; // Prevent multiple clicks + if (!selectedFiles?.length) { + file?.focus(); + return; + } @@ - if (file) modalStore.trigger(modal); + modalStore.trigger(modal); }frontend/src/lib/components/Forms/AutocompleteSelect.svelte (2)
221-227: Do not unconditionally drop the last path segment.Unconditional path.pop() removes a legitimate segment when there is no duplicate, causing incorrect breadcrumbs and sorting. Make it conditional against the actual label/name.
Apply:
- const path: string[] = - pathField && object?.[pathField] - ? object[pathField].map((obj: { str: string; id: string }) => { - return obj.str; - }) - : []; - path.pop(); // remove duplicate last part if it exists + const pathObjs = pathField && Array.isArray(object?.[pathField]) ? object[pathField] : []; + const path: string[] = pathObjs.map((obj: { str: string; id: string }) => obj.str); + // Remove duplicate last segment only if it matches the label/name + const last = path.at(-1); + const labelForCheck = + optionsLabelField === 'auto' + ? (object?.name || object?.description || '').toString() + : String(getNestedValue(object, optionsLabelField) ?? ''); + if (last && last.trim && (last.trim() === labelForCheck.trim() || last.trim() === String(object?.name ?? '').trim())) { + path.pop(); + }
235-244: Type-safety: avoid spreading optionsInfoFields into infoString.Spreading adds unknown keys (fields, separator) to infoString, violating the declared Option.infoString shape and likely TS excess property checks. Assign only supported keys.
- let infoString: { string: string; position: 'suffix' | 'prefix' } | undefined = undefined; + let infoString: + | { string: string; position: 'suffix' | 'prefix'; classes?: string } + | undefined = undefined; if (infoFields.length > 0) { - const separator = optionsInfoFields.separator || ' '; - const position = optionsInfoFields.position || 'suffix'; - infoString = { - string: infoFields.join(separator), - position: position, - ...optionsInfoFields - }; + const separator = optionsInfoFields.separator ?? ' '; + const position = optionsInfoFields.position ?? 'suffix'; + infoString = { + string: infoFields.join(separator), + position, + classes: optionsInfoFields.classes + }; }
♻️ Duplicate comments (2)
frontend/messages/da.json (1)
1247-1252: Reconfirm usage of addQualification.Per the earlier verification note, this key was unused across the UI. Either wire it to the intended button/CTA or remove it from all locales if not needed.
.github/workflows/backend-coverage.yaml (1)
76-83: LGTM: fixed port indexing and moved coverage under Poetry.The ports['5432'] change resolves actionlint; running coverage via Poetry is cleaner.
🧹 Nitpick comments (11)
frontend/messages/pt.json (3)
962-962: Align with existing terminology: use “valores de negócio”Elsewhere “businessValue” is translated as “Valor de negócio”. Recommend keeping the same noun phrase here.
Apply this diff:
-"fearedEventAssetHelpText": "Os eventos temidos são geralmente vinculados a valores empresariais (ativos primários). Ativos de suporte são permitidos aqui para flexibilidade.", +"fearedEventAssetHelpText": "Os eventos temidos são geralmente vinculados a valores de negócio (ativos primários). Ativos de suporte são permitidos aqui para flexibilidade.",
965-966: Make wording consistent: use articles in both labels“securityObjectiveOrdering” uses “dos …”; mirror that for “qualificationOrdering”.
Apply this diff:
-"qualificationOrdering": "Ordenação de qualificações", +"qualificationOrdering": "Ordenação das qualificações",
964-964: Consider “Sigla” instead of “Abreviação”“Sigla” is the more common UI label in PT-BR for short forms; optional change.
Apply this diff:
-"abbreviation": "Abreviação", +"abbreviation": "Sigla",frontend/src/routes/(app)/(third-party)/requirement-assessments/[id=uuid]/edit/+page.svelte (1)
642-649: Nit: Prefer Svelte’s on:click over onclick.Keeps code idiomatic and avoids assigning DOM properties directly.
- onclick={() => form.form.update((data) => { return { ...data, noRedirect: true }; })} + on:click={() => form.form.update((data) => ({ ...data, noRedirect: true }))}Apply similarly to the “Save and continue” button.
frontend/src/routes/(app)/(internal)/backup-restore/+page.svelte (4)
46-67: Enhance export: honor server filename, set credentials, minor a11yUse Content-Disposition filename when present and pass credentials explicitly. Also set a safer default filename on fallback.
- const response = await fetch('/backup-restore/dump-db/'); + const response = await fetch('/backup-restore/dump-db/', { + method: 'GET', + credentials: 'same-origin', + headers: { Accept: 'application/octet-stream' } + }); @@ - const blob = await response.blob(); - const filename = `backup-${new Date().toISOString().split('T')[0]}.bak`; + const blob = await response.blob(); + const cd = response.headers.get('content-disposition') ?? ''; + const match = cd.match(/filename\*?=(?:UTF-8''|")?([^\";]+)\"?/i); + const filename = match?.[1] ?? `backup-${new Date().toISOString().split('T')[0]}.bak`;Optional: add
aria-busy={isExporting}to the export button for accessibility.
108-122: Nit: simplify i18n usage and add aria-busy
m.exporting ? m.exporting() : 'Exporting...'is redundant if the key exists in messages. Also expose busy state for a11y.- <button + <button type="button" class="btn preset-filled-primary-500 {exportButtonStyles}" - disabled={isExporting} + disabled={isExporting} + aria-busy={isExporting} onclick={handleExport} > {#if isExporting} <i class="fa-solid fa-spinner fa-spin"></i> - {m.exporting ? m.exporting() : 'Exporting...'} + {m.exporting()} {:else} {m.exportDatabase()} {/if} </button>
130-167: Reset uploading state on failure and set aria-busy on Upload buttonEnsure
isUploadingresets even if the enhanceupdate()throws; expose busy to AT.- use:enhance={() => { - return async ({ update }) => { - await update(); - isUploading = false; - }; - }} + use:enhance={() => { + return async ({ update }) => { + try { + await update(); + } finally { + isUploading = false; + } + }; + }} @@ - <button + <button class="btn preset-filled-secondary-500 {uploadButtonStyles}" type="button" - disabled={isUploading} + disabled={isUploading || !selectedFiles?.length} + aria-busy={isUploading} onclick={modalConfirm} >
96-99: Permission check: prefer truthy flag over hasOwn
Object.hasOwn(user.permissions, 'backup')passes even ifbackup: false. Use the boolean flag directly.-const canBackup: boolean = Object.hasOwn(user.permissions, 'backup'); +const canBackup: boolean = !!user.permissions?.backup;frontend/src/lib/components/Forms/ModelForm/ReferenceControlForm.svelte (1)
68-68: pathField applied to folder selectorLooks good. Optional: the folders endpoint string is duplicated across forms; consider centralizing it in a constants module to prevent drift.
frontend/src/lib/components/Forms/AutocompleteSelect.svelte (2)
269-276: Prefer path-aware sort over contentType check.Sorting only when both options have contentType can skip correct ordering for data that carries path but no contentType. Sort when both have a non-empty path.
- // Sort folder by path - if (a.contentType && b.contentType) { - const aPath = a.path.join('') + a.label; - const bPath = b.path.join('') + b.label; + // Sort by path when both provide it + if ((a.path?.length ?? 0) > 0 && (b.path?.length ?? 0) > 0) { + const aPath = a.path.join('/') + '/' + a.label; + const bPath = b.path.join('/') + '/' + b.label; const alphaCompare = aPath.localeCompare(bPath); - return alphaCompare !== 0 ? alphaCompare : aPath.length - bPath.length; + return alphaCompare !== 0 ? alphaCompare : a.path.length - b.path.length; }
17-24: Minor: extract InfoString type for reuse.Define a reusable type and reference it from Option and local variables to keep shapes in sync.
interface Option { label: string; value: string | number; suggested?: boolean; translatedLabel?: string; - path?: string[]; - infoString?: { - string: string; - position: 'suffix' | 'prefix'; - classes?: string; - }; + path?: string[]; + infoString?: InfoString; contentType?: string; } + +type InfoString = { + string: string; + position: 'suffix' | 'prefix'; + classes?: string; +};
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (45)
.github/workflows/backend-coverage.yaml(1 hunks)enterprise/frontend/src/lib/components/Forms/ModelForm/EbiosRmForm.svelte(1 hunks)enterprise/frontend/src/lib/components/Forms/ModelForm/FolderForm.svelte(1 hunks)frontend/messages/ar.json(1 hunks)frontend/messages/cs.json(1 hunks)frontend/messages/da.json(1 hunks)frontend/messages/de.json(1 hunks)frontend/messages/el.json(1 hunks)frontend/messages/en.json(2 hunks)frontend/messages/es.json(1 hunks)frontend/messages/fr.json(3 hunks)frontend/messages/hi.json(1 hunks)frontend/messages/hu.json(1 hunks)frontend/messages/id.json(1 hunks)frontend/messages/it.json(1 hunks)frontend/messages/nl.json(1 hunks)frontend/messages/pl.json(1 hunks)frontend/messages/pt.json(1 hunks)frontend/messages/ro.json(1 hunks)frontend/messages/sv.json(1 hunks)frontend/messages/tr.json(1 hunks)frontend/messages/uk.json(1 hunks)frontend/messages/ur.json(1 hunks)frontend/src/lib/components/Forms/AutocompleteSelect.svelte(5 hunks)frontend/src/lib/components/Forms/ModelForm/AppliedControlPolicyForm.svelte(1 hunks)frontend/src/lib/components/Forms/ModelForm/AssetForm.svelte(1 hunks)frontend/src/lib/components/Forms/ModelForm/CampaignForm.svelte(1 hunks)frontend/src/lib/components/Forms/ModelForm/EbiosRmForm.svelte(1 hunks)frontend/src/lib/components/Forms/ModelForm/ElementaryActionForm.svelte(1 hunks)frontend/src/lib/components/Forms/ModelForm/EntityForm.svelte(1 hunks)frontend/src/lib/components/Forms/ModelForm/EvidenceForm.svelte(1 hunks)frontend/src/lib/components/Forms/ModelForm/FearedEventForm.svelte(1 hunks)frontend/src/lib/components/Forms/ModelForm/IncidentForm.svelte(1 hunks)frontend/src/lib/components/Forms/ModelForm/PerimeterForm.svelte(1 hunks)frontend/src/lib/components/Forms/ModelForm/ProcessingForm.svelte(1 hunks)frontend/src/lib/components/Forms/ModelForm/ReferenceControlForm.svelte(1 hunks)frontend/src/lib/components/Forms/ModelForm/RiskAcceptanceForm.svelte(1 hunks)frontend/src/lib/components/Forms/ModelForm/SecurityExceptionForm.svelte(1 hunks)frontend/src/lib/components/Forms/ModelForm/TaskTemplateForm.svelte(1 hunks)frontend/src/lib/components/Forms/ModelForm/ThreatForm.svelte(1 hunks)frontend/src/lib/components/Forms/ModelForm/VulnerabilitiesForm.svelte(1 hunks)frontend/src/lib/utils/schemas.ts(4 hunks)frontend/src/routes/(app)/(internal)/backup-restore/+page.svelte(3 hunks)frontend/src/routes/(app)/(third-party)/requirement-assessments/[id=uuid]/edit/+page.server.ts(2 hunks)frontend/src/routes/(app)/(third-party)/requirement-assessments/[id=uuid]/edit/+page.svelte(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (17)
- frontend/messages/hi.json
- frontend/messages/id.json
- frontend/messages/es.json
- frontend/messages/ro.json
- frontend/messages/pl.json
- frontend/messages/el.json
- frontend/messages/fr.json
- frontend/messages/sv.json
- frontend/messages/uk.json
- frontend/messages/hu.json
- frontend/messages/ur.json
- frontend/messages/cs.json
- frontend/messages/it.json
- frontend/messages/tr.json
- frontend/messages/nl.json
- frontend/messages/ar.json
- frontend/messages/de.json
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/lib/utils/schemas.ts (1)
backend/core/base_models.py (1)
NameDescriptionMixin(111-124)
🪛 Checkov (3.2.334)
.github/workflows/backend-coverage.yaml
[MEDIUM] 76-77: Basic Auth Credentials
(CKV_SECRET_4)
🔇 Additional comments (21)
frontend/messages/da.json (1)
1247-1252: Danish translations and JSON update look correct."deselect", "saveAndContinue", "abbreviation", "qualificationOrdering", "securityObjectiveOrdering", and "addQualification" read well and match existing naming patterns. JSON structure remains valid.
frontend/messages/en.json (2)
1827-1832: LGTM: new qualification-related strings
daysOfWeekErrorMessage,weeksOfMonthErrorMessage,abbreviation,qualificationOrdering,securityObjectiveOrdering, andaddQualificationare clear and consistent with existing style.
1805-1806: Keep qualificationsChartTitle: it’s used in frontend/src/routes/(app)/(internal)/analytics/+page.svelte at line 479; no removal needed.saveAndContinueremains valid.frontend/messages/pt.json (1)
963-963: LGTM — keys added correctly“saveAndContinue” and “addQualification” read well and match existing action-label style.
Also applies to: 967-967
enterprise/frontend/src/lib/components/Forms/ModelForm/FolderForm.svelte (1)
51-56: Add path field availability check for AutocompleteSelect.If the folders API doesn’t include a path array by default, this prop will have no effect. Ensure the endpoint returns path for options, or pass the extra field explicitly.
Optionally:
<AutocompleteSelect {form} optionsEndpoint="folders?content_type=DO&content_type=GL" optionsSelf={object} field="parent_folder" pathField="path" + optionsExtraFields={[['path']]} cacheLock={cacheLocks['parent_folder']} bind:cachedValue={formDataCache['parent_folder']} label={m.parentDomain()} hide={initialData.parent_folder} />frontend/src/lib/utils/schemas.ts (1)
881-922: Cross-field validation looks solid.Guards MONTHLY/YEARLY schedules correctly and allows others to pass.
frontend/src/lib/components/Forms/ModelForm/ThreatForm.svelte (1)
30-31: LGTM: path-aware folder selectPassing
pathField="path"aligns with the AutocompleteSelect path support introduced in this PR.Please confirm:
- AutocompleteSelect.svelte exposes a typed
pathField?: string.- The
foldersendpoint includes apathfield in its items; if not, addoptionsExtraFields={[['path','str']]}here.frontend/src/lib/components/Forms/ModelForm/RiskAcceptanceForm.svelte (1)
53-54: LGTM: consistent pathField usage for foldersConsistent with other forms; no functional concerns.
Verify the
folders?content_type=DOresponse includespath; otherwise addoptionsExtraFieldsto fetch it.frontend/src/lib/components/Forms/ModelForm/VulnerabilitiesForm.svelte (1)
30-31: LGTM: pathField addedMatches the AutocompleteSelect API update.
Confirm the
foldersoptions includepathby default; if not, appendoptionsExtraFields={[['path','str']]}.frontend/src/lib/components/Forms/ModelForm/PerimeterForm.svelte (1)
30-31: LGTM: folder AutocompleteSelect now path-awareChange is minimal and consistent across the app.
Ensure backend responses for folders include
path; otherwise passoptionsExtraFieldsto fetch it.frontend/src/lib/components/Forms/ModelForm/TaskTemplateForm.svelte (1)
54-54: pathField wired for folder AutocompleteSelectLooks correct and consistent with the new AutocompleteSelect API. Please confirm the folders endpoint returns a path array (or that the component maps it accordingly) so the path segments render as expected.
frontend/src/lib/components/Forms/ModelForm/AssetForm.svelte (1)
116-116: Good: pathField added to folder selectorMatches the pattern already used for parent_assets; should enable path-aware rendering for folder too.
frontend/src/lib/components/Forms/ModelForm/FearedEventForm.svelte (1)
40-40: pathField set on folder selectorLooks fine. If AutocompleteSelect doesn’t infer the folders endpoint for this field, consider aligning with other forms by passing optionsEndpoint explicitly.
frontend/src/lib/components/Forms/ModelForm/EvidenceForm.svelte (1)
63-63: pathField added to folder AutocompleteSelectChange is straightforward and consistent with the broader update.
frontend/src/lib/components/Forms/ModelForm/AppliedControlPolicyForm.svelte (1)
279-279: LGTM: pathField for folder selection is consistent with API changesNo issues. Ensure backend items expose "path" as expected.
frontend/src/lib/components/Forms/ModelForm/SecurityExceptionForm.svelte (1)
88-88: LGTM: pathField added for folder AutocompleteSelectMatches the new AutocompleteSelect contract. Confirm the endpoint returns a "path" field (array/string as expected by the component).
frontend/src/lib/components/Forms/ModelForm/EbiosRmForm.svelte (1)
70-70: LGTM: pathField="path" for folder mappingAligned with the standardized folder selection behavior. Validate the DO-only folders endpoint includes "path".
frontend/src/lib/components/Forms/ModelForm/EntityForm.svelte (1)
45-45: LGTM: pathField parameterization for folder fieldConsistent with the AutocompleteSelect update; no further changes needed.
enterprise/frontend/src/lib/components/Forms/ModelForm/EbiosRmForm.svelte (1)
69-69: Consistent path-aware folder selection.Matches the project-wide shift to pathField="path".
frontend/src/lib/components/Forms/ModelForm/IncidentForm.svelte (1)
67-71: Confirm backendfoldersendpoint returnspatharray for DO & GL
In IncidentForm.svelte,pathField="path"andoptionsEndpoint="folders?content_type=DO&content_type=GL"are correctly applied. Verify that thefolders?content_type=DO&content_type=GLAPI response includes thepatharray for both DO and GL entries.frontend/src/lib/components/Forms/ModelForm/CampaignForm.svelte (1)
113-119: Good: folder AutocompleteSelect now path-aware.No further changes needed here.
frontend/src/lib/components/Forms/ModelForm/ElementaryActionForm.svelte
Outdated
Show resolved
Hide resolved
| @@ -60,6 +60,7 @@ | |||
| {form} | |||
| optionsEndpoint="folders?content_type=DO&content_type=GL" | |||
| field="folder" | |||
| pathField="path" | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Approve: pathField wiring; verify API shape and call-site coverage
Looks good and consistent with the new AutocompleteSelect API. Please confirm the folders endpoints include a "path" field in the returned option items, and that all folder AutocompleteSelect usages across the repo now pass pathField to avoid inconsistent UX.
Run this to verify the component prop and catch any remaining folder selects missing pathField:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) Ensure AutocompleteSelect declares the prop"
rg -nP 'export\s+let\s+pathField' --type svelte
echo "2) Find AutocompleteSelect folder fields missing pathField"
python - <<'PY'
import re, subprocess, sys
files = subprocess.check_output(["fd","-t","f","-e","svelte"], text=True).splitlines()
bad = []
for f in files:
s = open(f, encoding="utf-8", errors="ignore").read()
for m in re.finditer(r'<AutocompleteSelect\b[\s\S]*?/>', s, flags=re.S):
tag = m.group(0)
if 'field="folder"' in tag and 'pathField=' not in tag:
bad.append(f); break
if bad:
print("Missing pathField on folder AutocompleteSelect in:"); [print(" -", f) for f in bad]; sys.exit(1)
print("All folder AutocompleteSelect usages include pathField.")
PYLength of output: 203
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) Ensure AutocompleteSelect declares the prop"
rg -nP 'export\s+let\s+pathField' -g '*.svelte'
echo "2) Find AutocompleteSelect folder usages missing pathField"
python - <<'PY'
import re, subprocess, sys
# find all .svelte files
files = subprocess.check_output(["fd", "-e", "svelte"], text=True).splitlines()
bad = []
for f in files:
try:
content = open(f, encoding="utf-8", errors="ignore").read()
except Exception:
continue
for m in re.finditer(r'<AutocompleteSelect\b[\s\S]*?/>', content, flags=re.S):
tag = m.group(0)
if 'field="folder"' in tag and 'pathField=' not in tag:
bad.append(f)
break
if bad:
echo="Missing pathField on folder AutocompleteSelect in:"
print(echo)
for f in bad:
print(" -", f)
sys.exit(1)
print("All folder AutocompleteSelect usages include pathField.")
PYLength of output: 172
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) Locate AutocompleteSelect component file"
component_file=$(fd -e svelte -I | grep -i 'AutocompleteSelect\.svelte' || true)
if [ -z "$component_file" ]; then
echo "ERROR: AutocompleteSelect.svelte not found"; exit 1
fi
echo "Found: $component_file"
echo "2) Verify 'export let pathField' is declared"
if ! rg -nP 'export\s+let\s+pathField' "$component_file"; then
echo "ERROR: 'pathField' prop not declared in $component_file"; exit 1
fi
echo "3) Identify all AutocompleteSelect usages with field=\"folder\""
usages=$(rg -l '<AutocompleteSelect[^>]*field="folder"' -g '*.svelte')
if [ -z "$usages" ]; then
echo "No folder-field AutocompleteSelect usages found"
else
missing=()
for f in $usages; do
if ! rg -q 'pathField=' "$f"; then
missing+=("$f")
fi
done
if [ ${#missing[@]} -gt 0 ]; then
echo "Missing pathField in the following files:"
printf ' - %s\n' "${missing[@]}"
exit 1
else
echo "All folder-field AutocompleteSelect usages include pathField"
fi
fiLength of output: 893
Add missing export let pathField prop in AutocompleteSelect.svelte
In frontend/src/lib/components/Forms/AutocompleteSelect.svelte, declare export let pathField so that folder-field <AutocompleteSelect> usages can bind the pathField prop. After adding it, verify all <AutocompleteSelect field="folder" …> instances include pathField.
🤖 Prompt for AI Agents
In frontend/src/lib/components/Forms/AutocompleteSelect.svelte (near the top of
the file, lines ~1–5) add a prop declaration "export let pathField" so the
component accepts a pathField prop; then scan the codebase for all
<AutocompleteSelect field="folder" …> usages and ensure they pass the pathField
prop (e.g., pathField="path" or bind:pathField where appropriate), specifically
confirm frontend/src/lib/components/Forms/ModelForm/ProcessingForm.svelte line
63 includes pathField="path".
| let uploadButtonStyles = $derived(file && !isUploading ? '' : 'chip-disabled'); | ||
| let exportButtonStyles = $derived(isExporting ? 'chip-disabled' : ''); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Correct the upload button gating to check for an actual selected file
Current logic enables the button if the input element exists, not if a file is selected.
-let uploadButtonStyles = $derived(file && !isUploading ? '' : 'chip-disabled');
+let uploadButtonStyles = $derived(!!(selectedFiles?.length) && !isUploading ? '' : 'chip-disabled');📝 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.
| let uploadButtonStyles = $derived(file && !isUploading ? '' : 'chip-disabled'); | |
| let exportButtonStyles = $derived(isExporting ? 'chip-disabled' : ''); | |
| let uploadButtonStyles = $derived(!!(selectedFiles?.length) && !isUploading ? '' : 'chip-disabled'); | |
| let exportButtonStyles = $derived(isExporting ? 'chip-disabled' : ''); |
🤖 Prompt for AI Agents
In frontend/src/routes/(app)/(internal)/backup-restore/+page.svelte around lines
93–95, the upload button gating currently enables based on the input element
existence instead of whether a file is actually selected; change the derived
condition to check the selected file (e.g., file && (file.length ?? 1) > 0 or
file instanceof File depending on how `file` is represented) so the button is
only enabled when a real file is present, and update the derived store
expression accordingly to depend on the correct file property.
| <form | ||
| enctype="multipart/form-data" | ||
| method="post" | ||
| use:enhance={() => { | ||
| return async ({ update }) => { | ||
| await update(); | ||
| isUploading = false; | ||
| }; | ||
| }} | ||
| bind:this={form} | ||
| > | ||
| <div class="flex flex-col sm:flex-row sm:items-end gap-3"> | ||
| <div class="flex-1"> | ||
| <input | ||
| id="file" | ||
| type="file" | ||
| name="file" | ||
| class="input" | ||
| accept={authorizedExtensions.join(',')} | ||
| required | ||
| bind:value={file} | ||
| /> |
There was a problem hiding this comment.
Fix: file input is mis-bound; use bind:files and/or bind:this, not bind:value to an HTMLInputElement
bind:value={file} on an <input type="file"> is incorrect and currently binds a string path, not the selected FileList nor the element. This breaks the upload gating logic downstream.
Apply this diff within the form to correctly bind the element and selected files:
- accept={authorizedExtensions.join(',')}
- required
- bind:value={file}
+ accept={authorizedExtensions.join(',')}
+ required
+ bind:this={file}
+ bind:files={selectedFiles}And add this state near the other state declarations (outside the shown hunk):
let selectedFiles: FileList | null = $state(null);🤖 Prompt for AI Agents
In frontend/src/routes/(app)/(internal)/backup-restore/+page.svelte around lines
130-151, the file input incorrectly uses bind:value which binds a string path
instead of the FileList or element; replace bind:value with bind:files to
capture the selected FileList and optionally add bind:this if you need a
reference to the input element, then update any upload-gating logic to use that
selected FileList (not a string). Also add a state variable near the other state
declarations such as let selectedFiles: FileList | null = null so the bind:files
target exists and downstream checks use selectedFiles.
| data-testid="save-no-continue-button" | ||
| type="submit" | ||
| onclick={() => | ||
| form.form.update((data) => { | ||
| return { ...data, noRedirect: true }; | ||
| })}>{m.save()}</button | ||
| > | ||
| <button | ||
| class="btn preset-filled-secondary-500 font-semibold w-full" | ||
| data-testid="save-button" | ||
| type="submit">{m.save()}</button | ||
| type="submit">{m.saveAndContinue()}</button | ||
| > |
There was a problem hiding this comment.
Prevent sticky noRedirect across subsequent submits.
noRedirect stays true after the first “Save” and will also block redirect on the next “Save and continue.” Explicitly reset it on that button.
<button
class="btn preset-filled-primary-500 font-semibold w-full"
data-testid="save-no-continue-button"
type="submit"
onclick={() =>
form.form.update((data) => {
return { ...data, noRedirect: true };
})}>{m.save()}</button
>
<button
class="btn preset-filled-secondary-500 font-semibold w-full"
data-testid="save-button"
- type="submit">{m.saveAndContinue()}</button
+ type="submit"
+ onclick={() =>
+ form.form.update((data) => {
+ return { ...data, noRedirect: false };
+ })
+ }
+>{m.saveAndContinue()}</button
>📝 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.
| data-testid="save-no-continue-button" | |
| type="submit" | |
| onclick={() => | |
| form.form.update((data) => { | |
| return { ...data, noRedirect: true }; | |
| })}>{m.save()}</button | |
| > | |
| <button | |
| class="btn preset-filled-secondary-500 font-semibold w-full" | |
| data-testid="save-button" | |
| type="submit">{m.save()}</button | |
| type="submit">{m.saveAndContinue()}</button | |
| > | |
| <button | |
| class="btn preset-filled-primary-500 font-semibold w-full" | |
| data-testid="save-no-continue-button" | |
| type="submit" | |
| onclick={() => | |
| form.form.update((data) => { | |
| return { ...data, noRedirect: true }; | |
| }) | |
| } | |
| > | |
| {m.save()} | |
| </button> | |
| <button | |
| class="btn preset-filled-secondary-500 font-semibold w-full" | |
| data-testid="save-button" | |
| type="submit" | |
| onclick={() => | |
| form.form.update((data) => { | |
| return { ...data, noRedirect: false }; | |
| }) | |
| } | |
| > | |
| {m.saveAndContinue()} | |
| </button> |
🤖 Prompt for AI Agents
frontend/src/routes/(app)/(third-party)/requirement-assessments/[id=uuid]/edit/+page.svelte
around lines 642 to 653: the "Save and continue" submit button does not reset
form.form.noRedirect, so if a previous "Save" set noRedirect=true that value
persists and prevents redirect on subsequent submits; add an onclick handler to
the save-and-continue button that updates form.form to explicitly set
noRedirect:false (similar to the no-continue button which sets true) so each
submit uses the intended redirect behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/core/serializers.py (1)
598-632: Define explicit qualifications and drop deprecated existing_controls
- In RiskScenarioImportExportSerializer, add:
(to mirror the model’s ManyToManyField at core/models.py:2526)qualifications = HashSlugRelatedField(slug_field="pk", read_only=True, many=True)- Remove
"existing_controls"from Meta.fields in the same serializer (currently at serializers.py:619), relying onexisting_applied_controlsinstead.
🧹 Nitpick comments (2)
backend/core/serializers.py (2)
1582-1586: Make library read-only on write to avoid polluting loaded librariesEnd-users creating custom qualifications shouldn’t set
library. Keep it null; importer code can set it internally if needed.class QualificationWriteSerializer(BaseModelSerializer): + library = serializers.PrimaryKeyRelatedField(read_only=True) class Meta: model = Qualification exclude = ["translations"]
574-576: Standardize qualifications and add prefetch in RiskScenarioViewSet
- Replace
qualifications = FieldsRelatedField(many=True)
with
qualifications = FieldsRelatedField(["id", "name"], many=True)in backend/core/serializers.py- In
RiskScenarioViewSet.get_queryset()(backend/core/views.py:~2230), chain.prefetch_related("qualifications")to avoid N+1 on list endpoints
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
backend/core/serializers.py(2 hunks)frontend/src/routes/(app)/(internal)/risk-scenarios/[id=uuid]/+page.svelte(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/routes/(app)/(internal)/risk-scenarios/[id=uuid]/+page.svelte
🧰 Additional context used
🧬 Code graph analysis (1)
backend/core/serializers.py (2)
backend/core/serializer_fields.py (1)
FieldsRelatedField(24-86)backend/core/models.py (17)
Meta(107-108)Meta(162-163)Meta(190-191)Meta(195-197)Meta(992-994)Meta(1058-1060)Meta(1189-1191)Meta(1323-1325)Meta(1570-1573)Meta(1628-1630)Meta(1851-1853)Meta(2400-2408)Meta(2434-2436)Meta(2555-2557)Meta(2781-2783)Meta(2926-2928)Qualification(1431-1596)
⏰ 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: build (3.12)
- GitHub Check: 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: startup-functional-test (3.12)
- GitHub Check: Analyze (python)
🔇 Additional comments (1)
backend/core/serializers.py (1)
1588-1594: LGTM: Dedicated read serializer with library relation is consistentRead path mirrors other referentials (Threat, ReferenceControl) and excludes translations.
|
There are 3 remains things to do : Adding translations for the default qualifications (by adding a translation dict as a "translations" field for each qualification defined in core.models.Qualification.DEFAULT_QUALIFICATIONS). |
qualifications are managed similarly now for risk scenarios as for other usecases
remove old field
migrate qualification field content
for objectives and for qualifications
eric-intuitem
force-pushed
the
feat/custom_qualifications
branch
from
fb8579c to
0c0ce56
Compare
update for new qualifications format in risk scenarios
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/src/routes/(app)/(internal)/risk-scenarios/[id=uuid]/edit/+page.svelte (1)
419-427: Residual risk level bound to the wrong form fieldRiskLevel in the residual block writes to current_risk_level instead of residual_risk_level.
- <RiskLevel - form={_form} - field="current_risk_level" - label={m.residualRiskLevel()} - riskMatrix={data.riskMatrix} - probabilityField="residual_proba" - impactField="residual_impact" - helpText={m.residualRiskLevelHelpText()} - /> + <RiskLevel + form={_form} + field="residual_risk_level" + label={m.residualRiskLevel()} + riskMatrix={data.riskMatrix} + probabilityField="residual_proba" + impactField="residual_impact" + helpText={m.residualRiskLevelHelpText()} + />backend/core/serializers.py (1)
610-630: Unknown field in RiskScenarioImportExportSerializer.fieldsfields includes "existing_controls" which is not defined on the serializer/model (the defined field is existing_applied_controls). This will break serialization.
fields = [ "ref_id", "name", "description", "risk_assessment", "treatment", "threats", "vulnerabilities", "assets", - "existing_controls", "existing_applied_controls", "applied_controls", "current_proba", "current_impact", "residual_proba", "residual_impact", "strength_of_knowledge", "justification", "created_at", "updated_at", "qualifications", ]
♻️ Duplicate comments (2)
frontend/src/lib/utils/types.ts (1)
36-113: Duplicate entries in URL_MODEL (‘roles’, ‘processings’)roles appears twice (Lines 50 and 110) and processings appears twice (Lines 80 and 86). This can cause duplicate routes/menus.
'user-groups', - 'roles', 'role-assignments', 'compliance-assessments', @@ 'kill-chains', - 'processings', 'processing-natures', 'security-exceptions', @@ - // privacy, - 'processings', + // privacy, + 'processings', @@ - // roles, - 'roles', + // roles, + 'roles', 'permissions', 'qualifications'If you prefer keeping the first roles near user management, remove the later one instead; just ensure uniqueness across the array.
backend/core/models.py (1)
928-930: reference_count may over-count due to M2M duplicates — use distinct().Keeps counts accurate when the join returns duplicate rows.
- + Incident.objects.filter(qualifications__library=self).count() - + FearedEvent.objects.filter(qualifications__library=self).count() - + EscalationThreshold.objects.filter(qualifications__library=self).count() + + Incident.objects.filter(qualifications__library=self).distinct().count() + + FearedEvent.objects.filter(qualifications__library=self).distinct().count() + + EscalationThreshold.objects.filter(qualifications__library=self).distinct().count()
🧹 Nitpick comments (11)
frontend/messages/cs.json (2)
954-956: Czech grammar: use genitive plural for “ordering” labels.Prefer “Pořadí kvalifikací” and “Pořadí bezpečnostních cílů” (ordering of multiple items).
Apply:
-"qualificationOrdering": "Pořadí kvalifikace", -"securityObjectiveOrdering": "Pořadí bezpečnostního cíle", +"qualificationOrdering": "Pořadí kvalifikací", +"securityObjectiveOrdering": "Pořadí bezpečnostních cílů",
957-957: Capitalize list label to match surrounding UI.Most section/list labels are capitalized. Use “Kvalifikace”.
-"qualifications": "kvalifikace", +"qualifications": "Kvalifikace",frontend/messages/ro.json (1)
962-968: Minor consistency: capitalize plural labelMost top-level entity labels are capitalized in RO (e.g., “Politici”, “Biblioteci”). Consider “Calificări” instead of “calificări” for UI consistency. Optional.
- "qualifications": "calificări", + "qualifications": "Calificări",backend/core/views.py (1)
3270-3303: Add documentation for domain import M2M relationshipsThe
_create_model_objectsand_create_batchmethods handle domain import with M2M relationships. Consider adding docstring details about the newobjectivesandqualificationsrelationships being processed.def _create_model_objects(self, model, objects, link_dump_database_ids): - """Create all objects for a model after validation during domain import.""" + """Create all objects for a model after validation during domain import. + + Handles various M2M relationships including: + - AppliedControl.objectives + - RiskScenario.qualifications + """ logger.debug("Creating objects for model", model=model)backend/core/migrations/0093_qualification_library_and_more.py (1)
56-67: Remove temporary help text from production migrationThe help text "(temporary during migration)" should not be visible in production as it might confuse users if the migration fails partway through.
migrations.AddField( model_name="riskscenario", name="qualifications_tmp", field=models.ManyToManyField( blank=True, to="core.qualification", related_name="risk_scenarios_tmp", - help_text="(temporary during migration)", - verbose_name="Qualifications (tmp)", ), ),backend/core/models.py (6)
887-891: Stabilize library dump for qualifications (ordering + publish filter).Return a predictable, filtered list to avoid churn in dumps and exclude unpublished items.
- res["qualifications"] = [ - update_translations_in_object(model_to_dict(qualification)) - for qualification in self.qualifications.all() - ] + res["qualifications"] = [ + update_translations_in_object(model_to_dict(qualification)) + for qualification in self.qualifications + .filter(is_published=True) + .order_by("qualification_ordering", "id") + ]
905-907: Docstring out of sync with implementation.It now also counts incidents, feared events, and escalation thresholds. Consider updating the wording.
- Returns the number of distinct dependent libraries and risk and compliance assessments that reference objects from this library + Returns the number of distinct dependent libraries, risk/compliance assessments, and other domain objects (incidents, feared events, escalation thresholds) that reference objects from this library
1064-1066: Typo in docstring (“refence” → “reference”).- Returns True if the refence control can be deleted + Returns True if the reference control can be deleted
1577-1587: Block deletion when a Qualification is used by RiskScenarios.Currently only incidents/feared_events/escalation_thresholds are checked; consider also guarding on risk_scenarios to avoid surprising data loss.
return not any( [ self.incidents.exists(), self.feared_events.exists(), self.escalation_thresholds.exists(), + self.risk_scenarios.exists(), ] )
1433-1460: Add translations to DEFAULT_QUALIFICATIONS (per PR TODO).Populate the translations field so UI/lookups can localize names/abbrs. Example pattern below; replicate for the rest.
{ "abbreviation": "C", "qualification_ordering": 1, "security_objective_ordering": 1, "name": "Confidentiality", "urn": "urn:intuitem:risk:qualification:confidentiality", + "translations": { + "en": {"name": "Confidentiality", "abbreviation": "C"} + }, }, { "abbreviation": "I", "qualification_ordering": 2, "security_objective_ordering": 2, "name": "Integrity", "urn": "urn:intuitem:risk:qualification:integrity", + "translations": { + "en": {"name": "Integrity", "abbreviation": "I"} + }, },Would you like me to generate a complete patch adding translations for all defaults (and FR placeholders)?
Also applies to: 1441-1446
5352-5355: Audit log: include new qualifications M2M where relevant.Capture changes to RiskScenario/Incident qualifications like other M2Ms.
auditlog.register( RiskScenario, - m2m_fields={"owner", "applied_controls", "existing_applied_controls"}, + m2m_fields={"owner", "applied_controls", "existing_applied_controls", "qualifications"}, exclude_fields=common_exclude, ) @@ -auditlog.register( - Incident, - exclude_fields=common_exclude, -) +auditlog.register( + Incident, + m2m_fields={"qualifications"}, + exclude_fields=common_exclude, +)Also applies to: 5401-5406
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (35)
backend/core/migrations/0093_qualification_library_and_more.py(1 hunks)backend/core/models.py(8 hunks)backend/core/serializers.py(3 hunks)backend/core/views.py(8 hunks)backend/resilience/migrations/0003_alter_escalationthreshold_qualifications.py(1 hunks)frontend/messages/ar.json(1 hunks)frontend/messages/cs.json(1 hunks)frontend/messages/da.json(1 hunks)frontend/messages/de.json(1 hunks)frontend/messages/el.json(1 hunks)frontend/messages/en.json(1 hunks)frontend/messages/es.json(1 hunks)frontend/messages/fr.json(1 hunks)frontend/messages/hi.json(1 hunks)frontend/messages/hu.json(1 hunks)frontend/messages/id.json(1 hunks)frontend/messages/it.json(1 hunks)frontend/messages/nl.json(1 hunks)frontend/messages/pl.json(1 hunks)frontend/messages/pt.json(1 hunks)frontend/messages/ro.json(1 hunks)frontend/messages/sv.json(1 hunks)frontend/messages/tr.json(1 hunks)frontend/messages/uk.json(1 hunks)frontend/messages/ur.json(1 hunks)frontend/src/lib/components/Forms/ModelForm.svelte(2 hunks)frontend/src/lib/components/ModelTable/ModelTable.svelte(2 hunks)frontend/src/lib/utils/crud.ts(1 hunks)frontend/src/lib/utils/schemas.ts(2 hunks)frontend/src/lib/utils/table.ts(1 hunks)frontend/src/lib/utils/types.ts(1 hunks)frontend/src/routes/(app)/(internal)/risk-scenarios/[id=uuid]/+page.svelte(1 hunks)frontend/src/routes/(app)/(internal)/risk-scenarios/[id=uuid]/edit/+page.server.ts(1 hunks)frontend/src/routes/(app)/(internal)/risk-scenarios/[id=uuid]/edit/+page.svelte(1 hunks)frontend/src/routes/(app)/(internal)/stored-libraries/[id=uuid]/+page.svelte(4 hunks)
✅ Files skipped from review due to trivial changes (3)
- frontend/src/lib/components/ModelTable/ModelTable.svelte
- frontend/messages/pt.json
- frontend/messages/hu.json
🚧 Files skipped from review as they are similar to previous changes (24)
- frontend/messages/pl.json
- frontend/messages/en.json
- frontend/messages/da.json
- frontend/messages/de.json
- frontend/messages/el.json
- frontend/messages/ar.json
- frontend/src/lib/utils/table.ts
- frontend/messages/es.json
- frontend/src/routes/(app)/(internal)/risk-scenarios/[id=uuid]/+page.svelte
- frontend/messages/ur.json
- frontend/messages/hi.json
- frontend/messages/id.json
- frontend/messages/fr.json
- frontend/messages/tr.json
- frontend/messages/uk.json
- frontend/messages/nl.json
- frontend/src/lib/utils/crud.ts
- backend/resilience/migrations/0003_alter_escalationthreshold_qualifications.py
- frontend/src/routes/(app)/(internal)/stored-libraries/[id=uuid]/+page.svelte
- frontend/messages/sv.json
- frontend/src/lib/components/Forms/ModelForm.svelte
- frontend/src/routes/(app)/(internal)/risk-scenarios/[id=uuid]/edit/+page.server.ts
- frontend/src/lib/utils/schemas.ts
- frontend/messages/it.json
🧰 Additional context used
🧬 Code graph analysis (4)
backend/core/migrations/0093_qualification_library_and_more.py (2)
backend/core/models.py (1)
Qualification(1431-1596)backend/resilience/migrations/0003_alter_escalationthreshold_qualifications.py (1)
Migration(6-22)
backend/core/models.py (3)
backend/library/helpers.py (1)
update_translations_in_object(49-75)backend/ebios_rm/models.py (1)
FearedEvent(249-314)backend/resilience/models.py (1)
EscalationThreshold(176-248)
backend/core/views.py (1)
backend/core/models.py (1)
Qualification(1431-1596)
backend/core/serializers.py (2)
backend/core/serializer_fields.py (2)
FieldsRelatedField(24-86)HashSlugRelatedField(10-21)backend/core/models.py (7)
Meta(107-108)Meta(162-163)Meta(190-191)Meta(195-197)Meta(992-994)Meta(1058-1060)Qualification(1431-1596)
🔇 Additional comments (14)
frontend/messages/cs.json (2)
954-954: LGTM: “abbreviation” translation is correct.
953-958: i18n coverage verified across all locale files
All JSON files in frontend/messages parsed without errors and include abbreviation, qualificationOrdering, securityObjectiveOrdering, qualifications, and addQualification in every locale.frontend/src/routes/(app)/(internal)/risk-scenarios/[id=uuid]/edit/+page.svelte (1)
439-446: Endpoint-backed qualifications select looks correct; confirm payload shapeUsing optionsEndpoint="qualifications" with optionsLabelField="name" and optionsValueField="id" matches QualificationReadSerializer output and RiskScenario write expectations. LGTM; just ensure the API returns id and name in list responses.
backend/core/serializers.py (3)
572-574: Expose qualifications on RiskScenario read: looks goodFieldsRelatedField(many=True) aligns with the M2M and the frontend AutocompleteSelect. No issues.
605-606: Exporting qualifications by URN hash: OKHashSlugRelatedField(slug_field="urn", many=True, read_only=True) matches existing import/export patterns for related entities. LGTM.
1581-1593: Qualification serializers separation (read/write) is cleanDedicated write serializer and read serializer with library field are appropriate and match other referential entities. Excluding translations on write is consistent. LGTM.
backend/core/views.py (3)
3412-3415: LGTM! AppliedControl objectives handling added correctlyThe new
objective_idsmapping for AppliedControl's objectives relationship is properly extracted and will be set via M2M relations.
3599-3600: LGTM! AppliedControl objectives M2M assignment implementedThe new objectives field is correctly assigned using the mapped IDs during domain import.
3630-3631: LGTM! RiskScenario qualifications M2M assignment implementedThe qualifications field is properly assigned to RiskScenario using the Qualification model filter.
backend/core/migrations/0093_qualification_library_and_more.py (1)
1-4: The script above will reveal whether the qualification defaults and RunPython calls are present. Once we have those details, we can confirm if the migration needs anensure_default_qualificationsstep or if it’s already handled.backend/core/models.py (4)
909-911: Local imports to avoid circular deps — OK.Importing FearedEvent/EscalationThreshold inside the property is fine here.
913-917: Should risk assessments that reference qualifications via risk scenarios be counted?If yes, include this predicate in the existing RiskAssessment queryset ORs (keeps it one distinct() count rather than a separate summand).
RiskAssessment.objects.filter( - Q(risk_scenarios__threats__library=self) + Q(risk_scenarios__threats__library=self) + | Q(risk_scenarios__qualifications__library=self) | Q(risk_matrix__library=self) | Q(risk_scenarios__applied_controls__reference_control__library=self) )
996-1002: Threat.is_deletable docstring tweak — OK.Wording improvement looks good.
1550-1559: Qualification now library-linked and publishable — OK.FK to LoadedLibrary with CASCADE and published flag both make sense.
| def migrate_qualifications(apps, schema_editor): | ||
| """ | ||
| Copy old qualification values (list of names) into the new ManyToMany field. | ||
| Deterministic: assumes RiskScenario.qualifications (old field) was always | ||
| a list of strings corresponding exactly to Qualification.name values. | ||
| """ | ||
| RiskScenario = apps.get_model("core", "RiskScenario") | ||
| Qualification = apps.get_model("core", "Qualification") | ||
|
|
||
| # Build strict mapping name -> id | ||
| name_to_id = dict(Qualification.objects.values_list("name", "id")) | ||
|
|
||
| for rs in RiskScenario.objects.all(): | ||
| old_names = getattr(rs, "qualifications", None) | ||
| if not old_names: | ||
| continue | ||
| ids = [] | ||
| for name in old_names: | ||
| if name not in name_to_id: | ||
| raise RuntimeError(f"Unknown qualification name: {name}") | ||
| ids.append(name_to_id[name]) | ||
| if ids: | ||
| rs.qualifications_tmp.add(*ids) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and logging to data migration
The data migration should handle edge cases more gracefully and provide better debugging information. Consider:
- Logging unknown qualification names before raising the error
- Handling potential database integrity issues with transaction management
- Adding a reverse migration for rollback scenarios
def migrate_qualifications(apps, schema_editor):
"""
Copy old qualification values (list of names) into the new ManyToMany field.
Deterministic: assumes RiskScenario.qualifications (old field) was always
a list of strings corresponding exactly to Qualification.name values.
"""
+ from django.db import transaction
RiskScenario = apps.get_model("core", "RiskScenario")
Qualification = apps.get_model("core", "Qualification")
# Build strict mapping name -> id
name_to_id = dict(Qualification.objects.values_list("name", "id"))
- for rs in RiskScenario.objects.all():
- old_names = getattr(rs, "qualifications", None)
- if not old_names:
- continue
- ids = []
- for name in old_names:
- if name not in name_to_id:
- raise RuntimeError(f"Unknown qualification name: {name}")
- ids.append(name_to_id[name])
- if ids:
- rs.qualifications_tmp.add(*ids)
+ with transaction.atomic():
+ for rs in RiskScenario.objects.all():
+ old_names = getattr(rs, "qualifications", None)
+ if not old_names:
+ continue
+ ids = []
+ unknown_names = []
+ for name in old_names:
+ if name not in name_to_id:
+ unknown_names.append(name)
+ else:
+ ids.append(name_to_id[name])
+
+ if unknown_names:
+ # Log the problematic scenario for debugging
+ print(f"RiskScenario {rs.id} has unknown qualifications: {unknown_names}")
+ print(f"Available qualifications: {list(name_to_id.keys())}")
+ raise RuntimeError(
+ f"Unknown qualification names in RiskScenario {rs.id}: {unknown_names}"
+ )
+
+ if ids:
+ rs.qualifications_tmp.add(*ids)
+
+def reverse_migrate_qualifications(apps, schema_editor):
+ """Reverse migration to restore original data structure."""
+ RiskScenario = apps.get_model("core", "RiskScenario")
+
+ for rs in RiskScenario.objects.all():
+ qualification_names = list(
+ rs.qualifications_tmp.values_list("name", flat=True)
+ )
+ if qualification_names:
+ rs.qualifications = qualification_names
+ rs.save(update_fields=["qualifications"])Then update the RunPython operation:
- migrations.RunPython(migrate_qualifications, migrations.RunPython.noop),
+ migrations.RunPython(migrate_qualifications, reverse_migrate_qualifications),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def migrate_qualifications(apps, schema_editor): | |
| """ | |
| Copy old qualification values (list of names) into the new ManyToMany field. | |
| Deterministic: assumes RiskScenario.qualifications (old field) was always | |
| a list of strings corresponding exactly to Qualification.name values. | |
| """ | |
| RiskScenario = apps.get_model("core", "RiskScenario") | |
| Qualification = apps.get_model("core", "Qualification") | |
| # Build strict mapping name -> id | |
| name_to_id = dict(Qualification.objects.values_list("name", "id")) | |
| for rs in RiskScenario.objects.all(): | |
| old_names = getattr(rs, "qualifications", None) | |
| if not old_names: | |
| continue | |
| ids = [] | |
| for name in old_names: | |
| if name not in name_to_id: | |
| raise RuntimeError(f"Unknown qualification name: {name}") | |
| ids.append(name_to_id[name]) | |
| if ids: | |
| rs.qualifications_tmp.add(*ids) | |
| def migrate_qualifications(apps, schema_editor): | |
| """ | |
| Copy old qualification values (list of names) into the new ManyToMany field. | |
| Deterministic: assumes RiskScenario.qualifications (old field) was always | |
| a list of strings corresponding exactly to Qualification.name values. | |
| """ | |
| from django.db import transaction | |
| RiskScenario = apps.get_model("core", "RiskScenario") | |
| Qualification = apps.get_model("core", "Qualification") | |
| # Build strict mapping name -> id | |
| name_to_id = dict(Qualification.objects.values_list("name", "id")) | |
| with transaction.atomic(): | |
| for rs in RiskScenario.objects.all(): | |
| old_names = getattr(rs, "qualifications", None) | |
| if not old_names: | |
| continue | |
| ids = [] | |
| unknown_names = [] | |
| for name in old_names: | |
| if name not in name_to_id: | |
| unknown_names.append(name) | |
| else: | |
| ids.append(name_to_id[name]) | |
| if unknown_names: | |
| # Log the problematic scenario for debugging | |
| print(f"RiskScenario {rs.id} has unknown qualifications: {unknown_names}") | |
| print(f"Available qualifications: {list(name_to_id.keys())}") | |
| raise RuntimeError( | |
| f"Unknown qualification names in RiskScenario {rs.id}: {unknown_names}" | |
| ) | |
| if ids: | |
| rs.qualifications_tmp.add(*ids) | |
| def reverse_migrate_qualifications(apps, schema_editor): | |
| """Reverse migration to restore original data structure.""" | |
| RiskScenario = apps.get_model("core", "RiskScenario") | |
| for rs in RiskScenario.objects.all(): | |
| qualification_names = list( | |
| rs.qualifications_tmp.values_list("name", flat=True) | |
| ) | |
| if qualification_names: | |
| rs.qualifications = qualification_names | |
| rs.save(update_fields=["qualifications"]) | |
| # And in the migration operations list: | |
| # before: | |
| # migrations.RunPython(migrate_qualifications, migrations.RunPython.noop), | |
| # after: | |
| # migrations.RunPython(migrate_qualifications, reverse_migrate_qualifications), |
| qualifications = models.ManyToManyField( | ||
| Qualification, | ||
| blank=True, | ||
| verbose_name=_("Qualifications"), | ||
| related_name="risk_scenarios", | ||
| help_text=_("Qualifications carried by the risk scenario"), | ||
| ) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
JSON→M2M backfill for RiskScenario.qualifications is required.
The schema change is present; ensure migration 0092 includes a reversible RunPython that copies legacy JSON values into the M2M through table.
Example migration snippet (adjust legacy field name if needed):
def forwards(apps, schema_editor):
RiskScenario = apps.get_model("core", "RiskScenario")
Qualification = apps.get_model("core", "Qualification")
db = schema_editor.connection.alias
for rs in RiskScenario.objects.using(db).all():
legacy = getattr(rs, "_qualifications_legacy", None) or rs.__dict__.get("qualifications", [])
if not legacy:
continue
to_add = []
for item in legacy:
urn = item if isinstance(item, str) else (item.get("urn") if isinstance(item, dict) else None)
if urn:
q = Qualification.objects.using(db).filter(urn__iexact=urn).first()
if q:
to_add.append(q)
if to_add:
rs.qualifications.add(*to_add)
def backwards(apps, schema_editor):
RiskScenario = apps.get_model("core", "RiskScenario")
db = schema_editor.connection.alias
for rs in RiskScenario.objects.using(db).all():
setattr(rs, "_qualifications_legacy", [q.urn for q in rs.qualifications.all()])
rs.save(update_fields=["_qualifications_legacy"])I can open a follow-up PR with the exact migration once you confirm the legacy field name used in 0091/0092.
🏁 Script executed:
#!/bin/bash
# Verify a data-migration exists that backfills RiskScenario.qualifications
fd -t f -e py 'backend/**/migrations' | xargs rg -nP '0092|RiskScenario.*qualifications|RunPython|JSONField'Length of output: 292382
Make the qualifications migration reversible and backfill legacy JSON→M2M.
- Migration exists in backend/core/migrations/0093_qualification_library_and_more.py and currently calls RunPython(migrate_qualifications, migrations.RunPython.noop).
- Replace with a reversible RunPython: forwards must copy the legacy JSON (introduced in backend/core/migrations/0022_riskscenario_qualifications.py) into the new ManyToMany (map strings/dicts to Qualification by urn and rs.qualifications.add(*qs)); backwards should restore the legacy JSON (e.g., set legacy field to [q.urn for q in rs.qualifications.all()]).
- I can open a follow-up PR with the exact migration once you confirm the legacy field name.
🤖 Prompt for AI Agents
In backend/core/models.py around lines 3659 to 3665, the new ManyToManyField
"qualifications" needs a reversible migration that backfills from the legacy
JSON field and restores it on reverse; update
backend/core/migrations/0093_qualification_library_and_more.py to replace
RunPython(migrate_qualifications, migrations.RunPython.noop) with a reversible
RunPython function pair: forward should iterate RiskScenario objects, read the
legacy JSON field (use the legacy field name I'll confirm), normalize entries
(strings or dicts) to URNs, look up Qualification by urn and call
rs.qualifications.add(*found_qs) (skip missing quals, handle null/empty), and
save; reverse should iterate RiskScenario objects, build a list of qualification
urns from rs.qualifications.all() and set the legacy JSON field to that list (or
[]), saving each object; ensure the migration uses atomic operations and is
idempotent.
| many_to_many_map_ids["qualification_ids"] = get_mapped_ids( | ||
| _fields.pop("qualifications", []), link_dump_database_ids | ||
| ) |
There was a problem hiding this comment.
Fix indentation issue in qualification_ids mapping
The qualification_ids mapping for RiskScenario is incorrectly indented inside the loop for related fields. It should be at the same level as other field mappings.
for field in related__fields:
map_key = (
f"{field.rstrip('s')}_ids"
if not field.endswith("controls")
else f"{field}_ids"
)
many_to_many_map_ids[map_key] = get_mapped_ids(
_fields.pop(field, []), link_dump_database_ids
)
- many_to_many_map_ids["qualification_ids"] = get_mapped_ids(
- _fields.pop("qualifications", []), link_dump_database_ids
- )
+ many_to_many_map_ids["qualification_ids"] = get_mapped_ids(
+ _fields.pop("qualifications", []), link_dump_database_ids
+ )📝 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.
| many_to_many_map_ids["qualification_ids"] = get_mapped_ids( | |
| _fields.pop("qualifications", []), link_dump_database_ids | |
| ) | |
| for field in related__fields: | |
| map_key = ( | |
| f"{field.rstrip('s')}_ids" | |
| if not field.endswith("controls") | |
| else f"{field}_ids" | |
| ) | |
| many_to_many_map_ids[map_key] = get_mapped_ids( | |
| _fields.pop(field, []), link_dump_database_ids | |
| ) | |
| many_to_many_map_ids["qualification_ids"] = get_mapped_ids( | |
| _fields.pop("qualifications", []), link_dump_database_ids | |
| ) |
🤖 Prompt for AI Agents
In backend/core/views.py around lines 3472 to 3474, the assignment adding
"qualification_ids" to many_to_many_map_ids is incorrectly indented inside the
related-fields loop; move that block so the qualification_ids mapping (calling
get_mapped_ids on _fields.pop("qualifications", []), link_dump_database_ids)
sits at the same indentation level as the other top-level field mappings (i.e.,
outside the loop) so it is executed once per RiskScenario instead of per related
field.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
backend/library/utils.py (2)
755-764: Gate qualifications to enterprise and fix log labelOnly process qualifications when enterprise is installed; also correct the logger label.
Apply:
- if "qualifications" in library_objects: - qualification_data = library_objects["qualifications"] - if ( - qualification_import_error := self.init_qualifications( - qualification_data - ) - ) is not None: - logger.error("Threat import error", error=qualification_import_error) - return qualification_import_error + if apps.is_installed("enterprise_core") and "qualifications" in library_objects: + qualification_data = library_objects["qualifications"] + if ( + qualification_import_error := self.init_qualifications( + qualification_data + ) + ) is not None: + logger.error("Qualification import error", error=qualification_import_error) + return qualification_import_error
410-441: Make qualification import idempotent and validate abbreviation lengthUse
update_or_createkeyed by(urn, locale)to avoid duplicate rows on re-imports, and enforceabbreviationmax length (20) before hitting DB constraints.Apply:
class QualificationImporter: @@ def import_qualification(self, library_object: LoadedLibrary): - Qualification.objects.create( - library=library_object, - urn=self.qualification_data["urn"].lower(), - ref_id=self.qualification_data["ref_id"], - name=self.qualification_data["name"], - description=self.qualification_data.get("description"), - abbreviation=self.qualification_data.get("abbreviation"), - qualification_ordering=self.qualification_data.get( - "qualification_ordering", 0 - ), - security_objective_ordering=self.qualification_data.get( - "security_objective_ordering", 0 - ), - provider=library_object.provider, - is_published=True, - locale=library_object.locale, - translations=self.qualification_data.get("translations", {}), - default_locale=library_object.default_locale, # Change this in the future ? - ) + abbr = self.qualification_data.get("abbreviation") + if abbr is not None and len(abbr) > 20: + raise ValueError("abbreviation must be <= 20 characters") + + Qualification.objects.update_or_create( + urn=self.qualification_data["urn"].lower(), + locale=library_object.locale, + defaults=dict( + library=library_object, + ref_id=self.qualification_data["ref_id"], + name=self.qualification_data["name"], + description=self.qualification_data.get("description"), + abbreviation=abbr, + qualification_ordering=self.qualification_data.get( + "qualification_ordering", 0 + ), + security_objective_ordering=self.qualification_data.get( + "security_objective_ordering", 0 + ), + provider=library_object.provider, + is_published=True, + translations=self.qualification_data.get("translations", {}), + default_locale=library_object.default_locale, # Change this in the future ? + ), + )
🧹 Nitpick comments (10)
backend/library/utils.py (1)
5-5: Remove unused import or use it for enterprise gating
from django.apps import appsis unused unless you gate qualifications to enterprise; either remove it or apply the gating below.enterprise/frontend/src/lib/utils/models.ts (1)
1-5: Make override type-safe and freeze action literalsUse literal types to prevent drift and ease downstream narrowing.
-export const MODEL_MAP_OVERRIDE = { - qualifications: { - actions: ['add', 'view', 'change', 'delete'] - }, -}; +export const MODEL_MAP_OVERRIDE = { + qualifications: { + actions: ['add', 'view', 'change', 'delete'] as const, + }, +} as const;frontend/src/lib/components/DetailView/DetailView.svelte (1)
210-214: Consider extracting domain resolution into a helperThe folder-domain calculation is repeated across views; a small util (e.g., getDomainId(model, data, user)) would reduce drift.
frontend/src/routes/(app)/(internal)/risk-scenarios/[id=uuid]/+page.svelte (2)
26-31: Switch to model object for permissions — LGTMMinor note: for consistency with newer helpers, consider getModelInfo('risk-scenarios') instead of URL_MODEL_MAP here.
363-366: Simplify qualifications rendering and avoid per-item fallback commasCurrent loop can emit leading/trailing commas and multiple “undefined”. Join once with a single fallback.
Apply this diff:
- {#each data.scenario.qualifications as qualification, i} - {#if i > 0},{/if} - {qualification.str || m.undefined()} - {/each} + {#if data.scenario.qualifications?.length} + {data.scenario.qualifications.map((q) => q.str).filter(Boolean).join(', ')} + {:else} + {m.undefined()} + {/if}frontend/src/routes/(app)/(third-party)/compliance-assessments/[id=uuid]/+page.svelte (1)
51-56: Pass full ModelMapEntry to canPerformAction—use getModelInfo to honor overridesUsing URL_MODEL_MAP works but may miss enterprise overrides/actions. Switch to getModelInfo for consistency with the rest of the app.
Apply:
@@ - import { URL_MODEL_MAP } from '$lib/utils/crud'; + import { URL_MODEL_MAP, getModelInfo } from '$lib/utils/crud'; @@ - const model = URL_MODEL_MAP['compliance-assessments']; + const model = getModelInfo('compliance-assessments'); @@ - const requirementAssessmentModel = URL_MODEL_MAP['requirement-assessments']; + const requirementAssessmentModel = getModelInfo('requirement-assessments');Ensure both models have .name populated after this change.
Also applies to: 60-65
frontend/src/lib/utils/access-control.ts (1)
1-16: Guard on disallowed actions first; tiny readability winEarly-return if the action isn’t allowed by the model before checking domain permissions.
Apply:
export function canPerformAction({ user, action, model, domain }: CanPerformActionParams): boolean { - return ( - (user.domain_permissions[domain] || []).includes(`${action}_${model.name}`) && - !(model?.actions && !model.actions.includes(action)) - ); + if (model?.actions && !model.actions.includes(action)) return false; + return (user.domain_permissions[domain] || []).includes(`${action}_${model.name}`); }frontend/src/lib/utils/crud.ts (3)
732-749: Qualifications detail view: mark datetime fields; optionally link library
- created_at and updated_at should use type: 'datetime' for consistent rendering (used elsewhere).
- If the detail renderer supports linking FKs via urlModel, consider marking library as urlModel: 'libraries' so it’s clickable (verify typings).
Apply:
detailViewFields: [ { field: 'name' }, { field: 'description' }, { field: 'annotation' }, - { field: 'created_at' }, - { field: 'updated_at' }, + { field: 'created_at', type: 'datetime' }, + { field: 'updated_at', type: 'datetime' }, { field: 'urn' }, { field: 'id' }, { field: 'provider' }, { field: 'locale' }, { field: 'default_locale' }, { field: 'abbreviation' }, { field: 'qualification_ordering' }, { field: 'security_objective_ordering' }, - { field: 'library' } + { field: 'library' } ], actions: ['view']If linking is desired, and supported by your renderer types, change the last line to:
{ field: 'library', urlModel: 'libraries' }
1619-1622: getModelInfo refactor is cleaner; propagate the router usageNice guard for undefined and centralizing resolution in routeToModelInfo.
To keep behavior consistent (e.g., for model variants like *_duplicate), update helpers below to use getModelInfo instead of direct URL_MODEL_MAP access:
-export const urlParamModelForeignKeyFields = (model: string): ForeignKeyField[] => { - return URL_MODEL_MAP[model]?.foreignKeyFields || []; -}; +export const urlParamModelForeignKeyFields = (model: string): ForeignKeyField[] => { + const info = getModelInfo(model); + return info?.foreignKeyFields || []; +}; -export const urlParamModelSelectFields = (model: string): SelectField[] => { - return URL_MODEL_MAP[model]?.selectFields || []; -}; +export const urlParamModelSelectFields = (model: string): SelectField[] => { + const info = getModelInfo(model); + return info?.selectFields || []; +};Please verify no callers rely on the previous direct-lookup fallback semantics.
7-7: DecoupleURL_MODEL_MAPto eliminate the ES module cycle
We have a circular import:
frontend/src/lib/utils/crud.tsimportsrouteToModelInfo(line 1621)frontend/src/lib/utils/modelinfo-router.tsimportsURL_MODEL_MAP(line 1)To break this cycle, consider:
- Extracting
URL_MODEL_MAPinto its own module (e.g.utils/model-map.ts) that both files import- Or changing
routeToModelInfoto accept the map as an argument (e.g.routeToModelInfo(model, URL_MODEL_MAP)) somodelinfo-router.tsno longer imports fromcrud.ts
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (25)
.github/workflows/functional-tests.yml(1 hunks)backend/library/utils.py(8 hunks)backend/serdes/views.py(0 hunks)enterprise/frontend/src/lib/utils/modelinfo-router.ts(1 hunks)enterprise/frontend/src/lib/utils/models.ts(1 hunks)frontend/src/lib/components/DetailView/DetailView.svelte(1 hunks)frontend/src/lib/components/ModelTable/ModelTable.svelte(6 hunks)frontend/src/lib/components/TableRowActions/TableRowActions.svelte(3 hunks)frontend/src/lib/utils/access-control.ts(1 hunks)frontend/src/lib/utils/crud.ts(4 hunks)frontend/src/lib/utils/modelinfo-router.ts(1 hunks)frontend/src/routes/(app)/(internal)/[model=urlmodel]/+layout.server.ts(0 hunks)frontend/src/routes/(app)/(internal)/ebios-rm/[id=uuid]/workshop-1/baseline/+page.svelte(1 hunks)frontend/src/routes/(app)/(internal)/ebios-rm/[id=uuid]/workshop-1/ebios-rm-study/+page.svelte(1 hunks)frontend/src/routes/(app)/(internal)/incidents/[id=uuid]/+page.svelte(1 hunks)frontend/src/routes/(app)/(internal)/my-profile/+page.svelte(2 hunks)frontend/src/routes/(app)/(internal)/operating-modes/[id=uuid]/+page.svelte(1 hunks)frontend/src/routes/(app)/(internal)/operational-scenarios/[id=uuid]/+page.svelte(1 hunks)frontend/src/routes/(app)/(internal)/recap/+page.svelte(1 hunks)frontend/src/routes/(app)/(internal)/risk-assessments/[id=uuid]/+page.svelte(1 hunks)frontend/src/routes/(app)/(internal)/risk-scenarios/[id=uuid]/+page.svelte(2 hunks)frontend/src/routes/(app)/(internal)/ro-to/[id=uuid]/+page.svelte(1 hunks)frontend/src/routes/(app)/(internal)/stored-libraries/[id=uuid]/+page.svelte(3 hunks)frontend/src/routes/(app)/(third-party)/compliance-assessments/[id=uuid]/+page.svelte(2 hunks)frontend/src/routes/(app)/(third-party)/evidences/[id=uuid]/+page.svelte(1 hunks)
💤 Files with no reviewable changes (2)
- backend/serdes/views.py
- frontend/src/routes/(app)/(internal)/[model=urlmodel]/+layout.server.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/functional-tests.yml
- frontend/src/routes/(app)/(internal)/stored-libraries/[id=uuid]/+page.svelte
🧰 Additional context used
🧬 Code graph analysis (5)
frontend/src/lib/utils/modelinfo-router.ts (2)
enterprise/frontend/src/lib/utils/modelinfo-router.ts (1)
routeToModelInfo(4-11)frontend/src/lib/utils/crud.ts (1)
URL_MODEL_MAP(147-1468)
enterprise/frontend/src/lib/utils/modelinfo-router.ts (2)
frontend/src/lib/utils/crud.ts (1)
URL_MODEL_MAP(147-1468)enterprise/frontend/src/lib/utils/models.ts (1)
MODEL_MAP_OVERRIDE(1-5)
frontend/src/lib/utils/access-control.ts (2)
frontend/src/lib/utils/types.ts (1)
User(5-24)frontend/src/lib/utils/crud.ts (1)
ModelMapEntry(122-141)
frontend/src/lib/utils/crud.ts (2)
frontend/src/lib/utils/types.ts (1)
urlModel(117-117)frontend/src/lib/utils/modelinfo-router.ts (1)
routeToModelInfo(3-9)
backend/library/utils.py (1)
backend/core/models.py (3)
Qualification(1431-1596)LoadedLibrary(849-971)create(3001-3003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: startup-docker-compose-test
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: build_enterprise_frontend
- GitHub Check: build_community_frontend
- GitHub Check: test (3.12)
- GitHub Check: build (3.12)
🔇 Additional comments (19)
frontend/src/routes/(app)/(internal)/risk-assessments/[id=uuid]/+page.svelte (1)
36-41: Verified: allcanPerformActioncalls switch to full model
No remaining instances ofmodel.namein call sites.frontend/src/routes/(app)/(internal)/recap/+page.svelte (1)
28-34: LGTM: permission refactor consistencyUsing the full
modelobject withcanPerformActionis consistent with the new API.frontend/src/routes/(app)/(internal)/ro-to/[id=uuid]/+page.svelte (1)
42-48: LGTM: switched to full model for ACL
model: modelaligns with the updated access-control helper.frontend/src/routes/(app)/(internal)/operational-scenarios/[id=uuid]/+page.svelte (1)
50-55: LGTM: updated canPerformAction signaturePassing the full model object is correct here.
backend/library/utils.py (3)
571-592: Good validation scaffold for qualificationsValidation and error aggregation mirror other importers and look consistent.
494-513: LGTM: wire qualifications into object lists and stateAdding "qualifications" to OBJECT_FIELDS and
_qualificationsstate is correct.
841-843: LGTM: import loopDelegating to each importer is consistent with other object types.
frontend/src/routes/(app)/(internal)/operating-modes/[id=uuid]/+page.svelte (1)
69-78: Approve: passing full model object to canPerformAction is correct; no legacy string-based usages found.frontend/src/routes/(app)/(internal)/incidents/[id=uuid]/+page.svelte (1)
172-181: Permission check update LGTMSwitching to model: data.model aligns with ModelMapEntry-based permissions.
frontend/src/routes/(app)/(third-party)/evidences/[id=uuid]/+page.svelte (1)
70-79: Consistent use of full model object for permissionsChange is correct and matches the new access-control signature.
frontend/src/routes/(app)/(internal)/ebios-rm/[id=uuid]/workshop-1/baseline/+page.svelte (1)
65-73: Pass full model object to canPerformAction — LGTMChange aligns with the updated access-control API. Domain resolution looks consistent with other views.
frontend/src/lib/components/DetailView/DetailView.svelte (1)
205-215: Pass full model object to canPerformAction — LGTMMatches the project-wide shift to object-based permissions.
frontend/src/routes/(app)/(internal)/ebios-rm/[id=uuid]/workshop-1/ebios-rm-study/+page.svelte (1)
87-95: Pass full model object to canPerformAction — LGTMConsistent with the new access-control signature.
frontend/src/lib/components/ModelTable/ModelTable.svelte (3)
9-14: Model info now resolved via getModelInfo—LGTMThis aligns with the new routing and supports enterprise overrides.
Also applies to: 139-139
341-351: Permission checks now pass the model object—LGTMUsing ModelMapEntry matches access-control.ts and enables per-model action gating.
Confirm that getModelInfo(URLModel) always returns an entry with a valid name for all tables rendered here.
Also applies to: 357-371
412-412: Added data-testid hooks—nice for E2E stabilitydata-testid="model-table" and per-row ids will simplify selectors.
Also applies to: 510-510
frontend/src/lib/components/TableRowActions/TableRowActions.svelte (2)
129-144: Switched to ModelMapEntry for auth checks—LGTMDomain resolution and fallbacks remain unchanged.
Also applies to: 146-160
162-169: Explicit Boolean coercion for displayEdit—LGTMMakes intent clear with no behavior change.
frontend/src/lib/utils/crud.ts (1)
140-141: actions metadata addition looks goodAdding actions?: Array<'add' | 'view' | 'change' | 'delete'> enables per-model UI capabilities control.
| export function routeToModelInfo(model: string) { | ||
| const baseModel = model.split('_')[0]; | ||
| const map = URL_MODEL_MAP[model] || URL_MODEL_MAP[baseModel] || {}; | ||
| // The urlmodel of {model}_duplicate must be {model} | ||
| map['urlModel'] = baseModel; | ||
| const override = MODEL_MAP_OVERRIDE[baseModel] || {}; | ||
| return {...map, ...override}; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Bug: mutates URL_MODEL_MAP entries in place
Assigning map['urlModel'] mutates the shared object; side effects can leak globally. Clone before setting urlModel.
Apply:
-export function routeToModelInfo(model: string) {
- const baseModel = model.split('_')[0];
- const map = URL_MODEL_MAP[model] || URL_MODEL_MAP[baseModel] || {};
- // The urlmodel of {model}_duplicate must be {model}
- map['urlModel'] = baseModel;
- const override = MODEL_MAP_OVERRIDE[baseModel] || {};
- return {...map, ...override};
-}
+export function routeToModelInfo(model: string) {
+ const baseModel = model.split('_')[0];
+ const base = URL_MODEL_MAP[model] ?? URL_MODEL_MAP[baseModel] ?? {};
+ const map = { ...base, urlModel: baseModel };
+ const override = MODEL_MAP_OVERRIDE[baseModel] ?? {};
+ return { ...map, ...override };
+}Search for other places that might mutate URL_MODEL_MAP entries.
🏁 Script executed:
#!/bin/bash
rg -nP "URL_MODEL_MAP\\[[^\\]]+\\]\\s*\\[|Object\\.assign\\(\\s*URL_MODEL_MAP" -C2Length of output: 3743
Shallow-clone before setting urlModel
Apply the following change to avoid mutating shared URL_MODEL_MAP:
export function routeToModelInfo(model: string) {
- const baseModel = model.split('_')[0];
- const map = URL_MODEL_MAP[model] || URL_MODEL_MAP[baseModel] || {};
- // The urlmodel of {model}_duplicate must be {model}
- map['urlModel'] = baseModel;
- const override = MODEL_MAP_OVERRIDE[baseModel] || {};
- return {...map, ...override};
+ const baseModel = model.split('_')[0];
+ const base = URL_MODEL_MAP[model] ?? URL_MODEL_MAP[baseModel] ?? {};
+ const map = { ...base, urlModel: baseModel };
+ const override = MODEL_MAP_OVERRIDE[baseModel] ?? {};
+ return { ...map, ...override };
}No other write-operations on URL_MODEL_MAP entries were found in the codebase.
📝 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.
| export function routeToModelInfo(model: string) { | |
| const baseModel = model.split('_')[0]; | |
| const map = URL_MODEL_MAP[model] || URL_MODEL_MAP[baseModel] || {}; | |
| // The urlmodel of {model}_duplicate must be {model} | |
| map['urlModel'] = baseModel; | |
| const override = MODEL_MAP_OVERRIDE[baseModel] || {}; | |
| return {...map, ...override}; | |
| } | |
| export function routeToModelInfo(model: string) { | |
| const baseModel = model.split('_')[0]; | |
| const base = URL_MODEL_MAP[model] ?? URL_MODEL_MAP[baseModel] ?? {}; | |
| const map = { ...base, urlModel: baseModel }; | |
| const override = MODEL_MAP_OVERRIDE[baseModel] ?? {}; | |
| return { ...map, ...override }; | |
| } |
🤖 Prompt for AI Agents
In enterprise/frontend/src/lib/utils/modelinfo-router.ts around lines 4 to 11,
the function mutates the object returned from URL_MODEL_MAP by setting
map['urlModel'], which can mutate shared map entries; fix by shallow-cloning the
resolved map before modifying it (e.g., const mapCopy = {...map}), set
mapCopy['urlModel'] = baseModel, then return the merged result using mapCopy and
the override instead of mutating the original.
| export function routeToModelInfo(model: string) { | ||
| const baseModel = model.split('_')[0]; | ||
| const map = URL_MODEL_MAP[model] || URL_MODEL_MAP[baseModel] || {}; | ||
| // The urlmodel of {model}_duplicate must be {model} | ||
| map['urlModel'] = baseModel; | ||
| return map; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid mutating URL_MODEL_MAP entries
Setting map['urlModel'] mutates shared config. Return a new object to prevent side effects.
Apply this diff:
export function routeToModelInfo(model: string) {
const baseModel = model.split('_')[0];
- const map = URL_MODEL_MAP[model] || URL_MODEL_MAP[baseModel] || {};
- // The urlmodel of {model}_duplicate must be {model}
- map['urlModel'] = baseModel;
- return map;
+ const entry = URL_MODEL_MAP[model] || URL_MODEL_MAP[baseModel] || {};
+ // The urlmodel of {model}_duplicate must be {model}
+ return { ...entry, urlModel: baseModel };
}📝 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.
| export function routeToModelInfo(model: string) { | |
| const baseModel = model.split('_')[0]; | |
| const map = URL_MODEL_MAP[model] || URL_MODEL_MAP[baseModel] || {}; | |
| // The urlmodel of {model}_duplicate must be {model} | |
| map['urlModel'] = baseModel; | |
| return map; | |
| } | |
| export function routeToModelInfo(model: string) { | |
| const baseModel = model.split('_')[0]; | |
| const entry = URL_MODEL_MAP[model] || URL_MODEL_MAP[baseModel] || {}; | |
| // The urlmodel of {model}_duplicate must be {model} | |
| return { ...entry, urlModel: baseModel }; | |
| } |
🤖 Prompt for AI Agents
In frontend/src/lib/utils/modelinfo-router.ts around lines 3 to 9, the code
mutates shared URL_MODEL_MAP by assigning map['urlModel'] = baseModel; instead
return a new object so you don't modify the shared config. Replace the mutation
with creating and returning a copy (e.g. using the object spread or
Object.assign) that includes all existing map properties plus urlModel:
baseModel, so return { ...map, urlModel: baseModel } instead of mutating map.
| @@ -6,6 +6,7 @@ | |||
| import { getLocale } from '$paraglide/runtime'; | |||
| import Anchor from '$lib/components/Anchor/Anchor.svelte'; | |||
| import { canPerformAction } from '$lib/utils/access-control'; | |||
| import { getModelInfo } from '$lib/utils/crud'; | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Guard against undefined from getModelInfo('users') to avoid runtime/type errors
getModelInfo may return undefined per the new typing; add a small guard (or non-null assertion) before passing to canPerformAction.
const user = page.data.user;
-const canEditObject: boolean = canPerformAction({
+const userModel = getModelInfo('users');
+const canEditObject: boolean = userModel
+ ? canPerformAction({
user,
action: 'change',
- model: getModelInfo('users'),
+ model: userModel,
domain: user.root_folder_id
- });
+ })
+ : false;Optional check to ensure the model entry exists:
Also applies to: 28-34
🏁 Script executed:
#!/bin/bash
# Verify "users" exists in the URL model map
rg -nP "URL_MODEL_MAP\s*=\s*{(?s).*['\"]users['\"]\s*:" -C2 frontendLength of output: 74
🏁 Script executed:
#!/bin/bash
# Locate definition and usage of getModelInfo
rg -nR "getModelInfo" -C3 frontend
# Search for any model map definitions that might include "users"
rg -nR "URL_MODEL_MAP" -C3 frontend || rg -nR "MODEL_MAP" -C3 frontendLength of output: 1317
🏁 Script executed:
#!/bin/bash
# Locate the TypeScript definition of getModelInfo
rg -n "export function getModelInfo" -C5 -g "*.ts" .
# Locate the URL_MODEL_MAP declaration to verify 'users' entry
rg -n "URL_MODEL_MAP\s*=" -C5 -g "*.ts" .Length of output: 98
🏁 Script executed:
#!/bin/bash
# List files in the utils directory containing crud
ls frontend/src/lib/utils || echo "Directory not found"
# Display the first 200 lines of any crud.* files
for f in frontend/src/lib/utils/crud.*; do
[ -f "$f" ] && {
echo "=== $f ==="
sed -n '1,200p' "$f"
}
doneLength of output: 5876
🏁 Script executed:
#!/bin/bash
# Find all occurrences of getModelInfo to locate its implementation
rg -n "getModelInfo" -C3 -g "*.ts"
# Show content of modelinfo-router.ts around the definition of getModelInfo
grep -R "getModelInfo" -n frontend/src/lib/utils/modelinfo-router.ts | cut -d: -f1 | while read -r ln; do
sed -n "$((ln-5)),$((ln+20))p" frontend/src/lib/utils/modelinfo-router.ts
done
# Verify 'users' key exists in URL_MODEL_MAP
grep -R "URL_MODEL_MAP" -n frontend/src/lib/utils/crud.ts | cut -d: -f1 | head -1 | while read -r start; do
sed -n "$start, $((start+500))p" frontend/src/lib/utils/crud.ts | grep -C2 "'users'"
doneLength of output: 91723
Use non-null assertion for getModelInfo('users')
Replace the unguarded call with a non-null assertion to satisfy the return type and avoid passing undefined:
-const canEditObject = canPerformAction({
- user,
- action: 'change',
- model: getModelInfo('users'),
- domain: user.root_folder_id
-});
+const canEditObject = canPerformAction({
+ user,
+ action: 'change',
+ model: getModelInfo('users')!,
+ domain: user.root_folder_id
+});Apply the same ! assertion for the getModelInfo('users') call at lines 28–34.
📝 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.
| import { getModelInfo } from '$lib/utils/crud'; | |
| const canEditObject = canPerformAction({ | |
| user, | |
| action: 'change', | |
| model: getModelInfo('users')!, | |
| domain: user.root_folder_id | |
| }); |
🤖 Prompt for AI Agents
In frontend/src/routes/(app)/(internal)/my-profile/+page.svelte at line 9 and
especially around lines 28–34, the calls to getModelInfo('users') are unguarded
and may be inferred as possibly undefined; update those calls by appending a
non-null assertion (!) to getModelInfo('users') so the expressions become
getModelInfo('users')! (apply the same change to every occurrence on lines
28–34) to satisfy the return type and avoid passing undefined.
Summary by CodeRabbit