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

17 files changed, 151 insertions, 45 deletions

diff --git a/app/controllers/concerns/issuable_collections.rb b/app/controllers/concerns/issuable_collections.rb
index 789e0dc736e..07d0bf16d93 100644
--- a/app/controllers/concerns/issuable_collections.rb
+++ b/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
diff --git a/app/controllers/concerns/issues_action.rb b/app/controllers/concerns/issuable_collections_action.rb
index a75590457d6..18ed4027eac 100644
--- a/app/controllers/concerns/issues_action.rb
+++ b/app/controllers/concerns/issuable_collections_action.rb
@@ -1,6 +1,6 @@
 # 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
diff --git a/app/controllers/concerns/merge_requests_action.rb b/app/controllers/concerns/merge_requests_action.rb
deleted file mode 100644
index ed10f32512e..00000000000
--- a/app/controllers/concerns/merge_requests_action.rb
+++ /dev/null
@@ -1,25 +0,0 @@
-# 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
diff --git a/app/controllers/dashboard_controller.rb b/app/controllers/dashboard_controller.rb
index be2d9512c01..75329b05a6f 100644
--- a/app/controllers/dashboard_controller.rb
+++ b/app/controllers/dashboard_controller.rb
@@ -1,8 +1,7 @@
 # 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) }
diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb
index c5d8ac2ed77..15aadf3f74b 100644
--- a/app/controllers/groups_controller.rb
+++ b/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
 
diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb
index e3e60665506..fd5f3eeaa99 100644
--- a/app/controllers/projects/issues_controller.rb
+++ b/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)
diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb
index accc37557b0..bc0a3d3526d 100644
--- a/app/controllers/projects/merge_requests_controller.rb
+++ b/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
diff --git a/app/models/issue.rb b/app/models/issue.rb
index 5c4ecbfdf4e..182c5d3d4b0 100644
--- a/app/models/issue.rb
+++ b/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'
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 7206d858dae..84cb8e1c50b 100644
--- a/app/models/merge_request.rb
+++ b/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
diff --git a/changelogs/unreleased/50352-sort-save.yml b/changelogs/unreleased/50352-sort-save.yml
new file mode 100644
index 00000000000..cd046c8b785
--- /dev/null
+++ b/changelogs/unreleased/50352-sort-save.yml
@@ -0,0 +1,5 @@
+---
+title: Save issues/merge request sorting options to backend
+merge_request: 24198
+author:
+type: added
diff --git a/db/migrate/20190116234221_add_sorting_fields_to_user_preference.rb b/db/migrate/20190116234221_add_sorting_fields_to_user_preference.rb
new file mode 100644
index 00000000000..7bf581fe9b0
--- /dev/null
+++ b/db/migrate/20190116234221_add_sorting_fields_to_user_preference.rb
@@ -0,0 +1,21 @@
+# 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
diff --git a/db/schema.rb b/db/schema.rb
index 859f007dbe1..7c1733becb9 100644
--- a/db/schema.rb
+++ b/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
 
diff --git a/spec/controllers/concerns/issuable_collections_spec.rb b/spec/controllers/concerns/issuable_collections_spec.rb
index 5a3a7a15f5a..307c5d60c57 100644
--- a/spec/controllers/concerns/issuable_collections_spec.rb
+++ b/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) { {} }
diff --git a/spec/controllers/projects/issues_controller_spec.rb b/spec/controllers/projects/issues_controller_spec.rb
index a2c3bb2919d..8ea5b4ea09c 100644
--- a/spec/controllers/projects/issues_controller_spec.rb
+++ b/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 }
diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb
index 53d5bf752ef..01a27f0429b 100644
--- a/spec/controllers/projects/merge_requests_controller_spec.rb
+++ b/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 }
diff --git a/spec/models/user_preference_spec.rb b/spec/models/user_preference_spec.rb
index 2898613545c..b2ef17a81d4 100644
--- a/spec/models/user_preference_spec.rb
+++ b/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
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 d86838719d4..98ab04c5636 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,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