137751 - Linkify bug numbers in dependency fields on show_activity.cgi

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, enhancement, P4)

Description

[Jonas Jørgensen]

When viewing the activity history of a bug, it would be nice to have the bug 
numbers which have been added to or removed from the BugsThisDependsOn or 
OtherBugsDependingOnThis fields linkified.

Comment 1

[Sidra M. S. Vitale]

Attachment: changes to DumpBugActivity to provide show_bug links for dependencies

catches [case-insentively] BugsThisDependsOn/dependson,
OtherBugsDependingOnThis/blocked, in an attempt to be backwards-compatible with
previous BZ's.

Comment 2

[Sidra M. S. Vitale]

Attachment: linkify dependent bugs

tweak to the tmpl to show show_bug.cgi link for 'BugsThisDependsOn' and
'OtherBugsDependingOnThis' fields.

Comment 3

[GavinS]

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

Comment 4

[GavinS]

Attachment: a perl solution to use GetBugLink on the bugids in the activity log

attachment 91895 [details] [diff] [review] is a template solution (which does not use GetBugLink())

This patch is an alternative, perl solution, using GetBugLink()

Comment 5

[Jouni Heikniemi]

Comment on attachment 123821 [details] [diff] [review]
a perl solution to use GetBugLink on the bugids in the activity log

Apart from the following code nits AND the fact that it causes my runtests.pl
to fail in 008filter without an error message, this works and is fine with me.
Requesting review from Dave; would like to hear whether adding this
infrastructure is considered worth it and if cgi.pl is the right place.

>+    my $ret= $bugs;

Nit: space before =.

>+    if (defined $bugs &&
>+        $bugs ne "") {

Nit: No need for this wrap.

>+        $ret = join(", ",
>+                    @buglinks);

And neither this one.

Comment 6

[Dave Miller [:justdave]]

Bugzilla 2.20 feature set is now frozen as of 15 Sept 2004.  Anything flagged
enhancement that hasn't already landed is being pushed out.  If this bug is
otherwise ready to land, we'll handle it on a case-by-case basis, please set the
blocking2.20 flag to '?' if you think it qualifies.

Comment 7

[Frédéric Buclin]

Comment on attachment 91895 [details] [diff] [review]
linkify dependent bugs

Does not work if you change several dependencies at once.

Comment 8

[Frédéric Buclin]

GavinS, is it possible to have it out of CGI.pl?

Comment 9

[Frédéric Buclin]

Comment on attachment 123821 [details] [diff] [review]
a perl solution to use GetBugLink on the bugids in the activity log

Looks like we should stop adding new functions to CGI.pl...

Comment 10

[Colin Ogilvie [:cso]]

Attachment: Patch v1

This patch adds a new column to fielddefs to say if a field is a bugid. This is
then used int the tempate to format the buglink.

Note: if bug 283919 is checked in, this patch will bitrot - and vice-versa.

Comment 11

[Frédéric Buclin]

Comment on attachment 176365 [details] [diff] [review]
Patch v1

1) What happens when several bugs are added or removed from the dependency
list? Are they correctly parsed? I don't think so.

2) I dislike the idea of adding a new attribute for such a minor thing. As
isbugid only depends on the field name, I would prefer a new subroutine
IsBugID() which defines isbugid for you.

If you disagree with my review, ask myk or justdave to overwrite my decision.

Comment 12

[Colin Ogilvie [:cso]]

(In reply to comment #11)
> (From update of attachment 176365 [details] [diff] [review] [edit])
> 1) What happens when several bugs are added or removed from the dependency
> list? Are they correctly parsed? I don't think so.

I'll give you that, I didn't test with more than one. I'd guess no, too.

> 2) I dislike the idea of adding a new attribute for such a minor thing. As
> isbugid only depends on the field name, I would prefer a new subroutine
> IsBugID() which defines isbugid for you.

I disagree here - looking at attachment 173177 [details] [diff] [review] on bug 91037 (Custom Field)
assuming that Myk's way was the way forward with custom fields, I would need to
go and hack the bugzilla function you suggest to say my new field is a bug id
and to link it. With this method, I could just specify it on the custom fields form.

Comment 13

[Colin Ogilvie [:cso]]

OK, justdave seems to think a function is better, so I'll probably just combine
bits of both pathces that aren't obsolete so that the functions aren't in the
global .pl files.

Comment 14

[Colin Ogilvie [:cso]]

Attachment: Patch v2

Patch v2

Removed it from a Database Attribute to be a function call instead.

Comment 15

[Colin Ogilvie [:cso]]

Comment on attachment 176617 [details] [diff] [review]
Patch v2

Theres a problem in this that I only noticed when trying it on landfill. It
fails due to UserInGroup not being a visible function.

Cancelling review, and will set depends on bug 283581

Comment 16

[David D. Kilzer (ddk)]

Attachment: Patch v3

Previous patches had bitrotten since CGI.pl was removed, so I marked them all obsolete.

Added a new 'bug_list_link' filter to Bugzilla/Template.pm and added special cases for 'blocked' and 'dependson' in template/en/default/bug/activity/table.html.tmpl to call the new filter.

Comment 17

[David D. Kilzer (ddk)]

Comment on attachment 215494 [details] [diff] [review]
Patch v3

Nit (copy-paste):

>   # Contributor(s): Gervase Markham <[e-mail]>
>+  # Contributor(s): David D. Kilzer <[e-mail]>

Comment 18

[David D. Kilzer (ddk)]

Attachment: Patch v4

Same as Patch v3 except fixed copy-paste nit from Comment #17.

Comment 19

[Teemu Mannermaa (:wicked)]

Comment on attachment 215497 [details] [diff] [review]
Patch v4

Eep, makes test suite fail:

not ok 220 - (en/default) template/en/default/bug/activity/table.html.tmpl has unfiltered directives:
#   91: change.removed FILTER bug_list_link
#   111: change.added FILTER bug_list_link
# --ERROR
#     Failed test (t/008filter.t at line 131)

Comment 20

[timeless]

Attachment: v5

Comment 21

[Teemu Mannermaa (:wicked)]

Comment on attachment 217543 [details] [diff] [review]
v5

>Index: template/en/default/bug/activity/table.html.tmpl
>===================================================================
>+                  [% change.removed FILTER bug_list_link FILTER none%]

Nit: Add missing space between none and % before commit.

Comment 22

[Dave Miller [:justdave]]

Make sure to credit both ddkilzer and timeless for the patch in the checkin comment.

Comment 23

[Frédéric Buclin]

Checking in Bugzilla/Template.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Template.pm,v  <--  Template.pm
new revision: 1.46; previous revision: 1.45
done
Checking in template/en/default/bug/activity/table.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/activity/table.html.tmpl,v  <--  table.html.tmpl
new revision: 1.12; previous revision: 1.11
done

Comment 24

[Frédéric Buclin]

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

Comment 25

[Max Kanat-Alexander]

Added to the release notes as part of bug 349423.