From 38f17547387521103c34e8ffe34cd204a6b1cba5 Mon Sep 17 00:00:00 2001 From: David Cook Date: Wed, 21 Jan 2026 16:57:29 +1100 Subject: [PATCH 1/4] Add spec This duplicates the 'returns data' spec above, but will be revelant in the following commits. --- .../spec/services/affiliate_sales_query_spec.rb | 9 +++++++++ 1 file changed, 9 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 7b30126378..e9598c1332 100644 --- a/engines/dfc_provider/spec/services/affiliate_sales_query_spec.rb +++ b/engines/dfc_provider/spec/services/affiliate_sales_query_spec.rb @@ -91,6 +91,15 @@ RSpec.describe AffiliateSalesQuery do ) end + it "returns data from variant if line item doesn't have it" do + labelled_row = query.label_row(query.data(order1.distributor).first) + + expect(labelled_row).to include( + product_name: "Tomatoes", + unit_name: "Tomatoes - Roma", + ) + end + context "with multiple orders" do let!(:order2) { create(:order_with_totals_and_distribution, :completed, variant: product.variants.first, From b98552003cbde2112c3e96708c03d9c739a2c082 Mon Sep 17 00:00:00 2001 From: Ashish Gaur Date: Wed, 16 Apr 2025 16:23:31 +0530 Subject: [PATCH 2/4] 13220 Add Product name in Order LineItem and update it during order creation 13220 Fixes affiliate sales spec 13220 Use before_create to update product name 13220 Fixes rubocop warnings 13220 Update product_name in line_item in specs 13220 Fix before_create lint 13220 Add spec for checking product_name is not set in reports 13220 Fixes rubocop issue 13220 Add migrations for updating the existing line items 13220 Fixing lint issues 13220 Set product_name in line_item before doing validation 13220 Fix linter issues 13220 Fixes spec 13220 Fixes linter issues 13220 Review comments 13220 Review comments 13220 Add default product name 13220 Use product_name instead of variant product name when using line item 13220 Fix specs 13220 Revert change in affiliate_sales_data_spec CL-13220 Store variant name in line_item 13220 Default variant name to original variant's full name for line_items 13220 Add missing frozen string literal 13220 Add spec for full_variant_name 13220 Remove UpdateProductNameInLineItems and AddNotNullToProductNameInLineItems migrations 13220 Remove presence validation for product_name 13220 Use full_product_name which defaults to variant product name if empty --- app/models/spree/line_item.rb | 26 +++++++++++++++++++ app/services/line_item_syncer.rb | 2 +- ...16074033_add_product_name_to_line_items.rb | 7 +++++ ...13110052_add_variant_name_to_line_items.rb | 7 +++++ db/schema.rb | 2 ++ lib/reporting/reports/bulk_coop/base.rb | 4 +-- .../reports/orders_and_fulfillment/base.rb | 4 +-- ...er_cycle_supplier_totals_by_distributor.rb | 2 +- spec/factories/line_item_factory.rb | 1 + ...order_cycle_customer_totals_report_spec.rb | 2 +- ...plier_totals_by_distributor_report_spec.rb | 13 +++++----- ...rders_cycle_supplier_totals_report_spec.rb | 23 +++++++++++++--- spec/models/spree/line_item_spec.rb | 11 ++++++++ spec/services/orders/sync_service_spec.rb | 6 ++--- 14 files changed, 91 insertions(+), 19 deletions(-) create mode 100644 db/migrate/20250416074033_add_product_name_to_line_items.rb create mode 100644 db/migrate/20250713110052_add_variant_name_to_line_items.rb diff --git a/app/models/spree/line_item.rb b/app/models/spree/line_item.rb index fed86cd802..c1312df1dd 100644 --- a/app/models/spree/line_item.rb +++ b/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 diff --git a/app/services/line_item_syncer.rb b/app/services/line_item_syncer.rb index 010312fef4..fb2af07b13 100644 --- a/app/services/line_item_syncer.rb +++ b/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 diff --git a/db/migrate/20250416074033_add_product_name_to_line_items.rb b/db/migrate/20250416074033_add_product_name_to_line_items.rb new file mode 100644 index 0000000000..92c256fdae --- /dev/null +++ b/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 diff --git a/db/migrate/20250713110052_add_variant_name_to_line_items.rb b/db/migrate/20250713110052_add_variant_name_to_line_items.rb new file mode 100644 index 0000000000..dffc7735d6 --- /dev/null +++ b/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 diff --git a/db/schema.rb b/db/schema.rb index 9ebfe7a970..ccbcc56233 100644 --- a/db/schema.rb +++ b/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 diff --git a/lib/reporting/reports/bulk_coop/base.rb b/lib/reporting/reports/bulk_coop/base.rb index 371713d676..bc7503d2f7 100644 --- a/lib/reporting/reports/bulk_coop/base.rb +++ b/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) diff --git a/lib/reporting/reports/orders_and_fulfillment/base.rb b/lib/reporting/reports/orders_and_fulfillment/base.rb index fef26eb194..8c083efe25 100644 --- a/lib/reporting/reports/orders_and_fulfillment/base.rb +++ b/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 diff --git a/lib/reporting/reports/orders_and_fulfillment/order_cycle_supplier_totals_by_distributor.rb b/lib/reporting/reports/orders_and_fulfillment/order_cycle_supplier_totals_by_distributor.rb index 039ed25e6d..83f5d31704 100644 --- a/lib/reporting/reports/orders_and_fulfillment/order_cycle_supplier_totals_by_distributor.rb +++ b/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, diff --git a/spec/factories/line_item_factory.rb b/spec/factories/line_item_factory.rb index f575fb0d3b..284c7440d8 100644 --- a/spec/factories/line_item_factory.rb +++ b/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 diff --git a/spec/lib/reports/orders_and_fulfillment/order_cycle_customer_totals_report_spec.rb b/spec/lib/reports/orders_and_fulfillment/order_cycle_customer_totals_report_spec.rb index f7e4231fc1..892a592afe 100644 --- a/spec/lib/reports/orders_and_fulfillment/order_cycle_customer_totals_report_spec.rb +++ b/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 diff --git a/spec/lib/reports/orders_and_fulfillment/order_cycle_supplier_totals_by_distributor_report_spec.rb b/spec/lib/reports/orders_and_fulfillment/order_cycle_supplier_totals_by_distributor_report_spec.rb index eb4f5661f3..cc1fe3c826 100644 --- a/spec/lib/reports/orders_and_fulfillment/order_cycle_supplier_totals_by_distributor_report_spec.rb +++ b/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") diff --git a/spec/lib/reports/orders_and_fulfillment/orders_cycle_supplier_totals_report_spec.rb b/spec/lib/reports/orders_and_fulfillment/orders_cycle_supplier_totals_report_spec.rb index 1fd1ae3841..a76c4448fc 100644 --- a/spec/lib/reports/orders_and_fulfillment/orders_cycle_supplier_totals_report_spec.rb +++ b/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 diff --git a/spec/models/spree/line_item_spec.rb b/spec/models/spree/line_item_spec.rb index 5ece7b8034..616c95a134 100644 --- a/spec/models/spree/line_item_spec.rb +++ b/spec/models/spree/line_item_spec.rb @@ -62,6 +62,17 @@ RSpec.describe Spree::LineItem do end end + context '#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 '.currency' do it 'returns the globally configured currency' do line_item.currency == 'USD' diff --git a/spec/services/orders/sync_service_spec.rb b/spec/services/orders/sync_service_spec.rb index 31fe0d4136..594e76ae11 100644 --- a/spec/services/orders/sync_service_spec.rb +++ b/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 From afdb04438646fa88a35c7ad27abbe4ee47d10615 Mon Sep 17 00:00:00 2001 From: David Cook Date: Mon, 5 Jan 2026 15:38:06 +1100 Subject: [PATCH 3/4] Load names from line item where available. I'm not sure why Arel complained about SQL in the SELECT clause (fields), but not the GROUP BY clause (key_fields). Maybe because it's not susceptible to user injection. I'm also not sure why I couldn't use the aliases defined in SELECT in the GROUP BY clause, but hey this seems to work. --- .../app/services/affiliate_sales_query.rb | 10 +++++----- .../spec/services/affiliate_sales_query_spec.rb | 16 +++++++++------- 2 files changed, 14 insertions(+), 12 deletions(-) diff --git a/engines/dfc_provider/app/services/affiliate_sales_query.rb b/engines/dfc_provider/app/services/affiliate_sales_query.rb index 155243745a..e3dd760979 100644 --- a/engines/dfc_provider/app/services/affiliate_sales_query.rb +++ b/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, 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 e9598c1332..047cbcab9c 100644 --- a/engines/dfc_provider/spec/services/affiliate_sales_query_spec.rb +++ b/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,20 +78,22 @@ 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) - pending "#13220 store product and variant names" expect(labelled_row).to include( product_name: "Tomatoes", - unit_name: "Tomatoes - Roma", + 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) expect(labelled_row).to include( @@ -131,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", From c00e7eeecfeea3a43e9e0cf9293a8d9f1c678866 Mon Sep 17 00:00:00 2001 From: David Cook Date: Wed, 21 Jan 2026 17:33:30 +1100 Subject: [PATCH 4/4] Add missing spec --- spec/models/spree/line_item_spec.rb | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/spec/models/spree/line_item_spec.rb b/spec/models/spree/line_item_spec.rb index 616c95a134..ce400a0462 100644 --- a/spec/models/spree/line_item_spec.rb +++ b/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,7 +62,7 @@ RSpec.describe Spree::LineItem do end end - context '#full_variant_name' do + 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 @@ -73,6 +73,17 @@ RSpec.describe Spree::LineItem do 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'