Merge branch '48803-mrs-are-showing-some-discussions-as-unresolved' into 'master'

Resolve "MRs are showing some discussions as unresolved"

Closes #48803

See merge request gitlab-org/gitlab-ce!20351

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

@@ -85,9 +85,9 @@ export const allDiscussions = (state, getters) => {
 export const resolvedDiscussionsById = state => {
   const map = {};
 
-  state.discussions.forEach(n => {
+  state.discussions.filter(d => d.resolvable).forEach(n => {
     if (n.notes) {
-      const resolved = n.notes.every(note => note.resolved && !note.system);
+      const resolved = n.notes.filter(note => note.resolvable).every(note => note.resolved);
 
       if (resolved) {
         map[n.id] = n;

spec/javascripts/fixtures/merge_requests.rb

@@ -80,6 +80,13 @@ describe Projects::MergeRequestsController, '(JavaScript fixtures)', type: :cont
     render_discussions_json(merge_request, example.description)
   end
 
+  it 'merge_requests/resolved_diff_discussion.json' do |example|
+    note = create(:discussion_note_on_merge_request, :resolved, project: project, author: admin, position: position, noteable: merge_request)
+    create(:system_note, project: project, author: admin, noteable: merge_request, discussion_id: note.discussion.id)
+
+    render_discussions_json(merge_request, example.description)
+  end
+
   context 'with image diff' do
     let(:merge_request2) { create(:merge_request_with_diffs, :with_image_diffs, source_project: project, title: "Added images") }
     let(:image_path) { "files/images/ee_repo_logo.png" }

spec/javascripts/notes/components/discussion_counter_spec.js

@@ -32,12 +32,12 @@ describe('DiscussionCounter component', () => {
           {
             ...discussionMock,
             id: discussionMock.id,
-            notes: [{ ...discussionMock.notes[0], resolved: true }],
+            notes: [{ ...discussionMock.notes[0], resolvable: true, resolved: true }],
           },
           {
             ...discussionMock,
             id: discussionMock.id + 1,
-            notes: [{ ...discussionMock.notes[0], resolved: false }],
+            notes: [{ ...discussionMock.notes[0], resolvable: true, resolved: false }],
           },
         ];
         const firstDiscussionId = discussionMock.id + 1;

spec/javascripts/notes/components/noteable_discussion_spec.js

@@ -4,22 +4,23 @@ import noteableDiscussion from '~/notes/components/noteable_discussion.vue';
 import '~/behaviors/markdown/render_gfm';
 import { noteableDataMock, discussionMock, notesDataMock } from '../mock_data';
 
+const discussionWithTwoUnresolvedNotes = 'merge_requests/resolved_diff_discussion.json';
+
 describe('noteable_discussion component', () => {
+  const Component = Vue.extend(noteableDiscussion);
   let store;
   let vm;
 
-  beforeEach(() => {
-    const Component = Vue.extend(noteableDiscussion);
+  preloadFixtures(discussionWithTwoUnresolvedNotes);
 
+  beforeEach(() => {
     store = createStore();
     store.dispatch('setNoteableData', noteableDataMock);
     store.dispatch('setNotesData', notesDataMock);
 
     vm = new Component({
       store,
-      propsData: {
-        discussion: discussionMock,
-      },
+      propsData: { discussion: discussionMock },
     }).$mount();
   });
 
@@ -84,7 +85,9 @@ describe('noteable_discussion component', () => {
       });
 
       it('is true if there are two unresolved discussions', done => {
-        spyOnProperty(vm, 'unresolvedDiscussions').and.returnValue([{}, {}]);
+        const discussion = getJSONFixture(discussionWithTwoUnresolvedNotes)[0];
+        discussion.notes[0].resolved = false;
+        vm.$store.dispatch('setInitialNotes', [discussion, discussion]);
 
         Vue.nextTick()
           .then(() => {
@@ -105,12 +108,12 @@ describe('noteable_discussion component', () => {
           {
             ...discussionMock,
             id: discussionMock.id + 1,
-            notes: [{ ...discussionMock.notes[0], resolved: true }],
+            notes: [{ ...discussionMock.notes[0], resolvable: true, resolved: true }],
           },
           {
             ...discussionMock,
             id: discussionMock.id + 2,
-            notes: [{ ...discussionMock.notes[0], resolved: false }],
+            notes: [{ ...discussionMock.notes[0], resolvable: true, resolved: false }],
           },
         ];
         const nextDiscussionId = discussionMock.id + 2;

spec/javascripts/notes/mock_data.js

@@ -303,6 +303,7 @@ export const discussionMock = {
     },
   ],
   individual_note: false,
+  resolvable: true,
 };
 
 export const loggedOutnoteableData = {

spec/javascripts/notes/stores/getters_spec.js

@@ -7,9 +7,13 @@ import {
   collapseNotesMock,
 } from '../mock_data';
 
+const discussionWithTwoUnresolvedNotes = 'merge_requests/resolved_diff_discussion.json';
+
 describe('Getters Notes Store', () => {
   let state;
 
+  preloadFixtures(discussionWithTwoUnresolvedNotes);
+
   beforeEach(() => {
     state = {
       discussions: [individualNote],
@@ -22,12 +26,26 @@ describe('Getters Notes Store', () => {
       noteableData: noteableDataMock,
     };
   });
+
   describe('discussions', () => {
     it('should return all discussions in the store', () => {
       expect(getters.discussions(state)).toEqual([individualNote]);
     });
   });
 
+  describe('resolvedDiscussionsById', () => {
+    it('ignores unresolved system notes', () => {
+      const [discussion] = getJSONFixture(discussionWithTwoUnresolvedNotes);
+      discussion.notes[0].resolved = true;
+      discussion.notes[1].resolved = false;
+      state.discussions.push(discussion);
+
+      expect(getters.resolvedDiscussionsById(state)).toEqual({
+        [discussion.id]: discussion,
+      });
+    });
+  });
+
   describe('Collapsed notes', () => {
     const stateCollapsedNotes = {
       discussions: collapseNotesMock,