Commit 3fade221 authored by Patrick Derichs's avatar Patrick Derichs Committed by Shinya Maeda

Fix pagination for resource milestone events API

Also update specs and refactor ResourceMilestoneEventFinder to be a
service instead.
parent 7292be58
# frozen_string_literal: true # frozen_string_literal: true
class ResourceMilestoneEventFinder class ResourceMilestoneEventFinder
include FinderMethods def initialize(current_user, eventable)
MAX_PER_PAGE = 100
attr_reader :params, :current_user, :eventable
def initialize(current_user, eventable, params = {})
@current_user = current_user @current_user = current_user
@eventable = eventable @eventable = eventable
@params = params
end end
# Returns the ResourceMilestoneEvents of the eventable
# visible to the user.
#
# @return ResourceMilestoneEvent::ActiveRecord_AssociationRelation
def execute def execute
Kaminari.paginate_array(visible_events) eventable.resource_milestone_events.include_relations
.where(milestone_id: readable_milestone_ids) # rubocop: disable CodeReuse/ActiveRecord
end end
private private
def visible_events attr_reader :current_user, :eventable
@visible_events ||= visible_to_user(events)
end
def events def readable_milestone_ids
@events ||= eventable.resource_milestone_events.include_relations.page(page).per(per_page) readable_milestones = events_milestones.select do |milestone|
end parent_availabilities[key_for_parent(milestone.parent)]
end
def visible_to_user(events) readable_milestones.map(&:id).uniq
events.select { |event| visible_for_user?(event) }
end end
def visible_for_user?(event) # rubocop: disable CodeReuse/ActiveRecord
milestone = event_milestones[event.milestone_id] def events_milestones
return if milestone.blank? @events_milestones ||= Milestone.where(id: unique_milestone_ids_from_events)
.includes(:project, :group)
end
# rubocop: enable CodeReuse/ActiveRecord
parent = milestone.parent def relevant_milestone_parents
parent_availabilities[key_for_parent(parent)] events_milestones.map(&:parent).uniq
end end
def parent_availabilities 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)] [key_for_parent(parent), Ability.allowed?(current_user, :read_milestone, parent)]
end end
end end
def key_for_parent(parent) # rubocop: disable CodeReuse/ActiveRecord
"#{parent.class.name}_#{parent.id}" def unique_milestone_ids_from_events
end eventable.resource_milestone_events.select(:milestone_id).distinct
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 }
end end
# rubocop: enable CodeReuse/ActiveRecord
def per_page def key_for_parent(parent)
[params[:per_page], MAX_PER_PAGE].compact.min "#{parent.class.name}_#{parent.id}"
end
def page
params[:page] || 1
end end
end end
---
title: Fix pagination for resource milestone events api
merge_request: 33845
author:
type: fixed
...@@ -26,8 +26,7 @@ module API ...@@ -26,8 +26,7 @@ module API
get ":id/#{eventables_str}/:eventable_id/resource_milestone_events" do get ":id/#{eventables_str}/:eventable_id/resource_milestone_events" do
eventable = find_noteable(eventable_type, params[:eventable_id]) eventable = find_noteable(eventable_type, params[:eventable_id])
opts = { page: params[:page], per_page: params[:per_page] } events = ResourceMilestoneEventFinder.new(current_user, eventable).execute
events = ResourceMilestoneEventFinder.new(current_user, eventable, opts).execute
present paginate(events), with: Entities::ResourceMilestoneEvent present paginate(events), with: Entities::ResourceMilestoneEvent
end end
......
...@@ -42,18 +42,6 @@ RSpec.describe ResourceMilestoneEventFinder do ...@@ -42,18 +42,6 @@ RSpec.describe ResourceMilestoneEventFinder do
expect(subject).to be_empty expect(subject).to be_empty
end 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 context 'when multiple events share the same milestone' do
it 'avoids N+1 queries' do it 'avoids N+1 queries' do
issue_project.add_developer(user) issue_project.add_developer(user)
...@@ -71,8 +59,8 @@ RSpec.describe ResourceMilestoneEventFinder do ...@@ -71,8 +59,8 @@ RSpec.describe ResourceMilestoneEventFinder do
create_event(milestone2, :add) create_event(milestone2, :add)
create_event(milestone2, :remove) create_event(milestone2, :remove)
# 1 events + 1 milestones + 1 project + 1 user + 4 ability # 1 milestones + 1 project + 1 user + 4 ability
expect { described_class.new(user, issue).execute }.not_to exceed_query_limit(control_count + 7) expect { described_class.new(user, issue).execute }.not_to exceed_query_limit(control_count + 6)
end end
end end
......
...@@ -16,6 +16,29 @@ RSpec.shared_examples 'resource_milestone_events API' do |parent_type, eventable ...@@ -16,6 +16,29 @@ RSpec.shared_examples 'resource_milestone_events API' do |parent_type, eventable
expect(json_response.first['action']).to eq(event.action) expect(json_response.first['action']).to eq(event.action)
end 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 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) 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 ...@@ -60,6 +83,20 @@ RSpec.shared_examples 'resource_milestone_events API' do |parent_type, eventable
end end
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) def create_event(milestone, action: :add)
create(:resource_milestone_event, eventable.class.name.underscore => eventable, milestone: milestone, action: action) create(:resource_milestone_event, eventable.class.name.underscore => eventable, milestone: milestone, action: action)
end end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment