junk mail controls should whitelist on multiple address books

RESOLVED FIXED

Status

MailNews Core
Filters
--
enhancement
RESOLVED FIXED
15 years ago
10 years ago

People

(Reporter: xolaware.llc, Assigned: Ian Neal)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

15 years ago
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

15 years ago
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

Updated

15 years ago
Keywords: nsbeta1

Comment 2

15 years ago
Mail triage team: nsbeta1-
Keywords: nsbeta1-

Updated

15 years ago
Keywords: nsbeta1
mass re-assign.
Assignee: naving → sspitzer

Updated

15 years ago
OS: MacOS X → All
Hardware: Macintosh → All

Comment 4

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

Comment 5

15 years ago
*** Bug 207153 has been marked as a duplicate of this bug. ***
(Assignee)

Updated

15 years ago
Blocks: 209551

Comment 6

14 years ago
*** Bug 237850 has been marked as a duplicate of this bug. ***
Product: MailNews → Core

Updated

13 years ago
Blocks: 180012

Updated

13 years ago
No longer blocks: 209551

Updated

13 years ago
Summary: junk mail controls should allow multiple address books → junk mail controls should whitelist on multiple address books

Comment 7

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

Comment 8

13 years ago
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. ***

Comment 10

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

Comment 11

12 years ago
(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

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

Comment 13

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

Comment 14

12 years ago
there's no blocking relationship that I can see here.
No longer blocks: 180012

Comment 15

12 years ago
same issue in tb 1.5.0, i do agree with topic starter.
(Assignee)

Comment 16

11 years ago
Created attachment 248800 [details] [diff] [review]
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.
Assignee: sspitzer → iann_bugzilla
Status: NEW → ASSIGNED
Attachment #248800 - Flags: review?(neil)

Updated

11 years ago
Duplicate of this bug: 365210

Updated

11 years ago
Duplicate of this bug: 369904

Updated

11 years ago
Duplicate of this bug: 371909
(Assignee)

Updated

11 years ago
Attachment #248800 - Flags: review?(neil)
(Assignee)

Comment 20

11 years ago
Created attachment 257882 [details] [diff] [review]
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
Attachment #248800 - Attachment is obsolete: true
(Assignee)

Updated

11 years ago
Attachment #257882 - Flags: review?(neil)

Comment 21

11 years ago
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 22

11 years ago
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.
(Assignee)

Comment 23

11 years ago
Created attachment 262816 [details] [diff] [review]
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.
Attachment #257882 - Attachment is obsolete: true
Attachment #262816 - Flags: review?(neil)
(Assignee)

Comment 24

11 years ago
(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

11 years ago
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

11 years ago
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+
(Assignee)

Comment 27

11 years ago
Created attachment 262929 [details] [diff] [review]
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=
Attachment #262816 - Attachment is obsolete: true
Attachment #262929 - Flags: superreview?(mscott)
Attachment #262929 - Flags: review+

Comment 28

11 years ago
Hi Ian, can you attach a screen shot?
(Assignee)

Comment 29

11 years ago
Created attachment 262935 [details]
Screenshot of patched pref panel

This is how the new pref panel looks for Junk Settings

Comment 30

11 years ago
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+
(Assignee)

Comment 31

11 years ago
Created attachment 263307 [details] [diff] [review]
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
Attachment #262929 - Attachment is obsolete: true
Attachment #263307 - Flags: superreview+
Attachment #263307 - Flags: review+
(Assignee)

Updated

11 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Keywords: fixed-seamonkey1.5a
Resolution: --- → FIXED

Updated

11 years ago
Duplicate of this bug: 390254

Updated

10 years ago
Duplicate of this bug: 402441

Updated

10 years ago
Duplicate of this bug: 406841

Comment 35

10 years ago
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).
Duplicate of this bug: 423066
Duplicate of this bug: 430456
Product: Core → MailNews Core
Depends on: 449747
You need to log in before you can comment on or make changes to this bug.