From 05aa628a1e285c3a105d633c39cfbf5317f78bba Mon Sep 17 00:00:00 2001 From: Vasilii Iakliushin Date: Mon, 25 Apr 2022 17:08:03 +0200 Subject: [PATCH] Add pagination for File Blame page Contributes to https://gitlab.com/gitlab-org/gitlab/-/issues/225174 * Add BlameService to control pagination and blame range calculations * Add Kaminari pagination for File Blame --- app/controllers/projects/blame_controller.rb | 7 +- app/services/projects/blame_service.rb | 65 +++++++++ app/views/projects/blame/show.html.haml | 3 + .../development/blame_page_pagination.yml | 8 ++ spec/features/projects/blobs/blame_spec.rb | 67 +++++++++ spec/services/projects/blame_service_spec.rb | 129 ++++++++++++++++++ 6 files changed, 277 insertions(+), 2 deletions(-) create mode 100644 app/services/projects/blame_service.rb create mode 100644 config/feature_flags/development/blame_page_pagination.yml create mode 100644 spec/features/projects/blobs/blame_spec.rb create mode 100644 spec/services/projects/blame_service_spec.rb diff --git a/app/controllers/projects/blame_controller.rb b/app/controllers/projects/blame_controller.rb index 57a06f26f8c397..64ced43311a3ab 100644 --- a/app/controllers/projects/blame_controller.rb +++ b/app/controllers/projects/blame_controller.rb @@ -23,8 +23,11 @@ def show environment_params[:find_latest] = true @environment = ::Environments::EnvironmentsByDeploymentsFinder.new(@project, current_user, environment_params).execute.last - @blame = Gitlab::Blame.new(@blob, @commit) - @blame = Gitlab::View::Presenter::Factory.new(@blame, project: @project, path: @path).fabricate! + blame_service = Projects::BlameService.new(@blob, @commit, params.permit(:page)) + + @blame = Gitlab::View::Presenter::Factory.new(blame_service.blame, project: @project, path: @path).fabricate! + + render locals: { blame_pagination: blame_service.pagination } end end diff --git a/app/services/projects/blame_service.rb b/app/services/projects/blame_service.rb new file mode 100644 index 00000000000000..f7c1240a3ba6ac --- /dev/null +++ b/app/services/projects/blame_service.rb @@ -0,0 +1,65 @@ +# frozen_string_literal: true + +# Service class to correctly initialize Gitlab::Blame and Kaminari pagination +# objects +module Projects + class BlameService + PER_PAGE = 1000 + + def initialize(blob, commit, params) + @blob = blob + @commit = commit + @page = extract_page(params) + end + + def blame + Gitlab::Blame.new(blob, commit, range: blame_range) + end + + def pagination + return unless pagination_enabled? + + Kaminari.paginate_array([], total_count: blob_lines_count) + .page(page) + .per(per_page) + .limit(per_page) + end + + private + + attr_reader :blob, :commit, :page + + def blame_range + return unless pagination_enabled? + + first_line = (page - 1) * per_page + 1 + last_line = (first_line + per_page).to_i - 1 + + first_line..last_line + end + + def extract_page(params) + page = params.fetch(:page, 1).to_i + + return 1 if page < 1 || overlimit?(page) + + page + end + + def per_page + PER_PAGE + end + + def overlimit?(page) + page * per_page >= blob_lines_count + per_page + end + + def blob_lines_count + @blob_lines_count ||= blob.data.lines.count + end + + def pagination_enabled? + Feature.enabled?(:blame_page_pagination, commit.project) + end + end +end diff --git a/app/views/projects/blame/show.html.haml b/app/views/projects/blame/show.html.haml index 7f72438c3f9dae..bc1e62a8980442 100644 --- a/app/views/projects/blame/show.html.haml +++ b/app/views/projects/blame/show.html.haml @@ -58,3 +58,6 @@ #{line} - current_line += line_count + + - if blame_pagination + = paginate(blame_pagination, theme: "gitlab") diff --git a/config/feature_flags/development/blame_page_pagination.yml b/config/feature_flags/development/blame_page_pagination.yml new file mode 100644 index 00000000000000..49fb1a7f60524e --- /dev/null +++ b/config/feature_flags/development/blame_page_pagination.yml @@ -0,0 +1,8 @@ +--- +name: blame_page_pagination +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/85827 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/360927 +milestone: '15.0' +type: development +group: group::source code +default_enabled: false diff --git a/spec/features/projects/blobs/blame_spec.rb b/spec/features/projects/blobs/blame_spec.rb new file mode 100644 index 00000000000000..bb3b5cd931cbd4 --- /dev/null +++ b/spec/features/projects/blobs/blame_spec.rb @@ -0,0 +1,67 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe 'File blame', :js do + include TreeHelper + + let_it_be(:project) { create(:project, :public, :repository) } + + let(:path) { 'CHANGELOG' } + + def visit_blob_blame(path) + visit project_blame_path(project, tree_join('master', path)) + wait_for_all_requests + end + + it 'displays the blame page without pagination' do + visit_blob_blame(path) + + expect(page).to have_css('.blame-commit') + expect(page).not_to have_css('.gl-pagination') + end + + context 'when blob length is over the blame range limit' do + before do + stub_const('Projects::BlameService::PER_PAGE', 2) + end + + it 'displays two first lines of the file with pagination' do + visit_blob_blame(path) + + expect(page).to have_css('.blame-commit') + expect(page).to have_css('.gl-pagination') + + expect(page).to have_css('#L1') + expect(page).not_to have_css('#L3') + expect(find('.page-link.active')).to have_text('1') + end + + context 'when user clicks on the next button' do + before do + visit_blob_blame(path) + + find('.js-next-button').click + end + + it 'displays next two lines of the file with pagination' do + expect(page).not_to have_css('#L1') + expect(page).to have_css('#L3') + expect(find('.page-link.active')).to have_text('2') + end + end + + context 'when feature flag disabled' do + before do + stub_feature_flags(blame_page_pagination: false) + end + + it 'displays the blame page without pagination' do + visit_blob_blame(path) + + expect(page).to have_css('.blame-commit') + expect(page).not_to have_css('.gl-pagination') + end + end + end +end diff --git a/spec/services/projects/blame_service_spec.rb b/spec/services/projects/blame_service_spec.rb new file mode 100644 index 00000000000000..40b2bc869dc0b9 --- /dev/null +++ b/spec/services/projects/blame_service_spec.rb @@ -0,0 +1,129 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Projects::BlameService, :aggregate_failures do + subject(:service) { described_class.new(blob, commit, params) } + + let_it_be(:project) { create(:project, :repository) } + let_it_be(:commit) { project.repository.commit } + let_it_be(:blob) { project.repository.blob_at('HEAD', 'README.md') } + + let(:params) { { page: page } } + let(:page) { nil } + + before do + stub_const("#{described_class.name}::PER_PAGE", 2) + end + + describe '#blame' do + subject { service.blame } + + it 'returns a correct Gitlab::Blame object' do + is_expected.to be_kind_of(Gitlab::Blame) + + expect(subject.blob).to eq(blob) + expect(subject.commit).to eq(commit) + expect(subject.range).to eq(1..2) + end + + describe 'Pagination range calculation' do + subject { service.blame.range } + + context 'with page = 1' do + let(:page) { 1 } + + it { is_expected.to eq(1..2) } + end + + context 'with page = 2' do + let(:page) { 2 } + + it { is_expected.to eq(3..4) } + end + + context 'with page = 3 (overlimit)' do + let(:page) { 3 } + + it { is_expected.to eq(1..2) } + end + + context 'with page = 0 (incorrect)' do + let(:page) { 0 } + + it { is_expected.to eq(1..2) } + end + + context 'when feature flag disabled' do + before do + stub_feature_flags(blame_page_pagination: false) + end + + it { is_expected.to be_nil } + end + end + end + + describe '#pagination' do + subject { service.pagination } + + it 'returns a pagination object' do + is_expected.to be_kind_of(Kaminari::PaginatableArray) + + expect(subject.current_page).to eq(1) + expect(subject.total_pages).to eq(2) + expect(subject.total_count).to eq(4) + end + + context 'when feature flag disabled' do + before do + stub_feature_flags(blame_page_pagination: false) + end + + it { is_expected.to be_nil } + end + + context 'when per_page is above the global max per page limit' do + before do + stub_const("#{described_class.name}::PER_PAGE", 1000) + allow(blob).to receive_message_chain(:data, :lines, :count) { 500 } + end + + it 'returns a correct pagination object' do + is_expected.to be_kind_of(Kaminari::PaginatableArray) + + expect(subject.current_page).to eq(1) + expect(subject.total_pages).to eq(1) + expect(subject.total_count).to eq(500) + end + end + + describe 'Current page' do + subject { service.pagination.current_page } + + context 'with page = 1' do + let(:page) { 1 } + + it { is_expected.to eq(1) } + end + + context 'with page = 2' do + let(:page) { 2 } + + it { is_expected.to eq(2) } + end + + context 'with page = 3 (overlimit)' do + let(:page) { 3 } + + it { is_expected.to eq(1) } + end + + context 'with page = 0 (incorrect)' do + let(:page) { 0 } + + it { is_expected.to eq(1) } + end + end + end +end -- GitLab