124937 - Templatise show_activity.cgi

Status: RESOLVED FIXED

(Bugzilla :: User Interface, defect)

Description

[Gervase Markham [:gerv]]

Here's another one - this is reasonably simple. We're knocking them off... 

Gerv

Comment 1

[Gervase Markham [:gerv]]

Attachment: Patch v0.5

This isn't quite ready yet - it changes DumpBugActivity significantly
(templatisation and cleanup) but doesn't change process_bug.cgi to use the new
version, so that will break. Still, a quick glance over it to tell me if I've
made egregious errors would be appreciated.

Gerv

Comment 2

[Dave Miller [:justdave]]

does this take into account bug 94303?

Comment 3

[Gervase Markham [:gerv]]

Ook. No it doesn't. But it could. I'll take it into account while producing
version 1.0.

Gerv

Comment 4

[Gervase Markham [:gerv]]

Attachment: Patch v0.9

Now fixes bug 94303, but doesn't change process_bug, as it's being templatised
over in bug 125660 , and there are other templatisation patches which touch
that exact part of process_bug.

Gerv

Comment 5

[Bradley Baetz (:bbaetz)]

Why does GetBugActivity poke $::vars directly, rather than just return 2 vars?

Comment 6

[Gervase Markham [:gerv]]

How do you return two vars? 

return $foo, $bar;

?

Gerv

Comment 7

[Bradley Baetz (:bbaetz)]

Return a two element array or list.

Comment 8

[Gervase Markham [:gerv]]

Actually, but 110012 is the real dependency, because it touches process_bug.cgi.

Gerv

Comment 9

[Ville Skyttä]

+            [% IF change.attachid %]
+              <a href="attachment.cgi?id=[% change.attachid %]&action=view">
+              Attachment #[% change.attachid %]</a>         

...should read &amp;action=view.

Comment 10

[Gervase Markham [:gerv]]

Attachment: Patch v.1

This should be it, just about. I've changed comments.tmpl to use "comments"
instead of "bug.comments", which seems more correct.

Gerv

Comment 11

[Bradley Baetz (:bbaetz)]

Comment on attachment 76307 [details] [diff] [review]
Patch v.1

>Index: template/default/show/activity.html.tmpl
>===================================================================
>RCS file: template/default/show/activity.html.tmpl
>diff -N template/default/show/activity.html.tmpl
>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ template/default/show/activity.html.tmpl	26 Mar 2002 23:57:23 -0000
>@@ -0,0 +1,91 @@
>+  #     attach_id: integer. If the change was adding an attachment, its id.
>+  # incomplete_data: boolean. True if some of the data is incomplete (because
>+  #                  it was affected by an old Bugzilla bug.)
>+  #%]
>+ 

Can you mention the bug # here, in the comment? Of course, 'starts with ?' is a
really bad heruistic,
but you're not changing that.

>+        [% FOREACH change = operation.changes %]
>+          [% "<tr>" IF loop.index > 0 %]
>+            <td>
>+              [% IF change.attachid %]
>+                <a href="attachment.cgi?id=[% change.attachid %]&action=view">
>+                Attachment #[% change.attachid %]</a>         
>+              [% END %]
>+              [% change.field %]

This should be FILTER html, too, except that then you'd lose the attachment
links.

We don't ahve any fields with objectionable names, so I guess you can leave
this for the moment.

r=bbaetz

Comment 12

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

Comment on attachment 76307 [details] [diff] [review]
Patch v.1

>Index: CGI.pl

>+    my $operation = {};
>+    my $changes = ();

"$changes = ()" --> "$changes = []" since you are initializing an array
reference
(just like you use "{}" when initializing a hash reference).


>+        # Check for the results of an old Bugzilla data corruption bug
>+        $incomplete_data = 1 if ($added =~ /^\?/ || $removed =~ /^\?/);

Nit: unnecessary parentheses.


>+            $operation = {};
>+            $changes = ();

"$changes = ()" --> "$changes = []" since you are initializing an array
reference.


Fix these problems and I'll r= it.

Comment 13

[Gervase Markham [:gerv]]

Attachment: Patch v.2

> >+	    # Check for the results of an old Bugzilla data corruption bug
> >+	    $incomplete_data = 1 if ($added =~ /^\?/ || $removed =~ /^\?/);
> 
> Nit: unnecessary parentheses.

Because it's a double-barrelled clause, I think keeping them aids readability.

> Can you mention the bug # here, in the comment?

If I knew what it was :-| I don't think any of the old comments said.

Gerv

Comment 14

[Gervase Markham [:gerv]]

Attachment: Patch v.3

v.2 is the same patch as v.1. Oops. :-)

Gerv

Comment 15

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

Comment on attachment 77223 [details] [diff] [review]
Patch v.3

Works, looks good.  r=myk

Comment 16

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

>Because it's a double-barrelled clause, I think keeping them aids readability.

My mistake, you are right.  Nit rescinded. :-)

Comment 17

[Bradley Baetz (:bbaetz)]

Comment on attachment 77223 [details] [diff] [review]
Patch v.3

WFM, too. the corruption bug was just an extra biut on the orignal checkin, so
don't bother with it.

r=bbaetz

Comment 18

[Bradley Baetz (:bbaetz)]

Oh, and you need to change multiple.tmpl, too:

  [% PROCESS show/comments.tmpl comments=bug.comments %]

else comments don't show up on the long list stuff.

Comment 19

[Gervase Markham [:gerv]]

Fixed.

Checking in show_activity.cgi;
/cvsroot/mozilla/webtools/bugzilla/show_activity.cgi,v  <--  show_activity.cgi
new revision: 1.8; previous revision: 1.7
done
Checking in CGI.pl;
/cvsroot/mozilla/webtools/bugzilla/CGI.pl,v  <--  CGI.pl
new revision: 1.140; previous revision: 1.139
done
Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.120; previous revision: 1.119
done
RCS file:
/cvsroot/mozilla/webtools/bugzilla/template/default/show/activity.html.tmpl,v
done
Checking in template/default/show/activity.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/default/show/activity.html.tmpl,v 
<--  activity.html.tmpl
initial revision: 1.1
done
RCS file:
/cvsroot/mozilla/webtools/bugzilla/template/default/show/bug-activity.html.tmpl,v
done
Checking in template/default/show/bug-activity.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/default/show/bug-activity.html.tmpl,v
 <--  bug-activity.html.tmpl
initial revision: 1.1
done
Checking in template/default/show/comments.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/default/show/comments.tmpl,v  <-- 
comments.tmpl
new revision: 1.3; previous revision: 1.2
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
new revision: 1.8; previous revision: 1.7
done
Checking in template/default/show/multiple.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/default/show/multiple.tmpl,v  <-- 
multiple.tmpl
new revision: 1.2; previous revision: 1.1
done

Gerv