attachment.cgi should work with no parameters

RESOLVED FIXED in Bugzilla 2.18

Status

()

defect
P4
trivial
RESOLVED FIXED
18 years ago
7 years ago

People

(Reporter: CodeMachine, Assigned: bugzilla)

Tracking

2.13
Bugzilla 2.18
Dependency tree / graph
Bug Flags:
approval +

Details

()

Attachments

(2 attachments, 3 obsolete attachments)

Reporter

Description

18 years ago
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.
Reporter

Updated

18 years ago
Target Milestone: --- → Bugzilla 2.16
Reporter

Updated

18 years ago
Priority: -- → P4
Reporter

Comment 1

18 years ago
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

Updated

17 years ago
Component: Creating/Changing Bugs → attachment and request management

Updated

17 years ago

Comment 4

16 years ago
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

Comment 5

15 years ago
Assignee: myk → bugzilla
Status: NEW → ASSIGNED

Updated

15 years ago
Attachment #145043 - Flags: review?

Comment 6

15 years ago
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 7

15 years ago
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 :)
Assignee

Comment 8

15 years ago
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
Assignee

Updated

15 years ago
Attachment #145048 - Flags: review?
Assignee

Comment 9

15 years ago
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

15 years ago
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+

Updated

15 years ago
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-
Assignee

Comment 12

15 years ago
Attachment #145048 - Attachment is obsolete: true
Assignee

Updated

15 years ago
Attachment #145198 - Flags: review?

Comment 13

15 years ago
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+

Updated

15 years ago
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+

Comment 15

15 years ago
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: 15 years ago
Resolution: --- → FIXED
Target Milestone: Bugzilla 2.20 → Bugzilla 2.18
Assignee

Comment 16

15 years ago
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 → ---
Assignee

Comment 17

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

Updated

15 years ago
Attachment #145820 - Flags: review?

Comment 18

15 years ago
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+

Updated

15 years ago
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-

Comment 20

15 years ago
.. 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.

Comment 25

15 years ago
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

Updated

15 years ago
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+

Updated

15 years ago
Flags: approval?
Flags: approval? → approval+

Comment 27

15 years ago
/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: 15 years ago15 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.

Updated

11 years ago
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.