635993 - Attachment reminder doesn't recognize file names with matching file-type extensions

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[rsx11m]

This seems to have worked as introduced in bug 492555 comment #21, but while testing for bug 635938, I've noticed that something like "blah.doc" does no longer trigger the attachment reminder whereas "blah .doc" with as space does. Multi-token words like "semi-attached" still trigger based on "attached", but somewhere the "." handling must have got lost which is needed to correctly
match any file extensions in file names.

Tested in current Windows and Linux trunk builds.

Comment 1

[rsx11m]

Interesting, it doesn't seem to work in Thunderbird 3.1.8 already, so this
must have sneaked in earlier. I don't have 3.0.x any more around.

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Taking to put on my plate for regression window hunting..

Comment 3

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Works:

Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a1pre) Gecko/20091110 Shredder/3.1a1pre

Fails:

Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a1pre) Gecko/20091111 Shredder/3.1a1pre

http://hg.mozilla.org/comm-central/pushloghtml?startdate=2009-11-10&enddate=2009-11-11

Highly likely to be bug 527018.

Comment 4

[Blake Winton (:bwinton) (:☕️)]

I strongly agree.  And since we have some pretty decent test coverage in this area, this should be a simple bug for someone new (or a student) to fix, but I forget the magic whiteboard incantation for that…

Thanks for finding the problem commit!

Comment 5

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to comment #4)
> I strongly agree.  And since we have some pretty decent test coverage in this
> area, this should be a simple bug for someone new (or a student) to fix, but I
> forget the magic whiteboard incantation for that…

"student-project" keyword will be the magic..

> Thanks for finding the problem commit!

No problem, took me a quite a while, but eventually got it nonetheless. :)

Comment 6

[rsx11m]

Interestingly, a link like http://blah.com/blah1.pdf still triggers the attachment reminder based on the ".pdf" part, per bug 647871.

Comment 7

[Jason Yeo[:jyeo]]

Attachment: 635993patch

Comment 8

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 560746 [details] [diff] [review]
635993patch

Review of attachment 560746 [details] [diff] [review]:
-----------------------------------------------------------------

Given that we've already regressed this once, I think we should add some tests to make sure we're still doing the right thing.

I also don't see the attachment reminder pop up for _any_ text I type in, so, uh, this is extra-hard to make sure we've gotten right.

Thanks,
Blake.

Comment 9

[Jason Yeo[:jyeo]]

Attachment: 635993#2.patch

Comment 10

[Jason Yeo[:jyeo]]

Attachment: 635993#3.patch

Comment 11

[Jason Yeo[:jyeo]]

Attachment: 635993#4.patch

Comment 12

[Jason Yeo[:jyeo]]

Attachment: 635993#5.patch

Trimmed my patch to less than 8 characters per line.

Comment 13

[Jason Yeo[:jyeo]]

> Trimmed my patch to less than 8 characters per line.
i meant 80 characters...

Comment 14

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 567597 [details] [diff] [review]
635993#5.patch

Review of attachment 567597 [details] [diff] [review]:
-----------------------------------------------------------------

This is certainly an improvement, but I think there are a few more cases that we want to handle, so I'm going to say r-, because I want another chance to think up evil tests after you've fixed these ones.  ;)

Thanks,
Blake.

::: mail/base/modules/attachmentChecker.js
@@ -91,9 +91,9 @@
> > -    var start = IsCJK(kw.charCodeAt(0)) ? "" : ("(^|" + NOT_W + ")");
> > +    var re;
> > -    var end = IsCJK(kw.charCodeAt(kw.length - 1)) ? "" : ("(" + NOT_W + "|$)");
> > +    var matching;
NaN more ...
> > +      re = new RegExp("(([^\\s]*)\\b)" + kw + "\\b", "ig");
> > -    if (matching && !(/^http|^ftp/i.test(matching[0])))
> > +      matching = mailData.match(re);
> > -      // We're not worried about matching too much because we only add the
NaN more ...

Since this for loop has a few lines in its body, I'ld like to see {}s around it.

::: mail/base/test/unit/test_attachmentChecker.js
@@ -100,0 +86,6 @@
> > +  ["Simple keyword", "latte.ca", "latte", "latte"],
> > +  ["Extension", "testing document.pdf", ".pdf", "document.pdf"],
> > +  ["Two Extensions", "testing document.pdf and test.pdf", ".pdf",
> > +    "document.pdf,test.pdf"],
NaN more ...

// Some tests that fail that I think should pass...
["Should match", "I've attached the http/test.pdf file", ".pdf", "http/test.pdf"],
["Should still fail", "a https://test.pdf a", ".pdf", ""],
["Should match Japanese", "a test.添付 a", ".添付", "test.添付"],
["Should match Greek", "a test.Θεωρία a", ".Θεωρία", "test.Θεωρία"],
["Should match once", "a test.pdf.doc a", ".pdf,.doc", "test.pdf.doc"],

Comment 15

[Jason Yeo[:jyeo]]

Attachment: 635993#6.patch

I've added Blake's test cases and made the patch pass them.

Comment 16

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 575888 [details] [diff] [review]
635993#6.patch

Review of attachment 575888 [details] [diff] [review]:
-----------------------------------------------------------------

Nice!  The code is now complicated enough that I'm not entirely sure it's correct, but the test coverage is awesome, and so I believe that we're doing the right thing.

r=me!

Comment 17

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/d2d1125046e9