Last Comment Bug 635993 - Attachment reminder doesn't recognize file names with matching file-type extensions
: Attachment reminder doesn't recognize file names with matching file-type exte...
Status: RESOLVED FIXED
[good first bug]
: regression, student-project
Product: Thunderbird
Classification: Client Software
Component: Message Compose Window (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Thunderbird 11.0
Assigned To: Jason Yeo[:jyeo]
:
:
Mentors:
Depends on:
Blocks: 527018
  Show dependency treegraph
 
Reported: 2011-02-22 14:07 PST by rsx11m
Modified: 2011-12-19 11:08 PST (History)
7 users (show)
standard8: wanted‑thunderbird+
standard8: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
-
wanted
-


Attachments
635993patch (1.78 KB, patch)
2011-09-17 18:36 PDT, Jason Yeo[:jyeo]
bwinton: review-
Details | Diff | Splinter Review
635993#2.patch (4.53 KB, patch)
2011-10-08 10:44 PDT, Jason Yeo[:jyeo]
no flags Details | Diff | Splinter Review
635993#3.patch (4.59 KB, patch)
2011-10-08 11:36 PDT, Jason Yeo[:jyeo]
no flags Details | Diff | Splinter Review
635993#4.patch (4.58 KB, patch)
2011-10-08 12:18 PDT, Jason Yeo[:jyeo]
no flags Details | Diff | Splinter Review
635993#5.patch (4.60 KB, patch)
2011-10-17 15:25 PDT, Jason Yeo[:jyeo]
bwinton: review-
Details | Diff | Splinter Review
635993#6.patch (5.48 KB, patch)
2011-11-21 09:28 PST, Jason Yeo[:jyeo]
bwinton: review+
Details | Diff | Splinter Review

Description rsx11m 2011-02-22 14:07:17 PST
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 2011-02-22 21:11:48 PST
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] 2011-02-23 01:43:16 PST
Taking to put on my plate for regression window hunting..
Comment 3 Gary Kwong [:gkw] [:nth10sd] 2011-02-25 08:43:23 PST
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) (:☕️) 2011-02-25 09:06:16 PST
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] 2011-02-25 09:08:06 PST
(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 2011-04-06 09:40:44 PDT
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] 2011-09-17 18:36:43 PDT
Created attachment 560746 [details] [diff] [review]
635993patch
Comment 8 Blake Winton (:bwinton) (:☕️) 2011-09-20 13:02:15 PDT
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] 2011-10-08 10:44:54 PDT
Created attachment 565746 [details] [diff] [review]
635993#2.patch
Comment 10 Jason Yeo[:jyeo] 2011-10-08 11:36:05 PDT
Created attachment 565750 [details] [diff] [review]
635993#3.patch
Comment 11 Jason Yeo[:jyeo] 2011-10-08 12:18:26 PDT
Created attachment 565751 [details] [diff] [review]
635993#4.patch
Comment 12 Jason Yeo[:jyeo] 2011-10-17 15:25:33 PDT
Created attachment 567597 [details] [diff] [review]
635993#5.patch

Trimmed my patch to less than 8 characters per line.
Comment 13 Jason Yeo[:jyeo] 2011-10-17 15:27:26 PDT
> Trimmed my patch to less than 8 characters per line.
i meant 80 characters...
Comment 14 Blake Winton (:bwinton) (:☕️) 2011-10-26 08:53:59 PDT
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] 2011-11-21 09:28:35 PST
Created attachment 575888 [details] [diff] [review]
635993#6.patch

I've added Blake's test cases and made the patch pass them.
Comment 16 Blake Winton (:bwinton) (:☕️) 2011-11-30 12:10:47 PST
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, afk until Dec) 2011-12-13 05:51:06 PST
Checked in: http://hg.mozilla.org/comm-central/rev/d2d1125046e9

Note You need to log in before you can comment on or make changes to this bug.