Merge branch '31289-show-issue-summary-on-releases-page-2' into 'master'

cache release issue counters

See merge request gitlab-org/gitlab!24284

app/helpers/milestones_helper.rb

@@ -151,18 +151,20 @@ module MilestonesHelper
   end
 
   def milestone_issues_tooltip_text(milestone)
-    issues = milestone.count_issues_by_state(current_user)
+    total = milestone.total_issues_count(current_user)
+    opened = milestone.opened_issues_count(current_user)
+    closed = milestone.closed_issues_count(current_user)
 
-    return _("Issues") if issues.empty?
+    return _("Issues") if total.zero?
 
     content = []
 
-    if issues["opened"]
-      content << n_("1 open issue", "%{issues} open issues", issues["opened"]) % { issues: issues["opened"] }
+    if opened > 0
+      content << n_("1 open issue", "%{issues} open issues", opened) % { issues: opened }
     end
 
-    if issues["closed"]
-      content << n_("1 closed issue", "%{issues} closed issues", issues["closed"]) % { issues: issues["closed"] }
+    if closed > 0
+      content << n_("1 closed issue", "%{issues} closed issues", closed) % { issues: closed }
     end
 
     content.join('<br />').html_safe

app/models/concerns/milestoneish.rb

@@ -2,14 +2,28 @@
 
 module Milestoneish
   def total_issues_count(user)
+    @total_issues_count ||=
+      if Feature.enabled?(:cached_milestone_issue_counters)
+        Milestones::IssuesCountService.new(self).count
+      else
         count_issues_by_state(user).values.sum
       end
+  end
 
   def closed_issues_count(user)
+    @close_issues_count ||=
+      if Feature.enabled?(:cached_milestone_issue_counters)
+        Milestones::ClosedIssuesCountService.new(self).count
+      else
         closed_state_id = Issue.available_states[:closed]
 
         count_issues_by_state(user)[closed_state_id].to_i
       end
+  end
+
+  def opened_issues_count(user)
+    total_issues_count(user) - closed_issues_count(user)
+  end
 
   def complete?(user)
     total_issues_count(user) > 0 && total_issues_count(user) == closed_issues_count(user)

app/services/issues/base_service.rb

@@ -34,6 +34,18 @@ module Issues
     def update_project_counter_caches?(issue)
       super || issue.confidential_changed?
     end
+
+    def delete_milestone_closed_issue_counter_cache(milestone)
+      return unless milestone
+
+      Milestones::ClosedIssuesCountService.new(milestone).delete_cache
+    end
+
+    def delete_milestone_total_issue_counter_cache(milestone)
+      return unless milestone
+
+      Milestones::IssuesCountService.new(milestone).delete_cache
+    end
   end
 end
 

app/services/issues/close_service.rb

@@ -38,6 +38,8 @@ module Issues
         issue.update_project_counter_caches
 
         store_first_mentioned_in_commit_at(issue, closed_via) if closed_via.is_a?(MergeRequest)
+
+        delete_milestone_closed_issue_counter_cache(issue.milestone)
       end
 
       issue

app/services/issues/create_service.rb

@@ -29,6 +29,7 @@ module Issues
       todo_service.new_issue(issuable, current_user)
       user_agent_detail_service.create
       resolve_discussions_with_issue(issuable)
+      delete_milestone_total_issue_counter_cache(issuable.milestone)
 
       super
     end

app/services/issues/reopen_service.rb

@@ -12,6 +12,7 @@ module Issues
         execute_hooks(issue, 'reopen')
         invalidate_cache_counts(issue, users: issue.assignees)
         issue.update_project_counter_caches
+        delete_milestone_closed_issue_counter_cache(issue.milestone)
       end
 
       issue

app/services/issues/update_service.rb

@@ -115,10 +115,26 @@ module Issues
     end
 
     def handle_milestone_change(issue)
-      return if skip_milestone_email
-
       return unless issue.previous_changes.include?('milestone_id')
 
+      invalidate_milestone_issue_counters(issue)
+      send_milestone_change_notification(issue)
+    end
+
+    def invalidate_milestone_issue_counters(issue)
+      issue.previous_changes['milestone_id'].each do |milestone_id|
+        next unless milestone_id
+
+        milestone = Milestone.find_by_id(milestone_id)
+
+        delete_milestone_closed_issue_counter_cache(milestone)
+        delete_milestone_total_issue_counter_cache(milestone)
+      end
+    end
+
+    def send_milestone_change_notification(issue)
+      return if skip_milestone_email
+
       if issue.milestone.nil?
         notification_service.async.removed_milestone_issue(issue, current_user)
       else

app/services/milestones/closed_issues_count_service.rb

+# frozen_string_literal: true
+
+module Milestones
+  class ClosedIssuesCountService < BaseCountService
+    def initialize(milestone)
+      @milestone = milestone
+    end
+
+    def cache_key
+      "milestone_closed_issues_count_#{@milestone.milestoneish_id}"
+    end
+
+    def relation_for_count
+      @milestone.issues.closed
+    end
+  end
+end

app/services/milestones/issues_count_service.rb

+# frozen_string_literal: true
+
+module Milestones
+  class IssuesCountService < BaseCountService
+    def initialize(milestone)
+      @milestone = milestone
+    end
+
+    def cache_key
+      "milestone_total_issues_count_#{@milestone.milestoneish_id}"
+    end
+
+    def relation_for_count
+      @milestone.issues
+    end
+  end
+end

app/services/milestones/transfer_service.rb

@@ -22,7 +22,7 @@ module Milestones
         milestones_to_transfer.find_each do |milestone|
           new_milestone = find_or_create_milestone(milestone)
 
-          update_issues_milestone(milestone.id, new_milestone&.id)
+          update_issues_milestone(milestone, new_milestone)
           update_merge_requests_milestone(milestone.id, new_milestone&.id)
         end
       end
@@ -68,9 +68,12 @@ module Milestones
     end
 
     # rubocop: disable CodeReuse/ActiveRecord
-    def update_issues_milestone(old_milestone_id, new_milestone_id)
-      Issue.where(project: project, milestone_id: old_milestone_id)
-        .update_all(milestone_id: new_milestone_id)
+    def update_issues_milestone(old_milestone, new_milestone)
+      Issue.where(project: project, milestone_id: old_milestone.id)
+        .update_all(milestone_id: new_milestone&.id)
+
+      delete_milestone_issues_caches(old_milestone)
+      delete_milestone_issues_caches(new_milestone)
     end
     # rubocop: enable CodeReuse/ActiveRecord
 
@@ -80,5 +83,12 @@ module Milestones
         .update_all(milestone_id: new_milestone_id)
     end
     # rubocop: enable CodeReuse/ActiveRecord
+
+    def delete_milestone_issues_caches(milestone)
+      return unless milestone
+
+      Milestones::IssuesCountService.new(milestone).delete_cache
+      Milestones::ClosedIssuesCountService.new(milestone).delete_cache
+    end
   end
 end

spec/models/concerns/milestoneish_spec.rb

@@ -211,20 +211,21 @@ describe Milestone, 'Milestoneish' do
     end
   end
 
-  describe '#complete?' do
+  describe '#complete?', :use_clean_rails_memory_store_caching do
     it 'returns false when has items opened' do
       expect(milestone.complete?(non_member)).to eq false
     end
 
     it 'returns true when all items are closed' do
       issue.close
-      merge_request.close
+      security_issue_1.close
+      security_issue_2.close
 
       expect(milestone.complete?(non_member)).to eq true
     end
   end
 
-  describe '#percent_complete' do
+  describe '#percent_complete', :use_clean_rails_memory_store_caching do
     context 'division by zero' do
       let(:new_milestone) { build_stubbed(:milestone) }
 
@@ -233,6 +234,29 @@ describe Milestone, 'Milestoneish' do
   end
 
   describe '#count_issues_by_state' do
+    describe '#total_issues_count', :use_clean_rails_memory_store_caching do
+      it 'counts all issues including confidential' do
+        expect(milestone.total_issues_count(guest)).to eq 9
+      end
+    end
+
+    describe '#opened_issues_count', :use_clean_rails_memory_store_caching do
+      it 'counts all open issues including confidential' do
+        expect(milestone.opened_issues_count(guest)).to eq 3
+      end
+    end
+
+    describe '#closed_issues_count', :use_clean_rails_memory_store_caching do
+      it 'counts all closed issues including confidential' do
+        expect(milestone.closed_issues_count(guest)).to eq 6
+      end
+    end
+
+    context 'when cached_milestone_issue_counters are disabled' do
+      before do
+        stub_feature_flags(cached_milestone_issue_counters: false)
+      end
+
       it 'does not count confidential issues for non project members' do
         expect(milestone.closed_issues_count(non_member)).to eq 2
         expect(milestone.total_issues_count(non_member)).to eq 3
@@ -263,6 +287,7 @@ describe Milestone, 'Milestoneish' do
         expect(milestone.total_issues_count(admin)).to eq 9
       end
     end
+  end
 
   describe '#remaining_days' do
     it 'shows 0 if no due date' do

spec/services/issues/close_service_spec.rb

@@ -178,35 +178,55 @@ describe Issues::CloseService do
     end
 
     context "valid params" do
-      before do
+      def close_issue
         perform_enqueued_jobs do
           described_class.new(project, user).close_issue(issue)
         end
       end
 
       it 'closes the issue' do
+        close_issue
+
         expect(issue).to be_valid
         expect(issue).to be_closed
       end
 
       it 'records closed user' do
+        close_issue
+
         expect(issue.closed_by_id).to be(user.id)
       end
 
       it 'sends email to user2 about assign of new issue', :sidekiq_might_not_need_inline do
+        close_issue
+
         email = ActionMailer::Base.deliveries.last
         expect(email.to.first).to eq(user2.email)
         expect(email.subject).to include(issue.title)
       end
 
       it 'creates system note about issue reassign' do
+        close_issue
+
         note = issue.notes.last
         expect(note.note).to include "closed"
       end
 
       it 'marks todos as done' do
+        close_issue
+
         expect(todo.reload).to be_done
       end
+
+      it 'deletes milestone issue counters cache' do
+        issue.update(milestone: create(:milestone, project: project))
+
+        expect_next_instance_of(Milestones::ClosedIssuesCountService, issue.milestone) do |service|
+          expect(service).to receive(:delete_cache).and_call_original
+        end
+
+        close_issue
+      end
     end
 
     context 'when issue is not confidential' do

spec/services/issues/create_service_spec.rb

@@ -196,6 +196,14 @@ describe Issues::CreateService do
           end
         end
       end
+
+      it 'deletes milestone issues count cache' do
+        expect_next_instance_of(Milestones::IssuesCountService, milestone) do |service|
+          expect(service).to receive(:delete_cache).and_call_original
+        end
+
+        issue
+      end
     end
 
     context 'issue create service' do

spec/services/issues/reopen_service_spec.rb

@@ -43,6 +43,16 @@ describe Issues::ReopenService do
           .to change { project.open_issues_count }.from(0).to(1)
       end
 
+      it 'deletes milestone issue counters cache' do
+        issue.update(milestone: create(:milestone, project: project))
+
+        expect_next_instance_of(Milestones::ClosedIssuesCountService, issue.milestone) do |service|
+          expect(service).to receive(:delete_cache).and_call_original
+        end
+
+        described_class.new(project, user).execute(issue)
+      end
+
       context 'when issue is not confidential' do
         it 'executes issue hooks' do
           expect(project).to receive(:execute_hooks).with(an_instance_of(Hash), :issue_hooks)

spec/services/issues/update_service_spec.rb

@@ -412,9 +412,24 @@ describe Issues::UpdateService, :mailer do
           should_email(subscriber)
           should_not_email(non_subscriber)
         end
+
+        it 'clears milestone issue counters cache' do
+          issue.milestone = create(:milestone, project: project)
+
+          issue.save
+
+          expect_next_instance_of(Milestones::IssuesCountService, issue.milestone) do |service|
+            expect(service).to receive(:delete_cache).and_call_original
+          end
+          expect_next_instance_of(Milestones::ClosedIssuesCountService, issue.milestone) do |service|
+            expect(service).to receive(:delete_cache).and_call_original
           end
 
-      context 'when the milestone is changed' do
+          update_issue(milestone_id: "")
+        end
+      end
+
+      context 'when the milestone is assigned' do
         before do
           stub_feature_flags(track_resource_milestone_change_events: false)
         end
@@ -444,6 +459,43 @@ describe Issues::UpdateService, :mailer do
           should_email(subscriber)
           should_not_email(non_subscriber)
         end
+
+        it 'deletes issue counters cache for the milestone' do
+          milestone = create(:milestone, project: project)
+
+          expect_next_instance_of(Milestones::IssuesCountService, milestone) do |service|
+            expect(service).to receive(:delete_cache).and_call_original
+          end
+          expect_next_instance_of(Milestones::ClosedIssuesCountService, milestone) do |service|
+            expect(service).to receive(:delete_cache).and_call_original
+          end
+
+          update_issue(milestone: milestone)
+        end
+      end
+
+      context 'when the milestone is changed' do
+        it 'deletes issue counters cache for both milestones' do
+          old_milestone = create(:milestone, project: project)
+          new_milestone = create(:milestone, project: project)
+
+          issue.update!(milestone: old_milestone)
+
+          expect_next_instance_of(Milestones::IssuesCountService, old_milestone) do |service|
+            expect(service).to receive(:delete_cache).and_call_original
+          end
+          expect_next_instance_of(Milestones::ClosedIssuesCountService, old_milestone) do |service|
+            expect(service).to receive(:delete_cache).and_call_original
+          end
+          expect_next_instance_of(Milestones::IssuesCountService, new_milestone) do |service|
+            expect(service).to receive(:delete_cache).and_call_original
+          end
+          expect_next_instance_of(Milestones::ClosedIssuesCountService, new_milestone) do |service|
+            expect(service).to receive(:delete_cache).and_call_original
+          end
+
+          update_issue(milestone: new_milestone)
+        end
       end
 
       context 'when the labels change' do

spec/services/milestones/closed_issues_count_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Milestones::ClosedIssuesCountService, :use_clean_rails_memory_store_caching do
+  let(:project) { create(:project) }
+  let(:milestone) { create(:milestone, project: project) }
+
+  before do
+    create(:issue, milestone: milestone, project: project)
+    create(:issue, :confidential, milestone: milestone, project: project)
+
+    create(:issue, :closed, milestone: milestone, project: project)
+    create(:issue, :closed, :confidential, milestone: milestone, project: project)
+  end
+
+  subject { described_class.new(milestone) }
+
+  it_behaves_like 'a counter caching service'
+
+  it 'counts closed issues including confidential' do
+    expect(subject.count).to eq(2)
+  end
+end

spec/services/milestones/issues_count_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Milestones::IssuesCountService, :use_clean_rails_memory_store_caching do
+  let(:project) { create(:project) }
+  let(:milestone) { create(:milestone, project: project) }
+
+  before do
+    create(:issue, milestone: milestone, project: project)
+    create(:issue, :confidential, milestone: milestone, project: project)
+
+    create(:issue, :closed, milestone: milestone, project: project)
+    create(:issue, :closed, milestone: milestone, project: project)
+  end
+
+  subject { described_class.new(milestone) }
+
+  it_behaves_like 'a counter caching service'
+
+  it 'counts all issues including confidential' do
+    expect(subject.count).to eq(4)
+  end
+end

spec/services/milestones/transfer_service_spec.rb

@@ -40,6 +40,25 @@ describe Milestones::TransferService do
           expect(new_milestone.project_milestone?).to be_truthy
         end
 
+        it 'deletes milestone issue counters cache for both milestones' do
+          new_milestone = create(:milestone, project: project, title: group_milestone.title)
+
+          expect_next_instance_of(Milestones::IssuesCountService, group_milestone) do |service|
+            expect(service).to receive(:delete_cache).and_call_original
+          end
+          expect_next_instance_of(Milestones::ClosedIssuesCountService, group_milestone) do |service|
+            expect(service).to receive(:delete_cache).and_call_original
+          end
+          expect_next_instance_of(Milestones::IssuesCountService, new_milestone) do |service|
+            expect(service).to receive(:delete_cache).and_call_original
+          end
+          expect_next_instance_of(Milestones::ClosedIssuesCountService, new_milestone) do |service|
+            expect(service).to receive(:delete_cache).and_call_original
+          end
+
+          service.execute
+        end
+
         it 'does not apply new project milestone to issues with project milestone' do
           service.execute