diff --git a/docs/openapi.yaml b/docs/openapi.yaml index 46db003..75f03a1 100644 --- a/docs/openapi.yaml +++ b/docs/openapi.yaml @@ -2049,6 +2049,36 @@ paths: "404": { $ref: "#/components/responses/NotFound" } "422": { $ref: "#/components/responses/Validation" } + /api/v1/projects/{projectID}/sections/reorder: + parameters: + - { name: projectID, in: path, required: true, schema: { type: string } } + post: + tags: [projects] + summary: Set the order the project's sections appear in. Editors and owners. + description: | + The whole list at once, for the same reasons as `/projects/reorder`: a + project has a handful of sections, so sending the order you want is + simpler than midpoints and cannot land half-applied. + + Ids belonging to another project are skipped rather than refused. + requestBody: + required: true + content: + application/json: + schema: + type: object + required: [ids] + properties: + ids: + type: array + maxItems: 500 + items: { type: string } + description: Section ids, top to bottom. + responses: + "204": { description: Reordered. } + "404": { $ref: "#/components/responses/NotFound" } + "422": { $ref: "#/components/responses/Validation" } + /api/v1/sections/{sectionID}: parameters: - { name: sectionID, in: path, required: true, schema: { type: string } } diff --git a/internal/httpapi/projects_handlers.go b/internal/httpapi/projects_handlers.go index 0ba3dd0..2ee6908 100644 --- a/internal/httpapi/projects_handlers.go +++ b/internal/httpapi/projects_handlers.go @@ -356,6 +356,34 @@ func (s *Server) handleCreateSection(w http.ResponseWriter, r *http.Request) { writeJSON(w, http.StatusCreated, sectionJSON{sec.ID, sec.ProjectID, sec.Name, sec.SortOrder}) } +// handleReorderSections sets the order of the sections inside one project. +// +// Under /projects/{projectID}/ rather than beside the other section routes, which +// hang off a section id: the thing being changed here is the project's list, not +// any one section in it. That also puts it behind the same project permission +// check as everything else in this subtree, instead of resolving a section to its +// project one id at a time and hoping they all named the same one. +func (s *Server) handleReorderSections(w http.ResponseWriter, r *http.Request) { + var req reorderProjectsRequest + if err := decodeJSON(w, r, &req); err != nil { + return + } + // The same ceiling as the projects list, and a bound on the transaction rather + // than an opinion about how many sections a project may have. + if len(req.IDs) > 500 { + writeFieldErrors(w, map[string]string{"ids": "at most 500 at a time"}) + return + } + + projectID := chi.URLParam(r, "projectID") + if err := s.db.ReorderSections(r.Context(), projectID, req.IDs); err != nil { + s.internal(w, r, "reorder sections", err) + return + } + s.activity(r, projectID, "", "section.updated", nil) + w.WriteHeader(http.StatusNoContent) +} + // sectionAccess resolves a section to its project and checks the caller may edit // it. Sections have no permissions of their own — they belong to a project, and // that is where access is decided. diff --git a/internal/httpapi/projects_test.go b/internal/httpapi/projects_test.go index 8742ccc..5a938c9 100644 --- a/internal/httpapi/projects_test.go +++ b/internal/httpapi/projects_test.go @@ -327,6 +327,71 @@ func TestAProjectCanHaveSeveralSections(t *testing.T) { } } +// Sections can be dragged into another order, and the order is the project's. +// +// The list is what a project's plan *is* — "Planlægning" before "I gang" is the +// order the work happens in — and until this endpoint existed the only way to +// change it was to delete a section and make it again, which moves its tasks to +// the unsectioned area on the way past. +func TestSectionsCanBeReordered(t *testing.T) { + ts := newTestServer(t) + ts.bootstrap(t) + + _, project := ts.do(t, "POST", "/api/v1/projects", map[string]any{"name": "Ombygning"}) + id := project["id"].(string) + + ids := map[string]string{} + names := []string{"Planlægning", "I gang", "Til gennemsyn"} + for _, name := range names { + _, body := ts.do(t, "POST", "/api/v1/projects/"+id+"/sections", map[string]any{"name": name}) + ids[name] = body["id"].(string) + } + + want := []string{"Til gennemsyn", "Planlægning", "I gang"} + order := make([]string, len(want)) + for i, name := range want { + order[i] = ids[name] + } + + resp, body := ts.do(t, "POST", "/api/v1/projects/"+id+"/sections/reorder", + map[string]any{"ids": order}) + if resp.StatusCode != http.StatusNoContent { + t.Fatalf("reorder: status %d, body %v", resp.StatusCode, body) + } + + _, listed := ts.do(t, "GET", "/api/v1/projects/"+id+"/sections", nil) + got, _ := listed["sections"].([]any) + if len(got) != len(want) { + t.Fatalf("the project has %d sections, want %d", len(got), len(want)) + } + for i, raw := range got { + if name := raw.(map[string]any)["name"]; name != want[i] { + t.Errorf("section %d is %v, want %q", i, name, want[i]) + } + } + + // A section belonging to somebody else's project is not moved by naming it + // here. The ids in the body are not checked one by one — the WHERE clause is + // what refuses them — so this is the assertion that it actually does. + _, other := ts.do(t, "POST", "/api/v1/projects", map[string]any{"name": "Sommerhus"}) + otherID := other["id"].(string) + _, foreign := ts.do(t, "POST", "/api/v1/projects/"+otherID+"/sections", + map[string]any{"name": "Havearbejde"}) + foreignID := foreign["id"].(string) + + resp, _ = ts.do(t, "POST", "/api/v1/projects/"+id+"/sections/reorder", + map[string]any{"ids": []string{foreignID}}) + if resp.StatusCode != http.StatusNoContent { + t.Fatalf("reorder with a foreign id: status %d", resp.StatusCode) + } + + _, stillThere := ts.do(t, "GET", "/api/v1/projects/"+otherID+"/sections", nil) + elsewhere, _ := stillThere["sections"].([]any) + if len(elsewhere) != 1 || elsewhere[0].(map[string]any)["name"] != "Havearbejde" { + t.Errorf("the other project's section did not survive: %v", elsewhere) + } +} + // A project can be created straight into a group. // // `group_id` was declared on the request type, documented on it, and applied by diff --git a/internal/httpapi/server.go b/internal/httpapi/server.go index bab03d2..afb964b 100644 --- a/internal/httpapi/server.go +++ b/internal/httpapi/server.go @@ -426,6 +426,7 @@ func New(cfg *config.Config, db *store.DB, log *slog.Logger, web fs.FS) *Server r.Group(func(r chi.Router) { r.Use(s.requireProject(store.RoleEditor)) r.Post("/sections", s.handleCreateSection) + r.Post("/sections/reorder", s.handleReorderSections) }) r.Group(func(r chi.Router) { diff --git a/internal/store/projects.go b/internal/store/projects.go index bb3ab83..840d771 100644 --- a/internal/store/projects.go +++ b/internal/store/projects.go @@ -646,6 +646,35 @@ func (db *DB) ReorderProjects(ctx context.Context, userID string, ids []string) }) } +// ReorderSections writes the order of one project's sections, top to bottom. +// +// The whole list at once, the same as ReorderProjects and for the same reasons: a +// project has a handful of sections, so writing 0, 1, 2 … is one small +// transaction that cannot run out of precision, and a list is impossible to get +// into the half-ordered state a sequence of individual moves leaves behind when +// one of them fails. +// +// Scoped to the project rather than to a user. A section has no owner of its own — +// it belongs to a project, and whether the caller may rearrange it is a question +// about that project, which the handler has already asked. Ids belonging to some +// other project are skipped by the WHERE clause rather than refused, so a stale +// list from a client that has just moved a section elsewhere cannot rearrange a +// project the caller was never looking at. +func (db *DB) ReorderSections(ctx context.Context, projectID string, ids []string) error { + return db.Tx(ctx, func(tx *sql.Tx) error { + now := time.Now().Unix() + for i, id := range ids { + if _, err := tx.ExecContext(ctx, + `UPDATE sections SET sort_order = ?, updated_at = ? + WHERE id = ? AND project_id = ? AND deleted_at IS NULL`, + float64(i), now, id, projectID); err != nil { + return err + } + } + return nil + }) +} + // SectionByName resolves a "/name" from quick add within one project. // // Case-insensitive, and scoped to the project the task is landing in — a section diff --git a/web/e2e/smoke.spec.js b/web/e2e/smoke.spec.js index 58c7d0b..850ad4b 100644 --- a/web/e2e/smoke.spec.js +++ b/web/e2e/smoke.spec.js @@ -1973,6 +1973,76 @@ test('et projekt kan have flere sektioner @forms', async ({ page }) => { expect(trouble).toEqual([]); }); +/** + * Sektioner kan trækkes op og ned. + * + * Rækkefølgen af sektioner *er* planen — "Planlægning" står før "I gang", fordi + * det er den vej arbejdet går — og indtil nu kunne den kun laves om ved at slette + * en sektion og lave den igen, hvilket smider dens opgaver ud i det usektionerede + * område på vejen. + * + * Målt på rækkefølgen af overskrifterne på siden og bagefter på, hvad serveren + * svarer efter en genindlæsning: den første halvdel kan bestå af, at fladen + * flyttede et element, den anden kan kun bestå, hvis skrivningen gik igennem. + */ +test('sektioner kan trækkes i en anden rækkefølge, og den overlever en genindlæsning', 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('Havehus'); + await sidebar.getByLabel('Projektnavn').press('Enter'); + await expect(page.getByRole('heading', { name: 'Havehus' })).toBeVisible(); + + for (const name of ['Fundament', 'Rejsning', 'Tag']) { + await page.getByRole('button', { name: '+ Tilføj sektion' }).click(); + const field = page.getByLabel('Ny sektion'); + await field.fill(name); + await field.press('Enter'); + await expect(page.getByRole('heading', { name, exact: true })).toBeVisible(); + } + + const order = () => + page.locator('.section-head h2').evaluateAll((els) => els.map((e) => e.textContent.trim())); + + expect(await order()).toEqual(['Fundament', 'Rejsning', 'Tag']); + + // Tag trækkes helt op. Sluppet i den øverste halvdel af "Fundament", som er + // gaben over den — hvilken halvdel man rammer, er hele forskellen på over og + // under, så positionen er skrevet ud og ikke overladt til midten. + await page + .locator('.section-head') + .filter({ hasText: 'Tag' }) + .dragTo(page.locator('.section-head').filter({ hasText: 'Fundament' }), { + targetPosition: { x: 40, y: 3 } + }); + + await expect.poll(order).toEqual(['Tag', 'Fundament', 'Rejsning']); + + await page.reload(); + await expect(page.getByRole('heading', { name: 'Tag', exact: true })).toBeVisible(); + expect(await order(), 'serveren tog ikke rækkefølgen').toEqual([ + 'Tag', + 'Fundament', + 'Rejsning' + ]); + + // Og den anden vej: sluppet i den nederste halvdel lander den under. + await page + .locator('.section-head') + .filter({ hasText: 'Tag' }) + .dragTo(page.locator('.section-head').filter({ hasText: 'Rejsning' }), { + targetPosition: { x: 40, y: 28 } + }); + + await expect.poll(order).toEqual(['Fundament', 'Rejsning', 'Tag']); + + expect(trouble).toEqual([]); +}); + /** * Form fields on a touch device are at least 16px. * diff --git a/web/src/lib/api.js b/web/src/lib/api.js index a96cdc2..354611f 100644 --- a/web/src/lib/api.js +++ b/web/src/lib/api.js @@ -283,6 +283,10 @@ export const api = { createSection: (projectId, name) => post(`/projects/${projectId}/sections`, { name }), updateSection: (id, data) => patch(`/sections/${id}`, data), deleteSection: (id) => del(`/sections/${id}`), + // The whole list, the same as projects and groups: a project has a handful of + // sections, and an order that cannot land half-applied is worth more than the + // bytes a single move would save. + reorderSections: (projectId, ids) => post(`/projects/${projectId}/sections/reorder`, { ids }), listMembers: (projectId) => get(`/projects/${projectId}/members`), invite: (projectId, email, role) => post(`/projects/${projectId}/invites`, { email, role }), diff --git a/web/src/lib/dnd.js b/web/src/lib/dnd.js index faf5c8b..b8e6fd4 100644 --- a/web/src/lib/dnd.js +++ b/web/src/lib/dnd.js @@ -18,6 +18,7 @@ export const TASK = 'application/x-verdande-task'; export const PROJECT = 'application/x-verdande-project'; export const GROUP = 'application/x-verdande-group'; export const NOTE = 'application/x-verdande-note'; +export const SECTION = 'application/x-verdande-section'; /** Starts a drag carrying one id of the given kind. */ export function startDrag(event, type, id) { diff --git a/web/src/routes/projekt/[id]/+page.svelte b/web/src/routes/projekt/[id]/+page.svelte index f4dfaf0..37d0b7e 100644 --- a/web/src/routes/projekt/[id]/+page.svelte +++ b/web/src/routes/projekt/[id]/+page.svelte @@ -6,7 +6,7 @@ import { app, completedView, projectName } from '$lib/stores.svelte.js'; import { COLORS, colorVar } from '$lib/colors.js'; import { eventName, eventDetail } from '$lib/events.js'; - import { TASK, carries, dragged, accept } from '$lib/dnd.js'; + import { TASK, SECTION, startDrag, carries, dragged, accept } from '$lib/dnd.js'; import TaskList from '$lib/components/TaskList.svelte'; import TaskRow from '$lib/components/TaskRow.svelte'; import QuickAdd from '$lib/components/QuickAdd.svelte'; @@ -386,6 +386,80 @@ } } + // --- moving a section --------------------------------------------------------- + // + // The order of the sections is the order of the plan — "Planlægning" stands + // before "I gang" because that is when the work happens — and until this it + // could only be changed by deleting a section and making it again, which drops + // its tasks into the unsectioned area on the way past. + // + // The whole heading is the handle, not a grip beside it. A section heading + // holds nothing that is itself dragged, so there is no gesture to steal — which + // is the reason the sidebar's rows needed one, and the reason these do not. + + let draggingSection = $state(null); + /** The heading whose gap is lit up, and which side of it. */ + let overSectionEdge = $state(null); + let overSectionBelow = $state(false); + + function clearSectionDrag() { + draggingSection = null; + overSectionEdge = null; + } + + function onSectionDragStart(event, section) { + draggingSection = section.id; + startDrag(event, SECTION, section.id); + } + + function onHeadDragOver(event, section) { + if (!canEdit || !carries(event, SECTION) || draggingSection === section.id) return; + accept(event); + const box = event.currentTarget.getBoundingClientRect(); + overSectionEdge = section.id; + overSectionBelow = event.clientY > box.top + box.height / 2; + } + + /** + * Optimistic, and the whole list at once. + * + * The array is what the page renders from, so moving the entry moves the + * section; `sort_order` is set alongside it only so a later read of this list + * agrees with what is on screen. If the write fails the previous array goes + * back — a half-moved section is worse than one that did not move. + */ + async function onHeadDrop(event, target) { + if (!carries(event, SECTION)) return; + event.preventDefault(); + event.stopPropagation(); + const id = dragged(event, SECTION) || draggingSection; + const below = overSectionBelow; + clearSectionDrag(); + if (!id || id === target.id || !canEdit) return; + + const moved = sections.find((s) => s.id === id); + if (!moved) return; + + const without = sections.filter((s) => s.id !== id); + const at = without.findIndex((s) => s.id === target.id); + if (at < 0) return; + + const ordered = [...without]; + ordered.splice(below ? at + 1 : at, 0, moved); + + const previous = sections; + sections = ordered.map((s, i) => ({ ...s, sort_order: i })); + try { + await api.reorderSections( + project.id, + ordered.map((s) => s.id) + ); + } catch (e) { + sections = previous; + app.toast(humanMessage(e)); + } + } + async function removeSection(section) { const count = open.filter((t) => t.section_id === section.id).length; const warning = count @@ -739,7 +813,22 @@ ondragleave={() => (overSection = null)} ondrop={(e) => onSectionDrop(e, section.id)} > -
+ + +
onSectionDragStart(e, section)} + ondragend={clearSectionDrag} + ondragover={(e) => onHeadDragOver(e, section)} + ondragleave={() => (overSectionEdge = null)} + ondrop={(e) => onHeadDrop(e, section)} + > {#if renamingSection === section.id}
renameSection(e, section)}>