feat: add a progress field on applied controls #1443
Conversation
WalkthroughThis pull request introduces a new Changes
Sequence DiagramsequenceDiagram
participant User
participant Form
participant Model
participant Backend
User->>Form: Enters Applied Control
Form->>Model: Sets progress_field
Model->>Backend: Saves control
Backend-->>Model: Validate status
alt Status is "active"
Model->>Model: Set progress_field to 100
end
Model-->>Form: Return updated control
Form-->>User: Display control details
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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: 2
🧹 Nitpick comments (1)
backend/core/models.py (1)
1870-1871: Improve auto-update logic for progress field.The current implementation only sets progress to 100 when status is "active". Consider these improvements:
- Reset progress to 0 when status changes from "active" to any other status
- Validate that manual progress updates don't exceed 100
Apply this diff to improve the logic:
- if self.status == "active": - self.progress_field = 100 + if not self._state.adding: # Only for updates + old_instance = AppliedControl.objects.get(pk=self.pk) + if self.status == "active" and old_instance.status != "active": + self.progress_field = 100 + elif self.status != "active" and old_instance.status == "active": + self.progress_field = 0
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
backend/core/migrations/0050_appliedcontrol_progress_field.py(1 hunks)backend/core/models.py(2 hunks)backend/core/views.py(2 hunks)frontend/messages/en.json(1 hunks)frontend/messages/fr.json(1 hunks)frontend/src/lib/components/Forms/ModelForm/AppliedControlPolicyForm.svelte(2 hunks)frontend/src/lib/utils/crud.ts(1 hunks)frontend/src/lib/utils/schemas.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- backend/core/migrations/0050_appliedcontrol_progress_field.py
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: startup-docker-compose-test
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: build (3.12)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: test (3.12)
- GitHub Check: ruff (3.12)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (7)
frontend/src/lib/components/Forms/ModelForm/AppliedControlPolicyForm.svelte (1)
7-7: LGTM! Verify the translation key exists.The
<Score>component integration looks good with appropriate configuration for progress tracking (0-100 range). The implementation aligns with the PR objectives.Let's verify that the translation key exists in the messages files:
Also applies to: 143-151
✅ Verification successful
Translation key verification successful ✓
The
progresstranslation key is properly defined in all language files and contains appropriate values.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the progress translation key exists in the messages files. echo "Checking for progress translation key in message files..." rg -l "progress[\"']?\s*:" frontend/messages/Length of output: 564
Script:
#!/bin/bash # Check the actual content of the progress key in the English messages file echo "Checking content of progress key in English messages file..." cat frontend/messages/en.json | jq '.progress'Length of output: 188
frontend/src/lib/utils/schemas.ts (1)
156-157: LGTM! Schema definition matches the backend model.The
progress_fieldschema is correctly defined as an optional number with default value 0, matching the backend model definition.frontend/src/lib/utils/crud.ts (1)
266-267: LGTM! Field added to detail view configuration.The
progress_fieldis correctly added todetailViewFields, enabling progress tracking in detail views.backend/core/views.py (2)
1058-1058: LGTM! Progress field added to filter options.The addition of
progress_fieldto the filterset fields enables filtering of applied controls based on their progress value, which aligns with the PR objectives.
1412-1412: LGTM! Progress field added to duplicate functionality.The addition of
progress_fieldensures that the progress value is properly carried over when duplicating an applied control.frontend/messages/en.json (1)
1143-1144: LGTM! Translations added for progress field.The translations are properly added:
redZoneRadiustranslation is reinstatedprogressFieldtranslation is added for the new progress fieldfrontend/messages/fr.json (1)
1142-1143: LGTM! The translations are accurate.Both translations are grammatically correct and accurately represent their English counterparts:
- "Rayon de la zone rouge" correctly translates "red zone radius"
- "Progrès" correctly translates "progress field"
backend/core/models.py
Outdated
| progress_field = models.IntegerField(default=0, verbose_name=_("Progress Field")) | ||
|
|
There was a problem hiding this comment.
Create a migration file for the new field.
A migration file is needed to add the progress_field to the database schema.
Run Django's makemigrations command to generate the migration file:
#!/bin/bash
# Description: Check if migration file exists and show how to create it.
echo "Checking for migration file..."
find backend/core/migrations -type f -name "*_appliedcontrol_progress_field.py"
echo -e "\nIf no migration file is found, create one by running:"
echo "python manage.py makemigrations core"🛠️ Refactor suggestion
Add validators to ensure progress stays within 0-100 range.
The progress_field should include validators to ensure values stay within the valid range.
Apply this diff to add validators:
- progress_field = models.IntegerField(default=0, verbose_name=_("Progress Field"))
+ progress_field = models.IntegerField(
+ default=0,
+ verbose_name=_("Progress Field"),
+ validators=[
+ MinValueValidator(0, message="Progress cannot be less than 0"),
+ MaxValueValidator(100, message="Progress cannot be more than 100")
+ ]
+ )📝 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.
| progress_field = models.IntegerField(default=0, verbose_name=_("Progress Field")) | |
| progress_field = models.IntegerField( | |
| default=0, | |
| verbose_name=_("Progress Field"), | |
| validators=[ | |
| MinValueValidator(0, message="Progress cannot be less than 0"), | |
| MaxValueValidator(100, message="Progress cannot be more than 100") | |
| ] | |
| ) |
|
@melinoix the color scheme should be inverted: going from red (0) to green (100), |
…hub.com:intuitem/ciso-assistant-community into CA-839-add-a-progress-field-on-applied-controls
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/core/models.py (1)
1857-1864: Add help text to document the field's behavior.Consider adding help_text to document that the progress field represents a percentage (0-100) and its automatic update behavior when status changes to "active".
progress_field = models.IntegerField( default=0, verbose_name=_("Progress Field"), + help_text=_("Progress percentage (0-100). Automatically set to 100 when status becomes active."), validators=[ MinValueValidator(0, message="Progress cannot be less than 0"), MaxValueValidator(100, message="Progress cannot be more than 100"), ], )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/core/migrations/0050_appliedcontrol_progress_field.py(1 hunks)backend/core/models.py(3 hunks)frontend/src/lib/components/Forms/ModelForm/AppliedControlPolicyForm.svelte(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/core/migrations/0050_appliedcontrol_progress_field.py
- frontend/src/lib/components/Forms/ModelForm/AppliedControlPolicyForm.svelte
⏰ 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: functional-tests (3.12, chromium)
- 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: test (3.12)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
| if self.status == "active": | ||
| self.progress_field = 100 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve status handling in the save method.
Consider these improvements:
- Use the Status enum instead of string literal for better type safety
- Handle the case when status changes from "active" to another status
- if self.status == "active":
+ if self.status == self.Status.ACTIVE:
self.progress_field = 100
+ elif self.status != self.Status.ACTIVE and self._state.adding is False:
+ # Only update progress if this is not a new instance
+ original = type(self).objects.get(pk=self.pk)
+ if original.status == self.Status.ACTIVE:
+ # Reset progress when moving from active to another status
+ self.progress_field = 0Committable suggestion skipped: line range outside the PR's diff.
This is a new field. It is an integer between 0 to 100 (percentage). Defaults to 0. Cannot be Null.
When the status is set to “active”, this field is set to 100.
Summary by CodeRabbit
Release Notes
New Features
Internationalization
User Interface
Backend