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 /spec/services | |
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
Diffstat (limited to 'spec/services')
-rw-r--r-- | spec/services/groups/update_service_spec.rb | 15 | ||||
-rw-r--r-- | spec/services/notification_service_spec.rb | 323 | ||||
-rw-r--r-- | spec/services/projects/update_service_spec.rb | 22 |
3 files changed, 289 insertions, 71 deletions
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 |