536768 - Storing junk filter settings broken for deferred email account if spamActionTargetAccount/spamActionTargetFolder points to hidden serverX or deleted serverX, account/folder selection at Junk Settings UI appears blank and is not saved

Status: RESOLVED FIXED

(MailNews Core :: Account Manager, defect)

Description

[RuCla]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; de; rv:1.9.1.6) Gecko/20091201 YFF35 Firefox/3.5.6
Build Identifier: Thunderbird 3.0

I have updated my thunderbird installation to 3.0 and I when I try to store the junk filter settings nothing happens: 
The trusted address books and the global settings for junk filter (like move to folder) of the tools -> account settings are not stored.
I use the german version. 

Reproducible: Always

Steps to Reproduce:
1. Open the account settings dialog
2. Select Junk Filter
3. Make some changes
4. Leave the dialog with "OK"
5. Open the account settings dialog again and check the junk entries
Actual Results:  
The changes are not stored

Expected Results:  
The changes shoulb be stored

Comment 1

[cantalou]

Tested with GMAIL account and Thunderbird v3.0:
Mozilla/5.0 (Windows; U; Windows NT 5.1; fr; rv:1.9.1.5) Gecko/20091204 Lightning/1.0b1pre Thunderbird/3.0 ID:20091204171430

My changes are stored OK

I changed it to Account Manager

**********************
Could you please try to start TB in safe mode
http://kb.mozillazine.org/Safe_mode
Maybe your problem is coming from an extension.

If problem is not coming back in safe mode, could you please give us list of your extension.
Nightly Tester Tools extension may help you
https://addons.mozilla.org/en-US/thunderbird/addon/6543
Tools => Nightly Tester Tools => Copy List of Extensions to Clipboard

If problem is coming back in safe mode, could you please check your error console (Tools => Error Console) and copy & paste any message in this bug

Comment 2

[RuCla]

I started Thunderbird in Safe mode and when I set the checkboxes for the address books, I get the following error (in Error Console):
Fehler: uncaught exception: unable to find folder to select!

Then I started the settings dialog again and I changed the movement folder and I get the same exception.

I have just made the change for a local folder and the folder is created and available

Comment 3

[WADA:World Anti-bad-Duping Agency]

"Junk folder" version of phenomenon of bug 535183 on "Copies&Folders". See bug 535183 comment #6.
AFAIR, already opened bug exists for both "Copies&Folders" case and "Junk folder" case. Cause is "user didn't do required actions when user changed POP3 account setting to use Global Inbox" in many cases.

Comment 4

[RuCla]

But my accounts were already set to a global inbox in tb 2. so I have not changed this...

Comment 5

[WADA:World Anti-bad-Duping Agency]

(In reply to comment #4)
> But my accounts were already set to a global inbox in tb 2. so I have not
> changed this...

(a) You defined POP3 accounts by Tb2 as "use Global Inbox" upon initial definition of the accounts? (b) Or you changed the accounts from "no Global Inbox use" to "use Global Inbox" by Tb2?

If (b), "I have not changed this..." may be cause.
 1. Upon account definition, "Junk folder" is set to Junk of the account.
 2. Change account from "no Global Inbox use" to "use Global Inbox".
    => (2-1) Tb warns user by dialog which states required action.
       But user didin't do required actions.
 3. By change from "no Global Inbox use" to "use Global Inbox",
    serverx.deferred_to_account=accountN(owner of Global Inbox) is set and the
    account is hidden at folder pane. Thus Junk of the account can't be viewd.
 4. Tb2 doesn't warn this situation.
    (4-1) Tb3 warns this situation by "won't save setting with no message".
Bug for improvement of (2-1) is already opened. Bug for (4-1) is also already opened as I said.

If (a), and even if (b) and you did required actions properly, similar situation to bug 534382 or bug 535183 may occur upon upgrade to Tb3 if garbages existed in your prefs.js. In this case, (4-1) can happen as seen in bug 535183.

(In reply to comment #2)
> I have just made the change for a local folder and the folder is created and available

What was set before your change to folder of "local folder"?

Comment 6

[RuCla]

It was and is empty

Comment 7

[RuCla]

I have already defined "use Global inbox" in TB2. After I have updated to TB3 I didn't make any changes. Now I have checked the setting and it correct. So what shall I do to fix the error?

Comment 8

[RuCla]

There is another strange thing:
Sometimes the junk filter (and all other filter) work proper (e.g. move to the set folder) and sometimes they do not work. After I restart TB and when I execute the filter (also junk filter) manually they work properly.
BTW.: Is there a problem with extension ToneQuilla (1.0.1) and TB3?

Comment 9

[WADA:World Anti-bad-Duping Agency]

(In reply to comment #2)
> I have just made the change for a local folder and ...
(In reply to comment #6)
> It was and is empty

What operation at which panel do you mean by "I have just made the change for a local folder"?
What do you mean by "empty"? What was/is "empty"?
Account Settings/Junk Settings is displayed initially as next.
  [ ] Move new junk messages to: (unchecked)
      [ x ] "Junk" Folder on: <this account's name> (grayed out)
      [   ] Other: Junk on <this account's name>    (grayed out)
  (If account is defined by Tb with "use Global Inbox" since initial,)
  (<account's name of Global Inbox> is displayed.                    )

(In reply to comment #2)
> I have just made the change for a local folder and the folder is created and available

Does it mean that Junk folder of "Local Folders" didn't exist until your change?
If yes, cause of "Junk setting is not saved" may be "pointed folder doesn't exist" in your case, because Tb2 didn't create Junk folder automatically when "Junk" Folder on: <account name>" is set.
AFAIR, if Junk of Local Folders is pointed but if Junk folder of Local Folders doesn't exist, Tb2 moved Junk mail to Trash, and the mail was lost by Unjunk(known bug of Tb2, is already fixed by Tb3).

(In reply to comment #8)
> There is another strange thing:

bug 310431 and bug 324777 are still alive.

Comment 10

[RuCla]

The settings are like this:

[URL=http://img693.imageshack.us/i/junk.png/][IMG]http://img693.imageshack.us/img693/2191/junk.th.png[/IMG][/URL]

And I have changed
[ ] Move new junk messages to: (unchecked)
      [  ] "Junk" Folder on: <this account's name> (grayed out)
      [ x] Other: Junk on <Local folder>    (grayed out)

And I have only one Local Folder, see:

[URL=http://img136.imageshack.us/i/localfolder.png/][IMG]http://img136.imageshack.us/img136/1694/localfolder.th.png[/IMG][/URL]

and this folder was already defined in tb2

Comment 11

[WADA:World Anti-bad-Duping Agency]

I could observe "empty" you said by changing a POP3 account to "use Global Inbox(Local Folders account)" using Tb3.
> (A)
> [ ] Move new junk messages to: (unchecked)
>     [ ] "Junk" Folder on: *EMPTY* (not grayed out)
      [ ] Other: *EMPTY*            (not grayed out)
At this step, check/uncheck of "Junk" Folder on:/Others: was posibble, and account/folder selection was possible.

After cacel and re-entering to Account Settings, checked "Move new junk...", and unchecked "Move new junk...". Following was displayed.
> (B)
> [ ] Move new junk messages to: (unchecked)
>     [x] "Junk" Folder on: *EMPTY* (grayed out)
>     [ ] Other: *EMPTY*            (grayed out)

After OK, I re-entered in Account Settings. Junk setting display returned to (A).

At (A), select an POP3 account, and press OK.
> [ ] Move new junk messages to: (unchecked)
>     [ ] "Junk" Folder on: <other POP3 account name> (not grayed out)
      [ ] Other: *EMPTY*                              (not grayed out)
When entered Account Settings again, Junk setting display became next.
> (C)
> [ ] Move new junk messages to: (unchecked)
>     [x] "Junk" Folder on: <other POP3 account name> (grayed out)
      [ ] Other: Junk on <Local Folders>              (grayed out)

"Junk setting is not saved" looks to be caused by (A) in your case.
The "empty" display me be a warning of "selection/change is required after change to Global Inbox use by user".

Comment 12

[WADA:World Anti-bad-Duping Agency]

Phenomenon was;
(1) POP3 account(c@c.c.c) is defined, and next is set. 
> mail.server.server6.spamActionTargetAccount;mailbox://c@c.c.c
(2) This POP3 account(c@c.c.c) is changed to "use Global Inbox(Local Folders Account)".
(3) At Junk setting, *EMPTY* is displayed, because c@c.c.c is hidden and is not displayed in account/folder selection list.
(4) After change of Junk setting to non-hidden account/non-hidden folder at UI,
    change is not saved in prefs.js(no change in Config Editor).
    Still next is set.
> mail.server.server6.spamActionTargetAccount;mailbox://c@c.c.c
    => *EMPTY* is displayed upon next Account Settings or by restart of Tb.

Confirming.

Workaround:
  Change Junk account/folder selection before change to "use Global Inbox".
Once problem of this bug happened, next procedure is required.
(1) change back to no Global Inbox use, (2) change Junk account/folder selection, then (3) change to "use Global Inbox" again.

Comment 13

[RuCla]

I have tried to realize the workaround but it failed. When I make step 3 (use Global Inbox) the same problem as before occurs because the junk folders of the accounts are now hidden again and when I select the local folder junk folder nothing happens....
Is there another way of a workaround?

Comment 14

[WADA:World Anti-bad-Duping Agency]

(In reply to comment #13)

Did you do step (2) properly?
(a) Check "Move new junk..." => "Junk" Folder on:/Other: can be changed
(b) Select "Junk" Folder on: Local Folders / Other: Junk on Local Folders
(c) OK, and save
(d) If you don't want to move junk mail automatically, unheck "Move new junk..."

If Tb3, (b) should currently be executed before step (3), and (a) is required for (b).

Comment 15

[RuCla]

I have two questions:
1. When you write (a), do you mean the settings for each account or the global settings of the local folder?
2. What do you mean, when you write "save" in step (c)?

So I have performed all steps, but the settings still looks like in comment 10 described...

Comment 16

[WADA:World Anti-bad-Duping Agency]

(In reply to comment #15)
> I have two questions:
> 1. When you write (a), do you mean the settings for each account or the global
> settings of the local folder?

Junk settings of POP3 each account who is changed back to "no Global Inbox" at step (1) for recovery.

> 2. What do you mean, when you write "save" in step (c)?

Press "OK" at Junk settings panel, and force save in prefs.js. 
If possible, check next entries for the account.
> mail.server.serverX.spamActionTargetAccount
> mail.server.serverX.spamActionTargetFolder
If these are set to non-hidden account/folder even after "enable use Global Inbox again", problem of this bug won't occur.

If multiple accounts who uses Global Inbox have such setting(points hidden account/hidden folder, change setting of all such accounts at once, to avoid unwanted problem.

Comment 17

[RuCla]

After enabling Global Inbox I got the following entries:
mail.server.server1.spamActionTargetAccount;mailbox://[e-mail]
mail.server.server1.spamActionTargetFolder;mailbox://nobody@Local%20Folders/Junk

mail.server.server2.spamActionTargetAccount;mailbox://[e-mail]
mail.server.server2.spamActionTargetFolder;mailbox://nobody@Local%20Folders/Junk

mail.server.server3.spamActionTargetAccount;mailbox://[e-mail]
mail.server.server3.spamActionTargetFolder;mailbox://nobody@Local%20Folders/Junk

mail.server.server4.spamActionTargetAccount;mailbox://[e-mail]
mail.server.server4.spamActionTargetFolder;mailbox://nobody@Local%20Folders/Junk

mail.server.server5.spamActionTargetAccount;mailbox://[e-mail]
mail.server.server5.spamActionTargetFolder;mailbox://nobody@Local%20Folders/Junk

mail.server.server6.spamActionTargetAccount;mailbox://[e-mail]
mail.server.server6.spamActionTargetFolder;mailbox://nobody@Local%20Folders/Junk

mail.server.server7.spamActionTargetAccount;mailbox://[e-mail]
mail.server.server7.spamActionTargetFolder;mailbox://nobody@Local%20Folders/Junk

mail.server.server8.spamActionTargetAccount;mailbox://[e-mail]
mail.server.server8.spamActionTargetFolder;mailbox://nobody@Local%20Folders/Junk

Can you please tell me to which value I should change them?

Comment 18

[WADA:World Anti-bad-Duping Agency]

(In reply to comment #17)
> After enabling Global Inbox I got the following entries:
>(snip)
> Can you please tell me to which value I should change them?

If all above xxx.serverX.xxx settings has deferred_to_account=accountN(==use Global Inbox), all serverX should be set next. 
> mail.server.serverX.spamActionTargetAccount;mailbox://nobody@Local%20Folders

It can be set/saved via ordinal UI by next;
 1. Change to "no Global Inbox use" (no deferred_to_account is set).
    Server Settings/Advanced, Inbox for this server's account
 2. account selection for "Junk" Folder on: at Junk Settings panel.
    folder selection for Others: at Junk Settings panel.
    - You didn't do;
      "Junk" Folder on: Local Folders => saved in spamActionTargetAccount 
    - You did do;
      Other: Junk on Local Folders    => saved in spamActionTargetFolder

Comment 19

[WADA:World Anti-bad-Duping Agency]

mailbox://xyz@a.b.c in spamActionTargetAccount/spamActionTargetFolder points;
  First serverX which has serverX.hostname=a.b.c & serverX.userName=xyz.
  If xyz contains "@", it's escaped by "%40".
Problem occurs when;
  spamActionTargetAccount and/or spamActionTargetFolder points serverX for;
    a) hidden account by "use Global Inbox"
       accountN.server=serverX, serverX.deferred_to_account=accountA
    b) hidden account by hidden=true ("Smart Folders")
       accountN.server=serverX, serverX.hidden=true
    c) deleted account/server
       no serverX corresponds to mailbox://xyz@a.b.c
So, problem occurs on both ordinal account and POP3 account who uses Global Inbox.

And, once problem occurred at Junk Settings panel of an account(empty is displayed), empty is still displayed when user switched to Junk Settings panel of other account.
It produces user's confusion and it makes recovery via ordinal UI difficult.

Change spamActionTargetAccount/spamActionTargetFolder via Config Editor, please.

Comment 20

[RuCla]

I have tested Comment 18 and the wordaround works fine for me!

Comment 22

[Wayne Mery (:wsmwk)]

good workaround at http://getsatisfaction.com/mozilla_messaging/topics/tbird_3_junk_account_settings_not_saved#reply_1975930 in http://gsfn.us/t/r4kx

Comment 23

[Wayne Mery (:wsmwk)]

can someone check if bug 546482 and bug 676228 are duplicates?

Comment 25

[WADA:World Anti-bad-Duping Agency]

xref bug 534382. Issue like bug 534382 upon profile migration can generate condition which can produce problem of this bug.

Comment 26

[WADA:World Anti-bad-Duping Agency]

FYI.

After Tb 3, account creation is always done by auto-config if mail account. In auto-config of POP3, account is always created as ordinal account(not use Global Inbox). When user changes ordinal POP3 account to Global-Inox-use-account, Tb currently doesn't change "pointer to folder in Junk Settings/Copies&Folders settings" automatically, so Tb shows user a warning dialog which requests user to change them manually. If user ignores the warning dialog, condition of "Junk Settings/Copies&Folders points folder of hidden account".

Because Tb 3 or later doesn't force "use of Global Inbox" on POP3 account in Tb when account creation any more, number of POP3 users who uses Global Inbox is smaller than before. However, majority of users who changes POP3 account from ordinal one to Global-Inbox-use will ignore the warning dialog shown by Tb, I believe. So, opening rate of bug report like "Junk Settings/Copies&Folders doesn't save" won't be declined very much, even after frequency of this bug's problem produced by "duplicated servers by old/unknown bugs"+"duplicated server deletion by profile migration" will becomes ZERO.

Comment 27

[GrafLukas]

Update: The bug occurs in TB8 as well, several users auf the german version of TB8 complain about it, including me. I have never had the problem before.

The "Account Settings" screen looks like [url=http://getsatisfaction.com/mozilla_messaging/topics/tbird_3_junk_account_settings_not_saved]in the screenshot of this thread[/url], the workaround given there would partly seem to work - but only as long as I only apply changes to the Junk Settings in "Local Folders". As soon as I start applying changes to the Junk Settings of different accounts, the bug will come back.

I would strongly appreciate if this could be worked on and be fixed!

Comment 30

[Wayne Mery (:wsmwk)]

I'm unsure to which of the 3 bugs this belongs, so I'm just going to cite it http://getsatisfaction.com/mozilla_messaging/topics/junk_filter_not_working_after_os_reinstall

Comment 31

[:aceman]

Patch in bug 472959 is nearing checkin. It basically resets all invalid targets (folders or accounts) for junk filtering to some safe values and disables moving of junk mail. Yes, Junk messages will no longer be automatically moved, but at least it will allow the user to properly set them again. The form controls should not be broken and unusable as in the screenshots here.

Let's see if it fixes this bug as well.

Comment 32

[:aceman]

So the patch was checked in. Can anybody retest this in TB13?

Comment 33

[:aceman]

I tested this and it seems the fix in bug 472959 is not enough. I can't reset the folder to the Current account/Junk. In case of deferred folders I probably need to reset it to the Deferred account/Junk.

Comment 35

[:aceman]

Note to myself: When the current account is deferred to another one, the invalid target variants I have found so far:
1. the targetAccount/Folder points to the current account (server).
In this case, reset the targets to the pointed to server (and its Junk folder).
2. the targetAccount/Folder points to another account (server), that itself is deferred (points inbox to another server).
In this case follow the deferring until a valid undeferred account is found. In worst case reset to Local Folders, that can't be deferred.

(Bug 472959 fixed only the case where the current account is not deferred.)

Comment 37

[:aceman]

I'll revisit this shortly once my other patches in the junk settings land.

Comment 38

[jonwestburne]

I have found this difficult to follow but worked out the following solution:
Problem:
TB will be installed and the user will have chosen the option to use a global inbox
The user will click on a warning that folders will cease to be available and filters etc must be changed but it will not spell out that it covers junk mail settings.
The user will "lose" messages identified as junk which will go to the hidden junk folder for that mail service.
If the user tries to change the junk setting the change will not be saved and if they check the Error console it will say a folder cannot be found.
Solution:
I used notebook to edit the prefs.js file to delete the "SpamActionTargetFolder", a user set entry for that appeared for some of my mail server. I cannot say whether this is actually necessary.
What is necessary:
Change every mail service from global to "inbox for this servers account" (server settings - advanced)
Close and open TB - it will now show all the inboxes and junk folders for every mail service so missing messages can be looked for.  It should also give junk settings for each mail service that previously did not show up.
For each mail service on junk settings tick on "move new junk messages to" and select "junk folder on" and choose the folder that has your global inbox in it - NB the second option "other" and choosing the same folder does not work.
When all the mail services point to a folder that will not be hidden you can change those you want to back to global inbox.  Close and open TB.
Thereafter you should see the folders you want and Junk Settings should show up and only point to visible folders.
The fix I would like to see is for the change to global inbox to include an explicit change to a global junk folder.

Comment 39

[:aceman]

Just to disambiguate the bugs:

- Bug 734034 is about the Junk target folder not getting automatically set to the deferred to folder once "use Inbox of other account" or "Global inbox" is used. So the junk mails go into the original junk folder that is now hidden. The Junk target should be properly set automatically.

- This bug 536768 will try to fix settings of already broken accounts (in the wrong state from point 1).

Comment 40

[:aceman]

Attachment: screenshot of a broken Junk settings pane

This is what the Junk settings pane in the account will look like for anybody experiencing this bug.

Comment 41

[:aceman]

Attachment: patch

There are already enough bugs and GS reports.

Let's start some work.
The patch uses a simpler version: If the deferred to account is still not usable (deferred again), then falls back on Local Folders. I am not sure following the path of deferring further is wanted.

The chooseJunkTargetFolder function is intentionally put into amUtils.js so that it will be later reused in bug 734034.

Comment 42

[Ian Neal]

Comment on attachment 623353 [details] [diff] [review]
patch

r-
With this applied the "Automatically delete junk mail older than" text doesn't get properly disabled when switching between Junk Settings for IMAP and local folders.

Comment 43

[:aceman]

Not sure what you mean. That text is only disabled when whole "move new junk messages to" is disabled. Then both the pickers and this checkbox+text+number field are disabled. There is no more granularity. I have bugs for that like bug 728681.

Comment 44

[Ian Neal]

STR:
1/ Create a profile with an IMAP account.
2/ Go into account settings
3/ Select "Junk Settings" pane under IMAP account.
4/ Look at items underneath "Move new junk messages to:" (which should be unchecked)
5/ Everything should be greyed out.
6/ Select "Junk Settings" pane under Local Folders
7/ Look at items underneath "Move new junk messages to:" (which should be unchecked)
8/ Not everything is greyed out ("Automatically delete junk mail older than" isn't disabled).
9/ Switch back to "Junk Settings" pane under IMAP account.
10/ Look at items underneath "Move new junk messages to:"
11/ Not everything is greyed out ("Automatically delete junk mail older than" isn't disabled).

Comment 45

[:aceman]

I can't reproduce that. Are you sure it only happens with my patch?
Are there any exceptions in Error console?

Comment 46

[Ian Neal]

(In reply to :aceman from comment #45)
> I can't reproduce that. Are you sure it only happens with my patch?
> Are there any exceptions in Error console?

Nothing in the error console and doesn't happen without your patch. I am testing on linux.

Comment 47

[:aceman]

Is the addresbook whitelist populated correctly? Can you attach a screenshot?

Comment 48

[Ian Neal]

Attachment: Incorrectly active settings

Comment 49

[:aceman]

Thanks. Notice that "days" is disabled. How would that work?

Comment 50

[:aceman]

I see the "days" element is getting disabled in a different way.

And I can also reproduce your problem now. The catch was to uncheck the "move" in both accounts and then close AM and reopen it.

Comment 51

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 623353 [details] [diff] [review]
patch

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

Removing review request until Ian's issue is resolved.

Comment 52

[Wayne Mery (:wsmwk)]

workaround solved Frank's problem noted in http://getsatisfaction.com/mozilla_messaging/topics/thunderbird_wont_remember_junk_and_trash_settings?utm_content=reply_link&utm_medium=email&utm_source=reply_notification#reply_8892420

Comment 53

[Mike Conley (:mconley) (:⚙️)]

aceman:

You asked me to reproduce Ian's issue - I've successfully reproduced it.

-Mike

Comment 54

[:aceman]

Thanks mconley. I have no idea why the patch would affect disabling that unrelated element (and only that one).

Comment 55

[Wayne Mery (:wsmwk)]

there seems to be quite a few recent reports against version 11 and version 12, respectively (partial list, and some tagged speculatively)
https://getsatisfaction.com/mozilla_messaging/tags/tb11junk
https://getsatisfaction.com/mozilla_messaging/tags/tb12junk

Comment 56

[Wayne Mery (:wsmwk)]

requesting tracking to potentially get this in v14 because of uptick in reports against recent versions.

Comment 57

[:aceman]

If only I could find out why the patch breaks other things (comment 42).

Comment 58

[:aceman]

Still no progress in reproducing it. Any ideas? Ian, do you get the problem consistently every time?

Comment 59

[Ian Neal]

(In reply to :aceman from comment #58)
> Still no progress in reproducing it. Any ideas? Ian, do you get the problem
> consistently every time?

Yes, I do.

Comment 60

[:aceman]

Attachment: patch v2

Could you try this?

Comment 61

[Ian Neal]

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

No, that has not fixed it :(

Comment 62

[:aceman]

Attachment: patch v3

Next try.

Comment 63

[:aceman]

Just remove the debugging alert :)

Comment 64

[Ian Neal]

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

As well as an alert popup, when I click on the local folders junk settings I get the following in the error console:
Timestamp: 23/05/12 01:19:01
Error: NS_ERROR_ILLEGAL_VALUE: Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIRDFService.GetResource]
Source File: resource:///modules/MailUtils.js
Line: 83

Comment 65

[:aceman]

And what about the field disabling?

Comment 66

[:aceman]

I do not get any such error in TB.

Comment 67

[Ian Neal]

(In reply to :aceman from comment #65)
> And what about the field disabling?

That's not fixed either.

Comment 68

[:aceman]

Attachment: patch v4

Thanks to mconley we could find one exception throwing on a new account that had the target prefs not initialized (empty). So here is a new patch.

Comment 69

[:aceman]

Bwinton, there is one catch in this patch. If the targets are silently fixed by the code, the user will not know it. He may see the final pane values are OK so he may press Cancel. Then the fixed values will not be saved and the problem will continue.
Until the sliding alerts are implemented in other bug, we should solve this in some way:
1. alert the user about the need to OK the settings (with standard obtrusive alert).
2. save the new settings silently (I am not sure how to do that).

Comment 70

[:aceman]

But the silent save would need to save only the fixed settings, not everything (also from other panels) because the user may want to Cancel all the other settings and we must not save them forcefully.

Comment 73

[:aceman]

Workaround until this bug is fixed properly:
1. Go into Account settings -> "account with the problem" -> Server settings -> Advanced.
2. disable the global inbox (select "inbox for this server's account)
3. then go into Junk settings, set the "Junk folder on" to be "Local Folders" and "Other" to be "Junk on Local Folders" (or other safe account that will is not set to store it's mail into some other account).
4. set "Enable adaptive junk mail controls" to the state you wish.
5. Close the account settings with OK.
6. Go back into account setting to see if the settings now stick and are OK.
7. If yes, you can enable Global inbox on the account again (Account settings -> "account with the problem" -> Server settings -> Advanced).

Comment 74

[jweber.mail]

Hi to all,
I did it like that (comment of 2012-06-28 13:27:25 PDT) and it works.
Thanks and regards
Joachim

Comment 75

[Ian Neal]

Comment on attachment 634583 [details] [diff] [review]
patch v4

This is bit rotted by your patch from bug 742503, do you have a version that can be applied on top of that?
Cancelling review request until new patch is available.

Comment 76

[:aceman]

It may be true that the patch will be bitrotted by bug 742503 but that one is not yet checked in. How do you see the bitrot and why is it a problem?

Comment 77

[Ian Neal]

(In reply to :aceman from comment #76)
> It may be true that the patch will be bitrotted by bug 742503 but that one
> is not yet checked in. How do you see the bitrot and why is it a problem?

I want to test the patch here running on top of the patch from bug 742503 and make sure there are no issues. I've previously tested both separately but want to make sure one doesn't break the other.

Comment 78

[:aceman]

Ok, but other reviewers may not want such a version. So I'll better wait until bug 742503 lands.
You could at least say if the problem from comment 42 is fixed in the standalone run.

Comment 79

[:aceman]

Or you can decide which bug is more important to get into TB16 and leave that patch unchanged and let the reviewing continue. (I'd say this one.) As I will not be able to make any new patches until 17th of July :(

Comment 80

[Ian Neal]

Comment on attachment 634583 [details] [diff] [review]
patch v4

Code review first:
>+++ b/mailnews/base/prefs/content/am-junk.js
>+function checkTargetFolder(targetURI, folderName)
Nit: arguments should start with a, e.g. here it should have been aTargetURI and aFolderName

>+{
>+  let returnFolder = targetURI;

If you passed either the id ("server.spamActionTargetAccount" and "server.spamActionTargetFolder") or target type (Account and Folder), you could have respectively:
let returnFolder = document.getElementById(aId).value;
or
let returnFolder = document.getElementById("server.spamActionTarget" + aType).value;
>+  let isServer = (folderName != "");
You probably don't even need to pass a folder name either, as you know from the id/type whether you need to use "Junk" or you could pass a true/false instead, so something like:
function checkTargetFolder(aId, aIsAccount)
or:
function checkTargetFolder(aType, aIsAccount)
>+
>+  try {
>+    // does the target account exist?
>+    let targetServer = GetMsgFolderFromUri(returnFolder + (isServer?("/" + folderName):""), !isServer).server;
Nit: should have spaces around the '?' and ':'. You don't need () around '"/" + folderName' so it would be something like:
let targetServer = GetMsgFolderFromUri(returnFolder + (aIsAccount ? "/Junk" : ""), !aIsAccount).server;

> function onInit(aPageId, aServerId)
> {
>   // manually adjust several pref UI elements
>+  document.getElementById('spamLevel').setAttribute("checked",
>+    document.getElementById('server.spamLevel').value > 0);
> 
>+  let deferredToURI = null;
>+  if (gDeferredToAccount)
>+    deferredToURI = MailServices.accounts.getAccount(gDeferredToAccount).incomingServer.serverURI;
Nit: where it makes sense, remember the 80 character limit, so please wrap this line.
> 
>+  let spamActionTargetAccount = document.getElementById("server.spamActionTargetAccount").value;
>+  let spamActionTargetFolder = document.getElementById("server.spamActionTargetFolder").value;
>+
>+  // check if folder targets are valid
>+  spamActionTargetAccount = checkTargetFolder(spamActionTargetAccount, "Junk");
>+  spamActionTargetFolder = checkTargetFolder(spamActionTargetFolder, "");
See above, would become something like:
let spamActionTargetAccount = checkTargetFolder("server.spamActionTargetAccount", true);
let spamActionTargetFolder = checkTargetFolder(("server.spamActionTargetFolder", false);
or:
let spamActionTargetAccount = checkTargetFolder("Account", true);
let spamActionTargetFolder = checkTargetFolder("Folder", false);

>+++ b/mailnews/base/prefs/content/amUtils.js
>+/**
>+ * Finds a usable target for storing Junk mail.
>+ * If the passed in server is not usable, choose Local Folders.
>+ *
>+ * @param targetURI   the URI of a server or folder to try first
>+ * @param folderName  the folder name to append to the server URI
>+ *                     (if serverURI is only a server URI)
>+ * @return  the server/folder URI of a usable target for storing Junk
>+ */
>+function chooseJunkTargetFolder(targetURI, folderName)
Any reason why this is in amUtils.js and not am-junk.js?
Nit: arguments should start with a.
Rather than passing a folder name, just pass true or false.
>+{
>+  let server = null;
>+  let isServer = (folderName == "");
Note in the other function it was isServer = (folderName != "") and the logic for appending the folder name was also reversed.

>+  return server.serverURI + (!isServer?("/" + folderName):"");
Nit: should have spaces around the '?' and ':'. You don't need () around '"/" + folderName' so it would be something like:
return server.serverURI + (aIsAccount ? "/Junk" : "");

r- as I want to review any subsequent patch.

Comment 81

[Mark Banner (:standard8)]

Not tracking, but it'd be really good to get an updated patch for this.

Comment 82

[:aceman]

Attachment: patch v5

Ian, the 2 functions are in amUtils.js because I want to reuse them outside of am-junk.js, see comment 41.

Also, for that reason I want to keep them getting a folderURI instead of element ID. In the other usage (server settings -> advanced), there will be no elements holding the values, I just hope to get the URIs in some way (e.g. from accountValues).

But I have removed the passing of folderName. I once thought I need to use these functions also for other special folders (Sent, Drafts, etc.) but it seems those already cope themselves and are not hit by this bug (maybe because they are pinned to an identity, not account). So I made the 2 functions junk specific as you propose and let them take an aIsServer bool paramater.

Comment 83

[Ian Neal]

Comment on attachment 644451 [details] [diff] [review]
patch v5

>+function checkJunkTargetFolder(aTargetURI, aIsServer)
>+{
>+  let returnFolder = aTargetURI;
Why do this, why not just use aTargetURI throughout?

r=me with that addressed/fixed.

Comment 84

[:aceman]

Attachment: patch v6

Ok, reworked the function.

Comment 85

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 644783 [details] [diff] [review]
patch v6

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

This looks good - just a few style nits.

::: mailnews/base/prefs/content/am-junk.js
@@ +18,4 @@
>  
> +  let deferredToURI = null;
> +  if (gDeferredToAccount)
> +    deferredToURI = MailServices.accounts.getAccount(gDeferredToAccount)

Style nit: this is an awkward way of breaking up each component. Instead, try:

MailServices.accounts
            .getAccount(gDeferredToAccount)
            .incomingServer
            .serverURI;

@@ +38,5 @@
> +
> +  if (!spamActionTargetAccount) {
> +    // spamActionTargetAccount is not valid,
> +    // reset to default behavior to NOT move junk messages...
> +    if (moveTargetModeValue == 0)

I'm not wild about the moveTargetModeValue magic numbers. I know that's how you found them, but we might want to do the boyscout thing and replace those with constants while we're here.

Just define const kJunkFolder = 0; and const kOtherFolder = 1; and use those instead.

@@ +67,5 @@
> +  try {
> +    folder = GetMsgFolderFromUri(spamActionTargetFolder, true);
> +    document.getElementById("actionFolderPopup").selectFolder(folder);
> +  } catch (e) {
> +   /* OK for folder to not exist */

Nit: mis-aligned comment, and should use // instead of /* */

@@ +141,5 @@
>                            .getBranch("mail.server." +
>                                        account.incomingServer.key + ".");
> +
> +  if (getAccountValue(account, accountValues, "server", "type", null, false) == "pop3")
> +  {

No braces needed for the one-liner conditional.

@@ +142,5 @@
>                                        account.incomingServer.key + ".");
> +
> +  if (getAccountValue(account, accountValues, "server", "type", null, false) == "pop3")
> +  {
> +    gDeferredToAccount = getAccountValue(account, accountValues, "pop3", "deferredToAccount", null, false);

This line is > 80 chars I believe, and should be broken up over 2 lines.

::: mailnews/base/prefs/content/amUtils.js
@@ +101,5 @@
> +function checkJunkTargetFolder(aTargetURI, aIsServer)
> +{
> +  try {
> +    // Does the target account exist?
> +    let targetServer = GetMsgFolderFromUri(aTargetURI + (aIsServer ? "/Junk" : ""), !aIsServer).server;

> 80 chars

Comment 86

[:aceman]

Attachment: patch v7

Thanks, done.

Comment 87

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 645826 [details] [diff] [review]
patch v7

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

Ok, this looks OK to me too - just one tiny nit. Thanks aceman!

::: mailnews/base/prefs/content/am-junk.js
@@ +7,5 @@
>  Components.utils.import("resource:///modules/iteratorUtils.jsm");
>  
>  const KEY_ISP_DIRECTORY_LIST = "ISPDL";
>  var gPrefBranch = null;
> +var gDeferredToAccount = "";

We prefer let over var.

Comment 88

[:aceman]

Attachment: patch v8

Finally! Launch the fireworks:)

Comment 89

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/8fc474e677e6

Comment 90

[:aceman]

Wayne, can you get any adventurous GS reporters to try out current TB17 nightlies with this fix and verify it?

Comment 91

[Trevor]

2012-008-15 has fixed the issue I reported in bug 668410, which is a duplicate of this one.

Comment 95

[Steve Roylance]

version 17.0 works as it should
FIXED verified

Comment 96

[Wayne Mery (:wsmwk)]

THANKS aceman and everyone for alll this great work!

Just finished merging a bunch of topics into https://getsatisfaction.com/mozilla_messaging/topics/tbird_3_junk_account_settings_not_saved which now has 70 followers. And https://getsatisfaction.com/mozilla_messaging/topics/tbird_3_junk_account_settings_not_saved#reply_10748541  asks for feedback.

Comment 97

[:aceman]

We still need to fix the part in bug 734034 to be 100% happy :)
Without that, in the time between deferring an account and visiting the account settings the junk mail will not be moved.

Comment 98

[:aceman]

Test in bug 810763.