From 38f16582725709ee89c7eeadd4415ed3e2fa57dc Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Tue, 1 Nov 2016 00:02:27 -0200 Subject: [PATCH] Refactoring notification service to find subscriptions per project --- app/services/notification_service.rb | 26 ++--- spec/services/notification_service_spec.rb | 116 ++++++++++++++------- 2 files changed, 92 insertions(+), 50 deletions(-) diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb index 3060babaf6b..ecdcbf08ee1 100644 --- a/app/services/notification_service.rb +++ b/app/services/notification_service.rb @@ -75,7 +75,7 @@ class NotificationService # * watchers of the issue's labels # def relabeled_issue(issue, added_labels, current_user) - relabeled_resource_email(issue, added_labels, current_user, :relabeled_issue_email) + relabeled_resource_email(issue, issue.project, added_labels, current_user, :relabeled_issue_email) end # When create a merge request we should send an email to: @@ -118,7 +118,7 @@ class NotificationService # * watchers of the mr's labels # def relabeled_merge_request(merge_request, added_labels, current_user) - relabeled_resource_email(merge_request, added_labels, current_user, :relabeled_merge_request_email) + relabeled_resource_email(merge_request, merge_request.target_project, added_labels, current_user, :relabeled_merge_request_email) end def close_mr(merge_request, current_user) @@ -205,7 +205,7 @@ class NotificationService recipients = reject_muted_users(recipients, note.project) - recipients = add_subscribed_users(recipients, note.noteable) + recipients = add_subscribed_users(recipients, note.project, note.noteable) recipients = reject_unsubscribed_users(recipients, note.noteable) recipients = reject_users_without_access(recipients, note.noteable) @@ -505,17 +505,17 @@ class NotificationService end end - def add_subscribed_users(recipients, target) + def add_subscribed_users(recipients, project, target) return recipients unless target.respond_to? :subscribers - recipients + target.subscribers + recipients + target.subscribers(project) end - def add_labels_subscribers(recipients, target, labels: nil) + def add_labels_subscribers(recipients, project, target, labels: nil) return recipients unless target.respond_to? :labels (labels || target.labels).each do |label| - recipients += label.subscribers + recipients += label.subscribers(project) end recipients @@ -571,8 +571,8 @@ class NotificationService end end - def relabeled_resource_email(target, labels, current_user, method) - recipients = build_relabeled_recipients(target, current_user, labels: labels) + def relabeled_resource_email(target, project, labels, current_user, method) + recipients = build_relabeled_recipients(target, project, current_user, labels: labels) label_names = labels.map(&:name) recipients.each do |recipient| @@ -608,10 +608,10 @@ class NotificationService end recipients = reject_muted_users(recipients, project) - recipients = add_subscribed_users(recipients, target) + recipients = add_subscribed_users(recipients, project, target) if [:new_issue, :new_merge_request].include?(custom_action) - recipients = add_labels_subscribers(recipients, target) + recipients = add_labels_subscribers(recipients, project, target) end recipients = reject_unsubscribed_users(recipients, target) @@ -622,8 +622,8 @@ class NotificationService recipients.uniq end - def build_relabeled_recipients(target, current_user, labels:) - recipients = add_labels_subscribers([], target, labels: labels) + def build_relabeled_recipients(target, project, current_user, labels:) + recipients = add_labels_subscribers([], project, target, labels: labels) recipients = reject_unsubscribed_users(recipients, target) recipients = reject_users_without_access(recipients, target) recipients.delete(current_user) diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index 8ce35354c22..3a5bd4a4084 100644 --- a/spec/services/notification_service_spec.rb +++ b/spec/services/notification_service_spec.rb @@ -342,7 +342,9 @@ describe NotificationService, services: true do end describe 'Issues' do - let(:project) { create(:empty_project, :public) } + let(:group) { create(:group) } + let(:project) { create(:empty_project, :public, namespace: group) } + let(:another_project) { create(:empty_project, :public, namespace: group) } let(:issue) { create :issue, project: project, assignee: create(:user), description: 'cc @participant' } before do @@ -377,13 +379,21 @@ describe NotificationService, services: true do end it "emails subscribers of the issue's labels" do - subscriber = create(:user) - label = create(:label, issues: [issue]) + user_1 = create(:user) + user_2 = create(:user) + user_3 = create(:user) + label = create(:label, project: project, issues: [issue]) + group_label = create(:group_label, group: group, issues: [issue]) issue.reload - label.toggle_subscription(subscriber) + label.toggle_subscription(user_1) + group_label.toggle_subscription(user_2, project) + group_label.toggle_subscription(user_3, another_project) + notification.new_issue(issue, @u_disabled) - should_email(subscriber) + should_email(user_1) + should_email(user_2) + should_not_email(user_3) end context 'confidential issues' do @@ -399,7 +409,7 @@ describe NotificationService, services: true do project.team << [member, :developer] project.team << [guest, :guest] - label = create(:label, issues: [confidential_issue]) + label = create(:label, project: project, issues: [confidential_issue]) confidential_issue.reload label.toggle_subscription(non_member) label.toggle_subscription(author) @@ -554,20 +564,28 @@ describe NotificationService, services: true do end describe '#relabeled_issue' do - let(:label) { create(:label, issues: [issue]) } - let(:label2) { create(:label) } - let!(:subscriber_to_label) { create(:user).tap { |u| label.toggle_subscription(u) } } - let!(:subscriber_to_label2) { create(:user).tap { |u| label2.toggle_subscription(u) } } + let(:group_label_1) { create(:group_label, group: group, title: 'Group Label 1', issues: [issue]) } + let(:group_label_2) { create(:group_label, group: group, title: 'Group Label 2') } + let(:label_1) { create(:label, project: project, title: 'Label 1', issues: [issue]) } + let(:label_2) { create(:label, project: project, title: 'Label 2') } + let!(:subscriber_to_group_label_1) { create(:user).tap { |u| group_label_1.toggle_subscription(u, project) } } + let!(:subscriber_to_group_label_2) { create(:user).tap { |u| group_label_2.toggle_subscription(u, project) } } + let!(:subscriber_to_group_label_2_on_another_project) { create(:user).tap { |u| group_label_2.toggle_subscription(u, another_project) } } + let!(:subscriber_to_label_1) { create(:user).tap { |u| label_1.toggle_subscription(u) } } + let!(:subscriber_to_label_2) { create(:user).tap { |u| label_2.toggle_subscription(u) } } it "emails subscribers of the issue's added labels only" do - notification.relabeled_issue(issue, [label2], @u_disabled) + notification.relabeled_issue(issue, [group_label_2, label_2], @u_disabled) - should_not_email(subscriber_to_label) - should_email(subscriber_to_label2) + should_not_email(subscriber_to_label_1) + should_not_email(subscriber_to_group_label_1) + should_not_email(subscriber_to_group_label_2_on_another_project) + should_email(subscriber_to_group_label_2) + should_email(subscriber_to_label_2) end it "doesn't send email to anyone but subscribers of the given labels" do - notification.relabeled_issue(issue, [label2], @u_disabled) + notification.relabeled_issue(issue, [group_label_2, label_2], @u_disabled) should_not_email(issue.assignee) should_not_email(issue.author) @@ -578,8 +596,11 @@ describe NotificationService, services: true do should_not_email(@watcher_and_subscriber) should_not_email(@unsubscriber) should_not_email(@u_participating) - should_not_email(subscriber_to_label) - should_email(subscriber_to_label2) + should_not_email(subscriber_to_label_1) + should_not_email(subscriber_to_group_label_1) + should_not_email(subscriber_to_group_label_2_on_another_project) + should_email(subscriber_to_group_label_2) + should_email(subscriber_to_label_2) end context 'confidential issues' do @@ -590,8 +611,8 @@ describe NotificationService, services: true do let(:guest) { create(:user) } let(:admin) { create(:admin) } let(:confidential_issue) { create(:issue, :confidential, project: project, title: 'Confidential issue', author: author, assignee: assignee) } - let!(:label_1) { create(:label, issues: [confidential_issue]) } - let!(:label_2) { create(:label) } + let!(:label_1) { create(:label, project: project, issues: [confidential_issue]) } + let!(:label_2) { create(:label, project: project) } it "emails subscribers of the issue's labels that can read the issue" do project.team << [member, :developer] @@ -725,7 +746,9 @@ describe NotificationService, services: true do end describe 'Merge Requests' do - let(:project) { create(:project, :public) } + let(:group) { create(:group) } + let(:project) { create(:project, :public, namespace: group) } + let(:another_project) { create(:empty_project, :public, namespace: group) } let(:merge_request) { create :merge_request, source_project: project, assignee: create(:user), description: 'cc @participant' } before do @@ -758,12 +781,20 @@ describe NotificationService, services: true do end it "emails subscribers of the merge request's labels" do - subscriber = create(:user) - label = create(:label, merge_requests: [merge_request]) - label.toggle_subscription(subscriber) + user_1 = create(:user) + user_2 = create(:user) + user_3 = create(:user) + label = create(:label, project: project, merge_requests: [merge_request]) + group_label = create(:group_label, group: group, merge_requests: [merge_request]) + label.toggle_subscription(user_1) + group_label.toggle_subscription(user_2, project) + group_label.toggle_subscription(user_3, another_project) + notification.new_merge_request(merge_request, @u_disabled) - should_email(subscriber) + should_email(user_1) + should_email(user_2) + should_not_email(user_3) end context 'participating' do @@ -857,20 +888,28 @@ describe NotificationService, services: true do end describe '#relabel_merge_request' do - let(:label) { create(:label, merge_requests: [merge_request]) } - let(:label2) { create(:label) } - let!(:subscriber_to_label) { create(:user).tap { |u| label.toggle_subscription(u) } } - let!(:subscriber_to_label2) { create(:user).tap { |u| label2.toggle_subscription(u) } } + let(:group_label_1) { create(:group_label, group: group, title: 'Group Label 1', merge_requests: [merge_request]) } + let(:group_label_2) { create(:group_label, group: group, title: 'Group Label 2') } + let(:label_1) { create(:label, project: project, title: 'Label 1', merge_requests: [merge_request]) } + let(:label_2) { create(:label, project: project, title: 'Label 2') } + let!(:subscriber_to_group_label_1) { create(:user).tap { |u| group_label_1.toggle_subscription(u, project) } } + let!(:subscriber_to_group_label_2) { create(:user).tap { |u| group_label_2.toggle_subscription(u, project) } } + let!(:subscriber_to_group_label_2_on_another_project) { create(:user).tap { |u| group_label_2.toggle_subscription(u, another_project) } } + let!(:subscriber_to_label_1) { create(:user).tap { |u| label_1.toggle_subscription(u) } } + let!(:subscriber_to_label_2) { create(:user).tap { |u| label_2.toggle_subscription(u) } } it "emails subscribers of the merge request's added labels only" do - notification.relabeled_merge_request(merge_request, [label2], @u_disabled) + notification.relabeled_merge_request(merge_request, [group_label_2, label_2], @u_disabled) - should_not_email(subscriber_to_label) - should_email(subscriber_to_label2) + should_not_email(subscriber_to_label_1) + should_not_email(subscriber_to_group_label_1) + should_not_email(subscriber_to_group_label_2_on_another_project) + should_email(subscriber_to_group_label_2) + should_email(subscriber_to_label_2) end it "doesn't send email to anyone but subscribers of the given labels" do - notification.relabeled_merge_request(merge_request, [label2], @u_disabled) + notification.relabeled_merge_request(merge_request, [group_label_2, label_2], @u_disabled) should_not_email(merge_request.assignee) should_not_email(merge_request.author) @@ -881,8 +920,11 @@ describe NotificationService, services: true do should_not_email(@unsubscriber) should_not_email(@u_participating) should_not_email(@u_lazy_participant) - should_not_email(subscriber_to_label) - should_email(subscriber_to_label2) + should_not_email(subscriber_to_label_1) + should_not_email(subscriber_to_group_label_1) + should_not_email(subscriber_to_group_label_2_on_another_project) + should_email(subscriber_to_group_label_2) + should_email(subscriber_to_label_2) end end @@ -1290,10 +1332,10 @@ describe NotificationService, services: true do project.team << [@unsubscriber, :master] project.team << [@watcher_and_subscriber, :master] - issuable.subscriptions.create(user: @subscriber, subscribed: true) - issuable.subscriptions.create(user: @subscribed_participant, subscribed: true) - issuable.subscriptions.create(user: @unsubscriber, subscribed: false) + issuable.subscriptions.create(user: @subscriber, project: project, subscribed: true) + issuable.subscriptions.create(user: @subscribed_participant, project: project, subscribed: true) + issuable.subscriptions.create(user: @unsubscriber, project: project, subscribed: false) # Make the watcher a subscriber to detect dupes - issuable.subscriptions.create(user: @watcher_and_subscriber, subscribed: true) + issuable.subscriptions.create(user: @watcher_and_subscriber, project: project, subscribed: true) end end