feat: align audits scores with framework boundaries on update #2840
Conversation
WalkthroughDetects framework score-boundary changes during library updates, raises Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Browser UI
participant Server as App Server (views)
participant Loader as LoadedLibrary / LibraryUpdater
participant DB as Database (Assessments)
UI->>Server: POST /update (optional ?action=ACTION)
Server->>Loader: library.update(strategy=<action> or None)
alt Boundaries unchanged or strategy provided
Loader->>DB: bulk_update ComplianceAssessment boundaries (where prev defaults)
Loader->>DB: bulk_update RequirementAssessment answers/scores/is_scored/documentation_score
Loader-->>Server: success
Server-->>UI: 200 { "status":"success" }
else Boundaries changed and strategy is None
Loader-->>Server: raise ScoreChangeDetected(details, choices)
Server-->>UI: 409 { "error":"score_change_detected", "choices":[...] }
Note right of UI: UI displays ChoicesModal with choices
UI->>Server: POST /update?action=rule_of_three
Server->>Loader: library.update(strategy="rule_of_three")
Loader->>DB: apply transform (rule_of_three) and bulk_update
Loader-->>Server: success
Server-->>UI: 200 { "status":"success" }
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Mohamed-Hacene
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
backend/core/models.py (1)
696-697: bulk_update skips model hooks; refresh impacted CA metrics/updated_at after score writes.
bulk_updatewon’t triggerRequirementAssessment.save()(which bumps CA.updated_at and daily metrics). After clamping scores, refresh affected CAs.Add right after this bulk_update:
# Recompute metrics and bump timestamps for impacted CAs impacted_ca_ids = {ra.compliance_assessment_id for ra in requirement_assessment_objects_to_update} for ca in ComplianceAssessment.objects.filter(id__in=impacted_ca_ids): ca.upsert_daily_metrics() ca.updated_at = timezone.now() ca.save(update_fields=["updated_at"])
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/core/models.py(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: 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: test (3.12)
- GitHub Check: build (3.12)
- GitHub Check: build_community_frontend
- GitHub Check: build_enterprise_frontend
- GitHub Check: Analyze (python)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
backend/core/models.py (2)
538-563: CA bounds: preserves user overrides and runs before RA clamping.The guarded update against previous defaults avoids overwriting customized CAs, and ordering is correct. Nice.
604-619: Dedupe RA updates to avoid redundant bulk_update work.The same RA can be appended multiple times (clamp branch, no-questions path, and after answers). This inflates the list and work done. Keep a companion id set and append each RA once.
@@ - requirement_assessment_objects_to_update = [] + requirement_assessment_objects_to_update = [] + requirement_assessment_objects_to_update_ids: set[int] = set() @@ - if clamped != ra.score: - ra.score = clamped - requirement_assessment_objects_to_update.append(ra) + if clamped != ra.score: + ra.score = clamped + if ra.id not in requirement_assessment_objects_to_update_ids: + requirement_assessment_objects_to_update.append(ra) + requirement_assessment_objects_to_update_ids.add(ra.id) @@ - if not questions: - requirement_assessment_objects_to_update.append(ra) + if not questions: + if ra.id not in requirement_assessment_objects_to_update_ids: + requirement_assessment_objects_to_update.append(ra) + requirement_assessment_objects_to_update_ids.add(ra.id) continue @@ - ra.answers = answers - requirement_assessment_objects_to_update.append(ra) + ra.answers = answers + if ra.id not in requirement_assessment_objects_to_update_ids: + requirement_assessment_objects_to_update.append(ra) + requirement_assessment_objects_to_update_ids.add(ra.id)Also applies to: 620-621, 690-692, 730-733
🧹 Nitpick comments (2)
backend/core/models.py (2)
525-528: Also select_related('requirement') to avoid N+1 on ra.requirement.You already added select_related('compliance_assessment'); add 'requirement' too since it’s dereferenced below when keying by ra.requirement.urn.
- for ra in RequirementAssessment.objects.filter( - requirement__framework=new_framework - ).select_related("compliance_assessment"): + for ra in ( + RequirementAssessment.objects + .filter(requirement__framework=new_framework) + .select_related("compliance_assessment", "requirement") + ):
602-602: Nit: fix comment typo.“anwsers” → “answers”.
- # update anwsers or score for each ra for the current requirement_node, when relevant + # update answers or score for each ra for the current requirement_node, when relevant
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/core/models.py(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: build (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: test (3.12)
- GitHub Check: build_community_frontend
- GitHub Check: startup-docker-compose-test
- GitHub Check: build_enterprise_frontend
- GitHub Check: Analyze (python)
🔇 Additional comments (1)
backend/core/models.py (1)
469-472: Good: capturing previous Framework defaults before update_or_create.This enables safe CA-bound propagation decisions later. LGTM.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (4)
backend/core/models.py (4)
564-564: Good: avoid N+1 on RA → CA.select_related("compliance_assessment") fixes the N+1. LGTM.
615-637: Preserve user CA overrides; update only defaults-first CAs (and before RA).This aligns with the model contract. LGTM.
804-806: Dedup RA updates with an id set to avoid O(n²) list scans.Maintain a companion set and guard appends by id.
- if ra not in requirement_assessment_objects_to_update: - requirement_assessment_objects_to_update.append(ra) + if ra.id not in requirement_assessment_objects_to_update_ids: + requirement_assessment_objects_to_update.append(ra) + requirement_assessment_objects_to_update_ids.add(ra.id)Outside this hunk, initialize once before loops:
requirement_assessment_objects_to_update_ids = set()
846-860: Good: include is_scored in bulk update and fix dynamic IG sync after bulk_update.This addresses post-bulk consistency for dynamic frameworks. LGTM.
🧹 Nitpick comments (4)
frontend/src/lib/components/Modals/ChoicesModal.svelte (1)
14-22: Consider stronger typing for the parent prop.Line 15 types
parentasany, which bypasses type safety. If the parent is expected to have specific methods likeonConfirm()(used on line 71), consider defining an interface:interface ModalParent { onConfirm: () => void; } interface Props { parent: ModalParent; // ... other props }backend/library/views.py (1)
246-251: Consider more specific exception handling.Lines 246-251 catch a generic
Exception, which may hide specific error types. Consider catching more specific exceptions (e.g.,ValueError,IntegrityError) to provide better error messages and debugging context.Additionally, line 249 has a comment noting the error message needs translation. Consider using a translatable key:
- {"error": "Failed to load library"}, # This must translated + {"error": "libraryLoadFailed"},backend/core/models.py (2)
681-681: Use id-set for membership (O(1)) instead of model-in-list (O(n)).Precompute once: ca_ids_to_update = {ca.id for ca in compliance_assessments_to_update} and test against ra.compliance_assessment_id.
- and ra.compliance_assessment in compliance_assessments_to_update + and ra.compliance_assessment_id in ca_ids_to_updateAdd (outside this hunk):
ca_ids_to_update = {ca.id for ca in compliance_assessments_to_update}
733-735: Skip unconditional RA write when there are no questions.This causes needless bulk_update writes. Remove the append here; scoring branch already queued changes when needed.
- if ra not in requirement_assessment_objects_to_update: - requirement_assessment_objects_to_update.append(ra) continue
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
backend/core/models.py(9 hunks)backend/library/views.py(3 hunks)frontend/messages/en.json(1 hunks)frontend/src/lib/components/Modals/ChoicesModal.svelte(1 hunks)frontend/src/lib/components/ModelTable/LibraryActions.svelte(3 hunks)frontend/src/routes/(app)/(internal)/loaded-libraries/[id=uuid]/+page.server.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
frontend/src/routes/(app)/(internal)/loaded-libraries/[id=uuid]/+page.server.ts (2)
frontend/src/lib/utils/constants.ts (1)
BASE_API_URL(4-8)frontend/src/lib/utils/i18n.ts (1)
safeTranslate(88-90)
backend/library/views.py (1)
backend/core/models.py (5)
StoredLibrary(242-365)LoadedLibrary(1027-1137)LibraryUpdater(368-1024)update(1033-1045)ScoreChangeDetected(369-388)
backend/core/models.py (1)
backend/core/utils.py (1)
update_selected_implementation_groups(693-725)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: functional-tests (3.12, chromium, tests/functional/domain-import.test.ts)
- GitHub Check: functional-tests (3.12, chromium, tests/functional/domain-import.test.ts)
- GitHub Check: functional-tests (3.12, chromium, tests/functional/domain-import.test.ts)
- GitHub Check: functional-tests (3.12, chromium, tests/functional/domain-import.test.ts)
- GitHub Check: functional-tests (3.12, chromium, tests/functional/domain-import.test.ts)
- GitHub Check: functional-tests (3.12, chromium, tests/functional/domain-import.test.ts)
- GitHub Check: functional-tests (3.12, chromium, tests/functional/domain-import.test.ts)
- GitHub Check: functional-tests (3.12, chromium, tests/functional/domain-import.test.ts)
- GitHub Check: functional-tests (3.12, chromium, tests/functional/domain-import.test.ts)
- GitHub Check: functional-tests (3.12, chromium, tests/functional/domain-import.test.ts)
🔇 Additional comments (15)
frontend/src/routes/(app)/(internal)/loaded-libraries/[id=uuid]/+page.server.ts (1)
14-14: LGTM! Response handling correctly updated.The changes properly:
- Parse JSON responses instead of text (line 14)
- Handle the new score-change conflict case with a 409 status and structured error data (lines 17-22)
- Use safe translation for error messages (line 26)
These align well with the backend changes that introduce strategy-driven updates and the ScoreChangeDetected exception.
Also applies to: 17-26
frontend/src/lib/components/ModelTable/LibraryActions.svelte (2)
117-131: LGTM! Enhanced update flow with modal integration.The form enhancement correctly:
- Accepts the form parameter for proper integration (line 117)
- Checks for score_change_detected in the result (line 124)
- Triggers the choices modal with the provided strategies (line 125)
- Maintains existing table invalidation behavior (lines 127-129)
This provides a good user experience for handling score boundary changes.
134-134: LGTM! Icon styling updated.The icon classes use appropriate Tailwind CSS syntax for styling the update indicator.
frontend/src/lib/components/Modals/ChoicesModal.svelte (3)
41-44: LGTM! Form setup is correct.The superForm configuration properly:
- Sets dataType to 'json' for structured data
- Generates a unique form ID using crypto.randomUUID()
59-72: Verify form submission behavior after action modification.Lines 60-71 have an unusual flow:
- Line 60: preventDefault() stops the form submission
- Line 70: Modifies the form action by appending the query parameter
- Line 71: Calls parent.onConfirm()
It's unclear if the form is meant to submit after these operations. If the form should submit, you may need to explicitly call
form.submit()after modifying the action. If not, the preventDefault is correct but the action modification on line 70 may be unnecessary.Consider clarifying the intended behavior:
onclick={(e) => { e.preventDefault(); // Modify action e.currentTarget.form.action += `&action=${choice.action}`; // If submission is needed: e.currentTarget.form.submit(); // Or if parent.onConfirm handles it: parent.onConfirm(); }}
74-84: LGTM! Tooltip integration is well-implemented.The Tooltip component correctly:
- Conditionally renders only when description exists (line 74)
- Uses safeTranslate for internationalization (line 82)
- Provides proper positioning and styling
backend/library/views.py (4)
22-22: LGTM! LibraryUpdater import added.The import is necessary for accessing LibraryUpdater.ScoreChangeDetected exception in the update flows.
496-503: LGTM! Strategy validation is well-implemented.The validation correctly:
- Retrieves the action parameter (line 496)
- Validates against allowed strategies (line 497)
- Returns a clear error message with appropriate 400 status (lines 498-503)
533-539: LGTM! Response normalization provides consistency.The normalized response structure ensures consistent API behavior:
- Returns
{"status": "success"}on successful updates (line 534)- Returns
{"status": "error", "error": error_msg}with 400 status for errors (lines 536-539)
511-532: LGTM! Exception handling is comprehensive.The try/except block properly handles the ScoreChangeDetected exception with a 409 status and structured response (lines 513-526), returning all necessary context: framework_urn, prev_scores, new_scores, affected_assessments, and strategies. Generic exceptions are logged and handled with a 422 status (lines 527-532).
Verification confirmed StoredLibraryViewSet does not have an _update method, so no consistency check is needed between viewsets.
frontend/messages/en.json (1)
2609-2616: Verify frontend-to-backend strategy name mapping.The localization keys follow correct naming conventions (camelCase for frontend), and the backend expects "rule_of_three" as the strategy value. However, the implementation of how these choices are presented to users and what action values are submitted to the backend cannot be verified from the current codebase.
Ensure that when a user selects a strategy choice in ChoicesModal, the
actionfield submitted to the backend is"rule_of_three"(not"ruleOfThree"), and similarly"clamp"and"reset"for the other strategies.backend/core/models.py (4)
369-389: Nice: explicit exception for score-boundary changes.Clean shape and payload for the UI flow. LGTM.
390-399: Strategy plumbed through LibraryUpdater.init.Constructor and storage look good; matches frontend flow.
575-614: Score change detection flow reads well.Detection + early exception with affected CAs is sound. LGTM.
1033-1045: Public API: LoadedLibrary.update(strategy=...).Signature change and propagation to LibraryUpdater look correct.
frontend/src/routes/(app)/(internal)/loaded-libraries/[id=uuid]/+page.server.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
backend/core/models.py (2)
511-521: Make score-change detection atomic to avoid partial writes.
ScoreChangeDetected can be raised after Framework.update_or_create, leaving Framework updated while CA/RA are not, causing inconsistent bounds. Wrap per-framework work in a transaction and raise before any write or rely on rollback.Apply:
- for new_framework in self.new_frameworks: + for new_framework in self.new_frameworks: + with transaction.atomic(): requirement_nodes = new_framework["requirement_nodes"] framework_dict = {**new_framework} del framework_dict["requirement_nodes"] - prev_fw = Framework.objects.filter(urn=framework_dict["urn"]).first() + prev_fw = Framework.objects.filter(urn=framework_dict["urn"]).first() prev_min = getattr(prev_fw, "min_score", None) prev_max = getattr(prev_fw, "max_score", None) prev_def = getattr(prev_fw, "scores_definition", None) + # Pre-check score changes before writing Framework + pre_new_min = framework_dict.get("min_score") + pre_new_max = framework_dict.get("max_score") + pre_new_def = framework_dict.get("scores_definition") + pre_changed = (prev_min != pre_new_min) or (prev_max != pre_new_max) or (prev_def != pre_new_def) - new_framework, _ = Framework.objects.update_or_create( + new_framework, _ = Framework.objects.update_or_create( urn=new_framework["urn"], defaults=framework_dict, create_defaults={ ... }, ) @@ - score_boundaries_changed = ( - prev_min != new_framework.min_score - or prev_max != new_framework.max_score - or prev_def != new_framework.scores_definition - ) + score_boundaries_changed = pre_changed @@ - if ( - score_boundaries_changed - and self.strategy is None - and compliance_assessments - ): + if score_boundaries_changed and self.strategy is None and compliance_assessments: ... if affected_cas: - raise self.ScoreChangeDetected(...) + # raising inside atomic ensures the Framework write rolls back + raise self.ScoreChangeDetected(...)Also applies to: 575-614
775-816: Minor: avoid shadowing builtins and keep naming clear.
Rename local variable “type” to “q_type”.- type = question.get("type") + q_type = question.get("type") @@ - if type == "multiple_choice": + if q_type == "multiple_choice": @@ - elif type == "unique_choice": + elif q_type == "unique_choice": @@ - elif type == "text": + elif q_type == "text": @@ - elif type == "date": + elif q_type == "date":frontend/src/lib/components/ModelTable/LibraryActions.svelte (1)
66-71: Broken action URL interpolation (stored-libraries).
Literal "{library.id}" won’t interpolate in Svelte. Use expression template.- action="/stored-libraries/{library.id}?/load" + action={`/stored-libraries/${library.id}?/load`}
♻️ Duplicate comments (4)
backend/core/models.py (3)
492-501: Docstring fix looks good.
Accurately reflects self.strategy usage and options.
562-565: Nice: avoided N+1 by select_related('compliance_assessment').
This addresses the earlier N+1 concern.
615-660: Guarded CA bound sync preserves overrides; ca_bounds map is good.
Matches the “only update CAs still on previous defaults” guidance and enables O(1) lookups.frontend/src/lib/components/ModelTable/LibraryActions.svelte (1)
7-13: Good: ModalSettings type import added.
Resolves the missing type reference flagged earlier.
🧹 Nitpick comments (6)
frontend/src/routes/(app)/(internal)/loaded-libraries/[id=uuid]/+page.server.ts (1)
9-9: Consider validating the action parameter.The
actionparameter is extracted directly from the URL without validation. If the backend uses this value in routing or business logic, consider validating it against expected values to prevent unexpected behavior.Example validation:
const action = new URL(event.request.url).searchParams.get('action'); +const validActions = ['clamp', 'reset', 'rule_of_three']; +if (action && !validActions.includes(action)) { + return fail(400, { error: 'invalid_action' }); +}frontend/messages/pt.json (1)
982-991: No critical issues; optional wording consistency suggestions availableJSON structure and syntax are correct. The trailing comma after "viewLess" is properly in place, and "ruleOfThreeDescription" correctly lacks a trailing comma as the final key. If desired, the Portuguese wording in the following can be refined for consistency:
"scoreChangeDetected": Consider "Detectada alteração nas pontuações" instead of "Alteração de pontuação detectada""clampDescription": Consider "Restringir os valores aos limites definidos" instead of "Restringir o valor dentro dos limites definidos"These changes are optional and purely stylistic; the current implementation is correct and functional.
frontend/messages/cs.json (1)
970-979: Maintain consistent verb forms for action labelsChange "clamp" to verb form "Omezit" for consistency with "reset": "Resetovat":
- "clamp": "Omezení", + "clamp": "Omezit",The trailing comma after "viewLess" is already correctly in place.
frontend/messages/de.json (1)
992-998: Minor terminology note on "Dreigesetz" vs. "Dreisatz".The term "Dreigesetz" for "Rule of Three" is mathematically correct but less common in German UI. Consider whether "Dreisatz" (the standard German mathematical term) would be more familiar to end users, depending on your audience's technical background.
backend/core/models.py (1)
569-575: Deduplicate RA updates; avoid O(n²) list membership checks.
Track ids in a companion set for O(1) dedupe.- requirement_assessment_objects_to_update = [] + requirement_assessment_objects_to_update = [] + _ra_update_ids = set() @@ - requirement_assessment_objects_to_update.append(ra) + if ra.id not in _ra_update_ids: + requirement_assessment_objects_to_update.append(ra) + _ra_update_ids.add(ra.id) @@ - requirement_assessment_objects_to_update.append(ra) + if ra.id not in _ra_update_ids: + requirement_assessment_objects_to_update.append(ra) + _ra_update_ids.add(ra.id) @@ - requirement_assessment_objects_to_update.append(ra) + if ra.id not in _ra_update_ids: + requirement_assessment_objects_to_update.append(ra) + _ra_update_ids.add(ra.id) @@ - if ra not in requirement_assessment_objects_to_update: - requirement_assessment_objects_to_update.append(ra) + if ra.id not in _ra_update_ids: + requirement_assessment_objects_to_update.append(ra) + _ra_update_ids.add(ra.id) @@ - if ra not in requirement_assessment_objects_to_update: - requirement_assessment_objects_to_update.append(ra) + if ra.id not in _ra_update_ids: + requirement_assessment_objects_to_update.append(ra) + _ra_update_ids.add(ra.id)Also applies to: 699-756, 757-760, 829-831
frontend/src/lib/components/ModelTable/LibraryActions.svelte (1)
25-33: Optional: type choices for clarity.
Use a precise type for choices.- function choicesModal(choices: object[]): void { + type Choice = { name: string; description: string; action: string }; + function choicesModal(choices: Choice[]): void {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
backend/core/models.py(9 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/es.json(1 hunks)frontend/messages/fr.json(1 hunks)frontend/messages/hi.json(1 hunks)frontend/messages/hr.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/ModelTable/LibraryActions.svelte(3 hunks)frontend/src/routes/(app)/(internal)/loaded-libraries/[id=uuid]/+page.server.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-04-06T13:31:28.465Z
Learnt from: ab-smith
Repo: intuitem/ciso-assistant-community PR: 1805
File: frontend/messages/de.json:0-0
Timestamp: 2025-04-06T13:31:28.465Z
Learning: The "youCanSetPAsswordHere" key is an important translation that should be maintained in the language files for the CISO Assistant application.
Applied to files:
frontend/messages/nl.json
🧬 Code graph analysis (2)
frontend/src/routes/(app)/(internal)/loaded-libraries/[id=uuid]/+page.server.ts (2)
frontend/src/lib/utils/constants.ts (1)
BASE_API_URL(4-8)frontend/src/lib/utils/i18n.ts (1)
safeTranslate(88-90)
backend/core/models.py (1)
backend/core/utils.py (1)
update_selected_implementation_groups(693-725)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: test (3.12)
- GitHub Check: build_community_frontend
- GitHub Check: build_enterprise_frontend
- GitHub Check: build (3.12)
- GitHub Check: Analyze (python)
🔇 Additional comments (18)
frontend/messages/el.json (1)
1679-1687: JSON structure and translation keys look good.The new Greek translations for the score change detection feature follow the established naming conventions and proper JSON syntax. The trailing comma placement is correct, and the key-description pairing pattern is consistent with the rest of the file.
Please verify that:
- The Greek translations accurately convey the intended meaning of their English counterparts (e.g., confirm "Περιορισμός" correctly represents "clamp", "Κανόνας των Τριών" represents "rule of three")
- These translation keys are actually referenced by the frontend components that implement the score change detection UI
frontend/messages/hr.json (1)
1949-1957: Translations look good. All eight new Croatian translation keys are accurate, properly formatted, and consistent with the existing locale file structure. The terminology is contextually appropriate for the score-change handling feature (clamp, reset, rule of three), and the descriptions follow the established pattern. JSON validity is maintained with correct comma placement.frontend/messages/pl.json (1)
1722-1730: All new Polish translations properly added and consistent across all locales.Verification confirms all eight new translation keys (scoreChangeDetected, scoreChangeDetectedDescription, clamp, clampDescription, reset, resetDescription, ruleOfThree, ruleOfThreeDescription) are present across all 21 locale files with valid JSON syntax. The Polish translations follow the established pattern and are complete.
frontend/messages/id.json (1)
1167-1175: All new localization keys are correctly added across all 21 locale files with perfect consistency.Verification confirms that all nine new keys (
viewLess,scoreChangeDetected,scoreChangeDetectedDescription,clamp,clampDescription,reset,resetDescription,ruleOfThree,ruleOfThreeDescription) are present in all 21 locale files. The JSON structure is valid, naming conventions are consistent with existing entries, and the localization system maintains structural integrity across the entire codebase.frontend/messages/hi.json (1)
951-959: Localization entries properly formatted and verified across all locales.The eight new translation keys for score change detection strategies are correctly added to hi.json with proper JSON syntax and consistent formatting. Verification confirms these same 8 keys are present across all 21 locale files (ar, cs, da, de, el, en, es, fr, hr, hu, id, it, nl, pl, pt, ro, sv, tr, uk, ur), ensuring consistent i18n coverage.
frontend/src/routes/(app)/(internal)/loaded-libraries/[id=uuid]/+page.server.ts (1)
14-14: Good improvement to useres.json()for JSON responses.This change properly handles JSON parsing and is more idiomatic than the previous text-based approach.
frontend/messages/sv.json (3)
976-981: Validate strategy descriptions match their functional behavior.Three score-update strategies are introduced: "clamp" (begränsa värdet inom de definierade gränserna), "reset" (återställ revisionernas poäng), and "ruleOfThree" (tillämpa trets regler). Ensure each description accurately reflects what the backend strategy does so users can make informed decisions.
973-981: Localization consistency verified across all 21 supported languages.All eight new translation keys (scoreChangeDetected, scoreChangeDetectedDescription, clamp, clampDescription, reset, resetDescription, ruleOfThree, ruleOfThreeDescription) have been confirmed present in every supported locale file, including sv.json and the remaining 20 languages (ar, cs, da, de, el, en, es, fr, hi, hr, hu, id, it, nl, pl, pt, ro, tr, uk, ur). The localization update is complete and consistent.
974-975: Translation is accurate and properly aligned.The Swedish translation correctly conveys the feature intent. "Revisioner" is the established term in this codebase for audits, terminology aligns with related descriptions ("Återställ revisionernas poäng"), and the tone matches other UI descriptions in sv.json. The phrasing clearly communicates to Swedish users that they must select a handling strategy for score updates.
frontend/messages/hu.json (1)
982-991: No issues found; JSON is valid and trailing comma is correctly in place.Verification confirms the JSON syntax is correct and the trailing comma after "viewLess" is present. The Hungarian translation suggestions in the original comment are marked as optional improvements; they remain stylistic choices rather than required fixes.
frontend/messages/da.json (1)
1262-1271: Optional Danish phrasing refinements (verified)The Danish plural of "score" is "scorer", and "Reglen om tre" is more natural Danish phrasing than "Regel af Tre". The trailing comma after "viewLess" is already present, so no change needed there.
Optional improvements:
- Change "scores" to "scorer" in scoreChangeDetectedDescription
- Change "Regel af Tre" to "Reglen om tre"
- Simplify "scoreopdateringerne" to "opdateringer" (less specific but clearer)
frontend/messages/uk.json (1)
1496-1504: All JSON structure and formatting verified as correct.The file is valid JSON with proper comma placement:
- "viewLess" has the required trailing comma
- All subsequent entries are properly comma-separated
- The final entry correctly lacks a trailing comma before the closing brace
No changes needed.
frontend/messages/tr.json (1)
1715-1723: LGTMTurkish translations are clear and consistent with existing terminology.
frontend/messages/it.json (1)
1359-1367: LGTMItalian entries are accurate and consistent with prior wording.
frontend/messages/fr.json (1)
2567-2574: New localization keys added for score change detection strategy.These eight new keys support the score change detection feature introduced in this PR. French translations are contextually appropriate:
- "Changement de score détecté" and description clearly communicate the feature
- Strategy options ("Borner", "Réinitialiser", "Règle de trois") are well-localized
No issues detected. JSON structure is valid.
frontend/messages/es.json (1)
989-997: Localization keys properly added in Spanish with appropriate comma adjustment.The comma added at line 989 correctly enables the new entries below. Spanish translations are well-aligned with the feature:
- "Cambio de puntuación detectado" / description convey score change intent clearly
- Strategy labels ("Restringir", "Restablecer", "Regla de Tres") are equivalent to French and contextually sound
- All eight keys match the corresponding French file, maintaining i18n symmetry
JSON syntax is valid throughout.
frontend/messages/de.json (1)
990-998: All translation keys are consistent across all 22 locale files.Verification confirms that all 8 new translation keys (scoreChangeDetected, scoreChangeDetectedDescription, clamp, clampDescription, reset, resetDescription, ruleOfThree, ruleOfThreeDescription) are present in all 22 locale files (ar, cs, da, de, el, en, es, fr, hi, hr, hu, id, it, nl, pl, pt, ro, sv, tr, uk, ur). The multilingual consistency is maintained correctly, and the de.json changes are part of a complete, coordinated update across all supported languages.
frontend/src/lib/components/ModelTable/LibraryActions.svelte (1)
24-41: Code correctly passes themessageprop to ChoicesModal.Verification confirms ChoicesModal.svelte defines
message?: string;in its Props interface and uses it in the template with<span>{message}</span>. LibraryActions.svelte correctly passes this prop. Thedescriptionfield exists only within theChoiceinterface for individual choice items, not as a top-level component prop.
frontend/src/routes/(app)/(internal)/loaded-libraries/[id=uuid]/+page.server.ts
Show resolved
Hide resolved
Mohamed-Hacene
requested review from
ab-smith,
eric-intuitem and
nas-tabchiche
Mohamed-Hacene
removed request for
ab-smith,
eric-intuitem and
nas-tabchiche
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
backend/core/models.py (2)
773-843: Recompute IGs only when answers actually changed; capture precise CA set.Normalize answers into a new dict, compare with old, only mark RA/CA when changed; then use answers_changed_ca_ids after bulk_update. This prevents unnecessary IG recomputation. (Previously flagged.)
Apply:
- requirement_assessment_objects_to_update = [] + requirement_assessment_objects_to_update = [] + answers_changed_ca_ids = set() @@ - answers = ra.answers or {} + old_answers = ra.answers or {} + answers = dict(old_answers) if isinstance(old_answers, dict) else {} @@ - ra.answers = answers - if ra.id not in requirement_assessment_objects_to_update_ids: - requirement_assessment_objects_to_update.append(ra) - requirement_assessment_objects_to_update_ids.add(ra.id) + if answers != old_answers: + ra.answers = answers + if ra.id not in requirement_assessment_objects_to_update_ids: + requirement_assessment_objects_to_update.append(ra) + requirement_assessment_objects_to_update_ids.add(ra.id) + answers_changed_ca_ids.add(ra.compliance_assessment_id) @@ - # Keep selected_implementation_groups consistent for dynamic frameworks - if new_framework.is_dynamic(): - impacted_ca_ids = { - ra.compliance_assessment_id - for ra in requirement_assessment_objects_to_update - if ra.answers is not None # answers potentially changed - } - for ca in ComplianceAssessment.objects.filter( - id__in=impacted_ca_ids - ): - update_selected_implementation_groups(ca) + # Keep selected_implementation_groups consistent for dynamic frameworks + if new_framework.is_dynamic(): + for ca in ComplianceAssessment.objects.filter( + id__in=answers_changed_ca_ids + ): + update_selected_implementation_groups(ca)Based on learnings
Also applies to: 879-896
738-759: Critical: rule_of_three path doesn’t persist score.The success path sets ra.score but never appends RA to the update list → bulk_update won’t save it.
Apply:
- elif self.strategy == "rule_of_three": + elif self.strategy == "rule_of_three": + orig_score = ra.score @@ - scaled = ca_min + (normalized * (ca_max - ca_min)) - # Round to int and clamp into [ca_min, ca_max] - ra.score = max(min(int(round(scaled)), ca_max), ca_min) + scaled = ca_min + (normalized * (ca_max - ca_min)) + # Round to int and clamp into [ca_min, ca_max] + new_score = max(min(int(round(scaled)), ca_max), ca_min) + if new_score != orig_score: + ra.score = new_score + requirement_assessment_objects_to_update.append(ra)
🧹 Nitpick comments (3)
backend/core/models.py (3)
380-400: Strategy identifiers: align 'name' vs 'action' for FE contract.You expose name="ruleOfThree" while action="rule_of_three". Verify FE expects this mix; otherwise standardize to one canonical identifier everywhere.
712-716: Use id-set membership; object-in-list check is O(n).Switch to ids for O(1) membership and clarity.
- if ( - ra.is_scored - and ra.score is not None - and ra.compliance_assessment in compliance_assessments_to_update - ): + if ( + ra.is_scored + and ra.score is not None + and ra.compliance_assessment_id in ca_ids_to_update + ):And define the set once after building compliance_assessments_to_update:
+ ca_ids_to_update = {ca.id for ca in compliance_assessments_to_update}
580-585: Deduplicate RA updates with an id set to avoid O(n²) list scans.Multiple append sites can add the same RA several times. Track ids alongside the list.
- requirement_assessment_objects_to_update = [] + requirement_assessment_objects_to_update = [] + requirement_assessment_objects_to_update_ids = set() @@ - requirement_assessment_objects_to_update.append(ra) + if ra.id not in requirement_assessment_objects_to_update_ids: + requirement_assessment_objects_to_update.append(ra) + requirement_assessment_objects_to_update_ids.add(ra.id) @@ - requirement_assessment_objects_to_update.append(ra) + if ra.id not in requirement_assessment_objects_to_update_ids: + requirement_assessment_objects_to_update.append(ra) + requirement_assessment_objects_to_update_ids.add(ra.id) @@ - if ra not in requirement_assessment_objects_to_update: - requirement_assessment_objects_to_update.append(ra) + if ra.id not in requirement_assessment_objects_to_update_ids: + requirement_assessment_objects_to_update.append(ra) + requirement_assessment_objects_to_update_ids.add(ra.id) @@ - ra.answers = answers - if ra not in requirement_assessment_objects_to_update: - requirement_assessment_objects_to_update.append(ra) + ra.answers = answers + if ra.id not in requirement_assessment_objects_to_update_ids: + requirement_assessment_objects_to_update.append(ra) + requirement_assessment_objects_to_update_ids.add(ra.id)Also applies to: 736-767, 769-771, 839-842
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/core/models.py(9 hunks)backend/library/views.py(3 hunks)frontend/messages/en.json(1 hunks)frontend/messages/fr.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/messages/en.json
- backend/library/views.py
🧰 Additional context used
🧬 Code graph analysis (1)
backend/core/models.py (1)
backend/core/utils.py (1)
update_selected_implementation_groups(693-725)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: build (3.12)
- GitHub Check: build_enterprise_frontend
- GitHub Check: build_community_frontend
- GitHub Check: test (3.12)
- GitHub Check: Analyze (python)
🔇 Additional comments (4)
frontend/messages/fr.json (1)
2585-2592: Translation keys properly added for score-change handling feature.The eight new French translation keys are well-formatted and contextually appropriate for the score-change detection feature. JSON syntax is valid, naming conventions are consistent with the file, and keys are logically placed in the file structure.
Ensure that all other locale files (en.json, es.json, it.json, etc.) have been updated with the corresponding translations for these eight keys to maintain consistency across supported languages and prevent missing key errors at runtime.
backend/core/models.py (3)
503-512: Docstring now matches runtime behavior.Good correction: update_frameworks documents self.strategy and allowed values.
573-576: N+1 avoided on RA fetch.select_related("compliance_assessment") is the right fix here.
1069-1081: Strategy is correctly threaded through update().LoadedLibrary.update accepts strategy and passes it to LibraryUpdater. Good.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
backend/core/models.py (2)
627-672: Initialize ca_bounds and precompute updated_ca_ids for O(1) membership.
- Make ca_bounds always defined to avoid future refactor pitfalls.
- Build updated_ca_ids once and use id membership in the RA loop.
- compliance_assessments_to_update = [] + compliance_assessments_to_update = [] + ca_bounds = {} # ensure defined even if no CA is updated @@ - if compliance_assessments_to_update: + if compliance_assessments_to_update: ComplianceAssessment.objects.bulk_update( compliance_assessments_to_update, ["min_score", "max_score", "scores_definition"], batch_size=100, ) - ca_bounds = { + ca_bounds = { ca.id: ( ( ca.min_score @@ for ca in compliance_assessments_to_update } + + # For fast inclusion checks in RA loop + updated_ca_ids = {ca.id for ca in compliance_assessments_to_update}Based on learnings.
843-847: Only recompute IGs when answers changed — LGTManswers_changed_ca_ids is precise; bulk_update includes is_scored.
Also applies to: 885-896
🧹 Nitpick comments (4)
backend/core/models.py (4)
573-576: Prevent N+1 on ra.requirement (select the FK).Accessing ra.requirement to key the dict triggers a query per RA. Select it up front.
- for ra in RequirementAssessment.objects.filter( - requirement__framework=new_framework - ).select_related("compliance_assessment"): + for ra in ( + RequirementAssessment.objects + .filter(requirement__framework=new_framework) + .select_related("compliance_assessment", "requirement") + ):
713-717: Avoid O(n) CA membership checks per RA.Use id-set membership built once instead of linear list scans.
- if ( - ra.is_scored - and ra.score is not None - and ra.compliance_assessment in compliance_assessments_to_update - ): + if ( + ra.is_scored + and ra.score is not None + and ra.compliance_assessment_id in updated_ca_ids + ):
770-773: Dedupe RA updates without O(n) scans; avoid no-op appends.Current list-membership check is O(n). Track ids to dedupe in O(1) and avoid appending unchanged RAs.
- requirement_assessment_objects_to_update = [] + requirement_assessment_objects_to_update = [] + requirement_assessment_update_ids = set() @@ - if ra not in requirement_assessment_objects_to_update: - requirement_assessment_objects_to_update.append(ra) + if ra.id not in requirement_assessment_update_ids: + requirement_assessment_objects_to_update.append(ra) + requirement_assessment_update_ids.add(ra.id)
791-793: Don’t shadow built-in type().Rename local variable to avoid shadowing and improve readability.
- type = question.get("type") + qtype = question.get("type") @@ - if type == "multiple_choice": + if qtype == "multiple_choice": @@ - elif type == "unique_choice": + elif qtype == "unique_choice": @@ - elif type == "text": + elif qtype == "text": @@ - elif type == "date": + elif qtype == "date":Also applies to: 808-809, 820-832
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/core/models.py(8 hunks)frontend/messages/nl.json(1 hunks)frontend/messages/ro.json(1 hunks)frontend/src/lib/components/ModelTable/LibraryActions.svelte(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/src/lib/components/ModelTable/LibraryActions.svelte
- frontend/messages/ro.json
🧰 Additional context used
🧬 Code graph analysis (1)
backend/core/models.py (1)
backend/core/utils.py (1)
update_selected_implementation_groups(693-725)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: build (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: test (3.12)
- GitHub Check: build_community_frontend
- GitHub Check: build_enterprise_frontend
- GitHub Check: Analyze (python)
🔇 Additional comments (6)
frontend/messages/nl.json (1)
985-993: ✅ Critical issue resolved: French translations replaced with proper Dutch.The French translations that were flagged in the previous review have been successfully corrected. All nine new entries (lines 985–993) now contain proper Dutch text. The feature strings for score-change handling are now localized correctly for Dutch users.
Minor note on consistency: The wording in a few entries varies slightly from the suggested translations in the past review (e.g., "Resetten" instead of "Opnieuw instellen" for the
resetkey, and minor phrasing differences in descriptions). However, all current translations are grammatically sound Dutch and preserve the intended meaning. If you'd prefer to align exactly with the previously suggested wording for consistency with translation guidelines, you're welcome to adjust these entries; otherwise, the current translations are acceptable.backend/core/models.py (5)
502-512: Docstring now reflects self.strategy — LGTMClear and accurate; matches the implementation.
739-756: rule_of_three: int rounding + clamp + persistence — LGTMCorrectly casts to int, clamps to CA bounds, and appends RA for update.
Also applies to: 758-762, 763-769
595-626: Score-change detection and UX handshake — LGTMRaising ScoreChangeDetected with prev/new and affected CAs aligns backend/frontend contract.
380-401: Public exception payload looks good.Fields and strategies cover the flow cleanly.
1069-1081: Strategy propagation from LoadedLibrary.update — LGTMAPI surface is backward compatible; defaults preserved.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
backend/core/models.py (2)
583-589: Deduplicate RA updates; avoid unconditional appends.Multiple append paths can add the same RA several times (O(n²) bulk_update). Keep a companion id-set and use a helper to enqueue once. Also avoid updating RAs with no questions unless a field actually changed.
Apply:
requirement_assessment_objects_to_create = [] - requirement_assessment_objects_to_update = [] + requirement_assessment_objects_to_update = [] + requirement_assessment_objects_to_update_ids = set() answers_changed_ca_ids = set() requirement_node_objects_to_update = [] @@ + def _enqueue_ra_update(ra): + if ra.id not in requirement_assessment_objects_to_update_ids: + requirement_assessment_objects_to_update.append(ra) + requirement_assessment_objects_to_update_ids.add(ra.id) @@ - requirement_assessment_objects_to_update.append(ra) + _enqueue_ra_update(ra) @@ - requirement_assessment_objects_to_update.append(ra) + _enqueue_ra_update(ra) @@ - requirement_assessment_objects_to_update.append(ra) + _enqueue_ra_update(ra) @@ - if ra not in requirement_assessment_objects_to_update: - requirement_assessment_objects_to_update.append(ra) + # Only enqueue if something else in this loop modified the RA + _enqueue_ra_update(ra) @@ - requirement_assessment_objects_to_update.append(ra) + _enqueue_ra_update(ra)Also applies to: 744-745, 764-764, 777-779, 782-784, 861-863
804-847: Avoid shadowing builtintype; useq_type.Shadowing builtins is error-prone and harms readability.
Apply:
- type = question.get("type") + q_type = question.get("type") @@ - if type == "multiple_choice": + if q_type == "multiple_choice": @@ - elif type == "unique_choice": + elif q_type == "unique_choice": @@ - elif type == "text": + elif q_type == "text": @@ - elif type == "date": + elif q_type == "date":
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/core/models.py(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
backend/core/models.py (1)
backend/core/utils.py (1)
update_selected_implementation_groups(693-725)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: build (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: 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: Analyze (python)
🔇 Additional comments (1)
backend/core/models.py (1)
739-779: Clarify refactoring intent:_enqueue_ra_updatefunction does not exist in the codebase.The review references introducing
_enqueue_ra_updateto centralize RA updates and prevent duplicates, but this function is not present anywhere inbackend/. The verification cannot proceed as stated since it's conditional on this function's existence.Current state: Direct appends to
requirement_assessment_objects_to_updateexist at lines 744, 764, 770, 779, 783, and 861. Line 782–783 already implements a defensive deduplication pattern (if ra not in requirement_assessment_objects_to_update:), but this is inconsistent across the codebase.Clarify: Should this PR introduce
_enqueue_ra_updateas a helper to consolidate all RA mutations, or is the current implementation acceptable as-is?
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
frontend/messages/nl.json (1)
985-992: Missing Dutch translation key: "reset"The new score-change feature translations are mostly complete, but the
"reset"key is missing. While"resetDescription"is present on line 990, the base translation key"reset"(which should map to"Opnieuw instellen") is absent. This will cause the frontend UI to display an empty label or fallback text when rendering the reset strategy option.Add the missing translation between lines 989 and 990:
"clampDescription": "Beperk de waarde binnen de gedefinieerde grenzen", + "reset": "Opnieuw instellen", "resetDescription": "Reset de scores van de audits",frontend/messages/ar.json (1)
950-957: Refine Arabic wording in clampDescription (duplicate of past review).Line 953 uses "المعرفة" which should be replaced with "المحددة" for clearer wording. The previous review flagged this, but the correction has not been applied.
- "clampDescription": "تقييد القيمة ضمن الحدود المعرفة", + "clampDescription": "تقييد القيمة ضمن الحدود المحددة",backend/core/models.py (2)
553-556: Avoidset()on QuerySet: Django model instances are unhashable → runtimeTypeErrorWrapping
LoadedLibrary.objects.filter(urn__in=involved_library_urns)inset()will fail at runtime because Django model instances are unhashable. You only need an iterable forlibrary__in=…, so the raw queryset is sufficient.Proposed fix:
- involved_library_urns = [*self.dependencies, self.old_library.urn] - involved_libraries = set( - LoadedLibrary.objects.filter(urn__in=involved_library_urns) - ) + involved_library_urns = [*self.dependencies, self.old_library.urn] + involved_libraries = LoadedLibrary.objects.filter( + urn__in=involved_library_urns + )If you also need a hashable container later, derive it from primitive fields (e.g.
values_list("id", flat=True)), not model instances.#!/bin/bash # Quick sanity check: ensure no other set(queryset) patterns remain rg -n "set\(\s*[^)]*objects\.filter" backend || true
705-712: Guard against nullableComplianceAssessment.perimeterwhen creating new RAs (and avoid N+1)
ComplianceAssessment.perimeteris declared nullable; usingca.perimeter.folderdirectly when creating newRequirementAssessmentinstances can raiseAttributeErrorif a CA exists without a perimeter, and also causes a potential N+1 onperimeter.Consider:
- for ca in compliance_assessments: - requirement_assessment_objects_to_create.append( - RequirementAssessment( - compliance_assessment=ca, - requirement=requirement_node_object, - folder=ca.perimeter.folder, - answers=transform_questions_to_answers(questions) - if questions - else {}, - ) - ) + for ca in compliance_assessments: + requirement_assessment_objects_to_create.append( + RequirementAssessment( + compliance_assessment=ca, + requirement=requirement_node_object, + folder=( + ca.folder + or ( + ca.perimeter.folder + if ca.perimeter_id + else Folder.get_root_folder() + ) + ), + answers=transform_questions_to_answers(questions) + if questions + else {}, + ) + )Optionally, to avoid N+1s on
folder/perimeter, you can fetchcompliance_assessmentswith:- compliance_assessments = [ - *ComplianceAssessment.objects.filter(framework=new_framework) - ] + compliance_assessments = [ + *ComplianceAssessment.objects.filter(framework=new_framework) + .select_related("folder", "perimeter") + ]
🧹 Nitpick comments (1)
backend/core/models.py (1)
786-863: Dynamic IG recomputation only tracks answer JSON changes; may miss pure definition changesThe new
answers_changed_ca_idsset is a good optimization over the previous broadra.answers is not Nonecheck, but note a subtle edge case:
update_selected_implementation_groups()depends on both RA answers and the framework/question-level IG definitions (select_implementation_groupsin choices andimplementation_groups_definition).- If a library update changes IG mappings or default selections without changing the stored answers (same choice URNs),
answers != old_answersstays false, soanswers_changed_ca_idsremains empty andupdate_selected_implementation_groups()won’t run, leavingselected_implementation_groupspotentially stale.If you need strict correctness here, consider additionally triggering recomputation when the IG definitions change (e.g., detect a change in
prev_fw.implementation_groups_definitionvsnew_framework.implementation_groups_definition, and in that case recompute IGs for all CAs of that framework, not only those inanswers_changed_ca_ids).Also applies to: 900-911
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
backend/core/models.py(4 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/hr.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)
🚧 Files skipped from review as they are similar to previous changes (13)
- frontend/messages/ro.json
- frontend/messages/id.json
- frontend/messages/es.json
- frontend/messages/tr.json
- frontend/messages/ur.json
- frontend/messages/cs.json
- frontend/messages/fr.json
- frontend/messages/de.json
- frontend/messages/el.json
- frontend/messages/hi.json
- frontend/messages/pl.json
- frontend/messages/en.json
- frontend/messages/sv.json
🧰 Additional context used
🧬 Code graph analysis (1)
backend/core/models.py (1)
backend/core/utils.py (1)
update_selected_implementation_groups(693-725)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: test (3.12)
- GitHub Check: build_community_frontend
- GitHub Check: build_enterprise_frontend
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: startup-docker-compose-test
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: build (3.12)
- GitHub Check: Analyze (python)
🔇 Additional comments (7)
frontend/messages/da.json (1)
1262-1269: Well-structured localization additions for score-change handling.The new translation keys follow the file's established conventions and clearly support the score-change detection feature. Key naming is consistent (label + description pairs), and the Danish translations appear appropriate for the UI context.
frontend/messages/hu.json (1)
982-989: Translation keys properly added with correct formatting.The eight new keys for score-change detection feature follow the established naming convention, JSON structure, and Hungarian translation patterns. Descriptions are contextually appropriate for the feature.
Please verify that these Hungarian translations align with their counterparts in other locale files (en.json, es.json, fr.json, etc.) to ensure consistency across the UI. Since the PR mirrors localization expansions across multiple languages, confirming translation parity will help maintain consistency.
frontend/messages/hr.json (1)
1948-1955: Localization keys correctly added with appropriate Croatian translations.The eight new keys for score-change handling are properly formatted, well-positioned after
downloadCertificate, and provide semantically coherent Croatian translations that align with the feature intent (detecting framework boundary changes and applying transformation strategies: clamp, reset, rule-of-three).frontend/messages/it.json (1)
1359-1366: Localization keys correctly added with appropriate Italian translations.The eight new keys mirror the HR translations with correct Italian equivalents. Positioning after
downloadCertificateis consistent, and translations clearly convey the score transformation strategies (clamping, resetting, and proportional scaling) needed for the framework update workflow.backend/core/models.py (3)
380-409:ScoreChangeDetectedpayload and strategy plumbing look solidEncapsulating previous/new score boundaries, affected CA IDs, and the available strategies in a dedicated exception is a clean way to surface user choices to the frontend, and the
strategyattribute onLibraryUpdater/LoadedLibrary.update()wires this through without breaking existing callers (defaultNone). No changes needed here.
590-629: Score-boundary change detection and RA transformation logic are consistent with CA overridesThe updated
update_frameworks()flow correctly:
- Captures previous framework bounds/definition before
update_or_create.- Detects boundary changes and, when
self.strategy is None, raisesScoreChangeDetectedonly if there are CAs still on the previous framework defaults, preserving user-customized CAs.- Updates CA bounds only for those still matching the previous defaults, leaving overridden CAs untouched.
- Uses a
ca_boundsmap plus per-CA gating (ra.compliance_assessment in compliance_assessments_to_update) so RA score transformations (reset / clamp / rule_of_three) only apply where CA bounds were actually updated.- Ensures
rule_of_threescaling is safe (guards against invalid old ranges, clamps to CA bounds, and writes integers).This matches the intended “strategy-driven” semantics without clobbering per-assessment overrides.
Also applies to: 630-676, 717-780
1084-1086: API change:LoadedLibrary.update(strategy=None)– recursive updates will raiseScoreChangeDetectedwithout explicit handlingThe
update()method accepts an optionalstrategyparameter and threads it toLibraryUpdater. When score boundaries change and no strategy is provided,LibraryUpdater.update_frameworks()raisesScoreChangeDetected.Verified status:
✓ User-facing entry point (backend/library/views.py:536) properly catches
ScoreChangeDetectedand returns a 409 response with framework details, score changes, affected assessments, and available strategies for the frontend modal.⚠ Recursive calls via
update_dependencies()(backend/core/models.py:471) andcheck_and_import_dependencies()(backend/library/utils.py:706) calldependency.update()without passing a strategy. When score boundaries change for dependencies,ScoreChangeDetectedwill be raised and propagate to the caller. This is intentional—the exception bubbles up to the user-facing API handler which surfaces it with strategy options. However, ensure this behavior is documented and matches the intended UX (dependencies triggering the score-change modal).
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/core/models.py (1)
445-472: Propagate strategy when updating dependencies to avoid repeated ScoreChangeDetected loopsRight now
update_dependenciesalways callsdependency.update()with the defaultstrategy=None. If a dependency library has framework score-boundary changes and existing CAs, itsupdate()will raiseScoreChangeDetected. The client then retries the main library update with a chosen strategy, but on retry this method still callsdependency.update()withstrategy=None, so the same exception is raised again and the update can never complete.You should at least propagate a concrete strategy into dependency updates (or force a safe default like
"clamp") so dependencies don’t continually block the main update once the user has made a choice.For example:
- if (error_msg := dependency.update()) not in [None, "libraryHasNoUpdate"]: + # Propagate a concrete strategy to dependencies to avoid repeated + # ScoreChangeDetected loops; default to "clamp" when none was provided. + effective_strategy = self.strategy or "clamp" + if (error_msg := dependency.update(strategy=effective_strategy)) not in [ + None, + "libraryHasNoUpdate", + ]: return error_msg
🧹 Nitpick comments (3)
frontend/messages/hi.json (1)
952-958: Polish Hindi phrasing and a small grammar fix.Avoid Hinglish and fix agreement in “सीमाओं”.
Apply this diff:
@@ - "scoreChangeDetectedDescription": "स्कोर में परिवर्तन का पता चला है। संबद्ध ऑडिट्स के लिए स्कोर अपडेट्स को कैसे हैंडल करना चुनें।", + "scoreChangeDetectedDescription": "स्कोर में परिवर्तन का पता चला है। संबंधित ऑडिट के स्कोर अपडेट्स को कैसे प्रबंधित करना है, चुनें।", @@ - "ruleOfThreeDescription": "नए सीमाओं के बीच स्कोर को स्केल करने के लिए तीन का नियम लागू करें", + "ruleOfThreeDescription": "नई सीमाओं के बीच स्कोर को स्केल करने के लिए तीन का नियम लागू करें",backend/core/models.py (2)
514-678: Score-boundary change detection and CA/RA transformation logic are correct but RA updates could be de-duplicatedThe overall flow here is solid:
- You correctly:
- Capture previous framework bounds (
prev_min/max/def).- Detect real boundary changes via
score_boundaries_changed.- Limit
ScoreChangeDetectedto CAs still on previous framework defaults (preserving user overrides).- Guard CA updates so only those still matching previous defaults are bulk-updated.
- Build
ca_boundsfrom updated CAs and use it for both clamping andrule_of_three, with sensible fallbacks to framework or 0–100.- Apply strategies only to RAs whose CA bounds have actually changed (
ra.compliance_assessment in compliance_assessments_to_update).The
rule_of_threebranch now scales into CA-specific bounds, casts toint, and clamps to[ca_min, ca_max], which matches the earlier IntegerField and range-safety concerns.One small improvement:
requirement_assessment_objects_to_updatecan accumulate the sameRequirementAssessmentmultiple times (e.g., first via score strategy, then via answers or the “no questions” branch). This is functionally safe but causes redundant work inbulk_updateand makes the IG recomputation set (answers_changed_ca_ids) slightly less obvious.You can cheaply dedupe while appending using an ID set:
- requirement_assessment_objects_to_update = [] + requirement_assessment_objects_to_update = [] + requirement_assessment_ids_to_update: set[int] = set() @@ - ra.score = None - ra.is_scored = False - requirement_assessment_objects_to_update.append(ra) + ra.score = None + ra.is_scored = False + if ra.id not in requirement_assessment_ids_to_update: + requirement_assessment_objects_to_update.append(ra) + requirement_assessment_ids_to_update.add(ra.id) @@ 768-788 (and similarly in other branches) - ra.score = max( - min(int(round(scaled)), ca_max), ca_min - ) - requirement_assessment_objects_to_update.append(ra) + ra.score = max( + min(int(round(scaled)), ca_max), ca_min + ) + if ra.id not in requirement_assessment_ids_to_update: + requirement_assessment_objects_to_update.append(ra) + requirement_assessment_ids_to_update.add(ra.id) @@ - ra.score = clamped - requirement_assessment_objects_to_update.append(ra) + ra.score = clamped + if ra.id not in requirement_assessment_ids_to_update: + requirement_assessment_objects_to_update.append(ra) + requirement_assessment_ids_to_update.add(ra.id) @@ 790-793 - if ra not in requirement_assessment_objects_to_update: - requirement_assessment_objects_to_update.append(ra) + if ra.id not in requirement_assessment_ids_to_update: + requirement_assessment_objects_to_update.append(ra) + requirement_assessment_ids_to_update.add(ra.id)This keeps the logic as-is while making the batch update strictly O(n) and easier to reason about.
Also applies to: 726-789
6480-6557: compute_score_and_result logic is sound; consider not forcing score when no scoring is definedThe helper correctly:
- Ignores non-visible questions via
_is_question_visible.- Tracks visible vs answered question counts and short-circuits to
not_applicable/not_assessedbefore computing a final result.- Aggregates
add_scorecontributions andcompute_resultbooleans across both single and multiple choice questions.- Clamps the computed score into the CA’s
[min_score, max_score]and persists with targetedupdate_fields.One nit: when no
add_scoreis ever encountered (is_score_computedstaysFalse),total_scoreremains 0 butself.scoreis still set and clamped to[min_score, max_score](often ending up atmin_score), even thoughis_scoredremainsFalseand this value will be ignored in global scoring. For clarity, you might want to explicitly clear the score in that case:- self.score = max(min(total_score, max_score), min_score) + if is_score_computed: + self.score = max(min(total_score, max_score), min_score) + else: + self.score = Noneand keep the existing
update_fieldslogic as-is. This makes the persisted data better reflect whether a numeric score was actually derived.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
backend/core/models.py(4 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/hr.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)
🚧 Files skipped from review as they are similar to previous changes (17)
- frontend/messages/da.json
- frontend/messages/ar.json
- frontend/messages/hu.json
- frontend/messages/pt.json
- frontend/messages/ro.json
- frontend/messages/de.json
- frontend/messages/id.json
- frontend/messages/nl.json
- frontend/messages/sv.json
- frontend/messages/en.json
- frontend/messages/es.json
- frontend/messages/cs.json
- frontend/messages/el.json
- frontend/messages/tr.json
- frontend/messages/uk.json
- frontend/messages/ur.json
- frontend/messages/pl.json
🧰 Additional context used
🧬 Code graph analysis (1)
backend/core/models.py (1)
backend/core/utils.py (1)
update_selected_implementation_groups(693-725)
⏰ 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). (33)
- GitHub Check: enterprise-functional-tests (3.12, chromium, tests/functional/enterprise/settings/client.test.ts)
- GitHub Check: enterprise-functional-tests (3.12, chromium, tests/functional/detailed/login.test.ts)
- GitHub Check: functional-tests (3.12, chromium, tests/functional/detailed/login.test.ts)
- GitHub Check: functional-tests (3.12, chromium, tests/functional/domain-import.test.ts)
- GitHub Check: enterprise-functional-tests (3.12, chromium, tests/functional/nav.test.ts)
- GitHub Check: enterprise-functional-tests (3.12, chromium, tests/functional/detailed/settings/sso.test.ts)
- GitHub Check: enterprise-functional-tests (3.12, chromium, tests/functional/detailed/incidents.test.ts)
- GitHub Check: enterprise-functional-tests (3.12, chromium, tests/functional/detailed/settings/general.test.ts)
- GitHub Check: enterprise-functional-tests (3.12, chromium, tests/functional/detailed/ebios-rm.test.ts)
- GitHub Check: enterprise-functional-tests (3.12, chromium, tests/functional/detailed/compliance-assessments.tes...
- GitHub Check: functional-tests (3.12, chromium, tests/functional/detailed/settings/general.test.ts)
- GitHub Check: enterprise-functional-tests (3.12, chromium, tests/functional/detailed/tprm.test.ts)
- GitHub Check: enterprise-functional-tests (3.12, chromium, tests/functional/detailed/business-impact-analysis.t...
- GitHub Check: enterprise-functional-tests (3.12, chromium, tests/functional/detailed/mappings.test.ts)
- GitHub Check: enterprise-functional-tests (3.12, chromium, tests/functional/detailed/common.test.ts)
- GitHub Check: enterprise-functional-tests (3.12, chromium, tests/functional/detailed/my-assignments.test.ts)
- GitHub Check: functional-tests (3.12, chromium, tests/functional/detailed/common.test.ts)
- GitHub Check: functional-tests (3.12, chromium, tests/functional/backup-restore.test.ts)
- GitHub Check: enterprise-functional-tests (3.12, chromium, tests/functional/user-route.test.ts)
- GitHub Check: functional-tests (3.12, chromium, tests/functional/detailed/my-assignments.test.ts)
- GitHub Check: functional-tests (3.12, chromium, tests/functional/nav.test.ts)
- GitHub Check: enterprise-functional-tests (3.12, chromium, tests/functional/detailed/findings-assessments.test.ts)
- GitHub Check: functional-tests (3.12, chromium, tests/functional/i18n.test.ts)
- GitHub Check: functional-tests (3.12, chromium, tests/functional/detailed/tprm.test.ts)
- GitHub Check: functional-tests (3.12, chromium, tests/functional/detailed/compliance-assessments.test.ts)
- GitHub Check: functional-tests (3.12, chromium, tests/functional/detailed/incidents.test.ts)
- GitHub Check: functional-tests (3.12, chromium, tests/functional/detailed/business-impact-analysis.test.ts)
- GitHub Check: functional-tests (3.12, chromium, tests/functional/user-route.test.ts)
- GitHub Check: functional-tests (3.12, chromium, tests/functional/detailed/ebios-rm.test.ts)
- GitHub Check: functional-tests (3.12, chromium, tests/functional/detailed/mappings.test.ts)
- GitHub Check: functional-tests (3.12, chromium, tests/functional/detailed/findings-assessments.test.ts)
- GitHub Check: test (3.12)
- GitHub Check: build (3.12)
🔇 Additional comments (6)
frontend/messages/hr.json (1)
1948-1960: Localization keys properly structured and complete for score-change feature. ✅All new translation keys follow the established naming convention (camelCase with paired descriptions), maintain valid JSON syntax, and align with the backend's strategy-driven score transformation feature (clamp, reset, rule-of-three). Croatian translations are consistent with the existing terminology throughout the file (e.g., "Resetiraj" aligns with other reset operations). The additions comprehensively cover the UI requirements: detection messaging, strategy descriptions, and operation status feedback.
frontend/messages/it.json (1)
1359-1366: Translations look good; no duplication found.All nine new keys (scoreChangeDetected, scoreChangeDetectedDescription, clamp, clampDescription, reset, resetDescription, ruleOfThree, ruleOfThreeDescription) are present exactly once in the file with consistent labeling and descriptions matching other locales.
frontend/messages/fr.json (1)
2672-2678: LGTM; consider a tiny wording tweak (optional).Current French reads well. Optionally singularize "scores" for style:
- "scoreChangeDetectedDescription": "Un changement de scores a été détecté. Choisissez comment gérer les mises à jour des scores pour les audits associés.", + "scoreChangeDetectedDescription": "Un changement de score a été détecté. Choisissez comment gérer les mises à jour des scores pour les audits associés.",All new keys are present across all locale files with no duplicates detected.
backend/core/models.py (3)
380-410: Strategy plumbing and API surface for score-boundary handling look coherentThe introduction of
ScoreChangeDetected, thestrategyfield onLibraryUpdater/LoadedLibrary.update, and the updatedupdate_frameworksdocstring form a consistent public contract: strategies are carried from the API down to framework updates, and the exception exposes all data the frontend needs (framework_urn, old/new bounds, affected CAs, strategy options). I don’t see functional issues here.Also applies to: 503-512, 1093-1106
553-571: Library involvement and RA creation flow look good (fixes prior N+1 / null-perimeter issues)Two notable fixes here look correct:
LoadedLibrary.objects.filter(urn__in=involved_library_urns)drops the problematicset()wrapper and feeds the queryset directly into__in, avoiding the “unhashable type: Model” issue while keeping the query efficient.- When creating new
RequirementAssessmentinstances, you now derivefolderasca.folder or (ca.perimeter.folder if ca.perimeter else Folder.get_root_folder()), which safely handles nullable perimeters and leverages the already-setAssessment.folderwhen available.No further issues spotted in this part of the flow.
Also applies to: 707-724
795-872: Answers normalization and IG recompute scope are precise and efficientThe answers-normalization block is defensive and well-scoped:
- Starts from a safe dict copy of existing answers.
- Drops answers for removed questions and initializes new/updated questions.
- Normalizes by type (multi/unique choice, text, date) and discards invalid shapes and stale choices.
- Only writes back and queues an update when
answers != old_answers.- Tracks
answers_changed_ca_idsprecisely for CAs whose answers actually changed.Combined with the dynamic-framework IG recomputation gated on
answers_changed_ca_ids, this should avoid the previous over-recompute behavior while keeping IGs consistent. This looks solid.Also applies to: 909-920
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/models.py (1)
445-472: Critical: strategy is not propagated to dependent libraries, causing repeated ScoreChangeDetected
LibraryUpdaternow accepts astrategy, butupdate_dependenciesstill callsdependency.update()without forwardingself.strategy. As a result:
- On the initial update (no strategy), dependencies correctly raise
ScoreChangeDetected.- After the user picks a strategy and retries, the root library gets the strategy, but dependencies are still updated with
strategy=None, so they keep raisingScoreChangeDetectedand the update can never complete.To align behavior and avoid this loop, propagate the chosen strategy to dependent libraries:
- if (error_msg := dependency.update()) not in [None, "libraryHasNoUpdate"]: + if (error_msg := dependency.update(self.strategy)) not in [ + None, + "libraryHasNoUpdate", + ]: return error_msgThis ensures the same chosen strategy is applied across the whole dependency graph on the second attempt, and all boundary changes are handled consistently.
🧹 Nitpick comments (1)
backend/core/models.py (1)
726-804: Strategy application to RA scores and documentation_score is correct but can add duplicatesThe
transform_valuehelper encapsulates the three strategies cleanly:
reset: always returnsNone.rule_of_three: normalizes usingprev_min/prev_max, scales into[ca_min, ca_max], casts tointand clamps, with a fallback to plain clamping when the old range is invalid.- Else: clamps directly into
[ca_min, ca_max](the “clamp” behavior).Using CA-specific bounds via
ca_bounds.get(ra.compliance_assessment_id, (default_min, default_max))and updating bothscoreanddocumentation_scoreensures IntegerField-safe, in-range values.One caveat: the same
RequirementAssessmentcan be appended multiple times torequirement_assessment_objects_to_update(e.g. once forscore, once fordocumentation_score).bulk_updatetolerates this but does extra work. Consider tracking a companionidsset (or reusing aca_ids_to_updateset for membership checks) to ensure each RA is only appended once.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/core/models.py(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
backend/core/models.py (1)
backend/core/utils.py (1)
update_selected_implementation_groups(693-725)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: build_community_frontend
- GitHub Check: build_enterprise_frontend
- GitHub Check: build (3.12)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: startup-docker-compose-test
- GitHub Check: test (3.12)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (7)
backend/core/models.py (7)
379-405: ScoreChangeDetected exception and strategy plumbing look coherentThe inner
ScoreChangeDetectedexception correctly captures framework URN, previous/new score settings, affected assessment IDs, and exposes astrategiespayload that cleanly separates displaynamefrom backendaction(clamp,reset,rule_of_three). TheLibraryUpdater.__init__change to accept and store an optionalstrategywires this metadata through without breaking existing call sites (thanks to the defaultNone). This is a solid foundation for the frontend-driven “choose a strategy then retry” flow.
502-532: Framework score-boundary detection and guarding via strategy are well-structuredThe updated
update_frameworksdocstring now reflects that score handling is driven byself.strategy, and the implementation matches it: you captureprev_min/max/defbeforeupdate_or_create, computescore_boundaries_changed, and only raiseScoreChangeDetectedwhen (a) boundaries actually changed, (b) a strategy was not provided, and (c) there are CAs still on the previous defaults. Wrapping each framework update in its owntransaction.atomic()also ensures that when the exception is raised, all changes for that framework (including deleted/updated requirement nodes) are rolled back.
567-677: CA bounds update preserves overrides and supports rule-of-three safelyThe
compliance_assessmentsqueryset now usesselect_related("folder", "perimeter"), and the CA update loop correctly:
- Identifies only CAs still on the previous framework defaults (
still_on_prev_defaults),- Updates their min/max/scores_definition when boundaries changed,
- Builds a
ca_boundsdict once afterbulk_update, resolving per-CA min/max with framework/default fallbacks and using 0/100 as a last resort.This addresses the earlier concerns about overwriting user-customized CA bounds and also provides an O(1) lookup structure for downstream RA transformations.
707-721: RA creation folder fallback is now robust to nullable perimeterThe new folder expression:
folder=( ca.folder or ( ca.perimeter.folder if ca.perimeter else Folder.get_root_folder() ) )correctly prefers
ca.folder(already set byAssessment.save), then safely falls back toca.perimeter.folderonly when a perimeter is present, and finally to the root folder. This eliminates the previous risk ofAttributeErrorwhenperimeteris nullable, while still leveraging theselect_related("folder", "perimeter")optimization on the CA queryset.
810-887: Answer normalization and IG recomputation targeting are precise and efficientThe updated answers block:
- Clones
old_answersdefensively (answers = dict(old_answers)) before mutation.- Drops answers for removed questions and sanitizes values per
type(multiple_choice,unique_choice,text,date) with sensible fallbacks.- Only assigns back to
ra.answersand adds torequirement_assessment_objects_to_updatewhenanswers != old_answers, and simultaneously tracksanswers_changed_ca_ids.Combined with the later
if new_framework.is_dynamic():loop overComplianceAssessment.objects.filter(id__in=answers_changed_ca_ids), this shrinks IG recomputation to only those CAs whose answers actually changed, fixing the earlier “recompute on any non-null answers” behavior without sacrificing correctness.
910-935: Consolidated bulk_update for requirement nodes and RAs is consistent with new strategy logicThe single
bulk_updateonRequirementNodeusing a union of observed fields (plus{"name", "description", "order_id", "questions"}) avoids the dual-update bug and ensures all modified fields are persisted. TheRequirementAssessment.bulk_updatenow includes"answers", "score", "is_scored", "documentation_score"so strategy-driven changes and answer-normalization are flushed in one pass. The subsequent dynamic-framework handling that callsupdate_selected_implementation_groupsonly foranswers_changed_ca_idsis aligned with the earlier tracking logic.
1108-1121: LoadedLibrary.update strategy threading is correct (once dependency propagation is fixed)The new signature
def update(self, strategy=None)plus construction ofLibraryUpdater(self, new_library, strategy)correctly exposes the strategy selection to the update workflow without breaking existing callers. Onceupdate_dependenciesis adjusted to passself.strategytodependency.update(...), the entire call chain (view → LoadedLibrary.update → LibraryUpdater.update_library/update_frameworks) will respect the same strategy for both the target library and its dependencies.Please confirm that all external callers of
LoadedLibrary.update(e.g. management commands, background jobs) either pass an explicit strategy or are expected to surfaceScoreChangeDetectedto a caller that can handle it.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
backend/core/models.py (2)
730-807: Consider deduplicating RAs in the bulk-update list
requirement_assessment_objects_to_updatecan receive the same RA multiple times (e.g. once forscore, once fordocumentation_score, and once foranswers), which is functionally fine but causes redundant work inbulk_update.You can cheaply dedupe by tracking ids:
- requirement_assessment_objects_to_create = [] - requirement_assessment_objects_to_update = [] + requirement_assessment_objects_to_create = [] + requirement_assessment_objects_to_update = [] + ras_to_update_ids = set() @@ - if new_score != old_score: - ra.score = new_score - ra.is_scored = ( - new_score is not None and self.strategy != "reset" - ) - requirement_assessment_objects_to_update.append(ra) + if new_score != old_score: + ra.score = new_score + ra.is_scored = ( + new_score is not None and self.strategy != "reset" + ) + if ra.id not in ras_to_update_ids: + requirement_assessment_objects_to_update.append(ra) + ras_to_update_ids.add(ra.id) @@ - if new_doc_score != old_doc_score: - ra.documentation_score = new_doc_score - requirement_assessment_objects_to_update.append(ra) + if new_doc_score != old_doc_score: + ra.documentation_score = new_doc_score + if ra.id not in ras_to_update_ids: + requirement_assessment_objects_to_update.append(ra) + ras_to_update_ids.add(ra.id) @@ - if not questions: - if ra not in requirement_assessment_objects_to_update: - requirement_assessment_objects_to_update.append(ra) + if not questions: + if ra.id not in ras_to_update_ids: + requirement_assessment_objects_to_update.append(ra) + ras_to_update_ids.add(ra.id) @@ - if answers != old_answers: - ra.answers = answers - requirement_assessment_objects_to_update.append(ra) - answers_changed_ca_ids.add(ra.compliance_assessment_id) + if answers != old_answers: + ra.answers = answers + if ra.id not in ras_to_update_ids: + requirement_assessment_objects_to_update.append(ra) + ras_to_update_ids.add(ra.id) + answers_changed_ca_ids.add(ra.compliance_assessment_id)Not critical, but it will avoid unnecessary duplicate UPDATEs on large frameworks.
Also applies to: 927-932
813-889: Answers normalization + IG recomputation look good; tiny cleanup possible
- Normalizing
answersagainst the newquestionsschema, pruning removed question keys, and only enqueueing RAs whenanswers != old_answersis a solid approach and works well withanswers_changed_ca_idsto limit IG recomputation to CAs whose answers truly changed.- Minor nit: the initial
answers = ra.answers or {}assignment is redundant since you immediately reconstructanswersfromold_answers. You can simplify slightly:- answers = ra.answers or {} - old_answers = ra.answers or {} - answers = ( - dict(old_answers) if isinstance(old_answers, dict) else {} - ) + old_answers = ra.answers or {} + answers = ( + dict(old_answers) if isinstance(old_answers, dict) else {} + )Behavior stays identical, with a bit less noise.
Also applies to: 927-938
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/core/models.py(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
backend/core/models.py (1)
backend/core/utils.py (1)
update_selected_implementation_groups(693-725)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: build_enterprise_frontend
- GitHub Check: build_community_frontend
- GitHub Check: startup-docker-compose-test
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: build (3.12)
- GitHub Check: test (3.12)
- GitHub Check: Analyze (python)
🔇 Additional comments (3)
backend/core/models.py (3)
380-410: Score-change exception and strategy wiring look coherent and safeThe
ScoreChangeDetectedpayload (framework URN, prev/new scores, affected CA ids, strategies) and the propagation ofstrategyfromLoadedLibrary.update→LibraryUpdater→update_dependenciesare consistent. Exceptions will correctly abort the outer transaction via@transaction.atomic, and existing call sites that invokeupdate()without a strategy remain valid thanks to the defaultNone. I don’t see functional issues here.Also applies to: 445-475, 1112-1124
556-575: Library/CA loading and RA folder fallback handle previous issues correctly
- Replacing
set(LoadedLibrary.objects.filter(...))with a plain queryset forinvolved_librariesfixes the “unhashable type” problem while remaining compatible withlibrary__in=filters.- Adding
.select_related("folder", "perimeter")on theComplianceAssessmentqueryset plus thefolder=(ca.folder or (ca.perimeter.folder if ca.perimeter else Folder.get_root_folder()))fallback avoids N+1s and prevents crashes whenperimeteris null.No further changes needed here.
Also applies to: 710-723
521-681: Score-boundary change detection and CA update guard preserve overrides correctly
- Capturing
prev_min/prev_max/prev_defbeforeupdate_or_createand computingscore_boundaries_changedensures both range and definition tweaks are detected.- The
affected_casfilter plus theself.strategy is Nonecheck mean you only raiseScoreChangeDetectedwhen there are existing CAs still on the previous framework defaults, avoiding silent remaps while not blocking cases with no attached CAs or already-customized ones.- The
still_on_prev_defaultsguard in the CA loop ensures only CAs that still exactly match the old framework defaults get their bounds updated, leaving user-customized CAs untouched.- Initializing
ca_bounds = {}unconditionally and then populating it only when there are CA updates avoidsNameErrorand provides correct per-CA min/max with fallbacks to framework or 0–100.This flow looks logically sound and aligned with the intended semantics.
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.