Merge pull request #12671 from wandji20/wb-OFN-12591

Ensure product category error message is shown when creating new product [OFN-12591]

.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

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')
       )

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')
       )

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?
 

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")

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:

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
 

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

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

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

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