diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index 5f671a1e887..fd4b9ed7e74 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -12,22 +12,6 @@ Layout/EmptyLines: Exclude: - 'app/services/products_renderer.rb' -# Offense count: 6 -# This cop supports safe autocorrection (--autocorrect). -# Configuration parameters: EnforcedStyle, IndentationWidth. -# SupportedStyles: aligned, indented, indented_relative_to_receiver -Layout/MultilineMethodCallIndentation: - Exclude: - - 'app/services/products_renderer.rb' - -# Offense count: 2 -# This cop supports safe autocorrection (--autocorrect). -# Configuration parameters: EnforcedStyle, IndentationWidth. -# SupportedStyles: aligned, indented -Layout/MultilineOperationIndentation: - Exclude: - - 'app/services/products_renderer.rb' - # Offense count: 16 # Configuration parameters: AllowComments, AllowEmptyLambdas. Lint/EmptyBlock: @@ -101,7 +85,7 @@ Lint/UselessMethodDefinition: Exclude: - 'app/models/spree/gateway.rb' -# Offense count: 23 +# Offense count: 24 # Configuration parameters: AllowedMethods, AllowedPatterns, CountRepeatedAttributes, Max. Metrics/AbcSize: Exclude: @@ -114,6 +98,7 @@ Metrics/AbcSize: - 'app/helpers/spree/admin/navigation_helper.rb' - 'app/models/enterprise_group.rb' - 'app/models/enterprise_relationship.rb' + - 'app/models/product_import/entry_processor.rb' - 'app/models/spree/ability.rb' - 'app/models/spree/address.rb' - 'app/models/spree/order/checkout.rb' @@ -142,7 +127,7 @@ Metrics/BlockNesting: Exclude: - 'app/models/spree/payment/processing.rb' -# Offense count: 47 +# Offense count: 46 # Configuration parameters: CountComments, Max, CountAsOne. Metrics/ClassLength: Exclude: @@ -178,7 +163,6 @@ Metrics/ClassLength: - 'app/models/spree/variant.rb' - 'app/models/spree/zone.rb' - 'app/reflexes/admin/orders_reflex.rb' - - 'app/reflexes/products_reflex.rb' - 'app/serializers/api/cached_enterprise_serializer.rb' - 'app/serializers/api/enterprise_shopfront_serializer.rb' - 'app/services/cart_service.rb' @@ -408,7 +392,6 @@ RSpecRails/HaveHttpStatus: - 'spec/controllers/stripe/webhooks_controller_spec.rb' - 'spec/controllers/user_passwords_controller_spec.rb' - 'spec/controllers/user_registrations_controller_spec.rb' - - 'spec/requests/admin/images_spec.rb' - 'spec/requests/api/routes_spec.rb' - 'spec/requests/checkout/stripe_sca_spec.rb' - 'spec/requests/home_controller_spec.rb' @@ -719,7 +702,7 @@ Style/ClassAndModuleChildren: - 'lib/open_food_network/locking.rb' - 'spec/models/spree/payment_method_spec.rb' -# Offense count: 2 +# Offense count: 1 # This cop supports unsafe autocorrection (--autocorrect-all). # Configuration parameters: EnforcedStyle. # SupportedStyles: always, always_true, never diff --git a/app/models/product_import/entry_processor.rb b/app/models/product_import/entry_processor.rb index 8ce73489a94..c7596cc1ae3 100644 --- a/app/models/product_import/entry_processor.rb +++ b/app/models/product_import/entry_processor.rb @@ -162,7 +162,7 @@ def save_new_product(entry) return end - product = Spree::Product.new + product = Spree::Product.new(supplier_id: entry.enterprise_id) product.assign_attributes( entry.assignable_attributes.except('id', 'on_hand', 'on_demand', 'display_name') ) diff --git a/app/models/product_import/entry_validator.rb b/app/models/product_import/entry_validator.rb index 2705ff1126a..4db5a60d2c1 100644 --- a/app/models/product_import/entry_validator.rb +++ b/app/models/product_import/entry_validator.rb @@ -377,7 +377,7 @@ def product_validation(entry) end def mark_as_new_product(entry) - new_product = Spree::Product.new + new_product = Spree::Product.new(supplier_id: entry.enterprise_id) new_product.assign_attributes( entry.assignable_attributes.except('id', 'on_hand', 'on_demand', 'display_name') ) diff --git a/app/models/spree/product.rb b/app/models/spree/product.rb index 3703cc1864e..d484b6d1236 100755 --- a/app/models/spree/product.rb +++ b/app/models/spree/product.rb @@ -69,6 +69,7 @@ class Product < ApplicationRecord attr_accessor :price, :display_as, :unit_value, :unit_description, :tax_category_id, :shipping_category_id, :primary_taxon_id, :supplier_id + after_validation :validate_variant_attrs, on: :create after_create :ensure_standard_variant after_update :touch_supplier, if: :saved_change_to_primary_taxon_id? around_destroy :destruction @@ -289,6 +290,21 @@ def description=(html) private + def validate_variant_attrs + # Avoid running validation when we can't set variant attrs + # eg clone product. Will raise error if clonning a product with no variant + return if variants.first&.valid? + + unless Spree::Taxon.find_by(id: primary_taxon_id) + errors.add(:primary_taxon_id, + I18n.t('activerecord.errors.models.spree/product.must_exist')) + end + return if Enterprise.find_by(id: supplier_id) + + errors.add(:supplier_id, + I18n.t('activerecord.errors.models.spree/product.must_exist')) + end + def update_units return unless saved_change_to_variant_unit? || saved_change_to_variant_unit_name? diff --git a/app/views/spree/admin/products/new.html.haml b/app/views/spree/admin/products/new.html.haml index db7968a1dae..70d620ef799 100644 --- a/app/views/spree/admin/products/new.html.haml +++ b/app/views/spree/admin/products/new.html.haml @@ -12,7 +12,7 @@ = f.label :supplier, t(".supplier") %span.required * = f.select :supplier_id, options_from_collection_for_select(@producers, :id, :name, @product.supplier_id), { include_blank: t("spree.admin.products.new.supplier_select_placeholder") }, { "data-controller": "tom-select", class: "primary" } - = f.error_message_on :supplier + = f.error_message_on :supplier_id .eight.columns.omega = f.field_container :name do = f.label :name, t(".product_name") diff --git a/config/locales/en.yml b/config/locales/en.yml index 83735ca5af1..d834a7c4f19 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -68,7 +68,7 @@ en: spree/product: name: "Product Name" price: "Price" - primary_taxon: "Product Category" + primary_taxon_id: "Product Category" shipping_category_id: "Shipping Category" variant_unit: "Variant Unit" variant_unit_name: "Variant Unit Name" @@ -120,6 +120,8 @@ en: attributes: base: card_expired: "has expired" + spree/product: + must_exist: 'must exist' order_cycle: attributes: orders_close_at: diff --git a/engines/dfc_provider/app/services/supplied_product_builder.rb b/engines/dfc_provider/app/services/supplied_product_builder.rb index 9c1e83e8f5b..ee64cccafea 100644 --- a/engines/dfc_provider/app/services/supplied_product_builder.rb +++ b/engines/dfc_provider/app/services/supplied_product_builder.rb @@ -35,9 +35,7 @@ def self.import_variant(supplied_product, supplier) apply(supplied_product, variant) end else - product = import_product(supplied_product) - product.ensure_standard_variant - product.variants.first.supplier = supplier + product = import_product(supplied_product, supplier) product.variants.first end.tap do |variant| link = supplied_product.semanticId @@ -62,15 +60,16 @@ def self.referenced_spree_product(supplied_product, supplier) end end - def self.import_product(supplied_product) + def self.import_product(supplied_product, supplier) Spree::Product.new( name: supplied_product.name, description: supplied_product.description, - price: 0 # will be in DFC Offer + price: 0, # will be in DFC Offer + supplier_id: supplier.id, + primary_taxon_id: taxon(supplied_product).id ).tap do |product| QuantitativeValueBuilder.apply(supplied_product.quantity, product) product.ensure_standard_variant - product.variants.first.primary_taxon = taxon(supplied_product) end end diff --git a/engines/dfc_provider/spec/services/supplied_product_builder_spec.rb b/engines/dfc_provider/spec/services/supplied_product_builder_spec.rb index c39aab031e7..4b3fa4c4450 100644 --- a/engines/dfc_provider/spec/services/supplied_product_builder_spec.rb +++ b/engines/dfc_provider/spec/services/supplied_product_builder_spec.rb @@ -107,7 +107,7 @@ } it "creates a new Spree::Product" do - product = builder.import_product(supplied_product) + product = builder.import_product(supplied_product, supplier) expect(product).to be_a(Spree::Product) expect(product.name).to eq("Tomato") @@ -117,7 +117,7 @@ describe "taxon" do it "assigns the taxon matching the DFC product type" do - product = builder.import_product(supplied_product) + product = builder.import_product(supplied_product, supplier) expect(product.variants.first.primary_taxon).to eq(taxon) end diff --git a/spec/controllers/api/v0/products_controller_spec.rb b/spec/controllers/api/v0/products_controller_spec.rb index df999c4ab4c..3633dab6ea7 100644 --- a/spec/controllers/api/v0/products_controller_spec.rb +++ b/spec/controllers/api/v0/products_controller_spec.rb @@ -120,7 +120,10 @@ expect(response.status).to eq(422) expect(json_response["error"]).to eq("Invalid resource. Please fix errors and try again.") errors = json_response["errors"] - expect(errors.keys).to match_array(["name", "variant_unit", "price"]) + expect(errors.keys).to match_array([ + "name", "variant_unit", "price", + "primary_taxon_id", "supplier_id" + ]) end it "can update a product" do diff --git a/spec/controllers/spree/admin/products_controller_spec.rb b/spec/controllers/spree/admin/products_controller_spec.rb index f41c4adfcd8..f7f81f2cdc0 100644 --- a/spec/controllers/spree/admin/products_controller_spec.rb +++ b/spec/controllers/spree/admin/products_controller_spec.rb @@ -171,6 +171,17 @@ expect(response.status).to eq 200 end end + + describe "when variant attributes are missing" do + it 'renders form with errors' do + spree_post :create, product: product_attrs.merge!( + { supplier_id: nil, primary_taxon_id: nil } + ), + button: 'create' + expect(response.status).to eq 200 + expect(response).to render_template('spree/admin/products/new') + end + end end describe "updating a product" do diff --git a/spec/system/admin/products_spec.rb b/spec/system/admin/products_spec.rb index abe75464db2..9fad202f793 100644 --- a/spec/system/admin/products_spec.rb +++ b/spec/system/admin/products_spec.rb @@ -170,14 +170,7 @@ expect(page).to have_content "Unit value is not a number" end - it "creating product with empty product category" do - pending("#12591") - - login_as_admin - visit spree.admin_products_path - - click_link 'New Product' - + it "creating product with empty product category fails" do fill_in 'product_name', with: 'Hot Cakes' select 'New supplier', from: 'product_supplier_id' select "Weight (kg)", from: 'product_variant_unit_with_scale' @@ -194,6 +187,23 @@ expect(page).to have_content "Product Category must exist" end + it "creating product with empty product supplier fails" do + fill_in 'product_name', with: 'Hot Cakes' + select "Weight (kg)", from: 'product_variant_unit_with_scale' + fill_in "product_unit_value", with: '1' + fill_in 'product_price', with: '1.99' + select taxon.name, from: "product_primary_taxon_id" + fill_in 'product_on_hand', with: 0 + check 'product_on_demand' + select 'Test Tax Category', from: 'product_tax_category_id' + fill_in_trix_editor 'product_description', + with: 'In demand, and on_demand! The hottest cakes in town.' + click_button 'Create' + + expect(current_path).to eq spree.admin_products_path + expect(page).to have_content "Supplier must exist" + end + describe "localization settings" do shared_examples "with different price values" do |localized_number, price| context "when enable_localized_number is set to #{localized_number}" do