-
Notifications
You must be signed in to change notification settings - Fork 567
feat: Make data wizard update objects #3075
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
WalkthroughSingle-file refactor of Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
backend/data_wizard/views.py (2)
523-613: Consider extending upsert behavior to remaining_process_*methods for consistency.The upsert pattern (lookup existing instance before serialization) has been implemented for Users, Assets, AppliedControls, Perimeters, and ElementaryActions. However,
_process_reference_controls,_process_threats,_process_folders,_process_processings, and TPRM-related methods still use create-only behavior.If the intent is to support full upsert semantics across the data wizard, consider applying the same pattern to these methods in a follow-up. If this is intentional (e.g., these entities should always create new records), documenting this distinction would help future maintainers.
265-265: Consider updating log messages to reflect upsert behavior.Log messages like
"Error creating asset"don't reflect that the operation might be an update. Consider using"Error saving asset"or"Error creating/updating asset"for accuracy.This applies to similar log messages in
_process_applied_controls(line 331),_process_perimeters(line 387),_process_elementary_actions(line 513), and_process_users(line 212).
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/data_wizard/views.py(13 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: 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: build_enterprise_frontend
- GitHub Check: build_community_frontend
- GitHub Check: build (3.12)
- GitHub Check: test (3.12)
- GitHub Check: Analyze (python)
🔇 Additional comments (6)
backend/data_wizard/views.py (6)
8-9: Import changes look correct.The added imports (
ElementaryAction,Asset) are required for the new instance lookup functionality in the upsert flow. Narrowingtprm.modelsto onlyEntityis appropriate sinceSolutionandContractare only accessed via their serializers.Also applies to: 18-18, 36-36
179-218: Upsert pattern for users looks correct.The implementation properly:
- Validates the mandatory email field before lookup
- Uses email as the unique identifier to find existing users
- Passes the found instance (or
None) to the serializer for create/update behavior
246-253: Asset upsert with composite key (name, folder) is appropriate.The lookup correctly uses both
nameandfolderto identify existing assets, which prevents unintended cross-folder updates. Django's ORM accepts folder IDs directly for ForeignKey lookups.
310-319: Applied control upsert follows the same consistent pattern.Uses the same composite key approach (name, folder) as assets, maintaining consistency across entity types.
368-375: Perimeter upsert maintains consistency with other entities.Follows the established composite key pattern (name, folder) for instance lookup.
492-501: Elementary action upsert completes the consistent pattern across all entity types.The implementation properly uses the newly imported
ElementaryActionmodel for instance lookups, maintaining the same composite key approach.
The update doesn't concern the following objects:
These objects are created with the f"{object_type}_{timestamp}" format which is meant to be a unique name generated by the backend (not user-provided and therefore not updatable)