Last Comment Bug 119703 - Create an attachment by pasting it into a text field
: Create an attachment by pasting it into a text field
Status: RESOLVED FIXED
[wanted-bmo][bmo4.0-post]
: selenium
Product: Bugzilla
Classification: Server Software
Component: Attachments & Requests (show other bugs)
: 2.10
: All All
: P3 enhancement with 2 votes (vote)
: Bugzilla 4.2
Assigned To: Frédéric Buclin
: default-qa
Mentors:
: 119704 232218 566069 606979 635034 (view as bug list)
Depends on:
Blocks: 367651 1018516 579749 584110 600516 608179 850608
  Show dependency treegraph
 
Reported: 2002-01-12 15:46 PST by Doug Turner (:dougt)
Modified: 2014-07-03 10:49 PDT (History)
19 users (show)
mkanat: approval+
LpSolit: testcase+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch, v1 (23.89 KB, patch)
2010-07-08 09:11 PDT, Frédéric Buclin
mkanat: review-
Details | Diff | Splinter Review
patch, v2 (28.71 KB, patch)
2010-07-15 14:53 PDT, Frédéric Buclin
mkanat: review+
Details | Diff | Splinter Review
solid (564 bytes, image/gif)
2014-07-03 09:41 PDT, Sam O'neal
no flags Details

Description Doug Turner (:dougt) 2002-01-12 15:46:40 PST
It would be nice if I could just paste a patch into a text field instead of
having to create a temp file.  Any chance of doing something like this??
Comment 1 Bradley Baetz (:bbaetz) 2002-01-12 15:49:28 PST
*** Bug 119704 has been marked as a duplicate of this bug. ***
Comment 2 Matthew Tuck [:CodeMachine] 2002-01-14 03:17:17 PST
Anything's possible.

Why do you have to create temp files?  Wouldn't the patch usually start as a
file?

On the rare occasion I have had to create a temp file (through telnet), they
never seem to work anyway.
Comment 3 Doug Turner (:dougt) 2002-01-14 09:48:31 PST
Well, here is what I do....

a) modify code.
b) cvs diff -u > xx
c) click on Create a New Attachment
d) find the file in the File Picker

I was just thinking that c/d could be replaced with a copy and paste.
Comment 4 Dave Miller [:justdave] (justdave@bugzilla.org) 2003-01-21 23:55:53 PST
Reassigning all of my "future" targetted bugs to indicate that I'm not presently
working on them, and someone else could feel free to work on them.
Comment 5 Dave Miller [:justdave] (justdave@bugzilla.org) 2003-01-21 23:59:14 PST
Reassigning all of my "future" targetted bugs to indicate that I'm not presently
working on them, and someone else could feel free to work on them. (sorry for
the spam if you got this twice, it didn't take right the first time)
Comment 6 Frédéric Buclin 2007-10-03 10:33:52 PDT
*** Bug 232218 has been marked as a duplicate of this bug. ***
Comment 7 Nicholas Sushkin 2010-02-11 08:03:28 PST
Instead of going through multiple steps starting with pasting clipboard into a new temporary file, I should be able to paste clipboard contents into a text/plain attachment directly through a form.
Comment 8 Denis Roy 2010-03-03 12:35:33 PST
This functionality has been requested at Eclipse.  See http://bugs.eclipse.org/304229
Comment 9 Paul Fullbright 2010-03-08 07:21:40 PST
To follow up on the Eclipse request, another major use case is for users to attach a stack trace in a simple way.  As it is now, users often paste large multithreaded stack traces directly into a bug description, which impairs the communication involved.
Comment 10 Frédéric Buclin 2010-05-14 17:19:19 PDT
*** Bug 566069 has been marked as a duplicate of this bug. ***
Comment 11 Frédéric Buclin 2010-07-08 05:45:20 PDT
Should be pretty trivial to do now that I refactored Attachment.pm.
Comment 12 Frédéric Buclin 2010-07-08 09:11:46 PDT
Created attachment 456452 [details] [diff] [review]
patch, v1

Was indeed pretty easy to implement. My patch from bug 490930 must land first (it already has r+ and a+) due to conflicts.

In a separate bug, I plan to let attachment.cgi extract URLs from plain/text files, and display them above the attachment itself, so that if you paste a URL, you still have an easy way to click on it (to reproduce the old behavior).

Note that when you paste text directly, the MIME type is automatically set to text/plain. I may relax that in a separate bug. Same for the is_patch attribute.

mkanat, could you look at pieces which are outside my ownership, especially in WS and Install/?
Comment 13 Max Kanat-Alexander 2010-07-13 14:38:48 PDT
Comment on attachment 456452 [details] [diff] [review]
patch, v1

  Hmm, I don't fully understand why you're removing isurl as part of this patch? I'm perfectly happy to see that feature go away, but it doesn't seem like this new patch actually fully duplicates that functionality?

  In any case:

>=== modified file 'Bugzilla/Field.pm'
>--- Bugzilla/Field.pm	2010-07-05 23:40:12 +0000
>+++ Bugzilla/Field.pm	2010-07-08 14:46:46 +0000
>@@ -238,7 +238,6 @@
>      buglist => 1},
>     {name => 'content',               desc => 'Content'},
>     {name => 'attach_data.thedata',   desc => 'Attachment data'},
>-    {name => 'attachments.isurl',     desc => 'Attachment is a URL'},

  You also need to add code somewhere to delete the field from fielddefs. This field never could have been updated, right? So it will never be in bugs_activity.

>=== modified file 'Bugzilla/WebService/Bug.pm'
> C<boolean> True if the attachment is obsolete, False otherwise.
> 
>-=item C<is_url>
>-
>-C<boolean> True if the attachment is a URL instead of actual data,

  You'll also need to update History to note that this field went away in Bugzilla 4.2.

  Also, I just added add_attachment to WebServices (it's pending checkin right now) so that will have to be updated as well. It will also need a History section, because it is going in to 4.0. (And stuff will also have to be removed from Errors and WS_ERROR_CODES or whatever it's called.)

>=== modified file 'template/en/default/attachment/createformcontents.html.tmpl'
>-  <th><label for="attachurl">AttachURL</label>:</th>
>+  <th><label for="attach_text">&nbsp;</label></th>

  There's no reason to have an empty <label>.

>+    <em>Or paste the text to be added as an attachment (instead of attaching a file).</em><br>
>+    <textarea id="attach_text" name="attach_text" cols="60" rows="10"
>+              onkeyup="URLFieldHandler()" onblur="URLFieldHandler()"></textarea>

  I think that it would be a really good idea to hide this field by default, and only show it if somebody clicks on a link. (And then you could hide the data field when the attach_text field was shown.)
Comment 14 Guy Pyrzak 2010-07-13 21:40:21 PDT
I think this is a great idea and probably one I'd use a lot personally. Some smarts might be if the only text pasted in matches the URL Regex then it assumes it is a URL? or does that have security implications, or maybe it's just dumb period...

I do agree with Mkanat that it would be nice to hide the big text box behind a hyperlink of sorts but not 100% necessary. Using TUI might be a good idea here as well. I find that the fewer form widgets in view the less complicated/scary the page appears. Especially when those fields are redundant and don't work in tandem.
Comment 15 Frédéric Buclin 2010-07-15 11:12:04 PDT
(In reply to comment #13)
>   Hmm, I don't fully understand why you're removing isurl as part of this
> patch? I'm perfectly happy to see that feature go away, but it doesn't seem
> like this new patch actually fully duplicates that functionality?

As pyrzak said, I'm going to add a regexp (in a separate bug), and if it matches, then Bugzilla will linkify the URL, restoring the behavior of isurl.
Comment 16 Max Kanat-Alexander 2010-07-15 14:23:49 PDT
  Okay. That sounds fine. :-) Or actually, why not just run the text through quoteUrls?
Comment 17 Frédéric Buclin 2010-07-15 14:26:36 PDT
Because we display the file as is (coming from the alternate attachment server), not an altered one, in Details.
Comment 18 Frédéric Buclin 2010-07-15 14:53:21 PDT
Created attachment 457682 [details] [diff] [review]
patch, v2

I fixed everything you commented on.
Comment 19 Max Kanat-Alexander 2010-07-15 15:27:20 PDT
(In reply to comment #17)
> Because we display the file as is (coming from the alternate attachment
> server), not an altered one, in Details.

  Ah, okay. Well, maybe there could be a &quoteUrls=1 option or something, but that would be another bug for sure.
Comment 20 Max Kanat-Alexander 2010-07-15 19:21:43 PDT
Comment on attachment 457682 [details] [diff] [review]
patch, v2

This looks great, and is really cool! :-) On checkin, I'd like it if you could change the "text_field" class to something more specific, like "attach_text", because TUI classes are global across all of Bugzilla.

Also, it might be nice to take the "or paste text as attachment" text and make it non-italic, so that it's more noticeable. But that's something we can fix later or pyrzak can make some decision about if he wants to.
Comment 21 Frédéric Buclin 2010-07-18 10:18:43 PDT
Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/trunk/
modified attachment.cgi
modified bugzilla.dtd
modified post_bug.cgi
modified Bugzilla/Attachment.pm
modified Bugzilla/Field.pm
modified Bugzilla/Config/Attachment.pm
modified Bugzilla/DB/Schema.pm
modified Bugzilla/Install/DB.pm
modified Bugzilla/WebService/Bug.pm
modified Bugzilla/WebService/Constants.pm
modified docs/en/xml/using.xml
modified js/attachment.js
modified template/en/default/admin/params/attachment.html.tmpl
modified template/en/default/attachment/create.html.tmpl
modified template/en/default/attachment/createformcontents.html.tmpl
modified template/en/default/attachment/edit.html.tmpl
modified template/en/default/attachment/list.html.tmpl
modified template/en/default/bug/show.xml.tmpl
modified template/en/default/bug/create/create.html.tmpl
modified template/en/default/global/code-error.html.tmpl
modified template/en/default/global/field-descs.none.tmpl
modified template/en/default/global/user-error.html.tmpl
modified xt/lib/Bugzilla/Test/Search/Constants.pm
Committed revision 7383.
Comment 22 Frédéric Buclin 2010-10-25 08:45:54 PDT
*** Bug 606979 has been marked as a duplicate of this bug. ***
Comment 23 Frédéric Buclin 2011-02-17 14:03:55 PST
*** Bug 635034 has been marked as a duplicate of this bug. ***
Comment 24 David Lawrence [:dkl] 2011-02-17 15:16:28 PST
jhammel. I am going to work on backporting this to bmo/4.0 so we can have it sooner than 4.2.

Dave
Comment 25 Max Kanat-Alexander 2011-02-17 15:17:26 PST
(In reply to comment #24)
> jhammel. I am going to work on backporting this to bmo/4.0 so we can have it
> sooner than 4.2.

  For what it's worth, I wouldn't recommend backporting much of anything from 4.2 at this stage--it has never even had a development release and seems to be rather unstable.
Comment 26 Frédéric Buclin 2011-02-17 15:23:49 PST
(In reply to comment #25)
>   For what it's worth, I wouldn't recommend backporting much of anything from
> 4.2 at this stage--it has never even had a development release and seems to be
> rather unstable.

I agree with Max here. This is a major change to the code which probably depends on some other changes. A good way to break everything. I wouldn't recommend to backport this.
Comment 27 David Lawrence [:dkl] 2011-02-22 14:29:38 PST
So even if I only pull out the part that displays a textarea for pasting text and using that instead of any uploaded data or url, that will be dependent on various other changes and not be worth the trouble?
Comment 28 David Lawrence [:dkl] 2011-03-22 12:42:12 PDT
Will look into this in more detail once 4.0 is in production.
Comment 29 Max Kanat-Alexander 2011-03-22 12:45:32 PDT
(In reply to comment #27)
> So even if I only pull out the part that displays a textarea for pasting text
> and using that instead of any uploaded data or url, that will be dependent on
> various other changes and not be worth the trouble?

  Right. There is no "only the part that displays a textarea for pasting text", there's a lot of prerequisite work and a huge patch here that should not be backported.
Comment 30 Byron Jones ‹:glob› 2011-09-14 14:32:03 PDT
*** Bug 686772 has been marked as a duplicate of this bug. ***
Comment 31 Frédéric Buclin 2011-12-26 07:50:19 PST
Added to relnotes in bug 713346.
Comment 32 Ed Morley [:emorley] 2013-03-13 04:53:46 PDT
Frederic, for the rest of bugs on b.m.o regressions are usually marked as blocking the originating bug, not vice versa. Is this not the case with the Bugzilla product?
Comment 33 Frédéric Buclin 2013-03-13 04:57:02 PDT
(In reply to Ed Morley (Away until 13th March) [:edmorley UTC+0] from comment #32)
> Frederic, for the rest of bugs on b.m.o regressions are usually marked as
> blocking the originating bug, not vice versa. Is this not the case with the
> Bugzilla product?

Correct. For us, a regression depends on the original implementation.
Comment 34 Sam O'neal 2014-07-03 09:41:36 PDT Comment hidden (spam)
Comment 35 Sam O'neal 2014-07-03 09:46:34 PDT Comment hidden (spam)
Comment 36 Frédéric Buclin 2014-07-03 09:48:26 PDT
Sam: please stop playing in this bug!! This is not the right place for it.
Comment 37 Sam O'neal 2014-07-03 09:48:54 PDT Comment hidden (spam)

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