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

Support creating/publishing drafts with commit ID

See merge request gitlab-org/gitlab-ee!14520

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

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,

db/migrate/20190703043358_add_commit_id_to_draft_notes.rb

+# 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

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"

ee/app/assets/javascripts/notes/mixins/diff_line_note_form.js

@@ -14,6 +14,7 @@ export default {
     }),
     ...mapGetters('diffs', ['getDiffFileByHash']),
     ...mapGetters('batchComments', ['shouldRenderDraftRowInDiscussion', 'draftForDiscussion']),
+    ...mapState('diffs', ['commit']),
   },
   methods: {
     ...mapActions('diffs', ['cancelCommentForm']),
@@ -61,6 +62,11 @@ export default {
         ...this.diffFileCommentForm,
       });
 
+      const diffFileHeadSha =
+        this.commit && this.diffFile && this.diffFile.diff_refs && this.diffFile.diff_refs.head_sha;
+
+      postData.data.note.commit_id = diffFileHeadSha || null;
+
       return this.saveDraft(postData)
         .then(() => {
           if (positionType === IMAGE_DIFF_POSITION_TYPE) {

ee/app/controllers/projects/merge_requests/drafts_controller.rb

@@ -83,10 +83,16 @@ class Projects::MergeRequests::DraftsController < Projects::MergeRequests::Appli
 
   def draft_note_params
     params.require(:draft_note).permit(
+      :commit_id,
       :note,
       :position,
       :resolve_discussion
-    )
+    ).tap do |h|
+      # Old FE version will still be sending `draft_note[commit_id]` as 'undefined'.
+      # That can result to having a note linked to a commit with 'undefined' ID
+      # which is non-existent.
+      h[:commit_id] = nil if h[:commit_id] == 'undefined'
+    end
   end
 
   def prepare_notes_for_rendering(notes)

ee/app/models/draft_note.rb

@@ -3,9 +3,12 @@ class DraftNote < ApplicationRecord
   include DiffPositionableNote
   include Gitlab::Utils::StrongMemoize
   include Sortable
+  include ShaAttribute
 
   PUBLISH_ATTRS = %i(noteable_id noteable_type type note).freeze
-  DIFF_ATTRS = %i(position original_position change_position).freeze
+  DIFF_ATTRS = %i(position original_position change_position commit_id).freeze
+
+  sha_attribute :commit_id
 
   # Attribute used to store quick actions changes and users referenced.
   attr_accessor :commands_changes
@@ -44,7 +47,7 @@ class DraftNote < ApplicationRecord
   end
 
   def for_commit?
-    false
+    commit_id.present?
   end
 
   def resolvable?
@@ -112,4 +115,8 @@ class DraftNote < ApplicationRecord
       file
     end
   end
+
+  def commit
+    @commit ||= project.commit(commit_id) if commit_id.present?
+  end
 end

ee/changelogs/unreleased/10646-create-drafts-with-commit-id.yml

+---
+title: Support creating/publishing drafts with commit ID
+merge_request: 14520
+author:
+type: fixed

ee/spec/controllers/projects/merge_requests/drafts_controller_spec.rb

@@ -2,6 +2,8 @@
 require 'spec_helper'
 
 describe Projects::MergeRequests::DraftsController do
+  include RepoHelpers
+
   let(:project)       { create(:project, :repository) }
   let(:merge_request) { create(:merge_request_with_diffs, target_project: project, source_project: project) }
   let(:user)          { project.owner }
@@ -62,7 +64,7 @@ describe Projects::MergeRequests::DraftsController do
     end
 
     it 'creates draft note with position' do
-      diff_refs = project.commit(RepoHelpers.sample_commit.id).try(:diff_refs)
+      diff_refs = project.commit(sample_commit.id).try(:diff_refs)
 
       position = Gitlab::Diff::Position.new(
         old_path: "files/ruby/popen.rb",
@@ -135,6 +137,40 @@ describe Projects::MergeRequests::DraftsController do
         end.to change { DraftNote.count }.by(0)
       end
     end
+
+    context 'commit_id is present' do
+      let(:commit) { project.commit(sample_commit.id) }
+
+      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: commit.diff_refs
+        )
+      end
+
+      before do
+        create_draft_note(draft_overrides: { commit_id: commit_id, position: position.to_json })
+      end
+
+      context 'value is a commit sha' do
+        let(:commit_id) { commit.id }
+
+        it 'creates the draft note with commit ID' do
+          expect(DraftNote.last.commit_id).to eq(commit_id)
+        end
+      end
+
+      context 'value is "undefined"' do
+        let(:commit_id) { 'undefined' }
+
+        it 'creates the draft note with nil commit ID' do
+          expect(DraftNote.last.commit_id).to be_nil
+        end
+      end
+    end
   end
 
   describe 'PUT #update' do
@@ -211,7 +247,7 @@ describe Projects::MergeRequests::DraftsController do
     end
 
     it 'publishes draft notes with position' do
-      diff_refs = project.commit(RepoHelpers.sample_commit.id).try(:diff_refs)
+      diff_refs = project.commit(sample_commit.id).try(:diff_refs)
 
       position = Gitlab::Diff::Position.new(
         old_path: "files/ruby/popen.rb",

ee/spec/frontend/diffs/components/diff_line_note_form_spec.js

+import { shallowMount, createLocalVue } from '@vue/test-utils';
+import Vuex from 'vuex';
+import DiffLineNoteForm from '~/diffs/components/diff_line_note_form.vue';
+import NoteForm from '~/notes/components/note_form.vue';
+import diffFileMockData from '../mock_data/diff_file';
+import note from '../../notes/mock_data';
+
+const localVue = createLocalVue();
+localVue.use(Vuex);
+
+describe('EE DiffLineNoteForm', () => {
+  let storeOptions;
+  let saveDraft;
+  let wrapper;
+
+  const createStoreOptions = headSha => {
+    const state = {
+      notes: {
+        notesData: { draftsPath: null },
+        noteableData: {},
+      },
+    };
+    const getters = {
+      getUserData: jest.fn(),
+      isLoggedIn: jest.fn(),
+      noteableType: jest.fn(),
+    };
+
+    return {
+      state,
+      getters,
+      modules: {
+        diffs: {
+          namespaced: true,
+          state: { commit: headSha || null },
+          getters: {
+            getDiffFileByHash: jest.fn().mockReturnValue(() => ({
+              diff_refs: {
+                head_sha: headSha || null,
+              },
+            })),
+          },
+        },
+        batchComments: {
+          namespaced: true,
+          actions: { saveDraft },
+        },
+      },
+    };
+  };
+
+  const createComponent = (props = {}) => {
+    const store = new Vuex.Store(storeOptions);
+
+    // deep clone the mock data
+    const diffFile = JSON.parse(JSON.stringify(diffFileMockData));
+    const diffLines = diffFile.highlighted_diff_lines;
+
+    wrapper = shallowMount(DiffLineNoteForm, {
+      propsData: {
+        diffFileHash: diffFile.file_hash,
+        diffLines,
+        line: diffLines[0],
+        noteTargetLine: diffLines[0],
+        ...props,
+      },
+      store,
+      sync: false,
+      localVue,
+    });
+  };
+
+  beforeEach(() => {
+    saveDraft = jest.fn();
+    storeOptions = createStoreOptions();
+  });
+
+  afterEach(() => {
+    wrapper.destroy();
+  });
+
+  const submitNoteAddToReview = () =>
+    wrapper.find(NoteForm).vm.$emit('handleFormUpdateAddToReview', note);
+  const saveDraftCommitId = () => saveDraft.mock.calls[0][1].data.note.commit_id;
+
+  describe('when user submits note to review', () => {
+    it('should call saveDraft action with commit_id === null when store has no commit', () => {
+      createComponent();
+
+      submitNoteAddToReview();
+
+      expect(saveDraft).toHaveBeenCalledTimes(1);
+      expect(saveDraftCommitId()).toBe(null);
+    });
+
+    it('should call saveDraft action with commit_id when store has commit', () => {
+      const HEAD_SHA = 'abc123';
+      storeOptions = createStoreOptions(HEAD_SHA);
+      createComponent();
+
+      submitNoteAddToReview();
+
+      expect(saveDraft).toHaveBeenCalledTimes(1);
+      expect(saveDraftCommitId()).toBe(HEAD_SHA);
+    });
+  });
+});

ee/spec/frontend/diffs/mock_data/diff_file.js

+// Copied from spec/javasripts/diffs/mock_data/diff_file.js
+
+export default {
+  submodule: false,
+  submodule_link: null,
+  blob: {
+    id: '9e10516ca50788acf18c518a231914a21e5f16f7',
+    path: 'CHANGELOG',
+    name: 'CHANGELOG',
+    mode: '100644',
+    readable_text: true,
+    icon: 'file-text-o',
+  },
+  blob_path: 'CHANGELOG',
+  blob_name: 'CHANGELOG',
+  blob_icon: '<i aria-hidden="true" data-hidden="true" class="fa fa-file-text-o fa-fw"></i>',
+  file_hash: '1c497fbb3a46b78edf04cc2a2fa33f67e3ffbe2a',
+  file_path: 'CHANGELOG',
+  new_file: false,
+  deleted_file: false,
+  renamed_file: false,
+  old_path: 'CHANGELOG',
+  new_path: 'CHANGELOG',
+  mode_changed: false,
+  a_mode: '100644',
+  b_mode: '100644',
+  text: true,
+  viewer: {
+    name: 'text',
+    error: null,
+    collapsed: false,
+  },
+  added_lines: 2,
+  removed_lines: 0,
+  diff_refs: {
+    base_sha: 'e63f41fe459e62e1228fcef60d7189127aeba95a',
+    start_sha: 'd9eaefe5a676b820c57ff18cf5b68316025f7962',
+    head_sha: 'c48ee0d1bf3b30453f5b32250ce03134beaa6d13',
+  },
+  content_sha: 'c48ee0d1bf3b30453f5b32250ce03134beaa6d13',
+  stored_externally: null,
+  external_storage: null,
+  old_path_html: 'CHANGELOG',
+  new_path_html: 'CHANGELOG',
+  edit_path: '/gitlab-org/gitlab-test/edit/spooky-stuff/CHANGELOG',
+  view_path: '/gitlab-org/gitlab-test/blob/spooky-stuff/CHANGELOG',
+  replaced_view_path: null,
+  collapsed: false,
+  renderIt: false,
+  too_large: false,
+  context_lines_path:
+    '/gitlab-org/gitlab-test/blob/c48ee0d1bf3b30453f5b32250ce03134beaa6d13/CHANGELOG/diff',
+  highlighted_diff_lines: [
+    {
+      line_code: '1c497fbb3a46b78edf04cc2a2fa33f67e3ffbe2a_1_1',
+      type: 'new',
+      old_line: null,
+      new_line: 1,
+      discussions: [],
+      text: '+<span id="LC1" class="line" lang="plaintext">  - Bad dates</span>\n',
+      rich_text: '+<span id="LC1" class="line" lang="plaintext">  - Bad dates</span>\n',
+      meta_data: null,
+    },
+    {
+      line_code: '1c497fbb3a46b78edf04cc2a2fa33f67e3ffbe2a_1_2',
+      type: 'new',
+      old_line: null,
+      new_line: 2,
+      discussions: [],
+      text: '+<span id="LC2" class="line" lang="plaintext"></span>\n',
+      rich_text: '+<span id="LC2" class="line" lang="plaintext"></span>\n',
+      meta_data: null,
+    },
+    {
+      line_code: '1c497fbb3a46b78edf04cc2a2fa33f67e3ffbe2a_1_3',
+      type: null,
+      old_line: 1,
+      new_line: 3,
+      discussions: [],
+      text: ' <span id="LC3" class="line" lang="plaintext">v6.8.0</span>\n',
+      rich_text: ' <span id="LC3" class="line" lang="plaintext">v6.8.0</span>\n',
+      meta_data: null,
+    },
+    {
+      line_code: '1c497fbb3a46b78edf04cc2a2fa33f67e3ffbe2a_2_4',
+      type: null,
+      old_line: 2,
+      new_line: 4,
+      discussions: [],
+      text: ' <span id="LC4" class="line" lang="plaintext"></span>\n',
+      rich_text: ' <span id="LC4" class="line" lang="plaintext"></span>\n',
+      meta_data: null,
+    },
+    {
+      line_code: '1c497fbb3a46b78edf04cc2a2fa33f67e3ffbe2a_3_5',
+      type: null,
+      old_line: 3,
+      new_line: 5,
+      discussions: [],
+      text: ' <span id="LC5" class="line" lang="plaintext">v6.7.0</span>\n',
+      rich_text: ' <span id="LC5" class="line" lang="plaintext">v6.7.0</span>\n',
+      meta_data: null,
+    },
+    {
+      line_code: null,
+      type: 'match',
+      old_line: null,
+      new_line: null,
+      discussions: [],
+      text: '',
+      rich_text: '',
+      meta_data: {
+        old_pos: 3,
+        new_pos: 5,
+      },
+    },
+  ],
+  parallel_diff_lines: [
+    {
+      left: {
+        type: 'empty-cell',
+      },
+      right: {
+        line_code: '1c497fbb3a46b78edf04cc2a2fa33f67e3ffbe2a_1_1',
+        type: 'new',
+        old_line: null,
+        new_line: 1,
+        discussions: [],
+        text: '+<span id="LC1" class="line" lang="plaintext">  - Bad dates</span>\n',
+        rich_text: '<span id="LC1" class="line" lang="plaintext">  - Bad dates</span>\n',
+        meta_data: null,
+      },
+    },
+    {
+      left: {
+        type: 'empty-cell',
+      },
+      right: {
+        line_code: '1c497fbb3a46b78edf04cc2a2fa33f67e3ffbe2a_1_2',
+        type: 'new',
+        old_line: null,
+        new_line: 2,
+        discussions: [],
+        text: '+<span id="LC2" class="line" lang="plaintext"></span>\n',
+        rich_text: '<span id="LC2" class="line" lang="plaintext"></span>\n',
+        meta_data: null,
+      },
+    },
+    {
+      left: {
+        line_Code: '1c497fbb3a46b78edf04cc2a2fa33f67e3ffbe2a_1_3',
+        type: null,
+        old_line: 1,
+        new_line: 3,
+        discussions: [],
+        text: ' <span id="LC3" class="line" lang="plaintext">v6.8.0</span>\n',
+        rich_text: '<span id="LC3" class="line" lang="plaintext">v6.8.0</span>\n',
+        meta_data: null,
+      },
+      right: {
+        line_code: '1c497fbb3a46b78edf04cc2a2fa33f67e3ffbe2a_1_3',
+        type: null,
+        old_line: 1,
+        new_line: 3,
+        discussions: [],
+        text: ' <span id="LC3" class="line" lang="plaintext">v6.8.0</span>\n',
+        rich_text: '<span id="LC3" class="line" lang="plaintext">v6.8.0</span>\n',
+        meta_data: null,
+      },
+    },
+    {
+      left: {
+        line_code: '1c497fbb3a46b78edf04cc2a2fa33f67e3ffbe2a_2_4',
+        type: null,
+        old_line: 2,
+        new_line: 4,
+        discussions: [],
+        text: ' <span id="LC4" class="line" lang="plaintext"></span>\n',
+        rich_text: '<span id="LC4" class="line" lang="plaintext"></span>\n',
+        meta_data: null,
+      },
+      right: {
+        line_code: '1c497fbb3a46b78edf04cc2a2fa33f67e3ffbe2a_2_4',
+        type: null,
+        old_line: 2,
+        new_line: 4,
+        discussions: [],
+        text: ' <span id="LC4" class="line" lang="plaintext"></span>\n',
+        rich_text: '<span id="LC4" class="line" lang="plaintext"></span>\n',
+        meta_data: null,
+      },
+    },
+    {
+      left: {
+        line_code: '1c497fbb3a46b78edf04cc2a2fa33f67e3ffbe2a_3_5',
+        type: null,
+        old_line: 3,
+        new_line: 5,
+        discussions: [],
+        text: ' <span id="LC5" class="line" lang="plaintext">v6.7.0</span>\n',
+        rich_text: '<span id="LC5" class="line" lang="plaintext">v6.7.0</span>\n',
+        meta_data: null,
+      },
+      right: {
+        line_code: '1c497fbb3a46b78edf04cc2a2fa33f67e3ffbe2a_3_5',
+        type: null,
+        old_line: 3,
+        new_line: 5,
+        discussions: [],
+        text: ' <span id="LC5" class="line" lang="plaintext">v6.7.0</span>\n',
+        rich_text: '<span id="LC5" class="line" lang="plaintext">v6.7.0</span>\n',
+        meta_data: null,
+      },
+    },
+    {
+      left: {
+        line_code: null,
+        type: 'match',
+        old_line: null,
+        new_line: null,
+        discussions: [],
+        text: '',
+        rich_text: '',
+        meta_data: {
+          old_pos: 3,
+          new_pos: 5,
+        },
+      },
+      right: {
+        line_code: null,
+        type: 'match',
+        old_line: null,
+        new_line: null,
+        discussions: [],
+        text: '',
+        rich_text: '',
+        meta_data: {
+          old_pos: 3,
+          new_pos: 5,
+        },
+      },
+    },
+  ],
+  discussions: [],
+  renderingLines: false,
+};

ee/spec/models/draft_note_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe DraftNote do
+  include RepoHelpers
+
+  describe 'validations' do
+    it_behaves_like 'a valid diff positionable note', :draft_note
+  end
+end

ee/spec/services/draft_notes/publish_service_spec.rb

@@ -2,16 +2,30 @@
 require 'spec_helper'
 
 describe DraftNotes::PublishService do
+  include RepoHelpers
+
   let(:merge_request) { create(:merge_request) }
   let(:project) { merge_request.target_project }
   let(:user) { merge_request.author }
+  let(:commit) { project.commit(sample_commit.id) }
+
+  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: commit.diff_refs
+    )
+  end
 
   def publish(draft: nil)
     DraftNotes::PublishService.new(merge_request, user).execute(draft)
   end
 
   context 'single draft note' do
-    let!(:drafts) { create_list(:draft_note, 2, merge_request: merge_request, author: user) }
+    let(:commit_id) { nil }
+    let!(:drafts) { create_list(:draft_note, 2, merge_request: merge_request, author: user, commit_id: commit_id, position: position) }
 
     it 'publishes' do
       expect { publish(draft: drafts.first) }.to change { DraftNote.count }.by(-1).and change { Note.count }.by(1)
@@ -28,12 +42,25 @@ describe DraftNotes::PublishService do
 
       expect(result[:status]).to eq(:success)
     end
+
+    context 'commit_id is set' do
+      let(:commit_id) { commit.id }
+
+      it 'creates note from draft with commit_id' do
+        result = publish(draft: drafts.first)
+
+        expect(result[:status]).to eq(:success)
+        expect(merge_request.notes.first.commit_id).to eq(commit_id)
+      end
+    end
   end
 
   context 'multiple draft notes' do
+    let(:commit_id) { nil }
+
     before do
-      create(:draft_note, merge_request: merge_request, author: user, note: 'first note')
-      create(:draft_note, merge_request: merge_request, author: user, note: 'second note')
+      create(:draft_note, merge_request: merge_request, author: user, note: 'first note', commit_id: commit_id, position: position)
+      create(:draft_note, merge_request: merge_request, author: user, note: 'second note', commit_id: commit_id, position: position)
     end
 
     context 'when review fails to create' do
@@ -77,6 +104,20 @@ describe DraftNotes::PublishService do
 
       publish
     end
+
+    context 'commit_id is set' do
+      let(:commit_id) { commit.id }
+
+      it 'creates note from draft with commit_id' do
+        result = publish
+
+        expect(result[:status]).to eq(:success)
+
+        merge_request.notes.each do |note|
+          expect(note.commit_id).to eq(commit_id)
+        end
+      end
+    end
   end
 
   context 'draft notes with suggestions' do

spec/db/schema_spec.rb

@@ -29,7 +29,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],

spec/javascripts/diffs/mock_data/diff_file.js

+// Copied to ee/spec/frontend/diffs/mock_data/diff_file.js
+
 export default {
   submodule: false,
   submodule_link: null,

spec/javascripts/notes/mock_data.js

+// Copied to ee/spec/frontend/notes/mock_data.js
+
 export const notesDataMock = {
   discussionsPath: '/gitlab-org/gitlab-ce/issues/26/discussions.json',
   lastFetchedAt: 1501862675,

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

spec/support/shared_examples/models/diff_positionable_note_shared_examples.rb

+# 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