Refactor add_users method for project and groupdz-cleanup-add-users

Signed-off-by: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>

9 files changed, 53 insertions, 32 deletions

diff --git a/app/models/group.rb b/app/models/group.rb
index 106084175ff..cbc10b00cf5 100644
--- a/app/models/group.rb
+++ b/app/models/group.rb
@@ -125,7 +125,7 @@ class Group < Namespace
   end
 
   def add_users(users, access_level, current_user: nil, expires_at: nil)
-    GroupMember.add_users_to_group(
+    GroupMember.add_users(
       self,
       users,
       access_level,
diff --git a/app/models/member.rb b/app/models/member.rb
index 0545bd4eedf..97fba501759 100644
--- a/app/models/member.rb
+++ b/app/models/member.rb
@@ -151,6 +151,22 @@ class Member < ActiveRecord::Base
       member
     end
 
+    def add_users(source, users, access_level, current_user: nil, expires_at: nil)
+      return [] unless users.present?
+
+      self.transaction do
+        users.map do |user|
+          add_user(
+            source,
+            user,
+            access_level,
+            current_user: current_user,
+            expires_at: expires_at
+          )
+        end
+      end
+    end
+
     def access_levels
       Gitlab::Access.sym_options
     end
@@ -173,18 +189,6 @@ class Member < ActiveRecord::Base
       # There is no current user for bulk actions, in which case anything is allowed
       !current_user || current_user.can?(:"update_#{member.type.underscore}", member)
     end
-
-    def add_users_to_source(source, users, access_level, current_user: nil, expires_at: nil)
-      users.each do |user|
-        add_user(
-          source,
-          user,
-          access_level,
-          current_user: current_user,
-          expires_at: expires_at
-        )
-      end
-    end
   end
 
   def real_source_type
diff --git a/app/models/members/group_member.rb b/app/models/members/group_member.rb
index 483425cd30f..28e10bc6172 100644
--- a/app/models/members/group_member.rb
+++ b/app/models/members/group_member.rb
@@ -21,18 +21,6 @@ class GroupMember < Member
     Gitlab::Access.sym_options_with_owner
   end
 
-  def self.add_users_to_group(group, users, access_level, current_user: nil, expires_at: nil)
-    self.transaction do
-      add_users_to_source(
-        group,
-        users,
-        access_level,
-        current_user: current_user,
-        expires_at: expires_at
-      )
-    end
-  end
-
   def group
     source
   end
diff --git a/app/models/members/project_member.rb b/app/models/members/project_member.rb
index 912820b51ac..b3a91feb091 100644
--- a/app/models/members/project_member.rb
+++ b/app/models/members/project_member.rb
@@ -16,7 +16,7 @@ class ProjectMember < Member
   before_destroy :delete_member_todos
 
   class << self
-    # Add users to project teams with passed access option
+    # Add users to projects with passed access option
     #
     # access can be an integer representing a access code
     # or symbol like :master representing role
@@ -39,7 +39,7 @@ class ProjectMember < Member
         project_ids.each do |project_id|
           project = Project.find(project_id)
 
-          add_users_to_source(
+          add_users(
             project,
             users,
             access_level,
diff --git a/app/models/project_team.rb b/app/models/project_team.rb
index 6d6644053f8..543b9b293e0 100644
--- a/app/models/project_team.rb
+++ b/app/models/project_team.rb
@@ -50,8 +50,8 @@ class ProjectTeam
   end
 
   def add_users(users, access_level, current_user: nil, expires_at: nil)
-    ProjectMember.add_users_to_projects(
-      [project.id],
+    ProjectMember.add_users(
+      project,
       users,
       access_level,
       current_user: current_user,
diff --git a/changelogs/unreleased/dz-cleanup-add-users.yml b/changelogs/unreleased/dz-cleanup-add-users.yml
new file mode 100644
index 00000000000..ba1e2d609f9
--- /dev/null
+++ b/changelogs/unreleased/dz-cleanup-add-users.yml
@@ -0,0 +1,4 @@
+---
+title: Refactor add_users method for project and group
+merge_request: 10850
+author:
diff --git a/spec/controllers/projects/labels_controller_spec.rb b/spec/controllers/projects/labels_controller_spec.rb
index 6a6e9bf378a..05999431d8f 100644
--- a/spec/controllers/projects/labels_controller_spec.rb
+++ b/spec/controllers/projects/labels_controller_spec.rb
@@ -127,7 +127,7 @@ describe Projects::LabelsController do
 
     context 'group owner' do
       before do
-        GroupMember.add_users_to_group(group, [user], :owner)
+        GroupMember.add_users(group, [user], :owner)
       end
 
       it 'gives access' do
diff --git a/spec/models/member_spec.rb b/spec/models/member_spec.rb
index c720cc9f2c2..b0f3657d3b5 100644
--- a/spec/models/member_spec.rb
+++ b/spec/models/member_spec.rb
@@ -386,6 +386,31 @@ describe Member, models: true do
     end
   end
 
+  describe '.add_users' do
+    %w[project group].each do |source_type|
+      context "when source is a #{source_type}" do
+        let!(:source) { create(source_type, :public, :access_requestable) }
+        let!(:user) { create(:user) }
+        let!(:admin) { create(:admin) }
+
+        it 'returns a <Source>Member objects' do
+          members = described_class.add_users(source, [user], :master)
+
+          expect(members).to be_a Array
+          expect(members.first).to be_a "#{source_type.classify}Member".constantize
+          expect(members.first).to be_persisted
+        end
+
+        it 'returns an empty array' do
+          members = described_class.add_users(source, [], :master)
+
+          expect(members).to be_a Array
+          expect(members).to be_empty
+        end
+      end
+    end
+  end
+
   describe '#accept_request' do
     let(:member) { create(:project_member, requested_at: Time.now.utc) }
 
diff --git a/spec/models/members/group_member_spec.rb b/spec/models/members/group_member_spec.rb
index 024380b7ebb..17765b25856 100644
--- a/spec/models/members/group_member_spec.rb
+++ b/spec/models/members/group_member_spec.rb
@@ -13,12 +13,12 @@ describe GroupMember, models: true do
     end
   end
 
-  describe '.add_users_to_group' do
+  describe '.add_users' do
     it 'adds the given users to the given group' do
       group = create(:group)
       users = create_list(:user, 2)
 
-      described_class.add_users_to_group(
+      described_class.add_users(
         group,
         [users.first.id, users.second],
         described_class::MASTER