414509 - offer View All (non obsolete) attachments

Status: RESOLVED FIXED

(Bugzilla :: Attachments & Requests, enhancement)

Description

[timeless]

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

[Max Kanat-Alexander]

Yeah, that sounds like the right implementation and a sensible thing to do.

Comment 2

[Max Kanat-Alexander]

Oh, except it should be &hide_obsolete=1, not a new whole action.

Comment 3

[Guy Pyrzak]

Yoink! (the sound of taking a bug)

Comment 4

[Guy Pyrzak]

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

Comment 5

[Guy Pyrzak]

Attachment: V2

Comment 6

[Frédéric Buclin]

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!

Comment 7

[Frédéric Buclin]

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

Comment 8

[Max Kanat-Alexander]

(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

[Frédéric Buclin]

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

Sure, this would work too.

Comment 10

[Guy Pyrzak]

Attachment: v3

Comment 11

[Frédéric Buclin]

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.

Comment 12

[Guy Pyrzak]

Committing to: bzr+ssh://[e-mail]/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.