Merge lp:~openerp-dev/openobject-server/6.1-opw-575655-rha into lp:openobject-server/6.1

[Rifakat Husen (OpenERP)]

Hello,

Changed field attribute to readonly when we provide group in read on field definition.
'test': fields.char('Test', size=64, read=['base.group_sale_salesman'], write=['base.group_sale_manager']),
This field will be grey out for the user who belongs to above read group.

Currently user is not able to change the value if he has read access, it always resets but we can face problem
with on_change(), on_chnage event always take value from user interface so it may raise
problem.

Furthermore, user won't be able to see the field(invisible) if he doesn't belong to read or write
group.

Thanks for your review.

Regards,
Rifakat Haradwala

[lp:~openerp-dev/openobject-server/6.1-opw-575655-rha]

4207. By Rifakat Husen (OpenERP) on 2012-06-12

[FIX] improved fix to check model access security group

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

Hi,

Unfortunately the `read` and `write` attributes on OpenERP fields are an artifact of past experimentation, and never fully functional. They are hidden and undocumented, and none of the official OpenERP modules use them, for this very reason. In fact we should have removed them a while ago, but this was apparently overlooked.

In any case, it is not a good idea to use or depend on them, because they are not part of the system design. This patch may slightly change and possibly improve the way they are used, but they are many more areas where users might experience strange behaviors.

We also cannot allow patching this experimental/deprecated part of the system in a stable release because it is located in a core area, and any patch there will possibly introduce dangerous regressions. As all customers in production are not supposed to be using this, there is no sense in compromising the stability of the system for every customer just because someone had the idea of digging into the code and playing with that experimental part, sorry.

For all of the above reason, we have to reject this merge proposal, even though the patch might be interesting for anyone who would like to develop some custom behavior based on these experimental idea.

I hope you understand this decision...

Thanks!

review: Disapprove

[Vadim - Enapps LTD (vadim-enapps)]

Hi Olivier, thanks for the detailed answer.
All clear, tell me please what is it supposed to be done with the feature requirement. it is very common when we require the operators to see a certain field, but not to write. i.e.:

1 - Sale operator to see the credit terms for the sales order, but not be able to change it as this is a function normally performed by finance only.
2 - Finance personnel to be able to place a stock_picking on hold with a checkbox and warehouse personnel to see the info, but not be able to amend it. It is a feature of pretty much any serious erp system in the world to have the read/write by field. Thanks to Rifakat for the smart pointers - now we know where to look to develop this further, but i definitely see that lack of such a feature limits us in what we can construct with openerp.

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

On 06/13/2012 02:28 PM, Vadim - Enapps LTD wrote:
> Hi Olivier, thanks for the detailed answer. All clear, tell me please what
> is it supposed to be done with the feature requirement. it is very common
> when we require the operators to see a certain field, but not to write.

There is currently no mechanism to accomplish that by default. We might support
this in the future, but nothing is planned yet, as most SMBs will never need
this. When you think about it, you're trying to trust someone to have full
control on a kind of document, let them create/modify/delete the documents, but
not let them touch one of the fields on these documents?! The rationale for
doing this is often flawed.

> i.e.:
>
> 1 - Sale operator to see the credit terms for the sales order, but not be
> able to change it as this is a function normally performed by finance only.

In fact, payment terms are something that is commonly discussed between
Salesmen and Customers in many companies. Certain products such as services may
be associated with different payment terms, for example. In any case, it is the
responsibility of whoever validates the Sale Order to double-check this.
If the Salesman is allowed to validate a Sale Order, then he should be trusted
to choose appropriate terms. If there is a second layer of approval, then let
the supervisor (presumably from Finance) verify the terms before approving.

> 2 - Finance personnel to be able to place a stock_picking on hold with a
> checkbox and warehouse personnel to see the info, but not be able to amend
> it. It is a feature of pretty much any serious erp system in the world to
> have the read/write by field. Thanks to Rifakat for the smart pointers - now
> we know where to look to develop this further, but i definitely see that
> lack of such a feature limits us in what we can construct with openerp.

Similarly, you're trusting warehouse workers to manage pickings,
create/split/deliver them, but you don't trust them to respect a simple "on
hold" workflow flag? This might indicate that the responsibilities are not
properly assigned. If finance personnel are the ones who can put a picking on
hold, on what basis do they do it? Per customer? Per invoice? The flag could be
put on the customer or invoice, and the workflow made to handle it
appropriately on the picking - but the flag should not be located on the
picking itself. In most companies finance personnel would not have access to
pickings.

By applying the same logic everywhere, I think you will not only have a simple
and direct implementation of the business rules, but also avoid unintuitive
user experience.

Now if you really want to do things the hard way, it is very simple to
customize and extend OpenERP, as usual. For example you can have different
views for the same document, one where certain fields are read-only (possibly
even read-only related fields), and one where the master fields are exposed.
You then assign a different menuitem with the appropriate view to each group.
And that's just one way to do it, many other solutions are possible.

Unmerged revisions

4207. By Rifakat Husen (OpenERP) on 2012-06-12

[FIX] improved fix to check model access security group

4206. By Rifakat Husen (OpenERP) on 2012-06-12

[FIX] orm: changed field attribute based on the read and write group assigned on the field
* if group has read access on the field then that field will be greyed out for that user
* if user of the group doesn't belong to read or write then it will be invisible

=== modified file 'openerp/osv/orm.py'
--- openerp/osv/orm.py	2012-06-09 01:05:49 +0000
+++ openerp/osv/orm.py	2012-06-12 14:14:23 +0000
@@ -1598,6 +1598,7 @@
         result = False
         fields = {}
         children = True
+        ir_model_access = self.pool.get('ir.model.access')
 
         modifiers = {}
 
@@ -1610,7 +1611,6 @@
             """ Set invisible to true if the user is not in the specified groups. """
             if node.get('groups'):
                 groups = node.get('groups').split(',')
-                ir_model_access = self.pool.get('ir.model.access')
                 can_see = any(ir_model_access.check_groups(cr, user, group) for group in groups)
                 if not can_see:
                     node.set('invisible', '1')
@@ -1649,6 +1649,25 @@
                     column = False
 
                 if column:
+                    write_groups = []
+                    field_write = False
+                    if column.read:
+                        cr.execute('''SELECT g.id FROM ir_model_access a JOIN
+                         ir_model m ON (a.model_id=m.id) JOIN res_groups g ON
+                         (a.group_id=g.id) WHERE m.model=%s AND a.perm_''' + 'write', (self._name,))
+                        groups = cr.fetchall()
+                        group_ids = map(lambda x:x[0] , groups)
+                        cr.execute("select count(*) from res_groups_users_rel where gid IN %s and uid=%s", (tuple(group_ids), user) )
+                        model_write_access = cr.fetchall()
+                        if column.write:
+                            field_write = any(ir_model_access.check_groups(cr, user, group) for group in column.write)
+                        field_see = any(ir_model_access.check_groups(cr, user, group) for group in column.read)
+                        if field_see and not field_write and model_write_access[0][0]==0:
+                            node.set('readonly', '1')
+                            modifiers['readonly'] = True
+                        if not (field_see and field_write) and model_write_access[0][0]==0:
+                            node.set('invisible', '1')
+                            modifiers['invisible'] = True
                     relation = self.pool.get(column._obj)
 
                     children = False