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

VERIFIED FIXED in mozilla0.8

Status

P3
normal
VERIFIED FIXED
19 years ago
10 years ago

People

(Reporter: lchiang, Assigned: chuang)

Tracking

(Blocks: 1 bug)

Trunk
mozilla0.8

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: relnote-user [nsbeta1+])

Attachments

(6 attachments)

(Reporter)

Description

19 years ago
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?
(Reporter)

Updated

19 years ago
QA Contact: lchiang → esther

Comment 1

19 years ago
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).
(Reporter)

Comment 2

19 years ago
Jennifer - can you add this to your ab spec?


Keywords: beta2
(Reporter)

Comment 3

19 years ago
bulk move to M16.  
Target Milestone: --- → M16

Updated

19 years ago
Keywords: nsbeta2

Comment 4

19 years ago
Putting on [nsbeta2+][5/16] radar.  
Whiteboard: [nsbeta2+][5/16]

Comment 5

19 years ago
Mass moving M16 to M17 - look for nsbeta2 before anything else.
Target Milestone: M16 → M17
(Assignee)

Comment 6

19 years ago
Reassign to David.  Address book can't tell if the email addres is from mail or 
newsgroups.
Assignee: chuang → bienvenu

Comment 7

19 years ago
accept
Status: NEW → ASSIGNED
Target Milestone: M17 → M20

Comment 8

19 years ago
Putting on [nsbeta2-] radar.  Missed Netscape 6 train.  Move to MFuture 
milestone.
Whiteboard: [nsbeta2+][5/16] → [nsbeta2-]

Comment 9

19 years ago
possible item for beta2 relnotes
Keywords: relnote2

Comment 10

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

Updated

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

Comment 12

18 years ago
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: nsbeta2 → mail5
Whiteboard: [nsbeta2-] relnote-user → relnote-user

Comment 13

18 years ago
Correction, should be "mail3", important features.
Keywords: mail5 → mail3

Comment 14

18 years ago
This is not going into the release notes, since it's covered in the online help.

Comment 15

18 years ago
Changing qa assign to myself.
QA Contact: esther → pmock

Comment 16

18 years ago
changing priorities
Priority: P3 → P2

Comment 18

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

Comment 19

18 years ago
reassigning to chuang. Candice, could you look at this in the mail code, then.
Assignee: bienvenu → chuang
Status: ASSIGNED → NEW
(Assignee)

Comment 20

18 years ago
Created attachment 22169 [details] [diff] [review]
Patch file
(Assignee)

Comment 21

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

Updated

18 years ago
Keywords: patch

Comment 22

18 years ago
+    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)) {

Comment 23

18 years ago
Created attachment 22190 [details]
Screen shot of new pref

Comment 24

18 years ago
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.

Comment 27

18 years ago
Let me know if the screen shot accurately captures the actual behavior as 
implemented. Thanks.

Comment 28

18 years ago
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?
(Reporter)

Comment 29

18 years ago
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.

(Assignee)

Comment 30

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

Comment 31

18 years ago
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?

Comment 32

18 years ago
aren't all messages supposed to be identical? ie unless i'm wrong, don't rely 
on a bug to distinguish context.

Comment 33

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

Comment 34

18 years ago
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. 

Comment 35

18 years ago
Created attachment 22271 [details]
Updated AB Pref screen shot

Comment 36

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

Comment 37

18 years ago
Created attachment 22384 [details] [diff] [review]
Patch with pref

Comment 38

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

Comment 39

18 years ago
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.

Comment 40

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

Comment 42

18 years ago
Created attachment 22666 [details] [diff] [review]
Final Patch file 1
(Assignee)

Comment 43

18 years ago
Created attachment 22667 [details] [diff] [review]
Final Patch file 2 (mailnews.js)
(Assignee)

Comment 44

18 years ago
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.

Comment 45

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

Comment 46

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

Comment 47

18 years ago
I'll file a bug for it later.
r=sspitzer

sorry for the delay (on vacation)
(Assignee)

Comment 49

18 years ago
Fix check in 1/17/01.
Status: ASSIGNED → RESOLVED
Last Resolved: 18 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
Blocks: 498215
(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.