diff options
author | Thong Kuah <tkuah@gitlab.com> | 2018-10-30 23:33:43 +1300 |
---|---|---|
committer | Thong Kuah <tkuah@gitlab.com> | 2018-11-01 19:37:32 +1300 |
commit | 1163b235391668d53ae0cea80bc22d40b365e0a7 (patch) | |
tree | ce2cf692f41fba52eb42e767611e130399c85499 | |
parent | 88800abcd8741b07114c2850e00b74fbecfbf90e (diff) | |
download | gitlab-ce-1163b235391668d53ae0cea80bc22d40b365e0a7.tar.gz |
Move view and path concerns to presenters
- Move show path for cluster to ClusterPresenter
- Create ClusterablePresenter to encapsulate logic. Consolidates
scattered methods from BaseController and ClustersHelper into an object.
-rw-r--r-- | app/controllers/clusters/base_controller.rb | 24 | ||||
-rw-r--r-- | app/controllers/clusters_controller.rb | 16 | ||||
-rw-r--r-- | app/helpers/clusters_helper.rb | 19 | ||||
-rw-r--r-- | app/presenters/clusterable_presenter.rb | 30 | ||||
-rw-r--r-- | app/presenters/clusters/cluster_presenter.rb | 8 | ||||
-rw-r--r-- | app/presenters/project_clusterable_presenter.rb | 15 | ||||
-rw-r--r-- | app/views/clusters/_advanced_settings.html.haml | 2 | ||||
-rw-r--r-- | app/views/clusters/_cluster.html.haml | 4 | ||||
-rw-r--r-- | app/views/clusters/_empty_state.html.haml | 4 | ||||
-rw-r--r-- | app/views/clusters/show.html.haml | 14 | ||||
-rw-r--r-- | spec/presenters/clusterable_presenter_spec.rb | 17 | ||||
-rw-r--r-- | spec/presenters/clusters/cluster_presenter_spec.rb | 14 | ||||
-rw-r--r-- | spec/presenters/project_clusterable_presenter_spec.rb | 50 |
13 files changed, 157 insertions, 60 deletions
diff --git a/app/controllers/clusters/base_controller.rb b/app/controllers/clusters/base_controller.rb index 2e9997dfc08..8908b26b914 100644 --- a/app/controllers/clusters/base_controller.rb +++ b/app/controllers/clusters/base_controller.rb @@ -11,7 +11,7 @@ class Clusters::BaseController < ApplicationController layout :determine_layout - helper_method :clusters_page_path, :cluster_page_path, :new_cluster_page_path + helper_method :clusterable private @@ -43,27 +43,7 @@ class Clusters::BaseController < ApplicationController end def clusterable - if project_type? - project - end - end - - def cluster_page_path(cluster) - if project_type? - project_cluster_path(project, cluster) - end - end - - def clusters_page_path - if project_type? - project_clusters_path(project) - end - end - - def new_cluster_page_path - if project_type? - new_project_cluster_path(project) - end + @clusterable ||= ClusterablePresenter.fabricate(project, current_user: current_user) end def project_type? diff --git a/app/controllers/clusters_controller.rb b/app/controllers/clusters_controller.rb index 5290ab1e624..7aad70870ba 100644 --- a/app/controllers/clusters_controller.rb +++ b/app/controllers/clusters_controller.rb @@ -50,7 +50,7 @@ class ClustersController < Clusters::BaseController end format.html do flash[:notice] = _('Kubernetes cluster was successfully updated.') - redirect_to cluster_page_path(cluster) + redirect_to cluster.show_path end end else @@ -64,7 +64,7 @@ class ClustersController < Clusters::BaseController def destroy if cluster.destroy flash[:notice] = _('Kubernetes cluster integration was successfully removed.') - redirect_to clusters_page_path, status: :found + redirect_to clusterable.index_path, status: :found else flash[:notice] = _('Kubernetes cluster integration was not removed.') render :show @@ -75,9 +75,10 @@ class ClustersController < Clusters::BaseController @gcp_cluster = ::Clusters::CreateService .new(current_user, create_gcp_cluster_params) .execute(access_token: token_in_session) + .present(current_user: current_user) if @gcp_cluster.persisted? - redirect_to cluster_page_path(@gcp_cluster) + redirect_to @gcp_cluster.show_path else generate_gcp_authorize_url validate_gcp_token @@ -91,9 +92,10 @@ class ClustersController < Clusters::BaseController @user_cluster = ::Clusters::CreateService .new(current_user, create_user_cluster_params) .execute(access_token: token_in_session) + .present(current_user: current_user) if @user_cluster.persisted? - redirect_to cluster_page_path(@user_cluster) + redirect_to @user_cluster.show_path else generate_gcp_authorize_url validate_gcp_token @@ -148,7 +150,7 @@ class ClustersController < Clusters::BaseController ]).merge( provider_type: :gcp, platform_type: :kubernetes, - clusterable: clusterable + clusterable: clusterable.subject ) end @@ -166,12 +168,12 @@ class ClustersController < Clusters::BaseController ]).merge( provider_type: :user, platform_type: :kubernetes, - clusterable: clusterable + clusterable: clusterable.subject ) end def generate_gcp_authorize_url - state = generate_session_key_redirect(new_cluster_page_path.to_s) + state = generate_session_key_redirect(clusterable.new_path.to_s) @authorize_url = GoogleApi::CloudPlatform::Client.new( nil, callback_google_api_auth_url, diff --git a/app/helpers/clusters_helper.rb b/app/helpers/clusters_helper.rb index 4b4945adc4b..360885fe179 100644 --- a/app/helpers/clusters_helper.rb +++ b/app/helpers/clusters_helper.rb @@ -6,14 +6,6 @@ module ClustersHelper false end - def clusterable - @project - end - - def can_create_cluster? - can?(current_user, :create_cluster, clusterable) - end - def render_gcp_signup_offer return if Gitlab::CurrentSettings.current_application_settings.hide_third_party_offers? return unless show_gcp_signup_offer? @@ -24,17 +16,8 @@ module ClustersHelper end def hidden_clusterable_fields - clusterable_params.map do |key, value| + clusterable.clusterable_params.map do |key, value| hidden_field_tag(key, value) end.reduce(&:safe_concat) end - - def clusterable_params - case clusterable - when Project - { project_id: clusterable.to_param, namespace_id: clusterable.namespace.to_param } - else - {} - end - end end diff --git a/app/presenters/clusterable_presenter.rb b/app/presenters/clusterable_presenter.rb new file mode 100644 index 00000000000..c857d57b003 --- /dev/null +++ b/app/presenters/clusterable_presenter.rb @@ -0,0 +1,30 @@ +# frozen_string_literal: true + +class ClusterablePresenter < Gitlab::View::Presenter::Delegated + presents :clusterable + + def self.fabricate(clusterable, **attributes) + presenter_class = "#{clusterable.class.name}ClusterablePresenter".constantize + attributes_with_presenter_class = attributes.merge(presenter_class: presenter_class) + + Gitlab::View::Presenter::Factory + .new(clusterable, attributes_with_presenter_class) + .fabricate! + end + + def can_create_cluster? + can?(current_user, :create_cluster, clusterable) + end + + def index_path + raise NotImplementedError + end + + def new_path + raise NotImplementedError + end + + def clusterable_params + raise NotImplementedError + end +end diff --git a/app/presenters/clusters/cluster_presenter.rb b/app/presenters/clusters/cluster_presenter.rb index dfdd8e82f97..78d632eb77c 100644 --- a/app/presenters/clusters/cluster_presenter.rb +++ b/app/presenters/clusters/cluster_presenter.rb @@ -11,5 +11,13 @@ module Clusters def can_toggle_cluster? can?(current_user, :update_cluster, cluster) && created? end + + def show_path + if cluster.project_type? + project_cluster_path(project, cluster) + else + raise NotImplementedError + end + end end end diff --git a/app/presenters/project_clusterable_presenter.rb b/app/presenters/project_clusterable_presenter.rb new file mode 100644 index 00000000000..f986b5584a3 --- /dev/null +++ b/app/presenters/project_clusterable_presenter.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +class ProjectClusterablePresenter < ClusterablePresenter + def index_path + project_clusters_path(clusterable) + end + + def new_path + new_project_cluster_path(clusterable) + end + + def clusterable_params + { project_id: clusterable.to_param, namespace_id: clusterable.namespace.to_param } + end +end diff --git a/app/views/clusters/_advanced_settings.html.haml b/app/views/clusters/_advanced_settings.html.haml index c91202d9359..e25076248d2 100644 --- a/app/views/clusters/_advanced_settings.html.haml +++ b/app/views/clusters/_advanced_settings.html.haml @@ -12,4 +12,4 @@ = s_('ClusterIntegration|Remove Kubernetes cluster integration') %p = s_("ClusterIntegration|Remove this Kubernetes cluster's configuration from this project. This will not delete your actual Kubernetes cluster.") - = link_to(s_('ClusterIntegration|Remove integration'), cluster_path(@cluster, clusterable_params), method: :delete, class: 'btn btn-danger', data: { confirm: s_("ClusterIntegration|Are you sure you want to remove this Kubernetes cluster's integration? This will not delete your actual Kubernetes cluster.")}) + = link_to(s_('ClusterIntegration|Remove integration'), cluster_path(@cluster, clusterable.clusterable_params), method: :delete, class: 'btn btn-danger', data: { confirm: s_("ClusterIntegration|Are you sure you want to remove this Kubernetes cluster's integration? This will not delete your actual Kubernetes cluster.")}) diff --git a/app/views/clusters/_cluster.html.haml b/app/views/clusters/_cluster.html.haml index a3dcbc25399..709d6711f33 100644 --- a/app/views/clusters/_cluster.html.haml +++ b/app/views/clusters/_cluster.html.haml @@ -2,7 +2,7 @@ .table-section.section-30 .table-mobile-header{ role: "rowheader" }= s_("ClusterIntegration|Kubernetes cluster") .table-mobile-content - = link_to cluster.name, cluster_page_path(cluster) + = link_to cluster.name, cluster.show_path .table-section.section-30 .table-mobile-header{ role: "rowheader" }= s_("ClusterIntegration|Environment scope") .table-mobile-content= cluster.environment_scope @@ -16,7 +16,7 @@ class: "#{'is-checked' if cluster.enabled?} #{'is-disabled' if !cluster.can_toggle_cluster?}", "aria-label": s_("ClusterIntegration|Toggle Kubernetes Cluster"), disabled: !cluster.can_toggle_cluster?, - data: { endpoint: cluster_path(cluster, clusterable_params.merge(format: :json)) } } + data: { endpoint: cluster_path(cluster, clusterable.clusterable_params.merge(format: :json)) } } %input.js-project-feature-toggle-input{ type: "hidden", value: cluster.enabled? } = icon("spinner spin", class: "loading-icon") %span.toggle-icon diff --git a/app/views/clusters/_empty_state.html.haml b/app/views/clusters/_empty_state.html.haml index 200c19e45b1..800e76d92ef 100644 --- a/app/views/clusters/_empty_state.html.haml +++ b/app/views/clusters/_empty_state.html.haml @@ -7,6 +7,6 @@ - link_to_help_page = link_to(_('Learn more about Kubernetes'), help_page_path('user/project/clusters/index'), target: '_blank', rel: 'noopener noreferrer') %p= s_('ClusterIntegration|Kubernetes clusters allow you to use review apps, deploy your applications, run your pipelines, and much more in an easy way. %{link_to_help_page}').html_safe % { link_to_help_page: link_to_help_page} - - if can_create_cluster? + - if clusterable.can_create_cluster? .text-center - = link_to s_('ClusterIntegration|Add Kubernetes cluster'), new_cluster_page_path, class: 'btn btn-success' + = link_to s_('ClusterIntegration|Add Kubernetes cluster'), clusterable.new_path, class: 'btn btn-success' diff --git a/app/views/clusters/show.html.haml b/app/views/clusters/show.html.haml index dd48ffb8ef7..c0169fbf5a2 100644 --- a/app/views/clusters/show.html.haml +++ b/app/views/clusters/show.html.haml @@ -1,18 +1,18 @@ - @content_class = "limit-container-width" unless fluid_layout -- add_to_breadcrumbs "Kubernetes Clusters", clusters_page_path +- add_to_breadcrumbs "Kubernetes Clusters", clusterable.index_path - breadcrumb_title @cluster.name - page_title _("Kubernetes Cluster") - manage_prometheus_path = edit_project_service_path(@cluster.project, 'prometheus') if @project - expanded = Rails.env.test? -- status_path = status_cluster_path(@cluster.id, clusterable_params.merge(format: :json)) if can?(current_user, :admin_cluster, @cluster) +- status_path = status_cluster_path(@cluster.id, clusterable.clusterable_params.merge(format: :json)) if can?(current_user, :admin_cluster, @cluster) .edit-cluster-form.js-edit-cluster-form{ data: { status_path: status_path, - install_helm_path: install_applications_cluster_path(@cluster, :helm, clusterable_params), - install_ingress_path: install_applications_cluster_path(@cluster, :ingress, clusterable_params), - install_prometheus_path: install_applications_cluster_path(@cluster, :prometheus, clusterable_params), - install_runner_path: install_applications_cluster_path(@cluster, :runner, clusterable_params), - install_jupyter_path: install_applications_cluster_path(@cluster, :jupyter, clusterable_params), + install_helm_path: install_applications_cluster_path(@cluster, :helm, clusterable.clusterable_params), + install_ingress_path: install_applications_cluster_path(@cluster, :ingress, clusterable.clusterable_params), + install_prometheus_path: install_applications_cluster_path(@cluster, :prometheus, clusterable.clusterable_params), + install_runner_path: install_applications_cluster_path(@cluster, :runner, clusterable.clusterable_params), + install_jupyter_path: install_applications_cluster_path(@cluster, :jupyter, clusterable.clusterable_params), toggle_status: @cluster.enabled? ? 'true': 'false', cluster_status: @cluster.status_name, cluster_status_reason: @cluster.status_reason, diff --git a/spec/presenters/clusterable_presenter_spec.rb b/spec/presenters/clusterable_presenter_spec.rb new file mode 100644 index 00000000000..4f4ae5e07c5 --- /dev/null +++ b/spec/presenters/clusterable_presenter_spec.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe ClusterablePresenter do + include Gitlab::Routing.url_helpers + + describe '.fabricate' do + let(:project) { create(:project) } + + subject { described_class.fabricate(project) } + + it 'creates an object from a descendant presenter' do + expect(subject).to be_kind_of(ProjectClusterablePresenter) + end + end +end diff --git a/spec/presenters/clusters/cluster_presenter_spec.rb b/spec/presenters/clusters/cluster_presenter_spec.rb index e96dbfb73c0..7af181f37d5 100644 --- a/spec/presenters/clusters/cluster_presenter_spec.rb +++ b/spec/presenters/clusters/cluster_presenter_spec.rb @@ -1,7 +1,9 @@ require 'spec_helper' describe Clusters::ClusterPresenter do - let(:cluster) { create(:cluster, :provided_by_gcp) } + include Gitlab::Routing.url_helpers + + let(:cluster) { create(:cluster, :provided_by_gcp, :project) } subject(:presenter) do described_class.new(cluster) @@ -71,4 +73,14 @@ describe Clusters::ClusterPresenter do it { is_expected.to eq(false) } end end + + describe '#show_path' do + subject { described_class.new(cluster).show_path } + + context 'project_type cluster' do + let(:project) { cluster.project } + + it { is_expected.to eq(project_cluster_path(project, cluster)) } + end + end end diff --git a/spec/presenters/project_clusterable_presenter_spec.rb b/spec/presenters/project_clusterable_presenter_spec.rb new file mode 100644 index 00000000000..c9045f0175a --- /dev/null +++ b/spec/presenters/project_clusterable_presenter_spec.rb @@ -0,0 +1,50 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe ProjectClusterablePresenter do + include Gitlab::Routing.url_helpers + + let(:presenter) { described_class.new(project) } + let(:project) { create(:project) } + + describe '#can_create_cluster?' do + let(:user) { create(:user) } + + subject { presenter.can_create_cluster? } + + before do + allow(presenter).to receive(:current_user).and_return(user) + end + + context 'when user can create' do + before do + project.add_maintainer(user) + end + + it { is_expected.to be_truthy } + end + + context 'when user cannot create' do + it { is_expected.to be_falsey } + end + end + + describe '#index_path' do + subject { presenter.index_path } + + it { is_expected.to eq(project_clusters_path(project)) } + end + + describe '#new_path' do + subject { presenter.new_path } + + it { is_expected.to eq(new_project_cluster_path(project)) } + end + + describe '#clusterable_params' do + subject { presenter.clusterable_params } + + it { is_expected.to eq({ project_id: project.to_param, namespace_id: project.namespace.to_param }) } + end +end |