fix(data_wizard): Make audit and risk assessment perimeter optional#3799
fix(data_wizard): Make audit and risk assessment perimeter optional#3799monsieurswag wants to merge 2 commits intomainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughPerimeter is now optional for compliance and risk assessment imports. Backend helpers conditionally resolve Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cli/clica.py (1)
324-330:⚠️ Potential issue | 🟠 MajorThese commands now hide every target selector.
register_data_wizard_command()derivesshow_perimeter_optionfromrequires_perimeter, andshow_folder_optionis already false forComplianceAssessment/RiskAssessment. After this change,--helpshows neither--perimeternor--folder, while the backend still requires one of them. The CLI also stops catching that invalid combination before uploading the file.💡 At minimum, keep the selectors visible again
{ "command": "import_compliance_assessments", "model_type": "ComplianceAssessment", "help": "Import compliance assessments using the Data Wizard backend.", "requires_folder": False, "requires_perimeter": False, + "show_folder_option": True, + "show_perimeter_option": True, "requires_framework": True, "requires_matrix": False, }, @@ { "command": "import_risk_assessment", "model_type": "RiskAssessment", "help": "Import risk assessments using the Data Wizard backend.", "requires_folder": False, "requires_perimeter": False, + "show_folder_option": True, + "show_perimeter_option": True, "requires_framework": False, "requires_matrix": True, },Also applies to: 342-348
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/clica.py` around lines 324 - 330, The CLI command dict for "import_compliance_assessments" (and the similar block at 342-348) hides selector flags because register_data_wizard_command derives show_perimeter_option from requires_perimeter and folder is already false; explicitly enable the selector visibility so users see the options and the CLI can validate them: update the command config passed to register_data_wizard_command (for "import_compliance_assessments" and the other affected command) to include an explicit show_perimeter_option: True (or otherwise set the appropriate requires_perimeter/ show_folder_option flags) so the --perimeter/--folder options are shown and the CLI can validate the combination before upload.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/data_wizard/views.py`:
- Around line 2529-2541: The code currently raises an AssertionError when
neither perimeter nor folder_id is provided; instead, return a normal import
result dict (the same shape as the existing results variable) so callers like
process_excel_file() receive a dict not an exception-wrapped Response. In the
branch where perimeter is None and folder_id is None, populate results["failed"]
(e.g., set to 1) and append an explanatory message to results["errors"] (e.g.,
"A folder must be specified when there's no perimeter"), then return results
immediately; update the block around Perimeter.objects.get, perimeter, folder_id
to perform this return rather than raising. Ensure the returned shape matches
the existing results dict.
---
Outside diff comments:
In `@cli/clica.py`:
- Around line 324-330: The CLI command dict for "import_compliance_assessments"
(and the similar block at 342-348) hides selector flags because
register_data_wizard_command derives show_perimeter_option from
requires_perimeter and folder is already false; explicitly enable the selector
visibility so users see the options and the CLI can validate them: update the
command config passed to register_data_wizard_command (for
"import_compliance_assessments" and the other affected command) to include an
explicit show_perimeter_option: True (or otherwise set the appropriate
requires_perimeter/ show_folder_option flags) so the --perimeter/--folder
options are shown and the CLI can validate the combination before upload.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 09b25df8-ce4f-455c-80b8-4178cdfc4344
📒 Files selected for processing (2)
backend/data_wizard/views.pycli/clica.py
| results = {"successful": 0, "failed": 0, "errors": []} | ||
| try: | ||
| # Get the perimeter object to extract its folder ID | ||
| perimeter = Perimeter.objects.get(id=perimeter_id) | ||
| folder_id = perimeter.folder.id | ||
| perimeter = None | ||
| if perimeter_id is not None: | ||
| perimeter = Perimeter.objects.get(id=perimeter_id) | ||
|
|
||
| if perimeter is not None: | ||
| folder_id = perimeter.folder.id | ||
| elif folder_id is None: | ||
| raise AssertionError( | ||
| "A folder must be specified when there's no perimeter!" | ||
| ) |
There was a problem hiding this comment.
Return a normal import result here instead of raising.
This branch is newly reachable now that perimeter is optional. Raising here falls into the broad except below, which returns a DRF Response from a helper that otherwise returns a dict; process_excel_file() then wraps that value in another response. Please fail with the same result shape as the other import helpers.
💡 Minimal fix
- elif folder_id is None:
- raise AssertionError(
- "A folder must be specified when there's no perimeter!"
- )
+ elif folder_id is None:
+ results["failed"] = len(records)
+ results["errors"].append(
+ {"error": "A folder must be specified when there's no perimeter."}
+ )
+ return results🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/data_wizard/views.py` around lines 2529 - 2541, The code currently
raises an AssertionError when neither perimeter nor folder_id is provided;
instead, return a normal import result dict (the same shape as the existing
results variable) so callers like process_excel_file() receive a dict not an
exception-wrapped Response. In the branch where perimeter is None and folder_id
is None, populate results["failed"] (e.g., set to 1) and append an explanatory
message to results["errors"] (e.g., "A folder must be specified when there's no
perimeter"), then return results immediately; update the block around
Perimeter.objects.get, perimeter, folder_id to perform this return rather than
raising. Ensure the returned shape matches the existing results dict.
| "help": "Import compliance assessments using the Data Wizard backend.", | ||
| "requires_folder": False, | ||
| "requires_perimeter": True, | ||
| "requires_perimeter": False, |
There was a problem hiding this comment.
almost, now we're in a situation where domain (folder) and perimeter are optional
JiwaniZakir
left a comment
JiwaniZakir
left a comment
There was a problem hiding this comment.
The use of AssertionError in both _process_compliance_assessment and _process_risk_assessment is inappropriate for input validation — AssertionError is intended for internal invariant checks and can be silently disabled when Python runs with the -O optimization flag. A ValueError (or a domain-specific exception) would be more semantically correct and safer here.
More critically, in cli/clica.py, both model type configs still have "requires_folder": False even after setting "requires_perimeter": False. This means a user can now invoke the CLI without providing either a perimeter or a folder, and the error won't surface until the request hits the backend and raises that AssertionError. The CLI should enforce the "at least one of perimeter or folder is required" constraint at the argument-parsing level, or at minimum requires_folder should be conditionally tied to whether a perimeter is provided.
It's also worth adding a test scenario where perimeter_id=None and folder_id=None are both passed to each function, to verify the error path behaves as expected rather than producing an unhandled ObjectDoesNotExist or silent failure.
4 participants
Currently CLI users are forced to specify a
Perimeterwhen usingimport-risk-assessmentorimport-compliance-assessmentcommands.This is wrong as both the
RiskAssessment.perimeterandComplianceAssessment.perimeterfields are optional (they areForeignKeywithnull=True).Now users can either specify a perimeter, or only specify the folder if they want no perimeter.
Summary by CodeRabbit
Release Notes