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

Add 'Clean Jobs' to Delayed Jobs #18

Merged
merged 3 commits into from
Sep 24, 2019

Conversation

alexleroyross
Copy link

@alexleroyross alexleroyross commented Sep 21, 2019

There is now a button in the Delayed tab that cleans jobs.

It's difficult for me to test that this actually works. Here's what I've been doing to test that this works:

  1. Put the same Clean All button into failed jobs
  2. Click it and verify that it moves the failed jobs into Waiting
    This works.

Is there a simple way to create a test job that is Delayed, so I can test this more directly?

I've tried calling moveToDelayed() on newly created jobs, but that seems to move the jobs to Waiting rather than Delayed, so I can't test the Clean All button. Or, perhaps it moves the jobs to the delayed status for a brief second, but they are immediately promoted to the waiting status for processing. Either way, I can't test the Clean All button in the Delayed category. Is there a way to force jobs to stay in the Delayed category?

Despite these difficulties with testing, I modeled this code verbatim after the 'Retry All' option that is available to Failed jobs. The only difference is that the logic calls queue.clean() instead of job.retry(). Because the Clean All button moves jobs to Waiting when I place it in the Failed category, I believe it would work in the Delayed category.

I understand that this PR isn't ready to merge yet. Any pointers would be appreciated. Thank you very much.

closes #3

Alex Ross added 3 commits September 13, 2019 10:56
- Created button for UI
- Added route for logic
- I think Clean All works now
cleanAll,
selectedStatuses,
setSelectedStatuses,
}
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My code formatter changed this return statement from a single line to multiple lines. I can change it back if this is an issue.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it was probably because a new item was added, so it's fine :D

@vcapretz
Copy link
Contributor

hey @alexleroyross, thank's for the PR!

I do understand testing wise we could do much better in the lib, it's something that needs to be done.

What I did to test a delayed queue was to change the .add method under example.js and add a delayed option as so:

example.add({ title: req.query.title }, { delay: 100000 })

this way the jobs are forced to go to the delayed status as soon as you add them, and it works as expected 😄

so I would say we are ready to merge this one, what do you think?

@alexleroyross
Copy link
Author

hey @alexleroyross, thank's for the PR!

I do understand testing wise we could do much better in the lib, it's something that needs to be done.

What I did to test a delayed queue was to change the .add method under example.js and add a delayed option as so:

example.add({ title: req.query.title }, { delay: 100000 })

this way the jobs are forced to go to the delayed status as soon as you add them, and it works as expected 😄

so I would say we are ready to merge this one, what do you think?

Oh, I see! I did not realize that supplying that 'delay' parameter was the same thing as moving a Queue to the 'delayed' status. Thank you!

I tested it on my end, and it worked for me as well. Awesome! As far as I can tell, it looks like it is ready to merge, but it's up to you! Thank you so much for your help. I'll leave it up to you to close this and merge it.

@vcapretz vcapretz merged commit e54ac6f into felixmosh:master Sep 24, 2019
@vcapretz
Copy link
Contributor

Awesome work @alexleroyross! I'll release a new version in npm now with this addition 🥇

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.

Add option to clean jobs
2 participants