Merge branch '48237-toggle-file-comments' into 'master'

Resolve "Toggle file comments in merge request does not update toggle buttons"

Closes #48237 and #48537

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

app/assets/javascripts/diffs/components/diff_file.vue

@@ -31,9 +31,6 @@ export default {
     };
   },
   computed: {
-    isDiscussionsExpanded() {
-      return true; // TODO: @fatihacet - Fix this.
-    },
     isCollapsed() {
       return this.file.collapsed || false;
     },
@@ -131,7 +128,6 @@ export default {
       :diff-file="file"
       :collapsible="true"
       :expanded="!isCollapsed"
-      :discussions-expanded="isDiscussionsExpanded"
       :add-merge-request-buttons="true"
       class="js-file-title file-title"
       @toggleFile="handleToggle"

app/assets/javascripts/diffs/components/diff_file_header.vue

 <script>
 import _ from 'underscore';
+import { mapActions, mapGetters } from 'vuex';
 import ClipboardButton from '~/vue_shared/components/clipboard_button.vue';
 import Icon from '~/vue_shared/components/icon.vue';
 import FileIcon from '~/vue_shared/components/file_icon.vue';
@@ -38,11 +39,6 @@ export default {
       required: false,
       default: true,
     },
-    discussionsExpanded: {
-      type: Boolean,
-      required: false,
-      default: true,
-    },
     currentUser: {
       type: Object,
       required: true,
@@ -54,6 +50,10 @@ export default {
     };
   },
   computed: {
+    ...mapGetters('diffs', ['diffHasExpandedDiscussions']),
+    hasExpandedDiscussions() {
+      return this.diffHasExpandedDiscussions(this.diffFile);
+    },
     icon() {
       if (this.diffFile.submodule) {
         return 'archive';
@@ -88,9 +88,6 @@ export default {
     collapseIcon() {
       return this.expanded ? 'chevron-down' : 'chevron-right';
     },
-    isDiscussionsExpanded() {
-      return this.discussionsExpanded && this.expanded;
-    },
     viewFileButtonText() {
       const truncatedContentSha = _.escape(truncateSha(this.diffFile.contentSha));
       return sprintf(
@@ -113,7 +110,8 @@ export default {
     },
   },
   methods: {
-    handleToggle(e, checkTarget) {
+    ...mapActions('diffs', ['toggleFileDiscussions']),
+    handleToggleFile(e, checkTarget) {
       if (
         !checkTarget ||
         e.target === this.$refs.header ||
@@ -125,6 +123,9 @@ export default {
     showForkMessage() {
       this.$emit('showForkMessage');
     },
+    handleToggleDiscussions() {
+      this.toggleFileDiscussions(this.diffFile);
+    },
   },
 };
 </script>
@@ -133,7 +134,7 @@ export default {
   <div
     ref="header"
     class="js-file-title file-title file-title-flex-parent"
-    @click="handleToggle($event, true)"
+    @click="handleToggleFile($event, true)"
   >
     <div class="file-header-content">
       <icon
@@ -216,10 +217,11 @@ export default {
         v-if="diffFile.blob && diffFile.blob.readableText"
       >
         <button
-          :class="{ active: isDiscussionsExpanded }"
+          :class="{ active: hasExpandedDiscussions }"
           :title="s__('MergeRequests|Toggle comments for this file')"
-          class="btn js-toggle-diff-comments"
+          class="js-btn-vue-toggle-comments btn"
           type="button"
+          @click="handleToggleDiscussions"
         >
           <icon name="comment" />
         </button>

app/assets/javascripts/diffs/store/actions.js

@@ -82,5 +82,32 @@ export const expandAllFiles = ({ commit }) => {
   commit(types.EXPAND_ALL_FILES);
 };
 
+/**
+ * Toggles the file discussions after user clicked on the toggle discussions button.
+ *
+ * Gets the discussions for the provided diff.
+ *
+ * If all discussions are expanded, it will collapse all of them
+ * If all discussions are collapsed, it will expand all of them
+ * If some discussions are open and others closed, it will expand the closed ones.
+ *
+ * @param {Object} diff
+ */
+export const toggleFileDiscussions = ({ getters, dispatch }, diff) => {
+  const discussions = getters.getDiffFileDiscussions(diff);
+  const shouldCloseAll = getters.diffHasAllExpandedDiscussions(diff);
+  const shouldExpandAll = getters.diffHasAllCollpasedDiscussions(diff);
+
+  discussions.forEach(discussion => {
+    const data = { discussionId: discussion.id };
+
+    if (shouldCloseAll) {
+      dispatch('collapseDiscussion', data, { root: true });
+    } else if (shouldExpandAll || (!shouldCloseAll && !shouldExpandAll && !discussion.expanded)) {
+      dispatch('expandDiscussion', data, { root: true });
+    }
+  });
+};
+
 // prevent babel-plugin-rewire from generating an invalid default during karma tests
 export default () => {};

app/assets/javascripts/diffs/store/getters.js

+import _ from 'underscore';
 import { PARALLEL_DIFF_VIEW_TYPE, INLINE_DIFF_VIEW_TYPE } from '../constants';
 
 export const isParallelView = state => state.diffViewType === PARALLEL_DIFF_VIEW_TYPE;
@@ -8,5 +9,52 @@ export const areAllFilesCollapsed = state => state.diffFiles.every(file => file.
 
 export const commitId = state => (state.commit && state.commit.id ? state.commit.id : null);
 
-// prevent babel-plugin-rewire from generating an invalid default during karma tests
+/**
+ * Checks if the diff has all discussions expanded
+ * @param {Object} diff
+ * @returns {Boolean}
+ */
+export const diffHasAllExpandedDiscussions = (state, getters) => diff => {
+  const discussions = getters.getDiffFileDiscussions(diff);
+
+  return (discussions.length && discussions.every(discussion => discussion.expanded)) || false;
+};
+
+/**
+ * Checks if the diff has all discussions collpased
+ * @param {Object} diff
+ * @returns {Boolean}
+ */
+export const diffHasAllCollpasedDiscussions = (state, getters) => diff => {
+  const discussions = getters.getDiffFileDiscussions(diff);
+
+  return (discussions.length && discussions.every(discussion => !discussion.expanded)) || false;
+};
+
+/**
+ * Checks if the diff has any open discussions
+ * @param {Object} diff
+ * @returns {Boolean}
+ */
+export const diffHasExpandedDiscussions = (state, getters) => diff => {
+  const discussions = getters.getDiffFileDiscussions(diff);
+
+  return (
+    (discussions.length && discussions.find(discussion => discussion.expanded) !== undefined) ||
+    false
+  );
+};
+
+/**
+ * Returns an array with the discussions of the given diff
+ * @param {Object} diff
+ * @returns {Array}
+ */
+export const getDiffFileDiscussions = (state, getters, rootState, rootGetters) => diff =>
+  rootGetters.discussions.filter(
+    discussion =>
+      discussion.diff_discussion && _.isEqual(discussion.diff_file.file_hash, diff.fileHash),
+  ) || [];
+
+// prevent babel-plugin-rewire from generating an invalid default during karma∂ tests
 export default () => {};

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

@@ -15,6 +15,8 @@ let eTagPoll;
 
 export const expandDiscussion = ({ commit }, data) => commit(types.EXPAND_DISCUSSION, data);
 
+export const collapseDiscussion = ({ commit }, data) => commit(types.COLLAPSE_DISCUSSION, data);
+
 export const setNotesData = ({ commit }, data) => commit(types.SET_NOTES_DATA, data);
 
 export const setNoteableData = ({ commit }, data) => commit(types.SET_NOTEABLE_DATA, data);

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

 export const ADD_NEW_NOTE = 'ADD_NEW_NOTE';
 export const ADD_NEW_REPLY_TO_DISCUSSION = 'ADD_NEW_REPLY_TO_DISCUSSION';
 export const DELETE_NOTE = 'DELETE_NOTE';
-export const EXPAND_DISCUSSION = 'EXPAND_DISCUSSION';
 export const REMOVE_PLACEHOLDER_NOTES = 'REMOVE_PLACEHOLDER_NOTES';
 export const SET_NOTES_DATA = 'SET_NOTES_DATA';
 export const SET_NOTEABLE_DATA = 'SET_NOTEABLE_DATA';
@@ -11,12 +10,16 @@ export const SET_LAST_FETCHED_AT = 'SET_LAST_FETCHED_AT';
 export const SET_TARGET_NOTE_HASH = 'SET_TARGET_NOTE_HASH';
 export const SHOW_PLACEHOLDER_NOTE = 'SHOW_PLACEHOLDER_NOTE';
 export const TOGGLE_AWARD = 'TOGGLE_AWARD';
-export const TOGGLE_DISCUSSION = 'TOGGLE_DISCUSSION';
 export const UPDATE_NOTE = 'UPDATE_NOTE';
 export const UPDATE_DISCUSSION = 'UPDATE_DISCUSSION';
 export const SET_DISCUSSION_DIFF_LINES = 'SET_DISCUSSION_DIFF_LINES';
 export const SET_NOTES_FETCHED_STATE = 'SET_NOTES_FETCHED_STATE';
 
+// DISCUSSION
+export const COLLAPSE_DISCUSSION = 'COLLAPSE_DISCUSSION';
+export const EXPAND_DISCUSSION = 'EXPAND_DISCUSSION';
+export const TOGGLE_DISCUSSION = 'TOGGLE_DISCUSSION';
+
 // Issue
 export const CLOSE_ISSUE = 'CLOSE_ISSUE';
 export const REOPEN_ISSUE = 'REOPEN_ISSUE';

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

@@ -58,6 +58,11 @@ export default {
     discussion.expanded = true;
   },
 
+  [types.COLLAPSE_DISCUSSION](state, { discussionId }) {
+    const discussion = utils.findNoteObjectById(state.discussions, discussionId);
+    discussion.expanded = false;
+  },
+
   [types.REMOVE_PLACEHOLDER_NOTES](state) {
     const { discussions } = state;
 

changelogs/unreleased/48237-toggle-file-comments.yml

+---
+title: Fixes toggle discussion button not expanding collapsed discussions
+merge_request: 20452
+author:
+type: fixed

spec/features/projects/merge_requests/user_comments_on_diff_spec.rb

@@ -31,7 +31,7 @@ describe 'User comments on a diff', :js do
           page.within('.files > div:nth-child(3)') do
             expect(page).to have_content('Line is wrong')
 
-            find('.js-toggle-diff-comments').click
+            find('.js-btn-vue-toggle-comments').click
 
             expect(page).not_to have_content('Line is wrong')
           end
@@ -64,7 +64,7 @@ describe 'User comments on a diff', :js do
 
           # Hide the comment.
           page.within('.files > div:nth-child(3)') do
-            find('.js-toggle-diff-comments').click
+            find('.js-btn-vue-toggle-comments').click
 
             expect(page).not_to have_content('Line is wrong')
           end
@@ -77,7 +77,7 @@ describe 'User comments on a diff', :js do
 
           # Show the comment.
           page.within('.files > div:nth-child(3)') do
-            find('.js-toggle-diff-comments').click
+            find('.js-btn-vue-toggle-comments').click
           end
 
           # Now both the comments should be shown.

spec/javascripts/diffs/store/actions_spec.js

@@ -191,4 +191,48 @@ describe('DiffsStoreActions', () => {
       );
     });
   });
+
+  describe('toggleFileDiscussions', () => {
+    it('should dispatch collapseDiscussion when all discussions are expanded', () => {
+      const getters = {
+        getDiffFileDiscussions: jasmine.createSpy().and.returnValue([{ id: 1 }]),
+        diffHasAllExpandedDiscussions: jasmine.createSpy().and.returnValue(true),
+        diffHasAllCollpasedDiscussions: jasmine.createSpy().and.returnValue(false),
+      };
+
+      const dispatch = jasmine.createSpy('dispatch');
+
+      actions.toggleFileDiscussions({ getters, dispatch });
+
+      expect(dispatch).toHaveBeenCalledWith('collapseDiscussion', { discussionId: 1 }, { root: true });
+    });
+
+    it('should dispatch expandDiscussion when all discussions are collapsed', () => {
+      const getters = {
+        getDiffFileDiscussions: jasmine.createSpy().and.returnValue([{ id: 1 }]),
+        diffHasAllExpandedDiscussions: jasmine.createSpy().and.returnValue(false),
+        diffHasAllCollpasedDiscussions: jasmine.createSpy().and.returnValue(true),
+      };
+
+      const dispatch = jasmine.createSpy();
+
+      actions.toggleFileDiscussions({ getters, dispatch });
+
+      expect(dispatch).toHaveBeenCalledWith('expandDiscussion', { discussionId: 1 }, { root: true });
+    });
+
+    it('should dispatch expandDiscussion when some discussions are collapsed and others are expanded for the collapsed discussion', () => {
+      const getters = {
+        getDiffFileDiscussions: jasmine.createSpy().and.returnValue([{ expanded: false, id: 1 }]),
+        diffHasAllExpandedDiscussions: jasmine.createSpy().and.returnValue(false),
+        diffHasAllCollpasedDiscussions: jasmine.createSpy().and.returnValue(false),
+      };
+
+      const dispatch = jasmine.createSpy();
+
+      actions.toggleFileDiscussions({ getters, dispatch });
+
+      expect(dispatch).toHaveBeenCalledWith('expandDiscussion', { discussionId: 1 }, { root: true });
+    });
+  });
 });

spec/javascripts/diffs/store/getters_spec.js

 import * as getters from '~/diffs/store/getters';
 import state from '~/diffs/store/modules/diff_state';
 import { PARALLEL_DIFF_VIEW_TYPE, INLINE_DIFF_VIEW_TYPE } from '~/diffs/constants';
+import discussion from '../mock_data/diff_discussions';
 
-describe('DiffsStoreGetters', () => {
+describe('Diffs Module Getters', () => {
   let localState;
+  let discussionMock;
+  let discussionMock1;
+
+  const diffFileMock = {
+    fileHash: '9732849daca6ae818696d9575f5d1207d1a7f8bb',
+  };
 
   beforeEach(() => {
     localState = state();
+    discussionMock = Object.assign({}, discussion);
+    discussionMock.diff_file.file_hash = diffFileMock.fileHash;
+
+    discussionMock1 = Object.assign({}, discussion);
+    discussionMock1.diff_file.file_hash = diffFileMock.fileHash;
   });
 
   describe('isParallelView', () => {
@@ -63,4 +75,113 @@ describe('DiffsStoreGetters', () => {
       expect(getters.commitId(localState)).toEqual(null);
     });
   });
+
+  describe('diffHasAllExpandedDiscussions', () => {
+    it('returns true when all discussions are expanded', () => {
+      expect(
+        getters.diffHasAllExpandedDiscussions(localState, {
+          getDiffFileDiscussions: () => [discussionMock, discussionMock],
+        })(diffFileMock),
+      ).toEqual(true);
+    });
+
+    it('returns false when there are no discussions', () => {
+      expect(
+        getters.diffHasAllExpandedDiscussions(localState, {
+          getDiffFileDiscussions: () => [],
+        })(diffFileMock),
+      ).toEqual(false);
+    });
+
+    it('returns false when one discussions is collapsed', () => {
+      discussionMock1.expanded = false;
+
+      expect(
+        getters.diffHasAllExpandedDiscussions(localState, {
+          getDiffFileDiscussions: () => [discussionMock, discussionMock1],
+        })(diffFileMock),
+      ).toEqual(false);
+    });
+  });
+
+  describe('diffHasAllCollpasedDiscussions', () => {
+    it('returns true when all discussions are collapsed', () => {
+      discussionMock.diff_file.file_hash = diffFileMock.fileHash;
+      discussionMock.expanded = false;
+
+      expect(
+        getters.diffHasAllCollpasedDiscussions(localState, {
+          getDiffFileDiscussions: () => [discussionMock],
+        })(diffFileMock),
+      ).toEqual(true);
+    });
+
+    it('returns false when there are no discussions', () => {
+      expect(
+        getters.diffHasAllCollpasedDiscussions(localState, {
+          getDiffFileDiscussions: () => [],
+        })(diffFileMock),
+      ).toEqual(false);
+    });
+
+    it('returns false when one discussions is expanded', () => {
+      discussionMock1.expanded = false;
+
+      expect(
+        getters.diffHasAllCollpasedDiscussions(localState, {
+          getDiffFileDiscussions: () => [discussionMock, discussionMock1],
+        })(diffFileMock),
+      ).toEqual(false);
+    });
+  });
+
+  describe('diffHasExpandedDiscussions', () => {
+    it('returns true when one of the discussions is expanded', () => {
+      discussionMock1.expanded = false;
+
+      expect(
+        getters.diffHasExpandedDiscussions(localState, {
+          getDiffFileDiscussions: () => [discussionMock, discussionMock],
+        })(diffFileMock),
+      ).toEqual(true);
+    });
+
+    it('returns false when there are no discussions', () => {
+      expect(
+        getters.diffHasExpandedDiscussions(localState, { getDiffFileDiscussions: () => [] })(
+          diffFileMock,
+        ),
+      ).toEqual(false);
+    });
+
+    it('returns false when no discussion is expanded', () => {
+      discussionMock.expanded = false;
+      discussionMock1.expanded = false;
+
+      expect(
+        getters.diffHasExpandedDiscussions(localState, {
+          getDiffFileDiscussions: () => [discussionMock, discussionMock1],
+        })(diffFileMock),
+      ).toEqual(false);
+    });
+  });
+
+  describe('getDiffFileDiscussions', () => {
+    it('returns an array with discussions when fileHash matches and the discussion belongs to a diff', () => {
+      discussionMock.diff_file.file_hash = diffFileMock.fileHash;
+
+      expect(
+        getters.getDiffFileDiscussions(localState, {}, {}, { discussions: [discussionMock] })(
+          diffFileMock,
+        ).length,
+      ).toEqual(1);
+    });
+
+    it('returns an empty array when no discussions are found in the given diff', () => {
+      expect(
+        getters.getDiffFileDiscussions(localState, {}, {}, { discussions: [] })(diffFileMock)
+          .length,
+      ).toEqual(0);
+    });
+  });
 });

spec/javascripts/notes/stores/actions_spec.js

@@ -128,6 +128,19 @@ describe('Actions Notes Store', () => {
     });
   });
 
+  describe('collapseDiscussion', () => {
+    it('should commit collapse discussion', done => {
+      testAction(
+        actions.collapseDiscussion,
+        { discussionId: discussionMock.id },
+        { notes: [discussionMock] },
+        [{ type: 'COLLAPSE_DISCUSSION', payload: { discussionId: discussionMock.id } }],
+        [],
+        done,
+      );
+    });
+  });
+
   describe('async methods', () => {
     const interceptor = (request, next) => {
       next(

spec/javascripts/notes/stores/mutation_spec.js

@@ -74,6 +74,20 @@ describe('Notes Store mutations', () => {
     });
   });
 
+  describe('COLLAPSE_DISCUSSION', () => {
+    it('should collpase an expanded discussion', () => {
+      const discussion = Object.assign({}, discussionMock, { expanded: true });
+
+      const state = {
+        discussions: [discussion],
+      };
+
+      mutations.COLLAPSE_DISCUSSION(state, { discussionId: discussion.id });
+
+      expect(state.discussions[0].expanded).toEqual(false);
+    });
+  });
+
   describe('REMOVE_PLACEHOLDER_NOTES', () => {
     it('should remove all placeholder notes in indivudal notes and discussion', () => {
       const placeholderNote = Object.assign({}, individualNote, { isPlaceholderNote: true });