Merge branch 'ee-26723-discussion-filters' into 'master'

[EE-PORT] Resolve "Filter discussion (tab) by comments or activity in issues and merge requests"

See merge request gitlab-org/gitlab-ee!7816

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

@@ -234,7 +234,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 {
@@ -254,6 +264,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 {
@@ -300,7 +314,6 @@ ul.related-merge-requests > li {
 
     .new-branch-col {
       padding-top: 0;
-      text-align: right;
       align-self: center;
     }
 
@@ -312,6 +325,11 @@ ul.related-merge-requests > li {
   }
 }
 
+@include media-breakpoint-up(lg) {
+  .new-branch-col {
+    text-align: right;
+  }
+}
 
 .issue-token {
   display: inline-flex;

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

@@ -3,6 +3,7 @@
 module IssuableActions
   prepend EE::IssuableActions
   extend ActiveSupport::Concern
+  include Gitlab::Utils::StrongMemoize
 
   included do
     before_action :labels, only: [:show, :new, :edit]
@@ -96,10 +97,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) }
 
@@ -111,6 +116,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 = 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 = ResourceEvents::MergeIntoNotesService.new(noteable, current_user, last_fetched_at: current_fetched_at).execute(notes)
     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

@@ -26,6 +26,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
 
@@ -136,4 +138,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

@@ -114,6 +114,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]) }

app/models/user.rb

@@ -154,6 +154,7 @@ class User < ActiveRecord::Base
   belongs_to :accepted_term, class_name: 'ApplicationSetting::Term'
 
   has_one :status, class_name: 'UserStatus'
+  has_one :user_preference
 
   #
   # Validations
@@ -226,6 +227,8 @@ class User < ActiveRecord::Base
   enum project_view: [:readme, :activity, :files]
 
   delegate :path, to: :namespace, allow_nil: true, prefix: true
+  delegate :notes_filter_for, to: :user_preference
+  delegate :set_notes_filter, to: :user_preference
 
   accepts_nested_attributes_for :namespace
 
@@ -1389,6 +1392,11 @@ class User < ActiveRecord::Base
     !consented_usage_stats? && 7.days.ago > self.created_at && !has_current_license? && User.single_user?
   end
 
+  # Avoid migrations only building user preference object when needed.
+  def user_preference
+    super.presence || build_user_preference
+  end
+
   def todos_limited_to(ids)
     todos.where(id: ids)
   end

app/models/user_preference.rb

+# frozen_string_literal: true
+
+class UserPreference < ActiveRecord::Base
+  # We could use enums, but Rails 4 doesn't support multiple
+  # enum options with same name for multiple fields, also it creates
+  # extra methods that aren't really needed here.
+  NOTES_FILTERS = { all_notes: 0, only_comments: 1 }.freeze
+
+  belongs_to :user
+
+  validates :issue_notes_filter, :merge_request_notes_filter, inclusion: { in: NOTES_FILTERS.values }, presence: true
+
+  class << self
+    def notes_filters
+      {
+        s_('Notes|Show all activity') => NOTES_FILTERS[:all_notes],
+        s_('Notes|Show comments only') => NOTES_FILTERS[:only_comments]
+      }
+    end
+  end
+
+  def set_notes_filter(filter_id, issuable)
+    # No need to update the column if the value is already set.
+    if filter_id && NOTES_FILTERS.values.include?(filter_id)
+      field = notes_filter_field_for(issuable)
+      self[field] = filter_id
+
+      save if attribute_changed?(field)
+    end
+
+    notes_filter_for(issuable)
+  end
+
+  # Returns the current discussion filter for a given issuable
+  # or issuable type.
+  def notes_filter_for(resource)
+    self[notes_filter_field_for(resource)]
+  end
+
+  private
+
+  def notes_filter_field_for(resource)
+    field_key =
+      if resource.is_a?(Issuable)
+        resource.model_name.param_key
+      else
+        resource
+      end
+
+    "#{field_key}_notes_filter"
+  end
+end

app/serializers/current_user_entity.rb

+# frozen_string_literal: true
+
+# Always use this entity when rendering data for current user
+# for attributes that does not need to be visible to other users
+# like user preferences.
+class CurrentUserEntity < UserEntity
+  expose :user_preference, using: UserPreferenceEntity
+end

app/serializers/merge_request_user_entity.rb

 # frozen_string_literal: true
 
-class MergeRequestUserEntity < UserEntity
+class MergeRequestUserEntity < CurrentUserEntity
   include RequestAwareEntity
   include BlobHelper
   include TreeHelper

app/serializers/user_preference_entity.rb

+# frozen_string_literal: true
+
+class UserPreferenceEntity < Grape::Entity
+  expose :issue_notes_filter
+  expose :merge_request_notes_filter
+
+  expose :notes_filters do |user_preference|
+    UserPreference.notes_filters
+  end
+end

app/views/projects/issues/_discussion.html.haml

@@ -10,4 +10,4 @@
     noteable_data: serialize_issuable(@issue),
     noteable_type: 'Issue',
     target_type: 'issue',
-    current_user_data: UserSerializer.new.represent(current_user, only_path: true).to_json } }
+    current_user_data: UserSerializer.new.represent(current_user, {only_path: true}, CurrentUserEntity).to_json } }

app/views/projects/issues/_new_branch.html.haml

@@ -8,12 +8,13 @@
   - create_branch_path = project_branches_path(@project, branch_name: @issue.to_branch_name, ref: @project.default_branch, issue_iid: @issue.iid)
   - refs_path = refs_namespace_project_path(@project.namespace, @project, search: '')
 
-  .create-mr-dropdown-wrap{ data: { can_create_path: can_create_path, create_mr_path: create_mr_path, create_branch_path: create_branch_path, refs_path: refs_path } }
+  .create-mr-dropdown-wrap.d-inline-block{ data: { can_create_path: can_create_path, create_mr_path: create_mr_path, create_branch_path: create_branch_path, refs_path: refs_path } }
     .btn-group.unavailable
       %button.btn.btn-grouped{ type: 'button', disabled: 'disabled' }
         = icon('spinner', class: 'fa-spin')
         %span.text
           Checking branch availability…
+
     .btn-group.available.hidden
       %button.btn.js-create-merge-request.btn-success.btn-inverted{ type: 'button', data: { action: data_action } }
         = value

app/views/projects/issues/show.html.haml

@@ -87,11 +87,12 @@
     #related-branches{ data: { url: related_branches_project_issue_path(@project, @issue) } }
       // This element is filled in using JavaScript.
 
-  .content-block.emoji-block
+  .content-block.emoji-block.emoji-block-sticky
     .row
-      .col-sm-8.js-noteable-awards
+      .col-md-12.col-lg-6.js-noteable-awards
         = render 'award_emoji/awards_block', awardable: @issue, inline: true
-      .col-sm-4.new-branch-col
+      .col-md-12.col-lg-6.new-branch-col
+        #js-vue-discussion-filter{ data: { default_filter: current_user&.notes_filter_for(@issue), notes_filters: UserPreference.notes_filters.to_json } }
         = render 'new_branch' unless @issue.confidential?
 
   %section.issuable-discussion

app/views/projects/merge_requests/show.html.haml

@@ -51,8 +51,10 @@
               = tab_link_for @merge_request, :diffs do
                 Changes
                 %span.badge.badge-pill= @merge_request.diff_size
-
-        #js-vue-discussion-counter
+        .d-inline-flex.flex-wrap
+          #js-vue-discussion-filter{ data: { default_filter: current_user&.notes_filter_for(@merge_request),
+            notes_filters: UserPreference.notes_filters.to_json } }
+          #js-vue-discussion-counter
 
     .tab-content#diff-notes-app
       #notes.notes.tab-pane.voting_notes

changelogs/unreleased/26723-discussion-filters.yml

+---
+title: Filter notes by comments or activity for issues and merge requests
+merge_request:
+author:
+type: added

db/migrate/20180925200829_create_user_preferences.rb

+# frozen_string_literal: true
+
+class CreateUserPreferences < ActiveRecord::Migration
+  DOWNTIME = false
+
+  class UserPreference < ActiveRecord::Base
+    self.table_name = 'user_preferences'
+
+    NOTES_FILTERS = { all_notes: 0, comments: 1 }.freeze
+  end
+
+  def change
+    create_table :user_preferences do |t|
+      t.references :user,
+                   null: false,
+                   index: { unique: true },
+                   foreign_key: { on_delete: :cascade }
+
+      t.integer :issue_notes_filter,
+                default: UserPreference::NOTES_FILTERS[:all_notes],
+                null: false, limit: 2
+
+      t.integer :merge_request_notes_filter,
+                default: UserPreference::NOTES_FILTERS[:all_notes],
+                null: false,
+                limit: 2
+
+      t.timestamps_with_timezone null: false
+    end
+  end
+end

db/schema.rb

@@ -2873,6 +2873,16 @@ ActiveRecord::Schema.define(version: 20181013005024) do
   add_index "user_interacted_projects", ["project_id", "user_id"], name: "index_user_interacted_projects_on_project_id_and_user_id", unique: true, using: :btree
   add_index "user_interacted_projects", ["user_id"], name: "index_user_interacted_projects_on_user_id", using: :btree
 
+  create_table "user_preferences", force: :cascade do |t|
+    t.integer "user_id", null: false
+    t.integer "issue_notes_filter", limit: 2, default: 0, null: false
+    t.integer "merge_request_notes_filter", limit: 2, default: 0, null: false
+    t.datetime_with_timezone "created_at", null: false
+    t.datetime_with_timezone "updated_at", null: false
+  end
+
+  add_index "user_preferences", ["user_id"], name: "index_user_preferences_on_user_id", unique: true, using: :btree
+
   create_table "user_statuses", primary_key: "user_id", force: :cascade do |t|
     t.integer "cached_markdown_version"
     t.string "emoji", default: "speech_balloon", null: false
@@ -3370,6 +3380,7 @@ ActiveRecord::Schema.define(version: 20181013005024) do
   add_foreign_key "user_custom_attributes", "users", on_delete: :cascade
   add_foreign_key "user_interacted_projects", "projects", on_delete: :cascade
   add_foreign_key "user_interacted_projects", "users", on_delete: :cascade
+  add_foreign_key "user_preferences", "users", on_delete: :cascade
   add_foreign_key "user_statuses", "users", on_delete: :cascade
   add_foreign_key "user_synced_attributes_metadata", "users", on_delete: :cascade
   add_foreign_key "users", "application_setting_terms", column: "accepted_term_id", name: "fk_789cd90b35", on_delete: :cascade

ee/app/assets/javascripts/batch_comments/stores/modules/batch_comments/actions.js

@@ -70,10 +70,11 @@ export const publishReview = ({ commit, dispatch, getters }) => {
 };
 
 export const updateDiscussionsAfterPublish = ({ dispatch, getters, rootGetters }) =>
-  dispatch('fetchDiscussions', getters.getNotesData.discussionsPath, { root: true }).then(() =>
-    dispatch('diffs/assignDiscussionsToDiff', rootGetters.discussionsStructuredByLineCode, {
-      root: true,
-    }),
+  dispatch('fetchDiscussions', { path: getters.getNotesData.discussionsPath }, { root: true }).then(
+    () =>
+      dispatch('diffs/assignDiscussionsToDiff', rootGetters.discussionsStructuredByLineCode, {
+        root: true,
+      }),
   );
 
 export const discardReview = ({ commit, getters }) => {

locale/gitlab.pot

@@ -5383,6 +5383,12 @@ msgstr ""
 msgid "Notes|Are you sure you want to cancel creating this comment?"
 msgstr ""
 
+msgid "Notes|Show all activity"
+msgstr ""
+
+msgid "Notes|Show comments only"
+msgstr ""
+
 msgid "Notification events"
 msgstr ""
 
@@ -7230,6 +7236,9 @@ msgstr ""
 msgid "Something went wrong while fetching %{listType} list"
 msgstr ""
 
+msgid "Something went wrong while fetching comments. Please try again."
+msgstr ""
+
 msgid "Something went wrong while fetching group member contributions"
 msgstr ""
 

spec/controllers/projects/issues_controller_spec.rb

@@ -1028,6 +1028,13 @@ describe Projects::IssuesController do
           .not_to exceed_query_limit(control)
       end
 
+      context 'when user is setting notes filters' do
+        let(:issuable) { issue }
+        let!(:discussion_note) { create(:discussion_note_on_issue, :system, noteable: issuable, project: project) }
+
+        it_behaves_like 'issuable notes filter'
+      end
+
       context 'with cross-reference system note', :request_store do
         let(:new_issue) { create(:issue) }
         let(:cross_reference) { "mentioned in #{new_issue.to_reference(issue.project)}" }

spec/controllers/projects/merge_requests_controller_spec.rb

@@ -87,6 +87,14 @@ describe Projects::MergeRequestsController do
       end
     end
 
+    context 'when user is setting notes filters' do
+      let(:issuable) { merge_request }
+      let!(:discussion_note) { create(:discussion_note_on_merge_request, :system, noteable: issuable, project: project) }
+      let!(:discussion_comment) { create(:discussion_note_on_merge_request, noteable: issuable, project: project) }
+
+      it_behaves_like 'issuable notes filter'
+    end
+
     describe 'as json' do
       context 'with basic serializer param' do
         it 'renders basic MR entity as json' do

spec/controllers/projects/notes_controller_spec.rb

@@ -47,6 +47,37 @@ describe Projects::NotesController do
       get :index, request_params
     end
 
+    context 'when user notes_filter is present' do
+      let(:notes_json) { parsed_response[:notes] }
+      let!(:comment) { create(:note, noteable: issue, project: project) }
+      let!(:system_note) { create(:note, noteable: issue, project: project, system: true) }
+
+      it 'filters system notes by comments' do
+        user.set_notes_filter(UserPreference::NOTES_FILTERS[:only_comments], issue)
+
+        get :index, request_params
+
+        expect(notes_json.count).to eq(1)
+        expect(notes_json.first[:id].to_i).to eq(comment.id)
+      end
+
+      it 'returns all notes' do
+        user.set_notes_filter(UserPreference::NOTES_FILTERS[:all_notes], issue)
+
+        get :index, request_params
+
+        expect(notes_json.map { |note| note[:id].to_i }).to contain_exactly(comment.id, system_note.id)
+      end
+
+      it 'does not merge label event notes' do
+        user.set_notes_filter(UserPreference::NOTES_FILTERS[:only_comments], issue)
+
+        expect(ResourceEvents::MergeIntoNotesService).not_to receive(:new)
+
+        get :index, request_params
+      end
+    end
+
     context 'for a discussion note' do
       let(:project) { create(:project, :repository) }
       let!(:note) { create(:discussion_note_on_merge_request, project: project) }

spec/factories/user_preferences.rb

+# frozen_string_literal: true
+
+FactoryBot.define do
+  factory :user_preference do
+    user
+
+    trait :only_comments do
+      issue_notes_filter { UserPreference::NOTES_FILTERS[:only_comments] }
+      merge_request_notes_filter { UserPreference::NOTE_FILTERS[:only_comments] }
+    end
+  end
+end

spec/finders/notes_finder_spec.rb

@@ -9,6 +9,27 @@ describe NotesFinder do
   end
 
   describe '#execute' do
+    context 'when notes filter is present' do
+      let!(:comment) { create(:note_on_issue, project: project) }
+      let!(:system_note) { create(:note_on_issue, project: project, system: true) }
+
+      it 'filters system notes' do
+        finder = described_class.new(project, user, notes_filter: UserPreference::NOTES_FILTERS[:only_comments])
+
+        notes = finder.execute
+
+        expect(notes).to match_array(comment)
+      end
+
+      it 'gets all notes' do
+        finder = described_class.new(project, user, notes_filter: UserPreference::NOTES_FILTERS[:all_activity])
+
+        notes = finder.execute
+
+        expect(notes).to match_array([comment, system_note])
+      end
+    end
+
     it 'finds notes on merge requests' do
       create(:note_on_merge_request, project: project)
 

spec/javascripts/notes/components/discussion_filter_spec.js

+import Vue from 'vue';
+import createStore from '~/notes/stores';
+import DiscussionFilter from '~/notes/components/discussion_filter.vue';
+import { mountComponentWithStore } from '../../helpers/vue_mount_component_helper';
+import { discussionFiltersMock, discussionMock } from '../mock_data';
+
+describe('DiscussionFilter component', () => {
+  let vm;
+  let store;
+
+  beforeEach(() => {
+    store = createStore();
+
+    const discussions = [{
+      ...discussionMock,
+      id: discussionMock.id,
+      notes: [{ ...discussionMock.notes[0], resolvable: true, resolved: true }],
+    }];
+    const Component = Vue.extend(DiscussionFilter);
+    const defaultValue = discussionFiltersMock[0].value;
+
+    store.state.discussions = discussions;
+    vm = mountComponentWithStore(Component, {
+      el: null,
+      store,
+      props: {
+        filters: discussionFiltersMock,
+        defaultValue,
+      },
+    });
+  });
+
+  afterEach(() => {
+    vm.$destroy();
+  });
+
+  it('renders the all filters', () => {
+    expect(vm.$el.querySelectorAll('.dropdown-menu li').length).toEqual(discussionFiltersMock.length);
+  });
+
+  it('renders the default selected item', () => {
+    expect(vm.$el.querySelector('#discussion-filter-dropdown').textContent.trim()).toEqual(discussionFiltersMock[0].title);
+  });
+
+  it('updates to the selected item', () => {
+    const filterItem = vm.$el.querySelector('.dropdown-menu li:last-child button');
+    filterItem.click();
+
+    expect(vm.currentFilter.title).toEqual(filterItem.textContent.trim());
+  });
+
+  it('only updates when selected filter changes', () => {
+    const filterItem = vm.$el.querySelector('.dropdown-menu li:first-child button');
+
+    spyOn(vm, 'filterDiscussion');
+    filterItem.click();
+
+    expect(vm.filterDiscussion).not.toHaveBeenCalled();
+  });
+});

spec/javascripts/notes/components/note_app_spec.js

@@ -97,8 +97,7 @@ describe('note_app', () => {
     });
 
     it('should render list of notes', done => {
-      const note =
-        mockData.INDIVIDUAL_NOTE_RESPONSE_MAP.GET[
+      const note = mockData.INDIVIDUAL_NOTE_RESPONSE_MAP.GET[
           '/gitlab-org/gitlab-ce/issues/26/discussions.json'
         ][0].notes[0];
 

spec/javascripts/notes/mock_data.js

@@ -1244,3 +1244,18 @@ export const discussion3 = {
 export const unresolvableDiscussion = {
   resolvable: false,
 };
+
+export const discussionFiltersMock = [
+  {
+    title: 'Show all activity',
+    value: 0,
+  },
+  {
+    title: 'Show comments only',
+    value: 1,
+  },
+  {
+    title: 'Show system notes only',
+    value: 2,
+  },
+];

spec/models/note_spec.rb

@@ -865,5 +865,29 @@ describe Note do
         note.save!
       end
     end
+
+    describe '#with_notes_filter' do
+      let!(:comment) { create(:note) }
+      let!(:system_note) { create(:note, system: true) }
+
+      context 'when notes filter is nil' do
+        subject { described_class.with_notes_filter(nil) }
+
+        it { is_expected.to include(comment, system_note) }
+      end
+
+      context 'when notes filter is set to all notes' do
+        subject { described_class.with_notes_filter(UserPreference::NOTES_FILTERS[:all_notes]) }
+
+        it { is_expected.to include(comment, system_note) }
+      end
+
+      context 'when notes filter is set to only comments' do
+        subject { described_class.with_notes_filter(UserPreference::NOTES_FILTERS[:only_comments]) }
+
+        it { is_expected.to include(comment) }
+        it { is_expected.not_to include(system_note) }
+      end
+    end
   end
 end

spec/models/user_preference_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe UserPreference do
+  describe '#set_notes_filter' do
+    let(:issuable) { build_stubbed(:issue) }
+    let(:user_preference) { create(:user_preference) }
+    let(:only_comments) { described_class::NOTES_FILTERS[:only_comments] }
+
+    it 'returns updated discussion filter' do
+      filter_name =
+        user_preference.set_notes_filter(only_comments, issuable)
+
+      expect(filter_name).to eq(only_comments)
+    end
+
+    it 'updates discussion filter for issuable class' do
+      user_preference.set_notes_filter(only_comments, issuable)
+
+      expect(user_preference.reload.issue_notes_filter).to eq(only_comments)
+    end
+
+    context 'when notes_filter parameter is invalid' do
+      it 'returns the current notes filter' do
+        user_preference.set_notes_filter(only_comments, issuable)
+
+        expect(user_preference.set_notes_filter(9999, issuable)).to eq(only_comments)
+      end
+    end
+  end
+end

spec/models/user_spec.rb

@@ -743,6 +743,15 @@ describe User do
     end
   end
 
+  describe 'ensure user preference' do
+    it 'has user preference upon user initialization' do
+      user = build(:user)
+
+      expect(user.user_preference).to be_present
+      expect(user.user_preference).not_to be_persisted
+    end
+  end
+
   describe 'ensure incoming email token' do
     it 'has incoming email token' do
       user = create(:user)

spec/support/shared_examples/controllers/issuable_notes_filter_shared_examples.rb

+shared_examples 'issuable notes filter' do
+  it 'sets discussion filter' do
+    notes_filter = UserPreference::NOTES_FILTERS[:only_comments]
+
+    get :discussions, namespace_id: project.namespace, project_id: project, id: issuable.iid, notes_filter: notes_filter
+
+    expect(user.reload.notes_filter_for(issuable)).to eq(notes_filter)
+    expect(UserPreference.count).to eq(1)
+  end
+
+  it 'expires notes e-tag cache for issuable if filter changed' do
+    notes_filter = UserPreference::NOTES_FILTERS[:only_comments]
+
+    expect_any_instance_of(issuable.class).to receive(:expire_note_etag_cache)
+
+    get :discussions, namespace_id: project.namespace, project_id: project, id: issuable.iid, notes_filter: notes_filter
+  end
+
+  it 'does not expires notes e-tag cache for issuable if filter did not change' do
+    notes_filter = UserPreference::NOTES_FILTERS[:only_comments]
+    user.set_notes_filter(notes_filter, issuable)
+
+    expect_any_instance_of(issuable.class).not_to receive(:expire_note_etag_cache)
+
+    get :discussions, namespace_id: project.namespace, project_id: project, id: issuable.iid, notes_filter: notes_filter
+  end
+
+  it 'does not set notes filter when database is in read only mode' do
+    allow(Gitlab::Database).to receive(:read_only?).and_return(true)
+    notes_filter = UserPreference::NOTES_FILTERS[:only_comments]
+
+    get :discussions, namespace_id: project.namespace, project_id: project, id: issuable.iid, notes_filter: notes_filter
+
+    expect(user.reload.notes_filter_for(issuable)).to eq(0)
+  end
+
+  it 'returns no system note' do
+    user.set_notes_filter(UserPreference::NOTES_FILTERS[:only_comments], issuable)
+
+    get :discussions, namespace_id: project.namespace, project_id: project, id: issuable.iid
+
+    expect(JSON.parse(response.body).count).to eq(1)
+  end
+
+  context 'when filter is set to "only_comments"' do
+    it 'does not merge label event notes' do
+      user.set_notes_filter(UserPreference::NOTES_FILTERS[:only_comments], issuable)
+
+      expect(ResourceEvents::MergeIntoNotesService).not_to receive(:new)
+
+      get :discussions, namespace_id: project.namespace, project_id: project, id: issuable.iid
+    end
+  end
+end