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

app/controllers/spree/admin/products_controller.rb

@@ -46,7 +46,7 @@ module Spree
             # Re-fill the form with deleted params on product
             @on_hand = request.params[:product][:on_hand]
             @on_demand = request.params[:product][:on_demand]
-            render :new
+            render :new, status: :unprocessable_entity
           end
         end
       end

app/models/product_import/entry_processor.rb

@@ -149,6 +149,7 @@ module ProductImport
       end
     end
 
+    # rubocop:disable Metrics/AbcSize
     def save_new_product(entry)
       @already_created ||= {}
       # If we've already added a new product with these attributes
@@ -162,7 +163,7 @@ module ProductImport
         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')
       )
@@ -177,6 +178,7 @@ module ProductImport
 
       @already_created.deep_merge! entry.enterprise_id => { entry.name => product.id }
     end
+    # rubocop:enable Metrics/AbcSize
 
     def save_variant(entry)
       variant = entry.product_object

app/models/product_import/entry_validator.rb

@@ -377,7 +377,7 @@ module ProductImport
     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 @@ module Spree
     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 @@ module Spree
 
     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 @@ class SuppliedProductBuilder < DfcBuilder
         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 @@ class SuppliedProductBuilder < DfcBuilder
     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 @@ RSpec.describe SuppliedProductBuilder do
     }
 
     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 @@ RSpec.describe SuppliedProductBuilder do
 
     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 @@ RSpec.describe Api::V0::ProductsController, type: :controller do
       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

@@ -168,7 +168,17 @@ RSpec.describe Spree::Admin::ProductsController, type: :controller do
 
         spree_put :create, product: product_attrs_with_image
 
-        expect(response.status).to eq 200
+        expect(response.status).to eq 422
+      end
+    end
+
+    describe "when variant attributes are missing" do
+      it 'renders 422 error' do
+        spree_post :create, product: product_attrs.merge!(
+          { supplier_id: nil, primary_taxon_id: nil }
+        ),
+                            button: 'create'
+        expect(response.status).to eq 422
       end
     end
   end

spec/system/admin/products_spec.rb

@@ -170,14 +170,7 @@ RSpec.describe '
       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'