Closed Bug 635993 Opened 10 years ago Closed 9 years ago

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

Categories

(Thunderbird :: Message Compose Window, defect)

defect
Not set
normal

Tracking

(blocking-thunderbird5.0 -, thunderbird5.0 wanted, blocking-thunderbird3.1 -)

RESOLVED FIXED
Thunderbird 11.0
Tracking Status
blocking-thunderbird5.0 --- -
thunderbird5.0 --- wanted
blocking-thunderbird3.1 --- -

People

(Reporter: rsx11m.pub, Assigned: jyeo)

References

Details

(Keywords: regression, student-project, Whiteboard: [good first bug])

Attachments

(1 file, 5 obsolete files)

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.
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.
blocking-thunderbird3.1: --- → ?
blocking-thunderbird5.0: --- → ?
Taking to put on my plate for regression window hunting..
Assignee: nobody → gary
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.
Assignee: gary → nobody
Blocks: 527018
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!
(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. :)
Keywords: student-project
Whiteboard: [good first bug]
Interestingly, a link like http://blah.com/blah1.pdf still triggers the attachment reminder based on the ".pdf" part, per bug 647871.
blocking-thunderbird3.1: ? → -
blocking-thunderbird5.0: ? → -
Flags: wanted-thunderbird+
Attached patch 635993patch (obsolete) — Splinter Review
Attachment #560746 - Flags: review?(bwinton)
Assignee: nobody → jasonyeo88
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.
Attachment #560746 - Flags: review?(bwinton) → review-
Attached patch 635993#2.patch (obsolete) — Splinter Review
Attachment #565746 - Flags: review?(bwinton)
Attachment #560746 - Attachment is obsolete: true
Attached patch 635993#3.patch (obsolete) — Splinter Review
Attachment #565746 - Attachment is obsolete: true
Attachment #565746 - Flags: review?(bwinton)
Attachment #565750 - Flags: review?(bwinton)
Attached patch 635993#4.patch (obsolete) — Splinter Review
Attachment #565750 - Attachment is obsolete: true
Attachment #565750 - Flags: review?(bwinton)
Attachment #565751 - Flags: review?(bwinton)
Attached patch 635993#5.patch (obsolete) — Splinter Review
Trimmed my patch to less than 8 characters per line.
Attachment #565751 - Attachment is obsolete: true
Attachment #565751 - Flags: review?(bwinton)
Attachment #567597 - Flags: review?
Attachment #567597 - Flags: review? → review?(bwinton)
> Trimmed my patch to less than 8 characters per line.
i meant 80 characters...
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"],
Attachment #567597 - Flags: review?(bwinton) → review-
Attached patch 635993#6.patchSplinter Review
I've added Blake's test cases and made the patch pass them.
Attachment #567597 - Attachment is obsolete: true
Attachment #575888 - Flags: review?(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!
Attachment #575888 - Flags: review?(bwinton) → review+
Checked in: http://hg.mozilla.org/comm-central/rev/d2d1125046e9
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 11.0
You need to log in before you can comment on or make changes to this bug.