Allow using different preference defaults depending on a Solidus version #4064

waiting-for-dev · 2021-05-17T06:02:37Z

This feature is similar to Rail's
load_defaults.
However, instead of loading imperatively, the defaults for a given
version keep the history of the value in the preference declaration.
Both systems are upgrade-friendly, as users need to adjust the new
version defaults manually. The main advantage of this commit
implementation is communication (as the code tells about its current
state and history). However, it's architecturally more complex.

Spree::Core::VersionedValue implements the core behavior. This class
accepts a specification of how a value has changed in time and returns
the result for a given solidus version. The specification consists of an
initial value and a series of boundaries when it changed:

value = Spree::Core::VersionedValue.new(false, "3.0" => true)
value.call("2.0") # => false
value.call("3.0") # => true

Spree::AppConfiguration bundles the behavior into the preferences
system. Its .by_version method builds a Proc that accepts a solidus
version and returns the corresponding value. It's meant to be used in
the default: keyword argument:

preference :foo, :boolean, default: by_version(false, "3.0" => false)

Accordingly, Spree::Preferences::Preferable has been modified to
provide arguments to the default: Proc. As this module is not only
included in Spree::AppConfiguration, it defaults to supply no
arguments to it, but Spree::AppConfiguration overrides it to add a
#loaded_defaults attribute.

Users can specify the version defaults they want to use through the
#load_defaults method in Spree::AppConfiguration (which gets bound
in the Spree.config method used in the spree initializer):

Spree.config do |config|
  config.load_defaults '3.1'
end

We have modified the spree generator template to add that line of code
defaulting to the current Solidus version. As the sibling step, we
should create an update task creating another initializer à la
new_framework_defaults in Rails.

The tricky part in the rollout of this new feature could be the first
upgrade after it, as current Solidus installations don't know about
load_defaults. Therefore, an upgrade will leave them with the new
defaults unless they manually add the load_defaults line.

Related to discussion: #4045

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)
I have attached screenshots to this PR for visual changes (if needed)

core/lib/spree/app_configuration.rb

core/lib/spree/preferences/preferable_class_methods.rb

aldesantis

@waiting-for-dev this is really impressive, thanks! I agree that it's architecturally more complex, but the implementation is simple and elegant enough to make dealing with that complexity almost a no-brainer.

kennyadsl · 2021-05-21T12:12:31Z

Before merging, I'd like to understand how we want to communicate this to users. Can a section similar to this Rails one be enough?

kennyadsl

Left a non-blocking comment. Thank you Marc, this is awesome!

core/spec/lib/spree/app_configuration_spec.rb

waiting-for-dev · 2021-05-26T10:34:38Z

I added a new commit generalizing the possibility to use different defaults by version to any Preferable class, and not only to Spree::AppConfiguration. Please, @kennyadsl , @aldesantis , I'd appreciate if you can take a look at the second commit.

waiting-for-dev · 2021-05-27T11:05:45Z

I added a new commit generalizing the possibility to use different defaults by version to any Preferable class, and not only to Spree::AppConfiguration. Please, @kennyadsl , @aldesantis , I'd appreciate if you can take a look at the second commit.

I pushed a new commit where, instead of enabling the new feature for any Preferable class, it's only done for descendants of Spree::Preferences::Configuration. I.e., configuration classes for core, backend, frontend & engine. After talking with @kennyadsl , we decided to set other Preferable classes aside, as they are ActiveRecord models that have very different behavior and can be configured through the backend.

Following the work done in solidusio#4064, this commits introduces a Rails generator that creates a new initializer called, by default, `new_solidus_defaults.rb`. This initializer works in a very similar way that [`new_framework_defaults.rb` does in Rails](https://guides.rubyonrails.org/upgrading_ruby_on_rails.html#configure-framework-defaults). It allows users to preview the defaults that have changed on a new Solidus version, as they are printed one by one on a commented line. Users can then keep enabling them while updating their application code. The main difference with the Rails system is that the `load_defaults` call is added to the initializer instead of the main configuration file (it would be the `spree.rb` initializer in our case). Users can remove it altogether when they're done with the process. The reason is that in our case, the `loaded_defaults` instance variable of the application configurations defaults to the last version, so it's not needed anymore when the process is done. In contrast, calling `load_defaults` in Rails changes the defaults imperatively to the new values (so it defaults to the outdated ones). For now, we're leaving this as a generator, but we could reference it from a rake task, although probably there's no need. Be aware that users need to provide the version from which they are updating. It's an option that offers more flexibility, as users can update from versions different from the latest one. It also plays well with our system's flexibility, for instance, to change defaults between a pre-release and a release. However, we can add some code to default to the latest minor version, but we should keep that information in our code, and that's a small burden for our update process.

aldesantis · 2021-05-28T13:49:29Z

@waiting-for-dev makes sense to me!

jarednorman

A lot of moving parts here, but I've gone over it a couple times now and I think it all makes sense.

kennyadsl · 2021-06-01T07:48:21Z

I think we are ready here, can we just rebase to remove commit 2 and 3 (3 reverts 2 and we can probably have the history cleaner by doing so).

This feature is similar to [Rail's `load_defaults`](https://guides.rubyonrails.org/configuring.html#results-of-config-load-defaults). However, instead of loading imperatively, the defaults for a given version keep the history of the value in the preference declaration. Both systems are upgrade-friendly, as users need to adjust the new version defaults manually. The main advantage of this commit implementation is communication (as the code tells about its current state and history). However, it's architecturally more complex. `Spree::Core::VersionedValue` implements the core behavior. This class accepts a specification of how a value has changed in time and returns the result for a given solidus version. The specification consists of an initial value and a series of boundaries when it changed: ```ruby value = Spree::Core::VersionedValue.new(false, "3.0" => true) value.call("2.0") # => false value.call("3.0") # => true ``` `Spree::Preferences::Configuration` bundles the behavior into the preferences system. Its `.by_version` method builds a `Proc` that accepts a solidus version and returns the corresponding value. It's meant to be used in the `default:` keyword argument: ```ruby preference :foo, :boolean, default: by_version(false, "3.0" => false) ``` Accordingly, `Spree::Preferences::Preferable` has been modified to provide arguments to the `default:` `Proc`. As this module is not only included in `Spree::Preferences::Configuration` classes, it defaults to supply no arguments to it, but `Spree::Preferences::Configuration` overrides it to add the `#loaded_defaults` attribute. Users can specify the version defaults they want to use through the `#load_defaults` method in `Spree::AppConfiguration`, `Spree::BackendConfiguration`, `Spree::FrontendConfiguration` and `Spree::ApiConfiguration`. For instance, for `Spree::AppConfiguration', which gets bound in the `Spree.config` method used in the `spree` initializer: ```ruby Spree.config do |config| config.load_defaults '3.1' end ``` We have modified the `spree` generator template to add that line of code defaulting to the current Solidus version. As the sibling step, we should create an update task creating another initializer à la `new_framework_defaults` in Rails. Related to discussion solidusio#4045

waiting-for-dev · 2021-06-01T09:02:20Z

I think we are ready here, can we just rebase to remove commit 2 and 3 (3 reverts 2 and we can probably have the history cleaner by doing so).

I squashed all the commits and updated the commit message. I also rebased from master.

I made an extra addition, adding the load_defaults call in the backend, frontend & API configuration blocks in the spree.rb initializer template.

Following the work done in solidusio#4064, this commits introduces a Rails generator that creates a new initializer called, by default, `new_solidus_defaults.rb`. This initializer works in a very similar way that [`new_framework_defaults.rb` does in Rails](https://guides.rubyonrails.org/upgrading_ruby_on_rails.html#configure-framework-defaults). It allows users to preview the defaults that have changed on a new Solidus version, as they are printed one by one on a commented line. Users can then keep enabling them while updating their application code. The main difference with the Rails system is that the `load_defaults` call is added to the initializer instead of the main configuration file (it would be the `spree.rb` initializer in our case). Users can remove it altogether when they're done with the process. The reason is that in our case, the `loaded_defaults` instance variable of the application configurations defaults to the last version, so it's not needed anymore when the process is done. In contrast, calling `load_defaults` in Rails changes the defaults imperatively to the new values (so it defaults to the outdated ones). For now, we're leaving this as a generator, but we could reference it from a rake task, although probably there's no need. Be aware that users need to provide the version from which they are updating. It's an option that offers more flexibility, as users can update from versions different from the latest one. It also plays well with our system's flexibility, for instance, to change defaults between a pre-release and a release. However, we can add some code to default to the latest minor version, but we should keep that information in our code, and that's a small burden for our update process.

jarednorman

The approach for handling changes is very clever.

kennyadsl · 2021-06-09T12:28:35Z

Merging, but I think we need to call config.load_defaults in the dummy apps as well.

waiting-for-dev · 2021-06-10T04:02:15Z

Merging, but I think we need to call config.load_defaults in the dummy apps as well.

Thanks, @kennyadsl. It's not needed because of load_defaults defaulting to the current Solidus version, but I'm introducing it on #4087 to avoid the warning we introduce there.

Following the work done in solidusio#4064, this commits introduces a Rails generator that creates a new initializer called, by default, `new_solidus_defaults.rb`. This initializer works in a very similar way that [`new_framework_defaults.rb` does in Rails](https://guides.rubyonrails.org/upgrading_ruby_on_rails.html#configure-framework-defaults). It allows users to preview the defaults that have changed on a new Solidus version, as they are printed one by one on a commented line. Users can then keep enabling them while updating their application code. We're adding the `load_defaults` call with the old version value to the generated initializer. Users can remove the file altogether when they're done with the process. At that point, we require them to add `load_defaults` with the new version to the main initializer file (`spree.rb`). Even if there's no actual need for that, as `loaded_defaults` on the configuration class defaults to the current Solidus version, we want to enforce that so that users are on the safe side for the next version upgrade. If they don't add it, we emit a warning. For now, we're leaving this as a generator, but we could reference it from a rake task, although probably there's no need. Be aware that users need to provide the version from which they are updating. It's an option that offers more flexibility, as users can update from versions different from the latest one. It also plays well with our system's flexibility, for instance, to change defaults between a pre-release and a release. However, we can add some code to default to the latest minor version, but we should keep that information in our code, and that's a small burden for our update process.

Solidus 3.1 shipped with a new preferences system, where a default can take different values depending on the Solidus version defaults that have been loaded. See solidusio#4064 for details. To achieve those above, when a proc is given as the default value constructor, it now should take the loaded Solidus version as an argument. That's a breaking change, as some extensions or user-defined preferences may be using zero-arity lambdas. This commit deprecates zero-arity lambdas but wraps them into another lambda, taking and disregarding a single argument. That's only needed for procs with lambda semantics, as raw procs will ignore the provided extra argument. When it comes to the implementation, as it's something to be ditched in the next major release, we've opted for the more straightforward solution. I.e., wrapping the lambda into the `Preferable` module even if it only affects `AppConfiguration` classes. The default-handling logic is very entangled into the former, and it'd take more work to extract it. Fixes solidusio#4165

Solidus 3.1 shipped with a new preferences system, where a default can take different values depending on the Solidus version defaults that have been loaded. See #4064 for details. To achieve those above, when a proc is given as the default value constructor, it now should take the loaded Solidus version as an argument. That's a breaking change, as some extensions or user-defined preferences may be using zero-arity lambdas. This commit deprecates zero-arity lambdas but wraps them into another lambda, taking and disregarding a single argument. That's only needed for procs with lambda semantics, as raw procs will ignore the provided extra argument. When it comes to the implementation, as it's something to be ditched in the next major release, we've opted for the more straightforward solution. I.e., wrapping the lambda into the `Preferable` module even if it only affects `AppConfiguration` classes. The default-handling logic is very entangled into the former, and it'd take more work to extract it. Fixes #4165

As instance methods are deprecated since this PR[0]. [0] `https://github.com/solidusio/solidus/pull/4064`

Following the work done in solidusio#4064, this commits introduces a Rails generator that creates a new initializer called, by default, `new_solidus_defaults.rb`. This initializer works in a very similar way that [`new_framework_defaults.rb` does in Rails](https://guides.rubyonrails.org/upgrading_ruby_on_rails.html#configure-framework-defaults). It allows users to preview the defaults that have changed on a new Solidus version, as they are printed one by one on a commented line. Users can then keep enabling them while updating their application code. We're adding the `load_defaults` call with the old version value to the generated initializer. Users can remove the file altogether when they're done with the process. At that point, we require them to add `load_defaults` with the new version to the main initializer file (`spree.rb`). Even if there's no actual need for that, as `loaded_defaults` on the configuration class defaults to the current Solidus version, we want to enforce that so that users are on the safe side for the next version upgrade. If they don't add it, we emit a warning. For now, we're leaving this as a generator, but we could reference it from a rake task, although probably there's no need. Be aware that users need to provide the version from which they are updating. It's an option that offers more flexibility, as users can update from versions different from the latest one. It also plays well with our system's flexibility, for instance, to change defaults between a pre-release and a release. However, we can add some code to default to the latest minor version, but we should keep that information in our code, and that's a small burden for our update process.

Solidus 3.1 shipped with a new preferences system, where a default can take different values depending on the Solidus version defaults that have been loaded. See solidusio#4064 for details. To achieve those above, when a proc is given as the default value constructor, it now should take the loaded Solidus version as an argument. That's a breaking change, as some extensions or user-defined preferences may be using zero-arity lambdas. This commit deprecates zero-arity lambdas but wraps them into another lambda, taking and disregarding a single argument. That's only needed for procs with lambda semantics, as raw procs will ignore the provided extra argument. When it comes to the implementation, as it's something to be ditched in the next major release, we've opted for the more straightforward solution. I.e., wrapping the lambda into the `Preferable` module even if it only affects `AppConfiguration` classes. The default-handling logic is very entangled into the former, and it'd take more work to extract it. Fixes solidusio#4165

Following the work done in solidusio#4064, this commits introduces a Rails generator that creates a new initializer called, by default, `new_solidus_defaults.rb`. This initializer works in a very similar way that [`new_framework_defaults.rb` does in Rails](https://guides.rubyonrails.org/upgrading_ruby_on_rails.html#configure-framework-defaults). It allows users to preview the defaults that have changed on a new Solidus version, as they are printed one by one on a commented line. Users can then keep enabling them while updating their application code. We're adding the `load_defaults` call with the old version value to the generated initializer. Users can remove the file altogether when they're done with the process. At that point, we require them to add `load_defaults` with the new version to the main initializer file (`spree.rb`). Even if there's no actual need for that, as `loaded_defaults` on the configuration class defaults to the current Solidus version, we want to enforce that so that users are on the safe side for the next version upgrade. If they don't add it, we emit a warning. For now, we're leaving this as a generator, but we could reference it from a rake task, although probably there's no need. Be aware that users need to provide the version from which they are updating. It's an option that offers more flexibility, as users can update from versions different from the latest one. It also plays well with our system's flexibility, for instance, to change defaults between a pre-release and a release. However, we can add some code to default to the latest minor version, but we should keep that information in our code, and that's a small burden for our update process.

Solidus 3.1 shipped with a new preferences system, where a default can take different values depending on the Solidus version defaults that have been loaded. See solidusio#4064 for details. To achieve those above, when a proc is given as the default value constructor, it now should take the loaded Solidus version as an argument. That's a breaking change, as some extensions or user-defined preferences may be using zero-arity lambdas. This commit deprecates zero-arity lambdas but wraps them into another lambda, taking and disregarding a single argument. That's only needed for procs with lambda semantics, as raw procs will ignore the provided extra argument. When it comes to the implementation, as it's something to be ditched in the next major release, we've opted for the more straightforward solution. I.e., wrapping the lambda into the `Preferable` module even if it only affects `AppConfiguration` classes. The default-handling logic is very entangled into the former, and it'd take more work to extract it. Fixes solidusio#4165

houndci-bot reviewed May 17, 2021

View reviewed changes

core/lib/spree/app_configuration.rb Outdated Show resolved Hide resolved

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

waiting-for-dev force-pushed the waiting-for-dev/default_by_version branch from f2385a8 to 20f4db2 Compare May 17, 2021 06:04

aldesantis approved these changes May 20, 2021

View reviewed changes

aldesantis requested a review from a team May 20, 2021 10:00

kennyadsl approved these changes May 21, 2021

View reviewed changes

core/spec/lib/spree/app_configuration_spec.rb Outdated Show resolved Hide resolved

kennyadsl added Important Change changelog:solidus_core Changes to the solidus_core gem labels May 21, 2021

waiting-for-dev force-pushed the waiting-for-dev/default_by_version branch from 20f4db2 to 040f586 Compare May 21, 2021 13:23

waiting-for-dev mentioned this pull request May 28, 2021

Introduce Solidus update process #4087

Merged

5 tasks

aldesantis approved these changes May 28, 2021

View reviewed changes

jarednorman approved these changes May 31, 2021

View reviewed changes

waiting-for-dev force-pushed the waiting-for-dev/default_by_version branch from 21911cd to b897ecc Compare June 1, 2021 09:01

jarednorman approved these changes Jun 1, 2021

View reviewed changes

kennyadsl merged commit f6ce20f into solidusio:master Jun 9, 2021

kennyadsl deleted the waiting-for-dev/default_by_version branch June 9, 2021 12:28

waiting-for-dev mentioned this pull request Sep 15, 2021

Add deprecation path for arity-zero preference defaults #4170

Merged

5 tasks

garciajordy mentioned this pull request Jan 3, 2022

Make default_environment a class method boomerdigital/solidus_avatax_certified#168

Merged

garciajordy referenced this pull request in garciajordy/solidus_avatax_certified Jan 5, 2022

Make default_environment a class method

24e3c85

As instance methods are deprecated since this PR[0]. [0] `https://github.com/solidusio/solidus/pull/4064`

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow using different preference defaults depending on a Solidus version #4064

Allow using different preference defaults depending on a Solidus version #4064

waiting-for-dev commented May 17, 2021

aldesantis left a comment

kennyadsl commented May 21, 2021

kennyadsl left a comment

waiting-for-dev commented May 26, 2021

waiting-for-dev commented May 27, 2021

aldesantis commented May 28, 2021

jarednorman left a comment

kennyadsl commented Jun 1, 2021

waiting-for-dev commented Jun 1, 2021

jarednorman left a comment

kennyadsl commented Jun 9, 2021

waiting-for-dev commented Jun 10, 2021

Allow using different preference defaults depending on a Solidus version #4064

Allow using different preference defaults depending on a Solidus version #4064

Conversation

waiting-for-dev commented May 17, 2021

aldesantis left a comment

Choose a reason for hiding this comment

kennyadsl commented May 21, 2021

kennyadsl left a comment

Choose a reason for hiding this comment

waiting-for-dev commented May 26, 2021

waiting-for-dev commented May 27, 2021

aldesantis commented May 28, 2021

jarednorman left a comment

Choose a reason for hiding this comment

kennyadsl commented Jun 1, 2021

waiting-for-dev commented Jun 1, 2021

jarednorman left a comment

Choose a reason for hiding this comment

kennyadsl commented Jun 9, 2021

waiting-for-dev commented Jun 10, 2021