diff options
author | Brett Walker <bwalker@gitlab.com> | 2019-08-15 17:37:36 +0000 |
---|---|---|
committer | Douglas Barbosa Alexandre <dbalexandre@gmail.com> | 2019-08-15 17:37:36 +0000 |
commit | 3489dc3d7277bf478f68e1b3e0353b702f652acc (patch) | |
tree | 896dc6ef08d8347e992365594ce7c4b0d49e6ee4 | |
parent | 23754943a7ec119f123694a93c79fc07c32b7ba5 (diff) | |
download | gitlab-ce-3489dc3d7277bf478f68e1b3e0353b702f652acc.tar.gz |
Allow disabling group/project email notifications
- Adds UI to configure in group and project settings
- Removes notification configuration for users when
disabled at group or project level
26 files changed, 601 insertions, 73 deletions
diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb index 5472ef05d7c..886d1f99d69 100644 --- a/app/controllers/groups_controller.rb +++ b/app/controllers/groups_controller.rb @@ -176,6 +176,7 @@ class GroupsController < Groups::ApplicationController [ :avatar, :description, + :emails_disabled, :lfs_enabled, :name, :path, diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index d4ff72c2314..e04cbf10470 100644 --- a/app/controllers/projects_controller.rb +++ b/app/controllers/projects_controller.rb @@ -361,6 +361,7 @@ class ProjectsController < Projects::ApplicationController :container_registry_enabled, :default_branch, :description, + :emails_disabled, :external_authorization_classification_label, :import_url, :issues_tracker, diff --git a/app/models/namespace.rb b/app/models/namespace.rb index 058350b16ce..9f9c4288667 100644 --- a/app/models/namespace.rb +++ b/app/models/namespace.rb @@ -172,6 +172,13 @@ class Namespace < ApplicationRecord end end + # any ancestor can disable emails for all descendants + def emails_disabled? + strong_memoize(:emails_disabled) do + Feature.enabled?(:emails_disabled, self, default_enabled: true) && self_and_ancestors.where(emails_disabled: true).exists? + end + end + def lfs_enabled? # User namespace will always default to the global setting Gitlab.config.lfs.enabled diff --git a/app/models/notification_recipient.rb b/app/models/notification_recipient.rb index a7f73c0f29c..8e44e3d8e17 100644 --- a/app/models/notification_recipient.rb +++ b/app/models/notification_recipient.rb @@ -4,6 +4,7 @@ class NotificationRecipient include Gitlab::Utils::StrongMemoize attr_reader :user, :type, :reason + def initialize(user, type, **opts) unless NotificationSetting.levels.key?(type) || type == :subscription raise ArgumentError, "invalid type: #{type.inspect}" @@ -30,6 +31,7 @@ class NotificationRecipient def notifiable? return false unless has_access? + return false if emails_disabled? return false if own_activity? # even users with :disabled notifications receive manual subscriptions @@ -109,6 +111,12 @@ class NotificationRecipient private + # They are disabled if the project or group has disallowed it. + # No need to check the group if there is already a project + def emails_disabled? + @project ? @project.emails_disabled? : @group&.emails_disabled? + end + def read_ability return if @skip_read_ability return @read_ability if instance_variable_defined?(:@read_ability) diff --git a/app/models/project.rb b/app/models/project.rb index 0c57ed3e43a..820a8082b27 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -631,6 +631,13 @@ class Project < ApplicationRecord alias_method :ancestors, :ancestors_upto + def emails_disabled? + strong_memoize(:emails_disabled) do + # disabling in the namespace overrides the project setting + Feature.enabled?(:emails_disabled, self, default_enabled: true) && (super || namespace.emails_disabled?) + end + end + def lfs_enabled? return namespace.lfs_enabled? if self[:lfs_enabled].nil? diff --git a/app/models/project_services/emails_on_push_service.rb b/app/models/project_services/emails_on_push_service.rb index 45de64a9990..8ca40138a8f 100644 --- a/app/models/project_services/emails_on_push_service.rb +++ b/app/models/project_services/emails_on_push_service.rb @@ -24,6 +24,7 @@ class EmailsOnPushService < Service def execute(push_data) return unless supported_events.include?(push_data[:object_kind]) + return if project.emails_disabled? EmailsOnPushWorker.perform_async( project_id, diff --git a/app/policies/group_policy.rb b/app/policies/group_policy.rb index 52c944491bf..c686e7763bb 100644 --- a/app/policies/group_policy.rb +++ b/app/policies/group_policy.rb @@ -92,6 +92,7 @@ class GroupPolicy < BasePolicy enable :change_visibility_level enable :set_note_created_at + enable :set_emails_disabled end rule { can?(:read_nested_project_resources) }.policy do diff --git a/app/policies/project_policy.rb b/app/policies/project_policy.rb index e79bac6bee3..b8dee1b0789 100644 --- a/app/policies/project_policy.rb +++ b/app/policies/project_policy.rb @@ -162,6 +162,7 @@ class ProjectPolicy < BasePolicy enable :set_issue_created_at enable :set_issue_updated_at enable :set_note_created_at + enable :set_emails_disabled end rule { can?(:guest_access) }.policy do diff --git a/app/services/groups/update_service.rb b/app/services/groups/update_service.rb index 73e1e00dc33..116756bacfe 100644 --- a/app/services/groups/update_service.rb +++ b/app/services/groups/update_service.rb @@ -46,6 +46,11 @@ module Groups params.delete(:parent_id) end + # overridden in EE + def remove_unallowed_params + params.delete(:emails_disabled) unless can?(current_user, :set_emails_disabled, group) + end + def valid_share_with_group_lock_change? return true unless changing_share_with_group_lock? return true if can?(current_user, :change_share_with_group_lock, group) diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb index 21fab22e0d4..83710ffce2f 100644 --- a/app/services/notification_service.rb +++ b/app/services/notification_service.rb @@ -321,6 +321,9 @@ class NotificationService end def decline_project_invite(project_member) + # Must always send, regardless of project/namespace configuration since it's a + # response to the user's action. + mailer.member_invite_declined_email( project_member.real_source_type, project_member.project.id, @@ -351,8 +354,8 @@ class NotificationService end def decline_group_invite(group_member) - # always send this one, since it's a response to the user's own - # action + # Must always send, regardless of project/namespace configuration since it's a + # response to the user's action. mailer.member_invite_declined_email( group_member.real_source_type, @@ -410,6 +413,10 @@ class NotificationService end def pipeline_finished(pipeline, recipients = nil) + # Must always check project configuration since recipients could be a list of emails + # from the PipelinesEmailService integration. + return if pipeline.project.emails_disabled? + email_template = "pipeline_#{pipeline.status}_email" return unless mailer.respond_to?(email_template) @@ -428,6 +435,8 @@ class NotificationService end def autodevops_disabled(pipeline, recipients) + return if pipeline.project.emails_disabled? + recipients.each do |recipient| mailer.autodevops_disabled_email(pipeline, recipient).deliver_later end @@ -472,10 +481,14 @@ class NotificationService end def repository_cleanup_success(project, user) + return if project.emails_disabled? + mailer.send(:repository_cleanup_success_email, project, user).deliver_later end def repository_cleanup_failure(project, user, error) + return if project.emails_disabled? + mailer.send(:repository_cleanup_failure_email, project, user, error).deliver_later end diff --git a/app/services/projects/update_service.rb b/app/services/projects/update_service.rb index caab946174d..8acbdc7e02b 100644 --- a/app/services/projects/update_service.rb +++ b/app/services/projects/update_service.rb @@ -9,6 +9,7 @@ module Projects # rubocop: disable CodeReuse/ActiveRecord def execute + remove_unallowed_params validate! ensure_wiki_exists if enabling_wiki? @@ -54,6 +55,10 @@ module Projects end end + def remove_unallowed_params + params.delete(:emails_disabled) unless can?(current_user, :set_emails_disabled, project) + end + def after_update todos_features_changes = %w( issues_access_level diff --git a/changelogs/unreleased/50020-allow-email-notifications-to-be-disabled-for-all-users-of-a-group.yml b/changelogs/unreleased/50020-allow-email-notifications-to-be-disabled-for-all-users-of-a-group.yml new file mode 100644 index 00000000000..9137e9339aa --- /dev/null +++ b/changelogs/unreleased/50020-allow-email-notifications-to-be-disabled-for-all-users-of-a-group.yml @@ -0,0 +1,5 @@ +--- +title: Allow email notifications to be disabled for all members of a group or project +merge_request: 30755 +author: Dustin Spicuzza +type: added diff --git a/db/migrate/20190715215532_add_project_emails_disabled.rb b/db/migrate/20190715215532_add_project_emails_disabled.rb new file mode 100644 index 00000000000..536ea34c0fb --- /dev/null +++ b/db/migrate/20190715215532_add_project_emails_disabled.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +class AddProjectEmailsDisabled < ActiveRecord::Migration[5.2] + DOWNTIME = false + + def change + add_column :projects, :emails_disabled, :boolean + end +end diff --git a/db/migrate/20190715215549_add_group_emails_disabled.rb b/db/migrate/20190715215549_add_group_emails_disabled.rb new file mode 100644 index 00000000000..d3fd4d2d923 --- /dev/null +++ b/db/migrate/20190715215549_add_group_emails_disabled.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +class AddGroupEmailsDisabled < ActiveRecord::Migration[5.2] + DOWNTIME = false + + def change + add_column :namespaces, :emails_disabled, :boolean + end +end diff --git a/db/schema.rb b/db/schema.rb index 7c4a91da750..0f8c3cae689 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -2175,6 +2175,7 @@ ActiveRecord::Schema.define(version: 2019_08_12_070645) do t.boolean "membership_lock", default: false t.integer "last_ci_minutes_usage_notification_level" t.integer "subgroup_creation_level", default: 1 + t.boolean "emails_disabled" t.index ["created_at"], name: "index_namespaces_on_created_at" t.index ["custom_project_templates_group_id", "type"], name: "index_namespaces_on_custom_project_templates_group_id_and_type", where: "(custom_project_templates_group_id IS NOT NULL)" t.index ["file_template_project_id"], name: "index_namespaces_on_file_template_project_id" @@ -2745,6 +2746,7 @@ ActiveRecord::Schema.define(version: 2019_08_12_070645) do t.boolean "reset_approvals_on_push", default: true t.boolean "service_desk_enabled", default: true t.integer "approvals_before_merge", default: 0, null: false + t.boolean "emails_disabled" t.index ["archived", "pending_delete", "merge_requests_require_code_owner_approval"], name: "projects_requiring_code_owner_approval", where: "((pending_delete = false) AND (archived = false) AND (merge_requests_require_code_owner_approval = true))" t.index ["created_at"], name: "index_projects_on_created_at" t.index ["creator_id"], name: "index_projects_on_creator_id" diff --git a/lib/gitlab/import_export/import_export.yml b/lib/gitlab/import_export/import_export.yml index 1b7fc5fa10f..bd0f3e70749 100644 --- a/lib/gitlab/import_export/import_export.yml +++ b/lib/gitlab/import_export/import_export.yml @@ -137,6 +137,7 @@ excluded_attributes: - :packages_enabled - :mirror_last_update_at - :mirror_last_successful_update_at + - :emails_disabled namespaces: - :runners_token - :runners_token_encrypted diff --git a/spec/factories/services.rb b/spec/factories/services.rb index f3e662ad4f5..b2d6ada91fa 100644 --- a/spec/factories/services.rb +++ b/spec/factories/services.rb @@ -16,6 +16,19 @@ FactoryBot.define do ) end + factory :emails_on_push_service do + project + type 'EmailsOnPushService' + active true + push_events true + tag_push_events true + properties( + recipients: 'test@example.com', + disable_diffs: true, + send_from_committer_email: true + ) + end + factory :mock_deployment_service do project type 'MockDeploymentService' diff --git a/spec/models/namespace_spec.rb b/spec/models/namespace_spec.rb index 2b9c3c43af9..972f26ac745 100644 --- a/spec/models/namespace_spec.rb +++ b/spec/models/namespace_spec.rb @@ -853,4 +853,64 @@ describe Namespace do it { is_expected.to be_falsy } end end + + describe '#emails_disabled?' do + context 'when not a subgroup' do + it 'returns false' do + group = create(:group, emails_disabled: false) + + expect(group.emails_disabled?).to be_falsey + end + + it 'returns true' do + group = create(:group, emails_disabled: true) + + expect(group.emails_disabled?).to be_truthy + end + end + + context 'when a subgroup' do + let(:grandparent) { create(:group) } + let(:parent) { create(:group, parent: grandparent) } + let(:group) { create(:group, parent: parent) } + + it 'returns false' do + expect(group.emails_disabled?).to be_falsey + end + + context 'when ancestor emails are disabled' do + it 'returns true' do + grandparent.update_attribute(:emails_disabled, true) + + expect(group.emails_disabled?).to be_truthy + end + end + end + + context 'when :emails_disabled feature flag is off' do + before do + stub_feature_flags(emails_disabled: false) + end + + context 'when not a subgroup' do + it 'returns false' do + group = create(:group, emails_disabled: true) + + expect(group.emails_disabled?).to be_falsey + end + end + + context 'when a subgroup and ancestor emails are disabled' do + let(:grandparent) { create(:group) } + let(:parent) { create(:group, parent: grandparent) } + let(:group) { create(:group, parent: parent) } + + it 'returns false' do + grandparent.update_attribute(:emails_disabled, true) + + expect(group.emails_disabled?).to be_falsey + end + end + end + end end diff --git a/spec/models/notification_recipient_spec.rb b/spec/models/notification_recipient_spec.rb index 4122736c148..2ba53818e54 100644 --- a/spec/models/notification_recipient_spec.rb +++ b/spec/models/notification_recipient_spec.rb @@ -9,6 +9,38 @@ describe NotificationRecipient do subject(:recipient) { described_class.new(user, :watch, target: target, project: project) } + describe '#notifiable?' do + let(:recipient) { described_class.new(user, :mention, target: target, project: project) } + + context 'when emails are disabled' do + it 'returns false if group disabled' do + expect(project.namespace).to receive(:emails_disabled?).and_return(true) + expect(recipient).to receive(:emails_disabled?).and_call_original + expect(recipient.notifiable?).to eq false + end + + it 'returns false if project disabled' do + expect(project).to receive(:emails_disabled?).and_return(true) + expect(recipient).to receive(:emails_disabled?).and_call_original + expect(recipient.notifiable?).to eq false + end + end + + context 'when emails are enabled' do + it 'returns true if group enabled' do + expect(project.namespace).to receive(:emails_disabled?).and_return(false) + expect(recipient).to receive(:emails_disabled?).and_call_original + expect(recipient.notifiable?).to eq true + end + + it 'returns true if project enabled' do + expect(project).to receive(:emails_disabled?).and_return(false) + expect(recipient).to receive(:emails_disabled?).and_call_original + expect(recipient.notifiable?).to eq true + end + end + end + describe '#has_access?' do before do allow(user).to receive(:can?).and_call_original diff --git a/spec/models/project_services/emails_on_push_service_spec.rb b/spec/models/project_services/emails_on_push_service_spec.rb index 0a58eb367e3..ffe241aa880 100644 --- a/spec/models/project_services/emails_on_push_service_spec.rb +++ b/spec/models/project_services/emails_on_push_service_spec.rb @@ -20,4 +20,24 @@ describe EmailsOnPushService do it { is_expected.not_to validate_presence_of(:recipients) } end end + + context 'project emails' do + let(:push_data) { { object_kind: 'push' } } + let(:project) { create(:project, :repository) } + let(:service) { create(:emails_on_push_service, project: project) } + + it 'does not send emails when disabled' do + expect(project).to receive(:emails_disabled?).and_return(true) + expect(EmailsOnPushWorker).not_to receive(:perform_async) + + service.execute(push_data) + end + + it 'does send emails when enabled' do + expect(project).to receive(:emails_disabled?).and_return(false) + expect(EmailsOnPushWorker).to receive(:perform_async) + + service.execute(push_data) + end + end end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 2afe1253e29..b4edbd97d20 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -2315,6 +2315,57 @@ describe Project do end end + describe '#emails_disabled?' do + let(:project) { create(:project, emails_disabled: false) } + + context 'emails disabled in group' do + it 'returns true' do + allow(project.namespace).to receive(:emails_disabled?) { true } + + expect(project.emails_disabled?).to be_truthy + end + end + + context 'emails enabled in group' do + before do + allow(project.namespace).to receive(:emails_disabled?) { false } + end + + it 'returns false' do + expect(project.emails_disabled?).to be_falsey + end + + it 'returns true' do + project.update_attribute(:emails_disabled, true) + + expect(project.emails_disabled?).to be_truthy + end + end + + context 'when :emails_disabled feature flag is off' do + before do + stub_feature_flags(emails_disabled: false) + end + + context 'emails disabled in group' do + it 'returns false' do + allow(project.namespace).to receive(:emails_disabled?) { true } + + expect(project.emails_disabled?).to be_falsey + end + end + + context 'emails enabled in group' do + it 'returns false' do + allow(project.namespace).to receive(:emails_disabled?) { false } + project.update_attribute(:emails_disabled, true) + + expect(project.emails_disabled?).to be_falsey + end + end + end + end + describe '#lfs_enabled?' do let(:project) { create(:project) } diff --git a/spec/services/groups/update_service_spec.rb b/spec/services/groups/update_service_spec.rb index 5d4576139f7..12e9c2b2f3a 100644 --- a/spec/services/groups/update_service_spec.rb +++ b/spec/services/groups/update_service_spec.rb @@ -86,6 +86,7 @@ describe Groups::UpdateService do context "unauthorized visibility_level validation" do let!(:service) { described_class.new(internal_group, user, visibility_level: 99) } + before do internal_group.add_user(user, Gitlab::Access::MAINTAINER) end @@ -96,6 +97,20 @@ describe Groups::UpdateService do end end + context 'when updating #emails_disabled' do + let(:service) { described_class.new(internal_group, user, emails_disabled: true) } + + it 'updates the attribute' do + internal_group.add_user(user, Gitlab::Access::OWNER) + + expect { service.execute }.to change { internal_group.emails_disabled }.to(true) + end + + it 'does not update when not group owner' do + expect { service.execute }.not_to change { internal_group.emails_disabled } + end + end + context 'rename group' do let!(:service) { described_class.new(internal_group, user, path: SecureRandom.hex) } diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index 1dcade1de0d..d925aa2b6c3 100644 --- a/spec/services/notification_service_spec.rb +++ b/spec/services/notification_service_spec.rb @@ -240,45 +240,50 @@ describe NotificationService, :mailer do end describe '#new_note' do - it do - add_users(project) - add_user_subscriptions(issue) - reset_delivered_emails! + context do + before do + add_users(project) + add_user_subscriptions(issue) + reset_delivered_emails! + end - expect(SentNotification).to receive(:record).with(issue, any_args).exactly(10).times + it do + expect(SentNotification).to receive(:record).with(issue, any_args).exactly(10).times - notification.new_note(note) + notification.new_note(note) - should_email(@u_watcher) - should_email(note.noteable.author) - should_email(note.noteable.assignees.first) - should_email(@u_custom_global) - should_email(@u_mentioned) - should_email(@subscriber) - should_email(@watcher_and_subscriber) - should_email(@subscribed_participant) - should_email(@u_custom_off) - should_email(@unsubscribed_mentioned) - should_not_email(@u_guest_custom) - should_not_email(@u_guest_watcher) - should_not_email(note.author) - should_not_email(@u_participating) - should_not_email(@u_disabled) - should_not_email(@unsubscriber) - should_not_email(@u_outsider_mentioned) - should_not_email(@u_lazy_participant) - end + should_email(@u_watcher) + should_email(note.noteable.author) + should_email(note.noteable.assignees.first) + should_email(@u_custom_global) + should_email(@u_mentioned) + should_email(@subscriber) + should_email(@watcher_and_subscriber) + should_email(@subscribed_participant) + should_email(@u_custom_off) + should_email(@unsubscribed_mentioned) + should_not_email(@u_guest_custom) + should_not_email(@u_guest_watcher) + should_not_email(note.author) + should_not_email(@u_participating) + should_not_email(@u_disabled) + should_not_email(@unsubscriber) + should_not_email(@u_outsider_mentioned) + should_not_email(@u_lazy_participant) + end - it "emails the note author if they've opted into notifications about their activity" do - add_users(project) - add_user_subscriptions(issue) - reset_delivered_emails! + it "emails the note author if they've opted into notifications about their activity" do + note.author.notified_of_own_activity = true - note.author.notified_of_own_activity = true + notification.new_note(note) - notification.new_note(note) + should_email(note.author) + end - should_email(note.author) + it_behaves_like 'project emails are disabled' do + let(:notification_target) { note } + let(:notification_trigger) { notification.new_note(note) } + end end it 'filters out "mentioned in" notes' do @@ -337,6 +342,11 @@ describe NotificationService, :mailer do it_behaves_like 'new note notifications' + it_behaves_like 'project emails are disabled' do + let(:notification_target) { note } + let(:notification_trigger) { notification.new_note(note) } + end + context 'which is a subgroup' do let!(:parent) { create(:group) } let!(:group) { create(:group, parent: parent) } @@ -472,6 +482,11 @@ describe NotificationService, :mailer do expect(Notify).not_to receive(:note_issue_email) notification.new_note(mentioned_note) end + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { note } + let(:notification_trigger) { notification.new_note(note) } + end end end @@ -619,6 +634,11 @@ describe NotificationService, :mailer do notification.new_note(note) should_not_email(@u_committer) end + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { note } + let(:notification_trigger) { notification.new_note(note) } + end end end @@ -645,6 +665,11 @@ describe NotificationService, :mailer do .to contain_exactly(*merge_request.assignees.pluck(:id), merge_request.author.id, @u_watcher.id) expect(SentNotification.last.in_reply_to_discussion_id).to eq(note.discussion_id) end + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { note } + let(:notification_trigger) { notification.new_note(note) } + end end end end @@ -819,6 +844,11 @@ describe NotificationService, :mailer do should_email(user_4) end + it_behaves_like 'project emails are disabled' do + let(:notification_target) { issue } + let(:notification_trigger) { notification.new_issue(issue, @u_disabled) } + end + context 'confidential issues' do let(:author) { create(:user) } let(:assignee) { create(:user) } @@ -861,6 +891,11 @@ describe NotificationService, :mailer do let(:mentionable) { issue } include_examples 'notifications for new mentions' + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { issue } + let(:notification_trigger) { send_notifications(@u_watcher, @u_participant_mentioned, @u_custom_global, @u_mentioned) } + end end describe '#reassigned_issue' do @@ -969,6 +1004,11 @@ describe NotificationService, :mailer do let(:issuable) { issue } let(:notification_trigger) { notification.reassigned_issue(issue, @u_disabled, [assignee]) } end + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { issue } + let(:notification_trigger) { notification.reassigned_issue(issue, @u_disabled, [assignee]) } + end end describe '#relabeled_issue' do @@ -1028,6 +1068,11 @@ describe NotificationService, :mailer do should_email(subscriber_to_both) end + it_behaves_like 'project emails are disabled' do + let(:notification_target) { issue } + let(:notification_trigger) { notification.relabeled_issue(issue, [group_label_2, label_2], @u_disabled) } + end + context 'confidential issues' do let(:author) { create(:user) } let(:assignee) { create(:user) } @@ -1065,12 +1110,19 @@ describe NotificationService, :mailer do end describe '#removed_milestone_issue' do - it_behaves_like 'altered milestone notification on issue' do + context do let(:milestone) { create(:milestone, project: project, issues: [issue]) } let!(:subscriber_to_new_milestone) { create(:user) { |u| issue.toggle_subscription(u, project) } } - before do - notification.removed_milestone_issue(issue, issue.author) + it_behaves_like 'altered milestone notification on issue' do + before do + notification.removed_milestone_issue(issue, issue.author) + end + end + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { issue } + let(:notification_trigger) { notification.removed_milestone_issue(issue, issue.author) } end end @@ -1110,12 +1162,19 @@ describe NotificationService, :mailer do end describe '#changed_milestone_issue' do - it_behaves_like 'altered milestone notification on issue' do + context do let(:new_milestone) { create(:milestone, project: project, issues: [issue]) } let!(:subscriber_to_new_milestone) { create(:user) { |u| issue.toggle_subscription(u, project) } } - before do - notification.changed_milestone_issue(issue, new_milestone, issue.author) + it_behaves_like 'altered milestone notification on issue' do + before do + notification.changed_milestone_issue(issue, new_milestone, issue.author) + end + end + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { issue } + let(:notification_trigger) { notification.changed_milestone_issue(issue, new_milestone, issue.author) } end end @@ -1183,6 +1242,11 @@ describe NotificationService, :mailer do let(:issuable) { issue } let(:notification_trigger) { notification.close_issue(issue, @u_disabled) } end + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { issue } + let(:notification_trigger) { notification.close_issue(issue, @u_disabled) } + end end describe '#reopen_issue' do @@ -1214,6 +1278,11 @@ describe NotificationService, :mailer do let(:issuable) { issue } let(:notification_trigger) { notification.reopen_issue(issue, @u_disabled) } end + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { issue } + let(:notification_trigger) { notification.reopen_issue(issue, @u_disabled) } + end end describe '#issue_moved' do @@ -1240,6 +1309,11 @@ describe NotificationService, :mailer do let(:issuable) { issue } let(:notification_trigger) { notification.issue_moved(issue, new_issue, @u_disabled) } end + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { issue } + let(:notification_trigger) { notification.issue_moved(issue, new_issue, @u_disabled) } + end end describe '#issue_due' do @@ -1280,6 +1354,11 @@ describe NotificationService, :mailer do let(:issuable) { issue } let(:notification_trigger) { notification.issue_due(issue) } end + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { issue } + let(:notification_trigger) { notification.issue_due(issue) } + end end end @@ -1374,6 +1453,11 @@ describe NotificationService, :mailer do should_email(user_4) end + it_behaves_like 'project emails are disabled' do + let(:notification_target) { merge_request } + let(:notification_trigger) { notification.new_merge_request(merge_request, @u_disabled) } + end + context 'participating' do it_should_behave_like 'participating by assignee notification' do let(:participant) { create(:user, username: 'user-participant')} @@ -1406,6 +1490,11 @@ describe NotificationService, :mailer do let(:mentionable) { merge_request } include_examples 'notifications for new mentions' + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { merge_request } + let(:notification_trigger) { send_notifications(@u_watcher, @u_participant_mentioned, @u_custom_global, @u_mentioned) } + end end describe '#reassigned_merge_request' do @@ -1449,6 +1538,11 @@ describe NotificationService, :mailer do let(:issuable) { merge_request } let(:notification_trigger) { notification.reassigned_merge_request(merge_request, current_user, [assignee]) } end + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { merge_request } + let(:notification_trigger) { notification.reassigned_merge_request(merge_request, current_user, [assignee]) } + end end describe '#push_to_merge_request' do @@ -1479,6 +1573,11 @@ describe NotificationService, :mailer do let(:issuable) { merge_request } let(:notification_trigger) { notification.push_to_merge_request(merge_request, @u_disabled) } end + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { merge_request } + let(:notification_trigger) { notification.push_to_merge_request(merge_request, @u_disabled) } + end end describe '#relabel_merge_request' do @@ -1512,28 +1611,43 @@ describe NotificationService, :mailer do should_not_email(@u_participating) should_not_email(@u_lazy_participant) end + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { merge_request } + let(:notification_trigger) { notification.relabeled_merge_request(merge_request, [group_label_2, label_2], @u_disabled) } + end end describe '#removed_milestone_merge_request' do - it_behaves_like 'altered milestone notification on merge request' do - let(:milestone) { create(:milestone, project: project, merge_requests: [merge_request]) } - let!(:subscriber_to_new_milestone) { create(:user) { |u| merge_request.toggle_subscription(u, project) } } + let(:milestone) { create(:milestone, project: project, merge_requests: [merge_request]) } + let!(:subscriber_to_new_milestone) { create(:user) { |u| merge_request.toggle_subscription(u, project) } } + it_behaves_like 'altered milestone notification on merge request' do before do notification.removed_milestone_merge_request(merge_request, merge_request.author) end end + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { merge_request } + let(:notification_trigger) { notification.removed_milestone_merge_request(merge_request, merge_request.author) } + end end describe '#changed_milestone_merge_request' do - it_behaves_like 'altered milestone notification on merge request' do - let(:new_milestone) { create(:milestone, project: project, merge_requests: [merge_request]) } - let!(:subscriber_to_new_milestone) { create(:user) { |u| merge_request.toggle_subscription(u, project) } } + let(:new_milestone) { create(:milestone, project: project, merge_requests: [merge_request]) } + let!(:subscriber_to_new_milestone) { create(:user) { |u| merge_request.toggle_subscription(u, project) } } + it_behaves_like 'altered milestone notification on merge request' do before do notification.changed_milestone_merge_request(merge_request, new_milestone, merge_request.author) end end + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { merge_request } + let(:notification_trigger) { notification.changed_milestone_merge_request(merge_request, new_milestone, merge_request.author) } + end end describe '#merge_request_unmergeable' do @@ -1544,6 +1658,11 @@ describe NotificationService, :mailer do expect(email_recipients.size).to eq(1) end + it_behaves_like 'project emails are disabled' do + let(:notification_target) { merge_request } + let(:notification_trigger) { notification.merge_request_unmergeable(merge_request) } + end + describe 'when merge_when_pipeline_succeeds is true' do before do merge_request.update( @@ -1590,6 +1709,11 @@ describe NotificationService, :mailer do let(:issuable) { merge_request } let(:notification_trigger) { notification.close_mr(merge_request, @u_disabled) } end + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { merge_request } + let(:notification_trigger) { notification.close_mr(merge_request, @u_disabled) } + end end describe '#merged_merge_request' do @@ -1642,6 +1766,11 @@ describe NotificationService, :mailer do let(:issuable) { merge_request } let(:notification_trigger) { notification.merge_mr(merge_request, @u_disabled) } end + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { merge_request } + let(:notification_trigger) { notification.merge_mr(merge_request, @u_disabled) } + end end describe '#reopen_merge_request' do @@ -1672,6 +1801,11 @@ describe NotificationService, :mailer do let(:issuable) { merge_request } let(:notification_trigger) { notification.reopen_mr(merge_request, @u_disabled) } end + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { merge_request } + let(:notification_trigger) { notification.reopen_mr(merge_request, @u_disabled) } + end end describe "#resolve_all_discussions" do @@ -1695,6 +1829,11 @@ describe NotificationService, :mailer do let(:issuable) { merge_request } let(:notification_trigger) { notification.resolve_all_discussions(merge_request, @u_disabled) } end + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { merge_request } + let(:notification_trigger) { notification.resolve_all_discussions(merge_request, @u_disabled) } + end end end @@ -1719,6 +1858,11 @@ describe NotificationService, :mailer do should_not_email(@u_disabled) end + it_behaves_like 'project emails are disabled' do + let(:notification_target) { project } + let(:notification_trigger) { notification.project_was_moved(project, "gitlab/gitlab") } + end + context 'users not having access to the new location' do it 'does not send email' do old_user = create(:user) @@ -1762,6 +1906,11 @@ describe NotificationService, :mailer do should_only_email(@u_participating) end + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { project } + let(:notification_trigger) { notification.project_exported(project, @u_participating) } + end end describe '#project_not_exported' do @@ -1770,6 +1919,11 @@ describe NotificationService, :mailer do should_only_email(@u_participating) end + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { project } + let(:notification_trigger) { notification.project_not_exported(project, @u_participating, ['error']) } + end end end end @@ -1800,6 +1954,11 @@ describe NotificationService, :mailer do should_email(maintainer) should_not_email(developer) end + + it_behaves_like 'group emails are disabled' do + let(:notification_target) { group } + let(:notification_trigger) { group.request_access(added_user) } + end end describe '#decline_group_invite' do @@ -1839,6 +1998,11 @@ describe NotificationService, :mailer do should_not_email_anyone end end + + it_behaves_like 'group emails are disabled' do + let(:notification_target) { group } + let(:notification_trigger) { group.add_guest(added_user) } + end end end @@ -1859,6 +2023,11 @@ describe NotificationService, :mailer do should_only_email(project.owner) end + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { project } + let(:notification_trigger) { project.request_access(added_user) } + end end context 'for a project in a group' do @@ -1878,7 +2047,7 @@ describe NotificationService, :mailer do end end - describe '#decline_group_invite' do + describe '#decline_project_invite' do let(:member) { create(:user) } before do @@ -1900,6 +2069,11 @@ describe NotificationService, :mailer do should_only_email(added_user) end + it_behaves_like 'project emails are disabled' do + let(:notification_target) { project } + let(:notification_trigger) { create_member! } + end + context 'when notifications are disabled' do before do create_global_setting_for(added_user, :disabled) @@ -2071,6 +2245,11 @@ describe NotificationService, :mailer do should_only_email(u_custom_notification_enabled, kind: :bcc) end + it_behaves_like 'project emails are disabled' do + let(:notification_target) { pipeline } + let(:notification_trigger) { notification.pipeline_finished(pipeline) } + end + context 'when the creator has group notification email set' do let(:group_notification_email) { 'user+group@example.com' } @@ -2100,6 +2279,11 @@ describe NotificationService, :mailer do should_only_email(u_member, kind: :bcc) end + it_behaves_like 'project emails are disabled' do + let(:notification_target) { pipeline } + let(:notification_trigger) { notification.pipeline_finished(pipeline) } + end + context 'when the creator has group notification email set' do let(:group_notification_email) { 'user+group@example.com' } @@ -2215,6 +2399,11 @@ describe NotificationService, :mailer do should_only_email(u_maintainer1, u_maintainer2, u_owner) end + it_behaves_like 'project emails are disabled' do + let(:notification_target) { domain } + let(:notification_trigger) { notify! } + end + it 'emails nobody if the project is missing' do domain.project = nil @@ -2224,30 +2413,6 @@ describe NotificationService, :mailer do end end end - - describe '#pages_domain_verification_failed' do - it 'emails current watching maintainers' do - notification.pages_domain_verification_failed(domain) - - should_only_email(u_maintainer1, u_maintainer2, u_owner) - end - end - - describe '#pages_domain_enabled' do - it 'emails current watching maintainers' do - notification.pages_domain_enabled(domain) - - should_only_email(u_maintainer1, u_maintainer2, u_owner) - end - end - - describe '#pages_domain_disabled' do - it 'emails current watching maintainers' do - notification.pages_domain_disabled(domain) - - should_only_email(u_maintainer1, u_maintainer2, u_owner) - end - end end context 'Auto DevOps notifications' do @@ -2266,6 +2431,11 @@ describe NotificationService, :mailer do should_email(owner, times: 1) # Once for the disable pipeline. should_email(pipeline_user, times: 2) # Once for the new permission, once for the disable. end + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { project } + let(:notification_trigger) { notification.autodevops_disabled(pipeline, [owner.email, pipeline_user.email]) } + end end end @@ -2279,6 +2449,11 @@ describe NotificationService, :mailer do should_email(user) end + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { project } + let(:notification_trigger) { notification.repository_cleanup_success(project, user) } + end end describe '#repository_cleanup_failure' do @@ -2287,6 +2462,11 @@ describe NotificationService, :mailer do should_email(user) end + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { project } + let(:notification_trigger) { notification.repository_cleanup_failure(project, user, 'Some error') } + end end end @@ -2320,6 +2500,11 @@ describe NotificationService, :mailer do should_only_email(u_maintainer1, u_maintainer2, u_owner) end + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { project } + let(:notification_trigger) { notification.remote_mirror_update_failed(remote_mirror) } + end end end diff --git a/spec/services/projects/update_service_spec.rb b/spec/services/projects/update_service_spec.rb index 82010dd283c..31bd0f0f836 100644 --- a/spec/services/projects/update_service_spec.rb +++ b/spec/services/projects/update_service_spec.rb @@ -369,9 +369,28 @@ describe Projects::UpdateService do end end + context 'when updating #emails_disabled' do + it 'updates the attribute for the project owner' do + expect { update_project(project, user, emails_disabled: true) } + .to change { project.emails_disabled } + .to(true) + end + + it 'does not update when not project owner' do + maintainer = create(:user) + project.add_user(maintainer, :maintainer) + + expect { update_project(project, maintainer, emails_disabled: true) } + .not_to change { project.emails_disabled } + end + end + context 'with external authorization enabled' do before do enable_external_authorization_service_check + + allow(::Gitlab::ExternalAuthorization) + .to receive(:access_allowed?).with(user, 'default_label', project.full_path).and_call_original end it 'does not save the project with an error if the service denies access' do @@ -402,8 +421,7 @@ describe Projects::UpdateService do end it 'does not check the label when it does not change' do - expect(::Gitlab::ExternalAuthorization) - .not_to receive(:access_allowed?) + expect(::Gitlab::ExternalAuthorization).to receive(:access_allowed?).once update_project(project, user, { name: 'New name' }) end diff --git a/spec/support/helpers/email_helpers.rb b/spec/support/helpers/email_helpers.rb index 83ba654fab3..024340310a1 100644 --- a/spec/support/helpers/email_helpers.rb +++ b/spec/support/helpers/email_helpers.rb @@ -31,6 +31,10 @@ module EmailHelpers expect(ActionMailer::Base.deliveries).to be_empty end + def should_email_anyone + expect(ActionMailer::Base.deliveries).not_to be_empty + end + def email_recipients(kind: :to) ActionMailer::Base.deliveries.flat_map(&kind) end diff --git a/spec/support/shared_examples/services/notification_service_shared_examples.rb b/spec/support/shared_examples/services/notification_service_shared_examples.rb new file mode 100644 index 00000000000..dd338ea47c7 --- /dev/null +++ b/spec/support/shared_examples/services/notification_service_shared_examples.rb @@ -0,0 +1,54 @@ +# frozen_string_literal: true + +# Note that we actually update the attribute on the target_project/group, rather than +# using `allow`. This is because there are some specs where, based on how the notification +# is done, using an `allow` doesn't change the correct object. +shared_examples 'project emails are disabled' do + let(:target_project) { notification_target.is_a?(Project) ? notification_target : notification_target.project } + + before do + reset_delivered_emails! + target_project.clear_memoization(:emails_disabled) + end + + it 'sends no emails with project emails disabled' do + target_project.update_attribute(:emails_disabled, true) + + notification_trigger + + should_not_email_anyone + end + + it 'sends emails to someone' do + target_project.update_attribute(:emails_disabled, false) + + notification_trigger + + should_email_anyone + end +end + +shared_examples 'group emails are disabled' do + let(:target_group) { notification_target.is_a?(Group) ? notification_target : notification_target.project.group } + + before do + reset_delivered_emails! + target_group.clear_memoization(:emails_disabled) + end + + it 'sends no emails with group emails disabled' do + target_group.update_attribute(:emails_disabled, true) + + notification_trigger + + should_not_email_anyone + end + + it 'sends emails to someone' do + target_group.update_attribute(:emails_disabled, false) + + notification_trigger + + should_email_anyone + end +end |