incorporate reviewzj-slack-slash-commands

18 files changed, 195 insertions, 129 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 88001b78658..1d4d45d202f 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -18,6 +18,7 @@ v 8.12.0 (unreleased)
   - Make push events have equal vertical spacing.
   - API: Ensure invitees are not returned in Members API.
   - Preserve applied filters on issues search.
+  - Support for four Slack slash commands !6400
   - Add two-factor recovery endpoint to internal API !5510
   - Pass the "Remember me" value to the U2F authentication form
   - Display stages in valid order in stages dropdown on build page
diff --git a/app/controllers/integrations_controller.rb b/app/controllers/integrations_controller.rb
index bb2705f1ff9..dbd73c8d890 100644
--- a/app/controllers/integrations_controller.rb
+++ b/app/controllers/integrations_controller.rb
@@ -1,16 +1,14 @@
 class IntegrationsController < ApplicationController
   respond_to :json
 
-  skip_before_filter :verify_authenticity_token
+  skip_before_action :verify_authenticity_token
   skip_before_action :authenticate_user!
 
-  before_action :integration
-
   def trigger
-    service = service(params[:command])
+    triggered_service = service(integration, params[:command])
 
-    if integration && service
-      render json: service.new(project, nil, params).execute
+    if triggered_service
+      render json: triggered_service.new(project, nil, params).execute
     else
       render json: no_integration_found
     end
@@ -33,15 +31,16 @@ class IntegrationsController < ApplicationController
     integration.project
   end
 
-  def service(command)
-    case command
-    when '/issue'
+  def service(integration, command)
+    return nil unless integration
+
+    if command == '/issue' && project.issues_enabled? && project.default_issues_tracker?
       Integrations::IssueService
-    when '/merge-request'
+    elsif command == '/merge-request' && project.merge_requests_enabled?
       Integrations::MergeRequestService
-    when '/pipeline'
+    elsif command == '/pipeline' && project.builds_enabled?
       Integrations::PipelineService
-    when '/snippet'
+    elsif command == '/snippet' && project.snippets_enabled?
       Integrations::ProjectSnippetService
     end
   end
diff --git a/app/models/integration.rb b/app/models/integration.rb
index 8e73e822432..3b373c55336 100644
--- a/app/models/integration.rb
+++ b/app/models/integration.rb
@@ -1,3 +1,6 @@
 class Integration < ActiveRecord::Base
-  belongs_to :project
+  belongs_to :project, required: true, validate: true
+
+  validates :name, presence: true
+  validates :external_token, presence: true, uniqueness: true
 end
diff --git a/app/models/project.rb b/app/models/project.rb
index 990a4ff124a..b5df7162e8a 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -17,7 +17,9 @@ class Project < ActiveRecord::Base
 
   UNKNOWN_IMPORT_URL = 'http://unknown.git'
 
-  delegate :feature_available?, :builds_enabled?, :wiki_enabled?, :merge_requests_enabled?, to: :project_feature, allow_nil: true
+  delegate :feature_available?, :builds_enabled?, :wiki_enabled?, :merge_requests_enabled?,
+                                :issues_enabled?, :snippets_enabled?, 
+                                to: :project_feature, allow_nil: true
 
   default_value_for :archived, false
   default_value_for :visibility_level, gitlab_config_features.visibility_level
@@ -142,7 +144,6 @@ class Project < ActiveRecord::Base
   has_many :deployments, dependent: :destroy
   has_many :integrations, dependent: :destroy
 
-
   accepts_nested_attributes_for :variables, allow_destroy: true
   accepts_nested_attributes_for :project_feature
 
diff --git a/app/models/project_feature.rb b/app/models/project_feature.rb
index 8c9534c3565..358beb658c8 100644
--- a/app/models/project_feature.rb
+++ b/app/models/project_feature.rb
@@ -52,6 +52,18 @@ class ProjectFeature < ActiveRecord::Base
     merge_requests_access_level > DISABLED
   end
 
+  def issues_enabled?
+    return true unless issues_access_level
+
+    issues_access_level > DISABLED
+  end
+
+  def snippets_enabled?
+    return true unless snippets_access_level
+
+    snippets_access_level > DISABLED
+  end
+
   private
 
   def get_permission(user, level)
diff --git a/app/services/integrations/base_service.rb b/app/services/integrations/base_service.rb
index 4f91dc7ec1c..5c6f7430c9c 100644
--- a/app/services/integrations/base_service.rb
+++ b/app/services/integrations/base_service.rb
@@ -5,7 +5,7 @@ module Integrations
         if resource_id
           find_resource
         else
-          collection.search(params[:text]).limit(10)
+          collection.search(params[:text]).limit(5)
         end
 
       generate_response(resource)
@@ -13,32 +13,22 @@ module Integrations
 
     private
 
-    def klass
-      raise NotImplementedError
-    end
-
     def find_resource
       collection.find_by(iid: resource_id)
     end
 
-    def title(*args)
-      raise NotImplementedError
+    def title(resource)
+      format("#{resource.title}")
     end
 
-    def link(*args)
+    def link(resource)
       raise NotImplementedError
     end
 
     def resource_id
-      if params[:text].is_a?(Integer) || params[:text].match(/\A\d+\z/)
-        params[:text].to_i
-      else
-        nil
-      end
-    end
+      data = params[:text].to_s.match(/\A(\$|#|!)?(\d+)\z/)
 
-    def fallback(*args)
-      raise NotImplementedError
+      data ? data[2].to_i : nil
     end
 
     def collection
@@ -46,22 +36,15 @@ module Integrations
     end
 
     def generate_response(resource)
-      if resource.nil?
-        respond_404
-      elsif resource.respond_to?(:count)
-        return generate_response(resource.first) if resource.count == 1
-        return no_search_results if resource.empty?
+      return respond_404 if resource.nil?
+      return single_resource(resource) unless resource.respond_to?(:count)
 
-        {
-          response_type: :ephemeral,
-          text: "Search results for #{params[:text]}",
-          attachments: resource.map { |item| small_attachment(item) }
-        }
+      if resource.empty?
+        no_search_results
+      elsif resource.count == 1
+        single_resource(resource.first)
       else
-        {
-          response_type: :in_channel,
-          attachments: [ large_attachment(resource) ]
-        }
+        multiple_resources(resource)
       end
     end
 
@@ -71,11 +54,26 @@ module Integrations
 
     def no_search_results
       {
-        text: "No search results for #{params[:text]}. :(",
+        text: "No search results for \"#{params[:text]}\". :disappointed:",
         response_type: :ephemeral
       }
     end
 
+    def single_resource(resource)
+      {
+        response_type: :in_channel,
+        attachments: [ large_attachment(resource) ]
+      }
+    end
+
+    def multiple_resources(resources)
+      {
+        response_type: :ephemeral,
+        text: "Search results for \"#{params[:text]}\"",
+        attachments: resources.map { |item| small_attachment(item) }
+      }
+    end
+
     def respond_404
       {
         text: "404 not found! Please make you use the right identifier. :boom:",
@@ -93,17 +91,16 @@ module Integrations
         title: title(issuable),
         title_link: link(issuable),
         text: issuable.description || "", # Slack doesn't like null
-        color: "#C95823"
       }
     end
 
     def fields(issuable)
       result = [
-          {
-              title: 'Author',
-              value: issuable.author.name,
-              short: true
-          }
+        {
+          title: 'Author',
+          value: issuable.author.name,
+          short: true
+        }
       ]
 
       if issuable.assignee
diff --git a/app/services/integrations/issue_service.rb b/app/services/integrations/issue_service.rb
index b00901afe3d..d1f53314dd4 100644
--- a/app/services/integrations/issue_service.rb
+++ b/app/services/integrations/issue_service.rb
@@ -1,14 +1,9 @@
 module Integrations
-  class IssueService < BaseService
-
+  class IssueService < Integrations::BaseService
     private
 
-    def klass
-      Issue
-    end
-
-    def title(issue)
-      format("##{issue.iid} #{issue.title}")
+    def collection
+      project.issues
     end
 
     def link(issue)
diff --git a/app/services/integrations/merge_request_service.rb b/app/services/integrations/merge_request_service.rb
index 6d8b3485cb5..4bec4b68c8a 100644
--- a/app/services/integrations/merge_request_service.rb
+++ b/app/services/integrations/merge_request_service.rb
@@ -1,24 +1,15 @@
 module Integrations
-  class MergeRequestService < BaseService
-
+  class MergeRequestService < Integrations::BaseService
     private
 
-    def klass
-      MergeRequest
-    end
-
     def collection
-      klass.where(target_project: project)
-    end
-
-    def title(merge_request)
-      format("!#{merge_request.iid} #{merge_request.title}")
+      project.merge_requests
     end
 
     def link(merge_request)
-      Gitlab::Routing.url_helpers.namespace_project_merge_request_url(project.namespace,
-                                                                      project,
-                                                                      merge_request)
+      Gitlab::Routing.
+        url_helpers
+        .namespace_project_merge_request_url(project.namespace, project, merge_request)
     end
   end
 end
diff --git a/app/services/integrations/pipeline_service.rb b/app/services/integrations/pipeline_service.rb
index d4562fe6ecd..543d85a4b8f 100644
--- a/app/services/integrations/pipeline_service.rb
+++ b/app/services/integrations/pipeline_service.rb
@@ -1,6 +1,5 @@
 module Integrations
-  class PipelineService < BaseService
-
+  class PipelineService < Integrations::BaseService
     def execute
       resource =
         if resource_id
@@ -15,19 +14,15 @@ module Integrations
     private
 
     def merge_request_pipeline(iid)
-      project.merge_requests.find_by(iid: iid).pipeline
+      project.merge_requests.find_by(iid: iid).try(:pipeline)
     end
 
     def pipeline_by_ref(ref)
       project.pipelines.where(ref: ref).last
     end
 
-    def klass
-      Pipeline
-    end
-
     def title(pipeline)
-      "##{pipeline.id} Pipeline for #{pipeline.ref}: #{pipeline.status}"
+      "Pipeline for #{pipeline.ref}: #{pipeline.status}"
     end
 
     def link(pipeline)
@@ -41,8 +36,29 @@ module Integrations
         fallback: title(pipeline),
         title: title(pipeline),
         title_link: link(pipeline),
-        color: "#C95823"
+        fields: [
+          fields(pipeline)
+        ]
       }
     end
+
+    def fields(pipeline)
+      commit = pipeline.commit
+
+      return [] unless commit
+
+      [
+        {
+          title: 'Author',
+          value: commit.author.name,
+          short: true
+        },
+        {
+          title: 'Commit Title',
+          value: commit.title,
+          short: true
+        }
+      ]
+    end
   end
 end
diff --git a/app/services/integrations/project_snippet_service.rb b/app/services/integrations/project_snippet_service.rb
index a363c176ed3..89a46cb9122 100644
--- a/app/services/integrations/project_snippet_service.rb
+++ b/app/services/integrations/project_snippet_service.rb
@@ -1,30 +1,19 @@
 module Integrations
-  class ProjectSnippetService < BaseService
-
+  class ProjectSnippetService < Integrations::BaseService
     private
 
-    def klass
-      ProjectSnippet
+    def collection
+      project.snippets
     end
 
     def find_resource
-      collection.find_by(id: params[:text])
-    end
-
-    def title(snippet)
-      format("$#{snippet.id} #{snippet.title}")
+      collection.find_by(id: resource_id)
     end
 
     def link(snippet)
-      Gitlab::Routing.url_helpers.namespace_project_snippet_url(project.namespace,
-                                                                project, snippet)
-    end
-
-    def attachment(snippet)
-      {
-        text: slack_format(snippet.content),
-        color: '#C95823',
-      }
+      Gitlab::Routing.
+        url_helpers.
+        namespace_project_snippet_url(project.namespace, project, snippet)
     end
 
     def small_attachment(snippet)
@@ -32,8 +21,7 @@ module Integrations
         fallback: snippet.title,
         title: title(snippet),
         title_link: link(snippet),
-        text: snippet.description || "", # Slack doesn't like null
-        color: '#345'
+        text: snippet.content || "", # Slack doesn't like null
       }
     end
 
@@ -41,7 +29,7 @@ module Integrations
       [
         {
           title: 'Author',
-          value: snippet.author,
+          value: snippet.author.name,
           short: true
         }
       ]
diff --git a/app/views/projects/integrations/_form.html.haml b/app/views/projects/integrations/_form.html.haml
index 1bb1b439996..8bc400c197e 100644
--- a/app/views/projects/integrations/_form.html.haml
+++ b/app/views/projects/integrations/_form.html.haml
@@ -3,11 +3,20 @@
     %h4.prepend-top-0
       Slack Integration (Experimental)
     %p
-      -# TODO
-      In Slack, create a new custom integration, and add a slash commands for each: `/issue`, `/merge-request`, `/pipeline`, and `/snippet`. Enter each token in a new integration here.
+      In Slack, create a new custom integration, and add a slash command for each:
+      = succeed ',' do
+        %code /issue
+      = succeed ',' do
+        %code /merge-request
+      = succeed ',' do
+        %code /pipeline
+      = succeed '.' do
+        %code /snippet
+
+      Enter each token in a new integration here.
 
     %p
-      The URL for slack to POST to is:
+      The URL for Slack to POST to is:
       = integrations_trigger_url
 
     = succeed "." do
diff --git a/db/migrate/20160913092152_create_integrations.rb b/db/migrate/20160913092152_create_integrations.rb
index 04ece0be8b2..4ac8f281199 100644
--- a/db/migrate/20160913092152_create_integrations.rb
+++ b/db/migrate/20160913092152_create_integrations.rb
@@ -1,4 +1,6 @@
 class CreateIntegrations < ActiveRecord::Migration
+  DOWNTIME = false
+
   def change
     create_table :integrations do |t|
       t.references :project, index: true, foreign_key: true
diff --git a/doc/project_services/slack-slash-commands.md b/doc/project_services/slack-slash-commands.md
new file mode 100644
index 00000000000..2451055a4c9
--- /dev/null
+++ b/doc/project_services/slack-slash-commands.md
@@ -0,0 +1,45 @@
+# Slack slash commands
+
+**NOTE:** At this time Slack slash commands for GitLab are experimental are it is
+likely to change in the future.
+
+When using Slack you could already use the [Slack service](slack.md) to
+notify your team on events on your GitLab instance. With Slack slash commands there
+is now a way for you to request data from GitLab.
+
+GitLab provides support for `/issue`, `/merge-request`, `/pipeline`, and `/snippet`
+commands.
+
+## Configuration
+
+To enable a slash command for your project, visit the **Integrations** page on your project.
+When creating a new integration you will have to list
+
+
+For configuring Slack slash commands for your project, you will have to register
+a custom slash command for your Slack app.
+
+
+A GitLab administrator can add a service template that sets a default for each
+project. This makes it much easier to configure individual projects.
+
+After the template is created, the template details will be pre-filled on a
+project's Service page.
+
+## Enable a Service template
+
+In GitLab's Admin area, navigate to **Service Templates** and choose the
+service template you wish to create.
+
+For example, in the image below you can see Redmine.
+
+![Redmine service template](img/services_templates_redmine_example.png)
+
+
+
+
+
+---
+
+
+[slack-service-docs]: ./slack
diff --git a/spec/controllers/integrations_controller_spec.rb b/spec/controllers/integrations_controller_spec.rb
index 3aad02da4cb..53f4619c983 100644
--- a/spec/controllers/integrations_controller_spec.rb
+++ b/spec/controllers/integrations_controller_spec.rb
@@ -6,16 +6,16 @@ RSpec.describe IntegrationsController, type: :controller do
       let(:slack_params) do
         {
           format: :json,
-          "token"=>"24randomcharacters",
-          "team_id"=>"T123456T9",
-          "team_domain"=>"mepmep",
-          "channel_id"=>"C12345678",
-          "channel_name"=>"general",
-          "user_id"=>"U12345678",
-          "user_name"=>"mep",
-          "command"=>"/issue",
-          "text"=>"3",
-          "response_url"=>"https://hooks.slack.com/commands/T123456T9/79958163905/siWqY7Qtx8z0zWFsXBod9VEy"
+          "token" => "24randomcharacters",
+          "team_id" => "T123456T9",
+          "team_domain" => "mepmep",
+          "channel_id" => "C12345678",
+          "channel_name" => "general",
+          "user_id" => "U12345678",
+          "user_name" => "mep",
+          "command" => "/issue",
+          "text" => "3",
+          "response_url" => "https://hooks.slack.com/commands/T123456T9/79958163905/siWqY7Qtx8z0zWFsXBod9VEy"
         }
       end
 
diff --git a/spec/models/integration_spec.rb b/spec/models/integration_spec.rb
index aae8d31dae1..b8aa4acb211 100644
--- a/spec/models/integration_spec.rb
+++ b/spec/models/integration_spec.rb
@@ -1,5 +1,15 @@
 require 'rails_helper'
 
 RSpec.describe Integration, type: :model do
-  pending "add some examples to (or delete) #{__FILE__}"
+  subject { create(:integration) }
+
+  describe 'associations' do
+    it { is_expected.to belong_to(:project) }
+  end
+
+  describe 'validation' do
+    it { is_expected.to validate_presence_of(:project) }
+    it { is_expected.to validate_presence_of(:name) }
+    it { is_expected.to validate_presence_of(:external_token) }
+  end
 end
diff --git a/spec/services/integrations/merge_request_service_spec.rb b/spec/services/integrations/merge_request_service_spec.rb
index 9e4e5f21951..d42829ecd24 100644
--- a/spec/services/integrations/merge_request_service_spec.rb
+++ b/spec/services/integrations/merge_request_service_spec.rb
@@ -12,19 +12,16 @@ describe Integrations::MergeRequestService, services: true do
       let(:mr)     { create(:merge_request, source_project: project) }
 
       it 'returns the issue by ID' do
-        expect(subject[:text]).to match /!\d+\s#{Regexp.quote(mr.title)}/
+        expect(subject[:attachments].first[:fallback]).to eq mr.title
       end
     end
 
     context 'when searching with only one result' do
-      let(:title)  { 'Aint this a String?' }
-      let(:params) { { text: title[2..7] } }
+      let(:params)          { { text: merge_request.title[2..7] } }
+      let!(:merge_request)  { create(:merge_request, source_project: project) }
 
       it 'returns the search results' do
-        create(:merge_request, source_project: project, title: title)
-        create(:merge_request, source_project: project)
-
-        expect(subject[:text]).to match /!\d+\sAint\sthis/
+        expect(subject[:attachments].first[:fallback]).to eq merge_request.title
       end
     end
   end
diff --git a/spec/services/integrations/pipeline_service_spec.rb b/spec/services/integrations/pipeline_service_spec.rb
index 1e4a2561f8c..19e0d5ed5be 100644
--- a/spec/services/integrations/pipeline_service_spec.rb
+++ b/spec/services/integrations/pipeline_service_spec.rb
@@ -12,7 +12,7 @@ describe Integrations::PipelineService, services: true do
       let(:params) { { text: pipeline.ref } }
 
       it 'returns the pipeline by ID' do
-        expect(subject[:text]).to match /#\d+\ Pipeline\ for\ #{pipeline.ref}: #{pipeline.status}/
+        expect(subject[:attachments].first[:fallback]).to match /Pipeline\ for\ #{pipeline.ref}: #{pipeline.status}/
       end
     end
   end
diff --git a/spec/services/integrations/project_snippet_service_spec.rb b/spec/services/integrations/project_snippet_service_spec.rb
index 8d2a16ac815..6efb4a09152 100644
--- a/spec/services/integrations/project_snippet_service_spec.rb
+++ b/spec/services/integrations/project_snippet_service_spec.rb
@@ -9,10 +9,10 @@ describe Integrations::ProjectSnippetService, services: true do
   describe '#execute' do
     context 'looking up by ID' do
       let(:snippet) { create(:project_snippet, project: project) }
-      let(:params) { { text: snippet.id } }
+      let(:params) { { text: "$#{snippet.id}" } }
 
-      it 'returns the issue by ID' do
-        expect(subject[:text]).to match /\$\d+\s#{Regexp.quote(snippet.title)}/
+      it 'returns the snippet by ID' do
+        expect(subject[:attachments].first[:fallback]).to eq snippet.title
       end
     end
   end