Merge branch 'issue_237977-refresh_blocking_issues_cache_after_reopen' into 'master'

Fix blocking issues count cache refresh

See merge request gitlab-org/gitlab!46585

app/services/issues/reopen_service.rb

@@ -5,8 +5,6 @@ module Issues
     def execute(issue)
       return issue unless can?(current_user, :reopen_issue, issue)
 
-      before_reopen(issue)
-
       if issue.reopen
         event_service.reopen_issue(issue, current_user)
         create_note(issue, 'reopened')
@@ -23,14 +21,8 @@ module Issues
 
     private
 
-    def before_reopen(issue)
-      # Overriden in EE
-    end
-
     def create_note(issue, state = issue.state)
       SystemNoteService.change_status(issue, issue.project, current_user, state, nil)
     end
   end
 end
-
-Issues::ReopenService.prepend_if_ee('EE::Issues::ReopenService')

ee/app/models/ee/issue.rb

@@ -66,6 +66,12 @@ module EE
       validate :validate_confidential_epic
 
       after_create :update_generic_alert_title, if: :generic_alert_with_default_title?
+
+      state_machine :state_id do
+        after_transition do |issue|
+          issue.refresh_blocking_and_blocked_issues_cache!
+        end
+      end
     end
 
     class_methods do
@@ -88,8 +94,12 @@ module EE
       blocking_issues_ids.any?
     end
 
+    def blocked_by_issues
+      self.class.where(id: blocking_issues_ids)
+    end
+
     # Used on EE::IssueEntity to expose blocking issues URLs
-    def blocked_by_issues(user)
+    def blocked_by_issues_for(user)
       return ::Issue.none unless blocked?
 
       issues =
@@ -212,6 +222,18 @@ module EE
       update!(blocking_issues_count: blocking_count)
     end
 
+    def refresh_blocking_and_blocked_issues_cache!
+      self_and_blocking_issues_ids = [self.id] + blocking_issues_ids
+      blocking_issues_count_by_id = ::IssueLink.blocking_issues_for_collection(self_and_blocking_issues_ids).to_sql
+
+      self.class.connection.execute <<~SQL
+        UPDATE issues
+        SET blocking_issues_count = grouped_counts.count
+        FROM (#{blocking_issues_count_by_id}) AS grouped_counts
+        WHERE issues.id = grouped_counts.blocking_issue_id
+      SQL
+    end
+
     override :relocation_target
     def relocation_target
       super || promoted_to_epic

ee/app/models/ee/issue_link.rb

@@ -52,16 +52,16 @@ module EE
       end
 
       def blocking_issues_for_collection(issues_ids)
+        open_state_id = ::Issuable::STATE_ID_MAP[:opened]
+
         from_union([
-          select('COUNT(*), issue_links.source_id AS blocking_issue_id')
+          select("COUNT(CASE WHEN issues.state_id = #{open_state_id} then 1 else null end), issue_links.source_id AS blocking_issue_id")
             .joins(:target)
-            .where(issues: { state_id: ::Issue.available_states[:opened] })
             .where(link_type: ::IssueLink::TYPE_BLOCKS)
             .where(source_id: issues_ids)
             .group(:blocking_issue_id),
-          select('COUNT(*), issue_links.target_id AS blocking_issue_id')
+          select("COUNT(CASE WHEN issues.state_id = #{open_state_id} then 1 else null end), issue_links.target_id AS blocking_issue_id")
             .joins(:source)
-            .where(issues: { state_id: ::Issue.available_states[:opened] })
             .where(link_type: ::IssueLink::TYPE_IS_BLOCKED_BY)
             .where(target_id: issues_ids)
             .group(:blocking_issue_id)

ee/app/serializers/ee/issue_entity.rb

@@ -10,7 +10,7 @@ module EE
         expose :blocked?, as: :blocked
 
         expose :blocked_by_issues do |issue|
-          issues = issue.blocked_by_issues(request.current_user)
+          issues = issue.blocked_by_issues_for(request.current_user)
           serializer_options = options.merge(only: [:iid, :web_url])
 
           ::IssueEntity.represent(issues, serializer_options)

ee/app/services/ee/issues/reopen_service.rb

-# frozen_string_literal: true
-
-module EE
-  module Issues
-    module ReopenService
-      extend ::Gitlab::Utils::Override
-
-      override :before_reopen
-      def before_reopen(issue)
-        # Assign blocking_issues_count to issue object instead of performing an update,
-        # this way we can keep issue#previous_changes attributes consistent.
-        # Some services may use them to perform callbacks like StatusPage::TriggerPublishService
-        issue.blocking_issues_count = ::IssueLink.blocking_issues_count_for(issue)
-
-        super
-      end
-    end
-  end
-end

ee/app/views/projects/issues/_alert_blocked.html.haml

-- blocked_by_issues = @issue.blocked_by_issues(current_user)
+- blocked_by_issues = @issue.blocked_by_issues_for(current_user)
 - blocked_by_issues_links = blocked_by_issues.map { |blocking_issue| link_to "\##{blocking_issue.iid}", project_issue_path(blocking_issue.project, blocking_issue), class: 'gl-link' }.join(', ').html_safe
 
-- if @issue.blocked? && @issue.blocked_by_issues(current_user).length > 0
+- if @issue.blocked? && @issue.blocked_by_issues_for(current_user).length > 0
   .hidden.js-close-blocked-issue-warning.js-issuable-buttons.gl-alert.gl-alert-warning.gl-mt-5{ role: 'alert', data: { 'action': "close-reopen" } }
     = sprite_icon('warning', css_class: 'gl-icon gl-alert-icon')
     %h4.gl-alert-title

ee/changelogs/unreleased/issue_237977-refresh_blocking_issues_cache_after_reopen.yml

+---
+title: Fix blocking issues count cache when changing issue state
+merge_request: 46585
+author:
+type: fixed

ee/spec/models/issue_spec.rb

@@ -751,13 +751,13 @@ RSpec.describe Issue do
         project.add_developer(user)
         other_project_blocking_issue.project.add_developer(user)
 
-        expect(issue.blocked_by_issues(user)).to match_array([blocking_issue, blocked_by_issue, other_project_blocking_issue, confidential_blocked_by_issue])
+        expect(issue.blocked_by_issues_for(user)).to match_array([blocking_issue, blocked_by_issue, other_project_blocking_issue, confidential_blocked_by_issue])
       end
     end
 
     context 'when user cannot read issues' do
       it 'returns empty array' do
-        expect(issue.blocked_by_issues(user)).to be_empty
+        expect(issue.blocked_by_issues_for(user)).to be_empty
       end
     end
 
@@ -766,7 +766,7 @@ RSpec.describe Issue do
         guest = create(:user)
         project.add_guest(guest)
 
-        expect(issue.blocked_by_issues(guest)).to match_array([blocking_issue, blocked_by_issue])
+        expect(issue.blocked_by_issues_for(guest)).to match_array([blocking_issue, blocked_by_issue])
       end
     end
   end
@@ -835,6 +835,51 @@ RSpec.describe Issue do
     end
   end
 
+  context 'when changing state of blocking issues' do
+    let_it_be(:project) { create(:project) }
+    let_it_be(:blocking_issue1) { create(:issue, project: project) }
+    let_it_be(:blocking_issue2) { create(:issue, project: project) }
+    let_it_be(:blocked_issue) { create(:issue, project: project) }
+    let_it_be(:blocked_by_blocked_issue) { create(:issue, project: project) }
+
+    before_all do
+      create(:issue_link, source: blocking_issue1, target: blocked_issue, link_type: IssueLink::TYPE_BLOCKS)
+      create(:issue_link, source: blocking_issue2, target: blocked_issue, link_type: IssueLink::TYPE_BLOCKS)
+      create(:issue_link, source: blocked_issue, target: blocked_by_blocked_issue, link_type: IssueLink::TYPE_BLOCKS)
+    end
+
+    before do
+      blocked_issue.update(blocking_issues_count: 0)
+    end
+
+    context 'when blocked issue is closed' do
+      it 'updates blocking and blocked issues cache' do
+        blocked_issue.close
+
+        expect(blocking_issue1.reload.blocking_issues_count).to eq(0)
+        expect(blocking_issue2.reload.blocking_issues_count).to eq(0)
+        expect(blocked_issue.reload.blocking_issues_count).to eq(1)
+      end
+    end
+
+    context 'when blocked issue is reopened' do
+      before do
+        blocked_issue.close
+        blocked_issue.update(blocking_issues_count: 0)
+        blocking_issue1.update(blocking_issues_count: 0)
+        blocking_issue2.update(blocking_issues_count: 0)
+      end
+
+      it 'updates blocking and blocked issues cache' do
+        blocked_issue.reopen
+
+        expect(blocking_issue1.reload.blocking_issues_count).to eq(1)
+        expect(blocking_issue2.reload.blocking_issues_count).to eq(1)
+        expect(blocked_issue.reload.blocking_issues_count).to eq(1)
+      end
+    end
+  end
+
   describe '#supports_iterations?' do
     let(:group) { build_stubbed(:group) }
     let(:project_with_group) { build_stubbed(:project, group: group) }

ee/spec/services/ee/issues/reopen_service_spec.rb

-# frozen_string_literal: true
-
-require 'spec_helper'
-
-RSpec.describe Issues::ReopenService do
-  let_it_be(:project) { create(:project) }
-  let_it_be(:user) { create(:user) }
-  let_it_be(:issue) { create(:issue, :closed, project: project) }
-  let_it_be(:blocked_issue) { create(:issue, project: project) }
-
-  subject { described_class.new(project, user).execute(issue) }
-
-  before do
-    create(:issue_link, source: issue, target: blocked_issue, link_type: ::IssueLink::TYPE_BLOCKS)
-    issue.update!(blocking_issues_count: 0)
-  end
-
-  describe '#execute' do
-    context 'when user is not authorized to reopen issue' do
-      before do
-        project.add_guest(user)
-      end
-
-      it 'does not update blocking issues count' do
-        expect { subject }.not_to change { issue.blocking_issues_count }.from(0)
-      end
-    end
-
-    context 'when user is authorized to reopen issue' do
-      before do
-        project.add_maintainer(user)
-      end
-
-      it 'updates blocking issues count' do
-        expect { subject }.to change { issue.blocking_issues_count }.from(0).to(1)
-      end
-    end
-  end
-end