diff --git a/app/models/concerns/checks_core_workflow.rb b/app/models/concerns/checks_core_workflow.rb index 141a769d8..5827302ee 100644 --- a/app/models/concerns/checks_core_workflow.rb +++ b/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 diff --git a/app/models/core_workflow/attributes.rb b/app/models/core_workflow/attributes.rb index b00efded8..6463d7ab9 100644 --- a/app/models/core_workflow/attributes.rb +++ b/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? diff --git a/app/models/group.rb b/app/models/group.rb index 71a1b923a..352722566 100644 --- a/app/models/group.rb +++ b/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 diff --git a/app/models/object_manager/attribute/validation/required.rb b/app/models/object_manager/attribute/validation/required.rb index 1bc6847ae..47440372d 100644 --- a/app/models/object_manager/attribute/validation/required.rb +++ b/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? diff --git a/app/models/organization.rb b/app/models/organization.rb index a9f587224..26d4813b7 100644 --- a/app/models/organization.rb +++ b/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 diff --git a/app/models/sla.rb b/app/models/sla.rb index ef68287cf..5accf2d84 100644 --- a/app/models/sla.rb +++ b/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) diff --git a/app/models/ticket.rb b/app/models/ticket.rb index e69a292c7..fa224a3ec 100644 --- a/app/models/ticket.rb +++ b/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' diff --git a/app/models/user.rb b/app/models/user.rb index 1816f11d6..a93432e4e 100644 --- a/app/models/user.rb +++ b/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 diff --git a/spec/models/object_manager/attribute/validation/required_spec.rb b/spec/models/object_manager/attribute/validation/required_spec.rb index 3d8e29416..3805922d0 100644 --- a/spec/models/object_manager/attribute/validation/required_spec.rb +++ b/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 diff --git a/spec/system/ticket/zoom_spec.rb b/spec/system/ticket/zoom_spec.rb index 84603a6e4..588b9ef3e 100644 --- a/spec/system/ticket/zoom_spec.rb +++ b/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