A template I did on the plane... This is customer-facing, so a 2.16 blocker. (This is probably another candidate for Bug.pm, but we can do that later.) Gerv

Here it is. It includes GetComments(), a utility function I'm also going to use in show_bug.cgi's templatisation, and also a quick tweak to linkify "new attachment" comments - not sure where that came from. Gerv

When reviewing this patch, I noticed that the comments section when viewing a single bug using show_bug.cgi breaks and displays HASH(0x83b06a4) instead of the proper comments. The comments do display properly in long_list.cgi though. This is probably due to changes made in the quoteUrls function. Does this happen for anyone else?

Yes, it's because I removed the first, useless, parameter to quoteUrls. I've also done this in my show_bug.cgi templatisation, the other CGI which uses that function. Ignore it for now, and we'll try and sync the checking-in of the two to make this problem not show up in CVS. Any other issues, or is that an r=? ;-) Gerv

Comment on attachment 61807 [details] [diff] [review] Patch v.1 The only issue I encountered was the change in quoteUrls causing breakage in the stock show_bug.cgi but since this is known and will be fixed later, it looks good for long_list.cgi. r=dkl

Comment on attachment 61807 [details] [diff] [review] Patch v.1 >+ [% IF bug.qa_contact %] >+ [% PROCESS cell attr = { description => "QA Contact", name => "qa_contact" } %] >+ [% END %] Some bmo bugs have an empty qa_contact, and I suspect that this would miss those - you should use the param instead. OTOH, not displaying the heading could be valid, I guess, although possibly confusing. The old code used the param. >+ [% IF bug.target_milestone %] >+ <b>Target Milestone:</b>&nbsp; >+ [% bug.target_milestone %] >+ [% END %] I'm not sure if the same argument applies here; I think it doesn't. >+ [% IF bug.keywords %] >+ <tr> >+ <td colspan="4"> >+ <b>Keywords: </b>&nbsp;[% bug.keywords %] >+ </td> >+ </tr> >+ [% END %] ditto

Comments addressed. Looking for re-review from bbaetz or anyone else. Gerv

Comment on attachment 66345 [details] [diff] [review] Patch v.2 Looks good. Works as expected. r=dkl

err. Which of my comments did you addresS? You didn't change anything I mentioned, or comment on it.

Bradley - I've changed qa contact to use the param, and target_milestone, and set up a use_keywords boolean. These are the three things you commented on. Gerv

Comment on attachment 66345 [details] [diff] [review] Patch v.2 grr. /me wants a "cc yourself" button on the attachment comment screen. Anyway, I missed that, sorry. This fails to apply, because you're trying to remove a line referring to showattachment.cgi, but myk has changed that. Manually editing the patch got it to apply, and it worked, so r=bbaetz. It does need that fixed though - if thats the only change, then carry my r= though. Do not check this in before checking in the show_bug stuff.

Sorted. Gerv

Comment on attachment 66345 [details] [diff] [review] Patch v.2 removing second-review from an obsoleted and needs-worked patch so it'll stop matching my queries looking for things to check in. :-)

Comment on attachment 67582 [details] [diff] [review] Patch v.3 This has r=bbaetz. dkl - can you give it a once-over again and make sure you are still happy with it? Gerv

Comment on attachment 67582 [details] [diff] [review] Patch v.3 Patch v3 applies cleanly and all works as expected. r=dkl

Checking in globals.pl; /cvsroot/mozilla/webtools/bugzilla/globals.pl,v <-- globals.pl new revision: 1.140; previous revision: 1.139 done Checking in long_list.cgi; /cvsroot/mozilla/webtools/bugzilla/long_list.cgi,v <-- long_list.cgi new revision: 1.22; previous revision: 1.21 done Checking in template/default/show/comments.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/default/show/comments.tmpl,v <-- comments.tmpl initial revision: 1.1 done Checking in template/default/show/multiple.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/default/show/multiple.tmpl,v <-- multiple.tmpl initial revision: 1.1 done

Er, Dave... note bbaetz' comment: "Do not check this in before checking in the show_bug stuff." You'll have broken comment display in show_bug, I think, because of the change in the parameters to the function call quoteUrls(). Won't you? Gerv

Note that this problem is fairly easy to fix - just change the other call to quoteUrls. You may want to do that instead of backing the patch out. I can do that in a few hours (when I finish work) if necessary. Gerv

Why isn't there a dependency set? :-) That's what those are for. Also, this created tree bustage. Name "legal_keywords" used only once in long_list.cgi Checking in long_list.cgi; /cvsroot/mozilla/webtools/bugzilla/long_list.cgi,v <-- long_list.cgi new revision: 1.23; previous revision: 1.22 done

I backed it out. It's broken on Perl 5.00503. The globals.pl patch makes use of "our", which doesn't work in 5.00503. Are we upping the Perl requirement, or is there another way to do this?

Comment on attachment 67582 [details] [diff] [review] Patch v.3 >Index: globals.pl >@@ -1548,5 +1565,181 @@ >+# Global Templatization Code >+our $template = Template->new( >+our $vars = As stated above, "our" doesn't work in Perl 5.00503 according to tinderbox.

Setting dependency on bug 110012 so we don't accidently check in out of order again.

v.4 is a clean patch without the template files, which are already checked in. I've also started using the global template declarations, used "vars" instead of "sillyness", and picked a sensible filename for Content-Disposition. In addition, it quickly patches show_bug.cgi to use the new quoteUrls calling convention, so this patch has no dependencies and, subject to quick re-review, can be checked in as-is. Gerv

Comment on attachment 69278 [details] [diff] [review] Patch v.4 I don't see show_bug.cgi in this patch... and as long as you need a new attachment :) >Index: long_list.cgi [snip] > my $generic_query = " >-select >- bugs.bug_id, >- bugs.product, [snip] >+ SELECT bugs.bug_id, bugs.product, bugs.version, bugs.rep_platform, >+ bugs.op_sys, bugs.bug_status, bugs.resolution, bugs.priority, Why did you collapse this list? Granted it make is shorter, but it also makes it more difficult to add new things (you have to rearrange 5 lines to add something near the begining or have a > 80 character line). >+# Work out a sensible filename for Content-Disposition. >+# Sadly, I don't think we can tell if this was a named query. If you were really desperate you could check the HTTP_REFERER for a named query, but it probably isn't worth it.

> I don't see show_bug.cgi in this patch... and as long as you need a new > attachment :) The relevant call to quoteUrls is actually in GetCommentsAsHTML in globals.pl. > Why did you collapse this list? Granted it make is shorter, but it also > makes it more difficult to add new things (you have to rearrange 5 lines to > add something near the begining or have a > 80 character line). Well, don't add things near the beginning, then :-) MySQL doesn't care what order you request them in. > If you were really desperate you could check the HTTP_REFERER for a named > query, but it probably isn't worth it. You're right :-) Gerv

(This comment was supposed to go in last night, but it midaired.) I think there are several lessons here: 1) Use dependencies properly 2) If you are reviewing something, sync your tree before you test it 3) Let people check in their own patches whenever possible Comments? Gerv

Comment on attachment 69278 [details] [diff] [review] Patch v.4 I've addressed Jake's points - no new patch is necessary. This patch is ready for review. Gerv

Comment on attachment 69278 [details] [diff] [review] Patch v.4 > my $generic_query = " >-select >- bugs.bug_id, >- bugs.product, >- bugs.version, >- bugs.rep_platform, >- bugs.op_sys, >- bugs.bug_status, >- bugs.bug_severity, >- bugs.priority, >- bugs.resolution, >- assign.login_name, >- report.login_name, >- bugs.component, >- bugs.bug_file_loc, >- bugs.short_desc, >- bugs.target_milestone, >- bugs.qa_contact, >- bugs.status_whiteboard, >- bugs.keywords >-from bugs,profiles assign,profiles report >-where assign.userid = bugs.assigned_to and report.userid = bugs.reporter and"; Jake is right. Please don't collapse this, it makes it _much_ harder to read and verify. No need to reattach, just change and check in. >+ my %bug; >+ my @row = FetchSQLData(); >+ >+ foreach my $field ("bug_id", "product", "version", "rep_platform", >+ "op_sys", "bug_status", "resolution", "priority", >+ "bug_severity", "component", "assigned_to", "reporter", >+ "bug_file_loc", "short_desc", "target_milestone", >+ "qa_contact", "status_whiteboard", "keywords") We could probably avoid having to repeat everything here by simply using the field name for the variable name. The two exceptions are assigned_to and reporter, which would have to be handled separately (just =~ it, i guess). Do you agree? If you do, please file a bug or remind me to do it. Other than that it looks fine. Uncollapse and r=kiko

Checking in long_list.cgi; /cvsroot/mozilla/webtools/bugzilla/long_list.cgi,v <-- long_list.cgi new revision: 1.25; previous revision: 1.24 done Checking in globals.pl; /cvsroot/mozilla/webtools/bugzilla/globals.pl,v <-- globals.pl new revision: 1.144; previous revision: 1.143 done Gerv