Address review

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