From 33d82ccb453ef020d28f1307be1a3a97130c9e0b Mon Sep 17 00:00:00 2001
From: Jarka Kadlecova <jarka@gitlab.com>
Date: Mon, 22 May 2017 12:07:12 +0200
Subject: [PATCH 01/26] simplify test&save actions when setting a service
 integration

---
 .../projects/services_controller.rb           |  32 +++--
 app/models/project_services/asana_service.rb  |   3 +-
 .../project_services/assembla_service.rb      |   2 +-
 app/models/project_services/bamboo_service.rb |   4 +-
 .../project_services/buildkite_service.rb     |   4 +-
 .../project_services/campfire_service.rb      |   4 +-
 .../chat_notification_service.rb              |   2 +-
 .../custom_issue_tracker_service.rb           |   6 +-
 .../project_services/drone_ci_service.rb      |   4 +-
 .../project_services/external_wiki_service.rb |   2 +-
 .../project_services/flowdock_service.rb      |   2 +-
 .../project_services/gemnasium_service.rb     |   4 +-
 .../project_services/hipchat_service.rb       |   2 +-
 app/models/project_services/irker_service.rb  |   2 +-
 .../project_services/issue_tracker_service.rb |   6 +-
 app/models/project_services/jira_service.rb   |  10 +-
 .../project_services/mock_ci_service.rb       |   3 +-
 .../pipelines_email_service.rb                |   3 +-
 .../pivotaltracker_service.rb                 |   3 +-
 .../project_services/prometheus_service.rb    |   3 +-
 .../project_services/pushover_service.rb      |   6 +-
 .../project_services/teamcity_service.rb      |   4 +-
 app/views/shared/_field.html.haml             |   1 +
 config/routes/project.rb                      |   2 +-
 features/project/service.feature              |  26 ++--
 features/steps/project/services.rb            |  70 +++++-----
 .../projects/services_controller_spec.rb      | 121 ++++++++----------
 spec/factories/services.rb                    |   6 +
 .../services/mattermost_slash_command_spec.rb |  16 ++-
 .../services/slack_slash_command_spec.rb      |  14 +-
 30 files changed, 190 insertions(+), 177 deletions(-)

diff --git a/app/controllers/projects/services_controller.rb b/app/controllers/projects/services_controller.rb
index f9d798d0455..89c4e81a36f 100644
--- a/app/controllers/projects/services_controller.rb
+++ b/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
diff --git a/app/models/project_services/asana_service.rb b/app/models/project_services/asana_service.rb
index 3728f5642e4..9ce2d1153a7 100644
--- a/app/models/project_services/asana_service.rb
+++ b/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',
diff --git a/app/models/project_services/assembla_service.rb b/app/models/project_services/assembla_service.rb
index aeeff8917bf..ae6af732ed4 100644
--- a/app/models/project_services/assembla_service.rb
+++ b/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
diff --git a/app/models/project_services/bamboo_service.rb b/app/models/project_services/bamboo_service.rb
index 3f5b3eb159b..42939ea0ec8 100644
--- a/app/models/project_services/bamboo_service.rb
+++ b/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' }
diff --git a/app/models/project_services/buildkite_service.rb b/app/models/project_services/buildkite_service.rb
index 5fb95050b83..fc30f6e3365 100644
--- a/app/models/project_services/buildkite_service.rb
+++ b/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',
diff --git a/app/models/project_services/campfire_service.rb b/app/models/project_services/campfire_service.rb
index 0de59af5652..c3f5b310619 100644
--- a/app/models/project_services/campfire_service.rb
+++ b/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
 
diff --git a/app/models/project_services/chat_notification_service.rb b/app/models/project_services/chat_notification_service.rb
index 779ef54cfcb..da7a4067d8e 100644
--- a/app/models/project_services/chat_notification_service.rb
+++ b/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' }
diff --git a/app/models/project_services/custom_issue_tracker_service.rb b/app/models/project_services/custom_issue_tracker_service.rb
index dea915a4d05..b9e3e982b64 100644
--- a/app/models/project_services/custom_issue_tracker_service.rb
+++ b/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
diff --git a/app/models/project_services/drone_ci_service.rb b/app/models/project_services/drone_ci_service.rb
index 2717c240f05..f6cade9c290 100644
--- a/app/models/project_services/drone_ci_service.rb
+++ b/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
diff --git a/app/models/project_services/external_wiki_service.rb b/app/models/project_services/external_wiki_service.rb
index b4d7c977ce4..720ad61162e 100644
--- a/app/models/project_services/external_wiki_service.rb
+++ b/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
 
diff --git a/app/models/project_services/flowdock_service.rb b/app/models/project_services/flowdock_service.rb
index 2a05d757eb4..2db95b9aaa3 100644
--- a/app/models/project_services/flowdock_service.rb
+++ b/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
 
diff --git a/app/models/project_services/gemnasium_service.rb b/app/models/project_services/gemnasium_service.rb
index f271e1f1739..017a9b2df6e 100644
--- a/app/models/project_services/gemnasium_service.rb
+++ b/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
 
diff --git a/app/models/project_services/hipchat_service.rb b/app/models/project_services/hipchat_service.rb
index c19fed339ba..e3906943ecd 100644
--- a/app/models/project_services/hipchat_service.rb
+++ b/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) },
diff --git a/app/models/project_services/irker_service.rb b/app/models/project_services/irker_service.rb
index a51d43adcb9..19357f90810 100644
--- a/app/models/project_services/irker_service.rb
+++ b/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 ' \
diff --git a/app/models/project_services/issue_tracker_service.rb b/app/models/project_services/issue_tracker_service.rb
index eddf308eae3..ff138b9066d 100644
--- a/app/models/project_services/issue_tracker_service.rb
+++ b/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
 
diff --git a/app/models/project_services/jira_service.rb b/app/models/project_services/jira_service.rb
index 25d098b63c0..5384d75994a 100644
--- a/app/models/project_services/jira_service.rb
+++ b/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
 
diff --git a/app/models/project_services/mock_ci_service.rb b/app/models/project_services/mock_ci_service.rb
index 546b6e0a498..e5109dbbc93 100644
--- a/app/models/project_services/mock_ci_service.rb
+++ b/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
 
diff --git a/app/models/project_services/pipelines_email_service.rb b/app/models/project_services/pipelines_email_service.rb
index f824171ad09..9d37184be2c 100644
--- a/app/models/project_services/pipelines_email_service.rb
+++ b/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' }
     ]
diff --git a/app/models/project_services/pivotaltracker_service.rb b/app/models/project_services/pivotaltracker_service.rb
index d86f4f6f448..f9dfa2e91c3 100644
--- a/app/models/project_services/pivotaltracker_service.rb
+++ b/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',
diff --git a/app/models/project_services/prometheus_service.rb b/app/models/project_services/prometheus_service.rb
index ec72cb6856d..110b8bc209b 100644
--- a/app/models/project_services/prometheus_service.rb
+++ b/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
diff --git a/app/models/project_services/pushover_service.rb b/app/models/project_services/pushover_service.rb
index fc29a5277bb..aa7bd4c3c84 100644
--- a/app/models/project_services/pushover_service.rb
+++ b/app/models/project_services/pushover_service.rb
@@ -19,10 +19,10 @@ class PushoverService < Service
 
   def fields
     [
-      { type: 'text', name: 'api_key', placeholder: 'Your application key' },
-      { type: 'text', name: 'user_key', placeholder: 'Your user key' },
+      { type: 'text', name: 'api_key', placeholder: 'Your application key', required: true },
+      { type: 'text', name: 'user_key', placeholder: 'Your user key', required: true },
       { type: 'text', name: 'device', placeholder: 'Leave blank for all active devices' },
-      { type: 'select', name: 'priority', choices:
+      { type: 'select', name: 'priority', required: true, choices:
         [
           ['Lowest Priority', -2],
           ['Low Priority', -1],
diff --git a/app/models/project_services/teamcity_service.rb b/app/models/project_services/teamcity_service.rb
index b16beb406b9..cbe137452bd 100644
--- a/app/models/project_services/teamcity_service.rb
+++ b/app/models/project_services/teamcity_service.rb
@@ -50,9 +50,9 @@ class TeamcityService < CiService
   def fields
     [
       { type: 'text', name: 'teamcity_url',
-        placeholder: 'TeamCity root URL like https://teamcity.example.com' },
+        placeholder: 'TeamCity root URL like https://teamcity.example.com', required: true },
       { type: 'text', name: 'build_type',
-        placeholder: 'Build configuration ID' },
+        placeholder: 'Build configuration ID', required: true },
       { type: 'text', name: 'username',
         placeholder: 'A user with permissions to trigger a manual build' },
       { type: 'password', name: 'password' }
diff --git a/app/views/shared/_field.html.haml b/app/views/shared/_field.html.haml
index d74b0043949..d67e20e988d 100644
--- a/app/views/shared/_field.html.haml
+++ b/app/views/shared/_field.html.haml
@@ -3,6 +3,7 @@
 - value = @service.send(name)
 - type = field[:type]
 - placeholder = field[:placeholder]
+- required = field[:required]
 - choices = field[:choices]
 - default_choice = field[:default_choice]
 - help = field[:help]
diff --git a/config/routes/project.rb b/config/routes/project.rb
index 5aac44fce10..14718e2f3c4 100644
--- a/config/routes/project.rb
+++ b/config/routes/project.rb
@@ -67,7 +67,7 @@ constraints(ProjectUrlConstrainer.new) do
 
       resources :services, constraints: { id: /[^\/]+/ }, only: [:index, :edit, :update] do
         member do
-          get :test
+          put :test
         end
       end
 
diff --git a/features/project/service.feature b/features/project/service.feature
index cce5f58adec..54f07ebca92 100644
--- a/features/project/service.feature
+++ b/features/project/service.feature
@@ -11,77 +11,77 @@ Feature: Project Services
     When I visit project "Shop" services page
     And I click hipchat service link
     And I fill hipchat settings
-    Then I should see hipchat service settings saved
+    Then I should see the Hipchat success message
 
   Scenario: Activate hipchat service with custom server
     When I visit project "Shop" services page
     And I click hipchat service link
     And I fill hipchat settings with custom server
-    Then I should see hipchat service settings with custom server saved
+    Then I should see the Hipchat success message
 
   Scenario: Activate pivotaltracker service
     When I visit project "Shop" services page
     And I click pivotaltracker service link
     And I fill pivotaltracker settings
-    Then I should see pivotaltracker service settings saved
+    Then I should see the Pivotaltracker success message
 
   Scenario: Activate Flowdock service
     When I visit project "Shop" services page
     And I click Flowdock service link
     And I fill Flowdock settings
-    Then I should see Flowdock service settings saved
+    Then I should see the Flowdock success message
 
   Scenario: Activate Assembla service
     When I visit project "Shop" services page
     And I click Assembla service link
     And I fill Assembla settings
-    Then I should see Assembla service settings saved
+    Then I should see the Assembla success message
 
   Scenario: Activate Slack notifications service
     When I visit project "Shop" services page
     And I click Slack notifications service link
     And I fill Slack notifications settings
-    Then I should see Slack Notifications service settings saved
+    Then I should see the Slack notifications success message
 
   Scenario: Activate Pushover service
     When I visit project "Shop" services page
     And I click Pushover service link
     And I fill Pushover settings
-    Then I should see Pushover service settings saved
+    Then I should see the Pushover success message
 
   Scenario: Activate email on push service
     When I visit project "Shop" services page
     And I click email on push service link
     And I fill email on push settings
-    Then I should see email on push service settings saved
+    Then I should see the Emails on push success message
 
   Scenario: Activate JIRA service
     When I visit project "Shop" services page
     And I click jira service link
     And I fill jira settings
-    Then I should see jira service settings saved
+    Then I should see the JIRA success message
 
   Scenario: Activate Irker (IRC Gateway) service
     When I visit project "Shop" services page
     And I click Irker service link
     And I fill Irker settings
-    Then I should see Irker service settings saved
+    Then I should see the Irker success message
 
   Scenario: Activate Atlassian Bamboo CI service
     When I visit project "Shop" services page
     And I click Atlassian Bamboo CI service link
     And I fill Atlassian Bamboo CI settings
-    Then I should see Atlassian Bamboo CI service settings saved
+    Then I should see the Bamboo success message
     And I should see empty field Change Password
 
   Scenario: Activate jetBrains TeamCity CI service
     When I visit project "Shop" services page
     And I click jetBrains TeamCity CI service link
     And I fill jetBrains TeamCity CI settings
-    Then I should see jetBrains TeamCity CI service settings saved
+    Then I should see the JetBrains success message
 
   Scenario: Activate Asana service
     When I visit project "Shop" services page
     And I click Asana service link
     And I fill Asana settings
-    Then I should see Asana service settings saved
+    Then I should see the Asana success message
diff --git a/features/steps/project/services.rb b/features/steps/project/services.rb
index 66368a159ec..6bac4df16f8 100644
--- a/features/steps/project/services.rb
+++ b/features/steps/project/services.rb
@@ -34,8 +34,8 @@ class Spinach::Features::ProjectServices < Spinach::FeatureSteps
     click_button 'Save'
   end
 
-  step 'I should see hipchat service settings saved' do
-    expect(find_field('Room').value).to eq 'gitlab'
+  step 'I should see the Hipchat success message' do
+    expect(page).to have_content 'HipChat activated.'
   end
 
   step 'I fill hipchat settings with custom server' do
@@ -46,10 +46,6 @@ class Spinach::Features::ProjectServices < Spinach::FeatureSteps
     click_button 'Save'
   end
 
-  step 'I should see hipchat service settings with custom server saved' do
-    expect(find_field('Server').value).to eq 'https://chat.example.com'
-  end
-
   step 'I click pivotaltracker service link' do
     click_link 'PivotalTracker'
   end
@@ -60,8 +56,8 @@ class Spinach::Features::ProjectServices < Spinach::FeatureSteps
     click_button 'Save'
   end
 
-  step 'I should see pivotaltracker service settings saved' do
-    expect(find_field('Token').value).to eq 'verySecret'
+  step 'I should see the Pivotaltracker success message' do
+    expect(page).to have_content 'PivotalTracker activated.'
   end
 
   step 'I click Flowdock service link' do
@@ -74,8 +70,8 @@ class Spinach::Features::ProjectServices < Spinach::FeatureSteps
     click_button 'Save'
   end
 
-  step 'I should see Flowdock service settings saved' do
-    expect(find_field('Token').value).to eq 'verySecret'
+  step 'I should see the Flowdock success message' do
+    expect(page).to have_content 'Flowdock activated.'
   end
 
   step 'I click Assembla service link' do
@@ -88,8 +84,8 @@ class Spinach::Features::ProjectServices < Spinach::FeatureSteps
     click_button 'Save'
   end
 
-  step 'I should see Assembla service settings saved' do
-    expect(find_field('Token').value).to eq 'verySecret'
+  step 'I should see the Assembla success message' do
+    expect(page).to have_content 'Assembla activated.'
   end
 
   step 'I click Asana service link' do
@@ -103,9 +99,8 @@ class Spinach::Features::ProjectServices < Spinach::FeatureSteps
     click_button 'Save'
   end
 
-  step 'I should see Asana service settings saved' do
-    expect(find_field('Api key').value).to eq 'verySecret'
-    expect(find_field('Restrict to branch').value).to eq 'master'
+  step 'I should see the Asana success message' do
+    expect(page).to have_content 'Asana activated.'
   end
 
   step 'I click email on push service link' do
@@ -113,12 +108,13 @@ class Spinach::Features::ProjectServices < Spinach::FeatureSteps
   end
 
   step 'I fill email on push settings' do
+    check 'Active'
     fill_in 'Recipients', with: 'qa@company.name'
     click_button 'Save'
   end
 
-  step 'I should see email on push service settings saved' do
-    expect(find_field('Recipients').value).to eq 'qa@company.name'
+  step 'I should see the Emails on push success message' do
+    expect(page).to have_content 'Emails on push activated.'
   end
 
   step 'I click Irker service link' do
@@ -132,9 +128,8 @@ class Spinach::Features::ProjectServices < Spinach::FeatureSteps
     click_button 'Save'
   end
 
-  step 'I should see Irker service settings saved' do
-    expect(find_field('Recipients').value).to eq 'irc://chat.freenode.net/#commits'
-    expect(find_field('Colorize messages').value).to eq '1'
+  step 'I should see the Irker success message' do
+    expect(page).to have_content 'Irker (IRC gateway) activated.'
   end
 
   step 'I click Slack notifications service link' do
@@ -147,8 +142,8 @@ class Spinach::Features::ProjectServices < Spinach::FeatureSteps
     click_button 'Save'
   end
 
-  step 'I should see Slack Notifications service settings saved' do
-    expect(find_field('Webhook').value).to eq 'https://hooks.slack.com/services/SVRWFV0VVAR97N/B02R25XN3/ZBqu7xMupaEEICInN685'
+  step 'I should see the Slack notifications success message' do
+    expect(page).to have_content 'Slack notifications activated.'
   end
 
   step 'I click Pushover service link' do
@@ -165,12 +160,8 @@ class Spinach::Features::ProjectServices < Spinach::FeatureSteps
     click_button 'Save'
   end
 
-  step 'I should see Pushover service settings saved' do
-    expect(find_field('Api key').value).to eq 'verySecret'
-    expect(find_field('User key').value).to eq 'verySecret'
-    expect(find_field('Device').value).to eq 'myDevice'
-    expect(find_field('Priority').find('option[selected]').value).to eq '1'
-    expect(find_field('Sound').find('option[selected]').value).to eq 'bike'
+  step 'I should see the Pushover success message' do
+    expect(page).to have_content 'Pushover activated.'
   end
 
   step 'I click jira service link' do
@@ -178,6 +169,8 @@ class Spinach::Features::ProjectServices < Spinach::FeatureSteps
   end
 
   step 'I fill jira settings' do
+    check 'Active'
+
     fill_in 'Web URL', with: 'http://jira.example'
     fill_in 'JIRA API URL', with: 'http://jira.example/api'
     fill_in 'Username', with: 'gitlab'
@@ -186,11 +179,8 @@ class Spinach::Features::ProjectServices < Spinach::FeatureSteps
     click_button 'Save'
   end
 
-  step 'I should see jira service settings saved' do
-    expect(find_field('Web URL').value).to eq 'http://jira.example'
-    expect(find_field('JIRA API URL').value).to eq 'http://jira.example/api'
-    expect(find_field('Username').value).to eq 'gitlab'
-    expect(find_field('Project Key').value).to eq 'GITLAB'
+  step 'I should see the JIRA success message' do
+    expect(page).to have_content 'JIRA activated.'
   end
 
   step 'I click Atlassian Bamboo CI service link' do
@@ -206,13 +196,13 @@ class Spinach::Features::ProjectServices < Spinach::FeatureSteps
     click_button 'Save'
   end
 
-  step 'I should see Atlassian Bamboo CI service settings saved' do
-    expect(find_field('Bamboo url').value).to eq 'http://bamboo.example.com'
-    expect(find_field('Build key').value).to eq 'KEY'
-    expect(find_field('Username').value).to eq 'user'
+  step 'I should see the Bamboo success message' do
+    expect(page).to have_content 'Atlassian Bamboo CI activated.'
   end
 
   step 'I should see empty field Change Password' do
+    click_link 'Atlassian Bamboo CI'
+
     expect(find_field('Enter new password').value).to be_nil
   end
 
@@ -229,9 +219,7 @@ class Spinach::Features::ProjectServices < Spinach::FeatureSteps
     click_button 'Save'
   end
 
-  step 'I should see JetBrains TeamCity CI service settings saved' do
-    expect(find_field('Teamcity url').value).to eq 'http://teamcity.example.com'
-    expect(find_field('Build type').value).to eq 'GitlabTest_Build'
-    expect(find_field('Username').value).to eq 'user'
+  step 'I should see the JetBrains success message' do
+    expect(page).to have_content 'JetBrains TeamCity CI activated.'
   end
 end
diff --git a/spec/controllers/projects/services_controller_spec.rb b/spec/controllers/projects/services_controller_spec.rb
index 2d892f4a2b7..7aa2492edf6 100644
--- a/spec/controllers/projects/services_controller_spec.rb
+++ b/spec/controllers/projects/services_controller_spec.rb
@@ -3,7 +3,9 @@ require 'spec_helper'
 describe Projects::ServicesController do
   let(:project) { create(:project, :repository) }
   let(:user)    { create(:user) }
-  let(:service) { create(:service, project: project) }
+  let(:service) { create(:hipchat_service, project: project) }
+  let(:hipchat_client) { { '#room' => double(send: true) } }
+  let(:service_params) { { token: 'hipchat_token_p', room: '#room' } }
 
   before do
     sign_in(user)
@@ -13,97 +15,84 @@ describe Projects::ServicesController do
     controller.instance_variable_set(:@service, service)
   end
 
-  shared_examples_for 'services controller' do |referrer|
-    before do
-      request.env["HTTP_REFERER"] = referrer
+  describe '#test' do
+    context 'when can_test? returns false' do
+      it 'renders 404' do
+        allow_any_instance_of(Service).to receive(:can_test?).and_return(false)
+
+        put :test, namespace_id: project.namespace.id, project_id: project.id, id: service.id
+
+        expect(response).to have_http_status(404)
+      end
     end
 
-    describe "#test" do
-      context 'when can_test? returns false' do
-        it 'renders 404' do
-          allow_any_instance_of(Service).to receive(:can_test?).and_return(false)
+    context 'success' do
+      context 'with empty project' do
+        let(:project) { create(:empty_project) }
 
-          get :test, namespace_id: project.namespace.id, project_id: project.id, id: service.id, format: :html
+        context 'with chat notification service' do
+          let(:service) { project.create_microsoft_teams_service(webhook: 'http://webhook.com') }
 
-          expect(response).to have_http_status(404)
-        end
-      end
+          it 'returns success' do
+            allow_any_instance_of(MicrosoftTeams::Notifier).to receive(:ping).and_return(true)
 
-      context 'success' do
-        context 'with empty project' do
-          let(:project) { create(:empty_project) }
+            put :test, namespace_id: project.namespace.id, project_id: project.id, id: service.id
 
-          context 'with chat notification service' do
-            let(:service) { project.create_microsoft_teams_service(webhook: 'http://webhook.com') }
-
-            it 'redirects and show success message' do
-              allow_any_instance_of(MicrosoftTeams::Notifier).to receive(:ping).and_return(true)
-
-              get :test, namespace_id: project.namespace.id, project_id: project.id, id: service.id, format: :html
-
-              expect(response).to redirect_to(root_path)
-              expect(flash[:notice]).to eq('We sent a request to the provided URL')
-            end
-          end
-
-          it 'redirects and show success message' do
-            expect(service).to receive(:test).and_return(success: true, result: 'done')
-
-            get :test, namespace_id: project.namespace.id, project_id: project.id, id: service.id, format: :html
-
-            expect(response).to redirect_to(root_path)
-            expect(flash[:notice]).to eq('We sent a request to the provided URL')
+            expect(response.status).to eq(200)
           end
         end
 
-        it "redirects and show success message" do
-          expect(service).to receive(:test).and_return(success: true, result: 'done')
+        it 'returns success' do
+          expect(HipChat::Client).to receive(:new).with('hipchat_token_p', anything).and_return(hipchat_client)
 
-          get :test, namespace_id: project.namespace.id, project_id: project.id, id: service.id, format: :html
+          put :test, namespace_id: project.namespace.id, project_id: project.id, id: service.id, service: service_params
 
-          expect(response).to redirect_to(root_path)
-          expect(flash[:notice]).to eq('We sent a request to the provided URL')
+          expect(response.status).to eq(200)
         end
       end
 
-      context 'failure' do
-        it "redirects and show failure message" do
-          expect(service).to receive(:test).and_return(success: false, result: 'Bad test')
+      it 'returns success' do
+        expect(HipChat::Client).to receive(:new).with('hipchat_token_p', anything).and_return(hipchat_client)
 
-          get :test, namespace_id: project.namespace.id, project_id: project.id, id: service.id, format: :html
+        put :test, namespace_id: project.namespace.id, project_id: project.id, id: service.id, service: service_params
 
-          expect(response).to redirect_to(root_path)
-          expect(flash[:alert]).to eq('We tried to send a request to the provided URL but an error occurred: Bad test')
-        end
+        expect(response.status).to eq(200)
+      end
+
+      def built_service
       end
     end
-  end
 
-  describe 'referrer defined' do
-    it_should_behave_like 'services controller' do
-      let!(:referrer) { "/" }
-    end
-  end
+    context 'failure' do
+      it 'returns 500 status code and the error message' do
+        expect(HipChat::Client).to receive(:new).with('hipchat_token_p', anything).and_raise('Bad test')
 
-  describe 'referrer undefined' do
-    it_should_behave_like 'services controller' do
-      let!(:referrer) { nil }
+        put :test, namespace_id: project.namespace.id, project_id: project.id, id: service.id, service: service_params
+
+        expect(response.status).to eq(200)
+        expect(JSON.parse(response.body)).
+          to eq('error' => true, 'message' => 'Test failed', 'service_response' => 'Bad test')
+      end
     end
   end
 
   describe 'PUT #update' do
-    context 'on successful update' do
-      it 'sets the flash' do
-        expect(service).to receive(:to_param).and_return('hipchat')
-        expect(service).to receive(:event_names).and_return(HipchatService.event_names)
-
+    context 'when param `active` is set to true' do
+      it 'activates the service and redirects to integrations paths' do
         put :update,
-          namespace_id: project.namespace.id,
-          project_id: project.id,
-          id: service.id,
-          service: { active: false }
+          namespace_id: project.namespace.id, project_id: project.id, id: service.id, service: { active: true }
 
-        expect(flash[:notice]).to eq 'Successfully updated.'
+        expect(response).to redirect_to(namespace_project_settings_integrations_path(project.namespace, project))
+        expect(flash[:notice]).to eq 'HipChat activated.'
+      end
+    end
+
+    context 'when param `active` is set to false' do
+      it 'does not  activate the service but saves the settings' do
+        put :update,
+          namespace_id: project.namespace.id, project_id: project.id, id: service.id, service: { active: false }
+
+        expect(flash[:notice]).to eq 'HipChat settings saved, but not activated.'
       end
     end
   end
diff --git a/spec/factories/services.rb b/spec/factories/services.rb
index 3fad4d2d658..e7366a7fd1c 100644
--- a/spec/factories/services.rb
+++ b/spec/factories/services.rb
@@ -33,4 +33,10 @@ FactoryGirl.define do
       project_key: 'jira-key'
     )
   end
+
+  factory :hipchat_service do
+    project factory: :empty_project
+    type 'HipchatService'
+    token 'test_token'
+  end
 end
diff --git a/spec/features/projects/services/mattermost_slash_command_spec.rb b/spec/features/projects/services/mattermost_slash_command_spec.rb
index dc3854262e7..bb12f64a552 100644
--- a/spec/features/projects/services/mattermost_slash_command_spec.rb
+++ b/spec/features/projects/services/mattermost_slash_command_spec.rb
@@ -24,15 +24,25 @@ feature 'Setup Mattermost slash commands', :feature, :js do
       expect(token_placeholder).to eq('XXxxXXxxXXxxXXxxXXxxXXxx')
     end
 
-    it 'shows the token after saving' do
+    it 'redirects to the integrations page after saving but not activating' do
       token = ('a'..'z').to_a.join
 
       fill_in 'service_token', with: token
       click_on 'Save'
 
-      value = find_field('service_token').value
+      expect(current_path).to eq(namespace_project_settings_integrations_path(project.namespace, project))
+      expect(page).to have_content('Mattermost slash commands settings saved, but not activated.')
+    end
 
-      expect(value).to eq(token)
+    it 'redirects to the integrations page after activating' do
+      token = ('a'..'z').to_a.join
+
+      fill_in 'service_token', with: token
+      check 'service_active'
+      click_on 'Save'
+
+      expect(current_path).to eq(namespace_project_settings_integrations_path(project.namespace, project))
+      expect(page).to have_content(' Mattermost slash commands activated.')
     end
 
     it 'shows the add to mattermost button' do
diff --git a/spec/features/projects/services/slack_slash_command_spec.rb b/spec/features/projects/services/slack_slash_command_spec.rb
index db903a0c8f0..f53b820c460 100644
--- a/spec/features/projects/services/slack_slash_command_spec.rb
+++ b/spec/features/projects/services/slack_slash_command_spec.rb
@@ -21,13 +21,21 @@ feature 'Slack slash commands', feature: true do
     expect(page).to have_content('This service allows users to perform common')
   end
 
-  it 'shows the token after saving' do
+  it 'redirects to the integrations page after saving but not activating' do
     fill_in 'service_token', with: 'token'
     click_on 'Save'
 
-    value = find_field('service_token').value
+    expect(current_path).to eq(namespace_project_settings_integrations_path(project.namespace, project))
+    expect(page).to have_content('Slack slash commands settings saved, but not activated.')
+  end
 
-    expect(value).to eq('token')
+  it 'redirects to the integrations page after activating' do
+    fill_in 'service_token', with: 'token'
+    check 'service_active'
+    click_on 'Save'
+
+    expect(current_path).to eq(namespace_project_settings_integrations_path(project.namespace, project))
+    expect(page).to have_content('Slack slash commands activated.')
   end
 
   it 'shows the correct trigger url' do

From 338a73ec03ceb2b61a824aebe4437a52b56f0f03 Mon Sep 17 00:00:00 2001
From: kushalpandya <kushal@gitlab.com>
Date: Thu, 25 May 2017 14:16:52 +0530
Subject: [PATCH 02/26] Add support for required input validation

---
 app/views/shared/_field.html.haml | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/app/views/shared/_field.html.haml b/app/views/shared/_field.html.haml
index d67e20e988d..3bda8cd5e9c 100644
--- a/app/views/shared/_field.html.haml
+++ b/app/views/shared/_field.html.haml
@@ -15,14 +15,14 @@
     = form.label name, title, class: "control-label"
   .col-sm-10
     - if type == 'text'
-      = form.text_field name, class: "form-control", placeholder: placeholder
+      = form.text_field name, class: "form-control", placeholder: placeholder, required: required
     - elsif type == 'textarea'
-      = form.text_area name, rows: 5, class: "form-control", placeholder: placeholder
+      = form.text_area name, rows: 5, class: "form-control", placeholder: placeholder, required: required
     - elsif type == 'checkbox'
       = form.check_box name
     - elsif type == 'select'
       = form.select name, options_for_select(choices, value ? value : default_choice), {}, { class: "form-control" }
     - elsif type == 'password'
-      = form.password_field name, autocomplete: "new-password", class: "form-control"
+      = form.password_field name, autocomplete: "new-password", class: "form-control", required: value.present? ? nil : :required
     - if help
       %span.help-block= help

From f5f20274fa6b27b40182e4d1a2c7164f4ca51543 Mon Sep 17 00:00:00 2001
From: kushalpandya <kushal@gitlab.com>
Date: Thu, 25 May 2017 14:17:43 +0530
Subject: [PATCH 03/26] Integrations Bundle

---
 app/assets/javascripts/integrations/index.js  |   6 ++
 .../integrations/integration_settings_form.js | 100 ++++++++++++++++++
 2 files changed, 106 insertions(+)
 create mode 100644 app/assets/javascripts/integrations/index.js
 create mode 100644 app/assets/javascripts/integrations/integration_settings_form.js

diff --git a/app/assets/javascripts/integrations/index.js b/app/assets/javascripts/integrations/index.js
new file mode 100644
index 00000000000..82255fd284e
--- /dev/null
+++ b/app/assets/javascripts/integrations/index.js
@@ -0,0 +1,6 @@
+/* eslint-disable no-new */
+import IntegrationSettingsForm from './integration_settings_form';
+
+$(() => {
+  new IntegrationSettingsForm('.js-integration-settings-from');
+});
diff --git a/app/assets/javascripts/integrations/integration_settings_form.js b/app/assets/javascripts/integrations/integration_settings_form.js
new file mode 100644
index 00000000000..b3adc15e3ee
--- /dev/null
+++ b/app/assets/javascripts/integrations/integration_settings_form.js
@@ -0,0 +1,100 @@
+/* global Flash */
+
+export default class IntegrationSettingsForm {
+  constructor(formSelector) {
+    this.$form = $(formSelector);
+
+    // Form Metadata
+    this.endPoint = this.$form.attr('action');
+
+    // Form Child Elements
+    this.$serviceToggle = this.$form.find('#service_active');
+    this.$submitBtn = this.$form.find('button[type="submit"]');
+    this.$submitBtnLoader = this.$submitBtn.find('.js-btn-spinner');
+    this.$submitBtnLabel = this.$submitBtn.find('.js-btn-label');
+
+    // Class Member methods
+    this.handleServiceToggle = this.handleServiceToggle.bind(this);
+    this.handleSettingsSave = this.handleSettingsSave.bind(this);
+
+    this.init();
+  }
+
+  init() {
+    // Initialize View
+    this.toggleSubmitBtnLabel(this.$serviceToggle.is(':checked'));
+
+    // Bind Event Listeners
+    this.$serviceToggle.on('change', this.handleServiceToggle);
+    this.$submitBtn.on('click', this.handleSettingsSave);
+  }
+
+  handleSettingsSave(e) {
+    if (this.$serviceToggle.is(':checked')) {
+      e.preventDefault();
+      this.testSettings(this.$form.serialize());
+    }
+  }
+
+  handleServiceToggle(e) {
+    this.toggleSubmitBtnLabel($(e.currentTarget).is(':checked'));
+  }
+
+  /**
+   * Toggle Submit button label based on Integration status
+   */
+  toggleSubmitBtnLabel(serviceActive) {
+    this.$submitBtnLabel.text(
+      serviceActive ?
+        'Test settings and save changes' :
+        'Save changes');
+  }
+
+  /**
+   * Toggle Submit button state based on provided boolean value of `saveTestActive`
+   * When enabled, it does two things, and reverts back when disabled
+   *
+   * 1. It shows load spinner on submit button
+   * 2. Makes submit button disabled
+   */
+  toggleSubmitBtnState(saveTestActive) {
+    if (saveTestActive) {
+      this.$submitBtn.disable();
+      this.$submitBtnLoader.removeClass('hidden');
+    } else {
+      this.$submitBtn.enable();
+      this.$submitBtnLoader.addClass('hidden');
+    }
+  }
+
+  /* eslint-disable promise/catch-or-return, no-new */
+  /**
+   * Test Integration config
+   */
+  testSettings(formData) {
+    this.toggleSubmitBtnState(true);
+    $.ajax({
+      type: 'PUT',
+      url: `${this.endPoint}/test`,
+      data: formData,
+    })
+    .done((res) => {
+      if (res.error) {
+        new Flash(`${res.message}.`, null, null, {
+          title: 'Save anyway',
+          clickHandler: () => {
+            this.$form.submit();
+          },
+        });
+      } else {
+        this.$form.submit();
+      }
+    })
+    .fail(() => {
+      new Flash('Something went wrong on our end.');
+    })
+    .always(() => {
+      this.toggleSubmitBtnState(false);
+    });
+  }
+}

From c1ef4347ed2f2767e045f0d145092abd7d525465 Mon Sep 17 00:00:00 2001
From: kushalpandya <kushal@gitlab.com>
Date: Thu, 25 May 2017 14:18:10 +0530
Subject: [PATCH 04/26] Add support for action links

---
 app/assets/javascripts/flash.js             | 15 +++++++++++++--
 app/assets/stylesheets/framework/flash.scss | 16 ++++++++++++++++
 2 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/app/assets/javascripts/flash.js b/app/assets/javascripts/flash.js
index eec30624ff2..4d00858f7e8 100644
--- a/app/assets/javascripts/flash.js
+++ b/app/assets/javascripts/flash.js
@@ -7,8 +7,8 @@ window.Flash = (function() {
     return $(this).fadeOut();
   };
 
-  function Flash(message, type, parent) {
-    var flash, textDiv;
+  function Flash(message, type, parent, actionConfig) {
+    var flash, textDiv, actionLink;
     if (type == null) {
       type = 'alert';
     }
@@ -30,6 +30,17 @@ window.Flash = (function() {
       text: message
     });
     textDiv.appendTo(flash);
+
+    if (actionConfig) {
+      actionLink = $('<a/>', {
+        "class": "flash-action",
+        "href": "#",
+        text: actionConfig.title
+      });
+
+      actionLink.appendTo(flash);
+      this.flashContainer.on('click', '.flash-action', actionConfig.clickHandler);
+    }
     if (this.flashContainer.parent().hasClass('content-wrapper')) {
       textDiv.addClass('container-fluid container-limited');
     }
diff --git a/app/assets/stylesheets/framework/flash.scss b/app/assets/stylesheets/framework/flash.scss
index 25b4feca3c3..38d884bc7eb 100644
--- a/app/assets/stylesheets/framework/flash.scss
+++ b/app/assets/stylesheets/framework/flash.scss
@@ -16,6 +16,22 @@
     @extend .alert;
     @extend .alert-danger;
     margin: 0;
+
+    .flash-text,
+    .flash-action {
+      display: inline-block;
+    }
+
+    a.flash-action {
+      margin-left: 5px;
+      text-decoration: none;
+      font-weight: normal;
+      border-bottom: 1px solid;
+
+      &:hover {
+        border-color: transparent;
+      }
+    }
   }
 
   .flash-notice,

From fc2cc1e235f9d760b9b69fe5911f8e4396ab8965 Mon Sep 17 00:00:00 2001
From: kushalpandya <kushal@gitlab.com>
Date: Thu, 25 May 2017 14:22:07 +0530
Subject: [PATCH 05/26] Remove test settings button, enhance Save changes
 button, include Integrations bundle

---
 app/views/projects/services/_form.html.haml | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/app/views/projects/services/_form.html.haml b/app/views/projects/services/_form.html.haml
index f1a80f1d5e1..b6ff2806793 100644
--- a/app/views/projects/services/_form.html.haml
+++ b/app/views/projects/services/_form.html.haml
@@ -1,3 +1,6 @@
+- content_for :page_specific_javascripts do
+  = webpack_bundle_tag('integrations')
+
 .row.prepend-top-default.append-bottom-default
   .col-lg-3
     %h4.prepend-top-0
@@ -6,15 +9,17 @@
 
     %p= @service.description
   .col-lg-9
-    = form_for(@service, as: :service, url: namespace_project_service_path(@project.namespace, @project, @service.to_param), method: :put, html: { class: 'form-horizontal' }) do |form|
+    = form_for(@service, as: :service, url: namespace_project_service_path(@project.namespace, @project, @service.to_param), method: :put, html: { class: 'gl-show-field-errors form-horizontal js-integration-settings-from' }) do |form|
       = render 'shared/service_settings', form: form, subject: @service
       .footer-block.row-content-block
-        = form.submit 'Save changes', class: 'btn btn-save'
+        %button.btn.btn-save{ type: 'submit' }
+          %i.fa.fa-spinner.fa-spin.hidden.js-btn-spinner{ "aria-hidden" => true }
+          %span.js-btn-label
+            Save changes
         &nbsp;
         - if @service.valid? && @service.activated?
           - unless @service.can_test?
             - disabled_class = 'disabled'
             - disabled_title = @service.disabled_title
 
-          = link_to 'Test settings', test_namespace_project_service_path(@project.namespace, @project, @service), class: "btn #{disabled_class}", title: disabled_title
         = link_to "Cancel", namespace_project_settings_integrations_path(@project.namespace, @project), class: "btn btn-cancel"

From fb1ba0a55f07eca026b67c0c48c00be3569653b2 Mon Sep 17 00:00:00 2001
From: kushalpandya <kushal@gitlab.com>
Date: Thu, 25 May 2017 14:22:28 +0530
Subject: [PATCH 06/26] Add Integrations bundle

---
 config/webpack.config.js | 1 +
 1 file changed, 1 insertion(+)

diff --git a/config/webpack.config.js b/config/webpack.config.js
index c77b1d6334c..d3c09103c5f 100644
--- a/config/webpack.config.js
+++ b/config/webpack.config.js
@@ -41,6 +41,7 @@ var config = {
     group:                './group.js',
     groups_list:          './groups_list.js',
     issue_show:           './issue_show/index.js',
+    integrations:         './integrations',
     locale:               './locale/index.js',
     main:                 './main.js',
     merge_conflicts:      './merge_conflicts/merge_conflicts_bundle.js',

From 791e5024efbfaadf342f9a019b59e65af0249c3b Mon Sep 17 00:00:00 2001
From: kushalpandya <kushal@gitlab.com>
Date: Thu, 25 May 2017 14:24:02 +0530
Subject: [PATCH 07/26] Prevent default action of Flash banner action link
 click

---
 .../javascripts/integrations/integration_settings_form.js      | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/app/assets/javascripts/integrations/integration_settings_form.js b/app/assets/javascripts/integrations/integration_settings_form.js
index b3adc15e3ee..bb551b5bb4c 100644
--- a/app/assets/javascripts/integrations/integration_settings_form.js
+++ b/app/assets/javascripts/integrations/integration_settings_form.js
@@ -82,7 +82,8 @@ export default class IntegrationSettingsForm {
       if (res.error) {
         new Flash(`${res.message}.`, null, null, {
           title: 'Save anyway',
-          clickHandler: () => {
+          clickHandler: (e) => {
+            e.preventDefault();
             this.$form.submit();
           },
         });

From c5e28a7b411fee60b9151f1477c420af46bcdf02 Mon Sep 17 00:00:00 2001
From: kushalpandya <kushal@gitlab.com>
Date: Thu, 25 May 2017 15:05:19 +0530
Subject: [PATCH 08/26] Validate config form only when service is marked active

---
 .../integrations/integration_settings_form.js | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/app/assets/javascripts/integrations/integration_settings_form.js b/app/assets/javascripts/integrations/integration_settings_form.js
index bb551b5bb4c..499d73b49a4 100644
--- a/app/assets/javascripts/integrations/integration_settings_form.js
+++ b/app/assets/javascripts/integrations/integration_settings_form.js
@@ -22,7 +22,7 @@ export default class IntegrationSettingsForm {
 
   init() {
     // Initialize View
-    this.toggleSubmitBtnLabel(this.$serviceToggle.is(':checked'));
+    this.toggleServiceState(this.$serviceToggle.is(':checked'));
 
     // Bind Event Listeners
     this.$serviceToggle.on('change', this.handleServiceToggle);
@@ -31,13 +31,24 @@ export default class IntegrationSettingsForm {
 
   handleSettingsSave(e) {
     if (this.$serviceToggle.is(':checked')) {
-      e.preventDefault();
-      this.testSettings(this.$form.serialize());
+      if (this.$form.get(0).checkValidity()) {
+        e.preventDefault();
+        this.testSettings(this.$form.serialize());
+      }
     }
   }
 
   handleServiceToggle(e) {
-    this.toggleSubmitBtnLabel($(e.currentTarget).is(':checked'));
+    this.toggleServiceState($(e.currentTarget).is(':checked'));
+  }
+
+  toggleServiceState(serviceActive) {
+    this.toggleSubmitBtnLabel(serviceActive);
+    if (serviceActive) {
+      this.$form.removeAttr('novalidate');
+    } else if (!this.$form.attr('novalidate')) {
+      this.$form.attr('novalidate', 'novalidate');
+    }
   }
 
   /**

From 69c47658c7d21c577c6ce73806e62be9ff713084 Mon Sep 17 00:00:00 2001
From: kushalpandya <kushal@gitlab.com>
Date: Thu, 25 May 2017 15:05:51 +0530
Subject: [PATCH 09/26] Fix bug where form's `novalidate` attribute is not
 respected

---
 app/assets/javascripts/gl_field_errors.js | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/app/assets/javascripts/gl_field_errors.js b/app/assets/javascripts/gl_field_errors.js
index 4f226ff96ea..4bef60264bb 100644
--- a/app/assets/javascripts/gl_field_errors.js
+++ b/app/assets/javascripts/gl_field_errors.js
@@ -31,9 +31,13 @@ class GlFieldErrors {
    * and prevents disabling of invalid submit button by application.js */
 
   catchInvalidFormSubmit (event) {
-    if (!event.currentTarget.checkValidity()) {
-      event.preventDefault();
-      event.stopPropagation();
+    const $form = $(event.currentTarget);
+
+    if (!$form.attr('novalidate')) {
+      if (!event.currentTarget.checkValidity()) {
+        event.preventDefault();
+        event.stopPropagation();
+      }
     }
   }
 

From 095d9b5b238caf27ac9a9023bfed81629e9ecb5c Mon Sep 17 00:00:00 2001
From: kushalpandya <kushal@gitlab.com>
Date: Mon, 29 May 2017 12:16:02 +0530
Subject: [PATCH 10/26] Use `@service.can_test?` to enable test before save

---
 app/views/projects/services/_form.html.haml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/views/projects/services/_form.html.haml b/app/views/projects/services/_form.html.haml
index b6ff2806793..0d7002f6ba5 100644
--- a/app/views/projects/services/_form.html.haml
+++ b/app/views/projects/services/_form.html.haml
@@ -9,7 +9,7 @@
 
     %p= @service.description
   .col-lg-9
-    = form_for(@service, as: :service, url: namespace_project_service_path(@project.namespace, @project, @service.to_param), method: :put, html: { class: 'gl-show-field-errors form-horizontal js-integration-settings-from' }) do |form|
+    = form_for(@service, as: :service, url: namespace_project_service_path(@project.namespace, @project, @service.to_param), method: :put, html: { class: 'gl-show-field-errors form-horizontal js-integration-settings-from', data: { "can-test" => "#{@service.can_test?}" } }) do |form|
       = render 'shared/service_settings', form: form, subject: @service
       .footer-block.row-content-block
         %button.btn.btn-save{ type: 'submit' }

From 44f2504a811b107a35403d9c0a8b8fce66f502c8 Mon Sep 17 00:00:00 2001
From: kushalpandya <kushal@gitlab.com>
Date: Mon, 29 May 2017 12:16:34 +0530
Subject: [PATCH 11/26] Initialize canTestService from form meta

---
 .../integrations/integration_settings_form.js          | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/app/assets/javascripts/integrations/integration_settings_form.js b/app/assets/javascripts/integrations/integration_settings_form.js
index 499d73b49a4..5d555a3776e 100644
--- a/app/assets/javascripts/integrations/integration_settings_form.js
+++ b/app/assets/javascripts/integrations/integration_settings_form.js
@@ -6,6 +6,7 @@ export default class IntegrationSettingsForm {
 
     // Form Metadata
     this.endPoint = this.$form.attr('action');
+    this.canTestService = this.$form.data('can-test');
 
     // Form Child Elements
     this.$serviceToggle = this.$form.find('#service_active');
@@ -31,7 +32,8 @@ export default class IntegrationSettingsForm {
 
   handleSettingsSave(e) {
     if (this.$serviceToggle.is(':checked')) {
-      if (this.$form.get(0).checkValidity()) {
+      if (this.$form.get(0).checkValidity() &&
+          this.canTestService) {
         e.preventDefault();
         this.testSettings(this.$form.serialize());
       }
@@ -43,7 +45,7 @@ export default class IntegrationSettingsForm {
   }
 
   toggleServiceState(serviceActive) {
-    this.toggleSubmitBtnLabel(serviceActive);
+    this.toggleSubmitBtnLabel(serviceActive, this.canTestService);
     if (serviceActive) {
       this.$form.removeAttr('novalidate');
     } else if (!this.$form.attr('novalidate')) {
@@ -54,9 +56,9 @@ export default class IntegrationSettingsForm {
   /**
    * Toggle Submit button label based on Integration status
    */
-  toggleSubmitBtnLabel(serviceActive) {
+  toggleSubmitBtnLabel(serviceActive, canTestService) {
     this.$submitBtnLabel.text(
-      serviceActive ?
+      serviceActive && canTestService ?
         'Test settings and save changes' :
         'Save changes');
   }

From b71025c014babf9663e0451ad21eabde91570259 Mon Sep 17 00:00:00 2001
From: Jarka Kadlecova <jarka@gitlab.com>
Date: Fri, 26 May 2017 12:50:52 +0200
Subject: [PATCH 12/26] Add feature tests for improved JIRA settings

---
 .../integrations/integration_settings_form.js |  2 +-
 .../projects/services_controller.rb           | 21 +++--
 .../chat_notification_service.rb              |  4 -
 app/models/project_services/jira_service.rb   |  4 -
 changelogs/unreleased/31511-jira-settings.yml |  4 +
 .../projects/services/jira_service_spec.rb    | 79 +++++++++++++++++++
 .../services/mattermost_slash_command_spec.rb |  6 +-
 7 files changed, 100 insertions(+), 20 deletions(-)
 create mode 100644 changelogs/unreleased/31511-jira-settings.yml
 create mode 100644 spec/features/projects/services/jira_service_spec.rb

diff --git a/app/assets/javascripts/integrations/integration_settings_form.js b/app/assets/javascripts/integrations/integration_settings_form.js
index 5d555a3776e..5ecab0b11a2 100644
--- a/app/assets/javascripts/integrations/integration_settings_form.js
+++ b/app/assets/javascripts/integrations/integration_settings_form.js
@@ -93,7 +93,7 @@ export default class IntegrationSettingsForm {
     })
     .done((res) => {
       if (res.error) {
-        new Flash(`${res.message}.`, null, null, {
+        new Flash(`${res.message}`, null, null, {
           title: 'Save anyway',
           clickHandler: (e) => {
             e.preventDefault();
diff --git a/app/controllers/projects/services_controller.rb b/app/controllers/projects/services_controller.rb
index 89c4e81a36f..264665942f8 100644
--- a/app/controllers/projects/services_controller.rb
+++ b/app/controllers/projects/services_controller.rb
@@ -22,17 +22,22 @@ class Projects::ServicesController < Projects::ApplicationController
   end
 
   def test
-    return render json: {}, status: :not_found unless @service.can_test?
-
-    data = @service.test_data(project, current_user)
-    outcome = @service.test(data)
-
     message = {}
-    unless outcome[:success]
-      message = { error: true, message: 'Test failed', service_response: outcome[:result].to_s }
+
+    if @service.can_test?
+      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 }
+      end
+
+      status = :ok
+    else
+      status = :not_found
     end
 
-    render json: message, status: :ok
+    render json: message, status: status
   end
 
   private
diff --git a/app/models/project_services/chat_notification_service.rb b/app/models/project_services/chat_notification_service.rb
index da7a4067d8e..6d1a321f651 100644
--- a/app/models/project_services/chat_notification_service.rb
+++ b/app/models/project_services/chat_notification_service.rb
@@ -21,10 +21,6 @@ class ChatNotificationService < Service
     end
   end
 
-  def can_test?
-    valid?
-  end
-
   def self.supported_events
     %w[push issue confidential_issue merge_request note tag_push
        pipeline wiki_page]
diff --git a/app/models/project_services/jira_service.rb b/app/models/project_services/jira_service.rb
index 5384d75994a..489208a3fd6 100644
--- a/app/models/project_services/jira_service.rb
+++ b/app/models/project_services/jira_service.rb
@@ -175,10 +175,6 @@ class JiraService < IssueTrackerService
     { success: result.present?, result: result }
   end
 
-  def can_test?
-    username.present? && password.present?
-  end
-
   # JIRA does not need test data.
   # We are requesting the project that belongs to the project key.
   def test_data(user = nil, project = nil)
diff --git a/changelogs/unreleased/31511-jira-settings.yml b/changelogs/unreleased/31511-jira-settings.yml
new file mode 100644
index 00000000000..3592e92b49e
--- /dev/null
+++ b/changelogs/unreleased/31511-jira-settings.yml
@@ -0,0 +1,4 @@
+---
+title: Simplify test&save actions when setting a service integration
+merge_request: 11599
+author:
diff --git a/spec/features/projects/services/jira_service_spec.rb b/spec/features/projects/services/jira_service_spec.rb
new file mode 100644
index 00000000000..d40c332be6f
--- /dev/null
+++ b/spec/features/projects/services/jira_service_spec.rb
@@ -0,0 +1,79 @@
+require 'spec_helper'
+
+feature 'Setup Jira service', :feature, :js do
+  let(:user) { create(:user) }
+  let(:project) { create(:empty_project) }
+  let(:service) { project.create_jira_service }
+
+  let(:url) { 'http://jira.example.com' }
+  let(:project_url) { 'http://username:password@jira.example.com/rest/api/2/project/GitLabProject' }
+
+  def fill_form(active = true)
+    check 'Active' if active
+
+    fill_in 'service_url', with: url
+    fill_in 'service_project_key', with: 'GitLabProject'
+    fill_in 'service_username', with: 'username'
+    fill_in 'service_password', with: 'password'
+    fill_in 'service_jira_issue_transition_id', with: '25'
+  end
+
+  before do
+    project.team << [user, :master]
+    login_as(user)
+
+    visit namespace_project_settings_integrations_path(project.namespace, project)
+  end
+
+  describe 'user sets and activates Jira Service' do
+    context 'when Jira connection test succeeds' do
+      before do
+        WebMock.stub_request(:get, project_url)
+      end
+
+      it 'activates the JIRA service' do
+        click_link('JIRA')
+        fill_form
+        click_button('Test settings and save changes')
+        wait_for_ajax
+
+        expect(page).to have_content('JIRA activated.')
+        expect(current_path).to eq(namespace_project_settings_integrations_path(project.namespace, project))
+      end
+    end
+
+    context 'when Jira connection test fails' do
+      before do
+        WebMock.stub_request(:get, project_url).to_return(status: 401)
+      end
+
+      it 'activates the JIRA service' do
+        click_link('JIRA')
+        fill_form
+        click_button('Test settings and save changes')
+        wait_for_ajax
+
+        expect(page).to have_content('Test failed.Save anyway')
+
+        click_on('Save anyway')
+        wait_for_ajax
+
+        expect(page).to have_content('JIRA activated.')
+        expect(current_path).to eq(namespace_project_settings_integrations_path(project.namespace, project))
+      end
+    end
+  end
+
+  describe 'user sets Jira Service but keeps it non active' do
+    context 'when Jira connection test succeeds' do
+      it 'activates the JIRA service' do
+        click_link('JIRA')
+        fill_form(false)
+        click_button('Save changes')
+
+        expect(page).to have_content('JIRA settings saved, but not activated.')
+        expect(current_path).to eq(namespace_project_settings_integrations_path(project.namespace, project))
+      end
+    end
+  end
+end
diff --git a/spec/features/projects/services/mattermost_slash_command_spec.rb b/spec/features/projects/services/mattermost_slash_command_spec.rb
index bb12f64a552..1fe82222e59 100644
--- a/spec/features/projects/services/mattermost_slash_command_spec.rb
+++ b/spec/features/projects/services/mattermost_slash_command_spec.rb
@@ -28,7 +28,7 @@ feature 'Setup Mattermost slash commands', :feature, :js do
       token = ('a'..'z').to_a.join
 
       fill_in 'service_token', with: token
-      click_on 'Save'
+      click_on 'Save changes'
 
       expect(current_path).to eq(namespace_project_settings_integrations_path(project.namespace, project))
       expect(page).to have_content('Mattermost slash commands settings saved, but not activated.')
@@ -39,10 +39,10 @@ feature 'Setup Mattermost slash commands', :feature, :js do
 
       fill_in 'service_token', with: token
       check 'service_active'
-      click_on 'Save'
+      click_on 'Save changes'
 
       expect(current_path).to eq(namespace_project_settings_integrations_path(project.namespace, project))
-      expect(page).to have_content(' Mattermost slash commands activated.')
+      expect(page).to have_content('Mattermost slash commands activated.')
     end
 
     it 'shows the add to mattermost button' do

From 6fad5640e73837ba148b0ed005205adff91d8a4e Mon Sep 17 00:00:00 2001
From: Jarka Kadlecova <jarka@gitlab.com>
Date: Mon, 29 May 2017 11:29:43 +0200
Subject: [PATCH 13/26] fix failing specs

---
 .../projects/services_controller_spec.rb      |  4 +--
 .../projects/services/jira_service_spec.rb    |  8 ++---
 .../project_services/jira_service_spec.rb     | 35 -------------------
 3 files changed, 6 insertions(+), 41 deletions(-)

diff --git a/spec/controllers/projects/services_controller_spec.rb b/spec/controllers/projects/services_controller_spec.rb
index 7aa2492edf6..4fe1e848965 100644
--- a/spec/controllers/projects/services_controller_spec.rb
+++ b/spec/controllers/projects/services_controller_spec.rb
@@ -64,14 +64,14 @@ describe Projects::ServicesController do
     end
 
     context 'failure' do
-      it 'returns 500 status code and the error message' do
+      it 'returns success status code and the error message' do
         expect(HipChat::Client).to receive(:new).with('hipchat_token_p', anything).and_raise('Bad test')
 
         put :test, namespace_id: project.namespace.id, project_id: project.id, id: service.id, service: service_params
 
         expect(response.status).to eq(200)
         expect(JSON.parse(response.body)).
-          to eq('error' => true, 'message' => 'Test failed', 'service_response' => 'Bad test')
+          to eq('error' => true, 'message' => 'Test failed.', 'service_response' => 'Bad test')
       end
     end
   end
diff --git a/spec/features/projects/services/jira_service_spec.rb b/spec/features/projects/services/jira_service_spec.rb
index d40c332be6f..3d951d6b3e3 100644
--- a/spec/features/projects/services/jira_service_spec.rb
+++ b/spec/features/projects/services/jira_service_spec.rb
@@ -35,7 +35,7 @@ feature 'Setup Jira service', :feature, :js do
         click_link('JIRA')
         fill_form
         click_button('Test settings and save changes')
-        wait_for_ajax
+        wait_for_requests
 
         expect(page).to have_content('JIRA activated.')
         expect(current_path).to eq(namespace_project_settings_integrations_path(project.namespace, project))
@@ -51,12 +51,12 @@ feature 'Setup Jira service', :feature, :js do
         click_link('JIRA')
         fill_form
         click_button('Test settings and save changes')
-        wait_for_ajax
+        wait_for_requests
 
         expect(page).to have_content('Test failed.Save anyway')
 
-        click_on('Save anyway')
-        wait_for_ajax
+        find('.flash-alert .flash-action').trigger('click')
+        wait_for_requests
 
         expect(page).to have_content('JIRA activated.')
         expect(current_path).to eq(namespace_project_settings_integrations_path(project.namespace, project))
diff --git a/spec/models/project_services/jira_service_spec.rb b/spec/models/project_services/jira_service_spec.rb
index 1920b5bf42b..0ee050196e4 100644
--- a/spec/models/project_services/jira_service_spec.rb
+++ b/spec/models/project_services/jira_service_spec.rb
@@ -69,41 +69,6 @@ describe JiraService, models: true do
     end
   end
 
-  describe '#can_test?' do
-    let(:jira_service) { described_class.new }
-
-    it 'returns false if username is blank' do
-      allow(jira_service).to receive_messages(
-        url: 'http://jira.example.com',
-        username: '',
-        password: '12345678'
-      )
-
-      expect(jira_service.can_test?).to be_falsy
-    end
-
-    it 'returns false if password is blank' do
-      allow(jira_service).to receive_messages(
-        url: 'http://jira.example.com',
-        username: 'tester',
-        password: ''
-      )
-
-      expect(jira_service.can_test?).to be_falsy
-    end
-
-    it 'returns true if password and username are present' do
-      jira_service = described_class.new
-      allow(jira_service).to receive_messages(
-        url: 'http://jira.example.com',
-        username: 'tester',
-        password: '12345678'
-      )
-
-      expect(jira_service.can_test?).to be_truthy
-    end
-  end
-
   describe '#close_issue' do
     let(:custom_base_url) { 'http://custom_url' }
     let(:user)    { create(:user) }

From 4341801a416a30319cdf646c647b03b629491e05 Mon Sep 17 00:00:00 2001
From: kushalpandya <kushal@gitlab.com>
Date: Tue, 30 May 2017 16:55:50 +0530
Subject: [PATCH 14/26] Fix class name typo

---
 app/assets/javascripts/integrations/index.js | 2 +-
 app/views/projects/services/_form.html.haml  | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/app/assets/javascripts/integrations/index.js b/app/assets/javascripts/integrations/index.js
index 82255fd284e..69db098d981 100644
--- a/app/assets/javascripts/integrations/index.js
+++ b/app/assets/javascripts/integrations/index.js
@@ -2,5 +2,5 @@
 import IntegrationSettingsForm from './integration_settings_form';
 
 $(() => {
-  new IntegrationSettingsForm('.js-integration-settings-from');
+  new IntegrationSettingsForm('.js-integration-settings-form');
 });
diff --git a/app/views/projects/services/_form.html.haml b/app/views/projects/services/_form.html.haml
index 0d7002f6ba5..8eeed5cda09 100644
--- a/app/views/projects/services/_form.html.haml
+++ b/app/views/projects/services/_form.html.haml
@@ -9,7 +9,7 @@
 
     %p= @service.description
   .col-lg-9
-    = form_for(@service, as: :service, url: namespace_project_service_path(@project.namespace, @project, @service.to_param), method: :put, html: { class: 'gl-show-field-errors form-horizontal js-integration-settings-from', data: { "can-test" => "#{@service.can_test?}" } }) do |form|
+    = form_for(@service, as: :service, url: namespace_project_service_path(@project.namespace, @project, @service.to_param), method: :put, html: { class: 'gl-show-field-errors form-horizontal js-integration-settings-form', data: { "can-test" => "#{@service.can_test?}" } }) do |form|
       = render 'shared/service_settings', form: form, subject: @service
       .footer-block.row-content-block
         %button.btn.btn-save{ type: 'submit' }

From f57a4a945a60bd41d46d3253178bff7b1879799f Mon Sep 17 00:00:00 2001
From: kushalpandya <kushal@gitlab.com>
Date: Tue, 30 May 2017 16:58:09 +0530
Subject: [PATCH 15/26] Add comments for method and minor refinements

---
 .../integrations/integration_settings_form.js            | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/app/assets/javascripts/integrations/integration_settings_form.js b/app/assets/javascripts/integrations/integration_settings_form.js
index 5ecab0b11a2..5745475123d 100644
--- a/app/assets/javascripts/integrations/integration_settings_form.js
+++ b/app/assets/javascripts/integrations/integration_settings_form.js
@@ -44,6 +44,9 @@ export default class IntegrationSettingsForm {
     this.toggleServiceState($(e.currentTarget).is(':checked'));
   }
 
+  /**
+   * Change Form's validation enforcement based on service status (active/inactive)
+   */
   toggleServiceState(serviceActive) {
     this.toggleSubmitBtnLabel(serviceActive, this.canTestService);
     if (serviceActive) {
@@ -54,7 +57,7 @@ export default class IntegrationSettingsForm {
   }
 
   /**
-   * Toggle Submit button label based on Integration status
+   * Toggle Submit button label based on Integration status and ability to test service
    */
   toggleSubmitBtnLabel(serviceActive, canTestService) {
     this.$submitBtnLabel.text(
@@ -76,7 +79,9 @@ export default class IntegrationSettingsForm {
       this.$submitBtnLoader.removeClass('hidden');
     } else {
       this.$submitBtn.enable();
-      this.$submitBtnLoader.addClass('hidden');
+      if (!this.$submitBtnLoader.hasClass('hidden')) {
+        this.$submitBtnLoader.addClass('hidden');
+      }
     }
   }
 

From a678f42ebb5bcb7434954865b2b2ecd67463aa3c Mon Sep 17 00:00:00 2001
From: kushalpandya <kushal@gitlab.com>
Date: Tue, 30 May 2017 17:00:26 +0530
Subject: [PATCH 16/26] Tests for `integration_settings_form`

---
 spec/javascripts/fixtures/services.rb         |  32 +++
 .../integration_settings_form_spec.js         | 197 ++++++++++++++++++
 2 files changed, 229 insertions(+)
 create mode 100644 spec/javascripts/fixtures/services.rb
 create mode 100644 spec/javascripts/integrations/integration_settings_form_spec.js

diff --git a/spec/javascripts/fixtures/services.rb b/spec/javascripts/fixtures/services.rb
new file mode 100644
index 00000000000..6e74bc46036
--- /dev/null
+++ b/spec/javascripts/fixtures/services.rb
@@ -0,0 +1,32 @@
+require 'spec_helper'
+
+describe Projects::ServicesController, '(JavaScript fixtures)', type: :controller do
+  include JavaScriptFixturesHelpers
+
+  let(:admin)     { create(:admin) }
+  let(:namespace) { create(:namespace, name: 'frontend-fixtures' )}
+  let(:project)   { create(:project_empty_repo, namespace: namespace, path: 'services-project') }
+  let!(:service)  { create(:custom_issue_tracker_service, project: project, title: 'Custom Issue Tracker') }
+  
+
+  render_views
+
+  before(:all) do
+    clean_frontend_fixtures('services/')
+  end
+
+  before(:each) do
+    sign_in(admin)
+  end
+
+  it 'services/edit_service.html.raw' do |example|
+    get :edit,
+      namespace_id: namespace,
+      project_id: project,
+      id: service.to_param
+
+
+    expect(response).to be_success
+    store_frontend_fixture(response, example.description)
+  end
+end
diff --git a/spec/javascripts/integrations/integration_settings_form_spec.js b/spec/javascripts/integrations/integration_settings_form_spec.js
new file mode 100644
index 00000000000..a8ee7db6432
--- /dev/null
+++ b/spec/javascripts/integrations/integration_settings_form_spec.js
@@ -0,0 +1,197 @@
+import IntegrationSettingsForm from '~/integrations/integration_settings_form';
+
+describe('IntegrationSettingsForm', () => {
+  const FIXTURE = 'services/edit_service.html.raw';
+  preloadFixtures(FIXTURE);
+
+  beforeEach(() => {
+    loadFixtures(FIXTURE);
+  });
+
+  describe('contructor', () => {
+    let integrationSettingsForm;
+
+    beforeEach(() => {
+      integrationSettingsForm = new IntegrationSettingsForm('.js-integration-settings-form');
+      spyOn(integrationSettingsForm, 'init');
+    });
+
+    it('should initialize form element refs on class object', () => {
+      // Form Reference
+      expect(integrationSettingsForm.$form).toBeDefined();
+      expect(integrationSettingsForm.$form.prop('nodeName')).toEqual('FORM');
+
+      // Form Child Elements
+      expect(integrationSettingsForm.$serviceToggle).toBeDefined();
+      expect(integrationSettingsForm.$submitBtn).toBeDefined();
+      expect(integrationSettingsForm.$submitBtnLoader).toBeDefined();
+      expect(integrationSettingsForm.$submitBtnLabel).toBeDefined();
+    });
+
+    it('should initialize form metadata on class object', () => {
+      expect(integrationSettingsForm.endPoint).toBeDefined();
+      expect(integrationSettingsForm.canTestService).toBeDefined();
+    });
+  });
+
+  describe('toggleServiceState', () => {
+    let integrationSettingsForm;
+
+    beforeEach(() => {
+      integrationSettingsForm = new IntegrationSettingsForm('.js-integration-settings-form');
+    });
+
+    it('should remove `novalidate` attribute to form when called with `true`', () => {
+      integrationSettingsForm.toggleServiceState(true);
+
+      expect(integrationSettingsForm.$form.attr('novalidate')).not.toBeDefined();
+    });
+
+    it('should set `novalidate` attribute to form when called with `false`', () => {
+      integrationSettingsForm.toggleServiceState(false);
+
+      expect(integrationSettingsForm.$form.attr('novalidate')).toBeDefined();
+    });
+  });
+
+  describe('toggleSubmitBtnLabel', () => {
+    let integrationSettingsForm;
+
+    beforeEach(() => {
+      integrationSettingsForm = new IntegrationSettingsForm('.js-integration-settings-form');
+    });
+
+    it('should set Save button label to "Test settings and save changes" when serviceActive & canTestService are `true`', () => {
+      integrationSettingsForm.toggleSubmitBtnLabel(true, true);
+
+      expect(integrationSettingsForm.$submitBtnLabel.text()).toEqual('Test settings and save changes');
+    });
+
+    it('should set Save button label to "Save changes" when either serviceActive or canTestService (or both) is `false`', () => {
+      integrationSettingsForm.toggleSubmitBtnLabel(false, false);
+
+      expect(integrationSettingsForm.$submitBtnLabel.text()).toEqual('Save changes');
+
+      integrationSettingsForm.toggleSubmitBtnLabel(false, true);
+
+      expect(integrationSettingsForm.$submitBtnLabel.text()).toEqual('Save changes');
+
+      integrationSettingsForm.toggleSubmitBtnLabel(true, false);
+
+      expect(integrationSettingsForm.$submitBtnLabel.text()).toEqual('Save changes');
+    });
+  });
+
+  describe('toggleSubmitBtnState', () => {
+    let integrationSettingsForm;
+
+    beforeEach(() => {
+      integrationSettingsForm = new IntegrationSettingsForm('.js-integration-settings-form');
+    });
+
+    it('should disable Save button and show loader animation when called with `true`', () => {
+      integrationSettingsForm.toggleSubmitBtnState(true);
+
+      expect(integrationSettingsForm.$submitBtn.is(':disabled')).toBeTruthy();
+      expect(integrationSettingsForm.$submitBtnLoader.hasClass('hidden')).toBeFalsy();
+    });
+
+    it('should enable Save button and hide loader animation when called with `false`', () => {
+      integrationSettingsForm.toggleSubmitBtnState(false);
+
+      expect(integrationSettingsForm.$submitBtn.is(':disabled')).toBeFalsy();
+      expect(integrationSettingsForm.$submitBtnLoader.hasClass('hidden')).toBeTruthy();
+    });
+  });
+
+  describe('testSettings', () => {
+    let integrationSettingsForm;
+    let formData;
+
+    beforeEach(() => {
+      integrationSettingsForm = new IntegrationSettingsForm('.js-integration-settings-form');
+      formData = integrationSettingsForm.$form.serialize();
+    });
+
+    it('should make an ajax request with provided `formData`', () => {
+      const deferred = $.Deferred();
+      spyOn($, 'ajax').and.returnValue(deferred.promise());
+
+      integrationSettingsForm.testSettings(formData);
+
+      expect($.ajax).toHaveBeenCalledWith({
+        type: 'PUT',
+        url: `${integrationSettingsForm.endPoint}/test`,
+        data: formData,
+      });
+    });
+
+    it('should show error Flash with `Save anyway` action if ajax request responds with error in test', () => {
+      const errorMessage = 'Test failed.';
+      const deferred = $.Deferred();
+      spyOn($, 'ajax').and.returnValue(deferred.promise());
+
+      integrationSettingsForm.testSettings(formData);
+
+      deferred.resolve({ error: true, message: errorMessage });
+
+      const $flashContainer = $('.flash-container');
+      expect($flashContainer.find('.flash-text').text()).toEqual(errorMessage);
+      expect($flashContainer.find('.flash-action')).toBeDefined();
+      expect($flashContainer.find('.flash-action').text()).toEqual('Save anyway');
+    });
+
+    it('should submit form if ajax request responds without any error in test', () => {
+      const deferred = $.Deferred();
+      spyOn($, 'ajax').and.returnValue(deferred.promise());
+
+      integrationSettingsForm.testSettings(formData);
+
+      spyOn(integrationSettingsForm.$form, 'submit');
+      deferred.resolve({ error: false });
+
+      expect(integrationSettingsForm.$form.submit).toHaveBeenCalled();
+    });
+
+    it('should submit form when clicked on `Save anyway` action of error Flash', () => {
+      const errorMessage = 'Test failed.';
+      const deferred = $.Deferred();
+      spyOn($, 'ajax').and.returnValue(deferred.promise());
+
+      integrationSettingsForm.testSettings(formData);
+
+      deferred.resolve({ error: true, message: errorMessage });
+
+      const $flashAction = $('.flash-container .flash-action');
+      expect($flashAction).toBeDefined();
+
+      spyOn(integrationSettingsForm.$form, 'submit');
+      $flashAction.trigger('click');
+      expect(integrationSettingsForm.$form.submit).toHaveBeenCalled();
+    });
+
+    it('should show error Flash if ajax request failed', () => {
+      const errorMessage = 'Something went wrong on our end.';
+      const deferred = $.Deferred();
+      spyOn($, 'ajax').and.returnValue(deferred.promise());
+
+      integrationSettingsForm.testSettings(formData);
+
+      deferred.reject();
+
+      expect($('.flash-container .flash-text').text()).toEqual(errorMessage);
+    });
+
+    it('should always call `toggleSubmitBtnState` with `false` once request is completed', () => {
+      const deferred = $.Deferred();
+      spyOn($, 'ajax').and.returnValue(deferred.promise());
+
+      integrationSettingsForm.testSettings(formData);
+
+      spyOn(integrationSettingsForm, 'toggleSubmitBtnState');
+      deferred.reject();
+
+      expect(integrationSettingsForm.toggleSubmitBtnState).toHaveBeenCalledWith(false);
+    });
+  });
+});

From 84c68bb1402528944aa433b7a120d392fd93845b Mon Sep 17 00:00:00 2001
From: Jarka Kadlecova <jarka@gitlab.com>
Date: Tue, 30 May 2017 14:53:01 +0200
Subject: [PATCH 17/26] Address MR comments

---
 app/models/project_services/deployment_service.rb    |  4 ++++
 app/models/project_services/jira_service.rb          |  2 +-
 app/models/project_services/mock_ci_service.rb       |  4 ++++
 .../project_services/mock_monitoring_service.rb      |  4 ++++
 .../controllers/projects/services_controller_spec.rb |  3 ---
 spec/features/projects/services/jira_service_spec.rb | 12 ++++++++++++
 6 files changed, 25 insertions(+), 4 deletions(-)

diff --git a/app/models/project_services/deployment_service.rb b/app/models/project_services/deployment_service.rb
index 91a55514a9a..5b8320158fc 100644
--- a/app/models/project_services/deployment_service.rb
+++ b/app/models/project_services/deployment_service.rb
@@ -30,4 +30,8 @@ class DeploymentService < Service
   def terminals(environment)
     raise NotImplementedError
   end
+
+  def can_test?
+    false
+  end
 end
diff --git a/app/models/project_services/jira_service.rb b/app/models/project_services/jira_service.rb
index 489208a3fd6..2450fb43212 100644
--- a/app/models/project_services/jira_service.rb
+++ b/app/models/project_services/jira_service.rb
@@ -91,7 +91,7 @@ class JiraService < IssueTrackerService
       { 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 }
+      { type: 'text', name: 'jira_issue_transition_id', placeholder: '' }
     ]
   end
 
diff --git a/app/models/project_services/mock_ci_service.rb b/app/models/project_services/mock_ci_service.rb
index e5109dbbc93..72ddf9a4be3 100644
--- a/app/models/project_services/mock_ci_service.rb
+++ b/app/models/project_services/mock_ci_service.rb
@@ -80,4 +80,8 @@ class MockCiService < CiService
       :error
     end
   end
+
+  def can_test?
+    false
+  end
 end
diff --git a/app/models/project_services/mock_monitoring_service.rb b/app/models/project_services/mock_monitoring_service.rb
index dd04e04e198..ed0318c6b27 100644
--- a/app/models/project_services/mock_monitoring_service.rb
+++ b/app/models/project_services/mock_monitoring_service.rb
@@ -14,4 +14,8 @@ class MockMonitoringService < MonitoringService
   def metrics(environment)
     JSON.parse(File.read(Rails.root + 'spec/fixtures/metrics.json'))
   end
+
+  def can_test?
+    false
+  end
 end
diff --git a/spec/controllers/projects/services_controller_spec.rb b/spec/controllers/projects/services_controller_spec.rb
index 4fe1e848965..23b463c0082 100644
--- a/spec/controllers/projects/services_controller_spec.rb
+++ b/spec/controllers/projects/services_controller_spec.rb
@@ -58,9 +58,6 @@ describe Projects::ServicesController do
 
         expect(response.status).to eq(200)
       end
-
-      def built_service
-      end
     end
 
     context 'failure' do
diff --git a/spec/features/projects/services/jira_service_spec.rb b/spec/features/projects/services/jira_service_spec.rb
index 3d951d6b3e3..9a092f4ed93 100644
--- a/spec/features/projects/services/jira_service_spec.rb
+++ b/spec/features/projects/services/jira_service_spec.rb
@@ -47,6 +47,18 @@ feature 'Setup Jira service', :feature, :js do
         WebMock.stub_request(:get, project_url).to_return(status: 401)
       end
 
+      it 'shows errors when some required fields are not filled in' do
+        click_link('JIRA')
+
+        check 'Active'
+        fill_in 'service_password', with: 'password'
+        click_button('Test settings and save changes')
+
+        page.within('.service-settings') do
+          expect(page).to have_content('This field is required.')
+        end
+      end
+
       it 'activates the JIRA service' do
         click_link('JIRA')
         fill_form

From 00cc34cc73572c3b700976166e483d79a2ccdaed Mon Sep 17 00:00:00 2001
From: kushalpandya <kushal@gitlab.com>
Date: Wed, 31 May 2017 12:46:50 +0530
Subject: [PATCH 18/26] Add doc comment

---
 app/assets/javascripts/flash.js | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/app/assets/javascripts/flash.js b/app/assets/javascripts/flash.js
index 4d00858f7e8..de909946271 100644
--- a/app/assets/javascripts/flash.js
+++ b/app/assets/javascripts/flash.js
@@ -7,6 +7,19 @@ window.Flash = (function() {
     return $(this).fadeOut();
   };
 
+  /**
+   * Flash banner supports different types of Flash configurations
+   * along with ability to provide actionConfig which can be used to show
+   * additional action or link on banner next to message
+   *
+   * @param {String} message Flash message
+   * @param {String} type Type of Flash, it can be `notice` or `alert` (default)
+   * @param {Object} parent Reference to Parent element under which Flash needs to appear
+   * @param {Object} actionConfig Map of config to show action on banner
+   *    @param {String} href URL to which action link should point (default '#')
+   *    @param {String} title Title of action
+   *    @param {Function} clickHandler Method to call when action is clicked on
+   */
   function Flash(message, type, parent, actionConfig) {
     var flash, textDiv, actionLink;
     if (type == null) {
@@ -33,8 +46,8 @@ window.Flash = (function() {
 
     if (actionConfig) {
       actionLink = $('<a/>', {
-        "class": "flash-action",
-        "href": "#",
+        class: 'flash-action',
+        href: actionConfig.href || '#',
         text: actionConfig.title
       });
 

From 7357d35494103f6f728736e9bf1264a979e02a86 Mon Sep 17 00:00:00 2001
From: kushalpandya <kushal@gitlab.com>
Date: Wed, 31 May 2017 12:47:07 +0530
Subject: [PATCH 19/26] Update as per review feedback

---
 app/assets/javascripts/integrations/index.js  |  3 +-
 .../integrations/integration_settings_form.js | 56 ++++++++++---------
 2 files changed, 32 insertions(+), 27 deletions(-)

diff --git a/app/assets/javascripts/integrations/index.js b/app/assets/javascripts/integrations/index.js
index 69db098d981..10fe6bac0e8 100644
--- a/app/assets/javascripts/integrations/index.js
+++ b/app/assets/javascripts/integrations/index.js
@@ -2,5 +2,6 @@
 import IntegrationSettingsForm from './integration_settings_form';
 
 $(() => {
-  new IntegrationSettingsForm('.js-integration-settings-form');
+  const integrationSettingsForm = new IntegrationSettingsForm('.js-integration-settings-form');
+  integrationSettingsForm.init();
 });
diff --git a/app/assets/javascripts/integrations/integration_settings_form.js b/app/assets/javascripts/integrations/integration_settings_form.js
index 5745475123d..ddd3a6aab99 100644
--- a/app/assets/javascripts/integrations/integration_settings_form.js
+++ b/app/assets/javascripts/integrations/integration_settings_form.js
@@ -5,20 +5,14 @@ export default class IntegrationSettingsForm {
     this.$form = $(formSelector);
 
     // Form Metadata
-    this.endPoint = this.$form.attr('action');
     this.canTestService = this.$form.data('can-test');
+    this.testEndPoint = this.$form.data('test-url');
 
     // Form Child Elements
     this.$serviceToggle = this.$form.find('#service_active');
     this.$submitBtn = this.$form.find('button[type="submit"]');
     this.$submitBtnLoader = this.$submitBtn.find('.js-btn-spinner');
     this.$submitBtnLabel = this.$submitBtn.find('.js-btn-label');
-
-    // Class Member methods
-    this.handleServiceToggle = this.handleServiceToggle.bind(this);
-    this.handleSettingsSave = this.handleSettingsSave.bind(this);
-
-    this.init();
   }
 
   init() {
@@ -26,17 +20,26 @@ export default class IntegrationSettingsForm {
     this.toggleServiceState(this.$serviceToggle.is(':checked'));
 
     // Bind Event Listeners
-    this.$serviceToggle.on('change', this.handleServiceToggle);
-    this.$submitBtn.on('click', this.handleSettingsSave);
+    this.$serviceToggle.on('change', e => this.handleServiceToggle(e));
+    this.$submitBtn.on('click', e => this.handleSettingsSave(e));
   }
 
   handleSettingsSave(e) {
-    if (this.$serviceToggle.is(':checked')) {
-      if (this.$form.get(0).checkValidity() &&
-          this.canTestService) {
-        e.preventDefault();
-        this.testSettings(this.$form.serialize());
-      }
+    // Check if Service is marked active, as if not marked active,
+    // We can skip testing it and directly go ahead to allow form to
+    // be submitted
+    if (!this.$serviceToggle.is(':checked')) {
+      return;
+    }
+
+    // Service was marked active so now we check;
+    // 1) If form contents are valid
+    // 2) If this service can be tested
+    // If both conditions are true, we override form submission
+    // and test the service using provided configuration.
+    if (this.$form.get(0).checkValidity() && this.canTestService) {
+      e.preventDefault();
+      this.testSettings(this.$form.serialize());
     }
   }
 
@@ -48,7 +51,7 @@ export default class IntegrationSettingsForm {
    * Change Form's validation enforcement based on service status (active/inactive)
    */
   toggleServiceState(serviceActive) {
-    this.toggleSubmitBtnLabel(serviceActive, this.canTestService);
+    this.toggleSubmitBtnLabel(serviceActive);
     if (serviceActive) {
       this.$form.removeAttr('novalidate');
     } else if (!this.$form.attr('novalidate')) {
@@ -59,11 +62,14 @@ export default class IntegrationSettingsForm {
   /**
    * Toggle Submit button label based on Integration status and ability to test service
    */
-  toggleSubmitBtnLabel(serviceActive, canTestService) {
-    this.$submitBtnLabel.text(
-      serviceActive && canTestService ?
-        'Test settings and save changes' :
-        'Save changes');
+  toggleSubmitBtnLabel(serviceActive) {
+    let btnLabel = 'Save changes';
+
+    if (serviceActive && this.canTestService) {
+      btnLabel = 'Test settings and save changes';
+    }
+
+    this.$submitBtnLabel.text(btnLabel);
   }
 
   /**
@@ -79,9 +85,7 @@ export default class IntegrationSettingsForm {
       this.$submitBtnLoader.removeClass('hidden');
     } else {
       this.$submitBtn.enable();
-      if (!this.$submitBtnLoader.hasClass('hidden')) {
-        this.$submitBtnLoader.addClass('hidden');
-      }
+      this.$submitBtnLoader.addClass('hidden');
     }
   }
 
@@ -93,12 +97,12 @@ export default class IntegrationSettingsForm {
     this.toggleSubmitBtnState(true);
     $.ajax({
       type: 'PUT',
-      url: `${this.endPoint}/test`,
+      url: this.testEndPoint,
       data: formData,
     })
     .done((res) => {
       if (res.error) {
-        new Flash(`${res.message}`, null, null, {
+        new Flash(res.message, null, null, {
           title: 'Save anyway',
           clickHandler: (e) => {
             e.preventDefault();

From 325c55fb24e00883ee11738bd0f3695e45ace468 Mon Sep 17 00:00:00 2001
From: kushalpandya <kushal@gitlab.com>
Date: Wed, 31 May 2017 12:47:18 +0530
Subject: [PATCH 20/26] Remove extra new line

---
 spec/javascripts/fixtures/services.rb | 1 -
 1 file changed, 1 deletion(-)

diff --git a/spec/javascripts/fixtures/services.rb b/spec/javascripts/fixtures/services.rb
index 6e74bc46036..554451d1bbf 100644
--- a/spec/javascripts/fixtures/services.rb
+++ b/spec/javascripts/fixtures/services.rb
@@ -25,7 +25,6 @@ describe Projects::ServicesController, '(JavaScript fixtures)', type: :controlle
       project_id: project,
       id: service.to_param
 
-
     expect(response).to be_success
     store_frontend_fixture(response, example.description)
   end

From 576e30ee60f02c8af4cc50e3cc44b9f519e89170 Mon Sep 17 00:00:00 2001
From: kushalpandya <kushal@gitlab.com>
Date: Wed, 31 May 2017 12:47:41 +0530
Subject: [PATCH 21/26] Provide `test-url` via backend

---
 app/views/projects/services/_form.html.haml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/views/projects/services/_form.html.haml b/app/views/projects/services/_form.html.haml
index 8eeed5cda09..0f91f08ab10 100644
--- a/app/views/projects/services/_form.html.haml
+++ b/app/views/projects/services/_form.html.haml
@@ -9,7 +9,7 @@
 
     %p= @service.description
   .col-lg-9
-    = form_for(@service, as: :service, url: namespace_project_service_path(@project.namespace, @project, @service.to_param), method: :put, html: { class: 'gl-show-field-errors form-horizontal js-integration-settings-form', data: { "can-test" => "#{@service.can_test?}" } }) do |form|
+    = form_for(@service, as: :service, url: namespace_project_service_path(@project.namespace, @project, @service.to_param), method: :put, html: { class: 'gl-show-field-errors form-horizontal js-integration-settings-form', data: { "can-test" => "#{@service.can_test?}", "test-url" => "#{test_namespace_project_service_path(@project.namespace, @project, @service)}" } }) do |form|
       = render 'shared/service_settings', form: form, subject: @service
       .footer-block.row-content-block
         %button.btn.btn-save{ type: 'submit' }

From dc3de71fb79e1b9bb72eb9cf283fb94d7a089c90 Mon Sep 17 00:00:00 2001
From: kushalpandya <kushal@gitlab.com>
Date: Wed, 31 May 2017 12:48:02 +0530
Subject: [PATCH 22/26] Update tests based on source changes

---
 .../integration_settings_form_spec.js           | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/spec/javascripts/integrations/integration_settings_form_spec.js b/spec/javascripts/integrations/integration_settings_form_spec.js
index a8ee7db6432..e2a8259dea9 100644
--- a/spec/javascripts/integrations/integration_settings_form_spec.js
+++ b/spec/javascripts/integrations/integration_settings_form_spec.js
@@ -29,7 +29,7 @@ describe('IntegrationSettingsForm', () => {
     });
 
     it('should initialize form metadata on class object', () => {
-      expect(integrationSettingsForm.endPoint).toBeDefined();
+      expect(integrationSettingsForm.testEndPoint).toBeDefined();
       expect(integrationSettingsForm.canTestService).toBeDefined();
     });
   });
@@ -62,22 +62,25 @@ describe('IntegrationSettingsForm', () => {
     });
 
     it('should set Save button label to "Test settings and save changes" when serviceActive & canTestService are `true`', () => {
-      integrationSettingsForm.toggleSubmitBtnLabel(true, true);
+      integrationSettingsForm.canTestService = true;
 
+      integrationSettingsForm.toggleSubmitBtnLabel(true);
       expect(integrationSettingsForm.$submitBtnLabel.text()).toEqual('Test settings and save changes');
     });
 
     it('should set Save button label to "Save changes" when either serviceActive or canTestService (or both) is `false`', () => {
-      integrationSettingsForm.toggleSubmitBtnLabel(false, false);
+      integrationSettingsForm.canTestService = false;
 
+      integrationSettingsForm.toggleSubmitBtnLabel(false);
       expect(integrationSettingsForm.$submitBtnLabel.text()).toEqual('Save changes');
 
-      integrationSettingsForm.toggleSubmitBtnLabel(false, true);
-
+      integrationSettingsForm.toggleSubmitBtnLabel(true);
       expect(integrationSettingsForm.$submitBtnLabel.text()).toEqual('Save changes');
 
-      integrationSettingsForm.toggleSubmitBtnLabel(true, false);
 
+      integrationSettingsForm.canTestService = true;
+
+      integrationSettingsForm.toggleSubmitBtnLabel(false);
       expect(integrationSettingsForm.$submitBtnLabel.text()).toEqual('Save changes');
     });
   });
@@ -121,7 +124,7 @@ describe('IntegrationSettingsForm', () => {
 
       expect($.ajax).toHaveBeenCalledWith({
         type: 'PUT',
-        url: `${integrationSettingsForm.endPoint}/test`,
+        url: integrationSettingsForm.testEndPoint,
         data: formData,
       });
     });

From 73af7178a5d5002924603b5688adfaac2d5736eb Mon Sep 17 00:00:00 2001
From: kushalpandya <kushal@gitlab.com>
Date: Wed, 31 May 2017 13:50:29 +0530
Subject: [PATCH 23/26] Update as per review

---
 app/views/projects/services/_form.html.haml                     | 2 +-
 spec/javascripts/integrations/integration_settings_form_spec.js | 1 -
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/app/views/projects/services/_form.html.haml b/app/views/projects/services/_form.html.haml
index 0f91f08ab10..a51eb4c587e 100644
--- a/app/views/projects/services/_form.html.haml
+++ b/app/views/projects/services/_form.html.haml
@@ -13,7 +13,7 @@
       = render 'shared/service_settings', form: form, subject: @service
       .footer-block.row-content-block
         %button.btn.btn-save{ type: 'submit' }
-          %i.fa.fa-spinner.fa-spin.hidden.js-btn-spinner{ "aria-hidden" => true }
+          = icon('spinner spin', class: 'hidden js-btn-spinner')
           %span.js-btn-label
             Save changes
         &nbsp;
diff --git a/spec/javascripts/integrations/integration_settings_form_spec.js b/spec/javascripts/integrations/integration_settings_form_spec.js
index e2a8259dea9..45909d4e70e 100644
--- a/spec/javascripts/integrations/integration_settings_form_spec.js
+++ b/spec/javascripts/integrations/integration_settings_form_spec.js
@@ -77,7 +77,6 @@ describe('IntegrationSettingsForm', () => {
       integrationSettingsForm.toggleSubmitBtnLabel(true);
       expect(integrationSettingsForm.$submitBtnLabel.text()).toEqual('Save changes');
 
-
       integrationSettingsForm.canTestService = true;
 
       integrationSettingsForm.toggleSubmitBtnLabel(false);

From 112d540df4a37aeca198dc39e715e04e3655c1f7 Mon Sep 17 00:00:00 2001
From: kushalpandya <kushal@gitlab.com>
Date: Wed, 31 May 2017 20:25:57 +0530
Subject: [PATCH 24/26] Action link role sets to `button` if no HREF is given

---
 app/assets/javascripts/flash.js | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/app/assets/javascripts/flash.js b/app/assets/javascripts/flash.js
index de909946271..ccff8f0ace7 100644
--- a/app/assets/javascripts/flash.js
+++ b/app/assets/javascripts/flash.js
@@ -45,11 +45,17 @@ window.Flash = (function() {
     textDiv.appendTo(flash);
 
     if (actionConfig) {
-      actionLink = $('<a/>', {
+      const actionLinkConfig = {
         class: 'flash-action',
         href: actionConfig.href || '#',
         text: actionConfig.title
-      });
+      };
+
+      if (!actionConfig.href) {
+        actionLinkConfig.role = 'button';
+      }
+
+      actionLink = $('<a/>', actionLinkConfig);
 
       actionLink.appendTo(flash);
       this.flashContainer.on('click', '.flash-action', actionConfig.clickHandler);

From b487a32cc0fc14e4c0977cb6ff298131818fea16 Mon Sep 17 00:00:00 2001
From: Jarka Kadlecova <jarka@gitlab.com>
Date: Fri, 2 Jun 2017 13:44:04 +0200
Subject: [PATCH 25/26] Address review comments

---
 app/controllers/projects/services_controller.rb      | 4 ++--
 app/views/projects/services/_form.html.haml          | 4 ++--
 changelogs/unreleased/31511-jira-settings.yml        | 2 +-
 spec/features/projects/services/jira_service_spec.rb | 2 +-
 4 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/app/controllers/projects/services_controller.rb b/app/controllers/projects/services_controller.rb
index 264665942f8..704f8cc8a79 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 :service, only: [:edit, :update, :test]
-  before_action :build_service, only: [:update, :test]
+  before_action :update_service, only: [:update, :test]
 
   respond_to :html
 
@@ -50,7 +50,7 @@ class Projects::ServicesController < Projects::ApplicationController
     end
   end
 
-  def build_service
+  def update_service
     @service.assign_attributes(service_params[:service])
   end
 
diff --git a/app/views/projects/services/_form.html.haml b/app/views/projects/services/_form.html.haml
index a51eb4c587e..9167789a69d 100644
--- a/app/views/projects/services/_form.html.haml
+++ b/app/views/projects/services/_form.html.haml
@@ -9,7 +9,7 @@
 
     %p= @service.description
   .col-lg-9
-    = form_for(@service, as: :service, url: namespace_project_service_path(@project.namespace, @project, @service.to_param), method: :put, html: { class: 'gl-show-field-errors form-horizontal js-integration-settings-form', data: { "can-test" => "#{@service.can_test?}", "test-url" => "#{test_namespace_project_service_path(@project.namespace, @project, @service)}" } }) do |form|
+    = form_for(@service, as: :service, url: namespace_project_service_path(@project.namespace, @project, @service.to_param), method: :put, html: { class: 'gl-show-field-errors form-horizontal js-integration-settings-form', data: { 'can-test' => @service.can_test?, 'test-url' => test_namespace_project_service_path } }) do |form|
       = render 'shared/service_settings', form: form, subject: @service
       .footer-block.row-content-block
         %button.btn.btn-save{ type: 'submit' }
@@ -22,4 +22,4 @@
             - disabled_class = 'disabled'
             - disabled_title = @service.disabled_title
 
-        = link_to "Cancel", namespace_project_settings_integrations_path(@project.namespace, @project), class: "btn btn-cancel"
+        = link_to 'Cancel', namespace_project_settings_integrations_path(@project.namespace, @project), class: 'btn btn-cancel'
diff --git a/changelogs/unreleased/31511-jira-settings.yml b/changelogs/unreleased/31511-jira-settings.yml
index 3592e92b49e..4f9ddb13ef6 100644
--- a/changelogs/unreleased/31511-jira-settings.yml
+++ b/changelogs/unreleased/31511-jira-settings.yml
@@ -1,4 +1,4 @@
 ---
-title: Simplify test&save actions when setting a service integration
+title: Simplify testing and saving service integrations
 merge_request: 11599
 author:
diff --git a/spec/features/projects/services/jira_service_spec.rb b/spec/features/projects/services/jira_service_spec.rb
index 9a092f4ed93..191a7103871 100644
--- a/spec/features/projects/services/jira_service_spec.rb
+++ b/spec/features/projects/services/jira_service_spec.rb
@@ -76,7 +76,7 @@ feature 'Setup Jira service', :feature, :js do
     end
   end
 
-  describe 'user sets Jira Service but keeps it non active' do
+  describe 'user sets Jira Service but keeps it disabled' do
     context 'when Jira connection test succeeds' do
       it 'activates the JIRA service' do
         click_link('JIRA')

From 6838c16c49185fb72aa58a9c6591da4ec0fc7acb Mon Sep 17 00:00:00 2001
From: kushalpandya <kushal@gitlab.com>
Date: Fri, 2 Jun 2017 18:05:47 +0530
Subject: [PATCH 26/26] Minor refactor based on review feedback

---
 app/views/shared/_field.html.haml                    | 2 +-
 spec/features/projects/services/jira_service_spec.rb | 3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/app/views/shared/_field.html.haml b/app/views/shared/_field.html.haml
index 3bda8cd5e9c..795447a9ca6 100644
--- a/app/views/shared/_field.html.haml
+++ b/app/views/shared/_field.html.haml
@@ -23,6 +23,6 @@
     - elsif type == 'select'
       = form.select name, options_for_select(choices, value ? value : default_choice), {}, { class: "form-control" }
     - elsif type == 'password'
-      = form.password_field name, autocomplete: "new-password", class: "form-control", required: value.present? ? nil : :required
+      = form.password_field name, autocomplete: "new-password", class: "form-control", required: value.blank? && :required
     - if help
       %span.help-block= help
diff --git a/spec/features/projects/services/jira_service_spec.rb b/spec/features/projects/services/jira_service_spec.rb
index 191a7103871..c96d87e5708 100644
--- a/spec/features/projects/services/jira_service_spec.rb
+++ b/spec/features/projects/services/jira_service_spec.rb
@@ -65,7 +65,8 @@ feature 'Setup Jira service', :feature, :js do
         click_button('Test settings and save changes')
         wait_for_requests
 
-        expect(page).to have_content('Test failed.Save anyway')
+        expect(find('.flash-container-page')).to have_content 'Test failed.'
+        expect(find('.flash-container-page')).to have_content 'Save anyway'
 
         find('.flash-alert .flash-action').trigger('click')
         wait_for_requests