339437 - Make flag requestee a select box and limit users to requestable only

Status: RESOLVED FIXED

(Bugzilla :: Attachments & Requests, enhancement)

Description

[Teemu Mannermaa (:wicked)]

When a requestee is selected for a flag, the selection should be a listbox that is populated with users that are in the grant group of the flag. There still should be a possibility to select somebody that's not in the populated list (see bug 294021).

Comment 1

[Max Kanat-Alexander]

This should be optional on the flag type. That is, I should be able to select it per flag-type.

Comment 2

[Seth]

Attachment: Patch to have the requestee be a drop down list

This replaces the input box with a dropdown of users who are allowed to grant the specific flag.  Only occurs if the 'usemenuforusers' param is selected.
Patched 3.0 Stable

Modifies: 
User.pm -> Adds the User ID to fields returned by get_userlist() reduce queries to DB
FlagType.pm -> Adds the method can_grant_list(AllUsers)

Template/flag/list.html.tmpl -> Adds the drop down logic, modifies javascript to handle selects and inputs.
Template/global/userselect.html.tmpl -> Modified so you can pass in your own user list.

Comment 3

[Seth]

Comment on attachment 267764 [details] [diff] [review]
Patch to have the requestee be a drop down list

Index: webtools/bugzilla/Bugzilla/FlagType.pm
===================================================================
RCS file: /cvsroot/mozilla/webtools/bugzilla/Bugzilla/FlagType.pm,v
retrieving revision 1.37
diff -u -r1.37 FlagType.pm
--- webtools/bugzilla/Bugzilla/FlagType.pm	24 Aug 2006 15:56:39 -0000	1.37
+++ webtools/bugzilla/Bugzilla/FlagType.pm	8 Jun 2007 23:07:49 -0000
@@ -358,7 +358,17 @@
                                        FROM $tables WHERE $criteria");
     return $count;
 }
-
+sub can_grant_list{
+	my ($self, $alluserspassed) = @_ ;
+	my @custusers;	
+	my @allusers = @$alluserspassed;
+	for my $i(0..$#allusers){
+	   if(new Bugzilla::User($allusers[$i]{userid})->can_set_flag($self)){
+	 	push(@custusers,$allusers[$i]);
+	   }
+	}
+	return @custusers;
+}
 ######################################################################
 # Private Functions
 ######################################################################
Index: webtools/bugzilla/Bugzilla/User.pm
===================================================================
RCS file: /cvsroot/mozilla/webtools/bugzilla/Bugzilla/User.pm,v
retrieving revision 1.148
diff -u -r1.148 User.pm
--- webtools/bugzilla/Bugzilla/User.pm	5 Feb 2007 21:34:20 -0000	1.148
+++ webtools/bugzilla/Bugzilla/User.pm	8 Jun 2007 23:07:49 -0000
@@ -1492,7 +1492,7 @@
     return $self->{'userlist'} if defined $self->{'userlist'};
 
     my $dbh = Bugzilla->dbh;
-    my $query  = "SELECT DISTINCT login_name, realname,";
+    my $query  = "SELECT DISTINCT login_name, userid, realname,";
     if (Bugzilla->params->{'usevisibilitygroups'}) {
         $query .= " COUNT(group_id) ";
     } else {
@@ -1512,9 +1512,10 @@
     $sth->execute;
 
     my @userlist;
-    while (my($login, $name, $visible) = $sth->fetchrow_array) {
+    while (my($login, $userid, $name, $visible) = $sth->fetchrow_array) {
         push @userlist, {
             login => $login,
+            userid => $userid,
             identity => $name ? "$name <$login>" : $login,
             visible => $visible,
         };
Index: webtools/bugzilla/template/en/default/flag/list.html.tmpl
===================================================================
RCS file: /cvsroot/mozilla/webtools/bugzilla/template/en/default/flag/list.html.tmpl,v
retrieving revision 1.27
diff -u -r1.27 list.html.tmpl
--- webtools/bugzilla/template/en/default/flag/list.html.tmpl	14 Jan 2007 20:37:57 -0000	1.27
+++ webtools/bugzilla/template/en/default/flag/list.html.tmpl	8 Jun 2007 23:07:49 -0000
@@ -45,6 +45,10 @@
   function disableRequesteeFields()
   {
     var inputElements = document.getElementsByTagName("input");
+    var el2=document.getElementsByTagName("select");
+    for(var i=0;i<el2.length;i++){//Combine inputs with selects
+    	inputElements[inputElements.length+i]=el2.item(i);
+    }
     var inputElement, id, flagField;
     for ( var i=0 ; i<inputElements.length ; i++ )
     {
@@ -105,7 +109,7 @@
         <td>
           <select id="flag-[% flag.id %]" name="flag-[% flag.id %]" 
                   title="[% type.description FILTER html %]"
-                  onchange="toggleRequesteeField(this);">
+                  onchange="toggleRequesteeField(this);" [%UNLESS (user.can_set_flag(type) ||(type.is_requestable && 

user.can_request_flag(type))) %]disabled="true"[% END %]>
             [%# Only display statuses the user is allowed to set. %]
             [% IF user.can_request_flag(type) %]
               <option value="X"></option>
@@ -128,15 +132,27 @@
         [% IF any_flags_requesteeble %]
           <td>
             [% IF type.is_active && type.is_requesteeble %]
-              <span style="white-space: nowrap;">
-                (<input type="text" size="30" maxlength="255"
+              <span style="white-space: nowrap;">(
+              [% IF Param('usemenuforusers') %]
+                [% INCLUDE global/userselect.html.tmpl
+		         name => "requestee-$flag.id"
+		         id => "requestee-$flag.id"
+		         value => flag.requestee.login
+		         disabled => 0
+		         multiple => 0
+		         emptyok => 1
+		         customuserlist => flag.type.can_grant_list(user.get_userlist)
+       			%]
+              [% ELSE %]
+                <input type="text" size="30" maxlength="255"
                         id="requestee-[% flag.id %]" 
                         name="requestee-[% flag.id %]"
                         [% IF flag.status == "?" && flag.requestee %]
                           value="[% flag.requestee.login FILTER html %]"
                         [% END %]
-                 >)
-              </span>
+                 >
+                 [% END %]
+                 )</span>
             [% END %]
           </td>
         [% END %]
@@ -155,7 +171,7 @@
         <td>
           <select id="flag_type-[% type.id %]" name="flag_type-[% type.id %]" 
                   title="[% type.description FILTER html %]"
-                  [% " disabled=\"disabled\"" UNLESS user.can_request_flag(type) %]
+                  [%UNLESS (user.can_set_flag(type) = 1 ||(type.is_requestable && user.can_request_flag(type))) 

%]disabled="true"[% END %]
                   onchange="toggleRequesteeField(this);">
             <option value="X"></option>
             [% IF type.is_requestable && user.can_request_flag(type) %]
@@ -171,9 +187,22 @@
           <td>
             [% IF type.is_requesteeble %]
               <span style="white-space: nowrap;">
-                (<input type="text" size="30" maxlength="255"
+                ([% IF Param('usemenuforusers') %]
+		         [% INCLUDE global/userselect.html.tmpl
+		         name => "requestee_type-$type.id"
+		         id => "requestee_type-$type.id"
+		         value => ""
+		         disabled => 0
+		         multiple => 0
+		         emptyok => 1
+		         customuserlist => type.can_grant_list(user.get_userlist)
+		       %]
+		    [% ELSE %]
+       <input type="text" size="30" maxlength="255"
                         id="requestee_type-[% type.id %]" 
-                        name="requestee_type-[% type.id %]">)
+                        name="requestee_type-[% type.id %]">
+			[% END %]
+       		)
               </span>
             [% END %]
           </td>
@@ -214,9 +243,21 @@
         <td>
           [% IF type.is_requesteeble %]
               <span style="white-space: nowrap;">
-                (<input type="text" size="30" maxlength="255"
+                ([% IF Param('usemenuforusers') %]
+		         [% INCLUDE global/userselect.html.tmpl
+		         name => "requestee_type-$type.id"
+		         id => "requestee_type-$type.id"
+		         value => ""
+		         disabled => 0
+		         multiple => 0
+		         emptyok => 1
+		         customuserlist => type.can_grant_list(user.get_userlist)
+		       %]
+		    [% ELSE %]
+                <input type="text" size="30" maxlength="255"
                         id="requestee_type-[% type.id %]" 
-                        name="requestee_type-[% type.id %]">)
+                        name="requestee_type-[% type.id %]">
+            [% END %])
               </span>
           [% END %]
         </td>
Index: webtools/bugzilla/template/en/default/global/userselect.html.tmpl
===================================================================
RCS file: /cvsroot/mozilla/webtools/bugzilla/template/en/default/global/userselect.html.tmpl,v
retrieving revision 1.5
diff -u -r1.5 userselect.html.tmpl
--- webtools/bugzilla/template/en/default/global/userselect.html.tmpl	17 Mar 2005 15:50:45 -0000	1.5
+++ webtools/bugzilla/template/en/default/global/userselect.html.tmpl	8 Jun 2007 23:07:49 -0000
@@ -26,7 +26,7 @@
   # size: optional, input only; size attribute value
   # emptyok: optional, select only;  if true, prepend menu option to start of select
   # multiple: optional, do multiselect box, value is size (height) of box
-  #
+  # customuserlist: optional, specify a limited list of users to use
   #%]
 
 [% IF Param("usemenuforusers") %]
@@ -40,7 +40,10 @@
   [% IF emptyok %]
     <option value=""></option>
   [% END %]
-  [% FOREACH tmpuser = user.get_userlist %]
+  [% UNLESS customuserlist %]
+  	[% customuserlist = user.get_userlist %]
+  [% END %]
+  [% FOREACH tmpuser = customuserlist %]
     [% IF tmpuser.visible OR value.match("\\b$tmpuser.login\\b") %]
       <option value="[% tmpuser.login FILTER html %]"
         [% " selected=\"selected\"" IF value.match("\\b$tmpuser.login\\b") %]

Comment 4

[Frédéric Buclin]

Comment on attachment 267764 [details] [diff] [review]
Patch to have the requestee be a drop down list

>Index: webtools/bugzilla/Bugzilla/FlagType.pm

>+sub can_grant_list{

You need to add POD to describe this method. Moreover, it's a *method*, not a function, so it should be move in the appropriate section in FlagType.pm.


>+	my ($self, $alluserspassed) = @_ ;

You always pass $user->get_userlist to this method, so the last argument is currently useless. We will add it in the future if we really need it.


>+	my @allusers = @$alluserspassed;

@allusers should fall back to $user->get_userlist if $alluserspassed is not given. And per my previous command, that's what @allusers should be set to.


>+	for my $i(0..$#allusers){
>+	   if(new Bugzilla::User($allusers[$i]{userid})->can_set_flag($self)){
>+	 	push(@custusers,$allusers[$i]);
>+	   }
>+	}

Use Bugzilla::User->new_from_list(), it's much faster.



>Index: webtools/bugzilla/Bugzilla/User.pm

>-    my $query  = "SELECT DISTINCT login_name, realname,";
>+    my $query  = "SELECT DISTINCT login_name, userid, realname,";

You don't need 'userid', unless you use new_from_list(), which only accepts IDs.



>Index: webtools/bugzilla/template/en/default/flag/list.html.tmpl

>+    var el2=document.getElementsByTagName("select");

Nit: add whitespaces around = and other operators.


>+    for(var i=0;i<el2.length;i++){//Combine inputs with selects
>+    	inputElements[inputElements.length+i]=el2.item(i);
>+    }

This looks wrong to me. inputElements.length will be incremented after each insertion, so incrementing inputElements by i instead of 1 will add many "holes" in your array.


>           <select id="flag-[% flag.id %]" name="flag-[% flag.id %]" 
>                   title="[% type.description FILTER html %]"
>-                  onchange="toggleRequesteeField(this);">
>+                  onchange="toggleRequesteeField(this);" [%UNLESS (user.can_set_flag(type) ||(type.is_requestable && user.can_request_flag(type))) %]disabled="true"[% END %]>

Don't do that. Disabling the flag will be seen as an attempt to cancel the flag. Leave this part of the code alone.


>+              [% IF Param('usemenuforusers') %]
>+                [% INCLUDE global/userselect.html.tmpl
>+		         name => "requestee-$flag.id"
>+		         id => "requestee-$flag.id"
>+		         value => flag.requestee.login
>+		         disabled => 0
>+		         multiple => 0
>+		         emptyok => 1
>+		         customuserlist => flag.type.can_grant_list(user.get_userlist)
>+       			%]

Better is to call flag.type.can_grant_list with no argument, per my previous comments.


>           <select id="flag_type-[% type.id %]" name="flag_type-[% type.id %]" 
>                   title="[% type.description FILTER html %]"
>-                  [% " disabled=\"disabled\"" UNLESS user.can_request_flag(type) %]
>+                  [%UNLESS (user.can_set_flag(type) = 1 ||(type.is_requestable && user.can_request_flag(type))) %]disabled="true"[% END %]
>                   onchange="toggleRequesteeField(this);">

Same comment as above. Leave this alone.

Comment 5

[Seth]

Thanks for the quick detailed review.  I have all of the issues you pointed out resolved, except I am having trouble not passing in the userlist.  I cannot seem to find a way of accessing a user I can call get_userlist() on.  I tried  Bugzilla::User->get_userlist() but it throws "undef error - Can't use string ("Bugzilla::User") as a HASH ref while "strict refs" in use at Bugzilla/User.pm line 1492." and Bugzilla->user->get_userlist(); throws  Bad argument param sent to Bugzilla::User::new function. any insight into how to resolve this?

Comment 6

[Frédéric Buclin]

(In reply to comment #5)
> and Bugzilla->user->get_userlist(); throws  Bad argument param sent
> to Bugzilla::User::new function. any insight into how to resolve this?

Bugzilla->user->get_userlist is valid. Where do you try to insert that? If you are in a method of User.pm, then you should write $self->get_userlist, because $self is the current user. If you are outside User.pm, then Bugzilla->user is the right way to go.

While I'm on it, I'm reassigning the bug to you as you are actively working on it.

Comment 7

[Seth]

Attachment: Patch to make the requestee field be a grop down

Changed so user.get_userlist is no longer passed in, User.pm is no longer modified, fixed javascript to not use nodelist and no longer disables the flag selects if user cannot change.

Comment 8

[Frédéric Buclin]

There are three ways to implement this request:

1) Look at 'usemenuforusers' and display the list of available users when this parameter is turned on.

2) Only display the list of available users if the grant group is not empty. Else leave the requestee field as a free text field as it is now.

3) Combine 1) and 2), i.e. only display the list of available users if the grant group is not empty *AND* 'usemenuforusers' is turned on.


1) is suitable for installations with very few users, and works the same way as other user fields when 'usemenuforusers' is on.
2) and 3) are suitable even for large installations where the grant group usually contains a very small number of users allowed to set that flag.

Probably the right solution is either 1) or 2). 3) would look inconsistent to users. Do you guys have any preference?

Comment 9

[Max Kanat-Alexander]

Well, the easiest and most sensible solution would be #1. So I'd go with that unless somebody has some other points to make.

Comment 10

[Frédéric Buclin]

Comment on attachment 268096 [details] [diff] [review]
Patch to make the requestee field be a grop down

>Index: Bugzilla/FlagType.pm

>+The array is populated with hashrefs containing the login, identity and visible.

s/visible/visibility of users/


>+Users that are not visible to this user will have 'visible' set to zero.

No need to specify this. This is a property of get_userlist.


>+sub can_grant_list{

Thinking about it again, grant_list() would be a better name than can_grant_list().


>+	for my $i(0..$#allusers){

I much prefer the use of foreach here:

foreach my $user (@allusers) {...}



>Index: template/en/default/flag/list.html.tmpl

>+              [% IF Param('usemenuforusers') %]
>+                [% INCLUDE global/userselect.html.tmpl
>+		         name => "requestee-$flag.id"
>+		         id => "requestee-$flag.id"
>+		         value => flag.requestee.login
>+		         disabled => 0
>+		         multiple => 0
>+		         emptyok => 1

No need to specify |disabled => 0| as that's the default. Moreover, you should be allowed to request more than one requestee at once as flags may be multiplicable. I think 3 would be a good value (but the UI will not be nice as is). About emptyok, we could probably remove it, unless you want to request from the wind and from some specific user at the same time.


>+                 >
>+                 [% END %]
>+                 )</span>
>             [% END %]

Fix the indentation correctly, and remove tabs (they are not allowed in templates).


>-                  [% " disabled=\"disabled\"" UNLESS user.can_request_flag(type) %]
>+                 [% " disabled=\"disabled\"" UNLESS user.can_request_flag(type) %]

Remove this change.


>+                ([% IF Param('usemenuforusers') %]
>+		         [% INCLUDE global/userselect.html.tmpl
>+		         name => "requestee_type-$type.id"
>+		         id => "requestee_type-$type.id"
>+		         value => ""
>+		         disabled => 0
>+		         multiple => 0
>+		         emptyok => 1

Same comments here.


>+                ([% IF Param('usemenuforusers') %]
>+		         [% INCLUDE global/userselect.html.tmpl
>+		         name => "requestee_type-$type.id"
>+		         id => "requestee_type-$type.id"
>+		         value => ""
>+		         disabled => 0
>+		         multiple => 0
>+		         emptyok => 1
>+		         customuserlist => flag.type.can_grant_list(user.get_userlist)

Same comments + can_grant_list() no longer takes an argument.



>Index: template/en/default/global/userselect.html.tmpl

>+  # customuserlist: optional, specify a limited list of users to use

Nit: custom_userlist would be more readable.


Else your patch looks good, but I'm not sure about the UI when usemenuforusers is turned on. Well, we already know that the UI is not nice when this param is turned on, so there is nothing new here. :)

Comment 11

[Seth]

Attachment: Patch to make flag requestor a drop down V3

Implemented your comments. For multiple and emptyok, i changed them to depend on whether the flag if multipliable when they are being used to add flag requests(the second 2 instances)  For the first instance, this is only used to show existing flags, and therefore needs only a single select drop down.

Comment 12

[Seth]

Attachment: Patch to make flag requestor a drop down V4

Fixed 2 copy/paste bugs

Comment 13

[Frédéric Buclin]

Attachment: patch, v4.1

Here is the patch I'm going to check in. I removed tabs which were in your patch and also fixed the indentation in templates. I also did a small cleanup in FlagType.pm to better match our guideline.

Thanks for the patch. r=LpSolit

Comment 14

[Frédéric Buclin]

Checking in Bugzilla/FlagType.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/FlagType.pm,v  <--  FlagType.pm
new revision: 1.38; previous revision: 1.37
done
Checking in skins/standard/global.css;
/cvsroot/mozilla/webtools/bugzilla/skins/standard/global.css,v  <--  global.css
new revision: 1.38; previous revision: 1.37
done
Checking in template/en/default/attachment/edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/attachment/edit.html.tmpl,v  <--  edit.html.tmpl
new revision: 1.43; previous revision: 1.42
done
Checking in template/en/default/flag/list.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/flag/list.html.tmpl,v  <--  list.html.tmpl
new revision: 1.29; previous revision: 1.28
done
Checking in template/en/default/global/userselect.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/userselect.html.tmpl,v  <--  userselect.html.tmpl
new revision: 1.7; previous revision: 1.6
done

Comment 16

[miketosh]

FYI, this patch didn't work too well for me when applied to Bugzilla 3.0.0.  Maybe I have too many users (500) and too many flags that are specifically requestable (10) to go through iterations of each user and check user->cansetflag.

Or maybe I had a mispelling.  I doubt that, and I checked it several times.

Instead of going user-by-user, wouldn't it be better to get the list of users of groups that are inherited by the grant group?  The Bugzilla/User.pm has a flatten_group_membership, so wouldn't that be much better used to find all members of a flattened group?

Comment 17

[miketosh]

I created the following sub in Bugzilla/Group.pm as a quicker way to get a list of members of a group.  Maybe this goes against your code logic that you normally write to, but it is so much quicker for me.  Now I can go and set FlagType.pm


sub members_inherited {
    my ($self) = @_;
    return $self->{members_inherited}
           if exists $self->{members_inherited};

    require Bugzilla::User;
    my $sql = 'SELECT DISTINCT user_id AS user_id FROM user_group_map
          WHERE isbless = 0 AND group_id IN(' .
          join(',',@{Bugzilla::User->flatten_group_membership($self->id)}) .
          ')';
    my $in_member_ids = Bugzilla->dbh->selectcol_arrayref($sql,undef) || [];
    $self->{members_inherited} = Bugzilla::User->new_from_list($in_member_ids);
    return $self->{members_inherited};
}

Comment 18

[Frédéric Buclin]

Your patch doesn't fix the problem. Where do you take the grant group into account? And what about group visibility? Anyway, the select box is not recommended when you have more than a few tens of users in your installation.

Comment 19

[miketosh]

Sorry, that was only one piece that helped me get where i need to be.  I took an the patch v4.1, and changed the grant_list sub of FlagType.pm to work correctly with the above members_inherited (for me who doesn't use visibility groups)


sub grant_list {
    my $self = shift;

    if (!defined $self->{'grant_group'} && $self->{'grant_group_id'}) {
        $self->{'grant_group'} = new Bugzilla::Group($self->{'grant_group_id'});
    }
    my $grant_group = $self->grant_group;
    my @custusers = @{$grant_group->members_inherited};
    return \@custusers;
}


The problem with the patch, was it would bomb if the grant_group had not yet been instantiated.  Or that was the error I got.  So here is my version of that.

Other than these two pieces, I appreciate the changes incorporated into your patch that I was able to borrow.  Which is why I felt to post my changes too.

Comment 20

[miketosh]

Oh, my Grant Groups all have less than 20 users each, so that is one of the reasons I wanted to be able to fix them.  We originally had hard coded the user list into the template in 2.20, so this is a vast improvement.

Comment 21

[Frédéric Buclin]

If you have the feeling that the code could be optimized without breaking if the grant group is undefined or when group visibility restrictions are in use, feel free to file a new bug and attach a fully functional patch. Pasting your changes in a comment in this bug has no chance to be included in the main code. If there is a real performance gain, I would be happy to take your patch. You can CC me on the new bug, if you want to.

Comment 23

[Max Kanat-Alexander]

Added to the release notes for Bugzilla 3.2 in a patch on bug 432331.