Merge branch '50352-sort-save' into 'master'

Save sorting preference for Issues/MRs in BE

Closes #50352

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

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