362186 - entries from ldap address book not displayed (offline) - you can only search

Status: RESOLVED FIXED

(MailNews Core :: Address Book, defect)

Description

[James Andrewartha]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8a6) Gecko/20050111
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8a6) Gecko/20050111

After downloading an LDAP address book, there is a local .mab file with all the addresses in it, however you can't view them all, you can only search for addresses. This is probably because nsAbLDAPDirectory::GetOperations only returns opSearch: http://lxr.mozilla.org/mozilla/source/mailnews/addrbook/src/nsAbLDAPDirectory.cpp#227

Reproducible: Always

Steps to Reproduce:
1. Add LDAP address book
2. Open the address book, open the LDAP ab's properties, offline, download now
3. Click on the LDAP address book

Actual Results:  
No addresses are displayed

Expected Results:  
All the addresses in the LDAP address book should be displayed

I'm not sure how hard it would be to add support for opRead given the .mab is on disk, I'm not familar with MailNews code at all.

Comment 1

[James Andrewartha]

Actually, the GetOperations call is ignored. The important piece of code is at http://lxr.mozilla.org/mozilla/source/mailnews/addrbook/src/nsAbLDAPDirectory.cpp#237 which either queries the local replica if offline, or does a search using the current query on the address database.

It would be better to emulate http://lxr.mozilla.org/mozilla/source/mailnews/addrbook/src/nsAbMDBDirectory.cpp#356 which, if no search is active, returns an enumerator for the entire address book. For offline mode this is easy, for online we can search for @ or * which will match all email addresses. Although this might be bad to do every time, so perhaps hook into the existing replication code and only do it at startup or something, and then dump all entries from the replica.

Comment 2

[Mark Banner (:standard8)]

(In reply to comment #1)
> Actually, the GetOperations call is ignored. The important piece of code is at
> http://lxr.mozilla.org/mozilla/source/mailnews/addrbook/src/nsAbLDAPDirectory.cpp#237
> which either queries the local replica if offline, or does a search using the
> current query on the address database.

> It would be better to emulate
> http://lxr.mozilla.org/mozilla/source/mailnews/addrbook/src/nsAbMDBDirectory.cpp#356
> which, if no search is active, returns an enumerator for the entire address
> book. For offline mode this is easy, for online we can search for @ or * which
> will match all email addresses. Although this might be bad to do every time, so
> perhaps hook into the existing replication code and only do it at startup or
> something, and then dump all entries from the replica.

Requiring a search for ldap entries whilst online was an intentional implementation - the implementers did not want to put an unnecessary load on the servers.

There is a separate bug on the online ldap requiring a search - I can't find it at the moment, its probably in Core/Mailnews:Address Books or Core/Mailnews:LDAP Integration.

For the offline part I'd agree that we should show the list by default, but I'd disagree that you'd need to change the code as much as you are suggesting. We should just need to remove the mIsQueryURI from the if statement, and update the localDirectoryURI - the rest of it will fall into place, the enumerator will be obtained from the nsAbMDBDirectory implementation which will read the downloaded address book for us.

As a side note, if we were to allow online, you still couldn't return an enumerator because the LDAP search is asynchronous - you must start a separate search.

Comment 3

[James Andrewartha]

(In reply to comment #2)
> Requiring a search for ldap entries whilst online was an intentional
> implementation - the implementers did not want to put an unnecessary load on
> the servers.

Yes, I can see why they'd want to do that.

> There is a separate bug on the online ldap requiring a search - I can't find it
> at the moment, its probably in Core/Mailnews:Address Books or
> Core/Mailnews:LDAP Integration.

Is bug 300892 the one you're thinking of?
 
> For the offline part I'd agree that we should show the list by default, but I'd
> disagree that you'd need to change the code as much as you are suggesting. We
> should just need to remove the mIsQueryURI from the if statement, and update
> the localDirectoryURI - the rest of it will fall into place, the enumerator
> will be obtained from the nsAbMDBDirectory implementation which will read the
> downloaded address book for us.

Ok, that seems pretty simple, I'll whip up a patch.

> As a side note, if we were to allow online, you still couldn't return an
> enumerator because the LDAP search is asynchronous - you must start a separate
> search.

Between this and the load issue I wonder whether the offline copy (if available) could be shown by default, although there would need to be some way to tell the user that it may not list all entries and they should search for the latest information. There's also the performance issues in bug 153228 and bug 117230 to consider.

Comment 4

[James Andrewartha]

Attachment: patch to display all contacts of replicated ldap database when offline

Comment 5

[Mark Banner (:standard8)]

(In reply to comment #3)
> (In reply to comment #2)
> > There is a separate bug on the online ldap requiring a search - I can't find it
> > at the moment, its probably in Core/Mailnews:Address Books or
> > Core/Mailnews:LDAP Integration.
> Is bug 300892 the one you're thinking of?

Possibly, though I thought there was another one about. Maybe not.

> > As a side note, if we were to allow online, you still couldn't return an
> > enumerator because the LDAP search is asynchronous - you must start a separate
> > search.
> Between this and the load issue I wonder whether the offline copy (if
> available) could be shown by default, although there would need to be some way
> to tell the user that it may not list all entries and they should search for
> the latest information. There's also the performance issues in bug 153228 and
> bug 117230 to consider.

Ideally we should have some way of caching entries (which is slightly different to offline copy), or even virtual list views have been suggested in the past. I think we should address this part in bug 300892 or any other suitable one, and fix the offline issue here.

I've started looking at your patch. Should hopefully finish the review today.

Comment 6

[Mark Banner (:standard8)]

Comment on attachment 247251 [details] [diff] [review]
patch to display all contacts of replicated ldap database when offline

+        localDirectoryURI = localDirectoryURI + NS_LITERAL_CSTRING("?") + mQueryString;

Should either be:

localDirectoryURI += NS_LITERAL_CSTRING("?") + mQueryString;

or

localDirectoryURI.AppendLiteral("?");
localDirectoryURI.Append(mQueryString);

I think I prefer the first of these - as long as David's also happy with it.

r=me with that line improved.

Comment 7

[David :Bienvenu]

Comment on attachment 247251 [details] [diff] [review]
patch to display all contacts of replicated ldap database when offline

Either of Mark's suggestions seems ok. Thx for the patch!

Comment 8

[Mark Banner (:standard8)]

Confirming bug and moving to Core Address Book as it affects both TB & SM.

Comment 9

[Mark Banner (:standard8)]

Comment on attachment 247251 [details] [diff] [review]
patch to display all contacts of replicated ldap database when offline

Setting r+ again. Looks like bugzilla lost it.

Comment 10

[Mark Banner (:standard8)]

I've just checked this in on James' behalf. Thanks for the patch James. Marking fixed.

Comment 11

[Mark Banner (:standard8)]

Comment on attachment 247251 [details] [diff] [review]
patch to display all contacts of replicated ldap database when offline

Do we want this for the next version of Thunderbird?

Low risk affecting offline mode for LDAP Directories only.

Comment 12

[Mark Banner (:standard8)]

Checked into 1.8 branch as well.

Comment 13

[Jay Patel [:jay]]

I have not had any luck testing this fix, so if anyone that was seeing this problem before can test with the latest TB 1.8 nightly build and verify this, that'll be great!

James:  Any luck with the latest builds?  Can you please retest this.  Thanks.


In case anyone wants to know, here is what I was doing:
1. Setup my Mozilla LDAP in AB
2. Test that LDAP worked (had to search to get any results, but based on comment #2, that is by design), search results gave me a bunch of addresses
3. Select Mozilla LDAP->Properties->Offline->Download
4. Click download and see the "Replication Succeeded" message
5. Check my profile directory, find new "abook-1.mab" file, but it's only 1455K and does not appear to have any addresses in it.
6. Set TB Offline, close AB, reopen AB, try to search for Mozilla LDAP address... no luck.

Not sure if I did something wrong or if my profile is horked somehow... so let me know if there is anything else I can try. 

Comment 14

[Mark Banner (:standard8)]

(In reply to comment #13)
> Not sure if I did something wrong or if my profile is horked somehow... so let
> me know if there is anything else I can try. 

Are you using an LDAP connection that uses authenticated connections? If so, you're probably hitting bug 316170. Currently Download/Offline Replication only works for non-auth connections.

Comment 15

[James Andrewartha]

I just tried Thunderbird version 2 beta 2 (20070116) and can confirm that I see all the entries on the LDAP address book when offline.