Merge pull request #6305 from mamhoff/country-dependent

tvdeyen · web-flow · commit 907bf8af3047 · 2025-08-05T14:02:07.000+02:00
Country and state: Restrict deletion if addresses present
diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml
diff --git a/admin/spec/features/zones_spec.rb b/admin/spec/features/zones_spec.rb
@@ -4,17 +4,20 @@
 
 describe "Zones", :js, type: :feature do
   before { sign_in create(:admin_user, email: 'admin@example.com') }
+  let(:canada) { create(:country, iso: "CA") }
+  let(:france) { create(:country, iso: "FR") }
+  let(:usa) { create(:country) }
 
   let(:states) do
     [
-      create(:state, name: "Alberta", country: create(:country, iso: "CA")),
-      create(:state, name: "Manitoba", country: create(:country, iso: "CA"))
+      create(:state, name: "Alberta", country: canada),
+      create(:state, name: "Manitoba", country: canada)
     ]
   end
 
   it "lists zones and allows deleting them" do
-    create(:zone, name: "Europe")
-    create(:zone, name: "North America")
+    create(:zone, name: "Europe", countries: [france])
+    create(:zone, name: "North America", countries: [usa, canada])
 
     visit "/admin/zones"
     expect(page).to have_content("Europe")
diff --git a/admin/spec/requests/solidus_admin/zones_spec.rb b/admin/spec/requests/solidus_admin/zones_spec.rb
@@ -5,7 +5,9 @@
 
 RSpec.describe "SolidusAdmin::ZonesController", type: :request do
   include_examples "CRUD resource requests", "zone" do
-    let(:countries) { create_list(:country, 2) }
+    let(:usa) { create(:country) }
+    let(:canada) { create(:country, iso: "CA") }
+    let(:countries) { [usa, canada] }
     let(:resource_class) { Spree::Zone }
     let(:valid_attributes) { { name: "Zone with countries", country_ids: countries.map(&:id) } }
     let(:invalid_attributes) { { name: "" } }
@@ -15,16 +17,21 @@
     end
 
     it "updates zone members" do
-      zone = create(:zone, :with_country)
+      brazil = create(:country, iso: "BR")
+      zone = create(:zone, countries: [brazil])
       expect { patch solidus_admin.zone_path(zone), params: { zone: valid_attributes } }.to change(Spree::ZoneMember, :count).by(1)
     end
   end
 
   context "N+1" do
-    before do
-      create_list(:zone, 2, :with_country)
-      create_list(:zone, 2, :with_state)
-    end
+    let(:usa) { create(:country) }
+    let(:canada) { create(:country, iso: "CA") }
+    let(:new_york) { create(:state, state_code: "NY", country: usa) }
+    let(:north_carolina) { create(:state, state_code: "NC", country: usa) }
+    let!(:usa_zone) { create(:zone, countries: [usa]) }
+    let!(:canada_zone) { create(:zone, countries: [canada]) }
+    let!(:new_york_zone) { create(:zone, states: [new_york]) }
+    let!(:north_carolina_zone) { create(:zone, states: [north_carolina]) }
 
     let(:expected_count) do
       [
diff --git a/api/spec/requests/spree/api/countries_spec.rb b/api/spec/requests/spree/api/countries_spec.rb
@@ -16,7 +16,7 @@ module Spree::Api
     end
 
     context "with two countries" do
-      before { @zambia = create(:country, name: "Zambia") }
+      before { @zambia = create(:country, iso: "ZA", name: "Zambia") }
 
       it "can view all countries" do
         get spree.api_countries_path
diff --git a/api/spec/requests/spree/api/states_spec.rb b/api/spec/requests/spree/api/states_spec.rb
@@ -4,7 +4,8 @@
 
 module Spree::Api
   describe 'States', type: :request do
-    let!(:state) { create(:state, name: "Victoria") }
+    let(:australia) { create(:country, iso: "AU") }
+    let!(:state) { create(:state, country: australia, name: "Victoria") }
     let(:attributes) { [:id, :name, :abbr, :country_id] }
 
     before do
@@ -37,7 +38,7 @@ module Spree::Api
     end
 
     context "with two states" do
-      before { create(:state, name: "New South Wales") }
+      before { create(:state, country: australia, name: "New South Wales") }
 
       it "gets all states for a country" do
         country = create(:country, states_required: true)
diff --git a/backend/spec/features/admin/orders/customer_details_spec.rb b/backend/spec/features/admin/orders/customer_details_spec.rb
@@ -147,20 +147,6 @@
       end
     end
 
-    context "country associated was removed" do
-      let(:brazil) { create(:country, iso: "BR", name: "Brazil") }
-
-      before do
-        order.bill_address.country.destroy
-        stub_spree_preferences(default_country_iso: brazil.iso)
-      end
-
-      it "sets default country when displaying form" do
-        click_link "Customer"
-        expect(page).to have_field("order_bill_address_attributes_country_id", with: brazil.id, visible: false)
-      end
-    end
-
     # Regression test for https://github.com/spree/spree/issues/942
     context "errors when no shipping methods are available" do
       before do
diff --git a/core/app/models/spree/address.rb b/core/app/models/spree/address.rb
@@ -11,10 +11,10 @@ class Address < Spree::Base
     mattr_accessor :state_validator_class
     self.state_validator_class = Spree::Address::StateValidator
 
-    belongs_to :country, class_name: "Spree::Country", optional: true
+    belongs_to :country, class_name: "Spree::Country"
     belongs_to :state, class_name: "Spree::State", optional: true
 
-    validates :address1, :city, :country_id, :name, presence: true
+    validates :address1, :city, :name, presence: true
     validates :zipcode, presence: true, if: :require_zipcode?
     validates :phone, presence: true, if: :require_phone?
 
diff --git a/core/app/models/spree/country.rb b/core/app/models/spree/country.rb
@@ -2,11 +2,22 @@
 
 module Spree
   class Country < Spree::Base
-    has_many :states, -> { order(:name) }, dependent: :destroy
-    has_many :addresses, dependent: :nullify
-    has_many :prices, class_name: "Spree::Price", foreign_key: "country_iso", primary_key: "iso"
-
-    validates :name, :iso_name, presence: true
+    has_many :states,
+      -> { order(:name) },
+      dependent: :destroy,
+      inverse_of: :country
+    has_many :addresses,
+      dependent: :restrict_with_error,
+      inverse_of: :country
+    has_many :prices,
+      class_name: "Spree::Price",
+      foreign_key: "country_iso",
+      primary_key: "iso",
+      dependent: :restrict_with_error,
+      inverse_of: :country
+
+    validates :name, :iso_name, :iso, presence: true
+    validates :iso, uniqueness: true
 
     self.allowed_ransackable_attributes = %w[name]
 
diff --git a/core/app/models/spree/state.rb b/core/app/models/spree/state.rb
@@ -2,10 +2,10 @@
 
 module Spree
   class State < Spree::Base
-    belongs_to :country, class_name: 'Spree::Country', optional: true
-    has_many :addresses, dependent: :nullify
+    belongs_to :country, class_name: 'Spree::Country'
+    has_many :addresses, dependent: :nullify, inverse_of: :state
 
-    validates :country, :name, presence: true
+    validates :name, presence: true
 
     scope :with_name_or_abbr, ->(name_or_abbr) do
       where(
diff --git a/core/db/migrate/20250628094037_change_countries_iso_to_unique.rb b/core/db/migrate/20250628094037_change_countries_iso_to_unique.rb
@@ -0,0 +1,9 @@
+# frozen_string_literal: true
+
+class ChangeCountriesIsoToUnique < ActiveRecord::Migration[7.0]
+  def change
+    remove_index :spree_countries, :iso
+
+    add_index :spree_countries, :iso, unique: true
+  end
+end
diff --git a/core/db/migrate/20250709073151_add_country_foreign_keys.rb b/core/db/migrate/20250709073151_add_country_foreign_keys.rb
@@ -0,0 +1,75 @@
+# frozen_string_literal: true
+
+class AddCountryForeignKeys < ActiveRecord::Migration[7.0]
+  FOREIGN_KEY_VIOLATION_ERRORS = %w[PG::ForeignKeyViolation Mysql2::Error SQLite3::ConstraintException]
+
+  def up
+    # Uncomment the following code to remove orphaned records if this migration fails
+    #
+    # say_with_time "Removing orphaned states (no corresponding country)" do
+    #   Spree::State.left_joins(:country).where(spree_countries: { id: nil }).delete_all
+    # end
+
+    begin
+      add_foreign_key :spree_states, :spree_countries, column: :country_id, null: false, on_delete: :cascade
+    rescue ActiveRecord::StatementInvalid => e
+      if e.cause.class.name.in?(FOREIGN_KEY_VIOLATION_ERRORS)
+        say <<~MSG
+          ⚠️ Foreign key constraint failed when adding :spree_states => :spree_countries.
+          To fix this:
+            1. Uncomment the code that removes orphaned records.
+            2. Rerun the migration.
+          Offending error: #{e.cause.class} - #{e.cause.message}
+        MSG
+      end
+      raise
+    end
+
+    # Uncomment the following code to remove orphaned records if this migration fails
+    #
+    # say_with_time "Updating orphaned addresses (no corresponding country) to use default country" do
+    #   Spree::Address.left_joins(:country).where(spree_countries: { id: nil }).update_all(country: Spree::Country.default)
+    # end
+
+    begin
+      add_foreign_key :spree_addresses, :spree_countries, column: :country_id, null: false, on_delete: :restrict
+    rescue ActiveRecord::StatementInvalid => e
+      if e.cause.class.name.in?(FOREIGN_KEY_VIOLATION_ERRORS)
+        say <<~MSG
+          ⚠️ Foreign key constraint failed when adding :spree_addresses => :spree_countries.
+          To fix this:
+            1. Uncomment the code that removes orphaned records.
+            2. Rerun the migration.
+          Offending error: #{e.cause.class} - #{e.cause.message}
+        MSG
+      end
+      raise
+    end
+    # Uncomment the following code to remove orphaned records if this migration fails
+    #
+    # say_with_time "Deleting orphaned prices (country ID without corresponding country)" do
+    #   Spree::Price.where.not(country_iso: nil).left_joins(:country).where(spree_countries: { iso: nil }).update_all(country_iso: Spree::Config.default_country_iso)
+    # end
+
+    begin
+      add_foreign_key :spree_prices, :spree_countries, column: :country_iso, primary_key: :iso, null: true, on_delete: :restrict
+    rescue ActiveRecord::StatementInvalid => e
+      if e.cause.class.name.in?(FOREIGN_KEY_VIOLATION_ERRORS)
+        say <<~MSG
+          ⚠️ Foreign key constraint failed when adding :spree_prices => :spree_countries.
+          To fix this:
+            1. Uncomment the code that removes orphaned records.
+            2. Rerun the migration.
+          Offending error: #{e.cause.class} - #{e.cause.message}
+        MSG
+      end
+      raise
+    end
+  end
+
+  def down
+    remove_foreign_key :spree_states, :spree_countries, column: :country_id, null: false, on_delete: :cascade
+    remove_foreign_key :spree_addresses, :spree_countries, column: :country_id, null: false, on_delete: :restrict
+    remove_foreign_key :spree_prices, :spree_countries, column: :country_id, null: true, on_delete: :restrict
+  end
+end
diff --git a/core/db/migrate/20250709084513_add_state_foreign_keys.rb b/core/db/migrate/20250709084513_add_state_foreign_keys.rb
@@ -0,0 +1,30 @@
+# frozen_string_literal: true
+
+class AddStateForeignKeys < ActiveRecord::Migration[7.0]
+  FOREIGN_KEY_VIOLATION_ERRORS = %w[PG::ForeignKeyViolation Mysql2::Error SQLite3::ConstraintException]
+
+  def up
+    # Uncomment the following code to remove orphaned records if this migration fails
+    #
+    # say_with_time "Resetting state IDs on addresses where the state record is no longer present" do
+    #   Spree::Address.where.not(state_id: nil).left_joins(:state).where(spree_states: { id: nil }).update_all(state_id: nil)
+    # end
+
+    add_foreign_key :spree_addresses, :spree_states, column: :state_id, null: true, on_delete: :restrict
+  rescue ActiveRecord::StatementInvalid => e
+    if e.cause.class.name.in?(FOREIGN_KEY_VIOLATION_ERRORS)
+      say <<~MSG
+        ⚠️ Foreign key constraint failed when adding :spree_addresses => :spree_states.
+        To fix this:
+          1. Uncomment the code that removes orphaned records.
+          2. Rerun the migration.
+        Offending error: #{e.cause.class} - #{e.cause.message}
+      MSG
+    end
+    raise
+  end
+
+  def down
+    remove_foreign_key :spree_addresses, :spree_states, column: :state_id, null: true, on_delete: :restrict
+  end
+end
diff --git a/core/spec/helpers/base_helper_spec.rb b/core/spec/helpers/base_helper_spec.rb
@@ -11,7 +11,9 @@
     let(:country) { create(:country) }
 
     before do
-      3.times { create(:country) }
+      create(:country, iso: "BR")
+      create(:country, iso: "DE")
+      create(:country, iso: "FR")
     end
 
     context "with no checkout zone defined" do
@@ -24,7 +26,7 @@
       end
 
       it "uses locales for country names" do
-        expect(available_countries).to include(having_attributes(name: "United States of America"))
+        expect(available_countries).to include(having_attributes(name: "Brazil"))
       end
     end
 
diff --git a/core/spec/lib/spree/core/importer/order_spec.rb b/core/spec/lib/spree/core/importer/order_spec.rb
@@ -226,7 +226,7 @@ module Core
           }
         end
 
-        let(:other_state) { create(:state, name: "Uhuhuh", country: create(:country)) }
+        let(:other_state) { create(:state, name: "Uhuhuh", country: create(:country, iso: "BR")) }
 
         before do
           ship_address.delete(:state_id)
diff --git a/core/spec/models/spree/country_spec.rb b/core/spec/models/spree/country_spec.rb
@@ -104,16 +104,15 @@
   end
 
   describe '#prices' do
-    let(:country) { create(:country) }
+    let(:country) { create(:country, iso: "BR") }
     subject { country.prices }
 
     it { is_expected.to be_a(ActiveRecord::Associations::CollectionProxy) }
 
     context "if the country has associated prices" do
-      let!(:price_one) { create(:price) }
-      let!(:price_two) { create(:price) }
+      let!(:price_one) { create(:price, country:) }
+      let!(:price_two) { create(:price, country:) }
       let!(:price_three) { create(:price) }
-      let(:country) { create(:country, prices: [price_one, price_two]) }
 
       it { is_expected.to contain_exactly(price_one, price_two) }
     end
diff --git a/core/spec/models/spree/credit_card_spec.rb b/core/spec/models/spree/credit_card_spec.rb
@@ -88,7 +88,7 @@ def self.payment_states
     end
 
     let!(:persisted_card) { Spree::CreditCard.find(credit_card.id) }
-    let(:country) { create(:country, states_required: true) }
+    let(:country) { create(:country, iso: "BR", states_required: true) }
     let(:state) { create(:state, country:) }
     let(:valid_address_attributes) do
       {
diff --git a/core/spec/models/spree/price_spec.rb b/core/spec/models/spree/price_spec.rb
@@ -169,7 +169,7 @@
 
   describe 'scopes' do
     describe '.for_any_country' do
-      let(:country) { create(:country) }
+      let(:country) { create(:country, iso: "BR") }
       let!(:fallback_price) { create(:price, country_iso: nil) }
       let!(:country_price) { create(:price, country:) }
 
diff --git a/core/spec/models/spree/tax_rate_spec.rb b/core/spec/models/spree/tax_rate_spec.rb
@@ -64,12 +64,14 @@
     end
 
     context "when no rate zones match the tax zone" do
-      let(:rate_zone) { create(:zone, :with_country) }
+      let(:usa) { create(:country) }
+      let(:rate_zone) { create(:zone, countries: [usa]) }
       let!(:rate) { create :tax_rate, zone: rate_zone }
 
       context "when there is no default tax zone" do
         context "and the zone has no shared members with the rate zone" do
-          let(:zone) { create(:zone, :with_country) }
+          let(:canada) { create(:country, iso: "CA") }
+          let(:zone) { create(:zone, countries: [canada]) }
 
           it "should return an empty array" do
             expect(subject).to eq([])
@@ -94,8 +96,8 @@
         end
 
         context "when the tax_zone is contained within a rate zone" do
-          let(:country1) { create :country }
-          let(:country2) { create :country }
+          let(:country1) { create :country, iso: "FR" }
+          let(:country2) { create :country, iso: "BR" }
           let(:rate_zone) { create(:zone, countries: [country1, country2]) }
           let(:zone) { create(:zone, countries: [country1]) }
 
@@ -127,7 +129,8 @@
         end
 
         context "when the zone is outside the default zone" do
-          let(:zone) { create(:zone, :with_country) }
+          let(:brazil) { create(:country, iso: "BR") }
+          let(:zone) { create(:zone, countries: [brazil]) }
 
           it { is_expected.to be_empty }
         end
diff --git a/core/spec/models/spree/zone_spec.rb b/core/spec/models/spree/zone_spec.rb
