Fix preferences columns to be always serialized

[mamhoff]

Description

This fixes an issue that arose because of commit b947015, which
initialized the serialization of the preference column only if the
respective subclass had any preferences defined. This is not always the
case, and previous versions of Solidus would always allow a subclass of
e.g. Spree::Calculator to access their preferences attribute as a
Hash.

The intention of the commit in question was to speed up initialization
for models that do not have a preferences column. Instead of complex
metamagic, this moves the initialization of the preferences column and
preference defaults to those classes that actually needed, actually
yielding MORE performance benefits, as serialize and the
after_initialize hook are only called once, not once per preference
being defined.

Checklist:

• I have followed Pull Request guidelines
• I have added a detailed description into each commit message
• I have added tests to cover this change (if needed)

[kennyadsl]

Thanks Martin, one question: does this require to add

serialize :preferences, Hash
after_initialize :initialize_preference_defaults

explicitly to any custom model defined in host applications and extensions that has preferences?

[mamhoff]

@kennyadsl Yes, that is true. I could add a changelog entry and/or raise an exception when it's not defined.

[kennyadsl]

Can you please provide an example of the code broken by this change? It's still not super clear to me and would help to evaluate if this needed to be backported.

[mamhoff]

All of our calculators that do not have preferences (they're custom code) return nil instead of the previous {}. We have code that relies on that for setting preferences.

[mamhoff]

So, if you added a custom shipping calculator with no preferences, theses two lines would definitely break if preferences returns nil instead of a Hash: https://github.com/solidusio/solidus/blob/master/core/app/models/spree/stock/estimator.rb#L65-L66. And that's also what's breaking our build.

[kennyadsl]

I have a failing spec for the estimator when there's a shipping calculator without preferences

          # core/spec/models/spree/stock/estimator_spec.rb:96

          context 'with a custom shipping calculator with no preference' do
            class Spree::Calculator::Shipping::NoPreferences < Spree::ShippingCalculator
              def compute_package(_package)
                # no op
              end
            end

            let!(:shipping_methods) do
              [
                create(:shipping_method, calculator: Spree::Calculator::Shipping::NoPreferences.new)
              ]
            end

            it 'does not raise an error' do
              expect { subject.shipping_rates(package) }.not_to raise_error
            end
          end

I think the best approach here is by finding a way to let preferences return a Hash even when there are preferences set. That change is needed for Rails 6.1 compatibility and we can't just revert it.

[kennyadsl]

I should have a fix that doesn't involve reverting that commit, or introduce a breaking change. If that works well, I think we can still keep this PR for Solidus 3, I like that it's more explicit and can remove the hacky code I'm going to propose. I'd love to have a period when this is optional though and introduces a deprecation warning when the serialization is not explicit.

[kennyadsl]

@mamhoff I've submitted #3998, what do you think?

[houndci-bot]

Style/RedundantSelf: Redundant self detected.