Closed Bug 87770 Opened 20 years ago Closed 17 years ago

attachment.cgi should work with no parameters

Categories

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

2.13
defect

Tracking

()

RESOLVED FIXED
Bugzilla 2.18

People

(Reporter: CodeMachine, Assigned: bugzilla)

References

()

Details

Attachments

(2 files, 3 obsolete files)

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.
Target Milestone: --- → Bugzilla 2.16
Priority: -- → P4
Moving to new Bugzilla product ...
Assignee: tara → myk
Component: Bugzilla → Creating/Changing Bugs
Product: Webtools → Bugzilla
Version: Bugzilla 2.13 → 2.13
Depends on: 98602
No longer depends on: 98602
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.
Target Milestone: Bugzilla 2.16 → Bugzilla 2.18
createattachment is no more, but attachment.cgi has the same issue
Summary: createattachment should work with no parameters. → attachment.cgi should work with no parameters
Component: Creating/Changing Bugs → attachment and request management
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.
Target Milestone: Bugzilla 2.18 → Bugzilla 2.20
Assignee: myk → bugzilla
Status: NEW → ASSIGNED
Attachment #145043 - Flags: review?
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.
Attachment #145043 - Flags: review? → review-
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 :)
I haven't done the spacing here, but I may do that later this week, as I change
all the $::FORM usage...
Attachment #145043 - Attachment is obsolete: true
Attachment #145048 - Flags: review?
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 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
Attachment #145048 - Flags: review? → review+
Flags: approval?
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]
Flags: approval? → approval-
Attachment #145048 - Attachment is obsolete: true
Attachment #145198 - Flags: review?
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.
Attachment #145198 - Flags: review? → review+
Flags: approval- → approval?
>  <p>Attachment id: <input name="id" size="6">

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

Please fix this before checking in.

a=myk
Flags: approval? → approval+
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
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Target Milestone: Bugzilla 2.20 → Bugzilla 2.18
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...
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
This patch doesn't rely on the text on the buttons, and so can be localised
properly in the template
Attachment #145820 - Flags: review?
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.
Attachment #145820 - Flags: review? → review+
Status: REOPENED → ASSIGNED
Flags: approval+ → approval?
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
Attachment #145820 - Flags: review-
.. thus making this page not usable in Lynx? I know of at least one person who's
going to complain :-)
> .. 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.
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 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.
> 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.
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
Attachment #145820 - Attachment is obsolete: true
Attachment #146035 - Flags: review?(myk)
Comment on attachment 146035 [details] [diff] [review]
kiko_v1: use button

r=myk

&nbsp; 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.
Attachment #146035 - Flags: review?(myk) → review+
Flags: approval?
Flags: approval? → approval+
/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.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago17 years ago
Resolution: --- → FIXED
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?
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.
Blocks: 461083
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.