diff options
author | Douwe Maan <douwe@selenight.nl> | 2017-05-24 18:10:10 +0300 |
---|---|---|
committer | Douwe Maan <douwe@selenight.nl> | 2017-05-24 18:10:10 +0300 |
commit | 0bf339f0ed2f91a805e99fc7483572ad7d22093a (patch) | |
tree | f536be0bb67775cabcc99b7540bcfd7a108bebcf | |
parent | 6e698b254ecddf23a866d9e98a885912102ccbce (diff) |
Address review
-rw-r--r-- | app/models/concerns/discussion_on_diff.rb | 8 | ||||
-rw-r--r-- | app/models/note.rb | 7 | ||||
-rw-r--r-- | db/migrate/20170521184006_add_change_position_to_notes.rb | 18 | ||||
-rw-r--r-- | lib/gitlab/diff/position_tracer.rb | 4 | ||||
-rw-r--r-- | spec/models/concerns/discussion_on_diff_spec.rb | 26 | ||||
-rw-r--r-- | spec/services/system_note_service_spec.rb | 2 |
6 files changed, 39 insertions, 26 deletions
diff --git a/app/models/concerns/discussion_on_diff.rb b/app/models/concerns/discussion_on_diff.rb index a7bdf5587b2..eee1a36ac6b 100644 --- a/app/models/concerns/discussion_on_diff.rb +++ b/app/models/concerns/discussion_on_diff.rb @@ -47,4 +47,12 @@ module DiscussionOnDiff prev_lines end + + def line_code_in_diffs(diff_refs) + if active?(diff_refs) + line_code + elsif diff_refs && created_at_diff?(diff_refs) + original_line_code + end + end end diff --git a/app/models/note.rb b/app/models/note.rb index fc8baa66b2f..60257aac93b 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -124,12 +124,7 @@ class Note < ActiveRecord::Base groups = {} diff_notes.fresh.discussions.each do |discussion| - line_code = - if discussion.active?(diff_refs) - discussion.line_code - elsif diff_refs && discussion.created_at_diff?(diff_refs) - discussion.original_line_code - end + line_code = discussion.line_code_in_diffs(diff_refs) if line_code discussions = groups[line_code] ||= [] diff --git a/db/migrate/20170521184006_add_change_position_to_notes.rb b/db/migrate/20170521184006_add_change_position_to_notes.rb index 0b199dade25..219ed1ade4c 100644 --- a/db/migrate/20170521184006_add_change_position_to_notes.rb +++ b/db/migrate/20170521184006_add_change_position_to_notes.rb @@ -7,24 +7,6 @@ class AddChangePositionToNotes < ActiveRecord::Migration # Set this constant to true if this migration requires downtime. DOWNTIME = false - # When a migration requires downtime you **must** uncomment the following - # constant and define a short and easy to understand explanation as to why the - # migration requires downtime. - # DOWNTIME_REASON = '' - - # When using the methods "add_concurrent_index", "remove_concurrent_index" or - # "add_column_with_default" you must disable the use of transactions - # as these methods can not run in an existing transaction. - # When using "add_concurrent_index" or "remove_concurrent_index" methods make sure - # that either of them is the _only_ method called in the migration, - # any other changes should go in a separate migration. - # This ensures that upon failure _only_ the index creation or removing fails - # and can be retried or reverted easily. - # - # To disable transactions uncomment the following line and remove these - # comments: - # disable_ddl_transaction! - def change add_column :notes, :change_position, :text end diff --git a/lib/gitlab/diff/position_tracer.rb b/lib/gitlab/diff/position_tracer.rb index 46f8f7e8238..dcabb5f7fe5 100644 --- a/lib/gitlab/diff/position_tracer.rb +++ b/lib/gitlab/diff/position_tracer.rb @@ -46,12 +46,14 @@ module Gitlab # Position # start_sha - ID of commit A # head_sha - ID of commit B + # base_sha - ID of base commit of A and B # old_path - path as of A (nil if file was newly created) # new_path - path as of B (nil if file was deleted) # old_line - line number as of A (nil if file was newly created) # new_line - line number as of B (nil if file was deleted) # - # We can easily update `start_sha` and `head_sha` to hold the IDs of commits C and D, + # We can easily update `start_sha` and `head_sha` to hold the IDs of + # commits C and D, and can trivially determine `base_sha` based on those, # but need to find the paths and line numbers as of C and D. # # If the file was unchanged or newly created in A->B, the path as of D can be found diff --git a/spec/models/concerns/discussion_on_diff_spec.rb b/spec/models/concerns/discussion_on_diff_spec.rb index 8571e85627c..f3e148f95f0 100644 --- a/spec/models/concerns/discussion_on_diff_spec.rb +++ b/spec/models/concerns/discussion_on_diff_spec.rb @@ -21,4 +21,30 @@ describe DiscussionOnDiff, model: true do end end end + + describe '#line_code_in_diffs' do + context 'when the discussion is active in the diff' do + let(:diff_refs) { subject.position.diff_refs } + + it 'returns the current line code' do + expect(subject.line_code_in_diffs(diff_refs)).to eq(subject.line_code) + end + end + + context 'when the discussion was created in the diff' do + let(:diff_refs) { subject.original_position.diff_refs } + + it 'returns the original line code' do + expect(subject.line_code_in_diffs(diff_refs)).to eq(subject.original_line_code) + end + end + + context 'when the discussion is unrelated to the diff' do + let(:diff_refs) { subject.project.commit(RepoHelpers.sample_commit.id).diff_refs } + + it 'returns nil' do + expect(subject.line_code_in_diffs(diff_refs)).to be_nil + end + end + end end diff --git a/spec/services/system_note_service_spec.rb b/spec/services/system_note_service_spec.rb index c737a1f82a3..5a7cfaff7fb 100644 --- a/spec/services/system_note_service_spec.rb +++ b/spec/services/system_note_service_spec.rb @@ -1049,7 +1049,7 @@ describe SystemNoteService, services: true do it_behaves_like 'a system note' do let(:expected_noteable) { discussion.first_note.noteable } - let(:action) { 'outdated' } + let(:action) { 'outdated' } end it 'creates a new note in the discussion' do |