From b7ab40c5e2eefc3f9d3ed4c2a7be1d823d6091e1 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Marc=20Busqu=C3=A9?= <marc@lamarciana.com>
Date: Tue, 20 Apr 2021 16:00:01 +0200
Subject: [PATCH 1/4] Load defaults for the latest Rails minor version in the
 dummy app

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
---
 core/lib/spree/testing_support/dummy_app.rb           |  6 ++++++
 .../lib/spree/core/testing_support/dummy_app_spec.rb  | 11 +++++++++++
 2 files changed, 17 insertions(+)
 create mode 100644 core/spec/lib/spree/core/testing_support/dummy_app_spec.rb

diff --git a/core/lib/spree/testing_support/dummy_app.rb b/core/lib/spree/testing_support/dummy_app.rb
index b96ce9652c6..8785799a9dd 100644
--- a/core/lib/spree/testing_support/dummy_app.rb
+++ b/core/lib/spree/testing_support/dummy_app.rb
@@ -46,6 +46,12 @@ def self.setup(gem_root:, lib_name:, auto_migrate: true)
   end
 
   class Application < ::Rails::Application
+    config.load_defaults("#{Rails::VERSION::MAJOR}.#{Rails::VERSION::MINOR}")
+    # TODO: Remove the three following settings once the codebase has been adapted
+    # to load_defaults
+    config.active_record.has_many_inversing = nil
+    config.active_storage.track_variants = nil
+    config.active_record.belongs_to_required_by_default = false
     config.eager_load = false
     config.cache_classes = true
     config.cache_store = :memory_store
diff --git a/core/spec/lib/spree/core/testing_support/dummy_app_spec.rb b/core/spec/lib/spree/core/testing_support/dummy_app_spec.rb
new file mode 100644
index 00000000000..953947ffa5e
--- /dev/null
+++ b/core/spec/lib/spree/core/testing_support/dummy_app_spec.rb
@@ -0,0 +1,11 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+RSpec.describe DummyApp do
+  it 'loads default from the Rails version in use' do
+    expect(
+      DummyApp::Application.config.loaded_config_version
+    ).to eq("#{Rails::VERSION::MAJOR}.#{Rails::VERSION::MINOR}")
+  end
+end

From 58eac9720e70187b89b6dfa56267c8f5c41200d5 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Marc=20Busqu=C3=A9?= <marc@lamarciana.com>
Date: Thu, 22 Apr 2021 10:29:10 +0200
Subject: [PATCH 2/4] Default `belongs_to_required_by_default` in the dummy app

`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.
---
 core/lib/spree/testing_support/dummy_app.rb | 3 +--
 frontend/spec/features/taxons_spec.rb       | 6 +++---
 2 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/core/lib/spree/testing_support/dummy_app.rb b/core/lib/spree/testing_support/dummy_app.rb
index 8785799a9dd..924047148d9 100644
--- a/core/lib/spree/testing_support/dummy_app.rb
+++ b/core/lib/spree/testing_support/dummy_app.rb
@@ -47,11 +47,10 @@ def self.setup(gem_root:, lib_name:, auto_migrate: true)
 
   class Application < ::Rails::Application
     config.load_defaults("#{Rails::VERSION::MAJOR}.#{Rails::VERSION::MINOR}")
-    # TODO: Remove the three following settings once the codebase has been adapted
+    # TODO: Remove the two following settings once the codebase has been adapted
     # to load_defaults
     config.active_record.has_many_inversing = nil
     config.active_storage.track_variants = nil
-    config.active_record.belongs_to_required_by_default = false
     config.eager_load = false
     config.cache_classes = true
     config.cache_store = :memory_store
diff --git a/frontend/spec/features/taxons_spec.rb b/frontend/spec/features/taxons_spec.rb
index 53721ddf727..cd01d20db38 100644
--- a/frontend/spec/features/taxons_spec.rb
+++ b/frontend/spec/features/taxons_spec.rb
@@ -4,9 +4,9 @@
 
 describe "viewing products", type: :feature, inaccessible: true do
   let!(:taxonomy) { create(:taxonomy, name: "Category") }
-  let!(:super_clothing) { taxonomy.root.children.create(name: "Super Clothing") }
-  let!(:t_shirts) { super_clothing.children.create(name: "T-Shirts") }
-  let!(:xxl) { t_shirts.children.create(name: "XXL") }
+  let!(:super_clothing) { taxonomy.root.children.create!(name: "Super Clothing", taxonomy: taxonomy) }
+  let!(:t_shirts) { super_clothing.children.create!(name: "T-Shirts", taxonomy: taxonomy) }
+  let!(:xxl) { t_shirts.children.create!(name: "XXL", taxonomy: taxonomy) }
   let!(:product) do
     product = create(:product, name: "Superman T-Shirt")
     product.taxons << t_shirts

From f37b12a31cfaaf9404f3d27f2079a0f17a5cc243 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Marc=20Busqu=C3=A9?= <marc@lamarciana.com>
Date: Thu, 22 Apr 2021 16:16:48 +0200
Subject: [PATCH 3/4] Fix invalidating attachment's blob in test

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
---
 .../spec/features/admin/products/edit/images_spec.rb   | 10 +++++++++-
 core/lib/spree/testing_support/dummy_app.rb            |  4 +---
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/backend/spec/features/admin/products/edit/images_spec.rb b/backend/spec/features/admin/products/edit/images_spec.rb
index 1ff20b4ab19..d885bacb37b 100644
--- a/backend/spec/features/admin/products/edit/images_spec.rb
+++ b/backend/spec/features/admin/products/edit/images_spec.rb
@@ -78,7 +78,7 @@
         end
 
         click_button "Update"
-        Spree::Image.first.attachment.blob.update(key: 11)
+        invalidate_attachment(Spree::Image.first.attachment)
         visit current_path
         expect(page).to have_xpath("//img[contains(@src, 'assets/noimage/mini')]")
       end
@@ -136,4 +136,12 @@
       expect(page).to have_css("thead th", count: 4)
     end
   end
+
+  def invalidate_attachment(attachment)
+    blob = attachment.blob
+    blob.variant_records.each do |variant_record|
+      variant_record.update(variation_digest: SecureRandom.uuid)
+    end
+    blob.update(key: SecureRandom.uuid)
+  end
 end
diff --git a/core/lib/spree/testing_support/dummy_app.rb b/core/lib/spree/testing_support/dummy_app.rb
index 924047148d9..3cf802cafef 100644
--- a/core/lib/spree/testing_support/dummy_app.rb
+++ b/core/lib/spree/testing_support/dummy_app.rb
@@ -47,10 +47,8 @@ def self.setup(gem_root:, lib_name:, auto_migrate: true)
 
   class Application < ::Rails::Application
     config.load_defaults("#{Rails::VERSION::MAJOR}.#{Rails::VERSION::MINOR}")
-    # TODO: Remove the two following settings once the codebase has been adapted
-    # to load_defaults
+    # TODO: Remove once the codebase has been adapted to the new default
     config.active_record.has_many_inversing = nil
-    config.active_storage.track_variants = nil
     config.eager_load = false
     config.cache_classes = true
     config.cache_store = :memory_store

From 55bd0e41ed4e34799566655d1b8843d53184ff84 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Marc=20Busqu=C3=A9?= <marc@lamarciana.com>
Date: Fri, 30 Apr 2021 07:17:17 +0200
Subject: [PATCH 4/4] Fix app and tests to work with
 `ActiveRecord.has_many_inverse`

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 https://github.com/rails/rails/pull/34533 &
https://github.com/rails/rails/pull/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:

- https://github.com/rails/rails/issues/42102 forces us to use
  `Spree::Order#shipments#push` instead of `Spree::Order#shipments#=` when
  the proposed shipments get created from the order.
- https://github.com/rails/rails/issues/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.
---
 core/app/models/spree/order.rb                    | 5 ++++-
 core/app/models/spree/product.rb                  | 1 -
 core/app/models/spree/stock/simple_coordinator.rb | 7 ++++++-
 core/app/models/spree/variant.rb                  | 2 +-
 core/lib/spree/testing_support/dummy_app.rb       | 2 --
 core/spec/lib/search/variant_spec.rb              | 1 +
 core/spec/models/spree/adjustment_spec.rb         | 6 +++---
 core/spec/models/spree/order_contents_spec.rb     | 2 +-
 core/spec/models/spree/payment_spec.rb            | 2 +-
 core/spec/models/spree/product_spec.rb            | 2 +-
 core/spec/models/spree/variant_spec.rb            | 2 +-
 11 files changed, 19 insertions(+), 13 deletions(-)

diff --git a/core/app/models/spree/order.rb b/core/app/models/spree/order.rb
index 4443f97f9e8..72f7bc57d80 100644
--- a/core/app/models/spree/order.rb
+++ b/core/app/models/spree/order.rb
@@ -490,7 +490,10 @@ def create_proposed_shipments
         raise CannotRebuildShipments.new(I18n.t('spree.cannot_rebuild_shipments_shipments_not_pending'))
       else
         shipments.destroy_all
-        self.shipments = Spree::Config.stock.coordinator_class.new(self).shipments
+        # TODO: We can use `self.shipments#=` instead of `#push` when
+        # https://github.com/rails/rails/issues/42102 is fixed and we deprecate
+        # Rails versions with the bug
+        shipments.push(*Spree::Config.stock.coordinator_class.new(self).shipments)
       end
     end
 
diff --git a/core/app/models/spree/product.rb b/core/app/models/spree/product.rb
index 6928b3f4af7..3a99482bced 100644
--- a/core/app/models/spree/product.rb
+++ b/core/app/models/spree/product.rb
@@ -45,7 +45,6 @@ class Product < Spree::Base
 
     has_many :variants,
       -> { where(is_master: false).order(:position) },
-      inverse_of: :product,
       class_name: 'Spree::Variant'
 
     has_many :variants_including_master,
diff --git a/core/app/models/spree/stock/simple_coordinator.rb b/core/app/models/spree/stock/simple_coordinator.rb
index 6b0fe0d5786..e7fcce67236 100644
--- a/core/app/models/spree/stock/simple_coordinator.rb
+++ b/core/app/models/spree/stock/simple_coordinator.rb
@@ -73,11 +73,16 @@ def build_shipments
         packages = split_packages(packages)
 
         # Turn the Stock::Packages into a Shipment with rates
-        packages.map do |package|
+        shipments = packages.map do |package|
           shipment = package.shipment = package.to_shipment
           shipment.shipping_rates = Spree::Config.stock.estimator_class.new.shipping_rates(package)
           shipment
         end
+
+        # Make sure we don't add the proposed shipments to the order
+        order.shipments = order.shipments - shipments
+
+        shipments
       end
 
       def split_packages(initial_packages)
diff --git a/core/app/models/spree/variant.rb b/core/app/models/spree/variant.rb
index 591239bc914..a51d408e4ef 100644
--- a/core/app/models/spree/variant.rb
+++ b/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
     belongs_to :tax_category, class_name: 'Spree::TaxCategory', optional: true
 
     delegate :name, :description, :slug, :available_on, :discontinue_on, :discontinued?,
diff --git a/core/lib/spree/testing_support/dummy_app.rb b/core/lib/spree/testing_support/dummy_app.rb
index 3cf802cafef..104139fe21e 100644
--- a/core/lib/spree/testing_support/dummy_app.rb
+++ b/core/lib/spree/testing_support/dummy_app.rb
@@ -47,8 +47,6 @@ def self.setup(gem_root:, lib_name:, auto_migrate: true)
 
   class Application < ::Rails::Application
     config.load_defaults("#{Rails::VERSION::MAJOR}.#{Rails::VERSION::MINOR}")
-    # TODO: Remove once the codebase has been adapted to the new default
-    config.active_record.has_many_inversing = nil
     config.eager_load = false
     config.cache_classes = true
     config.cache_store = :memory_store
diff --git a/core/spec/lib/search/variant_spec.rb b/core/spec/lib/search/variant_spec.rb
index 5c778417bfb..8d210e1d81c 100644
--- a/core/spec/lib/search/variant_spec.rb
+++ b/core/spec/lib/search/variant_spec.rb
@@ -59,6 +59,7 @@ def refute_found(query_string, variant)
           described_class.new(variant.sku, scope: Spree::Variant.in_stock).results
         ).to include variant
 
+        variant.stock_items.reload # See https://github.com/rails/rails/issues/42094
         variant.stock_items.each { |si| si.set_count_on_hand(0) }
         expect(
           described_class.new(variant.sku, scope: Spree::Variant.in_stock).results
diff --git a/core/spec/models/spree/adjustment_spec.rb b/core/spec/models/spree/adjustment_spec.rb
index 5720d053f38..b50af5ac879 100644
--- a/core/spec/models/spree/adjustment_spec.rb
+++ b/core/spec/models/spree/adjustment_spec.rb
@@ -4,7 +4,7 @@
 
 RSpec.describe Spree::Adjustment, type: :model do
   let!(:store) { create :store }
-  let(:order) { Spree::Order.new }
+  let(:order) { create :order }
   let(:line_item) { create :line_item, order: order }
 
   let(:adjustment) { Spree::Adjustment.create!(label: 'Adjustment', adjustable: order, order: order, amount: 5) }
@@ -43,7 +43,7 @@
   end
 
   context '#currency' do
-    let(:order) { Spree::Order.new currency: 'JPY' }
+    let(:order) { create :order, currency: 'JPY' }
 
     it 'returns the adjustables currency' do
       expect(adjustment.currency).to eq 'JPY'
@@ -67,7 +67,7 @@
     end
 
     context "with currency set to JPY" do
-      let(:order) { Spree::Order.new currency: 'JPY' }
+      let(:order) { create :order, currency: 'JPY' }
 
       context "when adjustable is set to an order" do
         it "displays in JPY" do
diff --git a/core/spec/models/spree/order_contents_spec.rb b/core/spec/models/spree/order_contents_spec.rb
index 9bfa2e8b744..dc18f2030e0 100644
--- a/core/spec/models/spree/order_contents_spec.rb
+++ b/core/spec/models/spree/order_contents_spec.rb
@@ -25,7 +25,7 @@
 
       it "ensure shipment calls update_amounts instead of order calling ensure_updated_shipments" do
         expect(subject.order).to_not receive(:ensure_updated_shipments)
-        expect(shipment).to receive(:update_amounts)
+        expect(shipment).to receive(:update_amounts).at_least(:once)
         subject.add(variant, 1, shipment: shipment)
       end
 
diff --git a/core/spec/models/spree/payment_spec.rb b/core/spec/models/spree/payment_spec.rb
index 2a5a2125425..e38a7a832ff 100644
--- a/core/spec/models/spree/payment_spec.rb
+++ b/core/spec/models/spree/payment_spec.rb
@@ -8,7 +8,7 @@
   let(:refund_reason) { create(:refund_reason) }
 
   let(:gateway) do
-    gateway = Spree::PaymentMethod::BogusCreditCard.new(active: true, name: 'Bogus gateway')
+    gateway = Spree::PaymentMethod::BogusCreditCard.create!(active: true, name: 'Bogus gateway')
     allow(gateway).to receive_messages(source_required?: true)
     gateway
   end
diff --git a/core/spec/models/spree/product_spec.rb b/core/spec/models/spree/product_spec.rb
index 6b66a753b4b..cac6c32b703 100644
--- a/core/spec/models/spree/product_spec.rb
+++ b/core/spec/models/spree/product_spec.rb
@@ -129,7 +129,7 @@ class Extension < Spree::Base
         it "should set deleted_at value" do
           product.discard
           expect(product.deleted_at).not_to be_nil
-          expect(product.variants_including_master).to all(be_discarded)
+          expect(product.variants_including_master.reload).to all(be_discarded)
         end
       end
     end
diff --git a/core/spec/models/spree/variant_spec.rb b/core/spec/models/spree/variant_spec.rb
index 2e1e5a39a12..55ed9e45e89 100644
--- a/core/spec/models/spree/variant_spec.rb
+++ b/core/spec/models/spree/variant_spec.rb
@@ -616,7 +616,7 @@
       variant.discard
 
       expect(variant.images).to be_empty
-      expect(variant.stock_items).to be_empty
+      expect(variant.stock_items.reload).to be_empty
       expect(variant.prices).to be_empty
       expect(variant.currently_valid_prices).to be_empty
     end