Merge branch '30299-fix-reply-polling-and-resolvable-status' into 'master'

Fix resolvable status for replies to comments

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

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

@@ -93,6 +93,7 @@ export default {
   },
   computed: {
     ...mapGetters([
+      'convertedDisscussionIds',
       'getNoteableData',
       'nextUnresolvedDiscussionId',
       'unresolvedDiscussionsCount',
@@ -301,6 +302,10 @@ export default {
         note: { note: noteText },
       };
 
+      if (this.convertedDisscussionIds.includes(this.discussion.id)) {
+        postData.return_discussion = true;
+      }
+
       if (this.discussion.for_commit) {
         postData.note_project_id = this.discussion.project_id;
       }

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

@@ -60,6 +60,7 @@ export default {
     ...mapGetters([
       'isNotesFetched',
       'discussions',
+      'convertedDisscussionIds',
       'getNotesDataByProp',
       'isLoading',
       'commentsDisabled',
@@ -193,7 +194,9 @@ export default {
           />
           <placeholder-note v-else :key="discussion.id" :note="discussion.notes[0]" />
         </template>
-        <template v-else-if="discussion.individual_note">
+        <template
+          v-else-if="discussion.individual_note && !convertedDisscussionIds.includes(discussion.id)"
+        >
           <system-note
             v-if="discussion.notes[0].system"
             :key="discussion.id"

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

@@ -83,12 +83,44 @@ export const updateNote = ({ commit, dispatch }, { endpoint, note }) =>
       dispatch('startTaskList');
     });
 
-export const replyToDiscussion = ({ commit }, { endpoint, data }) =>
+export const updateOrCreateNotes = ({ commit, state, getters, dispatch }, notes) => {
+  const { notesById } = getters;
+
+  notes.forEach(note => {
+    if (notesById[note.id]) {
+      commit(types.UPDATE_NOTE, note);
+    } else if (note.type === constants.DISCUSSION_NOTE || note.type === constants.DIFF_NOTE) {
+      const discussion = utils.findNoteObjectById(state.discussions, note.discussion_id);
+
+      if (discussion) {
+        commit(types.ADD_NEW_REPLY_TO_DISCUSSION, note);
+      } else if (note.type === constants.DIFF_NOTE) {
+        dispatch('fetchDiscussions', { path: state.notesData.discussionsPath });
+      } else {
+        commit(types.ADD_NEW_NOTE, note);
+      }
+    } else {
+      commit(types.ADD_NEW_NOTE, note);
+    }
+  });
+};
+
+export const replyToDiscussion = ({ commit, state, getters, dispatch }, { endpoint, data }) =>
   service
     .replyToDiscussion(endpoint, data)
     .then(res => res.json())
     .then(res => {
-      commit(types.ADD_NEW_REPLY_TO_DISCUSSION, res);
+      if (res.discussion) {
+        commit(types.UPDATE_DISCUSSION, res.discussion);
+
+        updateOrCreateNotes({ commit, state, getters, dispatch }, res.discussion.notes);
+
+        dispatch('updateMergeRequestWidget');
+        dispatch('startTaskList');
+        dispatch('updateResolvableDiscussonsCounts');
+      } else {
+        commit(types.ADD_NEW_REPLY_TO_DISCUSSION, res);
+      }
 
       return res;
     });
@@ -262,25 +294,7 @@ export const saveNote = ({ commit, dispatch }, noteData) => {
 
 const pollSuccessCallBack = (resp, commit, state, getters, dispatch) => {
   if (resp.notes && resp.notes.length) {
-    const { notesById } = getters;
-
-    resp.notes.forEach(note => {
-      if (notesById[note.id]) {
-        commit(types.UPDATE_NOTE, note);
-      } else if (note.type === constants.DISCUSSION_NOTE || note.type === constants.DIFF_NOTE) {
-        const discussion = utils.findNoteObjectById(state.discussions, note.discussion_id);
-
-        if (discussion) {
-          commit(types.ADD_NEW_REPLY_TO_DISCUSSION, note);
-        } else if (note.type === constants.DIFF_NOTE) {
-          dispatch('fetchDiscussions', { path: state.notesData.discussionsPath });
-        } else {
-          commit(types.ADD_NEW_NOTE, note);
-        }
-      } else {
-        commit(types.ADD_NEW_NOTE, note);
-      }
-    });
+    updateOrCreateNotes({ commit, state, getters, dispatch }, resp.notes);
 
     dispatch('startTaskList');
   }

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

@@ -4,6 +4,8 @@ import { collapseSystemNotes } from './collapse_utils';
 
 export const discussions = state => collapseSystemNotes(state.discussions);
 
+export const convertedDisscussionIds = state => state.convertedDisscussionIds;
+
 export const targetNoteHash = state => state.targetNoteHash;
 
 export const getNotesData = state => state.notesData;

app/assets/javascripts/notes/stores/modules/index.js

@@ -5,6 +5,7 @@ import mutations from '../mutations';
 export default () => ({
   state: {
     discussions: [],
+    convertedDisscussionIds: [],
     targetNoteHash: null,
     lastFetchedAt: null,
 

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

@@ -266,7 +266,7 @@ export default {
   },
 
   [types.CONVERT_TO_DISCUSSION](state, discussionId) {
-    const discussion = utils.findNoteObjectById(state.discussions, discussionId);
-    Object.assign(discussion, { individual_note: false });
+    const convertedDisscussionIds = [...state.convertedDisscussionIds, discussionId];
+    Object.assign(state, { convertedDisscussionIds });
   },
 };

spec/javascripts/helpers/vuex_action_helper.js

@@ -84,7 +84,10 @@ export default (
     done();
   };
 
-  const result = action({ commit, state, dispatch, rootState: state, rootGetters: state }, payload);
+  const result = action(
+    { commit, state, dispatch, rootState: state, rootGetters: state, getters: state },
+    payload,
+  );
 
   return new Promise(resolve => {
     setImmediate(resolve);

spec/javascripts/notes/stores/actions_spec.js

 import Vue from 'vue';
 import $ from 'jquery';
 import _ from 'underscore';
+import { TEST_HOST } from 'spec/test_constants';
 import { headersInterceptor } from 'spec/helpers/vue_resource_helper';
 import * as actions from '~/notes/stores/actions';
+import * as mutationTypes from '~/notes/stores/mutation_types';
+import * as notesConstants from '~/notes/constants';
 import createStore from '~/notes/stores';
 import mrWidgetEventHub from '~/vue_merge_request_widget/event_hub';
 import testAction from '../../helpers/vuex_action_helper';
@@ -599,4 +602,139 @@ describe('Actions Notes Store', () => {
       );
     });
   });
+
+  describe('updateOrCreateNotes', () => {
+    let commit;
+    let dispatch;
+    let state;
+
+    beforeEach(() => {
+      commit = jasmine.createSpy('commit');
+      dispatch = jasmine.createSpy('dispatch');
+      state = {};
+    });
+
+    afterEach(() => {
+      commit.calls.reset();
+      dispatch.calls.reset();
+    });
+
+    it('Updates existing note', () => {
+      const note = { id: 1234 };
+      const getters = { notesById: { 1234: note } };
+
+      actions.updateOrCreateNotes({ commit, state, getters, dispatch }, [note]);
+
+      expect(commit.calls.allArgs()).toEqual([[mutationTypes.UPDATE_NOTE, note]]);
+    });
+
+    it('Creates a new note if none exisits', () => {
+      const note = { id: 1234 };
+      const getters = { notesById: {} };
+      actions.updateOrCreateNotes({ commit, state, getters, dispatch }, [note]);
+
+      expect(commit.calls.allArgs()).toEqual([[mutationTypes.ADD_NEW_NOTE, note]]);
+    });
+
+    describe('Discussion notes', () => {
+      let note;
+      let getters;
+
+      beforeEach(() => {
+        note = { id: 1234 };
+        getters = { notesById: {} };
+      });
+
+      it('Adds a reply to an existing discussion', () => {
+        state = { discussions: [note] };
+        const discussionNote = {
+          ...note,
+          type: notesConstants.DISCUSSION_NOTE,
+          discussion_id: 1234,
+        };
+
+        actions.updateOrCreateNotes({ commit, state, getters, dispatch }, [discussionNote]);
+
+        expect(commit.calls.allArgs()).toEqual([
+          [mutationTypes.ADD_NEW_REPLY_TO_DISCUSSION, discussionNote],
+        ]);
+      });
+
+      it('fetches discussions for diff notes', () => {
+        state = { discussions: [], notesData: { discussionsPath: 'Hello world' } };
+        const diffNote = { ...note, type: notesConstants.DIFF_NOTE, discussion_id: 1234 };
+
+        actions.updateOrCreateNotes({ commit, state, getters, dispatch }, [diffNote]);
+
+        expect(dispatch.calls.allArgs()).toEqual([
+          ['fetchDiscussions', { path: state.notesData.discussionsPath }],
+        ]);
+      });
+
+      it('Adds a new note', () => {
+        state = { discussions: [] };
+        const discussionNote = {
+          ...note,
+          type: notesConstants.DISCUSSION_NOTE,
+          discussion_id: 1234,
+        };
+
+        actions.updateOrCreateNotes({ commit, state, getters, dispatch }, [discussionNote]);
+
+        expect(commit.calls.allArgs()).toEqual([[mutationTypes.ADD_NEW_NOTE, discussionNote]]);
+      });
+    });
+  });
+
+  describe('replyToDiscussion', () => {
+    let res = { discussion: { notes: [] } };
+    const payload = { endpoint: TEST_HOST, data: {} };
+    const interceptor = (request, next) => {
+      next(
+        request.respondWith(JSON.stringify(res), {
+          status: 200,
+        }),
+      );
+    };
+
+    beforeEach(() => {
+      Vue.http.interceptors.push(interceptor);
+    });
+
+    afterEach(() => {
+      Vue.http.interceptors = _.without(Vue.http.interceptors, interceptor);
+    });
+
+    it('updates discussion if response contains disussion', done => {
+      testAction(
+        actions.replyToDiscussion,
+        payload,
+        {
+          notesById: {},
+        },
+        [{ type: mutationTypes.UPDATE_DISCUSSION, payload: res.discussion }],
+        [
+          { type: 'updateMergeRequestWidget' },
+          { type: 'startTaskList' },
+          { type: 'updateResolvableDiscussonsCounts' },
+        ],
+        done,
+      );
+    });
+
+    it('adds a reply to a discussion', done => {
+      res = {};
+
+      testAction(
+        actions.replyToDiscussion,
+        payload,
+        {
+          notesById: {},
+        },
+        [{ type: mutationTypes.ADD_NEW_REPLY_TO_DISCUSSION, payload: res }],
+        [],
+        done,
+      );
+    });
+  });
 });

spec/javascripts/notes/stores/mutation_spec.js

@@ -527,17 +527,13 @@ describe('Notes Store mutations', () => {
         id: 42,
         individual_note: true,
       };
-      state = { discussions: [discussion] };
+      state = { convertedDisscussionIds: [] };
     });
 
-    it('toggles individual_note', () => {
+    it('adds a disucssion to convertedDisscussionIds', () => {
       mutations.CONVERT_TO_DISCUSSION(state, discussion.id);
 
-      expect(discussion.individual_note).toBe(false);
-    });
-
-    it('throws if discussion was not found', () => {
-      expect(() => mutations.CONVERT_TO_DISCUSSION(state, 99)).toThrow();
+      expect(state.convertedDisscussionIds).toContain(discussion.id);
     });
   });
 });