232659 - Fix inconsistent attachment links (and clean up diff viewer UI while we're at it)

Status: VERIFIED FIXED

(Bugzilla :: User Interface, defect)

Description

[[e-mail]]

On a Differences screen:
[1] View: http://bugzilla.mozilla.org/attachment.cgi?id=<id>
[2] Edit: http://bugzilla.mozilla.org/attachment.cgi?action=edit&id=<id>
On an Edit Attachment screen:
[3] View: http://bugzilla.mozilla.org/attachment.cgi?id=<id>
[4] iframe src: http://bugzilla.mozilla.org/attachment.cgi?id=<id>&action=view
On a Bug screen:
[5] View: http://bugzilla.mozilla.org/attachment.cgi?id=<id>&action=view
[6] Edit: http://bugzilla.mozilla.org/attachment.cgi?id=<id>&action=edit
Assuming the old links 4-6 are correct, the new links 1-3 are wrong.
Having these wrong links (especially [2]) mucks up your history.

Comment 1

[Myk Melez [:myk] [@mykmelez]]

> On a Differences screen:
> [1] View: http://bugzilla.mozilla.org/attachment.cgi?id=<id>

correct

> [2] Edit: http://bugzilla.mozilla.org/attachment.cgi?action=edit&id=<id>

wrong; should be:

http://bugzilla.mozilla.org/attachment.cgi?id=<id>&action=edit

> On an Edit Attachment screen:
> [3] View: http://bugzilla.mozilla.org/attachment.cgi?id=<id>

correct

> [4] iframe src: http://bugzilla.mozilla.org/attachment.cgi?id=<id>&action=view

wrong; should be:

http://bugzilla.mozilla.org/attachment.cgi?id=<id>

> On a Bug screen:
> [5] View: http://bugzilla.mozilla.org/attachment.cgi?id=<id>&action=view

wrong; should be:

http://bugzilla.mozilla.org/attachment.cgi?id=<id>

> [6] Edit: http://bugzilla.mozilla.org/attachment.cgi?id=<id>&action=edit

correct

Comment 2

[GavinS]

Issue [5] is fixed by bug#252839

Comment 3

[Christian Reis]

I'll get to these this afternoon.

Comment 4

[Christian Reis]

I had not encountered the bizarre url module, which is what causes 2. I'm
conflict-blocked on this by bug XXX but will fix as soon as I have that checked
in. I have a fix for the iframe madness in hand.

Comment 5

[Christian Reis]

For the record, bug XXX is actually bug 252943. :-)

Comment 6

[Christian Reis]

Dammit. That figures. We need to rip out use of the URL module if we want to be
consistent: 

  The ordering of the parameters is non-deterministic due to fact that Perl's 
  hashes themselves are unordered. This isn't a problem as the ordering of CGI 
  parameters is insignificant (to the best of my knowledge).

from http://www.template-toolkit.org/docs/plain/Modules/Template/Plugin/URL.html

I'll do it, given it's only used in one file, but it's a shame.

Comment 7

[Christian Reis]

Attachment: kiko_v1: argh.

Went on cleanup rampage. Too much stuff there is kinda boogey.

Does anyone understand what the headers=0/1 parameter is for? If noone can, I
want to go ahead and rip it out as well. Whatever it is for, it looks pretty
broken, or at least misnamed.

Comment 8

[Christian Reis]

John, any clue as to what headers was meant to be? See

http://www.async.com.br/~kiko/mybugzilla/attachment.cgi?id=8&action=diff&headers=0

versus

http://www.async.com.br/~kiko/mybugzilla/attachment.cgi?id=8&action=diff&headers=1

-- ?

Comment 9

[Jouni Heikniemi]

Comment on attachment 154468 [details] [diff] [review]
kiko_v1: argh.

>Index: template/en/default/attachment/diff-header.html.tmpl
>===================================================================

>+[% BLOCK url %]attachment.cgi?id=[% id FILTER html %][% END %]

viewurl or whatever.

>+[% BLOCK interurl %]attachment.cgi?oldid=[% oldid FILTER html %]&newid=[% newid FILTER html %]&action=interdiff[% END %]

interdiffurl? 

Wrap the line to avoid exceeding 80 chars.

>+      <a href="[% INCLUDE url id=newid %]&amp;action=diff[% new_url %]">[% new_desc FILTER html %]</a> (#[% newid %])

Wrap.

new_url? Didn't you just remove its construction?

>+    | <a href="[% INCLUDE url id=attachid -%][%- "&context=$context&collapsed=$collapsed&headers=$headers&action=diff&format=raw" FILTER html %]">Raw&nbsp;Unified</a>

Wrap it. Besides, this is seriously wrong; you need to html encode - or, to be
exact, url_quote - each of the parameter values by itself. Now if one of the
params contains &'s, it gets wrongly encoded.

>+[% ELSE %]
>+    [%# add some spacing %]
>+    <br><br>

The comments looks like pretty unnecessary here... If it's needed, explain
where this "spacing" goes ("below xxx")

Comment 10

[Christian Reis]

(In reply to comment #9)
> >+[% BLOCK interurl %]attachment.cgi?oldid=[% oldid FILTER html
%]&newid=[% newid FILTER html %]&action=interdiff[% END %]
> 
> Wrap the line to avoid exceeding 80 chars.

You're being unfair to me: this is a URL, and wrapping inside the TT blocks
makes it so hard to read.

> >+      <a href="[% INCLUDE url id=newid %]&amp;action=diff[% new_url %]">[%
new_desc FILTER html %]</a> (#[% newid %])
> 
> Wrap.

HOW? :)
 
> new_url? Didn't you just remove its construction?

Missed that. thanks.

> >+    | <a href="[% INCLUDE url id=attachid -%][%-
"&context=$context&collapsed=$collapsed&headers=$headers&action=diff&format=raw"
FILTER html %]">Raw&nbsp;Unified</a>
> 
> Wrap it. Besides, this is seriously wrong; you need to html encode - or, to be
> exact, url_quote - each of the parameter values by itself. Now if one of the
> params contains &'s, it gets wrongly encoded.

Yeah.

Comment 11

[Christian Reis]

Attachment: kiko_v2: reviewed

I created specific url blocks for each action. I didn't do any wrapping -- this
may be ironic as I'm actually the wrappage fascist onboard, but the reality is
that URLs are very hard to wrap correctly. My patch does shorten then
significantly by using blocks. The other comments were observed.

Comment 12

[John Keiser (jkeiser)]

Kiko: headers=0 is used for the Edit Attachment screen, when you do View
Attachment As Diff.  The headers muck up the iframe view, so we link to it with
headers=0 from there.

Comment 13

[Christian Reis]

Hmmm. Maybe I'm against that redundant functionality anyway -- it's just a click
away with Diff, isn't it?

Comment 14

[John Keiser (jkeiser)]

No more redundant than the attachment view in the window ... the point is, you
look at the attachment while you are writing comments.  If it's on the same
page, it becomes a good deal easier--no switching between them.

IMO the View Attachment As Diff should become the default when you are in Edit
Attachment.

Comment 15

[Christian Reis]

Filed bug 254612 on a different way to solve that, perhaps using a different URI
for the header-enabled version (and removing all this confusing header=0 stuff).

Comment 16

[Christian Reis]

Attachment: kiko_v3: visual cleanups

Jouni took so long I went ahead and fixed this page to look nicer (jut
colouring headers correctly and tweaking font sizes). Take a look at:

  http://www.async.com.br/~kiko/mybugzilla/attachment.cgi?id=5&action=diff

Comment 17

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 156808 [details] [diff] [review]
kiko_v3: visual cleanups

re: wrapping, 80 chars isn't ironclad, and if longer lines are more readable in
specific cases (of which URLs in particular come to mind), then let's use them.

>-<table class="file_table"><thead><tr><td class="file_head" colspan="2"><a style="text-decoration: none" href="#" onclick="return twisty_click(this)">[% collapsed ? '(+)' : '(-)' %]</a><input type="checkbox" name="[% file.filename FILTER html %]"[% collapsed ? '' : ' checked' %] style="display: none"> 
>+<table class="file_table"><thead><tr><td class="file_head" colspan="2"><a style="text-decoration: none; font-family: monospace; font-size: 1.1em;" href="#" onclick="return twisty_click(this)">[% collapsed ? '(+)' : '(-)' %]</a><input type="checkbox" name="[% file.filename FILTER html %]"[% collapsed ? '' : ' checked' %] style="display: none"> 

Urk, it'd be great if these styles could get moved into a stylesheet or <STYLE>
tag.

>+[% BLOCK viewurl %]attachment.cgi?id=[% id FILTER html %][% END %]
>+[% BLOCK editurl %][% INCLUDE viewurl %]&amp;action=edit[% END %]
>+[% BLOCK diffurl %][% INCLUDE viewurl %]&amp;action=diff[% END %]
>+[% BLOCK interdiffurl %]attachment.cgi?oldid=[% oldid FILTER html %]&amp;newid=[% newid FILTER html %]&amp;action=interdiff&amp;format=raw[% END %]

IDs don't need filtering, and BLOCKs like these that don't harmfully change
variable values should be PROCESSed instead of INCLUDEd for performance.


>-      [% description FILTER html %] (#[% attachid %])
>+      Attachment [% description FILTER html %] (#[% attachid %])

This is going to be confusing for some attachment summaries, but it'll work if
you use the format "Attachment #<id>: <summary>".

Otherwise looks great.

Comment 18

[Christian Reis]

Thanks, will have an updated patch by thursday.

Comment 19

[Jouni Heikniemi]

(In reply to comment #17)
> (From update of attachment 156808 [details] [diff] [review])
> re: wrapping, 80 chars isn't ironclad, and if longer lines are more readable 
> in specific cases (of which URLs in particular come to mind), then let's use 
> them.

While I agree there's no need to be totally strict about 80 chars, most URLs
don't really gain readability by not being split to several lines. On the
contrary, splitting makes more information visible at a glance. If we're talking
about 100 vs. 80, leaving stuff on one line is easy to defend. If we're talking
about 250 vs. 80, splitting comes to mind much more easily.

Comment 20

[Jouni Heikniemi]

Comment on attachment 156808 [details] [diff] [review]
kiko_v3: visual cleanups

>Index: template/en/default/attachment/diff-file.html.tmpl
>===================================================================
>+  /* border-bottom the last open context section in the listing */
>+  border-bottom: 1px solid black;

Nit: "border-bottom" is not a verb. "Draw a border below the". 

In addition to that, I agree with Myk's comments. Resummarize the bug when you
attach your next patch.

Comment 21

[Christian Reis]

Attachment: kiko_v4: incorporate comments

This patch:
  - moves style information into the style BLOCK
  - uses attachid instead of id for the attachment id, avoiding filtering
  - changes the attachment number/summary ordering as per Myk's request
  - adds some spacing to the style block to make it more consistent

Comment 22

[Christian Reis]

Attachment: kiko_v5: fix most validation issues

As a bonus, I've fixed most of the validation issues this page has. There is
one left which is a javascript error which I consider to be out of scope and
hopefully you'll agree. I'll file a bug upon fixing this one to solve that.

Comment 23

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 157756 [details] [diff] [review]
kiko_v5: fix most validation issues

>  - uses attachid instead of id for the attachment id, avoiding filtering

(sorry if my earlier review comment about this was ambiguous)

You don't need to change "id" to "attachid" to avoid the need to pass the value
through the HTML filter, since regardless of the variable's name its value is
integer, and integer values contain no special HTML characters.  If the filter
test complains when you use "id," add "id" to the list of filter exceptions in
filterexceptions.pl.

Changing "id" to "attachid" is dangerous because it confusingly uses that
variable name to mean both "the global value representing the attachment
currently being diffed" and "the local value representing the attachment that
would be diffed if the user clicks this link," where the former sometimes but
not always equals the latter.  This introduces a dangerous possibility of
regressions (particularly when using PROCESS instead of INCLUDE to improve
performance) that we should avoid.

"attachid" should go back to being "id", just as in your previous patch, but
without HTML filtering, with "id" added to the filter exceptions list if
necessary, and with occurrences of "INCLUDE (view|edit|diff|interdiff)url"
changed to PROCESS.



>Index: template/en/default/attachment/diff-header.html.tmpl

>+[% BLOCK viewurl %]attachment.cgi?id=[% attachid %][% END %]
>+[% BLOCK editurl %][% PROCESS viewurl %]&action=edit[% END %]
>+[% BLOCK diffurl %][% PROCESS viewurl %]&action=diff[% END %]
>+[% BLOCK interdiffurl %]attachment.cgi?oldid=[% oldid %]&newid=[% newid %]&action=interdiff&format=raw[% END %]

"interdiffurl" is a misleading name for this block, since it only generates
URLs to raw interdiffs, not to interdiffs in general.  This should be called
something like "rawinterdiffurl".

Nit: blocks are best placed after template initialization (default variable
definition, etc.) but before content generation (think placement of functions
in a Perl script relative to the main body of code).



>-    [% USE url('attachment.cgi', newid = newid, oldid = oldid, action = 'interdiff') %]
>-    <a href="[% url(format = 'raw') %]">Raw Unified</a>
>-    [% IF attachid %]
>-    <br>
>-    [% ELSE %]
>+    <a href="[% INCLUDE interdiffurl newid=newid oldid=oldid %]">Raw Unified</a>

You don't need to pass newid and oldid here, since the block has access to the
calling block's context.

Otherwise it looks good; almost there.

Comment 24

[Christian Reis]

Attachment: kiko_v6: review comments go in

- Swapped INCLUDE for PROCESS
- Killed interdiffurl -- it was only used in one place and complicated things
more than simplified them
- Added id to filterexceptions
- Moved the BLOCKs down to right before output (hopefully).

Should be good as gold. The (+)/(-) are still really ugly but there is no good
way around that cross-browser that doesn't involve images, and I have some
images handy from my dependency tree work that might be usable in a future
patch.

Comment 25

[Christian Reis]

Comment on attachment 158365 [details] [diff] [review]
kiko_v6: review comments go in

Please ignore the spurious filterexception inclusion, I've fixed it here.

Comment 26

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 158365 [details] [diff] [review]
kiko_v6: review comments go in

Looks good, works. r=myk

Comment 27

[Myk Melez [:myk] [@mykmelez]]

This is low enough risk, and a useful enough polish to our URLs, to go into 2.18.

Comment 28

[Christian Reis]

HEAD:

/cvsroot/mozilla/webtools/bugzilla/template/en/default/filterexceptions.pl,v 
<--  filterexceptions.pl
new revision: 1.22; previous revision: 1.21
/cvsroot/mozilla/webtools/bugzilla/template/en/default/attachment/diff-file.html.tmpl,v
 <--  diff-file.html.tmpl
new revision: 1.4; previous revision: 1.3
/cvsroot/mozilla/webtools/bugzilla/template/en/default/attachment/diff-header.html.tmpl,v
 <--  diff-header.html.tmpl
new revision: 1.6; previous revision: 1.5
/cvsroot/mozilla/webtools/bugzilla/template/en/default/attachment/edit.html.tmpl,v
 <--  edit.html.tmpl
new revision: 1.25; previous revision: 1.24

Branch (fitting in a minor fix to list to make the consistency make sense)

/cvsroot/mozilla/webtools/bugzilla/template/en/default/filterexceptions.pl,v 
<--  filterexceptions.pl
new revision: 1.16.2.1; previous revision: 1.16
/cvsroot/mozilla/webtools/bugzilla/template/en/default/attachment/diff-file.html.tmpl,v
 <--  diff-file.html.tmpl
new revision: 1.2.2.1; previous revision: 1.2
/cvsroot/mozilla/webtools/bugzilla/template/en/default/attachment/diff-header.html.tmpl,v
 <--  diff-header.html.tmpl
new revision: 1.4.2.2; previous revision: 1.4.2.1
/cvsroot/mozilla/webtools/bugzilla/template/en/default/attachment/edit.html.tmpl,v
 <--  edit.html.tmpl
new revision: 1.24.2.1; previous revision: 1.24
/cvsroot/mozilla/webtools/bugzilla/template/en/default/attachment/list.html.tmpl,v
 <--  list.html.tmpl
new revision: 1.17.2.1; previous revision: 1.17

Thanks. Just found other cases of action=view, though. :-(

Comment 29

[[e-mail]]

Comment on attachment 158365 [details] [diff] [review]
kiko_v6: review comments go in

> [% title = BLOCK %]
>   [% IF attachid %]
>-Attachment #[% attachid %] for [% terms.Bug %] #[% bugid %]
>+Attachment #[% attachid %] for [% terms.bug %] #[% bugid %]
>   [% ELSE %]
>-Interdiff of #[% oldid %] and #[% newid %] for #[% terms.Bug %] #[% bugid %]
>+Interdiff of #[% oldid %] and #[% newid %] for #[% terms.bug %] #[% bugid %]
>   [% END %]
> [% END %]
I see the old code was wrong too, anyway, fyi, the Interdiff line has an
extraneous # before the [% terms.Bug %]

Comment 30

[[e-mail]]

You can still get an &action=view in at least one case, but it's not a link ;-)