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

@@ -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, '…'.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

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

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

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