989653 - send filelink attachment always triggers the attachment reminder

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[Magnus Melin [:mkmelin]]

Add a attachment using filelink, then try to send. The attachment reminder pops up.

This is a regression from tb24, and pretty annoying.

Comment 1

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v1

Possible fix.
The issue (according to me) was the file link notification in the attachmentReminderbox and the condition being checked was attachmentReminderbox.currentNotification[1].

[1] http://mxr.mozilla.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#2555

Thanks for cc'ing me. I now have a Box account :)

Comment 2

[Magnus Melin [:mkmelin]]

Comment on attachment 8398986 [details] [diff] [review]
Patch v1

Review of attachment 8398986 [details] [diff] [review]:
-----------------------------------------------------------------

Ah yes, there is a notification, but the wrong notification. r=mkmelin

Comment 3

[Suyash Agarwal (:sshagarwal)]

Do we need anything else? Or can I ask for check-in?

Comment 4

[Magnus Melin [:mkmelin]]

A test would be nice, though I won't insist on it.

Comment 5

[Suyash Agarwal (:sshagarwal)]

Sorry to ask, but what should the test verify?
If there is attachment reminder notification, show the prompt. If there is no notification or some other notification (and all other things are in favor (Remind Me later is unchecked and pref is set to true), show the notification.

Should I write a test verifying this --^ ?

Comment 6

[Magnus Melin [:mkmelin]]

Yes, and that it doesn't show when not needed. test-attachment-reminder.js might cover some of it already

Comment 7

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v2

Comment 8

[Magnus Melin [:mkmelin]]

Comment on attachment 8399119 [details] [diff] [review]
Patch v2

Review of attachment 8399119 [details] [diff] [review]:
-----------------------------------------------------------------

Great! r=mkmelin

::: mail/test/mozmill/composition/test-attachment-reminder.js
@@ +384,5 @@
>  }
>  
>  /**
> + * Bug 989653
> + * Send filelink attachment should not trigger the 

whitespace, and perhaps the whole sentence fits in one line?

@@ +408,5 @@
> +  let commonDialogAppears = true;
> +  try {
> +    // This should timeout and an error should be thrown.
> +    wait_for_modal_dialog("commonDialog");
> +  } catch (Ex) {

nit: usually e or ex (not with capital E)

Comment 9

[:aceman]

Comment on attachment 8399119 [details] [diff] [review]
Patch v2

Review of attachment 8399119 [details] [diff] [review]:
-----------------------------------------------------------------

Congrats to creating a new test!

The patch seems to work for me.

But the test does not fail if the patch to MsgComposeCommands.js is not applied so it does not test the specific problem of this bug. So that needs to be fixed.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +2552,5 @@
>      //    that the message has no attachment(s) yet, message still contains some attachment
>      //    keywords, and notification was not dismissed).
>      if (gManualAttachmentReminder || (getPref("mail.compose.attachment_reminder_aggressive") &&
> +         document.getElementById("attachmentNotificationBox")
> +                 .getNotificationWithValue("attachmentReminder"))) {

We also use .currentNotification in handleESC(). But in that case we probably want to close any notification that has a close button. So you just need to update the comment there that is handles not only attachment reminder bar.

::: mail/test/mozmill/composition/test-attachment-reminder.js
@@ +384,5 @@
>  }
>  
>  /**
> + * Bug 989653
> + * Send filelink attachment should not trigger the 

Kill the trailing space.

@@ +411,5 @@
> +    wait_for_modal_dialog("commonDialog");
> +  } catch (Ex) {
> +    commonDialogAppears = false;
> +  }
> +  assert_equals(commonDialogAppears, false);

This could probably be shortened to:
try {
  wait_for_modal_dialog("commonDialog");
  do_trow("A dialog appeared that was not expected");
} catch(e) { }

Comment 10

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v3

Okay, so I think we end it now.

Comment 11

[:aceman]

Comment on attachment 8400124 [details] [diff] [review]
Patch v3

Review of attachment 8400124 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/components/compose/content/MsgComposeCommands.js
@@ +1776,5 @@
>   */
>  function handleEsc()
>  {
>    // If there is an attachment reminder notification AND focus is in message body
>    // or on the notification, hide it.

I still see references to "attachment reminder" here...

::: mail/test/mozmill/composition/test-attachment-reminder.js
@@ +328,5 @@
> + * Bug 989653
> + * Send filelink attachment should not trigger the
> + * attachment reminder.
> + */
> +function test_attachment_reminder_prompt_at_send() {

This could use a better function name. Like test_attachment_vs_filelink_reminder .

@@ +336,5 @@
> +                  "There is no body. I hope you don't mind!");
> +  let nBox = cwc.e(kBoxId);
> +
> +  // There should be no notification yet.
> +  assert_automatic_reminder_state(cwc, false);

Can we check here if there is any notification, not just attachment reminder?

@@ +342,5 @@
> +  // Bring up the FileLink notification.
> +  let kOfferThreshold = "mail.compose.big_attachments.threshold_kb";
> +  let maxSize = Services.prefs.getIntPref(kOfferThreshold, 0) * 1024;
> +  add_attachment(cwc, "http://www.example.com/1", maxSize);
> +  assert_notification_displayed(cwc, kBoxId, "bigAttachment", true);

Add a "assert_automatic_reminder_state(cwc, false);" here too.

Comment 12

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v3.5

Made the changes for the suggestions.

Thanks.

Comment 13

[:aceman]

Comment on attachment 8400149 [details] [diff] [review]
Patch v3.5

Review of attachment 8400149 [details] [diff] [review]:
-----------------------------------------------------------------

>* * *
>another refrecen
>* * *
>another reference
What is this? :)

OK, this test seems to work now. It properly fails if I leave the bug in MsgComposeCommands.js in.

::: mail/test/mozmill/composition/test-attachment-reminder.js
@@ +323,5 @@
>  
>    close_compose_window(cwc);
>  }
>  
> +function assert_no_notification(cwc, aValue)

Add documentation what this function does, in the /** */ style.

@@ +327,5 @@
> +function assert_no_notification(cwc, aValue)
> +{
> +  let nBox = cwc.e(kBoxId);
> +  let notification = nBox.currentNotification;
> +  if ((notification && !aValue) || (!notification && aValue))

Would it work like "(notification == null) == aValue"?

@@ +328,5 @@
> +{
> +  let nBox = cwc.e(kBoxId);
> +  let notification = nBox.currentNotification;
> +  if ((notification && !aValue) || (!notification && aValue))
> +    throw new Error("notification exists");

"Attachment notification in wrong state".

@@ +339,5 @@
> + */
> +function test_attachment_vs_filelink_reminder() {
> +  // Open a blank message compose
> +  let cwc = open_compose_new_mail();
> +  setupComposeWin(cwc, "[e-mail]", "Testing Filelink notification",

Better use example.invalid .

@@ +349,5 @@
> +  // Bring up the FileLink notification.
> +  let kOfferThreshold = "mail.compose.big_attachments.threshold_kb";
> +  let maxSize = Services.prefs.getIntPref(kOfferThreshold, 0) * 1024;
> +  add_attachment(cwc, "http://www.example.com/1", maxSize);
> +  assert_notification_displayed(cwc, kBoxId, "bigAttachment", true);

Before this line, add comment that: the filelink attachment proposal should be up but not the attachment reminder and it should also not interfere with the sending of the message.

Comment 14

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v4

Are we ready to land this now? :)

Comment 15

[:aceman]

Comment on attachment 8400835 [details] [diff] [review]
Patch v4

Review of attachment 8400835 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/test/mozmill/composition/test-attachment-reminder.js
@@ +330,5 @@
> + * @param aController  Compose Window Controller
> + * @param aValue       True if notification should exist
> + *                     False otherwise.
> + */
> +function assert_no_notification(aCwc, aValue)

aController or aCwc?

@@ +333,5 @@
> + */
> +function assert_no_notification(aCwc, aValue)
> +{
> +  let nBox = aCwc.e(kBoxId);
> +  let notification = nBox.currentNotification;

You can merge these 2 lines.

Comment 16

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v4.1

Made the suggested changes. Thanks.

Re-requesting review as the patch is changed a bit.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/2a1878f04d4e