Deprecate calling preferences without serialization

core/app/models/spree/base.rb

@@ -1,28 +1,50 @@
 # frozen_string_literal: true
 
+require 'spree/preferences/persistable'
+
 class Spree::Base < ActiveRecord::Base
-  include Spree::Preferences::Preferable
   include Spree::Core::Permalinks
   include Spree::RansackableAttributes
 
-  def preferences
-    read_attribute(:preferences) || self.class.preferences_coder_class.new
+  def self.preference(*args)
+    Spree::Deprecation.warn <<~WARN
+      #{name} has a `preferences` column, but does not explicitly (de)serialize this column.
+      In order to make #{name} work with future versions of Solidus (and Rails), please add the
+      following line to your class:
+      ```
+      class #{name}
+        include Spree::Preferences::Persistable
+        ...
+      end
+      ```
+    WARN
+    include Spree::Preferences::Persistable
+    preference(*args)
   end
 
-  def initialize_preference_defaults
-    if has_attribute?(:preferences)
-      self.preferences = default_preferences.merge(preferences)
-    end
-  end
+  def preferences
+    value = read_attribute(:preferences)
+    if !value.is_a?(Hash)
+      Spree::Deprecation.warn <<~WARN
+        #{self.class.name} has a `preferences` column, but does not explicitly (de)serialize this column.
+        In order to make #{self.class.name} work with future versions of Solidus (and Rails), please add the
+        following lines to your class:
+        ```
+        class #{self.class.name}
+          include Spree::Preferences::Persistable
+          ...
+        end
+        ```
+      WARN
+      self.class.include Spree::Preferences::Persistable
 
-  # Only run preference initialization on models which requires it. Improves
-  # performance of record initialization slightly.
-  def self.preference(*args)
-    # after_initialize can be called multiple times with the same symbol, it
-    # will only be called once on initialization.
-    serialize :preferences, preferences_coder_class
-    after_initialize :initialize_preference_defaults
-    super
+      ActiveRecord::Type::Serialized.new(
+        ActiveRecord::Type::Text.new,
+        ActiveRecord::Coders::YAMLColumn.new(:preferences, Hash)
+      ).deserialize(value)
+    else
+      value
+    end
   end
 
   if Kaminari.config.page_method_name != :page
@@ -35,10 +57,6 @@ def self.page(num)
     end
   end
 
-  def self.preferences_coder_class
-    Hash
-  end
-
   self.abstract_class = true
 
   # Provides a scope that should be included any time products

core/app/models/spree/calculator.rb

@@ -1,7 +1,11 @@
 # frozen_string_literal: true
 
+require 'spree/preferences/persistable'
+
 module Spree
   class Calculator < Spree::Base
+    include Spree::Preferences::Persistable
+
     belongs_to :calculable, polymorphic: true, optional: true
 
     # This method calls a compute_<computable> method. must be overriden in concrete calculator.

core/app/models/spree/payment_method.rb

@@ -1,5 +1,6 @@
 # frozen_string_literal: true
 
+require 'spree/preferences/persistable'
 require 'spree/preferences/statically_configurable'
 
 module Spree
@@ -11,6 +12,8 @@ module Spree
   # This class is not meant to be instantiated. Please create instances of concrete payment methods.
   #
   class PaymentMethod < Spree::Base
+    include Spree::Preferences::Persistable
+
     preference :server, :string, default: 'test'
     preference :test_mode, :boolean, default: true
 

core/app/models/spree/promotion_action.rb

@@ -1,11 +1,14 @@
 # frozen_string_literal: true
 
+require 'spree/preferences/persistable'
+
 module Spree
   # Base class for all types of promotion action.
   #
   # PromotionActions perform the necessary tasks when a promotion is activated
   # by an event and determined to be eligible.
   class PromotionAction < Spree::Base
+    include Spree::Preferences::Persistable
     include Spree::SoftDeletable
 
     belongs_to :promotion, class_name: 'Spree::Promotion', inverse_of: :promotion_actions, optional: true

core/app/models/spree/promotion_rule.rb

@@ -1,8 +1,12 @@
 # frozen_string_literal: true
 
+require 'spree/preferences/persistable'
+
 module Spree
   # Base class for all promotion rules
   class PromotionRule < Spree::Base
+    include Spree::Preferences::Persistable
+
     belongs_to :promotion, class_name: 'Spree::Promotion', inverse_of: :promotion_rules, optional: true
 
     scope :of_type, ->(type) { where(type: type) }

core/lib/spree/preferences/persistable.rb

@@ -0,0 +1,23 @@
+# frozen_string_literal: true
+
+module Spree
+  module Preferences
+    module Persistable
+      extend ActiveSupport::Concern
+
+      included do
+        include Spree::Preferences::Preferable
+        serialize :preferences, Hash
+        after_initialize :initialize_preference_defaults
+      end
+
+      private
+
+      def initialize_preference_defaults
+        if has_attribute?(:preferences)
+          self.preferences = default_preferences.merge(preferences)
+        end
+      end
+    end
+  end
+end

core/spec/models/spree/base_spec.rb

@@ -0,0 +1,62 @@
+# frozen_string_literal: true
+
+require "rails_helper"
+
+RSpec.describe Spree::Base do
+  before(:all) do
+    class CreatePrefTest < ActiveRecord::Migration[4.2]
+      def self.up
+        create_table :pref_tests do |item|
+          item.string :col
+          item.text :preferences
+        end
+      end
+
+      def self.down
+        drop_table :pref_tests
+      end
+    end
+
+    @migration_verbosity = ActiveRecord::Migration[4.2].verbose
+    ActiveRecord::Migration[4.2].verbose = false
+    CreatePrefTest.migrate(:up)
+  end
+
+  after(:all) do
+    CreatePrefTest.migrate(:down)
+    ActiveRecord::Migration[4.2].verbose = @migration_verbosity
+  end
+
+  context "with a class that has a preference column but does not explicitly serialize" do
+    before :all do
+      class PrefTestWithoutSerialization < Spree::Base
+        self.table_name = :pref_tests
+      end
+    end
+
+    before(:each) do
+      allow(Spree::Deprecation).to receive(:warn).
+        with(/^PrefTestWithoutSerialization has a `preferences` column, but does not explicitly \(de\)serialize this column.*/m, any_args)
+    end
+
+    after(:all) do
+      Object.send(:remove_const, :PrefTestWithoutSerialization)
+    end
+
+    it "returns a Hash nevertheless" do
+      instance = PrefTestWithoutSerialization.new
+      expect(instance.preferences).to be_a(Hash)
+    end
+
+    it "returns a Hash when there's already values in the table" do
+      ActiveRecord::Base.connection.execute("INSERT INTO pref_tests (col, preferences) VALUES ('test', '---\n:percent: 20')")
+      instance = PrefTestWithoutSerialization.first
+      expect(instance.preferences).to include(percent: 20)
+    end
+
+    it "includes the persistable module when calling #preference and sets the preference default" do
+      PrefTestWithoutSerialization.preference :percentage, :number, default: 5
+      expect(PrefTestWithoutSerialization.new.preferences).to eq(percentage: 5)
+    end
+  end
+end

core/spec/models/spree/calculator_spec.rb

@@ -12,16 +12,16 @@ def compute_simple_computable(_line_item)
   class SimpleComputable
   end
 
-  describe "#calculators" do
-    before do
-      expect(Spree::Deprecation).to receive(:warn).
-        with(/^Calling \.calculators is deprecated/, any_args)
-    end
+  describe "preferences" do
+    subject { SimpleCalculator.new.preferences }
+
+    it { is_expected.to eq({}) }
 
-    subject { Spree::Calculator.calculators }
+    context "with preferences stored" do
+      let(:calculator) { SimpleCalculator.create(preferences: { a: "1" }) }
+      subject { calculator.reload.preferences }
 
-    it 'returns the (deprecated) calculator step' do
-      expect(subject).to be_a Spree::Core::Environment::Calculators
+      it { is_expected.to eq({ a: "1" }) }
     end
   end
 

core/spec/models/spree/payment_method_spec.rb

@@ -32,6 +32,12 @@
     })
   end
 
+  describe "preferences" do
+    subject { described_class.new.preferences }
+
+    it { is_expected.to be_a(Hash) }
+  end
+
   describe "available_to_[<users>, <admin>, <store>]" do
     context "when searching for payment methods available to users and admins" do
       subject { Spree::PaymentMethod.available_to_users.available_to_admin }

core/spec/models/spree/preferences/preferable_spec.rb

@@ -327,6 +327,8 @@ def self.down
       CreatePrefTest.migrate(:up)
 
       class PrefTest < Spree::Base
+        include Spree::Preferences::Persistable
+
         preference :pref_test_pref, :string, default: 'abc'
         preference :pref_test_any, :any, default: []
         preference :pref_test_encrypted_string, :encrypted_string, encryption_key: 'VkYp3s6v9y$B?E(H+MbQeThWmZq4t7w!'

core/spec/models/spree/promotion_action_spec.rb

@@ -3,6 +3,12 @@
 require 'rails_helper'
 
 RSpec.describe Spree::PromotionAction, type: :model do
+  describe "preferences" do
+    subject { described_class.new.preferences }
+
+    it { is_expected.to eq({}) }
+  end
+
   describe '#remove_from' do
     class MyPromotionAction < Spree::PromotionAction
       def perform(options = {})

core/spec/models/spree/promotion_rule_spec.rb

@@ -12,6 +12,12 @@ def eligible?(_promotable, _options = {})
       end
     end
 
+    describe "preferences" do
+      subject { described_class.new.preferences }
+
+      it { is_expected.to be_a(Hash) }
+    end
+
     it "forces developer to implement eligible? method" do
       expect { BadTestRule.new.eligible?("promotable") }.to raise_error NotImplementedError
     end

core/spec/models/spree/stock/estimator_spec.rb

@@ -114,6 +114,29 @@ def compute_package(_package)
             end
           end
 
+          context 'with a custom shipping calculator with preference' do
+            class Spree::Calculator::Shipping::WithUnknownPreferences < Spree::ShippingCalculator
+              def compute_package(_package)
+                # no op
+              end
+            end
+
+            let!(:shipping_methods) do
+              [
+                create(
+                  :shipping_method,
+                  calculator: Spree::Calculator::Shipping::WithUnknownPreferences.new(
+                    preferences: { a: "b" }
+                  )
+                )
+              ]
+            end
+
+            it 'does not raise an error' do
+              expect { subject.shipping_rates(package) }.not_to raise_error
+            end
+          end
+
           context 'with two shipping methods of different cost' do
             let!(:shipping_methods) do
               [

guides/source/developers/preferences/add-model-preferences.html.md

@@ -4,7 +4,12 @@ Solidus comes with many model-specific preferences. They are configured to have
 default values that are appropriate for typical stores. Additional preferences
 can be added by your application or included extensions.
 
-Preferences can be set on any model that inherits from [`Spree::Base`][spree-base].
+Preferences can be set on any model that includes `Spree::Preferences::Persistable`.
+In core Solidus, these are all classes inheriting from:
+  - `Spree::Calculator`
+  - `Spree::PromotionAction`
+  - `Spree::PaymentMethod`
+  - `Spree::PromotionRule`
 
 Note that model preferences apply only to the current model. To learn more about
 application-wide preferences, see the [App configuration][app-configuration]
@@ -21,6 +26,7 @@ You can define preferences for a model within the model itself:
 ```ruby
 module MyStore
   class User < Spree::Base
+    include Spree::Preferences::Persistable
     preference :hot_salsa, :boolean
     preference :dark_chocolate, :boolean, default: true
     preference :color, :string
@@ -30,7 +36,7 @@ module MyStore
 end
 ```
 
-This will work because User is a subclass of Spree::Base. If found,
+This will work because User includes [`Spree::Preferences::Persistable`][spree-persistable]. If found,
 the preferences attribute gets serialized into a Hash and merged with the default values.
 
 <!-- TODO:
@@ -178,4 +184,4 @@ user.preferred_color_type # => :string
 ```
 
 [app-configuration]: app-configuration.html
-[spree-base]: https://github.com/solidusio/solidus/blob/master/core/app/models/spree/base.rb
+[spree-persistable]: https://github.com/solidusio/solidus/blob/master/core/lib/spree/preferences/persistable.rb