Refactor insite service for members api

- streamline code and cleanup.

app/services/members/invite_service.rb

@@ -2,112 +2,97 @@
 
 module Members
   class InviteService < Members::BaseService
-    DEFAULT_LIMIT = 100
+    BlankEmailsError = Class.new(StandardError)
+    TooManyEmailsError = Class.new(StandardError)
 
-    attr_reader :errors
+    def initialize(*args)
+      super
 
-    def initialize(current_user, params)
-      @current_user, @params = current_user, params.dup
       @errors = {}
+      @emails = params[:email]&.split(',')&.uniq&.flatten
     end
 
     def execute(source)
-      return error(s_('Email cannot be blank')) if params[:email].blank?
+      validate_emails!
 
-      emails = params[:email].split(',').uniq.flatten
-      return error(s_("Too many users specified (limit is %{user_limit})") % { user_limit: user_limit }) if
-        user_limit && emails.size > user_limit
-
-      emails.each do |email|
-        next if existing_member?(source, email)
-        next if existing_invite?(source, email)
-        next if existing_request?(source, email)
-
-        if existing_user?(email)
-          add_existing_user_as_member(current_user, source, params, email)
-          next
-        end
-
-        invite_new_member_and_user(current_user, source, params, email)
-      end
-
-      return success unless errors.any?
-
-      error(errors)
+      @source = source
+      emails.each(&method(:process_email))
+      result
+    rescue BlankEmailsError, TooManyEmailsError => e
+      error(e.message)
     end
 
     private
 
-    def invite_new_member_and_user(current_user, source, params, email)
-      new_member = (source.class.name + 'Member').constantize.create(source_id: source.id,
-        user_id: nil,
-        access_level: params[:access_level],
-        invite_email: email,
-        created_by_id: current_user.id,
-        expires_at: params[:expires_at])
-
-      unless new_member.valid? && new_member.persisted?
-        errors[params[:email]] = new_member.errors.full_messages.to_sentence
-      end
-    end
+    attr_reader :source, :errors, :emails
 
-    def add_existing_user_as_member(current_user, source, params, email)
-      new_member = create_member(current_user, existing_user(email), source, params.merge({ invite_email: email }))
+    def validate_emails!
+      raise BlankEmailsError, s_('AddMember|Email cannot be blank') if emails.blank?
 
-      unless new_member.valid? && new_member.persisted?
-        errors[email] = new_member.errors.full_messages.to_sentence
+      if user_limit && emails.size > user_limit
+        raise TooManyEmailsError, s_("AddMember|Too many users specified (limit is %{user_limit})") % { user_limit: user_limit }
       end
     end
 
-    def create_member(current_user, user, source, params)
-      source.add_user(user, params[:access_level], current_user: current_user, expires_at: params[:expires_at])
+    def user_limit
+      limit = params.fetch(:limit, Members::CreateService::DEFAULT_LIMIT)
+
+      limit < 0 ? nil : limit
     end
 
-    def user_limit
-      limit = params.fetch(:limit, DEFAULT_LIMIT)
+    def process_email(email)
+      return if existing_member?(email)
+      return if existing_invite?(email)
+      return if existing_request?(email)
 
-      limit && limit < 0 ? nil : limit
+      add_member(email)
     end
 
-    def existing_member?(source, email)
+    def existing_member?(email)
       existing_member = source.members.with_user_by_email(email).exists?
 
       if existing_member
-        errors[email] = "Already a member of #{source.name}"
+        errors[email] = s_("AddMember|Already a member of %{source_name}") % { source_name: source.name }
         return true
       end
 
       false
     end
 
-    def existing_invite?(source, email)
+    def existing_invite?(email)
       existing_invite = source.members.search_invite_email(email).exists?
 
       if existing_invite
-        errors[email] = "Member already invited to #{source.name}"
+        errors[email] = s_("AddMember|Member already invited to %{source_name}") % { source_name: source.name }
         return true
       end
 
       false
     end
 
-    def existing_request?(source, email)
+    def existing_request?(email)
       existing_request = source.requesters.with_user_by_email(email).exists?
 
       if existing_request
-        errors[email] = "Member cannot be invited because they already requested to join #{source.name}"
+        errors[email] = s_("AddMember|Member cannot be invited because they already requested to join %{source_name}") % { source_name: source.name }
         return true
       end
 
       false
     end
 
-    def existing_user(email)
-      User.find_by_email(email)
+    def add_member(email)
+      new_member = source.add_user(email, params[:access_level], current_user: current_user, expires_at: params[:expires_at])
+
+      errors[email] = new_member.errors.full_messages.to_sentence if new_member.invalid?
     end
 
-    def existing_user?(email)
-      existing_user(email).present?
+    def result
+      if errors.any?
+        error(errors)
+      else
+        success
+      end
     end
   end
 end

locale/gitlab.pot

@@ -1951,9 +1951,21 @@ msgstr ""
 msgid "AddContextCommits|Add/remove"
 msgstr ""
 
+msgid "AddMember|Already a member of %{source_name}"
+msgstr ""
+
+msgid "AddMember|Email cannot be blank"
+msgstr ""
+
 msgid "AddMember|Invite limit of %{daily_invites} per day exceeded"
 msgstr ""
 
+msgid "AddMember|Member already invited to %{source_name}"
+msgstr ""
+
+msgid "AddMember|Member cannot be invited because they already requested to join %{source_name}"
+msgstr ""
+
 msgid "AddMember|No users specified."
 msgstr ""
 
@@ -11050,9 +11062,6 @@ msgstr ""
 msgid "Email address to use for Support Desk"
 msgstr ""
 
-msgid "Email cannot be blank"
-msgstr ""
-
 msgid "Email could not be sent"
 msgstr ""
 
@@ -31261,9 +31270,6 @@ msgstr ""
 msgid "Too many projects enabled. You will need to manage them via the console or the API."
 msgstr ""
 
-msgid "Too many users specified (limit is %{user_limit})"
-msgstr ""
-
 msgid "Too much data"
 msgstr ""
 

spec/requests/api/invitations_spec.rb

@@ -3,14 +3,14 @@
 require 'spec_helper'
 
 RSpec.describe API::Invitations do
-  let(:maintainer) { create(:user, username: 'maintainer_user') }
-  let(:developer) { create(:user) }
-  let(:access_requester) { create(:user) }
-  let(:stranger) { create(:user) }
+  let_it_be(:maintainer) { create(:user, username: 'maintainer_user') }
+  let_it_be(:developer) { create(:user) }
+  let_it_be(:access_requester) { create(:user) }
+  let_it_be(:stranger) { create(:user) }
   let(:email) { 'email1@example.com' }
   let(:email2) { 'email2@example.com' }
 
-  let(:project) do
+  let_it_be(:project) do
     create(:project, :public, creator_id: maintainer.id, namespace: maintainer.namespace) do |project|
       project.add_developer(developer)
       project.add_maintainer(maintainer)
@@ -18,7 +18,7 @@ RSpec.describe API::Invitations do
     end
   end
 
-  let!(:group) do
+  let_it_be(:group, reload: true) do
     create(:group, :public) do |group|
       group.add_developer(developer)
       group.add_owner(maintainer)

spec/services/members/invite_service_spec.rb

@@ -2,76 +2,155 @@
 
 require 'spec_helper'
 
-RSpec.describe Members::InviteService do
-  let(:project) { create(:project) }
-  let(:user) { create(:user) }
-  let(:project_user) { create(:user) }
-
-  before do
-    project.add_maintainer(user)
+RSpec.describe Members::InviteService, :aggregate_failures do
+  let_it_be(:project) { create(:project) }
+  let_it_be(:user) { project.owner }
+  let_it_be(:project_user) { create(:user) }
+  let(:params) { {} }
+  let(:base_params) { { access_level: Gitlab::Access::GUEST } }
+
+  subject(:result) { described_class.new(user, base_params.merge(params)).execute(project) }
+
+  context 'when email is previously unused by current members' do
+    let(:params) { { email: 'email@example.org' } }
+
+    it 'successfully creates a member' do
+      expect { result }.to change(ProjectMember, :count).by(1)
+      expect(result[:status]).to eq(:success)
+    end
   end
 
-  it 'adds an existing user to members' do
-    params = { email: project_user.email.to_s, access_level: Gitlab::Access::GUEST }
-    result = described_class.new(user, params).execute(project)
+  context 'when emails are passed as an array' do
+    let(:params) { { email: %w[email@example.org email2@example.org] } }
 
-    expect(result[:status]).to eq(:success)
-    expect(project.users).to include project_user
+    it 'successfully creates members' do
+      expect { result }.to change(ProjectMember, :count).by(2)
+      expect(result[:status]).to eq(:success)
+    end
   end
 
-  it 'creates a new user for an unknown email address' do
-    params = { email: 'email@example.org', access_level: Gitlab::Access::GUEST }
-    result = described_class.new(user, params).execute(project)
+  context 'when emails are passed as an empty string' do
+    let(:params) { { email: '' } }
 
-    expect(result[:status]).to eq(:success)
+    it 'returns an error' do
+      expect(result[:status]).to eq(:error)
+      expect(result[:message]).to eq('Email cannot be blank')
+    end
   end
 
-  it 'limits the number of emails to 100' do
-    emails = Array.new(101).map { |n| "email#{n}@example.com" }
-    params = { email: emails, access_level: Gitlab::Access::GUEST }
+  context 'when email param is not included' do
+    it 'returns an error' do
+      expect(result[:status]).to eq(:error)
+      expect(result[:message]).to eq('Email cannot be blank')
+    end
+  end
 
-    result = described_class.new(user, params).execute(project)
+  context 'when email is not a valid email' do
+    let(:params) { { email: '_bogus_' } }
 
-    expect(result[:status]).to eq(:error)
-    expect(result[:message]).to eq('Too many users specified (limit is 100)')
+    it 'returns an error' do
+      expect { result }.not_to change(ProjectMember, :count)
+      expect(result[:status]).to eq(:error)
+      expect(result[:message]['_bogus_']).to eq("Invite email is invalid")
+    end
   end
 
-  it 'does not invite an invalid email' do
-    params = { email: project_user.id.to_s, access_level: Gitlab::Access::GUEST }
-    result = described_class.new(user, params).execute(project)
+  context 'when duplicate email addresses are passed' do
+    let(:params) { { email: 'email@example.org,email@example.org' } }
+
+    it 'only creates one member per unique address' do
+      expect { result }.to change(ProjectMember, :count).by(1)
+      expect(result[:status]).to eq(:success)
+    end
+  end
 
-    expect(result[:status]).to eq(:error)
-    expect(result[:message][project_user.id.to_s]).to eq("Invite email is invalid")
-    expect(project.users).not_to include project_user
+  context 'when observing email limits' do
+    let_it_be(:emails) { Array(1..101).map { |n| "email#{n}@example.com" } }
+
+    context 'when over the allowed default limit of emails' do
+      let(:params) { { email: emails } }
+
+      it 'limits the number of emails to 100' do
+        expect { result }.not_to change(ProjectMember, :count)
+        expect(result[:status]).to eq(:error)
+        expect(result[:message]).to eq('Too many users specified (limit is 100)')
+      end
+    end
+
+    context 'when over the allowed custom limit of emails' do
+      let(:params) { { email: 'email@example.org,email2@example.org', limit: 1 } }
+
+      it 'limits the number of emails to the limit supplied' do
+        expect { result }.not_to change(ProjectMember, :count)
+        expect(result[:status]).to eq(:error)
+        expect(result[:message]).to eq('Too many users specified (limit is 1)')
+      end
+    end
+
+    context 'when limit allowed is disabled via limit param' do
+      let(:params) { { email: emails, limit: -1 } }
+
+      it 'does not limit number of emails' do
+        expect { result }.to change(ProjectMember, :count).by(101)
+        expect(result[:status]).to eq(:success)
+      end
+    end
   end
 
-  it 'does not invite to an invalid access level' do
-    params = { email: project_user.email, access_level: -1 }
-    result = described_class.new(user, params).execute(project)
+  context 'when email belongs to an existing user' do
+    let(:params) { { email: project_user.email } }
 
-    expect(result[:status]).to eq(:error)
-    expect(result[:message][project_user.email]).to eq("Access level is not included in the list")
+    it 'adds an existing user to members' do
+      expect { result }.to change(ProjectMember, :count).by(1)
+      expect(result[:status]).to eq(:success)
+      expect(project.users).to include project_user
+    end
   end
 
-  it 'does not add a member with an existing invite' do
-    invited_member = create(:project_member, :invited, project: project)
+  context 'when access level is not valid' do
+    let(:params) { { email: project_user.email, access_level: -1 } }
 
-    params = { email: invited_member.invite_email,
-               access_level: Gitlab::Access::GUEST }
-    result = described_class.new(user, params).execute(project)
+    it 'returns an error' do
+      expect { result }.not_to change(ProjectMember, :count)
+      expect(result[:status]).to eq(:error)
+      expect(result[:message][project_user.email]).to eq("Access level is not included in the list")
+    end
+  end
+
+  context 'when invite already exists for an included email' do
+    let!(:invited_member) { create(:project_member, :invited, project: project) }
+    let(:params) { { email: "#{invited_member.invite_email},#{project_user.email}" } }
 
-    expect(result[:status]).to eq(:error)
-    expect(result[:message][invited_member.invite_email]).to eq("Member already invited to #{project.name}")
+    it 'adds new email and returns an error for the already invited email' do
+      expect { result }.to change(ProjectMember, :count).by(1)
+      expect(result[:status]).to eq(:error)
+      expect(result[:message][invited_member.invite_email]).to eq("Member already invited to #{project.name}")
+      expect(project.users).to include project_user
+    end
   end
 
-  it 'does not add a member with an access_request' do
-    requested_member = create(:project_member, :access_request, project: project)
+  context 'when access request already exists for an included email' do
+    let!(:requested_member) { create(:project_member, :access_request, project: project) }
+    let(:params) { { email: "#{requested_member.user.email},#{project_user.email}" } }
+
+    it 'adds new email and returns an error for the already invited email' do
+      expect { result }.to change(ProjectMember, :count).by(1)
+      expect(result[:status]).to eq(:error)
+      expect(result[:message][requested_member.user.email])
+        .to eq("Member cannot be invited because they already requested to join #{project.name}")
+      expect(project.users).to include project_user
+    end
+  end
 
-    params = { email: requested_member.user.email,
-               access_level: Gitlab::Access::GUEST }
-    result = described_class.new(user, params).execute(project)
+  context 'when email is already a member on the project' do
+    let!(:existing_member) { create(:project_member, :guest, project: project) }
+    let(:params) { { email: "#{existing_member.user.email},#{project_user.email}" } }
 
-    expect(result[:status]).to eq(:error)
-    expect(result[:message][requested_member.user.email]).to eq("Member cannot be invited because they already requested to join #{project.name}")
+    it 'adds new email and returns an error for the already invited email' do
+      expect { result }.to change(ProjectMember, :count).by(1)
+      expect(result[:status]).to eq(:error)
+      expect(result[:message][existing_member.user.email]).to eq("Already a member of #{project.name}")
+      expect(project.users).to include project_user
+    end
   end
 end