Merge branch '21068-unrevert-optimize-issuable-updates' into 'master'

Reinstate "Optimize issuable updates"

See merge request gitlab-org/gitlab!59468

app/models/concerns/issuable.rb

@@ -137,6 +137,14 @@ module Issuable
     scope :references_project, -> { references(:project) }
     scope :non_archived, -> { join_project.where(projects: { archived: false }) }
 
+    scope :includes_for_bulk_update, -> do
+      associations = %i[author assignees epic group labels metrics project source_project target_project].select do |association|
+        reflect_on_association(association)
+      end
+
+      includes(*associations)
+    end
+
     attr_mentionable :title, pipeline: :single_line
     attr_mentionable :description
 

app/models/concerns/milestoneable.rb

@@ -39,11 +39,13 @@ module Milestoneable
     private
 
     def milestone_is_valid
-      errors.add(:milestone_id, 'is invalid') if respond_to?(:milestone_id) && milestone_id.present? && !milestone_available?
+      errors.add(:milestone_id, 'is invalid') if respond_to?(:milestone_id) && !milestone_available?
     end
   end
 
   def milestone_available?
+    return true if milestone_id.blank?
+
     project_id == milestone&.project_id || project.ancestors_upto.compact.include?(milestone&.group)
   end
 

app/services/issuable/bulk_update_service.rb

@@ -15,7 +15,7 @@ module Issuable
       set_update_params(type)
       items = update_issuables(type, ids)
 
-      response_success(payload: { count: items.count })
+      response_success(payload: { count: items.size })
     rescue ArgumentError => e
       response_error(e.message, 422)
     end
@@ -59,10 +59,17 @@ module Issuable
 
     def find_issuables(parent, model_class, ids)
       if parent.is_a?(Project)
-        model_class.id_in(ids).of_projects(parent)
+        projects = parent
       elsif parent.is_a?(Group)
-        model_class.id_in(ids).of_projects(parent.all_projects)
+        projects = parent.all_projects
+      else
+        return
       end
+
+      model_class
+        .id_in(ids)
+        .of_projects(projects)
+        .includes_for_bulk_update
     end
 
     def response_success(message: nil, payload: nil)

changelogs/unreleased/21068-optimize-issueable-updates.yml

+---
+title: Optimize issuable updates
+merge_request: 59468
+author:
+type: performance

ee/app/services/ee/issuable/bulk_update_service.rb

@@ -9,9 +9,12 @@ module EE
 
       override :find_issuables
       def find_issuables(parent, model_class, ids)
-        return model_class.id_in(ids).in_selected_groups(parent.self_and_descendants) if model_class == ::Epic
+        return super unless model_class == ::Epic
 
-        super
+        model_class
+          .id_in(ids)
+          .in_selected_groups(parent.self_and_descendants)
+          .includes_for_bulk_update
       end
 
       override :permitted_attrs

ee/spec/models/concerns/ee/milestoneable_spec.rb

@@ -4,11 +4,11 @@ require 'spec_helper'
 
 RSpec.describe EE::Milestoneable do
   describe '#milestone_available?' do
-    context 'no Epic' do
-      let(:issue) { create(:issue) }
+    context 'for epics' do
+      let(:epic) { build(:epic) }
 
-      it 'returns false' do
-        expect(issue.milestone_available?).to be_falsy
+      it 'returns true' do
+        expect(epic.milestone_available?).to be(true)
       end
     end
   end
@@ -21,7 +21,7 @@ RSpec.describe EE::Milestoneable do
       let(:epic) { build(:epic) }
 
       it 'returns false' do
-        expect(epic.supports_milestone?).to be_falsy
+        expect(epic.supports_milestone?).to be(false)
       end
     end
   end

spec/models/concerns/issuable_spec.rb

@@ -65,6 +65,23 @@ RSpec.describe Issuable do
     it { expect(issuable_class).to respond_to(:opened) }
     it { expect(issuable_class).to respond_to(:closed) }
     it { expect(issuable_class).to respond_to(:assigned) }
+
+    describe '.includes_for_bulk_update' do
+      before do
+        stub_const('Example', Class.new(ActiveRecord::Base))
+
+        Example.class_eval do
+          include Issuable # adds :labels and :metrics, among others
+
+          belongs_to :author
+          has_many :assignees
+        end
+      end
+
+      it 'includes available associations' do
+        expect(Example.includes_for_bulk_update.includes_values).to eq([:author, :assignees, :labels, :metrics])
+      end
+    end
   end
 
   describe 'author_name' do

spec/models/concerns/milestoneable_spec.rb

@@ -50,13 +50,13 @@ RSpec.describe Milestoneable do
       it 'returns true with a milestone from the issue project' do
         milestone = create(:milestone, project: project)
 
-        expect(build_milestoneable(milestone.id).milestone_available?).to be_truthy
+        expect(build_milestoneable(milestone.id).milestone_available?).to be(true)
       end
 
       it 'returns true with a milestone from the issue project group' do
         milestone = create(:milestone, group: group)
 
-        expect(build_milestoneable(milestone.id).milestone_available?).to be_truthy
+        expect(build_milestoneable(milestone.id).milestone_available?).to be(true)
       end
 
       it 'returns true with a milestone from the the parent of the issue project group' do
@@ -64,19 +64,23 @@ RSpec.describe Milestoneable do
         group.update!(parent: parent)
         milestone = create(:milestone, group: parent)
 
-        expect(build_milestoneable(milestone.id).milestone_available?).to be_truthy
+        expect(build_milestoneable(milestone.id).milestone_available?).to be(true)
+      end
+
+      it 'returns true with a blank milestone' do
+        expect(build_milestoneable('').milestone_available?).to be(true)
       end
 
       it 'returns false with a milestone from another project' do
         milestone = create(:milestone)
 
-        expect(build_milestoneable(milestone.id).milestone_available?).to be_falsey
+        expect(build_milestoneable(milestone.id).milestone_available?).to be(false)
       end
 
       it 'returns false with a milestone from another group' do
         milestone = create(:milestone, group: create(:group))
 
-        expect(build_milestoneable(milestone.id).milestone_available?).to be_falsey
+        expect(build_milestoneable(milestone.id).milestone_available?).to be(false)
       end
     end
   end