From f05d27b58b4222620904362871eed7d7278c4f8c Mon Sep 17 00:00:00 2001 From: David Cook Date: Wed, 27 Sep 2023 12:15:00 +1000 Subject: [PATCH] Show error message summary at top of form --- app/reflexes/products_reflex.rb | 9 +-- app/views/admin/products_v3/_table.html.haml | 17 ++--- app/webpacker/css/admin_v3/shared/forms.scss | 14 ++++ config/locales/en.yml | 11 ++- spec/reflexes/products_reflex_spec.rb | 13 ++-- .../system/admin/products_v3/products_spec.rb | 71 ++++++++++++------- 6 files changed, 89 insertions(+), 46 deletions(-) diff --git a/app/reflexes/products_reflex.rb b/app/reflexes/products_reflex.rb index 8e18198a40..01736eedab 100644 --- a/app/reflexes/products_reflex.rb +++ b/app/reflexes/products_reflex.rb @@ -41,8 +41,7 @@ class ProductsReflex < ApplicationReflex # flash[:success] = with_locale { I18n.t('.success') } # morph_admin_flashes # ERROR: selector morph type has already been set elsif product_set.errors.present? - # @error_msg = with_locale{ I18n.t('.products_have_error', count: product_set.invalid.count) } - @error_msg = "#{product_set.invalid.count} products have errors." + @error_counts = { saved: product_set.saved_count, invalid: product_set.invalid.count } end render_products_form @@ -87,10 +86,12 @@ class ProductsReflex < ApplicationReflex end def render_products_form + locals = { products: @products } + locals[:error_counts] = @error_counts if @error_counts.present? + cable_ready.replace( selector: "#products-form", - html: render(partial: "admin/products_v3/table", - locals: { products: @products, error_msg: @error_msg }) + html: render(partial: "admin/products_v3/table", locals:) ).broadcast morph :nothing diff --git a/app/views/admin/products_v3/_table.html.haml b/app/views/admin/products_v3/_table.html.haml index 33806fc195..97122ee0e2 100644 --- a/app/views/admin/products_v3/_table.html.haml +++ b/app/views/admin/products_v3/_table.html.haml @@ -1,15 +1,16 @@ = form_with url: bulk_update_admin_products_path, method: :patch, id: "products-form", builder: BulkFormBuilder, html: {'data-reflex-serialize-form': true, 'data-reflex': 'submit->products#bulk_update', - 'data-controller': "bulk-form", 'data-bulk-form-disable-selector-value': "#sort,#filters"} do |form| - %fieldset.form-actions.hidden{ 'data-bulk-form-target': "actions" } + 'data-controller': "bulk-form", 'data-bulk-form-disable-selector-value': "#sort,#filters" } do |form| + %fieldset.form-actions{ class: ("hidden" unless defined?(error_counts)), 'data-bulk-form-target': "actions" } .container - .status.ten.columns - .changed_summary{ 'data-bulk-form-target': "changedSummary", 'data-translation-key': 'admin.products_v3.table.changed_summary'} - - if defined?(error_msg) && error_msg.present? - .error - = error_msg - .form-buttons.six.columns + .status.eleven.columns + .modified_summary{ 'data-bulk-form-target': "changedSummary", 'data-translation-key': 'admin.products_v3.table.changed_summary'} + - if defined?(error_counts) + .error_summary + -# X products were saved correctly, but Y products could not be saved correctly. Please review errors and try again + = t('.error_summary.saved', count: error_counts[:saved]) + t('.error_summary.invalid', count: error_counts[:invalid]) + .form-buttons.five.columns = form.submit t('.reset'), type: :reset, class: "medium", 'data-reflex': 'click->products#fetch' = form.submit t('.save'), class: "medium" %table.products diff --git a/app/webpacker/css/admin_v3/shared/forms.scss b/app/webpacker/css/admin_v3/shared/forms.scss index 96cef47615..35f8697e04 100644 --- a/app/webpacker/css/admin_v3/shared/forms.scss +++ b/app/webpacker/css/admin_v3/shared/forms.scss @@ -282,6 +282,20 @@ fieldset { text-align: center; } +.modified_summary { + font-size: inherit; +} + +.error_summary { + font-size: inherit; + + @extend .icon-remove-sign; + &:before { + font-family: FontAwesome; + color: $color-error; + } +} + select { @extend input, [type="text"]; background-color: white; diff --git a/config/locales/en.yml b/config/locales/en.yml index 68cd6ef091..edbc5b677f 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -837,13 +837,18 @@ en: zero: "" one: "%{count} product modified." other: "%{count} products modified." + error_summary: + saved: + zero: "" + one: "%{count} product was saved correctly, but " + other: "%{count} products were saved correctly, but " + invalid: + one: "%{count} product could not be saved. Please review the errors and try again." + other: "%{count} products could not be saved. Please review the errors and try again." save: Save changes reset: Discard changes bulk_update: # TODO: fix these success: "Products successfully updated" #TODO: add count - products_have_error: - one: "%{count} product has an error." - other: "%{count} products have an error." product_import: title: Product Import file_not_found: File not found or could not be opened diff --git a/spec/reflexes/products_reflex_spec.rb b/spec/reflexes/products_reflex_spec.rb index ae95f16b8c..0672c46863 100644 --- a/spec/reflexes/products_reflex_spec.rb +++ b/spec/reflexes/products_reflex_spec.rb @@ -13,7 +13,7 @@ describe ProductsReflex, type: :reflex do Flipper.enable(:admin_style_v3) end - describe 'fetch' do + describe '#fetch' do subject{ build_reflex(method_name: :fetch, **context) } describe "sorting" do @@ -41,6 +41,7 @@ describe ProductsReflex, type: :reflex do sku: "APL-01", price: 5.25) } + let!(:product_c) { create(:simple_product, name: "Carrots", sku: "CAR-00") } let!(:product_b) { create(:simple_product, name: "Bananas", sku: "BAN-00") } let!(:product_a) { create(:simple_product, name: "Apples", sku: "APL-00") } @@ -123,17 +124,21 @@ describe ProductsReflex, type: :reflex do "products" => [ { "id" => product_a.id.to_s, - "name" => "", + "name" => "Pommes", }, { "id" => product_b.id.to_s, - "name" => "", + "name" => "", # Name can't be blank + }, + { + "id" => product_c.id.to_s, + "name" => "", # Name can't be blank }, ] } reflex = run_reflex(:bulk_update, params:) - expect(reflex.get(:error_msg)).to include "2 products have errors" + expect(reflex.get(:error_counts)).to eq({ saved: 1, invalid: 2 }) # # WTF # expect{ reflex(:bulk_update, params:) }.to broadcast( diff --git a/spec/system/admin/products_v3/products_spec.rb b/spec/system/admin/products_v3/products_spec.rb index 8ea6f57c48..5b403b0e18 100644 --- a/spec/system/admin/products_v3/products_spec.rb +++ b/spec/system/admin/products_v3/products_spec.rb @@ -270,37 +270,54 @@ describe 'As an admin, I can see the new product page' do end end - it "shows errors for both product and variant fields" do - within row_containing_name("Apples") do - fill_in "Name", with: "" - fill_in "SKU", with: "A" * 256 - end - within row_containing_name("Medium box") do - fill_in "Name", with: "L" * 256 - fill_in "SKU", with: "1" * 256 + context "with invalid data" do + let!(:product_b) { create(:simple_product, name: "Bananas") } + + before do + within row_containing_name("Apples") do + fill_in "Name", with: "" + fill_in "SKU", with: "A" * 256 + end + within row_containing_name("Medium box") do + fill_in "Name", with: "L" * 256 + fill_in "SKU", with: "1" * 256 + fill_in "Price", with: "10.25" + end end - expect { - click_button "Save changes" - product_a.reload - }.to_not change { product_a.name } + it "shows errors for both product and variant fields" do + # Also update another product with valid data + within row_containing_name("Bananas") do + fill_in "Name", with: "Bananes" + end - # (there's no identifier displayed, so the user must remember which product it is..) - within row_containing_name("") do - expect(page).to have_field "Name", with: "" - expect(page).to have_content "can't be blank" - expect(page).to have_field "SKU", with: "A" * 256 - expect(page).to have_content "is too long" - end - pending - within row_containing_name("L" * 256) do - expect(page).to have_field "Name", with: "L" * 256 - expect(page).to have_field "SKU", with: "1" * 256 - expect(page).to have_content "is too long" - expect(page).to have_field "Price", with: "10.25" - end + expect { + click_button "Save changes" + product_a.reload + }.to_not change { product_a.name } - expect(page).to have_content "Please review the errors and try again" + # pending("unchanged rows are being saved") # TODO: don't report unchanged rows + # expect(page).to_not have_content("rows were saved correctly") + # Both the product and variant couldn't be saved. + expect(page).to have_content("1 product could not be saved") + expect(page).to have_content "Please review the errors and try again" + + # (there's no identifier displayed, so the user must remember which product it is..) + within row_containing_name("") do + expect(page).to have_field "Name", with: "" + expect(page).to have_content "can't be blank" + expect(page).to have_field "SKU", with: "A" * 256 + expect(page).to have_content "is too long" + end + + pending "bug #11748" + within row_containing_name("L" * 256) do + expect(page).to have_field "Name", with: "L" * 256 + expect(page).to have_field "SKU", with: "1" * 256 + expect(page).to have_content "is too long" + expect(page).to have_field "Price", with: "10.25" # other updated value is retained + end + end end end