From 6253d4456a98613b419d766a03af7ff9b9fcf2af Mon Sep 17 00:00:00 2001 From: James Edwards-Jones Date: Sun, 18 Feb 2018 23:47:37 +0000 Subject: Backport changes from EE's GithubService integration Adds detailed_status to pipeline hook data Adds detailed_description option for Services Integration edit page renders 404 if a service is disabled --- app/controllers/projects/services_controller.rb | 5 +++++ app/views/projects/services/_form.html.haml | 3 +++ changelogs/unreleased/ce-jej-github-project-service-for-ci.yml | 5 +++++ lib/gitlab/data_builder/pipeline.rb | 1 + spec/lib/gitlab/data_builder/pipeline_spec.rb | 1 + 5 files changed, 15 insertions(+) create mode 100644 changelogs/unreleased/ce-jej-github-project-service-for-ci.yml diff --git a/app/controllers/projects/services_controller.rb b/app/controllers/projects/services_controller.rb index daa5c88aae0..b0da0d4dac5 100644 --- a/app/controllers/projects/services_controller.rb +++ b/app/controllers/projects/services_controller.rb @@ -3,6 +3,7 @@ class Projects::ServicesController < Projects::ApplicationController # Authorize before_action :authorize_admin_project! + before_action :ensure_service_enabled before_action :service, only: [:edit, :update, :test] respond_to :html @@ -54,4 +55,8 @@ class Projects::ServicesController < Projects::ApplicationController def service @service ||= @project.find_or_initialize_service(params[:id]) end + + def ensure_service_enabled + render_404 unless service + end end diff --git a/app/views/projects/services/_form.html.haml b/app/views/projects/services/_form.html.haml index 17e804d682b..053ea24b848 100644 --- a/app/views/projects/services/_form.html.haml +++ b/app/views/projects/services/_form.html.haml @@ -5,6 +5,9 @@ = boolean_to_icon @service.activated? %p= @service.description + + - if @service.respond_to?(:detailed_description) + %p= @service.detailed_description .col-lg-9 = form_for(@service, as: :service, url: project_service_path(@project, @service.to_param), method: :put, html: { class: 'gl-show-field-errors form-horizontal integration-settings-form js-integration-settings-form', data: { 'can-test' => @service.can_test?, 'test-url' => test_project_service_path(@project, @service) } }) do |form| = render 'shared/service_settings', form: form, subject: @service diff --git a/changelogs/unreleased/ce-jej-github-project-service-for-ci.yml b/changelogs/unreleased/ce-jej-github-project-service-for-ci.yml new file mode 100644 index 00000000000..6102b7ecd93 --- /dev/null +++ b/changelogs/unreleased/ce-jej-github-project-service-for-ci.yml @@ -0,0 +1,5 @@ +--- +title: Hook data for pipelines includes detailed_status +merge_request: 17607 +author: +type: changed diff --git a/lib/gitlab/data_builder/pipeline.rb b/lib/gitlab/data_builder/pipeline.rb index e47fb85b5ee..1e283cc092b 100644 --- a/lib/gitlab/data_builder/pipeline.rb +++ b/lib/gitlab/data_builder/pipeline.rb @@ -22,6 +22,7 @@ module Gitlab sha: pipeline.sha, before_sha: pipeline.before_sha, status: pipeline.status, + detailed_status: pipeline.detailed_status(nil).label, stages: pipeline.stages_names, created_at: pipeline.created_at, finished_at: pipeline.finished_at, diff --git a/spec/lib/gitlab/data_builder/pipeline_spec.rb b/spec/lib/gitlab/data_builder/pipeline_spec.rb index f13041e498c..9ca960502c8 100644 --- a/spec/lib/gitlab/data_builder/pipeline_spec.rb +++ b/spec/lib/gitlab/data_builder/pipeline_spec.rb @@ -26,6 +26,7 @@ describe Gitlab::DataBuilder::Pipeline do it { expect(attributes[:tag]).to eq(pipeline.tag) } it { expect(attributes[:id]).to eq(pipeline.id) } it { expect(attributes[:status]).to eq(pipeline.status) } + it { expect(attributes[:detailed_status]).to eq('passed') } it { expect(build_data).to be_a(Hash) } it { expect(build_data[:id]).to eq(build.id) } -- cgit v1.2.1 From ef15668d8214e8cce5732525c872d5b8f57e337a Mon Sep 17 00:00:00 2001 From: James Edwards-Jones Date: Sat, 24 Feb 2018 03:18:14 +0000 Subject: Service integration displays validation errors on test fail Fixes attempts to update a service integration which had `can_test?` set to true but validations were causing the "Test and save changes" button to return "Something went wrong on our end." Removes references to index action left from 0af99433143727088b6a0a1b2163751c05d80ce6 --- app/controllers/projects/services_controller.rb | 28 ++++++++++++---------- config/routes/project.rb | 2 +- .../projects/services_controller_spec.rb | 12 ++++++++++ 3 files changed, 29 insertions(+), 13 deletions(-) diff --git a/app/controllers/projects/services_controller.rb b/app/controllers/projects/services_controller.rb index b0da0d4dac5..f14cb5f6a9f 100644 --- a/app/controllers/projects/services_controller.rb +++ b/app/controllers/projects/services_controller.rb @@ -4,7 +4,7 @@ class Projects::ServicesController < Projects::ApplicationController # Authorize before_action :authorize_admin_project! before_action :ensure_service_enabled - before_action :service, only: [:edit, :update, :test] + before_action :service respond_to :html @@ -24,26 +24,30 @@ class Projects::ServicesController < Projects::ApplicationController end def test - message = {} + if @service.can_test? + render json: service_test_response, status: :ok + else + render json: {}, status: :not_found + end + end + + private - if @service.can_test? && @service.update_attributes(service_params[:service]) + def service_test_response + if @service.update_attributes(service_params[:service]) data = @service.test_data(project, current_user) outcome = @service.test(data) - unless outcome[:success] - message = { error: true, message: 'Test failed.', service_response: outcome[:result].to_s } + if outcome[:success] + {} + else + { error: true, message: 'Test failed.', service_response: outcome[:result].to_s } end - - status = :ok else - status = :not_found + { error: true, message: 'Validations failed.', service_response: @service.errors.full_messages.join(',') } end - - render json: message, status: status end - private - def success_message if @service.active? "#{@service.title} activated." diff --git a/config/routes/project.rb b/config/routes/project.rb index cb46c439415..710fe0ec325 100644 --- a/config/routes/project.rb +++ b/config/routes/project.rb @@ -69,7 +69,7 @@ constraints(ProjectUrlConstrainer.new) do end end - resources :services, constraints: { id: %r{[^/]+} }, only: [:index, :edit, :update] do + resources :services, constraints: { id: %r{[^/]+} }, only: [:edit, :update] do member do put :test end diff --git a/spec/controllers/projects/services_controller_spec.rb b/spec/controllers/projects/services_controller_spec.rb index 847ac6f2be0..e4dc61b3a68 100644 --- a/spec/controllers/projects/services_controller_spec.rb +++ b/spec/controllers/projects/services_controller_spec.rb @@ -23,6 +23,18 @@ describe Projects::ServicesController do end end + context 'when validations fail' do + let(:service_params) { { active: 'true', token: '' } } + + it 'returns error messages in JSON response' do + put :test, namespace_id: project.namespace, project_id: project, id: :hipchat, service: service_params + + expect(json_response['message']).to eq "Validations failed." + expect(json_response['service_response']).to eq "Token can't be blank" + expect(response).to have_gitlab_http_status(200) + end + end + context 'success' do context 'with empty project' do let(:project) { create(:project) } -- cgit v1.2.1 From 93af1af67fc6af2805f3342aed1fc15a4360870d Mon Sep 17 00:00:00 2001 From: James Edwards-Jones Date: Tue, 6 Mar 2018 23:20:01 +0000 Subject: Hides Triggers if integration only has one event Removes confusing/unnecessary checkboxes when trying to configure an integration. If there is only one supported event we don't need to allow these to be individually disabled since the integration can be disabled instead. E.g. Project Integrations for GitHub, Bugzilla, Asana, Pipeline emails and Gemnasium Allows integrations to override which triggers are configurable --- app/models/service.rb | 11 +++++++ app/views/shared/_service_settings.html.haml | 4 +-- ...ej-integrations-can-hide-trigger-checkboxes.yml | 6 ++++ .../projects/services/disable_triggers_spec.rb | 35 ++++++++++++++++++++++ 4 files changed, 54 insertions(+), 2 deletions(-) create mode 100644 changelogs/unreleased/ce-jej-integrations-can-hide-trigger-checkboxes.yml create mode 100644 spec/features/projects/services/disable_triggers_spec.rb diff --git a/app/models/service.rb b/app/models/service.rb index 369cae2e85f..99bf757ae44 100644 --- a/app/models/service.rb +++ b/app/models/service.rb @@ -129,6 +129,17 @@ class Service < ActiveRecord::Base fields end + def configurable_events + events = self.class.supported_events + + # No need to disable individual triggers when there is only one + if events.count == 1 + [] + else + events + end + end + def supported_events self.class.supported_events end diff --git a/app/views/shared/_service_settings.html.haml b/app/views/shared/_service_settings.html.haml index 61b39afb5d4..355b3ac75ae 100644 --- a/app/views/shared/_service_settings.html.haml +++ b/app/views/shared/_service_settings.html.haml @@ -13,12 +13,12 @@ .col-sm-10 = form.check_box :active, disabled: disable_fields_service?(@service) - - if @service.supported_events.present? + - if @service.configurable_events.present? .form-group = form.label :url, "Trigger", class: 'control-label' .col-sm-10 - - @service.supported_events.each do |event| + - @service.configurable_events.each do |event| %div = form.check_box service_event_field_name(event), class: 'pull-left' .prepend-left-20 diff --git a/changelogs/unreleased/ce-jej-integrations-can-hide-trigger-checkboxes.yml b/changelogs/unreleased/ce-jej-integrations-can-hide-trigger-checkboxes.yml new file mode 100644 index 00000000000..771df06e7a6 --- /dev/null +++ b/changelogs/unreleased/ce-jej-integrations-can-hide-trigger-checkboxes.yml @@ -0,0 +1,6 @@ +--- +title: Avoid showing unnecessary Trigger checkboxes for project Integrations with + only one event +merge_request: 17607 +author: +type: changed diff --git a/spec/features/projects/services/disable_triggers_spec.rb b/spec/features/projects/services/disable_triggers_spec.rb new file mode 100644 index 00000000000..1a13fe03a67 --- /dev/null +++ b/spec/features/projects/services/disable_triggers_spec.rb @@ -0,0 +1,35 @@ +require 'spec_helper' + +describe 'Disable individual triggers' do + let(:project) { create(:project) } + let(:user) { project.owner } + let(:checkbox_selector) { 'input[type=checkbox][id$=_events]' } + + before do + sign_in(user) + + visit(project_settings_integrations_path(project)) + + click_link(service_name) + end + + context 'service has multiple supported events' do + let(:service_name) { 'HipChat' } + + it 'shows trigger checkboxes' do + event_count = HipchatService.supported_events.count + + expect(page).to have_content "Trigger" + expect(page).to have_css(checkbox_selector, count: event_count) + end + end + + context 'services only has one supported event' do + let(:service_name) { 'Asana' } + + it "doesn't show unnecessary Trigger checkboxes" do + expect(page).not_to have_content "Trigger" + expect(page).not_to have_css(checkbox_selector) + end + end +end -- cgit v1.2.1