From 7414047b924eb0cf241b7ceb108f88ddd8468a0c Mon Sep 17 00:00:00 2001 From: Luis Ramos Date: Fri, 10 Apr 2020 12:21:42 +0100 Subject: [PATCH 1/5] Switch from old success/error to modern then/catch structure Catch() will get a few more errors then errors() Also, add try/catch inside catch to detect any errors parsing the response error payload --- .../darkswarm/services/checkout.js.coffee | 22 +++++++++++-------- .../services/checkout_spec.js.coffee | 1 + 2 files changed, 14 insertions(+), 9 deletions(-) diff --git a/app/assets/javascripts/darkswarm/services/checkout.js.coffee b/app/assets/javascripts/darkswarm/services/checkout.js.coffee index 83cf018365..890d45c2e5 100644 --- a/app/assets/javascripts/darkswarm/services/checkout.js.coffee +++ b/app/assets/javascripts/darkswarm/services/checkout.js.coffee @@ -14,15 +14,19 @@ Darkswarm.factory 'Checkout', ($injector, CurrentOrder, ShippingMethods, StripeE submit: => Loading.message = t 'submitting_order' - $http.put('/checkout.json', {order: @preprocess()}).success (data, status)=> - Navigation.go data.path - .error (response, status)=> - if response.path - Navigation.go response.path - else - Loading.clear() - @errors = response.errors - RailsFlashLoader.loadFlash(response.flash) + $http.put('/checkout.json', {order: @preprocess()}) + .then (response) => + Navigation.go response.data.path + .catch (response) => + try + if response.data.path + Navigation.go response.data.path + else + Loading.clear() + @errors = response.data.errors + RailsFlashLoader.loadFlash(response.data.flash) + catch error + RailsFlashLoader.loadFlash("Unkown error occurred") # Rails wants our Spree::Address data to be provided with _attributes preprocess: -> diff --git a/spec/javascripts/unit/darkswarm/services/checkout_spec.js.coffee b/spec/javascripts/unit/darkswarm/services/checkout_spec.js.coffee index 4ff2f0d97a..6b1b7d6e40 100644 --- a/spec/javascripts/unit/darkswarm/services/checkout_spec.js.coffee +++ b/spec/javascripts/unit/darkswarm/services/checkout_spec.js.coffee @@ -119,6 +119,7 @@ describe 'Checkout service', -> it "puts errors into the scope", -> $httpBackend.expectPUT("/checkout.json").respond 400, {errors: {error: "frogs"}} Checkout.submit() + $httpBackend.flush() expect(Checkout.errors).toEqual {error: "frogs"} From cdf5bcb7eb129820f44a48696810eece78c0ba41 Mon Sep 17 00:00:00 2001 From: Luis Ramos Date: Tue, 14 Apr 2020 13:44:58 +0100 Subject: [PATCH 2/5] Improve unexpected error handling and add test cases for it --- .../darkswarm/services/checkout.js.coffee | 5 +++- .../services/checkout_spec.js.coffee | 28 ++++++++++++++++++- 2 files changed, 31 insertions(+), 2 deletions(-) diff --git a/app/assets/javascripts/darkswarm/services/checkout.js.coffee b/app/assets/javascripts/darkswarm/services/checkout.js.coffee index 890d45c2e5..c6e9282c59 100644 --- a/app/assets/javascripts/darkswarm/services/checkout.js.coffee +++ b/app/assets/javascripts/darkswarm/services/checkout.js.coffee @@ -22,11 +22,14 @@ Darkswarm.factory 'Checkout', ($injector, CurrentOrder, ShippingMethods, StripeE if response.data.path Navigation.go response.data.path else + throw response unless response.data.errors || response.data.flash + Loading.clear() @errors = response.data.errors RailsFlashLoader.loadFlash(response.data.flash) catch error - RailsFlashLoader.loadFlash("Unkown error occurred") + RailsFlashLoader.loadFlash(error: t("checkout.failed")) # inform the user about the unexpected error + throw error # generate a BugsnagJS alert # Rails wants our Spree::Address data to be provided with _attributes preprocess: -> diff --git a/spec/javascripts/unit/darkswarm/services/checkout_spec.js.coffee b/spec/javascripts/unit/darkswarm/services/checkout_spec.js.coffee index 6b1b7d6e40..b8575f5336 100644 --- a/spec/javascripts/unit/darkswarm/services/checkout_spec.js.coffee +++ b/spec/javascripts/unit/darkswarm/services/checkout_spec.js.coffee @@ -6,7 +6,9 @@ describe 'Checkout service', -> flash = null scope = null FlashLoaderMock = - loadFlash: (arg)-> + loadFlash: (arg) -> + Loading = + clear: (arg)-> paymentMethods = [{ id: 99 test: "foo" @@ -48,6 +50,7 @@ describe 'Checkout service', -> module 'Darkswarm' module ($provide)-> $provide.value "RailsFlashLoader", FlashLoaderMock + $provide.value "Loading", Loading $provide.value "currentOrder", orderData $provide.value "shippingMethods", shippingMethods $provide.value "paymentMethods", paymentMethods @@ -123,6 +126,29 @@ describe 'Checkout service', -> $httpBackend.flush() expect(Checkout.errors).toEqual {error: "frogs"} + it "throws an exception and sends a flash message to the flash service when reponse doesnt contain errors nor a flash message", -> + spyOn(FlashLoaderMock, "loadFlash") # Stubbing out writes to window.location + $httpBackend.expectPUT("/checkout.json").respond 400, "broken response" + try + Checkout.submit() + $httpBackend.flush() + catch e + expect(e.data).toBe("broken response") + + expect(FlashLoaderMock.loadFlash).toHaveBeenCalledWith({ error: t("checkout.failed") }) + + it "throws an exception and sends a flash message to the flash service when an exception is thrown while handling the error", -> + spyOn(FlashLoaderMock, "loadFlash") # Stubbing out writes to window.location + spyOn(Loading, "clear").and.callFake(-> throw "unexpected error") + $httpBackend.expectPUT("/checkout.json").respond 400, {flash: {error: "frogs"}} + try + Checkout.submit() + $httpBackend.flush() + catch e + expect(e).toBe("unexpected error") + + expect(FlashLoaderMock.loadFlash).toHaveBeenCalledWith({ error: t("checkout.failed") }) + describe "when using the Stripe Connect gateway", -> beforeEach inject ($injector, StripeElements) -> Checkout.order.payment_method_id = 666 From 62471bf2abfa2056119ffda116ea0fe89332e724 Mon Sep 17 00:00:00 2001 From: Luis Ramos Date: Tue, 14 Apr 2020 13:50:38 +0100 Subject: [PATCH 3/5] Clear Loading spinner when exception is caught --- .../javascripts/darkswarm/services/checkout.js.coffee | 1 + .../unit/darkswarm/services/checkout_spec.js.coffee | 7 ++++--- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/app/assets/javascripts/darkswarm/services/checkout.js.coffee b/app/assets/javascripts/darkswarm/services/checkout.js.coffee index c6e9282c59..d974004d92 100644 --- a/app/assets/javascripts/darkswarm/services/checkout.js.coffee +++ b/app/assets/javascripts/darkswarm/services/checkout.js.coffee @@ -28,6 +28,7 @@ Darkswarm.factory 'Checkout', ($injector, CurrentOrder, ShippingMethods, StripeE @errors = response.data.errors RailsFlashLoader.loadFlash(response.data.flash) catch error + Loading.clear() RailsFlashLoader.loadFlash(error: t("checkout.failed")) # inform the user about the unexpected error throw error # generate a BugsnagJS alert diff --git a/spec/javascripts/unit/darkswarm/services/checkout_spec.js.coffee b/spec/javascripts/unit/darkswarm/services/checkout_spec.js.coffee index b8575f5336..263fe502f2 100644 --- a/spec/javascripts/unit/darkswarm/services/checkout_spec.js.coffee +++ b/spec/javascripts/unit/darkswarm/services/checkout_spec.js.coffee @@ -3,6 +3,7 @@ describe 'Checkout service', -> orderData = null $httpBackend = null Navigation = null + navigationSpy = null flash = null scope = null FlashLoaderMock = @@ -64,7 +65,7 @@ describe 'Checkout service', -> scope.Checkout = Checkout Navigation = $injector.get("Navigation") flash = $injector.get("flash") - spyOn(Navigation, "go") # Stubbing out writes to window.location + navigationSpy = spyOn(Navigation, "go") # Stubbing out writes to window.location it "defaults to no shipping method", -> expect(Checkout.order.shipping_method_id).toEqual null @@ -139,8 +140,8 @@ describe 'Checkout service', -> it "throws an exception and sends a flash message to the flash service when an exception is thrown while handling the error", -> spyOn(FlashLoaderMock, "loadFlash") # Stubbing out writes to window.location - spyOn(Loading, "clear").and.callFake(-> throw "unexpected error") - $httpBackend.expectPUT("/checkout.json").respond 400, {flash: {error: "frogs"}} + navigationSpy.and.callFake(-> throw "unexpected error") + $httpBackend.expectPUT("/checkout.json").respond 400, {path: 'path'} try Checkout.submit() $httpBackend.flush() From 47a93568dcecfcd03b164848ed7fd890ae85c164 Mon Sep 17 00:00:00 2001 From: Luis Ramos Date: Tue, 14 Apr 2020 13:55:20 +0100 Subject: [PATCH 4/5] Make code simpler by extracting methods --- .../darkswarm/services/checkout.js.coffee | 25 +++++++++++-------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/app/assets/javascripts/darkswarm/services/checkout.js.coffee b/app/assets/javascripts/darkswarm/services/checkout.js.coffee index d974004d92..3b7c2756d7 100644 --- a/app/assets/javascripts/darkswarm/services/checkout.js.coffee +++ b/app/assets/javascripts/darkswarm/services/checkout.js.coffee @@ -19,19 +19,24 @@ Darkswarm.factory 'Checkout', ($injector, CurrentOrder, ShippingMethods, StripeE Navigation.go response.data.path .catch (response) => try - if response.data.path - Navigation.go response.data.path - else - throw response unless response.data.errors || response.data.flash - - Loading.clear() - @errors = response.data.errors - RailsFlashLoader.loadFlash(response.data.flash) + @handle_checkout_error_response(response) catch error - Loading.clear() - RailsFlashLoader.loadFlash(error: t("checkout.failed")) # inform the user about the unexpected error + @loadFlash(error: t("checkout.failed")) # inform the user about the unexpected error throw error # generate a BugsnagJS alert + handle_checkout_error_response: (response) => + if response.data.path + Navigation.go response.data.path + else + throw response unless response.data.errors || response.data.flash + + @errors = response.data.errors + @loadFlash(response.data.flash) + + loadFlash: (flash) => + Loading.clear() + RailsFlashLoader.loadFlash(flash) + # Rails wants our Spree::Address data to be provided with _attributes preprocess: -> munged_order = From cedf1b26f2ed1af96f8a61edc2c79379786af599 Mon Sep 17 00:00:00 2001 From: Luis Ramos Date: Tue, 14 Apr 2020 14:24:08 +0100 Subject: [PATCH 5/5] If no flash is sent from the server, show the generic error --- .../darkswarm/services/checkout.js.coffee | 2 +- .../services/checkout_spec.js.coffee | 22 ++++++++++++++----- 2 files changed, 17 insertions(+), 7 deletions(-) diff --git a/app/assets/javascripts/darkswarm/services/checkout.js.coffee b/app/assets/javascripts/darkswarm/services/checkout.js.coffee index 3b7c2756d7..f0da646ebe 100644 --- a/app/assets/javascripts/darkswarm/services/checkout.js.coffee +++ b/app/assets/javascripts/darkswarm/services/checkout.js.coffee @@ -28,7 +28,7 @@ Darkswarm.factory 'Checkout', ($injector, CurrentOrder, ShippingMethods, StripeE if response.data.path Navigation.go response.data.path else - throw response unless response.data.errors || response.data.flash + throw response unless response.data.flash @errors = response.data.errors @loadFlash(response.data.flash) diff --git a/spec/javascripts/unit/darkswarm/services/checkout_spec.js.coffee b/spec/javascripts/unit/darkswarm/services/checkout_spec.js.coffee index 263fe502f2..d7e23b457b 100644 --- a/spec/javascripts/unit/darkswarm/services/checkout_spec.js.coffee +++ b/spec/javascripts/unit/darkswarm/services/checkout_spec.js.coffee @@ -120,21 +120,31 @@ describe 'Checkout service', -> $httpBackend.flush() expect(FlashLoaderMock.loadFlash).toHaveBeenCalledWith {error: "frogs"} - it "puts errors into the scope", -> - $httpBackend.expectPUT("/checkout.json").respond 400, {errors: {error: "frogs"}} + it "puts errors into the scope when there is a flash messages", -> + $httpBackend.expectPUT("/checkout.json").respond 400, {errors: {error: "frogs"}, flash: {error: "flash frogs"}} Checkout.submit() $httpBackend.flush() expect(Checkout.errors).toEqual {error: "frogs"} + it "throws exception and sends generic flash message when there are errors but no flash message", -> + $httpBackend.expectPUT("/checkout.json").respond 400, {errors: {error: "broken response"}} + try + Checkout.submit() + $httpBackend.flush() + catch error + expect(error.data.errors.error).toBe("broken response") + + expect(Checkout.errors).toEqual {} + it "throws an exception and sends a flash message to the flash service when reponse doesnt contain errors nor a flash message", -> spyOn(FlashLoaderMock, "loadFlash") # Stubbing out writes to window.location $httpBackend.expectPUT("/checkout.json").respond 400, "broken response" try Checkout.submit() $httpBackend.flush() - catch e - expect(e.data).toBe("broken response") + catch error + expect(error.data).toBe("broken response") expect(FlashLoaderMock.loadFlash).toHaveBeenCalledWith({ error: t("checkout.failed") }) @@ -145,8 +155,8 @@ describe 'Checkout service', -> try Checkout.submit() $httpBackend.flush() - catch e - expect(e).toBe("unexpected error") + catch error + expect(error).toBe("unexpected error") expect(FlashLoaderMock.loadFlash).toHaveBeenCalledWith({ error: t("checkout.failed") })