Merge pull request #12441 from cyrillefr/DeleteButtonMissingBeforeSavingVariant

[BUU] Delete Button missing before saving variant

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

@@ -49,7 +49,7 @@
                   = form.submit t('.save'), class: "medium"
       %tr
         %th.align-left= # image
-        = render partial: 'spree/admin/shared/stimulus_sortable_header', 
+        = render partial: 'spree/admin/shared/stimulus_sortable_header',
             locals: { column: :name, sorted: params.dig(:q, :s), default: 'name asc' }
         %th.align-left.with-input= t('admin.products_page.columns.sku')
         %th.align-left.with-input= t('admin.products_page.columns.unit_scale')
@@ -65,19 +65,19 @@
       = form.fields_for("products", product, index: product_index) do |product_form|
         %tbody.relaxed{ data: { 'record-id': product_form.object.id,
                                 controller: "nested-form product",
-                                action: 'rails-nested-form:add->bulk-form#registerElements' } }
+                                action: 'rails-nested-form:add->bulk-form#registerElements rails-nested-form:remove->bulk-form#toggleFormChanged' } }
           %tr
             = render partial: 'product_row', locals: { f: product_form, product:, producer_options: }
 
           - product.variants.each_with_index do |variant, variant_index|
             = form.fields_for("products][#{product_index}][variants_attributes][", variant, index: variant_index) do |variant_form|
-              %tr.condensed{ 'data-controller': "variant" }
+              %tr.condensed{ '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: }
 
           = form.fields_for("products][#{product_index}][variants_attributes][NEW_RECORD", product.variants.build) do |new_variant_form|
             %template{ 'data-nested-form-target': "template" }
-              %tr.condensed{ 'data-controller': "variant" }
-                = render partial: 'variant_row', locals: { variant: new_variant_form.object, f: new_variant_form, category_options:, tax_category_options: }
+              %tr.condensed{ 'data-controller': "variant", 'class': "nested-form-wrapper", 'data-new-record': "true" }
+                = render partial: 'variant_row', locals: { variant: new_variant_form.object, f: new_variant_form, category_options:, tax_category_options:  }
 
           %tr{ 'data-nested-form-target': "target" }
           %tr.condensed

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

@@ -59,11 +59,14 @@
 %td.align-left
   -# empty
 %td.align-right
-  - if variant.persisted?
-    = render(VerticalEllipsisMenu::Component.new) do
+  = render(VerticalEllipsisMenu::Component.new) do
+    - if variant.persisted?
       = link_to t('admin.products_page.actions.edit'), edit_admin_product_variant_path(variant.product, variant)
       - 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",
             "data-modal-link-modal-dataset-value": {'data-current-id': variant.id}.to_json }
           = t('admin.products_page.actions.delete')
+    - else
+      %a{ 'data-action': "nested-form#remove", class: 'delete' }
+        = t('admin.products_page.actions.remove')

app/webpacker/controllers/bulk_form_controller.js

@@ -132,8 +132,12 @@ export default class BulkFormController extends Controller {
   }
 
   #isChanged(element) {
-    if (element.type == "checkbox") {
+    if(!element.isConnected) {
+      return false;
+
+    } else if (element.type == "checkbox") {
       return element.defaultChecked !== undefined && element.checked != element.defaultChecked;
+
     } else if (element.type == "select-one") {
       // (weird) Behavior of select element's include_blank option in Rails:
       //   If a select field has include_blank option selected (its value will be ''),
@@ -146,6 +150,7 @@ export default class BulkFormController extends Controller {
       const areBothBlank = selectedOption.value === '' && defaultSelected === undefined
 
       return !areBothBlank && selectedOption !== defaultSelected;
+
     } else {
       return element.defaultValue !== undefined && element.value != element.defaultValue;
     }

config/locales/en.yml

@@ -601,6 +601,7 @@ en:
         edit: Edit
         clone: Clone
         delete: Delete
+        remove: Remove
       image:
         edit: Edit
     adjustments:

spec/system/admin/products_v3/products_spec.rb

@@ -216,6 +216,16 @@ RSpec.describe 'As an enterprise user, I can manage my products', feature: :admi
       create(:simple_product, name: "Apples", sku: "APL-00",
                               variant_unit: "weight", variant_unit_scale: 1) # Grams
     }
+    let(:variant_b1) {
+      product_b.variants.first.tap{ |v|
+        v.update! display_name: "Medium box", sku: "TMT-01", price: 5, on_hand: 5,
+                  on_demand: false
+      }
+    }
+    let(:product_b) {
+      create(:simple_product, name: "Tomatoes", sku: "TMT-01",
+                              variant_unit: "weight", variant_unit_scale: 1) # Grams
+    }
     before do
       visit admin_products_url
     end
@@ -546,6 +556,107 @@ RSpec.describe 'As an enterprise user, I can manage my products', feature: :admi
         end
       end
 
+      it 'removes a newly added not persisted variant' do
+        click_on "New variant"
+        new_variant_row = find_field("Name", placeholder: "Apples", with: "").ancestor("tr")
+        within new_variant_row do
+          fill_in "Name", with: "Large box"
+          fill_in "SKU", with: "APL-02"
+          expect(page).to have_field("Name", placeholder: "Apples", with: "Large box")
+        end
+
+        expect(page).to have_text("1 product modified.")
+        expect(page).to have_css('form.disabled-section#filters') # ie search/sort disabled
+
+        within new_variant_row do
+          page.find(".vertical-ellipsis-menu").click
+          page.find('a', text: 'Remove').click
+        end
+
+        expect(page).not_to have_field("Name", placeholder: "Apples", with: "Large box")
+        expect(page).not_to have_text("1 product modified.")
+        expect(page).not_to have_css('form.disabled-section#filters')
+      end
+
+      it "removes newly added not persistent Variants one at a time" do
+        click_on "New variant"
+
+        first_new_variant_row = find_field("Name", placeholder: "Apples", with: "").ancestor("tr")
+        within first_new_variant_row do
+          fill_in "Name", with: "Large box"
+        end
+
+        click_on "New variant"
+        second_new_variant_row = find_field("Name", placeholder: "Apples", with: "").ancestor("tr")
+        within second_new_variant_row do
+          fill_in "Name", with: "Huge box"
+        end
+
+        expect(page).to have_text("1 product modified.")
+        expect(page).to have_css('form.disabled-section#filters')
+
+        within first_new_variant_row do
+          page.find(".vertical-ellipsis-menu").click
+          page.find('a', text: 'Remove').click
+        end
+
+        expect(page).to have_text("1 product modified.")
+
+        within second_new_variant_row do
+          page.find(".vertical-ellipsis-menu").click
+          page.find('a', text: 'Remove').click
+        end
+        # Only when all non persistent variants are gone that product is non modified
+        expect(page).not_to have_text("1 product modified.")
+        expect(page).not_to have_css('form.disabled-section#filters')
+      end
+
+      context "With 2 products" do
+        before do
+          variant_b1
+          # To add 2nd product on page
+          page.refresh
+        end
+
+        it "removes newly added Variants across products" do
+          click_on "New variant"
+          apples_new_variant_row =
+            find_field("Name", placeholder: "Apples", with: "").ancestor("tr")
+          within apples_new_variant_row do
+            fill_in "Name", with: "Large box"
+          end
+
+          tomatoes_part = page.all('tbody')[1]
+          within tomatoes_part do
+            click_on "New variant"
+          end
+          tomatoes_new_variant_row =
+            find_field("Name", placeholder: "Tomatoes", with: "").ancestor("tr")
+          within tomatoes_new_variant_row do
+            fill_in "Name", with: "Huge box"
+          end
+          expect(page).to have_text("2 products modified.")
+          expect(page).to have_css('form.disabled-section#filters') # ie search/sort disabled
+
+          within apples_new_variant_row do
+            page.find(".vertical-ellipsis-menu").click
+            page.find('a', text: 'Remove').click
+          end
+          # New variant for apples is no more, expect only 1 modified product
+          expect(page).to have_text("1 product modified.")
+          # search/sort still disabled
+          expect(page).to have_css('form.disabled-section#filters')
+
+          within tomatoes_new_variant_row do
+            page.find(".vertical-ellipsis-menu").click
+            page.find('a', text: 'Remove').click
+          end
+          # Back to page without any alteration
+          expect(page).not_to have_text("1 product modified.")
+          expect(page).not_to have_css('form.disabled-section#filters')
+        end
+      end
+
       context "with invalid data" do
         before do
           click_on "New variant"
@@ -624,6 +735,22 @@ RSpec.describe 'As an enterprise user, I can manage my products', feature: :admi
           expect(new_variant.price).to eq 10.25
           expect(new_variant.unit_value).to eq 200
         end
+
+        it "removes unsaved record" do
+          click_button "Save changes"
+
+          expect(page).to have_text("1 product could not be saved.")
+
+          within row_containing_name("N" * 256) do
+            page.find(".vertical-ellipsis-menu").click
+            page.find('a', text: 'Remove').click
+          end
+
+          # Now that invalid variant is removed, we can proceed to save
+          click_button "Save changes"
+          expect(page).not_to have_text("1 product could not be saved.")
+          expect(page).not_to have_css('form.disabled-section#filters')
+        end
       end
     end