160410 - defparams.pl should support pulldown menus for options

Status: RESOLVED FIXED

(Bugzilla :: Administration, task)

Description

[J. Paul Reed [:preed]]

This is probably a dup, but I couldn't find a bug for it when I searched;
defparams.pl should provide a way for us to create "select box"-type/pulldown
menu options, i.e. a, b, c, or d.

Currently, there are only freeform and on/off options supported.

Comment 1

[J. Paul Reed [:preed]]

Zach informed me this is administration, not installation... so

-> admin

Comment 2

[J. Paul Reed [:preed]]

-> default Admin owner

Comment 3

[J. Paul Reed [:preed]]

Reassigning to myself.

Comment 4

[J. Paul Reed [:preed]]

Attachment: Patch against the 2.16 branch

Initial version of "multi-list" params.

Things to note:

-- A "mailmodule" param has been added to defparams.pl to aid in testing and so
everyone can see how the patch works; that param will be removed before
checking the rest of the code in

-- This patch is for the 2.16 branch, but I should think it'd apply cleanly to
the trunk too (right now, at least)

-- The patch has been moderately tested in terms of switching params, resetting
the param to the default; everything worked as expected.

Comment 5

[J. Paul Reed [:preed]]

Attachment: 2.16 patch, take 2

Damn that was quick.

Added a couple of things I forgot to add to make the checking safer and fixed a
couple of mistakes in the example I give in the comment in defparams.pl

Comment 6

[J. Paul Reed [:preed]]

Attachment: Take 3, aka Paul-is-stupid

crap, Crap, CRAP!

This has nothing to do with templates... so that template mod shouldn't be in
here (although, it does prove that I was testing this on a real running
Bugzilla installation ;-)

Comment 7

[Dave Miller [:justdave]]

Comment on attachment 94001 [details] [diff] [review]
Take 3, aka Paul-is-stupid

nits: this isn't a multi-select (aka list of choices like the statuses on the
query page, choose one or more), it's a single-select (popup menu).

The same code could certainly be used for both.

How about instead of using the first as a default, you have an anonymous array
that contains two anonymous arrays, the first containing a list of the default
values, the second containing the possible choices.  That would allow actual
multi-selects to work.	However, what you've got probably works fine for a
popup/dropdown menu, we should just call it something different. We can make
another bug to do actual multiselects if we need them.

Comment 8

[J. Paul Reed [:preed]]

Attachment: Take 4

It slices, it dices, it does multi-selects.

Things to note:

-- There are a couple of test DefParams to play with; again, they'll go away
when this gets checked in.

-- The array holding the defaults for the multi-select params uses the array
INDICES to specify the defaults, NOT the values; this turned out to be a LOT
easier in editparams.cgi when we were building the options used for the reset
button. If we didn't do it that way, we'd have to do some weird, inefficient
looping to find the value and then use the index. It's a bit weird to use, but
it works and it's simple (and more importantly, it makes the code simple(r)).

-- Multi-select params will always get set, even if they didn't change; this is
due to the way doeditparams.cgi checks for the same values; because
multi-select needs to use %::MFORM, not %::FORM, there would be a non-trivial
amount of changed code, which I don't want to possibly hork up. This is a small
enough issue that it can be relnoted, if anyone even notices.

Comment 9

[Joel Peshkin]

Almost hate to jump in now, but it may pay to make sure that it is possible to
rename a param by having the old name override the default.

In the current (no-selects) system, this would be done like....

# for compatibility with 2.16 and prior, makeproductgroups defaults to
# the old usebuggroups parameter if it exists
DefParam("makeproductgroups",
         "If this is on, Bugzilla will associate a bug group with each product
in the database.",
         "b",
         $::param{'usebuggroups'} || 0);

It would be a good thing if the syntax for DefParam with selects also permitted
the default to be taken from the old param if defined.

Any chance of making the default in the select an argument that is capable of
accepting the old parameter (or something that can be readily obtained from the
old parameter) as a default??

Comment 10

[J. Paul Reed [:preed]]

For a single select, that would be easy to do. We'd have to add in a second
'default array' like the multi-select list has, so it would the DefParam call
would look something like:

# s -- A list of values, with one selectable (shows up as a select box)
#      To specify the list of values, make the default value a reference to
#      an anonymous array with your default value as the first option, i.e.:
#       DefParam("multitest", "A list of options", "m", [ ['a','b','c'], 'a'],
#                \&check_multi);
#      
#      Here, 'a' is the default option, and 'b' and 'c' are other possible
#      options, but that's it!

For a multi-select, it would be more difficult. Assuming you're converting an
old option, and it was set to 'a' or something, you could still have it be one
of the default options in the array... the only issue there is you'd have to
find the array index. Maybe we could write a helper function for it, but I'm
more curious as to whether or not that will come up at all, and if so, with
multi-select boxes.

By the very nature of multi-selects, I don't think it will... in other words,
there's never been a way to do multiple options before, so I don't see how you'd
really need to convert an old option. But maybe I'm being dense.

Comment 11

[Joel Peshkin]

From conversation on IRC:(conclusion)
   Using older params to set default on new params could be implemented in the
future if/when it comes up by using a subroutine that takes the old value as
args and returns the 2 arrays that this patch uses.   No need to hold up this
patch to implement that.

-Joel

Comment 12

[J. Paul Reed [:preed]]

Attachment: Take 5

Same as Take 4, except it fixes the problem Joel mentions in bug 9; the
declaration of default options is much cleaner now, and the changes to the
patch were so small (and they really did help clean up the code) that it made
sense to just make them, even if we never used them.

Looking at the issues Joel raised also reminded me that Param() needs to be
modified to... the other patches were handling the eval()uation of the string
themselves, and this is wrong.

One other caveat to add: it seems both single selects and multi-selects get
reset even though they don't change; again, this is due to the way we check the
old values... but I don't think it's a big enough issue to worry about it...
correct me if I'm wrong.

Anyone else want any features while I'm still in a good mood?! :-)

Comment 13

[Joel Peshkin]

Comment on attachment 94093 [details] [diff] [review]
Take 5


This is very close, but id does not handle the case where the multi-select
param is first being added to the data/params file.  What is written intitially
into the file is a sey of ARRAY refs instead of the string.  If this is forced,
it works properly thereafter.



multiselecttest:A list of options; you should only be able to select many
options Reset Content-type: text/html
Software error:

Multi-list param 'multiselecttest' eval() failure; data/params is horked at
globals.pl line 1517.


$::param{'multiselecttest'} = ['ARRAY(0x82c7fac)','ARRAY(0x8537900)'];

Comment 14

[Joel Peshkin]

Any reason not to use Data::Dumper to write out the data/params file?

Comment 15

[J. Paul Reed [:preed]]

Attachment: Take 6

This version writes out the default values if the param is undefined correctly.


Also, I made the error reporting a bit more verbose in case of eval() failure.

Comment 16

[J. Paul Reed [:preed]]

re comment 14, from IRC:

<jp-laptop> oh, re: your other comment about Data::Dumper...
<jp-laptop> I have no idea... but if so, that's another bug. :-)

Comment 17

[Joel Peshkin]

Comment on attachment 94214 [details] [diff] [review]
Take 6

Hey.. this works.  Gets my r= now.

Still should get a second review

Comment 18

[J. Paul Reed [:preed]]

Mr. Justdave; r2=, bitte?

Comment 19

[Dave Miller [:justdave]]

Comment on attachment 94093 [details] [diff] [review]
Take 5

denying review on needs-work patch

Comment 20

[Bradley Baetz (:bbaetz)]

Comment on attachment 94214 [details] [diff] [review]
Take 6

>+DefParam("multiselecttest",
>+         "A list of options; you should only be able to select many options",
>+         "m",
>+         [ ['a','b','c','d','e'], ['a', 'e'] ],
>+         \&check_multi);
>+
>+DefParam("singleselecttest",
>+         "A list of options; you should only be able to select one",
>+         "s",
>+         [ ['a','b','c','d'], 'c'],
>+         \&check_multi);

Remove these.

>+        /^m$/ && do {
>+            my $optList = $::param_default{$i}->[0]; #'cause we use it so much 
>+            ## showing 5 options seems like a nice round number; this should
>+            ## probably be configurable; if you care, file a bug ;-)
>+            my $boxSize = $#{$optList} <= 5 ? $#{$optList} + 1 : 5;
>+

If there are six elements, the list will be of size 6, but if there are 7, then
the list will be of size 5
 +, as discussed, use scalar(@array) instead


>Index: globals.pl
>===================================================================
>RCS file: /cvsroot/mozilla/webtools/bugzilla/globals.pl,v
>retrieving revision 1.169.2.9
>diff -u -r1.169.2.9 globals.pl
>--- globals.pl	28 Jul 2002 00:54:28 -0000	1.169.2.9
>+++ globals.pl	6 Aug 2002 19:03:23 -0000
>@@ -1424,7 +1424,14 @@
> sub Param ($) {
>     my ($value) = (@_);
>     if (defined $::param{$value}) {
>-        return $::param{$value};
>+        if ($::param_type{$value} eq "m") {
>+            my $valueList = eval($::param{$value});
>+            return $valueList if (!($@) && ref($valueList) eq "ARRAY");
>+            die "Multi-list param '$value' eval() failure ('$@'); data/params is horked";
>+        }
>+        else {
>+            return $::param{$value};
>+        }
>     }
 This code in there three times - can't you use a sub or something?

Comment 21

[J. Paul Reed [:preed]]

Attachment: v7

Fixes the issues bbaetz raised in his review and on IRC.

Comment 22

[J. Paul Reed [:preed]]

Attachment: v7, take 2

Ok, *that* was a patch to OpenRatings; *this* time around, we should get the
right Bugzilla patch.

Comment 23

[J. Paul Reed [:preed]]

Attachment: v8

Incorporates some of the suggestions bbaetz made on IRC; also, checks the
return values from get_select_list_index()

Comment 24

[Bradley Baetz (:bbaetz)]

Comment on attachment 94823 [details] [diff] [review]
v8


>     if ($::FORM{$i} ne Param($i)) {
>         if (defined $::param_checker{$i}) {
>             my $ref = $::param_checker{$i};
>-            my $ok = &$ref($::FORM{$i});
>+            my $ok = &$ref($::FORM{$i}, $i);

If you change the format for the checker function, please document it

+sub get_select_param_index {
+    my ($pramName, $val) = (@_);
+

'a's are cheap - change this to $paramName.

With that, r=bbaetz x2

Comment 25

[J. Paul Reed [:preed]]

Attachment: Checked in version

This is the (final) version I'm about to checkin.

Comment 26

[J. Paul Reed [:preed]]

Checked in:

Checking in defparams.pl;
/cvsroot/mozilla/webtools/bugzilla/defparams.pl,v  <--  defparams.pl
new revision: 1.83; previous revision: 1.82
done
Checking in doeditparams.cgi;
/cvsroot/mozilla/webtools/bugzilla/doeditparams.cgi,v  <--  doeditparams.cgi
new revision: 1.19; previous revision: 1.18
done
Checking in editparams.cgi;
/cvsroot/mozilla/webtools/bugzilla/editparams.cgi,v  <--  editparams.cgi
new revision: 1.16; previous revision: 1.15
done
Checking in globals.pl;
/cvsroot/mozilla/webtools/bugzilla/globals.pl,v  <--  globals.pl
new revision: 1.186; previous revision: 1.185
done

Comment 27

[J. Paul Reed [:preed]]

Updating the milestone (after the fact) so justdave doesn't have a coronary
again. :-)