Ticket #9330 (closed PLIP: fixed)

Opened 5 years ago

Last modified 4 years ago

Add ability to choose group when adding new site members

Reported by: aclark Owned by: aclark
Priority: minor Milestone: 4.0
Component: Unknown Version:
Keywords: Cc: plip-advisories@…

Description (last modified by aclark) (diff)

Proposer: Alex Clark
Seconder: Eric Steele

Motivation

I recently came across a small usability enhancement I'd like to make. When adding a user, it is often part of a two-step process to make that user an administrator. Why not add a checkbox to the registration form that would allow end users to have the option to make the new user a site manager, e.g.

[*] Allow user to manage site?

Alternatively, a menu containing a list of site groups could be used. E.g.

Choose group for new user

  • Manager
  • Reviewer
  • Member
  • etc.

Assumptions

Proposal assumes new feature would be desired, but no research has been done to substantiate that claim.

Proposal & Implementation

I propose modifying 'skins/plone_login/register.cpy' to add this feature. The implementation would involve UI components (e.g. adding form fields) as well as modifying the call to portal_registration to include the options necessary to make the new member a part of the selected group(s). Possibly move register.cpy to a browser view.

Deliverables

  • Code
    • Modifications to register.cpy and/or modifications plus move to browser view.
  • Unit tests
    • Add unit tests to cover newly added code and feature set.
  • Localization
    • Add localization as appropriate.
  • Documentation
    • Add documentation as appropriate, e.g. include in list of new features.

Risks

Low

Participants

Alex Clark <aclark>

Progress

None yet

Change History

comment:1 Changed 5 years ago by aclark

  • Description modified (diff)

comment:2 Changed 5 years ago by esteele

  • Description modified (diff)

comment:3 Changed 5 years ago by aclark

  • Description modified (diff)

comment:4 Changed 5 years ago by aclark

  • Milestone changed from 3.x to 4.0

comment:5 Changed 5 years ago by limi

Assigning a role to a user directly is something we're trying to avoid, it is harder to keep track of, which is why we discourage it, and ship with an Administrators group instead.

If this feature is going to be added, my recommendation would be a generalized "also add the user to these groups" functionality, not set a role directly on the user.

comment:6 Changed 5 years ago by gotcha

I second limi about adding to groups rather than granting roles...

comment:7 Changed 5 years ago by mborch

As on UNIX; makes sense to me.

comment:8 Changed 5 years ago by csenger

I also think we should force the admins to use groups instead of roles - at least make that the much more obvious choice.

I'd suggest to also remove the constrains from the security control panel for admins. Not being able to add a member without email verification and the possibility to set a password is a bit too much of a good advice for people that can disable these options within 3 clicks.

comment:9 Changed 5 years ago by erikrose

Clearing Owner field of 4.0 PLIPs so we can use it to mean "implementor". (Many of these owners were automatically assigned from choosing a Component that had a default owner.)

comment:10 Changed 5 years ago by alecm

  • Owner set to aclark

If this PLIP was for group assignment, rather than role assignment, I would be +1. It would need to allow assignment of multiple groups with a sensible UI. As it is: -1.

comment:11 Changed 5 years ago by jensens

-1 as already said, using roles is wrong here. Using groups would be a +1 from me.

comment:12 Changed 5 years ago by smcmahon

  • Cc plip-advisories@… added

comment:13 Changed 5 years ago by aclark

I'm certainly willing to accommodate the use of groups instead of roles, assuming I can make it work for my use case which is "check a box, make this person an administrator" (i.e. there is a Managers group right? Or if not I"m allowed to create one?" I'll update the Description accordingly.

comment:14 Changed 5 years ago by aclark

  • Description modified (diff)

comment:15 Changed 5 years ago by MatthewWilkes

FWT Vote: +1

comment:16 Changed 5 years ago by rossp

FWT vote: +1. Use groups, not roles. :)

comment:17 Changed 5 years ago by davisagli

FWT vote: +1 if using groups.

comment:18 Changed 5 years ago by calvinhp

FWT Vote: +1 if we are using groups and not roles

comment:19 Changed 5 years ago by raphael

FWT Vote: +1 if we are using groups and not roles

comment:20 Changed 5 years ago by esteele

Approved by FWT vote.

comment:21 Changed 5 years ago by aclark

  • Status changed from new to assigned

comment:22 Changed 5 years ago by aclark

(In [28155]) Branch CMFPlone 4.0 branch to work on plip 9330, refs #9330.

comment:23 Changed 5 years ago by aclark

(In [28156]) Add buildout config file for plip 9330, refs #9330.

comment:24 Changed 5 years ago by aclark

(In [29055]) First cut adding role selection to join form, refs #9330, wherein we protect the field by checking for manager role (so only managers get the choice) as well as populate the vocab via context/getGlobalSiteRoles and select 'Member' as the default. Next up: make it work.

comment:25 Changed 5 years ago by aclark

  • Summary changed from Add ability to choose role when adding new site members to Add ability to choose group when adding new site members

comment:26 Changed 5 years ago by aclark

(In [29057]) Errr, what I mean to say is, first cut adding group selection to join form, refs #9330, wherein we protect the field by checking for manager role (so only managers get the choice) as well as populate the vocab via context/portal_groups/getGroupIds and select 'AuthenticatedUsers' as the default. Next up: make it work.

comment:27 Changed 5 years ago by aclark

(In [29068]) Make a note on something I need to do later, namely call acl_users to get virtual groups instead of pre-populating it by hand with a known good string (i.e. AuthenticatedUsers), refs #9330.

comment:28 Changed 5 years ago by aclark

(In [29069]) Make input name something we can look for in the request, e.g. group.Administrators, group.Reviewers (refs #9330).

comment:29 Changed 5 years ago by aclark

(In [29070]) W00t, first working cut, refs #9330.

comment:30 Changed 5 years ago by esteele

Your PLIP has been reviewed by the Framework team. Feel free to discuss any suggested changes either here in the PLIP ticket or on the mailing lists. Final deadline for this PLIP is set for September 23.

comment:31 Changed 5 years ago by aclark

(In [29852]) Move getGroupIds call inside block protected by 'Manage users' permission (and change permission from 'Manage portal' to 'Manage users'). This makes self registration work again. refs #9330

comment:32 Changed 5 years ago by aclark

(In [29853]) Do not allow user to select groups if 'Many groups' is checked in the usergroup control panel. refs #9330

comment:33 Changed 5 years ago by aclark

(In [29854]) Use group title instead of id; re-position ignore definition closer to the block it is used in. (Next, make it call PAS for list of auto-groups.) refs #9330

comment:34 follow-up: ↓ 36 Changed 5 years ago by rossp

Revisited my review. All of my comments appear to have been addressed quite nicely. I note that my last comment regarding using multi-select with lots of groups doesn't seem necessary since the form doesn't render the groups checkboxes if "Many Groups" is checked. +1 for merge.

comment:35 Changed 5 years ago by aclark

(In [30081]) The publisher supports argument conversion, so why not use it? (Who knew?! ;-) refs #9330

comment:36 in reply to: ↑ 34 Changed 5 years ago by aclark

Replying to rossp:

Revisited my review. All of my comments appear to have been addressed quite nicely. I note that my last comment regarding using multi-select with lots of groups doesn't seem necessary since the form doesn't render the groups checkboxes if "Many Groups" is checked. +1 for merge.

Thanks! I'm not exactly sure how to address that "multi-select with lots of groups" anyway, so happy to leave it out (and like you say it's not needed). How would you do that if you were going to? tal:condition blocks to do check box by default, and multiselect when len(groups) > MANY, I imagine.

comment:37 Changed 5 years ago by MatthewWilkes

Adding a user without any groups causes a KeyError. This is true for any self-registrations and many admin-registrations.

FWT vote: -1 on merging

comment:38 Changed 5 years ago by aclark

(In [30238]) A simple fix, I hope. Refs #9330. +1 Matthew Wilkes, +1! :-)

comment:39 Changed 5 years ago by rossp

Everyting works for me, can't reproduce MatthewWilkes's error. Still +1 for merge.

comment:40 Changed 5 years ago by aclark

I fixed his error in [30238]

comment:41 Changed 5 years ago by erikrose

New error: check, say, the Reviewers box when adding a user, forget to fill out a username, submit the form, and the Reviewers box is no longer checked.

comment:42 Changed 5 years ago by erikrose

(That's not worth holding up the merge for, though.)

comment:43 Changed 5 years ago by erikrose

(In [30255]) Changed a has_key (which is deprecated--use "x in y" instead) plus an if to a get for fewer code paths. No behavior changes. Refs #9330.

comment:44 Changed 5 years ago by rossp

(In [30257]) Add postback support so that selected groups are preserved when validation errors occur. Refs #9330 comment 41.

comment:45 Changed 5 years ago by rossp

Having just looked at #9310 and #9330 again, I realized that merging these two PLIPs will mean mostly re-implementing #9330 since #9310 uses formlib and #9330 makes changes to the old form controller template. Since #9310 preserves the old form controller template, we could still merge #9330 but it would be unused by default.

If the implementer is up to it, however, it should be possible to add a field and widget to the #9310 formlib form.

comment:46 Changed 5 years ago by aclark

That's fine. If it is all the same to everyone else, I'll just merge #9330 to the 4.0 branch (trivial) then set out to "fix" things once #9310 gets merged (or ASAP, if it has been merged already).

comment:47 Changed 5 years ago by esteele

This PLIP has been accepted for merging into Plone 4.0

The final vote was: Alec Mitchell +1 David Glick +1 Erik Rose +1 Laurence Rowe +1 Matthew Wilkes -1 Ross Patterson +1

Please merge your branches into the Plone 4.0 head by end-of-day Friday Oct 16. If you need assistance with merging, please contact me.

We'll be assigning a documentation ticket to this PLIP shortly. Please assist the docs team in documenting the changes and new features that this PLIP introduces.

comment:48 Changed 5 years ago by esteele

Please assist the doc team in creating/updating documentation relating to this PLIP. See #9617.

comment:49 Changed 5 years ago by aclark

(In [30655]) Branch for 9330, refs #9330 (I assume this is where the flexible registration stuff gets done.)

comment:50 Changed 4 years ago by aclark

(In [31105]) Merging #9330, refs #9330

comment:51 Changed 4 years ago by esteele

  • Status changed from assigned to closed
  • Resolution set to fixed
Note: See TracTickets for help on using tickets.