379831 - option to de-activate Account Wizard at start

Status: NEW

(MailNews Core :: Account Manager, enhancement)

Description

[Regis R.]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.3) Gecko/20070309 Firefox/2.0.0.3
Build Identifier: 2.0.0.0

I'd like to have an option to de-activate the launching of Account Wizard
at start of thunderbird when no account is configured.

The reason is that the OS user account I use to update thunderbird and check for updates is an administrative user.

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Karsten Düsterloh]

Attachment: proof of concept: only open account wizard if autoadmin.hide_account_wizard is not true

Do not open the account manager even if no accounts are defined, if autoadmin.hide_account_wizard is true.
This works "fine" for messenger.xul and the addressbook main panel, but will crash on first sight of the compose window - and I'm not sure there's much we can do about it. I also got some JS errors for undefined variables, but fixing those (not part of this poc) doesn't make it not crash...

(I'm not sure if I understand comment #0 correctly or if it's useful at all - I investigated this as part of making the mailnews main windows available to automated testing.)

Comment 2

[Regis R.]

Yes, I think you've understood my comment.

For my personal usage, I've just configured a news account for the administrative user, so there's no more pop-up on start.

Comment 3

[:aceman]

Confirming the enhancement request.
Does this patch still apply to current TB codebase (TB7-8-9)? 

You need to request review for the patch from somebody responsible for this component. See https://wiki.mozilla.org/Modules/Thunderbird .

Comment 4

[:aceman]

Maybe the existing pref of "mail.disable_new_account_addition" (used e.g. in the Account central) would be enough for this. Just the Account Wizard needs to observe it.

I'll try to update the patch. Karsten, is it free to take?

Comment 5

[:aceman]

Attachment: patch updated

Use the pref mail.disable_new_account_addition and check if it is locked. This implementation is not strictly necessary, but it is so to be inline with other spots where this pref is checked.
(see http://mxr.mozilla.org/comm-central/search?string=mail.disable_new_account_addition&tree=comm-central)

One thing to note is that skipping via the Account wizard via the pref will still create the Local Folders account (as server1). Before the patch, the manual canceling of the Wizard did not cause Local Folders to be created. Is it a problem?

Comment 6

[Ian Neal]

Comment on attachment 598304 [details] [diff] [review]
patch updated

>+    var prefillAccount;
>+    var state = true;
Nit: unused variable

>           var adminUrl;
>           try {
>+            adminUrl = Services.prefs.getCharPref("autoadmin.global_config_url");
>           }
>           catch (ex) {}
>           if (!adminUrl)
>             newProfile = false;
These two lines could be removed.
>         }
> 
>+        let openWizard = (newProfile && !accountCount) ||
Just use !adminUrl instead of newProfile here.
>+                         (accountCount == invalidAccounts.length) ||
>+                         prefillAccount ||
>+                         (!gAnyValidIdentity && wizardCallback);
>+        if (openWizard) {
>+          try {
>+            openWizard = !Services.prefs.prefIsLocked("mail.disable_new_account_addition");
>+          }
>+          catch (ex) {}
You shouldn't need a try/catch here which means it could be combined with the rest of the defining of openWizard
>+        }
>+
>+        if (openWizard)
You could now completely remove the defining of openWizard and just move it all into this if.
r- as I would like to see the new patch

Comment 7

[:aceman]

(In reply to Ian Neal from comment #6)
> >           var adminUrl;
> >           try {
> >+            adminUrl = Services.prefs.getCharPref("autoadmin.global_config_url");
> >           }
> >           catch (ex) {}
> >           if (!adminUrl)
> >             newProfile = false;
> These two lines could be removed.
> >         }
> > 
> >+        let openWizard = (newProfile && !accountCount) ||
> Just use !adminUrl instead of newProfile here.
Wouldn't it be just adminUrl (no negation)?

Comment 8

[Ian Neal]

(In reply to :aceman from comment #7)
> (In reply to Ian Neal from comment #6)
> > >           var adminUrl;
> > >           try {
> > >+            adminUrl = Services.prefs.getCharPref("autoadmin.global_config_url");
> > >           }
> > >           catch (ex) {}
> > >           if (!adminUrl)
> > >             newProfile = false;
> > These two lines could be removed.
> > >         }
> > > 
> > >+        let openWizard = (newProfile && !accountCount) ||
> > Just use !adminUrl instead of newProfile here.
> Wouldn't it be just adminUrl (no negation)?
Correct, sorry.

Comment 9

[:aceman]

Attachment: patch v3

Comment 10

[Ian Neal]

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


>+        if (((adminUrl && !accountCount) ||
>+            (accountCount == invalidAccounts.length) ||
>+            prefillAccount ||
>+            (!gAnyValidIdentity && wizardCallback)) &&
Nit: There is space for this is to added to the end of the previous line.
>+            !Services.prefs.prefIsLocked("mail.disable_new_account_addition"))

Comment 11

[:aceman]

That is true, I just let the logic groups to be on separate lines. But maybe it is not important.

Comment 12

[:aceman]

What about the change in behaviour regarding creation of the Local folders?
actually when they are created, after next restart TB does not open the wizard anymore. It would solve the reporters problem if he just let it create the Local Folders and then TB would not annoy him again. However, there was probably no way to let it create the Local folders before.

Comment 13

[:aceman]

I wanted to say that currently this new code only serves a purpose only at first start by skipping the wizard and creating Local Folders. After that, the code and pref is not really needed, TB will be happy as 1 account already exists (Local Folders).

Comment 14

[:aceman]

Attachment: patch v3

Comment 15

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

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

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

Hey aceman - just a drive-by review here.

::: mailnews/base/prefs/content/accountUtils.js
@@ +113,5 @@
>   *                   doesn't take any arguments.
>   */
>  function verifyAccounts(wizardCallback, needsIdentity, wizardOpen)
>  {
> +    var prefillAccount;

We prefer let over var now.

@@ +136,5 @@
>  
>          // if there are no accounts, or all accounts are "invalid"
>          // then kick off the account migration. Or if this is a new (to Mozilla) profile.
>          // MCD can set up accounts without the profile being used yet
> +        var adminUrl = null;

We prefer let over var now.

@@ +149,2 @@
>  
>          // openWizard is true if messenger migration returns some kind of

Is this comment still true?  Or does it need to be updated?

@@ +157,5 @@
>          // compose window. This last condition is so that we open the account
>          // wizard if the user does not have any identities defined and tries to
>          // compose mail.
>  
> +        if (((adminUrl && !accountCount) || (accountCount == invalidAccounts.length) ||

The log in here is really convoluted.  At the very least, we should assign each condition to a descriptive alias, like:

let noAccountsAndMCDConfigured = adminUrl && !accountCount;
let allAccountsInvalid = accountCount == invalidAccounts.length;
let noValidIdentity = (!gAnyValidIdentity && wizardCallback)) &&
                      Services.prefs.prefIsLocked("mail.disable_new_account_addition");

And then our conditional becomes much more readable, with:

if (noAccountsAndMCDConfigured || allAccountsInvalid || prefillAccount || noValidIdentity) {
  // ...
}

Comment 16

[:aceman]

Attachment: patch v5

What about this?

Comment 17

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

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

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

aceman:

This looks good to me.  Just a few nits that I noticed from a quick glance.

-Mike

::: mailnews/base/prefs/content/accountUtils.js
@@ +83,5 @@
>  
>    try {
>      var shellService = Components.classes["@mozilla.org/suite/shell-service;1"]
>                                   .getService(nsIShellService);
> +    // XXX where is the defaultAccount variable used?

Good question.  If the answer is "nowhere", feel free to excise it.

@@ +131,1 @@
>          if (invalidAccounts.length > 0 && invalidAccounts.length == accountCount) {

We don't need braces for the if-block if it's only a one-liner.

@@ +148,2 @@
>  
> +        // * noAccountsAndMCDConfigured is true if messenger migration returns

To make this bulleted list easier to read, can you indent the lines following the bullet so that they're two spaces inwards from the bullet?

Example

// * This is some bullet text and notice
//   how this line is indented so that the
//   "h" falls beneath the "T" in "This"?

@@ +148,3 @@
>  
> +        // * noAccountsAndMCDConfigured is true if messenger migration returns
> +        // some kind of error (including those cases where there is nothing to migrate).

I think the comment would be clearer as:

"noAccountsAndMCDConfigured is true if there was no profile to migrate, or if the migration failed.

@@ +148,5 @@
>  
> +        // * noAccountsAndMCDConfigured is true if messenger migration returns
> +        // some kind of error (including those cases where there is nothing to migrate).
> +        let noAccountsAndMCDConfigured = (!accountCount && newProfile) ||
> +                                         (accountCount == invalidAccounts.length);

Since we're still within the first parentheses, this line should be indented another space.

@@ +155,3 @@
>          // identity. Since local and RSS folders are identity-less accounts, if you
>          // only have one of those, it will be false.
> +        // * needsIdentity is true only when verifyAccounts is called from the

It's strange to comment about needsIdentity here, actually, since it's a parameter being passed to this function.  If anything, that sort of documentation should be part of the function documentation.

@@ +162,4 @@
>  
> +        // * prefillAccount is non-null if there is at least one invalid account.
> +        if ((noAccountsAndMCDConfigured || noValidIdentity || prefillAccount) &&
> +          !Services.prefs.prefIsLocked("mail.disable_new_account_addition"))

Since we're still inside the first parenthesis, this line should be indented such that the ! is lined up beneath the second (.

Comment 18

[:aceman]

(In reply to Mike Conley (:mconley) from comment #17)
> > +        let noAccountsAndMCDConfigured = (!accountCount && newProfile) ||
> > +                                         (accountCount == invalidAccounts.length);
> 
> Since we're still within the first parentheses, this line should be indented
> another space.

I don't see where we are within parentheses here.

> 
> @@ +155,3 @@
> >          // identity. Since local and RSS folders are identity-less accounts, if you
> >          // only have one of those, it will be false.
> > +        // * needsIdentity is true only when verifyAccounts is called from the
> 
> It's strange to comment about needsIdentity here, actually, since it's a
> parameter being passed to this function.  If anything, that sort of
> documentation should be part of the function documentation.
That is true. There is already the same comment in the function description. So I'll remove it here.

Comment 19

[:aceman]

I should get back to this...

Comment 20

[:aceman]

Attachment: patch v6

Comment 21

[Ian Neal]

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


>+++ b/mailnews/base/prefs/content/accountUtils.js

> Components.utils.import("resource://gre/modules/Services.jsm");
>+Components.utils.import("resource:///modules/mailServices.js");
There probably needs to be an audit of where Services.jsm and mailServices.js are imported as we seem to be doing it just in case rather than checking it is not already in the context.
Is the conversion of the rest of the file to Services.jsm another bug?

> function verifyAccounts(wizardCallback, needsIdentity, wizardOpen)
> {
>-  var openWizard = false;
>-  var prefillAccount;
>-  var state=true;
>-  var ret = true;
>+    let prefillAccount;
>+    let ret = true;
The majority of functions in this file have 2 rather than 4 space indentation, so I would look at moving to that here rather than 4.
> 
>     try {
>         // migrate quoting preferences from global to per account. This function returns
>         // true if it had to migrate, which we will use to mean this is a just migrated
>         // or new profile
>+        let newProfile = migrateGlobalQuotingPrefs(MailServices.accounts.allIdentities);
> 
>+        let accounts = MailServices.accounts.accounts;
> 
>         // as long as we have some accounts, we're fine.
>+        let accountCount = accounts.Count();
>+        let invalidAccounts = getInvalidAccounts(accounts);
>+        if (invalidAccounts.length > 0 && invalidAccounts.length == accountCount)
>+          prefillAccount = invalidAccounts[0];
> 
>         // if there are no accounts, or all accounts are "invalid"
>         // then kick off the account migration. Or if this is a new (to Mozilla) profile.
>         // MCD can set up accounts without the profile being used yet
>         if (newProfile) {
>           // check if MCD is configured. If not, say this is not a new profile
>           // so that we don't accidentally remigrate non MCD profiles.
>+          let adminUrl = null;
>           try {
>+            adminUrl = Services.prefs.getCharPref("autoadmin.global_config_url");
>           }
>           catch (ex) {}
>           if (!adminUrl)
>             newProfile = false;
Rather than defining adminUrl, can't we just do:
try {
  if (!Services.prefs.getCharPref("autoadmin.global_config_url"))
    newProfile = false;
}
catch(ex) {}

>         }
> 
>+        // noAccountsAndMCDConfigured is true if there was no profile to
>+        // migrate, or if the migration failed.
>+        let noAccountsAndMCDConfigured = (!accountCount && newProfile) ||
>+                                         (accountCount == invalidAccounts.length);
>+
>+        // gAnyValidIdentity is true when you've got at least one *valid*
>+        // identity. Since local and RSS folders are identity-less accounts,
>+        // if you only have one of those, it will be false.
>+        let noValidIdentity = (!gAnyValidIdentity && needsIdentity);
Nit: () not needed here.

const NS_SHELLSERVICE_CID = "@mozilla.org/suite/shell-service;1";
Does having this as a const add anything or should it just be inlined?

Comment 22

[:aceman]

(In reply to Ian Neal from comment #21)
> >+++ b/mailnews/base/prefs/content/accountUtils.js
> > Components.utils.import("resource://gre/modules/Services.jsm");
> >+Components.utils.import("resource:///modules/mailServices.js");
> There probably needs to be an audit of where Services.jsm and
> mailServices.js are imported as we seem to be doing it just in case rather
> than checking it is not already in the context.
You can propose how to do such an audit in bug 720356. I think these imports should just be included in all toplevel files so that all other js files can use them automatically. Is that even possible?

> Is the conversion of the rest of the file to Services.jsm another bug?
Yes, bug 776705.

Comment 23

[:aceman]

Attachment: patch v7

Fixed Ian's nits.

Comment 24

[Ian Neal]

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

>+      let adminUrl = null;
Nit: You don't use this any more.

Comment 25

[:aceman]

Attachment: patch v8

Thanks. I wonder if I get to v10 here ;)

Comment 26

[Ian Neal]

Comment on attachment 645399 [details] [diff] [review]
patch v8

>+    if (newProfile) {
>+      // check if MCD is configured. If not, say this is not a new profile
>+      // so that we don't accidentally remigrate non MCD profiles.
>+      try {
>+        if (!Services.prefs.getCharPref("autoadmin.global_config_url"))
>+          newProfile = false;
>+      } catch (ex) {}
Just wondering if newProfile should be false too for when the pref does not exist?

We'll get to V10 yet ;)

Comment 27

[:aceman]

catch (ex) {newProfile = false; }  ?

Comment 28

[Ian Neal]

(In reply to :aceman from comment #27)
> catch (ex) {newProfile = false; }  ?

Something like that, yes.

Comment 29

[:aceman]

Attachment: patch v9

Comment 30

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

So I'm pretty happy with this patch, but I think I'm going to ask for a Mozmill test, since we're fiddling with start-up and first-time-experience stuff.

Comment 31

[:aceman]

I understand that. However I can't do it now and I am not interested in this feature (I just unbitrotted the patch) so it is not worth for me to learn the stuff needed for creating a test.

Comment 32

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

Comment on attachment 645817 [details] [diff] [review]
patch v9

Hrm, fair enough.

Removing my r? from the patch, until a test is included.