Closed
Bug 79252
Opened 23 years ago
Closed 23 years ago
LDAP prefs are not correctly migrated
Categories
(MailNews Core :: Profile Migration, defect, P1)
Tracking
(Not tracked)
VERIFIED
FIXED
mozilla0.9.2
People
(Reporter: bugzilla, Assigned: srilatha)
References
Details
(Whiteboard: [PDT+] Have a fix)
Attachments
(8 files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review |
If I use a LDAP server from a migrated profile, it won't works. Apparently the
prefs for the server URI has changed since 4.x
Reporter | ||
Comment 3•23 years ago
|
||
Here is was I have in my prefs:
user_pref("ldap_2.servers.InfoSpaceDirectory.csid", "UTF-8");
user_pref("ldap_2.servers.InfoSpaceDirectory.description", "InfoSpace Directory");
user_pref("ldap_2.servers.InfoSpaceDirectory.filename", "ldap.infospace.com4");
user_pref("ldap_2.servers.InfoSpaceDirectory.position", 5);
user_pref("ldap_2.servers.InfoSpaceDirectory.replication.lastChangeNumber", 0);
user_pref("ldap_2.servers.InfoSpaceDirectory.searchBase", "c=US");
user_pref("ldap_2.servers.InfoSpaceDirectory.serverName", "ldap.infospace.com");
and here is was I think we are suppose to have:
user_pref("ldap_2.servers.infospace.csid", "UTF-8");
user_pref("ldap_2.servers.infospace.filename", "ldap.infospace.com9");
user_pref("ldap_2.servers.infospace.replication.lastChangeNumber", 0);
user_pref("ldap_2.servers.infospace.searchBase", "o=netscape communications
corp.,c=us");
user_pref("ldap_2.servers.infospace.uri",
"ldap://ldap.infospace.com:389/o=netscape communications corp.,c=us");
Assignee | ||
Comment 4•23 years ago
|
||
only the first directory server is getting migrated and the rest are not.
In your prefs after migration this is what you were supposed to have.
user_pref("ldap_2.servers.InfoSpaceDirectory.csid", "UTF-8");
user_pref("ldap_2.servers.InfoSpaceDirectory.description", "InfoSpace Directory");
user_pref("ldap_2.servers.InfoSpaceDirectory.filename", "ldap.infospace.com4");
user_pref("ldap_2.servers.infospace.replication.lastChangeNumber", 0);
user_pref("ldap_2.servers.infospace.uri",
"ldap://ldap.infospace.com:389/o=netscape communications corp.,c=us");
I have patch. will post it right away.
Status: NEW → ASSIGNED
Assignee | ||
Comment 5•23 years ago
|
||
Assignee | ||
Comment 6•23 years ago
|
||
ducarroz, can you try the patch and see if this will fix the problem.
You need to set the preference
ldap_2.prefs_migrated to false.
Assignee | ||
Comment 7•23 years ago
|
||
Cc in bhuvan for a review
Assignee | ||
Comment 8•23 years ago
|
||
ccing Seth for a sr
Comment 9•23 years ago
|
||
Priority P2, TFV 0.9.1.
Priority: -- → P2
Target Milestone: --- → mozilla0.9.1
Comment 10•23 years ago
|
||
note, we doing some sort of ldap pref migration already.
see nsPrefMigration.cpp
but, that only happens at profile migration. and if someone migrated from 4.x to
mozilla 0.9 or netscape 6.0, they won't go through that code again when they run
a newer build.
question: your code only gets run when ldap pref UI is brought up.
what happens I had certain prefs in 4.x, migrate to 6.x, but don't bring up the
pref UI first? won't things fail since my prefs aren't migrated? or are those
prefs only involved with the UI?
Updated•23 years ago
|
Priority: P2 → P1
Assignee | ||
Comment 11•23 years ago
|
||
You are right about code gets run when ldap pref UI is brought up.
Since, by default no server is selected for autocompletion, you need to go to
preferences to select a server.
So, the prefs get migrated before you use it.
Comment 12•23 years ago
|
||
> by default no server is selected for autocompletio
what if I used an ldap server for autocomplete in 4.x, and then I migrated over
to 6.x?
does that case work as expected?
Assignee | ||
Comment 13•23 years ago
|
||
Actually, the preference has changed from 4.x
In 4.x we used to have
user_pref("ldap_2.servers.<server-name>.autocomplete.enabled", true);
but in 6.x it is
user_pref("ldap_2.autocomplete.directoryServer", <server-name>);
So the first time when you use a migrated profile, this preference does not
exist. you will need to select a server again.
Comment 14•23 years ago
|
||
if that's the case, then you haven't fixed the problem.
LDAP prefs are still not correctly migrated.
instead of doing the migration in the .js for the UI, you should figure out the
proper place to migrate the prefs so that if I used LDAP autocomplete in 4.x, it
works in 6.x.
nsPrefMigration.cpp is not the correct place for that, since if the user has
already migrated (to mozilla 0.9 or netscape 6.0) they won't be going through
that code again.
if we do it right, a user will get and install mozilla 1.0 / 6.x, and LDAP
autocomplete will just start working as it did in 4.x, assuming they migrated
their 4.x profile.
Assignee | ||
Comment 15•23 years ago
|
||
Migrating the LDAP prefs the first time compose window, And in migrate()
checking for the autocompletion.enabled pref for each server and initialize
ldap_2.autocomplete.directoryServer should fix this.
Comment 16•23 years ago
|
||
that makes sense. instead of duplicating the code, make sure to put the code in
the proper service (any ideas where?) and then call it from both places.
Assignee | ||
Comment 17•23 years ago
|
||
Comment 18•23 years ago
|
||
Srilatha,
Your changes look good. Couple of things from my end :
* Don't you still need to incorporate your new LDAP js file in (xpinstall)
packages files to have it exported properly into the builds
* looks like LDAPPrefsService is defined but not used in MsgComposeCommands.js
file..
r=bhuvan
It will be better to get Candice's review on LDAP specific code segments as I am
no expert there.
Assignee | ||
Comment 19•23 years ago
|
||
You are right, I need add the js file to packages.
Eventhough we are not using LDAPPrefsService in MsgComposeCommands.js, when we
call the constructor we are migrating the 4.x LDAP prefs and initializing the
nsLDAPService. This is essential when the user opens the mail compose window
before going to preferences.
I am building on mac and I have some mac specific stuff to add to the patch.
Will post an updated patch soon.
Assignee | ||
Comment 20•23 years ago
|
||
Comment 21•23 years ago
|
||
Setting target milestone to 0.9.2 (check it in anytime, even before, when the
tree is open for). Per PDT triage.
Target Milestone: mozilla0.9.1 → mozilla0.9.2
Comment 22•23 years ago
|
||
general comments
-----------------
Any LDAP dependencies need to be conditionalized, so that building
without MOZ_LDAP_XPCOM defined generates a still-working, LDAP-free
build. This means that most Makefile.in and makefile.in changes will
need to be tweaked, as well as some Mac magic. On Unix, be sure to
conditionally tweak the REQUIRES lines as well, so that
MOZ_TRACK_MODULE_DEPS builds continue to work. See
xpfe/components/{build,autocomplete/*} for examples. It may also be
necessary to have JS detect whether or not this is an LDAP build
before deciding to execute LDAP-dependant code.
When adding new files, at least, can you wrap your code at 80 columns
for readability?
Unfortunately, dump() sends stuff to the console not just in debug
builds, but in release builds as well. So if you could take out the
calls to dump(), that would be good.
There seem to be a fair number of explicit |catch| clauses which don't
do anything but silently ignore the error, and attempt to continue
executing with obviously horked data (eg null interfaces) rather than
returning. In many of these cases, I presume the right thing to do is
to either not |catch| at all and let the exception unwind up the
stack, or else give the user some sort of other feedback that there
was a problem. Can you go through the catches and fix these things in
the appropriate ways?
nsILDAPPrefsService.idl:
------------------------
license nits: you've got an extra < before your name; the copyright
year is wrong.
misc nits: I think "6.x format" by the migrate decl() should really be
"mozilla format"; the "serves" at the interface comment should be
"servers"
There appear to be spacing problems in and around the comments; I bet
there are some tabs there.
nsISupportsArrays do not map directly to JS arrays. If you want to
use them from JS, you need createInstance the nsSupportsArray
contractid then call the nsISupportsArray interface methods to
manipulate it. So if anyone tries to use the getter and setter for
availDirectories, it'll do something evil and won't work. The arrays
that do map to JS arrays directly are raw XPCOM arrays; see
nsILDAPMessage.idl and nsLDAPDatasource.js for examples of that in
use. Note that XPIDL doesn't currently supports arrays being
attributes, so getters and setters need to be declared directly. All
that said, as far as I can tell, none of the attributes listed in
nsILDAPPrefsService.js are actually used outside of that file -- they
seem to be just private internal properties of that component. So
could they actually be removed from the interface altogether -- or
do you anticipate needing to access them from outside in some case?
It looks to me like the deleteServer method is just there to provide
symmetry with the addServer method. Rather than hanging these two
methods off nsILDAPPrefsService service, how about just adding a createServer
helper method onto nsILDAPService itself, and then calling into the
nsILDAPService directly for all three things?
.migrate():
It'd be nice if there were a comment somewhere that explained what the
significance of the 2 in ldap_2 is ...
>+ ldapUrl = Components.classes["@mozilla.org/network/ldap-url;1"];
>+ ldapUrl = ldapUrl.getService(nsILDAPURL);
LDAP URLs are not singletons, so instead of getService, use
createInstance.
It looks like at most one directory server will be enabled due to the
use of the enable variable. I assume that's intentional, but it's not
obvious from reading the code. If it is intentional, can you please a
comment indicating that? In general, more comments are likely to make
for faster reviewer turnaround, I suspect.
Assignee | ||
Comment 24•23 years ago
|
||
Assignee | ||
Comment 25•23 years ago
|
||
Few things in this patch that are different from the previous one
nsLDPAPrefsService does not have the nsLDAPService stuff since we decided to
drop ldapservice.
added ifdef MOZ_LDAP_XPCOM to the makefiles.
removed dumps. probably added a couple ( when prefs service or ldap url
fails).
Fixed the "Catch" caluses
Fixed license brb
Assignee | ||
Comment 26•23 years ago
|
||
Comment 27•23 years ago
|
||
Because you're calling migrate() from the constructor, when getService() is
called from MsgComposeCommands.js, it may have the side effect of doing the
migration. This seems a bit unintuitive to me; if you could add a comment to
MsgComposeCommands.js explicitly mentioning that just getting the service may
have this possible side-effect, you've got r=dmose@netscape.com. No need to
post another patch here.
I don't think it matters a lot in this particular case, so it's not important to
me that you change this here, but this is worth keeping in mind going forward:
One issue with the XPCOM model is that it's impossible to communicate any sort
of failure back from an object's constructor, because the failure can only get
communicated back to the factory, not to whoever actually asked for the
component to be instantiated. The general workaround for this is that in XPCOM
components, one never does anything that could fail inside the constructor, but
instead moves such initialization to a separate init() method which any clients
call manually.
Assignee | ||
Comment 28•23 years ago
|
||
Assignee | ||
Comment 29•23 years ago
|
||
made change to Makefile.in so that it builds on linux
Modified pref calls so that we save Multibyte character values right.
I have built this on mac too. Will post a patch for that soon
Depends on: 71247
Comment 30•23 years ago
|
||
I notice that this version no longer modifies pref-addressing.xul. I assume
this is intentional?
My only concern is this:
++ if (dn && ldapService)
++ ldapUrl.dn = ldapService.UCS2toUTF8(dn);
This means that if the LDAP Service is unavailable, it will migrate everything
_except_ the DN, but mark the preferences as migrated. This seems like
confusing behavior to me. Aborting the entire migration if ldapService is
unavailable seems more reasonable to me, since it doesn't leave the user with
half-baked prefs.
r=dmose@netscape.com if you address these concerns; no new patch necessary if
those are the only changes.
Assignee | ||
Comment 31•23 years ago
|
||
pref-addressing.xul: Yes it is intentional. I had this change in my tree
(removes javascript error). It is a fix for bug 79780.
Will make the change such that we return if ldapservice is not available
if (dn && ldapService)
ldapUrl.dn = ldapService.UCS2toUTF8(dn);
else
return;
Assignee | ||
Comment 32•23 years ago
|
||
Comment 33•23 years ago
|
||
Please do not use the nsIPref interface. It has been depricated in favor of
nsIPrefService and nsIPrefBranch.
Assignee | ||
Comment 34•23 years ago
|
||
After talking to Brian, he gave me an o.k on using nsIPref since these methods
in nsIPrefBranch and nsIPrefService are not usable from js.
Assignee | ||
Comment 35•23 years ago
|
||
Comment 36•23 years ago
|
||
looks good.
I'd add a call to flush out the prefs file to disk after you are done migrating.
something like:
+ gPrefInt.SetBoolPref("ldap_2.prefs_migrated", true);
+ gPrefInt.savePrefFile(null);
that's been the common practice: migrate, save prefs.
do that, and sr=sspitzer
Comment 37•23 years ago
|
||
a=dbaron for trunk checkin (on behalf of drivers) assuming above comments have
been addressed
Assignee | ||
Comment 38•23 years ago
|
||
Fix checked in
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 39•23 years ago
|
||
This fix as caused regression bug 86996!
Reporter | ||
Comment 40•23 years ago
|
||
oops, wrong bug number! This is causing regression bug 86966
Updated•20 years ago
|
Product: MailNews → Core
Updated•16 years ago
|
Product: Core → MailNews Core
You need to log in
before you can comment on or make changes to this bug.
Description
•