MR commits display commit ID for discussions

Fixes a bug where merge request comments made
in the context of a specific commit no longer specify
which commit they were created on

4 files changed, 93 insertions, 27 deletions

diff --git a/app/assets/javascripts/notes/components/noteable_discussion.vue b/app/assets/javascripts/notes/components/noteable_discussion.vue
index d9dd08a7a6b..7c3f5d00308 100644
--- a/app/assets/javascripts/notes/components/noteable_discussion.vue
+++ b/app/assets/javascripts/notes/components/noteable_discussion.vue
@@ -178,31 +178,32 @@ export default {
         commitId = `<span class="commit-sha">${truncateSha(commitId)}</span>`;
       }
 
-      let text = s__('MergeRequests|started a discussion');
+      const {
+        for_commit: isForCommit,
+        diff_discussion: isDiffDiscussion,
+        active: isActive,
+      } = this.discussion;
 
-      if (this.discussion.for_commit) {
+      let text = s__('MergeRequests|started a discussion');
+      if (isForCommit) {
         text = s__(
           'MergeRequests|started a discussion on commit %{linkStart}%{commitId}%{linkEnd}',
         );
-      } else if (this.discussion.diff_discussion) {
-        if (this.discussion.active) {
-          text = s__('MergeRequests|started a discussion on %{linkStart}the diff%{linkEnd}');
-        } else {
-          text = s__(
-            'MergeRequests|started a discussion on %{linkStart}an old version of the diff%{linkEnd}',
-          );
-        }
+      } else if (isDiffDiscussion && commitId) {
+        text = isActive
+          ? s__('MergeRequests|started a discussion on commit %{linkStart}%{commitId}%{linkEnd}')
+          : s__(
+              'MergeRequests|started a discussion on an outdated change in commit %{linkStart}%{commitId}%{linkEnd}',
+            );
+      } else if (isDiffDiscussion) {
+        text = isActive
+          ? s__('MergeRequests|started a discussion on %{linkStart}the diff%{linkEnd}')
+          : s__(
+              'MergeRequests|started a discussion on %{linkStart}an old version of the diff%{linkEnd}',
+            );
       }
 
-      return sprintf(
-        text,
-        {
-          commitId,
-          linkStart,
-          linkEnd,
-        },
-        false,
-      );
+      return sprintf(text, { commitId, linkStart, linkEnd }, false);
     },
     diffLine() {
       if (this.discussion.diff_discussion && this.discussion.truncated_diff_lines) {
diff --git a/changelogs/unreleased/winh-merge-request-commit-context.yml b/changelogs/unreleased/winh-merge-request-commit-context.yml
new file mode 100644
index 00000000000..9e12a926af4
--- /dev/null
+++ b/changelogs/unreleased/winh-merge-request-commit-context.yml
@@ -0,0 +1,5 @@
+---
+title: Display commit ID for discussions made on merge request commits
+merge_request: 23837
+author:
+type: fixed
diff --git a/locale/gitlab.pot b/locale/gitlab.pot
index fed490309cc..260f581446b 100644
--- a/locale/gitlab.pot
+++ b/locale/gitlab.pot
@@ -4135,6 +4135,9 @@ msgstr ""
 msgid "MergeRequests|started a discussion on %{linkStart}the diff%{linkEnd}"
 msgstr ""
 
+msgid "MergeRequests|started a discussion on an outdated change in commit %{linkStart}%{commitId}%{linkEnd}"
+msgstr ""
+
 msgid "MergeRequests|started a discussion on commit %{linkStart}%{commitId}%{linkEnd}"
 msgstr ""
 
diff --git a/spec/javascripts/notes/components/noteable_discussion_spec.js b/spec/javascripts/notes/components/noteable_discussion_spec.js
index 106a4ac2546..3aff2dd0641 100644
--- a/spec/javascripts/notes/components/noteable_discussion_spec.js
+++ b/spec/javascripts/notes/components/noteable_discussion_spec.js
@@ -133,8 +133,10 @@ describe('noteable_discussion component', () => {
     });
   });
 
-  describe('commit discussion', () => {
+  describe('action text', () => {
     const commitId = 'razupaltuff';
+    const truncatedCommitId = commitId.substr(0, 8);
+    let commitElement;
 
     beforeEach(() => {
       vm.$destroy();
@@ -143,7 +145,6 @@ describe('noteable_discussion component', () => {
         projectPath: 'something',
       };
 
-      vm.$destroy();
       vm = new Component({
         propsData: {
           discussion: {
@@ -159,17 +160,73 @@ describe('noteable_discussion component', () => {
         },
         store,
       }).$mount();
+
+      commitElement = vm.$el.querySelector('.commit-sha');
+    });
+
+    describe('for commit discussions', () => {
+      it('should display a monospace started a discussion on commit', () => {
+        expect(vm.$el).toContainText(`started a discussion on commit ${truncatedCommitId}`);
+        expect(commitElement).not.toBe(null);
+        expect(commitElement).toHaveText(truncatedCommitId);
+      });
     });
 
-    it('displays a monospace started a discussion on commit', () => {
-      const truncatedCommitId = commitId.substr(0, 8);
+    describe('for diff discussion with a commit id', () => {
+      it('should display started discussion on commit header', done => {
+        vm.discussion.for_commit = false;
 
-      expect(vm.$el).toContainText(`started a discussion on commit ${truncatedCommitId}`);
+        vm.$nextTick(() => {
+          expect(vm.$el).toContainText(`started a discussion on commit ${truncatedCommitId}`);
+          expect(commitElement).not.toBe(null);
 
-      const commitElement = vm.$el.querySelector('.commit-sha');
+          done();
+        });
+      });
 
-      expect(commitElement).not.toBe(null);
-      expect(commitElement).toHaveText(truncatedCommitId);
+      it('should display outdated change on commit header', done => {
+        vm.discussion.for_commit = false;
+        vm.discussion.active = false;
+
+        vm.$nextTick(() => {
+          expect(vm.$el).toContainText(
+            `started a discussion on an outdated change in commit ${truncatedCommitId}`,
+          );
+
+          expect(commitElement).not.toBe(null);
+
+          done();
+        });
+      });
+    });
+
+    describe('for diff discussions without a commit id', () => {
+      it('should show started a discussion on the diff text', done => {
+        Object.assign(vm.discussion, {
+          for_commit: false,
+          commit_id: null,
+        });
+
+        vm.$nextTick(() => {
+          expect(vm.$el).toContainText('started a discussion on the diff');
+
+          done();
+        });
+      });
+
+      it('should show discussion on older version text', done => {
+        Object.assign(vm.discussion, {
+          for_commit: false,
+          commit_id: null,
+          active: false,
+        });
+
+        vm.$nextTick(() => {
+          expect(vm.$el).toContainText('started a discussion on an old version of the diff');
+
+          done();
+        });
+      });
     });
   });
 });