Auto merge of #4156 - Turbo87:crate-404, r=locks

Show 404 pages for unknown crates/versions

Instead of redirecting back to a known URL and showing a notification message, we now stay on the requested URL and show an error page instead.

<img width="721" alt="Bildschirmfoto 2021-11-10 um 14 41 29" src="https://user-images.githubusercontent.com/141300/141200350-d31a3b60-79a9-43eb-b104-aee1238d55f3.png">

(though with "Crate/Version not found" instead)

The implementation is a little questionable, but due to the way the `error` substates in Ember.js work I've found no other way to implement this.

This PR is based on #4154.

Author: bors

app/controllers/catch-all.js

@@ -0,0 +1,9 @@
+import Controller from '@ember/controller';
+import { action } from '@ember/object';
+
+export default class CatchAllController extends Controller {
+  @action reload(event) {
+    event.preventDefault();
+    this.model.transition.retry();
+  }
+}

app/routes/catch-all.js

@@ -0,0 +1,73 @@
+import Route from '@ember/routing/route';
+import { inject as service } from '@ember/service';
+
+/**
+ * This is a weird route... but let me explain.
+ *
+ * This is the default route that gets used if no other matching route is found.
+ *
+ * This route is *also* used as a generic error page via:
+ *
+ * ```js
+ * this.router.replaceWith('catch-all', { transition, title: 'Something failed' });
+ * ```
+ *
+ * Ideally we would use the `error` substate/routes of Ember.js, but those don't
+ * update the URL when an error happens. This causes the native back button of the
+ * browser to behave in strange way, so we avoid using the broken built-in error
+ * routes.
+ */
+export default class CatchAllRoute extends Route {
+  @service router;
+
+  /**
+   * If `transitionTo('catch-all', 'foo')` is used, this hook will not get called.
+   * If the second argument is an object, then the second object will be the `model`
+   * of this route, and the `serialize()` hook gets called to figure out what the
+   * URL of this route should be. The URL is automatically assembled from the passed-in
+   * transition object.
+   */
+  serialize({ transition }) {
+    return { path: this.pathForRouteInfo(transition.to) };
+  }
+
+  /**
+   * This internal method takes a `RouteInfo` object from Ember.js (e.g. `transition.to`)
+   * and returns the corresponding `:path` route parameter for this `catch-all` route.
+   * @return {string}
+   */
+  pathForRouteInfo(routeInfo) {
+    let routeName = routeInfo.name;
+    let params = paramsForRouteInfo(routeInfo);
+    let queryParams = routeInfo.queryParams;
+    return this.router.urlFor(routeName, ...params, { queryParams }).slice(1);
+  }
+}
+
+/**
+ * Returns all route parameters for the passed-in `RouteInfo` object.
+ *
+ * These can be used in `router.urlFor(...)` calls.
+ */
+function paramsForRouteInfo(routeInfo) {
+  let routeInfos = [...allRouteInfos(routeInfo)].reverse();
+
+  let params = [];
+  for (let routeInfo of routeInfos) {
+    for (let paramName of routeInfo.paramNames) {
+      params.push(routeInfo.params[paramName]);
+    }
+  }
+  return params;
+}
+
+/**
+ * Iterates upwards through the `RouteInfo` "family tree" until the top-most
+ * `RouteInfo` is reached.
+ */
+function* allRouteInfos(routeInfo) {
+  yield routeInfo;
+  while ((routeInfo = routeInfo.parent)) {
+    yield routeInfo;
+  }
+}

app/routes/crate.js

@@ -3,20 +3,18 @@ import { inject as service } from '@ember/service';
 
 export default class CrateRoute extends Route {
   @service headData;
-  @service notifications;
+  @service router;
   @service store;
 
-  async model(params) {
+  async model(params, transition) {
     try {
       return await this.store.find('crate', params.crate_id);
     } catch (error) {
       if (error.errors?.some(e => e.detail === 'Not Found')) {
-        this.notifications.error(`Crate '${params.crate_id}' does not exist`);
+        this.router.replaceWith('catch-all', { transition, error, title: 'Crate not found' });
       } else {
-        this.notifications.error(`Loading data for the '${params.crate_id}' crate failed. Please try again later!`);
+        this.router.replaceWith('catch-all', { transition, error, title: 'Crate failed to load', tryAgain: true });
       }
-
-      this.replaceWith('index');
     }
   }
 

app/routes/crate/version.js

@@ -6,28 +6,25 @@ import { didCancel } from 'ember-concurrency';
 import { AjaxError } from '../../utils/ajax';
 
 export default class VersionRoute extends Route {
-  @service notifications;
+  @service router;
   @service sentry;
 
-  async model(params) {
+  async model(params, transition) {
     let crate = this.modelFor('crate');
 
     let versions;
     try {
       versions = await crate.get('versions');
-    } catch {
-      this.notifications.error(`Loading data for the '${crate.name}' crate failed. Please try again later!`);
-      this.replaceWith('index');
-      return;
+    } catch (error) {
+      return this.router.replaceWith('catch-all', { transition, error, title: 'Crate failed to load', tryAgain: true });
     }
 
     let version;
     let requestedVersion = params.version_num;
     if (requestedVersion) {
       version = versions.find(version => version.num === requestedVersion);
       if (!version) {
-        this.notifications.error(`Version '${requestedVersion}' of crate '${crate.name}' does not exist`);
-        this.replaceWith('crate.index');
+        return this.router.replaceWith('catch-all', { transition, title: 'Version not found' });
       }
     } else {
       let { defaultVersion } = crate;

app/templates/catch-all.hbs

@@ -2,10 +2,12 @@
   <div local-class="content">
     {{svg-jar "cuddlyferris" local-class="logo"}}
 
-    <h1 local-class="title" data-test-title>Page not found</h1>
+    <h1 local-class="title" data-test-title>{{or @model.title "Page not found"}}</h1>
 
-    <a href="javascript:history.back()" local-class="link">
-      Go Back
-    </a>
+    {{#if @model.tryAgain}}
+      <a href="javascript:location.reload()" local-class="link" data-test-try-again {{on "click" this.reload}}>Try Again</a>
+    {{else}}
+      <a href="javascript:history.back()" local-class="link" data-test-go-back>Go Back</a>
+    {{/if}}
   </div>
 </div>

tests/acceptance/404-test.js

@@ -13,6 +13,8 @@ module('Acceptance | 404', function (hooks) {
     assert.equal(currentURL(), '/unknown-route');
     assert.dom('[data-test-404-page]').exists();
     assert.dom('[data-test-title]').hasText('Page not found');
+    assert.dom('[data-test-go-back]').exists();
+    assert.dom('[data-test-try-again]').doesNotExist();
 
     await percySnapshot(assert);
   });

tests/acceptance/crate-test.js

@@ -83,18 +83,22 @@ module('Acceptance | crate page', function (hooks) {
 
   test('unknown crate shows an error message', async function (assert) {
     await visit('/crates/nanomsg');
-    assert.equal(currentURL(), '/');
-    assert.dom('[data-test-notification-message]').hasText("Crate 'nanomsg' does not exist");
+    assert.equal(currentURL(), '/crates/nanomsg');
+    assert.dom('[data-test-404-page]').exists();
+    assert.dom('[data-test-title]').hasText('Crate not found');
+    assert.dom('[data-test-go-back]').exists();
+    assert.dom('[data-test-try-again]').doesNotExist();
   });
 
   test('other crate loading error shows an error message', async function (assert) {
     this.server.get('/api/v1/crates/:crate_name', {}, 500);
 
     await visit('/crates/nanomsg');
-    assert.equal(currentURL(), '/');
-    assert
-      .dom('[data-test-notification-message]')
-      .hasText("Loading data for the 'nanomsg' crate failed. Please try again later!");
+    assert.equal(currentURL(), '/crates/nanomsg');
+    assert.dom('[data-test-404-page]').exists();
+    assert.dom('[data-test-title]').hasText('Crate failed to load');
+    assert.dom('[data-test-go-back]').doesNotExist();
+    assert.dom('[data-test-try-again]').exists();
   });
 
   test('unknown versions fall back to latest version and show an error message', async function (assert) {
@@ -104,10 +108,11 @@ module('Acceptance | crate page', function (hooks) {
 
     await visit('/crates/nanomsg/0.7.0');
 
-    assert.equal(currentURL(), '/crates/nanomsg');
-    assert.dom('[data-test-heading] [data-test-crate-name]').hasText('nanomsg');
-    assert.dom('[data-test-heading] [data-test-crate-version]').hasText('0.6.1');
-    assert.dom('[data-test-notification-message]').hasText("Version '0.7.0' of crate 'nanomsg' does not exist");
+    assert.equal(currentURL(), '/crates/nanomsg/0.7.0');
+    assert.dom('[data-test-404-page]').exists();
+    assert.dom('[data-test-title]').hasText('Version not found');
+    assert.dom('[data-test-go-back]').exists();
+    assert.dom('[data-test-try-again]').doesNotExist();
   });
 
   test('other versions loading error shows an error message', async function (assert) {
@@ -119,10 +124,11 @@ module('Acceptance | crate page', function (hooks) {
 
     await visit('/');
     await click('[data-test-just-updated] [data-test-crate-link="0"]');
-    assert.equal(currentURL(), '/');
-    assert
-      .dom('[data-test-notification-message]')
-      .hasText("Loading data for the 'nanomsg' crate failed. Please try again later!");
+    assert.equal(currentURL(), '/crates/nanomsg');
+    assert.dom('[data-test-404-page]').exists();
+    assert.dom('[data-test-title]').hasText('Crate failed to load');
+    assert.dom('[data-test-go-back]').doesNotExist();
+    assert.dom('[data-test-try-again]').exists();
   });
 
   test('navigating to the all versions page', async function (assert) {

tests/routes/crate/version/model-test.js

@@ -1,8 +1,10 @@
-import { currentURL, visit } from '@ember/test-helpers';
+import { currentURL } from '@ember/test-helpers';
 import { module, test } from 'qunit';
 
 import { setupApplicationTest } from 'cargo/tests/helpers';
 
+import { visit } from '../../../helpers/visit-ignoring-abort';
+
 module('Route | crate.version | model() hook', function (hooks) {
   setupApplicationTest(hooks);
 
@@ -20,17 +22,18 @@ module('Route | crate.version | model() hook', function (hooks) {
       assert.dom('[data-test-notification-message]').doesNotExist();
     });
 
-    test('redirects to unspecific version URL', async function (assert) {
+    test('shows error page for unknown versions', async function (assert) {
       let crate = this.server.create('crate', { name: 'foo' });
       this.server.create('version', { crate, num: '1.0.0' });
       this.server.create('version', { crate, num: '1.2.3', yanked: true });
       this.server.create('version', { crate, num: '2.0.0-beta.1' });
 
       await visit('/crates/foo/2.0.0');
-      assert.equal(currentURL(), `/crates/foo`);
-      assert.dom('[data-test-crate-name]').hasText('foo');
-      assert.dom('[data-test-crate-version]').hasText('1.0.0');
-      assert.dom('[data-test-notification-message="error"]').hasText("Version '2.0.0' of crate 'foo' does not exist");
+      assert.equal(currentURL(), `/crates/foo/2.0.0`);
+      assert.dom('[data-test-404-page]').exists();
+      assert.dom('[data-test-title]').hasText('Version not found');
+      assert.dom('[data-test-go-back]').exists();
+      assert.dom('[data-test-try-again]').doesNotExist();
     });
   });