From 32fc1eae61473e14ffa12b8d2145459d02d72d11 Mon Sep 17 00:00:00 2001 From: Jean-Baptiste Bellet Date: Mon, 13 Feb 2023 15:58:53 +0100 Subject: [PATCH 1/7] Remove `ship_address` errors if shipping method isn't selected --- app/controllers/split_checkout_controller.rb | 17 ++++++++++++++++- spec/system/consumer/split_checkout_spec.rb | 17 ++++++++++++++--- 2 files changed, 30 insertions(+), 4 deletions(-) diff --git a/app/controllers/split_checkout_controller.rb b/app/controllers/split_checkout_controller.rb index 6ded33fb28..fbb7dccec8 100644 --- a/app/controllers/split_checkout_controller.rb +++ b/app/controllers/split_checkout_controller.rb @@ -49,7 +49,7 @@ class SplitCheckoutController < ::BaseController def render_error flash.now[:error] ||= I18n.t( 'split_checkout.errors.saving_failed', - messages: @order.errors.full_messages.to_sentence + messages: order_error_messages ) render status: :unprocessable_entity, operations: cable_car. @@ -57,6 +57,21 @@ class SplitCheckoutController < ::BaseController replace("#flashes", partial("shared/flashes", locals: { flashes: flash })) end + def order_error_messages + # Remove ship_address.* errors if no shipping method is not selected + remove_ship_address_errors if @order.errors[:shipping_method].present? + + end + + def remove_ship_address_errors + @order.errors.delete("ship_address.firstname") + @order.errors.delete("ship_address.address1") + @order.errors.delete("ship_address.city") + @order.errors.delete("ship_address.phone") + @order.errors.delete("ship_address.lastname") + @order.errors.delete("ship_address.zipcode") + end + def flash_error_when_no_shipping_method_available flash[:error] = I18n.t('split_checkout.errors.no_shipping_methods_available') end diff --git a/spec/system/consumer/split_checkout_spec.rb b/spec/system/consumer/split_checkout_spec.rb index 80eec57fcb..c9cb17c323 100644 --- a/spec/system/consumer/split_checkout_spec.rb +++ b/spec/system/consumer/split_checkout_spec.rb @@ -187,6 +187,19 @@ describe "As a consumer, I want to checkout my order" do click_on "Checkout as guest" end + context "should show proper list of errors" do + before do + click_button "Next - Payment method" + expect(page).to have_content "Saving failed, please update the highlighted fields." + end + + it "should not display any shipping errors messages when shipping method is not selected" do + expect(page).not_to have_content "Shipping address line 1 can't be blank" + expect(page).not_to have_content "Shipping address suburb 1 can't be blank" + expect(page).not_to have_content "Shipping address postcode can't be blank" + end + end + it "should allow visit '/checkout/details'" do expect(page).to have_current_path("/checkout/details") end @@ -306,8 +319,6 @@ describe "As a consumer, I want to checkout my order" do click_button "Next - Payment method" expect(page).to have_content "Saving failed, please update the highlighted fields." - expect(page).to have_content "Shipping address line 1 can't be blank" - expect(page).to have_content "Shipping address same as billing address?" expect(page).to have_content "Save as default shipping address" expect(page).to have_checked_field "Shipping address same as billing address?" end @@ -563,7 +574,7 @@ describe "As a consumer, I want to checkout my order" do end within ".flash[type='error']" do expect(page).to have_content("Saving failed, please update the highlighted fields") - expect(page).to have_content("can't be blank", count: 13) + expect(page).to have_content("can't be blank", count: 7) expect(page).to have_content("is invalid", count: 1) end end From 9a03023b6b609baf4033751e988ae7179213d94a Mon Sep 17 00:00:00 2001 From: Jean-Baptiste Bellet Date: Mon, 13 Feb 2023 16:01:21 +0100 Subject: [PATCH 2/7] Replace bill_address phone by Customer phone to improve readability TBS; --- app/controllers/split_checkout_controller.rb | 1 - config/locales/en.yml | 2 ++ spec/system/consumer/split_checkout_spec.rb | 5 +++++ 3 files changed, 7 insertions(+), 1 deletion(-) diff --git a/app/controllers/split_checkout_controller.rb b/app/controllers/split_checkout_controller.rb index fbb7dccec8..a0aed03943 100644 --- a/app/controllers/split_checkout_controller.rb +++ b/app/controllers/split_checkout_controller.rb @@ -60,7 +60,6 @@ class SplitCheckoutController < ::BaseController def order_error_messages # Remove ship_address.* errors if no shipping method is not selected remove_ship_address_errors if @order.errors[:shipping_method].present? - end def remove_ship_address_errors diff --git a/config/locales/en.yml b/config/locales/en.yml index 019042608e..33ae55daef 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -35,6 +35,8 @@ en: firstname: "First name" lastname: "Last name" zipcode: "Shipping address postcode" + spree/order/bill_address: + phone: Customer phone spree/user: password: "Password" password_confirmation: "Password confirmation" diff --git a/spec/system/consumer/split_checkout_spec.rb b/spec/system/consumer/split_checkout_spec.rb index c9cb17c323..0da3fe9135 100644 --- a/spec/system/consumer/split_checkout_spec.rb +++ b/spec/system/consumer/split_checkout_spec.rb @@ -198,6 +198,11 @@ describe "As a consumer, I want to checkout my order" do expect(page).not_to have_content "Shipping address suburb 1 can't be blank" expect(page).not_to have_content "Shipping address postcode can't be blank" end + + it "should not display bill address phone number error message" do + expect(page).not_to have_content "Bill address phone can't be blank" + expect(page).to have_content "Customer phone can't be blank" + end end it "should allow visit '/checkout/details'" do From a39598d0495a215d6485ed8ca2dd600dc329a65a Mon Sep 17 00:00:00 2001 From: Jean-Baptiste Bellet Date: Mon, 13 Feb 2023 16:03:52 +0100 Subject: [PATCH 3/7] Reorder error messages to improve readability tbs; --- app/controllers/split_checkout_controller.rb | 25 ++++++++++++++++++++ spec/system/consumer/split_checkout_spec.rb | 11 +++++++++ 2 files changed, 36 insertions(+) diff --git a/app/controllers/split_checkout_controller.rb b/app/controllers/split_checkout_controller.rb index a0aed03943..997ef5e5bf 100644 --- a/app/controllers/split_checkout_controller.rb +++ b/app/controllers/split_checkout_controller.rb @@ -60,6 +60,9 @@ class SplitCheckoutController < ::BaseController def order_error_messages # Remove ship_address.* errors if no shipping method is not selected remove_ship_address_errors if @order.errors[:shipping_method].present? + # Reorder errors to make sure the most important ones are shown first + # and finally, return the error messages to sentence + reorder_errors.map(&:full_message).to_sentence end def remove_ship_address_errors @@ -71,6 +74,28 @@ class SplitCheckoutController < ::BaseController @order.errors.delete("ship_address.zipcode") end + def reorder_errors + @order.errors.sort_by do |e| + case e.attribute + when /email/i then 0 + when /phone/i then 1 + when /bill_address/i then 2 + bill_address_error_order(e) + else 20 + end + end + end + + def bill_address_error_order(error) + case error.attribute + when /firstname/i then 0 + when /lastname/i then 1 + when /address1/i then 2 + when /city/i then 3 + when /zipcode/i then 4 + else 5 + end + end + def flash_error_when_no_shipping_method_available flash[:error] = I18n.t('split_checkout.errors.no_shipping_methods_available') end diff --git a/spec/system/consumer/split_checkout_spec.rb b/spec/system/consumer/split_checkout_spec.rb index 0da3fe9135..299ffaceef 100644 --- a/spec/system/consumer/split_checkout_spec.rb +++ b/spec/system/consumer/split_checkout_spec.rb @@ -203,6 +203,17 @@ describe "As a consumer, I want to checkout my order" do expect(page).not_to have_content "Bill address phone can't be blank" expect(page).to have_content "Customer phone can't be blank" end + + context "with no email filled in" do + before do + fill_in "Email", with: "" + click_button "Next - Payment method" + end + + it "should display error message in the right order" do + expect(page).to have_content "Customer E-Mail can't be blank, Customer E-Mail is invalid, Customer phone can't be blank, Bill address firstname can't be blank, Bill address lastname can't be blank, Bill address address1 can't be blank, Bill address city can't be blank, Bill address zipcode can't be blank, and Shipping method Select a shipping method" + end + end end it "should allow visit '/checkout/details'" do From 88607a3dceb06cd900cf3484f506a8b83e1ba276 Mon Sep 17 00:00:00 2001 From: Jean-Baptiste Bellet Date: Tue, 14 Feb 2023 09:44:50 +0100 Subject: [PATCH 4/7] Improve bill address error message "(Street + House number)" instead of "address1" and "postcode" instead of "zipcode" --- config/locales/en.yml | 2 ++ spec/models/spree/order_spec.rb | 6 ++++++ spec/system/consumer/split_checkout_spec.rb | 2 +- 3 files changed, 9 insertions(+), 1 deletion(-) diff --git a/config/locales/en.yml b/config/locales/en.yml index 33ae55daef..84158b1d46 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -36,6 +36,8 @@ en: lastname: "Last name" zipcode: "Shipping address postcode" spree/order/bill_address: + address1: "Bill address (Street + House number)" + zipcode: "Bill address postcode" phone: Customer phone spree/user: password: "Password" diff --git a/spec/models/spree/order_spec.rb b/spec/models/spree/order_spec.rb index 8c1adba511..e42a7616c6 100644 --- a/spec/models/spree/order_spec.rb +++ b/spec/models/spree/order_spec.rb @@ -14,6 +14,12 @@ describe Spree::Order do order.save expect(order.errors.full_messages).to include "Shipping address line 1 can't be blank" end + + it "provides friendly error messages for bill address" do + order.bill_address = Spree::Address.new + order.save + expect(order.errors.full_messages).to include "Bill address (Street + House number) can't be blank" + end end context "#products" do diff --git a/spec/system/consumer/split_checkout_spec.rb b/spec/system/consumer/split_checkout_spec.rb index 299ffaceef..f595e64159 100644 --- a/spec/system/consumer/split_checkout_spec.rb +++ b/spec/system/consumer/split_checkout_spec.rb @@ -211,7 +211,7 @@ describe "As a consumer, I want to checkout my order" do end it "should display error message in the right order" do - expect(page).to have_content "Customer E-Mail can't be blank, Customer E-Mail is invalid, Customer phone can't be blank, Bill address firstname can't be blank, Bill address lastname can't be blank, Bill address address1 can't be blank, Bill address city can't be blank, Bill address zipcode can't be blank, and Shipping method Select a shipping method" + expect(page).to have_content "Customer E-Mail can't be blank, Customer E-Mail is invalid, Customer phone can't be blank, Bill address firstname can't be blank, Bill address lastname can't be blank, Bill address (Street + House number) can't be blank, Bill address city can't be blank, Bill address postcode can't be blank, and Shipping method Select a shipping method" end end end From 4cea7b195771b80d6cbd775589f8efff828aa2e1 Mon Sep 17 00:00:00 2001 From: Jean-Baptiste Bellet Date: Tue, 14 Feb 2023 09:45:15 +0100 Subject: [PATCH 5/7] Improve error message, and use the same than the view --- config/locales/en.yml | 4 ++-- spec/models/spree/order_spec.rb | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/config/locales/en.yml b/config/locales/en.yml index 84158b1d46..c33177750a 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -27,9 +27,9 @@ en: spree/shipping_method: Shipping Method attributes: spree/order/ship_address: - address1: "Shipping address line 1" + address1: "Shipping address (Street + House number)" address2: "Shipping address line 2" - city: "Shipping address suburb 1" + city: "Shipping address city" country: "Shipping address country" phone: "Phone number" firstname: "First name" diff --git a/spec/models/spree/order_spec.rb b/spec/models/spree/order_spec.rb index e42a7616c6..cce3536522 100644 --- a/spec/models/spree/order_spec.rb +++ b/spec/models/spree/order_spec.rb @@ -12,7 +12,7 @@ describe Spree::Order do it "provides friendly error messages" do order.ship_address = Spree::Address.new order.save - expect(order.errors.full_messages).to include "Shipping address line 1 can't be blank" + expect(order.errors.full_messages).to include "Shipping address (Street + House number) can't be blank" end it "provides friendly error messages for bill address" do From 08fb496f55873acaf52b6f854570171c68ca49d8 Mon Sep 17 00:00:00 2001 From: Jean-Baptiste Bellet Date: Tue, 14 Feb 2023 09:46:29 +0100 Subject: [PATCH 6/7] Don't need to show any ship_address errors if no ship selected nor if the ship_address is the same than the billing one --- app/controllers/split_checkout_controller.rb | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/app/controllers/split_checkout_controller.rb b/app/controllers/split_checkout_controller.rb index 997ef5e5bf..752cec3010 100644 --- a/app/controllers/split_checkout_controller.rb +++ b/app/controllers/split_checkout_controller.rb @@ -59,12 +59,17 @@ class SplitCheckoutController < ::BaseController def order_error_messages # Remove ship_address.* errors if no shipping method is not selected - remove_ship_address_errors if @order.errors[:shipping_method].present? + remove_ship_address_errors if no_ship_address_needed? + # Reorder errors to make sure the most important ones are shown first # and finally, return the error messages to sentence reorder_errors.map(&:full_message).to_sentence end + def no_ship_address_needed? + @order.errors[:shipping_method].present? || params[:ship_address_same_as_billing] == "1" + end + def remove_ship_address_errors @order.errors.delete("ship_address.firstname") @order.errors.delete("ship_address.address1") From 0a3d362282262864b0787be95eebebf8cdd172df Mon Sep 17 00:00:00 2001 From: Jean-Baptiste Bellet Date: Wed, 15 Feb 2023 10:00:53 +0100 Subject: [PATCH 7/7] Still improve error message for Billing address ``` "Bill" is a person's name, and I don't know his address ``` ;) --- config/locales/en.yml | 8 ++++++-- spec/models/spree/order_spec.rb | 2 +- spec/system/consumer/split_checkout_spec.rb | 2 +- 3 files changed, 8 insertions(+), 4 deletions(-) diff --git a/config/locales/en.yml b/config/locales/en.yml index c33177750a..49b7506aef 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -36,8 +36,12 @@ en: lastname: "Last name" zipcode: "Shipping address postcode" spree/order/bill_address: - address1: "Bill address (Street + House number)" - zipcode: "Bill address postcode" + address1: "Billing address (Street + House number)" + zipcode: "Billing address postcode" + city: "Billing address city" + country: "Billing address country" + firstname: "Billing address first name" + lastname: "Billing address last name" phone: Customer phone spree/user: password: "Password" diff --git a/spec/models/spree/order_spec.rb b/spec/models/spree/order_spec.rb index cce3536522..3ba51a4e81 100644 --- a/spec/models/spree/order_spec.rb +++ b/spec/models/spree/order_spec.rb @@ -18,7 +18,7 @@ describe Spree::Order do it "provides friendly error messages for bill address" do order.bill_address = Spree::Address.new order.save - expect(order.errors.full_messages).to include "Bill address (Street + House number) can't be blank" + expect(order.errors.full_messages).to include "Billing address (Street + House number) can't be blank" end end diff --git a/spec/system/consumer/split_checkout_spec.rb b/spec/system/consumer/split_checkout_spec.rb index f595e64159..b293aff3df 100644 --- a/spec/system/consumer/split_checkout_spec.rb +++ b/spec/system/consumer/split_checkout_spec.rb @@ -211,7 +211,7 @@ describe "As a consumer, I want to checkout my order" do end it "should display error message in the right order" do - expect(page).to have_content "Customer E-Mail can't be blank, Customer E-Mail is invalid, Customer phone can't be blank, Bill address firstname can't be blank, Bill address lastname can't be blank, Bill address (Street + House number) can't be blank, Bill address city can't be blank, Bill address postcode can't be blank, and Shipping method Select a shipping method" + expect(page).to have_content "Customer E-Mail can't be blank, Customer E-Mail is invalid, Customer phone can't be blank, Billing address first name can't be blank, Billing address last name can't be blank, Billing address (Street + House number) can't be blank, Billing address city can't be blank, Billing address postcode can't be blank, and Shipping method Select a shipping method" end end end