Load defaults for the latest Rails minor version in the dummy app #4035

waiting-for-dev · 2021-04-22T06:11:08Z

Before this change, the dummy app used in the test suite was not loading
any new default that Rails has added since 5.0.

However, we still use old defaults for three settings that make our test
suite fail. Each one of them will be worked out in individual commits.

See https://api.rubyonrails.org/classes/Rails/Application/Configuration.html#method-i-load_defaults

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/spec/lib/spree/core/testing_support/dummy_app_spec.rb

waiting-for-dev · 2021-04-22T08:39:21Z

I have started removing the overridden defaults in this same PR. If you'd prefer it to be in another one, please tell me.

However, look at the diff for each commit to better understanding.

kennyadsl · 2021-04-22T11:56:13Z

I think that removed the overridden defaults in this PR is ok.

Before this change, the dummy app used in the test suite was not loading any new default that Rails has added since 5.0. However, we still use old defaults for three settings that make our test suite fail. Each one of them will be worked out in individual commits. See https://api.rubyonrails.org/classes/Rails/Application/Configuration.html#method-i-load_defaults

`load_defaults` was added in the dummy app in a recent commit. This one fixes some tests so that we no longer need to use the old default for `belongs_to_required_by_default` setting. As the taxonomy association for taxons is now required, and we have indeed treated it as required so far, we just need to add it when we create taxons resources.

Using `load_defaults` for Rails 6.1 (added to the dummy app in a recent commit) makes `active_storage.track_variants` true by default. Here we remove the overridden setting and adapt our test to invalidate the blob with the new default in place. See also rails/rails#37901

core/app/models/spree/order.rb

We added the `load_defaults` setting to the dummy app used in the tests in a recent commit. Rails 6.1 added a new default `has_many_inversing` (see rails/rails#34533 & rails/rails#37429), which is enabled by default on new applications. This setting broke some tests and created a couple of bugs on Solidus. This commit should fix all of this. CI will only examine the case when the new setting is enabled, but a local run confirmed the suite is green when it's off. A couple of the modifications introduced are attributable to two different bugs in Rails discovered during the process: - rails/rails#42102 forces us to use `Spree::Order#shipments#push` instead of `Spree::Order#shipments#=` when the proposed shipments get created from the order. - rails/rails#42094 only applies to a rare corner case, but it hit us in the test case `takes into account a passed in scope` for `Spree::Core::Search::Variant`. We also fixed two bugs on our side: - `Spree::Variant#product` Rails association was configured as the inverse of `Spree::Product#variants`. However, `Spree::Product#variants` has a custom scope which filters out master variants (https://github.com/solidusio/solidus/blob/2ea829645b00fcedd5bfd69e045bddab7f40beb9/core/app/models/spree/product.rb#L47). Instead, `Spree::Product#variants_including_master` is now used. - `Spree::Stock::SimpleCoordinator#build_shipments` is meant to build shipments presented as an option to the user. To calculate their associated shipping rates, it delegates the shipments to `Spree::Stock::Estimator`. This service needs to know the order instance those shipments would be assigned to. With the current implementation, this information is taken from the shipment itself as it's already associated to that order when it's initialized on `Spree::Stock::Package` (https://github.com/solidusio/solidus/blob/2ea829645b00fcedd5bfd69e045bddab7f40beb9/core/app/models/spree/stock/package.rb#L127). Before the `has_many_inversing` feature, this initialization wasn't reflected in the inverse association `Spree::Order#shipments`, but it's no longer the case. For this reason, after we have calculated the shipping rates, we need to remove the shipping instances through `order.shipments = order.shipments - shipment`. Follow-up work could clean this up if we pass the order as a parameter to the estimator. Still, it would have backward compatibility issues for user-created estimators as the interface would change. The following snippet isolates the issue: ```ruby require 'bundler/inline' gemfile(true) do source 'https://rubygems.org' gem 'activerecord', '6.1.3.1' gem 'sqlite3' end require 'active_record' ActiveRecord::Base.has_many_inversing = true ActiveRecord::Base.establish_connection(adapter: 'sqlite3', database: 'bug.db') ActiveRecord::Schema.define do create_table :parents, force: true do |t| t.timestamps end create_table :nodes, force: true do |t| t.references :parent, foreign_key: false t.timestamps end end class Parent < ActiveRecord::Base has_many :nodes, inverse_of: :parent end class Node < ActiveRecord::Base belongs_to :parent, inverse_of: :nodes end require 'minitest/autorun' class BugTest < Minitest::Test def test_with_has_many_inversing # IT SUCCEEDS parent = Parent.new parent.save Node.new(parent: parent) assert_equal( 1, parent.nodes.size ) end def test_without_has_many_inversing # IT FAILS ActiveRecord::Base.has_many_inversing = false parent = Parent.new parent.save Node.new(parent: parent) assert_equal( 1, parent.nodes.size ) ActiveRecord::Base.has_many_inversing = true end end ``` Other than that, other minor modifications done in the test suite include: - A few`#reload` calls are needed to refresh the new and improved cache. - Some parent records need to be persisted before creating one of their children. We haven't investigated this point further. It could be due to some undocumented change of behavior in Rails or another minor bug.

waiting-for-dev · 2021-04-30T09:36:38Z

OK! This was tough, but everything is 🟢 now 🙂

When reviewing this PR, I strongly recommend going commit by commit, and reading the commit message first, as I guess it can be hard to follow how the different changes are related otherwise.

kennyadsl

Stellar work @waiting-for-dev, thanks. Just one note: can we check that the other config used later in core/lib/spree/testing_support/dummy_app.rb are not overriding the defaults of some old versions of Rails? Maybe we can take advantage of the change and cleanup a little bit those configs?

aldesantis

Outstanding debugging work on this one @waiting-for-dev, thanks so much! 🙇

As for the dummy app config cleanup, I'd do that in a separate PR if that works with you @kennyadsl!

spaghetticode · 2021-05-05T15:19:27Z

core/app/models/spree/variant.rb

@@ -30,7 +30,7 @@ class Variant < Spree::Base
    attr_writer :rebuild_vat_prices
    include Spree::DefaultPrice

-    belongs_to :product, -> { with_discarded }, touch: true, class_name: 'Spree::Product', inverse_of: :variants, optional: false
+    belongs_to :product, -> { with_discarded }, touch: true, class_name: 'Spree::Product', inverse_of: :variants_including_master, optional: false


It seems to me that this change (and the one in product.rb) are not really related to the issue with ActiveRecord.has_many_inverse, but they've been an issue for a while.

What about moving these changes into their own commit? I think it would give the fix more prominence and make the last commit a bit lighter.

Ops, I merged too fast. Sorry, @spaghetticode! 🤦‍♂️

I would have agreed with you for what it's worth. Maybe something we can take into account in the next PRs. Sorry again.

Fixing that association solved two test failures:

solidus/api/spec/requests/spree/api/products_controller_spec.rb

Line 251 in 69b9a0f

it "creates with embedded variants" do

1) Spree::Api::ProductsController as an admin creating a product creates with embedded variants Failure/Error: expect(variants.count).to eq(2) expected: 2 got: 3 (compared using ==)

solidus/core/spec/lib/spree/core/testing_support/factories/variant_factory_spec.rb

Line 25 in 69b9a0f

it "builds a master variant properly" do

1) variant factory master variant builds a master variant properly Failure/Error: expect(product.variants).to be_empty expected `#<ActiveRecord::Associations::CollectionProxy []>.empty?` to be truthy, got false

I didn't debug further, though, as I saw that the change made sense.

Here it's a description of the reason for each change. Keep in mind that CI test against Rails 5.2 as the minimal version: - serve_static_assets: Removed it. It's [not available since Rails 5.0](https://guides.rubyonrails.org/5_0_release_notes.html#railties-removals). - public_file_server.enabled: Set explicitly to `true`. Even if it was already `true`, a [real Rails application will default it to `false` in the production environment unless a env variable `RAILS_SERVE_STATIC_FILES` is present](https://github.com/rails/rails/blob/f95c0b7e96eb36bc3efc0c5beffbb9e84ea664e4/railties/lib/rails/generators/rails/app/templates/config/environments/production.rb.tt#L27). This way it's less surprising. - whiny_nils: Removed. It's [not available since Rails 4.1](https://guides.rubyonrails.org/4_1_release_notes.html#railties-removals). - action_controller.allow_forgery_protection: Set to true to mimic production. - action_controller.default_protect_from_forgery: Set to true to mimic production. - action_controller.perform_caching: Set to true to mimic production. - active_support.deprecation: Remove duplicated entry. - action_mailer.delivery_job: Removed because `load_defaults` take care of it. - storage_path: Removed. It wasn't a Rails setting, but used as a convenience. It was confusing. - action_controller.include_all_helpers: Removed as it's already the default. - log_level: Added. It already defaulted to `:debug`, but a real Rails app will default to `:info` in production. To make things less surprising let's be explicit. - action_mailer.perform_caching: Added to mimic production. - i18n.fallbacks: Added to mimic production. - active_record.dump_schema_after_migration: Added to mimic production and for better performance. See solidusio#4035

Here it's a description of the reason for each change. Keep in mind that CI test against Rails 5.2 as the minimal version: - serve_static_assets: Removed it. It's [not available since Rails 5.0](https://guides.rubyonrails.org/5_0_release_notes.html#railties-removals). - public_file_server.enabled: Set explicitly to `true`. Even if it was already `true`, a [real Rails application will default it to `false` in the production environment unless a env variable `RAILS_SERVE_STATIC_FILES` is present](https://github.com/rails/rails/blob/f95c0b7e96eb36bc3efc0c5beffbb9e84ea664e4/railties/lib/rails/generators/rails/app/templates/config/environments/production.rb.tt#L27). This way it's less surprising. - whiny_nils: Removed. It's [not available since Rails 4.1](https://guides.rubyonrails.org/4_1_release_notes.html#railties-removals). - action_controller.allow_forgery_protection: Set to true to mimic production. - action_controller.default_protect_from_forgery: Set to true to mimic production. - action_controller.perform_caching: Set to true to mimic production. - active_support.deprecation: Remove duplicated entry. - action_mailer.delivery_job: Removed because `load_defaults` take care of it. - storage_path: Removed. It wasn't a Rails setting, but used as a convenience. It was confusing. - action_controller.include_all_helpers: Removed as it's already the default. - log_level: Added. It already defaulted to `:debug`, but a real Rails app will default to `:info` in production. To make things less surprising let's be explicit. - action_mailer.perform_caching: Added to mimic production. - i18n.fallbacks: Added to mimic production. - active_record.dump_schema_after_migration: Added to mimic production and for better performance. - assets: No need to check whether it's a method on `config`. See solidusio#4035

Here it's a description of the reason for each change. Keep in mind that CI test against Rails 5.2 as the minimal version: - serve_static_assets: Removed it. It's [not available since Rails 5.0](https://guides.rubyonrails.org/5_0_release_notes.html#railties-removals). - public_file_server.enabled: Set explicitly to `true`. Even if it was already `true`, a [real Rails application will default it to `false` in the production environment unless a env variable `RAILS_SERVE_STATIC_FILES` is present](https://github.com/rails/rails/blob/f95c0b7e96eb36bc3efc0c5beffbb9e84ea664e4/railties/lib/rails/generators/rails/app/templates/config/environments/production.rb.tt#L27). This way it's less surprising. - whiny_nils: Removed. It's [not available since Rails 4.1](https://guides.rubyonrails.org/4_1_release_notes.html#railties-removals). - action_controller.allow_forgery_protection: Set to true to mimic production. - action_controller.default_protect_from_forgery: Set to true to mimic production. - action_controller.perform_caching: Set to true to mimic production. - active_support.deprecation: Remove duplicated entry. - action_mailer.delivery_job: Removed because `load_defaults` take care of it. - storage_path: Removed. It wasn't a Rails setting, but used as a convenience. It was confusing. - action_controller.include_all_helpers: Removed as it's already the default. - log_level: Added. It already defaulted to `:debug`, but a real Rails app will default to `:info` in production. To make things less surprising let's be explicit. - action_mailer.perform_caching: Added to mimic production. - i18n.fallbacks: Added to mimic production. - active_record.dump_schema_after_migration: Added to mimic production and for better performance. - assets: There's no need to check whether it's a method on `config`. See solidusio#4035

Due to the recent Rails 6.1 update, some tests have failed due to a default `has_many_inversing` feature introduce in that major update. Solidus versions < 3.1 will have to implement some work arounds to successfully pass certain tests. This commit fixes the specific tests that are affected by the patched feature. For more information please see PR #[4035](solidusio/solidus#4035) and this [commit](solidusio/solidus@55bd0e4) on [Solidus](https://github.com/solidusio/solidus).

Due to the changes in solidusio#4035 we had a case in `Spree::Stock::Estimator` where we had to reset side effects from calculating proposed shipping rates with a line like `order.shipments = order.shipments - shipment`, this commit removes the need to reverse side effects by creating a deep copy of the passed in order and using the copied version to do the calculations instead so we don't mutate the original order in any unintended way.

Here it's a description of the reason for each change. Keep in mind that CI test against Rails 5.2 as the minimal version: - serve_static_assets: Removed it. It's [not available since Rails 5.0](https://guides.rubyonrails.org/5_0_release_notes.html#railties-removals). - public_file_server.enabled: Set explicitly to `true`. Even if it was already `true`, a [real Rails application will default it to `false` in the production environment unless a env variable `RAILS_SERVE_STATIC_FILES` is present](https://github.com/rails/rails/blob/f95c0b7e96eb36bc3efc0c5beffbb9e84ea664e4/railties/lib/rails/generators/rails/app/templates/config/environments/production.rb.tt#L27). This way it's less surprising. - whiny_nils: Removed. It's [not available since Rails 4.1](https://guides.rubyonrails.org/4_1_release_notes.html#railties-removals). - action_controller.allow_forgery_protection: Set to true to mimic production. - action_controller.default_protect_from_forgery: Set to true to mimic production. - action_controller.perform_caching: Set to true to mimic production. - active_support.deprecation: Remove duplicated entry. - action_mailer.delivery_job: Removed because `load_defaults` take care of it. - storage_path: Removed. It wasn't a Rails setting, but used as a convenience. It was confusing. - action_controller.include_all_helpers: Removed as it's already the default. - log_level: Added. It already defaulted to `:debug`, but a real Rails app will default to `:info` in production. To make things less surprising let's be explicit. - action_mailer.perform_caching: Added to mimic production. - i18n.fallbacks: Added to mimic production. - active_record.dump_schema_after_migration: Added to mimic production and for better performance. - assets: There's no need to check whether it's a method on `config`. See solidusio#4035

houndci-bot reviewed Apr 22, 2021

View reviewed changes

core/spec/lib/spree/core/testing_support/dummy_app_spec.rb Outdated Show resolved Hide resolved

waiting-for-dev force-pushed the waiting-for-dev/load_defaults branch from f746923 to 31711c2 Compare April 22, 2021 06:11

waiting-for-dev added 3 commits April 30, 2021 07:20

waiting-for-dev force-pushed the waiting-for-dev/load_defaults branch from 846b611 to 56f0c0d Compare April 30, 2021 05:20

houndci-bot reviewed Apr 30, 2021

View reviewed changes

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

waiting-for-dev force-pushed the waiting-for-dev/load_defaults branch from 56f0c0d to 55bd0e4 Compare April 30, 2021 09:31

kennyadsl reviewed May 3, 2021

View reviewed changes

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

Simplify fetching the order to calculate rates for proposed shipments #4046

Closed

jarednorman approved these changes May 5, 2021

View reviewed changes

aldesantis approved these changes May 5, 2021

View reviewed changes

spaghetticode reviewed May 5, 2021

View reviewed changes

kennyadsl merged commit 69b9a0f into solidusio:master May 5, 2021

kennyadsl deleted the waiting-for-dev/load_defaults branch May 5, 2021 15:19

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

Update defaults in dummy application #4047

Merged

5 tasks

kennyadsl added the changelog:solidus_core Changes to the solidus_core gem label May 28, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Load defaults for the latest Rails minor version in the dummy app #4035

Load defaults for the latest Rails minor version in the dummy app #4035

waiting-for-dev commented Apr 22, 2021

waiting-for-dev commented Apr 22, 2021 •

edited

Loading

kennyadsl commented Apr 22, 2021

waiting-for-dev commented Apr 30, 2021

kennyadsl left a comment

aldesantis left a comment •

edited

Loading

spaghetticode May 5, 2021

kennyadsl May 5, 2021

waiting-for-dev May 5, 2021

Load defaults for the latest Rails minor version in the dummy app #4035

Load defaults for the latest Rails minor version in the dummy app #4035

Conversation

waiting-for-dev commented Apr 22, 2021

waiting-for-dev commented Apr 22, 2021 • edited Loading

kennyadsl commented Apr 22, 2021

waiting-for-dev commented Apr 30, 2021

kennyadsl left a comment

Choose a reason for hiding this comment

aldesantis left a comment • edited Loading

Choose a reason for hiding this comment

spaghetticode May 5, 2021

Choose a reason for hiding this comment

kennyadsl May 5, 2021

Choose a reason for hiding this comment

waiting-for-dev May 5, 2021

Choose a reason for hiding this comment

waiting-for-dev commented Apr 22, 2021 •

edited

Loading

aldesantis left a comment •

edited

Loading