feature/move-message-start-events #1070
Conversation
…he corresponding file w/ burnettk
WalkthroughThe update introduces a new attribute Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: .coderabbit.yaml
Files ignored due to path filters (1)
spiffworkflow-backend/migrations/versions/c6e246c3c04e_.pyis excluded by:!spiffworkflow-backend/migrations/**
Files selected for processing (3)
- spiffworkflow-backend/src/spiffworkflow_backend/models/message_triggerable_process_model.py (1 hunks)
- spiffworkflow-backend/src/spiffworkflow_backend/services/spec_file_service.py (2 hunks)
- spiffworkflow-backend/tests/spiffworkflow_backend/unit/test_message_service.py (2 hunks)
Additional comments: 2
spiffworkflow-backend/src/spiffworkflow_backend/models/message_triggerable_process_model.py (1)
- 11-11: The addition of the
file_nameattribute to theMessageTriggerableProcessModelclass looks good and follows the established pattern for defining model attributes. Ensure that the corresponding database migration script is in place to add this column to the database.spiffworkflow-backend/tests/spiffworkflow_backend/unit/test_message_service.py (1)
- 257-291: The new test method
test_can_delete_message_start_events_from_database_if_model_no_longer_references_itis well-structured and effectively tests the functionality of deleting obsolete message start events from the database. Ensure that additional tests are in place to cover edge cases and scenarios where multiple message start events might be affected.
| message_triggerable_process_model = MessageTriggerableProcessModel( | ||
| message_name=message_name, | ||
| process_model_identifier=ref.relative_location, | ||
| file_name=ref.file_name, | ||
| ) | ||
| db.session.add(message_triggerable_process_model) | ||
| else: | ||
| if message_triggerable_process_model.process_model_identifier != ref.relative_location: | ||
| raise ProcessModelFileInvalidError( | ||
| f"Message model is already used to start process model {ref.relative_location}" | ||
| ) | ||
| elif message_triggerable_process_model.file_name is None: | ||
| message_triggerable_process_model.file_name = ref.file_name | ||
| db.session.add(message_triggerable_process_model) | ||
| current_triggerable_processes.remove(message_triggerable_process_model) | ||
| for trigger_pm in current_triggerable_processes: | ||
| db.session.delete(trigger_pm) | ||
|
|
||
| @staticmethod | ||
| def update_correlation_cache(ref: Reference) -> None: |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [313-337]
The updates to the update_message_trigger_cache method incorporate necessary logic for managing message triggerable processes in relation to their associated files. However, modifying a list (current_triggerable_processes) while iterating over it could lead to unexpected behavior. Consider using a different approach, such as filtering the list comprehensively before the loop or accumulating items to delete in a separate list and deleting them in a subsequent step, to avoid potential issues.
- for trigger_pm in current_triggerable_processes:
- db.session.delete(trigger_pm)
+ to_delete = [trigger_pm for trigger_pm in current_triggerable_processes if condition]
+ for trigger_pm in to_delete:
+ db.session.delete(trigger_pm)
Implements #1027
This adds a "file_name" column to message_triggerable_process_model and will delete the process from the table if it is no longer found in the file when updating the cache for that file.
Summary by CodeRabbit