Merge branch 'ph/326168/batchSuggestionsCommitMessage' into 'master'

Allow users to add a commit message with batch suggestions

See merge request gitlab-org/gitlab!66247

app/assets/javascripts/api.js

@@ -499,10 +499,10 @@ const Api = {
     return axios.put(url, params);
   },
 
-  applySuggestionBatch(ids) {
+  applySuggestionBatch(ids, message) {
     const url = Api.buildUrl(Api.applySuggestionBatchPath);
 
-    return axios.put(url, { ids });
+    return axios.put(url, { ids, commit_message: message });
   },
 
   commitPipelines(projectId, sha) {

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

@@ -51,7 +51,7 @@ export default {
     },
   },
   computed: {
-    ...mapGetters(['getDiscussion', 'suggestionsCount']),
+    ...mapGetters(['getDiscussion', 'suggestionsCount', 'getSuggestionsFilePaths']),
     ...mapGetters('diffs', ['suggestionCommitMessage']),
     discussion() {
       if (!this.note.isDraft) return {};
@@ -74,9 +74,10 @@ export default {
       // Please see this issue comment for why these
       //  are hard-coded to 1:
       //  https://gitlab.com/gitlab-org/gitlab/-/issues/291027#note_468308022
-      const suggestionsCount = 1;
-      const filesCount = 1;
-      const filePaths = this.file ? [this.file.file_path] : [];
+      const suggestionsCount = this.batchSuggestionsInfo.length || 1;
+      const batchFilePaths = this.getSuggestionsFilePaths();
+      const filePaths = batchFilePaths.length ? batchFilePaths : [this.file.file_path];
+      const filesCount = filePaths.length;
       const suggestion = this.suggestionCommitMessage({
         file_paths: filePaths.join(', '),
         suggestions_count: suggestionsCount,
@@ -131,8 +132,8 @@ export default {
         message,
       }).then(callback);
     },
-    applySuggestionBatch({ flashContainer }) {
-      return this.submitSuggestionBatch({ flashContainer });
+    applySuggestionBatch({ message, flashContainer }) {
+      return this.submitSuggestionBatch({ message, flashContainer });
     },
     addSuggestionToBatch(suggestionId) {
       const { discussion_id: discussionId, id: noteId } = this.note;

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

@@ -631,7 +631,7 @@ export const submitSuggestion = (
     });
 };
 
-export const submitSuggestionBatch = ({ commit, dispatch, state }, { flashContainer }) => {
+export const submitSuggestionBatch = ({ commit, dispatch, state }, { message, flashContainer }) => {
   const suggestionIds = state.batchSuggestionsInfo.map(({ suggestionId }) => suggestionId);
 
   const resolveAllDiscussions = () =>
@@ -644,7 +644,7 @@ export const submitSuggestionBatch = ({ commit, dispatch, state }, { flashContai
   commit(types.SET_RESOLVING_DISCUSSION, true);
   dispatch('stopPolling');
 
-  return Api.applySuggestionBatch(suggestionIds)
+  return Api.applySuggestionBatch(suggestionIds, message)
     .then(() => Promise.all(resolveAllDiscussions()))
     .then(() => commit(types.CLEAR_SUGGESTION_BATCH))
     .catch((err) => {

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

@@ -283,3 +283,14 @@ export const suggestionsCount = (state, getters) =>
 
 export const hasDrafts = (state, getters, rootState, rootGetters) =>
   Boolean(rootGetters['batchComments/hasDrafts']);
+
+export const getSuggestionsFilePaths = (state) => () =>
+  state.batchSuggestionsInfo.reduce((acc, suggestion) => {
+    const discussion = state.discussions.find((d) => d.id === suggestion.discussionId);
+
+    if (acc.indexOf(discussion?.diff_file?.file_path) === -1) {
+      acc.push(discussion.diff_file.file_path);
+    }
+
+    return acc;
+  }, []);

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

 <script>
 import { GlDropdown, GlDropdownForm, GlFormTextarea, GlButton } from '@gitlab/ui';
+import { __, n__ } from '~/locale';
 
 export default {
   components: { GlDropdown, GlDropdownForm, GlFormTextarea, GlButton },
@@ -13,12 +14,26 @@ export default {
       type: String,
       required: true,
     },
+    batchSuggestionsCount: {
+      type: Number,
+      required: false,
+      default: 0,
+    },
   },
   data() {
     return {
       message: null,
     };
   },
+  computed: {
+    dropdownText() {
+      if (this.batchSuggestionsCount <= 1) {
+        return __('Apply suggestion');
+      }
+
+      return n__('Apply %d suggestion', 'Apply %d suggestions', this.batchSuggestionsCount);
+    },
+  },
   methods: {
     onApply() {
       this.$emit('apply', this.message);
@@ -29,10 +44,11 @@ export default {
 
 <template>
   <gl-dropdown
-    :text="__('Apply suggestion')"
+    :text="dropdownText"
     :disabled="disabled"
     boundary="window"
     right
+    lazy
     menu-class="gl-w-full!"
     data-qa-selector="apply_suggestion_dropdown"
     @shown="$refs.commitMessage.$el.focus()"

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

@@ -54,8 +54,8 @@ export default {
     applySuggestion(callback, message) {
       this.$emit('apply', { suggestionId: this.suggestion.id, callback, message });
     },
-    applySuggestionBatch() {
-      this.$emit('applyBatch');
+    applySuggestionBatch(message) {
+      this.$emit('applyBatch', message);
     },
     addSuggestionToBatch() {
       this.$emit('addToBatch', this.suggestion.id);

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

@@ -58,12 +58,19 @@ export default {
       isApplyingSingle: false,
     };
   },
+
   computed: {
     isApplying() {
       return this.isApplyingSingle || this.isApplyingBatch;
     },
     tooltipMessage() {
-      return this.canApply ? __('This also resolves this thread') : this.inapplicableReason;
+      if (!this.canApply) {
+        return this.inapplicableReason;
+      }
+
+      return this.batchSuggestionsCount > 1
+        ? __('This also resolves all related threads')
+        : __('This also resolves this thread');
     },
     isDisableButton() {
       return this.isApplying || !this.canApply;
@@ -72,13 +79,30 @@ export default {
       if (this.isApplyingSingle || this.batchSuggestionsCount < 2) {
         return __('Applying suggestion...');
       }
+
       return __('Applying suggestions...');
     },
     isLoggedIn() {
       return isLoggedIn();
     },
+    showApplySuggestion() {
+      if (!this.isLoggedIn) return false;
+
+      if (this.batchSuggestionsCount >= 1 && !this.isBatched) {
+        return false;
+      }
+
+      return true;
+    },
   },
   methods: {
+    apply(message) {
+      if (this.batchSuggestionsCount > 1) {
+        this.applySuggestionBatch(message);
+      } else {
+        this.applySuggestion(message);
+      }
+    },
     applySuggestion(message) {
       if (!this.canApply) return;
       this.isApplyingSingle = true;
@@ -88,9 +112,9 @@ export default {
     applySuggestionCallback() {
       this.isApplyingSingle = false;
     },
-    applySuggestionBatch() {
+    applySuggestionBatch(message) {
       if (!this.canApply) return;
-      this.$emit('applyBatch');
+      this.$emit('applyBatch', message);
     },
     addSuggestionToBatch() {
       this.$emit('addToBatch');
@@ -115,7 +139,8 @@ export default {
       <gl-loading-icon size="sm" class="d-flex-center mr-2" />
       <span>{{ applyingSuggestionsMessage }}</span>
     </div>
-    <div v-else-if="canApply && isBatched" class="d-flex align-items-center">
+    <div v-else-if="canApply" class="d-flex align-items-center">
+      <div v-if="isBatched">
         <gl-button
           class="btn-inverted js-remove-from-batch-btn btn-grouped"
           :disabled="isApplying"
@@ -123,23 +148,10 @@ export default {
         >
           {{ __('Remove from batch') }}
         </gl-button>
-      <gl-button
-        v-gl-tooltip.viewport="__('This also resolves all related threads')"
-        class="btn-inverted js-apply-batch-btn btn-grouped"
-        data-qa-selector="apply_suggestions_batch_button"
-        :disabled="isApplying"
-        variant="success"
-        @click="applySuggestionBatch"
-      >
-        {{ __('Apply suggestions') }}
-        <span class="badge badge-pill badge-pill-success">
-          {{ batchSuggestionsCount }}
-        </span>
-      </gl-button>
       </div>
-    <div v-else class="d-flex align-items-center">
+      <div v-else>
         <gl-button
-        v-if="suggestionsCount > 1 && !isDisableButton"
+          v-if="!isDisableButton && suggestionsCount > 1"
           class="btn-inverted js-add-to-batch-btn btn-grouped"
           data-qa-selector="add_suggestion_batch_button"
           :disabled="isDisableButton"
@@ -147,13 +159,15 @@ export default {
         >
           {{ __('Add suggestion to batch') }}
         </gl-button>
+      </div>
       <apply-suggestion
-        v-if="isLoggedIn"
+        v-if="showApplySuggestion"
         v-gl-tooltip.viewport="tooltipMessage"
         :disabled="isDisableButton"
         :default-commit-message="defaultCommitMessage"
+        :batch-suggestions-count="batchSuggestionsCount"
         class="gl-ml-3"
-        @apply="applySuggestion"
+        @apply="apply"
       />
     </div>
   </div>

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

@@ -68,6 +68,10 @@ export default {
     if (this.suggestionsWatch) {
       this.suggestionsWatch();
     }
+
+    if (this.defaultCommitMessageWatch) {
+      this.defaultCommitMessageWatch();
+    }
   },
   methods: {
     renderSuggestions() {
@@ -123,12 +127,16 @@ export default {
         suggestionDiff.suggestionsCount = this.suggestionsCount;
       });
 
+      this.defaultCommitMessageWatch = this.$watch('defaultCommitMessage', () => {
+        suggestionDiff.defaultCommitMessage = this.defaultCommitMessage;
+      });
+
       suggestionDiff.$on('apply', ({ suggestionId, callback, message }) => {
         this.$emit('apply', { suggestionId, callback, flashContainer: this.$el, message });
       });
 
-      suggestionDiff.$on('applyBatch', () => {
-        this.$emit('applyBatch', { flashContainer: this.$el });
+      suggestionDiff.$on('applyBatch', (message) => {
+        this.$emit('applyBatch', { message, flashContainer: this.$el });
       });
 
       suggestionDiff.$on('addToBatch', (suggestionId) => {

app/assets/stylesheets/framework/markdown_area.scss

@@ -153,6 +153,10 @@
     vertical-align: middle;
     margin-bottom: 3px;
   }
+
+  .dropdown-chevron {
+    margin-bottom: 0;
+  }
 }
 
 @include media-breakpoint-down(xs) {

locale/gitlab.pot

@@ -4110,6 +4110,11 @@ msgstr ""
 msgid "Apply"
 msgstr ""
 
+msgid "Apply %d suggestion"
+msgid_plural "Apply %d suggestions"
+msgstr[0] ""
+msgstr[1] ""
+
 msgid "Apply a label"
 msgstr ""
 
@@ -4119,9 +4124,6 @@ msgstr ""
 msgid "Apply suggestion"
 msgstr ""
 
-msgid "Apply suggestions"
-msgstr ""
-
 msgid "Apply template"
 msgstr ""
 

qa/qa/page/merge_request/show.rb

@@ -93,7 +93,6 @@ module QA
         end
 
         view 'app/assets/javascripts/vue_shared/components/markdown/suggestion_diff_header.vue' do
-          element :apply_suggestions_batch_button
           element :add_suggestion_batch_button
         end
 
@@ -353,10 +352,6 @@ module QA
           all_elements(:add_suggestion_batch_button, minimum: 1).first.click
         end
 
-        def apply_suggestions_batch
-          all_elements(:apply_suggestions_batch_button, minimum: 1).first.click
-        end
-
         def cherry_pick!
           click_element(:cherry_pick_button, Page::Component::CommitModal)
           click_element(:submit_commit_button)

qa/qa/specs/features/browser_ui/3_create/merge_request/suggestions/batch_suggestion_spec.rb

@@ -50,7 +50,7 @@ module QA
         Page::MergeRequest::Show.perform do |merge_request|
           merge_request.click_diffs_tab
           4.times { merge_request.add_suggestion_to_batch }
-          merge_request.apply_suggestions_batch
+          merge_request.apply_suggestion_with_message("Custom commit message")
 
           expect(merge_request).to have_css('.badge-success', text: "Applied", count: 4)
         end

spec/features/merge_request/user_suggests_changes_on_diff_spec.rb

@@ -159,7 +159,12 @@ RSpec.describe 'User comments on a diff', :js do
           wait_for_requests
 
           expect(page).to have_content('Remove from batch')
-          expect(page).to have_content("Apply suggestions #{index + 1}")
+
+          if index < 1
+            expect(page).to have_content("Apply suggestion")
+          else
+            expect(page).to have_content("Apply #{index + 1} suggestions")
+          end
         end
       end
 
@@ -167,13 +172,12 @@ RSpec.describe 'User comments on a diff', :js do
         click_button('Remove from batch')
         wait_for_requests
 
-        expect(page).to have_content('Apply suggestion')
         expect(page).to have_content('Add suggestion to batch')
       end
 
       page.within("[id='#{files[1][:hash]}']") do
         expect(page).to have_content('Remove from batch')
-        expect(page).to have_content('Apply suggestions 1')
+        expect(page).to have_content('Apply suggestion')
       end
     end
 

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

@@ -60,7 +60,7 @@ describe('Suggestion Diff component', () => {
     expect(findHelpButton().exists()).toBe(true);
   });
 
-  it('renders apply suggestion and add to batch buttons', () => {
+  it('renders add to batch button when more than 1 suggestion', () => {
     createComponent({
       suggestionsCount: 2,
     });
@@ -68,8 +68,7 @@ describe('Suggestion Diff component', () => {
     const applyBtn = findApplyButton();
     const addToBatchBtn = findAddToBatchButton();
 
-    expect(applyBtn.exists()).toBe(true);
-    expect(applyBtn.html().includes('Apply suggestion')).toBe(true);
+    expect(applyBtn.exists()).toBe(false);
 
     expect(addToBatchBtn.exists()).toBe(true);
     expect(addToBatchBtn.html().includes('Add suggestion to batch')).toBe(true);
@@ -85,7 +84,7 @@ describe('Suggestion Diff component', () => {
 
   describe('when apply suggestion is clicked', () => {
     beforeEach(() => {
-      createComponent();
+      createComponent({ batchSuggestionsCount: 0 });
 
       findApplyButton().vm.$emit('apply');
     });
@@ -140,11 +139,11 @@ describe('Suggestion Diff component', () => {
 
   describe('apply suggestions is clicked', () => {
     it('emits applyBatch', () => {
-      createComponent({ isBatched: true });
+      createComponent({ isBatched: true, batchSuggestionsCount: 2 });
 
-      findApplyBatchButton().vm.$emit('click');
+      findApplyButton().vm.$emit('apply');
 
-      expect(wrapper.emitted().applyBatch).toEqual([[]]);
+      expect(wrapper.emitted().applyBatch).toEqual([[undefined]]);
     });
   });
 
@@ -155,23 +154,24 @@ describe('Suggestion Diff component', () => {
         isBatched: true,
       });
 
-      const applyBatchBtn = findApplyBatchButton();
+      const applyBatchBtn = findApplyButton();
       const removeFromBatchBtn = findRemoveFromBatchButton();
 
       expect(removeFromBatchBtn.exists()).toBe(true);
       expect(removeFromBatchBtn.html().includes('Remove from batch')).toBe(true);
 
       expect(applyBatchBtn.exists()).toBe(true);
-      expect(applyBatchBtn.html().includes('Apply suggestions')).toBe(true);
+      expect(applyBatchBtn.html().includes('Apply suggestion')).toBe(true);
       expect(applyBatchBtn.html().includes(String('9'))).toBe(true);
     });
 
     it('hides add to batch and apply buttons', () => {
       createComponent({
         isBatched: true,
+        batchSuggestionsCount: 9,
       });
 
-      expect(findApplyButton().exists()).toBe(false);
+      expect(findApplyButton().exists()).toBe(true);
       expect(findAddToBatchButton().exists()).toBe(false);
     });
 
@@ -215,9 +215,8 @@ describe('Suggestion Diff component', () => {
     });
 
     it('disables apply suggestion and hides add to batch button', () => {
-      expect(findApplyButton().exists()).toBe(true);
+      expect(findApplyButton().exists()).toBe(false);
       expect(findAddToBatchButton().exists()).toBe(false);
-      expect(findApplyButton().attributes('disabled')).toBe('true');
     });
   });
 
@@ -225,20 +224,11 @@ describe('Suggestion Diff component', () => {
     const findTooltip = () => getBinding(findApplyButton().element, 'gl-tooltip');
 
     it('renders correct tooltip message when button is applicable', () => {
-      createComponent();
+      createComponent({ batchSuggestionsCount: 0 });
       const tooltip = findTooltip();
 
       expect(tooltip.modifiers.viewport).toBe(true);
       expect(tooltip.value).toBe('This also resolves this thread');
     });
-
-    it('renders the inapplicable reason in the tooltip when button is not applicable', () => {
-      const inapplicableReason = 'lorem';
-      createComponent({ canApply: false, inapplicableReason });
-      const tooltip = findTooltip();
-
-      expect(tooltip.modifiers.viewport).toBe(true);
-      expect(tooltip.value).toBe(inapplicableReason);
-    });
   });
 });

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

@@ -77,7 +77,7 @@ describe('Suggestion Diff component', () => {
   it.each`
     event                | childArgs         | args
     ${'apply'}           | ${['test-event']} | ${[{ callback: 'test-event', suggestionId }]}
-    ${'applyBatch'}      | ${[]}             | ${[]}
+    ${'applyBatch'}      | ${['test-event']} | ${['test-event']}
     ${'addToBatch'}      | ${[]}             | ${[suggestionId]}
     ${'removeFromBatch'} | ${[]}             | ${[suggestionId]}
   `('emits $event event on sugestion diff header $event', ({ event, childArgs, args }) => {