Last Comment Bug 880464 - Setting up an RSS account first should not make it the default account
: Setting up an RSS account first should not make it the default account
Product: MailNews Core
Classification: Components
Component: Account Manager (show other bugs)
: Trunk
: All All
-- minor (vote)
: Thunderbird 25.0
Assigned To: rsx11m
Depends on:
Blocks: 880602 881114 437139
  Show dependency treegraph
Reported: 2013-06-06 14:52 PDT by rsx11m
Modified: 2013-08-06 14:33 PDT (History)
7 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---

Proposed fix (1.24 KB, patch)
2013-06-06 15:08 PDT, rsx11m
no flags Details | Diff | Splinter Review
Proposed fix (v2) (2.48 KB, patch)
2013-06-07 08:50 PDT, rsx11m
bwinton: review+
iann_bugzilla: review+
standard8: approval‑comm‑aurora+
Details | Diff | Splinter Review
Patch for comm-beta (1.28 KB, patch)
2013-07-23 06:32 PDT, rsx11m
standard8: approval‑comm‑beta+
Details | Diff | Splinter Review

Description User image rsx11m 2013-06-06 14:52:07 PDT
(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


 4. Should be mail.accountmanager.defaultaccount = account1 (local)
 5. prefs shouldn't be set as they are not applicable for RSS accounts
Comment 1 User image rsx11m 2013-06-06 15:08:54 PDT
Created attachment 759438 [details] [diff] [review]
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 User image :aceman 2013-06-06 15:19:01 PDT
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 User image rsx11m 2013-06-06 15:30:20 PDT
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 User image rsx11m 2013-06-06 15:35:01 PDT
(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 User image :aceman 2013-06-07 00:04:56 PDT
OK, thanks. As long as the account wizard is used for new accounts, this could work.
Comment 6 User image rsx11m 2013-06-07 05:24:11 PDT
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 User image rsx11m 2013-06-07 08:50:01 PDT
Created attachment 759766 [details] [diff] [review]
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 User image Ian Neal 2013-06-17 17:02:37 PDT
Comment on attachment 759766 [details] [diff] [review]
Proposed fix (v2)

Seems to do what it says on the tin r=me
Comment 9 User image rsx11m 2013-07-08 06:32:24 PDT
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 User image Blake Winton (:bwinton) (:☕️) 2013-07-08 07:39:44 PDT
Comment on attachment 759766 [details] [diff] [review]
Proposed fix (v2)

Yeah, this also makes sense to me.  r=me.
Comment 11 User image rsx11m 2013-07-08 07:45:26 PDT
Thanks, push for trunk please.
Comment 12 User image rsx11m 2013-07-08 07:57:58 PDT
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 User image Ryan VanderMeulen [:RyanVM] 2013-07-11 18:53:59 PDT
Comment 14 User image Mark Banner (:standard8) 2013-07-23 04:33:51 PDT
Comment 15 User image rsx11m 2013-07-23 06:27:07 PDT
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 User image rsx11m 2013-07-23 06:32:49 PDT
Created attachment 779733 [details] [diff] [review]
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,
Comment 17 User image 2013-07-24 04:48:41 PDT
Actually it seems that I don't have Bugzilla permission to approve patches.
Comment 18 User image :aceman 2013-07-24 05:02:48 PDT
OK, then just say you approve the patch and standard8 comes around to decide the approval and finds your comment ;)
Comment 19 User image rsx11m 2013-07-24 06:35:13 PDT
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 User image Justin Wood (:Callek) [away until Feb 27] 2013-07-29 20:06:50 PDT
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...
Comment 21 User image rsx11m 2013-07-29 20:12:35 PDT
Heh, these approval flags are really picky who's flipping them... ;-)
Thanks, and I'll leave the request up for Mark to retrospectively approve.

Note You need to log in before you can comment on or make changes to this bug.