From 163b3e8640fe31df544b613529cafb3fa0759d9d Mon Sep 17 00:00:00 2001 From: Stefan Date: Sat, 6 Jun 2020 18:57:59 +0200 Subject: [PATCH 1/6] Only setup non persistent product property row if no product properties present --- .../admin/product_properties_controller.rb | 2 +- .../admin/products/properties_spec.rb | 21 ++++++++++++------- 2 files changed, 14 insertions(+), 9 deletions(-) diff --git a/backend/app/controllers/spree/admin/product_properties_controller.rb b/backend/app/controllers/spree/admin/product_properties_controller.rb index 256d80d65a6..248ae76d8a7 100644 --- a/backend/app/controllers/spree/admin/product_properties_controller.rb +++ b/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 diff --git a/backend/spec/features/admin/products/properties_spec.rb b/backend/spec/features/admin/products/properties_spec.rb index 7a347359595..14fa74e1acf 100644 --- a/backend/spec/features/admin/products/properties_spec.rb +++ b/backend/spec/features/admin/products/properties_spec.rb @@ -88,25 +88,30 @@ 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 + # delete 2nd line (persisted product) + accept_alert do + within_row(2) { click_icon :trash } + end # Give fadeOut time to complete expect(page).to have_css('tr.product_property', count: 1) @@ -115,7 +120,7 @@ expect(page).not_to have_content("Product is not found") - check_property_row_count(2) + check_persisted_property_row_count(1) end def fill_in_property @@ -133,9 +138,9 @@ def delete_product_property 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 From 378e6b82bd0ccee702691953d7ffcea328df4ebc Mon Sep 17 00:00:00 2001 From: Stefan Date: Sat, 6 Jun 2020 19:14:11 +0200 Subject: [PATCH 2/6] always append new nonpersistent item rows at the end --- backend/app/assets/javascripts/spree/backend/admin.js | 2 +- backend/spec/features/admin/products/properties_spec.rb | 9 +++------ 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/backend/app/assets/javascripts/spree/backend/admin.js b/backend/app/assets/javascripts/spree/backend/admin.js index cd18f02cbf4..9da477ea166 100644 --- a/backend/app/assets/javascripts/spree/backend/admin.js +++ b/backend/app/assets/javascripts/spree/backend/admin.js @@ -73,7 +73,7 @@ Spree.ready(function(){ var el = $(this); el.prop('href', '#'); }) - $(target).prepend(new_table_row); + $(target).append(new_table_row); }) $('body').on('click', '.delete-resource', function() { diff --git a/backend/spec/features/admin/products/properties_spec.rb b/backend/spec/features/admin/products/properties_spec.rb index 14fa74e1acf..4a75ec84a58 100644 --- a/backend/spec/features/admin/products/properties_spec.rb +++ b/backend/spec/features/admin/products/properties_spec.rb @@ -108,10 +108,7 @@ end expect(page).to have_css('tr.product_property', count: 2) - # delete 2nd line (persisted product) - accept_alert do - within_row(2) { click_icon :trash } - end + delete_product_property # Give fadeOut time to complete expect(page).to have_css('tr.product_property', count: 1) @@ -120,7 +117,7 @@ expect(page).not_to have_content("Product is not found") - check_persisted_property_row_count(1) + check_persisted_property_row_count(0) end def fill_in_property @@ -133,7 +130,7 @@ 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 From 41d5f8f532b39f5106810f1fa58fe814695ab1cb Mon Sep 17 00:00:00 2001 From: Stefan Date: Sat, 6 Jun 2020 19:21:44 +0200 Subject: [PATCH 3/6] create new js non persistent items without id, no sort handle and proper action to remove resource --- .../assets/javascripts/spree/backend/admin.js | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/backend/app/assets/javascripts/spree/backend/admin.js b/backend/app/assets/javascripts/spree/backend/admin.js index 9da477ea166..a753d024a42 100644 --- a/backend/app/assets/javascripts/spree/backend/admin.js +++ b/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,13 +74,10 @@ 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', '#'); - }) + + // Add a remove button to the actions column of the new row + new_table_row.find("td").last().empty().append(''); + $(target).append(new_table_row); }) From c86f8a8f89c989342b70ce76694a385b02d29fd7 Mon Sep 17 00:00:00 2001 From: Stefan Date: Sat, 6 Jun 2020 19:33:27 +0200 Subject: [PATCH 4/6] initialize items rows always with delete for persistent and remove for non persistent items --- .../admin/option_types/_option_value_fields.html.erb | 9 ++++++++- .../product_properties/_product_property_fields.html.erb | 9 +++++++-- .../spec/features/admin/products/option_types_spec.rb | 4 +++- 3 files changed, 18 insertions(+), 4 deletions(-) diff --git a/backend/app/views/spree/admin/option_types/_option_value_fields.html.erb b/backend/app/views/spree/admin/option_types/_option_value_fields.html.erb index f1d0a6b8917..7fad0c35ee3 100644 --- a/backend/app/views/spree/admin/option_types/_option_value_fields.html.erb +++ b/backend/app/views/spree/admin/option_types/_option_value_fields.html.erb @@ -7,5 +7,12 @@ <%= f.text_field :name %> <%= f.text_field :presentation %> - <%= link_to_remove_fields t('spree.actions.remove'), f, no_text: true %> + + <% 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 %> + diff --git a/backend/app/views/spree/admin/product_properties/_product_property_fields.html.erb b/backend/app/views/spree/admin/product_properties/_product_property_fields.html.erb index 9508f00b7db..f4aadc0a02d 100644 --- a/backend/app/views/spree/admin/product_properties/_product_property_fields.html.erb +++ b/backend/app/views/spree/admin/product_properties/_product_property_fields.html.erb @@ -12,8 +12,13 @@ <%= f.text_field :value %> - <% 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 %> diff --git a/backend/spec/features/admin/products/option_types_spec.rb b/backend/spec/features/admin/products/option_types_spec.rb index c33dea3ef11..1d5c512f5f8 100644 --- a/backend/spec/features/admin/products/option_types_spec.rb +++ b/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") From ac7cd7b32a856302333952fb25d114343c9f53a7 Mon Sep 17 00:00:00 2001 From: Stefan Date: Sat, 6 Jun 2020 19:36:23 +0200 Subject: [PATCH 5/6] deprecate link_to_remove_fields --- backend/app/helpers/spree/admin/base_helper.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/backend/app/helpers/spree/admin/base_helper.rb b/backend/app/helpers/spree/admin/base_helper.rb index 7df39fe38ff..09648112b6e 100644 --- a/backend/app/helpers/spree/admin/base_helper.rb +++ b/backend/app/helpers/spree/admin/base_helper.rb @@ -132,7 +132,9 @@ 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') end From 6e3360fca715fe69c54b96baabeced4a102b4b95 Mon Sep 17 00:00:00 2001 From: Stefan Date: Sat, 6 Jun 2020 20:33:45 +0200 Subject: [PATCH 6/6] remove unnecessary ajax call when removing nonpersistent items --- .../assets/javascripts/spree/backend/admin.js | 24 ++++--------------- .../app/helpers/spree/admin/base_helper.rb | 2 +- 2 files changed, 5 insertions(+), 21 deletions(-) diff --git a/backend/app/assets/javascripts/spree/backend/admin.js b/backend/app/assets/javascripts/spree/backend/admin.js index a753d024a42..96c6ba53714 100644 --- a/backend/app/assets/javascripts/spree/backend/admin.js +++ b/backend/app/assets/javascripts/spree/backend/admin.js @@ -106,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; }); diff --git a/backend/app/helpers/spree/admin/base_helper.rb b/backend/app/helpers/spree/admin/base_helper.rb index 09648112b6e..bb928285384 100644 --- a/backend/app/helpers/spree/admin/base_helper.rb +++ b/backend/app/helpers/spree/admin/base_helper.rb @@ -134,7 +134,7 @@ def link_to_remove_fields(name, form, options = {}) 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') end