Closed Bug 251669 Opened 17 years ago Closed 17 years ago

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

Categories

(Bugzilla :: User Interface, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
Bugzilla 2.20

People

(Reporter: glob, Assigned: glob)

References

Details

Attachments

(2 files, 8 obsolete files)

add an option to show users in a drop down menu instead of a text edit field if
there's less than X users.
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
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.
Summary: add an option to show users in a drop down menu instead of a text edit field if there's less than X users → add an option to show users in a drop down menu instead of a text edit field
Attached patch early patch (obsolete) β€” β€” Splinter Review
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.
Attached patch usemenuforusers v3 (obsolete) β€” β€” Splinter Review
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
Attachment #153418 - Attachment is obsolete: true
Attachment #154007 - Flags: review?(kiko)
Comment on attachment 154007 [details] [diff] [review]
usemenuforusers v3

clearing review request, forgot about buglist --> reassign
Attachment #154007 - Attachment is patch: false
Attachment #154007 - Flags: review?(kiko)
Attached patch usemenuforusers v4 (obsolete) β€” β€” Splinter Review
Attachment #154007 - Attachment is obsolete: true
Attachment #154007 - Attachment is patch: true
Attachment #154008 - Flags: review?(kiko)
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?
Attachment #154008 - Flags: review?(kiko) → review-
> >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>.
Attached patch usemenuforusers v5 (obsolete) β€” β€” Splinter Review
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
Attachment #154008 - Attachment is obsolete: true
Attachment #154856 - Flags: review?(kiko)
Comment on attachment 154856 [details] [diff] [review]
usemenuforusers v5

oops, swap files
Attachment #154856 - Flags: review?(kiko)
Attached patch usemenuforusers v6 (obsolete) β€” β€” Splinter Review
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.
Attachment #154856 - Attachment is obsolete: true
Attachment #154914 - Flags: review?(kiko)
'emptyok' and 'userlist' should probably be detailed in the INTERFACE comments
as well?
Attached patch usemenuforusers v7 (obsolete) β€” β€” Splinter Review
adds 'emptyok' and 'userlist' to the INTERFACE comments
Attachment #154914 - Attachment is obsolete: true
Attachment #154914 - Flags: review?(kiko)
Attachment #154989 - Flags: review?(kiko)
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.
Attachment #154989 - Flags: review?(kiko)
Attachment #154989 - Flags: review?(bugreport)
Attachment #154989 - Flags: review+
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?
Attachment #154989 - Flags: review?(bugreport) → review-
(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.
Depends on: 186093
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"



Priority: -- → P3
Hardware: PC → All
Target Milestone: --- → Bugzilla 2.20
Attached patch usemenuforusers v8 (obsolete) β€” β€” Splinter Review
Attachment #154989 - Attachment is obsolete: true
Attachment #155272 - Flags: review?(bugreport)
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.
Attachment #155272 - Flags: review?(kiko)
Attachment #155272 - Flags: review?(bugreport)
Attachment #155272 - Flags: review+
Attached patch usemenuforusers v9 (obsolete) β€” β€” Splinter Review
fixes connect_main sillyness
Attachment #155272 - Attachment is obsolete: true
Attachment #155272 - Flags: review?(kiko)
Attachment #155370 - Flags: review?(kiko)
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.
Attached patch usemenuforusers v10 β€” β€” Splinter Review
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.
Attachment #155370 - Attachment is obsolete: true
Attachment #155582 - Flags: review+
Flags: approval?
*** Bug 52557 has been marked as a duplicate of this bug. ***
Flags: approval? → approval+
can someone please check this one in for me?
checked in for glob

good job
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Attached patch Patch for 2.18 β€” β€” Splinter Review
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.
Flags: approval2.18-
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.
Attachment #155370 - Flags: review?(kiko)
(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
Blocks: 465589
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.