Incorporate feedback

changelogs/unreleased/zj-format-chat-messages.yml

+---
+title: Reformat messages ChatOps
+merge_request: 8528
+author:

lib/gitlab/chat_commands/issue_create.rb

@@ -35,7 +35,7 @@ module Gitlab
       end
 
       def presenter(issue)
-        Gitlab::ChatCommands::Presenters::ShowIssue.new(issue)
+        Gitlab::ChatCommands::Presenters::NewIssue.new(issue)
       end
     end
   end

lib/gitlab/chat_commands/presenters/access.rb

-module Gitlab::ChatCommands::Presenters
-  class Access < Gitlab::ChatCommands::Presenters::Base
+module Gitlab
+  module ChatCommands
+    module Presenters
+      class Access < Presenters::Base
         def access_denied
           ephemeral_response(text: "Whoops! This action is not allowed. This incident will be [reported](https://xkcd.com/838/).")
         end
@@ -19,4 +21,6 @@ module Gitlab::ChatCommands::Presenters
           ephemeral_response(text: message)
         end
       end
+    end
+  end
 end

lib/gitlab/chat_commands/presenters/base.rb

-module Gitlab::ChatCommands::Presenters
+module Gitlab
+  module ChatCommands
+    module Presenters
       class Base
         include Gitlab::Routing.url_helpers
 
@@ -70,4 +72,6 @@ module Gitlab::ChatCommands::Presenters
           )
         end
       end
+    end
+  end
 end

lib/gitlab/chat_commands/presenters/deploy.rb

-module Gitlab::ChatCommands::Presenters
-  class Deploy < Gitlab::ChatCommands::Presenters::Base
+module Gitlab
+  module ChatCommands
+    module Presenters
+      class Deploy < Presenters::Base
         def present(from, to)
           message = "Deployment started from #{from} to #{to}. [Follow its progress](#{resource_url})."
+
           in_channel_response(text: message)
         end
 
@@ -21,4 +24,6 @@ module Gitlab::ChatCommands::Presenters
           )
         end
       end
+    end
+  end
 end

lib/gitlab/chat_commands/presenters/help.rb

-module Gitlab::ChatCommands::Presenters
-  class Help < Gitlab::ChatCommands::Presenters::Base
+module Gitlab
+  module ChatCommands
+    module Presenters
+      class Help < Presenters::Base
         def present(trigger)
-      message =
+          ephemeral_response(text: help_message(trigger))
+        end
+
+        private
+
+        def help_message(trigger)
           if @resource.none?
             "No commands available :thinking_face:"
           else
             header_with_list("Available commands", full_commands(trigger))
           end
-
-      ephemeral_response(text: message)
         end
 
-    private
-
         def full_commands(trigger)
           @resource.map { |command| "#{trigger} #{command.help_message}" }
         end
       end
+    end
+  end
 end

lib/gitlab/chat_commands/presenters/issuable.rb

-module Gitlab::ChatCommands::Presenters
-  class Issuable < Gitlab::ChatCommands::Presenters::Base
+module Gitlab
+  module ChatCommands
+    module Presenters
+      class Issuable < Presenters::Base
         private
 
+        def color(issuable)
+          issuable.open? ? '#38ae67' : '#d22852'
+        end
+
+        def status_text(issuable)
+          issuable.open? ? 'Open' : 'Closed'
+        end
+
         def project
           @resource.project
         end
@@ -30,4 +40,6 @@ module Gitlab::ChatCommands::Presenters
           ]
         end
       end
+    end
+  end
 end

lib/gitlab/chat_commands/presenters/list_issues.rb

-module Gitlab::ChatCommands::Presenters
-  class ListIssues < Gitlab::ChatCommands::Presenters::Base
+module Gitlab
+  module ChatCommands
+    module Presenters
+      class ListIssues < Presenters::Issuable
         def present
-      ephemeral_response(text: "Here are the issues I found:", attachments: attachments)
+          text = if @resource.count >= 5
+                   "Here are the first 5 issues I found:"
+                 else
+                   "Here are the #{@resource.count} issues I found:"
+                 end
+
+          ephemeral_response(text: text, attachments: attachments)
         end
 
         private
 
         def attachments
           @resource.map do |issue|
-        state = issue.open? ? "Open" : "Closed"
+            url = "[#{issue.to_reference}](#{url_for([namespace, project, issue])})"
 
             {
-          fallback: "Issue #{issue.to_reference}: #{issue.title}",
-          color: "#d22852",
-          text: "[#{issue.to_reference}](#{url_for([namespace, project, issue])}) · #{issue.title} (#{state})",
+              color: color(issue),
+              fallback: "#{issue.to_reference} #{issue.title}",
+              text: "#{url} · #{issue.title} (#{status_text(issue)})",
+
               mrkdwn_in: [
                 "text"
               ]
@@ -29,4 +38,6 @@ module Gitlab::ChatCommands::Presenters
           @namespace ||= project.namespace.becomes(Namespace)
         end
       end
+    end
+  end
 end

lib/gitlab/chat_commands/presenters/new_issue.rb

+module Gitlab
+  module ChatCommands
+    module Presenters
+      class NewIssue < Presenters::Issuable
+        def present
+          in_channel_response(show_issue)
+        end
+
+        private
+
+        def show_issue
+          {
+            attachments: [
+              {
+                title:        "#{@resource.title} · #{@resource.to_reference}",
+                title_link:   resource_url,
+                author_name:  author.name,
+                author_icon:  author.avatar_url,
+                fallback:     "New issue #{@resource.to_reference}: #{@resource.title}",
+                pretext:      pretext,
+                color:        color(@resource),
+                fields:       fields,
+                mrkdwn_in: [
+                  :title,
+                  :text
+                ]
+              }
+            ]
+          }
+        end
+
+        def pretext
+          "I opened an issue on behalf on #{author_profile_link}: *#{@resource.to_reference}* from #{project.name_with_namespace}"
+        end
+
+        def author_profile_link
+          "[#{author.to_reference}](#{url_for(author)})"
+        end
+      end
+    end
+  end
+end

lib/gitlab/chat_commands/presenters/show_issue.rb

-module Gitlab::ChatCommands::Presenters
-  class ShowIssue < Gitlab::ChatCommands::Presenters::Issuable
+module Gitlab
+  module ChatCommands
+    module Presenters
+      class ShowIssue < Presenters::Issuable
         def present
           in_channel_response(show_issue)
         end
@@ -10,15 +12,17 @@ module Gitlab::ChatCommands::Presenters
           {
             attachments: [
               {
-            title:        @resource.title,
+                title:        "#{@resource.title} · #{@resource.to_reference}",
                 title_link:   resource_url,
                 author_name:  author.name,
                 author_icon:  author.avatar_url,
-            fallback:     "#{@resource.to_reference}: #{@resource.title}",
+                fallback:     "New issue #{@resource.to_reference}: #{@resource.title}",
+                pretext:      pretext,
                 text:         text,
+                color:        color(@resource),
                 fields:       fields,
                 mrkdwn_in: [
-              :title,
+                  :pretext,
                   :text
                 ]
               }
@@ -27,12 +31,24 @@ module Gitlab::ChatCommands::Presenters
         end
 
         def text
-      message = ""
+          message = "**#{status_text(@resource)}**"
+
+          if @resource.upvotes.zero? && @resource.downvotes.zero? && @resource.user_notes_count.zero?
+            return message
+          end
+
+          message << " · "
           message << ":+1: #{@resource.upvotes} " unless @resource.upvotes.zero?
           message << ":-1: #{@resource.downvotes} " unless @resource.downvotes.zero?
           message << ":speech_balloon: #{@resource.user_notes_count}" unless @resource.user_notes_count.zero?
 
           message
         end
+
+        def pretext
+          "Issue *#{@resource.to_reference} from #{project.name_with_namespace}"
+        end
+      end
+    end
   end
 end

spec/lib/gitlab/chat_commands/issue_search_spec.rb

@@ -26,7 +26,7 @@ describe Gitlab::ChatCommands::IssueSearch, service: true do
 
       it 'returns all results' do
         expect(subject).to have_key(:attachments)
-        expect(subject[:text]).to match("Here are the issues I found:")
+        expect(subject[:text]).to eq("Here are the 2 issues I found:")
       end
     end
 

spec/lib/gitlab/chat_commands/issue_show_spec.rb

@@ -20,7 +20,7 @@ describe Gitlab::ChatCommands::IssueShow, service: true do
 
       it 'returns the issue' do
         expect(subject[:response_type]).to be(:in_channel)
-        expect(title).to eq(issue.title)
+        expect(title).to start_with(issue.title)
       end
 
       context 'when its reference is given' do
@@ -28,7 +28,7 @@ describe Gitlab::ChatCommands::IssueShow, service: true do
 
         it 'shows the issue' do
           expect(subject[:response_type]).to be(:in_channel)
-          expect(title).to eq(issue.title)
+          expect(title).to start_with(issue.title)
         end
       end
     end

spec/lib/gitlab/chat_commands/presenters/deploy_spec.rb

@@ -32,7 +32,7 @@ describe Gitlab::ChatCommands::Presenters::Deploy do
   end
 
   describe '#too_many_actions' do
-    subject { described_class.new(nil).too_many_actions }
+    subject { described_class.new([]).too_many_actions }
 
     it { is_expected.to have_key(:text) }
     it { is_expected.to have_key(:response_type) }

spec/lib/gitlab/chat_commands/presenters/list_issues_spec.rb

@@ -3,13 +3,12 @@ require 'spec_helper'
 describe Gitlab::ChatCommands::Presenters::ListIssues do
   let(:project) { create(:empty_project) }
   let(:message) { subject[:text] }
-  let(:issue) { project.issues.first }
 
   before { create_list(:issue, 2, project: project) }
 
   subject { described_class.new(project.issues).present }
 
-  it do
+  it 'formats the message correct' do
     is_expected.to have_key(:text)
     is_expected.to have_key(:status)
     is_expected.to have_key(:response_type)
@@ -19,6 +18,6 @@ describe Gitlab::ChatCommands::Presenters::ListIssues do
   it 'shows a list of results' do
     expect(subject[:response_type]).to be(:ephemeral)
 
-    expect(message).to start_with("Here are the issues I found")
+    expect(message).to start_with("Here are the 2 issues I found")
   end
 end

spec/lib/gitlab/chat_commands/presenters/show_issue_spec.rb

@@ -12,7 +12,7 @@ describe Gitlab::ChatCommands::Presenters::ShowIssue do
   it 'shows the issue' do
     expect(subject[:response_type]).to be(:in_channel)
     expect(subject).to have_key(:attachments)
-    expect(attachment[:title]).to eq(issue.title)
+    expect(attachment[:title]).to start_with(issue.title)
   end
 
   context 'with upvotes' do
@@ -21,7 +21,7 @@ describe Gitlab::ChatCommands::Presenters::ShowIssue do
     end
 
     it 'shows the upvote count' do
-      expect(attachment[:text]).to start_with(":+1: 1")
+      expect(attachment[:text]).to start_with("**Open** · :+1: 1")
     end
   end
 end

spec/lib/mattermost/client_spec.rb

+require 'spec_helper'
+
+describe Mattermost::Client do
+  let(:user) { build(:user) }
+
+  subject { described_class.new(user) }
+
+  context 'JSON parse error' do
+    before do
+      Struct.new("Request", :body, :success?)
+    end
+
+    it 'yields an error on malformed JSON' do
+      bad_json = Struct::Request.new("I'm not json", true)
+      expect { subject.send(:json_response, bad_json) }.to raise_error(Mattermost::ClientError)
+    end
+
+    it 'shows a client error if the request was unsuccessful' do
+      bad_request = Struct::Request.new("true", false)
+
+      expect { subject.send(:json_response, bad_request) }.to raise_error(Mattermost::ClientError)
+    end
+  end
+end

spec/lib/mattermost/command_spec.rb

+require 'spec_helper'
+
+describe Mattermost::Command do
+  let(:params) { { 'token' => 'token', team_id: 'abc' } }
+
+  before do
+    Mattermost::Session.base_uri('http://mattermost.example.com')
+
+    allow_any_instance_of(Mattermost::Client).to receive(:with_session).
+      and_yield(Mattermost::Session.new(nil))
+  end
+
+  describe '#create' do
+    let(:params) do
+      { team_id: 'abc',
+        trigger: 'gitlab'
+      }
+    end
+
+    subject { described_class.new(nil).create(params) }
+
+    context 'for valid trigger word' do
+      before do
+        stub_request(:post, 'http://mattermost.example.com/api/v3/teams/abc/commands/create').
+          with(body: {
+            team_id: 'abc',
+            trigger: 'gitlab' }.to_json).
+          to_return(
+            status: 200,
+            headers: { 'Content-Type' => 'application/json' },
+            body: { token: 'token' }.to_json
+          )
+      end
+
+      it 'returns a token' do
+        is_expected.to eq('token')
+      end
+    end
+
+    context 'for error message' do
+      before do
+        stub_request(:post, 'http://mattermost.example.com/api/v3/teams/abc/commands/create').
+          to_return(
+            status: 500,
+            headers: { 'Content-Type' => 'application/json' },
+            body: {
+              id: 'api.command.duplicate_trigger.app_error',
+              message: 'This trigger word is already in use. Please choose another word.',
+              detailed_error: '',
+              request_id: 'obc374man7bx5r3dbc1q5qhf3r',
+              status_code: 500
+            }.to_json
+          )
+      end
+
+      it 'raises an error with message' do
+        expect { subject }.to raise_error(Mattermost::Error, 'This trigger word is already in use. Please choose another word.')
+      end
+    end
+  end
+end

spec/lib/mattermost/session_spec.rb

+require 'spec_helper'
+
+describe Mattermost::Session, type: :request do
+  let(:user) { create(:user) }
+
+  let(:gitlab_url) { "http://gitlab.com" }
+  let(:mattermost_url) { "http://mattermost.com" }
+
+  subject { described_class.new(user) }
+
+  # Needed for doorkeeper to function
+  it { is_expected.to respond_to(:current_resource_owner) }
+  it { is_expected.to respond_to(:request) }
+  it { is_expected.to respond_to(:authorization) }
+  it { is_expected.to respond_to(:strategy) }
+
+  before do
+    described_class.base_uri(mattermost_url)
+  end
+
+  describe '#with session' do
+    let(:location) { 'http://location.tld' }
+    let!(:stub) do
+      WebMock.stub_request(:get, "#{mattermost_url}/api/v3/oauth/gitlab/login").
+        to_return(headers: { 'location' => location }, status: 307)
+    end
+
+    context 'without oauth uri' do
+      it 'makes a request to the oauth uri' do
+        expect { subject.with_session }.to raise_error(Mattermost::NoSessionError)
+      end
+    end
+
+    context 'with oauth_uri' do
+      let!(:doorkeeper) do
+        Doorkeeper::Application.create(
+          name: "GitLab Mattermost",
+          redirect_uri: "#{mattermost_url}/signup/gitlab/complete\n#{mattermost_url}/login/gitlab/complete",
+          scopes: "")
+      end
+
+      context 'without token_uri' do
+        it 'can not create a session' do
+          expect do
+            subject.with_session
+          end.to raise_error(Mattermost::NoSessionError)
+        end
+      end
+
+      context 'with token_uri' do
+        let(:state) { "state" }
+        let(:params) do
+          { response_type: "code",
+            client_id: doorkeeper.uid,
+            redirect_uri: "#{mattermost_url}/signup/gitlab/complete",
+            state: state }
+        end
+        let(:location) do
+          "#{gitlab_url}/oauth/authorize?#{URI.encode_www_form(params)}"
+        end
+
+        before do
+          WebMock.stub_request(:get, "#{mattermost_url}/signup/gitlab/complete").
+            with(query: hash_including({ 'state' => state })).
+            to_return do |request|
+              post "/oauth/token",
+                client_id: doorkeeper.uid,
+                client_secret: doorkeeper.secret,
+                redirect_uri: params[:redirect_uri],
+                grant_type: 'authorization_code',
+                code: request.uri.query_values['code']
+
+              if response.status == 200
+                { headers: { 'token' => 'thisworksnow' }, status: 202 }
+              end
+            end
+
+          WebMock.stub_request(:post, "#{mattermost_url}/api/v3/users/logout").
+            to_return(headers: { Authorization: 'token thisworksnow' }, status: 200)
+        end
+
+        it 'can setup a session' do
+          subject.with_session do |session|
+          end
+
+          expect(subject.token).not_to be_nil
+        end
+
+        it 'returns the value of the block' do
+          result = subject.with_session do |session|
+            "value"
+          end
+
+          expect(result).to eq("value")
+        end
+      end
+    end
+
+    context 'with lease' do
+      before do
+        allow(subject).to receive(:lease_try_obtain).and_return('aldkfjsldfk')
+      end
+
+      it 'tries to obtain a lease' do
+        expect(subject).to receive(:lease_try_obtain)
+        expect(Gitlab::ExclusiveLease).to receive(:cancel)
+
+        # Cannot setup a session, but we should still cancel the lease
+        expect { subject.with_session }.to raise_error(Mattermost::NoSessionError)
+      end
+    end
+
+    context 'without lease' do
+      before do
+        allow(subject).to receive(:lease_try_obtain).and_return(nil)
+      end
+
+      it 'returns a NoSessionError error' do
+        expect { subject.with_session }.to raise_error(Mattermost::NoSessionError)
+      end
+    end
+  end
+end

spec/lib/mattermost/team_spec.rb

+require 'spec_helper'
+
+describe Mattermost::Team do
+  before do
+    Mattermost::Session.base_uri('http://mattermost.example.com')
+
+    allow_any_instance_of(Mattermost::Client).to receive(:with_session).
+      and_yield(Mattermost::Session.new(nil))
+  end
+
+  describe '#all' do
+    subject { described_class.new(nil).all }
+
+    context 'for valid request' do
+      let(:response) do
+        [{
+           "id" => "xiyro8huptfhdndadpz8r3wnbo",
+           "create_at" => 1482174222155,
+           "update_at" => 1482174222155,
+           "delete_at" => 0,
+           "display_name" => "chatops",
+           "name" => "chatops",
+           "email" => "admin@example.com",
+           "type" => "O",
+           "company_name" => "",
+           "allowed_domains" => "",
+           "invite_id" => "o4utakb9jtb7imctdfzbf9r5ro",
+           "allow_open_invite" => false }]
+      end
+
+      before do
+        stub_request(:get, 'http://mattermost.example.com/api/v3/teams/all').
+          to_return(
+            status: 200,
+            headers: { 'Content-Type' => 'application/json' },
+            body: response.to_json
+          )
+      end
+
+      it 'returns a token' do
+        is_expected.to eq(response)
+      end
+    end
+
+    context 'for error message' do
+      before do
+        stub_request(:get, 'http://mattermost.example.com/api/v3/teams/all').
+          to_return(
+            status: 500,
+            headers: { 'Content-Type' => 'application/json' },
+            body: {
+              id: 'api.team.list.app_error',
+              message: 'Cannot list teams.',
+              detailed_error: '',
+              request_id: 'obc374man7bx5r3dbc1q5qhf3r',
+              status_code: 500
+            }.to_json
+          )
+      end
+
+      it 'raises an error with message' do
+        expect { subject }.to raise_error(Mattermost::Error, 'Cannot list teams.')
+      end
+    end
+  end
+end