Links to obsoleted attachment should use line-through style. In bug 153547 comment 3, I would like to have the link use a line-through style as is done for links to resolved bugs.

I'm working on this for Zippy.

Tested to work.

Comment on attachment 128089 [details] [diff] [review] Patch >diff -p -u -d -r1.242 globals.pl >@@ -905,10 +905,12 @@ sub quoteUrls { >- ~<a href=\"attachment.cgi?id=$2&amp;action=view\">$1</a>~igx; >+ ~GetAttachmentLink($2, $1) >+ ~egmx; Is this a safe swap? >+sub GetAttachmentLink { >+ my ($attachid, $link_text) = @_; >+ detaint_natural($attachid) || die "GetAttachmentLink() called with non-integer attachment number"; It would be nicer if this line wrapped at 72-76 chars >+ if (MoreSQLData()) { >+ my ($bugid, $isobsolete, $desc) = FetchSQLData(); >+ my $title = ""; >+ my $className = ""; >+ if (CanSeeBug($bugid, $::userid)) { >+ $title = $desc; >+ } >+ if ($isobsolete) { >+ $className = "bz_obsolete"; >+ } >+ $::attachlink{$attachid} = [value_quote($title), $className]; It only makes sense to set the tuple if the user CanSeeBug, so you might want to move it in: if (CanSeeBug()) { # ... if ($isobsolete) { # ... } # ... } else { $::attachlink{$attachid} = []; } It's a nit, though (I realize this follows GetBugLink somewhat). >+ # Now that we know we've got all the information we're gonna get, let's >+ # return the link (which is the whole reason we were called :) >+ my ($title, $className) = @{$::attachlink{$attachid}}; >+ # $title will be undefined if the bug didn't exist in the database. >+ if (defined $title) { >+ my $linkval = "attachment.cgi?id=$attachid&amp;action=view"; >+ return qq{<a href="$linkval" class="$className" title="$title">$link_text</a>}; >+ } In that case, you would do the check first (if $::attachlink{$attachid} was non-empty) and then unpack the tuple inside the if clause. At any rate, these are nits. The code looks nice (I do wonder if a cache for attachment lookups is really that important, but it's done for bug #s... I suppose for really long bugs it might save tens of SQL lookups). r=kiko

>Is this a safe swap? Yes. >It would be nicer if this line wrapped at 72-76 chars Done. >It only makes sense to set the tuple if the user CanSeeBug The only thing that matters is that they don't get the attachment description. In fact, since we allow seeing whether bugs are resolved or not even if the user can't see them, I'm more inclined to allow them to see whether an attachment is obsoleted. >In that case, you would do the check first I suppose, although this follows more closely with the style of related things, specifically GetBugLink, so I am inclined to leave things the way they are. Seeking approval for the attached patch if I fix the long line to wrap.

mozilla/webtools/bugzilla> cvs ci globals.pl css/edit_bug.css template/en/default/attachment/list.html.tmpl Checking in globals.pl; /cvsroot/mozilla/webtools/bugzilla/globals.pl,v <-- globals.pl new revision: 1.243; previous revision: 1.242 done Checking in css/edit_bug.css; /cvsroot/mozilla/webtools/bugzilla/css/edit_bug.css,v <-- edit_bug.css new revision: 1.4; previous revision: 1.3 done Checking in template/en/default/attachment/list.html.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/en/default/attachment/list.html.tmpl,v <-- list.html.tmpl new revision: 1.11; previous revision: 1.10 done