Merge 4009-external-users into issue_12658

6 files changed, 255 insertions, 18 deletions

diff --git a/spec/services/git_push_service_spec.rb b/spec/services/git_push_service_spec.rb
index 82813cee227..145bc937560 100644
--- a/spec/services/git_push_service_spec.rb
+++ b/spec/services/git_push_service_spec.rb
@@ -172,8 +172,8 @@ describe GitPushService, services: true do
   end
 
 
-  describe "Web Hooks" do
-    context "execute web hooks" do
+  describe "Webhooks" do
+    context "execute webhooks" do
       it "when pushing a branch for the first time" do
         expect(project).to receive(:execute_hooks)
         expect(project.default_branch).to eq("master")
@@ -401,6 +401,45 @@ describe GitPushService, services: true do
     end
   end
 
+  describe "housekeeping" do
+    let(:housekeeping) { Projects::HousekeepingService.new(project) }
+
+    before do
+      allow(Projects::HousekeepingService).to receive(:new).and_return(housekeeping)
+    end
+
+    it 'does not perform housekeeping when not needed' do
+      expect(housekeeping).not_to receive(:execute)
+
+      execute_service(project, user, @oldrev, @newrev, @ref)
+    end
+
+    context 'when housekeeping is needed' do
+      before do
+        allow(housekeeping).to receive(:needed?).and_return(true)
+      end
+
+      it 'performs housekeeping' do
+        expect(housekeeping).to receive(:execute)
+
+        execute_service(project, user, @oldrev, @newrev, @ref)
+      end
+
+      it 'does not raise an exception' do
+        allow(housekeeping).to receive(:try_obtain_lease).and_return(false)
+
+        execute_service(project, user, @oldrev, @newrev, @ref)
+      end
+    end
+
+
+    it 'increments the push counter' do
+      expect(housekeeping).to receive(:increment!)
+
+      execute_service(project, user, @oldrev, @newrev, @ref)
+    end
+  end
+
   def execute_service(project, user, oldrev, newrev, ref)
     service = described_class.new(project, user, oldrev: oldrev, newrev: newrev, ref: ref )
     service.execute
diff --git a/spec/services/git_tag_push_service_spec.rb b/spec/services/git_tag_push_service_spec.rb
index b982274c529..cc780587e74 100644
--- a/spec/services/git_tag_push_service_spec.rb
+++ b/spec/services/git_tag_push_service_spec.rb
@@ -78,8 +78,8 @@ describe GitTagPushService, services: true do
     end
   end
 
-  describe "Web Hooks" do
-    context "execute web hooks" do
+  describe "Webhooks" do
+    context "execute webhooks" do
       it "when pushing tags" do
         expect(project).to receive(:execute_hooks)
         service.execute(project, user, 'oldrev', 'newrev', 'refs/tags/v1.0.0')
diff --git a/spec/services/issues/update_service_spec.rb b/spec/services/issues/update_service_spec.rb
index e579e49dfa7..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
@@ -48,7 +49,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 +149,48 @@ describe Issues::UpdateService, services: true do
       end
     end
 
+    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
+
+        should_email(subscriber)
+        should_not_email(non_subscriber)
+      end
+
+      context 'when issue has the `label` label' do
+        before { issue.labels << label }
+
+        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
+
+          should_not_email(subscriber)
+          should_not_email(non_subscriber)
+        end
+
+        it 'does not send notifications for removed labels' do
+          opts = { label_ids: [label2.id] }
+
+          perform_enqueued_jobs do
+            @issue = Issues::UpdateService.new(project, user, opts).execute(issue)
+          end
+
+          should_not_email(subscriber)
+          should_not_email(non_subscriber)
+        end
+      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..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]
@@ -53,7 +54,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 +177,48 @@ describe MergeRequests::UpdateService, services: true do
       end
     end
 
+    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
+
+        should_email(subscriber)
+        should_not_email(non_subscriber)
+      end
+
+      context 'when issue has the `label` label' do
+        before { merge_request.labels << label }
+
+        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
+
+          should_not_email(subscriber)
+          should_not_email(non_subscriber)
+        end
+
+        it 'does not send notifications for removed labels' do
+          opts = { label_ids: [label2.id] }
+
+          perform_enqueued_jobs do
+            @merge_request = MergeRequests::UpdateService.new(project, user, opts).execute(merge_request)
+          end
+
+          should_not_email(subscriber)
+          should_not_email(non_subscriber)
+        end
+      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..b5407397c1d 100644
--- a/spec/services/notification_service_spec.rb
+++ b/spec/services/notification_service_spec.rb
@@ -224,6 +224,15 @@ describe NotificationService, services: true do
 
         should_not_email(issue.assignee)
       end
+
+      it "emails subscribers of the issue's labels" do
+        subscriber = create(:user)
+        label = create(:label, issues: [issue])
+        label.toggle_subscription(subscriber)
+        notification.new_issue(issue, @u_disabled)
+
+        should_email(subscriber)
+      end
     end
 
     describe :reassigned_issue do
@@ -296,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)
@@ -349,6 +387,15 @@ describe NotificationService, services: true do
         should_not_email(@u_participating)
         should_not_email(@u_disabled)
       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)
+        notification.new_merge_request(merge_request, @u_disabled)
+
+        should_email(subscriber)
+      end
     end
 
     describe :reassigned_merge_request do
@@ -366,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)
@@ -467,16 +543,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/services/projects/housekeeping_service_spec.rb b/spec/services/projects/housekeeping_service_spec.rb
new file mode 100644
index 00000000000..93bf1b81fbe
--- /dev/null
+++ b/spec/services/projects/housekeeping_service_spec.rb
@@ -0,0 +1,48 @@
+require 'spec_helper'
+
+describe Projects::HousekeepingService do
+  subject { Projects::HousekeepingService.new(project) }
+  let(:project) { create :project }
+
+  describe 'execute' do
+    before do
+      project.pushes_since_gc = 3
+      project.save!
+    end
+
+    it 'enqueues a sidekiq job' do
+      expect(subject).to receive(:try_obtain_lease).and_return(true)
+      expect(GitlabShellWorker).to receive(:perform_async).with(:gc, project.path_with_namespace)
+
+      subject.execute
+      expect(project.pushes_since_gc).to eq(0)
+    end
+
+    it 'does not enqueue a job when no lease can be obtained' do
+      expect(subject).to receive(:try_obtain_lease).and_return(false)
+      expect(GitlabShellWorker).not_to receive(:perform_async)
+
+      expect { subject.execute }.to raise_error(Projects::HousekeepingService::LeaseTaken)
+      expect(project.pushes_since_gc).to eq(0)
+    end
+  end
+
+  describe 'needed?' do
+    it 'when the count is low enough' do
+      expect(subject.needed?).to eq(false)
+    end
+
+    it 'when the count is high enough' do
+      allow(project).to receive(:pushes_since_gc).and_return(10)
+      expect(subject.needed?).to eq(true)
+    end
+  end
+
+  describe 'increment!' do
+    it 'increments the pushes_since_gc counter' do
+      expect(project.pushes_since_gc).to eq(0)
+      subject.increment!
+      expect(project.pushes_since_gc).to eq(1)
+    end
+  end
+end