From 61f8b5c7f4e0790ef8fa258b9fb443ca97e0e79d Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Wed, 22 Oct 2025 14:59:55 +1100 Subject: [PATCH 1/5] Add strict locals for some products V3 templates Rails now allows you to define which local a template is expecting: https://edgeguides.rubyonrails.org/7_1_release_notes.html#allow-templates-to-set-strict-locals --- app/views/admin/products_v3/_content.html.haml | 1 + app/views/admin/products_v3/_product_variant_row.html.haml | 1 + app/views/admin/products_v3/_table.html.haml | 1 + app/views/admin/products_v3/_variant_row.html.haml | 1 + app/views/admin/products_v3/index.html.haml | 1 + 5 files changed, 5 insertions(+) diff --git a/app/views/admin/products_v3/_content.html.haml b/app/views/admin/products_v3/_content.html.haml index 118b470e17..d54148c508 100644 --- a/app/views/admin/products_v3/_content.html.haml +++ b/app/views/admin/products_v3/_content.html.haml @@ -1,3 +1,4 @@ +-# locals: (products:, pagy:, search_term:, producer_options:, producer_id:, category_options:, category_id:, tax_category_options:, available_tags:, tags:, flashes:, display_search_filter:) %turbo-frame#products-content{ target: "_top", refresh: "morph" } .spinner-overlay{ "data-controller": "loading", "data-products-target": "loading", class: "hidden" } .spinner-container 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 f49c756078..5879b8124f 100644 --- a/app/views/admin/products_v3/_product_variant_row.html.haml +++ b/app/views/admin/products_v3/_product_variant_row.html.haml @@ -1,3 +1,4 @@ +-# locals: (form:, product:, product_index:, producer_options:, category_options:, tax_category_options:) = form.fields_for("products", product, index: product_index) do |product_form| %tbody.relaxed{ id: dom_id(product), data: { 'record-id': product_form.object.id, controller: "nested-form product", diff --git a/app/views/admin/products_v3/_table.html.haml b/app/views/admin/products_v3/_table.html.haml index 003194a3fb..92a1ea9393 100644 --- a/app/views/admin/products_v3/_table.html.haml +++ b/app/views/admin/products_v3/_table.html.haml @@ -1,3 +1,4 @@ +-# locals: (products:, producer_options:, category_options:, tax_category_options:) = form_with url: admin_products_bulk_update_path, method: :post, id: "products-form", builder: BulkFormBuilder, html: { data: { 'turbo-frame': "_self", diff --git a/app/views/admin/products_v3/_variant_row.html.haml b/app/views/admin/products_v3/_variant_row.html.haml index 9d8fbeb999..46bdc4ed47 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 @@ +-# locals: (variant:, f:, 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 diff --git a/app/views/admin/products_v3/index.html.haml b/app/views/admin/products_v3/index.html.haml index 38e4ed355b..6193139b5a 100644 --- a/app/views/admin/products_v3/index.html.haml +++ b/app/views/admin/products_v3/index.html.haml @@ -1,3 +1,4 @@ +-# locals: (producers:, categories:, tax_category_options:, available_tags:, flash:) - content_for :page_title do = t('.header.title') - content_for :page_actions do From 25d55fec24ebf61305992c5c6b7699f2eae879b3 Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Wed, 22 Oct 2025 15:06:53 +1100 Subject: [PATCH 2/5] Filter out variant the user is not allowed to update With a product with mutiple variant, we can end in a scenario where a user sees variant associated to producer it doesn't have permission for. This prevents the user from updating any variant. This fix filter out variant a user shoudn't be seeing --- app/controllers/admin/products_v3_controller.rb | 17 +++++++++++------ app/views/admin/products_v3/_content.html.haml | 4 ++-- .../products_v3/_product_variant_row.html.haml | 4 +++- app/views/admin/products_v3/_table.html.haml | 4 ++-- app/views/admin/products_v3/index.html.haml | 8 +++++--- 5 files changed, 23 insertions(+), 14 deletions(-) diff --git a/app/controllers/admin/products_v3_controller.rb b/app/controllers/admin/products_v3_controller.rb index 6d1ae388cb..7656c8709f 100644 --- a/app/controllers/admin/products_v3_controller.rb +++ b/app/controllers/admin/products_v3_controller.rb @@ -11,7 +11,8 @@ module Admin def index fetch_products render "index", - locals: { producers:, categories:, tax_category_options:, available_tags:, flash: } + locals: { producer_options:, categories:, tax_category_options:, available_tags:, + flash:, allowed_producers: } session[:products_return_to_url] = request.url end @@ -32,7 +33,8 @@ module Admin render "index", status: :unprocessable_entity, locals: { - producers:, categories:, tax_category_options:, available_tags:, flash: + producer_options:, categories:, tax_category_options:, available_tags:, + flash: } end end @@ -88,7 +90,7 @@ module Admin flash.now[:success] = t('.success') @product_index = "-#{@cloned_product.id}" - @producer_options = producers + @producer_options = producer_options @category_options = categories @tax_category_options = tax_category_options rescue ActiveRecord::ActiveRecordError => e @@ -132,10 +134,13 @@ module Admin end end - def producers - producers = OpenFoodNetwork::Permissions.new(spree_current_user) + def allowed_producers + OpenFoodNetwork::Permissions.new(spree_current_user) .managed_product_enterprises.is_primary_producer.by_name - producers.map { |p| [p.name, p.id] } + end + + def producer_options + allowed_producers.map { |p| [p.name, p.id] } end def categories diff --git a/app/views/admin/products_v3/_content.html.haml b/app/views/admin/products_v3/_content.html.haml index d54148c508..3a84d074c0 100644 --- a/app/views/admin/products_v3/_content.html.haml +++ b/app/views/admin/products_v3/_content.html.haml @@ -1,4 +1,4 @@ --# locals: (products:, pagy:, search_term:, producer_options:, producer_id:, category_options:, category_id:, tax_category_options:, available_tags:, tags:, flashes:, display_search_filter:) +-# locals: (products:, pagy:, search_term:, producer_options:, producer_id:, category_options:, category_id:, tax_category_options:, available_tags:, tags:, flashes:, display_search_filter:, allowed_producers:) %turbo-frame#products-content{ target: "_top", refresh: "morph" } .spinner-overlay{ "data-controller": "loading", "data-products-target": "loading", class: "hidden" } .spinner-container @@ -18,7 +18,7 @@ .container.results .sixteen.columns = render partial: 'sort', locals: { pagy:, search_term:, producer_id:, category_id:, tags: } - = render partial: 'table', locals: { products:, producer_options:, category_options:, tax_category_options: } + = render partial: 'table', locals: { products:, producer_options:, category_options:, tax_category_options: , allowed_producers: } - if pagy.present? && pagy.pages > 1 = render partial: 'admin/shared/stimulus_pagination', locals: { pagy: pagy } - else 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 5879b8124f..722abb2af7 100644 --- a/app/views/admin/products_v3/_product_variant_row.html.haml +++ b/app/views/admin/products_v3/_product_variant_row.html.haml @@ -1,4 +1,4 @@ --# locals: (form:, product:, product_index:, producer_options:, category_options:, tax_category_options:) +-# locals: (form:, product:, product_index:, producer_options:, category_options:, tax_category_options:, allowed_producers:) = form.fields_for("products", product, index: product_index) do |product_form| %tbody.relaxed{ id: dom_id(product), data: { 'record-id': product_form.object.id, controller: "nested-form product", @@ -8,6 +8,8 @@ = render partial: 'product_row', locals: { f: product_form, product:, product_index: } - product.variants.each_with_index do |variant, variant_index| + + - next unless 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: } diff --git a/app/views/admin/products_v3/_table.html.haml b/app/views/admin/products_v3/_table.html.haml index 92a1ea9393..7d2750a5a9 100644 --- a/app/views/admin/products_v3/_table.html.haml +++ b/app/views/admin/products_v3/_table.html.haml @@ -1,4 +1,4 @@ --# locals: (products:, producer_options:, category_options:, tax_category_options:) +-# locals: (products:, producer_options:, category_options:, tax_category_options:, allowed_producers:) = form_with url: admin_products_bulk_update_path, method: :post, id: "products-form", builder: BulkFormBuilder, html: { data: { 'turbo-frame': "_self", @@ -70,4 +70,4 @@ %th.align-left.col-inherits_properties= t('admin.products_page.columns.inherits_properties') %th.align-right= t('admin.products_page.columns.actions') - products.each_with_index do |product, product_index| - = render partial: 'product_variant_row', locals: { form:, product:, product_index:, producer_options:, category_options:, tax_category_options: } + = render partial: 'product_variant_row', locals: { form:, product:, product_index:, producer_options:, category_options:, tax_category_options: , allowed_producers:} diff --git a/app/views/admin/products_v3/index.html.haml b/app/views/admin/products_v3/index.html.haml index 6193139b5a..04d992e3d4 100644 --- a/app/views/admin/products_v3/index.html.haml +++ b/app/views/admin/products_v3/index.html.haml @@ -1,4 +1,4 @@ --# locals: (producers:, categories:, tax_category_options:, available_tags:, flash:) +-# locals: (producer_options:, categories:, tax_category_options:, available_tags:, flash:, allowed_producers:) - content_for :page_title do = t('.header.title') - content_for :page_actions do @@ -14,11 +14,13 @@ #products_v3_page{ 'data-turbo': true } = render partial: "content", locals: { products: @products, pagy: @pagy, search_term: @search_term, - producer_options: producers, producer_id: @producer_id, + producer_options:, producer_id: @producer_id, category_options: categories, category_id: @category_id, tax_category_options:, available_tags:, tags: @tags, flashes: flash, - display_search_filter: (@products.any? || @search_term.present? || @category_id.present?) } + display_search_filter: (@products.any? || @search_term.present? || @category_id.present?), + allowed_producers:} + - %w[product variant].each do |object_type| = render partial: 'delete_modal', locals: { object_type: } #modal-component From ab443fa50f5ab22ab27a85b0f9b3acfdab878ea3 Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Wed, 22 Oct 2025 15:33:29 +1100 Subject: [PATCH 3/5] Refactor the clone template to use local variable and add define locals on the template --- .../admin/products_v3_controller.rb | 22 ++++++++++--------- .../_product_variant_row.html.haml | 2 +- .../admin/products_v3/clone.turbo_stream.haml | 14 +++++++----- 3 files changed, 21 insertions(+), 17 deletions(-) diff --git a/app/controllers/admin/products_v3_controller.rb b/app/controllers/admin/products_v3_controller.rb index 7656c8709f..c503b5e899 100644 --- a/app/controllers/admin/products_v3_controller.rb +++ b/app/controllers/admin/products_v3_controller.rb @@ -34,7 +34,7 @@ module Admin render "index", status: :unprocessable_entity, locals: { producer_options:, categories:, tax_category_options:, available_tags:, - flash: + allowed_producers:, flash: } end end @@ -80,27 +80,29 @@ module Admin end def clone - @product = Spree::Product.find(params[:id]) - authorize! :clone, @product + product = Spree::Product.find(params[:id]) + authorize! :clone, product status = :ok begin - @cloned_product = @product.duplicate + cloned_product = product.duplicate flash.now[:success] = t('.success') - @product_index = "-#{@cloned_product.id}" - @producer_options = producer_options - @category_options = categories - @tax_category_options = tax_category_options + product_index = "-#{cloned_product.id}" rescue ActiveRecord::ActiveRecordError => e flash.now[:error] = clone_error_message(e) status = :unprocessable_entity - @product_index = "-1" # Create a unique enough index + product_index = "-1" # Create a unique enough index end respond_with do |format| - format.turbo_stream { render :clone, status: } + format.turbo_stream { + render :clone, status:, + locals: { product:, cloned_product:, product_index:, producer_options:, + category_options: categories, tax_category_options:, + allowed_producers: } + } end 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 722abb2af7..616eb8268f 100644 --- a/app/views/admin/products_v3/_product_variant_row.html.haml +++ b/app/views/admin/products_v3/_product_variant_row.html.haml @@ -1,4 +1,4 @@ --# locals: (form:, product:, product_index:, producer_options:, category_options:, tax_category_options:, allowed_producers:) +-# locals: (form:, product:, product_index:, producer_options:, category_options:, tax_category_options:, allowed_producers:, should_slide_in: false) = form.fields_for("products", product, index: product_index) do |product_form| %tbody.relaxed{ id: dom_id(product), data: { 'record-id': product_form.object.id, controller: "nested-form product", diff --git a/app/views/admin/products_v3/clone.turbo_stream.haml b/app/views/admin/products_v3/clone.turbo_stream.haml index 94d39c1acd..a4ba827897 100644 --- a/app/views/admin/products_v3/clone.turbo_stream.haml +++ b/app/views/admin/products_v3/clone.turbo_stream.haml @@ -1,16 +1,18 @@ +-# locals: (product:, cloned_product:, product_index:, producer_options:, category_options: category, tax_category_options:, allowed_producers:) - unless flash[:error] - product_body = nil - form_with do |form| - product_body = render(partial: 'product_variant_row', locals: { form:, - product: @cloned_product, - product_index: @product_index, - producer_options: @producer_options, - category_options: @category_options, - tax_category_options: @tax_category_options, + product: cloned_product, + product_index:, + producer_options:, + category_options:, + tax_category_options:, + allowed_producers:, should_slide_in: true }, formats: :html) - = turbo_stream.after dom_id(@product) do + = turbo_stream.after dom_id(product) do = product_body = turbo_stream.append "flashes" do From e8b81c1ff68ef17a5ca7f582e76a63bfb3f77059 Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Wed, 22 Oct 2025 16:48:16 +1100 Subject: [PATCH 4/5] Fix variant filtering We don't want to filter out variant missing producer, so that the user can address the problem. --- app/views/admin/products_v3/_product_variant_row.html.haml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) 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 616eb8268f..c2869deb3f 100644 --- a/app/views/admin/products_v3/_product_variant_row.html.haml +++ b/app/views/admin/products_v3/_product_variant_row.html.haml @@ -9,7 +9,8 @@ - product.variants.each_with_index do |variant, variant_index| - - next unless allowed_producers.include?(variant.supplier) + -# 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, category_options:, tax_category_options:, producer_options: } From 460d109bd291621a2a684c5cce29e562cd1faf20 Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Tue, 11 Nov 2025 11:35:19 +1100 Subject: [PATCH 5/5] Update product ability A user has product permission if it is a supplier of at least one of the product's variants --- app/models/spree/ability.rb | 7 +++++-- spec/models/spree/ability_spec.rb | 23 +++++++++++++++++++++++ 2 files changed, 28 insertions(+), 2 deletions(-) diff --git a/app/models/spree/ability.rb b/app/models/spree/ability.rb index 155f611d56..63833e309f 100644 --- a/app/models/spree/ability.rb +++ b/app/models/spree/ability.rb @@ -196,12 +196,15 @@ 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 + # one of the product's associated variants can [:admin, :read, :index, :update, :seo, :group_buy_options, :bulk_update, :clone, :delete, :destroy], Spree::Product do |product| - OpenFoodNetwork::Permissions.new(user).managed_product_enterprises.include?( - product.variants.first.supplier + variant_suppliers = product.variants.map(&:supplier) + OpenFoodNetwork::Permissions.new(user).managed_product_enterprises.intersect?( + variant_suppliers ) end diff --git a/spec/models/spree/ability_spec.rb b/spec/models/spree/ability_spec.rb index 42e0300b36..8d0aab1c4c 100644 --- a/spec/models/spree/ability_spec.rb +++ b/spec/models/spree/ability_spec.rb @@ -324,6 +324,29 @@ RSpec.describe Spree::Ability do ) end + context "with mutiple variant with different supplier" do + let(:product1) { create(:product, supplier_id: create(:supplier_enterprise).id) } + let(:product1_other_variant) { create(:variant, product: product1, supplier: s1) } + + it "is able to read/write their enterprises' products and variants" do + product1_other_variant + + is_expected.to have_ability([:admin, :read, :update, :bulk_update, :clone, :destroy], + for: product1) + + is_expected.to have_ability( + [:admin, :index, :read, :edit, :update, :search, :destroy, + :delete], for: product1.variants.last + ) + + # First variant belongs to another supplier + is_expected.not_to have_ability( + [:admin, :index, :read, :edit, :update, :search, :destroy, + :delete], for: product1.variants.first + ) + end + end + it "should be able to read/write related enterprises' products " \ "and variants with manage_products permission" do er_ps