Merge branch 'vij-billable-member-source' into 'master'

Establish billable member membership type with a Group

See merge request gitlab-org/gitlab!60600

doc/api/members.md

@@ -288,7 +288,8 @@ Example response:
     "state": "active",
     "avatar_url": "https://www.gravatar.com/avatar/c2525a7f58ae3776070e44c106c48e15?s=80&d=identicon",
     "web_url": "http://192.168.1.8:3000/root",
-    "last_activity_on": "2021-01-27"
+    "last_activity_on": "2021-01-27",
+    "membership_type": "group_member"
   },
   {
     "id": 2,
@@ -298,7 +299,8 @@ Example response:
     "avatar_url": "https://www.gravatar.com/avatar/c2525a7f58ae3776070e44c106c48e15?s=80&d=identicon",
     "web_url": "http://192.168.1.8:3000/root",
     "email": "john@example.com",
-    "last_activity_on": "2021-01-25"
+    "last_activity_on": "2021-01-25",
+    "membership_type": "group_member"
   },
   {
     "id": 3,
@@ -307,7 +309,8 @@ Example response:
     "state": "active",
     "avatar_url": "https://www.gravatar.com/avatar/c2525a7f58ae3776070e44c106c48e15?s=80&d=identicon",
     "web_url": "http://192.168.1.8:3000/root",
-    "last_activity_on": "2021-01-20"
+    "last_activity_on": "2021-01-20",
+    "membership_type": "group_invite"
   }
 ]
 ```

ee/app/finders/billed_users_finder.rb

@@ -8,12 +8,18 @@ class BilledUsersFinder
   end
 
   def execute
-    return User.none unless group_billed_user_ids.any?
+    return {} unless user_ids.any?
 
-    users = ::User.id_in(group_billed_user_ids)
+    users = ::User.id_in(user_ids)
     users = users.search(search_term) if search_term
 
-    users.sort_by_attribute(order_by)
+    {
+      users: users.sort_by_attribute(order_by),
+      group_member_user_ids: group_billed_user_ids[:group_member_user_ids],
+      project_member_user_ids: group_billed_user_ids[:project_member_user_ids],
+      shared_group_user_ids: group_billed_user_ids[:shared_group_user_ids],
+      shared_project_user_ids: group_billed_user_ids[:shared_project_user_ids]
+    }
   end
 
   private
@@ -23,4 +29,8 @@ class BilledUsersFinder
   def group_billed_user_ids
     @group_billed_user_ids ||= group.billed_user_ids
   end
+
+  def user_ids
+    group_billed_user_ids[:user_ids]
+  end
 end

ee/app/models/ee/group.rb

@@ -297,31 +297,21 @@ module EE
 
     override :billable_members_count
     def billable_members_count(requested_hosted_plan = nil)
-      billed_user_ids(requested_hosted_plan).count
+      billable_ids = billed_user_ids(requested_hosted_plan)
+
+      billable_ids[:user_ids].count
     end
 
     # For now, we are not billing for members with a Guest role for subscriptions
     # with a Gold/Ultimate plan. The other plans will treat Guest members as a regular member
     # for billing purposes.
     #
-    # We are plucking the user_ids from the "Members" table in an array and
+    # For the user_ids key, we are plucking the user_ids from the "Members" table in an array and
     # converting the array of user_ids to a Set which will have unique user_ids.
     def billed_user_ids(requested_hosted_plan = nil)
-      if ([actual_plan_name, requested_hosted_plan] & [::Plan::GOLD, ::Plan::ULTIMATE]).any?
-        strong_memoize(:billed_user_ids) do
-          (billed_group_members.non_guests.distinct.pluck(:user_id) +
-          billed_project_members.non_guests.distinct.pluck(:user_id) +
-          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_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_to_project_members.distinct.pluck(:user_id)).to_set
-        end
-      end
+      exclude_guests = ([actual_plan_name, requested_hosted_plan] & [::Plan::GOLD, ::Plan::ULTIMATE]).any?
+
+      exclude_guests ? billed_user_ids_excluding_guests : billed_user_ids_including_guests
     end
 
     override :supports_events?
@@ -518,6 +508,40 @@ module EE
       errors.add(:custom_project_templates_group_id, 'has to be a subgroup of the group')
     end
 
+    def billed_user_ids_excluding_guests
+      strong_memoize(:billed_user_ids_excluding_guests) do
+        group_member_ids = billed_group_members.non_guests.distinct.pluck(:user_id)
+        project_member_ids = billed_project_members.non_guests.distinct.pluck(:user_id)
+        shared_group_ids = billed_shared_non_guests_group_members.non_guests.distinct.pluck(:user_id)
+        shared_project_ids = billed_invited_non_guests_group_to_project_members.non_guests.distinct.pluck(:user_id)
+
+        {
+          user_ids: (group_member_ids + project_member_ids + shared_group_ids + shared_project_ids).to_set,
+          group_member_user_ids: group_member_ids.to_set,
+          project_member_user_ids: project_member_ids.to_set,
+          shared_group_user_ids: shared_group_ids.to_set,
+          shared_project_user_ids: shared_project_ids.to_set
+        }
+      end
+    end
+
+    def billed_user_ids_including_guests
+      strong_memoize(:billed_user_ids_including_guests) do
+        group_member_ids = billed_group_members.distinct.pluck(:user_id)
+        project_member_ids = billed_project_members.distinct.pluck(:user_id)
+        shared_group_ids = billed_shared_group_members.distinct.pluck(:user_id)
+        shared_project_ids = billed_invited_group_to_project_members.distinct.pluck(:user_id)
+
+        {
+          user_ids: (group_member_ids + project_member_ids + shared_group_ids + shared_project_ids).to_set,
+          group_member_user_ids: group_member_ids.to_set,
+          project_member_user_ids: project_member_ids.to_set,
+          shared_group_user_ids: shared_group_ids.to_set,
+          shared_project_user_ids: shared_project_ids.to_set
+        }
+      end
+    end
+
     # Members belonging directly to Group or its subgroups
     def billed_group_members
       ::GroupMember.active_without_invites_and_requests.where(

ee/app/models/ee/namespace.rb

@@ -277,7 +277,13 @@ module EE
     # This method is overwritten in Group where we made the calculation
     # for Group namespaces.
     def billed_user_ids(_requested_hosted_plan = nil)
-      [owner_id]
+      {
+        user_ids: [owner_id],
+        group_member_user_ids: [],
+        project_member_user_ids: [],
+        shared_group_user_ids: [],
+        shared_project_user_ids: []
+      }
     end
 
     def eligible_for_trial?

ee/app/models/ee/user.rb

@@ -282,7 +282,7 @@ module EE
       namespace.present? &&
       active? &&
       !namespace.root_ancestor.free_plan? &&
-      namespace.root_ancestor.billed_user_ids.include?(self.id)
+      namespace.root_ancestor.billed_user_ids[:user_ids].include?(self.id)
     end
 
     def group_sso?(group)

ee/changelogs/unreleased/vij-billable-member-source.yml

+---
+title: Return billable member relationship to a group in REST API
+merge_request: 60600
+author:
+type: changed

ee/lib/ee/api/entities/billable_member.rb

@@ -6,6 +6,20 @@ module EE
       class BillableMember < ::API::Entities::UserBasic
         expose :public_email, as: :email
         expose :last_activity_on
+        expose :membership_type
+
+        private
+
+        def membership_type
+          return 'group_member'   if user_in_array?(:group_member_user_ids)
+          return 'project_member' if user_in_array?(:project_member_user_ids)
+          return 'group_invite'   if user_in_array?(:shared_group_user_ids)
+          return 'project_invite' if user_in_array?(:shared_project_user_ids)
+        end
+
+        def user_in_array?(name)
+          options.fetch(name, []).include?(object.id)
+        end
       end
     end
   end

ee/lib/ee/api/members.rb

@@ -69,13 +69,18 @@ module EE
             bad_request!(nil) unless ::Ability.allowed?(current_user, :admin_group_member, group)
 
             sorting = params[:sort] || 'id_asc'
-            users = paginate(
-              BilledUsersFinder.new(group,
-                                    search_term: params[:search],
-                                    order_by: sorting).execute
-            )
 
-            present users, with: ::EE::API::Entities::BillableMember, current_user: current_user
+            result = BilledUsersFinder.new(group,
+                                             search_term: params[:search],
+                                             order_by: sorting).execute
+
+            present paginate(result[:users]),
+                    with: ::EE::API::Entities::BillableMember,
+                    current_user: current_user,
+                    group_member_user_ids: result[:group_member_user_ids],
+                    project_member_user_ids: result[:project_member_user_ids],
+                    shared_group_user_ids: result[:shared_group_user_ids],
+                    shared_project_user_ids: result[:shared_project_user_ids]
           end
 
           desc 'Get the memberships of a billable user of a root group.' do
@@ -93,7 +98,7 @@ module EE
 
             user = ::User.find(params[:user_id])
 
-            not_found!('User') unless group.billed_user_ids.include?(user.id)
+            not_found!('User') unless group.billed_user_ids[:user_ids].include?(user.id)
 
             memberships = user.members.in_hierarchy(group).including_source
 

ee/spec/finders/billed_users_finder_spec.rb

@@ -3,7 +3,7 @@
 require 'spec_helper'
 
 RSpec.describe BilledUsersFinder do
-  let_it_be(:group) { create(:group) }
+  let_it_be_with_refind(:group) { create(:group) }
 
   let(:search_term) { nil }
   let(:order_by) { nil }
@@ -17,10 +17,10 @@ RSpec.describe BilledUsersFinder do
     subject { described_class.new(group, search_term: search_term, order_by: order_by) }
 
     context 'when a group does not have any billed users' do
-      it 'returns an empty array' do
-        allow(group).to receive(:billed_user_ids).and_return([])
+      it 'returns an empty object' do
+        allow(group).to receive(:billed_user_ids).and_return({ user_ids: [] })
 
-        expect(subject.execute).to be_empty
+        expect(subject.execute).to eq({})
       end
     end
 
@@ -31,7 +31,7 @@ RSpec.describe BilledUsersFinder do
         let(:order_by) { 'name_desc' }
 
         it 'sorts results accordingly' do
-          expect(subject.execute).to eq([john_smith, john_doe].map(&:user))
+          expect(subject.execute[:users]).to eq([john_smith, john_doe].map(&:user))
         end
       end
 
@@ -39,7 +39,7 @@ RSpec.describe BilledUsersFinder do
         subject { described_class.new(group, search_term: search_term) }
 
         it 'sorts expected results in name_asc order' do
-          expect(subject.execute).to eq([john_doe, john_smith].map(&:user))
+          expect(subject.execute[:users]).to eq([john_doe, john_smith].map(&:user))
         end
       end
     end
@@ -50,7 +50,7 @@ RSpec.describe BilledUsersFinder do
       it 'returns expected users in name asc order when a sorting is not provided either' do
         allow(group).to receive(:billed_user_members).and_return([john_doe, john_smith, sophie, maria])
 
-        expect(subject.execute).to eq([john_doe, john_smith, maria, sophie].map(&:user))
+        expect(subject.execute[:users]).to eq([john_doe, john_smith, maria, sophie].map(&:user))
       end
 
       context 'and when a sorting parameter is provided (eg name descending)' do
@@ -59,7 +59,38 @@ RSpec.describe BilledUsersFinder do
         subject { described_class.new(group, search_term: search_term, order_by: order_by) }
 
         it 'sorts results accordingly' do
-          expect(subject.execute).to eq([sophie, maria, john_smith, john_doe].map(&:user))
+          expect(subject.execute[:users]).to eq([sophie, maria, john_smith, john_doe].map(&:user))
+        end
+      end
+    end
+
+    context 'with billable group members including shared members' do
+      let_it_be(:shared_with_group_member) { create(:group_member, user: create(:user, name: 'Shared Group User')) }
+      let_it_be(:shared_with_project_member) { create(:group_member, user: create(:user, name: 'Shared Project User')) }
+      let_it_be(:project) { create(:project, group: group) }
+
+      before do
+        create(:group_group_link, shared_group: group, shared_with_group: shared_with_group_member.group)
+        create(:project_group_link, group: shared_with_project_member.group, project: project)
+      end
+
+      it 'returns a hash of users and user ids' do
+        expect(subject.execute.keys).to eq([
+          :users,
+          :group_member_user_ids,
+          :project_member_user_ids,
+          :shared_group_user_ids,
+          :shared_project_user_ids
+        ])
+      end
+
+      it 'returns the correct user ids' do
+        result = subject.execute
+
+        aggregate_failures do
+          expect(result[:group_member_user_ids]).to contain_exactly(*[maria, john_smith, john_doe, sophie].map(&:user_id))
+          expect(result[:shared_group_user_ids]).to contain_exactly(shared_with_group_member.user_id)
+          expect(result[:shared_project_user_ids]).to contain_exactly(shared_with_project_member.user_id)
         end
       end
     end

ee/spec/lib/ee/api/entities/billable_member_spec.rb

@@ -5,9 +5,17 @@ require 'spec_helper'
 RSpec.describe ::EE::API::Entities::BillableMember do
   let(:last_activity_on) { Date.today }
   let(:public_email) { nil }
-  let(:member) { build(:user, public_email: public_email, email: 'private@email.com', last_activity_on: last_activity_on) }
+  let(:member) { build(:user, id: non_existing_record_id, public_email: public_email, email: 'private@email.com', last_activity_on: last_activity_on) }
+  let(:options) do
+    {
+      group_member_user_ids: [],
+      project_member_user_ids: [],
+      shared_group_user_ids: [],
+      shared_project_user_ids: []
+    }
+  end
 
-  subject(:entity_representation) { described_class.new(member).as_json }
+  subject(:entity_representation) { described_class.new(member, options).as_json }
 
   it 'returns the last_activity_on attribute' do
     expect(entity_representation[:last_activity_on]).to eq last_activity_on
@@ -35,4 +43,34 @@ RSpec.describe ::EE::API::Entities::BillableMember do
       end
     end
   end
+
+  context 'with different group membership types' do
+    using RSpec::Parameterized::TableSyntax
+
+    where(:user_ids, :expected_value) do
+      :group_member_user_ids   | 'group_member'
+      :project_member_user_ids | 'project_member'
+      :shared_group_user_ids   | 'group_invite'
+      :shared_project_user_ids | 'project_invite'
+    end
+
+    with_them do
+      let(:options) { super().merge(user_ids => [member.id]) }
+
+      it 'returns the expected value' do
+        expect(entity_representation[:membership_type]).to eq expected_value
+      end
+    end
+
+    context 'with a missing membership type' do
+      before do
+        options.delete(:group_member_user_ids)
+      end
+
+      it 'does not raise an error' do
+        expect(options[:group_member_user_ids]).to be_nil
+        expect { entity_representation }.not_to raise_error
+      end
+    end
+  end
 end

ee/spec/models/ee/namespace_spec.rb

@@ -768,7 +768,14 @@ RSpec.describe Namespace do
       let(:user) { create(:user) }
 
       it 'returns 1' do
-        expect(user.namespace.billed_user_ids).to eq([user.id])
+        expect(user.namespace.billed_user_ids.keys).to eq([
+          :user_ids,
+          :group_member_user_ids,
+          :project_member_user_ids,
+          :shared_group_user_ids,
+          :shared_project_user_ids
+        ])
+        expect(user.namespace.billed_user_ids[:user_ids]).to eq([user.id])
       end
     end
 
@@ -783,13 +790,25 @@ RSpec.describe Namespace do
         group.add_guest(guest)
       end
 
+      subject(:billed_user_ids) { group.billed_user_ids }
+
+      it 'returns a breakdown of billable user ids' do
+        expect(billed_user_ids.keys).to eq([
+          :user_ids,
+          :group_member_user_ids,
+          :project_member_user_ids,
+          :shared_group_user_ids,
+          :shared_project_user_ids
+        ])
+      end
+
       context 'with a ultimate plan' do
         before do
           create(:gitlab_subscription, namespace: group, hosted_plan: ultimate_plan)
         end
 
         it 'does not include guest users and only active users' do
-          expect(group.billed_user_ids).to match_array([developer.id])
+          expect(billed_user_ids[:user_ids]).to match_array([developer.id])
         end
 
         context 'when group has a project and users are invited to it' do
@@ -803,14 +822,19 @@ RSpec.describe Namespace do
             project.add_developer(create(:user, :blocked))
           end
 
-          it 'includes invited active users except guests to the group' do
-            expect(group.billed_user_ids).to match_array([project_developer.id, developer.id])
+          it 'includes invited active users except guests to the group', :aggregate_failures do
+            expect(billed_user_ids[:user_ids]).to match_array([project_developer.id, developer.id])
+            expect(billed_user_ids[:project_member_user_ids]).to match_array([project_developer.id, developer.id])
+            expect(billed_user_ids[:group_member_user_ids]).to match_array([developer.id])
+            expect(billed_user_ids[:shared_group_user_ids]).to match_array([])
+            expect(billed_user_ids[:shared_project_user_ids]).to match_array([])
           end
 
           context 'with project bot users' do
             include_context 'project bot users'
 
-            it { expect(group.billed_user_ids).not_to include(project_bot.id) }
+            it { expect(billed_user_ids[:user_ids]).not_to include(project_bot.id) }
+            it { expect(billed_user_ids[:project_member_user_ids]).not_to include(project_bot.id) }
           end
 
           context 'when group is invited to the project' do
@@ -829,8 +853,12 @@ RSpec.describe Namespace 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])
+              it 'includes only active users except guests of the invited groups', :aggregate_failures do
+                expect(billed_user_ids[:user_ids]).to match_array([invited_group_developer.id, project_developer.id, developer.id])
+                expect(billed_user_ids[:shared_group_user_ids]).to match_array([])
+                expect(billed_user_ids[:shared_project_user_ids]).to match_array([invited_group_developer.id, developer.id])
+                expect(billed_user_ids[:group_member_user_ids]).to match_array([developer.id])
+                expect(billed_user_ids[:project_member_user_ids]).to match_array([developer.id, project_developer.id])
               end
             end
 
@@ -839,8 +867,9 @@ RSpec.describe Namespace 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])
+              it 'does not include any members from the invited group', :aggregate_failures do
+                expect(billed_user_ids[:user_ids]).to match_array([project_developer.id, developer.id])
+                expect(billed_user_ids[:shared_project_user_ids]).to be_empty
               end
             end
           end
@@ -860,8 +889,9 @@ RSpec.describe Namespace do
           end
 
           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])
+            expect(billed_user_ids[:user_ids]).to match_array([shared_group_developer.id, developer.id])
+            expect(billed_user_ids[:shared_group_user_ids]).to match_array([shared_group_developer.id])
+            expect(shared_group.billed_user_ids[:user_ids]).not_to include([developer.id])
           end
 
           context 'when subgroup invited another group to collaborate' do
@@ -882,9 +912,10 @@ RSpec.describe Namespace do
               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])
+                expect(billed_user_ids[:user_ids]).to match_array([shared_group_developer.id, developer.id, another_shared_group_developer.id])
+                expect(billed_user_ids[:shared_group_user_ids]).to match_array([shared_group_developer.id, another_shared_group_developer.id])
+                expect(shared_group.billed_user_ids[:user_ids]).not_to include([developer.id])
+                expect(another_shared_group.billed_user_ids[:user_ids]).not_to include([developer.id, shared_group_developer.id])
               end
             end
 
@@ -895,8 +926,9 @@ RSpec.describe Namespace do
                                                     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])
+              it 'does not includes any user from the shared group from the subgroup', :aggregate_failures do
+                expect(billed_user_ids[:user_ids]).to match_array([shared_group_developer.id, developer.id])
+                expect(billed_user_ids[:shared_group_user_ids]).to match_array([shared_group_developer.id])
               end
             end
           end
@@ -905,9 +937,12 @@ RSpec.describe Namespace do
 
       context 'with other plans' do
         %i[bronze_plan premium_plan].each do |plan|
-          it 'includes active guest users' do
+          subject(:billed_user_ids) { group.billed_user_ids }
+
+          it 'includes active guest users', :aggregate_failures do
             create(:gitlab_subscription, namespace: group, hosted_plan: send(plan))
-            expect(group.billed_user_ids).to match_array([guest.id, developer.id])
+            expect(billed_user_ids[:user_ids]).to match_array([guest.id, developer.id])
+            expect(billed_user_ids[:group_member_user_ids]).to match_array([guest.id, developer.id])
           end
 
           context 'when group has a project and users invited to it' do
@@ -923,14 +958,16 @@ RSpec.describe Namespace do
               project.add_developer(developer)
             end
 
-            it 'includes invited active users to the group' do
-              expect(group.billed_user_ids).to match_array([guest.id, developer.id, project_guest.id, project_developer.id])
+            it 'includes invited active users to the group', :aggregate_failures do
+              expect(billed_user_ids[:user_ids]).to match_array([guest.id, developer.id, project_guest.id, project_developer.id])
+              expect(billed_user_ids[:project_member_user_ids]).to match_array([developer.id, project_guest.id, project_developer.id])
             end
 
             context 'with project bot users' do
               include_context 'project bot users'
 
-              it { expect(group.billed_user_ids).not_to include(project_bot.id) }
+              it { expect(billed_user_ids[:user_ids]).not_to include(project_bot.id) }
+              it { expect(billed_user_ids[:project_member_user_ids]).not_to include(project_bot.id) }
             end
 
             context 'when group is invited to the project' do
@@ -946,13 +983,21 @@ RSpec.describe Namespace do
                 create(:project_group_link, project: project, group: invited_group)
               end
 
-              it 'includes the unique active users and guests of the invited groups' do
-                expect(group.billed_user_ids).to match_array([guest.id,
-                                                              developer.id,
-                                                              project_guest.id,
-                                                              project_developer.id,
-                                                              invited_group_developer.id,
-                                                              invited_group_guest.id])
+              it 'includes the unique active users and guests of the invited groups', :aggregate_failures do
+                expect(billed_user_ids[:user_ids]).to match_array([
+                  guest.id,
+                  developer.id,
+                  project_guest.id,
+                  project_developer.id,
+                  invited_group_developer.id,
+                  invited_group_guest.id
+                ])
+
+                expect(billed_user_ids[:shared_project_user_ids]).to match_array([
+                  developer.id,
+                  invited_group_developer.id,
+                  invited_group_guest.id
+                ])
               end
             end
           end
@@ -973,8 +1018,9 @@ RSpec.describe Namespace do
             end
 
             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])
+              expect(billed_user_ids[:user_ids]).to match_array([developer.id, guest.id, shared_group_developer.id, shared_group_guest.id])
+              expect(billed_user_ids[:shared_group_user_ids]).to match_array([shared_group_developer.id, shared_group_guest.id])
+              expect(shared_group.billed_user_ids[:user_ids]).to match_array([shared_group_developer.id, shared_group_guest.id])
             end
           end
         end