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

[FIX][10.0][multi_company_abstract] Fix company_id computation issue (+ add unit test) #91

Merged
merged 1 commit into from
Apr 23, 2018

Conversation

acsonefho
Copy link
Contributor

Steps to reproduce (actual) error:

  • Configuration: product.template inherit multi.company.abstract
  • Log in (not with Admin) with a user set with at least 2 company_ids
  • Browse a product with at least 2 company_ids
  • Check the company_id of the product
  • If the company_id of the product is the same than the current company, stay on the same product but switch your current company (top right button)
  • The company_id of the product is not your actual company: you will have some issue (for example to use pricelist (price list set to without_discount as discount_policy) on sale.order because this model use the product.company_id.currency_id => BOOM, AccessError exception because you don't have rights to read another company than the current one).

Fix (this MR):

  • To compute the company_id field of a multi.company.abstract, give the priority to the company of the current user. If this company is not into company_ids of the targeted model, pick the first company (actual behaviour)

@pedrobaeza
Copy link
Member

This seems similar to #86, isn't it?

@acsonefho
Copy link
Contributor Author

@pedrobaeza Ah yes exactly same issue (so quite similar fix). Sorry, I didn't see it before

@pedrobaeza
Copy link
Member

No problem. Let's keep this one as better documented and with tests.

@@ -1,12 +1,13 @@
# -*- coding: utf-8 -*-
Copy link
Member

Choose a reason for hiding this comment

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

Don't remove this in v10.

# Copyright 2017 LasLabs Inc.
# License LGPL-3.0 or later (http://www.gnu.org/licenses/lgpl.html).

{
'name': 'Multi Company Base',
'summary': 'Provides a base for adding multi-company support to models.',
'version': '10.0.1.0.1',
'author': "LasLabs, Tecnativa, Odoo Community Association (OCA)",
'author': "LasLabs, Tecnativa,"
"ACSONE SA/NV,"
Copy link
Member

Choose a reason for hiding this comment

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

This patch is not enough for granting you co-authorship. Put you in contributors in README instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok no problem. My internal check becomes red due to the coding utf-8 and because I didn't add my company into the module. I'll correct this in few minutes

@acsonefho acsonefho force-pushed the fix_multi_company_issue branch 3 times, most recently from 867cc4b to e06a3e3 Compare March 27, 2018 11:34
@acsonefho
Copy link
Contributor Author

@pedrobaeza The test is red due to another module. Seems ok for you now?
Thanks

@pedrobaeza
Copy link
Member

No, @acsonefho , Travis is failing due to this flake8 error in your changes:

base_multi_company/tests/test_multi_company_abstract.py:188:13: F841 local variable '_' is assigned to but never used

[FIX] Fix issue based on the computation of company_id for multi_company_abstract models

[FIX] Fix issue based on the computation of company_id for multi_company_abstract models

[FIX] Fix issue based on the computation of company_id for multi_company_abstract models

[FIX] Bad branch version pushed before

[FIX] Bad branch version pushed before

[FIX] manifest

Fix flake

Flake8 on OCA tests
Copy link
Member

@SimoRubi SimoRubi 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, very nice with tests too!

@pedrobaeza pedrobaeza merged commit f605422 into OCA:10.0 Apr 23, 2018
SilvioC2C pushed a commit to camptocamp/multi-company that referenced this pull request Mar 7, 2023
BSSBG-2615: Complete mrp.production labels
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.

3 participants