Allow Member.add_user to handle access requesters

Changes include:

- Ensure Member.add_user is not called directly when not necessary
- New GroupMember.add_users_to_group to have the same abstraction level as for Project
- Refactor Member.add_user to take a source instead of an array of members
- Fix Rubocop offenses
- Always use Project#add_user instead of project.team.add_user
- Factorize users addition as members in Member.add_users_to_source
- Make access_level a keyword argument in GroupMember.add_users_to_group and ProjectMember.add_users_to_projects
- Destroy any requester before adding them as a member
- Improve the way we handle access requesters in Member.add_user
  Instead of removing the requester and creating a new member,
  we now simply accepts their access request. This way, they will
  receive a "access request granted" email.
- Fix error that was previously silently ignored
- Stop raising when access level is invalid in Member, let Rails validation do their work

Signed-off-by: Rémy Coutable <remy@rymai.me>

1 files changed, 33 insertions, 17 deletions

diff --git a/spec/models/members/project_member_spec.rb b/spec/models/members/project_member_spec.rb
index 805c15a4e5e..d85a1c1e3b2 100644
--- a/spec/models/members/project_member_spec.rb
+++ b/spec/models/members/project_member_spec.rb
@@ -15,6 +15,26 @@ describe ProjectMember, models: true do
     it { is_expected.to include_module(Gitlab::ShellAdapter) }
   end
 
+  describe '.access_level_roles' do
+    it 'returns Gitlab::Access.options' do
+      expect(described_class.access_level_roles).to eq(Gitlab::Access.options)
+    end
+  end
+
+  describe '.add_user' do
+    context 'when called with the project owner' do
+      it 'adds the user as a member' do
+        project = create(:empty_project)
+
+        expect(project.users).not_to include(project.owner)
+
+        described_class.add_user(project, project.owner, :master, current_user: project.owner)
+
+        expect(project.users.reload).to include(project.owner)
+      end
+    end
+  end
+
   describe '#real_source_type' do
     subject { create(:project_member).real_source_type }
 
@@ -50,7 +70,7 @@ describe ProjectMember, models: true do
     end
   end
 
-  describe :import_team do
+  describe '.import_team' do
     before do
       @project_1 = create :project
       @project_2 = create :project
@@ -81,25 +101,21 @@ describe ProjectMember, models: true do
   end
 
   describe '.add_users_to_projects' do
-    before do
-      @project_1 = create :project
-      @project_2 = create :project
+    it 'adds the given users to the given projects' do
+      projects = create_list(:empty_project, 2)
+      users = create_list(:user, 2)
 
-      @user_1 = create :user
-      @user_2 = create :user
-
-      ProjectMember.add_users_to_projects(
-        [@project_1.id, @project_2.id],
-        [@user_1.id, @user_2.id],
-        ProjectMember::MASTER
-      )
-    end
+      described_class.add_users_to_projects(
+        [projects.first.id, projects.second],
+        [users.first.id, users.second],
+        described_class::MASTER)
 
-    it { expect(@project_1.users).to include(@user_1) }
-    it { expect(@project_1.users).to include(@user_2) }
+      expect(projects.first.users).to include(users.first)
+      expect(projects.first.users).to include(users.second)
 
-    it { expect(@project_2.users).to include(@user_1) }
-    it { expect(@project_2.users).to include(@user_2) }
+      expect(projects.second.users).to include(users.first)
+      expect(projects.second.users).to include(users.second)
+    end
   end
 
   describe '.truncate_teams' do