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] fields,models: assure check_company and _check_company_auto makes sense #46721

Closed

Conversation

MiquelRForgeFlow
Copy link
Contributor

@MiquelRForgeFlow MiquelRForgeFlow commented Mar 3, 2020

As how the company consistency related things work now, it is necessary to add some logged warnings to detect wrong uses. Thus, the added warnings are:

1. Don't put _check_company_auto to a model which doesn't have a 'company_id' field.
2. Don't put check_company=True attribute to fields whose model doesn't have a 'company_id' field.
3. Don't put check_company=True attribute to fields whose model doesn't have _check_company_auto=True (or explicit _check_company_auto=False, cases where is preferred to call _check_company() method in another method distinct from write/create)

Description of the issue/feature this PR addresses:

  • From (1.) warning: detected 0 cases.
  • From (2.) warning: detected 1 cases: product.category doesn't have 'company_id' field
  • From (3.) warning: detected several cases. Excluding the ones I marked with explicit _check_company_auto=False, the detected ones are:

account.analytic.line.employee_id
product.supplierinfo.name
product.supplierinfo.product_id
product.supplierinfo.product_tmpl_id
purchase.order.user_id
purchase.requisition.user_id
sale.order.template.line.product_id
stock.quant.package.packaging_id
choose.delivery.package.delivery_packaging_id

Current behavior before PR:

The check_company=True attributes in several fields are not working.

Desired behavior after PR is merged:

The detected cases above are fixed.

--
I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr

@sle-odoo
Copy link
Contributor

sle-odoo commented Mar 3, 2020

  1. Don't put check_company=True attribute to fields whose model doesn't have a 'company_id' field.

i think you missed the fact that check_company=True is set on property fields on these occurrences, so it is working properly and should not be reported by your warnings

@MiquelRForgeFlow
Copy link
Contributor Author

I don't understand your statement. Those four property fields of product.category don't need the company_check=True, because as I said, the model product.category doesn't have company_id field nor _check_company_auto=True. Thus, when you create/write a product category, the _check_company() method will never be called.

@sle-odoo
Copy link
Contributor

sle-odoo commented Mar 3, 2020

but a proper domain will be applied

return "[('company_id', 'in', [allowed_company_ids[0], False])]"

@MiquelRForgeFlow
Copy link
Contributor Author

MiquelRForgeFlow commented Mar 3, 2020

Interesting. I didn't know that this _description_domain method that is called when loading views uses the check_company attribute. It is used to fill the domain in case the field doesn't have a domain. So if I am not mistaken, your initial point is that in the second warning (and the third), we should let property fields have check_company=True in models that don't have company_id/_check_company_auto because they could still help in this domain view case. Ok.

But also note that those four property fields of product category have already a defined domain, so the return you pointed won't apply to them. Their domain will be the one they have explicitly defined so they don't need the check_company=True :)

@sle-odoo
Copy link
Contributor

sle-odoo commented Mar 4, 2020

But also note that those four property fields of product category have already a defined domain

they don't ? https://github.com/odoo/odoo/blob/13.0/addons/stock_account/views/product_views.xml#L22

@MiquelRForgeFlow
Copy link
Contributor Author

here 🤔

domain="[('company_id', '=', allowed_company_ids[0]), ('deprecated', '=', False)]", check_company=True,

@sle-odoo
Copy link
Contributor

sle-odoo commented Mar 5, 2020

oh you're right :)
well in this case, the check_company=True attribute indeed doesn't do anything.

so we could either

  • remove them in master
  • add a required company_id and change the property fields to classical fields and set check_company everywhere

the second option has a functional impact and i don't think we want to go in that direction, i'll ask around

…nsistency

As how the company consistency related things work now, it is necessary to add some logged warnings to detect wrong uses. Thus, the added warnings are:

- Don't put _check_company_auto to a model which doesn't have a 'company_id' field.
- Don't put check_company attribute to fields whose model doesn't have a 'company_id' field.
- Don't put check_company attribute to fields whose model doesn't have _check_company_auto=True (or explicit _check_company_auto=False, cases where is preferred to call _check_company() method in another method distinct from write/create)
@robodoo robodoo removed the CI 🤖 Robodoo has seen passing statuses label Mar 6, 2020
@robodoo robodoo added the CI 🤖 Robodoo has seen passing statuses label Mar 6, 2020
@MiquelRForgeFlow
Copy link
Contributor Author

excluded property fields in warnings 2 and 3 😉

@C3POdoo
Copy link
Contributor

C3POdoo commented Apr 16, 2024

Dear @MiquelRForgeFlow,

Thank you for your contribution but the version 13.0 is no longer supported.
We only support the last 3 stable versions so no longer accepts patches into this branch.

We apology if we could not look at your request in time.
If the contribution still makes sense for the upper version, please let us know and do not hesitate to recreate one for the recent versions. We will try to check it as soon as possible.

This is an automated message.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI 🤖 Robodoo has seen passing statuses
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants