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

[10.0][IMP] Use the technical user to recompute and export #515

Conversation

acsonefho
Copy link
Contributor

@acsonefho acsonefho commented Nov 18, 2019

On the shopinvader backend view, you have buttons to "Recompute" and "Export".

Edit the current behavior to use the technical user (only if it set on the current user's company).
Because currently, if a user click on this button, he can't switch his company until the end of each jobs (it will cause Access rights exception).

As the technical user not required (on the company) and to keep compatibility: no error will be raised if no technical user (it will use the current user, as currently).

Build/Tests will fails because need update on oca_dependencies.txt (fixed into #505 )

Copy link

@Cedric-Pigeon Cedric-Pigeon left a comment

Choose a reason for hiding this comment

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

Could you please add a test?

@acsonefho acsonefho force-pushed the 10.0-force_technical_user_recompute_export branch from 3fef3ad to b3211f5 Compare November 18, 2019 09:46
@acsonefho
Copy link
Contributor Author

Could you please add a test?

Done :)

Copy link

@Cedric-Pigeon Cedric-Pigeon left a comment

Choose a reason for hiding this comment

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

Code review

Copy link
Collaborator

@lmignon lmignon left a comment

Choose a reason for hiding this comment

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

@acsonefho I would avoid a new dependency for shopinvader_search_engine....

@Cedric-Pigeon
Copy link

@lmignon I am vacillating about your comment. On one hand, I agree we must limit as must as possible dependencies but on the other hand, this is the only way to trust the behaviour in a multi-company context.
The problem is if a user clicks on recompute and changes companies, he is not aware of the failing jobs...

…n shopinvader backend view)

[ADD] Add unit tests
@Cedric-Pigeon Cedric-Pigeon force-pushed the 10.0-force_technical_user_recompute_export branch from b3211f5 to acc37cf Compare March 20, 2020 20:40
Copy link
Contributor

@sebastienbeau sebastienbeau left a comment

Choose a reason for hiding this comment

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

I am ok for adding this new dependency.
For me the base_technical_user should be in the core to solve all multi-company issue.

Regarding the implementation, maybe it will be smarted to add some helper on base_technical_user ?
What do you think about moving the logic of _use_technical_user_if_set in base_technical_user?

Maybe something like (with a better naming) we can inherit the BaseModel class of odoo

def sudo_tech(raise_if_missing=False):
    self_sudoer = self
    tech_user = self.env.user.company_id.user_tech_id
    if tech_user:
        self_sudoer = self.sudo(tech_user.id)
    elif raise_if_missing:
        raise UserError("The technical user is missing in the company {}".format(self.env.user.company_id.name))
    return self_sudoer

Then you can use it everywhere with

self.sudo_tech().mapped("se_backend_id.index_ids").force_recompute_all_binding()

What do you think ?
@Cedric-Pigeon @lmignon

@Cedric-Pigeon
Copy link

@acsonefho @sebastienbeau's idea is really good and will simplify a lot of pieces of code. Can you proceed?

@acsonefho
Copy link
Contributor Author

@acsonefho @sebastienbeau's idea is really good and will simplify a lot of pieces of code. Can you proceed?

Ok, I plan to do it in few days

@sebastienbeau
Copy link
Contributor

I have done a PR in server-tools OCA/server-tools#1824

@sebastienbeau
Copy link
Contributor

PR is merge I am going to modify the PR and merge it

@sebastienbeau
Copy link
Contributor

Replaced by : #622

@shopinvader-git-bot shopinvader-git-bot merged commit acc37cf into shopinvader:10.0 Apr 24, 2020
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