Merge lp:~openerp-dev/openobject-server/6.0-opw-10420-ira into lp:openobject-server/6.0

[Ila Rana(Open ERP)]

Using point_of_sale module we can't open cash register if journal user belonging to parent company and journal sequence have child company. so to access child_company journal I have made changes in ir_sequance.py file. So if user have parent company and journal sequence belonging to child company he can easily access.

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

Hi,

I understand that it makes sense to allow a user to increment sequences in other companies if he is currently allowed to "see" these other companies.
Indeed, the typical case is when the user is in the parent company, and wants to use a sequence from a child company.
However, the rule to know what companies a user is currently allowed to see is not hardcoded, it is not "current or children", it is defined by the ir.rules on the Company object.

Therefore, don't you think a better way to determine the list of allowed companies is to do a normal res_company.search([]) as the current user?
i.e. simplify the code with just:
  visible_company_ids = self.pool.get('res.company').search(cr, uid, [], context=context)

and pass "tuple(visible_company_ids)" to the SQL query.

What do you think?

Thanks!

review: Needs Information

[Jay Vora (Serpent Consulting Services) (jayvora)]

Agreed!

Will have some more tests and update you, thanks for the time!

[lp:~openerp-dev/openobject-server/6.0-opw-10420-ira]

3457. By Ila Rana(Open ERP) on 2011-07-04

[FIX]:cash_register_can_not_open(case:10420)

[Ila Rana(Open ERP) (ira-openerp)]

I have improved get_id function of ir_sequence as per Olivier's suggestion.

Thanks Olivier and Jay for your suggestions,
Ila

review: Needs Resubmitting

[Leonardo Pistone (lepistone)]

Hi Jai, Ira, Olivier,

don't know if that's the right place to comment as it's merged already... anyway, that broke a customer of mine.

That is the scenario: there are companies A and B, with no parent-child relationship, and all rules are set so that everyone can see data for the current company only.

Some users can switch companies, and to be able to do that they have both A and B in their res.user company_ids.

every company has their own sequences, so when for example I make a new sale order, that should be from the current company, as it was before that merge. Now sequence numbers come from anyone of the companies_ids, which (at least in that case) is clearly wrong.

Thanks!

review: Disapprove

[Ila Rana(Open ERP) (ira-openerp)]

Leonardo,

Sorry for any inconvenience.

Record rules are meant to be worked for multi-company architecture which has companies' parent-child relationship.

If you have your companies in parallel, the sequence won't be working for you.

For your aim, you can simply do this:
company_ids += self.pool.get('res.users').browse(cr, uid, uid).company_ids

This will allow the access to ALLOWED companies.

As this is not a general case, I afraid we won't be able to do on stable!

Hope this helps you.

Thanks.

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

@Ila: I think you misunsderstood Leonardo's use case, no code change
should be necessary, if I understand correctly his explanation.

On 07/11/2011 10:42 AM, Leonardo Pistone - Agile BG - Domsense wrote:
> don't know if that's the right place to comment as it's merged
> already... anyway, that broke a customer of mine.

Hello Leonardo,

It's a sensible place to discuss it, yes :-)

> That is the scenario: there are companies A and B, with no
> parent-child relationship, and all rules are set so that everyone can
> see data for the current company only.
>
> Some users can switch companies, and to be able to do that they have
> both A and B in their res.user company_ids.
>
> every company has their own sequences, so when for example I make a
> new sale order, that should be from the current company, as it was
> before that merge.

This sounds like a valid use case, that is supported by our current
implementation. Supporting this kind of setup is in fact the reason
for my first comment on this merge prop: I asked *not* to hardcode the
rule about parent-child companies in the code, but to rely on the rules
defined by the administrator.

So if you make sure that the rules of your database only permit seeing
the sequences of the current user company, you should get the result you
expect, because the search() at l.9 of the diff will only return the
current company sequence.
You're saying that all your rules only allow seeing current company
data, just make sure it's the same for ir.sequence too.
BTW, don't test this with the admin (ID=1) account, because it bypasses
all security rules, and should never be used for business flows! (we
should find a way to make this obvious to everyone, I think many are
using admin for everyday transactions)

Hope this helps..

[Leonardo Pistone (lepistone)]

Olivier,

thanks for your reply. A perfectly agree with you when you say

> So if you make sure that the rules of your database only permit seeing
> the sequences of the current user company,

i.e. i'd be very happy to set up a rule on ir.sequence for my needs.

The problem is that in that the l.9 of the diff says

company_ids = self.pool.get('res.company').search(cr, uid, [], context=context)

that uses the record rules of the company object, not the ir.sequence object. I think that is wrong.

My proposal is to forget all SQL here and just do a simple self.pool.get('ir.sequence').search

That would follow rules for ir.sequence, and everyone would be able to set them up according to their needs.

Thanks!

review: Needs Fixing

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

On 07/11/2011 12:14 PM, Leonardo Pistone - Agile BG - Domsense wrote:
>> So if you make sure that the rules of your database only permit
>> seeing the sequences of the current user company,
>
> i.e. i'd be very happy to set up a rule on ir.sequence for my needs.
>
> The problem is that in that the l.9 of the diff says
>
> company_ids = self.pool.get('res.company').search(cr, uid, [],
> context=context)
>
> that uses the record rules of the company object, not the ir.sequence
> object. I think that is wrong.

Oops, I apologize to Ila and Leonardo, I meant to say that it should
work if you setup a rule on *res.company* that only allows the user to
see her own current company! In that case line 9 will only select the
current user company.
And this rule on res.company does not prevent switching to a different
company (if you have several company_ids), as the user preferences
specifically allow you to see all your company_ids.

This certainly seems consistent with your database rules: it explicitly
prevents the user from selecting an incompatible company on any document
-> the company selection field will only ever show one.

> My proposal is to forget all SQL here and just do a simple
> self.pool.get('ir.sequence').search
>
> That would follow rules for ir.sequence, and everyone would be able
> to set them up according to their needs.

I agree that it would be simpler and more intuitive.
The reason for the SQL here is the "FOR UPDATE NOWAIT" clause, that
implements the proper serialization of sequence increments, and is
definitely required.
But perhaps we could still improve this with the following:
- add an ir.rule on ir.sequence (like all defaults, filters on current
and child companies)
- use ir.sequence.search() to find the visible sequences for the current
user (limit=1) + read/browse to get the sequence data
- then do a "SELECT FOR UPDATE NOWAIT" on the selected sequence, to
implement the proper locking mechanism
- and finally do the update of the sequence (could be done with write())

This would make it more intuitive during the configuration of
multicompany setups, but it will require an update of 'base' during
upgrade, to make sure the rule on ir.sequence is created.

Jay, Ila, what do you think of this proposal? Is it perhaps too much for
stable, and should go in trunk only?

Leonardo, even if we perform this change, you should still have an
appropriate rule for res.company, to make the security policy
consistent. So regardless of what we do, if you have consistent rules on
both res.company and ir.sequence, you'll be safe.

Does it sound right (this time ;-))?

[Leonardo Pistone (lepistone)]

Olivier,

what you say sounds reasonable to me... still, in my situation, if I revert to the default rule for res.company, that is: [('id','child_of',[user.company_id.id])] , I cannot switch companies.

The only way that I found to be able to switch companies is to set the res.company rule [('id','in',[x.id for x in user.company_ids])].

Am I missing something?

Thanks a lot,

Leo

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

On 07/11/2011 04:26 PM, Leonardo Pistone - Agile BG - Domsense wrote:
> The only way that I found to be able to switch companies is to set
> the res.company rule [('id','in',[x.id for x in user.company_ids])].
>
> Am I missing something?

How are you switching companies? The user preferences panel has a
special system to display the list of user.company_ids, regardless of
the security rules, for this very purpose.
I just checked on a 6.0 system (latest server and addons versions) and I
can switch between companies with any user (who belongs to multi
companies group), even if I set the res.company rule to something that
does not match any company. And this works also if the companies I
belong to are not related (no ancestor/descendant relationship between
them).

Technically, this works because res.company has an overridden _search()
method that checks the context for a special key, and that special key
is passed by the res_users.company_id field, so it is used when loading
the selectable companies in the user preferences.

If this doesn't work for you, can you investigate and perhaps report a
bug if you find that it is reproducible with official addons?

[Leonardo Pistone (lepistone)]

Olivier, I didn't know about the overridden res_company search before. Thanks for your explanation! I did some tests, and the scenario you describe works in the gtk but not in the web client, so I posted https://bugs.launchpad.net/openobject-client-web/+bug/809178.

Thanks!

=== modified file 'bin/addons/base/ir/ir_sequence.py'
--- bin/addons/base/ir/ir_sequence.py	2010-12-10 22:42:58 +0000
+++ bin/addons/base/ir/ir_sequence.py	2011-07-04 07:01:37 +0000
@@ -75,15 +75,15 @@
 
     def get_id(self, cr, uid, sequence_id, test='id', context=None):
         assert test in ('code','id')
-        company_id = self.pool.get('res.users').read(cr, uid, uid, ['company_id'], context=context)['company_id'][0] or None
+        company_ids = self.pool.get('res.company').search(cr, uid, [], context=context)
         cr.execute('''SELECT id, number_next, prefix, suffix, padding
                       FROM ir_sequence
                       WHERE %s=%%s
                        AND active=true
-                       AND (company_id = %%s or company_id is NULL)
+                       AND (company_id in %%s or company_id is NULL)
                       ORDER BY company_id, id
                       FOR UPDATE NOWAIT''' % test,
-                      (sequence_id, company_id))
+                      (sequence_id, tuple(company_ids)))
         res = cr.dictfetchone()
         if res:
             cr.execute('UPDATE ir_sequence SET number_next=number_next+number_increment WHERE id=%s AND active=true', (res['id'],))