Address review

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

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] ||= []

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

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

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