From 8bcc47ac02e69eb4564238b454ca8286a4126765 Mon Sep 17 00:00:00 2001 From: George Koltsov Date: Wed, 21 Aug 2019 10:13:45 +0000 Subject: Add SortingPreference concern Sorting preference functionality has been extracted from `IssuableCollections` to a new `SortingPreference` concern in order to reuse this functionality in projects (and groups in the future). --- app/controllers/concerns/issuable_collections.rb | 56 +------------ .../concerns/issuable_collections_action.rb | 2 +- app/controllers/concerns/sorting_preference.rb | 85 +++++++++++++++++++ app/controllers/dashboard/projects_controller.rb | 22 +++-- app/controllers/explore/projects_controller.rb | 19 ++++- app/controllers/projects/issues_controller.rb | 2 +- .../projects/merge_requests_controller.rb | 2 +- app/models/project.rb | 2 + ...orgekoltsov-18720-persistent-dashboard-sort.yml | 5 ++ ...d_projects_sorting_field_to_user_preferences.rb | 13 +++ db/schema.rb | 1 + .../concerns/issuable_collections_spec.rb | 72 ---------------- .../concerns/sorting_preference_spec.rb | 93 +++++++++++++++++++++ .../dashboard/projects_controller_spec.rb | 8 ++ .../explore/projects_controller_spec.rb | 95 +++++++++++++++------- ...t_order_from_user_preference_shared_examples.rb | 6 +- 16 files changed, 313 insertions(+), 170 deletions(-) create mode 100644 app/controllers/concerns/sorting_preference.rb create mode 100644 changelogs/unreleased/georgekoltsov-18720-persistent-dashboard-sort.yml create mode 100644 db/migrate/20190808152507_add_projects_sorting_field_to_user_preferences.rb create mode 100644 spec/controllers/concerns/sorting_preference_spec.rb diff --git a/app/controllers/concerns/issuable_collections.rb b/app/controllers/concerns/issuable_collections.rb index 3489ea78b77..8ea77b994de 100644 --- a/app/controllers/concerns/issuable_collections.rb +++ b/app/controllers/concerns/issuable_collections.rb @@ -2,8 +2,8 @@ module IssuableCollections extend ActiveSupport::Concern - include CookiesHelper include SortingHelper + include SortingPreference include Gitlab::IssuableMetadata include Gitlab::Utils::StrongMemoize @@ -127,47 +127,8 @@ module IssuableCollections 'opened' end - def set_sort_order - set_sort_order_from_user_preference || set_sort_order_from_cookie || default_sort_order - end - - def set_sort_order_from_user_preference - return unless current_user - return unless issuable_sorting_field - - user_preference = current_user.user_preference - - sort_param = params[:sort] - sort_param ||= user_preference[issuable_sorting_field] - - return sort_param if Gitlab::Database.read_only? - - if user_preference[issuable_sorting_field] != sort_param - user_preference.update(issuable_sorting_field => sort_param) - end - - sort_param - end - - # Implement issuable_sorting_field method on controllers - # to choose which column to store the sorting parameter. - def issuable_sorting_field - nil - end - - def set_sort_order_from_cookie - sort_param = params[:sort] if params[:sort].present? - # fallback to legacy cookie value for backward compatibility - sort_param ||= cookies['issuable_sort'] - sort_param ||= cookies[remember_sorting_key] - - sort_value = update_cookie_value(sort_param) - set_secure_cookie(remember_sorting_key, sort_value) - sort_value - end - - def remember_sorting_key - @remember_sorting_key ||= "#{collection_type.downcase}_sort" + def legacy_sort_cookie_name + 'issuable_sort' end def default_sort_order @@ -178,17 +139,6 @@ module IssuableCollections end end - # Update old values to the actual ones. - def update_cookie_value(value) - case value - when 'id_asc' then sort_value_oldest_created - when 'id_desc' then sort_value_recently_created - when 'downvotes_asc' then sort_value_popularity - when 'downvotes_desc' then sort_value_popularity - else value - end - end - def finder @finder ||= issuable_finder_for(finder_type) end diff --git a/app/controllers/concerns/issuable_collections_action.rb b/app/controllers/concerns/issuable_collections_action.rb index 4ad287c4a13..0a6f684a9fc 100644 --- a/app/controllers/concerns/issuable_collections_action.rb +++ b/app/controllers/concerns/issuable_collections_action.rb @@ -32,7 +32,7 @@ module IssuableCollectionsAction private - def issuable_sorting_field + def sorting_field case action_name when 'issues' Issue::SORTING_PREFERENCE_FIELD diff --git a/app/controllers/concerns/sorting_preference.rb b/app/controllers/concerns/sorting_preference.rb new file mode 100644 index 00000000000..a51b68147d5 --- /dev/null +++ b/app/controllers/concerns/sorting_preference.rb @@ -0,0 +1,85 @@ +# frozen_string_literal: true + +module SortingPreference + include SortingHelper + include CookiesHelper + + def set_sort_order + set_sort_order_from_user_preference || set_sort_order_from_cookie || params[:sort] || default_sort_order + end + + # Implement sorting_field method on controllers + # to choose which column to store the sorting parameter. + def sorting_field + nil + end + + # Implement default_sort_order method on controllers + # to choose which default sort should be applied if + # sort param is not provided. + def default_sort_order + nil + end + + # Implement legacy_sort_cookie_name method on controllers + # to set sort from cookie for backwards compatibility. + def legacy_sort_cookie_name + nil + end + + private + + def set_sort_order_from_user_preference + return unless current_user + return unless sorting_field + + user_preference = current_user.user_preference + + sort_param = params[:sort] + sort_param ||= user_preference[sorting_field] + + return sort_param if Gitlab::Database.read_only? + + if user_preference[sorting_field] != sort_param + user_preference.update(sorting_field => sort_param) + end + + sort_param + end + + def set_sort_order_from_cookie + return unless legacy_sort_cookie_name + + sort_param = params[:sort] if params[:sort].present? + # fallback to legacy cookie value for backward compatibility + sort_param ||= cookies[legacy_sort_cookie_name] + sort_param ||= cookies[remember_sorting_key] + + sort_value = update_cookie_value(sort_param) + set_secure_cookie(remember_sorting_key, sort_value) + sort_value + end + + # Convert sorting_field to legacy cookie name for backwards compatibility + # :merge_requests_sort => 'mergerequest_sort' + # :issues_sort => 'issue_sort' + def remember_sorting_key + @remember_sorting_key ||= sorting_field + .to_s + .split('_')[0..-2] + .map(&:singularize) + .join('') + .concat('_sort') + end + + # Update old values to the actual ones. + def update_cookie_value(value) + case value + when 'id_asc' then sort_value_oldest_created + when 'id_desc' then sort_value_recently_created + when 'downvotes_asc' then sort_value_popularity + when 'downvotes_desc' then sort_value_popularity + else value + end + end +end diff --git a/app/controllers/dashboard/projects_controller.rb b/app/controllers/dashboard/projects_controller.rb index daeb8fda417..71f18694613 100644 --- a/app/controllers/dashboard/projects_controller.rb +++ b/app/controllers/dashboard/projects_controller.rb @@ -4,10 +4,12 @@ class Dashboard::ProjectsController < Dashboard::ApplicationController include ParamsBackwardCompatibility include RendersMemberAccess include OnboardingExperimentHelper + include SortingHelper + include SortingPreference prepend_before_action(only: [:index]) { authenticate_sessionless_user!(:rss) } before_action :set_non_archived_param - before_action :default_sorting + before_action :set_sorting before_action :projects, only: [:index] skip_cross_project_access_check :index, :starred @@ -59,11 +61,6 @@ class Dashboard::ProjectsController < Dashboard::ApplicationController end end - def default_sorting - params[:sort] ||= 'latest_activity_desc' - @sort = params[:sort] - end - # rubocop: disable CodeReuse/ActiveRecord def load_projects(finder_params) @total_user_projects_count = ProjectsFinder.new(params: { non_public: true }, current_user: current_user).execute @@ -88,4 +85,17 @@ class Dashboard::ProjectsController < Dashboard::ApplicationController Events::RenderService.new(current_user).execute(@events, atom_request: request.format.atom?) end + + def set_sorting + params[:sort] = set_sort_order + @sort = params[:sort] + end + + def default_sort_order + sort_value_latest_activity + end + + def sorting_field + Project::SORTING_PREFERENCE_FIELD + end end diff --git a/app/controllers/explore/projects_controller.rb b/app/controllers/explore/projects_controller.rb index ef86d5f981a..271f2b4b57d 100644 --- a/app/controllers/explore/projects_controller.rb +++ b/app/controllers/explore/projects_controller.rb @@ -3,12 +3,13 @@ class Explore::ProjectsController < Explore::ApplicationController include ParamsBackwardCompatibility include RendersMemberAccess + include SortingHelper + include SortingPreference before_action :set_non_archived_param + before_action :set_sorting def index - params[:sort] ||= 'latest_activity_desc' - @sort = params[:sort] @projects = load_projects respond_to do |format| @@ -23,7 +24,6 @@ class Explore::ProjectsController < Explore::ApplicationController def trending params[:trending] = true - @sort = params[:sort] @projects = load_projects respond_to do |format| @@ -67,4 +67,17 @@ class Explore::ProjectsController < Explore::ApplicationController prepare_projects_for_rendering(projects) end # rubocop: enable CodeReuse/ActiveRecord + + def set_sorting + params[:sort] = set_sort_order + @sort = params[:sort] + end + + def default_sort_order + sort_value_latest_activity + end + + def sorting_field + Project::SORTING_PREFERENCE_FIELD + end end diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb index bc9166b9df3..b7fd286bfe0 100644 --- a/app/controllers/projects/issues_controller.rb +++ b/app/controllers/projects/issues_controller.rb @@ -190,7 +190,7 @@ class Projects::IssuesController < Projects::ApplicationController protected - def issuable_sorting_field + def sorting_field Issue::SORTING_PREFERENCE_FIELD end diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index f4d381244d9..f4cc0a5851b 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -219,7 +219,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo alias_method :issuable, :merge_request alias_method :awardable, :merge_request - def issuable_sorting_field + def sorting_field MergeRequest::SORTING_PREFERENCE_FIELD end diff --git a/app/models/project.rb b/app/models/project.rb index 8efe4b06f87..10679fb1f85 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -55,6 +55,8 @@ class Project < ApplicationRecord VALID_MIRROR_PORTS = [22, 80, 443].freeze VALID_MIRROR_PROTOCOLS = %w(http https ssh git).freeze + SORTING_PREFERENCE_FIELD = :projects_sort + cache_markdown_field :description, pipeline: :description delegate :feature_available?, :builds_enabled?, :wiki_enabled?, diff --git a/changelogs/unreleased/georgekoltsov-18720-persistent-dashboard-sort.yml b/changelogs/unreleased/georgekoltsov-18720-persistent-dashboard-sort.yml new file mode 100644 index 00000000000..7eed8550acd --- /dev/null +++ b/changelogs/unreleased/georgekoltsov-18720-persistent-dashboard-sort.yml @@ -0,0 +1,5 @@ +--- +title: Add persistance to last choice of projects sorting on projects dashboard page +merge_request: 31669 +author: +type: added diff --git a/db/migrate/20190808152507_add_projects_sorting_field_to_user_preferences.rb b/db/migrate/20190808152507_add_projects_sorting_field_to_user_preferences.rb new file mode 100644 index 00000000000..941fead655e --- /dev/null +++ b/db/migrate/20190808152507_add_projects_sorting_field_to_user_preferences.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true + +class AddProjectsSortingFieldToUserPreferences < ActiveRecord::Migration[5.2] + DOWNTIME = false + + def up + add_column :user_preferences, :projects_sort, :string, limit: 64 + end + + def down + remove_column :user_preferences, :projects_sort + end +end diff --git a/db/schema.rb b/db/schema.rb index ce5fd38129a..3f7917654cf 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -3411,6 +3411,7 @@ ActiveRecord::Schema.define(version: 2019_08_15_093949) do t.string "epics_sort" t.integer "roadmap_epics_state" t.string "roadmaps_sort" + t.string "projects_sort", limit: 64 t.index ["user_id"], name: "index_user_preferences_on_user_id", unique: true end diff --git a/spec/controllers/concerns/issuable_collections_spec.rb b/spec/controllers/concerns/issuable_collections_spec.rb index f210537aad5..7bdf5c49425 100644 --- a/spec/controllers/concerns/issuable_collections_spec.rb +++ b/spec/controllers/concerns/issuable_collections_spec.rb @@ -24,78 +24,6 @@ describe IssuableCollections do controller end - describe '#set_sort_order_from_user_preference' do - describe 'when sort param given' do - let(:params) { { sort: 'updated_desc' } } - - context 'when issuable_sorting_field is defined' do - before do - controller.class.define_method(:issuable_sorting_field) { :issues_sort} - end - - it 'sets user_preference with the right value' do - controller.send(:set_sort_order_from_user_preference) - - expect(user.user_preference.reload.issues_sort).to eq('updated_desc') - end - end - - context 'when no issuable_sorting_field is defined on the controller' do - it 'does not touch user_preference' do - allow(user).to receive(:user_preference) - - controller.send(:set_sort_order_from_user_preference) - - expect(user).not_to have_received(:user_preference) - end - end - end - - context 'when a user sorting preference exists' do - let(:params) { {} } - - before do - controller.class.define_method(:issuable_sorting_field) { :issues_sort } - end - - it 'returns the set preference' do - user.user_preference.update(issues_sort: 'updated_asc') - - sort_preference = controller.send(:set_sort_order_from_user_preference) - - expect(sort_preference).to eq('updated_asc') - end - end - end - - describe '#set_set_order_from_cookie' do - describe 'when sort param given' do - let(:cookies) { {} } - let(:params) { { sort: 'downvotes_asc' } } - - it 'sets the cookie with the right values and flags' do - allow(controller).to receive(:cookies).and_return(cookies) - - controller.send(:set_sort_order_from_cookie) - - expect(cookies['issue_sort']).to eq({ value: 'popularity', secure: false, httponly: false }) - end - end - - describe 'when cookie exists' do - let(:cookies) { { 'issue_sort' => 'id_asc' } } - let(:params) { {} } - - it 'sets the cookie with the right values and flags' do - allow(controller).to receive(:cookies).and_return(cookies) - - controller.send(:set_sort_order_from_cookie) - - expect(cookies['issue_sort']).to eq({ value: 'created_asc', secure: false, httponly: false }) - end - end - end - describe '#page_count_for_relation' do let(:params) { { state: 'opened' } } diff --git a/spec/controllers/concerns/sorting_preference_spec.rb b/spec/controllers/concerns/sorting_preference_spec.rb new file mode 100644 index 00000000000..a36124c6776 --- /dev/null +++ b/spec/controllers/concerns/sorting_preference_spec.rb @@ -0,0 +1,93 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe SortingPreference do + let(:user) { create(:user) } + + let(:controller_class) do + Class.new do + def self.helper_method(name); end + + include SortingPreference + include SortingHelper + end + end + + let(:controller) { controller_class.new } + + before do + allow(controller).to receive(:params).and_return(ActionController::Parameters.new(params)) + allow(controller).to receive(:current_user).and_return(user) + allow(controller).to receive(:legacy_sort_cookie_name).and_return('issuable_sort') + allow(controller).to receive(:sorting_field).and_return(:issues_sort) + end + + describe '#set_sort_order_from_user_preference' do + subject { controller.send(:set_sort_order_from_user_preference) } + + context 'when sort param given' do + let(:params) { { sort: 'updated_desc' } } + + context 'when sorting_field is defined' do + it 'sets user_preference with the right value' do + is_expected.to eq('updated_desc') + end + end + + context 'when no sorting_field is defined on the controller' do + before do + allow(controller).to receive(:sorting_field).and_return(nil) + end + + it 'does not touch user_preference' do + expect(user).not_to receive(:user_preference) + + subject + end + end + end + + context 'when a user sorting preference exists' do + let(:params) { {} } + + before do + user.user_preference.update!(issues_sort: 'updated_asc') + end + + it 'returns the set preference' do + is_expected.to eq('updated_asc') + end + end + end + + describe '#set_set_order_from_cookie' do + subject { controller.send(:set_sort_order_from_cookie) } + + before do + allow(controller).to receive(:cookies).and_return(cookies) + end + + context 'when sort param given' do + let(:cookies) { {} } + let(:params) { { sort: 'downvotes_asc' } } + + it 'sets the cookie with the right values and flags' do + subject + + expect(cookies['issue_sort']).to eq(value: 'popularity', secure: false, httponly: false) + end + end + + context 'when cookie exists' do + let(:cookies) { { 'issue_sort' => 'id_asc' } } + let(:params) { {} } + + it 'sets the cookie with the right values and flags' do + subject + + expect(cookies['issue_sort']).to eq(value: 'created_asc', secure: false, httponly: false) + end + end + end +end diff --git a/spec/controllers/dashboard/projects_controller_spec.rb b/spec/controllers/dashboard/projects_controller_spec.rb index 6591901a9dc..8b95c9f2496 100644 --- a/spec/controllers/dashboard/projects_controller_spec.rb +++ b/spec/controllers/dashboard/projects_controller_spec.rb @@ -40,6 +40,14 @@ describe Dashboard::ProjectsController do expect(assigns(:projects)).to eq([project, project2]) end + + context 'project sorting' do + let(:project) { create(:project) } + + it_behaves_like 'set sort order from user preference' do + let(:sorting_param) { 'created_asc' } + end + end end end diff --git a/spec/controllers/explore/projects_controller_spec.rb b/spec/controllers/explore/projects_controller_spec.rb index 463586ee422..6752d2b8ebd 100644 --- a/spec/controllers/explore/projects_controller_spec.rb +++ b/spec/controllers/explore/projects_controller_spec.rb @@ -3,56 +3,91 @@ require 'spec_helper' describe Explore::ProjectsController do - describe 'GET #index.json' do - render_views + shared_examples 'explore projects' do + describe 'GET #index.json' do + render_views - before do - get :index, format: :json + before do + get :index, format: :json + end + + it { is_expected.to respond_with(:success) } end - it { is_expected.to respond_with(:success) } - end + describe 'GET #trending.json' do + render_views - describe 'GET #trending.json' do - render_views + before do + get :trending, format: :json + end - before do - get :trending, format: :json + it { is_expected.to respond_with(:success) } + end + + describe 'GET #starred.json' do + render_views + + before do + get :starred, format: :json + end + + it { is_expected.to respond_with(:success) } end - it { is_expected.to respond_with(:success) } + describe 'GET #trending' do + context 'sorting by update date' do + let(:project1) { create(:project, :public, updated_at: 3.days.ago) } + let(:project2) { create(:project, :public, updated_at: 1.day.ago) } + + before do + create(:trending_project, project: project1) + create(:trending_project, project: project2) + end + + it 'sorts by last updated' do + get :trending, params: { sort: 'updated_desc' } + + expect(assigns(:projects)).to eq [project2, project1] + end + + it 'sorts by oldest updated' do + get :trending, params: { sort: 'updated_asc' } + + expect(assigns(:projects)).to eq [project1, project2] + end + end + end end - describe 'GET #starred.json' do - render_views + context 'when user is signed in' do + let(:user) { create(:user) } before do - get :starred, format: :json + sign_in(user) end - it { is_expected.to respond_with(:success) } - end + include_examples 'explore projects' - describe 'GET #trending' do - context 'sorting by update date' do - let(:project1) { create(:project, :public, updated_at: 3.days.ago) } - let(:project2) { create(:project, :public, updated_at: 1.day.ago) } + context 'user preference sorting' do + let(:project) { create(:project) } - before do - create(:trending_project, project: project1) - create(:trending_project, project: project2) + it_behaves_like 'set sort order from user preference' do + let(:sorting_param) { 'created_asc' } end + end + end - it 'sorts by last updated' do - get :trending, params: { sort: 'updated_desc' } + context 'when user is not signed in' do + include_examples 'explore projects' - expect(assigns(:projects)).to eq [project2, project1] - end + context 'user preference sorting' do + let(:project) { create(:project) } + let(:sorting_param) { 'created_asc' } - it 'sorts by oldest updated' do - get :trending, params: { sort: 'updated_asc' } + it 'does not set sort order from user preference' do + expect_any_instance_of(UserPreference).not_to receive(:update) - expect(assigns(:projects)).to eq [project1, project2] + get :index, params: { sort: sorting_param } end end end diff --git a/spec/support/shared_examples/controllers/set_sort_order_from_user_preference_shared_examples.rb b/spec/support/shared_examples/controllers/set_sort_order_from_user_preference_shared_examples.rb index 1cd14ea2251..d89eded6e69 100644 --- a/spec/support/shared_examples/controllers/set_sort_order_from_user_preference_shared_examples.rb +++ b/spec/support/shared_examples/controllers/set_sort_order_from_user_preference_shared_examples.rb @@ -2,14 +2,14 @@ shared_examples 'set sort order from user preference' do describe '#set_sort_order_from_user_preference' do - # There is no issuable_sorting_field defined in any CE controllers yet, + # There is no sorting_field defined in any CE controllers yet, # however any other field present in user_preferences table can be used for testing. context 'when database is in read-only mode' do it 'does not update user preference' do allow(Gitlab::Database).to receive(:read_only?).and_return(true) - expect_any_instance_of(UserPreference).not_to receive(:update).with({ controller.send(:issuable_sorting_field) => sorting_param }) + expect_any_instance_of(UserPreference).not_to receive(:update).with({ controller.send(:sorting_field) => sorting_param }) get :index, params: { namespace_id: project.namespace, project_id: project, sort: sorting_param } end @@ -19,7 +19,7 @@ shared_examples 'set sort order from user preference' do it 'updates user preference' do allow(Gitlab::Database).to receive(:read_only?).and_return(false) - expect_any_instance_of(UserPreference).to receive(:update).with({ controller.send(:issuable_sorting_field) => sorting_param }) + expect_any_instance_of(UserPreference).to receive(:update).with({ controller.send(:sorting_field) => sorting_param }) get :index, params: { namespace_id: project.namespace, project_id: project, sort: sorting_param } end -- cgit v1.2.1