FIX: replace 'discourse_email_parser' with 'email_reply_trimmer' to better trim replies from plain text emails

FIX: undefined method `number_to_human_size' when email contains attachments

Gemfile

@@ -64,7 +64,7 @@ gem 'aws-sdk', require: false
 gem 'excon', require: false
 gem 'unf', require: false
 
-gem 'discourse_email_parser'
+gem 'email_reply_trimmer', '0.0.3'
 
 # note: for image_optim to correctly work you need to follow
 # https://github.com/toy/image_optim

Gemfile.lock

@@ -73,10 +73,10 @@ GEM
     diff-lcs (1.2.5)
     discourse-qunit-rails (0.0.8)
       railties
-    discourse_email_parser (0.6.1)
     docile (1.1.5)
     domain_name (0.5.25)
       unf (>= 0.0.5, < 1.0.0)
+    email_reply_trimmer (0.0.3)
     ember-data-source (1.0.0.beta.16.1)
       ember-source (~> 1.8)
     ember-handlebars-template (0.1.5)
@@ -411,7 +411,7 @@ DEPENDENCIES
   byebug
   certified
   discourse-qunit-rails
-  discourse_email_parser
+  email_reply_trimmer (= 0.0.3)
   ember-rails
   ember-source (= 1.12.2)
   excon

lib/email/receiver.rb

@@ -5,6 +5,7 @@ require_dependency "email/html_cleaner"
 module Email
 
   class Receiver
+    include ActionView::Helpers::NumberHelper
 
     class ProcessingError             < StandardError; end
     class EmptyEmailError             < ProcessingError; end
@@ -110,29 +111,23 @@ module Email
       end
 
       # prefer text over html
-      if text.present?
+      text = trim_discourse_markers(text) if text.present?
-        text_encoding = text.encoding
+      text = EmailReplyTrimmer.trim(text) if text.present?
-        text = DiscourseEmailParser.parse_reply(text)
+      return text if text.present?
-        text = try_to_encode(text, text_encoding)
-        return text if text.present?
-      end
 
       # clean the html if that's all we've got
-      if html.present?
+      html = Email::HtmlCleaner.new(html).output_html if html.present?
-        html_encoding = html.encoding
+      html = trim_discourse_markers(html) if html.present?
-        html = Email::HtmlCleaner.new(html).output_html
+      html = EmailReplyTrimmer.trim(html) if html.present?
-        html = DiscourseEmailParser.parse_reply(html)
+      return html if html.present?
-        html = try_to_encode(html, html_encoding)
-        return html if html.present?
-      end
     end
 
     def fix_charset(mail_part)
       return nil if mail_part.blank? || mail_part.body.blank?
 
-      string = mail_part.body.to_s
+      string = mail_part.body.decoded rescue nil
 
-      # TODO (use charlock_holmes to properly detect encoding)
+      return nil if string.blank?
 
       # 1) use the charset provided
       if mail_part.charset.present?
@@ -150,6 +145,14 @@ module Email
       nil
     end
 
+    def previous_replies_regex
+      @previous_replies_regex ||= /^---\n\*#{I18n.t("user_notifications.previous_discussion")}\*\n/im
+    end
+
+    def trim_discourse_markers(reply)
+      reply.split(previous_replies_regex)[0]
+    end
+
     def from
       @from ||= @mail[:from].address_list.addresses.first
     end

spec/components/email/receiver_spec.rb

@@ -172,20 +172,17 @@ describe Email::Receiver do
 
     it "handles inline reply" do
       expect { process(:inline_reply) }.to change { topic.posts.count }
-      expect(topic.posts.last.raw).to eq("On Tue, Jan 15, 2016 at 11:12 AM, Bar Foo <info@unconfigured.discourse.org> wrote:\n\n> WAT <https://bar.com/users/wat> November 28\n>\n> This is the previous post.\n\nAnd this is *my* reply :+1:")
+      expect(topic.posts.last.raw).to eq("> WAT <https://bar.com/users/wat> November 28\n>\n> This is the previous post.\n\nAnd this is *my* reply :+1:")
     end
 
     it "retrieves the first part of multiple replies" do
       expect { process(:inline_mixed_replies) }.to change { topic.posts.count }
-      expect(topic.posts.last.raw).to eq("On Tue, Jan 15, 2016 at 11:12 AM, Bar Foo <info@unconfigured.discourse.org> wrote:\n\n> WAT <https://bar.com/users/wat> November 28\n>\n> This is the previous post.\n\nAnd this is *my* reply :+1:\n\n> This is another post.\n\nAnd this is **another** reply.")
+      expect(topic.posts.last.raw).to eq("> WAT <https://bar.com/users/wat> November 28\n>\n> This is the previous post.\n\nAnd this is *my* reply :+1:\n\n> This is another post.\n\nAnd this is **another** reply.")
     end
 
-    it "strips signatures" do
+    it "strips mobile/webmail signatures" do
       expect { process(:iphone_signature) }.to change { topic.posts.count }
       expect(topic.posts.last.raw).to eq("This is not the signature you're looking for.")
-
-      expect { process(:signature) }.to change { topic.posts.count }
-      expect(topic.posts.last.raw).to eq("You shall not sign!")
     end
 
     it "strips 'original message' context" do
@@ -193,14 +190,20 @@ describe Email::Receiver do
       expect(topic.posts.last.raw).to eq("This is a reply :)")
     end
 
-    it "supports attachments" do
+    it "supports attached images" do
-      expect { process(:no_body_with_attachments) }.to change { topic.posts.count }
+      expect { process(:no_body_with_image) }.to change { topic.posts.count }
       expect(topic.posts.last.raw).to match(/<img/)
 
-      expect { process(:inline_attachment) }.to change { topic.posts.count }
+      expect { process(:inline_image) }.to change { topic.posts.count }
       expect(topic.posts.last.raw).to match(/Before\s+<img.+\s+After/m)
     end
 
+    it "supports attachments" do
+      SiteSetting.authorized_extensions = "txt"
+      expect { process(:attached_txt_file) }.to change { topic.posts.count }
+      expect(topic.posts.last.raw).to match(/text\.txt/)
+    end
+
     it "supports liking via email" do
       expect { process(:like) }.to change(PostAction, :count)
     end

spec/fixtures/emails/attached_txt_file.eml

@@ -0,0 +1,30 @@
+Return-Path: <discourse@bar.com>
+From: Foo Bar <discourse@bar.com>
+To: reply+4f97315cc828096c9cb34c6f1a0d6fe8@bar.com
+Date: Sat, 30 Jan 2016 01:10:11 +0100
+Message-ID: <38@foo.bar.mail>
+Mime-Version: 1.0
+Content-Type: multipart/mixed;
+ boundary="--==_mimepart_56abff5d49749_ddf83fca6d033a28548ad";
+ charset=UTF-8
+Content-Transfer-Encoding: 7bit
+
+
+----==_mimepart_56abff5d49749_ddf83fca6d033a28548ad
+Content-Type: text/plain;
+ charset=UTF-8;
+ filename=text.txt
+Content-Transfer-Encoding: 7bit
+Content-Disposition: attachment;
+ filename=text.txt
+Content-ID: <56abff637aac_ddf83fca6d033a2855099@HAL.lan.mail>
+
+This is a txt file.
+
+----==_mimepart_56abff5d49749_ddf83fca6d033a28548ad
+Content-Type: text/plain;
+ charset=UTF-8
+Content-Transfer-Encoding: 7bit
+
+Please find some text file attached.
+----==_mimepart_56abff5d49749_ddf83fca6d033a28548ad--

spec/fixtures/emails/signature.eml

@@ -1,12 +0,0 @@
-Return-Path: <discourse@bar.com>
-From: Foo Bar <discourse@bar.com>
-To: reply+4f97315cc828096c9cb34c6f1a0d6fe8@bar.com
-Date: Fri, 15 Jan 2016 00:12:43 +0100
-Message-ID: <26@foo.bar.mail>
-Mime-Version: 1.0
-Content-Type: text/plain; charset=UTF-8
-
-You shall not sign!
-
----
-Foo Bar