Merge branch 'issue_8110' into 'master'

Allow slack service to send messages on different channels

closes #8110 

## Allow slack service to send messages on different channels  

![new_slack_service](/uploads/87de0bd6b02a4f7853358676b5e74dff/new_slack_service.png)

## Does this MR meet the acceptance criteria?

- [x] [CHANGELOG](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CHANGELOG) entry added
- [x] [Documentation created/updated](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/doc/development/doc_styleguide.md)
- [x] API support added
- Tests
  - [x] Added for this feature/bug
  - [x] All builds are passing
- [x] Conform by the [style guides](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CONTRIBUTING.md#style-guides)
- [x] Branch has no merge conflicts with `master` (if you do - rebase it please)
- [x] [Squashed related commits together](https://git-scm.com/book/en/Git-Tools-Rewriting-History#Squashing-Commits)

See merge request !5124

CHANGELOG

@@ -108,6 +108,7 @@ v 8.10.0 (unreleased)
   - Don't render discussion notes when requesting diff tab through AJAX
   - Add basic system information like memory and disk usage to the admin panel
   - Don't garbage collect commits that have related DB records like comments
+  - Allow to setup event by channel on slack service
   - More descriptive message for git hooks and file locks
   - Aliases of award emoji should be stored as original name. !5060 (dixpac)
   - Handle custom Git hook result in GitLab UI

app/controllers/admin/services_controller.rb

@@ -1,4 +1,6 @@
 class Admin::ServicesController < Admin::ApplicationController
+  include ServiceParams
+
   before_action :service, only: [:edit, :update]
 
   def index
@@ -13,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
@@ -37,15 +39,4 @@ class Admin::ServicesController < Admin::ApplicationController
   def service
     @service ||= Service.where(id: params[:id], template: true).first
   end
-
-  def application_services_params
-    application_services_params = params.permit(:id,
-      service: Projects::ServicesController::ALLOWED_PARAMS)
-    if application_services_params[:service].is_a?(Hash)
-      Projects::ServicesController::FILTER_BLANK_PARAMS.each do |param|
-        application_services_params[:service].delete(param) if application_services_params[:service][param].blank? 
-      end
-    end
-    application_services_params
-  end
 end

app/controllers/concerns/service_params.rb

@@ -0,0 +1,35 @@
+module ServiceParams
+  extend ActiveSupport::Concern
+
+  ALLOWED_PARAMS = [:title, :token, :type, :active, :api_key, :api_url, :api_version, :subdomain,
+                    :room, :recipients, :project_url, :webhook,
+                    :user_key, :device, :priority, :sound, :bamboo_url, :username, :password,
+                    :build_key, :server, :teamcity_url, :drone_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, :build_events, :wiki_page_events,
+                    :notify_only_broken_builds, :add_pusher,
+                    :send_from_committer_email, :disable_diffs, :external_wiki_url,
+                    :notify, :color,
+                    :server_host, :server_port, :default_irc_uri, :enable_ssl_verification,
+                    :jira_issue_transition_id]
+
+  # Parameters to ignore if no value is specified
+  FILTER_BLANK_PARAMS = [:password]
+
+  def service_params
+    dynamic_params = []
+    dynamic_params.concat(@service.event_channel_names)
+
+    service_params = params.permit(:id, service: ALLOWED_PARAMS + dynamic_params)
+
+    if service_params[:service].is_a?(Hash)
+      FILTER_BLANK_PARAMS.each do |param|
+        service_params[:service].delete(param) if service_params[:service][param].blank?
+      end
+    end
+
+    service_params
+  end
+end

app/controllers/projects/services_controller.rb

@@ -1,20 +1,5 @@
 class Projects::ServicesController < Projects::ApplicationController
-  ALLOWED_PARAMS = [:title, :token, :type, :active, :api_key, :api_url, :api_version, :subdomain,
-                    :room, :recipients, :project_url, :webhook,
-                    :user_key, :device, :priority, :sound, :bamboo_url, :username, :password,
-                    :build_key, :server, :teamcity_url, :drone_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, :build_events, :wiki_page_events,
-                    :notify_only_broken_builds, :add_pusher,
-                    :send_from_committer_email, :disable_diffs, :external_wiki_url,
-                    :notify, :color,
-                    :server_host, :server_port, :default_irc_uri, :enable_ssl_verification,
-                    :jira_issue_transition_id]
-
-  # Parameters to ignore if no value is specified
-  FILTER_BLANK_PARAMS = [:password]
+  include ServiceParams
 
   # Authorize
   before_action :authorize_admin_project!
@@ -33,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:
@@ -64,12 +49,4 @@ class Projects::ServicesController < Projects::ApplicationController
   def service
     @service ||= @project.services.find { |service| service.to_param == params[:id] }
   end
-
-  def service_params
-    service_params = params.require(:service).permit(ALLOWED_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

@@ -0,0 +1,25 @@
+module ServicesHelper
+  def service_event_description(event)
+    case event
+    when "push"
+      "Event will be triggered by a push to the repository"
+    when "tag_push"
+      "Event will be triggered when a new tag is pushed to the repository"
+    when "note"
+      "Event will be triggered when someone adds a comment"
+    when "issue"
+      "Event will be triggered when an issue is created/updated/merged"
+    when "merge_request"
+      "Event will be triggered when a merge request is created/updated/merged"
+    when "build"
+      "Event will be triggered when a build status changes"
+    when "wiki_page"
+      "Event will be triggered when a wiki page is created/updated"
+    end
+  end
+
+  def service_event_field_name(event)
+    event = event.pluralize if %w[merge_request issue].include?(event)
+    "#{event}_events"
+  end
+end

app/models/project_services/slack_service.rb

@@ -4,6 +4,9 @@ class SlackService < Service
   validates :webhook, presence: true, url: true, if: :activated?
 
   def initialize_properties
+    # Custom serialized properties initialization
+    self.supported_events.each { |event| self.class.prop_accessor(event_channel_name(event)) }
+
     if properties.nil?
       self.properties = {}
       self.notify_only_broken_builds = true
@@ -29,13 +32,15 @@ class SlackService < Service
   end
 
   def fields
+    default_fields =
       [
-      { type: 'text', name: 'webhook',
-        placeholder: 'https://hooks.slack.com/services/...' },
+        { type: 'text', name: 'webhook',   placeholder: 'https://hooks.slack.com/services/...' },
         { type: 'text', name: 'username', placeholder: 'username' },
-      { type: 'text', name: 'channel', placeholder: '#channel' },
+        { type: 'text', name: 'channel', placeholder: "#general" },
         { type: 'checkbox', name: 'notify_only_broken_builds' },
       ]
+
+    default_fields + build_event_channels
   end
 
   def supported_events
@@ -74,7 +79,10 @@ class SlackService < Service
       end
 
     opt = {}
-    opt[:channel] = channel if channel
+
+    event_channel = get_channel_field(object_kind) || channel
+
+    opt[:channel] = event_channel if event_channel
     opt[:username] = username if username
 
     if message
@@ -83,8 +91,35 @@ class SlackService < Service
     end
   end
 
+  def event_channel_names
+    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)
+    field_name = event_channel_name(event)
+    self.public_send(field_name)
+  end
+
+  def build_event_channels
+    supported_events.reduce([]) do |channels, event|
+      channels << { type: 'text', name: event_channel_name(event), placeholder: "#general" }
+    end
+  end
+
+  def event_channel_name(event)
+    "#{event}_channel"
+  end
+
   def project_name
     project.name_with_namespace.gsub(/\s/, '')
   end

app/models/service.rb

@@ -82,6 +82,18 @@ class Service < ActiveRecord::Base
     Gitlab::PushDataBuilder.build_sample(project, user)
   end
 
+  def event_channel_names
+    []
+  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/projects/services/_form.html.haml

@@ -8,6 +8,7 @@
   .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|
       = render 'shared/service_settings', form: form
+
       = form.submit 'Save changes', class: 'btn btn-save'
        
       - if @service.valid? && @service.activated?

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.label :url, "Trigger", class: 'control-label'
+
   .col-sm-10
-      - if @service.supported_events.include?("push")
+    - @service.supported_events.each do |event|
       %div
-          = form.check_box :push_events, class: 'pull-left'
+        = form.check_box service_event_field_name(event), 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")
-        %div
-          = form.check_box :wiki_page_events, 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)
 
-- @service.fields.each do |field|
+      - if field
+        %p
+          = form.text_field field[:name], class: "form-control", placeholder: field[:placeholder]
+
+      %p.light
+        = service_event_description(event)
+
+- @service.global_fields.each do |field|
   - type = field[:type]
 
   - if type == 'fieldset'

doc/integration/slack.md

@@ -26,12 +26,11 @@ 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
+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
     - Username: Fill this in if you want to change the username of the bot
-    - Channel: Fill this in if you want to change the channel where the messages will be posted
     - Mark it as active
 
 1. Save your settings

doc/project_services/project_services.md

@@ -45,7 +45,7 @@ further configuration instructions and details. Contributions are welcome.
 | PivotalTracker | Project Management Software (Source Commits Endpoint) |
 | Pushover | Pushover makes it easy to get real-time notifications on your Android device, iPhone, iPad, and Desktop |
 | [Redmine](redmine.md) | Redmine issue tracker |
-| Slack | A team communication tool for the 21st century |
+| [Slack](slack.md) | A team communication tool for the 21st century |
 
 ## Services Templates
 

doc/project_services/slack.md

@@ -0,0 +1,26 @@
+# Slack Service
+
+Go to your project's **Settings > Services > Slack** and you will see a checkbox with the following events that can be triggered:
+
+* Push
+* Issue
+* Merge request
+* Note
+* Tag push
+* Build
+* Wiki page
+
+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)
+
+
+| Field | Description |
+| ----- | ----------- |
+| `Webhook`   | The incoming webhook url which you have to setup on slack. (https://my.slack.com/services/new/incoming-webhook/) |
+| `Username`   | Optional username which can be on messages sent to slack. |
+| `notify only broken builds`    | Notify only about broken builds, when build events are marked to be sent.|
+
+

features/steps/admin/settings.rb

@@ -27,19 +27,19 @@ class Spinach::Features::AdminSettings < Spinach::FeatureSteps
 
   step 'I check all events and submit form' do
     page.check('Active')
-    page.check('Push events')
-    page.check('Tag push events')
-    page.check('Comments')
-    page.check('Issues events')
-    page.check('Merge Request events')
-    page.check('Build events')
+    page.check('Push')
+    page.check('Tag push')
+    page.check('Note')
+    page.check('Issue')
+    page.check('Merge request')
+    page.check('Build')
     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'
+    fill_in 'service_push_channel', with: '#test_channel'
     page.check('Notify only broken builds')
   end
 
@@ -56,6 +56,6 @@ class Spinach::Features::AdminSettings < Spinach::FeatureSteps
   step 'I should see Slack settings saved' do
     expect(find_field('Webhook').value).to eq 'http://localhost'
     expect(find_field('Username').value).to eq 'test_user'
-    expect(find_field('Channel').value).to eq '#test_channel'
+    expect(find('#service_push_channel').value).to eq '#test_channel'
   end
 end

spec/features/projects/slack_service/slack_service_spec.rb

@@ -0,0 +1,26 @@
+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

@@ -124,6 +124,7 @@ describe SlackService, models: true do
        and_return(
          double(:slack_service).as_null_object
        )
+
       slack.execute(push_sample_data)
     end
 
@@ -136,6 +137,76 @@ describe SlackService, models: true do
         )
       slack.execute(push_sample_data)
     end
+
+    context "event channels" do
+      it "uses the right channel for push event" do
+        slack.update_attributes(push_channel: "random")
+
+        expect(Slack::Notifier).to receive(:new).
+         with(webhook_url, channel: "random").
+         and_return(
+           double(:slack_service).as_null_object
+         )
+
+        slack.execute(push_sample_data)
+      end
+
+      it "uses the right channel for merge request event" do
+        slack.update_attributes(merge_request_channel: "random")
+
+        expect(Slack::Notifier).to receive(:new).
+         with(webhook_url, channel: "random").
+         and_return(
+           double(:slack_service).as_null_object
+         )
+
+        slack.execute(@merge_sample_data)
+      end
+
+      it "uses the right channel for issue event" do
+        slack.update_attributes(issue_channel: "random")
+
+        expect(Slack::Notifier).to receive(:new).
+         with(webhook_url, channel: "random").
+         and_return(
+           double(:slack_service).as_null_object
+         )
+
+        slack.execute(@issues_sample_data)
+      end
+
+      it "uses the right channel for wiki event" do
+        slack.update_attributes(wiki_page_channel: "random")
+
+        expect(Slack::Notifier).to receive(:new).
+         with(webhook_url, channel: "random").
+         and_return(
+           double(:slack_service).as_null_object
+         )
+
+        slack.execute(@wiki_page_sample_data)
+      end
+
+      context "note event" do
+        let(:issue_note) do
+          create(:note_on_issue, project: project, note: "issue note")
+        end
+
+        it "uses the right channel" do
+          slack.update_attributes(note_channel: "random")
+
+          note_data = Gitlab::NoteDataBuilder.build(issue_note, user)
+
+          expect(Slack::Notifier).to receive(:new).
+           with(webhook_url, channel: "random").
+           and_return(
+             double(:slack_service).as_null_object
+           )
+
+          slack.execute(note_data)
+        end
+      end
+    end
   end
 
   describe "Note events" do