Apply reviewer feedback

- Add docs tweak to board list type
- Remove user_id index
- Make specs better

app/graphql/mutations/boards/lists/update.rb

@@ -17,7 +17,7 @@ module Mutations
 
         argument :collapsed, GraphQL::BOOLEAN_TYPE,
                   required: false,
-                  description: 'Indicates if list is collapsed for this user.'
+                  description: 'Indicates if the list is collapsed for this user.'
 
         field :list,
               Types::BoardListType,

app/graphql/types/board_list_type.rb

@@ -19,7 +19,7 @@ module Types
     field :label, Types::LabelType, null: true,
           description: 'Label of the list.'
     field :collapsed, GraphQL::BOOLEAN_TYPE, null: true,
-          description: 'Indicates if list is collapsed for this user.'
+          description: 'Indicates if the list is collapsed for this user.'
     field :issues_count, GraphQL::INT_TYPE, null: true,
           description: 'Count of issues in the list.'
 

app/models/concerns/boards/listable.rb

@@ -13,6 +13,14 @@ module Boards
       scope :ordered, -> { order(:list_type, :position) }
       scope :destroyable, -> { where(list_type: list_types.slice(*destroyable_types).values) }
       scope :movable, -> { where(list_type: list_types.slice(*movable_types).values) }
+
+      class << self
+        def preload_preferences_for_user(lists, user)
+          return unless user
+
+          lists.each { |list| list.preferences_for(user) }
+        end
+      end
     end
 
     class_methods do

app/models/list.rb

@@ -18,14 +18,6 @@ class List < ApplicationRecord
 
   alias_method :preferences, :list_user_preferences
 
-  class << self
-    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
 

db/migrate/20210217101901_create_epic_list_user_preferences.rb

 # frozen_string_literal: true
 
 class CreateEpicListUserPreferences < ActiveRecord::Migration[6.0]
-  include Gitlab::Database::MigrationHelpers
-
   DOWNTIME = false
 
-  disable_ddl_transaction!
-
   def up
     create_table :boards_epic_list_user_preferences do |t|
-      t.references :user, index: true, null: false, foreign_key: { on_delete: :cascade }
-      t.references :epic_list, index: true, null: false, foreign_key: { to_table: :boards_epic_lists, on_delete: :cascade }
+      t.bigint :user_id, null: false
+      t.bigint :epic_list_id, index: true, null: false
       t.timestamps_with_timezone null: false
       t.boolean :collapsed, null: false, default: false
     end
 
-    add_concurrent_index :boards_epic_list_user_preferences, [:user_id, :epic_list_id], unique: true, name: 'index_epic_board_list_preferences_on_user_and_list'
+    add_index :boards_epic_list_user_preferences, [:user_id, :epic_list_id], unique: true, name: 'index_epic_board_list_preferences_on_user_and_list'
   end
 
   def down

db/migrate/20210222085529_add_epic_board_user_preference_user_fk.rb

+# frozen_string_literal: true
+
+class AddEpicBoardUserPreferenceUserFk < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_foreign_key :boards_epic_list_user_preferences, :users, column: :user_id, on_delete: :cascade
+  end
+
+  def down
+    with_lock_retries do
+      remove_foreign_key_if_exists :boards_epic_list_user_preferences, :users
+    end
+  end
+end

db/migrate/20210222085551_add_epic_board_user_preference_epic_list_fk.rb

+# frozen_string_literal: true
+
+class AddEpicBoardUserPreferenceEpicListFk < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_foreign_key :boards_epic_list_user_preferences, :boards_epic_lists, column: :epic_list_id, on_delete: :cascade
+  end
+
+  def down
+    with_lock_retries do
+      remove_foreign_key_if_exists :boards_epic_list_user_preferences, :boards_epic_lists
+    end
+  end
+end

db/schema_migrations/20210222085529

+858cd59ea324e3653801055c7f3fae2152b04ac175945a59faa00d67ae7fa223
\ No newline at end of file

db/schema_migrations/20210222085551

+9e6f99ed0c3d4d76a8c290308805cabf84aa7e5fb6dc2b06d973d9d8726fc4d8
\ No newline at end of file

db/structure.sql

@@ -9359,39 +9359,39 @@ CREATE TABLE application_settings (
     elasticsearch_indexed_file_size_limit_kb integer DEFAULT 1024 NOT NULL,
     enforce_namespace_storage_limit boolean DEFAULT false NOT NULL,
     container_registry_delete_tags_service_timeout integer DEFAULT 250 NOT NULL,
-    kroki_url character varying,
-    kroki_enabled boolean,
-    elasticsearch_client_request_timeout integer DEFAULT 0 NOT NULL,
     gitpod_enabled boolean DEFAULT false NOT NULL,
     gitpod_url text DEFAULT 'https://gitpod.io/'::text,
+    elasticsearch_client_request_timeout integer DEFAULT 0 NOT NULL,
     abuse_notification_email character varying,
     require_admin_approval_after_user_signup boolean DEFAULT true NOT NULL,
     help_page_documentation_base_url text,
     automatic_purchased_storage_allocation boolean DEFAULT false NOT NULL,
+    container_registry_expiration_policies_worker_capacity integer DEFAULT 0 NOT NULL,
     encrypted_ci_jwt_signing_key text,
     encrypted_ci_jwt_signing_key_iv text,
-    container_registry_expiration_policies_worker_capacity integer DEFAULT 0 NOT NULL,
-    elasticsearch_analyzers_smartcn_enabled boolean DEFAULT false NOT NULL,
-    elasticsearch_analyzers_smartcn_search boolean DEFAULT false NOT NULL,
-    elasticsearch_analyzers_kuromoji_enabled boolean DEFAULT false NOT NULL,
-    elasticsearch_analyzers_kuromoji_search boolean DEFAULT false NOT NULL,
     secret_detection_token_revocation_enabled boolean DEFAULT false NOT NULL,
     secret_detection_token_revocation_url text,
     encrypted_secret_detection_token_revocation_token text,
     encrypted_secret_detection_token_revocation_token_iv text,
+    elasticsearch_analyzers_smartcn_enabled boolean DEFAULT false NOT NULL,
+    elasticsearch_analyzers_smartcn_search boolean DEFAULT false NOT NULL,
+    elasticsearch_analyzers_kuromoji_enabled boolean DEFAULT false NOT NULL,
+    elasticsearch_analyzers_kuromoji_search boolean DEFAULT false NOT NULL,
+    new_user_signups_cap integer,
     domain_denylist_enabled boolean DEFAULT false,
     domain_denylist text,
     domain_allowlist text,
-    new_user_signups_cap integer,
     encrypted_cloud_license_auth_token text,
     encrypted_cloud_license_auth_token_iv text,
     secret_detection_revocation_token_types_url text,
     cloud_license_enabled boolean DEFAULT false NOT NULL,
+    kroki_url text,
+    kroki_enabled boolean DEFAULT false NOT NULL,
     disable_feed_token boolean DEFAULT false NOT NULL,
     personal_access_token_prefix text,
     rate_limiting_response_text text,
-    invisible_captcha_enabled boolean DEFAULT false NOT NULL,
     container_registry_cleanup_tags_service_max_list_size integer DEFAULT 200 NOT NULL,
+    invisible_captcha_enabled boolean DEFAULT false NOT NULL,
     enforce_ssh_key_expiration boolean DEFAULT false NOT NULL,
     git_two_factor_session_expiry integer DEFAULT 15 NOT NULL,
     asset_proxy_allowlist text,
@@ -9401,7 +9401,7 @@ CREATE TABLE application_settings (
     kroki_formats jsonb DEFAULT '{}'::jsonb NOT NULL,
     CONSTRAINT app_settings_container_reg_cleanup_tags_max_list_size_positive CHECK ((container_registry_cleanup_tags_service_max_list_size >= 0)),
     CONSTRAINT app_settings_registry_exp_policies_worker_capacity_positive CHECK ((container_registry_expiration_policies_worker_capacity >= 0)),
-    CONSTRAINT check_17d9558205 CHECK ((char_length((kroki_url)::text) <= 1024)),
+    CONSTRAINT check_17d9558205 CHECK ((char_length(kroki_url) <= 1024)),
     CONSTRAINT check_2dba05b802 CHECK ((char_length(gitpod_url) <= 255)),
     CONSTRAINT check_51700b31b5 CHECK ((char_length(default_branch_name) <= 255)),
     CONSTRAINT check_57123c9593 CHECK ((char_length(help_page_documentation_base_url) <= 255)),
@@ -11627,6 +11627,7 @@ CREATE TABLE custom_emoji (
     name text NOT NULL,
     file text NOT NULL,
     external boolean DEFAULT true NOT NULL,
+    creator_id bigint NOT NULL,
     CONSTRAINT check_8c586dd507 CHECK ((char_length(name) <= 36)),
     CONSTRAINT check_dd5d60f1fb CHECK ((char_length(file) <= 255))
 );
@@ -21619,8 +21620,6 @@ CREATE INDEX index_boards_epic_boards_on_group_id ON boards_epic_boards USING bt
 
 CREATE INDEX index_boards_epic_list_user_preferences_on_epic_list_id ON boards_epic_list_user_preferences USING btree (epic_list_id);
 
-CREATE INDEX index_boards_epic_list_user_preferences_on_user_id ON boards_epic_list_user_preferences USING btree (user_id);
-
 CREATE INDEX index_boards_epic_lists_on_epic_board_id ON boards_epic_lists USING btree (epic_board_id);
 
 CREATE UNIQUE INDEX index_boards_epic_lists_on_epic_board_id_and_label_id ON boards_epic_lists USING btree (epic_board_id, label_id) WHERE (list_type = 1);
@@ -24556,6 +24555,9 @@ ALTER TABLE ONLY milestones
 ALTER TABLE ONLY vulnerabilities
     ADD CONSTRAINT fk_959d40ad0a FOREIGN KEY (confirmed_by_id) REFERENCES users(id) ON DELETE SET NULL;
 
+ALTER TABLE ONLY boards_epic_list_user_preferences
+    ADD CONSTRAINT fk_95eac55851 FOREIGN KEY (epic_list_id) REFERENCES boards_epic_lists(id) ON DELETE CASCADE;
+
 ALTER TABLE ONLY application_settings
     ADD CONSTRAINT fk_964370041d FOREIGN KEY (usage_stats_set_by_user_id) REFERENCES users(id) ON DELETE SET NULL;
 
@@ -24853,6 +24855,9 @@ ALTER TABLE ONLY design_management_designs_versions
 ALTER TABLE ONLY analytics_devops_adoption_segments
     ADD CONSTRAINT fk_f5aa768998 FOREIGN KEY (namespace_id) REFERENCES namespaces(id) ON DELETE CASCADE;
 
+ALTER TABLE ONLY boards_epic_list_user_preferences
+    ADD CONSTRAINT fk_f5f2fe5c1f FOREIGN KEY (user_id) REFERENCES users(id) ON DELETE CASCADE;
+
 ALTER TABLE ONLY cluster_agents
     ADD CONSTRAINT fk_f7d43dee13 FOREIGN KEY (created_by_user_id) REFERENCES users(id) ON DELETE SET NULL;
 
@@ -25762,9 +25767,6 @@ ALTER TABLE ONLY packages_debian_project_distributions
 ALTER TABLE ONLY packages_rubygems_metadata
     ADD CONSTRAINT fk_rails_95a3f5ce78 FOREIGN KEY (package_id) REFERENCES packages_packages(id) ON DELETE CASCADE;
 
-ALTER TABLE ONLY boards_epic_list_user_preferences
-    ADD CONSTRAINT fk_rails_95eac55851 FOREIGN KEY (epic_list_id) REFERENCES boards_epic_lists(id) ON DELETE CASCADE;
-
 ALTER TABLE ONLY packages_pypi_metadata
     ADD CONSTRAINT fk_rails_9698717cdd FOREIGN KEY (package_id) REFERENCES packages_packages(id) ON DELETE CASCADE;
 
@@ -26278,9 +26280,6 @@ ALTER TABLE ONLY resource_state_events
 ALTER TABLE ONLY packages_debian_group_components
     ADD CONSTRAINT fk_rails_f5f1ef54c6 FOREIGN KEY (distribution_id) REFERENCES packages_debian_group_distributions(id) ON DELETE CASCADE;
 
-ALTER TABLE ONLY boards_epic_list_user_preferences
-    ADD CONSTRAINT fk_rails_f5f2fe5c1f FOREIGN KEY (user_id) REFERENCES users(id) ON DELETE CASCADE;
-
 ALTER TABLE ONLY incident_management_oncall_shifts
     ADD CONSTRAINT fk_rails_f6eef06841 FOREIGN KEY (participant_id) REFERENCES incident_management_oncall_participants(id) ON DELETE CASCADE;
 

doc/api/graphql/reference/index.md

@@ -525,7 +525,7 @@ Represents a list for an issue board.
 | Field | Type | Description |
 | ----- | ---- | ----------- |
 | `assignee` | User | Assignee in the list. |
-| `collapsed` | Boolean | Indicates if list is collapsed for this user. |
+| `collapsed` | Boolean | Indicates if the list is collapsed for this user. |
 | `id` | ID! | ID (global ID) of the list. |
 | `issues` | IssueConnection | Board issues. |
 | `issuesCount` | Int | Count of issues in the list. |

ee/app/graphql/resolvers/boards/epic_lists_resolver.rb

@@ -22,6 +22,11 @@ module Resolvers
         # point there is not reason to introduce a ListService
         # https://gitlab.com/gitlab-org/gitlab/-/issues/294043
         lists = epic_board.epic_lists
+
+        if load_preferences?(lookahead)
+          ::Boards::EpicList.preload_preferences_for_user(lists, current_user)
+        end
+
         lists = lists.where(id: id.model_id) if id # rubocop: disable CodeReuse/ActiveRecord
 
         offset_pagination(apply_lookahead(lists))

ee/spec/models/boards/epic_list_spec.rb

@@ -4,7 +4,7 @@ require 'spec_helper'
 
 RSpec.describe Boards::EpicList do
   it_behaves_like 'boards listable model', :epic_list
-  it_behaves_like 'list_preferences_for user', :epic_list, :epic_list_id, ::Boards::EpicListUserPreference
+  it_behaves_like 'list_preferences_for user', :epic_list, :epic_list_id
 
   describe 'associations' do
     subject { build(:epic_list) }

ee/spec/models/boards/epic_list_user_preference_spec.rb

@@ -13,6 +13,8 @@ RSpec.describe Boards::EpicListUserPreference do
   describe 'relationships' do
     it { is_expected.to belong_to(:epic_list) }
     it { is_expected.to belong_to(:user) }
+    it { is_expected.to validate_presence_of(:epic_list) }
+    it { is_expected.to validate_presence_of(:user) }
 
     it do
       is_expected.to validate_uniqueness_of(:user_id).scoped_to(:epic_list_id)

ee/spec/requests/api/graphql/boards/epic_board_list_epics_query_spec.rb

@@ -53,28 +53,4 @@ RSpec.describe 'get list of epics for an epic  board list' do
       let(:first_param) { 2 }
     end
   end
-
-  describe 'field values' do
-    def query(params = {})
-      graphql_query_for(:group, { full_path: group.full_path },
-        <<~BOARDS
-          epicBoard(id: "#{board.to_global_id}") {
-            lists(id: "#{list.to_global_id}") {
-              nodes {
-                #{all_graphql_fields_for('epic_list'.classify)}
-              }
-            }
-          }
-        BOARDS
-      )
-    end
-
-    it 'returns the correct values for collapsed' do
-      board.epic_lists.first.update_preferences_for(current_user, collapsed: true)
-
-      post_graphql(query, current_user: current_user)
-
-      expect(graphql_dig_at(graphql_data, 'group', 'epicBoard', 'lists', 'nodes', 0, 'collapsed')).to eq(true)
-    end
-  end
 end

ee/spec/requests/api/graphql/boards/epic_lists_query_spec.rb

@@ -54,5 +54,34 @@ RSpec.describe 'get list of epic boards' do
         let(:first_param) { 2 }
       end
     end
+
+    it 'avoids N+1 queries' do
+      list1.update_preferences_for(current_user, collapsed: true)
+
+      control = ActiveRecord::QueryRecorder.new { post_graphql(pagination_query, current_user: current_user) }
+
+      list2.update_preferences_for(current_user, collapsed: true)
+
+      expect { post_graphql(pagination_query, current_user: current_user) }.not_to exceed_query_limit(control)
+    end
+
+    describe 'field values' do
+      let_it_be(:other_user) { create(:user) }
+
+      it 'returns the correct values for collapsed' do
+        list1.update_preferences_for(current_user, collapsed: true)
+        list1.update_preferences_for(other_user, collapsed: false)
+
+        post_graphql(pagination_query, current_user: current_user)
+
+        # ordered by list_type then position - backlog first and closed last.
+        assert_field_value('id', [global_id_of(list3), global_id_of(list1), global_id_of(list2)])
+        assert_field_value('collapsed', [false, true, false])
+      end
+    end
+  end
+
+  def assert_field_value(field, expected_value)
+    expect(graphql_dig_at(graphql_data, 'group', 'epicBoard', 'lists', 'nodes', field)).to eq(expected_value)
   end
 end

spec/models/list_spec.rb

@@ -5,7 +5,7 @@ require 'spec_helper'
 RSpec.describe List do
   it_behaves_like 'having unique enum values'
   it_behaves_like 'boards listable model', :list
-  it_behaves_like 'list_preferences_for user', :list, :list_id, ListUserPreference
+  it_behaves_like 'list_preferences_for user', :list, :list_id
 
   describe 'relationships' do
     it { is_expected.to belong_to(:board) }

spec/support/shared_examples/models/boards/user_preferences_shared_examples.rb

 # frozen_string_literal: true
 
-RSpec.shared_examples 'list_preferences_for user' do |list_factory, list_id, preference_klass|
+RSpec.shared_examples 'list_preferences_for user' do |list_factory, list_id_attribute|
   subject { create(list_factory) } # rubocop:disable Rails/SaveBang
 
-  let(:user) { create(:user) }
+  let_it_be(:user) { create(:user) }
 
   describe '#preferences_for' do
     context 'when user is nil' do
@@ -11,7 +11,7 @@ RSpec.shared_examples 'list_preferences_for user' do |list_factory, list_id, pre
         preferences = subject.preferences_for(nil)
 
         expect(preferences).not_to be_persisted
-        expect(preferences.send(list_id)).to eq(subject.id)
+        expect(preferences[list_id_attribute]).to eq(subject.id)
         expect(preferences.user_id).to be_nil
       end
     end
@@ -35,7 +35,7 @@ RSpec.shared_examples 'list_preferences_for user' do |list_factory, list_id, pre
 
         expect(preferences).not_to be_persisted
         expect(preferences.user_id).to eq(user.id)
-        expect(preferences.send(list_id)).to eq(subject.id)
+        expect(preferences.public_send(list_id_attribute)).to eq(subject.id)
       end
     end
   end
@@ -44,7 +44,7 @@ RSpec.shared_examples 'list_preferences_for user' do |list_factory, list_id, pre
     context 'when user is present' do
       context 'when there are no preferences for user' do
         it 'creates new user preferences' do
-          expect { subject.update_preferences_for(user, collapsed: true) }.to change { preference_klass.count }.by(1)
+          expect { subject.update_preferences_for(user, collapsed: true) }.to change { subject.preferences.count }.by(1)
           expect(subject.preferences_for(user).collapsed).to eq(true)
         end
       end
@@ -53,14 +53,14 @@ RSpec.shared_examples 'list_preferences_for user' do |list_factory, list_id, pre
         it 'updates user preferences' do
           subject.update_preferences_for(user, collapsed: false)
 
-          expect { subject.update_preferences_for(user, collapsed: true) }.not_to change { preference_klass.count }
+          expect { subject.update_preferences_for(user, collapsed: true) }.not_to change { subject.preferences.count }
           expect(subject.preferences_for(user).collapsed).to eq(true)
         end
       end
 
       context 'when user is nil' do
         it 'does not create user preferences' do
-          expect { subject.update_preferences_for(nil, collapsed: true) }.not_to change { preference_klass.count }
+          expect { subject.update_preferences_for(nil, collapsed: true) }.not_to change { subject.preferences.count }
         end
       end
     end