When receiving a new email, the notification doesn't display the email address of the sender when he has no name

RESOLVED FIXED in Thunderbird 15.0

Status

Thunderbird
Mail Window Front End
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Frédéric Buclin, Assigned: hiro)

Tracking

6 Branch
Thunderbird 15.0
x86
Linux
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

6 years ago
Some users didn't set their real name when sending an email, so only their email address is displayed in the From: field, i.e.

  foo@bar.com

instead of

  Mr Foo <foo@bar.com>

When such emails arrive in the inbox, the notification which popup doesn't mention who sent the email. All we see is "... from " at the end of the message. If no name is set, then it should fallback to "... from foo@bar.com" so that we can at a glance see who sent the email and decide if it should be read immediately or can wait a bit while doing some other work.
(Reporter)

Comment 1

6 years ago
Note to self: newMailNotification_messagetitle is located in chrome/en-US/locale/en-US/messenger/messenger.properties and is called by nsMessengerUnixIntegration::BuildNotificationBody() in mailnews/base/src/nsMessengerUnixIntegration.cpp line 347 only.
(Assignee)

Comment 2

6 years ago
Created attachment 603152 [details] [diff] [review]
Fix

msg_parse_Header_addresses returns 0-length string, so we should also check the string.
Assignee: nobody → hiikezoe
Status: NEW → ASSIGNED
Attachment #603152 - Flags: review?(mbanner)
Comment on attachment 603152 [details] [diff] [review]
Fix

I'm not quite sure I see why this is happening. test_nsIMsgHeaderParser2.js already has at least one test for this and we use the function in lots of places.

Can you try to extend the test_nsIMsgHeaderParser2.js test to show this case please?
(Assignee)

Comment 4

6 years ago
Created attachment 609667 [details] [diff] [review]
Fix with a new test

I do not prefer that a test checks several things (in this case this bug and others) because it's difficult to find cause if regression happens.
Attachment #603152 - Attachment is obsolete: true
Attachment #603152 - Flags: review?(mbanner)
Attachment #609667 - Flags: review?(mbanner)

Comment 5

6 years ago
If the thing to test is very related, keeping it in a separate file isn't nice imho. E.g. it makes it very difficult to find when creating related tests. (And at the very least, the file name of the test should be descriptive.)
(Assignee)

Comment 6

6 years ago
(In reply to Magnus Melin from comment #5)
> If the thing to test is very related, keeping it in a separate file isn't
> nice imho. E.g. it makes it very difficult to find when creating related
> tests. (And at the very least, the file name of the test should be
> descriptive.)

I do not think it is difficult to create related tests. But I agree your comment in parentheses. Bug number file name is too bad. That was my neglect. Can you suggest better name for this test please?

Comment 7

6 years ago
test_nsIMsgHeaderParser-parseHeadersWithArray.js? BTW, the test file doesn't have a licence block (MPL2 or such).
Just to follow up on the separate test file - I'm happy with that, in theory it could be said the parseHeadersWithArray check shouldn't actually be in test_nsIMsgHeaderParser2.js, as it isn't quite related to the tests in the rest of the file.
Comment on attachment 609667 [details] [diff] [review]
Fix with a new test

>diff --git a/mailnews/mime/test/unit/test_bug684865.js b/mailnews/mime/test/unit/test_bug684865.js

Please name this file test_parseHeadersWithArray.js and update the xpcshell.ini to match. r=me with that fixed.
Attachment #609667 - Flags: review?(mbanner) → review+
Hiro ?
(Assignee)

Comment 11

5 years ago
Created attachment 623582 [details] [diff] [review]
Change the test file name,

carrying over review+.
Attachment #609667 - Attachment is obsolete: true
Attachment #623582 - Flags: review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
(Assignee)

Comment 12

5 years ago
Comment on attachment 623582 [details] [diff] [review]
Change the test file name,

Oops! Wrong patch
Attachment #623582 - Attachment is obsolete: true
Attachment #623582 - Flags: review+
(Assignee)

Comment 13

5 years ago
Created attachment 623589 [details] [diff] [review]
This is correct!
Attachment #623589 - Flags: review+
https://hg.mozilla.org/comm-central/rev/83a7ea3d40c2
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite-
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 15.0

Updated

5 years ago
Depends on: 756971

Comment 15

5 years ago
So an alternative would be to change

      author.Assign(names[0] ? names[0] : emails[0]);

to 

author.Assign(names[0] && *(names[0]) ? names[0] : emails[0]);

in http://mxr.mozilla.org/comm-central/source/mailnews/base/src/nsMessengerUnixIntegration.cpp#337

Updated

5 years ago
Depends on: 757846

Updated

5 years ago
Depends on: 761654
You need to log in before you can comment on or make changes to this bug.