From eb287111e2148be7bf7c493b3392eef7113e8675 Mon Sep 17 00:00:00 2001 From: Luke Duncalfe Date: Tue, 13 Dec 2022 12:09:29 +1300 Subject: [PATCH 1/4] Validate notification integrations channel limit This change adds a new validation to our Slack and Mattermost notification integrations that each event trigger is configured for at most 10 unique channels. It also adds a tweak so we send to a unique list of channels, to avoid the ability for the integrations to post the same thing to the same channel. Changelog: changed --- .../integrations/base_chat_notification.rb | 21 +++++++++++-- locale/gitlab.pot | 3 ++ .../base_chat_notification_spec.rb | 30 +++++++++++++++++-- 3 files changed, 49 insertions(+), 5 deletions(-) diff --git a/app/models/integrations/base_chat_notification.rb b/app/models/integrations/base_chat_notification.rb index fa5ce098379c48..6664a7278cb4c0 100644 --- a/app/models/integrations/base_chat_notification.rb +++ b/app/models/integrations/base_chat_notification.rb @@ -23,6 +23,7 @@ class BaseChatNotification < Integration ].freeze SECRET_MASK = '************' + CHANNEL_LIMIT_PER_EVENT = 10 attribute :category, default: 'chat' @@ -37,7 +38,8 @@ class BaseChatNotification < Integration presence: true, public_url: true, if: -> (integration) { integration.activated? && integration.requires_webhook? } - validates :labels_to_be_notified_behavior, inclusion: { in: LABEL_NOTIFICATION_BEHAVIOURS }, allow_blank: true + validates :labels_to_be_notified_behavior, inclusion: { in: LABEL_NOTIFICATION_BEHAVIOURS }, allow_blank: true, if: :activated? + validate :validate_channel_limit, if: :activated? def initialize_properties super @@ -300,7 +302,7 @@ def channels_for_event(event) channel_names = event_channel_value(event).presence || channel.presence return [] unless channel_names - channel_names.split(',').map(&:strip) + channel_names.split(',').map(&:strip).uniq end def unique_channels @@ -308,6 +310,21 @@ def unique_channels channels_for_event(event) end.uniq end + + def validate_channel_limit + supported_events.each do |event| + count = channels_for_event(event).count + next unless count > CHANNEL_LIMIT_PER_EVENT + + errors.add( + event_channel_name(event).to_sym, + format( + s_('SlackIntegration|is limited to %{limit} channels'), + limit: CHANNEL_LIMIT_PER_EVENT + ) + ) + end + end end end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 13ae7fc419eeaf..b5b3333aecbe1f 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -40071,6 +40071,9 @@ msgstr "" msgid "SlackIntegration|You may need to reinstall the GitLab for Slack app when we %{linkStart}make updates or change permissions%{linkEnd}." msgstr "" +msgid "SlackIntegration|is limited to %{limit} channels" +msgstr "" + msgid "SlackModal|Are you sure you want to change the project?" msgstr "" diff --git a/spec/models/integrations/base_chat_notification_spec.rb b/spec/models/integrations/base_chat_notification_spec.rb index 1527ffd7278395..13dd9e03ab1182 100644 --- a/spec/models/integrations/base_chat_notification_spec.rb +++ b/spec/models/integrations/base_chat_notification_spec.rb @@ -9,13 +9,33 @@ describe 'validations' do before do - allow(subject).to receive(:activated?).and_return(true) + subject.active = active + allow(subject).to receive(:default_channel_placeholder).and_return('placeholder') allow(subject).to receive(:webhook_help).and_return('help') end - it { is_expected.to validate_presence_of :webhook } - it { is_expected.to validate_inclusion_of(:labels_to_be_notified_behavior).in_array(%w[match_any match_all]).allow_blank } + def build_channel_list(count) + (1..count).map { |i| "##{i}" }.join(',') + end + + context 'when active' do + let(:active) { true } + + it { is_expected.to validate_presence_of :webhook } + it { is_expected.to validate_inclusion_of(:labels_to_be_notified_behavior).in_array(%w[match_any match_all]).allow_blank } + it { is_expected.to allow_value(build_channel_list(10)).for(:push_channel) } + it { is_expected.not_to allow_value(build_channel_list(11)).for(:push_channel) } + end + + context 'when inactive' do + let(:active) { false } + + it { is_expected.not_to validate_presence_of :webhook } + it { is_expected.not_to validate_inclusion_of(:labels_to_be_notified_behavior).in_array(%w[match_any match_all]).allow_blank } + it { is_expected.to allow_value(build_channel_list(10)).for(:push_channel) } + it { is_expected.to allow_value(build_channel_list(11)).for(:push_channel) } + end end describe '#execute' do @@ -309,6 +329,10 @@ context 'with multiple channel names with spaces specified' do it_behaves_like 'with channel specified', 'slack-integration, #slack-test, @UDLP91W0A', ['slack-integration', '#slack-test', '@UDLP91W0A'] end + + context 'with duplicate channel names' do + it_behaves_like 'with channel specified', '#slack-test,#slack-test,#slack-test-2', ['#slack-test', '#slack-test-2'] + end end describe '#default_channel_placeholder' do -- GitLab From 7d36ab70f7ceff431b006b2cf3a14eb07c8847b4 Mon Sep 17 00:00:00 2001 From: Luke Duncalfe Date: Mon, 23 Jan 2023 12:45:30 +1300 Subject: [PATCH 2/4] Update integration docs --- doc/user/project/integrations/mattermost.md | 2 ++ doc/user/project/integrations/slack.md | 2 ++ 2 files changed, 4 insertions(+) diff --git a/doc/user/project/integrations/mattermost.md b/doc/user/project/integrations/mattermost.md index 39b89cd87a9946..d7d0a55889b0e0 100644 --- a/doc/user/project/integrations/mattermost.md +++ b/doc/user/project/integrations/mattermost.md @@ -36,6 +36,8 @@ Display name override is not enabled by default, you need to ask your administra ## Configure GitLab to send notifications to Mattermost +> [Changed](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/106760) in GitLab 15.9, Mattermost channels are limited to 10 per event. + After the Mattermost instance has an incoming webhook set up, you can set up GitLab to send the notifications: diff --git a/doc/user/project/integrations/slack.md b/doc/user/project/integrations/slack.md index 9932e782ff41b5..524d7ac179e61c 100644 --- a/doc/user/project/integrations/slack.md +++ b/doc/user/project/integrations/slack.md @@ -28,6 +28,8 @@ to control GitLab from Slack. Slash commands are configured separately. ## Configure GitLab +> [Changed](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/106760) in GitLab 15.9, Slack channels are limited to 10 per event. + 1. On the top bar, select **Main menu > Projects** and find your project. 1. On the left sidebar, select **Settings > Integrations**. 1. Select **Slack notifications**. -- GitLab From d9140b13c574d29a36b93843ec1492005953edda Mon Sep 17 00:00:00 2001 From: Luke Duncalfe Date: Tue, 7 Feb 2023 11:07:02 +1300 Subject: [PATCH 3/4] Apply reviewer feedback --- app/models/integrations/base_chat_notification.rb | 2 +- locale/gitlab.pot | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/models/integrations/base_chat_notification.rb b/app/models/integrations/base_chat_notification.rb index 6664a7278cb4c0..963ba918089024 100644 --- a/app/models/integrations/base_chat_notification.rb +++ b/app/models/integrations/base_chat_notification.rb @@ -319,7 +319,7 @@ def validate_channel_limit errors.add( event_channel_name(event).to_sym, format( - s_('SlackIntegration|is limited to %{limit} channels'), + s_('SlackIntegration|cannot have more than %{limit} channels'), limit: CHANNEL_LIMIT_PER_EVENT ) ) diff --git a/locale/gitlab.pot b/locale/gitlab.pot index b5b3333aecbe1f..570e19285621ee 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -40071,7 +40071,7 @@ msgstr "" msgid "SlackIntegration|You may need to reinstall the GitLab for Slack app when we %{linkStart}make updates or change permissions%{linkEnd}." msgstr "" -msgid "SlackIntegration|is limited to %{limit} channels" +msgid "SlackIntegration|cannot have more than %{limit} channels" msgstr "" msgid "SlackModal|Are you sure you want to change the project?" -- GitLab From 03d412ed9e6ca8a47c74d0ee761bbb6e37ec6f61 Mon Sep 17 00:00:00 2001 From: Ashraf Khamis Date: Wed, 25 Jan 2023 01:14:05 +0000 Subject: [PATCH 4/4] Apply 2 suggestion(s) to 2 file(s) --- doc/user/project/integrations/mattermost.md | 2 +- doc/user/project/integrations/slack.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/doc/user/project/integrations/mattermost.md b/doc/user/project/integrations/mattermost.md index d7d0a55889b0e0..3782d3acd0cb35 100644 --- a/doc/user/project/integrations/mattermost.md +++ b/doc/user/project/integrations/mattermost.md @@ -36,7 +36,7 @@ Display name override is not enabled by default, you need to ask your administra ## Configure GitLab to send notifications to Mattermost -> [Changed](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/106760) in GitLab 15.9, Mattermost channels are limited to 10 per event. +> [Changed](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/106760) in GitLab 15.9 to limit Mattermost channels to 10 per event. After the Mattermost instance has an incoming webhook set up, you can set up GitLab to send the notifications: diff --git a/doc/user/project/integrations/slack.md b/doc/user/project/integrations/slack.md index 524d7ac179e61c..d14401a5c9dedc 100644 --- a/doc/user/project/integrations/slack.md +++ b/doc/user/project/integrations/slack.md @@ -28,7 +28,7 @@ to control GitLab from Slack. Slash commands are configured separately. ## Configure GitLab -> [Changed](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/106760) in GitLab 15.9, Slack channels are limited to 10 per event. +> [Changed](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/106760) in GitLab 15.9 to limit Slack channels to 10 per event. 1. On the top bar, select **Main menu > Projects** and find your project. 1. On the left sidebar, select **Settings > Integrations**. -- GitLab