More refactoring, push present to base command

app/models/project.rb

@@ -91,7 +91,7 @@ class Project < ActiveRecord::Base
   has_one :assembla_service, dependent: :destroy
   has_one :asana_service, dependent: :destroy
   has_one :gemnasium_service, dependent: :destroy
-  has_one :mattermost_chat_service, dependent: :destroy
+  has_one :mattermost_command_service, dependent: :destroy
   has_one :slack_service, dependent: :destroy
   has_one :buildkite_service, dependent: :destroy
   has_one :bamboo_service, dependent: :destroy

app/models/project_services/mattermost_command_service.rb

@@ -38,19 +38,19 @@ class MattermostCommandService < ChatService
     user = find_chat_user(params)
     unless user
       url = authorize_chat_name_url(params)
-      return Mattermost::Presenter.authorize_user(url)
+      return Mattermost::Presenter.authorize_chat_name(url)
     end
 
-    Mattermost::CommandService.new(project, user, params).execute
+    Gitlab::ChatCommands::Command.new(project, user, params).execute
   end
 
   private
 
   def find_chat_user(params)
-    ChatNames::FindUserService.new(chat_names, params).execute
+    ChatNames::FindUserService.new(self, params).execute
   end
 
   def authorize_chat_name_url(params)
-    ChatNames::RequestService.new(self, params).execute
+    ChatNames::AuthorizeUserService.new(self, params).execute
   end
 end

lib/api/services.rb

@@ -61,7 +61,6 @@ module API
     end
 
     resource :projects do
-
       desc 'Trigger a slash command' do
         detail 'Added in GitLab 8.13'
       end
@@ -78,7 +77,8 @@ module API
         result = service.try(:active?) && service.try(:trigger, params)
 
         if result
-          present result, status: result[:status] || 200
+          status result[:status] || 200
+          present result
         else
           not_found!('Service')
         end

lib/gitlab/chat_commands/base_command.rb

@@ -15,6 +15,14 @@ module Gitlab
         raise NotImplementedError
       end
 
+      def self.allowed?(_, _)
+        true
+      end
+
+      def self.can?(object, action, subject)
+        Ability.allowed?(object, action, subject)
+      end
+
       def execute(_)
         raise NotImplementedError
       end
@@ -31,21 +39,11 @@ module Gitlab
 
       private
 
-      def can?(object, action, subject)
-        Ability.allowed?(object, action, subject)
-      end
-
       def find_by_iid(iid)
         resource = collection.find_by(iid: iid)
 
         readable?(resource) ? resource : nil
       end
-
-      def search_results(query)
-        collection.search(query).limit(QUERY_LIMIT).select do |resource|
-          readable?(resource)
-        end
-      end
     end
   end
 end

lib/gitlab/chat_commands/command.rb

@@ -7,10 +7,14 @@ module Gitlab
       ].freeze
 
       def execute
-        klass, match = fetch_klass
+        command, match = match_command
 
-        if klass
-          present klass.new(project, current_user, params).execute(match)
+        if command
+          if command.allowed?(project, current_user)
+            present command.new(project, current_user, params).execute(match)
+          else
+            access_denied
+          end
         else
           help(help_messages)
         end
@@ -18,7 +22,7 @@ module Gitlab
 
       private
 
-      def fetch_klass
+      def match_command
         match = nil
         service = available_commands.find do |klass|
           match = klass.match(command)
@@ -28,9 +32,7 @@ module Gitlab
       end
 
       def help_messages
-        available_commands.map do |klass|
-          klass.help_message
-        end
+        available_commands.map(&:help_message)
       end
 
       def available_commands
@@ -43,13 +45,17 @@ module Gitlab
         params[:text]
       end
 
-      def present(resource)
-        Mattermost::Presenter.present(resource)
-      end
-
       def help(messages)
         Mattermost::Presenter.help(messages, params[:command])
       end
+
+      def access_denied
+        Mattermost::Presenter.access_denied
+      end
+
+      def present(resource)
+        Mattermost::Presenter.present(resource)
+      end
     end
   end
 end

lib/gitlab/chat_commands/issue_command.rb

@@ -10,7 +10,7 @@ module Gitlab
       end
 
       def readable?(issue)
-        can?(current_user, :read_issue, issue)
+        self.class.can?(current_user, :read_issue, issue)
       end
     end
   end

lib/gitlab/chat_commands/issue_create.rb

@@ -9,9 +9,11 @@ module Gitlab
         'issue create <title>\n<description>'
       end
 
-      def execute(match)
-        return nil unless can?(current_user, :create_issue, project)
+      def self.allowed?(project, user)
+        can?(user, :create_issue, project)
+      end
 
+      def execute(match)
         title = match[:title]
         description = match[:description]
 

lib/mattermost/presenter.rb

@@ -4,24 +4,19 @@ module Mattermost
       include Rails.application.routes.url_helpers
 
       def authorize_chat_name(url)
-        message = "Hi there! We've yet to get acquainted! Please introduce yourself by [connection your GitLab profile](#{url})!"
+        message = ":wave: Hi there! Before I do anything for you, please [connect your GitLab account](#{url})."
 
         ephemeral_response(message)
       end
 
-      def help(messages, command)
-        return ephemeral_response("No commands configured") unless messages.count > 1
-        message = ["Available commands:"]
-
-        messages.each do |messsage|
-          message << "- #{command} #{message}"
-        end
+      def help(commands, trigger)
+        if commands.count == 0
+          ephemeral_response("No commands configured") unless messages.count > 1
+        else
+          message = header_with_list("Available commands", commands)
 
-        ephemeral_response(message.join("\n"))
+          ephemeral_response(message)
         end
-
-      def not_found
-        ephemeral_response("404 not found! GitLab couldn't find what your were looking for! :boom:")
       end
 
       def present(resource)
@@ -40,8 +35,16 @@ module Mattermost
         single_resource(resource)
       end
 
+      def access_denied
+        ephemeral_response("Whoops! That action is not allowed. This incident will be [reported](https://xkcd.com/838/).")
+      end
+
       private
 
+      def not_found
+        ephemeral_response("404 not found! GitLab couldn't find what your were looking for! :boom:")
+      end
+
       def single_resource(resource)
         return error(resource) if resource.errors.any?
 
@@ -52,23 +55,33 @@ module Mattermost
       end
 
       def multiple_resources(resources)
-        message = "Multiple results were found:\n"
-        message << resources.map { |resource| "- #{title(resource)}" }.join("\n")
+        resources.map! { |resource| title(resource) }
+
+        message = header_with_list("Multiple results were found:", resources)
 
         ephemeral_response(message)
       end
 
       def error(resource)
-        message = "The action was not succesfull because:\n"
-        message << resource.errors.messages.map { |message| "- #{message}" }.join("\n")
+        message = header_with_list("The action was not succesful, because:", resource.errors.messages)
 
-        ephemeral_response(resource.errors.messages.join("\n"))
+        ephemeral_response(message)
       end
 
       def title(resource)
         "[#{resource.to_reference} #{resource.title}](#{url(resource)})"
       end
 
+      def header_with_list(header, items)
+        message = [header]
+
+        items.each do |item|
+          message << "- #{item}"
+        end
+
+        message.join("\n")
+      end
+
       def url(resource)
         url_for(
           [
@@ -82,14 +95,16 @@ module Mattermost
       def ephemeral_response(message)
         {
           response_type: :ephemeral,
-          text: message
+          text: message,
+          status: 200
         }
       end
 
       def in_channel_response(message)
         {
           response_type: :in_channel,
-          text: message
+          text: message,
+          status: 200
         }
       end
     end

spec/lib/gitlab/chat_commands/command_spec.rb

@@ -26,6 +26,15 @@ describe Gitlab::ChatCommands::Command, service: true do
       end
     end
 
+    context 'the user can not create an issue' do
+      let(:params) { { text: "issue create my new issue" } }
+
+      it 'rejects the actions' do
+        expect(subject[:response_type]).to be(:ephemeral)
+        expect(subject[:text]).to start_with('Whoops! That action is not allowed')
+      end
+    end
+
     context 'issue is succesfully created' do
       let(:params) { { text: "issue create my new issue" } }
 

spec/lib/gitlab/chat_name_token_spec.rb

@@ -12,9 +12,9 @@ describe Gitlab::ChatNameToken, lib: true do
   end
 
   context 'when storing data' do
-    let(:data) {
+    let(:data) do
       { key: 'value' }
-    }
+    end
 
     subject { described_class.new(@token) }
 

spec/models/project_spec.rb

@@ -35,6 +35,7 @@ describe Project, models: true do
     it { is_expected.to have_one(:hipchat_service).dependent(:destroy) }
     it { is_expected.to have_one(:flowdock_service).dependent(:destroy) }
     it { is_expected.to have_one(:assembla_service).dependent(:destroy) }
+    it { is_expected.to have_one(:mattermost_command_service).dependent(:destroy) }
     it { is_expected.to have_one(:gemnasium_service).dependent(:destroy) }
     it { is_expected.to have_one(:buildkite_service).dependent(:destroy) }
     it { is_expected.to have_one(:bamboo_service).dependent(:destroy) }

spec/services/chat_names/authorize_user_service_spec.rb

@@ -17,7 +17,7 @@ describe ChatNames::AuthorizeUserService, services: true do
     end
 
     context 'when there are missing parameters' do
-      let(:params) { { } }
+      let(:params) { {} }
 
       it 'does not request a new token' do
         is_expected.to be_nil