Merge branch 'issue_40630' into 'master'

Save board lists collapsed setting

Closes #52758

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

app/controllers/boards/lists_controller.rb

@@ -4,7 +4,7 @@ module Boards
   class ListsController < Boards::ApplicationController
     include BoardsResponses
 
-    before_action :authorize_admin_list, only: [:create, :update, :destroy, :generate]
+    before_action :authorize_admin_list, only: [:create, :destroy, :generate]
     before_action :authorize_read_list, only: [:index]
     skip_before_action :authenticate_user!, only: [:index]
 
@@ -15,7 +15,7 @@ module Boards
     end
 
     def create
-      list = Boards::Lists::CreateService.new(board.parent, current_user, list_params).execute(board)
+      list = Boards::Lists::CreateService.new(board.parent, current_user, create_list_params).execute(board)
 
       if list.valid?
         render json: serialize_as_json(list)
@@ -26,12 +26,13 @@ module Boards
 
     def update
       list = board.lists.movable.find(params[:id])
-      service = Boards::Lists::MoveService.new(board_parent, current_user, move_params)
+      service = Boards::Lists::UpdateService.new(board_parent, current_user, update_list_params)
+      result = service.execute(list)
 
-      if service.execute(list)
+      if result[:status] == :success
         head :ok
       else
-        head :unprocessable_entity
+        head result[:http_status]
       end
     end
 
@@ -50,7 +51,8 @@ module Boards
       service = Boards::Lists::GenerateService.new(board_parent, current_user)
 
       if service.execute(board)
-        render json: serialize_as_json(board.lists.movable)
+        lists = board.lists.movable.preload_associations(current_user)
+        render json: serialize_as_json(lists)
       else
         head :unprocessable_entity
       end
@@ -62,12 +64,12 @@ module Boards
       %i[label_id]
     end
 
-    def list_params
+    def create_list_params
       params.require(:list).permit(list_creation_attrs)
     end
 
-    def move_params
-      params.require(:list).permit(:position)
+    def update_list_params
+      params.require(:list).permit(:collapsed, :position)
     end
 
     def serialize_as_json(resource)
@@ -78,7 +80,9 @@ module Boards
       {
         only: [:id, :list_type, :position],
         methods: [:title],
-        label: true
+        label: true,
+        collapsed: true,
+        current_user: current_user
       }
     end
   end

app/models/list.rb

 # frozen_string_literal: true
 
 class List < ApplicationRecord
+  include Importable
+
   belongs_to :board
   belongs_to :label
-  include Importable
+  has_many :list_user_preferences
 
   enum list_type: { backlog: 0, label: 1, closed: 2, assignee: 3, milestone: 4 }
 
@@ -16,9 +18,24 @@ class List < ApplicationRecord
 
   scope :destroyable, -> { where(list_type: list_types.slice(*destroyable_types).values) }
   scope :movable, -> { where(list_type: list_types.slice(*movable_types).values) }
-  scope :preload_associations, -> { preload(:board, :label) }
+
+  scope :preload_associations, -> (user) do
+    preload(:board, label: :priorities)
+      .with_preferences_for(user)
+  end
+
   scope :ordered, -> { order(:list_type, :position) }
 
+  # Loads list with preferences for given user
+  # if preferences exists for user or not
+  scope :with_preferences_for, -> (user) do
+    return unless user
+
+    includes(:list_user_preferences).where(list_user_preferences: { user_id: [user.id, nil] })
+  end
+
+  alias_method :preferences, :list_user_preferences
+
   class << self
     def destroyable_types
       [:label]
@@ -29,6 +46,31 @@ class List < ApplicationRecord
     end
   end
 
+  def preferences_for(user)
+    return preferences.build unless user
+
+    if preferences.loaded?
+      preloaded_preferences_for(user)
+    else
+      preferences.find_or_initialize_by(user: user)
+    end
+  end
+
+  def preloaded_preferences_for(user)
+    user_preferences =
+      preferences.find do |preference|
+        preference.user_id == user.id
+      end
+
+    user_preferences || preferences.build(user: user)
+  end
+
+  def update_preferences_for(user, preferences = {})
+    return unless user
+
+    preferences_for(user).update(preferences)
+  end
+
   def destroyable?
     self.class.destroyable_types.include?(list_type&.to_sym)
   end
@@ -43,6 +85,14 @@ class List < ApplicationRecord
 
   def as_json(options = {})
     super(options).tap do |json|
+      json[:collapsed] = false
+
+      if options.key?(:collapsed)
+        preferences = preferences_for(options[:current_user])
+
+        json[:collapsed] = preferences.collapsed?
+      end
+
       if options.key?(:label)
         json[:label] = label.as_json(
           project: board.project,

app/models/list_user_preference.rb

+# frozen_string_literal: true
+
+class ListUserPreference < ApplicationRecord
+  belongs_to :user
+  belongs_to :list
+
+  validates :user, presence: true
+  validates :list, presence: true
+  validates :user_id, uniqueness: { scope: :list_id, message: "should have only one list preference per user" }
+end

app/services/boards/lists/list_service.rb

@@ -6,7 +6,7 @@ module Boards
       def execute(board)
         board.lists.create(list_type: :backlog) unless board.lists.backlog.exists?
 
-        board.lists.preload_associations
+        board.lists.preload_associations(current_user)
       end
     end
   end

app/services/boards/lists/update_service.rb

+# frozen_string_literal: true
+
+module Boards
+  module Lists
+    class UpdateService < Boards::BaseService
+      def execute(list)
+        return not_authorized if preferences? && !can_read?(list)
+        return not_authorized if position? && !can_admin?(list)
+
+        if update_preferences(list) || update_position(list)
+          success(list: list)
+        else
+          error(list.errors.messages, 422)
+        end
+      end
+
+      def update_preferences(list)
+        return unless preferences?
+
+        list.update_preferences_for(current_user, preferences)
+      end
+
+      def update_position(list)
+        return unless position?
+
+        move_service = Boards::Lists::MoveService.new(parent, current_user, params)
+
+        move_service.execute(list)
+      end
+
+      def preferences
+        { collapsed: Gitlab::Utils.to_boolean(params[:collapsed]) }
+      end
+
+      def not_authorized
+        error("Not authorized", 403)
+      end
+
+      def preferences?
+        params.has_key?(:collapsed)
+      end
+
+      def position?
+        params.has_key?(:position)
+      end
+
+      def can_read?(list)
+        Ability.allowed?(current_user, :read_list, parent)
+      end
+
+      def can_admin?(list)
+        Ability.allowed?(current_user, :admin_list, parent)
+      end
+    end
+  end
+end

changelogs/unreleased/issue_40630.yml

+---
+title: Save collapsed option for board lists in database
+merge_request: 31069
+author:
+type: added

db/migrate/20190822181528_create_list_user_preferences.rb

+# frozen_string_literal: true
+
+class CreateListUserPreferences < ActiveRecord::Migration[5.2]
+  DOWNTIME = false
+
+  def change
+    create_table :list_user_preferences do |t|
+      t.references :user, index: true, null: false, foreign_key: { on_delete: :cascade }
+      t.references :list, index: true, null: false, foreign_key: { on_delete: :cascade }
+      t.timestamps_with_timezone null: false
+      t.boolean :collapsed
+    end
+
+    add_index :list_user_preferences, [:user_id, :list_id], unique: true
+  end
+end

db/schema.rb

@@ -1922,6 +1922,17 @@ ActiveRecord::Schema.define(version: 2019_08_28_083843) do
     t.datetime "updated_at"
   end
 
+  create_table "list_user_preferences", force: :cascade do |t|
+    t.bigint "user_id", null: false
+    t.bigint "list_id", null: false
+    t.datetime_with_timezone "created_at", null: false
+    t.datetime_with_timezone "updated_at", null: false
+    t.boolean "collapsed"
+    t.index ["list_id"], name: "index_list_user_preferences_on_list_id"
+    t.index ["user_id", "list_id"], name: "index_list_user_preferences_on_user_id_and_list_id", unique: true
+    t.index ["user_id"], name: "index_list_user_preferences_on_user_id"
+  end
+
   create_table "lists", id: :serial, force: :cascade do |t|
     t.integer "board_id", null: false
     t.integer "label_id"
@@ -3880,6 +3891,8 @@ ActiveRecord::Schema.define(version: 2019_08_28_083843) do
   add_foreign_key "labels", "projects", name: "fk_7de4989a69", on_delete: :cascade
   add_foreign_key "lfs_file_locks", "projects", on_delete: :cascade
   add_foreign_key "lfs_file_locks", "users", on_delete: :cascade
+  add_foreign_key "list_user_preferences", "lists", on_delete: :cascade
+  add_foreign_key "list_user_preferences", "users", on_delete: :cascade
   add_foreign_key "lists", "boards", name: "fk_0d3f677137", on_delete: :cascade
   add_foreign_key "lists", "labels", name: "fk_7a5553d60f", on_delete: :cascade
   add_foreign_key "lists", "milestones", on_delete: :cascade

spec/controllers/boards/lists_controller_spec.rb

@@ -30,6 +30,21 @@ describe Boards::ListsController do
       expect(json_response.length).to eq 3
     end
 
+    it 'avoids n+1 queries when serializing lists' do
+      list_1 = create(:list, board: board)
+      list_1.update_preferences_for(user, { collapsed: true })
+
+      control_count = ActiveRecord::QueryRecorder.new { read_board_list user: user, board: board }.count
+
+      list_2 = create(:list, board: board)
+      list_2.update_preferences_for(user, { collapsed: true })
+
+      list_3 = create(:list, board: board)
+      list_3.update_preferences_for(user, { collapsed: true })
+
+      expect { read_board_list user: user, board: board }.not_to exceed_query_limit(control_count)
+    end
+
     context 'with unauthorized user' do
       let(:unauth_user) { create(:user) }
 
@@ -154,6 +169,22 @@ describe Boards::ListsController do
       end
     end
 
+    context 'with collapsed preference' do
+      it 'saves collapsed preference for user' do
+        save_setting user: user, board: board, list: planning, setting: { collapsed: true }
+
+        expect(planning.preferences_for(user).collapsed).to eq(true)
+        expect(response).to have_gitlab_http_status(200)
+      end
+
+      it 'saves not collapsed preference for user' do
+        save_setting user: user, board: board, list: planning, setting: { collapsed: false }
+
+        expect(planning.preferences_for(user).collapsed).to eq(false)
+        expect(response).to have_gitlab_http_status(200)
+      end
+    end
+
     def move(user:, board:, list:, position:)
       sign_in(user)
 
@@ -166,6 +197,19 @@ describe Boards::ListsController do
 
       patch :update, params: params, as: :json
     end
+
+    def save_setting(user:, board:, list:, setting: {})
+      sign_in(user)
+
+      params = { namespace_id: project.namespace.to_param,
+                 project_id: project,
+                 board_id: board.to_param,
+                 id: list.to_param,
+                 list: setting,
+                 format: :json }
+
+      patch :update, params: params, as: :json
+    end
   end
 
   describe 'DELETE destroy' do

spec/lib/gitlab/import_export/all_models.yml

@@ -483,3 +483,4 @@ lists:
 - milestone
 - board
 - label
+- list_user_preferences

spec/models/list_spec.rb

@@ -81,4 +81,83 @@ describe List do
       expect(subject.title).to eq 'Closed'
     end
   end
+
+  describe '#update_preferences_for' do
+    let(:user) { create(:user) }
+    let(:list) { create(:list) }
+
+    context 'when user is present' do
+      context 'when there are no preferences for user' do
+        it 'creates new user preferences' do
+          expect { list.update_preferences_for(user, collapsed: true) }.to change { ListUserPreference.count }.by(1)
+          expect(list.preferences_for(user).collapsed).to eq(true)
+        end
+      end
+
+      context 'when there are preferences for user' do
+        it 'updates user preferences' do
+          list.update_preferences_for(user, collapsed: false)
+
+          expect { list.update_preferences_for(user, collapsed: true) }.not_to change { ListUserPreference.count }
+          expect(list.preferences_for(user).collapsed).to eq(true)
+        end
+      end
+
+      context 'when user is nil' do
+        it 'does not create user preferences' do
+          expect { list.update_preferences_for(nil, collapsed: true) }.not_to change { ListUserPreference.count }
+        end
+      end
+    end
+  end
+
+  describe '#preferences_for' do
+    let(:user) { create(:user) }
+    let(:list) { create(:list) }
+
+    context 'when user is nil' do
+      it 'returns not persisted preferences' do
+        preferences = list.preferences_for(nil)
+
+        expect(preferences.persisted?).to eq(false)
+        expect(preferences.list_id).to eq(list.id)
+        expect(preferences.user_id).to be_nil
+      end
+    end
+
+    context 'when a user preference already exists' do
+      before do
+        list.update_preferences_for(user, collapsed: true)
+      end
+
+      it 'loads preference for user' do
+        preferences = list.preferences_for(user)
+
+        expect(preferences).to be_persisted
+        expect(preferences.collapsed).to eq(true)
+      end
+
+      context 'when preferences are already loaded for user' do
+        it 'gets preloaded user preferences' do
+          fetched_list = described_class.where(id: list.id).with_preferences_for(user).first
+
+          expect(fetched_list).to receive(:preloaded_preferences_for).with(user).and_call_original
+
+          preferences = fetched_list.preferences_for(user)
+
+          expect(preferences.collapsed).to eq(true)
+        end
+      end
+    end
+
+    context 'when preferences for user does not exist' do
+      it 'returns not persisted preferences' do
+        preferences = list.preferences_for(user)
+
+        expect(preferences.persisted?).to eq(false)
+        expect(preferences.user_id).to eq(user.id)
+        expect(preferences.list_id).to eq(list.id)
+      end
+    end
+  end
 end

spec/models/list_user_preference_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe ListUserPreference do
+  set(:user) { create(:user) }
+  set(:list) { create(:list) }
+
+  before do
+    list.update_preferences_for(user, { collapsed: true })
+  end
+
+  describe 'relationships' do
+    it { is_expected.to belong_to(:list) }
+    it { is_expected.to belong_to(:user) }
+
+    it do
+      is_expected.to validate_uniqueness_of(:user_id).scoped_to(:list_id)
+                       .with_message("should have only one list preference per user")
+    end
+  end
+end

spec/services/boards/lists/list_service_spec.rb

@@ -3,13 +3,15 @@
 require 'spec_helper'
 
 describe Boards::Lists::ListService do
+  let(:user) { create(:user) }
+
   describe '#execute' do
     context 'when board parent is a project' do
       let(:project) { create(:project) }
       let(:board) { create(:board, project: project) }
       let(:label) { create(:label, project: project) }
       let!(:list) { create(:list, board: board, label: label) }
-      let(:service) { described_class.new(project, double) }
+      let(:service) { described_class.new(project, user) }
 
       it_behaves_like 'lists list service'
     end
@@ -19,7 +21,7 @@ describe Boards::Lists::ListService do
       let(:board) { create(:board, group: group) }
       let(:label) { create(:group_label, group: group) }
       let!(:list) { create(:list, board: board, label: label) }
-      let(:service) { described_class.new(group, double) }
+      let(:service) { described_class.new(group, user) }
 
       it_behaves_like 'lists list service'
     end

spec/services/boards/lists/update_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Boards::Lists::UpdateService do
+  let(:user) { create(:user) }
+  let!(:list) { create(:list, board: board, position: 0) }
+
+  shared_examples 'moving list' do
+    context 'when user can admin list' do
+      it 'calls Lists::MoveService to update list position' do
+        board.parent.add_developer(user)
+        service = described_class.new(board.parent, user, position: 1)
+
+        expect(Boards::Lists::MoveService).to receive(:new).with(board.parent, user, { position: 1 }).and_call_original
+        expect_any_instance_of(Boards::Lists::MoveService).to receive(:execute).with(list)
+
+        service.execute(list)
+      end
+    end
+
+    context 'when user cannot admin list' do
+      it 'does not call Lists::MoveService to update list position' do
+        service = described_class.new(board.parent, user, position: 1)
+
+        expect(Boards::Lists::MoveService).not_to receive(:new)
+
+        service.execute(list)
+      end
+    end
+  end
+
+  shared_examples 'updating list preferences' do
+    context 'when user can read list' do
+      it 'updates list preference for user' do
+        board.parent.add_guest(user)
+        service = described_class.new(board.parent, user, collapsed: true)
+
+        service.execute(list)
+
+        expect(list.preferences_for(user).collapsed).to eq(true)
+      end
+    end
+
+    context 'when user cannot read list' do
+      it 'does not update list preference for user' do
+        service = described_class.new(board.parent, user, collapsed: true)
+
+        service.execute(list)
+
+        expect(list.preferences_for(user).collapsed).to be_nil
+      end
+    end
+  end
+
+  describe '#execute' do
+    context 'when position parameter is present' do
+      context 'for projects' do
+        it_behaves_like 'moving list' do
+          let(:project) { create(:project, :private) }
+          let(:board) { create(:board, project: project) }
+        end
+      end
+
+      context 'for groups' do
+        it_behaves_like 'moving list' do
+          let(:group) { create(:group, :private) }
+          let(:board) { create(:board, group: group) }
+        end
+      end
+    end
+
+    context 'when collapsed parameter is present' do
+      context 'for projects' do
+        it_behaves_like 'updating list preferences' do
+          let(:project) { create(:project, :private) }
+          let(:board) { create(:board, project: project) }
+        end
+      end
+
+      context 'for groups' do
+        it_behaves_like 'updating list preferences' do
+          let(:group) { create(:group, :private) }
+          let(:board) { create(:board, group: group) }
+        end
+      end
+    end
+  end
+end