880464 - Setting up an RSS account first should not make it the default account

Status: RESOLVED FIXED

(MailNews Core :: Account Manager, defect)

Description

[rsx11m]

(Quoting rsx11m from bug 437139 comment #21)
> So, I've looked further into the default-account issue and found that the
> tests have to be a bit more extensive than just checking if it exists. As it
> turns out, also non-mail accounts (like the RSS account) may end up being
> set as the default account. I've managed to do this by cancelling the
> initial account setup first, then creating the RSS account, and only after
> that creating the e-mail account.

This works for news accounts (in which case Local Folders becomes the default account) but not for the feeds account. Neither a news or a feeds account should be permitted to be defined as the default account; only imap, pop3, or movemail.

Steps to reproduce:

 1. Start with a new profile
 2. Set up an RSS (Blogs & News Feeds) account
 3. Check its account key (should be "account2")
 4. See that mail.accountmanager.defaultaccount = account2 (feeds acct.)
 5. Also, mail.server.server2.{download_on_biff,login_at_startup} = true

Expected:

 4. Should be mail.accountmanager.defaultaccount = account1 (local)
 5. prefs shouldn't be set as they are not applicable for RSS accounts

Comment 1

[rsx11m]

Attachment: Proposed fix

The problem is that EnableCheckMailAtStartUpIfNeeded() is called in FinishAccount() based on serverIsNntp() as an explicit check whether or not it is a news server, thus also being triggered for RSS accounts.

I'm changing this to a check for the canBeDefaultServer attribute of the incoming server instead, which should be true for mail accounts (i.e., imap, pop3, and movemail) but not for any other account type (specifically not nntp or rss).

Tested with all five account types in varying order.

Comment 2

[:aceman]

Local Folders also is not a good account to be set as default.

See also bug 342632 and bug 875675. Fixing bug 342632 is a long term problem.

But if you want I think I can make it like this:
1. if only Local Account exists, it is reported as default.
2. if a rss/news account is created, still Local Accounts is the default (the first account)
3. if a pop3/imap4 account is created now TB will automatically switch to it as the default account.
4. if any new pop3/imap4 accounts are created after that, the account from step 3. is still the default.

I have an idea how to do at least this much without extensive changes at all callers.

Would this work for you?

Comment 3

[rsx11m]

This is essentially what my patch does with a 1-line change, isn't it?

Meaning, if no default account exists and the first account created is one which cannot be set as a default account (i.e., canBeDefaultServer==false), EnableCheckMailAtStartUpIfNeeded() isn't called and thus no default server is set, hence Local Folders becomes the default account implicitly (which is not ideal but works until bug 342632 is fixed).

Procedures for mail accounts vary between Thunderbird and SeaMonkey. I'm making adjustments to the default-account handling for the auto-config part in bug 437139, and SeaMonkey still uses AccountWizard.js for it, which is covered (I've tested the change both against Thunderbird and SeaMonkey to make sure that everything behaves as intended).

Thus, unless I've missed something, my patch here should be everything that's needed (along with the changes in bug 437139) to make this work properly for both applications.

Comment 4

[rsx11m]

(In reply to :aceman from comment #2)
> 3. if a pop3/imap4 account is created now TB will automatically switch to it
> as the default account.

That's the part which currently doesn't work with Thunderbird's auto-config but which I'm fixing in the other bug. If you want to test, you'll have to apply both patches.

Comment 5

[:aceman]

OK, thanks. As long as the account wizard is used for new accounts, this could work.

Comment 6

[rsx11m]

The canBeDefaultServer attribute was introduced with bug 66460, where also the code in question came from hard-wiring not setting the default server for NNTP accounts. Thus, it was no longer sufficient after bug 225158 added RSS support.

Consequently, the change here to generalize that test should be made either way, regardless of fixing/improving the default account in the other bugs.

Comment 7

[rsx11m]

Attachment: Proposed fix (v2)

I'm moving hunk #2 of attachment 758719 [details] [diff] [review] from bug 437139 over here as it's basically fixing the same issue for the auto-config code (in addition item #3 of aceman's list in comment #2), thus it was somewhat misplaced there anyway.

For createInBackend.js, the inServer.canBeDefaultServer term corresponds to the check introduced for AccountWizard.js before. The two other terms correspond to the intial check if a default account is already set in EnableCheckMailAtStartUpIfNeeded().

Ian, from SeaMonkey's perspective this is equivalent to attachment 759438 [details] [diff] [review] given that accountcreation/* is NPOTB.

Comment 8

[Ian Neal]

Comment on attachment 759766 [details] [diff] [review]
Proposed fix (v2)

Seems to do what it says on the tin r=me

Comment 9

[rsx11m]

Blake, ping for review. As it's a regression fix, it would be nice to also give this some exposure on the beta channel.

Comment 10

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

Comment on attachment 759766 [details] [diff] [review]
Proposed fix (v2)

Yeah, this also makes sense to me.  r=me.

Comment 11

[rsx11m]

Thanks, push for trunk please.

Comment 12

[rsx11m]

Comment on attachment 759766 [details] [diff] [review]
Proposed fix (v2)

Given that comm-central is still closed due to bustages, I'm requesting the approvals in parallel.

[Approval Request Comment]
Regression caused by (bug #): bug 225158 (comment #6); bug 422814 (TB part only)
User impact if declined: Creating an RSS account before creating a mail account fails to properly register that account as the default account, which in turn is used in various context.

Testing completed (on c-c, etc.): Tested on 24.0 branch.

Risk to taking this patch (and alternatives if risky): low

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/389cfc3d5e9f

Comment 14

[Mark Banner (:standard8)]

https://hg.mozilla.org/releases/comm-aurora/rev/891f6e1fd7dc

Comment 15

[rsx11m]

Mark, thanks for checking this in for comm-aurora. Since you've wontfixed the patch for status-thunderbird23 but left the comm-beta request pending, I assume that Thunderbird won't have another 23.0 beta and that it's up to SeaMonkey whether or not to take it on comm-beta for 2.20.

Comment 16

[rsx11m]

Attachment: Patch for comm-beta

[Approval Request Comment] (see comment #12)

This version against comm-beta has the Thunderbird-specific hunk removed.
Neil, pinging you for approval per Ian's suggestion in the meeting this morning, http://logbot.glob.com.au/?c=mozilla%23seamonkey&s=23+Jul+2013&e=23+Jul+2013#c593220

Comment 17

[[e-mail]]

Actually it seems that I don't have Bugzilla permission to approve patches.

Comment 18

[:aceman]

OK, then just say you approve the patch and standard8 comes around to decide the approval and finds your comment ;)

Comment 19

[rsx11m]

Hmm, that's turning out to be more complicated than imagined...  :-)

We have a few days before 2.20 beta 3 is due, thus if either of the friendly people with sufficient privileges could take care of it I'd appreciate it.

Comment #17 indeed sounds like Neil would have approved it if he could do so, as empowered by Ian per the IRC discussion referred to above.

Comment 20

[Justin Wood (:Callek)]

So I also can't flip approval+ in mailnews core components (mental note, nag Standard8 about this some more, since after TB first-beta its always just SeaMonkey anyway)

However I'm checking in anyway since we're about to spin our final beta this cycle...

https://hg.mozilla.org/releases/comm-beta/rev/3b2907dc7e10

Comment 21

[rsx11m]

Heh, these approval flags are really picky who's flipping them... ;-)
Thanks, and I'll leave the request up for Mark to retrospectively approve.