Move code to presenter

Part of the code such as #show_path is already present on the presenter.
Also avoid having code in two places (helper and presenter)

Sanitize and assert html_safe. Additional layer of defense - on top of
GitLab already requiring group names to be composed of small set of
chars A-Z, - and spaces.

Only link to cluster if user can read cluster

Make clear that arg is a GroupClusterablePresenter

Add more specs for completeness

app/helpers/clusters_helper.rb

@@ -6,18 +6,6 @@ module ClustersHelper
     false
   end
 
-  # We do not want to show the group path for clusters belonging to the
-  # clusterable, only for the ancestor clusters.
-  def cluster_group_path_display(cluster, clusterable)
-    if cluster.group_type? && cluster.group.id != clusterable.id
-      components = cluster.group.full_path_components
-
-      group_path_shortened(components) + ' / ' + link_to_cluster(cluster)
-    else
-      link_to_cluster(cluster)
-    end
-  end
-
   def render_gcp_signup_offer
     return if Gitlab::CurrentSettings.current_application_settings.hide_third_party_offers?
     return unless show_gcp_signup_offer?
@@ -30,28 +18,4 @@ module ClustersHelper
   def render_cluster_help_content?(clusters, clusterable)
     clusters.length > clusterable.clusters.length
   end
-
-  private
-
-  def components_split_by_horizontal_ellipsis(components)
-    [
-      components.first,
-      sprite_icon('ellipsis_h', size: 12, css_class: 'vertical-align-middle').html_safe,
-      components.last
-    ]
-  end
-
-  def link_to_cluster(cluster)
-    link_to cluster.name, cluster.show_path
-  end
-
-  def group_path_shortened(components)
-    breadcrumb = if components.size > 2
-                   components_split_by_horizontal_ellipsis(components)
-                 else
-                   components
-                 end
-
-    breadcrumb.join(' / ').html_safe
-  end
 end

app/presenters/clusters/cluster_presenter.rb

@@ -2,8 +2,22 @@
 
 module Clusters
   class ClusterPresenter < Gitlab::View::Presenter::Delegated
+    include ActionView::Helpers::SanitizeHelper
+    include ActionView::Helpers::UrlHelper
+    include IconsHelper
+
     presents :cluster
 
+    # We do not want to show the group path for clusters belonging to the
+    # clusterable, only for the ancestor clusters.
+    def item_link(clusterable_presenter)
+      if cluster.group_type? && clusterable != clusterable_presenter.subject
+        contracted_group_name(cluster.group) + ' / ' + link_to_cluster
+      else
+        link_to_cluster
+      end
+    end
+
     def gke_cluster_url
       "https://console.cloud.google.com/kubernetes/clusters/details/#{provider.zone}/#{name}" if gcp?
     end
@@ -12,6 +26,10 @@ module Clusters
       can?(current_user, :update_cluster, cluster) && created?
     end
 
+    def can_read_cluster?
+      can?(current_user, :read_cluster, cluster)
+    end
+
     def cluster_type_description
       if cluster.project_type?
         s_("ClusterIntegration|Project cluster")
@@ -29,5 +47,29 @@ module Clusters
         raise NotImplementedError
       end
     end
+
+    private
+
+    def clusterable
+      if cluster.group_type?
+        cluster.group
+      elsif cluster.project_type?
+        cluster.project
+      end
+    end
+
+    def contracted_group_name(group)
+      sanitize(group.full_name)
+        .sub(%r{\/.*\/}, "/ #{contracted_icon} /")
+        .html_safe
+    end
+
+    def contracted_icon
+      sprite_icon('ellipsis_h', size: 12, css_class: 'vertical-align-middle')
+    end
+
+    def link_to_cluster
+      link_to_if(can_read_cluster?, cluster.name, show_path)
+    end
   end
 end

app/views/clusters/clusters/_cluster.html.haml

@@ -3,7 +3,7 @@
     .table-section.section-60
       .table-mobile-header{ role: "rowheader" }= s_("ClusterIntegration|Kubernetes cluster")
       .table-mobile-content
-        = cluster_group_path_display(cluster, clusterable)
+        = cluster.item_link(clusterable)
         - unless cluster.enabled?
           %span.badge.badge-danger Connection disabled
     .table-section.section-25

spec/helpers/clusters_helper_spec.rb

-# frozen_string_literal: true
-
-require 'spec_helper'
-
-describe ClustersHelper do
-  describe '#cluster_group_path_display' do
-    subject { helper.cluster_group_path_display(cluster.present, clusterable) }
-
-    context 'for a group cluster' do
-      let(:cluster) { create(:cluster, :group) }
-      let(:clusterable) { cluster.group }
-      let(:cluster_link) { "<a href=\"/groups/#{clusterable.name}/-/clusters/#{cluster.id}\">#{cluster.name}</a>" }
-
-      it 'returns link for cluster' do
-        expect(subject).to eq(cluster_link)
-      end
-
-      it 'escapes group name' do
-        expect(subject).to be_html_safe
-      end
-    end
-
-    context 'for a project cluster' do
-      let(:cluster) { create(:cluster, :project) }
-      let(:clusterable) { cluster.project }
-      let(:cluster_link) { "<a href=\"/#{clusterable.namespace.name}/#{clusterable.name}/clusters/#{cluster.id}\">#{cluster.name}</a>" }
-
-      it 'returns link for cluster' do
-        expect(subject).to eq(cluster_link)
-      end
-
-      it 'escapes group name' do
-        expect(subject).to be_html_safe
-      end
-    end
-
-    context 'with subgroups' do
-      let(:root_group) { create(:group, name: 'root_group') }
-      let(:cluster) { create(:cluster, :group, groups: [root_group]) }
-      let(:clusterable) { create(:group, name: 'group', parent: root_group) }
-
-      subject { helper.cluster_group_path_display(cluster.present, clusterable) }
-
-      context 'with just one group' do
-        let(:cluster_link) { "<a href=\"/groups/root_group/-/clusters/#{cluster.id}\">#{cluster.name}</a>" }
-
-        it 'returns the group path' do
-          expect(subject).to eq("root_group / #{cluster_link}")
-        end
-      end
-
-      context 'with multiple parent groups', :nested_groups do
-        let(:sub_group) { create(:group, name: 'sub_group', parent: root_group) }
-        let(:cluster) { create(:cluster, :group, groups: [sub_group]) }
-
-        it 'returns the full path with trailing slash' do
-          expect(subject).to include('root_group / sub_group /')
-        end
-      end
-
-      context 'with deeper nested groups', :nested_groups do
-        let(:sub_group) { create(:group, name: 'sub_group', parent: root_group) }
-        let(:sub_sub_group) { create(:group, name: 'sub_sub_group', parent: sub_group) }
-        let(:cluster) { create(:cluster, :group, groups: [sub_sub_group]) }
-
-        it 'includes an horizontal ellipsis' do
-          expect(subject).to include('ellipsis_h')
-        end
-      end
-    end
-  end
-end

spec/presenters/clusters/cluster_presenter_spec.rb

@@ -4,9 +4,10 @@ describe Clusters::ClusterPresenter do
   include Gitlab::Routing.url_helpers
 
   let(:cluster) { create(:cluster, :provided_by_gcp, :project) }
+  let(:user) { create(:user) }
 
   subject(:presenter) do
-    described_class.new(cluster)
+    described_class.new(cluster, current_user: user)
   end
 
   it 'inherits from Gitlab::View::Presenter::Delegated' do
@@ -27,6 +28,129 @@ describe Clusters::ClusterPresenter do
     end
   end
 
+  describe '#item_link' do
+    let(:clusterable_presenter) { double('ClusterablePresenter', subject: clusterable) }
+
+    subject { presenter.item_link(clusterable_presenter) }
+
+    context 'for a group cluster' do
+      let(:cluster) { create(:cluster, cluster_type: :group_type, groups: [group]) }
+      let(:group) { create(:group, name: 'Foo') }
+      let(:cluster_link) { "<a href=\"#{group_cluster_path(cluster.group, cluster)}\">#{cluster.name}</a>" }
+
+      before do
+        group.add_maintainer(user)
+      end
+
+      shared_examples 'ancestor clusters' do
+        context 'ancestor clusters', :nested_groups do
+          let(:root_group) { create(:group, name: 'Root Group') }
+          let(:parent) { create(:group, name: 'parent', parent: root_group) }
+          let(:child) { create(:group, name: 'child', parent: parent) }
+          let(:group) { create(:group, name: 'group', parent: child) }
+
+          before do
+            root_group.add_maintainer(user)
+          end
+
+          context 'top level group cluster' do
+            let(:cluster) { create(:cluster, cluster_type: :group_type, groups: [root_group]) }
+
+            it 'returns full group names and link for cluster' do
+              expect(subject).to eq("Root Group / #{cluster_link}")
+            end
+
+            it 'is html safe' do
+              expect(presenter).to receive(:sanitize).with('Root Group').and_call_original
+
+              expect(subject).to be_html_safe
+            end
+          end
+
+          context 'first level group cluster' do
+            let(:cluster) { create(:cluster, cluster_type: :group_type, groups: [parent]) }
+
+            it 'returns full group names and link for cluster' do
+              expect(subject).to eq("Root Group / parent / #{cluster_link}")
+            end
+
+            it 'is html safe' do
+              expect(presenter).to receive(:sanitize).with('Root Group / parent').and_call_original
+
+              expect(subject).to be_html_safe
+            end
+          end
+
+          context 'second level group cluster' do
+            let(:cluster) { create(:cluster, cluster_type: :group_type, groups: [child]) }
+
+            let(:ellipsis_h) do
+              /.*ellipsis_h.*/
+            end
+
+            it 'returns clipped group names and link for cluster' do
+              expect(subject).to match("Root Group / #{ellipsis_h} / child / #{cluster_link}")
+            end
+
+            it 'is html safe' do
+              expect(presenter).to receive(:sanitize).with('Root Group / parent / child').and_call_original
+
+              expect(subject).to be_html_safe
+            end
+          end
+        end
+      end
+
+      context 'for a project clusterable' do
+        let(:clusterable) { project }
+        let(:project) { create(:project, group: group) }
+
+        it 'returns the group name and the link for cluster' do
+          expect(subject).to eq("Foo / #{cluster_link}")
+        end
+
+        it 'is html safe' do
+          expect(presenter).to receive(:sanitize).with('Foo').and_call_original
+
+          expect(subject).to be_html_safe
+        end
+
+        include_examples 'ancestor clusters'
+      end
+
+      context 'for the group clusterable for the cluster' do
+        let(:clusterable) { group }
+
+        it 'returns link for cluster' do
+          expect(subject).to eq(cluster_link)
+        end
+
+        include_examples 'ancestor clusters'
+
+        it 'is html safe' do
+          expect(subject).to be_html_safe
+        end
+      end
+    end
+
+    context 'for a project cluster' do
+      let(:cluster) { create(:cluster, :project) }
+      let(:cluster_link) { "<a href=\"#{project_cluster_path(cluster.project, cluster)}\">#{cluster.name}</a>" }
+
+      before do
+        cluster.project.add_maintainer(user)
+      end
+
+      context 'for the project clusterable' do
+        let(:clusterable) { cluster.project }
+
+        it 'returns link for cluster' do
+          expect(subject).to eq(cluster_link)
+        end
+      end
+    end
+  end
+
   describe '#gke_cluster_url' do
     subject { described_class.new(cluster).gke_cluster_url }