add cancel all button to dashboard#1455
Conversation
for more information, see https://pre-commit.ci
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1455 +/- ##
===========================================
+ Coverage 89.20% 89.24% +0.04%
===========================================
Files 103 104 +1
Lines 3732 3785 +53
Branches 460 465 +5
===========================================
+ Hits 3329 3378 +49
- Misses 326 329 +3
- Partials 77 78 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
for more information, see https://pre-commit.ci
…hub.com/ssl-hep/ServiceX into 1296-add-cancel-all-button-to-dashboard
| if current_app.config.get("ENABLE_AUTH"): | ||
| user = self.get_requesting_user() | ||
| user_id = user.id if user is not None else None | ||
| transform_reqs = TransformRequest.query.filter( |
There was a problem hiding this comment.
I feel Ben always likes these to be added to the model class so they can be reused easily
There was a problem hiding this comment.
Two class methods were added to support this.
| cls.transformer_manager = transformer_manager | ||
|
|
||
| @auth_required | ||
| def get(self): |
There was a problem hiding this comment.
Not to be "that guy", but this should be a POST and not a GET since it is taking an action...
There was a problem hiding this comment.
Yeah you're right, I copied the pattern from the existing cancel route.
There was a problem hiding this comment.
Both Cancel and Cancel All were updated to use post.
There was a problem hiding this comment.
Hm, can cancel be called by the frontend CLI?
There was a problem hiding this comment.
Yes, it can... I'm not sure it's worth making a breaking change over. Maybe keep cancel endpoint a GET and make CancelAll a POST.
Good catch, @ponyisi
There was a problem hiding this comment.
I've reverted the cancel to use get.
Could we support the same logic on both get and post and then deprecate the get on some timeline?
There was a problem hiding this comment.
Sounds reasonable. (Sounds like a feature flag. :-) )
There was a problem hiding this comment.
@ponyisi @BenGalewsky this has been finalized, with an additional frontend PR added to support it.
for more information, see https://pre-commit.ci
…hub.com/ssl-hep/ServiceX into 1296-add-cancel-all-button-to-dashboard
for more information, see https://pre-commit.ci
…hub.com/ssl-hep/ServiceX into 1296-add-cancel-all-button-to-dashboard
for more information, see https://pre-commit.ci
…96-add-cancel-all-button-to-dashboard
…hub.com/ssl-hep/ServiceX into 1296-add-cancel-all-button-to-dashboard
…96-add-cancel-all-button-to-dashboard
for more information, see https://pre-commit.ci
…hub.com/ssl-hep/ServiceX into 1296-add-cancel-all-button-to-dashboard
for more information, see https://pre-commit.ci
…hub.com/ssl-hep/ServiceX into 1296-add-cancel-all-button-to-dashboard
…96-add-cancel-all-button-to-dashboard
| mock_conn.execute.assert_called_once() | ||
|
|
||
|
|
||
| class TestShutdownPod: |
There was a problem hiding this comment.
Why are these tests here, and not in test_transformer_manager.py?
| .all() | ||
| ) | ||
|
|
||
| def shutdown_pod(self, transformer_manager: TransformerManager): |
There was a problem hiding this comment.
This doesn't belong in the database models. I think it should be an operation in transformer_manager.py
There was a problem hiding this comment.
Agree, in my earlier comment I just meant that the DB query (active_user_transformations) should be in the model class
| "capabilities": [ | ||
| "poll_local_transformation_results", | ||
| "long_sample_titles_10240", | ||
| "post_cancel_transform", |
There was a problem hiding this comment.
I find this name confusing... it sounds like it describes something that happens after a cancel (post-cancel-bliss). Maybe cancel_transform_post_method or somthing
Adds Cancel All button to the Dashboard when ENABLE_AUTH=True. This will cancel all active transformations based upon the existing individual Cancel logic.
When ENABLE_AUTH=False, the button is not present and the route returns 400.