Commit 65349c22 authored by Rémy Coutable's avatar Rémy Coutable

Make slash commands contextual

- Return only slash commands that make sense for the current noteable
- Allow slash commands decription to be dynamic

Other improvements:

- Add permission checks in slash commands definition
- Use IssuesFinder and MergeRequestsFinder
- Use next if instead of a unless block, and use splat operator instead of flatten
Signed-off-by: 's avatarRémy Coutable <remy@rymai.me>
parent 23db6449
...@@ -209,7 +209,8 @@ gem 'mousetrap-rails', '~> 1.4.6' ...@@ -209,7 +209,8 @@ gem 'mousetrap-rails', '~> 1.4.6'
# Detect and convert string character encoding # Detect and convert string character encoding
gem 'charlock_holmes', '~> 0.7.3' gem 'charlock_holmes', '~> 0.7.3'
# Parse duration # Parse time & duration
gem 'chronic', '~> 0.10.2'
gem 'chronic_duration', '~> 0.10.6' gem 'chronic_duration', '~> 0.10.6'
gem 'sass-rails', '~> 5.0.0' gem 'sass-rails', '~> 5.0.0'
......
...@@ -128,6 +128,7 @@ GEM ...@@ -128,6 +128,7 @@ GEM
mime-types (>= 1.16) mime-types (>= 1.16)
cause (0.1) cause (0.1)
charlock_holmes (0.7.3) charlock_holmes (0.7.3)
chronic (0.10.2)
chronic_duration (0.10.6) chronic_duration (0.10.6)
numerizer (~> 0.1.1) numerizer (~> 0.1.1)
chunky_png (1.3.5) chunky_png (1.3.5)
...@@ -822,6 +823,7 @@ DEPENDENCIES ...@@ -822,6 +823,7 @@ DEPENDENCIES
capybara-screenshot (~> 1.0.0) capybara-screenshot (~> 1.0.0)
carrierwave (~> 0.10.0) carrierwave (~> 0.10.0)
charlock_holmes (~> 0.7.3) charlock_holmes (~> 0.7.3)
chronic (~> 0.10.2)
chronic_duration (~> 0.10.6) chronic_duration (~> 0.10.6)
coffee-rails (~> 4.1.0) coffee-rails (~> 4.1.0)
connection_pool (~> 2.0) connection_pool (~> 2.0)
......
...@@ -146,7 +146,7 @@ class ProjectsController < Projects::ApplicationController ...@@ -146,7 +146,7 @@ class ProjectsController < Projects::ApplicationController
mergerequests: autocomplete.merge_requests, mergerequests: autocomplete.merge_requests,
labels: autocomplete.labels, labels: autocomplete.labels,
members: participants, members: participants,
commands: autocomplete.commands commands: autocomplete.commands(note_type, note_id)
} }
respond_to do |format| respond_to do |format|
......
...@@ -16,8 +16,27 @@ module Projects ...@@ -16,8 +16,27 @@ module Projects
@project.labels.select([:title, :color]) @project.labels.select([:title, :color])
end end
def commands def commands(noteable_type, noteable_id)
SlashCommands::InterpretService.command_definitions SlashCommands::InterpretService.command_definitions(
project: @project,
noteable: command_target(noteable_type, noteable_id),
current_user: current_user
)
end
private
def command_target(noteable_type, noteable_id)
case noteable_type
when 'Issue'
IssuesFinder.new(current_user, project_id: @project.id, state: 'all').
execute.find_or_initialize_by(iid: noteable_id)
when 'MergeRequest'
MergeRequestsFinder.new(current_user, project_id: @project.id, state: 'all').
execute.find_or_initialize_by(iid: noteable_id)
else
nil
end
end end
end end
end end
...@@ -10,7 +10,7 @@ module SlashCommands ...@@ -10,7 +10,7 @@ module SlashCommands
@noteable = noteable @noteable = noteable
@updates = {} @updates = {}
commands = extractor.extract_commands!(content) commands = extractor(noteable: noteable).extract_commands!(content)
commands.each do |command| commands.each do |command|
__send__(*command) __send__(*command)
end end
...@@ -20,28 +20,57 @@ module SlashCommands ...@@ -20,28 +20,57 @@ module SlashCommands
private private
def extractor def extractor(opts = {})
@extractor ||= Gitlab::SlashCommands::Extractor.new(self.class.command_names) opts.merge!(current_user: current_user, project: project)
Gitlab::SlashCommands::Extractor.new(self.class.command_names(opts))
end end
desc 'Close this issue or merge request' desc ->(opts) { "Close this #{opts[:noteable].to_ability_name.humanize(capitalize: false)}" }
condition ->(opts) do
opts[:noteable] &&
opts[:noteable].open? &&
opts[:current_user] &&
opts[:project] &&
opts[:current_user].can?(:"update_#{opts[:noteable].to_ability_name}", opts[:project])
end
command :close do command :close do
@updates[:state_event] = 'close' @updates[:state_event] = 'close'
end end
desc 'Reopen this issue or merge request' desc ->(opts) { "Reopen this #{opts[:noteable].to_ability_name.humanize(capitalize: false)}" }
condition ->(opts) do
opts[:noteable] &&
opts[:noteable].closed? &&
opts[:current_user] &&
opts[:project] &&
opts[:current_user].can?(:"update_#{opts[:noteable].to_ability_name}", opts[:project])
end
command :open, :reopen do command :open, :reopen do
@updates[:state_event] = 'reopen' @updates[:state_event] = 'reopen'
end end
desc 'Change title' desc 'Change title'
params '<New title>' params '<New title>'
condition ->(opts) do
opts[:noteable] &&
opts[:noteable].persisted? &&
opts[:current_user] &&
opts[:project] &&
opts[:current_user].can?(:"update_#{opts[:noteable].to_ability_name}", opts[:project])
end
command :title do |title_param| command :title do |title_param|
@updates[:title] = title_param @updates[:title] = title_param
end end
desc 'Assign' desc 'Assign'
params '@user' params '@user'
condition ->(opts) do
opts[:noteable] &&
opts[:current_user] &&
opts[:project] &&
opts[:current_user].can?(:"admin_#{opts[:noteable].to_ability_name}", opts[:project])
end
command :assign, :reassign do |assignee_param| command :assign, :reassign do |assignee_param|
user = extract_references(assignee_param, :user).first user = extract_references(assignee_param, :user).first
return unless user return unless user
...@@ -50,12 +79,26 @@ module SlashCommands ...@@ -50,12 +79,26 @@ module SlashCommands
end end
desc 'Remove assignee' desc 'Remove assignee'
condition ->(opts) do
opts[:noteable] &&
opts[:noteable].assignee_id? &&
opts[:current_user] &&
opts[:project] &&
opts[:current_user].can?(:"admin_#{opts[:noteable].to_ability_name}", opts[:project])
end
command :unassign, :remove_assignee do command :unassign, :remove_assignee do
@updates[:assignee_id] = nil @updates[:assignee_id] = nil
end end
desc 'Set milestone' desc 'Set milestone'
params '%"milestone"' params '%"milestone"'
condition ->(opts) do
opts[:noteable] &&
opts[:current_user] &&
opts[:project] &&
opts[:current_user].can?(:"admin_#{opts[:noteable].to_ability_name}", opts[:project]) &&
opts[:project].milestones.active.any?
end
command :milestone do |milestone_param| command :milestone do |milestone_param|
milestone = extract_references(milestone_param, :milestone).first milestone = extract_references(milestone_param, :milestone).first
return unless milestone return unless milestone
...@@ -64,12 +107,26 @@ module SlashCommands ...@@ -64,12 +107,26 @@ module SlashCommands
end end
desc 'Remove milestone' desc 'Remove milestone'
condition ->(opts) do
opts[:noteable] &&
opts[:noteable].milestone_id? &&
opts[:current_user] &&
opts[:project] &&
opts[:current_user].can?(:"admin_#{opts[:noteable].to_ability_name}", opts[:project])
end
command :clear_milestone, :remove_milestone do command :clear_milestone, :remove_milestone do
@updates[:milestone_id] = nil @updates[:milestone_id] = nil
end end
desc 'Add label(s)' desc 'Add label(s)'
params '~label1 ~"label 2"' params '~label1 ~"label 2"'
condition ->(opts) do
opts[:noteable] &&
opts[:current_user] &&
opts[:project] &&
opts[:current_user].can?(:"admin_#{opts[:noteable].to_ability_name}", opts[:project]) &&
opts[:project].labels.any?
end
command :label, :labels do |labels_param| command :label, :labels do |labels_param|
label_ids = find_label_ids(labels_param) label_ids = find_label_ids(labels_param)
return if label_ids.empty? return if label_ids.empty?
...@@ -79,6 +136,13 @@ module SlashCommands ...@@ -79,6 +136,13 @@ module SlashCommands
desc 'Remove label(s)' desc 'Remove label(s)'
params '~label1 ~"label 2"' params '~label1 ~"label 2"'
condition ->(opts) do
opts[:noteable] &&
opts[:noteable].labels.any? &&
opts[:current_user] &&
opts[:project] &&
opts[:current_user].can?(:"admin_#{opts[:noteable].to_ability_name}", opts[:project])
end
command :unlabel, :remove_label, :remove_labels do |labels_param| command :unlabel, :remove_label, :remove_labels do |labels_param|
label_ids = find_label_ids(labels_param) label_ids = find_label_ids(labels_param)
return if label_ids.empty? return if label_ids.empty?
...@@ -87,52 +151,85 @@ module SlashCommands ...@@ -87,52 +151,85 @@ module SlashCommands
end end
desc 'Remove all labels' desc 'Remove all labels'
condition ->(opts) do
opts[:noteable] &&
opts[:noteable].labels.any? &&
opts[:current_user] &&
opts[:project] &&
opts[:current_user].can?(:"admin_#{opts[:noteable].to_ability_name}", opts[:project])
end
command :clear_labels, :clear_label do command :clear_labels, :clear_label do
@updates[:label_ids] = [] @updates[:label_ids] = []
end end
desc 'Add a todo' desc 'Add a todo'
condition ->(opts) do
opts[:noteable] &&
opts[:noteable].persisted? &&
opts[:current_user] &&
!TodosFinder.new(opts[:current_user]).execute.exists?(target: opts[:noteable])
end
command :todo do command :todo do
@updates[:todo_event] = 'add' @updates[:todo_event] = 'add'
end end
desc 'Mark todo as done' desc 'Mark todo as done'
condition ->(opts) do
opts[:noteable] &&
opts[:current_user] &&
TodosFinder.new(opts[:current_user]).execute.exists?(target: opts[:noteable])
end
command :done do command :done do
@updates[:todo_event] = 'done' @updates[:todo_event] = 'done'
end end
desc 'Subscribe' desc 'Subscribe'
condition ->(opts) do
opts[:noteable] &&
opts[:current_user] &&
opts[:noteable].persisted? &&
!opts[:noteable].subscribed?(opts[:current_user])
end
command :subscribe do command :subscribe do
@updates[:subscription_event] = 'subscribe' @updates[:subscription_event] = 'subscribe'
end end
desc 'Unsubscribe' desc 'Unsubscribe'
condition ->(opts) do
opts[:noteable] &&
opts[:current_user] &&
opts[:noteable].persisted? &&
opts[:noteable].subscribed?(opts[:current_user])
end
command :unsubscribe do command :unsubscribe do
@updates[:subscription_event] = 'unsubscribe' @updates[:subscription_event] = 'unsubscribe'
end end
desc 'Set a due date' desc 'Set due date'
params '<YYYY-MM-DD> | <N days>' params 'a date in natural language'
condition ->(opts) do
opts[:noteable] &&
opts[:noteable].respond_to?(:due_date) &&
opts[:current_user] &&
opts[:project] &&
opts[:current_user].can?(:"update_#{opts[:noteable].to_ability_name}", opts[:project])
end
command :due_date, :due do |due_date_param| command :due_date, :due do |due_date_param|
return unless noteable.respond_to?(:due_date) due_date = Chronic.parse(due_date_param).try(:to_date)
due_date = begin
if due_date_param.casecmp('tomorrow').zero?
Date.tomorrow
else
Time.now + ChronicDuration.parse(due_date_param)
end
rescue ChronicDuration::DurationParseError
Date.parse(due_date_param) rescue nil
end
@updates[:due_date] = due_date if due_date @updates[:due_date] = due_date if due_date
end end
desc 'Remove due date' desc 'Remove due date'
condition ->(opts) do
opts[:noteable] &&
opts[:noteable].respond_to?(:due_date) &&
opts[:noteable].due_date? &&
opts[:current_user] &&
opts[:project] &&
opts[:current_user].can?(:"update_#{opts[:noteable].to_ability_name}", opts[:project])
end
command :clear_due_date do command :clear_due_date do
return unless noteable.respond_to?(:due_date)
@updates[:due_date] = nil @updates[:due_date] = nil
end end
......
...@@ -25,5 +25,5 @@ do. ...@@ -25,5 +25,5 @@ do.
| `/done` | None | Mark todo as done | | `/done` | None | Mark todo as done |
| `/subscribe` | None | Subscribe | | `/subscribe` | None | Subscribe |
| `/unsubscribe` | None | Unsubscribe | | `/unsubscribe` | None | Unsubscribe |
| `/due_date <YYYY-MM-DD> | <N days>` | `/due` | Set a due date | | `/due_date a date in natural language` | `/due` | Set due date |
| `/clear_due_date` | None | Remove due date | | `/clear_due_date` | None | Remove due date |
...@@ -8,15 +8,25 @@ module Gitlab ...@@ -8,15 +8,25 @@ module Gitlab
end end
module ClassMethods module ClassMethods
def command_definitions def command_definitions(opts = {})
@command_definitions @command_definitions.map do |cmd_def|
end next if cmd_def[:cond_lambda] && !cmd_def[:cond_lambda].call(opts)
cmd_def = cmd_def.dup
def command_names if cmd_def[:description].present? && cmd_def[:description].respond_to?(:call)
command_definitions.flat_map do |command_definition| cmd_def[:description] = cmd_def[:description].call(opts) rescue ''
unless command_definition[:noop]
[command_definition[:name], command_definition[:aliases]].flatten
end end
cmd_def
end.compact
end
def command_names(opts = {})
command_definitions(opts).flat_map do |command_definition|
next if command_definition[:noop]
[command_definition[:name], *command_definition[:aliases]]
end.compact end.compact
end end
...@@ -35,6 +45,11 @@ module Gitlab ...@@ -35,6 +45,11 @@ module Gitlab
@noop = noop @noop = noop
end end
# Allows to define if a lambda to conditionally return an action
def condition(cond_lambda)
@cond_lambda = cond_lambda
end
# Registers a new command which is recognizeable # Registers a new command which is recognizeable
# from body of email or comment. # from body of email or comment.
# Example: # Example:
...@@ -53,6 +68,10 @@ module Gitlab ...@@ -53,6 +68,10 @@ module Gitlab
define_method(proxy_method_name, &block) define_method(proxy_method_name, &block)
define_method(command_name) do |*args| define_method(command_name) do |*args|
unless @cond_lambda.nil? || @cond_lambda.call(project: project, current_user: current_user, noteable: noteable)
return
end
proxy_method = method(proxy_method_name) proxy_method = method(proxy_method_name)
if proxy_method.arity == -1 || proxy_method.arity == args.size if proxy_method.arity == -1 || proxy_method.arity == args.size
...@@ -70,13 +89,16 @@ module Gitlab ...@@ -70,13 +89,16 @@ module Gitlab
name: command_name, name: command_name,
aliases: aliases, aliases: aliases,
description: @description || '', description: @description || '',
params: @params || [], params: @params || []
noop: @noop || false
} }
command_definition[:noop] = @noop unless @noop.nil?
command_definition[:cond_lambda] = @cond_lambda unless @cond_lambda.nil?
@command_definitions << command_definition @command_definitions << command_definition
@description = nil @description = nil
@params = nil @params = nil
@noop = nil
@cond_lambda = nil
end end
end end
end end
......
...@@ -20,7 +20,7 @@ X-Received: by 10.0.0.1 with SMTP id n7mr11234144ipb.85.1371157428600; Thu, ...@@ -20,7 +20,7 @@ X-Received: by 10.0.0.1 with SMTP id n7mr11234144ipb.85.1371157428600; Thu,
X-Scanned-By: MIMEDefang 2.69 on IPv6:2001:470:1d:165::1 X-Scanned-By: MIMEDefang 2.69 on IPv6:2001:470:1d:165::1
/close /close
/unsubscribe /todo
On Sun, Jun 9, 2013 at 1:39 PM, eviltrout via Discourse Meta On Sun, Jun 9, 2013 at 1:39 PM, eviltrout via Discourse Meta
......
require 'spec_helper' require 'spec_helper'
describe Gitlab::SlashCommands::Dsl do describe Gitlab::SlashCommands::Dsl do
COND_LAMBDA = ->(opts) { opts[:project] == 'foo' }
before :all do before :all do
DummyClass = Class.new do DummyClass = Class.new do
include Gitlab::SlashCommands::Dsl include Gitlab::SlashCommands::Dsl
...@@ -20,18 +21,23 @@ describe Gitlab::SlashCommands::Dsl do ...@@ -20,18 +21,23 @@ describe Gitlab::SlashCommands::Dsl do
arg1 arg1
end end
desc 'A command with two args' desc ->(opts) { "A dynamic description for #{opts.fetch(:noteable)}" }
params 'The first argument', 'The second argument' params 'The first argument', 'The second argument'
command :two_args do |arg1, arg2| command :two_args do |arg1, arg2|
[arg1, arg2] [arg1, arg2]
end end
command :wildcard do |*args| noop true
command :cc do |*args|
args args
end end
noop true condition COND_LAMBDA
command :cc do |*args| command :cond_action do |*args|
args
end
command :wildcard do |*args|
args args
end end
end end
...@@ -39,27 +45,73 @@ describe Gitlab::SlashCommands::Dsl do ...@@ -39,27 +45,73 @@ describe Gitlab::SlashCommands::Dsl do
let(:dummy) { DummyClass.new } let(:dummy) { DummyClass.new }
describe '.command_definitions' do describe '.command_definitions' do
it 'returns an array with commands definitions' do let(:base_expected) do
expected = [ [
{ name: :no_args, aliases: [:none], description: 'A command with no args', params: [], noop: false }, { name: :no_args, aliases: [:none], description: 'A command with no args', params: [] },
{ name: :returning, aliases: [], description: 'A command returning a value', params: [], noop: false }, { name: :returning, aliases: [], description: 'A command returning a value', params: [] },
{ name: :one_arg, aliases: [:once, :first], description: '', params: ['The first argument'], noop: false }, { name: :one_arg, aliases: [:once, :first], description: '', params: ['The first argument'] },
{ name: :two_args, aliases: [], description: 'A command with two args', params: ['The first argument', 'The second argument'], noop: false }, { name: :two_args, aliases: [], description: '', params: ['The first argument', 'The second argument'] },
{ name: :wildcard, aliases: [], description: '', params: [], noop: false }, { name: :cc, aliases: [], description: '', params: [], noop: true },
{ name: :cc, aliases: [], description: '', params: [], noop: true } { name: :wildcard, aliases: [], description: '', params: [] }
] ]
end
expect(DummyClass.command_definitions).to eq expected it 'returns an array with commands definitions' do
expect(DummyClass.command_definitions).to match_array base_expected
end
context 'with options passed' do
context 'when condition is met' do
let(:expected) { base_expected << { name: :cond_action, aliases: [], description: '', params: [], cond_lambda: COND_LAMBDA } }
it 'returns an array with commands definitions' do
expect(DummyClass.command_definitions(project: 'foo')).to match_array expected
end
end
context 'when condition is not met' do
it 'returns an array with commands definitions without actions that did not met conditions' do
expect(DummyClass.command_definitions(project: 'bar')).to match_array base_expected
end
end
context 'when description can be generated dynamically' do
it 'returns an array with commands definitions with dynamic descriptions' do
base_expected[3][:description] = 'A dynamic description for merge request'
expect(DummyClass.command_definitions(noteable: 'merge request')).to match_array base_expected
end
end
end end
end end
describe '.command_names' do describe '.command_names' do
it 'returns an array with commands definitions' do let(:base_expected) do
expect(DummyClass.command_names).to eq [ [
:no_args, :none, :returning, :one_arg, :no_args, :none, :returning, :one_arg,
:once, :first, :two_args, :wildcard :once, :first, :two_args, :wildcard
] ]
end end
it 'returns an array with commands definitions' do
expect(DummyClass.command_names).to eq base_expected
end
context 'with options passed' do
context 'when condition is met' do
let(:expected) { base_expected << :cond_action }
it 'returns an array with commands definitions' do
expect(DummyClass.command_names(project: 'foo')).to match_array expected
end
end
context 'when condition is not met' do
it 'returns an array with commands definitions without action that did not met conditions' do
expect(DummyClass.command_names(project: 'bar')).to match_array base_expected
end
end
end
end end
describe 'command with no args' do describe 'command with no args' do
......
...@@ -8,38 +8,152 @@ describe SlashCommands::InterpretService, services: true do ...@@ -8,38 +8,152 @@ describe SlashCommands::InterpretService, services: true do
let(:inprogress) { create(:label, project: project, title: 'In Progress') } let(:inprogress) { create(:label, project: project, title: 'In Progress') }
let(:bug) { create(:label, project: project, title: 'Bug') } let(:bug) { create(:label, project: project, title: 'Bug') }
before do
project.team << [user, :developer]
end
describe '#command_names' do describe '#command_names' do
subject { described_class.command_names } subject do
described_class.command_names(
project: project,
noteable: issue,
current_user: user
)
end
it 'returns the known commands' do it 'returns the basic known commands' do
is_expected.to match_array([ is_expected.to match_array([
:open, :reopen,
:close, :close,
:title, :title,
:assign, :reassign, :assign, :reassign,
:unassign, :remove_assignee,
:milestone,
:clear_milestone, :remove_milestone,
:labels, :label,
:unlabel, :remove_labels, :remove_label,
:clear_labels, :clear_label,
:todo, :todo,
:done,
:subscribe, :subscribe,
:unsubscribe, :due_date, :due
:due_date, :due,
:clear_due_date
]) ])
end end