From 4a453fbb4b75212f98f2781b0ee35e3dbfdb44d2 Mon Sep 17 00:00:00 2001 From: Olaoluwa Oluro Date: Tue, 16 Dec 2025 10:07:23 +0000 Subject: [PATCH] PUT/DESTROY discussions for Rapid Diffs commits Changelog: added --- .../rapid_diffs/discussion_actions.rb | 69 +++++- app/controllers/projects/commit_controller.rb | 24 ++- config/routes/repository.rb | 4 +- .../rapid_diffs/discussion_actions_spec.rb | 48 +++++ .../projects/commit_controller_spec.rb | 201 ++++++++++++++++++ 5 files changed, 334 insertions(+), 12 deletions(-) diff --git a/app/controllers/concerns/rapid_diffs/discussion_actions.rb b/app/controllers/concerns/rapid_diffs/discussion_actions.rb index f759ffc7d51d25..96573f0a24143d 100644 --- a/app/controllers/concerns/rapid_diffs/discussion_actions.rb +++ b/app/controllers/concerns/rapid_diffs/discussion_actions.rb @@ -3,7 +3,7 @@ module RapidDiffs module DiscussionActions # rubocop:disable Gitlab/ModuleWithInstanceVariables -- Need @project as is - def create_discussions_for_resource + def create_discussion_for_resource return render_404 unless rapid_diffs_enabled? return access_denied! unless can?(current_user, :create_note, noteable) return unless valid_discussion_params? @@ -18,14 +18,40 @@ def create_discussions_for_resource discussion = note.discussion prepare_notes_for_rendering(discussion.notes) - serialized_discussion = RapidDiffs::DiscussionSerializer.new( - project: @project, - noteable: noteable, - current_user: current_user, - note_entity: RapidDiffs::NoteEntity - ).represent(discussion) + render json: { discussion: serialize_discussion(discussion) } + end + + def update_discussion_for_resource + return render_404 unless rapid_diffs_enabled? + return render json: { errors: "Note not found" }, status: :not_found unless note + return access_denied! unless can?(current_user, :admin_note, note) + + updated_note = Notes::UpdateService.new(@project, current_user, update_note_params).execute(note) + + if updated_note.destroyed? + head :gone + return + end + + if updated_note.errors.present? + render json: { errors: updated_note.errors.full_messages.to_sentence }, status: :unprocessable_entity + return + end - render json: { discussion: serialized_discussion } + discussion = updated_note.discussion + prepare_notes_for_rendering(discussion.notes) + + render json: { discussion: serialize_discussion(discussion) } + end + + def delete_discussion_for_resource + return render_404 unless rapid_diffs_enabled? + return render json: { errors: "Note not found" }, status: :not_found unless note + return access_denied! unless can?(current_user, :admin_note, note) + + Notes::DestroyService.new(@project, current_user).execute(note) if note.editable? + + head :no_content end # rubocop:enable Gitlab/ModuleWithInstanceVariables @@ -43,6 +69,33 @@ def create_note_params raise NotImplementedError, "#{self.class} must implement #create_note_params" end + def update_note_params + raise NotImplementedError, "#{self.class} must implement #update_note_params" + end + + def note + raise NotImplementedError, "#{self.class} must implement #note" + end + + # rubocop:disable Gitlab/ModuleWithInstanceVariables -- Need @project as is + def serialize_discussion(discussion) + RapidDiffs::DiscussionSerializer.new( + project: @project, + noteable: noteable, + current_user: current_user, + note_entity: RapidDiffs::NoteEntity + ).represent(discussion) + end + # rubocop:enable Gitlab/ModuleWithInstanceVariables + + def grouped_discussions + raise NotImplementedError, "#{self.class} must implement #grouped_discussions" + end + + def timeline_discussions + raise NotImplementedError, "#{self.class} must implement #timeline_discussions" + end + def valid_discussion_params? if create_note_params[:in_reply_to_discussion_id].present? validate_reply_target! diff --git a/app/controllers/projects/commit_controller.rb b/app/controllers/projects/commit_controller.rb index 13ddffaf32e390..09f2d850942025 100644 --- a/app/controllers/projects/commit_controller.rb +++ b/app/controllers/projects/commit_controller.rb @@ -21,7 +21,7 @@ class Projects::CommitController < Projects::ApplicationController before_action :define_environment, only: [:show, :rapid_diffs, :diff_for_path, :diff_files, :pipelines, :merge_requests] before_action :define_commit_box_vars, only: [:show, :pipelines, :rapid_diffs] - before_action :define_note_vars, only: [:show, :diff_for_path, :diff_files, :discussions, :create_discussions] + before_action :define_note_vars, only: [:show, :diff_for_path, :diff_files, :discussions, :create_discussion] before_action :authorize_edit_tree!, only: [:revert, :cherry_pick] before_action :rate_limit_for_expanded_diff_files, only: :diff_files @@ -66,8 +66,16 @@ def discussions render json: { discussions: serialized_discussions } end - def create_discussions - create_discussions_for_resource + def create_discussion + create_discussion_for_resource + end + + def update_discussion + update_discussion_for_resource + end + + def delete_discussion + delete_discussion_for_resource end def diff_for_path @@ -221,6 +229,16 @@ def create_note_params end end + def update_note_params + params.require(:note).permit(:note) + end + + # rubocop: disable CodeReuse/ActiveRecord + def note + @note ||= @project.notes.find_by(id: params[:note_id]) + end + # rubocop: enable CodeReuse/ActiveRecord + def enrich_note_params(create_params, in_reply_to_discussion_id) if in_reply_to_discussion_id.present? create_params[:in_reply_to_discussion_id] = in_reply_to_discussion_id diff --git a/config/routes/repository.rb b/config/routes/repository.rb index 4ad21649580456..21aa31b85d4b5e 100644 --- a/config/routes/repository.rb +++ b/config/routes/repository.rb @@ -122,7 +122,9 @@ get :diffs_stats get :diff_file get :discussions - post :discussions, to: 'commit#create_discussions' + post :discussions, to: 'commit#create_discussion' + put 'discussions/:note_id', to: 'commit#update_discussion' + delete 'discussions/:note_id', to: 'commit#delete_discussion' end end diff --git a/spec/controllers/concerns/rapid_diffs/discussion_actions_spec.rb b/spec/controllers/concerns/rapid_diffs/discussion_actions_spec.rb index d699bf6e749ad2..affe9219b8857b 100644 --- a/spec/controllers/concerns/rapid_diffs/discussion_actions_spec.rb +++ b/spec/controllers/concerns/rapid_diffs/discussion_actions_spec.rb @@ -18,6 +18,22 @@ def call_noteable def call_create_note_params create_note_params end + + def call_update_note_params + update_note_params + end + + def call_note + note + end + + def call_grouped_discussions + grouped_discussions + end + + def call_timeline_discussions + timeline_discussions + end end end @@ -44,4 +60,36 @@ def call_create_note_params end.to raise_error(NotImplementedError, /must implement #create_note_params/) end end + + describe '#update_note_params' do + it 'raises NotImplementedError' do + expect do + controller.new.call_update_note_params + end.to raise_error(NotImplementedError, /must implement #update_note_params/) + end + end + + describe '#note' do + it 'raises NotImplementedError' do + expect do + controller.new.call_note + end.to raise_error(NotImplementedError, /must implement #note/) + end + end + + describe '#grouped_discussions' do + it 'raises NotImplementedError' do + expect do + controller.new.call_grouped_discussions + end.to raise_error(NotImplementedError, /must implement #grouped_discussions/) + end + end + + describe '#timeline_discussions' do + it 'raises NotImplementedError' do + expect do + controller.new.call_timeline_discussions + end.to raise_error(NotImplementedError, /must implement #timeline_discussions/) + end + end end diff --git a/spec/requests/projects/commit_controller_spec.rb b/spec/requests/projects/commit_controller_spec.rb index 7c1b967c1a8ffa..dde66330e8d2a3 100644 --- a/spec/requests/projects/commit_controller_spec.rb +++ b/spec/requests/projects/commit_controller_spec.rb @@ -339,6 +339,207 @@ end end + describe 'PUT #update_discussion' do + let_it_be(:sha) { "913c66a37b4a45b9769037c55c2d238bd0942d2e" } + let_it_be(:commit) { project.commit_by(oid: sha) } + + let!(:note) { create(:note_on_commit, project: project, commit_id: sha, author: user) } + let(:note_id) { note.id } + + let(:discussions_path) do + discussions_namespace_project_commit_path(namespace_id: project.namespace, project_id: project, id: sha) + end + + let(:request_params) { { note: { note: 'Updated note text' } } } + let(:send_request) { put "#{discussions_path}/#{note_id}", params: request_params } + + before do + sign_in(user) + end + + context 'when feature flag is disabled' do + before do + stub_feature_flags(rapid_diffs_on_commit_show: false) + end + + it 'returns 404' do + send_request + + expect(response).to have_gitlab_http_status(:not_found) + end + end + + context 'when note does not exist' do + let(:note_id) { non_existing_record_id } + + it 'returns not found error' do + send_request + + expect(response).to have_gitlab_http_status(:not_found) + json_response = Gitlab::Json.parse(response.body) + expect(json_response['errors']).to eq('Note not found') + end + end + + context 'when user cannot admin the note' do + before do + allow(Ability).to receive(:allowed?).and_call_original + allow(Ability).to receive(:allowed?).with(user, :admin_note, note).and_return(false) + end + + it 'returns access denied' do + send_request + + expect(response).to have_gitlab_http_status(:not_found) + end + end + + context 'when updating a note successfully' do + it 'updates the note' do + expect { send_request }.not_to change { Note.count } + + expect(response).to have_gitlab_http_status(:ok) + expect(response.content_type).to eq('application/json; charset=utf-8') + + json_response = Gitlab::Json.parse(response.body) + expect(json_response).to have_key('discussion') + expect(json_response['discussion']['notes'].first['note']).to eq('Updated note text') + + note.reload + expect(note.note).to eq('Updated note text') + end + end + + context 'when updating a diff note' do + let!(:note) { create(:diff_note_on_commit, project: project, commit_id: sha, author: user) } + + it 'updates the diff note' do + expect { send_request }.not_to change { Note.count } + + expect(response).to have_gitlab_http_status(:ok) + + json_response = Gitlab::Json.parse(response.body) + expect(json_response['discussion']['diff_discussion']).to be true + expect(json_response['discussion']['notes'].first['note']).to eq('Updated note text') + + note.reload + expect(note.note).to eq('Updated note text') + end + end + + context 'when note is destroyed after update due to quick actions' do + before do + allow_next_instance_of(Notes::UpdateService) do |service| + allow(service).to receive(:execute) do |n| + allow(n).to receive(:destroyed?).and_return(true) + n + end + end + end + + it 'returns gone status' do + send_request + + expect(response).to have_gitlab_http_status(:gone) + end + end + + context 'when update fails with validation error' do + let(:request_params) { { note: { note: '' } } } + + it 'returns unprocessable entity' do + send_request + + expect(response).to have_gitlab_http_status(:unprocessable_entity) + json_response = Gitlab::Json.parse(response.body) + expect(json_response).to have_key('errors') + end + end + end + + describe 'DELETE #delete_discussion' do + let_it_be(:sha) { "913c66a37b4a45b9769037c55c2d238bd0942d2e" } + let_it_be(:commit) { project.commit_by(oid: sha) } + + let!(:note) { create(:note_on_commit, project: project, commit_id: sha, author: user) } + let(:note_id) { note.id } + + let(:discussions_path) do + discussions_namespace_project_commit_path(namespace_id: project.namespace, project_id: project, id: sha) + end + + let(:send_request) { delete "#{discussions_path}/#{note_id}" } + + before do + sign_in(user) + end + + context 'when feature flag is disabled' do + before do + stub_feature_flags(rapid_diffs_on_commit_show: false) + end + + it 'returns 404' do + send_request + + expect(response).to have_gitlab_http_status(:not_found) + end + end + + context 'when note does not exist' do + let(:note_id) { non_existing_record_id } + + it 'returns not found error' do + send_request + + expect(response).to have_gitlab_http_status(:not_found) + json_response = Gitlab::Json.parse(response.body) + expect(json_response['errors']).to eq('Note not found') + end + end + + context 'when user cannot admin the note' do + before do + allow(Ability).to receive(:allowed?).and_call_original + allow(Ability).to receive(:allowed?).with(user, :admin_note, note).and_return(false) + end + + it 'returns access denied' do + send_request + + expect(response).to have_gitlab_http_status(:not_found) + end + end + + context 'when deleting a note successfully' do + it 'deletes the note' do + expect { send_request }.to change { Note.count }.by(-1) + + expect(response).to have_gitlab_http_status(:no_content) + end + end + + context 'when deleting a diff note' do + let!(:note) { create(:diff_note_on_commit, project: project, commit_id: sha, author: user) } + + it 'deletes the diff note' do + expect { send_request }.to change { Note.count }.by(-1) + + expect(response).to have_gitlab_http_status(:no_content) + end + end + + context 'when note is a system note and is not editable' do + let!(:note) { create(:note_on_commit, :system, project: project, commit_id: sha) } + + it 'does not allows access to note' do + send_request + + expect(response).to have_gitlab_http_status(:not_found) + end + end + end + describe '#rapid_diffs' do let_it_be(:sha) { "913c66a37b4a45b9769037c55c2d238bd0942d2e" } let_it_be(:commit) { project.commit_by(oid: sha) } -- GitLab