Simplify the slash commands DSL to store action blocks instead of creating methods

Other improvements:
- Ensure slash commands autocomplete doesn't break when noteable_type is not given
- Slash commands: improve autocomplete behavior and /due command
- We don't display slash commands for note edit forms.
- Add tests for reply by email with slash commands
- Be sure to execute slash commands after the note creation in Notes::CreateService
Signed-off-by: 's avatarRémy Coutable <remy@rymai.me>
parent aadc5062
......@@ -249,7 +249,8 @@
}
}
});
return this.input.atwho({
// We don't instantiate the slash commands autocomplete for note edit forms
$("form:not(.edit-note) .js-gfm-input").atwho({
at: '/',
alias: 'commands',
displayTpl: function(value) {
......@@ -284,6 +285,7 @@
beforeInsert: this.DefaultOptions.beforeInsert
}
});
return;
},
destroyAtWho: function() {
return this.input.atwho('destroy');
......
......@@ -134,10 +134,8 @@ class ProjectsController < Projects::ApplicationController
end
def autocomplete_sources
note_type = params['type']
note_id = params['type_id']
autocomplete = ::Projects::AutocompleteService.new(@project, current_user)
participants = ::Projects::ParticipantsService.new(@project, current_user).execute(note_type, note_id)
autocomplete = ::Projects::AutocompleteService.new(@project, current_user, params)
participants = ::Projects::ParticipantsService.new(@project, current_user, params).execute
@suggestions = {
emojis: Gitlab::AwardEmoji.urls,
......@@ -146,7 +144,7 @@ class ProjectsController < Projects::ApplicationController
mergerequests: autocomplete.merge_requests,
labels: autocomplete.labels,
members: participants,
commands: autocomplete.commands(note_type, note_id)
commands: autocomplete.commands
}
respond_to do |format|
......
......@@ -94,10 +94,10 @@ class IssuableBaseService < BaseService
end
def merge_slash_commands_into_params!(issuable)
command_params = SlashCommands::InterpretService.new(project, current_user).
commands = SlashCommands::InterpretService.new(project, current_user).
execute(params[:description], issuable)
params.merge!(command_params)
params.merge!(commands)
end
def create_issuable(issuable, attributes)
......
......@@ -14,7 +14,8 @@ module Notes
# We execute commands (extracted from `params[:note]`) on the noteable
# **before** we save the note because if the note consists of commands
# only, there is no need be create a note!
commands_executed = SlashCommandsService.new(project, current_user).execute(note)
slash_commands_service = SlashCommandsService.new(project, current_user)
commands = slash_commands_service.extract_commands(note)
if note.save
# Finish the harder work in the background
......@@ -24,7 +25,7 @@ module Notes
# We must add the error after we call #save because errors are reset
# when #save is called
if commands_executed && note.note.blank?
if slash_commands_service.execute(commands, note) && note.note.blank?
note.errors.add(:commands_only, 'Your commands are being executed.')
end
......
......@@ -6,16 +6,19 @@ module Notes
'MergeRequest' => MergeRequests::UpdateService
}
def execute(note)
noteable_update_service = UPDATE_SERVICES[note.noteable_type]
return false unless noteable_update_service
return false unless can?(current_user, :"update_#{note.noteable_type.underscore}", note.noteable)
def extract_commands(note)
@noteable_update_service = UPDATE_SERVICES[note.noteable_type]
return [] unless @noteable_update_service
return [] unless can?(current_user, :"update_#{note.noteable_type.underscore}", note.noteable)
commands = SlashCommands::InterpretService.new(project, current_user).
SlashCommands::InterpretService.new(project, current_user).
execute(note.note, note.noteable)
end
def execute(commands, note)
if commands.any?
noteable_update_service.new(project, current_user, commands).execute(note.noteable)
@noteable_update_service.new(project, current_user, commands).
execute(note.noteable)
end
end
end
......
......@@ -16,26 +16,34 @@ module Projects
@project.labels.select([:title, :color])
end
def commands(noteable_type, noteable_id)
def commands
# We don't return commands when editing an issue or merge request
# This should be improved by not enabling autocomplete at the JS-level
# following this suggestion: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5021#note_13837384
return [] if !target || %w[edit update].include?(params[:action_name])
SlashCommands::InterpretService.command_definitions(
project: @project,
noteable: command_target(noteable_type, noteable_id),
project: project,
noteable: target,
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
def target
@target ||= begin
noteable_id = params[:type_id]
case params[: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
......
module Projects
class ParticipantsService < BaseService
def execute(noteable_type, noteable_id)
@noteable_type = noteable_type
@noteable_id = noteable_id
attr_reader :noteable_type, :noteable_id
def execute
@noteable_type = params[:type]
@noteable_id = params[:type_id]
project_members = sorted(project.team.members)
participants = target_owner + participants_in_target + all_members + groups + project_members
participants.uniq
......@@ -10,13 +13,15 @@ module Projects
def target
@target ||=
case @noteable_type
when "Issue"
project.issues.find_by_iid(@noteable_id)
when "MergeRequest"
project.merge_requests.find_by_iid(@noteable_id)
when "Commit"
project.commit(@noteable_id)
case noteable_type
when 'Issue'
IssuesFinder.new(current_user, project_id: project.id, state: 'all').
execute.find_by(iid: noteable_id)
when 'MergeRequest'
MergeRequestsFinder.new(current_user, project_id: project.id, state: 'all').
execute.find_by(iid: noteable_id)
when 'Commit'
project.commit(noteable_id)
else
nil
end
......
......@@ -11,8 +11,8 @@ module SlashCommands
@updates = {}
commands = extractor(noteable: noteable).extract_commands!(content)
commands.each do |command|
__send__(*command)
commands.each do |command, *args|
execute_command(command, *args)
end
@updates
......@@ -30,8 +30,9 @@ module SlashCommands
"Close this #{noteable.to_ability_name.humanize(capitalize: false)}"
end
condition do
noteable.persisted? &&
noteable.open? &&
current_user.can?(:"update_#{noteable.to_ability_name}", project)
current_user.can?(:"update_#{noteable.to_ability_name}", noteable)
end
command :close do
@updates[:state_event] = 'close'
......@@ -42,7 +43,7 @@ module SlashCommands
end
condition do
noteable.closed? &&
current_user.can?(:"update_#{noteable.to_ability_name}", project)
current_user.can?(:"update_#{noteable.to_ability_name}", noteable)
end
command :open, :reopen do
@updates[:state_event] = 'reopen'
......@@ -52,7 +53,7 @@ module SlashCommands
params '<New title>'
condition do
noteable.persisted? &&
current_user.can?(:"update_#{noteable.to_ability_name}", project)
current_user.can?(:"update_#{noteable.to_ability_name}", noteable)
end
command :title do |title_param|
@updates[:title] = title_param
......@@ -65,9 +66,8 @@ module SlashCommands
end
command :assign, :reassign do |assignee_param|
user = extract_references(assignee_param, :user).first
return unless user
@updates[:assignee_id] = user.id
@updates[:assignee_id] = user.id if user
end
desc 'Remove assignee'
......@@ -87,9 +87,8 @@ module SlashCommands
end
command :milestone do |milestone_param|
milestone = extract_references(milestone_param, :milestone).first
return unless milestone
@updates[:milestone_id] = milestone.id
@updates[:milestone_id] = milestone.id if milestone
end
desc 'Remove milestone'
......@@ -109,9 +108,8 @@ module SlashCommands
end
command :label, :labels do |labels_param|
label_ids = find_label_ids(labels_param)
return if label_ids.empty?
@updates[:add_label_ids] = label_ids
@updates[:add_label_ids] = label_ids unless label_ids.empty?
end
desc 'Remove label(s)'
......@@ -122,9 +120,8 @@ module SlashCommands
end
command :unlabel, :remove_label, :remove_labels do |labels_param|
label_ids = find_label_ids(labels_param)
return if label_ids.empty?
@updates[:remove_label_ids] = label_ids
@updates[:remove_label_ids] = label_ids unless label_ids.empty?
end
desc 'Remove all labels'
......@@ -139,7 +136,6 @@ module SlashCommands
desc 'Add a todo'
condition do
noteable.persisted? &&
current_user &&
!TodoService.new.todo_exist?(noteable, current_user)
end
command :todo do
......@@ -148,7 +144,6 @@ module SlashCommands
desc 'Mark todo as done'
condition do
current_user &&
TodoService.new.todo_exist?(noteable, current_user)
end
command :done do
......@@ -174,12 +169,12 @@ module SlashCommands
end
desc 'Set due date'
params 'a date in natural language'
params '<in 2 days | this Friday | December 31st>'
condition do
noteable.respond_to?(:due_date) &&
current_user.can?(:"update_#{noteable.to_ability_name}", project)
current_user.can?(:"update_#{noteable.to_ability_name}", noteable)
end
command :due_date, :due do |due_date_param|
command :due, :due_date do |due_date_param|
due_date = Chronic.parse(due_date_param).try(:to_date)
@updates[:due_date] = due_date if due_date
......@@ -189,7 +184,7 @@ module SlashCommands
condition do
noteable.respond_to?(:due_date) &&
noteable.due_date? &&
current_user.can?(:"update_#{noteable.to_ability_name}", project)
current_user.can?(:"update_#{noteable.to_ability_name}", noteable)
end
command :clear_due_date do
@updates[:due_date] = nil
......
......@@ -2,6 +2,6 @@
- noteable_class = @noteable.class if @noteable.present?
:javascript
GitLab.GfmAutoComplete.dataSource = "#{autocomplete_sources_namespace_project_path(project.namespace, project, type: noteable_class, type_id: params[:id])}"
GitLab.GfmAutoComplete.dataSource = "#{autocomplete_sources_namespace_project_path(project.namespace, project, type: noteable_class, type_id: params[:id], action_name: action_name)}"
GitLab.GfmAutoComplete.cachedData = undefined;
GitLab.GfmAutoComplete.setup();
......@@ -25,5 +25,5 @@ do.
| `/done` | None | Mark todo as done |
| `/subscribe` | None | Subscribe |
| `/unsubscribe` | None | Unsubscribe |
| `/due_date a date in natural language` | `/due` | Set due date |
| `/due <in 2 days | this Friday | December 31st>` | `/due_date` | Set due date |
| `/clear_due_date` | None | Remove due date |
......@@ -4,20 +4,34 @@ module Gitlab
extend ActiveSupport::Concern
included do
@command_definitions = []
cattr_accessor :definitions
end
module ClassMethods
# This method is used to generate the autocompletion menu
# It returns no-op slash commands (such as `/cc`)
def execute_command(name, *args)
name = name.to_sym
cmd_def = self.class.definitions.find do |cmd_def|
self.class.command_name_and_aliases(cmd_def).include?(name)
end
return unless cmd_def && cmd_def[:action_block]
return if self.class.command_unavailable?(cmd_def, self)
block_arity = cmd_def[:action_block].arity
if block_arity == -1 || block_arity == args.size
instance_exec(*args, &cmd_def[:action_block])
end
end
class_methods do
# This method is used to generate the autocompletion menu.
# It returns no-op slash commands (such as `/cc`).
def command_definitions(opts = {})
@command_definitions.map do |cmd_def|
self.definitions.map do |cmd_def|
context = OpenStruct.new(opts)
next if cmd_def[:cond_block] && !context.instance_exec(&cmd_def[:cond_block])
next if command_unavailable?(cmd_def, context)
cmd_def = cmd_def.dup
if cmd_def[:description].present? && cmd_def[:description].respond_to?(:call)
if cmd_def[:description].respond_to?(:call)
cmd_def[:description] = context.instance_exec(&cmd_def[:description]) rescue ''
end
......@@ -30,13 +44,24 @@ module Gitlab
# It excludes no-op slash commands (such as `/cc`).
# This list can then be given to `Gitlab::SlashCommands::Extractor`.
def command_names(opts = {})
command_definitions(opts).flat_map do |command_definition|
next if command_definition[:noop]
self.definitions.flat_map do |cmd_def|
next if cmd_def[:opts].fetch(:noop, false)
[command_definition[:name], *command_definition[:aliases]]
context = OpenStruct.new(opts)
next if command_unavailable?(cmd_def, context)
command_name_and_aliases(cmd_def)
end.compact
end
def command_unavailable?(cmd_def, context)
cmd_def[:condition_block] && !context.instance_exec(&cmd_def[:condition_block])
end
def command_name_and_aliases(cmd_def)
[cmd_def[:name], *cmd_def[:aliases]]
end
# Allows to give a description to the next slash command.
# This description is shown in the autocomplete menu.
# It accepts a block that will be evaluated with the context given to
......@@ -81,7 +106,7 @@ module Gitlab
# # Awesome code block
# end
def condition(&block)
@cond_block = block
@condition_block = block
end
# Registers a new command which is recognizeable from body of email or
......@@ -95,45 +120,22 @@ module Gitlab
# end
def command(*command_names, &block)
opts = command_names.extract_options!
command_name, *aliases = command_names
proxy_method_name = "__#{command_name}__"
if block_given?
# This proxy method is needed because calling `return` from inside a
# block/proc, causes a `return` from the enclosing method or lambda,
# otherwise a LocalJumpError error is raised.
define_method(proxy_method_name, &block)
define_method(command_name) do |*args|
return if @cond_block && !instance_exec(&@cond_block)
proxy_method = method(proxy_method_name)
if proxy_method.arity == -1 || proxy_method.arity == args.size
instance_exec(*args, &proxy_method)
end
end
private command_name
aliases.each do |alias_command|
alias_method alias_command, command_name
private alias_command
end
end
name, *aliases = command_names
command_definition = {
name: command_name,
self.definitions ||= []
self.definitions << {
name: name,
aliases: aliases,
description: @description || '',
params: @params || []
params: @params || [],
condition_block: @condition_block,
action_block: block,
opts: opts
}
command_definition[:noop] = opts[:noop] || false
command_definition[:cond_block] = @cond_block
@command_definitions << command_definition
@description = nil
@params = nil
@cond_block = nil
@condition_block = nil
end
end
end
......
......@@ -55,5 +55,4 @@ feature 'Issues > User uses slash commands', feature: true, js: true do
end
end
end
end
Return-Path: <jake@adventuretime.ooo>
Received: from iceking.adventuretime.ooo ([unix socket]) by iceking (Cyrus v2.2.13-Debian-2.2.13-19+squeeze3) with LMTPA; Thu, 13 Jun 2013 17:03:50 -0400
Received: from mail-ie0-x234.google.com (mail-ie0-x234.google.com [IPv6:2607:f8b0:4001:c03::234]) by iceking.adventuretime.ooo (8.14.3/8.14.3/Debian-9.4) with ESMTP id r5DL3nFJ016967 (version=TLSv1/SSLv3 cipher=RC4-SHA bits=128 verify=NOT) for <reply+59d8df8370b7e95c5a49fbf86aeb2c93@appmail.adventuretime.ooo>; Thu, 13 Jun 2013 17:03:50 -0400
Received: by mail-ie0-f180.google.com with SMTP id f4so21977375iea.25 for <reply+59d8df8370b7e95c5a49fbf86aeb2c93@appmail.adventuretime.ooo>; Thu, 13 Jun 2013 14:03:48 -0700
Received: by 10.0.0.1 with HTTP; Thu, 13 Jun 2013 14:03:48 -0700
Date: Thu, 13 Jun 2013 17:03:48 -0400
From: Jake the Dog <jake@adventuretime.ooo>
To: reply+59d8df8370b7e95c5a49fbf86aeb2c93@appmail.adventuretime.ooo
Message-ID: <CADkmRc+rNGAGGbV2iE5p918UVy4UyJqVcXRO2=otppgzduJSg@mail.gmail.com>
In-Reply-To: <issue_1@localhost>
References: <issue_1@localhost> <reply-59d8df8370b7e95c5a49fbf86aeb2c93@localhost>
Subject: re: [Discourse Meta] eviltrout posted in 'Adventure Time Sux'
Mime-Version: 1.0
Content-Type: text/plain;
charset=ISO-8859-1
Content-Transfer-Encoding: 7bit
X-Sieve: CMU Sieve 2.2
X-Received: by 10.0.0.1 with SMTP id n7mr11234144ipb.85.1371157428600; Thu,
13 Jun 2013 14:03:48 -0700 (PDT)
X-Scanned-By: MIMEDefang 2.69 on IPv6:2001:470:1d:165::1
Cool!
/close
/todo
/due tomorrow
On Sun, Jun 9, 2013 at 1:39 PM, eviltrout via Discourse Meta
<reply+59d8df8370b7e95c5a49fbf86aeb2c93@appmail.adventuretime.ooo> wrote:
>
>
>
> eviltrout posted in 'Adventure Time Sux' on Discourse Meta:
>
> ---
> hey guys everyone knows adventure time sucks!
>
> ---
> Please visit this link to respond: http://localhost:3000/t/adventure-time-sux/1234/3
>
> To unsubscribe from these emails, visit your [user preferences](http://localhost:3000/user_preferences).
>
......@@ -21,6 +21,7 @@ X-Scanned-By: MIMEDefang 2.69 on IPv6:2001:470:1d:165::1
/close
/todo
/due tomorrow
On Sun, Jun 9, 2013 at 1:39 PM, eviltrout via Discourse Meta
......
......@@ -75,13 +75,54 @@ describe Gitlab::Email::Handler::CreateNoteHandler, lib: true do
project.team << [user, :developer]
end
it 'raises a CommandsOnlyNoteError' do
expect { receiver.execute }.not_to raise_error
it 'does not raise an error' do
expect(TodoService.new.todo_exist?(noteable, user)).to be_falsy
# One system note is created for the 'close' event
expect { receiver.execute }.to change { noteable.notes.count }.by(1)
expect(noteable.reload).to be_closed
expect(noteable.due_date).to eq(Date.tomorrow)
expect(TodoService.new.todo_exist?(noteable, user)).to be_truthy
end
end
end
end
context 'when the note contains slash commands' do
let!(:email_raw) { fixture_file("emails/commands_in_reply.eml") }
context 'and current user cannot update noteable' do
it 'post a note and does not update the noteable' do
expect(TodoService.new.todo_exist?(noteable, user)).to be_falsy
# One system note is created for the new note
expect { receiver.execute }.to change { noteable.notes.count }.by(1)
expect(noteable.reload).to be_open
expect(noteable.due_date).to be_nil
expect(TodoService.new.todo_exist?(noteable, user)).to be_falsy
end
end
context 'and current user can update noteable' do
before do
project.team << [user, :developer]
end
it 'post a note and updates the noteable' do
expect(TodoService.new.todo_exist?(noteable, user)).to be_falsy
# One system note is created for the new note, one for the 'close' event
expect { receiver.execute }.to change { noteable.notes.count }.by(2)
expect(noteable.reload).to be_closed
expect(noteable.due_date).to eq(Date.tomorrow)
expect(TodoService.new.todo_exist?(noteable, user)).to be_truthy
end
end
end
context "when the reply is blank" do
let!(:email_raw) { fixture_file("emails/no_content_reply.eml") }
......
......@@ -46,12 +46,42 @@ describe Gitlab::SlashCommands::Dsl do
describe '.command_definitions' do
let(:base_expected) do
[
{ name: :no_args, aliases: [:none], description: 'A command with no args', params: [], noop: false, cond_block: nil },
{ name: :returning, aliases: [], description: 'A command returning a value', params: [], noop: false, cond_block: nil },
{ name: :one_arg, aliases: [:once, :first], description: '', params: ['The first argument'], noop: false, cond_block: nil },
{ name: :two_args, aliases: [], description: '', params: ['The first argument', 'The second argument'], noop: false, cond_block: nil },
{ name: :cc, aliases: [], description: '', params: [], noop: true, cond_block: nil },
{ name: :wildcard, aliases: [], description: '', params: [], noop: false, cond_block: nil }
{
name: :no_args, aliases: [:none],
description: 'A command with no args', params: [],
condition_block: nil, action_block: a_kind_of(Proc),
opts: {}
},
{
name: :returning, aliases: [],
description: 'A command returning a value', params: [],
condition_block: nil, action_block: a_kind_of(Proc),
opts: {}
},
{
name: :one_arg, aliases: [:once, :first],
description: '', params: ['The first argument'],
condition_block: nil, action_block: a_kind_of(Proc),
opts: {}
},
{
name: :two_args, aliases: [],
description: '', params: ['The first argument', 'The second argument'],
condition_block: nil, action_block: a_kind_of(Proc),
opts: {}
},
{
name: :cc, aliases: [],
description: '', params: [],
condition_block: nil, action_block: nil,
opts: { noop: true }
},
{
name: :wildcard, aliases: [],
description: '', params: [],
condition_block: nil, action_block: a_kind_of(Proc),
opts: {}
}
]
end
......@@ -61,7 +91,14 @@ describe Gitlab::SlashCommands::Dsl do
context 'with options passed' do
context 'when condition is met' do
let(:expected) { base_expected << { name: :cond_action, aliases: [], description: '', params: [], noop: false, cond_block: a_kind_of(Proc) } }
let(:expected) do
base_expected << {
name: :cond_action, aliases: [],
description: '', params: [],
condition_block: a_kind_of(Proc), action_block: a_kind_of(Proc),
opts: {}
}
end
it 'returns an array with commands definitions' do
expect(DummyClass.command_definitions(project: 'foo')).to match_array expected
......@@ -115,76 +152,78 @@ describe Gitlab::SlashCommands::Dsl do
let(:dummy) { DummyClass.new(nil) }
describe 'command with no args' do
context 'called with no args' do
it 'succeeds' do
expect(dummy.__send__(:no_args)).to eq 'Hello World!'
describe '#execute_command' do
describe 'command with no args' do
context 'called with no args' do
it 'succeeds' do
expect(dummy.execute_command(:no_args)).to eq 'Hello World!'
end
end
end
end
describe 'command with an explicit return' do
context 'called with no args' do
it 'succeeds' do
expect(dummy.__send__(:returning)).to eq 42
describe 'command with an explicit return' do
context 'called with no args' do
it 'succeeds' do
expect { dummy.execute_command(:returning) }.to raise_error(LocalJumpError)
end
end
end
end
describe 'command with one arg' do
context 'called with one arg' do
it 'succeeds' do
expect(dummy.__send__(:one_arg, 42)).to eq 42
describe 'command with one arg' do
context 'called with one arg' do
it 'succeeds' do
expect(dummy.execute_command(:one_arg, 42)).to eq 42
end
end
end
end
describe 'command with two args' do
context 'called with two args' do
it 'succeeds' do
expect(dummy.__send__(:two_args, 42, 'foo')).to eq [42, 'foo']
describe 'command with two args' do
context 'called with two args' do
it 'succeeds' do
expect(dummy.execute_command(:two_args, 42, 'foo')).to eq [42, 'foo']
end
end
end
end
describe 'noop command' do
it 'is not meant to be called directly' do
expect { dummy.__send__(:cc) }.to raise_error(NoMethodError)
end
end
describe 'command with condition' do
context 'when condition is not met' do
describe 'noop command' do
it 'returns nil' do
expect(dummy.__send__(:cond_action)).to be_nil
expect(dummy.execute_command(:cc)).to be_nil
end
end
context 'when condition is met' do
let(:dummy) { DummyClass.new('foo') }
describe 'command with condition' do
context 'when condition is not met' do