fix(client): basket errors throw 500 #2711

TDA · 2015-07-08T23:01:51Z

Fixes #2644
Should be green
@vladikoff
@shane-tomlinson

TDA · 2015-07-08T23:02:22Z

app/scripts/lib/marketing-email-client.js

@@ -30,7 +30,8 @@ define([
        url: url,
        type: method,
        accessToken: accessToken,
-        data: data
+        data: data,
+        timeout: 2500


This fixes the subscribe/unsubscribe issue, and returns an error as expected.

Let's make 2500 a named constant like DEFAULT_XHR_TIMEOUT_MS and allow an override to be passed in to the MarketingEmailClient constructor.

TDA · 2015-07-08T23:05:00Z

app/scripts/views/settings/communication_preferences.js

-          self.$('.spinner').hide();
-          self.displayError(err);
+          self.$('.spinner').hide(); // <-- I don't think this ever works as intended
+          self.displayError(err); // <-- This logs to the console, but should actually display .error
          return false;


self returns the proper object (a div with class communication-preferences as expected), but fails to hide the .spinner or display the .error class.

shane-tomlinson · 2015-07-09T10:51:44Z

app/scripts/views/settings/communication_preferences.js

+            $('.spinner').hide();
+            $('.error').html(err).show();
+            setTimeout(function() {
+              self.navigate('/');


Let's just inform the user, without doing a redirect, for now.

If the service is unavailable, we should probably hide the subscribe & unsubscribe buttons, or just not render them in the first place.

Let's also log any error that occurs, it doesn't look like that is being done unless the user is redirected to /unexpected_error`, which is both unfortunate and unexpected.

If the error is thrown at the /lookup-user, the buttons arent displayed at all. Actually nothing is displayed, so I added only the error message now, is that all right?

@shane-tomlinson I did not get the logging part, I dont see anything being logged for the unexpected_error page.

TDA · 2015-07-09T20:49:13Z

r? @vladikoff @shane-tomlinson

jrgm · 2015-07-12T02:42:20Z

So, re: https://bugzilla.mozilla.org/show_bug.cgi?id=1182679 it seems our handling of 400 responses needs some work too.

TDA · 2015-07-12T03:52:20Z

@jrgm What do you suggest jrgm? Do we consider that a separate issue, or try to fix that in this? I think the former, but let me know if its the latter :)

shane-tomlinson · 2015-07-13T22:21:51Z

app/scripts/views/settings/communication_preferences.js

      return emailPrefs.fetch()
        .fail(function (err) {
          if (MarketingEmailErrors.is(err, 'UNKNOWN_EMAIL')) {
            // user has not yet opted in to Basket yet. Ignore.
            return;
          }
+          if (MarketingEmailErrors.is(err, 'SERVICE_UNAVAILABLE')) {


I think we should remove the if statement and use this code generically + log to the console.

TDA · 2015-07-14T21:14:44Z

@shane-tomlinson final r?

vladikoff · 2015-07-14T21:25:22Z

@shane-tomlinson @TDA we probably need unit tests for this behaviour

TDA · 2015-07-14T22:25:51Z

@vladikoff Isnt there a test for this already in /app/tests/spec/views/settings?

it('shows any other fetch errors', function () {
        emailPrefsModel.fetch.restore();
        sinon.stub(emailPrefsModel, 'fetch', function () {
          return p.reject(MarketingEmailErrors.toError('USAGE_ERROR'));
        });

        return render()
          .then(function () {
            assert.isTrue(view.isErrorVisible());
          });

shane-tomlinson · 2015-07-16T00:34:22Z

OK, I think this is going to be slightly more complex than I thought. When testing locally, I see the following when opening "communication preferences":

The problems:

There should be no Error: before the actual error. When printing the error, I think err should be err.message || err)
The user has no way to go back.

I'm trying to think of the best way to handle this.

I think it will be to save the error to an instance variable, allow the page to display, and in the template, check if there is an error. If an error occurred, then print the error but not the rest of the form. For a reference of another view that does this, see complete_signup.js

shane-tomlinson · 2015-07-16T00:36:54Z

When printing the error, I think err should be err.message || err)

Ahha, complete_signup.js uses self.translateError(err) to handle this.

TDA · 2015-07-16T17:37:51Z

@shane-tomlinson Should be good to go once Travis and CCI turn green :)

shane-tomlinson · 2015-07-16T18:38:38Z

app/scripts/views/settings/communication_preferences.js

+          // View isnt rendered yet, so display the spinner manually.
+          $('.spinner').hide();
+          self._error = self.translateError(err);
+          return;


nix this return

shane-tomlinson · 2015-07-16T23:26:17Z

app/scripts/lib/errors.js

        return this.toError('UNEXPECTED_ERROR');
      }

-      return this.toError(errObj.errno);
+      var normalizedError = this.toError(serverError.errno);
+      normalizedError.code = serverError.code;


Can you add a comment above this line about what the code represents and why the code is transferred across?

Fixes mozilla#2644 set a timeout for ajax requests display error to user and log to console handles both 400 and 500 errors

shane-tomlinson · 2015-07-17T00:22:40Z

Manually merged in 70b9df4, closing.

TDA added WIP waffle:needs review and removed WIP labels Jul 8, 2015

TDA reviewed Jul 8, 2015
View reviewed changes

pdehaan added the WIP label Jul 8, 2015

TDA reviewed Jul 8, 2015
View reviewed changes

vladikoff changed the title ~~fix(client): basket errors throw 500~~ WIP fix(client): basket errors throw 500 Jul 8, 2015

shane-tomlinson reviewed Jul 9, 2015
View reviewed changes

TDA force-pushed the issue-2644-basket-500-errors branch 2 times, most recently from d8b57ea to 6392c11 Compare July 9, 2015 19:25

TDA removed the WIP label Jul 9, 2015

TDA changed the title ~~WIP fix(client): basket errors throw 500~~ fix(client): basket errors throw 500 Jul 9, 2015

rfk assigned vladikoff Jul 11, 2015

rfk added this to the train 42 milestone Jul 11, 2015

rfk added the waffle:review label Jul 13, 2015

vladikoff assigned shane-tomlinson and unassigned vladikoff Jul 13, 2015

shane-tomlinson reviewed Jul 13, 2015
View reviewed changes

TDA force-pushed the issue-2644-basket-500-errors branch 3 times, most recently from 2ece498 to 1be0c10 Compare July 14, 2015 16:49

TDA force-pushed the issue-2644-basket-500-errors branch 3 times, most recently from ce5317d to b4eccc4 Compare July 14, 2015 23:22

TDA force-pushed the issue-2644-basket-500-errors branch from b4eccc4 to 1c3f79e Compare July 16, 2015 17:37

shane-tomlinson reviewed Jul 16, 2015
View reviewed changes

TDA force-pushed the issue-2644-basket-500-errors branch 3 times, most recently from efd0435 to 0c17199 Compare July 16, 2015 23:03

shane-tomlinson reviewed Jul 16, 2015
View reviewed changes

TDA force-pushed the issue-2644-basket-500-errors branch from 0c17199 to da8c5db Compare July 16, 2015 23:49

fix(client): basket errors throw 500

a337b4e

Fixes mozilla#2644 set a timeout for ajax requests display error to user and log to console handles both 400 and 500 errors

TDA force-pushed the issue-2644-basket-500-errors branch from da8c5db to a337b4e Compare July 17, 2015 00:00

shane-tomlinson closed this Jul 17, 2015

shane-tomlinson removed the waffle:needs review label Jul 17, 2015

ckarlof removed the waffle:review label Jul 17, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(client): basket errors throw 500 #2711

fix(client): basket errors throw 500 #2711

TDA commented Jul 8, 2015

TDA Jul 8, 2015

shane-tomlinson Jul 9, 2015

TDA Jul 9, 2015

TDA Jul 8, 2015

shane-tomlinson Jul 9, 2015

shane-tomlinson Jul 9, 2015

shane-tomlinson Jul 9, 2015

TDA Jul 9, 2015

TDA Jul 9, 2015

TDA commented Jul 9, 2015

jrgm commented Jul 12, 2015

TDA commented Jul 12, 2015

shane-tomlinson Jul 13, 2015

TDA commented Jul 14, 2015

vladikoff commented Jul 14, 2015

TDA commented Jul 14, 2015

shane-tomlinson commented Jul 16, 2015

shane-tomlinson commented Jul 16, 2015

TDA commented Jul 16, 2015

shane-tomlinson Jul 16, 2015

shane-tomlinson Jul 16, 2015

shane-tomlinson commented Jul 17, 2015

fix(client): basket errors throw 500 #2711

fix(client): basket errors throw 500 #2711

Conversation

TDA commented Jul 8, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TDA commented Jul 9, 2015

jrgm commented Jul 12, 2015

TDA commented Jul 12, 2015

Choose a reason for hiding this comment

TDA commented Jul 14, 2015

vladikoff commented Jul 14, 2015

TDA commented Jul 14, 2015

shane-tomlinson commented Jul 16, 2015

shane-tomlinson commented Jul 16, 2015

TDA commented Jul 16, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shane-tomlinson commented Jul 17, 2015