From 9ad2cf78ce296cadc82b106c7ad3d4c500bf5764 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Fri, 12 Jan 2024 16:02:27 +1100 Subject: [PATCH 1/4] Spec requiring tax category when creating products We observed an error when an instance requires a tax category and we tried to create products via the DFC API: * https://github.com/openfoodfoundation/openfoodnetwork/issues/11212 But I found that the error only appears after changing the instance config without declaring a tax category as default. The right setup as in the spec does work. The spec passes. I don't think that this needs any fix. We shouldn't assign any tax category just because it's required. The instance manager needs to select a default. --- engines/dfc_provider/spec/requests/supplied_products_spec.rb | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/engines/dfc_provider/spec/requests/supplied_products_spec.rb b/engines/dfc_provider/spec/requests/supplied_products_spec.rb index 3e59aa2bad..fad98579c1 100644 --- a/engines/dfc_provider/spec/requests/supplied_products_spec.rb +++ b/engines/dfc_provider/spec/requests/supplied_products_spec.rb @@ -79,6 +79,11 @@ describe "SuppliedProducts", type: :request, swagger_doc: "dfc.yaml", rswag_auto end it "creates a product and variant" do |example| + # Despite requiring a tax catogory... + # https://github.com/openfoodfoundation/openfoodnetwork/issues/11212 + create(:tax_category, is_default: true) + Spree::Config.products_require_tax_category = true + expect { submit_request(example.metadata) } .to change { enterprise.supplied_products.count }.by(1) From 95c6a56e2ecd01f4dbd258bf51720f7805702b05 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Fri, 12 Jan 2024 16:49:45 +1100 Subject: [PATCH 2/4] Simplify loading of default tax category The logic doesn't change but I simplified it and added more detailed specs. --- app/models/spree/variant.rb | 6 +---- spec/models/spree/variant_spec.rb | 39 +++++++++++++++++++++---------- 2 files changed, 28 insertions(+), 17 deletions(-) diff --git a/app/models/spree/variant.rb b/app/models/spree/variant.rb index adfa2b479d..1d8dda53ff 100644 --- a/app/models/spree/variant.rb +++ b/app/models/spree/variant.rb @@ -167,11 +167,7 @@ module Spree end def tax_category - if self[:tax_category_id].nil? - TaxCategory.find_by(is_default: true) - else - TaxCategory.find(self[:tax_category_id]) - end + super || TaxCategory.find_by(is_default: true) end def price_with_fees(distributor, order_cycle) diff --git a/spec/models/spree/variant_spec.rb b/spec/models/spree/variant_spec.rb index d729f08bd5..82317e9571 100644 --- a/spec/models/spree/variant_spec.rb +++ b/spec/models/spree/variant_spec.rb @@ -23,20 +23,35 @@ describe Spree::Variant do end describe "tax category" do - context "when a tax category is required" do - it "is invalid when a tax category is not provided" do - with_products_require_tax_category(true) do - expect(build_stubbed(:variant, tax_category_id: nil)).not_to be_valid - end - end + # `build_stubbed` avoids creating a tax category in the database. + subject(:variant) { build_stubbed(:variant) } + + it "is valid when empty by default" do + expect(variant.tax_category).to eq nil + expect(variant).to be_valid end - context "when a tax category is not required" do - it "is valid when a tax category is not provided" do - with_products_require_tax_category(false) do - expect(build_stubbed(:variant, tax_category_id: nil)).to be_valid - end - end + it "loads the default tax category" do + default = create(:tax_category, is_default: true) + + expect(variant.tax_category).to eq default + expect { + variant.tax_category = nil + }.to_not change { + variant.tax_category + } + expect(variant).to be_valid + end + + it "doesn't load any tax category" do + non_default = create(:tax_category, is_default: false) + expect(variant.tax_category).to eq nil + end + + context "when a tax category is required" do + before { Spree::Config.products_require_tax_category = true } + + it { is_expected.to validate_presence_of :tax_category } end end end From c5b64e875fffa874e6692b3a90cbe6210514aba6 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Fri, 12 Jan 2024 17:12:40 +1100 Subject: [PATCH 3/4] Remove unnecessary spec helper Best viewed ignoring whitespaces. Products don't require a tax category by default. And when you activate it via Spree::Config then it's automatically reset at the end of the spec. We don't need this helper to do it. --- spec/support/products_helper.rb | 9 ------- spec/system/admin/products_spec.rb | 40 ++++++++++++++---------------- 2 files changed, 19 insertions(+), 30 deletions(-) diff --git a/spec/support/products_helper.rb b/spec/support/products_helper.rb index 1a865f3fd4..3fcc9e4143 100644 --- a/spec/support/products_helper.rb +++ b/spec/support/products_helper.rb @@ -2,15 +2,6 @@ module OpenFoodNetwork module ProductsHelper - def with_products_require_tax_category(value) - original_value = Spree::Config.products_require_tax_category - - Spree::Config.products_require_tax_category = value - yield - ensure - Spree::Config.products_require_tax_category = original_value - end - shared_examples "modifying product actions are restricted" do it "cannot create a new product if not an admin" do api_post :create, product: { name: "Brand new product!" } diff --git a/spec/system/admin/products_spec.rb b/spec/system/admin/products_spec.rb index 0a83938ce0..12f1cddee5 100644 --- a/spec/system/admin/products_spec.rb +++ b/spec/system/admin/products_spec.rb @@ -356,32 +356,30 @@ describe ' context "products do not require a tax category" do it "creating a new product" do - with_products_require_tax_category(false) do - visit spree.admin_products_path - click_link 'New Product' + visit spree.admin_products_path + click_link 'New Product' - fill_in 'product_name', with: 'A new product !!!' - fill_in 'product_price', with: '19.99' + fill_in 'product_name', with: 'A new product !!!' + fill_in 'product_price', with: '19.99' - expect(page).to have_selector('#product_supplier_id') - select 'Another Supplier', from: 'product_supplier_id' - select 'Weight (g)', from: 'product_variant_unit_with_scale' - fill_in 'product_unit_value', with: '500' - select taxon.name, from: "product_primary_taxon_id" - select 'None', from: "product_tax_category_id" + expect(page).to have_selector('#product_supplier_id') + select 'Another Supplier', from: 'product_supplier_id' + select 'Weight (g)', from: 'product_variant_unit_with_scale' + fill_in 'product_unit_value', with: '500' + select taxon.name, from: "product_primary_taxon_id" + select 'None', from: "product_tax_category_id" - # Should only have suppliers listed which the user can manage - expect(page).to have_select 'product_supplier_id', - with_options: [@supplier2.name, @supplier_permitted.name] - expect(page).not_to have_select 'product_supplier_id', with_options: [@supplier.name] + # Should only have suppliers listed which the user can manage + expect(page).to have_select 'product_supplier_id', + with_options: [@supplier2.name, @supplier_permitted.name] + expect(page).not_to have_select 'product_supplier_id', with_options: [@supplier.name] - click_button 'Create' + click_button 'Create' - expect(flash_message).to eq('Product "A new product !!!" has been successfully created!') - product = Spree::Product.find_by(name: 'A new product !!!') - expect(product.supplier).to eq(@supplier2) - expect(product.variants.first.tax_category).to be_nil - end + expect(flash_message).to eq('Product "A new product !!!" has been successfully created!') + product = Spree::Product.find_by(name: 'A new product !!!') + expect(product.supplier).to eq(@supplier2) + expect(product.variants.first.tax_category).to be_nil end end From ad26a006e238d0ffba49e5afa5262c9708504aad Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Fri, 12 Jan 2024 17:18:07 +1100 Subject: [PATCH 4/4] Remove unnecessary spec helper module These shared examples were used in only one spec file. It's much easier to read having all the related specs in one file instead of hiding some in a helper module. It's also one less file to load whenever we run specs. --- spec/base_spec_helper.rb | 1 - .../api/v0/products_controller_spec.rb | 15 ++++++++++++- spec/support/products_helper.rb | 22 ------------------- 3 files changed, 14 insertions(+), 24 deletions(-) delete mode 100644 spec/support/products_helper.rb diff --git a/spec/base_spec_helper.rb b/spec/base_spec_helper.rb index 6e36c6ed46..f1d4cc7818 100644 --- a/spec/base_spec_helper.rb +++ b/spec/base_spec_helper.rb @@ -231,7 +231,6 @@ RSpec.configure do |config| config.include PreferencesHelper config.include OpenFoodNetwork::FiltersHelper config.include OpenFoodNetwork::EnterpriseGroupsHelper - config.include OpenFoodNetwork::ProductsHelper config.include OpenFoodNetwork::DistributionHelper config.include OpenFoodNetwork::HtmlHelper config.include ActionView::Helpers::DateHelper diff --git a/spec/controllers/api/v0/products_controller_spec.rb b/spec/controllers/api/v0/products_controller_spec.rb index b96081a57e..af6dbfe032 100644 --- a/spec/controllers/api/v0/products_controller_spec.rb +++ b/spec/controllers/api/v0/products_controller_spec.rb @@ -52,7 +52,20 @@ describe Api::V0::ProductsController, type: :controller do expect(response.status).to eq(404) end - include_examples "modifying product actions are restricted" + it "cannot create a new product if not an admin" do + api_post :create, product: { name: "Brand new product!" } + assert_unauthorized! + end + + it "cannot update a product" do + api_put :update, id: product.to_param, product: { name: "I hacked your store!" } + assert_unauthorized! + end + + it "cannot delete a product" do + api_delete :destroy, id: product.to_param + assert_unauthorized! + end end context "as an enterprise user" do diff --git a/spec/support/products_helper.rb b/spec/support/products_helper.rb deleted file mode 100644 index 3fcc9e4143..0000000000 --- a/spec/support/products_helper.rb +++ /dev/null @@ -1,22 +0,0 @@ -# frozen_string_literal: true - -module OpenFoodNetwork - module ProductsHelper - shared_examples "modifying product actions are restricted" do - it "cannot create a new product if not an admin" do - api_post :create, product: { name: "Brand new product!" } - assert_unauthorized! - end - - it "cannot update a product" do - api_put :update, id: product.to_param, product: { name: "I hacked your store!" } - assert_unauthorized! - end - - it "cannot delete a product" do - api_delete :destroy, id: product.to_param - assert_unauthorized! - end - end - end -end