Closed Bug 374388 Opened 13 years ago Closed 13 years ago
Print failing recipient for SMTP errors after RCPT TO command
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:18.104.22.168) Gecko/20070312 Firefox/22.214.171.124 Build Identifier: 126.96.36.199 (20070312) (Gentoo ebuild) I just spent a lot of time trying to make sense of an SMTP error. I tried to send an email to over a dozen recipients and the server simply said: "550 5.1.2 Cannot resolve your domain" in response to one of them. Due to bug 294296 which is now fixed but still present in my build, Thunderbird showd the following error message (or rather its translation): "An error occurred while sending mail. The mail server responded: 550 5.1.2 Cannot resolve your domain. Please verify that your email address is correct in your Mail preferences and try again." Even with the correct error message, NS_ERROR_SENDING_RCPT_COMMAND, the user would get no information as to what mail address actually failed. In the end I used openssl s_client to talk directly to the server to match response messages to RCPT TO commands, but it took me a lot of time and I would not expect this of any user. Reproducible: Always Steps to Reproduce: 1. Specify a mail recipient that the SMTP server will reject 2. Specify some valid recipients before and after the invalid one 3. Send the message Actual Results: Thunderbird's error message does not mention the recipient that caused the error. Unless the mail server error message includes it, you will not be told. Expected Results: Have the Thunderbird error message include the recipient that caused the error. I believe I have a patch for this already. Thunderbird is still compiling, if it succeeds I'll post it here.
This should address the issue in the latest CVS version. Stripping of processed addresses is moved from when they are sent to when the matching response was received, so that the address for the current response can be given as an argument to the error message format. I adjusted the english error message, but if someone can think of a better wording, feel free. Other languages should be updated as well, but as the additional argument was appended, compatibility should not be an issue.
Since there's a patch already, I'll confirm this. In order for the patch to proceed, you need to request a review and superreview on the patch; click the Details link next to the patch's listing in the attachment table, above, set the Review state to '?' and select a reviewer. When you retrieved the source code for the build, did you fetch the trunk? It's too late for an enhancement like this in the 1.8 or 1.8.0 branches.
Assignee: mscott → nobody
Status: UNCONFIRMED → NEW
Component: Message Compose Window → Networking: SMTP
Ever confirmed: true
Product: Thunderbird → Core
QA Contact: message-compose
Version: unspecified → Trunk
(In reply to comment #2) > In order for the patch to proceed, you need to request a review and > superreview on the patch; click the Details link next to the patch's listing > in the attachment table, above, set the Review state to '?' and select a > reviewer. How should I select a reviewer? I just set it to David Bienvenu, as the CVS logs look like he reviewed the last few modifications to this file. Is that the correct way? > When you retrieved the source code for the build, did you fetch the trunk? I retrieved code from trunk, but applied the resulting patches to the TB 188.8.131.52 source tarball to test it in a live application. The patch I applied had a modification to take bug 294296 into account as well. > It's too late for an enhancement like this in the 1.8 or 1.8.0 branches. Sad, but I can understand there has to be a deadline somewhere.
Comment on attachment 258959 [details] [diff] [review] Patch to nsSmtpProtocol.cpp and en-US composeMsgs.properties Christian, weren't you looking at a similar issue?
That's right, my approach is part of patch to bug 361433 while the issue is filed as bug 276029 and is also part of bug 197134.
Severity: enhancement → normal
Comment on attachment 258959 [details] [diff] [review] Patch to nsSmtpProtocol.cpp and en-US composeMsgs.properties Ok from me under the premise you also patch composeMsgs.properties under mailnews for SeaMonkey. Also m_addressesLeft--; if(m_addressesLeft > 0) could be if(--m_addressesLeft > 0)
Attachment #258959 - Flags: review?(ch.ey) → review+
Comment on attachment 258959 [details] [diff] [review] Patch to nsSmtpProtocol.cpp and en-US composeMsgs.properties sr=bienvenu, if you address Christian's comments. Thx for the patch. Could you attach a new patch for checkin? Thx!
Attachment #258959 - Flags: superreview?(bienvenu) → superreview+
OK, I addressed both issues from comment #6. Does this patch need to be reviewed and super-reviewed again formaly? Is it correct to request reviews again if a previous patch was already reviewed? Anyway I'll be glad to see this included. My very first Mozilla patch... ;-)
Attachment #259098 - Flags: superreview? → superreview?(bienvenu)
Comment on attachment 259098 [details] [diff] [review] Improved according to comment #6 No, I think Christian already gave his r= so you didn't really need to re-request review. I'll land this. One issue is that if we change a string like this, it's a lot easier for the localizers to know they need to change their localization if we actually add a new string instead of changing the old one. (This is because of some automated tools they use, I believe) I think we could do that by just changing the error code to a new value. I might try to do that before landing. Thx again for the patch.
(In reply to comment #9) > I'll land this. Thank's! > One issue is that if we change a string like this, it's a lot > easier for the localizers to know they need to change their localization if we > actually add a new string instead of changing the old one. (This is because of > some automated tools they use, I believe) I think we could do that by just > changing the error code to a new value. I might try to do that before landing. I've been thinking about this myself. I'm not sure what the alternative is. If there is a possibility that a release might be without a matching translation, I'd rather keep the existing translation instead of having that string untranslated. If it is guaranteed that all strings are translated before a localized build is released, then adding a new error code sounds a good idea.
David, did you land that patch anywhere? I wasn't able to find indication for it on trunk or 1.8 and 1.8.0 branches.
No, it would be trunk-only, and it looks like I haven't landed it there since the bug isn't marked fixed - I'll check, and land it if I haven't already.
this patch also deals with the new suite locale strings...
Attachment #269129 - Flags: superreview?(mscott)
Attachment #269129 - Flags: superreview?(mscott) → superreview+
fix checked in, thx, Martin
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
This has been in trunk for half a year now. How would this get moved to the 1.8 branch as well? Or will there be a TB release based on 1.9 soon enough to not warrant such a merge?
You need to log in before you can comment on or make changes to this bug.