Address review feedback
This commit is contained in:
parent
32e31d626f
commit
41007f6d3c
|
@ -9,7 +9,7 @@ class Projects::DiscussionsController < Projects::ApplicationController
|
||||||
|
|
||||||
discussion.resolve!(current_user)
|
discussion.resolve!(current_user)
|
||||||
|
|
||||||
MergeRequests::AllDiscussionsResolvedService.new(project, current_user).execute(merge_request)
|
MergeRequests::ResolvedDiscussionNotificationService.new(project, current_user).execute(merge_request)
|
||||||
|
|
||||||
render json: {
|
render json: {
|
||||||
resolved_by: discussion.resolved_by.try(:name),
|
resolved_by: discussion.resolved_by.try(:name),
|
||||||
|
|
|
@ -72,7 +72,7 @@ class Projects::NotesController < Projects::ApplicationController
|
||||||
|
|
||||||
note.resolve!(current_user)
|
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
|
discussion = note.discussion
|
||||||
|
|
||||||
|
@ -166,7 +166,7 @@ class Projects::NotesController < Projects::ApplicationController
|
||||||
}
|
}
|
||||||
|
|
||||||
if note.diff_note?
|
if note.diff_note?
|
||||||
discussion = note.as_discussion
|
discussion = note.to_discussion
|
||||||
|
|
||||||
attrs.merge!(
|
attrs.merge!(
|
||||||
diff_discussion_html: diff_discussion_html(discussion),
|
diff_discussion_html: diff_discussion_html(discussion),
|
||||||
|
|
|
@ -113,7 +113,7 @@ class DiffNote < Note
|
||||||
Discussion.new(discussion_notes)
|
Discussion.new(discussion_notes)
|
||||||
end
|
end
|
||||||
|
|
||||||
def as_discussion
|
def to_discussion
|
||||||
Discussion.new([self])
|
Discussion.new([self])
|
||||||
end
|
end
|
||||||
|
|
||||||
|
|
|
@ -58,6 +58,8 @@ class Discussion
|
||||||
first_note.discussion_id
|
first_note.discussion_id
|
||||||
end
|
end
|
||||||
|
|
||||||
|
alias_method :to_param, :id
|
||||||
|
|
||||||
def diff_discussion?
|
def diff_discussion?
|
||||||
first_note.diff_note?
|
first_note.diff_note?
|
||||||
end
|
end
|
||||||
|
|
|
@ -1,5 +1,5 @@
|
||||||
module MergeRequests
|
module MergeRequests
|
||||||
class AllDiscussionsResolvedService < MergeRequests::BaseService
|
class ResolvedDiscussionNotificationService < MergeRequests::BaseService
|
||||||
def execute(merge_request)
|
def execute(merge_request)
|
||||||
return unless merge_request.discussions_resolved?
|
return unless merge_request.discussions_resolved?
|
||||||
|
|
|
@ -5,6 +5,16 @@ describe Projects::DiscussionsController do
|
||||||
let(:project) { create(:project) }
|
let(:project) { create(:project) }
|
||||||
let(:merge_request) { create(:merge_request, source_project: project) }
|
let(:merge_request) { create(:merge_request, source_project: project) }
|
||||||
let(:note) { create(:diff_note_on_merge_request, noteable: merge_request, 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
|
describe 'POST resolve' do
|
||||||
before do
|
before do
|
||||||
|
@ -13,7 +23,7 @@ describe Projects::DiscussionsController do
|
||||||
|
|
||||||
context "when the user is not authorized to resolve the discussion" do
|
context "when the user is not authorized to resolve the discussion" do
|
||||||
it "returns status 404" 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)
|
expect(response).to have_http_status(404)
|
||||||
end
|
end
|
||||||
|
@ -30,7 +40,7 @@ describe Projects::DiscussionsController do
|
||||||
end
|
end
|
||||||
|
|
||||||
it "returns status 404" 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)
|
expect(response).to have_http_status(404)
|
||||||
end
|
end
|
||||||
|
@ -38,25 +48,26 @@ describe Projects::DiscussionsController do
|
||||||
|
|
||||||
context "when the discussion is resolvable" do
|
context "when the discussion is resolvable" do
|
||||||
it "resolves the discussion" 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
|
end
|
||||||
|
|
||||||
it "checks whether all notes are resolved" do
|
it "sends notifications if all discussions are resolved" do
|
||||||
expect_any_instance_of(MergeRequests::AllDiscussionsResolvedService).to receive(:execute).with(merge_request)
|
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
|
end
|
||||||
|
|
||||||
it "returns the name of the resolving user" do
|
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)
|
expect(JSON.parse(response.body)["resolved_by"]).to eq(user.name)
|
||||||
end
|
end
|
||||||
|
|
||||||
it "returns status 200" do
|
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)
|
expect(response).to have_http_status(200)
|
||||||
end
|
end
|
||||||
|
@ -67,11 +78,13 @@ describe Projects::DiscussionsController do
|
||||||
describe 'DELETE unresolve' do
|
describe 'DELETE unresolve' do
|
||||||
before do
|
before do
|
||||||
sign_in user
|
sign_in user
|
||||||
|
|
||||||
|
note.discussion.resolve!(user)
|
||||||
end
|
end
|
||||||
|
|
||||||
context "when the user is not authorized to resolve the discussion" do
|
context "when the user is not authorized to resolve the discussion" do
|
||||||
it "returns status 404" 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)
|
expect(response).to have_http_status(404)
|
||||||
end
|
end
|
||||||
|
@ -88,7 +101,7 @@ describe Projects::DiscussionsController do
|
||||||
end
|
end
|
||||||
|
|
||||||
it "returns status 404" 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)
|
expect(response).to have_http_status(404)
|
||||||
end
|
end
|
||||||
|
@ -96,13 +109,13 @@ describe Projects::DiscussionsController do
|
||||||
|
|
||||||
context "when the discussion is resolvable" do
|
context "when the discussion is resolvable" do
|
||||||
it "unresolves the discussion" 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
|
end
|
||||||
|
|
||||||
it "returns status 200" do
|
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)
|
expect(response).to have_http_status(200)
|
||||||
end
|
end
|
||||||
|
|
|
@ -6,6 +6,14 @@ describe Projects::NotesController do
|
||||||
let(:issue) { create(:issue, project: project) }
|
let(:issue) { create(:issue, project: project) }
|
||||||
let(:note) { create(:note, noteable: 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
|
describe 'POST toggle_award_emoji' do
|
||||||
before do
|
before do
|
||||||
sign_in(user)
|
sign_in(user)
|
||||||
|
@ -14,123 +22,130 @@ describe Projects::NotesController do
|
||||||
|
|
||||||
it "toggles the award emoji" do
|
it "toggles the award emoji" do
|
||||||
expect do
|
expect do
|
||||||
post(:toggle_award_emoji, namespace_id: project.namespace.path,
|
post(:toggle_award_emoji, request_params.merge(name: "thumbsup"))
|
||||||
project_id: project.path, id: note.id, name: "thumbsup")
|
|
||||||
end.to change { note.award_emoji.count }.by(1)
|
end.to change { note.award_emoji.count }.by(1)
|
||||||
|
|
||||||
expect(response).to have_http_status(200)
|
expect(response).to have_http_status(200)
|
||||||
end
|
end
|
||||||
|
|
||||||
it "removes the already awarded emoji" do
|
it "removes the already awarded emoji" do
|
||||||
post(:toggle_award_emoji, namespace_id: project.namespace.path,
|
post(:toggle_award_emoji, request_params.merge(name: "thumbsup"))
|
||||||
project_id: project.path, id: note.id, name: "thumbsup")
|
|
||||||
|
|
||||||
expect do
|
expect do
|
||||||
post(:toggle_award_emoji, namespace_id: project.namespace.path,
|
post(:toggle_award_emoji, request_params.merge(name: "thumbsup"))
|
||||||
project_id: project.path, id: note.id, name: "thumbsup")
|
|
||||||
end.to change { AwardEmoji.count }.by(-1)
|
end.to change { AwardEmoji.count }.by(-1)
|
||||||
|
|
||||||
expect(response).to have_http_status(200)
|
expect(response).to have_http_status(200)
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
describe 'POST resolve' do
|
describe "resolving and unresolving" do
|
||||||
before do
|
let(:merge_request) { create(:merge_request, source_project: project) }
|
||||||
sign_in user
|
let(:note) { create(:diff_note_on_merge_request, noteable: merge_request, project: project) }
|
||||||
end
|
|
||||||
|
|
||||||
context "when the user is not authorized to resolve the note" do
|
describe 'POST resolve' 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
|
before do
|
||||||
project.team << [user, :developer]
|
sign_in user
|
||||||
end
|
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
|
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)
|
expect(response).to have_http_status(404)
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
context "when the note is resolvable" do
|
context "when the user is authorized to resolve the note" do
|
||||||
let(:merge_request) { create(:merge_request, source_project: project) }
|
before do
|
||||||
let(:note) { create(:diff_note_on_merge_request, noteable: merge_request, project: project) }
|
project.team << [user, :developer]
|
||||||
|
|
||||||
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
|
end
|
||||||
|
|
||||||
it "checks whether all notes are resolved" do
|
context "when the note is not resolvable" do
|
||||||
expect_any_instance_of(MergeRequests::AllDiscussionsResolvedService).to receive(:execute).with(merge_request)
|
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
|
end
|
||||||
|
|
||||||
it "returns the name of the resolving user" do
|
context "when the note is resolvable" do
|
||||||
post :resolve, namespace_id: project.namespace.path, project_id: project.path, id: note.id
|
it "resolves the note" do
|
||||||
|
post :resolve, request_params
|
||||||
|
|
||||||
expect(JSON.parse(response.body)["resolved_by"]).to eq(user.name)
|
expect(note.reload.resolved?).to be true
|
||||||
end
|
expect(note.reload.resolved_by).to eq(user)
|
||||||
|
end
|
||||||
|
|
||||||
it "returns status 200" do
|
it "sends notifications if all discussions are resolved" do
|
||||||
post :resolve, namespace_id: project.namespace.path, project_id: project.path, id: note.id
|
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
|
||||||
end
|
end
|
||||||
end
|
|
||||||
|
|
||||||
describe 'DELETE unresolve' do
|
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
|
before do
|
||||||
project.team << [user, :developer]
|
sign_in user
|
||||||
|
|
||||||
|
note.resolve!(user)
|
||||||
end
|
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
|
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)
|
expect(response).to have_http_status(404)
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
context "when the note is resolvable" do
|
context "when the user is authorized to resolve the note" do
|
||||||
let(:merge_request) { create(:merge_request, source_project: project) }
|
before do
|
||||||
let(:note) { create(:diff_note_on_merge_request, noteable: merge_request, project: project) }
|
project.team << [user, :developer]
|
||||||
|
|
||||||
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
|
end
|
||||||
|
|
||||||
it "returns status 200" do
|
context "when the note is not resolvable" do
|
||||||
delete :unresolve, namespace_id: project.namespace.path, project_id: project.path, id: note.id
|
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
|
end
|
||||||
end
|
end
|
||||||
|
|
|
@ -103,7 +103,7 @@ describe DiffNote, models: true do
|
||||||
|
|
||||||
describe "#active?" do
|
describe "#active?" do
|
||||||
context "when noteable is a commit" 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
|
it "returns true" do
|
||||||
expect(subject.active?).to be true
|
expect(subject.active?).to be true
|
||||||
|
|
|
@ -1,6 +1,6 @@
|
||||||
require 'spec_helper'
|
require 'spec_helper'
|
||||||
|
|
||||||
describe MergeRequests::AllDiscussionsResolvedService, services: true do
|
describe MergeRequests::ResolvedDiscussionNotificationService, services: true do
|
||||||
let(:merge_request) { create(:merge_request) }
|
let(:merge_request) { create(:merge_request) }
|
||||||
let(:user) { create(:user) }
|
let(:user) { create(:user) }
|
||||||
let(:project) { merge_request.project }
|
let(:project) { merge_request.project }
|
Loading…
Reference in New Issue