Commit e924f27a authored by Shreyas Agarwal's avatar Shreyas Agarwal Committed by Jan Provaznik

Resolve "Defects with the share with group feature"

parent 97ceffeb
......@@ -13,6 +13,8 @@ class GroupGroupLink < ApplicationRecord
validates :group_access, inclusion: { in: Gitlab::Access.all_values },
presence: true
scope :non_guests, -> { where('group_access > ?', Gitlab::Access::GUEST) }
def self.access_options
Gitlab::Access.options_with_owner
end
......
......@@ -3,11 +3,6 @@
class ProjectGroupLink < ApplicationRecord
include Expirable
GUEST = 10
REPORTER = 20
DEVELOPER = 30
MAINTAINER = 40
belongs_to :project
belongs_to :group
......@@ -18,6 +13,8 @@ class ProjectGroupLink < ApplicationRecord
validates :group_access, inclusion: { in: Gitlab::Access.values }, presence: true
validate :different_group
scope :non_guests, -> { where('group_access > ?', Gitlab::Access::GUEST) }
after_commit :refresh_group_members_authorized_projects
alias_method :shared_with_group, :group
......@@ -27,7 +24,7 @@ class ProjectGroupLink < ApplicationRecord
end
def self.default_access
DEVELOPER
Gitlab::Access::DEVELOPER
end
def self.search(query)
......
......@@ -261,15 +261,15 @@ module EE
strong_memoize(:gold_billed_user_ids) do
(billed_group_members.non_guests.distinct.pluck(:user_id) +
billed_project_members.non_guests.distinct.pluck(:user_id) +
billed_shared_group_members.non_guests.distinct.pluck(:user_id) +
billed_invited_group_members.non_guests.distinct.pluck(:user_id)).to_set
billed_shared_non_guests_group_members.non_guests.distinct.pluck(:user_id) +
billed_invited_non_guests_group_to_project_members.non_guests.distinct.pluck(:user_id)).to_set
end
else
strong_memoize(:non_gold_billed_user_ids) do
(billed_group_members.distinct.pluck(:user_id) +
billed_project_members.distinct.pluck(:user_id) +
billed_shared_group_members.distinct.pluck(:user_id) +
billed_invited_group_members.distinct.pluck(:user_id)).to_set
billed_invited_group_to_project_members.distinct.pluck(:user_id)).to_set
end
end
end
......@@ -320,35 +320,62 @@ module EE
errors.add(:custom_project_templates_group_id, 'has to be a subgroup of the group')
end
# Members belonging directly to Group or its subgroups
def billed_group_members
::GroupMember.active_without_invites_and_requests.where(
source_id: self_and_descendants
)
end
# Members belonging directly to Projects within Group or Projects within subgroups
def billed_project_members
::ProjectMember.active_without_invites_and_requests.where(
source_id: ::Project.joins(:group).where(namespace: self_and_descendants)
)
end
def billed_invited_group_members
# Members belonging to Groups invited to collaborate with Projects
def billed_invited_group_to_project_members
invited_or_shared_group_members(invited_groups_in_projects)
end
def billed_shared_group_members
return ::GroupMember.none unless ::Feature.enabled?(:share_group_with_group)
invited_or_shared_group_members(shared_groups)
def billed_invited_non_guests_group_to_project_members
invited_or_shared_group_members(invited_group_as_non_guests_in_projects)
end
def invited_or_shared_group_members(groups)
::GroupMember.active_without_invites_and_requests.where(source_id: ::Gitlab::ObjectHierarchy.new(groups).base_and_ancestors)
def invited_group_as_non_guests_in_projects
invited_groups_in_projects.merge(::ProjectGroupLink.non_guests)
end
def invited_groups_in_projects
::Group.joins(:project_group_links)
.where(project_group_links: { project_id: all_projects })
end
# Members belonging to Groups invited to collaborate with Groups and Subgroups
def billed_shared_group_members
return ::GroupMember.none unless ::Feature.enabled?(:share_group_with_group)
invited_or_shared_group_members(invited_group_in_groups)
end
def billed_shared_non_guests_group_members
return ::GroupMember.none unless ::Feature.enabled?(:share_group_with_group)
invited_or_shared_group_members(invited_non_guest_group_in_groups)
end
def invited_non_guest_group_in_groups
invited_group_in_groups.merge(::GroupGroupLink.non_guests)
end
def invited_group_in_groups
::Group.joins(:shared_group_links)
.where(group_group_links: { shared_group_id: ::Group.groups_including_descendants_by([self]) })
end
def invited_or_shared_group_members(groups)
::GroupMember.active_without_invites_and_requests.where(source_id: ::Gitlab::ObjectHierarchy.new(groups).base_and_ancestors)
end
end
end
---
title: Fix billed users id and count from shared group
merge_request: 28645
author:
type: fixed
......@@ -1030,11 +1030,26 @@ describe Namespace do
invited_group.add_guest(create(:user))
invited_group.add_developer(create(:user, :blocked))
invited_group.add_developer(developer)
create(:project_group_link, project: project, group: invited_group)
end
it 'includes the only active users except guests of the invited groups' do
expect(group.billed_user_ids).to match_array([invited_group_developer.id, project_developer.id, developer.id])
context 'when group is invited as non guest' do
before do
create(:project_group_link, project: project, group: invited_group)
end
it 'includes the only active users except guests of the invited groups' do
expect(group.billed_user_ids).to match_array([invited_group_developer.id, project_developer.id, developer.id])
end
end
context 'when group is invited as a guest to the project' do
before do
create(:project_group_link, :guest, project: project, group: invited_group)
end
it 'does not include any members from the invited group' do
expect(group.billed_user_ids).to match_array([project_developer.id, developer.id])
end
end
end
end
......@@ -1048,8 +1063,8 @@ describe Namespace do
shared_group.add_guest(create(:user))
shared_group.add_developer(create(:user, :blocked))
create(:group_group_link, { shared_with_group: group,
shared_group: shared_group })
create(:group_group_link, { shared_with_group: shared_group,
shared_group: group })
end
context 'when feature is not enabled' do
......@@ -1057,8 +1072,9 @@ describe Namespace do
stub_feature_flags(share_group_with_group: false)
end
it 'does not include users coming from the shared groups' do
it 'does not include users coming from the shared groups', :aggregate_failures do
expect(group.billed_user_ids).to match_array([developer.id])
expect(shared_group.billed_user_ids).not_to include([developer.id])
end
end
......@@ -1067,8 +1083,46 @@ describe Namespace do
stub_feature_flags(share_group_with_group: true)
end
it 'includes active users from the shared group to the billed members count' do
it 'includes active users from the shared group to the billed members', :aggregate_failures do
expect(group.billed_user_ids).to match_array([shared_group_developer.id, developer.id])
expect(shared_group.billed_user_ids).not_to include([developer.id])
end
context 'when subgroup invited another group to collaborate' do
let(:another_shared_group) { create(:group) }
let(:another_shared_group_developer) { create(:user) }
before do
another_shared_group.add_developer(another_shared_group_developer)
another_shared_group.add_guest(create(:user))
another_shared_group.add_developer(create(:user, :blocked))
end
context 'when subgroup invites another group as non guest' do
before do
subgroup = create(:group, parent: group)
create(:group_group_link, { shared_with_group: another_shared_group,
shared_group: subgroup })
end
it 'includes all the active and non guest users from the shared group', :aggregate_failures do
expect(group.billed_user_ids).to match_array([shared_group_developer.id, developer.id, another_shared_group_developer.id])
expect(shared_group.billed_user_ids).not_to include([developer.id])
expect(another_shared_group.billed_user_ids).not_to include([developer.id, shared_group_developer.id])
end
end
context 'when subgroup invites another group as guest' do
before do
subgroup = create(:group, parent: group)
create(:group_group_link, :guest, { shared_with_group: another_shared_group,
shared_group: subgroup })
end
it 'does not includes any user from the shared group from the subgroup' do
expect(group.billed_user_ids).to match_array([shared_group_developer.id, developer.id])
end
end
end
end
end
......@@ -1133,8 +1187,8 @@ describe Namespace do
shared_group.add_guest(shared_group_guest)
shared_group.add_developer(create(:user, :blocked))
create(:group_group_link, { shared_with_group: group,
shared_group: shared_group })
create(:group_group_link, { shared_with_group: shared_group,
shared_group: group })
end
context 'when feature is not enabled' do
......@@ -1152,8 +1206,9 @@ describe Namespace do
stub_feature_flags(share_group_with_group: true)
end
it 'includes active users from the shared group including guests' do
it 'includes active users from the shared group including guests', :aggregate_failures do
expect(group.billed_user_ids).to match_array([developer.id, guest.id, shared_group_developer.id, shared_group_guest.id])
expect(shared_group.billed_user_ids).to match_array([shared_group_developer.id, shared_group_guest.id])
end
end
end
......@@ -1229,8 +1284,8 @@ describe Namespace do
shared_group.add_guest(create(:user))
shared_group.add_developer(create(:user, :blocked))
create(:group_group_link, { shared_with_group: group,
shared_group: shared_group })
create(:group_group_link, { shared_with_group: shared_group,
shared_group: group })
end
context 'when feature is not enabled' do
......@@ -1303,8 +1358,8 @@ describe Namespace do
shared_group.add_guest(create(:user))
shared_group.add_developer(create(:user, :blocked))
create(:group_group_link, { shared_with_group: group,
shared_group: shared_group })
create(:group_group_link, { shared_with_group: shared_group,
shared_group: group })
end
context 'when feature is not enabled' do
......
......@@ -13,7 +13,7 @@ describe Groups::SharedProjectsController do
Projects::GroupLinks::CreateService.new(
project,
user,
link_group_access: ProjectGroupLink::DEVELOPER
link_group_access: Gitlab::Access::DEVELOPER
).execute(group)
end
......
......@@ -4,6 +4,12 @@ FactoryBot.define do
factory :group_group_link do
shared_group { create(:group) }
shared_with_group { create(:group) }
group_access { GroupMember::DEVELOPER }
group_access { Gitlab::Access::DEVELOPER }
trait(:guest) { group_access { Gitlab::Access::GUEST } }
trait(:reporter) { group_access { Gitlab::Access::REPORTER } }
trait(:developer) { group_access { Gitlab::Access::DEVELOPER } }
trait(:owner) { group_access { Gitlab::Access::OWNER } }
trait(:maintainer) { group_access { Gitlab::Access::MAINTAINER } }
end
end
......@@ -5,5 +5,10 @@ FactoryBot.define do
project
group
expires_at { nil }
trait(:guest) { group_access { Gitlab::Access::GUEST } }
trait(:reporter) { group_access { Gitlab::Access::REPORTER } }
trait(:developer) { group_access { Gitlab::Access::DEVELOPER } }
trait(:maintainer) { group_access { Gitlab::Access::MAINTAINER } }
end
end
......@@ -123,7 +123,7 @@ describe GroupDescendantsFinder do
project = create(:project, namespace: group)
other_project = create(:project)
other_project.project_group_links.create(group: group,
group_access: ProjectGroupLink::MAINTAINER)
group_access: Gitlab::Access::MAINTAINER)
expect(finder.execute).to contain_exactly(project)
end
......
......@@ -15,6 +15,22 @@ describe GroupGroupLink do
it { is_expected.to belong_to(:shared_with_group) }
end
describe 'scopes' do
describe '.non_guests' do
let!(:group_group_link_reporter) { create :group_group_link, :reporter }
let!(:group_group_link_maintainer) { create :group_group_link, :maintainer }
let!(:group_group_link_owner) { create :group_group_link, :owner }
let!(:group_group_link_guest) { create :group_group_link, :guest }
it 'returns all records which are greater than Guests access' do
expect(described_class.non_guests).to match_array([
group_group_link_reporter, group_group_link,
group_group_link_maintainer, group_group_link_owner
])
end
end
end
describe 'validation' do
it { is_expected.to validate_presence_of(:shared_group) }
......
......@@ -32,6 +32,23 @@ describe ProjectGroupLink do
end
end
describe 'scopes' do
describe '.non_guests' do
let!(:project_group_link_reporter) { create :project_group_link, :reporter }
let!(:project_group_link_maintainer) { create :project_group_link, :maintainer }
let!(:project_group_link_developer) { create :project_group_link }
let!(:project_group_link_guest) { create :project_group_link, :guest }
it 'returns all records which are greater than Guests access' do
expect(described_class.non_guests).to match_array([
project_group_link_reporter,
project_group_link_developer,
project_group_link_maintainer
])
end
end
end
describe "destroying a record", :delete do
it "refreshes group users' authorized projects" do
project = create(:project, :private)
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment