586101 - Quickfiltering for "Recipients" fails on BCC recipients (does not find/return messages with matching recipients in Bcc: header)

Status: ASSIGNED

(Thunderbird :: Search, defect)

Description

[Thomas D. (:thomas8)]

+++ This bug was initially created as a clone of Bug #213318 +++

STR

1 have messages in ANY folder that have visible-for-you BCC recipients header, e.g. Bcc: [e-mail]
- e.g. in your SENT folder, because you sent message(s) to BCC recipients
- or in any folder, with "Tools > Account Settings > Your Account > Copies & Folders > Place replies in the folder of the message being replied to" enabled
- or because you manually copied such sent messages into any folder

2 Ctrl+F, enter "blind" into the filter bar search box (with recipient filter ON, as is the default)

Actual result:

- matching message containing "blind" as BCC-recipient is not found

Expected result:

- matching message containing "blind" as BCC-recipient should be found (clearly, BCC recipients are a legitimate and not infrequent variant of "Recipients", and there is no indication whatsoever that we exlude Bcc header)

This problem was initially reported by Wayne Mery (:wsmwk) as early as 2003 (7 years ago) as bug 213318, of which this an updated clone. This is non-trivial for users who regularly use BCC recipients, e.g. as an anti-spam measure. It's about time to fix it.

Please set wanted-thunderbird+ flag for this bug, and a target milestone, if possible.

Comment 1

[Fernando García Iglesias]

I use the quick filter window on the upper right side of the Thunderbird windows. Since several months I send all my messages using BCC: instead of To: using the MailTweak plug-in (BTW, it would be interesting to let the user choose if the BCC is the default mode).

I see this error is still happening, so when I want to locate a sent message to a certain e-mail, I need to open the advanced search windows (CTR + CAPS + F), and select to find in the From, To, CC and BCC field.

Comment 2

[Thomas D. (:thomas8)]

What is the official procedure to request wanted-thunderbird+ flag?
This is basic quicksearch/filter functionality that's known broken since 2003. Shame. I am failing to understand how new features like quick filter bar can be implemented without ensuring that they actually work for expectable use cases.

Comment 3

[john]

The functionality for "recipient" does not include BCC recipients, only TO and CC, as of version 12.

Is there a hidden config switch that would engage BCC in the Quick Filter, so as to avoid using the cumbersome advanced find tool ?

Comment 5

[Andrew Sutherland [:asuth] (he/him)]

It's simple to change the search predicate used by the quick filter:
http://mxr.mozilla.org/comm-central/source/mail/base/modules/quickFilterManager.js#1217

The problem is there is no to/cc/bcc predicate; just a from/to/cc/bcc predicate (AllAddresses):
http://mxr.mozilla.org/comm-central/source/mailnews/base/search/public/nsMsgSearchCore.idl#52

The manager is oriented so that the predicates happen in parallel, so it's not entirely trivial to realize when sender and recipients are both flagged and then upgrade to AllAddresses.

I think an additional predicate would need to be added for similar semantics; e-mail address predicates actually match against the display name and e-mail address separately.  A work-around would be to use the HdrProperty predicate on bcc.

Comment 6

[john]

I am perplexed by comment #5.   A user modification to the searches, to add "BCC", was simple enough [and appears to work...].   Why is it difficult to add an underlying predicate search [To/CC/BCC] to the search list, so that the recipient button could rely on it?

Make #53, for to/cc/bcc, and start the custom stack at 54-99, instead ?  Or move custom to 53, and tick upwards the stack to 54-99, placing to/cc/bcc at 52.

or...am I missing something

Comment 7

[john]

(In reply to john ruskin from comment #6)
> or...am I missing something

Granted, I probably am . . .

Comment 8

[Andrew Sutherland [:asuth] (he/him)]

It shouldn't be difficult; just more difficult than I was expecting!  The C++ mods required should be minimal and just minor variations on the existing code.  It's just more than changing a constant.  Aceman seems interested in this bug, so I'm hoping he'll bring some of his skills to bear!

Can you clarify what you mean by user modification?  If you change it to nsMsgSearchAttrib.AllAddresses, it will work, but cast too wide a net.  If you change it to nsMsgSearchAttrib.BCC, it might do something wacky because the constant should not exist and then it's possible a much different search than expected gets run.

I see the search dialog (control-shift-F) does not include an explicit BCC option, so this makes me think my analysis is probably correct.

Comment 9

[Andrew Sutherland [:asuth] (he/him)]

bienvenu, I assume there's nothing stopping us from adding a recipients search predicate (to/cc/bcc) in addition to our all addressses one (from/to/cc/bcc)?

Comment 10

[john]

By "User Modification", I meant that I added a custom search term "BCC" [within the control-shift-F] which executed the search on BCC addresses as I would expect ....  From which I assumed adding/creating a "To/CC/BCC" to the predicate list wouldn't be a massively messy step.

Andrew's solution path in comment #9 also moots bug 763631

Comment 12

[:aceman]

Hey, I was interested in this just as a bug triager :)
But yes, if you decide I can just copy "TOorCC" to a new nsMsgSearchAttrib.Recipient and add BCC matching into it (copy the part from "AllAddresses"), then I think I am able to do it. It looks like the new option would also appear in the search dialog and as a Filter condition. That may not be a bad thing.
Is that what you meant?

Comment 13

[john]

1. Need to amend test at  ::
     http://mxr.mozilla.org/comm-central/source/mailnews/base/test/unit/test_searchAddressInAb.js

2a. Add after line 73 in http://mxr.mozilla.org/comm-central/source/mailnews/base/search/src/nsMsgSearchTerm.cpp  ::
2b. Add after liine 1104 in http://mxr.mozilla.org/comm-central/source/mailnews/base/search/src/nsMsgSearchAdapter.cpp  ::
    {nsMsgSearchAttrib::ToOrCCorBCC,     "to or cc or bcc"},

3.  Add in the region of http://mxr.mozilla.org/comm-central/source/mailnews/base/search/public/nsMsgSearchCore.idl ::
     an attribute type for TOorCCorBCC, and a number

4.  Modify line 1217 in http://mxr.mozilla.org/comm-central/source/mail/base/modules/quickFilterManager.js
     to the new attribute type TOorCCorBCC

However...where is the underlying code for managing the TOorCC attribute stored -- where one might describe/define a new TOorCCorBCC?  I am definitely missing something....

Comment 14

[john]

In the SEARCH AREA (Control-Shift-F) I note that some folks might want, with good reason, to retain the TOorCC functionality, independent of one for TOorCCorBCC, leaving -both- accessible from the drop down search . . . also retaining the existing "AllAddresses" (from/to/cc/bcc)

 . . . but still mapping your suggested new TOorCCorBCC to the recipient button in the QuickFilter area.

aceman, is that what you meant ?

Comment 15

[:aceman]

This is where ToOrCC is used: http://mxr.mozilla.org/comm-central/ident?i=ToOrCC . All those places would need a copy of ToOrCC to be ToOrCCOrBCC.

Yes, Ctrl-shift-F is the Search dialog I mentioned. The new SearchAttrib would appear there as a new option under "To or CC".

I just ask if that is what asuth wants. Make it a full blown new nsMsgSearchAttrib. I am just not sure if it is possible to insert it here http://mxr.mozilla.org/comm-central/source/mailnews/base/search/public/nsMsgSearchCore.idl#65 as a new number 9 and shift the rest.

Comment 16

[john]

A> I thought this....
...shift/renumber -and- reset the range of 'other'
...or alternatively, put it down by 52, moving 53 other and the range....


B> I need a learning curve, as I'm still thinking densely: 
I had looked there, in that cross reference URL, for all the places where ToOrCC is used, and identified my list, above, in comment #13, of places for changes.

What I still don't understand is which code makes the actual search of address fields [ie, -does- the boolean: "To or CC"].  All the code snippets I saw seem to either refer to or index that search.  I would have thought that I would have found a code snippet that -did- the search, from which one could copy/modify/add a new snippet for TOorCCorBCC

What am I missing ?

Comment 17

[:aceman]

I think this is the real code:
http://mxr.mozilla.org/comm-central/source/mailnews/base/search/src/nsMsgLocalSearch.cpp#453

Comment 18

[john]

5.   Add new blocks of code, to http://mxr.mozilla.org/comm-central/source/mailnews/base/search/src/nsMsgImapSearch.cpp

at lines 214, 368 and so on, to add blocks of TOorCCorBCC, to the existing online and offline tables

Comment 19

[john]

is there also code, for OnLine IMAP searches?  that one, at line 453, and in that file, has comments which imply that it is for POP and OffLine IMAP

Comment 20

[john]

never mind...brain viscosity, i see it, the substance of my comment #18, item 5

Comment 21

[:aceman]

Yes, it looks like that could be enough in mailnews/base/search/src/nsMsgImapSearch.cpp.

Are you going to make the patch? You should first wait reply from asuth and bienvenu if this is what they wanted.

Comment 22

[David :Bienvenu]

(In reply to Andrew Sutherland (:asuth) from comment #9)
> bienvenu, I assume there's nothing stopping us from adding a recipients
> search predicate (to/cc/bcc) in addition to our all addressses one
> (from/to/cc/bcc)?

No, nothing technical stopping us. We're not talking about exposing it in the UI, right? There's always grief when people find there is no bcc header in e-mail they receive :-(

Comment 23

[:aceman]

David, I assumed in comment 12 the new search match will appear in the UI if we just fully copy an existing ToOrCC definition...

Comment 24

[john]

I'm laughing: despite other spectacular programming efficiencies that go back to IBM 1620s, and move forward through Fortran, VBA HTML/CSS, and the like, I could write out what I -think- might work, but I am only reading, now, how to use pymake , making diff files, and etc, just to make the patch, not even getting close to making a working TBird with the patch, nor the modifications to what I think might be a test, per my item 1 in comment #13.   Plus, I think I would have to find what would be item 6, amend the nsMsgSearch.... reference for the Recipient button.

I probably could write code, and where it should go - but beyond that, I might melt something, either in my head, or the world at large.   Learning curve needed, first.

So, I laugh, asking: when you say "going to make the patch" . . . what do you mean, uhm....precisely.   I was merely ecstatic that the bug might be fixed, and looking through the code.   Be glad to help, now, how I can.

Comment 25

[:aceman]

Yes, your comments and looking through the code was highly spot on. So I thought you want to write the patch when you are already so far.

Comment 26

[Andrew Sutherland [:asuth] (he/him)]

(In reply to David :Bienvenu from comment #22)
> (In reply to Andrew Sutherland (:asuth) from comment #9)
> > bienvenu, I assume there's nothing stopping us from adding a recipients
> > search predicate (to/cc/bcc) in addition to our all addressses one
> > (from/to/cc/bcc)?
> 
> No, nothing technical stopping us. We're not talking about exposing it in
> the UI, right? There's always grief when people find there is no bcc header
> in e-mail they receive :-(

Blake?  My personal opinion is that our current phrasing of "recipients" that (should) cover to/cc/bcc seems good and is a sensible use-case.  Phrasing it as "To, Cc, or Bcc (when available)" seems viable for that UI.  I think that surfacing just BCC on its own in advanced search (control-shift-f) and message filters may be asking for trouble since people might think we are able to logically deduce something about the bcc list even if it's not present.

constants-wise, are you fine with renumbering or would you prefer that new constants get added at the end?

Comment 27

[Andrew Sutherland [:asuth] (he/him)]

er, blake, question for you in comment 26.  I mid-aired and forgot I was cc'ing...

Comment 28

[David :Bienvenu]

(In reply to Andrew Sutherland (:asuth) from comment #26)

> 
> constants-wise, are you fine with renumbering or would you prefer that new
> constants get added at the end?

renumbering is OK - the days where we persisted the constants are long gone.

Comment 29

[Thomas D. (:thomas8)]

Similar bug (which could perhaps be fixed here, too): Bug 535879

Comment 30

[Blake Winton (:bwinton) (:☕️)]

(In reply to Andrew Sutherland (:asuth) from comment #26)
> (In reply to David :Bienvenu from comment #22)
> > No, nothing technical stopping us. We're not talking about exposing it in
> > the UI, right? There's always grief when people find there is no bcc header
> > in e-mail they receive :-(
> Blake?  My personal opinion is that our current phrasing of "recipients"
> that (should) cover to/cc/bcc seems good and is a sensible use-case.

+1.

> Phrasing it as "To, Cc, or Bcc (when available)" seems viable for that UI. 
> I think that surfacing just BCC on its own in advanced search
> (control-shift-f) and message filters may be asking for trouble since people
> might think we are able to logically deduce something about the bcc list
> even if it's not present.

+1.

Comment 31

[john]

In reply to comment #30
> Phrasing it as "To, Cc, or Bcc (when available)" seems viable for that UI. 
> I think that surfacing just BCC on its own in advanced search
> (control-shift-f) and message filters may be asking for trouble since people
> might think we are able to logically deduce something about the bcc list
> even if it's not present.

There is a logical problem:  at first I thought these two suggestions might be appropriate:

1) replacing "...when available", with "To/Cc plus BCC you send", as more accurate
2) Also, since BCC filtering is useful, include a BCC UI case in the search, which could be titled similarly, as: "BCC you send".

However, BCC for logical "recipients" filters from two source: as mail SENT by the user, and also as mail RECEIVED as a BCC for just that user.

I wonder if the proposed changes will catch both BCC groups

Comment 32

[john]

(In reply to :aceman from comment #25)
> Yes, your comments and looking through the code was highly spot on. So I
> thought you want to write the patch when you are already so far.

Thanks, but....after looking through the steps to creating a patch [over and above suggesting coding], I note that I don't have a linux box, and have a broad learning curve to get a Try done, or a build, and on....

Can I just . . . watch and learn?

Comment 33

[:aceman]

Yes, no problem. I also do not guarantee I'll surely implement this all to finish. But I have done something like this already, so I'll try. The exact wording of the option visible in UI can be bashed on in parallel :)

David, what about this comment: 
// 34 - 43, reserved for ab / LDAP
Do I need to somehow shift some constants in ab / LDAP too? I couldn't find any.

Comment 34

[Thomas D. (:thomas8)]

I don't have time to spell out all the reasons in detail right now, but I strongly recommend the following:

1) make quick filter bar's "Recipients" button include BCC header for search results (without changing its caption)

2) in the code, name the new search term "Recipients", consistent with the existing nsMsgSearchAttribValues for "Subject" and "Sender" (not "From"), and also consistent with the quick filter buttons:
> const nsMsgSearchAttribValue Recipients = 10

3) While we are here, fix this:
Bug 535879 - filters: add "reply-to" header to the "all addresses" ("from, to, cc or bcc") filter search term

4) While we are here, add this:

> const nsMsgSearchAttribValue BCC = 8

5a) For Tools > Message Filters and Advanced Search (Ctrl+Shift+F), surface the following in the UI (existing entries unmarked, new entries marked with *, modified entries marked with #):

- BCC* (we definitely *do* want that exposed, and I don't see any potential for confusion here, it's no different or more confusing than CC which we already have, and both message filters and advanced search are for advanced users anyway)
- To or CC
- To, CC or BCC *
- From, Reply-To, To, Cc or Bcc # (Bug 535879)
(if we want less entries, we might skip "To or CC", but it might help to keep it, and it doesn't hurt)

5b) After fixing this bug, we might even consider natural language terms like "Recipients" for Message Filters and Advanced search, but not sure about that (is it possible to have a tooltip on dropdown list items? We could then use natural language terms and include the details in the tooltip)

Comment 35

[David :Bienvenu]

(In reply to :aceman from comment #33)

> David, what about this comment: 
> // 34 - 43, reserved for ab / LDAP
> Do I need to somehow shift some constants in ab / LDAP too? I couldn't find
> any.

I couldn't find them either - perhaps that's an obsolete comment. Mark might remember, or be able to point to ab search code.

Comment 36

[:aceman]

(In reply to Thomas D. from comment #34)
> I don't have time to spell out all the reasons in detail right now, but I
> strongly recommend the following:
> 
> 1) make quick filter bar's "Recipients" button include BCC header for search
> results (without changing its caption)
> 
> 2) in the code, name the new search term "Recipients", consistent with the
> existing nsMsgSearchAttribValues for "Subject" and "Sender" (not "From"),
> and also consistent with the quick filter buttons:
> > const nsMsgSearchAttribValue Recipients = 10
This is what I am going to do here.

> 3) While we are here, fix this:
> Bug 535879 - filters: add "reply-to" header to the "all addresses" ("from,
> to, cc or bcc") filter search term
This is not as trivial, I couldn't yet find out what code to just copy and tweak :) Let's keep that to bug 535879.

> 4) While we are here, add this:
> 
> > const nsMsgSearchAttribValue BCC = 8
> 
> 5a) For Tools > Message Filters and Advanced Search (Ctrl+Shift+F), surface
> the following in the UI (existing entries unmarked, new entries marked with
> *, modified entries marked with #):
> 
> - BCC* (we definitely *do* want that exposed, and I don't see any potential
> for confusion here, it's no different or more confusing than CC which we
> already have, and both message filters and advanced search are for advanced
> users anyway)

I think BCC is doable from existing code. Is there a bug for it?

> - To or CC
> - To, CC or BCC *
> - From, Reply-To, To, Cc or Bcc # (Bug 535879)
> (if we want less entries, we might skip "To or CC", but it might help to
> keep it, and it doesn't hurt)

When the previous 2 items are in place.

> 5b) After fixing this bug, we might even consider natural language terms
> like "Recipients" for Message Filters and Advanced search, but not sure
> about that (is it possible to have a tooltip on dropdown list items? We
> could then use natural language terms and include the details in the tooltip)

Maybe a tooltip is possible, I'll try.

Comment 37

[john]

> In reply to comment #30 and comment #31
On a little research / test case, I note that incoming mail, addressed to the USER via a BCC, does not appear to include any header information reflecting the USER, merely ";undisclosed recipients".   On the assumption that is driven by the sender's server, and not TBird, I amend my wondering to:

I wonder how, or if, TBird would/should regard mail, addressed to a USER via a BCC -- it doesn't appear to be addressed TO:, or searchable as such, nor as a BCC.

I assume it would not be caught by an AllAddressee search, in its current incarnation; while not a part of this bug #586101, I wonder if it is plausible to include RECEIVED-AS-BCC mail as a change to the boolean search for AllAddresses

Comment 38

[Thomas D. (:thomas8)]

(In reply to :aceman from comment #33)
> Yes, no problem. I also do not guarantee I'll surely implement this all to
> finish. But I have done something like this already, so I'll try. The exact
> wording of the option visible in UI can be bashed on in parallel :)
> 
> David, what about this comment: 
> // 34 - 43, reserved for ab / LDAP
> Do I need to somehow shift some constants in ab / LDAP too? I couldn't find
> any.

I think they just reserved 10 empty slots after the last nsMsgSearchAttribValue pertaining to AB card properties. My reading is that the values (numbers) of these slots are completely irrelevant, and we are shifting all the numbers anyway including
> const nsMsgSearchAttribValue OtherHeader
which must be the last entry, but the number is irrelevant as commented in the code. I suppose that the order of dropdown entries is derived from the numbers, so we can just keep a set of 10 empty slots numbers for AB at the current position, starting with whatever happens to be the number of
> const nsMsgSearchAttribValue Department
increased by +1 after our changes here. Just my reading, no guarantees.

Comment 39

[john]

aceman:

re the renumbering....
This doesn't seem to impact your bug patch, but you're further in to the renumbering issue, than I am, and you might think of something.

check out this commentary:
http://mxr.mozilla.org/comm-central/source/mailnews/base/search/src/nsMsgLocalSearch.cpp#629

Comment 40

[Thomas D. (:thomas8)]

(In reply to john ruskin from comment #37)

> I assume it would not be caught by an AllAddressee search, in its current
> incarnation; while not a part of this bug #586101, I wonder if it is
> plausible to include RECEIVED-AS-BCC mail as a change to the boolean search
> for AllAddresses

If that's possible at all, it's certainly non-trivial, and not part of this bug.
Here, we only look at explicit BCC headers found in source of mail, usually found in sent mail which might be moved/copied/fcc'ed to any folder including inbox.

Comment 41

[:aceman]

Attachment: WIP patch

OK, here is some preliminary patch for those eager to try something out and see how the whole feature looks in a patch:)
It does not yet update the test. I named the SearchAttribute::Recipient, which is open to discussion. Also the UI (search and filters) show "Recipient" which can easily be changed.
I have tested it on a local POP3 account. It appeared to me when using the "Recipients" in the quick filter bar that it matched also the "From" field, which would be strange. I had a test draft message having From: aceman@server, and BCC: aceman@server. Searching for "aceman" found this message even without the patch. I had to edit the message so that the From is somebody else. I am not sure why that it. Asuth?

Comment 42

[:aceman]

Unfortunately, I'll be away for a week, so will not work on the patch. But I will collect feedback after that :)
Thanks for all the useful hints here.

Comment 43

[john]

shot in the dark:  possible that the code uses 'recipients', and there is confusion between our "newer" meaning, and the code use at OLD LINE # 466 et seq [as was used in the AllAddresses] -- what is "getter_Copies(recipients) ... getting ?

Comment 44

[john]

In reply to comment #41
I think i mis-understood your comment
When you did your test search, did you test for BCC=aceman, with a search for some BODY=fooBar?   But, even if not:
If you searched for "Aceman", it may have found both the SENT folder copy, and the INBOX copy --> validly discovering FROM=aceman and BCC=aceman

Comment 45

[:aceman]

(In reply to john ruskin from comment #43)
> shot in the dark:  possible that the code uses 'recipients', and there is
> confusion between our "newer" meaning, and the code use at OLD LINE # 466 et
> seq [as was used in the AllAddresses] -- what is "getter_Copies(recipients)
> ... getting ?
I hope "recipients" in this case is just an arbitrary variable getting the results of GetRecipients(). It is used the same in ToOrCC so it should be correct. I assume it is the "To" field.


(In reply to john ruskin from comment #44)
> In reply to comment #41
> I think i mis-understood your comment
> When you did your test search, did you test for BCC=aceman, with a search
> for some BODY=fooBar?   But, even if not:
> If you searched for "Aceman", it may have found both the SENT folder copy,
> and the INBOX copy --> validly discovering FROM=aceman and BCC=aceman

I was using the quick filter bar above the message list. So I could not have 2 conditions like BCC=aceman OR BODY=fooBar. It was only Recipients=aceman, with recipients being the old meaning (=ToOrCC). It still found the msg having From=aceman and BCC=aceman. I searched only on Inbox, where I moved a draft message I composed for the test.

Comment 46

[Thomas D. (:thomas8)]

Attachment: Testcase1.eml

Comment 47

[Thomas D. (:thomas8)]

(In reply to :aceman from comment #45)

> I was using the quick filter bar above the message list. So I could not have
> 2 conditions like BCC=aceman OR BODY=fooBar. It was only Recipients=aceman,
> with recipients being the old meaning (=ToOrCC). It still found the msg
> having From=aceman and BCC=aceman. I searched only on Inbox, where I moved a
> draft message I composed for the test.

Aceman:
- could u pls include detailed STR for the false positive
- could u pls add that test msg as attachment, after doing exactly this to anonymize if you need to:
  - text editor, replace all instances of "aceman" with "spiderman"
  - replace yourmailserver.com with asdf.com
  - replace other private details, but ensure each different detail ends up with a different replacement (so yourserver1.com always becomes asdf1.com, etc.)

fwiw, I also had a false positive when testing quickfilter, but it was for "bodytext" where another msg was found which did not contain any trace of the word "bodytext" anywhere in source. but testcase of attachment 632594 [details] behaves as expected.

Comment 48

[Thomas D. (:thomas8)]

(In reply to Thomas D. from comment #47)
> (In reply to :aceman from comment #45)

Aceman, or simply forward as attachment to my private email address the original test msg which is found as false positive when recipient-filtering for aceman.

Comment 49

[:aceman]

Attachment: testcase 2

Comment 50

[:aceman]

Attachment: testcase 3

Comment 51

[:aceman]

Thomas, STRs here:
Import testcase 2 and testcase 3 into TB. Both messages are tweaked versions of your testcase.
Testcase 2 is:
BCC: [e-mail]
From: John Doe <[e-mail]>

Testcase 3 is:
BCC: [e-mail]
From: John Doe <[e-mail]>

(no other recipient headers like CC, reply-to etc.)

Now use the Quick filter bar in the folder where you imported the messages.
Select only to match on "Recipients".
Write string to search as "blindjohn".

Result:
Testcase 2 message is matched. WHY???

Expected result:
No match, "blindjohn" is not a recipient in the current base Thunderbird matching on "To or CC" only. He is the sender.

Test 2:
Search on "blindjohn" and choose on the "Sender" button (not "Recipients"). The same testcase 2 msg is matched. That is actually correct.

Thomas, can you confirm that?

Comment 52

[john]

Thomas and aceman:  I have noted false positives, in the past few days, when testing my BCC issues.

I posit that they may be connected to database issues [which seemed to cure some issues on folder compaction and repair in my case].   This idea mitigates in favor of test cases on nearly empty accounts?

Also, since IMAP/Server and POP/LocalIMAP are two different programming areas, I wonder what kind of account you all run the test cases suggested; equally, I wonder about the general test bed, if it tests into both areas...

Also:  just curious....was the only copy of the test message in the single folder where the email was found?  ie....you didn't send an email to yourself, merely imported the test eml to a single folder.

Comment 53

[john]

Attachment: Test2, with added TO: field

I added TO: [e-mail] to Test2, creating this Test2a
Placed Test2 and Test2a into a new folder in LocalFolders, to avoid any issue on database.   I ran this, obviously, in the code on POP/LocalIMAP

Searched for "asdf" in w/ RECIPIENT and retrieved BOTH emails -- expected ONE

Searched for "blindjohn" w/ RECIPIENT and retrieved only ONE email, TEST2 -- expected none

From this, I would suggest that the routine that locates recipients [deep beyond where aceman and I have been toying with code], looks for BCCs if there are no TOs.

I also think we all just discovered a fault in the RECIPIENT search, deep beyond where we're coding.  My bet is that both this fault, and the behavior in the prior paragraph, are in the same logic run, and that the original author contemplated the utility of looking for BCCs if there was no TO, but miscoded the logic -- just my guess.

Someone have a way to test these two test cases on SERVER-IMAP ?

Comment 54

[john]

AH....!   Using the search function, From/To/CC/BCC, for "asdf", also returns both emails.

So the defect occurs both with To/CC [currently mapped to Recipients button] and AllAddresses [mapped within the search drop down list]

See lines: 453 to 487 in
    http://mxr.mozilla.org/comm-central/source/mailnews/base/search/src/nsMsgLocalSearch.cpp#453

From this, I am thinking there is a defect in the logic that both rely on, which could either be the boolKeepGoing" and the coding that surrounds/uses it, or the logic deeper in, mentioned in comment #53.

My bet is the logic at lines 453 to 488, for the two cases where there are searches of multiple fields...

Comment 55

[john]

Learning Curve:  

msgToMatch->GetAuthor(getter_Copies(matchString));
msgToMatch->GetCcList(getter_Copies(ccList));
msgToMatch->GetBccList(getter_Copies(bccList));
msgToMatch->GetRecipients(getter_Copies(recipients));

I am having trouble deciphering what gets executed and why:  I am looking at
http://mxr.mozilla.org/comm-central/source/mailnews/db/msgdb/src/nsMsgHdr.cpp#640
lines 640 to 663

Also looking at the likes of this range of code:
http://mxr.mozilla.org/comm-central/source/mailnews/base/util/nsMsgIdentity.cpp#184
thru line 196
and line 193 piques my interest, but the coding is lost to me.   I take in inferences that the code looks further than mere BCClists, which makes me look at our error and wonder ...

Comment 56

[john]

Should Comment #53 and testcase Test2a [Test2 with added TO: field]

be identified as a new bug ?

Comment 57

[Thomas D. (:thomas8)]

(In reply to :aceman from comment #51)
> Testcase 2 is:
> BCC: [e-mail]
> From: John Doe <[e-mail]>
> 
> Testcase 3 is:
> BCC: [e-mail]
> From: John Doe <[e-mail]>
> 
> Result:
> Testcase 2 message is matched. WHY???
> Thomas, can you confirm that?

Yes, confirming for both tests as described in comment 51.
I think analysis of John's comment 53 is right:

If case of:
To: tojohn AND
BCC: blindjohn
...searching for Recipients-only: blindjohn returns nothing, because To: header takes precedence (and searching bcc isn't fully implemented, this bug)

If case of:
BCC: blindjohn (without To-header etc.)
...searching for Recipients-only: blindjohn returns this case, if To: header is missing, there seems to be some fallback code logic (or bug) to look into BCC header instead. I would try to get some experts on board and make this consistent here, not much point of filing that as a bug, because we *want* this behaviour, only more reliable and consistent.

It's a bit less surprising if you consider that Thunderbird is a wild and untamed bird, and the birdmen are few ;)

Comment 58

[Thomas D. (:thomas8)]

(In reply to :aceman from comment #50)
> Created attachment 632645 [details]
> testcase 3

Aceman, thanks for these useful testcases. They would be even greater if the subject of test mails indicated the testcase # and some abbreviated hints about the specifics of that testcase (but ensure hints in subjects are not creating false positives that spoil the test). It's kinda hard right now to tell testcases apart.

Comment 59

[john]

Attachment: TestCase2a with TO: field [uploaded into plain text]

see comment #53
This File effects the same text case, 2a, as obsolete one
This time: Uploaded in plain text, with BODY explaining use

Comment 60

[David :Bienvenu]

Comment on attachment 632438 [details] [diff] [review]
WIP patch

looks like a good start. I'm still not clear on the ldap/addrbook gap comment

Comment 61

[john]

In reply to Comment #60:
> I'm still not clear on the ldap/addrbook gap comment

David: The code [for the drop down list in search] included a gap of 10 numbers.  

See: http://mxr.mozilla.org/comm-central/source/mailnews/base/search/public/nsMsgSearchCore.idl#93

Our collective assumption is that -no- code, anywhere else in the TBird code, expects those numbers to be reserved/unused; if that's true, then aceman's code is ok.   Otherwise, it needs massaging.

Comment 62

[David :Bienvenu]

(In reply to john :ruskin from comment #61)

> Our collective assumption is that -no- code, anywhere else in the TBird
> code, expects those numbers to be reserved/unused; if that's true, then
> aceman's code is ok.   Otherwise, it needs massaging.

Right, that's what I'm not sure about, sorry to be unclear.

Comment 63

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 632438 [details] [diff] [review]
WIP patch

The UI seems good to me.  f+.

Later,
Blake.

Comment 64

[:aceman]

Comment on attachment 632438 [details] [diff] [review]
WIP patch

rkent, what do you think of the patch here? And what about the concerns raised in previous comments? Bienvenu and john ruskin.

Comment 65

[Thomas D. (:thomas8)]

(In reply to :aceman from comment #64)
> Comment on attachment 632438 [details] [diff] [review]
> WIP patch
> 
> rkent, what do you think of the patch here? And what about the concerns
> raised in previous comments? Bienvenu and john ruskin.

(In reply to David :Bienvenu from comment #62)
> (In reply to john :ruskin from comment #61)
> 
> > See: http://mxr.mozilla.org/comm-central/source/mailnews/base/search/public
> > /nsMsgSearchCore.idl#93
> > 
> > Our collective assumption is that -no- code, anywhere else in the TBird
> > code, expects those numbers to be reserved/unused; if that's true, then
> > aceman's code is ok.   Otherwise, it needs massaging.
> 
> Right, that's what I'm not sure about, sorry to be unclear.

An observation to confirm that those 10 slots currently reserved for AB/LDAP are random (and we can use them as we please):

http://hg.mozilla.org/releases/comm-1.9.1/rev/6bdc9d138c65#l3.31

Before first introduction of AllAddresses nsMsgSearchAttribValue, we had:
// 33 - 43, reserved for ab / LDAP;
After introduction of AllAddresses, we had:
// 33 - 42, reserved for ab / LDAP;
Now, we have:
// 34 - 43, reserved for ab / LDAP;

Also, AllAddresses was introduced as 43, and now it's 9. The highest value for "Customize" changed from 49 to 52. The numbers really don't matter, nor the position of the free slots, nor their number, nor their existence.

Comment 66

[Thomas D. (:thomas8)]

Numbers (Values of nsMsgSearchAttrib) are relevant only for one purpose:
http://mxr.mozilla.org/comm-central/source/mailnews/base/search/public/nsMsgSearchCore.idl#47

> * Definitions of search attribute types. The numerical order
> * from here will also be used to determine the order that the
> * attributes display in the filter editor.

So the purpose of the reserved slots is to reserve some numbers where AB/LDAP search expressions for future use can be easily inserted without tweaking all the other numbers, while getting them displayed in correct order.

Comment 67

[Kent James (:rkent)]

Comment on attachment 632438 [details] [diff] [review]
WIP patch

I'm not quite sure what feedback+ or feedback- means, so I'll do a feedback+ to be cheery though I have some reservations. I did not do a thorough code review, only conceptual.

In some of the work I did earlier on BCC search, one of the main reservations has always been that very few users really understand BCC. In particular, it is impossible to search for BCC on an incoming email by design, so a BCC search is really only valid in the very narrow case of messages that have been sent by you. Any explicit exposure therefore of "BCC" in a search term is likely to mislead the vast majority of users.

Now you have not exactly done that here, but instead are now exposing both a "Recipients" as well as a "ToOrCc" search term in the UI. Those are identical, except in the narrow case of message sent by you. That is a subtle difference that will not be understood by the average user. I also have the secondary concern that an excessively long list of search terms is confusing to users, so we should be cautious about adding search terms exposed to the UI in the main code. Instead, use extensions (such as my FiltaQuilla) to expose rarely used search terms. That is the whole point of allowing custom search terms.

What I would support is extending the "ToOrCc" search term to include Bcc as well, and rename it in the UI to "Recipients". That way you are not adding any new search terms to the UI. I can't imagine a common use for searching ToOrCc but not searching Bcc as well, but if such a case exists it could be handled in an extension.

Unfortunately you then have the problem of backwards compatibility with existing saved searches and filters. You'll probably need to keep the search term label named "ToOrCc" to avoid breaking existing filters and saved searches.

Comment 68

[:aceman]

That would simplify things a lot. Who should we ask for objections?

Comment 69

[Kent James (:rkent)]

(In reply to :aceman from comment #68)
> That would simplify things a lot. Who should we ask for objections?

Just let the comment stand for a day or so, there are plenty of cc's to this bug already who can express opinions.

Comment 70

[Thomas D. (:thomas8)]

(In reply to Kent James (:rkent) from comment #67)
> Comment on attachment 632438 [details] [diff] [review]
> WIP patch
>
> Any explicit exposure therefore of
> "BCC" in a search term is likely to mislead the vast majority of users.

I think it also depends where and how you expose it, and I have some doubts if hiding "BCC" makes that problem go away or if we are actually manifesting the problem of uneducated users, while removing relevant information for advanced users. For example, a user who really doesn't understand or even know bcc might still think he can find *all* mail to himself with ToOrCC filter, even if we don't mention BCC.

> Now you have not exactly done that here, but instead are now exposing both a
> "Recipients" as well as a "ToOrCc" search term in the UI. Those are
> identical, except in the narrow case of message sent by you. That is a
> subtle difference that will not be understood by the average user.

Both Advanced Search and Message Filters are for advanced users.
I cannot imagine any average user trying to set up a message filter. Imo average users are also very unlikey to even find advanced search, and much less use it. More so after we'll be hiding all the menus in a menu button which will require even more steps to discover this search. Even as an advanced user, I rarely use advanced search, because Quick Filter and Global Search do the trick for 90% of my use cases. (Fwiw, I have a few saved searches and message filters set up).
If Advanced Search was designed for the average user, I'd assume it would not be so blatantly neglected in ux and functionality as it currently is (no message preview, no context menu, missing actions for found messages etc.).

> I also
> have the secondary concern that an excessively long list of search terms is
> confusing to users, so we should be cautious about adding search terms
> exposed to the UI in the main code. Instead, use extensions (such as my
> FiltaQuilla) to expose rarely used search terms. That is the whole point of
> allowing custom search terms.

Crucial functionality should not be left to addons.
Anything search is crucial.
If we drop ToOrCC from the list, there will be no way to add the same amount of functionality back without Addons.
Due to our currently limited UI, this will prevent most advanced searches that for whatever reason want to *exclude* BCC from searching. Doing To and BCC as separate conditions is not the same, because you need OR for that, which means you cannot add any other conditions with AND.

> What I would support is extending the "ToOrCc" search term to include Bcc as
> well, and rename it in the UI to "Recipients". That way you are not adding
> any new search terms to the UI. I can't imagine a common use for searching
> ToOrCc but not searching Bcc as well, but if such a case exists it could be
> handled in an extension.

I'm a bit worried about UI consistency here.
We already have the following strings in the UI for advanced search & message filters:

From
To
To or CC
From, To, CC or BCC

Regardless if we keep To or CC in that list or not, just adding "Recipients" to that list (for advanced users!) is really odd (inconsistent) and fuzzy:

Stringlist C (current patch)

From
To
[To or CC]
Recipients
From, To, CC or BCC

For the sake of consistency, either change all of those terms to natural language, or keep all of them technical, but don't switch language register at random. 

Stringlist B (natural terms):

Sender
To
[To or CC]
Recipients
Sender or Recipients

Btw I think Recipients (plural) is slightly better than Recipient (singular); it's also consistent with Quick filter buttons.

Ideally (if it's technically possible - is it?), combine the best of both worlds:
Use natural language terms for the list entries and add details in a tooltip for each list entry (combines . If that's not possible, stay with technical terms:

Stringlist A: technical terms

From
To
[To or CC]
To, CC or BCC
From, To, CC or BCC

Given that both filters and advanced search are for more advanced users, I think it's helpful to have precise information what this filter does.

> Unfortunately you then have the problem of backwards compatibility with
> existing saved searches and filters. You'll probably need to keep the search
> term label named "ToOrCc" to avoid breaking existing filters and saved
> searches.

To use ToOrCC where we really mean ToOrCCOrBCC is asking for trouble.
I strongly advise against such foul compromises. In a few years time, coders will curse us for not knowing if ToOrCC includes BCC or not.
Names should be telling, and they should do what they say, not more, not less.
And well, I wouldn't remove nor alter the meaning of nsMsgSearchAttrib ToOrCC, because altering the meaning might also break the logic of existing filters and saved searches.

Comment 71

[Kent James (:rkent)]

Thomas,

I've given my feedback, so I think I will leave it to others to respond to your points.

I do however take strong objection to this:

"Crucial functionality should not be left to addons.
Anything search is crucial."

There is nothing about fine parsing of BCC search that is crucial. You can't both argue that this feature is only for advanced users, and it is also crucial.

And a main value of addons is to prevent clutter of the main user interface by rarely used features. I would certainly say that wanting to search To and CC but not BCC is a rarely used feature. Just because it is "search" cannot be used to trump all arguments for its inclusion.

Comment 72

[Kent James (:rkent)]

Comment on attachment 632438 [details] [diff] [review]
WIP patch

Forget to set the flag.

Comment 73

[:aceman]

So what do we do now?

Comment 74

[José Fortes]

I was to record a bug for this precise thread description. However, after browsing the old discussion, it's not clear to me what does "assigned" currently mean. I understand that comment 34 proposal was essentially implemented years ago, and in fact I've been using this feature (locating recipients, even in BCC, in the Sent folder with Quick Filter) until very recently, when it stopped including BCC recipients.

So, I would like to propose to review this later change in behavior as a bug. I find a very useful feature to locate with "Recipients" even in BCC in the Sent folder, since because of the different interface using the alternative Filter is cumbersome and less practical in order to find a particular message.

Of course my gratitude to all that work on your part as well as your attention. Best regards.

Comment 75

[:aceman]

Yes, the bug is still assigned to me and I could finish it. I don't think any part of it has landed yet. But it appears to me there were some disagreement on how the feature actually should work (or be labeled in the UI). So the work has stalled. If any body can say how to proceed now, I could continue on the patch.

Comment 76

[José Fortes]

(In reply to :aceman from comment #75)
> Yes, the bug is still assigned to me and I could finish it. I don't think
> any part of it has landed yet. But it appears to me there were some
> disagreement on how the feature actually should work (or be labeled in the
> UI). So the work has stalled. If any body can say how to proceed now, I
> could continue on the patch.

If it helps, in my opinion it worked well until recently, so just including back BCC within Recipients for the Sent folder in Quick Filter would be fine!

Comment 77

[Kent James (:rkent)]

(In reply to José Fortes from comment #76)
> 
> If it helps, in my opinion it worked well until recently, so just including
> back BCC within Recipients for the Sent folder in Quick Filter would be fine!

Looking at the patch again, the discussion got complex because the proposal was to add an additional search term, then we argued over that, as that would also be exposed in the already-too-complex advanced search.

If the only goal here is to add BCC to the recipients in quick filter search, there is no reason that the same term has to be added to the advanced search dialogs. I believe that you could add the Recipients term as in the patch, but set Available=false, and then it would not show in the dialog. I believe that as long as enabled is true, then the search term would still work in quick filter. I'd have to check the code carefully to be sure this is true, as this stuff has very little documentation, but that would allow you to fix Quick Filter, adding BCC to Recipients there (which I don't believe is controversial) while also avoiding adding the same term to the advanced search dialog (which has From, To, CC, or BCC available as a reasonable alternative).

The other option would be to do quick search recipients with a custom search term. That would be easy to implement, but might have performance issues. C++ would be better, this is performance sensitive code.

Comment 78

[Thomas D. (:thomas8)]

(In reply to Kent James (:rkent) from comment #77)
> (In reply to José Fortes from comment #76)
> > 
> > If it helps, in my opinion it worked well until recently, so just including
> > back BCC within Recipients for the Sent folder in Quick Filter would be fine!
> 
> Looking at the patch again, the discussion got complex because the proposal
> was to add an additional search term, then we argued over that, as that
> would also be exposed in the already-too-complex advanced search.

Yeah, I agree that the current dropdown list of filter criteria is complex, messy, and unintuitive. 
In bug 1127267, I propose a complete makeover of that list, also to incorporate this bug 586101 and its two siblings:

Bug 586101 - Add "BCC" to "To or CC" Filter (=> "Recipient")
Bug 600928 - Add "Reply-To" to "From" Filter (=> "Sender")
Bug 535879 - Add "Reply-To" to "From, To, CC, or BCC" Filter (=> "Sender or Recipient")

> If the only goal here is to add BCC to the recipients in quick filter
> search, there is no reason that the same term has to be added to the
> advanced search dialogs. I believe that you could add the Recipients term as
> in the patch, but set Available=false, and then it would not show in the
> dialog. I believe that as long as enabled is true, then the search term
> would still work in quick filter. I'd have to check the code carefully to be
> sure this is true, as this stuff has very little documentation, but that
> would allow you to fix Quick Filter, adding BCC to Recipients there (which I
> don't believe is controversial)

+1, sounds good to get rolling again, thanks.

> while also avoiding adding the same term to
> the advanced search dialog (which has From, To, CC, or BCC available as a
> reasonable alternative).

My proposal of Bug 1127267 attachment 8556401 [details] [1] seeks to address Kent's concerns about adding more complexity to the criteria list in a complete structural makeover. It also creates ux-consistency of both search behaviour and labelling between "Quick Filter Bar" and Advanced Search / Filters, for the benefit of both simple and advanced users.

[1] To view the XUL demo of attachment 8556401 [details] in FF:
- Install Remote XUL Manager:
  https://addons.mozilla.org/en/firefox/addon/remote-xul-manager/
- add a permission for bugzilla.mozilla.org (and <file> for local xul files): about:addons > Remote XUL Manager > Settings button

Comment 79

[Thomas D. (:thomas8)]

See also: [Bug 522886] Implement "Recipients" column that shows all recipients (To, CC, and BCC)
I think that's currently stalled because we weren't sure how to transition the underlying database now that we want "Recipients" field to actually/correctly contain all recipient types instead of only to-Recipients.

Comment 80

[Wayne Mery (:wsmwk)]

(In reply to :aceman from comment #75)
> Yes, the bug is still assigned to me and I could finish it. I don't think
> any part of it has landed yet. But it appears to me there were some
> disagreement on how the feature actually should work (or be labeled in the
> UI). So the work has stalled. If any body can say how to proceed now, I
> could continue on the patch.

aceman, I'd say go for simplicity

Comment 81

[Wayne Mery (:wsmwk)]

Aceman is waiting for guidance. So getting back to comment 77, my only interest is in quickfiter being fixed to get some relief, and have zero interest in the side issues which generated so much discussion (and I don't plan to be involved in much more) and add complexity.  

Can we move on with the patch (which has several feedback+)?  (I'm not sure who to ask here, so taking a shot in the dark with magnus)

Comment 82

[:aceman]

Would the situation here be improved by bug 1127267, by changing the labels and reorganizing the list of terms?

Comment 83

[Magnus Melin [:mkmelin]]

The patch looks ok. I would do what it has, and not fold the bcc case into ToOrCc.

Comment 84

[Magnus Melin [:mkmelin]]

(In reply to :aceman from comment #82)
> Would the situation here be improved by bug 1127267, by changing the labels
> and reorganizing the list of terms?

I think so.

Comment 85

[Kent James (:rkent)]

"Would the situation here be improved by bug 1127267, by changing the labels and reorganizing the list of terms?"

I like the term "Recipients" for "To, CC, and BCC" but the whole proposal adds 4 additional search terms to an already complex table, plus directly exposes BCC as a search term, both of which I am against. It seems to be driven also by "Crucial functionality should not be left to addons. Anything search is crucial." which I have also opposed. So on the whole I don't like that. If you really wanted to implement something like bug 1127267, perhaps you could have an initial "Basic Search" list, and an "Advanced" button that pops up all of the other options. Merely reordering options helps a little but not enough to overcome the problems of adding 4 additional terms.

Looking over the around 10 requests in FiltaQuilla for new search terms, none have mentioned BCC. Most of these would require significant backend work to implement, like "Search for the existence of a header" or "Search for messages in which any message in the thread has been starred". Broken Body search is also a critical problem, and that (plus exposing attachment type and name to search) are the important issues worth working on.

This is only my opinion, and I do not have a veto over this. Please do whatever the majority of contributors want.

Comment 86

[:aceman]

(In reply to Kent James (:rkent) from comment #85)
> If you really wanted to implement something like bug 1127267, perhaps
> you could have an initial "Basic Search" list, and an "Advanced" button that
> pops up all of the other options.

This can be done easily, I have a WIP patch ready. It has only one cosmetic problem that when you click the "Show Advanced items" item (which will be an item in the field menulist, similar to "Customize"), the popup closes. You need to reopen it to see the advanced items. I couldn't find a way to keep the menupopup open after running the command handler. stopPropagation() nor preventDefault seem to work. But we can polish that in the other bug.

Comment 87

[seans]

Sorry to jump in, but it is really hard to follow this comment thread and the referenced bugs.

It appears that there is still no way to use the quickfilter to find an email with an email address in the Bcc field as of 52.1.1.

Is there going to be a fix that will allow quick filtering to catch Bcc?

Comment 89

[Thom]

This is still happening in 78.3.1. When searching through the body aswell, bcc adresses are found. But in big folders Thunderbird can get really slow if it searches through the body aswell.

Comment 90

[alta88]

TotalQuickFilter implements filtering for All Addresses, which includes Bcc.

Comment 91

[Thom]

Alright, but we should not expect all users to install that to fix this bug :)

Comment 92

[alta88]

No, you shouldn't. But it is very unlikely any resources will be allocated to this bug, imo. So there you are ;)

Comment 93

[Thom]

Thanks, I will look into it for sure!