Fixed issues #1259 and #1598 by adding ObjectManager::Attribute validations.

app/models/concerns/has_object_manager_attributes_validation.rb

@@ -0,0 +1,9 @@
+# Copyright (C) 2018 Zammad Foundation, http://zammad-foundation.org/
+module HasObjectManagerAttributesValidation
+  extend ActiveSupport::Concern
+
+  included do
+    include ActiveModel::Validations
+    validates_with ObjectManager::Attribute::Validation, on: %i[create update]
+  end
+end

app/models/group.rb

@@ -6,6 +6,7 @@ class Group < ApplicationModel
   include ChecksClientNotification
   include ChecksLatestChangeObserved
   include HasHistory
+  include HasObjectManagerAttributesValidation
 
   belongs_to :email_address
   belongs_to :signature

app/models/object_manager/attribute/validation.rb

@@ -0,0 +1,66 @@
+class ObjectManager::Attribute::Validation < ActiveModel::Validator
+  include ::Mixin::HasBackends
+
+  def validate(record)
+    return if validation_unneeded?
+
+    @record = record
+    sanitize_memory_cache
+
+    return if attributes_unchanged?
+
+    model_attributes.select(&:editable).each do |attribute|
+      perform_validation(attribute)
+    end
+  end
+
+  private
+
+  attr_reader :record
+
+  def validation_unneeded?
+    return true if Setting.get('import_mode')
+
+    ApplicationHandleInfo.current != 'application_server'
+  end
+
+  def attributes_unchanged?
+    model_attributes.none? do |attribute|
+      record.will_save_change_to_attribute?(attribute.name)
+    end
+  end
+
+  def model_attributes
+    @model_attributes ||= begin
+      object_lookup_id = ObjectLookup.by_name(record.class.name)
+      @active_attributes.select { |attribute| attribute.object_lookup_id == object_lookup_id }
+    end
+  end
+
+  def perform_validation(attribute)
+    backends.each do |backend|
+      backend.validate(
+        record:    record,
+        attribute: attribute
+      )
+    end
+  end
+
+  def validations
+    @validations ||= backend.map { backend.new }
+  end
+
+  def sanitize_memory_cache
+    @model_attributes = nil
+
+    latest_cache_key = active_attributes_in_db.cache_key
+    return if @previous_cache_key == latest_cache_key
+
+    @previous_cache_key = latest_cache_key
+    @active_attributes  = active_attributes_in_db.to_a
+  end
+
+  def active_attributes_in_db
+    ObjectManager::Attribute.where(active: true)
+  end
+end

app/models/object_manager/attribute/validation/backend.rb

@@ -0,0 +1,26 @@
+class ObjectManager::Attribute::Validation::Backend
+  include Mixin::IsBackend
+
+  def self.inherited(subclass)
+    subclass.is_backend_of(::ObjectManager::Attribute::Validation)
+  end
+
+  def self.validate(*args)
+    new(*args).validate
+  end
+
+  attr_reader :record, :attribute, :value, :previous_value
+
+  def initialize(record:, attribute:)
+    @record         = record
+    @attribute      = attribute
+    @value          = record[attribute.name]
+    @previous_value = record.attribute_in_database(attribute.name)
+  end
+
+  def invalid_because_attribute(message)
+    record.errors.add attribute.name.to_sym, message
+  end
+end
+
+Mixin::RequiredSubPaths.eager_load_recursive(__dir__)

app/models/object_manager/attribute/validation/date.rb

@@ -0,0 +1,30 @@
+class ObjectManager::Attribute::Validation::Date < ObjectManager::Attribute::Validation::Backend
+
+  def validate
+    return if value.blank?
+    return if irrelevant_attribute?
+
+    validate_past
+    validate_future
+  end
+
+  private
+
+  def irrelevant_attribute?
+    %w[date datetime].exclude?(attribute.data_type)
+  end
+
+  def validate_past
+    return if attribute.data_option[:past]
+    return if !value.past?
+
+    invalid_because_attribute('does not allow past dates.')
+  end
+
+  def validate_future
+    return if attribute.data_option[:future]
+    return if !value.future?
+
+    invalid_because_attribute('does not allow future dates.')
+  end
+end

app/models/object_manager/attribute/validation/required.rb

@@ -0,0 +1,45 @@
+class ObjectManager::Attribute::Validation::Required < ObjectManager::Attribute::Validation::Backend
+
+  def validate
+    return if value.present?
+    return if optional_for_user?
+
+    invalid_because_attribute('is required but missing.')
+  end
+
+  private
+
+  def optional_for_user?
+    return true if system_user?
+    return true if required_for_permissions.blank?
+    return false if required_for_permissions.include?('-all-')
+
+    !user.permissions?(required_for_permissions)
+  end
+
+  def system_user?
+    user_id.blank? || user_id == 1
+  end
+
+  def user_id
+    user_id ||= UserInfo.current_user_id
+  end
+
+  def user
+    @user ||= User.lookup(id: user_id)
+  end
+
+  def required_for_permissions
+    @required_for_permissions ||= begin
+      attribute.screens[action]&.each_with_object([]) do |(permission, config), result|
+        result.push(permission) if config[:required].present?
+      end
+    end
+  end
+
+  def action
+    return :edit if record.persisted?
+
+    attribute.screens.keys.find { |e| e.start_with?('create') }
+  end
+end

app/models/organization.rb

@@ -8,6 +8,7 @@ class Organization < ApplicationModel
   include HasSearchIndexBackend
   include CanCsvImport
   include ChecksHtmlSanitized
+  include HasObjectManagerAttributesValidation
 
   include Organization::ChecksAccess
   include Organization::Assets

app/models/ticket.rb

@@ -14,6 +14,7 @@ class Ticket < ApplicationModel
   include HasKarmaActivityLog
   include HasLinks
   include Ticket::ChecksAccess
+  include HasObjectManagerAttributesValidation
 
   include Ticket::Escalation
   include Ticket::Subject

app/models/ticket/article.rb

@@ -6,6 +6,7 @@ class Ticket::Article < ApplicationModel
   include HasHistory
   include ChecksHtmlSanitized
   include CanCsvImport
+  include HasObjectManagerAttributesValidation
 
   include Ticket::Article::ChecksAccess
   include Ticket::Article::Assets

app/models/user.rb

@@ -9,6 +9,7 @@ class User < ApplicationModel
   include ChecksHtmlSanitized
   include HasGroups
   include HasRoles
+  include HasObjectManagerAttributesValidation
 
   include User::ChecksAccess
   include User::Assets

db/migrate/20181102142526_object_manager_attribute_indexes.rb

@@ -0,0 +1,7 @@
+class ObjectManagerAttributeIndexes < ActiveRecord::Migration[5.1]
+  def change
+
+    add_index :object_manager_attributes, :active
+    add_index :object_manager_attributes, :updated_at
+  end
+end

lib/mixin/has_backends.rb

@@ -0,0 +1,13 @@
+module Mixin
+  module HasBackends
+    extend ActiveSupport::Concern
+
+    included do
+      cattr_accessor :backends do
+        Set.new
+      end
+
+      require_dependency "#{name}::Backend".underscore
+    end
+  end
+end

lib/mixin/is_backend.rb

@@ -0,0 +1,12 @@
+module Mixin
+  module IsBackend
+    extend ActiveSupport::Concern
+
+    class_methods do
+
+      def is_backend_of(klass) # rubocop:disable Naming/PredicateName
+        klass.backends.add(self)
+      end
+    end
+  end
+end

spec/lib/mixin/has_backends_examples.rb

@@ -0,0 +1,12 @@
+RSpec.shared_examples 'Mixin::HasBackends' do
+
+  describe '.backends' do
+    it 'is a Set' do
+      expect(described_class.backends).to be_a(Set)
+    end
+  end
+
+  it "auto requires #{described_class}::Backend" do
+    expect(described_class).to be_const_defined(:Backend)
+  end
+end

spec/models/concerns/has_object_manager_attributes_validation_examples.rb

@@ -0,0 +1,5 @@
+RSpec.shared_examples 'HasObjectManagerAttributesValidation' do
+  it 'validates ObjectManager::Attributes' do
+    expect(described_class.validators.map(&:class)).to include(ObjectManager::Attribute::Validation)
+  end
+end

spec/models/group_spec.rb

@@ -1,8 +1,10 @@
 require 'rails_helper'
 require 'models/application_model_examples'
 require 'models/concerns/can_be_imported_examples'
+require 'models/concerns/has_object_manager_attributes_validation_examples'
 
 RSpec.describe Group, type: :model do
   it_behaves_like 'ApplicationModel'
   it_behaves_like 'CanBeImported'
+  it_behaves_like 'HasObjectManagerAttributesValidation'
 end

spec/models/object_manager/attribute/validation/backend_examples.rb

@@ -0,0 +1,15 @@
+require 'rails_helper'
+
+RSpec.shared_examples 'a validation without errors' do
+  it 'validatates without errors' do
+    subject.validate
+    expect(record.errors).to be_blank
+  end
+end
+
+RSpec.shared_examples 'a validation with errors' do
+  it 'validates with errors' do
+    subject.validate
+    expect(record.errors).to be_present
+  end
+end

spec/models/object_manager/attribute/validation/backend_spec.rb

@@ -0,0 +1,58 @@
+require 'rails_helper'
+
+RSpec.describe ObjectManager::Attribute::Validation::Backend do
+
+  it 'registers inheriting classes as ObjectManager::Attribute::Validation backends' do
+    backends = spy
+    expect(ObjectManager::Attribute::Validation).to receive(:backends).and_return(backends)
+    backend = Class.new(described_class)
+    expect(backends).to have_received(:add).with(backend)
+  end
+
+  describe 'backend interface' do
+
+    let(:record) { build(:user) }
+    let(:attribute) { ::ObjectManager::Attribute.find_by(name: 'firstname') }
+
+    subject do
+      described_class.new(
+        record:    record,
+        attribute: attribute
+      )
+    end
+
+    it 'has attr_accessor for record' do
+      expect(subject.record).to eq(record)
+    end
+
+    it 'has attr_accessor for attribute' do
+      expect(subject.attribute).to eq(attribute)
+    end
+
+    it 'has attr_accessor for value' do
+      expect(subject.value).to eq(record[attribute.name])
+    end
+
+    it 'has attr_accessor for previous_value' do
+      record.save!
+      previous_value         = record[attribute.name]
+      record[attribute.name] = 'changed'
+      expect(subject.previous_value).to eq(previous_value)
+    end
+
+    describe '.invalid_because_attribute' do
+
+      before do
+        subject.invalid_because_attribute('has value that is ... .')
+      end
+
+      it 'adds Rails validation error' do
+        expect(record.errors.count).to be(1)
+      end
+
+      it 'uses ObjectManager::Attribute#name as ActiveModel::Errors identifier' do
+        expect(record.errors.to_h).to have_key(attribute.name.to_sym)
+      end
+    end
+  end
+end

spec/models/object_manager/attribute/validation/date_spec.rb

@@ -0,0 +1,61 @@
+require 'rails_helper'
+require 'models/object_manager/attribute/validation/backend_examples'
+
+RSpec.describe ::ObjectManager::Attribute::Validation::Date do
+
+  let(:record) { build(:user) }
+  let(:attribute) { build(:object_manager_attribute_date) }
+  subject do
+    described_class.new(
+      record:    record,
+      attribute: attribute
+    )
+  end
+
+  before do
+    allow(subject).to receive(:value).and_return(value)
+  end
+
+  shared_examples 'data_option validator' do |data_option:, value:|
+    context "for data_option '#{data_option}'" do
+
+      let(:value) { value }
+
+      context 'when data_option is set to true' do
+
+        before { attribute.data_option[data_option] = true }
+
+        it_behaves_like 'a validation without errors'
+      end
+
+      context 'when data_option is set to false' do
+
+        before { attribute.data_option[data_option] = false }
+
+        it_behaves_like 'a validation with errors'
+      end
+    end
+  end
+
+  it_behaves_like 'data_option validator', data_option: :future, value: Time.current.tomorrow.midnight
+  it_behaves_like 'data_option validator', data_option: :past, value: Time.current.yesterday.midnight
+
+  context 'when validation should not be performed' do
+
+    context 'for blank value' do
+
+      let(:value) { nil }
+
+      it_behaves_like 'a validation without errors'
+    end
+
+    context 'for irrelevant attribute data_type' do
+
+      let(:value) { 'some_value' }
+
+      before { attribute.data_type = 'select' }
+
+      it_behaves_like 'a validation without errors'
+    end
+  end
+end

spec/models/object_manager/attribute/validation/required_spec.rb

@@ -0,0 +1,155 @@
+require 'rails_helper'
+require 'models/object_manager/attribute/validation/backend_examples'
+
+RSpec.describe ::ObjectManager::Attribute::Validation::Required do
+
+  let(:record) { build(:user) }
+  let(:attribute) { build(:object_manager_attribute_date) }
+  subject do
+    described_class.new(
+      record:    record,
+      attribute: attribute
+    )
+  end
+
+  before do
+    allow(subject).to receive(:value).and_return(value)
+  end
+
+  context 'when validation should be performed' do
+
+    let(:value) { nil }
+
+    shared_examples 'a permission based validator' do |permission:|
+
+      let(:performing_user) { create(:agent_user) }
+
+      before { UserInfo.current_user_id = performing_user.id }
+
+      context "for applying permission (#{permission})" do
+
+        let(:permission) { permission }
+
+        before do
+          attribute.screens = {
+            action => {
+              permission => {
+                required: true
+              }
+            }
+          }
+        end
+
+        context 'when action is edit' do
+
+          let(:action) { 'edit' }
+          let(:record) { create(:user) }
+
+          it_behaves_like 'a validation with errors'
+        end
+
+        context 'when action is create_...' do
+
+          let(:action) { 'create_middle' }
+
+          it_behaves_like 'a validation with errors'
+        end
+      end
+    end
+
+    it_behaves_like 'a permission based validator', permission: 'ticket.agent'
+    it_behaves_like 'a permission based validator', permission: '-all-'
+  end
+
+  context 'when validation should not be performed' do
+
+    context 'for present value' do
+
+      let(:value) { 'some_value' }
+
+      it_behaves_like 'a validation without errors'
+    end
+
+    context 'when value is actually blank' do
+
+      let(:value) { nil }
+
+      context "when action wasn't performed by a user" do
+        context 'for blank UserInfo.current_user_id', current_user_id: nil do
+          it_behaves_like 'a validation without errors'
+        end
+
+        context 'for system UserInfo.current_user_id', current_user_id: 1 do
+          it_behaves_like 'a validation without errors'
+        end
+      end
+
+      context 'for required => false' do
+
+        let(:performing_user) { create(:agent_user) }
+
+        before { UserInfo.current_user_id = performing_user.id }
+
+        context 'for applying permission' do
+
+          let(:permission) { 'ticket.agent' }
+
+          before do
+            attribute.screens = {
+              action => {
+                permission => {
+                  required: false
+                }
+              }
+            }
+          end
+
+          context 'when action is edit' do
+
+            let(:action) { 'edit' }
+            let(:record) { create(:user) }
+
+            it_behaves_like 'a validation without errors'
+          end
+
+          context 'when action is create_...' do
+
+            let(:action) { 'create_middle' }
+
+            it_behaves_like 'a validation without errors'
+          end
+        end
+      end
+
+      context 'for not applying permission' do
+
+        let(:permission) { 'ticket.customer' }
+
+        before do
+          attribute.screens = {
+            action => {
+              permission => {
+                required: true
+              }
+            }
+          }
+        end
+
+        context 'when action is edit' do
+
+          let(:action) { 'edit' }
+          let(:record) { create(:user) }
+
+          it_behaves_like 'a validation without errors'
+        end
+
+        context 'when action is create_...' do
+
+          let(:action) { 'create_middle' }
+
+          it_behaves_like 'a validation without errors'
+        end
+      end
+    end
+  end
+end

spec/models/object_manager/attribute/validation_spec.rb

@@ -0,0 +1,98 @@
+require 'rails_helper'
+require 'lib/mixin/has_backends_examples'
+
+RSpec.describe ObjectManager::Attribute::Validation, application_handle: 'application_server' do
+  it_behaves_like 'Mixin::HasBackends'
+
+  it 'is a ActiveModel::Validator' do
+    expect(described_class).to be < ActiveModel::Validator
+  end
+
+  describe '#validate' do
+
+    subject { described_class.new }
+    let(:record) { build(:user) }
+    let(:backend) { spy }
+
+    around do |example|
+      original_backends = described_class.backends.dup
+      begin
+        example.run
+      ensure
+        described_class.backends = original_backends
+      end
+    end
+
+    before do
+      described_class.backends = Set[backend]
+    end
+
+    it 'sends .validate to backends' do
+      subject.validate(record)
+      expect(backend).to have_received(:validate).with(record: record, attribute: instance_of(::ObjectManager::Attribute)).at_least(:once)
+    end
+
+    context 'for cached ObjectManager::Attribute records' do
+
+      it 'fetches current records when in memory Cache is blank' do
+        allow(::ObjectManager::Attribute).to receive(:where).and_call_original
+        subject.validate(record)
+        expect(::ObjectManager::Attribute).to have_received(:where).twice
+      end
+
+      it "doesn't fetch current records when in memory Cache is valid" do
+        subject.validate(record)
+
+        allow(::ObjectManager::Attribute).to receive(:where).and_call_original
+        subject.validate(record)
+        expect(::ObjectManager::Attribute).to have_received(:where).once
+      end
+
+      it 'fetches current records when in memory Cache is outdated' do
+        subject.validate(record)
+
+        ::ObjectManager::Attribute.last.touch
+
+        allow(::ObjectManager::Attribute).to receive(:where).and_call_original
+        subject.validate(record)
+        expect(::ObjectManager::Attribute).to have_received(:where).twice
+      end
+    end
+
+    context 'when no validation is performed' do
+
+      it 'is skipped because of irrelevant ApplicationHandleInfo', application_handle: 'non_application_server' do
+        subject.validate(record)
+        expect(backend).to_not have_received(:validate)
+      end
+
+      it 'is skipped because of import_mode is active' do
+        expect(Setting).to receive(:get).with('import_mode').and_return(true)
+        subject.validate(record)
+        expect(backend).to_not have_received(:validate)
+      end
+
+      it 'is skipped because of unchanged attributes' do
+        record.save!
+        RSpec::Mocks.space.proxy_for(backend).reset
+        subject.validate(record)
+        expect(backend).not_to have_received(:validate)
+      end
+
+      context 'when caused by ObjectManager::Attribute records' do
+
+        it 'is skipped because no custom attributes are present' do
+          ::ObjectManager::Attribute.update(editable: false)
+          subject.validate(record)
+          expect(backend).not_to have_received(:validate)
+        end
+
+        it 'is skipped because no active attributes are present' do
+          ::ObjectManager::Attribute.update(active: false)
+          subject.validate(record)
+          expect(backend).not_to have_received(:validate)
+        end
+      end
+    end
+  end
+end

spec/models/organization_spec.rb

@@ -3,12 +3,14 @@ require 'models/application_model_examples'
 require 'models/concerns/can_lookup_examples'
 require 'models/concerns/has_search_index_backend_examples'
 require 'models/concerns/has_xss_sanitized_note_examples'
+require 'models/concerns/has_object_manager_attributes_validation_examples'
 
 RSpec.describe Organization, type: :model do
   it_behaves_like 'ApplicationModel', can_assets: { associations: :members }
   it_behaves_like 'CanLookup'
   it_behaves_like 'HasSearchIndexBackend', indexed_factory: :organization
   it_behaves_like 'HasXssSanitizedNote', model_factory: :organization
+  it_behaves_like 'HasObjectManagerAttributesValidation'
 
   describe '.where_or_cis' do
     it 'finds instance by querying multiple attributes case insensitive' do

spec/models/ticket/article_spec.rb

@@ -1,10 +1,12 @@
 require 'rails_helper'
 require 'models/application_model_examples'
 require 'models/concerns/can_be_imported_examples'
+require 'models/concerns/has_object_manager_attributes_validation_examples'
 
 RSpec.describe Ticket::Article, type: :model do
   it_behaves_like 'ApplicationModel'
   it_behaves_like 'CanBeImported'
+  it_behaves_like 'HasObjectManagerAttributesValidation'
 
   describe 'Callbacks, Observers, & Async Transactions' do
     describe 'NULL byte handling (via ChecksAttributeValuesAndLength concern):' do

spec/models/ticket_spec.rb

@@ -3,12 +3,14 @@ require 'models/application_model_examples'
 require 'models/concerns/can_be_imported_examples'
 require 'models/concerns/can_lookup_examples'
 require 'models/concerns/has_xss_sanitized_note_examples'
+require 'models/concerns/has_object_manager_attributes_validation_examples'
 
 RSpec.describe Ticket, type: :model do
   it_behaves_like 'ApplicationModel'
   it_behaves_like 'CanBeImported'
   it_behaves_like 'CanLookup'
   it_behaves_like 'HasXssSanitizedNote', model_factory: :ticket
+  it_behaves_like 'HasObjectManagerAttributesValidation'
 
   subject(:ticket) { create(:ticket) }
 

spec/models/user_spec.rb

@@ -6,6 +6,7 @@ require 'models/concerns/has_groups_permissions_examples'
 require 'models/concerns/has_xss_sanitized_note_examples'
 require 'models/concerns/can_be_imported_examples'
 require 'models/concerns/can_lookup_examples'
+require 'models/concerns/has_object_manager_attributes_validation_examples'
 
 RSpec.describe User, type: :model do
   it_behaves_like 'ApplicationModel', can_assets: { associations: :organization }
@@ -15,6 +16,7 @@ RSpec.describe User, type: :model do
   it_behaves_like 'HasGroups and Permissions', group_access_no_permission_factory: :user
   it_behaves_like 'CanBeImported'
   it_behaves_like 'CanLookup'
+  it_behaves_like 'HasObjectManagerAttributesValidation'
 
   subject(:user) { create(:user) }
   let(:admin)    { create(:admin_user) }

spec/support/application_handle_info.rb

@@ -0,0 +1,11 @@
+RSpec.configure do |config|
+
+  config.around(:each, :application_handle) do |example|
+    ApplicationHandleInfo.current = example.metadata[:application_handle]
+    begin
+      example.run
+    ensure
+      ApplicationHandleInfo.current = nil
+    end
+  end
+end

spec/support/user_info.rb

@@ -25,4 +25,13 @@ end
 
 RSpec.configure do |config|
   config.include ZammadSpecSupportUserInfo
+
+  config.around(:each, :current_user_id) do |example|
+    UserInfo.current_user_id = example.metadata[:current_user_id]
+    begin
+      example.run
+    ensure
+      UserInfo.current_user_id = nil
+    end
+  end
 end