feat: refactor framework questions #1671
Conversation
* Create docker-compose.ps1 This is the initialization script for Windows user * translate doc-pol in French * Create cjis.yaml create mapping for cjis update fix error message * Avoid reckless error localization lookups * Remove is_open_for_signup method override The signup check is already done in the SAML views * chore: Organize imports * chore: Some cleaning * Modèle ANSSI ajout de nouveaux framework et homogénéisation des références des bibliothèques * Modèles ANSSI ajout de nouveaux framework et homogénéisation des références des bibliothèques * Fix objects_meta always counting 1 object even when there is more than that * Fix null name for implementation groups * Add extra log useful for mapping issues * Update utils.py fix and rationalize messages * Update utils.py * add GL on costs and losses * Add migration to fix the wrong objects_meta counters * Formatter * quick cleanup * Fix reference controls french translation It translates to "Mesures de référence", not "Contrôles de référence" * Remove debug code * Remove unnecessary click handler * Change all backend endpoint URLs not ending with a slash to stop useless redirections when requesting the API * Fix broken dependency links for loaded library detail views * Backend formatter * Update data-model.md * Simplify post token generation flow This essentially removes redundant or irrelevant checks from allauth. As we will most certainly only have a single social account adapter for the entire lifespan of the project, these changes will only save us from headaches. * Render IdP-initiated SSO rejected error * Create enum for SAML-related auth errors * Internationalize auth errors * Use FieldsRelatedField to serialize dependencies * chore: Run formatting * chore: Organize imports * Bump django from 5.0.7 to 5.0.8 in /backend Bumps [django](https://github.com/django/django) from 5.0.7 to 5.0.8. - [Commits](django/django@5.0.7...5.0.8) --- updated-dependencies: - dependency-name: django dependency-type: direct:production ... Signed-off-by: dependabot[bot] <[email protected]> * TPRM model Leverage compliance assessment. * fix translation spec * Update data-model.md clarify solution. small fixes. * Update data-model.md Consider contract as a subset of applied controls. A domain can be owned by one entity at most. * Update id and new template anssi framework * Update id and new template anssi framework * TPRM * Update data-model.md * Update startup-tests.yml try fix docker test error * Update startup-tests.yml try fix docker test * 724: improve framework naming rendering * Small ui improvement * also improve library view * Add RTS DORA on JET, on OVS * Add PART-IS * fix: typo in quality checks * chore: update translations with Fink 🐦 * Update libraries.test.ts try fix functional tests * try fix functional tests - une name instead of ref to get a lib. - adjust library name for NIST CSF, so that lib name=referential name (assumption for tests) * Update page-content.ts prettier * Update README.md * fix: allow empty link * fix French translation for iso27005 matrix * Update README.md * fix: revert evidence link field schema * chore: add missing comma * Render authentication errors * feat: raise urn max length * chore: format migration * Add Romanian translation * update the excel sheet * generate the new yaml with annotations and levels * Update README.md Add in 1.6.6 * Update README.md * Update README.md --------- Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: Fabrizio Di Carlo <[email protected]> Co-authored-by: eric-intuitem <[email protected]> Co-authored-by: Nassim Tabchiche <[email protected]> Co-authored-by: protocolpaladin <[email protected]> Co-authored-by: Nassim <[email protected]> Co-authored-by: monsieurswag <[email protected]> Co-authored-by: Abder <[email protected]> Co-authored-by: ImanABS <[email protected]> Co-authored-by: monsieurswag <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Mohamed-Hacene <[email protected]> Co-authored-by: Mohamed-Hacene <[email protected]>
I have aranged, using the ciso assistant tools, two italian frameworks.
WalkthroughThis update transitions the data model and related logic from supporting a single question and answer per requirement to supporting multiple questions and answers. The migration includes backend model, serializer, utility, and template changes, as well as frontend component and localization updates, and introduces a database migration script and YAML migration tool to facilitate the new structure. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Frontend
participant Backend
participant Database
User->>Frontend: Open requirement assessment
Frontend->>Backend: Fetch requirement node (with questions)
Backend->>Database: Query RequirementNode (questions)
Database-->>Backend: Return node with questions object
Backend-->>Frontend: Return node data (questions: {urn: {...}, ...})
Frontend->>User: Render multiple questions
User->>Frontend: Submit answers to questions
Frontend->>Backend: Send answers object (answers: {urn: value/list})
Backend->>Database: Store answers in RequirementAssessment
Database-->>Backend: Confirm save
Backend-->>Frontend: Acknowledge update
Frontend->>User: Display saved answers
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (10)
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
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: 1
🧹 Nitpick comments (6)
documentation/architecture/data-model.md (6)
1145-1145: Clarify JSON form referenceThe phrase "as a JSON form" may be ambiguous—consider linking to the "Question and answer format" section or specifying the expected schema.
1287-1288: Standardize JSON casing and phrasingUpdate "json" to "JSON" and refine the description, for example:
- - answers: a json corresponding to the answers of the requirement node questions. + - answers: a JSON object representing the answers to the requirement node questions.
1309-1309: Standardize JSON casing and phrasingSimilarly, update the questions line to use uppercase "JSON" and clarify:
- - questions: a json corresponding to the optional questions of the requirement node. + - questions: a JSON object defining the optional questions for the requirement node.
1311-1313: Improve heading and introductory sentenceConsider rephrasing for clarity and to remove redundancy:
- ### Question and answer format - The format for questions and answers json fields will evolve over time. The initial format is the following: + ### Question and answer format + The format for the `questions` and `answers` JSON fields will evolve; the initial format is as follows:🧰 Tools
🪛 LanguageTool
[uncategorized] ~1311-~1311: Possible missing preposition found.
Context: ...t The format for questions and answers json fields will evolve over time. The initi...(AI_HYDRA_LEO_MISSING_OF)
[style] ~1311-~1311: This phrase is redundant. Consider writing “evolve”.
Context: ... questions and answers json fields will evolve over time. The initial format is the following: ...(EVOLVE_OVER_TIME)
1315-1339: Validate JSON example syntaxEnsure the sample JSON is valid (e.g., remove trailing commas after the last property) and wrap it in a proper code block:
{ "urn:intuitem:risk:req_node:example:a.1:question:1": { "type": "unique_choice", "choices": [ { "urn": "...", "value": "yes" }, { "urn": "...", "value": "no" }, { "urn": "...", "value": "n/a" } ], "text": "Question title" }, ... }
1341-1352: Validate answers JSON snippetThe answers example should be valid JSON and illustrate both array and scalar cases. For clarity, separate the two cases or include a note explaining when each is used:
{ "urn:intuitem:risk:req_node:example:a.1:question:1": [ "urn:intuitem:risk:framework:example:answer01:choice:1", "urn:intuitem:risk:framework:example:answer01:choice:2" ], "urn:intuitem:risk:req_node:example:a.1:question:2": "yes" }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
backend/core/models.py(6 hunks)backend/core/serializers.py(2 hunks)documentation/architecture/data-model.md(3 hunks)frontend/messages/cs.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/id.json(1 hunks)frontend/messages/nl.json(1 hunks)frontend/messages/pt.json(1 hunks)frontend/messages/ro.json(1 hunks)frontend/src/lib/utils/schemas.ts(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- frontend/messages/nl.json
- frontend/messages/id.json
- frontend/messages/cs.json
- frontend/messages/es.json
🚧 Files skipped from review as they are similar to previous changes (7)
- frontend/src/lib/utils/schemas.ts
- frontend/messages/fr.json
- backend/core/serializers.py
- frontend/messages/de.json
- frontend/messages/pt.json
- frontend/messages/ro.json
- backend/core/models.py
🧰 Additional context used
🪛 LanguageTool
documentation/architecture/data-model.md
[uncategorized] ~1311-~1311: Possible missing preposition found.
Context: ...t The format for questions and answers json fields will evolve over time. The initi...
(AI_HYDRA_LEO_MISSING_OF)
[style] ~1311-~1311: This phrase is redundant. Consider writing “evolve”.
Context: ... questions and answers json fields will evolve over time. The initial format is the following: ...
(EVOLVE_OVER_TIME)
🪛 Biome (1.9.4)
frontend/messages/en.json
[error] 1390-1390: The key date was already declared.
This where a duplicated key was declared again.
If a key is defined multiple times, only the last definition takes effect. Previous definitions are ignored.
(lint/suspicious/noDuplicateObjectKeys)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
documentation/architecture/data-model.md (1)
1285-1285: Heading follows existing conventionsThe new
### Requirement assessmentheading aligns with other model headings and is consistent with the document structure.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
backend/core/migrations/0069_remove_requirementassessment_answer_and_more.py (2)
24-27: Unused variablechoices_mapscan be removed
choices_mapsis built but never referenced, creating dead code and minor overhead.- # Pre-build choices map for efficiency - choices_maps = {} - for idx, choice_text in enumerate(choices_texts): - choices_maps[idx] = {"urn": f"choice:{idx + 1}", "value": choice_text}Simply delete these lines (or actually use the structure if you intended to).
Tidying this up avoids confusion for future maintainers.
69-83: Load only required fields when iterating to reduce memory footprint
Requirementnode.objects.iterator()pulls all columns for every row, which can be heavy if the table is large.
You only needid,urn, andquestion, so restrict the query:- for instance in Requirementnode.objects.iterator(): + for instance in ( + Requirementnode.objects.only("id", "urn", "question").iterator() + ):This keeps each ORM object light and speeds up the migration.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/core/migrations/0069_remove_requirementassessment_answer_and_more.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/core/migrations/0069_remove_requirementassessment_answer_and_more.py (1)
backend/core/models.py (2)
RequirementAssessment(3761-3932)RequirementNode(1096-1166)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: build (3.12)
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: Analyze (python)
- GitHub Check: test (3.12)
- GitHub Check: Analyze (javascript-typescript)
backend/core/migrations/0069_remove_requirementassessment_answer_and_more.py
Outdated
Show resolved
Hide resolved
| # Handle different question types | ||
| if question_type in ["text", "date"]: | ||
| # For text and date types, keep the answer value as is | ||
| new_answers[question_urn] = answer_value | ||
| else: | ||
| # Handle single-choice answers | ||
| if isinstance(answer_value, str): | ||
| if answer_value in choices_map: | ||
| new_answers[question_urn] = choices_map[answer_value] | ||
| else: | ||
| # Keep free-text answers as they are | ||
| new_answers[question_urn] = answer_value |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Multiple‑choice answers containing lists are silently dropped
answer_value is assumed to be a string. For multiple‑choice questions the legacy data may be a list of texts.
Current logic skips these, so answers are lost.
Guard for lists and map each item:
- else:
- # Handle single-choice answers
- if isinstance(answer_value, str):
- if answer_value in choices_map:
- new_answers[question_urn] = choices_map[answer_value]
- else:
- # Keep free-text answers as they are
- new_answers[question_urn] = answer_value
+ else:
+ if isinstance(answer_value, list):
+ new_answers[question_urn] = [
+ choices_map.get(v, v) for v in answer_value
+ ]
+ elif isinstance(answer_value, str):
+ new_answers[question_urn] = choices_map.get(
+ answer_value, answer_value
+ )Please confirm the expected legacy shape and adjust accordingly.
#!/bin/bash
set -e
Locate the migration file
MIGRATION_FILE=$(find . -type f | rg '0069_remove_requirementassessment_answer_and_more.py')
echo "Migration file found at: $MIGRATION_FILE"
echo
echo "---- Snippet around lines 155-166 ----"
sed -n '140,180p' "$MIGRATION_FILE"
echo
echo "---- Checking for existing list handling of answer_value ----"
grep -En "isinstance\s*(\sanswer_value\s,\slist\s)" "$MIGRATION_FILE" || echo "No isinstance(..., list) checks found in this file."
🏁 Script executed:
#!/bin/bash
set -e
# Find the migration file
MIGRATION_FILE=$(fd --full-path 0069_remove_requirementassessment_answer_and_more.py)
echo "Migration file located at: $MIGRATION_FILE"
echo
# Show the context around the reported lines
echo "---- Lines 150-185 of the migration ----"
sed -n '150,185p' "$MIGRATION_FILE"
echo
# Check for any handling of list-type answer_value
echo "---- Checking for isinstance(answer_value, list) ----"
grep -En "isinstance\s*\(\s*answer_value\s*,\s*list\s*\)" "$MIGRATION_FILE" || echo "No list handling detected."Length of output: 2520
Handle list‑type multiple‑choice answers
The current migration only checks for str values, so any legacy answers stored as lists (e.g. multiple‑choice questions) will be dropped. In backend/core/migrations/0069_remove_requirementassessment_answer_and_more.py (around lines 155–166), update the else‑block to something like:
else:
- # Handle single-choice answers
- if isinstance(answer_value, str):
- if answer_value in choices_map:
- new_answers[question_urn] = choices_map[answer_value]
- else:
- # Keep free-text answers as they are
- new_answers[question_urn] = answer_value
+ # Handle multiple‑ and single‑choice answers
+ if isinstance(answer_value, list):
+ # Map each selected option, keep free‑text items
+ new_answers[question_urn] = [
+ choices_map.get(item, item) for item in answer_value
+ ]
+ elif isinstance(answer_value, str):
+ new_answers[question_urn] = choices_map.get(
+ answer_value, answer_value
+ )• Please confirm that legacy multiple‑choice answers indeed arrive as list[str] and adjust this logic if the shape differs.
📝 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.
| # Handle different question types | |
| if question_type in ["text", "date"]: | |
| # For text and date types, keep the answer value as is | |
| new_answers[question_urn] = answer_value | |
| else: | |
| # Handle single-choice answers | |
| if isinstance(answer_value, str): | |
| if answer_value in choices_map: | |
| new_answers[question_urn] = choices_map[answer_value] | |
| else: | |
| # Keep free-text answers as they are | |
| new_answers[question_urn] = answer_value | |
| # Handle different question types | |
| if question_type in ["text", "date"]: | |
| # For text and date types, keep the answer value as is | |
| new_answers[question_urn] = answer_value | |
| else: | |
| # Handle multiple- and single-choice answers | |
| if isinstance(answer_value, list): | |
| # Map each selected option, keep free-text items | |
| new_answers[question_urn] = [ | |
| choices_map.get(item, item) for item in answer_value | |
| ] | |
| elif isinstance(answer_value, str): | |
| # Map single-choice answers, or keep free-text | |
| new_answers[question_urn] = choices_map.get( | |
| answer_value, answer_value | |
| ) |
There was a problem hiding this comment.
It looks okay to me functionally, however there are a few changes to bring to this PR before it can be merged:
- demo data cannot be loaded (as shown in functional tests, does not work on my machine either). The
dummy-domainfile might be out of date. - when incrementing schema version, keep old
sample-domain-schema-{n}.bakfile, and just addsample-domain-schema-{n+1}.bak - are you sure the data migration is running? I see no calls to
migrations.RunPythonon the migration file.
|
Migration alignment needed |
…b.com:intuitem/ciso-assistant-community into CA-743-Refactor-framework-questions-structure
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
backend/core/migrations/0070_remove_requirementassessment_answer_and_more.py (2)
10-15: Doc-string still describes the old single-question modelThe doc-string speaks about a field named
"question"and a list named"questions"in the legacy schema, which is misleading – until this migration there was only"question". Please update the description so future maintainers do not waste time chasing a non-existentquestionsattribute in the old model.
23-27:choices_mapsis computed but never used
choices_mapsis built on lines 23-27 and then ignored. Either remove the block or use the map inside the loop to avoid running a wasted O(N) loop per node.- # Pre-build choices map for efficiency - choices_maps = {} - for idx, choice_text in enumerate(choices_texts): - choices_maps[idx] = {"urn": f"choice:{idx + 1}", "value": choice_text}documentation/architecture/data-model.md (1)
1145-1146: Pluralisation mismatch with the new schemaThe bullet still says “a requirement node can include questions … corresponds to a JSON answer”.
To stay consistent with the code & migration, change “answer” → “answers”.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/core/migrations/0070_remove_requirementassessment_answer_and_more.py(1 hunks)documentation/architecture/data-model.md(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
- frontend/messages/fr.json
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/core/migrations/0070_remove_requirementassessment_answer_and_more.py (1)
backend/core/models.py (2)
RequirementAssessment(3761-3932)RequirementNode(1096-1166)
🪛 LanguageTool
documentation/architecture/data-model.md
[style] ~1311-~1311: This phrase is redundant. Consider writing “evolve”.
Context: ... questions and answers json fields will evolve over time. The initial format is the following: ...
(EVOLVE_OVER_TIME)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: build (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: test (3.12)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
backend/core/migrations/0070_remove_requirementassessment_answer_and_more.py
Outdated
Show resolved
Hide resolved
backend/core/migrations/0070_remove_requirementassessment_answer_and_more.py
Outdated
Show resolved
Hide resolved
| operations = [ | ||
| migrations.RemoveField( | ||
| model_name="requirementassessment", | ||
| name="answer", | ||
| ), | ||
| migrations.RunPython(migrate_questions, reverse_migrations), | ||
| migrations.RemoveField( | ||
| model_name="requirementnode", | ||
| name="question", | ||
| ), | ||
| migrations.AddField( | ||
| model_name="requirementassessment", | ||
| name="answers", | ||
| field=models.JSONField(blank=True, null=True, verbose_name="Answers"), | ||
| ), | ||
| migrations.RunPython(migrate_answers_format, reverse_migrations), | ||
| migrations.AddField( | ||
| model_name="requirementnode", | ||
| name="questions", | ||
| field=models.JSONField(blank=True, null=True, verbose_name="Questions"), | ||
| ), | ||
| ] |
There was a problem hiding this comment.
Broken migration ordering – columns removed before data is copied
The current sequence:
- Remove
answer - Run
migrate_questions - Remove
question - Add
answers - Run
migrate_answers_format - Add
questions
means the data you are trying to migrate no longer exists and the target columns are missing at migration time.
Re-order the operations so that new columns are created first, data is copied, and only then are the legacy columns dropped:
operations = [
- migrations.RemoveField(
- model_name="requirementassessment",
- name="answer",
- ),
- migrations.RunPython(migrate_questions, reverse_migrations),
- migrations.RemoveField(
- model_name="requirementnode",
- name="question",
- ),
- migrations.AddField(
- model_name="requirementassessment",
- name="answers",
- field=models.JSONField(blank=True, null=True, verbose_name="Answers"),
- ),
- migrations.RunPython(migrate_answers_format, reverse_migrations),
- migrations.AddField(
- model_name="requirementnode",
- name="questions",
- field=models.JSONField(blank=True, null=True, verbose_name="Questions"),
- ),
+ # 1️⃣ Add new columns first
+ migrations.AddField(
+ model_name="requirementnode",
+ name="questions",
+ field=models.JSONField(blank=True, null=True, verbose_name="Questions"),
+ ),
+ migrations.AddField(
+ model_name="requirementassessment",
+ name="answers",
+ field=models.JSONField(blank=True, null=True, verbose_name="Answers"),
+ ),
+ # 2️⃣ Migrate data
+ migrations.RunPython(migrate_questions, reverse_migrations),
+ migrations.RunPython(migrate_answers_format, reverse_migrations),
+ # 3️⃣ Drop legacy columns
+ migrations.RemoveField(
+ model_name="requirementassessment",
+ name="answer",
+ ),
+ migrations.RemoveField(
+ model_name="requirementnode",
+ name="question",
+ ),
]This preserves data and avoids runtime failures.
📝 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.
| operations = [ | |
| migrations.RemoveField( | |
| model_name="requirementassessment", | |
| name="answer", | |
| ), | |
| migrations.RunPython(migrate_questions, reverse_migrations), | |
| migrations.RemoveField( | |
| model_name="requirementnode", | |
| name="question", | |
| ), | |
| migrations.AddField( | |
| model_name="requirementassessment", | |
| name="answers", | |
| field=models.JSONField(blank=True, null=True, verbose_name="Answers"), | |
| ), | |
| migrations.RunPython(migrate_answers_format, reverse_migrations), | |
| migrations.AddField( | |
| model_name="requirementnode", | |
| name="questions", | |
| field=models.JSONField(blank=True, null=True, verbose_name="Questions"), | |
| ), | |
| ] | |
| operations = [ | |
| # 1️⃣ Add new columns first | |
| migrations.AddField( | |
| model_name="requirementnode", | |
| name="questions", | |
| field=models.JSONField(blank=True, null=True, verbose_name="Questions"), | |
| ), | |
| migrations.AddField( | |
| model_name="requirementassessment", | |
| name="answers", | |
| field=models.JSONField(blank=True, null=True, verbose_name="Answers"), | |
| ), | |
| # 2️⃣ Migrate data | |
| migrations.RunPython(migrate_questions, reverse_migrations), | |
| migrations.RunPython(migrate_answers_format, reverse_migrations), | |
| # 3️⃣ Drop legacy columns | |
| migrations.RemoveField( | |
| model_name="requirementassessment", | |
| name="answer", | |
| ), | |
| migrations.RemoveField( | |
| model_name="requirementnode", | |
| name="question", | |
| ), | |
| ] |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
backend/core/migrations/0069_remove_requirementassessment_answer_and_more.py (1)
155-166:⚠️ Potential issueRetain multiple-choice answers stored as lists.
The current branch only handles
stranswers; legacy multiple-choice answers arriving aslist[str]are discarded.
Apply the earlier review’s logic:- # Handle single-choice answers - if isinstance(answer_value, str): - if answer_value in choices_map: - new_answers[question_urn] = choices_map[answer_value] - else: - # Keep free-text answers as they are - new_answers[question_urn] = answer_value + # Handle multiple- and single-choice answers + if isinstance(answer_value, list): + new_answers[question_urn] = [ + choices_map.get(v, v) for v in answer_value + ] + elif isinstance(answer_value, str): + new_answers[question_urn] = choices_map.get( + answer_value, answer_value + )This preserves all user responses and aligns with the new schema.
🧹 Nitpick comments (1)
backend/core/migrations/0069_remove_requirementassessment_answer_and_more.py (1)
23-27: Remove unusedchoices_mapsvariable.
choices_mapsis built but never referenced, creating dead code and a misleading hint that an optimisation exists.- # Pre-build choices map for efficiency - choices_maps = {} - for idx, choice_text in enumerate(choices_texts): - choices_maps[idx] = {"urn": f"choice:{idx + 1}", "value": choice_text}Simply drop these lines or utilise the map in the loop below.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/core/migrations/0069_remove_requirementassessment_answer_and_more.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/core/migrations/0069_remove_requirementassessment_answer_and_more.py (1)
backend/core/models.py (2)
RequirementAssessment(3761-3932)RequirementNode(1096-1166)
🔇 Additional comments (2)
backend/core/migrations/0069_remove_requirementassessment_answer_and_more.py (2)
208-229: 👍 Operations order now preserves data before dropping legacy columns.The sequence “add → migrate → remove” for both
RequirementNodeandRequirementAssessmentmitigates data-loss risk flagged in earlier reviews.
195-201: Consider implementing a minimal rollback or explicitno_reverse_sql=True.The stubbed
reverse_migrationslogs a warning but leaves the migration technically reversible in Django’s eyes.
Either:
- Implement an actual down-migration, or
- Mark it irreversible to avoid false expectations:
migrations.RunPython(migrate_questions, reverse_code=migrations.RunPython.noop)or keep the helper but raise
IrreversibleError.
backend/core/migrations/0069_remove_requirementassessment_answer_and_more.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
backend/core/migrations/0071_remove_requirementassessment_answer_and_more.py (3)
24-27:choices_mapsis computed but never used – drop or repurpose itThe loop builds
choices_maps, yet the variable is not referenced afterwards.
Either remove these four lines to avoid the needless O(n) work or reuse the pre-computed structure inside the later loop to avoid recomputing choices for every question.- # Pre-build choices map for efficiency - choices_maps = {} - for idx, choice_text in enumerate(choices_texts): - choices_maps[idx] = {"urn": f"choice:{idx + 1}", "value": choice_text}
67-83: Iterator chunk size is not honoured
batch_size = 1000, butRequirementnode.objects.iterator()defaults to 2 000.
Pass the desired chunk explicitly to keep memory usage predictable:- for instance in Requirementnode.objects.iterator(): + for instance in Requirementnode.objects.iterator(chunk_size=batch_size):
195-201: One-way migration: consider at least a minimal reverse implementationThe stub reverse merely logs a warning. While one-way migrations are acceptable, Django will still show them as irreversible. Providing a minimal reverse (e.g., copying
questions → questionandanswers → answerwithout complex mapping) allows teams to back out of the release if unexpected issues arise.If full reversal is impractical, add
elidable = Trueto the operation or mark theRunPythonsteps aselidable=Trueto suppress the Django warning.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
backend/core/migrations/0071_remove_requirementassessment_answer_and_more.py(1 hunks)backend/core/models.py(6 hunks)backend/core/serializers.py(2 hunks)frontend/messages/cs.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/id.json(1 hunks)frontend/messages/it.json(1 hunks)frontend/messages/nl.json(1 hunks)frontend/messages/pt.json(1 hunks)frontend/messages/ro.json(1 hunks)frontend/src/lib/utils/schemas.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- frontend/messages/cs.json
- frontend/messages/it.json
🚧 Files skipped from review as they are similar to previous changes (11)
- frontend/src/lib/utils/schemas.ts
- backend/core/serializers.py
- frontend/messages/ro.json
- frontend/messages/de.json
- frontend/messages/nl.json
- frontend/messages/id.json
- frontend/messages/fr.json
- frontend/messages/es.json
- frontend/messages/en.json
- frontend/messages/pt.json
- backend/core/models.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/core/migrations/0071_remove_requirementassessment_answer_and_more.py (1)
backend/core/models.py (2)
RequirementAssessment(3819-3990)RequirementNode(1097-1167)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: Analyze (python)
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: test (3.12)
- GitHub Check: build (3.12)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
backend/core/migrations/0071_remove_requirementassessment_answer_and_more.py (1)
37-52: Assumes onequestion_typeper node – verify this matches legacy data
question_typeis read once fromold_dataand applied to every question in the loop.
If the legacy schema ever allowed different types per question (e.g., a mix of text and choice questions under one node), all questions will inherit the same (possibly wrong) type here, causing downstream misclassification.Please confirm the historical constraints; if heterogeneous types were allowed, move the type into each question entry in the original payload and derive it per-question instead.
| # Process assessments in batches | ||
| batch_size = 1000 | ||
| assessments_to_update = [] | ||
|
|
||
| for instance in RequirementAssessment.objects.iterator(): | ||
| old_answer_data = getattr(instance, "answer", None) | ||
| new_answers = {} | ||
|
|
||
| if not old_answer_data or not isinstance(old_answer_data, dict): |
There was a problem hiding this comment.
Multi-choice answers are silently discarded – data loss risk
migrate_answers_format handles choice answers only when answer_value is a str.
For “multipleChoice” questions the legacy answer is typically a list/tuple of strings.
These will fail the isinstance(answer_value, str) check and be skipped, so the user’s answers disappear.
Proposed fix (inside the else branch):
- # Handle single-choice answers
- if isinstance(answer_value, str):
- if answer_value in choices_map:
- new_answers[question_urn] = choices_map[answer_value]
- else:
- # Keep free-text answers as they are
- new_answers[question_urn] = answer_value
+ # Single-choice string or multi-choice list/tuple
+ if isinstance(answer_value, str):
+ new_answers[question_urn] = choices_map.get(
+ answer_value, answer_value
+ )
+ elif isinstance(answer_value, (list, tuple)):
+ new_answers[question_urn] = [
+ choices_map.get(v, v) for v in answer_value
+ ]
+ else:
+ # Fallback – store raw value to avoid data loss
+ new_answers[question_urn] = answer_valueWithout this, historical multiple-choice data will be irretrievably lost.
Also applies to: 155-167
There was a problem hiding this comment.
Good but "multiple choices" was not an option before this PR
There was a problem hiding this comment.
@Mohamed-Hacene You're absolutely right! After reviewing the code changes across the PR, I can see that "multiple choice" is a new question type being introduced in this refactoring. The migration doesn't need to handle arrays of answers since they didn't exist in the previous data model.
The current migration logic is correct since it only needs to handle the question types that existed before (text, date, and single-choice questions). Thanks for clarifying this point!
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Documentation
Chores