Merge branch '325598-cache-mr-count-on-milestone-page' into 'master'

Cache MRs count on milestone list page as well as simplify queries

See merge request gitlab-org/gitlab!57714

app/models/concerns/milestoneish.rb

@@ -15,6 +15,10 @@ module Milestoneish
     total_issues_count - closed_issues_count
   end
 
+  def total_merge_requests_count
+    @total_merge_request_count ||= Milestones::MergeRequestsCountService.new(self).count
+  end
+
   def complete?
     total_issues_count > 0 && total_issues_count == closed_issues_count
   end

app/services/merge_requests/after_create_service.rb

@@ -30,6 +30,8 @@ module MergeRequests
 
       Gitlab::UsageDataCounters::MergeRequestCounter.count(:create)
       link_lfs_objects(merge_request)
+
+      delete_milestone_total_merge_requests_counter_cache(merge_request.milestone)
     end
 
     def link_lfs_objects(merge_request)

app/services/merge_requests/base_service.rb

@@ -195,6 +195,12 @@ module MergeRequests
 
       merge_request.update(merge_error: message) if save_message_on_model
     end
+
+    def delete_milestone_total_merge_requests_counter_cache(milestone)
+      return unless milestone
+
+      Milestones::MergeRequestsCountService.new(milestone).delete_cache
+    end
   end
 end
 

app/services/merge_requests/update_service.rb

@@ -200,10 +200,15 @@ module MergeRequests
 
       merge_request_activity_counter.track_milestone_changed_action(user: current_user)
 
+      previous_milestone = Milestone.find_by_id(merge_request.previous_changes['milestone_id'].first)
+      delete_milestone_total_merge_requests_counter_cache(previous_milestone)
+
       if merge_request.milestone.nil?
         notification_service.async.removed_milestone_merge_request(merge_request, current_user)
       else
         notification_service.async.changed_milestone_merge_request(merge_request, merge_request.milestone, current_user)
+
+        delete_milestone_total_merge_requests_counter_cache(merge_request.milestone)
       end
     end
 

app/services/milestones/merge_requests_count_service.rb

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

app/services/milestones/transfer_service.rb

@@ -24,6 +24,9 @@ module Milestones
 
           update_issues_milestone(milestone, new_milestone)
           update_merge_requests_milestone(milestone.id, new_milestone&.id)
+
+          delete_milestone_counts_caches(milestone)
+          delete_milestone_counts_caches(new_milestone)
         end
       end
     end
@@ -71,9 +74,6 @@ module Milestones
     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
 
@@ -84,11 +84,12 @@ module Milestones
     end
     # rubocop: enable CodeReuse/ActiveRecord
 
-    def delete_milestone_issues_caches(milestone)
+    def delete_milestone_counts_caches(milestone)
       return unless milestone
 
       Milestones::IssuesCountService.new(milestone).delete_cache
       Milestones::ClosedIssuesCountService.new(milestone).delete_cache
+      Milestones::MergeRequestsCountService.new(milestone).delete_cache
     end
   end
 end

app/views/shared/milestones/_milestone.html.haml

@@ -44,7 +44,7 @@
       = link_to pluralize(milestone.total_issues_count, _('Issue')), issues_path
       - if milestone.merge_requests_enabled?
         ·
-        = link_to pluralize(milestone.merge_requests_visible_to_user(current_user).size, _('Merge request')), merge_requests_path
+        = link_to pluralize(milestone.total_merge_requests_count, _('Merge request')), merge_requests_path
       .float-lg-right.light #{milestone.percent_complete}% complete
     .col-sm-2
       .milestone-actions.d-flex.justify-content-sm-start.justify-content-md-end

changelogs/unreleased/cache-mr-count-on-milestone-page.yml

+---
+title: Cache MRs count on milestone page
+merge_request: 57714
+author:
+type: performance

spec/models/concerns/milestoneish_spec.rb

@@ -262,6 +262,12 @@ RSpec.describe Milestone, 'Milestoneish' do
     end
   end
 
+  describe '#total_merge_requests_count' do
+    it 'counts merge requests' do
+      expect(milestone.total_merge_requests_count).to eq 1
+    end
+  end
+
   describe '#remaining_days' do
     it 'shows 0 if no due date' do
       milestone = build_stubbed(:milestone)

spec/services/merge_requests/after_create_service_spec.rb

@@ -100,6 +100,22 @@ RSpec.describe MergeRequests::AfterCreateService do
       expect { execute_service }.to change { counter.read(:create) }.by(1)
     end
 
+    context 'with a milestone' do
+      let(:milestone) { create(:milestone, project: merge_request.target_project) }
+
+      before do
+        merge_request.update!(milestone_id: milestone.id)
+      end
+
+      it 'deletes the cache key for milestone merge request counter', :use_clean_rails_memory_store_caching do
+        expect_next_instance_of(Milestones::MergeRequestsCountService, milestone) do |service|
+          expect(service).to receive(:delete_cache).and_call_original
+        end
+
+        execute_service
+      end
+    end
+
     context 'todos' do
       it 'does not creates todos' do
         attributes = {

spec/services/merge_requests/update_service_spec.rb

@@ -272,6 +272,41 @@ RSpec.describe MergeRequests::UpdateService, :mailer do
 
           it_behaves_like 'updates milestone'
         end
+
+        context 'milestone counters cache reset' do
+          let(:milestone_old) { create(:milestone, project: project) }
+          let(:opts) { { milestone: milestone_old } }
+
+          it 'deletes milestone counters' do
+            expect_next_instance_of(Milestones::MergeRequestsCountService, milestone_old) do |service|
+              expect(service).to receive(:delete_cache).and_call_original
+            end
+
+            expect_next_instance_of(Milestones::MergeRequestsCountService, milestone) do |service|
+              expect(service).to receive(:delete_cache).and_call_original
+            end
+
+            update_merge_request(milestone: milestone)
+          end
+
+          it 'deletes milestone counters when the milestone is removed' do
+            expect_next_instance_of(Milestones::MergeRequestsCountService, milestone_old) do |service|
+              expect(service).to receive(:delete_cache).and_call_original
+            end
+
+            update_merge_request(milestone: nil)
+          end
+
+          it 'deletes milestone counters when the milestone was not set' do
+            update_merge_request(milestone: nil)
+
+            expect_next_instance_of(Milestones::MergeRequestsCountService, milestone) do |service|
+              expect(service).to receive(:delete_cache).and_call_original
+            end
+
+            update_merge_request(milestone: milestone)
+          end
+        end
       end
 
       it 'executes hooks with update action' do

spec/services/milestones/merge_requests_count_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Milestones::MergeRequestsCountService, :use_clean_rails_memory_store_caching do
+  let_it_be(:project) { create(:project, :empty_repo) }
+  let_it_be(:milestone) { create(:milestone, project: project) }
+
+  before_all do
+    create(:merge_request, milestone: milestone, source_project: project)
+    create(:merge_request, :closed, milestone: milestone, source_project: project)
+  end
+
+  subject { described_class.new(milestone) }
+
+  it_behaves_like 'a counter caching service'
+
+  it 'counts all merge requests' do
+    expect(subject.count).to eq(2)
+  end
+end

spec/services/milestones/transfer_service_spec.rb

@@ -50,7 +50,7 @@ RSpec.describe Milestones::TransferService do
           end
         end
 
-        it 'deletes milestone issue counters cache for both milestones' do
+        it 'deletes milestone 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|
@@ -59,12 +59,18 @@ RSpec.describe Milestones::TransferService do
           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::MergeRequestsCountService, 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
+          expect_next_instance_of(Milestones::MergeRequestsCountService, new_milestone) do |service|
+            expect(service).to receive(:delete_cache).and_call_original
+          end
 
           service.execute
         end