Data obj show use params #1160
Data obj show use params #1160
Conversation
…rade # Conflicts: # spiffworkflow-backend/poetry.lock # spiffworkflow-backend/src/spiffworkflow_backend/services/process_instance_processor.py
… view the contents of a data object from the web ui
WalkthroughWalkthroughThe recent updates in the 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: 8
Configuration used: .coderabbit.yaml
Files selected for processing (9)
- spiffworkflow-backend/poetry.lock (3 hunks)
- spiffworkflow-backend/pyproject.toml (2 hunks)
- spiffworkflow-backend/src/spiffworkflow_backend/models/message_instance.py (1 hunks)
- spiffworkflow-backend/src/spiffworkflow_backend/services/process_instance_processor.py (1 hunks)
- spiffworkflow-backend/src/spiffworkflow_backend/services/script_unit_test_runner.py (1 hunks)
- spiffworkflow-backend/tests/data/random_fact/random_fact_set.bpmn (1 hunks)
- spiffworkflow-backend/tests/spiffworkflow_backend/integration/test_json_file_data_store.py (1 hunks)
- spiffworkflow-backend/tests/spiffworkflow_backend/unit/test_process_instance_processor.py (1 hunks)
- spiffworkflow-frontend/src/routes/ProcessInstanceShow.tsx (1 hunks)
Additional comments: 25
spiffworkflow-backend/tests/spiffworkflow_backend/integration/test_json_file_data_store.py (1)
- 10-10: The class name
TestJSONFileDataStorehas been correctly renamed toTestJsonFileDataStoreto follow Python naming conventions.spiffworkflow-backend/tests/data/random_fact/random_fact_set.bpmn (1)
- 1-51: The BPMN process definition for "Process_SecondFact" appears to be correctly structured and complete, with a start event, a script task for fetching a fact from an API, and an end event with documentation.
spiffworkflow-backend/src/spiffworkflow_backend/models/message_instance.py (1)
- 117-117: Replacing
_evaluatewithenvironment.evaluateenhances code clarity and maintainability by using a more descriptive method name. Ensure that theenvironment.evaluatemethod is correctly implemented and tested for this use case.spiffworkflow-backend/pyproject.toml (3)
- 29-29: Updating the
SpiffWorkflowdependency to track themainbranch is a significant change that could introduce instability. Ensure that continuous integration tests are in place to catch any issues arising from future changes to theSpiffWorkflowproject.Consider adding or enhancing CI tests to automatically test against the latest changes in the
SpiffWorkflowmain branch.
- 107-107: Downgrading
pytest-xdistto3.3.1due to compatibility issues is understandable. However, keep an eye on future releases for potential fixes that allow upgrading to newer versions.- 107-107: Downgrading
banditto1.7.7addresses specific issues introduced in1.7.3. It's good practice to periodically review dependency versions for possible upgrades that may resolve the issues or introduce new features.spiffworkflow-backend/tests/spiffworkflow_backend/unit/test_process_instance_processor.py (15)
- 35-51: The test method
test_script_engine_can_use_custom_scriptsis well-structured and follows good testing practices. However, consider externalizing the expected outcomes (e.g., the hardcoded string in the assertion) to a configuration or a separate data file. This approach can improve maintainability if the underlying scripts or expected outcomes change in the future.- 27-54: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [52-112]
The test method
test_sets_permission_correctly_on_human_taskis comprehensive and effectively checks permission settings on human tasks. Consider using constants or configuration files for hardcoded identifiers (e.g., "Finance Team") and user names to improve maintainability and ease of updates.
- 27-54: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [113-213]
The test method
test_sets_permission_correctly_on_human_task_when_using_dictthoroughly checks permission settings on human tasks using a dictionary for potential owners. Similar to previous suggestions, consider using constants or configuration files for hardcoded identifiers and user names to enhance maintainability.
- 27-54: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [214-226]
The test method
test_can_load_up_processor_after_running_model_with_call_activitieseffectively checks if theProcessInstanceProcessorcan be loaded correctly after running a model with call activities. It follows good testing practices and clearly asserts the expected outcome.
- 27-54: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [227-273]
The test method
test_properly_resets_process_to_given_taskis comprehensive and effectively checks the functionality related to resetting a process instance to a specific task. It follows good testing practices and clearly asserts the expected outcomes after the reset.
- 27-54: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [274-366]
The test method
test_properly_resets_process_to_given_task_with_call_activitythoroughly checks the functionality related to resetting a process instance to a specific task within a call activity. It is comprehensive and clearly asserts the expected outcomes after the reset, effectively handling the complexity of call activities and subprocesses.
- 27-54: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [367-406]
The test method
test_properly_resets_process_on_tasks_with_boundary_eventseffectively checks the functionality related to resetting a process instance on tasks with boundary events. It follows good testing practices and clearly asserts the expected outcomes after the reset.
- 27-54: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [407-438]
The test method
test_step_through_gatewayeffectively checks the functionality related to stepping through a gateway. It follows good testing practices and clearly asserts the expected outcomes before and after the gateway.
- 27-54: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [439-631]
The test method
test_properly_saves_tasks_when_runningis comprehensive and effectively checks the functionality related to saving tasks when the process instance is running. It handles the complexity of multiple data sets and execution strategies well, clearly asserting the expected outcomes.
- 27-54: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [632-652]
The test method
test_does_not_recreate_human_tasks_on_multiple_saveseffectively checks the functionality related to not recreating human tasks on multiple saves of the process instance. It follows good testing practices and clearly asserts the expected outcome.
- 27-54: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [653-670]
The test method
test_it_can_loopback_to_previous_bpmn_task_with_gatewayeffectively checks the functionality related to looping back to a previous BPMN task with a gateway. It follows good testing practices and clearly asserts the expected outcomes.
- 27-54: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [671-695]
The test method
test_it_can_loopback_to_previous_bpmn_subprocess_with_gatewayeffectively checks the functionality related to looping back to a previous BPMN subprocess with a gateway. It follows good testing practices and clearly asserts the expected outcomes.
- 27-54: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [696-724]
The test method
test_task_data_is_set_even_if_process_instance_errors_and_creates_task_failed_eventeffectively checks the functionality related to setting task data even in the event of a process instance error. It follows good testing practices and clearly asserts the expected outcomes.
- 27-54: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [725-736]
The test method
test_can_complete_task_with_call_activity_after_manual_taskeffectively checks the functionality related to completing a task with a call activity after a manual task. It follows good testing practices and clearly asserts the expected outcomes.
- 27-54: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [737-769]
The test method
test_can_store_instructions_for_end_usereffectively checks the functionality related to storing instructions for the end user. It follows good testing practices and clearly asserts the expected outcomes.spiffworkflow-frontend/src/routes/ProcessInstanceShow.tsx (1)
- 797-803: The logic for constructing
additionalParamsbased on the presence ofprocess_identifierandbpmn_process_guidin the search parameters is a good approach to dynamically handle query parameters. However, ensure that the values retrieved fromsearchParams.get()are properly sanitized and validated before use to prevent any potential security issues.spiffworkflow-backend/poetry.lock (3)
- 2219-2225: The downgrade of
pytest-xdistto version3.3.1is noted. Ensure that this version is compatible with the project's requirements and that all tests pass with this version.- 2977-2978: The update to track the
mainbranch ofSpiffWorkflowis acknowledged. Consider implementing a strategy to manage potential instability from tracking a dynamic branch, such as regular compatibility testing or pinning to specific stable commits for releases.- 3454-3454: The update to the
content-hashis appropriate and reflects the changes made to the dependencies. This ensures the integrity of thepoetry.lockfile.
spiffworkflow-backend/src/spiffworkflow_backend/services/script_unit_test_runner.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: .coderabbit.yaml
Files selected for processing (1)
- spiffworkflow-backend/src/spiffworkflow_backend/services/process_instance_processor.py (1 hunks)
Additional comments: 3
spiffworkflow-backend/src/spiffworkflow_backend/services/process_instance_processor.py (3)
- 350-357: The
evaluatemethod in theCustomBpmnScriptEngineclass directly interacts with the superclass'sevaluatemethod, passing along a potentially augmentedmethodsdictionary as theexternal_context. This design is sound, but ensure that the augmentation process in__get_augment_methodsdoes not inadvertently introduce security vulnerabilities, especially when merging external context data. Consider validating or sanitizing the external context data before merging.- 350-357: In the
CustomBpmnScriptEngineclass, theevaluatemethod's exception handling is robust, distinguishing between a generalWorkflowExceptionand a more specificWorkflowTaskException. This granularity in error handling is beneficial for debugging and error tracking. However, ensure that sensitive information is not leaked in the error messages, especially when these exceptions are logged or exposed to end-users.- 350-357: The
terminatemethod in theProcessInstanceProcessorclass correctly handles the process termination by updating the process instance status and committing the changes to the database. This method also invokesremove_spiff_tasks_for_terminationto clean up related tasks. Ensure that all related resources, such as future tasks or subprocesses, are also appropriately handled during termination to avoid leaving the system in an inconsistent state.
spiffworkflow-backend/src/spiffworkflow_backend/services/process_instance_processor.py
Outdated
Show resolved
Hide resolved
spiffworkflow-backend/src/spiffworkflow_backend/services/process_instance_processor.py
Outdated
Show resolved
Hide resolved
# Conflicts: # spiffworkflow-backend/src/spiffworkflow_backend/services/script_unit_test_runner.py
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yaml
Files selected for processing (1)
- .gitignore (1 hunks)
Additional comments: 1
.gitignore (1)
- 13-13: The addition of
.mypy_cacheto the.gitignorefile is a good practice for Python projects using MyPy, as it prevents the MyPy cache directory from being tracked by version control. This change aligns with the PR's objectives of enhancing dependency management and improving the development workflow.
This allows the frontend to fallback to using search params for the process identifier and guid if a corresponding task cannot be found. If no task is found then this is most likely inside a call activity where we already know from params where the object should be coming from.
Summary by CodeRabbit
pytest-xdistandbanditdependencies.