Commit c1d821ed authored by Phil Hughes's avatar Phil Hughes

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

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

Closes #26723

See merge request gitlab-org/gitlab-ce!19193
parents 10bb8297 86ead874
...@@ -4,6 +4,7 @@ import { mapActions, mapState, mapGetters } from 'vuex'; ...@@ -4,6 +4,7 @@ import { mapActions, mapState, mapGetters } from 'vuex';
import initDiffsApp from '../diffs'; import initDiffsApp from '../diffs';
import notesApp from '../notes/components/notes_app.vue'; import notesApp from '../notes/components/notes_app.vue';
import discussionCounter from '../notes/components/discussion_counter.vue'; import discussionCounter from '../notes/components/discussion_counter.vue';
import initDiscussionFilters from '../notes/discussion_filters';
import store from './stores'; import store from './stores';
import MergeRequest from '../merge_request'; import MergeRequest from '../merge_request';
...@@ -88,5 +89,6 @@ export default function initMrNotes() { ...@@ -88,5 +89,6 @@ export default function initMrNotes() {
}, },
}); });
initDiscussionFilters(store);
initDiffsApp(store); initDiffsApp(store);
} }
...@@ -56,10 +56,11 @@ export default { ...@@ -56,10 +56,11 @@ export default {
</script> </script>
<template> <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>
<div <div
v-if="discussionCount > 0"
:class="{ 'has-next-btn': hasNextButton }" :class="{ 'has-next-btn': hasNextButton }"
class="line-resolve-all"> class="line-resolve-all">
<span <span
......
<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>
...@@ -50,11 +50,11 @@ export default { ...@@ -50,11 +50,11 @@ export default {
}, },
data() { data() {
return { return {
isLoading: true, currentFilter: null,
}; };
}, },
computed: { computed: {
...mapGetters(['isNotesFetched', 'discussions', 'getNotesDataByProp', 'discussionCount']), ...mapGetters(['isNotesFetched', 'discussions', 'getNotesDataByProp', 'discussionCount', 'isLoading']),
noteableType() { noteableType() {
return this.noteableData.noteableType; return this.noteableData.noteableType;
}, },
...@@ -102,6 +102,7 @@ export default { ...@@ -102,6 +102,7 @@ export default {
}, },
methods: { methods: {
...mapActions({ ...mapActions({
setLoadingState: 'setLoadingState',
fetchDiscussions: 'fetchDiscussions', fetchDiscussions: 'fetchDiscussions',
poll: 'poll', poll: 'poll',
actionToggleAward: 'toggleAward', actionToggleAward: 'toggleAward',
...@@ -133,19 +134,19 @@ export default { ...@@ -133,19 +134,19 @@ export default {
return discussion.individual_note ? { note: discussion.notes[0] } : { discussion }; return discussion.individual_note ? { note: discussion.notes[0] } : { discussion };
}, },
fetchNotes() { fetchNotes() {
return this.fetchDiscussions(this.getNotesDataByProp('discussionsPath')) return this.fetchDiscussions({ path: this.getNotesDataByProp('discussionsPath') })
.then(() => { .then(() => {
this.initPolling(); this.initPolling();
}) })
.then(() => { .then(() => {
this.isLoading = false; this.setLoadingState(false);
this.setNotesFetchedState(true); this.setNotesFetchedState(true);
eventHub.$emit('fetchedNotesData'); eventHub.$emit('fetchedNotesData');
}) })
.then(() => this.$nextTick()) .then(() => this.$nextTick())
.then(() => this.checkLocationHash()) .then(() => this.checkLocationHash())
.catch(() => { .catch(() => {
this.isLoading = false; this.setLoadingState(false);
this.setNotesFetchedState(true); this.setNotesFetchedState(true);
Flash('Something went wrong while fetching comments. Please try again.'); Flash('Something went wrong while fetching comments. Please try again.');
}); });
......
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;
};
import Vue from 'vue'; import Vue from 'vue';
import notesApp from './components/notes_app.vue'; import notesApp from './components/notes_app.vue';
import initDiscussionFilters from './discussion_filters';
import createStore from './stores'; import createStore from './stores';
document.addEventListener('DOMContentLoaded', () => { document.addEventListener('DOMContentLoaded', () => {
const store = createStore(); const store = createStore();
initDiscussionFilters(store);
return new Vue({ return new Vue({
el: '#js-vue-notes', el: '#js-vue-notes',
components: { components: {
......
...@@ -5,8 +5,9 @@ import * as constants from '../constants'; ...@@ -5,8 +5,9 @@ import * as constants from '../constants';
Vue.use(VueResource); Vue.use(VueResource);
export default { export default {
fetchDiscussions(endpoint) { fetchDiscussions(endpoint, filter) {
return Vue.http.get(endpoint); const config = filter !== undefined ? { params: { notes_filter: filter } } : null;
return Vue.http.get(endpoint, config);
}, },
deleteNote(endpoint) { deleteNote(endpoint) {
return Vue.http.delete(endpoint); return Vue.http.delete(endpoint);
......
...@@ -11,6 +11,7 @@ import loadAwardsHandler from '../../awards_handler'; ...@@ -11,6 +11,7 @@ import loadAwardsHandler from '../../awards_handler';
import sidebarTimeTrackingEventHub from '../../sidebar/event_hub'; import sidebarTimeTrackingEventHub from '../../sidebar/event_hub';
import { isInViewport, scrollToElement } from '../../lib/utils/common_utils'; import { isInViewport, scrollToElement } from '../../lib/utils/common_utils';
import mrWidgetEventHub from '../../vue_merge_request_widget/event_hub'; import mrWidgetEventHub from '../../vue_merge_request_widget/event_hub';
import { __ } from '~/locale';
let eTagPoll; let eTagPoll;
...@@ -36,9 +37,9 @@ export const setNotesFetchedState = ({ commit }, state) => ...@@ -36,9 +37,9 @@ export const setNotesFetchedState = ({ commit }, state) =>
export const toggleDiscussion = ({ commit }, data) => commit(types.TOGGLE_DISCUSSION, data); export const toggleDiscussion = ({ commit }, data) => commit(types.TOGGLE_DISCUSSION, data);
export const fetchDiscussions = ({ commit }, path) => export const fetchDiscussions = ({ commit }, { path, filter }) =>
service service
.fetchDiscussions(path) .fetchDiscussions(path, filter)
.then(res => res.json()) .then(res => res.json())
.then(discussions => { .then(discussions => {
commit(types.SET_INITIAL_DISCUSSIONS, discussions); commit(types.SET_INITIAL_DISCUSSIONS, discussions);
...@@ -251,7 +252,7 @@ const pollSuccessCallBack = (resp, commit, state, getters, dispatch) => { ...@@ -251,7 +252,7 @@ const pollSuccessCallBack = (resp, commit, state, getters, dispatch) => {
if (discussion) { if (discussion) {
commit(types.ADD_NEW_REPLY_TO_DISCUSSION, note); commit(types.ADD_NEW_REPLY_TO_DISCUSSION, note);
} else if (note.type === constants.DIFF_NOTE) { } else if (note.type === constants.DIFF_NOTE) {
dispatch('fetchDiscussions', state.notesData.discussionsPath); dispatch('fetchDiscussions', { path: state.notesData.discussionsPath });
} else { } else {
commit(types.ADD_NEW_NOTE, note); commit(types.ADD_NEW_NOTE, note);
} }
...@@ -345,5 +346,23 @@ export const updateMergeRequestWidget = () => { ...@@ -345,5 +346,23 @@ export const updateMergeRequestWidget = () => {
mrWidgetEventHub.$emit('mr.discussion.updated'); 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 // prevent babel-plugin-rewire from generating an invalid default during karma tests
export default () => {}; export default () => {};
...@@ -11,6 +11,8 @@ export const getNotesData = state => state.notesData; ...@@ -11,6 +11,8 @@ export const getNotesData = state => state.notesData;
export const isNotesFetched = state => state.isNotesFetched; export const isNotesFetched = state => state.isNotesFetched;
export const isLoading = state => state.isLoading;
export const getNotesDataByProp = state => prop => state.notesData[prop]; export const getNotesDataByProp = state => prop => state.notesData[prop];
export const getNoteableData = state => state.noteableData; export const getNoteableData = state => state.noteableData;
......
...@@ -11,6 +11,7 @@ export default () => ({ ...@@ -11,6 +11,7 @@ export default () => ({
// View layer // View layer
isToggleStateButtonLoading: false, isToggleStateButtonLoading: false,
isNotesFetched: false, isNotesFetched: false,
isLoading: true,
// holds endpoints and permissions provided through haml // holds endpoints and permissions provided through haml
notesData: { notesData: {
......
...@@ -14,6 +14,7 @@ export const UPDATE_NOTE = 'UPDATE_NOTE'; ...@@ -14,6 +14,7 @@ export const UPDATE_NOTE = 'UPDATE_NOTE';
export const UPDATE_DISCUSSION = 'UPDATE_DISCUSSION'; export const UPDATE_DISCUSSION = 'UPDATE_DISCUSSION';
export const SET_DISCUSSION_DIFF_LINES = 'SET_DISCUSSION_DIFF_LINES'; export const SET_DISCUSSION_DIFF_LINES = 'SET_DISCUSSION_DIFF_LINES';
export const SET_NOTES_FETCHED_STATE = 'SET_NOTES_FETCHED_STATE'; export const SET_NOTES_FETCHED_STATE = 'SET_NOTES_FETCHED_STATE';
export const SET_NOTES_LOADING_STATE = 'SET_NOTES_LOADING_STATE';
// DISCUSSION // DISCUSSION
export const COLLAPSE_DISCUSSION = 'COLLAPSE_DISCUSSION'; export const COLLAPSE_DISCUSSION = 'COLLAPSE_DISCUSSION';
......
...@@ -216,6 +216,10 @@ export default { ...@@ -216,6 +216,10 @@ export default {
Object.assign(state, { isNotesFetched: value }); Object.assign(state, { isNotesFetched: value });
}, },
[types.SET_NOTES_LOADING_STATE](state, value) {
state.isLoading = value;
},
[types.SET_DISCUSSION_DIFF_LINES](state, { discussionId, diffLines }) { [types.SET_DISCUSSION_DIFF_LINES](state, { discussionId, diffLines }) {
const discussion = utils.findNoteObjectById(state.discussions, discussionId); const discussion = utils.findNoteObjectById(state.discussions, discussionId);
......
...@@ -185,7 +185,17 @@ ul.related-merge-requests > li { ...@@ -185,7 +185,17 @@ ul.related-merge-requests > li {
} }
.new-branch-col { .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 { .create-mr-dropdown-wrap {
...@@ -205,6 +215,10 @@ ul.related-merge-requests > li { ...@@ -205,6 +215,10 @@ ul.related-merge-requests > li {
.btn-group:not(.hidden) { .btn-group:not(.hidden) {
display: flex; display: flex;
@include media-breakpoint-down(md) {
margin-top: $gl-padding-8;
}
} }
.js-create-merge-request { .js-create-merge-request {
...@@ -251,7 +265,6 @@ ul.related-merge-requests > li { ...@@ -251,7 +265,6 @@ ul.related-merge-requests > li {
.new-branch-col { .new-branch-col {
padding-top: 0; padding-top: 0;
text-align: right;
align-self: center; align-self: center;
} }
...@@ -262,3 +275,9 @@ ul.related-merge-requests > li { ...@@ -262,3 +275,9 @@ ul.related-merge-requests > li {
} }
} }
} }
@include media-breakpoint-up(lg) {
.new-branch-col {
text-align: right;
}
}
...@@ -818,9 +818,17 @@ ...@@ -818,9 +818,17 @@
display: flex; display: flex;
justify-content: space-between; justify-content: space-between;
@include media-breakpoint-down(xs) { @include media-breakpoint-down(md) {
flex-direction: column-reverse; 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) { .limit-container-width:not(.container-limited) {
......
...@@ -618,7 +618,6 @@ ul.notes { ...@@ -618,7 +618,6 @@ ul.notes {
.line-resolve-all-container { .line-resolve-all-container {
@include notes-media('min', map-get($grid-breakpoints, sm)) { @include notes-media('min', map-get($grid-breakpoints, sm)) {
margin-right: 0; margin-right: 0;
padding-left: $gl-padding;
} }
> div { > div {
...@@ -756,3 +755,23 @@ ul.notes { ...@@ -756,3 +755,23 @@ ul.notes {
margin-top: 4px; 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;
}
}
}
...@@ -2,6 +2,7 @@ ...@@ -2,6 +2,7 @@
module IssuableActions module IssuableActions
extend ActiveSupport::Concern extend ActiveSupport::Concern
include Gitlab::Utils::StrongMemoize
included do included do
before_action :labels, only: [:show, :new, :edit] before_action :labels, only: [:show, :new, :edit]
...@@ -95,10 +96,14 @@ module IssuableActions ...@@ -95,10 +96,14 @@ module IssuableActions
def discussions def discussions
notes = issuable.discussion_notes notes = issuable.discussion_notes
.inc_relations_for_view .inc_relations_for_view
.with_notes_filter(notes_filter)
.includes(:noteable) .includes(:noteable)
.fresh .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 = prepare_notes_for_rendering(notes)
notes = notes.reject { |n| n.cross_reference_not_visible_for?(current_user) } notes = notes.reject { |n| n.cross_reference_not_visible_for?(current_user) }
...@@ -110,6 +115,32 @@ module IssuableActions ...@@ -110,6 +115,32 @@ module IssuableActions
private 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 def discussion_serializer
DiscussionSerializer.new(project: project, noteable: issuable, current_user: current_user, note_entity: ProjectNoteEntity) DiscussionSerializer.new(project: project, noteable: issuable, current_user: current_user, note_entity: ProjectNoteEntity)
end end
......
...@@ -17,10 +17,17 @@ module NotesActions ...@@ -17,10 +17,17 @@ module NotesActions
notes_json = { notes: [], last_fetched_at: current_fetched_at } notes_json = { notes: [], last_fetched_at: current_fetched_at }
notes = notes_finder.execute notes = notes_finder
.inc_relations_for_view .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 = prepare_notes_for_rendering(notes)
notes = notes.reject { |n| n.cross_reference_not_visible_for?(current_user) } notes = notes.reject { |n| n.cross_reference_not_visible_for?(current_user) }
...@@ -224,6 +231,10 @@ module NotesActions ...@@ -224,6 +231,10 @@ module NotesActions
request.headers['X-Last-Fetched-At'] request.headers['X-Last-Fetched-At']
end end
def notes_filter
current_user&.notes_filter_for(params[:target_type])
end
def notes_finder def notes_finder
@notes_finder ||= NotesFinder.new(project, current_user, finder_params) @notes_finder ||= NotesFinder.new(project, current_user, finder_params)
end end
......
...@@ -68,7 +68,7 @@ class Projects::NotesController < Projects::ApplicationController ...@@ -68,7 +68,7 @@ class Projects::NotesController < Projects::ApplicationController
alias_method :awardable, :note alias_method :awardable, :note
def finder_params def finder_params
params.merge(last_fetched_at: last_fetched_at) params.merge(last_fetched_at: last_fetched_at, notes_filter: notes_filter)
end end
def authorize_admin_note! def authorize_admin_note!
......
...@@ -24,6 +24,8 @@ class NotesFinder ...@@ -24,6 +24,8 @@ class NotesFinder
def execute def execute
notes = init_collection notes = init_collection
notes = since_fetch_at(notes) notes = since_fetch_at(notes)
notes = notes.with_notes_filter(@params[:notes_filter]) if notes_filter?
notes.fresh notes.fresh
end end
...@@ -134,4 +136,8 @@ class NotesFinder ...@@ -134,4 +136,8 @@ class NotesFinder
last_fetched_at = Time.at(@params.fetch(:last_fetched_at, 0).to_i) last_fetched_at = Time.at(@params.fetch(:last_fetched_at, 0).to_i)
notes.updated_after(last_fetched_at - FETCH_OVERLAP) notes.updated_after(last_fetched_at - FETCH_OVERLAP)
end end
def notes_filter?
@params[:notes_filter].present?
end
end end
...@@ -110,6 +110,15 @@ class Note < ActiveRecord::Base ...@@ -110,6 +110,15 @@ class Note < ActiveRecord::Base
:system_note_metadata, :note_diff_file) :system_note_metadata, :note_diff_file)
end 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 :diff_notes, -> { where(type: %w(LegacyDiffNote DiffNote)) }
scope :new_diff_notes, -> { where(type: 'DiffNote') } scope :new_diff_notes, -> { where(type: 'DiffNote') }
scope :non_diff_notes, -> { where(type: ['Note', 'DiscussionNote', nil]) } scope :non_diff_notes, -> { where(type: ['Note', 'DiscussionNote', nil]) }
......
...@@ -152,6 +152,7 @@ class User < ActiveRecord::Base ...@@ -152,6 +152,7 @@ class User < ActiveRecord::Base
belongs_to :accepted_term, class_name: 'ApplicationSetting::Term' belongs_to :accepted_term, class_name: 'ApplicationSetting::Term'
has_one :status, class_name: 'UserStatus' has_one :status, class_name: 'UserStatus'
has_one :user_preference
# #
# Validations # Validations
...@@ -224,6 +225,8 @@ class User < ActiveRecord::Base ...@@ -224,6 +225,8 @@ class User < ActiveRecord::Base
enum project_view: [:readme, :activity, :files] enum project_view: [:readme, :activity, :files]
delegate :path, to: :namespace, allow_nil: true, prefix: true delegate :path, to: :namespace, allow_nil: true, prefix: true
delegate :notes_filter_for, to: :user_preference
delegate :set_notes_filter, to: :user_preference
state_machine :state, initial: :active do state_machine :state, initial: :active do
event :block do event :block do
...@@ -1367,6 +1370,11 @@ class User < ActiveRecord::Base ...@@ -1367,6 +1370,11 @@ class User < ActiveRecord::Base
!consented_usage_stats? && 7.days.ago > self.created_at && !has_current_license? && User.single_user? !consented_usage_stats? && 7.days.ago > self.created_at && !has_current_license? && User.single_user?
end end
# Avoid migrations only building user preference object when needed.
def user_preference
super.presence || build_user_preference
end
def todos_limited_to(ids) def todos_limited_to(ids)
todos.where(id: ids) todos.where(id: ids)
end end
......
# 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
# 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
# frozen_string_literal: true # frozen_string_literal: true
class MergeRequestUserEntity < UserEntity class MergeRequestUserEntity < CurrentUserEntity
include RequestAwareEntity include RequestAwareEntity
include BlobHelper include BlobHelper
include TreeHelper include TreeHelper
......
# 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
...@@ -10,4 +10,4 @@ ...@@ -10,4 +10,4 @@
noteable_data: serialize_issuable(@issue), noteable_data: serialize_issuable(@issue),
noteable_type: 'Issue', noteable_type: 'Issue',
target_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 } }
...@@ -8,12 +8,13 @@ ...@@ -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) - 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: '') - 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 .btn-group.unavailable
%button.btn.btn-grouped{ type: 'button', disabled: 'disabled' } %button.btn.btn-grouped{ type: 'button', disabled: 'disabled' }
= icon('spinner', class: 'fa-spin') = icon('spinner', class: 'fa-spin')
%span.text %span.text
Checking branch availability… Checking branch availability…
.btn-group.available.hidden .btn-group.available.hidden
%button.btn.js-create-merge-request.btn-success.btn-inverted{ type: 'button', data: { action: data_action } } %button.btn.js-create-merge-request.btn-success.btn-inverted{ type: 'button', data: { action: data_action } }
= value = value
......
...@@ -77,11 +77,12 @@ ...@@ -77,11 +77,12 @@
#related-branches{ data: { url: related_branches_project_issue_path(@project, @issue) } } #related-branches{ data: { url: related_branches_project_issue_path(@project, @issue) } }
// This element is filled in using JavaScript. // This element is filled in using JavaScript.
.content-block.emoji-block .content-block.emoji-block.emoji-block-sticky
.row .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 = 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? = render 'new_branch' unless @issue.confidential?
%section.issuable-discussion %section.issuable-discussion
......
...@@ -51,8 +51,10 @@ ...@@ -51,8 +51,10 @@
= tab_link_for @merge_request, :diffs do = tab_link_for @merge_request, :diffs do
Changes Changes
%span.badge.badge-pill= @merge_request.diff_size %span.badge.badge-pill= @merge_request.diff_size
.d-inline-flex.flex-wrap
#js-vue-discussion-counter #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 .tab-content#diff-notes-app
#notes.notes.tab-pane.voting_notes #notes.notes.tab-pane.voting_notes
......
---
title: Filter notes by comments or activity for issues and merge requests
merge_request:
author:
type: added
# 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
...@@ -2134,6 +2134,16 @@ ActiveRecord::Schema.define(version: 20181013005024) do ...@@ -2134,6 +2134,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", ["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 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| create_table "user_statuses", primary_key: "user_id", force: :cascade do |t|
t.integer "cached_markdown_version" t.integer "cached_markdown_version"
t.string "emoji", default: "speech_balloon", null: false t.string "emoji", default: "speech_balloon", null: false
...@@ -2460,6 +2470,7 @@ ActiveRecord::Schema.define(version: 20181013005024) do ...@@ -2460,6 +2470,7 @@ ActiveRecord::Schema.define(version: 20181013005024) do
add_foreign_key "user_custom_attributes", "users", on_delete: :cascade add_foreign_key "user_custom_attributes", "users", on_delete: :cascade
add_foreign_key "user_interacted_projects", "projects", name: "fk_722ceba4f7", on_delete: :cascade add_foreign_key "user_interacted_projects", "projects", name: "fk_722ceba4f7", on_delete: :cascade
add_foreign_key "user_interacted_projects", "users", name: "fk_0894651f08", on_delete: :cascade add_foreign_key "user_interacted_projects", "users", name: "fk_0894651f08", 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_statuses", "users", on_delete: :cascade
add_foreign_key "user_synced_attributes_metadata", "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 add_foreign_key "users", "application_setting_terms", column: "accepted_term_id", name: "fk_789cd90b35", on_delete: :cascade
......
...@@ -4132,6 +4132,12 @@ msgstr "" ...@@ -4132,6 +4132,12 @@ msgstr ""
msgid "Notes|Are you sure you want to cancel creating this comment?" msgid "Notes|Are you sure you want to cancel creating this comment?"
msgstr "" msgstr ""
msgid "Notes|Show all activity"
msgstr ""
msgid "Notes|Show comments only"
msgstr ""
msgid "Notification events" msgid "Notification events"
msgstr "" msgstr ""
...@@ -5589,6 +5595,9 @@ msgstr "" ...@@ -5589,6 +5595,9 @@ msgstr ""
msgid "Something went wrong while closing the %{issuable}. Please try again later" msgid "Something went wrong while closing the %{issuable}. Please try again later"
msgstr "" msgstr ""
msgid "Something went wrong while fetching comments. Please try again."
msgstr ""
msgid "Something went wrong while fetching the projects." msgid "Something went wrong while fetching the projects."
msgstr "" msgstr ""
......
...@@ -1028,6 +1028,13 @@ describe Projects::IssuesController do ...@@ -1028,6 +1028,13 @@ describe Projects::IssuesController do
.not_to exceed_query_limit(control) .not_to exceed_query_limit(control)
end 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 context 'with cross-reference system note', :request_store do
let(:new_issue) { create(:issue) } let(:new_issue) { create(:issue) }
let(:cross_reference) { "mentioned in #{new_issue.to_reference(issue.project)}" } let(:cross_reference) { "mentioned in #{new_issue.to_reference(issue.project)}" }
......
...@@ -87,6 +87,14 @@ describe Projects::MergeRequestsController do ...@@ -87,6 +87,14 @@ describe Projects::MergeRequestsController do
end end
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 describe 'as json' do
context 'with basic serializer param' do context 'with basic serializer param' do
it 'renders basic MR entity as json' do it 'renders basic MR entity as json' do
......
...@@ -47,6 +47,37 @@ describe Projects::NotesController do ...@@ -47,6 +47,37 @@ describe Projects::NotesController do
get :index, request_params get :index, request_params
end 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 context 'for a discussion note' do
let(:project) { create(:project, :repository) } let(:project) { create(:project, :repository) }
let!(:note) { create(:discussion_note_on_merge_request, project: project) } let!(:note) { create(:discussion_note_on_merge_request, project: project) }
......
# 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
...@@ -9,6 +9,27 @@ describe NotesFinder do ...@@ -9,6 +9,27 @@ describe NotesFinder do
end end
describe '#execute' do 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 it 'finds notes on merge requests' do
create(:note_on_merge_request, project: project) create(:note_on_merge_request, project: project)
......
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();
});
});
...@@ -97,8 +97,7 @@ describe('note_app', () => { ...@@ -97,8 +97,7 @@ describe('note_app', () => {
}); });
it('should render list of notes', done => { it('should render list of notes', done => {
const note = const note = mockData.INDIVIDUAL_NOTE_RESPONSE_MAP.GET[
mockData.INDIVIDUAL_NOTE_RESPONSE_MAP.GET[
'/gitlab-org/gitlab-ce/issues/26/discussions.json' '/gitlab-org/gitlab-ce/issues/26/discussions.json'
][0].notes[0]; ][0].notes[0];
......
...@@ -1244,3 +1244,18 @@ export const discussion3 = { ...@@ -1244,3 +1244,18 @@ export const discussion3 = {
export const unresolvableDiscussion = { export const unresolvableDiscussion = {
resolvable: false, resolvable: false,
}; };
export const discussionFiltersMock = [
{
title: 'Show all activity',
value: 0,
},
{
title: 'Show comments only',
value: 1,
},
{
title: 'Show system notes only',
value: 2,
},
];
...@@ -865,5 +865,29 @@ describe Note do ...@@ -865,5 +865,29 @@ describe Note do
note.save! note.save!
end end
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
end end
# 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
...@@ -715,6 +715,15 @@ describe User do ...@@ -715,6 +715,15 @@ describe User do
end end
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 describe 'ensure incoming email token' do
it 'has incoming email token' do it 'has incoming email token' do
user = create(:user) user = create(:user)
......
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
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment