From 286d0e3a3417d91d41dae9ab1a8cf2bc5c431753 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc=20Busqu=C3=A9?= Date: Fri, 12 Mar 2021 06:47:21 +0100 Subject: [PATCH 1/2] Do not allow prices with nil amount This logic will help in an upcoming work that will copy all prices from a master variant over its children variant (thus enforcing that no "empty" prices are created). This overtolerance was introduced in 58ec5cd5. That commit introduced the `Spree::Price` model to allow dealing with multiple currencies. It allowed `amount` to be `nil` for master variants, variants with no master variant, and variants with a master variant with no price. AFAIK, these three scenarios are no considered by our business logic. In fact, the logic was removed 5 days later in 9d563e3. 58ec5cd5 also allowed for `amount` to be `nil` in any other case, but then it copied it from the master variant: https://github.com/solidusio/solidus/commit/9d563e38ee4ac3f6d3f656f204bd1cc84348bb4e#diff-c083f45179b62dc8c47e4fdcbf7ee31da80552100fc0258238817086e46e2925L35 However, this was also done in the controller layer and it's still in place: https://github.com/solidusio/solidus/blob/3cc2d3e492b0aa6c84ac66814db26bea4f17d2e0/backend/app/controllers/spree/admin/variants_controller.rb#L19 --- core/app/models/spree/price.rb | 2 +- ...0312061050_change_column_null_on_prices.rb | 7 ++++++ .../delete_prices_with_nul_amount.rake | 13 ++++++++++ core/lib/tasks/upgrade.rake | 7 ++++++ .../delete_prices_with_nil_amount_spec.rb | 24 +++++++++++++++++++ core/spec/models/spree/price_spec.rb | 2 +- 6 files changed, 53 insertions(+), 2 deletions(-) create mode 100644 core/db/migrate/20210312061050_change_column_null_on_prices.rb create mode 100644 core/lib/tasks/migrations/delete_prices_with_nul_amount.rake create mode 100644 core/spec/lib/tasks/migrations/delete_prices_with_nil_amount_spec.rb diff --git a/core/app/models/spree/price.rb b/core/app/models/spree/price.rb index defe76c780a..59cf304d51f 100644 --- a/core/app/models/spree/price.rb +++ b/core/app/models/spree/price.rb @@ -13,7 +13,7 @@ class Price < Spree::Base delegate :tax_rates, to: :variant validate :check_price - validates :amount, allow_nil: true, numericality: { + validates :amount, numericality: { greater_than_or_equal_to: 0, less_than_or_equal_to: MAXIMUM_AMOUNT } diff --git a/core/db/migrate/20210312061050_change_column_null_on_prices.rb b/core/db/migrate/20210312061050_change_column_null_on_prices.rb new file mode 100644 index 00000000000..19c88ce3a28 --- /dev/null +++ b/core/db/migrate/20210312061050_change_column_null_on_prices.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +class ChangeColumnNullOnPrices < ActiveRecord::Migration[5.2] + def change + change_column_null(:spree_prices, :amount, false) + end +end diff --git a/core/lib/tasks/migrations/delete_prices_with_nul_amount.rake b/core/lib/tasks/migrations/delete_prices_with_nul_amount.rake new file mode 100644 index 00000000000..399fb6a136f --- /dev/null +++ b/core/lib/tasks/migrations/delete_prices_with_nul_amount.rake @@ -0,0 +1,13 @@ +# frozen_string_literal: true + +namespace :solidus do + namespace :migrations do + namespace :delete_prices_with_nul_amount do + task up: :environment do + print "Deleting prices wich amount attribute is nil ... " + Spree::Price.where(amount: nil).delete_all + puts "Success" + end + end + end +end diff --git a/core/lib/tasks/upgrade.rake b/core/lib/tasks/upgrade.rake index d57aba14e43..861232cbd0c 100644 --- a/core/lib/tasks/upgrade.rake +++ b/core/lib/tasks/upgrade.rake @@ -9,5 +9,12 @@ namespace :solidus do ] do puts "Your Solidus install is ready for Solidus 2.11" end + + desc "Upgrade Solidus to version 3.0" + task three_point_zero: [ + 'solidus:migrations:delete_prices_with_nul_amount:up', + ] do + puts "Your Solidus install is ready for Solidus 3.0" + end end end diff --git a/core/spec/lib/tasks/migrations/delete_prices_with_nil_amount_spec.rb b/core/spec/lib/tasks/migrations/delete_prices_with_nil_amount_spec.rb new file mode 100644 index 00000000000..49d8739bcba --- /dev/null +++ b/core/spec/lib/tasks/migrations/delete_prices_with_nil_amount_spec.rb @@ -0,0 +1,24 @@ +# frozen_string_literal: true + +require 'rails_helper' + +path = Spree::Core::Engine.root.join('lib/tasks/migrations/delete_prices_with_nul_amount.rake') + +RSpec.describe 'solidus:migrations:delete_prices_with_nul_amount' do + describe 'up' do + include_context( + 'rake', + task_path: path, + task_name: 'solidus:migrations:delete_prices_with_nul_amount:up' + ) + + it 'removes all prices which amount column is NULL' do + price = create(:price) + expect(Spree::Price).to receive(:where).with(amount: nil).and_return(Spree::Price.where(id: price)) + + task.invoke + + expect { price.reload }.to raise_error(ActiveRecord::RecordNotFound) + end + end +end diff --git a/core/spec/models/spree/price_spec.rb b/core/spec/models/spree/price_spec.rb index f9e76f26121..c285a28c13d 100644 --- a/core/spec/models/spree/price_spec.rb +++ b/core/spec/models/spree/price_spec.rb @@ -16,7 +16,7 @@ context 'when the amount is nil' do let(:amount) { nil } - it { is_expected.to be_valid } + it { is_expected.not_to be_valid } end context 'when the amount is less than 0' do From 29820581b8455d7e0a9712b50a3c5e293ef0ac80 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc=20Busqu=C3=A9?= Date: Mon, 12 Apr 2021 04:57:54 +0200 Subject: [PATCH 2/2] Fix typo: `nul` -> `nil` --- ...h_nul_amount.rake => delete_prices_with_nil_amount.rake} | 2 +- core/lib/tasks/upgrade.rake | 2 +- .../tasks/migrations/delete_prices_with_nil_amount_spec.rb | 6 +++--- 3 files changed, 5 insertions(+), 5 deletions(-) rename core/lib/tasks/migrations/{delete_prices_with_nul_amount.rake => delete_prices_with_nil_amount.rake} (85%) diff --git a/core/lib/tasks/migrations/delete_prices_with_nul_amount.rake b/core/lib/tasks/migrations/delete_prices_with_nil_amount.rake similarity index 85% rename from core/lib/tasks/migrations/delete_prices_with_nul_amount.rake rename to core/lib/tasks/migrations/delete_prices_with_nil_amount.rake index 399fb6a136f..96cbaafda3b 100644 --- a/core/lib/tasks/migrations/delete_prices_with_nul_amount.rake +++ b/core/lib/tasks/migrations/delete_prices_with_nil_amount.rake @@ -2,7 +2,7 @@ namespace :solidus do namespace :migrations do - namespace :delete_prices_with_nul_amount do + namespace :delete_prices_with_nil_amount do task up: :environment do print "Deleting prices wich amount attribute is nil ... " Spree::Price.where(amount: nil).delete_all diff --git a/core/lib/tasks/upgrade.rake b/core/lib/tasks/upgrade.rake index 861232cbd0c..cb10b9cf31c 100644 --- a/core/lib/tasks/upgrade.rake +++ b/core/lib/tasks/upgrade.rake @@ -12,7 +12,7 @@ namespace :solidus do desc "Upgrade Solidus to version 3.0" task three_point_zero: [ - 'solidus:migrations:delete_prices_with_nul_amount:up', + 'solidus:migrations:delete_prices_with_nil_amount:up', ] do puts "Your Solidus install is ready for Solidus 3.0" end diff --git a/core/spec/lib/tasks/migrations/delete_prices_with_nil_amount_spec.rb b/core/spec/lib/tasks/migrations/delete_prices_with_nil_amount_spec.rb index 49d8739bcba..331fab94b89 100644 --- a/core/spec/lib/tasks/migrations/delete_prices_with_nil_amount_spec.rb +++ b/core/spec/lib/tasks/migrations/delete_prices_with_nil_amount_spec.rb @@ -2,14 +2,14 @@ require 'rails_helper' -path = Spree::Core::Engine.root.join('lib/tasks/migrations/delete_prices_with_nul_amount.rake') +path = Spree::Core::Engine.root.join('lib/tasks/migrations/delete_prices_with_nil_amount.rake') -RSpec.describe 'solidus:migrations:delete_prices_with_nul_amount' do +RSpec.describe 'solidus:migrations:delete_prices_with_nil_amount' do describe 'up' do include_context( 'rake', task_path: path, - task_name: 'solidus:migrations:delete_prices_with_nul_amount:up' + task_name: 'solidus:migrations:delete_prices_with_nil_amount:up' ) it 'removes all prices which amount column is NULL' do