From 3ff4ab6ec18eca9e25c47a90d9ee1f68148913c0 Mon Sep 17 00:00:00 2001
From: Thomas Randolph <trandolph@gitlab.com>
Date: Tue, 9 Jul 2024 23:00:53 -0600
Subject: [PATCH 1/3] Don't send commit messages from the FE if they're
 untouched

This can cause "race conditions" where - for example - an approval
AFTER clicking "automerge" will not include the approver's name
because the message was locked in by the front end before the
approval happened.

Changelog: changed
---
 .../components/states/ready_to_merge.vue                   | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/app/assets/javascripts/vue_merge_request_widget/components/states/ready_to_merge.vue b/app/assets/javascripts/vue_merge_request_widget/components/states/ready_to_merge.vue
index 4238198f0fddb3..87e350bcbccb6c 100644
--- a/app/assets/javascripts/vue_merge_request_widget/components/states/ready_to_merge.vue
+++ b/app/assets/javascripts/vue_merge_request_widget/components/states/ready_to_merge.vue
@@ -415,7 +415,6 @@ export default {
 
       const options = {
         sha: latestSha || this.mr.sha,
-        commit_message: this.commitMessage,
         auto_merge_strategy: useAutoMerge ? this.preferredAutoMergeStrategy : undefined,
         should_remove_source_branch: this.removeSourceBranch === true,
         squash: this.squashBeforeMerge,
@@ -425,10 +424,14 @@ export default {
       // If users can't alter the squash message (e.g. for 1-commit merge requests),
       // we shouldn't send the commit message because that would make the backend
       // do unnecessary work.
-      if (this.shouldShowSquashBeforeMerge) {
+      if (this.shouldShowSquashBeforeMerge && this.squashCommitMessageIsTouched) {
         options.squash_commit_message = this.squashCommitMessage;
       }
 
+      if (this.commitMessageIsTouched) {
+        options.commit_message = this.commitMessage;
+      }
+
       this.isMakingRequest = true;
       this.editCommitMessage = false;
 
-- 
GitLab


From 914f4801515cd4de22cd6148c899180d58c6cbce Mon Sep 17 00:00:00 2001
From: Thomas Randolph <trandolph@gitlab.com>
Date: Thu, 11 Jul 2024 19:07:25 -0600
Subject: [PATCH 2/3] Remove the test check for the unmodified commit message

---
 .../components/states/mr_widget_ready_to_merge_spec.js           | 1 -
 1 file changed, 1 deletion(-)

diff --git a/spec/frontend/vue_merge_request_widget/components/states/mr_widget_ready_to_merge_spec.js b/spec/frontend/vue_merge_request_widget/components/states/mr_widget_ready_to_merge_spec.js
index 20c9b49b2a6fa6..5e4fe90ca7a17c 100644
--- a/spec/frontend/vue_merge_request_widget/components/states/mr_widget_ready_to_merge_spec.js
+++ b/spec/frontend/vue_merge_request_widget/components/states/mr_widget_ready_to_merge_spec.js
@@ -403,7 +403,6 @@ describe('ReadyToMerge', () => {
       expect(params).toEqual(
         expect.objectContaining({
           sha: '12345678',
-          commit_message: commitMessage,
           should_remove_source_branch: false,
           auto_merge_strategy: 'merge_when_pipeline_succeeds',
         }),
-- 
GitLab


From 9e6d01703da8515775adcce69186b58efebb511a Mon Sep 17 00:00:00 2001
From: Thomas Randolph <trandolph@gitlab.com>
Date: Wed, 17 Jul 2024 14:31:19 -0600
Subject: [PATCH 3/3] Test our four "new" scenarios

- Squash with edit
- Squash with no edit
- Merge with edit
- Merge with no edit
---
 .../components/states/ready_to_merge.vue      |   1 +
 .../states/mr_widget_ready_to_merge_spec.js   | 113 ++++++++++++++++++
 2 files changed, 114 insertions(+)

diff --git a/app/assets/javascripts/vue_merge_request_widget/components/states/ready_to_merge.vue b/app/assets/javascripts/vue_merge_request_widget/components/states/ready_to_merge.vue
index 87e350bcbccb6c..59ff7a3cede45d 100644
--- a/app/assets/javascripts/vue_merge_request_widget/components/states/ready_to_merge.vue
+++ b/app/assets/javascripts/vue_merge_request_widget/components/states/ready_to_merge.vue
@@ -617,6 +617,7 @@ export default {
                     :label="__('Squash commit message')"
                     input-id="squash-message-edit"
                     class="gl-m-0! gl-p-0!"
+                    data-testid="squash-commit-message"
                     @input="setSquashCommitMessage"
                   >
                     <template #header>
diff --git a/spec/frontend/vue_merge_request_widget/components/states/mr_widget_ready_to_merge_spec.js b/spec/frontend/vue_merge_request_widget/components/states/mr_widget_ready_to_merge_spec.js
index 5e4fe90ca7a17c..31907639f1c762 100644
--- a/spec/frontend/vue_merge_request_widget/components/states/mr_widget_ready_to_merge_spec.js
+++ b/spec/frontend/vue_merge_request_widget/components/states/mr_widget_ready_to_merge_spec.js
@@ -156,6 +156,10 @@ const findDeleteSourceBranchCheckbox = () =>
 const triggerApprovalUpdated = () => eventHub.$emit('ApprovalUpdated');
 const triggerEditCommitInput = () =>
   wrapper.find('[data-testid="widget_edit_commit_message"]').vm.$emit('input', true);
+const triggerEditSquashInput = (text) =>
+  wrapper.find('[data-testid="squash-commit-message"]').vm.$emit('input', text);
+const triggerEditMergeInput = (text) =>
+  wrapper.find('[data-testid="merge-commit-message"]').vm.$emit('input', text);
 const findMergeHelperText = () => wrapper.find('[data-testid="auto-merge-helper-text"]');
 const findTextareas = () => wrapper.findAllComponents(GlFormTextarea);
 
@@ -409,6 +413,115 @@ describe('ReadyToMerge', () => {
       );
     });
 
+    describe('commit message content', () => {
+      describe('with squashing', () => {
+        const NEW_SQUASH_MESSAGE = 'updated squash message';
+
+        it('sends the user-updated squash message', async () => {
+          createComponent({
+            mr: { shouldRemoveSourceBranch: false, enableSquashBeforeMerge: true },
+          });
+
+          jest.spyOn(eventHub, '$emit').mockImplementation(() => {});
+          jest.spyOn(service, 'merge').mockResolvedValue(response('merge_when_pipeline_succeeds'));
+
+          await triggerEditCommitInput();
+          await findCheckboxElement().vm.$emit('input', true);
+          await triggerEditSquashInput(NEW_SQUASH_MESSAGE);
+
+          findMergeButton().vm.$emit('click');
+
+          await waitForPromises();
+
+          const params = service.merge.mock.calls[0][0];
+
+          expect(params).toEqual(
+            expect.objectContaining({
+              sha: '12345678',
+              should_remove_source_branch: false,
+              auto_merge_strategy: 'merge_when_pipeline_succeeds',
+              squash_commit_message: NEW_SQUASH_MESSAGE,
+            }),
+          );
+        });
+
+        it('does not send the squash message if the user has not updated it', async () => {
+          createComponent({
+            mr: { shouldRemoveSourceBranch: false, enableSquashBeforeMerge: true },
+          });
+
+          jest.spyOn(eventHub, '$emit').mockImplementation(() => {});
+          jest.spyOn(service, 'merge').mockResolvedValue(response('merge_when_pipeline_succeeds'));
+
+          await triggerEditCommitInput();
+          await findCheckboxElement().vm.$emit('input', true);
+
+          findMergeButton().vm.$emit('click');
+
+          await waitForPromises();
+
+          const params = service.merge.mock.calls[0][0];
+
+          expect(params).toEqual(
+            expect.not.objectContaining({
+              squash_commit_message: expect.any(String),
+            }),
+          );
+        });
+      });
+
+      describe('without squashing', () => {
+        const NEW_COMMIT_MESSAGE = 'updated commit message';
+
+        it('sends the user-updated commit message', async () => {
+          createComponent({ mr: { shouldRemoveSourceBranch: false } });
+
+          jest.spyOn(eventHub, '$emit').mockImplementation(() => {});
+          jest.spyOn(service, 'merge').mockResolvedValue(response('merge_when_pipeline_succeeds'));
+
+          await triggerEditCommitInput();
+          await triggerEditMergeInput(NEW_COMMIT_MESSAGE);
+
+          findMergeButton().vm.$emit('click');
+
+          await waitForPromises();
+
+          const params = service.merge.mock.calls[0][0];
+
+          expect(params).toEqual(
+            expect.objectContaining({
+              sha: '12345678',
+              should_remove_source_branch: false,
+              auto_merge_strategy: 'merge_when_pipeline_succeeds',
+              commit_message: NEW_COMMIT_MESSAGE,
+              squash: false,
+              skip_merge_train: false,
+            }),
+          );
+        });
+
+        it('does not send the commit message if the user has not updated it', async () => {
+          createComponent({ mr: { shouldRemoveSourceBranch: false } });
+
+          jest.spyOn(eventHub, '$emit').mockImplementation(() => {});
+          jest.spyOn(service, 'merge').mockResolvedValue(response('merge_when_pipeline_succeeds'));
+
+          await triggerEditCommitInput(); // Note this is intentional: `commit_message` shouldn't send until they actually edit it, even if they check the box
+          findMergeButton().vm.$emit('click');
+
+          await waitForPromises();
+
+          const params = service.merge.mock.calls[0][0];
+
+          expect(params).toEqual(
+            expect.not.objectContaining({
+              commit_message: expect.any(String),
+            }),
+          );
+        });
+      });
+    });
+
     it('should handle merge failed', async () => {
       createComponent({ mr: { availableAutoMergeStrategies: [] } });
 
-- 
GitLab