Improving the original label-subscribing implementation

1. Make the "subscribed" text in Issuable sidebar reflect the labels
   subscription status

2. Current user mut be logged-in to toggle issue/MR/label subscription

8 files changed, 259 insertions, 137 deletions

diff --git a/spec/factories/labels.rb b/spec/factories/labels.rb
index 91a7400afa1..ea2be8928d5 100644
--- a/spec/factories/labels.rb
+++ b/spec/factories/labels.rb
@@ -13,7 +13,7 @@
 
 FactoryGirl.define do
   factory :label do
-    title { FFaker::Color.name }
+    sequence(:title) { |n| "label#{n}" }
     color "#990000"
     project
   end
diff --git a/spec/mailers/notify_spec.rb b/spec/mailers/notify_spec.rb
index 232a11245a6..f910424d85b 100644
--- a/spec/mailers/notify_spec.rb
+++ b/spec/mailers/notify_spec.rb
@@ -100,6 +100,34 @@ describe Notify do
           end
         end
 
+        describe 'that have been relabeled' do
+          subject { Notify.relabeled_issue_email(recipient.id, issue.id, %w[foo bar baz], current_user.id) }
+
+          it_behaves_like 'a multiple recipients email'
+          it_behaves_like 'an answer to an existing thread', 'issue'
+          it_behaves_like 'it should show Gmail Actions View Issue link'
+          it_behaves_like 'a user cannot unsubscribe through footer link'
+          it_behaves_like 'an email with a labels subscriptions link in its footer'
+
+          it 'is sent as the author' do
+            sender = subject.header[:from].addrs[0]
+            expect(sender.display_name).to eq(current_user.name)
+            expect(sender.address).to eq(gitlab_sender)
+          end
+
+          it 'has the correct subject' do
+            is_expected.to have_subject /#{issue.title} \(##{issue.iid}\)/
+          end
+
+          it 'contains the names of the added labels' do
+            is_expected.to have_body_text /foo, bar, and baz/
+          end
+
+          it 'contains a link to the issue' do
+            is_expected.to have_body_text /#{namespace_project_issue_path project.namespace, project, issue}/
+          end
+        end
+
         describe 'status changed' do
           let(:status) { 'closed' }
           subject { Notify.issue_status_changed_email(recipient.id, issue.id, status, current_user.id) }
@@ -219,6 +247,34 @@ describe Notify do
           end
         end
 
+        describe 'that have been relabeled' do
+          subject { Notify.relabeled_merge_request_email(recipient.id, merge_request.id, %w[foo bar baz], current_user.id) }
+
+          it_behaves_like 'a multiple recipients email'
+          it_behaves_like 'an answer to an existing thread', 'merge_request'
+          it_behaves_like 'it should show Gmail Actions View Merge request link'
+          it_behaves_like 'a user cannot unsubscribe through footer link'
+          it_behaves_like 'an email with a labels subscriptions link in its footer'
+
+          it 'is sent as the author' do
+            sender = subject.header[:from].addrs[0]
+            expect(sender.display_name).to eq(current_user.name)
+            expect(sender.address).to eq(gitlab_sender)
+          end
+
+          it 'has the correct subject' do
+            is_expected.to have_subject /#{merge_request.title} \(##{merge_request.iid}\)/
+          end
+
+          it 'contains the names of the added labels' do
+            is_expected.to have_body_text /foo, bar, and baz/
+          end
+
+          it 'contains a link to the merge request' do
+            is_expected.to have_body_text /#{namespace_project_merge_request_path project.namespace, project, merge_request}/
+          end
+        end
+
         describe 'status changed' do
           let(:status) { 'reopened' }
           subject { Notify.merge_request_status_email(recipient.id, merge_request.id, status, current_user.id) }
diff --git a/spec/mailers/shared/notify.rb b/spec/mailers/shared/notify.rb
index 48c851ebbd6..6019af544d3 100644
--- a/spec/mailers/shared/notify.rb
+++ b/spec/mailers/shared/notify.rb
@@ -112,6 +112,10 @@ shared_examples 'an unsubscribeable thread' do
   it { is_expected.to have_body_text /unsubscribe/ }
 end
 
-shared_examples "a user cannot unsubscribe through footer link" do
+shared_examples 'a user cannot unsubscribe through footer link' do
   it { is_expected.not_to have_body_text /unsubscribe/ }
 end
+
+shared_examples 'an email with a labels subscriptions link in its footer' do
+  it { is_expected.to have_body_text /label subscriptions/ }
+end
diff --git a/spec/models/concerns/issuable_spec.rb b/spec/models/concerns/issuable_spec.rb
index aff384c2949..be29b6d66ff 100644
--- a/spec/models/concerns/issuable_spec.rb
+++ b/spec/models/concerns/issuable_spec.rb
@@ -113,6 +113,48 @@ describe Issue, "Issuable" do
     end
   end
 
+  describe '#subscribed?' do
+    context 'user is not a participant in the issue' do
+      before { allow(issue).to receive(:participants).with(user).and_return([]) }
+
+      it 'returns false when no subcription exists' do
+        expect(issue.subscribed?(user)).to be_falsey
+      end
+
+      it 'returns true when a subcription exists and subscribed is true' do
+        issue.subscriptions.create(user: user, subscribed: true)
+
+        expect(issue.subscribed?(user)).to be_truthy
+      end
+
+      it 'returns false when a subcription exists and subscribed is false' do
+        issue.subscriptions.create(user: user, subscribed: false)
+
+        expect(issue.subscribed?(user)).to be_falsey
+      end
+    end
+
+    context 'user is a participant in the issue' do
+      before { allow(issue).to receive(:participants).with(user).and_return([user]) }
+
+      it 'returns false when no subcription exists' do
+        expect(issue.subscribed?(user)).to be_truthy
+      end
+
+      it 'returns true when a subcription exists and subscribed is true' do
+        issue.subscriptions.create(user: user, subscribed: true)
+
+        expect(issue.subscribed?(user)).to be_truthy
+      end
+
+      it 'returns false when a subcription exists and subscribed is false' do
+        issue.subscriptions.create(user: user, subscribed: false)
+
+        expect(issue.subscribed?(user)).to be_falsey
+      end
+    end
+  end
+
   describe "#to_hook_data" do
     let(:data) { issue.to_hook_data(user) }
     let(:project) { issue.project }
diff --git a/spec/models/concerns/subscribable_spec.rb b/spec/models/concerns/subscribable_spec.rb
index 9ee60426a5d..e31fdb0bffb 100644
--- a/spec/models/concerns/subscribable_spec.rb
+++ b/spec/models/concerns/subscribable_spec.rb
@@ -1,15 +1,56 @@
-require "spec_helper"
+require 'spec_helper'
 
-describe Subscribable, "Subscribable" do
+describe Subscribable, 'Subscribable' do
   let(:resource) { create(:issue) }
   let(:user) { create(:user) }
 
-  describe "#subscribed?" do
-    it do
+  describe '#subscribed?' do
+    it 'returns false when no subcription exists' do
       expect(resource.subscribed?(user)).to be_falsey
-      resource.toggle_subscription(user)
+    end
+
+    it 'returns true when a subcription exists and subscribed is true' do
+      resource.subscriptions.create(user: user, subscribed: true)
+
       expect(resource.subscribed?(user)).to be_truthy
+    end
+
+    it 'returns false when a subcription exists and subscribed is false' do
+      resource.subscriptions.create(user: user, subscribed: false)
+
+      expect(resource.subscribed?(user)).to be_falsey
+    end
+  end
+  describe '#subscribers' do
+    it 'returns [] when no subcribers exists' do
+      expect(resource.subscribers).to be_empty
+    end
+
+    it 'returns the subscribed users' do
+      resource.subscriptions.create(user: user, subscribed: true)
+      resource.subscriptions.create(user: create(:user), subscribed: false)
+
+      expect(resource.subscribers).to eq [user]
+    end
+  end
+
+  describe '#toggle_subscription' do
+    it 'toggles the current subscription state for the given user' do
+      expect(resource.subscribed?(user)).to be_falsey
+
       resource.toggle_subscription(user)
+
+      expect(resource.subscribed?(user)).to be_truthy
+    end
+  end
+
+  describe '#unsubscribe' do
+    it 'unsubscribes the given current user' do
+      resource.subscriptions.create(user: user, subscribed: true)
+      expect(resource.subscribed?(user)).to be_truthy
+
+      resource.unsubscribe(user)
+
       expect(resource.subscribed?(user)).to be_falsey
     end
   end
diff --git a/spec/services/issues/update_service_spec.rb b/spec/services/issues/update_service_spec.rb
index dc9d8329751..4ffe753fef5 100644
--- a/spec/services/issues/update_service_spec.rb
+++ b/spec/services/issues/update_service_spec.rb
@@ -6,6 +6,7 @@ describe Issues::UpdateService, services: true do
   let(:user3) { create(:user) }
   let(:issue) { create(:issue, title: 'Old title', assignee_id: user3.id) }
   let(:label) { create(:label) }
+  let(:label2) { create(:label) }
   let(:project) { issue.project }
 
   before do
@@ -148,57 +149,45 @@ 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) }
+    context 'when the issue is relabeled' do
+      let!(:non_subscriber) { create(:user) }
+      let!(:subscriber) { create(:user).tap { |u| label.toggle_subscription(u) } }
 
+      it 'sends notifications for subscribers of newly added labels' do
         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) }
+      context 'when issue has the `label` label' do
+        before { issue.labels << label }
 
-        opts = { label_ids: [label.id, second_label.id] }
+        it 'does not send notifications for existing labels' do
+          opts = { label_ids: [label.id, label2.id] }
 
-        perform_enqueued_jobs do
-          @issue = Issues::UpdateService.new(project, user, opts).execute(issue)
-        end
+          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
+          should_not_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) }
+        it 'does not send notifications for removed labels' do
+          opts = { label_ids: [label2.id] }
 
-        opts = { label_ids: [second_label.id] }
+          perform_enqueued_jobs do
+            @issue = Issues::UpdateService.new(project, user, opts).execute(issue)
+          end
 
-        perform_enqueued_jobs do
-          @issue = Issues::UpdateService.new(project, user, opts).execute(issue)
+          should_not_email(subscriber)
+          should_not_email(non_subscriber)
         end
-
-        @issue.reload
-        should_not_email(subscriber)
-        should_not_email(non_subscriber)
       end
     end
 
diff --git a/spec/services/merge_requests/update_service_spec.rb b/spec/services/merge_requests/update_service_spec.rb
index 104e63ccfee..cb8cff2fa8c 100644
--- a/spec/services/merge_requests/update_service_spec.rb
+++ b/spec/services/merge_requests/update_service_spec.rb
@@ -7,6 +7,7 @@ describe MergeRequests::UpdateService, services: true do
   let(:merge_request) { create(:merge_request, :simple, title: 'Old title', assignee_id: user3.id) }
   let(:project) { merge_request.project }
   let(:label) { create(:label) }
+  let(:label2) { create(:label) }
 
   before do
     project.team << [user, :master]
@@ -176,57 +177,45 @@ 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) }
+    context 'when the issue is relabeled' do
+      let!(:non_subscriber) { create(:user) }
+      let!(:subscriber) { create(:user).tap { |u| label.toggle_subscription(u) } }
 
+      it 'sends notifications for subscribers of newly added labels' do
         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) }
+      context 'when issue has the `label` label' do
+        before { merge_request.labels << label }
 
-        opts = { label_ids: [label.id, second_label.id] }
+        it 'does not send notifications for existing labels' do
+          opts = { label_ids: [label.id, label2.id] }
 
-        perform_enqueued_jobs do
-          @merge_request = MergeRequests::UpdateService.new(project, user, opts).execute(merge_request)
-        end
+          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
+          should_not_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) }
+        it 'does not send notifications for removed labels' do
+          opts = { label_ids: [label2.id] }
 
-        opts = { label_ids: [second_label.id] }
+          perform_enqueued_jobs do
+            @merge_request = MergeRequests::UpdateService.new(project, user, opts).execute(merge_request)
+          end
 
-        perform_enqueued_jobs do
-          @merge_request = MergeRequests::UpdateService.new(project, user, opts).execute(merge_request)
+          should_not_email(subscriber)
+          should_not_email(non_subscriber)
         end
-
-        @merge_request.reload
-        should_not_email(subscriber)
-        should_not_email(non_subscriber)
       end
     end
 
diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb
index 35afa768057..b5407397c1d 100644
--- a/spec/services/notification_service_spec.rb
+++ b/spec/services/notification_service_spec.rb
@@ -225,14 +225,13 @@ 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)
+      it "emails subscribers of the issue's labels" do
+        subscriber = create(:user)
         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
 
@@ -306,6 +305,35 @@ describe NotificationService, services: true do
       end
     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) } }
+
+      it "emails subscribers of the issue's added labels only" do
+        notification.relabeled_issue(issue, [label2], @u_disabled)
+
+        should_not_email(subscriber_to_label)
+        should_email(subscriber_to_label2)
+      end
+
+      it "doesn't send email to anyone but subscribers of the given labels" do
+        notification.relabeled_issue(issue, [label2], @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)
+        should_not_email(subscriber_to_label)
+        should_email(subscriber_to_label2)
+      end
+    end
+
     describe :close_issue do
       it 'should sent email to issue assignee and issue author' do
         notification.close_issue(issue, @u_disabled)
@@ -336,32 +364,6 @@ 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
@@ -386,15 +388,13 @@ describe NotificationService, services: true do
         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
+      it "emails subscribers of the merge request's labels" do
+        subscriber = create(:user)
+        label = create(:label, merge_requests: [merge_request])
         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
 
@@ -413,6 +413,35 @@ describe NotificationService, services: true do
       end
     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) } }
+
+      it "emails subscribers of the merge request's added labels only" do
+        notification.relabeled_merge_request(merge_request, [label2], @u_disabled)
+
+        should_not_email(subscriber_to_label)
+        should_email(subscriber_to_label2)
+      end
+
+      it "doesn't send email to anyone but subscribers of the given labels" do
+        notification.relabeled_merge_request(merge_request, [label2], @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)
+        should_not_email(subscriber_to_label)
+        should_email(subscriber_to_label2)
+      end
+    end
+
     describe :closed_merge_request do
       it do
         notification.close_mr(merge_request, @u_disabled)
@@ -457,34 +486,6 @@ 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