diff --git a/app/models/member.rb b/app/models/member.rb index 64e0d33fb20..69406379948 100644 --- a/app/models/member.rb +++ b/app/models/member.rb @@ -28,17 +28,34 @@ class Member < ActiveRecord::Base allow_nil: true } + # This scope encapsulates (most of) the conditions a row in the member table + # must satisfy if it is a valid permission. Of particular note: + # + # * Access requests must be excluded + # * Blocked users must be excluded + # * Invitations take effect immediately + # * expires_at is not implemented. A background worker purges expired rows + scope :active, -> do + is_external_invite = arel_table[:user_id].eq(nil).and(arel_table[:invite_token].not_eq(nil)) + user_is_active = User.arel_table[:state].eq(:active) + + includes(:user).references(:users) + .where(is_external_invite.or(user_is_active)) + .where(requested_at: nil) + end + scope :invite, -> { where.not(invite_token: nil) } scope :non_invite, -> { where(invite_token: nil) } scope :request, -> { where.not(requested_at: nil) } - scope :has_access, -> { where('access_level > 0') } - scope :guests, -> { where(access_level: GUEST) } - scope :reporters, -> { where(access_level: REPORTER) } - scope :developers, -> { where(access_level: DEVELOPER) } - scope :masters, -> { where(access_level: MASTER) } - scope :owners, -> { where(access_level: OWNER) } - scope :owners_and_masters, -> { where(access_level: [OWNER, MASTER]) } + scope :has_access, -> { active.where('access_level > 0') } + + scope :guests, -> { active.where(access_level: GUEST) } + scope :reporters, -> { active.where(access_level: REPORTER) } + scope :developers, -> { active.where(access_level: DEVELOPER) } + scope :masters, -> { active.where(access_level: MASTER) } + scope :owners, -> { active.where(access_level: OWNER) } + scope :owners_and_masters, -> { active.where(access_level: [OWNER, MASTER]) } before_validation :generate_invite_token, on: :create, if: -> (member) { member.invite_email.present? } diff --git a/spec/models/member_spec.rb b/spec/models/member_spec.rb index fef90d9b5cb..0b1634f654a 100644 --- a/spec/models/member_spec.rb +++ b/spec/models/member_spec.rb @@ -57,7 +57,7 @@ describe Member, models: true do describe 'Scopes & finders' do before do - project = create(:project) + project = create(:empty_project) group = create(:group) @owner_user = create(:user).tap { |u| group.add_owner(u) } @owner = group.members.find_by(user_id: @owner_user.id) @@ -65,6 +65,15 @@ describe Member, models: true do @master_user = create(:user).tap { |u| project.team << [u, :master] } @master = project.members.find_by(user_id: @master_user.id) + @blocked_user = create(:user).tap do |u| + project.team << [u, :master] + project.team << [u, :developer] + + u.block! + end + @blocked_master = project.members.find_by(user_id: @blocked_user.id, access_level: Gitlab::Access::MASTER) + @blocked_developer = project.members.find_by(user_id: @blocked_user.id, access_level: Gitlab::Access::DEVELOPER) + Member.add_user( project.members, 'toto1@example.com', @@ -73,7 +82,7 @@ describe Member, models: true do ) @invited_member = project.members.invite.find_by_invite_email('toto1@example.com') - accepted_invite_user = build(:user) + accepted_invite_user = build(:user, state: :active) Member.add_user( project.members, 'toto2@example.com', @@ -91,7 +100,7 @@ describe Member, models: true do describe '.access_for_user_ids' do it 'returns the right access levels' do - users = [@owner_user.id, @master_user.id] + users = [@owner_user.id, @master_user.id, @blocked_user.id] expected = { @owner_user.id => Gitlab::Access::OWNER, @master_user.id => Gitlab::Access::MASTER @@ -125,6 +134,19 @@ describe Member, models: true do it { expect(described_class.request).not_to include @accepted_request_member } end + describe '.developers' do + subject { described_class.developers.to_a } + + it { is_expected.not_to include @owner } + it { is_expected.not_to include @master } + it { is_expected.to include @invited_member } + it { is_expected.to include @accepted_invite_member } + it { is_expected.not_to include @requested_member } + it { is_expected.to include @accepted_request_member } + it { is_expected.not_to include @blocked_master } + it { is_expected.not_to include @blocked_developer } + end + describe '.owners_and_masters' do it { expect(described_class.owners_and_masters).to include @owner } it { expect(described_class.owners_and_masters).to include @master } @@ -132,6 +154,20 @@ describe Member, models: true do it { expect(described_class.owners_and_masters).not_to include @accepted_invite_member } it { expect(described_class.owners_and_masters).not_to include @requested_member } it { expect(described_class.owners_and_masters).not_to include @accepted_request_member } + it { expect(described_class.owners_and_masters).not_to include @blocked_master } + end + + describe '.has_access' do + subject { described_class.has_access.to_a } + + it { is_expected.to include @owner } + it { is_expected.to include @master } + it { is_expected.to include @invited_member } + it { is_expected.to include @accepted_invite_member } + it { is_expected.not_to include @requested_member } + it { is_expected.to include @accepted_request_member } + it { is_expected.not_to include @blocked_master } + it { is_expected.not_to include @blocked_developer } end end