251669 - add an option to show users in a drop down menu instead of a text edit field

Status: RESOLVED FIXED

(Bugzilla :: User Interface, enhancement, P3)

Description

[:glob ✱]

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

Comment 1

[:glob ✱]

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.

Comment 2

[:glob ✱]

Attachment: early patch

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.

Comment 3

[:glob ✱]

Attachment: usemenuforusers v3

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 4

[:glob ✱]

Comment on attachment 154007 [details] [diff] [review]
usemenuforusers v3

clearing review request, forgot about buglist --> reassign

Comment 5

[:glob ✱]

Attachment: usemenuforusers v4

Comment 6

[Christian Reis]

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

Comment 7

[Christian Reis]

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?

Comment 8

[:glob ✱]

> >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.

Comment 9

[Joel Peshkin]

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.

Comment 10

[Christian Reis]

(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>.

Comment 11

[:glob ✱]

Attachment: usemenuforusers v5

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 12

[:glob ✱]

Comment on attachment 154856 [details] [diff] [review]
usemenuforusers v5

oops, swap files

Comment 13

[:glob ✱]

Attachment: usemenuforusers v6

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.

Comment 14

[GavinS]

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

Comment 15

[:glob ✱]

Attachment: usemenuforusers v7

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

Comment 16

[Christian Reis]

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 17

[Joel Peshkin]

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?

Comment 18

[:glob ✱]

(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.

Comment 19

[Joel Peshkin]

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.

Comment 20

[:glob ✱]

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.

Comment 21

[Joel Peshkin]

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 22

[:glob ✱]

Attachment: usemenuforusers v8

Comment 23

[Joel Peshkin]

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 24

[Joel Peshkin]

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.

Comment 25

[:glob ✱]

Attachment: usemenuforusers v9

fixes connect_main sillyness

Comment 26

[Christian Reis]

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.

Comment 27

[:glob ✱]

> 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.

Comment 28

[:glob ✱]

Attachment: usemenuforusers v10

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.

Comment 29

[Dave Miller [:justdave]]

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

Comment 30

[:glob ✱]

can someone please check this one in for me?

Comment 31

[Joel Peshkin]

checked in for glob

good job

Comment 32

[:glob ✱]

Attachment: Patch for 2.18

Comment 33

[Dave Miller [:justdave]]

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.

Comment 34

[rob roth]

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>

Comment 35

[rob roth]

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

Comment 36

[rob roth]

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.

Comment 37

[Dave Miller [:justdave]]

(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.

Comment 38

[Dave Miller [:justdave]]

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

Comment 39

[Marc Schumann [:Wurblzap]]

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

Comment 40

[Alistair]

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