Merge branch 'new-diff-notes' into 'master'

New diff notes

Fixes #12732, #14731, #19375, #14783 

Builds on https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/4110

To do:
- [x] Get it mostly working
- [x] Validate position validity
- [x] Fix: Don’t link to `#`
- [x] Fix: Base ref can be `nil`, potentially, when the MR has an oprhan source branch => Yep, doesn’t work. We need to store a `start_id`
- [x] Optimize: Fewer duplicate `git diff` compares
- [x] Optimize: Pass paths to `PositionTracer#diff` for faster diffs
- [x] Refactor: Use `head_id` in `MergeRequest`/`MergeRequestDiff` instead of `source_sha`
- [x] Refactor: Convert existing array-based diff refs to the DiffRefs model
- [x] Tweak: Use `note_type` in `Autosave` key
- [x] Tweak: Remove `line_code: note.line_code` from `link_to_reply_discussion`
- [x] Update: `SentNotifications` and reply-by-email receiver
- [x] Update: MR diff notification email
- [x] Update: API (MR, Commit note creation and entity)
- [x] Update: GitHub importer
- [x] Address any other TODO comments
- [x] Fix: Suppress "edited 4 minutes ago"
- [x] Write tests
  - [x] `LineMapper`
  - [x] `PositionTracer`
  - [x] `Position`
  - [x] `DiffPositionUpdateService`
  - [x] `DiffNote`
  - [x] `MergeRequests::RefreshService` / `MergeRequest#update_diff_notes_positions`
- [x] Make sure commits with diff notes don't get cleaned up, since this would prevent the diff notes from being rendered (https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5062)

Future improvements:
- Display unresolved comments on files outside the diff, if the comment was added when that file _was_ part of the diff
- Allow commenting on sections between hunks, when expanding the diff using `...`
  - (We'd need to generate line code based on Position if we have it, even if it falls outside bounds of diff)
- `diff_hunk` on diff note API entity
- Show diff hunk in notification email
- Resolved line notes would have a boolean, and be inactive through `notes.any? { !active? || resolved? }`
- Multi line notes would store a number of positions, and do the right thing (™) in grouping and then rendering if the first item is multiline? => true
- Image diff notes could store x,y,width,height instead of old_line,new_line for similar grouping. Does it need a reference to say if it's on old or new? These can't have line_codes, clearly. Rendering would be interesting.
- Show commit line comments in the MR diff
- Comment on specific selected words
- Comment on file header
- Unfold top of discussion diff note
- New diff notes API for commits and MRs

/cc @rspeicher

See merge request !4101

6 files changed, 259 insertions, 27 deletions

diff --git a/spec/models/diff_note_spec.rb b/spec/models/diff_note_spec.rb
new file mode 100644
index 00000000000..af8e890ca95
--- /dev/null
+++ b/spec/models/diff_note_spec.rb
@@ -0,0 +1,191 @@
+require 'spec_helper'
+
+describe DiffNote, models: true do
+  include RepoHelpers
+
+  let(:project) { create(:project) }
+  let(:merge_request) { create(:merge_request, source_project: project) }
+  let(:commit) { project.commit(sample_commit.id) }
+
+  let(:path) { "files/ruby/popen.rb" }
+
+  let!(:position) do
+    Gitlab::Diff::Position.new(
+      old_path: path,
+      new_path: path,
+      old_line: nil,
+      new_line: 14,
+      diff_refs: merge_request.diff_refs
+    )
+  end
+
+  let!(:new_position) do
+    Gitlab::Diff::Position.new(
+      old_path: path,
+      new_path: path,
+      old_line: 16,
+      new_line: 22,
+      diff_refs: merge_request.diff_refs
+    )
+  end
+
+  subject { create(:diff_note_on_merge_request, project: project, position: position, noteable: merge_request) }
+
+  describe "#position=" do
+    context "when provided a string" do
+      it "sets the position" do
+        subject.position = new_position.to_json
+
+        expect(subject.position).to eq(new_position)
+      end
+    end
+
+    context "when provided a hash" do
+      it "sets the position" do
+        subject.position = new_position.to_h
+
+        expect(subject.position).to eq(new_position)
+      end
+    end
+
+    context "when provided a position object" do
+      it "sets the position" do
+        subject.position = new_position
+
+        expect(subject.position).to eq(new_position)
+      end
+    end
+  end
+
+  describe "#diff_file" do
+    it "returns the correct diff file" do
+      diff_file = subject.diff_file
+
+      expect(diff_file.old_path).to eq(position.old_path)
+      expect(diff_file.new_path).to eq(position.new_path)
+      expect(diff_file.diff_refs).to eq(position.diff_refs)
+    end
+  end
+
+  describe "#diff_line" do
+    it "returns the correct diff line" do
+      diff_line = subject.diff_line
+
+      expect(diff_line.added?).to be true
+      expect(diff_line.new_line).to eq(position.new_line)
+      expect(diff_line.text).to eq("+    vars = {")
+    end
+  end
+
+  describe "#line_code" do
+    it "returns the correct line code" do
+      line_code = Gitlab::Diff::LineCode.generate(position.file_path, position.new_line, 15)
+
+      expect(subject.line_code).to eq(line_code)
+    end
+  end
+
+  describe "#for_line?" do
+    context "when provided the correct diff line" do
+      it "returns true" do
+        expect(subject.for_line?(subject.diff_line)).to be true
+      end
+    end
+
+    context "when provided a different diff line" do
+      it "returns false" do
+        some_line = subject.diff_file.diff_lines.first
+
+        expect(subject.for_line?(some_line)).to be false
+      end
+    end
+  end
+
+  describe "#active?" do
+    context "when noteable is a commit" do
+      subject { create(:diff_note_on_commit, project: project, position: position) }
+
+      it "returns true" do
+        expect(subject.active?).to be true
+      end
+    end
+
+    context "when noteable is a merge request" do
+      context "when the merge request's diff refs match that of the diff note" do
+        it "returns true" do
+          expect(subject.active?).to be true
+        end
+      end
+
+      context "when the merge request's diff refs don't match that of the diff note" do
+        before do
+          allow(subject.noteable).to receive(:diff_refs).and_return(commit.diff_refs)
+        end
+
+        it "returns false" do
+          expect(subject.active?).to be false
+        end
+      end
+    end
+  end
+
+  describe "creation" do
+    describe "updating of position" do
+      context "when noteable is a commit" do
+        let(:diff_note) { create(:diff_note_on_commit, project: project, position: position) }
+
+        it "doesn't use the DiffPositionUpdateService" do
+          expect(Notes::DiffPositionUpdateService).not_to receive(:new)
+
+          diff_note
+        end
+
+        it "doesn't update the position" do
+          diff_note
+
+          expect(diff_note.original_position).to eq(position)
+          expect(diff_note.position).to eq(position)
+        end
+      end
+
+      context "when noteable is a merge request" do
+        let(:diff_note) { create(:diff_note_on_merge_request, project: project, position: position, noteable: merge_request) }
+
+        context "when the note is active" do
+          it "doesn't use the DiffPositionUpdateService" do
+            expect(Notes::DiffPositionUpdateService).not_to receive(:new)
+
+            diff_note
+          end
+
+          it "doesn't update the position" do
+            diff_note
+
+            expect(diff_note.original_position).to eq(position)
+            expect(diff_note.position).to eq(position)
+          end
+        end
+
+        context "when the note is outdated" do
+          before do
+            allow(merge_request).to receive(:diff_refs).and_return(commit.diff_refs)
+          end
+
+          it "uses the DiffPositionUpdateService" do
+            service = instance_double("Notes::DiffPositionUpdateService")
+            expect(Notes::DiffPositionUpdateService).to receive(:new).with(
+              project,
+              nil,
+              old_diff_refs: position.diff_refs,
+              new_diff_refs: commit.diff_refs,
+              paths: [path]
+            ).and_return(service)
+            expect(service).to receive(:execute)
+
+            diff_note
+          end
+        end
+      end
+    end
+  end
+end
diff --git a/spec/models/event_spec.rb b/spec/models/event_spec.rb
index 6ac19756f15..b5d0d79e14e 100644
--- a/spec/models/event_spec.rb
+++ b/spec/models/event_spec.rb
@@ -56,7 +56,7 @@ describe Event, models: true do
     end
 
     context 'merge request diff note event' do
-      let(:target) { create(:note_on_merge_request_diff) }
+      let(:target) { create(:legacy_diff_note_on_merge_request) }
 
       it { is_expected.to be_note }
     end
@@ -132,7 +132,7 @@ describe Event, models: true do
     context 'merge request diff note event' do
       let(:project) { create(:project, :public) }
       let(:merge_request) { create(:merge_request, source_project: project, author: author, assignee: assignee) }
-      let(:note_on_merge_request) { create(:note_on_merge_request_diff, noteable: merge_request, project: project) }
+      let(:note_on_merge_request) { create(:legacy_diff_note_on_merge_request, noteable: merge_request, project: project) }
       let(:target) { note_on_merge_request }
 
       it { expect(event.visible_to_user?(non_member)).to eq true }
diff --git a/spec/models/legacy_diff_note_spec.rb b/spec/models/legacy_diff_note_spec.rb
index b2d06853886..d64d89edbd3 100644
--- a/spec/models/legacy_diff_note_spec.rb
+++ b/spec/models/legacy_diff_note_spec.rb
@@ -2,7 +2,7 @@ require 'spec_helper'
 
 describe LegacyDiffNote, models: true do
   describe "Commit diff line notes" do
-    let!(:note) { create(:note_on_commit_diff, note: "+1 from me") }
+    let!(:note) { create(:legacy_diff_note_on_commit, note: "+1 from me") }
     let!(:commit) { note.noteable }
 
     it "should save a valid note" do
@@ -17,7 +17,7 @@ describe LegacyDiffNote, models: true do
 
   describe '#active?' do
     it 'is always true when the note has no associated diff' do
-      note = build(:note_on_merge_request_diff)
+      note = build(:legacy_diff_note_on_merge_request)
 
       expect(note).to receive(:diff).and_return(nil)
 
@@ -25,7 +25,7 @@ describe LegacyDiffNote, models: true do
     end
 
     it 'is never true when the note has no noteable associated' do
-      note = build(:note_on_merge_request_diff)
+      note = build(:legacy_diff_note_on_merge_request)
 
       expect(note).to receive(:diff).and_return(double)
       expect(note).to receive(:noteable).and_return(nil)
@@ -34,7 +34,7 @@ describe LegacyDiffNote, models: true do
     end
 
     it 'returns the memoized value if defined' do
-      note = build(:note_on_merge_request_diff)
+      note = build(:legacy_diff_note_on_merge_request)
 
       note.instance_variable_set(:@active, 'foo')
       expect(note).not_to receive(:find_noteable_diff)
@@ -45,7 +45,7 @@ describe LegacyDiffNote, models: true do
     context 'for a merge request noteable' do
       it 'is false when noteable has no matching diff' do
         merge = build_stubbed(:merge_request, :simple)
-        note = build(:note_on_merge_request_diff, noteable: merge)
+        note = build(:legacy_diff_note_on_merge_request, noteable: merge)
 
         allow(note).to receive(:diff).and_return(double)
         expect(note).to receive(:find_noteable_diff).and_return(nil)
@@ -63,9 +63,9 @@ describe LegacyDiffNote, models: true do
         code = Gitlab::Diff::LineCode.generate(diff.new_path, line.new_pos, line.old_pos)
 
         # We're persisting in order to trigger the set_diff callback
-        note = create(:note_on_merge_request_diff, noteable: merge,
-                                                   line_code: code,
-                                                   project: merge.source_project)
+        note = create(:legacy_diff_note_on_merge_request,  noteable: merge,
+                                                           line_code: code,
+                                                           project: merge.source_project)
 
         # Make sure we don't get a false positive from a guard clause
         expect(note).to receive(:find_noteable_diff).and_call_original
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index ceb4d64698f..a4b6ff8f8ad 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -1,6 +1,8 @@
 require 'spec_helper'
 
 describe MergeRequest, models: true do
+  include RepoHelpers
+
   subject { create(:merge_request) }
 
   describe 'associations' do
@@ -62,7 +64,7 @@ describe MergeRequest, models: true do
     end
   end
 
-  describe '#target_sha' do
+  describe '#target_branch_sha' do
     context 'when the target branch does not exist anymore' do
       let(:project) { create(:project) }
 
@@ -73,32 +75,32 @@ describe MergeRequest, models: true do
       end
 
       it 'returns nil' do
-        expect(subject.target_sha).to be_nil
+        expect(subject.target_branch_sha).to be_nil
       end
     end
   end
 
-  describe '#source_sha' do
+  describe '#source_branch_sha' do
     let(:last_branch_commit) { subject.source_project.repository.commit(subject.source_branch) }
 
     context 'with diffs' do
       subject { create(:merge_request, :with_diffs) }
       it 'returns the sha of the source branch last commit' do
-        expect(subject.source_sha).to eq(last_branch_commit.sha)
+        expect(subject.source_branch_sha).to eq(last_branch_commit.sha)
       end
     end
 
     context 'without diffs' do
       subject { create(:merge_request, :without_diffs) }
       it 'returns the sha of the source branch last commit' do
-        expect(subject.source_sha).to eq(last_branch_commit.sha)
+        expect(subject.source_branch_sha).to eq(last_branch_commit.sha)
       end
     end
 
     context 'when the merge request is being created' do
       subject { build(:merge_request, source_branch: nil, compare_commits: []) }
       it 'returns nil' do
-        expect(subject.source_sha).to be_nil
+        expect(subject.source_branch_sha).to be_nil
       end
     end
   end
@@ -252,12 +254,14 @@ describe MergeRequest, models: true do
     end
 
     it "can be removed if the last commit is the head of the source branch" do
-      allow(subject.source_project).to receive(:commit).and_return(subject.last_commit)
+      allow(subject.source_project).to receive(:commit).and_return(subject.diff_head_commit)
 
       expect(subject.can_remove_source_branch?(user)).to be_truthy
     end
 
     it "cannot be removed if the last commit is not also the head of the source branch" do
+      subject.source_branch = "lfs"
+
       expect(subject.can_remove_source_branch?(user)).to be_falsey
     end
   end
@@ -363,7 +367,7 @@ describe MergeRequest, models: true do
           and_return(2)
 
         subject.diverged_commits_count
-        allow(subject).to receive(:source_sha).and_return('123abc')
+        allow(subject).to receive(:source_branch_sha).and_return('123abc')
         subject.diverged_commits_count
       end
 
@@ -373,7 +377,7 @@ describe MergeRequest, models: true do
           and_return(2)
 
         subject.diverged_commits_count
-        allow(subject).to receive(:target_sha).and_return('123abc')
+        allow(subject).to receive(:target_branch_sha).and_return('123abc')
         subject.diverged_commits_count
       end
     end
@@ -392,11 +396,10 @@ describe MergeRequest, models: true do
 
   describe '#pipeline' do
     describe 'when the source project exists' do
-      it 'returns the latest commit' do
-        commit   = double(:commit, id: '123abc')
+      it 'returns the latest pipeline' do
         pipeline = double(:ci_pipeline, ref: 'master')
 
-        allow(subject).to receive(:last_commit).and_return(commit)
+        allow(subject).to receive(:diff_head_sha).and_return('123abc')
 
         expect(subject.source_project).to receive(:pipeline).
           with('123abc', 'master').
@@ -464,7 +467,7 @@ describe MergeRequest, models: true do
     context 'when it is not broken and has no conflicts' do
       it 'is marked as mergeable' do
         allow(subject).to receive(:broken?) { false }
-        allow(project.repository).to receive(:can_be_merged?) { true }
+        allow(project.repository).to receive(:can_be_merged?).and_return(true)
 
         expect { subject.check_if_can_be_merged }.to change { subject.merge_status }.to('can_be_merged')
       end
@@ -481,7 +484,7 @@ describe MergeRequest, models: true do
     context 'when it has conflicts' do
       before do
         allow(subject).to receive(:broken?) { false }
-        allow(project.repository).to receive(:can_be_merged?) { false }
+        allow(project.repository).to receive(:can_be_merged?).and_return(false)
       end
 
       it 'becomes unmergeable' do
@@ -608,4 +611,42 @@ describe MergeRequest, models: true do
       end
     end
   end
+
+  describe "#reload_diff" do
+    let(:note) { create(:diff_note_on_merge_request, project: subject.project, noteable: subject) }
+
+    let(:commit) { subject.project.commit(sample_commit.id) }
+
+    it "reloads the diff content" do
+      expect(subject.merge_request_diff).to receive(:reload_content)
+
+      subject.reload_diff
+    end
+
+    it "updates diff note positions" do
+      old_diff_refs = subject.diff_refs
+
+      merge_request_diff = subject.merge_request_diff
+
+      # Update merge_request_diff so that #diff_refs will return commit.diff_refs
+      allow(merge_request_diff).to receive(:reload_content) do
+        merge_request_diff.base_commit_sha = commit.parent_id
+        merge_request_diff.start_commit_sha = commit.parent_id
+        merge_request_diff.head_commit_sha = commit.sha
+      end
+
+      expect(Notes::DiffPositionUpdateService).to receive(:new).with(
+        subject.project,
+        nil,
+        old_diff_refs: old_diff_refs,
+        new_diff_refs: commit.diff_refs,
+        paths: note.position.paths
+      ).and_call_original
+      expect_any_instance_of(Notes::DiffPositionUpdateService).to receive(:execute).with(note)
+
+      expect_any_instance_of(DiffNote).to receive(:save).once
+
+      subject.reload_diff
+    end
+  end
 end
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index 2e89d6de3a2..1b434a726dc 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -312,7 +312,7 @@ describe Project, models: true do
     it 'should update merge request commits with new one if pushed to source branch' do
       project.update_merge_requests(prev_commit_id, commit_id, "refs/heads/#{merge_request.source_branch}", key.user)
       merge_request.reload
-      expect(merge_request.last_commit.id).to eq(commit_id)
+      expect(merge_request.diff_head_sha).to eq(commit_id)
     end
   end
 
diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb
index 7975fc64e59..24e49c8def3 100644
--- a/spec/models/repository_spec.rb
+++ b/spec/models/repository_spec.rb
@@ -12,8 +12,8 @@ describe Repository, models: true do
   end
   let(:merge_commit) do
     source_sha = repository.find_branch('feature').target
-    merge_commit_id = repository.merge(user, source_sha, 'master', commit_options)
-    repository.commit(merge_commit_id)
+    merge_commit_sha = repository.merge(user, source_sha, 'master', commit_options)
+    repository.commit(merge_commit_sha)
   end
 
   describe :branch_names_contains do