Skip to content
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

Pi migration 2 #1902

Merged
merged 15 commits into from
Jul 11, 2024
Merged

Pi migration 2 #1902

merged 15 commits into from
Jul 11, 2024

Conversation

jasquat
Copy link
Contributor

@jasquat jasquat commented Jul 10, 2024

Supports #1671

This adds a web ui and several fixes the process instance migration code.

Adds:

  • support to revert to old version
  • add process_instance_migration_event_detail table
    • this stores the initial and target sha256 forms of the spiff serialized process model
  • adds web page to handle migrations
  • new api to get migration events:
    • GET /process-instance-events/{modified_process_model_identifier}/{process_instance_id}/migration

Summary by CodeRabbit

  • New Features

    • Introduced process instance migration event listing and migration details tracking.
    • Added optional target_bpmn_process_hash parameter for process instance migration.
  • Bug Fixes

    • Updated email addresses in acceptance tests configuration.
  • Enhancements

    • Expanded permissions for reading process instance events.
    • Modified environment variable in docker-compose.yml.
  • Refactor

    • Applied @dataclass decorator to ProcessInstanceEventModel.
  • Documentation

    • Improved API documentation with new query parameters and endpoint paths for migration events.

Copy link
Contributor

coderabbitai bot commented Jul 10, 2024

Walkthrough

Walkthrough

The changes encompass several enhancements and fixes, including the introduction of new query parameters and endpoints for process instance migration events, updates to authorization handling, and the addition of new exception and data models. Backend service methods have been expanded to handle process migration details, and minor updates to frontend components and configuration files have been made to support these backend changes. The environment setup and tests have also been fine-tuned.

Changes

Files Change Summary
spiffworkflow-backend/src/spiffworkflow_backend/api.yml Added query parameters and new endpoint path for process instance migration events, updated permissions.
spiffworkflow-backend/src/spiffworkflow_backend/exceptions/error.py Introduced new exception class ProcessInstanceMigrationUnnecessaryError.
spiffworkflow-backend/src/spiffworkflow_backend/models/process_instance_event.py Added @dataclass decorator, new relationship declaration for migration_details, and related fields.
spiffworkflow-backend/src/spiffworkflow_backend/models/process_instance_migration_detail.py Added new dataclass ProcessInstanceMigrationDetailModel with fields for migration details and relationships.
spiffworkflow-backend/src/spiffworkflow_backend/routes/process_instance_events_controller.py Added function process_instance_migration_event_list to handle listing migration events.
spiffworkflow-backend/src/spiffworkflow_backend/routes/process_instances_controller.py Updated to handle ProcessInstanceMigrationUnnecessaryError and include a target_bpmn_process_hash parameter for migrations.
spiffworkflow-backend/src/spiffworkflow_backend/services/authorization_service.py Expanded permissions to include access to /process-instance-events paths for reading.
spiffworkflow-backend/src/spiffworkflow_backend/services/process_instance_processor.py Updated _get_full_bpmn_process_dict method to accept additional parameters and included new logic.
spiffworkflow-backend/src/spiffworkflow_backend/services/process_instance_service.py Introduced logic for process instance migration, added methods check_process_instance_can_be_migrated and migrate_process_instance, handled ProcessInstanceMigrationUnnecessaryError.
spiffworkflow-backend/src/spiffworkflow_backend/services/process_instance_tmp_service.py Added ProcessInstanceMigrationDetailDict and ProcessInstanceMigrationDetailModel to handle migration details.
spiffworkflow-frontend/src/a-spiffui-v2/views/Dashboards/myProcesses/CellRenderer.tsx Updated CellRenderer to accept an optional title prop and conditionally render content.
spiffworkflow-frontend/src/a-spiffui-v2/views/Dashboards/myProcesses/MyProcesses.tsx Added a console.log statement in the renderCell function.
spiffworkflow-backend/bin/local_development_environment_setup Adjusted conditional checks for setting use_local_open_id and acceptance_test_mode based on command-line arguments.
spiffworkflow-backend/src/spiffworkflow_backend/config/permissions/acceptance_tests.yml Updated email addresses for ciadmin1 user and admin group.
spiffworkflow-backend/tests/spiffworkflow_backend/integration/test_process_instances_controller.py Added assertions for current_bpmn_process_hash and current_git_revision in response JSON.
spiffworkflow-backend/docker-compose.yml Modified SPIFFWORKFLOW_BACKEND_OPEN_ID_SERVER_URL environment variable.
sequenceDiagram
    participant User
    participant Frontend
    participant Backend
    participant AuthService
    participant ProcessService

    User ->> Frontend: Request migration event list
    Frontend ->> Backend: GET /process-instance-events/{...}/migration
    Backend ->> AuthService: Check read permissions
    AuthService -->> Backend: Permission granted
    Backend ->> ProcessService: Fetch migration events
    ProcessService -->> Backend: Migration events data
    Backend -->> Frontend: JSON response with migration events
    Frontend -->> User: Display migration events
    
    User ->> Frontend: Initiate process migration
    Frontend ->> Backend: POST /process-instances/{id}/migrate
    Backend ->> ProcessService: Migrate process instance
    ProcessService -->> Backend: Migration success/failure
    Backend -->> Frontend: JSON response with migration status
    Frontend -->> User: Display migration status
Loading

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?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 10

Outside diff range, codebase verification and nitpick comments (4)
spiffworkflow-backend/src/spiffworkflow_backend/routes/process_instances_controller.py (1)

577-577: Document the new parameter target_bpmn_process_hash.

Add a docstring or inline comment to describe the purpose of the target_bpmn_process_hash parameter.

def process_instance_migrate(
    process_instance_id: int,
    modified_process_model_identifier: str,
    target_bpmn_process_hash: str | None = None,  # Target BPMN process hash for migration
) -> flask.wrappers.Response:
spiffworkflow-backend/src/spiffworkflow_backend/services/process_instance_service.py (1)

154-189: Update function calls to match the new signature.

The following function calls to check_process_instance_can_be_migrated need to be updated to include the target_bpmn_process_hash parameter:

  • spiffworkflow-backend/tests/spiffworkflow_backend/unit/test_process_instance_service.py at lines 351 and 394
  • spiffworkflow-backend/src/spiffworkflow_backend/routes/process_instances_controller.py at line 563
Analysis chain

LGTM! But verify the function usage in the codebase.

The code changes in check_process_instance_can_be_migrated are approved.

However, ensure that all function calls to check_process_instance_can_be_migrated match the new signature.


We need to correct our approach to locate the function usage. We'll use fd to find all Python files and then search within those files for the function check_process_instance_can_be_migrated.


We need to verify if the usages of the function check_process_instance_can_be_migrated in the identified files match the new signature.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify all function calls to `check_process_instance_can_be_migrated` match the new signature.

# Test: Search for the function usage. Expect: Only occurrences of the new signature.
rg --type python -A 5 $'check_process_instance_can_be_migrated'

Length of output: 93


Script:

#!/bin/bash
# Description: Verify all function calls to `check_process_instance_can_be_migrated` match the new signature.

# Find all Python files and search for the function usage.
fd -e py -x grep -Hn 'check_process_instance_can_be_migrated' {}

Length of output: 969


Script:

#!/bin/bash
# Extract specific lines from the identified files to verify the function usage.

# Verify usage in test_process_instance_service.py
sed -n '351p;394p' ./spiffworkflow-backend/tests/spiffworkflow_backend/unit/test_process_instance_service.py

# Verify usage in process_instance_service.py
sed -n '227p' ./spiffworkflow-backend/src/spiffworkflow_backend/services/process_instance_service.py

# Verify usage in process_instances_controller.py
sed -n '563p' ./spiffworkflow-backend/src/spiffworkflow_backend/routes/process_instances_controller.py

Length of output: 706

spiffworkflow-backend/src/spiffworkflow_backend/services/process_instance_processor.py (2)

922-922: Remove debug print statement.

The debug print statement is not necessary and should be removed to clean up the code.

- # print(f"➡️ ➡️ ➡️  process_copy.keys(): {process_copy.keys()}")

922-922: Remove debug print statement.

The debug print statement is not necessary and should be removed to clean up the code.

- # print(f"➡️ ➡️ ➡️  process_copy.keys(): {process_copy.keys()}")
Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 6d16438 and 1aa3861.

Files ignored due to path filters (2)
  • spiffworkflow-backend/migrations/versions/ac125644907a_.py is excluded by !spiffworkflow-backend/migrations/**
  • spiffworkflow-backend/poetry.lock is excluded by !**/*.lock
Files selected for processing (23)
  • spiffworkflow-backend/src/spiffworkflow_backend/api.yml (2 hunks)
  • spiffworkflow-backend/src/spiffworkflow_backend/exceptions/error.py (1 hunks)
  • spiffworkflow-backend/src/spiffworkflow_backend/models/process_instance_event.py (3 hunks)
  • spiffworkflow-backend/src/spiffworkflow_backend/models/process_instance_migration_detail.py (1 hunks)
  • spiffworkflow-backend/src/spiffworkflow_backend/routes/process_instance_events_controller.py (2 hunks)
  • spiffworkflow-backend/src/spiffworkflow_backend/routes/process_instances_controller.py (3 hunks)
  • spiffworkflow-backend/src/spiffworkflow_backend/services/authorization_service.py (3 hunks)
  • spiffworkflow-backend/src/spiffworkflow_backend/services/process_instance_processor.py (4 hunks)
  • spiffworkflow-backend/src/spiffworkflow_backend/services/process_instance_service.py (5 hunks)
  • spiffworkflow-backend/src/spiffworkflow_backend/services/process_instance_tmp_service.py (3 hunks)
  • spiffworkflow-backend/tests/spiffworkflow_backend/helpers/base_test.py (2 hunks)
  • spiffworkflow-backend/tests/spiffworkflow_backend/integration/test_process_instance_events_controller.py (1 hunks)
  • spiffworkflow-backend/tests/spiffworkflow_backend/integration/test_process_instances_controller.py (1 hunks)
  • spiffworkflow-backend/tests/spiffworkflow_backend/scripts/test_get_all_permissions.py (1 hunks)
  • spiffworkflow-backend/tests/spiffworkflow_backend/unit/test_authorization_service.py (5 hunks)
  • spiffworkflow-backend/tests/spiffworkflow_backend/unit/test_process_instance_service.py (5 hunks)
  • spiffworkflow-frontend/src/a-spiffui-v2/views/Dashboards/myProcesses/CellRenderer.tsx (2 hunks)
  • spiffworkflow-frontend/src/a-spiffui-v2/views/Dashboards/myProcesses/MyProcesses.tsx (1 hunks)
  • spiffworkflow-frontend/src/hooks/UriListForPermissions.tsx (1 hunks)
  • spiffworkflow-frontend/src/interfaces.ts (1 hunks)
  • spiffworkflow-frontend/src/routes/ProcessInstanceMigratePage.tsx (1 hunks)
  • spiffworkflow-frontend/src/routes/ProcessInstanceRoutes.tsx (2 hunks)
  • spiffworkflow-frontend/src/routes/ProcessInstanceShow.tsx (5 hunks)
Files skipped from review due to trivial changes (1)
  • spiffworkflow-backend/src/spiffworkflow_backend/exceptions/error.py
Additional context used
Biome
spiffworkflow-frontend/src/routes/ProcessInstanceMigratePage.tsx

[error] 353-353: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)


[error] 34-34: Don't use 'Function' as a type.

Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.

(lint/complexity/noBannedTypes)


[error] 35-35: Don't use 'Function' as a type.

Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.

(lint/complexity/noBannedTypes)

Additional comments not posted (43)
spiffworkflow-backend/src/spiffworkflow_backend/models/process_instance_migration_detail.py (3)

1-5: Imports look good.

The imports for dataclass, TypedDict, ForeignKey, and relationship are necessary and correct for this file.


11-15: TypedDict definition looks good.

The ProcessInstanceMigrationDetailDict correctly defines the expected types for each field.


18-29: Dataclass definition looks good.

The ProcessInstanceMigrationDetailModel correctly defines the table name, columns, and relationships for the model.

spiffworkflow-backend/src/spiffworkflow_backend/models/process_instance_event.py (2)

3-5: Imports look good.

The imports for dataclass, ForeignKey, relationship, and validates are necessary and correct for this file.


Line range hint 33-50:
Dataclass definition looks good.

The ProcessInstanceEventModel correctly defines the table name, columns, and relationships for the model, including the new relationship for migration details.

spiffworkflow-frontend/src/a-spiffui-v2/views/Dashboards/myProcesses/CellRenderer.tsx (3)

11-15: Addition of the title prop looks good.

The title prop is optional and used correctly in the CellRenderer component.


63-63: Usage of the title prop looks good.

The title prop is used correctly in the Typography component to display additional information.


69-81: Usage of the title prop looks good.

The title prop is used correctly in the Typography component to display additional information.

spiffworkflow-frontend/src/routes/ProcessInstanceRoutes.tsx (1)

42-45: Addition of the ProcessInstanceMigratePage route looks good.

The new route correctly adds a path for the ProcessInstanceMigratePage component.

spiffworkflow-backend/tests/spiffworkflow_backend/scripts/test_get_all_permissions.py (1)

40-44: New Permission Added

The new permission for /process-instance-events/hey:group:* has been correctly added to the expected_permissions list.

spiffworkflow-backend/tests/spiffworkflow_backend/integration/test_process_instance_events_controller.py (2)

1-7: New Imports Added

The new imports for ProcessInstanceModel, ProcessInstanceEventType, UserModel, and ProcessInstanceTmpService are necessary for the new test and align with the rest of the file's structure.


11-62: New Test Function Added

The new test function test_process_instance_migration_event_list is correctly implemented and covers the necessary test cases for the migration event list functionality.

However, ensure that the test cases cover all possible scenarios, such as edge cases or error conditions.

spiffworkflow-frontend/src/hooks/UriListForPermissions.tsx (1)

25-25: New URI Added

The new URI processInstanceMigratePath has been correctly added and aligns with the rest of the URIs.

spiffworkflow-backend/src/spiffworkflow_backend/services/process_instance_tmp_service.py (1)

33-33: Include type hint for migration_details.

The migration_details parameter should have a type hint for better readability and maintainability.

-        migration_details: ProcessInstanceMigrationDetailDict | None = None,
+        migration_details: Optional[ProcessInstanceMigrationDetailDict] = None,

Likely invalid or redundant comment.

spiffworkflow-backend/tests/spiffworkflow_backend/integration/test_process_instances_controller.py (1)

164-167: Duplicate comment: Add assertions for new fields in migration check response.

Ensure that the new fields (current_bpmn_process_hash and current_git_revision) are correctly asserted.

spiffworkflow-frontend/src/interfaces.ts (1)

552-560: New Interface Addition: MigrationEvent

The MigrationEvent interface is well-defined and appropriately typed. It captures all necessary details related to process instance migrations.

spiffworkflow-backend/tests/spiffworkflow_backend/unit/test_process_instance_service.py (5)

Line range hint 164-245:
New Test Method: test_it_can_migrate_a_process_instance

The test method test_it_can_migrate_a_process_instance is comprehensive, covering the creation, migration, and validation of a process instance. It correctly verifies the initial and target BPMN process hashes and the migration details.


246-315: New Test Method: test_it_can_migrate_a_process_instance_and_revert

The test method test_it_can_migrate_a_process_instance_and_revert is comprehensive, covering the creation, migration, reversion, and validation of a process instance. It correctly verifies the state of tasks before and after reversion.


Line range hint 317-330:
New Test Method: test_it_can_check_if_a_process_instance_can_be_migrated

The test method test_it_can_check_if_a_process_instance_can_be_migrated is well-defined, covering the ability to check if a process instance can be migrated. It correctly sets up the test conditions and verifies the expected behavior.


Line range hint 332-362:
New Test Method: test_it_raises_if_a_process_instance_cannot_be_migrated_to_new_process_model_version

The test method test_it_raises_if_a_process_instance_cannot_be_migrated_to_new_process_model_version is well-defined, covering the scenario where a process instance cannot be migrated to a new process model version. It correctly sets up the test conditions and expects the appropriate error to be raised.


Line range hint 54-90:
Test Method: test_save_file_data_v0

The test method test_save_file_data_v0 is comprehensive, covering various scenarios for saving file data with digest references. It correctly sets up the test conditions and verifies the expected behavior.

spiffworkflow-backend/tests/spiffworkflow_backend/helpers/base_test.py (1)

Line range hint 342-361:
Updated Method: create_process_instance_from_process_model

The method create_process_instance_from_process_model now includes an additional parameter bpmn_version_control_identifier. The change is well-integrated and enhances the method's functionality.

spiffworkflow-backend/src/spiffworkflow_backend/routes/process_instances_controller.py (1)

556-561: Ensure the dictionary keys are consistent and meaningful.

The keys used in the dictionary should be meaningful and consistent with the rest of the codebase. Ensure that these keys are documented if they are used elsewhere.

spiffworkflow-backend/tests/spiffworkflow_backend/unit/test_authorization_service.py (5)

131-134: LGTM!

The test method test_explode_permissions_all_on_process_group correctly includes the new permission "/process-instance-events/some-process-group:some-process-model:*", "read".


172-174: LGTM!

The test method test_explode_permissions_start_on_process_group correctly includes the new permission "/process-instance-events/some-process-group:some-process-model:*", "read".


221-224: LGTM!

The test method test_explode_permissions_all_on_process_model correctly includes the new permission "/process-instance-events/some-process-group:some-process-model/*", "read".


262-264: LGTM!

The test method test_explode_permissions_start_on_process_model correctly includes the new permission "/process-instance-events/some-process-group:some-process-model/*", "read".


Line range hint 545-549:
LGTM!

The _expected_support_permissions method correctly includes the new permissions related to process instance events and migrations.

spiffworkflow-backend/src/spiffworkflow_backend/services/process_instance_service.py (1)

Line range hint 212-280:
LGTM! But verify the function usage in the codebase.

The code changes in migrate_process_instance are approved.

However, ensure that all function calls to migrate_process_instance match the new signature.

spiffworkflow-backend/src/spiffworkflow_backend/services/authorization_service.py (3)

73-73: Added permissions for new paths.

The new paths /process-instance-events and /process-instance-migrate have been added with read and create permissions respectively. Ensure these paths are correct and intended for the new functionality.


563-564: Added read permissions for new paths.

The paths /process-instance-events and /event-error-details have been included in the permissions assignment. Ensure these paths are correct and intended for the new functionality.


667-667: Added read permissions for process-instance events.

The path /process-instance-events/* has been included in the permissions assignment. Ensure this path is correct and intended for the new functionality.

spiffworkflow-frontend/src/routes/ProcessInstanceShow.tsx (5)

26-26: New import for Migrate icon.

The import for the Migrate icon from @carbon/icons-react is correctly added.


155-155: New permission added for process instance migration.

The processInstanceMigratePath permission is correctly added to the permissionRequestData.


596-600: New function to navigate to process instance migration page.

The navigateToProcessInstanceMigratePage function is correctly implemented for navigation purposes.


644-658: New migration button function.

The migrateButton function is correctly implemented to render the migration button when the process instance is suspended.


1631-1633: Integration of migration button in button icons.

The migration button is correctly integrated into the buttonIcons function, ensuring it is rendered based on permissions.

spiffworkflow-backend/src/spiffworkflow_backend/services/process_instance_processor.py (4)

794-798: Ensure new parameters are utilized correctly.

The new parameters spiff_serializer_version, bpmn_process_definition, bpmn_process, and bpmn_process_definition_id are added. Ensure these parameters are used correctly and do not introduce any side effects.


Line range hint 802-817:
Verify the new logic handling additional parameters.

The new logic handles the additional parameters. Ensure the existing functionality is not broken and the new parameters are utilized correctly.


910-918: Ensure new parameters are utilized correctly.

The new parameters spiff_serializer_version, bpmn_process_definition, bpmn_process, and bpmn_process_definition_id are added. Ensure these parameters are used correctly and do not introduce any side effects.


910-918: Ensure new parameters are utilized correctly.

The new parameters spiff_serializer_version, bpmn_process_definition, bpmn_process, and bpmn_process_definition_id are added. Ensure these parameters are used correctly and do not introduce any side effects.

spiffworkflow-backend/src/spiffworkflow_backend/api.yml (2)

1512-1517: Addition of query parameter target_bpmn_process_hash looks good.

The parameter is correctly placed, described, and its schema is defined properly.


2938-2963: Addition of the new endpoint path for process instance migration events looks good.

The endpoint is correctly defined with appropriate parameters, tags, operationId, summary, and responses.

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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

Outside diff range, codebase verification and nitpick comments (1)
spiffworkflow-frontend/src/a-spiffui-v2/views/Dashboards/myProcesses/MyProcesses.tsx (1)

94-94: Remove Unnecessary console.log Statement

The console.log statement in the renderCell function is unnecessary and can affect performance.

-            console.log(params);
Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 1aa3861 and 7d34fd5.

Files selected for processing (5)
  • spiffworkflow-backend/keycloak/bin/start_keycloak (4 hunks)
  • spiffworkflow-backend/src/spiffworkflow_backend/config/default.py (1 hunks)
  • spiffworkflow-backend/src/spiffworkflow_backend/services/process_instance_tmp_service.py (4 hunks)
  • spiffworkflow-frontend/src/a-spiffui-v2/views/Dashboards/myProcesses/MyProcesses.tsx (1 hunks)
  • spiffworkflow-frontend/src/routes/ProcessInstanceMigratePage.tsx (1 hunks)
Files skipped from review due to trivial changes (1)
  • spiffworkflow-backend/src/spiffworkflow_backend/config/default.py
Files skipped from review as they are similar to previous changes (1)
  • spiffworkflow-frontend/src/routes/ProcessInstanceMigratePage.tsx
Additional context used
Learnings (1)
spiffworkflow-frontend/src/a-spiffui-v2/views/Dashboards/myProcesses/MyProcesses.tsx (1)
Learnt from: jasquat
PR: sartography/spiff-arena#1902
File: spiffworkflow-frontend/src/a-spiffui-v2/views/Dashboards/myProcesses/MyProcesses.tsx:94-94
Timestamp: 2024-07-11T14:25:22.833Z
Learning: Unnecessary `console.log` statements should be removed from production code to avoid performance impacts.
Ruff
spiffworkflow-backend/src/spiffworkflow_backend/services/process_instance_tmp_service.py

51-51: Line too long (135 > 130)

(E501)

Additional comments not posted (7)
spiffworkflow-backend/src/spiffworkflow_backend/services/process_instance_tmp_service.py (3)

86-89: Check for add_to_db_session before adding migration details.

Ensure add_to_db_session is checked before calling add_process_instance_migration_detail to maintain consistency.


93-103: Ensure migration details dictionary contains required keys.

Before accessing dictionary keys, ensure they exist to avoid KeyError.


33-33: Ensure migration details are correctly handled.

Ensure that migration details are correctly handled when adding them to the process instance event.

spiffworkflow-backend/keycloak/bin/start_keycloak (4)

11-11: Properly handle script errors.

Ensure that the error handling in the script correctly captures and reports errors.


20-20: Set default realm name.

Ensure that the default realm name is set correctly if not provided.


23-23: Check for existing Docker network.

Ensure that the script checks for the existence of the Docker network before creating it.


56-59: Set script directory.

Ensure that the script correctly sets the directory for the script.

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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

Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 7d34fd5 and 86af91a.

Files selected for processing (1)
  • spiffworkflow-backend/src/spiffworkflow_backend/services/process_instance_tmp_service.py (4 hunks)
Files skipped from review as they are similar to previous changes (1)
  • spiffworkflow-backend/src/spiffworkflow_backend/services/process_instance_tmp_service.py

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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

Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 86af91a and 70f791d.

Files ignored due to path filters (1)
  • spiffworkflow-backend/poetry.lock is excluded by !**/*.lock
Files selected for processing (5)
  • spiffworkflow-backend/bin/local_development_environment_setup (1 hunks)
  • spiffworkflow-backend/src/spiffworkflow_backend/config/permissions/acceptance_tests.yml (1 hunks)
  • spiffworkflow-backend/src/spiffworkflow_backend/routes/process_instance_events_controller.py (3 hunks)
  • spiffworkflow-backend/src/spiffworkflow_backend/services/process_instance_processor.py (3 hunks)
  • spiffworkflow-backend/tests/spiffworkflow_backend/integration/test_process_instances_controller.py (1 hunks)
Files skipped from review due to trivial changes (1)
  • spiffworkflow-backend/src/spiffworkflow_backend/config/permissions/acceptance_tests.yml
Files skipped from review as they are similar to previous changes (3)
  • spiffworkflow-backend/src/spiffworkflow_backend/routes/process_instance_events_controller.py
  • spiffworkflow-backend/src/spiffworkflow_backend/services/process_instance_processor.py
  • spiffworkflow-backend/tests/spiffworkflow_backend/integration/test_process_instances_controller.py
Additional comments not posted (3)
spiffworkflow-backend/bin/local_development_environment_setup (3)

20-20: LGTM!

The comment is clear and indicates the purpose of the following code block.


25-27: LGTM! But verify the logic.

The code changes are approved.

However, ensure the logic for handling the acceptance argument is correct and doesn't interfere with other conditions.

Verification successful

Let's refine our search by focusing on .sh files to verify the logic for handling the acceptance argument.


Let's broaden the search to include all file types to ensure we capture any usage of the acceptance argument across the entire codebase.


Logic for handling the acceptance argument is verified.

The code correctly sets the acceptance_test_mode flag when the acceptance argument is passed and uses it appropriately to set the SPIFFWORKFLOW_BACKEND_PERMISSIONS_FILE_NAME environment variable.

  • spiffworkflow-backend/bin/local_development_environment_setup:26: acceptance_test_mode="true"
  • spiffworkflow-backend/bin/local_development_environment_setup:29: if [[ "$acceptance_test_mode" == "true" ]]; then
  • spiffworkflow-backend/bin/local_development_environment_setup:30: export SPIFFWORKFLOW_BACKEND_PERMISSIONS_FILE_NAME=acceptance_tests.yml
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the logic for handling the `acceptance` argument.

# Test: Search for the usage of the script. Expect: Correct handling of the `acceptance` argument.
rg --type shell -A 3 $'acceptance'

Length of output: 63


Script:

#!/bin/bash
# Description: Verify the logic for handling the `acceptance` argument.

# Find all shell scripts and search for the usage of 'acceptance'
fd -e sh -x rg 'acceptance' {}

Length of output: 32


Script:

#!/bin/bash
# Description: Verify the logic for handling the `acceptance` argument.

# Search for the usage of 'acceptance' across all files
rg 'acceptance'

Length of output: 4331


21-23: LGTM! But verify the logic.

The code changes are approved.

However, ensure the logic for handling the keycloak argument is correct and doesn't interfere with other conditions.

Verification successful

Verification Successful: Logic for handling the keycloak argument is correct.

The script correctly sets use_local_open_id to false when the keycloak argument is provided, and this logic does not interfere with other conditions. The script processes subsequent arguments correctly due to the use of shift.

  • spiffworkflow-backend/bin/local_development_environment_setup
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the logic for handling the `keycloak` argument.

# Test: Search for the usage of the script. Expect: Correct handling of the `keycloak` argument.
rg --type shell -A 3 $'keycloak'

Length of output: 61


Script:

#!/bin/bash
# Description: Verify the logic for handling the `keycloak` argument.

# Test: Search for the usage of the script. Expect: Correct handling of the `keycloak` argument.
rg --glob '*.sh' -A 3 'keycloak'

Length of output: 32


Script:

#!/bin/bash
# Description: Verify the logic for handling the `keycloak` argument.

# Test: Search for the usage of the script. Expect: Correct handling of the `keycloak` argument.
rg 'keycloak' -A 3

Length of output: 37410


Script:

#!/bin/bash
# Description: Extract the content of the local_development_environment_setup script for detailed review.

# Display the content of the script to understand the broader context and interactions.
cat spiffworkflow-backend/bin/local_development_environment_setup

Length of output: 3790

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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

Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 70f791d and 413d02e.

Files selected for processing (1)
  • spiffworkflow-backend/docker-compose.yml (1 hunks)
Files skipped from review due to trivial changes (1)
  • spiffworkflow-backend/docker-compose.yml

@jasquat jasquat merged commit 2e1ee93 into main Jul 11, 2024
23 checks passed
@jasquat jasquat deleted the pi-migration-2 branch July 11, 2024 17:46
@coderabbitai coderabbitai bot mentioned this pull request Nov 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant