Merge branch '213211-resolve-count-issue-in-sharing-group' into 'master'

Resolve "Defects with the share with group feature"

Closes #213211

See merge request gitlab-org/gitlab!28645

app/models/group_group_link.rb

@@ -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

app/models/project_group_link.rb

@@ -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)

ee/app/models/ee/group.rb

@@ -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

ee/changelogs/unreleased/213211-resolve-count-issue-in-sharing-group.yml

+---
+title: Fix billed users id and count from shared group
+merge_request: 28645
+author:
+type: fixed

ee/spec/models/ee/namespace_spec.rb

@@ -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

spec/controllers/groups/shared_projects_controller_spec.rb

@@ -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
 

spec/factories/group_group_links.rb

@@ -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

spec/factories/project_group_links.rb

@@ -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

spec/finders/group_descendants_finder_spec.rb

@@ -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

spec/models/group_group_link_spec.rb

@@ -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) }
 

spec/models/project_group_link_spec.rb

@@ -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)