diff options
author | Thong Kuah <tkuah@gitlab.com> | 2018-12-10 07:05:23 +0300 |
---|---|---|
committer | Thong Kuah <tkuah@gitlab.com> | 2018-12-16 23:51:53 +0300 |
commit | 0e78834bc939980e40aef65b6b51f29293dab6d9 (patch) | |
tree | 7c028831ce89fa9682e19e8c1143a87a8e6938be | |
parent | 0369e7590428923c0ab2b10a8911f6c60ee93d62 (diff) |
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
-rw-r--r-- | app/helpers/clusters_helper.rb | 36 | ||||
-rw-r--r-- | app/presenters/clusters/cluster_presenter.rb | 42 | ||||
-rw-r--r-- | app/views/clusters/clusters/_cluster.html.haml | 2 | ||||
-rw-r--r-- | spec/helpers/clusters_helper_spec.rb | 72 | ||||
-rw-r--r-- | spec/presenters/clusters/cluster_presenter_spec.rb | 126 |
5 files changed, 168 insertions, 110 deletions
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 } |