Merge branch 'slash-commands-changes-for-chatops' into 'master'

Expose ChatName objects to slash commands

See merge request gitlab-org/gitlab-ce!17295

12 files changed, 81 insertions, 24 deletions

diff --git a/app/models/chat_name.rb b/app/models/chat_name.rb
index f321db75eeb..fbd0f123341 100644
--- a/app/models/chat_name.rb
+++ b/app/models/chat_name.rb
@@ -1,4 +1,6 @@
 class ChatName < ActiveRecord::Base
+  LAST_USED_AT_INTERVAL = 1.hour
+
   belongs_to :service
   belongs_to :user
 
@@ -9,4 +11,23 @@ class ChatName < ActiveRecord::Base
 
   validates :user_id, uniqueness: { scope: [:service_id] }
   validates :chat_id, uniqueness: { scope: [:service_id, :team_id] }
+
+  # Updates the "last_used_timestamp" but only if it wasn't already updated
+  # recently.
+  #
+  # The throttling this method uses is put in place to ensure that high chat
+  # traffic doesn't result in many UPDATE queries being performed.
+  def update_last_used_at
+    return unless update_last_used_at?
+
+    obtained = Gitlab::ExclusiveLease
+      .new("chat_name/last_used_at/#{id}", timeout: LAST_USED_AT_INTERVAL.to_i)
+      .try_obtain
+
+    touch(:last_used_at) if obtained
+  end
+
+  def update_last_used_at?
+    last_used_at.nil? || last_used_at > LAST_USED_AT_INTERVAL.ago
+  end
 end
diff --git a/app/models/project_services/slash_commands_service.rb b/app/models/project_services/slash_commands_service.rb
index eb4da68bb7e..37ea45109ae 100644
--- a/app/models/project_services/slash_commands_service.rb
+++ b/app/models/project_services/slash_commands_service.rb
@@ -30,10 +30,10 @@ class SlashCommandsService < Service
   def trigger(params)
     return unless valid_token?(params[:token])
 
-    user = find_chat_user(params)
+    chat_user = find_chat_user(params)
 
-    if user
-      Gitlab::SlashCommands::Command.new(project, user, params).execute
+    if chat_user&.user
+      Gitlab::SlashCommands::Command.new(project, chat_user, params).execute
     else
       url = authorize_chat_name_url(params)
       Gitlab::SlashCommands::Presenters::Access.new(url).authorize
diff --git a/app/services/chat_names/find_user_service.rb b/app/services/chat_names/find_user_service.rb
index 4f5c5567b42..d458b814183 100644
--- a/app/services/chat_names/find_user_service.rb
+++ b/app/services/chat_names/find_user_service.rb
@@ -9,8 +9,8 @@ module ChatNames
       chat_name = find_chat_name
       return unless chat_name
 
-      chat_name.touch(:last_used_at)
-      chat_name.user
+      chat_name.update_last_used_at
+      chat_name
     end
 
     private
diff --git a/lib/gitlab/slash_commands/base_command.rb b/lib/gitlab/slash_commands/base_command.rb
index cc3c9a50555..466554e398c 100644
--- a/lib/gitlab/slash_commands/base_command.rb
+++ b/lib/gitlab/slash_commands/base_command.rb
@@ -31,10 +31,11 @@ module Gitlab
         raise NotImplementedError
       end
 
-      attr_accessor :project, :current_user, :params
+      attr_accessor :project, :current_user, :params, :chat_name
 
-      def initialize(project, user, params = {})
-        @project, @current_user, @params = project, user, params.dup
+      def initialize(project, chat_name, params = {})
+        @project, @current_user, @params = project, chat_name.user, params.dup
+        @chat_name = chat_name
       end
 
       private
diff --git a/lib/gitlab/slash_commands/command.rb b/lib/gitlab/slash_commands/command.rb
index a78408b0519..85aaa6b0eba 100644
--- a/lib/gitlab/slash_commands/command.rb
+++ b/lib/gitlab/slash_commands/command.rb
@@ -13,12 +13,13 @@ module Gitlab
 
         if command
           if command.allowed?(project, current_user)
-            command.new(project, current_user, params).execute(match)
+            command.new(project, chat_name, params).execute(match)
           else
             Gitlab::SlashCommands::Presenters::Access.new.access_denied
           end
         else
-          Gitlab::SlashCommands::Help.new(project, current_user, params).execute(available_commands, params[:text])
+          Gitlab::SlashCommands::Help.new(project, chat_name, params)
+            .execute(available_commands, params[:text])
         end
       end
 
diff --git a/spec/lib/gitlab/slash_commands/command_spec.rb b/spec/lib/gitlab/slash_commands/command_spec.rb
index 0173a45d480..e3447d974aa 100644
--- a/spec/lib/gitlab/slash_commands/command_spec.rb
+++ b/spec/lib/gitlab/slash_commands/command_spec.rb
@@ -3,10 +3,11 @@ require 'spec_helper'
 describe Gitlab::SlashCommands::Command do
   let(:project) { create(:project) }
   let(:user) { create(:user) }
+  let(:chat_name) { double(:chat_name, user: user) }
 
   describe '#execute' do
     subject do
-      described_class.new(project, user, params).execute
+      described_class.new(project, chat_name, params).execute
     end
 
     context 'when no command is available' do
@@ -88,7 +89,7 @@ describe Gitlab::SlashCommands::Command do
   end
 
   describe '#match_command' do
-    subject { described_class.new(project, user, params).match_command.first }
+    subject { described_class.new(project, chat_name, params).match_command.first }
 
     context 'IssueShow is triggered' do
       let(:params) { { text: 'issue show 123' } }
diff --git a/spec/lib/gitlab/slash_commands/deploy_spec.rb b/spec/lib/gitlab/slash_commands/deploy_spec.rb
index 74b5ef4bb26..0d57334aa4c 100644
--- a/spec/lib/gitlab/slash_commands/deploy_spec.rb
+++ b/spec/lib/gitlab/slash_commands/deploy_spec.rb
@@ -4,6 +4,7 @@ describe Gitlab::SlashCommands::Deploy do
   describe '#execute' do
     let(:project) { create(:project) }
     let(:user) { create(:user) }
+    let(:chat_name) { double(:chat_name, user: user) }
     let(:regex_match) { described_class.match('deploy staging to production') }
 
     before do
@@ -16,7 +17,7 @@ describe Gitlab::SlashCommands::Deploy do
     end
 
     subject do
-      described_class.new(project, user).execute(regex_match)
+      described_class.new(project, chat_name).execute(regex_match)
     end
 
     context 'if no environment is defined' do
diff --git a/spec/lib/gitlab/slash_commands/issue_new_spec.rb b/spec/lib/gitlab/slash_commands/issue_new_spec.rb
index 3b077c58c50..8e7df946529 100644
--- a/spec/lib/gitlab/slash_commands/issue_new_spec.rb
+++ b/spec/lib/gitlab/slash_commands/issue_new_spec.rb
@@ -4,6 +4,7 @@ describe Gitlab::SlashCommands::IssueNew do
   describe '#execute' do
     let(:project) { create(:project) }
     let(:user) { create(:user) }
+    let(:chat_name) { double(:chat_name, user: user) }
     let(:regex_match) { described_class.match("issue create bird is the word") }
 
     before do
@@ -11,7 +12,7 @@ describe Gitlab::SlashCommands::IssueNew do
     end
 
     subject do
-      described_class.new(project, user).execute(regex_match)
+      described_class.new(project, chat_name).execute(regex_match)
     end
 
     context 'without description' do
diff --git a/spec/lib/gitlab/slash_commands/issue_search_spec.rb b/spec/lib/gitlab/slash_commands/issue_search_spec.rb
index 35d01efc1bd..189e9592f1b 100644
--- a/spec/lib/gitlab/slash_commands/issue_search_spec.rb
+++ b/spec/lib/gitlab/slash_commands/issue_search_spec.rb
@@ -6,10 +6,11 @@ describe Gitlab::SlashCommands::IssueSearch do
     let!(:confidential) { create(:issue, :confidential, project: project, title: 'mepmep find') }
     let(:project) { create(:project) }
     let(:user) { create(:user) }
+    let(:chat_name) { double(:chat_name, user: user) }
     let(:regex_match) { described_class.match("issue search find") }
 
     subject do
-      described_class.new(project, user).execute(regex_match)
+      described_class.new(project, chat_name).execute(regex_match)
     end
 
     context 'when the user has no access' do
diff --git a/spec/lib/gitlab/slash_commands/issue_show_spec.rb b/spec/lib/gitlab/slash_commands/issue_show_spec.rb
index e5834d5a2ee..b1db1638237 100644
--- a/spec/lib/gitlab/slash_commands/issue_show_spec.rb
+++ b/spec/lib/gitlab/slash_commands/issue_show_spec.rb
@@ -5,6 +5,7 @@ describe Gitlab::SlashCommands::IssueShow do
     let(:issue) { create(:issue, project: project) }
     let(:project) { create(:project) }
     let(:user) { issue.author }
+    let(:chat_name) { double(:chat_name, user: user) }
     let(:regex_match) { described_class.match("issue show #{issue.iid}") }
 
     before do
@@ -12,7 +13,7 @@ describe Gitlab::SlashCommands::IssueShow do
     end
 
     subject do
-      described_class.new(project, user).execute(regex_match)
+      described_class.new(project, chat_name).execute(regex_match)
     end
 
     context 'the issue exists' do
diff --git a/spec/models/chat_name_spec.rb b/spec/models/chat_name_spec.rb
index e89e534d914..504bc710b25 100644
--- a/spec/models/chat_name_spec.rb
+++ b/spec/models/chat_name_spec.rb
@@ -14,4 +14,24 @@ describe ChatName do
 
   it { is_expected.to validate_uniqueness_of(:user_id).scoped_to(:service_id) }
   it { is_expected.to validate_uniqueness_of(:chat_id).scoped_to(:service_id, :team_id) }
+
+  describe '#update_last_used_at', :clean_gitlab_redis_shared_state do
+    it 'updates the last_used_at timestamp' do
+      expect(subject.last_used_at).to be_nil
+
+      subject.update_last_used_at
+
+      expect(subject.last_used_at).to be_present
+    end
+
+    it 'does not update last_used_at if it was recently updated' do
+      subject.update_last_used_at
+
+      time = subject.last_used_at
+
+      subject.update_last_used_at
+
+      expect(subject.last_used_at).to eq(time)
+    end
+  end
 end
diff --git a/spec/services/chat_names/find_user_service_spec.rb b/spec/services/chat_names/find_user_service_spec.rb
index 79aaac3aeb6..5734b10109a 100644
--- a/spec/services/chat_names/find_user_service_spec.rb
+++ b/spec/services/chat_names/find_user_service_spec.rb
@@ -1,6 +1,6 @@
 require 'spec_helper'
 
-describe ChatNames::FindUserService do
+describe ChatNames::FindUserService, :clean_gitlab_redis_shared_state do
   describe '#execute' do
     let(:service) { create(:service) }
 
@@ -13,21 +13,30 @@ describe ChatNames::FindUserService do
       context 'when existing user is requested' do
         let(:params) { { team_id: chat_name.team_id, user_id: chat_name.chat_id } }
 
-        it 'returns the existing user' do
-          is_expected.to eq(user)
+        it 'returns the existing chat_name' do
+          is_expected.to eq(chat_name)
         end
 
-        it 'updates when last time chat name was used' do
+        it 'updates the last used timestamp if one is not already set' do
           expect(chat_name.last_used_at).to be_nil
 
           subject
 
-          initial_last_used = chat_name.reload.last_used_at
-          expect(initial_last_used).to be_present
+          expect(chat_name.reload.last_used_at).to be_present
+        end
+
+        it 'only updates an existing timestamp once within a certain time frame' do
+          service = described_class.new(service, params)
+
+          expect(chat_name.last_used_at).to be_nil
+
+          service.execute
+
+          time = chat_name.reload.last_used_at
 
-          Timecop.travel(2.days.from_now) { described_class.new(service, params).execute }
+          service.execute
 
-          expect(chat_name.reload.last_used_at).to be > initial_last_used
+          expect(chat_name.reload.last_used_at).to eq(time)
         end
       end