Merge branch 'fix-pagination-for-resource-milestone-events-api' into 'master'

Fix pagination for resource milestone events api

See merge request gitlab-org/gitlab!33845

app/finders/resource_milestone_event_finder.rb

 # frozen_string_literal: true
 
 class ResourceMilestoneEventFinder
-  include FinderMethods
-
-  MAX_PER_PAGE = 100
-
-  attr_reader :params, :current_user, :eventable
-
-  def initialize(current_user, eventable, params = {})
+  def initialize(current_user, eventable)
     @current_user = current_user
     @eventable = eventable
-    @params = params
   end
 
+  # Returns the ResourceMilestoneEvents of the eventable
+  # visible to the user.
+  #
+  # @return ResourceMilestoneEvent::ActiveRecord_AssociationRelation
   def execute
-    Kaminari.paginate_array(visible_events)
+    eventable.resource_milestone_events.include_relations
+      .where(milestone_id: readable_milestone_ids) # rubocop: disable CodeReuse/ActiveRecord
   end
 
   private
 
-  def visible_events
-    @visible_events ||= visible_to_user(events)
-  end
+  attr_reader :current_user, :eventable
 
-  def events
-    @events ||= eventable.resource_milestone_events.include_relations.page(page).per(per_page)
-  end
+  def readable_milestone_ids
+    readable_milestones = events_milestones.select do |milestone|
+      parent_availabilities[key_for_parent(milestone.parent)]
+    end
 
-  def visible_to_user(events)
-    events.select { |event| visible_for_user?(event) }
+    readable_milestones.map(&:id).uniq
   end
 
-  def visible_for_user?(event)
-    milestone = event_milestones[event.milestone_id]
-    return if milestone.blank?
+  # rubocop: disable CodeReuse/ActiveRecord
+  def events_milestones
+    @events_milestones ||= Milestone.where(id: unique_milestone_ids_from_events)
+                             .includes(:project, :group)
+  end
+  # rubocop: enable CodeReuse/ActiveRecord
 
-    parent = milestone.parent
-    parent_availabilities[key_for_parent(parent)]
+  def relevant_milestone_parents
+    events_milestones.map(&:parent).uniq
   end
 
   def parent_availabilities
-    @parent_availabilities ||= relevant_parents.to_h do |parent|
+    @parent_availabilities ||= relevant_milestone_parents.to_h do |parent|
       [key_for_parent(parent), Ability.allowed?(current_user, :read_milestone, parent)]
     end
   end
 
-  def key_for_parent(parent)
-    "#{parent.class.name}_#{parent.id}"
-  end
-
-  def event_milestones
-    @milestones ||= events.map(&:milestone).uniq.to_h do |milestone|
-      [milestone.id, milestone]
-    end
-  end
-
-  def relevant_parents
-    @relevant_parents ||= event_milestones.map { |_id, milestone| milestone.parent }
+  # rubocop: disable CodeReuse/ActiveRecord
+  def unique_milestone_ids_from_events
+    eventable.resource_milestone_events.select(:milestone_id).distinct
   end
+  # rubocop: enable CodeReuse/ActiveRecord
 
-  def per_page
-    [params[:per_page], MAX_PER_PAGE].compact.min
-  end
-
-  def page
-    params[:page] || 1
+  def key_for_parent(parent)
+    "#{parent.class.name}_#{parent.id}"
   end
 end

changelogs/unreleased/fix-pagination-for-resource-milestone-events-api.yml

+---
+title: Fix pagination for resource milestone events api
+merge_request: 33845
+author:
+type: fixed

lib/api/resource_milestone_events.rb

@@ -26,8 +26,7 @@ module API
         get ":id/#{eventables_str}/:eventable_id/resource_milestone_events" do
           eventable = find_noteable(eventable_type, params[:eventable_id])
 
-          opts = { page: params[:page], per_page: params[:per_page] }
-          events = ResourceMilestoneEventFinder.new(current_user, eventable, opts).execute
+          events = ResourceMilestoneEventFinder.new(current_user, eventable).execute
 
           present paginate(events), with: Entities::ResourceMilestoneEvent
         end

spec/finders/resource_milestone_event_finder_spec.rb

@@ -42,18 +42,6 @@ RSpec.describe ResourceMilestoneEventFinder do
       expect(subject).to be_empty
     end
 
-    it 'paginates results' do
-      milestone = create(:milestone, project: issue_project)
-      create_event(milestone)
-      create_event(milestone)
-      issue_project.add_guest(user)
-
-      paginated = described_class.new(user, issue, per_page: 1).execute
-
-      expect(subject.count).to eq 2
-      expect(paginated.count).to eq 1
-    end
-
     context 'when multiple events share the same milestone' do
       it 'avoids N+1 queries' do
         issue_project.add_developer(user)
@@ -71,8 +59,8 @@ RSpec.describe ResourceMilestoneEventFinder do
         create_event(milestone2, :add)
         create_event(milestone2, :remove)
 
-        # 1 events + 1 milestones + 1 project + 1 user + 4 ability
-        expect { described_class.new(user, issue).execute }.not_to exceed_query_limit(control_count + 7)
+        # 1 milestones + 1 project + 1 user + 4 ability
+        expect { described_class.new(user, issue).execute }.not_to exceed_query_limit(control_count + 6)
       end
     end
 

spec/support/shared_examples/requests/api/resource_milestone_events_api_shared_examples.rb

@@ -16,6 +16,29 @@ RSpec.shared_examples 'resource_milestone_events API' do |parent_type, eventable
       expect(json_response.first['action']).to eq(event.action)
     end
 
+    context 'when there is an event with a milestone which is not visible for requesting user' do
+      let!(:private_project) { create(:project, :private) }
+      let!(:private_milestone) { create(:milestone, project: private_project) }
+
+      let!(:other_user) { create(:user) }
+
+      it 'returns the expected events' do
+        create_event(private_milestone)
+
+        get api("/#{parent_type}/#{parent.id}/#{eventable_type}/#{eventable[id_name]}/resource_milestone_events", other_user)
+
+        expect(response).to have_gitlab_http_status(:ok)
+        expect(response).to include_pagination_headers
+        expect(response.headers['X-Total']).to eq('1')
+        expect(json_response).to be_an Array
+        expect(json_response.size).to eq(1)
+
+        expect(json_response.first['id']).to eq(event.id)
+        expect(json_response.first['milestone']['id']).to eq(event.milestone.id)
+        expect(json_response.first['action']).to eq(event.action)
+      end
+    end
+
     it "returns a 404 error when eventable id not found" do
       get api("/#{parent_type}/#{parent.id}/#{eventable_type}/#{non_existing_record_id}/resource_milestone_events", user)
 
@@ -60,6 +83,20 @@ RSpec.shared_examples 'resource_milestone_events API' do |parent_type, eventable
     end
   end
 
+  describe 'pagination' do
+    let!(:event1) { create_event(milestone) }
+    let!(:event2) { create_event(milestone) }
+
+    # https://gitlab.com/gitlab-org/gitlab/-/issues/220192
+    it 'returns the second page' do
+      get api("/#{parent_type}/#{parent.id}/#{eventable_type}/#{eventable[id_name]}/resource_milestone_events?page=2&per_page=1", user)
+
+      expect(response).to have_gitlab_http_status(:ok)
+      expect(json_response.count).to eq(1)
+      expect(json_response.first['id']).to eq(event2.id)
+    end
+  end
+
   def create_event(milestone, action: :add)
     create(:resource_milestone_event, eventable.class.name.underscore => eventable, milestone: milestone, action: action)
   end