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

RESOLVED FIXED

Status

MailNews Core
Address Book
RESOLVED FIXED
11 years ago
9 years ago

People

(Reporter: James Andrewartha, Assigned: James Andrewartha)

Tracking

({fixed1.8.1.2})

Trunk
fixed1.8.1.2

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

11 years ago
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.
(Assignee)

Comment 1

11 years ago
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.
Summary: entries from downloaded address book not displayed - you can only search → entries from ldap address book not displayed - you can only search
(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.
(Assignee)

Comment 3

11 years ago
(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.
(Assignee)

Comment 4

11 years ago
Created attachment 247251 [details] [diff] [review]
patch to display all contacts of replicated ldap database when offline

Updated

11 years ago
Attachment #247251 - Flags: superreview?(bienvenu)
Attachment #247251 - Flags: review?(bugzilla)
(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 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.
Attachment #247251 - Flags: review?(bugzilla) → review+

Comment 7

11 years ago
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!
Attachment #247251 - Flags: superreview?(bienvenu) → superreview+
Confirming bug and moving to Core Address Book as it affects both TB & SM.
Assignee: mscott → nobody
Status: UNCONFIRMED → NEW
Component: Address Book → MailNews: Address Book
Ever confirmed: true
Flags: review+
OS: Windows 2000 → All
Product: Thunderbird → Core
QA Contact: address-book → addressbook
Hardware: PC → All
Summary: entries from ldap address book not displayed - you can only search → entries from ldap address book not displayed (offline) - you can only search
Version: unspecified → Trunk
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.
Attachment #247251 - Flags: review+
Assignee: nobody → trs80
I've just checked this in on James' behalf. Thanks for the patch James. Marking fixed.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
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.
Attachment #247251 - Flags: approval-thunderbird2?

Updated

11 years ago
Attachment #247251 - Flags: approval-thunderbird2? → approval-thunderbird2+
Checked into 1.8 branch as well.
Keywords: fixed1.8.1.2

Comment 13

11 years ago
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. 
(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.

(Assignee)

Comment 15

11 years ago
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.
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.