Links to obsoleted attachment should use line-through style

RESOLVED FIXED in Bugzilla 2.18

Status

()

enhancement
P3
normal
RESOLVED FIXED
17 years ago
7 years ago

People

(Reporter: mats, Assigned: caillon)

Tracking

2.17
Bugzilla 2.18
Dependency tree / graph
Bug Flags:
approval +

Details

()

Attachments

(1 attachment)

Reporter

Description

17 years ago
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.

Updated

17 years ago
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
Posted patch PatchSplinter Review
Tested to work.

Comment 3

16 years ago
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.
Flags: approval?
Flags: approval? → approval+
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
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED

Updated

7 years ago
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.