Resolve "Filter discussion (tab) by comments or activity in issues and merge requests"

app/assets/javascripts/mr_notes/index.js

@@ -4,6 +4,7 @@ import { mapActions, mapState, mapGetters } from 'vuex';
 import initDiffsApp from '../diffs';
 import notesApp from '../notes/components/notes_app.vue';
 import discussionCounter from '../notes/components/discussion_counter.vue';
+import initDiscussionFilters from '../notes/discussion_filters';
 import store from './stores';
 import MergeRequest from '../merge_request';
@@ -88,5 +89,6 @@ export default function initMrNotes() {
     },
   });
+  initDiscussionFilters(store);
   initDiffsApp(store);
 }

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

@@ -56,10 +56,11 @@ export default {
 </script>
 <template>
-  <div class="line-resolve-all-container prepend-top-10">
+  <div
+    v-if="discussionCount > 0"
+    class="line-resolve-all-container prepend-top-8">
     <div>
       <div
-        v-if="discussionCount > 0"
         :class="{ 'has-next-btn': hasNextButton }"
         class="line-resolve-all">
         <span

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

+<script>
+import $ from 'jquery';
+import Icon from '~/vue_shared/components/icon.vue';
+import { mapGetters, mapActions } from 'vuex';
+
+export default {
+  components: {
+    Icon,
+  },
+  props: {
+    filters: {
+      type: Array,
+      required: true,
+    },
+    defaultValue: {
+      type: Number,
+      default: null,
+      required: false,
+    },
+  },
+  data() {
+    return { currentValue: this.defaultValue };
+  },
+  computed: {
+    ...mapGetters([
+      'getNotesDataByProp',
+    ]),
+    currentFilter() {
+      if (!this.currentValue) return this.filters[0];
+      return this.filters.find(filter => filter.value === this.currentValue);
+    },
+  },
+  methods: {
+    ...mapActions(['filterDiscussion']),
+    selectFilter(value) {
+      const filter = parseInt(value, 10);
+
+      // close dropdown
+      $(this.$refs.dropdownToggle).dropdown('toggle');
+
+      if (filter === this.currentValue) return;
+      this.currentValue = filter;
+      this.filterDiscussion({ path: this.getNotesDataByProp('discussionsPath'), filter });
+    },
+  },
+};
+</script>
+
+<template>
+  <div class="discussion-filter-container d-inline-block align-bottom">
+    <button
+      id="discussion-filter-dropdown"
+      ref="dropdownToggle"
+      class="btn btn-default"
+      data-toggle="dropdown"
+      aria-expanded="false"
+    >
+      {{ currentFilter.title }}
+      <icon name="chevron-down" />
+    </button>
+    <div
+      class="dropdown-menu dropdown-menu-selectable dropdown-menu-right"
+      aria-labelledby="discussion-filter-dropdown">
+      <div class="dropdown-content">
+        <ul>
+          <li
+            v-for="filter in filters"
+            :key="filter.value"
+          >
+            <button
+              :class="{ 'is-active': filter.value === currentValue }"
+              type="button"
+              @click="selectFilter(filter.value)"
+            >
+              {{ filter.title }}
+            </button>
+          </li>
+        </ul>
+      </div>
+    </div>
+  </div>
+</template>

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

@@ -50,11 +50,11 @@ export default {
   },
   data() {
     return {
-      isLoading: true,
+      currentFilter: null,
     };
   },
   computed: {
-    ...mapGetters(['isNotesFetched', 'discussions', 'getNotesDataByProp', 'discussionCount']),
+    ...mapGetters(['isNotesFetched', 'discussions', 'getNotesDataByProp', 'discussionCount', 'isLoading']),
     noteableType() {
       return this.noteableData.noteableType;
     },
@@ -102,6 +102,7 @@ export default {
   },
   methods: {
     ...mapActions({
+      setLoadingState: 'setLoadingState',
       fetchDiscussions: 'fetchDiscussions',
       poll: 'poll',
       actionToggleAward: 'toggleAward',
@@ -133,19 +134,19 @@ export default {
       return discussion.individual_note ? { note: discussion.notes[0] } : { discussion };
     },
     fetchNotes() {
-      return this.fetchDiscussions(this.getNotesDataByProp('discussionsPath'))
+      return this.fetchDiscussions({ path: this.getNotesDataByProp('discussionsPath') })
         .then(() => {
           this.initPolling();
         })
         .then(() => {
-          this.isLoading = false;
+          this.setLoadingState(false);
           this.setNotesFetchedState(true);
           eventHub.$emit('fetchedNotesData');
         })
         .then(() => this.$nextTick())
         .then(() => this.checkLocationHash())
         .catch(() => {
-          this.isLoading = false;
+          this.setLoadingState(false);
           this.setNotesFetchedState(true);
           Flash('Something went wrong while fetching comments. Please try again.');
         });

app/assets/javascripts/notes/discussion_filters.js

+import Vue from 'vue';
+import DiscussionFilter from './components/discussion_filter.vue';
+
+export default (store) => {
+  const discussionFilterEl = document.getElementById('js-vue-discussion-filter');
+
+  if (discussionFilterEl) {
+    const { defaultFilter, notesFilters } = discussionFilterEl.dataset;
+    const defaultValue = defaultFilter ? parseInt(defaultFilter, 10) : null;
+    const filterValues = notesFilters ? JSON.parse(notesFilters) : {};
+    const filters = Object.keys(filterValues).map(entry =>
+      ({ title: entry, value: filterValues[entry] }));
+
+    return new Vue({
+      el: discussionFilterEl,
+      name: 'DiscussionFilter',
+      components: {
+        DiscussionFilter,
+      },
+      store,
+      render(createElement) {
+        return createElement('discussion-filter', {
+          props: {
+            filters,
+            defaultValue,
+          },
+        });
+      },
+    });
+  }
+
+  return null;
+};

app/assets/javascripts/notes/index.js

 import Vue from 'vue';
 import notesApp from './components/notes_app.vue';
+import initDiscussionFilters from './discussion_filters';
 import createStore from './stores';
 document.addEventListener('DOMContentLoaded', () => {
   const store = createStore();
+  initDiscussionFilters(store);
+
   return new Vue({
     el: '#js-vue-notes',
     components: {

app/assets/javascripts/notes/services/notes_service.js

@@ -5,8 +5,9 @@ import * as constants from '../constants';
 Vue.use(VueResource);
 export default {
-  fetchDiscussions(endpoint) {
-    return Vue.http.get(endpoint);
+  fetchDiscussions(endpoint, filter) {
+    const config = filter !== undefined ? { params: { notes_filter: filter } } : null;
+    return Vue.http.get(endpoint, config);
   },
   deleteNote(endpoint) {
     return Vue.http.delete(endpoint);

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

@@ -11,6 +11,7 @@ import loadAwardsHandler from '../../awards_handler';
 import sidebarTimeTrackingEventHub from '../../sidebar/event_hub';
 import { isInViewport, scrollToElement } from '../../lib/utils/common_utils';
 import mrWidgetEventHub from '../../vue_merge_request_widget/event_hub';
+import { __ } from '~/locale';
 let eTagPoll;
@@ -36,9 +37,9 @@ export const setNotesFetchedState = ({ commit }, state) =>
 export const toggleDiscussion = ({ commit }, data) => commit(types.TOGGLE_DISCUSSION, data);
-export const fetchDiscussions = ({ commit }, path) =>
+export const fetchDiscussions = ({ commit }, { path, filter }) =>
   service
-    .fetchDiscussions(path)
+    .fetchDiscussions(path, filter)
     .then(res => res.json())
     .then(discussions => {
       commit(types.SET_INITIAL_DISCUSSIONS, discussions);
@@ -251,7 +252,7 @@ const pollSuccessCallBack = (resp, commit, state, getters, dispatch) => {
         if (discussion) {
           commit(types.ADD_NEW_REPLY_TO_DISCUSSION, note);
         } else if (note.type === constants.DIFF_NOTE) {
-          dispatch('fetchDiscussions', state.notesData.discussionsPath);
+          dispatch('fetchDiscussions', { path: state.notesData.discussionsPath });
         } else {
           commit(types.ADD_NEW_NOTE, note);
         }
@@ -345,5 +346,23 @@ export const updateMergeRequestWidget = () => {
   mrWidgetEventHub.$emit('mr.discussion.updated');
 };
+export const setLoadingState = ({ commit }, data) => {
+  commit(types.SET_NOTES_LOADING_STATE, data);
+};
+
+export const filterDiscussion = ({ dispatch }, { path, filter }) => {
+  dispatch('setLoadingState', true);
+  dispatch('fetchDiscussions', { path, filter })
+    .then(() => {
+      dispatch('setLoadingState', false);
+      dispatch('setNotesFetchedState', true);
+    })
+    .catch(() => {
+      dispatch('setLoadingState', false);
+      dispatch('setNotesFetchedState', true);
+      Flash(__('Something went wrong while fetching comments. Please try again.'));
+    });
+};
+
 // prevent babel-plugin-rewire from generating an invalid default during karma tests
 export default () => {};

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

@@ -11,6 +11,8 @@ export const getNotesData = state => state.notesData;
 export const isNotesFetched = state => state.isNotesFetched;
+export const isLoading = state => state.isLoading;
+
 export const getNotesDataByProp = state => prop => state.notesData[prop];
 export const getNoteableData = state => state.noteableData;

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

@@ -11,6 +11,7 @@ export default () => ({
     // View layer
     isToggleStateButtonLoading: false,
     isNotesFetched: false,
+    isLoading: true,
     // holds endpoints and permissions provided through haml
     notesData: {

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

@@ -14,6 +14,7 @@ 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';
+export const SET_NOTES_LOADING_STATE = 'SET_NOTES_LOADING_STATE';
 // DISCUSSION
 export const COLLAPSE_DISCUSSION = 'COLLAPSE_DISCUSSION';

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

@@ -216,6 +216,10 @@ export default {
     Object.assign(state, { isNotesFetched: value });
   },
+  [types.SET_NOTES_LOADING_STATE](state, value) {
+    state.isLoading = value;
+  },
+
   [types.SET_DISCUSSION_DIFF_LINES](state, { discussionId, diffLines }) {
     const discussion = utils.findNoteObjectById(state.discussions, discussionId);

app/assets/stylesheets/pages/issues.scss

@@ -185,7 +185,17 @@ ul.related-merge-requests > li {
 }
 .new-branch-col {
-  padding-top: 10px;
+  font-size: 0;
+
+  .discussion-filter-container {
+    &:not(:only-child) {
+      margin-right: $gl-padding-8;
+    }
+
+    @include media-breakpoint-down(md) {
+      margin-top: $gl-padding-8;
+    }
+  }
 }
 .create-mr-dropdown-wrap {
@@ -205,6 +215,10 @@ ul.related-merge-requests > li {
   .btn-group:not(.hidden) {
     display: flex;
+
+    @include media-breakpoint-down(md) {
+      margin-top: $gl-padding-8;
+    }
   }
   .js-create-merge-request {
@@ -251,7 +265,6 @@ ul.related-merge-requests > li {
     .new-branch-col {
       padding-top: 0;
-      text-align: right;
       align-self: center;
     }
@@ -262,3 +275,9 @@ ul.related-merge-requests > li {
     }
   }
 }
+
+@include media-breakpoint-up(lg) {
+  .new-branch-col {
+    text-align: right;
+  }
+}

app/assets/stylesheets/pages/merge_requests.scss

@@ -818,9 +818,17 @@
   display: flex;
   justify-content: space-between;
-  @include media-breakpoint-down(xs) {
+  @include media-breakpoint-down(md) {
     flex-direction: column-reverse;
   }
+
+  .discussion-filter-container {
+    margin-top: $gl-padding-8;
+
+    &:not(:only-child) {
+      padding-right: $gl-padding-8;
+    }
+  }
 }
 .limit-container-width:not(.container-limited) {

app/assets/stylesheets/pages/notes.scss

@@ -618,7 +618,6 @@ ul.notes {
 .line-resolve-all-container {
   @include notes-media('min', map-get($grid-breakpoints, sm)) {
     margin-right: 0;
-    padding-left: $gl-padding;
   }
   > div {
@@ -756,3 +755,23 @@ ul.notes {
     margin-top: 4px;
   }
 }
+
+.discussion-filter-container {
+
+  .btn > svg {
+    width: $gl-col-padding;
+    height: $gl-col-padding;
+  }
+
+  .dropdown-menu {
+    margin-bottom: $gl-padding-4;
+
+    @include media-breakpoint-down(md) {
+      margin-left: $btn-side-margin + $contextual-sidebar-collapsed-width;
+    }
+
+    @include media-breakpoint-down(xs) {
+      margin-left: $btn-side-margin;
+    }
+  }
+}

app/controllers/concerns/issuable_actions.rb

@@ -2,6 +2,7 @@
 module IssuableActions
   extend ActiveSupport::Concern
+  include Gitlab::Utils::StrongMemoize
   included do
     before_action :labels, only: [:show, :new, :edit]
@@ -95,10 +96,14 @@ module IssuableActions
   def discussions
     notes = issuable.discussion_notes
       .inc_relations_for_view
+      .with_notes_filter(notes_filter)
       .includes(:noteable)
       .fresh
-    notes = ResourceEvents::MergeIntoNotesService.new(issuable, current_user).execute(notes)
+    if notes_filter != UserPreference::NOTES_FILTERS[:only_comments]
+      notes = ResourceEvents::MergeIntoNotesService.new(issuable, current_user).execute(notes)
+    end
+
     notes = prepare_notes_for_rendering(notes)
     notes = notes.reject { |n| n.cross_reference_not_visible_for?(current_user) }
@@ -110,6 +115,32 @@ module IssuableActions
   private
+  def notes_filter
+    strong_memoize(:notes_filter) do
+      notes_filter_param = params[:notes_filter]&.to_i
+
+      # GitLab Geo does not expect database UPDATE or INSERT statements to happen
+      # on GET requests.
+      # This is just a fail-safe in case notes_filter is sent via GET request in GitLab Geo.
+      if Gitlab::Database.read_only?
+        notes_filter_param || current_user&.notes_filter_for(issuable)
+      else
+        notes_filter = current_user&.set_notes_filter(notes_filter_param, issuable) || notes_filter_param
+
+        # We need to invalidate the cache for polling notes otherwise it will
+        # ignore the filter.
+        # The ideal would be to invalidate the cache for each user.
+        issuable.expire_note_etag_cache if notes_filter_updated?
+
+        notes_filter
+      end
+    end
+  end
+
+  def notes_filter_updated?
+    current_user&.user_preference&.previous_changes&.any?
+  end
+
   def discussion_serializer
     DiscussionSerializer.new(project: project, noteable: issuable, current_user: current_user, note_entity: ProjectNoteEntity)
   end

app/controllers/concerns/notes_actions.rb

@@ -17,10 +17,17 @@ module NotesActions
     notes_json = { notes: [], last_fetched_at: current_fetched_at }
-    notes = notes_finder.execute
-      .inc_relations_for_view
-    notes = ResourceEvents::MergeIntoNotesService.new(noteable, current_user, last_fetched_at: current_fetched_at).execute(notes)
+    notes = notes_finder
+              .execute
+              .inc_relations_for_view
+
+    if notes_filter != UserPreference::NOTES_FILTERS[:only_comments]
+      notes =
+        ResourceEvents::MergeIntoNotesService
+          .new(noteable, current_user, last_fetched_at: current_fetched_at)
+          .execute(notes)
+    end
     notes = prepare_notes_for_rendering(notes)
     notes = notes.reject { |n| n.cross_reference_not_visible_for?(current_user) }
@@ -224,6 +231,10 @@ module NotesActions
     request.headers['X-Last-Fetched-At']
   end
+  def notes_filter
+    current_user&.notes_filter_for(params[:target_type])
+  end
+
   def notes_finder
     @notes_finder ||= NotesFinder.new(project, current_user, finder_params)
   end

app/controllers/projects/notes_controller.rb

@@ -68,7 +68,7 @@ class Projects::NotesController < Projects::ApplicationController
   alias_method :awardable, :note
   def finder_params
-    params.merge(last_fetched_at: last_fetched_at)
+    params.merge(last_fetched_at: last_fetched_at, notes_filter: notes_filter)
   end
   def authorize_admin_note!

app/finders/notes_finder.rb

@@ -24,6 +24,8 @@ class NotesFinder
   def execute
     notes = init_collection
     notes = since_fetch_at(notes)
+    notes = notes.with_notes_filter(@params[:notes_filter]) if notes_filter?
+
     notes.fresh
   end
@@ -134,4 +136,8 @@ class NotesFinder
     last_fetched_at = Time.at(@params.fetch(:last_fetched_at, 0).to_i)
     notes.updated_after(last_fetched_at - FETCH_OVERLAP)
   end
+
+  def notes_filter?
+    @params[:notes_filter].present?
+  end
 end

app/models/note.rb

@@ -110,6 +110,15 @@ class Note < ActiveRecord::Base
              :system_note_metadata, :note_diff_file)
   end
+  scope :with_notes_filter, -> (notes_filter) do
+    case notes_filter
+    when UserPreference::NOTES_FILTERS[:only_comments]
+      user
+    else
+      all
+    end
+  end
+
   scope :diff_notes, -> { where(type: %w(LegacyDiffNote DiffNote)) }
   scope :new_diff_notes, -> { where(type: 'DiffNote') }
   scope :non_diff_notes, -> { where(type: ['Note', 'DiscussionNote', nil]) }