Closed Bug 33543 Opened 24 years ago Closed 24 years ago

Addresses from reading a newsgroup posting shouldn't be added to collected addresses

Categories

(SeaMonkey :: MailNews: Address Book & Contacts, defect, P3)

defect

Tracking

(Not tracked)

VERIFIED FIXED
mozilla0.8

People

(Reporter: lchiang, Assigned: chuang)

References

(Blocks 1 open bug)

Details

(Whiteboard: relnote-user [nsbeta1+])

Attachments

(6 files)

Addresses from reading a newsgroup posting shouldn't be added to collected 
addresses

Win32 2000-03-27-09-m15 build

1)  Open a newsgroup
2)  Select a posting by someone who you don't have in your collected address 
book
3)  Open your collected address book

Actual results:  The sender of the newsgroup posting is added to your collected 
address book

Expected results:  I don't think that the sender of a newsgroup posting should 
be added to the collected address book.  Otherwise, my collected address book 
will fill up with names of people that I do not even know simply because I am 
browsing or reading through a newsgroup.  In most cases, this is different than 
reading emails that may be addressed to me or cc'd to me.

cc: jglick - what do you think, Jennifer?
QA Contact: lchiang → esther
Very good point. Most users probably won't want authors of newgroup postings 
added to their Collected Addresses AB.  

I say we don't add authors/recipients from newsgroups.  

If time permits we could make it a Preference (with default turned off).
Jennifer - can you add this to your ab spec?


Keywords: beta2
bulk move to M16.  
Target Milestone: --- → M16
Keywords: nsbeta2
Putting on [nsbeta2+][5/16] radar.  
Whiteboard: [nsbeta2+][5/16]
Mass moving M16 to M17 - look for nsbeta2 before anything else.
Target Milestone: M16 → M17
Reassign to David.  Address book can't tell if the email addres is from mail or 
newsgroups.
Assignee: chuang → bienvenu
accept
Status: NEW → ASSIGNED
Target Milestone: M17 → M20
Putting on [nsbeta2-] radar.  Missed Netscape 6 train.  Move to MFuture 
milestone.
Whiteboard: [nsbeta2+][5/16] → [nsbeta2-]
possible item for beta2 relnotes
Keywords: relnote2
Based on leger's comments 5-30-00 I'm changing target milestone to Future.
David, if you disagree, please change accordingly.
Target Milestone: M20 → Future
Keywords: relnote3, relnoteRTM
Doesn't this make the Collected Addresses function pretty useless for anyone who 
reads news, then? :-(

Gerv
Keywords: relnote2, relnote3
Whiteboard: [nsbeta2-] → [nsbeta2-] relnote-user
Removing nsbeta2 keyword and adding mail5 keyword so this get nominated for 6.5 
release. Having a preference that allows the user to turn off the CAB (Collected 
Address Book) for newsgroup postings would be a big plus. It would also prevent 
a lot of users from reaching the upper CAB limit.
Keywords: nsbeta2mail5
Whiteboard: [nsbeta2-] relnote-user → relnote-user
Correction, should be "mail3", important features.
Keywords: mail5mail3
This is not going into the release notes, since it's covered in the online help.
Changing qa assign to myself.
QA Contact: esther → pmock
changing priorities
Priority: P3 → P2
marking nsbeta1+ and moving to mozilla0.8

This includes adding a pref for this.
Keywords: nsbeta1
Priority: P2 → P3
Whiteboard: relnote-user → relnote-user [nsbeta1+]
Target Milestone: Future → mozilla0.8
reassigning to chuang. Candice, could you look at this in the mail code, then.
Assignee: bienvenu → chuang
Status: ASSIGNED → NEW
Attached patch Patch fileSplinter Review
The patch is to check if "Newsgroups" is in the msg header, then won't add teh 
sender email into collected address book.

Jennifer,  I need the UI instruction on adding the pref.
Status: NEW → ASSIGNED
Keywords: patch
+    if ( (!headerInfo) || (!headerInfo->name) || (!(*headerInfo->name)) ||
+      (!headerInfo->value) || (!(*headerInfo->value)))
you don't need most of those ()s
simplification1:
if ( !headerInfo || !headerInfo->name || !*headerInfo->name || 
!headerInfo->value || !*headerInfo->value)
logical collection:
if (!(headerInfo && headerInfo->name && *headerInfo->name && headerInfo->value 
&& *headerInfo->value))
use this^^

+    if (nsCRT::strcasecmp("Newsgroups", headerInfo->name) == 0)
+       {
don't explicitly compare with zero.
instead do this:
if (!nsCRT::strcasecmp("Newsgroups", headerInfo->name)) {
Attached image Screen shot of new pref
Note some wording changes.  In the describe text at the beginning, "mail" is 
replaced with "messages" since news is now included.  "my" is removed.
It looks like if someone posts a message to a newsgroups and cc's me,  and I
read the message in my Inbox, I won't collect their address.  (this is different
than reading the newsgroup posting.)

is that the desired behaviour?  (I'm ok with it, I just want to bring it up.)

timeless provided some good suggestions.  can you try them out, and attach a new
patch, so I can review the final form of the patch?
adding myself to the cc list.
Let me know if the screen shot accurately captures the actual behavior as 
implemented. Thanks.
aren't we supposed to check a pref to see if we should collect addresses from
the newsgroup? I don't see us checking a pref in the patch - did I miss something?
re: "It looks like if someone posts a message to a newsgroups and cc's me,  and
I read the message in my Inbox, I won't collect their address.  (this is
different than reading the newsgroup posting."

I think in this case we should be consistent in that any message which passes
through the user's Inbox should be subject to the same "address collection"
preferences regardless of the origin of the message.

1. I haven't done the pref part, I like to check in this patch first.  I'll pst 
a final one when I get the No.2 taken care.

2. Regarding on Seth's comment,  I'll try it out.  But since I can only detect 
it from the msg header, there has to be enough information for me to tell it's a 
mail msg but origin from a news server.
I tried to post a message to a newsgroup and cc to myself.  It looks like in 
message header, the newsgroup msg header will have "NNTP-Posting-Host" and the 
cc msg header doesn't.   Instead of checking "Newsgroups",  I'll check for 
"NNTP-Posting-Host".   Since I'm not that familiar with msg header, anyone has 
objections?
aren't all messages supposed to be identical? ie unless i'm wrong, don't rely 
on a bug to distinguish context.
If someone knows of a way to better detect the origin of where a message comes
from then I think we should do that.

But if we can't, then I think it's very acceptable to say that we don't collect
addresses for messages addressed to newsgroups even if they show up in a mail
folder. People who spend enough of their time reading messages like this that it
will be an issue probably aren't going to want to turn off collection for
newsgroups anyway.
Thanks to Seth's email for the link 
http://www.vex.net/yarn/list/199901/0094.html. "NNTP-Posting-Host" is not a 
required header in newsgroup msg header.  So I'll use "Newsgroups".  This means 
the email address from a mail message which is originated from a newsgroup will 
not be collected if the collected newsgroup pref is not checked. 
Before "Incoming messages" and "Outgoing messages" meant both mail and news. 
Since we are now separating out mail from newsgroups, we should probably 
change "Incoming messages" to "Incoming mail messages" and "Outgoing mail 
messages" so it is more clear. 
Attached patch Patch with prefSplinter Review
can we cache this pref value so we only read them once when msgHdrViewOverlay.js
first gets intialized?

If we were worried about false hits when reading a message you were cc'ed on
that happened to be posting to a newsgroup we can always try to be fancy and
figure out the current account in the folder pane. If it's a news account then
we know to disable the collected address book. If it's non news, then go ahead
and add entries....that may be more trouble than it's worth, I just wanted to
throw it out there.
I'm not sure about cache the pref, what if the pref get changed after 
msgHdrViewOverlay.js initialized?

The place where we add the email address in the collected address is in mime 
code.  I'm not familiar with that part of code, if someone can point out the way 
to get the current account in the folder pane in mime, yes, that's the right way 
to do it.
the bracing in @@ -167,6 +167,20 @@ is very messy, please use something normal

+  for (PRInt32 j=0; j<mHeaderArray->Count(); j++) {
+    headerInfoType *headerInfo = (headerInfoType *)mHeaderArray->ElementAt(j);
+    if (!(headerInfo && headerInfo->name && *headerInfo->name))
+      continue;
+
+    if (!nsCRT::strcasecmp("Newsgroups", headerInfo->name)) {
+      bFromNewsgroups = PR_TRUE;
+      break;
+    }
+  }
Keywords: review
chuang wrote:  "what if the pref get changed after msgHdrViewOverlay.js
initialized?"

if that is an issue (I don't know enough about msgHdrViewOverlay.js), then you
can use pref callbacks from JS to get notified when a pref changes.  check out
nsIPrefListener.

so, if it isn't an issue, cache the pref value like mscott suggests.  if it is
an issue do the pref callback thing.

also, instead of 

handleHeader: function(headerName, headerValue, fromNewsgroups) 

is should be generalized to

handleHeader: function(headerName, headerValue, dontCollectAddress)

fix the implementation of handleHeader().

the caller (from mime/emitters/src/nsMimeHtmlEmitter.cpp) is fine.

please address those issues and attach a new patch.
Hopefully he 2 patch are the final.  The two prefs (for Incoming and newsgroup) 
have cached in msgHdrViewOverlay.js as mscott suggested.  The name in 
msgHdrViewOverlay.js has chnaged as Seth suggested.

Since we cached the prefs as globals, if user changed these two prefs, it won't 
be working until user close and reopen the Mail window or restart the app.  A 
bug 65613 has filed to state the problem.
silly thought, 
@@ -32,6 +33,7 @@
                if( emailCollection.checked ){
could we have an <emailCollectionBroadcaster> and have everything that is 
getting disabled toggled observe it?

this thought is not required for current patch, but if a new patch is needed, 
please consider.

+ ((gCollectIncoming && !dontCollectAddress) || (gCollectNewsgroup && 
dontCollectAddress)))
looks messy
+ dontCollectAddress ? gCollectIncoming : gCollectNewsgroup)
should work, and is hopefuly clearer

+  for (PRInt32 j=0; j<mHeaderArray->Count(); j++)
+  {
it's a counter, so PRUInt32 would make more sense
+  for (PRUInt32 j=0; j<mHeaderArray->Count(); j++) {

+    if (!nsCRT::strcasecmp("Newsgroups", headerInfo->name))
+       {
+      bFromNewsgroups = PR_TRUE;
+         break;
+       }
I hope that isn't a known bracing style
+    if (!nsCRT::strcasecmp("Newsgroups", headerInfo->name)) {
+      bFromNewsgroups = PR_TRUE;
+      break;
+    }

please consider following these sugggestions.

22667: r=timeless
the bracing is probably screwy because of tabs.
Is there a bug filed for the fact that we are just looking at the newsgroups:
header to determine if the posting was read in a newsgroup?
sr=bienvenu
I'll file a bug for it later.
r=sspitzer

sorry for the delay (on vacation)
Fix check in 1/17/01.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
VERIFIED FIXED on Windows 2000 build 2001020504, Linux build 2001020506 and Mac
build 2001020513.  Browsing a newsgroup's posting doesn't add any additional
entries into the Collected Address Book.
Status: RESOLVED → VERIFIED
Product: Browser → Seamonkey
(In reply to comment #45)
> + ((gCollectIncoming && !dontCollectAddress) || (gCollectNewsgroup && 
> dontCollectAddress)))
> looks messy
> + dontCollectAddress ? gCollectIncoming : gCollectNewsgroup)
> should work, and is hopefuly clearer

I filed bug 498215.
QA Contact: pmock → addressbook
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: