Persist truncated note diffs on a new table45190-create-notes-diff-files

We request Gitaly in a N+1 manner to build discussion diffs. Once the diffs are from different revisions, it's hard to make a single request to the service in order to build the whole response.
With this change we solve this problem and simplify a lot fetching this piece of info.

7 files changed, 226 insertions, 2 deletions

diff --git a/spec/lib/gitlab/diff/file_spec.rb b/spec/lib/gitlab/diff/file_spec.rb
index 0c2e18c268a..0588fe935c3 100644
--- a/spec/lib/gitlab/diff/file_spec.rb
+++ b/spec/lib/gitlab/diff/file_spec.rb
@@ -468,4 +468,58 @@ describe Gitlab::Diff::File do
       end
     end
   end
+
+  describe '#diff_hunk' do
+    let(:raw_diff) do
+      <<EOS
+@@ -6,12 +6,18 @@ module Popen
+
+   def popen(cmd, path=nil)
+     unless cmd.is_a?(Array)
+-      raise "System commands must be given as an array of strings"
++      raise RuntimeError, "System commands must be given as an array of strings"
+     end
+
+     path ||= Dir.pwd
+-    vars = { "PWD" => path }
+-    options = { chdir: path }
++
++    vars = {
++      "PWD" => path
++    }
++
++    options = {
++      chdir: path
++    }
+
+     unless File.directory?(path)
+       FileUtils.mkdir_p(path)
+@@ -19,6 +25,7 @@ module Popen
+
+     @cmd_output = ""
+     @cmd_status = 0
++
+     Open3.popen3(vars, *cmd, options) do |stdin, stdout, stderr, wait_thr|
+       @cmd_output << stdout.read
+       @cmd_output << stderr.read
+EOS
+    end
+
+    it 'returns raw diff up to given line index' do
+      allow(diff_file).to receive(:raw_diff) { raw_diff }
+      diff_line = instance_double(Gitlab::Diff::Line, index: 5)
+
+      diff_hunk = <<EOS
+@@ -6,12 +6,18 @@ module Popen
+
+   def popen(cmd, path=nil)
+     unless cmd.is_a?(Array)
+-      raise "System commands must be given as an array of strings"
++      raise RuntimeError, "System commands must be given as an array of strings"
+     end
+EOS
+
+      expect(diff_file.diff_hunk(diff_line)).to eq(diff_hunk)
+    end
+  end
 end
diff --git a/spec/lib/gitlab/import_export/all_models.yml b/spec/lib/gitlab/import_export/all_models.yml
index 8b46b04b8b5..fb5fd300dbb 100644
--- a/spec/lib/gitlab/import_export/all_models.yml
+++ b/spec/lib/gitlab/import_export/all_models.yml
@@ -35,6 +35,7 @@ notes:
 - todos
 - events
 - system_note_metadata
+- note_diff_file
 label_links:
 - target
 - label
diff --git a/spec/models/concerns/discussion_on_diff_spec.rb b/spec/models/concerns/discussion_on_diff_spec.rb
index 30572ce9332..8cd129dc851 100644
--- a/spec/models/concerns/discussion_on_diff_spec.rb
+++ b/spec/models/concerns/discussion_on_diff_spec.rb
@@ -1,7 +1,7 @@
 require 'spec_helper'
 
 describe DiscussionOnDiff do
-  subject { create(:diff_note_on_merge_request).to_discussion }
+  subject { create(:diff_note_on_merge_request, line_number: 18).to_discussion }
 
   describe "#truncated_diff_lines" do
     let(:truncated_lines) { subject.truncated_diff_lines }
diff --git a/spec/models/diff_note_spec.rb b/spec/models/diff_note_spec.rb
index fb51c0172ab..8624f0daa4d 100644
--- a/spec/models/diff_note_spec.rb
+++ b/spec/models/diff_note_spec.rb
@@ -84,7 +84,47 @@ describe DiffNote do
     end
   end
 
-  describe "#diff_file" do
+  describe '#create_diff_file callback' do
+    let(:noteable) { create(:merge_request) }
+    let(:project) { noteable.project }
+
+    context 'merge request' do
+      let!(:diff_note) { create(:diff_note_on_merge_request, project: project, noteable: noteable) }
+
+      it 'creates a diff note file' do
+        expect(diff_note.reload.note_diff_file).to be_present
+      end
+
+      it 'does not create diff note file if it is a reply' do
+        expect { create(:diff_note_on_merge_request, noteable: noteable, in_reply_to: diff_note) }
+          .not_to change(NoteDiffFile, :count)
+      end
+    end
+
+    context 'commit' do
+      let!(:diff_note) { create(:diff_note_on_commit, project: project) }
+
+      it 'creates a diff note file' do
+        expect(diff_note.reload.note_diff_file).to be_present
+      end
+
+      it 'does not create diff note file if it is a reply' do
+        expect { create(:diff_note_on_commit, in_reply_to: diff_note) }
+          .not_to change(NoteDiffFile, :count)
+      end
+    end
+  end
+
+  describe '#diff_file', :clean_gitlab_redis_shared_state do
+    context 'when note_diff_file association exists' do
+      it 'returns persisted diff file data' do
+        diff_file = subject.diff_file
+
+        expect(diff_file.diff.to_hash.with_indifferent_access)
+          .to include(subject.note_diff_file.to_hash)
+      end
+    end
+
     context 'when the discussion was created in the diff' do
       it 'returns correct diff file' do
         diff_file = subject.diff_file
@@ -115,6 +155,26 @@ describe DiffNote do
         expect(diff_file.diff_refs).to eq(position.diff_refs)
       end
     end
+
+    context 'note diff file creation enqueuing' do
+      it 'enqueues CreateNoteDiffFileWorker if it is the first note of a discussion' do
+        subject.note_diff_file.destroy!
+
+        expect(CreateNoteDiffFileWorker).to receive(:perform_async).with(subject.id)
+
+        subject.reload.diff_file
+      end
+
+      it 'does not enqueues CreateNoteDiffFileWorker if not first note of a discussion' do
+        mr = create(:merge_request)
+        diff_note = create(:diff_note_on_merge_request, project: mr.project, noteable: mr)
+        reply_diff_note = create(:diff_note_on_merge_request, in_reply_to: diff_note)
+
+        expect(CreateNoteDiffFileWorker).not_to receive(:perform_async).with(reply_diff_note.id)
+
+        reply_diff_note.reload.diff_file
+      end
+    end
   end
 
   describe "#diff_line" do
diff --git a/spec/models/note_diff_file_spec.rb b/spec/models/note_diff_file_spec.rb
new file mode 100644
index 00000000000..591c1a89748
--- /dev/null
+++ b/spec/models/note_diff_file_spec.rb
@@ -0,0 +1,11 @@
+require 'rails_helper'
+
+describe NoteDiffFile do
+  describe 'associations' do
+    it { is_expected.to belong_to(:diff_note) }
+  end
+
+  describe 'validations' do
+    it { is_expected.to validate_presence_of(:diff_note) }
+  end
+end
diff --git a/spec/services/notes/create_service_spec.rb b/spec/services/notes/create_service_spec.rb
index f5cff66de6d..2b2b983494f 100644
--- a/spec/services/notes/create_service_spec.rb
+++ b/spec/services/notes/create_service_spec.rb
@@ -57,6 +57,88 @@ describe Notes::CreateService do
       end
     end
 
+    context 'note diff file' do
+      let(:project_with_repo) { create(:project, :repository) }
+      let(:merge_request) do
+        create(:merge_request,
+               source_project: project_with_repo,
+               target_project: project_with_repo)
+      end
+      let(:line_number) { 14 }
+      let(:position) do
+        Gitlab::Diff::Position.new(old_path: "files/ruby/popen.rb",
+                                   new_path: "files/ruby/popen.rb",
+                                   old_line: nil,
+                                   new_line: line_number,
+                                   diff_refs: merge_request.diff_refs)
+      end
+      let(:previous_note) do
+        create(:diff_note_on_merge_request, noteable: merge_request, project: project_with_repo)
+      end
+
+      context 'when eligible to have a note diff file' do
+        let(:new_opts) do
+          opts.merge(in_reply_to_discussion_id: nil,
+                     type: 'DiffNote',
+                     noteable_type: 'MergeRequest',
+                     noteable_id: merge_request.id,
+                     position: position.to_h)
+        end
+
+        it 'note is associated with a note diff file' do
+          note = described_class.new(project_with_repo, user, new_opts).execute
+
+          expect(note).to be_persisted
+          expect(note.note_diff_file).to be_present
+        end
+      end
+
+      context 'when DiffNote is a reply' do
+        let(:new_opts) do
+          opts.merge(in_reply_to_discussion_id: previous_note.discussion_id,
+                     type: 'DiffNote',
+                     noteable_type: 'MergeRequest',
+                     noteable_id: merge_request.id,
+                     position: position.to_h)
+        end
+
+        it 'note is not associated with a note diff file' do
+          note = described_class.new(project_with_repo, user, new_opts).execute
+
+          expect(note).to be_persisted
+          expect(note.note_diff_file).to be_nil
+        end
+
+        context 'when DiffNote from an image' do
+          let(:image_position) do
+            Gitlab::Diff::Position.new(old_path: "files/images/6049019_460s.jpg",
+                                       new_path: "files/images/6049019_460s.jpg",
+                                       width: 100,
+                                       height: 100,
+                                       x: 1,
+                                       y: 100,
+                                       diff_refs: merge_request.diff_refs,
+                                       position_type: 'image')
+          end
+
+          let(:new_opts) do
+            opts.merge(in_reply_to_discussion_id: nil,
+                       type: 'DiffNote',
+                       noteable_type: 'MergeRequest',
+                       noteable_id: merge_request.id,
+                       position: image_position.to_h)
+          end
+
+          it 'note is not associated with a note diff file' do
+            note = described_class.new(project_with_repo, user, new_opts).execute
+
+            expect(note).to be_persisted
+            expect(note.note_diff_file).to be_nil
+          end
+        end
+      end
+    end
+
     context 'note with commands' do
       context 'as a user who can update the target' do
         context '/close, /label, /assign & /milestone' do
diff --git a/spec/workers/create_note_diff_file_worker_spec.rb b/spec/workers/create_note_diff_file_worker_spec.rb
new file mode 100644
index 00000000000..0ac946a1232
--- /dev/null
+++ b/spec/workers/create_note_diff_file_worker_spec.rb
@@ -0,0 +1,16 @@
+require 'spec_helper'
+
+describe CreateNoteDiffFileWorker do
+  describe '#perform' do
+    let(:diff_note) { create(:diff_note_on_merge_request) }
+
+    it 'creates diff file' do
+      diff_note.note_diff_file.destroy!
+
+      expect_any_instance_of(DiffNote).to receive(:create_diff_file)
+        .and_call_original
+
+      described_class.new.perform(diff_note.id)
+    end
+  end
+end