5179 - Need to be able to put attachment on new bug.

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, enhancement, P3)

Description

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

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

[Terry Weissman]

Reassigning to [e-mail], who now has front-line responsibility for
all Bonsai and Bugzilla bugs.

Comment 2

[Eli Goldberg]

[CC:ing self, so that the bug writing guidelines can be promptly updated if this
alteration is made.]

Comment 3

[Terry Weissman]

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

[Terry Weissman]

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

[Peter Maydell]

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

[Aleksey Nogin]

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

[Matthew Tuck [:CodeMachine]]

It would probably be best to file the multiple attach issue as a separate bug
report.

Comment 8

[Matthew Tuck [:CodeMachine]]

See the super dooper new bug #31144 for a proposal which would allow this and
more.

Comment 9

[Terry Weissman]

[e-mail] 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

[Chris Yeh]

*** Bug 43161 has been marked as a duplicate of this bug. ***

Comment 11

[Stephen Lee]

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

[Zach Lipton [:zach]]

2.18 watch!

Comment 13

[Dave Miller [:justdave]]

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

[Andreas Franke (gone)]

-> Bugzilla product, Creating Bugs component, reassigning.

Comment 15

[Andreas Franke (gone)]

*** Bug 127274 has been marked as a duplicate of this bug. ***

Comment 16

[mental]

*** Bug 140863 has been marked as a duplicate of this bug. ***

Comment 17

[Andreas Franke (gone)]

*** Bug 142519 has been marked as a duplicate of this bug. ***

Comment 18

[Jouni Heikniemi]

*** Bug 157205 has been marked as a duplicate of this bug. ***

Comment 19

[Jacob Steenhagen]

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

[Guilliano Balandres]

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

[Dave Miller [:justdave]]

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

[Dave Miller [:justdave]]

comment #20 has been dealt with by using Perl's CGI module to handle the form
data...

Comment 23

[Dave Miller [:justdave]]

*** Bug 202423 has been marked as a duplicate of this bug. ***

Comment 24

[Vlad Dascalu]

*** Bug 232645 has been marked as a duplicate of this bug. ***

Comment 25

[Dave Miller [:justdave]]

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

[Dave Miller [:justdave]]

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

[Dave Miller [:justdave]]

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

[:glob ✱]

*** Bug 279901 has been marked as a duplicate of this bug. ***

Comment 29

[Marc Schumann [:Wurblzap]]

Attachment: 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

[Marc Schumann [:Wurblzap]]

Oh yeah, and flags don't work either.

Comment 31

[Frédéric Buclin]

Comment on attachment 218418 [details] [diff] [review]
Patch

Let's put it in my radar.

Comment 32

[Frédéric Buclin]

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

[Marc Schumann [:Wurblzap]]

Attachment: 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

[Frédéric Buclin]

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

[Marc Schumann [:Wurblzap]]

Ok, your idea is more straightforward and not as muddled as mine. I'll upload a new patch.

Comment 36

[Frédéric Buclin]

(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

[Frédéric Buclin]

Comment on attachment 221991 [details] [diff] [review]
Patch 1.2

Per my previous comments. I will wait for an updated patch.

Comment 38

[Marc Schumann [:Wurblzap]]

Attachment: Patch 1.3

Addressed comments.

Comment 39

[Frédéric Buclin]

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

[Dave Miller [:justdave]]

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

[Frédéric Buclin]

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

[Max Kanat-Alexander]

Added to relnotes as part of bug 349423.

Comment 43

[Frédéric Buclin]

Documentation updated as part of the doc patch from bug 174039.