Merge pull request #13261 from ashishp91/13220-orders-fulfillment-report-product-name-issue

Fix Orders and fulfillment report fetches product name from product list (and not from order)

app/models/spree/line_item.rb

@@ -24,6 +24,8 @@ module Spree
     before_validation :copy_price
     before_validation :copy_tax_category
     before_validation :copy_dimensions
+    before_validation :copy_product_name, on: :create
+    before_validation :copy_variant_name, on: :create
 
     validates :quantity, numericality: {
       only_integer: true,
@@ -250,6 +252,18 @@ module Spree
       adjustments.enterprise_fee
     end
 
+    def full_variant_name
+      return variant_name if variant_name.present?
+
+      variant.full_name
+    end
+
+    def full_product_name
+      return product_name if product_name.present?
+
+      variant.product.name
+    end
+
     private
 
     def computed_weight_from_variant
@@ -274,6 +288,18 @@ module Spree
       order.create_tax_charge!
     end
 
+    def copy_product_name
+      return if variant.nil? || variant.product.nil?
+
+      self.product_name = variant.product.name
+    end
+
+    def copy_variant_name
+      return if variant.nil?
+
+      self.variant_name = variant.full_name
+    end
+
     def update_inventory_before_destroy
       # This is necessary before destroying the line item
       #   so that update_inventory will restore stock to the variant

app/services/line_item_syncer.rb

@@ -77,7 +77,7 @@ class LineItemSyncer
   end
 
   def add_order_update_issue(order, line_item)
-    issue_description = "#{line_item.product.name} - #{line_item.variant.full_name}"
+    issue_description = "#{line_item.product.name} - #{line_item.full_variant_name}"
     issue_description << " - #{stock_issue_description(line_item)}" if line_item.insufficient_stock?
     order_update_issues.add(order, issue_description)
   end

db/migrate/20250416074033_add_product_name_to_line_items.rb

@@ -0,0 +1,7 @@
+# frozen_string_literal: true
+
+class AddProductNameToLineItems < ActiveRecord::Migration[7.0]
+  def change
+    add_column :spree_line_items, :product_name, :string
+  end
+end

db/migrate/20250713110052_add_variant_name_to_line_items.rb

@@ -0,0 +1,7 @@
+# frozen_string_literal: true
+
+class AddVariantNameToLineItems < ActiveRecord::Migration[7.0]
+  def change
+    add_column :spree_line_items, :variant_name, :string
+  end
+end

db/schema.rb

@@ -579,6 +579,8 @@ ActiveRecord::Schema[7.1].define(version: 2025_11_26_005628) do
     t.decimal "width", precision: 8, scale: 2
     t.decimal "depth", precision: 8, scale: 2
     t.string "unit_presentation"
+    t.string "product_name"
+    t.string "variant_name"
     t.index ["order_id"], name: "index_line_items_on_order_id"
     t.index ["variant_id"], name: "index_line_items_on_variant_id"
   end

engines/dfc_provider/app/services/affiliate_sales_query.rb

@@ -14,7 +14,7 @@ class AffiliateSalesQuery
           },
         )
         .group(key_fields)
-        .pluck(fields)
+        .pluck(Arel.sql(fields))
     end
 
     # Create a hash with labels for an array of data points:
@@ -48,8 +48,8 @@ class AffiliateSalesQuery
 
     def fields
       <<~SQL.squish
-        spree_products.name AS product_name,
-        spree_variants.display_name AS unit_name,
+        COALESCE(spree_line_items.product_name, spree_products.name) AS product_name,
+        COALESCE(spree_line_items.variant_name, spree_variants.display_name) AS unit_name,
         spree_variants.variant_unit AS unit_type,
         spree_variants.unit_value AS units,
         spree_variants.unit_presentation,
@@ -65,8 +65,8 @@ class AffiliateSalesQuery
 
     def key_fields
       <<~SQL.squish
-        product_name,
-        unit_name,
+        COALESCE(spree_line_items.product_name, spree_products.name),
+        COALESCE(spree_line_items.variant_name, spree_variants.display_name),
         unit_type,
         units,
         spree_variants.unit_presentation,

engines/dfc_provider/spec/services/affiliate_sales_query_spec.rb

@@ -63,7 +63,7 @@ RSpec.describe AffiliateSalesQuery do
 
       expect(labelled_row).to include(
         product_name: "Tomatoes",
-        unit_name: "Tomatoes - Roma",
+        unit_name: "Tomatoes - Roma (1kg)",
         unit_type: "weight",
         units: 1000.to_f,
         unit_presentation: "1kg",
@@ -78,16 +78,27 @@ RSpec.describe AffiliateSalesQuery do
 
     it "returns data stored in line item at time of order" do
       # Records are updated after the orders are created
-      product.update! name: "Tomatoes Updated"
-      variant1.update! display_name: "Tomatoes - Updated Roma", price: 11
+      product.update! name: "Tommy toes"
+      variant1.update! display_name: "Tommy toes - Roma", price: 11
+
+      labelled_row = query.label_row(query.data(order1.distributor).first)
+
+      expect(labelled_row).to include(
+        product_name: "Tomatoes",
+        unit_name: "Tomatoes - Roma (1kg)",
+        price: 10.to_d, # this price is hardcoded in the line item factory.
+      )
+    end
+
+    it "returns data from variant if line item doesn't have it" do
+      # Old line item records (before migration 20250713110052) don't have these values stored
+      order1.line_items.first.update! product_name: nil, variant_name: nil
 
       labelled_row = query.label_row(query.data(order1.distributor).first)
 
-      pending "#13220 store product and variant names"
       expect(labelled_row).to include(
         product_name: "Tomatoes",
         unit_name: "Tomatoes - Roma",
-        price: 10.to_d, # this price is hardcoded in the line item factory.
       )
     end
 
@@ -122,12 +133,12 @@ RSpec.describe AffiliateSalesQuery do
 
           expect(labelled_data).to include a_hash_including(
             product_name: "Tomatoes",
-            unit_name: "Tomatoes - Roma",
+            unit_name: "Tomatoes - Roma (1kg)",
             quantity_sold: 1,
           )
           expect(labelled_data).to include a_hash_including(
             product_name: "Tomatoes",
-            unit_name: "Tomatoes - Cherry",
+            unit_name: "Tomatoes - Cherry (500g)",
             quantity_sold: 1,
             units: 500,
             unit_presentation: "500g",

lib/reporting/reports/bulk_coop/base.rb

@@ -85,7 +85,7 @@ module Reporting
         end
 
         def product_name(line_items)
-          line_items.first.product.name
+          line_items.first.full_product_name
         end
 
         def remainder(line_items)
@@ -118,7 +118,7 @@ module Reporting
         end
 
         def variant_product_name(line_items)
-          line_items.first.variant.product.name
+          line_items.first.full_product_name
         end
 
         def weight_from_unit_value(line_items)

lib/reporting/reports/orders_and_fulfillment/base.rb

@@ -41,7 +41,7 @@ module Reporting
         end
 
         def variant_name
-          proc { |line_items| line_items.first.variant.full_name }
+          proc { |line_items| line_items.first.full_variant_name }
         end
 
         def variant_sku
@@ -57,7 +57,7 @@ module Reporting
         end
 
         def product_name
-          proc { |line_items| line_items.first.variant.product.name }
+          proc { |line_items| line_items.first.full_product_name }
         end
 
         def product_tax_category

lib/reporting/reports/orders_and_fulfillment/order_cycle_supplier_totals_by_distributor.rb

@@ -25,7 +25,7 @@ module Reporting
             },
             {
               group_by: proc { |line_items, _row| line_items.first.variant },
-              sort_by: proc { |variant| variant.product.name }
+              sort_by: proc { |variant| variant.line_items.first.full_product_name }
             },
             {
               group_by: :hub,

spec/factories/line_item_factory.rb

@@ -6,6 +6,7 @@ FactoryBot.define do
     price { BigDecimal('10.00') }
     order
     variant
+    product_name { variant.product.name }
   end
 
   factory :line_item_with_shipment, parent: :line_item do

spec/lib/reports/orders_and_fulfillment/order_cycle_customer_totals_report_spec.rb

@@ -28,7 +28,7 @@ RSpec.describe Reporting::Reports::OrdersAndFulfillment::OrderCycleCustomerTotal
         completed_at: order_date,
       ).tap do |order|
         order.line_items[0].variant.supplier.update(name: "Apple Farmer")
-        order.line_items[0].product.update(name: "Apples")
+        order.line_items[0].update(product_name: "Apples")
         order.line_items[0].variant.update(sku: "APP")
       end
     end

spec/lib/reports/orders_and_fulfillment/order_cycle_supplier_totals_by_distributor_report_spec.rb

@@ -32,9 +32,10 @@ module Reporting
           end
 
           it "lists products sorted by name" do
-            order.line_items[0].variant.product.update(name: "Cucumber")
-            order.line_items[1].variant.product.update(name: "Apple")
-            order.line_items[2].variant.product.update(name: "Banane")
+            order.line_items[0].update(product_name: "Cucumber")
+            order.line_items[1].update(product_name: "Apple")
+            order.line_items[2].update(product_name: "Banane")
+
             product_names = report.rows.map(&:product).compact_blank
             expect(product_names).to eq(["Apple", "Banane", "Cucumber"])
           end
@@ -79,9 +80,9 @@ module Reporting
           end
 
           it "lists products sorted by name" do
-            order.line_items[0].variant.product.update(name: "Cucumber")
-            order.line_items[1].variant.product.update(name: "Apple")
-            order.line_items[2].variant.product.update(name: "Banane")
+            order.line_items[0].update(product_name: "Cucumber")
+            order.line_items[1].update(product_name: "Apple")
+            order.line_items[2].update(product_name: "Banane")
             product_names = report.rows.map(&:product).compact_blank
             # only the supplier's variant is displayed
             expect(product_names).to include("Cucumber")

spec/lib/reports/orders_and_fulfillment/orders_cycle_supplier_totals_report_spec.rb

@@ -23,14 +23,14 @@ RSpec.describe Reporting::Reports::OrdersAndFulfillment::OrderCycleSupplierTotal
     report.table_rows
   end
 
+  let(:item) { order.line_items.first }
+  let(:variant) { item.variant }
+
   it "generates the report" do
     expect(report_table.length).to eq(1)
   end
 
   describe "total_units column" do
-    let(:item) { order.line_items.first }
-    let(:variant) { item.variant }
-
     it "contains a sum of total items" do
       variant.update!(variant_unit: "items", variant_unit_name: "bottle", unit_value: 6) # six-pack
       item.update!(final_weight_volume: nil) # reset unit information
@@ -178,4 +178,21 @@ RSpec.describe Reporting::Reports::OrdersAndFulfillment::OrderCycleSupplierTotal
     expect(last_column_title).to eq "SKU"
     expect(first_row_last_column_value).to eq variant_sku
   end
+
+  it "doesn't update product name in report" do
+    variant_sku = order.line_items.first.variant.sku
+    last_column_title = table_headers[-3]
+    first_row_last_column_value = report_table.first[-3]
+
+    expect(last_column_title).to eq "SKU"
+    expect(first_row_last_column_value).to eq variant_sku
+
+    expect(report_table.first[1]).to eq(variant.product.name)
+    product_name = variant.product.name
+    variant.product.update(name: "#{product_name} Updated")
+
+    new_report = described_class.new(current_user, params)
+
+    expect(new_report.table_rows.first[1]).to eq(product_name)
+  end
 end

spec/models/spree/line_item_spec.rb

@@ -14,7 +14,7 @@ RSpec.describe Spree::LineItem do
     it { is_expected.to have_many(:adjustments) }
   end
 
-  context '#save' do
+  describe '#save' do
     it 'should update inventory, totals, and tax' do
       # Regression check for Spree #1481
       expect(line_item.order).to receive(:create_tax_charge!)
@@ -23,7 +23,7 @@ RSpec.describe Spree::LineItem do
     end
   end
 
-  context '#destroy' do
+  describe '#destroy' do
     # Regression test for Spree #1481
     it "applies tax adjustments" do
       expect(line_item.order).to receive(:create_tax_charge!)
@@ -42,7 +42,7 @@ RSpec.describe Spree::LineItem do
   end
 
   # Test for Spree #3391
-  context '#copy_price' do
+  describe '#copy_price' do
     it "copies over a variant's prices" do
       line_item.price = nil
       line_item.currency = nil
@@ -54,7 +54,7 @@ RSpec.describe Spree::LineItem do
   end
 
   # Test for Spree #3481
-  context '#copy_tax_category' do
+  describe '#copy_tax_category' do
     it "copies over a variant's tax category" do
       line_item.tax_category = nil
       line_item.copy_tax_category
@@ -62,6 +62,28 @@ RSpec.describe Spree::LineItem do
     end
   end
 
+  describe '#full_variant_name' do
+    it "returns variant's full name" do
+      expect(line_item.full_variant_name).to eq(line_item.variant.full_name)
+    end
+
+    it "uses variant.full_name when variant_name is nil" do
+      line_item.variant_name = nil
+      expect(line_item.full_variant_name).to eq(line_item.variant.full_name)
+    end
+  end
+
+  describe '#full_product_name' do
+    it "returns product's full name" do
+      expect(line_item.full_product_name).to eq(line_item.product.name)
+    end
+
+    it "uses product.name when product_name is nil" do
+      line_item.product_name = nil
+      expect(line_item.full_product_name).to eq(line_item.product.name)
+    end
+  end
+
   describe '.currency' do
     it 'returns the globally configured currency' do
       line_item.currency == 'USD'

spec/services/orders/sync_service_spec.rb

@@ -438,7 +438,7 @@ RSpec.describe Orders::SyncService do
           expect(order.reload.total.to_f).to eq 59.97
           line_item = order.line_items.find_by(variant_id: sli.variant_id)
           expect(syncer.order_update_issues[order.id])
-            .to include "#{line_item.product.name} - #{line_item.variant.full_name} - " \
+            .to include "#{line_item.product.name} - #{line_item.full_variant_name} - " \
                         "Insufficient stock available"
         end
 
@@ -453,7 +453,7 @@ RSpec.describe Orders::SyncService do
 
           line_item = order.line_items.find_by(variant_id: sli.variant_id)
           expect(syncer.order_update_issues[order.id])
-            .to include "#{line_item.product.name} - #{line_item.variant.full_name} - Out of Stock"
+            .to include "#{line_item.product.name} - #{line_item.full_variant_name} - Out of Stock"
         end
       end
     end
@@ -498,7 +498,7 @@ RSpec.describe Orders::SyncService do
           expect(changed_line_item.reload.quantity).to eq 2
           expect(order.reload.total.to_f).to eq 79.96
           expect(syncer.order_update_issues[order.id])
-            .to include "#{changed_line_item.product.name} - #{changed_line_item.variant.full_name}"
+            .to include "#{changed_line_item.product.name} - #{changed_line_item.full_variant_name}"
         end
       end
     end