98602 - Templatize createattachment.cgi

Status: RESOLVED FIXED

(Bugzilla :: Attachments & Requests, defect)

Description

[Myk Melez [:myk] [@mykmelez]]

createattachment.cgi should be templatized as part of the overall effort to
templatize the entire application, the tracking bug for which is bug 86168.

Probably the best way to do this is to incorporate attachment creation
capabilities into attachment.cgi.

When doing this work, keep in mind the features requested in bugs 87420, 87770,
93749, 97768, and possibly 98096.
bug 87420 - Pressing back on "create attachment".
bug 87770 - createattachment should work with no parameters.
bug 93749 - Bugzilla tells me to `Create an Attachment' when I already did
bug 97768 - Create Attachment: MIME types for Microsoft Word, Excel, Powerpoint,
etc.
bug 98096 - Creating an attachment should give more information.

Comment 1

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v1: first cut (still has issues)

Comment 2

[Myk Melez [:myk] [@mykmelez]]

The patch I just attached extends attachment.cgi with templates and logic for
creating attachments.  It supports content (MIME) type autosensing (bug 86397),
tells you the right thing when you have created an attachment (bug 93749),
allows installations to customize MIME types (bug 97768), allows you to comment
on the bug when creating the attachment (bug 98601), and also allows you to
obsolete old attachments when creating a new one (bug number?).

Issues include:

1. It uses TT's "wrap" plugin to wrap comments, and that plugin also wraps long
URLs.
2. It doesn't take mid-air collisions into account.
3. It may not handle empty form field values (if it doesn't, then neither does
the current version).
4. May or may not address bug 87420.
5. Does not address bug 87770 or bug 98096.

Comment 3

[Jacob Steenhagen]

>obsolete old attachments when creating a new one (bug number?).

Bug 98111

Comment 4

[Myk Melez [:myk] [@mykmelez]]

Thanks Jake.  Adding dependency from bug 98111, targeting to 2.16, and accepting.

Comment 5

[Matthew Tuck [:CodeMachine]]

Where does it use wrap, and what for?

Also, why do we need content types if it has autosense?  The point of autosense
was to remove the need for content types.

Comment 6

[Myk Melez [:myk] [@mykmelez]]

> Where does it use wrap, and what for?

It uses wrap to wrap the comments field on the server side.  I could have made
the comments field wrap on the browser side (like the comments field in
show_bug.cgi), but that behavior is a bug according to bug 11901, so I thought I
should fix the problem in any new code.

> Also, why do we need content types if it has autosense?  The point of 
> autosense was to remove the need for content types.

There seems to be mixed support for autosensing among browsers, (f.e. Mozilla in
bug 54940), so I designed the new screen to allow the user to specify a content
type if they know their browser won't assign one correctly.  The default is
"autosense".

Comment 7

[Jacob Steenhagen]

I agree 100% with having the ability to specify the MIME type on the upload
screen.  When I create a diff, I name it <bug#>.dif... I know if I use IE to
upload it, it will report its MIME type as some strange Excel file.  So for that
reason alone, I'd love to have the ability to say "This is text/plain" on the
upload screen.

Comment 8

[Myk Melez [:myk] [@mykmelez]]

> I agree 100% with having the ability to specify the MIME type on the upload
> screen.  When I create a diff, I name it <bug#>.dif... I know if I use IE to
> upload it, it will report its MIME type as some strange Excel file.  So for that
> reason alone, I'd love to have the ability to say "This is text/plain" on the
> upload screen.

Although technically this can be accomplished without giving the user the
ability to specify the type, since the "patch" checkbox overrides the "content
type" fields and sets the type to "text/plain", the essential point is correct:
users may sometimes have a reason to request a specific type, and it is useful
to enable them to do so.

Perhaps after we implement this feature if it turns out that autosense works 99%
of the time we can go back and remove this feature and make people use the "edit
attachment" screen to modify the type the last 1% of the time.  At this point I
think an intermediate step between the current situation (all manual entry) and
the ideal situation (all autosensing) is best.

Comment 9

[Matthew Tuck [:CodeMachine]]

> It uses wrap to wrap the comments field on the server side.  I could
> have made the comments field wrap on the browser side (like the comments
> field in show_bug.cgi), but that behavior is a bug according to bug 11901,
> so I thought I should fix the problem in any new code.

I guess you all know by now I want to see display-time wrapping, and I haven't
seen any statement as to why we shouldn't do it that way.  As such, I'd rather
not wrap any hard wrap any comments since once we do we can't fix them.  At the
moment, at least comments will display-wrap properly if we fix Bugzilla to
display-wrap.

Comment 10

[Myk Melez [:myk] [@mykmelez]]

>At the
>moment, at least comments will display-wrap properly if we fix Bugzilla to
>display-wrap.

Not true.  Comments in Bugzilla are hard-wrapped by the browser and stored in
the database with the hard wraps intact.  Switching to client-side soft wrapping
only works for future comments, which makes the switch a much more complicated
affair.

Comment 11

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v2: addresses URL wrapping issue

Comment 12

[Myk Melez [:myk] [@mykmelez]]

>1. It uses TT's "wrap" plugin to wrap comments, and that plugin also wraps long
>URLs.

I fixed this by requiring a newer version of Text::Wrap.

>2. It doesn't take mid-air collisions into account.

This should be resolved in bug 99215.

>3. It may not handle empty form field values (if it doesn't, then neither does
>the current version).

I think this is irrelevant if it hasn't come up before.

>4. May or may not address bug 87420.

It does, the two screens are at different URLs.

>5. Does not address bug 87770 or bug 98096.

It doesn't.  Removing them from the dependency list.

So, ready to review.

Comment 13

[Matthew Tuck [:CodeMachine]]

>> At the moment, at least comments will display-wrap properly if we fix
>> Bugzilla to display-wrap.
> Not true.

I was referring specifically to attachment descriptions on attachment creations,
which aren't currently wrapped at all.

Comment 14

[Jacob Steenhagen]

Comment on attachment 51376 [details] [diff] [review]
patch v2: addresses URL wrapping issue

>Index: attachment.cgi
>===================================================================
>RCS file: /cvsroot/mozilla/webtools/bugzilla/attachment.cgi,v
>retrieving revision 1.2
>diff -u -r1.2 attachment.cgi
>--- attachment.cgi	2001/08/31 21:40:33	1.2
>+++ attachment.cgi	2001/09/29 01:45:30
...
>@@ -276,23 +404,129 @@
...
>+  # Insert a comment about the new attachment into the database.
>+  my $comment = "Created an attachment (id=$attachid): $::FORM{'description'}\n\n";
>+  if ( $::FORM{'comment'} ) {
>+    use Text::Wrap;
>+    $Text::Wrap::columns = 80;
>+    $Text::Wrap::huge = 'overflow';
>+    $comment .= Text::Wrap::wrap('', '', $::FORM{'comment'});
>+  }
>+  AppendComment($::FORM{'bugid'}, 
>+                $::COOKIE{"Bugzilla_login"},
>+                $comment);
...
This will cause long descriptions of the attachment to not be properly wraped.
...
>+  # Send mail to let people know the attachment has been created.  Uses a 
>+  # special syntax of the "open" and "exec" commands to capture the output of 
>+  # "processmail", which "system" doesn't allow, without running the command 
>+  # through a shell, which backticks (``) do.
>+  #system ("./processmail", $bugid , $::userid);
>+  #my $mailresults = `./processmail $bugid $::userid`;
>+  my $mailresults = '';
>+  open(PMAIL, "-|") or exec('./processmail', $::FORM{'bugid'}, $::COOKIE{'Bugzilla_login'});
>+  $mailresults .= $_ while <PMAIL>;
>+  close(PMAIL);
...
I can't say that I'm really fond of this different mail layout... maybe it's
just because I'm used to it, but I like the output given when creating/changing
bugs better than the "new" one given when updating attachments.
...
>+  # !!! Create TT filter and move value_quote logic into template!
>+  $vars->{'description'} = value_quote($description);
...
Doesn't the FILTER method used for quoting <>& also do "?  (I thought I saw
that message on the TT mailing list).

Comment 15

[Myk Melez [:myk] [@mykmelez]]

>This will cause long descriptions of the attachment to not be properly wraped.

Why not?

>I can't say that I'm really fond of this different mail layout... maybe it's
>just because I'm used to it, but I like the output given when creating/changing
>bugs better than the "new" one given when updating attachments.

The update attachment code calls processmail just like the update bug code, so
the mail layout should be the same.  How is it different?

>Doesn't the FILTER method used for quoting <>& also do "?  (I thought I saw
>that message on the TT mailing list).


Yep, it was in a response to my message. :-)  I guess I just forgot (or I did
this attachment before I found that out).

Comment 16

[Jacob Steenhagen]

>>This will cause long descriptions of the attachment to not be properly wraped.
>
>Why not?

If I read all the code right, the description is pulled into $comment before the
wraping takes effect.

>The update attachment code calls processmail just like the update bug code, so
>the mail layout should be the same.  How is it different?

It's not the mail that's different, it's the changed notice and the list of
e-mail addresses on the web page.

Comment 17

[Myk Melez [:myk] [@mykmelez]]

>If I read all the code right, the description is pulled into $comment before
>the wraping takes effect.

Right, which means the description will be wrapped.  Is that improper?

>It's not the mail that's different, it's the changed notice and the list of
>e-mail addresses on the web page.

Oh, I understand what you mean now.  This problem is addressed in my patches for
bug 98801.

Comment 18

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v3: resolves wrapping issue

Comment 19

[Myk Melez [:myk] [@mykmelez]]

Ok, so I finally figured out what Jake was talking about via a conversation with
him in the IRC channel, and he's right, patch v2 didn't wrap the description. 
Patch v3 fixes that.

Re: the way the "created" notification displays, the patches on this bug use the
old method (the one everyone likes) when an attachment gets created.  The new
method (used when an attachment is updated, which everyone hates) is not a part
of these patches, and it won't be a part of attachment updating after bug 98801
gets fixed.

Ready for re-review.

Comment 20

[Jacob Steenhagen]

Comment on attachment 52328 [details] [diff] [review]
patch v3: resolves wrapping issue

The Create New Attachment link still links to createattachment.cgi instead of
attachment.cgi?bugid=1&action=enter.  Also, when I entered that URL by hand I got
an error:
Template process failed: file error - attachment/enter.atml: not found

enter.atml does exist:
[jake@c539723-a attachment]$ pwd
/var/www/bugzilla/template/default/attachment
[jake@c539723-a attachment]$ ll
total 24
drwxr-xr-x    2 jake     apache       4096 Oct  5 20:42 CVS
-rwxr-xr-x    1 jake     apache       7121 Oct  6 12:20 edit.atml
-rwxr-xr-x    1 jake     apache       1641 Aug 30 23:54 list.atml
-rwxr-xr-x    1 jake     apache        353 Aug 30 23:54 updated.atml
-rwxr-xr-x    1 jake     apache       2106 Aug 30 23:54 viewall.atml

Comment 21

[Jacob Steenhagen]

OK, I take that back, enter.atml didn't exist... :)  Silly me. patch doesn't
seem work very well when it spans multiple directories. It still needs that link
change, though...

Comment 22

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v4: createattachment -> attachment

Comment 23

[Myk Melez [:myk] [@mykmelez]]

The latest patch does these things:

1. updates the link to createattachment.cgi to point it to the new version of
the "create attachment" screen located at attachment.cgi?bugid=###&action=enter;

2. changes the name of the contenttypes.atml file to contenttypes to conform
with the standard for naming template fragments versus complete templates;

3. adds a message to the "created" screen telling the user what content type was
auto-detected if the user chose content type auto-detection.

Comment 24

[Jacob Steenhagen]

Comment on attachment 52536 [details] [diff] [review]
patch v4: createattachment -> attachment

Maybe I'm just blind, but I don't see edit.atml in this patch... (but I know there
are changes requried in it... at the very least 'mimetype' -> 'contenttype').

Also, this fails the test (sh runtests.sh):
main::validateFilename() called too early to check prototype at attachment.cgi
        line 100 (#1)

    (W prototype) You've called a function that has a prototype before the parser saw a
    definition or declaration for it, and Perl could not check that the call
    conforms to the prototype.  You need to either add an early prototype
    declaration for the subroutine in question, or move the subroutine
    definition ahead of the call to get proper prototype checking.  Alternatively,
    if you are certain that you're calling the function correctly, you may put
    an ampersand before the name to avoid the warning.  See perlsub.

attachment.cgi syntax OK
#     Failed test (t/001compile.t at line 70)
not ok 2 - attachment.cgi--WARNING

Also (again), when I made a really long comment while attaching a bug, it put my
comment in the Additional comments field twice... once wrapped and once not.

Comment 25

[Zach Lipton [:zach]]

please run tests before posting patches

Comment 26

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v5: fixes known issues (apply with "-p0" option to patch)

Comment 27

[Myk Melez [:myk] [@mykmelez]]

The latest patch:

1. Passes tests.
2. Wraps comments right!
3. Reorganizes "content type" entry field.
4. Includes changes to enter.atml.
5. Applies cleanly with the "-p0" option to patch.

Ready for review!

Comment 28

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v6: misc cleanup (apply with "-p0" option to patch)

Comment 29

[Myk Melez [:myk] [@mykmelez]]

The latest patch fixes up a few inconsistencies in the use of the words
"mimetype" and "contenttype" as variable names.  It also replaces a few
instances where something is being value_quoted for use in HTML templates with
the "html" filter.

Comment 30

[Jacob Steenhagen]

Comment on attachment 53295 [details] [diff] [review]
patch v6: misc cleanup (apply with "-p0" option to patch)

Looks good to me...
r=jake

Comment 31

[Gervase Markham [:gerv]]

Comment on attachment 53295 [details] [diff] [review]
patch v6: misc cleanup (apply with "-p0" option to patch)

+    $bugid == $::FORM{'bugid'}
+      || DisplayError("Attachment #$attachid ($description) is attached to 
+           bug #$bugid, but you flagged it for obsoletion when creating 
+           a new attachment to bug #$::FORM{'bugid'}.")
+        && exit;
+
+    if ( $isobsolete )
+    {
+      $description = html_quote($description);
+      DisplayError("Attachment #$attachid ($description) is already obsolete.");
+      exit;
+    }

You fail to html_quote $description for the first error.

A whole load of code for entering patches seems to have appeared in attachment.cgi, but not been removed from anywhere. Why is this?

Gerv

Comment 32

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v7: escapes description and fixes addl. bug

Comment 33

[Myk Melez [:myk] [@mykmelez]]

> You fail to html_quote $description for the first error.

Fixed in the latest patch.

> A whole load of code for entering patches seems to have appeared in
> attachment.cgi, but not been removed from anywhere. Why is this?

Because the new code only gets used if you have the attachment tracker turned
on; the old code still gets used on installations that do not use the attachment
tracker.

In addition to fixing the bug Gerv mentions, I found another bug that prevented
users who were not logged in from creating attachments.  That bug has also been
fixed in the latest patch.

Jake, Gerv, re-review?

Comment 34

[Gervase Markham [:gerv]]

Comment on attachment 53379 [details] [diff] [review]
patch v7: escapes description and fixes addl. bug

r=gerv.

Gerv

Comment 35

[Jacob Steenhagen]

Comment on attachment 53379 [details] [diff] [review]
patch v7: escapes description and fixes addl. bug

OK, this looks good to me...

Comment 36

[Myk Melez [:myk] [@mykmelez]]

RCS file:
/cvsroot/mozilla/webtools/bugzilla/template/default/attachment/contenttypes,v
done
Checking in template/default/attachment/contenttypes;
/cvsroot/mozilla/webtools/bugzilla/template/default/attachment/contenttypes,v 
<--  contenttypes
initial revision: 1.1
done
RCS file:
/cvsroot/mozilla/webtools/bugzilla/template/default/attachment/created.atml,v
done
Checking in template/default/attachment/created.atml;
/cvsroot/mozilla/webtools/bugzilla/template/default/attachment/created.atml,v 
<--  created.atml
initial revision: 1.1
done
Checking in template/default/attachment/edit.atml;
/cvsroot/mozilla/webtools/bugzilla/template/default/attachment/edit.atml,v  <--
 edit.atml
new revision: 1.4; previous revision: 1.3
done
RCS file:
/cvsroot/mozilla/webtools/bugzilla/template/default/attachment/enter.atml,v
done
Checking in template/default/attachment/enter.atml;
/cvsroot/mozilla/webtools/bugzilla/template/default/attachment/enter.atml,v  <--
 enter.atml
initial revision: 1.1
done
Checking in template/default/attachment/list.atml;
/cvsroot/mozilla/webtools/bugzilla/template/default/attachment/list.atml,v  <--
 list.atml
new revision: 1.2; previous revision: 1.1
done
Checking in template/default/attachment/viewall.atml;
/cvsroot/mozilla/webtools/bugzilla/template/default/attachment/viewall.atml,v 
<--  viewall.atml
new revision: 1.2; previous revision: 1.1
done