[IMP] queue: Store context in order to reuse it #121
Conversation
|
IMO it is better to add an explicit argument in the method if you want to keep something. |
|
That is what I have usually done and I always thought that it was a limitation. |
|
I also agree that is good to add corresponding arguments, but there can be context values that are out of your control, like |
|
I find that the context is sometimes/often out of control, it may even contain values which cannot be serialized. If the requirement concerns predefined fields, such as |
|
It is true, that reusing context might be crazy, but it is a tool that is used sometimes and it is interesting to store it somehow. A list of context could be fine. I will work on a safe list of fields. |
|
@guewen The list of safe context keys has been added. It might be better now. |
queue_job/models/base.py
Outdated
| @@ -27,6 +27,20 @@ def _register_hook(self): | |||
| for job_method in job_methods: | |||
| self.env['queue.job.function']._register_job(self, job_method) | |||
|
|
|||
| @api.model | |||
| def get_job_context_keys(self): | |||
There was a problem hiding this comment.
these methods should be private IMO
There was a problem hiding this comment.
Suggestion: setup the context keys to pass on the @job decorator
@job(context_allow=['foo'])
With ['force_company', 'lang', 'tz'] by default and new keys added to the list.
The filtering of the context keys could then be hidden in the job implementation details, instead of cluttering the base model (I try hard to put as less method as possible here).
Would it work for you?
There was a problem hiding this comment.
Or if the safe list really needs to be global, then it can be added to queue.job
There was a problem hiding this comment.
great idea about the job. I will make the change
|
Will this pass some context to the job by default, that was not passed before? |
|
Yes, the context was not passed to the job. |
|
@etobella then I'd say it must not be passed at all unless asked explicitly by the developer, as this could change the behavior of existing deployments. |
|
@sbidoul That is the last change we applied today. The context keys are defined on the job decorator in order to ensure flexibility |
etobella
force-pushed
the
11.0-add-context
branch
from
f6994f6 to
d9b8bfa
Compare
etobella
force-pushed
the
11.0-add-context
branch
2 times, most recently
from
852cbe6 to
d9716c6
Compare
etobella
force-pushed
the
11.0-add-context
branch
from
d9716c6 to
494782e
Compare
|
@simahawk your comments have been attended |
|
With this implementation, how can we add a new key into the |
|
This PR has the |
|
Any chance this PR will be merged soon (and maybe ported to v10)? |
|
There is an issue I'm not sure how to handle, which is that in higher versions of Odoo, the context should be stored in the Serialized records (see #281, #283). This change cannot be ported as-is to Odoo 12+. My feeling is that we should address this problem in more recent versions of Odoo, and see how to transition on migrations. Otherwise we will support something in 11.0 that will be lost when you upgrade... |
|
FWIW in many cases (actually I never needed the context for this reason), it is IMO better to pass an explicit argument to the job method than rely on an arbitrary context |
With these PR, the context is stored and it can be used in the job.
Sometimes, you don't want to lose the context of the user.