Merge lp:~therp-nl/openerp-web/lp1179839 into lp:openerp-web/7.0

Proposed by Holger Brunn (Therp)
Status: Approved
Approved by: Xavier (Open ERP)
Approved revision: 3922
Proposed branch: lp:~therp-nl/openerp-web/lp1179839
Merge into: lp:openerp-web/7.0
Diff against target: 18 lines (+7/-1)
1 file modified
addons/web/static/src/js/view_form.js (+7/-1)
To merge this branch: bzr merge lp:~therp-nl/openerp-web/lp1179839
Reviewer Review Type Date Requested Status
Xavier (Open ERP) (community) Approve
Holger Brunn (Therp) (community) Needs Resubmitting
Review via email: mp+163659@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Xavier (Open ERP) (xmo-deactivatedaccount) wrote :

This will break the case for which this very check was added, quoting the commit message:

m2m lists inherit (from listview/view) the handling of access rights
attributes (e.g. @create, @delete) in which the access rights to the
related model are those checked for the view. This is generally true,
but *not* for m2ms: even if a user has no creation rights to the
related model, he can still create a *relation* between the current
and related models.

The m2m access rights are really governed by the *current* (source)
model, in which case the user won't get to see an "editable" view of
the m2m in the first place.

So just override is_action_enabled to disable it in m2ms.

tl;dr: access rights attributes of m2m are set based on rights to the object linked to, but this makes no sense because even if you can't create an object you can still create a link between current and (existing) linked.

review: Disapprove
Revision history for this message
Holger Brunn (Therp) (hbrunn) wrote :

Well, that's true of course.

But is_action_enabled returns the value of the corresponding attribute in the view definition, defaulting to true if not set otherwise explicitly by a developer. So how can this break anything?

Revision history for this message
Xavier (Open ERP) (xmo-deactivatedaccount) wrote :

If the user doesn't have write access to the model the m2m links to, the attributes will be present and false (because it's based solely on the rights to the model) even though the user will expect, be expected to and should be able to create or destroy *relations*.

Consider users and groups: even if a user doesn't have the right to *create* (or delete, or edit) groups, he may still be able to add or remove (existing) groups to and from users.

Revision history for this message
Holger Brunn (Therp) (hbrunn) wrote :

I just made a test with a user without create rights for users. Opened a group with it and its user_ids' many2many attrs has none of those attributes set. Are you talking about a feature in current trunk maybe? But also there I don't see any difference.

The original version of is_action_enabled does that:

is_action_enabled: function(action) {
        var attrs = this.fields_view.arch.attrs;
        return (action in attrs) ? JSON.parse(attrs[action]) : true;
    }
http://bazaar.launchpad.net/~openerp/openerp-web/7.0/view/head:/addons/web/static/src/js/views.js#L1464

so I don't see how that changes anything for an empty attrs?

Revision history for this message
Xavier (Open ERP) (xmo-deactivatedaccount) wrote :

> Are you talking about a feature in current trunk maybe? But also there I don't see any difference.

No it was an example predicated on a user having write access to users but not groups, e.g. a team leader who could add access rights to his team members but not create groups himself. OpenERP does not set that up by default but it makes sense as far as I can think.

> so I don't see how that changes anything for an empty attrs?

No idea why you're mentioning "empty attrs".

Revision history for this message
Holger Brunn (Therp) (hbrunn) wrote :

I talk about empty attrs (meaning, no create/delete/edit properties in attrs) because this is what you end up with if you don't define this attributes on your form/tree view. Then there is no difference between the override I would like to delete in this MP (which breaks setting those attributes on many2many fields) and the one defined in forms.js. I would like to understand which check you think the original function breaks.

To sum up: I think the function is_action_enabled only is about the UI telling whether or not the developer writing a form wants to have certain buttons there, which has nothing to do with the underlying access rights. Do we agree on that? And if so, why do we still need to disable that for m2m fields?

Revision history for this message
Xavier (Open ERP) (xmo-deactivatedaccount) wrote :

> I talk about empty attrs (meaning, no create/delete/edit properties in attrs) because this is what you end up with if you don't define this attributes on your form/tree view.

No, that's what you end up with if you don't define the attribute *and
the user has these access rights to the corresponding model*.

> I would like to understand which check you think the original function breaks.

I've told you: if you have an M2M between two objects A and B, and the
user has c/u/d access to A but not to B the attributes will be set (to
false) on the view for B inside the form of A. Even though the user is
perfectly able to create *links* between A and B objects, he just isn't
able to create (or delete or edit) B objects.

> I think the function is_action_enabled only is about the UI telling whether or not the developer writing a form wants to have certain buttons there

You think wrong.

> Do we agree on that?

No, we most definitely do not, the ability for view writers to override
these attributes was added as a side-benefit but their original user
case is to indicate to the client that the current user does not have
create, write and/or delete right on the model without an extra request:
http://bazaar.launchpad.net/~openerp/openobject-server/7.0/revision/4393#openerp/osv/orm.py

lp:~therp-nl/openerp-web/lp1179839 updated
3922. By Holger Brunn (Therp)

[FIX] avoid misbehavior on lacking user rights on the object
needs lp:~therp-nl/openobject-server/lp1179839

Revision history for this message
Holger Brunn (Therp) (hbrunn) wrote :

Thanks for the pointer to the code, now I get it.

And it also explains why I didn't run into that: I used record rules to forbid creating object B to myself.

Anyways, I think it's very unfortunate not to have this feature for many2many fields, control over those buttons is an important key to creating a smooth user experience.

With a small change in the server: http://bazaar.launchpad.net/~therp-nl/openobject-server/lp1179839/revision/4976 and the code updated as I did in this branch, we should both be happy.

What do you think?

review: Needs Resubmitting
Revision history for this message
Xavier (Open ERP) (xmo-deactivatedaccount) wrote :

I believe it should, though it really feels like piling hacks upon hacks (not that this is anything new). I'll mark this proposal as approved, conditional on the merging of the other one in the mainline server.

Revision history for this message
Xavier (Open ERP) (xmo-deactivatedaccount) :
review: Approve

Unmerged revisions

3922. By Holger Brunn (Therp)

[FIX] avoid misbehavior on lacking user rights on the object
needs lp:~therp-nl/openobject-server/lp1179839

3921. By Holger Brunn (Therp)

[FIX] don't overwrite is_action_enabled in Many2ManyListView

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'addons/web/static/src/js/view_form.js'
2--- addons/web/static/src/js/view_form.js 2013-04-17 13:34:38 +0000
3+++ addons/web/static/src/js/view_form.js 2013-05-17 14:21:27 +0000
4@@ -4366,7 +4366,13 @@
5 });
6 }
7 },
8- is_action_enabled: function () { return true; },
9+ is_action_enabled: function () {
10+ if(this.embedded_view) {
11+ return this._super.apply(this, arguments);
12+ } else {
13+ return true;
14+ }
15+ },
16 });
17
18 instance.web.form.FieldMany2ManyKanban = instance.web.form.AbstractField.extend(instance.web.form.CompletionFieldMixin, {