Refactor service settings view

app/controllers/admin/services_controller.rb

@@ -15,7 +15,7 @@ class Admin::ServicesController < Admin::ApplicationController
   end
 
   def update
-    if service.update_attributes(application_services_params[:service])
+    if service.update_attributes(service_params[:service])
       redirect_to admin_application_settings_services_path,
         notice: 'Application settings saved successfully'
     else

app/controllers/concerns/service_params.rb

@@ -18,18 +18,18 @@ module ServiceParams
   # Parameters to ignore if no value is specified
   FILTER_BLANK_PARAMS = [:password]
 
-  def application_services_params
+  def service_params
     dynamic_params = []
     dynamic_params.concat(@service.event_channel_names)
 
-    application_services_params = params.permit(:id, service: ALLOWED_PARAMS + dynamic_params)
+    service_params = params.permit(:id, service: ALLOWED_PARAMS + dynamic_params)
 
-    if application_services_params[:service].is_a?(Hash)
+    if service_params[:service].is_a?(Hash)
       FILTER_BLANK_PARAMS.each do |param|
-        application_services_params[:service].delete(param) if application_services_params[:service][param].blank?
+        service_params[:service].delete(param) if service_params[:service][param].blank?
       end
     end
 
-    application_services_params
+    service_params
   end
 end

app/controllers/projects/services_controller.rb

@@ -18,7 +18,7 @@ class Projects::ServicesController < Projects::ApplicationController
   end
 
   def update
-    if @service.update_attributes(service_params)
+    if @service.update_attributes(service_params[:service])
       redirect_to(
         edit_namespace_project_service_path(@project.namespace, @project,
                                             @service.to_param, notice:
@@ -49,17 +49,4 @@ class Projects::ServicesController < Projects::ApplicationController
   def service
     @service ||= @project.services.find { |service| service.to_param == params[:id] }
   end
-
-  def service_params
-    dynamic_params = []
-    dynamic_params.concat(@service.event_channel_names) if @service.is_a?(SlackService)
-
-    service_params = params.require(:service).permit(ALLOWED_PARAMS + dynamic_params)
-
-    FILTER_BLANK_PARAMS.each do |param|
-      service_params.delete(param) if service_params[param].blank?
-    end
-
-    service_params
-  end
 end

app/helpers/services_helper.rb

@@ -2,19 +2,19 @@ module ServicesHelper
   def service_event_description(event)
     case event
     when "push"
-      "Webhook will be triggered by a push to the repository"
+      "Event will be triggered by a push to the repository"
     when "tag_push"
-      "Webhook will be triggered when a new tag is pushed to the repository"
+      "Event will be triggered when a new tag is pushed to the repository"
     when "note"
-      "Webhook will be triggered when someone adds a comment"
+      "Event will be triggered when someone adds a comment"
     when "issue"
-      "Webhook will be triggered when an issue is created/updated/merged"
+      "Event will be triggered when an issue is created/updated/merged"
     when "merge_request"
-      "Webhook will be triggered when a merge request is created/updated/merged"
+      "Event will be triggered when a merge request is created/updated/merged"
     when "build"
-      "Webhook will be triggered when a build status changes"
+      "Event will be triggered when a build status changes"
     when "wiki_page"
-      "Webhook will be triggered when a wiki page is created/updated"
+      "Event will be triggered when a wiki page is created/updated"
     end
   end
 

app/models/project_services/slack_service.rb

@@ -95,6 +95,14 @@ class SlackService < Service
     supported_events.map { |event| event_channel_name(event) }
   end
 
+  def event_field(event)
+    fields.find { |field| field[:name] == event_channel_name(event) }
+  end
+
+  def global_fields
+    fields.reject { |field| field[:name].end_with?('channel') }
+  end
+
   private
 
   def get_channel_field(event)

app/models/service.rb

@@ -84,6 +84,14 @@ class Service < ActiveRecord::Base
     []
   end
 
+  def event_field(event)
+    nil
+  end
+
+  def global_fields
+    fields
+  end
+
   def supported_events
     %w(push tag_push issue merge_request wiki_page)
   end

app/views/admin/services/_form.html.haml

@@ -4,9 +4,6 @@
 %p #{@service.description} template
 
 = form_for :service, url: admin_application_settings_service_path, method: :put, html: { class: 'form-horizontal fieldset-form' } do |form|
-  - if @service.is_a?(SlackService)
-    = render 'projects/services/slack/service_settings', form: form
-  - else
   = render 'shared/service_settings', form: form
 
   .form-actions

app/views/projects/services/_form.html.haml

@@ -7,9 +7,6 @@
     %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|
-      - if @service.is_a?(SlackService)
-        = render 'projects/services/slack/service_settings', form: form
-      - else
       = render 'shared/service_settings', form: form
 
       = form.submit 'Save changes', class: 'btn btn-save'

app/views/projects/services/slack/_service_settings.html.haml

-= form_errors(@service)
-
-- if @service.help.present?
-  .well
-    = preserve do
-      = markdown @service.help
-
-.form-group
-  = form.label :active, "Active", class: "control-label"
-  .col-sm-10
-    = form.check_box :active
-
-.form-group
-  = form.label :url, "Trigger", class: 'control-label'
-
-  .col-sm-10
-    - @service.supported_events.each do |event|
-      %div
-        = form.check_box service_event_field_name(event), class: 'pull-left'
-        .prepend-left-20
-          = form.label service_event_field_name(event), class: 'list-label' do
-            %strong
-              = event.humanize
-
-      %p
-        - field = @service.fields.select{ |field| field[:name] == "#{event}_channel"}.first
-        = form.text_field field[:name], class: "form-control", placeholder: field[:placeholder]
-
-      %p.light
-        = service_event_description(event)
-
-- @service.fields.each do |field|
-  - if %w(webhook username notify_only_broken_builds).include?(field[:name])
-    = render 'shared/field', form: form, field: field

app/views/shared/_service_settings.html.haml

@@ -10,69 +10,28 @@
   .col-sm-10
     = form.check_box :active
 
-- if @service.supported_events.length > 1
-  .form-group
+.form-group
   = form.label :url, "Trigger", class: 'control-label'
+
   .col-sm-10
-      - if @service.supported_events.include?("push")
-        %div
-          = form.check_box :push_events, class: 'pull-left'
-          .prepend-left-20
-            = form.label :push_events, class: 'list-label' do
-              %strong Push events
-            %p.light
-              This url will be triggered by a push to the repository
-      - if @service.supported_events.include?("tag_push")
-        %div
-          = form.check_box :tag_push_events, class: 'pull-left'
-          .prepend-left-20
-            = form.label :tag_push_events, class: 'list-label' do
-              %strong Tag push events
-            %p.light
-              This url will be triggered when a new tag is pushed to the repository
-      - if @service.supported_events.include?("note")
-        %div
-          = form.check_box :note_events, class: 'pull-left'
-          .prepend-left-20
-            = form.label :note_events, class: 'list-label' do
-              %strong Comments
-            %p.light
-              This url will be triggered when someone adds a comment
-      - if @service.supported_events.include?("issue")
-        %div
-          = form.check_box :issues_events, class: 'pull-left'
-          .prepend-left-20
-            = form.label :issues_events, class: 'list-label' do
-              %strong Issues events
-            %p.light
-              This url will be triggered when an issue is created/updated/merged
-      - if @service.supported_events.include?("merge_request")
-        %div
-          = form.check_box :merge_requests_events, class: 'pull-left'
-          .prepend-left-20
-            = form.label :merge_requests_events, class: 'list-label' do
-              %strong Merge Request events
-            %p.light
-              This url will be triggered when a merge request is created/updated/merged
-      - if @service.supported_events.include?("build")
-        %div
-          = form.check_box :build_events, class: 'pull-left'
-          .prepend-left-20
-            = form.label :build_events, class: 'list-label' do
-              %strong Build events
-            %p.light
-              This url will be triggered when a build status changes
-      - if @service.supported_events.include?("wiki_page")
+    - @service.supported_events.each do |event|
       %div
-          = form.check_box :wiki_page_events, class: 'pull-left'
+        = form.check_box service_event_field_name(event), class: 'pull-left'
         .prepend-left-20
-            = form.label :wiki_page_events, class: 'list-label' do
-              %strong Wiki Page events
-            %p.light
-              This url will be triggered when a wiki page is created/updated
+          = form.label service_event_field_name(event), class: 'list-label' do
+            %strong
+              = event.humanize
 
+      - field = @service.event_field(event)
+
+      - if field
+        %p
+          = form.text_field field[:name], class: "form-control", placeholder: field[:placeholder]
+
+      %p.light
+        = service_event_description(event)
 
-- @service.fields.each do |field|
+- @service.global_fields.each do |field|
   - type = field[:type]
 
   - if type == 'fieldset'

doc/integration/slack.md

@@ -26,7 +26,7 @@ After Slack is ready we need to setup GitLab. Here are the steps to achieve this
 
 1.  Navigate to Settings -> Services -> Slack
 
-1. Pick the triggers you want to activate and respective channel(#general by default).
+1. Pick the triggers you want to activate and respective channel (`#general` by default).
 
 1.  Fill in your Slack details
     - Webhook: Paste the Webhook URL from the step above

doc/project_services/slack.md

@@ -10,8 +10,8 @@ Go to your project's **Settings > Services > Slack** and you will see a checkbox
 * Build
 * Wiki page
 
-Bellow each of these event checkboxes you will have a input to insert which Slack channel do you want to send that event message,
-#general channel is default.
+Below each of these event checkboxes you will have an input to insert which Slack channel do you want to send that event message,
+`#general` channel is default.
 
 
 ![Slack configuration](img/slack_configuration.png)

spec/features/projects/slack_service/slack_service_spec.rb

+require 'spec_helper'
+
+feature 'Projects > Slack service > Setup events', feature: true do
+  let(:user) { create(:user) }
+  let(:service) { SlackService.new }
+  let(:project) { create(:project, slack_service: service) }
+
+  background do
+    service.fields
+    service.update_attributes(push_channel: 1, issue_channel: 2, merge_request_channel: 3, note_channel: 4, tag_push_channel: 5, build_channel: 6, wiki_page_channel: 7)
+    project.team << [user, :master]
+    login_as(user)
+  end
+
+  scenario 'user can filter events by channel' do
+    visit edit_namespace_project_service_path(project.namespace, project, service)
+
+    expect(page.find_field("service_push_channel").value).to have_content '1'
+    expect(page.find_field("service_issue_channel").value).to have_content '2'
+    expect(page.find_field("service_merge_request_channel").value).to have_content '3'
+    expect(page.find_field("service_note_channel").value).to have_content '4'
+    expect(page.find_field("service_tag_push_channel").value).to have_content '5'
+    expect(page.find_field("service_build_channel").value).to have_content '6'
+    expect(page.find_field("service_wiki_page_channel").value).to have_content '7'
+  end
+end

spec/models/project_services/slack_service_spec.rb

@@ -139,7 +139,7 @@ describe SlackService, models: true do
     end
 
     context "event channels" do
-      it "should user the right channel for push event" do
+      it "uses the right channel for push event" do
         slack.update_attributes(push_channel: "random")
 
         expect(Slack::Notifier).to receive(:new).
@@ -151,7 +151,7 @@ describe SlackService, models: true do
         slack.execute(push_sample_data)
       end
 
-      it "should use the right channel for merge request event" do
+      it "uses the right channel for merge request event" do
         slack.update_attributes(merge_request_channel: "random")
 
         expect(Slack::Notifier).to receive(:new).
@@ -163,7 +163,7 @@ describe SlackService, models: true do
         slack.execute(@merge_sample_data)
       end
 
-      it "should use the right channel for issue event" do
+      it "uses the right channel for issue event" do
         slack.update_attributes(issue_channel: "random")
 
         expect(Slack::Notifier).to receive(:new).
@@ -175,7 +175,7 @@ describe SlackService, models: true do
         slack.execute(@issues_sample_data)
       end
 
-      it "should use the right channel for wiki event" do
+      it "uses the right channel for wiki event" do
         slack.update_attributes(wiki_page_channel: "random")
 
         expect(Slack::Notifier).to receive(:new).
@@ -192,7 +192,7 @@ describe SlackService, models: true do
           create(:note_on_issue, project: project, note: "issue note")
         end
 
-        it "should use the right channel" do
+        it "uses the right channel" do
           slack.update_attributes(note_channel: "random")
 
           note_data = Gitlab::NoteDataBuilder.build(issue_note, user)