Closed Bug 187771 Opened 22 years ago Closed 17 years ago

junk mail controls should whitelist on multiple address books

Categories

(MailNews Core :: Filters, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: xolaware.llc, Assigned: iannbugzilla)

References

Details

Attachments

(2 files, 4 obsolete files)

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".
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/
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: nsbeta1
Mail triage team: nsbeta1-
Keywords: nsbeta1-
Keywords: nsbeta1
mass re-assign.
Assignee: naving → sspitzer
OS: MacOS X → All
Hardware: Macintosh → All
*** Bug 192349 has been marked as a duplicate of this bug. ***
*** Bug 207153 has been marked as a duplicate of this bug. ***
Blocks: 209551
*** Bug 237850 has been marked as a duplicate of this bug. ***
Product: MailNews → Core
Blocks: 180012
No longer blocks: 209551
Summary: junk mail controls should allow multiple address books → junk mail controls should whitelist on multiple address books
*** Bug 221942 has been marked as a duplicate of this bug. ***
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.

*** Bug 291661 has been marked as a duplicate of this bug. ***
*** Bug 309929 has been marked as a duplicate of this bug. ***
(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

*** Bug 323179 has been marked as a duplicate of this bug. ***
*** Bug 324007 has been marked as a duplicate of this bug. ***
there's no blocking relationship that I can see here.
No longer blocks: 180012
same issue in tb 1.5.0, i do agree with topic starter.
Attached patch First draft of patch v0.1 (obsolete) — Splinter Review
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.
Assignee: sspitzer → iann_bugzilla
Status: NEW → ASSIGNED
Attachment #248800 - Flags: review?(neil)
Attachment #248800 - Flags: review?(neil)
Attached patch Using onSave patch v0.2 (obsolete) — Splinter Review
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
Attachment #248800 - Attachment is obsolete: true
Attachment #257882 - Flags: review?(neil)
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.
Attachment #257882 - Flags: review?(neil) → review-
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.
Changes since v0.2:
* Use comma to space as the separator
* Process value even when it's empty so checkboxes get set correctly.
Attachment #257882 - Attachment is obsolete: true
Attachment #262816 - Flags: review?(neil)
(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 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 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 ;-)
Attachment #262816 - Flags: review?(neil) → review+
Attached patch listitem checkless patch v0.3a (obsolete) — Splinter Review
Changes from v0.3:
* use getRowCount/getItemAtIndex instead of listitem checking
* included missed TB dtd diff

Carrying forward r= and requesting sr=
Attachment #262816 - Attachment is obsolete: true
Attachment #262929 - Flags: superreview?(mscott)
Attachment #262929 - Flags: review+
Hi Ian, can you attach a screen shot?
This is how the new pref panel looks for Junk Settings
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.
Attachment #262929 - Flags: superreview?(mscott) → superreview+
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
Attachment #262929 - Attachment is obsolete: true
Attachment #263307 - Flags: superreview+
Attachment #263307 - Flags: review+
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
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!! :-)
This will be fixed in Thunderbird 3 (or an alpha of it).
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: