Merge pull request #13650 from rioug/13539-update-variant-multi-producer

Fix update multi producer products

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:,
+                          allowed_producers:, flash:
                         }
       end
     end
@@ -78,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 = producers
-        @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
 
@@ -132,10 +136,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

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
 

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:, allowed_producers:)
 %turbo-frame#products-content{ target: "_top", refresh: "morph" }
   .spinner-overlay{ "data-controller": "loading", "data-products-target": "loading", class: "hidden" }
     .spinner-container
@@ -17,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

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:, 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",
@@ -7,6 +8,9 @@
       = render partial: 'product_row', locals: { f: product_form, product:, product_index: }
 
     - product.variants.each_with_index do |variant, variant_index|
+
+      -# 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: }

app/views/admin/products_v3/_table.html.haml

@@ -1,3 +1,4 @@
+-# 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",
@@ -69,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:}

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

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

app/views/admin/products_v3/index.html.haml

@@ -1,3 +1,4 @@
+-# 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
@@ -13,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

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