Change SlackService to SlackNotificationsServiceissue_22269_fix_eeissue_22269

19 files changed, 223 insertions, 168 deletions

diff --git a/app/models/project.rb b/app/models/project.rb
index 19c2d24212d..5d092ca42c2 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -95,8 +95,8 @@ class Project < ActiveRecord::Base
   has_one :asana_service, dependent: :destroy
   has_one :gemnasium_service, dependent: :destroy
   has_one :mattermost_slash_commands_service, dependent: :destroy
-  has_one :mattermost_service, dependent: :destroy
-  has_one :slack_service, dependent: :destroy
+  has_one :mattermost_notification_service, dependent: :destroy
+  has_one :slack_notification_service, dependent: :destroy
   has_one :buildkite_service, dependent: :destroy
   has_one :bamboo_service, dependent: :destroy
   has_one :teamcity_service, dependent: :destroy
diff --git a/app/models/project_services/chat_notification_service.rb b/app/models/project_services/chat_notification_service.rb
new file mode 100644
index 00000000000..b0556987721
--- /dev/null
+++ b/app/models/project_services/chat_notification_service.rb
@@ -0,0 +1,147 @@
+# Base class for Chat notifications services
+# This class is not meant to be used directly, but only to inherit from.
+class ChatNotificationService < Service
+  include ChatMessage
+
+  default_value_for :category, 'chat'
+
+  prop_accessor :webhook, :username, :channel
+  boolean_accessor :notify_only_broken_builds, :notify_only_broken_pipelines
+
+  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
+      self.notify_only_broken_pipelines = true
+    end
+  end
+
+  def can_test?
+    valid?
+  end
+
+  def supported_events
+    %w[push issue confidential_issue merge_request note tag_push
+       build pipeline wiki_page]
+  end
+
+  def execute(data)
+    return unless supported_events.include?(data[:object_kind])
+    return unless webhook.present?
+
+    object_kind = data[:object_kind]
+
+    data = data.merge(
+      project_url: project_url,
+      project_name: project_name
+    )
+
+    # WebHook events often have an 'update' event that follows a 'open' or
+    # 'close' action. Ignore update events for now to prevent duplicate
+    # messages from arriving.
+
+    message = get_message(object_kind, data)
+
+    return false unless message
+
+    opt = {}
+
+    opt[:channel]  = get_channel_field(object_kind).presence || channel || default_channel
+    opt[:username] = username if username
+    notifier = Slack::Notifier.new(webhook, opt)
+    notifier.ping(message.pretext, attachments: message.attachments, fallback: message.fallback)
+
+    true
+  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
+
+  def default_channel
+    raise NotImplementedError
+  end
+
+  private
+
+  def get_message(object_kind, data)
+    case object_kind
+    when "push", "tag_push"
+      PushMessage.new(data)
+    when "issue"
+      IssueMessage.new(data) unless is_update?(data)
+    when "merge_request"
+      MergeMessage.new(data) unless is_update?(data)
+    when "note"
+      NoteMessage.new(data)
+    when "build"
+      BuildMessage.new(data) if should_build_be_notified?(data)
+    when "pipeline"
+      PipelineMessage.new(data) if should_pipeline_be_notified?(data)
+    when "wiki_page"
+      WikiPageMessage.new(data)
+    end
+  end
+
+  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: default_channel }
+    end
+  end
+
+  def event_channel_name(event)
+    "#{event}_channel"
+  end
+
+  def project_name
+    project.name_with_namespace.gsub(/\s/, '')
+  end
+
+  def project_url
+    project.web_url
+  end
+
+  def is_update?(data)
+    data[:object_attributes][:action] == 'update'
+  end
+
+  def should_build_be_notified?(data)
+    case data[:commit][:status]
+    when 'success'
+      !notify_only_broken_builds?
+    when 'failed'
+      true
+    else
+      false
+    end
+  end
+
+  def should_pipeline_be_notified?(data)
+    case data[:object_attributes][:status]
+    when 'success'
+      !notify_only_broken_pipelines?
+    when 'failed'
+      true
+    else
+      false
+    end
+  end
+end
diff --git a/app/models/project_services/chat_service.rb b/app/models/project_services/chat_service.rb
index 8ac049ba939..574788462de 100644
--- a/app/models/project_services/chat_service.rb
+++ b/app/models/project_services/chat_service.rb
@@ -1,148 +1,21 @@
 # Base class for Chat services
-# This class is not meant to be used directly, but only to inherrit from.
+# This class is not meant to be used directly, but only to inherit from.
 class ChatService < Service
-  include ChatMessage
-
   default_value_for :category, 'chat'
 
-  prop_accessor :webhook, :username, :channel
-  boolean_accessor :notify_only_broken_builds, :notify_only_broken_pipelines
-
-  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)) }
+  has_many :chat_names, foreign_key: :service_id
 
-    if properties.nil?
-      self.properties = {}
-      self.notify_only_broken_builds = true
-      self.notify_only_broken_pipelines = true
-    end
-  end
-
-  def can_test?
-    valid?
+  def valid_token?(token)
+    self.respond_to?(:token) &&
+      self.token.present? &&
+      ActiveSupport::SecurityUtils.variable_size_secure_compare(token, self.token)
   end
 
   def supported_events
-    %w[push issue confidential_issue merge_request note tag_push
-       build pipeline wiki_page]
+    []
   end
 
-  def execute(data)
-    return unless supported_events.include?(data[:object_kind])
-    return unless webhook.present?
-
-    object_kind = data[:object_kind]
-
-    data = data.merge(
-      project_url: project_url,
-      project_name: project_name
-    )
-
-    # WebHook events often have an 'update' event that follows a 'open' or
-    # 'close' action. Ignore update events for now to prevent duplicate
-    # messages from arriving.
-
-    message = get_message(object_kind, data)
-
-    return false unless message
-
-    opt = {}
-
-    opt[:channel]  = get_channel_field(object_kind).presence || channel || default_channel
-    opt[:username] = username if username
-
-    notifier = Slack::Notifier.new(webhook, opt)
-    notifier.ping(message.pretext, attachments: message.attachments, fallback: message.fallback)
-
-    true
-  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
-
-  def default_channel
+  def trigger(params)
     raise NotImplementedError
   end
-
-  private
-
-  def get_message(object_kind, data)
-    case object_kind
-    when "push", "tag_push"
-      PushMessage.new(data)
-    when "issue"
-      IssueMessage.new(data) unless is_update?(data)
-    when "merge_request"
-      MergeMessage.new(data) unless is_update?(data)
-    when "note"
-      NoteMessage.new(data)
-    when "build"
-      BuildMessage.new(data) if should_build_be_notified?(data)
-    when "pipeline"
-      PipelineMessage.new(data) if should_pipeline_be_notified?(data)
-    when "wiki_page"
-      WikiPageMessage.new(data)
-    end
-  end
-
-  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: default_channel }
-    end
-  end
-
-  def event_channel_name(event)
-    "#{event}_channel"
-  end
-
-  def project_name
-    project.name_with_namespace.gsub(/\s/, '')
-  end
-
-  def project_url
-    project.web_url
-  end
-
-  def is_update?(data)
-    data[:object_attributes][:action] == 'update'
-  end
-
-  def should_build_be_notified?(data)
-    case data[:commit][:status]
-    when 'success'
-      !notify_only_broken_builds?
-    when 'failed'
-      true
-    else
-      false
-    end
-  end
-
-  def should_pipeline_be_notified?(data)
-    case data[:object_attributes][:status]
-    when 'success'
-      !notify_only_broken_pipelines?
-    when 'failed'
-      true
-    else
-      false
-    end
-  end
 end
diff --git a/app/models/project_services/mattermost_service.rb b/app/models/project_services/mattermost_notification_service.rb
index 9d61c251a32..de18c4b1f00 100644
--- a/app/models/project_services/mattermost_service.rb
+++ b/app/models/project_services/mattermost_notification_service.rb
@@ -1,4 +1,4 @@
-class MattermostService < ChatService
+class MattermostNotificationService < ChatNotificationService
   def title
     'Mattermost notifications'
   end
@@ -8,7 +8,7 @@ class MattermostService < ChatService
   end
 
   def to_param
-    'mattermost'
+    'mattermost_notification'
   end
 
   def help
@@ -28,7 +28,7 @@ class MattermostService < ChatService
 
   def default_fields
     [
-      { type: 'text', name: 'webhook',   placeholder: 'http://mattermost_host/hooks/...' },
+      { type: 'text', name: 'webhook', placeholder: 'http://mattermost_host/hooks/...' },
       { type: 'text', name: 'username', placeholder: 'username' },
       { type: 'checkbox', name: 'notify_only_broken_builds' },
       { type: 'checkbox', name: 'notify_only_broken_pipelines' },
diff --git a/app/models/project_services/mattermost_slash_commands_service.rb b/app/models/project_services/mattermost_slash_commands_service.rb
index 3993dfbda17..33431f41dc2 100644
--- a/app/models/project_services/mattermost_slash_commands_service.rb
+++ b/app/models/project_services/mattermost_slash_commands_service.rb
@@ -1,18 +1,8 @@
-class MattermostSlashCommandsService < Service
+class MattermostSlashCommandsService < ChatService
   include TriggersHelper
 
   prop_accessor :token
 
-  def valid_token?(token)
-    self.respond_to?(:token) &&
-      self.token.present? &&
-      ActiveSupport::SecurityUtils.variable_size_secure_compare(token, self.token)
-  end
-
-  def supported_events
-    []
-  end
-
   def can_test?
     false
   end
diff --git a/app/models/project_services/slack_service.rb b/app/models/project_services/slack_notification_service.rb
index 0df1743c4ba..3cbf89efba4 100644
--- a/app/models/project_services/slack_service.rb
+++ b/app/models/project_services/slack_notification_service.rb
@@ -1,4 +1,4 @@
-class SlackService < ChatService
+class SlackNotificationService < ChatNotificationService
   def title
     'Slack notifications'
   end
@@ -8,7 +8,7 @@ class SlackService < ChatService
   end
 
   def to_param
-    'slack'
+    'slack_notification'
   end
 
   def help
diff --git a/app/models/service.rb b/app/models/service.rb
index 8e58f2a1925..0bbab078cf6 100644
--- a/app/models/service.rb
+++ b/app/models/service.rb
@@ -220,8 +220,8 @@ class Service < ActiveRecord::Base
       pivotaltracker
       pushover
       redmine
-      mattermost
-      slack
+      mattermost_notification
+      slack_notification
       teamcity
     ]
   end
diff --git a/db/migrate/20141006143943_move_slack_service_to_webhook.rb b/db/migrate/20141006143943_move_slack_service_to_webhook.rb
index 8cb120f7007..42e88d6d6e3 100644
--- a/db/migrate/20141006143943_move_slack_service_to_webhook.rb
+++ b/db/migrate/20141006143943_move_slack_service_to_webhook.rb
@@ -1,7 +1,11 @@
 # rubocop:disable all
 class MoveSlackServiceToWebhook < ActiveRecord::Migration
+
+  DOWNTIME = true
+  DOWNTIME_REASON = 'Move old fields "token" and "subdomain" to one single field "webhook"'
+
   def change
-    SlackService.all.each do |slack_service|
+    SlackNotificationService.all.each do |slack_service|
       if ["token", "subdomain"].all? { |property| slack_service.properties.key? property }
         token = slack_service.properties['token']
         subdomain = slack_service.properties['subdomain']
diff --git a/db/migrate/20161213172958_change_slack_service_to_slack_notification_service.rb b/db/migrate/20161213172958_change_slack_service_to_slack_notification_service.rb
new file mode 100644
index 00000000000..a7278d7b5a6
--- /dev/null
+++ b/db/migrate/20161213172958_change_slack_service_to_slack_notification_service.rb
@@ -0,0 +1,14 @@
+class ChangeSlackServiceToSlackNotificationService < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = true
+  DOWNTIME_REASON = 'Rename SlackService to SlackNotificationService'
+
+  def up
+    execute("UPDATE services SET type = 'SlackNotificationService' WHERE type = 'SlackService'")
+  end
+
+  def down
+    execute("UPDATE services SET type = 'SlackService' WHERE type = 'SlackNotificationService'")
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
index 4711b7873af..5bd4ef1f21b 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -11,7 +11,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20161212142807) do
+ActiveRecord::Schema.define(version: 20161213172958) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
diff --git a/lib/api/services.rb b/lib/api/services.rb
index b1e072b4f47..59232c84c24 100644
--- a/lib/api/services.rb
+++ b/lib/api/services.rb
@@ -473,7 +473,7 @@ module API
           desc: 'The description of the tracker'
         }
       ],
-      'slack' => [
+      'slack-notification' => [
         {
           required: true,
           name: :webhook,
@@ -493,6 +493,14 @@ module API
           desc: 'The channel name'
         }
       ],
+      'mattermost-notification' => [
+        {
+          required: true,
+          name: :webhook,
+          type: String,
+          desc: 'The Mattermost webhook. e.g. http://mattermost_host/hooks/...'
+        }
+      ],
       'teamcity' => [
         {
           required: true,
diff --git a/spec/features/projects/import_export/test_project_export.tar.gz b/spec/features/projects/import_export/test_project_export.tar.gz
index bfe59bdb90e..d3165d07d7b 100644
--- a/spec/features/projects/import_export/test_project_export.tar.gz
+++ b/spec/features/projects/import_export/test_project_export.tar.gz
diff --git a/spec/features/projects/services/slack_service_spec.rb b/spec/features/projects/services/slack_service_spec.rb
index 16541f51d98..320ed13a01d 100644
--- a/spec/features/projects/services/slack_service_spec.rb
+++ b/spec/features/projects/services/slack_service_spec.rb
@@ -2,8 +2,8 @@ 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) }
+  let(:service) { SlackNotificationService.new }
+  let(:project) { create(:project, slack_notification_service: service) }
 
   background do
     service.fields
diff --git a/spec/lib/gitlab/import_export/all_models.yml b/spec/lib/gitlab/import_export/all_models.yml
index 068137f6255..9b49d6837c3 100644
--- a/spec/lib/gitlab/import_export/all_models.yml
+++ b/spec/lib/gitlab/import_export/all_models.yml
@@ -136,8 +136,8 @@ project:
 - assembla_service
 - asana_service
 - gemnasium_service
-- slack_service
-- mattermost_service
+- slack_notification_service
+- mattermost_notification_service
 - buildkite_service
 - bamboo_service
 - teamcity_service
diff --git a/spec/models/project_services/chat_notification_service_spec.rb b/spec/models/project_services/chat_notification_service_spec.rb
new file mode 100644
index 00000000000..b4fb1cd9ed9
--- /dev/null
+++ b/spec/models/project_services/chat_notification_service_spec.rb
@@ -0,0 +1,12 @@
+require 'spec_helper'
+
+describe ChatNotificationService, models: true do
+  describe "Associations" do
+
+    before do
+      allow(subject).to receive(:activated?).and_return(true)
+    end
+
+    it { is_expected.to validate_presence_of :webhook }
+  end
+end
diff --git a/spec/models/project_services/chat_service_spec.rb b/spec/models/project_services/chat_service_spec.rb
index e6314a43501..c6a45a3e1be 100644
--- a/spec/models/project_services/chat_service_spec.rb
+++ b/spec/models/project_services/chat_service_spec.rb
@@ -2,7 +2,14 @@ require 'spec_helper'
 
 describe ChatService, models: true do
   describe "Associations" do
-    before { allow(subject).to receive(:activated?).and_return(true) }
-    it { is_expected.to validate_presence_of :webhook }
+    it { is_expected.to have_many :chat_names }
+  end
+
+  describe '#valid_token?' do
+    subject { described_class.new }
+
+    it 'is false as it has no token' do
+      expect(subject.valid_token?('wer')).to be_falsey
+    end
   end
 end
diff --git a/spec/models/project_services/slack_service_spec.rb b/spec/models/project_services/mattermost_notification_service_spec.rb
index 4928391fd7e..c01e64b4c8e 100644
--- a/spec/models/project_services/slack_service_spec.rb
+++ b/spec/models/project_services/mattermost_notification_service_spec.rb
@@ -1,5 +1,5 @@
 require 'spec_helper'
 
-describe SlackService, models: true do
+describe MattermostNotificationService, models: true do
   it_behaves_like "slack or mattermost"
 end
diff --git a/spec/models/project_services/mattermost_service_spec.rb b/spec/models/project_services/slack_notification_service_spec.rb
index 1e5b4c715c3..59ddddf7454 100644
--- a/spec/models/project_services/mattermost_service_spec.rb
+++ b/spec/models/project_services/slack_notification_service_spec.rb
@@ -1,5 +1,5 @@
 require 'spec_helper'
 
-describe MattermostService, models: true do
+describe SlackNotificationService, models: true do
   it_behaves_like "slack or mattermost"
 end
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index 1d8e42202ea..bab3c3dbb02 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -22,8 +22,8 @@ describe Project, models: true do
     it { is_expected.to have_many(:protected_branches).dependent(:destroy) }
     it { is_expected.to have_many(:chat_services) }
     it { is_expected.to have_one(:forked_project_link).dependent(:destroy) }
-    it { is_expected.to have_one(:slack_service).dependent(:destroy) }
-    it { is_expected.to have_one(:mattermost_service).dependent(:destroy) }
+    it { is_expected.to have_one(:slack_notification_service).dependent(:destroy) }
+    it { is_expected.to have_one(:mattermost_notification_service).dependent(:destroy) }
     it { is_expected.to have_one(:pushover_service).dependent(:destroy) }
     it { is_expected.to have_one(:asana_service).dependent(:destroy) }
     it { is_expected.to have_many(:boards).dependent(:destroy) }