diff --git a/app/controllers/projects/blame_controller.rb b/app/controllers/projects/blame_controller.rb index 57a06f26f8c397b45c92606600dd1a45a69f88bd..64ced43311a3ab0254c87fb6406d7145db08297e 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 0000000000000000000000000000000000000000..f7c1240a3ba6acbd28a4f2e1890a1d36c33e49b1 --- /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 7f72438c3f9dae3d2d98d5b2d24854b25c34cdb7..bc1e62a8980442892c48115f24d6b67497dcdc02 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 0000000000000000000000000000000000000000..49fb1a7f60524e9108b403cb036e569631335642 --- /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 0000000000000000000000000000000000000000..bb3b5cd931cbd449a07d7795f5ef1e89ea8d0a98 --- /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 0000000000000000000000000000000000000000..40b2bc869dc0b9ce963a2bbfbb790bd4c9c1a3e3 --- /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