Closed Bug 153583 Opened 19 years ago Closed 18 years ago

Links to obsoleted attachment should use line-through style


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




Bugzilla 2.18


(Reporter: mats, Assigned: caillon)





(1 file)

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.
Severity: minor → enhancement
OS: Windows 98 → All
Hardware: PC → All
Priority: -- → P3
Target Milestone: --- → Bugzilla 2.18
I'm working on this for Zippy.
Assignee: myk → caillon
Attached patch PatchSplinter Review
Tested to work.
Comment on attachment 128089 [details] [diff] [review]

>diff -p -u -d -r1.242
>@@ -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?


>It would be nicer if this line wrapped at 72-76 chars


>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

>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.
Flags: approval?
Flags: approval? → approval+
mozilla/webtools/bugzilla> cvs ci css/edit_bug.css
Checking in;
/cvsroot/mozilla/webtools/bugzilla/,v  <--
new revision: 1.243; previous revision: 1.242
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
Checking in template/en/default/attachment/list.html.tmpl;
 <--  list.html.tmpl
new revision: 1.11; previous revision: 1.10
Closed: 18 years ago
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.