Fixes #2429 - Note is not being updated because of the lack of lastname

app/assets/javascripts/app/models/user.coffee

@@ -6,9 +6,9 @@ class App.User extends App.Model
 #  @hasMany 'roles', 'App.Role'
   @configure_attributes = [
     { name: 'login',            display: __('Login'),         tag: 'input',    type: 'text',     limit: 100, null: false, autocapitalize: false, signup: false, quick: false },
-    { name: 'firstname',        display: __('Firstname'),     tag: 'input',    type: 'text',     limit: 100, null: false, signup: true, info: true, invite_agent: true, invite_customer: true },
+    { name: 'firstname',        display: __('Firstname'),     tag: 'input',    type: 'text',     limit: 100, null: true, signup: true, info: true, invite_agent: true, invite_customer: true },
-    { name: 'lastname',         display: __('Lastname'),      tag: 'input',    type: 'text',     limit: 100, null: false, signup: true, info: true, invite_agent: true, invite_customer: true },
+    { name: 'lastname',         display: __('Lastname'),      tag: 'input',    type: 'text',     limit: 100, null: true, signup: true, info: true, invite_agent: true, invite_customer: true },
-    { name: 'email',            display: __('Email'),         tag: 'input',    type: 'email',    limit: 100, null: false, signup: true, info: true, invite_agent: true, invite_customer: true },
+    { name: 'email',            display: __('Email'),         tag: 'input',    type: 'email',    limit: 100, null: true, signup: true, info: true, invite_agent: true, invite_customer: true },
     { name: 'organization_id',  display: __('Organization'),  tag: 'select',   multiple: false, nulloption: true, null: true, relation: 'Organization', signup: false, info: true, invite_customer: true },
     { name: 'created_by_id',    display: __('Created by'),    relation: 'User', readonly: 1 },
     { name: 'created_at',       display: __('Created at'),    tag: 'datetime',  readonly: 1 },

app/controllers/application_controller/handles_errors.rb

@@ -84,8 +84,8 @@ module ApplicationController::HandlesErrors
       error: e.message
     }
 
-    if e.message =~ %r{Validation failed: (.+?)(,|$)}i
+    if (message = e.try(:record)&.errors&.full_messages&.first)
-      data[:error_human] = $1
+      data[:error_human] = message
     elsif e.message.match?(%r{(already exists|duplicate key|duplicate entry)}i)
       data[:error_human] = __('Object already exists!')
     elsif e.message =~ %r{null value in column "(.+?)" violates not-null constraint}i || e.message =~ %r{Field '(.+?)' doesn't have a default value}i

app/controllers/users_controller.rb

@@ -982,9 +982,8 @@ curl http://localhost/api/v1/users/avatar -v -u #{login}:#{password} -H "Content
     user.source        = 'signup'
 
     if email_taken_by # show fake OK response to avoid leaking that email is already in use
-      User.without_callback :validation, :before, :ensure_uniq_email do # skip unique email validation
+      user.skip_ensure_uniq_email = true
-        user.valid? # trigger errors raised in validations
+      user.validate!
-      end
 
       result = User.password_reset_new_token(email_taken_by.email)
       NotificationFactory::Mailer.notification(

app/models/user.rb

@@ -44,13 +44,16 @@ class User < ApplicationModel
   has_many                :data_privacy_tasks,     as: :deletable
   belongs_to              :organization,           inverse_of: :members, optional: true
 
-  before_validation :check_name, :check_email, :check_login, :ensure_uniq_email, :ensure_password, :ensure_roles, :ensure_identifier
+  before_validation :check_name, :check_email, :check_login, :ensure_password, :ensure_roles
   before_validation :check_mail_delivery_failed, on: :update
   before_create     :check_preferences_default, :validate_preferences, :validate_ooo, :domain_based_assignment, :set_locale
   before_update     :check_preferences_default, :validate_preferences, :validate_ooo, :reset_login_failed_after_password_change, :validate_agent_limit_by_attributes, :last_admin_check_by_attribute
   before_destroy    :destroy_longer_required_objects, :destroy_move_dependency_ownership
   after_commit      :update_caller_id
 
+  validate :ensure_identifier, :ensure_email
+  validate :ensure_uniq_email, unless: :skip_ensure_uniq_email
+
   # workflow checks should run after before_create and before_update callbacks
   include ChecksCoreWorkflow
 
@@ -859,51 +862,49 @@ try to find correct name
     preferences.fetch(:locale) { Locale.default }
   end
 
+  attr_accessor :skip_ensure_uniq_email
+
   private
 
   def check_name
-    if firstname.present?
+    firstname&.strip!
-      firstname.strip!
+    lastname&.strip!
-    end
-    if lastname.present?
-      lastname.strip!
-    end
 
-    return true if firstname.present? && lastname.present?
+    return if firstname.present? && lastname.present?
 
     if (firstname.blank? && lastname.present?) || (firstname.present? && lastname.blank?)
       used_name = firstname.presence || lastname
       (local_firstname, local_lastname) = User.name_guess(used_name, email)
-
     elsif firstname.blank? && lastname.blank? && email.present?
       (local_firstname, local_lastname) = User.name_guess('', email)
     end
 
-    self.firstname = local_firstname if local_firstname.present?
+    check_name_apply(:firstname, local_firstname)
-    self.lastname = local_lastname if local_lastname.present?
+    check_name_apply(:lastname, local_lastname)
+  end
 
-    if firstname.present? && firstname.match(%r{^[A-z]+$}) && (firstname.downcase == firstname || firstname.upcase == firstname)
+  def check_name_apply(identifier, input)
-      firstname.capitalize!
+    self[identifier] = input if input.present?
-    end
+
-    if lastname.present? && lastname.match(%r{^[A-z]+$}) && (lastname.downcase == lastname || lastname.upcase == lastname)
+    self[identifier].capitalize! if self[identifier]&.match? %r{^([[:upper:]]+|[[:lower:]]+)$}
-      lastname.capitalize!
-    end
-    true
   end
 
   def check_email
-    return true if Setting.get('import_mode')
+    return if Setting.get('import_mode')
-    return true if email.blank?
+    return if email.blank?
 
     self.email = email.downcase.strip
-    return true if id == 1
+  end
+
+  def ensure_email
+    return if email.blank?
+    return if id == 1
 
     email_address_validation = EmailAddressValidation.new(email)
-    if !email_address_validation.valid_format?
-      raise Exceptions::UnprocessableEntity, "Invalid email '#{email}'"
-    end
 
-    true
+    return if email_address_validation.valid_format?
+
+    errors.add :base, "Invalid email '#{email}'"
   end
 
   def check_login
@@ -914,7 +915,7 @@ try to find correct name
     end
 
     # if email has changed, login is old email, change also login
-    if changes && changes['email'] && changes['email'][0] == login
+    if email_changed? && email_was == login
       self.login = email
     end
 
@@ -943,27 +944,26 @@ try to find correct name
   end
 
   def ensure_roles
-    return true if role_ids.present?
+    return if role_ids.present?
 
     self.role_ids = Role.signup_role_ids
   end
 
   def ensure_identifier
-    return true if email.present? || firstname.present? || lastname.present? || phone.present?
+    return if login.present? && !login.start_with?('auto-')
-    return true if login.present? && !login.start_with?('auto-')
+    return if [email, firstname, lastname, phone].any?(&:present?)
 
-    raise Exceptions::UnprocessableEntity, __('Minimum one identifier (login, firstname, lastname, phone or email) for user is required.')
+    errors.add :base, __('At least one identifier (firstname, lastname, phone or email) for user is required.')
   end
 
   def ensure_uniq_email
-    return true if Setting.get('user_email_multiple_use')
+    return if Setting.get('user_email_multiple_use')
-    return true if Setting.get('import_mode')
+    return if Setting.get('import_mode')
-    return true if email.blank?
+    return if email.blank?
-    return true if !changes
+    return if !email_changed?
-    return true if !changes['email']
+    return if !User.exists?(email: email.downcase.strip)
-    return true if !User.exists?(email: email.downcase.strip)
 
-    raise Exceptions::UnprocessableEntity, "Email address '#{email.downcase.strip}' is already used for other user."
+    errors.add :base, "Email address '#{email.downcase.strip}' is already used for other user."
   end
 
   def validate_roles(role)
@@ -1166,7 +1166,6 @@ raise 'Minimum one user need to have admin permissions'
 
   def ensure_password
     self.password = ensured_password
-    true
   end
 
   def ensured_password

db/migrate/20211025093743_issue_2429_user_identifier_validation.rb

@@ -0,0 +1,31 @@
+# Copyright (C) 2012-2021 Zammad Foundation, http://zammad-foundation.org/
+
+class Issue2429UserIdentifierValidation < ActiveRecord::Migration[6.0]
+  def up
+    return if !Setting.exists?(name: 'system_init_done')
+
+    %i[firstname lastname email phone].each { |elem| update_single(elem) }
+  end
+
+  def update_single(elem)
+    attr = ObjectManager::Attribute.for_object(User).find_by(name: elem)
+
+    attr.screens.each do |_, value|
+      if value.try(:key?, 'null')
+        value['null'] = true
+      end
+
+      next if !value.is_a? Hash
+
+      value.each do |_, inner_value|
+        if inner_value.try(:key?, 'null')
+          inner_value['null'] = true
+        end
+      end
+    end
+
+    attr.save!
+  rescue => e
+    Rails.logger.error e
+  end
+end

db/seeds/object_manager_attributes.rb

@@ -567,7 +567,7 @@ ObjectManager::Attribute.add(
   data_option: {
     type:       'text',
     maxlength:  150,
-    null:       false,
+    null:       true,
     item_class: 'formGroup--halfSize',
   },
   editable:    false,
@@ -575,27 +575,27 @@ ObjectManager::Attribute.add(
   screens:     {
     signup:          {
       '-all-' => {
-        null: false,
+        null: true,
       },
     },
     invite_agent:    {
       '-all-' => {
-        null: false,
+        null: true,
       },
     },
     invite_customer: {
       '-all-' => {
-        null: false,
+        null: true,
       },
     },
     edit:            {
       '-all-' => {
-        null: false,
+        null: true,
       },
     },
     create:          {
       '-all-' => {
-        null: false,
+        null: true,
       },
     },
     view:            {
@@ -619,7 +619,7 @@ ObjectManager::Attribute.add(
   data_option: {
     type:       'text',
     maxlength:  150,
-    null:       false,
+    null:       true,
     item_class: 'formGroup--halfSize',
   },
   editable:    false,
@@ -627,27 +627,27 @@ ObjectManager::Attribute.add(
   screens:     {
     signup:          {
       '-all-' => {
-        null: false,
+        null: true,
       },
     },
     invite_agent:    {
       '-all-' => {
-        null: false,
+        null: true,
       },
     },
     invite_customer: {
       '-all-' => {
-        null: false,
+        null: true,
       },
     },
     edit:            {
       '-all-' => {
-        null: false,
+        null: true,
       },
     },
     create:          {
       '-all-' => {
-        null: false,
+        null: true,
       },
     },
     view:            {
@@ -679,17 +679,17 @@ ObjectManager::Attribute.add(
   screens:     {
     signup:          {
       '-all-' => {
-        null: false,
+        null: true,
       },
     },
     invite_agent:    {
       '-all-' => {
-        null: false,
+        null: true,
       },
     },
     invite_customer: {
       '-all-' => {
-        null: false,
+        null: true,
       },
     },
     edit:            {

i18n/zammad.pot

@@ -925,6 +925,10 @@ msgstr ""
 msgid "Assignment timeout in minutes if assigned agent is not working on it. Ticket will be shown as unassigend."
 msgstr ""
 
+#: app/models/user.rb
+msgid "At least one identifier (firstname, lastname, phone or email) for user is required."
+msgstr ""
+
 #: app/models/object_manager/attribute.rb
 msgid "At least one letters is needed"
 msgstr ""
@@ -5784,10 +5788,6 @@ msgstr ""
 msgid "Minimum of one permission is needed!"
 msgstr ""
 
-#: app/models/user.rb
-msgid "Minimum one identifier (login, firstname, lastname, phone or email) for user is required."
-msgstr ""
-
 #: app/models/role.rb
 #: app/models/user.rb
 msgid "Minimum one user needs to have admin permissions."

spec/db/migrate/issue_2429_user_identifier_validation_spec.rb

@@ -0,0 +1,39 @@
+# Copyright (C) 2012-2021 Zammad Foundation, http://zammad-foundation.org/
+
+require 'rails_helper'
+
+RSpec.describe Issue2429UserIdentifierValidation, type: :db_migration do
+  let(:elem) { ObjectManager::Attribute.for_object(User).find_by(name: 'firstname') }
+
+  it 'resets value directly in screen' do
+    elem.screens = { screen1: { asd: true, null: false } }
+    elem.save!
+
+    migrate
+
+    expect(elem.reload.screens).to eq({ screen1: { asd: true, null: true } }.deep_stringify_keys)
+  end
+
+  it 'resets value nested in permission' do
+    elem.screens = { screen1: { all: { asd: true, null: false } } }
+    elem.save!
+
+    migrate
+
+    expect(elem.reload.screens).to eq({ screen1: { all: { asd: true, null: true } } }.deep_stringify_keys)
+  end
+
+  it 'does not touch when null not present directly in screen' do
+    elem.screens = { screen1: { all: { asd: true } } }
+    elem.save!
+
+    expect { migrate }.not_to change { elem.reload.updated_at }
+  end
+
+  it 'does not touch when null not present in permission' do
+    elem.screens = { screen1: { asd: true } }
+    elem.save!
+
+    expect { migrate }.not_to change { elem.reload.updated_at }
+  end
+end

spec/models/user_spec.rb

@@ -603,6 +603,14 @@ RSpec.describe User, type: :model do
         expect(new_agent.login.sub!(agent.login, '')).to be_a_uuid
       end
     end
+
+    describe '#check_name' do
+      it 'guesses user first/last name with non-ASCII characters' do
+        user = create(:user, firstname: 'perkūnas ąžuolas', lastname: '')
+
+        expect(user).to have_attributes(firstname: 'Perkūnas', lastname: 'Ąžuolas')
+      end
+    end
   end
 
   describe 'Attributes:' do

spec/requests/user_spec.rb

@@ -315,7 +315,7 @@ RSpec.describe 'User', type: :request do
       post '/api/v1/users', params: params, as: :json
       expect(response).to have_http_status(:unprocessable_entity)
       expect(json_response).to be_truthy
-      expect(json_response['error']).to eq('Minimum one identifier (login, firstname, lastname, phone or email) for user is required.')
+      expect(json_response['error']).to eq('At least one identifier (firstname, lastname, phone or email) for user is required.')
 
       # invalid email
       params = { firstname: 'newfirstname123', email: 'some_what', note: 'some note' }
@@ -1225,7 +1225,7 @@ RSpec.describe 'User', type: :request do
 
     it 'requires at least one identifier' do
       make_request({ web: 'example.com' })
-      expect(json_response['error']).to start_with('Minimum one identifier')
+      expect(json_response['error']).to start_with('At least one identifier')
     end
 
     it 'takes first name as identifier' do

spec/system/manage/users_spec.rb

@@ -112,16 +112,20 @@ RSpec.describe 'Manage > Users', type: :system do
   end
 
   context 'updating a user' do
-    before do
+    let(:user) { create(:admin) }
-      create(:admin)
+    let(:row)  { find 'table.user-list tbody tr', text: user.firstname }
-    end
+
+    before do
+      user
 
-    it 'handles permission checkboxes correctly' do
       visit '#manage/users'
 
       within(:active_content) do
-        click 'table.user-list tbody tr:first-child'
+        row.click
       end
+    end
+
+    it 'handles permission checkboxes correctly' do
       in_modal disappears: false do
         scroll_into_view 'table.settings-list'
         within 'table.settings-list tbody tr:first-child' do
@@ -136,6 +140,38 @@ RSpec.describe 'Manage > Users', type: :system do
         end
       end
     end
+
+    it 'allows to update a user with no email/first/last/phone if login is present' do
+      in_modal do
+        fill_in 'Firstname', with: ''
+        fill_in 'Lastname', with: ''
+        fill_in 'Email', with: ''
+        fill_in 'Phone', with: ''
+
+        click_on 'Submit'
+      end
+
+      within :active_content do
+        expect(page).to have_no_text(user.firstname)
+      end
+    end
+
+    context 'when user has auto login' do
+      let(:user) { create(:admin, login: "auto-#{SecureRandom.uuid}") }
+
+      it 'does not allow to update a user with no email/first/last/phone' do
+        in_modal disappears: false do
+          fill_in 'Firstname', with: ''
+          fill_in 'Lastname', with: ''
+          fill_in 'Email', with: ''
+          fill_in 'Phone', with: ''
+
+          click_on 'Submit'
+
+          expect(page).to have_text('At least one identifier')
+        end
+      end
+    end
   end
 
   describe 'check user edit permissions', authenticated_as: -> { user } do

test/unit/user_test.rb

@@ -496,7 +496,7 @@ class UserTest < ActiveSupport::TestCase
     assert(admin1.id)
     assert_equal(admin1.email, email1)
 
-    assert_raises(Exceptions::UnprocessableEntity) do
+    assert_raises(ActiveRecord::RecordInvalid) do
       User.create!(
         login:         "#{email1}-1",
         firstname:     'Role',
@@ -522,7 +522,7 @@ class UserTest < ActiveSupport::TestCase
       created_by_id: 1,
     )
 
-    assert_raises(Exceptions::UnprocessableEntity) do
+    assert_raises(ActiveRecord::RecordInvalid) do
       admin2.email = email1
       admin2.save!
     end