Closed
Bug 103953
Opened 23 years ago
Closed 23 years ago
Templatise enter_bug.cgi
Categories
(Bugzilla :: Creating/Changing Bugs, defect, P1)
Tracking
()
RESOLVED
FIXED
Bugzilla 2.16
People
(Reporter: gerv, Assigned: gerv)
References
Details
Attachments
(2 files, 14 obsolete files)
(deleted),
patch
|
gerv
:
review+
justdave
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review |
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
Assignee | ||
Comment 1•23 years ago
|
||
CCing myk. First-shot patch coming up. Gerv
Assignee | ||
Comment 2•23 years ago
|
||
Assignee | ||
Comment 3•23 years ago
|
||
It's basically a rewrite of enter_bug.cgi - so if you want just the file attached, let me know. Gerv
Updated•23 years ago
|
Blocks: bz-template
Comment 4•23 years ago
|
||
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> </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.
Attachment #52815 -
Flags: review-
Assignee | ||
Comment 5•23 years ago
|
||
Assignee | ||
Comment 6•23 years ago
|
||
Assignee | ||
Comment 7•23 years ago
|
||
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
Assignee | ||
Updated•23 years ago
|
Updated•23 years ago
|
Priority: -- → P2
Target Milestone: --- → Bugzilla 2.16
Assignee | ||
Comment 8•23 years ago
|
||
Assignee | ||
Comment 9•23 years ago
|
||
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•23 years ago
|
||
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•23 years ago
|
||
>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.
Assignee | ||
Comment 12•23 years ago
|
||
> 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
Assignee | ||
Comment 13•23 years ago
|
||
> 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
Assignee | ||
Comment 14•23 years ago
|
||
Assignee | ||
Comment 15•23 years ago
|
||
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•23 years ago
|
||
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•23 years ago
|
||
this blocks a blocker, therefore it's a blocker, too.
Severity: normal → blocker
Priority: P2 → P1
Comment 18•23 years ago
|
||
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.
Assignee | ||
Comment 19•23 years ago
|
||
> 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•23 years ago
|
||
It's existence was publically disclosed in the first Bugzilla status update. Basically you run runtests.sh or runtests.sh --verbose.
Assignee | ||
Comment 21•23 years ago
|
||
> 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
Status: NEW → ASSIGNED
Assignee | ||
Comment 22•23 years ago
|
||
Comment 23•23 years ago
|
||
> 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•23 years ago
|
||
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•23 years ago
|
||
- 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?
Assignee | ||
Comment 26•23 years ago
|
||
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
Assignee | ||
Comment 27•23 years ago
|
||
Comment 28•23 years ago
|
||
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•23 years ago
|
||
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>\ \?
Attachment #55776 -
Flags: review-
Comment 30•23 years ago
|
||
> somewhat unfriendly
Beginner's guide for interpreting my comments: Please mentally add a smiley to
the end of every sentence.
Assignee | ||
Comment 31•23 years ago
|
||
> 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
Assignee | ||
Comment 32•23 years ago
|
||
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
Attachment #52815 -
Attachment is obsolete: true
Attachment #53126 -
Attachment is obsolete: true
Attachment #53127 -
Attachment is obsolete: true
Attachment #54449 -
Attachment is obsolete: true
Attachment #54510 -
Attachment is obsolete: true
Attachment #55776 -
Attachment is obsolete: true
Attachment #56367 -
Attachment is obsolete: true
Assignee | ||
Comment 33•23 years ago
|
||
Note to self: Remove debugging code _before_ making patches. Gerv
Attachment #57384 -
Attachment is obsolete: true
Comment 34•23 years ago
|
||
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•23 years ago
|
||
Comment on attachment 57393 [details] [diff] [review] Patch v.8 marking needs-work per ddk's review in the previous comment.
Attachment #57393 -
Flags: review-
Assignee | ||
Comment 36•23 years ago
|
||
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
Assignee | ||
Comment 37•23 years ago
|
||
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
Assignee | ||
Comment 38•23 years ago
|
||
Here's version 9, with the trivial changes requested by the last review. Looking for re-review. Gerv
Attachment #57393 -
Attachment is obsolete: true
Assignee | ||
Comment 39•23 years ago
|
||
This is a 2.16 blocker - Dave, Myk, can you find someone to review it? Gerv
Comment 40•23 years ago
|
||
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.
Assignee | ||
Comment 41•23 years ago
|
||
OK, fixed those two things. Come on, people - just two teeeny r='s? :-) Gerv
Comment 42•23 years ago
|
||
Comment on attachment 61170 [details] [diff] [review] Patch v.9 r=kiko Looks good to me.
Attachment #61170 -
Flags: review+
Comment 43•23 years ago
|
||
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•23 years ago
|
||
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•23 years ago
|
||
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]; }
Assignee | ||
Comment 46•23 years ago
|
||
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
Assignee | ||
Comment 47•23 years ago
|
||
Attachment #61170 -
Attachment is obsolete: true
Comment 48•23 years ago
|
||
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.
Attachment #63122 -
Flags: review-
Comment 49•23 years ago
|
||
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•23 years ago
|
||
$#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?
Assignee | ||
Comment 51•23 years ago
|
||
> 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
Assignee | ||
Comment 52•23 years ago
|
||
So, if neither of those comments actually requires code changes, is this good to go? Gerv
Comment 53•23 years ago
|
||
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•23 years ago
|
||
>> 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•23 years ago
|
||
>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•23 years ago
|
||
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•23 years ago
|
||
>> 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 -%]
Assignee | ||
Comment 58•23 years ago
|
||
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
Assignee | ||
Comment 59•23 years ago
|
||
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•23 years ago
|
||
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•23 years ago
|
||
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?
Assignee | ||
Comment 62•23 years ago
|
||
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•23 years ago
|
||
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•23 years ago
|
||
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.
Assignee | ||
Comment 65•23 years ago
|
||
Note to self: incorporate patch from bug 119671. Gerv
Comment 66•23 years ago
|
||
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•23 years ago
|
||
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?
Assignee | ||
Comment 68•23 years ago
|
||
> 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•23 years ago
|
||
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•23 years ago
|
||
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.
Assignee | ||
Comment 71•23 years ago
|
||
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
Attachment #63122 -
Attachment is obsolete: true
Comment 72•23 years ago
|
||
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.
Attachment #66350 -
Flags: review-
Comment 73•23 years ago
|
||
Index: template/default/entry/enter_bug.tmpl + <input name=short_desc size="60" value="[% short_desc FILTER html %]" /> Use " " around "short_desc"
Assignee | ||
Comment 74•23 years ago
|
||
> 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
Assignee | ||
Comment 75•23 years ago
|
||
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
Attachment #66350 -
Attachment is obsolete: true
Assignee | ||
Comment 76•23 years ago
|
||
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
Attachment #66980 -
Attachment is obsolete: true
Comment 77•23 years ago
|
||
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.
Attachment #66981 -
Flags: review-
Attachment #66981 -
Flags: review+
Assignee | ||
Comment 78•23 years ago
|
||
Fixed the MFORM problem. No urlbase needed. Gerv
Attachment #66981 -
Attachment is obsolete: true
Assignee | ||
Updated•23 years ago
|
Attachment #67138 -
Flags: review+
Assignee | ||
Comment 79•23 years ago
|
||
*** Bug 119671 has been marked as a duplicate of this bug. ***
Comment 80•23 years ago
|
||
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
Attachment #67138 -
Flags: review+
Assignee | ||
Comment 81•23 years ago
|
||
Checked in. Wahey :-) Gerv
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Comment 82•23 years ago
|
||
Here's a couple of XHTML fixes, to be applied after Patch v.14.
Assignee | ||
Comment 83•23 years ago
|
||
(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•23 years ago
|
||
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.
Assignee | ||
Comment 85•23 years ago
|
||
2xr=gerv, checked in. New issues in new bugs, please. :-) Gerv
Updated•12 years ago
|
QA Contact: matty_is_a_geek → default-qa
You need to log in
before you can comment on or make changes to this bug.
Description
•