Fix rewriting issue references with group milestones

7 files changed, 77 insertions, 24 deletions

diff --git a/app/models/milestone.rb b/app/models/milestone.rb
index a3070a12b7c..47e6b785c39 100644
--- a/app/models/milestone.rb
+++ b/app/models/milestone.rb
@@ -162,9 +162,7 @@ class Milestone < ActiveRecord::Base
   #   Milestone.first.to_reference(cross_namespace_project)  # => "gitlab-org/gitlab-ce%1"
   #   Milestone.first.to_reference(same_namespace_project)   # => "gitlab-ce%1"
   #
-  def to_reference(from_project = nil, format: :iid, full: false)
-    return if group_milestone? && format != :name
-
+  def to_reference(from_project = nil, format: :name, full: false)
     format_reference = milestone_format_reference(format)
     reference = "#{self.class.reference_prefix}#{format_reference}"
 
@@ -241,6 +239,10 @@ class Milestone < ActiveRecord::Base
   def milestone_format_reference(format = :iid)
     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?('"')
       %("#{name}")
     else
diff --git a/changelogs/unreleased/fix-gb-fix-moving-issue-with-ambiguous-references.yml b/changelogs/unreleased/fix-gb-fix-moving-issue-with-ambiguous-references.yml
new file mode 100644
index 00000000000..f90766ef6d2
--- /dev/null
+++ b/changelogs/unreleased/fix-gb-fix-moving-issue-with-ambiguous-references.yml
@@ -0,0 +1,5 @@
+---
+title: Fix errors when moving issue with reference to a group milestone
+merge_request: 14294
+author:
+type: fixed
diff --git a/lib/gitlab/gfm/reference_rewriter.rb b/lib/gitlab/gfm/reference_rewriter.rb
index b984492d369..455814a9159 100644
--- a/lib/gitlab/gfm/reference_rewriter.rb
+++ b/lib/gitlab/gfm/reference_rewriter.rb
@@ -29,6 +29,8 @@ module Gitlab
     #  http://gitlab.com/some/link/#1234, and code `puts #1234`'
     #
     class ReferenceRewriter
+      RewriteError = Class.new(StandardError)
+
       def initialize(text, source_project, current_user)
         @text = text
         @source_project = source_project
@@ -61,6 +63,10 @@ module Gitlab
         cross_reference = build_cross_reference(referable, target_project)
         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
         substitution_valid?(new_text) ? cross_reference : reference
       end
diff --git a/spec/lib/banzai/filter/milestone_reference_filter_spec.rb b/spec/lib/banzai/filter/milestone_reference_filter_spec.rb
index ebd6c79077e..fe7a8c84c9e 100644
--- a/spec/lib/banzai/filter/milestone_reference_filter_spec.rb
+++ b/spec/lib/banzai/filter/milestone_reference_filter_spec.rb
@@ -296,7 +296,7 @@ describe Banzai::Filter::MilestoneReferenceFilter do
 
   context 'project milestones' do
     let(:milestone) { create(:milestone, project: project) }
-    let(:reference) { milestone.to_reference }
+    let(:reference) { milestone.to_reference(format: :iid) }
 
     include_examples 'reference parsing'
 
diff --git a/spec/lib/gitlab/gfm/reference_rewriter_spec.rb b/spec/lib/gitlab/gfm/reference_rewriter_spec.rb
index a3d323fe28a..7dc06c90078 100644
--- a/spec/lib/gitlab/gfm/reference_rewriter_spec.rb
+++ b/spec/lib/gitlab/gfm/reference_rewriter_spec.rb
@@ -1,11 +1,14 @@
 require 'spec_helper'
 
 describe Gitlab::Gfm::ReferenceRewriter do
-  let(:text) { 'some text' }
-  let(:old_project) { create(:project, name: 'old-project') }
-  let(:new_project) { create(:project, name: 'new-project') }
+  let(:group) { create(:group) }
+  let(:old_project) { create(:project, name: 'old-project', group: group) }
+  let(:new_project) { create(:project, name: 'new-project', group: group) }
   let(:user) { create(:user) }
 
+  let(:old_project_ref) { old_project.to_reference(new_project) }
+  let(:text) { 'some text' }
+
   before do
     old_project.team << [user, :reporter]
   end
@@ -39,7 +42,7 @@ describe Gitlab::Gfm::ReferenceRewriter do
         it { is_expected.not_to include merge_request.to_reference(new_project) }
       end
 
-      context 'description ambigous elements' do
+      context 'rewrite ambigous references' do
         context 'url' do
           let(:url) { 'http://gitlab.com/#1' }
           let(:text) { "This references #1, but not #{url}" }
@@ -66,23 +69,21 @@ describe Gitlab::Gfm::ReferenceRewriter do
 
         context 'description with project labels' do
           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
             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
 
           context 'label referenced by text' do
             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
 
         context 'description with group labels' do
           let(:old_group) { create(: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
             old_project.update(namespace: old_group)
@@ -90,21 +91,53 @@ describe Gitlab::Gfm::ReferenceRewriter do
 
           context 'label referenced by id' do
             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
 
           context 'label referenced by text' do
             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
+
+    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
+
+      let(:text) { 'milestone %"10.0"' }
+
+      it { is_expected.to eq text }
+    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
 
-      context 'reference contains milestone' do
-        let(:milestone) { create(:milestone) }
-        let(:text) { "milestone ref: #{milestone.to_reference}" }
+      let(:text) { 'milestone: %"9.0"' }
 
-        it { is_expected.to eq text }
+      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
diff --git a/spec/models/milestone_spec.rb b/spec/models/milestone_spec.rb
index d3da0107d5c..13e37fffa4e 100644
--- a/spec/models/milestone_spec.rb
+++ b/spec/models/milestone_spec.rb
@@ -238,7 +238,7 @@ describe Milestone do
       let(:milestone) { build_stubbed(:milestone, iid: 1, project: project, name: 'milestone') }
 
       it 'returns a String reference to the object' do
-        expect(milestone.to_reference).to eq '%1'
+        expect(milestone.to_reference).to eq '%"milestone"'
       end
 
       it 'returns a reference by name when the format is set to :name' do
@@ -246,24 +246,29 @@ describe Milestone do
       end
 
       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
 
     context 'for a group milestone' do
       let(:milestone) { build_stubbed(:milestone, iid: 1, group: group, name: 'milestone') }
 
-      it 'returns nil with the default format' do
-        expect(milestone.to_reference).to be_nil
+      it 'returns a group milestone reference with a default format' do
+        expect(milestone.to_reference).to eq '%"milestone"'
       end
 
       it 'returns a reference by name when the format is set to :name' do
         expect(milestone.to_reference(format: :name)).to eq '%"milestone"'
       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"'
       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
 
diff --git a/spec/services/system_note_service_spec.rb b/spec/services/system_note_service_spec.rb
index c2d6d7781b9..b1241cd8d0b 100644
--- a/spec/services/system_note_service_spec.rb
+++ b/spec/services/system_note_service_spec.rb
@@ -232,7 +232,9 @@ describe SystemNoteService do
 
       context 'when milestone added' 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