Can apply suggestions in batches.

app/assets/javascripts/api.js

@@ -38,6 +38,7 @@ const Api = {
   userPostStatusPath: '/api/:version/user/status',
   commitPath: '/api/:version/projects/:id/repository/commits',
   applySuggestionPath: '/api/:version/suggestions/:id/apply',
+  applySuggestionBatchPath: '/api/:version/suggestions/batch/apply?batch_ids=:batch_ids',
   commitPipelinesPath: '/:project_id/commit/:sha/pipelines',
   branchSinglePath: '/api/:version/projects/:id/repository/branches/:branch',
   createBranchPath: '/api/:version/projects/:id/repository/branches',
@@ -294,6 +295,15 @@ const Api = {
     return axios.put(url);
   },
 
+  applySuggestionBatch(batchIds) {
+    const batchIdsStr = batchIds.join(',');
+    const url = Api.buildUrl(Api.applySuggestionBatchPath).replace(
+      ':batch_ids',
+      encodeURIComponent(batchIdsStr),
+    );
+    return axios.put(url);
+  },
+
   commitPipelines(projectId, sha) {
     const encodedProjectId = projectId
       .split('/')

app/assets/javascripts/notes/components/note_body.vue

 <script>
-import { mapActions } from 'vuex';
+import { mapActions, mapGetters } from 'vuex';
 import $ from 'jquery';
 import '~/behaviors/markdown/render_gfm';
 import getDiscussion from 'ee_else_ce/notes/mixins/get_discussion';
@@ -45,6 +45,7 @@ export default {
     },
   },
   computed: {
+    ...mapGetters(['batchSuggestionIds']),
     noteBody() {
       return this.note.note;
     },
@@ -74,7 +75,13 @@ export default {
     }
   },
   methods: {
-    ...mapActions(['submitSuggestion']),
+    ...mapActions([
+      'submitSuggestion',
+      'submitSuggestionBatch',
+      'pushSuggestionToBatch',
+      'popSuggestionFromBatch',
+      'clearSuggestionBatch',
+    ]),
     renderGFM() {
       $(this.$refs['note-body']).renderGFM();
     },
@@ -91,6 +98,17 @@ export default {
         callback,
       );
     },
+    applySuggestionBatch({ flashContainer }) {
+      return this.submitSuggestionBatch(flashContainer);
+    },
+    addSuggestionToBatch(suggestionId) {
+      const { discussion_id: discussionId, id: noteId } = this.note;
+
+      this.pushSuggestionToBatch({ suggestionId, discussionId, noteId });
+    },
+    removeSuggestionFromBatch(suggestionId) {
+      this.popSuggestionFromBatch(suggestionId);
+    },
   },
 };
 </script>
@@ -100,10 +118,14 @@ export default {
     <suggestions
       v-if="hasSuggestion && !isEditing"
       :suggestions="note.suggestions"
+      :batch-suggestion-ids="batchSuggestionIds"
       :note-html="note.note_html"
       :line-type="lineType"
       :help-page-path="helpPagePath"
       @apply="applySuggestion"
+      @applyBatch="applySuggestionBatch"
+      @addToBatch="addSuggestionToBatch"
+      @removeFromBatch="removeSuggestionFromBatch"
     />
     <div v-else class="note-text md" v-html="note.note_html"></div>
     <note-form

app/assets/javascripts/notes/stores/actions.js

@@ -506,6 +506,42 @@ export const submitSuggestion = (
     });
 };
 
+export const submitSuggestionBatch = ({ commit, dispatch, state }, flashContainer) => {
+  const dispatchResolveDiscussion = discussionId =>
+    dispatch('resolveDiscussion', { discussionId }).catch(() => {});
+
+  commit(types.SET_APPLYING_BATCH_STATE);
+
+  const suggestionIds = state.batchSuggestionIds.map(suggestion => suggestion.id);
+
+  return Api.applySuggestionBatch(suggestionIds)
+    .then(() => commit(types.APPLY_SUGGESTION_BATCH))
+    .then(() =>
+      Promise.all(
+        state.batchSuggestionIds.map(suggestion =>
+          dispatchResolveDiscussion(suggestion.discussionId),
+        ),
+      ),
+    )
+    .then(() => commit(types.CLEAR_SUGGESTION_BATCH))
+    .catch(err => {
+      const defaultMessage = __(
+        'Something went wrong while applying the batch of suggestions. Please try again.',
+      );
+      const flashMessage = err.response.data ? `${err.response.data.message}.` : defaultMessage;
+
+      Flash(__(flashMessage), 'alert', flashContainer);
+    });
+};
+
+export const clearSuggestionBatch = ({ commit }) => commit(types.CLEAR_SUGGESTION_BATCH);
+
+export const pushSuggestionToBatch = ({ commit }, { suggestionId, noteId, discussionId }) =>
+  commit(types.ADD_SUGESTION_TO_BATCH, { suggestionId, noteId, discussionId });
+
+export const popSuggestionFromBatch = ({ commit }, suggestionId) =>
+  commit(types.REMOVE_SUGESTION_FROM_BATCH, suggestionId);
+
 export const convertToDiscussion = ({ commit }, noteId) =>
   commit(types.CONVERT_TO_DISCUSSION, noteId);
 

app/assets/javascripts/notes/stores/getters.js

@@ -211,5 +211,7 @@ export const getDiscussion = state => discussionId =>
 
 export const commentsDisabled = state => state.commentsDisabled;
 
+export const batchSuggestionIds = state => state.batchSuggestionIds;
+
 // prevent babel-plugin-rewire from generating an invalid default during karma tests
 export default () => {};

app/assets/javascripts/notes/stores/modules/index.js

@@ -9,6 +9,7 @@ export default () => ({
     targetNoteHash: null,
     lastFetchedAt: null,
     currentDiscussionId: null,
+    batchSuggestionIds: [],
 
     // View layer
     isToggleStateButtonLoading: false,

app/assets/javascripts/notes/stores/mutation_types.js

@@ -17,6 +17,11 @@ export const SET_NOTES_FETCHED_STATE = 'SET_NOTES_FETCHED_STATE';
 export const SET_NOTES_LOADING_STATE = 'SET_NOTES_LOADING_STATE';
 export const DISABLE_COMMENTS = 'DISABLE_COMMENTS';
 export const APPLY_SUGGESTION = 'APPLY_SUGGESTION';
+export const APPLY_SUGGESTION_BATCH = 'APPLY_SUGGESTION_BATCH';
+export const ADD_SUGESTION_TO_BATCH = 'ADD_SUGESTION_TO_BATCH';
+export const SET_APPLYING_BATCH_STATE = 'SET_APPLYING_BATCH_STATE';
+export const CLEAR_SUGGESTION_BATCH = 'CLEAR_SUGGESTION_BATCH';
+export const REMOVE_SUGESTION_FROM_BATCH = 'REMOVE_SUGESTION_FROM_BATCH';
 export const CONVERT_TO_DISCUSSION = 'CONVERT_TO_DISCUSSION';
 export const REMOVE_CONVERTED_DISCUSSION = 'REMOVE_CONVERTED_DISCUSSION';
 

app/assets/javascripts/notes/stores/mutations.js

@@ -209,11 +209,72 @@ export default {
     const noteObj = utils.findNoteObjectById(state.discussions, discussionId);
     const comment = utils.findNoteObjectById(noteObj.notes, noteId);
 
-    comment.suggestions = comment.suggestions.map(suggestion => ({
-      ...suggestion,
-      applied: suggestion.applied || suggestion.id === suggestionId,
-      appliable: false,
-    }));
+    comment.suggestions = comment.suggestions.map(s => {
+      let suggestion = s;
+      if (suggestion.id === suggestionId) {
+        suggestion = {
+          ...suggestion,
+          applied: true,
+          appliable: false,
+        };
+      }
+      return suggestion;
+    });
+  },
+
+  [types.APPLY_SUGGESTION_BATCH](state) {
+    state.batchSuggestionIds.forEach(batchSuggestion => {
+      const { discussionId, noteId, id: suggestionId } = batchSuggestion;
+
+      const noteObj = utils.findNoteObjectById(state.discussions, discussionId);
+      const comment = utils.findNoteObjectById(noteObj.notes, noteId);
+
+      comment.suggestions = comment.suggestions.map(s => {
+        let suggestion = s;
+        if (suggestion.id === suggestionId) {
+          suggestion = {
+            ...suggestion,
+            applied: true,
+            applying_batch: false,
+            appliable: false,
+          };
+        }
+        return suggestion;
+      });
+    });
+  },
+
+  [types.CLEAR_SUGGESTION_BATCH](state) {
+    state.batchSuggestionIds.splice(0, state.batchSuggestionIds.length);
+  },
+
+  [types.SET_APPLYING_BATCH_STATE](state) {
+    state.batchSuggestionIds.forEach(batchSuggestion => {
+      const { discussionId, noteId, id: suggestionId } = batchSuggestion;
+
+      const noteObj = utils.findNoteObjectById(state.discussions, discussionId);
+      const comment = utils.findNoteObjectById(noteObj.notes, noteId);
+
+      comment.suggestions = comment.suggestions.map(suggestion => ({
+        ...suggestion,
+        applying_batch: suggestion.id === suggestionId,
+      }));
+    });
+  },
+
+  [types.ADD_SUGESTION_TO_BATCH](state, { noteId, discussionId, suggestionId }) {
+    state.batchSuggestionIds.push({
+      id: suggestionId,
+      noteId,
+      discussionId,
+    });
+  },
+
+  [types.REMOVE_SUGESTION_FROM_BATCH](state, suggestionId) {
+    const removableIndex = state.batchSuggestionIds.findIndex(
+      batchSuggestion => batchSuggestion.id === suggestionId,
+    );
+    state.batchSuggestionIds.splice(removableIndex, 1);
   },
 
   [types.UPDATE_DISCUSSION](state, noteData) {

app/assets/javascripts/notes/stores/utils.js

@@ -5,7 +5,7 @@ import { sprintf, __ } from '~/locale';
 // factory function because global flag makes RegExp stateful
 const createQuickActionsRegex = () => /^\/\w+.*$/gm;
 
-export const findNoteObjectById = (notes, id) => notes.filter(n => n.id === id)[0];
+export const findNoteObjectById = (notes, id) => notes.find(n => n.id === id);
 
 export const getQuickActionText = note => {
   let text = __('Applying command');

app/assets/javascripts/vue_shared/components/markdown/suggestion_diff.vue

@@ -13,6 +13,11 @@ export default {
       type: Object,
       required: true,
     },
+    batchSuggestionIds: {
+      type: Array,
+      required: false,
+      default: () => [],
+    },
     disabled: {
       type: Boolean,
       required: false,
@@ -27,11 +32,28 @@ export default {
     lines() {
       return selectDiffLines(this.suggestion.diff_lines);
     },
+    isBatched() {
+      return Boolean(
+        this.batchSuggestionIds.find(batchSuggestion => batchSuggestion.id === this.suggestion.id),
+      );
+    },
+    batchIdsCount() {
+      return this.batchSuggestionIds.length;
+    },
   },
   methods: {
     applySuggestion(callback) {
       this.$emit('apply', { suggestionId: this.suggestion.id, callback });
     },
+    applySuggestionBatch() {
+      this.$emit('applyBatch');
+    },
+    addSuggestionToBatch() {
+      this.$emit('addToBatch', this.suggestion.id);
+    },
+    removeSuggestionFromBatch() {
+      this.$emit('removeFromBatch', this.suggestion.id);
+    },
   },
 };
 </script>
@@ -42,8 +64,15 @@ export default {
       class="qa-suggestion-diff-header js-suggestion-diff-header"
       :can-apply="suggestion.appliable && suggestion.current_user.can_apply && !disabled"
       :is-applied="suggestion.applied"
+      :is-batched="isBatched"
+      :is-applying-batch="suggestion.applying_batch"
+      :batch-ids-count="batchIdsCount"
+      :batch-suggestion-ids="batchSuggestionIds"
       :help-page-path="helpPagePath"
       @apply="applySuggestion"
+      @applyBatch="applySuggestionBatch"
+      @addToBatch="addSuggestionToBatch"
+      @removeFromBatch="removeSuggestionFromBatch"
     />
     <table class="mb-3 md-suggestion-diff js-syntax-highlight code">
       <tbody>

app/assets/javascripts/vue_shared/components/markdown/suggestion_diff_header.vue

@@ -6,6 +6,11 @@ export default {
   components: { Icon, GlButton, GlLoadingIcon },
   directives: { 'gl-tooltip': GlTooltipDirective },
   props: {
+    batchIdsCount: {
+      type: Number,
+      required: false,
+      default: 0,
+    },
     canApply: {
       type: Boolean,
       required: false,
@@ -16,6 +21,16 @@ export default {
       required: true,
       default: false,
     },
+    isBatched: {
+      type: Boolean,
+      required: false,
+      default: false,
+    },
+    isApplyingBatch: {
+      type: Boolean,
+      required: false,
+      default: false,
+    },
     helpPagePath: {
       type: String,
       required: true,
@@ -23,17 +38,32 @@ export default {
   },
   data() {
     return {
-      isApplying: false,
+      isApplyingSingle: false,
     };
   },
+  computed: {
+    isApplying() {
+      return this.isApplyingSingle || this.isApplyingBatch;
+    },
+  },
   methods: {
     applySuggestion() {
       if (!this.canApply) return;
-      this.isApplying = true;
+      this.isApplyingSingle = true;
       this.$emit('apply', this.applySuggestionCallback);
     },
     applySuggestionCallback() {
-      this.isApplying = false;
+      this.isApplyingSingle = false;
+    },
+    applySuggestionBatch() {
+      if (!this.canApply) return;
+      this.$emit('applyBatch');
+    },
+    addSuggestionToBatch() {
+      this.$emit('addToBatch');
+    },
+    removeSuggestionFromBatch() {
+      this.$emit('removeFromBatch');
     },
   },
 };
@@ -52,15 +82,44 @@ export default {
       <gl-loading-icon class="d-flex-center mr-2" />
       <span>{{ __('Applying suggestion') }}</span>
     </div>
-    <gl-button
-      v-else-if="canApply"
-      v-gl-tooltip.viewport="__('This also resolves the discussion')"
-      class="btn-inverted js-apply-btn"
-      :disabled="isApplying"
-      variant="success"
-      @click="applySuggestion"
-    >
-      {{ __('Apply suggestion') }}
-    </gl-button>
+    <div v-else-if="canApply" class="d-flex align-items-center">
+      <span v-if="isBatched">
+        <gl-button
+          class="btn-inverted js-remove-from-batch-btn btn-grouped"
+          :disabled="isApplying"
+          variant="danger"
+          @click="removeSuggestionFromBatch"
+        >
+          {{ __('Remove from batch') }}
+        </gl-button>
+        <gl-button
+          v-gl-tooltip.viewport="__('This also resolves all related discussions')"
+          class="btn-inverted js-apply-batch-btn btn-grouped"
+          :disabled="isApplying"
+          variant="success"
+          @click="applySuggestionBatch"
+        >
+          {{ __('Apply suggestions') }} <span class="badge badge-pill">{{ batchIdsCount }}</span>
+        </gl-button>
+      </span>
+      <span v-else>
+        <gl-button
+          v-gl-tooltip.viewport="__('This also resolves the discussion')"
+          class="btn-inverted js-apply-btn btn-grouped"
+          :disabled="isApplying"
+          variant="success"
+          @click="applySuggestion"
+        >
+          {{ __('Apply suggestion') }}
+        </gl-button>
+        <gl-button
+          class="btn-inverted js-add-to-batch-btn btn-grouped"
+          :disabled="isApplying"
+          @click="addSuggestionToBatch"
+        >
+          {{ __('Add suggestion to batch') }}
+        </gl-button>
+      </span>
+    </div>
   </div>
 </template>

app/assets/javascripts/vue_shared/components/markdown/suggestions.vue

@@ -16,6 +16,11 @@ export default {
       required: false,
       default: () => [],
     },
+    batchSuggestionIds: {
+      type: Array,
+      required: false,
+      default: () => [],
+    },
     noteHtml: {
       type: String,
       required: true,
@@ -68,18 +73,30 @@ export default {
       this.isRendered = true;
     },
     generateDiff(suggestionIndex) {
-      const { suggestions, disabled, helpPagePath } = this;
+      const { suggestions, disabled, batchSuggestionIds, helpPagePath } = this;
       const suggestion =
         suggestions && suggestions[suggestionIndex] ? suggestions[suggestionIndex] : {};
       const SuggestionDiffComponent = Vue.extend(SuggestionDiff);
       const suggestionDiff = new SuggestionDiffComponent({
-        propsData: { disabled, suggestion, helpPagePath },
+        propsData: { disabled, suggestion, batchSuggestionIds, helpPagePath },
       });
 
       suggestionDiff.$on('apply', ({ suggestionId, callback }) => {
         this.$emit('apply', { suggestionId, callback, flashContainer: this.$el });
       });
 
+      suggestionDiff.$on('applyBatch', () => {
+        this.$emit('applyBatch', { flashContainer: this.$el });
+      });
+
+      suggestionDiff.$on('addToBatch', suggestionId => {
+        this.$emit('addToBatch', suggestionId);
+      });
+
+      suggestionDiff.$on('removeFromBatch', suggestionId => {
+        this.$emit('removeFromBatch', suggestionId);
+      });
+
       return suggestionDiff;
     },
     reset() {

app/services/files/multi_service.rb

@@ -66,7 +66,7 @@ def validate_file_status!(action)
       file_path = action[:previous_path] || action[:file_path]
 
       if file_has_changed?(file_path, action[:last_commit_id])
-        raise_error("The file has changed since you started editing it: #{file_path}")
+        raise FileChangedError, _('The file has changed since you started editing it: %{file_path}') % { file_path: file_path }
       end
     end
   end

app/services/suggestions/apply_service.rb

@@ -2,15 +2,17 @@
 
 module Suggestions
   class ApplyService < ::BaseService
-    DEFAULT_SUGGESTION_COMMIT_MESSAGE = 'Apply suggestion to %{file_path}'
+    DEFAULT_SUGGESTION_COMMIT_MESSAGE = 'Apply %{suggestions_count} suggestion(s) to file(s): %{file_path}'
 
+    ## TODO: change 'file_path' to 'file_paths' disucss how to display in commit message, lots of adjusts needed elsewhere, including docs
     PLACEHOLDERS = {
-      'project_path' => ->(suggestion, user) { suggestion.project.path },
-      'project_name' => ->(suggestion, user) { suggestion.project.name },
-      'file_path' => ->(suggestion, user) { suggestion.file_path },
-      'branch_name' => ->(suggestion, user) { suggestion.branch },
-      'username' => ->(suggestion, user) { user.username },
-      'user_full_name' => ->(suggestion, user) { user.name }
+      'project_path' => ->(suggestions, user) { suggestions.first.project.path },
+      'project_name' => ->(suggestions, user) { suggestions.first.project.name },
+      'file_path' => ->(suggestions, user) { suggestions.map(&:file_path).join(" ") },
+      'branch_name' => ->(suggestions, user) { suggestions.first.branch },
+      'suggestions_count' => ->(suggestions, user) { suggestions.count },
+      'username' => ->(suggestions, user) { user.username },
+      'user_full_name' => ->(suggestions, user) { user.name }
     }.freeze
 
     # This regex is built dynamically using the keys from the PLACEHOLDER struct.
@@ -24,31 +26,67 @@ def initialize(current_user)
       @current_user = current_user
     end
 
-    def execute(suggestion)
-      unless suggestion.appliable?(cached: false)
-        return error('Suggestion is not appliable')
-      end
+    def execute(suggestions)
+      blobs_data = {}
 
-      unless latest_source_head?(suggestion)
-        return error('The file has been changed')
-      end
+      error_message = nil
+
+      suggestions.each do |suggestion|
+        unless suggestion.appliable?(cached: false)
+          error_message = 'Suggestion is not appliable'
+          break
+        end
+
+        unless latest_source_head?(suggestion)
+          error_message = 'The file has been changed'
+          break
+        end
+
+        diff_file = suggestion.diff_file
+
+        unless diff_file
+          error_message = 'A file was not found' ## TODO: Adjust language to make sense with more than one file. This needs to happen in a bunch of places.
+          break
+        end
+
+        file_path_sym = diff_file.file_path.to_sym
 
-      diff_file = suggestion.diff_file
+        blob_data = blobs_data[file_path_sym] ||= {
+          blob: diff_file.new_blob,
+          range_and_content_groups: []
+        }
 
-      unless diff_file
-        return error('The file was not found')
+        current_ranges = get_current_line_ranges(blob_data)
+
+        new_range = [suggestion.from_line_index, suggestion.to_line_index]
+
+        suggestion_content = suggestion.to_content
+
+        if has_range_conflict?(current_ranges, new_range)
+          error_message = 'Suggestions are not appliable. Some suggestions overlap.' ## TODO: make test for this
+          break
+        else
+          blobs_data[file_path_sym][:range_and_content_groups] << {
+             range: new_range,
+             content: suggestion_content
+          }
+        end
       end
 
-      params = file_update_params(suggestion, diff_file)
-      result = ::Files::UpdateService.new(suggestion.project, current_user, params).execute
+      return error(error_message) unless error_message.nil?
+
+      multi_update_params = multi_update_params(suggestions, blobs_data)
+      result = ::Files::MultiService.new(suggestions.first.project, current_user, multi_update_params).execute
 
       if result[:status] == :success
-        suggestion.update(commit_id: result[:result], applied: true)
+        suggestions.each do |suggestion|
+          suggestion.update(commit_id: result[:result], applied: true)
+        end
       end
 
       result
-    rescue Files::UpdateService::FileChangedError
-      error('The file has been changed')
+    rescue Files::MultiService::FileChangedError ## update for MultiService
+      error(_('A file has changed'))
     end
 
     private
@@ -63,7 +101,7 @@ def latest_source_head?(suggestion)
     end
 
     def file_update_params(suggestion, diff_file)
-      blob = diff_file.new_blob
+      blob = blobs[diff_file.file_path.to_sym]
       project = suggestion.project
       file_path = suggestion.file_path
       branch_name = suggestion.branch
@@ -85,26 +123,69 @@ def file_update_params(suggestion, diff_file)
       }
     end
 
-    def new_file_content(suggestion, blob)
-      range = suggestion.from_line_index..suggestion.to_line_index
+    def multi_update_params(suggestions, blobs_data)
+      actions = blobs_data.values.map do |blob_data|
+        blob = blob_data[:blob]
+        file_content = new_file_content(blob_data)
+        file_last_commit = Gitlab::Git::Commit.last_for_path(suggestions.first.project.repository,
+          blob.commit_id,
+          blob.path)
+
+        {
+          action: 'update', ## TODO: replace with constant
+          file_path: blob.path,
+          content: file_content,
+          last_commit_id: file_last_commit&.id
+        }
+      end
 
-      blob.load_all_data!
-      content = blob.data.lines
-      content[range] = suggestion.to_content
+      branch_name = suggestions.first.branch
+      # commit_message = "Apply #{suggestions.count} #{'suggestion'.pluralize(suggestions.count)} on #{branch_name}"
+      commit_message = processed_suggestions_commit_message(suggestions)
+
+      {
+        commit_message: commit_message,
+        branch_name: branch_name,
+        start_branch: branch_name,
+        actions: actions
+      }
+    end
+
+    def new_file_content(blob_data)
+      blob_data[:blob].load_all_data!
+      content = blob_data[:blob].data.lines
+
+      blob_data[:range_and_content_groups].each do |r_and_c|
+        range = r_and_c[:range][0]..r_and_c[:range][1]
+        content[range] = r_and_c[:content]
+      end
 
       content.join
     end
 
-    def suggestion_commit_message(project)
-      project.suggestion_commit_message.presence || DEFAULT_SUGGESTION_COMMIT_MESSAGE
+    def suggestions_commit_message(suggestions)
+      suggestions.first.project.suggestion_commit_message.presence || DEFAULT_SUGGESTION_COMMIT_MESSAGE
+      ## TODO: update this "suggestion_commit_message" in the model
     end
 
-    def processed_suggestion_commit_message(suggestion)
-      message = suggestion_commit_message(suggestion.project)
+    def processed_suggestions_commit_message(suggestions)
+      message = suggestions_commit_message(suggestions)
 
       Gitlab::StringPlaceholderReplacer.replace_string_placeholders(message, PLACEHOLDERS_REGEX) do |key|
-        PLACEHOLDERS[key].call(suggestion, current_user)
+        PLACEHOLDERS[key].call(suggestions, current_user)
+      end
+    end
+
+    def has_range_conflict?(previous_ranges, current_range)
+      previous_ranges.any? do |previous_range|
+        current_range.any? do |index|
+          index.between?(previous_range[0], previous_range[1])
+        end
       end
     end
+
+    def get_current_line_ranges(blob_data)
+      blob_data[:range_and_content_groups].map { |r_and_c| r_and_c[:range] }
+    end
   end
 end

lib/api/helpers.rb

@@ -229,6 +229,15 @@ def authorize!(action, subject = :global, reason = nil)
       forbidden!(reason) unless can?(current_user, action, subject)
     end
 
+    def authorize_all_subjects!(action, subjects = [:global], reason = nil)
+      subjects.each do |subject|
+        unless can?(current_user, action, subject)
+          forbidden!(reason)
+          break
+        end
+      end
+    end
+
     def authorize_push_project
       authorize! :push_code, user_project
     end

lib/api/suggestions.rb

@@ -12,15 +12,23 @@ class Suggestions < Grape::API
         requires :id, type: String, desc: 'The suggestion ID'
       end
       put ':id/apply' do
-        suggestion = Suggestion.find_by_id(params[:id])
+        batch_ids = params[:batch_ids].present? ? params[:batch_ids].split(',') : []
 
-        not_found! unless suggestion
-        authorize! :apply_suggestion, suggestion
+        suggestions =
+          if batch_ids.any?
+            Suggestion.find(batch_ids)
+          else
+            [Suggestion.find_by_id(params[:id])]
+          end
 
-        result = ::Suggestions::ApplyService.new(current_user).execute(suggestion)
+        not_found! unless suggestions.any?
+        authorize_all_subjects! :apply_suggestion, suggestions
+
+        result = ::Suggestions::ApplyService.new(current_user).execute(suggestions)
 
         if result[:status] == :success
-          present suggestion, with: Entities::Suggestion, current_user: current_user
+          presentable_suggestions = suggestions.one? ? suggestions.first : suggestions
+          present presentable_suggestions, with: Entities::Suggestion, current_user: current_user ## What does this do and how to update for multiple suggestions?
         else
           http_status = result[:http_status] || 400
           render_api_error!(result[:message], http_status)

locale/gitlab.pot

@@ -799,6 +799,9 @@ msgstr ""
 msgid "A deleted user"
 msgstr ""
 
+msgid "A file has changed"
+msgstr ""
+
 msgid "A file with '%{file_name}' already exists in %{branch} branch"
 msgstr ""
 
@@ -1170,6 +1173,9 @@ msgstr ""
 msgid "Add request manually"
 msgstr ""
 
+msgid "Add suggestion to batch"
+msgstr ""
+
 msgid "Add system hook"
 msgstr ""
 
@@ -2096,6 +2102,9 @@ msgstr ""
 msgid "Apply suggestion"
 msgstr ""
 
+msgid "Apply suggestions"
+msgstr ""
+
 msgid "Apply template"
 msgstr ""
 
@@ -16081,6 +16090,9 @@ msgstr ""
 msgid "Remove fork relationship"
 msgstr ""
 
+msgid "Remove from batch"
+msgstr ""
+
 msgid "Remove from board"
 msgstr ""
 
@@ -18028,6 +18040,9 @@ msgstr ""
 msgid "Something went wrong while adding your award. Please try again."
 msgstr ""
 
+msgid "Something went wrong while applying the batch of suggestions. Please try again."
+msgstr ""
+
 msgid "Something went wrong while applying the suggestion. Please try again."
 msgstr ""
 
@@ -19275,6 +19290,9 @@ msgstr ""
 msgid "The file has been successfully deleted."
 msgstr ""
 
+msgid "The file has changed since you started editing it: %{file_path}"
+msgstr ""
+
 msgid "The file name should have a .yml extension"
 msgstr ""
 
@@ -19748,6 +19766,9 @@ msgstr ""
 msgid "This action can lead to data loss. To prevent accidental actions we ask you to confirm your intention."
 msgstr ""
 
+msgid "This also resolves all related discussions"
+msgstr ""
+
 msgid "This also resolves the discussion"
 msgstr ""
 

spec/frontend/vue_shared/components/markdown/__snapshots__/suggestion_diff_spec.js.snap

@@ -5,6 +5,8 @@ exports[`Suggestion Diff component matches snapshot 1`] = `
   class="md-suggestion"
 >
   <suggestion-diff-header-stub
+    batch-suggestion-ids=""
+    batchidscount="0"
     class="qa-suggestion-diff-header js-suggestion-diff-header"
     helppagepath="path_to_docs"
   />

spec/frontend/vue_shared/components/markdown/suggestion_diff_header_spec.js

@@ -6,6 +6,9 @@ const DEFAULT_PROPS = {
   canApply: true,
   isApplied: false,
   helpPagePath: 'path_to_docs',
+  batchIdsCount: 0,
+  isBatched: false,
+  isApplyingBatch: false,
 };
 
 describe('Suggestion Diff component', () => {
@@ -25,6 +28,7 @@ describe('Suggestion Diff component', () => {
   });
 
   const findApplyButton = () => wrapper.find('.js-apply-btn');
+  const findAddToBatchButton = () => wrapper.find('.js-add-to-batch-btn');
   const findHeader = () => wrapper.find('.js-suggestion-diff-header');
   const findHelpButton = () => wrapper.find('.js-help-btn');
   const findLoading = () => wrapper.find(GlLoadingIcon);
@@ -44,19 +48,24 @@ describe('Suggestion Diff component', () => {
     expect(findHelpButton().exists()).toBe(true);
   });
 
-  it('renders an apply button', () => {
+  it('renders apply suggestion and add to batch buttons', () => {
     createComponent();
 
     const applyBtn = findApplyButton();
+    const addToBatchBtn = findAddToBatchButton();
 
     expect(applyBtn.exists()).toBe(true);
     expect(applyBtn.html().includes('Apply suggestion')).toBe(true);
+
+    expect(addToBatchBtn.exists()).toBe(true);
+    expect(addToBatchBtn.html().includes('Add suggestion to batch')).toBe(true);
   });
 
-  it('does not render an apply button if `canApply` is set to false', () => {
+  it('does not render apply suggestion and add to batch buttons if `canApply` is set to false', () => {
     createComponent({ canApply: false });
 
     expect(findApplyButton().exists()).toBe(false);
+    expect(findAddToBatchButton().exists()).toBe(false);
   });
 
   describe('when apply suggestion is clicked', () => {
@@ -73,8 +82,9 @@ describe('Suggestion Diff component', () => {
       });
     });
 
-    it('hides apply button', () => {
+    it('hides  apply suggestion and add to batch buttons', () => {
       expect(findApplyButton().exists()).toBe(false);
+      expect(findAddToBatchButton().exists()).toBe(false);
     });
 
     it('shows loading', () => {

spec/requests/api/suggestions_spec.rb

@@ -19,23 +19,44 @@
                                diff_refs: merge_request.diff_refs)
   end
 
+  let(:position2) do
+    Gitlab::Diff::Position.new(old_path: "files/ruby/popen.rb",
+                               new_path: "files/ruby/popen.rb",
+                               old_line: nil,
+                               new_line: 15,
+                               diff_refs: merge_request.diff_refs)
+  end
+
   let(:diff_note) do
     create(:diff_note_on_merge_request, noteable: merge_request,
                                         position: position,
                                         project: project)
   end
 
+  let(:diff_note2) do
+    create(:diff_note_on_merge_request, noteable: merge_request,
+                                        position: position2,
+                                        project: project)
+  end
+
   describe "PUT /suggestions/:id/apply" do
     let(:url) { "/suggestions/#{suggestion.id}/apply" }
+    let(:batch_url) { "/suggestions/batch/apply?batch_ids=#{suggestion.id},#{suggestion2.id}" }
 
-    context 'when successfully applies patch' do
+    context 'when successfully applies suggestions' do
       let(:suggestion) do
         create(:suggestion, note: diff_note,
                             from_content: "      raise RuntimeError, \"System commands must be given as an array of strings\"\n",
                             to_content: "      raise RuntimeError, 'Explosion'\n      # explosion?")
       end
 
-      it 'returns 200 with json content' do
+      let(:suggestion2) do
+        create(:suggestion, note: diff_note2,
+                            from_content: "      \"PWD\" => path\n",
+                            to_content: "      *** FOO ***\n")
+      end
+
+      it 'returns 200 with json content for single suggestion' do
         project.add_maintainer(user)
 
         put api(url, user), params: { id: suggestion.id }
@@ -45,14 +66,28 @@
           .to include('id', 'from_line', 'to_line', 'appliable', 'applied',
                       'from_content', 'to_content')
       end
+
+      it 'returns 200 with json content for a batch of suggestions' do
+        project.add_maintainer(user)
+
+        put api(batch_url, user), params: { id: "batch" }
+
+        expect(response).to have_gitlab_http_status(200)
+        expect(json_response).to all(include('id', 'from_line', 'to_line',
+          'appliable', 'applied', 'from_content', 'to_content'))
+      end
     end
 
-    context 'when not able to apply patch' do
+    context 'when not able to apply suggestions' do
       let(:suggestion) do
         create(:suggestion, :unappliable, note: diff_note)
       end
 
-      it 'returns 400 with json content' do
+      let(:suggestion2) do
+        create(:suggestion, :unappliable, note: diff_note2)
+      end
+
+      it 'returns 400 with json content for a single suggestion' do
         project.add_maintainer(user)
 
         put api(url, user), params: { id: suggestion.id }
@@ -60,6 +95,15 @@
         expect(response).to have_gitlab_http_status(400)
         expect(json_response).to eq({ 'message' => 'Suggestion is not appliable' })
       end
+
+      it 'returns 400 with json content for a batch of suggestions' do
+        project.add_maintainer(user)
+
+        put api(batch_url, user), params: { id: "batch" }
+
+        expect(response).to have_gitlab_http_status(400)
+        expect(json_response).to eq({ 'message' => 'Suggestion is not appliable' })
+      end
     end
 
     context 'when unauthorized' do
@@ -69,7 +113,13 @@
                             to_content: "      raise RuntimeError, 'Explosion'\n      # explosion?")
       end
 
-      it 'returns 403 with json content' do
+      let(:suggestion2) do
+        create(:suggestion, note: diff_note2,
+               from_content: "      \"PWD\" => path\n",
+               to_content: "      *** FOO ***\n")
+      end
+
+      it 'returns 403 with json content for a single suggestion' do
         project.add_reporter(user)
 
         put api(url, user), params: { id: suggestion.id }
@@ -77,6 +127,15 @@
         expect(response).to have_gitlab_http_status(403)
         expect(json_response).to eq({ 'message' => '403 Forbidden' })
       end
+
+      it 'returns 403 with json content for a batch of suggestions' do
+        project.add_reporter(user)
+
+        put api(batch_url, user), params: { id: "batch" }
+
+        expect(response).to have_gitlab_http_status(403)
+        expect(json_response).to eq({ 'message' => '403 Forbidden' })
+      end
     end
   end
 end

spec/services/files/multi_service_spec.rb

@@ -71,10 +71,9 @@
         end
 
         it 'rejects the commit' do
-          results = subject.execute
-
-          expect(results[:status]).to eq(:error)
-          expect(results[:message]).to match(new_file_path)
+          expect { subject.execute }
+            .to raise_error(Files::MultiService::FileChangedError,
+              "The file has changed since you started editing it: #{original_file_path}")
         end
       end
 
@@ -133,8 +132,9 @@
         end
 
         it 'rejects the commit' do
-          expect(result[:status]).to eq(:error)
-          expect(result[:message]).to match(original_file_path)
+          expect { subject.execute }
+            .to raise_error(Files::MultiService::FileChangedError,
+              "The file has changed since you started editing it: #{original_file_path}")
         end
       end