Implemented issue zammad#1251 - Config option to have uniq email addr…

…esses for users.

app/controllers/users_controller.rb

@@ -132,6 +132,10 @@ def create
         raise Exceptions::UnprocessableEntity, 'Attribute \'email\' required!'
       end
 
+      # check if user already exists
+      exists = User.find_by(email: clean_params[:email].downcase.strip)
+      raise Exceptions::UnprocessableEntity, 'Email address is already used for other user.' if exists
+
       user = User.new(clean_params)
       user.associations_from_param(params)
       user.updated_by_id = 1
@@ -171,11 +175,6 @@ def create
       user.associations_from_param(params)
     end
 
-    # check if user already exists
-    if user.email.present?
-      exists = User.where(email: user.email.downcase).first
-      raise Exceptions::UnprocessableEntity, 'User already exists!' if exists
-    end
     user.save!
 
     # if first user was added, set system init done

app/models/user.rb

@@ -38,7 +38,7 @@ class User < ApplicationModel
   load 'user/search_index.rb'
   include User::SearchIndex
 
-  before_validation :check_name, :check_email, :check_login, :ensure_password, :ensure_roles, :ensure_identifier
+  before_validation :check_name, :check_email, :check_login, :ensure_uniq_email, :ensure_password, :ensure_roles, :ensure_identifier
   before_create   :check_preferences_default, :validate_roles, :domain_based_assignment, :set_locale
   before_update   :check_preferences_default, :validate_roles, :reset_login_failed
   after_create    :avatar_for_email_check
@@ -845,7 +845,7 @@ def check_name
 
   def check_email
     return true if Setting.get('import_mode')
-    return true if email.empty?
+    return true if email.blank?
     self.email = email.downcase.strip
     return true if id == 1
     raise Exceptions::UnprocessableEntity, 'Invalid email' if email !~ /@/
@@ -897,6 +897,16 @@ def ensure_identifier
     raise Exceptions::UnprocessableEntity, 'Minimum one identifier (login, firstname, lastname, phone or email) for user is required.'
   end
 
+  def ensure_uniq_email
+    return true if Setting.get('user_email_multiple_use')
+    return true if Setting.get('import_mode')
+    return true if email.blank?
+    return true if !changes
+    return true if !changes['email']
+    return true if !User.find_by(email: email.downcase.strip)
+    raise Exceptions::UnprocessableEntity, 'Email address is already used for other user.'
+  end
+
   def validate_roles
     return true if !role_ids
     role_ids.each { |role_id|

db/migrate/20170714000002_user_email_multiple_use.rb

@@ -0,0 +1,34 @@
+class UserEmailMultipleUse < ActiveRecord::Migration
+  def up
+
+    # return if it's a new setup
+    return if !Setting.find_by(name: 'system_init_done')
+
+    Setting.create_if_not_exists(
+      title: 'User email for muliple users',
+      name: 'user_email_multiple_use',
+      area: 'Model::User',
+      description: 'Allow to use email address for muliple users.',
+      options: {
+        form: [
+          {
+            display: '',
+            null: true,
+            name: 'user_email_multiple_use',
+            tag: 'boolean',
+            options: {
+              true  => 'yes',
+              false => 'no',
+            },
+          },
+        ],
+      },
+      state: false,
+      preferences: {
+        permission: ['admin'],
+      },
+      frontend: false
+    )
+  end
+
+end

db/seeds/settings.rb

@@ -705,6 +705,31 @@
   },
   frontend: true
 )
+Setting.create_if_not_exists(
+  title: 'User email for muliple users',
+  name: 'user_email_multiple_use',
+  area: 'Model::User',
+  description: 'Allow to use email address for muliple users.',
+  options: {
+    form: [
+      {
+        display: '',
+        null: true,
+        name: 'user_email_multiple_use',
+        tag: 'boolean',
+        options: {
+          true  => 'yes',
+          false => 'no',
+        },
+      },
+    ],
+  },
+  state: false,
+  preferences: {
+    permission: ['admin'],
+  },
+  frontend: false
+)
 Setting.create_if_not_exists(
   title: 'Authentication via %s',
   name: 'auth_ldap',

test/controllers/user_organization_controller_test.rb

@@ -126,7 +126,15 @@ class UserOrganizationControllerTest < ActionDispatch::IntegrationTest
     assert_response(422)
     result = JSON.parse(@response.body)
     assert(result['error'])
-    assert_equal('User already exists!', result['error'])
+    assert_equal('Email address is already used for other user.', result['error'])
+
+    # email missing with enabled feature
+    params = { firstname: 'some firstname', signup: true }
+    post '/api/v1/users', params.to_json, headers
+    assert_response(422)
+    result = JSON.parse(@response.body)
+    assert(result['error'])
+    assert_equal('Attribute \'email\' required!', result['error'])
 
     # email missing with enabled feature
     params = { firstname: 'some firstname', signup: true }
@@ -330,7 +338,7 @@ class UserOrganizationControllerTest < ActionDispatch::IntegrationTest
     assert_response(422)
     result = JSON.parse(@response.body)
     assert(result)
-    assert_equal('User already exists!', result['error'])
+    assert_equal('Email address is already used for other user.', result['error'])
 
     # missing required attributes
     params = { note: 'some note' }

test/integration/geo_location_test.rb

@@ -51,7 +51,7 @@ class GeoLocationTest < ActiveSupport::TestCase
       login: 'some_geo_login2',
       firstname: 'First',
       lastname: 'Last',
-      email: 'some_geo_login1@example.com',
+      email: 'some_geo_login2@example.com',
       password: 'test',
       street: 'Marienstrasse 13',
       city: 'Berlin',

test/unit/activity_stream_test.rb

@@ -10,7 +10,7 @@ class ActivityStreamTest < ActiveSupport::TestCase
       login: 'admin',
       firstname: 'Bob',
       lastname: 'Smith',
-      email: '[email protected]',
+      email: 'bob+active_stream@example.com',
       password: 'some_pass',
       active: true,
       roles: roles,
@@ -23,7 +23,7 @@ class ActivityStreamTest < ActiveSupport::TestCase
   end
 
   test 'ticket+user' do
-    ticket = Ticket.create(
+    ticket = Ticket.create!(
       group_id: Group.lookup(name: 'Users').id,
       customer_id: @current_user.id,
       owner_id: User.lookup(login: '-').id,
@@ -35,7 +35,7 @@ class ActivityStreamTest < ActiveSupport::TestCase
     )
     travel 2.seconds
 
-    article = Ticket::Article.create(
+    article = Ticket::Article.create!(
       ticket_id: ticket.id,
       updated_by_id: @current_user.id,
       created_by_id: @current_user.id,
@@ -86,12 +86,12 @@ class ActivityStreamTest < ActiveSupport::TestCase
     assert(stream.empty?)
 
     # cleanup
-    ticket.destroy
+    ticket.destroy!
     travel_back
   end
 
   test 'organization' do
-    organization = Organization.create(
+    organization = Organization.create!(
       name: 'some name',
       updated_by_id: @current_user.id,
       created_by_id: @current_user.id,
@@ -125,12 +125,12 @@ class ActivityStreamTest < ActiveSupport::TestCase
     assert(stream.empty?)
 
     # cleanup
-    organization.destroy
+    organization.destroy!
     travel_back
   end
 
   test 'user with update check false' do
-    user = User.create(
+    user = User.create!(
       login: '[email protected]',
       email: '[email protected]',
       firstname: 'Bob Smith II',
@@ -157,12 +157,12 @@ class ActivityStreamTest < ActiveSupport::TestCase
     assert(stream.empty?)
 
     # cleanup
-    user.destroy
+    user.destroy!
     travel_back
   end
 
   test 'user with update check true' do
-    user = User.create(
+    user = User.create!(
       login: '[email protected]',
       email: '[email protected]',
       firstname: 'Bob Smith II',
@@ -204,7 +204,7 @@ class ActivityStreamTest < ActiveSupport::TestCase
     assert(stream.empty?)
 
     # cleanup
-    user.destroy
+    user.destroy!
     travel_back
   end
 

test/unit/history_test.rb

@@ -155,12 +155,12 @@ class HistoryTest < ActiveSupport::TestCase
 
       # use transaction
       ActiveRecord::Base.transaction do
-        ticket = Ticket.create(test[:ticket_create][:ticket])
+        ticket = Ticket.create!(test[:ticket_create][:ticket])
         test[:ticket_create][:article][:ticket_id] = ticket.id
-        article = Ticket::Article.create(test[:ticket_create][:article])
+        article = Ticket::Article.create!(test[:ticket_create][:article])
 
-        assert_equal(ticket.class.to_s, 'Ticket')
-        assert_equal(article.class.to_s, 'Ticket::Article')
+        assert_equal(ticket.class, Ticket)
+        assert_equal(article.class, Ticket::Article)
 
         # update ticket
         if test[:ticket_update][:ticket]
@@ -185,25 +185,21 @@ class HistoryTest < ActiveSupport::TestCase
     }
 
     # delete tickets
-    tickets.each { |ticket|
-      ticket_id = ticket.id
-      ticket.destroy
-      found = Ticket.where(id: ticket_id).first
-      assert_not(found, 'Ticket destroyed')
-    }
+    tickets.each(&:destroy!)
   end
 
   test 'user' do
+    name = rand(999_999)
     tests = [
 
       # test 1
       {
         user_create: {
           user: {
-            login: 'some_login_test',
+            login: "some_login_test-#{name}",
             firstname: 'Bob',
             lastname: 'Smith',
-            email: '[email protected]',
+            email: "somebody-#{name}@example.com",
             active: true,
             updated_by_id: current_user.id,
             created_by_id: current_user.id,
@@ -213,7 +209,7 @@ class HistoryTest < ActiveSupport::TestCase
           user: {
             firstname: 'Bob',
             lastname: 'Master',
-            email: '[email protected]',
+            email: "master-#{name}@example.com",
             active: false,
           },
         },
@@ -236,8 +232,8 @@ class HistoryTest < ActiveSupport::TestCase
             history_object: 'User',
             history_type: 'updated',
             history_attribute: 'email',
-            value_from: '[email protected]',
-            value_to: '[email protected]',
+            value_from: "somebody-#{name}@example.com",
+            value_to: "master-#{name}@example.com",
           },
           {
             result: true,
@@ -258,9 +254,8 @@ class HistoryTest < ActiveSupport::TestCase
 
       # user transaction
       ActiveRecord::Base.transaction do
-        user = User.create(test[:user_create][:user])
-
-        assert_equal(user.class.to_s, 'User')
+        user = User.create!(test[:user_create][:user])
+        assert_equal(user.class, User)
 
         # update user
         if test[:user_update][:user]
@@ -277,12 +272,7 @@ class HistoryTest < ActiveSupport::TestCase
     }
 
     # delete user
-    users.each { |user|
-      user_id = user.id
-      user.destroy
-      found = User.where(id: user_id).first
-      assert_not(found, 'User destroyed')
-    }
+    users.each(&:destroy!)
   end
 
   test 'organization' do
@@ -328,9 +318,8 @@ class HistoryTest < ActiveSupport::TestCase
 
       # user transaction
       ActiveRecord::Base.transaction do
-        organization = Organization.create(test[:organization_create][:organization])
-
-        assert_equal(organization.class.to_s, 'Organization')
+        organization = Organization.create!(test[:organization_create][:organization])
+        assert_equal(organization.class, Organization)
 
         # update organization
         if test[:organization_update][:organization]
@@ -346,12 +335,7 @@ class HistoryTest < ActiveSupport::TestCase
     }
 
     # delete user
-    organizations.each { |organization|
-      organization_id = organization.id
-      organization.destroy
-      found = Organization.where(id: organization_id).first
-      assert_not(found, 'Organization destroyed')
-    }
+    organizations.each(&:destroy!)
   end
 
   def history_check(history_list, history_check)