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

Clean all for failed job #39

Merged
merged 2 commits into from
Dec 5, 2019
Merged

Clean all for failed job #39

merged 2 commits into from
Dec 5, 2019

Conversation

ex7r3me
Copy link
Contributor

@ex7r3me ex7r3me commented Nov 17, 2019

Now there's a 'clean all' button for failed tasks.
We may want to generalize clean functions by sending params in request body or even change the HTTP method to DELETE with the resource ('delayed', 'failed') in the URL.

I changed clean names to cleanDelayed to prevent confusion, but I know this PR might not be ready to merge. I can change the code to your suggestion.

We can also add a button to clean a single job with maybe with a small trash icon or a more options button, what do you think?

related to #24

- add route for clean all for failed jobs
- add button for clean all in the UI
- rename clean to 'cleanDelayed' for delayed job related functions
@vcapretz
Copy link
Contributor

hey @ex7r3me I like the idea of allowing to clean the failed queue as well, thank you for this PR! 😄

I would just keep one implementation for the routes and use a URL param for getting which queue is it, so in the UI we would call it like

fetch(`${basePath}/queues/${queueName}/clean/failed`, { method: 'put' }).then(...
fetch(`${basePath}/queues/${queueName}/clean/delayed`, { method: 'put' }).then(...
fetch(`${basePath}/queues/${queueName}/clean/whateverHere`, { method: 'put' }).then(...

what do you think?

- change implementation of clean all to use URL param to choose which queue to clean
- change routes in UI and router
@ex7r3me
Copy link
Contributor Author

ex7r3me commented Nov 21, 2019

hey @ex7r3me I like the idea of allowing to clean the failed queue as well, thank you for this PR!

I would just keep one implementation for the routes and use a URL param for getting which queue is it, so in the UI we would call it like

fetch(`${basePath}/queues/${queueName}/clean/failed`, { method: 'put' }).then(...
fetch(`${basePath}/queues/${queueName}/clean/delayed`, { method: 'put' }).then(...
fetch(`${basePath}/queues/${queueName}/clean/whateverHere`, { method: 'put' }).then(...

what do you think?

sure! it's much nicer like this. I've just updated the PR with new routes, do we need any other changes?

@vcapretz
Copy link
Contributor

vcapretz commented Dec 5, 2019

hey @ex7r3me sorry for the very long time to answer it 😅 I totally forgot this PR, my bad

it looks very good now, thank you very much for this PR. I'm merging it and releasing it

@vcapretz vcapretz merged commit 425eec5 into felixmosh:master Dec 5, 2019
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.

2 participants