Improve style, add more tests

10 files changed, 38 insertions, 29 deletions

diff --git a/changelogs/unreleased/zj-slash-commands-mattermost.yml b/changelogs/unreleased/zj-slash-commands-mattermost.yml
index 84aca57e666..996ffe954f3 100644
--- a/changelogs/unreleased/zj-slash-commands-mattermost.yml
+++ b/changelogs/unreleased/zj-slash-commands-mattermost.yml
@@ -1,4 +1,4 @@
 ---
-title: Add first slash commands
+title: Added Mattermost slash command
 merge_request: 7438
 author: 
diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb
index d6526ec4fdc..2c593dbb4ea 100644
--- a/lib/api/helpers.rb
+++ b/lib/api/helpers.rb
@@ -85,20 +85,11 @@ module API
       end
     end
 
-    def project_service
-      @project_service ||= user_project.find_or_initialize_service(params[:service_slug].underscore)
+    def project_service(project = user_project)
+      @project_service ||= project.find_or_initialize_service(params[:service_slug].underscore)
       @project_service || not_found!("Service")
     end
 
-    def service_by_slug(project, slug)
-      underscored_service = slug.underscore
-
-      not_found!('Service') unless Service.available_services_names.include?(underscored_service)
-      service_method = "#{underscored_service}_service"
-
-      service = project.public_send(service_method)
-    end
-
     def service_attributes
       @service_attributes ||= project_service.fields.inject([]) do |arr, hash|
         arr << hash[:name].to_sym
diff --git a/lib/api/services.rb b/lib/api/services.rb
index 163187d450d..e3c6a998631 100644
--- a/lib/api/services.rb
+++ b/lib/api/services.rb
@@ -67,7 +67,9 @@ module API
       post ':id/services/:service_slug/trigger' do
         project = Project.find_with_namespace(params[:id]) || Project.find_by(id: params[:id])
 
-        service = service_by_slug(project, params[:service_slug])
+        not_found! unless project
+
+        service = project_service(project)
 
         result = service.try(:active?) && service.try(:trigger, params)
 
@@ -75,7 +77,7 @@ module API
           status result[:status] || 200
           present result
         else
-          not_found!('Service')
+          not_found!
         end
       end
     end
diff --git a/lib/gitlab/chat_commands/base_command.rb b/lib/gitlab/chat_commands/base_command.rb
index f84aca5365d..e59d69b72b9 100644
--- a/lib/gitlab/chat_commands/base_command.rb
+++ b/lib/gitlab/chat_commands/base_command.rb
@@ -3,7 +3,7 @@ module Gitlab
     class BaseCommand
       QUERY_LIMIT = 5
 
-      def self.match(_)
+      def self.match(_text)
         raise NotImplementedError
       end
 
@@ -11,11 +11,11 @@ module Gitlab
         raise NotImplementedError
       end
 
-      def self.available?(_)
+      def self.available?(_project)
         raise NotImplementedError
       end
 
-      def self.allowed?(_, _)
+      def self.allowed?(_user, _ability)
         true
       end
 
diff --git a/lib/mattermost/presenter.rb b/lib/mattermost/presenter.rb
index d7455d39bce..b4e7358770f 100644
--- a/lib/mattermost/presenter.rb
+++ b/lib/mattermost/presenter.rb
@@ -1,7 +1,7 @@
 module Mattermost
   class Presenter
     class << self
-      include Rails.application.routes.url_helpers
+      include Gitlab::Routing.url_helpers
 
       def authorize_chat_name(url)
         message = if url
@@ -14,7 +14,7 @@ module Mattermost
       end
 
       def help(commands, trigger)
-        if commands.count == 0
+        if commands.empty?
           ephemeral_response("No commands configured") unless messages.count > 1
         else
           message = header_with_list("Available commands", commands)
@@ -50,7 +50,7 @@ module Mattermost
       end
 
       def single_resource(resource)
-        return error(resource) if resource.errors.any?
+        return error(resource) if resource.errors.any? || !resource.persisted?
 
         message = "### #{title(resource)}"
         message << "\n\n#{resource.description}" if resource.description
diff --git a/spec/lib/gitlab/chat_commands/command_spec.rb b/spec/lib/gitlab/chat_commands/command_spec.rb
index 11b607a6843..f44c848479e 100644
--- a/spec/lib/gitlab/chat_commands/command_spec.rb
+++ b/spec/lib/gitlab/chat_commands/command_spec.rb
@@ -7,7 +7,7 @@ describe Gitlab::ChatCommands::Command, service: true do
   subject { described_class.new(project, user, params).execute }
 
   describe '#execute' do
-    context 'when no command is not available' do
+    context 'when no command is available' do
       let(:params) { { text: 'issue show 1' } }
       let(:project) { create(:project, has_external_issue_tracker: true) }
 
@@ -45,6 +45,10 @@ describe Gitlab::ChatCommands::Command, service: true do
       it 'presents the issue' do
         expect(subject[:text]).to match("my new issue")
       end
+
+      it 'shows a link to the new issue' do
+        expect(subject[:text]).to match(/\/issues\/\d+/)
+      end
     end
   end
 end
diff --git a/spec/lib/gitlab/chat_commands/issue_create_spec.rb b/spec/lib/gitlab/chat_commands/issue_create_spec.rb
index 4831f24efed..df0c317ccea 100644
--- a/spec/lib/gitlab/chat_commands/issue_create_spec.rb
+++ b/spec/lib/gitlab/chat_commands/issue_create_spec.rb
@@ -34,7 +34,7 @@ describe Gitlab::ChatCommands::IssueCreate, service: true do
     end
   end
 
-  describe 'self.match' do
+  describe '.match' do
     it 'matches the title without description' do
       match = described_class.match("issue create my title")
 
diff --git a/spec/lib/gitlab/import_export/all_models.yml b/spec/lib/gitlab/import_export/all_models.yml
index 02b11bd999a..bc837299b9e 100644
--- a/spec/lib/gitlab/import_export/all_models.yml
+++ b/spec/lib/gitlab/import_export/all_models.yml
@@ -116,6 +116,7 @@ project:
 - base_tags
 - tag_taggings
 - tags
+- chat_services
 - creator
 - group
 - namespace
@@ -127,6 +128,7 @@ project:
 - emails_on_push_service
 - builds_email_service
 - pipelines_email_service
+- mattermost_command_service
 - irker_service
 - pivotaltracker_service
 - hipchat_service
@@ -188,4 +190,4 @@ award_emoji:
 - awardable
 - user
 priorities:
-- label
\ No newline at end of file
+- label
diff --git a/spec/models/project_services/mattermost_command_service_spec.rb b/spec/models/project_services/mattermost_command_service_spec.rb
index 757ad687bc4..21bf1d35e3f 100644
--- a/spec/models/project_services/mattermost_command_service_spec.rb
+++ b/spec/models/project_services/mattermost_command_service_spec.rb
@@ -55,11 +55,11 @@ describe MattermostCommandService, models: true do
         end
 
         context 'when an auth url can be generated' do
-          let(:params) do 
+          let(:params) do
             {
-              team_domain: 'http://domain.tld', 
-              team_id: 'T3423423', 
-              user_id: 'U234234', 
+              team_domain: 'http://domain.tld',
+              team_id: 'T3423423',
+              user_id: 'U234234',
               user_name: 'mepmep',
               token: 'token'
             }
@@ -72,7 +72,7 @@ describe MattermostCommandService, models: true do
           end
 
           it 'generates the url' do
-            response =  service.trigger(params)
+            response = service.trigger(params)
 
             expect(response[:text]).to start_with(':wave: Hi there!')
           end
diff --git a/spec/requests/api/services_spec.rb b/spec/requests/api/services_spec.rb
index fb234ab8ed1..765d662e52b 100644
--- a/spec/requests/api/services_spec.rb
+++ b/spec/requests/api/services_spec.rb
@@ -102,6 +102,8 @@ describe API::API, api: true  do
     end
 
     context 'the service exists' do
+      let(:params) { { token: 'token' } }
+
       context 'the service is not active' do
         let!(:inactive_service) do
           project.create_mattermost_command_service(
@@ -124,7 +126,6 @@ describe API::API, api: true  do
             properties: { token: 'token' }
           )
         end
-        let(:params) { { token: 'token' } }
 
         it 'retusn status 200' do
           post api("/projects/#{project.id}/services/mattermost_command/trigger"), params
@@ -132,6 +133,15 @@ describe API::API, api: true  do
           expect(response).to have_http_status(200)
         end
       end
+
+      context 'when the project can not be found' do
+        it 'returns a generic 404' do
+          post api("/projects/404/services/mattermost_command/trigger"), params
+
+          expect(response).to have_http_status(404)
+          expect(json_response["message"]).to eq '404 Not Found'
+        end
+      end
     end
   end
 end