From c609107379eca94e020c14748911ccd6c0215ce4 Mon Sep 17 00:00:00 2001
From: Maikel Linke <maikel@email.org.au>
Date: Tue, 8 Oct 2024 15:56:09 +1100
Subject: [PATCH 01/11] Avoid race condition between checkout and stock sync

---
 app/jobs/stock_sync_job.rb | 37 ++++++++++++++++++++++++-------------
 1 file changed, 24 insertions(+), 13 deletions(-)

diff --git a/app/jobs/stock_sync_job.rb b/app/jobs/stock_sync_job.rb
index 9359aeb81e..a6559ab923 100644
--- a/app/jobs/stock_sync_job.rb
+++ b/app/jobs/stock_sync_job.rb
@@ -27,26 +27,37 @@ class StockSyncJob < ApplicationJob
   end
 
   def perform(user, semantic_id)
+    products = load_products(user, semantic_id)
+    products_by_id = products.index_by(&:semanticId)
+    product_ids = products_by_id.keys
+    variants = linked_variants(user.enterprises, product_ids)
+
+    # Avoid race condition between checkout and stock sync.
+    Spree::Variant.transaction do
+      variants.order(:id).lock.each do |variant|
+        next if variant.on_demand
+
+        product = products_by_id[variant.semantic_links[0].semantic_id]
+        catalog_item = product&.catalogItems&.first
+        CatalogItemBuilder.apply_stock(catalog_item, variant)
+        variant.stock_items[0].save!
+      end
+    end
+  end
+
+  def load_products(user, semantic_id)
     urls = FdcUrlBuilder.new(semantic_id)
     json_catalog = DfcRequest.new(user).call(urls.catalog_url)
     graph = DfcIo.import(json_catalog)
 
-    products = graph.select do |subject|
+    graph.select do |subject|
       subject.is_a? DataFoodConsortium::Connector::SuppliedProduct
     end
-    products_by_id = products.index_by(&:semanticId)
-    product_ids = products_by_id.keys
-    variants = Spree::Variant.where(supplier: user.enterprises)
+  end
+
+  def linked_variants(enterprises, product_ids)
+    Spree::Variant.where(supplier: enterprises)
       .includes(:semantic_links).references(:semantic_links)
       .where(semantic_links: { semantic_id: product_ids })
-
-    variants.each do |variant|
-      next if variant.on_demand
-
-      product = products_by_id[variant.semantic_links[0].semantic_id]
-      catalog_item = product&.catalogItems&.first
-      CatalogItemBuilder.apply_stock(catalog_item, variant)
-      variant.stock_items[0].save!
-    end
   end
 end

From 664f324db6587fa8201d1f8c34912af8079a58af Mon Sep 17 00:00:00 2001
From: Maikel Linke <maikel@email.org.au>
Date: Tue, 8 Oct 2024 16:31:19 +1100
Subject: [PATCH 02/11] Sync stock of multiple linked catalogs

And the logic becomes a bit simpler.
---
 app/jobs/stock_sync_job.rb       | 19 ++++++++++---------
 spec/jobs/stock_sync_job_spec.rb |  9 ++++++---
 2 files changed, 16 insertions(+), 12 deletions(-)

diff --git a/app/jobs/stock_sync_job.rb b/app/jobs/stock_sync_job.rb
index a6559ab923..04a849c3c7 100644
--- a/app/jobs/stock_sync_job.rb
+++ b/app/jobs/stock_sync_job.rb
@@ -12,12 +12,14 @@ class StockSyncJob < ApplicationJob
     stock_controlled_variants = order.variants.reject(&:on_demand)
     links = SemanticLink.where(variant_id: stock_controlled_variants.map(&:id))
     semantic_ids = links.pluck(:semantic_id)
-
-    return if semantic_ids.empty?
+    catalog_ids = semantic_ids.map do |product_id|
+      FdcUrlBuilder.new(product_id).catalog_url
+    end
 
     user = order.distributor.owner
-    reference_id = semantic_ids.first # Assuming one catalog for now.
-    perform_later(user, reference_id)
+    catalog_ids.uniq.each do |catalog_id|
+      perform_later(user, catalog_id)
+    end
   rescue StandardError => e
     # Errors here shouldn't affect the shopping. So let's report them
     # separately:
@@ -26,8 +28,8 @@ class StockSyncJob < ApplicationJob
     end
   end
 
-  def perform(user, semantic_id)
-    products = load_products(user, semantic_id)
+  def perform(user, catalog_id)
+    products = load_products(user, catalog_id)
     products_by_id = products.index_by(&:semanticId)
     product_ids = products_by_id.keys
     variants = linked_variants(user.enterprises, product_ids)
@@ -45,9 +47,8 @@ class StockSyncJob < ApplicationJob
     end
   end
 
-  def load_products(user, semantic_id)
-    urls = FdcUrlBuilder.new(semantic_id)
-    json_catalog = DfcRequest.new(user).call(urls.catalog_url)
+  def load_products(user, catalog_id)
+    json_catalog = DfcRequest.new(user).call(catalog_id)
     graph = DfcIo.import(json_catalog)
 
     graph.select do |subject|
diff --git a/spec/jobs/stock_sync_job_spec.rb b/spec/jobs/stock_sync_job_spec.rb
index 2abc9eb85a..f80bc42285 100644
--- a/spec/jobs/stock_sync_job_spec.rb
+++ b/spec/jobs/stock_sync_job_spec.rb
@@ -10,6 +10,9 @@ RSpec.describe StockSyncJob do
   let(:beans_retail_link) {
     "https://env-0105831.jcloud-ver-jpe.ik-server.com/api/dfc/Enterprises/test-hodmedod/SuppliedProducts/44519466467635"
   }
+  let(:catalog_link) {
+    "https://env-0105831.jcloud-ver-jpe.ik-server.com/api/dfc/Enterprises/test-hodmedod/SuppliedProducts"
+  }
 
   describe ".sync_linked_catalogs" do
     subject { StockSyncJob.sync_linked_catalogs(order) }
@@ -23,7 +26,7 @@ RSpec.describe StockSyncJob do
       )
 
       expect { subject }.to enqueue_job(StockSyncJob)
-        .with(user, beans_retail_link)
+        .with(user, catalog_link)
     end
 
     it "reports errors" do
@@ -34,8 +37,8 @@ RSpec.describe StockSyncJob do
     end
   end
 
-  describe "#peform" do
-    subject { StockSyncJob.perform_now(user, beans_retail_link) }
+  describe "#perform" do
+    subject { StockSyncJob.perform_now(user, catalog_link) }
 
     before do
       distributor.save!

From adf0340153e526a4ff80b51206fc7e2990728ca2 Mon Sep 17 00:00:00 2001
From: Maikel Linke <maikel@email.org.au>
Date: Tue, 8 Oct 2024 16:47:43 +1100
Subject: [PATCH 03/11] Remove duplicate method

The method `CheckoutCallbacks#valid_order_line_items?` was a duplicate
of `OrderStockCheck#valid_order_line_items?`.

Apparently, it had been extracted twice:

 * 1d074c2151289e880b4971e9e490f29fcdeda095
 * 06eb98bdf4fe289ef1f1aaaddbfcbb5674dae09e

But the first commit duplicated the method while the second moved the
original declaration.
---
 app/controllers/concerns/checkout_callbacks.rb | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/app/controllers/concerns/checkout_callbacks.rb b/app/controllers/concerns/checkout_callbacks.rb
index c7eb90ebde..f703fdc790 100644
--- a/app/controllers/concerns/checkout_callbacks.rb
+++ b/app/controllers/concerns/checkout_callbacks.rb
@@ -63,12 +63,6 @@ module CheckoutCallbacks
     end
   end
 
-  def valid_order_line_items?
-    @order.insufficient_stock_lines.empty? &&
-      OrderCycles::DistributedVariantsService.new(@order.order_cycle, @order.distributor).
-        distributes_order_variants?(@order)
-  end
-
   def ensure_order_not_completed
     redirect_to main_app.cart_path if @order.completed?
   end

From 71ca292c923ec97cd50bb88ceb424bd6f8c95238 Mon Sep 17 00:00:00 2001
From: Maikel Linke <maikel@email.org.au>
Date: Tue, 8 Oct 2024 16:59:23 +1100
Subject: [PATCH 04/11] Synchronise stock with DFC catalog during checkout

This will delay the checkout request by a few seconds if there's stock
to sync. But we minimise the chance of missing reduced stock from orders
on another platform.

We still have a gap between the checkout and placing a backorder. In
that time we can't guarantee enough stock. But let's tackle that after
the pilot.
---
 .../concerns/checkout_callbacks.rb            | 14 +++++++++
 app/jobs/stock_sync_job.rb                    | 31 ++++++++++++++-----
 spec/controllers/checkout_controller_spec.rb  | 14 +++++++++
 spec/jobs/stock_sync_job_spec.rb              | 24 ++++++++++++++
 4 files changed, 75 insertions(+), 8 deletions(-)

diff --git a/app/controllers/concerns/checkout_callbacks.rb b/app/controllers/concerns/checkout_callbacks.rb
index f703fdc790..3b0fba0b1c 100644
--- a/app/controllers/concerns/checkout_callbacks.rb
+++ b/app/controllers/concerns/checkout_callbacks.rb
@@ -9,6 +9,12 @@ module CheckoutCallbacks
     # Otherwise we fail on duplicate indexes or end up with negative stock.
     prepend_around_action CurrentOrderLocker, only: [:edit, :update]
 
+    # We want to download the latest stock data before anything else happens.
+    # We don't want it to be in the same database transaction as the order
+    # locking because this action locks a different set of variants and it
+    # could cause race conditions.
+    prepend_around_action :sync_stock, only: :update
+
     prepend_before_action :check_hub_ready_for_checkout
     prepend_before_action :check_order_cycle_expiry
     prepend_before_action :require_order_cycle
@@ -25,6 +31,14 @@ module CheckoutCallbacks
 
   private
 
+  def sync_stock
+    if current_order&.state == "confirmation"
+      StockSyncJob.sync_linked_catalogs_now(current_order)
+    end
+
+    yield
+  end
+
   def load_order
     @order = current_order
     @order.manual_shipping_selection = true
diff --git a/app/jobs/stock_sync_job.rb b/app/jobs/stock_sync_job.rb
index 04a849c3c7..14d27f54e3 100644
--- a/app/jobs/stock_sync_job.rb
+++ b/app/jobs/stock_sync_job.rb
@@ -9,15 +9,8 @@ class StockSyncJob < ApplicationJob
   # enqueue a new job. That should save some time loading the order with
   # all the stock data to make this decision.
   def self.sync_linked_catalogs(order)
-    stock_controlled_variants = order.variants.reject(&:on_demand)
-    links = SemanticLink.where(variant_id: stock_controlled_variants.map(&:id))
-    semantic_ids = links.pluck(:semantic_id)
-    catalog_ids = semantic_ids.map do |product_id|
-      FdcUrlBuilder.new(product_id).catalog_url
-    end
-
     user = order.distributor.owner
-    catalog_ids.uniq.each do |catalog_id|
+    catalog_ids(order).each do |catalog_id|
       perform_later(user, catalog_id)
     end
   rescue StandardError => e
@@ -28,6 +21,28 @@ class StockSyncJob < ApplicationJob
     end
   end
 
+  def self.sync_linked_catalogs_now(order)
+    user = order.distributor.owner
+    catalog_ids(order).each do |catalog_id|
+      perform_now(user, catalog_id)
+    end
+  rescue StandardError => e
+    # Errors here shouldn't affect the shopping. So let's report them
+    # separately:
+    Bugsnag.notify(e) do |payload|
+      payload.add_metadata(:order, order)
+    end
+  end
+
+  def self.catalog_ids(order)
+    stock_controlled_variants = order.variants.reject(&:on_demand)
+    links = SemanticLink.where(variant_id: stock_controlled_variants.map(&:id))
+    semantic_ids = links.pluck(:semantic_id)
+    semantic_ids.map do |product_id|
+      FdcUrlBuilder.new(product_id).catalog_url
+    end.uniq
+  end
+
   def perform(user, catalog_id)
     products = load_products(user, catalog_id)
     products_by_id = products.index_by(&:semanticId)
diff --git a/spec/controllers/checkout_controller_spec.rb b/spec/controllers/checkout_controller_spec.rb
index ea761d876b..ed1da29653 100644
--- a/spec/controllers/checkout_controller_spec.rb
+++ b/spec/controllers/checkout_controller_spec.rb
@@ -451,6 +451,20 @@ RSpec.describe CheckoutController, type: :controller do
           expect(response).to redirect_to order_path(order, order_token: order.token)
           expect(order.reload.state).to eq "complete"
         end
+
+        it "syncs stock before locking the order" do
+          actions = []
+          expect(StockSyncJob).to receive(:sync_linked_catalogs_now) do
+            actions << "sync stock"
+          end
+          expect(CurrentOrderLocker).to receive(:around) do
+            actions << "lock order"
+          end
+
+          put(:update, params:)
+
+          expect(actions).to eq ["sync stock", "lock order"]
+        end
       end
 
       context "when accepting T&Cs is required" do
diff --git a/spec/jobs/stock_sync_job_spec.rb b/spec/jobs/stock_sync_job_spec.rb
index f80bc42285..8fd7029e18 100644
--- a/spec/jobs/stock_sync_job_spec.rb
+++ b/spec/jobs/stock_sync_job_spec.rb
@@ -37,6 +37,30 @@ RSpec.describe StockSyncJob do
     end
   end
 
+  describe ".sync_linked_catalogs_now" do
+    subject { StockSyncJob.sync_linked_catalogs_now(order) }
+    it "ignores products without semantic link" do
+      expect(StockSyncJob).not_to receive(:perform_now)
+      expect { subject }.not_to enqueue_job(StockSyncJob)
+    end
+
+    it "performs stock check now" do
+      beans.semantic_links << SemanticLink.new(
+        semantic_id: beans_retail_link
+      )
+
+      expect(StockSyncJob).to receive(:perform_now).with(user, catalog_link)
+      expect { subject }.not_to raise_error
+    end
+
+    it "reports errors" do
+      expect(order).to receive(:variants).and_raise("test error")
+      expect(Bugsnag).to receive(:notify).and_call_original
+
+      expect { subject }.not_to raise_error
+    end
+  end
+
   describe "#perform" do
     subject { StockSyncJob.perform_now(user, catalog_link) }
 

From f0b6403c1d3d1b4814c97c546d801d5bb514517e Mon Sep 17 00:00:00 2001
From: Maikel Linke <maikel@email.org.au>
Date: Thu, 10 Oct 2024 09:58:01 +1100
Subject: [PATCH 05/11] Fix locally flaky spec around date filters

This spec would fail on Australian systems early in the morning or in
other timezones accordingly.
---
 .../spec/services/affiliate_sales_query_spec.rb            | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/engines/dfc_provider/spec/services/affiliate_sales_query_spec.rb b/engines/dfc_provider/spec/services/affiliate_sales_query_spec.rb
index f1ab830977..4ae4b91954 100644
--- a/engines/dfc_provider/spec/services/affiliate_sales_query_spec.rb
+++ b/engines/dfc_provider/spec/services/affiliate_sales_query_spec.rb
@@ -11,6 +11,13 @@ RSpec.describe AffiliateSalesQuery do
     let(:yesterday) { Time.zone.yesterday }
     let(:tomorrow) { Time.zone.tomorrow }
 
+    around do |example|
+      # Query dates are interpreted as UTC while the spec runs in
+      # Melbourne time. At noon in Melbourne, the date is the same.
+      # That simplifies the spec.
+      Timecop.travel(Time.zone.today.noon, &example)
+    end
+
     it "returns data" do
       # Test data creation takes time.
       # So I'm executing more tests in one `it` block here.

From a838ef4a21ab397d61d3f9c6c159711d49e20a17 Mon Sep 17 00:00:00 2001
From: Maikel Linke <maikel@email.org.au>
Date: Thu, 10 Oct 2024 14:04:54 +1100
Subject: [PATCH 06/11] DRY DFC product import

---
 .../admin/dfc_product_imports_controller.rb       | 15 +--------------
 .../dfc_provider/supplied_products_controller.rb  | 10 +---------
 .../app/services/supplied_product_builder.rb      | 12 ++++++++++++
 3 files changed, 14 insertions(+), 23 deletions(-)

diff --git a/app/controllers/admin/dfc_product_imports_controller.rb b/app/controllers/admin/dfc_product_imports_controller.rb
index e85170db2f..7def2815e4 100644
--- a/app/controllers/admin/dfc_product_imports_controller.rb
+++ b/app/controllers/admin/dfc_product_imports_controller.rb
@@ -26,7 +26,7 @@ module Admin
       # * First step: import all products for given enterprise.
       # * Second step: render table and let user decide which ones to import.
       imported = graph.map do |subject|
-        import_product(subject, enterprise)
+        SuppliedProductBuilder.store_product(subject, enterprise)
       end
 
       @count = imported.compact.count
@@ -37,18 +37,5 @@ module Admin
     def fetch_catalog(url)
       DfcRequest.new(spree_current_user).call(url)
     end
-
-    # Most of this code is the same as in the DfcProvider::SuppliedProductsController.
-    def import_product(subject, enterprise)
-      return unless subject.is_a? DataFoodConsortium::Connector::SuppliedProduct
-
-      variant = SuppliedProductBuilder.import_variant(subject, enterprise)
-      product = variant.product
-
-      product.save! if product.new_record?
-      variant.save! if variant.new_record?
-
-      variant
-    end
   end
 end
diff --git a/engines/dfc_provider/app/controllers/dfc_provider/supplied_products_controller.rb b/engines/dfc_provider/app/controllers/dfc_provider/supplied_products_controller.rb
index 3e5b26ec8d..e2aaad9203 100644
--- a/engines/dfc_provider/app/controllers/dfc_provider/supplied_products_controller.rb
+++ b/engines/dfc_provider/app/controllers/dfc_provider/supplied_products_controller.rb
@@ -14,18 +14,10 @@ module DfcProvider
 
       return head :bad_request unless supplied_product
 
-      variant = SuppliedProductBuilder.import_variant(
+      variant = SuppliedProductBuilder.store_product(
         supplied_product,
         current_enterprise,
       )
-      product = variant.product
-
-      if variant.new_record?
-        variant.supplier = current_enterprise
-        variant.save!
-      end
-
-      product.save! if product.new_record?
 
       supplied_product = SuppliedProductBuilder.supplied_product(variant)
       render json: DfcIo.export(supplied_product)
diff --git a/engines/dfc_provider/app/services/supplied_product_builder.rb b/engines/dfc_provider/app/services/supplied_product_builder.rb
index 839ae0ce46..b335dd8f6d 100644
--- a/engines/dfc_provider/app/services/supplied_product_builder.rb
+++ b/engines/dfc_provider/app/services/supplied_product_builder.rb
@@ -26,6 +26,18 @@ class SuppliedProductBuilder < DfcBuilder
     )
   end
 
+  def self.store_product(subject, enterprise)
+    return unless subject.is_a? DataFoodConsortium::Connector::SuppliedProduct
+
+    variant = SuppliedProductBuilder.import_variant(subject, enterprise)
+    product = variant.product
+
+    product.save! if product.new_record?
+    variant.save! if variant.new_record?
+
+    variant
+  end
+
   def self.import_variant(supplied_product, supplier)
     product = referenced_spree_product(supplied_product, supplier)
 

From e429cb719818ace260e3f48e53160d28bb4de7f3 Mon Sep 17 00:00:00 2001
From: Maikel Linke <maikel@email.org.au>
Date: Thu, 10 Oct 2024 14:06:42 +1100
Subject: [PATCH 07/11] Style growing class

---
 .../dfc_provider/app/services/supplied_product_builder.rb   | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/engines/dfc_provider/app/services/supplied_product_builder.rb b/engines/dfc_provider/app/services/supplied_product_builder.rb
index b335dd8f6d..e95e45c7d0 100644
--- a/engines/dfc_provider/app/services/supplied_product_builder.rb
+++ b/engines/dfc_provider/app/services/supplied_product_builder.rb
@@ -42,11 +42,7 @@ class SuppliedProductBuilder < DfcBuilder
     product = referenced_spree_product(supplied_product, supplier)
 
     if product
-      Spree::Variant.new(
-        product:,
-        supplier:,
-        price: 0,
-      ).tap do |variant|
+      Spree::Variant.new( product:, supplier:, price: 0,).tap do |variant|
         apply(supplied_product, variant)
       end
     else

From bda506528f8d6d3d07a17baed21816f357bc838f Mon Sep 17 00:00:00 2001
From: Maikel Linke <maikel@email.org.au>
Date: Thu, 10 Oct 2024 14:08:02 +1100
Subject: [PATCH 08/11] Fix import of zero-weight products

We don't allow variants to have zero weight or volume. But a DFC import
in production showed that some catalogs list products with zero weight.
Despite the products having a weight, it's simpler to treat these as
items.
---
 .../services/quantitative_value_builder.rb    |  9 ++++-
 .../spec/fixtures/files/product.GET.json      | 38 +++++++++++++++++++
 .../services/supplied_product_builder_spec.rb | 27 +++++++++++++
 3 files changed, 73 insertions(+), 1 deletion(-)
 create mode 100644 engines/dfc_provider/spec/fixtures/files/product.GET.json

diff --git a/engines/dfc_provider/app/services/quantitative_value_builder.rb b/engines/dfc_provider/app/services/quantitative_value_builder.rb
index ecbc0f4c59..31401a3369 100644
--- a/engines/dfc_provider/app/services/quantitative_value_builder.rb
+++ b/engines/dfc_provider/app/services/quantitative_value_builder.rb
@@ -29,11 +29,18 @@ class QuantitativeValueBuilder < DfcBuilder
 
   def self.apply(quantity, product)
     measure, unit_name, unit_scale = map_unit(quantity.unit)
+    value = quantity.value.to_f * unit_scale
+
+    if measure.in?(%w(weight volume)) && value <= 0
+      measure = "items"
+      unit_name = "items"
+      value = 1
+    end
 
     product.variant_unit = measure
     product.variant_unit_name = unit_name if measure == "items"
     product.variant_unit_scale = unit_scale
-    product.unit_value = quantity.value.to_f * unit_scale
+    product.unit_value = value
   end
 
   # Map DFC units to OFN fields:
diff --git a/engines/dfc_provider/spec/fixtures/files/product.GET.json b/engines/dfc_provider/spec/fixtures/files/product.GET.json
new file mode 100644
index 0000000000..ef8c076da2
--- /dev/null
+++ b/engines/dfc_provider/spec/fixtures/files/product.GET.json
@@ -0,0 +1,38 @@
+{
+  "@context": "https://www.datafoodconsortium.org",
+  "@graph": [
+    {
+      "@id": "_:b433",
+      "@type": "dfc-b:QuantitativeValue",
+      "dfc-b:hasUnit": "dfc-m:Kilogram",
+      "dfc-b:value": "0"
+    },
+    {
+      "@id": "_:b434",
+      "@type": "dfc-b:Price",
+      "dfc-b:VATrate": "1",
+      "dfc-b:hasUnit": "dfc-m:Euro",
+      "dfc-b:value": "12.06"
+    },
+    {
+      "@id": "https://example-producer.jcloud-ver-jpe.ik-server.com/api/dfc/Enterprises/5c21b9-95/SuppliedProducts/49055026544964",
+      "@type": "dfc-b:SuppliedProduct",
+      "dfc-b:hasQuantity": "_:b433",
+      "dfc-b:name": "Fillet Steak - 201g x 1 Steak",
+      "dfc-b:referencedBy": "https://example-producer.jcloud-ver-jpe.ik-server.com/api/dfc/Enterprises/5c21b9-95/SuppliedProducts/49055026544964/CatalogItem"
+    },
+    {
+      "@id": "https://example-producer.jcloud-ver-jpe.ik-server.com/api/dfc/Enterprises/5c21b9-95/SuppliedProducts/49055026544964/CatalogItem",
+      "@type": "dfc-b:CatalogItem",
+      "dfc-b:offeredThrough": "https://example-producer.jcloud-ver-jpe.ik-server.com/api/dfc/Enterprises/5c21b9-95/SuppliedProducts/49055026544964/Offer",
+      "dfc-b:stockLimitation": "11"
+    },
+    {
+      "@id": "https://example-producer.jcloud-ver-jpe.ik-server.com/api/dfc/Enterprises/5c21b9-95/SuppliedProducts/49055026544964/Offer",
+      "@type": "dfc-b:Offer",
+      "dfc-b:hasPrice": {
+        "@id": "_:b434"
+      }
+    }
+  ]
+}
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 855690e3d2..104f088338 100644
--- a/engines/dfc_provider/spec/services/supplied_product_builder_spec.rb
+++ b/engines/dfc_provider/spec/services/supplied_product_builder_spec.rb
@@ -84,6 +84,33 @@ RSpec.describe SuppliedProductBuilder do
     end
   end
 
+  describe ".store_product" do
+    let(:subject) { builder.store_product(product, supplier) }
+    let(:product) {
+      DfcIo.import(product_json).find do |subject|
+        subject.is_a? DataFoodConsortium::Connector::SuppliedProduct
+      end
+    }
+    let(:product_json) { ExampleJson.read("product.GET") }
+
+    before do
+      taxon.save!
+    end
+
+    it "stores a new Spree Product and Variant" do
+      expect { subject }.to change {
+        Spree::Product.count
+      }.by(1)
+
+      expect(subject).to be_a(Spree::Variant)
+      expect(subject).to be_valid
+      expect(subject).to be_persisted
+      expect(subject.name).to eq("Fillet Steak - 201g x 1 Steak")
+      expect(subject.variant_unit).to eq("items")
+      expect(subject.unit_value).to eq(1)
+    end
+  end
+
   describe ".import_product" do
     let(:supplied_product) do
       DfcProvider::SuppliedProduct.new(

From 260e7ba81773db152165116bc3b87cf129139908 Mon Sep 17 00:00:00 2001
From: Maikel Linke <maikel@email.org.au>
Date: Thu, 10 Oct 2024 16:33:24 +1100
Subject: [PATCH 09/11] Update products when importing them multiple times

Instead of creating a new variant every time.
---
 .../admin/dfc_product_imports_controller.rb          | 10 +++++++++-
 .../dfc_provider/supplied_products_controller.rb     |  5 +----
 .../app/services/supplied_product_builder.rb         |  9 +++++++++
 spec/system/admin/dfc_product_import_spec.rb         | 12 ++++++++----
 4 files changed, 27 insertions(+), 9 deletions(-)

diff --git a/app/controllers/admin/dfc_product_imports_controller.rb b/app/controllers/admin/dfc_product_imports_controller.rb
index 7def2815e4..c16031bf20 100644
--- a/app/controllers/admin/dfc_product_imports_controller.rb
+++ b/app/controllers/admin/dfc_product_imports_controller.rb
@@ -26,7 +26,15 @@ module Admin
       # * First step: import all products for given enterprise.
       # * Second step: render table and let user decide which ones to import.
       imported = graph.map do |subject|
-        SuppliedProductBuilder.store_product(subject, enterprise)
+        next unless subject.is_a? DataFoodConsortium::Connector::SuppliedProduct
+
+        existing_variant = enterprise.supplied_variants.linked_to(subject.semanticId)
+
+        if existing_variant
+          SuppliedProductBuilder.update_product(subject, existing_variant)
+        else
+          SuppliedProductBuilder.store_product(subject, enterprise)
+        end
       end
 
       @count = imported.compact.count
diff --git a/engines/dfc_provider/app/controllers/dfc_provider/supplied_products_controller.rb b/engines/dfc_provider/app/controllers/dfc_provider/supplied_products_controller.rb
index e2aaad9203..438c9c86a3 100644
--- a/engines/dfc_provider/app/controllers/dfc_provider/supplied_products_controller.rb
+++ b/engines/dfc_provider/app/controllers/dfc_provider/supplied_products_controller.rb
@@ -33,10 +33,7 @@ module DfcProvider
 
       return head :bad_request unless supplied_product
 
-      SuppliedProductBuilder.apply(supplied_product, variant)
-
-      variant.product.save!
-      variant.save!
+      SuppliedProductBuilder.update_product(supplied_product, variant)
     end
 
     private
diff --git a/engines/dfc_provider/app/services/supplied_product_builder.rb b/engines/dfc_provider/app/services/supplied_product_builder.rb
index e95e45c7d0..f3df73baf8 100644
--- a/engines/dfc_provider/app/services/supplied_product_builder.rb
+++ b/engines/dfc_provider/app/services/supplied_product_builder.rb
@@ -38,6 +38,15 @@ class SuppliedProductBuilder < DfcBuilder
     variant
   end
 
+  def self.update_product(supplied_product, variant)
+    apply(supplied_product, variant)
+
+    variant.product.save!
+    variant.save!
+
+    variant
+  end
+
   def self.import_variant(supplied_product, supplier)
     product = referenced_spree_product(supplied_product, supplier)
 
diff --git a/spec/system/admin/dfc_product_import_spec.rb b/spec/system/admin/dfc_product_import_spec.rb
index a22328650a..55780a683d 100644
--- a/spec/system/admin/dfc_product_import_spec.rb
+++ b/spec/system/admin/dfc_product_import_spec.rb
@@ -48,6 +48,10 @@ RSpec.describe "DFC Product Import" do
       refresh_token: ENV.fetch("OPENID_REFRESH_TOKEN"),
       updated_at: 1.day.ago,
     )
+    product_id =
+      "https://env-0105831.jcloud-ver-jpe.ik-server.com/api/dfc/Enterprises/test-hodmedod/SuppliedProducts/44519466467635"
+    linked_variant = source_product.variants.first
+    linked_variant.semantic_links << SemanticLink.new(semantic_id: product_id)
 
     visit admin_product_import_path
 
@@ -58,14 +62,14 @@ RSpec.describe "DFC Product Import" do
 
     expect {
       click_button "Import"
-    }.to change {
-      enterprise.supplied_products.count
-    }
+      linked_variant.reload
+    }.to change { enterprise.supplied_products.count }
+      .and change { linked_variant.display_name }
+      .and change { linked_variant.unit_value }
 
     expect(page).to have_content "Importing a DFC product catalog"
 
     product = Spree::Product.last
-
     expect(product.variants[0].semantic_links).to be_present
     expect(product.image).to be_present
   end

From cde757efbdc2e9b8dbef20968070cfad90f9dbd9 Mon Sep 17 00:00:00 2001
From: Maikel Linke <maikel@email.org.au>
Date: Thu, 10 Oct 2024 16:41:19 +1100
Subject: [PATCH 10/11] Split growing class

---
 .../app/services/image_builder.rb             | 21 +++++++++++++++++++
 .../app/services/supplied_product_builder.rb  | 20 +-----------------
 2 files changed, 22 insertions(+), 19 deletions(-)
 create mode 100644 engines/dfc_provider/app/services/image_builder.rb

diff --git a/engines/dfc_provider/app/services/image_builder.rb b/engines/dfc_provider/app/services/image_builder.rb
new file mode 100644
index 0000000000..df6141ce07
--- /dev/null
+++ b/engines/dfc_provider/app/services/image_builder.rb
@@ -0,0 +1,21 @@
+# frozen_string_literal: true
+
+require "private_address_check"
+require "private_address_check/tcpsocket_ext"
+
+class ImageBuilder < DfcBuilder
+  def self.import(image_link)
+    url = URI.parse(image_link)
+    filename = File.basename(image_link)
+
+    Spree::Image.new.tap do |image|
+      PrivateAddressCheck.only_public_connections do
+        image.attachment.attach(io: url.open, filename:)
+      end
+    end
+  rescue StandardError
+    # Any URL parsing or network error shouldn't impact the product import
+    # at all. Maybe we'll add UX for error handling later.
+    nil
+  end
+end
diff --git a/engines/dfc_provider/app/services/supplied_product_builder.rb b/engines/dfc_provider/app/services/supplied_product_builder.rb
index f3df73baf8..94d6afd611 100644
--- a/engines/dfc_provider/app/services/supplied_product_builder.rb
+++ b/engines/dfc_provider/app/services/supplied_product_builder.rb
@@ -1,8 +1,5 @@
 # frozen_string_literal: true
 
-require "private_address_check"
-require "private_address_check/tcpsocket_ext"
-
 class SuppliedProductBuilder < DfcBuilder
   def self.supplied_product(variant)
     id = urls.enterprise_supplied_product_url(
@@ -91,7 +88,7 @@ class SuppliedProductBuilder < DfcBuilder
       price: 0, # will be in DFC Offer
       supplier_id: supplier.id,
       primary_taxon_id: taxon(supplied_product).id,
-      image: image(supplied_product),
+      image: ImageBuilder.import(supplied_product.image),
     ).tap do |product|
       QuantitativeValueBuilder.apply(supplied_product.quantity, product)
       product.ensure_standard_variant
@@ -121,20 +118,5 @@ class SuppliedProductBuilder < DfcBuilder
     Spree::Taxon.find_by(dfc_id:) || Spree::Taxon.first
   end
 
-  def self.image(supplied_product)
-    url = URI.parse(supplied_product.image)
-    filename = File.basename(supplied_product.image)
-
-    Spree::Image.new.tap do |image|
-      PrivateAddressCheck.only_public_connections do
-        image.attachment.attach(io: url.open, filename:)
-      end
-    end
-  rescue StandardError
-    # Any URL parsing or network error shouldn't impact the product import
-    # at all. Maybe we'll add UX for error handling later.
-    nil
-  end
-
   private_class_method :product_type, :taxon
 end

From 86c91143b7212750d09fe3cf44bb1718aea511cf Mon Sep 17 00:00:00 2001
From: Maikel Linke <maikel@email.org.au>
Date: Thu, 10 Oct 2024 16:59:04 +1100
Subject: [PATCH 11/11] Update more variant data on import

---
 .../app/services/supplied_product_builder.rb          | 11 ++++++-----
 spec/system/admin/dfc_product_import_spec.rb          |  1 +
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/engines/dfc_provider/app/services/supplied_product_builder.rb b/engines/dfc_provider/app/services/supplied_product_builder.rb
index 94d6afd611..4eb488c4f2 100644
--- a/engines/dfc_provider/app/services/supplied_product_builder.rb
+++ b/engines/dfc_provider/app/services/supplied_product_builder.rb
@@ -53,14 +53,10 @@ class SuppliedProductBuilder < DfcBuilder
       end
     else
       product = import_product(supplied_product, supplier)
-      product.variants.first
+      product.variants.first.tap { |variant| apply(supplied_product, variant) }
     end.tap do |variant|
       link = supplied_product.semanticId
-      catalog_item = supplied_product&.catalogItems&.first
-      offer = catalog_item&.offers&.first
       variant.semantic_links.new(semantic_id: link) if link.present?
-      CatalogItemBuilder.apply_stock(catalog_item, variant)
-      OfferBuilder.apply(offer, variant)
     end
   end
 
@@ -102,6 +98,11 @@ class SuppliedProductBuilder < DfcBuilder
     variant.primary_taxon = taxon(supplied_product)
     QuantitativeValueBuilder.apply(supplied_product.quantity, variant.product)
     variant.unit_value = variant.product.unit_value
+
+    catalog_item = supplied_product&.catalogItems&.first
+    offer = catalog_item&.offers&.first
+    CatalogItemBuilder.apply_stock(catalog_item, variant)
+    OfferBuilder.apply(offer, variant)
   end
 
   def self.product_type(variant)
diff --git a/spec/system/admin/dfc_product_import_spec.rb b/spec/system/admin/dfc_product_import_spec.rb
index 55780a683d..af01620894 100644
--- a/spec/system/admin/dfc_product_import_spec.rb
+++ b/spec/system/admin/dfc_product_import_spec.rb
@@ -66,6 +66,7 @@ RSpec.describe "DFC Product Import" do
     }.to change { enterprise.supplied_products.count }
       .and change { linked_variant.display_name }
       .and change { linked_variant.unit_value }
+      .and change { linked_variant.price }
 
     expect(page).to have_content "Importing a DFC product catalog"