Merge branch '4794-optimise-close-issue-method' into 'master'

Remove unnecessary query from close_issue method [RUN ALL RSPEC] [RUN AS-IF-FOSS]

See merge request gitlab-org/gitlab!61087

app/models/issue.rb

@@ -176,8 +176,16 @@ class Issue < ApplicationRecord
     state :opened, value: Issue.available_states[:opened]
     state :closed, value: Issue.available_states[:closed]
 
-    before_transition any => :closed do |issue|
+    before_transition any => :closed do |issue, transition|
+      args = transition.args
+
       issue.closed_at = issue.system_note_timestamp
+
+      next if args.empty?
+
+      next unless args.first.is_a?(User)
+
+      issue.closed_by = args.first
     end
 
     before_transition closed: :opened do |issue|

app/services/issues/close_service.rb

@@ -24,8 +24,7 @@ module Issues
         return issue
       end
 
-      if project.issues_enabled? && issue.close
-        issue.update(closed_by: current_user)
+      if project.issues_enabled? && issue.close(current_user)
         event_service.close_issue(issue, current_user)
         create_note(issue, closed_via) if system_note
 

changelogs/unreleased/4794-optimise-close-issue-method.yml

+---
+title: Remove unnecessary query from close_issue method
+merge_request: 61087
+author:
+type: performance

spec/models/issue_spec.rb

@@ -242,16 +242,36 @@ RSpec.describe Issue do
 
       expect { issue.close }.to change { issue.state_id }.from(open_state).to(closed_state)
     end
+
+    context 'when an argument is provided' do
+      context 'and the argument is a User' do
+        it 'changes closed_by to the given user' do
+          expect { issue.close(user) }.to change { issue.closed_by }.from(nil).to(user)
+        end
+      end
+
+      context 'and the argument is a not a User' do
+        it 'does not change closed_by' do
+          expect { issue.close("test") }.not_to change { issue.closed_by }
+        end
+      end
+    end
+
+    context 'when an argument is not provided' do
+      it 'does not change closed_by' do
+        expect { issue.close }.not_to change { issue.closed_by }
+      end
+    end
   end
 
   describe '#reopen' do
     let(:issue) { create(:issue, project: reusable_project, state: 'closed', closed_at: Time.current, closed_by: user) }
 
-    it 'sets closed_at to nil when an issue is reopend' do
+    it 'sets closed_at to nil when an issue is reopened' do
       expect { issue.reopen }.to change { issue.closed_at }.to(nil)
     end
 
-    it 'sets closed_by to nil when an issue is reopend' do
+    it 'sets closed_by to nil when an issue is reopened' do
       expect { issue.reopen }.to change { issue.closed_by }.from(user).to(nil)
     end
 

spec/services/issues/close_service_spec.rb

@@ -91,7 +91,7 @@ RSpec.describe Issues::CloseService do
         end
       end
 
-      context 'with innactive external issue tracker supporting close_issue' do
+      context 'with inactive external issue tracker supporting close_issue' do
         let!(:external_issue_tracker) { create(:jira_service, project: project, active: false) }
 
         it 'does not close the issue on the external issue tracker' do
@@ -220,6 +220,14 @@ RSpec.describe Issues::CloseService do
         end
       end
 
+      it 'verifies the number of queries' do
+        recorded = ActiveRecord::QueryRecorder.new { close_issue }
+        expected_queries = 23
+
+        expect(recorded.count).to be <= expected_queries
+        expect(recorded.cached_count).to eq(0)
+      end
+
       it 'closes the issue' do
         close_issue
 
@@ -230,7 +238,7 @@ RSpec.describe Issues::CloseService do
       it 'records closed user' do
         close_issue
 
-        expect(issue.closed_by_id).to be(user.id)
+        expect(issue.reload.closed_by_id).to be(user.id)
       end
 
       it 'sends email to user2 about assign of new issue', :sidekiq_might_not_need_inline do
@@ -254,6 +262,16 @@ RSpec.describe Issues::CloseService do
         expect(todo.reload).to be_done
       end
 
+      context 'when closing the issue fails' do
+        it 'does not assign a closed_by value for the issue' do
+          allow(issue).to receive(:close).and_return(false)
+
+          close_issue
+
+          expect(issue.closed_by_id).to be_nil
+        end
+      end
+
       context 'when there is an associated Alert Management Alert' do
         context 'when alert can be resolved' do
           let!(:alert) { create(:alert_management_alert, issue: issue, project: project) }