diff --git a/app/models/project.rb b/app/models/project.rb index def7d6151c09831e36bfaeae55568928518eb922..ecb59cb7e7b8c9c85fe4c20704644392d0cd4912 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -342,6 +342,8 @@ def self.integration_association_name(name) has_many :hooks, class_name: 'ProjectHook' has_many :protected_branches has_many :exported_protected_branches + has_many :all_protected_branches, ->(project) { ProtectedBranch.unscope(:where).from_union(project.protected_branches, project.group_protected_branches) }, class_name: 'ProtectedBranch' + has_many :protected_tags has_many :repository_languages, -> { order "share DESC" } has_many :designs, inverse_of: :project, class_name: 'DesignManagement::Design' @@ -1278,8 +1280,8 @@ def autoclose_referenced_issues def preload_protected_branches ActiveRecord::Associations::Preloader.new( - records: [self], - associations: { protected_branches: [:push_access_levels, :merge_access_levels] } + records: [all_protected_branches, protected_branches].flatten, + associations: [:push_access_levels, :merge_access_levels] ).call end @@ -2948,15 +2950,9 @@ def limited_protected_branches(limit) end def group_protected_branches - root_namespace.is_a?(Group) ? root_namespace.protected_branches : ProtectedBranch.none - end + return root_namespace.protected_branches if allow_protected_branches_for_group? && root_namespace.is_a?(Group) - def all_protected_branches - if allow_protected_branches_for_group? - @all_protected_branches ||= ProtectedBranch.from_union([protected_branches, group_protected_branches]) - else - protected_branches - end + ProtectedBranch.none end def allow_protected_branches_for_group? diff --git a/lib/api/branches.rb b/lib/api/branches.rb index c3a3058b95b71f3d75cca768117f3605333a0bda..8d0dff20dbacefe828edb8e4bfcd45a1b744f912 100644 --- a/lib/api/branches.rb +++ b/lib/api/branches.rb @@ -48,8 +48,6 @@ class Branches < ::API::Base end get ':id/repository/branches', urgency: :low do cache_action([user_project, :branches, current_user, declared_params], expires_in: 30.seconds) do - user_project.preload_protected_branches - repository = user_project.repository branches_finder = BranchesFinder.new(repository, declared_params(include_missing: false)) @@ -57,6 +55,7 @@ class Branches < ::API::Base merged_branch_names = repository.merged_branch_names(branches.map(&:name)) + user_project.preload_protected_branches if branches.present? present_cached( branches, with: Entities::Branch, diff --git a/lib/api/entities/branch.rb b/lib/api/entities/branch.rb index 7a6a3da6e696ca72cdbaad74c2b267342994869f..327aa0ffc221f9d2fa5d1841749d8ffea93773be 100644 --- a/lib/api/entities/branch.rb +++ b/lib/api/entities/branch.rb @@ -36,7 +36,7 @@ class Branch < Grape::Entity type: 'boolean', example: true } do |repo_branch, options| - ::ProtectedBranch.developers_can?(:push, repo_branch.name, protected_refs: options[:project].protected_branches) + ::ProtectedBranch.developers_can?(:push, repo_branch.name, protected_refs: options[:project].all_protected_branches) end expose :developers_can_merge, @@ -44,7 +44,7 @@ class Branch < Grape::Entity type: 'boolean', example: true } do |repo_branch, options| - ::ProtectedBranch.developers_can?(:merge, repo_branch.name, protected_refs: options[:project].protected_branches) + ::ProtectedBranch.developers_can?(:merge, repo_branch.name, protected_refs: options[:project].all_protected_branches) end expose :can_push, diff --git a/spec/lib/gitlab/import_export/all_models.yml b/spec/lib/gitlab/import_export/all_models.yml index b993f933b48738b5e24b05d099e6890910cbc8bf..6d59a48742aec4fb60ee6beb9ac143f7b4f33beb 100644 --- a/spec/lib/gitlab/import_export/all_models.yml +++ b/spec/lib/gitlab/import_export/all_models.yml @@ -645,6 +645,7 @@ project: - notes - snippets - hooks +- all_protected_branches - protected_branches - protected_tags - project_members diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 51e32d473a6836469f9d2f72d8469f3923e8673b..ce0e3ea1c6e4667043812778e2d8ceae3886422f 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -196,6 +196,7 @@ it { is_expected.to have_many(:alert_hooks_integrations).class_name('Integration') } it { is_expected.to have_many(:incident_hooks_integrations).class_name('Integration') } it { is_expected.to have_many(:relation_import_trackers).class_name('Projects::ImportExport::RelationImportTracker') } + it { is_expected.to have_many(:all_protected_branches).class_name('ProtectedBranch') } # GitLab Pages it { is_expected.to have_many(:pages_domains) } diff --git a/spec/models/protected_branch_spec.rb b/spec/models/protected_branch_spec.rb index b34e4a7bf3000c0818f46c72a82bd8cbf7737f7c..06f77ea9ebb7e63290f37131e57198adac049f23 100644 --- a/spec/models/protected_branch_spec.rb +++ b/spec/models/protected_branch_spec.rb @@ -83,34 +83,14 @@ end describe '.protected_refs' do - let_it_be(:project) { create(:project) } + let(:project) { build_stubbed(:project) } subject { described_class.protected_refs(project) } - context 'when feature flag enabled' do - before do - stub_feature_flags(group_protected_branches: true) - stub_feature_flags(allow_protected_branches_for_group: true) - end - - it 'call `all_protected_branches`' do - expect(project).to receive(:all_protected_branches) - - subject - end - end - - context 'when feature flag disabled' do - before do - stub_feature_flags(group_protected_branches: false) - stub_feature_flags(allow_protected_branches_for_group: false) - end - - it 'call `protected_branches`' do - expect(project).to receive(:protected_branches) + it 'call `all_protected_branches`' do + expect(project).to receive(:all_protected_branches) - subject - end + subject end end diff --git a/spec/requests/api/branches_spec.rb b/spec/requests/api/branches_spec.rb index c1fe5c6d0a97f822f92a95100b0a9ddf8f70f798..45ab91c74e84950a04bba6820b23eaff713fface 100644 --- a/spec/requests/api/branches_spec.rb +++ b/spec/requests/api/branches_spec.rb @@ -5,7 +5,7 @@ RSpec.describe API::Branches, feature_category: :source_code_management do let_it_be(:user) { create(:user) } - let(:project) { create(:project, :repository, creator: user, path: 'my.project', create_branch: 'ends-with.txt') } + let(:project) { create(:project, :in_group, :repository, creator: user, path: 'my.project', create_branch: 'ends-with.txt') } let(:guest) { create(:user, guest_of: project) } let(:branch_name) { 'feature' } let(:branch_sha) { '0b4bc9a49b562e85de7cc9e834518ea6828729b9' } @@ -295,10 +295,79 @@ def check_merge_status(json_response) new_branch_name = 'protected-branch' ::Branches::CreateService.new(project, current_user).execute(new_branch_name, 'master') create(:protected_branch, name: new_branch_name, project: project) + create(:protected_branch, name: new_branch_name, project: nil, group: project.group) expect do get api(route, current_user), params: { per_page: 100 } - end.not_to exceed_query_limit(control).with_threshold(1) + end.not_to exceed_query_limit(control) + end + end + + context 'with group protected branches' do + subject(:request) { get api(route, user) } + + let!(:group_protected_branch) do + create(:protected_branch, + *access_levels, + project: nil, + group: project.group, + name: '*' + ) + end + + context 'maintainers allowed to push and merge' do + let(:access_levels) { [] } + + it 'responds with correct attributes related to push and merge' do + request + + expect(json_response.dig(0, 'developers_can_merge')).to be_falsey + expect(json_response.dig(0, 'developers_can_push')).to be_falsey + expect(json_response.dig(0, 'can_push')).to be_truthy + end + + context 'and there is a more permissive project level protected branch' do + let!(:project_level_protected_branch) do + create(:protected_branch, + :developers_can_merge, + :developers_can_push, + project: project, + name: '*' + ) + end + + it 'responds with correct attributes related to push and merge' do + request + + expect(json_response.dig(0, 'developers_can_merge')).to be_truthy + expect(json_response.dig(0, 'developers_can_push')).to be_truthy + expect(json_response.dig(0, 'can_push')).to be_truthy + end + end + end + + context 'when developers can push and merge' do + let(:access_levels) { %i[developers_can_merge developers_can_push] } + + it 'responds with correct attributes related to push and merge' do + request + + expect(json_response.dig(0, 'developers_can_merge')).to be_truthy + expect(json_response.dig(0, 'developers_can_push')).to be_truthy + expect(json_response.dig(0, 'can_push')).to be_truthy + end + end + + context 'when no one can push and merge' do + let(:access_levels) { %i[no_one_can_merge no_one_can_push] } + + it 'responds with correct attributes related to push and merge' do + request + + expect(json_response.dig(0, 'developers_can_merge')).to be_falsey + expect(json_response.dig(0, 'developers_can_push')).to be_falsey + expect(json_response.dig(0, 'can_push')).to be_falsey + end end end