-
Notifications
You must be signed in to change notification settings - Fork 42
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
✨ Add cancel option for multiple analyses #2099
✨ Add cancel option for multiple analyses #2099
Conversation
55a4d7a
to
cf9e79e
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2099 +/- ##
==========================================
+ Coverage 39.20% 41.98% +2.78%
==========================================
Files 146 175 +29
Lines 4857 5630 +773
Branches 1164 1341 +177
==========================================
+ Hits 1904 2364 +460
- Misses 2939 3250 +311
- Partials 14 16 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
d8c2bb5
to
09d2827
Compare
Signed-off-by: MiriSafra <[email protected]>
09d2827
to
3302462
Compare
Signed-off-by: MiriSafra <[email protected]>
Signed-off-by: MiriSafra <[email protected]>
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.
The action looks good! The missing part is confirmation dialog - user needs to confirm all destructive actions to prevent accidental loss of data.
Please also attach screenshots presenting the change.
client/src/app/pages/applications/applications-table/applications-table.tsx
Outdated
Show resolved
Hide resolved
client/src/app/pages/applications/applications-table/applications-table.tsx
Show resolved
Hide resolved
client/src/app/pages/applications/applications-table/applications-table.tsx
Show resolved
Hide resolved
Signed-off-by: MiriSafra <[email protected]>
Signed-off-by: shevijacobson <[email protected]>
…ltiple-analyses 🐛 fix confirm dialog
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.
LGTM! Thanks for contributing!
As a follow-up, could you create an issue to implement bulk canceling? Similar as it's done for deleting applications.
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.
There is something off with the isTaskCancellable
test. When I select a single application that has no anlaysis task at all, the cancel action should be disabled. Right now it is not.
client/src/app/pages/applications/applications-table/applications-table.tsx
Show resolved
Hide resolved
Signed-off-by: MiriSafra <[email protected]>
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.
Updated isTaskCancellable
with !!task
check.
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.
LGTM
Added support for cancelling multiple analyses simultaneously. Integrated functionality into the actions menu for bulk applications.
Resolves: https://issues.redhat.com/browse/MTA-3639
New Bulk Cancel Analyses Button
Cancel Analyses Button Disabled When No Applications Are Selected
Cancel Analyses Button Disabled When No Running Analyses Are Selected
Confirm Dialog Triggered on Cancel Analyses Button Click
Action Cancels All Running Analyses for Selected Applications