Templatise long_list.cgi

RESOLVED FIXED in Bugzilla 2.16

Status

()

Bugzilla
Query/Bug List
P1
blocker
RESOLVED FIXED
16 years ago
5 years ago

People

(Reporter: gerv, Assigned: gerv)

Tracking

2.15
Bugzilla 2.16

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

16 years ago
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
(Assignee)

Updated

16 years ago
Severity: normal → blocker
Status: NEW → ASSIGNED
Priority: -- → P1
(Assignee)

Comment 1

16 years ago
Created attachment 61807 [details] [diff] [review]
Patch v.1

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
(Assignee)

Updated

16 years ago
Target Milestone: --- → Bugzilla 2.16

Comment 2

16 years ago
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?
(Assignee)

Comment 3

16 years ago
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 4

16 years ago
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
Attachment #61807 - Flags: review+

Updated

16 years ago
Keywords: patch
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
(Assignee)

Comment 6

16 years ago
Created attachment 66345 [details] [diff] [review]
Patch v.2

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

Gerv

Comment 7

16 years ago
Comment on attachment 66345 [details] [diff] [review]
Patch v.2

Looks good. Works as expected.
r=dkl
Attachment #66345 - Flags: review+
err. Which of my comments did you addresS? You didn't change anything I
mentioned, or comment on it.
(Assignee)

Updated

16 years ago
Keywords: review
(Assignee)

Comment 9

16 years ago
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

Updated

16 years ago
Attachment #66345 - Flags: review-
Attachment #66345 - Flags: review+
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.
(Assignee)

Comment 11

16 years ago
Created attachment 67582 [details] [diff] [review]
Patch v.3

Sorted.

Gerv
Attachment #61807 - Attachment is obsolete: true
Attachment #66345 - Attachment is obsolete: true
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. :-)
Attachment #66345 - Flags: review+
(Assignee)

Updated

16 years ago
Blocks: 120660
(Assignee)

Comment 13

16 years ago
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
Attachment #67582 - Flags: review+
Comment on attachment 67582 [details] [diff] [review]
Patch v.3

Patch v3 applies cleanly and all works as expected.
r=dkl
Attachment #67582 - Flags: review+
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
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
(Assignee)

Comment 16

16 years ago
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

(Assignee)

Comment 17

16 years ago
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?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Attachment #67582 - Flags: review+ → review-
Setting dependency on bug 110012 so we don't accidently check in out of order again.
Depends on: 110012
(Assignee)

Comment 22

16 years ago
Created attachment 69278 [details] [diff] [review]
Patch v.4

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
Attachment #67582 - Attachment is obsolete: true

Comment 23

16 years ago
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.
Attachment #69278 - Flags: review-
(Assignee)

Comment 24

16 years ago
> 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

(Assignee)

Comment 25

16 years ago
(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
(Assignee)

Updated

16 years ago
Attachment #69278 - Flags: review-
(Assignee)

Comment 26

16 years ago
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 27

16 years ago
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
Attachment #69278 - Flags: review+
(Assignee)

Comment 28

16 years ago
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
Status: REOPENED → RESOLVED
Last Resolved: 16 years ago16 years ago
No longer depends on: 110012
Resolution: --- → FIXED
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.