Merge branch 'vij-modify-approve-member-endpoint' into 'master'

Modify pending member approval endpoint

See merge request gitlab-org/gitlab!75769

doc/api/members.md

@@ -601,18 +601,18 @@ curl --request DELETE --header "PRIVATE-TOKEN: <your_access_token>" "https://git
 Approves a pending user for a group and its subgroups and projects.
 
 ```plaintext
-PUT /groups/:id/members/:user_id/approve
+PUT /groups/:id/members/:member_id/approve
 ```
 
 | Attribute | Type | Required | Description |
 | --------- | ---- | -------- | ----------- |
 | `id`      | integer/string | yes | The ID or [URL-encoded path of the root group](index.md#namespaced-path-encoding) owned by the authenticated user |
-| `user_id` | integer | yes   | The user ID of the member |
+| `member_id` | integer | yes   | The ID of the member |
 
 Example request:
 
 ```shell
-curl --request PUT --header "PRIVATE-TOKEN: <your_access_token>" "https://gitlab.example.com/api/v4/groups/:id/members/:user_id/approve"
+curl --request PUT --header "PRIVATE-TOKEN: <your_access_token>" "https://gitlab.example.com/api/v4/groups/:id/members/:member_id/approve"
 ```
 
 ## Approve all pending members for a group

ee/app/services/members/activate_service.rb

@@ -14,9 +14,9 @@ module Members
   class ActivateService
     include BaseServiceUtility
 
-    def initialize(group, user: nil, activate_all: false, current_user:)
+    def initialize(group, member: nil, activate_all: false, current_user:)
       @group = group
-      @user = user
+      @member = member
       @current_user = current_user
       @activate_all = activate_all
     end
@@ -24,6 +24,7 @@ module Members
     def execute
       return error(_('No group provided')) unless group
       return error(_('You do not have permission to approve a member'), :forbidden) unless allowed?
+      return error(_('No member provided')) unless activate_all || member
 
       if activate_memberships
         log_event
@@ -36,11 +37,11 @@ module Members
 
     private
 
-    attr_reader :current_user, :group, :user, :activate_all
+    attr_reader :current_user, :group, :member, :activate_all
 
     def activate_memberships
       memberships_found = false
-      memberships = activate_all ? awaiting_memberships : user_memberships
+      memberships = activate_all ? awaiting_memberships : scoped_memberships
 
       memberships.find_each do |member|
         memberships_found = true
@@ -52,8 +53,12 @@ module Members
     end
 
     # rubocop: disable CodeReuse/ActiveRecord
-    def user_memberships
-      awaiting_memberships.where(user_id: user.id)
+    def scoped_memberships
+      if member.invite?
+        awaiting_memberships.where(invite_email: member.invite_email)
+      else
+        awaiting_memberships.where(user_id: member.user_id)
+      end
     end
     # rubocop: enable CodeReuse/ActiveRecord
 
@@ -71,7 +76,7 @@ module Members
         approved_by: current_user.id
       }.tap do |params|
         params[:message] = activate_all ? 'Approved all pending group members' : 'Group member access approved'
-        params[:user] = user.id unless activate_all
+        params[:member] = member.id unless activate_all
       end
 
       Gitlab::AppLogger.info(log_params)

ee/lib/ee/api/members.rb

@@ -56,17 +56,18 @@ module EE
 
           desc 'Approves a pending member'
           params do
-            requires :user_id, type: Integer, desc: 'The user ID of the member requiring approval'
+            requires :member_id, type: Integer, desc: 'The ID of the member requiring approval'
           end
-          put ':id/members/:user_id/approve' do
+          put ':id/members/:member_id/approve' do
             group = find_group!(params[:id])
-            user = ::User.find(params[:user_id])
+            member = ::Member.find_by_id(params[:member_id])
 
+            not_found! unless member
             bad_request! unless group.root?
             bad_request! unless can?(current_user, :admin_group_member, group)
 
             result = ::Members::ActivateService
-              .new(group, user: user, current_user: current_user)
+              .new(group, member: member, current_user: current_user)
               .execute
 
             if result[:status] == :success

ee/spec/requests/api/members_spec.rb

@@ -960,22 +960,24 @@ RSpec.describe API::Members do
       group.add_owner(owner)
     end
 
-    describe 'PUT /groups/:id/members/:user_id/approve' do
-      let(:url) { "/groups/#{group.id}/members/#{developer.id}/approve" }
+    describe 'PUT /groups/:id/members/:member_id/approve' do
+      let_it_be(:member) { create(:group_member, :awaiting, group: group, user: developer) }
+
+      let(:url) { "/groups/#{group.id}/members/#{member.id}/approve" }
 
       context 'with invalid params' do
         context 'when a subgroup is used' do
-          let(:url) { "/groups/#{subgroup.id}/members/#{developer.id}/approve" }
+          let(:url) { "/groups/#{subgroup.id}/members/#{member.id}/approve" }
 
           it 'returns a bad request response' do
-            put api(url, not_an_owner)
+            put api(url, owner)
 
             expect(response).to have_gitlab_http_status(:bad_request)
           end
         end
 
         context 'when no group is found' do
-          let(:url) { "/groups/#{non_existing_record_id}/members/#{developer.id}/approve" }
+          let(:url) { "/groups/#{non_existing_record_id}/members/#{member.id}/approve" }
 
           it 'returns a not found response' do
             put api(url, owner)
@@ -994,7 +996,7 @@ RSpec.describe API::Members do
       end
 
       context 'when the current user has permission to approve' do
-        context 'when the user is not found' do
+        context 'when the member is not found' do
           let(:url) { "/groups/#{group.id}/members/#{non_existing_record_id}/approve" }
 
           it 'returns not found response' do
@@ -1004,7 +1006,9 @@ RSpec.describe API::Members do
           end
         end
 
-        context 'when the activation fails due to no members to activate' do
+        context 'when the activation fails due to no pending members to activate' do
+          let(:member) { create(:group_member, :active, group: group) }
+
           it 'returns a bad request response' do
             put api(url, owner)
 
@@ -1012,38 +1016,35 @@ RSpec.describe API::Members do
           end
         end
 
-        context 'when the user is a pending member of a root group' do
+        shared_examples 'successful activation' do
           it 'activates the member' do
-            pending_member = create(:group_member, :awaiting, group: group, user: developer)
             put api(url, owner)
 
             expect(response).to have_gitlab_http_status(:success)
-            expect(pending_member.reload.active?).to be true
+            expect(member.reload.active?).to be true
           end
         end
 
-        context 'when the user is a pending member of a subgroup' do
-          it 'activates the member' do
-            pending_member = create(:group_member, :awaiting, group: subgroup, user: developer)
+        context 'when the member is a root group member' do
+          it_behaves_like 'successful activation'
+        end
 
-            put api(url, owner)
+        context 'when the member is a subgroup member' do
+          let(:member) { create(:group_member, :awaiting, group: subgroup) }
 
-            expect(response).to have_gitlab_http_status(:success)
-            expect(pending_member.reload.active?).to be true
-          end
+          it_behaves_like 'successful activation'
         end
 
-        context 'when the user is a pending member of a project' do
-          let(:developer) { create(:user) }
+        context 'when the member is a project member' do
+          let(:member) { create(:project_member, :awaiting, project: project) }
 
-          it 'activates the member' do
-            pending_member = create(:project_member, :awaiting, project: project, user: developer)
+          it_behaves_like 'successful activation'
+        end
 
-            put api(url, owner)
+        context 'when the member is an invited user' do
+          let(:member) { create(:group_member, :awaiting, :invited, group: group) }
 
-            expect(response).to have_gitlab_http_status(:success)
-            expect(pending_member.reload.active?).to be true
-          end
+          it_behaves_like 'successful activation'
         end
       end
     end

ee/spec/services/members/activate_service_spec.rb

@@ -10,10 +10,11 @@ RSpec.describe Members::ActivateService do
     let_it_be(:project) { create(:project, group: root_group) }
     let_it_be(:sub_group) { create(:group, parent: root_group) }
 
+    let(:member) { nil }
     let(:group) { root_group }
     let(:activate_all) { false }
 
-    subject(:execute) { described_class.new(group, user: user, current_user: current_user, activate_all: activate_all).execute }
+    subject(:execute) { described_class.new(group, member: member, current_user: current_user, activate_all: activate_all).execute }
 
     context 'when unauthorized' do
       it 'returns an access error' do
@@ -35,7 +36,7 @@ RSpec.describe Members::ActivateService do
       end
     end
 
-    shared_examples 'successful user activation' do
+    shared_examples 'successful member activation' do
       before do
         expect(member.awaiting?).to be true
       end
@@ -49,7 +50,7 @@ RSpec.describe Members::ActivateService do
         expect(Gitlab::AppLogger).to receive(:info).with(
           message: "Group member access approved",
           group: group.id,
-          user: user.id,
+          member: member.id,
           approved_by: current_user.id
         )
 
@@ -62,28 +63,39 @@ RSpec.describe Members::ActivateService do
         group.add_owner(current_user)
       end
 
-      context 'when activating an individual user' do
-        context 'when user is an awaiting member of a root group' do
-          it_behaves_like 'successful user activation' do
+      context 'when activating an individual member' do
+        context 'when no member is provided' do
+          it 'returns an error' do
+            result = execute
+
+            expect(result[:status]).to eq :error
+            expect(result[:message]).to eq 'No member provided'
+          end
+        end
+
+        context 'when member is an awaiting member of a root group' do
+          it_behaves_like 'successful member activation' do
             let(:member) { create(:group_member, :awaiting, group: root_group, user: user) }
           end
         end
 
-        context 'when user is an awaiting member of a sub-group' do
+        context 'when member is an awaiting member of a sub-group' do
           let(:group) { sub_group }
 
-          it_behaves_like 'successful user activation' do
+          it_behaves_like 'successful member activation' do
             let(:member) { create(:group_member, :awaiting, group: sub_group, user: user) }
           end
         end
 
-        context 'when user is an awaiting member of a project' do
-          it_behaves_like 'successful user activation' do
+        context 'when member is an awaiting member of a project' do
+          it_behaves_like 'successful member activation' do
             let(:member) { create(:project_member, :awaiting, project: project, user: user) }
           end
         end
 
-        context 'when user is not an awaiting member' do
+        context 'when member is not an awaiting member' do
+          let(:member) { create(:group_member, :active, group: root_group, user: user) }
+
           it 'returns an error' do
             result = execute
 
@@ -91,6 +103,30 @@ RSpec.describe Members::ActivateService do
             expect(result[:message]).to eq 'No memberships found'
           end
         end
+
+        context 'when there are multiple awaiting member records in the hierarchy' do
+          context 'for existing members' do
+            let_it_be(:member) { create(:group_member, :awaiting, group: root_group, user: user) }
+            let_it_be(:sub_member) { create(:group_member, :awaiting, group: sub_group, user: user) }
+
+            it 'activates the members' do
+              expect(execute[:status]).to eq :success
+              expect(member.reload.active?).to be true
+              expect(sub_member.reload.active?).to be true
+            end
+          end
+
+          context 'for invited members' do
+            let_it_be(:member) { create(:group_member, :awaiting, :invited, group: root_group) }
+            let_it_be(:sub_member) { create(:group_member, :awaiting, :invited, group: sub_group, invite_email: member.invite_email) }
+
+            it 'activates the members' do
+              expect(execute[:status]).to eq :success
+              expect(member.reload.active?).to be true
+              expect(sub_member.reload.active?).to be true
+            end
+          end
+        end
       end
 
       context 'when activating all awaiting members' do
@@ -98,7 +134,7 @@ RSpec.describe Members::ActivateService do
         let!(:sub_group_members) { create_list(:group_member, 5, :awaiting, group: sub_group) }
         let!(:project_members) { create_list(:project_member, 5, :awaiting, project: project) }
 
-        let(:user) { nil }
+        let(:member) { nil }
         let(:activate_all) { true }
 
         it 'activates all awaiting group members' do

locale/gitlab.pot

@@ -23640,6 +23640,9 @@ msgstr ""
 msgid "No matching results..."
 msgstr ""
 
+msgid "No member provided"
+msgstr ""
+
 msgid "No members found"
 msgstr ""