Improve client XHR error handling

The default XHR error handler produce an alert which is appropriate to the response status code. It can be overridden per-request (by specifying the `errorHandler` option) so that the alert can be suppressed or displayed in a different position (e.g. inside a modal).

ref #118

framework/core/js/admin/src/components/EditGroupModal.js

@@ -85,13 +85,12 @@ export default class EditGroupModal extends Modal {
       namePlural: this.namePlural(),
       color: this.color(),
       icon: this.icon()
-    }).then(
+    }, {errorHandler: this.onerror.bind(this)})
-      () => this.hide(),
+      .then(this.hide.bind(this))
-      (response) => {
+      .catch(() => {
         this.loading = false;
-        this.handleErrors(response);
+        m.redraw();
-      }
+      });
-    );
   }
 
   deleteGroup() {

framework/core/js/admin/src/components/SettingsModal.js

@@ -68,11 +68,8 @@ export default class SettingsModal extends Modal {
     this.loading = true;
 
     saveSettings(this.dirty()).then(
-      () => this.hide(),
+      this.hide.bind(this),
-      () => {
+      this.loaded.bind(this)
-        this.loading = false;
-        m.redraw();
-      }
     );
   }
 }

framework/core/js/forum/src/components/AvatarEditor.js

@@ -163,5 +163,6 @@ export default class AvatarEditor extends Component {
    */
   failure() {
     this.loading = false;
+    m.redraw();
   }
 }

framework/core/js/forum/src/components/ChangeEmailModal.js

@@ -83,16 +83,8 @@ export default class ChangeEmailModal extends Modal {
 
     this.loading = true;
 
-    app.session.user.save({email: this.email()}).then(
+    app.session.user.save({email: this.email()}, {errorHandler: this.onerror.bind(this)})
-      () => {
+      .then(() => this.success = true)
-        this.loading = false;
+      .finally(this.loaded.bind(this));
-        this.success = true;
-        m.redraw();
-      },
-      response => {
-        this.loading = false;
-        this.handleErrors(response);
-      }
-    );
   }
 }

framework/core/js/forum/src/components/ChangePasswordModal.js

@@ -42,8 +42,8 @@ export default class ChangePasswordModal extends Modal {
       url: app.forum.attribute('apiUrl') + '/forgot',
       data: {email: app.session.user.email()}
     }).then(
-      () => this.hide(),
+      this.hide.bind(this),
-      () => this.loading = false
+      this.loaded.bind(this)
     );
   }
 }

framework/core/js/forum/src/components/ComposerBody.js

@@ -102,4 +102,12 @@ export default class ComposerBody extends Component {
    */
   onsubmit() {
   }
+
+  /**
+   * Stop loading.
+   */
+  loaded() {
+    this.loading = false;
+    m.redraw();
+  }
 }

framework/core/js/forum/src/components/DiscussionComposer.js

@@ -109,11 +109,7 @@ export default class DiscussionComposer extends ComposerBody {
         app.cache.discussionList.addDiscussion(discussion);
         m.route(app.route.discussion(discussion));
       },
-      response => {
+      this.loaded.bind(this)
-        this.loading = false;
-        m.redraw();
-        app.alertErrors(response.errors);
-      }
     );
   }
 }

framework/core/js/forum/src/components/DiscussionPage.js

@@ -128,7 +128,7 @@ export default class DiscussionPage extends Page {
       // component for the first time on page load, then any calls to m.redraw
       // will be ineffective and thus any configs (scroll code) will be run
       // before stuff is drawn to the page.
-      setTimeout(this.show.bind(this, preloadedDiscussion));
+      setTimeout(this.show.bind(this, preloadedDiscussion), 0);
     } else {
       const params = this.requestParams();
 

framework/core/js/forum/src/components/EditPostComposer.js

@@ -64,14 +64,8 @@ export default class EditPostComposer extends ComposerBody {
     const data = this.data();
 
     this.props.post.save(data).then(
-      () => {
+      () => app.composer.hide(),
-        app.composer.hide();
+      this.loaded.bind(this)
-        m.redraw();
-      },
-      () => {
-        this.loading = false;
-        m.redraw();
-      }
     );
   }
 }

framework/core/js/forum/src/components/EditUserModal.js

@@ -102,11 +102,7 @@ export default class EditUserModal extends Modal {
     );
   }
 
-  onsubmit(e) {
+  data() {
-    e.preventDefault();
-
-    this.loading = true;
-
     const groups = Object.keys(this.groups)
       .filter(id => this.groups[id]())
       .map(id => app.store.getById('groups', id));
@@ -121,12 +117,19 @@ export default class EditUserModal extends Modal {
       data.password = this.password();
     }
 
-    this.props.user.save(data).then(
+    return data;
-      () => this.hide(),
+  }
-      response => {
+
+  onsubmit(e) {
+    e.preventDefault();
+
+    this.loading = true;
+
+    this.props.user.save(this.data(), {errorHandler: this.onerror.bind(this)})
+      .then(this.hide.bind(this))
+      .catch(() => {
         this.loading = false;
-        this.handleErrors(response);
+        m.redraw();
-      }
+      });
-    );
   }
 }

framework/core/js/forum/src/components/ForgotPasswordModal.js

@@ -85,24 +85,12 @@ export default class ForgotPasswordModal extends Modal {
     app.request({
       method: 'POST',
       url: app.forum.attribute('apiUrl') + '/forgot',
-      data: {email: this.email()},
+      data: {email: this.email()}
-      handlers: {
+    })
-        404: () => {
+      .then(() => {
-          this.alert = new Alert({type: 'warning', message: 'That email wasn\'t found in our database.'});
-          throw new Error();
-        }
-      }
-    }).then(
-      () => {
-        this.loading = false;
         this.success = true;
         this.alert = null;
-        m.redraw();
+      })
-      },
+      .finally(this.loaded.bind(this));
-      response => {
-        this.loading = false;
-        this.handleErrors(response);
-      }
-    );
   }
 }

framework/core/js/forum/src/components/LogInModal.js

@@ -124,25 +124,25 @@ export default class LogInModal extends Modal {
     const email = this.email();
     const password = this.password();
 
-    app.session.login(email, password).then(
+    app.session.login(email, password, {errorHandler: this.onerror.bind(this)})
-      null,
+      .catch(this.loaded.bind(this));
-      response => {
-        this.loading = false;
-
-        if (response && response.code === 'confirm_email') {
-          this.alert = Alert.component({
-            children: app.trans('core.forum.log_in_confirmation_required_message', {email: response.email})
-          });
-        } else {
-          this.alert = Alert.component({
-            type: 'error',
-            children: app.trans('core.forum.log_in_invalid_login_message')
-          });
   }
 
-        m.redraw();
+  onerror(error) {
-        this.onready();
+    switch (error.status) {
+      case 401:
+        error.alert.props.children = app.trans('core.forum.log_in_confirmation_required_message', {email: error.response.emailConfirmationRequired});
+        delete error.alert.props.type;
+        break;
+
+      case 404:
+        error.alert.props.children = app.trans('core.forum.log_in_invalid_login_message');
+        break;
+
+      default:
+        // no default
     }
-    );
+
+    super.onerror(error);
   }
 }

framework/core/js/forum/src/components/NotificationList.js

@@ -115,13 +115,12 @@ export default class NotificationList extends Component {
     this.loading = true;
     m.redraw();
 
-    app.store.find('notifications').then(notifications => {
+    app.store.find('notifications')
+      .then(notifications => {
         app.session.user.pushAttributes({newNotificationsCount: 0});
         app.cache.notifications = notifications.sort((a, b) => b.time() - a.time());
-
+      })
-      this.loading = false;
+      .finally(this.loaded.bind(this));
-      m.redraw();
-    });
   }
 
   /**

framework/core/js/forum/src/components/ReplyComposer.js

@@ -36,7 +36,8 @@ export default class ReplyComposer extends ComposerBody {
 
     items.add('title', (
       <h3>
-        {icon('reply')}{' '}<a href={app.route.discussion(discussion)} config={m.route}>{discussion.title()}</a>
+        {icon('reply')} {' '}
+        <a href={app.route.discussion(discussion)} config={m.route}>{discussion.title()}</a>
       </h3>
     ));
 
@@ -93,11 +94,7 @@ export default class ReplyComposer extends ComposerBody {
 
         app.composer.hide();
       },
-      response => {
+      this.loaded.bind(this)
-        this.loading = false;
-        m.redraw();
-        app.alertErrors(response.errors);
-      }
     );
   }
 }

framework/core/js/forum/src/components/SignUpModal.js

@@ -178,7 +178,8 @@ export default class SignUpModal extends Modal {
     app.request({
       url: app.forum.attribute('baseUrl') + '/register',
       method: 'POST',
-      data
+      data,
+      errorHandler: this.onerror.bind(this)
     }).then(
       payload => {
         const user = app.store.pushPayload(payload);
@@ -190,14 +191,10 @@ export default class SignUpModal extends Modal {
           window.location.reload();
         } else {
           this.welcomeUser = user;
-          this.loading = false;
+          this.loaded();
-          m.redraw();
         }
       },
-      response => {
+      this.loaded.bind(this)
-        this.loading = false;
-        this.handleErrors(response);
-      }
     );
   }
 

framework/core/js/forum/src/components/UserBio.js

@@ -91,10 +91,7 @@ export default class UserBio extends Component {
     if (user.bio() !== value) {
       this.loading = true;
 
-      user.save({bio: value}).then(() => {
+      user.save({bio: value}).finally(this.loaded.bind(this));
-        this.loading = false;
-        m.redraw();
-      });
     }
 
     this.editing = false;

framework/core/js/lib/App.js

@@ -206,10 +206,14 @@ export default class App {
       try {
         return JSON.parse(responseText);
       } catch (e) {
-        throw new RequestError(e.message, responseText);
+        throw new RequestError(500, responseText);
       }
     });
 
+    options.errorHandler = options.errorHandler || (error => {
+      throw error;
+    });
+
     // When extracting the data from the response, we can check the server
     // response code and show an error message to the user if something's gone
     // awry.
@@ -225,26 +229,56 @@ export default class App {
 
       const status = xhr.status;
 
-      if (status >= 500 && status <= 599) {
+      if (status < 200 || status > 299) {
-        throw new RequestError('Internal Server Error', responseText);
+        throw new RequestError(status, responseText, xhr);
       }
 
       return responseText;
     };
 
-    this.alerts.dismiss(this.requestErrorAlert);
+    if (this.requestError) this.requestError.hideAlert();
 
     // Now make the request. If it's a failure, inspect the error that was
     // returned and show an alert containing its contents.
     return m.request(options).then(null, error => {
-      if (error instanceof RequestError) {
+      this.requestError = error;
-        this.alerts.show(this.requestErrorAlert = new Alert({
+
+      let children;
+
+      switch (error.status) {
+        case 422:
+          children = error.response.errors
+            .map(error => [error.detail, <br/>])
+            .reduce((a, b) => a.concat(b), [])
+            .slice(0, -1);
+          break;
+
+        case 401:
+        case 403:
+          children = 'You do not have permission to do that.';
+          break;
+
+        case 404:
+        case 410:
+          children = 'The requested resource was not found.';
+          break;
+
+        default:
+          children = 'Oops! Something went wrong. Please reload the page and try again.';
+      }
+
+      error.alert = new Alert({
         type: 'error',
-          children: 'Oops! Something went wrong. Please reload the page and try again.',
+        children,
         controls: app.forum.attribute('debug') ? [
           <Button className="Button Button--link" onclick={this.showDebug.bind(this, error)}>Debug</Button>
         ] : undefined
-        }));
+      });
+
+      try {
+        options.errorHandler(error);
+      } catch (error) {
+        this.alerts.show(error.alert);
       }
 
       throw error;
@@ -264,17 +298,19 @@ export default class App {
   /**
    * Show alert error messages for each error returned in an API response.
    *
-   * @param {Array} errors
+   * @param {Object} response
    * @public
    */
-  alertErrors(errors) {
+  alertErrors(response) {
-    errors.forEach(error => {
+    if (response.errors) {
+      response.errors.forEach(error => {
         this.alerts.show(new Alert({
           type: 'error',
           children: error.detail
         }));
       });
     }
+  }
 
   /**
    * Construct a URL to the route with the given name.

framework/core/js/lib/Model.js

@@ -117,10 +117,11 @@ export default class Model {
    *
    * @param {Object} attributes The attributes to save. If a 'relationships' key
    *     exists, it will be extracted and relationships will also be saved.
+   * @param {Object} [options]
    * @return {Promise}
    * @public
    */
-  save(attributes) {
+  save(attributes, options = {}) {
     const data = {
       type: this.data.type,
       id: this.data.id,
@@ -153,11 +154,11 @@ export default class Model {
 
     this.pushData(data);
 
-    return app.request({
+    return app.request(Object.assign({
       method: this.exists ? 'PATCH' : 'POST',
       url: app.forum.attribute('apiUrl') + this.apiEndpoint(),
       data: {data}
-    }).then(
+    }, options)).then(
       // If everything went well, we'll make sure the store knows that this
       // model exists now (if it didn't already), and we'll push the data that
       // the API returned into the store.
@@ -181,17 +182,18 @@ export default class Model {
    * Send a request to delete the resource.
    *
    * @param {Object} data Data to send along with the DELETE request.
+   * @param {Object} [options]
    * @return {Promise}
    * @public
    */
-  delete(data) {
+  delete(data, options = {}) {
     if (!this.exists) return m.deferred.resolve().promise;
 
-    return app.request({
+    return app.request(Object.assign({
       method: 'DELETE',
       url: app.forum.attribute('apiUrl') + this.apiEndpoint(),
       data
-    }).then(() => {
+    }, options)).then(() => {
       this.exists = false;
       this.store.remove(this);
     });

framework/core/js/lib/Session.js

@@ -26,15 +26,16 @@ export default class Session {
    *
    * @param {String} identification The username/email.
    * @param {String} password
+   * @param {Object} [options]
    * @return {Promise}
    * @public
    */
-  login(identification, password) {
+  login(identification, password, options = {}) {
-    return app.request({
+    return app.request(Object.assign({
       method: 'POST',
       url: app.forum.attribute('baseUrl') + '/login',
       data: {identification, password}
-    })
+    }, options))
       .then(() => window.location.reload());
   }
 

framework/core/js/lib/Store.js

@@ -79,10 +79,11 @@ export default class Store {
    *     Alternatively, if an object is passed, it will be handled as the
    *     `query` parameter.
    * @param {Object} [query]
+   * @param {Object} [options]
    * @return {Promise}
    * @public
    */
-  find(type, id, query = {}) {
+  find(type, id, query = {}, options = {}) {
     let data = query;
     let url = app.forum.attribute('apiUrl') + '/' + type;
 
@@ -94,11 +95,11 @@ export default class Store {
       url += '/' + id;
     }
 
-    return app.request({
+    return app.request(Object.assign({
       method: 'GET',
       url,
       data
-    }).then(this.pushPayload.bind(this));
+    }, options)).then(this.pushPayload.bind(this));
   }
 
   /**

framework/core/js/lib/components/Modal.js

@@ -109,27 +109,28 @@ export default class Modal extends Component {
   }
 
   /**
-   * Show an alert describing errors returned from the API, and give focus to
+   * Stop loading.
+   */
+  loaded() {
+    this.loading = false;
+    m.redraw();
+  }
+
+  /**
+   * Show an alert describing an error returned from the API, and give focus to
    * the first relevant field.
    *
-   * @param {Object} response
+   * @param {RequestError} error
    */
-  handleErrors(response) {
+  onerror(error) {
-    const errors = response && response.errors;
+    this.alert = error.alert;
-
-    if (errors) {
-      this.alert = new Alert({
-        type: 'error',
-        children: errors.map((error, k) => [error.detail, k < errors.length - 1 ? m('br') : ''])
-      });
-    }
 
     m.redraw();
 
-    if (errors) {
+    if (error.status === 422 && error.response.errors) {
-      this.$('form [name=' + errors[0].source.pointer.replace('/data/attributes/', '') + ']').select();
+      this.$('form [name=' + error.response.errors[0].source.pointer.replace('/data/attributes/', '') + ']').select();
     } else {
-      this.$('form :input:first').select();
+      this.onready();
     }
   }
 }

framework/core/js/lib/components/RequestErrorModal.js

@@ -5,10 +5,6 @@ export default class RequestErrorModal extends Modal {
     return 'RequestErrorModal Modal--large';
   }
 
-  title() {
-    return this.props.error.message;
-  }
-
   content() {
     let responseText;
 

framework/core/js/lib/utils/RequestError.js

@@ -1,6 +1,15 @@
 export default class RequestError {
-  constructor(message, responseText) {
+  constructor(status, responseText, xhr) {
-    this.message = message;
+    this.status = status;
     this.responseText = responseText;
+    this.xhr = xhr;
+
+    try {
+      this.response = JSON.parse(responseText);
+    } catch (e) {
+      this.response = null;
+    }
+
+    this.alert = null;
   }
 }

framework/core/src/Api/Controller/TokenController.php

@@ -12,11 +12,11 @@ namespace Flarum\Api\Controller;
 
 use Flarum\Api\Command\GenerateAccessToken;
 use Flarum\Core\Repository\UserRepository;
-use Flarum\Core\Exception\PermissionDeniedException;
 use Flarum\Event\UserEmailChangeWasRequested;
 use Flarum\Http\Controller\ControllerInterface;
 use Illuminate\Contracts\Bus\Dispatcher as BusDispatcher;
 use Illuminate\Contracts\Events\Dispatcher as EventDispatcher;
+use Illuminate\Database\Eloquent\ModelNotFoundException;
 use Psr\Http\Message\ServerRequestInterface;
 use Zend\Diactoros\Response\JsonResponse;
 
@@ -62,16 +62,13 @@ class TokenController implements ControllerInterface
         $user = $this->users->findByIdentification($identification);
 
         if (! $user || ! $user->checkPassword($password)) {
-            throw new PermissionDeniedException;
+            throw new ModelNotFoundException;
         }
 
         if (! $user->is_activated) {
             $this->events->fire(new UserEmailChangeWasRequested($user, $user->email));
 
-            return new JsonResponse([
+            return new JsonResponse(['emailConfirmationRequired' => $user->email], 401);
-                'code' => 'confirm_email',
-                'email' => $user->email
-            ], 401);
         }
 
         $token = $this->bus->dispatch(

framework/core/src/Core/Command/EditUserHandler.php

@@ -63,6 +63,7 @@ class EditUserHandler
 
         $attributes = array_get($data, 'attributes', []);
         $relationships = array_get($data, 'relationships', []);
+        $validate = [];
 
         if (isset($attributes['username'])) {
             $this->assertPermission($canEdit);
@@ -72,6 +73,10 @@ class EditUserHandler
         if (isset($attributes['email'])) {
             if ($isSelf) {
                 $user->requestEmailChange($attributes['email']);
+
+                if ($attributes['email'] !== $user->email) {
+                    $validate['email'] = $attributes['email'];
+                }
             } else {
                 $this->assertPermission($canEdit);
                 $user->changeEmail($attributes['email']);
@@ -81,6 +86,8 @@ class EditUserHandler
         if (isset($attributes['password'])) {
             $this->assertPermission($canEdit);
             $user->changePassword($attributes['password']);
+
+            $validate['password'] = $attributes['password'];
         }
 
         if (isset($attributes['bio'])) {
@@ -127,7 +134,7 @@ class EditUserHandler
             new UserWillBeSaved($user, $actor, $data)
         );
 
-        $this->validator->assertValid(array_merge($user->getDirty(), array_only($attributes, ['password', 'email'])));
+        $this->validator->assertValid(array_merge($user->getDirty(), $validate));
 
         $user->save();
 

framework/core/src/Forum/Controller/LoginController.php

@@ -55,12 +55,11 @@ class LoginController implements ControllerInterface
         $actor = $request->getAttribute('actor');
         $params = array_only($request->getParsedBody(), ['identification', 'password']);
 
-        $data = json_decode($this->apiClient->send($controller, $actor, [], $params)->getBody());
+        $response = $this->apiClient->send($controller, $actor, [], $params);
+
+        if ($response->getStatusCode() === 200) {
+            $data = json_decode($response->getBody());
 
-        // TODO: The client needs to pass through exceptions(?) or the whole
-        // response so we can look at the response code. For now if there isn't
-        // any useful data we just assume it's a 401.
-        if (isset($data->userId)) {
             // Extend the token's expiry to 2 weeks so that we can set a
             // remember cookie
             AccessToken::where('id', $data->token)->update(['expires_at' => new DateTime('+2 weeks')]);
@@ -68,11 +67,11 @@ class LoginController implements ControllerInterface
             event(new UserLoggedIn($this->users->findOrFail($data->userId), $data->token));
 
             return $this->withRememberCookie(
-                new JsonResponse($data),
+                $response,
                 $data->token
             );
         } else {
-            return new EmptyResponse(401);
+            return $response;
         }
     }
 }