6 files changed, 112 insertions, 82 deletions

diff --git a/app/models/group.rb b/app/models/group.rb
index aefb94b2ada..a2f88cca828 100644
--- a/app/models/group.rb
+++ b/app/models/group.rb
@@ -102,40 +102,44 @@ class Group < Namespace
     self[:lfs_enabled]
   end
 
-  def add_users(user_ids, access_level, current_user: nil, expires_at: nil)
-    user_ids.each do |user_id|
-      Member.add_user(
-        self.group_members,
-        user_id,
-        access_level,
-        current_user: current_user,
-        expires_at: expires_at
-      )
-    end
+  def add_users(users, access_level, current_user: nil, expires_at: nil)
+    GroupMember.add_users_to_group(
+      self,
+      users,
+      access_level,
+      current_user: current_user,
+      expires_at: expires_at
+    )
   end
 
   def add_user(user, access_level, current_user: nil, expires_at: nil)
-    add_users([user], access_level, current_user: current_user, expires_at: expires_at)
+    GroupMember.add_user(
+      self,
+      user,
+      access_level,
+      current_user: current_user,
+      expires_at: expires_at
+    )
   end
 
   def add_guest(user, current_user = nil)
-    add_user(user, Gitlab::Access::GUEST, current_user: current_user)
+    add_user(user, :guest, current_user: current_user)
   end
 
   def add_reporter(user, current_user = nil)
-    add_user(user, Gitlab::Access::REPORTER, current_user: current_user)
+    add_user(user, :reporter, current_user: current_user)
   end
 
   def add_developer(user, current_user = nil)
-    add_user(user, Gitlab::Access::DEVELOPER, current_user: current_user)
+    add_user(user, :developer, current_user: current_user)
   end
 
   def add_master(user, current_user = nil)
-    add_user(user, Gitlab::Access::MASTER, current_user: current_user)
+    add_user(user, :master, current_user: current_user)
   end
 
   def add_owner(user, current_user = nil)
-    add_user(user, Gitlab::Access::OWNER, current_user: current_user)
+    add_user(user, :owner, current_user: current_user)
   end
 
   def has_owner?(user)
diff --git a/app/models/member.rb b/app/models/member.rb
index 69406379948..265c11ca113 100644
--- a/app/models/member.rb
+++ b/app/models/member.rb
@@ -80,49 +80,70 @@ class Member < ActiveRecord::Base
       find_by(invite_token: invite_token)
     end
 
-    # This method is used to find users that have been entered into the "Add members" field.
-    # These can be the User objects directly, their IDs, their emails, or new emails to be invited.
-    def user_for_id(user_id)
-      return user_id if user_id.is_a?(User)
-
-      user = User.find_by(id: user_id)
-      user ||= User.find_by(email: user_id)
-      user ||= user_id
-      user
-    end
-
-    def add_user(members, user_id, access_level, current_user: nil, expires_at: nil)
-      user = user_for_id(user_id)
+    def add_user(source, user, access_level, current_user: nil, expires_at: nil)
+      user = retrieve_user(user)
+      access_level = retrieve_access_level(access_level)
 
       # `user` can be either a User object or an email to be invited
-      if user.is_a?(User)
-        member = members.find_or_initialize_by(user_id: user.id)
+      member =
+        if user.is_a?(User)
+          source.members.find_by(user_id: user.id) ||
+          source.requesters.find_by(user_id: user.id) ||
+          source.members.build(user_id: user.id)
+        else
+          source.members.build(invite_email: user)
+        end
+
+      return member unless can_update_member?(current_user, member)
+
+      member.attributes = {
+        created_by: member.created_by || current_user,
+        access_level: access_level,
+        expires_at: expires_at
+      }
+
+      if member.request?
+        member.accept_request
       else
-        member = members.build
-        member.invite_email = user
+        member.save
       end
 
-      if can_update_member?(current_user, member) || project_creator?(member, access_level)
-        member.created_by ||= current_user
-        member.access_level = access_level
-        member.expires_at = expires_at
+      member
+    end
 
-        member.save
-      end
+    def access_levels
+      Gitlab::Access.sym_options
     end
 
     private
 
+    # This method is used to find users that have been entered into the "Add members" field.
+    # These can be the User objects directly, their IDs, their emails, or new emails to be invited.
+    def retrieve_user(user)
+      return user if user.is_a?(User)
+
+      User.find_by(id: user) || User.find_by(email: user) || user
+    end
+
+    def retrieve_access_level(access_level)
+      access_levels.fetch(access_level) { access_level.to_i }
+    end
+
     def can_update_member?(current_user, member)
       # There is no current user for bulk actions, in which case anything is allowed
-      !current_user ||
-        current_user.can?(:update_group_member, member) ||
-        current_user.can?(:update_project_member, member)
+      !current_user || current_user.can?(:"update_#{member.type.underscore}", member)
     end
 
-    def project_creator?(member, access_level)
-      member.new_record? && member.owner? &&
-        access_level.to_i == ProjectMember::MASTER
+    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
 
diff --git a/app/models/members/group_member.rb b/app/models/members/group_member.rb
index 2f13d339c89..1b54a85d064 100644
--- a/app/models/members/group_member.rb
+++ b/app/models/members/group_member.rb
@@ -12,6 +12,22 @@ class GroupMember < Member
     Gitlab::Access.options_with_owner
   end
 
+  def self.access_levels
+    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 ec2d40eb11c..125f26369d7 100644
--- a/app/models/members/project_member.rb
+++ b/app/models/members/project_member.rb
@@ -34,36 +34,20 @@ class ProjectMember < Member
     #     :master
     #   )
     #
-    def add_users_to_projects(project_ids, user_ids, access, current_user: nil, expires_at: nil)
-      access_level = if roles_hash.has_key?(access)
-                       roles_hash[access]
-                     elsif roles_hash.values.include?(access.to_i)
-                       access
-                     else
-                       raise "Non valid access"
-                     end
-
-      users = user_ids.map { |user_id| Member.user_for_id(user_id) }
-
-      ProjectMember.transaction do
+    def add_users_to_projects(project_ids, users, access_level, current_user: nil, expires_at: nil)
+      self.transaction do
         project_ids.each do |project_id|
           project = Project.find(project_id)
 
-          users.each do |user|
-            Member.add_user(
-              project.project_members,
-              user,
-              access_level,
-              current_user: current_user,
-              expires_at: expires_at
-            )
-          end
+          add_users_to_source(
+            project,
+            users,
+            access_level,
+            current_user: current_user,
+            expires_at: expires_at
+          )
         end
       end
-
-      true
-    rescue
-      false
     end
 
     def truncate_teams(project_ids)
@@ -84,13 +68,15 @@ class ProjectMember < Member
       truncate_teams [project.id]
     end
 
-    def roles_hash
-      Gitlab::Access.sym_options
-    end
-
     def access_level_roles
       Gitlab::Access.options
     end
+
+    private
+
+    def can_update_member?(current_user, member)
+      super || (member.owner? && member.new_record?)
+    end
   end
 
   def access_field
diff --git a/app/models/project.rb b/app/models/project.rb
index 7265cb55594..507228606df 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -146,6 +146,7 @@ class Project < ActiveRecord::Base
 
   delegate :name, to: :owner, allow_nil: true, prefix: true
   delegate :members, to: :team, prefix: true
+  delegate :add_user, to: :team
 
   # Validations
   validates :creator, presence: true, on: :create
@@ -1016,10 +1017,6 @@ class Project < ActiveRecord::Base
     project_members.find_by(user_id: user)
   end
 
-  def add_user(user, access_level, current_user: nil, expires_at: nil)
-    team.add_user(user, access_level, current_user: current_user, expires_at: expires_at)
-  end
-
   def default_branch
     @default_branch ||= repository.root_ref if repository.exists?
   end
diff --git a/app/models/project_team.rb b/app/models/project_team.rb
index d9ce5088903..79d041d2775 100644
--- a/app/models/project_team.rb
+++ b/app/models/project_team.rb
@@ -33,18 +33,24 @@ class ProjectTeam
     member
   end
 
-  def add_users(users, access, current_user: nil, expires_at: nil)
+  def add_users(users, access_level, current_user: nil, expires_at: nil)
     ProjectMember.add_users_to_projects(
       [project.id],
       users,
-      access,
+      access_level,
       current_user: current_user,
       expires_at: expires_at
     )
   end
 
-  def add_user(user, access, current_user: nil, expires_at: nil)
-    add_users([user], access, current_user: current_user, expires_at: expires_at)
+  def add_user(user, access_level, current_user: nil, expires_at: nil)
+    ProjectMember.add_user(
+      project,
+      user,
+      access_level,
+      current_user: current_user,
+      expires_at: expires_at
+    )
   end
 
   # Remove all users from project team