From 9ebe112bcc754136211068b9e1f9d0cb02842f2b Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Fri, 11 Jan 2019 15:52:09 +0100 Subject: [PATCH 1/5] Abstract OFN's default stock location into a class --- app/services/default_stock_location.rb | 15 +++++++++ spec/factories.rb | 4 ++- spec/services/default_stock_location_spec.rb | 32 ++++++++++++++++++++ 3 files changed, 50 insertions(+), 1 deletion(-) create mode 100644 app/services/default_stock_location.rb create mode 100644 spec/services/default_stock_location_spec.rb diff --git a/app/services/default_stock_location.rb b/app/services/default_stock_location.rb new file mode 100644 index 0000000000..e2df33cb71 --- /dev/null +++ b/app/services/default_stock_location.rb @@ -0,0 +1,15 @@ +# Encapsulates the concept of default stock location that OFN has, as explained +# in https://github.com/openfoodfoundation/openfoodnetwork/wiki/Spree-Upgrade%3A-Stock-locations +class DefaultStockLocation + NAME = 'OFN default'.freeze + + def self.create! + country = Spree::Country.find_by_iso(ENV['DEFAULT_COUNTRY_CODE']) + state = country.states.first + Spree::StockLocation.create!(name: NAME, country_id: country.id, state_id: state.id) + end + + def self.destroy_all + Spree::StockLocation.where(name: NAME).destroy_all + end +end diff --git a/spec/factories.rb b/spec/factories.rb index 58273f7cb5..582dd4d98b 100644 --- a/spec/factories.rb +++ b/spec/factories.rb @@ -656,8 +656,10 @@ end FactoryBot.modify do factory :stock_location, class: Spree::StockLocation do + name { 'default' } + # keeps the test stock_location unique - initialize_with { Spree::StockLocation.find_or_create_by_name(name)} + initialize_with { DefaultStockLocation.find_or_create } end factory :shipment, class: Spree::Shipment do diff --git a/spec/services/default_stock_location_spec.rb b/spec/services/default_stock_location_spec.rb new file mode 100644 index 0000000000..b2b9691267 --- /dev/null +++ b/spec/services/default_stock_location_spec.rb @@ -0,0 +1,32 @@ +require 'spec_helper' + +describe DefaultStockLocation do + describe '.create!' do + it "names the location 'OFN default'" do + stock_location = described_class.create! + expect(stock_location.name).to eq('OFN default') + end + + it 'sets the location in the default country' do + default_country = Spree::Country.find_by_iso(ENV['DEFAULT_COUNTRY_CODE']) + stock_location = described_class.create! + expect(stock_location.country).to eq(default_country) + end + + it 'sets the first state in the country' do + default_country = Spree::Country.find_by_iso(ENV['DEFAULT_COUNTRY_CODE']) + stock_location = described_class.create! + expect(stock_location.state).to eq(default_country.states.first) + end + end + + describe '.destroy_all' do + it "removes all stock locations named 'OFN default'" do + create(:stock_location, name: 'OFN default') + create(:stock_location, name: 'OFN default') + + expect { described_class.destroy_all } + .to change { Spree::StockLocation.count }.from(2).to(0) + end + end +end From 730d5ff7d62714d9aef957479434be9cf114930e Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Thu, 3 Jan 2019 12:59:58 +0100 Subject: [PATCH 2/5] Include the default location in seed data This ensures that new OFN instances will have the proper stock location set up. --- db/seeds.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/db/seeds.rb b/db/seeds.rb index 17e5bf91b7..9f0868cb6b 100644 --- a/db/seeds.rb +++ b/db/seeds.rb @@ -50,3 +50,5 @@ end spree_user = Spree::User.first spree_user && spree_user.confirm! + +DefaultStockLocation.create! From 10a678bdfeb8a0e2964810a6263601226ab694f6 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Fri, 11 Jan 2019 15:46:41 +0100 Subject: [PATCH 3/5] Rely on Spree's default StockLocation --- app/services/default_stock_location.rb | 6 ++- spec/factories.rb | 5 ++- spec/services/default_stock_location_spec.rb | 43 +++++++++++++++++--- 3 files changed, 46 insertions(+), 8 deletions(-) diff --git a/app/services/default_stock_location.rb b/app/services/default_stock_location.rb index e2df33cb71..9269f17743 100644 --- a/app/services/default_stock_location.rb +++ b/app/services/default_stock_location.rb @@ -1,7 +1,7 @@ # Encapsulates the concept of default stock location that OFN has, as explained # in https://github.com/openfoodfoundation/openfoodnetwork/wiki/Spree-Upgrade%3A-Stock-locations class DefaultStockLocation - NAME = 'OFN default'.freeze + NAME = 'default'.freeze def self.create! country = Spree::Country.find_by_iso(ENV['DEFAULT_COUNTRY_CODE']) @@ -12,4 +12,8 @@ class DefaultStockLocation def self.destroy_all Spree::StockLocation.where(name: NAME).destroy_all end + + def self.find_or_create + Spree::StockLocation.find_or_create_by_name(NAME) + end end diff --git a/spec/factories.rb b/spec/factories.rb index 582dd4d98b..742f943ed7 100644 --- a/spec/factories.rb +++ b/spec/factories.rb @@ -656,10 +656,11 @@ end FactoryBot.modify do factory :stock_location, class: Spree::StockLocation do - name { 'default' } - # keeps the test stock_location unique initialize_with { DefaultStockLocation.find_or_create } + + # Ensures the name attribute is not assigned after instantiating the default location + transient { name 'default' } end factory :shipment, class: Spree::Shipment do diff --git a/spec/services/default_stock_location_spec.rb b/spec/services/default_stock_location_spec.rb index b2b9691267..8d91d7da83 100644 --- a/spec/services/default_stock_location_spec.rb +++ b/spec/services/default_stock_location_spec.rb @@ -4,7 +4,7 @@ describe DefaultStockLocation do describe '.create!' do it "names the location 'OFN default'" do stock_location = described_class.create! - expect(stock_location.name).to eq('OFN default') + expect(stock_location.name).to eq('default') end it 'sets the location in the default country' do @@ -21,12 +21,45 @@ describe DefaultStockLocation do end describe '.destroy_all' do - it "removes all stock locations named 'OFN default'" do - create(:stock_location, name: 'OFN default') - create(:stock_location, name: 'OFN default') + it "removes all stock locations named 'default'" do + create(:stock_location, name: 'default') expect { described_class.destroy_all } - .to change { Spree::StockLocation.count }.from(2).to(0) + .to change { Spree::StockLocation.count }.to(0) + end + end + + describe '.find_or_create' do + context 'when a location named default already exists' do + let!(:location) do + country = create(:country) + state = create(:state, country: country) + Spree::StockLocation.create!( + name: 'default', + country_id: country.id, + state_id: state.id + ) + end + + it 'returns the location' do + expect(described_class.find_or_create).to eq(location) + end + + it 'does not create any other location' do + expect { described_class.find_or_create }.not_to change(Spree::StockLocation, :count) + end + end + + context 'when a location named default does not exist' do + it 'returns the location' do + location = described_class.find_or_create + expect(location.name).to eq('default') + end + + it 'does not create any other location' do + expect { described_class.find_or_create } + .to change(Spree::StockLocation, :count).from(0).to(1) + end end end end From 817bf4caa1457c2b65331c1cc266a9de30a0d5c0 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Wed, 9 Jan 2019 18:56:40 +0100 Subject: [PATCH 4/5] Explain why a index forbids more than a location --- ...094641_add_uniqueness_of_variant_id_to_spree_stock_items.rb | 3 +++ 1 file changed, 3 insertions(+) diff --git a/db/migrate/20180906094641_add_uniqueness_of_variant_id_to_spree_stock_items.rb b/db/migrate/20180906094641_add_uniqueness_of_variant_id_to_spree_stock_items.rb index bc7713f428..93b2853208 100644 --- a/db/migrate/20180906094641_add_uniqueness_of_variant_id_to_spree_stock_items.rb +++ b/db/migrate/20180906094641_add_uniqueness_of_variant_id_to_spree_stock_items.rb @@ -1,3 +1,6 @@ +# Since OFN has only a single default StockLocation, variants in OFN can only +# have a stock item. By adding this unique index we constraint that at DB level +# ensuring data integrity. class AddUniquenessOfVariantIdToSpreeStockItems < ActiveRecord::Migration def change add_index :spree_stock_items, :variant_id, unique: true From 5b8c0dffe2a500b44c3c0920874cd364a820328c Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Fri, 11 Jan 2019 15:08:54 +0100 Subject: [PATCH 5/5] Remove test for a state that is no longer possible --- spec/models/concerns/variant_stock_spec.rb | 15 --------------- 1 file changed, 15 deletions(-) diff --git a/spec/models/concerns/variant_stock_spec.rb b/spec/models/concerns/variant_stock_spec.rb index 36fc33df24..2943b7575b 100644 --- a/spec/models/concerns/variant_stock_spec.rb +++ b/spec/models/concerns/variant_stock_spec.rb @@ -141,21 +141,6 @@ describe VariantStock do end describe '#on_demand=' do - context 'when the variant has multiple stock items' do - let(:variant) { create(:variant) } - - before do - # Spree creates a stock_item for each variant when creating a stock - # location by means of #propagate_variant - create(:stock_location, name: 'location') - create(:stock_location, name: 'another location') - end - - it 'raises' do - expect { variant.on_demand = true }.to raise_error(StandardError) - end - end - context 'when the variant has a stock item' do let(:variant) { create(:variant, on_demand: true) }