From 9b10b73a6517b4c222e91962f003786fb51611dd Mon Sep 17 00:00:00 2001
From: cyrillefr <cyrille.vasseur@gmail.com>
Date: Mon, 6 May 2024 13:11:17 +0200
Subject: [PATCH 1/6] Delete Button missing before saving variant

 - used the remove option of stimulus-components/rails-nested-form lib
 - add the Remove option in the menu
 - the corresponding spec
 - the locale
---
 app/views/admin/products_v3/_table.html.haml       |  4 ++--
 app/views/admin/products_v3/_variant_row.html.haml |  8 ++++++--
 config/locales/en.yml                              |  1 +
 spec/system/admin/products_v3/products_spec.rb     | 14 ++++++++++++++
 4 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/app/views/admin/products_v3/_table.html.haml b/app/views/admin/products_v3/_table.html.haml
index 23f6fb5fb4..c938771099 100644
--- a/app/views/admin/products_v3/_table.html.haml
+++ b/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')
@@ -76,7 +76,7 @@
 
           = 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" }
+              %tr.condensed{ 'data-controller': "variant", 'class': "nested-form-wrapper" }
                 = 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" }
diff --git a/app/views/admin/products_v3/_variant_row.html.haml b/app/views/admin/products_v3/_variant_row.html.haml
index 53ea27aad5..9c131c77d7 100644
--- a/app/views/admin/products_v3/_variant_row.html.haml
+++ b/app/views/admin/products_v3/_variant_row.html.haml
@@ -59,11 +59,15 @@
 %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" }
+        = f.hidden_field :_destroy
+        = t('admin.products_page.actions.remove')
diff --git a/config/locales/en.yml b/config/locales/en.yml
index 46197090c0..5adbcc7c0d 100644
--- a/config/locales/en.yml
+++ b/config/locales/en.yml
@@ -601,6 +601,7 @@ en:
         edit: Edit
         clone: Clone
         delete: Delete
+        remove: Remove
       image:
         edit: Edit
     adjustments:
diff --git a/spec/system/admin/products_v3/products_spec.rb b/spec/system/admin/products_v3/products_spec.rb
index 344df65c1b..5ed433daa3 100644
--- a/spec/system/admin/products_v3/products_spec.rb
+++ b/spec/system/admin/products_v3/products_spec.rb
@@ -546,6 +546,20 @@ 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")
+          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")
+      end
+
       context "with invalid data" do
         before do
           click_on "New variant"

From 36f3e4af02cfbfdbd26b4ecf1b66586d16c7bde4 Mon Sep 17 00:00:00 2001
From: cyrillefr <cyrille.vasseur@gmail.com>
Date: Tue, 7 May 2024 09:57:14 +0200
Subject: [PATCH 2/6] Requested changes on Delete button missing

- cleaned haml/html
---
 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 9c131c77d7..7136d16b79 100644
--- a/app/views/admin/products_v3/_variant_row.html.haml
+++ b/app/views/admin/products_v3/_variant_row.html.haml
@@ -68,6 +68,6 @@
             "data-modal-link-modal-dataset-value": {'data-current-id': variant.id}.to_json }
           = t('admin.products_page.actions.delete')
     - else
+      = f.hidden_field :_destroy
       %a{ 'data-action': "nested-form#remove" }
-        = f.hidden_field :_destroy
         = t('admin.products_page.actions.remove')

From 808f1c65f233286b1675c662999523fc91ceca11 Mon Sep 17 00:00:00 2001
From: cyrillefr <cyrille.vasseur@gmail.com>
Date: Mon, 13 May 2024 15:26:59 +0200
Subject: [PATCH 3/6] Requested changes on Delete Button missing

- Styling(in red) for the remove button/link in view

- A remove method to the bulk_form controller
    - removes elements from the Dom
    - removes changed elements from the binded Array in controller
    - so that menu that indicates changes disappear and blured elements
    - resume to non blurring state

- Added the corresponding specs
    - test with one, two variants
    - test with two different products
---
 .../admin/products_v3/_variant_row.html.haml  |  2 +-
 .../controllers/bulk_form_controller.js       | 34 +++++++
 .../system/admin/products_v3/products_spec.rb | 97 +++++++++++++++++++
 3 files changed, 132 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 7136d16b79..7255683347 100644
--- a/app/views/admin/products_v3/_variant_row.html.haml
+++ b/app/views/admin/products_v3/_variant_row.html.haml
@@ -69,5 +69,5 @@
           = t('admin.products_page.actions.delete')
     - else
       = f.hidden_field :_destroy
-      %a{ 'data-action': "nested-form#remove" }
+      %a{ 'data-action': "nested-form#remove bulk-form#remove", class: 'delete', 'data-temp_id': f.field_id(:remove_link) }
         = t('admin.products_page.actions.remove')
diff --git a/app/webpacker/controllers/bulk_form_controller.js b/app/webpacker/controllers/bulk_form_controller.js
index 54a8a812df..c3c73294ae 100644
--- a/app/webpacker/controllers/bulk_form_controller.js
+++ b/app/webpacker/controllers/bulk_form_controller.js
@@ -91,6 +91,40 @@ export default class BulkFormController extends Controller {
     }
   }
 
+  // Removes tr from DOM as well as corresponding elmnts from recordElement
+  // when clicking on 'Remove' button. For not persisted rows removed by
+  // stimulus components lib.
+  remove(event) {
+    // We need the temp id for the newly non persisted variant
+    let elmnt = event.target;
+    const recordContainer = elmnt.closest("[data-record-id]");
+    let recordId = recordContainer.dataset.recordId;
+    let elmnt_temp_id = elmnt.dataset.temp_id;
+    const re = /([0-9]*)_remove_link/;
+    let temp_id = elmnt_temp_id.match(re)[1]
+
+    // Store indexes and not elements
+    let elmntsToDelete = [];
+    this.recordElements[recordId].forEach((item, index) => {
+      if ((item.id).includes(temp_id)) {
+        elmntsToDelete.push(index);
+      }
+    });
+
+
+    // Want to delete ary.last - ary.first ie from to first up to last index
+    // In this way, we also delete unnecessary elements without reference to temp variant(id)
+    // like buttons but nonetheless part of the elements to be removed.
+    this.recordElements[recordId]
+      .splice(elmntsToDelete[0], elmntsToDelete[elmntsToDelete.length - 1] - elmntsToDelete[0] + 1);
+    this.toggleFormChanged();
+
+    // Otherwise, elements within tr may be re-added to recordElements.
+    // With the nested-form-wrapper(stimulus components) class added to a tr.
+    let tr = document.querySelector('.nested-form-wrapper[style="display: none;"]')
+    tr.remove();
+  }
+
   // private
 
   #registerSubmit() {
diff --git a/spec/system/admin/products_v3/products_spec.rb b/spec/system/admin/products_v3/products_spec.rb
index 5ed433daa3..8665039568 100644
--- a/spec/system/admin/products_v3/products_spec.rb
+++ b/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
@@ -553,11 +563,98 @@ RSpec.describe 'As an enterprise user, I can manage my products', feature: :admi
           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

From b45df8a723e4064a77135558d24f460a8f9780d1 Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Tue, 14 May 2024 10:17:50 +1000
Subject: [PATCH 4/6] Use built-in feature to delete new record row

I looked at the source code and found that we were missing one detail: data-new-record.
https://github.com/stimulus-components/stimulus-rails-nested-form/blob/master/src/index.ts#L32-L35

It's documented here, but it's easy to miss: https://www.stimulus-components.com/docs/stimulus-rails-nested-form/
---
 app/views/admin/products_v3/_table.html.haml      | 4 ++--
 app/webpacker/controllers/bulk_form_controller.js | 5 -----
 2 files changed, 2 insertions(+), 7 deletions(-)

diff --git a/app/views/admin/products_v3/_table.html.haml b/app/views/admin/products_v3/_table.html.haml
index c938771099..ec0159ec5c 100644
--- a/app/views/admin/products_v3/_table.html.haml
+++ b/app/views/admin/products_v3/_table.html.haml
@@ -76,8 +76,8 @@
 
           = 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", 'class': "nested-form-wrapper" }
-                = 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
diff --git a/app/webpacker/controllers/bulk_form_controller.js b/app/webpacker/controllers/bulk_form_controller.js
index c3c73294ae..511d058ddc 100644
--- a/app/webpacker/controllers/bulk_form_controller.js
+++ b/app/webpacker/controllers/bulk_form_controller.js
@@ -118,11 +118,6 @@ export default class BulkFormController extends Controller {
     this.recordElements[recordId]
       .splice(elmntsToDelete[0], elmntsToDelete[elmntsToDelete.length - 1] - elmntsToDelete[0] + 1);
     this.toggleFormChanged();
-
-    // Otherwise, elements within tr may be re-added to recordElements.
-    // With the nested-form-wrapper(stimulus components) class added to a tr.
-    let tr = document.querySelector('.nested-form-wrapper[style="display: none;"]')
-    tr.remove();
   }
 
   // private

From da77994a8960e5a2262999a0dab9006b7b88e108 Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Tue, 14 May 2024 10:39:14 +1000
Subject: [PATCH 5/6] Ignore disconnected form elements

When elements are removed from the DOM, they remain in the recordElements array. But we can simply ignore them.
We have to wait until after rails-nested-form:remove is completed before toggleFormChanged.

hmm It would be even better to remove them from the array..
---
 app/views/admin/products_v3/_table.html.haml  |  2 +-
 .../admin/products_v3/_variant_row.html.haml  |  3 +-
 .../controllers/bulk_form_controller.js       | 36 ++++---------------
 3 files changed, 8 insertions(+), 33 deletions(-)

diff --git a/app/views/admin/products_v3/_table.html.haml b/app/views/admin/products_v3/_table.html.haml
index ec0159ec5c..20fd3311ef 100644
--- a/app/views/admin/products_v3/_table.html.haml
+++ b/app/views/admin/products_v3/_table.html.haml
@@ -65,7 +65,7 @@
       = 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: }
 
diff --git a/app/views/admin/products_v3/_variant_row.html.haml b/app/views/admin/products_v3/_variant_row.html.haml
index 7255683347..040d77ce44 100644
--- a/app/views/admin/products_v3/_variant_row.html.haml
+++ b/app/views/admin/products_v3/_variant_row.html.haml
@@ -68,6 +68,5 @@
             "data-modal-link-modal-dataset-value": {'data-current-id': variant.id}.to_json }
           = t('admin.products_page.actions.delete')
     - else
-      = f.hidden_field :_destroy
-      %a{ 'data-action': "nested-form#remove bulk-form#remove", class: 'delete', 'data-temp_id': f.field_id(:remove_link) }
+      %a{ 'data-action': "nested-form#remove", class: 'delete' }
         = t('admin.products_page.actions.remove')
diff --git a/app/webpacker/controllers/bulk_form_controller.js b/app/webpacker/controllers/bulk_form_controller.js
index 511d058ddc..0af566b271 100644
--- a/app/webpacker/controllers/bulk_form_controller.js
+++ b/app/webpacker/controllers/bulk_form_controller.js
@@ -91,35 +91,6 @@ export default class BulkFormController extends Controller {
     }
   }
 
-  // Removes tr from DOM as well as corresponding elmnts from recordElement
-  // when clicking on 'Remove' button. For not persisted rows removed by
-  // stimulus components lib.
-  remove(event) {
-    // We need the temp id for the newly non persisted variant
-    let elmnt = event.target;
-    const recordContainer = elmnt.closest("[data-record-id]");
-    let recordId = recordContainer.dataset.recordId;
-    let elmnt_temp_id = elmnt.dataset.temp_id;
-    const re = /([0-9]*)_remove_link/;
-    let temp_id = elmnt_temp_id.match(re)[1]
-
-    // Store indexes and not elements
-    let elmntsToDelete = [];
-    this.recordElements[recordId].forEach((item, index) => {
-      if ((item.id).includes(temp_id)) {
-        elmntsToDelete.push(index);
-      }
-    });
-
-
-    // Want to delete ary.last - ary.first ie from to first up to last index
-    // In this way, we also delete unnecessary elements without reference to temp variant(id)
-    // like buttons but nonetheless part of the elements to be removed.
-    this.recordElements[recordId]
-      .splice(elmntsToDelete[0], elmntsToDelete[elmntsToDelete.length - 1] - elmntsToDelete[0] + 1);
-    this.toggleFormChanged();
-  }
-
   // private
 
   #registerSubmit() {
@@ -161,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 ''),
@@ -175,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;
     }

From 00f6d01738c9680ed24168412e1b760889f4cc0c Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Tue, 14 May 2024 11:06:25 +1000
Subject: [PATCH 6/6] Remove errored variants

I found another case.
---
 app/views/admin/products_v3/_table.html.haml   |  2 +-
 spec/system/admin/products_v3/products_spec.rb | 16 ++++++++++++++++
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/app/views/admin/products_v3/_table.html.haml b/app/views/admin/products_v3/_table.html.haml
index 20fd3311ef..27d5d7434f 100644
--- a/app/views/admin/products_v3/_table.html.haml
+++ b/app/views/admin/products_v3/_table.html.haml
@@ -71,7 +71,7 @@
 
           - 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|
diff --git a/spec/system/admin/products_v3/products_spec.rb b/spec/system/admin/products_v3/products_spec.rb
index 8665039568..757be44d02 100644
--- a/spec/system/admin/products_v3/products_spec.rb
+++ b/spec/system/admin/products_v3/products_spec.rb
@@ -735,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