Address review feedback

9 files changed, 116 insertions, 86 deletions

diff --git a/app/controllers/projects/discussions_controller.rb b/app/controllers/projects/discussions_controller.rb
index 502aa20d7b6..620b41b1011 100644
--- a/app/controllers/projects/discussions_controller.rb
+++ b/app/controllers/projects/discussions_controller.rb
@@ -9,7 +9,7 @@ class Projects::DiscussionsController < Projects::ApplicationController
 
     discussion.resolve!(current_user)
 
-    MergeRequests::AllDiscussionsResolvedService.new(project, current_user).execute(merge_request)
+    MergeRequests::ResolvedDiscussionNotificationService.new(project, current_user).execute(merge_request)
 
     render json: {
       resolved_by: discussion.resolved_by.try(:name),
diff --git a/app/controllers/projects/notes_controller.rb b/app/controllers/projects/notes_controller.rb
index 934a7961a63..c16fc228f27 100644
--- a/app/controllers/projects/notes_controller.rb
+++ b/app/controllers/projects/notes_controller.rb
@@ -72,7 +72,7 @@ class Projects::NotesController < Projects::ApplicationController
 
     note.resolve!(current_user)
 
-    MergeRequests::AllDiscussionsResolvedService.new(project, current_user).execute(note.noteable)
+    MergeRequests::ResolvedDiscussionNotificationService.new(project, current_user).execute(note.noteable)
 
     discussion = note.discussion
 
@@ -166,7 +166,7 @@ class Projects::NotesController < Projects::ApplicationController
       }
 
       if note.diff_note?
-        discussion = note.as_discussion
+        discussion = note.to_discussion
 
         attrs.merge!(
           diff_discussion_html: diff_discussion_html(discussion),
diff --git a/app/models/diff_note.rb b/app/models/diff_note.rb
index 64c347a3e92..cfcdce790e5 100644
--- a/app/models/diff_note.rb
+++ b/app/models/diff_note.rb
@@ -113,7 +113,7 @@ class DiffNote < Note
     Discussion.new(discussion_notes)
   end
 
-  def as_discussion
+  def to_discussion
     Discussion.new([self])
   end
 
diff --git a/app/models/discussion.rb b/app/models/discussion.rb
index 09d4596094e..b26d2282b00 100644
--- a/app/models/discussion.rb
+++ b/app/models/discussion.rb
@@ -57,6 +57,8 @@ class Discussion
   def id
     first_note.discussion_id
   end
+  
+  alias_method :to_param, :id
 
   def diff_discussion?
     first_note.diff_note?
diff --git a/app/services/merge_requests/all_discussions_resolved_service.rb b/app/services/merge_requests/resolved_discussion_notification_service.rb
index c686b23335b..3a09350c847 100644
--- a/app/services/merge_requests/all_discussions_resolved_service.rb
+++ b/app/services/merge_requests/resolved_discussion_notification_service.rb
@@ -1,5 +1,5 @@
 module MergeRequests
-  class AllDiscussionsResolvedService < MergeRequests::BaseService
+  class ResolvedDiscussionNotificationService < MergeRequests::BaseService
     def execute(merge_request)
       return unless merge_request.discussions_resolved?
 
diff --git a/spec/controllers/projects/discussions_controller_spec.rb b/spec/controllers/projects/discussions_controller_spec.rb
index 14c8a76fb8e..ff617fea847 100644
--- a/spec/controllers/projects/discussions_controller_spec.rb
+++ b/spec/controllers/projects/discussions_controller_spec.rb
@@ -5,6 +5,16 @@ describe Projects::DiscussionsController do
   let(:project) { create(:project) }
   let(:merge_request) { create(:merge_request, source_project: project) }
   let(:note) { create(:diff_note_on_merge_request, noteable: merge_request, project: project) }
+  let(:discussion) { note.discussion }
+
+  let(:request_params) do
+    {
+      namespace_id: project.namespace,
+      project_id: project,
+      merge_request_id: merge_request,
+      id: note.discussion_id
+    }
+  end
 
   describe 'POST resolve' do
     before do
@@ -13,7 +23,7 @@ describe Projects::DiscussionsController do
 
     context "when the user is not authorized to resolve the discussion" do
       it "returns status 404" do
-        post :resolve, namespace_id: project.namespace.path, project_id: project.path, merge_request_id: merge_request.iid, id: note.discussion_id
+        post :resolve, request_params
 
         expect(response).to have_http_status(404)
       end
@@ -30,7 +40,7 @@ describe Projects::DiscussionsController do
         end
 
         it "returns status 404" do
-          post :resolve, namespace_id: project.namespace.path, project_id: project.path, merge_request_id: merge_request.iid, id: note.discussion_id
+          post :resolve, request_params
 
           expect(response).to have_http_status(404)
         end
@@ -38,25 +48,26 @@ describe Projects::DiscussionsController do
 
       context "when the discussion is resolvable" do
         it "resolves the discussion" do
-          expect_any_instance_of(Discussion).to receive(:resolve!).with(user)
+          post :resolve, request_params
 
-          post :resolve, namespace_id: project.namespace.path, project_id: project.path, merge_request_id: merge_request.iid, id: note.discussion_id
+          expect(note.reload.discussion.resolved?).to be true
+          expect(note.reload.discussion.resolved_by).to eq(user)
         end
 
-        it "checks whether all notes are resolved" do
-          expect_any_instance_of(MergeRequests::AllDiscussionsResolvedService).to receive(:execute).with(merge_request)
+        it "sends notifications if all discussions are resolved" do
+          expect_any_instance_of(MergeRequests::ResolvedDiscussionNotificationService).to receive(:execute).with(merge_request)
 
-          post :resolve, namespace_id: project.namespace.path, project_id: project.path, merge_request_id: merge_request.iid, id: note.discussion_id
+          post :resolve, request_params
         end
 
         it "returns the name of the resolving user" do
-          post :resolve, namespace_id: project.namespace.path, project_id: project.path, merge_request_id: merge_request.iid, id: note.discussion_id
+          post :resolve, request_params
 
           expect(JSON.parse(response.body)["resolved_by"]).to eq(user.name)
         end
 
         it "returns status 200" do
-          post :resolve, namespace_id: project.namespace.path, project_id: project.path, merge_request_id: merge_request.iid, id: note.discussion_id
+          post :resolve, request_params
 
           expect(response).to have_http_status(200)
         end
@@ -67,11 +78,13 @@ describe Projects::DiscussionsController do
   describe 'DELETE unresolve' do
     before do
       sign_in user
+
+      note.discussion.resolve!(user)
     end
 
     context "when the user is not authorized to resolve the discussion" do
       it "returns status 404" do
-        delete :unresolve, namespace_id: project.namespace.path, project_id: project.path, merge_request_id: merge_request.iid, id: note.discussion_id
+        delete :unresolve, request_params
 
         expect(response).to have_http_status(404)
       end
@@ -88,7 +101,7 @@ describe Projects::DiscussionsController do
         end
 
         it "returns status 404" do
-          delete :unresolve, namespace_id: project.namespace.path, project_id: project.path, merge_request_id: merge_request.iid, id: note.discussion_id
+          delete :unresolve, request_params
 
           expect(response).to have_http_status(404)
         end
@@ -96,13 +109,13 @@ describe Projects::DiscussionsController do
 
       context "when the discussion is resolvable" do
         it "unresolves the discussion" do
-          expect_any_instance_of(Discussion).to receive(:unresolve!)
+          delete :unresolve, request_params
 
-          delete :unresolve, namespace_id: project.namespace.path, project_id: project.path, merge_request_id: merge_request.iid, id: note.discussion_id
+          expect(note.reload.discussion.resolved?).to be false
         end
 
         it "returns status 200" do
-          delete :unresolve, namespace_id: project.namespace.path, project_id: project.path, merge_request_id: merge_request.iid, id: note.discussion_id
+          delete :unresolve, request_params
 
           expect(response).to have_http_status(200)
         end
diff --git a/spec/controllers/projects/notes_controller_spec.rb b/spec/controllers/projects/notes_controller_spec.rb
index d57d5c0b963..92e38b02615 100644
--- a/spec/controllers/projects/notes_controller_spec.rb
+++ b/spec/controllers/projects/notes_controller_spec.rb
@@ -6,6 +6,14 @@ describe Projects::NotesController do
   let(:issue)   { create(:issue, project: project) }
   let(:note)    { create(:note, noteable: issue, project: project) }
 
+  let(:request_params) do
+    {
+      namespace_id: project.namespace,
+      project_id: project,
+      id: note
+    }
+  end
+
   describe 'POST toggle_award_emoji' do
     before do
       sign_in(user)
@@ -14,123 +22,130 @@ describe Projects::NotesController do
 
     it "toggles the award emoji" do
       expect do
-        post(:toggle_award_emoji, namespace_id: project.namespace.path,
-                                  project_id: project.path, id: note.id, name: "thumbsup")
+        post(:toggle_award_emoji, request_params.merge(name: "thumbsup"))
       end.to change { note.award_emoji.count }.by(1)
 
       expect(response).to have_http_status(200)
     end
 
     it "removes the already awarded emoji" do
-      post(:toggle_award_emoji, namespace_id: project.namespace.path,
-                                project_id: project.path, id: note.id, name: "thumbsup")
+      post(:toggle_award_emoji, request_params.merge(name: "thumbsup"))
 
       expect do
-        post(:toggle_award_emoji, namespace_id: project.namespace.path,
-                                  project_id: project.path, id: note.id, name: "thumbsup")
+        post(:toggle_award_emoji, request_params.merge(name: "thumbsup"))
       end.to change { AwardEmoji.count }.by(-1)
 
       expect(response).to have_http_status(200)
     end
   end
 
-  describe 'POST resolve' do
-    before do
-      sign_in user
-    end
+  describe "resolving and unresolving" do
+    let(:merge_request) { create(:merge_request, source_project: project) }
+    let(:note) { create(:diff_note_on_merge_request, noteable: merge_request, project: project) }
 
-    context "when the user is not authorized to resolve the note" do
-      it "returns status 404" do
-        post :resolve, namespace_id: project.namespace.path, project_id: project.path, id: note.id
-
-        expect(response).to have_http_status(404)
-      end
-    end
-
-    context "when the user is authorized to resolve the note" do
+    describe 'POST resolve' do
       before do
-        project.team << [user, :developer]
+        sign_in user
       end
 
-      context "when the note is not resolvable" do
+      context "when the user is not authorized to resolve the note" do
         it "returns status 404" do
-          post :resolve, namespace_id: project.namespace.path, project_id: project.path, id: note.id
+          post :resolve, request_params
 
           expect(response).to have_http_status(404)
         end
       end
 
-      context "when the note is resolvable" do
-        let(:merge_request) { create(:merge_request, source_project: project) }
-        let(:note) { create(:diff_note_on_merge_request, noteable: merge_request, project: project) }
-
-        it "resolves the note" do
-          expect_any_instance_of(DiffNote).to receive(:resolve!).with(user)
-
-          post :resolve, namespace_id: project.namespace.path, project_id: project.path, id: note.id
+      context "when the user is authorized to resolve the note" do
+        before do
+          project.team << [user, :developer]
         end
 
-        it "checks whether all notes are resolved" do
-          expect_any_instance_of(MergeRequests::AllDiscussionsResolvedService).to receive(:execute).with(merge_request)
+        context "when the note is not resolvable" do
+          before do
+            note.update(system: true)
+          end
 
-          post :resolve, namespace_id: project.namespace.path, project_id: project.path, id: note.id
+          it "returns status 404" do
+            post :resolve, request_params
+
+            expect(response).to have_http_status(404)
+          end
         end
 
-        it "returns the name of the resolving user" do
-          post :resolve, namespace_id: project.namespace.path, project_id: project.path, id: note.id
+        context "when the note is resolvable" do
+          it "resolves the note" do
+            post :resolve, request_params
 
-          expect(JSON.parse(response.body)["resolved_by"]).to eq(user.name)
-        end
+            expect(note.reload.resolved?).to be true
+            expect(note.reload.resolved_by).to eq(user)
+          end
 
-        it "returns status 200" do
-          post :resolve, namespace_id: project.namespace.path, project_id: project.path, id: note.id
+          it "sends notifications if all discussions are resolved" do
+            expect_any_instance_of(MergeRequests::ResolvedDiscussionNotificationService).to receive(:execute).with(merge_request)
 
-          expect(response).to have_http_status(200)
-        end
-      end
-    end
-  end
+            post :resolve, request_params
+          end
 
-  describe 'DELETE unresolve' do
-    before do
-      sign_in user
-    end
+          it "returns the name of the resolving user" do
+            post :resolve, request_params
+
+            expect(JSON.parse(response.body)["resolved_by"]).to eq(user.name)
+          end
 
-    context "when the user is not authorized to resolve the note" do
-      it "returns status 404" do
-        delete :unresolve, namespace_id: project.namespace.path, project_id: project.path, id: note.id
+          it "returns status 200" do
+            post :resolve, request_params
 
-        expect(response).to have_http_status(404)
+            expect(response).to have_http_status(200)
+          end
+        end
       end
     end
 
-    context "when the user is authorized to resolve the note" do
+    describe 'DELETE unresolve' do
       before do
-        project.team << [user, :developer]
+        sign_in user
+
+        note.resolve!(user)
       end
 
-      context "when the note is not resolvable" do
+      context "when the user is not authorized to resolve the note" do
         it "returns status 404" do
-          delete :unresolve, namespace_id: project.namespace.path, project_id: project.path, id: note.id
+          delete :unresolve, request_params
 
           expect(response).to have_http_status(404)
         end
       end
 
-      context "when the note is resolvable" do
-        let(:merge_request) { create(:merge_request, source_project: project) }
-        let(:note) { create(:diff_note_on_merge_request, noteable: merge_request, project: project) }
+      context "when the user is authorized to resolve the note" do
+        before do
+          project.team << [user, :developer]
+        end
+
+        context "when the note is not resolvable" do
+          before do
+            note.update(system: true)
+          end
 
-        it "unresolves the note" do
-          expect_any_instance_of(DiffNote).to receive(:unresolve!)
+          it "returns status 404" do
+            delete :unresolve, request_params
 
-          delete :unresolve, namespace_id: project.namespace.path, project_id: project.path, id: note.id
+            expect(response).to have_http_status(404)
+          end
         end
 
-        it "returns status 200" do
-          delete :unresolve, namespace_id: project.namespace.path, project_id: project.path, id: note.id
+        context "when the note is resolvable" do
+          it "unresolves the note" do
+            delete :unresolve, request_params
+
+            expect(note.reload.resolved?).to be false
+          end
+
+          it "returns status 200" do
+            delete :unresolve, request_params
 
-          expect(response).to have_http_status(200)
+            expect(response).to have_http_status(200)
+          end
         end
       end
     end
diff --git a/spec/models/diff_note_spec.rb b/spec/models/diff_note_spec.rb
index 8aee3e11a4a..6b8e6577cfb 100644
--- a/spec/models/diff_note_spec.rb
+++ b/spec/models/diff_note_spec.rb
@@ -103,7 +103,7 @@ describe DiffNote, models: true do
 
   describe "#active?" do
     context "when noteable is a commit" do
-      subject { create(:diff_note_on_commit, project: project, position: position) }
+      subject { build(:diff_note_on_commit, project: project, position: position) }
 
       it "returns true" do
         expect(subject.active?).to be true
diff --git a/spec/services/merge_requests/all_discussions_resolved_service_spec.rb b/spec/services/merge_requests/resolved_discussion_notification_service.rb
index 1103be8d8ec..7ddd812e513 100644
--- a/spec/services/merge_requests/all_discussions_resolved_service_spec.rb
+++ b/spec/services/merge_requests/resolved_discussion_notification_service.rb
@@ -1,6 +1,6 @@
 require 'spec_helper'
 
-describe MergeRequests::AllDiscussionsResolvedService, services: true do
+describe MergeRequests::ResolvedDiscussionNotificationService, services: true do
   let(:merge_request) { create(:merge_request) }
   let(:user) { create(:user) }
   let(:project) { merge_request.project }