Auto merge of #3175 - Turbo87:default-version, r=pichfl

Simplify `crate.version` route implementation

This PR adds a `defaultVersion` property on the `crate` model and uses it in the `crate.version` (and `crate.index`) route to determine which version to display by default. This lets us get rid of the `semver` dependency in that route and puts the backend in charge of telling us what to display by default. This will allow us to address #654 in the near future, since the new `defaultVersion` property can be used in the search results too.

This is probably best reviewed commit-by-commit :)

r? `@pichfl`

Author: bors

app/models/crate.js

@@ -9,6 +9,7 @@ export default class Crate extends Model {
   @attr('date') created_at;
   @attr('date') updated_at;
   @attr max_version;
+  @attr max_stable_version;
   @attr newest_version;
 
   @attr description;
@@ -30,6 +31,21 @@ export default class Crate extends Model {
   @hasMany('categories', { async: true }) categories;
   @hasMany('dependency', { async: true }) reverse_dependencies;
 
+  /**
+   * This is the default version that will be shown when visiting the crate
+   * details page. Note that this can be `undefined` if all versions of the crate
+   * have been yanked.
+   * @return {string}
+   */
+  get defaultVersion() {
+    if (this.max_stable_version) {
+      return this.max_stable_version;
+    }
+    if (this.max_version && this.max_version !== '0.0.0') {
+      return this.max_version;
+    }
+  }
+
   follow = memberAction({ type: 'PUT', path: 'follow' });
   unfollow = memberAction({ type: 'DELETE', path: 'follow' });
 

app/routes/crate/version.js

@@ -2,63 +2,30 @@ import Route from '@ember/routing/route';
 import { inject as service } from '@ember/service';
 
 import * as Sentry from '@sentry/browser';
-import prerelease from 'semver/functions/prerelease';
 
 import { AjaxError } from '../../utils/ajax';
 
-function isUnstableVersion(version) {
-  return !!prerelease(version);
-}
-
 export default class VersionRoute extends Route {
   @service notifications;
 
   async model(params) {
-    const requestedVersion = params.version_num;
-    const crate = this.modelFor('crate');
-    const maxVersion = crate.max_version;
-
+    let crate = this.modelFor('crate');
     let versions = await crate.get('versions');
 
-    // Fallback to the crate's last stable version
-    // If `max_version` is `0.0.0` then all versions have been yanked
-    if (!params.version_num && maxVersion !== '0.0.0') {
-      if (isUnstableVersion(maxVersion)) {
-        // Find the latest version that is stable AND not-yanked.
-        const latestStableVersion = versions.find(version => !isUnstableVersion(version.num) && !version.yanked);
-
-        if (latestStableVersion == null) {
-          // Cannot find any version that is stable AND not-yanked.
-          // The fact that "maxVersion" itself cannot be found means that
-          // we have to fall back to the latest one that is unstable....
-
-          // Find the latest version that not yanked.
-          const latestUnyankedVersion = versions.find(version => !version.yanked);
-
-          if (latestUnyankedVersion == null) {
-            // There's not even any unyanked version...
-            params.version_num = maxVersion;
-          } else {
-            params.version_num = latestUnyankedVersion.num;
-          }
-        } else {
-          params.version_num = latestStableVersion.num;
-        }
-      } else {
-        params.version_num = maxVersion;
+    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');
       }
+    } else {
+      let { defaultVersion } = crate;
+      version = versions.find(version => version.num === defaultVersion) ?? versions.lastObject;
     }
 
-    const version = versions.find(version => version.num === params.version_num);
-    if (params.version_num && !version) {
-      this.notifications.error(`Version '${params.version_num}' of crate '${crate.name}' does not exist`);
-    }
-
-    return {
-      crate,
-      requestedVersion,
-      version: version || versions.find(version => version.num === maxVersion) || versions.objectAt(0),
-    };
+    return { crate, requestedVersion, version };
   }
 
   setupController(controller, model) {

mirage/serializers/crate.js

@@ -53,14 +53,15 @@ export default BaseSerializer.extend({
   _adjust(hash) {
     let versions = this.schema.versions.where({ crateId: hash.id });
     assert(`crate \`${hash.id}\` has no associated versions`, versions.length !== 0);
+    versions = versions.filter(it => !it.yanked);
 
     let versionNums = versions.models.map(it => it.num);
     semverSort(versionNums);
-    hash.max_version = versionNums[0];
+    hash.max_version = versionNums[0] ?? '0.0.0';
     hash.max_stable_version = versionNums.find(it => !prerelease(it)) ?? null;
 
-    let newestVersions = versions.sort((a, b) => compareIsoDates(b.updated_at, a.updated_at));
-    hash.newest_version = newestVersions.models[0].num;
+    let newestVersions = versions.models.sort((a, b) => compareIsoDates(b.updated_at, a.updated_at));
+    hash.newest_version = newestVersions[0]?.num ?? '0.0.0';
 
     hash.categories = hash.category_ids;
     delete hash.category_ids;

tests/acceptance/crate-test.js

@@ -88,7 +88,7 @@ module('Acceptance | crate page', function (hooks) {
 
     await visit('/crates/nanomsg/0.7.0');
 
-    assert.equal(currentURL(), '/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");

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

@@ -0,0 +1,95 @@
+import { currentURL, visit } from '@ember/test-helpers';
+import { setupApplicationTest } from 'ember-qunit';
+import { module, test } from 'qunit';
+
+import setupMirage from '../../../helpers/setup-mirage';
+
+module('Route | crate.version | model() hook', function (hooks) {
+  setupApplicationTest(hooks);
+  setupMirage(hooks);
+
+  module('with explicit version number in the URL', function () {
+    test('shows yanked 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/1.2.3');
+      assert.equal(currentURL(), `/crates/foo/1.2.3`);
+      assert.dom('[data-test-crate-name]').hasText('foo');
+      assert.dom('[data-test-crate-version]').hasText('1.2.3');
+      assert.dom('[data-test-notification-message]').doesNotExist();
+    });
+
+    test('redirects to unspecific version URL', 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");
+    });
+  });
+
+  module('without version number in the URL', function () {
+    test('defaults to the highest stable version', 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' });
+      this.server.create('version', { crate, num: '2.0.0' });
+
+      await visit('/crates/foo');
+      assert.equal(currentURL(), `/crates/foo`);
+      assert.dom('[data-test-crate-name]').hasText('foo');
+      assert.dom('[data-test-crate-version]').hasText('2.0.0');
+      assert.dom('[data-test-notification-message]').doesNotExist();
+    });
+
+    test('defaults to the highest stable version, even if there are higher prereleases', 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');
+      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]').doesNotExist();
+    });
+
+    test('defaults to the highest not-yanked version', async function (assert) {
+      let crate = this.server.create('crate', { name: 'foo' });
+      this.server.create('version', { crate, num: '1.0.0', yanked: true });
+      this.server.create('version', { crate, num: '1.2.3', yanked: true });
+      this.server.create('version', { crate, num: '2.0.0-beta.1' });
+      this.server.create('version', { crate, num: '2.0.0-beta.2' });
+      this.server.create('version', { crate, num: '2.0.0', yanked: true });
+
+      await visit('/crates/foo');
+      assert.equal(currentURL(), `/crates/foo`);
+      assert.dom('[data-test-crate-name]').hasText('foo');
+      assert.dom('[data-test-crate-version]').hasText('2.0.0-beta.2');
+      assert.dom('[data-test-notification-message]').doesNotExist();
+    });
+
+    test('if there are only yanked versions, it defaults to the latest version', async function (assert) {
+      let crate = this.server.create('crate', { name: 'foo' });
+      this.server.create('version', { crate, num: '1.0.0', yanked: true });
+      this.server.create('version', { crate, num: '1.2.3', yanked: true });
+      this.server.create('version', { crate, num: '2.0.0-beta.1', yanked: true });
+
+      await visit('/crates/foo');
+      assert.equal(currentURL(), `/crates/foo`);
+      assert.dom('[data-test-crate-name]').hasText('foo');
+      assert.dom('[data-test-crate-version]').hasText('2.0.0-beta.1');
+      assert.dom('[data-test-notification-message]').doesNotExist();
+    });
+  });
+});