Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

crate/delete: Add required reason input field #10380

Merged
merged 3 commits into from
Jan 14, 2025
Merged

crate/delete: Add required reason input field #10380

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f1b1b7a
crate/delete: Use proper `form` element for `submit` button
Turbo87 Jan 13, 2025
2807030
adapters/crate: Implement `urlForDeleteRecord()` hook
Turbo87 Jan 13, 2025
8719a86
crate/delete: Add required reason input field
Turbo87 Jan 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions app/adapters/crate.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,18 @@ export default class CrateAdapter extends ApplicationAdapter {
return `${baseUrl}/${crateName}`;
}

/** Adds a `reason` query parameter to the URL, if set in the `adapterOptions`. */
urlForDeleteRecord(id, modelName, snapshot) {
let url = super.urlForDeleteRecord(...arguments);

let reason = snapshot.adapterOptions.reason;
if (reason) {
url += `?reason=${encodeURIComponent(reason)}`;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Turbo87 🙋 If I understand correctly, the query string is named message at the backend

crates.io/src/controllers/krate/delete.rs

Lines 24 to 29 in 19c80e0

#[derive(Debug, Deserialize, FromRequestParts, utoipa::IntoParams)]
#[from_request(via(Query), rejection(QueryRejection))]
#[into_params(parameter_in = Query)]
pub struct DeleteQueryParams {
message: Option<String>,
}
, but here we are sending it as reason. I think this might be a mistake?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

damn, you're right... 🤦‍♂️

}

return url;
}

groupRecordsForFindMany(store, snapshots) {
let result = [];
for (let i = 0; i < snapshots.length; i += BULK_REQUEST_GROUP_SIZE) {
Expand Down
5 changes: 4 additions & 1 deletion app/controllers/crate/delete.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,15 +9,18 @@ export default class CrateSettingsController extends Controller {
@service notifications;
@service router;

@tracked reason = '';
@tracked isConfirmed;

@action toggleConfirmation() {
this.isConfirmed = !this.isConfirmed;
}

deleteTask = task(async () => {
let { reason } = this;

try {
await this.model.destroyRecord();
await this.model.destroyRecord({ adapterOptions: { reason } });
this.notifications.success(`Crate ${this.model.name} has been successfully deleted.`);
this.router.transitionTo('index');
} catch (error) {
Expand Down
1 change: 1 addition & 0 deletions app/routes/crate/delete.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ export default class SettingsRoute extends AuthenticatedRoute {

setupController(controller) {
super.setupController(...arguments);
controller.set('reason', '');
controller.set('isConfirmed', false);
}
}
9 changes: 9 additions & 0 deletions app/styles/crate/delete.module.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,15 @@
}
}

.reason {
margin-bottom: var(--space-m);
}

.reason-input {
composes: base-input from '../../styles/settings/tokens/new.module.css';
width: 100%;
}

.confirmation {
composes: warning-block;
display: block;
Expand Down
19 changes: 16 additions & 3 deletions app/templates/crate/delete.hbs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
<div local-class="wrapper">
<div local-class="content">
<form local-class="content" {{on "submit" (prevent-default (perform this.deleteTask))}}>
<h1 local-class="title" data-test-title>Delete the {{@model.name}} crate?</h1>

<p>Are you sure you want to delete the crate "{{@model.name}}"?</p>
Expand Down Expand Up @@ -36,6 +36,20 @@
</ol>
</div>

<div local-class="reason">
<h3>Reason:</h3>
<label>
<p>Please tell us why you are deleting this crate:</p>
<Input
@type="text"
@value={{this.reason}}
required={{true}}
local-class="reason-input"
data-test-reason
/>
</label>
</div>

<label local-class="confirmation">
<Input
@type="checkbox"
Expand All @@ -53,7 +67,6 @@
disabled={{or (not this.isConfirmed) this.deleteTask.isRunning}}
local-class="delete-button"
data-test-delete-button
{{on "click" (perform this.deleteTask)}}
>
Delete this crate
</button>
Expand All @@ -63,5 +76,5 @@
</div>
{{/if}}
</div>
</div>
</form>
</div>
1 change: 1 addition & 0 deletions e2e/acceptance/crate-deletion.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ test.describe('Acceptance | crate deletion', { tag: '@acceptance' }, () => {
await expect(page.locator('[data-test-title]')).toHaveText('Delete the foo crate?');
await expect(page.locator('[data-test-delete-button]')).toBeDisabled();

await page.fill('[data-test-reason]', "I don't need this crate anymore");
await page.click('[data-test-confirmation-checkbox]');
await expect(page.locator('[data-test-delete-button]')).toBeEnabled();

Expand Down
3 changes: 3 additions & 0 deletions e2e/routes/crate/delete.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ test.describe('Route: crate.delete', { tag: '@routes' }, () => {
await expect(page.locator('[data-test-title]')).toHaveText('Delete the foo crate?');
await percy.snapshot();

await page.fill('[data-test-reason]', "I don't need this crate anymore");
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();
Expand All @@ -72,6 +73,7 @@ test.describe('Route: crate.delete', { tag: '@routes' }, () => {
});

await page.goto('/crates/foo/delete');
await page.fill('[data-test-reason]', "I don't need this crate anymore");
await page.click('[data-test-confirmation-checkbox]');
await page.click('[data-test-delete-button]');
await expect(page.locator('[data-test-spinner]')).toBeVisible();
Expand All @@ -90,6 +92,7 @@ test.describe('Route: crate.delete', { tag: '@routes' }, () => {
});

await page.goto('/crates/foo/delete');
await page.fill('[data-test-reason]', "I don't need this crate anymore");
await page.click('[data-test-confirmation-checkbox]');
await page.click('[data-test-delete-button]');
await expect(page).toHaveURL('/crates/foo/delete');
Expand Down
3 changes: 2 additions & 1 deletion tests/acceptance/crate-deletion-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { click, currentURL } from '@ember/test-helpers';
import { click, currentURL, fillIn } from '@ember/test-helpers';
import { module, test } from 'qunit';

import { setupApplicationTest } from 'crates-io/tests/helpers';
Expand Down Expand Up @@ -29,6 +29,7 @@ module('Acceptance | crate deletion', function (hooks) {
assert.dom('[data-test-title]').hasText('Delete the foo crate?');
assert.dom('[data-test-delete-button]').isDisabled();

await fillIn('[data-test-reason]', "I don't need this crate anymore");
await click('[data-test-confirmation-checkbox]');
assert.dom('[data-test-delete-button]').isEnabled();

Expand Down
5 changes: 4 additions & 1 deletion tests/routes/crate/delete-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { click, currentURL, waitFor } from '@ember/test-helpers';
import { click, currentURL, fillIn, waitFor } from '@ember/test-helpers';
import { module, test } from 'qunit';

import { defer } from 'rsvp';
Expand Down Expand Up @@ -58,6 +58,7 @@ module('Route: crate.delete', function (hooks) {
assert.dom('[data-test-title]').hasText('Delete the foo crate?');
await percySnapshot(assert);

await fillIn('[data-test-reason]', "I don't need this crate anymore");
assert.dom('[data-test-delete-button]').isDisabled();
await click('[data-test-confirmation-checkbox]');
assert.dom('[data-test-delete-button]').isEnabled();
Expand All @@ -79,6 +80,7 @@ module('Route: crate.delete', function (hooks) {
this.server.delete('/api/v1/crates/foo', deferred.promise);

await visit('/crates/foo/delete');
await fillIn('[data-test-reason]', "I don't need this crate anymore");
await click('[data-test-confirmation-checkbox]');
let clickPromise = click('[data-test-delete-button]');
await waitFor('[data-test-spinner]');
Expand All @@ -98,6 +100,7 @@ module('Route: crate.delete', function (hooks) {
this.server.delete('/api/v1/crates/foo', payload, 422);

await visit('/crates/foo/delete');
await fillIn('[data-test-reason]', "I don't need this crate anymore");
await click('[data-test-confirmation-checkbox]');
await click('[data-test-delete-button]');
assert.strictEqual(currentURL(), '/crates/foo/delete');
Expand Down
Loading