From 1332051a6e52b8aeec26fb1683d04cd3ec2de512 Mon Sep 17 00:00:00 2001 From: David Cook Date: Tue, 3 Feb 2026 15:14:53 +1100 Subject: [PATCH 01/29] Move specs to relevant file These tests are about browsing products, not performing actions. Well, ok there's one about updating, which should probably go in the update file. But hey this is better than before. And admittedly the "Actions" file covers three different things, not just the actions menu. shrug. --- spec/system/admin/products_v3/actions_spec.rb | 102 +----------------- spec/system/admin/products_v3/index_spec.rb | 98 ++++++++++++++++- 2 files changed, 99 insertions(+), 101 deletions(-) diff --git a/spec/system/admin/products_v3/actions_spec.rb b/spec/system/admin/products_v3/actions_spec.rb index 383892ba86..161955279b 100644 --- a/spec/system/admin/products_v3/actions_spec.rb +++ b/spec/system/admin/products_v3/actions_spec.rb @@ -2,7 +2,7 @@ require "system_helper" -RSpec.describe 'As an enterprise user, I can manage my products' do +RSpec.describe 'As an enterprise user, I can perform actions on the products screen' do include AdminHelper include WebHelper include AuthenticationHelper @@ -19,18 +19,6 @@ RSpec.describe 'As an enterprise user, I can manage my products' do let(:categories_search_selector) { 'input[placeholder="Select category"]' } let(:tax_categories_search_selector) { 'input[placeholder="Search for tax categories"]' } - describe "with no products" do - before { visit admin_products_url } - it "can see the new product page" do - expect(page).to have_content "Bulk Edit Products" - expect(page).to have_text "No products found" - # displays buttons to add products with the correct links - expect(page).to have_link(class: "button", text: "New Product", href: "/admin/products/new") - expect(page).to have_link(class: "button", text: "Import multiple products", - href: admin_product_import_path) - end - end - describe "column selector" do let!(:product) { create(:simple_product) } @@ -105,8 +93,6 @@ RSpec.describe 'As an enterprise user, I can manage my products' do end end - describe "columns" - describe "Changing producers, category and tax category" do let!(:variant_a1) { product_a.variants.first.tap{ |v| @@ -527,90 +513,6 @@ RSpec.describe 'As an enterprise user, I can manage my products' do end end - context "as an enterprise manager" do - let(:supplier_managed1) { create(:supplier_enterprise, name: 'Supplier Managed 1') } - let(:supplier_managed2) { create(:supplier_enterprise, name: 'Supplier Managed 2') } - let(:supplier_unmanaged) { create(:supplier_enterprise, name: 'Supplier Unmanaged') } - let(:supplier_permitted) { create(:supplier_enterprise, name: 'Supplier Permitted') } - let(:distributor_managed) { create(:distributor_enterprise, name: 'Distributor Managed') } - let(:distributor_unmanaged) { create(:distributor_enterprise, name: 'Distributor Unmanaged') } - let!(:product_supplied) { create(:product, supplier_id: supplier_managed1.id, price: 10.0) } - let!(:product_not_supplied) { create(:product, supplier_id: supplier_unmanaged.id) } - let!(:product_supplied_permitted) { - create(:product, name: 'Product Permitted', supplier_id: supplier_permitted.id, price: 10.0) - } - let(:product_supplied_inactive) { - create(:product, supplier_id: supplier_managed1.id, price: 10.0) - } - - let!(:supplier_permitted_relationship) do - create(:enterprise_relationship, parent: supplier_permitted, child: supplier_managed1, - permissions_list: [:manage_products]) - end - - before do - enterprise_user = create(:user) - enterprise_user.enterprise_roles.build(enterprise: supplier_managed1).save - enterprise_user.enterprise_roles.build(enterprise: supplier_managed2).save - enterprise_user.enterprise_roles.build(enterprise: distributor_managed).save - - login_as enterprise_user - end - - it "shows only products that I supply" do - visit spree.admin_products_path - - # displays permitted product list only - expect(page).to have_selector row_containing_name(product_supplied.name) - expect(page).to have_selector row_containing_name(product_supplied_permitted.name) - expect(page).not_to have_selector row_containing_name(product_not_supplied.name) - end - - it "shows only suppliers that I manage or have permission to" do - visit spree.admin_products_path - - within row_containing_placeholder(product_supplied.name) do - expect(page).to have_select( - '_products_0_variants_attributes_0_supplier_id', - options: [ - 'Select producer', - supplier_managed1.name, supplier_managed2.name, supplier_permitted.name - ], selected: supplier_managed1.name - ) - end - - within row_containing_placeholder(product_supplied_permitted.name) do - expect(page).to have_select( - '_products_1_variants_attributes_0_supplier_id', - options: [ - 'Select producer', - supplier_managed1.name, supplier_managed2.name, supplier_permitted.name - ], selected: supplier_permitted.name - ) - end - end - - it "shows inactive products that I supply" do - product_supplied_inactive - - visit spree.admin_products_path - - expect(page).to have_selector row_containing_name(product_supplied_inactive.name) - end - - it "allows me to update a product" do - visit spree.admin_products_path - - within row_containing_name(product_supplied.name) do - fill_in "Name", with: "Pommes" - end - click_button "Save changes" - - expect(page).to have_content "Changes saved" - expect(page).to have_selector row_containing_name("Pommes") - end - end - def open_action_menu page.find(".vertical-ellipsis-menu").click end @@ -618,4 +520,4 @@ RSpec.describe 'As an enterprise user, I can manage my products' do def close_action_menu page.find("div#content").click end -end +end \ No newline at end of file diff --git a/spec/system/admin/products_v3/index_spec.rb b/spec/system/admin/products_v3/index_spec.rb index d0f3d92337..3658dbc726 100644 --- a/spec/system/admin/products_v3/index_spec.rb +++ b/spec/system/admin/products_v3/index_spec.rb @@ -2,7 +2,7 @@ require "system_helper" -RSpec.describe 'As an enterprise user, I can manage my products' do +RSpec.describe 'As an enterprise user, I can browse my products' do include AdminHelper include WebHelper include AuthenticationHelper @@ -19,6 +19,18 @@ RSpec.describe 'As an enterprise user, I can manage my products' do let(:categories_search_selector) { 'input[placeholder="Search for categories"]' } let(:tax_categories_search_selector) { 'input[placeholder="Search for tax categories"]' } + describe "with no products" do + before { visit admin_products_url } + it "can see the new product page" do + expect(page).to have_content "Bulk Edit Products" + expect(page).to have_text "No products found" + # displays buttons to add products with the correct links + expect(page).to have_link(class: "button", text: "New Product", href: "/admin/products/new") + expect(page).to have_link(class: "button", text: "Import multiple products", + href: admin_product_import_path) + end + end + describe "listing" do let!(:p1) { create(:product, name: "Product1") } let!(:p2) { create(:product, name: "Product2") } @@ -463,4 +475,88 @@ RSpec.describe 'As an enterprise user, I can manage my products' do end end end + + context "as an enterprise manager" do + let(:supplier_managed1) { create(:supplier_enterprise, name: 'Supplier Managed 1') } + let(:supplier_managed2) { create(:supplier_enterprise, name: 'Supplier Managed 2') } + let(:supplier_unmanaged) { create(:supplier_enterprise, name: 'Supplier Unmanaged') } + let(:supplier_permitted) { create(:supplier_enterprise, name: 'Supplier Permitted') } + let(:distributor_managed) { create(:distributor_enterprise, name: 'Distributor Managed') } + let(:distributor_unmanaged) { create(:distributor_enterprise, name: 'Distributor Unmanaged') } + let!(:product_supplied) { create(:product, supplier_id: supplier_managed1.id, price: 10.0) } + let!(:product_not_supplied) { create(:product, supplier_id: supplier_unmanaged.id) } + let!(:product_supplied_permitted) { + create(:product, name: 'Product Permitted', supplier_id: supplier_permitted.id, price: 10.0) + } + let(:product_supplied_inactive) { + create(:product, supplier_id: supplier_managed1.id, price: 10.0) + } + + let!(:supplier_permitted_relationship) do + create(:enterprise_relationship, parent: supplier_permitted, child: supplier_managed1, + permissions_list: [:manage_products]) + end + + before do + enterprise_user = create(:user) + enterprise_user.enterprise_roles.build(enterprise: supplier_managed1).save + enterprise_user.enterprise_roles.build(enterprise: supplier_managed2).save + enterprise_user.enterprise_roles.build(enterprise: distributor_managed).save + + login_as enterprise_user + end + + it "shows only products that I supply" do + visit spree.admin_products_path + + # displays permitted product list only + expect(page).to have_selector row_containing_name(product_supplied.name) + expect(page).to have_selector row_containing_name(product_supplied_permitted.name) + expect(page).not_to have_selector row_containing_name(product_not_supplied.name) + end + + it "shows only suppliers that I manage or have permission to" do + visit spree.admin_products_path + + within row_containing_placeholder(product_supplied.name) do + expect(page).to have_select( + '_products_0_variants_attributes_0_supplier_id', + options: [ + 'Select producer', + supplier_managed1.name, supplier_managed2.name, supplier_permitted.name + ], selected: supplier_managed1.name + ) + end + + within row_containing_placeholder(product_supplied_permitted.name) do + expect(page).to have_select( + '_products_1_variants_attributes_0_supplier_id', + options: [ + 'Select producer', + supplier_managed1.name, supplier_managed2.name, supplier_permitted.name + ], selected: supplier_permitted.name + ) + end + end + + it "shows inactive products that I supply" do + product_supplied_inactive + + visit spree.admin_products_path + + expect(page).to have_selector row_containing_name(product_supplied_inactive.name) + end + + it "allows me to update a product" do + visit spree.admin_products_path + + within row_containing_name(product_supplied.name) do + fill_in "Name", with: "Pommes" + end + click_button "Save changes" + + expect(page).to have_content "Changes saved" + expect(page).to have_selector row_containing_name("Pommes") + end + end end From 0f3b29954427e4f0c3fa3563b8c68a9700e8cd0b Mon Sep 17 00:00:00 2001 From: David Cook Date: Tue, 10 Feb 2026 11:30:57 +1100 Subject: [PATCH 02/29] Clean up spec There's no "COPY OF" product in the spec setup, so we don't need to check that it's not there. (unless maybe we added that to the product factory, but it seems unlikely). Also we can use helper method. --- spec/system/admin/products_v3/actions_spec.rb | 16 ++-------------- 1 file changed, 2 insertions(+), 14 deletions(-) diff --git a/spec/system/admin/products_v3/actions_spec.rb b/spec/system/admin/products_v3/actions_spec.rb index 161955279b..53079771df 100644 --- a/spec/system/admin/products_v3/actions_spec.rb +++ b/spec/system/admin/products_v3/actions_spec.rb @@ -246,24 +246,12 @@ RSpec.describe 'As an enterprise user, I can perform actions on the products scr describe "Cloning product" do it "shows the cloned product on page when clicked on the cloned option" do - # TODO, variant supplier missing, needs to be copied from variant and not product - within "table.products" do - # Gather input values, because page.content doesn't include them. - input_content = page.find_all('input[type=text]').map(&:value).join - - # Products does not include the cloned product. - expect(input_content).not_to match /COPY OF Apples/ - end - click_product_clone "Apples" expect(page).to have_content "Successfully cloned the product" within "table.products" do - # Gather input values, because page.content doesn't include them. - input_content = page.find_all('input[type=text]').map(&:value).join - - # Products include the cloned product. - expect(input_content).to match /COPY OF Apples/ + # Product list includes the cloned product. + expect(all_input_values).to match /COPY OF Apples/ end end end From e565243ce420b3da0cfd3ab1bd5e863c8c1e2c5f Mon Sep 17 00:00:00 2001 From: David Cook Date: Tue, 24 Feb 2026 15:57:44 +1100 Subject: [PATCH 03/29] Remove unnecessary before action Each context has different setup and needs to load the page after setup. --- spec/system/admin/products_v3/index_spec.rb | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/spec/system/admin/products_v3/index_spec.rb b/spec/system/admin/products_v3/index_spec.rb index 3658dbc726..e74517262f 100644 --- a/spec/system/admin/products_v3/index_spec.rb +++ b/spec/system/admin/products_v3/index_spec.rb @@ -35,11 +35,9 @@ RSpec.describe 'As an enterprise user, I can browse my products' do let!(:p1) { create(:product, name: "Product1") } let!(:p2) { create(:product, name: "Product2") } - before do - visit admin_products_url - end - it "displays a list of products" do + visit admin_products_path + within ".products" do # displays table header expect(page).to have_selector "th", text: "Name" From bd01b5f113ce19fdd3de2415f92c6e0682cc0007 Mon Sep 17 00:00:00 2001 From: David Cook Date: Mon, 9 Feb 2026 15:30:33 +1100 Subject: [PATCH 04/29] Add 'create sourced variant' option in variant actions menu For now, we will only be able to create sourced variant from variants that are visible to us (variants that we manage) In a later commit I will hide the option if you can't use it --- .../admin/products_v3/_variant_row.html.haml | 2 + config/locales/en.yml | 1 + spec/system/admin/products_v3/actions_spec.rb | 54 ++++++++++++++++++- 3 files changed, 56 insertions(+), 1 deletion(-) diff --git a/app/views/admin/products_v3/_variant_row.html.haml b/app/views/admin/products_v3/_variant_row.html.haml index 54bf2ca861..79340b0f3e 100644 --- a/app/views/admin/products_v3/_variant_row.html.haml +++ b/app/views/admin/products_v3/_variant_row.html.haml @@ -88,6 +88,8 @@ = render(VerticalEllipsisMenuComponent.new) do - if variant.persisted? = link_to t('admin.products_page.actions.edit'), edit_admin_product_variant_path(variant.product, variant) + / TODO: only show if have permission. need to load permissions efficiently please. maybe the PErmissions object can cache result for each enterprise. maybe we preload it with the product query. + = link_to t('admin.products_page.actions.create_sourced_variant'), "TODO" # see next commit - if variant.product.variants.size > 1 %a{ "data-controller": "modal-link", "data-action": "click->modal-link#setModalDataSetOnConfirm click->modal-link#open", "data-modal-link-target-value": "variant-delete-modal", "class": "delete", diff --git a/config/locales/en.yml b/config/locales/en.yml index fffe128a73..f2f0e77c7c 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -717,6 +717,7 @@ en: delete: Delete remove: Remove preview: Preview + create_sourced_variant: Create sourced variant image: edit: Edit product_preview: diff --git a/spec/system/admin/products_v3/actions_spec.rb b/spec/system/admin/products_v3/actions_spec.rb index 53079771df..006bce3132 100644 --- a/spec/system/admin/products_v3/actions_spec.rb +++ b/spec/system/admin/products_v3/actions_spec.rb @@ -272,6 +272,58 @@ RSpec.describe 'As an enterprise user, I can perform actions on the products scr end end + describe "Create sourced variant" do + let!(:variant) { + create(:variant, display_name: "My box", supplier: producer) + } + let!(:other_producer) { create(:supplier_enterprise) } + let!(:other_variant) { + create(:variant, display_name: "My friends box", supplier: other_producer) + } + let!(:enterprise_relationship) { + # Other producer grants me access to manage their variant + create(:enterprise_relationship, parent: other_producer, child: producer, + permissions_list: [:manage_products]) + } + + before do + visit admin_products_url + end + + context "with create_sourced_variants permission for my, and other's variants" do + it "shows an option to create sourced variant" do + create(:enterprise_relationship, parent: producer, child: producer, + permissions_list: [:create_sourced_variants]) + enterprise_relationship.permissions.create! name: :create_sourced_variants + + within row_containing_name("My box") do + page.find(".vertical-ellipsis-menu").click + expect(page).to have_link "Create sourced variant" # , href: admin_clone_product_path(product_a) + end + + within row_containing_name("My friends box") do + page.find(".vertical-ellipsis-menu").click + expect(page).to have_link "Create sourced variant" # , href: admin_clone_product_path(product_a) + end + end + end + + context "without create_sourced_variants permission" do + it "does not show the option in the menu" do + pending "TODO: hide option if you can't use it." + within row_containing_name("My box") do + page.find(".vertical-ellipsis-menu").click + expect(page).not_to have_link "Create sourced variant" + end + + within row_containing_name("My friends box") do + page.find(".vertical-ellipsis-menu").click + expect(page).not_to have_link "Create sourced variant" + end + end + end + end + describe "delete" do let!(:product_a) { create(:simple_product, name: "Apples", sku: "APL-00") } let(:delete_option_selector) { "a[data-controller='modal-link'].delete" } @@ -508,4 +560,4 @@ RSpec.describe 'As an enterprise user, I can perform actions on the products scr def close_action_menu page.find("div#content").click end -end \ No newline at end of file +end From 6fe2357ca0886fd53cdafe1dd26e9f028443d503 Mon Sep 17 00:00:00 2001 From: David Cook Date: Tue, 3 Feb 2026 12:54:04 +1100 Subject: [PATCH 05/29] Add enterprise permission create_sourced_variants --- .../admin/services/enterprise_relationships.js.coffee | 2 ++ config/locales/en.yml | 1 + .../services/enterprise_relationships_spec.js.coffee | 1 + spec/system/admin/enterprise_relationships_spec.rb | 9 +++++++-- 4 files changed, 11 insertions(+), 2 deletions(-) diff --git a/app/assets/javascripts/admin/services/enterprise_relationships.js.coffee b/app/assets/javascripts/admin/services/enterprise_relationships.js.coffee index 9d4668a8cc..1e57018e99 100644 --- a/app/assets/javascripts/admin/services/enterprise_relationships.js.coffee +++ b/app/assets/javascripts/admin/services/enterprise_relationships.js.coffee @@ -6,6 +6,7 @@ angular.module("ofn.admin").factory 'EnterpriseRelationships', ($http, enterpris 'manage_products' 'edit_profile' 'create_variant_overrides' + 'create_sourced_variants' ] constructor: -> @@ -30,3 +31,4 @@ angular.module("ofn.admin").factory 'EnterpriseRelationships', ($http, enterpris when "manage_products" then t('js.services.manage_products') when "edit_profile" then t('js.services.edit_profile') when "create_variant_overrides" then t('js.services.add_products_to_inventory') + when "create_sourced_variants" then t('js.services.create_sourced_variants') diff --git a/config/locales/en.yml b/config/locales/en.yml index f2f0e77c7c..4d84655ab1 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -3902,6 +3902,7 @@ en: manage_products: "manage products" edit_profile: "edit profile" add_products_to_inventory: "add products to inventory" + create_sourced_variants: "create sourced variants" resources: could_not_delete_customer: 'Could not delete customer' product_import: diff --git a/spec/javascripts/unit/admin/services/enterprise_relationships_spec.js.coffee b/spec/javascripts/unit/admin/services/enterprise_relationships_spec.js.coffee index 96ac4a8905..4a5907c681 100644 --- a/spec/javascripts/unit/admin/services/enterprise_relationships_spec.js.coffee +++ b/spec/javascripts/unit/admin/services/enterprise_relationships_spec.js.coffee @@ -16,3 +16,4 @@ describe "enterprise relationships", -> expect(EnterpriseRelationships.permission_presentation("manage_products")).toEqual "manage products" expect(EnterpriseRelationships.permission_presentation("edit_profile")).toEqual "edit profile" expect(EnterpriseRelationships.permission_presentation("create_variant_overrides")).toEqual "add products to inventory" + expect(EnterpriseRelationships.permission_presentation("create_sourced_variants")).toEqual "create sourced variants" diff --git a/spec/system/admin/enterprise_relationships_spec.rb b/spec/system/admin/enterprise_relationships_spec.rb index 1e86c9d229..03584eb516 100644 --- a/spec/system/admin/enterprise_relationships_spec.rb +++ b/spec/system/admin/enterprise_relationships_spec.rb @@ -47,18 +47,23 @@ create(:enterprise) uncheck 'to manage products' check 'to edit profile' check 'to add products to inventory' + check 'to create sourced variants' select2_select 'Two', from: 'enterprise_relationship_child_id' click_button 'Create' # Wait for row to appear since have_relationship doesn't wait expect(page).to have_selector 'tr', count: 2 + # Permissions appear.. in a different order for some reason. expect_relationship_with_permissions e1, e2, ['to add to order cycle', - 'to add products to inventory', 'to edit profile'] + 'to create sourced variants', + 'to add products to inventory', + 'to edit profile'] er = EnterpriseRelationship.where(parent_id: e1, child_id: e2).first expect(er).to be_present expect(er.permissions.map(&:name)).to match_array ['add_to_order_cycle', 'edit_profile', - 'create_variant_overrides'] + 'create_variant_overrides', + 'create_sourced_variants'] end it "attempting to create a relationship with invalid data" do From 766bedb773b489f81f759b6acde65192e637e0e8 Mon Sep 17 00:00:00 2001 From: David Cook Date: Tue, 10 Feb 2026 19:34:15 +1100 Subject: [PATCH 06/29] Label this feature as 'beta' MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The permission is effectively the feature toggle. Users can choose to use it, but shouldn't expect it all to work perfectly yet. When it's considered full featured, we just need to update the translation. Hm... I hope that's not too painful.🤞 --- config/locales/en.yml | 2 +- .../unit/admin/services/enterprise_relationships_spec.js.coffee | 2 +- spec/system/admin/enterprise_relationships_spec.rb | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/config/locales/en.yml b/config/locales/en.yml index 4d84655ab1..352e0875de 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -3902,7 +3902,7 @@ en: manage_products: "manage products" edit_profile: "edit profile" add_products_to_inventory: "add products to inventory" - create_sourced_variants: "create sourced variants" + create_sourced_variants: "create sourced variants [BETA]" resources: could_not_delete_customer: 'Could not delete customer' product_import: diff --git a/spec/javascripts/unit/admin/services/enterprise_relationships_spec.js.coffee b/spec/javascripts/unit/admin/services/enterprise_relationships_spec.js.coffee index 4a5907c681..c25764b986 100644 --- a/spec/javascripts/unit/admin/services/enterprise_relationships_spec.js.coffee +++ b/spec/javascripts/unit/admin/services/enterprise_relationships_spec.js.coffee @@ -16,4 +16,4 @@ describe "enterprise relationships", -> expect(EnterpriseRelationships.permission_presentation("manage_products")).toEqual "manage products" expect(EnterpriseRelationships.permission_presentation("edit_profile")).toEqual "edit profile" expect(EnterpriseRelationships.permission_presentation("create_variant_overrides")).toEqual "add products to inventory" - expect(EnterpriseRelationships.permission_presentation("create_sourced_variants")).toEqual "create sourced variants" + expect(EnterpriseRelationships.permission_presentation("create_sourced_variants")).toEqual "create sourced variants [BETA]" diff --git a/spec/system/admin/enterprise_relationships_spec.rb b/spec/system/admin/enterprise_relationships_spec.rb index 03584eb516..3f0525e668 100644 --- a/spec/system/admin/enterprise_relationships_spec.rb +++ b/spec/system/admin/enterprise_relationships_spec.rb @@ -56,7 +56,7 @@ create(:enterprise) # Permissions appear.. in a different order for some reason. expect_relationship_with_permissions e1, e2, ['to add to order cycle', - 'to create sourced variants', + 'to create sourced variants [BETA]', 'to add products to inventory', 'to edit profile'] er = EnterpriseRelationship.where(parent_id: e1, child_id: e2).first From 940aa57daf5eb35e559ba351087eea2d7e7e7561 Mon Sep 17 00:00:00 2001 From: David Cook Date: Mon, 9 Feb 2026 15:35:28 +1100 Subject: [PATCH 07/29] Set up permissions for creating source variants --- app/models/spree/ability.rb | 13 ++++++++-- lib/open_food_network/permissions.rb | 4 +++ spec/models/spree/ability_spec.rb | 39 ++++++++++++++++++++++++++++ 3 files changed, 54 insertions(+), 2 deletions(-) diff --git a/app/models/spree/ability.rb b/app/models/spree/ability.rb index cc4aeb007a..2b162f8750 100644 --- a/app/models/spree/ability.rb +++ b/app/models/spree/ability.rb @@ -202,7 +202,7 @@ module Spree def add_product_management_abilities(user) # Enterprise User can only access products that they are a supplier for can [:create], Spree::Product - # An enterperprise user can change a product if they are supplier of at least + # An enterprise user can change a product if they are supplier of at least # one of the product's associated variants can [:admin, :read, :index, :update, :seo, :group_buy_options, @@ -214,7 +214,16 @@ module Spree ) end - can [:admin, :index, :bulk_update, :destroy, :destroy_variant, :clone], :products_v3 + # An enterprise user can clone if they have been granted permission to the source variant. + # Technically I'd call this permission clone_source_variant, but it would be less confusing to + # use the same name as everywhere else. + can [:create_sourced_variant], Spree::Variant do |variant| + OpenFoodNetwork::Permissions.new(user). + enterprises_granting_sourced_variants.include? variant.supplier + end + + can [:admin, :index, :bulk_update, :destroy, :destroy_variant, :clone, + :create_sourced_variant], :products_v3 can [:create], Spree::Variant can [:admin, :index, :read, :edit, diff --git a/lib/open_food_network/permissions.rb b/lib/open_food_network/permissions.rb index b3239d1b72..10f8495e37 100644 --- a/lib/open_food_network/permissions.rb +++ b/lib/open_food_network/permissions.rb @@ -86,6 +86,10 @@ module OpenFoodNetwork managed_and_related_enterprises_granting :manage_products end + def enterprises_granting_sourced_variants + related_enterprises_granting :create_sourced_variants + end + def manages_one_enterprise? @user.enterprises.length == 1 end diff --git a/spec/models/spree/ability_spec.rb b/spec/models/spree/ability_spec.rb index 60b0b67f8e..9f71ac98e1 100644 --- a/spec/models/spree/ability_spec.rb +++ b/spec/models/spree/ability_spec.rb @@ -364,6 +364,19 @@ RSpec.describe Spree::Ability do for: p2.variants.first) end + describe "create_sourced_variant" do + it "should not be able to create sourced variant without permission" do + is_expected.not_to have_ability([:create_sourced_variant], for: p_related.variants.first) + end + + it "should be able to create sourced variant when granted permission" do + create(:enterprise_relationship, parent: s_related, child: s1, + permissions_list: [:create_sourced_variants]) + + is_expected.to have_ability([:create_sourced_variant], for: p_related.variants.first) + end + end + it "should not be able to access admin actions on orders" do is_expected.not_to have_ability([:admin], for: Spree::Order) end @@ -720,6 +733,19 @@ RSpec.describe Spree::Ability do it "can request permitted enterprise fees for an order cycle" do is_expected.to have_ability([:for_order_cycle], for: EnterpriseFee) end + + describe "create_sourced_variant" do + it "should not be able to create sourced variant without permission" do + is_expected.not_to have_ability([:create_sourced_variant], for: p_related.variants.first) + end + + it "should be able to create sourced variant when granted permission" do + create(:enterprise_relationship, parent: s_related, child: d1, + permissions_list: [:create_sourced_variants]) + + is_expected.to have_ability([:create_sourced_variant], for: p_related.variants.first) + end + end end context 'Order Cycle co-ordinator, distributor enterprise manager' do @@ -795,6 +821,19 @@ RSpec.describe Spree::Ability do it "has the ability to manage vouchers" do is_expected.to have_ability([:admin, :create], for: Voucher) end + + describe "create_sourced_variant for own enterprise" do + it "should not be able to create own sourced variant without permission" do + is_expected.not_to have_ability([:create_sourced_variant], for: p1.variants.first) + end + + it "should be able to create own sourced variant when granted self permission" do + create(:enterprise_relationship, parent: s1, child: s1, + permissions_list: [:create_sourced_variants]) + + is_expected.to have_ability([:create_sourced_variant], for: p1.variants.first) + end + end end context 'enterprise owner' do From eba2fbcc30a61b2fc21d1044dc737c58a3b0ddf8 Mon Sep 17 00:00:00 2001 From: David Cook Date: Mon, 9 Feb 2026 17:07:26 +1100 Subject: [PATCH 08/29] Create source variants --- .../admin/products_v3_controller.rb | 33 +++++++++++++++ .../_product_variant_row.html.haml | 2 +- .../admin/products_v3/_variant_row.html.haml | 4 +- .../create_sourced_variant.turbo_stream.haml | 16 ++++++++ config/locales/en.yml | 2 + config/routes/admin.rb | 1 + spec/requests/admin/products_v3_spec.rb | 41 +++++++++++++++++++ spec/system/admin/products_v3/actions_spec.rb | 17 ++++++-- 8 files changed, 110 insertions(+), 6 deletions(-) create mode 100644 app/views/admin/products_v3/create_sourced_variant.turbo_stream.haml diff --git a/app/controllers/admin/products_v3_controller.rb b/app/controllers/admin/products_v3_controller.rb index d5c3f5383c..ba7151f05b 100644 --- a/app/controllers/admin/products_v3_controller.rb +++ b/app/controllers/admin/products_v3_controller.rb @@ -107,6 +107,39 @@ module Admin end end + # Clone a variant, retaining a link to the "source" + def create_sourced_variant + source_variant = Spree::Variant.find(params[:variant_id]) + product_index = params[:product_index] + authorize! :create_sourced_variant, source_variant + status = :ok + + begin + variant = source_variant.dup #may need a VariantDuplicator like producs? + variant.price = source_variant.price + variant.save! + variant.on_demand = source_variant.on_demand + variant.on_hand = source_variant.on_hand + variant.save! + #todo: create link to source + + flash.now[:success] = t('.success') + variant_index = "-#{variant.id}" + rescue ActiveRecord::RecordInvalid + flash.now[:error] = variant.errors.full_messages.to_sentence + status = :unprocessable_entity + variant_index = "-1" # Create a unique-enough index + end + + respond_with do |format| + format.turbo_stream { + locals = { source_variant:, variant:, product_index:, variant_index:, + producer_options:, category_options: categories, tax_category_options: } + render :create_sourced_variant, status:, locals: + } + end + end + def index_url(params) "/admin/products?#{params.to_query}" # todo: fix routing so this can be automaticly generated end diff --git a/app/views/admin/products_v3/_product_variant_row.html.haml b/app/views/admin/products_v3/_product_variant_row.html.haml index 71ab6e6301..deb0c467d0 100644 --- a/app/views/admin/products_v3/_product_variant_row.html.haml +++ b/app/views/admin/products_v3/_product_variant_row.html.haml @@ -13,7 +13,7 @@ - next if variant.supplier.present? && !allowed_producers.include?(variant.supplier) = form.fields_for("products][#{product_index}][variants_attributes", variant, index: variant_index) do |variant_form| %tr.condensed{ id: dom_id(variant), 'data-controller': "variant", 'class': "nested-form-wrapper", 'data-new-record': variant.new_record? ? "true" : false } - = render partial: 'variant_row', locals: { variant:, f: variant_form, category_options:, tax_category_options:, producer_options: } + = render partial: 'variant_row', locals: { variant:, f: variant_form, product_index:, category_options:, tax_category_options:, producer_options: } = form.fields_for("products][#{product_index}][variants_attributes][NEW_RECORD", prepare_new_variant(product, producer_options)) do |new_variant_form| %template{ 'data-nested-form-target': "template" } diff --git a/app/views/admin/products_v3/_variant_row.html.haml b/app/views/admin/products_v3/_variant_row.html.haml index 79340b0f3e..d8edcaecd6 100644 --- a/app/views/admin/products_v3/_variant_row.html.haml +++ b/app/views/admin/products_v3/_variant_row.html.haml @@ -1,4 +1,4 @@ --# locals: (variant:, f:, category_options:, tax_category_options:, producer_options:) +-# locals: (variant:, f:, product_index: nil, category_options:, tax_category_options:, producer_options:) - method_on_demand, method_on_hand = variant.new_record? ? [:on_demand_desired, :on_hand_desired ]: [:on_demand, :on_hand] %td.col-image -# empty @@ -89,7 +89,7 @@ - if variant.persisted? = link_to t('admin.products_page.actions.edit'), edit_admin_product_variant_path(variant.product, variant) / TODO: only show if have permission. need to load permissions efficiently please. maybe the PErmissions object can cache result for each enterprise. maybe we preload it with the product query. - = link_to t('admin.products_page.actions.create_sourced_variant'), "TODO" # see next commit + = link_to t('admin.products_page.actions.create_sourced_variant'), admin_create_sourced_variant_path(variant_id: variant.id, product_index:), 'data-turbo-method': :post - if variant.product.variants.size > 1 %a{ "data-controller": "modal-link", "data-action": "click->modal-link#setModalDataSetOnConfirm click->modal-link#open", "data-modal-link-target-value": "variant-delete-modal", "class": "delete", diff --git a/app/views/admin/products_v3/create_sourced_variant.turbo_stream.haml b/app/views/admin/products_v3/create_sourced_variant.turbo_stream.haml new file mode 100644 index 0000000000..da025462c3 --- /dev/null +++ b/app/views/admin/products_v3/create_sourced_variant.turbo_stream.haml @@ -0,0 +1,16 @@ +-# locals: (variant:, source_variant:, product_index:, variant_index:, producer_options:, category_options:, tax_category_options:) +-# Pre-render the form, because you can't do it inside turbo stream block +- variant_row = nil +- fields_for("products][#{product_index}][variants_attributes", variant, index: variant_index) do |f| + - variant_row = render(partial: 'variant_row', formats: :html, + locals: { f:, + variant:, + producer_options:, + category_options:, + tax_category_options:}) += turbo_stream.after dom_id(source_variant) do + %tr.condensed{ id: dom_id(variant), 'data-controller': "variant", 'class': "nested-form-wrapper slide-in" } + = variant_row + += turbo_stream.append "flashes" do + = render(partial: 'admin/shared/flashes', locals: { flashes: flash }) diff --git a/config/locales/en.yml b/config/locales/en.yml index 352e0875de..e38653bf95 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -1099,6 +1099,8 @@ en: clone: success: Successfully cloned the product error: Unable to clone the product + create_sourced_variant: + success: "Successfully created sourced variant" tag_rules: rules_per_tag: one: "%{tag} has 1 rule" diff --git a/config/routes/admin.rb b/config/routes/admin.rb index e36ae10545..079dfee13c 100644 --- a/config/routes/admin.rb +++ b/config/routes/admin.rb @@ -82,6 +82,7 @@ Openfoodnetwork::Application.routes.draw do delete 'products_v3/:id', to: 'products_v3#destroy', as: 'product_destroy' delete 'products_v3/destroy_variant/:id', to: 'products_v3#destroy_variant', as: 'destroy_variant' post 'clone/:id', to: 'products_v3#clone', as: 'clone_product' + post 'products/create_sourced_variant', to: 'products_v3#create_sourced_variant', as: 'create_sourced_variant' resources :product_preview, only: [:show] resources :variant_overrides do diff --git a/spec/requests/admin/products_v3_spec.rb b/spec/requests/admin/products_v3_spec.rb index ff1a6850bb..a18f0c9f74 100644 --- a/spec/requests/admin/products_v3_spec.rb +++ b/spec/requests/admin/products_v3_spec.rb @@ -59,4 +59,45 @@ RSpec.describe "Admin::ProductsV3" do expect(response).to redirect_to('/unauthorized') end end + + describe "POST /admin/products/create_sourced_variant" do + let(:enterprise) { create(:supplier_enterprise) } + let(:user) { create(:user, enterprises: [enterprise]) } + + let(:supplier) { create(:supplier_enterprise) } + let(:variant) { create(:variant, display_name: "Original variant", supplier: supplier) } + + before do + sign_in user + end + + it "checks for permission" do + params = { variant_id: variant.id, product_index: 1 } + + expect { + post(admin_create_sourced_variant_path, as: :turbo_stream, params:) + expect(response).to redirect_to('/unauthorized') + }.not_to change { variant.product.variants.count } + end + + context "With create_sourced_variants permissions on supplier" do + let!(:enterprise_relationship) { + create(:enterprise_relationship, + parent: supplier, + child: enterprise, + permissions_list: [:create_sourced_variants]) + } + + it "creates a clone of the variant, retaining link as source" do + params = { variant_id: variant.id, product_index: 1 } + + expect { + post(admin_create_sourced_variant_path, as: :turbo_stream, params:) + + expect(response).to have_http_status(:ok) + expect(response.body).to match "Original variant" # cloned variant name + }.to change { variant.product.variants.count }.by(1) + end + end + end end diff --git a/spec/system/admin/products_v3/actions_spec.rb b/spec/system/admin/products_v3/actions_spec.rb index 006bce3132..07eff652e1 100644 --- a/spec/system/admin/products_v3/actions_spec.rb +++ b/spec/system/admin/products_v3/actions_spec.rb @@ -291,19 +291,30 @@ RSpec.describe 'As an enterprise user, I can perform actions on the products scr end context "with create_sourced_variants permission for my, and other's variants" do - it "shows an option to create sourced variant" do + it "creates a sourced variant" do create(:enterprise_relationship, parent: producer, child: producer, permissions_list: [:create_sourced_variants]) enterprise_relationship.permissions.create! name: :create_sourced_variants + # Check my own variant within row_containing_name("My box") do page.find(".vertical-ellipsis-menu").click - expect(page).to have_link "Create sourced variant" # , href: admin_clone_product_path(product_a) + + expect(page).to have_link "Create sourced variant" end + # Create variant sourced from my friend within row_containing_name("My friends box") do page.find(".vertical-ellipsis-menu").click - expect(page).to have_link "Create sourced variant" # , href: admin_clone_product_path(product_a) + + click_link "Create sourced variant" + end + + expect(page).to have_content "Successfully created sourced variant" + + within "table.products" do + # There are now two copies + expect(all_input_values).to match /My friends box.*My friends box/ end end end From 1c89e9979ec9be6e09609e8c7c5336a00ebf3ff3 Mon Sep 17 00:00:00 2001 From: David Cook Date: Wed, 11 Feb 2026 17:15:33 +1100 Subject: [PATCH 09/29] CreateVariantLinks [migration] Tried using the rails generator, but as usual it was a waste of time becuase it doesn't handle unusual cases. I found more good guidance from that stackoverflow post: > why are you worrying about your indexes? Build your app! Something's not right in the model, see next commit. --- app/models/spree/variant.rb | 4 ++++ app/models/variant_link.rb | 6 ++++++ .../20260211055758_create_variant_links.rb | 19 +++++++++++++++++++ db/schema.rb | 12 +++++++++++- 4 files changed, 40 insertions(+), 1 deletion(-) create mode 100644 app/models/variant_link.rb create mode 100644 db/migrate/20260211055758_create_variant_links.rb diff --git a/app/models/spree/variant.rb b/app/models/spree/variant.rb index d6eda45ca8..1245154860 100644 --- a/app/models/spree/variant.rb +++ b/app/models/spree/variant.rb @@ -72,6 +72,10 @@ module Spree has_many :semantic_links, as: :subject, dependent: :delete_all has_many :supplier_properties, through: :supplier, source: :properties + has_many :variant_links, dependent: :delete_all + has_many :source_variants, through: :variant_links + has_many :target_variants, through: :variant_links + localize_number :price, :weight validates_lengths_from_database diff --git a/app/models/variant_link.rb b/app/models/variant_link.rb new file mode 100644 index 0000000000..cedecb5c6d --- /dev/null +++ b/app/models/variant_link.rb @@ -0,0 +1,6 @@ +# frozen_string_literal: true + +class VariantLink < ApplicationRecord + belongs_to :source_variant, class_name: 'Spree::Variant', touch: true + belongs_to :target_variant, class_name: 'Spree::Variant', touch: true +end diff --git a/db/migrate/20260211055758_create_variant_links.rb b/db/migrate/20260211055758_create_variant_links.rb new file mode 100644 index 0000000000..6d84484511 --- /dev/null +++ b/db/migrate/20260211055758_create_variant_links.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +class CreateVariantLinks < ActiveRecord::Migration[7.1] + def change + # Create a join table to join two variants. One is the source of the other. + # Primary key index ensures uniqueness and assists querying. target_variant_id is the most + # likely subject and so is first in the index. + # An additional index for source_variant is also included because it may be helpful + # (https://stackoverflow.com/questions/10790518/best-sql-indexes-for-join-table). + create_table :variant_links, primary_key: [:target_variant_id, :source_variant_id] do |t| + t.integer :source_variant_id, null: false, index: true + t.integer :target_variant_id, null: false + + t.timestamps + end + add_foreign_key :variant_links, :spree_variants, column: :source_variant_id + add_foreign_key :variant_links, :spree_variants, column: :target_variant_id + end +end diff --git a/db/schema.rb b/db/schema.rb index aaef15970f..72a13aa378 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -258,8 +258,8 @@ ActiveRecord::Schema[7.1].define(version: 2026_03_06_015040) do t.text "white_label_logo_link" t.boolean "hide_groups_tab", default: false t.string "external_billing_id", limit: 128 - t.boolean "enable_producers_to_edit_orders", default: false, null: false t.boolean "show_customer_contacts_to_suppliers", default: false, null: false + t.boolean "enable_producers_to_edit_orders", default: false, null: false t.index ["address_id"], name: "index_enterprises_on_address_id" t.index ["is_primary_producer", "sells"], name: "index_enterprises_on_is_primary_producer_and_sells" t.index ["name"], name: "index_enterprises_on_name", unique: true @@ -1113,6 +1113,14 @@ ActiveRecord::Schema[7.1].define(version: 2026_03_06_015040) do t.datetime "updated_at", precision: nil, null: false end + create_table "variant_links", primary_key: ["target_variant_id", "source_variant_id"], force: :cascade do |t| + t.integer "source_variant_id", null: false + t.integer "target_variant_id", null: false + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + t.index ["source_variant_id"], name: "index_variant_links_on_source_variant_id" + end + create_table "variant_overrides", id: :serial, force: :cascade do |t| t.integer "variant_id", null: false t.integer "hub_id", null: false @@ -1278,6 +1286,8 @@ ActiveRecord::Schema[7.1].define(version: 2026_03_06_015040) do add_foreign_key "subscriptions", "spree_payment_methods", column: "payment_method_id", name: "subscriptions_payment_method_id_fk" add_foreign_key "subscriptions", "spree_shipping_methods", column: "shipping_method_id", name: "subscriptions_shipping_method_id_fk" add_foreign_key "tag_rules", "enterprises" + add_foreign_key "variant_links", "spree_variants", column: "source_variant_id" + add_foreign_key "variant_links", "spree_variants", column: "target_variant_id" add_foreign_key "variant_overrides", "enterprises", column: "hub_id", name: "variant_overrides_hub_id_fk" add_foreign_key "variant_overrides", "spree_variants", column: "variant_id", name: "variant_overrides_variant_id_fk" add_foreign_key "vouchers", "enterprises" From 04c0adf9603054bb38da049532aab82f81bfcc35 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 17 Feb 2026 01:52:09 +0000 Subject: [PATCH 10/29] Fix source_variants and target_variants associations in Variant model Co-authored-by: dacook <4188088+dacook@users.noreply.github.com> Thanks co-pilot for sending me in the right direction. Would this be neater as a has_and_belongs_to_many? Maybe but I will try to keep moving. --- app/models/spree/variant.rb | 11 ++++++++--- spec/models/spree/variant_spec.rb | 3 +++ 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/app/models/spree/variant.rb b/app/models/spree/variant.rb index 1245154860..16002013c5 100644 --- a/app/models/spree/variant.rb +++ b/app/models/spree/variant.rb @@ -72,9 +72,14 @@ module Spree has_many :semantic_links, as: :subject, dependent: :delete_all has_many :supplier_properties, through: :supplier, source: :properties - has_many :variant_links, dependent: :delete_all - has_many :source_variants, through: :variant_links - has_many :target_variants, through: :variant_links + # Linked variants: I may have one or many sources. + has_many :variant_links_as_target, class_name: 'VariantLink', foreign_key: :target_variant_id, + dependent: :delete_all, inverse_of: :target_variant + has_many :source_variants, through: :variant_links_as_target, source: :source_variant + # I may also have one more many targets. + has_many :variant_links_as_source, class_name: 'VariantLink', foreign_key: :source_variant_id, + dependent: :delete_all, inverse_of: :source_variant + has_many :target_variants, through: :variant_links_as_source, source: :target_variant localize_number :price, :weight diff --git a/spec/models/spree/variant_spec.rb b/spec/models/spree/variant_spec.rb index 86d3ca92ff..92138a0ea2 100644 --- a/spec/models/spree/variant_spec.rb +++ b/spec/models/spree/variant_spec.rb @@ -20,6 +20,9 @@ RSpec.describe Spree::Variant do it { is_expected.to have_many(:inventory_items) } it { is_expected.to have_many(:supplier_properties).through(:supplier) } + it { is_expected.to have_many(:source_variants).through(:variant_links_as_target) } + it { is_expected.to have_many(:target_variants).through(:variant_links_as_source) } + describe "shipping category" do it "sets a shipping category if none provided" do variant = build(:variant, shipping_category: nil) From b877540f5f2314d24964b74719a1b5f13b4924ef Mon Sep 17 00:00:00 2001 From: David Cook Date: Tue, 17 Feb 2026 12:36:53 +1100 Subject: [PATCH 11/29] Create sourced variant link on clone --- app/controllers/admin/products_v3_controller.rb | 2 +- spec/requests/admin/products_v3_spec.rb | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/app/controllers/admin/products_v3_controller.rb b/app/controllers/admin/products_v3_controller.rb index ba7151f05b..faf4836855 100644 --- a/app/controllers/admin/products_v3_controller.rb +++ b/app/controllers/admin/products_v3_controller.rb @@ -118,10 +118,10 @@ module Admin variant = source_variant.dup #may need a VariantDuplicator like producs? variant.price = source_variant.price variant.save! + variant.source_variants << source_variant variant.on_demand = source_variant.on_demand variant.on_hand = source_variant.on_hand variant.save! - #todo: create link to source flash.now[:success] = t('.success') variant_index = "-#{variant.id}" diff --git a/spec/requests/admin/products_v3_spec.rb b/spec/requests/admin/products_v3_spec.rb index a18f0c9f74..485273c699 100644 --- a/spec/requests/admin/products_v3_spec.rb +++ b/spec/requests/admin/products_v3_spec.rb @@ -97,6 +97,12 @@ RSpec.describe "Admin::ProductsV3" do expect(response).to have_http_status(:ok) expect(response.body).to match "Original variant" # cloned variant name }.to change { variant.product.variants.count }.by(1) + + new_variant = variant.product.variants.last + # The new variant is a target of the original. It is a "sourced" variant. + expect(variant.target_variants.first).to eq new_variant + # The new variant's source is the original + expect(new_variant.source_variants.first).to eq variant end end end From 5fc6d25a6920ae815eca013190606dce2b2c9bca Mon Sep 17 00:00:00 2001 From: David Cook Date: Tue, 17 Feb 2026 13:40:19 +1100 Subject: [PATCH 12/29] Display presence of variant link in UI It's quite ugly. But we will be iterating on this later. --- app/views/admin/products_v3/_variant_row.html.haml | 2 ++ config/locales/en.yml | 2 ++ spec/system/admin/products_v3/actions_spec.rb | 2 ++ 3 files changed, 6 insertions(+) diff --git a/app/views/admin/products_v3/_variant_row.html.haml b/app/views/admin/products_v3/_variant_row.html.haml index d8edcaecd6..359b2afbe4 100644 --- a/app/views/admin/products_v3/_variant_row.html.haml +++ b/app/views/admin/products_v3/_variant_row.html.haml @@ -2,6 +2,8 @@ - method_on_demand, method_on_hand = variant.new_record? ? [:on_demand_desired, :on_hand_desired ]: [:on_demand, :on_hand] %td.col-image -# empty + - variant.source_variants.each do |source_variant| + = content_tag(:span, "🔗", title: t('admin.products_page.variant_row.sourced_from', source_name: source_variant.name, source_id: source_variant.id)) %td.col-name.field.naked_inputs = f.hidden_field :id = f.text_field :display_name, 'aria-label': t('admin.products_page.columns.name'), placeholder: variant.product.name diff --git a/config/locales/en.yml b/config/locales/en.yml index e38653bf95..37be1f0f74 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -720,6 +720,8 @@ en: create_sourced_variant: Create sourced variant image: edit: Edit + variant_row: + sourced_from: "Sourced from: %{source_name} (%{source_id})" product_preview: product_preview: Product preview shop_tab: Shop diff --git a/spec/system/admin/products_v3/actions_spec.rb b/spec/system/admin/products_v3/actions_spec.rb index 07eff652e1..5170418283 100644 --- a/spec/system/admin/products_v3/actions_spec.rb +++ b/spec/system/admin/products_v3/actions_spec.rb @@ -315,6 +315,8 @@ RSpec.describe 'As an enterprise user, I can perform actions on the products scr within "table.products" do # There are now two copies expect(all_input_values).to match /My friends box.*My friends box/ + # One of them is designated as a source variant + expect(page).to have_content "🔗" end end end From 78db179ff38f6cbde29509690c8fd78f046bae85 Mon Sep 17 00:00:00 2001 From: David Cook Date: Wed, 4 Mar 2026 11:09:17 +1100 Subject: [PATCH 13/29] haml-lint:disable ViewLength TIL we have linting on haml. I couldn't think of a better way to handle this but would be glad to receive feedback. --- app/views/admin/products_v3/_variant_row.html.haml | 1 + 1 file changed, 1 insertion(+) diff --git a/app/views/admin/products_v3/_variant_row.html.haml b/app/views/admin/products_v3/_variant_row.html.haml index 359b2afbe4..e87803d34b 100644 --- a/app/views/admin/products_v3/_variant_row.html.haml +++ b/app/views/admin/products_v3/_variant_row.html.haml @@ -1,3 +1,4 @@ +-# haml-lint:disable ViewLength (This file is big, but doesn't make sense to split up at this point) -# locals: (variant:, f:, product_index: nil, category_options:, tax_category_options:, producer_options:) - method_on_demand, method_on_hand = variant.new_record? ? [:on_demand_desired, :on_hand_desired ]: [:on_demand, :on_hand] %td.col-image From b26152cf0e0c9a54aa7d904c16c0ecacf4ace22b Mon Sep 17 00:00:00 2001 From: David Cook Date: Tue, 24 Feb 2026 17:13:30 +1100 Subject: [PATCH 14/29] Only show option when you have permission Preload the allow list once in the controller. This controller was initially set up to avoid instance variables, and pass variables explicitly to the template. That's a good principle, but in practice we have a growing list of variables passed down the chain to multiple partials which is getting cumbersome. I think instance variables have their place after all. --- .../admin/products_v3_controller.rb | 6 +++++ .../_product_variant_row.html.haml | 1 + .../admin/products_v3/_variant_row.html.haml | 6 +++-- spec/system/admin/products_v3/actions_spec.rb | 9 +++---- spec/system/admin/products_v3/index_spec.rb | 26 +++++++++++++++++++ 5 files changed, 41 insertions(+), 7 deletions(-) diff --git a/app/controllers/admin/products_v3_controller.rb b/app/controllers/admin/products_v3_controller.rb index faf4836855..74f108dacc 100644 --- a/app/controllers/admin/products_v3_controller.rb +++ b/app/controllers/admin/products_v3_controller.rb @@ -8,6 +8,7 @@ module Admin before_action :init_filters_params before_action :init_pagination_params before_action :init_none_tag + before_action :fetch_data, include: [:index, :bulk_update] def index fetch_products @@ -209,6 +210,11 @@ module Admin ).distinct.order(:name).pluck(:name) end + def fetch_data + @allowed_source_producers = OpenFoodNetwork::Permissions.new(spree_current_user) + .enterprises_granting_sourced_variants + end + def fetch_products product_query = OpenFoodNetwork::Permissions.new(spree_current_user) .editable_products.merge(product_scope_with_includes).ransack(ransack_query).result diff --git a/app/views/admin/products_v3/_product_variant_row.html.haml b/app/views/admin/products_v3/_product_variant_row.html.haml index deb0c467d0..8c9cfd8895 100644 --- a/app/views/admin/products_v3/_product_variant_row.html.haml +++ b/app/views/admin/products_v3/_product_variant_row.html.haml @@ -11,6 +11,7 @@ -# Filter out variant a user has not permission to update, but keep variant with no supplier - next if variant.supplier.present? && !allowed_producers.include?(variant.supplier) + = form.fields_for("products][#{product_index}][variants_attributes", variant, index: variant_index) do |variant_form| %tr.condensed{ id: dom_id(variant), 'data-controller': "variant", 'class': "nested-form-wrapper", 'data-new-record': variant.new_record? ? "true" : false } = render partial: 'variant_row', locals: { variant:, f: variant_form, product_index:, category_options:, tax_category_options:, producer_options: } diff --git a/app/views/admin/products_v3/_variant_row.html.haml b/app/views/admin/products_v3/_variant_row.html.haml index e87803d34b..df56c92a2e 100644 --- a/app/views/admin/products_v3/_variant_row.html.haml +++ b/app/views/admin/products_v3/_variant_row.html.haml @@ -91,8 +91,10 @@ = render(VerticalEllipsisMenuComponent.new) do - if variant.persisted? = link_to t('admin.products_page.actions.edit'), edit_admin_product_variant_path(variant.product, variant) - / TODO: only show if have permission. need to load permissions efficiently please. maybe the PErmissions object can cache result for each enterprise. maybe we preload it with the product query. - = link_to t('admin.products_page.actions.create_sourced_variant'), admin_create_sourced_variant_path(variant_id: variant.id, product_index:), 'data-turbo-method': :post + + - if @allowed_source_producers.include?(variant.supplier) + = link_to t('admin.products_page.actions.create_sourced_variant'), admin_create_sourced_variant_path(variant_id: variant.id, product_index:), 'data-turbo-method': :post + - if variant.product.variants.size > 1 %a{ "data-controller": "modal-link", "data-action": "click->modal-link#setModalDataSetOnConfirm click->modal-link#open", "data-modal-link-target-value": "variant-delete-modal", "class": "delete", diff --git a/spec/system/admin/products_v3/actions_spec.rb b/spec/system/admin/products_v3/actions_spec.rb index 5170418283..a2d4f64262 100644 --- a/spec/system/admin/products_v3/actions_spec.rb +++ b/spec/system/admin/products_v3/actions_spec.rb @@ -286,16 +286,14 @@ RSpec.describe 'As an enterprise user, I can perform actions on the products scr permissions_list: [:manage_products]) } - before do - visit admin_products_url - end - context "with create_sourced_variants permission for my, and other's variants" do it "creates a sourced variant" do create(:enterprise_relationship, parent: producer, child: producer, permissions_list: [:create_sourced_variants]) enterprise_relationship.permissions.create! name: :create_sourced_variants + visit admin_products_url + # Check my own variant within row_containing_name("My box") do page.find(".vertical-ellipsis-menu").click @@ -323,7 +321,8 @@ RSpec.describe 'As an enterprise user, I can perform actions on the products scr context "without create_sourced_variants permission" do it "does not show the option in the menu" do - pending "TODO: hide option if you can't use it." + visit admin_products_url + within row_containing_name("My box") do page.find(".vertical-ellipsis-menu").click expect(page).not_to have_link "Create sourced variant" diff --git a/spec/system/admin/products_v3/index_spec.rb b/spec/system/admin/products_v3/index_spec.rb index e74517262f..d835b807c3 100644 --- a/spec/system/admin/products_v3/index_spec.rb +++ b/spec/system/admin/products_v3/index_spec.rb @@ -139,6 +139,32 @@ RSpec.describe 'As an enterprise user, I can browse my products' do expect(page).to have_select "variant_unit_with_scale", selected: "Items" expect(page).to have_field "variant_unit_name", with: "packet" end + + context "with sourced variant" do + let(:source_producer) { create(:supplier_enterprise) } + let(:p3) { create(:product, name: "Product3", supplier_id: source_producer.id) } + + let!(:v3_source) { p3.variants.first } + let!(:v3_sourced) { + create(:variant, display_name: "Variant3-sourced", product: p3, supplier: source_producer) + } + let!(:enterprise_relationship) { + # Other producer grants me access to manage their variant + create(:enterprise_relationship, parent: source_producer, child: producer, + permissions_list: [:manage_products]) + } + + before do + v3_sourced.source_variants << v3_source + visit admin_products_url + end + + it "shows sourced variant with indicator" do + within row_containing_name("Variant3-sourced") do + expect(page).to have_selector 'span[title*="Sourced from"]' + end + end + end end describe "sorting" do From 8955ffe1261c0604ec44cb22e9611f36afbae3fd Mon Sep 17 00:00:00 2001 From: David Cook Date: Wed, 25 Feb 2026 13:47:13 +1100 Subject: [PATCH 15/29] AddOwnerToSpreeVariants [migration] Should existing variants be migrated to have an owner (copied from supplier)? No, because you can change supplier. This concept needs work. --- app/models/spree/variant.rb | 1 + db/migrate/20260225022934_add_owner_to_spree_variants.rb | 8 ++++++++ db/schema.rb | 2 ++ spec/models/spree/variant_spec.rb | 1 + 4 files changed, 12 insertions(+) create mode 100644 db/migrate/20260225022934_add_owner_to_spree_variants.rb diff --git a/app/models/spree/variant.rb b/app/models/spree/variant.rb index 16002013c5..8276164fbc 100644 --- a/app/models/spree/variant.rb +++ b/app/models/spree/variant.rb @@ -40,6 +40,7 @@ module Spree belongs_to :shipping_category, class_name: 'Spree::ShippingCategory', optional: false belongs_to :primary_taxon, class_name: 'Spree::Taxon', touch: true, optional: false belongs_to :supplier, class_name: 'Enterprise', optional: false, touch: true + belongs_to :owner, class_name: 'Enterprise', optional: true delegate :name, :name=, :description, :description=, :meta_keywords, to: :product diff --git a/db/migrate/20260225022934_add_owner_to_spree_variants.rb b/db/migrate/20260225022934_add_owner_to_spree_variants.rb new file mode 100644 index 0000000000..d74c902eb2 --- /dev/null +++ b/db/migrate/20260225022934_add_owner_to_spree_variants.rb @@ -0,0 +1,8 @@ +# frozen_string_literal: true + +class AddOwnerToSpreeVariants < ActiveRecord::Migration[7.1] + def change + add_column :spree_variants, :owner_id, :integer + add_foreign_key :spree_variants, :enterprises, column: :owner_id + end +end diff --git a/db/schema.rb b/db/schema.rb index 72a13aa378..9db50b5bf2 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -1009,6 +1009,7 @@ ActiveRecord::Schema[7.1].define(version: 2026_03_06_015040) do t.bigint "supplier_id" t.float "variant_unit_scale" t.string "variant_unit_name", limit: 255 + t.integer "owner_id" t.index ["primary_taxon_id"], name: "index_spree_variants_on_primary_taxon_id" t.index ["product_id"], name: "index_variants_on_product_id" t.index ["shipping_category_id"], name: "index_spree_variants_on_shipping_category_id" @@ -1270,6 +1271,7 @@ ActiveRecord::Schema[7.1].define(version: 2026_03_06_015040) do add_foreign_key "spree_tax_rates", "spree_zones", column: "zone_id", name: "spree_tax_rates_zone_id_fk" add_foreign_key "spree_users", "spree_addresses", column: "bill_address_id", name: "spree_users_bill_address_id_fk" add_foreign_key "spree_users", "spree_addresses", column: "ship_address_id", name: "spree_users_ship_address_id_fk" + add_foreign_key "spree_variants", "enterprises", column: "owner_id" add_foreign_key "spree_variants", "enterprises", column: "supplier_id" add_foreign_key "spree_variants", "spree_products", column: "product_id", name: "spree_variants_product_id_fk" add_foreign_key "spree_variants", "spree_shipping_categories", column: "shipping_category_id" diff --git a/spec/models/spree/variant_spec.rb b/spec/models/spree/variant_spec.rb index 92138a0ea2..38655f5f1f 100644 --- a/spec/models/spree/variant_spec.rb +++ b/spec/models/spree/variant_spec.rb @@ -8,6 +8,7 @@ RSpec.describe Spree::Variant do it { is_expected.to have_many :semantic_links } it { is_expected.to belong_to(:product).required } it { is_expected.to belong_to(:supplier).required } + it { is_expected.to belong_to(:owner).optional } it { is_expected.to have_many(:inventory_units) } it { is_expected.to have_many(:line_items) } it { is_expected.to have_many(:stock_items) } From 5757f086ecf843ded46bc4bfeda65bad8397a42a Mon Sep 17 00:00:00 2001 From: David Cook Date: Tue, 3 Mar 2026 16:59:56 +1100 Subject: [PATCH 16/29] Set owner enterprise when creating source variant --- .../admin/products_v3_controller.rb | 2 ++ .../admin/products_v3/_variant_row.html.haml | 2 +- config/locales/en.yml | 2 +- spec/requests/admin/products_v3_spec.rb | 22 ++++++++++++++++++- spec/system/admin/products_v3/index_spec.rb | 8 ++++--- 5 files changed, 30 insertions(+), 6 deletions(-) diff --git a/app/controllers/admin/products_v3_controller.rb b/app/controllers/admin/products_v3_controller.rb index 74f108dacc..9dbafeb7ce 100644 --- a/app/controllers/admin/products_v3_controller.rb +++ b/app/controllers/admin/products_v3_controller.rb @@ -120,6 +120,8 @@ module Admin variant.price = source_variant.price variant.save! variant.source_variants << source_variant + # Owner is my enterprise which has permission to create sourced variants from that supplier + variant.owner_id = EnterpriseRelationship.permitted_by(source_variant.supplier).permitting(spree_current_user.enterprises).with_permission(:create_sourced_variants).pluck(:child_id).first variant.on_demand = source_variant.on_demand variant.on_hand = source_variant.on_hand variant.save! diff --git a/app/views/admin/products_v3/_variant_row.html.haml b/app/views/admin/products_v3/_variant_row.html.haml index df56c92a2e..1aa5f0649b 100644 --- a/app/views/admin/products_v3/_variant_row.html.haml +++ b/app/views/admin/products_v3/_variant_row.html.haml @@ -4,7 +4,7 @@ %td.col-image -# empty - variant.source_variants.each do |source_variant| - = content_tag(:span, "🔗", title: t('admin.products_page.variant_row.sourced_from', source_name: source_variant.name, source_id: source_variant.id)) + = content_tag(:span, "🔗", title: t('admin.products_page.variant_row.sourced_from', source_name: source_variant.name, source_id: source_variant.id, owner_name: variant.owner&.name)) %td.col-name.field.naked_inputs = f.hidden_field :id = f.text_field :display_name, 'aria-label': t('admin.products_page.columns.name'), placeholder: variant.product.name diff --git a/config/locales/en.yml b/config/locales/en.yml index 37be1f0f74..cfd5f8a4bb 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -721,7 +721,7 @@ en: image: edit: Edit variant_row: - sourced_from: "Sourced from: %{source_name} (%{source_id})" + sourced_from: "Sourced from: %{source_name} (%{source_id}); Owned by: %{owner_name}" product_preview: product_preview: Product preview shop_tab: Shop diff --git a/spec/requests/admin/products_v3_spec.rb b/spec/requests/admin/products_v3_spec.rb index 485273c699..e5fdfac7b7 100644 --- a/spec/requests/admin/products_v3_spec.rb +++ b/spec/requests/admin/products_v3_spec.rb @@ -88,7 +88,7 @@ RSpec.describe "Admin::ProductsV3" do permissions_list: [:create_sourced_variants]) } - it "creates a clone of the variant, retaining link as source" do + it "clones the variant, retaining link as source" do params = { variant_id: variant.id, product_index: 1 } expect { @@ -104,6 +104,26 @@ RSpec.describe "Admin::ProductsV3" do # The new variant's source is the original expect(new_variant.source_variants.first).to eq variant end + + context "and I'm also owner of another enterprise" do + let!(:enterprise2) { create(:enterprise) } + let(:user) { create(:user, enterprises: [enterprise, enterprise2]) } + + it "clones the variant, owned by my enterprise that has permission" do + enterprise2.owner = user + params = { variant_id: variant.id, product_index: 1 } + + expect { + post(admin_create_sourced_variant_path, as: :turbo_stream, params:) + + expect(response).to have_http_status(:ok) + }.to change { variant.product.variants.count }.by(1) + + # The new variant is owned by my enterprise that has permission, not the other one + new_variant = variant.product.variants.last + expect(new_variant.owner).to eq enterprise + end + end end end end diff --git a/spec/system/admin/products_v3/index_spec.rb b/spec/system/admin/products_v3/index_spec.rb index d835b807c3..83fddcf354 100644 --- a/spec/system/admin/products_v3/index_spec.rb +++ b/spec/system/admin/products_v3/index_spec.rb @@ -8,7 +8,7 @@ RSpec.describe 'As an enterprise user, I can browse my products' do include AuthenticationHelper include FileHelper - let(:producer) { create(:supplier_enterprise) } + let(:producer) { create(:supplier_enterprise, name: "My Enterprise") } let(:user) { create(:user, enterprises: [producer]) } before do @@ -146,7 +146,8 @@ RSpec.describe 'As an enterprise user, I can browse my products' do let!(:v3_source) { p3.variants.first } let!(:v3_sourced) { - create(:variant, display_name: "Variant3-sourced", product: p3, supplier: source_producer) + create(:variant, display_name: "Variant3-sourced", product: p3, supplier: source_producer, + owner: producer) } let!(:enterprise_relationship) { # Other producer grants me access to manage their variant @@ -161,7 +162,8 @@ RSpec.describe 'As an enterprise user, I can browse my products' do it "shows sourced variant with indicator" do within row_containing_name("Variant3-sourced") do - expect(page).to have_selector 'span[title*="Sourced from"]' + expect(page).to have_selector 'span[title*="Sourced from: "]' + expect(page).to have_selector 'span[title*="Owned by: My Enterprise"]' end end end From 299ada1220ccd5520f333cada4155cede5b5975f Mon Sep 17 00:00:00 2001 From: David Cook Date: Wed, 4 Mar 2026 12:35:13 +1100 Subject: [PATCH 17/29] Refactor: move variant duplication to model I tried to avoid it but rubocop made me move it. I think maybe it will need to go into a concern or service class later, but hopefully it's ok here for now. --- .../admin/products_v3_controller.rb | 10 +------ app/models/spree/variant.rb | 18 +++++++++++ spec/models/spree/variant_spec.rb | 30 +++++++++++++++++++ 3 files changed, 49 insertions(+), 9 deletions(-) diff --git a/app/controllers/admin/products_v3_controller.rb b/app/controllers/admin/products_v3_controller.rb index 9dbafeb7ce..073107c15e 100644 --- a/app/controllers/admin/products_v3_controller.rb +++ b/app/controllers/admin/products_v3_controller.rb @@ -116,15 +116,7 @@ module Admin status = :ok begin - variant = source_variant.dup #may need a VariantDuplicator like producs? - variant.price = source_variant.price - variant.save! - variant.source_variants << source_variant - # Owner is my enterprise which has permission to create sourced variants from that supplier - variant.owner_id = EnterpriseRelationship.permitted_by(source_variant.supplier).permitting(spree_current_user.enterprises).with_permission(:create_sourced_variants).pluck(:child_id).first - variant.on_demand = source_variant.on_demand - variant.on_hand = source_variant.on_hand - variant.save! + variant = source_variant.create_sourced_variant(spree_current_user) flash.now[:success] = t('.success') variant_index = "-#{variant.id}" diff --git a/app/models/spree/variant.rb b/app/models/spree/variant.rb index 8276164fbc..952f48d23d 100644 --- a/app/models/spree/variant.rb +++ b/app/models/spree/variant.rb @@ -273,6 +273,24 @@ module Spree @on_hand_desired = ActiveModel::Type::Integer.new.cast(val) end + # Clone this variant, retaining a 'source' link to it + def create_sourced_variant(user) + # Owner is my enterprise which has permission to create sourced variants from that supplier + owner_id = EnterpriseRelationship.permitted_by(supplier).permitting(user.enterprises) + .with_permission(:create_sourced_variants) + .pick(:child_id) + + dup.tap do |variant| + variant.price = price + variant.save! + variant.source_variants = [self] + variant.owner_id = owner_id + variant.on_demand = on_demand + variant.on_hand = on_hand + variant.save! + end + end + private def check_currency diff --git a/spec/models/spree/variant_spec.rb b/spec/models/spree/variant_spec.rb index 38655f5f1f..a53b3f533b 100644 --- a/spec/models/spree/variant_spec.rb +++ b/spec/models/spree/variant_spec.rb @@ -1005,4 +1005,34 @@ RSpec.describe Spree::Variant do expect(variant.unit_presentation).to eq "My display" end end + + describe "#create_sourced_variant" do + let(:user) { create(:user, enterprises: [enterprise]) } + let(:supplier) { variant.supplier } + let(:enterprise) { create(:enterprise) } + + context "with create_sourced_variants permissions on supplier" do + let!(:enterprise_relationship) { + create(:enterprise_relationship, + parent: supplier, + child: enterprise, + permissions_list: [:create_sourced_variants]) + } + + it "clones the variant, retaining a link to the source" do + variant.price = 10.95 + variant.save! + variant.on_demand = false + variant.on_hand = 5 + + sourced_variant = variant.create_sourced_variant(user) + + expect(sourced_variant.source_variants).to eq [variant] + expect(sourced_variant.owner).to eq enterprise + expect(sourced_variant.price).to eq 10.95 + expect(sourced_variant.on_demand).to eq false + expect(sourced_variant.on_hand).to eq 5 + end + end + end end From de6eb9e2813b3e89ce3c11f90825c973a10e2ec7 Mon Sep 17 00:00:00 2001 From: David Cook Date: Tue, 10 Mar 2026 14:03:46 +1100 Subject: [PATCH 18/29] Avoid useless updated_at column These records won't be updatable, but it might still be worth tracking when they were created. --- db/migrate/20260211055758_create_variant_links.rb | 2 +- db/schema.rb | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/db/migrate/20260211055758_create_variant_links.rb b/db/migrate/20260211055758_create_variant_links.rb index 6d84484511..3109172f8a 100644 --- a/db/migrate/20260211055758_create_variant_links.rb +++ b/db/migrate/20260211055758_create_variant_links.rb @@ -11,7 +11,7 @@ class CreateVariantLinks < ActiveRecord::Migration[7.1] t.integer :source_variant_id, null: false, index: true t.integer :target_variant_id, null: false - t.timestamps + t.datetime :created_at, null: false end add_foreign_key :variant_links, :spree_variants, column: :source_variant_id add_foreign_key :variant_links, :spree_variants, column: :target_variant_id diff --git a/db/schema.rb b/db/schema.rb index 9db50b5bf2..a05db7e719 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -1118,7 +1118,6 @@ ActiveRecord::Schema[7.1].define(version: 2026_03_06_015040) do t.integer "source_variant_id", null: false t.integer "target_variant_id", null: false t.datetime "created_at", null: false - t.datetime "updated_at", null: false t.index ["source_variant_id"], name: "index_variant_links_on_source_variant_id" end From 666e872ac83f3d26c4c08236528788984e306b1f Mon Sep 17 00:00:00 2001 From: David Cook Date: Tue, 10 Mar 2026 14:06:33 +1100 Subject: [PATCH 19/29] Revert uninentional schema change I guess my local db is slightly out of sync. --- db/schema.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/db/schema.rb b/db/schema.rb index a05db7e719..c0270cdc06 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -258,8 +258,8 @@ ActiveRecord::Schema[7.1].define(version: 2026_03_06_015040) do t.text "white_label_logo_link" t.boolean "hide_groups_tab", default: false t.string "external_billing_id", limit: 128 - t.boolean "show_customer_contacts_to_suppliers", default: false, null: false t.boolean "enable_producers_to_edit_orders", default: false, null: false + t.boolean "show_customer_contacts_to_suppliers", default: false, null: false t.index ["address_id"], name: "index_enterprises_on_address_id" t.index ["is_primary_producer", "sells"], name: "index_enterprises_on_is_primary_producer_and_sells" t.index ["name"], name: "index_enterprises_on_name", unique: true From 05c31db46ada919589ccd3d187df64a85e860506 Mon Sep 17 00:00:00 2001 From: David Cook Date: Tue, 10 Mar 2026 14:14:17 +1100 Subject: [PATCH 20/29] Remove touch I think I was just following convention from existing relationships. Perhaps you could argue that a variant is affected by the links added/removed.. but we never look at updated_at so really there's no point at all. --- app/models/variant_link.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/models/variant_link.rb b/app/models/variant_link.rb index cedecb5c6d..0030b80a5a 100644 --- a/app/models/variant_link.rb +++ b/app/models/variant_link.rb @@ -1,6 +1,6 @@ # frozen_string_literal: true class VariantLink < ApplicationRecord - belongs_to :source_variant, class_name: 'Spree::Variant', touch: true - belongs_to :target_variant, class_name: 'Spree::Variant', touch: true + belongs_to :source_variant, class_name: 'Spree::Variant' + belongs_to :target_variant, class_name: 'Spree::Variant' end From 7da6adfe4fbf9b277cd7be23c07866b9dfa3d656 Mon Sep 17 00:00:00 2001 From: David Cook Date: Tue, 10 Mar 2026 14:17:10 +1100 Subject: [PATCH 21/29] Use reference command For uniformity Co-authored-by: Maikel --- db/migrate/20260225022934_add_owner_to_spree_variants.rb | 3 +-- db/schema.rb | 3 ++- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/db/migrate/20260225022934_add_owner_to_spree_variants.rb b/db/migrate/20260225022934_add_owner_to_spree_variants.rb index d74c902eb2..89928af714 100644 --- a/db/migrate/20260225022934_add_owner_to_spree_variants.rb +++ b/db/migrate/20260225022934_add_owner_to_spree_variants.rb @@ -2,7 +2,6 @@ class AddOwnerToSpreeVariants < ActiveRecord::Migration[7.1] def change - add_column :spree_variants, :owner_id, :integer - add_foreign_key :spree_variants, :enterprises, column: :owner_id + add_reference :spree_variants, :owner, foreign_key: { to_table: :enterprises } end end diff --git a/db/schema.rb b/db/schema.rb index c0270cdc06..cf235ead0d 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -1009,7 +1009,8 @@ ActiveRecord::Schema[7.1].define(version: 2026_03_06_015040) do t.bigint "supplier_id" t.float "variant_unit_scale" t.string "variant_unit_name", limit: 255 - t.integer "owner_id" + t.bigint "owner_id" + t.index ["owner_id"], name: "index_spree_variants_on_owner_id" t.index ["primary_taxon_id"], name: "index_spree_variants_on_primary_taxon_id" t.index ["product_id"], name: "index_variants_on_product_id" t.index ["shipping_category_id"], name: "index_spree_variants_on_shipping_category_id" From 6ee715419a1ed3912074e20751b3b3729eb192ee Mon Sep 17 00:00:00 2001 From: David Cook Date: Tue, 10 Mar 2026 14:25:27 +1100 Subject: [PATCH 22/29] ORder by ID to ensure deterministic results --- spec/requests/admin/products_v3_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/requests/admin/products_v3_spec.rb b/spec/requests/admin/products_v3_spec.rb index e5fdfac7b7..54a854bd1f 100644 --- a/spec/requests/admin/products_v3_spec.rb +++ b/spec/requests/admin/products_v3_spec.rb @@ -98,7 +98,7 @@ RSpec.describe "Admin::ProductsV3" do expect(response.body).to match "Original variant" # cloned variant name }.to change { variant.product.variants.count }.by(1) - new_variant = variant.product.variants.last + new_variant = variant.product.variants.order(:id).last # The new variant is a target of the original. It is a "sourced" variant. expect(variant.target_variants.first).to eq new_variant # The new variant's source is the original @@ -120,7 +120,7 @@ RSpec.describe "Admin::ProductsV3" do }.to change { variant.product.variants.count }.by(1) # The new variant is owned by my enterprise that has permission, not the other one - new_variant = variant.product.variants.last + new_variant = variant.product.variants.order(:id).last expect(new_variant.owner).to eq enterprise end end From 7e8b3694be2cdcaba4d915158991fa864ce0f78e Mon Sep 17 00:00:00 2001 From: David Cook Date: Tue, 10 Mar 2026 15:32:34 +1100 Subject: [PATCH 23/29] Move class instance variable to helper --- app/controllers/admin/products_v3_controller.rb | 6 ------ app/helpers/admin/products_helper.rb | 5 +++++ app/views/admin/products_v3/_variant_row.html.haml | 2 +- 3 files changed, 6 insertions(+), 7 deletions(-) diff --git a/app/controllers/admin/products_v3_controller.rb b/app/controllers/admin/products_v3_controller.rb index 073107c15e..e201643511 100644 --- a/app/controllers/admin/products_v3_controller.rb +++ b/app/controllers/admin/products_v3_controller.rb @@ -8,7 +8,6 @@ module Admin before_action :init_filters_params before_action :init_pagination_params before_action :init_none_tag - before_action :fetch_data, include: [:index, :bulk_update] def index fetch_products @@ -204,11 +203,6 @@ module Admin ).distinct.order(:name).pluck(:name) end - def fetch_data - @allowed_source_producers = OpenFoodNetwork::Permissions.new(spree_current_user) - .enterprises_granting_sourced_variants - end - def fetch_products product_query = OpenFoodNetwork::Permissions.new(spree_current_user) .editable_products.merge(product_scope_with_includes).ransack(ransack_query).result diff --git a/app/helpers/admin/products_helper.rb b/app/helpers/admin/products_helper.rb index 90480e371f..18b2f2dda7 100644 --- a/app/helpers/admin/products_helper.rb +++ b/app/helpers/admin/products_helper.rb @@ -47,5 +47,10 @@ module Admin def variant_tag_enabled?(user) feature?(:variant_tag, user) || feature?(:variant_tag, *user.enterprises) end + + def allowed_source_producers + @allowed_source_producers ||= OpenFoodNetwork::Permissions.new(spree_current_user) + .enterprises_granting_sourced_variants + end end end diff --git a/app/views/admin/products_v3/_variant_row.html.haml b/app/views/admin/products_v3/_variant_row.html.haml index 1aa5f0649b..fbd272f2cf 100644 --- a/app/views/admin/products_v3/_variant_row.html.haml +++ b/app/views/admin/products_v3/_variant_row.html.haml @@ -92,7 +92,7 @@ - if variant.persisted? = link_to t('admin.products_page.actions.edit'), edit_admin_product_variant_path(variant.product, variant) - - if @allowed_source_producers.include?(variant.supplier) + - if allowed_source_producers.include?(variant.supplier) = link_to t('admin.products_page.actions.create_sourced_variant'), admin_create_sourced_variant_path(variant_id: variant.id, product_index:), 'data-turbo-method': :post - if variant.product.variants.size > 1 From c165ade4baed80a99c0466a2f94a032db3b4e926 Mon Sep 17 00:00:00 2001 From: David Cook Date: Tue, 10 Mar 2026 15:40:11 +1100 Subject: [PATCH 24/29] Avoid unnecessary save Actually, the variant factory is still adding an extra save. We should refactor Variant to avoid that.. but the afternoon slump has got me. --- app/models/spree/variant.rb | 2 +- spec/models/spree/variant_spec.rb | 6 +----- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/app/models/spree/variant.rb b/app/models/spree/variant.rb index 952f48d23d..eb36f6dacc 100644 --- a/app/models/spree/variant.rb +++ b/app/models/spree/variant.rb @@ -282,8 +282,8 @@ module Spree dup.tap do |variant| variant.price = price - variant.save! variant.source_variants = [self] + variant.stock_items << Spree::StockItem.new(variant:) variant.owner_id = owner_id variant.on_demand = on_demand variant.on_hand = on_hand diff --git a/spec/models/spree/variant_spec.rb b/spec/models/spree/variant_spec.rb index a53b3f533b..fad05d63bb 100644 --- a/spec/models/spree/variant_spec.rb +++ b/spec/models/spree/variant_spec.rb @@ -1018,13 +1018,9 @@ RSpec.describe Spree::Variant do child: enterprise, permissions_list: [:create_sourced_variants]) } + let(:variant) { create(:variant, price: 10.95, on_demand: false, on_hand: 5) } it "clones the variant, retaining a link to the source" do - variant.price = 10.95 - variant.save! - variant.on_demand = false - variant.on_hand = 5 - sourced_variant = variant.create_sourced_variant(user) expect(sourced_variant.source_variants).to eq [variant] From e9ce2df5a9e6f141a8724e903061641950c0f5c7 Mon Sep 17 00:00:00 2001 From: David Cook Date: Tue, 10 Mar 2026 16:45:24 +1100 Subject: [PATCH 25/29] Rename 'source variant' to linked variant (in most places) There are two types of linked variant associations: source and target, so we need to keep the name there. But when cloning a variant and retaining a link as source, we will prefer the general term 'linked variant'. Hopefully this name works well. --- .../enterprise_relationships.js.coffee | 4 +-- .../admin/products_v3_controller.rb | 12 +++---- app/helpers/admin/products_helper.rb | 2 +- app/models/spree/ability.rb | 8 ++--- app/models/spree/variant.rb | 6 ++-- .../admin/products_v3/_variant_row.html.haml | 2 +- ...> create_linked_variant.turbo_stream.haml} | 4 +-- config/locales/en.yml | 8 ++--- config/routes/admin.rb | 2 +- lib/open_food_network/permissions.rb | 4 +-- .../enterprise_relationships_spec.js.coffee | 2 +- spec/models/spree/ability_spec.rb | 32 +++++++++---------- spec/models/spree/variant_spec.rb | 18 +++++------ spec/requests/admin/products_v3_spec.rb | 12 +++---- .../admin/enterprise_relationships_spec.rb | 6 ++-- spec/system/admin/products_v3/actions_spec.rb | 26 +++++++-------- 16 files changed, 74 insertions(+), 74 deletions(-) rename app/views/admin/products_v3/{create_sourced_variant.turbo_stream.haml => create_linked_variant.turbo_stream.haml} (87%) diff --git a/app/assets/javascripts/admin/services/enterprise_relationships.js.coffee b/app/assets/javascripts/admin/services/enterprise_relationships.js.coffee index 1e57018e99..52106156da 100644 --- a/app/assets/javascripts/admin/services/enterprise_relationships.js.coffee +++ b/app/assets/javascripts/admin/services/enterprise_relationships.js.coffee @@ -6,7 +6,7 @@ angular.module("ofn.admin").factory 'EnterpriseRelationships', ($http, enterpris 'manage_products' 'edit_profile' 'create_variant_overrides' - 'create_sourced_variants' + 'create_linked_variants' ] constructor: -> @@ -31,4 +31,4 @@ angular.module("ofn.admin").factory 'EnterpriseRelationships', ($http, enterpris when "manage_products" then t('js.services.manage_products') when "edit_profile" then t('js.services.edit_profile') when "create_variant_overrides" then t('js.services.add_products_to_inventory') - when "create_sourced_variants" then t('js.services.create_sourced_variants') + when "create_linked_variants" then t('js.services.create_linked_variants') diff --git a/app/controllers/admin/products_v3_controller.rb b/app/controllers/admin/products_v3_controller.rb index e201643511..80b99aed73 100644 --- a/app/controllers/admin/products_v3_controller.rb +++ b/app/controllers/admin/products_v3_controller.rb @@ -108,14 +108,14 @@ module Admin end # Clone a variant, retaining a link to the "source" - def create_sourced_variant - source_variant = Spree::Variant.find(params[:variant_id]) + def create_linked_variant + linked_variant = Spree::Variant.find(params[:variant_id]) product_index = params[:product_index] - authorize! :create_sourced_variant, source_variant + authorize! :create_linked_variant, linked_variant status = :ok begin - variant = source_variant.create_sourced_variant(spree_current_user) + variant = linked_variant.create_linked_variant(spree_current_user) flash.now[:success] = t('.success') variant_index = "-#{variant.id}" @@ -127,9 +127,9 @@ module Admin respond_with do |format| format.turbo_stream { - locals = { source_variant:, variant:, product_index:, variant_index:, + locals = { linked_variant:, variant:, product_index:, variant_index:, producer_options:, category_options: categories, tax_category_options: } - render :create_sourced_variant, status:, locals: + render :create_linked_variant, status:, locals: } end end diff --git a/app/helpers/admin/products_helper.rb b/app/helpers/admin/products_helper.rb index 18b2f2dda7..8b07a650d9 100644 --- a/app/helpers/admin/products_helper.rb +++ b/app/helpers/admin/products_helper.rb @@ -50,7 +50,7 @@ module Admin def allowed_source_producers @allowed_source_producers ||= OpenFoodNetwork::Permissions.new(spree_current_user) - .enterprises_granting_sourced_variants + .enterprises_granting_linked_variants end end end diff --git a/app/models/spree/ability.rb b/app/models/spree/ability.rb index 2b162f8750..33e6181e50 100644 --- a/app/models/spree/ability.rb +++ b/app/models/spree/ability.rb @@ -215,15 +215,15 @@ module Spree end # An enterprise user can clone if they have been granted permission to the source variant. - # Technically I'd call this permission clone_source_variant, but it would be less confusing to + # Technically I'd call this permission clone_linked_variant, but it would be less confusing to # use the same name as everywhere else. - can [:create_sourced_variant], Spree::Variant do |variant| + can [:create_linked_variant], Spree::Variant do |variant| OpenFoodNetwork::Permissions.new(user). - enterprises_granting_sourced_variants.include? variant.supplier + enterprises_granting_linked_variants.include? variant.supplier end can [:admin, :index, :bulk_update, :destroy, :destroy_variant, :clone, - :create_sourced_variant], :products_v3 + :create_linked_variant], :products_v3 can [:create], Spree::Variant can [:admin, :index, :read, :edit, diff --git a/app/models/spree/variant.rb b/app/models/spree/variant.rb index eb36f6dacc..a57a873d0c 100644 --- a/app/models/spree/variant.rb +++ b/app/models/spree/variant.rb @@ -274,10 +274,10 @@ module Spree end # Clone this variant, retaining a 'source' link to it - def create_sourced_variant(user) - # Owner is my enterprise which has permission to create sourced variants from that supplier + def create_linked_variant(user) + # Owner is my enterprise which has permission to create variants sourced from that supplier owner_id = EnterpriseRelationship.permitted_by(supplier).permitting(user.enterprises) - .with_permission(:create_sourced_variants) + .with_permission(:create_linked_variants) .pick(:child_id) dup.tap do |variant| diff --git a/app/views/admin/products_v3/_variant_row.html.haml b/app/views/admin/products_v3/_variant_row.html.haml index fbd272f2cf..3982046e27 100644 --- a/app/views/admin/products_v3/_variant_row.html.haml +++ b/app/views/admin/products_v3/_variant_row.html.haml @@ -93,7 +93,7 @@ = link_to t('admin.products_page.actions.edit'), edit_admin_product_variant_path(variant.product, variant) - if allowed_source_producers.include?(variant.supplier) - = link_to t('admin.products_page.actions.create_sourced_variant'), admin_create_sourced_variant_path(variant_id: variant.id, product_index:), 'data-turbo-method': :post + = link_to t('admin.products_page.actions.create_linked_variant'), admin_create_linked_variant_path(variant_id: variant.id, product_index:), 'data-turbo-method': :post - if variant.product.variants.size > 1 %a{ "data-controller": "modal-link", "data-action": "click->modal-link#setModalDataSetOnConfirm click->modal-link#open", diff --git a/app/views/admin/products_v3/create_sourced_variant.turbo_stream.haml b/app/views/admin/products_v3/create_linked_variant.turbo_stream.haml similarity index 87% rename from app/views/admin/products_v3/create_sourced_variant.turbo_stream.haml rename to app/views/admin/products_v3/create_linked_variant.turbo_stream.haml index da025462c3..f67d9227d5 100644 --- a/app/views/admin/products_v3/create_sourced_variant.turbo_stream.haml +++ b/app/views/admin/products_v3/create_linked_variant.turbo_stream.haml @@ -1,4 +1,4 @@ --# locals: (variant:, source_variant:, product_index:, variant_index:, producer_options:, category_options:, tax_category_options:) +-# locals: (variant:, linked_variant:, product_index:, variant_index:, producer_options:, category_options:, tax_category_options:) -# Pre-render the form, because you can't do it inside turbo stream block - variant_row = nil - fields_for("products][#{product_index}][variants_attributes", variant, index: variant_index) do |f| @@ -8,7 +8,7 @@ producer_options:, category_options:, tax_category_options:}) -= turbo_stream.after dom_id(source_variant) do += turbo_stream.after dom_id(linked_variant) do %tr.condensed{ id: dom_id(variant), 'data-controller': "variant", 'class': "nested-form-wrapper slide-in" } = variant_row diff --git a/config/locales/en.yml b/config/locales/en.yml index cfd5f8a4bb..d7adcc8183 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -717,7 +717,7 @@ en: delete: Delete remove: Remove preview: Preview - create_sourced_variant: Create sourced variant + create_linked_variant: Create linked variant image: edit: Edit variant_row: @@ -1101,8 +1101,8 @@ en: clone: success: Successfully cloned the product error: Unable to clone the product - create_sourced_variant: - success: "Successfully created sourced variant" + create_linked_variant: + success: "Successfully created linked variant" tag_rules: rules_per_tag: one: "%{tag} has 1 rule" @@ -3906,7 +3906,7 @@ en: manage_products: "manage products" edit_profile: "edit profile" add_products_to_inventory: "add products to inventory" - create_sourced_variants: "create sourced variants [BETA]" + create_linked_variants: "create linked variants [BETA]" resources: could_not_delete_customer: 'Could not delete customer' product_import: diff --git a/config/routes/admin.rb b/config/routes/admin.rb index 079dfee13c..33553f5a51 100644 --- a/config/routes/admin.rb +++ b/config/routes/admin.rb @@ -82,7 +82,7 @@ Openfoodnetwork::Application.routes.draw do delete 'products_v3/:id', to: 'products_v3#destroy', as: 'product_destroy' delete 'products_v3/destroy_variant/:id', to: 'products_v3#destroy_variant', as: 'destroy_variant' post 'clone/:id', to: 'products_v3#clone', as: 'clone_product' - post 'products/create_sourced_variant', to: 'products_v3#create_sourced_variant', as: 'create_sourced_variant' + post 'products/create_linked_variant', to: 'products_v3#create_linked_variant', as: 'create_linked_variant' resources :product_preview, only: [:show] resources :variant_overrides do diff --git a/lib/open_food_network/permissions.rb b/lib/open_food_network/permissions.rb index 10f8495e37..0c9d0778f4 100644 --- a/lib/open_food_network/permissions.rb +++ b/lib/open_food_network/permissions.rb @@ -86,8 +86,8 @@ module OpenFoodNetwork managed_and_related_enterprises_granting :manage_products end - def enterprises_granting_sourced_variants - related_enterprises_granting :create_sourced_variants + def enterprises_granting_linked_variants + related_enterprises_granting :create_linked_variants end def manages_one_enterprise? diff --git a/spec/javascripts/unit/admin/services/enterprise_relationships_spec.js.coffee b/spec/javascripts/unit/admin/services/enterprise_relationships_spec.js.coffee index c25764b986..2a5cb5ddf2 100644 --- a/spec/javascripts/unit/admin/services/enterprise_relationships_spec.js.coffee +++ b/spec/javascripts/unit/admin/services/enterprise_relationships_spec.js.coffee @@ -16,4 +16,4 @@ describe "enterprise relationships", -> expect(EnterpriseRelationships.permission_presentation("manage_products")).toEqual "manage products" expect(EnterpriseRelationships.permission_presentation("edit_profile")).toEqual "edit profile" expect(EnterpriseRelationships.permission_presentation("create_variant_overrides")).toEqual "add products to inventory" - expect(EnterpriseRelationships.permission_presentation("create_sourced_variants")).toEqual "create sourced variants [BETA]" + expect(EnterpriseRelationships.permission_presentation("create_linked_variants")).toEqual "create linked variants [BETA]" diff --git a/spec/models/spree/ability_spec.rb b/spec/models/spree/ability_spec.rb index 9f71ac98e1..e9e6986120 100644 --- a/spec/models/spree/ability_spec.rb +++ b/spec/models/spree/ability_spec.rb @@ -364,16 +364,16 @@ RSpec.describe Spree::Ability do for: p2.variants.first) end - describe "create_sourced_variant" do - it "should not be able to create sourced variant without permission" do - is_expected.not_to have_ability([:create_sourced_variant], for: p_related.variants.first) + describe "create_linked_variant" do + it "should not be able to create linked variant without permission" do + is_expected.not_to have_ability([:create_linked_variant], for: p_related.variants.first) end - it "should be able to create sourced variant when granted permission" do + it "should be able to create linked variant when granted permission" do create(:enterprise_relationship, parent: s_related, child: s1, - permissions_list: [:create_sourced_variants]) + permissions_list: [:create_linked_variants]) - is_expected.to have_ability([:create_sourced_variant], for: p_related.variants.first) + is_expected.to have_ability([:create_linked_variant], for: p_related.variants.first) end end @@ -734,16 +734,16 @@ RSpec.describe Spree::Ability do is_expected.to have_ability([:for_order_cycle], for: EnterpriseFee) end - describe "create_sourced_variant" do - it "should not be able to create sourced variant without permission" do - is_expected.not_to have_ability([:create_sourced_variant], for: p_related.variants.first) + describe "create_linked_variant" do + it "should not be able to create linked variant without permission" do + is_expected.not_to have_ability([:create_linked_variant], for: p_related.variants.first) end - it "should be able to create sourced variant when granted permission" do + it "should be able to create linked variant when granted permission" do create(:enterprise_relationship, parent: s_related, child: d1, - permissions_list: [:create_sourced_variants]) + permissions_list: [:create_linked_variants]) - is_expected.to have_ability([:create_sourced_variant], for: p_related.variants.first) + is_expected.to have_ability([:create_linked_variant], for: p_related.variants.first) end end end @@ -822,16 +822,16 @@ RSpec.describe Spree::Ability do is_expected.to have_ability([:admin, :create], for: Voucher) end - describe "create_sourced_variant for own enterprise" do + describe "create_linked_variant for own enterprise" do it "should not be able to create own sourced variant without permission" do - is_expected.not_to have_ability([:create_sourced_variant], for: p1.variants.first) + is_expected.not_to have_ability([:create_linked_variant], for: p1.variants.first) end it "should be able to create own sourced variant when granted self permission" do create(:enterprise_relationship, parent: s1, child: s1, - permissions_list: [:create_sourced_variants]) + permissions_list: [:create_linked_variants]) - is_expected.to have_ability([:create_sourced_variant], for: p1.variants.first) + is_expected.to have_ability([:create_linked_variant], for: p1.variants.first) end end end diff --git a/spec/models/spree/variant_spec.rb b/spec/models/spree/variant_spec.rb index fad05d63bb..1b0bb47acc 100644 --- a/spec/models/spree/variant_spec.rb +++ b/spec/models/spree/variant_spec.rb @@ -1006,28 +1006,28 @@ RSpec.describe Spree::Variant do end end - describe "#create_sourced_variant" do + describe "#create_linked_variant" do let(:user) { create(:user, enterprises: [enterprise]) } let(:supplier) { variant.supplier } let(:enterprise) { create(:enterprise) } - context "with create_sourced_variants permissions on supplier" do + context "with create_linked_variants permissions on supplier" do let!(:enterprise_relationship) { create(:enterprise_relationship, parent: supplier, child: enterprise, - permissions_list: [:create_sourced_variants]) + permissions_list: [:create_linked_variants]) } let(:variant) { create(:variant, price: 10.95, on_demand: false, on_hand: 5) } it "clones the variant, retaining a link to the source" do - sourced_variant = variant.create_sourced_variant(user) + linked_variant = variant.create_linked_variant(user) - expect(sourced_variant.source_variants).to eq [variant] - expect(sourced_variant.owner).to eq enterprise - expect(sourced_variant.price).to eq 10.95 - expect(sourced_variant.on_demand).to eq false - expect(sourced_variant.on_hand).to eq 5 + expect(linked_variant.source_variants).to eq [variant] + expect(linked_variant.owner).to eq enterprise + expect(linked_variant.price).to eq 10.95 + expect(linked_variant.on_demand).to eq false + expect(linked_variant.on_hand).to eq 5 end end end diff --git a/spec/requests/admin/products_v3_spec.rb b/spec/requests/admin/products_v3_spec.rb index 54a854bd1f..de4418458c 100644 --- a/spec/requests/admin/products_v3_spec.rb +++ b/spec/requests/admin/products_v3_spec.rb @@ -60,7 +60,7 @@ RSpec.describe "Admin::ProductsV3" do end end - describe "POST /admin/products/create_sourced_variant" do + describe "POST /admin/products/create_linked_variant" do let(:enterprise) { create(:supplier_enterprise) } let(:user) { create(:user, enterprises: [enterprise]) } @@ -75,24 +75,24 @@ RSpec.describe "Admin::ProductsV3" do params = { variant_id: variant.id, product_index: 1 } expect { - post(admin_create_sourced_variant_path, as: :turbo_stream, params:) + post(admin_create_linked_variant_path, as: :turbo_stream, params:) expect(response).to redirect_to('/unauthorized') }.not_to change { variant.product.variants.count } end - context "With create_sourced_variants permissions on supplier" do + context "With create_linked_variants permissions on supplier" do let!(:enterprise_relationship) { create(:enterprise_relationship, parent: supplier, child: enterprise, - permissions_list: [:create_sourced_variants]) + permissions_list: [:create_linked_variants]) } it "clones the variant, retaining link as source" do params = { variant_id: variant.id, product_index: 1 } expect { - post(admin_create_sourced_variant_path, as: :turbo_stream, params:) + post(admin_create_linked_variant_path, as: :turbo_stream, params:) expect(response).to have_http_status(:ok) expect(response.body).to match "Original variant" # cloned variant name @@ -114,7 +114,7 @@ RSpec.describe "Admin::ProductsV3" do params = { variant_id: variant.id, product_index: 1 } expect { - post(admin_create_sourced_variant_path, as: :turbo_stream, params:) + post(admin_create_linked_variant_path, as: :turbo_stream, params:) expect(response).to have_http_status(:ok) }.to change { variant.product.variants.count }.by(1) diff --git a/spec/system/admin/enterprise_relationships_spec.rb b/spec/system/admin/enterprise_relationships_spec.rb index 3f0525e668..aef6096605 100644 --- a/spec/system/admin/enterprise_relationships_spec.rb +++ b/spec/system/admin/enterprise_relationships_spec.rb @@ -47,7 +47,7 @@ create(:enterprise) uncheck 'to manage products' check 'to edit profile' check 'to add products to inventory' - check 'to create sourced variants' + check 'to create linked variants' select2_select 'Two', from: 'enterprise_relationship_child_id' click_button 'Create' @@ -56,14 +56,14 @@ create(:enterprise) # Permissions appear.. in a different order for some reason. expect_relationship_with_permissions e1, e2, ['to add to order cycle', - 'to create sourced variants [BETA]', + 'to create linked variants [BETA]', 'to add products to inventory', 'to edit profile'] er = EnterpriseRelationship.where(parent_id: e1, child_id: e2).first expect(er).to be_present expect(er.permissions.map(&:name)).to match_array ['add_to_order_cycle', 'edit_profile', 'create_variant_overrides', - 'create_sourced_variants'] + 'create_linked_variants'] end it "attempting to create a relationship with invalid data" do diff --git a/spec/system/admin/products_v3/actions_spec.rb b/spec/system/admin/products_v3/actions_spec.rb index a2d4f64262..a885e166e9 100644 --- a/spec/system/admin/products_v3/actions_spec.rb +++ b/spec/system/admin/products_v3/actions_spec.rb @@ -272,7 +272,7 @@ RSpec.describe 'As an enterprise user, I can perform actions on the products scr end end - describe "Create sourced variant" do + describe "Create linked variant" do let!(:variant) { create(:variant, display_name: "My box", supplier: producer) } @@ -286,11 +286,11 @@ RSpec.describe 'As an enterprise user, I can perform actions on the products scr permissions_list: [:manage_products]) } - context "with create_sourced_variants permission for my, and other's variants" do - it "creates a sourced variant" do + context "with create_linked_variants permission for my, and other's variants" do + it "creates a linked variant" do create(:enterprise_relationship, parent: producer, child: producer, - permissions_list: [:create_sourced_variants]) - enterprise_relationship.permissions.create! name: :create_sourced_variants + permissions_list: [:create_linked_variants]) + enterprise_relationship.permissions.create! name: :create_linked_variants visit admin_products_url @@ -298,39 +298,39 @@ RSpec.describe 'As an enterprise user, I can perform actions on the products scr within row_containing_name("My box") do page.find(".vertical-ellipsis-menu").click - expect(page).to have_link "Create sourced variant" + expect(page).to have_link "Create linked variant" end - # Create variant sourced from my friend + # Create linked variant sourced from my friend within row_containing_name("My friends box") do page.find(".vertical-ellipsis-menu").click - click_link "Create sourced variant" + click_link "Create linked variant" end - expect(page).to have_content "Successfully created sourced variant" + expect(page).to have_content "Successfully created linked variant" within "table.products" do # There are now two copies expect(all_input_values).to match /My friends box.*My friends box/ - # One of them is designated as a source variant + # One of them is designated as a linked variant expect(page).to have_content "🔗" end end end - context "without create_sourced_variants permission" do + context "without create_linked_variants permission" do it "does not show the option in the menu" do visit admin_products_url within row_containing_name("My box") do page.find(".vertical-ellipsis-menu").click - expect(page).not_to have_link "Create sourced variant" + expect(page).not_to have_link "Create linked variant" end within row_containing_name("My friends box") do page.find(".vertical-ellipsis-menu").click - expect(page).not_to have_link "Create sourced variant" + expect(page).not_to have_link "Create linked variant" end end end From 18fb1cfa746912956e16037af04fa59829a4446d Mon Sep 17 00:00:00 2001 From: David Cook Date: Wed, 11 Mar 2026 10:44:15 +1100 Subject: [PATCH 26/29] Rename variant 'owner' to 'hub' As discussed by team, and using same nomenclature as VariantOverride. --- app/models/spree/variant.rb | 8 ++++---- app/views/admin/products_v3/_variant_row.html.haml | 2 +- config/locales/en.yml | 2 +- db/migrate/20260225022934_add_hub_to_spree_variants.rb | 7 +++++++ db/migrate/20260225022934_add_owner_to_spree_variants.rb | 7 ------- db/schema.rb | 6 +++--- spec/models/spree/variant_spec.rb | 4 ++-- spec/requests/admin/products_v3_spec.rb | 2 +- spec/system/admin/products_v3/index_spec.rb | 4 ++-- 9 files changed, 21 insertions(+), 21 deletions(-) create mode 100644 db/migrate/20260225022934_add_hub_to_spree_variants.rb delete mode 100644 db/migrate/20260225022934_add_owner_to_spree_variants.rb diff --git a/app/models/spree/variant.rb b/app/models/spree/variant.rb index a57a873d0c..13d556b377 100644 --- a/app/models/spree/variant.rb +++ b/app/models/spree/variant.rb @@ -40,7 +40,7 @@ module Spree belongs_to :shipping_category, class_name: 'Spree::ShippingCategory', optional: false belongs_to :primary_taxon, class_name: 'Spree::Taxon', touch: true, optional: false belongs_to :supplier, class_name: 'Enterprise', optional: false, touch: true - belongs_to :owner, class_name: 'Enterprise', optional: true + belongs_to :hub, class_name: 'Enterprise', optional: true delegate :name, :name=, :description, :description=, :meta_keywords, to: :product @@ -275,8 +275,8 @@ module Spree # Clone this variant, retaining a 'source' link to it def create_linked_variant(user) - # Owner is my enterprise which has permission to create variants sourced from that supplier - owner_id = EnterpriseRelationship.permitted_by(supplier).permitting(user.enterprises) + # Hub owner is my enterprise which has permission to create variant sourced from that supplier + hub_id = EnterpriseRelationship.permitted_by(supplier).permitting(user.enterprises) .with_permission(:create_linked_variants) .pick(:child_id) @@ -284,7 +284,7 @@ module Spree variant.price = price variant.source_variants = [self] variant.stock_items << Spree::StockItem.new(variant:) - variant.owner_id = owner_id + variant.hub_id = hub_id variant.on_demand = on_demand variant.on_hand = on_hand variant.save! diff --git a/app/views/admin/products_v3/_variant_row.html.haml b/app/views/admin/products_v3/_variant_row.html.haml index 3982046e27..f5aee06bf7 100644 --- a/app/views/admin/products_v3/_variant_row.html.haml +++ b/app/views/admin/products_v3/_variant_row.html.haml @@ -4,7 +4,7 @@ %td.col-image -# empty - variant.source_variants.each do |source_variant| - = content_tag(:span, "🔗", title: t('admin.products_page.variant_row.sourced_from', source_name: source_variant.name, source_id: source_variant.id, owner_name: variant.owner&.name)) + = content_tag(:span, "🔗", title: t('admin.products_page.variant_row.sourced_from', source_name: source_variant.name, source_id: source_variant.id, hub_name: variant.hub&.name)) %td.col-name.field.naked_inputs = f.hidden_field :id = f.text_field :display_name, 'aria-label': t('admin.products_page.columns.name'), placeholder: variant.product.name diff --git a/config/locales/en.yml b/config/locales/en.yml index d7adcc8183..2b8134bc77 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -721,7 +721,7 @@ en: image: edit: Edit variant_row: - sourced_from: "Sourced from: %{source_name} (%{source_id}); Owned by: %{owner_name}" + sourced_from: "Sourced from: %{source_name} (%{source_id}); Hub: %{hub_name}" product_preview: product_preview: Product preview shop_tab: Shop diff --git a/db/migrate/20260225022934_add_hub_to_spree_variants.rb b/db/migrate/20260225022934_add_hub_to_spree_variants.rb new file mode 100644 index 0000000000..bcfed767b8 --- /dev/null +++ b/db/migrate/20260225022934_add_hub_to_spree_variants.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +class AddHubToSpreeVariants < ActiveRecord::Migration[7.1] + def change + add_reference :spree_variants, :hub, foreign_key: { to_table: :enterprises } + end +end diff --git a/db/migrate/20260225022934_add_owner_to_spree_variants.rb b/db/migrate/20260225022934_add_owner_to_spree_variants.rb deleted file mode 100644 index 89928af714..0000000000 --- a/db/migrate/20260225022934_add_owner_to_spree_variants.rb +++ /dev/null @@ -1,7 +0,0 @@ -# frozen_string_literal: true - -class AddOwnerToSpreeVariants < ActiveRecord::Migration[7.1] - def change - add_reference :spree_variants, :owner, foreign_key: { to_table: :enterprises } - end -end diff --git a/db/schema.rb b/db/schema.rb index cf235ead0d..1c77a7ca8d 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -1009,8 +1009,8 @@ ActiveRecord::Schema[7.1].define(version: 2026_03_06_015040) do t.bigint "supplier_id" t.float "variant_unit_scale" t.string "variant_unit_name", limit: 255 - t.bigint "owner_id" - t.index ["owner_id"], name: "index_spree_variants_on_owner_id" + t.bigint "hub_id" + t.index ["hub_id"], name: "index_spree_variants_on_hub_id" t.index ["primary_taxon_id"], name: "index_spree_variants_on_primary_taxon_id" t.index ["product_id"], name: "index_variants_on_product_id" t.index ["shipping_category_id"], name: "index_spree_variants_on_shipping_category_id" @@ -1271,7 +1271,7 @@ ActiveRecord::Schema[7.1].define(version: 2026_03_06_015040) do add_foreign_key "spree_tax_rates", "spree_zones", column: "zone_id", name: "spree_tax_rates_zone_id_fk" add_foreign_key "spree_users", "spree_addresses", column: "bill_address_id", name: "spree_users_bill_address_id_fk" add_foreign_key "spree_users", "spree_addresses", column: "ship_address_id", name: "spree_users_ship_address_id_fk" - add_foreign_key "spree_variants", "enterprises", column: "owner_id" + add_foreign_key "spree_variants", "enterprises", column: "hub_id" add_foreign_key "spree_variants", "enterprises", column: "supplier_id" add_foreign_key "spree_variants", "spree_products", column: "product_id", name: "spree_variants_product_id_fk" add_foreign_key "spree_variants", "spree_shipping_categories", column: "shipping_category_id" diff --git a/spec/models/spree/variant_spec.rb b/spec/models/spree/variant_spec.rb index 1b0bb47acc..085be8a04b 100644 --- a/spec/models/spree/variant_spec.rb +++ b/spec/models/spree/variant_spec.rb @@ -8,7 +8,7 @@ RSpec.describe Spree::Variant do it { is_expected.to have_many :semantic_links } it { is_expected.to belong_to(:product).required } it { is_expected.to belong_to(:supplier).required } - it { is_expected.to belong_to(:owner).optional } + it { is_expected.to belong_to(:hub).optional } it { is_expected.to have_many(:inventory_units) } it { is_expected.to have_many(:line_items) } it { is_expected.to have_many(:stock_items) } @@ -1024,7 +1024,7 @@ RSpec.describe Spree::Variant do linked_variant = variant.create_linked_variant(user) expect(linked_variant.source_variants).to eq [variant] - expect(linked_variant.owner).to eq enterprise + expect(linked_variant.hub).to eq enterprise expect(linked_variant.price).to eq 10.95 expect(linked_variant.on_demand).to eq false expect(linked_variant.on_hand).to eq 5 diff --git a/spec/requests/admin/products_v3_spec.rb b/spec/requests/admin/products_v3_spec.rb index de4418458c..1f0773753a 100644 --- a/spec/requests/admin/products_v3_spec.rb +++ b/spec/requests/admin/products_v3_spec.rb @@ -121,7 +121,7 @@ RSpec.describe "Admin::ProductsV3" do # The new variant is owned by my enterprise that has permission, not the other one new_variant = variant.product.variants.order(:id).last - expect(new_variant.owner).to eq enterprise + expect(new_variant.hub).to eq enterprise end end end diff --git a/spec/system/admin/products_v3/index_spec.rb b/spec/system/admin/products_v3/index_spec.rb index 83fddcf354..d943e5a1d3 100644 --- a/spec/system/admin/products_v3/index_spec.rb +++ b/spec/system/admin/products_v3/index_spec.rb @@ -147,7 +147,7 @@ RSpec.describe 'As an enterprise user, I can browse my products' do let!(:v3_source) { p3.variants.first } let!(:v3_sourced) { create(:variant, display_name: "Variant3-sourced", product: p3, supplier: source_producer, - owner: producer) + hub: producer) } let!(:enterprise_relationship) { # Other producer grants me access to manage their variant @@ -163,7 +163,7 @@ RSpec.describe 'As an enterprise user, I can browse my products' do it "shows sourced variant with indicator" do within row_containing_name("Variant3-sourced") do expect(page).to have_selector 'span[title*="Sourced from: "]' - expect(page).to have_selector 'span[title*="Owned by: My Enterprise"]' + expect(page).to have_selector 'span[title*="Hub: My Enterprise"]' end end end From 827ba1990d604a5917b51c52390f559e00261b00 Mon Sep 17 00:00:00 2001 From: David Cook Date: Tue, 17 Mar 2026 16:36:24 +1100 Subject: [PATCH 27/29] Ensure changes are tracked on newly added variant I considered a few ways to do this. Cloned products are done with MutationObserver but it doesn't quite sit right with me. A dedicated controller for newly added rows would provide a good general solution. But do we want yet another controller? I'm not sure. This works and is pretty simple (although it requires a quick loop over _every_ form element.. let's see if we can avoid that.) --- .../create_linked_variant.turbo_stream.haml | 2 +- .../controllers/variant_controller.js | 7 +++++ spec/system/admin/products_v3/actions_spec.rb | 30 +++++++++++++++++++ 3 files changed, 38 insertions(+), 1 deletion(-) diff --git a/app/views/admin/products_v3/create_linked_variant.turbo_stream.haml b/app/views/admin/products_v3/create_linked_variant.turbo_stream.haml index f67d9227d5..88d5931901 100644 --- a/app/views/admin/products_v3/create_linked_variant.turbo_stream.haml +++ b/app/views/admin/products_v3/create_linked_variant.turbo_stream.haml @@ -9,7 +9,7 @@ category_options:, tax_category_options:}) = turbo_stream.after dom_id(linked_variant) do - %tr.condensed{ id: dom_id(variant), 'data-controller': "variant", 'class': "nested-form-wrapper slide-in" } + %tr.condensed{ id: dom_id(variant), 'data-controller': "variant", 'class': "nested-form-wrapper slide-in",'data-variant-bulk-form-outlet': "#products-form"} = variant_row = turbo_stream.append "flashes" do diff --git a/app/webpacker/controllers/variant_controller.js b/app/webpacker/controllers/variant_controller.js index 5a529a338f..86b10c11c6 100644 --- a/app/webpacker/controllers/variant_controller.js +++ b/app/webpacker/controllers/variant_controller.js @@ -4,6 +4,8 @@ import OptionValueNamer from "js/services/option_value_namer"; // Dynamically update related variant fields // export default class VariantController extends Controller { + static outlets = ["bulk-form"]; + connect() { // idea: create a helper that includes a nice getter/setter for Rails model attr values, just pass it the attribute name. // It could automatically find (and cache a ref to) each dom element and get/set the values. @@ -40,6 +42,11 @@ export default class VariantController extends Controller { // on display_as changed; update unit_to_display // TODO: optimise to avoid unnecessary OptionValueNamer calc this.displayAs.addEventListener("input", this.#updateUnitDisplay.bind(this), { passive: true }); + + // Register with bulk products form to listen for changes. Used when dynamically appending variants. + if (this.hasBulkFormOutlet) { + this.bulkFormOutlet.registerElements(); + } } disconnect() { diff --git a/spec/system/admin/products_v3/actions_spec.rb b/spec/system/admin/products_v3/actions_spec.rb index a885e166e9..16650bd588 100644 --- a/spec/system/admin/products_v3/actions_spec.rb +++ b/spec/system/admin/products_v3/actions_spec.rb @@ -252,6 +252,18 @@ RSpec.describe 'As an enterprise user, I can perform actions on the products scr within "table.products" do # Product list includes the cloned product. expect(all_input_values).to match /COPY OF Apples/ + + # And I can perform actions on the new product + within row_containing_name "COPY OF Apples" do + page.find(".vertical-ellipsis-menu").click + expect(page).to have_link "Edit" + expect(page).to have_link "Clone" + # expect(page).to have_link "Delete" # it's not a proper link :/ + + fill_in "Name", with: "My copy of Apples" + end + + click_button "Save changes" end end end @@ -315,6 +327,24 @@ RSpec.describe 'As an enterprise user, I can perform actions on the products scr expect(all_input_values).to match /My friends box.*My friends box/ # One of them is designated as a linked variant expect(page).to have_content "🔗" + + last_box = page.all(row_containing_name("My friends box")).last + # Close action menu (shouldn't need this, it should close itself) + last_box.click + + # And I can perform actions on the new product + within last_box do + page.find(".vertical-ellipsis-menu").click + expect(page).to have_link "Edit" + # expect(page).to have_link "Clone" # tofix: menu is partially obscured + # expect(page).to have_link "Delete" # it's not a proper link + + fill_in "Name", with: "My copy of Apples" + end + click_button "Save changes" + + # initially obscured by the previous message, then disappears before capybara sees it. + # expect(page).to have_content "Changes saved" end end end From 2004934399d6ca70ccc48ad8ff83497fde45f9c8 Mon Sep 17 00:00:00 2001 From: David Cook Date: Tue, 17 Mar 2026 16:43:29 +1100 Subject: [PATCH 28/29] Register only necessary elements This should be more efficient. Best viewed with whitespace ignored. --- .../controllers/bulk_form_controller.js | 17 +++++++++++------ app/webpacker/controllers/variant_controller.js | 3 ++- 2 files changed, 13 insertions(+), 7 deletions(-) diff --git a/app/webpacker/controllers/bulk_form_controller.js b/app/webpacker/controllers/bulk_form_controller.js index 6e82d2ce33..95eb2c1209 100644 --- a/app/webpacker/controllers/bulk_form_controller.js +++ b/app/webpacker/controllers/bulk_form_controller.js @@ -44,12 +44,17 @@ export default class BulkFormController extends Controller { } // Register any new elements (may be called by another controller after dynamically adding fields) - registerElements() { - const registeredElements = Object.values(this.recordElements).flat(); - // Select only elements that haven't been registered yet - const newElements = Array.from(this.form.elements).filter( - (n) => !registeredElements.includes(n), - ); + // May be called with array of elements to register, otherwise finds all un-registered elements. + registerElements(eventOrElements = null) { + let newElements; + + if (Array.isArray(eventOrElements)) { + newElements = eventOrElements; + } else { + const registeredElements = Object.values(this.recordElements).flat(); + // Select only elements that haven't been registered yet + newElements = Array.from(this.form.elements).filter((n) => !registeredElements.includes(n)); + } this.#registerElements(newElements); } diff --git a/app/webpacker/controllers/variant_controller.js b/app/webpacker/controllers/variant_controller.js index 86b10c11c6..29f24899a4 100644 --- a/app/webpacker/controllers/variant_controller.js +++ b/app/webpacker/controllers/variant_controller.js @@ -45,7 +45,8 @@ export default class VariantController extends Controller { // Register with bulk products form to listen for changes. Used when dynamically appending variants. if (this.hasBulkFormOutlet) { - this.bulkFormOutlet.registerElements(); + const formElements = this.element.querySelectorAll("input, select, textarea, button"); + this.bulkFormOutlet.registerElements(formElements); } } From 8e6f1c4e9932811f4dd43e047849999d791ad594 Mon Sep 17 00:00:00 2001 From: David Cook Date: Wed, 18 Mar 2026 13:37:33 +1100 Subject: [PATCH 29/29] Show display name --- app/views/admin/products_v3/_variant_row.html.haml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/admin/products_v3/_variant_row.html.haml b/app/views/admin/products_v3/_variant_row.html.haml index f5aee06bf7..cd9e8ae6ee 100644 --- a/app/views/admin/products_v3/_variant_row.html.haml +++ b/app/views/admin/products_v3/_variant_row.html.haml @@ -4,7 +4,7 @@ %td.col-image -# empty - variant.source_variants.each do |source_variant| - = content_tag(:span, "🔗", title: t('admin.products_page.variant_row.sourced_from', source_name: source_variant.name, source_id: source_variant.id, hub_name: variant.hub&.name)) + = content_tag(:span, "🔗", title: t('admin.products_page.variant_row.sourced_from', source_name: source_variant.display_name, source_id: source_variant.id, hub_name: variant.hub&.name)) %td.col-name.field.naked_inputs = f.hidden_field :id = f.text_field :display_name, 'aria-label': t('admin.products_page.columns.name'), placeholder: variant.product.name