feat: add ref id to assets #1509
Conversation
WalkthroughThe changes update how the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AssetForm
participant SchemaValidator
participant API
participant Database
User->>AssetForm: Enter ref_id value
AssetForm->>SchemaValidator: Validate ref_id (max 100 chars)
SchemaValidator-->>AssetForm: Return validation result
AssetForm->>API: Submit asset data with ref_id
API->>Database: Save asset record (including ref_id)
Database-->>API: Confirm data persistence
API-->>User: Return creation/update confirmation
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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: 0
🔭 Outside diff range comments (1)
backend/core/models.py (1)
2664-2664: 🛠️ Refactor suggestionAlign field validation with the default reference ID format.
The
get_default_ref_idmethod generates IDs in the format "R.{number}", but the field lacks validation to ensure manually entered IDs follow this format.- ref_id = models.CharField(max_length=8, blank=True, verbose_name=_("Reference ID")) + ref_id = models.CharField( + max_length=8, + blank=True, + validators=[ + RegexValidator( + regex=r'^R\.\d+$', + message='Reference ID must be in format R.{number}', + code='invalid_risk_ref_id' + ) + ], + verbose_name=_("Reference ID") + )
🧹 Nitpick comments (1)
backend/core/models.py (1)
1575-1575: Consider adding constraints and validation for the reference ID field.While the field definition is correct, consider the following improvements:
- Add a unique constraint if reference IDs should be unique across assets
- Add a validator to enforce a specific format (e.g., alphanumeric)
- ref_id = models.CharField(max_length=8, blank=True, verbose_name=_("Reference ID")) + ref_id = models.CharField( + max_length=8, + blank=True, + unique=True, # If ref_ids should be unique + validators=[ + RegexValidator( + regex=r'^[A-Za-z0-9-_.]+$', + message='Reference ID must be alphanumeric', + code='invalid_ref_id' + ) + ], + verbose_name=_("Reference ID") + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/core/migrations/0053_asset_ref_id.py(1 hunks)backend/core/models.py(1 hunks)frontend/src/lib/components/Forms/ModelForm/AssetForm.svelte(1 hunks)frontend/src/lib/utils/schemas.ts(1 hunks)frontend/src/lib/utils/table.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- 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: build (3.12)
- GitHub Check: ruff (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: test (3.12)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (4)
backend/core/migrations/0053_asset_ref_id.py (1)
1-19: LGTM! Migration looks good.The migration correctly adds the
ref_idfield to theassetmodel with appropriate constraints.frontend/src/lib/components/Forms/ModelForm/AssetForm.svelte (1)
73-79: LGTM! Form field implementation looks good.The TextField component for
ref_idis properly implemented with all necessary props and data binding.frontend/src/lib/utils/schemas.ts (1)
222-223: LGTM! Schema definition looks good.The
ref_idfield is properly defined in the AssetSchema with matching constraints (optional, max length 8).frontend/src/lib/utils/table.ts (1)
571-571: LGTM! Table configuration looks good.The
ref_idfield is properly added to both the header and body configurations of the assets table.Also applies to: 582-582
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/core/migrations/0054_alter_asset_ref_id_alter_riskscenario_ref_id.py (1)
5-5: Fix formatting issue.Remove the extra newline at line 5 to adhere to the formatting standards.
from django.db import migrations, models - class Migration(migrations.Migration):
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/core/migrations/0054_alter_asset_ref_id_alter_riskscenario_ref_id.py(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Backend Linters
backend/core/migrations/0054_alter_asset_ref_id_alter_riskscenario_ref_id.py
[warning] 1-1: File would be reformatted. Please adhere to the formatting standards.
⏰ 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-functional-tests (3.12, chromium)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: test (3.12)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: build (3.12)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
backend/core/migrations/0054_alter_asset_ref_id_alter_riskscenario_ref_id.py (3)
13-17: LGTM! Field modification for Asset model looks good.The field attributes are well-defined and the change from max_length=8 to max_length=100 is non-breaking.
18-22:✅ Verification successful
LGTM! Field modification for RiskScenario model looks good.
The field attributes are consistent with the Asset model and well-defined.
Let's verify if there's any existing data that needs attention:
🏁 Script executed:
#!/bin/bash # Description: Check for existing ref_id values in both models # that might be affected by the field modification # Search for model definitions to understand the table names rg -A 5 "class Asset\(" rg -A 5 "class RiskScenario\(" # Note: In a real environment, you would need to check the database # for records where ref_id length > 8 charactersLength of output: 712
Field Modification Approved for RiskScenario Model
The migration altering the
ref_idfield in the RiskScenario model (usingCharField(blank=True, max_length=100)) is consistent with the Asset model’s configuration. The executed script confirmed that both models are defined as expected.Please ensure that, in your production environment, any existing records with
ref_idvalues that might exceed prior length constraints are reviewed and handled appropriately.
1-1:❓ Verification inconclusive
Verify Django version compatibility.
The migration is generated using Django 5.1.5. Let's verify if this version is stable and compatible with the project's requirements.
🌐 Web query:
What is the latest stable version of Django?💡 Result:
The latest stable version of Django is 5.1.6, released on February 5, 2025[1][4][7]. This version supports Python 3.10, 3.11, 3.12, and Python 3.13 (added in Django 5.1.3)[1].
Key details:
- Security support for Django 5.1 will continue until December 31, 2025[2]
- Installation command:
python -m pip install Django==5.1.6(Linux/macOS)
py -m pip install Django==5.1.6(Windows)[4]- The previous LTS version remains 4.2.x (supported until April 2026)[2]
For production environments, using the latest stable release (currently v5.x) is recommended to benefit from security updates and new features[4].
Citations:
- 1: https://docs.djangoproject.com/en/5.1/faq/install/
- 2: https://endoflife.date/django
- 3: https://docs.djangoproject.com/en/5.1/releases/5.0/
- 4: https://www.djangoproject.com/download/
- 5: https://docs.djangoproject.com/en/5.1/releases/
- 6: https://www.django-rest-framework.org/community/release-notes/
- 7: https://pypi.org/project/Django/
- 8: https://en.wikipedia.org/wiki/Django_(web_framework)
Django Version Compatibility Verification
The migration header shows it was generated with Django 5.1.5, but the latest stable release is now Django 5.1.6. Please verify that your project’s environment is intentionally using 5.1.5. If not, consider upgrading to Django 5.1.6 to benefit from the latest security updates and fixes.
- Confirm that the Django version in your virtual environment or requirements is set as expected.
- If an upgrade is planned, update the migration header accordingly.
🧰 Tools
🪛 GitHub Actions: Backend Linters
[warning] 1-1: File would be reformatted. Please adhere to the formatting standards.
backend/core/migrations/0054_alter_asset_ref_id_alter_riskscenario_ref_id.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/core/migrations/0053_asset_ref_id_alter_riskscenario_ref_id.py (1)
1-27: Consider performance impact on large tables.For large tables, altering column definitions could potentially lock the table during the migration.
Consider:
- Running this migration during off-peak hours
- If the tables are large, use Django's database constraints to add the field without locking:
migrations.SeparateDatabaseAndState( state_operations=[...], # Your current operations database_operations=[...] # Custom SQL with ALTER TABLE LOCK=NONE )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/core/migrations/0053_asset_ref_id_alter_riskscenario_ref_id.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: build (3.12)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
backend/core/migrations/0053_asset_ref_id_alter_riskscenario_ref_id.py (2)
11-18: LGTM! Adding ref_id to Asset model.The field definition is consistent and well-structured with appropriate attributes.
Before applying this migration in production, verify that no unique constraints are needed on the
ref_idfield. If reference IDs should be unique per asset, consider adding a unique constraint:field=models.CharField( - blank=True, max_length=100, verbose_name="Reference ID" + blank=True, max_length=100, verbose_name="Reference ID", unique=True ),
19-25:❓ Verification inconclusive
Verify data safety for RiskScenario ref_id modification.
The field modification looks good, but since we're altering an existing field from 8 to 100 characters, ensure data safety.
Run this script to check for any existing data that might be affected:
🏁 Script executed:
#!/bin/bash # Description: Check for RiskScenario ref_ids that might be affected by the migration # Use Django's shell to inspect existing data python manage.py shell << EOF from core.models import RiskScenario scenarios = RiskScenario.objects.exclude(ref_id='').values_list('ref_id', flat=True) print("Current ref_ids:", list(scenarios)) print("Max length found:", max((len(id) for id in scenarios), default=0)) EOFLength of output: 116
Attention: Verify RiskScenario ref_id Data Safety Before Migration
The migration modifying
RiskScenario.ref_idappears correctly implemented.The initial verification script failed because
manage.pywas not found in the expected path. It’s likely that the Django project’s entry point is located elsewhere (e.g., within thebackenddirectory).Action Required: Please re-run the verification script from the correct project directory. For example:
# Run from the repository root cd backend python manage.py shell << EOF from core.models import RiskScenario scenarios = RiskScenario.objects.exclude(ref_id='').values_list('ref_id', flat=True) print("Current ref_ids:", list(scenarios)) print("Max length found:", max((len(id) for id in scenarios), default=0)) EOFOnce you confirm that existing
ref_idvalues are within safe limits, the migration can be considered safe.
Summary by CodeRabbit