From 0a2ed0327857ac3df612eb79bf24c937383368ce Mon Sep 17 00:00:00 2001 From: Tim Rogers Date: Tue, 13 Feb 2018 17:24:03 +0000 Subject: [PATCH 1/2] Support ActiveRecord transitions which don't include Statesman::Adapters::ActiveRecordTransition In #306, we made the "updated timestamp column" for a transition configurable through a `class_attribute` on `Statesman::Adapters::ActiveRecordTransition`, which is generally `include`d on transition classes. However, not all transition classes include this module, and indeed we have [advised][1] in the README not to include it when using a PostgreSQL JSON column type for metadata, as pointed out by #309. This leads to an exception like the following when trying to perform a transition: ``` NoMethodError: undefined method `updated_timestamp_column' for # ``` This fixes the issue by defaulting to using `:updated_at` as before if the transition class doesn't have an `.updated_timestamp_column` method defined. As a follow-up action, it'd be nice to get all transitions including the module to have a more consistent and friendly base for future improvements to Statesman, as discussed in the [issue comments][2]. This, however, would be a breaking change and should be introduced separately after more thought. Fixes #309. [1]: https://github.com/gocardless/statesman#using-postgresql-json-column [2]: https://github.com/gocardless/statesman/issues/309#issuecomment-365336286 --- lib/statesman/adapters/active_record.rb | 17 +++++++++++++++-- spec/statesman/adapters/active_record_spec.rb | 13 +++++++++++++ spec/support/active_record.rb | 7 +++++++ 3 files changed, 35 insertions(+), 2 deletions(-) diff --git a/lib/statesman/adapters/active_record.rb b/lib/statesman/adapters/active_record.rb index 6dd6f961..eaeb5ca8 100644 --- a/lib/statesman/adapters/active_record.rb +++ b/lib/statesman/adapters/active_record.rb @@ -123,7 +123,20 @@ def transition_conflict_error?(e) end def with_updated_timestamp(params) - return params if @transition_class.updated_timestamp_column.nil? + # TODO: Once we've set expectations that transition classes should conform to + # the interface of Adapters::ActiveRecordTransition as a breaking change in the + # next major version, we can stop calling `#respond_to?` first and instead + # assume that there is a `.updated_timestamp_column` method we can call. + # + # At the moment, most transition classes will include the module, but not all, + # not least because it doesn't work with PostgreSQL JSON columns for metadata. + column = if @transition_class.respond_to?(:updated_timestamp_column) + @transition_class.updated_timestamp_column + else + ActiveRecordTransition::DEFAULT_UPDATED_TIMESTAMP_COLUMN + end + + return params if column.nil? timestamp = if ::ActiveRecord::Base.default_timezone == :utc Time.now.utc @@ -131,7 +144,7 @@ def with_updated_timestamp(params) Time.now end - params.merge(@transition_class.updated_timestamp_column => timestamp) + params.merge(column => timestamp) end end end diff --git a/spec/statesman/adapters/active_record_spec.rb b/spec/statesman/adapters/active_record_spec.rb index 62bcdb05..cec103cb 100644 --- a/spec/statesman/adapters/active_record_spec.rb +++ b/spec/statesman/adapters/active_record_spec.rb @@ -163,6 +163,19 @@ to(change { previous_transition.reload.updated_at }) end + context "for a transition class without an updated timestamp column attribute" do + let!(:adapter) do + described_class.new(MyActiveRecordModelTransitionWithoutInclude, + model, + observer) + end + + it "defaults to touching the previous transition's updated_at timestamp" do + expect { Timecop.freeze(Time.now + 5.seconds) { create } }. + to(change { previous_transition.reload.updated_at }) + end + end + context "with a custom updated timestamp column set" do around do |example| MyActiveRecordModelTransition.updated_timestamp_column.tap do |original_value| diff --git a/spec/support/active_record.rb b/spec/support/active_record.rb index 2a72ecb6..efe3ad7d 100644 --- a/spec/support/active_record.rb +++ b/spec/support/active_record.rb @@ -40,6 +40,13 @@ class MyActiveRecordModelTransition < ActiveRecord::Base serialize :metadata, JSON end +class MyActiveRecordModelTransitionWithoutInclude < ActiveRecord::Base + self.table_name = "my_active_record_model_transitions" + + belongs_to :my_active_record_model + serialize :metadata, JSON +end + class CreateMyActiveRecordModelMigration < MIGRATION_CLASS def change create_table :my_active_record_models do |t| From 42cdd8401ce7c88c8895c20a52e06ff42064fa7f Mon Sep 17 00:00:00 2001 From: Tim Rogers Date: Wed, 14 Feb 2018 08:55:11 +0000 Subject: [PATCH 2/2] Clarify README instructions for ActiveRecord * If you're using the ActiveRecord adapter, your transition class should `include Statesman::Adapters::ActiveRecordTransition`. * If you're using a PostgreSQL JSON column to store metadata, you won't be able to `include Statesman::Adapters::ActiveRecordTransition` as there is an incompatability (which we'll aim to resolve soon, see #309). Without this module, if you want to customise your transition class's "updated timestamp column", you will need to define `.updated_timestamp_column` yourself. --- README.md | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index a72f6b9f..2f2add53 100644 --- a/README.md +++ b/README.md @@ -121,7 +121,7 @@ Order.not_in_state(:checking_out) # => [#] By default Statesman stores transition history in memory only. It can be persisted by configuring Statesman to use a different adapter. For example, -ActiveRecord within Rails: +for ActiveRecord within Rails: `config/initializers/statesman.rb`: @@ -137,6 +137,10 @@ Generate the transition model: $ rails g statesman:active_record_transition Order OrderTransition ``` +Your transition class should +`include Statesman::Adapters::ActiveRecordTransition` if you're using the +ActiveRecord adapter. + If you're using the ActiveRecord adapter and decide not to include the default `updated_at` column in your transition table, you'll need to configure the `updated_timestamp_column` option on the transition class, setting it to another column @@ -178,8 +182,12 @@ or 5. To do that t.json :metadata, default: {} ``` -* Remove `include Statesman::Adapters::ActiveRecordTransition` statement from your - transition model +* Remove the `include Statesman::Adapters::ActiveRecordTransition` statement from + your transition model. (If you want to customise your transition class's "updated + timestamp column", as described above, you should define a + `.updated_timestamp_column` method on your class and return the name of the column + as a symbol, or `nil` if you don't want to record an updated timestamp on + transitions.) ## Configuration