offer View All (non obsolete) attachments

RESOLVED FIXED in Bugzilla 4.2

Status

()

--
enhancement
RESOLVED FIXED
11 years ago
8 years ago

People

(Reporter: timeless, Assigned: guy.pyrzak)

Tracking

3.1.2
Bugzilla 4.2
Bug Flags:
approval +

Details

(URL)

Attachments

(1 attachment, 2 obsolete attachments)

v3
3.03 KB, patch
LpSolit
: review+
Details | Diff | Splinter Review
(Reporter)

Description

11 years ago
bug 413961 has too many attachments, i want to:
1. click "hide obsolete"
2. click "view all"
*see* only the non obsolete attachments.

note: i don't want to break current urls, i just want a new feature &action=viewnonobsolete

Comment 1

11 years ago
Yeah, that sounds like the right implementation and a sensible thing to do.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Mac OS X → All
Hardware: Macintosh → All

Comment 2

11 years ago
Oh, except it should be &hide_obsolete=1, not a new whole action.
(Assignee)

Comment 3

11 years ago
Yoink! (the sound of taking a bug)
Assignee: attach-and-request → guy.pyrzak
(Assignee)

Comment 4

9 years ago
Created attachment 430979 [details] [diff] [review]
V1

I'm attaching this for now but I think I need to add something to the page to indicate that obsolete attachments have been hidden.

Updated

9 years ago
Attachment #430979 - Attachment is patch: true
Attachment #430979 - Attachment mime type: application/octet-stream → text/plain
(Assignee)

Comment 5

9 years ago
Created attachment 430982 [details] [diff] [review]
V2
Attachment #430979 - Attachment is obsolete: true
Attachment #430982 - Flags: review?(LpSolit)

Comment 6

9 years ago
Comment on attachment 430982 [details] [diff] [review]
V2

>=== modified file 'skins/standard/attachment.css'

>+#hidden_obsolete_message{

Missing whitespace before {.


>=== modified file 'template/en/default/attachment/list.html.tmpl'

>+                  [% bugid %]&action=viewall
>+                  [%- "&hide_obsolete=1" IF obsolete_attachments %]">View All</a>

& -> &amp; (must be escaped).



>=== modified file 'template/en/default/attachment/show-multiple.html.tmpl'

>+  Obsolete attachments are hidden. To view all attachments (including obsolete) <a href="attachment.cgi?bugid=[% bug.id %]&action=viewall">click here</a>.

[% bug.id %] must be HTML filtered:

#   Failed test '(en/default) template/en/default/attachment/show-multiple.html.tmpl has unfiltered directives:
#   38: bug.id
# --ERROR'
#   at t/008filter.t line 132.


Otherwise looks good and works great!
Attachment #430982 - Flags: review?(LpSolit) → review-

Updated

9 years ago
Target Milestone: --- → Bugzilla 3.8

Updated

8 years ago
Target Milestone: Bugzilla 4.0 → ---

Comment 7

8 years ago
Thinking about it again, I think that "View All" should only display non-obsolete attachments, always.

Comment 8

8 years ago
(In reply to comment #7)
> Thinking about it again, I think that "View All" should only display
> non-obsolete attachments, always.

  Or perhaps just whatever is currently being displayed in the table, no?

Comment 9

8 years ago
(In reply to comment #8)
>   Or perhaps just whatever is currently being displayed in the table, no?

Sure, this would work too.
(Assignee)

Comment 10

8 years ago
Created attachment 474179 [details] [diff] [review]
v3
Attachment #430982 - Attachment is obsolete: true
Attachment #474179 - Flags: review?(LpSolit)

Comment 11

8 years ago
Comment on attachment 474179 [details] [diff] [review]
v3

>=== modified file 'attachment.cgi'

>+    if ( $cgi->param('hide_obsolete') ) {

Nit: no need for the extra whitespaces in parens.


>+        $attachments = [ grep { ! $_->isobsolete } @$attachments ];

Please write: @$attachments = grep { !$_->isobsolete } @$attachments;



=== modified file 'template/en/default/attachment/list.html.tmpl'

>+            <a id="view_all" href="attachment.cgi?bugid=
>+                  [% bugid %]&amp;action=viewall

Nit: please write [%- bugid %] instead, to make sure no newline is inserted in the URL (those are a pain for Selenium).



>=== modified file 'template/en/default/attachment/show-multiple.html.tmpl'

>+  <div id="hidden_obsolete_message">
>+  Obsolete attachments are hidden. To view all attachments (including obsolete) 
>+  <a href="attachment.cgi?bugid=[% bug.id FILTER html%]&amp;action=viewall">click here</a>.
>+  </div>

Please fix the indentation, and add a whitespace before %].


I like this patch. :) r=LpSolit with my comments fixed.
Attachment #474179 - Flags: review?(LpSolit) → review+

Updated

8 years ago
Flags: approval+
Target Milestone: --- → Bugzilla 4.2
(Assignee)

Comment 12

8 years ago
Committing to: bzr+ssh://guy.pyrzak%40gmail.com@bzr.mozilla.org/bugzilla/trunk/                                   
modified attachment.cgi
modified skins/standard/attachment.css
modified template/en/default/attachment/list.html.tmpl
modified template/en/default/attachment/show-multiple.html.tmpl
Committed revision 7511.
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.