86442 - Ldap Directory server got switched back to original one by itself

Status: VERIFIED FIXED

(MailNews Core :: LDAP Integration, defect, P2)

Description

[yulian chang]

2001061506 builds on all platforms

Directory server got switched back to original chosen server by itself after
editing.

Steps to repro:

1. Bring up Addressing Prefs. window 
2. remember the chosen Directory server from before in Address Autocompletion 
    (For example: A)
3. change to Server B
4. Click "Edit Directories"" button to bring up LDAP Directory Servers window
5. select any server and click Edit
6. click OK to close out "Directory Server Properties" window
7. click OK to close out "LDAP Directory Servers" window
8. the selection of server form step 3 (B) switch back to the server from step 2
(A) by itself

Comment 1

[Ryan Cassin]

Reassigning to a valid e-mail address.

Comment 2

[Srilatha Moturi]

Attachment: patch v1

Comment 3

[Srilatha Moturi]

The patch fixes bugs 86442, 88620, 87367, 88629, 88247, 88450

Comment 4

[Mohan Bhamidipati]

Patch looks fine to me; r=mohanb; 

Comment 5

[Srilatha Moturi]

ccins seth for sr=

Comment 6

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

It looks like you are going through prefs directly to get the per identity prefs.

that's wrong.  you should be going through the account manager.  

you should be getting the account manager, using the allIdentities attribute to
get all the identities, and then on each element of the array you get back, QI
to nsIMsgIdentity and then use the appropriate attribute of nsIMsgIdentity.

  attribute string directoryServer;
  attribute boolean overrideGlobalPref;
 

this existing line of XUL makes me nervous:

prefstring="mail.identity.%identitykey%.overrideGlobal_Pref"/>

if there are other places in your XUL & JS where you are going to prefs directly
and not going through the account manager, you need to fix them.

Comment 7

[Srilatha Moturi]

Attachment: patch v2

Comment 8

[Srilatha Moturi]

with the latest patch, reading the identity prefs will be done through account 
manager.

prefstring="mail.identity.%identitykey%.overrideGlobal_Pref"/>
This attribute is aded so that this preference can be lockable.
Eddyk is working on this. Eddy can you comment on this.
Thanks

Comment 9

[eddyk]

Both this panel and the offline-disk space panel avoid using the AccountManager.
 They got sr'd, so I thought while not optimal, it was an acceptable way of
doing things.

Regarding the prefstring attribute in the tag, I added some functionality a
while back where the AM uses the prefstring to disable xul elements.  It does
not (yet) load or save based on the prefstring.  This is a goal for the future,
which I have not completed.
Should either a code comment or documentation on this state of affairs be put
somewhere?

Comment 10

[eddyk]

Sorry brain fart.  The comment I made about AM usage was incorrect.  This panel
is in fact using AM.

To reiterate the prefstring with the %tokens% is currently for locking only.  It
was implemented in bug 70623.

Comment 11

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

sorry for the delay.  please feel free to send me direct email when you need a 
review.

seems fine, two comments:

1)

+      var currentIdenity = null;;

extra ;

2)

instead of:

+        identityServer[j] = new Array(2);
+        identityServer[j][0] = ;
+        identityServer[j][1] = false;

do this:

identityServer[j] = {server:currentIdentity.directoryServer, deleted:false};

then, instead of doing identityServer[j][0], you can do identityServer[j].server
and instead of identityServer[j][1] you can do identityServer[j].deleted

it makes the code much more readable.  (I'm guess the boolean was 
for "deleted", I'll leave the name to you.)

I'd recommend the same thing for cleaning up code like this:

          directoriesList.label = gAvailDirectories[0][1];
          directoriesList.value = gAvailDirectories[0][0];

but that can come later, in another bug.  I'll log a bug on you for that.

Comment 12

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

code clean up logged as bug #92437

Comment 13

[Srilatha Moturi]

Attachment: patch v3

Comment 14

[Srilatha Moturi]

In the latest patch
 Removed the extra ";"
 Cleaned up identityServer array based on your suggestion.

Will address the gAvailDirectories in bug 92437.

Comment 15

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

thanks for making that change.

sr=sspitzer

Comment 16

[Srilatha Moturi]

fix checked in

Comment 17

[yulian chang]

Verified with 2001080603 Windows trunck build.