From 2f045997516a11622a7c7229ac08ac0d288813df Mon Sep 17 00:00:00 2001
From: Stan Hu <stanhu@gmail.com>
Date: Fri, 24 Apr 2015 08:33:24 -0700
Subject: [PATCH] Fix bug where Slack service channel was not saved in admin
 template settings.

Consolidate allowed parameters in one place to avoid these kinds of bugs in the future.

Closes https://github.com/gitlabhq/gitlabhq/issues/9181
---
 CHANGELOG                                     |  2 +-
 app/controllers/admin/services_controller.rb  | 11 +----------
 .../projects/services_controller.rb           | 19 +++++++++----------
 features/admin/settings.feature               |  3 +++
 features/steps/admin/settings.rb              | 13 ++++++++++++-
 5 files changed, 26 insertions(+), 22 deletions(-)

diff --git a/CHANGELOG b/CHANGELOG
index 0d6b0f2d942..87189d97103 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -16,7 +16,7 @@ v 7.11.0 (unreleased)
   - Include commit comments in MR from a forked project.
   -
   -
-  -
+  - Fix bug where Slack service channel was not saved in admin template settings. (Stan Hu)
   - Move snippets UI to fluid layout
   - Improve UI for sidebar. Increase separation between navigation and content
   - Improve new project command options (Ben Bodenmiller)
diff --git a/app/controllers/admin/services_controller.rb b/app/controllers/admin/services_controller.rb
index c1fdcd7fab6..a62170662e1 100644
--- a/app/controllers/admin/services_controller.rb
+++ b/app/controllers/admin/services_controller.rb
@@ -40,15 +40,6 @@ class Admin::ServicesController < Admin::ApplicationController
 
   def application_services_params
     params.permit(:id,
-      service: [
-        :title, :token, :type, :active, :api_key, :subdomain,
-        :room, :recipients, :project_url, :webhook,
-        :user_key, :device, :priority, :sound, :bamboo_url, :username, :password,
-        :build_key, :server, :teamcity_url, :build_type,
-        :description, :issues_url, :new_issue_url, :restrict_to_branch,
-        :send_from_committer_email, :disable_diffs,
-        :push_events, :tag_push_events, :note_events, :issues_events,
-        :merge_requests_events
-    ])
+      service: Projects::ServicesController::ALLOWED_PARAMS)
   end
 end
diff --git a/app/controllers/projects/services_controller.rb b/app/controllers/projects/services_controller.rb
index ae8146dca59..73031851734 100644
--- a/app/controllers/projects/services_controller.rb
+++ b/app/controllers/projects/services_controller.rb
@@ -1,4 +1,12 @@
 class Projects::ServicesController < Projects::ApplicationController
+  ALLOWED_PARAMS = [:title, :token, :type, :active, :api_key, :subdomain,
+                    :room, :recipients, :project_url, :webhook,
+                    :user_key, :device, :priority, :sound, :bamboo_url, :username, :password,
+                    :build_key, :server, :teamcity_url, :build_type,
+                    :description, :issues_url, :new_issue_url, :restrict_to_branch, :channel,
+                    :colorize_messages, :channels,
+                    :push_events, :issues_events, :merge_requests_events, :tag_push_events,
+                    :note_events, :send_from_committer_email, :disable_diffs, :external_wiki_url]
   # Authorize
   before_action :authorize_admin_project!
   before_action :service, only: [:edit, :update, :test]
@@ -45,15 +53,6 @@ class Projects::ServicesController < Projects::ApplicationController
   end
 
   def service_params
-    params.require(:service).permit(
-      :title, :token, :type, :active, :api_key, :subdomain,
-      :room, :recipients, :project_url, :webhook,
-      :user_key, :device, :priority, :sound, :bamboo_url, :username, :password,
-      :build_key, :server, :teamcity_url, :build_type,
-      :description, :issues_url, :new_issue_url, :restrict_to_branch, :channel,
-      :colorize_messages, :channels,
-      :push_events, :issues_events, :merge_requests_events, :tag_push_events,
-      :note_events, :send_from_committer_email, :disable_diffs, :external_wiki_url
-    )
+    params.require(:service).permit(ALLOWED_PARAMS)
   end
 end
diff --git a/features/admin/settings.feature b/features/admin/settings.feature
index 52e47307b23..e38eea2cfed 100644
--- a/features/admin/settings.feature
+++ b/features/admin/settings.feature
@@ -11,6 +11,9 @@ Feature: Admin Settings
   Scenario: Change Slack Service Template settings
     When I click on "Service Templates"
     And I click on "Slack" service
+    And I fill out Slack settings
     Then I check all events and submit form
     And I should see service template settings saved
+    Then I click on "Slack" service
     And I should see all checkboxes checked
+    And I should see Slack settings saved
diff --git a/features/steps/admin/settings.rb b/features/steps/admin/settings.rb
index 87d4e969ff5..15ca0d80f1a 100644
--- a/features/steps/admin/settings.rb
+++ b/features/steps/admin/settings.rb
@@ -31,10 +31,15 @@ class Spinach::Features::AdminSettings < Spinach::FeatureSteps
     page.check('Comments')
     page.check('Issues events')
     page.check('Merge Request events')
-    fill_in 'Webhook', with: "http://localhost"
     click_on 'Save'
   end
 
+  step 'I fill out Slack settings' do
+    fill_in 'Webhook', with: 'http://localhost'
+    fill_in 'Username', with: 'test_user'
+    fill_in 'Channel', with: '#test_channel'
+  end
+
   step 'I should see service template settings saved' do
     page.should have_content 'Application settings saved successfully'
   end
@@ -44,4 +49,10 @@ class Spinach::Features::AdminSettings < Spinach::FeatureSteps
       checkbox.should be_checked
     end
   end
+
+  step 'I should see Slack settings saved' do
+    find_field('Webhook').value.should eq 'http://localhost'
+    find_field('Username').value.should eq 'test_user'
+    find_field('Channel').value.should eq '#test_channel'
+  end
 end