Remove duplication between Group and ProjectMember.

5 files changed, 75 insertions, 43 deletions

diff --git a/app/controllers/projects/project_members_controller.rb b/app/controllers/projects/project_members_controller.rb
index 9831a2089be..3da44cbc9d6 100644
--- a/app/controllers/projects/project_members_controller.rb
+++ b/app/controllers/projects/project_members_controller.rb
@@ -52,8 +52,7 @@ class Projects::ProjectMembersController < Projects::ApplicationController
 
     respond_to do |format|
       format.html do
-        redirect_to namespace_project_project_members_path(@project.namespace,
-                                                      @project)
+        redirect_to namespace_project_project_members_path(@project.namespace, @project)
       end
       format.js { render nothing: true }
     end
diff --git a/app/models/group.rb b/app/models/group.rb
index 2de397f90f7..1386a9eccc9 100644
--- a/app/models/group.rb
+++ b/app/models/group.rb
@@ -47,25 +47,8 @@ class Group < Namespace
   end
 
   def add_users(user_ids, access_level, current_user = nil)
-    users = user_ids.map do |user_id|
-      (user_id if user_id.is_a?(User)) ||
-        User.find_by(id: user_id) || 
-        User.find_by(email: user_id) || 
-        user_id
-    end
-
-    users.compact.each do |user|
-      if user.is_a?(User)
-        member = self.group_members.find_or_initialize_by(user_id: user.id)
-      else
-        member = self.group_members.build
-        member.invite_email = user
-      end
-
-      member.created_by ||= current_user
-      member.access_level = access_level
-
-      member.save
+    user_ids.each do |user_id|
+      Member.add_user(self.group_members, user_id, access_level, current_user)
     end
   end
 
diff --git a/app/models/member.rb b/app/models/member.rb
index 2421222eaa2..9340c442172 100644
--- a/app/models/member.rb
+++ b/app/models/member.rb
@@ -52,9 +52,39 @@ class Member < ActiveRecord::Base
 
   delegate :name, :username, :email, to: :user, prefix: true
 
-  def self.find_by_invite_token(invite_token)
-    invite_token = Devise.token_generator.digest(self, :invite_token, invite_token)
-    find_by(invite_token: invite_token)
+  class << self
+    def find_by_invite_token(invite_token)
+      invite_token = Devise.token_generator.digest(self, :invite_token, invite_token)
+      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)
+      user = user_for_id(user_id)
+      
+      # `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)
+      else
+        member = members.build
+        member.invite_email = user
+      end
+
+      member.created_by ||= current_user
+      member.access_level = access_level
+
+      member.save
+    end
   end
 
   def invite?
diff --git a/app/models/members/project_member.rb b/app/models/members/project_member.rb
index 8af7499dd82..ce704a60d63 100644
--- a/app/models/members/project_member.rb
+++ b/app/models/members/project_member.rb
@@ -60,29 +60,14 @@ class ProjectMember < Member
                        raise "Non valid access"
                      end
 
-      users = user_ids.map do |user_id|
-        (user_id if user_id.is_a?(User)) ||
-          User.find_by(id: user_id) || 
-          User.find_by(email: user_id) || 
-          user_id
-      end
+      users = user_ids.map { |user_id| Member.user_for_id(user_id) }
 
       ProjectMember.transaction do
         project_ids.each do |project_id|
           project = Project.find(project_id)
 
           users.each do |user|
-            if user.is_a?(User)
-              member = project.project_members.find_or_initialize_by(user_id: user.id)
-            else
-              member = project.project_members.build
-              member.invite_email = user
-            end
-
-            member.created_by ||= current_user
-            member.access_level = access_level
-
-            member.save
+            Member.add_user(project.project_members, user, access_level, current_user)
           end
         end
       end
diff --git a/spec/models/members_spec.rb b/spec/models/member_spec.rb
index c3e47ab7e9a..a27931cd4e4 100644
--- a/spec/models/members_spec.rb
+++ b/spec/models/member_spec.rb
@@ -58,6 +58,43 @@ describe Member do
     it { is_expected.to respond_to(:user_email) }
   end
 
+  describe ".add_user" do
+    let!(:user)    { create(:user) }
+    let(:project) { create(:project) }
+
+    context "when called with a user id" do
+      it "adds the user as a member" do
+        Member.add_user(project.project_members, user.id, ProjectMember::MASTER)
+
+        expect(project.users).to include(user)
+      end
+    end
+
+    context "when called with a user object" do
+      it "adds the user as a member" do
+        Member.add_user(project.project_members, user, ProjectMember::MASTER)
+
+        expect(project.users).to include(user)
+      end
+    end
+
+    context "when called with a known user email" do
+      it "adds the user as a member" do
+        Member.add_user(project.project_members, user.email, ProjectMember::MASTER)
+
+        expect(project.users).to include(user)
+      end
+    end
+
+    context "when called with an unknown user email" do
+      it "adds a member invite" do
+        Member.add_user(project.project_members, "user@example.com", ProjectMember::MASTER)
+
+        expect(project.project_members.invite.pluck(:invite_email)).to include("user@example.com")
+      end
+    end
+  end
+
   describe "#accept_invite!" do
 
     let!(:member) { create(:project_member, invite_email: "user@example.com", user: nil) }
@@ -89,7 +126,6 @@ describe Member do
   end
 
   describe "#decline_invite!" do
-
     let!(:member) { create(:project_member, invite_email: "user@example.com", user: nil) }
 
     it "destroys the member" do
@@ -106,7 +142,6 @@ describe Member do
   end
 
   describe "#generate_invite_token" do
-
     let!(:member) { create(:project_member, invite_email: "user@example.com", user: nil) }
     
     it "sets the invite token" do