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

[MERGE] base,* : remove force_company context key mechanism #36804

Open
wants to merge 13 commits into
base: master
from

Conversation

@Feyensv
Copy link
Contributor

commented Sep 12, 2019

ORM

  • Add with_company to update allowed_company_ids context key.
  • Consider sudoed environment to accept/refuse a company
  • Crash when invalid allowed_company_ids context is given.

If a computed field depends on the current company and/or restricted company, add company in depends_context decorator arguments (done by default for company_dependent fields).

Replace force_company and custom company* context keys by with_company uses.

  • remove force_company keys
  • remove company_id keys --> partly done, remaining is for master
  • remove company context keys
  • remove company_ids and multi_company context keys --> for Master

Task-ID

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

@C3POdoo C3POdoo added the RD label Sep 12, 2019
@Feyensv Feyensv requested review from antonylesuisse, rco-odoo and tivisse Sep 12, 2019
@Feyensv Feyensv force-pushed the odoo-dev:master-rem-force-comp-vfe branch from 796521c to 80de47e Sep 12, 2019
@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Sep 12, 2019
@Feyensv Feyensv requested a review from odony Sep 13, 2019
@tivisse

This comment has been minimized.

Copy link
Contributor

commented Sep 13, 2019

Hi @Feyensv

I probably won't take a look at this before the OXP. We're currently in a stabilization phase.

I also think that @odony and @rco-odoo should take a look a this too.

@Feyensv

This comment has been minimized.

Copy link
Contributor Author

commented Sep 13, 2019

@tivisse their review is already requested ^^.
No problem, either AL wants to merge this in 12.5 and we will request reviews personally, or it will wait in its current state 👍.

@robodoo robodoo removed the CI 🤖 label Sep 13, 2019
@Feyensv Feyensv force-pushed the odoo-dev:master-rem-force-comp-vfe branch 3 times, most recently from 38a5738 to b533f33 Sep 14, 2019
@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Sep 14, 2019
@Feyensv Feyensv force-pushed the odoo-dev:master-rem-force-comp-vfe branch from a3b13a1 to 40e6d52 Sep 16, 2019
@robodoo robodoo added the CI 🤖 label Sep 16, 2019
@Feyensv Feyensv force-pushed the odoo-dev:master-rem-force-comp-vfe branch from 40e6d52 to 987a031 Sep 16, 2019
@robodoo robodoo removed the CI 🤖 label Sep 16, 2019
@Feyensv Feyensv force-pushed the odoo-dev:master-rem-force-comp-vfe branch 2 times, most recently from 9ce411a to 94e32e1 Sep 16, 2019
addons/account/models/chart_template.py Outdated Show resolved Hide resolved
odoo/models.py Outdated Show resolved Hide resolved
addons/l10n_ar/models/account_fiscal_position.py Outdated Show resolved Hide resolved
addons/sale_timesheet/models/sale_order.py Outdated Show resolved Hide resolved
@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Sep 16, 2019
@Feyensv Feyensv force-pushed the odoo-dev:master-rem-force-comp-vfe branch from 6317284 to 0b36886 Sep 16, 2019
@robodoo robodoo added the CI 🤖 label Oct 8, 2019
@@ -94,7 +94,7 @@ class Product(models.Model):
@api.depends('stock_move_ids.product_qty', 'stock_move_ids.state')
@api.depends_context(
'lot_id', 'owner_id', 'package_id', 'from_date', 'to_date',
'company_owned', 'force_company',
'company_owned', 'company',

This comment has been minimized.

Copy link
@Feyensv

Feyensv Oct 9, 2019

Author Contributor

Shouldn't we add 'allowed_company_ids' also @sle-odoo ?

This comment has been minimized.

Copy link
@sle-odoo

sle-odoo Oct 9, 2019

Contributor

this field has a lot of side effects (i just listed some here)

i think it is better it is only computed with the records of the main company

This comment has been minimized.

Copy link
@Feyensv

Feyensv Oct 9, 2019

Author Contributor

Even in multi-company 🤔 ?
(Do we want to see all quantities on product form in multi-company or not 🤔 ?)

This comment has been minimized.

Copy link
@sle-odoo

sle-odoo Oct 9, 2019

Contributor

it has side effects so i would say no at the moment

This comment has been minimized.

Copy link
@Feyensv

Feyensv Oct 9, 2019

Author Contributor

Testing in #38304

@Feyensv Feyensv force-pushed the odoo-dev:master-rem-force-comp-vfe branch from a943911 to cdec972 Oct 9, 2019
@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Oct 9, 2019
odoo/api.py Outdated Show resolved Hide resolved
odoo/api.py Outdated
if not self.su:
user_company_ids = self.user.company_ids.ids
if any(cid not in user_company_ids for cid in allowed_company_ids):
raise AccessError(_("Access to unauthorized or invalid companies."))

This comment has been minimized.

Copy link
@odony

odony Oct 11, 2019

Contributor

should be covered with a regression test

elif self.env.is_superuser() or company_id in self.env.user.company_ids.ids:
allowed_company_ids.insert(0, company_id)
else:
raise AccessError(_("Company %i isn't allowed for user %s") % (company_id, self.env.user))

This comment has been minimized.

Copy link
@odony

odony Oct 11, 2019

Contributor

permission checks are redundant with what the env does, let's see with_company as syntactic sugar for context manipulation only (and the context is never trusted anyway, even afterwards)

'company_id': parent.company_id
}"
/>
<field name="product_uom_qty"/>

This comment has been minimized.

Copy link
@odony

odony Oct 11, 2019

Contributor

have a look at 1cedcf6 so you can double-check if this is really unused

@@ -79,8 +79,8 @@ def _create_production_location(self):
})

def _create_scrap_location(self):
parent_location = self.env.ref('stock.stock_location_locations_virtual', raise_if_not_found=False)

This comment has been minimized.

Copy link
@odony

odony Oct 11, 2019

Contributor

another PR?

base_domain = [
('auto_apply', '=', True),
('vat_required', '=', vat_required),
('company_id', '=', self.env.company.id),

This comment has been minimized.

Copy link
@odony

odony Oct 11, 2019

Contributor

for 13.0: with force_company too

if company.country_id == self.env.ref('base.ar'):
domain = [
('auto_apply', '=', True),
('l10n_ar_afip_responsibility_type_ids', '=', self.env['res.partner'].browse(
partner_id).l10n_ar_afip_responsibility_type_id.id),
('company_id', '=', self.env.company.id),

This comment has been minimized.

Copy link
@odony

odony Oct 11, 2019

Contributor

ok for 13.0

@@ -370,7 +370,7 @@ def _compute_has_unreconciled_entries(self):

def mark_as_reconciled(self):
self.env['account.partial.reconcile'].check_access_rights('write')
return self.sudo().with_context(company_id=self.env.company.id).write({'last_time_entries_checked': time.strftime(DEFAULT_SERVER_DATETIME_FORMAT)})
return self.sudo().write({'last_time_entries_checked': time.strftime(DEFAULT_SERVER_DATETIME_FORMAT)})

This comment has been minimized.

Copy link
@odony

odony Oct 11, 2019

Contributor

Should be safe to remove as the root cause was the old behavior of sudo(). Cfr OPW-1770018 and #19274 and related commits.

@Feyensv Feyensv changed the base branch from 13.0 to master Oct 11, 2019
Feyensv added 13 commits Sep 13, 2019
From now on, if one wants to force following operations to happen in a given company,
use with_company(company) or with_company(cid) to update the environment.
Use correct company to get fiscal positions and properties

Ensure company used to fetch fiscal positions is always the correct one
when coming from a company restricted model (company_id required, or
related.required).

NB: if the company_id isn't defined, with_company doesn't change the
environment.

purchase: get_fiscal_position doesn't consider company_id ctxt key

others: properties were accessed with potentially the wrong company.
@Feyensv Feyensv force-pushed the odoo-dev:master-rem-force-comp-vfe branch from cdec972 to da2df26 Oct 11, 2019
@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Oct 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.