diff --git a/app/assets/javascripts/notes.js b/app/assets/javascripts/notes.js index 042fe44e1c64176f4b9ff136695c9d756430ba41..877e873af7bdcf00257f8eaf3dd23d74dd6c8cf3 100644 --- a/app/assets/javascripts/notes.js +++ b/app/assets/javascripts/notes.js @@ -59,6 +59,7 @@ export default class Notes { this.postComment = this.postComment.bind(this); this.clearFlashWrapper = this.clearFlash.bind(this); this.onHashChange = this.onHashChange.bind(this); + this.handleQuickActionsCommands = this.handleQuickActionsCommands.bind(this); this.notes_url = notes_url; this.note_ids = note_ids; @@ -292,16 +293,16 @@ export default class Notes { handleQuickActions(noteEntity) { var votesBlock; - if (noteEntity.commands_changes) { - if ('merge' in noteEntity.commands_changes) { + if (noteEntity.quick_actions_commands) { + if ('merge' in noteEntity.quick_actions_commands) { Notes.checkMergeRequestStatus(); } - if ('emoji_award' in noteEntity.commands_changes) { + if ('emoji_award' in noteEntity.quick_actions_commands) { votesBlock = $('.js-awards-block').eq(0); loadAwardsHandler().then((awardsHandler) => { - awardsHandler.addAwardToEmojiBar(votesBlock, noteEntity.commands_changes.emoji_award); + awardsHandler.addAwardToEmojiBar(votesBlock, noteEntity.quick_actions_commands.emoji_award); awardsHandler.scrollToAwards(); }).catch(() => { // ignore @@ -338,6 +339,20 @@ export default class Notes { $note.toggleClass('target', addTargetClass); } + handleQuickActionsCommands(noteEntity, $notesList) { + const quickActionsCommands = noteEntity.quick_actions_commands; + const hasQuickActionsCommands = !_.isEmpty(quickActionsCommands); + + if (hasQuickActionsCommands) { + $notesList.find('.system-note.being-posted').remove(); + this.addFlash('Commands applied', 'notice', this.parentTimeline.get(0)); + + if (!noteEntity.valid) { + this.refresh(); + } + } + } + /** * Render note in main comments area. * @@ -348,15 +363,9 @@ export default class Notes { return this.renderDiscussionNote(noteEntity, $form); } + this.handleQuickActionsCommands(noteEntity, $notesList); + if (!noteEntity.valid) { - if (noteEntity.errors.commands_only) { - if (noteEntity.commands_changes && - Object.keys(noteEntity.commands_changes).length > 0) { - $notesList.find('.system-note.being-posted').remove(); - } - this.addFlash(noteEntity.errors.commands_only, 'notice', this.parentTimeline.get(0)); - this.refresh(); - } return; } @@ -1549,7 +1558,7 @@ export default class Notes { this.reenableTargetFormSubmitButton(e); } - if (note.commands_changes) { + if (note.quick_actions_commands) { this.handleQuickActions(note); } diff --git a/app/assets/javascripts/notes/components/comment_form.vue b/app/assets/javascripts/notes/components/comment_form.vue index e594377bc4098dd4747b906c6f9e9d9e50e63093..3abccbce22fa7019afb882f5d3dfefbb1a2a6e87 100644 --- a/app/assets/javascripts/notes/components/comment_form.vue +++ b/app/assets/javascripts/notes/components/comment_form.vue @@ -139,15 +139,11 @@ this.restartPolling(); if (res.errors) { - if (res.errors.commands_only) { - this.discard(); - } else { - Flash( - 'Something went wrong while adding your comment. Please try again.', - 'alert', - this.$refs.commentForm, - ); - } + Flash( + 'Something went wrong while adding your comment. Please try again.', + 'alert', + this.$refs.commentForm, + ); } else { this.discard(); } diff --git a/app/assets/javascripts/notes/stores/actions.js b/app/assets/javascripts/notes/stores/actions.js index 085b18642baf4110e3521c2ca4147b1a3b2f818e..82278e76a669ddc2dec1baed672a83cbeb5c7b5e 100644 --- a/app/assets/javascripts/notes/stores/actions.js +++ b/app/assets/javascripts/notes/stores/actions.js @@ -61,6 +61,60 @@ export const createNewNote = ({ commit }, { endpoint, data }) => service export const removePlaceholderNotes = ({ commit }) => commit(types.REMOVE_PLACEHOLDER_NOTES); +const handleQuickActionsCommands = (noteEntity, noteData) => { + const quickActionsCommands = noteEntity.quick_actions_commands; + const hasQuickActionsCommands = !_.isEmpty(quickActionsCommands); + const quickActionsResults = noteEntity.quick_actions_results; + const hasQuickActionsResults = !_.isEmpty(quickActionsResults); + let quickActionsMessage = null; + let quickActionsMessageType = 'notice'; + + if (hasQuickActionsCommands || hasQuickActionsResults) { + quickActionsMessage = 'Commands applied'; + eTagPoll.makeRequest(); + $('.js-gfm-input').trigger('clear-commands-cache.atwho'); + + if (quickActionsCommands.emoji_award) { + const votesBlock = $('.js-awards-block').eq(0); + + loadAwardsHandler() + .then((awardsHandler) => { + awardsHandler.addAwardToEmojiBar(votesBlock, quickActionsCommands.emoji_award); + awardsHandler.scrollToAwards(); + }) + .catch(() => { + Flash( + 'Something went wrong while adding your award. Please try again.', + 'alert', + noteData.flashContainer, + ); + }); + } + + if (quickActionsCommands.spend_time != null + || quickActionsCommands.time_estimate != null) { + sidebarTimeTrackingEventHub.$emit('timeTrackingUpdated', noteEntity); + } + } + + if (quickActionsResults && quickActionsResults.create_branch) { + const createBranchResult = quickActionsResults.create_branch; + + if (createBranchResult.status === 'error') { + if (hasQuickActionsCommands) { + quickActionsMessage = quickActionsMessage.concat(`, but the branch '${createBranchResult.branch_name}' could not be created.`); + } else { + quickActionsMessage = `The branch '${createBranchResult.branch_name}' could not be created.`; + quickActionsMessageType = 'alert'; + } + } + } + + if (quickActionsMessage) { + Flash(quickActionsMessage, quickActionsMessageType, noteData.flashContainer); + } +}; + export const saveNote = ({ commit, dispatch }, noteData) => { const { note } = noteData.data.note; let placeholderText = note; @@ -92,42 +146,8 @@ export const saveNote = ({ commit, dispatch }, noteData) => { return dispatch(methodToDispatch, noteData) .then((res) => { - const { errors } = res; - const commandsChanges = res.commands_changes; - - if (hasQuickActions && errors && Object.keys(errors).length) { - eTagPoll.makeRequest(); - - $('.js-gfm-input').trigger('clear-commands-cache.atwho'); - Flash('Commands applied', 'notice', noteData.flashContainer); - } + handleQuickActionsCommands(res, noteData); - if (commandsChanges) { - if (commandsChanges.emoji_award) { - const votesBlock = $('.js-awards-block').eq(0); - - loadAwardsHandler() - .then((awardsHandler) => { - awardsHandler.addAwardToEmojiBar(votesBlock, commandsChanges.emoji_award); - awardsHandler.scrollToAwards(); - }) - .catch(() => { - Flash( - 'Something went wrong while adding your award. Please try again.', - 'alert', - noteData.flashContainer, - ); - }); - } - - if (commandsChanges.spend_time != null || commandsChanges.time_estimate != null) { - sidebarTimeTrackingEventHub.$emit('timeTrackingUpdated', res); - } - } - - if (errors && errors.commands_only) { - Flash(errors.commands_only, 'notice', noteData.flashContainer); - } commit(types.REMOVE_PLACEHOLDER_NOTES); return res; diff --git a/app/assets/javascripts/sidebar/components/time_tracking/sidebar_time_tracking.js b/app/assets/javascripts/sidebar/components/time_tracking/sidebar_time_tracking.js index d32fe4abc7d036b4e7b6b903cbfd0e616f48cc6e..7ddbcff49fd7426d8ec0dde3edbd3244bacb99f5 100644 --- a/app/assets/javascripts/sidebar/components/time_tracking/sidebar_time_tracking.js +++ b/app/assets/javascripts/sidebar/components/time_tracking/sidebar_time_tracking.js @@ -29,8 +29,8 @@ export default { const subscribedCommands = ['spend_time', 'time_estimate']; let changedCommands; if (data !== undefined) { - changedCommands = data.commands_changes - ? Object.keys(data.commands_changes) + changedCommands = data.quick_actions_commands + ? Object.keys(data.quick_actions_commands) : []; } else { changedCommands = []; diff --git a/app/controllers/concerns/notes_actions.rb b/app/controllers/concerns/notes_actions.rb index e82a5650935301b99f0fb423c110ada6a390a426..2fd86a4ad23bc1f54ba1029f3e75dc155d2d6bbe 100644 --- a/app/controllers/concerns/notes_actions.rb +++ b/app/controllers/concerns/notes_actions.rb @@ -89,7 +89,8 @@ def note_html(note) def note_json(note) attrs = { - commands_changes: note.commands_changes + quick_actions_commands: note.quick_actions_commands, + quick_actions_results: note.quick_actions_results } if note.persisted? diff --git a/app/controllers/projects/branches_controller.rb b/app/controllers/projects/branches_controller.rb index 56df9991fdacdf796cfd600dcc368ad19b9b25cc..cffdf4c83937fade1d064efac27e3d4df680b335 100644 --- a/app/controllers/projects/branches_controller.rb +++ b/app/controllers/projects/branches_controller.rb @@ -38,18 +38,18 @@ def recent end def create - branch_name = sanitize(strip_tags(params[:branch_name])) - branch_name = Addressable::URI.unescape(branch_name) - redirect_to_autodeploy = project.empty_repo? && project.deployment_platform.present? - - result = CreateBranchService.new(project, current_user) - .execute(branch_name, ref) - - if params[:issue_iid] - issue = IssuesFinder.new(current_user, project_id: @project.id).find_by(iid: params[:issue_iid]) - SystemNoteService.new_issue_branch(issue, @project, current_user, branch_name) if issue - end + issue = + if params[:issue_iid] + IssuesFinder.new(current_user, project_id: project.id).find_by(iid: params[:issue_iid]) + end + result = + if issue + Issues::CreateBranchService.new(project, current_user).execute(issue, params[:branch_name], ref) + else + ::CreateBranchService.new(project, current_user).execute(params[:branch_name], ref) + end + branch_name = result[:branch]&.name respond_to do |format| format.html do @@ -58,7 +58,7 @@ def create redirect_to url_to_autodeploy_setup(project, branch_name), notice: view_context.autodeploy_flash_notice(branch_name) else - redirect_to project_tree_path(@project, branch_name) + redirect_to project_tree_path(project, branch_name) end else @error = result[:message] diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb index 949eee51090ef86d2ce68dc722dc1111ce6fef4b..1f0431cecb2a71f01324585236ae25cb755db640 100644 --- a/app/controllers/projects/issues_controller.rb +++ b/app/controllers/projects/issues_controller.rb @@ -150,8 +150,7 @@ def related_branches def can_create_branch can_create = current_user && - can?(current_user, :push_code, @project) && - @issue.can_be_worked_on?(current_user) + Ability.can_create_branch_from_issue?(current_user, project, issuable) respond_to do |format| format.json do diff --git a/app/models/ability.rb b/app/models/ability.rb index 0b6bcbde5d94aa639a3a147d923d741eb2748eca..f4e6c8c45c658920c7b2be8a7639a64b994c0696 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -32,6 +32,11 @@ def can_edit_note?(user, note) allowed?(user, :edit_note, note) end + def can_create_branch_from_issue?(user, project, issue) + allowed?(user, :push_code, project) && + issue.can_be_worked_on?(user) + end + def allowed?(user, action, subject = :global, opts = {}) if subject.is_a?(Hash) opts, subject = subject, :global diff --git a/app/models/note.rb b/app/models/note.rb index a42843037e90d617b2ff93bc2310d71c91fc6d1f..caa32f1102839b484e27e6e9cecfcb4b0d1fa08a 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -48,7 +48,9 @@ def values attr_accessor :user_visible_reference_count # Attribute used to store the attributes that have been changed by quick actions. - attr_accessor :commands_changes + attr_accessor :quick_actions_commands + # Attribute used to store results of quick actions that don't act on the note's noteable. + attr_accessor :quick_actions_results # A special role that may be displayed on issuable's discussions attr_accessor :special_role diff --git a/app/services/create_branch_service.rb b/app/services/create_branch_service.rb index 1f7e04e443562d1549a4920b5650a44dc1ab097d..9ab1fb46d49d8b6d6bc6602e37ffb3e012edab3b 100644 --- a/app/services/create_branch_service.rb +++ b/app/services/create_branch_service.rb @@ -1,29 +1,33 @@ class CreateBranchService < BaseService + include ActionView::Helpers::SanitizeHelper + def execute(branch_name, ref, create_master_if_empty: true) create_master_branch if create_master_if_empty && project.empty_repo? + sanitized_branch_name = sanitize_branch_name_for(branch_name) + result = ValidateNewBranchService.new(project, current_user) - .execute(branch_name) + .execute(sanitized_branch_name) return result if result[:status] == :error - new_branch = repository.add_branch(current_user, branch_name, ref) + new_branch = repository.add_branch(current_user, sanitized_branch_name, ref) if new_branch - success(new_branch) + success(branch: new_branch) else - error('Invalid reference name') + error('Invalid reference name', nil, branch_name) end rescue Gitlab::Git::HooksService::PreReceiveError => ex - error(ex.message) - end - - def success(branch) - super().merge(branch: branch) + error(ex.message, nil, branch_name) end private + def error(message, http_status = nil, branch_name = nil) + super(message, http_status).merge(branch_name: branch_name) + end + def create_master_branch project.repository.create_file( current_user, @@ -33,4 +37,8 @@ def create_master_branch branch_name: 'master' ) end + + def sanitize_branch_name_for(branch_name) + Addressable::URI.unescape(sanitize(strip_tags(branch_name))) + end end diff --git a/app/services/issues/create_branch_service.rb b/app/services/issues/create_branch_service.rb new file mode 100644 index 0000000000000000000000000000000000000000..f5df44cd27250cbcf4203a86d9a8e68b41ea0002 --- /dev/null +++ b/app/services/issues/create_branch_service.rb @@ -0,0 +1,11 @@ +module Issues + class CreateBranchService < Issues::BaseService + def execute(issue, branch_name, ref) + ::CreateBranchService.new(project, current_user).execute(branch_name, ref).tap do |result| + if result[:status] == :success + SystemNoteService.new_issue_branch(issue, project, current_user, result[:branch].name) + end + end + end + end +end diff --git a/app/services/notes/create_service.rb b/app/services/notes/create_service.rb index 9ea28733f5fe99aa9b4e07ccc7ebc8ae753a3226..179a08d1331e26f6356a13586d5769f249692d5a 100644 --- a/app/services/notes/create_service.rb +++ b/app/services/notes/create_service.rb @@ -19,9 +19,9 @@ def execute if quick_actions_service.supported?(note) options = { merge_request_diff_head_sha: merge_request_diff_head_sha } - content, command_params = quick_actions_service.extract_commands(note, options) + content, commands, results = quick_actions_service.extract_commands(note, options) - only_commands = content.empty? + only_quick_actions = content.empty? && (commands.any? || results.any?) note.note = content end @@ -31,21 +31,14 @@ def execute NewNoteWorker.perform_async(note.id) end - if !only_commands && note.save + if !only_quick_actions && note.save todo_service.new_note(note, current_user) end - if command_params.present? - quick_actions_service.execute(command_params, note) + quick_actions_service.execute(commands, 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, 'Commands applied') - end - - note.commands_changes = command_params - end + note.quick_actions_commands = commands + note.quick_actions_results = results note end diff --git a/app/services/notes/quick_actions_service.rb b/app/services/notes/quick_actions_service.rb index a8d0cc15527694c7cf3bc1b58b4a233e08a8d568..f15b40a6f340da33eba4fcbffc7546bb142a4e71 100644 --- a/app/services/notes/quick_actions_service.rb +++ b/app/services/notes/quick_actions_service.rb @@ -27,7 +27,7 @@ def extract_commands(note, options = {}) end def execute(command_params, note) - return if command_params.empty? + return if command_params.blank? return unless supported?(note) self.class.noteable_update_service(note).new(project, current_user, command_params).execute(note.noteable) diff --git a/app/services/quick_actions/interpret_service.rb b/app/services/quick_actions/interpret_service.rb index 7332790b7b1219df747b463b4bb8f79b1c9cfe89..5bc3b07e5e9069ac595a37aacb856e8009abd1a9 100644 --- a/app/services/quick_actions/interpret_service.rb +++ b/app/services/quick_actions/interpret_service.rb @@ -15,11 +15,12 @@ def execute(content, issuable) @issuable = issuable @updates = {} + @results = {} content, commands = extractor.extract_commands(content, context) extract_updates(commands, context) - [content, @updates] + [content, @updates, @results] end # Takes a text and interprets the commands that are extracted from it. @@ -451,7 +452,7 @@ def extractor desc 'Set target branch' explanation do |branch_name| - "Sets target branch to #{branch_name}." + "Sets target branch to `#{branch_name}`." end params '' condition do @@ -466,6 +467,23 @@ def extractor @updates[:target_branch] = branch_name if project.repository.branch_exists?(branch_name) end + desc 'Creates a new branch' + explanation do |branch_name = nil| + "Creates a `#{branch_name || issuable.to_branch_name}` branch." + end + params '' + condition do + issuable.is_a?(Issue) && + issuable.persisted? && + Ability.can_create_branch_from_issue?(current_user, project, issuable) + end + command :create_branch do |branch_name = nil| + branch_name ||= issuable.to_branch_name + ref = project.default_branch || 'master' + result = Issues::CreateBranchService.new(project, current_user).execute(issuable, branch_name, ref) + @results[:create_branch] = result + end + desc 'Move issue from one column of the board to another' explanation do |target_list_name| label = find_label_references(target_list_name).first diff --git a/app/services/validate_new_branch_service.rb b/app/services/validate_new_branch_service.rb index 7d1ed768ee86f6abf1cfce94768d41b6fa99ba17..f67f9e1800ec43878369832ff7694d9465deafcd 100644 --- a/app/services/validate_new_branch_service.rb +++ b/app/services/validate_new_branch_service.rb @@ -5,15 +5,21 @@ def execute(branch_name) valid_branch = Gitlab::GitRefValidator.validate(branch_name) unless valid_branch - return error('Branch name is invalid') + return error('Branch name is invalid', nil, branch_name) end if project.repository.branch_exists?(branch_name) - return error('Branch already exists') + return error('Branch already exists', nil, branch_name) end success rescue Gitlab::Git::HooksService::PreReceiveError => ex - error(ex.message) + error(ex.message, nil, branch_name) + end + + private + + def error(message, http_status = nil, branch_name = nil) + super(message, http_status).merge(branch_name: branch_name) end end diff --git a/changelogs/unreleased/27801-new-branch-slash-command.yml b/changelogs/unreleased/27801-new-branch-slash-command.yml new file mode 100644 index 0000000000000000000000000000000000000000..36af7bbe7ae19fde6c3029f58b97adf95cd985fa --- /dev/null +++ b/changelogs/unreleased/27801-new-branch-slash-command.yml @@ -0,0 +1,4 @@ +--- +title: Add a new /create_branch slash command to create a branch from an issue. +merge_request: 10384 +author: Nur Muhammad Bin Sirat diff --git a/doc/user/project/quick_actions.md b/doc/user/project/quick_actions.md index 6f729954bc8a0a071f4c766aa753772b4ea40c3c..b9c351126ea21e5a37fffdf89df5bd7f969f2cc0 100644 --- a/doc/user/project/quick_actions.md +++ b/doc/user/project/quick_actions.md @@ -15,9 +15,9 @@ do. | `/close` | Close the issue or merge request | | `/reopen` | Reopen the issue or merge request | | `/merge` | Merge (when pipeline succeeds) | -| `/title ` | Change title | -| `/assign @user1 @user2 ` | Add assignee(s) | -| `/reassign @user1 @user2 ` | Change assignee(s) | +| `/title New title` | Change title | +| `/assign @user1 @user2` | Assign the issue or merge request to @user1 and @user2. You can also use `me` to assign the issue or merge request to yourself. | +| `/reassign @user1 @user2` | Assign the issue or merge request to @user1 and @user2. You can also use `me` to assign the issue or merge request to yourself. | | `/unassign @user1 @user2` | Remove all or specific assignee(s) | | `/milestone %milestone` | Set milestone | | `/remove_milestone` | Remove milestone | @@ -28,14 +28,15 @@ do. | `/done` | Mark todo as done | | `/subscribe` | Subscribe | | `/unsubscribe` | Unsubscribe | -| /due <in 2 days | this Friday | December 31st> | Set due date | -| `/remove_due_date` | Remove due date | +| /due <in 2 days | this Friday | December 31st> | Set a due date | +| `/remove_due_date` | Remove the due date | | `/wip` | Toggle the Work In Progress status | | /estimate <1w 3d 2h 14m> | Set time estimate | | `/remove_estimate` | Remove estimated time | | /spend <time(1h 30m | -1h 5m)> <date(YYYY-MM-DD)> | Add or subtract spent time; optionally, specify the date that time was spent on | | `/remove_time_spent` | Remove time spent | -| `/target_branch ` | Set target branch for current merge request | +| `/target_branch branch-name` | Set target branch for the current merge request | +| `/create_branch branch-name` | Create a new branch. If you don't pass a branch name, it automatically generates it based on the issue IID and title. | | `/award :emoji:` | Toggle award for :emoji: | | `/weight <1-9>` | Set the weight of the issue | | `/clear_weight` | Clears the issue weight | diff --git a/lib/gitlab/email/handler/create_note_handler.rb b/lib/gitlab/email/handler/create_note_handler.rb index 8eea33b9ab512ce2202b4ecb0d90331d2b854c3c..5711bf2deca4738f83a51f7912a1e1f5996a7fbf 100644 --- a/lib/gitlab/email/handler/create_note_handler.rb +++ b/lib/gitlab/email/handler/create_note_handler.rb @@ -44,6 +44,16 @@ def sent_notification def create_note sent_notification.create_reply(message) end + + def note_contains_only_quick_actions?(note) + note.note.empty? && (note.quick_actions_commands.any? || note.quick_actions_results.any?) + end + + def verify_record!(record:, invalid_exception:, record_name:) + return if note_contains_only_quick_actions?(record) + + super + end end end end diff --git a/lib/gitlab/email/handler/reply_processing.rb b/lib/gitlab/email/handler/reply_processing.rb index 32c5caf93e8118af01dbe3b41962b4301b4d910f..41f4dffb0fe506afdaf65d20850ca4023f000fc5 100644 --- a/lib/gitlab/email/handler/reply_processing.rb +++ b/lib/gitlab/email/handler/reply_processing.rb @@ -38,7 +38,6 @@ def validate_permission!(permission) def verify_record!(record:, invalid_exception:, record_name:) return if record.persisted? - return if record.errors.key?(:commands_only) error_title = "The #{record_name} could not be created for the following reasons:" diff --git a/spec/controllers/projects/branches_controller_spec.rb b/spec/controllers/projects/branches_controller_spec.rb index b22ec247cfd6ef30144189f29c0524a19514e268..08f6f2ea66d5f700528f7dd833034c38cc9bf9ec 100644 --- a/spec/controllers/projects/branches_controller_spec.rb +++ b/spec/controllers/projects/branches_controller_spec.rb @@ -101,8 +101,7 @@ it 'redirects to newly created branch' do result = { status: :success, branch: double(name: branch) } - expect_any_instance_of(CreateBranchService).to receive(:execute).and_return(result) - expect(SystemNoteService).to receive(:new_issue_branch).and_return(true) + expect_any_instance_of(Issues::CreateBranchService).to receive(:execute).and_return(result) post :create, namespace_id: project.namespace.to_param, @@ -152,8 +151,7 @@ create(:cluster, :provided_by_gcp, projects: [project]) - expect_any_instance_of(CreateBranchService).to receive(:execute).and_return(result) - expect(SystemNoteService).to receive(:new_issue_branch).and_return(true) + expect_any_instance_of(Issues::CreateBranchService).to receive(:execute).and_return(result) post :create, namespace_id: project.namespace.to_param, diff --git a/spec/features/issues/user_uses_slash_commands_spec.rb b/spec/features/issues/user_uses_slash_commands_spec.rb index b0babf005866e83fab9c60d0854a93312de04688..dea4880d7337deee3485d45610b85df9892ee7b1 100644 --- a/spec/features/issues/user_uses_slash_commands_spec.rb +++ b/spec/features/issues/user_uses_slash_commands_spec.rb @@ -340,5 +340,49 @@ end end end + + describe 'branch creation' do + let(:project) { create(:project, :public, :repository) } + let(:issue) { create(:issue, project: project) } + + context 'when the user can create a branch' do + it 'creates the branch' do + write_note("/create_branch") + + expect(page).not_to have_content '/create_branch' + expect(page).to have_content 'Commands applied' + expect(page).to have_content "created branch #{issue.to_branch_name}" + end + end + + context 'when the user specifies an invalid branch name' do + it 'creates the branch' do + write_note("/create_branch A New Feature") + + expect(page).not_to have_content '/create_branch' + expect(page).to have_content "The branch 'A New Feature' could not be created." + end + end + + context 'when the user cannot create a branch' do + let(:guest) { create(:user) } + + before do + project.add_guest(guest) + gitlab_sign_out + sign_in(guest) + visit project_issue_path(project, issue) + end + + it 'does not create a branch' do + write_note("/create_branch") + + expect(page).not_to have_content '/create_branch' + expect(page).not_to have_content 'Commands applied' + + expect(project.repository.branch_names).not_to include(issue.to_branch_name) + end + end + end end end diff --git a/spec/fixtures/emails/commands_only_reply.eml b/spec/fixtures/emails/only_quick_actions_reply.eml similarity index 100% rename from spec/fixtures/emails/commands_only_reply.eml rename to spec/fixtures/emails/only_quick_actions_reply.eml diff --git a/spec/fixtures/emails/commands_in_reply.eml b/spec/fixtures/emails/quick_actions_in_reply.eml similarity index 100% rename from spec/fixtures/emails/commands_in_reply.eml rename to spec/fixtures/emails/quick_actions_in_reply.eml diff --git a/spec/javascripts/notes_spec.js b/spec/javascripts/notes_spec.js index e09b8dc7fc5450308419d945c0809cb721753fd8..22ffc0282585a709de4ad79a445a6cc1a6e67feb 100644 --- a/spec/javascripts/notes_spec.js +++ b/spec/javascripts/notes_spec.js @@ -550,16 +550,13 @@ import '~/notes'; }); }); - describe('postComment with Slash commands', () => { + describe('postComment with Quick actions', () => { const sampleComment = '/assign @root\n/award :100:'; const note = { - commands_changes: { + quick_actions_commands: { assignee_id: 1, emoji_award: '100' }, - errors: { - commands_only: ['Commands applied'] - }, valid: false }; let $form; @@ -583,7 +580,7 @@ import '~/notes'; $form.find('textarea.js-note-text').val(sampleComment); }); - it('should remove slash command placeholder when comment with slash commands is done posting', () => { + it('should remove quick action placeholder when comment with quick actions is done posting', () => { const deferred = $.Deferred(); spyOn($, 'ajax').and.returnValue(deferred.promise()); spyOn(gl.awardsHandler, 'addAwardToEmojiBar').and.callThrough(); diff --git a/spec/lib/gitlab/email/handler/create_note_handler_spec.rb b/spec/lib/gitlab/email/handler/create_note_handler_spec.rb index d0fa16ce4d17b3f407751f2a4c3213a582c2ce6b..b1f17adbd31551359f2ea8380b7aefd06557680f 100644 --- a/spec/lib/gitlab/email/handler/create_note_handler_spec.rb +++ b/spec/lib/gitlab/email/handler/create_note_handler_spec.rb @@ -56,7 +56,7 @@ end context 'because the note was commands only' do - let!(:email_raw) { fixture_file("emails/commands_only_reply.eml") } + let!(:email_raw) { fixture_file("emails/only_quick_actions_reply.eml") } context 'and current user cannot update noteable' do it 'raises a CommandsOnlyNoteError' do @@ -83,7 +83,7 @@ end context 'when the note contains quick actions' do - let!(:email_raw) { fixture_file("emails/commands_in_reply.eml") } + let!(:email_raw) { fixture_file("emails/quick_actions_in_reply.eml") } context 'and current user cannot update noteable' do it 'post a note and does not update the noteable' do diff --git a/spec/models/ability_spec.rb b/spec/models/ability_spec.rb index 71aa51e185713182ca0943fefc682f30e72d0d72..ddc0342e3a208e8b5c1f98b29278f05138d95c64 100644 --- a/spec/models/ability_spec.rb +++ b/spec/models/ability_spec.rb @@ -267,4 +267,25 @@ def users_for_snippet(snippet) end end end + + describe '.can_create_branch_from_issue?' do + let(:project) { create(:project) } + let(:issue) { create(:issue, project: project) } + + context 'users with different access' do + it 'returns true for developer' do + developer = create(:user) + project.add_developer(developer) + + expect(described_class.can_create_branch_from_issue?(developer, project, issue)).to eq(true) + end + + it 'returns false for guest' do + guest = create(:user) + project.add_guest(guest) + + expect(described_class.can_create_branch_from_issue?(guest, project, issue)).to eq(false) + end + end + end end diff --git a/spec/services/issues/create_branch_service_spec.rb b/spec/services/issues/create_branch_service_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..ae1f09b2cd8b0d4133492ed3e964c5bc020c983c --- /dev/null +++ b/spec/services/issues/create_branch_service_spec.rb @@ -0,0 +1,27 @@ +require 'spec_helper' + +describe Issues::CreateBranchService, services: true do + let(:user) { create(:user) } + let(:service) { described_class.new(project, user) } + + describe '#execute' do + context 'when repository is empty' do + let(:project) { create(:project, :repository) } + let(:issue) { create(:issue, project: project) } + + it 'creates a branch if the branch name is valid and adds a system note' do + result = service.execute(issue, 'my-issue-branch', 'master') + + expect(result[:status]).to eq(:success) + expect(issue.notes.last.note).to include('created branch [`my-issue-branch`]') + end + + it 'does not create a branch if branch name is invalid' do + result = service.execute(issue, 'NOT valid name', 'master') + + expect(result[:status]).to eq(:error) + expect(SystemNoteService).not_to receive(:new_issue_branch) + end + end + end +end diff --git a/spec/services/notes/create_service_spec.rb b/spec/services/notes/create_service_spec.rb index 661d26946e79476bb4de6f0796230417d85ccd0a..5a2899724b2009490a899582150e9e6b0ab556b3 100644 --- a/spec/services/notes/create_service_spec.rb +++ b/spec/services/notes/create_service_spec.rb @@ -58,7 +58,7 @@ end describe 'note with commands' do - describe '/close, /label, /assign & /milestone' do + describe '/close, /label & /assign' do let(:note_text) { %(HELLO\n/close\n/assign @#{user.username}\nWORLD) } it 'saves the note and does not alter the note text' do diff --git a/spec/services/notes/quick_actions_service_spec.rb b/spec/services/notes/quick_actions_service_spec.rb index 424cc9694d36f60dd60b0902abad63f56cb15361..d977181f262bbd40ad903b925a7c2e9a8877758d 100644 --- a/spec/services/notes/quick_actions_service_spec.rb +++ b/spec/services/notes/quick_actions_service_spec.rb @@ -23,10 +23,10 @@ let(:note_text) { %(/close\n/assign @#{assignee.username}") } it 'saves the note and does not alter the note text' do - content, command_params = service.extract_commands(note) + content, commands, _results = service.extract_commands(note) expect(content).to eq note_text - expect(command_params).to be_empty + expect(commands).to be_empty end end end @@ -36,10 +36,10 @@ let(:note_text) { %(HELLO\n/close\n/assign @#{assignee.username}\nWORLD) } it 'saves the note and does not alter the note text' do - content, command_params = service.extract_commands(note) + content, commands, _results = service.extract_commands(note) expect(content).to eq note_text - expect(command_params).to be_empty + expect(commands).to be_empty end end end @@ -62,8 +62,8 @@ end it 'closes noteable, sets labels, assigns, and sets milestone to noteable, and leave no note' do - content, command_params = service.extract_commands(note) - service.execute(command_params, note) + content, commands, _results = service.extract_commands(note) + service.execute(commands, note) expect(content).to eq '' expect(note.noteable).to be_closed @@ -81,8 +81,8 @@ let(:note_text) { '/reopen' } it 'opens the noteable, and leave no note' do - content, command_params = service.extract_commands(note) - service.execute(command_params, note) + content, commands, _results = service.extract_commands(note) + service.execute(commands, note) expect(content).to eq '' expect(note.noteable).to be_open @@ -93,8 +93,8 @@ let(:note_text) { '/spend 1h' } it 'updates the spent time on the noteable' do - content, command_params = service.extract_commands(note) - service.execute(command_params, note) + content, commands, _results = service.extract_commands(note) + service.execute(commands, note) expect(content).to eq '' expect(note.noteable.time_spent).to eq(3600) @@ -109,8 +109,8 @@ end it 'closes noteable, sets labels, assigns, and sets milestone to noteable' do - content, command_params = service.extract_commands(note) - service.execute(command_params, note) + content, commands, _results = service.extract_commands(note) + service.execute(commands, note) expect(content).to eq "HELLO\nWORLD" expect(note.noteable).to be_closed @@ -128,8 +128,8 @@ let(:note_text) { "HELLO\n/reopen\nWORLD" } it 'opens the noteable' do - content, command_params = service.extract_commands(note) - service.execute(command_params, note) + content, commands, _results = service.extract_commands(note) + service.execute(commands, note) expect(content).to eq "HELLO\nWORLD" expect(note.noteable).to be_open @@ -242,8 +242,8 @@ end it 'adds only one assignee from the list' do - _, command_params = service.extract_commands(note) - service.execute(command_params, note) + _content, commands, _results = service.extract_commands(note) + service.execute(commands, note) expect(note.noteable.assignees.count).to eq(2) end diff --git a/spec/services/quick_actions/interpret_service_spec.rb b/spec/services/quick_actions/interpret_service_spec.rb index f54cb1f965bbc4c17ca0e5aa692073d11de4c130..d1591c2239d2fb50874d675b371f18884aef6452 100644 --- a/spec/services/quick_actions/interpret_service_spec.rb +++ b/spec/services/quick_actions/interpret_service_spec.rb @@ -341,6 +341,36 @@ end end + shared_examples 'create branch command' do + context 'without params' do + it 'creates a new branch with issue title as branch name' do + _content, updates, results = service.execute('/create_branch', issuable) + + expect(project.repository.branch_names).to include(issuable.to_branch_name) + expect(updates).to be_empty + expect(results[:create_branch]).to include({ status: :success }) + end + end + + context 'with params' do + it 'creates a new branch if the params is a valid branch name' do + _content, updates, results = service.execute("/create_branch #{valid_branch_name}", issuable) + + expect(project.repository.branch_names).to include(valid_branch_name) + expect(updates).to be_empty + expect(results[:create_branch]).to include({ status: :success }) + end + + it 'does not create a new branch if the params is an invalid branch name' do + _content, updates, results = service.execute("/create_branch #{invalid_branch_name}", issuable) + + expect(project.repository.branch_names).not_to include(invalid_branch_name) + expect(updates).to be_empty + expect(results[:create_branch]).to include({ status: :error }) + end + end + end + it_behaves_like 'reopen command' do let(:content) { '/reopen' } let(:issuable) { issue } @@ -995,6 +1025,35 @@ end end + context '/create_branch command' do + let(:project) { create(:project, :repository) } + let(:service) { described_class.new(project, developer) } + let(:issue) { create(:issue, title: 'Some Big Issue', project: project) } + let(:valid_branch_name) { 'valid/branch-Name' } + + it_behaves_like 'create branch command' do + let(:issuable) { issue } + let(:invalid_branch_name) { 'Invalid Name For A Branch' } + end + + context 'unprivileged user' do + let(:other_user) { create(:user) } + let(:service) { described_class.new(project, other_user)} + + it_behaves_like 'empty command' do + let(:content) { "/create_branch #{valid_branch_name}" } + let(:issuable) { issue } + end + end + + context 'on merge requests' do + it_behaves_like 'empty command' do + let(:content) { "/create_branch #{valid_branch_name}" } + let(:issuable) { create(:merge_request, source_project: project) } + end + end + end + context '/board_move command' do let(:todo) { create(:label, project: project, title: 'To Do') } let(:inreview) { create(:label, project: project, title: 'In Review') } @@ -1255,12 +1314,36 @@ end describe 'target branch command' do - let(:content) { '/target_branch my-feature ' } + let(:content) { '/target_branch my-feature' } it 'includes the branch name' do _, explanations = service.explain(content, merge_request) - expect(explanations).to eq(['Sets target branch to my-feature.']) + expect(explanations).to eq(['Sets target branch to `my-feature`.']) + end + end + + describe 'create branch command' do + let(:project) { create(:project, :repository, :public) } + + context 'without a branch name' do + let(:content) { '/create_branch' } + + it 'includes the branch name' do + _, explanations = service.explain(content, issue) + + expect(explanations).to eq(["Creates a `#{issue.to_branch_name}` branch."]) + end + end + + context 'with a branch name' do + let(:content) { '/create_branch my-branch' } + + it 'includes the branch name' do + _, explanations = service.explain(content, issue) + + expect(explanations).to eq(["Creates a `my-branch` branch."]) + end end end