[v3.0] Fix app and tests to work with ActiveRecord.has_many_inverse

[waiting-for-dev]

Backports 55bd0e4

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:

• collection= for a has_many association doesn't persist added records already referencing the parent when has_many_inversing is enabled 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.
• Trying to update a second time a field belonging to a record, when this record has been accessed only the second time from a CollectionProxy with has_many_inversing that has been coerced to an array, and the given value is equal to the field's default, doesn't work 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
  (

  solidus/core/app/models/spree/product.rb

  Line 47 in 2ea8296

  -> { where(is_master: false).order(:position) },

  ).
  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
  (

  solidus/core/app/models/spree/stock/package.rb

  Line 127 in 2ea8296

  order: order,

  ).
  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:

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.