Make milestones issue counters independent of user

We want to expose milestone issue counts to the API for
https://gitlab.com/gitlab-org/gitlab/issues/31289.

The current milestones page is also very slow -
it has N+1 for the same counters.

We want to cache these counters, but they depend on user,
which makes cache useless.

So this commit removes dependency on `user` variable for
milestone issue counters

app/helpers/milestones_helper.rb

@@ -92,12 +92,12 @@ module MilestonesHelper
   end
 
   def milestone_progress_tooltip_text(milestone)
-    has_issues = milestone.total_issues_count(current_user) > 0
+    has_issues = milestone.total_issues_count > 0
 
     if has_issues
       [
         _('Progress'),
-        _("%{percent}%% complete") % { percent: milestone.percent_complete(current_user) }
+        _("%{percent}%% complete") % { percent: milestone.percent_complete }
       ].join('<br />')
     else
       _('Progress')
@@ -107,7 +107,7 @@ module MilestonesHelper
   def milestone_progress_bar(milestone)
     options = {
       class: 'progress-bar bg-success',
-      style: "width: #{milestone.percent_complete(current_user)}%;"
+      style: "width: #{milestone.percent_complete}%;"
     }
 
     content_tag :div, class: 'progress' do
@@ -151,9 +151,9 @@ module MilestonesHelper
   end
 
   def milestone_issues_tooltip_text(milestone)
-    total = milestone.total_issues_count(current_user)
-    opened = milestone.opened_issues_count(current_user)
-    closed = milestone.closed_issues_count(current_user)
+    total = milestone.total_issues_count
+    opened = milestone.opened_issues_count
+    closed = milestone.closed_issues_count
 
     return _("Issues") if total.zero?
 

app/models/concerns/milestoneish.rb

 # frozen_string_literal: true
 
 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
+  def total_issues_count
+    @total_issues_count ||= Milestones::IssuesCountService.new(self).count
   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
+  def closed_issues_count
+    @close_issues_count ||= Milestones::ClosedIssuesCountService.new(self).count
   end
 
-  def opened_issues_count(user)
-    total_issues_count(user) - closed_issues_count(user)
+  def opened_issues_count
+    total_issues_count - closed_issues_count
   end
 
-  def complete?(user)
-    total_issues_count(user) > 0 && total_issues_count(user) == closed_issues_count(user)
+  def complete?
+    total_issues_count > 0 && total_issues_count == closed_issues_count
   end
 
-  def percent_complete(user)
-    closed_issues_count(user) * 100 / total_issues_count(user)
+  def percent_complete
+    closed_issues_count * 100 / total_issues_count
   rescue ZeroDivisionError
     0
   end
@@ -135,12 +123,6 @@ module Milestoneish
     Gitlab::TimeTrackingFormatter.output(total_issue_time_estimate)
   end
 
-  def count_issues_by_state(user)
-    memoize_per_user(user, :count_issues_by_state) do
-      issues_visible_to_user(user).reorder(nil).group(:state_id).count
-    end
-  end
-
   private
 
   def memoize_per_user(user, method_name)

app/views/projects/milestones/show.html.haml

@@ -8,10 +8,10 @@
 
 = render_if_exists 'shared/milestones/burndown', milestone: @milestone, project: @project
 
-- if can?(current_user, :read_issue, @project) && @milestone.total_issues_count(current_user).zero?
+- if can?(current_user, :read_issue, @project) && @milestone.total_issues_count.zero?
   .alert.alert-success.prepend-top-default
     %span= _('Assign some issues to this milestone.')
-- elsif @milestone.complete?(current_user) && @milestone.active?
+- elsif @milestone.complete? && @milestone.active?
   .alert.alert-success.prepend-top-default
     %span= _('All issues for this milestone are closed. You may close this milestone now.')
 

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

@@ -42,11 +42,11 @@
 
     .col-sm-4.milestone-progress
       = milestone_progress_bar(milestone)
-      = link_to pluralize(milestone.total_issues_count(current_user), 'Issue'), issues_path
+      = 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
-      .float-lg-right.light #{milestone.percent_complete(current_user)}% complete
+      .float-lg-right.light #{milestone.percent_complete}% complete
     .col-sm-2
       .milestone-actions.d-flex.justify-content-sm-start.justify-content-md-end
         - if @project

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

@@ -7,7 +7,7 @@
       %a.gutter-toggle.float-right.js-sidebar-toggle.has-tooltip{ role: "button", href: "#", "aria-label" => "Toggle sidebar", title: sidebar_gutter_tooltip_text, data: { container: 'body', placement: 'left', boundary: 'viewport' } }
         = sidebar_gutter_toggle_icon
       .title.hide-collapsed
-        %strong.bold== #{milestone.percent_complete(current_user)}%
+        %strong.bold== #{milestone.percent_complete}%
         %span.hide-collapsed
           complete
       .value.hide-collapsed
@@ -15,7 +15,7 @@
 
     .block.milestone-progress.hide-expanded
       .sidebar-collapsed-icon.has-tooltip{ title: milestone_progress_tooltip_text(milestone), data: { container: 'body', html: 'true', placement: 'left', boundary: 'viewport' } }
-        %span== #{milestone.percent_complete(current_user)}%
+        %span== #{milestone.percent_complete}%
         = milestone_progress_bar(milestone)
 
     .block.start_date.hide-collapsed

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

@@ -8,7 +8,7 @@
 = render 'shared/milestones/deprecation_message' if is_dynamic_milestone
 = render 'shared/milestones/description', milestone: milestone
 
-- if milestone.complete?(current_user) && milestone.active?
+- if milestone.complete? && milestone.active?
   .alert.alert-success.prepend-top-default
     %span
       = _('All issues for this milestone are closed.')

changelogs/unreleased/31289-show-issue-summary-on-releases-page.yml

+---
+title: Cache milestone issue counters and make them independent of user permissions
+merge_request: 21554
+author:
+type: performance

spec/models/concerns/milestoneish_spec.rb

@@ -213,7 +213,7 @@ describe Milestone, 'Milestoneish' 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
+      expect(milestone.complete?).to eq false
     end
 
     it 'returns true when all items are closed' do
@@ -221,7 +221,7 @@ describe Milestone, 'Milestoneish' do
       security_issue_1.close
       security_issue_2.close
 
-      expect(milestone.complete?(non_member)).to eq true
+      expect(milestone.complete?).to eq true
     end
   end
 
@@ -229,63 +229,19 @@ describe Milestone, 'Milestoneish' do
     context 'division by zero' do
       let(:new_milestone) { build_stubbed(:milestone) }
 
-      it { expect(new_milestone.percent_complete(admin)).to eq(0) }
+      it { expect(new_milestone.percent_complete).to eq(0) }
     end
   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
+  describe '#closed_issues_count' 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)
+      expect(milestone.closed_issues_count).to eq 6
     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
-      end
-
-      it 'does not count confidential issues for project members with guest role' do
-        expect(milestone.closed_issues_count(guest)).to eq 2
-        expect(milestone.total_issues_count(guest)).to eq 3
   end
 
-      it 'counts confidential issues for author' do
-        expect(milestone.closed_issues_count(author)).to eq 4
-        expect(milestone.total_issues_count(author)).to eq 6
-      end
-
-      it 'counts confidential issues for assignee' do
-        expect(milestone.closed_issues_count(assignee)).to eq 4
-        expect(milestone.total_issues_count(assignee)).to eq 6
-      end
-
-      it 'counts confidential issues for project members' do
-        expect(milestone.closed_issues_count(member)).to eq 6
-        expect(milestone.total_issues_count(member)).to eq 9
-      end
-
-      it 'counts confidential issues for admin' do
-        expect(milestone.closed_issues_count(admin)).to eq 6
-        expect(milestone.total_issues_count(admin)).to eq 9
-      end
+  describe '#total_issues_count' do
+    it 'counts all issues including confidential' do
+      expect(milestone.total_issues_count).to eq 9
     end
   end
 

spec/models/milestone_spec.rb

@@ -231,17 +231,17 @@ describe Milestone do
   describe "#percent_complete" do
     it "does not count open issues" do
       milestone.issues << issue
-      expect(milestone.percent_complete(user)).to eq(0)
+      expect(milestone.percent_complete).to eq(0)
     end
 
     it "counts closed issues" do
       issue.close
       milestone.issues << issue
-      expect(milestone.percent_complete(user)).to eq(100)
+      expect(milestone.percent_complete).to eq(100)
     end
 
     it "recovers from dividing by zero" do
-      expect(milestone.percent_complete(user)).to eq(0)
+      expect(milestone.percent_complete).to eq(0)
     end
   end