Follow up fa4577d463 - Fixes #3710 - Unable to close tickets in certran cases if core workflow is used.

app/models/concerns/checks_core_workflow.rb

@@ -4,7 +4,8 @@ module ChecksCoreWorkflow
   extend ActiveSupport::Concern
 
   included do
-    before_validation :validate_workflows
+    before_create :validate_workflows
+    before_update :validate_workflows
 
     attr_accessor :screen
   end
@@ -32,14 +33,16 @@ module ChecksCoreWorkflow
     changes.each_key do |key|
       next if perform_result[:restrict_values][key].blank?
       next if self[key].blank?
-
-      value_found = perform_result[:restrict_values][key].any? { |value| value.to_s == self[key].to_s }
-      next if value_found
+      next if restricted_value?(perform_result, key)
 
       raise Exceptions::UnprocessableEntity, "Invalid value '#{self[key]}' for field '#{key}'!"
     end
   end
 
+  def restricted_value?(perform_result, key)
+    perform_result[:restrict_values][key].any? { |value| value.to_s == self[key].to_s }
+  end
+
   def check_visibility(perform_result)
     perform_result[:visibility].each_key do |key|
       next if perform_result[:visibility][key] != 'remove'
@@ -50,11 +53,28 @@ module ChecksCoreWorkflow
 
   def check_mandatory(perform_result)
     perform_result[:mandatory].each_key do |key|
-      next if %w[hide remove].include?(perform_result[:visibility][key])
-      next if !perform_result[:mandatory][key]
-      next if self[key].present?
+      next if field_visible?(perform_result, key)
+      next if !field_mandatory?(perform_result, key)
+      next if !column_value?(key)
+      next if !colum_default?(key)
 
       raise Exceptions::UnprocessableEntity, "Missing required value for field '#{key}'!"
     end
   end
+
+  def field_visible?(perform_result, key)
+    %w[hide remove].include?(perform_result[:visibility][key])
+  end
+
+  def field_mandatory?(perform_result, key)
+    perform_result[:mandatory][key]
+  end
+
+  def column_value?(key)
+    self[key].nil?
+  end
+
+  def colum_default?(key)
+    self.class.column_defaults[key].nil?
+  end
 end

app/models/core_workflow/attributes.rb

@@ -78,16 +78,11 @@ class CoreWorkflow::Attributes
   # dont cache this else the result object will work with references and cache bugs occur
   def visibility_default
     object_elements.each_with_object({}) do |attribute, result|
-      result[ attribute[:name] ] = if @payload['request_id'] == 'ChecksCoreWorkflow.validate_workflows'
-                                     'show'
-                                   else
-                                     screen_value(attribute, 'shown') == false ? 'hide' : 'show'
-                                   end
+      result[ attribute[:name] ] = screen_value(attribute, 'shown') == false ? 'hide' : 'show'
     end
   end
 
   def attribute_mandatory?(attribute)
-    return false if @payload['request_id'] == 'ChecksCoreWorkflow.validate_workflows'
     return screen_value(attribute, 'required').present? if !screen_value(attribute, 'required').nil?
     return screen_value(attribute, 'null').blank? if !screen_value(attribute, 'null').nil?
 

app/models/group.rb

@@ -4,7 +4,6 @@ class Group < ApplicationModel
   include CanBeImported
   include HasActivityStreamLog
   include ChecksClientNotification
-  include ChecksCoreWorkflow
   include ChecksHtmlSanitized
   include ChecksLatestChangeObserved
   include HasHistory
@@ -16,6 +15,9 @@ class Group < ApplicationModel
   belongs_to :email_address, optional: true
   belongs_to :signature, optional: true
 
+  # workflow checks should run after before_create and before_update callbacks
+  include ChecksCoreWorkflow
+
   validates :name, presence: true
 
   sanitized_html :note

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

@@ -3,6 +3,7 @@
 class ObjectManager::Attribute::Validation::Required < ObjectManager::Attribute::Validation::Backend
 
   def validate
+    return if record.class.include?(ChecksCoreWorkflow)
     return if !value.nil?
     return if optional_for_user?
 

app/models/organization.rb

@@ -3,7 +3,6 @@
 class Organization < ApplicationModel
   include HasActivityStreamLog
   include ChecksClientNotification
-  include ChecksCoreWorkflow
   include ChecksLatestChangeObserved
   include HasHistory
   include HasSearchIndexBackend
@@ -26,6 +25,9 @@ class Organization < ApplicationModel
   before_create :domain_cleanup
   before_update :domain_cleanup
 
+  # workflow checks should run after before_create and before_update callbacks
+  include ChecksCoreWorkflow
+
   validates :name,   presence: true
   validates :domain, presence: { message: 'required when Domain Based Assignment is enabled' }, if: :domain_assignment
 

app/models/sla.rb

@@ -3,15 +3,18 @@
 class Sla < ApplicationModel
   include ChecksClientNotification
   include ChecksConditionValidation
-  include ChecksCoreWorkflow
   include HasEscalationCalculationImpact
-
   include Sla::Assets
 
+  belongs_to :calendar, optional: true
+
+  # workflow checks should run after before_create and before_update callbacks
+  include ChecksCoreWorkflow
+
+  validates  :name, presence: true
+
   store      :condition
   store      :data
-  validates  :name, presence: true
-  belongs_to :calendar, optional: true
 
   def condition_matches?(ticket)
     query_condition, bind_condition, tables = Ticket.selector2sql(condition)

app/models/ticket.rb

@@ -4,7 +4,6 @@ class Ticket < ApplicationModel
   include CanBeImported
   include HasActivityStreamLog
   include ChecksClientNotification
-  include ChecksCoreWorkflow
   include ChecksLatestChangeObserved
   include CanCsvImport
   include ChecksHtmlSanitized
@@ -39,6 +38,9 @@ class Ticket < ApplicationModel
 
   include HasTransactionDispatcher
 
+  # workflow checks should run after before_create and before_update callbacks
+  include ChecksCoreWorkflow
+
   validates :group_id, presence: true
 
   activity_stream_permission 'ticket.agent'

app/models/user.rb

@@ -5,7 +5,6 @@ class User < ApplicationModel
   include CanBeImported
   include HasActivityStreamLog
   include ChecksClientNotification
-  include ChecksCoreWorkflow
   include HasHistory
   include HasSearchIndexBackend
   include CanCsvImport
@@ -54,6 +53,9 @@ class User < ApplicationModel
   before_destroy    :destroy_longer_required_objects, :destroy_move_dependency_ownership
   after_commit      :update_caller_id
 
+  # workflow checks should run after before_create and before_update callbacks
+  include ChecksCoreWorkflow
+
   store :preferences
 
   association_attributes_ignored :online_notifications, :templates, :taskbars, :user_devices, :chat_sessions, :karma_activity_logs, :cti_caller_ids, :text_modules, :customer_tickets, :owner_tickets, :created_recent_views, :chat_agents, :data_privacy_tasks, :overviews, :mentions

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

@@ -12,7 +12,7 @@ RSpec.describe ::ObjectManager::Attribute::Validation::Required do
     )
   end
 
-  let(:record) { build(:user) }
+  let(:record) { build(:ticket_article) }
   let(:attribute) { build(:object_manager_attribute_date) }
 
   it_behaves_like 'validate backend'
@@ -52,7 +52,7 @@ RSpec.describe ::ObjectManager::Attribute::Validation::Required do
         context 'when action is edit' do
 
           let(:action) { 'edit' }
-          let(:record) { create(:user) }
+          let(:record) { create(:ticket_article) }
 
           it_behaves_like 'a validation with errors'
         end
@@ -116,7 +116,7 @@ RSpec.describe ::ObjectManager::Attribute::Validation::Required do
           context 'when action is edit' do
 
             let(:action) { 'edit' }
-            let(:record) { create(:user) }
+            let(:record) { create(:ticket_article) }
 
             it_behaves_like 'a validation without errors'
           end
@@ -147,7 +147,7 @@ RSpec.describe ::ObjectManager::Attribute::Validation::Required do
         context 'when action is edit' do
 
           let(:action) { 'edit' }
-          let(:record) { create(:user) }
+          let(:record) { create(:ticket_article) }
 
           it_behaves_like 'a validation without errors'
         end

spec/system/ticket/zoom_spec.rb

@@ -2045,4 +2045,42 @@ RSpec.describe 'Ticket zoom', type: :system do
       expect_no_upload_and_text
     end
   end
+
+  describe 'Unable to close tickets in certran cases if core workflow is used #3710', authenticated_as: :authenticate, db_strategy: :reset do
+    let!(:ticket) { create(:ticket, group: Group.find_by(name: 'Users')) }
+    let(:field_name) { SecureRandom.uuid }
+    let(:field) do
+      create :object_manager_attribute_text, name: field_name, display: field_name, screens: {
+        'edit' => {
+          'ticket.agent' => {
+            'shown'    => false,
+            'required' => false,
+          }
+        }
+      }
+      ObjectManager::Attribute.migration_execute
+    end
+    let(:workflow) do
+      create(:core_workflow,
+             object:  'Ticket',
+             perform: { "ticket.#{field_name}" => { 'operator' => 'set_mandatory', 'set_mandatory' => 'true' } })
+    end
+
+    def authenticate
+      field
+      workflow
+      true
+    end
+
+    before do
+      visit "#ticket/zoom/#{ticket.id}"
+    end
+
+    it 'does save the ticket because the field is mandatory but hidden' do
+      admin = User.find_by(email: 'admin@example.com')
+      select admin.fullname, from: 'Owner'
+      find('.js-submit').click
+      expect(ticket.reload.owner_id).to eq(admin.id)
+    end
+  end
 end