Commit 96bb281c authored by Rémy Coutable's avatar Rémy Coutable

Merge branch 'fix/gb/fix-moving-issue-with-ambiguous-references' into 'master'

Fix rewriting issue references with group milestones

Closes #37746

See merge request gitlab-org/gitlab-ce!14294
parents d247841b b49bd4d3
...@@ -162,9 +162,7 @@ class Milestone < ActiveRecord::Base ...@@ -162,9 +162,7 @@ class Milestone < ActiveRecord::Base
# Milestone.first.to_reference(cross_namespace_project) # => "gitlab-org/gitlab-ce%1" # Milestone.first.to_reference(cross_namespace_project) # => "gitlab-org/gitlab-ce%1"
# Milestone.first.to_reference(same_namespace_project) # => "gitlab-ce%1" # Milestone.first.to_reference(same_namespace_project) # => "gitlab-ce%1"
# #
def to_reference(from_project = nil, format: :iid, full: false) def to_reference(from_project = nil, format: :name, full: false)
return if group_milestone? && format != :name
format_reference = milestone_format_reference(format) format_reference = milestone_format_reference(format)
reference = "#{self.class.reference_prefix}#{format_reference}" reference = "#{self.class.reference_prefix}#{format_reference}"
...@@ -241,6 +239,10 @@ class Milestone < ActiveRecord::Base ...@@ -241,6 +239,10 @@ class Milestone < ActiveRecord::Base
def milestone_format_reference(format = :iid) def milestone_format_reference(format = :iid)
raise ArgumentError, 'Unknown format' unless [:iid, :name].include?(format) raise ArgumentError, 'Unknown format' unless [:iid, :name].include?(format)
if group_milestone? && format == :iid
raise ArgumentError, 'Cannot refer to a group milestone by an internal id!'
end
if format == :name && !name.include?('"') if format == :name && !name.include?('"')
%("#{name}") %("#{name}")
else else
......
---
title: Fix errors when moving issue with reference to a group milestone
merge_request: 14294
author:
type: fixed
...@@ -29,6 +29,8 @@ module Gitlab ...@@ -29,6 +29,8 @@ module Gitlab
# http://gitlab.com/some/link/#1234, and code `puts #1234`' # http://gitlab.com/some/link/#1234, and code `puts #1234`'
# #
class ReferenceRewriter class ReferenceRewriter
RewriteError = Class.new(StandardError)
def initialize(text, source_project, current_user) def initialize(text, source_project, current_user)
@text = text @text = text
@source_project = source_project @source_project = source_project
...@@ -61,6 +63,10 @@ module Gitlab ...@@ -61,6 +63,10 @@ module Gitlab
cross_reference = build_cross_reference(referable, target_project) cross_reference = build_cross_reference(referable, target_project)
return reference if reference == cross_reference return reference if reference == cross_reference
if cross_reference.nil?
raise RewriteError, "Unspecified reference detected for #{referable.class.name}"
end
new_text = before + cross_reference + after new_text = before + cross_reference + after
substitution_valid?(new_text) ? cross_reference : reference substitution_valid?(new_text) ? cross_reference : reference
end end
......
...@@ -296,7 +296,7 @@ describe Banzai::Filter::MilestoneReferenceFilter do ...@@ -296,7 +296,7 @@ describe Banzai::Filter::MilestoneReferenceFilter do
context 'project milestones' do context 'project milestones' do
let(:milestone) { create(:milestone, project: project) } let(:milestone) { create(:milestone, project: project) }
let(:reference) { milestone.to_reference } let(:reference) { milestone.to_reference(format: :iid) }
include_examples 'reference parsing' include_examples 'reference parsing'
......
require 'spec_helper' require 'spec_helper'
describe Gitlab::Gfm::ReferenceRewriter do describe Gitlab::Gfm::ReferenceRewriter do
let(:text) { 'some text' } let(:group) { create(:group) }
let(:old_project) { create(:project, name: 'old-project') } let(:old_project) { create(:project, name: 'old-project', group: group) }
let(:new_project) { create(:project, name: 'new-project') } let(:new_project) { create(:project, name: 'new-project', group: group) }
let(:user) { create(:user) } let(:user) { create(:user) }
let(:old_project_ref) { old_project.to_reference(new_project) }
let(:text) { 'some text' }
before do before do
old_project.team << [user, :reporter] old_project.team << [user, :reporter]
end end
...@@ -39,7 +42,7 @@ describe Gitlab::Gfm::ReferenceRewriter do ...@@ -39,7 +42,7 @@ describe Gitlab::Gfm::ReferenceRewriter do
it { is_expected.not_to include merge_request.to_reference(new_project) } it { is_expected.not_to include merge_request.to_reference(new_project) }
end end
context 'description ambigous elements' do context 'rewrite ambigous references' do
context 'url' do context 'url' do
let(:url) { 'http://gitlab.com/#1' } let(:url) { 'http://gitlab.com/#1' }
let(:text) { "This references #1, but not #{url}" } let(:text) { "This references #1, but not #{url}" }
...@@ -66,23 +69,21 @@ describe Gitlab::Gfm::ReferenceRewriter do ...@@ -66,23 +69,21 @@ describe Gitlab::Gfm::ReferenceRewriter do
context 'description with project labels' do context 'description with project labels' do
let!(:label) { create(:label, id: 123, name: 'test', project: old_project) } let!(:label) { create(:label, id: 123, name: 'test', project: old_project) }
let(:project_ref) { old_project.to_reference(new_project) }
context 'label referenced by id' do context 'label referenced by id' do
let(:text) { '#1 and ~123' } let(:text) { '#1 and ~123' }
it { is_expected.to eq %Q{#{project_ref}#1 and #{project_ref}~123} } it { is_expected.to eq %Q{#{old_project_ref}#1 and #{old_project_ref}~123} }
end end
context 'label referenced by text' do context 'label referenced by text' do
let(:text) { '#1 and ~"test"' } let(:text) { '#1 and ~"test"' }
it { is_expected.to eq %Q{#{project_ref}#1 and #{project_ref}~123} } it { is_expected.to eq %Q{#{old_project_ref}#1 and #{old_project_ref}~123} }
end end
end end
context 'description with group labels' do context 'description with group labels' do
let(:old_group) { create(:group) } let(:old_group) { create(:group) }
let!(:group_label) { create(:group_label, id: 321, name: 'group label', group: old_group) } let!(:group_label) { create(:group_label, id: 321, name: 'group label', group: old_group) }
let(:project_ref) { old_project.to_reference(new_project) }
before do before do
old_project.update(namespace: old_group) old_project.update(namespace: old_group)
...@@ -90,22 +91,54 @@ describe Gitlab::Gfm::ReferenceRewriter do ...@@ -90,22 +91,54 @@ describe Gitlab::Gfm::ReferenceRewriter do
context 'label referenced by id' do context 'label referenced by id' do
let(:text) { '#1 and ~321' } let(:text) { '#1 and ~321' }
it { is_expected.to eq %Q{#{project_ref}#1 and #{project_ref}~321} } it { is_expected.to eq %Q{#{old_project_ref}#1 and #{old_project_ref}~321} }
end end
context 'label referenced by text' do context 'label referenced by text' do
let(:text) { '#1 and ~"group label"' } let(:text) { '#1 and ~"group label"' }
it { is_expected.to eq %Q{#{project_ref}#1 and #{project_ref}~321} } it { is_expected.to eq %Q{#{old_project_ref}#1 and #{old_project_ref}~321} }
end
end
end end
end end
context 'reference contains project milestone' do
let!(:milestone) do
create(:milestone, title: '9.0', project: old_project)
end
let(:text) { 'milestone: %"9.0"' }
it { is_expected.to eq %Q[milestone: #{old_project_ref}%"9.0"] }
end
context 'when referring to group milestone' do
let!(:milestone) do
create(:milestone, title: '10.0', group: group)
end end
context 'reference contains milestone' do let(:text) { 'milestone %"10.0"' }
let(:milestone) { create(:milestone) }
let(:text) { "milestone ref: #{milestone.to_reference}" }
it { is_expected.to eq text } it { is_expected.to eq text }
end end
context 'when referable has a nil reference' do
before do
create(:milestone, title: '9.0', project: old_project)
allow_any_instance_of(Milestone)
.to receive(:to_reference)
.and_return(nil)
end
let(:text) { 'milestone: %"9.0"' }
it 'raises an error that should be fixed' do
expect { subject }.to raise_error(
described_class::RewriteError,
'Unspecified reference detected for Milestone'
)
end
end end
end end
end end
...@@ -238,7 +238,7 @@ describe Milestone do ...@@ -238,7 +238,7 @@ describe Milestone do
let(:milestone) { build_stubbed(:milestone, iid: 1, project: project, name: 'milestone') } let(:milestone) { build_stubbed(:milestone, iid: 1, project: project, name: 'milestone') }
it 'returns a String reference to the object' do it 'returns a String reference to the object' do
expect(milestone.to_reference).to eq '%1' expect(milestone.to_reference).to eq '%"milestone"'
end end
it 'returns a reference by name when the format is set to :name' do it 'returns a reference by name when the format is set to :name' do
...@@ -246,24 +246,29 @@ describe Milestone do ...@@ -246,24 +246,29 @@ describe Milestone do
end end
it 'supports a cross-project reference' do it 'supports a cross-project reference' do
expect(milestone.to_reference(another_project)).to eq 'sample-project%1' expect(milestone.to_reference(another_project)).to eq 'sample-project%"milestone"'
end end
end end
context 'for a group milestone' do context 'for a group milestone' do
let(:milestone) { build_stubbed(:milestone, iid: 1, group: group, name: 'milestone') } let(:milestone) { build_stubbed(:milestone, iid: 1, group: group, name: 'milestone') }
it 'returns nil with the default format' do it 'returns a group milestone reference with a default format' do
expect(milestone.to_reference).to be_nil expect(milestone.to_reference).to eq '%"milestone"'
end end
it 'returns a reference by name when the format is set to :name' do it 'returns a reference by name when the format is set to :name' do
expect(milestone.to_reference(format: :name)).to eq '%"milestone"' expect(milestone.to_reference(format: :name)).to eq '%"milestone"'
end end
it 'does not supports cross-project references' do it 'does supports cross-project references within a group' do
expect(milestone.to_reference(another_project, format: :name)).to eq '%"milestone"' expect(milestone.to_reference(another_project, format: :name)).to eq '%"milestone"'
end end
it 'raises an error when using iid format' do
expect { milestone.to_reference(format: :iid) }
.to raise_error(ArgumentError, 'Cannot refer to a group milestone by an internal id!')
end
end end
end end
......
...@@ -232,7 +232,9 @@ describe SystemNoteService do ...@@ -232,7 +232,9 @@ describe SystemNoteService do
context 'when milestone added' do context 'when milestone added' do
it 'sets the note text' do it 'sets the note text' do
expect(subject.note).to eq "changed milestone to #{milestone.to_reference}" reference = milestone.to_reference(format: :iid)
expect(subject.note).to eq "changed milestone to #{reference}"
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