From cd850732072078e83b5920199f1bb525cad6baa4 Mon Sep 17 00:00:00 2001 From: Igor Drozdov Date: Thu, 26 Mar 2020 15:50:31 +0300 Subject: [PATCH 1/3] Create a table for storing diff note positions We'll extract there notes' position and original position --- app/models/diff_note_position.rb | 33 +++++++++++++++++ ...200326122700_create_diff_note_positions.rb | 21 +++++++++++ db/structure.sql | 35 +++++++++++++++++++ spec/factories/diff_note_positions.rb | 10 ++++++ spec/models/diff_note_position_spec.rb | 15 ++++++++ 5 files changed, 114 insertions(+) create mode 100644 app/models/diff_note_position.rb create mode 100644 db/migrate/20200326122700_create_diff_note_positions.rb create mode 100644 spec/factories/diff_note_positions.rb create mode 100644 spec/models/diff_note_position_spec.rb diff --git a/app/models/diff_note_position.rb b/app/models/diff_note_position.rb new file mode 100644 index 00000000000000..b144fabd8094ef --- /dev/null +++ b/app/models/diff_note_position.rb @@ -0,0 +1,33 @@ +# frozen_string_literal: true + +class DiffNotePosition < ApplicationRecord + belongs_to :note + + 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 +end 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 00000000000000..06de88d65f44ab --- /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 86a931b6cb4caa..724f1bc14795e3 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 00000000000000..add1db56a871aa --- /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 00000000000000..d63539d03f6591 --- /dev/null +++ b/spec/models/diff_note_position_spec.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe DiffNotePosition, type: :model do + it 'has a position attribute' do + diff_position = build(:diff_position) + line_code = 'bd4b7bfff3a247ccf6e3371c41ec018a55230bcc_534_521' + diff_note_position = build(:diff_note_position, 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 -- GitLab From 918747a87a147ad5fe2a9596bdc65ae7be2850ab Mon Sep 17 00:00:00 2001 From: Igor Drozdov Date: Thu, 2 Apr 2020 04:10:23 +0300 Subject: [PATCH 2/3] Capture diff note position on mergeability check When a merge request is checked whether it's mergeable merge_ref_head is updated and we need to update diff note positions within it --- app/models/diff_note_position.rb | 12 ++++ app/models/note.rb | 1 + .../capture_diff_positions_service.rb | 70 +++++++++++++++++++ .../mergeability_check_service.rb | 23 +++--- spec/models/diff_note_position_spec.rb | 36 +++++++--- .../capture_diff_positions_service_spec.rb | 67 ++++++++++++++++++ .../mergeability_check_service_spec.rb | 8 +++ spec/support/helpers/test_env.rb | 4 +- 8 files changed, 204 insertions(+), 17 deletions(-) create mode 100644 app/services/discussions/capture_diff_positions_service.rb create mode 100644 spec/services/discussions/capture_diff_positions_service_spec.rb diff --git a/app/models/diff_note_position.rb b/app/models/diff_note_position.rb index b144fabd8094ef..26264f49a8b6c8 100644 --- a/app/models/diff_note_position.rb +++ b/app/models/diff_note_position.rb @@ -3,6 +3,8 @@ class DiffNotePosition < ApplicationRecord belongs_to :note + self.inheritance_column = :_type_disabled + enum position_type: { text: 0, image: 1 @@ -30,4 +32,14 @@ def position 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 251a75e60251d5..e6ad7c2227f0bf 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/services/discussions/capture_diff_positions_service.rb b/app/services/discussions/capture_diff_positions_service.rb new file mode 100644 index 00000000000000..e7f933d8d84d6a --- /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 5b79e4d01f2fc6..88e6090af4d952 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/spec/models/diff_note_position_spec.rb b/spec/models/diff_note_position_spec.rb index d63539d03f6591..f56d560a7f4714 100644 --- a/spec/models/diff_note_position_spec.rb +++ b/spec/models/diff_note_position_spec.rb @@ -3,13 +3,33 @@ require 'spec_helper' describe DiffNotePosition, type: :model do - it 'has a position attribute' do - diff_position = build(:diff_position) - line_code = 'bd4b7bfff3a247ccf6e3371c41ec018a55230bcc_534_521' - diff_note_position = build(:diff_note_position, 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') + 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/services/discussions/capture_diff_positions_service_spec.rb b/spec/services/discussions/capture_diff_positions_service_spec.rb new file mode 100644 index 00000000000000..1b09e9a2108a9b --- /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 8f17e8083e3968..1701aabd79be3b 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 2e69c59c80a948..b61bc909d82c85 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 -- GitLab From e1d75c1db79ce8d77a1be2ee9796e381233754af Mon Sep 17 00:00:00 2001 From: Igor Drozdov Date: Mon, 30 Mar 2020 17:19:24 +0300 Subject: [PATCH 3/3] Display comments on master(HEAD) diff version master(HEAD) may differ from master(base) version, but we still need to display comments for both versions --- app/assets/javascripts/diffs/store/mutations.js | 17 ++++++++++------- app/assets/javascripts/diffs/store/utils.js | 9 ++++++--- app/models/diff_discussion.rb | 1 + app/serializers/discussion_entity.rb | 8 ++++++++ spec/serializers/discussion_entity_spec.rb | 5 +++++ 5 files changed, 30 insertions(+), 10 deletions(-) diff --git a/app/assets/javascripts/diffs/store/mutations.js b/app/assets/javascripts/diffs/store/mutations.js index bb4c80b5759829..8251a321c3e898 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 9c788e283b9fdb..5054666171ae2a 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 93e3ebf7896845..00d4b3d8bd6175 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/serializers/discussion_entity.rb b/app/serializers/discussion_entity.rb index e302672042e571..a4d904fd9c5721 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/spec/serializers/discussion_entity_spec.rb b/spec/serializers/discussion_entity_spec.rb index 4adf1dc5994ec0..28b42545f7dbd7 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 -- GitLab