Merge branch 'dz-cleanup-add-users' into 'master'

Refactor add_users method for project and group

See merge request !10850

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,

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

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

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,

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,

changelogs/unreleased/dz-cleanup-add-users.yml

+---
+title: Refactor add_users method for project and group
+merge_request: 10850
+author:

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

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) }

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