From 41007f6d3c30a294bbf361ff900b3b19bb463291 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Mon, 15 Aug 2016 18:45:23 -0500 Subject: [PATCH] Address review feedback --- .../projects/discussions_controller.rb | 2 +- app/controllers/projects/notes_controller.rb | 4 +- app/models/diff_note.rb | 2 +- app/models/discussion.rb | 2 + ...solved_discussion_notification_service.rb} | 2 +- .../projects/discussions_controller_spec.rb | 41 +++-- .../projects/notes_controller_spec.rb | 153 ++++++++++-------- spec/models/diff_note_spec.rb | 2 +- ...solved_discussion_notification_service.rb} | 2 +- 9 files changed, 120 insertions(+), 90 deletions(-) rename app/services/merge_requests/{all_discussions_resolved_service.rb => resolved_discussion_notification_service.rb} (79%) rename spec/services/merge_requests/{all_discussions_resolved_service_spec.rb => resolved_discussion_notification_service.rb} (94%) 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 similarity index 79% rename from app/services/merge_requests/all_discussions_resolved_service.rb rename to 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) + post :resolve, request_params + 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 + + it "returns status 200" do + post :resolve, request_params + + expect(response).to have_http_status(200) + end 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 + 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) } - - 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 + context "when the user is authorized to resolve the note" do + before do + project.team << [user, :developer] 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 not resolvable" do + before do + note.update(system: true) + end - expect(response).to have_http_status(200) + it "returns status 404" do + delete :unresolve, request_params + + expect(response).to have_http_status(404) + end + end + + 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) + 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 similarity index 94% rename from spec/services/merge_requests/all_discussions_resolved_service_spec.rb rename to 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 }