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

[18.0][MIG] queue_job: Migrate to 18.0 #692

Merged
merged 1,204 commits into from
Oct 15, 2024
Merged

Conversation

thienvh332
Copy link
Contributor

@thienvh332 thienvh332 commented Oct 7, 2024

Ported PRs

Changes

  • Cleaned TODO tags

  • Refactor the JobSerialized field on top of odoo.fields.Json

    • Result
>>> self.env.cr.execute("SELECT column_name, data_type FROM information_schema.columns WHERE table_name = 'queue_job' AND column_name = 'kwargs';")
>>> self.env.cr.fetchall()
[('kwargs', 'jsonb')]

  • Example for searching:
>>> self.env.cr.execute("SELECT kwargs->>'a' FROM queue_job WHERE kwargs ? 'a'")
>>> self.env.cr.dictfetchall()
[{'?column?': 'abc'}, {'?column?': 'abc'}]

simahawk and others added 30 commits October 7, 2024 16:52
The float_time widget shows hours seconds, we only want seconds.
The widget had been removed on the form view, but not on the tree view.
Use case:

* you have a root channel per scope/app (eg: root.edi)
* you have several sub channels (eg: root.edi.ubl.sales,
root.edi.gs1.delivery)
* you want to configure capacity only for the main channel "root.edi"

Before this change, the channel manager falls back on root channel.
However, if you have a specific parent channel configured
it sounds a good idea to use it.
Contributing since a while... :)
Updated by "Update PO files to match POT (msgmerge)" hook in Weblate.

Translation: queue-15.0/queue-15.0-queue_job
Translate-URL: https://translation.odoo-community.org/projects/queue-15-0/queue-15-0-queue_job/
test_queue_job will create channels root.sub and root.sub.sub, so the
tests doing the same thing will fail with a unique constraint error.
When displayed in a tab, the widget show the nodes offset, the d3
network is probably confused by the size or visibility of its canvas.
Install a listener on tabs to trigger a fit() on the network.
A graph of jobs always share the same graph_uuid, which can be
used to group jobs, but is also a faster way to find all the jobs
of the graph. Then we can use the dependencies field to build the
whole graph from the pre-selection of jobs.
OCA-git-bot and others added 8 commits October 11, 2024 14:59
Currently translated at 100.0% (12 of 12 strings)

Translation: queue-17.0/queue-17.0-test_queue_job
Translate-URL: https://translation.odoo-community.org/projects/queue-17-0/queue-17-0-test_queue_job/zh_CN/
Use the current company to trigger the job (+ add related tests)

[14.0][FIX] queu_job: allowed_company_ids => use with_company(...)
Fill allowed_company_ids from context with the job's company instead of every allowed companies of the user.
Because most of the time, a job is related to only one company. And adding every allowed companies of the user into the context may load some unexpected records (during search for example). Because standards ir.rule use ['|',('company_id','=',False),('company_id', 'in', company_ids)] and this 'company_ids' is filled with every allowed companies from the context.
OCA#348 added support for randomized retry intervals.
Configuration of randomized retry intervals is not possible due to the formatting checks not being updated.
This should fix it.
- an explicit flush is needed or child jobs won't be updated
- no need to forward port, this was fixed already in 16.0+
As the 'queue_job__no_delay' context key is the valid one to use,
don't raise warnings during tests, but log it as information level.
@sbidoul
Copy link
Member

sbidoul commented Oct 11, 2024

@thienvh332 please don't change the behaviour based on test_enabled. Better use self.assertLogs to trap the log and test that it actually logged what was expected. See for example: OCA/server-tools@7d2e3d8

@thienvh332
Copy link
Contributor Author

@thienvh332 please don't change the behaviour based on test_enabled. Better use self.assertLogs to trap the log and test that it actually logged what was expected. See for example: OCA/server-tools@7d2e3d8

Hi @sbidoul
I think you are right, I tried the update but it didn't work for me. Could you help me see what I am missing?

@thienvh332
Copy link
Contributor Author

As commented in unittest, I am not able to control the printing of WARNING. So I will revert to the original handling. If you guys have any suggestions to handle it I will fix it.

@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

@sbidoul
Copy link
Member

sbidoul commented Oct 14, 2024

The problem is we don't see what you tried anymore, so it's difficult to help.

@thienvh332
Copy link
Contributor Author

Hi @sbidoul , indeed I should have pushed a fixup commit instead of force-pushing

  • So here is what happened:

    • the .assertLogs was trapping the log as expected when calling node.__del__ explicitely
    • but python was still garbage collecting the node after the test, so an additional warning was emitted
  • To fix it, I now explicitly run the garbage collection with gc

Thanks for your help

@sbidoul
Copy link
Member

sbidoul commented Oct 15, 2024

@thienvh332 looks good, thanks!

@simahawk
Copy link
Contributor

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 18.0-ocabot-merge-pr-692-by-simahawk-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 8281c2b into OCA:18.0 Oct 15, 2024
7 checks passed
@OCA-git-bot
Copy link
Contributor

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

Copy link
Member

@mmequignon mmequignon left a comment

Choose a reason for hiding this comment

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

A few remarks, LG overall

@@ -279,20 +282,23 @@ def test_delayable_group_of_chain(self):
)

def test_log_not_delayed(self):
logger_name = "odoo.addons.queue_job"
with self.assertLogs(logger_name, level="WARN") as test:
with self.assertLogs(level=logging.WARNING) as test:
Copy link
Member

Choose a reason for hiding this comment

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

Any WARNING log would be catched here, we should keep the logger filter.

Comment on lines 36 to 40
result = (
self.env["test.queue.job"]
.with_context(_job_force_sync=True)
.delay_me(1, kwarg=2)
)
Copy link
Member

Choose a reason for hiding this comment

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

why aren't we keeping this _job_force_sync context key in the test_auto_delay_force_sync test method ?

Copy link
Contributor Author

@thienvh332 thienvh332 Oct 16, 2024

Choose a reason for hiding this comment

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

Hi @mmequignon ,

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.