[13.0] Add storage of context in jobs recordsets

[guewen]

Alternative to #406.
Adapted from acsone#1 with tests.

This version has a breaking change in the sense that now the basic arguments such as tz, lang, etc in the context are kept, so may bring behavior changes in the existing jobs. In a way, this could be considered a bug so... to discuss (or consider this is the version we want for 16.0, and the list will be empty in earlier versions).

I would not keep all keys by default for several reasons :

• not all values can be serialized to json (random python objects, ...)
• I'm not sure we really do want to keep all the keys (e.g. prefetch_fields)

This implementation may be simple and naive, but I do think it is enough, as people can customize the keys allowed in the context. Also, more smart solutions could easily lead to leaky abstractions, because jobs can have contexts in their self recordset, but also in args and kwargs which potentially would all need different handling (hence a global handling works for each case).

[florian-dacosta]

LGTM thanks!

[francesco-ooops]

@sbidoul@simahawk do you think we can proceed with this?

[sbidoul]

Yes, fine with me.

[simahawk]

/ocabot merge minor

Any plan for v14?

[OCA-git-bot]

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 13.0-ocabot-merge-pr-432-by-simahawk-bump-minor, awaiting test results.

[OCA-git-bot]

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

[AshishHirapara]

/ocabot merge minor

Any plan for v14?

@simahawk I have migrated this PR to 14.0 ==> #433

[francesco-ooops]

Hi @guewen@simahawk@acsonefho , so going back to the original need (from PR #407 ):

Keep context during import.
In some case, the lang set into the context is important to do the import.
For example when the file contain a translatable name as a key (ex: product.attribute) and currently as there is no context, the search is executed in English (default lang) instead of user's lang.

How do you suggest to implement this? should we create a new module "base_import_async_lang" here on OCA to set language in the context?
Thanks!

[francesco-ooops]

@OCA/connector-maintainers

[acsonefho]

Hi @guewen@simahawk@acsonefho , so going back to the original need (from PR #407 ):

Keep context during import.
In some case, the lang set into the context is important to do the import.
For example when the file contain a translatable name as a key (ex: product.attribute) and currently as there is no context, the search is executed in English (default lang) instead of user's lang.

How do you suggest to implement this? should we create a new module "base_import_async_lang" here on OCA to set language in the context? Thanks!

Maybe you can create a module who contains only this:
https://github.com/OCA/queue/pull/432/files#diff-c6aeb210a5f309f0b1f3ad1cc16e866ae61bab6793ac67eb2c187a28c1664fb6R41

Then ping Guewen and Simahawk to have review and feedback :)

[francesco-ooops]

@acsonefho thanks a lot!

[simahawk]

Instead of adding a new module, you can make base_import_async configurable. Eg: ir.config param by model to get the ctx keys to store by overriding _job_prepare_context_before_enqueue_keys. If you find a ctx key job_ctx_store_keys__$model_name you pick keys from there.
Hmm now that I write this I think we can add this option to queue_job straight... 🤔
@guewen what do you think?

[hildickethan]

Hello, I've been trying to use this feature to send a context from the creation of the job to the actual job runtime itself but I feel like there's something missing.
I inherited _job_prepare_context_before_enqueue_keys and added my key, this works to store the context and it saves in the records field as expected, but I feel like this bit from the original PR #406 is missing to complete the feature.

With this added it runs the jobs with the context that was passed, plus the ones added later for job_uuid and company, but right now it saves the context and does nothing with it.

I just want to check if this is intentionally left out (for backwards compatibility I guess?) and is planned for 16.0 or it was an oversight, because I couldn't find any discussion on this part specifically in the various related PRs

It can be worked around by searching the job_uuid and reading the field for the context, but it seems a bit weird to me

[francesco-ooops]

@simahawk how shall we proceed?

[sbidoul]

@francesco-ooops Expanding on @acsonefho 's suggestion, I'd create a module that does only this:

queue/queue_job/models/base.py

Line 261 in c8b16ff

# return ("tz", "lang", "allowed_company_ids", "force_company", "active_test")

Since that will become the default in v16, it seems reasonable to have module that anticipates the default, which you can install if you are sure that all your jobs are compatible with that.

I feel like this bit from the original PR #406 is missing to complete the feature.

@hildickethan-S73 that makes sense (disclaimer: I've not tested myself). Would you mind doing a PR with the missing part?

[guewen]

I totally agree with @acsonefho and @sbidoul proposal, it's like an opt-in to the behavior that will be the default in 16.0

[francesco-ooops]

@sbidoul@guewen thank you both!

[guewen]

@hildickethan-S73 ooch, that's indeed an oversight from my part 🤦.

This line that initializes the record(s) from the Json in JobDecoder has to set the context:

model = self.env(user=obj.get("uid"), su=obj.get("su"))[obj["model"]]

(Sorry, I'm on my phone and can't manage to copy-paste the exact link in the GitHub app😓)

[hildickethan]

@guewen Just wanted confirmation on it being intentional or not!
Here we go #448
I just copied the code that was in the original PR, the if might not be necessary but I haven't tested without

[AshishHirapara]

@guewen and @sbidoul should I also need to update the discussed changes in #433? It's for v14.

[GSLabIt]

@francesco-ooops Expanding on @acsonefho 's suggestion, I'd create a module that does only this:

queue/queue_job/models/base.py

Line 261 in c8b16ff

# return ("tz", "lang", "allowed_company_ids", "force_company", "active_test")

Since that will become the default in v16, it seems reasonable to have module that anticipates the default, which you can install if you are sure that all your jobs are compatible with that.

I feel like this bit from the original PR #406 is missing to complete the feature.

@hildickethan-S73 that makes sense (disclaimer: I've not tested myself). Would you mind doing a PR with the missing part?

Hi there, we developed a module to return the context as described, but the import (using base_import_async) failing because of not matching the translated country name (ie for italian language):

No matching record found for name 'Regno Unito' in field 'Country'
No matching record found for name 'Germania' in field 'Country'
No matching record found for name 'Francia' in field 'Country'
No matching record found for name 'Paesi Bassi' in field 'Country'

Are we missing something?

[GSLabIt]

@francesco-ooops Expanding on @acsonefho 's suggestion, I'd create a module that does only this:

queue/queue_job/models/base.py

Line 261 in c8b16ff

# return ("tz", "lang", "allowed_company_ids", "force_company", "active_test")

Since that will become the default in v16, it seems reasonable to have module that anticipates the default, which you can install if you are sure that all your jobs are compatible with that.

I feel like this bit from the original PR #406 is missing to complete the feature.

@hildickethan-S73 that makes sense (disclaimer: I've not tested myself). Would you mind doing a PR with the missing part?

Hi there, we developed a module to return the context as described, but the import (using base_import_async) failing because of not matching the translated country name (ie for italian language):

No matching record found for name 'Regno Unito' in field 'Country'
No matching record found for name 'Germania' in field 'Country'
No matching record found for name 'Francia' in field 'Country'
No matching record found for name 'Paesi Bassi' in field 'Country'

Are we missing something?

sorry my fault ..working properly

[francesco-ooops]

@francesco-ooops Expanding on @acsonefho 's suggestion, I'd create a module that does only this:

queue/queue_job/models/base.py

Line 261 in c8b16ff

# return ("tz", "lang", "allowed_company_ids", "force_company", "active_test")

Hi @sbidoul , here is the module, is it ok for you? #462