Commit a7905879 authored by Markus Koller's avatar Markus Koller

Merge branch '268207_validate_number_of_recipients_for_email_on_push_service' into 'master'

Cleanup and validate emails on push recipients

See merge request gitlab-org/gitlab!55550
parents 159ecc10 c743a380
......@@ -3,10 +3,19 @@
class EmailsOnPushService < Service
include NotificationBranchSelection
RECIPIENTS_LIMIT = 750
boolean_accessor :send_from_committer_email
boolean_accessor :disable_diffs
prop_accessor :recipients, :branches_to_be_notified
validates :recipients, presence: true, if: :valid_recipients?
validates :recipients, presence: true, if: :validate_recipients?
validate :number_of_recipients_within_limit, if: :validate_recipients?
def self.valid_recipients(recipients)
recipients.split.select do |recipient|
recipient.include?('@')
end.uniq(&:downcase)
end
def title
s_('EmailsOnPushService|Emails on push')
......@@ -67,7 +76,22 @@ class EmailsOnPushService < Service
{ type: 'checkbox', name: 'disable_diffs', title: s_("EmailsOnPushService|Disable code diffs"),
help: s_("EmailsOnPushService|Don't include possibly sensitive code diffs in notification body.") },
{ type: 'select', name: 'branches_to_be_notified', choices: branch_choices },
{ type: 'textarea', name: 'recipients', placeholder: s_('EmailsOnPushService|Emails separated by whitespace') }
{
type: 'textarea',
name: 'recipients',
placeholder: s_('EmailsOnPushService|tanuki@example.com gitlab@example.com'),
help: s_('EmailsOnPushService|Emails separated by whitespace.')
}
]
end
private
def number_of_recipients_within_limit
return if recipients.blank?
if self.class.valid_recipients(recipients).size > RECIPIENTS_LIMIT
errors.add(:recipients, s_("EmailsOnPushService|can't exceed %{recipients_limit}") % { recipients_limit: RECIPIENTS_LIMIT })
end
end
end
......@@ -6,7 +6,7 @@ class IrkerService < Service
prop_accessor :server_host, :server_port, :default_irc_uri
prop_accessor :recipients, :channels
boolean_accessor :colorize_messages
validates :recipients, presence: true, if: :valid_recipients?
validates :recipients, presence: true, if: :validate_recipients?
before_validation :get_channels
......
......@@ -5,7 +5,7 @@ class PipelinesEmailService < Service
prop_accessor :recipients, :branches_to_be_notified
boolean_accessor :notify_only_broken_pipelines, :notify_only_default_branch
validates :recipients, presence: true, if: :valid_recipients?
validates :recipients, presence: true, if: :validate_recipients?
def initialize_properties
if properties.nil?
......
......@@ -460,7 +460,7 @@ class Service < ApplicationRecord
errors.add(:project_id, 'The service cannot belong to both a project and a group') if project_id && group_id
end
def valid_recipients?
def validate_recipients?
activated? && !importing?
end
end
......
......@@ -56,7 +56,7 @@ class EmailsOnPushWorker # rubocop:disable Scalability/IdempotentWorker
end
end
valid_recipients(recipients).each do |recipient|
EmailsOnPushService.valid_recipients(recipients).each do |recipient|
send_email(
recipient,
project_id,
......@@ -92,10 +92,4 @@ class EmailsOnPushWorker # rubocop:disable Scalability/IdempotentWorker
email.header[:skip_premailer] = true if skip_premailer
email.deliver_now
end
def valid_recipients(recipients)
recipients.split.select do |recipient|
recipient.include?('@')
end.uniq(&:downcase)
end
end
---
title: Add validation for emails on push recipients
merge_request: 55550
author:
type: changed
......@@ -11544,7 +11544,7 @@ msgstr ""
msgid "EmailsOnPushService|Emails on push"
msgstr ""
msgid "EmailsOnPushService|Emails separated by whitespace"
msgid "EmailsOnPushService|Emails separated by whitespace."
msgstr ""
msgid "EmailsOnPushService|Send from committer"
......@@ -11553,6 +11553,12 @@ msgstr ""
msgid "EmailsOnPushService|Send notifications from the committer's email address if the domain is part of the domain GitLab is running on (e.g. %{domains})."
msgstr ""
msgid "EmailsOnPushService|can't exceed %{recipients_limit}"
msgstr ""
msgid "EmailsOnPushService|tanuki@example.com gitlab@example.com"
msgstr ""
msgid "Embed"
msgstr ""
......
......@@ -3,6 +3,8 @@
require 'spec_helper'
RSpec.describe EmailsOnPushService do
let_it_be(:project) { create_default(:project).freeze }
describe 'Validations' do
context 'when service is active' do
before do
......@@ -19,6 +21,42 @@ RSpec.describe EmailsOnPushService do
it { is_expected.not_to validate_presence_of(:recipients) }
end
describe 'validates number of recipients' do
before do
stub_const("#{described_class}::RECIPIENTS_LIMIT", 2)
end
subject(:service) { described_class.new(project: project, recipients: recipients, active: true) }
context 'valid number of recipients' do
let(:recipients) { 'foo@bar.com duplicate@example.com Duplicate@example.com invalid-email' }
it 'does not count duplicates and invalid emails' do
is_expected.to be_valid
end
end
context 'invalid number of recipients' do
let(:recipients) { 'foo@bar.com bar@foo.com bob@gitlab.com' }
it { is_expected.not_to be_valid }
it 'adds an error message' do
service.valid?
expect(service.errors).to contain_exactly('Recipients can\'t exceed 2')
end
context 'when service is not active' do
before do
service.active = false
end
it { is_expected.to be_valid }
end
end
end
end
describe '.new' do
......@@ -39,6 +77,14 @@ RSpec.describe EmailsOnPushService do
end
end
describe '.valid_recipients' do
let(:recipients) { '<invalid> foobar Valid@recipient.com Dup@lica.te dup@lica.te Dup@Lica.te' }
it 'removes invalid email addresses and removes duplicates by keeping the original capitalization' do
expect(described_class.valid_recipients(recipients)).to contain_exactly('Valid@recipient.com', 'Dup@lica.te')
end
end
describe '#execute' do
let(:push_data) { { object_kind: 'push' } }
let(:project) { create(:project, :repository) }
......
......@@ -28,6 +28,8 @@ Service.available_services_names.each do |service|
hash.merge!(k => 1234)
elsif service == 'jira' && k == :jira_issue_transition_id
hash.merge!(k => '1,2,3')
elsif service == 'emails_on_push' && k == :recipients
hash.merge!(k => 'foo@bar.com')
else
hash.merge!(k => "someword")
end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment