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

[MIG] pos_access_right: Migration to 12.0 #302

Merged
merged 14 commits into from Nov 5, 2018

Conversation

Projects
None yet
9 participants
@GabbasovDinar
Copy link
Member

GabbasovDinar commented Oct 29, 2018

No description provided.

@oca-clabot

This comment has been minimized.

Copy link

oca-clabot commented Oct 29, 2018

Hey @GabbasovDinar, thank you for your Pull Request.

It looks like some users haven't signed our Contributor License Agreement, yet.
You can read and sign our full Contributor License Agreement here: http://odoo-community.org/page/cla
Here is a list of the users:

Appreciation of efforts,
OCA CLAbot

@legalsylvain

This comment has been minimized.

Copy link
Contributor

legalsylvain commented Oct 29, 2018

Hi @GabbasovDinar. Thanks for porting this module. LGTM to me. (code review / No test).
could you remove readme file and create new one, based on new OCA convention ? Ref : https://github.com/OCA/maintainer-tools/tree/master/template/module/readme
(not a blocking point for me)
kind regards.

@pedrobaeza pedrobaeza added this to the 12.0 milestone Oct 29, 2018

@yelizariev

This comment has been minimized.

Copy link
Member

yelizariev commented Oct 29, 2018

Planned further updates are described here: #232 (comment)

@legalsylvain

This comment has been minimized.

Copy link
Contributor

legalsylvain commented Oct 29, 2018

I have an idea of improving the pos_access_right module. We can add new fields to res.groups (pos_discount, pos_change_unit_price and etc.). Each pos.config has many2many field to res.groups (one POS may have different access groups, for example, POS manager, POS Waiter, POS Cashier and etc). After loading the POS the user will have access rights that intersect with the rights from the pos.config and res.groups for the user. For example, the Admin belongs to POS-manager group, the Demo belongs to POS-user group, both these groups are specified in POS Config, then when changing the user in the POS, each user will have their own rights.

LGTM. Maybe a little more complex, but indeed it can fit with uncovered needs. Just one thing, about the pos.config.new_field_group_ids, please set all groups by default to avoid extra settings, when installing the module or creating a new pos.config.

Also I think that we could reduce python code of pos.config, remove all the group_xxx_id fields and loading instead the model "res.groups" in the point of sale. What do you think ?

@GabbasovDinar

This comment has been minimized.

Copy link
Member Author

GabbasovDinar commented Oct 29, 2018

Also I think that we could reduce python code of pos.config, remove all the group_xxx_id fields and loading instead the model "res.groups" in the point of sale. What do you think ?

I think we can load all fields of res.groups model using

    models.load_models({
        model: 'res.groups',
        fields: [],
        ids: function(self) {
              return [self.config.access_right_groups_ids]
        },
        loaded: function(self, groups) {
            self.groups = groups;
        },
    });

where self.config.access_right_groups_ids is many2many field to res.groups for pos.config

Cc: @legalsylvain

@legalsylvain

This comment has been minimized.

Copy link
Contributor

legalsylvain commented Oct 29, 2018

Yes ! something like that ! the unique question I'm asking me is how to know which group is "allow_discount". I wrote this python code, to call self.env('xml_id'). have you an idea how to have the xml_id in JS ?

@yelizariev

This comment has been minimized.

Copy link
Member

yelizariev commented Oct 29, 2018

@legalsylvain if I understand your question correctly, the answer is: checking wether "allow_discount" is in groups is just _.any(self.groups, function (gr) { return "allow_discount" in gr})

@legalsylvain

This comment has been minimized.

Copy link
Contributor

legalsylvain commented Oct 30, 2018

@legalsylvain if I understand your question correctly, the answer is: checking wether "allow_discount" is in groups is just _.any(self.groups, function (gr) { return "allow_discount" in gr})

but I'm asking me if allow_discount that is an xml_id will be loaded in JS part. load_models only load data (id, name, etc...) not xml) But maybe I'm wrong !
@GabbasovDinar, did you tested if it works ?
regards.

@GabbasovDinar

This comment has been minimized.

Copy link
Member Author

GabbasovDinar commented Oct 30, 2018

As I wrote earlier

We can add new fields to res.groups (pos_discount, pos_change_unit_price and etc.).

the allow_discount a new field to the res.groups model, it's not xml_id so this field will be loaded

Cc: @legalsylvain

@legalsylvain

This comment has been minimized.

Copy link
Contributor

legalsylvain commented Oct 30, 2018

We can add new fields to res.groups (pos_discount, pos_change_unit_price and etc.).

Ok ! I understand better this sentence.

Well, not sure it is the best design. (to add such new fields in all res.groups table, just for that feature.
Maybe, we could alternatively.

  • on load_models, add a context key : "load_xml_id=True".
  • on the search_read function, check if this key is present, and if yes, return the xml_ids.

what do you think ?

@yelizariev

This comment has been minimized.

Copy link
Member

yelizariev commented Oct 30, 2018

Well, not sure it is the best design. (to add such new fields in all res.groups table, just for that feature.

That is my concern too, but I don't see a better design.

In our idea we want to have minimal records res.groups (e.g. Manager, User), while xml approach is to have group per each feature (pos_discount, pos_change_unit_price and etc.). What I don't like is that we hardcode xmls of groups; also, removing any of those groups means removing the feature and only way to restore it is Upgrading the module.

@legalsylvain

This comment has been minimized.

Copy link
Contributor

legalsylvain commented Oct 30, 2018

In our idea we want to have minimal records res.groups (e.g. Manager, User), while xml approach is to have group per each feature (pos_discount, pos_change_unit_price and etc.).

I'm not sure to be agree with such approach. I think that having the possibility to give access to discount, and not to other feature (like change price) per user is the most modular approach. If we base this module only On Manager / user, we will reduce the modularity of the settings. Right ? or did I missed something ?
If you want to save time in recurring settings, the module https://github.com/OCA/server-tools/tree/10.0/base_user_role could be the good one.

What I don't like is that we hardcode xmls of groups; also, removing any of those groups means removing the feature and only way to restore it is Upgrading the module.

Well, xml_id are hardcoded in all Odoo. So, deleting a group is not a valid action in odoo. All odoo features will break. (sale, purchase, etc...) not only that module.

@yelizariev

This comment has been minimized.

Copy link
Member

yelizariev commented Oct 31, 2018

. If we base this module only On Manager / user, we will reduce the modularity of the settings. Right ? or did I missed something ?

No, for customization one can create new Group with Discount access only, for example.

Well, xml_id are hardcoded in all Odoo. So, deleting a group is not a valid action in odoo. All odoo features will break. (sale, purchase, etc...) not only that module.

It's not strictly the same. If you remove Sale/Manager group, you can restore it by recreating manually with necessary access rules (though, it's bad UX anyway). There are not so much examples in odoo when xml_id is used directly.

@yelizariev

This comment has been minimized.

Copy link
Member

yelizariev commented Oct 31, 2018

We will need someone else review on the approaches.
@blaggacao #232 (comment) ?
@pedrobaeza #232 (comment) ?

@GabbasovDinar

This comment has been minimized.

Copy link
Member Author

GabbasovDinar commented Oct 31, 2018

@legalsylvain yes, I agree

@legalsylvain

This comment has been minimized.

Copy link
Contributor

legalsylvain commented Oct 31, 2018

Is the PR ready to review ?

@GabbasovDinar

This comment has been minimized.

Copy link
Member Author

GabbasovDinar commented Oct 31, 2018

@legalsylvain yes, you can check the PR

@yelizariev

This comment has been minimized.

Copy link
Member

yelizariev commented Oct 31, 2018

Créating a group, that defines subgroup access is really the purpose of the module base_user_role. I think the new design proposed is reinventing a little bit the wheel.

I meant that if a user is not satisfied with predefined levels of access (Manager and User only), the user can create new groups and use them instead of default ones. It's not the question of making subgroups. But thank you for referencing base_user_role module -- I didn't know that

var _set_cashier_ = models.PosModel.prototype.set_cashier;
models.PosModel.prototype.set_cashier = function(user) {
if (user.group_id) {
this.gui.display_access_right(user);

This comment has been minimized.

Copy link
@chienandalu

chienandalu Oct 31, 2018

Contributor

Testing in runbot, when you change the cashier, it doesn't reload the permissions so the button classes are removed or added accordingly. Isn't the object attribute groups_id?

This comment has been minimized.

Copy link
@GabbasovDinar

GabbasovDinar Oct 31, 2018

Author Member

oops, yes, you right

@oca-clabot

This comment has been minimized.

Copy link

oca-clabot commented Oct 31, 2018

Hey @GabbasovDinar,
We acknowledge that the following users have signed our Contributor License Agreement:

Appreciation of efforts,
OCA CLAbot

@GabbasovDinar

This comment has been minimized.

Copy link
Member Author

GabbasovDinar commented Oct 31, 2018

@chienandalu
Copy link
Contributor

chienandalu left a comment

Great! Now it works 🙂

Review js lint https://travis-ci.org/OCA/pos/jobs/448792778#L601 to minimize warnings as well

'category': 'Point Of Sale',
'summary': 'Point of Sale - Extra Access Right for certain actions',
'author': 'La Louve, GRAP, Odoo Community Association (OCA)',
'website': 'http://www.lalouve.net/',

This comment has been minimized.

Copy link
@chienandalu
@blaggacao

This comment has been minimized.

Copy link

blaggacao commented Oct 31, 2018

@yelizariev Thanks for asking. First I would model the use case as follows:

The subjects "Permissions" needs to be applied to the objects "POS User" and "POS Config".

There are two observations:

  • It's POS scoped
  • There are two axis: User and Config

Now what is the best UX?
I guess because of POS scoped, this configuration should not be exposed in the general ACL configuration. There are two main arguments for it:

  • POS users are generally low tech
  • It's POS scoped

Implementation:
Given the two objects and the UX consideration, I think using plain res.group would not be a perfect congruence. I would seriously consider adding a res.pos.group for this subsystem.

Note on 2 objects:
it's not only for legal reasons, but also in reality devices might be defined as proxy for user roles where user control is too much of a moving target (auxiliary ad-hoc workforce).

EDIT: nowadays, it could be res.iot.group for what it's worth.

@chienandalu

This comment has been minimized.

Copy link
Contributor

chienandalu commented Oct 31, 2018

As I see it, the cashier (res.user) should be the one holding this permissions and the flow is correct as it is now.

Anyway, if a per pos.config restriction is needed, the proper iface_whatever settings should be added to allow to override the user permissions.

Additionally, It'd be great to have a sort of pos.team restricting users allowed in a single pos.config, although maybe that'd be scoped for another module...

@chienandalu
Copy link
Contributor

chienandalu left a comment

  • Can you generate the readme from the parts? (it'll save a commit once this is merged)
  • Can you squash the commits by author/logic?
@legalsylvain
Copy link
Contributor

legalsylvain left a comment

Implementation:
Given the two objects and the UX consideration, I think using plain res.group would not be a perfect congruence. I would seriously consider adding a res.pos.group for this subsystem.

I don't understand the reason to not use res.groups and I see it as providing more complexity for nothing.

So as said before, let's go reviewing the port of this module and make it available for 12. For a refactoring, adding extra models, and co, feel free @blaggacao to open an issue to propose what you imagine, and make the according PR. the initial objective of this PR is "Migrate pos_access_right in V12" (By the way, if you propose a res.pos.group, please set in an extra module, to make it available for other modules, if needed.)

kind regards.

About this PR : 👍 (code review / functional test).

Thanks for porting this module @GabbasovDinar

BTW, @GabbasovDinar, could you squach your commits into a single one, it will make it more easy to review for the other contributors.

@chienandalu, it seems the changes you asked as been done. Is it now ok for you ?

kind regards.

@chienandalu
Copy link
Contributor

chienandalu left a comment

@legalsylvain It's ok for me. Just rebase and squash the commit history is left

@blaggacao

This comment has been minimized.

Copy link

blaggacao commented Oct 31, 2018

Still off topic (on the phone hack):

Anyway, if a per pos.config restriction is needed, the proper iface_whatever settings should be added to allow to override the user permissions.

That!

Additionally, It'd be great to have a sort of pos.team restricting users allowed in a single pos.config, although maybe that'd be scoped for another module...

That! That!

@legalsylvain Agree. Will join with code. However, I'm against excessive feature compartmentalization. Not sure how much that fits OCA standards, maybe some compromise can be achieved

@GabbasovDinar GabbasovDinar force-pushed the GabbasovDinar:12.0-mig-pos_access_right branch from 281302f to 71acf4b Nov 1, 2018

@GabbasovDinar

This comment has been minimized.

Copy link
Member Author

GabbasovDinar commented Nov 1, 2018

[MIG] pos_access_right: Migration to 12.0
[DOC] update README

[FIX] fix variable name

[REF] the pos.config model load all fields

[DOC] update website link

[LINT] fix lint errors

[LINT] fix lint errors

@GabbasovDinar GabbasovDinar force-pushed the GabbasovDinar:12.0-mig-pos_access_right branch from 71acf4b to 1ba7f66 Nov 1, 2018

@legalsylvain

This comment has been minimized.

Copy link
Contributor

legalsylvain commented Nov 1, 2018

@legalsylvain It's ok for me. Just rebase and squash the commit history is left

AFAIK, it's not an OCA convention (just a tecnativa one). Do you have other remarks, or can we merge this PR ?

kind regards.

@chienandalu

This comment has been minimized.

Copy link
Contributor

chienandalu commented Nov 5, 2018

Well, I don't think it's Tecnativa's exclusive convention, but a practical way to keep the essential commit history from branch to branch 😄

@legalsylvain

This comment has been minimized.

Copy link
Contributor

legalsylvain commented Nov 5, 2018

Well, I don't think it's Tecnativa's exclusive convention, but a practical way to keep the essential commit history from branch to branch

Please don't block PR for that non OCA convention. Do you have other remarks on the PR, or is it all OK for you ?

regards.

@chienandalu

This comment has been minimized.

Copy link
Contributor

chienandalu commented Nov 5, 2018

Well, indeed it is an OCA convention: https://github.com/OCA/maintainer-tools/wiki/Merge-commits-in-pull-requests.

Anyway, I won't block this anymore...

@legalsylvain

This comment has been minimized.

Copy link
Contributor

legalsylvain commented Nov 5, 2018

Well, indeed it is an OCA convention: https://github.com/OCA/maintainer-tools/wiki/Merge-commits-in-pull-requests.

It doesn't talk to merge commits from other people (only your commits and transiflex commits) AFAIF. So the @GabbasovDinar PR is OK, regarding OCA convention.

thanks for approving this PR.

regards.

@legalsylvain legalsylvain merged commit 53f9cc4 into OCA:12.0 Nov 5, 2018

5 checks passed

ci/runbot runbot build 3344714-302-1ba7f6 (runtime 349s)
Details
codecov/patch No report found to compare against
Details
codecov/project No report found to compare against
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 56.522%
Details
@pedrobaeza

This comment has been minimized.

Copy link
Contributor

pedrobaeza commented Nov 5, 2018

@legalsylvain because you didn't follow these conventions in that time (because they weren't there on 8.0). But now, there's the opportunity of doing it. If you are so upset with this, don't do it on your PRs, but this is not your PR, and you shouldn't boycott the possibility of doing it right. Please abstent from merging next time.

@legalsylvain

This comment has been minimized.

Copy link
Contributor

legalsylvain commented Nov 5, 2018

@pedrobaeza : AFAIK, the current OCA convention says you should merge your commits, not the commits of all the contributors when cherry picking the history. Right ?

A PR, is not the right place to change OCA conventions. let's talking about that, in https://github.com/OCA/maintainer-tools.

Thanks !

@pedrobaeza

This comment has been minimized.

Copy link
Contributor

pedrobaeza commented Nov 5, 2018

@legalsylvain there's nothing to change, as the question is what I have already commented: you didn't do that squashing on 8.0. Now, it can be done. Then, let's do it. You don't have to do it, but don't block others that are willing to do it (I will, you know).

@blaggacao

This comment has been minimized.

Copy link

blaggacao commented Nov 5, 2018

WTF? 😉 This is overkill...

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.