Merge branch 'revert-e421e0c1' into 'master'

Revert "Merge branch '21068-optimize-issueable-updates' into 'master'"

See merge request gitlab-org/gitlab!59355

app/models/concerns/issuable.rb

@@ -137,14 +137,6 @@ 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,13 +39,11 @@ module Milestoneable
     private
 
     def milestone_is_valid
-      errors.add(:milestone_id, 'is invalid') if respond_to?(:milestone_id) && !milestone_available?
+      errors.add(:milestone_id, 'is invalid') if respond_to?(:milestone_id) && milestone_id.present? && !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.size })
+      response_success(payload: { count: items.count })
     rescue ArgumentError => e
       response_error(e.message, 422)
     end
@@ -59,17 +59,10 @@ module Issuable
 
     def find_issuables(parent, model_class, ids)
       if parent.is_a?(Project)
-        projects = parent
+        model_class.id_in(ids).of_projects(parent)
       elsif parent.is_a?(Group)
-        projects = parent.all_projects
-      else
-        return
+        model_class.id_in(ids).of_projects(parent.all_projects)
       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: 58114
-author:
-type: performance

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

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

ee/spec/lib/gitlab/analytics/cycle_analytics/request_params_spec.rb

@@ -204,11 +204,13 @@ RSpec.describe Gitlab::Analytics::CycleAnalytics::RequestParams do
 
     subject { described_class.new(params).to_data_attributes }
 
-    it { expect(subject[:milestone]).to eq('title') }
-    it { expect(subject[:assignees]).to eq('["username1"]') }
-    it { expect(subject[:labels]).to eq('["label1","label2"]') }
-    it { expect(subject[:author]).to eq('author') }
-    it { expect(subject[:stage]).to eq('{"id":1,"title":"Stage #1"}') }
+    it "has the correct attributes" do
+      expect(subject[:milestone]).to eq('title')
+      expect(subject[:assignees]).to eq('["username1"]')
+      expect(subject[:labels]).to eq('["label1","label2"]')
+      expect(subject[:author]).to eq('author')
+      expect(subject[:stage]).to eq(%Q|{"id":#{stage.id},"title":"#{stage.name}"}|)
+    end
   end
 
   describe 'sorting params' do

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 'for epics' do
-      let(:epic) { build(:epic) }
+    context 'no Epic' do
+      let(:issue) { create(:issue) }
 
-      it 'returns true' do
-        expect(epic.milestone_available?).to be(true)
+      it 'returns false' do
+        expect(issue.milestone_available?).to be_falsy
       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(false)
+        expect(epic.supports_milestone?).to be_falsy
       end
     end
   end

spec/models/concerns/issuable_spec.rb

@@ -65,23 +65,6 @@ 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(true)
+        expect(build_milestoneable(milestone.id).milestone_available?).to be_truthy
       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(true)
+        expect(build_milestoneable(milestone.id).milestone_available?).to be_truthy
       end
 
       it 'returns true with a milestone from the the parent of the issue project group' do
@@ -64,23 +64,19 @@ RSpec.describe Milestoneable do
         group.update!(parent: parent)
         milestone = create(:milestone, group: parent)
 
-        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)
+        expect(build_milestoneable(milestone.id).milestone_available?).to be_truthy
       end
 
       it 'returns false with a milestone from another project' do
         milestone = create(:milestone)
 
-        expect(build_milestoneable(milestone.id).milestone_available?).to be(false)
+        expect(build_milestoneable(milestone.id).milestone_available?).to be_falsey
       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(false)
+        expect(build_milestoneable(milestone.id).milestone_available?).to be_falsey
       end
     end
   end