Deprecate calling preferences without serialization #4013

mamhoff · 2021-03-29T10:37:07Z

Description

In Solidus < 2.11.4, Spree::Base had a serialize :preferences, Hash statement. This statement fails in Rails 6.1ff, as after that Rails will try deserializing the column even if it isn't there.

What this commit does is:

Remove the serialize call from Spree::Base, as many inheriting classes do not have a preferences column
Add the serialize and after_initialize call to those base classes that need it
- Spree::Calculator
- Spree::PromotionAction
- Spree::PaymentMethod
- Spree::PromotionRule
Add a new preferences getter to Spree::Base that exhibits the following behaviour: If preferences is not a Hash, we can safely assume that the class of the instance we're trying to get preferences from expects Spree::Base to perform the serialize call for them. In that case, call serialize on the instance's class, ensuring that all future loads of this type of object will have preferences serialized, and parse the attribute using ActiveRecord::Type::Serialized (as that's how Spree::Base worked).
Remove the preferences_coder_class method. I don't think it's prudent to add an extension point here for something that was hardcoded in the past and can easily be circumnavigated by e.g. setting one's custom class to serialize using JSON:

class MyPreferences
  serialize :preferences, JSON
  after_initialize :initialize_preference_defaults
  ...
end

This PR goes against the v2.11 branch of Solidus; I would like to uphold my PR #3997 for the master branch.

This means that the deprecation window for this change would be v2.11.8 => v3.0.0, but I think that's doable. There is a bit of work in related projects, but it just consists in adding to lines to configuration classes.

Checklist:

I have followed Pull Request guidelines
I have added a detailed description into each commit message
~~I have updated Guides and README accordingly to this change (if needed)~~
I have added tests to cover this change (if needed)
~~II have attached screenshots to this PR for visual changes (if needed)~~

core/app/models/spree/base.rb

core/spec/models/spree/calculator_spec.rb

core/app/models/spree/base.rb

tvdeyen

I like this approach.

kennyadsl

Great stuff Martin! I only left some comments/questions but overall I think it's good to merge both in 2.11 and 3.0. If that was breaking your application, I think it's safe to consider this a patch.

.circleci/config.yml

core/app/models/spree/promotion_rule.rb

core/app/models/spree/base.rb

core/spec/models/spree/base_spec.rb

core/lib/spree/preferences/persistable.rb

mamhoff

What about providing a module/concern for this? I'm not particularly interested in DRYing things, but if we need to make other changes in the future (eg. deprecate the way we serialize/initialize defaults) it could be handy.

I like that idea. Especially because that means we can have the callback and the after_initialize call in the same file. Pretty nice!

core/app/models/spree/base.rb

core/spec/models/spree/base_spec.rb

core/app/models/spree/base.rb

core/app/models/spree/calculator.rb

core/app/models/spree/base.rb

core/spec/models/spree/promotion_action_spec.rb

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. For classes that expect Spree::Base to call `serialize`, this commit adds deprecation messaging and parsing to Spree::Base#preferences.

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

This removes even more code from Spree::Base, and groups the few lines of code that are needed for persisted preferences in one module.

The use of any of the methods in `Preferable` without `Persistable` makes no sense. Let's declutter Spree::Base some more!

The error message here calls for "persistable", because all objects that inherit from Spree::Base *actually* need the Persistable module.

If you try accessing a calculator with preferences in the DB, and that calculator hasn't seen the `serialize` call, Rails will try accessing the preferences column directly and return a string :(

These methods aren't available automatically any longer, thanks to Rails 6.1. They actually weren't in the first place, because not every model has a `preferences` column.

kennyadsl

I left a non-blocking comment, let me know your thoughts but I'm fine merging even as is.

Thanks!

kennyadsl · 2021-03-31T06:40:03Z

core/spec/models/spree/base_spec.rb

+    end
+
+    before(:each) do
+      allow(Spree::Deprecation).to receive(:warn).


What about turning this into an expect? The concern is that it may not be emitting the deprecation warning when we expect this happens. I mean, stubbing the deprecation should not be used to avoid failures and clean the logs only but to actually test that users get a deprecation message.

I can't do that, because on the second test that runs, the method will already be defined and an expect would fail. Undoing that include turns out to be a lot of work...

Got it, and I can't think at a clean way to do it.

gmacdougall · 2021-03-31T15:39:11Z

I like this change and what it's doing. Thanks for the contribution Martin!

houndci-bot reviewed Mar 29, 2021

View reviewed changes

core/app/models/spree/base.rb Outdated Show resolved Hide resolved

core/app/models/spree/base.rb Outdated Show resolved Hide resolved

core/spec/models/spree/calculator_spec.rb Outdated Show resolved Hide resolved

mamhoff force-pushed the deprecate-calling-preferences-without-serialization branch 4 times, most recently from a91da40 to 48ee499 Compare March 29, 2021 11:52

houndci-bot reviewed Mar 29, 2021

View reviewed changes

core/app/models/spree/base.rb Outdated Show resolved Hide resolved

tvdeyen approved these changes Mar 29, 2021

View reviewed changes

mamhoff force-pushed the deprecate-calling-preferences-without-serialization branch 3 times, most recently from 376870c to 2a88e12 Compare March 29, 2021 16:20

kennyadsl reviewed Mar 29, 2021

View reviewed changes

mamhoff force-pushed the deprecate-calling-preferences-without-serialization branch from 2a88e12 to dd8db61 Compare March 29, 2021 20:54

houndci-bot reviewed Mar 29, 2021

View reviewed changes

core/lib/spree/preferences/persistable.rb Outdated Show resolved Hide resolved

mamhoff commented Mar 29, 2021

View reviewed changes

core/app/models/spree/base.rb Show resolved Hide resolved

core/app/models/spree/base.rb Outdated Show resolved Hide resolved

core/spec/models/spree/base_spec.rb Show resolved Hide resolved

mamhoff force-pushed the deprecate-calling-preferences-without-serialization branch from dd8db61 to 2313219 Compare March 29, 2021 21:07

houndci-bot reviewed Mar 29, 2021

View reviewed changes

core/app/models/spree/base.rb Outdated Show resolved Hide resolved

core/app/models/spree/base.rb Outdated Show resolved Hide resolved

core/app/models/spree/base.rb Outdated Show resolved Hide resolved

core/app/models/spree/base.rb Outdated Show resolved Hide resolved

mamhoff force-pushed the deprecate-calling-preferences-without-serialization branch 2 times, most recently from 17ca6d5 to e3d47e4 Compare March 30, 2021 07:31

kennyadsl reviewed Mar 30, 2021

View reviewed changes

core/app/models/spree/base.rb Outdated Show resolved Hide resolved

core/app/models/spree/calculator.rb Show resolved Hide resolved

core/app/models/spree/base.rb Outdated Show resolved Hide resolved

core/spec/models/spree/promotion_action_spec.rb Show resolved Hide resolved

mamhoff force-pushed the deprecate-calling-preferences-without-serialization branch from e3d47e4 to 9c472a9 Compare March 30, 2021 11:55

houndci-bot reviewed Mar 30, 2021

View reviewed changes

core/spec/models/spree/stock/estimator_spec.rb Outdated Show resolved Hide resolved

mamhoff mentioned this pull request Mar 30, 2021

Failing spec for calculator with preferences that aren't known #4005

Closed

5 tasks

mamhoff force-pushed the deprecate-calling-preferences-without-serialization branch from 8a97d6b to 302b078 Compare March 30, 2021 12:06

mamhoff added 5 commits March 30, 2021 14:36

Extract methods related to persisted preferences to module

8eec169

This removes even more code from Spree::Base, and groups the few lines of code that are needed for persisted preferences in one module.

Only include Preferable on classes actually needing it

6299f73

The use of any of the methods in `Preferable` without `Persistable` makes no sense. Let's declutter Spree::Base some more!

Deprecate calling "preference" without including Preferable

531debb

The error message here calls for "persistable", because all objects that inherit from Spree::Base *actually* need the Persistable module.

Failing for calculator with preferences that aren't known

dfcdb8f

If you try accessing a calculator with preferences in the DB, and that calculator hasn't seen the `serialize` call, Rails will try accessing the preferences column directly and return a string :(

Update Guides for Preferences::Persistable

167299e

These methods aren't available automatically any longer, thanks to Rails 6.1. They actually weren't in the first place, because not every model has a `preferences` column.

mamhoff mentioned this pull request Mar 30, 2021

Include Preference Handling as a Module #4015

Merged

4 tasks

mamhoff force-pushed the deprecate-calling-preferences-without-serialization branch from 302b078 to 167299e Compare March 30, 2021 12:52

kennyadsl approved these changes Mar 31, 2021

View reviewed changes

kennyadsl requested a review from a team March 31, 2021 06:47

jarednorman approved these changes Mar 31, 2021

View reviewed changes

kennyadsl merged commit 7566958 into solidusio:v2.11 Apr 1, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate calling preferences without serialization #4013

Deprecate calling preferences without serialization #4013

mamhoff commented Mar 29, 2021 •

edited

Loading

tvdeyen left a comment

kennyadsl left a comment

mamhoff left a comment

kennyadsl left a comment

kennyadsl Mar 31, 2021 •

edited

Loading

mamhoff Mar 31, 2021

kennyadsl Mar 31, 2021

gmacdougall commented Mar 31, 2021

Deprecate calling preferences without serialization #4013

Deprecate calling preferences without serialization #4013

Conversation

mamhoff commented Mar 29, 2021 • edited Loading

tvdeyen left a comment

Choose a reason for hiding this comment

kennyadsl left a comment

Choose a reason for hiding this comment

mamhoff left a comment

Choose a reason for hiding this comment

kennyadsl left a comment

Choose a reason for hiding this comment

kennyadsl Mar 31, 2021 • edited Loading

Choose a reason for hiding this comment

mamhoff Mar 31, 2021

Choose a reason for hiding this comment

kennyadsl Mar 31, 2021

Choose a reason for hiding this comment

gmacdougall commented Mar 31, 2021

mamhoff commented Mar 29, 2021 •

edited

Loading

kennyadsl Mar 31, 2021 •

edited

Loading