diff --git a/app/models/members/members/member_approval.rb b/app/models/members/members/member_approval.rb index 60318f9d4e5e7f2bc208bebca3aa6b5bd438a357..d9ed5a7bd9117f3215b131a01e00821aad86e04a 100644 --- a/app/models/members/members/member_approval.rb +++ b/app/models/members/members/member_approval.rb @@ -13,5 +13,15 @@ class MemberApproval < ApplicationRecord validates :new_access_level, presence: true validates :old_access_level, presence: true + validate :validate_unique_pending_approval, on: [:create, :update] + + private + + def validate_unique_pending_approval + if pending? && self.class.where(member_id: member_id, member_namespace_id: member_namespace_id, + new_access_level: new_access_level, status: 0).exists? + errors.add(:base, 'A pending approval for the same member, namespace, and access level already exists.') + end + end end end diff --git a/db/migrate/20240207193743_add_conditional_unique_index_to_member_approvals.rb b/db/migrate/20240207193743_add_conditional_unique_index_to_member_approvals.rb new file mode 100644 index 0000000000000000000000000000000000000000..d12f64667162b466885adaa781b6ade1dd90b9ef --- /dev/null +++ b/db/migrate/20240207193743_add_conditional_unique_index_to_member_approvals.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true + +class AddConditionalUniqueIndexToMemberApprovals < Gitlab::Database::Migration[2.2] + milestone '16.10' + + disable_ddl_transaction! + + INDEX_NAME = 'unique_member_approvals_on_pending_status' + + def up + add_concurrent_index :member_approvals, [:member_id, :member_namespace_id, :new_access_level], + unique: true, where: "status = 0", name: INDEX_NAME + end + + def down + remove_concurrent_index_by_name :member_approvals, INDEX_NAME + end +end diff --git a/db/schema_migrations/20240207193743 b/db/schema_migrations/20240207193743 new file mode 100644 index 0000000000000000000000000000000000000000..5908034820f0cf712919f62a269337a82e9a3106 --- /dev/null +++ b/db/schema_migrations/20240207193743 @@ -0,0 +1 @@ +f8ae8294d45a5d796a86d629bef9b25142895b497db778c694d8573338a274d2 \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index 008d218d43aa85c338bb045fe984841cf965e185..300a732536253442212aa4395e9f49fbd28c5659 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -36549,6 +36549,8 @@ CREATE UNIQUE INDEX unique_instance_google_cloud_logging_configurations ON audit CREATE UNIQUE INDEX unique_instance_google_cloud_logging_configurations_name ON audit_events_instance_google_cloud_logging_configurations USING btree (name); +CREATE UNIQUE INDEX unique_member_approvals_on_pending_status ON member_approvals USING btree (member_id, member_namespace_id, new_access_level) WHERE (status = 0); + CREATE UNIQUE INDEX unique_merge_request_diff_llm_summaries_on_mr_diff_id ON merge_request_diff_llm_summaries USING btree (merge_request_diff_id); CREATE UNIQUE INDEX unique_merge_request_metrics_by_merge_request_id ON merge_request_metrics USING btree (merge_request_id); diff --git a/ee/spec/models/factories_spec.rb b/ee/spec/models/factories_spec.rb index f60a03dceec7c4f1c8543b89180652cb474954b1..04a22dae3ef3267fbaa85800314d7a1a6fcd882f 100644 --- a/ee/spec/models/factories_spec.rb +++ b/ee/spec/models/factories_spec.rb @@ -170,6 +170,7 @@ wiki_page_meta workspace workspace_variable + member_approval ].to_set.freeze # Some factories and their corresponding models are based on diff --git a/spec/factories/member_approvals.rb b/spec/factories/member_approvals.rb new file mode 100644 index 0000000000000000000000000000000000000000..cf9dd80fd82a7825d09a99c607e73b3474f3ca62 --- /dev/null +++ b/spec/factories/member_approvals.rb @@ -0,0 +1,34 @@ +# frozen_string_literal: true + +FactoryBot.define do + factory :member_approval, class: 'Members::MemberApproval' do + requested_by { association(:user) } + reviewed_by { association(:user) } + old_access_level { ::Gitlab::Access::GUEST } + new_access_level { ::Gitlab::Access::DEVELOPER } + status { ::Members::MemberApproval.statuses[:pending] } + member { association(:project_member) } # default + member_namespace_id { member.member_namespace_id } + + # Traits for specific group members + trait :for_group_member do + member { association(:group_member) } + end + + trait :for_project_member do + member { association(:project_member) } + end + + trait(:guest) { old_access_level { GroupMember::GUEST } } + trait(:reporter) { old_access_level { GroupMember::REPORTER } } + trait(:developer) { old_access_level { GroupMember::DEVELOPER } } + trait(:maintainer) { old_access_level { GroupMember::MAINTAINER } } + trait(:owner) { old_access_level { GroupMember::OWNER } } + + trait(:to_guest) { new_access_level { GroupMember::GUEST } } + trait(:to_reporter) { new_access_level { GroupMember::REPORTER } } + trait(:to_developer) { new_access_level { GroupMember::DEVELOPER } } + trait(:to_maintainer) { new_access_level { GroupMember::MAINTAINER } } + trait(:to_owner) { new_access_level { GroupMember::OWNER } } + end +end diff --git a/spec/models/members/members/member_approval_spec.rb b/spec/models/members/members/member_approval_spec.rb index ed012a5a7c08b767ceacbfd3e3f5f19625c3edcb..06894f0b9944fa74de6de880cfc2881a7102cbe9 100644 --- a/spec/models/members/members/member_approval_spec.rb +++ b/spec/models/members/members/member_approval_spec.rb @@ -13,5 +13,46 @@ describe 'validations' do it { is_expected.to validate_presence_of(:new_access_level) } it { is_expected.to validate_presence_of(:old_access_level) } + + context 'when uniqness is enforced' do + let!(:member) { create(:project_member) } + let!(:member_approval) { create(:member_approval, member: member) } + + context 'with same member, namespace, and access level and pending status' do + let(:message) { 'A pending approval for the same member, namespace, and access level already exists.' } + + it 'disallows on create' do + duplicate_approval = build(:member_approval, member: member) + + expect(duplicate_approval).not_to be_valid + expect(duplicate_approval.errors[:base]).to include(message) + end + + it 'disallows on update' do + duplicate_approval = create(:member_approval, member: member, status: :approved) + expect(duplicate_approval).to be_valid + + duplicate_approval.status = ::Members::MemberApproval.statuses[:pending] + expect(duplicate_approval).not_to be_valid + expect(duplicate_approval.errors[:base]).to include(message) + end + end + + it 'allows duplicate member approvals with different statuses' do + member_approval.update!(status: ::Members::MemberApproval.statuses[:approved]) + + pending_approval = build(:member_approval, member: member) + + expect(pending_approval).to be_valid + end + + it 'allows duplicate member approvals with different access levels' do + different_approval = build(:member_approval, + member: member, + new_access_level: ::Gitlab::Access::MAINTAINER) + + expect(different_approval).to be_valid + end + end end end