Reimplement the architecture of user/groups controlpanel

[nileshgulia1]

https://github.com/plone/volto/issues/2009

[nileshgulia1]

@fredvd You can have a look at it. I also added cypress tests to detect regression in future.

[avoinea]

I think we should have the save button here as saving on checkbox select is bugous.

For example:

1. You select a role, click back and then Users & Roles:

You need to hard refresh to see the changes.

[avoinea]

This one seems even worse:

[nileshgulia1]

Updating a role doesn't return the roles updated for a specific user/group. Instead we have to re-call the /@users endpoint to fetch the updated user. In standard plone we do have a save button. Here are designs in mobile view. For web views, adapting to these can be a little tricky or maybe @albertcasado can have better options...

[sneridagh]

In @pbauer training, his first reaction was to look for the save button and when not finding one, feeling like unsure that by just clicking a checkbox, the role was set. In such a delicate action (setting user roles), I think we need a proper save button here.

@nileshgulia1 I know we might do an extra call, but in such operation I guess it's worth it.

@avoinea @tiberiuichim thoughts?

[avoinea]

Indeed, I also think that all the back-end persistent actions should be done via a Save button for UX consistency.

[tisto]

We could either show a toast message when the setting has been stored in the backend or add a dedicated save button. Though, the current situation is not very user friendly.

[nileshgulia1]

@sneridagh I gave this a try and now kinda works, see the demo. Also added the save button to indicate the roles have been saved in the backend. In Plone we also have "inherited" icon to show the user role belonging in a group. I can tackle that and some other cosmetics in a seperate PR. Since we fetch all users and groups on mount. I removed the need for "show-all users" button. Anyway the right place for that should not be inside the <Table/> component as it blocks the view on initial mount.

https://user-images.githubusercontent.com/22280901/103573306-094d9c00-4ef4-11eb-9cad-2cf7e0731bc7.mp4

-cc @tiberiuichim

[sneridagh]

@nileshgulia1 @avoinea how about this one? is it ready? Could you fix the conflicts?

[nileshgulia1]

@avoinea I made some UI changes you suggested.

• switch the order of label and icon, so first text then icon
• made both icon and label clickable.

[tisto]

I would recommend to either use the standard Pastanaga save button or use a toast message. I have a slight preference for a toast message here because we do not have those inline save buttons anywhere else in Volto if I am not mistaken. Would be nice to hear @albertcasado's opinion here as well...

[avoinea]

@tisto , @nileshgulia1, @sneridagh

IMHO the best approach would be to split this panel in two:

• Users
• Groups

This way we can move the buttons within Toolbar where they belong.

I don't see any good reason to have them together :wink:

[tisto]

+1. Since this control panel does not rely on registry entries we shouldn't have a problem here.

[nileshgulia1]

@tisto @sneridagh This is how the controlpanel looks right now.

• I have separated the "users" and "groups" to different components in the Controlpanel site.
• moved the action buttons in the toolbar.
• The toast is displayed on adding user and saving roles.

-cc @avoinea

[avoinea]

When adding a new group, the modal form seems to be adding a new User

Actually the required email property for group confused me. The form should respect the same required fields as the Plone one http://localhost:8080/Plone/@@usergroup-groupdetails

I get this error when I add a new group:

[avoinea]

This note also seems not to work:

Note that roles set here apply directly to a user. The symbolindicates a role inherited from membership in a group.

Plone:

vs

Volto:

[avoinea]

These panels should display the unauthorized error on backend errors:

See example:

https://github.com/plone/volto/blob/master/src/components/manage/Controlpanels/ContentTypes.jsx#L349-L352

[nileshgulia1]

@avoinea I've made some changes according to comments. https://github.com/plone/volto/pull/2064#issuecomment-765903020 I can't see this error when adding a new group. Can you please specify, how to reproduce it?

Also the modalForm, shows the "Add group" for groups. Let me know if this isn't happens at your end.

We should keeps the roles, here in the ModalForm atleast I guess?

[nileshgulia1]

This note also seems not to work:

Note that roles set here apply directly to a user. The symbolindicates a role inherited from membership in a group.

Plone:

vs

Volto:

@avoinea I found an issue here. If we change authentictedUsers role , then somehow the roles are not changing in the backend. Infact that particular role is 'inherited' and applied to the specific group as happens in the standard plone.

Due to this we cannot able to remove the inherited roles.

[avoinea]

If the user exists we should display the error within the modal or within a toast:

[avoinea]

I get the following error when I try to add a group that already exists:

[nileshgulia1]

@sneridagh You wanna take a final review to this! You can merge after that :)

[tisto]

@avoinea @nileshgulia1 @sneridagh I just tried to create a user and set permissions in a current Volto and the users and groups controlpanel is in a very sorry state right now. We should really try to finish this PR and improve the situation. With the seamless mode, we do not have the fallback to Plone Classic any longer and then the most basic control panels need to work (I ran into the problem that I can not check any checkboxes any longer, not sure if this is fixed by this PR as well).

[nileshgulia1]

@tisto The current state of "Users and groups" controlpanel is improved a lot in this PR. I've tried to cover most of the edge cases especially working with "inherited roles" (Decided to store inheritedAuthRole as a persistant reducer, For more info see: https://github.com/plone/volto/pull/2064#discussion_r609472618 ). For me its good to go, however it'd be great also @sneridagh to have a final look.

[sneridagh]

Also, we need an army of Cypress tests for it.

[pnicolli]

Finally had a chance to test this. The Users controlpanel works fine 👍 I found a few minor things I'm not a fan of:

1. IMHO, "No value" should never be present in select inputs

2. I see this Tick appearing when hovering the delete button, which is totally non-blocking for the merge, imho, but I think it isn't needed

3. In the Groups controlpanel, I would change the "Add new group" icon to one that represents more than one person, just to make it clear that it's different from the one in the Users controlpanel

Despite these comments and the facts that these controlpanel still lack the possibility to add a user to a group and to reset a user's password, I would totally suggest to merge this and work on the other things separately, since it's a major improvement over the current one in master.

If cypress tests are coming, it's even better, of course :)

Thanks @nileshgulia1

[nileshgulia1]

In the Groups controlpanel, I would change the "Add new group" icon to one that represents more than one person, just to make it clear that it's different from the one in the Users controlpanel.

@pnicolli Currently we don't have add-group.svg or something similar which will better indicate adding of groups. Maybe can just use the "+" icon?

As far as I remember, I looked into going deeper in "adding user to a group", might have to do some changes in restapi idk. I agree to make separate PR's on small fixes further. Let's not overfill this PR.

Apart from the icon issue, I think we can go ahead merging this.

-cc @sneridagh

[sneridagh]

@nileshgulia1 I'm moving forward also some fixes to the selectWidget, it might get some time to make it through (see my last draft PR). I would like the users and groups use its own select, with its special use case on it, so the default one is more agnostic and simple. How much would that cost?

[nileshgulia1]

@sneridagh I think there isn't much special case attached to the "Select" component that we use in this PR. We only need "selects" when creating a user/group in a ModalForm which is schema based. Its only with the "no-value" field that we needed to get rid of, at the same time I don't think its a blocker for the PR.

[nileshgulia1]

@sneridagh I've used the type: 'array' to indicate the use of ArrayWidget. I would wait for your select refactor PR to go in first and see if we can make it work. Separating/creating a new Select component( not a new widget ) for this specific use case, doesn't work for me as a quick feature and I think it will cost a lot more to make it work.

Also for this we would have to make a refactor to <ModalForm/> , which in many places we are already using it like this.

@sneridagh From the yesterday's meeting, We should only use SelectWidget with "plone.restapi" based json data, right?

Here, the use case we have is for listing roles coming from /@roles backend endpoint, so no hardcoded values.

[pnicolli]

Hi everyone, what is the state on this PR and related things that were mentioned in the last comments? The current volto user controlpanel is really not working and it really needs some sort of fix. As I mentioned the last time we talked about it in a volto meeting, I would totally accept merging a PR and reworking some of the stuff later. About a month has passed and I think something should be done about this, because it would be otherwise unacceptable to skip fixing a vital part of a cms like I believe this one is.

[nileshgulia1]

@pnicolli I'm okay to merge the PR. @sneridagh will merge once he gets back from holidays!

[nileshgulia1]

@sneridagh We can merge this once you give green flag. I accidentally rebased on wrong branches while testing and cleaned it up thereafter. However I request you to "squash and merge" this (If you have option).

[sneridagh]

@nileshgulia1 let's merge this asap after you take a look at the last issues!

[nileshgulia1]

@nileshgulia1 I finally had time to go through it properly. I've found some things, could you please check them and fix them?

The main one is that seems that a feature introduced in #1733 is missing (probably due to a merging issue) or did you already took care of it in your code?

Yes @sneridagh, I removed it intentionally, We "showAll" users/groups on intial load of the corresponding controlpanel now. Anyway the correct place for "ShowAll" button should not be inside the table, just my 2c.

I removed the obsolete icon file. We can merge now.

Thanks

[sneridagh]

We do show all users on initial load? How about if we have 100k users, eg. coming from ldap?

Could you quickly point me Where do we have that call?

[nileshgulia1]

@sneridagh I've tried to replicate what currently Plone has, here's the GET call: https://github.com/plone/volto/blob/2230371d2c8c8c07d14debde764cb0b104ec0370/src/components/manage/Controlpanels/Users/UsersControlpanel.jsx#L112

In the current scenario( volto.kitconcept.com), we don't show any user onload and have to click on "showAll" everytime, which intern gonna list "all users" without any batching. See below: https://github.com/plone/volto/blob/master/src/components/manage/Controlpanels/UsersControlpanel.jsx#L482

I don't remember the exact reason but the correct place for that button should be somewhere in the segment, not inside the table as it interferes with the content.

[nileshgulia1]

Maybe I'm missing something here, @sneridagh Let me know your thoughts, I'm okay to re-add the changes lost.

[sneridagh]

@nileshgulia1 in https://github.com/plone/volto/pull/1733 comments @tisto and others already raised that concern.

Also, per documentation, all users listings are not allowed if you are not manager: https://plonerestapi.readthedocs.io/en/latest/users.html

But these use case is covered I guess, because "normally" that control panel can't be accessed by users that are not managers.

@tisto I guess that we can continue if the "show all users" is the default. Still, the use case of having thousands users can be "problematic". What do you think?

@nileshgulia1 problem is, that we could DoS the server if it tries to retrieve all the users...

[nileshgulia1]

Also, per documentation, all users listings are not allowed if you are not manager:

Okay, this feature also lacks in current volto, as its there in Plone with LDAP accounts. I can take care of it too after this PR gets merged.