187771 - junk mail controls should whitelist on multiple address books

Status: RESOLVED FIXED

(MailNews Core :: Filters, enhancement)

Description

[xolaware.llc]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X; en-US; rv:1.3b) Gecko/20030103
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X; en-US; rv:1.3b) Gecko/20030103

when choosing "Junk Mail Controls..." the "Do not mark messages as junk if the
sender is in my address book:" should allow a set of checkboxes for all address
books, not just a menu choice of one address book.  or, at the very least, add
the option of "all".


Reproducible: Always

Steps to Reproduce:
1. create multiple address books
2. choose "Junk Mail Controls..." from the Tools menu
Actual Results:  
a menu allowing the choice of one address book

Expected Results:  
checkboxes beside each potential address book the user may want to allow mail to
appear from as "not junk".

Comment 1

[laurel]

The junk mail controls feature is still being revised... there are plans to
implement selection of multiple Address Books.  Spec (not final yet) in progress
at http://www.mozilla.org/mailnews/specs/spam/

Comment 2

[Samir Gehani]

Mail triage team: nsbeta1-

Comment 3

[(not reading, please use [e-mail] instead)]

mass re-assign.

Comment 4

[Sander]

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

Comment 5

[Sander]

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

Comment 6

[R.K.Aa.]

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

Comment 7

[Mike Cowperthwaite]

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

Comment 8

[David Hagood]

You may wish to re-think the change in the summary - it might not be a bad idea
to allow the mail controls to *blacklist* on an address book, as well.

Comment 9

[Matthias Versen [:Matti]]

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

Comment 10

[Adam Guthrie]

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

Comment 11

[Wayne Mery (:wsmwk)]

(David wrote in comment #8)
> You may wish to re-think the change in the summary - it might not be a bad idea
> to allow the mail controls to *blacklist* on an address book, as well.

not a bad idea, but shouldn't that be done with a seperate bug request 


I fail to see how this blocks bug 180012

Comment 12

[zug_treno]

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

Comment 13

[Andrew Schultz]

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

Comment 14

[David :Bienvenu]

there's no blocking relationship that I can see here.

Comment 15

[Patrizio Bassi]

same issue in tb 1.5.0, i do agree with topic starter.

Comment 16

[Ian Neal]

Attachment: First draft of patch v0.1

This patch:
* Changes am-junk UI to allow for multiple local address books to be used for whitelisting.
* Changes back end to look through each selected local address book to see if email address has been whitelisted.

Comment 20

[Ian Neal]

Attachment: Using onSave patch v0.2

Changes since v0.1:
* use OnSave rather than changing using onClick (which didn't capture changes made via keyboard) for junk UI
* backend now caches all valid directories into an array

Comment 21

[[e-mail]]

Comment on attachment 257882 [details] [diff] [review]
Using onSave patch v0.2

>+    var currentArray = wlValue.split(",");
I'm not sure that you can guarantee that URIs don't contain commas?
Maybe spaces would be safer.

>+    var wlNode;
>+    for (var i = 0; i < wList.childNodes.length; i++)
>+    {
>+      wlNode = wList.childNodes[i];
Nit: might as well declare var wlNode inside the loop.

>+     }
Nit: indentation wrong.

>+  var newArray = new Array();
Nit: var wlArray = []; (newArray is an awful name for a variable).

>+  <vbox>
>+    <hbox flex="1">
Nit: any reason not to just use <hbox> (no flex) here?

>+      PRInt32 index = -1;
>+      while (++index < whiteListArray.Count())
Use a for loop please.

>+        nsCOMPtr<nsIRDFResource> resource;
>+        rv = rdfService->GetResource(*whiteListArray[index], getter_AddRefs(resource));
>+        NS_ENSURE_SUCCESS(rv, rv);
>+
>+        nsCOMPtr<nsIAbMDBDirectory> whiteListDirectory = do_QueryInterface(resource, &rv);
>+        NS_ENSURE_SUCCESS(rv, rv);
>+        if (whiteListDirectory)
>+          whiteListDirArray.AppendObject(whiteListDirectory);
No need to ENSURE these. If something goes wrong then we just don't append.

>-    // if we can't get the db, we probably want to continue firing spam filters.
Put that comment back in to remind you ;-)

>+  if (useWhiteList || !trustedMailDomains.IsEmpty())
I think we should use whiteListDirArray.Count() != 0 instead of useWhiteList.

>+          PRInt32 index = -1;
>+          while (++index < whiteListDirArray.Count() && !cardExists)
>+            rv = whiteListDirArray[index]->HasCardForEmailAddress(authorEmailAddress, &cardExists);
Another for loop please. Also, you should not exit the loop if an address book returns a failure code while claiming the card exists.

Comment 22

[[e-mail]]

Comment on attachment 257882 [details] [diff] [review]
Using onSave patch v0.2

Oh, and the list doesn't refresh properly when switching between junk settings for two different accounts.

Comment 23

[Ian Neal]

Attachment: Space instead of comma patch v0.3

Changes since v0.2:
* Use comma to space as the separator
* Process value even when it's empty so checkboxes get set correctly.

Comment 24

[Ian Neal]

(In reply to comment #23)
> Created an attachment (id=262816) [details]
> Space instead of comma patch v0.3
> 
> Changes since v0.2:
> * Use comma to space as the separator
Of course I meant:
* Use space instead of comma as the separator

Comment 25

[[e-mail]]

Comment on attachment 262816 [details] [diff] [review]
Space instead of comma patch v0.3

>           PRBool cardExists = PR_FALSE;
>           // don't want to abort the rest of the scoring.
>           if (!authorEmailAddress.IsEmpty())
>-            rv = whiteListDirectory->HasCardForEmailAddress(authorEmailAddress, &cardExists);
>+        {
>+          for (PRInt32 index = 0; index < whiteListDirArray.Count() && !cardExists; index++)
>+            rv = whiteListDirArray[index]->HasCardForEmailAddress(authorEmailAddress, &cardExists);
>+            if (NS_FAILED(rv))
>+              cardExists = PR_FALSE;
>+        }
>           if (NS_SUCCEEDED(rv) && cardExists)
Nit: don't need the NS_SUCCEEDED any more.

Comment 26

[[e-mail]]

Comment on attachment 262816 [details] [diff] [review]
Space instead of comma patch v0.3

>+  var wlValue = "";
>+  if (document.getElementById("server.useWhiteList").checked)
>+    wlValue = document.getElementById("server.whiteListAbURI").value;
>+
>+  var currentArray = wlValue.split(" ");
I think this should say
var currentArray = [];
if (...checked)
  currentArray = ...value.split(" ");

>+  for (var i = 0; i < wList.childNodes.length; i++)
>+  {
>+    var wlNode = wList.childNodes[i];
>+    if (wlNode.localName == "listitem")
>+      wlNode.checked = (currentArray.indexOf(wlNode.id) > -1);
>+  }
You could alternatively use wList.getRowCount() and wList.getItemAtIndex(i)
thus saving you the listitem check.

>Index: suite/locales/en-US/chrome/mailnews/pref/am-junk.dtd
You forgot the TB file in this patch ;-)

Comment 27

[Ian Neal]

Attachment: listitem checkless patch v0.3a

Changes from v0.3:
* use getRowCount/getItemAtIndex instead of listitem checking
* included missed TB dtd diff

Carrying forward r= and requesting sr=

Comment 28

[Scott MacGregor]

Hi Ian, can you attach a screen shot?

Comment 29

[Ian Neal]

Attachment: Screenshot of patched pref panel

This is how the new pref panel looks for Junk Settings

Comment 30

[Scott MacGregor]

Comment on attachment 262929 [details] [diff] [review]
listitem checkless patch v0.3a

the patch looks good. One UI thought..

What do you think about removing the tree col that says "Address books"? I don't think it provides any extra information.

Comment 31

[Ian Neal]

Attachment: listheaderless patch v0.3b

Changes from v0.3a:
* Removed listheader (and associated entities) as suggested by mscott

Carried forward r/sr

Checking in (trunk)
mailnews/base/prefs/resources/content/am-junk.js;
new revision: 1.11; previous revision: 1.10
mailnews/base/prefs/resources/content/am-junk.xul;
new revision: 1.5; previous revision: 1.4
mailnews/base/util/nsMsgDBFolder.cpp;
new revision: 1.302; previous revision: 1.301
done

Comment 35

[Thomas Landauer]

What's the status on this one now? I have Thunderbird 2.0.0.12, and the issue isn't solved there. Do I have to install the patch manually? Don't know how this works? - Please explain somebody - thanks!! :-)

Comment 36

[Samuel Sidler (old account; do not CC)]

This will be fixed in Thunderbird 3 (or an alpha of it).