From 523faa670d4460f3ef18409fc2c08a4d979b5bb3 Mon Sep 17 00:00:00 2001 From: Luis Ramos Date: Wed, 25 Mar 2020 17:31:56 +0000 Subject: [PATCH 1/6] Remove FoundationRailsHelper, this is dead code now --- Gemfile | 1 - Gemfile.lock | 11 ----------- app/helpers/application_helper.rb | 2 -- 3 files changed, 14 deletions(-) diff --git a/Gemfile b/Gemfile index 7fbb46c5ee..72dc87335c 100644 --- a/Gemfile +++ b/Gemfile @@ -112,7 +112,6 @@ gem 'momentjs-rails' gem 'turbo-sprockets-rails3' gem "foundation-rails" -gem 'foundation_rails_helper', github: 'willrjmarshall/foundation_rails_helper', branch: "rails3" gem 'jquery-migrate-rails' gem 'jquery-rails', '3.1.5' diff --git a/Gemfile.lock b/Gemfile.lock index 84c427fb72..58fc7269d2 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -65,16 +65,6 @@ GIT rails-i18n spree_core (>= 1.1) -GIT - remote: https://github.com/willrjmarshall/foundation_rails_helper.git - revision: 4d5d53fdc4b1fb71e66524d298c5c635de82cfbb - branch: rails3 - specs: - foundation_rails_helper (0.4) - actionpack (>= 3.0) - activemodel (>= 3.0) - railties (>= 3.0) - PATH remote: engines/catalog specs: @@ -740,7 +730,6 @@ DEPENDENCIES foreigner foundation-icons-sass-rails foundation-rails - foundation_rails_helper! fuubar (~> 2.5.0) geocoder gmaps4rails diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb index 0142825667..2e03e68bc0 100644 --- a/app/helpers/application_helper.rb +++ b/app/helpers/application_helper.rb @@ -1,6 +1,4 @@ module ApplicationHelper - include FoundationRailsHelper::FlashHelper - def feature?(feature) OpenFoodNetwork::FeatureToggle.enabled? feature end From 6a1c541479b024bb120c5f9475d71afdaddf1814 Mon Sep 17 00:00:00 2001 From: Luis Ramos Date: Mon, 30 Mar 2020 19:59:40 +0100 Subject: [PATCH 2/6] Remove specific error color on checkout page so that the error message takes the default foundation error color which is white --- app/assets/stylesheets/darkswarm/checkout.css.scss | 5 ----- 1 file changed, 5 deletions(-) diff --git a/app/assets/stylesheets/darkswarm/checkout.css.scss b/app/assets/stylesheets/darkswarm/checkout.css.scss index 458fd24d5e..56c4f17170 100644 --- a/app/assets/stylesheets/darkswarm/checkout.css.scss +++ b/app/assets/stylesheets/darkswarm/checkout.css.scss @@ -109,9 +109,4 @@ checkout { } } } - - .error { - color: #c82020; - } } - From b898ce1ae1583ef0dacb607b8166934b2002bd8d Mon Sep 17 00:00:00 2001 From: Luis Ramos Date: Wed, 25 Mar 2020 17:33:29 +0000 Subject: [PATCH 3/6] Make checkout controller add flash error if order contains any type of error Here we add translations for a particular case where the credit card expiry date is in the past --- app/controllers/checkout_controller.rb | 7 +++---- config/locales/en.yml | 6 ++++++ spec/controllers/checkout_controller_spec.rb | 6 +++--- 3 files changed, 12 insertions(+), 7 deletions(-) diff --git a/app/controllers/checkout_controller.rb b/app/controllers/checkout_controller.rb index 65d308bc0d..987f8d50d7 100644 --- a/app/controllers/checkout_controller.rb +++ b/app/controllers/checkout_controller.rb @@ -165,7 +165,7 @@ class CheckoutController < Spree::StoreController checkout_succeeded redirect_to(order_path(@order)) && return else - flash[:error] = order_workflow_error + flash[:error] = order_error checkout_failed end end @@ -179,8 +179,6 @@ class CheckoutController < Spree::StoreController @order.select_shipping_method(shipping_method_id) if @order.state == "delivery" next if advance_order_state(@order) - - flash[:error] = order_workflow_error return update_failed end @@ -205,7 +203,7 @@ class CheckoutController < Spree::StoreController false end - def order_workflow_error + def order_error if @order.errors.present? @order.errors.full_messages.to_sentence else @@ -245,6 +243,7 @@ class CheckoutController < Spree::StoreController end def update_failed + flash[:error] = order_error if flash.empty? checkout_failed update_failed_response end diff --git a/config/locales/en.yml b/config/locales/en.yml index 86c8f1611e..d8cf8784cd 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -40,6 +40,8 @@ en: shipping_category_id: "Shipping Category" variant_unit: "Variant Unit" variant_unit_name: "Variant Unit Name" + spree/credit_card: + base: "Credit Card" order_cycle: orders_close_at: Close date errors: @@ -50,6 +52,10 @@ en: taken: "There's already an account for this email. Please login or reset your password." spree/order: no_card: There are no authorised credit cards available to charge + spree/credit_card: + attributes: + base: + card_expired: "has expired" order_cycle: attributes: orders_close_at: diff --git a/spec/controllers/checkout_controller_spec.rb b/spec/controllers/checkout_controller_spec.rb index b11ca1ab78..ccbf458674 100644 --- a/spec/controllers/checkout_controller_spec.rb +++ b/spec/controllers/checkout_controller_spec.rb @@ -197,13 +197,13 @@ describe CheckoutController, type: :controller do allow(controller).to receive(:current_order).and_return(order) end - it "returns errors" do + it "returns errors and flash if order.update_attributes fails" do spree_post :update, format: :json, order: {} expect(response.status).to eq(400) - expect(response.body).to eq({ errors: assigns[:order].errors, flash: {} }.to_json) + expect(response.body).to eq({ errors: assigns[:order].errors, flash: { error: order.errors.full_messages.to_sentence } }.to_json) end - it "returns flash" do + it "returns errors and flash if order.next fails" do allow(order).to receive(:update_attributes).and_return true allow(order).to receive(:next).and_return false spree_post :update, format: :json, order: {} From ce2a164c666961ca69340265595afae5bf0f275f Mon Sep 17 00:00:00 2001 From: Luis Ramos Date: Tue, 31 Mar 2020 13:37:03 +0100 Subject: [PATCH 4/6] Stop using f_form_for Add labels for some fields, this was done automatically by rails foundation helper --- app/views/checkout/_form.html.haml | 2 +- app/views/checkout/_shipping.html.haml | 3 ++- app/views/user_passwords/edit.html.haml | 4 +++- 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/app/views/checkout/_form.html.haml b/app/views/checkout/_form.html.haml index 901fe64a00..9a705ef4b5 100644 --- a/app/views/checkout/_form.html.haml +++ b/app/views/checkout/_form.html.haml @@ -3,7 +3,7 @@ = inject_available_payment_methods = inject_saved_credit_cards -= f_form_for current_order, += form_for current_order, html: {name: "checkout", id: "checkout_form", novalidate: true, diff --git a/app/views/checkout/_shipping.html.haml b/app/views/checkout/_shipping.html.haml index c137f2cdee..1e50dc33f8 100644 --- a/app/views/checkout/_shipping.html.haml +++ b/app/views/checkout/_shipping.html.haml @@ -52,7 +52,8 @@ .row .small-12.columns - = f.text_area :special_instructions, label: t(:checkout_instructions), size: "60x4", "ng-model" => "order.special_instructions" + %label{ for: 'order_special_instructions'}= t(:checkout_instructions) + = f.text_area :special_instructions, size: "60x4", "ng-model" => "order.special_instructions" .row .small-12.columns.text-right diff --git a/app/views/user_passwords/edit.html.haml b/app/views/user_passwords/edit.html.haml index 3042161881..eb1d9df0bc 100644 --- a/app/views/user_passwords/edit.html.haml +++ b/app/views/user_passwords/edit.html.haml @@ -1,4 +1,4 @@ -= f_form_for @spree_user, :as => :spree_user, :url => spree.spree_user_password_path, :method => :put do |f| += form_for @spree_user, :as => :spree_user, :url => spree.spree_user_password_path, :method => :put do |f| = render :partial => 'spree/shared/error_messages', :locals => { :target => @spree_user } %fieldset .row @@ -6,9 +6,11 @@ %legend= t(:change_my_password) .row .small-12.medium-6.large-4.columns.medium-centered.large-centered + %label{ for: 'spree_user_password'}= t(:password) = f.password_field :password .row .small-12.medium-6.large-4.columns.medium-centered.large-centered + %label{ for: 'spree_user_password_confirmation'}= t(:password_confirmation) = f.password_field :password_confirmation = f.hidden_field :reset_password_token .row From c3d25bf163a22d9c631e8ddc6c51387d4c3bda45 Mon Sep 17 00:00:00 2001 From: Luis Ramos Date: Tue, 31 Mar 2020 17:02:01 +0100 Subject: [PATCH 5/6] Make checkout controller send bugsnag alerts on every checkout problem There are two new situations here: we will see order.errors after update_attributes fails but before order restart; and we will see how often the order is not complete when the workflow finishes (maybe none) --- app/controllers/checkout_controller.rb | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/app/controllers/checkout_controller.rb b/app/controllers/checkout_controller.rb index 987f8d50d7..26c566e9a6 100644 --- a/app/controllers/checkout_controller.rb +++ b/app/controllers/checkout_controller.rb @@ -53,9 +53,8 @@ class CheckoutController < Spree::StoreController rescue Spree::Core::GatewayError => e rescue_from_spree_gateway_error(e) rescue StandardError => e - Bugsnag.notify(e) flash[:error] = I18n.t("checkout.failed") - update_failed + update_failed(e) end # Clears the cached order. Required for #current_order to return a new order @@ -179,6 +178,7 @@ class CheckoutController < Spree::StoreController @order.select_shipping_method(shipping_method_id) if @order.state == "delivery" next if advance_order_state(@order) + return update_failed end @@ -216,7 +216,7 @@ class CheckoutController < Spree::StoreController checkout_succeeded update_succeeded_response else - update_failed + update_failed(RuntimeError.new("Order not complete after the checkout workflow")) end end @@ -242,7 +242,9 @@ class CheckoutController < Spree::StoreController end end - def update_failed + def update_failed(error = RuntimeError.new(order_error)) + Bugsnag.notify(error) + flash[:error] = order_error if flash.empty? checkout_failed update_failed_response From 957b398a549a569d9946ba6fff720dfc057d7664 Mon Sep 17 00:00:00 2001 From: Luis Ramos Date: Mon, 30 Mar 2020 19:31:47 +0100 Subject: [PATCH 6/6] Add call to $evalAsync() after Loading and FlashLoader are updated so that a angular digest is triggered This is required so that Loading.clear triggers a refresh and makes its placeholder to be cleared --- .../darkswarm/services/stripe_elements.js.coffee | 6 ++++++ .../darkswarm/services/stripe_elements_spec.js.coffee | 9 ++++----- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/app/assets/javascripts/darkswarm/services/stripe_elements.js.coffee b/app/assets/javascripts/darkswarm/services/stripe_elements.js.coffee index acd220f092..6ec274cfec 100644 --- a/app/assets/javascripts/darkswarm/services/stripe_elements.js.coffee +++ b/app/assets/javascripts/darkswarm/services/stripe_elements.js.coffee @@ -15,6 +15,7 @@ Darkswarm.factory 'StripeElements', ($rootScope, Loading, RailsFlashLoader) -> if(response.error) Loading.clear() RailsFlashLoader.loadFlash({error: t("error") + ": #{response.error.message}"}) + @triggerAngularDigest() else secrets.token = response.token.id secrets.cc_type = @mapCC(response.token.card.brand) @@ -32,12 +33,17 @@ Darkswarm.factory 'StripeElements', ($rootScope, Loading, RailsFlashLoader) -> if(response.error) Loading.clear() RailsFlashLoader.loadFlash({error: t("error") + ": #{response.error.message}"}) + @triggerAngularDigest() else secrets.token = response.paymentMethod.id secrets.cc_type = response.paymentMethod.card.brand secrets.card = response.paymentMethod.card submit() + triggerAngularDigest: -> + # $evalAsync is improved way of triggering a digest without calling $apply + $rootScope.$evalAsync() + # Maps the brand returned by Stripe to that required by activemerchant mapCC: (ccType) -> switch ccType diff --git a/spec/javascripts/unit/darkswarm/services/stripe_elements_spec.js.coffee b/spec/javascripts/unit/darkswarm/services/stripe_elements_spec.js.coffee index 0252298f82..7fd1d471dd 100644 --- a/spec/javascripts/unit/darkswarm/services/stripe_elements_spec.js.coffee +++ b/spec/javascripts/unit/darkswarm/services/stripe_elements_spec.js.coffee @@ -46,11 +46,10 @@ describe 'StripeElements Service', -> it "doesn't submit the form, shows an error message instead", inject (Loading, RailsFlashLoader) -> spyOn(Loading, "clear") spyOn(RailsFlashLoader, "loadFlash") - StripeElements.requestToken(secrets, submit) - $rootScope.$digest() # required for #then to by called - expect(submit).not.toHaveBeenCalled() - expect(Loading.clear).toHaveBeenCalled() - expect(RailsFlashLoader.loadFlash).toHaveBeenCalledWith({error: "Error: There was a problem"}) + StripeElements.requestToken(secrets, submit).then (data) -> + expect(submit).not.toHaveBeenCalled() + expect(Loading.clear).toHaveBeenCalled() + expect(RailsFlashLoader.loadFlash).toHaveBeenCalledWith({error: "Error: There was a problem"}) describe 'mapCC', -> it "maps the brand returned by Stripe to that required by activemerchant", ->