Allow suggesting single line changes in diffs

4 files changed, 387 insertions, 0 deletions

diff --git a/spec/services/notes/update_service_spec.rb b/spec/services/notes/update_service_spec.rb
index 533dcdcd6cd..fd9bff46a06 100644
--- a/spec/services/notes/update_service_spec.rb
+++ b/spec/services/notes/update_service_spec.rb
@@ -20,6 +20,29 @@ describe Notes::UpdateService do
       @note.reload
     end
 
+    context 'suggestions' do
+      it 'refreshes note suggestions' do
+        markdown = <<-MARKDOWN.strip_heredoc
+          ```suggestion
+            foo
+          ```
+
+          ```suggestion
+            bar
+          ```
+        MARKDOWN
+
+        suggestion = create(:suggestion)
+        note = suggestion.note
+
+        expect { described_class.new(project, user, note: markdown).execute(note) }
+          .to change { note.suggestions.count }.from(1).to(2)
+
+        expect(note.suggestions.order(:relative_order).map(&:to_content))
+          .to eq(["  foo\n", "  bar\n"])
+      end
+    end
+
     context 'todos' do
       let!(:todo) { create(:todo, :assigned, user: user, project: project, target: issue, author: user2) }
 
diff --git a/spec/services/preview_markdown_service_spec.rb b/spec/services/preview_markdown_service_spec.rb
index b69977c812a..458cb8f1f31 100644
--- a/spec/services/preview_markdown_service_spec.rb
+++ b/spec/services/preview_markdown_service_spec.rb
@@ -19,6 +19,31 @@ describe PreviewMarkdownService do
     end
   end
 
+  describe 'suggestions' do
+    let(:params) { { text: "```suggestion\nfoo\n```", preview_suggestions: preview_suggestions } }
+    let(:service) { described_class.new(project, user, params) }
+
+    context 'when preview markdown param is present' do
+      let(:preview_suggestions) { true }
+
+      it 'returns users referenced in text' do
+        result = service.execute
+
+        expect(result[:suggestions]).to eq(['foo'])
+      end
+    end
+
+    context 'when preview markdown param is not present' do
+      let(:preview_suggestions) { false }
+
+      it 'returns users referenced in text' do
+        result = service.execute
+
+        expect(result[:suggestions]).to eq([])
+      end
+    end
+  end
+
   context 'new note with quick actions' do
     let(:issue) { create(:issue, project: project) }
     let(:params) do
diff --git a/spec/services/suggestions/apply_service_spec.rb b/spec/services/suggestions/apply_service_spec.rb
new file mode 100644
index 00000000000..3a483717756
--- /dev/null
+++ b/spec/services/suggestions/apply_service_spec.rb
@@ -0,0 +1,229 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Suggestions::ApplyService do
+  include ProjectForksHelper
+
+  let(:project) { create(:project, :repository) }
+  let(:user) { create(:user, :commit_email) }
+
+  let(:position) do
+    Gitlab::Diff::Position.new(old_path: "files/ruby/popen.rb",
+                               new_path: "files/ruby/popen.rb",
+                               old_line: nil,
+                               new_line: 9,
+                               diff_refs: merge_request.diff_refs)
+  end
+
+  let(:suggestion) do
+    create(:suggestion, note: diff_note,
+                        from_content: "      raise RuntimeError, \"System commands must be given as an array of strings\"\n",
+                        to_content: "      raise RuntimeError, 'Explosion'\n      # explosion?\n")
+  end
+
+  subject { described_class.new(user) }
+
+  context 'patch is appliable' do
+    let(:expected_content) do
+      <<-CONTENT.strip_heredoc
+          require 'fileutils'
+          require 'open3'
+
+          module Popen
+            extend self
+
+            def popen(cmd, path=nil)
+              unless cmd.is_a?(Array)
+                raise RuntimeError, 'Explosion'
+                # explosion?
+              end
+
+              path ||= Dir.pwd
+
+              vars = {
+                "PWD" => path
+              }
+
+              options = {
+                chdir: path
+              }
+
+              unless File.directory?(path)
+                FileUtils.mkdir_p(path)
+              end
+
+              @cmd_output = ""
+              @cmd_status = 0
+
+              Open3.popen3(vars, *cmd, options) do |stdin, stdout, stderr, wait_thr|
+                @cmd_output << stdout.read
+                @cmd_output << stderr.read
+                @cmd_status = wait_thr.value.exitstatus
+              end
+
+              return @cmd_output, @cmd_status
+            end
+          end
+      CONTENT
+    end
+
+    context 'non-fork project' do
+      let(:merge_request) do
+        create(:merge_request, source_project: project,
+                               target_project: project)
+      end
+
+      let!(:diff_note) do
+        create(:diff_note_on_merge_request, noteable: merge_request,
+                                            position: position,
+                                            project: project)
+      end
+
+      before do
+        project.add_maintainer(user)
+      end
+
+      it 'updates the file with the new contents' do
+        subject.execute(suggestion)
+
+        blob = project.repository.blob_at_branch(merge_request.source_branch,
+                                                 position.new_path)
+
+        expect(blob.data).to eq(expected_content)
+      end
+
+      it 'returns success status' do
+        result = subject.execute(suggestion)
+
+        expect(result[:status]).to eq(:success)
+      end
+
+      it 'updates suggestion applied and commit_id columns' do
+        expect { subject.execute(suggestion) }
+          .to change(suggestion, :applied)
+          .from(false).to(true)
+          .and change(suggestion, :commit_id)
+          .from(nil)
+      end
+
+      it 'created commit has users email and name' do
+        subject.execute(suggestion)
+
+        commit = project.repository.commit
+
+        expect(user.commit_email).not_to eq(user.email)
+        expect(commit.author_email).to eq(user.commit_email)
+        expect(commit.committer_email).to eq(user.commit_email)
+        expect(commit.author_name).to eq(user.name)
+      end
+    end
+
+    context 'fork-project' do
+      let(:project) { create(:project, :public, :repository) }
+
+      let(:forked_project) do
+        fork_project_with_submodules(project, user)
+      end
+
+      let(:merge_request) do
+        create(:merge_request,
+               source_branch: 'conflict-resolvable-fork', source_project: forked_project,
+               target_branch: 'conflict-start', target_project: project)
+      end
+
+      let!(:diff_note) do
+        create(:diff_note_on_merge_request, noteable: merge_request, position: position, project: project)
+      end
+
+      before do
+        project.add_maintainer(user)
+      end
+
+      it 'updates file in the source project' do
+        expect(Files::UpdateService).to receive(:new)
+          .with(merge_request.source_project, user, anything)
+          .and_call_original
+
+        subject.execute(suggestion)
+      end
+    end
+  end
+
+  context 'no permission' do
+    let(:merge_request) do
+      create(:merge_request, source_project: project,
+                             target_project: project)
+    end
+
+    let(:diff_note) do
+      create(:diff_note_on_merge_request, noteable: merge_request,
+                                          position: position,
+                                          project: project)
+    end
+
+    context 'user cannot write in project repo' do
+      before do
+        project.add_reporter(user)
+      end
+
+      it 'returns error' do
+        result = subject.execute(suggestion)
+
+        expect(result).to eq(message: "You are not allowed to push into this branch",
+                             status: :error)
+      end
+    end
+  end
+
+  context 'patch is not appliable' do
+    let(:merge_request) do
+      create(:merge_request, source_project: project,
+                             target_project: project)
+    end
+
+    let(:diff_note) do
+      create(:diff_note_on_merge_request, noteable: merge_request,
+                                          position: position,
+                                          project: project)
+    end
+
+    before do
+      project.add_maintainer(user)
+    end
+
+    context 'suggestion was already applied' do
+      it 'returns success status' do
+        result = subject.execute(suggestion)
+
+        expect(result[:status]).to eq(:success)
+      end
+    end
+
+    context 'note is outdated' do
+      before do
+        allow(diff_note).to receive(:active?) { false }
+      end
+
+      it 'returns error message' do
+        result = subject.execute(suggestion)
+
+        expect(result).to eq(message: 'Suggestion is not appliable',
+                             status: :error)
+      end
+    end
+
+    context 'suggestion was already applied' do
+      before do
+        suggestion.update!(applied: true, commit_id: 'sha')
+      end
+
+      it 'returns error message' do
+        result = subject.execute(suggestion)
+
+        expect(result).to eq(message: 'Suggestion is not appliable',
+                             status: :error)
+      end
+    end
+  end
+end
diff --git a/spec/services/suggestions/create_service_spec.rb b/spec/services/suggestions/create_service_spec.rb
new file mode 100644
index 00000000000..f1142c88a69
--- /dev/null
+++ b/spec/services/suggestions/create_service_spec.rb
@@ -0,0 +1,110 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Suggestions::CreateService 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(:position) do
+    Gitlab::Diff::Position.new(old_path: "files/ruby/popen.rb",
+                               new_path: "files/ruby/popen.rb",
+                               old_line: nil,
+                               new_line: 14,
+                               diff_refs: merge_request.diff_refs)
+  end
+
+  let(:markdown) do
+    <<-MARKDOWN.strip_heredoc
+        ```suggestion
+          foo
+          bar
+        ```
+
+        ```
+          nothing
+        ```
+
+        ```suggestion
+          xpto
+          baz
+        ```
+
+        ```thing
+          this is not a suggestion, it's a thing
+        ```
+    MARKDOWN
+  end
+
+  subject { described_class.new(note) }
+
+  describe '#execute' do
+    context 'should not try to parse suggestions' do
+      context 'when not a diff note for merge requests' do
+        let(:note) do
+          create(:diff_note_on_commit, project: project_with_repo,
+                                       note: markdown)
+        end
+
+        it 'does not try to parse suggestions' do
+          expect(Banzai::SuggestionsParser).not_to receive(:parse)
+
+          subject.execute
+        end
+      end
+
+      context 'when diff note is not for text' do
+        let(:note) do
+          create(:diff_note_on_merge_request, project: project_with_repo,
+                                              noteable: merge_request,
+                                              position: position,
+                                              note: markdown)
+        end
+
+        it 'does not try to parse suggestions' do
+          allow(note).to receive(:on_text?) { false }
+
+          expect(Banzai::SuggestionsParser).not_to receive(:parse)
+
+          subject.execute
+        end
+      end
+    end
+
+    context 'should create suggestions' do
+      let(:note) do
+        create(:diff_note_on_merge_request, project: project_with_repo,
+                                            noteable: merge_request,
+                                            position: position,
+                                            note: markdown)
+      end
+
+      context 'single line suggestions' do
+        it 'persists suggestion records' do
+          expect { subject.execute }
+            .to change { note.suggestions.count }
+            .from(0)
+            .to(2)
+        end
+
+        it 'persists original from_content lines and suggested lines' do
+          subject.execute
+
+          suggestions = note.suggestions.order(:relative_order)
+
+          suggestion_1 = suggestions.first
+          suggestion_2 = suggestions.last
+
+          expect(suggestion_1).to have_attributes(from_content: "    vars = {\n",
+                                                  to_content: "  foo\n  bar\n")
+
+          expect(suggestion_2).to have_attributes(from_content: "    vars = {\n",
+                                                  to_content: "  xpto\n  baz\n")
+        end
+      end
+    end
+  end
+end