957495 - Error message newly shown by patch for bug 782738 is wrong

Status: RESOLVED FIXED

(MailNews Core :: Filters, defect)

Description

[WADA:World Anti-bad-Duping Agency]

+++ This bug was initially created as a clone of Bug #931303 +++
This bug id spin-off of a part of bug 931303 comment #10.

Error message newly shown by patch for bug 782738 is wrong.

By patch for bug 782738, error check of (i) in following is added in addiion to existent error check of (ii), and the error is correctly chcked.
(i)  MsgDBHdr addition to Msgdatabase (<Folder>.msf relevant)
     (eg. for moved new mail by filter)
(ii) Write open of MsgStore file (<Folder> file relevant)
     (e.g. append of mail data of mail moved by filter)

However, same error message is shown, so the error message shown by (i) is pretty confusing, and it always produce misleading because the message mentions error around (ii) only.
The shown message is;
> http://mxr.mozilla.org/comm-central/source/mail/locales/en-US/chrome/messenger/messenger.properties#79
> filterFolderWriteFailed=The messages could not be filtered to folder '%S'
> because writing to folder failed.
> Verify that you have enough disk space, and that you have write privileges to the file system, then try again.

Comment 1

[:aceman]

OK, let's ask Irving as he reviewed the culprit patch whether he would accept the change proposed.

I would also support this change as there are bug reports cropping up about users getting folder write errors and filter move errors and distinguishing various causes by proper messages could help us diagnose those.

Comment 2

[:Irving Reid (No longer working on Firefox)]

Yes, go ahead and improve the error reporting on this issue.

Comment 3

[:aceman]

WADA, so you want to distinguish when this part fails:
2506   if (destMailDB)
2507     rv = destMailDB->CopyHdrFromExistingHdr(nsMsgKey_None, mailHdr, true,
2508                                             getter_AddRefs(newHdr));
2509   if (NS_SUCCEEDED(rv) && !newHdr)
2510     rv = NS_ERROR_UNEXPECTED;

from this one?

2513     rv = AppendMsgFromStream(inputStream, newHdr, messageLength,
2514                              destIFolder);
2515   }
2516 
2517   if (NS_FAILED(rv))

Yes, currently both failures show the message at:
2525       destIFolder->ThrowAlertMsg("filterFolderWriteFailed", msgWindow);

Comment 4

[WADA:World Anti-bad-Duping Agency]

(In reply to :aceman from comment #3)
> WADA, so you want to distinguish when this part fails:
> 2506   if (destMailDB)
> 2507     rv = destMailDB->CopyHdrFromExistingHdr(nsMsgKey_None, mailHdr,
> 2508                                             getter_AddRefs(newHdr));
> 2509   if (NS_SUCCEEDED(rv) && !newHdr)
> 2510     rv = NS_ERROR_UNEXPECTED;
> from this one?
> (snip)
Yes.
Although "modification of current message"(refer to both cases) is possible, I believe different messages are better, because "cause of error" and "required action to recover" is absolutely different.
- new message for error in CopyHdrFromExistingHdr(MsgDatabase==.msf related error).
- current message for error in "append mail data to msgStore file".

Comment 5

[:aceman]

But is the difference so important that we need to make it known to the common user?
He may not understand the difference of msf vs. the mbox file.
What would be the scenario when one operation succeeds and other fails?

Comment 6

[WADA:World Anti-bad-Duping Agency]

(In reply to :aceman from comment #5)
> But is the difference so important that we need to make it known to the common user?
When eror at "507 rv = destMailDB->CopyHdrFromExistingHdr", "what user needs to do for recovery" is usually "Rebuild-Index of relervant folder".
When error at "2513 rv = AppendMsgFromStream(inputStream, newHdr, messageLength", error occurs on file named "xxx" after "normal xxx.msf open, normal .msf read, normal MsgDHdr creation". So, current message suggests disk space check, write permission check etc.

Why the difference can be never-important?

> He may not understand the difference of msf vs. the mbox file.

Even if so, user needs to check about xxx.msf file if error at "507 rv = destMailDB->CopyHdrFromExistingHdr" occurred, and user needs to check about xxx file if error at "2513 rv = AppendMsgFromStream(inputStream, ..." occurred, before user starts recovery procedure from error.

> What would be the scenario when onen operation succeeds and other fails?

Tyipical case of error at "507 rv = destMailDB->CopyHdrFromExistingHdr" is bug　931303. Read that bug well, please.
A tyipical case of eror at "2513 rv = AppendMsgFromStream(inputStream, ..." is "disk full while appending mail data to file named XXX instead of XXX.msf". So current error message asks user to check it.
Both errors are absolutely independent.
After patch for bug 782738, Tb won't go ahead if error at "507 rv = destMailDB->CopyHdrFromExistingHdr" is detcted. So, after patch for bug 782738, eror at "2513 rv = AppendMsgFromStream" won't happen, if error at "507 rv = destMailDB->CopyHdrFromExistingHdr" is detected.

Comment 7

[:aceman]

Attachment: 957495.patch

How would you like this?

Comment 8

[Josiah Bruner [:jsbruner]]

Comment on attachment 8418276 [details] [diff] [review]
957495.patch

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

Other than a few nits I like this. ui-r+ with the comments addressed.

::: mail/locales/en-US/chrome/messenger/messenger.properties
@@ +80,5 @@
>  deletingMsgsFailed=Unable to delete messages in folder %S because it is in use by some other operation. Please wait for that operation to finish and then try again.
>  alertFilterCheckbox=Do not warn me again.
>  compactFolderDeniedLock=The folder '%S' cannot be compacted because another operation is in progress. Please try again later.
>  compactFolderWriteFailed=The folder '%S' could not be compacted because writing to folder failed. Verify that you have enough disk space, and that you have write privileges to the file system, then try again.
> +filterFolderHdrAddFailed=The messages could not be filtered to folder '%S' because adding a message to it failed. Verify that the folder is displaying properly or repairing it from the folder properties.

I think the last sentence should read: "Verify that the folder is displayed properly or try to repair it from the folder properties.

::: suite/locales/en-US/chrome/mailnews/messenger.properties
@@ +73,4 @@
>  alertFilterChanged=Filters associated with this folder will be updated.
>  filterDisabled=The folder '%S' could not be found, so filter(s) associated with this folder will be disabled. Verify that the folder exists, and that filters point to a valid destination folder.
>  filterFolderDeniedLocked=The messages could not be filtered to folder '%S' because another operation is in progress.
>  parsingFolderFailed=Unable to open the folder %S because it is in use by some other operation. Please wait for that operation to finish and then select the folder again. 

White space, could you remove this while you're here?

@@ +77,5 @@
>  deletingMsgsFailed=Unable to delete messages in folder %S because it is in use by some other operation. Please wait for that operation to finish and then try again.
>  alertFilterCheckbox=Do not warn me again.
>  compactFolderDeniedLock=The folder '%S' cannot be compacted because another operation is in progress. Please try again later.
>  compactFolderWriteFailed=The folder '%S' could not be compacted because writing to folder failed. Verify that you have enough disk space, and that you have write privileges to the file system, then try again.
> +filterFolderHdrAddFailed=The messages could not be filtered to folder '%S' because adding a message to it failed. Verify that the folder is displaying properly or repairing it from the folder properties.

I think the last sentence should read: "Verify that the folder is displayed properly or try to repair it from the folder properties.

Comment 9

[:aceman]

Attachment: patch v2

Thanks.

Comment 10

[:aceman]

WADA, can you please look over the patch if it is what you wanted? It is ready for check in now.

Comment 11

[WADA:World Anti-bad-Duping Agency]

I think new filterFolderHdrAddFailed is good.

Comment 12

[:aceman]

Thanks.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/66e6235a2201