388830 - Use JS to make sure there's a Description value when submitting an attachment

Status: RESOLVED FIXED

(Bugzilla :: Attachments & Requests, enhancement, P2)

Description

[timeless]

I just tried to file a bug with an attachment, unfortunately, I got this:

The bug was created successfully, but attachment creation failed. Please add your attachment by clicking the "Create a New Attachment" link below.

oddly, for once in my life, JavaScript was enabled. Bugzilla should onsubmit check the upload field and if it has a value it should complain if there's no description of the attachment.

steps:
try to file a bug with an attachment and request review, but don't include a description.

--
no, i'm not planning to fix this.

Comment 1

[Frédéric Buclin]

(In reply to comment #0)
> no, i'm not planning to fix this.

heh

Comment 2

[Max Kanat-Alexander]

Wow, could that summary have been any more complex?

Comment 3

[timeless]

Attachment: plans change

Comment 4

[Frédéric Buclin]

Comment on attachment 275503 [details] [diff] [review]
plans change

>+function submitVerifier() {

>+    if (attachment_true &&
>+        attachment_true').style.display == 'block' &&

No idea what you mean. You clearly have a syntax problem.

Comment 7

[Frédéric Buclin]

timeless, are you still working on it? Otherwise pyrzak could fix this bug himself. If the patch is not invasive, we could take it for 3.2 too.

Comment 8

[David]

To fix this issue, you only need to change line 186 of post_bug.cgi from
my $attach_id = Bugzilla::Attachment->insert_attachment_for_bug(!THROW_ERROR,
to 
my $attach_id = Bugzilla::Attachment->insert_attachment_for_bug(THROW_ERROR,
to have the error thrown if a description is not entered.

That is how it is coded in Attachment.pm

Comment 9

[David]

Attachment: Throw error if there is no attachment description

supply patch

Comment 10

[Frédéric Buclin]

Comment on attachment 342307 [details] [diff] [review]
Throw error if there is no attachment description

No, this change is completely wrong. You don't want to throw an error once the attachment is uploaded.

Comment 11

[David]

The error is thrown before the attachment is uploaded.

Unless I am mistaken, the file is not uploaded until after the description is
validated in Bugzilla::Attachment::insert_attachment_for_bug() when
_validate_data() is called.

Using this patch has validate_description return 0 if there is no description.

Comment 12

[Frédéric Buclin]

too late for 3.4.

Comment 13

[Max Kanat-Alexander]

This was one of the issues that pyrzak's students' research pointed out. It's particularly annoying when you've uploaded a large attachment only to get this error that easily could have been caught with JS before submitting the attachment.

Comment 14

[Max Kanat-Alexander]

Attachment: v1

I know that this doesn't handle enter_bug--we have a whole other bug to handle JS form validation on enter_bug.

I also know that this doesn't handle the "File" field, which would have been more complicated to handle and is less of an issue because if there's no file it takes no time to upload it. :-) (However, if you really do want me to validate the File field also, I'll do it.)

Comment 15

[Guy Pyrzak]

which other bugs handle enter_bug (i'd like to have it documented on this bug).

Comment 16

[Guy Pyrzak]

Comment on attachment 375153 [details] [diff] [review]
v1

move the javascript into attachment.js please. I'd prefer for you to validate the file field. But I agree. I doubt users run into that problem often if at all. So if you don't want to bother with that still that's fine.

Comment 17

[Guy Pyrzak]

Comment on attachment 375153 [details] [diff] [review]
v1

also. You should trim the value before checking it for "" because it also doesn't check for spaces which it will also fail on the server side. YAHOO.lang.trim should be used first.

Comment 18

[Max Kanat-Alexander]

The enter_bug bug is bug 490767.

Comment 19

[Max Kanat-Alexander]

Attachment: v2

This also introduces a very simple method of localizing strings in external files.

Comment 20

[David Lawrence [:dkl]]

Comment on attachment 395133 [details] [diff] [review]
v2

>Index: js/attachment.js
>+function validateAttachmentForm(theform) {
>+    var desc_value = YAHOO.lang.trim(theform.description.value);
>+    if (desc_value == '') {
>+        alert(BUGZILLA.string.attach_desc_required);
>+        return false;
>+    }
>+    return true;
>+}

You could shorten this as:

>+function validateAttachmentForm(theform) {
>+    if (YAHOO.lang.trim(theform.description.value == '') {
>+        alert(BUGZILLA.string.attach_desc_required);
>+        return false;
>+    }
>+    return true;
>+}

but just a nit-pick. I have tested this and works as expected. I like the idea of the BUGZILLA JS object in the header.html.tmpl although I could imagine that getting big over time. But I cannot not think of a better way to embed template toolkit variables into javascript values other than putting them there. You could maybe create a new template template/en/default/global/js-variables.none.tmpl and keep them all in there. But that can come later.

r=dkl

Comment 21

[David Lawrence [:dkl]]

(In reply to comment #20)
> >+function validateAttachmentForm(theform) {
> >+    if (YAHOO.lang.trim(theform.description.value == '') {
> >+        alert(BUGZILLA.string.attach_desc_required);
> >+        return false;
> >+    }
> >+    return true;
> >+}

Should have been:

+function validateAttachmentForm(theform) {
+    if (YAHOO.lang.trim(theform.description.value) == '') {
+        alert(BUGZILLA.string.attach_desc_required);
+        return false;
+    }
+    return true;
+}

Comment 23

[Frédéric Buclin]

This is a new feature, not a bug.

Comment 24

[Frédéric Buclin]

Don't override blocking flags. We won't take this patch on branches.

Comment 25

[Max Kanat-Alexander]

Checking in js/attachment.js;
/cvsroot/mozilla/webtools/bugzilla/js/attachment.js,v  <--  attachment.js
new revision: 1.7; previous revision: 1.6
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.42; previous revision: 1.41
done
Checking in template/en/default/global/header.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/header.html.tmpl,v  <--  header.html.tmpl
new revision: 1.64; previous revision: 1.63
done

Comment 27

[Max Kanat-Alexander]

Added to the release notes in bug 547466.