Closed Bug 234186 Opened 21 years ago Closed 21 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: 21 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: