The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Thunderbird 25.0

Status

MailNews Core
Account Manager
--
minor
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: rsx11m, Assigned: rsx11m)

Tracking

(Blocks: 2 bugs)

Trunk
Thunderbird 25.0
Dependency tree / graph

SeaMonkey Tracking Flags

(thunderbird23 wontfix, thunderbird24 fixed, thunderbird25 fixed, seamonkey2.20 fixed, seamonkey2.21 fixed, seamonkey2.22 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

4 years ago
(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
(Assignee)

Comment 1

4 years ago
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.
Attachment #759438 - Flags: review?(iann_bugzilla)
Attachment #759438 - Flags: review?(bwinton)

Comment 2

4 years ago
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?
(Assignee)

Comment 3

4 years ago
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.
(Assignee)

Comment 4

4 years ago
(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.

Updated

4 years ago
Blocks: 854098

Comment 5

4 years ago
OK, thanks. As long as the account wizard is used for new accounts, this could work.
(Assignee)

Comment 6

4 years ago
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.
(Assignee)

Comment 7

4 years ago
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.
Attachment #759438 - Attachment is obsolete: true
Attachment #759438 - Flags: review?(iann_bugzilla)
Attachment #759438 - Flags: review?(bwinton)
Attachment #759766 - Flags: review?(iann_bugzilla)
Attachment #759766 - Flags: review?(bwinton)
(Assignee)

Updated

4 years ago
Blocks: 437139
(Assignee)

Updated

4 years ago
Blocks: 880602
(Assignee)

Updated

4 years ago
No longer blocks: 854098
(Assignee)

Updated

4 years ago
Blocks: 881114

Comment 8

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

Seems to do what it says on the tin r=me
Attachment #759766 - Flags: review?(iann_bugzilla) → review+
(Assignee)

Comment 9

4 years ago
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 on attachment 759766 [details] [diff] [review]
Proposed fix (v2)

Yeah, this also makes sense to me.  r=me.
Attachment #759766 - Flags: review?(bwinton) → review+
(Assignee)

Comment 11

4 years ago
Thanks, push for trunk please.
Keywords: checkin-needed
(Assignee)

Comment 12

4 years ago
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
Attachment #759766 - Flags: approval-comm-beta?
Attachment #759766 - Flags: approval-comm-aurora?
(Assignee)

Updated

4 years ago
status-seamonkey2.20: --- → affected
status-seamonkey2.21: --- → affected
status-seamonkey2.22: --- → affected
status-thunderbird23: --- → affected
status-thunderbird24: --- → affected
status-thunderbird25: --- → affected
https://hg.mozilla.org/comm-central/rev/389cfc3d5e9f
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 25.0
Attachment #759766 - Flags: approval-comm-aurora? → approval-comm-aurora+
https://hg.mozilla.org/releases/comm-aurora/rev/891f6e1fd7dc
status-thunderbird23: affected → wontfix
status-thunderbird24: affected → fixed
status-thunderbird25: affected → fixed
(Assignee)

Comment 15

4 years ago
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.
status-seamonkey2.21: affected → fixed
status-seamonkey2.22: affected → fixed
(Assignee)

Updated

4 years ago
Attachment #759766 - Flags: approval-comm-beta?
(Assignee)

Comment 16

4 years ago
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, http://logbot.glob.com.au/?c=mozilla%23seamonkey&s=23+Jul+2013&e=23+Jul+2013#c593220
Attachment #779733 - Flags: approval-comm-beta?
Flags: needinfo?(neil)
Actually it seems that I don't have Bugzilla permission to approve patches.
Flags: needinfo?(neil)

Comment 18

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

Comment 19

4 years ago
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.
Flags: needinfo?(mbanner)
Flags: needinfo?(bugspam.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
Flags: needinfo?(bugspam.Callek)
(Assignee)

Comment 21

4 years ago
Heh, these approval flags are really picky who's flipping them... ;-)
Thanks, and I'll leave the request up for Mark to retrospectively approve.
status-seamonkey2.20: affected → fixed
Attachment #779733 - Flags: approval-comm-beta? → approval-comm-beta+
Flags: needinfo?(mbanner)
You need to log in before you can comment on or make changes to this bug.