Merge branch '10646-create-drafts-with-commit-id-ce' into 'master'

CE port of https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/14520

See merge request gitlab-org/gitlab-ce!30284

9 files changed, 81 insertions, 8 deletions

diff --git a/app/models/concerns/diff_positionable_note.rb b/app/models/concerns/diff_positionable_note.rb
index 2d09eff0111..195d9e107c5 100644
--- a/app/models/concerns/diff_positionable_note.rb
+++ b/app/models/concerns/diff_positionable_note.rb
@@ -10,6 +10,8 @@ module DiffPositionableNote
     serialize :original_position, Gitlab::Diff::Position # rubocop:disable Cop/ActiveRecordSerialize
     serialize :position, Gitlab::Diff::Position # rubocop:disable Cop/ActiveRecordSerialize
     serialize :change_position, Gitlab::Diff::Position # rubocop:disable Cop/ActiveRecordSerialize
+
+    validate :diff_refs_match_commit, if: :for_commit?
   end
 
   %i(original_position position change_position).each do |meth|
@@ -71,4 +73,10 @@ module DiffPositionableNote
       self.position = result[:position]
     end
   end
+
+  def diff_refs_match_commit
+    return if self.original_position.diff_refs == commit&.diff_refs
+
+    errors.add(:commit_id, 'does not match the diff refs')
+  end
 end
diff --git a/app/models/diff_note.rb b/app/models/diff_note.rb
index f75c32633b1..861185dc222 100644
--- a/app/models/diff_note.rb
+++ b/app/models/diff_note.rb
@@ -20,7 +20,6 @@ class DiffNote < Note
   validates :noteable_type, inclusion: { in: -> (_note) { noteable_types } }
   validate :positions_complete
   validate :verify_supported
-  validate :diff_refs_match_commit, if: :for_commit?
 
   before_validation :set_line_code, if: :on_text?
   after_save :keep_around_commits
@@ -154,12 +153,6 @@ class DiffNote < Note
     errors.add(:position, "is invalid")
   end
 
-  def diff_refs_match_commit
-    return if self.original_position.diff_refs == self.commit.diff_refs
-
-    errors.add(:commit_id, 'does not match the diff refs')
-  end
-
   def keep_around_commits
     shas = [
       self.original_position.base_sha,
diff --git a/db/migrate/20190703043358_add_commit_id_to_draft_notes.rb b/db/migrate/20190703043358_add_commit_id_to_draft_notes.rb
new file mode 100644
index 00000000000..022400ce585
--- /dev/null
+++ b/db/migrate/20190703043358_add_commit_id_to_draft_notes.rb
@@ -0,0 +1,11 @@
+# frozen_string_literal: true
+
+class AddCommitIdToDraftNotes < ActiveRecord::Migration[5.1]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def change
+    add_column :draft_notes, :commit_id, :binary
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
index 3d229c2353b..709f9ce2541 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -1136,6 +1136,7 @@ ActiveRecord::Schema.define(version: 2019_07_31_084415) do
     t.text "position"
     t.text "original_position"
     t.text "change_position"
+    t.binary "commit_id"
     t.index ["author_id"], name: "index_draft_notes_on_author_id"
     t.index ["discussion_id"], name: "index_draft_notes_on_discussion_id"
     t.index ["merge_request_id"], name: "index_draft_notes_on_merge_request_id"
diff --git a/spec/db/schema_spec.rb b/spec/db/schema_spec.rb
index 13fad1b6dc9..232890b1bba 100644
--- a/spec/db/schema_spec.rb
+++ b/spec/db/schema_spec.rb
@@ -27,7 +27,7 @@ describe 'Database schema' do
     cluster_providers_gcp: %w[gcp_project_id operation_id],
     deploy_keys_projects: %w[deploy_key_id],
     deployments: %w[deployable_id environment_id user_id],
-    draft_notes: %w[discussion_id],
+    draft_notes: %w[discussion_id commit_id],
     emails: %w[user_id],
     events: %w[target_id],
     epics: %w[updated_by_id last_edited_by_id start_date_sourcing_milestone_id due_date_sourcing_milestone_id],
diff --git a/spec/javascripts/diffs/mock_data/diff_file.js b/spec/javascripts/diffs/mock_data/diff_file.js
index 27428197c1c..531686efff1 100644
--- a/spec/javascripts/diffs/mock_data/diff_file.js
+++ b/spec/javascripts/diffs/mock_data/diff_file.js
@@ -1,3 +1,5 @@
+// Copied to ee/spec/frontend/diffs/mock_data/diff_file.js
+
 export default {
   submodule: false,
   submodule_link: null,
diff --git a/spec/javascripts/notes/mock_data.js b/spec/javascripts/notes/mock_data.js
index 1df5cf9ef68..5f81a168498 100644
--- a/spec/javascripts/notes/mock_data.js
+++ b/spec/javascripts/notes/mock_data.js
@@ -1,3 +1,5 @@
+// Copied to ee/spec/frontend/notes/mock_data.js
+
 export const notesDataMock = {
   discussionsPath: '/gitlab-org/gitlab-ce/issues/26/discussions.json',
   lastFetchedAt: 1501862675,
diff --git a/spec/models/diff_note_spec.rb b/spec/models/diff_note_spec.rb
index d9e1fe4b165..8d7dafc523d 100644
--- a/spec/models/diff_note_spec.rb
+++ b/spec/models/diff_note_spec.rb
@@ -34,6 +34,10 @@ describe DiffNote do
 
   subject { create(:diff_note_on_merge_request, project: project, position: position, noteable: merge_request) }
 
+  describe 'validations' do
+    it_behaves_like 'a valid diff positionable note', :diff_note_on_commit
+  end
+
   describe "#position=" do
     context "when provided a string" do
       it "sets the position" do
diff --git a/spec/support/shared_examples/models/diff_positionable_note_shared_examples.rb b/spec/support/shared_examples/models/diff_positionable_note_shared_examples.rb
new file mode 100644
index 00000000000..8b298c5c974
--- /dev/null
+++ b/spec/support/shared_examples/models/diff_positionable_note_shared_examples.rb
@@ -0,0 +1,52 @@
+# frozen_string_literal: true
+
+shared_examples_for 'a valid diff positionable note' do |factory_on_commit|
+  context 'for commit' do
+    let(:project) { create(:project, :repository) }
+    let(:commit) { project.commit(sample_commit.id) }
+    let(:commit_id) { commit.id }
+    let(:diff_refs) { commit.diff_refs }
+
+    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: diff_refs
+      )
+    end
+
+    subject { build(factory_on_commit, commit_id: commit_id, position: position) }
+
+    context 'position diff refs matches commit diff refs' do
+      it 'is valid' do
+        expect(subject).to be_valid
+        expect(subject.errors).not_to have_key(:commit_id)
+      end
+    end
+
+    context 'position diff refs does not match commit diff refs' do
+      let(:diff_refs) do
+        Gitlab::Diff::DiffRefs.new(
+          base_sha: "not_existing_sha",
+          head_sha: "existing_sha"
+        )
+      end
+
+      it 'is invalid' do
+        expect(subject).to be_invalid
+        expect(subject.errors).to have_key(:commit_id)
+      end
+    end
+
+    context 'commit does not exist' do
+      let(:commit_id) { 'non-existing' }
+
+      it 'is invalid' do
+        expect(subject).to be_invalid
+        expect(subject.errors).to have_key(:commit_id)
+      end
+    end
+  end
+end