87770 - attachment.cgi should work with no parameters

Status: RESOLVED FIXED

(Bugzilla :: Attachments & Requests, defect, P4)

Description

[Matthew Tuck [:CodeMachine]]

Most of the cgis in the product, if you type them in directly with no
parameters, would do something reasonable.  However, createattachment will give
a software error.  It should work like show_bug.cgi which asks for the bug id.

Comment 1

[Matthew Tuck [:CodeMachine]]

Moving to new Bugzilla product ...

Comment 2

[Dave Miller [:justdave]]

We are currently trying to wrap up Bugzilla 2.16.  We are now close enough to
release time that anything that wasn't already ranked at P1 isn't going to make
the cut.  Thus this is being retargetted at 2.18.  If you strongly disagree with
this retargetting, please comment, however, be aware that we only have about 2
weeks left to review and test anything at this point, and we intend to devote
this time to the remaining bugs that were designated as release blockers.

Comment 3

[Bradley Baetz (:bbaetz)]

createattachment is no more, but attachment.cgi has the same issue

Comment 4

[Joel Peshkin]

Unloved bugs targetted for 2.18 but untouched since 9-15-2003 are being
retargeted to 2.20
If you plan to act on one immediately, go ahead and pull it back to 2.18.

Comment 5

[GavinS]

Attachment: V1: allow the attachment id to be specified

Comment 6

[Christian Reis]

Comment on attachment 145043 [details] [diff] [review]
V1: allow the attachment id to be specified

>Index: attachment.cgi
>+    if ($param == 'id' &&
>+        ! $::FORM{'id'} ) {

Don't use FORM, use cgi->param("id") here.

>+        if ($::FORM{'action'} eq 'edit') {

Same.

>+++ template/en/default/attachment/choose.html.tmpl	Mon Mar 29 22:29:41 2004
>+      <option value="view" [% IF action == 'view' %] selected [% END %]>View</option>
>+      <option value="edit" [% IF action == 'edit' %] selected [% END %]>Edit</option>

An übernit here would be using

  [% 'selected' IF action == 'edit' %]

and avoiding the explicit END here.

Other than that looks okay.

Comment 7

[Vlad Dascalu]

Comment on attachment 145043 [details] [diff] [review]
V1: allow the attachment id to be specified

+{
   validateID();
-  view(); 
+  view();
 }

You could switch to 4 spaces ident style here while you're at it anyway :)

Comment 8

[GavinS]

Attachment: V2: review comments addressed (apart from the spacing one)

I haven't done the spacing here, but I may do that later this week, as I change
all the $::FORM usage...

Comment 9

[GavinS]

Side note to this bug: There are 2 review- flags shown for the first attachment on this bug now, but the 
one from Vlad is not shown in the Bug Activity - it should be, shouldn't it?

Comment 10

[Christian Reis]

Comment on attachment 145048 [details] [diff] [review]
V2: review comments addressed (apart from the spacing one)

>Index: attachment.cgi
>+    if ($param == 'id' &&
>+        ! $cgi->param('id') ) {

This line doesn't need a linebreak anymore.

>+        if ($cgi->param('action') eq 'edit') {
>+            $vars->{'action'} = 'edit';
>+        }
>+        else {
>+            $vars->{'action'} = 'view';
>+        }

I just wonder if you wanted to do a 

  my $vars; 

before this section, since there's nothing else you'd want to supply to this
template, right?

>+    You may <select name="action">
>+      <option value="view" [% 'selected' IF action == 'view' %]>View</option>
>+      <option value="edit" [% 'selected' IF action == 'edit' %]>Edit</option>
>+    </select> attachment id:

I don't particularly *like* the way this is worded, but I can't come up with a
2-minute suggestion either, so if you can improve on it, great -- otherwise not
a blocker.

r=kiko with the above comments addressed

Comment 11

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

Err, sorry, but the UI for this needs revision.  Specifically, "Edit" and "View"
should be buttons rather than options in a drop-down menu, f.e.:

Access an attachment by entering its ID into the form below:

Attachment ID: _______ [Edit] [View]

Since many users of this page will get here by accident, it would also be useful
(although not an approval blocker) to include an easy way to get to attachments
from a bug report, f.e.:

Alternately, access it from the list of attachments in its associated bug report:

Bug ID: ________ [View]

Comment 12

[GavinS]

Attachment: V3: Myk's GUI comments addressed

Comment 13

[Christian Reis]

Comment on attachment 145198 [details] [diff] [review]
V3: Myk's GUI comments addressed

I don't like the "awkward extra checks" but it seems to be the only way to have
a single input box and two buttons. 

>Index: show_bug.cgi
>-if (!defined $cgi->param('id') && $single) {
>+if ((!defined $cgi->param('id') || $cgi->param('id') eq '') 

nit: this is equivalent to the shorter

  if (!$cgi->param('id') && single) {

>+++ template/en/default/attachment/choose.html.tmpl	Wed >+[% PROCESS global/header.html.tmpl
>+   title = "Search by attachment number"
>+ %]

Perhaps we could use "Locate attachment" here.

Both are nits, non-blocking.

Comment 14

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

>  <p>Attachment id: <input name="id" size="6">

Typo: "Attachment id" -> "Attachment ID"

Please fix this before checking in.

a=myk

Comment 15

[Vlad Dascalu]

Checked in with the above suggestions.

FIXED.

Checking in attachment.cgi;
/cvsroot/mozilla/webtools/bugzilla/attachment.cgi,v  <--  attachment.cgi
new revision: 1.53; previous revision: 1.52
done
Checking in show_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/show_bug.cgi,v  <--  show_bug.cgi
new revision: 1.29; previous revision: 1.28
done
RCS 
file: /cvsroot/mozilla/webtools/bugzilla/template/en/default/attachment/choose.h
tml.tmpl,v
done
Checking in template/en/default/attachment/choose.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/attachment/choose.html.tm
pl,v  <--  choose.html.tmpl
initial revision: 1.1
done

Comment 16

[GavinS]

Only after you all reviewed, approved, and checked this in did it occur to me to check it for localisation 
- and as it stood, ithe patch was not localisable via the template.

Minor tweak in a mo...

Comment 17

[GavinS]

Attachment: Add-on patch to the just-checked-in fix to ensure localisation is possible

This patch doesn't rely on the text on the buttons, and so can be localised
properly in the template

Comment 18

[Vlad Dascalu]

Comment on attachment 145820 [details] [diff] [review]
Add-on patch to the just-checked-in fix to ensure localisation is possible

We try to get ride of the $::FORM thing because it's not compatible with
mod_perl and we want to provide mod_perl support for Bugzilla. We try to use
instead $cgi->param('view_btn') or similar alternatives. There is a bug opened
about this conversion. However, since attachment.cgi wasn't yet touched by this
conversion effort, I guess we can check this in and do the conversion for all
attachment.cgi once.

Personally in situations like this one I prefer a new opened bug, because now I
can't request approval on this patch without destroying myk's "a+" for the
previous patch.

Comment 19

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

Comment on attachment 145820 [details] [diff] [review]
Add-on patch to the just-checked-in fix to ensure localisation is possible

Use HTML 4 buttons for this instead, i.e.:

<button type="submit" name="action" value="edit">Edit</button>

http://www.w3.org/TR/html4/interact/forms.html#h-17.5

Comment 20

[Christian Reis]

.. thus making this page not usable in Lynx? I know of at least one person who's
going to complain :-)

Comment 21

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

> .. thus making this page not usable in Lynx?

Yes, if Lynx doesn't support this simple aspect of the years-old HTML 4 standard.

> I know of at least one person who's going to complain :-)

You can't please everyone.

Comment 22

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

I just tested Lynx 2.8.5, and it supports BUTTON tags.  It renders a BUTTON tag
as a working button, although it uses the value of its "value" attribute or the
string "(Submit)" as the label for the button instead of using the contents of
the tag.

Comment 23

[Dave Miller [:justdave]]

Comment on attachment 145820 [details] [diff] [review]
Add-on patch to the just-checked-in fix to ensure localisation is possible

All the graphing stuff uses this trick for the buttons, I don't see any problem
with it.  Although the graphing stuff still uses "action" in the name, and just
appends the action name to the button name, like "action-edit" or
"action-view".	There's a thing at the begining of chart.cgi that looks for a
param name starting with action- and sets action's value to the suffix.

Comment 24

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

> All the graphing stuff uses this trick for the buttons, I don't see any
> problem with it.

It's not a significant problem, but it's unnecessarily added complexity.  Better
to use the standards and simplify the program logic unless there's a good reason
otherwise, which Lynx, especially given its support for the standards, is not.

Comment 25

[Christian Reis]

Attachment: kiko_v1: use button

The &nbsps there make the buttons have equal width, which I appreciate
visually. I can remove them while checking in if they are deemed to be
problems.

See this live! http://www.async.com.br/~kiko/mybugzilla/attachment.cgi

Comment 26

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

Comment on attachment 146035 [details] [diff] [review]
kiko_v1: use button

r=myk

  seems unreliable for maintaining button width, but I guess it's OK. 
Please note its purpose in your checkin comment so future hackers know why it's
there.

Comment 27

[Christian Reis]

/cvsroot/mozilla/webtools/bugzilla/attachment.cgi,v  <--  attachment.cgi
new revision: 1.54; previous revision: 1.53
/cvsroot/mozilla/webtools/bugzilla/template/en/default/attachment/choose.html.tmpl,v
 <--  choose.html.tmpl
new revision: 1.2; previous revision: 1.1

Thanks.

Comment 28

[Justin Wood (:Callek)]

I personally would NOT have used &nbsp for the width issue, different fonts have
different "non-breaking space" width.

As an alternative, use CSS?  so those with proper HTML4 browsers see it as
intended, those with older browsers, will see just a slightly smaller button (no
big deal anyway).

Comments?

Comment 29

[Dave Miller [:justdave]]

yeah, css using a font-relative width measure like em would be a good way to do
it.  I don't think it's worth arguing over though.