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

[MULTICOMPANY] Removed "child_of" from all record rules #31943

Conversation

Projects
None yet
@hdh-odoo
Copy link
Contributor

commented Mar 19, 2019

Description of the issue/feature this PR addresses:

Description:
-By default users can see records of their company only.
-They can no longer see records of child companies as it was confusing before.

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

@robodoo robodoo added the seen 🙂 label Mar 19, 2019

@C3POdoo C3POdoo added the RD label Mar 19, 2019

@etobella

This comment has been minimized.

Copy link
Contributor

commented Mar 19, 2019

Why?!?!
This disable the options to work on a multicompany environment.
For example you could use https://github.com/eficent/multicompany-fixes
This was a topic on #20508

@sbidoul

This comment has been minimized.

Copy link
Contributor

commented Mar 19, 2019

Please don't do this!

@Yenthe666

This comment has been minimized.

Copy link
Contributor

commented Mar 19, 2019

That's going to be fun when a functional person makes mistakes in a multi company environment then. /sarcasm

@Cedric-Pigeon

This comment has been minimized.

Copy link
Contributor

commented Mar 19, 2019

It is a big big mistake to remove this feature. it is absolutely required to have efficient multi-company management.

@etobella

This comment has been minimized.

Copy link
Contributor

commented Mar 19, 2019

If necessary, improvements of code could be added in order to allow a real multicompany environment, not removing the option. We are currently working with 9 companies at the same time without using the Company selector widget. Users don't need to change between companies and no errors have been raised.
Data is perfectly fine and no problems on UX. That could be the real solution.

@mgielissen

This comment has been minimized.

Copy link

commented Mar 19, 2019

Please keep this!

@moylop260

This comment has been minimized.

Copy link
Contributor

commented Mar 19, 2019

What about using extra rules with a group_id=child_companies using child_of?

I mean,

  • Global and default behaviour is just my company.
  • But if you user has a child_companies group assigned then you can see records child of companies.

It could be have both worlds without problem IMHO

@jjscarafia

This comment has been minimized.

Copy link
Contributor

commented Mar 20, 2019

Please don't do this!

@mohamedhagag

This comment has been minimized.

Copy link
Contributor

commented Mar 20, 2019

Is this an April's fool ?!!

@anhvu-sg

This comment has been minimized.

Copy link

commented Mar 20, 2019

Please don't do this !!!

@Yenthe666

This comment has been minimized.

Copy link
Contributor

commented Mar 20, 2019

@mart-e & @sle-odoo I guess it is clear by the amount of responses that its not going to make a lot of people happy. Any chance you can look at this again please?

@elicoidal

This comment has been minimized.

Copy link

commented Mar 20, 2019

Would it make sense to make it "optional" so that we have the possibility to switch with one behavior or the other (specific module to load etc.)?

@fpodoo

This comment has been minimized.

Copy link
Contributor

commented Mar 20, 2019

Hello,

Some companies need 'child_of', but even more companies need '='. The setup we propose by default should be the easiest and safest choice, that's why we change the default config.

PROs of '=':

  • the safest choice, you can not mess up your data (e.g. an employee of the HQ can not report an expense with a product belonging to a subsidiary, or ship to a location of another warehouse)
  • easy to understand, no side effects
  • easier to report on a specific company (e.g. if you analyse sales of the HQ, the data are not messed up with extra companies)
  • faster: child_of adds a lot of SQL queries at each read

PROs of 'child_of':

  • allow more use cases (the main one being companies managing different brands with multi-level hierarchies)

Neither PROs or CONs::

  • both approaches allow products that are common to all companies (company_id=False)
  • both allows multi-company accounting reports as this is managed by the accounting report engine (and we are working on a real consolidation app)

The default configuration should fit the most common use case. '=' will fit the majority of the companies need out-of-the-box, whereas 'child_of' rarely work without an expert to do the setup for you. (we manage Odoo SA, with 6 subsidiaries without using parent_id on the company, as it's simpler; and I am happy to use the company switcher as it allows me to report on Odoo Belgium easily, the same way I report on Odoo US).

Most people do not understand the implications of setting a parent_id, so they just do it because the feature exists. It ends up in a overly complex setup, even though it's not always necessary.

As the setup by default should be the easiest and safest approach, we prefer to use '='. But, of course, it does not prevent you from converting the rule into 'child_of' if you have a use case where you absolutely need it.

@etobella

This comment has been minimized.

Copy link
Contributor

commented Mar 20, 2019

@fpodoo Just a question, Isn't it better to fix the issues between the selection of companies and add constrains than removing a feature?
We have been working on a solution that disallows the option to create inconsistent data and that solves your issue and keeps the feature. If you are interested we could talk about this in order to add to odoo core.

@lmignon

This comment has been minimized.

Copy link
Contributor

commented Mar 20, 2019

We must fix/improve existing, not remove something required for our customers. How will we justify to our customers that they will have to pay during the migration to restore something that was natively supported before?

@gdgellatly

This comment has been minimized.

Copy link
Contributor

commented Mar 20, 2019

@etobella describes the sap way. This should be the ideal. But actually as someone who runs a lot of multicompany I never use parent id.

But you could solve this in reverse by hiding parent Id unless specifically checked in config. Then you get all the pros of both methods.

@mohamedhagag

This comment has been minimized.

Copy link
Contributor

commented Mar 20, 2019

@fpodoo, @hdh-odoo
My opinion is,
It's logic if I'm on a parent company to have access to children companies data and intentionally take care of what I'm doing.

Otherwise I have to switch to the child company and do whatever I want.

It's a matter of user awareness and configuration of the companies hierarchy not to be forced programmatically.

@gustavovalverde

This comment has been minimized.

Copy link
Contributor

commented Mar 20, 2019

I've always wonder, when this decisions are taken, which implementations are being taken into consideration? Odoo S.A. implementations, or all the implementations made by the partners?

Somehow there must be information on how much companies are needing = and all the actual use cases using child_of that will break on a migration.

Anyways, this is removing a feature, but not solving the real problem of multi-companies. I do agree that #20508 is tackling the problem more efficiently and making it future-proof. We have environments with over +30 multi-companies; we know how painful it is to maintain 😄

P.D.: We've never used child_of for complex use cases (not the best approach); but just for financial reports consolidation using a 'virtual parent'.

@mreficent

This comment has been minimized.

Copy link
Contributor

commented Mar 20, 2019

Well, people, thanks for linking to #20508, but that old PR was a first approach and just to show up some ideas, but would need a lot work and fine tuning. But as @etobella said, it's something we are hoping to talk about someday. What we have tested and works really fine is this.

@bealdav

This comment has been minimized.

Copy link
Contributor

commented Mar 20, 2019

I love this point

As the setup by default should be the easiest and safest approach

My suggestions, then:

  • Translatable fields by default: Ok for product name but why for warehouse, location, delivery carrier, etc ; the most common case is only one lang there
  • Mail thread activated by default (sale, purchase) resulting a huge mail_message table after years : should be made inactive by UI.
  • Put a specific css to company dependent fields, then users'll understand this is their fields, not only the fields of the objects

Concerning security advanced rules, I think OCA'll find a good solution (a boolean on the rule may allowed to switch from a case to another)

Switch to jsonb fields for property fields could also help advanced scenarii to be implemented by community, for example

Thanks

@fpodoo

This comment has been minimized.

Copy link
Contributor

commented Mar 20, 2019

@etobella there is actually no technical issues: constraints are working well. It's just not simple as an UX point of view to have data mixed up.

@lmignon there is no change during migrations, as we will not overwrite and change the rules. It only applies to new databases

@gustavovalverde all our decisions are based on our experiences. That includes: project we do directly (hundreds per year), feedback from partners, support tickets (3500 / month), ... But most of the time, it's just common sense.

@bealdav I don't think there is a solution to find / fix. I do really think it's better for most companies if they are not tempted to create "child_of" rules.

@fpodoo

This comment has been minimized.

Copy link
Contributor

commented Mar 20, 2019

@bealdav I checked translatable fields: warehouse and location's names are not translatable. Delivery Method name is translatable, but it is normal: you want "Free delivery" or "UPS 1 day delivery" in the right language in your ecommerce, or in your SO. No issues there.

For mail threads, having large tables is not a real issues nowadays. People want to keep the history of communications; there is no reason to drop them as the impact on performance is minimal with indexes. We do have bad performances due to mail.thread but it's not linked to the table size; it's more for write/create operations (part of the code can be optimized, we are on it)

@blaggacao

This comment has been minimized.

Copy link
Contributor

commented Mar 20, 2019

Still a global configuration switch seems optimal in a UX point of view, because I don't want to sell consulting time spent tracing down all those cases where I have to replace = by child_of. Suggestion: create both, deactivate child_of ones by default and do the switch through a config item.

I wouldn't go so far as to extend any model and make this a functional feature. Being data driven feature was / is / will be just fine.

Just make setup easier in case we need it.

@etobella

This comment has been minimized.

Copy link
Contributor

commented Mar 21, 2019

So allowing the system (by interface and xmlrpc) to make an invoice for company A and accounts of company B is working as expected?
Let's take invoice as an example. IMO a constrain for the accounts (like the following) should be necessary:
https://github.com/Eficent/multicompany-fixes/blob/11.0/mcfix_account/models/account_invoice.py#L142
Also, most of the flows on the parent company may work just by adding
self = self.with_context(force_company=self.company.id)
You can see that most of the functions that we added were constrains or the force_company
https://github.com/Eficent/multicompany-fixes/blob/11.0/mcfix_account/models/account_invoice.py#L194
That is the exercise that we have made in order to have both worlds, a working multicompany environment and a consistent system.
The last point is to see only the expected data. That can be solved by adding the right domain on the fields:
https://github.com/Eficent/multicompany-fixes/blob/11.0/mcfix_account/views/account_invoice_view.xml#L24

If you have 10 companies and a single person on purchases is crazy to introduce the data on the system changing the company every time, as it is refreshing your interface every time. It could be faster and safer using our proposals.

@lmignon

This comment has been minimized.

Copy link
Contributor

commented Mar 21, 2019

@lmignon there is no change during migrations, as we will not overwrite and change the rules. It only applies to new databases

I think you forget a very important part of our work to ensure the quality of our developments and detect possible regressions when we add new functionalities, migrate or simply update Odoo sources: Unit tests. And these tests are not performed on migrated DBs.

You think that it's better to remove chilf_ofoperator by default in multi companies rules. OK but in place of removing this functionality why not to keep the existing rules for a specific security group to be able to enable these rules by configuration?

@blaggacao

This comment has been minimized.

Copy link
Contributor

commented Mar 21, 2019

I wouldn't make it a security group dependent configuration item, but at least a configuration item. It's metadata config, after initial setup it's usually still meant to be customized. This is why I would not "functionalize" it through a securities group.

@mcassuto

This comment has been minimized.

Copy link

commented Mar 25, 2019

@fpodoo Instead of removing all these rule, could you maintain them in a separate module ?

@hdh-odoo hdh-odoo force-pushed the odoo-dev:master-recordrule-multicompany-removed-childof-hdh branch from 87fa9e5 to 3e5532b Mar 26, 2019

@robodoo robodoo added the CI 🤖 label Mar 26, 2019

@hdh-odoo hdh-odoo force-pushed the odoo-dev:master-recordrule-multicompany-removed-childof-hdh branch from 5ce7cf2 to 1745dd3 Mar 26, 2019

@robodoo robodoo removed the CI 🤖 label Mar 26, 2019

@blaggacao

This comment has been minimized.

Copy link
Contributor

commented Mar 26, 2019

@etobella No functional programming on data driven settings, please 😉 Data driven means data driven not functinonal-data-functional-data indirection. The ACL being data driven is a fundamental and very important design choice. Still having both variants loaded (and maintained & tested), one active the other one inactive, I guess is what people ask for.

@etobella

This comment has been minimized.

Copy link
Contributor

commented Mar 26, 2019

@blaggacao Maybe it is not the best option.

My concern is that if this kind of rules are applied, Odoo will not review if the code works on multicompany environments as their standard rules do not allow it and more and more modules will be needed in order to make it work.

Now it is easy to make a real multicompany environment on odoo but it will be harder with time...

@mba-odoo mba-odoo force-pushed the odoo-dev:master-recordrule-multicompany-removed-childof-hdh branch 2 times, most recently from 8501036 to 215284e Mar 27, 2019

mba-odoo added a commit to odoo-dev/odoo that referenced this pull request Mar 27, 2019

[IMP] Stock : Replace the company id warehouse
Purpose of the commit is to replace the company_id of warehouse
as parent company do not have right to access the child company warehouse
so main flow tour was breaked.

Task-ID : 1957193
Closes : odoo#31943

@robodoo robodoo added the CI 🤖 label Mar 27, 2019

@mba-odoo mba-odoo force-pushed the odoo-dev:master-recordrule-multicompany-removed-childof-hdh branch from 215284e to 7190663 Mar 27, 2019

mba-odoo added a commit to odoo-dev/odoo that referenced this pull request Mar 27, 2019

[IMP] *_* : Replace the record rules for multi company
Purpose:
It is confusing for users to see the records from the company
he is connected to and the records of the children companies

So replace the 'child_of' oprator for company_id with the 'Equal(=)' operator
So by default users can see records of their Own company.
They can no longer see records of child companies as it was confusing before.

Task-ID : 1957193
Closes : odoo#31943

mba-odoo added a commit to odoo-dev/odoo that referenced this pull request Mar 27, 2019

[IMP] base : Apply group no one for Parent id in company.
Hide the field parent_id (on res.company) in debug mode as well as in multi company groups

Task-ID : 1957193
Closes : odoo#31943

mba-odoo added a commit to odoo-dev/odoo that referenced this pull request Mar 27, 2019

[IMP] Stock : Replace the company id warehouse
Due to change is record rules now parent company do not have
right to access the records of child company and
here in stock demo data ware house was created for another child company
which is not accessible by admin so main flow tour breaked.

So replace the company_id of warehouse
as parent company do not have right to access the child company's warehouse

Task-ID : 1957193
Closes : odoo#31943

mba-odoo added a commit to odoo-dev/odoo that referenced this pull request Mar 27, 2019

[IMP] maintenance : Improve the tes case for maintenance
Due to change in record rule now parent comapany do not have
right to access the records of child company

So modify the maintenance multi company test case and
switch the company of manager when manager is going to modify
the child company records.

Task-ID : 1957193
Closes : odoo#31943

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Mar 27, 2019

hdh-odoo and others added some commits Mar 19, 2019

[IMP] *_* : Replace the record rules for multi company
Purpose:
It is confusing for users to see the records from the company
he is connected to and the records of the children companies

So replace the 'child_of' oprator for company_id with the 'Equal(=)' operator
So by default users can see records of their Own company.
They can no longer see records of child companies as it was confusing before.

Task-ID : 1957193
Closes : #31943
[IMP] base : Apply group no one for Parent id in company.
Hide the field parent_id (on res.company) in debug mode as well as in multi company groups

Task-ID : 1957193
Closes : #31943
[IMP] Stock : Replace the company id warehouse
Due to change is record rules now parent company do not have
right to access the records of child company and
here in stock demo data ware house was created for another child company
which is not accessible by admin so main flow tour breaked.

So replace the company_id of warehouse
as parent company do not have right to access the child company's warehouse

Task-ID : 1957193
Closes : #31943
[IMP] maintenance : Improve the tes case for maintenance
Due to change in record rule now parent comapany do not have
right to access the records of child company

So modify the maintenance multi company test case and
switch the company of manager when manager is going to modify
the child company records.

Task-ID : 1957193
Closes : #31943

@mba-odoo mba-odoo force-pushed the odoo-dev:master-recordrule-multicompany-removed-childof-hdh branch from 7190663 to 722da1d Apr 2, 2019

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Apr 2, 2019

@tivisse tivisse closed this Apr 5, 2019

@tivisse tivisse deleted the odoo-dev:master-recordrule-multicompany-removed-childof-hdh branch Apr 5, 2019

@robodoo robodoo added closed 💔 and removed CI 🤖 labels Apr 5, 2019

@rvalyi

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2019

follow up: Odoo SA just claimed they will develop it another way:
https://twitter.com/bouvyd/status/1116033088553914368

@fpodoo

This comment has been minimized.

Copy link
Contributor

commented Apr 11, 2019

Evolution of the task:
1/ we'll use the "allowed companies" of the user to decide what he can see, instead of relying on a hierarchy of companies.
2/ what he actually sees will be defined by the context (multi-tab), and available in url (for when you copy / paste records links)
3/ the default will be to propose one company be défaut, but the user will be able to multi-select in his allowed companies.
4/ parent_id on companies is removed as it does not reflect the reality (a company has multiple shareholders, not one), and does not help for security (which depend on the people)
5/ most importantly, we will fix all access rights issues.

@blaggacao

This comment has been minimized.

Copy link
Contributor

commented Apr 11, 2019

Cool. @fpodoo Please kindly receive this sidenote: Accounting wise, sometimes the principal emitting a document can have different attributes in certain contexts within a company. This is relevant for permanent establishments / branch offices within a single Shareholder's entity (=CoA).

For example:

  • Munich branch (municipality income tax 1,5%%)
  • Berlin branch (municipality income tax 1,7%%)

We need be able to distinguish on what branch principal sales have been recorded in order to retrieve this data, while at the same time not setting up another CoA (=Shareholder's entity)

Some people (including me) are resolving this with company hierarchies, which is probably not optimal.
EDIT: Some people probably solve this with analytic accounting, which is neither a canonical solution.

A proper solution is a pending issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.