Closed Bug 234186 Opened 21 years ago Closed 20 years ago

Messages with mail server identity should not be sent through newsserver

Categories

(MailNews Core :: Composition, defect)

Product:
MailNews Core
Component:
Composition
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
VERIFIED FIXED

People

(Reporter: whimboo, Assigned: ch.ey)

References

Details

Attachments

(1 file, 4 obsolete files)

patch v5
3.76 KB, patch
neil
: review+
mscott
: superreview+
Details | Diff | Splinter Review 
Often I want to compose new mail during I visit a newsgroup. Clicking "Write"
inside the toolbar opens a new compose window with the current newsgroup inside
the newsgroup header. Here I have to do different actions:

1. Selecting the mail account
2. Clearing the newsgroup header

Sometimes I see that people forget to do the second action and their private
mail will be send to the current inserted newsgroup.

In that case I would prefer following enhancement:

After changing the account Mozilla should look which type the selected account
is of. If it's a newsgroup account there should be done nothing. Otherwise we
have a mail account and the newsgroup header should be removed.

In this way nobody would send private mail to a subscribed newsgroup because he
forgot to clear this header.
I don't think that this is useful.
Yes, you can have multiple identities for mail and news accounts, but you can
also tie a single identity to several accounts. 
Sure, but identities are visible more then one time within the from combobox. So
how do you recognize which account the selected identity belongs to? We need a
better solution for displaying the identities (like a treeview) - but this would
be another bug.

Definitly I select an identity which belongs to a mail account and my described
behavior should be done.
No, what you set in the "From" dropdown box is an _account_, which comprises of
an identity _and_ a server, with the account type ("pop3", "imap", "nntp", or
"none") in the server section. It does not make any sense to have a
"Newsgroups:" header line when a mail account is selected.

Futhermore, while every news account has an associated smtp server in case the
user wants to reply by mail to a newsgroup posting, it is not at all clear which
nntp server should be used for posting to a newsgroup when the user has set up
multiple news accounts, but a mail account is selected as "From".

Possible solutions would be:
- clear any nntp-related header lines when the user selects a mail account
- silently ignore these lines when the mail is sent
- pop up a dialog when the message is sent, asking the user how to handle it
I (now ;-) ) agree with Uwe that when changing the server/identity combination
in the from box to a mail server based one, Mozilla should *not* post to any
newsserver.
Existing newsgroup headers should *not* be discarded, though, because the choice
of additional headers must stay in the user's hands.

A warning dialog (that can be turned off!) seems to be a reasonable.
Summary updated to better fit the problem.
Severity: enhancement → normal
Summary: By changing to any mail account during compose of a new message to newsgroups should delete newsgroup header → Messages with mail server identity should not be sent through newsserver
Shouldn't the component be "Networking: General" or "Networking: News" rather
than "Composition"? The problem arises when the message is sent, not while
composing it.
The work could be done by the frontend JS code if we decide do simply delete any
newsgroup header or ask the user, this would be "Composition".
But if the sending backend should silently discard any newsgroup header, this
would be C++ code and "Networking: General" I think.
I believe that automatic deletion of headers probably set intentionally by the
user would be just *bad*. The problem is IMHO that the mail acount is sending
data to the newsserver...
(A news account sending data via mail is valid, though!)
Component: Composition → Networking: MailNews General
I've done a patch that I think fixes this bug.
But I'm not sure about the dialog text and layout.

Currently the text is
"You did specify a newsgroup as recipient for this message although the sender
identity isn't one for a newsgroup. Do you nevertheless want to continue?"
and it shows OK and Cancel.

Please make some suggestions about it.
Status: NEW → ASSIGNED
Component: Networking: MailNews General → Composition
Hardware: PC → All
First of all, the warning should be disengageable.

As for the wording, you'd better find some native English speaker. ;-)
My proposal would be:
+------------------------------------------------------+
| You have specified a newsgroup as recipient for this |
| message although the sender is a mail identity.      |
| Do you want to continue nonetheless?                 |
| [ ] Don't show me this warning again.                |
|           +-----------+      +-----------+           |
|           |    Yes    |      |    No     |           |
+-----------+-----------+------+-----------+-----------+
(We have far too many dialogs saying OK/Cancel instead of Yes/No already.)
> First of all, the warning should be disengageable.

Grmpf, more effort. :)
With disengageable you mean "remember the button I pressed" or "don't bother me
and just send it anyway"?

And any idea which prefname to use? I've nothing better than
mail.compose.newsgroup.dontAskBeforeSend.

> We have far too many dialogs saying OK/Cancel instead of Yes/No already.

That's because the PromptService's confirm function uses this. If one want to
use own button text one has to use the far more complex confirmEx function.
Assignee: sspitzer → ch.ey
Status: ASSIGNED → NEW
>> First of all, the warning should be disengageable.
> 
> Grmpf, more effort. :) With disengageable you mean "remember the
> button I pressed" or "don't bother me and just send it anyway"?

I'm somewhat undecided here, but I tend to say the latter.
Otherwise, if the dialog is hidden by the pref and mail does not get sent
without any notice (and maybe you forgot you making that [x]) - that would be
truly evil...

> And any idea which prefname to use? I've nothing better than 
> mail.compose.newsgroup.dontAskBeforeSend.

Maybe "mail.compose.warnMail2Newsgroup"? Dunno.

>> We have far too many dialogs saying OK/Cancel instead of Yes/No
>> already.
> 
> That's because the PromptService's confirm function uses this. If one
> want to use own button text one has to use the far more complex
> confirmEx function.

Yeah, I know. *veg*
But since this warning is meant for the *user*, (s)he shouldn't need to
interprete it...
> I'm somewhat undecided here, but I tend to say the latter. [...]

As I wrote in dcsmm it should be impossible now that a message goes into a
newsgroups when composed from a mail account. Instead it tries to connect the
first news server on the mail server port. I traced the problem down but I don't
think it can be fixed.

To cut a long story short, I doesn't make sense to keep the newsgroup in the
recipients list.
We can and should inform the user about his mistake, but with a changed second
sentence like "Do you want to correct this? Cancel will remove the newsgroup
address automatically."
In this case the suppressed alert won't be bad. In the worst case the user will
be alerted that no recipients were specified (if the newsgroup address was the
only one, no To, Bcc or CC).

> Maybe "mail.compose.warnMail2Newsgroup"? Dunno.

Nice, I'll use it.
(In reply to comment #13)
> "Do you want to correct this? Cancel will remove the newsgroup
> address automatically."

Not so good. In all other cases I can think of, "Cancel" takes you back to the
composer, while "Ok" proceeds with the current action.

I'd suggest something like "You cannot post to newsgroups through a mail
account. Press Ok to send as mail only, or Cancel to return to the composer window."
Or this way round.

I tried to avoid "to send as mail only" because Mozilla don't know if there are
any mail addresses. Ok, Mozilla knows, but to be correct we'd need two different
messages.
Attached patch proposed patch (obsolete) — — Splinter Review 
Patch to alert the user if he tries to post a message to a newsgroup over a
mail account, with ability to suppress it in the future.

If pressed OK or no alert, the newsgroups will be deleted. So the message will
be only sent by mail or Mozilla throws an alert if no mail addresses were
specified.

I'm not sure whether the newsgroup rows in the composer window should also be
cleared or not. But if, the user would have to reenter the recipient if he
typed one like
  Newsgroup: john.doe@example.com
before.
Attached patch patch v2 (obsolete) — — Splinter Review 
Urgh, the first try also made newsgroup posts over a newsgroup account
impossible.
Attachment #141449 - Attachment is obsolete: true
Attachment #141454 - Flags: review?(neil.parkwaycc.co.uk)
But I can post news using a mail identity...
And you think it's right so? Which build do you use?
Er, moment, mail identity? There's nothing like a mail identity.
Identities are identities and can be attached to each account and so each
incoming (and in case of NNTP also outgoing) server.

This bug is about posting news over a mail account. Each line in the From:
dropdown menu consists of identity - server. Posting news via "identity x -
server mail1" should be impossible. If this same identity is also attached to
another account which includes a news server (e.g. From: menu line "identity x -
server news1"), posting a news is ok.
Neil, any issues stoping you from review?
OK, so I created this message
snews://secnews.netscape.com:563/c1r9kg$gbi12@ripley.netscape.com
using the following steps:
1. Subscribed to the group
2. Clicked Compose
3. Changed the account from the drop-down
4. Typed in the subject and text
5. Clicked Send.
This using Mozilla 1.5 (ignore the UA spoofing).
That's it, yes, but what do you wanna tell us? Do you agree it shouldn't work or
don't you?

I know I wrote it's not possible anymore, if that is what you mean. I should
have written "with 1.6 or newer" (I think my patch for bug 228597 changed the
former behaviour).
Sorry, I hadn't realized that the behaviour had changed and you now can only
send news using an identity associated with a news server.
Disabling this wasn't intended (at least not with the patch that disabled it), I
was surprised too. While the user can't wrongly send private mail to a newsgroup
anymore, he now get's behaviour like hangs and error messages.
So this patch is still necessary to inform the user what's going on.
Comment on attachment 141454 [details] [diff] [review]
patch v2

Sorry for the delay, I had this applied but had completly forgotton to test
it... anyway I composed a message using my default mail identity, and I only
entered a newsgroup, and I got the dialog saying I can't post with a mail
identity, then I OK'd that and got another dialog saying that the sending
failed and I should add a recipient or newsgroup :-/
Err Neil, did you read the alert? It should say "Press OK to send with newsgroup
addresses removed, or Cancel to return to the composer window."
So if you only have entered one newsgroup and press OK, this newsgroup will be
removed and sending fails because you've no address entered.

Yes, you can say we shouldn't ask to remove the NG if there are only newsgroups.
If it's to you to define two strings I can do so.
Hmm... so how do you propose to handle these cases?
a. no recipients at all, news identity
b. no recipients at all, mail identity
c. news recipients only, mail identity
Neil, this patch isn't a magic bullet for all recipient problems.
The case of no recipients at all or no recipients after passing the new code
(clicking OK) will be handled by the code that already does this
(mime_sanity_check_fields() in nsMsgCompUtils.cpp).
Comment on attachment 141454 [details] [diff] [review]
patch v2

>+          const kDontAskAgainPref  = "mail.compose.warnMail2Newsgroup";
Not my favourite choice of pref - you're warning against posting without a news
server, surely? Also you have two spaces before the = sign...

>+          try {
>+            var pref = Components.classes["@mozilla.org/preferences-service;1"]
>+                                .getService(Components.interfaces.nsIPrefBranch);
>+            var dontAskAgain = pref.getBoolPref(kDontAskAgainPref);
>+          } catch (e) {
>+            // dontAskAgain not set - then we need to ask user
>+            dontAskAgain = false;
I'd prefer if you put var dontAskAgain = false; before the try.

>+          if(!dontAskAgain)
I'd like a space between the if and ( please.

>+            var checkbox = {value:0};
Shouldn't this value be false? The idl says inout boolean checkValue.

>+            var okToProceed = gPromptService.confirmCheck(
>+                              window,
>+                              sComposeMsgsBundle.getString("subjectDlogTitle"),
>+                              sComposeMsgsBundle.getString("recipientDlogMessage"),
>+                              sComposeMsgsBundle.getString("CheckMsg"),
>+                              checkbox);
>+
>+            try {
>+              if (checkbox.value != dontAskAgain)
Well, dontAskAgain is always false at this point...

>+                pref.setBoolPref(kDontAskAgainPref, checkbox.value);
Well, checkbox.value is always true at this point...

>+            } catch (ex) {}
>+
>+            if(!okToProceed) 
>+              return;
Wait a sec... if you tick the checkbox and click Cancel then next time you
won't get asked, it will go ahead and ignore the newsgroups...

>+recipientDlogMessage=You cannot post to newsgroups through a mail account. Press OK to send with newsgroup addresses removed, or Cancel to return to the composer window.
If the message needs to explain what OK and Cancel do, then it's badly worded.
Here's my attempt, but I'll solicit some further opinions:
If you continue your newsgroups will be ignored because you have not selected a
news account to post to.
Attachment #141454 - Flags: review?(neil.parkwaycc.co.uk) → review-
> Not my favourite choice of pref

Uh, "mail.compose.dontWarnMail2Newsgroup" would be the right logic. But is the
wording better?

> I'd prefer if you put var dontAskAgain = false; before the try.

Err, why? And if, instead in catch or additionally?

> Shouldn't this value be false? The idl says inout boolean checkValue.

Could also be false, yes. It's 0 in other places with this construction, so I
took this.

> if you tick the checkbox and click Cancel then next time you
> won't get asked, it will go ahead and ignore the newsgroups...

Well, yes. I think I had a good reason for this, but I can't remember. :-(
So I now put the if and the return before the try-catch.

Regarding the wording, I'd want to give the explanation first. So what about
"You cannot post to newsgroups through a mail account. Continue with newsgroups
ignored?"
(In reply to comment #31)
>>Not my favourite choice of pref
>Uh, "mail.compose.dontWarnMail2Newsgroup" would be the right logic.
Are you trying to warn about sending mail to a news server?
I don't think it's clear that that's what you're trying to prevent.
And it bears no relation to the displayed message!
How about mail.compose.ignoreNewsgroupsForMail?
>"You cannot post to newsgroups through a mail account. Continue with 
>newsgroups ignored?"
How about "This account only supports email recipients. 
Continuing will ignore newsgroups."
> Are you trying to warn about sending mail to a news server?

No, to warn about sending a text the user composed as mail to a newsgroup.

> And it bears no relation to the displayed message!
> How about mail.compose.ignoreNewsgroupsForMail?

Ok, now I think I know what you mean.
I (and Karsten who proposed this name) refer to the behaviour (show or don't
show) of the dialogue. You refer to how the recipients are processed.

I still tend to mine because the dialogue is what the pref controls.
ignoreNewsgroupsForMail set to false doesn't mean it will continue without
ignoring. 

> "This account only supports email recipients. Continuing will ignore newsgroups."

Accepted
(In reply to comment #33)
>>Are you trying to warn about sending mail to a news server?
>No, to warn about sending a text the user composed as mail to a newsgroup.
Well, I think I see what you mean now... perhaps it's too late at night ;-)
Any new thoughts after your enlightenment late at night?-)
No, I meant I understand your patch now, you can attach the latest version.
Attached patch patch v3 (obsolete) — — Splinter Review 
Version with requested changes.
Attachment #141454 - Attachment is obsolete: true
Comment on attachment 145623 [details] [diff] [review]
patch v3

>+          var dontAskAgain = false;
>+          try {
>+            var pref = Components.classes["@mozilla.org/preferences-service;1"]
>+                                .getService(Components.interfaces.nsIPrefBranch);
>+            dontAskAgain = pref.getBoolPref(kDontAskAgainPref);
>+          } catch (e) {
>+            // dontAskAgain not set - then we need to ask user
>+            dontAskAgain = false;
>+          }
Ah yes, I forgot to answer your question... the main idea was not to declare
dontAskAgain inside the try block, because it looks odd, but now you've moved
the declaration outside the try block you don't need the catch line. You can
comment the declaration instead, e.g. // default to ask user if the pref is not
set
Attachment #145623 - Flags: review+
Yep, I like this more.

> you don't need the catch line

But an empty catch statement, right?
Yeah, catch {} or however it's written in the rest of the module.
Attached patch patch v4 (obsolete) — — Splinter Review 
Ups, I completeley forgot this one.
This version is for the changed catch statement.
Attachment #145623 - Attachment is obsolete: true
Attachment #146169 - Flags: review?(neil.parkwaycc.co.uk)
Christian, take a look at the patch again. Not all lines follow the prefered
coding style e.g. 'if(servertype != "nntp" && msgCompFields.newsgroups != "")'.
I think you won't get r+.
Attachment #146169 - Flags: review?(neil.parkwaycc.co.uk)
I already gave r+ with those spacing errors - I need fewer midnight reviews ;-)
And you gave it for a version with the bad bad catch ...
I'll attach a patch with the spaces added soon - any other nits right now?
Comment on attachment 146169 [details] [diff] [review]
patch v4

>+              if (checkbox.value != false)
Don't need != false here, as you're not worried about the value being null.
[Interestingly 0 == false, 0 == "" and false == "" all compare true!]
Attached patch patch v5 — — Splinter Review 
Ok, so next try with these changes.
Attachment #146169 - Attachment is obsolete: true
Comment on attachment 146508 [details] [diff] [review]
patch v5

I wish diffdiff worked properly after the revision changes but I'm guessing
that you haven't changed anything major here ;-)
Attachment #146508 - Flags: review+
Comment on attachment 146508 [details] [diff] [review]
patch v5

Well, I hope so too. :) Thanks.
Attachment #146508 - Flags: superreview?(mscott)
Attachment #146508 - Flags: superreview?(mscott) → superreview+
Blocks: 240476
Fix checked in on 04-19 by Neil.
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
question long after the fact :)

wouldn't it have been cleaner to add a default value for
mail.compose.dontWarnMail2Newsgroup in mailnews.js

Then remove all of the try/catches around getting/setting the pref?
What about a pref to turn this entire thing OFF !!??
Sorry, what do you mean with turn it off? Warning the user?
Turn off this "feature". I want the abilitiy like I have now in 0.6 release to
choose any address I have configured, mail or otherwise. I can't do that with
this "fix". As soon as the ability to configure multiple identities in an
account becomes available then this will be a moot point.
You can now in the 0.6 release post a message to a newsgroup when you've chosen
a From: connected to a mail account?
That should be impossible anyway. But instead explaining why it throws some less
nice errors.

The entries in the From: dropdown consist of two informations: identity (first
part) and account (resp. incoming server connected with it) name (last part, the
light grey one).
When sending a mail, the SMTP server is determined by the identity part. When
sending to a newsgroup the NNTP server is determined by the incoming server
(because NNTP is two-way).
When a From: entry with a POP account part is chosen, Mozilla can't find the
right NNTP server since the incoming server for this entry is a POP3.
I have always, up until the latest nightlies, been able to post to a newsgroup,
choose ANY address (mail or otherwise) in the From drop-down and post. Been able
to do this in all versions of Mozilla as well as the latest 1.7rc1. I can even
choose any/all addresses that are pure "fake", not associated with either a real
mail OR news account.

This is a flexibility that will be sadly missed if allowed to become permanent
with no user selectable choice. However, multiple ID's will solve all this.
Mscott has a prefs.js hack that more or less works by setting up multiple ID's
based on a good address. Such as:

user_pref("mail.account.account1.identities", "id1,id22");

Then you set up id22 as the "fake" address.

Band-aid at best but a permanent "feature" sure would be nice.


Well, it's possible you can send news with any identity if you've only one news
account. The reason is that the POP server is taken from the dropdown but then
checked against the available NNTP servers. If it doesn't match one, the first
NNTP server is taken.

This replacing it's ok for people with one NNTP server but very dangerous for
others.
Preventing the user from accidentally sending a private mail through a
newsserver is a good feature I think. And as you wrote, if one wants to use any
identity for news, one can simply assign it. But a fake address isn't necessary
it can be the one of the first normal mail identity or an additional a.s.o.
I have 7 news server accounts and I can post to any one of them with any address
in the From drop-down in 0.6 release as well as Mozilla 1.7rc1.
Jay, I don't know what happens there. I can't reproduce what you're writing and
I thought I know the code.
Maybe you've a magic installation.

How should Mozilla know, which server to use to post if you select a POP account
entry in the dropdown? I just don't understand it.
Maybe we're talking about different scenarios.

I select an NNTP server from my list and a group, load the group, select a 
message to reply to.

I compose a reply. In the From drop-down, I choose something OTHER than the 
default, which can be ANY address in the drop-down list.
No, unfortunately not, that's the same scenario I used to test this.
The Mozilla I used to test (nightly 1.7-branch) ends up in sending the message
always to the first newsserver.
Jay, I'm still trying to understand this. I'd appreciate if you could try some
things.
Choose one a news account and reply to one message. Don't change the From: and
do Send Later. Then reply again but change the From: to one of a mail account.
Choose Send Later again.

1. Please copy the X-Identity-Key: and X-Account-Key: lines of the both messages
here.
2. Please have a look at the prefs and write what type of server is connected to
each Account-Key.

The second question is, what happens if you now do Send Unsent Messages? Are
both messages get posted via the same news server?
You may have been correct all along although I could post with email associated
From addresses from the drop-down. Now, for whatever reason I can't, at least
not all but still some of them. But now I get a different error "An NNTP error
occured, no such news group "xxxxxxxx" .. So I am thinking that I have a unique
situation here with a severely messed up profile. I abandoned 0.6 and am back to
the nightlies once again. Sorry for the trouble, just posting what I see.
Ok, that's better now. Not for the usability, I know - but for my sanity. ;)

Maybe (I don't know how you determined what server was used to send) all
messages have been sent through the first newsserver (which BTW isn't
necessarily the first in the folder pane, the internal list order is another one).
If this server carries the newsgroup you try to post to, you don't see any
complaints. But this is very fragile and I guess in the most configurations
there *is* a reason for the different servers.

And don't get me wrong, I really don't want to restrict users more than
necessary. But in this case here I don't see another solution.
For how it currently works, please see bug 228597.
Well, nice, now how do I disable this "feature"???

I have a mail account and NNTP accont set up to _share_ an identity. When I go
to a newsgroup, press "reply" and try to send my response, I now get this
warning and I have to go to the "From" field, replace the mail account with NNTP
account and only than I can send my message!
In reply to comment #64 .. Exactly the reason I commented on this bug and
pleaded for some way to disable this "feature". I can understand the reasoning
behind this feature but us more advanced users are being severely penalized. I
want the abilitiy to use ANY identity I want to send/post whenever/wherever I so
choose.
Aleksey, Jay, when replying to a news post, the From preselected in the composer
should be the one of the news identity/account combination.
Unfortunately it isn't, instead the first account with this shared identity is
selected. This is bug 228593.
The point that I'm trying to make is this. When I post to a news server/group,
the identity that's associated with that server is the one that is used to post
the message by default. However, there are some replies to some posts in the
same group that I would like to use another identity with another email address.
Some posts in the same group I'd like to use a false ID. With this "feature" I
cannot do so and THAT is what I would like to be able to DISable at my choosing.
jay, would you please stop complaining about bug 228597 in this bug? Thanks.
Product: MailNews → Core
Status: RESOLVED → VERIFIED
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: