Merge branch 'winh-merge-request-related-issues' into 'master'

Adjust position and wording for related issues in merge requests

Closes #33652

See merge request !11759

app/assets/javascripts/vue_merge_request_widget/components/mr_widget_related_links.js

 export default {
   name: 'MRWidgetRelatedLinks',
   props: {
+    isMerged: { type: Boolean, required: true },
     relatedLinks: { type: Object, required: true },
   },
   computed: {
+    // TODO: the following should be handled by i18n
+    closingText() {
+      if (this.isMerged) {
+        return `Closed ${this.issueLabel('closing')}`;
+      }
+
+      return `Closes ${this.issueLabel('closing')}`;
+    },
     hasLinks() {
       const { closing, mentioned, assignToMe } = this.relatedLinks;
       return closing || mentioned || assignToMe;
     },
+    // TODO: the following should be handled by i18n
+    mentionedText() {
+      if (this.isMerged) {
+        if (this.hasMultipleIssues(this.relatedLinks.mentioned)) {
+          return 'are mentioned but were not closed';
+        }
+
+        return 'is mentioned but was not closed';
+      }
+
+      if (this.hasMultipleIssues(this.relatedLinks.mentioned)) {
+        return 'are mentioned but will not be closed';
+      }
+
+      return 'is mentioned but will not be closed';
+    },
   },
   methods: {
     hasMultipleIssues(text) {
-      return !text ? false : text.match(/<\/a> and <a/);
+      return /<\/a>,? and <a/.test(text);
     },
+    // TODO: the following should be handled by i18n
     issueLabel(field) {
       return this.hasMultipleIssues(this.relatedLinks[field]) ? 'issues' : 'issue';
     },
-    verbLabel(field) {
-      return this.hasMultipleIssues(this.relatedLinks[field]) ? 'are' : 'is';
-    },
   },
   template: `
-    <section
-      v-if="hasLinks"
-      class="mr-info-list mr-links">
+    <div v-if="hasLinks">
       <div class="legend"></div>
       <p v-if="relatedLinks.closing">
-        Closes {{issueLabel('closing')}}
+        {{closingText}}
         <span v-html="relatedLinks.closing"></span>.
       </p>
       <p v-if="relatedLinks.mentioned">
         <span class="capitalize">{{issueLabel('mentioned')}}</span>
         <span v-html="relatedLinks.mentioned"></span>
-        {{verbLabel('mentioned')}} mentioned but will not be closed.
+        {{mentionedText}}
       </p>
       <p v-if="relatedLinks.assignToMe">
         <span v-html="relatedLinks.assignToMe"></span>
       </p>
-    </section>
+    </div>
   `,
 };

app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_merged.js

 /* global Flash */
 
 import mrWidgetAuthorTime from '../../components/mr_widget_author_time';
+import mrWidgetRelatedLinks from '../../components/mr_widget_related_links';
 import eventHub from '../../event_hub';
+import '../../../flash';
 
 export default {
   name: 'MRWidgetMerged',
@@ -11,6 +13,7 @@ export default {
   },
   components: {
     'mr-widget-author-and-time': mrWidgetAuthorTime,
+    'mr-widget-related-links': mrWidgetRelatedLinks,
   },
   data() {
     return {
@@ -18,6 +21,9 @@ export default {
     };
   },
   computed: {
+    shouldRenderRelatedLinks() {
+      return this.mr.relatedLinks && this.mr.isMerged;
+    },
     shouldShowRemoveSourceBranch() {
       const { sourceBranchRemoved, isRemovingSourceBranch, canRemoveSourceBranch } = this.mr;
 
@@ -86,6 +92,10 @@ export default {
             aria-hidden="true" />
           The source branch is being removed.
         </p>
+        <mr-widget-related-links
+          v-if="shouldRenderRelatedLinks"
+          :is-merged="mr.isMerged()"
+          :related-links="mr.relatedLinks" />
       </section>
       <div
         v-if="shouldShowMergedButtons"

app/assets/javascripts/vue_merge_request_widget/mr_widget_options.js

@@ -48,7 +48,7 @@ export default {
       return stateMaps.stateToComponentMap[this.mr.state];
     },
     shouldRenderMergeHelp() {
-      return stateMaps.statesToShowHelpWidget.indexOf(this.mr.state) > -1;
+      return !this.mr.isMerged;
     },
     shouldRenderPipelines() {
       return Object.keys(this.mr.pipeline).length || this.mr.hasCI;
@@ -238,9 +238,14 @@ export default {
         :is="componentName"
         :mr="mr"
         :service="service" />
-      <mr-widget-related-links
+      <section
         v-if="shouldRenderRelatedLinks"
-        :related-links="mr.relatedLinks" />
+        class="mr-info-list mr-links">
+        <div class="legend"></div>
+        <mr-widget-related-links
+          :is-merged="mr.isMerged"
+          :related-links="mr.relatedLinks" />
+      </section>
       <mr-widget-merge-help v-if="shouldRenderMergeHelp" />
     </div>
   `,

app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js

 import Timeago from 'timeago.js';
 import { getStateKey } from '../dependencies';
 
+const unmergedStates = [
+  'locked',
+  'conflicts',
+  'workInProgress',
+  'readyToMerge',
+  'checking',
+  'unresolvedDiscussions',
+  'pipelineFailed',
+  'pipelineBlocked',
+  'autoMergeFailed',
+];
+
 export default class MergeRequestStore {
 
   constructor(data) {
@@ -65,6 +77,7 @@ export default class MergeRequestStore {
     this.mergeActionsContentPath = data.commit_change_content_path;
     this.isRemovingSourceBranch = this.isRemovingSourceBranch || false;
     this.isOpen = data.state === 'opened' || data.state === 'reopened' || false;
+    this.isMerged = unmergedStates.indexOf(data.state) === -1;
     this.hasMergeableDiscussionsState = data.mergeable_discussions_state === false;
     this.canRemoveSourceBranch = currentUser.can_remove_source_branch || false;
     this.canMerge = !!data.merge_path;

app/assets/javascripts/vue_merge_request_widget/stores/state_maps.js

@@ -19,19 +19,6 @@ const stateToComponentMap = {
   shaMismatch: 'mr-widget-sha-mismatch',
 };
 
-const statesToShowHelpWidget = [
-  'locked',
-  'conflicts',
-  'workInProgress',
-  'readyToMerge',
-  'checking',
-  'unresolvedDiscussions',
-  'pipelineFailed',
-  'pipelineBlocked',
-  'autoMergeFailed',
-];
-
 export default {
   stateToComponentMap,
-  statesToShowHelpWidget,
 };

app/assets/stylesheets/pages/merge_requests.scss

@@ -372,6 +372,10 @@
       margin-left: 12px;
     }
 
+    &.mr-state-locked + .mr-info-list.mr-links {
+      margin-top: -16px;
+    }
+
     &.empty-state {
       .artwork {
         margin-bottom: $gl-padding;

spec/features/merge_requests/closes_issues_spec.rb

@@ -36,7 +36,7 @@ feature 'Merge Request closing issues message', feature: true, js: true do
     let(:merge_request_description) { "Description\n\nclosing #{issue_1.to_reference}, #{issue_2.to_reference}" }
 
     it 'does not display closing issue message' do
-      expect(page).to have_content("Closes issues #{issue_1.to_reference} and #{issue_2.to_reference}")
+      expect(page).to have_content("Closed issues #{issue_1.to_reference} and #{issue_2.to_reference}")
     end
   end
 
@@ -44,7 +44,7 @@ feature 'Merge Request closing issues message', feature: true, js: true do
     let(:merge_request_description) { "Description\n\nRefers to #{issue_1.to_reference} and #{issue_2.to_reference}" }
 
     it 'does not display closing issue message' do
-      expect(page).to have_content("Issues #{issue_1.to_reference} and #{issue_2.to_reference} are mentioned but will not be closed.")
+      expect(page).to have_content("Issues #{issue_1.to_reference} and #{issue_2.to_reference} are mentioned but were not closed")
     end
   end
 
@@ -52,8 +52,8 @@ feature 'Merge Request closing issues message', feature: true, js: true do
     let(:merge_request_title) { "closes #{issue_1.to_reference}\n\n refers to #{issue_2.to_reference}" }
 
     it 'does not display closing issue message' do
-      expect(page).to have_content("Closes issue #{issue_1.to_reference}.")
-      expect(page).to have_content("Issue #{issue_2.to_reference} is mentioned but will not be closed.")
+      expect(page).to have_content("Closed issue #{issue_1.to_reference}")
+      expect(page).to have_content("Issue #{issue_2.to_reference} is mentioned but was not closed")
     end
   end
 
@@ -61,7 +61,7 @@ feature 'Merge Request closing issues message', feature: true, js: true do
     let(:merge_request_title) { "closing #{issue_1.to_reference}, #{issue_2.to_reference}" }
 
     it 'does not display closing issue message' do
-      expect(page).to have_content("Closes issues #{issue_1.to_reference} and #{issue_2.to_reference}")
+      expect(page).to have_content("Closed issues #{issue_1.to_reference} and #{issue_2.to_reference}")
     end
   end
 
@@ -69,7 +69,7 @@ feature 'Merge Request closing issues message', feature: true, js: true do
     let(:merge_request_title) { "Refers to #{issue_1.to_reference} and #{issue_2.to_reference}" }
 
     it 'does not display closing issue message' do
-      expect(page).to have_content("Issues #{issue_1.to_reference} and #{issue_2.to_reference} are mentioned but will not be closed.")
+      expect(page).to have_content("Issues #{issue_1.to_reference} and #{issue_2.to_reference} are mentioned but were not closed")
     end
   end
 
@@ -77,8 +77,8 @@ feature 'Merge Request closing issues message', feature: true, js: true do
     let(:merge_request_title) { "closes #{issue_1.to_reference}\n\n refers to #{issue_2.to_reference}" }
 
     it 'does not display closing issue message' do
-      expect(page).to have_content("Closes issue #{issue_1.to_reference}. Issue #{issue_2.to_reference} is mentioned but will not be closed.")
-      expect(page).to have_content("Issue #{issue_2.to_reference} is mentioned but will not be closed.")
+      expect(page).to have_content("Closed issue #{issue_1.to_reference}")
+      expect(page).to have_content("Issue #{issue_2.to_reference} is mentioned but was not closed")
     end
   end
 end

spec/javascripts/vue_mr_widget/components/mr_widget_related_links_spec.js

 import Vue from 'vue';
-import relatedLinksComponent from '~/vue_merge_request_widget/components/mr_widget_related_links';
+import MRWidgetRelatedLinks from '~/vue_merge_request_widget/components/mr_widget_related_links';
 
-const createComponent = (data) => {
-  const Component = Vue.extend(relatedLinksComponent);
+describe('MRWidgetRelatedLinks', () => {
+  let vm;
+
+  beforeEach(() => {
+    const Component = Vue.extend(MRWidgetRelatedLinks);
+    vm = new Component({
+      el: document.createElement('div'),
+      propsData: {
+        isMerged: false,
+        relatedLinks: {},
+      },
+    });
+  });
 
-  return new Component({
-    el: document.createElement('div'),
-    propsData: data,
+  afterEach(() => {
+    vm.$destroy();
   });
-};
 
-describe('MRWidgetRelatedLinks', () => {
   describe('props', () => {
     it('should have props', () => {
-      const { relatedLinks } = relatedLinksComponent.props;
+      const { isMerged, relatedLinks } = MRWidgetRelatedLinks.props;
 
+      expect(isMerged).toBeDefined();
+      expect(isMerged.type).toBe(Boolean);
+      expect(isMerged.required).toBeTruthy();
       expect(relatedLinks).toBeDefined();
       expect(relatedLinks.type instanceof Object).toBeTruthy();
       expect(relatedLinks.required).toBeTruthy();
@@ -22,16 +33,38 @@ describe('MRWidgetRelatedLinks', () => {
   });
 
   describe('computed', () => {
+    describe('closingText', () => {
+      const dummyIssueLabel = 'dummy label';
+
+      beforeEach(() => {
+        spyOn(vm, 'issueLabel').and.returnValue(dummyIssueLabel);
+      });
+
+      it('outputs text for closing issues', () => {
+        vm.isMerged = false;
+
+        const text = vm.closingText;
+
+        expect(text).toBe(`Closes ${dummyIssueLabel}`);
+      });
+
+      it('outputs text for closed issues', () => {
+        vm.isMerged = true;
+
+        const text = vm.closingText;
+
+        expect(text).toBe(`Closed ${dummyIssueLabel}`);
+      });
+    });
+
     describe('hasLinks', () => {
       it('should return correct value when we have links reference', () => {
-        const data = {
-          relatedLinks: {
-            closing: '/foo',
-            mentioned: '/foo',
-            assignToMe: '/foo',
-          },
+        vm.relatedLinks = {
+          closing: '/foo',
+          mentioned: '/foo',
+          assignToMe: '/foo',
         };
-        const vm = createComponent(data);
+
         expect(vm.hasLinks).toBeTruthy();
 
         vm.relatedLinks.closing = null;
@@ -44,95 +77,160 @@ describe('MRWidgetRelatedLinks', () => {
         expect(vm.hasLinks).toBeFalsy();
       });
     });
-  });
 
-  describe('methods', () => {
-    const data = {
-      relatedLinks: {
-        closing: '<a href="#">#23</a> and <a>#42</a>',
-        mentioned: '<a href="#">#7</a>',
-      },
-    };
-    const vm = createComponent(data);
+    describe('mentionedText', () => {
+      it('outputs text for one mentioned issue before merging', () => {
+        vm.isMerged = false;
+        spyOn(vm, 'hasMultipleIssues').and.returnValue(false);
 
-    describe('hasMultipleIssues', () => {
-      it('should return true if the given text has multiple issues', () => {
-        expect(vm.hasMultipleIssues(data.relatedLinks.closing)).toBeTruthy();
+        const text = vm.mentionedText;
+
+        expect(text).toBe('is mentioned but will not be closed');
       });
 
-      it('should return false if the given text has one issue', () => {
-        expect(vm.hasMultipleIssues(data.relatedLinks.mentioned)).toBeFalsy();
+      it('outputs text for one mentioned issue after merging', () => {
+        vm.isMerged = true;
+        spyOn(vm, 'hasMultipleIssues').and.returnValue(false);
+
+        const text = vm.mentionedText;
+
+        expect(text).toBe('is mentioned but was not closed');
+      });
+
+      it('outputs text for multiple mentioned issue before merging', () => {
+        vm.isMerged = false;
+        spyOn(vm, 'hasMultipleIssues').and.returnValue(true);
+
+        const text = vm.mentionedText;
+
+        expect(text).toBe('are mentioned but will not be closed');
+      });
+
+      it('outputs text for multiple mentioned issue after merging', () => {
+        vm.isMerged = true;
+        spyOn(vm, 'hasMultipleIssues').and.returnValue(true);
+
+        const text = vm.mentionedText;
+
+        expect(text).toBe('are mentioned but were not closed');
       });
     });
+  });
 
-    describe('issueLabel', () => {
+  describe('methods', () => {
+    const relatedLinks = {
+      oneIssue: '<a href="#">#7</a>',
+      twoIssues: '<a href="#">#23</a> and <a>#42</a>',
+      threeIssues: '<a href="#">#1</a>, <a>#2</a>, and <a>#3</a>',
+    };
+
+    beforeEach(() => {
+      vm.relatedLinks = relatedLinks;
+    });
+
+    describe('hasMultipleIssues', () => {
       it('should return true if the given text has multiple issues', () => {
-        expect(vm.issueLabel('closing')).toEqual('issues');
+        expect(vm.hasMultipleIssues(relatedLinks.twoIssues)).toBeTruthy();
+        expect(vm.hasMultipleIssues(relatedLinks.threeIssues)).toBeTruthy();
       });
 
       it('should return false if the given text has one issue', () => {
-        expect(vm.issueLabel('mentioned')).toEqual('issue');
+        expect(vm.hasMultipleIssues(relatedLinks.oneIssue)).toBeFalsy();
       });
     });
 
-    describe('verbLabel', () => {
+    describe('issueLabel', () => {
       it('should return true if the given text has multiple issues', () => {
-        expect(vm.verbLabel('closing')).toEqual('are');
+        expect(vm.issueLabel('twoIssues')).toEqual('issues');
+        expect(vm.issueLabel('threeIssues')).toEqual('issues');
       });
 
       it('should return false if the given text has one issue', () => {
-        expect(vm.verbLabel('mentioned')).toEqual('is');
+        expect(vm.issueLabel('oneIssue')).toEqual('issue');
       });
     });
   });
 
   describe('template', () => {
-    it('should have only have closing issues text', () => {
-      const vm = createComponent({
-        relatedLinks: {
-          closing: '<a href="#">#23</a> and <a>#42</a>',
-        },
-      });
-      const content = vm.$el.textContent.replace(/\n(\s)+/g, ' ').trim();
-
-      expect(content).toContain('Closes issues #23 and #42');
-      expect(content).not.toContain('mentioned');
-    });
+    it('should have only have closing issues text', (done) => {
+      vm.relatedLinks = {
+        closing: '<a href="#">#23</a> and <a>#42</a>',
+      };
 
-    it('should have only have mentioned issues text', () => {
-      const vm = createComponent({
-        relatedLinks: {
-          mentioned: '<a href="#">#7</a>',
-        },
-      });
+      Vue.nextTick()
+      .then(() => {
+        const content = vm.$el.textContent.replace(/\n(\s)+/g, ' ').trim();
 
-      expect(vm.$el.innerText).toContain('issue #7');
-      expect(vm.$el.innerText).toContain('is mentioned but will not be closed.');
-      expect(vm.$el.innerText).not.toContain('Closes');
+        expect(content).toContain('Closes issues #23 and #42');
+        expect(content).not.toContain('mentioned');
+      })
+      .then(done)
+      .catch(done.fail);
     });
 
-    it('should have closing and mentioned issues at the same time', () => {
-      const vm = createComponent({
-        relatedLinks: {
-          closing: '<a href="#">#7</a>',
-          mentioned: '<a href="#">#23</a> and <a>#42</a>',
-        },
-      });
-      const content = vm.$el.textContent.replace(/\n(\s)+/g, ' ').trim();
+    it('should have only have mentioned issues text', (done) => {
+      vm.relatedLinks = {
+        mentioned: '<a href="#">#7</a>',
+      };
+
+      Vue.nextTick()
+      .then(() => {
+        expect(vm.$el.innerText).toContain('issue #7');
+        expect(vm.$el.innerText).toContain('is mentioned but will not be closed');
+        expect(vm.$el.innerText).not.toContain('Closes');
+      })
+      .then(done)
+      .catch(done.fail);
+    });
 
-      expect(content).toContain('Closes issue #7.');
-      expect(content).toContain('issues #23 and #42');
-      expect(content).toContain('are mentioned but will not be closed.');
+    it('should have closing and mentioned issues at the same time', (done) => {
+      vm.relatedLinks = {
+        closing: '<a href="#">#7</a>',
+        mentioned: '<a href="#">#23</a> and <a>#42</a>',
+      };
+
+      Vue.nextTick()
+      .then(() => {
+        const content = vm.$el.textContent.replace(/\n(\s)+/g, ' ').trim();
+
+        expect(content).toContain('Closes issue #7.');
+        expect(content).toContain('issues #23 and #42');
+        expect(content).toContain('are mentioned but will not be closed');
+      })
+      .then(done)
+      .catch(done.fail);
     });
 
-    it('should have assing issues link', () => {
-      const vm = createComponent({
-        relatedLinks: {
-          assignToMe: '<a href="#">Assign yourself to these issues</a>',
-        },
-      });
+    it('should have assing issues link', (done) => {
+      vm.relatedLinks = {
+        assignToMe: '<a href="#">Assign yourself to these issues</a>',
+      };
+
+      Vue.nextTick()
+      .then(() => {
+        expect(vm.$el.innerText).toContain('Assign yourself to these issues');
+      })
+      .then(done)
+      .catch(done.fail);
+    });
 
-      expect(vm.$el.innerText).toContain('Assign yourself to these issues');
+    it('should use different wording after merging', (done) => {
+      vm.isMerged = true;
+      vm.relatedLinks = {
+        closing: '<a href="#">#7</a>',
+        mentioned: '<a href="#">#23</a> and <a>#42</a>',
+      };
+
+      Vue.nextTick()
+      .then(() => {
+        const content = vm.$el.textContent.replace(/\n(\s)+/g, ' ').trim();
+
+        expect(content).toContain('Closed issue #7.');
+        expect(content).toContain('issues #23 and #42');
+        expect(content).toContain('are mentioned but were not closed');
+      })
+      .then(done)
+      .catch(done.fail);
     });
   });
 });

spec/javascripts/vue_mr_widget/mr_widget_options_spec.js

@@ -48,12 +48,13 @@ describe('mrWidgetOptions', () => {
     });
 
     describe('shouldRenderMergeHelp', () => {
-      it('should return false for the initial merged state', () => {
+      it('should return false after merging', () => {
+        vm.mr.isMerged = true;
         expect(vm.shouldRenderMergeHelp).toBeFalsy();
       });
 
-      it('should return true for a state which requires help widget', () => {
-        vm.mr.state = 'conflicts';
+      it('should return true before merging', () => {
+        vm.mr.isMerged = false;
         expect(vm.shouldRenderMergeHelp).toBeTruthy();
       });
     });

spec/javascripts/vue_mr_widget/stores/mr_widget_store_spec.js

@@ -18,5 +18,17 @@ describe('MergeRequestStore', () => {
       store.setData({ ...mockData, work_in_progress: !mockData.work_in_progress });
       expect(store.hasSHAChanged).toBe(false);
     });
+
+    it('sets isMerged to true for merged state', () => {
+      store.setData({ ...mockData, state: 'merged' });
+
+      expect(store.isMerged).toBe(true);
+    });
+
+    it('sets isMerged to false for readyToMerge state', () => {
+      store.setData({ ...mockData, state: 'readyToMerge' });
+
+      expect(store.isMerged).toBe(false);
+    });
   });
 });