diff options
22 files changed, 411 insertions, 203 deletions
diff --git a/app/helpers/gitlab_routing_helper.rb b/app/helpers/gitlab_routing_helper.rb index 1f7db9b2eb8..d4a91e533c1 100644 --- a/app/helpers/gitlab_routing_helper.rb +++ b/app/helpers/gitlab_routing_helper.rb @@ -47,14 +47,6 @@ module GitlabRoutingHelper project_pipeline_path(pipeline.project, pipeline.id, *args) end - def milestone_path(entity, *args) - if entity.is_group_milestone? - group_milestone_path(entity.group, entity, *args) - elsif entity.is_project_milestone? - project_milestone_path(entity.project, entity, *args) - end - end - def issue_url(entity, *args) project_issue_url(entity.project, entity, *args) end @@ -67,14 +59,6 @@ module GitlabRoutingHelper project_pipeline_url(pipeline.project, pipeline.id, *args) end - def milestone_url(entity, *args) - if entity.is_group_milestone? - group_milestone_url(entity.group, entity, *args) - elsif entity.is_project_milestone? - project_milestone_url(entity.project, entity, *args) - end - end - def pipeline_job_url(pipeline, build, *args) project_job_url(pipeline.project, build.id, *args) end diff --git a/app/helpers/milestones_routing_helper.rb b/app/helpers/milestones_routing_helper.rb new file mode 100644 index 00000000000..766d5262018 --- /dev/null +++ b/app/helpers/milestones_routing_helper.rb @@ -0,0 +1,17 @@ +module MilestonesRoutingHelper + def milestone_path(milestone, *args) + if milestone.is_group_milestone? + group_milestone_path(milestone.group, milestone, *args) + elsif milestone.is_project_milestone? + project_milestone_path(milestone.project, milestone, *args) + end + end + + def milestone_url(milestone, *args) + if milestone.is_group_milestone? + group_milestone_url(milestone.group, milestone, *args) + elsif milestone.is_project_milestone? + project_milestone_url(milestone.project, milestone, *args) + end + end +end diff --git a/app/models/milestone.rb b/app/models/milestone.rb index 48d00764965..01e0d0155a3 100644 --- a/app/models/milestone.rb +++ b/app/models/milestone.rb @@ -149,7 +149,9 @@ class Milestone < ActiveRecord::Base end ## - # Returns the String necessary to reference this Milestone in Markdown + # Returns the String necessary to reference this Milestone in Markdown. Group + # milestones only support name references, and do not support cross-project + # references. # # format - Symbol format to use (default: :iid, optional: :name) # @@ -161,12 +163,16 @@ class Milestone < ActiveRecord::Base # Milestone.first.to_reference(same_namespace_project) # => "gitlab-ce%1" # def to_reference(from_project = nil, format: :iid, full: false) - return if is_group_milestone? + return if is_group_milestone? && format != :name format_reference = milestone_format_reference(format) reference = "#{self.class.reference_prefix}#{format_reference}" - "#{project.to_reference(from_project, full: full)}#{reference}" + if project + "#{project.to_reference(from_project, full: full)}#{reference}" + else + reference + end end def reference_link_text(from_project = nil) diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb index 760a15e3ed0..7df5039f2e4 100644 --- a/app/services/issuable_base_service.rb +++ b/app/services/issuable_base_service.rb @@ -2,11 +2,8 @@ class IssuableBaseService < BaseService private def create_milestone_note(issuable) - milestone = issuable.milestone - return if milestone && milestone.is_group_milestone? - SystemNoteService.change_milestone( - issuable, issuable.project, current_user, milestone) + issuable, issuable.project, current_user, issuable.milestone) end def create_labels_note(issuable, old_labels) diff --git a/app/services/projects/autocomplete_service.rb b/app/services/projects/autocomplete_service.rb index fc85f398935..724a77c873a 100644 --- a/app/services/projects/autocomplete_service.rb +++ b/app/services/projects/autocomplete_service.rb @@ -5,7 +5,15 @@ module Projects end def milestones - @project.milestones.active.reorder(due_date: :asc, title: :asc).select([:iid, :title]) + finder_params = { + project_ids: [@project.id], + state: :active, + order: { due_date: :asc, title: :asc } + } + + finder_params[:group_ids] = [@project.group.id] if @project.group + + MilestonesFinder.new(finder_params).execute.select([:iid, :title]) end def merge_requests diff --git a/app/services/system_note_service.rb b/app/services/system_note_service.rb index 2dbee9c246e..1763f64a4e4 100644 --- a/app/services/system_note_service.rb +++ b/app/services/system_note_service.rb @@ -142,7 +142,8 @@ module SystemNoteService # # Returns the created Note object def change_milestone(noteable, project, author, milestone) - body = milestone.nil? ? 'removed milestone' : "changed milestone to #{milestone.to_reference(project)}" + format = milestone&.is_group_milestone? ? :name : :iid + body = milestone.nil? ? 'removed milestone' : "changed milestone to #{milestone.to_reference(project, format: format)}" create_note(NoteSummary.new(noteable, project, author, body, action: 'milestone')) end diff --git a/changelogs/unreleased/group-milestone-references-system-notes.yml b/changelogs/unreleased/group-milestone-references-system-notes.yml new file mode 100644 index 00000000000..58215352305 --- /dev/null +++ b/changelogs/unreleased/group-milestone-references-system-notes.yml @@ -0,0 +1,4 @@ +--- +title: Support Markdown references, autocomplete, and quick actions for group milestones +merge_request: +author: diff --git a/config/application.rb b/config/application.rb index f7145566262..47887bf8596 100644 --- a/config/application.rb +++ b/config/application.rb @@ -181,7 +181,11 @@ module Gitlab end end + # We add the MilestonesRoutingHelper because we know that this does not + # conflict with the methods defined in `project_url_helpers`, and we want + # these methods available in the same places. Gitlab::Routing.add_helpers(project_url_helpers) + Gitlab::Routing.add_helpers(MilestonesRoutingHelper) end end end diff --git a/doc/user/markdown.md b/doc/user/markdown.md index 0d29b471d52..b42b8f0a525 100644 --- a/doc/user/markdown.md +++ b/doc/user/markdown.md @@ -248,7 +248,7 @@ GFM will recognize the following: | `~123` | label by ID | | `~bug` | one-word label by name | | `~"feature request"` | multi-word label by name | -| `%123` | milestone by ID | +| `%123` | project milestone by ID | | `%v1.23` | one-word milestone by name | | `%"release candidate"` | multi-word milestone by name | | `9ba12248` | specific commit | @@ -262,7 +262,7 @@ GFM also recognizes certain cross-project references: |:----------------------------------------|:------------------------| | `namespace/project#123` | issue | | `namespace/project!123` | merge request | -| `namespace/project%123` | milestone | +| `namespace/project%123` | project milestone | | `namespace/project$123` | snippet | | `namespace/project@9ba12248` | specific commit | | `namespace/project@9ba12248...b19a04f5` | commit range comparison | @@ -274,7 +274,7 @@ It also has a shorthand version to reference other projects from the same namesp |:------------------------------|:------------------------| | `project#123` | issue | | `project!123` | merge request | -| `project%123` | milestone | +| `project%123` | project milestone | | `project$123` | snippet | | `project@9ba12248` | specific commit | | `project@9ba12248...b19a04f5` | commit range comparison | diff --git a/doc/user/project/milestones/index.md b/doc/user/project/milestones/index.md index 23ffde4e8bd..876b98a4dc5 100644 --- a/doc/user/project/milestones/index.md +++ b/doc/user/project/milestones/index.md @@ -56,4 +56,5 @@ total merge requests and issues. ## Quick actions -[Quick actions](../quick_actions.md) are available for assigning and removing project milestones only. [In the future](https://gitlab.com/gitlab-org/gitlab-ce/issues/34778), this will also apply to group milestones. +[Quick actions](../quick_actions.md) are available for assigning and removing +project and group milestones. diff --git a/lib/banzai/filter/abstract_reference_filter.rb b/lib/banzai/filter/abstract_reference_filter.rb index 685b43605ae..ef4578aabd6 100644 --- a/lib/banzai/filter/abstract_reference_filter.rb +++ b/lib/banzai/filter/abstract_reference_filter.rb @@ -54,42 +54,42 @@ module Banzai self.class.references_in(*args, &block) end + # Implement in child class + # Example: project.merge_requests.find def find_object(project, id) - # Implement in child class - # Example: project.merge_requests.find end - def find_object_cached(project, id) - if RequestStore.active? - cache = find_objects_cache[object_class][project.id] + # Override if the link reference pattern produces a different ID (global + # ID vs internal ID, for instance) to the regular reference pattern. + def find_object_from_link(project, id) + find_object(project, id) + end - get_or_set_cache(cache, id) { find_object(project, id) } - else + # Implement in child class + # Example: project_merge_request_url + def url_for_object(object, project) + end + + def find_object_cached(project, id) + cached_call(:banzai_find_object, id, path: [object_class, project.id]) do find_object(project, id) end end - def project_from_ref_cached(ref) - if RequestStore.active? - cache = project_refs_cache - - get_or_set_cache(cache, ref) { project_from_ref(ref) } - else - project_from_ref(ref) + def find_object_from_link_cached(project, id) + cached_call(:banzai_find_object_from_link, id, path: [object_class, project.id]) do + find_object_from_link(project, id) end end - def url_for_object(object, project) - # Implement in child class - # Example: project_merge_request_url + def project_from_ref_cached(ref) + cached_call(:banzai_project_refs, ref) do + project_from_ref(ref) + end end def url_for_object_cached(object, project) - if RequestStore.active? - cache = url_for_object_cache[object_class][project.id] - - get_or_set_cache(cache, object) { url_for_object(object, project) } - else + cached_call(:banzai_url_for_object, object, path: [object_class, project.id]) do url_for_object(object, project) end end @@ -120,7 +120,7 @@ module Banzai if link == inner_html && inner_html =~ /\A#{link_pattern}/ replace_link_node_with_text(node, link) do - object_link_filter(inner_html, link_pattern) + object_link_filter(inner_html, link_pattern, link_reference: true) end next @@ -128,7 +128,7 @@ module Banzai if link =~ /\A#{link_pattern}\z/ replace_link_node_with_href(node, link) do - object_link_filter(link, link_pattern, link_content: inner_html) + object_link_filter(link, link_pattern, link_content: inner_html, link_reference: true) end next @@ -146,15 +146,26 @@ module Banzai # text - String text to replace references in. # pattern - Reference pattern to match against. # link_content - Original content of the link being replaced. + # link_reference - True if this was using the link reference pattern, + # false otherwise. # # Returns a String with references replaced with links. All links # have `gfm` and `gfm-OBJECT_NAME` class names attached for styling. - def object_link_filter(text, pattern, link_content: nil) + def object_link_filter(text, pattern, link_content: nil, link_reference: false) references_in(text, pattern) do |match, id, project_ref, namespace_ref, matches| project_path = full_project_path(namespace_ref, project_ref) project = project_from_ref_cached(project_path) - if project && object = find_object_cached(project, id) + if project + object = + if link_reference + find_object_from_link_cached(project, id) + else + find_object_cached(project, id) + end + end + + if object title = object_link_title(object) klass = reference_class(object_sym) @@ -297,15 +308,17 @@ module Banzai RequestStore[:banzai_project_refs] ||= {} end - def find_objects_cache - RequestStore[:banzai_find_objects_cache] ||= Hash.new do |hash, key| - hash[key] = Hash.new { |h, k| h[k] = {} } - end - end + def cached_call(request_store_key, cache_key, path: []) + if RequestStore.active? + cache = RequestStore[request_store_key] ||= Hash.new do |hash, key| + hash[key] = Hash.new { |h, k| h[k] = {} } + end - def url_for_object_cache - RequestStore[:banzai_url_for_object] ||= Hash.new do |hash, key| - hash[key] = Hash.new { |h, k| h[k] = {} } + cache = cache.dig(*path) if path.any? + + get_or_set_cache(cache, cache_key) { yield } + else + yield end end diff --git a/lib/banzai/filter/milestone_reference_filter.rb b/lib/banzai/filter/milestone_reference_filter.rb index 45c033d32a8..4fc5f211e84 100644 --- a/lib/banzai/filter/milestone_reference_filter.rb +++ b/lib/banzai/filter/milestone_reference_filter.rb @@ -8,8 +8,15 @@ module Banzai Milestone end + # Links to project milestones contain the IID, but when we're handling + # 'regular' references, we need to use the global ID to disambiguate + # between group and project milestones. def find_object(project, id) - project.milestones.find_by(iid: id) + find_milestone_with_finder(project, id: id) + end + + def find_object_from_link(project, iid) + find_milestone_with_finder(project, iid: iid) end def references_in(text, pattern = Milestone.reference_pattern) @@ -22,7 +29,7 @@ module Banzai milestone = find_milestone($~[:project], $~[:namespace], $~[:milestone_iid], $~[:milestone_name]) if milestone - yield match, milestone.iid, $~[:project], $~[:namespace], $~ + yield match, milestone.id, $~[:project], $~[:namespace], $~ else match end @@ -36,7 +43,8 @@ module Banzai return unless project milestone_params = milestone_params(milestone_id, milestone_name) - project.milestones.find_by(milestone_params) + + find_milestone_with_finder(project, milestone_params) end def milestone_params(iid, name) @@ -47,15 +55,27 @@ module Banzai end end + def find_milestone_with_finder(project, params) + finder_params = { project_ids: [project.id], order: nil } + + # We don't support IID lookups for group milestones, because IIDs can + # clash between group and project milestones. + if project.group && !params[:iid] + finder_params[:group_ids] = [project.group.id] + end + + MilestonesFinder.new(finder_params).execute.find_by(params) + end + def url_for_object(milestone, project) - h = Gitlab::Routing.url_helpers - h.project_milestone_url(project, milestone, - only_path: context[:only_path]) + Gitlab::Routing + .url_helpers + .milestone_url(milestone, only_path: context[:only_path]) end def object_link_text(object, matches) milestone_link = escape_once(super) - reference = object.project.to_reference(project) + reference = object.project&.to_reference(project) if reference.present? "#{milestone_link} <i>in #{reference}</i>".html_safe diff --git a/spec/fixtures/markdown.md.erb b/spec/fixtures/markdown.md.erb index 58b43805705..4f46e40ce7a 100644 --- a/spec/fixtures/markdown.md.erb +++ b/spec/fixtures/markdown.md.erb @@ -227,8 +227,11 @@ References should be parseable even inside _<%= merge_request.to_reference %>_ e - Milestone in another project: <%= xmilestone.to_reference(project) %> - Ignored in code: `<%= simple_milestone.to_reference %>` - Ignored in links: [Link to <%= simple_milestone.to_reference %>](#milestone-link) -- Milestone by URL: <%= urls.project_milestone_url(milestone.project, milestone) %> +- Milestone by URL: <%= urls.milestone_url(milestone) %> - Link to milestone by URL: [Milestone](<%= milestone.to_reference %>) +- Group milestone by name: <%= Milestone.reference_prefix %><%= group_milestone.name %> +- Group milestone by name in quotes: <%= group_milestone.to_reference(format: :name) %> +- Group milestone by URL is ignore: <%= urls.milestone_url(group_milestone) %> ### Task Lists diff --git a/spec/helpers/gitlab_routing_helper_spec.rb b/spec/helpers/gitlab_routing_helper_spec.rb index 537e457513f..a44b200c5da 100644 --- a/spec/helpers/gitlab_routing_helper_spec.rb +++ b/spec/helpers/gitlab_routing_helper_spec.rb @@ -63,44 +63,4 @@ describe GitlabRoutingHelper do it { expect(resend_invite_group_member_path(group_member)).to eq resend_invite_group_group_member_path(group_member.source, group_member) } end end - - describe '#milestone_path' do - context 'for a group milestone' do - let(:milestone) { build_stubbed(:milestone, group: group, iid: 1) } - - it 'links to the group milestone page' do - expect(milestone_path(milestone)) - .to eq(group_milestone_path(group, milestone)) - end - end - - context 'for a project milestone' do - let(:milestone) { build_stubbed(:milestone, project: project, iid: 1) } - - it 'links to the project milestone page' do - expect(milestone_path(milestone)) - .to eq(project_milestone_path(project, milestone)) - end - end - end - - describe '#milestone_url' do - context 'for a group milestone' do - let(:milestone) { build_stubbed(:milestone, group: group, iid: 1) } - - it 'links to the group milestone page' do - expect(milestone_url(milestone)) - .to eq(group_milestone_url(group, milestone)) - end - end - - context 'for a project milestone' do - let(:milestone) { build_stubbed(:milestone, project: project, iid: 1) } - - it 'links to the project milestone page' do - expect(milestone_url(milestone)) - .to eq(project_milestone_url(project, milestone)) - end - end - end end diff --git a/spec/helpers/milestones_routing_helper_spec.rb b/spec/helpers/milestones_routing_helper_spec.rb new file mode 100644 index 00000000000..dc13a43c2ab --- /dev/null +++ b/spec/helpers/milestones_routing_helper_spec.rb @@ -0,0 +1,46 @@ +require 'spec_helper' + +describe MilestonesRoutingHelper do + let(:project) { build_stubbed(:project) } + let(:group) { build_stubbed(:group) } + + describe '#milestone_path' do + context 'for a group milestone' do + let(:milestone) { build_stubbed(:milestone, group: group, iid: 1) } + + it 'links to the group milestone page' do + expect(milestone_path(milestone)) + .to eq(group_milestone_path(group, milestone)) + end + end + + context 'for a project milestone' do + let(:milestone) { build_stubbed(:milestone, project: project, iid: 1) } + + it 'links to the project milestone page' do + expect(milestone_path(milestone)) + .to eq(project_milestone_path(project, milestone)) + end + end + end + + describe '#milestone_url' do + context 'for a group milestone' do + let(:milestone) { build_stubbed(:milestone, group: group, iid: 1) } + + it 'links to the group milestone page' do + expect(milestone_url(milestone)) + .to eq(group_milestone_url(group, milestone)) + end + end + + context 'for a project milestone' do + let(:milestone) { build_stubbed(:milestone, project: project, iid: 1) } + + it 'links to the project milestone page' do + expect(milestone_url(milestone)) + .to eq(project_milestone_url(project, milestone)) + end + end + end +end diff --git a/spec/lib/banzai/filter/milestone_reference_filter_spec.rb b/spec/lib/banzai/filter/milestone_reference_filter_spec.rb index 5db77566513..ebd6c79077e 100644 --- a/spec/lib/banzai/filter/milestone_reference_filter_spec.rb +++ b/spec/lib/banzai/filter/milestone_reference_filter_spec.rb @@ -3,57 +3,57 @@ require 'spec_helper' describe Banzai::Filter::MilestoneReferenceFilter do include FilterSpecHelper - let(:project) { create(:project, :public) } - let(:milestone) { create(:milestone, project: project) } - let(:reference) { milestone.to_reference } + let(:group) { create(:group, :public) } + let(:project) { create(:project, :public, group: group) } it 'requires project context' do expect { described_class.call('') }.to raise_error(ArgumentError, /:project/) end - %w(pre code a style).each do |elem| - it "ignores valid references contained inside '#{elem}' element" do - exp = act = "<#{elem}>milestone #{milestone.to_reference}</#{elem}>" - expect(reference_filter(act).to_html).to eq exp + shared_examples 'reference parsing' do + %w(pre code a style).each do |elem| + it "ignores valid references contained inside '#{elem}' element" do + exp = act = "<#{elem}>milestone #{reference}</#{elem}>" + expect(reference_filter(act).to_html).to eq exp + end end - end - it 'includes default classes' do - doc = reference_filter("Milestone #{reference}") - expect(doc.css('a').first.attr('class')).to eq 'gfm gfm-milestone has-tooltip' - end + it 'includes default classes' do + doc = reference_filter("Milestone #{reference}") - it 'includes a data-project attribute' do - doc = reference_filter("Milestone #{reference}") - link = doc.css('a').first + expect(doc.css('a').first.attr('class')).to eq 'gfm gfm-milestone has-tooltip' + end - expect(link).to have_attribute('data-project') - expect(link.attr('data-project')).to eq project.id.to_s - end + it 'includes a data-project attribute' do + doc = reference_filter("Milestone #{reference}") + link = doc.css('a').first - it 'includes a data-milestone attribute' do - doc = reference_filter("See #{reference}") - link = doc.css('a').first + expect(link).to have_attribute('data-project') + expect(link.attr('data-project')).to eq project.id.to_s + end - expect(link).to have_attribute('data-milestone') - expect(link.attr('data-milestone')).to eq milestone.id.to_s - end + it 'includes a data-milestone attribute' do + doc = reference_filter("See #{reference}") + link = doc.css('a').first + + expect(link).to have_attribute('data-milestone') + expect(link.attr('data-milestone')).to eq milestone.id.to_s + end - it 'supports an :only_path context' do - doc = reference_filter("Milestone #{reference}", only_path: true) - link = doc.css('a').first.attr('href') + it 'supports an :only_path context' do + doc = reference_filter("Milestone #{reference}", only_path: true) + link = doc.css('a').first.attr('href') - expect(link).not_to match %r(https?://) - expect(link).to eq urls - .project_milestone_path(project, milestone) + expect(link).not_to match %r(https?://) + expect(link).to eq urls.milestone_path(milestone) + end end - context 'Integer-based references' do + shared_examples 'Integer-based references' do it 'links to a valid reference' do doc = reference_filter("See #{reference}") - expect(doc.css('a').first.attr('href')).to eq urls - .project_milestone_url(project, milestone) + expect(doc.css('a').first.attr('href')).to eq urls.milestone_url(milestone) end it 'links with adjacent text' do @@ -68,15 +68,17 @@ describe Banzai::Filter::MilestoneReferenceFilter do end end - context 'String-based single-word references' do - let(:milestone) { create(:milestone, name: 'gfm', project: project) } + shared_examples 'String-based single-word references' do let(:reference) { "#{Milestone.reference_prefix}#{milestone.name}" } + before do + milestone.update!(name: 'gfm') + end + it 'links to a valid reference' do doc = reference_filter("See #{reference}") - expect(doc.css('a').first.attr('href')).to eq urls - .project_milestone_url(project, milestone) + expect(doc.css('a').first.attr('href')).to eq urls.milestone_url(milestone) expect(doc.text).to eq 'See gfm' end @@ -92,15 +94,17 @@ describe Banzai::Filter::MilestoneReferenceFilter do end end - context 'String-based multi-word references in quotes' do - let(:milestone) { create(:milestone, name: 'gfm references', project: project) } + shared_examples 'String-based multi-word references in quotes' do let(:reference) { milestone.to_reference(format: :name) } + before do + milestone.update!(name: 'gfm references') + end + it 'links to a valid reference' do doc = reference_filter("See #{reference}") - expect(doc.css('a').first.attr('href')).to eq urls - .project_milestone_url(project, milestone) + expect(doc.css('a').first.attr('href')).to eq urls.milestone_url(milestone) expect(doc.text).to eq 'See gfm references' end @@ -116,23 +120,27 @@ describe Banzai::Filter::MilestoneReferenceFilter do end end - describe 'referencing a milestone in a link href' do - let(:reference) { %Q{<a href="#{milestone.to_reference}">Milestone</a>} } + shared_examples 'referencing a milestone in a link href' do + let(:unquoted_reference) { "#{Milestone.reference_prefix}#{milestone.name}" } + let(:link_reference) { %Q{<a href="#{unquoted_reference}">Milestone</a>} } + + before do + milestone.update!(name: 'gfm') + end it 'links to a valid reference' do - doc = reference_filter("See #{reference}") + doc = reference_filter("See #{link_reference}") - expect(doc.css('a').first.attr('href')).to eq urls - .project_milestone_url(project, milestone) + expect(doc.css('a').first.attr('href')).to eq urls.milestone_url(milestone) end it 'links with adjacent text' do - doc = reference_filter("Milestone (#{reference}.)") + doc = reference_filter("Milestone (#{link_reference}.)") expect(doc.to_html).to match(%r(\(<a.+>Milestone</a>\.\))) end it 'includes a data-project attribute' do - doc = reference_filter("Milestone #{reference}") + doc = reference_filter("Milestone #{link_reference}") link = doc.css('a').first expect(link).to have_attribute('data-project') @@ -140,7 +148,7 @@ describe Banzai::Filter::MilestoneReferenceFilter do end it 'includes a data-milestone attribute' do - doc = reference_filter("See #{reference}") + doc = reference_filter("See #{link_reference}") link = doc.css('a').first expect(link).to have_attribute('data-milestone') @@ -148,7 +156,35 @@ describe Banzai::Filter::MilestoneReferenceFilter do end end - describe 'cross-project / cross-namespace complete reference' do + shared_examples 'linking to a milestone as the entire link' do + let(:unquoted_reference) { "#{Milestone.reference_prefix}#{milestone.name}" } + let(:link) { urls.milestone_url(milestone) } + let(:link_reference) { %Q{<a href="#{link}">#{link}</a>} } + + it 'replaces the link text with the milestone reference' do + doc = reference_filter("See #{link}") + + expect(doc.css('a').first.text).to eq(unquoted_reference) + end + + it 'includes a data-project attribute' do + doc = reference_filter("Milestone #{link_reference}") + link = doc.css('a').first + + expect(link).to have_attribute('data-project') + expect(link.attr('data-project')).to eq project.id.to_s + end + + it 'includes a data-milestone attribute' do + doc = reference_filter("See #{link_reference}") + link = doc.css('a').first + + expect(link).to have_attribute('data-milestone') + expect(link.attr('data-milestone')).to eq milestone.id.to_s + end + end + + shared_examples 'cross-project / cross-namespace complete reference' do let(:namespace) { create(:namespace) } let(:another_project) { create(:project, :public, namespace: namespace) } let(:milestone) { create(:milestone, project: another_project) } @@ -184,7 +220,7 @@ describe Banzai::Filter::MilestoneReferenceFilter do end end - describe 'cross-project / same-namespace complete reference' do + shared_examples 'cross-project / same-namespace complete reference' do let(:namespace) { create(:namespace) } let(:project) { create(:project, :public, namespace: namespace) } let(:another_project) { create(:project, :public, namespace: namespace) } @@ -221,7 +257,7 @@ describe Banzai::Filter::MilestoneReferenceFilter do end end - describe 'cross project shorthand reference' do + shared_examples 'cross project shorthand reference' do let(:namespace) { create(:namespace) } let(:project) { create(:project, :public, namespace: namespace) } let(:another_project) { create(:project, :public, namespace: namespace) } @@ -258,27 +294,53 @@ describe Banzai::Filter::MilestoneReferenceFilter do end end - describe 'cross project milestone references' do - let(:another_project) { create(:project, :public) } - let(:project_path) { another_project.full_path } - let(:milestone) { create(:milestone, project: another_project) } - let(:reference) { milestone.to_reference(project) } + context 'project milestones' do + let(:milestone) { create(:milestone, project: project) } + let(:reference) { milestone.to_reference } - let!(:result) { reference_filter("See #{reference}") } + include_examples 'reference parsing' - it 'points to referenced project milestone page' do - expect(result.css('a').first.attr('href')).to eq urls - .project_milestone_url(another_project, milestone) + it_behaves_like 'Integer-based references' + it_behaves_like 'String-based single-word references' + it_behaves_like 'String-based multi-word references in quotes' + it_behaves_like 'referencing a milestone in a link href' + it_behaves_like 'cross-project / cross-namespace complete reference' + it_behaves_like 'cross-project / same-namespace complete reference' + it_behaves_like 'cross project shorthand reference' + end + + context 'group milestones' do + let(:milestone) { create(:milestone, group: group) } + let(:reference) { milestone.to_reference(format: :name) } + + include_examples 'reference parsing' + + it_behaves_like 'String-based single-word references' + it_behaves_like 'String-based multi-word references in quotes' + it_behaves_like 'referencing a milestone in a link href' + + it 'does not support references by IID' do + doc = reference_filter("See #{Milestone.reference_prefix}#{milestone.iid}") + + expect(doc.css('a')).to be_empty end - it 'contains cross project content' do - expect(result.css('a').first.text).to eq "#{milestone.name} in #{project_path}" + it 'does not support references by link' do + doc = reference_filter("See #{urls.milestone_url(milestone)}") + + expect(doc.css('a').first.text).to eq(urls.milestone_url(milestone)) end - it 'escapes the name attribute' do - allow_any_instance_of(Milestone).to receive(:title).and_return(%{"></a>whatever<a title="}) - doc = reference_filter("See #{reference}") - expect(doc.css('a').first.text).to eq "#{milestone.name} in #{project_path}" + it 'does not support cross-project references' do + another_group = create(:group) + another_project = create(:project, :public, group: group) + project_reference = another_project.to_reference(project) + + milestone.update!(group: another_group) + + doc = reference_filter("See #{project_reference}#{reference}") + + expect(doc.css('a')).to be_empty end end end diff --git a/spec/models/milestone_spec.rb b/spec/models/milestone_spec.rb index b48aa9558d5..d3da0107d5c 100644 --- a/spec/models/milestone_spec.rb +++ b/spec/models/milestone_spec.rb @@ -230,16 +230,40 @@ describe Milestone do end describe '#to_reference' do - let(:project) { build(:project, name: 'sample-project') } - let(:milestone) { build(:milestone, iid: 1, project: project) } + let(:group) { build_stubbed(:group) } + let(:project) { build_stubbed(:project, name: 'sample-project') } + let(:another_project) { build_stubbed(:project, name: 'another-project', namespace: project.namespace) } + + context 'for a project 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' + 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 'returns a String reference to the object' do - expect(milestone.to_reference).to eq "%1" + it 'supports a cross-project reference' do + expect(milestone.to_reference(another_project)).to eq 'sample-project%1' + end end - it 'supports a cross-project reference' do - another_project = build(:project, name: 'another-project', namespace: project.namespace) - expect(milestone.to_reference(another_project)).to eq "sample-project%1" + 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 + 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 + expect(milestone.to_reference(another_project, format: :name)).to eq '%"milestone"' + end end end diff --git a/spec/services/projects/autocomplete_service_spec.rb b/spec/services/projects/autocomplete_service_spec.rb index c1f098530bf..426593be428 100644 --- a/spec/services/projects/autocomplete_service_spec.rb +++ b/spec/services/projects/autocomplete_service_spec.rb @@ -88,4 +88,31 @@ describe Projects::AutocompleteService do end end end + + describe '#milestones' do + let(:user) { create(:user) } + let(:group) { create(:group) } + let(:project) { create(:project, group: group) } + let!(:group_milestone) { create(:milestone, group: group) } + let!(:project_milestone) { create(:milestone, project: project) } + + let(:milestone_titles) { described_class.new(project, user).milestones.map(&:title) } + + it 'includes project and group milestones' do + expect(milestone_titles).to eq([group_milestone.title, project_milestone.title]) + end + + it 'does not include closed milestones' do + group_milestone.close + + expect(milestone_titles).to eq([project_milestone.title]) + end + + it 'does not include milestones from other projects in the group' do + other_project = create(:project, group: group) + project_milestone.update!(project: other_project) + + expect(milestone_titles).to eq([group_milestone.title]) + end + end end diff --git a/spec/services/system_note_service_spec.rb b/spec/services/system_note_service_spec.rb index e3805160b04..8f1eb4863d9 100644 --- a/spec/services/system_note_service_spec.rb +++ b/spec/services/system_note_service_spec.rb @@ -3,7 +3,8 @@ require 'spec_helper' describe SystemNoteService do include Gitlab::Routing - let(:project) { create(:project) } + let(:group) { create(:group) } + let(:project) { create(:project, group: group) } let(:author) { create(:user) } let(:noteable) { create(:issue, project: project) } let(:issue) { noteable } @@ -242,25 +243,51 @@ describe SystemNoteService do end describe '.change_milestone' do - subject { described_class.change_milestone(noteable, project, author, milestone) } + context 'for a project milestone' do + subject { described_class.change_milestone(noteable, project, author, milestone) } - let(:milestone) { create(:milestone, project: project) } + let(:milestone) { create(:milestone, project: project) } - it_behaves_like 'a system note' do - let(:action) { 'milestone' } - end + it_behaves_like 'a system note' do + let(:action) { 'milestone' } + end - context 'when milestone added' do - it 'sets the note text' do - expect(subject.note).to eq "changed milestone to #{milestone.to_reference}" + context 'when milestone added' do + it 'sets the note text' do + expect(subject.note).to eq "changed milestone to #{milestone.to_reference}" + end + end + + context 'when milestone removed' do + let(:milestone) { nil } + + it 'sets the note text' do + expect(subject.note).to eq 'removed milestone' + end end end - context 'when milestone removed' do - let(:milestone) { nil } + context 'for a group milestone' do + subject { described_class.change_milestone(noteable, project, author, milestone) } - it 'sets the note text' do - expect(subject.note).to eq 'removed milestone' + let(:milestone) { create(:milestone, group: group) } + + it_behaves_like 'a system note' do + let(:action) { 'milestone' } + end + + context 'when milestone added' do + it 'sets the note text to use the milestone name' do + expect(subject.note).to eq "changed milestone to #{milestone.to_reference(format: :name)}" + end + end + + context 'when milestone removed' do + let(:milestone) { nil } + + it 'sets the note text' do + expect(subject.note).to eq 'removed milestone' + end end end end diff --git a/spec/support/issuable_shared_examples.rb b/spec/support/issuable_shared_examples.rb index 970fe10db2b..42f3b4db23c 100644 --- a/spec/support/issuable_shared_examples.rb +++ b/spec/support/issuable_shared_examples.rb @@ -21,15 +21,15 @@ shared_examples 'system notes for milestones' do create(:group_member, group: group, user: user) end - it 'does not create system note' do + it 'creates a system note' do expect do update_issuable(milestone: group_milestone) - end.not_to change { Note.system.count } + end.to change { Note.system.count }.by(1) end end context 'project milestones' do - it 'creates system note' do + it 'creates a system note' do expect do update_issuable(milestone: create(:milestone)) end.to change { Note.system.count }.by(1) diff --git a/spec/support/markdown_feature.rb b/spec/support/markdown_feature.rb index 21a054af4e1..c90359d7cfa 100644 --- a/spec/support/markdown_feature.rb +++ b/spec/support/markdown_feature.rb @@ -23,7 +23,7 @@ class MarkdownFeature # Direct references ---------------------------------------------------------- def project - @project ||= create(:project, :repository).tap do |project| + @project ||= create(:project, :repository, group: group).tap do |project| project.team << [user, :master] end end @@ -75,6 +75,10 @@ class MarkdownFeature @milestone ||= create(:milestone, name: 'next goal', project: project) end + def group_milestone + @group_milestone ||= create(:milestone, name: 'group-milestone', group: group) + end + # Cross-references ----------------------------------------------------------- def xproject diff --git a/spec/support/matchers/markdown_matchers.rb b/spec/support/matchers/markdown_matchers.rb index 7afa57fb76b..d12b2757427 100644 --- a/spec/support/matchers/markdown_matchers.rb +++ b/spec/support/matchers/markdown_matchers.rb @@ -155,7 +155,7 @@ module MarkdownMatchers set_default_markdown_messages match do |actual| - expect(actual).to have_selector('a.gfm.gfm-milestone', count: 6) + expect(actual).to have_selector('a.gfm.gfm-milestone', count: 8) end end |