diff options
author | Douwe Maan <douwe@gitlab.com> | 2016-03-07 09:16:22 +0000 |
---|---|---|
committer | Douwe Maan <douwe@gitlab.com> | 2016-03-07 09:16:22 +0000 |
commit | 99f08b3f727e9d155ab10ad285fe48e0279fb79e (patch) | |
tree | 9e5c323be1015158f240e7b46c35412d99a74133 | |
parent | eab7892dc1eaca0bcca0fc82fb7da79b81cad39d (diff) | |
parent | b3f533c3a770dd6359ec8ab08a7e562e3311b209 (diff) | |
download | gitlab-ce-99f08b3f727e9d155ab10ad285fe48e0279fb79e.tar.gz |
Merge branch 'feature/cross-project-labels' into 'master'
Add support for cross project references for labels
## Summary
Support for cross project references for labels.
## Rationale
1. Cross project label references are currently not supported in GitLab
1. `to_reference` method signature in `Label` model breaks the abstraction introduced in `Referable`.
`concerns/referable.rb: def to_reference(_from_project = nil)`
Signatures:
```
label.rb: def to_reference(format = :id)
commit_range.rb: def to_reference(from_project = nil)
commit.rb: def to_reference(from_project = nil)
external_issue.rb: def to_reference(_from_project = nil)
group.rb: def to_reference(_from_project = nil)
issue.rb: def to_reference(from_project = nil)
merge_request.rb: def to_reference(from_project = nil)
milestone.rb: def to_reference(from_project = nil)
project.rb: def to_reference(_from_project = nil)
snippet.rb: def to_reference(from_project = nil)
user.rb: def to_reference(_from_project = nil)
```
This MR suggests using `def to_reference(from_project = nil, format: :id)` which makes use of keyword arguments and preserves abstract interface.
1. We need support for cross project label references when we want to move issue to another project
It may happen that issue description, system notes or comments contain reference to label and this reference will be invalid after moving issue to another project and will not be displayed correctly unless we have support for cross project references.
Merge request that needs this feature: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/2831
I think that cross project label references may be useful, (example: `Hey, see our issues for CI in GitLab CE! - gitab-org/gitlab-ce~"CI"`).
cc @JobV @DouweM @rspeicher
See merge request !2966
-rw-r--r-- | CHANGELOG | 1 | ||||
-rw-r--r-- | app/helpers/labels_helper.rb | 15 | ||||
-rw-r--r-- | app/models/label.rb | 44 | ||||
-rw-r--r-- | app/services/system_note_service.rb | 2 | ||||
-rw-r--r-- | doc/markdown/markdown.md | 1 | ||||
-rw-r--r-- | lib/banzai/filter/abstract_reference_filter.rb | 2 | ||||
-rw-r--r-- | lib/banzai/filter/label_reference_filter.rb | 79 | ||||
-rw-r--r-- | spec/fixtures/markdown.md.erb | 2 | ||||
-rw-r--r-- | spec/lib/banzai/filter/label_reference_filter_spec.rb | 27 | ||||
-rw-r--r-- | spec/models/label_spec.rb | 30 |
10 files changed, 128 insertions, 75 deletions
diff --git a/CHANGELOG b/CHANGELOG index b631b8ca5a4..3613f842662 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -9,6 +9,7 @@ v 8.6.0 (unreleased) - Indicate how much an MR diverged from the target branch (Pierre de La Morinerie) - Strip leading and trailing spaces in URL validator (evuez) - Return empty array instead of 404 when commit has no statuses in commit status API + - Add support for cross-project label references - Update documentation to reflect Guest role not being enforced on internal projects - Allow search for logged out users - Don't show Issues/MRs from archived projects in Groups view diff --git a/app/helpers/labels_helper.rb b/app/helpers/labels_helper.rb index 1c7fcc13b42..89a054289e8 100644 --- a/app/helpers/labels_helper.rb +++ b/app/helpers/labels_helper.rb @@ -50,19 +50,25 @@ module LabelsHelper @project.labels.pluck(:title) end - def render_colored_label(label) + def render_colored_label(label, label_suffix = '') label_color = label.color || Label::DEFAULT_COLOR text_color = text_color_for_bg(label_color) # Intentionally not using content_tag here so that this method can be called # by LabelReferenceFilter span = %(<span class="label color-label") + - %( style="background-color: #{label_color}; color: #{text_color}">) + - escape_once(label.name) + '</span>' + %(style="background-color: #{label_color}; color: #{text_color}">) + + %(#{escape_once(label.name)}#{label_suffix}</span>) span.html_safe end + def render_colored_cross_project_label(label) + label_suffix = label.project.name_with_namespace + label_suffix = " <i>in #{escape_once(label_suffix)}</i>" + render_colored_label(label, label_suffix) + end + def suggested_colors [ '#0033CC', @@ -119,5 +125,6 @@ module LabelsHelper end # Required for Banzai::Filter::LabelReferenceFilter - module_function :render_colored_label, :text_color_for_bg, :escape_once + module_function :render_colored_label, :render_colored_cross_project_label, + :text_color_for_bg, :escape_once end diff --git a/app/models/label.rb b/app/models/label.rb index c34f4e4ba60..5ff644b8426 100644 --- a/app/models/label.rb +++ b/app/models/label.rb @@ -48,10 +48,15 @@ class Label < ActiveRecord::Base '~' end + ## # Pattern used to extract label references from text + # + # This pattern supports cross-project references. + # def self.reference_pattern %r{ - #{reference_prefix} + (#{Project.reference_pattern})? + #{Regexp.escape(reference_prefix)} (?: (?<label_id>\d+) | # Integer-based label ID, or (?<label_name> @@ -62,24 +67,31 @@ class Label < ActiveRecord::Base }x end + def self.link_reference_pattern + nil + end + + ## # Returns the String necessary to reference this Label in Markdown # # format - Symbol format to use (default: :id, optional: :name) # - # Note that its argument differs from other objects implementing Referable. If - # a non-Symbol argument is given (such as a Project), it will default to :id. - # # Examples: # - # Label.first.to_reference # => "~1" - # Label.first.to_reference(:name) # => "~\"bug\"" + # Label.first.to_reference # => "~1" + # Label.first.to_reference(format: :name) # => "~\"bug\"" + # Label.first.to_reference(project) # => "gitlab-org/gitlab-ce~1" # # Returns a String - def to_reference(format = :id) - if format == :name && !name.include?('"') - %(#{self.class.reference_prefix}"#{name}") + # + def to_reference(from_project = nil, format: :id) + format_reference = label_format_reference(format) + reference = "#{self.class.reference_prefix}#{format_reference}" + + if cross_project_reference?(from_project) + project.to_reference + reference else - "#{self.class.reference_prefix}#{id}" + reference end end @@ -98,4 +110,16 @@ class Label < ActiveRecord::Base def template? template end + + private + + def label_format_reference(format = :id) + raise StandardError, 'Unknown format' unless [:id, :name].include?(format) + + if format == :name && !name.include?('"') + %("#{name}") + else + id + end + end end diff --git a/app/services/system_note_service.rb b/app/services/system_note_service.rb index edced010811..58a861ee08e 100644 --- a/app/services/system_note_service.rb +++ b/app/services/system_note_service.rb @@ -66,7 +66,7 @@ class SystemNoteService def self.change_label(noteable, project, author, added_labels, removed_labels) labels_count = added_labels.count + removed_labels.count - references = ->(label) { label.to_reference(:id) } + references = ->(label) { label.to_reference(format: :id) } added_labels = added_labels.map(&references).join(' ') removed_labels = removed_labels.map(&references).join(' ') diff --git a/doc/markdown/markdown.md b/doc/markdown/markdown.md index c400cdac64d..cbf57db5684 100644 --- a/doc/markdown/markdown.md +++ b/doc/markdown/markdown.md @@ -207,6 +207,7 @@ GFM also recognizes certain cross-project references: | `namespace/project$123` | snippet | | `namespace/project@9ba12248` | specific commit | | `namespace/project@9ba12248...b19a04f5` | commit range comparison | +| `namespace/project~"Some label"` | issues with given label | ## Task Lists diff --git a/lib/banzai/filter/abstract_reference_filter.rb b/lib/banzai/filter/abstract_reference_filter.rb index cdbaecf8d90..34c38913474 100644 --- a/lib/banzai/filter/abstract_reference_filter.rb +++ b/lib/banzai/filter/abstract_reference_filter.rb @@ -94,6 +94,8 @@ module Banzai object_link_filter(link, object_class.link_reference_pattern, link_text: text) end end + + doc end # Replace references (like `!123` for merge requests) in text with links diff --git a/lib/banzai/filter/label_reference_filter.rb b/lib/banzai/filter/label_reference_filter.rb index 95e7d209119..8147e5ed3c7 100644 --- a/lib/banzai/filter/label_reference_filter.rb +++ b/lib/banzai/filter/label_reference_filter.rb @@ -1,78 +1,47 @@ module Banzai module Filter # HTML filter that replaces label references with links. - class LabelReferenceFilter < ReferenceFilter - # Public: Find label references in text - # - # LabelReferenceFilter.references_in(text) do |match, id, name| - # "<a href=...>#{Label.find(id)}</a>" - # end - # - # text - String text to search. - # - # Yields the String match, an optional Integer label ID, and an optional - # String label name. - # - # Returns a String replaced with the return of the block. - def self.references_in(text) - text.gsub(Label.reference_pattern) do |match| - yield match, $~[:label_id].to_i, $~[:label_name] - end + class LabelReferenceFilter < AbstractReferenceFilter + def self.object_class + Label end - def self.referenced_by(node) - { label: LazyReference.new(Label, node.attr("data-label")) } + def find_object(project, id) + project.labels.find(id) end - def call - replace_text_nodes_matching(Label.reference_pattern) do |content| - label_link_filter(content) - end - - replace_link_nodes_with_href(Label.reference_pattern) do |link, text| - label_link_filter(link, link_text: text) + def self.references_in(text, pattern = Label.reference_pattern) + text.gsub(pattern) do |match| + yield match, $~[:label_id].to_i, $~[:label_name], $~[:project], $~ end end - # Replace label references in text with links to the label specified. - # - # text - String text to replace references in. - # - # Returns a String with label references replaced with links. All links - # have `gfm` and `gfm-label` class names attached for styling. - def label_link_filter(text, link_text: nil) - project = context[:project] - - self.class.references_in(text) do |match, id, name| - params = label_params(id, name) - - if label = project.labels.find_by(params) - url = url_for_label(project, label) - klass = reference_class(:label) - data = data_attribute( - original: link_text || match, - project: project.id, - label: label.id - ) + def references_in(text, pattern = Label.reference_pattern) + text.gsub(pattern) do |match| + project = project_from_ref($~[:project]) + params = label_params($~[:label_id].to_i, $~[:label_name]) + label = project.labels.find_by(params) - text = link_text || render_colored_label(label) - - %(<a href="#{url}" #{data} - class="#{klass}">#{escape_once(text)}</a>) + if label + yield match, label.id, $~[:project], $~ else match end end end - def url_for_label(project, label) + def url_for_object(label, project) h = Gitlab::Application.routes.url_helpers - h.namespace_project_issues_url( project.namespace, project, label_name: label.name, - only_path: context[:only_path]) + h.namespace_project_issues_url(project.namespace, project, label_name: label.name, + only_path: context[:only_path]) end - def render_colored_label(label) - LabelsHelper.render_colored_label(label) + def object_link_text(object, matches) + if context[:project] == object.project + LabelsHelper.render_colored_label(object) + else + LabelsHelper.render_colored_cross_project_label(object) + end end # Parameters to pass to `Label.find_by` based on the given arguments diff --git a/spec/fixtures/markdown.md.erb b/spec/fixtures/markdown.md.erb index fe6d42acee2..1772cc3f6a4 100644 --- a/spec/fixtures/markdown.md.erb +++ b/spec/fixtures/markdown.md.erb @@ -209,7 +209,7 @@ References should be parseable even inside _<%= merge_request.to_reference %>_ e - Label by ID: <%= simple_label.to_reference %> - Label by name: <%= Label.reference_prefix %><%= simple_label.name %> -- Label by name in quotes: <%= label.to_reference(:name) %> +- Label by name in quotes: <%= label.to_reference(format: :name) %> - Ignored in code: `<%= simple_label.to_reference %>` - Ignored in links: [Link to <%= simple_label.to_reference %>](#label-link) - Link to label by reference: [Label](<%= label.to_reference %>) diff --git a/spec/lib/banzai/filter/label_reference_filter_spec.rb b/spec/lib/banzai/filter/label_reference_filter_spec.rb index b46ccc47605..e2d21f53b7e 100644 --- a/spec/lib/banzai/filter/label_reference_filter_spec.rb +++ b/spec/lib/banzai/filter/label_reference_filter_spec.rb @@ -111,7 +111,7 @@ describe Banzai::Filter::LabelReferenceFilter, lib: true do context 'String-based multi-word references in quotes' do let(:label) { create(:label, name: 'gfm references', project: project) } - let(:reference) { label.to_reference(:name) } + let(:reference) { label.to_reference(format: :name) } it 'links to a valid reference' do doc = reference_filter("See #{reference}") @@ -176,4 +176,29 @@ describe Banzai::Filter::LabelReferenceFilter, lib: true do expect(result[:references][:label]).to eq [label] end end + + describe 'cross project label references' do + let(:another_project) { create(:empty_project, :public) } + let(:project_name) { another_project.name_with_namespace } + let(:label) { create(:label, project: another_project, color: '#00ff00') } + let(:reference) { label.to_reference(project) } + + let!(:result) { reference_filter("See #{reference}") } + + it 'points to referenced project issues page' do + expect(result.css('a').first.attr('href')) + .to eq urls.namespace_project_issues_url(another_project.namespace, + another_project, + label_name: label.name) + end + + it 'has valid color' do + expect(result.css('a span').first.attr('style')) + .to match /background-color: #00ff00/ + end + + it 'contains cross project content' do + expect(result.css('a').first.text).to eq "#{label.name} in #{project_name}" + end + end end diff --git a/spec/models/label_spec.rb b/spec/models/label_spec.rb index 696fbf7e0aa..0614ca1e7c9 100644 --- a/spec/models/label_spec.rb +++ b/spec/models/label_spec.rb @@ -59,18 +59,42 @@ describe Label, models: true do context 'using id' do it 'returns a String reference to the object' do expect(label.to_reference).to eq "~#{label.id}" - expect(label.to_reference(double('project'))).to eq "~#{label.id}" end end context 'using name' do it 'returns a String reference to the object' do - expect(label.to_reference(:name)).to eq %(~"#{label.name}") + expect(label.to_reference(format: :name)).to eq %(~"#{label.name}") end it 'uses id when name contains double quote' do label = create(:label, name: %q{"irony"}) - expect(label.to_reference(:name)).to eq "~#{label.id}" + expect(label.to_reference(format: :name)).to eq "~#{label.id}" + end + end + + context 'using invalid format' do + it 'raises error' do + expect { label.to_reference(format: :invalid) } + .to raise_error StandardError, /Unknown format/ + end + end + + context 'cross project reference' do + let(:project) { create(:project) } + + context 'using name' do + it 'returns cross reference with label name' do + expect(label.to_reference(project, format: :name)) + .to eq %Q(#{label.project.to_reference}~"#{label.name}") + end + end + + context 'using id' do + it 'returns cross reference with label id' do + expect(label.to_reference(project, format: :id)) + .to eq %Q(#{label.project.to_reference}~#{label.id}) + end end end end |