diff options
author | Timothy Andrew <mail@timothyandrew.net> | 2016-02-12 20:28:39 +0530 |
---|---|---|
committer | Rémy Coutable <remy@rymai.me> | 2016-03-15 17:25:37 +0100 |
commit | 0444fa560acd07255960284f19b1de6499cd5910 (patch) | |
tree | 005ce576bbe661242ee58c1e1ddebd9e665bd9ff /spec | |
parent | 178c80a561fa10a157bae6e5d4682b232ae727c7 (diff) | |
download | gitlab-ce-0444fa560acd07255960284f19b1de6499cd5910.tar.gz |
Original implementation to allow users to subscribe to labels
1. Allow subscribing (the current user) to a label
- Refactor the `Subscription` coffeescript class
- The main change is that it accepts a container, and conducts all
DOM queries within its scope. We need this because the labels
page has multiple instances of `Subscription` on the same page.
2. Creating an issue or MR with labels notifies users subscribed to those labels
- Label `has_many` subscribers through subscriptions.
3. Adding a label to an issue or MR notifies users subscribed to those labels
- This only applies to subscribers of the label that has just been
added, not all labels for the issue.
Diffstat (limited to 'spec')
-rw-r--r-- | spec/factories/labels.rb | 2 | ||||
-rw-r--r-- | spec/models/concerns/subscribable_spec.rb | 16 | ||||
-rw-r--r-- | spec/services/issues/update_service_spec.rb | 56 | ||||
-rw-r--r-- | spec/services/merge_requests/update_service_spec.rb | 56 | ||||
-rw-r--r-- | spec/services/notification_service_spec.rb | 87 | ||||
-rw-r--r-- | spec/spec_helper.rb | 1 | ||||
-rw-r--r-- | spec/support/email_helpers.rb | 13 |
7 files changed, 216 insertions, 15 deletions
diff --git a/spec/factories/labels.rb b/spec/factories/labels.rb index 6e70af10af3..91a7400afa1 100644 --- a/spec/factories/labels.rb +++ b/spec/factories/labels.rb @@ -13,7 +13,7 @@ FactoryGirl.define do factory :label do - title "Bug" + title { FFaker::Color.name } color "#990000" project end diff --git a/spec/models/concerns/subscribable_spec.rb b/spec/models/concerns/subscribable_spec.rb new file mode 100644 index 00000000000..9ee60426a5d --- /dev/null +++ b/spec/models/concerns/subscribable_spec.rb @@ -0,0 +1,16 @@ +require "spec_helper" + +describe Subscribable, "Subscribable" do + let(:resource) { create(:issue) } + let(:user) { create(:user) } + + describe "#subscribed?" do + it do + expect(resource.subscribed?(user)).to be_falsey + resource.toggle_subscription(user) + expect(resource.subscribed?(user)).to be_truthy + resource.toggle_subscription(user) + expect(resource.subscribed?(user)).to be_falsey + end + end +end diff --git a/spec/services/issues/update_service_spec.rb b/spec/services/issues/update_service_spec.rb index e579e49dfa7..dc9d8329751 100644 --- a/spec/services/issues/update_service_spec.rb +++ b/spec/services/issues/update_service_spec.rb @@ -48,7 +48,7 @@ describe Issues::UpdateService, services: true do it { expect(@issue.assignee).to eq(user2) } it { expect(@issue).to be_closed } it { expect(@issue.labels.count).to eq(1) } - it { expect(@issue.labels.first.title).to eq('Bug') } + it { expect(@issue.labels.first.title).to eq(label.name) } it 'should send email to user2 about assign of new issue and email to user3 about issue unassignment' do deliveries = ActionMailer::Base.deliveries @@ -148,6 +148,60 @@ describe Issues::UpdateService, services: true do end end + context "when the issue is relabeled" do + it "sends notifications for subscribers of newly added labels" do + subscriber, non_subscriber = create_list(:user, 2) + label.toggle_subscription(subscriber) + 2.times { label.toggle_subscription(non_subscriber) } + + opts = { label_ids: [label.id] } + + perform_enqueued_jobs do + @issue = Issues::UpdateService.new(project, user, opts).execute(issue) + end + + @issue.reload + should_email(subscriber) + should_not_email(non_subscriber) + end + + it "does send notifications for existing labels" do + second_label = create(:label) + issue.labels << label + subscriber, non_subscriber = create_list(:user, 2) + label.toggle_subscription(subscriber) + 2.times { label.toggle_subscription(non_subscriber) } + + opts = { label_ids: [label.id, second_label.id] } + + perform_enqueued_jobs do + @issue = Issues::UpdateService.new(project, user, opts).execute(issue) + end + + @issue.reload + should_email(subscriber) + should_not_email(non_subscriber) + end + + it "does not send notifications for removed labels" do + second_label = create(:label) + issue.labels << label + subscriber, non_subscriber = create_list(:user, 2) + label.toggle_subscription(subscriber) + 2.times { label.toggle_subscription(non_subscriber) } + + opts = { label_ids: [second_label.id] } + + perform_enqueued_jobs do + @issue = Issues::UpdateService.new(project, user, opts).execute(issue) + end + + @issue.reload + should_not_email(subscriber) + should_not_email(non_subscriber) + end + end + context 'when Issue has tasks' do before { update_issue({ description: "- [ ] Task 1\n- [ ] Task 2" }) } diff --git a/spec/services/merge_requests/update_service_spec.rb b/spec/services/merge_requests/update_service_spec.rb index 99703c7a8ec..104e63ccfee 100644 --- a/spec/services/merge_requests/update_service_spec.rb +++ b/spec/services/merge_requests/update_service_spec.rb @@ -53,7 +53,7 @@ describe MergeRequests::UpdateService, services: true do it { expect(@merge_request.assignee).to eq(user2) } it { expect(@merge_request).to be_closed } it { expect(@merge_request.labels.count).to eq(1) } - it { expect(@merge_request.labels.first.title).to eq('Bug') } + it { expect(@merge_request.labels.first.title).to eq(label.name) } it { expect(@merge_request.target_branch).to eq('target') } it 'should execute hooks with update action' do @@ -176,6 +176,60 @@ describe MergeRequests::UpdateService, services: true do end end + context "when the merge request is relabeled" do + it "sends notifications for subscribers of newly added labels" do + subscriber, non_subscriber = create_list(:user, 2) + label.toggle_subscription(subscriber) + 2.times { label.toggle_subscription(non_subscriber) } + + opts = { label_ids: [label.id] } + + perform_enqueued_jobs do + @merge_request = MergeRequests::UpdateService.new(project, user, opts).execute(merge_request) + end + + @merge_request.reload + should_email(subscriber) + should_not_email(non_subscriber) + end + + it "does send notifications for existing labels" do + second_label = create(:label) + merge_request.labels << label + subscriber, non_subscriber = create_list(:user, 2) + label.toggle_subscription(subscriber) + 2.times { label.toggle_subscription(non_subscriber) } + + opts = { label_ids: [label.id, second_label.id] } + + perform_enqueued_jobs do + @merge_request = MergeRequests::UpdateService.new(project, user, opts).execute(merge_request) + end + + @merge_request.reload + should_email(subscriber) + should_not_email(non_subscriber) + end + + it "does not send notifications for removed labels" do + second_label = create(:label) + merge_request.labels << label + subscriber, non_subscriber = create_list(:user, 2) + label.toggle_subscription(subscriber) + 2.times { label.toggle_subscription(non_subscriber) } + + opts = { label_ids: [second_label.id] } + + perform_enqueued_jobs do + @merge_request = MergeRequests::UpdateService.new(project, user, opts).execute(merge_request) + end + + @merge_request.reload + should_not_email(subscriber) + should_not_email(non_subscriber) + end + end + context 'when MergeRequest has tasks' do before { update_merge_request({ description: "- [ ] Task 1\n- [ ] Task 2" }) } diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index 2d0b5df4224..35afa768057 100644 --- a/spec/services/notification_service_spec.rb +++ b/spec/services/notification_service_spec.rb @@ -224,6 +224,16 @@ describe NotificationService, services: true do should_not_email(issue.assignee) end + + it "should email subscribers of the issue's labels" do + subscriber, non_subscriber = create_list(:user, 2) + label = create(:label, issues: [issue]) + label.toggle_subscription(subscriber) + 2.times { label.toggle_subscription(non_subscriber) } + notification.new_issue(issue, @u_disabled) + should_email(subscriber) + should_not_email(non_subscriber) + end end describe :reassigned_issue do @@ -326,6 +336,32 @@ describe NotificationService, services: true do should_not_email(@u_participating) end end + + describe :relabel_issue do + it "sends email to subscribers of the given labels" do + subscriber, non_subscriber = create_list(:user, 2) + label = create(:label, issues: [issue]) + label.toggle_subscription(subscriber) + 2.times { label.toggle_subscription(non_subscriber) } + notification.relabeled_issue(issue, [label], @u_disabled) + should_email(subscriber) + should_not_email(non_subscriber) + end + + it "doesn't send email to anyone but subscribers of the given labels" do + label = create(:label, issues: [issue]) + notification.relabeled_issue(issue, [label], @u_disabled) + + should_not_email(issue.assignee) + should_not_email(issue.author) + should_not_email(@u_watcher) + should_not_email(@u_participant_mentioned) + should_not_email(@subscriber) + should_not_email(@watcher_and_subscriber) + should_not_email(@unsubscriber) + should_not_email(@u_participating) + end + end end describe 'Merge Requests' do @@ -349,6 +385,17 @@ describe NotificationService, services: true do should_not_email(@u_participating) should_not_email(@u_disabled) end + + it "should email subscribers of the MR's labels" do + subscriber, non_subscriber = create_list(:user, 2) + label = create(:label) + merge_request.labels << label + label.toggle_subscription(subscriber) + 2.times { label.toggle_subscription(non_subscriber) } + notification.new_merge_request(merge_request, @u_disabled) + should_email(subscriber) + should_not_email(non_subscriber) + end end describe :reassigned_merge_request do @@ -410,6 +457,34 @@ describe NotificationService, services: true do should_not_email(@u_disabled) end end + + describe :relabel_merge_request do + it "sends email to subscribers of the given labels" do + subscriber, non_subscriber = create_list(:user, 2) + label = create(:label) + merge_request.labels << label + label.toggle_subscription(subscriber) + 2.times { label.toggle_subscription(non_subscriber) } + notification.relabeled_merge_request(merge_request, [label], @u_disabled) + should_email(subscriber) + should_not_email(non_subscriber) + end + + it "doesn't send email to anyone but subscribers of the given labels" do + label = create(:label) + merge_request.labels << label + notification.relabeled_merge_request(merge_request, [label], @u_disabled) + + should_not_email(merge_request.assignee) + should_not_email(merge_request.author) + should_not_email(@u_watcher) + should_not_email(@u_participant_mentioned) + should_not_email(@subscriber) + should_not_email(@watcher_and_subscriber) + should_not_email(@unsubscriber) + should_not_email(@u_participating) + end + end end describe 'Projects' do @@ -467,16 +542,4 @@ describe NotificationService, services: true do # Make the watcher a subscriber to detect dupes issuable.subscriptions.create(user: @watcher_and_subscriber, subscribed: true) end - - def sent_to_user?(user) - ActionMailer::Base.deliveries.map(&:to).flatten.count(user.email) == 1 - end - - def should_email(user) - expect(sent_to_user?(user)).to be_truthy - end - - def should_not_email(user) - expect(sent_to_user?(user)).to be_falsey - end end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 7d939ca7509..596d607f2a1 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -32,6 +32,7 @@ RSpec.configure do |config| config.include LoginHelpers, type: :feature config.include LoginHelpers, type: :request config.include StubConfiguration + config.include EmailHelpers config.include RelativeUrl, type: feature config.include TestEnv config.include ActiveJob::TestHelper diff --git a/spec/support/email_helpers.rb b/spec/support/email_helpers.rb new file mode 100644 index 00000000000..a85ab22ce36 --- /dev/null +++ b/spec/support/email_helpers.rb @@ -0,0 +1,13 @@ +module EmailHelpers + def sent_to_user?(user) + ActionMailer::Base.deliveries.map(&:to).flatten.count(user.email) == 1 + end + + def should_email(user) + expect(sent_to_user?(user)).to be_truthy + end + + def should_not_email(user) + expect(sent_to_user?(user)).to be_falsey + end +end |