Merge branch 'jprovazn-label-presenter' into 'master'

[CE] Use presenter for labels in UI

See merge request gitlab-org/gitlab-ce!27374

app/controllers/concerns/milestone_actions.rb

@@ -31,7 +31,7 @@ module MilestoneActions
       format.html { redirect_to milestone_redirect_path }
       format.json do
         render json: tabs_json("shared/milestones/_labels_tab", {
-          labels: @milestone.labels # rubocop:disable Gitlab/ModuleWithInstanceVariables
+          labels: @milestone.labels.map { |label| label.present(issuable_subject: @milestone.parent) } # rubocop:disable Gitlab/ModuleWithInstanceVariables
         })
       end
     end

app/helpers/labels_helper.rb

@@ -13,9 +13,7 @@ module LabelsHelper
 
   # Link to a Label
   #
-  # label   - Label object to link to
-  # subject - Project/Group object which will be used as the context for the
-  #           label's link. If omitted, defaults to the label's own group/project.
+  # label   - LabelPresenter object to link to
   # type    - The type of item the link will point to (:issue or
   #           :merge_request). If omitted, defaults to :issue.
   # block   - An optional block that will be passed to `link_to`, forming the
@@ -40,8 +38,8 @@ module LabelsHelper
   #   link_to_label(label) { "My Custom Label Text" }
   #
   # Returns a String
-  def link_to_label(label, subject: nil, type: :issue, tooltip: true, css_class: nil, &block)
-    link = label_filter_path(subject || label.subject, label, type: type)
+  def link_to_label(label, type: :issue, tooltip: true, css_class: nil, &block)
+    link = label.filter_path(type: type)
 
     if block_given?
       link_to link, class: css_class, &block
@@ -50,34 +48,6 @@ module LabelsHelper
     end
   end
 
-  def label_filter_path(subject, label, type: :issue)
-    case subject
-    when Group
-      send("#{type.to_s.pluralize}_group_path", # rubocop:disable GitlabSecurity/PublicSend
-                  subject,
-                  label_name: [label.name])
-    when Project
-      send("namespace_project_#{type.to_s.pluralize}_path", # rubocop:disable GitlabSecurity/PublicSend
-                  subject.namespace,
-                  subject,
-                  label_name: [label.name])
-    end
-  end
-
-  def edit_label_path(label)
-    case label
-    when GroupLabel then edit_group_label_path(label.group, label)
-    when ProjectLabel then edit_project_label_path(label.project, label)
-    end
-  end
-
-  def destroy_label_path(label)
-    case label
-    when GroupLabel then group_label_path(label.group, label)
-    when ProjectLabel then project_label_path(label.project, label)
-    end
-  end
-
   def render_label(label, tooltip: true, link: nil, css: nil)
     # if scoped label is used then EE wraps label tag with scoped label
     # doc link
@@ -168,10 +138,6 @@ module LabelsHelper
     end
   end
 
-  def can_subscribe_to_label_in_different_levels?(label)
-    defined?(@project) && label.is_a?(GroupLabel)
-  end
-
   def label_subscription_status(label, project)
     return 'group-level' if label.subscribed?(current_user)
     return 'project-level' if label.subscribed?(current_user, project)
@@ -241,8 +207,8 @@ module LabelsHelper
     "#{action} at #{level} level"
   end
 
-  def labels_sorted_by_title(labels)
-    labels.sort_by(&:title)
+  def presented_labels_sorted_by_title(labels, subject)
+    labels.sort_by(&:title).map { |label| label.present(issuable_subject: subject) }
   end
 
   def label_dropdown_data(project, opts = {})

app/models/global_label.rb

 # frozen_string_literal: true
 
 class GlobalLabel
+  include Presentable
+
   attr_accessor :title, :labels
   alias_attribute :name, :title
 
@@ -23,4 +25,8 @@ class GlobalLabel
     @labels = labels
     @first_label = labels.find { |lbl| lbl.description.present? } || labels.first
   end
+
+  def present(attributes)
+    super(attributes.merge(presenter_class: ::LabelPresenter))
+  end
 end

app/models/global_milestone.rb

@@ -8,7 +8,9 @@ class GlobalMilestone
   attr_reader :milestone
   alias_attribute :name, :title
 
-  delegate :title, :state, :due_date, :start_date, :participants, :project, :group, :expires_at, :closed?, :iid, :group_milestone?, :safe_title, :milestoneish_id, to: :milestone
+  delegate :title, :state, :due_date, :start_date, :participants, :project,
+           :group, :expires_at, :closed?, :iid, :group_milestone?, :safe_title,
+           :milestoneish_id, :parent, to: :milestone
 
   def to_hash
     {

app/models/label.rb

@@ -8,6 +8,7 @@ class Label < ApplicationRecord
   include OptionallySearch
   include Sortable
   include FromUnion
+  include Presentable
 
   cache_markdown_field :description, pipeline: :single_line
 
@@ -233,6 +234,10 @@ class Label < ApplicationRecord
     attributes
   end
 
+  def present(attributes)
+    super(attributes.merge(presenter_class: ::LabelPresenter))
+  end
+
   private
 
   def issues_count(user, params = {})

app/presenters/label_presenter.rb

+# frozen_string_literal: true
+
+class LabelPresenter < Gitlab::View::Presenter::Delegated
+  presents :label
+
+  def edit_path
+    case label
+    when GroupLabel then edit_group_label_path(label.group, label)
+    when ProjectLabel then edit_project_label_path(label.project, label)
+    end
+  end
+
+  def destroy_path
+    case label
+    when GroupLabel then group_label_path(label.group, label)
+    when ProjectLabel then project_label_path(label.project, label)
+    end
+  end
+
+  def filter_path(type: :issue)
+    case context_subject
+    when Group
+      send("#{type.to_s.pluralize}_group_path", # rubocop:disable GitlabSecurity/PublicSend
+                  context_subject,
+                  label_name: [label.name])
+    when Project
+      send("namespace_project_#{type.to_s.pluralize}_path", # rubocop:disable GitlabSecurity/PublicSend
+                  context_subject.namespace,
+                  context_subject,
+                  label_name: [label.name])
+    end
+  end
+
+  def can_subscribe_to_label_in_different_levels?
+    issuable_subject.is_a?(Project) && label.is_a?(GroupLabel)
+  end
+
+  private
+
+  def context_subject
+    issuable_subject || label.try(:subject)
+  end
+end

app/views/admin/labels/_label.html.haml

 %li.label-list-item{ id: dom_id(label) }
-  = render "shared/label_row", label: label
+  = render "shared/label_row", label: label.present(issuable_subject: nil)
   .label-actions-list
     = link_to edit_admin_label_path(label), class: 'btn btn-transparent label-action has-tooltip', title: _('Edit'), data: { placement: 'bottom' }, aria_label: _('Edit') do
       = sprite_icon('pencil')

app/views/groups/labels/index.html.haml

@@ -18,7 +18,7 @@
         .other-labels
           %h5= _('Labels')
           %ul.content-list.manage-labels-list.js-other-labels
-            = render partial: 'shared/label', subject: @group, collection: @labels, as: :label, locals: { use_label_priority: false }
+            = render partial: 'shared/label', collection: @labels, as: :label, locals: { use_label_priority: false, subject: @group }
             = paginate @labels, theme: 'gitlab'
       - elsif search.present?
         .nothing-here-block

app/views/projects/issues/_issue.html.haml

@@ -36,8 +36,8 @@
               = issue.due_date.to_s(:medium)
           - if issue.labels.any?
              
-            - labels_sorted_by_title(issue.labels).each do |label|
-              = link_to_label(label, subject: issue.project, css_class: 'label-link')
+            - presented_labels_sorted_by_title(issue.labels, issue.project).each do |label|
+              = link_to_label(label, css_class: 'label-link')
 
       .issuable-meta
         %ul.controls

app/views/projects/labels/index.html.haml

@@ -25,7 +25,7 @@
           #js-priority-labels-empty-state.priority-labels-empty-state{ class: "#{'hidden' unless @prioritized_labels.empty? && search.blank?}" }
             = render 'shared/empty_states/priority_labels'
           - if @prioritized_labels.present?
-            = render partial: 'shared/label', subject: @project, collection: @prioritized_labels, as: :label, locals: { force_priority: true }
+            = render partial: 'shared/label', collection: @prioritized_labels, as: :label, locals: { force_priority: true, subject: @project }
           - elsif search.present?
             .nothing-here-block
               = _('No prioritised labels with such name or description')
@@ -34,7 +34,7 @@
         .other-labels
           %h5{ class: ('hide' if hide) }= _('Other Labels')
           .content-list.manage-labels-list.js-other-labels
-            = render partial: 'shared/label', subject: @project, collection: @labels, as: :label
+            = render partial: 'shared/label', collection: @labels, as: :label, locals: { subject: @project }
             = paginate @labels, theme: 'gitlab'
       - elsif search.present?
         .other-labels

app/views/projects/merge_requests/_merge_request.html.haml

@@ -34,8 +34,8 @@
               = merge_request.target_branch
         - if merge_request.labels.any?
            
-          - labels_sorted_by_title(merge_request.labels).each do |label|
-            = link_to_label(label, subject: merge_request.project, type: :merge_request, css_class: 'label-link')
+          - presented_labels_sorted_by_title(merge_request.labels, merge_request.project).each do |label|
+            = link_to_label(label, type: :merge_request, css_class: 'label-link')
 
     .issuable-meta
       %ul.controls

app/views/shared/_delete_label_modal.html.haml

@@ -9,13 +9,13 @@
       .modal-body
         %p
           %strong= label.name
-          %span will be permanently deleted from #{label.is_a?(ProjectLabel)? label.project.name : label.group.name}. This cannot be undone.
+          %span will be permanently deleted from #{label.subject.name}. This cannot be undone.
 
       .modal-footer
         %a{ href: '#', data: { dismiss: 'modal' }, class: 'btn btn-default' } Cancel
 
         = link_to 'Delete label',
-          destroy_label_path(label),
+          label.destroy_path,
           title: 'Delete',
           method: :delete,
           class: 'btn btn-remove'

app/views/shared/_label.html.haml

+- label = label.present(issuable_subject: local_assigns[:subject])
 - label_css_id = dom_id(label)
 - status = label_subscription_status(label, @project).inquiry if current_user
-- subject = local_assigns[:subject]
 - use_label_priority = local_assigns.fetch(:use_label_priority, false)
 - force_priority = local_assigns.fetch(:force_priority, use_label_priority ? label.priority.present? : false)
 - toggle_subscription_path = toggle_subscription_label_path(label, @project) if current_user
 - tooltip_title = label_status_tooltip(label, status) if status
 
 %li.label-list-item{ id: label_css_id, data: { id: label.id } }
-  = render "shared/label_row", label: label, subject: subject, force_priority: force_priority
+  = render "shared/label_row", label: label, force_priority: force_priority
   %ul.label-actions-list
     - if @project
       %li.inline
@@ -21,7 +21,7 @@
           = sprite_icon('star')
     - if can?(current_user, :admin_label, label)
       %li.inline
-        = link_to edit_label_path(label), class: 'btn btn-transparent label-action edit has-tooltip', title: _('Edit'), data: { placement: 'bottom' }, aria_label: _('Edit') do
+        = link_to label.edit_path, class: 'btn btn-transparent label-action edit has-tooltip', title: _('Edit'), data: { placement: 'bottom' }, aria_label: _('Edit') do
           = sprite_icon('pencil')
     - if can?(current_user, :admin_label, label)
       %li.inline
@@ -48,7 +48,7 @@
                     %button.text-danger.remove-row{ type: 'button' }= _('Delete')
     - if current_user
       %li.inline.label-subscription
-        - if can_subscribe_to_label_in_different_levels?(label)
+        - if label.can_subscribe_to_label_in_different_levels?
           %button.js-unsubscribe-button.label-subscribe-button.btn.btn-default{ class: ('hidden' if status.unsubscribed?), data: { url: toggle_subscription_path, toggle: 'tooltip' }, title: tooltip_title }
             %span= _('Unsubscribe')
           .dropdown.dropdown-group-label{ class: ('hidden' unless status.unsubscribed?) }

app/views/shared/_label_row.html.haml

-- subject = local_assigns[:subject]
 - force_priority = local_assigns.fetch(:force_priority, false)
 - subject_or_group_defined = defined?(@project) || defined?(@group)
 - show_label_issues_link = subject_or_group_defined && show_label_issuables_link?(label, :issues, project: @project)
@@ -14,11 +13,11 @@
     %ul.label-links
       - if show_label_issues_link
         %li.label-link-item.inline
-          = link_to_label(label, subject: subject) { 'Issues' }
+          = link_to_label(label) { 'Issues' }
       - if show_label_merge_requests_link
         ·
         %li.label-link-item.inline
-          = link_to_label(label, subject: subject, type: :merge_request) { _('Merge requests') }
+          = link_to_label(label, type: :merge_request) { _('Merge requests') }
       - if force_priority
         ·
         %li.label-link-item.priority-badge.js-priority-badge.inline.prepend-left-10

app/views/shared/issuable/_sidebar.html.haml

@@ -106,7 +106,7 @@
         .value.issuable-show-labels.dont-hide.hide-collapsed.qa-labels-block{ class: ("has-labels" if selected_labels.any?) }
           - if selected_labels.any?
             - selected_labels.each do |label_hash|
-              = render_label(label_from_hash(label_hash), link: sidebar_label_filter_path(issuable_sidebar[:project_issuables_path], label_hash[:title]))
+              = render_label(label_from_hash(label_hash).present(issuable_subject: nil), link: sidebar_label_filter_path(issuable_sidebar[:project_issuables_path], label_hash[:title]))
           - else
             %span.no-value
               = _('None')

app/views/shared/milestones/_issuable.html.haml

@@ -21,7 +21,7 @@
       %span.issuable-number= issuable.to_reference
 
     - labels.each do |label|
-      = render_label(label, link: polymorphic_path(issuable_type_args, { milestone_title: @milestone.title, label_name: label.title, state: 'all' }))
+      = render_label(label.present(issuable_subject: project), link: polymorphic_path(issuable_type_args, { milestone_title: @milestone.title, label_name: label.title, state: 'all' }))
 
     %span.assignee-icon
       - assignees.each do |assignee|

lib/banzai/filter/label_reference_filter.rb

@@ -82,16 +82,18 @@ module Banzai
 
       def object_link_text(object, matches)
         label_suffix = ''
+        parent = project || group
 
         if project || full_path_ref?(matches)
           project_path    = full_project_path(matches[:namespace], matches[:project])
           parent_from_ref = from_ref_cached(project_path)
-          reference       = parent_from_ref.to_human_reference(project || group)
+          reference       = parent_from_ref.to_human_reference(parent)
 
           label_suffix = " <i>in #{reference}</i>" if reference.present?
         end
 
-        LabelsHelper.render_colored_label(object, label_suffix: label_suffix, title: tooltip_title(object))
+        presenter = object.present(issuable_subject: parent)
+        LabelsHelper.render_colored_label(presenter, label_suffix: label_suffix, title: tooltip_title(presenter))
       end
 
       def tooltip_title(label)

spec/helpers/labels_helper_spec.rb

@@ -67,27 +67,29 @@ describe LabelsHelper do
   describe 'link_to_label' do
     let(:project) { create(:project) }
     let(:label) { create(:label, project: project) }
+    let(:subject) { nil }
+    let(:label_presenter) { label.present(issuable_subject: subject) }
 
     context 'without subject' do
       it "uses the label's project" do
-        expect(link_to_label(label)).to match %r{<a href="/#{label.project.full_path}/issues\?label_name%5B%5D=#{label.name}">.*</a>}
+        expect(link_to_label(label_presenter)).to match %r{<a href="/#{label.project.full_path}/issues\?label_name%5B%5D=#{label.name}">.*</a>}
       end
     end
 
     context 'with a project as subject' do
       let(:namespace) { build(:namespace, name: 'foo3') }
-      let(:another_project) { build(:project, namespace: namespace, name: 'bar3') }
+      let(:subject) { build(:project, namespace: namespace, name: 'bar3') }
 
       it 'links to project issues page' do
-        expect(link_to_label(label, subject: another_project)).to match %r{<a href="/foo3/bar3/issues\?label_name%5B%5D=#{label.name}">.*</a>}
+        expect(link_to_label(label_presenter)).to match %r{<a href="/foo3/bar3/issues\?label_name%5B%5D=#{label.name}">.*</a>}
       end
     end
 
     context 'with a group as subject' do
-      let(:group) { build(:group, name: 'bar') }
+      let(:subject) { build(:group, name: 'bar') }
 
       it 'links to group issues page' do
-        expect(link_to_label(label, subject: group)).to match %r{<a href="/groups/bar/-/issues\?label_name%5B%5D=#{label.name}">.*</a>}
+        expect(link_to_label(label_presenter)).to match %r{<a href="/groups/bar/-/issues\?label_name%5B%5D=#{label.name}">.*</a>}
       end
     end
 
@@ -95,7 +97,7 @@ describe LabelsHelper do
       ['issue', :issue, 'merge_request', :merge_request].each do |type|
         context "set to #{type}" do
           it 'links to correct page' do
-            expect(link_to_label(label, type: type)).to match %r{<a href="/#{label.project.full_path}/#{type.to_s.pluralize}\?label_name%5B%5D=#{label.name}">.*</a>}
+            expect(link_to_label(label_presenter, type: type)).to match %r{<a href="/#{label.project.full_path}/#{type.to_s.pluralize}\?label_name%5B%5D=#{label.name}">.*</a>}
           end
         end
       end
@@ -104,14 +106,14 @@ describe LabelsHelper do
     context 'with a tooltip argument' do
       context 'set to false' do
         it 'does not include the has-tooltip class' do
-          expect(link_to_label(label, tooltip: false)).not_to match /has-tooltip/
+          expect(link_to_label(label_presenter, tooltip: false)).not_to match /has-tooltip/
         end
       end
     end
 
     context 'with block' do
       it 'passes the block to link_to' do
-        link = link_to_label(label) { 'Foo' }
+        link = link_to_label(label_presenter) { 'Foo' }
         expect(link).to match('Foo')
       end
     end
@@ -119,8 +121,8 @@ describe LabelsHelper do
     context 'without block' do
       it 'uses render_colored_label as the link content' do
         expect(self).to receive(:render_colored_label)
-          .with(label, tooltip: true).and_return('Foo')
-        expect(link_to_label(label)).to match('Foo')
+          .with(label_presenter, tooltip: true).and_return('Foo')
+        expect(link_to_label(label_presenter)).to match('Foo')
       end
     end
   end
@@ -237,16 +239,24 @@ describe LabelsHelper do
     end
   end
 
-  describe 'labels_sorted_by_title' do
+  describe 'presented_labels_sorted_by_title' do
+    let(:labels) do
+      [build(:label, title: 'a'),
+       build(:label, title: 'B'),
+       build(:label, title: 'c'),
+       build(:label, title: 'D')]
+    end
+
     it 'sorts labels alphabetically' do
-      label1 = double(:label, title: 'a')
-      label2 = double(:label, title: 'B')
-      label3 = double(:label, title: 'c')
-      label4 = double(:label, title: 'D')
-      labels = [label1, label2, label3, label4]
-
-      expect(labels_sorted_by_title(labels))
-        .to match_array([label2, label4, label1, label3])
+      sorted_ids = presented_labels_sorted_by_title(labels, nil).map(&:id)
+
+      expect(sorted_ids)
+        .to match_array([labels[1].id, labels[3].id, labels[0].id, labels[2].id])
+    end
+
+    it 'returns an array of label presenters' do
+      expect(presented_labels_sorted_by_title(labels, nil))
+        .to all(be_a(LabelPresenter))
     end
   end
 

spec/presenters/label_presenter_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe LabelPresenter do
+  include Gitlab::Routing.url_helpers
+
+  set(:group) { create(:group) }
+  set(:project) { create(:project, group: group) }
+  let(:label) { build_stubbed(:label, project: project).present(issuable_subject: project) }
+  let(:group_label) { build_stubbed(:group_label, group: group).present(issuable_subject: project) }
+
+  describe '#edit_path' do
+    context 'with group label' do
+      subject { group_label.edit_path }
+
+      it { is_expected.to eq(edit_group_label_path(group, group_label)) }
+    end
+
+    context 'with project label' do
+      subject { label.edit_path }
+
+      it { is_expected.to eq(edit_project_label_path(project, label)) }
+    end
+  end
+
+  describe '#destroy_path' do
+    context 'with group label' do
+      subject { group_label.destroy_path }
+
+      it { is_expected.to eq(group_label_path(group, group_label)) }
+    end
+
+    context 'with project label' do
+      subject { label.destroy_path }
+
+      it { is_expected.to eq(project_label_path(project, label)) }
+    end
+  end
+
+  describe '#filter_path' do
+    context 'with group as context subject' do
+      let(:label_in_group) { build_stubbed(:label, project: project).present(issuable_subject: group) }
+      subject { label_in_group.filter_path }
+
+      it { is_expected.to eq(issues_group_path(group, label_name: [label_in_group.title])) }
+    end
+
+    context 'with project as context subject' do
+      subject { label.filter_path }
+
+      it { is_expected.to eq(namespace_project_issues_path(group, project, label_name: [label.title])) }
+    end
+  end
+
+  describe '#can_subscribe_to_label_in_different_levels?' do
+    it 'returns true for group labels in project context' do
+      expect(group_label.can_subscribe_to_label_in_different_levels?).to be_truthy
+    end
+
+    it 'returns false for project labels in project context' do
+      expect(label.can_subscribe_to_label_in_different_levels?).to be_falsey
+    end
+  end
+end