fix(#248): fix 3 merge zone bugs — stale state, wrong placeholder, missing success feedback
- TagMergeZone: add $effect to reset targetId when tag prop changes (fixes stale form after navigation)
- TagMergeZone: pass merge-specific placeholder to TagParentPicker
- TagMergeZone: show success banner on form.mergeSuccess and goto() target tag
- +page.server.ts: merge action returns { mergeSuccess, mergeTargetId } instead of redirect
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
@@ -47,7 +47,7 @@ export const actions: Actions = {
|
|||||||
return fail(result.response.status, { error: getErrorMessage(code) });
|
return fail(result.response.status, { error: getErrorMessage(code) });
|
||||||
}
|
}
|
||||||
|
|
||||||
throw redirect(303, `/admin/tags/${result.data!.id}`);
|
return { mergeSuccess: true, mergeTargetId: result.data!.id };
|
||||||
},
|
},
|
||||||
|
|
||||||
delete: async ({ params, request, fetch }) => {
|
delete: async ({ params, request, fetch }) => {
|
||||||
|
|||||||
@@ -1,4 +1,5 @@
|
|||||||
<script lang="ts">
|
<script lang="ts">
|
||||||
|
import { goto } from '$app/navigation';
|
||||||
import { enhance } from '$app/forms';
|
import { enhance } from '$app/forms';
|
||||||
import { m } from '$lib/paraglide/messages.js';
|
import { m } from '$lib/paraglide/messages.js';
|
||||||
import TagParentPicker from '$lib/components/TagParentPicker.svelte';
|
import TagParentPicker from '$lib/components/TagParentPicker.svelte';
|
||||||
@@ -14,12 +15,23 @@ type FlatTag = {
|
|||||||
interface Props {
|
interface Props {
|
||||||
tag: { id: string; name: string; documentCount: number };
|
tag: { id: string; name: string; documentCount: number };
|
||||||
allTags: FlatTag[];
|
allTags: FlatTag[];
|
||||||
form: { error?: string } | null;
|
form: { error?: string; mergeSuccess?: boolean; mergeTargetId?: string } | null;
|
||||||
}
|
}
|
||||||
|
|
||||||
let { tag, allTags, form }: Props = $props();
|
let { tag, allTags, form }: Props = $props();
|
||||||
|
|
||||||
let targetId = $state('');
|
let targetId = $state('');
|
||||||
|
|
||||||
|
$effect(() => {
|
||||||
|
void tag.id;
|
||||||
|
targetId = '';
|
||||||
|
});
|
||||||
|
|
||||||
|
$effect(() => {
|
||||||
|
if (form?.mergeSuccess && form.mergeTargetId) {
|
||||||
|
goto(`/admin/tags/${form.mergeTargetId}`);
|
||||||
|
}
|
||||||
|
});
|
||||||
const step = $derived(targetId ? 2 : 1);
|
const step = $derived(targetId ? 2 : 1);
|
||||||
|
|
||||||
// All descendants of tag.id (to exclude from picker)
|
// All descendants of tag.id (to exclude from picker)
|
||||||
@@ -60,7 +72,12 @@ const targetTag = $derived(allTags.find((t) => t.id === targetId));
|
|||||||
<label for="mergePickerTarget-search" class="mb-1 block text-xs font-medium text-ink-2">
|
<label for="mergePickerTarget-search" class="mb-1 block text-xs font-medium text-ink-2">
|
||||||
{m.admin_tag_merge_target_label()}
|
{m.admin_tag_merge_target_label()}
|
||||||
</label>
|
</label>
|
||||||
<TagParentPicker name="mergePickerTarget" bind:value={targetId} excludeIds={excludeIds} />
|
<TagParentPicker
|
||||||
|
name="mergePickerTarget"
|
||||||
|
bind:value={targetId}
|
||||||
|
excludeIds={excludeIds}
|
||||||
|
placeholder={m.admin_tag_merge_target_placeholder()}
|
||||||
|
/>
|
||||||
</div>
|
</div>
|
||||||
|
|
||||||
<!-- Step 2: confirm -->
|
<!-- Step 2: confirm -->
|
||||||
@@ -108,6 +125,12 @@ const targetTag = $derived(allTags.find((t) => t.id === targetId));
|
|||||||
</div>
|
</div>
|
||||||
{/if}
|
{/if}
|
||||||
|
|
||||||
|
{#if form?.mergeSuccess}
|
||||||
|
<p data-testid="merge-success-banner" class="mt-3 text-xs text-green-700">
|
||||||
|
{m.admin_tag_merge_success()}
|
||||||
|
</p>
|
||||||
|
{/if}
|
||||||
|
|
||||||
{#if form?.error}
|
{#if form?.error}
|
||||||
<p class="mt-3 text-xs text-red-600">{form.error}</p>
|
<p class="mt-3 text-xs text-red-600">{form.error}</p>
|
||||||
{/if}
|
{/if}
|
||||||
|
|||||||
@@ -1,11 +1,20 @@
|
|||||||
import { afterEach, describe, expect, it, vi } from 'vitest';
|
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
|
||||||
import { cleanup, render } from 'vitest-browser-svelte';
|
import { cleanup, render } from 'vitest-browser-svelte';
|
||||||
import { page } from 'vitest/browser';
|
import { page } from 'vitest/browser';
|
||||||
import TagMergeZone from './TagMergeZone.svelte';
|
import TagMergeZone from './TagMergeZone.svelte';
|
||||||
|
|
||||||
vi.mock('$app/forms', () => ({ enhance: () => () => {} }));
|
vi.mock('$app/forms', () => ({ enhance: () => () => {} }));
|
||||||
|
vi.mock('$app/navigation', () => ({ goto: vi.fn() }));
|
||||||
|
|
||||||
afterEach(cleanup);
|
beforeEach(() => {
|
||||||
|
vi.useFakeTimers();
|
||||||
|
});
|
||||||
|
|
||||||
|
afterEach(() => {
|
||||||
|
cleanup();
|
||||||
|
vi.unstubAllGlobals();
|
||||||
|
vi.useRealTimers();
|
||||||
|
});
|
||||||
|
|
||||||
const tag = { id: 't1', name: 'Familie', documentCount: 3 };
|
const tag = { id: 't1', name: 'Familie', documentCount: 3 };
|
||||||
const allTags = [
|
const allTags = [
|
||||||
@@ -14,7 +23,7 @@ const allTags = [
|
|||||||
{ id: 't3', name: 'Urlaub', documentCount: 0, parentId: 't1' }
|
{ id: 't3', name: 'Urlaub', documentCount: 0, parentId: 't1' }
|
||||||
];
|
];
|
||||||
|
|
||||||
describe('TagMergeZone', () => {
|
describe('TagMergeZone – rendering', () => {
|
||||||
it('renders the merge heading', async () => {
|
it('renders the merge heading', async () => {
|
||||||
render(TagMergeZone, { tag, allTags, form: null });
|
render(TagMergeZone, { tag, allTags, form: null });
|
||||||
await expect.element(page.getByText(/Zusammenführen/i)).toBeInTheDocument();
|
await expect.element(page.getByText(/Zusammenführen/i)).toBeInTheDocument();
|
||||||
@@ -25,35 +34,79 @@ describe('TagMergeZone', () => {
|
|||||||
await expect.element(page.getByRole('combobox')).toBeInTheDocument();
|
await expect.element(page.getByRole('combobox')).toBeInTheDocument();
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it('combobox has merge-specific placeholder text', async () => {
|
||||||
|
render(TagMergeZone, { tag, allTags, form: null });
|
||||||
|
const input = await page.getByRole('combobox').element();
|
||||||
|
expect(input.getAttribute('placeholder')).toBe('Ziel-Schlagwort suchen …');
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
|
describe('TagMergeZone – step flow', () => {
|
||||||
it('step 2 is not shown before a target is selected', async () => {
|
it('step 2 is not shown before a target is selected', async () => {
|
||||||
const { container } = render(TagMergeZone, { tag, allTags, form: null });
|
const { container } = render(TagMergeZone, { tag, allTags, form: null });
|
||||||
expect(container.querySelector('[data-testid="merge-step2"]')).toBeFalsy();
|
expect(container.querySelector('[data-testid="merge-step2"]')).toBeFalsy();
|
||||||
});
|
});
|
||||||
|
|
||||||
it('shows step 2 confirm button after target is selected', async () => {
|
it('shows step 2 confirm button after target is selected', async () => {
|
||||||
const { container } = render(TagMergeZone, { tag, allTags, form: null });
|
vi.stubGlobal(
|
||||||
|
'fetch',
|
||||||
|
vi.fn().mockResolvedValue({
|
||||||
|
ok: true,
|
||||||
|
json: vi.fn().mockResolvedValue([{ id: 't2', name: 'Reise' }])
|
||||||
|
})
|
||||||
|
);
|
||||||
|
render(TagMergeZone, { tag, allTags, form: null });
|
||||||
|
|
||||||
// Simulate target selection by directly dispatching change event
|
const input = page.getByRole('combobox');
|
||||||
const input = container.querySelector<HTMLInputElement>('input[type="text"]');
|
await input.fill('R');
|
||||||
expect(input).toBeTruthy();
|
await vi.advanceTimersByTimeAsync(300);
|
||||||
|
await page.getByRole('option', { name: 'Reise' }).click();
|
||||||
|
await vi.advanceTimersByTimeAsync(0);
|
||||||
|
|
||||||
// Simulate the hidden input being set (mimic TagParentPicker selecting 't2')
|
await expect.element(page.getByTestId('merge-step2')).toBeInTheDocument();
|
||||||
const hidden = container.querySelector<HTMLInputElement>('input[name="targetId"]');
|
});
|
||||||
if (hidden) {
|
});
|
||||||
Object.defineProperty(hidden, 'value', { value: 't2', writable: true });
|
|
||||||
hidden.dispatchEvent(new Event('change', { bubbles: true }));
|
describe('TagMergeZone – stale state reset', () => {
|
||||||
}
|
it('resets target selection when tag prop changes', async () => {
|
||||||
|
vi.stubGlobal(
|
||||||
// The merge confirm button should appear in step 2
|
'fetch',
|
||||||
// We test the component in isolation: since TagParentPicker is real,
|
vi.fn().mockResolvedValue({
|
||||||
// focus the combobox to trigger the mock and look for the step indicator
|
ok: true,
|
||||||
await expect.element(page.getByText(/Schritt 1 von 2/i)).toBeInTheDocument();
|
json: vi.fn().mockResolvedValue([{ id: 't2', name: 'Reise' }])
|
||||||
});
|
})
|
||||||
|
);
|
||||||
it('form has action ?/merge', async () => {
|
const { rerender } = render(TagMergeZone, { tag, allTags, form: null });
|
||||||
const { container } = render(TagMergeZone, { tag, allTags, form: null });
|
|
||||||
// Select a target to trigger step 2 rendering
|
const input = page.getByRole('combobox');
|
||||||
// For now just check the component renders without error
|
await input.fill('R');
|
||||||
expect(container).toBeTruthy();
|
await vi.advanceTimersByTimeAsync(300);
|
||||||
|
await page.getByRole('option', { name: 'Reise' }).click();
|
||||||
|
await vi.advanceTimersByTimeAsync(0);
|
||||||
|
|
||||||
|
await expect.element(page.getByTestId('merge-step2')).toBeInTheDocument();
|
||||||
|
|
||||||
|
// Navigate to a different tag
|
||||||
|
await rerender({ tag: { id: 't2', name: 'Reise', documentCount: 1 }, allTags, form: null });
|
||||||
|
await vi.advanceTimersByTimeAsync(0);
|
||||||
|
|
||||||
|
// step 2 should be gone — targetId was reset
|
||||||
|
expect(document.querySelector('[data-testid="merge-step2"]')).toBeFalsy();
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
|
describe('TagMergeZone – success banner', () => {
|
||||||
|
it('shows success banner when form.mergeSuccess is true', async () => {
|
||||||
|
render(TagMergeZone, {
|
||||||
|
tag,
|
||||||
|
allTags,
|
||||||
|
form: { mergeSuccess: true, mergeTargetId: 't2' }
|
||||||
|
});
|
||||||
|
await expect.element(page.getByTestId('merge-success-banner')).toBeInTheDocument();
|
||||||
|
});
|
||||||
|
|
||||||
|
it('does not show success banner when form is null', async () => {
|
||||||
|
const { container } = render(TagMergeZone, { tag, allTags, form: null });
|
||||||
|
expect(container.querySelector('[data-testid="merge-success-banner"]')).toBeFalsy();
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|||||||
@@ -53,7 +53,7 @@ describe('tags/[id] — update action', () => {
|
|||||||
// ─── merge action ─────────────────────────────────────────────────────────────
|
// ─── merge action ─────────────────────────────────────────────────────────────
|
||||||
|
|
||||||
describe('tags/[id] — merge action', () => {
|
describe('tags/[id] — merge action', () => {
|
||||||
it('redirects to target tag on successful merge', async () => {
|
it('returns mergeSuccess and mergeTargetId on successful merge', async () => {
|
||||||
mockApi.POST.mockResolvedValue({
|
mockApi.POST.mockResolvedValue({
|
||||||
response: { ok: true },
|
response: { ok: true },
|
||||||
data: { id: 't2', name: 'Reise' }
|
data: { id: 't2', name: 'Reise' }
|
||||||
@@ -62,19 +62,13 @@ describe('tags/[id] — merge action', () => {
|
|||||||
const formData = new FormData();
|
const formData = new FormData();
|
||||||
formData.set('targetId', 't2');
|
formData.set('targetId', 't2');
|
||||||
|
|
||||||
let redirectUrl: string | null = null;
|
const result = await actions.merge({
|
||||||
try {
|
params: { id: 't1' },
|
||||||
await actions.merge({
|
request: { formData: async () => formData },
|
||||||
params: { id: 't1' },
|
fetch
|
||||||
request: { formData: async () => formData },
|
} as never);
|
||||||
fetch
|
|
||||||
} as never);
|
|
||||||
} catch (e: unknown) {
|
|
||||||
const r = e as { location?: string };
|
|
||||||
redirectUrl = r.location ?? null;
|
|
||||||
}
|
|
||||||
|
|
||||||
expect(redirectUrl).toBe('/admin/tags/t2');
|
expect(result).toEqual({ mergeSuccess: true, mergeTargetId: 't2' });
|
||||||
});
|
});
|
||||||
|
|
||||||
it('returns fail when merge API responds not ok', async () => {
|
it('returns fail when merge API responds not ok', async () => {
|
||||||
|
|||||||
Reference in New Issue
Block a user