Unified Handling of Option Values and Product Properties List

backend/app/assets/javascripts/spree/backend/admin.js

@@ -58,6 +58,14 @@ Spree.ready(function(){
   $('.spree_add_fields').click(function() {
     var target = $(this).data("target");
     var new_table_row = $(target + ' tr:visible:last').clone();
+
+    // remove id
+    new_table_row.attr("id", "");
+
+    // Remove sort handle
+    new_table_row.find("td").first().empty();
+
+    // set unique form ids and names of new item
     var new_id = new Date().getTime() + (uniqueId++);
     new_table_row.find("input, select").each(function () {
       var el = $(this);
@@ -66,14 +74,11 @@ Spree.ready(function(){
       el.prop("id", el.prop("id").replace(/\d+(?=[^\d]*$)/, new_id))
       el.prop("name", el.prop("name").replace(/\d+(?=[^\d]*$)/, new_id))
     })
-    // When cloning a new row, set the href of all icons to be an empty "#"
-    // This is so that clicking on them does not perform the actions for the
-    // duplicated row
-    new_table_row.find("a").each(function () {
-      var el = $(this);
-      el.prop('href', '#');
-    })
-    $(target).prepend(new_table_row);
+
+    // Add a remove button to the actions column of the new row
+    new_table_row.find("td").last().empty().append('<a class="spree_remove_fields no-text with-tip fa fa-trash icon_link with-tip" data-action="remove" href="#" data-original-title="Remove"><span class="text"></span></a>');
+
+    $(target).append(new_table_row);
   })
 
   $('body').on('click', '.delete-resource', function() {
@@ -101,26 +106,10 @@ Spree.ready(function(){
 
   $('body').on('click', 'a.spree_remove_fields', function() {
     var el = $(this);
-    el.prev("input[type=hidden]").val("1");
-    el.closest(".fields").hide();
-    if (el.prop("href").substr(-1) == '#') {
-      el.parents("tr").fadeOut('hide');
-    }else if (el.prop("href")) {
-      Spree.ajax({
-        type: 'POST',
-        url: el.prop("href"),
-        data: {
-          _method: 'delete',
-        },
-        success: function(response) {
-          el.parents("tr").fadeOut('hide');
-        },
-        error: function(response, textStatus, errorThrown) {
-          show_flash('error', response.responseText);
-        }
-
-      })
-    }
+    var table_row = el.parents("tr").first();
+    table_row.fadeOut("hide", function() {
+      table_row.remove();
+    });
     return false;
   });
 

backend/app/controllers/spree/admin/product_properties_controller.rb

@@ -15,7 +15,7 @@ def find_properties
       end
 
       def setup_property
-        @product.product_properties.build
+        @product.product_properties.build if @product.product_properties.empty?
       end
 
       def setup_variant_property_rules

backend/app/helpers/spree/admin/base_helper.rb

@@ -132,6 +132,8 @@ def link_to_remove_fields(name, form, options = {})
         link_to_with_icon('trash', name, url, class: "spree_remove_fields #{options[:class]}", data: { action: 'remove' }, title: t('spree.actions.remove')) +
           form.hidden_field(:_destroy)
       end
+      deprecate link_to_remove_fields: "Please use link_to_delete instead, Example: \n" \
+        "link_to_delete \"form.object\"", deprecator: Spree::Deprecation
 
       def spree_dom_id(record)
         dom_id(record, 'spree')

backend/app/views/spree/admin/option_types/_option_value_fields.html.erb

@@ -7,5 +7,12 @@
   </td>
   <td class="name"><%= f.text_field :name %></td>
   <td class="presentation"><%= f.text_field :presentation %></td>
-  <td class="actions"><%= link_to_remove_fields t('spree.actions.remove'), f, no_text: true %></td>
+  <td class="actions">
+    <% if f.object.persisted? %>
+      <%= link_to_delete f.object, no_text: true %>
+    <% else %>
+      <%= link_to_with_icon('trash', 'remove', '#', no_text: true, class: "spree_remove_fields",
+            data: { action: 'remove' }, title: t('spree.actions.remove')) %>
+    <% end %>
+  </td>
 </tr>

backend/app/views/spree/admin/product_properties/_product_property_fields.html.erb

@@ -12,8 +12,13 @@
     <%= f.text_field :value %>
   </td>
   <td class="actions">
-    <% if f.object.persisted? && can?(:destroy, f.object) %>
-      <%= link_to_delete f.object, no_text: true %>
+    <% if f.object.persisted? %>
+      <% if can?(:destroy, f.object) %>
+        <%= link_to_delete f.object, no_text: true %>
+      <% end %>
+    <% else %>
+      <%= link_to_with_icon('trash', 'remove', '#', no_text: true, class: "spree_remove_fields",
+            data: { action: 'remove' }, title: t('spree.actions.remove')) %>
     <% end %>
   </td>
 </tr>

backend/spec/features/admin/products/option_types_spec.rb

@@ -66,7 +66,9 @@
     expect(page).to have_title("#{option_value.option_type.name} - Option Types - Products")
     expect(page).to have_css("tbody#option_values tr", count: 1)
     within("tbody#option_values") do
-      find('.spree_remove_fields').click
+      accept_alert do
+        find('.fa-trash').click
+      end
     end
     # Assert that the field is hidden automatically
     expect(page).to have_no_css("tbody#option_values tr")

backend/spec/features/admin/products/properties_spec.rb

@@ -88,23 +88,25 @@
     it "successfully create and then remove product property" do
       fill_in_property
 
-      check_property_row_count(2)
+      check_persisted_property_row_count(1)
 
       delete_product_property
 
-      check_property_row_count(1)
+      check_persisted_property_row_count(0)
     end
 
     # Regression test for https://github.com/spree/spree/issues/4466
     it "successfully remove and create a product property at the same time" do
       fill_in_property
 
-      expect(page).to have_css('tr.product_property', count: 2)
+      expect(page).to have_css('tr.product_property', count: 1)
 
-      within '#spree_new_product_property' do
+      click_button "Add Product Properties"
+      within '.product_property:first-child' do
         find('[id$=_property_name]').set("New Property")
         find('[id$=_value]').set("New Value")
       end
+      expect(page).to have_css('tr.product_property', count: 2)
 
       delete_product_property
 
@@ -115,7 +117,7 @@
 
       expect(page).not_to have_content("Product is not found")
 
-      check_property_row_count(2)
+      check_persisted_property_row_count(0)
     end
 
     def fill_in_property
@@ -128,14 +130,14 @@ def fill_in_property
 
     def delete_product_property
       accept_alert do
-        click_icon :trash
+        within_row(1) { click_icon :trash }
       end
       expect(page).to have_content 'successfully removed'
     end
 
-    def check_property_row_count(expected_row_count)
+    def check_persisted_property_row_count(expected_row_count)
       click_link "Product Properties"
-      expect(page).to have_css("tbody#product_properties tr", count: expected_row_count)
+      expect(page).to have_css("tbody#product_properties tr:not(#spree_new_product_property)", count: expected_row_count)
     end
   end
 end