153583 - Links to obsoleted attachment should use line-through style

Status: RESOLVED FIXED

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

Description

[Mats Palmgren (inactive)]

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.

Comment 1

[Christopher Aillon (sabbatical, not receiving bugmail)]

I'm working on this for Zippy.

Comment 2

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Patch

Tested to work.

Comment 3

[Christian Reis]

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

Comment 4

[Christopher Aillon (sabbatical, not receiving bugmail)]

>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.

Comment 5

[Christopher Aillon (sabbatical, not receiving bugmail)]

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