-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Enhanced Workflow Instance Filters #4957
Conversation
Implemented a new feature in the workflow instances' querying API, enabling filtering based on timestamps. The changes include adding a new TimestampFilter class and modifying existing classes accordingly. This new filter supports various operators such as greater than and less than, providing flexibility in filtering workflow instances.
…nceList Timestamp filtering within WorkflowInstanceList is refactored using System.Linq.Dynamic.Core for more compact expression. This change results in simpler and more efficient code, reducing repetitive code lines. The UI updates include changing the MudGrid structure to table for timestamp filters display, offering a more organized view. Also, protection against null or minimum date values during parsing and comparisons is added, increasing stability and safety of operations.
The previous Get and Post method calls for handling '/workflow-instances' have been replaced with a Verbs method for HTTP verbs and a Routes method for setting the route.
The HasIncidents property in WorkflowInstanceFilter has been updated from a bool to a nullable bool so that it can represent three states (true, false, or null) instead of only two. This allows not only to filter workflow instances that have incidents, but also those that don't have any, enhancing the flexibility of the filter in the process.
Updated search term comparison in WorkflowInstanceFilter to use "Contains" instead of "Equals" to improve search flexibility. Also, enhanced the user interface in WorkflowInstanceList by adding debounce interval for the search input for performance reasons and by adding a Close button for convenience.
...clients/Elsa.Api.Client/Resources/WorkflowInstances/Requests/ListWorkflowInstancesRequest.cs
Show resolved
Hide resolved
src/modules/Elsa.Workflows.Api/Endpoints/WorkflowInstances/List/Endpoint.cs
Show resolved
Hide resolved
src/modules/Elsa.Workflows.Management/Models/TimestampFilter.cs
Outdated
Show resolved
Hide resolved
src/modules/Elsa.Workflows.Management/Models/TimestampFilter.cs
Outdated
Show resolved
Hide resolved
src/modules/Elsa.Workflows.Management/Models/TimestampFilter.cs
Outdated
Show resolved
Hide resolved
The code modifies the API endpoint for WorkflowInstances. It now checks that the input provided in the request is valid before proceeding. Furthermore, a new file called 'RequiredMembers.cs' has been added, which introduces two new attributes 'RequiredMemberAttribute' and 'CompilerFeatureRequiredAttribute'.
Co-authored-by: Béchir BEN AMEUR <32399944+bbenameur@users.noreply.github.com>
Co-authored-by: Béchir BEN AMEUR <32399944+bbenameur@users.noreply.github.com>
Co-authored-by: Béchir BEN AMEUR <32399944+bbenameur@users.noreply.github.com>
@bbenameur Thanks for the code review, I appreciate it! |
await SendOkAsync(response, cancellationToken); | ||
} | ||
|
||
private async Task<bool> ValidateInputAsync(Request request, CancellationToken cancellationToken) |
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.
Is there a specific reason you are not using the validation engine of fastendpoints?
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.
I wasn't sure how to do it for the TimestampFilters collection, but maybe I missed something obvious. If you have a suggestion then it'd be great to move it into its own DTO validator 👍🏻
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.
You can use custom functions for validation as well. I plan to have a PR up later today in which I use this
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.
I see - I will give that a try, thank you 👍🏻
|| instance.Id.Equals(searchTerm) | ||
|| instance.DefinitionId.Equals(searchTerm) | ||
|| instance.CorrelationId!.Equals(searchTerm) | ||
|| instance.DefinitionVersionId.Contains(searchTerm) |
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.
Should we only be able to search on an (part of) id or do we also want to be able to search on the name of the workflow definition?
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.
It would be nice to be able to also search on a part of the workflow definition name, but I wasn't sure if it is worth the added complexity to achieve that. I think that it would require to do a preliminary search on the workflow definitions, then selecting the matching definition IDs, and adding those to the workflow instance filter. If you think it is worthwhile, we can do it. It would certainly offer a very nice experience to the user.
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.
A version where we can reduce the complexity is where we can use the search in the workflow definition selection box and use that to select the definitions we are interested in.
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.
Aha - so basically grab a set of matching workflow definition IDs when the user enters a search term in the text field, and including that as part of the request. That's pretty smart! 😎 I will send another PR with that improvement 👍🏻
This PR enhances the ability to filter workflow instance search results. I includes the following filters:
The following filters already existed: