diff --git a/internal/httpapi/projects_test.go b/internal/httpapi/projects_test.go index faebdd6..8742ccc 100644 --- a/internal/httpapi/projects_test.go +++ b/internal/httpapi/projects_test.go @@ -365,3 +365,59 @@ func TestAProjectCanBeCreatedIntoAGroup(t *testing.T) { t.Errorf("the stored project has group_id = %v, want %q", found["group_id"], groupID) } } + +// A section holds as many tasks as you put in it. +// +// Reported from use: "sections can only have one task". Driven through the same +// endpoint the interface uses, with the same `after_id` the drop handler sends — +// the second task goes after the first, which is the case that would fail if +// positioning inside a section were wrong. +func TestASectionHoldsMoreThanOneTask(t *testing.T) { + ts := newTestServer(t) + ts.bootstrap(t) + + _, project := ts.do(t, "POST", "/api/v1/projects", map[string]any{"name": "Skovvænget"}) + projectID := project["id"].(string) + _, section := ts.do(t, "POST", "/api/v1/projects/"+projectID+"/sections", + map[string]any{"name": "Håndværker"}) + sectionID := section["id"].(string) + + var previous string + for _, content := range []string{"knager på badeværelset", "plade på opvaskemaskine", "fuge om vinduet"} { + _, created := ts.do(t, "POST", "/api/v1/tasks", map[string]any{ + "content": content, "project_id": projectID, + }) + id := created["id"].(string) + + resp, moved := ts.do(t, "POST", "/api/v1/tasks/"+id+"/move", map[string]any{ + "project_id": projectID, "section_id": sectionID, "after_id": previous, + }) + if resp.StatusCode != http.StatusOK { + t.Fatalf("move %q: status %d, body %v", content, resp.StatusCode, moved) + } + if moved["section_id"] != sectionID { + t.Fatalf("%q did not land in the section: %v", content, moved["section_id"]) + } + previous = id + } + + _, listed := ts.do(t, "GET", "/api/v1/tasks?project_id="+projectID, nil) + tasks, _ := listed["tasks"].([]any) + inSection := []string{} + for _, raw := range tasks { + task := raw.(map[string]any) + if task["section_id"] == sectionID { + inSection = append(inSection, task["content"].(string)) + } + } + if len(inSection) != 3 { + t.Fatalf("the section holds %d tasks, want 3: %v", len(inSection), inSection) + } + // And in the order they were placed, since each went after the one before it. + want := []string{"knager på badeværelset", "plade på opvaskemaskine", "fuge om vinduet"} + for i, content := range want { + if inSection[i] != content { + t.Errorf("position %d is %q, want %q — the whole list: %v", i, inSection[i], content, inSection) + } + } +} diff --git a/web/e2e/smoke.spec.js b/web/e2e/smoke.spec.js index 631a143..f2241c2 100644 --- a/web/e2e/smoke.spec.js +++ b/web/e2e/smoke.spec.js @@ -1203,6 +1203,75 @@ test('færdige opgaver kan vises og skjules igen', async ({ page }) => { expect(trouble).toEqual([]); }); +/** + * A section holds more than one task, and stops being highlighted afterwards. + * + * Both reported from use, and both the same root cause. TaskList read the dragged + * task from its *own* `draggingId`, which is only set when the drag started in + * that list — so a task dragged in from the unsectioned rows arrived as null and + * the handler returned early. It had already called `stopPropagation`, so the + * section around it never got the drop either. + * + * The result was a section you could only drop into while it was empty, because + * an empty one has no rows to aim at. Put one task in, and there was no way to add + * a second: it looked exactly like a section that holds one task. + * + * The same `stopPropagation` left the section's frame lit, since the handler that + * clears it never ran. + */ +test('en sektion kan rumme flere opgaver, og rammen forsvinder bagefter @forms', async ({ + page +}) => { + const trouble = watchForTrouble(page); + await page.goto('/'); + + const sidebar = page.getByRole('navigation', { name: 'Hovedmenu' }); + await sidebar.getByRole('button', { name: 'Nyt projekt' }).click(); + await sidebar.getByLabel('Projektnavn').fill('Badeværelset'); + await sidebar.getByLabel('Projektnavn').press('Enter'); + await expect(page.getByRole('heading', { name: 'Badeværelset' })).toBeVisible(); + + await page.getByRole('button', { name: '+ Tilføj sektion' }).click(); + await page.getByLabel('Ny sektion').fill('Håndværker'); + await page.getByLabel('Ny sektion').press('Enter'); + await expect(page.getByRole('heading', { name: 'Håndværker' })).toBeVisible(); + + for (const what of ['knager op', 'fuge om vinduet']) { + await page.getByLabel('Ny opgave').fill(what); + await page.getByLabel('Ny opgave').press('Enter'); + await expect(page.getByText(what, { exact: true })).toBeVisible(); + } + + const section = page.locator('section').filter({ + has: page.getByRole('heading', { name: 'Håndværker' }) + }); + const row = (what) => page.locator('.row').filter({ hasText: what }); + + // The first goes in while the section is empty — the only drop that used to + // work, because there was no row in the way to swallow it. + await row('knager op').dragTo(section); + await expect(section.getByText('knager op', { exact: true })).toBeVisible(); + + // And the second, aimed at the row that is now in there. This is the one that + // did nothing at all. + await row('fuge om vinduet').dragTo(section.getByText('knager op', { exact: true })); + await expect(section.getByText('fuge om vinduet', { exact: true })).toBeVisible(); + await expect(section.locator('.row')).toHaveCount(2); + + // The frame is a drop target being offered, not a state. Once the drag is over + // it has nothing left to say. + await expect(section).not.toHaveClass(/\bover\b/); + + // It survives a reload, so this is the server's answer and not the page's. + await page.reload(); + const after = page.locator('section').filter({ + has: page.getByRole('heading', { name: 'Håndværker' }) + }); + await expect(after.locator('.row')).toHaveCount(2); + + expect(trouble).toEqual([]); +}); + /** * The sidebar never scrolls sideways. * diff --git a/web/src/lib/components/TaskList.svelte b/web/src/lib/components/TaskList.svelte index ec48629..bff8a97 100644 --- a/web/src/lib/components/TaskList.svelte +++ b/web/src/lib/components/TaskList.svelte @@ -12,8 +12,9 @@ * drawn where the row would land. */ import { app } from '$lib/stores.svelte.js'; + import { t } from '$lib/i18n.svelte.js'; import { api } from '$lib/api.js'; - import { TASK, startDrag, carries, accept } from '$lib/dnd.js'; + import { TASK, startDrag, carries, dragged, accept } from '$lib/dnd.js'; import TaskRow from './TaskRow.svelte'; let { tasks, projectId, sectionId = '', canEdit = true } = $props(); @@ -33,7 +34,12 @@ } function onDragOver(event, task) { - if (!canEdit || !carries(event, TASK) || !draggingId || draggingId === task.id) return; + // `carries` rather than `draggingId`: a task dragged from another list — the + // unsectioned rows above, or another section — never called this component's + // own dragstart, so its `draggingId` is null. Refusing on that made every + // list reject everything that came from outside it, which is the whole point + // of dragging between them. + if (!canEdit || !carries(event, TASK) || draggingId === task.id) return; accept(event); // Which half of the row the pointer is in decides the gap. Comparing against @@ -54,13 +60,22 @@ // gesture and for sections with no rows to aim at. A drop that landed on a // row has already said exactly where it goes, so it stops here. event.stopPropagation(); - const id = draggingId; + // From the drag itself, not from `draggingId`, which is only set when the + // drag *started* in this list. A task dragged in from the unsectioned rows + // or another section left it null, so this returned early — and it had + // already called stopPropagation, so the section around it never got the + // drop either. The result: a task could only be dropped into a section with + // no rows in it to aim at, which made every section look like it could hold + // exactly one task. + const id = dragged(event, TASK) || draggingId; const below = overBelow; draggingId = null; overId = null; if (!id || !canEdit || id === target.id) return; const ordered = [...tasks].sort((a, b) => a.sort_order - b.sort_order); + // The dragged task may not be in this list at all — that is the cross-list + // case — so `without` is "this list minus it" either way. const without = ordered.filter((t) => t.id !== id); const at = without.findIndex((t) => t.id === target.id); if (at < 0) return; @@ -101,7 +116,7 @@ ); } catch { app.replace(id, previous); - app.toast('Kunne ikke flytte opgaven.'); + app.toast(t('task.moveFailed')); } } diff --git a/web/src/lib/locales/da.js b/web/src/lib/locales/da.js index be96652..0953121 100644 --- a/web/src/lib/locales/da.js +++ b/web/src/lib/locales/da.js @@ -587,5 +587,7 @@ export const da = { 'view.done': 'Færdige', 'view.showDone': 'Vis færdige', - 'view.hideDone': 'Skjul færdige' + 'view.hideDone': 'Skjul færdige', + + 'task.moveFailed': 'Kunne ikke flytte opgaven.' }; diff --git a/web/src/lib/locales/en.js b/web/src/lib/locales/en.js index 2c238f3..2da314c 100644 --- a/web/src/lib/locales/en.js +++ b/web/src/lib/locales/en.js @@ -582,5 +582,7 @@ export const en = { 'view.done': 'Done', 'view.showDone': 'Show done', - 'view.hideDone': 'Hide done' + 'view.hideDone': 'Hide done', + + 'task.moveFailed': 'The task could not be moved.' }; diff --git a/web/src/routes/projekt/[id]/+page.svelte b/web/src/routes/projekt/[id]/+page.svelte index e9ed300..a2be3f0 100644 --- a/web/src/routes/projekt/[id]/+page.svelte +++ b/web/src/routes/projekt/[id]/+page.svelte @@ -197,6 +197,24 @@ */ let overSection = $state(null); + /** + * Clears the highlight when the drag ends, however it ends. + * + * `dragleave` and the section's own `ondrop` are not enough. A drop on a *row* + * inside a section is handled by TaskList, which stops propagation — so the + * section never sees the drop and its frame stayed lit afterwards. A drag + * abandoned with Escape has the same shape: no drop at all, and the frame kept + * standing until the next one. + * + * `dragend` fires exactly once per drag, on the source, and bubbles to the + * window — so it is the one place that is true for every way a drag can finish. + */ + $effect(() => { + const clear = () => (overSection = null); + window.addEventListener('dragend', clear); + return () => window.removeEventListener('dragend', clear); + }); + function onSectionDragOver(event, sectionId) { if (!canEdit || !carries(event, TASK)) return; accept(event);