diff options
author | Rémy Coutable <remy@rymai.me> | 2017-11-22 22:48:50 +0100 |
---|---|---|
committer | Rémy Coutable <remy@rymai.me> | 2017-12-18 11:57:17 +0100 |
commit | 60c63c5bffabff911b0f314562ae45c6559e28d8 (patch) | |
tree | 8f4f0e74ac10e49d6894e286566822c80a80e6fd | |
parent | c5ec1d3c37884c7250eebc4b68dcf975ed64e803 (diff) | |
download | gitlab-ce-60c63c5bffabff911b0f314562ae45c6559e28d8.tar.gz |
Simplify quick actions JS handling
Stop putting content in `note.errors`, instead set
`note.quick_actions_commands` and `note.quick_actions_results`,
and display a notice message on the frontend when branch cannot be created.
Signed-off-by: Rémy Coutable <remy@rymai.me>
16 files changed, 123 insertions, 72 deletions
diff --git a/app/assets/javascripts/notes.js b/app/assets/javascripts/notes.js index 1556cffb1ba..33d869caefc 100644 --- a/app/assets/javascripts/notes.js +++ b/app/assets/javascripts/notes.js @@ -292,16 +292,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 @@ -348,15 +348,38 @@ export default class Notes { return this.renderDiscussionNote(noteEntity, $form); } + const quickActionsCommands = noteEntity.quick_actions_commands; + const hasQuickActionsCommands = quickActionsCommands && Object.keys(quickActionsCommands).length; + let quickActionsMessage = null; + let quickActionsMessageType = 'notice'; + + // If the note is invalid with quickActionsCommands, that means it only contained quick actions 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(); + if (hasQuickActionsCommands) { + quickActionsMessage = 'Commands applied'; + } + + const quickActionsResults = noteEntity.quick_actions_results; + + if (quickActionsResults && quickActionsResults.create_branch) { + const createBranchResult = quickActionsResults.create_branch; + + if (createBranchResult.status === 'error') { + if (quickActionsMessage) { + quickActionsMessage = `Commands applied, but the branch '${createBranchResult.branch_name}' could not be created.`; + } else { + quickActionsMessage = `The branch '${createBranchResult.branch_name}' could not be created!`; + quickActionsMessageType = 'alert'; + } } - this.addFlash(noteEntity.errors.commands_only.join(", "), 'notice', this.parentTimeline.get(0)); + } + + if (quickActionsMessage) { + this.addFlash(quickActionsMessage, quickActionsMessageType, this.parentTimeline.get(0)); this.refresh(); + $notesList.find('.system-note.being-posted').remove(); } + return; } @@ -1549,7 +1572,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 e594377bc40..3abccbce22f 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 085b18642ba..7d012a78854 100644 --- a/app/assets/javascripts/notes/stores/actions.js +++ b/app/assets/javascripts/notes/stores/actions.js @@ -92,23 +92,45 @@ export const saveNote = ({ commit, dispatch }, noteData) => { return dispatch(methodToDispatch, noteData) .then((res) => { - const { errors } = res; - const commandsChanges = res.commands_changes; + const quickActionsCommands = res.quick_actions_commands; + const hasQuickActionsCommands = quickActionsCommands + && Object.keys(quickActionsCommands).length; + let quickActionsMessage = null; + let quickActionsMessageType = 'notice'; + + // If the note is invalid with quickActionsCommands, that means it only + // contained quick actions + if (!res.valid) { + if (hasQuickActionsCommands) { + quickActionsMessage = 'Commands applied'; + } - if (hasQuickActions && errors && Object.keys(errors).length) { - eTagPoll.makeRequest(); + const quickActionsResults = res.quick_actions_results; + + if (quickActionsResults && quickActionsResults.create_branch) { + const createBranchResult = quickActionsResults.create_branch; + + if (createBranchResult.status === 'error') { + if (quickActionsMessage) { + quickActionsMessage = `Commands applied, but the branch '${createBranchResult.branch_name}' could not be created.`; + } else { + quickActionsMessage = `The branch '${createBranchResult.branch_name}' could not be created!`; + quickActionsMessageType = 'alert'; + } + } else { + quickActionsMessage = 'Commands applied'; + } + } - $('.js-gfm-input').trigger('clear-commands-cache.atwho'); - Flash('Commands applied', 'notice', noteData.flashContainer); - } + if (quickActionsMessage) { + Flash(quickActionsMessage, quickActionsMessageType, noteData.flashContainer); - if (commandsChanges) { - if (commandsChanges.emoji_award) { - const votesBlock = $('.js-awards-block').eq(0); + if (quickActionsCommands.emoji_award) { + const votesBlock = $('.js-awards-block').eq(0); - loadAwardsHandler() + loadAwardsHandler() .then((awardsHandler) => { - awardsHandler.addAwardToEmojiBar(votesBlock, commandsChanges.emoji_award); + awardsHandler.addAwardToEmojiBar(votesBlock, quickActionsCommands.emoji_award); awardsHandler.scrollToAwards(); }) .catch(() => { @@ -118,16 +140,21 @@ export const saveNote = ({ commit, dispatch }, noteData) => { noteData.flashContainer, ); }); - } + } - if (commandsChanges.spend_time != null || commandsChanges.time_estimate != null) { - sidebarTimeTrackingEventHub.$emit('timeTrackingUpdated', res); + if (quickActionsCommands.spend_time != null + || quickActionsCommands.time_estimate != null) { + sidebarTimeTrackingEventHub.$emit('timeTrackingUpdated', res); + } } } - if (errors && errors.commands_only) { - Flash(errors.commands_only, 'notice', noteData.flashContainer); + if (hasQuickActionsCommands) { + eTagPoll.makeRequest(); + + $('.js-gfm-input').trigger('clear-commands-cache.atwho'); } + 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 d32fe4abc7d..7ddbcff49fd 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 e82a5650935..2fd86a4ad23 100644 --- a/app/controllers/concerns/notes_actions.rb +++ b/app/controllers/concerns/notes_actions.rb @@ -89,7 +89,8 @@ module NotesActions 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/models/note.rb b/app/models/note.rb index 184fbd5f5ae..b201050bb57 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -45,7 +45,9 @@ class Note < ActiveRecord::Base 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 e33d549486a..7f2c7bbb5d0 100644 --- a/app/services/create_branch_service.rb +++ b/app/services/create_branch_service.rb @@ -14,20 +14,20 @@ class CreateBranchService < BaseService 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, diff --git a/app/services/notes/create_service.rb b/app/services/notes/create_service.rb index 47e20f5fe95..179a08d1331 100644 --- a/app/services/notes/create_service.rb +++ b/app/services/notes/create_service.rb @@ -37,18 +37,8 @@ module Notes 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_quick_actions - note.errors.add(:commands_only, 'Commands applied') - end - - create_branch_result = results&.delete(:create_branch) - if create_branch_result&.fetch(:status) == :error - note.errors.add(:commands_only, "Error creating branch: #{create_branch_result.fetch(:message)}") - end - - note.commands_changes = commands + note.quick_actions_commands = commands + note.quick_actions_results = results note end diff --git a/app/services/validate_new_branch_service.rb b/app/services/validate_new_branch_service.rb index 7d1ed768ee8..f67f9e1800e 100644 --- a/app/services/validate_new_branch_service.rb +++ b/app/services/validate_new_branch_service.rb @@ -5,15 +5,21 @@ class ValidateNewBranchService < BaseService 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/lib/gitlab/email/handler/create_note_handler.rb b/lib/gitlab/email/handler/create_note_handler.rb index 8eea33b9ab5..5711bf2deca 100644 --- a/lib/gitlab/email/handler/create_note_handler.rb +++ b/lib/gitlab/email/handler/create_note_handler.rb @@ -44,6 +44,16 @@ module Gitlab 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 32c5caf93e8..41f4dffb0fe 100644 --- a/lib/gitlab/email/handler/reply_processing.rb +++ b/lib/gitlab/email/handler/reply_processing.rb @@ -38,7 +38,6 @@ module Gitlab 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/features/issues/user_uses_slash_commands_spec.rb b/spec/features/issues/user_uses_slash_commands_spec.rb index 8ec4fe90849..761ef12609e 100644 --- a/spec/features/issues/user_uses_slash_commands_spec.rb +++ b/spec/features/issues/user_uses_slash_commands_spec.rb @@ -284,7 +284,7 @@ feature 'Issues > User uses quick actions', :js do write_note("/create_branch A New Feature") expect(page).not_to have_content '/create_branch' - expect(page).to have_content 'Branch name is invalid' + expect(page).to have_content "The branch 'A New Feature' could not be created!" end end diff --git a/spec/fixtures/emails/commands_only_reply.eml b/spec/fixtures/emails/only_quick_actions_reply.eml index 2d2e2f94290..2d2e2f94290 100644 --- a/spec/fixtures/emails/commands_only_reply.eml +++ b/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 index 712f6f797b4..712f6f797b4 100644 --- a/spec/fixtures/emails/commands_in_reply.eml +++ b/spec/fixtures/emails/quick_actions_in_reply.eml diff --git a/spec/javascripts/notes_spec.js b/spec/javascripts/notes_spec.js index e09b8dc7fc5..22ffc028258 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 d0fa16ce4d1..b1f17adbd31 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 @@ describe Gitlab::Email::Handler::CreateNoteHandler do 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 @@ describe Gitlab::Email::Handler::CreateNoteHandler do 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 |