From 029b7d2e9266246feff2f165a10b16be1d7fe88e Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Sat, 13 Aug 2016 11:58:51 -0500 Subject: [PATCH] Fixed specs and fixes based on failing specs --- app/controllers/projects/issues_controller.rb | 8 +- app/services/notes/create_service.rb | 23 +- app/services/notes/slash_commands_service.rb | 19 +- .../slash_commands/interpret_service.rb | 79 +++---- .../slash_commands/command_definition.rb | 11 +- lib/gitlab/slash_commands/dsl.rb | 5 +- lib/gitlab/slash_commands/extractor.rb | 12 +- .../slash_commands/command_definition_spec.rb | 143 ++++++++++++ spec/lib/gitlab/slash_commands/dsl_spec.rb | 190 ++++------------ .../gitlab/slash_commands/extractor_spec.rb | 55 +++-- spec/services/notes/create_service_spec.rb | 2 +- .../notes/slash_commands_service_spec.rb | 33 ++- .../slash_commands/interpret_service_spec.rb | 203 +++--------------- 13 files changed, 360 insertions(+), 423 deletions(-) create mode 100644 spec/lib/gitlab/slash_commands/command_definition_spec.rb diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb index c8eda6b27f4..d0cc4b55467 100644 --- a/app/controllers/projects/issues_controller.rb +++ b/app/controllers/projects/issues_controller.rb @@ -176,12 +176,7 @@ class Projects::IssuesController < Projects::ApplicationController protected def issue - @noteable = @issue ||= - begin - @project.issues.find_by!(iid: params[:id]) - rescue ActiveRecord::RecordNotFound - redirect_old - end + @noteable = @issue ||= @project.issues.find_by(iid: params[:id]) || redirect_old end alias_method :subscribable_resource, :issue alias_method :issuable, :issue @@ -225,7 +220,6 @@ class Projects::IssuesController < Projects::ApplicationController if issue redirect_to issue_path(issue) - return else raise ActiveRecord::RecordNotFound.new end diff --git a/app/services/notes/create_service.rb b/app/services/notes/create_service.rb index e9f37e04993..a36008c3ef5 100644 --- a/app/services/notes/create_service.rb +++ b/app/services/notes/create_service.rb @@ -15,20 +15,29 @@ module Notes # **before** we save the note because if the note consists of commands # only, there is no need be create a note! slash_commands_service = SlashCommandsService.new(project, current_user) - content, command_params = slash_commands_service.extract_commands(note) - note.note = content + if slash_commands_service.supported?(note) + content, command_params = slash_commands_service.extract_commands(note) - if note.save + only_commands = content.empty? + + note.note = content + end + + if !only_commands && note.save # Finish the harder work in the background NewNoteWorker.perform_in(2.seconds, note.id, params) todo_service.new_note(note, current_user) end - # We must add the error after we call #save because errors are reset - # when #save is called - if slash_commands_service.execute(command_params, note) && note.note.blank? - note.errors.add(:commands_only, 'Your commands have been executed!') + if command_params && command_params.any? + slash_commands_service.execute(command_params, note) + + # We must add the error after we call #save because errors are reset + # when #save is called + if only_commands + note.errors.add(:commands_only, 'Your commands have been executed!') + end end note diff --git a/app/services/notes/slash_commands_service.rb b/app/services/notes/slash_commands_service.rb index f2c43775b72..4a9a8a64653 100644 --- a/app/services/notes/slash_commands_service.rb +++ b/app/services/notes/slash_commands_service.rb @@ -5,10 +5,13 @@ module Notes 'MergeRequest' => MergeRequests::UpdateService } + def supported?(note) + noteable_update_service(note) && + can?(current_user, :"update_#{note.noteable_type.underscore}", note.noteable) + end + 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) + return [note.note, {}] unless supported?(note) SlashCommands::InterpretService.new(project, current_user). execute(note.note, note.noteable) @@ -16,9 +19,15 @@ module Notes def execute(command_params, note) return if command_params.empty? + return unless supported?(note) - @noteable_update_service.new(project, current_user, command_params). - execute(note.noteable) + noteable_update_service(note).new(project, current_user, command_params).execute(note.noteable) + end + + private + + def noteable_update_service(note) + UPDATE_SERVICES[note.noteable_type] end end end diff --git a/app/services/slash_commands/interpret_service.rb b/app/services/slash_commands/interpret_service.rb index f7b9547bd2b..126f97b0f9b 100644 --- a/app/services/slash_commands/interpret_service.rb +++ b/app/services/slash_commands/interpret_service.rb @@ -2,16 +2,16 @@ module SlashCommands class InterpretService < BaseService include Gitlab::SlashCommands::Dsl - attr_reader :noteable + attr_reader :issuable # Takes a text and interpret the commands that are extracted from it. # Returns a hash of changes to be applied to a record. - def execute(content, noteable) - @noteable = noteable + def execute(content, issuable) + @issuable = issuable @updates = {} opts = { - noteable: noteable, + issuable: issuable, current_user: current_user, project: project } @@ -35,23 +35,24 @@ module SlashCommands end desc do - "Close this #{noteable.to_ability_name.humanize(capitalize: false)}" + "Close this #{issuable.to_ability_name.humanize(capitalize: false)}" end condition do - noteable.persisted? && - noteable.open? && - current_user.can?(:"update_#{noteable.to_ability_name}", noteable) + issuable.persisted? && + issuable.open? && + current_user.can?(:"update_#{issuable.to_ability_name}", issuable) end command :close do @updates[:state_event] = 'close' end desc do - "Reopen this #{noteable.to_ability_name.humanize(capitalize: false)}" + "Reopen this #{issuable.to_ability_name.humanize(capitalize: false)}" end condition do - noteable.closed? && - current_user.can?(:"update_#{noteable.to_ability_name}", noteable) + issuable.persisted? && + issuable.closed? && + current_user.can?(:"update_#{issuable.to_ability_name}", issuable) end command :reopen, :open do @updates[:state_event] = 'reopen' @@ -60,8 +61,8 @@ module SlashCommands desc 'Change title' params '' condition do - noteable.persisted? && - current_user.can?(:"update_#{noteable.to_ability_name}", noteable) + issuable.persisted? && + current_user.can?(:"update_#{issuable.to_ability_name}", issuable) end command :title do |title_param| @updates[:title] = title_param @@ -70,7 +71,7 @@ module SlashCommands desc 'Assign' params '@user' condition do - current_user.can?(:"admin_#{noteable.to_ability_name}", project) + current_user.can?(:"admin_#{issuable.to_ability_name}", project) end command :assign do |assignee_param| user = extract_references(assignee_param, :user).first @@ -82,8 +83,9 @@ module SlashCommands desc 'Remove assignee' condition do - noteable.assignee_id? && - current_user.can?(:"admin_#{noteable.to_ability_name}", project) + issuable.persisted? && + issuable.assignee_id? && + current_user.can?(:"admin_#{issuable.to_ability_name}", project) end command :unassign, :remove_assignee do @updates[:assignee_id] = nil @@ -92,7 +94,7 @@ module SlashCommands desc 'Set milestone' params '%"milestone"' condition do - current_user.can?(:"admin_#{noteable.to_ability_name}", project) && + current_user.can?(:"admin_#{issuable.to_ability_name}", project) && project.milestones.active.any? end command :milestone do |milestone_param| @@ -104,8 +106,9 @@ module SlashCommands desc 'Remove milestone' condition do - noteable.milestone_id? && - current_user.can?(:"admin_#{noteable.to_ability_name}", project) + issuable.persisted? && + issuable.milestone_id? && + current_user.can?(:"admin_#{issuable.to_ability_name}", project) end command :clear_milestone, :remove_milestone do @updates[:milestone_id] = nil @@ -114,7 +117,7 @@ module SlashCommands desc 'Add label(s)' params '~label1 ~"label 2"' condition do - current_user.can?(:"admin_#{noteable.to_ability_name}", project) && + current_user.can?(:"admin_#{issuable.to_ability_name}", project) && project.labels.any? end command :label, :labels do |labels_param| @@ -126,8 +129,9 @@ module SlashCommands desc 'Remove label(s)' params '~label1 ~"label 2"' condition do - noteable.labels.any? && - current_user.can?(:"admin_#{noteable.to_ability_name}", project) + issuable.persisted? && + issuable.labels.any? && + current_user.can?(:"admin_#{issuable.to_ability_name}", project) end command :unlabel, :remove_label, :remove_labels do |labels_param| label_ids = find_label_ids(labels_param) @@ -137,8 +141,9 @@ module SlashCommands desc 'Remove all labels' condition do - noteable.labels.any? && - current_user.can?(:"admin_#{noteable.to_ability_name}", project) + issuable.persisted? && + issuable.labels.any? && + current_user.can?(:"admin_#{issuable.to_ability_name}", project) end command :clear_labels, :clear_label do @updates[:label_ids] = [] @@ -146,8 +151,8 @@ module SlashCommands desc 'Add a todo' condition do - noteable.persisted? && - !TodoService.new.todo_exist?(noteable, current_user) + issuable.persisted? && + !TodoService.new.todo_exist?(issuable, current_user) end command :todo do @updates[:todo_event] = 'add' @@ -155,7 +160,8 @@ module SlashCommands desc 'Mark todo as done' condition do - TodoService.new.todo_exist?(noteable, current_user) + issuable.persisted? && + TodoService.new.todo_exist?(issuable, current_user) end command :done do @updates[:todo_event] = 'done' @@ -163,8 +169,8 @@ module SlashCommands desc 'Subscribe' condition do - noteable.persisted? && - !noteable.subscribed?(current_user) + issuable.persisted? && + !issuable.subscribed?(current_user) end command :subscribe do @updates[:subscription_event] = 'subscribe' @@ -172,8 +178,8 @@ module SlashCommands desc 'Unsubscribe' condition do - noteable.persisted? && - noteable.subscribed?(current_user) + issuable.persisted? && + issuable.subscribed?(current_user) end command :unsubscribe do @updates[:subscription_event] = 'unsubscribe' @@ -182,8 +188,8 @@ module SlashCommands desc 'Set due date' params '' condition do - noteable.respond_to?(:due_date) && - current_user.can?(:"update_#{noteable.to_ability_name}", noteable) + issuable.respond_to?(:due_date) && + current_user.can?(:"update_#{issuable.to_ability_name}", issuable) end command :due, :due_date do |due_date_param| due_date = Chronic.parse(due_date_param).try(:to_date) @@ -193,9 +199,10 @@ module SlashCommands desc 'Remove due date' condition do - noteable.respond_to?(:due_date) && - noteable.due_date? && - current_user.can?(:"update_#{noteable.to_ability_name}", noteable) + issuable.persisted? && + issuable.respond_to?(:due_date) && + issuable.due_date? && + current_user.can?(:"update_#{issuable.to_ability_name}", issuable) end command :clear_due_date do @updates[:due_date] = nil diff --git a/lib/gitlab/slash_commands/command_definition.rb b/lib/gitlab/slash_commands/command_definition.rb index 5dec6c91869..187c1c9489f 100644 --- a/lib/gitlab/slash_commands/command_definition.rb +++ b/lib/gitlab/slash_commands/command_definition.rb @@ -3,8 +3,8 @@ module Gitlab class CommandDefinition attr_accessor :name, :aliases, :description, :params, :condition_block, :action_block - def valid? - name.present? + def initialize(name) + @name = name end def all_names @@ -22,13 +22,6 @@ module Gitlab context.instance_exec(&condition_block) end - def to_description(opts) - return description unless description.respond_to?(:call) - - context = OpenStruct.new(opts) - context.instance_exec(&description) rescue '' - end - def execute(context, opts, *args) return if noop? || !available?(opts) diff --git a/lib/gitlab/slash_commands/dsl.rb b/lib/gitlab/slash_commands/dsl.rb index 58ba7027f84..7b1a094a7e6 100644 --- a/lib/gitlab/slash_commands/dsl.rb +++ b/lib/gitlab/slash_commands/dsl.rb @@ -73,16 +73,13 @@ module Gitlab def command(*command_names, &block) name, *aliases = command_names - definition = CommandDefinition.new - definition.name = name + definition = CommandDefinition.new(name) definition.aliases = aliases definition.description = @description || '' definition.params = @params || [] definition.condition_block = @condition_block definition.action_block = block - return unless definition.valid? - self.command_definitions << definition definition.all_names.each do |name| diff --git a/lib/gitlab/slash_commands/extractor.rb b/lib/gitlab/slash_commands/extractor.rb index a6838cb5e7c..02c4c8c492e 100644 --- a/lib/gitlab/slash_commands/extractor.rb +++ b/lib/gitlab/slash_commands/extractor.rb @@ -29,8 +29,8 @@ module Gitlab # commands = extractor.extract_commands(msg) #=> [['labels', '~foo ~"bar baz"']] # msg #=> "hello\nworld" # ``` - def extract_commands(content, opts) - return [] unless content + def extract_commands(content, opts = {}) + return [content, []] unless content content = content.dup @@ -107,7 +107,13 @@ module Gitlab # Command not in a blockquote, blockcode, or HTML tag: # /close - ^\/(?#{Regexp.union(names)})(?:$|\ (?[^\/\n]*)$) + ^\/ + (?#{Regexp.union(names)}) + (?: + [ ] + (?[^\/\n]*) + )? + (?:\n|$) ) }mx end diff --git a/spec/lib/gitlab/slash_commands/command_definition_spec.rb b/spec/lib/gitlab/slash_commands/command_definition_spec.rb new file mode 100644 index 00000000000..2a75fab24b0 --- /dev/null +++ b/spec/lib/gitlab/slash_commands/command_definition_spec.rb @@ -0,0 +1,143 @@ +require 'spec_helper' + +describe Gitlab::SlashCommands::CommandDefinition do + subject { described_class.new(:command) } + + describe "#all_names" do + context "when the command has aliases" do + before do + subject.aliases = [:alias1, :alias2] + end + + it "returns an array with the name and aliases" do + expect(subject.all_names).to eq([:command, :alias1, :alias2]) + end + end + + context "when the command doesn't have aliases" do + it "returns an array with the name" do + expect(subject.all_names).to eq([:command]) + end + end + end + + describe "#noop?" do + context "when the command has an action block" do + before do + subject.action_block = -> { } + end + + it "returns false" do + expect(subject.noop?).to be false + end + end + + context "when the command doesn't have an action block" do + it "returns true" do + expect(subject.noop?).to be true + end + end + end + + describe "#available?" do + let(:opts) { { go: false } } + + context "when the command has a condition block" do + before do + subject.condition_block = -> { go } + end + + context "when the condition block returns true" do + before do + opts[:go] = true + end + + it "returns true" do + expect(subject.available?(opts)).to be true + end + end + + context "when the condition block returns false" do + it "returns false" do + expect(subject.available?(opts)).to be false + end + end + end + + context "when the command doesn't have a condition block" do + it "returns true" do + expect(subject.available?(opts)).to be true + end + end + end + + describe "#execute" do + let(:context) { OpenStruct.new(run: false) } + + context "when the command is a noop" do + it "doesn't execute the command" do + expect(context).not_to receive(:instance_exec) + + subject.execute(context, {}) + + expect(context.run).to be false + end + end + + context "when the command is not a noop" do + before do + subject.action_block = -> { self.run = true } + end + + context "when the command is not available" do + before do + subject.condition_block = -> { false } + end + + it "doesn't execute the command" do + subject.execute(context, {}) + + expect(context.run).to be false + end + end + + context "when the command is available" do + context "when the command has an exact number of arguments" do + before do + subject.action_block = ->(arg) { self.run = arg } + end + + context "when the command is provided a wrong number of arguments" do + it "doesn't execute the command" do + subject.execute(context, {}, true, true) + + expect(context.run).to be false + end + end + + context "when the command is provided the right number of arguments" do + it "executes the command" do + subject.execute(context, {}, true) + + expect(context.run).to be true + end + end + end + + context "when the command has a variable number of arguments" do + before do + subject.action_block = ->(*args) { self.run = args.first } + end + + context "when the command is provided any number of arguments" do + it "executes the command" do + subject.execute(context, {}, true, true) + + expect(context.run).to be true + end + end + end + end + end + end +end diff --git a/spec/lib/gitlab/slash_commands/dsl_spec.rb b/spec/lib/gitlab/slash_commands/dsl_spec.rb index 500ff3ca1fe..87be3455baf 100644 --- a/spec/lib/gitlab/slash_commands/dsl_spec.rb +++ b/spec/lib/gitlab/slash_commands/dsl_spec.rb @@ -10,9 +10,9 @@ describe Gitlab::SlashCommands::Dsl do "Hello World!" end - desc 'A command returning a value' + desc { "A command with #{something}" } command :returning do - return 42 + 42 end params 'The first argument' @@ -28,7 +28,7 @@ describe Gitlab::SlashCommands::Dsl do [arg1, arg2] end - command :cc, noop: true + command :cc condition do project == 'foo' @@ -49,182 +49,74 @@ describe Gitlab::SlashCommands::Dsl do { name: :no_args, aliases: [:none], description: 'A command with no args', params: [], - condition_block: nil, action_block: a_kind_of(Proc), - opts: {} + condition_block: nil, action_block: a_kind_of(Proc) }, { name: :returning, aliases: [], description: 'A command returning a value', params: [], - condition_block: nil, action_block: a_kind_of(Proc), - opts: {} + condition_block: nil, action_block: a_kind_of(Proc) }, { name: :one_arg, aliases: [:once, :first], description: '', params: ['The first argument'], - condition_block: nil, action_block: a_kind_of(Proc), - opts: {} + condition_block: nil, action_block: a_kind_of(Proc) }, { name: :two_args, aliases: [], description: '', params: ['The first argument', 'The second argument'], - condition_block: nil, action_block: a_kind_of(Proc), - opts: {} + condition_block: nil, action_block: a_kind_of(Proc) }, { name: :cc, aliases: [], description: '', params: [], - condition_block: nil, action_block: nil, - opts: { noop: true } + condition_block: nil, action_block: nil }, { name: :wildcard, aliases: [], description: '', params: [], - condition_block: nil, action_block: a_kind_of(Proc), - opts: {} + condition_block: nil, action_block: a_kind_of(Proc) } ] end it 'returns an array with commands definitions' do - expect(DummyClass.command_definitions).to match_array base_expected - end + no_args_def, returning_def, one_arg_def, two_args_def, cc_def, cond_action_def, wildcard_def = DummyClass.command_definitions - context 'with options passed' do - context 'when condition is met' do - 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 + expect(no_args_def.name).to eq(:no_args) + expect(no_args_def.aliases).to eq([:none]) + expect(no_args_def.description).to eq('A command with no args') + expect(no_args_def.params).to eq([]) + expect(no_args_def.condition_block).to be_nil + expect(no_args_def.action_block).to be_a_kind_of(Proc) - it 'returns an array with commands definitions' do - expect(DummyClass.command_definitions(project: 'foo')).to match_array expected - end - end + expect(returning_def.name).to eq(:returning) + expect(returning_def.aliases).to eq([]) + expect(returning_def.description).to be_a_kind_of(Proc) + expect(returning_def.to_h(something: "a block description")[:description]).to eq('A command with a block description') + expect(returning_def.params).to eq([]) + expect(returning_def.condition_block).to be_nil + expect(returning_def.action_block).to be_a_kind_of(Proc) - 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 + expect(one_arg_def.name).to eq(:one_arg) + expect(one_arg_def.aliases).to eq([:once, :first]) + expect(one_arg_def.description).to eq('') + expect(one_arg_def.params).to eq(['The first argument']) + expect(one_arg_def.condition_block).to be_nil + expect(one_arg_def.action_block).to be_a_kind_of(Proc) - 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(cc_def.name).to eq(:cc) + expect(cc_def.aliases).to eq([]) + expect(cc_def.description).to eq('') + expect(cc_def.params).to eq([]) + expect(cc_def.condition_block).to be_nil + expect(cc_def.action_block).to be_nil - expect(DummyClass.command_definitions(noteable: 'merge request')).to match_array base_expected - end - end - end - end - - describe '.command_names' do - let(:base_expected) do - [ - :no_args, :none, :returning, :one_arg, - :once, :first, :two_args, :wildcard - ] - 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 - - let(:dummy) { DummyClass.new(nil) } - - 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 - - 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 - - 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 - - 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 - - describe 'noop command' do - it 'returns nil' do - expect(dummy.execute_command(:cc)).to be_nil - end - end - - describe 'command with condition' do - context 'when condition is not met' do - it 'returns nil' do - expect(dummy.execute_command(:cond_action)).to be_nil - end - end - - context 'when condition is met' do - let(:dummy) { DummyClass.new('foo') } - - it 'succeeds' do - expect(dummy.execute_command(:cond_action, 42)).to eq 42 - end - end - end - - describe 'command with wildcard' do - context 'called with no args' do - it 'succeeds' do - expect(dummy.execute_command(:wildcard)).to eq [] - end - end - - context 'called with one arg' do - it 'succeeds' do - expect(dummy.execute_command(:wildcard, 42)).to eq [42] - end - end - - context 'called with two args' do - it 'succeeds' do - expect(dummy.execute_command(:wildcard, 42, 'foo')).to eq [42, 'foo'] - end - end + expect(wildcard_def.name).to eq(:wildcard) + expect(wildcard_def.aliases).to eq([]) + expect(wildcard_def.description).to eq('') + expect(wildcard_def.params).to eq([]) + expect(wildcard_def.condition_block).to be_nil + expect(wildcard_def.action_block).to be_a_kind_of(Proc) end end end diff --git a/spec/lib/gitlab/slash_commands/extractor_spec.rb b/spec/lib/gitlab/slash_commands/extractor_spec.rb index 8a6801205fa..09f909dcdd2 100644 --- a/spec/lib/gitlab/slash_commands/extractor_spec.rb +++ b/spec/lib/gitlab/slash_commands/extractor_spec.rb @@ -1,32 +1,43 @@ require 'spec_helper' describe Gitlab::SlashCommands::Extractor do - let(:extractor) { described_class.new([:open, :assign, :labels, :power]) } + let(:definitions) do + Class.new do + include Gitlab::SlashCommands::Dsl + + command(:reopen, :open) { } + command(:assign) { } + command(:labels) { } + command(:power) { } + end.command_definitions + end + + let(:extractor) { described_class.new(definitions) } shared_examples 'command with no argument' do it 'extracts command' do - commands = extractor.extract_commands(original_msg) + msg, commands = extractor.extract_commands(original_msg) expect(commands).to eq [['open']] - expect(original_msg).to eq final_msg + expect(msg).to eq final_msg end end shared_examples 'command with a single argument' do it 'extracts command' do - commands = extractor.extract_commands(original_msg) + msg, commands = extractor.extract_commands(original_msg) expect(commands).to eq [['assign', '@joe']] - expect(original_msg).to eq final_msg + expect(msg).to eq final_msg end end shared_examples 'command with multiple arguments' do it 'extracts command' do - commands = extractor.extract_commands(original_msg) + msg, commands = extractor.extract_commands(original_msg) expect(commands).to eq [['labels', '~foo ~"bar baz" label']] - expect(original_msg).to eq final_msg + expect(msg).to eq final_msg end end @@ -49,7 +60,7 @@ describe Gitlab::SlashCommands::Extractor do context 'in the middle of a line' do it 'does not extract command' do msg = "hello\nworld /open" - commands = extractor.extract_commands(msg) + msg, commands = extractor.extract_commands(msg) expect(commands).to be_empty expect(msg).to eq "hello\nworld /open" @@ -59,7 +70,7 @@ describe Gitlab::SlashCommands::Extractor do context 'at the end of content' do it_behaves_like 'command with no argument' do let(:original_msg) { "hello\n/open" } - let(:final_msg) { "hello\n" } + let(:final_msg) { "hello" } end end end @@ -82,7 +93,7 @@ describe Gitlab::SlashCommands::Extractor do context 'in the middle of a line' do it 'does not extract command' do msg = "hello\nworld /assign @joe" - commands = extractor.extract_commands(msg) + msg, commands = extractor.extract_commands(msg) expect(commands).to be_empty expect(msg).to eq "hello\nworld /assign @joe" @@ -92,14 +103,14 @@ describe Gitlab::SlashCommands::Extractor do context 'at the end of content' do it_behaves_like 'command with a single argument' do let(:original_msg) { "hello\n/assign @joe" } - let(:final_msg) { "hello\n" } + let(:final_msg) { "hello" } end end context 'when argument is not separated with a space' do it 'does not extract command' do msg = "hello\n/assign@joe\nworld" - commands = extractor.extract_commands(msg) + msg, commands = extractor.extract_commands(msg) expect(commands).to be_empty expect(msg).to eq "hello\n/assign@joe\nworld" @@ -125,7 +136,7 @@ describe Gitlab::SlashCommands::Extractor do context 'in the middle of a line' do it 'does not extract command' do msg = %(hello\nworld /labels ~foo ~"bar baz" label) - commands = extractor.extract_commands(msg) + msg, commands = extractor.extract_commands(msg) expect(commands).to be_empty expect(msg).to eq %(hello\nworld /labels ~foo ~"bar baz" label) @@ -135,14 +146,14 @@ describe Gitlab::SlashCommands::Extractor do context 'at the end of content' do it_behaves_like 'command with multiple arguments' do let(:original_msg) { %(hello\n/labels ~foo ~"bar baz" label) } - let(:final_msg) { "hello\n" } + let(:final_msg) { "hello" } end end context 'when argument is not separated with a space' do it 'does not extract command' do msg = %(hello\n/labels~foo ~"bar baz" label\nworld) - commands = extractor.extract_commands(msg) + msg, commands = extractor.extract_commands(msg) expect(commands).to be_empty expect(msg).to eq %(hello\n/labels~foo ~"bar baz" label\nworld) @@ -152,7 +163,7 @@ describe Gitlab::SlashCommands::Extractor do it 'extracts command with multiple arguments and various prefixes' do msg = %(hello\n/power @user.name %9.10 ~"bar baz.2"\nworld) - commands = extractor.extract_commands(msg) + msg, commands = extractor.extract_commands(msg) expect(commands).to eq [['power', '@user.name %9.10 ~"bar baz.2"']] expect(msg).to eq "hello\nworld" @@ -160,15 +171,15 @@ describe Gitlab::SlashCommands::Extractor do it 'extracts multiple commands' do msg = %(hello\n/power @user.name %9.10 ~"bar baz.2" label\nworld\n/open) - commands = extractor.extract_commands(msg) + msg, commands = extractor.extract_commands(msg) expect(commands).to eq [['power', '@user.name %9.10 ~"bar baz.2" label'], ['open']] - expect(msg).to eq "hello\nworld\n" + expect(msg).to eq "hello\nworld" end it 'does not alter original content if no command is found' do msg = 'Fixes #123' - commands = extractor.extract_commands(msg) + msg, commands = extractor.extract_commands(msg) expect(commands).to be_empty expect(msg).to eq 'Fixes #123' @@ -177,7 +188,7 @@ describe Gitlab::SlashCommands::Extractor do it 'does not extract commands inside a blockcode' do msg = "Hello\r\n```\r\nThis is some text\r\n/close\r\n/assign @user\r\n```\r\n\r\nWorld" expected = msg.delete("\r") - commands = extractor.extract_commands(msg) + msg, commands = extractor.extract_commands(msg) expect(commands).to be_empty expect(msg).to eq expected @@ -186,7 +197,7 @@ describe Gitlab::SlashCommands::Extractor do it 'does not extract commands inside a blockquote' do msg = "Hello\r\n>>>\r\nThis is some text\r\n/close\r\n/assign @user\r\n>>>\r\n\r\nWorld" expected = msg.delete("\r") - commands = extractor.extract_commands(msg) + msg, commands = extractor.extract_commands(msg) expect(commands).to be_empty expect(msg).to eq expected @@ -195,7 +206,7 @@ describe Gitlab::SlashCommands::Extractor do it 'does not extract commands inside a HTML tag' do msg = "Hello\r\n
\r\nThis is some text\r\n/close\r\n/assign @user\r\n
\r\n\r\nWorld" expected = msg.delete("\r") - commands = extractor.extract_commands(msg) + msg, commands = extractor.extract_commands(msg) expect(commands).to be_empty expect(msg).to eq expected diff --git a/spec/services/notes/create_service_spec.rb b/spec/services/notes/create_service_spec.rb index 92dbccf0729..93885c84dc3 100644 --- a/spec/services/notes/create_service_spec.rb +++ b/spec/services/notes/create_service_spec.rb @@ -56,7 +56,7 @@ describe Notes::CreateService, services: true do it "creates regular note if emoji name is invalid" do opts = { - note: ':smile: moretext: ', + note: ':smile: moretext:', noteable_type: 'Issue', noteable_id: issue.id } diff --git a/spec/services/notes/slash_commands_service_spec.rb b/spec/services/notes/slash_commands_service_spec.rb index 5632ec09834..9a262fcf32f 100644 --- a/spec/services/notes/slash_commands_service_spec.rb +++ b/spec/services/notes/slash_commands_service_spec.rb @@ -12,7 +12,6 @@ describe Notes::SlashCommandsService, services: true do before do note.note = note_text - described_class.new(project, master).execute(note) end describe 'note with only command' do @@ -20,7 +19,10 @@ describe Notes::SlashCommandsService, services: true do let(:note_text) { %(/close\n/assign @#{assignee.username}") } it 'saves the note and does not alter the note text' do - expect(note.note).to eq note_text + content, command_params = service.extract_commands(note) + + expect(content).to eq note_text + expect(command_params).to be_empty end end end @@ -30,7 +32,10 @@ describe Notes::SlashCommandsService, services: true do let(:note_text) { %(HELLO\n/close\n/assign @#{assignee.username}\nWORLD) } it 'saves the note and does not alter the note text' do - expect(note.note).to eq note_text + content, command_params = service.extract_commands(note) + + expect(content).to eq note_text + expect(command_params).to be_empty end end end @@ -53,9 +58,10 @@ describe Notes::SlashCommandsService, services: true do end it 'closes noteable, sets labels, assigns, and sets milestone to noteable, and leave no note' do - described_class.new(project, master).execute(note) + content, command_params = service.extract_commands(note) + service.execute(command_params, note) - expect(note.note).to eq '' + expect(content).to eq '' expect(note.noteable).to be_closed expect(note.noteable.labels).to match_array(labels) expect(note.noteable.assignee).to eq(assignee) @@ -71,9 +77,10 @@ describe Notes::SlashCommandsService, services: true do let(:note_text) { '/open' } it 'opens the noteable, and leave no note' do - described_class.new(project, master).execute(note) + content, command_params = service.extract_commands(note) + service.execute(command_params, note) - expect(note.note).to eq '' + expect(content).to eq '' expect(note.noteable).to be_open end end @@ -86,9 +93,10 @@ describe Notes::SlashCommandsService, services: true do end it 'closes noteable, sets labels, assigns, and sets milestone to noteable' do - described_class.new(project, master).execute(note) + content, command_params = service.extract_commands(note) + service.execute(command_params, note) - expect(note.note).to eq "HELLO\nWORLD" + expect(content).to eq "HELLO\nWORLD" expect(note.noteable).to be_closed expect(note.noteable.labels).to match_array(labels) expect(note.noteable.assignee).to eq(assignee) @@ -104,9 +112,10 @@ describe Notes::SlashCommandsService, services: true do let(:note_text) { "HELLO\n/open\nWORLD" } it 'opens the noteable' do - described_class.new(project, master).execute(note) + content, command_params = service.extract_commands(note) + service.execute(command_params, note) - expect(note.note).to eq "HELLO\nWORLD" + expect(content).to eq "HELLO\nWORLD" expect(note.noteable).to be_open end end @@ -114,6 +123,8 @@ describe Notes::SlashCommandsService, services: true do end describe '#execute' do + let(:service) { described_class.new(project, master) } + it_behaves_like 'note on noteable that supports slash commands' do let(:note) { build(:note_on_issue, project: project) } end diff --git a/spec/services/slash_commands/interpret_service_spec.rb b/spec/services/slash_commands/interpret_service_spec.rb index 0cf77e53435..c20aa90ddde 100644 --- a/spec/services/slash_commands/interpret_service_spec.rb +++ b/spec/services/slash_commands/interpret_service_spec.rb @@ -12,141 +12,6 @@ describe SlashCommands::InterpretService, services: true do project.team << [user, :developer] end - describe '#command_names' do - subject do - described_class.command_names( - project: project, - noteable: issue, - current_user: user - ) - end - - it 'returns the basic known commands' do - is_expected.to match_array([ - :close, - :title, - :assign, :reassign, - :todo, - :subscribe, - :due_date, :due - ]) - end - - context 'when noteable is open' do - it 'includes the :close command' do - is_expected.to include(*[:close]) - end - end - - context 'when noteable is closed' do - before do - issue.close! - end - - it 'includes the :open, :reopen commands' do - is_expected.to include(*[:open, :reopen]) - end - end - - context 'when noteable has an assignee' do - before do - issue.update(assignee_id: user.id) - end - - it 'includes the :unassign, :remove_assignee commands' do - is_expected.to include(*[:unassign, :remove_assignee]) - end - end - - context 'when noteable has a milestone' do - before do - issue.update(milestone: milestone) - end - - it 'includes the :clear_milestone, :remove_milestone commands' do - is_expected.to include(*[:milestone, :clear_milestone, :remove_milestone]) - end - end - - context 'when project has a milestone' do - before do - milestone - end - - it 'includes the :milestone command' do - is_expected.to include(*[:milestone]) - end - end - - context 'when noteable has a label' do - before do - issue.update(label_ids: [bug.id]) - end - - it 'includes the :unlabel, :remove_labels, :remove_label, :clear_labels, :clear_label commands' do - is_expected.to include(*[:unlabel, :remove_labels, :remove_label, :clear_labels, :clear_label]) - end - end - - context 'when project has a label' do - before do - inprogress - end - - it 'includes the :labels, :label commands' do - is_expected.to include(*[:labels, :label]) - end - end - - context 'when user has no todo' do - it 'includes the :todo command' do - is_expected.to include(*[:todo]) - end - end - - context 'when user has a todo' do - before do - TodoService.new.mark_todo(issue, user) - end - - it 'includes the :done command' do - is_expected.to include(*[:done]) - end - end - - context 'when user is not subscribed' do - it 'includes the :subscribe command' do - is_expected.to include(*[:subscribe]) - end - end - - context 'when user is subscribed' do - before do - issue.subscribe(user) - end - - it 'includes the :unsubscribe command' do - is_expected.to include(*[:unsubscribe]) - end - end - - context 'when noteable has a no due date' do - it 'includes the :due_date, :due commands' do - is_expected.to include(*[:due_date, :due]) - end - end - - context 'when noteable has a due date' do - before do - issue.update(due_date: Date.today) - end - - it 'includes the :clear_due_date command' do - is_expected.to include(*[:due_date, :due, :clear_due_date]) - end - end - end - describe '#execute' do let(:service) { described_class.new(project, user) } let(:merge_request) { create(:merge_request, source_project: project) } @@ -154,60 +19,60 @@ describe SlashCommands::InterpretService, services: true do shared_examples 'open command' do it 'returns state_event: "open" if content contains /open' do issuable.close! - changes = service.execute(content, issuable) + _, updates = service.execute(content, issuable) - expect(changes).to eq(state_event: 'reopen') + expect(updates).to eq(state_event: 'reopen') end end shared_examples 'close command' do it 'returns state_event: "close" if content contains /open' do - changes = service.execute(content, issuable) + _, updates = service.execute(content, issuable) - expect(changes).to eq(state_event: 'close') + expect(updates).to eq(state_event: 'close') end end shared_examples 'title command' do it 'populates title: "A brand new title" if content contains /title A brand new title' do - changes = service.execute(content, issuable) + _, updates = service.execute(content, issuable) - expect(changes).to eq(title: 'A brand new title') + expect(updates).to eq(title: 'A brand new title') end end shared_examples 'assign command' do it 'fetches assignee and populates assignee_id if content contains /assign' do - changes = service.execute(content, issuable) + _, updates = service.execute(content, issuable) - expect(changes).to eq(assignee_id: user.id) + expect(updates).to eq(assignee_id: user.id) end end shared_examples 'unassign command' do it 'populates assignee_id: nil if content contains /unassign' do issuable.update(assignee_id: user.id) - changes = service.execute(content, issuable) + _, updates = service.execute(content, issuable) - expect(changes).to eq(assignee_id: nil) + expect(updates).to eq(assignee_id: nil) end end shared_examples 'milestone command' do it 'fetches milestone and populates milestone_id if content contains /milestone' do milestone # populate the milestone - changes = service.execute(content, issuable) + _, updates = service.execute(content, issuable) - expect(changes).to eq(milestone_id: milestone.id) + expect(updates).to eq(milestone_id: milestone.id) end end shared_examples 'clear_milestone command' do it 'populates milestone_id: nil if content contains /clear_milestone' do issuable.update(milestone_id: milestone.id) - changes = service.execute(content, issuable) + _, updates = service.execute(content, issuable) - expect(changes).to eq(milestone_id: nil) + expect(updates).to eq(milestone_id: nil) end end @@ -215,86 +80,86 @@ describe SlashCommands::InterpretService, services: true do it 'fetches label ids and populates add_label_ids if content contains /label' do bug # populate the label inprogress # populate the label - changes = service.execute(content, issuable) + _, updates = service.execute(content, issuable) - expect(changes).to eq(add_label_ids: [bug.id, inprogress.id]) + expect(updates).to eq(add_label_ids: [bug.id, inprogress.id]) end end shared_examples 'unlabel command' do it 'fetches label ids and populates remove_label_ids if content contains /unlabel' do issuable.update(label_ids: [inprogress.id]) # populate the label - changes = service.execute(content, issuable) + _, updates = service.execute(content, issuable) - expect(changes).to eq(remove_label_ids: [inprogress.id]) + expect(updates).to eq(remove_label_ids: [inprogress.id]) end end shared_examples 'clear_labels command' do it 'populates label_ids: [] if content contains /clear_labels' do issuable.update(label_ids: [inprogress.id]) # populate the label - changes = service.execute(content, issuable) + _, updates = service.execute(content, issuable) - expect(changes).to eq(label_ids: []) + expect(updates).to eq(label_ids: []) end end shared_examples 'todo command' do it 'populates todo_event: "add" if content contains /todo' do - changes = service.execute(content, issuable) + _, updates = service.execute(content, issuable) - expect(changes).to eq(todo_event: 'add') + expect(updates).to eq(todo_event: 'add') end end shared_examples 'done command' do it 'populates todo_event: "done" if content contains /done' do TodoService.new.mark_todo(issuable, user) - changes = service.execute(content, issuable) + _, updates = service.execute(content, issuable) - expect(changes).to eq(todo_event: 'done') + expect(updates).to eq(todo_event: 'done') end end shared_examples 'subscribe command' do it 'populates subscription_event: "subscribe" if content contains /subscribe' do - changes = service.execute(content, issuable) + _, updates = service.execute(content, issuable) - expect(changes).to eq(subscription_event: 'subscribe') + expect(updates).to eq(subscription_event: 'subscribe') end end shared_examples 'unsubscribe command' do it 'populates subscription_event: "unsubscribe" if content contains /unsubscribe' do issuable.subscribe(user) - changes = service.execute(content, issuable) + _, updates = service.execute(content, issuable) - expect(changes).to eq(subscription_event: 'unsubscribe') + expect(updates).to eq(subscription_event: 'unsubscribe') end end shared_examples 'due_date command' do it 'populates due_date: Date.new(2016, 8, 28) if content contains /due_date 2016-08-28' do - changes = service.execute(content, issuable) + _, updates = service.execute(content, issuable) - expect(changes).to eq(due_date: defined?(expected_date) ? expected_date : Date.new(2016, 8, 28)) + expect(updates).to eq(due_date: defined?(expected_date) ? expected_date : Date.new(2016, 8, 28)) end end shared_examples 'clear_due_date command' do it 'populates due_date: nil if content contains /clear_due_date' do issuable.update(due_date: Date.today) - changes = service.execute(content, issuable) + _, updates = service.execute(content, issuable) - expect(changes).to eq(due_date: nil) + expect(updates).to eq(due_date: nil) end end shared_examples 'empty command' do it 'populates {} if content contains an unsupported command' do - changes = service.execute(content, issuable) + _, updates = service.execute(content, issuable) - expect(changes).to be_empty + expect(updates).to be_empty end end