Cache milestone issue counts

Add milestone close issues count service
Add milestone issue count service
Use count issue services in milestoneish
Clear milestone issue counters cache on issue creation
Clear milestone closed issue counters cache
Clear milestone issue counters cache on updating issue
Clear milestone issue counters on project transfer
Add a changelog for milestones page improvement
Fix milestones issue count cache keys

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