show_bug.cgi, process_bug.cgi, show_activity.cgi, xml.cgi and bug_form.pl should all be one templatised CGI called show_bug.cgi. This will bring bug activity inline, make XML output the template it was always meant to be, stop people bookmarking process_bug.cgi, and generally make life nicer all round. This will involve a _severe_ spanking. :-) I believe this is the way to go to templatise the bug display code. Gerv

blocks a blocker so it's a blocker.

Maybe it will stop bookmarking process_bug.cgi, but is it really worth it? I suppose it is consistent with most of the product, but it sounds like a major effort, and could easily result in a whopper of a CGI.

Once bug 109530 is fixed, Bug.pm could be quite useful for this template as it already puts most (all?) relevent information into a single hash. Anything that isn't in that has that's needed could (and probably should) be added.

I'm already a long way through this; if someone wants me to use Bug.pm, they'd better explain how, and quick :-) Gerv

#!/usr/bin/perl -wT use strict; use lib "."; use Bug; require "CGI.pl"; my $bug_num = 1; my $user_id = 1; my $bug = new Bug($bug_num, $user_id); print $bug->{'short_desc'} . "\n"; ================================= That's a brief summary :)

OK, so I bit off a bit more than I could chew here; I'm trying to scale back the work to the minimum possible. Stay tuned. Gerv

Here we are. I've: - templatised show_bug (actually bug_form.pl), and turned it into a procedure rather than a "do". - modified a few globals and things to support all this. Templatising process_bug.cgi should wait until this is in; it's a more complex job. Once this is in, I might have another go at combining process_bug and show_bug - it can be done, but it's a big job. This patch is unavoidably tied up with the one for long_list.cgi - they share common code. Gerv

CCing some people. Gerv

Was directed to this bug for a reqest to get the reset button (re)moved from bug 93908. Should be a simple change to make.

When applying the patches for long_list and show_bug (bug 115369) have common blocks ... tho all works out in the end. Except ... this patch for show_bug ends up with no footer.

Sorry ... to explain better ... PutFooter or templatised equivalent is not included, resulting in no footer in the output. Also, in order to template/default/attachment/list.atml to use bug ids to create valid links for 'Create a New Attachment' and 'View All', bugid needs to be added to $vars; or list.atml $vars->{'bug'} = \%bug; $vars->{'bugid'} = $bug{'bug_id'}; $vars->{'user'} = \%user;

OK, so I've added in the "footer" call to the bottom of show_bug.tmpl and removed the Reset button. However, I think that some more reviewing can be done here before a new patch shows up. Come on, someone :-) Gerv

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

Comment on attachment 63034 [details] [diff] [review] Patch v.1 >Index: template/default/show/show_bug.tmpl These should be moved to the "bug" directory per our discussion in n.p.m.webtools (unless you disagree; I didn't see another post from you on this subject in the thread). news://news.mozilla.org:119/3C485A1B.7020605@mozilla.org >+ <td>&nbsp;</td> >+ [% PROCESS select sel = { description => "Platform", >+ name => "rep_platform" } %] >+ <td align="right"> Abstracting out the select code to this extent makes it difficult to see the structure of the table, which is important in templates because we want non-programmers to be able to hack them. It would be better to use the select block to generate only the select itself and add in the field label separately. >+ <a href="describecomponents.cgi?product=[% bug.product %]"> >+ Component</a>: No need to be this severe in limiting yourself to 80-character long lines at the expense of readability. :-) >+ <select name="component"> >+ [% FOREACH x = component_ %] >+ <option value="[% x %]" >+ [% " selected" IF x == bug.${sel.name} %]>[% x %]</option> ${sel.name} -> component >+ <a href="bug_status.html#bug_status">Status</a>: There is no "#bug_status" anchor in the bug_status.html file. >+[%# *** AssignedTo Milestone QAContact URL Whiteboard Keywords *** %] Don't forget summary. Also, why group "assigned to" and "milestone" with the fields below it rather than the fields above it whose presentational structure they share? >+ <td></td> >+ <td></td> >+ <td>&nbsp;</td> <td colspan="3">? >+[%# *** Attachments *** %] The old attachment interface has now been removed via bug 104521, so this entire section can be replaced by [% INCLUDE attachment/list.atml %]. >+ <a href="showdependencytree.cgi?id=$id">Show dependency tree</a> ... >+ <a href="showdependencygraph.cgi?id=$id">Show dependency graph</a> $id -> [% bug.bug_id %] >+ <input type="checkbox" name="bit-[% group.bit %]" value="1" >+ [% " checked" IF group.ingroup %] >+ [% " disabled='disabled'" IF NOT group.ison %]> " checked='checked'"? >+ [% IF bug.bug_status == "UNCONFIRMED" && user.canmodify %] "canmodify" is ambiguous; "caneditorconfirm", although clumsier, is clearer. However, since this is actually the only place where it is needed, you might as well just get rid of it and check both canedit and canconfirm here. >+ [% IF user.canedit %] >+ [% IF bug.isopened %] >+ [% IF bug.status != "ASSIGNED" %] >+ <input type="radio" name="knob" value="accept" /> >+ Accept bug ( >+ [% "confirm bug, " IF user.canmodify && bug.isunconfirmed %]change If the user has canedit privileges (checked in one of the enclosing conditionals), then they certainly have canmodify privileges, so canmodify does not need to be checked here. >+ [% IF bug.isunconfirmed && user.canmodify %] Another unnecessary canmodify. >+ &nbsp;&nbsp;&nbsp;&nbsp;<input type="checkbox" name="andconfirm"> >+ and confirm bug (change status to <b>NEW</b>) >+ <br> >+ [% END %] >+ [% knum = knum + 1 %] >+ >+ <input type="radio" name="knob" value="reassignbycomponent"> >+ Reassign bug to owner >+ [% "and QA contact" IF useqacontact %] >+ of selected component >+ <br> >+ [% IF bug.isunconfirmed && user.canmodify %] Another unnecessary canmodify. >+[%############################################################################%] >+[%# Block for SELECT fields #%] >+[%############################################################################%] >+ >+[% BLOCK select %] >+ >+ [% DEFAULT sel.url = "bug_status.html#$sel.name" %] >+ <td align="right"> >+ <b> >+ [% IF sel.url != "none" %] >+ <a href="[% sel.url %]">[% sel.description %]</a>: >+ [% ELSE %] >+ [% sel.description %]: >+ [% END %] >+ </b> >+ </td> >+ <td> >+ <select name="[% sel.name %]"> >+ [% FOREACH x = ${sel.name} %] >+ <option value="[% x %]" >+ [% " selected" IF x == bug.${sel.name} %]>[% x %]</option> >+ [% END %] >+ </select> >+ </td> >+ <td>&nbsp;</td> >+[% END %] This block is elegant and efficient, but it's also overcomplicated for template code, which non-programmers should be able to understand. Besides focusing the block on the select menu itself (mentioned earlier), the block would be much clearer if it received parameters individually rather than as part of a "sel" hash and if it was passed the list of options directly rather than having to interpolate it from the menu name. >Index: template/default/show/navigate.tmpl >+[% IF bug_list.size %] I used to do this myself, but I recommend doing "bug_list.size > 0" instead for clarity (particularly for non-programmers). >Index: template/default/show/choose_bug.tmpl Our file namespace is a mess of concatenated and underscored words. If I was able to start over again I would standardize the lot of it; in the meantime we should standardize on something for the template files at least. How about hyphens? They make for both readable and typable filenames on all manner of filesystems and are recommended by the Mozilla style guide. http://mozilla.org/README-style.html >+ <input type="submit" value="Show Me This Bug" size="6" /> Size? >Index: show_bug.cgi >+show_bug(); It's a bit strange for this function to be called "show_bug" but defined in "bug_form" which is called from the show_bug.cgi script. Nevertheless, the real solution is to move the show_bug function into Bug.pm as Bug::display and do away with bug_form.pl altogether. Since show_bug is the only function in bug_form.pl, it should be easy to do this, and as I think about it now I would grant review on the code even without Bug::display being integrated with the rest of the code in Bug.pm. As you said before, doing everything originally called for in this bug is a huge load of work, and it makes sense to split it up into multiple pieces. Also, show_bug.cgi should handle the case where the user loads that script without giving it a bug ID, and show_bug/Bug::display should return nothing if the bug ID it is given is not defined, since displaying a bug and displaying a form for entering a bug number to display are entirely separate activities, and separating them allows us to reuse show_bug/Bug::display in situations where we may not necessarily want to return a form if a bug number does not exist. >Index: Attachment.pm > sub list > { >- # Displays a table of attachments for a given bug along with links for >- # viewing, editing, or making requests for each attachment. >- >+ # Creates an array of hashes containing the data for a table of attachments # for a given bug. These data should be given to attachment/list.atml in an >+ # "attachments" variable. > my ($bugid) = @_; This function should be renamed since its purpose is changing. I have been using the name "query" in similar modules (i.e. Request.pm) for the function that retrieves the set of objects matching given criteria (i.e. bug ID, status, active flag, etc.). Since you are rewriting this comment, you might as well clean up my erroneous overuse of technical terminology to describe what is happening here: "Retrieves and returns an array of attachment records for a given bug." should do the trick. The word "records" in that sentence refers to both database records and Perl "records", as complex data structures involving multiple levels of hashes and arrays are often called in Perl. >- # Format the attachment's creation/modification date into something readable. >+ # Format the attachment's creation/modification date into something >+ # readable. This hunk fails because of the fix for bug 93037 and should be removed. >Index: bug_form.pl >+ # Attachments >+ if (Param('useattachmenttracker')) { >+ use Attachment; >+ $bug{'attachments'} = &Attachment::list($id); >+ } else { >+ my @attachments; >+ SendSQL("SELECT attach_id, creation_ts, mimetype, description " . >+ "FROM attachments WHERE bug_id = $id"); >+ while (MoreSQLData()) { >+ my ($attachid, $date, $mimetype, $desc) = FetchSQLData(); >+ if ($date =~ /^(\d\d)(\d\d)(\d\d)(\d\d)(\d\d)(\d\d)(\d\d)$/) { >+ $date = "$3/$4/$2 $5:$6"; >+ } This stuff can all go away now that bug 104521 has landed. >+ # This measns that all product groups will be skipped, but measns -> means >+ $bug{'isopened'} = >+ ($bug{'bug_status'} =~ /^(UNCONFIRMED|NEW|ASSIGNED|REOPENED)$/); Use the "IsOpenedState" function from globals.pl here. >+ my $this_bug_idx = lsearch(\@bug_list, $id); >+ if ($this_bug_idx != -1 && $this_bug_idx + 1 < @bug_list) { >+ $vars->{'next_bug_id'} = $bug_list[$this_bug_idx + 1]; Looks like a bit of left-over code from before you moved this logic into navigate.tmpl. >+ # This is length in number of comments >+ $bug{'longdesclength'} = scalar(@{$bug{'comments'}}); In the original code this value is, believe it or not, the length of the string containing the HTML representation of the description and all the comments on the bug. Presumably this anticipates an editable description/comments. In any case, you are doing it right, just make sure you also hack process_bug.cgi to use this value. >Index: globals.pl >+ SendSQL("SELECT profiles.realname, profiles.login_name, >+ date_format(longdescs.bug_when,'%Y-%m-%d %H:%i'), >+ longdescs.thetext >+ FROM longdescs, profiles >+ WHERE profiles.userid = longdescs.who >+ AND longdescs.bug_id = $id >+ ORDER BY longdescs.bug_when"); The formatting of this query seems a little drunk.

Myk - thanks for the great review :-) > These should be moved to the "bug" directory per our discussion in That discussion's not finished yet :-) I need to comment to that message. > Besides focusing the block on the select menu itself (mentioned earlier), > the block would be much clearer if it received parameters individually > rather than as part of a "sel" hash and if it was passed the list of > options directly rather than having to interpolate it from the menu name. I agree with you 50%. I've changed it not to use a hash, but I believe interpolating from the menu name is the right thing to do - it should become an idiom rather than an abnormality. I've used it in several templates. > Our file namespace is a mess of concatenated and underscored words. > If I was able to start over again I would standardize the lot of it; in > the meantime we should standardize on something for the template files > at least. How about hyphens? They make for both readable and typable > filenames on all manner of filesystems and are recommended by the > Mozilla style guide. They are for HTML file names; source file names are different. I think we should go with underscore-separated words, because that won't invalidate every single filename we've got (as hyphens would) because it's been followed in quite a few cases. >Index: Attachment.pm > > sub list > > { > > This function should be renamed since its purpose is changing. I have > been using the name "query" in similar modules (i.e. Request.pm) for the > function that retrieves the set of objects matching given criteria (i.e. bug > ID, status, active flag, etc.). Do you want me to do this? Other than that, all comments (where I could understand what you wanted) addressed :-) Please have another look. Gerv

>They are for HTML file names; source file names are different. I think we >should go with underscore-separated words, because that won't invalidate every >single filename we've got (as hyphens would) because it's been followed in >quite a few cases. There are no existing template files with underscores, and only six out of 40 CGI scripts have underscores (enter_bug.cgi, long_list.cgi, post_bug.cgi, process_bug.cgi, show_activity.cgi, show_bug.cgi), all of which will be removed during post-templatization script consolidation, so there is no precedent or advantage to sticking with underscores. Nevertheless, the files will have to be renamed anyway after consolidation, and perhaps it makes more sense to name them in the old style pending that work, so go ahead and leave the names as-is. >Index: Attachment.pm > > This function should be renamed since its purpose is changing. >Do you want me to do this? Yes. >Other than that, all comments (where I could understand what you wanted) >addressed :-) Please have another look. Overall looks good. I rescind my review comments telling you to put show_bug() into Bug.pm and the template files into the bug/ sub-directory. Those things need to be done, but they can happen during the script consolidation process after this patch has been checked in. So, the only things left to do in this patch are: 1. Modify process_bug.cgi to use the new code for displaying "the next bug on your list". >+ SendSQL("SELECT profiles.realname, profiles.login_name, >+ date_format(longdescs.bug_when,'%Y-%m-%d %H:%i'), >+ longdescs.thetext >+ FROM longdescs, profiles >+ WHERE profiles.userid = longdescs.who >+ AND longdescs.bug_id = $id >+ ORDER BY longdescs.bug_when"); 2. Re-align this query, which is still inconsistently aligned and hard to read. Left-align the edges of all text following capitalized SQL keywords and then either left-align or right-align the edges of the SQL keywords themselves, i.e.: >+ SendSQL("SELECT profiles.realname, profiles.login_name, >+ date_format(longdescs.bug_when,'%Y-%m-%d %H:%i'), >+ longdescs.thetext >+ FROM longdescs, profiles >+ WHERE profiles.userid = longdescs.who >+ AND longdescs.bug_id = $id >+ ORDER BY longdescs.bug_when"); or: >+ SendSQL("SELECT profiles.realname, profiles.login_name, >+ date_format(longdescs.bug_when,'%Y-%m-%d %H:%i'), >+ longdescs.thetext >+ FROM longdescs, profiles >+ WHERE profiles.userid = longdescs.who >+ AND longdescs.bug_id = $id >+ ORDER BY longdescs.bug_when"); >+our $template = Template->new( 3. Remove the global templatization code, which has already landed and is now part of globals.pl, and use "$::template" and "$::vars" instead of "our $template" and "our $vars" for compatibility with Perl 5.005.

> There are no existing template files with underscores, <snip> So what you are saying is that we have a completely clean slate? I didn't know about the disappearance of the underscored files you mention. If they are going away, I'd be happy with either underscores or hyphens. > >Index: Attachment.pm > > > This function should be renamed since its purpose is changing. > >Do you want me to do this? > > Yes. Right. I'm not certain I know how to do this - where is it called? > 1. Modify process_bug.cgi to use the new code for displaying "the next bug on > your list". Ah. Yes. Gerv

>So what you are saying is that we have a completely clean slate? Not immediately, but in the near future yes. >Right. I'm not certain I know how to do this - where is it called? Yours is the only invocation, so you can just rename it. :-)

Here's version 3. Most things addressed, although I haven't renamed the template files yet, as it's not clear what scheme we are going for. I've had to hack on a fair number of things to make the required changes... Prompt review requested - of all the big templatisation patches, this one is furthest from being cooked, so needs special attention. Gerv

Note to self: save all files before doing cvs diff. Gerv

Comment on attachment 67791 [details] [diff] [review] Patch v.4 r=myk; There are still some issues that could be addressed, but nothing that needs to hold up checkin.

Cool - thanks, Myk. Dave, Myk - who else could we line up to review this? Gerv

Comment on attachment 67791 [details] [diff] [review] Patch v.4 This won't apply to the tip, and if I back-date my repository, apply the patch, then update, I get tons of conflicts.

Just FYI, I *tried* to resolve the conflicts, but the bug_form.pl file was just one big huge conflict. Even telling patch to ignore whitespace. Looks like you pretty much rewrite the whole thing, and too many changes have been made to it since the patch was made.

Here's a refreshed version. Bradley, please stop checking into bug_form.pl ;-) This will need to be refreshed again after long_list goes in (which should happen first.) But it can certainly be reviewed meantime. Gerv

Bleh. Its only conflicting because you indented... Anyway, the attachment.cgi change is bogus, and has been correctly fixed in cvs - your version allows any text through, which defeats the purpose of taint checking.... Rather than using lsearch in the template, can you use contains from the bits myk added? That was just a quick glance - I'll do a proper review tomorrow, if I get time.

OK, what order are we checking in? comment 25 indicates the patch on bug 115369 needs to be checked in before this one, but bug 115369 comment 10 and bug 115369 comment 16 indicate that this patch should be checked in before that one. Can someone make up our minds please?

Bug 115369 first :-) Then, I'll refresh this patch to remove the bits which are in bug 115369. After that, someone can look at it again. Sorry for the confusion :-) Gerv

> Rather than using lsearch in the template, can you use contains from the bits > myk added? They do different things. I use lsearch to get the position in the array - Myk's stuff doesn't give that. Patch v.6, post long_list.cgi checkin, attached. Gerv

There's an awful lot of whitespace between comments with this patch... otherwise it looks pretty good. the existing system already has a little more than there should be I think, but this adds even more. There's 4 blank lines after each comment before the header for the next comment. (there are 3 between each on b.m.o right now, I think 2 would be plenty). Actually, if I copy and paste, the second of these blank lines has two spaces on it. :-)

Dave - comments.tmpl is already checked in, as part of long_list.cgi's templatisation, so if you have problems with its formatting, you need to file a separate bug. ;-) Apart from that, is that a review? Myk, does Patch v.6 still have your review? Gerv

Again, I haven't gone through this fully yet. You ignore Param('usetargetmilestone') - the field always shows. The spacing is different. If you look at landfill bug 2 before and after, after your patch there is extra wrapping (target milestone + tara's name is wrapped, and before it isn't) Actually, this may be because you now put a space between the : and the field, on the page. Not sure if it matters, then - I think its just an edge case. Resolved bugs are missing the option to VERIFY them. Should the navigation stuff always give you the first/last options, even if you're not on a list? Can you make teh action in choose_bug a paramater, so that it can be used for xml.cgi, too? Thats the template part done... The comments extra line thing is now bug 127507, which needs comment - the solution is trivial, but there are two choices.

Comment on attachment 70414 [details] [diff] [review] Patch v.6 needs work from my last comment, but there is still other stuff that could be looked at

> Should the navigation stuff always give you the first/last options, even if > you're not on a list? Sure. > Can you make teh action in choose_bug a paramater, so that it can be used for > xml.cgi, too? xml.cgi requires you to be able to choose multiple bugs. We could use the same template for both, but this would require show_bug to redirect to long_list if multiple bugs were selected. Is that the right idea? Is it OK if I regenerate the patch after you've had time to look a bit closer? Gerv

Good point, wrt xml.cgi. OTOH, eventually weren't you going to merge long_list and show_bug + the xml output, or something? Anyway, with the help of cvs diff -w, here are my other comments: bug_form.pl: Ug, we don't use the votes cache for this? /me sighs, and moves on. Group boxes + associated bits aren't being shown... + $user{'canedit'} = UserInGroup("editbugs") + || $::userid == $bug{'assigned_to'} + || $::userid == $bug{'reporter'} + || $::userid == $bug{'qa_contact'} + || $::userid == 0; + $user{'canconfirm'} = UserInGroup("canconfirm") || ($::userid == 0); Test UserInGroup last, since that involves a db query which you can possibly avoid. I haven't played arround with the various permissions testing stuff, but it seems good. I'm ready for your next set ;)

Oops, missed the other files. Attachment.pm: Why is this needed? Why should the attachments be on the bugs object from the POV of this file? just PROCESS it with attachments = bug.attachments from show_bug. ditto for the corresponding template changes process_bug: Don't use our $vars; that requires perl 5.6. You seem to have lost the taint checks for showing the next page. Oh, and I suppose making bug_form.pl a module of some sort, or part of Bug.pm, or something, is outside the scope of this bug? ;)

> You ignore Param('usetargetmilestone') - the field always shows. Is checking that it has a milestone not sufficient? Oh, well - check added. > Resolved bugs are missing the option to VERIFY them. Urk. > Should the navigation stuff always give you the first/last options, even if > you're not on a list? Probably :-) See the new version, and tell me what you think. > Attachment.pm: Why is this needed? Why should the attachments be on the bugs > object from the POV of this file? just PROCESS it with attachments = > bug.attachments from show_bug. ditto for the corresponding template changes I take your point about including the template correctly, but the changes to Attachment.pm are still needed, surely? We don't want Attachment.pm to output the attachment stuff otherwise it appears at the very top of the file :-) Attachments.pm should return a data structure. > Oh, and I suppose making bug_form.pl a module of some sort, or part of > Bug.pm, or something, is outside the scope of this bug? ;) :-P Ready for re-review. Gerv

> Is checking that it has a milestone not sufficient? Oh, well - check added. Welcome to the bugzilla database schema: target_milestone varchar(20) not null default "---" ;)

Comment on attachment 71181 [details] [diff] [review] Patch v.7 >+ [% IF Param('usestatuswhiteboard') %] ... >+ [% END %] >+ >+ [% IF use_keywords %] ... >+ [% END %] Why are these two different? >+ [% IF use_votes %] ... >+ [% END %] This is differnet because the product may disable voting, right? >+ [% IF groups %] ... >+ <input type="checkbox" name="bit-[% group.bit %]" value="1" >+ [% " 'checked'" IF group.ison %] >+ [% " disabled='disabled'" IF NOT group.ingroup %] /> The default checked stuff isn't working, and it should be checked='checked'. All groups default to off. >+[%# *** Knob *** %] >+ [% knum = 1 %] This is for js, right? >+ [% ELSE %] >+ [% IF bug.resolution != "MOVED" || >+ (bug.resolution == "MOVED" && user.canmove) %] >+ <input type="radio" name="knob" value="reopen" /> Reopen bug >+ <br /> missing an add for knum here >+ [% END %] >+[%############################################################################%] >+[%# Block for SELECT fields #%] >+[%############################################################################%] >+ >+[% BLOCK select %] >+ <td> >+ <select name="[% selname %]"> >+ [% FOREACH x = ${selname} %] >+ <option value="[% x %]" >+ [% " selected" IF x == bug.${selname} %]>[% x %]</option> >+ [% END %] >+ </select> >+ </td> >+ <td>&nbsp;</td> >+[% END %] This is used in so many places. All of them are identical, aren't they? Can you move this into a BLOCK initialised in the constructor in globals.pl? (See http://www.template-toolkit.org/docs/plain/Manual/Config.html#Template_Files_an d_Blocks, the BLOCKS section) A later bug can be filed to pull out the other places; maybe you want to leave this, too. >Index: process_bug.cgi >=================================================================== >RCS file: /cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v >retrieving revision 1.116 >diff -u -r1.116 process_bug.cgi >--- process_bug.cgi 13 Feb 2002 03:05:15 -0000 1.116 >+++ process_bug.cgi 24 Feb 2002 10:42:49 -0000 >-if (defined $::FORM{'id'}) { >- navigation_header(); >- if (defined $::next_bug) { >- # If there is another bug, then we're going to display it, >- # so check that its a legal bug >- # We need to check that its a number first >- if (!(detaint_natural($::next_bug) && CanSeeBug($::next_bug))) { >- # This isn't OK >- # Rather than error out (which could validly happen if there >- # was a bug in the list whose group was changed in the meantime) >- # just remove references to it >- undef $::next_bug; >- } >- } >-} >+# Show next bug, if it exists. >+if ($::COOKIE{"BUGLIST"} && $::FORM{'id'}) { >+ my @bugs = split(/:/, $::COOKIE{"BUGLIST"}); >+ my $cur = lsearch(\@bugs, $::FORM{"id"}); >+ if ($cur >= 0 && $cur < $#bugs) { >+ my $next_bug = $bugs[$cur + 1]; >+ detaint_natural($next_bug); Err, when I said that you'd missed out my taint stuff, I didn't just mean that you should detaint it. Your code allows any bug to be seen + you don't even check the result... You need to only show the bug if CanSeeBug($next_bug) returns true (if it returns false, $next_bug will have no value). Before passing it through canseebug, you need to check that its a number, first, to prevent passing arbitary sql through. Then you need to undef $::next_bug so that the navigation_header routine doesn't show it. You obviously shouldn't have to do the logic exactly as I did it, expecially since I presume that the plan is to rmeove that routine from CGI.pl, and you're reordering in preparation. IF you're doing that, then I wont' coimplain that you're duplicating work which is in that routine.

> >+ [% IF Param('usestatuswhiteboard') %] > >+ [% IF use_keywords %] > > Why are these two different? usestatuswhiteboard is a param. You work out whether you are using keywords or not by finding out whether @::legal_keywords contains any items. It's not a param. > This is differnet because the product may disable voting, right? Yep. > >+ [% " 'checked'" IF group.ison %] > > The default checked stuff isn't working, and it should be checked='checked'. > All groups default to off. The inner set of quotes is the problem. Oops. > >+ [% knum = 1 %] > > This is for js, right? Yep. > This is used in so many places. All of them are identical, aren't they? Can > you move this into a BLOCK initialised in the constructor in globals.pl? Er... this looks a little bit complex. Can we file a bug and optimise it later? I've renamed all the templates to conform to the current thinking on style. Gerv

Comment on attachment 71191 [details] [diff] [review] Patch v.8 I've jsut noticed that in bug_form.pl, you only moced the useringroup call to the end for canconfirm, but canedit still has it at the front. Make the == 0 check the first one, then the checks for assigned to/reporter/qa_contact, and only then make the db query. You've change the template files tom cply with this weeks format, I guess. The checked options should be checked='checked' for xhtml compliance, I think. Anyway, fix those and I think that'll be it.

Review comments addressed. Gerv

Comment on attachment 71245 [details] [diff] [review] Patch v.9 r=bbaetz

Comment on attachment 71245 [details] [diff] [review] Patch v.9 Actually, this fails tests. removing first-review (sorry!)

This one passes tests. r=bbaetz? :-) Gerv

Comment on attachment 72829 [details] [diff] [review] Patch v.10 r=bbaetz (the accesskey change too...)

(X)HTML fixes: lowercase elements and attributes, checked="checked" and friends, added <label>'s. Obsoletes v.10.

Comment on attachment 74610 [details] [diff] [review] Patch v.11 Having chatted to Ville, he's going to do the XHTML stuff as a later patch if it's still thought necessary. Gerv

It seems to me that Patch v.10 doesn't apply cleanly any more: At least Attachment.pm and bug_form.pl seem to have rejects... :( Gerv, can you check and provide a new one if necessary?

Here's an unrotted version. Gerv

Comment on attachment 75743 [details] [diff] [review] Patch v.12 r= carries (it's the same patch.) Can I ask again: please, someone, review this! It's blocking a whole load of other 2.16 work. Gerv

Comment on attachment 75743 [details] [diff] [review] Patch v.12 [this time I'll comment before clicking submit] >+<form method="get" action="show_bug.cgi"> >+ <p> >+ You may find a single bug by entering its bug id here: >+ <input name="id" size="6"/> You're missing a space there, which is probably going to cause problems for older browsers However, I have been infomred that we're not meant to be using xhtml, apparently,. because it won't validate with an html doctype, evne though it will work. So all the <foo /> style tags probably need to be removed, or so I've been told recently. See #mozwebtols for more info

Remove XHTML slashes, for a quiet life. When this argument is resolved, we can add them back in - or not. In the mean time, let's get review! Gerv

Comment on attachment 75752 [details] [diff] [review] Patch v.13 r=bbaetz (carried over; confirmed on IRC.) Gerv

Comment on attachment 75752 [details] [diff] [review] Patch v.13 I installed this patch on my local installation, and it seems to work. I've played around with some changes, and I haven't found any problems. I feel comfortable to give it t(ested)=afranke, which counts as a review last I heard. Feel free to play with it at http://bugzilla.mathweb.org:8000/show_bug.cgi?id=1 .

Gerv, have you tried this with Mozilla 0.9.9? On bugzilla.mozilla.org, there is already some strange overlapping between the field names and the text input fields (look at "QA Contact", "URL", "Summary", "Status Whiteboard", and "Keywords"), but with your patch, the colons at the other fields above are not part of the link any more, and so the same overlap occurs there. I guess that this is not a bug in your patch, but a problem with Mozilla. Do you know if there's an open bug about it? Will it be fixed for 1.0? Or is there a workaround?

> I guess that this is not a bug in your patch, but a problem with Mozilla. Do you > know if there's an open bug about it? Will it be fixed for 1.0? Or is there a > workaround? I'm not seeing the problem with 20020315 on Linux. If it seems to occur widely, we can juggle the template to work around it. Checking in show_bug.cgi; /cvsroot/mozilla/webtools/bugzilla/show_bug.cgi,v <-- show_bug.cgi new revision: 1.18; previous revision: 1.17 done Checking in Attachment.pm; /cvsroot/mozilla/webtools/bugzilla/Attachment.pm,v <-- Attachment.pm new revision: 1.8; previous revision: 1.7 done Checking in bug_form.pl; /cvsroot/mozilla/webtools/bugzilla/bug_form.pl,v <-- bug_form.pl new revision: 1.89; previous revision: 1.88 done Checking in process_bug.cgi; /cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v <-- process_bug.cgi new revision: 1.118; previous revision: 1.117 done RCS file: /cvsroot/mozilla/webtools/bugzilla/template/default/show/choose_bug.html.tmpl,v done Checking in template/default/show/choose_bug.html.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/default/show/choose_bug.html.tmpl,v <-- choose_bug.html.tmpl initial revision: 1.1 done RCS file: /cvsroot/mozilla/webtools/bugzilla/template/default/show/show_bug.html.tmpl,v done Checking in template/default/show/show_bug.html.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/default/show/show_bug.html.tmpl,v <-- show_bug.html.tmpl initial revision: 1.1 done RCS file: /cvsroot/mozilla/webtools/bugzilla/template/default/show/navigate.html.tmpl,v done Checking in template/default/show/navigate.html.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/default/show/navigate.html.tmpl,v <-- navigate.html.tmpl initial revision: 1.1 done Wahey! Now let's get working on those dependencies. Gerv

> I'm not seeing the problem with 20020315 on Linux. Indeed, it seems to be a Mozilla problem that is fixed in current nightlies.