From 2c3e8d39f47a4209a1eed944edb1748157c2806d Mon Sep 17 00:00:00 2001 From: Jose Ivan Vargas Date: Tue, 3 Jan 2017 17:57:55 -0600 Subject: [PATCH 1/4] Port of 26165-combine-runners-and-variables-and-triggers-and-ci-cd-pipelines-settings-pages to EE --- app/assets/javascripts/dispatcher.js.es6 | 2 +- .../projects/pipelines_settings_controller.rb | 11 +--- .../projects/runners_controller.rb | 8 +-- .../projects/settings/ci_cd_controller.rb | 44 +++++++++++++++ .../projects/triggers_controller.rb | 10 ++-- .../projects/variables_controller.rb | 14 +++-- app/helpers/gitlab_routing_helper.rb | 6 +- .../layouts/nav/_project_settings.html.haml | 16 +----- .../{show.html.haml => _show.html.haml} | 6 +- .../{index.html.haml => _index.html.haml} | 6 +- .../runners/_shared_runners.html.haml | 2 +- .../runners/_specific_runners.html.haml | 4 +- .../projects/settings/ci_cd/show.html.haml | 6 ++ .../{index.html.haml => _index.html.haml} | 8 +-- .../{index.html.haml => _index.html.haml} | 8 +-- config/routes/project.rb | 1 + .../projects/settings/ci_cd_controller.rb | 20 +++++++ .../projects/variables_controller_spec.rb | 56 +++++++++++++++++++ .../security/project/internal_access_spec.rb | 14 +++++ .../security/project/private_access_spec.rb | 14 +++++ .../security/project/public_access_spec.rb | 14 +++++ spec/features/triggers_spec.rb | 2 +- spec/features/variables_spec.rb | 2 +- 23 files changed, 209 insertions(+), 65 deletions(-) create mode 100644 app/controllers/projects/settings/ci_cd_controller.rb rename app/views/projects/pipelines_settings/{show.html.haml => _show.html.haml} (97%) rename app/views/projects/runners/{index.html.haml => _index.html.haml} (87%) create mode 100644 app/views/projects/settings/ci_cd/show.html.haml rename app/views/projects/triggers/{index.html.haml => _index.html.haml} (93%) rename app/views/projects/variables/{index.html.haml => _index.html.haml} (74%) create mode 100644 spec/controllers/projects/settings/ci_cd_controller.rb create mode 100644 spec/controllers/projects/variables_controller_spec.rb diff --git a/app/assets/javascripts/dispatcher.js.es6 b/app/assets/javascripts/dispatcher.js.es6 index 709f1c513e20c2..cc3dd1f5f0d275 100644 --- a/app/assets/javascripts/dispatcher.js.es6 +++ b/app/assets/javascripts/dispatcher.js.es6 @@ -269,7 +269,7 @@ new gl.ProtectedBranchCreate(); new gl.ProtectedBranchEditList(); break; - case 'projects:variables:index': + case 'projects:ci_cd:show': new gl.ProjectVariables(); break; case 'ci:lints:create': diff --git a/app/controllers/projects/pipelines_settings_controller.rb b/app/controllers/projects/pipelines_settings_controller.rb index 53ce23221edf85..c8c80551ac9663 100644 --- a/app/controllers/projects/pipelines_settings_controller.rb +++ b/app/controllers/projects/pipelines_settings_controller.rb @@ -2,20 +2,13 @@ class Projects::PipelinesSettingsController < Projects::ApplicationController before_action :authorize_admin_pipeline! def show - @ref = params[:ref] || @project.default_branch || 'master' - - @badges = [Gitlab::Badge::Build::Status, - Gitlab::Badge::Coverage::Report] - - @badges.map! do |badge| - badge.new(@project, @ref).metadata - end + redirect_to namespace_project_settings_ci_cd_path(@project.namespace, @project, params: params) end def update if @project.update_attributes(update_params) flash[:notice] = "CI/CD Pipelines settings for '#{@project.name}' were successfully updated." - redirect_to namespace_project_pipelines_settings_path(@project.namespace, @project) + redirect_to namespace_project_settings_ci_cd_path(@project.namespace, @project) else render 'show' end diff --git a/app/controllers/projects/runners_controller.rb b/app/controllers/projects/runners_controller.rb index 53c36635efe1ec..74c54037ba9bcf 100644 --- a/app/controllers/projects/runners_controller.rb +++ b/app/controllers/projects/runners_controller.rb @@ -5,11 +5,7 @@ class Projects::RunnersController < Projects::ApplicationController layout 'project_settings' def index - @project_runners = project.runners.ordered - @assignable_runners = current_user.ci_authorized_runners. - assignable_for(project).ordered.page(params[:page]).per(20) - @shared_runners = Ci::Runner.shared.active - @shared_runners_count = @shared_runners.count(:all) + redirect_to namespace_project_settings_ci_cd_path(@project.namespace, @project) end def edit @@ -53,7 +49,7 @@ def show def toggle_shared_runners project.toggle!(:shared_runners_enabled) - redirect_to namespace_project_runners_path(project.namespace, project) + redirect_to namespace_project_settings_ci_cd_path(@project.namespace, @project) end protected diff --git a/app/controllers/projects/settings/ci_cd_controller.rb b/app/controllers/projects/settings/ci_cd_controller.rb new file mode 100644 index 00000000000000..6f009d6195062c --- /dev/null +++ b/app/controllers/projects/settings/ci_cd_controller.rb @@ -0,0 +1,44 @@ +module Projects + module Settings + class CiCdController < Projects::ApplicationController + before_action :authorize_admin_pipeline! + + def show + define_runners_variables + define_secret_variables + define_triggers_variables + define_badges_variables + end + + private + + def define_runners_variables + @project_runners = @project.runners.ordered + @assignable_runners = current_user.ci_authorized_runners. + assignable_for(project).ordered.page(params[:page]).per(20) + @shared_runners = Ci::Runner.shared.active + @shared_runners_count = @shared_runners.count(:all) + end + + def define_secret_variables + @variable = Ci::Variable.new + end + + def define_triggers_variables + @triggers = @project.triggers + @trigger = Ci::Trigger.new + end + + def define_badges_variables + @ref = params[:ref] || @project.default_branch || 'master' + + @badges = [Gitlab::Badge::Build::Status, + Gitlab::Badge::Coverage::Report] + + @badges.map! do |badge| + badge.new(@project, @ref).metadata + end + end + end + end +end diff --git a/app/controllers/projects/triggers_controller.rb b/app/controllers/projects/triggers_controller.rb index 92359745cec883..224a25eec99ec2 100644 --- a/app/controllers/projects/triggers_controller.rb +++ b/app/controllers/projects/triggers_controller.rb @@ -4,8 +4,7 @@ class Projects::TriggersController < Projects::ApplicationController layout 'project_settings' def index - @triggers = project.triggers - @trigger = Ci::Trigger.new + redirect_to namespace_project_settings_ci_cd_path(@project.namespace, @project) end def create @@ -13,17 +12,18 @@ def create @trigger.save if @trigger.valid? - redirect_to namespace_project_triggers_path(@project.namespace, @project) + flash[:notice] = "Trigger has been created successfully" else @triggers = project.triggers.select(&:persisted?) - render :index end + redirect_to namespace_project_settings_ci_cd_path(@project.namespace, @project) end def destroy trigger.destroy + flash[:alert] = "Trigger removed" - redirect_to namespace_project_triggers_path(@project.namespace, @project) + redirect_to namespace_project_settings_ci_cd_path(@project.namespace, @project) end private diff --git a/app/controllers/projects/variables_controller.rb b/app/controllers/projects/variables_controller.rb index 6f0687293907c6..d07ef7889ca148 100644 --- a/app/controllers/projects/variables_controller.rb +++ b/app/controllers/projects/variables_controller.rb @@ -4,7 +4,7 @@ class Projects::VariablesController < Projects::ApplicationController layout 'project_settings' def index - @variable = Ci::Variable.new + redirect_to namespace_project_settings_ci_cd_path(@project.namespace, @project) end def show @@ -15,27 +15,29 @@ def update @variable = @project.variables.find(params[:id]) if @variable.update_attributes(project_params) - redirect_to namespace_project_variables_path(project.namespace, project), notice: 'Variable was successfully updated.' + flash[:notice] = 'Variables were successfully updated.' else - render action: "show" + flash[:alert] = @variable.errors.full_messages.join(',').html_safe end + redirect_to namespace_project_settings_ci_cd_path(project.namespace, project) end def create @variable = Ci::Variable.new(project_params) if @variable.valid? && @project.variables << @variable - redirect_to namespace_project_variables_path(project.namespace, project), notice: 'Variables were successfully updated.' + flash[:notice] = 'Variables were successfully updated.' else - render action: "index" + flash[:alert] = @variable.errors.full_messages.join(',').html_safe end + redirect_to namespace_project_settings_ci_cd_path(project.namespace, project) end def destroy @key = @project.variables.find(params[:id]) @key.destroy - redirect_to namespace_project_variables_path(project.namespace, project), notice: 'Variable was successfully removed.' + redirect_to namespace_project_settings_ci_cd_path(project.namespace, project), notice: 'Variable was successfully removed.' end private diff --git a/app/helpers/gitlab_routing_helper.rb b/app/helpers/gitlab_routing_helper.rb index 2159e4ce21a42c..f16a63e2178921 100644 --- a/app/helpers/gitlab_routing_helper.rb +++ b/app/helpers/gitlab_routing_helper.rb @@ -211,8 +211,12 @@ def artifacts_action_path(path, project, build) def project_settings_integrations_path(project, *args) namespace_project_settings_integrations_path(project.namespace, project, *args) end - + def project_settings_members_path(project, *args) namespace_project_settings_members_path(project.namespace, project, *args) end + + def project_settings_ci_cd_path(project, *args) + namespace_project_settings_ci_cd_path(project.namespace, project, *args) + end end diff --git a/app/views/layouts/nav/_project_settings.html.haml b/app/views/layouts/nav/_project_settings.html.haml index c2281f4ee9b1d3..b7fe5b90dcf64d 100644 --- a/app/views/layouts/nav/_project_settings.html.haml +++ b/app/views/layouts/nav/_project_settings.html.haml @@ -18,20 +18,8 @@ Protected Branches - if @project.feature_available?(:builds, current_user) - = nav_link(controller: :runners) do - = link_to namespace_project_runners_path(@project.namespace, @project), title: 'Runners' do - %span - Runners - = nav_link(controller: :variables) do - = link_to namespace_project_variables_path(@project.namespace, @project), title: 'Variables' do - %span - Variables - = nav_link(controller: :triggers) do - = link_to namespace_project_triggers_path(@project.namespace, @project), title: 'Triggers' do - %span - Triggers - = nav_link(controller: :pipelines_settings) do - = link_to namespace_project_pipelines_settings_path(@project.namespace, @project), title: 'CI/CD Pipelines' do + = nav_link(controller: :ci_cd) do + = link_to namespace_project_settings_ci_cd_path(@project.namespace, @project), title: 'CI/CD Pipelines' do %span CI/CD Pipelines = nav_link(controller: :push_rules) do diff --git a/app/views/projects/pipelines_settings/show.html.haml b/app/views/projects/pipelines_settings/_show.html.haml similarity index 97% rename from app/views/projects/pipelines_settings/show.html.haml rename to app/views/projects/pipelines_settings/_show.html.haml index 18328c67f0243a..8024fb8979d6fa 100644 --- a/app/views/projects/pipelines_settings/show.html.haml +++ b/app/views/projects/pipelines_settings/_show.html.haml @@ -1,9 +1,7 @@ -- page_title "CI/CD Pipelines" - .row.prepend-top-default .col-lg-3.profile-settings-sidebar %h4.prepend-top-0 - = page_title + CI/CD Pipelines .col-lg-9 = form_for @project, url: namespace_project_pipelines_settings_path(@project.namespace.becomes(Namespace), @project) do |f| %fieldset.builds-feature @@ -95,4 +93,4 @@ %hr .row.prepend-top-default - = render partial: 'badge', collection: @badges + = render partial: 'projects/pipelines_settings/badge', collection: @badges diff --git a/app/views/projects/runners/index.html.haml b/app/views/projects/runners/_index.html.haml similarity index 87% rename from app/views/projects/runners/index.html.haml rename to app/views/projects/runners/_index.html.haml index d6f691d9c24077..f9808f7c990f6b 100644 --- a/app/views/projects/runners/index.html.haml +++ b/app/views/projects/runners/_index.html.haml @@ -1,5 +1,3 @@ -- page_title "Runners" - .light.prepend-top-default %p A 'Runner' is a process which runs a job. @@ -22,6 +20,6 @@ %p.lead To start serving your jobs you can either add specific Runners to your project or use shared Runners .row .col-sm-6 - = render 'specific_runners' + = render 'projects/runners/specific_runners' .col-sm-6 - = render 'shared_runners' + = render 'projects/runners/shared_runners' diff --git a/app/views/projects/runners/_shared_runners.html.haml b/app/views/projects/runners/_shared_runners.html.haml index 5afa193357ef72..0671dd66e78d29 100644 --- a/app/views/projects/runners/_shared_runners.html.haml +++ b/app/views/projects/runners/_shared_runners.html.haml @@ -22,7 +22,7 @@ - else %h4.underlined-title Available shared Runners : #{@shared_runners_count} %ul.bordered-list.available-shared-runners - = render partial: 'runner', collection: @shared_runners, as: :runner + = render partial: 'projects/runners/runner', collection: @shared_runners, as: :runner - if @shared_runners_count > 10 .light and #{@shared_runners_count - 10} more... diff --git a/app/views/projects/runners/_specific_runners.html.haml b/app/views/projects/runners/_specific_runners.html.haml index dcff675eafc387..6b8e6bd4feef60 100644 --- a/app/views/projects/runners/_specific_runners.html.haml +++ b/app/views/projects/runners/_specific_runners.html.haml @@ -20,10 +20,10 @@ - if @project_runners.any? %h4.underlined-title Runners activated for this project %ul.bordered-list.activated-specific-runners - = render partial: 'runner', collection: @project_runners, as: :runner + = render partial: 'projects/runners/runner', collection: @project_runners, as: :runner - if @assignable_runners.any? %h4.underlined-title Available specific runners %ul.bordered-list.available-specific-runners - = render partial: 'runner', collection: @assignable_runners, as: :runner + = render partial: 'projects/runners/runner', collection: @assignable_runners, as: :runner = paginate @assignable_runners, theme: "gitlab" diff --git a/app/views/projects/settings/ci_cd/show.html.haml b/app/views/projects/settings/ci_cd/show.html.haml new file mode 100644 index 00000000000000..52f5f7b81e2afb --- /dev/null +++ b/app/views/projects/settings/ci_cd/show.html.haml @@ -0,0 +1,6 @@ +- page_title "CI/CD Pipelines" + += render 'projects/runners/index' += render 'projects/variables/index' += render 'projects/triggers/index' += render 'projects/pipelines_settings/show' diff --git a/app/views/projects/triggers/index.html.haml b/app/views/projects/triggers/_index.html.haml similarity index 93% rename from app/views/projects/triggers/index.html.haml rename to app/views/projects/triggers/_index.html.haml index b9c4e323430a73..5cb1818ae54914 100644 --- a/app/views/projects/triggers/index.html.haml +++ b/app/views/projects/triggers/_index.html.haml @@ -1,9 +1,7 @@ -- page_title "Triggers" - .row.prepend-top-default.append-bottom-default .col-lg-3 %h4.prepend-top-0 - = page_title + Triggers %p.prepend-top-20 Triggers can force a specific branch or tag to get rebuilt with an API call. %p.append-bottom-0 @@ -25,12 +23,12 @@ %th %strong Last used %th - = render partial: 'trigger', collection: @triggers, as: :trigger + = render partial: 'projects/triggers/trigger', collection: @triggers, as: :trigger - else %p.settings-message.text-center.append-bottom-default No triggers have been created yet. Add one using the button below. - = form_for @trigger, url: url_for(controller: 'projects/triggers', action: 'create') do |f| + = form_for @trigger, url: url_for(controller: '/projects/triggers', action: 'create') do |f| = f.submit "Add trigger", class: 'btn btn-success' .panel-footer diff --git a/app/views/projects/variables/index.html.haml b/app/views/projects/variables/_index.html.haml similarity index 74% rename from app/views/projects/variables/index.html.haml rename to app/views/projects/variables/_index.html.haml index cf7ae0b489f35c..1b852a9c5b36e9 100644 --- a/app/views/projects/variables/index.html.haml +++ b/app/views/projects/variables/_index.html.haml @@ -1,12 +1,10 @@ -- page_title "Variables" - .row.prepend-top-default.append-bottom-default .col-lg-3 - = render "content" + = render "projects/variables/content" .col-lg-9 %h5.prepend-top-0 Add a variable - = render "form", btn_text: "Add new variable" + = render "projects/variables/form", btn_text: "Add new variable" %hr %h5.prepend-top-0 Your variables (#{@project.variables.size}) @@ -14,5 +12,5 @@ %p.settings-message.text-center.append-bottom-0 No variables found, add one with the form above. - else - = render "table" + = render "projects/variables/table" %button.btn.btn-info.js-btn-toggle-reveal-values{ "data-status" => 'hidden' } Reveal Values diff --git a/config/routes/project.rb b/config/routes/project.rb index 4f4aecaa367f39..c10fc1a3cdf9c9 100644 --- a/config/routes/project.rb +++ b/config/routes/project.rb @@ -362,6 +362,7 @@ namespace :settings do resource :members, only: [:show] + resource :ci_cd, only: [:show], controller: 'ci_cd' resource :integrations, only: [:show] end diff --git a/spec/controllers/projects/settings/ci_cd_controller.rb b/spec/controllers/projects/settings/ci_cd_controller.rb new file mode 100644 index 00000000000000..e9a91cff1b3474 --- /dev/null +++ b/spec/controllers/projects/settings/ci_cd_controller.rb @@ -0,0 +1,20 @@ +require('spec_helper') + +describe Projects::Settings::CiCdController do + let(:project) { create(:empty_project, :public, :access_requestable) } + let(:user) { create(:user) } + + before do + project.team << [user, :master] + sign_in(user) + end + + describe 'GET show' do + it 'renders show with 200 status code' do + get :show, namespace_id: project.namespace, project_id: project + + expect(response).to have_http_status(200) + expect(response).to render_template(:show) + end + end +end diff --git a/spec/controllers/projects/variables_controller_spec.rb b/spec/controllers/projects/variables_controller_spec.rb new file mode 100644 index 00000000000000..9914e217d70e89 --- /dev/null +++ b/spec/controllers/projects/variables_controller_spec.rb @@ -0,0 +1,56 @@ +require 'spec_helper' + +describe Projects::VariablesController do + let(:project) { create(:empty_project) } + let(:user) { create(:user) } + + before do + sign_in(user) + project.team << [user, :master] + end + + describe 'POST #create' do + context 'variable is valid' do + it 'shows a success flash message' do + post :create, namespace_id: project.namespace.to_param, project_id: project.to_param, + variable: { key: "one", value: "two" } + expect(flash[:notice]).to include 'Variables were successfully updated.' + expect(response).to redirect_to(namespace_project_settings_ci_cd_path(project.namespace, project)) + end + end + + context 'variable is invalid' do + it 'shows an alert flash message' do + post :create, namespace_id: project.namespace.to_param, project_id: project.to_param, + variable: { key: "..one", value: "two" } + expect(flash[:alert]).to include 'Key can contain only letters, digits and \'_\'.' + expect(response).to redirect_to(namespace_project_settings_ci_cd_path(project.namespace, project)) + end + end + end + + describe 'POST #update' do + let(:variable) { create(:ci_variable) } + + context 'updating a variable with valid characters' do + before do + variable.gl_project_id = project.id + project.variables << variable + end + + it 'shows a success flash message' do + post :update, namespace_id: project.namespace.to_param, project_id: project.to_param, + id: variable.id, variable: { key: variable.key, value: 'two' } + expect(flash[:notice]).to include 'Variables were successfully updated.' + expect(response).to redirect_to(namespace_project_settings_ci_cd_path(project.namespace, project)) + end + + it 'shows an alert flash message' do + post :update, namespace_id: project.namespace.to_param, project_id: project.to_param, + id: variable.id, variable: { key: '?', value: variable.value } + expect(flash[:alert]).to include 'Key can contain only letters, digits and \'_\'.' + expect(response).to redirect_to(namespace_project_settings_ci_cd_path(project.namespace, project)) + end + end + end +end diff --git a/spec/features/security/project/internal_access_spec.rb b/spec/features/security/project/internal_access_spec.rb index 5c3fe62379fa11..67dc4bbec1dbf9 100644 --- a/spec/features/security/project/internal_access_spec.rb +++ b/spec/features/security/project/internal_access_spec.rb @@ -96,6 +96,20 @@ it { is_expected.to be_denied_for(:external) } end + describe "GET /:project_path/settings/ci_cd" do + subject { namespace_project_settings_ci_cd_path(project.namespace, project) } + + it { is_expected.to be_allowed_for(:admin) } + it { is_expected.to be_allowed_for(:owner).of(project) } + it { is_expected.to be_allowed_for(:master).of(project) } + it { is_expected.to be_denied_for(:developer).of(project) } + it { is_expected.to be_denied_for(:reporter).of(project) } + it { is_expected.to be_denied_for(:guest).of(project) } + it { is_expected.to be_denied_for(:user) } + it { is_expected.to be_denied_for(:visitor) } + it { is_expected.to be_denied_for(:external) } + end + describe "GET /:project_path/blob" do let(:commit) { project.repository.commit } subject { namespace_project_blob_path(project.namespace, project, File.join(commit.id, '.gitignore')) } diff --git a/spec/features/security/project/private_access_spec.rb b/spec/features/security/project/private_access_spec.rb index 828d7bc496e622..02954147f5e1d4 100644 --- a/spec/features/security/project/private_access_spec.rb +++ b/spec/features/security/project/private_access_spec.rb @@ -92,8 +92,22 @@ it { is_expected.to be_allowed_for(:reporter).of(project) } it { is_expected.to be_allowed_for(:guest).of(project) } it { is_expected.to be_denied_for(:user) } + it { is_expected.to be_denied_for(:visitor) } it { is_expected.to be_denied_for(:external) } + end + + describe "GET /:project_path/settings/ci_cd" do + subject { namespace_project_settings_ci_cd_path(project.namespace, project) } + + it { is_expected.to be_allowed_for(:admin) } + it { is_expected.to be_allowed_for(:owner).of(project) } + it { is_expected.to be_allowed_for(:master).of(project) } + it { is_expected.to be_denied_for(:developer).of(project) } + it { is_expected.to be_denied_for(:reporter).of(project) } + it { is_expected.to be_denied_for(:guest).of(project) } + it { is_expected.to be_denied_for(:user) } it { is_expected.to be_denied_for(:visitor) } + it { is_expected.to be_denied_for(:external) } end describe "GET /:project_path/blob" do diff --git a/spec/features/security/project/public_access_spec.rb b/spec/features/security/project/public_access_spec.rb index 0a5ed2aa36db18..36107fea3a3a4e 100644 --- a/spec/features/security/project/public_access_spec.rb +++ b/spec/features/security/project/public_access_spec.rb @@ -96,6 +96,20 @@ it { is_expected.to be_allowed_for(:external) } end + describe "GET /:project_path/settings/ci_cd" do + subject { namespace_project_settings_ci_cd_path(project.namespace, project) } + + it { is_expected.to be_allowed_for(:admin) } + it { is_expected.to be_allowed_for(:owner).of(project) } + it { is_expected.to be_allowed_for(:master).of(project) } + it { is_expected.to be_denied_for(:developer).of(project) } + it { is_expected.to be_denied_for(:reporter).of(project) } + it { is_expected.to be_denied_for(:guest).of(project) } + it { is_expected.to be_denied_for(:user) } + it { is_expected.to be_denied_for(:visitor) } + it { is_expected.to be_denied_for(:external) } + end + describe "GET /:project_path/pipelines" do subject { namespace_project_pipelines_path(project.namespace, project) } diff --git a/spec/features/triggers_spec.rb b/spec/features/triggers_spec.rb index 72354834c5a539..4a7511589d6963 100644 --- a/spec/features/triggers_spec.rb +++ b/spec/features/triggers_spec.rb @@ -7,7 +7,7 @@ before do @project = FactoryGirl.create :empty_project @project.team << [user, :master] - visit namespace_project_triggers_path(@project.namespace, @project) + visit namespace_project_settings_ci_cd_path(@project.namespace, @project) end context 'create a trigger' do diff --git a/spec/features/variables_spec.rb b/spec/features/variables_spec.rb index ff30ffd78206ca..9a4bc027004126 100644 --- a/spec/features/variables_spec.rb +++ b/spec/features/variables_spec.rb @@ -10,7 +10,7 @@ project.team << [user, :master] project.variables << variable - visit namespace_project_variables_path(project.namespace, project) + visit namespace_project_settings_ci_cd_path(project.namespace, project) end it 'shows list of variables' do -- GitLab From b0fb0eeecb0397aae7f1adf18f3cb2a68f1620af Mon Sep 17 00:00:00 2001 From: Jose Ivan Vargas Date: Fri, 3 Feb 2017 17:36:17 -0600 Subject: [PATCH 2/4] Improved code styling on the variables_controller_spec Also updated the #update action inside the variables controller as to render the show and not redirect back to the settings route --- .../projects/variables_controller.rb | 5 ++--- .../projects/variables_controller_spec.rb | 18 +++++++++++------- 2 files changed, 13 insertions(+), 10 deletions(-) diff --git a/app/controllers/projects/variables_controller.rb b/app/controllers/projects/variables_controller.rb index d07ef7889ca148..2ebd8ccee70551 100644 --- a/app/controllers/projects/variables_controller.rb +++ b/app/controllers/projects/variables_controller.rb @@ -15,11 +15,10 @@ def update @variable = @project.variables.find(params[:id]) if @variable.update_attributes(project_params) - flash[:notice] = 'Variables were successfully updated.' + redirect_to namespace_project_variables_path(project.namespace, project), notice: 'Variable was successfully updated.' else - flash[:alert] = @variable.errors.full_messages.join(',').html_safe + render action: "show" end - redirect_to namespace_project_settings_ci_cd_path(project.namespace, project) end def create diff --git a/spec/controllers/projects/variables_controller_spec.rb b/spec/controllers/projects/variables_controller_spec.rb index 9914e217d70e89..228cb5135545d3 100644 --- a/spec/controllers/projects/variables_controller_spec.rb +++ b/spec/controllers/projects/variables_controller_spec.rb @@ -14,6 +14,7 @@ it 'shows a success flash message' do post :create, namespace_id: project.namespace.to_param, project_id: project.to_param, variable: { key: "one", value: "two" } + expect(flash[:notice]).to include 'Variables were successfully updated.' expect(response).to redirect_to(namespace_project_settings_ci_cd_path(project.namespace, project)) end @@ -23,6 +24,7 @@ it 'shows an alert flash message' do post :create, namespace_id: project.namespace.to_param, project_id: project.to_param, variable: { key: "..one", value: "two" } + expect(flash[:alert]).to include 'Key can contain only letters, digits and \'_\'.' expect(response).to redirect_to(namespace_project_settings_ci_cd_path(project.namespace, project)) end @@ -40,16 +42,18 @@ it 'shows a success flash message' do post :update, namespace_id: project.namespace.to_param, project_id: project.to_param, - id: variable.id, variable: { key: variable.key, value: 'two' } - expect(flash[:notice]).to include 'Variables were successfully updated.' - expect(response).to redirect_to(namespace_project_settings_ci_cd_path(project.namespace, project)) + id: variable.id, variable: { key: variable.key, value: 'two' } + + expect(flash[:notice]).to include 'Variable was successfully updated.' + expect(response).to redirect_to(namespace_project_variables_path(project.namespace, project)) end - it 'shows an alert flash message' do + it 'renders the action #show if the variable key is invalid' do post :update, namespace_id: project.namespace.to_param, project_id: project.to_param, - id: variable.id, variable: { key: '?', value: variable.value } - expect(flash[:alert]).to include 'Key can contain only letters, digits and \'_\'.' - expect(response).to redirect_to(namespace_project_settings_ci_cd_path(project.namespace, project)) + id: variable.id, variable: { key: '?', value: variable.value } + + expect(response).to have_http_status(200) + expect(response).to render_template :show end end end -- GitLab From b6daf53a9143d08ed97277d542acdc6e9a78ee14 Mon Sep 17 00:00:00 2001 From: Jose Ivan Vargas Date: Mon, 6 Feb 2017 13:18:21 -0600 Subject: [PATCH 3/4] Changed #create actions to render the "show" partial. This happens when creating a variable or trigger fails, as to show the form_errors partial accordingly --- app/controllers/projects/triggers_controller.rb | 4 ++-- app/controllers/projects/variables_controller.rb | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/app/controllers/projects/triggers_controller.rb b/app/controllers/projects/triggers_controller.rb index 224a25eec99ec2..b2c11ea4156d28 100644 --- a/app/controllers/projects/triggers_controller.rb +++ b/app/controllers/projects/triggers_controller.rb @@ -12,11 +12,11 @@ def create @trigger.save if @trigger.valid? - flash[:notice] = "Trigger has been created successfully" + redirect_to namespace_project_variables_path(project.namespace, project), notice: 'Trigger was created successfully.' else @triggers = project.triggers.select(&:persisted?) + render action: "show" end - redirect_to namespace_project_settings_ci_cd_path(@project.namespace, @project) end def destroy diff --git a/app/controllers/projects/variables_controller.rb b/app/controllers/projects/variables_controller.rb index 2ebd8ccee70551..a4d1b1ee69b7d8 100644 --- a/app/controllers/projects/variables_controller.rb +++ b/app/controllers/projects/variables_controller.rb @@ -26,10 +26,10 @@ def create if @variable.valid? && @project.variables << @variable flash[:notice] = 'Variables were successfully updated.' + redirect_to namespace_project_settings_ci_cd_path(project.namespace, project) else - flash[:alert] = @variable.errors.full_messages.join(',').html_safe + render "show" end - redirect_to namespace_project_settings_ci_cd_path(project.namespace, project) end def destroy -- GitLab From be1ad4e1bb992b3118acfe94649ae3aeea82c030 Mon Sep 17 00:00:00 2001 From: Jose Ivan Vargas Date: Mon, 6 Feb 2017 15:09:17 -0600 Subject: [PATCH 4/4] Fixed variables_controller_spec.rb test --- spec/controllers/projects/variables_controller_spec.rb | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/spec/controllers/projects/variables_controller_spec.rb b/spec/controllers/projects/variables_controller_spec.rb index 228cb5135545d3..9fa358f7d62116 100644 --- a/spec/controllers/projects/variables_controller_spec.rb +++ b/spec/controllers/projects/variables_controller_spec.rb @@ -25,8 +25,7 @@ post :create, namespace_id: project.namespace.to_param, project_id: project.to_param, variable: { key: "..one", value: "two" } - expect(flash[:alert]).to include 'Key can contain only letters, digits and \'_\'.' - expect(response).to redirect_to(namespace_project_settings_ci_cd_path(project.namespace, project)) + expect(response).to render_template("projects/variables/show") end end end -- GitLab