Print failing recipient for SMTP errors after RCPT TO command

RESOLVED FIXED in Thunderbird 3

Status

RESOLVED FIXED
12 years ago
9 years ago

People

(Reporter: Martin.vGagern, Assigned: Martin.vGagern)

Tracking

Trunk
Thunderbird 3
x86
All

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

12 years ago
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.2) Gecko/20070312 Firefox/2.0.0.2
Build Identifier: 1.5.0.10 (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.
(Assignee)

Comment 1

12 years ago
Created attachment 258959 [details] [diff] [review]
Patch to nsSmtpProtocol.cpp and en-US composeMsgs.properties

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.

Comment 2

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

Updated

12 years ago
Attachment #258959 - Flags: review?(bienvenu)
(Assignee)

Comment 3

12 years ago
(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 1.5.0.10 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 4

12 years ago
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?
Attachment #258959 - Flags: superreview?(bienvenu)
Attachment #258959 - Flags: review?(ch.ey)
Attachment #258959 - Flags: review?(bienvenu)

Comment 5

12 years ago
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 6

12 years ago
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+

Updated

12 years ago
Assignee: nobody → Martin.vGagern

Comment 7

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

Comment 8

12 years ago
Created attachment 259098 [details] [diff] [review]
Improved according to comment #6

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 #258959 - Attachment is obsolete: true
Attachment #259098 - Flags: superreview?
Attachment #259098 - Flags: review?(ch.ey)
(Assignee)

Updated

12 years ago
Attachment #259098 - Flags: superreview? → superreview?(bienvenu)

Comment 9

12 years ago
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.
Attachment #259098 - Flags: superreview?(bienvenu)
Attachment #259098 - Flags: superreview+
Attachment #259098 - Flags: review?(ch.ey)
Attachment #259098 - Flags: review+

Updated

12 years ago
Duplicate of this bug: 276029
(Assignee)

Comment 11

12 years ago
(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.

Comment 12

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

Comment 13

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

Comment 14

12 years ago
Created attachment 269129 [details] [diff] [review]
add new string id so localizers know to change string

this patch also deals with the new suite locale strings...
Attachment #269129 - Flags: superreview?(mscott)

Updated

12 years ago
Attachment #269129 - Flags: superreview?(mscott) → superreview+

Comment 15

12 years ago
fix checked in, thx, Martin
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
(Assignee)

Comment 16

11 years ago
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?
Product: Core → MailNews Core

Updated

10 years ago
Duplicate of this bug: 478633

Updated

9 years ago
Target Milestone: --- → Thunderbird 3

Updated

9 years ago
Duplicate of this bug: 336851
You need to log in before you can comment on or make changes to this bug.