add an option to show users in a drop down menu instead of a text edit field if there's less than X users.

this is very similar to bug 52557, but i don't like how that patch is executed. i'll have a play with it on this bug, and see what i can do.

just an initial patch to show what i'm thinking of. it's functional, but i've only updated edit_bug cc, qa_contact, and reassign_to.

implements a menu for selecting users : - seperates condition and drawing of edit / menu into global/userselect.html.tmpl - uses a single parameter to toggle option across all user fields - edit*.cgi still use existing interface (once they are templated, i'll fix that) - uses same form name as existing input element, so existing javascript (eg. default assigned_to) doesn't break

Comment on attachment 154007 [details] [diff] [review] usemenuforusers v3 clearing review request, forgot about buglist --> reassign

One question here is if we should include joel's visibility-foo here as well or leave that for the next patch.

Comment on attachment 154008 [details] [diff] [review] usemenuforusers v4 Finally, a patch I can actually understand. :-) I like this change. Please be encouraged by the easy review -- we should be able to get this right quite quickly. Sorry for taking a bit to look at the patch. >Index: buglist.cgi >+ GetUserTable($vars); I'd much rather this was $vars{'userlist'} = GetUserTable(). Can't this be placed somewhere better than globals.pl? Do you see a module where you think it could fit? User.pm perhaps? >Index: globals.pl >+$::UserTableLoaded = 0; >+sub GetUserTable($) { We don't use these ($) things. :-) >+ my $vars = shift; >+ >+ $vars->{'usemenuforusers'} = Param('usemenuforusers') ? 1 : 0; >+ return unless Param('usemenuforusers'); >+ >+ if(!$::UserTableLoaded) { It's much nicer if you do something like: my $user_table; sub GetUserTable { # ... return $user_table if defined $user_table; # do stuff >Index: template/en/default/bug/edit.html.tmpl >=================================================================== >RCS file: /cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/edit.html.tmpl,v >retrieving revision 1.40 >diff -u -r1.40 edit.html.tmpl >--- template/en/default/bug/edit.html.tmpl 7 Mar 2004 23:27:30 -0000 1.40 >+++ template/en/default/bug/edit.html.tmpl 16 Jul 2004 15:05:43 -0000 >@@ -21,6 +21,7 @@ > #%] > > [% PROCESS global/variables.none.tmpl %] >+[% PROCESS global/userselect.html.tmpl %] > > [% PROCESS bug/time.html.tmpl %] > >@@ -163,7 +164,13 @@ > <b><u>A</u>dd&nbsp;CC:</b> > </td> > <td> >- <input name="newcc" size="30" value="" accesskey="a"> >+ [% PROCESS userselect >+ name => "newcc" >+ value => "" >+ accesskey => "a" >+ size => 30 >+ emptyok => 1 >+ %] Why PROCESS once and then reuse the block defined there? Isn't it better just to process global/userselect.html.tmpl there and have the <select> box appear in the top level of the template file instead of declaring a block there?

> >Index: buglist.cgi > >+ GetUserTable($vars); > I'd much rather this was $vars{'userlist'} = GetUserTable(). i'm passing in $vars as i set 'userlist' and 'usemenuforusers'. how about ($vars{'usemenuforusers'}, $vars{'userlist'}) = GetUserTable() > Can't this be placed somewhere better than globals.pl? Do you see a module > where you think it could fit? User.pm perhaps? yeah, i wasn't sure where to put it either. User.pm is a single bugzilla user object, so i don't think this belongs there. > Why PROCESS once and then reuse the block defined there? Isn't it better just > to process global/userselect.html.tmpl there and have the <select> box appear > in the top level of the template file instead of declaring a block there? because i'm new to template toolkit :) thanks for the review, hopefully i'll get an updated patch this weekend.

Now that bug 251837 has landed, you probably should make the same change on your query to populate the drop-down lists as we have in the wildcard code. It's just an extra join in your getusertable query.

(In reply to comment #8) > i'm passing in $vars as i set 'userlist' and 'usemenuforusers'. how about > > ($vars{'usemenuforusers'}, $vars{'userlist'}) = GetUserTable() You should probably check for the Param in the callsite *or* in the template code you wrote that handles the select block -- the latter may prove to be the cleanest solution. > > Can't this be placed somewhere better than globals.pl? Do you see a module > > where you think it could fit? User.pm perhaps? > > yeah, i wasn't sure where to put it either. > User.pm is a single bugzilla user object, so i don't think this belongs there. Not everything there is a method, if you look closely enough <wink>.

changes globals.pl::GetUserTable($vars) to $vars{'userlist'} = Bugzilla::User::get_userlist() tidies up get_userlist() code, and adds code for user visibility (bug 251837) checks Param("usemenuforusers") in template fixes template PROCESS calls

Comment on attachment 154856 [details] [diff] [review] usemenuforusers v5 oops, swap files

removes *.swp from patch :) this feature is mutually exclusive with user visibility groups, so i've reverted the user list back to all, and updated defparams.

'emptyok' and 'userlist' should probably be detailed in the INTERFACE comments as well?

adds 'emptyok' and 'userlist' to the INTERFACE comments

Comment on attachment 154989 [details] [diff] [review] usemenuforusers v7 Joel, can you check out the defparams.pl bits to ensure this was what you expected. I'd ask you not to request full visibility support here -- we can add it as a next bug, to avoid a longer review cycle; this patch looks pretty much self-contained and clean as it is. General *.cgi comment: I'd rather see us adding elements to $vars closer to where the template is processed than up near GetVersionTable. For instance, in show_bug.cgi, try line 111 or so. >Index: template/en/default/global/userselect.html.tmpl >+ [% IF emptyok %] >+ <option value="">-</option> >+ [% END %] Maybe use just a blank option here. A single hyphen isn't what I'd expect to see, at least. >Index: Bugzilla/User.pm >+my $userlist; >+sub get_userlist { >+ >+ return unless Param('usemenuforusers'); Would removing this check leave us with a more generally useful function? >+ return $userlist if defined $userlist; >+ >+ my $sth = Bugzilla::DB->connect_main->prepare( >+ "SELECT userid FROM profiles WHERE disabledtext=''" >+ ); >+ $sth->execute; My question here is if we need to be worried about visibility -- should any user have a way of seeing all other users? Or is this precisely why you disabled Joel's visibility stuff here? If you have a good rationale, please comment here (with XXXs if we are in agreement that visibility bits should be added here eventually). Marking a conditional r+, with more input from Joel.

Comment on attachment 154989 [details] [diff] [review] usemenuforusers v7 Looks like the consensus on IRC is to take the usemenuforusers check out of User.pm and have this patch use bug 186093 to let the templates decide to call get_userlist themselves. Also, this function can get everything it needs from a single query. Why is it first fetching userids from profiles and then calling User->new which has to go right back to the same rows?

(In reply to comment #17) > Also, this function can get everything it needs from a single query. Why is it > first fetching userids from profiles and then calling User->new which has to go > right back to the same rows? the idea was to resuse the same identity generation code.

Isn't it a bit silly to implement all sorts of cross-checks and restrictions to make this not work with visibility when all youhave to do to make it work properly with visibility is to change.... "SELECT userid FROM profiles WHERE disabledtext=''" to my $query = "SELECT DISTINCT userid" . "FROM profiles "; if (&::Param('usevisibilitygroups')) { $query .= ", user_group_map "; } $query .= "WHERE disabledtext = '' "; if (&::Param('usevisibilitygroups')) { $query .= "AND user_group_map.user_id = userid " . "AND isbless = 0 " . "AND group_id IN(" . join(', ', (-1, @{$user->visible_groups_inherited})) . ") " . "AND grant_type <> " . GRANT_DERIVED; } and then the 2 feature will work together? Doing this will squash a huge number of newsgroup requests from people who cerate 2 bugzilla installations for 2 groups and then struggle to try to recombine the 2 later.

the problem isn't with generating the list of users, it's with how to display them. i guess when i build the select, i can run through it to ensure that the selected user is in the list, and add it if it's missing. bug 186093 simplifies that process a lot.

OK, from the irc conversation, the issue is the concern that the existing selected user might not be on the list. So, we can return all of the users in the list and indicate to the template which ones are visible. That means the SQL could be .... my $query = "SELECT DISTINCT userid," . if (&::Param('usevisibilitygroups')) { $query .= " COUNT(group_id) "; } else { $query .= " 1 "; } $query .= "FROM profiles "; if (&::Param('usevisibilitygroups')) { $query .= "LEFT JOIN user_group_map " . "ON user_group_map.user_id = userid AND isbless = 0 " . "AND group_id IN(" . join(', ', (-1, @{$user->visible_groups_inherited})) . ") " . "AND grant_type <> " . GRANT_DERIVED; } $query .= " WHERE disabledtext = '' GROUP BY userid"; if (&::Param('usevisibilitygroups')) { } or we could let the template decide if it is ignoring the visible field and make the query fixed as... "SELECT DISTINCT userid, COUNT(group_id) FROM profiles LEFT JOIN user_group_map ON user_group_map.user_id = userid AND isbless = 0 AND group_id IN(" . join(', ', (-1, @{$user->visible_groups_inherited})) . ") AND grant_type <> " . GRANT_DERIVED . " WHERE disabledtext = '' GROUP BY userid"

Comment on attachment 155272 [details] [diff] [review] usemenuforusers v8 >+ >+ my $sth = Bugzilla::DB->connect_main->prepare($query); >+ $sth->execute; >+ That should be Bugzilla->dbh I'll look at the rest tonight.

Comment on attachment 155272 [details] [diff] [review] usemenuforusers v8 very nice fix the item in comment 23 and r=joel This could be used on substantially larger sites if we had the option of using a javascript picker for multi-selecting CC additons. This does not seem to work with flags, but it does not break them either, so it should not block this landing.

fixes connect_main sillyness

Comment on attachment 155370 [details] [diff] [review] usemenuforusers v9 >Index: template/en/default/global/userselect.html.tmpl >+sub get_userlist { >+ my $self = shift; >+ >+ return $self->{'userlist'} if defined $self->{'userlist'}; >+ >+ my $query = "SELECT DISTINCT login_name, realname,"; >+ if (&::Param('usevisibilitygroups')) { >+ $query .= " COUNT(group_id) "; >+ } else { >+ $query .= " 1 "; >+ } >+ $query .= "FROM profiles "; >+ if (&::Param('usevisibilitygroups')) { >+ $query .= "LEFT JOIN user_group_map " . >+ "ON user_group_map.user_id = userid AND isbless = 0 " . >+ "AND group_id IN(" . >+ join(', ', (-1, @{$self->visible_groups_inherited})) . ") " . I would love it if you could separate this joined statement into a separate variable, but that's a nit. >Index: template/en/default/bug/knob.html.tmpl >+ [% safe_assigned_to = FILTER upper; bug.assigned_to.email; END %] Okay, this part has confused me. Why FILTER upper? The original code has FILTER js FILTER html -- and this seems to match with your code in userselect. Is this correct? I loved the way this turned out. Mark my r+ if you address the issues above; if you're in doubt, you can request r=joel -- the only thing I'm confused about is the FILTER upper (see the HTML generated!) but you may have a good justification I'm not seeing. Thanks for doing this.

> Okay, this part has confused me. Why FILTER upper? The original code has FILTER > js FILTER html -- and this seems to match with your code in userselect. Is this > correct? that's a very good question. i can't believe that after all these attempts, i missed that. it should be FILTER js. i suspect upper was in the mix while i was playing around with filters. grr. sorry about that.

fixes filter to use 'js' instead of 'upper' i also found an issue caused by using PROCESS instead of INCLUDE .. the variables passed into userselect.tmpl were being set as global instead of localised to that call.

*** Bug 52557 has been marked as a duplicate of this bug. ***

can someone please check this one in for me?

checked in for glob good job

Just for the record, the 2.18 patch is *not* authorized for checkin (it's too major of a change for the stable branch), but if you're an admin running a 2.18-based system and want to use it, feel free to apply it to your own system.

applying the "Patch for 2.18" patch to 2.18rc2 using the following syntax: patch -p0 <patch.txt results in numerous errors. I've duplicated this on a live install and test install using a clean gzip extraction of 2.18rc2. C:\temp999\bugzilla-2.18rc2>patch -p0 <patch.txt patching file template/en/default/global/userselect.html.tmpl patching file defparams.pl Hunk #1 FAILED at 1082. 1 out of 1 hunk FAILED -- saving rejects to file defparams.pl.rej patching file Bugzilla/User.pm Hunk #1 FAILED at 21. Hunk #2 succeeded at 713 with fuzz 1. 1 out of 3 hunks FAILED -- saving rejects to file Bugzilla/User.pm.rej patching file template/en/default/bug/edit.html.tmpl Hunk #1 FAILED at 163. Hunk #2 FAILED at 276. 2 out of 2 hunks FAILED -- saving rejects to file template/en/default/bug/edit.h tml.tmpl.rej patching file template/en/default/bug/knob.html.tmpl Hunk #1 FAILED at 96. 1 out of 1 hunk FAILED -- saving rejects to file template/en/default/bug/knob.ht ml.tmpl.rej patching file template/en/default/bug/create/create.html.tmpl Hunk #1 FAILED at 184. Hunk #2 FAILED at 197. 2 out of 2 hunks FAILED -- saving rejects to file template/en/default/bug/create /create.html.tmpl.rej patching file template/en/default/list/edit-multiple.html.tmpl Hunk #1 FAILED at 302. 1 out of 1 hunk FAILED -- saving rejects to file template/en/default/list/edit-m ultiple.html.tmpl.rej C:\temp999\bugzilla-2.18rc2>

note, some succeed if I up the fuzz factor, but many still fail. C:\temp999\bugzilla-2.18rc2>patch -p0 -F6 <patch.txt patching file template/en/default/global/userselect.html.tmpl patching file defparams.pl Hunk #1 succeeded at 1082 with fuzz 3. patching file Bugzilla/User.pm Hunk #1 succeeded at 21 with fuzz 3. Hunk #2 succeeded at 713 with fuzz 1. patching file template/en/default/bug/edit.html.tmpl Hunk #1 FAILED at 163. Hunk #2 FAILED at 276. 2 out of 2 hunks FAILED -- saving rejects to file template/en/default/bug/edit.h tml.tmpl.rej patching file template/en/default/bug/knob.html.tmpl Hunk #1 FAILED at 96. 1 out of 1 hunk FAILED -- saving rejects to file template/en/default/bug/knob.ht ml.tmpl.rej patching file template/en/default/bug/create/create.html.tmpl Hunk #1 FAILED at 184. Hunk #2 FAILED at 197. 2 out of 2 hunks FAILED -- saving rejects to file template/en/default/bug/create /create.html.tmpl.rej patching file template/en/default/list/edit-multiple.html.tmpl Hunk #1 FAILED at 302. 1 out of 1 hunk FAILED -- saving rejects to file template/en/default/list/edit- m ultiple.html.tmpl.rej

By manually editing each file using the data from here: http://bugzilla.mozilla.org/attachment.cgi?id=155815&action=diff and enabling the dropdown parameter setting, this now works properly, so there is a problem with the patch posted here. btw, GREAT enhancement. thanks byron.

(In reply to comment #36) > By manually editing each file using the data from here: > http://bugzilla.mozilla.org/attachment.cgi?id=155815&action=diff > > and enabling the dropdown parameter setting, this now works properly, so there > is a problem with the patch posted here. FWIW, I just applied this to both 2.18rc2 and the soon-to-be 2.18rc3 and it applied without errors. The patch does have Windows line endings in it. The version of patch that's on Mac OS X transparently handled it (it gave me warnings that it was stripping trailing CRs, but it worked). You might need to convert the line endings if your version of patch doesn't deal with it gracefully.

*** Bug 262796 has been marked as a duplicate of this bug. ***

*** Bug 285110 has been marked as a duplicate of this bug. ***

I tried this patch but it didn't seem to work - the patch had some problems the same as with Rob Roth, so i manually edited and checked the files, ran the checksetup.pl then logged in to my Bugzilla install, set usemenuforusers to On, but when i add a new bug there is no difference to the CC box. No errors are thrown. I'm running 2.18.3