From 32401535b9b043e258e925474dbd54adbf5a5056 Mon Sep 17 00:00:00 2001 From: George Thomas Date: Thu, 12 Jul 2018 14:30:36 +0530 Subject: [PATCH] Improve email address parsing If you enter the following RFC 2822 compliant address: `John Doe ` Gitlab will attempt to send three emails: 1) John 2) Doe 3) john@doe.com With this change given the following: `John Doe ` `Jane Doe ` Gitlab will send emails to `johndoe@example.com` and `janedoe@example.com` --- app/workers/emails_on_push_worker.rb | 8 ++- .../accept-rf3-2822-compliant-addresses.yml | 5 ++ .../project/integrations/emails_on_push.md | 2 +- spec/workers/emails_on_push_worker_spec.rb | 61 +++++++++++++++---- 4 files changed, 62 insertions(+), 14 deletions(-) create mode 100644 changelogs/unreleased/accept-rf3-2822-compliant-addresses.yml diff --git a/app/workers/emails_on_push_worker.rb b/app/workers/emails_on_push_worker.rb index 8d0cfc73ccd..17ad1d5ab88 100644 --- a/app/workers/emails_on_push_worker.rb +++ b/app/workers/emails_on_push_worker.rb @@ -51,7 +51,7 @@ class EmailsOnPushWorker end end - recipients.split.each do |recipient| + valid_recipients(recipients).each do |recipient| begin send_email( recipient, @@ -89,4 +89,10 @@ class EmailsOnPushWorker email.header[:skip_premailer] = true if skip_premailer email.deliver_now end + + def valid_recipients(recipients) + recipients.split.select do |recipient| + recipient.include?('@') + end + end end diff --git a/changelogs/unreleased/accept-rf3-2822-compliant-addresses.yml b/changelogs/unreleased/accept-rf3-2822-compliant-addresses.yml new file mode 100644 index 00000000000..97d017613ba --- /dev/null +++ b/changelogs/unreleased/accept-rf3-2822-compliant-addresses.yml @@ -0,0 +1,5 @@ +--- +title: Emails on push recipients now accepts formats like John Doe +merge_request: +author: George Thomas +type: added diff --git a/doc/user/project/integrations/emails_on_push.md b/doc/user/project/integrations/emails_on_push.md index c01da883562..b050c83fb72 100644 --- a/doc/user/project/integrations/emails_on_push.md +++ b/doc/user/project/integrations/emails_on_push.md @@ -6,7 +6,7 @@ every change that is pushed to your project. Navigate to the [Integrations page](project_services.md#accessing-the-project-services) and select the **Emails on push** service to configure it. -In the _Recipients_ area, provide a list of emails separated by commas. +In the _Recipients_ area, provide a list of emails separated by spaces or newlines. You can configure any of the following settings depending on your preference. diff --git a/spec/workers/emails_on_push_worker_spec.rb b/spec/workers/emails_on_push_worker_spec.rb index 318aad4bc1e..f17c5ac6aac 100644 --- a/spec/workers/emails_on_push_worker_spec.rb +++ b/spec/workers/emails_on_push_worker_spec.rb @@ -100,10 +100,6 @@ describe EmailsOnPushWorker, :mailer do end context "when there are multiple recipients" do - let(:recipients) do - 1.upto(5).map { |i| user.email.sub('@', "+#{i}@") }.join("\n") - end - before do # This is a hack because we modify the mail object before sending, for efficency, # but the TestMailer adapter just appends the objects to an array. To clone a mail @@ -114,16 +110,57 @@ describe EmailsOnPushWorker, :mailer do end end - it "sends the mail to each of the recipients" do - perform - expect(ActionMailer::Base.deliveries.count).to eq(5) - expect(ActionMailer::Base.deliveries.map(&:to).flatten).to contain_exactly(*recipients.split) + context "when the recipient addresses are a list of email addresses" do + let(:recipients) do + 1.upto(5).map { |i| user.email.sub('@', "+#{i}@") }.join("\n") + end + + it "sends the mail to each of the recipients" do + perform + + expect(ActionMailer::Base.deliveries.count).to eq(5) + expect(email_recipients).to contain_exactly(*recipients.split) + end + + it "only generates the mail once" do + expect(Notify).to receive(:repository_push_email).once.and_call_original + expect(Premailer::Rails::CustomizedPremailer).to receive(:new).once.and_call_original + + perform + end end - it "only generates the mail once" do - expect(Notify).to receive(:repository_push_email).once.and_call_original - expect(Premailer::Rails::CustomizedPremailer).to receive(:new).once.and_call_original - perform + context "when the recipient addresses contains angle brackets and are separated by spaces" do + let(:recipients) { "John Doe Jane Doe " } + + it "accepts emails separated by whitespace" do + perform + + expect(ActionMailer::Base.deliveries.count).to eq(2) + expect(email_recipients).to contain_exactly("johndoe@example.com", "janedoe@example.com") + end + end + + context "when the recipient addresses contain a mix of emails with and without angle brackets" do + let(:recipients) { "johndoe@example.com Jane Doe " } + + it "accepts both kind of emails" do + perform + + expect(ActionMailer::Base.deliveries.count).to eq(2) + expect(email_recipients).to contain_exactly("johndoe@example.com", "janedoe@example.com") + end + end + + context "when the recipient addresses contains angle brackets and are separated by newlines" do + let(:recipients) { "John Doe \nJane Doe " } + + it "accepts emails separated by newlines" do + perform + + expect(ActionMailer::Base.deliveries.count).to eq(2) + expect(email_recipients).to contain_exactly("johndoe@example.com", "janedoe@example.com") + end end end end