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

[DOC] Add words of caution about sudo #9655

Closed
wants to merge 3 commits into from

Conversation

clonedagain
Copy link

sudo may introduce perfs problem and break multi-company features when used without caution.

This adds the needed documentation.

@clonedagain
Copy link
Author

@lmignon I propose to update the official docs with what I understand from you comments in OCA/stock-logistics-warehouse#81 (comment) and OCA/stock-logistics-warehouse#93 (comment)

@clonedagain
Copy link
Author

@odony would you agree with adding this bit of documentation please?

@lmignon
Copy link
Contributor

lmignon commented Nov 20, 2015

@clonedagain 👍

@xmo-odoo
Copy link
Collaborator

That looks good, although the pseudo-paragraphs in the text are not going to work once compiled to RST, either create "genuine" paragraphs in plain text (with a blank line inbetween) or create single paragraphs, but not just carriage returns between sentences.

Might be good to use warning rST directives as well for better visibility, for the cache and multi-company parts at least.

@clonedagain
Copy link
Author

Amended as suggested by @xmo-odoo.

@clonedagain
Copy link
Author

maybe warning is overkill and notewould suffice?

@@ -5306,6 +5306,10 @@ def with_env(self, env):
""" Returns a new version of this recordset attached to the provided
environment

.. warning::
The new environment may not benefit from the initial environment's
data cache. This may introduce performance problems.
Copy link
Contributor

Choose a reason for hiding this comment

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

May I suggest a wording change?

The new environment will not benefit from the current environment's
data cache, so later data access may incur extra delays while re-fetching
from the database.

My main point is that it doesn't introduce "problems", it may cause extra delays that may or may not be a problem (user-facing actions vs background jobs, etc.) The documentation should just state the facts, the developer should infer the consequences according to his particular application.

@odony
Copy link
Contributor

odony commented Nov 20, 2015

On 11/20/2015 12:02 PM, Lionel Sausin wrote:

maybe |warning| is overkill and |note|would suffice?

Yes, note sounds reasonable indeed.

@wtaferner
Copy link
Contributor

Well, based on this additional very welcome documentation for devs I thought about maybe doing it even better and respect the company from the original environment? I don't know if this is easily possible concerning other side-effects but it seems not impossible on a quick look in sudo...self.env.user.company_id is available before returning with_env, right?

At least this is what I did in v7 for my multicompany instance to change the company of the sudo user back and forth to actually get it right.

My 2 cents about this issue.

@clonedagain
Copy link
Author

@wtaferner this is out of scope here but you could file an issue about it.

still objective hopefully
@wtaferner
Copy link
Contributor

@clonedagain
Sorry for hijacking...you are right...thank you for contributing this PR

@clonedagain
Copy link
Author

@odony I integrated your suggestions but added an the concrete examples of default company and default BoM.

@clonedagain
Copy link
Author

@wtaferner no problem you're welcome

odony pushed a commit that referenced this pull request Nov 20, 2015
@odony
Copy link
Contributor

odony commented Nov 20, 2015

Merged/squashed at 6188bc6 in 8.0, thanks!

@odony odony closed this Nov 20, 2015
@clonedagain clonedagain deleted the 8.0-doc-warning-sudo branch November 23, 2015 09:25
@clonedagain clonedagain restored the 8.0-doc-warning-sudo branch November 23, 2015 09:52
@clonedagain clonedagain deleted the 8.0-doc-warning-sudo branch November 23, 2015 09:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants