162789 - Need ability to search/filter based on presence in an address book

Status: VERIFIED FIXED

(SeaMonkey :: MailNews: Message Display, defect)

Description

[Scott MacGregor]

For a project I'm working on, I need the ability to do a search for the sender
in a personal address book (imagine setting up a filter or doing a search where
the criteria is "If Sender is in my Personal Address Book" or "if Sender is in
my Work address book")

Patches coming up.

Comment 1

[Scott MacGregor]

Attachment: required address book changes to expose some functionality

This address book patch allows the search code to ask a mdb based directory if
a card exists for a particular email address. This is the same technique the
CAB uses to determine if it needs to add a new collected address.

Comment 2

[Scott MacGregor]

Attachment: Search code modifications

The search code modifications patch contains the search related changes to
support this. We've introduced a new search attribute, SenderInAddressBook. The
value attribute for this search term stores a URI which identifies the address
book to be searched. This allows a  UI layer to let the user pick the address
book they wish to search against.

Comment 3

[Scott MacGregor]

Attachment: address book, filter, search and mailWidgets inclusive patch

This is a patch which contains all of the changes necessary for supporting
search for sender in an address book and filter based on sender in an address
book. 

It includes:

1) Changes to mailWidgets.xml which has the xbl bindings for search terms in
our UI. If the user selects the "Sender in AB" search attribute, then the
search value element turns into a combo box listing all of your local address
books so you can pick one.

2) nsIAbMDBDirectory interface changes to make it easy to ask if a card exists
for an email address. This just calls straight into an existing method on a mdb
database which the CAB already uses.

3)Adding backend search and filter support for searching by sender in an
address book. The search value for such a search is a string which points to
the URI of the address book the user wants to search on. A msg search term now
has to know how to get the RDF data source for this URI (which is cached for
performance reasons), then when given an email address string, looks it up in
the address book. 

4) Searching by sender in an address book is added to the search validity
tables for offline searching and filtering. This causes this option to show up
in the search and filter dialogs as a valid search attribute.

Comment 4

[Scott MacGregor]

UI Notes:
This feature is going to require a couple wording changes from the text I have
now  In the search attribute drop down box, we already use the word "Sender" to
represent searching against a specific sender's name. We need some other phrase
to represent a search by sender in an address book.

Currently I made up the following which cleary doesn't make sense:
  Attribute Name            Operators                    Value
"Search in AB"                "Is" / "Isn't"          "Personal Address Book"

screen shots coming up. 

Comment 5

[Scott MacGregor]

Attachment: screen shot of filter dialog

Comment 6

[Navin Gupta]

Comment on attachment 95623 [details] [diff] [review]
address book, filter, search and mailWidgets inclusive patch

r=naving

Comment 7

[Scott MacGregor]

tweaking the summary

Comment 8

[Scott MacGregor]

Attachment: updated all inclusive patch

This updated patch contains the latest word smith ideas from Jennifer for this
dialog. In order to do that I had to introduce two new search operators:

IsntInAB
and
IsInAB

I'll post a screen shot.

Comment 9

[Scott MacGregor]

Attachment: screen shot

Comment 10

[Scott MacGregor]

cc'ing David and Naving. We have a patch which allows you to filter or search
(locally) for the sender being in a local address book. 

In order to do this, we had to add a new search attribute enum and a two new
search  operators:
    const nsMsgSearchAttribValue SenderInAddressBook = 47;
    const nsMsgSearchAttribValue Label = 48; 
    
    const nsMsgSearchOpValue IsInAB = 16;
    const nsMsgSearchOpValue IsntInAB = 17;

These values can now get written out to rules.dat when the user creates a filter
using this stuff. When a client without this path reads in rules.dat, it doesn't
have a search attribute for SenderInAddressBook. So it incorrectly parses the
text in the rules.dat file as a custom header and it adds it to the custom
headers list. As a result, every time rules.dat is parsed, the search code
creates a new custom header entry. 

We need to figure out a way to add this new search attribute (and the operators)
without causing problems for users that switch between builds that know about
this stuff and builds that don't. 

Possible solutions:
1) Fork rules.dat so we now have a new rules1.dat file 
2) bump the version number. This may make all filters inoperable when you go
back to the older build though.
3) Other options?

Comment 11

[Scott MacGregor]

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

Comment 12

[David :Bienvenu]

bumping the version number won't help - from what I can see, the file version
code is used to upgrade old filter files, but "future" versions aren't rejected
or handled. Also, the search term parsing code nsMsgSearchTerm::DeStreamNew
doesn't currently get passed the version number.

It's a shame, because I think the parsing code could have worked - custom
headers are quoted, and other attributes are not quoted.
NS_MsgGetAttributeFromString says any attribute it doesn't recognize is a custom
header. But even if it didn't do that, we'd need to remember the "future"
attribute as a string, disable the filter in memory, and make sure to file it
out correctly. That's a fair chunk of work.

I don't see any choice but to fork the rules.dat files. We should come up with a
strategy for dealing with this going forward. The simplest strategy is just to
ignore future version filter files - that's a bit harsh, but fairly standard. A
more difficult strategy is to gracefully handle future filter formats w/o
crunching them.

Comment 13

[Scott MacGregor]

Navin and I were just talking about this in my cube when David made his post. We
also think the only solution is to fork rules.dat.

What about the following:

1) The next time we write out the filter rules, we write them to a new file:
"rules1.dat" (is there a better name we should use?)

2) When reading in the filters, we first look for rules1.dat, if we don't find
it we fall back to rules.dat.

In short, the first time you edit your filters with this new build we will fork
rules.dat and from that point forward trunk builds would look at the new filter
file. 

Comment 14

[David :Bienvenu]

I think that's fine, but, we should, at some point, have a strategy for dealing
with future file versions. When we do this, we should bump the file version, and
we should make the filing in code just ignore, for now, future file versions.
It's better than losing your filters, or having them corrupted. We probably
should put up an alert if you try to load a filter file with a newer version
because failing silently would cause a lot of bugs to get filed.

The other thing is, we should try to coordinate this checkin with all the other
filter actions, etc, that we're adding, so that we just bump the file version once.

Comment 15

[Erich 'Ricky' Iseli]

Sorry I'm late I'm commenting on this, but I've been watching bug 34340 for some
time now and didn't notice this one.

Re: Comment#1
> This is the same technique the CAB uses to determine if it needs to add a new
> collected address. 

There's a bug about collecting addresses (couldn't find it right now) that
collects addresses even if they are in another AB, or if the address is not
defined as primary. Obviously, if somebody's alternate address is in my personal
address book, I want it to be treated as being in the personal addressbook.

Is this issue addressed by this bug? Has this been fixed? Should a dependency be
set on these bugs?

Comment 16

[Scott MacGregor]

I think I'm going to go ahead and land the backend portion of this patch and
until we are ready with the other filter changes, I just won't land anything
that causes this to show up in the UI. That way folks can't create filters using
it and we won't have problems with rules.dat until we make our one time fork.
This would also allow Seth to work on some of his filter improvements without
conflicing with any of my changes.

Navin, can you review this patch? You've already reviewed it once before but I
had to make a few tweaks when I updated it to the trunk and made it match
Jennifer's spec. Thanks!

Comment 17

[Navin Gupta]

Comment on attachment 98632 [details] [diff] [review]
updated all inclusive patch

r=naving patch looks good but are mailWidgets.xml changes going in ? you might
want to test how the partial patch 
behavior will be on trunk.

Comment 18

[Navin Gupta]

you can file/give forking bug to me. 

Comment 19

[David :Bienvenu]

Comment on attachment 98632 [details] [diff] [review]
updated all inclusive patch

sr=bienvenu. Looks fine. It is going to cause a problem with filter after the
fact, however, because that has to use Search with the appropriate folder
scope, and this attribute seems to only work with the offline mail search
scope. Is this code just for filters, or does it also work with Search? If the
latter, what happens when you try this search on an imap folder?

Comment 20

[Scott MacGregor]

It works for filters and searching. However it only shows up if you are doing an
offline search. How would you do this in an online imap search? Bypass the
encoding and treat it as if it were an offline search? 

Comment 21

[David :Bienvenu]

I think currently we can't mix scopes like this. We can search over multiple
scopes, which would be required for this, but we can't use boolean operators
between the scopes. Ideally, what we'd do is run the search over the normal
scope (e.g., imap) for the terms that needed to use the imap server, and combine
those results (and/or) with the results of the offline mail search scope for
is/isn't in AB.

Comment 22

[Scott MacGregor]

I've landed the backend support for this. Everything is checked in, except I
commented out the changes in nsMsgImapSearch.cpp so it won't show up in the UI.

I'll keep this bug open until we figure out our forking of rules.dat plan. 

Comment 23

[Ian Pottinger]

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

Comment 24

[Matthias Versen [:Matti]]

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

Comment 25

[Jo Hermans]

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

Comment 26

[Jesse Ruderman]

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

Comment 27

[Jesse Ruderman]

See also bug 187768 (not a dup, oops), allow filter of "To or CC" to use "is in
Address Book..." and "is not in Address Book...".

Comment 28

[Sander]

What part of this functionality isn't yet working that this bug is still open? I
can currently both search and filter for sender is/isn't in my address book.

Comment 29

[David :Bienvenu]

yes, I think this is all checked in now. Please re-open if you disagree.

Comment 30

[Erich 'Ricky' Iseli]

Verified fixed in latest nightly 2003020608. It was already fixed in 1.3a,
however some minor changes were done to it later.

Comment 31

[David Hagood]

This feature does not seem to allow the selection of an LDAP based address book
- only local address books.

That kind of sucks in a corporate environment.