Commit 482d6285 authored by Mario de la Ossa's avatar Mario de la Ossa

Paginate profile group notifications

It's an extremely slow page with a hard-to-fix N+1 so let's paginate it
to minimize loading time
parent f91bef1c
# frozen_string_literal: true # frozen_string_literal: true
class Profiles::NotificationsController < Profiles::ApplicationController class Profiles::NotificationsController < Profiles::ApplicationController
NOTIFICATIONS_PER_PAGE = 10
# rubocop: disable CodeReuse/ActiveRecord # rubocop: disable CodeReuse/ActiveRecord
def show def show
@user = current_user @user = current_user
@group_notifications = current_user.notification_settings.preload_source_route.for_groups.order(:id) @user_groups = user_groups
@group_notifications += GroupsFinder.new( @group_notifications = user_groups.map { |group| current_user.notification_settings_for(group, inherit: true) }
current_user,
all_available: false,
exclude_group_ids: @group_notifications.select(:source_id)
).execute.map { |group| current_user.notification_settings_for(group, inherit: true) }
@project_notifications = current_user.notification_settings.for_projects.order(:id) @project_notifications = current_user.notification_settings.for_projects.order(:id)
.preload_source_route .preload_source_route
.select { |notification| current_user.can?(:read_project, notification.source) } .select { |notification| current_user.can?(:read_project, notification.source) }
...@@ -32,4 +31,10 @@ class Profiles::NotificationsController < Profiles::ApplicationController ...@@ -32,4 +31,10 @@ class Profiles::NotificationsController < Profiles::ApplicationController
def user_params def user_params
params.require(:user).permit(:notification_email, :notified_of_own_activity) params.require(:user).permit(:notification_email, :notified_of_own_activity)
end end
private
def user_groups
GroupsFinder.new(current_user).execute.order_name_asc.page(params[:page]).per(NOTIFICATIONS_PER_PAGE)
end
end end
...@@ -39,10 +39,11 @@ ...@@ -39,10 +39,11 @@
%hr %hr
%h5 %h5
= _('Groups (%{count})') % { count: @group_notifications.size } = _('Groups (%{count})') % { count: @user_groups.total_count }
%div %div
- @group_notifications.each do |setting| - @group_notifications.each do |setting|
= render 'group_settings', setting: setting, group: setting.source = render 'group_settings', setting: setting, group: setting.source
= paginate @user_groups, theme: 'gitlab'
%h5 %h5
= _('Projects (%{count})') % { count: @project_notifications.size } = _('Projects (%{count})') % { count: @project_notifications.size }
%p.account-well %p.account-well
......
---
title: Paginate profile group notifications
merge_request: 40326
author:
type: added
...@@ -53,6 +53,22 @@ RSpec.describe Profiles::NotificationsController do ...@@ -53,6 +53,22 @@ RSpec.describe Profiles::NotificationsController do
end end
end end
context 'with group notifications' do
let_it_be(:group) { create(:group) }
let_it_be(:subgroups) { create_list(:group, 10, parent: group) }
before do
group.add_developer(user)
sign_in(user)
stub_const('Profiles::NotificationsController::NOTIFICATIONS_PER_PAGE', 5)
get :show
end
it 'paginates the groups' do
expect(assigns(:group_notifications).count).to eq(5)
end
end
context 'with project notifications' do context 'with project notifications' do
let!(:notification_setting) { create(:notification_setting, source: project, user: user, level: :watch) } let!(:notification_setting) { create(:notification_setting, source: project, user: user, level: :watch) }
......
...@@ -25,7 +25,8 @@ RSpec.describe 'view user notifications' do ...@@ -25,7 +25,8 @@ RSpec.describe 'view user notifications' do
end end
describe 'GET /profile/notifications' do describe 'GET /profile/notifications' do
it 'avoid N+1 due to an additional groups (with no parent group)' do # To be fixed in https://gitlab.com/gitlab-org/gitlab/-/merge_requests/40457
it 'has an N+1 due to an additional groups (with no parent group) - but should not' do
get_profile_notifications get_profile_notifications
control = ActiveRecord::QueryRecorder.new do control = ActiveRecord::QueryRecorder.new do
...@@ -36,7 +37,7 @@ RSpec.describe 'view user notifications' do ...@@ -36,7 +37,7 @@ RSpec.describe 'view user notifications' do
expect do expect do
get_profile_notifications get_profile_notifications
end.not_to exceed_query_limit(control) end.to exceed_query_limit(control)
end end
end end
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