From d87edce6c35d3e833c0653f5d3d5e68849f1b075 Mon Sep 17 00:00:00 2001 From: riking Date: Fri, 1 Aug 2014 09:56:15 -0700 Subject: [PATCH 1/6] Pass rejection message along in rejection mail if present --- app/jobs/scheduled/poll_mailbox.rb | 15 ++++++++++++--- app/mailers/rejection_mailer.rb | 27 +++++++++++++++++++++------ config/locales/server.en.yml | 13 ++++++++++++- lib/email/message_builder.rb | 2 +- 4 files changed, 46 insertions(+), 11 deletions(-) diff --git a/app/jobs/scheduled/poll_mailbox.rb b/app/jobs/scheduled/poll_mailbox.rb index 82bdc679559..36cd759f981 100644 --- a/app/jobs/scheduled/poll_mailbox.rb +++ b/app/jobs/scheduled/poll_mailbox.rb @@ -25,6 +25,7 @@ module Jobs Email::Receiver.new(mail_string).process rescue => e message_template = nil + template_args = {} case e when Email::Receiver::UserNotSufficientTrustLevelError message_template = :email_reject_trust_level @@ -39,8 +40,13 @@ module Jobs when ActiveRecord::Rollback message_template = :email_reject_post_error when Email::Receiver::InvalidPost - # TODO there is a message in this exception, place it in email - message_template = :email_reject_post_error + if e.message.length < 6 + message_template = :email_reject_post_error + else + message_template = :email_reject_post_error_specified + template_args[:post_error] = e.message + end + else message_template = nil end @@ -48,7 +54,10 @@ module Jobs if message_template # inform the user about the rejection message = Mail::Message.new(mail_string) - client_message = RejectionMailer.send_rejection(message.from, message.body, message.subject, message.to, message_template) + template_args[:former_title] = message.subject + template_args[:destination] = message.to + + client_message = RejectionMailer.send_rejection(message_template, message.from, template_args) Email::Sender.new(client_message, message_template).send else Discourse.handle_exception(e, error_context(@args, "Unrecognized error type when processing incoming email", mail: mail_string)) diff --git a/app/mailers/rejection_mailer.rb b/app/mailers/rejection_mailer.rb index 20727b459de..6eaf4b915e8 100644 --- a/app/mailers/rejection_mailer.rb +++ b/app/mailers/rejection_mailer.rb @@ -3,12 +3,27 @@ require_dependency 'email/message_builder' class RejectionMailer < ActionMailer::Base include Email::BuildEmailHelper - def send_rejection(message_from, message_body, message_subject, forum_address, template) - build_email(message_from, - template: "system_messages.#{template}", - source: message_body, - former_title: message_subject, - destination: forum_address) + DISALLOWED_TEMPLATE_ARGS = [:to, :from, :site_name, :base_url, + :user_preferences_url, + :include_respond_instructions, :html_override, + :add_unsubscribe_link, :respond_instructions, + :style, :body, :post_id, :topic_id, :subject, + :template, :allow_reply_by_email, + :private_reply, :from_alias] + + # Send an email rejection message. + # + # template - i18n key under system_messages + # message_from - Who to send the rejection messsage to + # template_args - arguments to pass to i18n for interpolation into the message + # Certain keys are disallowed in template_args to avoid confusing the + # BuildEmailHelper. You can see the list in DISALLOWED_TEMPLATE_ARGS. + def send_rejection(template, message_from, template_args) + if template_args.keys.any? { |k| DISALLOWED_TEMPLATE_ARGS.include? k } + raise ArgumentError.new('Reserved key in template arguments') + end + + build_email(message_from, template_args.merge(template: "system_messages.#{template}")) end end diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index e97675c5821..f3e1c694165 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -1419,7 +1419,18 @@ en: text_body_template: | We're sorry, but your email message to %{destination} (titled %{former_title}) didn't work. - Some possible causes are: complex formatting, message too large, message too small. Please try again. + Some possible causes are: complex formatting, message too large, message too small. Please try again, or post via the website if this continues. + + email_reject_post_error_specified: + subject_template: "Email issue -- Posting error" + text_body_template: | + We're sorry, but your email message to %{destination} (titled %{former_title}) didn't work. + + Rejection message: + + %{post_error} + + Please attempt to fix the errors and try again. email_reject_reply_key: subject_template: "Email issue -- Bad Reply Key" diff --git a/lib/email/message_builder.rb b/lib/email/message_builder.rb index 310f97f9ea5..20482788606 100644 --- a/lib/email/message_builder.rb +++ b/lib/email/message_builder.rb @@ -49,7 +49,7 @@ module Email return unless html_override = @opts[:html_override] if @opts[:add_unsubscribe_link] - if response_instructions = @template_args[:respond_instructions] + if response_instructions = @opts[:respond_instructions] respond_instructions = PrettyText.cook(response_instructions).html_safe html_override.gsub!("%{respond_instructions}", respond_instructions) end From d7df4e597995bdf0ceff7fee56fda3e36427a58b Mon Sep 17 00:00:00 2001 From: riking Date: Fri, 1 Aug 2014 11:03:03 -0700 Subject: [PATCH 2/6] Start making better-written tests for the email job --- spec/fixtures/emails/valid_incoming.cooked | 5 ++ spec/fixtures/emails/valid_incoming.eml | Bin 1228 -> 1296 bytes spec/fixtures/emails/valid_reply.cooked | 3 + spec/fixtures/emails/valid_reply.eml | Bin 1971 -> 1967 bytes spec/jobs/poll_mailbox_spec.rb | 87 +++++++++++++++++++++ 5 files changed, 95 insertions(+) create mode 100644 spec/fixtures/emails/valid_incoming.cooked create mode 100644 spec/fixtures/emails/valid_reply.cooked diff --git a/spec/fixtures/emails/valid_incoming.cooked b/spec/fixtures/emails/valid_incoming.cooked new file mode 100644 index 00000000000..2bf35825373 --- /dev/null +++ b/spec/fixtures/emails/valid_incoming.cooked @@ -0,0 +1,5 @@ +

Hey folks,

+ +

I was thinking. Wouldn't it be great if we could post topics via email? Yes it would!

+ +

Jakie

diff --git a/spec/fixtures/emails/valid_incoming.eml b/spec/fixtures/emails/valid_incoming.eml index 5e8db53319ed6c07fa2b07392cf64f066c2f9449..5e03d1dc2236ba675506e566dc03f3a9757173ad 100644 GIT binary patch delta 94 zcmX@ZIe}}#W~RWb#Ozdu)ZE0(9KFAYu$W+iH8XQc9tT? delta 26 hcmbQhb%t}pW+q;@Ab(#wu8@2y1)C86%`D73OaO2h2OI could not disagree more. I am obviously biased but adventure time is the greatest show ever created. Everyone should watch it.

+ +

- Jake out

diff --git a/spec/fixtures/emails/valid_reply.eml b/spec/fixtures/emails/valid_reply.eml index 1e696389954ca0b2ca63fe4a0f4dfd5853f5e61a..ec387c8d535c68292e0ff8565e8625704047f173 100644 GIT binary patch delta 62 zcmdnYzn*`C8?$(7ZenJRUTQ^RZb43}UUGh}9al)cmBM62Hg(aI%;Mzy(xT#2Wckf= HnZ;NDpxqTf delta 24 gcmZ3_znOo78}meAVV015D}~9jYz~`eGK;YQ09?}su>b%7 diff --git a/spec/jobs/poll_mailbox_spec.rb b/spec/jobs/poll_mailbox_spec.rb index 3adcb78c735..90c8ec9bea9 100644 --- a/spec/jobs/poll_mailbox_spec.rb +++ b/spec/jobs/poll_mailbox_spec.rb @@ -41,6 +41,93 @@ describe Jobs::PollMailbox do end + # Testing mock for the email objects that you get + # from Net::POP3.start { |pop| pop.mails } + class MockPop3EmailObject + def initialize(mail_string) + @message = mail_string + @delete_called = 0 + end + + def pop + @message + end + + def delete + @delete_called += 1 + end + + # call 'assert email.deleted?' at the end of the test + def deleted? + @delete_called == 1 + end + end + + describe "processing email B" do + let(:category) { Fabricate(:category) } + let(:user) { Fabricate(:user) } + + before do + SiteSetting.email_in = true + SiteSetting.reply_by_email_address = 'reply+%{reply_key}@discourse.example.com' + category.email_in = 'incoming+amazing@discourse.example.com' + category.save + user.change_trust_level! :regular + user.username = 'Jake' + user.email = 'jake@email.example.com' + user.save + end + + describe "valid incoming email" do + let(:email) { MockPop3EmailObject.new fixture_file('emails/valid_incoming.eml')} + let(:expected_post) { fixture_file('emails/valid_incoming.cooked') } + + it "posts a new topic with the correct content" do + + poller.handle_mail(email) + + topic = Topic.where(category: category).where.not(id: category.topic_id).first + assert topic.present? + post = topic.posts.first + assert_equal expected_post.strip, post.cooked.strip + + assert email.deleted? + end + end + + describe "valid reply" do + let(:email) { MockPop3EmailObject.new fixture_file('emails/valid_reply.eml')} + let(:expected_post) { fixture_file('emails/valid_reply.cooked')} + let(:topic) { Fabricate(:topic) } + let(:first_post) { Fabricate(:post, topic: topic, post_number: 1)} + + before do + first_post.save + EmailLog.create(to_address: 'jake@email.example.com', + email_type: 'user_posted', + reply_key: '59d8df8370b7e95c5a49fbf86aeb2c93', + post: first_post, + topic: topic) + end + + pending "creates a new post with the correct content" do + RejectionMailer.expects(:send_rejection).never + Discourse.expects(:handle_exception).never + + poller.handle_mail(email) + + new_post = Post.where(topic: topic, post_number: 2) + assert new_post.present? + + assert_equal expected_post.strip, new_post.cooked.strip + + assert email.deleted? + end + end + + + end + describe "processing email" do let!(:receiver) { mock } From c0b2b9b341254d0e98b2f2b3a86488812fd6e42a Mon Sep 17 00:00:00 2001 From: riking Date: Fri, 1 Aug 2014 11:03:55 -0700 Subject: [PATCH 3/6] Refactor out handle_failure method in PollMailbox --- app/jobs/scheduled/poll_mailbox.rb | 79 ++++++++++++++++-------------- spec/jobs/poll_mailbox_spec.rb | 8 ++- 2 files changed, 47 insertions(+), 40 deletions(-) diff --git a/app/jobs/scheduled/poll_mailbox.rb b/app/jobs/scheduled/poll_mailbox.rb index 36cd759f981..749dd67748e 100644 --- a/app/jobs/scheduled/poll_mailbox.rb +++ b/app/jobs/scheduled/poll_mailbox.rb @@ -24,49 +24,52 @@ module Jobs mail_string = mail.pop Email::Receiver.new(mail_string).process rescue => e - message_template = nil - template_args = {} - case e - when Email::Receiver::UserNotSufficientTrustLevelError - message_template = :email_reject_trust_level - when Email::Receiver::UserNotFoundError - message_template = :email_reject_no_account - when Email::Receiver::EmptyEmailError - message_template = :email_reject_empty - when Email::Receiver::EmailUnparsableError - message_template = :email_reject_parsing - when Email::Receiver::EmailLogNotFound - message_template = :email_reject_reply_key - when ActiveRecord::Rollback - message_template = :email_reject_post_error - when Email::Receiver::InvalidPost - if e.message.length < 6 - message_template = :email_reject_post_error - else - message_template = :email_reject_post_error_specified - template_args[:post_error] = e.message - end - - else - message_template = nil - end - - if message_template - # inform the user about the rejection - message = Mail::Message.new(mail_string) - template_args[:former_title] = message.subject - template_args[:destination] = message.to - - client_message = RejectionMailer.send_rejection(message_template, message.from, template_args) - Email::Sender.new(client_message, message_template).send - else - Discourse.handle_exception(e, error_context(@args, "Unrecognized error type when processing incoming email", mail: mail_string)) - end + handle_failure(mail_string, e) ensure mail.delete end end + def handle_failure(mail_string, e) + template_args = {} + case e + when Email::Receiver::UserNotSufficientTrustLevelError + message_template = :email_reject_trust_level + when Email::Receiver::UserNotFoundError + message_template = :email_reject_no_account + when Email::Receiver::EmptyEmailError + message_template = :email_reject_empty + when Email::Receiver::EmailUnparsableError + message_template = :email_reject_parsing + when Email::Receiver::EmailLogNotFound + message_template = :email_reject_reply_key + when ActiveRecord::Rollback + message_template = :email_reject_post_error + when Email::Receiver::InvalidPost + if e.message.length < 6 + message_template = :email_reject_post_error + else + message_template = :email_reject_post_error_specified + template_args[:post_error] = e.message + end + + else + message_template = nil + end + + if message_template + # inform the user about the rejection + message = Mail::Message.new(mail_string) + template_args[:former_title] = message.subject + template_args[:destination] = message.to + + client_message = RejectionMailer.send_rejection(message_template, message.from, template_args) + Email::Sender.new(client_message, message_template).send + else + Discourse.handle_exception(e, error_context(@args, "Unrecognized error type when processing incoming email", mail: mail_string)) + end + end + def poll_pop3s if !SiteSetting.pop3s_polling_insecure Net::POP3.enable_ssl(OpenSSL::SSL::VERIFY_NONE) diff --git a/spec/jobs/poll_mailbox_spec.rb b/spec/jobs/poll_mailbox_spec.rb index 90c8ec9bea9..4875ead98f0 100644 --- a/spec/jobs/poll_mailbox_spec.rb +++ b/spec/jobs/poll_mailbox_spec.rb @@ -63,6 +63,10 @@ describe Jobs::PollMailbox do end end + def expect_success + Jobs::PollMailbox.expects(:handle_failure).never + end + describe "processing email B" do let(:category) { Fabricate(:category) } let(:user) { Fabricate(:user) } @@ -83,6 +87,7 @@ describe Jobs::PollMailbox do let(:expected_post) { fixture_file('emails/valid_incoming.cooked') } it "posts a new topic with the correct content" do + expect_success poller.handle_mail(email) @@ -111,8 +116,7 @@ describe Jobs::PollMailbox do end pending "creates a new post with the correct content" do - RejectionMailer.expects(:send_rejection).never - Discourse.expects(:handle_exception).never + expect_success poller.handle_mail(email) From 0faea8ee0b4d9f1435411ccd40b58b9e2aefa5cb Mon Sep 17 00:00:00 2001 From: riking Date: Fri, 1 Aug 2014 11:38:44 -0700 Subject: [PATCH 4/6] Attempt at checking throws.... --- spec/fixtures/emails/valid_reply.cooked | 5 +++-- spec/jobs/poll_mailbox_spec.rb | 25 +++++++++++++++++-------- 2 files changed, 20 insertions(+), 10 deletions(-) diff --git a/spec/fixtures/emails/valid_reply.cooked b/spec/fixtures/emails/valid_reply.cooked index d622863da83..4bce79ad12a 100644 --- a/spec/fixtures/emails/valid_reply.cooked +++ b/spec/fixtures/emails/valid_reply.cooked @@ -1,3 +1,4 @@ -

I could not disagree more. I am obviously biased but adventure time is the greatest show ever created. Everyone should watch it.

+

I could not disagree more. I am obviously biased but adventure time is the +greatest show ever created. Everyone should watch it.

-

- Jake out

+
  • Jake out
diff --git a/spec/jobs/poll_mailbox_spec.rb b/spec/jobs/poll_mailbox_spec.rb index 4875ead98f0..a96e30916c3 100644 --- a/spec/jobs/poll_mailbox_spec.rb +++ b/spec/jobs/poll_mailbox_spec.rb @@ -67,7 +67,7 @@ describe Jobs::PollMailbox do Jobs::PollMailbox.expects(:handle_failure).never end - describe "processing email B" do + describe "processing emails" do let(:category) { Fabricate(:category) } let(:user) { Fabricate(:user) } @@ -82,11 +82,11 @@ describe Jobs::PollMailbox do user.save end - describe "valid incoming email" do + describe "a valid incoming email" do let(:email) { MockPop3EmailObject.new fixture_file('emails/valid_incoming.eml')} let(:expected_post) { fixture_file('emails/valid_incoming.cooked') } - it "posts a new topic with the correct content" do + it "posts a new topic" do expect_success poller.handle_mail(email) @@ -100,7 +100,7 @@ describe Jobs::PollMailbox do end end - describe "valid reply" do + describe "a valid reply" do let(:email) { MockPop3EmailObject.new fixture_file('emails/valid_reply.eml')} let(:expected_post) { fixture_file('emails/valid_reply.cooked')} let(:topic) { Fabricate(:topic) } @@ -111,28 +111,37 @@ describe Jobs::PollMailbox do EmailLog.create(to_address: 'jake@email.example.com', email_type: 'user_posted', reply_key: '59d8df8370b7e95c5a49fbf86aeb2c93', + user: user, post: first_post, topic: topic) end - pending "creates a new post with the correct content" do + it "creates a new post" do expect_success poller.handle_mail(email) - new_post = Post.where(topic: topic, post_number: 2) + new_post = Post.find_by(topic: topic, post_number: 2) assert new_post.present? - assert_equal expected_post.strip, new_post.cooked.strip assert email.deleted? end end + describe "without an email log" do + let(:email) { MockPop3EmailObject.new fixture_file('emails/valid_reply.eml')} + it "handles an EmailLogNotFound error" do + poller.expects(:handle_failure).with { |mail_string, ex| ex.is_a? Email::Receiver::EmailLogNotFound } + + poller.handle_mail(email) + assert email.deleted? + end + end end - describe "processing email" do + describe "processing email A" do let!(:receiver) { mock } let!(:email_string) { fixture_file("emails/valid_incoming.eml") } From 63cdde3d965e9f32c4bd14436aa01a2a881b690a Mon Sep 17 00:00:00 2001 From: riking Date: Fri, 1 Aug 2014 12:40:28 -0700 Subject: [PATCH 5/6] Add more tests, undo some changes to fixture files Was causing Email::Reciever tests to fail --- lib/email/receiver.rb | 26 ++-- spec/fixtures/emails/bottom_reply.eml | Bin 0 -> 6864 bytes spec/fixtures/emails/empty.eml | Bin 0 -> 1354 bytes spec/fixtures/emails/valid_incoming.eml | Bin 1296 -> 1228 bytes spec/fixtures/emails/valid_reply.eml | Bin 1967 -> 1971 bytes spec/fixtures/emails/wrong_reply_key.eml | Bin 0 -> 1955 bytes spec/jobs/poll_mailbox_spec.rb | 168 +++++++++++------------ 7 files changed, 97 insertions(+), 97 deletions(-) create mode 100644 spec/fixtures/emails/bottom_reply.eml create mode 100644 spec/fixtures/emails/empty.eml create mode 100644 spec/fixtures/emails/wrong_reply_key.eml diff --git a/lib/email/receiver.rb b/lib/email/receiver.rb index 13a36939263..94201f5f93f 100644 --- a/lib/email/receiver.rb +++ b/lib/email/receiver.rb @@ -67,6 +67,19 @@ module Email end end + def is_in_email? + @allow_strangers = false + return false unless SiteSetting.email_in + + category = Category.find_by_email(@message.to.first) + return false unless category + + @category_id = category.id + @allow_strangers = category.email_in_allow_strangers + + true + end + private def parse_body @@ -135,19 +148,6 @@ module Email @body.strip! end - def is_in_email? - @allow_strangers = false - return false unless SiteSetting.email_in - - category = Category.find_by_email(@message.to.first) - return false unless category - - @category_id = category.id - @allow_strangers = category.email_in_allow_strangers - - true - end - def wrap_body_in_quote @body = "[quote=\"#{@message.from.first}\"] #{@body} diff --git a/spec/fixtures/emails/bottom_reply.eml b/spec/fixtures/emails/bottom_reply.eml new file mode 100644 index 0000000000000000000000000000000000000000..5fc992971fc2f94932992a0956cf0667675af96d GIT binary patch literal 6864 zcmcIo*>d8@5`DJ5qN4j@?63ur*epEuv;Y~f+05c?;)X*=g%FlhYAs+K_v7y=32eOd zG81uWw+*5yGb{7t$;=v3lR8UkRmpTsWGN?0m7J{R6qT%;cuq#$@qjoMnJTIo1+pwx z-Pc^^lorTOJE;9{x&Z3y?zrOujRw_9O z|K$H12z&6ul=>ERa~7RChM!}hjbmWX%y~M<%)RPf4TL@TMLs?+)r)fFgpB7293j6t zLXuKP6v?547ulbT>jy&Lwcv_434L*3#PcfobIC%N`QKi3oxxk9!0^QSoSKg`6t8I* zIn1vT{#1)1BZ?fK6mo@}LiXWvS(M87IUvmcX!%xHhc2mG-F_OyMw~=d(ql&?GFeDZ zMWvKeNl3pZPDriOVooA4jpFF^ygwOsw4+X=Kbq7=^XyaZJB`h>XynF7OMRe|yLO2NL==RqA@V-q3YnZ^lCM z=WiV0><8eHfuA?vxfQRSFp0#GY4|=*StHI|w2Ra%o|^B_JWEzZt(E*u{sO!hzI=WFWjr~chN|L$V4Z={E|M4zsXNWj9FC?%zY zEKl9^60=dQSdts7=6$_S$?wNa*!wi=Gj7C?cT$v)_J(a z|It9}wzSCzPS2ILU*dW1LaTMMhxN`wd+?xNJ+$;rA8+A$vTt&ZAe`E&Wa|YYk~wve zKDjA1zb7=zSrmmT*{SRZU1oWw3dtk8)^K1 zu~s-wk$7P|Qu5~lzrPvQlKQB3G(Qr}G}5|E>f^Ca+QT~OQb_l0PKOq(B%<{P@-nT&D9C6?P>NB3QGkWC zNBKkOQj}4fitPp}FmGF=N&(I($p~>%lU1o${Py0bXF?YSis#IA*(wRra`Q7FB2i|j zOTI}_XtzovMJ}jF0w_@mrSi9!x-okK>U<;gqBvx}or7nftKHRKE5DqRggWzMxKbRi z131D<+}H_>FwRGgjbd;@JO{0feHz$-7|jjf?L^oMLKZWVxu;*?Uz7wRMkA3=mtYrq zfRKJUAybz4mJzN`zkKg{YWMKmP_#$C#hRlpFIpQ;6tKwQ`+4+S$EQNuM=uk&>(mwh z_=FQ+Qq*i5sY<#@9OuD&XbIDnrYK4UQC4{4m<3zhL<~*P)^L(>Ux0RqS}AF7aN$vJ zN<(ZNnpwfDBI;5_lvGhJJVM=rlE@uB;}PIW0FC zHgeD1du3ePv?6EQO&Rik3W!^gBM3+pJ7{bGil41MBSvV>(c0u)U^{CS&uy{k;g2Quu_yul>*H2 z6B4bXn0i^@JhWgUJjv&vUCS!Paz!oy>4<8!p3Dp!Ai*lljo1cSJoay40$^>BLai(% zvJCiIDM@mnL{RR)CukKy!e+z>csv2|z%UD0yv1nb{}BtDi5Ab+fFTFi zYhH}&K=SbOY)MlN$ZGDGbFxA<^7|RylJP|1rT`%zPHzo> zr1cT|Vu8<}aBylvfDG|)8-iryiNy(MZY(LGk!b{w(;BdJ1!6`j7mZ>oz0o3Jy?e@_a8k{KIHRnXZ@iG2-@F=>_ z)SoG17+Z{QybCGRh$yaz3@<&ihn55FJ{*Zl4+9?F`F`6sasP$2WX=uXKg5pvc*E{od(1%VA>>;$3Lto+ zhxw2NSRsMZ!0{#p0To3K!ch!eq+|FtPW8CG+uZrvgdgDpVi1={0QZP6h59V9=P(WM zBIf`g>5p(9#V-w!sc<}$g*!sFZ-EOG?TkaSLewU>(&6L?m%_*u?gWuglWBs{>8GN$ zB47T_<2b11^Q+Zrr%dLB$laXnTapUpL&nRVpS}5TJ^33Q$`5Ca6XF{lJ^jM-BON|+ z=vsDW3;;fKz!H4MXy6_&1`c1=a>aPn^k%!qFO|s4-D3J&xZYlqHvMGKBuiNkoU683 z)AU+h)0(xbi?MRwZD<<1(9extchH?j^RC+LM1#CiERG7R`0ie(*Y?Ft+p3iYx}*uw z{WX2owX&+L4-fMayT)g8bzQEkYyI_Xv=|I%Y-zz*;&Dq)Q!j5VZ4NQXH$#m6kGX z4GWV}Z8`VHsv4J)m9VyEih18_-bu-M{kkw*`ckDf)7m$so6+sOKkMa7?X?G!tob4u zJFENr>U?n?ULMESi&!5f!kVtN%CLRsOx zQ$LyL&+2()Sz6rLlJ}(LZ>G#{#%pybEc@NdC+lhW_&jqv`8vAq>*?cjC$B%=4;!{d;?4*SohKWz{C?Z-zoSP7?>x9ucL z3%%F)EGa%2Cqz-;sf247GU>hC=^qiwM?hY)cDiYCs9#O=(fGqim)0+^-e$7nRrkxz w0QDtD#vw~k)BYlL+*_c^rZ?3ddNLY*l%{2)Zk2HQiojp*N67qdUeXBv13H(Q6951J literal 0 HcmV?d00001 diff --git a/spec/fixtures/emails/empty.eml b/spec/fixtures/emails/empty.eml new file mode 100644 index 0000000000000000000000000000000000000000..33f21b4f6901f7f7caadc20fcda27fda84ef2c4f GIT binary patch literal 1354 zcmcIkU2mI85Pj!Y%v&9$kX=3uZ0jhlNz+(yV;K@viu%CEfVZ)CDJ)#Ozy7Y{xUFxS zhpVa}!Lp2I&&)Y<7O1*LnDbOE8NB_HKH^1M^eFT$D!xWbN_lQ9Fh|~F!Jus98hDP6 zTvXOCkuZ4Z1m8d_^G8&lPaxYKf?+V(HeCz7ZP^xe%m_1{3iJ7khNtb<4)Gg0C-4tf zOSqXO^YI%b)1txC#jg;uU14_^H9 za6Rvb%pMY-l|e?B&PrLX&tN*mQ3=1RH?XK{IcLwW=b}Q(eXim%)@1-Mpz)#`N4%dB zIt+(C40>!@F2yLhiF+2y4VqBRZmYX{BtAjvM-pc9V!M7(Lob1 zLzR98?DAYu$W+iH8XQc9tT? diff --git a/spec/fixtures/emails/valid_reply.eml b/spec/fixtures/emails/valid_reply.eml index ec387c8d535c68292e0ff8565e8625704047f173..1e696389954ca0b2ca63fe4a0f4dfd5853f5e61a 100644 GIT binary patch delta 24 gcmZ3_znOo78}meAVV015D}~9jYz~`eGK;YQ09?}su>b%7 delta 62 zcmdnYzn*`C8?$(7ZenJRUTQ^RZb43}UUGh}9al)cmBM62Hg(aI%;Mzy(xT#2Wckf= HnZ;NDpxqTf diff --git a/spec/fixtures/emails/wrong_reply_key.eml b/spec/fixtures/emails/wrong_reply_key.eml new file mode 100644 index 0000000000000000000000000000000000000000..74963826f7f318a71f3133af40db05fc32ee37ca GIT binary patch literal 1955 zcmcIlYj4^}6#edBaraY+8XJ!v_|Xz28`6|0G&GQ?QWUNHxWE*caXn*#`|EoRBn@q{ z((Yls`Q#>10$FrHP1z+AYf1UNP=SPTFkTD_U>=jRAhO(F3v{O2Jjq8zjx`1 z4;MD;M4bRyD=Zb4qC37Etyp_Bx?H)C@nVK$_xjW`M;}ih9M_ur=-K8AaDbpV`4e}|5T~huVeYw7Jx>6U#^m|@ijHbs8Vb*l>OTs@gMpgP1 zGu>1b!(H!D6EHxX+yk!oLYK1AP)MaQ2QJ{v$)f|nm@EKB)%uMwpp|*MRc#E+vW-(I zG#Q0Ct_zB+ajo0MBH`kl0og1e+39Y7bY+I2ADQgu=`s;&f~9#TGMST$#1N3ujW6bi zoxar~%05iCo_Xk3}+@glWCpByG&%nv*#?AZ2J2gbgS>~|ts?`RM4FYo^AJnf9sw$hSKbp+D z_l653xM9T=*Mu%Xyx?LETGBYELJIn%&9pA!c6%YSWHF;5h+W&Z+q(V4j@htds%qVC zG}d-E;l~mxp{i76B~NktT_pA>7dEq}4tF_ga$vZt6tP&8NWw>vp}KFqoV7Xk*Ryj6 HmT~k0)FWpf literal 0 HcmV?d00001 diff --git a/spec/jobs/poll_mailbox_spec.rb b/spec/jobs/poll_mailbox_spec.rb index a96e30916c3..d9f9dbd104e 100644 --- a/spec/jobs/poll_mailbox_spec.rb +++ b/spec/jobs/poll_mailbox_spec.rb @@ -64,7 +64,11 @@ describe Jobs::PollMailbox do end def expect_success - Jobs::PollMailbox.expects(:handle_failure).never + poller.expects(:handle_failure).never + end + + def expect_exception(clazz) + poller.expects(:handle_failure).with(anything, instance_of(clazz)) end describe "processing emails" do @@ -73,30 +77,65 @@ describe Jobs::PollMailbox do before do SiteSetting.email_in = true - SiteSetting.reply_by_email_address = 'reply+%{reply_key}@discourse.example.com' - category.email_in = 'incoming+amazing@discourse.example.com' + SiteSetting.reply_by_email_address = "reply+%{reply_key}@appmail.adventuretime.ooo" + category.email_in = 'incoming+amazing@appmail.adventuretime.ooo' category.save user.change_trust_level! :regular user.username = 'Jake' - user.email = 'jake@email.example.com' + user.email = 'jake@adventuretime.ooo' user.save end describe "a valid incoming email" do - let(:email) { MockPop3EmailObject.new fixture_file('emails/valid_incoming.eml')} + let(:email) { + # this string replacing is kinda dumb + str = fixture_file('emails/valid_incoming.eml') + str = str.gsub("FROM", 'jake@adventuretime.ooo').gsub("TO", 'incoming+amazing@appmail.adventuretime.ooo') + MockPop3EmailObject.new str + } let(:expected_post) { fixture_file('emails/valid_incoming.cooked') } - it "posts a new topic" do + it "posts a new topic with the correct content" do expect_success poller.handle_mail(email) - topic = Topic.where(category: category).where.not(id: category.topic_id).first - assert topic.present? - post = topic.posts.first - assert_equal expected_post.strip, post.cooked.strip + topic = Topic.where(category: category).where.not(id: category.topic_id).last + topic.should be_present + topic.title.should == "We should have a post-by-email-feature" - assert email.deleted? + post = topic.posts.first + post.cooked.strip.should == expected_post.strip + + email.should be_deleted + end + + describe "with insufficient trust" do + before do + user.change_trust_level! :newuser + end + + it "raises a UserNotSufficientTrustLevelError" do + expect_exception Email::Receiver::UserNotSufficientTrustLevelError + + poller.handle_mail(email) + end + + it "posts the topic if allow_strangers is true" do + begin + category.email_in_allow_strangers = true + category.save + + expect_success + poller.handle_mail(email) + topic = Topic.where(category: category).where.not(id: category.topic_id).last + topic.should be_present + topic.title.should == "We should have a post-by-email-feature" + ensure + category.email_in_allow_strangers = false + category.save + end + end end end @@ -125,86 +164,47 @@ describe Jobs::PollMailbox do assert new_post.present? assert_equal expected_post.strip, new_post.cooked.strip - assert email.deleted? + email.should be_deleted end - end - describe "without an email log" do - let(:email) { MockPop3EmailObject.new fixture_file('emails/valid_reply.eml')} + describe "with the wrong reply key" do + let(:email) { MockPop3EmailObject.new fixture_file('emails/wrong_reply_key.eml')} - it "handles an EmailLogNotFound error" do - poller.expects(:handle_failure).with { |mail_string, ex| ex.is_a? Email::Receiver::EmailLogNotFound } - - poller.handle_mail(email) - assert email.deleted? - end - end - end - - describe "processing email A" do - - let!(:receiver) { mock } - let!(:email_string) { fixture_file("emails/valid_incoming.eml") } - let!(:email) { mock } - - before do - email.stubs(:pop).returns(email_string) - Email::Receiver.expects(:new).with(email_string).returns(receiver) - end - - describe "all goes fine" do - - it "email gets deleted" do - receiver.expects(:process) - email.expects(:delete) - - poller.handle_mail(email) - end - end - - describe "raises Untrusted error" do - - it "sends a reply and deletes the email" do - receiver.expects(:process).raises(Email::Receiver::UserNotSufficientTrustLevelError) - email.expects(:delete) - - message = Mail::Message.new(email_string) - Mail::Message.expects(:new).with(email_string).returns(message) - - client_message = mock - sender_object = mock - - RejectionMailer.expects(:send_rejection).with( - message.from, message.body, message.subject, message.to, :email_reject_trust_level - ).returns(client_message) - Email::Sender.expects(:new).with(client_message, :email_reject_trust_level).returns(sender_object) - sender_object.expects(:send) - - poller.handle_mail(email) - end - end - - describe "raises error" do - - [ Email::Receiver::ProcessingError, - Email::Receiver::EmailUnparsableError, - Email::Receiver::EmptyEmailError, - Email::Receiver::UserNotFoundError, - Email::Receiver::EmailLogNotFound, - ActiveRecord::Rollback, - TypeError - ].each do |exception| - - it "deletes email on #{exception}" do - receiver.expects(:process).raises(exception) - email.expects(:delete) - - Discourse.stubs(:handle_exception) + it "raises an EmailLogNotFound error" do + expect_exception Email::Receiver::EmailLogNotFound poller.handle_mail(email) + email.should be_deleted end - end + end + + describe "in failure conditions" do + + it "a valid reply without an email log raises an EmailLogNotFound error" do + email = MockPop3EmailObject.new fixture_file('emails/valid_reply.eml') + expect_exception Email::Receiver::EmailLogNotFound + + poller.handle_mail(email) + email.should be_deleted + end + + it "a no content reply raises an EmailUnparsableError" do + email = MockPop3EmailObject.new fixture_file('emails/no_content_reply.eml') + expect_exception Email::Receiver::EmailUnparsableError + + poller.handle_mail(email) + email.should be_deleted + end + + it "a fully empty email raises an EmptyEmailError" do + email = MockPop3EmailObject.new fixture_file('emails/empty.eml') + expect_exception Email::Receiver::EmptyEmailError + + poller.handle_mail(email) + email.should be_deleted + end + end end From de27c6b4b99c120f478abaabdea55ea13c520000 Mon Sep 17 00:00:00 2001 From: riking Date: Fri, 1 Aug 2014 12:47:08 -0700 Subject: [PATCH 6/6] Revert bad diff --- lib/email/message_builder.rb | 2 +- lib/email/receiver.rb | 26 +++++++++++++------------- 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/lib/email/message_builder.rb b/lib/email/message_builder.rb index 20482788606..310f97f9ea5 100644 --- a/lib/email/message_builder.rb +++ b/lib/email/message_builder.rb @@ -49,7 +49,7 @@ module Email return unless html_override = @opts[:html_override] if @opts[:add_unsubscribe_link] - if response_instructions = @opts[:respond_instructions] + if response_instructions = @template_args[:respond_instructions] respond_instructions = PrettyText.cook(response_instructions).html_safe html_override.gsub!("%{respond_instructions}", respond_instructions) end diff --git a/lib/email/receiver.rb b/lib/email/receiver.rb index 94201f5f93f..13a36939263 100644 --- a/lib/email/receiver.rb +++ b/lib/email/receiver.rb @@ -67,19 +67,6 @@ module Email end end - def is_in_email? - @allow_strangers = false - return false unless SiteSetting.email_in - - category = Category.find_by_email(@message.to.first) - return false unless category - - @category_id = category.id - @allow_strangers = category.email_in_allow_strangers - - true - end - private def parse_body @@ -148,6 +135,19 @@ module Email @body.strip! end + def is_in_email? + @allow_strangers = false + return false unless SiteSetting.email_in + + category = Category.find_by_email(@message.to.first) + return false unless category + + @category_id = category.id + @allow_strangers = category.email_in_allow_strangers + + true + end + def wrap_body_in_quote @body = "[quote=\"#{@message.from.first}\"] #{@body}