fix(errors): use the correct context for error messages

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.
mozilla · Sep 16, 2014 · 9eba71a · 9eba71a
1 parent 66ebc83
commit 9eba71a
Show file tree

Hide file tree

Showing 5 changed files with 32 additions and 21 deletions.
diff --git a/app/scripts/lib/auth-errors.js b/app/scripts/lib/auth-errors.js
@@ -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;

diff --git a/app/scripts/views/base.js b/app/scripts/views/base.js
@@ -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;
@@ -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);
@@ -318,6 +319,18 @@ 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
@@ -327,8 +340,8 @@ function (_, Backbone, $, p, Session, AuthErrors, Url, Strings,
      * @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);
@@ -367,10 +380,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
@@ -395,6 +406,7 @@ function (_, Backbone, $, p, Session, AuthErrors, Url, Strings,
     },
 
     _normalizeError: function (err, errors) {
+      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.

diff --git a/app/scripts/views/form.js b/app/scripts/views/form.js
@@ -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.
+            self.displayError(err);
+            throw err;
           })
           .then(_.bind(self.afterSubmit, self));
     })),

diff --git a/app/scripts/views/settings/avatar_gravatar.js b/app/scripts/views/settings/avatar_gravatar.js
@@ -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;
         });
     }
   });

diff --git a/app/tests/spec/views/settings/avatar_gravatar.js b/app/tests/spec/views/settings/avatar_gravatar.js
@@ -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());
             });
         });