diff --git a/app/assets/javascripts/diffs/store/mutations.js b/app/assets/javascripts/diffs/store/mutations.js index bb4c80b5759829dbe0e5dc474d60441a19c62b68..8251a321c3e898153826c538ff0f92879fc96908 100644 --- a/app/assets/javascripts/diffs/store/mutations.js +++ b/app/assets/javascripts/diffs/store/mutations.js @@ -174,15 +174,18 @@ export default { [types.SET_LINE_DISCUSSIONS_FOR_FILE](state, { discussion, diffPositionByLineCode, hash }) { const { latestDiff } = state; - const discussionLineCode = discussion.line_code; + const discussionLineCodes = [discussion.line_code, ...discussion.line_codes]; const fileHash = discussion.diff_file.file_hash; const lineCheck = line => - line.line_code === discussionLineCode && - isDiscussionApplicableToLine({ - discussion, - diffPosition: diffPositionByLineCode[line.line_code], - latestDiff, - }); + discussionLineCodes.some( + discussionLineCode => + line.line_code === discussionLineCode && + isDiscussionApplicableToLine({ + discussion, + diffPosition: diffPositionByLineCode[line.line_code], + latestDiff, + }), + ); const mapDiscussions = (line, extraCheck = () => true) => ({ ...line, discussions: extraCheck() diff --git a/app/assets/javascripts/diffs/store/utils.js b/app/assets/javascripts/diffs/store/utils.js index 9c788e283b9fdbe5447b5effc8bf140f4f632d6e..5054666171ae2a0a7bd55ffeeb1bc3ff8778bd7d 100644 --- a/app/assets/javascripts/diffs/store/utils.js +++ b/app/assets/javascripts/diffs/store/utils.js @@ -439,10 +439,13 @@ export function isDiscussionApplicableToLine({ discussion, diffPosition, latestD const { line_code, ...diffPositionCopy } = diffPosition; if (discussion.original_position && discussion.position) { - const originalRefs = discussion.original_position; - const refs = discussion.position; + const discussionPositions = [ + discussion.original_position, + discussion.position, + ...discussion.positions, + ]; - return isEqual(refs, diffPositionCopy) || isEqual(originalRefs, diffPositionCopy); + return discussionPositions.some(position => isEqual(position, diffPositionCopy)); } // eslint-disable-next-line diff --git a/app/models/diff_discussion.rb b/app/models/diff_discussion.rb index 93e3ebf7896845b259a69388c3f2fc301cd92c52..00d4b3d8bd6175b71672344758eb1ad5badec386 100644 --- a/app/models/diff_discussion.rb +++ b/app/models/diff_discussion.rb @@ -11,6 +11,7 @@ def self.note_class end delegate :position, + :diff_note_positions, :original_position, :change_position, :on_text?, diff --git a/app/models/diff_note_position.rb b/app/models/diff_note_position.rb new file mode 100644 index 0000000000000000000000000000000000000000..26264f49a8b6c82dcabf79369ce08360b6ee62b9 --- /dev/null +++ b/app/models/diff_note_position.rb @@ -0,0 +1,45 @@ +# frozen_string_literal: true + +class DiffNotePosition < ApplicationRecord + belongs_to :note + + self.inheritance_column = :_type_disabled + + enum position_type: { + text: 0, + image: 1 + } + + enum type: { + merge_ref_head: 0 + } + + def position + Gitlab::Diff::Position.new( + old_path: old_path, + new_path: new_path, + old_line: old_line, + new_line: new_line, + position_type: position_type, + diff_refs: Gitlab::Diff::DiffRefs.new( + base_sha: base_sha, + start_sha: start_sha, + head_sha: head_sha + ) + ) + end + + def position=(position) + assign_attributes(position.to_h) + end + + def self.create_or_update_by(type, params) + safe_ensure_unique do + diff_note_position = find_or_initialize_by(type: type) + + diff_note_position.assign_attributes(params) + + diff_note_position.save! + end + end +end diff --git a/app/models/note.rb b/app/models/note.rb index 251a75e60251d595d938924871d8c1f63238c52b..e6ad7c2227f0bff19300f9c535f6f3de73865946 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -83,6 +83,7 @@ def value?(val) has_many :events, as: :target, dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent has_one :system_note_metadata has_one :note_diff_file, inverse_of: :diff_note, foreign_key: :diff_note_id + has_many :diff_note_positions delegate :gfm_reference, :local_reference, to: :noteable delegate :name, to: :project, prefix: true diff --git a/app/serializers/discussion_entity.rb b/app/serializers/discussion_entity.rb index e302672042e571abfef2aeff278beaf193c55ffe..a4d904fd9c5721b073d3b42256105addc9b057e2 100644 --- a/app/serializers/discussion_entity.rb +++ b/app/serializers/discussion_entity.rb @@ -12,6 +12,14 @@ class DiscussionEntity < Grape::Entity expose :active?, as: :active, if: -> (d, _) { d.diff_discussion? } expose :project_id + expose :positions, if: -> (d, _) { d.diff_discussion? && !d.legacy_diff_discussion? } do |discussion| + discussion.diff_note_positions.map(&:position) + end + + expose :line_codes, if: -> (d, _) { d.diff_discussion? && !d.legacy_diff_discussion? } do |discussion| + discussion.diff_note_positions.map(&:line_code) + end + expose :notes do |discussion, opts| request.note_entity.represent(discussion.notes, opts) end diff --git a/app/services/discussions/capture_diff_positions_service.rb b/app/services/discussions/capture_diff_positions_service.rb new file mode 100644 index 0000000000000000000000000000000000000000..e7f933d8d84d6aec0d6ea841aebbb836ff7ec4b6 --- /dev/null +++ b/app/services/discussions/capture_diff_positions_service.rb @@ -0,0 +1,70 @@ +# frozen_string_literal: true + +module Discussions + class CaptureDiffPositionsService < BaseService + def execute(merge_request, params) + old_diff_refs, new_diff_refs = build_diff_refs(merge_request, params) + + return unless old_diff_refs && new_diff_refs + + discussions, paths = build_discussions(merge_request, old_diff_refs) + + return if discussions.empty? + + tracer = build_tracer(old_diff_refs, new_diff_refs, paths) + + discussions.each do |discussion| + capture_position_for_discussion(tracer, discussion) + end + end + + private + + def capture_position_for_discussion(tracer, discussion) + result = tracer.trace(discussion.position) + return unless result + + position = result[:position] + + # Currently position data is copied across all notes of a discussion + # It makes sense to store a position only for the first note instead + # Within the newly introduced table we can start doing just that + note = discussion.notes.first + note.diff_note_positions.create_or_update_by(:merge_ref_head, + position: position, + line_code: position.line_code(project.repository)) + end + + def build_diff_refs(merge_request, params) + return unless params + return unless merge_request.has_complete_diff_refs? + + old_diff_refs = merge_request.diff_refs + new_diff_refs = Gitlab::Diff::DiffRefs.new( + base_sha: params[:source_id], + start_sha: params[:target_id], + head_sha: params[:commit_id]) + + return if new_diff_refs == old_diff_refs + + [old_diff_refs, new_diff_refs] + end + + def build_discussions(merge_request, diff_refs) + active_diff_discussions = merge_request.notes.new_diff_notes.discussions.select do |discussion| + discussion.active?(diff_refs) + end + paths = active_diff_discussions.flat_map { |n| n.diff_file.paths }.uniq + + [active_diff_discussions, paths] + end + + def build_tracer(old_diff_refs, new_diff_refs, paths) + Gitlab::Diff::PositionTracer.new( + project: project, + old_diff_refs: old_diff_refs, + new_diff_refs: new_diff_refs, + paths: paths) + end + end +end diff --git a/app/services/merge_requests/mergeability_check_service.rb b/app/services/merge_requests/mergeability_check_service.rb index 5b79e4d01f2fc618c1b2b2537bd2c3e47f250854..88e6090af4d952e4bcfd34acb531988be76f7201 100644 --- a/app/services/merge_requests/mergeability_check_service.rb +++ b/app/services/merge_requests/mergeability_check_service.rb @@ -100,17 +100,19 @@ def payload end def merge_ref_head_payload - commit = merge_request.merge_ref_head + strong_memoize(:merge_ref_head_payload) do + commit = merge_request.merge_ref_head - return unless commit + next unless commit - target_id, source_id = commit.parent_ids + target_id, source_id = commit.parent_ids - { - commit_id: commit.id, - source_id: source_id, - target_id: target_id - } + { + commit_id: commit.id, + source_id: source_id, + target_id: target_id + } + end end def update_merge_status @@ -118,11 +120,16 @@ def update_merge_status if can_git_merge? && merge_to_ref merge_request.mark_as_mergeable + update_diff_discussion_positions! else merge_request.mark_as_unmergeable end end + def update_diff_discussion_positions! + Discussions::CaptureDiffPositionsService.new(project).execute(merge_request, merge_ref_head_payload) + end + def recheck! if !merge_request.recheck_merge_status? && outdated_merge_ref? merge_request.mark_as_unchecked diff --git a/db/migrate/20200326122700_create_diff_note_positions.rb b/db/migrate/20200326122700_create_diff_note_positions.rb new file mode 100644 index 0000000000000000000000000000000000000000..06de88d65f44ab7ea35c91d3dc5314454719e0fd --- /dev/null +++ b/db/migrate/20200326122700_create_diff_note_positions.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +class CreateDiffNotePositions < ActiveRecord::Migration[6.0] + DOWNTIME = false + + def change + create_table :diff_note_positions do |t| + t.references :note, foreign_key: { on_delete: :cascade }, null: false + t.integer :old_line + t.integer :new_line + t.integer :position_type, limit: 2, null: false + t.integer :type, limit: 2, null: false + t.string :line_code, limit: 255, null: false + t.binary :base_sha, null: false + t.binary :start_sha, null: false + t.binary :head_sha, null: false + t.text :old_path, null: false + t.text :new_path, null: false + end + end +end diff --git a/db/structure.sql b/db/structure.sql index 86a931b6cb4caa8c16e93b6ee84dac3f5b69d499..724f1bc14795e393d8128fd81cae223ae5a71bfb 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -2113,6 +2113,30 @@ CREATE SEQUENCE public.design_user_mentions_id_seq ALTER SEQUENCE public.design_user_mentions_id_seq OWNED BY public.design_user_mentions.id; +CREATE TABLE public.diff_note_positions ( + id bigint NOT NULL, + note_id bigint NOT NULL, + old_line integer, + new_line integer, + position_type smallint NOT NULL, + type smallint NOT NULL, + line_code character varying(255) NOT NULL, + base_sha bytea NOT NULL, + start_sha bytea NOT NULL, + head_sha bytea NOT NULL, + old_path text NOT NULL, + new_path text NOT NULL +); + +CREATE SEQUENCE public.diff_note_positions_id_seq + START WITH 1 + INCREMENT BY 1 + NO MINVALUE + NO MAXVALUE + CACHE 1; + +ALTER SEQUENCE public.diff_note_positions_id_seq OWNED BY public.diff_note_positions.id; + CREATE TABLE public.draft_notes ( id bigint NOT NULL, merge_request_id integer NOT NULL, @@ -7034,6 +7058,8 @@ ALTER TABLE ONLY public.design_management_versions ALTER COLUMN id SET DEFAULT n ALTER TABLE ONLY public.design_user_mentions ALTER COLUMN id SET DEFAULT nextval('public.design_user_mentions_id_seq'::regclass); +ALTER TABLE ONLY public.diff_note_positions ALTER COLUMN id SET DEFAULT nextval('public.diff_note_positions_id_seq'::regclass); + ALTER TABLE ONLY public.draft_notes ALTER COLUMN id SET DEFAULT nextval('public.draft_notes_id_seq'::regclass); ALTER TABLE ONLY public.emails ALTER COLUMN id SET DEFAULT nextval('public.emails_id_seq'::regclass); @@ -7730,6 +7756,9 @@ ALTER TABLE ONLY public.design_management_versions ALTER TABLE ONLY public.design_user_mentions ADD CONSTRAINT design_user_mentions_pkey PRIMARY KEY (id); +ALTER TABLE ONLY public.diff_note_positions + ADD CONSTRAINT diff_note_positions_pkey PRIMARY KEY (id); + ALTER TABLE ONLY public.draft_notes ADD CONSTRAINT draft_notes_pkey PRIMARY KEY (id); @@ -8973,6 +9002,8 @@ CREATE UNIQUE INDEX index_design_management_versions_on_sha_and_issue_id ON publ CREATE UNIQUE INDEX index_design_user_mentions_on_note_id ON public.design_user_mentions USING btree (note_id); +CREATE INDEX index_diff_note_positions_on_note_id ON public.diff_note_positions USING btree (note_id); + CREATE INDEX index_draft_notes_on_author_id ON public.draft_notes USING btree (author_id); CREATE INDEX index_draft_notes_on_discussion_id ON public.draft_notes USING btree (discussion_id); @@ -10938,6 +10969,9 @@ ALTER TABLE ONLY public.project_statistics ALTER TABLE ONLY public.user_details ADD CONSTRAINT fk_rails_12e0b3043d FOREIGN KEY (user_id) REFERENCES public.users(id) ON DELETE CASCADE; +ALTER TABLE ONLY public.diff_note_positions + ADD CONSTRAINT fk_rails_13c7212859 FOREIGN KEY (note_id) REFERENCES public.notes(id) ON DELETE CASCADE; + ALTER TABLE ONLY public.users_security_dashboard_projects ADD CONSTRAINT fk_rails_150cd5682c FOREIGN KEY (project_id) REFERENCES public.projects(id) ON DELETE CASCADE; @@ -12919,6 +12953,7 @@ COPY "schema_migrations" (version) FROM STDIN; 20200325160952 20200325183636 20200326114443 +20200326122700 20200326124443 20200326134443 20200326135443 diff --git a/spec/factories/diff_note_positions.rb b/spec/factories/diff_note_positions.rb new file mode 100644 index 0000000000000000000000000000000000000000..add1db56a871aa4810bc3c3f5895246cd8cbfc87 --- /dev/null +++ b/spec/factories/diff_note_positions.rb @@ -0,0 +1,10 @@ +# frozen_string_literal: true + +FactoryBot.define do + factory :diff_note_position do + association :note, factory: :diff_note_on_merge_request + line_code { note.line_code } + position { note.position } + type { :merge_ref_head } + end +end diff --git a/spec/models/diff_note_position_spec.rb b/spec/models/diff_note_position_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..f56d560a7f471499d5a13675b6025c2c2737ca83 --- /dev/null +++ b/spec/models/diff_note_position_spec.rb @@ -0,0 +1,35 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe DiffNotePosition, type: :model do + describe '.create_or_update_by' do + context 'when a diff note' do + let(:note) { create(:diff_note_on_merge_request) } + let(:diff_position) { build(:diff_position) } + let(:line_code) { 'bd4b7bfff3a247ccf6e3371c41ec018a55230bcc_534_521' } + let(:diff_note_position) { note.diff_note_positions.first } + + context 'does not have a diff note position' do + it 'creates a diff note position' do + note.diff_note_positions.create_or_update_by(:merge_ref_head, line_code: line_code, position: diff_position) + + expect(diff_note_position.position).to eq(diff_position) + expect(diff_note_position.line_code).to eq(line_code) + expect(diff_note_position.position_type).to eq('text') + end + end + + context 'has a diff note position' do + it 'updates the existing diff note position' do + create(:diff_note_position, note: note) + note.diff_note_positions.create_or_update_by(:merge_ref_head, line_code: line_code, position: diff_position) + + expect(diff_note_position.position).to eq(diff_position) + expect(diff_note_position.line_code).to eq(line_code) + expect(note.diff_note_positions.size).to eq(1) + end + end + end + end +end diff --git a/spec/serializers/discussion_entity_spec.rb b/spec/serializers/discussion_entity_spec.rb index 4adf1dc5994ec07eda53ece618a635af085737e6..28b42545f7dbd70b971536df18d4277388e9ca5c 100644 --- a/spec/serializers/discussion_entity_spec.rb +++ b/spec/serializers/discussion_entity_spec.rb @@ -67,6 +67,7 @@ context 'when diff file is present' do let(:note) { create(:diff_note_on_merge_request) } + let!(:diff_note_position) { create(:diff_note_position, note: note) } it 'exposes diff file attributes' do expect(subject.keys.sort).to include( @@ -74,8 +75,12 @@ :truncated_diff_lines, :position, :line_code, + :line_codes, + :positions, :active ) + expect(subject[:line_codes]).to eq([note.line_code, diff_note_position.line_code]) + expect(subject[:positions]).to eq([note.position, diff_note_position.position]) end end end diff --git a/spec/services/discussions/capture_diff_positions_service_spec.rb b/spec/services/discussions/capture_diff_positions_service_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..1b09e9a2108a9b2b25937a88f097178503158e12 --- /dev/null +++ b/spec/services/discussions/capture_diff_positions_service_spec.rb @@ -0,0 +1,67 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Discussions::CaptureDiffPositionsService do + context 'when merge request has a discussion' do + let(:source_branch) { 'compare-with-merge-head-source' } + let(:target_branch) { 'compare-with-merge-head-target' } + let(:merge_request) { create(:merge_request, source_branch: source_branch, target_branch: target_branch) } + let(:project) { merge_request.project } + + let(:offset) { 30 } + let(:first_new_line) { 508 } + let(:second_new_line) { 521 } + + let(:service) { described_class.new(project, nil) } + + def build_position(new_line, diff_refs) + path = 'files/markdown/ruby-style-guide.md' + Gitlab::Diff::Position.new(old_path: path, new_path: path, + new_line: new_line, diff_refs: diff_refs) + end + + def note_for(new_line) + position = build_position(new_line, merge_request.diff_refs) + create(:diff_note_on_merge_request, project: project, position: position, noteable: merge_request) + end + + def verify_diff_note_position!(note, line) + id, old_line, new_line = note.line_code.split('_') + + expect(new_line).to eq(line.to_s) + expect(note.diff_note_positions.size).to eq(1) + + diff_position = note.diff_note_positions.last + diff_refs = Gitlab::Diff::DiffRefs.new( + base_sha: merge_request.target_branch_sha, + start_sha: merge_request.target_branch_sha, + head_sha: merge_request.merge_ref_head.sha) + + expect(diff_position.line_code).to eq("#{id}_#{old_line.to_i - offset}_#{new_line}") + expect(diff_position.position).to eq(build_position(new_line.to_i, diff_refs)) + end + + let!(:first_discussion_note) { note_for(first_new_line) } + let!(:second_discussion_note) { note_for(second_new_line) } + let!(:second_discussion_another_note) do + create(:diff_note_on_merge_request, + project: project, + position: second_discussion_note.position, + discussion_id: second_discussion_note.discussion_id, + noteable: merge_request) + end + + context 'and position of the discussion changed on target branch head' do + it 'diff positions are created for the first notes of the discussions' do + params = MergeRequests::MergeToRefService.new(project, merge_request.author).execute(merge_request) + service.execute(merge_request, params) + + verify_diff_note_position!(first_discussion_note, first_new_line) + verify_diff_note_position!(second_discussion_note, second_new_line) + + expect(second_discussion_another_note.diff_note_positions).to be_empty + end + end + end +end diff --git a/spec/services/merge_requests/mergeability_check_service_spec.rb b/spec/services/merge_requests/mergeability_check_service_spec.rb index 8f17e8083e39681acbdd0499762749eb25844391..1701aabd79be3bb508b9c6068ba0763816bbab62 100644 --- a/spec/services/merge_requests/mergeability_check_service_spec.rb +++ b/spec/services/merge_requests/mergeability_check_service_spec.rb @@ -33,6 +33,14 @@ expect(merge_request.merge_status).to eq('can_be_merged') end + it 'update diff discussion positions' do + expect_next_instance_of(Discussions::CaptureDiffPositionsService) do |service| + expect(service).to receive(:execute) + end + + subject + end + it 'updates the merge ref' do expect { subject }.to change(merge_request, :merge_ref_head).from(nil) end diff --git a/spec/support/helpers/test_env.rb b/spec/support/helpers/test_env.rb index 2e69c59c80a9483219be4281a767f9edd8c85b1f..b61bc909d82c85f935d8838e336ad1812db9b2a6 100644 --- a/spec/support/helpers/test_env.rb +++ b/spec/support/helpers/test_env.rb @@ -73,7 +73,9 @@ module TestEnv 'submodule_inside_folder' => 'b491b92', 'png-lfs' => 'fe42f41', 'sha-starting-with-large-number' => '8426165', - 'invalid-utf8-diff-paths' => '99e4853' + 'invalid-utf8-diff-paths' => '99e4853', + 'compare-with-merge-head-source' => 'b5f4399', + 'compare-with-merge-head-target' => '2f1e176' }.freeze # gitlab-test-fork is a fork of gitlab-fork, but we don't necessarily