Merge branch '32390-some-board-lists-missing-for-some-users' into 'master'

Reimplement preloading of user preferences for board lists

See merge request gitlab-org/gitlab!17314

app/controllers/boards/lists_controller.rb

@@ -11,6 +11,8 @@ module Boards
     def index
       lists = Boards::Lists::ListService.new(board.parent, current_user).execute(board)
 
+      List.preload_preferences_for_user(lists, current_user)
+
       render json: serialize_as_json(lists)
     end
 
@@ -51,7 +53,10 @@ module Boards
       service = Boards::Lists::GenerateService.new(board_parent, current_user)
 
       if service.execute(board)
-        lists = board.lists.movable.preload_associations(current_user)
+        lists = board.lists.movable.preload_associations
+
+        List.preload_preferences_for_user(lists, current_user)
+
         render json: serialize_as_json(lists)
       else
         head :unprocessable_entity

app/models/list.rb

@@ -21,20 +21,10 @@ 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, -> (user) do
-    preload(:board, label: :priorities)
-  end
+  scope :preload_associations, -> { preload(:board, label: :priorities) }
 
   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
@@ -45,25 +35,25 @@ class List < ApplicationRecord
     def movable_types
       [:label]
     end
+
+    def preload_preferences_for_user(lists, user)
+      return unless user
+
+      lists.each { |list| list.preferences_for(user) }
+    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
+    BatchLoader.for(list_id: id, user_id: user.id).batch(default_value: preferences.build(user: user)) do |items, loader|
+      list_ids = items.map { |i| i[:list_id] }
+      user_ids = items.map { |i| i[:user_id] }
 
-  def preloaded_preferences_for(user)
-    user_preferences =
-      preferences.find do |preference|
-        preference.user_id == user.id
+      ListUserPreference.where(list_id: list_ids, user_id: user_ids).find_each do |preference|
+        loader.call({ list_id: preference.list_id, user_id: preference.user_id }, preference)
       end
-
-    user_preferences || preferences.build(user: user)
+    end
   end
 
   def update_preferences_for(user, preferences = {})

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(current_user)
+        board.lists.preload_associations
       end
     end
   end

spec/controllers/boards/lists_controller_spec.rb

@@ -14,6 +14,10 @@ describe Boards::ListsController do
   end
 
   describe 'GET index' do
+    before do
+      create(:list, board: board)
+    end
+
     it 'returns a successful 200 response' do
       read_board_list user: user, board: board
 
@@ -22,27 +26,22 @@ describe Boards::ListsController do
     end
 
     it 'returns a list of board lists' do
-      create(:list, board: board)
-
       read_board_list user: user, board: board
 
       expect(response).to match_response_schema('lists')
       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 })
+    context 'when another user has list preferences' do
+      before do
+        board.lists.first.update_preferences_for(guest, collapsed: true)
+      end
 
-      list_3 = create(:list, board: board)
-      list_3.update_preferences_for(user, { collapsed: true })
+      it 'returns the complete list of board lists' do
+        read_board_list user: user, board: board
 
-      expect { read_board_list user: user, board: board }.not_to exceed_query_limit(control_count)
+        expect(json_response.length).to eq 3
+      end
     end
 
     context 'with unauthorized user' do

spec/models/list_spec.rb

@@ -136,18 +136,6 @@ describe List do
         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

spec/requests/boards/lists_controller_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Boards::ListsController do
+  describe '#index' do
+    let(:board) { create(:board) }
+    let(:user) { board.project.owner }
+
+    it 'does not have N+1 queries' do
+      login_as(user)
+
+      # First request has more queries because we create the default `backlog` list
+      get board_lists_path(board)
+
+      create(:list, board: board)
+
+      control_count = ActiveRecord::QueryRecorder.new { get board_lists_path(board) }.count
+
+      create_list(:list, 5, board: board)
+
+      expect { get board_lists_path(board) }.not_to exceed_query_limit(control_count)
+    end
+  end
+end