From 7e4f76ea8978bfff42124013cd38ed5ad38873b5 Mon Sep 17 00:00:00 2001
From: Thong Kuah <tkuah@gitlab.com>
Date: Wed, 7 Nov 2018 01:13:33 +1300
Subject: Show clusters of ancestors in cluster list page

Show both the cluster(s) of the clusterable, and the cluster(s) of
ancestor groups.
---
 app/controllers/clusters/clusters_controller.rb    |  4 +-
 app/finders/cluster_ancestors_finder.rb            | 20 +++++++
 .../unreleased/34758-list-ancestor-clusters.yml    |  5 ++
 spec/finders/cluster_ancestors_finder_spec.rb      | 61 ++++++++++++++++++++++
 4 files changed, 88 insertions(+), 2 deletions(-)
 create mode 100644 app/finders/cluster_ancestors_finder.rb
 create mode 100644 changelogs/unreleased/34758-list-ancestor-clusters.yml
 create mode 100644 spec/finders/cluster_ancestors_finder_spec.rb

diff --git a/app/controllers/clusters/clusters_controller.rb b/app/controllers/clusters/clusters_controller.rb
index 9aa8b758539..7a4ed840e3a 100644
--- a/app/controllers/clusters/clusters_controller.rb
+++ b/app/controllers/clusters/clusters_controller.rb
@@ -18,8 +18,8 @@ class Clusters::ClustersController < Clusters::BaseController
   STATUS_POLLING_INTERVAL = 10_000
 
   def index
-    clusters = ClustersFinder.new(clusterable, current_user, :all).execute
-    @clusters = clusters.page(params[:page]).per(20)
+    clusters = ClusterAncestorsFinder.new(clusterable.subject, current_user).execute
+    @clusters = Kaminari.paginate_array(clusters).page(params[:page]).per(20)
   end
 
   def new
diff --git a/app/finders/cluster_ancestors_finder.rb b/app/finders/cluster_ancestors_finder.rb
new file mode 100644
index 00000000000..9f85e6e5c31
--- /dev/null
+++ b/app/finders/cluster_ancestors_finder.rb
@@ -0,0 +1,20 @@
+# frozen_string_literal: true
+
+class ClusterAncestorsFinder
+  def initialize(clusterable, user)
+    @clusterable = clusterable
+    @user = user
+  end
+
+  def execute
+    clusterable.clusters + ancestor_clusters
+  end
+
+  private
+
+  attr_reader :clusterable, :user
+
+  def ancestor_clusters
+    Clusters::Cluster.ancestor_clusters_for_clusterable(clusterable)
+  end
+end
diff --git a/changelogs/unreleased/34758-list-ancestor-clusters.yml b/changelogs/unreleased/34758-list-ancestor-clusters.yml
new file mode 100644
index 00000000000..8fdba7ba90a
--- /dev/null
+++ b/changelogs/unreleased/34758-list-ancestor-clusters.yml
@@ -0,0 +1,5 @@
+---
+title: Show clusters of ancestors in cluster list page
+merge_request: 22996
+author:
+type: changed
diff --git a/spec/finders/cluster_ancestors_finder_spec.rb b/spec/finders/cluster_ancestors_finder_spec.rb
new file mode 100644
index 00000000000..c1897ef9077
--- /dev/null
+++ b/spec/finders/cluster_ancestors_finder_spec.rb
@@ -0,0 +1,61 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe ClusterAncestorsFinder, '#execute' do
+  let(:group) { create(:group) }
+  let(:project) { create(:project, group: group) }
+  let(:user) { create(:user) }
+
+  let!(:project_cluster) do
+    create(:cluster, :provided_by_user, cluster_type: :project_type, projects: [project])
+  end
+
+  let!(:group_cluster) do
+    create(:cluster, :provided_by_user, cluster_type: :group_type, groups: [group])
+  end
+
+  subject { described_class.new(clusterable, user).execute }
+
+  context 'for a project' do
+    let(:clusterable) { project }
+
+    it 'returns the project clusters followed by group clusters' do
+      is_expected.to eq([project_cluster, group_cluster])
+    end
+
+    context 'nested groups', :nested_groups do
+      let(:group) { create(:group, parent: parent_group) }
+      let(:parent_group) { create(:group) }
+
+      let!(:parent_group_cluster) do
+        create(:cluster, :provided_by_user, cluster_type: :group_type, groups: [parent_group])
+      end
+
+      it 'returns the project clusters followed by group clusters ordered ascending the hierarchy' do
+        is_expected.to eq([project_cluster, group_cluster, parent_group_cluster])
+      end
+    end
+  end
+
+  context 'for a group' do
+    let(:clusterable) { group }
+
+    it 'returns the list of group clusters' do
+      is_expected.to eq([group_cluster])
+    end
+
+    context 'nested groups', :nested_groups do
+      let(:group) { create(:group, parent: parent_group) }
+      let(:parent_group) { create(:group) }
+
+      let!(:parent_group_cluster) do
+        create(:cluster, :provided_by_user, cluster_type: :group_type, groups: [parent_group])
+      end
+
+      it 'returns the list of group clusters ordered ascending the hierarchy' do
+        is_expected.to eq([group_cluster, parent_group_cluster])
+      end
+    end
+  end
+end
-- 
cgit v1.2.1


From bfec22a0fa6f5be137738fca87ce2b736ffea6ba Mon Sep 17 00:00:00 2001
From: Mike Greiling <mike@pixelcog.com>
Date: Tue, 4 Dec 2018 15:05:53 -0600
Subject: Correctly enable "Add Kubernetes cluster" button

---
 app/views/clusters/clusters/_buttons.html.haml | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/app/views/clusters/clusters/_buttons.html.haml b/app/views/clusters/clusters/_buttons.html.haml
index db2e247e341..9daf5a95e79 100644
--- a/app/views/clusters/clusters/_buttons.html.haml
+++ b/app/views/clusters/clusters/_buttons.html.haml
@@ -1,4 +1,7 @@
 -# This partial is overridden in EE
 .nav-controls
-  %span.btn.btn-add-cluster.disabled.js-add-cluster
-    = s_("ClusterIntegration|Add Kubernetes cluster")
+  - if clusterable.can_create_cluster? && clusterable.clusters.empty?
+    = link_to s_('ClusterIntegration|Add Kubernetes cluster'), clusterable.new_path, class: 'btn btn-success'
+  - else
+    %span.btn.btn-add-cluster.disabled.js-add-cluster
+      = s_("ClusterIntegration|Add Kubernetes cluster")
-- 
cgit v1.2.1


From 671c5b2e6f4032574e1e0192595ca7791e29973b Mon Sep 17 00:00:00 2001
From: Mike Greiling <mike@pixelcog.com>
Date: Tue, 4 Dec 2018 17:07:10 -0600
Subject: Render group namespaces in clusters index

---
 app/views/clusters/clusters/_cluster.html.haml | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/app/views/clusters/clusters/_cluster.html.haml b/app/views/clusters/clusters/_cluster.html.haml
index e15de3d504d..7d6331e74fb 100644
--- a/app/views/clusters/clusters/_cluster.html.haml
+++ b/app/views/clusters/clusters/_cluster.html.haml
@@ -3,6 +3,12 @@
     .table-section.section-60
       .table-mobile-header{ role: "rowheader" }= s_("ClusterIntegration|Kubernetes cluster")
       .table-mobile-content
+        - if cluster.group_type? && cluster.group.id != clusterable.id
+          - if cluster.group.ancestors.any?
+            = "#{cluster.group.ancestors.first.name} /"
+            - if cluster.group.ancestors.length > 1
+              = "&hellip; /".html_safe
+          = "#{cluster.group.name} /"
         = link_to cluster.name, cluster.show_path
         - unless cluster.enabled?
           %span.badge.badge-danger Connection disabled
-- 
cgit v1.2.1


From 887bbd8e7e788ffba86829d7943c64994264ed9d Mon Sep 17 00:00:00 2001
From: Mike Greiling <mike@pixelcog.com>
Date: Tue, 4 Dec 2018 17:38:03 -0600
Subject: Update link to documentation

Make link clear in note box

Also update strings to be translatable
---
 app/views/clusters/clusters/index.html.haml | 7 +++++++
 locale/gitlab.pot                           | 3 +++
 2 files changed, 10 insertions(+)

diff --git a/app/views/clusters/clusters/index.html.haml b/app/views/clusters/clusters/index.html.haml
index ad6d1d856d6..219f71f7751 100644
--- a/app/views/clusters/clusters/index.html.haml
+++ b/app/views/clusters/clusters/index.html.haml
@@ -11,6 +11,13 @@
       .nav-text
         = s_("ClusterIntegration|Kubernetes clusters can be used to deploy applications and to provide Review Apps for this project")
       = render 'clusters/clusters/buttons'
+
+    - if @clusters.length > clusterable.clusters.length
+      .bs-callout.bs-callout-info
+        = s_("ClusterIntegration|Clusters are utilized by selecting the nearest ancestor with a matching environment scope. For example, project clusters will override group clusters.")
+        %strong
+          = link_to _('More information'), help_page_path('user/group/clusters/', anchor: 'cluster-precedence')
+
     .clusters-table.js-clusters-list
       .gl-responsive-table-row.table-row-header{ role: "row" }
         .table-section.section-60{ role: "rowheader" }
diff --git a/locale/gitlab.pot b/locale/gitlab.pot
index 0584d2006f7..ef1c56a50ab 100644
--- a/locale/gitlab.pot
+++ b/locale/gitlab.pot
@@ -1497,6 +1497,9 @@ msgstr ""
 msgid "ClusterIntegration|Choose which of your environments will use this cluster."
 msgstr ""
 
+msgid "ClusterIntegration|Clusters are utilized by selecting the nearest ancestor with a matching environment scope. For example, project clusters will override group clusters."
+msgstr ""
+
 msgid "ClusterIntegration|Copy API URL"
 msgstr ""
 
-- 
cgit v1.2.1


From 9dc67bf10cf9a00825a1895c2dc37b6df91c7246 Mon Sep 17 00:00:00 2001
From: Thong Kuah <tkuah@gitlab.com>
Date: Thu, 6 Dec 2018 11:32:57 +1300
Subject: Move group path display logic to helper

Add specs
---
 app/helpers/clusters_helper.rb                 | 22 ++++++++
 app/views/clusters/clusters/_cluster.html.haml |  7 +--
 spec/helpers/clusters_helper_spec.rb           | 75 ++++++++++++++++++++++++++
 3 files changed, 98 insertions(+), 6 deletions(-)
 create mode 100644 spec/helpers/clusters_helper_spec.rb

diff --git a/app/helpers/clusters_helper.rb b/app/helpers/clusters_helper.rb
index 916dcb1a308..37735d751ad 100644
--- a/app/helpers/clusters_helper.rb
+++ b/app/helpers/clusters_helper.rb
@@ -6,6 +6,28 @@ 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
+      group_path_shortened(cluster.group)
+    end
+  end
+
+  def group_path_shortened(group)
+    components = group.full_path_components
+
+    breadcrumb = if components.size > 2
+                   [components.first, '&hellip;'.html_safe, components.last]
+                 else
+                   components
+                 end
+
+    breadcrumb.each_with_object(''.html_safe) do |component, string|
+      string.concat(component + ' / ')
+    end
+  end
+
   def render_gcp_signup_offer
     return if Gitlab::CurrentSettings.current_application_settings.hide_third_party_offers?
     return unless show_gcp_signup_offer?
diff --git a/app/views/clusters/clusters/_cluster.html.haml b/app/views/clusters/clusters/_cluster.html.haml
index 7d6331e74fb..94be148b55d 100644
--- a/app/views/clusters/clusters/_cluster.html.haml
+++ b/app/views/clusters/clusters/_cluster.html.haml
@@ -3,12 +3,7 @@
     .table-section.section-60
       .table-mobile-header{ role: "rowheader" }= s_("ClusterIntegration|Kubernetes cluster")
       .table-mobile-content
-        - if cluster.group_type? && cluster.group.id != clusterable.id
-          - if cluster.group.ancestors.any?
-            = "#{cluster.group.ancestors.first.name} /"
-            - if cluster.group.ancestors.length > 1
-              = "&hellip; /".html_safe
-          = "#{cluster.group.name} /"
+        = cluster_group_path_display(cluster, clusterable)
         = link_to cluster.name, cluster.show_path
         - unless cluster.enabled?
           %span.badge.badge-danger Connection disabled
diff --git a/spec/helpers/clusters_helper_spec.rb b/spec/helpers/clusters_helper_spec.rb
new file mode 100644
index 00000000000..c65d32aedb7
--- /dev/null
+++ b/spec/helpers/clusters_helper_spec.rb
@@ -0,0 +1,75 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe ClustersHelper do
+  describe '#cluster_group_path_display' do
+    let(:group) { create(:group, name: 'group') }
+    let(:cluster) { create(:cluster, cluster_type: :group_type, groups: [group]) }
+    let(:clusterable) { group }
+
+    subject { helper.cluster_group_path_display(cluster, clusterable) }
+
+    it 'returns nothing' do
+      is_expected.to be_nil
+    end
+
+    context 'for another clusterable' do
+      let(:clusterable) { create(:group) }
+
+      it 'returns the group path' do
+        is_expected.to eq('group / ')
+      end
+    end
+
+    context 'for a project cluster' do
+      let(:cluster) { create(:cluster, :project) }
+      let(:clusterable) { cluster.project }
+
+      it 'returns nothing' do
+        is_expected.to be_nil
+      end
+    end
+  end
+
+  describe '#group_path_shortened' do
+    let(:group) { create(:group, name: 'group') }
+
+    subject { helper.group_path_shortened(group) }
+
+    it 'returns the group name with trailing slash' do
+      is_expected.to eq('group / ')
+    end
+
+    it 'escapes group name' do
+      expect(CGI).to receive(:escapeHTML).with('group / ').and_call_original
+
+      subject
+    end
+
+    context 'subgroup', :nested_groups do
+      let(:root_group) { create(:group, name: 'root') }
+      let(:group) { create(:group, name: 'group', parent: root_group) }
+
+      it 'returns the full path with trailing slash' do
+        is_expected.to eq('root / group / ')
+      end
+
+      it 'escapes group names' do
+        expect(CGI).to receive(:escapeHTML).with('root / ').and_call_original
+        expect(CGI).to receive(:escapeHTML).with('group / ').and_call_original
+
+        subject
+      end
+
+      context 'deeper nested' do
+        let(:next_group) { create(:group, name: 'next', parent: root_group) }
+        let(:group) { create(:group, name: 'group', parent: next_group) }
+
+        it 'returns a shorted path with trailing slash' do
+          is_expected.to eq('root / &hellip; / group / ')
+        end
+      end
+    end
+  end
+end
-- 
cgit v1.2.1


From aa86223a4a24396a23db00ba88e3950dc77b8410 Mon Sep 17 00:00:00 2001
From: Mayra Cabrera <mcabrera@gitlab.com>
Date: Thu, 6 Dec 2018 14:55:41 -0600
Subject: Cleans cluster views a bit

- Moves logic into ClustersHelp
- Add CSS to ensure compatibility with EE view.
---
 app/helpers/clusters_helper.rb                 | 4 ++++
 app/views/clusters/clusters/_buttons.html.haml | 2 +-
 app/views/clusters/clusters/index.html.haml    | 2 +-
 3 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/app/helpers/clusters_helper.rb b/app/helpers/clusters_helper.rb
index 37735d751ad..6a555f46d8e 100644
--- a/app/helpers/clusters_helper.rb
+++ b/app/helpers/clusters_helper.rb
@@ -36,4 +36,8 @@ module ClustersHelper
       render 'clusters/clusters/gcp_signup_offer_banner'
     end
   end
+
+  def display_clusters_callout?(clusters, clusterable)
+    clusters.length > clusterable.clusters.length
+  end
 end
diff --git a/app/views/clusters/clusters/_buttons.html.haml b/app/views/clusters/clusters/_buttons.html.haml
index 9daf5a95e79..9238903aa10 100644
--- a/app/views/clusters/clusters/_buttons.html.haml
+++ b/app/views/clusters/clusters/_buttons.html.haml
@@ -1,7 +1,7 @@
 -# This partial is overridden in EE
 .nav-controls
   - if clusterable.can_create_cluster? && clusterable.clusters.empty?
-    = link_to s_('ClusterIntegration|Add Kubernetes cluster'), clusterable.new_path, class: 'btn btn-success'
+    = link_to s_('ClusterIntegration|Add Kubernetes cluster'), clusterable.new_path, class: 'btn btn-success js-add-cluster'
   - else
     %span.btn.btn-add-cluster.disabled.js-add-cluster
       = s_("ClusterIntegration|Add Kubernetes cluster")
diff --git a/app/views/clusters/clusters/index.html.haml b/app/views/clusters/clusters/index.html.haml
index 219f71f7751..94565e53e06 100644
--- a/app/views/clusters/clusters/index.html.haml
+++ b/app/views/clusters/clusters/index.html.haml
@@ -12,7 +12,7 @@
         = s_("ClusterIntegration|Kubernetes clusters can be used to deploy applications and to provide Review Apps for this project")
       = render 'clusters/clusters/buttons'
 
-    - if @clusters.length > clusterable.clusters.length
+    - if display_clusters_callout?(@clusters, clusterable)
       .bs-callout.bs-callout-info
         = s_("ClusterIntegration|Clusters are utilized by selecting the nearest ancestor with a matching environment scope. For example, project clusters will override group clusters.")
         %strong
-- 
cgit v1.2.1


From 0369e7590428923c0ab2b10a8911f6c60ee93d62 Mon Sep 17 00:00:00 2001
From: Mayra Cabrera <mcabrera@gitlab.com>
Date: Fri, 7 Dec 2018 08:28:50 -0600
Subject: Refactor methods on ClusterHelper

- Use sprite_icon instead of hardcoded html
- Use a more descriptive method name
- Changes specs to use html_safe matcher
---
 app/helpers/clusters_helper.rb                 | 44 +++++++++-----
 app/views/clusters/clusters/_cluster.html.haml |  1 -
 app/views/clusters/clusters/index.html.haml    |  2 +-
 spec/helpers/clusters_helper_spec.rb           | 79 +++++++++++++-------------
 4 files changed, 68 insertions(+), 58 deletions(-)

diff --git a/app/helpers/clusters_helper.rb b/app/helpers/clusters_helper.rb
index 6a555f46d8e..ae6c71e0019 100644
--- a/app/helpers/clusters_helper.rb
+++ b/app/helpers/clusters_helper.rb
@@ -10,21 +10,11 @@ module ClustersHelper
   # clusterable, only for the ancestor clusters.
   def cluster_group_path_display(cluster, clusterable)
     if cluster.group_type? && cluster.group.id != clusterable.id
-      group_path_shortened(cluster.group)
-    end
-  end
-
-  def group_path_shortened(group)
-    components = group.full_path_components
-
-    breadcrumb = if components.size > 2
-                   [components.first, '&hellip;'.html_safe, components.last]
-                 else
-                   components
-                 end
+      components = cluster.group.full_path_components
 
-    breadcrumb.each_with_object(''.html_safe) do |component, string|
-      string.concat(component + ' / ')
+      group_path_shortened(components) + ' / ' + link_to_cluster(cluster)
+    else
+      link_to_cluster(cluster)
     end
   end
 
@@ -37,7 +27,31 @@ module ClustersHelper
     end
   end
 
-  def display_clusters_callout?(clusters, clusterable)
+  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
diff --git a/app/views/clusters/clusters/_cluster.html.haml b/app/views/clusters/clusters/_cluster.html.haml
index 94be148b55d..98715bdf655 100644
--- a/app/views/clusters/clusters/_cluster.html.haml
+++ b/app/views/clusters/clusters/_cluster.html.haml
@@ -4,7 +4,6 @@
       .table-mobile-header{ role: "rowheader" }= s_("ClusterIntegration|Kubernetes cluster")
       .table-mobile-content
         = cluster_group_path_display(cluster, clusterable)
-        = link_to cluster.name, cluster.show_path
         - unless cluster.enabled?
           %span.badge.badge-danger Connection disabled
     .table-section.section-25
diff --git a/app/views/clusters/clusters/index.html.haml b/app/views/clusters/clusters/index.html.haml
index 94565e53e06..932395eb50e 100644
--- a/app/views/clusters/clusters/index.html.haml
+++ b/app/views/clusters/clusters/index.html.haml
@@ -12,7 +12,7 @@
         = s_("ClusterIntegration|Kubernetes clusters can be used to deploy applications and to provide Review Apps for this project")
       = render 'clusters/clusters/buttons'
 
-    - if display_clusters_callout?(@clusters, clusterable)
+    - if render_cluster_help_content?(@clusters, clusterable)
       .bs-callout.bs-callout-info
         = s_("ClusterIntegration|Clusters are utilized by selecting the nearest ancestor with a matching environment scope. For example, project clusters will override group clusters.")
         %strong
diff --git a/spec/helpers/clusters_helper_spec.rb b/spec/helpers/clusters_helper_spec.rb
index c65d32aedb7..776007666c0 100644
--- a/spec/helpers/clusters_helper_spec.rb
+++ b/spec/helpers/clusters_helper_spec.rb
@@ -4,70 +4,67 @@ require 'spec_helper'
 
 describe ClustersHelper do
   describe '#cluster_group_path_display' do
-    let(:group) { create(:group, name: 'group') }
-    let(:cluster) { create(:cluster, cluster_type: :group_type, groups: [group]) }
-    let(:clusterable) { group }
+    subject { helper.cluster_group_path_display(cluster.present, clusterable) }
 
-    subject { helper.cluster_group_path_display(cluster, 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 nothing' do
-      is_expected.to be_nil
-    end
-
-    context 'for another clusterable' do
-      let(:clusterable) { create(:group) }
+      it 'returns link for cluster' do
+        expect(subject).to eq(cluster_link)
+      end
 
-      it 'returns the group path' do
-        is_expected.to eq('group / ')
+      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 nothing' do
-        is_expected.to be_nil
+      it 'returns link for cluster' do
+        expect(subject).to eq(cluster_link)
       end
-    end
-  end
-
-  describe '#group_path_shortened' do
-    let(:group) { create(:group, name: 'group') }
 
-    subject { helper.group_path_shortened(group) }
-
-    it 'returns the group name with trailing slash' do
-      is_expected.to eq('group / ')
+      it 'escapes group name' do
+        expect(subject).to be_html_safe
+      end
     end
 
-    it 'escapes group name' do
-      expect(CGI).to receive(:escapeHTML).with('group / ').and_call_original
+    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
-    end
+      subject { helper.cluster_group_path_display(cluster.present, clusterable) }
 
-    context 'subgroup', :nested_groups do
-      let(:root_group) { create(:group, name: 'root') }
-      let(:group) { create(:group, name: 'group', parent: root_group) }
+      context 'with just one group' do
+        let(:cluster_link) { "<a href=\"/groups/root_group/-/clusters/#{cluster.id}\">#{cluster.name}</a>" }
 
-      it 'returns the full path with trailing slash' do
-        is_expected.to eq('root / group / ')
+        it 'returns the group path' do
+          expect(subject).to eq("root_group / #{cluster_link}")
+        end
       end
 
-      it 'escapes group names' do
-        expect(CGI).to receive(:escapeHTML).with('root / ').and_call_original
-        expect(CGI).to receive(:escapeHTML).with('group / ').and_call_original
+      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]) }
 
-        subject
+        it 'returns the full path with trailing slash' do
+          expect(subject).to include('root_group / sub_group /')
+        end
       end
 
-      context 'deeper nested' do
-        let(:next_group) { create(:group, name: 'next', parent: root_group) }
-        let(:group) { create(:group, name: 'group', parent: next_group) }
+      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 'returns a shorted path with trailing slash' do
-          is_expected.to eq('root / &hellip; / group / ')
+        it 'includes an horizontal ellipsis' do
+          expect(subject).to include('ellipsis_h')
         end
       end
     end
-- 
cgit v1.2.1


From 0e78834bc939980e40aef65b6b51f29293dab6d9 Mon Sep 17 00:00:00 2001
From: Thong Kuah <tkuah@gitlab.com>
Date: Mon, 10 Dec 2018 17:05:23 +1300
Subject: 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                     |  36 ------
 app/presenters/clusters/cluster_presenter.rb       |  42 +++++++
 app/views/clusters/clusters/_cluster.html.haml     |   2 +-
 spec/helpers/clusters_helper_spec.rb               |  72 ------------
 spec/presenters/clusters/cluster_presenter_spec.rb | 126 ++++++++++++++++++++-
 5 files changed, 168 insertions(+), 110 deletions(-)
 delete mode 100644 spec/helpers/clusters_helper_spec.rb

diff --git a/app/helpers/clusters_helper.rb b/app/helpers/clusters_helper.rb
index ae6c71e0019..fac606ee8db 100644
--- a/app/helpers/clusters_helper.rb
+++ b/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
diff --git a/app/presenters/clusters/cluster_presenter.rb b/app/presenters/clusters/cluster_presenter.rb
index 0473bb8c72a..7a5b68f9a4b 100644
--- a/app/presenters/clusters/cluster_presenter.rb
+++ b/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
diff --git a/app/views/clusters/clusters/_cluster.html.haml b/app/views/clusters/clusters/_cluster.html.haml
index 98715bdf655..b89789e9915 100644
--- a/app/views/clusters/clusters/_cluster.html.haml
+++ b/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
diff --git a/spec/helpers/clusters_helper_spec.rb b/spec/helpers/clusters_helper_spec.rb
deleted file mode 100644
index 776007666c0..00000000000
--- a/spec/helpers/clusters_helper_spec.rb
+++ /dev/null
@@ -1,72 +0,0 @@
-# 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
diff --git a/spec/presenters/clusters/cluster_presenter_spec.rb b/spec/presenters/clusters/cluster_presenter_spec.rb
index 63c2ff26a45..754ba0a594c 100644
--- a/spec/presenters/clusters/cluster_presenter_spec.rb
+++ b/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 }
 
-- 
cgit v1.2.1


From 2ad5f999e95ed0627e2c8aea9da670b7da559bab Mon Sep 17 00:00:00 2001
From: Thong Kuah <tkuah@gitlab.com>
Date: Fri, 7 Dec 2018 07:47:41 +1300
Subject: Check can :read_clusters in finder

This is in addtion to the can checks we have in the controller, as a
finder can be used elsewhere in the future.
---
 app/finders/cluster_ancestors_finder.rb       | 12 +++++++++---
 spec/finders/cluster_ancestors_finder_spec.rb | 16 ++++++++++++++++
 2 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/app/finders/cluster_ancestors_finder.rb b/app/finders/cluster_ancestors_finder.rb
index 9f85e6e5c31..586fceb258a 100644
--- a/app/finders/cluster_ancestors_finder.rb
+++ b/app/finders/cluster_ancestors_finder.rb
@@ -1,18 +1,24 @@
 # frozen_string_literal: true
 
 class ClusterAncestorsFinder
-  def initialize(clusterable, user)
+  def initialize(clusterable, current_user)
     @clusterable = clusterable
-    @user = user
+    @current_user = current_user
   end
 
   def execute
+    return [] unless can_read_clusters?
+
     clusterable.clusters + ancestor_clusters
   end
 
   private
 
-  attr_reader :clusterable, :user
+  attr_reader :clusterable, :current_user
+
+  def can_read_clusters?
+    Ability.allowed?(current_user, :read_cluster, clusterable)
+  end
 
   def ancestor_clusters
     Clusters::Cluster.ancestor_clusters_for_clusterable(clusterable)
diff --git a/spec/finders/cluster_ancestors_finder_spec.rb b/spec/finders/cluster_ancestors_finder_spec.rb
index c1897ef9077..332086c42e2 100644
--- a/spec/finders/cluster_ancestors_finder_spec.rb
+++ b/spec/finders/cluster_ancestors_finder_spec.rb
@@ -20,6 +20,10 @@ describe ClusterAncestorsFinder, '#execute' do
   context 'for a project' do
     let(:clusterable) { project }
 
+    before do
+      project.add_maintainer(user)
+    end
+
     it 'returns the project clusters followed by group clusters' do
       is_expected.to eq([project_cluster, group_cluster])
     end
@@ -38,9 +42,21 @@ describe ClusterAncestorsFinder, '#execute' do
     end
   end
 
+  context 'user cannot read clusters for clusterable' do
+    let(:clusterable) { project }
+
+    it 'returns nothing' do
+      is_expected.to be_empty
+    end
+  end
+
   context 'for a group' do
     let(:clusterable) { group }
 
+    before do
+      group.add_maintainer(user)
+    end
+
     it 'returns the list of group clusters' do
       is_expected.to eq([group_cluster])
     end
-- 
cgit v1.2.1


From f82c9dbe44d5d003dbeb084f07615ba26c2294b6 Mon Sep 17 00:00:00 2001
From: Thong Kuah <tkuah@gitlab.com>
Date: Tue, 11 Dec 2018 18:13:29 +1300
Subject: Use finder to decide to show note to user

Given the note is about how to interpret ancestor clusters, use the
finder which actually knows if there are any ancestor clusters to find
out if the note should be shown, rather than passing the same info via a
view to a helper

Added note about Kaminari.paginate_array

Link to followup issue too
---
 app/controllers/clusters/clusters_controller.rb | 14 +++++++++++++-
 app/finders/cluster_ancestors_finder.rb         | 11 ++++++++++-
 app/helpers/clusters_helper.rb                  |  4 ----
 app/views/clusters/clusters/index.html.haml     |  2 +-
 4 files changed, 24 insertions(+), 7 deletions(-)

diff --git a/app/controllers/clusters/clusters_controller.rb b/app/controllers/clusters/clusters_controller.rb
index 7a4ed840e3a..b9717b97640 100644
--- a/app/controllers/clusters/clusters_controller.rb
+++ b/app/controllers/clusters/clusters_controller.rb
@@ -18,8 +18,20 @@ class Clusters::ClustersController < Clusters::BaseController
   STATUS_POLLING_INTERVAL = 10_000
 
   def index
-    clusters = ClusterAncestorsFinder.new(clusterable.subject, current_user).execute
+    finder = ClusterAncestorsFinder.new(clusterable.subject, current_user)
+    clusters = finder.execute
+
+    # Note: We are paginating through an array here but this should OK as:
+    #
+    # In CE, we can have a maximum group nesting depth of 21, so including
+    # project cluster, we can have max 22 clusters for a group hierachy.
+    # In EE (Premium) we can have any number, as multiple clusters are
+    # supported, but the number of clusters are fairly low currently.
+    #
+    # See https://gitlab.com/gitlab-org/gitlab-ce/issues/55260 also.
     @clusters = Kaminari.paginate_array(clusters).page(params[:page]).per(20)
+
+    @has_ancestor_clusters = finder.has_ancestor_clusters?
   end
 
   def new
diff --git a/app/finders/cluster_ancestors_finder.rb b/app/finders/cluster_ancestors_finder.rb
index 586fceb258a..2f9709ee057 100644
--- a/app/finders/cluster_ancestors_finder.rb
+++ b/app/finders/cluster_ancestors_finder.rb
@@ -1,6 +1,8 @@
 # frozen_string_literal: true
 
 class ClusterAncestorsFinder
+  include Gitlab::Utils::StrongMemoize
+
   def initialize(clusterable, current_user)
     @clusterable = clusterable
     @current_user = current_user
@@ -12,6 +14,10 @@ class ClusterAncestorsFinder
     clusterable.clusters + ancestor_clusters
   end
 
+  def has_ancestor_clusters?
+    ancestor_clusters.any?
+  end
+
   private
 
   attr_reader :clusterable, :current_user
@@ -20,7 +26,10 @@ class ClusterAncestorsFinder
     Ability.allowed?(current_user, :read_cluster, clusterable)
   end
 
+  # This unfortunately returns an Array, not a Relation!
   def ancestor_clusters
-    Clusters::Cluster.ancestor_clusters_for_clusterable(clusterable)
+    strong_memoize(:ancestor_clusters) do
+      Clusters::Cluster.ancestor_clusters_for_clusterable(clusterable)
+    end
   end
 end
diff --git a/app/helpers/clusters_helper.rb b/app/helpers/clusters_helper.rb
index fac606ee8db..916dcb1a308 100644
--- a/app/helpers/clusters_helper.rb
+++ b/app/helpers/clusters_helper.rb
@@ -14,8 +14,4 @@ module ClustersHelper
       render 'clusters/clusters/gcp_signup_offer_banner'
     end
   end
-
-  def render_cluster_help_content?(clusters, clusterable)
-    clusters.length > clusterable.clusters.length
-  end
 end
diff --git a/app/views/clusters/clusters/index.html.haml b/app/views/clusters/clusters/index.html.haml
index 932395eb50e..58d0a304363 100644
--- a/app/views/clusters/clusters/index.html.haml
+++ b/app/views/clusters/clusters/index.html.haml
@@ -12,7 +12,7 @@
         = s_("ClusterIntegration|Kubernetes clusters can be used to deploy applications and to provide Review Apps for this project")
       = render 'clusters/clusters/buttons'
 
-    - if render_cluster_help_content?(@clusters, clusterable)
+    - if @has_ancestor_clusters
       .bs-callout.bs-callout-info
         = s_("ClusterIntegration|Clusters are utilized by selecting the nearest ancestor with a matching environment scope. For example, project clusters will override group clusters.")
         %strong
-- 
cgit v1.2.1