Merge branch '21983-member-add_user-doesn-t-detect-existing-members-that-have-requested-access' into 'master'

Resolve "`Member.add_user`doesn't detect existing members that have requested access"

## What does this MR do?

This merge request handle the case when an access requester is added to a group or project (via the members page or the API).

In `Member.add_user`, if an access requester already exists, we simply accept their request (and set the `created_by`, `access_level` and `expires_at` attributes if given).

## Are there points in the code the reviewer needs to double check?

I've taken the opportunity to cleanup the whole `{Group,Project}Member.add_user*` methods since it was quite a mess.

## What are the relevant issue numbers?

Closes #21983

See merge request !6393

2 files changed, 6 insertions, 15 deletions

diff --git a/lib/api/members.rb b/lib/api/members.rb
index 37f0a6512f4..a18ce769e29 100644
--- a/lib/api/members.rb
+++ b/lib/api/members.rb
@@ -59,13 +59,6 @@ module API
           authorize_admin_source!(source_type, source)
           required_attributes! [:user_id, :access_level]
 
-          access_requester = source.requesters.find_by(user_id: params[:user_id])
-          if access_requester
-            # We pass current_user = access_requester so that the requester doesn't
-            # receive a "access denied" email
-            ::Members::DestroyService.new(access_requester, access_requester.user).execute
-          end
-
           member = source.members.find_by(user_id: params[:user_id])
 
           # This is to ensure back-compatibility but 409 behavior should be used
@@ -73,18 +66,12 @@ module API
           conflict!('Member already exists') if source_type == 'group' && member
 
           unless member
-            source.add_user(params[:user_id], params[:access_level], current_user: current_user, expires_at: params[:expires_at])
-            member = source.members.find_by(user_id: params[:user_id])
+            member = source.add_user(params[:user_id], params[:access_level], current_user: current_user, expires_at: params[:expires_at])
           end
 
-          if member
+          if member.persisted? && member.valid?
             present member.user, with: Entities::Member, member: member
           else
-            # Since `source.add_user` doesn't return a member object, we have to
-            # build a new one and populate its errors in order to render them.
-            member = source.members.build(attributes_for_keys([:user_id, :access_level, :expires_at]))
-            member.valid? # populate the errors
-
             # This is to ensure back-compatibility but 400 behavior should be used
             # for all validation errors in 9.0!
             render_api_error!('Access level is not known', 422) if member.errors.key?(:access_level)
diff --git a/lib/gitlab/access.rb b/lib/gitlab/access.rb
index a533bac2692..9b484a2ecfd 100644
--- a/lib/gitlab/access.rb
+++ b/lib/gitlab/access.rb
@@ -53,6 +53,10 @@ module Gitlab
         }
       end
 
+      def sym_options_with_owner
+        sym_options.merge(owner: OWNER)
+      end
+
       def protection_options
         {
           "Not protected: Both developers and masters can push new commits, force push, or delete the branch." => PROTECTION_NONE,