simplify test&save actions when setting a service integration

app/controllers/projects/services_controller.rb

@@ -4,6 +4,7 @@ class Projects::ServicesController < Projects::ApplicationController
   # Authorize
   before_action :authorize_admin_project!
   before_action :service, only: [:edit, :update, :test]
+  before_action :build_service, only: [:update, :test]
   respond_to :html
@@ -13,36 +14,41 @@ class Projects::ServicesController < Projects::ApplicationController
   end
   def update
-    @service.assign_attributes(service_params[:service])
     if @service.save(context: :manual_change)
-      redirect_to(
-        edit_namespace_project_service_path(@project.namespace, @project, @service.to_param),
-        notice: 'Successfully updated.'
-      )
+      redirect_to(namespace_project_settings_integrations_path(@project.namespace, @project), notice: success_message)
     else
       render 'edit'
     end
   end
   def test
-    return render_404 unless @service.can_test?
+    return render json: {}, status: :not_found unless @service.can_test?
     data = @service.test_data(project, current_user)
     outcome = @service.test(data)
-    if outcome[:success]
-      message = { notice: 'We sent a request to the provided URL' }
-    else
-      error_message = "We tried to send a request to the provided URL but an error occurred"
-      error_message << ": #{outcome[:result]}" if outcome[:result].present?
-      message = { alert: error_message }
+    message = {}
+    unless outcome[:success]
+      message = { error: true, message: 'Test failed', service_response: outcome[:result].to_s }
     end
-    redirect_back_or_default(options: message)
+    render json: message, status: :ok
   end
   private
+  def success_message
+    if @service.active?
+      "#{@service.title} activated."
+    else
+      "#{@service.title} settings saved, but not activated."
+    end
+  end
+
+  def build_service
+    @service.assign_attributes(service_params[:service])
+  end
+
   def service
     @service ||= @project.find_or_initialize_service(params[:id])
   end

app/models/project_services/asana_service.rb

@@ -34,7 +34,8 @@ http://app.asana.com/-/account_api'
       {
         type: 'text',
         name: 'api_key',
-        placeholder: 'User Personal Access Token. User must have access to task, all comments will be attributed to this user.'
+        placeholder: 'User Personal Access Token. User must have access to task, all comments will be attributed to this user.',
+        required: true
       },
       {
         type: 'text',

app/models/project_services/assembla_service.rb

@@ -18,7 +18,7 @@ class AssemblaService < Service
   def fields
     [
-      { type: 'text', name: 'token', placeholder: '' },
+      { type: 'text', name: 'token', placeholder: '', required: true },
       { type: 'text', name: 'subdomain', placeholder: '' }
     ]
   end

app/models/project_services/bamboo_service.rb

@@ -47,9 +47,9 @@ class BambooService < CiService
   def fields
     [
         { type: 'text', name: 'bamboo_url',
-          placeholder: 'Bamboo root URL like https://bamboo.example.com' },
+          placeholder: 'Bamboo root URL like https://bamboo.example.com', required: true },
         { type: 'text', name: 'build_key',
-          placeholder: 'Bamboo build plan key like KEY' },
+          placeholder: 'Bamboo build plan key like KEY', required: true },
         { type: 'text', name: 'username',
           placeholder: 'A user with API access, if applicable' },
         { type: 'password', name: 'password' }

app/models/project_services/buildkite_service.rb

@@ -58,11 +58,11 @@ class BuildkiteService < CiService
     [
       { type: 'text',
         name: 'token',
-        placeholder: 'Buildkite project GitLab token' },
+        placeholder: 'Buildkite project GitLab token', required: true },
       { type: 'text',
         name: 'project_url',
-        placeholder: "#{ENDPOINT}/example/project" },
+        placeholder: "#{ENDPOINT}/example/project", required: true },
       { type: 'checkbox',
         name: 'enable_ssl_verification',

app/models/project_services/campfire_service.rb

@@ -18,7 +18,7 @@ class CampfireService < Service
   def fields
     [
-      { type: 'text', name: 'token',     placeholder: '' },
+      { type: 'text', name: 'token',     placeholder: '', required: true },
       { type: 'text', name: 'subdomain', placeholder: '' },
       { type: 'text', name: 'room',      placeholder: '' }
     ]
@@ -76,7 +76,7 @@ class CampfireService < Service
   # Returns a list of rooms, or [].
   # https://github.com/basecamp/campfire-api/blob/master/sections/rooms.md#get-rooms
   def rooms(auth)
-    res = self.class.get("/rooms.json", auth) 
+    res = self.class.get("/rooms.json", auth)
     res.code == 200 ? res["rooms"] : []
   end

app/models/project_services/chat_notification_service.rb

@@ -36,7 +36,7 @@ class ChatNotificationService < Service
   def default_fields
     [
-      { type: 'text', name: 'webhook', placeholder: "e.g. #{webhook_placeholder}" },
+      { type: 'text', name: 'webhook', placeholder: "e.g. #{webhook_placeholder}", required: true },
       { type: 'text', name: 'username', placeholder: 'e.g. GitLab' },
       { type: 'checkbox', name: 'notify_only_broken_pipelines' },
       { type: 'checkbox', name: 'notify_only_default_branch' }

app/models/project_services/custom_issue_tracker_service.rb

@@ -31,9 +31,9 @@ class CustomIssueTrackerService < IssueTrackerService
     [
       { type: 'text', name: 'title', placeholder: title },
       { type: 'text', name: 'description', placeholder: description },
-      { type: 'text', name: 'project_url', placeholder: 'Project url' },
-      { type: 'text', name: 'issues_url', placeholder: 'Issue url' },
-      { type: 'text', name: 'new_issue_url', placeholder: 'New Issue url' }
+      { type: 'text', name: 'project_url', placeholder: 'Project url', required: true },
+      { type: 'text', name: 'issues_url', placeholder: 'Issue url', required: true },
+      { type: 'text', name: 'new_issue_url', placeholder: 'New Issue url', required: true }
     ]
   end
 end

app/models/project_services/drone_ci_service.rb

@@ -93,8 +93,8 @@ class DroneCiService < CiService
   def fields
     [
-      { type: 'text', name: 'token', placeholder: 'Drone CI project specific token' },
-      { type: 'text', name: 'drone_url', placeholder: 'http://drone.example.com' },
+      { type: 'text', name: 'token', placeholder: 'Drone CI project specific token', required: true },
+      { type: 'text', name: 'drone_url', placeholder: 'http://drone.example.com', required: true },
       { type: 'checkbox', name: 'enable_ssl_verification', title: "Enable SSL verification" }
     ]
   end

app/models/project_services/external_wiki_service.rb

@@ -19,7 +19,7 @@ class ExternalWikiService < Service
   def fields
     [
-      { type: 'text', name: 'external_wiki_url', placeholder: 'The URL of the external Wiki' }
+      { type: 'text', name: 'external_wiki_url', placeholder: 'The URL of the external Wiki', required: true }
     ]
   end

app/models/project_services/flowdock_service.rb

@@ -18,7 +18,7 @@ class FlowdockService < Service
   def fields
     [
-      { type: 'text', name: 'token', placeholder: 'Flowdock Git source token' }
+      { type: 'text', name: 'token', placeholder: 'Flowdock Git source token', required: true }
     ]
   end

app/models/project_services/gemnasium_service.rb

@@ -18,8 +18,8 @@ class GemnasiumService < Service
   def fields
     [
-      { type: 'text', name: 'api_key', placeholder: 'Your personal API KEY on gemnasium.com ' },
-      { type: 'text', name: 'token', placeholder: 'The project\'s slug on gemnasium.com' }
+      { type: 'text', name: 'api_key', placeholder: 'Your personal API KEY on gemnasium.com ', required: true },
+      { type: 'text', name: 'token', placeholder: 'The project\'s slug on gemnasium.com', required: true }
     ]
   end

app/models/project_services/hipchat_service.rb

@@ -33,7 +33,7 @@ class HipchatService < Service
   def fields
     [
-      { type: 'text', name: 'token',     placeholder: 'Room token' },
+      { type: 'text', name: 'token',     placeholder: 'Room token', required: true },
       { type: 'text', name: 'room',      placeholder: 'Room name or ID' },
       { type: 'checkbox', name: 'notify' },
       { type: 'select', name: 'color', choices: %w(yellow red green purple gray random) },

app/models/project_services/irker_service.rb

@@ -49,7 +49,7 @@ class IrkerService < Service
         help: 'A default IRC URI to prepend before each recipient (optional)',
         placeholder: 'irc://irc.network.net:6697/' },
       { type: 'textarea', name: 'recipients',
-        placeholder: 'Recipients/channels separated by whitespaces',
+        placeholder: 'Recipients/channels separated by whitespaces', required: true,
         help: 'Recipients have to be specified with a full URI: '\
         'irc[s]://irc.network.net[:port]/#channel. Special cases: if '\
         'you want the channel to be a nickname instead, append ",isnick" to ' \

app/models/project_services/issue_tracker_service.rb

@@ -32,9 +32,9 @@ class IssueTrackerService < Service
   def fields
     [
       { type: 'text', name: 'description', placeholder: description },
-      { type: 'text', name: 'project_url', placeholder: 'Project url' },
-      { type: 'text', name: 'issues_url', placeholder: 'Issue url' },
-      { type: 'text', name: 'new_issue_url', placeholder: 'New Issue url' }
+      { type: 'text', name: 'project_url', placeholder: 'Project url', required: true },
+      { type: 'text', name: 'issues_url', placeholder: 'Issue url', required: true },
+      { type: 'text', name: 'new_issue_url', placeholder: 'New Issue url', required: true }
     ]
   end

app/models/project_services/jira_service.rb

@@ -86,12 +86,12 @@ class JiraService < IssueTrackerService
   def fields
     [
-      { type: 'text', name: 'url', title: 'Web URL', placeholder: 'https://jira.example.com' },
+      { type: 'text', name: 'url', title: 'Web URL', placeholder: 'https://jira.example.com', required: true },
       { type: 'text', name: 'api_url', title: 'JIRA API URL', placeholder: 'If different from Web URL' },
-      { type: 'text', name: 'project_key', placeholder: 'Project Key' },
-      { type: 'text', name: 'username', placeholder: '' },
-      { type: 'password', name: 'password', placeholder: '' },
-      { type: 'text', name: 'jira_issue_transition_id', placeholder: '' }
+      { type: 'text', name: 'project_key', placeholder: 'Project Key', required: true },
+      { type: 'text', name: 'username', placeholder: '', required: true },
+      { type: 'password', name: 'password', placeholder: '', required: true },
+      { type: 'text', name: 'jira_issue_transition_id', placeholder: '', required: true }
     ]
   end

app/models/project_services/mock_ci_service.rb

@@ -21,7 +21,8 @@ class MockCiService < CiService
     [
       { type: 'text',
         name: 'mock_service_url',
-        placeholder: 'http://localhost:4004' }
+        placeholder: 'http://localhost:4004',
+        required: true }
     ]
   end

app/models/project_services/pipelines_email_service.rb

@@ -53,7 +53,8 @@ class PipelinesEmailService < Service
     [
       { type: 'textarea',
         name: 'recipients',
-        placeholder: 'Emails separated by comma' },
+        placeholder: 'Emails separated by comma',
+        required: true },
       { type: 'checkbox',
         name: 'notify_only_broken_pipelines' }
     ]

app/models/project_services/pivotaltracker_service.rb

@@ -23,7 +23,8 @@ class PivotaltrackerService < Service
       {
         type: 'text',
         name: 'token',
-        placeholder: 'Pivotal Tracker API token.'
+        placeholder: 'Pivotal Tracker API token.',
+        required: true
       },
       {
         type: 'text',

app/models/project_services/prometheus_service.rb

@@ -49,7 +49,8 @@ class PrometheusService < MonitoringService
         type: 'text',
         name: 'api_url',
         title: 'API URL',
-        placeholder: 'Prometheus API Base URL, like http://prometheus.example.com/'
+        placeholder: 'Prometheus API Base URL, like http://prometheus.example.com/',
+        required: true
       }
     ]
   end