Merge branch 'georgekoltsov/18720-persistent-dashboard-sort' into 'master'

Sorting projects - Persist last choice

See merge request gitlab-org/gitlab-ce!31669

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

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

app/controllers/concerns/sorting_preference.rb

+# 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

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

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

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
 

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
 

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?,

changelogs/unreleased/georgekoltsov-18720-persistent-dashboard-sort.yml

+---
+title: Add persistance to last choice of projects sorting on projects dashboard page
+merge_request: 31669
+author:
+type: added

db/migrate/20190808152507_add_projects_sorting_field_to_user_preferences.rb

+# 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

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
 

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' } }
 

spec/controllers/concerns/sorting_preference_spec.rb

+# 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

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
 

spec/controllers/explore/projects_controller_spec.rb

@@ -3,6 +3,7 @@
 require 'spec_helper'
 
 describe Explore::ProjectsController do
+  shared_examples 'explore projects' do
     describe 'GET #index.json' do
       render_views
 
@@ -56,4 +57,38 @@ describe Explore::ProjectsController do
         end
       end
     end
+  end
+
+  context 'when user is signed in' do
+    let(:user) { create(:user) }
+
+    before do
+      sign_in(user)
+    end
+
+    include_examples 'explore projects'
+
+    context 'user preference sorting' do
+      let(:project) { create(:project) }
+
+      it_behaves_like 'set sort order from user preference' do
+        let(:sorting_param) { 'created_asc' }
+      end
+    end
+  end
+
+  context 'when user is not signed in' do
+    include_examples 'explore projects'
+
+    context 'user preference sorting' do
+      let(:project) { create(:project) }
+      let(:sorting_param) { 'created_asc' }
+
+      it 'does not set sort order from user preference' do
+        expect_any_instance_of(UserPreference).not_to receive(:update)
+
+        get :index, params: { sort: sorting_param }
+      end
+    end
+  end
 end

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