Feature/homepage extension filter refactor #919
Conversation
WalkthroughWalkthroughThe recent updates to the SpiffWorkflow backend and frontend aim to enhance functionality, improve code quality, and refine user and extension handling. Key changes include the introduction of type hints for better code clarity, the restructuring of components for improved modularity, and significant updates in navigation and UI components, aiming for a more dynamic and user-friendly interface. Additionally, new scripts for user and group management have been introduced, and existing components and routes have been updated to accommodate these changes. 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: 7
Configuration used: .coderabbit.yaml
Files selected for processing (26)
- spiffworkflow-backend/src/spiffworkflow_backend/models/group.py (2 hunks)
- spiffworkflow-backend/src/spiffworkflow_backend/routes/extensions_controller.py (2 hunks)
- spiffworkflow-backend/src/spiffworkflow_backend/routes/tasks_controller.py (1 hunks)
- spiffworkflow-backend/src/spiffworkflow_backend/scripts/get_all_permissions.py (1 hunks)
- spiffworkflow-backend/src/spiffworkflow_backend/scripts/get_current_user.py (2 hunks)
- spiffworkflow-backend/src/spiffworkflow_backend/scripts/get_groups_for_user.py (1 hunks)
- spiffworkflow-backend/src/spiffworkflow_backend/services/authorization_service.py (1 hunks)
- spiffworkflow-backend/tests/spiffworkflow_backend/scripts/test_get_groups_for_user.py (1 hunks)
- spiffworkflow-frontend/src/components/ExtensionUxElementForDisplay.tsx (1 hunks)
- spiffworkflow-frontend/src/components/NavigationBar.tsx (2 hunks)
- spiffworkflow-frontend/src/components/ProcessInstanceListSaveAsReport.tsx (2 hunks)
- spiffworkflow-frontend/src/components/ProcessInstanceListTable.tsx (7 hunks)
- spiffworkflow-frontend/src/components/ProcessInstanceListTableWithFilters.tsx (1 hunks)
- spiffworkflow-frontend/src/components/SpiffTabs.tsx (1 hunks)
- spiffworkflow-frontend/src/components/TaskRouteTabs.tsx (1 hunks)
- spiffworkflow-frontend/src/extension_ui_schema_interfaces.ts (2 hunks)
- spiffworkflow-frontend/src/index.css (1 hunks)
- spiffworkflow-frontend/src/interfaces.ts (1 hunks)
- spiffworkflow-frontend/src/routes/BaseRoutes.tsx (2 hunks)
- spiffworkflow-frontend/src/routes/CompletedInstances.tsx (1 hunks)
- spiffworkflow-frontend/src/routes/Extension.tsx (10 hunks)
- spiffworkflow-frontend/src/routes/HomeRoutes.tsx (1 hunks)
- spiffworkflow-frontend/src/routes/InProgressInstances.tsx (1 hunks)
- spiffworkflow-frontend/src/routes/ProcessInstanceList.tsx (2 hunks)
- spiffworkflow-frontend/src/routes/ProcessModelShow.tsx (2 hunks)
- spiffworkflow-frontend/src/routes/RootRoute.tsx (1 hunks)
Files not summarized due to errors (2)
- spiffworkflow-frontend/src/components/ProcessInstanceListTable.tsx: Error: Message exceeds token limit
- spiffworkflow-frontend/src/components/ProcessInstanceListTableWithFilters.tsx: Error: Message exceeds token limit
Files skipped from review due to trivial changes (4)
- spiffworkflow-backend/src/spiffworkflow_backend/scripts/get_all_permissions.py
- spiffworkflow-backend/src/spiffworkflow_backend/services/authorization_service.py
- spiffworkflow-frontend/src/index.css
- spiffworkflow-frontend/src/routes/RootRoute.tsx
Additional comments: 37
spiffworkflow-frontend/src/components/TaskRouteTabs.tsx (2)
- 1-3: Imports have been updated to remove unused hooks and add
SpiffTabinterface andSpiffTabscomponent.- 11-16: The component now uses
SpiffTabsfor rendering tabs, which simplifies the component by removing direct navigation logic and state management for selected tab index.spiffworkflow-backend/src/spiffworkflow_backend/scripts/get_current_user.py (1)
- 21-21: The
runmethod now directly returns the user object as a dictionary usingg.user.as_dict(), which simplifies the code by removing unnecessary serialization steps.spiffworkflow-frontend/src/routes/HomeRoutes.tsx (1)
- 19-19: The route path has been updated from "grouped" to "in-progress" to better reflect the purpose of the route. This change aligns the route path with the component it renders, improving readability and maintainability.
spiffworkflow-frontend/src/components/ExtensionUxElementForDisplay.tsx (1)
- 10-37: Refactoring the component logic into
ExtensionUxElementMapand introducingelementCallbackIfNotFoundparameter improves modularity and allows for conditional rendering based on the presence ofelementCallbackIfNotFound. This enhances the component's flexibility in handling UX elements.spiffworkflow-frontend/src/components/SpiffTabs.tsx (1)
- 10-39: The
SpiffTabscomponent correctly implements dynamic tab selection based on the current URL path, usinguseLocationto set the active tab. This approach enhances user experience by reflecting the current navigation state in the tab selection.spiffworkflow-backend/src/spiffworkflow_backend/models/group.py (2)
- 25-25: The use of the
@dataclassdecorator simplifies theGroupModelclass definition by automatically generating boilerplate code like__init__, which enhances code readability and maintainability.- 31-32: Adding type hints to the
nameandidentifierfields improves code clarity and type safety, making the codebase more maintainable and easier to understand.spiffworkflow-frontend/src/routes/ProcessInstanceList.tsx (1)
- 6-6: The change to use
ProcessInstanceListTableWithFiltersinstead ofProcessInstanceListTableindicates an enhancement in functionality, allowing for filtering capabilities directly within the process instance list table. This should improve user interaction and data presentation.spiffworkflow-frontend/src/extension_ui_schema_interfaces.ts (2)
- 32-32: The addition of
ui_schema_page_components_variableto theUiSchemaActioninterface allows for more dynamic and flexible UI customization based on the action's outcome, enhancing the application's extensibility.- 80-80: Replacing
redirect_towithui_schema_page_componentsin theExtensionApiResponseinterface aligns with the enhanced customization capabilities, allowing for a more dynamic response structure that can include multiple UI components.spiffworkflow-frontend/src/components/NavigationBar.tsx (2)
- 71-71: The logic for determining the active navigation item has been expanded to include additional path patterns, enhancing the navigation bar's responsiveness to user navigation and improving the user experience.
- 240-240: The "Processes" link now dynamically uses the
processGroupPathvariable for itshrefattribute, which improves maintainability by centralizing the path definition.spiffworkflow-frontend/src/interfaces.ts (2)
- 507-510: The
SpiffTabinterface is well-defined with appropriate properties for tab functionality.- 512-515: The
SpiffTableHeaderinterface correctly defines properties for table headers, including tooltip support.spiffworkflow-frontend/src/routes/Extension.tsx (6)
- 25-44: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [17-30]
Imports for
ExtensionApiResponse,SpiffTabs,ProcessInstanceListTable, andCreateNewInstanceare correctly added to support the new functionality.
- 33-40: The addition of the
pageIdentifierprop to theExtensioncomponent is correctly implemented, allowing for dynamic handling of extension pages.- 114-129: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [100-126]
The
processLoadResultfunction has been updated to handleExtensionApiResponseand dynamically set UI schema page components based on the response. This change supports the dynamic rendering of components based on extension responses.
- 140-156: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [143-180]
The logic to determine
pageIdentifierToUseand to set UI schema page components and definitions based on the extension UI schema file is correctly implemented. This enhances the dynamic rendering capabilities of the extension component.
- 218-218: The
processSubmitResultfunction correctly handles navigation and task data updates based on the extension API response, supporting dynamic UI updates after form submissions.- 332-342: The dynamic component rendering logic based on
uiSchemaPageComponentsis correctly implemented, allowing for the flexible inclusion of various components within the extension page.spiffworkflow-frontend/src/components/ProcessInstanceListTable.tsx (7)
- 43-57: The addition of the
headerprop of typeSpiffTableHeadertoProcessInstanceListTableis correctly implemented, allowing for customizable table headers with tooltips.- 85-127: The logic to set process instances and report metadata from the API result is correctly implemented, ensuring that the table data and metadata are updated based on the API response.
- 130-181: The
getProcessInstancesfunction correctly fetches process instances based on the provided report metadata or additional filters, supporting dynamic table data fetching.- 184-221: The useEffect hook correctly handles the fetching of process instances and report metadata based on
reportIdentifierorreportMetadata, and implements auto-reloading functionality.- 342-409: The
tableTitleLinefunction correctly handles the rendering of the table title and the filter button link based on theshowLinkToReportandheaderprops, supporting customizable table headers and navigation to filtered lists.- 520-532: Error handling for conflicting props (
additionalReportFiltersandreportMetadata,reportIdentifierandreportMetadata) is correctly implemented, providing clear guidance on prop usage.- 544-586: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [534-584]
The rendering logic for the table, including pagination and handling of empty states, is correctly implemented. This ensures that the table is rendered appropriately based on the available data and pagination settings.
spiffworkflow-frontend/src/routes/ProcessModelShow.tsx (3)
- 690-691: The addition of
showLinkToReportandvariant="for-me"props to theProcessInstanceListTablecomponent aligns with the PR's objective to enhance homepage customization and immediate filtering on process instances. These changes seem to support a more tailored user experience by providing specific views and direct report access based on the user's actions or roles.- 690-691: The removal of
filtersEnabledandshowReportsprops from theProcessInstanceListTablecomponent, as mentioned in the summary, suggests a simplification or reconfiguration of the component's capabilities. Ensure that the removal of these props does not adversely affect any dependent features or user expectations regarding filtering and report visibility. It might be beneficial to verify that all necessary functionality is either preserved or suitably replaced to maintain or enhance the user experience.Verification successful
The search results indicate that both
filtersEnabledandshowReportsprops are still actively used within theProcessInstanceListTableWithFilters.tsxcomponent and related components. This suggests that the removal of these props from theProcessInstanceListTablecomponent, as mentioned in the summary, does not negatively impact the functionality, given that their usage is confined to a specific component (ProcessInstanceListTableWithFilters.tsx) and its interactions. Therefore, it appears that the removal of these props has been managed in a way that preserves or suitably replaces necessary functionality, likely by consolidating or reorganizing features within the application.* 690-691: The update to the `additionalReportFilters` prop within the `ProcessInstanceListTable` component, which includes a filter for `process_model_identifier`, enhances the specificity and relevance of the displayed process instances. This change is in line with the PR's objective to provide immediate filtering on process instances, improving the user experience by ensuring that the data presented is directly relevant to the user's context or actions.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify removal of filtersEnabled and showReports props does not negatively impact functionality. # Check for any references to these props in the project to ensure there are no unresolved dependencies. rg "filtersEnabled|showReports" --type tsxLength of output: 72
Script:
#!/bin/bash # Adjusting the command to search for references without specifying the file type, as 'tsx' was not recognized. rg "filtersEnabled|showReports"Length of output: 1449
spiffworkflow-backend/src/spiffworkflow_backend/routes/tasks_controller.py (4)
- 1024-1030: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-1]
The file imports several modules and classes. Ensure all imports are used within the file to avoid unnecessary dependencies that could increase memory usage and startup time.
Verification successful
Based on the extensive output from the verification scripts, it's clear that the vast majority of imports in
tasks_controller.pyare indeed utilized within the file. Each import from the list provided has been checked for usage, and the script output confirms their presence in various parts of the code. This includes direct function calls, class instantiations, type annotations, and other forms of references.Given this evidence, it can be concluded that the imports are appropriately used, and there's no indication of unnecessary dependencies that could increase memory usage or startup time without reason.
* 1027-1027: The variable `group_identifier_column` is conditionally defined based on the database type. This approach is clear, but it's important to ensure that the `SPIFFWORKFLOW_BACKEND_DATABASE_TYPE` configuration is always correctly set and validated at the application startup to prevent runtime errors. * 1027-1027: The use of `func.max` in the PostgreSQL condition suggests aggregation without a corresponding `GROUP BY` clause for other columns in the select statement. This could lead to unexpected behavior or errors in PostgreSQL. Verify that this query executes as expected in both MySQL and PostgreSQL environments. * 1027-1027: The label `assigned_user_group_identifier` is reused for both MySQL and PostgreSQL paths, which is good for consistency. However, ensure that the logic encapsulating this variable's usage accounts for the differences in aggregation functions (`func.max` vs. direct assignment) between the two database types.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify if all imported modules are used within the file. rg --type py 'import' spiffworkflow-backend/src/spiffworkflow_backend/routes/tasks_controller.pyLength of output: 4713
Script:
#!/bin/bash # Define a list of imported names based on the output imports=( json os uuid OrderedDict Generator Any TypedDict flask.wrappers sentry_sdk current_app g jsonify make_response stream_with_context Response OperationalError WorkflowTaskException BpmnWorkflow SpiffTask TaskState and_ asc desc func aliased AliasedClass ProcessInstanceMigrator ApiError HumanTaskAlreadyCompletedError HumanTaskNotFoundError UserDoesNotHaveAccessToTaskError SpiffworkflowBaseDBModel db GroupModel HumanTaskModel HumanTaskUserModel JsonDataDict JsonDataModel ProcessInstanceModel ProcessInstanceModelSchema ProcessInstanceStatus ProcessInstanceTaskDataCannotBeUpdatedError ProcessInstanceEventType ProcessModelInfo Task TaskModel TaskDefinitionModel TaskDraftDataDict TaskDraftDataModel TaskInstructionsForEndUserModel UserModel _find_principal_or_raise _find_process_instance_by_id_or_raise _find_process_instance_for_me_or_raise _get_process_model AuthorizationService ErrorHandlingService FileSystemService GitCommandError GitService JinjaService ProcessInstanceProcessor ProcessInstanceIsAlreadyLockedError ProcessInstanceQueueService ProcessInstanceService ProcessInstanceTmpService ProcessModelService SpecFileService TaskModelError TaskService ) # Path to the file file_path="spiffworkflow-backend/src/spiffworkflow_backend/routes/tasks_controller.py" # Iterate over each import and search for its usage within the file for import in "${imports[@]}"; do echo "Searching for usage of $import in $file_path" rg --type py "\b$import\b" "$file_path" doneLength of output: 59641
Verification successful
The review comment highlighted the reuse of the label
assigned_user_group_identifierfor both MySQL and PostgreSQL paths in thetasks_controller.pyfile, emphasizing the need to ensure that the logic accounts for the differences in aggregation functions between the two database types. The script output confirms that the labelassigned_user_group_identifieris indeed used in three different contexts within the file:
- As part of a
func.max()call, which is typically used in SQL queries to get the maximum value of a column.- Directly labeling
GroupModel.identifierwithout usingfunc.max().- Again, in a context where
func.max()is used.This indicates that the code does account for different scenarios where either a direct assignment or an aggregation function (like
func.max) might be necessary, depending on the database operation being performed. The presence of both usages suggests that the implementation is likely tailored to handle the nuances of interacting with different database types, as mentioned in the review comment.Given this information, it appears that the code does indeed consider the differences in database operations, specifically in how
assigned_user_group_identifieris used in contexts that might require aggregation versus direct assignment. Therefore, the review comment's concern seems to have been addressed in the implementation.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for usage of `assigned_user_group_identifier` to ensure it's correctly handled in different DB contexts. rg 'assigned_user_group_identifier' spiffworkflow-backend/src/spiffworkflow_backend/routes/tasks_controller.pyLength of output: 405
spiffworkflow-frontend/src/components/ProcessInstanceListTableWithFilters.tsx (2)
- 115-122: The use of
usePermissionFetcherand the subsequent checkcanSearchUsersare good examples of encapsulating permission logic. However, ensure that the permissions are adequately cached to avoid unnecessary re-fetching on re-renders.Verify that the permissions fetching logic is optimized to prevent unnecessary network requests or computations, especially in a component that seems to be re-rendered frequently.
- 540-546: Using
useDebouncedCallbackfor debouncing the search input is a good practice. However, ensure that the debounce delay (250ms) is optimized for user experience and system performance.Test the user experience with the current debounce delay to ensure it strikes a good balance between responsiveness and unnecessary API calls. Adjust the delay based on user feedback and performance considerations.
spiffworkflow-frontend/src/components/ProcessInstanceListTableWithFilters.tsx
Show resolved
Hide resolved
spiffworkflow-frontend/src/components/ProcessInstanceListTableWithFilters.tsx
Show resolved
Hide resolved
spiffworkflow-frontend/src/components/ProcessInstanceListTableWithFilters.tsx
Show resolved
Hide resolved
spiffworkflow-frontend/src/components/ProcessInstanceListTableWithFilters.tsx
Show resolved
Hide resolved
spiffworkflow-frontend/src/components/ProcessInstanceListTableWithFilters.tsx
Show resolved
Hide resolved
spiffworkflow-frontend/src/components/ProcessInstanceListTableWithFilters.tsx
Show resolved
Hide resolved
spiffworkflow-frontend/src/components/ProcessInstanceListTableWithFilters.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Configuration used: .coderabbit.yaml
Files selected for processing (2)
- spiffworkflow-frontend/cypress/e2e/process_instances.cy.js (2 hunks)
- spiffworkflow-frontend/src/components/ProcessInstanceListTable.tsx (7 hunks)
Files not summarized due to errors (1)
- spiffworkflow-frontend/src/components/ProcessInstanceListTable.tsx: Error: Message exceeds token limit
Additional comments: 3
spiffworkflow-frontend/cypress/e2e/process_instances.cy.js (2)
- 10-19: The addition of setting the time (
HH:mm) for both start and end date filters is a good enhancement for precision in filtering process instances. This change addresses potential issues with tests failing due to slight time differences and makes the filtering more accurate.- 10-19: Removing the click action on the filter button and relying on the form's auto-submit feature (or similar mechanism) could improve the test's reliability by reducing dependencies on UI elements that might change. However, ensure that the application's current behavior supports this change, and there's an implicit or explicit submission of the filter criteria after setting the date and time.
spiffworkflow-frontend/src/components/ProcessInstanceListTable.tsx (1)
- 10-19: The addition of time filtering alongside date filtering in the Cypress test file enhances the precision of filtering actions within tests. This change is beneficial for ensuring that tests remain reliable even when executed around the time boundaries of process instances.
spiffworkflow-frontend/src/components/ProcessInstanceListTable.tsx
Outdated
Show resolved
Hide resolved
spiffworkflow-frontend/src/components/ProcessInstanceListTable.tsx
Outdated
Show resolved
Hide resolved
spiffworkflow-frontend/src/components/ProcessInstanceListTable.tsx
Outdated
Show resolved
Hide resolved
… put together the other elements
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Configuration used: .coderabbit.yaml
Files selected for processing (1)
- spiffworkflow-frontend/src/components/ProcessInstanceListTable.tsx (8 hunks)
Files not summarized due to errors (1)
- spiffworkflow-frontend/src/components/ProcessInstanceListTable.tsx: Error: Message exceeds token limit
Additional comments: 2
spiffworkflow-frontend/src/components/ProcessInstanceListTable.tsx (2)
- 43-57: The props for
ProcessInstanceListTableare clearly defined with optional types and default values where applicable. This is a good practice for component flexibility and readability.- 108-228: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [75-219]
The use of
useState,useEffect, anduseCallbackhooks for managing state and side effects inProcessInstanceListTableis appropriate. However, ensure that the cleanup function in theuseEffecthook that handles auto-reloading is correctly implemented to prevent potential memory leaks or unnecessary network requests after the component unmounts. This is especially important given the dynamic nature of theautoReloadprop.
Implements #743 #537
This adds the ability to customize the homepage using an extension. It has enough features to fully re-implement the default homepage including the tabs and tables as well as being able to implement status' request to remove the "Status" column.
Main changes:
Summary by CodeRabbit
SpiffTabsReact component for managing tabbed interfaces.