Add specs for NotesController and DiscussionsController

app/controllers/projects/discussions_controller.rb

@@ -34,7 +34,7 @@ class Projects::DiscussionsController < Projects::ApplicationController
   end
   def discussion
-    @discussion ||= @merge_request.discussions.find { |d| d.id == params[:id] } || render_404
+    @discussion ||= @merge_request.find_discussion(params[:id]) || render_404
   end
   def authorize_resolve_discussion!

app/controllers/projects/notes_controller.rb

@@ -5,7 +5,7 @@ class Projects::NotesController < Projects::ApplicationController
   before_action :authorize_read_note!
   before_action :authorize_create_note!, only: [:create]
   before_action :authorize_admin_note!, only: [:update, :destroy]
-  before_action :authorize_resolve_note!, only: [:resolve]
+  before_action :authorize_resolve_note!, only: [:resolve, :unresolve]
   before_action :find_current_user_notes, only: [:index]
   def index

app/models/merge_request.rb

@@ -424,6 +424,10 @@ class MergeRequest < ActiveRecord::Base
       discussions
   end
+  def find_discussion(discussion_id)
+    discussions.find { |d| d.id == discussion_id }
+  end
+
   def discussions_resolvable?
     discussions.any?(&:resolvable?)
   end

spec/controllers/projects/discussions_controller_spec.rb

+require 'spec_helper'
+
+describe Projects::DiscussionsController do
+  let(:user)    { create(:user) }
+  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) }
+
+  describe 'POST resolve' do
+    before do
+      sign_in user
+    end
+
+    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
+
+        expect(response).to have_http_status(404)
+      end
+    end
+
+    context "when the user is authorized to resolve the discussion" do
+      before do
+        project.team << [user, :developer]
+      end
+
+      context "when the discussion is not resolvable" do
+        before do
+          note.update(system: true)
+        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
+
+          expect(response).to have_http_status(404)
+        end
+      end
+
+      context "when the discussion is resolvable" do
+        it "resolves the discussion" do
+          expect_any_instance_of(Discussion).to receive(:resolve!).with(user)
+
+          post :resolve, namespace_id: project.namespace.path, project_id: project.path, merge_request_id: merge_request.iid, id: note.discussion_id
+        end
+
+        it "checks whether all notes are resolved" do
+          expect_any_instance_of(MergeRequests::AllDiscussionsResolvedService).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
+        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
+
+          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
+
+          expect(response).to have_http_status(200)
+        end
+      end
+    end
+  end
+
+  describe 'DELETE unresolve' do
+    before do
+      sign_in 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
+
+        expect(response).to have_http_status(404)
+      end
+    end
+
+    context "when the user is authorized to resolve the discussion" do
+      before do
+        project.team << [user, :developer]
+      end
+
+      context "when the discussion is not resolvable" do
+        before do
+          note.update(system: true)
+        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
+
+          expect(response).to have_http_status(404)
+        end
+      end
+
+      context "when the discussion is resolvable" do
+        it "unresolves the discussion" do
+          expect_any_instance_of(Discussion).to receive(:unresolve!)
+
+          delete :unresolve, namespace_id: project.namespace.path, project_id: project.path, merge_request_id: merge_request.iid, id: note.discussion_id
+        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
+
+          expect(response).to have_http_status(200)
+        end
+      end
+    end
+  end
+end

spec/controllers/projects/notes_controller_spec.rb

-require('spec_helper')
+require 'spec_helper'
 describe Projects::NotesController do
   let(:user)    { create(:user) }
@@ -6,7 +6,7 @@ describe Projects::NotesController do
   let(:issue)   { create(:issue, project: project) }
   let(:note)    { create(:note, noteable: issue, project: project) }
-  describe 'POST #toggle_award_emoji' do
+  describe 'POST toggle_award_emoji' do
     before do
       sign_in(user)
       project.team << [user, :developer]
@@ -33,4 +33,106 @@ describe Projects::NotesController do
       expect(response).to have_http_status(200)
     end
   end
+
+  describe 'POST resolve' do
+    before do
+      sign_in user
+    end
+
+    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
+      before do
+        project.team << [user, :developer]
+      end
+
+      context "when the note is not resolvable" 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 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
+        end
+
+        it "checks whether all notes are resolved" do
+          expect_any_instance_of(MergeRequests::AllDiscussionsResolvedService).to receive(:execute).with(merge_request)
+
+          post :resolve, namespace_id: project.namespace.path, project_id: project.path, id: note.id
+        end
+
+        it "returns the name of the resolving user" do
+          post :resolve, namespace_id: project.namespace.path, project_id: project.path, id: note.id
+
+          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, id: note.id
+
+          expect(response).to have_http_status(200)
+        end
+      end
+    end
+  end
+
+  describe 'DELETE unresolve' do
+    before do
+      sign_in user
+    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
+
+        expect(response).to have_http_status(404)
+      end
+    end
+
+    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
+        it "returns status 404" do
+          delete :unresolve, namespace_id: project.namespace.path, project_id: project.path, id: note.id
+
+          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 "unresolves the note" do
+          expect_any_instance_of(DiffNote).to receive(:unresolve!)
+
+          delete :unresolve, namespace_id: project.namespace.path, project_id: project.path, id: note.id
+        end
+
+        it "returns status 200" do
+          delete :unresolve, namespace_id: project.namespace.path, project_id: project.path, id: note.id
+
+          expect(response).to have_http_status(200)
+        end
+      end
+    end
+  end
 end