1743312 - Creating a new profile with a news account doesn't work

Status: RESOLVED FIXED

(MailNews Core :: Networking: NNTP, defect)

Description

[newsfan]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/96.0.4664.45 Safari/537.36

Steps to reproduce:

Start TB with -p and create a new profile. Skip setting up an account and then set up a news account instead. Enter all the details, press Finish.

Actual results:

Nothing.

Expected results:

The news account should have been set up and shown in the folder pane ready to subscribe to news groups. The account was set up if you look in the A/M. In a debug version, there are a few errors coming out of the A/M.

This works with the old C++ news module.

Other observations: To debug anything in news is severely hampered by bug 1737485. I came here from bug 1743264 which is very hard to reproduce but also happens with the new news JS module in TB 95 beta. But it's mostly a view issue, not an NNTP protocol issue.

Comment 1

[Ping Chen [:rnons] (away)]

Attachment: Bug 1743312 - Fix creating a newsgroup account as the first account. r=mkmelin

• Emit account-created event to init folderPane
• Use the same handler for menus and account central button

Comment 2

[Ping Chen [:rnons] (away)]

This works with the old C++ news module.

In my testing, it's the same with mailnews.nntp.jsmodule set to false. The problem is the folder pane is not initialized when creating newsgroup as the first account.

Comment 3

[alta88]

Never done: https://bugzilla.mozilla.org/show_bug.cgi?id=942615#c29

Comment 4

[newsfan]

(In reply to Ping Chen (:rnons) from comment #2)

In my testing, it's the same with mailnews.nntp.jsmodule set to false. The problem is the folder pane is not initialized when creating newsgroup as the first account.

You are correct. In the sequence 1) Start TB on new profile 2) switch pref 3) close TB 4) start TB again 5) create news account, you run into the same problem. We erroneously did this: 1) Start TB on new profile 2) create news account --> Nothing 3) switch pref and restart 4) create news account with success.

Sorry and thank you for taking care of the issue.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/0a8719bf592c
Fix creating a newsgroup account as the first account. r=mkmelin

Comment 6

[alta88]

// The following if..else block is the same as in feedAccountWizard.js.

But that's not true, is it. You haven't copied it exactly, and the results will not be the same, ie the code fails to always post the account-created notification, which is wrong.

Comment 7

[Ping Chen [:rnons] (away)]

Attachment: Bug 1743312 - Emit account-created for every newly created feed/news account. r=mkmelin

The account-created event is used to

• Set app.use_without_mail_account
• Init or expand the folder pane

Comment 8

[Ping Chen [:rnons] (away)]

app.use_without_mail_account confused me a lot, in my opinion, what we really need is a pref to control whether to show accountsetup page on start. As long as a user manually closes the accountsetup page, or there are any existing accounts, we should not show accountsetup page on start.

The following are two places I think app.use_without_mail_account is unnecessarily used

1. we "load the Mail UI" without considering MailServices.accounts.accounts first https://searchfox.org/comm-central/rev/e875e53981dfc4795d9d689bcf476b6437438b90/mail/base/content/msgMail3PaneWindow.js#727
2. we check both the pref and accounts https://searchfox.org/comm-central/rev/e875e53981dfc4795d9d689bcf476b6437438b90/mail/base/content/folderDisplay.js#1695-1696

In my opinion, MailServices.accounts.accounts is much more reliable than app.use_without_mail_account, we should use MailServices.accounts.accounts as the single source of truth.

Comment 9

[alta88]

Nope. What you've now done is emit the notification where it isn't necessary. Do not change the feedAccountWizard.js code. It's clear you haven't tried every single permutation and are just throwing stuff in - don't do that.

Also, don't ask for review from someone who is just going to rubber stamp it without looking. I wrote the feeds thing, so ask me.

You are very correct in realizing app.use_without_mail_account is just wrong.

Comment 10

[Ping Chen [:rnons] (away)]

(In reply to alta88 from comment #9)

Nope. What you've now done is emit the notification where it isn't necessary. Do not change the feedAccountWizard.js code. It's clear you haven't tried every single permutation and are just throwing stuff in - don't do that.

You're right it's unnecessary on the account settings page. Seems there is another code path to init/expand the folderpane from the account settings page, can you point it to me, thanks.

Comment 11

[Ping Chen [:rnons] (away)]

To me an extra event doesn't matter, actually I think it's more consistent to always trigger it every time a new account is created, no matter from account central, menus or account settings. Please let me know if there are other problems.

Comment 12

[Alessandro Castellani [:aleca]]

Thanks for the Cc on this.

(In reply to alta88 from comment #9)

Also, don't ask for review from someone who is just going to rubber stamp it without looking. I wrote the feeds thing, so ask me.

I'm sorry but this is not correct, and also, as usual, not very polite.
You're not a module owner nor a core developer, so asking a review to one of the core devs is correct.
Your input and help are always appreciated, but please be respectful of everyone's work and always keep a positive attitude.

You are very correct in realizing app.use_without_mail_account is just wrong.
app.use_without_mail_account confused me a lot, in my opinion, what we really need is a pref to control whether to show accountsetup page on start. As long as a user manually closes the accountsetup page, or there are any existing accounts, we should not show accountsetup page on start.

This is not correct!
Users can use TB as a simple calendar or address book application without setting up any account.
That pref is necessary to allow users using it in that way, we had requests for this feature, and we need to keep it.

If the user doesn't have any account currently configured, we prompt the email setup page, but this is indeed not super correct as we should return a more generic setup hub allowing the user to setup whatever they want, without forcing the email account form. That's planned for later on before the next ESR.

The app.use_without_mail_account pref is set to TRUE only if the user closes the Email Setup tab by clicking on the CANCEL button and confirming the exit dialog with the checkbox checked. I still need to improve that tab a bit in order to trigger that confirmation dialog when the tab is closed with the X or the shortcut.

You're right it's unnecessary on the account settings page. Seems there is another code path to init/expand the folderpane from the account settings page, can you point it to me, thanks.

Please, don't trigger the init of the folder pane from another location. I spent a lot of time trying to unify the messy startup code we have, which is not great yet, but at least with those postMessage(), we have a single source of truth to trigger the initialization of the UI after an account has been created.

we "load the Mail UI" without considering MailServices.accounts.accounts first https://searchfox.org/comm-central/rev/e875e53981dfc4795d9d689bcf476b6437438b90/mail/base/content/msgMail3PaneWindow.js#727

That's not true.
In this portion of code we check if the user has changed that pref to TRUE, and if it did, we don't load the Mail UI at all since that pref remains TRUE unless the user creates a proper account.
If that pref is FALSE, we run the verifyExistingAccounts() https://searchfox.org/comm-central/rev/e875e53981dfc4795d9d689bcf476b6437438b90/mail/base/content/msgMail3PaneWindow.js#798

we check both the pref and accounts https://searchfox.org/comm-central/rev/e875e53981dfc4795d9d689bcf476b6437438b90/mail/base/content/folderDisplay.js#1695-1696

We do that because we use the gFolderDisplay.folderPaneVisible as condition many times across the application, which means we don't need to check for existing accounts if the user specifically set that pref to TRUE.

You wrote in the newly uploaded patch:

The account-created event is used to
Set app.use_without_mail_account
Init or expand the folder pane

This is not entirely correct as that event resets the app.use_without_mail_account to FALSE, since a new account was created, we make sure that pref is reset so we avoid loading problems.

Happy to help further in identifying potential issues or misunderstanding.
Apologies if my code is not super clear.

Comment 13

[Ping Chen [:rnons] (away)]

I think we all agree a pref is needed to not keep showing account setup page on start. I'm still not clear why MailServices.accounts.accounts is not good enough in other places though. Is getting MailServices.accounts.accounts much more expensive than getting a pref value?

Comment 14

[Alessandro Castellani [:aleca]]

(In reply to Ping Chen (:rnons) from comment #13)

I think we all agree a pref is needed to not keep showing account setup page on start. I'm still not clear why MailServices.accounts.accounts is not good enough in other places though. Is getting MailServices.accounts.accounts much more expensive than getting a pref value?

I don't know how expensive calling MailServices.accounts.accounts is, but yeah, I added that pref as condition because who cares how many accounts we have if the user specifically doesn't want to use TB with any account, and due to how the code works, as soon as an account is created that pref is cleared, unless the user goes into about:config to manually change it.

I'm not oppose to removing that pref as condition if not needed.
This whole workflow definitely needs better test coverage.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/94db4e415bae
Emit account-created for every newly created feed/news account. r=mkmelin

Comment 16

[Magnus Melin [:mkmelin]]

Was 91 really affected?

Comment 17

[Ping Chen [:rnons] (away)]

91 is affected, but at least works after restart. It's difficult to apply the patches without uplifting bug 1727304 first.

Comment 18

[Magnus Melin [:mkmelin]]

Ok, I think let's wontfix 91 then and look ahead.