Skip to content

Conversation

@acsonefho
Copy link
Contributor

@acsonefho acsonefho commented Aug 2, 2021

I currently have some trouble to trigger job because they use the default company of the user instead of the active one.
I see this open discussion: #363 but IMO using a serialized context is not in opposition with this solution as the key allowed_company_ids is not always in the context (only if the user switch company?).

I did 2 commits (IMO it's important to keep them):

  1. Force companies into the context (via allowed_company_ids key)
  2. Only load the job's company into allowed_company_ids because it could load too many unexpected records (during search).

Use the current company to trigger the job (+ add related tests)
@OCA-git-bot
Copy link
Contributor

Hi @acsonefho! The addon you are improving has no declared maintainer.
If you care, you can consider adopting it. To do so, please read the
maintainer role description then create a
pull request to add your GitHub login to the maintainers key of the
addon manifest.

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.
@acsonefho acsonefho force-pushed the 13.0-queue_job_current_company branch from 113c8af to 4d43bd8 Compare August 18, 2021 07:33
@sbidoul
Copy link
Member

sbidoul commented Aug 18, 2021

Shouldn't we rather design the jobs to work correctly irrespective of the user and job company_id ?

@guewen
Copy link
Member

guewen commented Sep 3, 2021

Hi, maybe I don't get the use case properly, but I'm not sure to agree with:

using a serialized context is not in opposition with this solution as the key allowed_company_ids is not always in the context

If the job was called synchronously like a normal method, and the allowed_company_ids was not in the context, you would have the same issue then? So it means the issue is on the caller not setting it. I don't see why calling asynchronously would need to have a different behavior? And if the caller did set allowed_company_ids then if we store the context, we'll have it automatically.

@acsonefho
Copy link
Contributor Author

Hi, maybe I don't get the use case properly, but I'm not sure to agree with:

using a serialized context is not in opposition with this solution as the key allowed_company_ids is not always in the context

If the job was called synchronously like a normal method, and the allowed_company_ids was not in the context, you would have the same issue then? So it means the issue is on the caller not setting it. I don't see why calling asynchronously would need to have a different behavior? And if the caller did set allowed_company_ids then if we store the context, we'll have it automatically.

IMO if the allowed_company_ids is not into the context, so the default company of the user is used. So job.company_id == self.env.user.company_id == self.env.company is True so no issue.
But if the user switch company to trigger a job, job.company_id == self.env.user.company_id == self.env.company become False. But job.company_id == self.env.company still True. It's why it's required to keep the current company to trigger the job.

I totally agree with the option to store the context into the job. That fix this company issue. But for now as nobody agree to store it

@guewen
Copy link
Member

guewen commented Nov 10, 2021

But for now as nobody agree to store it

I do agree to store it (in the recordset as discussed in #281 (comment)), I proposed it here #283. I'm sure other people agree too, the variances of opinions may be more in what exactly in the context we allow to store or not, but we can start small with an allow-list with the standard keys.
It's just that nobody worked on it.

@github-actions
Copy link

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

@github-actions github-actions bot added the stale PR/Issue without recent activity, it'll be soon closed automatically. label Apr 17, 2022
@github-actions github-actions bot closed this May 22, 2022
@sbidoul sbidoul deleted the 13.0-queue_job_current_company branch May 22, 2022 13:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stale PR/Issue without recent activity, it'll be soon closed automatically.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants