Merge branch 'fix-slack-channel-in-admin-settings' into 'master'

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
Closes #1501

See merge request !574

CHANGELOG

@@ -17,7 +17,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)

app/controllers/admin/services_controller.rb

@@ -40,15 +40,6 @@ class Admin::ServicesController < Admin::ApplicationController
 
   def application_services_params
     params.permit(:id,
-      service: [
+      service: Projects::ServicesController::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,
-        :send_from_committer_email, :disable_diffs,
-        :push_events, :tag_push_events, :note_events, :issues_events,
-        :merge_requests_events
-    ])
   end
 end

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(
+    params.require(:service).permit(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
-    )
   end
 end

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

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