diff --git a/app/finders/projects/branch_rules_finder.rb b/app/finders/projects/branch_rules_finder.rb index b8afddbbfd658f22c803e614ef8822dac52b62e6..77908c660fdde859d8d18fe2bfd6ed16ffa305da 100644 --- a/app/finders/projects/branch_rules_finder.rb +++ b/app/finders/projects/branch_rules_finder.rb @@ -16,6 +16,8 @@ def initialize(project, custom_rules:, protected_branches:) @project = project @custom_rules = custom_rules @protected_branches = protected_branches + @default_branch = project.default_branch + @default_protected_branch = ProtectedBranch.default_branch_for(project) if @default_branch end def execute(cursor: nil, limit: DEFAULT_LIMIT) @@ -30,7 +32,7 @@ def execute(cursor: nil, limit: DEFAULT_LIMIT) private - attr_reader :project, :custom_rules, :protected_branches + attr_reader :project, :custom_rules, :protected_branches, :default_branch, :default_protected_branch def paginate_from_start(limit) return custom_rules_page(limit) if custom_rules.size > limit @@ -52,13 +54,18 @@ def paginate_after_protected_branch_rule(after_cursor, limit) # Only apply cursor if we have both name and id query = query.after_name_and_id(after_cursor['name'], after_cursor['id']) if after_cursor && after_cursor['id'] - # Add one extra to check if a next page exists - branches = query.limit(limit + 1).to_a + # Exclude the default branch from the query if we're not on the first page + query = query.excluding_name(@default_branch) if after_cursor && @default_branch + + fetch_limit = calculate_fetch_limit(after_cursor, limit) + branches = query.limit(fetch_limit).to_a return Page.new if branches.empty? has_next = branches.size > limit page_branches = branches.first(limit) + page_branches = prioritise_default_branch(page_branches) if first_page_with_default_branch?(after_cursor) + cursor = has_next ? encode_cursor(page_branches.last.name, page_branches.last.id) : nil Page.new( @@ -68,6 +75,22 @@ def paginate_after_protected_branch_rule(after_cursor, limit) ) end + def calculate_fetch_limit(after_cursor, limit) + # On first page, we'll prepend the default branch, so fetch one fewer + return limit if first_page_with_default_branch?(after_cursor) + + limit + 1 + end + + def first_page_with_default_branch?(after_cursor) + @default_protected_branch && !after_cursor + end + + def prioritise_default_branch(page_branches) + filtered = page_branches - [@default_protected_branch] + [@default_protected_branch] + filtered + end + def custom_rules_page(limit) rules = custom_rules.first(limit) diff --git a/app/models/protected_branch.rb b/app/models/protected_branch.rb index 200f1537e3400b82ea421d1d840c9ef99b4a7b24..927abc87d7bd5ed02f24896c17bdd4c9232c38bc 100644 --- a/app/models/protected_branch.rb +++ b/app/models/protected_branch.rb @@ -29,6 +29,8 @@ class ProtectedBranch < ApplicationRecord where('(name > ?) OR (name = ? AND id > ?)', name, name, id).order(:name, :id) } + scope :excluding_name, ->(name) { where.not(name: name) if name.present? } + protected_ref_access_levels :merge, :push def self.get_ids_by_name(name) diff --git a/spec/finders/projects/branch_rules_finder_spec.rb b/spec/finders/projects/branch_rules_finder_spec.rb index bd93effe868f43b068148010a2a0aac2cbe22a5d..76c595008d0ce6bb03d20abcfe6198447b47f915 100644 --- a/spec/finders/projects/branch_rules_finder_spec.rb +++ b/spec/finders/projects/branch_rules_finder_spec.rb @@ -209,6 +209,33 @@ end end + context 'with default branch' do + before do + create(:protected_branch, project: project, name: project.default_branch) + create(:protected_branch, project: project, name: 'zzz-branch-a') + create(:protected_branch, project: project, name: 'zzz-branch-b') + end + + describe 'first page' do + it 'prioritises default branch' do + page = finder.execute(cursor: nil, limit: 3) + + expect(page.rules.first).to eq(all_branches_rule) + expect(page.rules.second.protected_branch.name).to eq(project.default_branch) + end + end + + describe 'second page' do + it 'excludes the default branch' do + first_page = finder.execute(cursor: nil, limit: 3) + second_page = finder.execute(cursor: first_page.end_cursor, limit: 3) + + branch_names = second_page.rules.filter_map { |rule| rule.protected_branch&.name } + expect(branch_names).not_to include(project.default_branch) + end + end + end + context 'for cursor edge cases' do let!(:protected_branch) { create(:protected_branch, project: project, name: 'main') } diff --git a/spec/models/protected_branch_spec.rb b/spec/models/protected_branch_spec.rb index b4e4265882cb2d536027493ec77024c564431a24..af6b87427a2e95d27561d2730d00326d28dd1a69 100644 --- a/spec/models/protected_branch_spec.rb +++ b/spec/models/protected_branch_spec.rb @@ -316,6 +316,31 @@ end end + describe '.excluding_name' do + let_it_be(:project) { create(:project) } + let_it_be(:branch_a) { create(:protected_branch, project: project, name: 'a') } + let_it_be(:branch_b) { create(:protected_branch, project: project, name: 'b') } + let_it_be(:branch_c) { create(:protected_branch, project: project, name: 'c') } + + it 'excludes the branch with the given name' do + result = described_class.where(project: project).excluding_name('b') + + expect(result).to contain_exactly(branch_a, branch_c) + end + + it 'returns all branches when name is nil' do + result = described_class.where(project: project).excluding_name(nil) + + expect(result).to contain_exactly(branch_a, branch_b, branch_c) + end + + it 'returns all branches when name is empty string' do + result = described_class.where(project: project).excluding_name('') + + expect(result).to contain_exactly(branch_a, branch_b, branch_c) + end + end + describe '#default_branch?' do context 'when group level' do subject { build_stubbed(:protected_branch, project: nil, group: build(:group)) }