734034 - invisible directories still used by thunderbird junk mail management (after deferring an account the junk mail target folder still points to the original account, not the deferred-to one)

Status: RESOLVED FIXED

(MailNews Core :: Account Manager, defect)

Description

[olivier RIEUX-LOPEZ]

User Agent: Mozilla/5.0 (Windows NT 5.1) AppleWebKit/535.11 (KHTML, like Gecko) Chrome/17.0.963.56 Safari/535.11

Steps to reproduce:

I use varions pop3 accounts. I configured thunderbird to store incoming messages of various accounts to a single "master" account INBOX folder. That is to say, for the "slave" accounts, I did: "tools/account settings/server settings/advanced/when downlowding messages use the following folder: inbox of another account "..."."
Unfortunately, the junk folder of one of the "slave" accounts was the target of various "master" and "slave" Junk filters (Tools/account settings/junk mail settings/move new junk mail to:"..."). 


Actual results:

The "slave" accounts folders were no-longer displayed in the folder pannel.
However, the junk filters were still storing messages there (which I found out much later).
When trying to configure the target of the junk mail filters (Tools/account settings/junk mail settings/move new junk mail to:"...")., the target did no apear in the dialog (due to the fact that it was an hidden folder, I assume) and thunderbird did not accept memorizing a different target. I had to "un-slave" the target account in order to see its junk folder in the settings dialog, and correct my settings. In the meantime, I could not access mail that had been erroneously detected as junk and stored in the hidden junk folder.


Expected results:

I believe folders should  not be hidden (either in the folder pannel and inthe setting dialogs). They should either be left visible, or deleted, with suitable management (automated action or error message) of their content and of settings refering to them.

Comment 2

[Ludovic Hirlimann [:Usul]]

So you are using a global inbox right ?

Comment 3

[olivier RIEUX-LOPEZ]

Well, I am not sure what you refer to. I did not any any built-in functionality with such a name, but the result indeed is that I have a global inbox with messages from various pop3 servers.

Comment 4

[WADA:World Anti-bad-Duping Agency]

We call "to store incoming messages of various accounts to a single master account INBOX folder" "Global Inbox".
  At Server Settings of a POP3 account/Advanced button,
  - Inbox for this server's account     <= Ordinal, Non Global Inbox 
  - Global Inbox(Local Folders account) <= Global Inbox type-1
  - Inbox for different account         <= Global Inbox type-2 (perhaps your case)
                                           (note: "Local Folders" can be selected)

When an POP3 account is changed from "not use Global Inbox" to "use Global Inbox", following message is shown by Tb.
> Deffer Account?
>
> If you store this account's new mail in a different account's Inbox,
> you will no longer be able to access already downloaded e-mail for this account.
> If you have mail in this account, please copy it to another account first.
> If you have filters that filter mail into this account, you should disable them
> or change the destination folder.
> If any accounts have special folders in this account (Sent, Drafts, Templates),
> you should change them to be in another account.
>
>   [ OK ]   [ Cancel ]

> Do you still want to store this account's e-mail in a different account?

Have you read the message well?
Have you executed required actions stated in the message?
Although Junk folder, Archives folder, Virtual Folder(Search folder) are not referred in the message, any user selectable folder setting needs to be changed manually.

> They should either be left visible, or deleted, with suitable management
> (automated action or error message) of their content and of settings refering to them.

"Hiding of deffer account" is done by user's request, and is current design/implementation of "Global Inbox".
Currently, "automatic setting change upon change to Global Inbox use" is not implemented yet. Because account and his folders is hidden, user has to change folder selection to visible folder at any setting. Above message is request from Tb for manual setting change to user.
IIRC, bug for it is already opened.
One of reeasons why "not implemente yet" is "Unified Folder" is already implemented. "Unified Folder" is a kind of successor of "Global Inbox".

Comment 5

[olivier RIEUX-LOPEZ]

OK

This is clear, I made a mistake.
I would have apreciated Thunderbird to let me see and/or modify my wrong settings after I made the mistake.
Thank you for your detailed explanation.

Regards.

Comment 6

[:aceman]

I think the fix for the already broken targets is bug 536768.
Preventing the targets to get broken when activating global inbox is the problem here.
The other targets (copies, archive, sent) are automatically fixed when an account is deferred to another one. I can look if and why junk target isn't.

Comment 7

[:aceman]

Confirming and taking.

Comment 8

[Dan Pernokis]

(In reply to :aceman from comment #6)
> I think the fix for the already broken targets is bug 536768.
> Preventing the targets to get broken when activating global inbox is the
> problem here.

And THIS was probably the source of my grief in bug #787573 (Junk Settings not being saved correctly).  I always switched new accounts & entities to use Global Inbox when they were created, but perhaps not soon enough to prevent the damage.

Comment 9

[:aceman]

Attachment: patch

So this seems to work.
It also fixes the loss of selection in the account tree after an account is defered.

I am not sure why we need to always pass 'accountValues' to get/setAccountValue. I'd think they should just take 'account' and work out accountValues internally. Or is that some optimization? But it complicates the callers a bit.

Comment 10

[:aceman]

Attachment: patch v2

Comment 11

[Ian Neal]

Comment on attachment 684961 [details] [diff] [review]
patch v2

>+    // This is the same sanitization as in am-junk.js.
>+    // TODO: Maybe the routines could be merged some day.
Would be good to have some sort of helper function, need to pass an object/array in and only pass back those that have changed.

>+    let deferredURI = serverSettings.deferredToAccount ?
>+                      MailServices.accounts.getAccount(serverSettings.deferredToAccount)
>+                                           .incomingServer.serverURI : null;
Would
let deferredURI = serverSettings.deferredToAccount &&
                  MailServices.accounts.getAccount(serverSettings.deferredToAccount)
                                       .incomingServer.serverURI;
be better?
>+
>+    for each (let account in fixIterator(MailServices.accounts.accounts,
>+                                         Components.interfaces.nsIMsgAccount)) {
>+      let accountValues = parent.getValueArrayFor(account);
>+      let type = parent.getAccountValue(account, accountValues, "server", "type", null, false);
>+      if (type == "pop3") {
You could inline type here.

r- for the moment.

Comment 12

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 684961 [details] [diff] [review]
patch v2

The text is long enough that I doubt people will read it, but the change makes sense, so I'll say ui-r=me.  ;)

Thanks,
Blake.

Comment 13

[:aceman]

Bwinton, maybe we could split it into some paragraphs? But I am not sure how that is done in this kind of prompt. Maybe with \n in the string?

Comment 14

[:aceman]

Attachment: patch v3

IanN, I made the shared function now. More comments than code, but at least it does not diverge in the future.

Bwinton, the string again :)

Comment 15

[:aceman]

Attachment: screenshot of new text

Comment 16

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 685288 [details]
screenshot of new text

(I'll do the review on the patch itself…)

Comment 17

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 685276 [details] [diff] [review]
patch v3

I'm not sure whether we need the localization note or not, but other than that I like how it looks.  (I mean, it's still way more text than anyone will read, but it's better than it used to be.  ;)

ui-r=me!

Thanks,
Blake.

Comment 18

[Wayne Mery (:wsmwk)]

(In reply to Blake Winton (:bwinton) from comment #17)
> Comment on attachment 685276 [details] [diff] [review]
> patch v3

still needing iann review

Comment 19

[Ian Neal]

Comment on attachment 685276 [details] [diff] [review]
patch v3

>+      let type = parent.getAccountValue(account, accountValues, "server", "type",
>+                                        null, false);
>+      // Try to keep this list of account types not having Junk settings
>+      // synchronized with the list in AccountManager.js.
>+      if (type != "nntp" && type != "rss" && type != "im") {
It would be good if there was a way of telling from the account object whether it was capable of having Junk settings or not, but that is outside of scope of this bug.
The code looks good and I've tested it as much as I can but I am not a regular user of junk mail management so could do with further testing from someone that is.

Comment 20

[Magnus Melin [:mkmelin]]

Comment on attachment 685276 [details] [diff] [review]
patch v3

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

Looks good to me. r=mkmelin

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/1e7fb1063216