kotovalexarian-likes-gitlab
/
gitlab-org--gitlab-foss
1
0
Fork 0
Code Releases Activity

Refactoring notification service to find subscriptions per project

This commit is contained in:
Douglas Barbosa Alexandre 2016-11-01 00:02:27 -02:00
parent 7fcd469e3e
commit 38f1658272
2 changed files with 92 additions and 50 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

26
app/services/notification_service.rb
View File

@ -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)

116
spec/services/notification_service_spec.rb
View File

@ -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