Skip to content
This repository has been archived by the owner on Apr 3, 2019. It is now read-only.

Commit

Permalink
fix(errors): use the correct context for error messages
Browse files Browse the repository at this point in the history
Fixes #1660.

This issue occurred because the context of an error was being discarded as the error was rethrown in the form submission handler. Rather than depend on error context being passed around, we store it on individual error objects.
  • Loading branch information
zaach committed Sep 16, 2014
1 parent 66ebc83 commit a3c4f2a
Show file tree
Hide file tree
Showing 5 changed files with 33 additions and 25 deletions.
1 change: 1 addition & 0 deletions app/scripts/lib/auth-errors.js
Expand Up @@ -174,6 +174,7 @@ function () {
err.errno = this.toCode(type);
err.namespace = this.NAMESPACE;
err.message = message;
err.errorContext = this;

if (context) {
err.context = context;
Expand Down
43 changes: 26 additions & 17 deletions app/scripts/views/base.js
Expand Up @@ -11,14 +11,16 @@ define([
'lib/promise',
'lib/session',
'lib/auth-errors',
'lib/oauth-errors',
'lib/profile',
'lib/url',
'lib/strings',
'lib/ephemeral-messages',
'lib/null-metrics',
'views/mixins/timer-mixin'
],
function (_, Backbone, $, p, Session, AuthErrors, Url, Strings,
EphemeralMessages, NullMetrics, TimerMixin) {
function (_, Backbone, $, p, Session, AuthErrors, OAuthErrors, Profile,
Url, Strings, EphemeralMessages, NullMetrics, TimerMixin) {
var ENTER_BUTTON_CODE = 13;
var DEFAULT_TITLE = window.document.title;
var EPHEMERAL_MESSAGE_ANIMATION_MS = 150;
Expand All @@ -45,16 +47,15 @@ function (_, Backbone, $, p, Session, AuthErrors, Url, Strings,
this._isSuccessVisible = true;
}

function displayError(displayStrategy, err, errors) {
function displayError(displayStrategy, err) {
/*jshint validthis: true*/
this.hideSuccess();
this.$('.spinner').hide();

errors = errors || AuthErrors;
err = this._normalizeError(err, errors);
err = this._normalizeError(err);

this.logError(err, errors);
var translated = this.translateError(err, errors);
this.logError(err);
var translated = this.translateError(err);

if (translated) {
this.$('.error')[displayStrategy](translated);
Expand Down Expand Up @@ -318,17 +319,28 @@ function (_, Backbone, $, p, Session, AuthErrors, Url, Strings,
return !!this._isSuccessVisible;
},

/**
* Return the error module that produced the error, based on the error's
* namespace.
*/
_errorContext: function (err) {
if (err && err.errorContext) {
return err.errorContext;
} else {
return AuthErrors;
}
},

/**
* Display an error message.
* @method translateError
* @param {string} err - an error object
* @param {object} errors - optional Errors object that transforms codes into messages
*
* @return {string} translated error text (if available), untranslated
* error text otw.
*/
translateError: function (err, errors) {
errors = errors || AuthErrors;
translateError: function (err) {
var errors = this._errorContext(err);

var msg = errors.toMessage(err);
var context = errors.toContext(err);
Expand All @@ -342,7 +354,6 @@ function (_, Backbone, $, p, Session, AuthErrors, Url, Strings,
* @method displayError
* @param {string} err - If err is not given, the contents of the
* `.error` element's text will not be updated.
* @param {object} errors - optional Errors object that transforms codes into messages
*
* @return {string} translated error text (if available), untranslated
* error text otw.
Expand All @@ -357,7 +368,6 @@ function (_, Backbone, $, p, Session, AuthErrors, Url, Strings,
* @method displayErrorUnsafe
* @param {string} err - If err is not given, the contents of the
* `.error` element's text will not be updated.
* @param {object} errors - optional Errors object that transforms codes into messages
*
* @return {string} translated error text (if available), untranslated
* error text otw.
Expand All @@ -367,10 +377,8 @@ function (_, Backbone, $, p, Session, AuthErrors, Url, Strings,
/**
* Log an error to the event stream
*/
logError: function (err, errors) {
errors = errors || AuthErrors;

err = this._normalizeError(err, errors);
logError: function (err) {
err = this._normalizeError(err);

// The error could already be logged, if so, abort mission.
// This can occur when `navigate` redirects a user to a different
Expand All @@ -394,7 +402,8 @@ function (_, Backbone, $, p, Session, AuthErrors, Url, Strings,
return screenName;
},

_normalizeError: function (err, errors) {
_normalizeError: function (err) {
var errors = this._errorContext(err);
if (! err) {
// likely an error in logic, display an unexpected error to the
// user and show a console trace to help us debug.
Expand Down
5 changes: 3 additions & 2 deletions app/scripts/views/form.js
Expand Up @@ -184,8 +184,9 @@ function (_, $, p, Validate, AuthErrors, BaseView, Tooltip,
}
})
.then(null, function (err) {
// surface returned message for testing.
throw self.displayError(err);
// display error and surface for testing.

This comment has been minimized.

Copy link
@shane-tomlinson

shane-tomlinson Sep 16, 2014

I dig this change.

self.displayError(err);
throw err;
})
.then(_.bind(self.afterSubmit, self));
})),
Expand Down
3 changes: 0 additions & 3 deletions app/scripts/views/settings/avatar_gravatar.js
Expand Up @@ -88,9 +88,6 @@ function ($, _, md5, FormView, Template, Constants, Session, Profile) {
self.navigate('settings/avatar', {
successUnsafe: t('Courtesy of <a href="https://www.gravatar.com">Gravatar</a>')
});
}, function (err) {
self.displayError(Profile.Errors.toMessage(err));
throw err;
});
}
});
Expand Down
6 changes: 3 additions & 3 deletions app/tests/spec/views/settings/avatar_gravatar.js
Expand Up @@ -115,17 +115,17 @@ function (chai, _, $, sinon, View, RouterMock, ProfileMock, Session, p, Profile)

it('submits and errors', function () {
sinon.stub(profileClientMock, 'postAvatar', function (url) {
return p.reject(Profile.Errors.toError('UNAUTHORIZED'));
return p.reject(Profile.Errors.toError('UNSUPPORTED_PROVIDER'));
});

return view.render()
.then(function () {
return view.submit();
return view.validateAndSubmit();
})
.then(function () {
assert.fail('unexpected success');
}, function (err) {
assert.isTrue(Profile.Errors.is(err, 'UNAUTHORIZED'));
assert.isTrue(Profile.Errors.is(err, 'UNSUPPORTED_PROVIDER'));
assert.isTrue(view.isErrorVisible());
});
});
Expand Down

0 comments on commit a3c4f2a

Please sign in to comment.