Last Comment Bug 5179 - Need to be able to put attachment on new bug.
: Need to be able to put attachment on new bug.
Status: RESOLVED FIXED
[content:attachments] enter_bug
: selenium
Product: Bugzilla
Classification: Server Software
Component: Creating/Changing Bugs (show other bugs)
: unspecified
: All All
P3 enhancement with 16 votes (vote)
: Bugzilla 3.0
Assigned To: Marc Schumann [:Wurblzap]
: default-qa
:
Mentors:
: 43161 127274 140863 142519 157205 202423 232645 279901 (view as bug list)
Depends on:
Blocks: 341934 302684 315860 341932 367077
  Show dependency treegraph
 
Reported: 1999-04-16 08:44 PDT by Mike Shaver (:shaver -- probably not reading bugmail closely)
Modified: 2007-02-12 12:36 PST (History)
25 users (show)
justdave: approval+
LpSolit: documentation+
LpSolit: testcase+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch (44.91 KB, patch)
2006-04-14 06:32 PDT, Marc Schumann [:Wurblzap]
no flags Details | Diff | Splinter Review
Patch 1.2 (49.00 KB, patch)
2006-05-14 15:33 PDT, Marc Schumann [:Wurblzap]
LpSolit: review-
Details | Diff | Splinter Review
Patch 1.3 (49.03 KB, patch)
2006-06-12 09:36 PDT, Marc Schumann [:Wurblzap]
LpSolit: review+
Details | Diff | Splinter Review

Description User image Mike Shaver (:shaver -- probably not reading bugmail closely) 1999-04-16 08:44:19 PDT
I'd like to be able to upload a stack trace or test file when I file a new bug,
not just later.
Comment 1 User image Terry Weissman 1999-04-19 18:20:59 PDT
Reassigning to dmose@mozilla.org, who now has front-line responsibility for
all Bonsai and Bugzilla bugs.
Comment 2 User image Eli Goldberg 1999-04-21 13:28:59 PDT
[CC:ing self, so that the bug writing guidelines can be promptly updated if this
alteration is made.]
Comment 3 User image Terry Weissman 1999-04-22 11:55:59 PDT
Reassigning back to me.  That stuff about me no longer being the front-line
person responsible for Bugzilla and Bonsai turned out to be short-lived.
Please pardon our confusion, and I'm very sorry about the spam.
Comment 4 User image Terry Weissman 1999-05-05 15:23:59 PDT
The problem here is (mostly) one of UI.  I don't want to cram the full "specify
an attachment" form into the main "submit a bug" UI.  Submitting a bug should be
as painless and easy as we can make it, so that very casual bugzilla users will
not be scared away from the idea of submitting bugs.

So, if I don't cram all this stuff there, then the best I can do is put a
checkbox that says "I have a file I'd like to attach", and then get the details
later.  This works out to almost the same as the current behavior, where you can
create a bug, then click on the link to bring up the bug, then click on the
"create attachment" link.  The checkbox is slightly better, in that it would
have one less click, and two less page loads.  And is more obvious.

It turns out that implementing the checkbox is a bit hard, but I've done the
easy thing for now.  Which is to put a link up immediately after creating a bug
that says "attach a file to this new bug".  This works out to the same number of
clicks as the checkbox, but one more page load.  And is a bit less obvious.

Good enough?
Comment 5 User image Peter Maydell 1999-07-29 08:24:59 PDT
Personally I think it's still not wonderful, in that it separates out a single
conceptual operation ("here is a patch, and here is my long comment on what it
is") into two actions, and results in everybody getting two pieces of Bugzilla
email rather than one.

A simple improvement would be to add a text box to the 'add attachment' page
that allowed you to alter the long description field simultaneously with adding
the attachment. This improves the UI for adding things to already-open bugs,
although the problem remains for creating a bug report with an attachment.

This should be easy to implement: just change createattachment.cgi to:
 (1) put a suitable text box in the HTML it outputs
 (2) use the contents of that text box in the call to AppendComment() where the
short description is currently used.

I might submit a patch if I feel sufficiently bored :-)
Comment 6 User image Aleksey Nogin 2000-01-19 16:23:59 PST
On a related note - it would be nice to be able to add several attachments in a
single commit.

I believe the best solution (from the UI perspective) would be to add a
"Preview" button along with the "Commit" button. The "Preview" button would
allow a person to review the changes, cancel them, modify them (by pressing the
"Back" button on the browser) and commit them as well as make some additional
changes (such as attaching a file) before commiting.
Comment 7 User image Matthew Tuck [:CodeMachine] 2000-03-08 04:25:57 PST
It would probably be best to file the multiple attach issue as a separate bug
report.
Comment 8 User image Matthew Tuck [:CodeMachine] 2000-03-09 03:24:24 PST
See the super dooper new bug #31144 for a proposal which would allow this and
more.
Comment 9 User image Terry Weissman 2000-04-13 07:36:55 PDT
tara@tequilarista.org is the new owner of Bugzilla and Bonsai.  (For details,
see my posting in netscape.public.mozilla.webtools,
news://news.mozilla.org/38F5D90D.F40E8C1A%40geocast.com .)
Comment 10 User image Chris Yeh 2000-08-25 21:05:43 PDT
*** Bug 43161 has been marked as a duplicate of this bug. ***
Comment 11 User image Stephen Lee 2001-02-23 06:03:36 PST
One of the users on our internal bugzilla system was wanting to add a comment 
and an attachment at the same time. At present when submitting an attachment, 
you need to do two bug updates, one to give the comments describing what the 
attachment is, and another to actually add the attachment.

ISTM that it would be useful either to have an "Additional Comments" field on 
the createattachment.cgi, or to be able to add attachment fields onto the 
standard output from bug_form.pl. The latter could work in a similar manner to 
the "And" and "Or" buttons on the query page.
Comment 12 User image Zach Lipton [:zach] 2001-04-28 16:25:31 PDT
2.18 watch!
Comment 13 User image Dave Miller [:justdave] (justdave@bugzilla.org) 2001-07-05 09:40:59 PDT
It's also been noted that the current method sends out two emails when you add a 
patch and comment on it.  If it was all done as the same action it could generate 
a single email.
Comment 14 User image Andreas Franke (gone) 2001-09-01 11:51:32 PDT
-> Bugzilla product, Creating Bugs component, reassigning.
Comment 15 User image Andreas Franke (gone) 2002-02-22 14:33:44 PST
*** Bug 127274 has been marked as a duplicate of this bug. ***
Comment 16 User image mental 2002-04-29 04:42:11 PDT
*** Bug 140863 has been marked as a duplicate of this bug. ***
Comment 17 User image Andreas Franke (gone) 2002-05-06 08:58:15 PDT
*** Bug 142519 has been marked as a duplicate of this bug. ***
Comment 18 User image Jouni Heikniemi 2002-07-15 23:27:26 PDT
*** Bug 157205 has been marked as a duplicate of this bug. ***
Comment 19 User image Jacob Steenhagen 2002-10-17 08:56:05 PDT
The add attachment stuff has been updated to allow comments to be added at the
same time an attachment is made, is still doesn't allow attachments to be made
to new bugs, but it does at least cover what was mentioned in comment #5 (and
also #11).
Comment 20 User image Guilliano Balandres 2002-12-09 04:08:31 PST
I see a problem with the implementation of this feature.
I've modified my local bugzilla version so that i can add an attachment on new 
bugs. You are running into problems with that because the form must be posted 
with enctype="multipart/form-data" and it seems like the Bugzilla CGI.pl cant 
extract the FORM header correctly if posted as enctype="multipart/form-data" 
and any field contains some special characters like „a string“.
I detail some Form Field values are not extracted if enctype="multipart/form-
data" and any textfield contains special characters.

I am not absolutly sure about that but i didnt do much modifications to the 
original enter_bug.cgi and post_bug.cgi and i am dont getting that undefined 
Form Field Value errors if enctype != "multipart/form-data".

If anybody has an idea how one could prevent that error. His thoughts would be 
highly appreciated by me.

Maybe anyone could reproduce that problem?

Maybe that error wont happen if the standard CGI Module would be used within 
Bugzilla?
Comment 21 User image Dave Miller [:justdave] (justdave@bugzilla.org) 2002-12-09 09:14:28 PST
Bugzilla does use the standard CGI module, as of about a month or two ago (it
was prior to 2.17.1 being posted I believe).
Comment 22 User image Dave Miller [:justdave] (justdave@bugzilla.org) 2003-04-17 09:54:31 PDT
comment #20 has been dealt with by using Perl's CGI module to handle the form
data...
Comment 23 User image Dave Miller [:justdave] (justdave@bugzilla.org) 2003-04-17 09:55:23 PDT
*** Bug 202423 has been marked as a duplicate of this bug. ***
Comment 24 User image Vlad Dascalu 2004-01-30 02:51:29 PST
*** Bug 232645 has been marked as a duplicate of this bug. ***
Comment 25 User image Dave Miller [:justdave] (justdave@bugzilla.org) 2004-03-14 23:13:54 PST
Enhancements which don't currently have patches on them which are targetted at
2.18 are being retargetted to 2.20 because we're about to freeze for 2.18. 
Consideration will be taken for moving items back to 2.18 on a case-by-case
basis (but is unlikely for enhancements)
Comment 26 User image Dave Miller [:justdave] (justdave@bugzilla.org) 2004-09-18 17:35:50 PDT
Bugzilla 2.20 feature set is now frozen as of 15 Sept 2004.  Anything flagged
enhancement that hasn't already landed is being pushed out.  If this bug is
otherwise ready to land, we'll handle it on a case-by-case basis, please set the
blocking2.20 flag to '?' if you think it qualifies.
Comment 27 User image Dave Miller [:justdave] (justdave@bugzilla.org) 2004-09-29 15:54:40 PDT
In the modern world it might be possible to pull this off without cluttering the
UI by putting the fields in the form and hide them via css.  Have the checkbox
for "I'd like to include an attached file" which unhides the relevant fields
when checked.
Comment 28 User image Byron Jones ‹:glob› 2005-01-26 07:06:53 PST
*** Bug 279901 has been marked as a duplicate of this bug. ***
Comment 29 User image Marc Schumann [:Wurblzap] 2006-04-14 06:32:36 PDT
Created attachment 218418 [details] [diff] [review]
Patch

This works basically. Perhaps somebody can take a look at it?

There is a known deficiency: attachment creation failures during bug creation aren't handled very well (read: Throw*Error hides successful bug creation, which is not good). They should be caught silently, so that we can display a message along the lines of "bug creation succeeded, but attachment creation failed. Please go to the bug and try attaching again".
Comment 30 User image Marc Schumann [:Wurblzap] 2006-04-14 06:37:31 PDT
Oh yeah, and flags don't work either.
Comment 31 User image Frédéric Buclin 2006-05-07 15:39:59 PDT
Comment on attachment 218418 [details] [diff] [review]
Patch

Let's put it in my radar.
Comment 32 User image Frédéric Buclin 2006-05-13 06:54:43 PDT
I like your patch, Marc (I haven't tested it yet, nor reviewed it in great details though). Just two minor questions:

1) why keeping some validation routines in attachment.cgi? Looks like you only moved into Attachment.pm those you need from post_bug.cgi, but maybe it would appear more logical to have them all in the same module (Attachment.pm).

2) I'm not a big fan using CGI data from Attachment.pm. I would have prefer collecting all required data in attachment.cgi and passing a hash to Attachment.pm instead. Did you think about this alternative?

Anyway, both are not critical and could be fixed in separate bugs, if desired. I will try to review this patch tomorrow evening, when I'm back home.
Comment 33 User image Marc Schumann [:Wurblzap] 2006-05-14 15:33:56 PDT
Created attachment 221991 [details] [diff] [review]
Patch 1.2

- Better error handling
- Friendlier to non-CSS browsers
- Friendlier to non-JS browsers

(In reply to comment #32)
> 1) why keeping some validation routines in attachment.cgi? Looks like you only
> moved into Attachment.pm those you need from post_bug.cgi, but maybe it would
> appear more logical to have them all in the same module (Attachment.pm).

Maybe... Let's decide when we know how the routines need to be modified. If we're clear about the structure they need to take, then perhaps we can move the yet-missing ones along, too.

> 2) I'm not a big fan using CGI data from Attachment.pm. I would have prefer
> collecting all required data in attachment.cgi and passing a hash to
> Attachment.pm instead. Did you think about this alternative?

I agree this would be cleaner. Two things come to mind, though. It would require code duplication accross enter_bug.cgi and attachment.cgi. And I don't know whether it's a good idea to put the uploaded file into a hash. Maybe we can find solutions for these.
Comment 34 User image Frédéric Buclin 2006-05-15 14:21:15 PDT
You know what? I don't like this attachment_errorhandler() function and the way these validate_foo() work. IMO, you should still use ThrowUserError() in them. Maybe something like:

Bugzilla::Attachment->validate_foo({no_error => 1});

and in validate_foo():

$vars->{'no_error'} ? return 0 : ThrowUserError();

One reason is that &$errorhandler("missing_content_type") is really designed for ThrowUserError(). Another reason is that 012throwables.t will be unable to find these error tags and will fail. And I strongly disagree to fix 012throwables.t in any way to match your changes.
Comment 35 User image Marc Schumann [:Wurblzap] 2006-05-15 14:29:16 PDT
Ok, your idea is more straightforward and not as muddled as mine. I'll upload a new patch.
Comment 36 User image Frédéric Buclin 2006-05-15 14:37:04 PDT
(In reply to comment #35)
> Ok, your idea is more straightforward and not as muddled as mine. I'll upload a
> new patch.
> 

Oh, and even easier: you could use the THROW_ERROR constant, see Constants.pm:

Bugzilla::Attachment->validate_foo(THROW_ERROR);
Bugzilla::Attachment->validate_foo();

and validate_foo() would get this constant in: $throw_error = shift;
Comment 37 User image Frédéric Buclin 2006-05-16 13:55:12 PDT
Comment on attachment 221991 [details] [diff] [review]
Patch 1.2

Per my previous comments. I will wait for an updated patch.
Comment 38 User image Marc Schumann [:Wurblzap] 2006-06-12 09:36:38 PDT
Created attachment 225282 [details] [diff] [review]
Patch 1.3

Addressed comments.
Comment 39 User image Frédéric Buclin 2006-06-17 14:18:01 PDT
Comment on attachment 225282 [details] [diff] [review]
Patch 1.3

>Index: Bugzilla/Attachment.pm

>+=item C<insert_attachment_for_bug($bug_id)>
>+
>+sub insert_attachment_for_bug {
>+    my ($class, $throw_error, $bug_id, $user, $timestamp, $hr_vars) = @_;

You must write: C<insert_attachment_for_bug($throw_error, $bug_id, $user, $timestamp, $hr_vars)> as you have to enumerate all arguments.


>+    else {
>+        $filename = _validate_filename($throw_error) || return 0;
>+        # need to validate content type before data as
>+        # we now check the content type for image/bmp in validateData()

Nit: s/validateData()/_validate_data/



>Index: template/en/default/attachment/createformcontents.html.tmpl

>+    [% IF flag_types.size > 0 %]
>+      [% PROCESS "flag/list.html.tmpl" bug_id=bugid attach_id=attachid %]<br>
>+    [% END %]

Write [% IF flag_types && flag_types.size > 0 %], else you are filling your apache error log:

enter_bug.cgi: Argument "" isn't numeric in numeric gt (>) at data/template/template/en/default/attachment/createformcontents.html.tmpl line 40.

That's because you cannot use flags in enter_bug.cgi and so flag_types is undefined.


You can easily fix all these comments on checkin. Your patch is working fine; I saw no regressions. r=LpSolit
Comment 40 User image Dave Miller [:justdave] (justdave@bugzilla.org) 2006-06-17 15:43:19 PDT
We might want to come up with a way to make the "messages" type messages show up a little louder when they indicate a "partial success, partial failure" condition (there's other places besides this bug where that's an issue, too, like failed review matches).  But that's another bug, and this looks awesome otherwise. :)
Comment 41 User image Frédéric Buclin 2006-06-17 16:26:30 PDT
Committing this patch now because it conflicts with bug 283926.


Checking in attachment.cgi;
/cvsroot/mozilla/webtools/bugzilla/attachment.cgi,v  <--  attachment.cgi
new revision: 1.111; previous revision: 1.110
done
Checking in post_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/post_bug.cgi,v  <--  post_bug.cgi
new revision: 1.147; previous revision: 1.146
done
Checking in Bugzilla/Attachment.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Attachment.pm,v  <--  Attachment.pm
new revision: 1.33; previous revision: 1.32
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/js/attachment.js,v
done
Checking in js/attachment.js;
/cvsroot/mozilla/webtools/bugzilla/js/attachment.js,v  <--  attachment.js
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/skins/standard/create_attachment.css,v
done
Checking in skins/standard/create_attachment.css;
/cvsroot/mozilla/webtools/bugzilla/skins/standard/create_attachment.css,v  <--  create_attachment.css
initial revision: 1.1
done
Checking in template/en/default/attachment/create.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/attachment/create.html.tmpl,v  <--  create.html.tmpl
new revision: 1.27; previous revision: 1.26
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/template/en/default/attachment/createformcontents.html.tmpl,v
done
Checking in template/en/default/attachment/createformcontents.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/attachment/createformcontents.html.tmpl,v  <--  createformcontents.html.tmpl
initial revision: 1.1
done
Checking in template/en/default/bug/create/create.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/create/create.html.tmpl,v  <--  create.html.tmpl
new revision: 1.60; previous revision: 1.59
done
Checking in template/en/default/global/messages.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/messages.html.tmpl,v  <--  messages.html.tmpl
new revision: 1.35; previous revision: 1.34
done
Comment 42 User image Max Kanat-Alexander 2006-09-07 17:03:58 PDT
Added to relnotes as part of bug 349423.
Comment 43 User image Frédéric Buclin 2006-11-22 08:00:27 PST
Documentation updated as part of the doc patch from bug 174039.

Note You need to log in before you can comment on or make changes to this bug.