437908 - Search/Filters don't check correctly for address in Mac OS X Address Book

Status: RESOLVED FIXED

(MailNews Core :: Filters, defect)

Description

[Mitra Ardron]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-GB; rv:1.9) Gecko/2008053008 Firefox/3.0
Build Identifier: version 3.0a2pre (2008060803)

Filters don't allow matching for addresses being in hte Mac Address Book

Reproducible: Always

Steps to Reproduce:
1.Create a filter
2.Check for "From" in "Mac OSX Address Book"
3.Test it
Actual Results:  
Doesn't match anything

Expected Results:  
It should match on all matching addresses, as for regular address book.

Comment 2

[Mark Banner (:standard8)]

This applies to search and filters.

Given that Mac OS X integration is one of the already discussed features of TB 3, we should fix this.

Comment 3

[Mark Banner (:standard8)]

I've just checked, searching and filtering currently assumes more database. Looks like we're going to need the work going on in bug 413260 to fix this.

Comment 4

[Mitra Ardron]

It seems like bug 413260 is being treated as a lower priority performance bug, while this is a reasonably serious (IMHO) break of stuff that was previously working.

The key importance to me of this bug is that filtering on presence of the sender in the Mac OSX Address Book is a pretty fundamental part of spam filtering for me, and a number of other people I work with. 

Is this really dependent on what I read ( maybe I'm misreading it) as a performance bug?

Comment 5

[Mark Banner (:standard8)]

(In reply to comment #4)
> Is this really dependent on what I read ( maybe I'm misreading it) as a
> performance bug?

You are misreading bug 413260. Its not a performance bug, but a re-organisation of the interfaces within the address book.

The next patch for that bug should hopefully be including the changes we need to make fixing this bug a reality.

Comment 6

[Joshua Cranmer [:jcranmer]]

I think it's really bug 449618 that will be instrumental in fixing this.

Comment 7

[Mitra Ardron]

I think this should be marked as blocking 3a2 because its a step backwards from 3a1, i.e. things that previously worked in 3a1 will stop working.

Comment 8

[Mark Banner (:standard8)]

(In reply to comment #7)
> I think this should be marked as blocking 3a2 because its a step backwards from
> 3a1, i.e. things that previously worked in 3a1 will stop working.
> 
I don't see how this ever worked in 3a1. The code just doesn't support it in filtering because it relies on the specific mork address book interfaces, not the generic ones.

Anyway 3.0a2 is all but released now, and it is not a significant enough bug to block it.

Fixing this shouldn't be too far off now anyway, I already have a patch, just waiting for bug 449618 to be resolved.

Comment 9

[Mitra Ardron]

Strange - I wonder if it was TB2 that it worked in, 

This definately USED to work - because I had spam filters that filtered stuff based on the sender's presence in the OSX address book which used to work, but don't work now. 

- Mitra

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

TB2 doesn't support the OS X address book. So no idea how you got this to work. Or have you used a modified TB2 version? There was a special build with the OS X address book feature enabled.

Comment 11

[Mitra Ardron]

That could have been it - everyone I know on Macs who used TB was on that OSX-modified version, unfortunately the OSX modifications weren't carried along with the rest of the developement of TB2 which was a substantial impedement to using it on Macs as that version had lots of other problems! 

Hopefully Mark's patch will fix it.

Comment 12

[Mark Banner (:standard8)]

Attachment: The fix

Now that bug 449618 is checked in, here is the patch to extend cardForEmailAddress to the OS X address book as well. This will enable search and filtering with the is & isn't in address book.

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Mark, can't we use the search feature which comes with the address book API? I believe it should be a lot faster because it is fully indexed:

http://developer.apple.com/documentation/UserExperience/Conceptual/AddressBook/Tasks/Searching.html#//apple_ref/doc/uid/20001024-103556

Comment 14

[David :Bienvenu]

Comment on attachment 334095 [details] [diff] [review]
The fix

looks good - one nit, you can move +  PRUint32 i; to the for loop, since you only use it there.

Comment 15

[Mark Banner (:standard8)]

(In reply to comment #13)
> Mark, can't we use the search feature which comes with the address book API? I
> believe it should be a lot faster because it is fully indexed:
> 
> http://developer.apple.com/documentation/UserExperience/Conceptual/AddressBook/Tasks/Searching.html#//apple_ref/doc/uid/20001024-103556

I think unless you have a reasonably large address book it probably won't be much faster. It certainly would be harder. You see we already have a local cache of all the cards in the address book, therefore its quite simple just to go through them to find a match.

If we go through the search API, then we'd have to locate which card to use there, but then still marry that up with our local cache.

So at a quick glance I don't feel that this would really benefit us, though only performance tests would tell. Feel free to do some investigations, at the moment the current situation seems reasonable enough, so I'm not planning on changing it for the time being.

Comment 16

[Mark Banner (:standard8)]

Patch now checked in, changeset id 124:5773e859a5a6. Will be in tomorrow's nightlies.

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

(In reply to comment #15)
> So at a quick glance I don't feel that this would really benefit us, though
> only performance tests would tell. Feel free to do some investigations, at the
> moment the current situation seems reasonable enough, so I'm not planning on
> changing it for the time being.

Ok, so I'm fine with that for now. Thanks. 

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Verified with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1a2pre) Gecko/20080819030054 Shredder/3.0b1pre ID:20080819030054

Mark, should we have some automated tests here? Or are manual tests in Litmus enough?

Comment 19

[Mark Banner (:standard8)]

(In reply to comment #18)
> Verified with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US;
> rv:1.9.1a2pre) Gecko/20080819030054 Shredder/3.0b1pre ID:20080819030054
> 
> Mark, should we have some automated tests here? Or are manual tests in Litmus
> enough?

Automated OS integration tests are at the moment potentially problematic at the moment because of the effect on user's system and the requirements for IT to provide specific versions of systems etc.

Rather than divert effort from TB 3.0 into solving those problems, I think we should park automating them for now, and provide litmus tests (Note I'll leave in-testsuite? to remind us later).

Comment 20

[Mitra Ardron]

This seems to work with a couple of caveats.

After I ran it the first time, and edited some items in my address book, it didn't seem to want to run again - I have been unable to repeat this, but someone who wrote the code might want to have a quick look for something obvious. 

There is a generic problem where it relies on the overlap between OSX and internal address books - so for example I have someone with a work and two home email addresses in OSX - if I look at the "Address Book" in Shredder, it only gets her work address and first home address. So this is all the filters see. 

I'm not sure if this qualifies as a seperate bug  / wish list (more than 2 email addresses per person?)

Comment 21

[Mark Banner (:standard8)]

(In reply to comment #20)
> This seems to work with a couple of caveats.
> 
> After I ran it the first time, and edited some items in my address book, it
> didn't seem to want to run again - I have been unable to repeat this, but
> someone who wrote the code might want to have a quick look for something
> obvious.

Nothing obvious as the code should be keeping an up to date copy of the cards in the address book.

> There is a generic problem where it relies on the overlap between OSX and
> internal address books - so for example I have someone with a work and two home
> email addresses in OSX - if I look at the "Address Book" in Shredder, it only
> gets her work address and first home address. So this is all the filters see. 
> 
> I'm not sure if this qualifies as a seperate bug  / wish list (more than 2
> email addresses per person?)

That would fall under bug 118665 (and possibly some others). We have a miss match between what OS X supports and what we support. Whilst we could special case searching for OS X AB, you still wouldn't get the email address displayed or anything, which is potentially more confusing. Therefore I think this specific problem is bug 118665 and not worth fixing separately.

Comment 22

[Mitra Ardron]

The first problem is happening a lot, sometimes restarting TB seems to fix it, but not always.  Its not 100% repeatable, but the symptom is that you

Tools -> Message Filters -> Select a filter -> Run Now 

and nothing happens.

When it works, the buttom changes to Stop and a progress bar appears. 

I've checked the Error Console and nothing appears there.

Comment 23

[Mark Banner (:standard8)]

Mitra, please can you try this on non-OS X address books. I very much doubt the your problem would be specific to just the OS X address book, though obviously I may be wrong.

Please could you try turning on filter logs and see if the logs show anything interesting.

Comment 24

[Mitra Ardron]

I don't have a non-OSX machine, 

I've turned on filter logs - in the case where the filter doesn't run, nothing shows up there and there are no Errors in the error console

Comment 25

[Mark Banner (:standard8)]

(In reply to comment #24)
> I don't have a non-OSX machine,

I just said non OS X address books, i.e local address books.

> I've turned on filter logs - in the case where the filter doesn't run, nothing
> shows up there and there are no Errors in the error console

Some thoughts:
 
Have you ensured that the case of the email addresses are the same in both the address book and the email?

Does running the filter manually on the message work?

Have you checked what TB's address book is listing at the same time as running the filters?

Comment 26

[Mitra Ardron]

Hi Mark

We seem to be miscommunicating about this bug ...

The problem is NOT that the filter runs, nad doesn't match theaddress. Its that sometimes the filter doesn't seem to get run at all.   I notice this because both the Inbox folder and the OSX Address book are huge, so when it DOES run, I get the progress bar. 

As far as I can tell it *is* running the filters correctly on incoming email, its the manual run of the filter that fails. 

Comment 27

[Mark Banner (:standard8)]

(In reply to comment #26)
> As far as I can tell it *is* running the filters correctly on incoming email,
> its the manual run of the filter that fails. 

So I've been running the filters manually (and automatically) occasionally to test this and I've not been seeing any problem with the matching.

Maybe the error is more to the manual filter not being run for some other reason, not the fact its got the OS X address book in it. I wondering if we should cover this in a separate bug.

Comment 28

[Mark Banner (:standard8)]

(In reply to comment #27)
> Maybe the error is more to the manual filter not being run for some other
> reason, not the fact its got the OS X address book in it. I wondering if we
> should cover this in a separate bug.

Mitra, please look for an existing bug or file a new one for this. I'm going to close this bug again as afaik we are now correctly using the Mac OS X Address Book in search and filtering - you bug seems more related to when the filters are fired, which would be a separate issue.

Comment 29

[Wayne Mery (:wsmwk)]

This FIXED bug is flagged with in‑testsuite?   It would be great if assignee or someone else can clear the flag if a test is not appropriate.  And if appropriate, create a test and plus the flag to finish off the bug.

Comment 30

[Ludovic Hirlimann [:Usul]]

https://litmus.mozilla.org/show_test.cgi?id=7757