diff --git a/app/models/issue.rb b/app/models/issue.rb index 3fc57757d361ab4538c2742a68eaf27a085bd483..4d3b2e644fb791962d750804be25dfd5d43aa4af 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -575,10 +575,10 @@ def blocked_for_repositioning? end # `from` argument can be a Namespace or Project. - def to_reference(from = nil, full: false) + def to_reference(from = nil, full: false, absolute_path: false) reference = "#{self.class.reference_prefix}#{iid}" - "#{namespace.to_reference_base(from, full: full)}#{reference}" + "#{namespace.to_reference_base(from, full: full, absolute_path: absolute_path)}#{reference}" end def suggested_branch_name @@ -817,7 +817,35 @@ def hook_attrs override :gfm_reference def gfm_reference(from = nil) - "#{work_item_type_with_default.name.underscore} #{to_reference(from)}" + # References can be ambiguous when two namespaces have the same path names. This is why we need to use absolute + # paths when cross-referencing between projects and groups. + # + # Example setup: + # - `gitlab` group + # - `gitlab` project within the `gitlab` group + # - In a project issue, we reference an epic with `epic gitlab#123` for a system note + # + # When resolving this system note, we would look for a namespace within the `gitlab` project's parent. + # This would be incorrect, as it would resolve to an ISSUE on the `gitlab` PROJECT, not the `gitlab` GROUP. + # + # This problem only occurs when there is a project with the same name as the group. Otherwise, our reference + # code attempts to resolve it on the group. + # + # Since the reference parser has no information about where each reference originated, we need to fix this in + # the reference itself by providing an absolute path. + # + # In the example above, the `from` argument is the Issue on the Project, and `self` is the item on the Group. + # Every time we reference from a project to a group, we need to use an absolute path. + # So we need to reference it as `epic /gitlab#123`. + # + # We could always use absolute paths to remove the ambiguity, but this would lead to longer references everywhere + # that are harder to read. + params = {} + if (from.is_a?(Project) || from.is_a?(Namespaces::ProjectNamespace)) && group_level? + params = { full: true, absolute_path: true } + end + + "#{work_item_type_with_default.name.underscore} #{to_reference(from, **params)}" end def skip_metrics? diff --git a/ee/spec/services/ee/system_notes/issuables_service_spec.rb b/ee/spec/services/ee/system_notes/issuables_service_spec.rb index 2019261c0884d11d1feab1b0a964c8e90b4f80e5..1c860f75fb3c563ad5852d02db6271a753939d4e 100644 --- a/ee/spec/services/ee/system_notes/issuables_service_spec.rb +++ b/ee/spec/services/ee/system_notes/issuables_service_spec.rb @@ -316,6 +316,8 @@ describe '#cross_reference' do let(:mentioned_in) { create(:issue, project: project) } + let(:new_note) { service.cross_reference(mentioned_in) } + subject { service.cross_reference(mentioned_in) } context 'when noteable is an epic' do @@ -333,6 +335,37 @@ end end + context 'with project and group having the same path name' do + using RSpec::Parameterized::TableSyntax + + let_it_be_with_reload(:group) { create(:group, path: 'same-name') } + let_it_be_with_reload(:project) { create(:project, path: 'same-name', group: group) } + let_it_be_with_reload(:noteable) { create(:issue, project: project) } + + let_it_be(:issue) { create(:issue, project: project, iid: 100) } + let_it_be(:issue_2) { create(:issue, project: project, iid: 101) } + let_it_be(:epic) { create(:epic, group: group, iid: 100) } + let_it_be(:epic_work_item) { create(:work_item, :epic, namespace: group, iid: 101) } + let_it_be(:merge_request) { create(:merge_request, source_project: project, iid: 100) } + + let(:service) { described_class.new(noteable: noteable, container: group, author: author) } + + where(:mentioned_in, :notable, :type) do + ref(:epic) | ref(:issue) | 'epic' + ref(:epic) | ref(:merge_request) | 'epic' + ref(:issue) | ref(:merge_request) | 'issue' + ref(:epic_work_item) | ref(:issue) | 'epic' + ref(:epic_work_item) | ref(:merge_request) | 'epic' + ref(:merge_request) | ref(:epic_work_item) | 'merge request' + end + + with_them do + it 'has the correct link' do + expect(new_note.note_html).to include(::Gitlab::UrlBuilder.build(mentioned_in, only_path: true)) + end + end + end + context 'when notable is not an epic' do it 'does not tracks epic cross reference event in usage ping' do expect(::Gitlab::UsageDataCounters::EpicActivityUniqueCounter).not_to receive(:track_epic_cross_referenced) diff --git a/spec/models/issue_spec.rb b/spec/models/issue_spec.rb index 5585e799761d1d344e010324e35a4be99a7d5416..928c1d51a9f65b9c73a8c083b72a9828f5f0a917 100644 --- a/spec/models/issue_spec.rb +++ b/spec/models/issue_spec.rb @@ -1862,6 +1862,25 @@ expect(issue.gfm_reference).to eq("#{expected_name} #{issue.to_reference}") end end + + context 'when referencing a group level issue' do + let_it_be(:group) { create(:group) } + let_it_be(:group_issue) { create(:issue, :group_level, namespace: group) } + let_it_be(:group_issue2) { create(:issue, :group_level, namespace: group) } + let_it_be(:issue) { create(:issue) } + + it 'uses uses an absolute path when reference comes from a project' do + expect(group_issue.gfm_reference(issue.project)).to eq("issue /#{group.full_path}##{group_issue.iid}") + end + + it 'uses uses an absolute path when reference comes from a project namespace' do + expect(group_issue.gfm_reference(issue.namespace)).to eq("issue /#{group.full_path}##{group_issue.iid}") + end + + it 'does not use an absolute path when reference comes from a group' do + expect(group_issue.gfm_reference(group)).to eq("issue ##{group_issue.iid}") + end + end end describe '#has_widget?' do