Merge branch 'accept-rf3-2822-compliant-addresses' into 'master'repo-forks/gitlab-ce-frozen-string-enable-apps-services-inner-even-more

Improve email address parsing

Closes #44676

See merge request gitlab-org/gitlab-ce!18192

4 files changed, 62 insertions, 14 deletions

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 <johndoe@example.com>
+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 <johndoe@example.com> Jane Doe <janedoe@example.com>" }
+
+        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 <janedoe@example.com>" }
+
+        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 <johndoe@example.com>\nJane Doe <janedoe@example.com>" }
+
+        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