103953 - Templatise enter_bug.cgi

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, defect, P1)

Description

[Gervase Markham [:gerv]]

Enter_bug.cgi should be templatised, as the first step towards making the
Bugzilla Helper merely another template for it, or to share the back end.

Gerv

Comment 1

[Gervase Markham [:gerv]]

CCing myk. First-shot patch coming up.

Gerv

Comment 2

[Gervase Markham [:gerv]]

Attachment: Patch v.1 - rough cut

Comment 3

[Gervase Markham [:gerv]]

It's basically a rewrite of enter_bug.cgi - so if you want just the file
attached, let me know.

Gerv

Comment 4

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 52815 [details] [diff] [review]
Patch v.1 - rough cut

>-# I've moved the call to confirm_login up to here, since if we're using bug
>-# groups to restrict bug entry, we need to know who the user is right from
>-# the start.  If that parameter is turned off, there's still no harm done in
>-# doing it now instead of a bit later.  -JMR, 2/18/00
>-# Except that it will cause people without cookies enabled to have to log
>-# in an extra time.  Only do it here if we really need to.  -terry, 3/10/00
>+# If we're using bug groups to restrict bug entry, we need to know who the 
>+# user is right from the start. 
> if (Param("usebuggroupsentry")) {
>     confirm_login();
> }
  ...
>+        if(Param("usebuggroupsentry") && GroupExists($p) && !UserInGroup($p)) {
>+            # If we're using bug groups to restrict entry on products, and
>+            # this product has a bug group, and the user is not in that
>+            # group, we don't want to include that product in this list.
>+            next;

Good idea cleaning up these comments and conditionals.  The conditionals could 
get even more succinct yet readable with postfix syntax:

    confirm_login() if Param("usebuggroupsentry");
    next if Param("usebuggroupsentry") && GroupExists($p) && !UserInGroup($p);


>+        $template->process("entry/no_products.atml", $vars)
>+          || DisplayError("Template process failed: " . $template->error());          

This and the other error messages employed by the script (no_components.atml,
permission_denied.atml) should use &DisplayError.  Note that this means you 
need to return HTTP headers just before processing the other templates rather 
than at the beginning of the script, since &DisplayError returns its own headers.


> sub formvalue {
>     my ($name, $default) = (@_);
>-    if (exists $::FORM{$name}) {
>-        return $::FORM{$name};
>-    }
>-    if (defined $default) {
>-        return $default;
>-    }
>-    return "";
>+    return $::FORM{$name} || $default || "";
>+#    if (exists $::FORM{$name}) {
>+#        return $::FORM{$name};
>+#    }
>+#    if (defined $default) {
>+#        return $default;
>+#    }
>+#    return "";

Excellent.  This should be tested and the old version removed from comments.


>             /Mozilla.*\(.*;.*; OSF.*\)/     && do {return "OSF/1";};
>             /Mozilla.*\(.*;.*; Linux.*\)/   && do {return "Linux";};
>             /Mozilla.*\(.*;.*; SunOS 5.*\)/ && do {return "Solaris";};
>+            /Mozilla.*\(.*;.*; SunOS.*\)/   && do {return "SunOS";};
>             /Mozilla.*\(.*;.*; SunOS.*\)/   && do {return "SunOS";};
>-            /Mozilla.*\(.*;.*; HP-UX.*\)/   && do {return "HP-UX";};
>             /Mozilla.*\(.*;.*; BSD\/OS.*\)/ && do {return "BSDI";};
>             /Mozilla.*\(.*;.*; FreeBSD.*\)/ && do {return "FreeBSD";};
>             /Mozilla.*\(Win16.*\)/          && do {return "Windows 3.1";};

The SunOS line is duplicated in this patch.  Perhaps you were using it
as a template for another platform but forgot to modify the template?


>+$vars->{'assign_elem'} = GeneratePersonInput('assigned_to', 1,
>+$vars->{'priority_popup'} = make_popup('priority', \@::legal_priority,
>+                                formvalue('priority', Param('defaultpriority')), 0);

GeneratePersonInput and make_popup are better implemented as TT filters.
An even better approach would be to use the CGI TT plugin.


>+$vars->{'reporter'} = $::COOKIE{'Bugzilla_login'};
>+$vars->{'user_agent'} = $ENV{'HTTP_USER_AGENT'};
>+$vars->{'product'} = $product;
>+$vars->{'val_product'} = value_quote($product);

Strings can be escaped inside templates using the "html" filter:

[% product FILTER html %]

This works for attribute values in addition to content because that filter
escapes quotation marks in addition to &, <, and >.


>+$vars->{'url_product'} = url_quote($product);

A "url" filter is not yet available but likely to become part of the next
TT version.  In the meantime, make url_quote into a filter and use it from
within your template.


>+$vars->{'version_popup'} = Version_element(pickversion(), $product);

The logic in Version_element should really be part of make_popup (or whatever
alternative gets used in its place), since it consists primarily of making sure
the list of versions for this product and the default version are both defined.


>+$vars->{'priority'} = value_quote(Param('defaultpriority'));

-> [% Param('defaultpriority') FILTER html %]


>+  <tr>
>+    <td>&nbsp;</td>
>+    <td></td><td></td><td></td><td></td><td></td> 

Why not use a "colspan" attribute?


>+[% IF Param('letsubmitterchoosepriority') %]
>+    <TD ALIGN=RIGHT><B><A HREF="bug_status.html#priority">Resolution<br>Priority</A>:</B></TD>
>+    <TD>[% priority_popup %]</TD>
>+[% ELSE %]
>+    <INPUT TYPE=HIDDEN NAME=priority VALUE="[% Param('defaultpriority') %]">

It is dangerous to pass the priority as a hidden field if the user is not supposed
to be able to modify it.  Better to set the priority to the default on the server
after the user has submitted the form.

Comment 5

[Gervase Markham [:gerv]]

Attachment: Patch v.2

Comment 6

[Gervase Markham [:gerv]]

Attachment: enter_bug.cgi for patch v.2

Comment 7

[Gervase Markham [:gerv]]

enter_bug.cgi is completely rewritten, and it's easier to review the file than a
patch, so I've attached that separately. The patch contains the templates and
the changes to globals.pl.

Gerv

Comment 8

[Gervase Markham [:gerv]]

Attachment: Patch v.3 - all in

Comment 9

[Gervase Markham [:gerv]]

This all-in patch should conform to the new coding standards. Please don't try
and review enter_bug.cgi in patch form! :-)

Gerv

Comment 10

[Matthew Tuck [:CodeMachine]]

Comments:

URL is not bold like all the other field names.

Any reason why the default URL "http://" doesn't appear anymore

Product descriptions don't seem to appear, because proddesc is not assigned to
at all.

Why has a url_quote filter been passed?  I thought TT already had the html
filter for that stuff.

Maybe we should change post_bug to take more parameters so we can extend the
template to pass extra data if we wish.

Comment 11

[Myk Melez [:myk] [@mykmelez]]

>Why has a url_quote filter been passed?  I thought TT already had the html
>filter for that stuff.

HTML and URL escaping are different (f.e. URL text needs to have spaces
escaped), and TT does not have a built in URL escaping filter, although there
was some discussion of that on the TT mailing list recently, and it is likely
that the next version will include one.

Comment 12

[Gervase Markham [:gerv]]

> URL is not bold like all the other field names.

It is for me, and there's <b> (now changed to <strong>) tags in the source :-)

> Any reason why the default URL "http://" doesn't appear anymore

Fixed.

> Product descriptions don't seem to appear, because proddesc is not assigned 
> to at all.

It's supposed to be assigned to in GetVersionTable() in globals.pl. We
definitely call this before doing the choose_product.atml template, so I'm not
sure why they aren't appearing.

> Maybe we should change post_bug to take more parameters so we can extend the
> template to pass extra data if we wish.

Maybe we should - but I want to keep the scope of this bug small. We can do that
later :-) Keywords would be one thing we'd want here.

Gerv

Comment 13

[Gervase Markham [:gerv]]

> Product descriptions don't seem to appear, because proddesc is not assigned 
> to at all.

Found the problem here - I was passing in the hash rather than a reference to
it. Fixed. New versions coming right up.

Gerv

Comment 14

[Gervase Markham [:gerv]]

Attachment: Patch v.4

Comment 15

[Gervase Markham [:gerv]]

matty/myk: do either of you plan to review this? The longer it sits here the
more likely it is that people will make changes to the old version, which causes
problems...

Gerv

Comment 16

[Matthew Tuck [:CodeMachine]]

Initial comments, still reviewing:

> # that disallownew was set for this bug, and so we don't want

s/bug/product/

prodlist doesn't need to be passed to choose_product.atml, you can use
proddesc.keys instead.  Maybe call proddesc prodsanddescs or something.

There is a URL in choose_product.atml, it is HTML quoted, but not URL quoted. 
Shouldn't it be?  Both or just URL quoting, and does the ordering matter?

You have added tabs into the source.  Please run the testing suite before
attaching patches.  Currently enter_bug.atml fails the test but that seems to be
because url_quote isn't passed by the template checker.  You should get Jake to
fix that.

Comment 17

[Dave Miller [:justdave]]

this blocks a blocker, therefore it's a blocker, too.

Comment 18

[Matthew Tuck [:CodeMachine]]

Further comments:

It looks to me like prodlist might be smaller than proddesc in order to avoid
showing confidential products.  In this case keys could be deleted out of
proddesc.

There are actually multiple instances where it looks to me like URLs should be
quoted.

While you're at it, you might want to deal with bug #30348/bug #100100.

There is an error saying something like "no components exist.  you can create
one by doing such and such".  The latter part should only exist if the user can
create components.

I think you should pass the list of possible statuses in any case.  The template
should make status a hidden or read-only field if there's only one.  This will
make it more forward compatible with customised statuses (bug #101179).

See this code:

my $check = ($group_bit == $bit);
   
if(formvalue("maketemplate","") eq "Remember values as bookmarkable template") 
{
    $check = formvalue("bit-$bit",0);
}

Usually we use else clauses.  =)

What is GetVersionList needed for?  It seems to pick the first version on the
list usually.  This is not what we want I'd say - see bug #98439.  It's also
inconsistent with other fields.

There is version detection code in there for Mozilla in pick_version.  However,
as bmo doesn't use this, does it serve a purpose anymore?

Not everything seems to be escaped that should be, eg version.

Comment 19

[Gervase Markham [:gerv]]

> Please run the testing suite before attaching patches.

I would be more likely to if I knew it existed :-) Where's the documentation?

Gerv

Comment 20

[Matthew Tuck [:CodeMachine]]

It's existence was publically disclosed in the first Bugzilla status update. 
Basically you run runtests.sh or runtests.sh --verbose.

Comment 21

[Gervase Markham [:gerv]]

> t/001compile........Can't locate Test/More.pm in @INC

Neither lxr.mozilla.org or www.perldoc.com know anything about a Test/More.pm.

I've addressed some of your other points, but I am reluctant to make significant
code changes to code I don't understand fully. Things like
forwards-compatibility with customised statuses and rearranging the way products
are done can wait; they would make this patch more of a regression risk.

No-one has proposed a better algorithm for getting the initial version (bug
98439). Again, I'd rather see that tackled separately.

New patch coming up.

Gerv

Comment 22

[Gervase Markham [:gerv]]

Attachment: Patch v.5

Comment 23

[Matthew Tuck [:CodeMachine]]

> I've addressed some of your other points, but I am reluctant to make
> significant code changes to code I don't understand fully.

Then let's make you understand them fully.  query.cgi should pass a list of
allowed initial statuses to the template, and the template should allow choosing
one of them.  How query.cgi does this later is irrelevant.  I don't know myself,
I only want to prepare the interface.  post_bug.cgi is already ready.

As for proddescs, I don't see what's hard to understand?  If the product is
confidential, delete it from proddescs?

> Things like forwards-compatibility with customised statuses and rearranging
> the way products are done can wait; they would make this patch more of a
> regression risk.

I am not really concerned with regression risk here, this is easily testable. 
Normally I'd say sure, do things in small increments, but we know this interface
is going to change in the short or mid term.  I intend on doing customised
statuses for 2.18.  It seems silly to do it wrong, because whenever template
interfaces change, administrators need to update their custom templates.  I
would like to avoid that wherever possible.  In this case custom query.atml
files would need to be updated from 2.16 to 2.18.

> No-one has proposed a better algorithm for getting the initial version.

This is not true.  The only proposal has been to have _no_ default, which to me,
seems the only sensible option.

Comment 24

[Zach Lipton [:zach]]

perldoc.com knows about Test::More for me, see http://www.perldoc.com/
cpan/Test/More.html. It's part of the Test::Simple package, try installing 
that if you can't install Test::More from CPAN. It's part of the standard 
module dist. but only in the latest versions of perl (it's a fairly new 
module). Note that the version on perldoc.com is pretty old, cpan.org has 
the latest though. 

Comment 25

[Matthew Tuck [:CodeMachine]]

-        return $value;
+       return $value;

Bad indent.

+    DisplayError("Sorry; you do not have the permissions necessary to " .
+                 "enter a bug against this product.\n");
 ...
+    my $error = "Sorry. There needs to be at least one component for this " .
+                "product in order to create a new bug. ";

Comma?

Comment 26

[Gervase Markham [:gerv]]

Matthew:
> Then let's make you understand them fully.

Both in this bug and the query.cgi one the tone of your review comments has been
somewhat unfriendly. Please could you consider the effect your words have on
other volunteers on this project.

New patch coming up.

Zach: I have Perl 5.6.0, shipped with Red Hat 7.1 - hardly antiquated. Perhaps
we could add Test::Simple to the optional section of checksetup.pl if it is
needed to run the tests?

Gerv

Comment 27

[Gervase Markham [:gerv]]

Attachment: Patch v.6

Comment 28

[Myk Melez [:myk] [@mykmelez]]

My review of v5 of the patch is on a machine at work to which I no longer have
access, but I'll update it for v6 and post it Monday.

Comment 29

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 55776 [details] [diff] [review]
Patch v.5

Changed my mind.  I took a look at v6 and it looks like most of my issues
are still there, so I'll post my review of v5 of the patch and re-review v7
when it comes out.

enter_bug.cgi:

Make sure you take the issue in bug 108516 into account.


># If we're using bug groups to restrict bug entry, we need to know who the
># user is right from the start.
>confirm_login() if (Param("usebuggroupsentry"));
  ...
>confirm_login();

There is a redundancy here.  If the user has already selected a product, then these two lines execute essentially one after the other (besides the "select a product" section and the function declarations there is only one line of code separating them).


>        next if (Param("usebuggroupsentry") &&
>                 GroupExists($p) &&
>                 !UserInGroup($p));

Per the style guide, this line should be broken up before the "&&" operators (or not broken up at all; it isn't very long).


>	DisplayError("'" . html_quote($product) . "' is not a valid product.");


I noticed a tab on this line.  Get rid of it or you will turn the tree orange, which is a very nice color for leaves, not trees.

>    my $group_bit = 0;

The value of this variable should be the string '0'.


>        $error .= "Go <a href=\"editcomponents.cgi\">here</a> " .
>                  "to create a new component.\n";

Yuck, the words "click" and "here", used solely or in combination, should never be used as links.  How about: "<a href=\"editcomponents.cgi\">Create a new component.</a>"


>        $vars->{'status'} = \[$unconfirmedstate, "NEW"];

Square brackets already return a reference to an array, so no need to use backslash to get a reference.


The rest of these issues are style recommendations and will not block my review:

>    foreach my $p (sort(keys %::versions)) {

It is easier to read a chain of built-in functions if you follow a consistent style re: parentheses (i.e. either "sort(keys(%::versions))" or "sort keys %::versions").


>    if ($value ne "") {
>       return $value;
>    }
(and many other places in the code)

More readable in postfix notation: return $value if $value ne "";


>    if (Param('usebrowserinfo')) {
>        if ($version eq "") {
>            if ($ENV{'HTTP_USER_AGENT'} =~ m@Mozilla[ /]([^ ]*)@) {
>                $version = $1;
>            }
>        }
>    }

Better as a single conditional (i.e. if (foo && bar && baz)).


>$vars->{'opsys'} = \@legal_opsys,

The comma should be a semi-colon.


>        my ($bit, $prodname, $description) = (FetchSQLData());

FetchSQLData() returns a list of values and list context is provided by the parentheses around the variable names, so no need for parentheses around the function call.


>        next unless(($prodname eq $product) ||
>                    (!defined($::proddesc{$prodname})));

While it is true that one should parenthesize when in doubt, there are two unnecessary sets of parentheses here.


enter_bug.atml:

>      <select name="version" size="5">
>        [% FOREACH v = version %]
>          <option value="[% v FILTER html %]"
>            [% " selected" IF v == version_default %]>[% v %]</option>
>        [% END %]
>      </select>

Do what you told me to do in your review of bug 103778 (make a BLOCK for creating select menus). :-)

>        <a href="bug_status.html#status">Initial state:</a>\

\?

Comment 30

[Matthew Tuck [:CodeMachine]]

> somewhat unfriendly

Beginner's guide for interpreting my comments:  Please mentally add a smiley to
the end of every sentence.

Comment 31

[Gervase Markham [:gerv]]

> Beginner's guide for interpreting my comments:  Please mentally add a smiley to
> the end of every sentence.

No. If you man thm, add thm. Plase tak th tim to mak th tiny ffort it rquires to
be polit.

Please mentally add e's at appropriate places in that sentence.

Gerv

Comment 32

[Gervase Markham [:gerv]]

Attachment: Patch v.7

Drat, you Myk - you've made me rewrite the template again by suggesting blocks
;-)

This is the latest and greatest. Loads of new innovations.

(Love the new attachment upload interface! :-)

Gerv

Comment 33

[Gervase Markham [:gerv]]

Attachment: Patch v.8

Note to self: Remove debugging code _before_ making patches.

Gerv

Comment 34

[David D. Kilzer (ddk)]

In reviewing attachment 57393 [details] [diff] [review] (Patch V.8), I found that it did not apply
cleanly to the latest enter_bug.cgi (v1.56).  Backing up a revision let
the patch apply cleanly.

However, the changes in enter_bug.cgi from v1.55 to v1.56 (fixing
bug 109138) do not seem to have survived through to the latest patch.
I believe the pickos() routine needs to be adjusted in this patch to
make sure nothing was lost from the fix to 109138.  Adding timeless to
the CC list for this bug (hope that's okay).

Also, running the output of the new enter_bug.cgi through 
http://validator.w3.org/ found that the DOCTYPE tag is missing from the
*.atml files.  Please add a DOCTYPE tag similar to the following either
in the *.atml files or as appropriate in the enter_bug.cgi file:

   <!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN">

Finally, after specifying a DOCTYPE of 'HTML 4.01 Transitional', there
are a number of errors that need to be fixed in the HTML itself.  (I
believe you may safely ignore the "unknown entity" errors that are
part of a URL within double quotes.  I think those are false alarms,
or I don't know the proper way to fix them.)

Comment 35

[Dave Miller [:justdave]]

Comment on attachment 57393 [details] [diff] [review]
Patch v.8

marking needs-work per ddk's review in the previous comment.

Comment 36

[Gervase Markham [:gerv]]

I won't be able to refresh this patch for a while; however, the issues raised so
far by ddk do not block further review. I will be somewhat annoyed if I fix
those in two weeks time, and then someone comes along and points out a load more
issues which are obvious also in the current patch.

Gerv

Comment 37

[Gervase Markham [:gerv]]

No other .atml files have DOCTYPE tags; this is something which is best placed
in the header template, when all the Bugzilla templates do, in fact, conform to
that DOCTYPE.

Refreshed patch coming up; presumably, as no-one has made any other comment on
it in these last few weeks, it will sail through review. I will not be happy if
that's not the case...

Gerv

Comment 38

[Gervase Markham [:gerv]]

Attachment: Patch v.9

Here's version 9, with the trivial changes requested by the last review.
Looking for re-review.

Gerv

Comment 39

[Gervase Markham [:gerv]]

This is a 2.16 blocker - Dave, Myk, can you find someone to review it?

Gerv

Comment 40

[Andreas Franke (gone)]

I have not reviewed it, but I have tested it: http://bugzilla.mathweb.org:8000/
I had to put the template files in their respective directories, but that can be
taken care of at check-in time. 

The only problem I found was that the default version was not selected. I
changed the $default{'version'} value in enter_bug.cgi to "unspecified":

$default{'version'} = "unspecified"; # should be Param('defaultversion')

and I added " selected" to the default version <option> in enter_bug.tmpl, like
it was already done there for components:
        <option value="[% v FILTER html %]"
          [%- " selected" IF v == default.version %]>[% v FILTER html
%]</option>

Otherwise it seems to work fine. Let's get it in.

Comment 41

[Gervase Markham [:gerv]]

OK, fixed those two things. Come on, people - just two teeeny r='s? :-)

Gerv

Comment 42

[Christian Reis]

Comment on attachment 61170 [details] [diff] [review]
Patch v.9

r=kiko

Looks good to me.

Comment 43

[Christian Reis]

Comment on attachment 61170 [details] [diff] [review]
Patch v.9


>+    if(Param("usebuggroups") && GroupExists($product)) {
>+        SendSQL("SELECT bit FROM groups ".
>+                "WHERE name = " . SqlQuote($product) . " " .
>+                "AND isbuggroup != 0");
>+        ($group_bit) = FetchSQLData();
>+    }

Hmmm. Isn't there a convenience function for this? How odd,
I thought there was.. Ok.

This patch was hard to read through, lots of changes. I vote
for check it in and pick up regressions later if they happen.

Comment 44

[David Lawrence [:dkl]]

I encountered the following problem when applying patch v9. It creates a the
choose_product.tmpl file in the bugzilla root instead of in the proper template
directories. Also enter_bug.cgi is looking for the choose_product.tmpl file in
template/default/global/. Would it not be better to put it in
template/default/entry/ along with the enter_bug.tmpl?

Comment 45

[David Lawrence [:dkl]]

Also when there is only one component defined for a particular product, that
component is automatically selected in enter_bug.cgi. The same should be true
for the version. 

Example:

if (1 == @{$::versions{$product}}) {
    # Only one version; just pick it.
    $::FORM{'version'} = $::versions{$product}->[0];
}

Comment 46

[Gervase Markham [:gerv]]

choose_product will be used elsewhere (for example, the forthcoming
describecomponents.cgi templatisation), so I put it in global.

> Also when there is only one component defined for a particular product, that
> component is automatically selected in enter_bug.cgi. The same should be true
> for the version. 

I have a better plan. We should always select the last version in the list. That
should be the latest one, which is as good a default as any, given that
Param('defaultversion') was a figment of my fevered imagination.

New patch coming up, with a tweak or two. Thanks for reviewing it!

Gerv

Comment 47

[Gervase Markham [:gerv]]

Attachment: Patch v.10

Comment 48

[David Lawrence [:dkl]]

Comment on attachment 63122 [details] [diff] [review]
Patch v.10

>+      <select name="component" size="5">
>+        [%- FOREACH c = component_ %]
>+          <option value="[% c FILTER html %]"
>+            [% " selected" IF c == default.component_ %]>[% c FILTER html -%]
>+          </option>
>+        [%- END %]
>+      </select>

The underscore (_) after default.component needs to be removed for this to 
work properly.

Comment 49

[David Lawrence [:dkl]]

Comment on attachment 63122 [details] [diff] [review]
Patch v.10


>+# Default version is the last one in the list (hopefully the latest one).
>+# Eventually maybe each product should have a "current version" parameter.
>+$vars->{'version'} = $::versions{$product} || [];
>+$default{'version'} = $vars->{'version'}->[$#{$vars->{'version'}}];

I am not sure but I thought the $#array syntax is being deprecated in newer 
versions of Perl in favor of @array for getting the number of elements in an
array.
This also seems to work well and is less typing ;)

$default{'version'} = $vars->{'version'}->[-1];

Comment 50

[Dave Miller [:justdave]]

$#array doesn't tell you the number of elements, it tells you the index of the
last element.  There's a subtle difference. :)  It's also usually one-off from
the number of elements, since arrays tend to be zero-based.  scalar(@array) will
tell you the number of elements.  Were we looking for the index of the last
element here perhaps? or was the wrong one used?

Comment 51

[Gervase Markham [:gerv]]

> The underscore (_) after default.component needs to be removed for this to 
> work properly.

That's definitely there on purpose. "component" is a reserved word, so I had to
use "component_" for the component list. I tried "components" but Myk didn't
like the inconsistent plurality with all the other ones. "component_" is what I
used in query.cgi.

As for the array thing, I didn't know about the deprecation. My Camel Book says
that scalar(@array) only works on simple arrays and not list values of other
sorts (whatever that means.) I can't remember if I tried it here.

Your -1 trick looks good, although it is a touch counter-intuitive.

Gerv

Comment 52

[Gervase Markham [:gerv]]

So, if neither of those comments actually requires code changes, is this good to go?

Gerv

Comment 53

[Dave Miller [:justdave]]

My personal preference for the version, platform, and opsys fields, is to have
them BLANK, and have post_bug.cgi complain if they aren't filled in.

This way we're more likely to have them accurate because the user has to
consciously choose one.  As it is now, half of the bugs get the wrong data on
them because people blindly accept the defaults, and those fields have more or
less become useless.

Comment 54

[David Lawrence [:dkl]]

>> The underscore (_) after default.component needs to be removed for this to 
>> work properly.
>
>That's definitely there on purpose. "component" is a reserved word, so I had to
>use "component_" for the component list. I tried "components" but Myk didn't
>like the inconsistent plurality with all the other ones. "component_" is what I
>used in query.cgi.

Hmm, I understand why you now need to use the underscore, but I swear that when
I was testing the patch, I was not able to get  the single component to be
selected automatically without removing the underscore and then it would work
properly. The thing that made me think of it was the line in enter_bug.cgi where
you are setting the default value for the components.

$default{'component'} = formvalue('component');

You did not include an underscore there and you were then referencing
default.component_ which did not match up. Changing the line to

$default{'component_'} = formvalue('component');

also fixed it for me.

Also the line:

elsif (1 == @{$::components{$product}}) {
   [..snip..]

should be 

elsif (1 <= @{$::components{$product}}) {

or it will not work for more than one component.

>As for the array thing, I didn't know about the deprecation. My Camel Book says
>that scalar(@array) only works on simple arrays and not list values of other
>sorts (whatever that means.) I can't remember if I tried it here.

Yeah, I was mistaken with my earlier comments. @array returns the number of
elements and $#array returns the index of the last element which is what we want
in this case.

>Your -1 trick looks good, although it is a touch counter-intuitive.

Yeah, scratch that idea ;)

Comment 55

[David Lawrence [:dkl]]

>My personal preference for the version, platform, and opsys fields, is to have
>them BLANK, and have post_bug.cgi complain if they aren't filled in.

I agree with this while making sure not to break bookmarked templates.

Comment 56

[Andreas Franke (gone)]

Please make sure that if there's only one version in the list, it will be
preselected. This is necessary for installations that don't use the version
field.

Comment 57

[Christopher Aillon (sabbatical, not receiving bugmail)]

>> The underscore (_) after default.component needs to be removed for this to 
>> work properly.

>That's definitely there on purpose. "component" is a reserved word, so I had to
>use "component_" for the component list. I tried "components" but Myk didn't
>like the inconsistent plurality with all the other ones. "component_" is what I
>used in query.cgi.

Gerv: use this instead then:

[% " selected" IF c == ${"default.component"} %]>[% c FILTER html -%]

Comment 58

[Gervase Markham [:gerv]]

About the whole "component_" thing - sorry, I didn't read your problem report
carefully enough.

The situation is:
"component" is a reserved word, so I can't pass in the component list as
$vars->{'component'}. So I use $vars->{'component_'}. 

I have a choice of $vars->{'default'}{'component'} or
$vars->{'default'}{'component_'} . I should probably go with _ for consistency.
But whichever one I choose, it has to match in the CGI and the template.

So I have messed it up, and will fix it.

Version, platform, opsys: so the plan is to select it if there's only one,
otherwise to have no selection, and get post_bug to complain? Are we always
guaranteed to have at least one? What do we do if the field in the DB is empty/null?

Gerv

Comment 59

[Gervase Markham [:gerv]]

post_bug.cgi currently only complains about missing version etc. if
strictvaluechecks is turned on. Is that good enough? Otherwise, if we have the
default behaviour in the multiple case to be no selection, people could easily
end up with NULL fields in the bugs table for things like version. Wouldn't that
be bad?

Apart from this issue, and component_ which I've fixed, I think this patch is done.

Gerv

Comment 60

[David Lawrence [:dkl]]

Latest Temple.pm (version 2.05+) has built in filter for url escaping that I
have made the suggestion in another bug report to use instead of importing
routines to do the same thing.

[% value FILTER uri %]

I suggest removing the following

+    FILTERS =>
+    {
+      url => \&url_quote,
+    } 

This of course puts changes the version dependency for Template-Toolkit in
checksetup.pl but I figure checksetup.pl should reflect latest stable release of
all dependencies when 2.16 ships anyway.

Otherwise I will apply the latest patch and check it out today.

Comment 61

[David Lawrence [:dkl]]

Maybe I am going crazy but with the latest v10 patch, it is putting te
choose_product.tmpl file in the bugzilla root instead of in the proper
template/default/global dir causing an error to be generated that the template
cannot be found. I am doing

patch -p0 < enter_bug_template.patch 

which generates

patching file enter_bug.cgi
patching file choose_product.tmpl
patching file template/default/entry/enter_bug.tmpl

After I move the choose_product.tmpl into the proper dir it works again. Is
anyone else testing this observing this behaviour?

Comment 62

[Gervase Markham [:gerv]]

Patch is misbehaving. Ignore it. What do you think of the code? :-)

Let's leave the URI filtering in at the moment; it's no big deal. When all the
Bugzilla developers have upgraded, then we can remove it.

Gerv

Comment 63

[Bradley Baetz (:bbaetz)]

Here's the comments I wrote while bz was down last night:

In addition to the other comments:

>Index: enter_bug.cgi
>===================================================================
>RCS file: /cvsroot/mozilla/webtools/bugzilla/enter_bug.cgi,v
>retrieving revision 1.57
>diff -u -r1.57 enter_bug.cgi
>--- enter_bug.cgi      2001/11/17 23:05:48     1.57
>+++ enter_bug.cgi      2001/12/31 11:20:52
>@@ -1,4 +1,5 @@
> #!/usr/bonsaitools/bin/perl -w
>+

(nit; extra line)

> # -*- Mode: perl; indent-tabs-mode: nil -*-
> #
> # The contents of this file are subject to the Mozilla Public

>+use vars qw(
>+  $unconfirmedstate
>+  %COOKIE
>+  @legal_opsys
>+  @legal_platform
>+  @legal_priority
>+  @legal_severity
>+);

why not @::legal_opsys (And so on), like the old sub needed? (It works, so 
I'm just curious)

>+my $template = Template->new(
>+{
>+    # Colon-separated list of directories containing templates.
>+    INCLUDE_PATH => "template/custom:template/default" ,
>+    # Allow templates to be specified with relative paths.
>+    RELATIVE => 1,
>+    PRE_CHOMP => 1,

We really need a central place for default filter parameters. Is there a 
bug on that?

>+    FILTERS =>
>+    {
>+      url => \&url_quote,
>+    } 

As of this afternoon, we require TT 2.06, so you should use the builtin 
uri filter instead. (and change
url->uri in the template files)

>+
>+        # Special hack: "0" in proddesc means disallownew was set.
>+        # Also, if we're using bug groups to restrict entry on products,
>+        # and this product has a bug group, and the user is not in that
>+        # group, we don't want to include that product in this list.
>+        if ((defined $::proddesc{$p} && $::proddesc{$p} eq '0') 
>+            || (Param("usebuggroupsentry") 
>+                && GroupExists($p) 
>+                && !UserInGroup($p)))
>+        {
>+            delete $::proddesc{$p};

do you really want to delete from the global $::proddesc? It seems yuck, 
and may cause problems later,
if we ever get modperl working. Maybe that doesn't matter for now, though.


>+elsif (1 == @{$::components{$product}}) {

[moz removed the rest of the patch here; the remainder is a biut out of 
context because convincing mozilla
to paste at the end of a test area is Fun]

+++ template/default/entry/enter_bug.tmpl       31 Dec 2001 11:18:36 -0000
@@ -0,0 +1,256 @@
+[%# The contents of this file are subject to the Mozilla Public

As an aside, are we not adding the LICENSE INFO headers in bugzilla?

+ [%# We can't use the select block in these two cases for various 
reasons. %]

Which are?

Comment 64

[Bradley Baetz (:bbaetz)]

Oh, one more thing. I have one group created, but the new code doesn't let me
enter a bug into that group. The old code does. A quick glance doesn't find
anything obvious, though.

Comment 65

[Gervase Markham [:gerv]]

Note to self: incorporate patch from bug 119671.

Gerv

Comment 66

[Bradley Baetz (:bbaetz)]

Oh, and the following line from the tempalte needs to be a param, rather than
hardcoding enter_bug.cgi in.

      <a href="enter_bug.cgi?product=[% p FILTER uri %]">

Comment 67

[Bradley Baetz (:bbaetz)]

OK, her's a strange one. In mozilla (but not ns4), the product selection screen
will only display the colon after the first product name. Is this invalid html,
or is this a really stramge mozilla bug?

Comment 68

[Gervase Markham [:gerv]]

> why not @::legal_opsys (And so on), like the old sub needed? (It works, so 
> I'm just curious)

It looks uglier. I dunno :-)

> We really need a central place for default filter parameters. Is there a 
> bug on that?

Myk's on the case. But we can probably do this last.

> >+            delete $::proddesc{$p};

> do you really want to delete from the global $::proddesc? It seems yuck, 
> and may cause problems later,
> if we ever get modperl working. Maybe that doesn't matter for now, though.

It doesn't matter for now; I don't assume it to be complete later on.

> As an aside, are we not adding the LICENSE INFO headers in bugzilla?

No. We aren't attempting to relicense Bugzilla.

> + [%# We can't use the select block in these two cases for various 
> reasons. %]

> Which are?

The Select block relies on the name of the form field being the same as the name
of the variable which contains the data (e.g. "version"). This won't work for
"component", because the form field has to be named "component" but the variable
is named "component_" because there's already a TT-internal "component" variable
(annoyingly.)

> Oh, one more thing. I have one group created, but the new code doesn't let me
> enter a bug into that group. The old code does. A quick glance doesn't find
> anything obvious, though.

How did this work before?

Gerv

Comment 69

[Bradley Baetz (:bbaetz)]

The groups stuff is because you have $vars->{'group'} = \@groups; but need
$vars->{'groups'} = \@groups;

Also, in the orignal code if a product has one one component, it is
automatically highlghted in the select list. This isn't the case in your code.

Comment 70

[Andreas Franke (gone)]

For the introduction of a bugzilla namespace (bug 87411) it would probably be
better to keep the :: for global variables, so you can find most of them easily.

Comment 71

[Gervase Markham [:gerv]]

Attachment: Patch v.11

Here's the new version - fixes all commented-on bugs, and incorporates the
patch from bug 119671. Looking for re-review from kiko, bbaetz, dkl, caillon or
others.

Gerv

Comment 72

[Bradley Baetz (:bbaetz)]

Comment on attachment 66350 [details] [diff] [review]
Patch v.11

>Index: enter_bug.cgi
>===================================================================
OK, quick things I noticed.

>+print "Content-type: text/html\n\n";
>+$template->process("entry/enter_bug.tmpl", $vars)
>+  || DisplayError("Template process failed: " . $template->error());          
>+exit;

DisplayError prints the content-type too, so if you get an erro in teh template
(see below), then you get that extra line.

>Index: template/default/entry/enter_bug.tmpl
>===================================================================
>RCS file: template/default/entry/enter_bug.tmpl
>diff -N template/default/entry/enter_bug.tmpl
>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ template/default/entry/enter_bug.tmpl	24 Jan 2002 22:14:47 -0000

>+  <tr>
>+    <td></td>
>+    <td colspan="3">
>+    [% IF group %]

I always get this text, because I have a group defined in my installation, even
though
the current user is not a part of it.

This should be:

[% IF group && group.first %], I think.

[choose_product.tmpl]

The url filter should be a uri filter instead.

Comment 73

[Tobias Burnus]

Index: template/default/entry/enter_bug.tmpl
+      <input name=short_desc size="60" value="[% short_desc FILTER html %]" />
Use " " around "short_desc"

Comment 74

[Gervase Markham [:gerv]]

> DisplayError prints the content-type too, so if you get an erro in teh template
> (see below), then you get that extra line.

We do this everywhere we execute a template. I'm in discussions with Myk about
fixing this problem in a better way. In the mean time, leave it.

Gerv

Comment 75

[Gervase Markham [:gerv]]

Attachment: Patch v.12

Nits picked. I'm sure there are a few more nits in there, but can we please
stick to functional deficiencies? We'll never get 2.18 at this rate.

Gerv

Comment 76

[Gervase Markham [:gerv]]

Attachment: Patch v.13

Hmm. Perhaps not having the template files might be considered a functional
deficiency. **** CVS. And I thought version 13 would be the unlucky one...

Gerv

Comment 77

[Bradley Baetz (:bbaetz)]

Comment on attachment 66981 [details] [diff] [review]
Patch v.13

Does the tests failing count as functional? ;)

'Name "main::MFORM" used only once'

Oh, and should the choose product filter (or the .cgi) include
Param('urlbase')?

Anyway, I'm happy with this, so r=bbaetz if you fix that warning. You can carry
it onto the next patch if thats all you change.

Comment 78

[Gervase Markham [:gerv]]

Attachment: Patch v.14

Fixed the MFORM problem. No urlbase needed.

Gerv

Comment 79

[Gervase Markham [:gerv]]

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

Comment 80

[Dave Miller [:justdave]]

Comment on attachment 67138 [details] [diff] [review]
Patch v.14

r= justdave

works as expected, I haven't been able to break it yet, here.

I had to back out timeless's last checkin to get the patch to apply, but cvs
successfully merged his checkin back in with no conflicts

> cvs -q up -r1.58 enter_bug.cgi
> patch -p0 < 67138.diff
> cvs -q up -A

Comment 81

[Gervase Markham [:gerv]]

Checked in. Wahey :-)

Gerv

Comment 82

[Ville Skyttä]

Attachment: XHTML validity fixes

Here's a couple of XHTML fixes, to be applied after Patch v.14.

Comment 83

[Gervase Markham [:gerv]]

(This should probably be a new bug, but never mind now.)

These don't break older browsers, do they?

Will the ä character cause trouble? I remember Häkan Waara having to remove it
from his name in some files once for some reason...

Gerv

Comment 84

[Ville Skyttä]

No, I don't think they break anything.

And the 'ä' character should be ok as long as the charset is ISO-8859-1, -15 
or UTF-8. Of course, it's no problem to change it to 'a' if you're suspicious.

Comment 85

[Gervase Markham [:gerv]]

2xr=gerv, checked in.

New issues in new bugs, please. :-)

Gerv