80060 - Deleting the selected Directory Server entry deletes wrong entry

Status: VERIFIED FIXED

(SeaMonkey :: MailNews: Address Book & Contacts, defect, P1)

Description

[yulian chang]

Netscape 6.5/2001050914 build

The immediate following Directory Server in LDAP Directory Server dialog server
listing got deleted instead of the selected server. If no following one, then
the previous server got deleted. 

This problem might relate to bug 80038:

Probably that's why the last one left in the LDAP Directory Server dialog can
never be deleted

Comment 1

[yulian chang]

Change the severity to critical.
Directory Server settings might get lost because of wrong deletion.

Steps to reproduce:

1. Launch LDAP Directory Servers dialog by clicking "Edit Directories" button in
the Preferences|Address Books
2. Add a couple of server entries
3. Select one server in the middle (X) and remember the following Server Name (Y)
4. Click on Delete
5. The selected entry (X) disappear, so far so good
6. Click OK to close the LDAP Directory Servers dialog window
7. Go back to "LDAP directory Servers" drop-down list, the selected entry (X)
from Step 3 still exists
8. Click "Edit Directories" button
9. The selected entry from Step 3 is in the LDAP Directory Servers dialog and it
happens that the deletion (for X) was actually entry (Y).
10. Continue deletion for (X) until all entries are gone in the LDAP Directory
Servers dialog
11. Repeat Step 6-8
12. The only entry (X) still exist and can never be deleted

Problem one : the selected entry cannot be deleted
Problem two : there will always be one entry in the LDAP Directory Servers
dialog window

Comment 2

[Srilatha Moturi]

*** Bug 80038 has been marked as a duplicate of this bug. ***

Comment 3

[Keyser Sose]

*** Bug 80592 has been marked as a duplicate of this bug. ***

Comment 4

[Marek Z. Jeziorek]

Setting target milestone to 0.9.2 (check it in anytime, even before, when the
tree is open for). Per PDT triage.

Comment 5

[Srilatha Moturi]

*** Bug 81532 has been marked as a duplicate of this bug. ***

Comment 6

[Srilatha Moturi]

Attachment: patch v1

Comment 7

[Srilatha Moturi]

ccing Bhuvan for review
ccing Seth for sr

Comment 8

[Srilatha Moturi]

Attachment: created a patch with the latest version of this file

Comment 9

[racham]

Srilatha,

I have couple of questions and comments :

1. Why is gRefresh removed now..why was it there in the beginning..?

2. in the block

+         if ((directoryServer != "") && (description != ""))
          {
            directoriesList.label = description;
            directoriesList.value = directoryServer;
          }
          else
          {
-           directoriesList.label = gAvailDirectories[0][1];
-           directoriesList.value = gAvailDirectories[0][0];
+        if(gAvailDirectories.length >= 1) {
+             directoriesList.label = gAvailDirectories[0][1];
+             directoriesList.value = gAvailDirectories[0][0];
+        }
+        else {
+          directoriesList.label = description;
+          directoriesList.label = description;
+        }

-- you can say if (directoryServer && description) in place of 
if ((directoryServer != "") && (description != ""))

-- the first else per the new structure can become 
else if(gAvailDirectories.length >= 1) {

-- what are we trying to set in the second else ? We know by now that 
description or/and directoryServer has/have no value and probably we don't want 
to set one value not set the other. So, may be the second else  should be 
+        else {
+          directoriesList.label = null;
+          directoriesList.label = null;
+        } 

that's just my suggestion. Let me know if there is a purpose for the way you 
have it.

3. Does your setting nextNode item before 
gPrefInt.DeleteBranch(gCurrentDirectoryServerId); caused the problem of deleting  
next and previous entries..?

4. Why are some of the names like netcenter, verisign etc in this file...? You 
should probably have them as a pref (probably ',' separated values) under 
mailnews-ns.js file under commercial, if needed by only commercial builds. You 
can then search for your gCurrentDirectoryServerId in that pref value and do the  
special things you want to do. That string will be empty for mozilla and hence 
that block can be skipped for mozilla.

bhuvan

Comment 10

[Srilatha Moturi]

Here is my explanation
1. Before I removed gReferesh, the user can chose autocomplete against a 
directory server and not select a directory server. With the new implementation, 
when the user selects the checkbox, a directory Server will be selected.
gRefresh is set to true if user adds or removes a directory.

2.
if ((directoryServer != "") && (description != ""))
because if one of these two exists and is an empty string I did not want the the 
listbox get an wmpty string value.
Just above this statement I had
try {
  var directoryServer = gPrefInt.CopyCharPref(pref_string_title);
}
catch (ex)
{
  directoryServer = "";
}
if (directoryServer != "")
{
  pref_string_title = directoryServer + ".description";
  try {
    description = gPrefInt.CopyCharPref(pref_string_title);
  }
  catch (ex) {
    description = "";
  } 
}
so directoryServer and description could be ""

I will attach new patch with else if and also with
+  directoriesList.label = null;
+  directoriesList.value = null;

3.actually, removing the selected node caused the problem.
4.netcenter, verisign, infospace are part of mailnews.js from the beginning. and 
hence they show up in the list of directories. If the user selects on of these 
for deletion, the only way to delete them is by setting position to 0. 

For them to not show up at all in mozilla builds we need to remove them from 
mailnews.js. Also we want to remove verisign and infospace directories 
completely. see bug# 79814.

Comment 11

[Srilatha Moturi]

Attachment: patch v2

Comment 12

[Srilatha Moturi]

*** Bug 82619 has been marked as a duplicate of this bug. ***

Comment 13

[Srilatha Moturi]

Attachment: patch v3

Comment 14

[Srilatha Moturi]

The above patch addresses the issue with netcenter, verisign and infospace 
directories. Apply the patch in bug 79814 before applying this patch.

Comment 15

[Dan Mosedale (:dmosedale, :dmose)]

*** Bug 83648 has been marked as a duplicate of this bug. ***

Comment 16

[Srilatha Moturi]

Attachment: patch v4

Comment 17

[Srilatha Moturi]

We do not have do deal with infospace, verisign and netcenter directories 
because they are not added( bug # 79814) to either mozilla or commercial trees. 
I have also modified the patch such that the preferences are deleted only when 
the user clicks on ok. see bug# 84222

Comment 18

[racham]

r=bhuvan

Please do test them before checking in. thanks.

bhuvan

Comment 19

[Jussi-Pekka Mantere]

PDT+ per 6/12 mtg.

Comment 20

[yulian chang]

Correct summary

Comment 21

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

seems ok to me.
  
sr=sspitzer

as we're not experts in pref-directory.js, please make sure to test really well
before landing.

Comment 22

[Asa Dotzler [:asa]]

a= [e-mail] for checkin to the trunk.
(on behalf of drivers) provided that sspitzer's request for thorough testing has
been satisfied. 

Comment 23

[Srilatha Moturi]

Tested this on windows, mac and Unix
Deleting directory seems to be working fine.
Fix checked in

Comment 24

[Srilatha Moturi]

Marking resolved

Comment 25

[yulian chang]

Verified with 20010618 winds/Linux/MacOS builds