Save sorting preference for Issues/MRs in BE

In order to let users' sorting preferences transfer between devices, we
save the preference for issues and MRs (one preference for issues, one
for MRs) in the backend inside the UserPreference object

app/controllers/concerns/issuable_collections.rb

@@ -129,13 +129,13 @@ module IssuableCollections
     return sort_param if Gitlab::Database.read_only?
 
     if user_preference[issuable_sorting_field] != sort_param
-      user_preference.update_attribute(issuable_sorting_field, sort_param)
+      user_preference.update(issuable_sorting_field => sort_param)
     end
 
     sort_param
   end
 
-  # Implement default_sorting_field method on controllers
+  # Implement issuable_sorting_field method on controllers
   # to choose which column to store the sorting parameter.
   def issuable_sorting_field
     nil

app/controllers/concerns/issues_action.rb → app/controllers/concerns/issuable_collections_action.rb

 # frozen_string_literal: true
 
-module IssuesAction
+module IssuableCollectionsAction
   extend ActiveSupport::Concern
   include IssuableCollections
   include IssuesCalendar
@@ -18,6 +18,12 @@ module IssuesAction
       format.atom { render layout: 'xml.atom' }
     end
   end
+
+  def merge_requests
+    @merge_requests = issuables_collection.page(params[:page])
+
+    @issuable_meta_data = issuable_meta_data(@merge_requests, collection_type)
+  end
   # rubocop:enable Gitlab/ModuleWithInstanceVariables
 
   def issues_calendar
@@ -26,8 +32,29 @@ module IssuesAction
 
   private
 
+  def issuable_sorting_field
+    case action_name
+    when 'issues'
+      Issue::SORTING_PREFERENCE_FIELD
+    when 'merge_requests'
+      MergeRequest::SORTING_PREFERENCE_FIELD
+    else
+      nil
+    end
+  end
+
   def finder_type
-    (super if defined?(super)) ||
-      (IssuesFinder if %w(issues issues_calendar).include?(action_name))
+    case action_name
+    when 'issues', 'issues_calendar'
+      IssuesFinder
+    when 'merge_requests'
+      MergeRequestsFinder
+    else
+      nil
+    end
+  end
+
+  def finder_options
+    super.merge(non_archived: true)
   end
 end

app/controllers/concerns/merge_requests_action.rb

-# frozen_string_literal: true
-
-module MergeRequestsAction
-  extend ActiveSupport::Concern
-  include IssuableCollections
-
-  # rubocop:disable Gitlab/ModuleWithInstanceVariables
-  def merge_requests
-    @merge_requests = issuables_collection.page(params[:page])
-
-    @issuable_meta_data = issuable_meta_data(@merge_requests, collection_type)
-  end
-  # rubocop:enable Gitlab/ModuleWithInstanceVariables
-
-  private
-
-  def finder_type
-    (super if defined?(super)) ||
-      (MergeRequestsFinder if action_name == 'merge_requests')
-  end
-
-  def finder_options
-    super.merge(non_archived: true)
-  end
-end

app/controllers/dashboard_controller.rb

 # frozen_string_literal: true
 
 class DashboardController < Dashboard::ApplicationController
-  include IssuesAction
-  include MergeRequestsAction
+  include IssuableCollectionsAction
 
   prepend_before_action(only: [:issues]) { authenticate_sessionless_user!(:rss) }
   prepend_before_action(only: [:issues_calendar]) { authenticate_sessionless_user!(:ics) }

app/controllers/groups_controller.rb

@@ -2,8 +2,7 @@
 
 class GroupsController < Groups::ApplicationController
   include API::Helpers::RelatedResourcesHelpers
-  include IssuesAction
-  include MergeRequestsAction
+  include IssuableCollectionsAction
   include ParamsBackwardCompatibility
   include PreviewMarkdown
 

app/controllers/projects/issues_controller.rb

@@ -191,6 +191,10 @@ class Projects::IssuesController < Projects::ApplicationController
 
   protected
 
+  def issuable_sorting_field
+    Issue::SORTING_PREFERENCE_FIELD
+  end
+
   # rubocop: disable CodeReuse/ActiveRecord
   def issue
     return @issue if defined?(@issue)

app/controllers/projects/merge_requests_controller.rb

@@ -230,6 +230,10 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
   alias_method :issuable, :merge_request
   alias_method :awardable, :merge_request
 
+  def issuable_sorting_field
+    MergeRequest::SORTING_PREFERENCE_FIELD
+  end
+
   def merge_params
     params.permit(merge_params_attributes)
   end

app/models/issue.rb

@@ -26,6 +26,8 @@ class Issue < ActiveRecord::Base
   DueThisMonth                    = DueDateStruct.new('Due This Month', 'month').freeze
   DueNextMonthAndPreviousTwoWeeks = DueDateStruct.new('Due Next Month And Previous Two Weeks', 'next_month_and_previous_two_weeks').freeze
 
+  SORTING_PREFERENCE_FIELD = :issues_sort
+
   belongs_to :project
   belongs_to :moved_to, class_name: 'Issue'
   belongs_to :closed_by, class_name: 'User'

app/models/merge_request.rb

@@ -21,6 +21,8 @@ class MergeRequest < ActiveRecord::Base
   self.reactive_cache_refresh_interval = 10.minutes
   self.reactive_cache_lifetime = 10.minutes
 
+  SORTING_PREFERENCE_FIELD = :merge_requests_sort
+
   ignore_column :locked_at,
                 :ref_fetched,
                 :deleted_at

changelogs/unreleased/50352-sort-save.yml

+---
+title: Save issues/merge request sorting options to backend
+merge_request: 24198
+author:
+type: added

db/migrate/20190116234221_add_sorting_fields_to_user_preference.rb

+# frozen_string_literal: true
+
+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class AddSortingFieldsToUserPreference < ActiveRecord::Migration[5.0]
+  include Gitlab::Database::MigrationHelpers
+
+  # Set this constant to true if this migration requires downtime.
+  DOWNTIME = false
+
+  def up
+    add_column :user_preferences, :issues_sort, :string
+    add_column :user_preferences, :merge_requests_sort, :string
+  end
+
+  def down
+    remove_column :user_preferences, :issues_sort
+    remove_column :user_preferences, :merge_requests_sort
+  end
+end

db/schema.rb

@@ -2146,6 +2146,8 @@ ActiveRecord::Schema.define(version: 20190124200344) do
     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
+    t.string "issues_sort"
+    t.string "merge_requests_sort"
     t.index ["user_id"], name: "index_user_preferences_on_user_id", unique: true, using: :btree
   end
 

spec/controllers/concerns/issuable_collections_spec.rb

@@ -17,10 +17,55 @@ describe IssuableCollections do
     controller = klass.new
 
     allow(controller).to receive(:params).and_return(ActionController::Parameters.new(params))
+    allow(controller).to receive(:current_user).and_return(user)
 
     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) { {} }

spec/controllers/projects/issues_controller_spec.rb

@@ -42,7 +42,9 @@ describe Projects::IssuesController do
 
       it_behaves_like "issuables list meta-data", :issue
 
-      it_behaves_like 'set sort order from user preference'
+      it_behaves_like 'set sort order from user preference' do
+        let(:sorting_param) { 'updated_asc' }
+      end
 
       it "returns index" do
         get :index, params: { namespace_id: project.namespace, project_id: project }

spec/controllers/projects/merge_requests_controller_spec.rb

@@ -153,7 +153,9 @@ describe Projects::MergeRequestsController do
 
     it_behaves_like "issuables list meta-data", :merge_request
 
-    it_behaves_like 'set sort order from user preference'
+    it_behaves_like 'set sort order from user preference' do
+      let(:sorting_param) { 'updated_asc' }
+    end
 
     context 'when page param' do
       let(:last_page) { project.merge_requests.page().total_pages }

spec/models/user_preference_spec.rb

@@ -3,9 +3,10 @@
 require 'spec_helper'
 
 describe UserPreference do
+  let(:user_preference) { create(:user_preference) }
+
   describe '#set_notes_filter' do
     let(:issuable) { build_stubbed(:issue) }
-    let(:user_preference) { create(:user_preference) }
 
     shared_examples 'setting system notes' do
       it 'returns updated discussion filter' do
@@ -50,4 +51,26 @@ describe UserPreference do
       end
     end
   end
+
+  describe 'sort_by preferences' do
+    shared_examples_for 'a sort_by preference' do
+      it 'allows nil sort fields' do
+        user_preference.update(attribute => nil)
+
+        expect(user_preference).to be_valid
+      end
+    end
+
+    context 'merge_requests_sort attribute' do
+      let(:attribute) { :merge_requests_sort }
+
+      it_behaves_like 'a sort_by preference'
+    end
+
+    context 'issues_sort attribute' do
+      let(:attribute) { :issues_sort }
+
+      it_behaves_like 'a sort_by preference'
+    end
+  end
 end

spec/support/shared_examples/controllers/set_sort_order_from_user_preference_shared_examples.rb

@@ -2,18 +2,12 @@ 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,
     # however any other field present in user_preferences table can be used for testing.
-    let(:sorting_field) { :issue_notes_filter }
-    let(:sorting_param) { 'any' }
-
-    before do
-      allow(controller).to receive(:issuable_sorting_field).and_return(sorting_field)
-    end
 
     context 'when database is in read-only mode' do
       it '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_attribute).with(sorting_field, sorting_param)
+        expect_any_instance_of(UserPreference).not_to receive(:update).with({ controller.send(:issuable_sorting_field) => sorting_param })
 
         get :index, params: { namespace_id: project.namespace, project_id: project, sort: sorting_param }
       end
@@ -23,7 +17,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_attribute).with(sorting_field, sorting_param)
+        expect_any_instance_of(UserPreference).to receive(:update).with({ controller.send(:issuable_sorting_field) => sorting_param })
 
         get :index, params: { namespace_id: project.namespace, project_id: project, sort: sorting_param }
       end