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

[12.0] Add shopinvader_customer_multi_user #471

Conversation

simahawk
Copy link
Contributor

Includes #469, look at last commit only,

@lmignon @sebastienbeau one more thing we've talked about in LLN.
Specs as we defined are here #454.

@lmignon
Copy link
Collaborator

lmignon commented Oct 23, 2019

Thank you @simahawk once again 'Cristal clear code' ! Looks good to me at first read. I'll test it into the coming days...

default=False, # let's be explicit here :)
help="Turn on this flag to enable multiple users per each customers. "
"Customers will be able to register many users that will use "
"the same data for shipping and invoicing.",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@simahawk We should explain that the multi user is by company....

@lmignon
Copy link
Collaborator

lmignon commented Oct 28, 2019

@simahawk can you fix the pylint error PLZ https://travis-ci.org/shopinvader/odoo-shopinvader/jobs/601762911#L389

Thanks

@codecov-io
Copy link

codecov-io commented Oct 29, 2019

Codecov Report

Merging #471 into 12.0 will decrease coverage by 33.73%.
The diff coverage is 54.05%.

Impacted file tree graph

@@             Coverage Diff             @@
##             12.0     #471       +/-   ##
===========================================
- Coverage   91.03%   57.29%   -33.74%     
===========================================
  Files         121       56       -65     
  Lines        3456     2323     -1133     
===========================================
- Hits         3146     1331     -1815     
- Misses        310      992      +682
Impacted Files Coverage Δ
shopinvader/services/customer.py 25.86% <0%> (-65.21%) ⬇️
shopinvader/services/partner_mixin.py 26.13% <0%> (-66.97%) ⬇️
shopinvader/models/res_partner.py 51.66% <25%> (-32.82%) ⬇️
shopinvader/services/address.py 39.18% <62.5%> (-54.93%) ⬇️
shopinvader/components/access_info.py 63.63% <63.63%> (ø)
shopinvader/models/shopinvader_partner.py 26.15% <0%> (-73.85%) ⬇️
...ader/wizards/shopinvader_variant_binding_wizard.py 22.8% <0%> (-73.69%) ⬇️
...der/wizards/shopinvader_category_binding_wizard.py 27.65% <0%> (-70.22%) ⬇️
shopinvader/services/abstract_mail.py 27.27% <0%> (-61.37%) ⬇️
... and 100 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dcdb8bf...6342e3c. Read the comment docs.

@simahawk simahawk force-pushed the 12-add-shopinvader_customer_multi_user branch from 472543a to 83cb5fb Compare November 5, 2019 16:58
@simahawk
Copy link
Contributor Author

simahawk commented Nov 5, 2019

@lmignon I've drafted a bit of refactoring here:

  • partner_user on service ctx
  • new component to deal with access to partner resources (not convinced yet but it's a start)

if we find a good way to track changes (eg: post a message or a notification when the sub user edits something) I think we are almost good.

Then probably, we should lock editing on "protected" records by raising errors.
In this case I wish we had a user record so that we could use record rules 😅

@simahawk simahawk force-pushed the 12-add-shopinvader_customer_multi_user branch from 83cb5fb to 01bbc83 Compare November 6, 2019 12:50

def profile(self, partner=None):
return {
"create": False,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've listed here CRUD operations, but create here makes little sense because you cannot create a new profile w/out registration.


def address(self, address_id):
return {
# TODO: create should be a global params
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

at the same time create here makes little sense because create address permission should be global.
I'd say we should have anaccess or permission key in the global payload which states what you can do or not.

This seems to be the right place (when you get the customer/profile information).
We could have something like:

    def get(self):
        if self.partner:
            customer = self._get_customer_info()
            return {
                "data": customer,
                "store_cache": {
                    "customer": customer,
                    "permissions": self.access_info.permissions(),
                }
            }
        else:
            return {"data": {}}

@lmignon @sebastienbeau what to you think?

@simahawk simahawk force-pushed the 12-add-shopinvader_customer_multi_user branch from 01bbc83 to 417ff23 Compare November 7, 2019 08:23
@simahawk
Copy link
Contributor Author

simahawk commented Nov 7, 2019

I've rebased this on top of #495 as it unifies customer info handling

Then the registration workflow changes as following:

* for each company binding a unique token is generated, you can see it in the Shopinvader tab on company form
* on Locomotive registration page users can enter the company token (if your theme does not support this yet, just add a text field named `company_token`)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This addon doesn't depend on locomotive. we should explain how the addon works reagrding the service layer...

"update": False,
"delete": False,
# easy check on client side for being able to edit
"readonly": False,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@simahawk Why not "readonly" : True ?

(
"unique_invader_user_token",
"unique(invader_user_token)",
"Already exists in database",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what already exists?

@simahawk
Copy link
Contributor Author

simahawk commented Nov 11, 2019

I've implemented it here https://github.com/shopinvader/odoo-shopinvader/pull/471/files#diff-6a87955a2b1b8be929d84353c6449701R37-R50

Basically by default they can do everything. Then customer multi user module overrides it to block creation of new addresses. Hence, the current behavior is:

  • main account not enabled -> no matter where you are, you cannot do anything. You are redirected to an account page /account/addresses-user-disabled which shows

Screenshot from 2019-11-11 07-55-41

  • address not enabled -> you cannot use it:

2019-11-09

  • you are a simple user -> you can edit only your own address if enabled:

2019-11-094

  • you are a simple user -> you cannot edit profile data:

2019-11-09=1

  • you are logged in as main company -> you see the token to share for allowing registration to your profile:

image

The WIP PR for the template is here shopinvader/shopinvader-template#21.
We'll merge it only when this PR is merged.

I don't think we need anymore to raise a specific error for users not validated. We could also get rid of the validation methods in the controller and maybe just leave an hook.

The addition of access and permission opens the door to fine grained control by scope on what users can do client side. There's only one issue: we cannot handle them when the components are loaded inside and event handler because at that stage there's no partner nor partner_user in the work context (eg: on payment transaction update).

Once we agreed on the current implementation I clean up and add proper test coverage (I left tests aside as I had to test w/ the UI as well).

@lmignon @sebastienbeau your feedback is appreciated 😉

@simahawk simahawk force-pushed the 12-add-shopinvader_customer_multi_user branch from 3002c3f to 945589f Compare November 27, 2019 08:28
@rousseldenis rousseldenis added this to the 12.0 milestone Nov 28, 2019
@simahawk simahawk force-pushed the 12-add-shopinvader_customer_multi_user branch from 6342e3c to 6cb9f7f Compare December 10, 2019 07:36
@simahawk
Copy link
Contributor Author

extrapolated permission handling to #533
should be rebased on it.

@simahawk
Copy link
Contributor Author

simahawk commented Jan 8, 2020

moving to v13 #545

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants