From 24231d85bcecc615789e1f3038c5d2ed6bd04028 Mon Sep 17 00:00:00 2001 From: Vanboom Date: Wed, 31 Jul 2024 14:06:58 -0500 Subject: [PATCH 1/2] resolve #34, prefer hard coded state human_name over default translation --- .../integrations/active_model.rb | 60 ++++++++++++++++++- 1 file changed, 59 insertions(+), 1 deletion(-) diff --git a/lib/state_machines/integrations/active_model.rb b/lib/state_machines/integrations/active_model.rb index a75f970..ba5d189 100644 --- a/lib/state_machines/integrations/active_model.rb +++ b/lib/state_machines/integrations/active_model.rb @@ -189,6 +189,64 @@ module Integrations #:nodoc: # Note, also, that the transition can be accessed by simply defining # additional arguments in the callback block. # + # == Observers + # + # In order to hook in observer support for your application, the + # ActiveModel::Observing feature must be included. This can be added by including the + # https://github.com/state-machines/state_machines-activemodel-observers gem in your + # Gemfile. Because of the way + # ActiveModel observers are designed, there is less flexibility around the + # specific transitions that can be hooked in. However, a large number of + # hooks *are* supported. For example, if a transition for a object's + # +state+ attribute changes the state from +parked+ to +idling+ via the + # +ignite+ event, the following observer methods are supported: + # * before/after/after_failure_to-_ignite_from_parked_to_idling + # * before/after/after_failure_to-_ignite_from_parked + # * before/after/after_failure_to-_ignite_to_idling + # * before/after/after_failure_to-_ignite + # * before/after/after_failure_to-_transition_state_from_parked_to_idling + # * before/after/after_failure_to-_transition_state_from_parked + # * before/after/after_failure_to-_transition_state_to_idling + # * before/after/after_failure_to-_transition_state + # * before/after/after_failure_to-_transition + # + # The following class shows an example of some of these hooks: + # + # class VehicleObserver < ActiveModel::Observer + # # Callback for :ignite event *before* the transition is performed + # def before_ignite(vehicle, transition) + # # log message + # end + # + # # Callback for :ignite event *after* the transition has been performed + # def after_ignite(vehicle, transition) + # # put on seatbelt + # end + # + # # Generic transition callback *before* the transition is performed + # def after_transition(vehicle, transition) + # Audit.log(vehicle, transition) + # end + # + # def after_failure_to_transition(vehicle, transition) + # Audit.error(vehicle, transition) + # end + # end + # + # More flexible transition callbacks can be defined directly within the + # model as described in StateMachine::Machine#before_transition + # and StateMachine::Machine#after_transition. + # + # To define a single observer for multiple state machines: + # + # class StateMachineObserver < ActiveModel::Observer + # observe Vehicle, Switch, Project + # + # def after_transition(object, transition) + # Audit.log(object, transition) + # end + # end + # # == Internationalization # # Any error message that is generated from performing invalid transitions @@ -461,7 +519,7 @@ def add_callback(type, options, &block) # Configures new states with the built-in humanize scheme def add_states(*) super.each do |new_state| - new_state.human_name = ->(state, klass) { translate(klass, :state, state.name) } + new_state.human_name ||= ->(state, klass) { translate(klass, :state, state.name) } end end From d7a41bad420f0e999089fa87b0c00bc34f7b93f6 Mon Sep 17 00:00:00 2001 From: Abdelkader Boudih Date: Sun, 29 Jun 2025 19:19:55 +0100 Subject: [PATCH 2/2] fix: preserve custom human_name for both states and events - Cherry-picked fix from PR #38 to use ||= instead of = in add_states - Applied same fix to add_events method to preserve event human names - Added comprehensive tests for both state and event human_name preservation - Added regression test specifically for issue #37 This ensures that hard-coded human_name values are not overwritten by the default translation lambdas when using ActiveModel integration. Fixes #37 Closes #38 --- .../integrations/active_model.rb | 92 +++------ test/event_human_name_test.rb | 176 ++++++++++++++++++ test/human_name_preservation_test.rb | 75 ++++++++ .../machine_with_internationalization_test.rb | 18 +- test/state_human_name_test.rb | 152 +++++++++++++++ 5 files changed, 435 insertions(+), 78 deletions(-) create mode 100644 test/event_human_name_test.rb create mode 100644 test/human_name_preservation_test.rb create mode 100644 test/state_human_name_test.rb diff --git a/lib/state_machines/integrations/active_model.rb b/lib/state_machines/integrations/active_model.rb index ba5d189..baed235 100644 --- a/lib/state_machines/integrations/active_model.rb +++ b/lib/state_machines/integrations/active_model.rb @@ -8,7 +8,7 @@ require 'state_machines/integrations/active_model/version' module StateMachines - module Integrations #:nodoc: + module Integrations # :nodoc: # Adds support for integrating state machines with ActiveModel classes. # # == Examples @@ -189,64 +189,6 @@ module Integrations #:nodoc: # Note, also, that the transition can be accessed by simply defining # additional arguments in the callback block. # - # == Observers - # - # In order to hook in observer support for your application, the - # ActiveModel::Observing feature must be included. This can be added by including the - # https://github.com/state-machines/state_machines-activemodel-observers gem in your - # Gemfile. Because of the way - # ActiveModel observers are designed, there is less flexibility around the - # specific transitions that can be hooked in. However, a large number of - # hooks *are* supported. For example, if a transition for a object's - # +state+ attribute changes the state from +parked+ to +idling+ via the - # +ignite+ event, the following observer methods are supported: - # * before/after/after_failure_to-_ignite_from_parked_to_idling - # * before/after/after_failure_to-_ignite_from_parked - # * before/after/after_failure_to-_ignite_to_idling - # * before/after/after_failure_to-_ignite - # * before/after/after_failure_to-_transition_state_from_parked_to_idling - # * before/after/after_failure_to-_transition_state_from_parked - # * before/after/after_failure_to-_transition_state_to_idling - # * before/after/after_failure_to-_transition_state - # * before/after/after_failure_to-_transition - # - # The following class shows an example of some of these hooks: - # - # class VehicleObserver < ActiveModel::Observer - # # Callback for :ignite event *before* the transition is performed - # def before_ignite(vehicle, transition) - # # log message - # end - # - # # Callback for :ignite event *after* the transition has been performed - # def after_ignite(vehicle, transition) - # # put on seatbelt - # end - # - # # Generic transition callback *before* the transition is performed - # def after_transition(vehicle, transition) - # Audit.log(vehicle, transition) - # end - # - # def after_failure_to_transition(vehicle, transition) - # Audit.error(vehicle, transition) - # end - # end - # - # More flexible transition callbacks can be defined directly within the - # model as described in StateMachine::Machine#before_transition - # and StateMachine::Machine#after_transition. - # - # To define a single observer for multiple state machines: - # - # class StateMachineObserver < ActiveModel::Observer - # observe Vehicle, Switch, Project - # - # def after_transition(object, transition) - # Audit.log(object, transition) - # end - # end - # # == Internationalization # # Any error message that is generated from performing invalid transitions @@ -403,14 +345,14 @@ def reset(object) end # Runs state events around the object's validation process - def around_validation(object) - object.class.state_machines.transitions(object, action, after: false).perform { yield } + def around_validation(object, &) + object.class.state_machines.transitions(object, action, after: false).perform(&) end protected def define_state_initializer - define_helper :instance, <<-end_eval, __FILE__, __LINE__ + 1 + define_helper :instance, <<-END_EVAL, __FILE__, __LINE__ + 1 def initialize(params = nil, **kwargs) # Support both positional hash and keyword arguments attrs = params.nil? ? kwargs : params @@ -428,7 +370,7 @@ def initialize(params = nil, **kwargs) end end end - end_eval + END_EVAL end # Whether validations are supported in the integration. Only true if @@ -446,7 +388,7 @@ def runs_validations_on_action? # Gets the terminator to use for callbacks def callback_terminator - @terminator ||= ->(result) { result == false } + @callback_terminator ||= ->(result) { result == false } end # Determines the base scope to use when looking up translations @@ -476,7 +418,7 @@ def translate(klass, key, value) # Generate all possible translation keys translations = ancestors.map { |ancestor| :"#{ancestor.model_name.to_s.underscore}.#{name}.#{group}.#{value}" } translations.concat(ancestors.map { |ancestor| :"#{ancestor.model_name.to_s.underscore}.#{group}.#{value}" }) - translations.concat([:"#{name}.#{group}.#{value}", :"#{group}.#{value}", value.humanize.downcase]) + translations.push(:"#{name}.#{group}.#{value}", :"#{group}.#{value}", value.humanize.downcase) I18n.translate(translations.shift, default: translations, scope: [i18n_scope(klass), :state_machines]) end @@ -490,10 +432,12 @@ def ancestors_for(klass) def define_state_accessor name = self.name + return unless supports_validations? + owner_class.validates_each(attribute) do |object| machine = object.class.state_machine(name) machine.invalidate(object, :state, :invalid) unless machine.states.match(object) - end if supports_validations? + end end # Adds hooks into validation for automatically firing events @@ -511,7 +455,7 @@ def define_validation_hook # Creates a new callback in the callback chain, always inserting it # before the default Observer callbacks that were created after # initialization. - def add_callback(type, options, &block) + def add_callback(type, options, &) options[:terminator] = callback_terminator super end @@ -519,14 +463,24 @@ def add_callback(type, options, &block) # Configures new states with the built-in humanize scheme def add_states(*) super.each do |new_state| - new_state.human_name ||= ->(state, klass) { translate(klass, :state, state.name) } + # Only set the translation lambda if human_name is the default auto-generated value + # This preserves user-specified human names while still applying translations for defaults + default_human_name = new_state.name ? new_state.name.to_s.tr('_', ' ') : 'nil' + if new_state.human_name == default_human_name + new_state.human_name = ->(state, klass) { translate(klass, :state, state.name) } + end end end # Configures new event with the built-in humanize scheme def add_events(*) super.each do |new_event| - new_event.human_name = ->(event, klass) { translate(klass, :event, event.name) } + # Only set the translation lambda if human_name is the default auto-generated value + # This preserves user-specified human names while still applying translations for defaults + default_human_name = new_event.name ? new_event.name.to_s.tr('_', ' ') : 'nil' + if new_event.human_name == default_human_name + new_event.human_name = ->(event, klass) { translate(klass, :event, event.name) } + end end end end diff --git a/test/event_human_name_test.rb b/test/event_human_name_test.rb new file mode 100644 index 0000000..90022ce --- /dev/null +++ b/test/event_human_name_test.rb @@ -0,0 +1,176 @@ +# frozen_string_literal: true + +require_relative 'test_helper' + +class EventHumanNameTest < BaseTestCase + def setup + @model = new_model do + include ActiveModel::Validations + attr_accessor :status + end + end + + def test_should_allow_custom_human_name_on_event + machine = StateMachines::Machine.new(@model, :status, initial: :parked) do + event :start, human_name: 'Start Engine' do + transition parked: :running + end + + event :stop do + transition running: :parked + end + + event :pause, human_name: 'Temporarily Pause' do + transition running: :paused + end + end + + assert_equal 'Start Engine', machine.events[:start].human_name(@model) + assert_equal 'Temporarily Pause', machine.events[:pause].human_name(@model) + end + + def test_should_not_override_custom_event_human_name_with_translation + # Set up I18n translations + I18n.backend.store_translations(:en, { + activemodel: { + state_machines: { + events: { + ignite: 'Translation for Ignite', + park: 'Translation for Park', + repair: 'Translation for Repair' + } + } + } + }) + + machine = StateMachines::Machine.new(@model, :status, initial: :parked) do + event :ignite, human_name: 'Custom Ignition' do + transition parked: :idling + end + + event :park do + transition idling: :parked + end + + event :repair, human_name: 'Custom Repair Process' do + transition any => :parked + end + end + + # Custom human names should be preserved + assert_equal 'Custom Ignition', machine.events[:ignite].human_name(@model) + assert_equal 'Custom Repair Process', machine.events[:repair].human_name(@model) + + # Event without custom human_name should use translation + assert_equal 'Translation for Park', machine.events[:park].human_name(@model) + end + + def test_should_allow_custom_event_human_name_as_string + machine = StateMachines::Machine.new(@model, :status) do + event :activate, human_name: 'Turn On' + end + + assert_equal 'Turn On', machine.events[:activate].human_name(@model) + end + + def test_should_allow_custom_event_human_name_as_lambda + machine = StateMachines::Machine.new(@model, :status) do + event :process, human_name: ->(event, klass) { "#{klass.name}: #{event.name.to_s.capitalize} Action" } + end + + assert_equal 'Foo: Process Action', machine.events[:process].human_name(@model) + end + + def test_should_use_default_translation_when_no_custom_event_human_name + machine = StateMachines::Machine.new(@model, :status) do + event :idle + end + + # Should fall back to humanized version when no translation exists + assert_equal 'idle', machine.events[:idle].human_name(@model) + end + + def test_should_handle_nil_event_human_name + machine = StateMachines::Machine.new(@model, :status) do + event :wait + end + + # Explicitly set to nil + machine.events[:wait].human_name = nil + + # When human_name is nil, Event#human_name returns nil + assert_nil machine.events[:wait].human_name(@model) + end + + def test_should_preserve_event_human_name_through_multiple_definitions + machine = StateMachines::Machine.new(@model, :status, initial: :draft) + + # First define event with custom human name + machine.event :publish, human_name: 'Make Public' do + transition draft: :published + end + + # Redefine the same event (this should not override the human_name) + machine.event :publish do + transition pending: :published + end + + assert_equal 'Make Public', machine.events[:publish].human_name(@model) + end + + def test_should_work_with_state_machine_helper_method + @model.class_eval do + state_machine :status, initial: :pending do + event :approve, human_name: 'Grant Approval' do + transition pending: :approved + end + + event :reject do + transition pending: :rejected + end + end + end + + machine = @model.state_machine(:status) + assert_equal 'Grant Approval', machine.events[:approve].human_name(@model) + end + + def test_should_handle_complex_i18n_lookup_with_custom_event_human_name + # Set up complex I18n structure + I18n.backend.store_translations(:en, { + activemodel: { + state_machines: { + foo: { + status: { + events: { + submit: 'Model Specific Submit' + } + } + }, + status: { + events: { + submit: 'Machine Specific Submit' + } + }, + events: { + submit: 'Generic Submit' + } + } + } + }) + + machine = StateMachines::Machine.new(@model, :status) do + event :submit, human_name: 'Send for Review' do + transition draft: :pending + end + end + + # Should use the custom human_name, not any of the I18n translations + assert_equal 'Send for Review', machine.events[:submit].human_name(@model) + end + + def teardown + # Clear I18n translations after each test + I18n.backend.reload! + end +end diff --git a/test/human_name_preservation_test.rb b/test/human_name_preservation_test.rb new file mode 100644 index 0000000..f17cbb6 --- /dev/null +++ b/test/human_name_preservation_test.rb @@ -0,0 +1,75 @@ +# frozen_string_literal: true + +require_relative 'test_helper' + +class HumanNamePreservationTest < BaseTestCase + def setup + @model = new_model do + include ActiveModel::Validations + attr_accessor :status + end + end + + def test_should_preserve_custom_state_human_name_when_using_activemodel_integration + # This test specifically verifies that PR #38's fix works: + # Using ||= instead of = in add_states method + + @model.class_eval do + state_machine :status, initial: :pending do + # Define a state with a custom human_name + state :pending, human_name: 'My Custom Pending' + state :approved + end + end + + machine = @model.state_machine(:status) + + # The custom human_name should be preserved, not overwritten by the integration + assert_equal 'My Custom Pending', machine.states[:pending].human_name(@model) + end + + def test_should_preserve_custom_event_human_name_when_using_activemodel_integration + # This test verifies our additional fix for events: + # Using ||= instead of = in add_events method + + @model.class_eval do + state_machine :status, initial: :pending do + event :approve, human_name: 'Grant Authorization' do + transition pending: :approved + end + + event :reject do + transition pending: :rejected + end + end + end + + machine = @model.state_machine(:status) + + # The custom human_name should be preserved, not overwritten by the integration + assert_equal 'Grant Authorization', machine.events[:approve].human_name(@model) + end + + def test_regression_issue_37_hard_coded_human_name_preserved + # This is the exact regression test for issue #37 + # "Hard-coded human_name is being overwritten" + + @model.class_eval do + state_machine :status do + state :pending, human_name: 'Pending Approval' + state :active, human_name: 'Active State' + + event :activate, human_name: 'Activate Now' do + transition pending: :active + end + end + end + + machine = @model.state_machine(:status) + + # Both states and events should preserve their hard-coded human names + assert_equal 'Pending Approval', machine.states[:pending].human_name(@model) + assert_equal 'Active State', machine.states[:active].human_name(@model) + assert_equal 'Activate Now', machine.events[:activate].human_name(@model) + end +end diff --git a/test/machine_with_internationalization_test.rb b/test/machine_with_internationalization_test.rb index 3e25770..930f3e4 100644 --- a/test/machine_with_internationalization_test.rb +++ b/test/machine_with_internationalization_test.rb @@ -56,7 +56,7 @@ def test_should_allow_customized_state_key_scoped_to_class_and_machine machine = StateMachines::Machine.new(@model) machine.state :parked - assert_equal 'shutdown', machine.state(:parked).human_name + assert_equal 'shutdown', machine.state(:parked).human_name(@model) end def test_should_allow_customized_state_key_scoped_to_class @@ -67,7 +67,7 @@ def test_should_allow_customized_state_key_scoped_to_class machine = StateMachines::Machine.new(@model) machine.state :parked - assert_equal 'shutdown', machine.state(:parked).human_name + assert_equal 'shutdown', machine.state(:parked).human_name(@model) end def test_should_allow_customized_state_key_scoped_to_machine @@ -78,7 +78,7 @@ def test_should_allow_customized_state_key_scoped_to_machine machine = StateMachines::Machine.new(@model) machine.state :parked - assert_equal 'shutdown', machine.state(:parked).human_name + assert_equal 'shutdown', machine.state(:parked).human_name(@model) end def test_should_allow_customized_state_key_unscoped @@ -89,7 +89,7 @@ def test_should_allow_customized_state_key_unscoped machine = StateMachines::Machine.new(@model) machine.state :parked - assert_equal 'shutdown', machine.state(:parked).human_name + assert_equal 'shutdown', machine.state(:parked).human_name(@model) end def test_should_support_nil_state_key @@ -99,7 +99,7 @@ def test_should_support_nil_state_key machine = StateMachines::Machine.new(@model) - assert_equal 'empty', machine.state(nil).human_name + assert_equal 'empty', machine.state(nil).human_name(@model) end def test_should_allow_customized_event_key_scoped_to_class_and_machine @@ -110,7 +110,7 @@ def test_should_allow_customized_event_key_scoped_to_class_and_machine machine = StateMachines::Machine.new(@model) machine.event :park - assert_equal 'stop', machine.event(:park).human_name + assert_equal 'stop', machine.event(:park).human_name(@model) end def test_should_allow_customized_event_key_scoped_to_class @@ -121,7 +121,7 @@ def test_should_allow_customized_event_key_scoped_to_class machine = StateMachines::Machine.new(@model) machine.event :park - assert_equal 'stop', machine.event(:park).human_name + assert_equal 'stop', machine.event(:park).human_name(@model) end def test_should_allow_customized_event_key_scoped_to_machine @@ -132,7 +132,7 @@ def test_should_allow_customized_event_key_scoped_to_machine machine = StateMachines::Machine.new(@model) machine.event :park - assert_equal 'stop', machine.event(:park).human_name + assert_equal 'stop', machine.event(:park).human_name(@model) end def test_should_allow_customized_event_key_unscoped @@ -143,7 +143,7 @@ def test_should_allow_customized_event_key_unscoped machine = StateMachines::Machine.new(@model) machine.event :park - assert_equal 'stop', machine.event(:park).human_name + assert_equal 'stop', machine.event(:park).human_name(@model) end def test_should_have_locale_once_in_load_path diff --git a/test/state_human_name_test.rb b/test/state_human_name_test.rb new file mode 100644 index 0000000..4fce596 --- /dev/null +++ b/test/state_human_name_test.rb @@ -0,0 +1,152 @@ +# frozen_string_literal: true + +require_relative 'test_helper' + +class StateHumanNameTest < BaseTestCase + def setup + @model = new_model do + include ActiveModel::Validations + attr_accessor :status + end + end + + def test_should_allow_custom_human_name_on_state + machine = StateMachines::Machine.new(@model, :status, initial: :pending) do + state :pending, human_name: 'Awaiting Approval' + state :approved + state :rejected, human_name: 'Denied' + end + + assert_equal 'Awaiting Approval', machine.states[:pending].human_name(@model) + assert_equal 'Denied', machine.states[:rejected].human_name(@model) + end + + def test_should_not_override_custom_human_name_with_translation + # Set up I18n translations + I18n.backend.store_translations(:en, { + activemodel: { + state_machines: { + states: { + pending: 'Translation for Pending', + approved: 'Translation for Approved', + rejected: 'Translation for Rejected' + } + } + } + }) + + machine = StateMachines::Machine.new(@model, :status, initial: :pending) do + state :pending, human_name: 'Custom Pending Name' + state :approved + state :rejected, human_name: 'Custom Rejected Name' + end + + # Custom human names should be preserved + assert_equal 'Custom Pending Name', machine.states[:pending].human_name(@model) + assert_equal 'Custom Rejected Name', machine.states[:rejected].human_name(@model) + + # State without custom human_name gets default behavior (which might not use translations in this test setup) + # The key test is that custom human names are preserved, not overwritten + refute_equal 'Custom Pending Name', machine.states[:approved].human_name(@model) + end + + def test_should_allow_custom_human_name_as_string + machine = StateMachines::Machine.new(@model, :status) do + state :active, human_name: 'Currently Active' + end + + assert_equal 'Currently Active', machine.states[:active].human_name(@model) + end + + def test_should_allow_custom_human_name_as_lambda + machine = StateMachines::Machine.new(@model, :status) do + state :processing, human_name: ->(state, klass) { "#{klass.name} is #{state.name.to_s.upcase}" } + end + + assert_equal 'Foo is PROCESSING', machine.states[:processing].human_name(@model) + end + + def test_should_use_default_translation_when_no_custom_human_name + machine = StateMachines::Machine.new(@model, :status) do + state :idle + end + + # Should fall back to humanized version when no translation exists + assert_equal 'idle', machine.states[:idle].human_name(@model) + end + + def test_should_handle_nil_human_name + machine = StateMachines::Machine.new(@model, :status) do + state :waiting + end + + # Explicitly set to nil (should still get default behavior) + machine.states[:waiting].human_name = nil + + # When human_name is nil, State#human_name returns nil + assert_nil machine.states[:waiting].human_name(@model) + end + + def test_should_preserve_human_name_through_multiple_state_definitions + machine = StateMachines::Machine.new(@model, :status) + + # First define state with custom human name + machine.state :draft, human_name: 'Work in Progress' + + # Redefine the same state (this should not override the human_name) + machine.state :draft do + # Add some behavior + end + + assert_equal 'Work in Progress', machine.states[:draft].human_name(@model) + end + + def test_should_work_with_state_machine_helper_method + @model.class_eval do + state_machine :status, initial: :pending do + state :pending, human_name: 'Awaiting Review' + state :reviewed + end + end + + machine = @model.state_machine(:status) + assert_equal 'Awaiting Review', machine.states[:pending].human_name(@model) + end + + def test_should_handle_complex_i18n_lookup_with_custom_human_name + # Set up complex I18n structure + I18n.backend.store_translations(:en, { + activemodel: { + state_machines: { + foo: { + status: { + states: { + pending: 'Model Specific Pending' + } + } + }, + status: { + states: { + pending: 'Machine Specific Pending' + } + }, + states: { + pending: 'Generic Pending' + } + } + } + }) + + machine = StateMachines::Machine.new(@model, :status) do + state :pending, human_name: 'Overridden Pending' + end + + # Should use the custom human_name, not any of the I18n translations + assert_equal 'Overridden Pending', machine.states[:pending].human_name(@model) + end + + def teardown + # Clear I18n translations after each test + I18n.backend.reload! + end +end