From aafc6ee4d14bfc918ec92cfcf8df41173289f021 Mon Sep 17 00:00:00 2001 From: Tobias Bieniek Date: Mon, 9 Dec 2024 17:06:39 +0100 Subject: [PATCH 1/2] adapters/application: Disable special case handling of 422 errors The crate deletion endpoint is our first one that uses "422 Unprocessable Entity" HTTP errors, but it looks like Ember Data is expecting a slightly different layout for them. Since we don't need the special handling code in Ember Data, we can disable the extra code path and treat them like any other 4xx error. --- app/adapters/application.js | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/app/adapters/application.js b/app/adapters/application.js index 508db496b4..5798b0dd38 100644 --- a/app/adapters/application.js +++ b/app/adapters/application.js @@ -3,6 +3,12 @@ import RESTAdapter from '@ember-data/adapter/rest'; export default class ApplicationAdapter extends RESTAdapter { namespace = 'api/v1'; + isInvalid() { + // HTTP 422 errors are causing all sorts of issues within Ember Data, + // so we disable their special case handling here, since we don't need/want it. + return false; + } + handleResponse(status, headers, payload, requestData) { if (typeof payload === 'string') { try { From 796d3f782bb4f51665c02122591c0e5c04e7f552 Mon Sep 17 00:00:00 2001 From: Tobias Bieniek Date: Tue, 10 Dec 2024 14:42:43 +0100 Subject: [PATCH 2/2] UI: Implement `/crates/:name/delete` route --- app/controllers/crate/delete.js | 32 ++++++++ app/router.js | 1 + app/routes/crate/delete.js | 25 ++++++ app/styles/application.module.css | 2 + app/styles/crate/delete.module.css | 86 ++++++++++++++++++++ app/templates/crate/delete.hbs | 64 +++++++++++++++ e2e/routes/crate/delete.spec.ts | 100 +++++++++++++++++++++++ public/assets/triangle-exclamation.svg | 4 + tests/routes/crate/delete-test.js | 108 +++++++++++++++++++++++++ 9 files changed, 422 insertions(+) create mode 100644 app/controllers/crate/delete.js create mode 100644 app/routes/crate/delete.js create mode 100644 app/styles/crate/delete.module.css create mode 100644 app/templates/crate/delete.hbs create mode 100644 e2e/routes/crate/delete.spec.ts create mode 100644 public/assets/triangle-exclamation.svg create mode 100644 tests/routes/crate/delete-test.js diff --git a/app/controllers/crate/delete.js b/app/controllers/crate/delete.js new file mode 100644 index 0000000000..866acfa277 --- /dev/null +++ b/app/controllers/crate/delete.js @@ -0,0 +1,32 @@ +import Controller from '@ember/controller'; +import { action } from '@ember/object'; +import { inject as service } from '@ember/service'; +import { tracked } from '@glimmer/tracking'; + +import { task } from 'ember-concurrency'; + +export default class CrateSettingsController extends Controller { + @service notifications; + @service router; + + @tracked isConfirmed; + + @action toggleConfirmation() { + this.isConfirmed = !this.isConfirmed; + } + + deleteTask = task(async () => { + try { + await this.model.destroyRecord(); + this.notifications.success(`Crate ${this.model.name} has been successfully deleted.`); + this.router.transitionTo('index'); + } catch (error) { + let detail = error.errors?.[0]?.detail; + if (detail && !detail.startsWith('{')) { + this.notifications.error(`Failed to delete crate: ${detail}`); + } else { + this.notifications.error('Failed to delete crate'); + } + } + }); +} diff --git a/app/router.js b/app/router.js index e0de0c9c4b..f6bc3248d1 100644 --- a/app/router.js +++ b/app/router.js @@ -20,6 +20,7 @@ Router.map(function () { this.route('owners'); this.route('settings'); + this.route('delete'); // Well-known routes this.route('docs'); diff --git a/app/routes/crate/delete.js b/app/routes/crate/delete.js new file mode 100644 index 0000000000..5a19c5c9cd --- /dev/null +++ b/app/routes/crate/delete.js @@ -0,0 +1,25 @@ +import { inject as service } from '@ember/service'; + +import AuthenticatedRoute from '../-authenticated-route'; + +export default class SettingsRoute extends AuthenticatedRoute { + @service router; + @service session; + + async afterModel(crate, transition) { + let user = this.session.currentUser; + let owners = await crate.owner_user; + let isOwner = owners.some(owner => owner.id === user.id); + if (!isOwner) { + this.router.replaceWith('catch-all', { + transition, + title: 'This page is only accessible by crate owners', + }); + } + } + + setupController(controller) { + super.setupController(...arguments); + controller.set('isConfirmed', false); + } +} diff --git a/app/styles/application.module.css b/app/styles/application.module.css index 3a8d4b583d..d072b62368 100644 --- a/app/styles/application.module.css +++ b/app/styles/application.module.css @@ -18,8 +18,10 @@ --orange-800: #9a3412; --orange-900: #7c2d12; + --yellow100: hsl(44, 100%, 90%); --yellow500: hsl(44, 100%, 60%); --yellow700: hsl(44, 67%, 50%); + --yellow800: hsl(44, 67%, 20%); --header-bg-color: light-dark(hsl(115, 31%, 20%), #141413); diff --git a/app/styles/crate/delete.module.css b/app/styles/crate/delete.module.css new file mode 100644 index 0000000000..ec626385c4 --- /dev/null +++ b/app/styles/crate/delete.module.css @@ -0,0 +1,86 @@ +.wrapper { + display: grid; + grid-template-columns: minmax(0, 1fr); + place-items: center; + margin: var(--space-s); +} + +.content { + max-width: 100%; + overflow-wrap: break-word; +} + +.title { + margin-top: 0; +} + +.warning-block { + background: light-dark(var(--yellow100), var(--yellow800)); + border-color: var(--yellow500); + border-left-style: solid; + border-left-width: 4px; + border-top-right-radius: var(--space-3xs); + border-bottom-right-radius: var(--space-3xs); + padding: var(--space-xs); +} + +.warning { + composes: warning-block; + display: flex; + + svg { + flex-shrink: 0; + width: 1em; + height: 1em; + color: var(--yellow500); + } + + p { + margin: 0 0 0 var(--space-xs); + text-wrap: pretty; + } +} + +.impact, .requirements { + li { + margin-bottom: var(--space-2xs); + } +} + +.requirements { + ul { + list-style: none; + padding-left: 0; + } +} + +.confirmation { + composes: warning-block; + display: block; + + input { + margin-right: var(--space-3xs); + } +} + +.actions { + margin-top: var(--space-m); + display: flex; + justify-content: center; + align-items: center; +} + +.delete-button { + composes: red-button from '../shared/buttons.module.css'; +} + +.spinner-wrapper { + position: relative; +} + +.spinner { + position: absolute; + --spinner-size: 1.5em; + top: calc(-.5 * var(--spinner-size)); + margin-left: var(--space-xs); +} diff --git a/app/templates/crate/delete.hbs b/app/templates/crate/delete.hbs new file mode 100644 index 0000000000..3cc114e2f3 --- /dev/null +++ b/app/templates/crate/delete.hbs @@ -0,0 +1,64 @@ +
+
+

Delete the {{@model.name}} crate?

+ +

Are you sure you want to delete the crate "{{@model.name}}"?

+ +
+ {{svg-jar "triangle-exclamation"}} +

Important: This action will permanently delete the crate and its associated versions. Deleting a crate cannot be reversed!

+
+ +
+

Potential Impact:

+
    +
  • Users will no longer be able to download this crate.
    • +
  • Any dependencies or projects relying on this crate will be broken.
    • +
  • Deleted crates cannot be restored.
    • +
+
+ +
+

Requirements:

+

A crate can only be deleted if:

+
    +
  1. the crate has been published for less than 72 hours, or
    2. +
  2. +
      +
    • the crate only has a single owner, and
      • +
    • the crate has been downloaded less than 100 times for each month it has been published, and
      • +
    • the crate is not depended upon by any other crate on crates.io.
      • +
    +
    3. +
+
+ + + +
+ + {{#if this.deleteTask.isRunning}} +
+ +
+ {{/if}} +
+
+
\ No newline at end of file diff --git a/e2e/routes/crate/delete.spec.ts b/e2e/routes/crate/delete.spec.ts new file mode 100644 index 0000000000..4e67ce4a60 --- /dev/null +++ b/e2e/routes/crate/delete.spec.ts @@ -0,0 +1,100 @@ +import { expect, test } from '@/e2e/helper'; + +test.describe('Route: crate.delete', { tag: '@routes' }, () => { + async function prepare({ mirage }) { + await mirage.addHook(server => { + let user = server.create('user'); + + let crate = server.create('crate', { name: 'foo' }); + server.create('version', { crate }); + server.create('crate-ownership', { crate, user }); + + authenticateAs(user); + }); + } + + test('unauthenticated', async ({ mirage, page }) => { + await mirage.addHook(server => { + let crate = server.create('crate', { name: 'foo' }); + server.create('version', { crate }); + }); + + await page.goto('/crates/foo/delete'); + await expect(page).toHaveURL('/crates/foo/delete'); + await expect(page.locator('[data-test-title]')).toHaveText('This page requires authentication'); + await expect(page.locator('[data-test-login]')).toBeVisible(); + }); + + test('not an owner', async ({ mirage, page }) => { + await mirage.addHook(server => { + let user1 = server.create('user'); + authenticateAs(user1); + + let user2 = server.create('user'); + let crate = server.create('crate', { name: 'foo' }); + server.create('version', { crate }); + server.create('crate-ownership', { crate, user: user2 }); + }); + + await page.goto('/crates/foo/delete'); + await expect(page).toHaveURL('/crates/foo/delete'); + await expect(page.locator('[data-test-title]')).toHaveText('This page is only accessible by crate owners'); + await expect(page.locator('[data-test-go-back]')).toBeVisible(); + }); + + test('happy path', async ({ mirage, page, percy }) => { + await prepare({ mirage }); + + await page.goto('/crates/foo/delete'); + await expect(page).toHaveURL('/crates/foo/delete'); + await expect(page.locator('[data-test-title]')).toHaveText('Delete the foo crate?'); + await percy.snapshot(); + + await expect(page.locator('[data-test-delete-button]')).toBeDisabled(); + await page.click('[data-test-confirmation-checkbox]'); + await expect(page.locator('[data-test-delete-button]')).toBeEnabled(); + await page.click('[data-test-delete-button]'); + + await expect(page).toHaveURL('/'); + + let message = 'Crate foo has been successfully deleted.'; + await expect(page.locator('[data-test-notification-message="success"]')).toHaveText(message); + + let crate = await page.evaluate(() => server.schema.crates.findBy({ name: 'foo' })); + expect(crate).toBeNull(); + }); + + test('loading state', async ({ page, mirage }) => { + await prepare({ mirage }); + await mirage.addHook(server => { + globalThis.deferred = require('rsvp').defer(); + server.delete('/api/v1/crates/foo', () => globalThis.deferred.promise); + }); + + await page.goto('/crates/foo/delete'); + await page.click('[data-test-confirmation-checkbox]'); + await page.click('[data-test-delete-button]'); + await expect(page.locator('[data-test-spinner]')).toBeVisible(); + await expect(page.locator('[data-test-confirmation-checkbox]')).toBeDisabled(); + await expect(page.locator('[data-test-delete-button]')).toBeDisabled(); + + await page.evaluate(async () => globalThis.deferred.resolve()); + await expect(page).toHaveURL('/'); + }); + + test('error state', async ({ page, mirage }) => { + await prepare({ mirage }); + await mirage.addHook(server => { + let payload = { errors: [{ detail: 'only crates without reverse dependencies can be deleted after 72 hours' }] }; + server.delete('/api/v1/crates/foo', payload, 422); + }); + + await page.goto('/crates/foo/delete'); + await page.click('[data-test-confirmation-checkbox]'); + await page.click('[data-test-delete-button]'); + await expect(page).toHaveURL('/crates/foo/delete'); + + let message = 'Failed to delete crate: only crates without reverse dependencies can be deleted after 72 hours'; + await expect(page.locator('[data-test-notification-message="error"]')).toHaveText(message); + }); +}); diff --git a/public/assets/triangle-exclamation.svg b/public/assets/triangle-exclamation.svg new file mode 100644 index 0000000000..d234873290 --- /dev/null +++ b/public/assets/triangle-exclamation.svg @@ -0,0 +1,4 @@ + + + + diff --git a/tests/routes/crate/delete-test.js b/tests/routes/crate/delete-test.js new file mode 100644 index 0000000000..2cd33a67bb --- /dev/null +++ b/tests/routes/crate/delete-test.js @@ -0,0 +1,108 @@ +import { click, currentURL, waitFor } from '@ember/test-helpers'; +import { module, test } from 'qunit'; + +import { defer } from 'rsvp'; + +import percySnapshot from '@percy/ember'; +import { Response } from 'miragejs'; + +import { setupApplicationTest } from 'crates-io/tests/helpers'; + +import { visit } from '../../helpers/visit-ignoring-abort'; + +module('Route: crate.delete', function (hooks) { + setupApplicationTest(hooks); + + function prepare(context) { + let user = context.server.create('user'); + + let crate = context.server.create('crate', { name: 'foo' }); + context.server.create('version', { crate }); + context.server.create('crate-ownership', { crate, user }); + + context.authenticateAs(user); + + return { user }; + } + + test('unauthenticated', async function (assert) { + let crate = this.server.create('crate', { name: 'foo' }); + this.server.create('version', { crate }); + + await visit('/crates/foo/delete'); + assert.strictEqual(currentURL(), '/crates/foo/delete'); + assert.dom('[data-test-title]').hasText('This page requires authentication'); + assert.dom('[data-test-login]').exists(); + }); + + test('not an owner', async function (assert) { + let user1 = this.server.create('user'); + this.authenticateAs(user1); + + let user2 = this.server.create('user'); + let crate = this.server.create('crate', { name: 'foo' }); + this.server.create('version', { crate }); + this.server.create('crate-ownership', { crate, user: user2 }); + + await visit('/crates/foo/delete'); + assert.strictEqual(currentURL(), '/crates/foo/delete'); + assert.dom('[data-test-title]').hasText('This page is only accessible by crate owners'); + assert.dom('[data-test-go-back]').exists(); + }); + + test('happy path', async function (assert) { + prepare(this); + + await visit('/crates/foo/delete'); + assert.strictEqual(currentURL(), '/crates/foo/delete'); + assert.dom('[data-test-title]').hasText('Delete the foo crate?'); + await percySnapshot(assert); + + assert.dom('[data-test-delete-button]').isDisabled(); + await click('[data-test-confirmation-checkbox]'); + assert.dom('[data-test-delete-button]').isEnabled(); + await click('[data-test-delete-button]'); + + assert.strictEqual(currentURL(), '/'); + + let message = 'Crate foo has been successfully deleted.'; + assert.dom('[data-test-notification-message="success"]').hasText(message); + + let crate = this.server.schema.crates.findBy({ name: 'foo' }); + assert.strictEqual(crate, null); + }); + + test('loading state', async function (assert) { + prepare(this); + + let deferred = defer(); + this.server.delete('/api/v1/crates/foo', deferred.promise); + + await visit('/crates/foo/delete'); + await click('[data-test-confirmation-checkbox]'); + let clickPromise = click('[data-test-delete-button]'); + await waitFor('[data-test-spinner]'); + assert.dom('[data-test-confirmation-checkbox]').isDisabled(); + assert.dom('[data-test-delete-button]').isDisabled(); + + deferred.resolve(new Response(204)); + await clickPromise; + + assert.strictEqual(currentURL(), '/'); + }); + + test('error state', async function (assert) { + prepare(this); + + let payload = { errors: [{ detail: 'only crates without reverse dependencies can be deleted after 72 hours' }] }; + this.server.delete('/api/v1/crates/foo', payload, 422); + + await visit('/crates/foo/delete'); + await click('[data-test-confirmation-checkbox]'); + await click('[data-test-delete-button]'); + assert.strictEqual(currentURL(), '/crates/foo/delete'); + + let message = 'Failed to delete crate: only crates without reverse dependencies can be deleted after 72 hours'; + assert.dom('[data-test-notification-message="error"]').hasText(message); + }); +});