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

[FIX] JobsTrap perform_enqueued_jobs not working with jobs creating new jobs (issue OCA#651) #652

Merged
merged 1 commit into from
May 24, 2024

Conversation

fd-oncodna
Copy link
Contributor

JobsTrap perform_enqueued_jobs not working with jobs creating new jobs. After a call to JobsTrap.perform_enqueued_jobs the enqueued_jobs attribute is empty, even if one of the jobs executed had the effect of creating new jobs. That is because after executing the jobs of the queue in a loop, JobsTrap.perform_enqueued_jobs sets the enqueued_jobs to []ignoring the newly created jobs.

@fd-oncodna fd-oncodna changed the title [FIX] fixes https://github.com/OCA/queue/issues/651 [FIX] JobsTrap perform_enqueued_jobs not working with jobs creating new jobs (issue OCA#651) May 19, 2024
@OCA-git-bot
Copy link
Contributor

Hi @guewen,
some modules you are maintaining are being modified, check this out!

Copy link
Member

@guewen guewen left a comment

Choose a reason for hiding this comment

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

Makes sense, thanks

@@ -227,13 +227,13 @@ def perform_enqueued_jobs(self):
def by_graph(job):
return job.graph_uuid or ""

sorted_jobs = sorted(self.enqueued_jobs, key=by_graph)
sorted_jobs = sorted(self.enqueued_jobs[:], key=by_graph)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't believe the extra [:] slicing here is necessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, since sorted returns already a new list.

Copy link
Contributor

@simahawk simahawk left a comment

Choose a reason for hiding this comment

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

Thanks for your contrib!
Could you please rewrite the commit msg to something like

[fix] queue_job: JobsTrap.perform_enqueued_jobs

Closes #651

pre-approving

@@ -227,13 +227,13 @@ def perform_enqueued_jobs(self):
def by_graph(job):
return job.graph_uuid or ""

sorted_jobs = sorted(self.enqueued_jobs, key=by_graph)
sorted_jobs = sorted(self.enqueued_jobs[:], key=by_graph)
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, since sorted returns already a new list.

@fd-oncodna
Copy link
Contributor Author

@simahawk everything's done ;)

@simahawk
Copy link
Contributor

/ocabot merge patch

@OCA-git-bot
Copy link
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 16.0-ocabot-merge-pr-652-by-simahawk-bump-patch, awaiting test results.

@simahawk
Copy link
Contributor

@fd-oncodna tnx, any chance you can backoprt this to 15 and 16?

@OCA-git-bot OCA-git-bot merged commit d42b14d into OCA:16.0 May 24, 2024
7 checks passed
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at 20d3436. Thanks a lot for contributing to OCA. ❤️

@fd-oncodna
Copy link
Contributor Author

@fd-oncodna tnx, any chance you can backoprt this to 15 and 16?

Yes, see #655 and #656

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants