144562 - Need a way to pre-configure email/news account setup

Status: VERIFIED FIXED

(SeaMonkey :: MailNews: Account Configuration, defect, P1)

Description

[tao]

Requirements 

General requirements
  o pre-configurate and lock server preferences before deploying them.
  o the ability to designate which account/server is the default for email or
    news separately.
  o when a preference is locked, the associated UI in the client should be
    disabled/greyed out.
  o end users should not be able to modify/delete pre-configure settings
  o the solution should work with both new profiles and existing profiles 
    which might have server settings already.
  o there should be preference of whether users are allowed to add new mail/news 
    account. 
  o how do we handle pre-existing server settings in old profiles
    - leave them alone (end users still can use them) but use new server 
      setting as the defaults. 
    - when locked, users won't be allowed to switch the default server.
  o users will get prompted to enter user information (account name, identity,
    etc.) if such information is not available in the user profile.

Comment 1

[Rodney Velasco]

Ninoschka, I should be default QA contact for this bug since this is an
customization feature.  I may need help testing overall account manager
functionality for regressions.  Feel free to reassign it back to yourself if you
feel you should own this.

Comment 2

[racham]

Attachment: patch, v1.0

Comment 3

[racham]

Attachment: sample pre-configured prefs used for testing

Here is the file that contains preconfigured prefs from both all.js and
mn_prefs.txt (a file which contains other preconfigured prefs which are to be
locked).

Comment 4

[racham]

I have requested Rodney & Ninoschka to run their test suites with the optimized
build I have provided them.

Also, I have requested Srilatha and Seth to take a look at the patch and provide
any feedback.

Comment 5

[(not reading, please use [e-mail] instead)]

looks good.  some minor comments / questions:

1)

Please add comments to your code.  If nothing else, just indicating that these
changes add support for pre-configure email/news accounts.
  
The account manager and account wizard code (and mozilla/mailnews in general) is
getting pretty complex.  The more comments we have, the better.

2)

+        catch (ex) {
+        }

and

+  catch (ex) {
     }

Do you expect to get into these catch() blocks, or would getting there be
unexpected?

If unexpected:

catch (ex) {
  dump("foobar failed: " + ex + "\n");
}

If expected:

catch (ex) {
  // we expect to get here in these scenarios...
}

3)

+    var smtpRequiresPrefStr = "mail.identity." + identity.key +
".smtpRequiresUsername";
+    smtpRequiresUsername = gPrefs.getBoolPref(smtpRequiresPrefStr);

Can we use nsIMsgIdentity's getBoolAttribute(in string name);

     smtpRequiresUsername = identity.getBoolAttribute(smtpRequiresUsername);

4) 

+
  // Time to check if there are any pre-configured accounts and if we need
+
  // add them to the list of existing set of accounts

Is there a tab / whitespace issue on that comment?

5)

+    PRBool addedPreConfigAccounts = false;
+    rv = m_prefs->GetBoolPref("mail.preconfigaccounts.added",
&addedPreConfigAccounts);
+
+    if (NS_SUCCEEDED(rv) && !addedPreConfigAccounts) {
+      nsXPIDLCString appendAccountList;
+      rv = m_prefs->CopyCharPref("mail.accountmanager.appendaccounts",
+                                          getter_Copies(appendAccountList));
+
+      if (NS_SUCCEEDED(rv) && appendAccountList.get()) {
+        if (accountList.get()) {
+          accountList += ",";
+          accountList += appendAccountList;
+        }
+        else {
+          accountList = appendAccountList;
+        }
+        rv = m_prefs->SetBoolPref("mail.preconfigaccounts.added", PR_TRUE);
+      }
+    }
   }


a) What happens if the appendedAccountList contains accounts already in account
list?  Or, is that not possible since this code only
gets called when there are now accounts?

b) In the code, how soon after we append this account and set this pref are we
flushing prefs to disk?  Or does account manager code already flush to disk
at the appropriate times?

6)

+      // Time to check if there are any pre-configured smtp servers and if we need
+      // add them to the list of existing set of accounts
+      PRBool addedPreConfigSmtpAccounts = false;
+      rv = prefs->GetBoolPref("mail.preconfigsmtpservers.added",
&addedPreConfigSmtpAccounts);
+
+      if (NS_SUCCEEDED(rv) && !addedPreConfigSmtpAccounts) {
+        nsXPIDLCString appendServerList;
+        rv = prefs->CopyCharPref("mail.smtpservers.appendsmtpservers",
+
				        getter_Copies(appendServerList));
+
+        if (NS_SUCCEEDED(rv) && appendServerList.get()) {
+          if (serverList.get()) {
+            serverList += ",";
+            serverList += appendServerList;
+          }
+          else {
+            serverList = appendServerList;
+          }
+          rv = prefs->SetBoolPref("mail.preconfigsmtpservers.added", PR_TRUE);
+        }
+      }

same as 5....
 
a) What happens if the appendServerList contains accounts already in serverList?
 Or, is that not possible since this code only
gets called when there are now accounts?

b) In the code, how soon after we append this server and set this pref are we
flushing prefs to disk?  Or does compose service code already flush to disk
at the appropriate times?

Comment 6

[Rodney Velasco]

Bhuvan,
I took a first look at your optimized build and I was seeing two issues. 

1) During the first launch of the optimized Mail/News build with no Mozilla
Profiles  or 4.x Migration, the wizard screen to prefill my identity information
appeared as expected.  After prefilling my name and e-mail address and
completing the account wizard I was able to use the mail account with no
problems.  But after I quit the browser and relaunched the mail and newsgroups
application, the Account Wizard kicked off once again and asked me to pre-fill
my identity information again.  

2) If I fill out the first Account Wizard Identity screen traverse to the "Next"
screen, click the "Back" button, it takes me all the way back to the "New
account setup" screen and asks me to to "Select the type of account you would
like to set up: Email Account, Newsgroup account"  
Since you preconfigured the mailserver information in the all.js file we should
never see this screen correct?  If I traverse through the next set of screens I
am able to specify server information (which I didn't see initially when I ran
the wizard the first time) which in turn allows the end user to specify any mail
server not pre configured by the IT admin.  

Comment 7

[racham]

Seth, Thanks for your comments.

* Comments 1, 2, 4 : I will add comments there and remove any unwanted
whitespace chars.
* Comment 3 : Yeah, I can use that attribute to do the same. OK, I will change
accordingly.
* Comment 5, 6 : These just are effective not for just no accounts case. They
are to be executed for no accounts/existing accounts/migrated account cases. As
far as AccountManager accounts concenred, I will add a check to see if that
account already exists and in which case we can prevent adding the samee string
twice. As far smtp servers concerned, there can multiple with the same server
name, given the way it is right now. At present, all these prefs seemed to be
flushed to the prefs file at the appropriate times.

thanks
Bhuvan

Comment 8

[racham]

I just spoke to Rodney on the phone to clarify the issues he has faced. Anyway,
here are the comments related to those issues.

Rodney, 

Thanks for your comments.

Comment 1 : This is happening because in the sample file I have provided, there
are both mail and news pre-configured accounts. So, both are invalid accounts.
Launching mailnews application looks for any existing invalid accounts and
triggers AccountWizard to complete that account again. So, the wizard you have
seen second time is to complete the setup of news account. If you just
pre-configure mail or news accounts, you will not see the wizard on the second
launch. We can enhance it by indicating the account we are trying to setup.

Comment 2 : This is a known bug. I think this is the right time to fix that one
also. 

Thanks again,
Bhuvan

Comment 9

[Jaime Rodriguez, Jr.]

what are the chances this will be resolved by the end of the week? pls update
ETA in the Status Whiteboard. thanks!

Comment 10

[racham]

I have already given Ninoschka an updated version of optimized so that she can
test further.

I will be posting an updated patch (with Seth's comments incorporated) soon.
Setting ETA to the end of this week (6/15).

Comment 11

[racham]

Attachment: patch, v1.1

New patch addressing comments & suggestions from Seth.

Comment 12

[racham]

Attachment: new sample pre-config file

This file has few more new prefs to be part of global prefs file and also some
new prefs to be locked in order to achieve locking without introducing new
prefs.

Comment 13

[David :Bienvenu]

fix tabs here

+
  // Time to check if there are any pre-configured accounts and if we need
+
  // add them to the list of existing set of accounts
+    PRBool addedPreConfigAccounts = false;

you can use PR_Free here:
+          PR_FREEIF(preConfigAccountsStr);

PR_FALSE
+      PRBool addedPreConfigSmtpAccounts = false;

you can use PR_Free here:
+            PR_FREEIF(preConfigSmtpServersStr);


"not alredy there in"
"already"

I'll look some more...

Comment 14

[racham]

Thanks David. Incorporated the above sugegstions in my local tree.

Comment 15

[Srilatha Moturi]

+    PRBool addedPreConfigAccounts = false;
+    rv = m_prefs->GetBoolPref("mail.preconfigaccounts.added", 
&addedPreConfigAccounts);
+
+    // If there are pre-configured accounts, add them to the existing account 
list
+    if (NS_SUCCEEDED(rv) && !addedPreConfigAccounts) {

Once this pref is set to TRUE, you won't be able to add any pre-configured 
accounts. This means that for a profile, the preconfigured the accounts can be 
added only once unless somehow we reset this pref to false.
probably the same goes for the smtp servers too 
"mail.preconfigsmtpservers.added"

Comment 16

[racham]

Working on adding versioning system so that we can avoid that problem..patch 
coming up soon. Thanks.

Comment 17

[racham]

Attachment: patch, v1.2

Added versioning system where ISPs can increase the default version pref if tey
ever have to add additional pre-configured accounts. The version number stored
in user's prefs.js will always be compared with the default version and the
process of appending new accounts/servers will happen based on that.

Comment 18

[Srilatha Moturi]

Initially "mail.append_preconfig_smtpservers.version" does not exist in the 
prefs.js file. So the initial value is 0 and after we add the preconfigured 
accounts it is set to 1. The value of this pref is 1 in the mailnews.js. 
+    // Update the account list if needed
+    if ((appendAccountsCurrentVersion <= appendAccountsDefaultVersion)) {
+
That means, this if condition will be true twice for the same preconfigured 
accounts. You can fix this by changing "<=" to "<". Same goes for the SMTP 
servers too.
other than this one issue, you have an r=srilatha

Comment 19

[racham]

I have just initialized those integers (appendAccountsCurrentVersion,
appendAccountsDefaultVersion) to ZERO. But the first time around, the value
current Version gets is the default one since there is no such pref in the
user's prefs.js.
Then it gets updated to 2 after adding the accounts/servers. So, next time
around the current version is 2 and the default one (in mailnews.js) is 1. So,
we will skip the process.

When the vendor wants to add more pre-configured accounts/servers, he/she will
change the default version (in mailnews.js) to 2 and add all required prefs.
Then (since versions are equal, time to upgrade), we will do the upgrade.

So, the current set up will work as expected to effect the upgrades as needed.
Let me know if I need to address any other issues.

Thanks for the review.

Comment 20

[Srilatha Moturi]

Comment on attachment 88101 [details] [diff] [review]
patch, v1.2

r=srilatha

Comment 21

[David :Bienvenu]

+    // Get the list of smtp servers and create keyed server list
+    if (NS_SUCCEEDED(rv) || appendServerList.get()) {

there are a couple instances of this. Do you mean to be checking if
appendServerList.Length() > 0 ? And the OR check (||) is a bit odd - how could
appendServerList have any data if the call failed?

Comment 22

[racham]

I have used .get() to check if the string has any content. I can use .Length() >
0. I will incorporate that one.

In if (NS_SUCCEEDED(rv) || appendServerList.get()), NS_SUCCEEDED(rv) is used to
see if the call to get mail.smtpservers is succeeded and appendServerList.get()
to see if we have any pre-configured servers. In either case, we want to build
smtpserver list. I can introduce 2 booleans (hasSmtpServers = serverList.Length
> 0 and one for appendserverlist) and then check if either them are true, if
that will make it more clear. Let me know. Thanks.

Comment 23

[David :Bienvenu]

+    nsXPIDLCString appendServerList;
+    prefs->CopyCharPref(PREF_MAIL_SMTPSERVERS_APPEND_SERVERS,
+
				    getter_Copies(appendServerList));
+
+    // Get the list of smtp servers and create keyed server list
+    if (NS_SUCCEEDED(rv) || appendServerList.get()) {

In this code, appendServerList will never have a length > 0 if CopyCharPref call
above failed, right? Because appendServerList  starts off empty, and then we
call CopyCharPref to fill it in. So, I'm claiming that this should just be
NS_SUCCEEDED(rv) - does that make sense or am I missing something? If I'm
missing something, you could add a comment that explains it...

Comment 24

[racham]

Attachment: patch, v1.3

Per AIM conversation with David, used .Length() > 0 and added more comments
where needed.

Comment 25

[racham]

Comment on attachment 88243 [details] [diff] [review]
patch, v1.3 

c/f r=srilatha

Comment 26

[David :Bienvenu]

Comment on attachment 88243 [details] [diff] [review]
patch, v1.3 

sr=bienvenu

Comment 27

[racham]

Fixed on the trunk. Marking Fixed.

Thanks for all the reviews and suggestions. 

Adding required keywords.

Comment 28

[Jaime Rodriguez, Jr.]

rvelasco - can you pls verify this on the trunk, then replace fixed1.0.1 with
verified1.0.1? thanks!

Comment 29

[scottputterman]

I think Jaime meant to say to change this bug to verified once it's been
verified on the trunk.  When it gets landed on the branch, we'll use the 1.0.1
keywords.

Comment 30

[racham]

I will request Ninoschka to run through the AccountManager test suite in detail.

Comment 31

[Ninoschka Baca]

Trunk build 2002-06-19: WinMe
The following tests behaved as expected:

1. 1st Profile
- Added an IMAP, POP, AOL, WebMail and News account
- Edited the Server Settings (i.e. biff to 1 minute, Empty Trash on Exit)
- Edited Copies & Folders so that the Sent, Drafts and Templates pointed to a
non-default folder. Then performed a Save As Draft, Save As Template and sent a
message and all messages appeared in the appropriate folders. 
- Sent and Received messages with all scenarios
- Set the WebMail account as the default
- Removed the WebMail account
- Again in all cases I was able to send/receive messages

2. 2nd Profile
- Activated a WebMail account, then added an IMAP, POP and AOL account
- Edited the Server Settings (i.e. biff to 1 minute, Empty Trash on Exit)
- Edited Copies & Folders so that the Sent, Drafts and Templates pointed to a
non-default folder. Then performed a Save As Draft, Save As Template and sent a
message and all messages appeared in the appropriate folders. 
- Sent and Received messages with all scenarios
- Set the AOL account as the default
- Removed all accounts except for the WebMail account
- Then added an IMAP account
- In all cases I was able to send/receive messages.

Comment 32

[Rodney Velasco]

Account pre-configuration works as desinged using todays trunk builds on all
platforms.  

Since bug 144563 is somewhat related to this bug I tested it two ways...

1. Preconfigured account with no locking as defined in bug 144563: Success
2. Preconfigured account with locking as defined in bug 144563: Success

Tested this on todays (2002061908) trunk builds.
Mac OS X.1.3 2002061908
Mac OS 9.2.2 2002061908
Linux 2.4.7-10 2002061908
Windows NT 4.0 2002061908

Marking this bug verified on trunk.

Comment 33

[Jaime Rodriguez, Jr.]

adt1.0.1 (on ADT's behalf) approval for checkin to the 1.0 branch, pending
drivers approval. pls check this in asap, then add the "fixed1.0.1" keyword.

Comment 34

[Judson Valeski]

please checkin to the 1.0.1 branch. once there, remove the "mozilla1.0.1+"
keyword and add the "fixed1.0.1" keyword.

Comment 35

[Rodney Velasco]

over to jimmyu for verification on the branch once it lands.

Comment 36

[racham]

Fix checked in on the branch. Adding fixed1.0.1 keyword.

Comment 37

[Jaime Rodriguez, Jr.]

jimmy - pls verify this fixed on the 1.0 branch.

Comment 38

[Jimmy Uen]

Verified on 1.0 branch
Mac OS9 20020622
Linux 20020622
Windows 20020622

1.0 branch builds for Mac OSX are not ready. Once it's available will verify
then mark this bug accordingly.

Comment 39

[Arthur]

Is this feature documented somewhere? Where do I have to put the pre-config file
so it gets read during install?

Comment 40

[Jimmy Uen]

Marking verified1.0.1

MacOSX 20020624

Comment 41

[[e-mail] (away - not reading bugmail)]

Attachment: vendor config template based on racham's sample file