772392 - Starred message shouldn't be deleted without alert/prompt

Status: ASSIGNED

(Thunderbird :: Folder and Message Lists, enhancement)

Description

[Vincent (caméléon)]

User Agent: Mozilla/5.0 (Windows NT 5.1; rv:8.0.1) Gecko/20100101 Firefox/8.0.1
Build ID: 20111120135848

Steps to reproduce:

User case from: https://getsatisfaction.com/mozilla_messaging/topics/can_you_write_protect_important_emails

1 - Starred an important message
2 - Select some messages that you want to delete, including a starred message
3 - Click "Delete"


Actual results:

All messages are deleted without any prompt


Expected results:

If a starred message is selected for deletion, there should be a prompt asking the user to confirm the action

Comment 1

[Wayne Mery (:wsmwk)]

workaround https://addons.mozilla.org/en-US/thunderbird/addon/prevent-delete/ (requires tagging)

Comment 2

[:aceman]

Attachment: 772392.patch

Hi, may this be what is requested in this bug?
Some UX decisions needed for the comments in the patch.

Comment 3

[Alessandro Castellani [:aleca]]

Comment on attachment 9113057 [details] [diff] [review]
772392.patch

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

::: mail/base/content/mail3PaneWindowCommands.js
@@ +758,5 @@
> +             let bundle = new StringBundle(
> +               "chrome://messenger/locale/messenger.properties"
> +             );
> +        // TODO: do we want confirmEx here? confirm shows OK/Cancel buttons. Do we want Yes/No?
> +        // Also, do we want to allow the user to tick "don't ask me again" for those who do not want this "protection"?

Yes, I think we should tackle it in this patch.

::: mail/locales/en-US/chrome/messenger/messenger.properties
@@ +673,5 @@
>  confirmMsgDelete.deleteFromTrash.desc=This will permanently delete messages from Trash. Are you sure you want to continue?
>  confirmMsgDelete.dontAsk.label=Don't ask me again.
>  confirmMsgDelete.delete.label=Delete
>  
> +confirmDeleteFlagged.title=Deletion of flagged messages

I think we should keep it somewhat consistent with the other confirmation messages, so this could be:
"Confirm Deletion of Starred Messages".
Or maybe we could also consider reusing directly the confirmMsgDelete.title since a proper explanation is in the text.

@@ +674,5 @@
>  confirmMsgDelete.dontAsk.label=Don't ask me again.
>  confirmMsgDelete.delete.label=Delete
>  
> +confirmDeleteFlagged.title=Deletion of flagged messages
> +confirmDeleteFlagged.text=Some of the messages you are going to delete are starred. Are you sure to remove those?

Mh, this sounds a bit weird.
Maybe something like: "Some of the messages you selected for deletion are starred. Are you sure you want to continue?"

Comment 4

[:aceman]

Attachment: 772392.patch v2

Thanks, fixed the issues.

Comment 5

[Alessandro Castellani [:aleca]]

Comment on attachment 9113987 [details] [diff] [review]
772392.patch v2

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

This looks good to me, great work!

Comment 6

[Vincent (caméléon)]

Comment on attachment 9113987 [details] [diff] [review]
772392.patch v2

I am sorry, but I cannot understand the code, so my advice is not required to review it

Comment 7

[:aceman]

Attachment: 772392.patch v3

The same UI, but I implemented the check in a different place where other similar checks reside.

Incidentally it affected a test so it was easy to add a test for this feature.
Try run:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=2b257da2acd59e347e786d8a474467235fdbbd4c

Comment 8

[Magnus Melin [:mkmelin] (away, back June 30)]

Comment on attachment 9118985 [details] [diff] [review]
772392.patch v3

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

::: mail/test/browser/folder-display/browser_messageCommandsOnMsgstore.js
@@ +205,5 @@
> +      .getButton("accept")
> +      .doCommand();
> +  });
> +  press_delete(mc);
> +  wait_for_modal_dialog("commonDialogWindow");

can you add a check for both cases, first put up the dialog bug press cancel, then put it up and press accept

Please after deletion check the current message is as expected. Other tests would of course start failing then, but it would be harder to understand why.

Mochitest also likes that you check stuff (Assert.ok etc.), whereas mozmill just checked everything kept going

::: mailnews/base/src/nsMsgDBView.cpp
@@ +3033,5 @@
>    uint32_t numMsgs;
>    messageArray->GetLength(&numMsgs);
>  
>    const char *warnCollapsedPref = "mail.warn_on_collapsed_thread_operation";
> +  const char *warnFlaggedDelPref = "mail.warn_on_flagged_delete";

The wording seems backwards. 

mail.warn_on_delete_flagged

@@ +3065,5 @@
>        warningName.AssignLiteral("confirmMsgDelete.collapsed.desc");
>      }
>    }
>  
> +  if (!activePref) {

This seems wrong. Doesn't it clobber other prefs, like confirmMsgDelete.deleteNoTrash.desc and news later?

Looks like the thing with activePref should be in a helper function and not like it currently is

@@ +3076,5 @@
> +      while (NS_SUCCEEDED(enumerator->HasMoreElements(&hasMore)) && hasMore) {
> +        nsCOMPtr<nsISupports> supports;
> +        rv = enumerator->GetNext(getter_AddRefs(supports));
> +        nsCOMPtr<nsIMsgDBHdr> msgHdr;
> +        msgHdr = do_QueryInterface(supports, &rv);

put these two on the same line
also missing rv check

Comment 9

[:aceman]

(In reply to Magnus Melin [:mkmelin] from comment #8)

can you add a check for both cases, first put up the dialog bug press
cancel, then put it up and press accept

OK.

Please after deletion check the current message is as expected. Other tests
would of course start failing then, but it would be harder to understand why.

Mochitest also likes that you check stuff (Assert.ok etc.), whereas mozmill
just checked everything kept going

OK.

::: mailnews/base/src/nsMsgDBView.cpp
The wording seems backwards.
mail.warn_on_delete_flagged

OK.

@@ +3065,5 @@

• if (!activePref) {

This seems wrong. Doesn't it clobber other prefs, like
confirmMsgDelete.deleteNoTrash.desc and news later?

Looks like the thing with activePref should be in a helper function and not
like it currently is

Yes, this is strange, because the whole function works in the way that only the first check that passes wins. It sets activePref and then no other
checks and warnings are done.

So in that way if you try to delete a msg from Trash, you get the trash warning, bot no longer the "deleting starred message" or any warning about news.

I think this could be rewritten to show all warnings that actually apply to the selected messages, but I would propose a new bug.
Do you agree?

@@ +3076,5 @@

•  while (NS_SUCCEEDED(enumerator->HasMoreElements(&hasMore)) && hasMore) {
  

•    nsCOMPtr<nsISupports> supports;
  

•    rv = enumerator->GetNext(getter_AddRefs(supports));
  

•    nsCOMPtr<nsIMsgDBHdr> msgHdr;
  

•    msgHdr = do_QueryInterface(supports, &rv);
  

put these two on the same line
also missing rv check

Yes, thanks.

Comment 10

[Magnus Melin [:mkmelin] (away, back June 30)]

Re-reading, maybe it's not worth doing in a helper, but just change the if's to be properly if and some else if. If you change it to use Preferences::GetBool instead of the old-style it would be much more readable. I would do it in this bug since it's so hard to verify the logic works in the current code.

Comment 11

[:aceman]

Attachment: 772392.patch v4

I don't see the benefit in creating 10 indentation levels from if() else if() blocks.
This is what I proposed, a loop that can show multiple warning if they are relevant, e.g. try deleting a starred msg from trash.

For the test, I tried to first cancel the warning, but cancelling the deletion throws and I somehow can't catch it (probably because it is issued somewhere deep in the internals of the nsIMsgDBView.doCommand, not directly as result of the JS code), see the try run:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=bc07cadf38680b6549e8f6050a271e061ba9e9be

Comment 12

[Magnus Melin [:mkmelin] (away, back June 30)]

Comment on attachment 9119523 [details] [diff] [review]
772392.patch v4

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

::: mail/test/browser/folder-display/browser_messageCommandsOnMsgstore.js
@@ +203,5 @@
> +        .querySelector("dialog")
> +        .getButton("cancel")
> +        .doCommand();
> +  } catch (e) {
> +    // Canceling the delete will throw.

Terrible API. Can we change that?
Seems some formatting is off here.

@@ +211,5 @@
> +    press_delete(mc);
> +    wait_for_modal_dialog("commonDialogWindow");
> +  // The message wasn't deleted so we still have it.
> +  curMessage = select_click_row(1);
> +  Assert.ok(curMessage.isFlagged, "Message should have been marked Flagged!");

can you do this check before you do anything, too. then here check that it's the same message

Comment 13

[:aceman]

(In reply to Magnus Melin [:mkmelin] from comment #12)

::: mail/test/browser/folder-display/browser_messageCommandsOnMsgstore.js
@@ +203,5 @@

•    .querySelector("dialog")
  

•    .getButton("cancel")
  

•    .doCommand();
  

• } catch (e) {
• // Canceling the delete will throw.

Terrible API. Can we change that?
I don't know how to change it: https://bugzilla.mozilla.org/show_bug.cgi?id=1504002 . It looks similar to https://bugzilla.mozilla.org/show_bug.cgi?id=696186, but here (at least in the tests) the try {} catch{} does not seem to work.

Seems some formatting is off here.
As the catch does not work, I intentionally didn't bother with the formatting.

@@ +211,5 @@

• press_delete(mc);
• wait_for_modal_dialog("commonDialogWindow");
• // The message wasn't deleted so we still have it.
• curMessage = select_click_row(1);
• Assert.ok(curMessage.isFlagged, "Message should have been marked Flagged!");

can you do this check before you do anything, too. then here check that it's
the same message

OK, I can. But we need to solve the problem above. If we can't cancel the warning, there is nothing to check.