diff options
45 files changed, 694 insertions, 31 deletions
diff --git a/app/assets/javascripts/mr_notes/index.js b/app/assets/javascripts/mr_notes/index.js index 8aabb840847..1c98683c597 100644 --- a/app/assets/javascripts/mr_notes/index.js +++ b/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); } diff --git a/app/assets/javascripts/notes/components/discussion_counter.vue b/app/assets/javascripts/notes/components/discussion_counter.vue index ad6e7cf501d..1f80f24e045 100644 --- a/app/assets/javascripts/notes/components/discussion_counter.vue +++ b/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 diff --git a/app/assets/javascripts/notes/components/discussion_filter.vue b/app/assets/javascripts/notes/components/discussion_filter.vue new file mode 100644 index 00000000000..27972682ca1 --- /dev/null +++ b/app/assets/javascripts/notes/components/discussion_filter.vue @@ -0,0 +1,82 @@ +<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> diff --git a/app/assets/javascripts/notes/components/notes_app.vue b/app/assets/javascripts/notes/components/notes_app.vue index 618a1581d8f..b0faa443a18 100644 --- a/app/assets/javascripts/notes/components/notes_app.vue +++ b/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.'); }); diff --git a/app/assets/javascripts/notes/discussion_filters.js b/app/assets/javascripts/notes/discussion_filters.js new file mode 100644 index 00000000000..012ffc4093e --- /dev/null +++ b/app/assets/javascripts/notes/discussion_filters.js @@ -0,0 +1,33 @@ +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; +}; diff --git a/app/assets/javascripts/notes/index.js b/app/assets/javascripts/notes/index.js index 3aef30c608c..2f715c85fa6 100644 --- a/app/assets/javascripts/notes/index.js +++ b/app/assets/javascripts/notes/index.js @@ -1,10 +1,13 @@ 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: { diff --git a/app/assets/javascripts/notes/services/notes_service.js b/app/assets/javascripts/notes/services/notes_service.js index f5dce94caad..47a6f07cce2 100644 --- a/app/assets/javascripts/notes/services/notes_service.js +++ b/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); diff --git a/app/assets/javascripts/notes/stores/actions.js b/app/assets/javascripts/notes/stores/actions.js index 7ab7e5a9abb..b5dd49bc6c9 100644 --- a/app/assets/javascripts/notes/stores/actions.js +++ b/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 () => {}; diff --git a/app/assets/javascripts/notes/stores/getters.js b/app/assets/javascripts/notes/stores/getters.js index a829149a17e..21c334a9d33 100644 --- a/app/assets/javascripts/notes/stores/getters.js +++ b/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; diff --git a/app/assets/javascripts/notes/stores/modules/index.js b/app/assets/javascripts/notes/stores/modules/index.js index 61dbb075586..400142668ea 100644 --- a/app/assets/javascripts/notes/stores/modules/index.js +++ b/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: { diff --git a/app/assets/javascripts/notes/stores/mutation_types.js b/app/assets/javascripts/notes/stores/mutation_types.js index 6f374f78691..2fa53aef1d4 100644 --- a/app/assets/javascripts/notes/stores/mutation_types.js +++ b/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'; diff --git a/app/assets/javascripts/notes/stores/mutations.js b/app/assets/javascripts/notes/stores/mutations.js index 73e55705f39..65085452139 100644 --- a/app/assets/javascripts/notes/stores/mutations.js +++ b/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); diff --git a/app/assets/stylesheets/pages/issues.scss b/app/assets/stylesheets/pages/issues.scss index 0f95fb911e1..8ea34f5d19d 100644 --- a/app/assets/stylesheets/pages/issues.scss +++ b/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; + } +} diff --git a/app/assets/stylesheets/pages/merge_requests.scss b/app/assets/stylesheets/pages/merge_requests.scss index 2feb7464ecb..fa6afbf81de 100644 --- a/app/assets/stylesheets/pages/merge_requests.scss +++ b/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) { diff --git a/app/assets/stylesheets/pages/notes.scss b/app/assets/stylesheets/pages/notes.scss index bfba1bf1b2b..be535ade0a6 100644 --- a/app/assets/stylesheets/pages/notes.scss +++ b/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; + } + } +} diff --git a/app/controllers/concerns/issuable_actions.rb b/app/controllers/concerns/issuable_actions.rb index 07e01e903ea..ad9cc0925b7 100644 --- a/app/controllers/concerns/issuable_actions.rb +++ b/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 diff --git a/app/controllers/concerns/notes_actions.rb b/app/controllers/concerns/notes_actions.rb index 3a45d6205ab..777b147e2dd 100644 --- a/app/controllers/concerns/notes_actions.rb +++ b/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 diff --git a/app/controllers/projects/notes_controller.rb b/app/controllers/projects/notes_controller.rb index 4bac763d000..3152a38fd8e 100644 --- a/app/controllers/projects/notes_controller.rb +++ b/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! diff --git a/app/finders/notes_finder.rb b/app/finders/notes_finder.rb index c67c2065440..817aac8b5d5 100644 --- a/app/finders/notes_finder.rb +++ b/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 diff --git a/app/models/note.rb b/app/models/note.rb index 95e1d3afa00..e1bd943e8e4 100644 --- a/app/models/note.rb +++ b/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]) } diff --git a/app/models/user.rb b/app/models/user.rb index 34efb22b359..ca7fc3b058f 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -152,6 +152,7 @@ class User < ActiveRecord::Base belongs_to :accepted_term, class_name: 'ApplicationSetting::Term' has_one :status, class_name: 'UserStatus' + has_one :user_preference # # Validations @@ -224,6 +225,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 state_machine :state, initial: :active do event :block do @@ -1367,6 +1370,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 diff --git a/app/models/user_preference.rb b/app/models/user_preference.rb new file mode 100644 index 00000000000..6cd91abc261 --- /dev/null +++ b/app/models/user_preference.rb @@ -0,0 +1,52 @@ +# 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 diff --git a/app/serializers/current_user_entity.rb b/app/serializers/current_user_entity.rb new file mode 100644 index 00000000000..71d14e727dd --- /dev/null +++ b/app/serializers/current_user_entity.rb @@ -0,0 +1,8 @@ +# 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 diff --git a/app/serializers/merge_request_user_entity.rb b/app/serializers/merge_request_user_entity.rb index fd2d2897113..53257b0602c 100644 --- a/app/serializers/merge_request_user_entity.rb +++ b/app/serializers/merge_request_user_entity.rb @@ -1,6 +1,6 @@ # frozen_string_literal: true -class MergeRequestUserEntity < UserEntity +class MergeRequestUserEntity < CurrentUserEntity include RequestAwareEntity include BlobHelper include TreeHelper diff --git a/app/serializers/user_preference_entity.rb b/app/serializers/user_preference_entity.rb new file mode 100644 index 00000000000..fbdaab459b3 --- /dev/null +++ b/app/serializers/user_preference_entity.rb @@ -0,0 +1,10 @@ +# 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 diff --git a/app/views/projects/issues/_discussion.html.haml b/app/views/projects/issues/_discussion.html.haml index 28998acdc13..4917f4b8903 100644 --- a/app/views/projects/issues/_discussion.html.haml +++ b/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 } } diff --git a/app/views/projects/issues/_new_branch.html.haml b/app/views/projects/issues/_new_branch.html.haml index a678cb6f058..5374f4a1de0 100644 --- a/app/views/projects/issues/_new_branch.html.haml +++ b/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 diff --git a/app/views/projects/issues/show.html.haml b/app/views/projects/issues/show.html.haml index c39fd0063be..b50b3ca207b 100644 --- a/app/views/projects/issues/show.html.haml +++ b/app/views/projects/issues/show.html.haml @@ -77,11 +77,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 diff --git a/app/views/projects/merge_requests/show.html.haml b/app/views/projects/merge_requests/show.html.haml index ef2fa8668c0..efc2d88172e 100644 --- a/app/views/projects/merge_requests/show.html.haml +++ b/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 diff --git a/changelogs/unreleased/26723-discussion-filters.yml b/changelogs/unreleased/26723-discussion-filters.yml new file mode 100644 index 00000000000..3abe95bf30d --- /dev/null +++ b/changelogs/unreleased/26723-discussion-filters.yml @@ -0,0 +1,5 @@ +--- +title: Filter notes by comments or activity for issues and merge requests +merge_request: +author: +type: added diff --git a/db/migrate/20180925200829_create_user_preferences.rb b/db/migrate/20180925200829_create_user_preferences.rb new file mode 100644 index 00000000000..755cabdabde --- /dev/null +++ b/db/migrate/20180925200829_create_user_preferences.rb @@ -0,0 +1,31 @@ +# 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 diff --git a/db/schema.rb b/db/schema.rb index 50989960aa9..ddfccbba678 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -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", ["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 @@ -2460,6 +2470,7 @@ ActiveRecord::Schema.define(version: 20181013005024) do 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", "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_synced_attributes_metadata", "users", on_delete: :cascade add_foreign_key "users", "application_setting_terms", column: "accepted_term_id", name: "fk_789cd90b35", on_delete: :cascade diff --git a/locale/gitlab.pot b/locale/gitlab.pot index bb18d4eccd8..d5e8123bf6b 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -4132,6 +4132,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 "" @@ -5589,6 +5595,9 @@ msgstr "" msgid "Something went wrong while closing the %{issuable}. Please try again later" msgstr "" +msgid "Something went wrong while fetching comments. Please try again." +msgstr "" + msgid "Something went wrong while fetching the projects." msgstr "" diff --git a/spec/controllers/projects/issues_controller_spec.rb b/spec/controllers/projects/issues_controller_spec.rb index 9df77560320..80138183c07 100644 --- a/spec/controllers/projects/issues_controller_spec.rb +++ b/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)}" } diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb index 78581dc37a4..dcfd6c05200 100644 --- a/spec/controllers/projects/merge_requests_controller_spec.rb +++ b/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 diff --git a/spec/controllers/projects/notes_controller_spec.rb b/spec/controllers/projects/notes_controller_spec.rb index e48c9dea976..9ac7b8ee8a8 100644 --- a/spec/controllers/projects/notes_controller_spec.rb +++ b/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) } diff --git a/spec/factories/user_preferences.rb b/spec/factories/user_preferences.rb new file mode 100644 index 00000000000..19059a93625 --- /dev/null +++ b/spec/factories/user_preferences.rb @@ -0,0 +1,12 @@ +# 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 diff --git a/spec/finders/notes_finder_spec.rb b/spec/finders/notes_finder_spec.rb index b776e9d856a..de9974c45e1 100644 --- a/spec/finders/notes_finder_spec.rb +++ b/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) diff --git a/spec/javascripts/notes/components/discussion_filter_spec.js b/spec/javascripts/notes/components/discussion_filter_spec.js new file mode 100644 index 00000000000..70dd5bb3be5 --- /dev/null +++ b/spec/javascripts/notes/components/discussion_filter_spec.js @@ -0,0 +1,60 @@ +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(); + }); +}); diff --git a/spec/javascripts/notes/components/note_app_spec.js b/spec/javascripts/notes/components/note_app_spec.js index 3e289a6b8e6..06b30375306 100644 --- a/spec/javascripts/notes/components/note_app_spec.js +++ b/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]; diff --git a/spec/javascripts/notes/mock_data.js b/spec/javascripts/notes/mock_data.js index 9a0e7f34a9c..ad0e793b915 100644 --- a/spec/javascripts/notes/mock_data.js +++ b/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, + }, +]; diff --git a/spec/models/note_spec.rb b/spec/models/note_spec.rb index 1783dd3206b..f9be61e4768 100644 --- a/spec/models/note_spec.rb +++ b/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 diff --git a/spec/models/user_preference_spec.rb b/spec/models/user_preference_spec.rb new file mode 100644 index 00000000000..64d9d9a78b4 --- /dev/null +++ b/spec/models/user_preference_spec.rb @@ -0,0 +1,32 @@ +# 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 diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 99d17f563d9..b3474e74aa4 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -715,6 +715,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) diff --git a/spec/support/shared_examples/controllers/issuable_notes_filter_shared_examples.rb b/spec/support/shared_examples/controllers/issuable_notes_filter_shared_examples.rb new file mode 100644 index 00000000000..9c9d7ad781e --- /dev/null +++ b/spec/support/shared_examples/controllers/issuable_notes_filter_shared_examples.rb @@ -0,0 +1,54 @@ +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 |