[Trunk/7.0] can not validate invoice in multi-company

[Nicolas Bessi - Camptocamp]

Hello,

When I tried to validate an invoice in multi company mode I have an exception "Can not read company".

The exception is risen by this line:
company_currency = inv.company_id.currency_id.id in account_invoice.py => action_move_create

The mutli company setting are correct (It will be proven below) but what is strange is that in pdb:

pp inv.company_id
  browse_record(res.company, 3)

It is ok but:

pp inv.company_id.name
2013-01-31 09:57:31,983 8617 WARNING None openerp.osv.orm: Access Denied by record rules for operation: read, uid: 5, model: res.company ..

It raise exception.

More fun :

pp self.pool['res.company'].browse(cr, uid, 3).currency_id.name
  u'EUR'

No exception…

This one is tricky isnt'it ?

Regards

Nicolas

[Nicolas JEUDY (njeudy)]

I think it's tricky to reproduce. :)
Is it a rapid way to setup and reproduce this from a new database ?

Think uid is not correctly passed through metod() ...

[Nicolas Bessi - Camptocamp (nbessi-c2c-deactivatedaccount)]

The uid is he same as the one in the beginning of signature and the same that is used to browse invoice.

Well to reproduce this you have to create a database with many companies.
You create one user per company that is accountant and authorized only for his company.
The you try to create a invoice with a user from the company that is not the main one (the one with base.main_company xml id).
When you validate the invoice it should break.

Regards

Nicolas

[Nicolas Bessi - Camptocamp (nbessi-c2c-deactivatedaccount)]

A workaround for the bug. It does not fix root cause but while waiting will allows people to work:

- company_currency = inv.company_id.currency_id.id
+ # workaround of bug no 1111298
+ company_currency = self.pool['res.company'].browse(cr, uid, inv.company_id.id).currency_id.id
+ # company_currency = inv.company_id.currency_id.id

Regards

Nicolas

[Amit Parik (amit-parik)]

Hello Nicolas,

You are correct faced the same problem at my end and attached the scrren shot.

Thank you!

[Nicolas Bessi - Camptocamp (nbessi-c2c-deactivatedaccount)]

Ok I have identified the source of the bug.

By default there is no ir_rule on res.currency but there is a default company_id set on res.currency that is the base.main_company id.

When you have a browse of res.currency from time to time the browse will do a read_flat and will ty to load the value of the related company that is protected by it's own ir.rule.

I do not know what is the best solution, put a rule on res.currency to be coherent with the rest of the models, or modify the creation of res.currency in order not to have company_id on it ?

Regards

Nicolas

[Nicolas Bessi - Camptocamp (nbessi-c2c-deactivatedaccount)]

Hello,

Ok my analysis was wrong. It was working by pure coincidence.

In fact when you do an inv.company_id.xxx we have a problem.

The point is the following
pp inv._data

=> {4: {'origin': False, 'text_condition2': <openerp.osv.orm.browse_null object at 0x5ec1510>, 'date_due': '2013-02-25', 'check_total': 0.0, 'partner_bank_id': <openerp.osv.orm.browse_null object at 0x5ec1110>, 'supplier_invoice_number': False, 'number': False, 'company_id': browse_record(res.company, 4), 'currency_id': browse_record(res.currency, 1), 'text_condition1': <openerp.osv.orm.browse_null object at 0x5ec1810>, 'partner_id': browse_record(res.partner, 11), 'id': 4, 'fiscal_position': browse_record(account.fiscal.position, 12), 'user_id': browse_record(res.users, 6), 'reference': False, 'note1': False, 'tax_line': [browse_record(account.invoice.tax, 20), browse_record(account.invoice.tax, 19)], 'payment_term': <openerp.osv.orm.browse_null object at 0x5ec10d0>, 'reference_type': u'none', 'journal_id': browse_record(account.journal, 18), 'amount_tax': 0.0, 'state': u'draft', 'type': u'out_invoice', 'sent': False, 'internal_number': False, 'account_id': browse_record(account.account, 4488), 'reconciled': False, 'residual': 0.0, 'move_name': False, 'date_invoice': False, 'note2': False, 'period_id': <openerp.osv.orm.browse_null object at 0x5ec0f90>, 'amount_untaxed': 50.0, 'move_id': <openerp.osv.orm.browse_null object at 0x5ec0dd0>, 'amount_total': 50.0, 'name': False, 'comment': False, 'invoice_line': [browse_record(account.invoice.line, 5)]}}

comp = inv.company_id
pp comp
=> browse_record(res.company, 4)
pp comp.data
=>{1: {'id': 1}, 4: {'id': 4}}

Here we can see the data are incorrect. I am not browsing the company_id 1. The id 1 is the id of the currency, but when __get_item__ is called it tries to read the company 1 that is not in allowed companies.

Regards

Nicolas

[Olivier Dony (Odoo) (odo-openerp)]

Nicolas, your analysis in comment #6 is correct, and what you describe in comment #7 is just the expected behavior for the caching/prefetching of browse_records. It simply happens to trigger the error here because of the problems described in comment #6.

I think the solution is the following:

1. Add a proper record rule on res.currency to make it consistent with the rest of the system.
2. Change the default value of the company_id field of res.currency to False
3. Apply the small change described in comment #3 to make sure the problem is fixed for existing databases where existing currencies already have a value for company_id. (it will not break anything)

Users who encounter this error should also remove the assigned Company for their existing currencies, to prevent future problems.

Thanks for reporting and providing an excellent analysis!

[Nicolas Bessi - Camptocamp (nbessi-c2c-deactivatedaccount)]

Thank for you reply.

My only fear about your fix is that it means that we may not be able to freely adapt multi company rules to fit customer needs as this patch is only scoped around currency.
What if I want to share catalogue, price etc… ?

Maybe I have a wrong understanding of the problem. Can you please confirm that point please.

Regards

Nicolas

[Olivier Dony (Odoo) (odo-openerp)]

There should be nothing specific to fear when defining custom multi-company rules, as long as you keep them consistent.

The error we saw here results from mixing records with inconsistent multi-company rules in the *same* browse_record context/cache. This must never happen in normal situations: first, because we re-browse records all the time when we switch methods or logical context, so there are little side-effects; secondly, because the record rules are normally consistent, so the cache should only ever contain a consistent set of records.
Here the inconsistency was rather large: users were not allowed to see companies they didn't belong to, but they were allowed to see currencies that belonged to other companies; it was quite easy to obtain a mess of mixed records that did not belong with each other.

Fixing the inconsistent default settings is really all there is to it (point 1 and 2 in comment #8) - and it only concerns currencies because it's the only case where we detected an obvious inconsistency.
Point 3. is an extra nice-to-have to smoothly cover existing databases that can live with the inconsistency but need to be able to validate invoices for a company that is not the main/first company.

Finally, keep in mind that the issue could be fixed quite easily in the database by correcting the inconsistent rules and/or the incorrect company assignation on currencies... That will always be possible if someone happens to configure an inconsistent multi-company environment.

[Nicolas Bessi - Camptocamp (nbessi-c2c-deactivatedaccount)]

Hello,

Thanks for your answer, but there is a point that is not totally correct . The bug appeared even when company_id was set to False on currency object… That means that user want to access currencies resources that where shared and do not belong to any company, so it appears to be a correct setup. That why that bug was so annoying.

But now we are at least aware of this potential cache glitch. I will try to take the orm patch and adapt it to have a more efficient filter done on Python size.

[Alexandre Fayolle - camptocamp (alexandre-fayolle-c2c)]

Hello

What is the current status of this bug?

[Alexandre Fayolle - camptocamp (alexandre-fayolle-c2c)]

I had to use the attached patch to be able to validate an invoice in multi company mode.

There are 3 other occurences of "inv.company_id.currency_id" in account_invoice.py which worries me a bit.

[Alexandre Fayolle - camptocamp (alexandre-fayolle-c2c)]

Ok, so I've found the patches Chris proposed through OPW but did not bother proposing as a patch here, and included them (with a slight improvement) here.

Please review and apply.

[Olivier Dony (Odoo) (odo-openerp)]

@Alexandre: thanks for reviewing Chris' patches, improving them and making the MP. They'll be merged after a final validation on runbot.

[Alexandre Fayolle - camptocamp (alexandre-fayolle-c2c)]

@odo-openerp thanks for taking care of this.

[Olivier Dony (Odoo) (odo-openerp)]

Alexandre's merge proposals were merged in 7.0 at the following revisions:
- addons 7.0: rev.9139 rev-id: <email address hidden>
- server 7.0: rev.4977 rev-id: <email address hidden>

The merge was not 100% trivial because some of the tests in the framework depended on the res.currency demo data in a way that was broken after the server patch, so an extra fix was required.

Thanks for reporting, testing the patch and following up until the final merge!

[Stefan Rijnhart (Opener) (stefan-opener)]

Reopening, we still get the problem because of the company assignment to EUR in base_data.xml.

[Olivier Dony (Odoo) (odo-openerp)]

The remaining issue detected by Stefan was fixed after merging his patch in server 7.0 at revision 4998 (rev-id: <email address hidden>).
Stefan, thanks for noticing it and providing the fix!

[Tung Tran Thanh (tung121089)]

Hello,

Please try to read the name of the currency from the invoice like this: inv.company_id.name. The problem is still there!

I think the fix does not address the main issue of this bug, which was described very precisely in the comment #7 by Nicolas Bessi. For me, it's not what described in the comment #6 that triggers the error in the cache function. The main issue here is at the way that ORM is handling the caching function.

After applying the fix, I still get an error why trying to call inv.company_id.name.

Take a look at the fix, which was proposed in the comment #8.

I agree with the first two points, which are adding a record rule for res.currency and updating company_id to False for all currencies, to make res.currency being handled consistently with the rest of the system. However, this bug will never gone without taking into account the third point, which is just a workaround for this specific case.

Correct me if I'm wrong!

Regards,

[Serpent Consulting Services (serpent-consulting-services)]

Guys,

This originates a bug even my user is allowed to see multiple currencies.

I cannot print a sale order report and it shows me blank prices. Problem is specially when I have digits=get_digits(dp='Account').

This rule should not be on 'read' operation.

Thanks.

[Agustín (atin81)]

I'm agree with #20, the main problem is the way ORM manages the cache data. I'm getting problems on a multi company environment used with Point of Sale and taxes configured to be included on product price.

Problem is on account.py on compute_all function

        if taxes and taxes[0].company_id.tax_calculation_rounding_method == 'round_globally':
            tax_compute_precision += 5

when calling __get_item__ for get the tax_calculation_rounding_method server is checking read permission for companies {1: {'id': 1}, 4: {'id': 4}}