Commit d62227a3 authored by Paul Slaughter's avatar Paul Slaughter

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
parents 325c321c d90865a9
...@@ -10,6 +10,8 @@ module DiffPositionableNote ...@@ -10,6 +10,8 @@ module DiffPositionableNote
serialize :original_position, Gitlab::Diff::Position # rubocop:disable Cop/ActiveRecordSerialize serialize :original_position, Gitlab::Diff::Position # rubocop:disable Cop/ActiveRecordSerialize
serialize :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 serialize :change_position, Gitlab::Diff::Position # rubocop:disable Cop/ActiveRecordSerialize
validate :diff_refs_match_commit, if: :for_commit?
end end
%i(original_position position change_position).each do |meth| %i(original_position position change_position).each do |meth|
...@@ -71,4 +73,10 @@ module DiffPositionableNote ...@@ -71,4 +73,10 @@ module DiffPositionableNote
self.position = result[:position] self.position = result[:position]
end end
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 end
...@@ -20,7 +20,6 @@ class DiffNote < Note ...@@ -20,7 +20,6 @@ class DiffNote < Note
validates :noteable_type, inclusion: { in: -> (_note) { noteable_types } } validates :noteable_type, inclusion: { in: -> (_note) { noteable_types } }
validate :positions_complete validate :positions_complete
validate :verify_supported validate :verify_supported
validate :diff_refs_match_commit, if: :for_commit?
before_validation :set_line_code, if: :on_text? before_validation :set_line_code, if: :on_text?
after_save :keep_around_commits after_save :keep_around_commits
...@@ -154,12 +153,6 @@ class DiffNote < Note ...@@ -154,12 +153,6 @@ class DiffNote < Note
errors.add(:position, "is invalid") errors.add(:position, "is invalid")
end 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 def keep_around_commits
shas = [ shas = [
self.original_position.base_sha, self.original_position.base_sha,
......
# 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
...@@ -1136,6 +1136,7 @@ ActiveRecord::Schema.define(version: 2019_07_31_084415) do ...@@ -1136,6 +1136,7 @@ ActiveRecord::Schema.define(version: 2019_07_31_084415) do
t.text "position" t.text "position"
t.text "original_position" t.text "original_position"
t.text "change_position" t.text "change_position"
t.binary "commit_id"
t.index ["author_id"], name: "index_draft_notes_on_author_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 ["discussion_id"], name: "index_draft_notes_on_discussion_id"
t.index ["merge_request_id"], name: "index_draft_notes_on_merge_request_id" t.index ["merge_request_id"], name: "index_draft_notes_on_merge_request_id"
......
...@@ -27,7 +27,7 @@ describe 'Database schema' do ...@@ -27,7 +27,7 @@ describe 'Database schema' do
cluster_providers_gcp: %w[gcp_project_id operation_id], cluster_providers_gcp: %w[gcp_project_id operation_id],
deploy_keys_projects: %w[deploy_key_id], deploy_keys_projects: %w[deploy_key_id],
deployments: %w[deployable_id environment_id user_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], emails: %w[user_id],
events: %w[target_id], events: %w[target_id],
epics: %w[updated_by_id last_edited_by_id start_date_sourcing_milestone_id due_date_sourcing_milestone_id], epics: %w[updated_by_id last_edited_by_id start_date_sourcing_milestone_id due_date_sourcing_milestone_id],
......
// Copied to ee/spec/frontend/diffs/mock_data/diff_file.js
export default { export default {
submodule: false, submodule: false,
submodule_link: null, submodule_link: null,
......
// Copied to ee/spec/frontend/notes/mock_data.js
export const notesDataMock = { export const notesDataMock = {
discussionsPath: '/gitlab-org/gitlab-ce/issues/26/discussions.json', discussionsPath: '/gitlab-org/gitlab-ce/issues/26/discussions.json',
lastFetchedAt: 1501862675, lastFetchedAt: 1501862675,
......
...@@ -34,6 +34,10 @@ describe DiffNote do ...@@ -34,6 +34,10 @@ describe DiffNote do
subject { create(:diff_note_on_merge_request, project: project, position: position, noteable: merge_request) } 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 describe "#position=" do
context "when provided a string" do context "when provided a string" do
it "sets the position" do it "sets the position" do
......
# 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
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment