From 2807687c73b9442809f863ab5d775ea7c64f7030 Mon Sep 17 00:00:00 2001 From: Chris Amico Date: Mon, 23 Sep 2024 14:45:33 -0400 Subject: [PATCH 01/12] Add a type and getApiResponse utility to wrap API responses --- src/lib/api/types.d.ts | 17 +++++++++---- src/lib/utils/api.ts | 42 +++++++++++++++++++++++++++++++++ src/lib/utils/tests/api.test.ts | 27 +++++++++++++++++++++ 3 files changed, 81 insertions(+), 5 deletions(-) diff --git a/src/lib/api/types.d.ts b/src/lib/api/types.d.ts index 7b39d1156..c0961eca9 100644 --- a/src/lib/api/types.d.ts +++ b/src/lib/api/types.d.ts @@ -33,11 +33,18 @@ export type ViewerMode = ReadMode | WriteMode; export type Zoom = number | Sizes | "width" | "height"; -export interface APIError { - error: { - status: number; - message: string; - }; +export interface APIError { + status: number; + message: string; + errors?: E; +} + +/** + * Wrap an API response so we can pass errors along + */ +export interface APIResponse { + data?: T; + error?: APIError; } export interface NoteHighlight { diff --git a/src/lib/utils/api.ts b/src/lib/utils/api.ts index 92cbafda3..d5a7462d9 100644 --- a/src/lib/utils/api.ts +++ b/src/lib/utils/api.ts @@ -1,5 +1,6 @@ import type { NumericRange } from "@sveltejs/kit"; import type { Page } from "@/api/types"; +import type { APIError, APIResponse } from "$lib/api/types"; import { CSRF_COOKIE_NAME, MAX_PER_PAGE } from "@/config/config"; export function isErrorCode(status: number): status is NumericRange<400, 599> { @@ -12,6 +13,47 @@ export function isRedirectCode( return status >= 300 && status <= 308; } +/** + * Handle what comes back from the API and return either data or errors. + * + * Two generic types are passed through: + * + * - T is data from the API + * - E is an error coming back from the API + * + * @param resp The fetch response from the API. If this is missing, fetch + * threw an error and we should send a 500 to the user because the API is + * probably down. + */ +export async function getApiResponse( + resp?: Response, +): Promise> { + const response: APIResponse = {}; + + if (!resp) { + response.error = { + status: 500, + message: "API error", + }; + + return response; + } + + if (isErrorCode(resp.status)) { + response.error = { + status: resp.status, + message: resp.statusText, + errors: await resp.json(), + }; + + return response; + } + + // everything worked + response.data = await resp.json(); + return response; +} + /** * Requests the specified URL and paginates through to return all * results if additional pages are present. diff --git a/src/lib/utils/tests/api.test.ts b/src/lib/utils/tests/api.test.ts index 6e4a8c661..ef9f5c960 100644 --- a/src/lib/utils/tests/api.test.ts +++ b/src/lib/utils/tests/api.test.ts @@ -2,9 +2,11 @@ import { describe, test, expect, vi } from "vitest"; import { isErrorCode, isRedirectCode, + getApiResponse, getPrivateAsset, getCsrfToken, } from "../api"; + import { CSRF_COOKIE_NAME } from "@/config/config.js"; describe("response code tests", () => { @@ -66,6 +68,31 @@ describe("api handling", () => { expect(url.href).toEqual(privateUrl); }); + + test("getApiResponse", async () => { + let resp: Response; + + const body = { test: true }; + const errors = { error: ["This is an error"] }; + resp = new Response(JSON.stringify(body), { status: 200 }); + + let response = await getApiResponse(resp); + + expect(response.data).toEqual(body); + expect(response.error).toBeUndefined(); + + resp = new Response(JSON.stringify(errors), { status: 400 }); + + response = await getApiResponse(resp); + expect(response.data).toBeUndefined(); + expect(response.error.status).toEqual(400); + expect(response.error.errors).toEqual(errors); + + // no response is a 500 + response = await getApiResponse(); + expect(response.data).toBeUndefined(); + expect(response.error.status).toEqual(500); + }); }); describe("cookie handling", () => { From 77119590372dfed7a3151aa66e8e34b9582b775e Mon Sep 17 00:00:00 2001 From: Chris Amico Date: Mon, 23 Sep 2024 22:54:40 -0400 Subject: [PATCH 02/12] Migrated documents and collaborators API methods to new response types and made tests pass --- package.json | 2 +- src/lib/api/collaborators.ts | 51 +++++----- src/lib/api/documents.ts | 118 ++++++++---------------- src/lib/api/tests/collaborators.test.ts | 18 ++-- src/lib/api/tests/documents.test.ts | 59 ++++++++---- src/lib/api/types.d.ts | 3 + src/lib/utils/api.ts | 2 +- 7 files changed, 117 insertions(+), 136 deletions(-) diff --git a/package.json b/package.json index 6863d3407..22c648067 100644 --- a/package.json +++ b/package.json @@ -88,7 +88,7 @@ "sync": "svelte-kit sync", "check": "svelte-kit sync && svelte-check --tsconfig ./jsconfig.json", "check:watch": "svelte-kit sync && svelte-check --tsconfig ./jsconfig.json --watch", - "test": "npm run test:unit && npm run test:browser", + "test": "TZ=UTC vitest run", "test:browser": "playwright test", "test:unit": "TZ=UTC vitest run", "test:watch": " TZ=UTC vitest watch", diff --git a/src/lib/api/collaborators.ts b/src/lib/api/collaborators.ts index 6f6b43de1..a4849714c 100644 --- a/src/lib/api/collaborators.ts +++ b/src/lib/api/collaborators.ts @@ -1,8 +1,13 @@ // manage users in a project -import type { ProjectAccess, ProjectUser } from "./types"; +import type { + APIResponse, + ProjectAccess, + ProjectUser, + ValidationError, +} from "./types"; import { APP_URL, BASE_API_URL, CSRF_HEADER_NAME } from "@/config/config.js"; -import { getAll, isErrorCode } from "$lib/utils/api"; +import { getAll, getApiResponse } from "$lib/utils/api"; export async function list(project_id: number, fetch = globalThis.fetch) { const endpoint = new URL( @@ -13,12 +18,15 @@ export async function list(project_id: number, fetch = globalThis.fetch) { return getAll(endpoint, undefined, fetch); } +/** + * Add a collaborator to a project + */ export async function add( project_id: number, user: { email: string; access: ProjectAccess }, csrf_token: string, fetch = globalThis.fetch, -) { +): Promise> { const endpoint = new URL(`projects/${project_id}/users/`, BASE_API_URL); const resp = await fetch(endpoint, { @@ -32,26 +40,19 @@ export async function add( method: "POST", }).catch(console.error); - if (!resp) { - throw new Error("API unavailable"); - } - - if (isErrorCode(resp.status)) { - const data = await resp.json(); - console.error(data); - throw new Error(resp.statusText); - } - - return resp.json(); + return getApiResponse(resp); } +/** + * Update a user's permissions within a project + */ export async function update( project_id: number, user_id: number, access: ProjectAccess, csrf_token: string, fetch = globalThis.fetch, -) { +): Promise> { const endpoint = new URL( `projects/${project_id}/users/${user_id}/`, BASE_API_URL, @@ -68,17 +69,7 @@ export async function update( method: "PATCH", }).catch(console.error); - if (!resp) { - throw new Error("API unavailable"); - } - - if (isErrorCode(resp.status)) { - const data = await resp.json(); - console.error(data); - throw new Error(resp.statusText); - } - - return resp.json(); + return getApiResponse(resp); } export async function remove( @@ -86,13 +77,13 @@ export async function remove( user_id: number, csrf_token: string, fetch = globalThis.fetch, -) { +): Promise> { const endpoint = new URL( `projects/${project_id}/users/${user_id}/`, BASE_API_URL, ); - return fetch(endpoint, { + const resp = await fetch(endpoint, { credentials: "include", headers: { "Content-type": "application/json", @@ -100,5 +91,7 @@ export async function remove( Referer: APP_URL, }, method: "DELETE", - }); + }).catch(console.error); + + return getApiResponse(resp); } diff --git a/src/lib/api/documents.ts b/src/lib/api/documents.ts index d9a989b34..66aa51ed6 100644 --- a/src/lib/api/documents.ts +++ b/src/lib/api/documents.ts @@ -2,6 +2,7 @@ * Lots of duplicated code here that should get consolidated at some point. */ import type { + APIResponse, Data, DataUpdate, Document, @@ -17,10 +18,10 @@ import type { ReadMode, WriteMode, ViewerMode, + ValidationError, } from "./types"; import { writable, type Writable } from "svelte/store"; -import { error } from "@sveltejs/kit"; import { DEFAULT_EXPAND } from "@/api/common.js"; import { isOrg } from "./accounts"; import { @@ -30,7 +31,7 @@ import { DC_BASE, EMBED_URL, } from "@/config/config.js"; -import { isErrorCode, getPrivateAsset } from "../utils/index"; +import { isErrorCode, getPrivateAsset, getApiResponse } from "../utils/api"; export const READING_MODES = new Set([ "document", @@ -60,7 +61,7 @@ export async function search( version: "2.0", }, fetch = globalThis.fetch, -): Promise { +): Promise> { const endpoint = new URL("documents/search/", BASE_API_URL); endpoint.searchParams.set("expand", DEFAULT_EXPAND); @@ -72,14 +73,11 @@ export async function search( } } - const resp = await fetch(endpoint, { credentials: "include" }); - - if (isErrorCode(resp.status)) { - console.error(await resp.json()); - error(resp.status, resp.statusText); - } + const resp = await fetch(endpoint, { credentials: "include" }).catch( + console.error, + ); - return resp.json(); + return getApiResponse(resp); } /** @@ -89,7 +87,7 @@ export async function search( export async function get( id: string | number, fetch: typeof globalThis.fetch = globalThis.fetch, -): Promise { +): Promise> { const endpoint = new URL(`documents/${id}.json`, BASE_API_URL); const expand = [ "user", @@ -105,20 +103,12 @@ export async function get( console.error, ); - // backend error, not much we can do - if (!resp) { - error(500); - } - - if (isErrorCode(resp.status)) { - error(resp.status, resp.statusText); - } - - return resp.json(); + return getApiResponse(resp); } /** * Get text for a document. It may be a private asset, which requires a two-step fetch. + * Errors will produce an empty response. */ export async function text( document: Document, @@ -177,15 +167,12 @@ export async function textPositions( * If documents contain a `file_url` property, the server will attempt to fetch and upload that file. * Otherwise, the response will contain all documents fields plus a `presigned_url` field, which should * be passed to `upload` to store the actual file. - * - * @async - * @export */ export async function create( documents: DocumentUpload[], csrf_token: string, fetch = globalThis.fetch, -): Promise { +): Promise> { const endpoint = new URL("documents/", BASE_API_URL); const resp = await fetch(endpoint, { @@ -199,20 +186,13 @@ export async function create( body: JSON.stringify(documents), }); - if (isErrorCode(resp.status)) { - throw new Error(await resp.text()); - } - - return resp.json() as Promise; + return getApiResponse(resp); } /** * Upload file data to a presigned_url on cloud storage. * Use this after running `create` to add documents to the database. * This function is a very thin wrapper around fetch. - * - * @async - * @export */ export async function upload( presigned_url: URL, @@ -230,9 +210,7 @@ export async function upload( /** * Tell the backend to begin processing a batch of documents. - * - * @async - * @export + * Only errors are returned here. A null response means success. */ export async function process( documents: { @@ -242,10 +220,10 @@ export async function process( }[], csrf_token: string, fetch = globalThis.fetch, -): Promise { +): Promise> { const endpoint = new URL("documents/process/", BASE_API_URL); - return fetch(endpoint, { + const resp = await fetch(endpoint, { credentials: "include", method: "POST", headers: { @@ -254,7 +232,9 @@ export async function process( Referer: APP_URL, }, body: JSON.stringify(documents), - }); + }).catch(console.error); + + return getApiResponse(resp); } /** @@ -264,46 +244,46 @@ export async function cancel( document: Document, csrf_token: string, fetch = globalThis.fetch, -): Promise { +): Promise> { const processing: Set = new Set(["pending", "readable"]); // non-processing status is a no-op - if (!processing.has(document.status)) return; + if (!processing.has(document.status)) return {}; const endpoint = new URL(`documents/${document.id}/process/`, BASE_API_URL); - return fetch(endpoint, { + const resp = await fetch(endpoint, { credentials: "include", method: "DELETE", headers: { [CSRF_HEADER_NAME]: csrf_token, Referer: APP_URL, }, - }); + }).catch(console.error); + + return getApiResponse(resp); } /** * Delete a document. There is no undo. - * - * @param id - * @param csrf_token - * @param fetch */ export async function destroy( id: string | number, csrf_token: string, fetch = globalThis.fetch, -) { +): Promise> { const endpoint = new URL(`documents/${id}/`, BASE_API_URL); - return fetch(endpoint, { + const resp = await fetch(endpoint, { credentials: "include", method: "DELETE", headers: { [CSRF_HEADER_NAME]: csrf_token, Referer: APP_URL, }, - }); + }).catch(console.log); + + return getApiResponse(resp); } /** @@ -317,18 +297,20 @@ export async function destroy_many( ids: (string | number)[], csrf_token: string, fetch = globalThis.fetch, -) { +): Promise> { const endpoint = new URL(`documents/`, BASE_API_URL); endpoint.searchParams.set("id__in", ids.join(",")); - return fetch(endpoint, { + const resp = await fetch(endpoint, { credentials: "include", method: "DELETE", headers: { [CSRF_HEADER_NAME]: csrf_token, Referer: APP_URL, }, - }); + }).catch(console.log); + + return getApiResponse(resp); } /** @@ -345,7 +327,7 @@ export async function edit( data: Partial, csrf_token: string, fetch = globalThis.fetch, -): Promise { +): Promise> { const endpoint = new URL(`documents/${id}/`, BASE_API_URL); const resp = await fetch(endpoint, { @@ -359,16 +341,7 @@ export async function edit( body: JSON.stringify(data), }).catch(console.error); - if (!resp) { - throw new Error("API unavailable"); - } - - if (isErrorCode(resp.status)) { - const { data } = await resp.json(); - throw new Error(data); - } - - return resp.json(); + return getApiResponse(resp); } /** @@ -411,7 +384,7 @@ export async function add_tags( values: string[], csrf_token: string, fetch = globalThis.fetch, -) { +): Promise> { const endpoint = new URL(`documents/${doc_id}/data/${key}/`, BASE_API_URL); const data: DataUpdate = { values }; @@ -426,15 +399,7 @@ export async function add_tags( body: JSON.stringify(data), }).catch(console.error); - if (!resp) { - throw new Error("API error"); - } - - if (isErrorCode(resp.status)) { - throw new Error(resp.statusText); - } - - return resp.json(); + return getApiResponse(resp); } export async function redact( @@ -442,7 +407,7 @@ export async function redact( redactions: Redaction[], csrf_token: string, fetch = globalThis.fetch, -) { +): Promise { const endpoint = new URL(`documents/${id}/redactions/`, BASE_API_URL); // redaction is a fire-and-reprocess method, so all we have to go on is a response @@ -460,9 +425,6 @@ export async function redact( /** * Get pending documents. This returns an empty array for any error. - * - * @param {fetch} fetch - * @returns {Promise} */ export async function pending(fetch = globalThis.fetch): Promise { const endpoint = new URL("documents/pending/", BASE_API_URL); diff --git a/src/lib/api/tests/collaborators.test.ts b/src/lib/api/tests/collaborators.test.ts index 6e354a2ca..f4622c78f 100644 --- a/src/lib/api/tests/collaborators.test.ts +++ b/src/lib/api/tests/collaborators.test.ts @@ -72,17 +72,18 @@ describe("manage project users", () => { }; }); - const result = await collaborators.add( + const { data } = await collaborators.add( project.id, { email: me.email, access: "admin" }, "token", mockFetch, ); - expect(result.results).toMatchObject([ - ...users.results, - { user: me, access: "admin" }, - ]); + expect(data).toMatchObject({ + previous: null, + next: null, + results: [...users.results, { user: me, access: "admin" }], + }); expect(mockFetch).toHaveBeenCalledWith( new URL(`projects/${project.id}/users/`, BASE_API_URL), @@ -120,7 +121,7 @@ describe("manage project users", () => { const me = users.results.find((u) => u.user.id === 1020); - const updated = await collaborators.update( + const { data: updated } = await collaborators.update( project.id, me.user.id, "edit", @@ -149,18 +150,19 @@ describe("manage project users", () => { return { ok: true, status: 204, + async json() {}, }; }); const me = users.results.find((u) => u.user.id === 1020); - const resp = await collaborators.remove( + const { data } = await collaborators.remove( project.id, me.user.id, "token", mockFetch, ); - expect(resp.status).toStrictEqual(204); + expect(data).toBeUndefined(); expect(mockFetch).toHaveBeenCalledWith( new URL(`projects/${project.id}/users/${me.user.id}/`, BASE_API_URL), { diff --git a/src/lib/api/tests/documents.test.ts b/src/lib/api/tests/documents.test.ts index 5057fd3db..fda238b98 100644 --- a/src/lib/api/tests/documents.test.ts +++ b/src/lib/api/tests/documents.test.ts @@ -109,8 +109,12 @@ describe("document fetching", () => { }; }); - const result = await documents.get(+document.id, mockFetch); + const { data: result, error } = await documents.get( + +document.id, + mockFetch, + ); + expect(error).toBeUndefined(); expect(result).toStrictEqual(document); expect(mockFetch).toBeCalledWith( new URL( @@ -134,8 +138,13 @@ describe("document fetching", () => { }; }); - const results = await documents.search("boston", { hl: true }, mockFetch); + const { data: results, error } = await documents.search( + "boston", + { hl: true }, + mockFetch, + ); + expect(error).toBeUndefined(); expect(results).toStrictEqual(search); expect(mockFetch).toBeCalledWith( new URL( @@ -240,9 +249,8 @@ describe("document uploads and processing", () => { original_extension: d.original_extension, })); - documents.create(docs, "token", mockFetch).then((d) => { - expect(d).toMatchObject(created); - }); + const { data } = await documents.create(docs, "token", mockFetch); + expect(data).toMatchObject(created); expect(mockFetch).toHaveBeenCalledWith( new URL("/api/documents/", DC_BASE), @@ -291,18 +299,18 @@ describe("document uploads and processing", () => { test("documents.process", async ({ created }) => { const mockFetch = vi.fn().mockImplementation(async () => ({ ok: true, - async text() { + async json() { return "OK"; }, })); - const resp = await documents.process( + const { data } = await documents.process( created.map((d) => ({ id: d.id })), "csrf_token", mockFetch, ); - expect(resp.ok).toBeTruthy(); + expect(data).toEqual("OK"); expect(mockFetch).toHaveBeenCalledWith( new URL("/api/documents/process/", DC_BASE), { @@ -324,20 +332,21 @@ describe("document uploads and processing", () => { return { ok: true, status: 200, + async json() {}, }; }); // cancelling a finished document is a noop - let result = await documents.cancel(document, csrf_token, mockFetch); + let { data } = await documents.cancel(document, csrf_token, mockFetch); - expect(result).toBeUndefined(); + expect(data).toBeUndefined(); - result = await documents.cancel( + ({ data } = await documents.cancel( { ...document, status: "pending" }, csrf_token, mockFetch, - ); - expect(result.status).toEqual(200); + )); + expect(data).toBeUndefined(); expect(mockFetch).toHaveBeenCalledTimes(1); expect(mockFetch).toHaveBeenCalledWith( @@ -404,12 +413,18 @@ describe("document write methods", () => { return { ok: true, status: 204, + json() {}, }; }); - const resp = await documents.destroy(document.id, "token", mockFetch); + const { data, error } = await documents.destroy( + document.id, + "token", + mockFetch, + ); - expect(resp.status).toStrictEqual(204); + expect(data).toBeUndefined(); + expect(error).toBeUndefined(); expect(mockFetch).toBeCalledWith( new URL(`documents/${document.id}/`, BASE_API_URL), { @@ -428,6 +443,7 @@ describe("document write methods", () => { return { ok: true, status: 204, + async json() {}, }; }); @@ -435,9 +451,14 @@ describe("document write methods", () => { const endpoint = new URL("documents/", BASE_API_URL); endpoint.searchParams.set("id__in", ids.join(",")); - const resp = await documents.destroy_many(ids, "token", mockFetch); + const { data, error } = await documents.destroy_many( + ids, + "token", + mockFetch, + ); - expect(resp.status).toStrictEqual(204); + expect(data).toBeUndefined(); + expect(error).toBeUndefined(); expect(mockFetch).toBeCalledWith(endpoint, { credentials: "include", method: "DELETE", @@ -460,7 +481,7 @@ describe("document write methods", () => { }; }); - let updated = await documents.edit( + const { data: updated, error } = await documents.edit( document.id, { title: "Updated title" }, "token", @@ -521,7 +542,7 @@ describe("document write methods", () => { }; }); - const data = await documents.add_tags( + const { data } = await documents.add_tags( document.id, "_tag", ["one", "two"], diff --git a/src/lib/api/types.d.ts b/src/lib/api/types.d.ts index c0961eca9..b9efeb4bb 100644 --- a/src/lib/api/types.d.ts +++ b/src/lib/api/types.d.ts @@ -288,3 +288,6 @@ export interface Flatpage { title: string; content: string; // Could be HTML or Markdown } + +// known errors +export interface ValidationError extends Record {} diff --git a/src/lib/utils/api.ts b/src/lib/utils/api.ts index d5a7462d9..8cf58b36b 100644 --- a/src/lib/utils/api.ts +++ b/src/lib/utils/api.ts @@ -26,7 +26,7 @@ export function isRedirectCode( * probably down. */ export async function getApiResponse( - resp?: Response, + resp?: Response | void, ): Promise> { const response: APIResponse = {}; From 89032bcb9b702f8ff68945bd1465b66953b99efc Mon Sep 17 00:00:00 2001 From: Chris Amico Date: Tue, 24 Sep 2024 08:58:15 -0400 Subject: [PATCH 03/12] Search results and viewer work again --- src/lib/api/flatpages.ts | 4 ++- .../components/layouts/DocumentBrowser.svelte | 32 +++++++++++++++++-- src/routes/(app)/documents/+page.svelte | 32 +++++-------------- src/routes/(app)/documents/+page.ts | 9 +----- .../(app)/documents/[id]-[slug]/+layout.ts | 6 +++- 5 files changed, 46 insertions(+), 37 deletions(-) diff --git a/src/lib/api/flatpages.ts b/src/lib/api/flatpages.ts index 3f23c884a..7ed52f952 100644 --- a/src/lib/api/flatpages.ts +++ b/src/lib/api/flatpages.ts @@ -1,7 +1,9 @@ import { BASE_API_URL } from "@/config/config"; import type { Flatpage } from "./types"; -export async function getTipOfDay(fetch = globalThis.fetch): Promise { +export async function getTipOfDay( + fetch = globalThis.fetch, +): Promise { const endpoint = new URL("flatpages/tipofday/", BASE_API_URL); try { const resp = await fetch(endpoint, { credentials: "include" }); diff --git a/src/lib/components/layouts/DocumentBrowser.svelte b/src/lib/components/layouts/DocumentBrowser.svelte index 6d422151d..6d5930150 100644 --- a/src/lib/components/layouts/DocumentBrowser.svelte +++ b/src/lib/components/layouts/DocumentBrowser.svelte @@ -1,5 +1,10 @@ @@ -68,7 +47,12 @@ - + {#if isSignedIn($me)} diff --git a/src/routes/(app)/documents/+page.ts b/src/routes/(app)/documents/+page.ts index 7c7465814..5b450efc0 100644 --- a/src/routes/(app)/documents/+page.ts +++ b/src/routes/(app)/documents/+page.ts @@ -22,14 +22,7 @@ export async function load({ url, fetch, data }) { options.cursor = cursor; } - const searchResults = search(query, options, fetch).catch((e) => { - console.error(e); - return { - results: [], - count: 0, - next: null, - } as DocumentResults; - }); + const searchResults = search(query, options, fetch); const pinnedAddons = getPinnedAddons(fetch).catch((e) => { console.error(e); diff --git a/src/routes/(app)/documents/[id]-[slug]/+layout.ts b/src/routes/(app)/documents/[id]-[slug]/+layout.ts index 66aaa96c9..09751562d 100644 --- a/src/routes/(app)/documents/[id]-[slug]/+layout.ts +++ b/src/routes/(app)/documents/[id]-[slug]/+layout.ts @@ -17,7 +17,11 @@ function documentPath(document: Document) { /** @type {import('./$types').PageLoad} */ export async function load({ fetch, params, parent, depends, url }) { - const document = await documents.get(+params.id, fetch).catch(console.error); + const { data: document, error: err } = await documents.get(+params.id, fetch); + + if (err) { + error(err.status, err.message); + } if (!document) { error(404, "Document not found"); From b24935185254a31afdf8d9de2cc1d735ba3ac506 Mon Sep 17 00:00:00 2001 From: Chris Amico Date: Tue, 24 Sep 2024 10:06:43 -0400 Subject: [PATCH 04/12] Apply suggestions from code review Co-authored-by: Allan Lasser --- src/lib/api/types.d.ts | 2 +- .../components/layouts/DocumentBrowser.svelte | 8 ++++---- src/lib/utils/api.ts | 20 +++++++++++++++++-- 3 files changed, 23 insertions(+), 7 deletions(-) diff --git a/src/lib/api/types.d.ts b/src/lib/api/types.d.ts index b9efeb4bb..39e048ebd 100644 --- a/src/lib/api/types.d.ts +++ b/src/lib/api/types.d.ts @@ -42,7 +42,7 @@ export interface APIError { /** * Wrap an API response so we can pass errors along */ -export interface APIResponse { +export interface APIResponse { data?: T; error?: APIError; } diff --git a/src/lib/components/layouts/DocumentBrowser.svelte b/src/lib/components/layouts/DocumentBrowser.svelte index 6d5930150..d0731bade 100644 --- a/src/lib/components/layouts/DocumentBrowser.svelte +++ b/src/lib/components/layouts/DocumentBrowser.svelte @@ -80,18 +80,18 @@ HIDE_COUNT: width < remToPx(26), }; - $: searchResults = documents.then((r) => excludeDeleted(r.data, $deleted)); + $: searchResults = documents.then((r) => excludeDeleted($deleted, r.data)); // filter out deleted documents that haven't been purged from search yet function excludeDeleted( - searchResults: DocumentResults, deleted: Set, + searchResults?: DocumentResults, // optional params must come second ): DocumentResults { if (deleted.size === 0) return searchResults; - const filtered = searchResults.results.filter( + const filtered = searchResults?.results.filter( (d) => !deleted.has(String(d.id)), - ); + ) ?? []; return { ...searchResults, diff --git a/src/lib/utils/api.ts b/src/lib/utils/api.ts index 8cf58b36b..17bc1ef80 100644 --- a/src/lib/utils/api.ts +++ b/src/lib/utils/api.ts @@ -25,7 +25,7 @@ export function isRedirectCode( * threw an error and we should send a 500 to the user because the API is * probably down. */ -export async function getApiResponse( +export async function getApiResponse( resp?: Response | void, ): Promise> { const response: APIResponse = {}; @@ -50,7 +50,23 @@ export async function getApiResponse( } // everything worked - response.data = await resp.json(); + try { + response.data = await resp.json(); + } catch (e) { + switch (e.name) { + case "SyntaxError": // provide more specific error + response.error = { + status: 500, + message: "The API returned invalid JSON" + } + break; + default: // catch-all handling + response.error = { + status: 500, + message: String(e), + } + } + } return response; } From ba1cfa2b2fb95e2380e8a8015d34b25342c8ed18 Mon Sep 17 00:00:00 2001 From: Chris Amico Date: Tue, 24 Sep 2024 10:46:07 -0400 Subject: [PATCH 05/12] Migrate notes --- src/lib/api/documents.ts | 3 + src/lib/api/flatpages.ts | 2 +- src/lib/api/notes.ts | 80 +++------- src/lib/api/tests/notes.test.ts | 17 +- src/lib/components/forms/EditNote.svelte | 14 +- src/lib/utils/api.ts | 14 +- .../documents/[id]-[slug]/+page.server.ts | 145 +++++++++--------- 7 files changed, 134 insertions(+), 141 deletions(-) diff --git a/src/lib/api/documents.ts b/src/lib/api/documents.ts index 66aa51ed6..7be4b2f45 100644 --- a/src/lib/api/documents.ts +++ b/src/lib/api/documents.ts @@ -402,6 +402,9 @@ export async function add_tags( return getApiResponse(resp); } +/** + * Redact a document. This is a fire-and-forget operation, so we return the response directly. + */ export async function redact( id: number | string, redactions: Redaction[], diff --git a/src/lib/api/flatpages.ts b/src/lib/api/flatpages.ts index 7ed52f952..199a8c2b7 100644 --- a/src/lib/api/flatpages.ts +++ b/src/lib/api/flatpages.ts @@ -1,5 +1,5 @@ -import { BASE_API_URL } from "@/config/config"; import type { Flatpage } from "./types"; +import { BASE_API_URL } from "@/config/config"; export async function getTipOfDay( fetch = globalThis.fetch, diff --git a/src/lib/api/notes.ts b/src/lib/api/notes.ts index 60bd802f6..c8b17c380 100644 --- a/src/lib/api/notes.ts +++ b/src/lib/api/notes.ts @@ -1,4 +1,11 @@ -import type { BBox, Document, Note, NoteResults } from "./types"; +import type { + APIResponse, + BBox, + Document, + Note, + NoteResults, + ValidationError, +} from "./types"; import { APP_URL, @@ -8,16 +15,14 @@ import { } from "@/config/config.js"; import { DEFAULT_EXPAND } from "@/api/common.js"; import { canonicalUrl } from "./documents"; -import { isErrorCode } from "../utils"; +import { getApiResponse, isErrorCode } from "../utils"; /** * Load notes from a single document from the API * @example https://api.www.documentcloud.org/api/documents/2622/notes/ + * @deprecated */ -export async function list( - doc_id: number, - fetch = globalThis.fetch, -): Promise { +export async function list(doc_id: number, fetch = globalThis.fetch) { const endpoint = new URL(`documents/${doc_id}/notes/`, BASE_API_URL); endpoint.searchParams.set("expand", DEFAULT_EXPAND); @@ -39,7 +44,7 @@ export async function get( doc_id: number, note_id: number, fetch = globalThis.fetch, -): Promise { +): Promise> { const endpoint = new URL( `documents/${doc_id}/notes/${note_id}/`, BASE_API_URL, @@ -49,30 +54,20 @@ export async function get( const resp = await fetch(endpoint, { credentials: "include" }); - if (isErrorCode(resp.status)) { - throw new Error(resp.statusText); - } - - return resp.json(); + return getApiResponse(resp); } // writing methods /** * Create a new note - * - * @param doc_id Document ID - * @param note Note data - * @param csrf_token - * @param fetch - * @returns {Note} */ export async function create( doc_id: string | number, note: Partial, csrf_token: string, fetch = globalThis.fetch, -): Promise { +): Promise> { const endpoint = new URL(`documents/${doc_id}/notes/`, BASE_API_URL); const resp = await fetch(endpoint, { @@ -84,28 +79,13 @@ export async function create( Referer: APP_URL, }, method: "POST", - }).catch(console.error); - - if (!resp) { - throw new Error("API error"); - } - - if (!resp.ok) { - throw new Error(resp.statusText); - } + }); - return resp.json(); + return getApiResponse(resp); } /** * Update a note and return the updated version - * - * @param doc_id Document ID - * @param note_id Note ID - * @param note Data to update (partial is fine) - * @param csrf_token - * @param fetch - * @returns {Note} */ export async function update( doc_id: string | number, @@ -113,7 +93,7 @@ export async function update( note: Partial, csrf_token: string, fetch = globalThis.fetch, -): Promise { +): Promise> { const endpoint = new URL( `documents/${doc_id}/notes/${note_id}/`, BASE_API_URL, @@ -128,40 +108,26 @@ export async function update( Referer: APP_URL, }, method: "PATCH", - }).catch(console.error); - - if (!resp) { - throw new Error("API error"); - } - - if (!resp.ok) { - throw new Error(resp.statusText); - } + }); - return resp.json(); + return getApiResponse(resp); } /** - * Delete a note and return the HTTP response - * - * @param doc_id Document ID - * @param note_id Note ID - * @param csrf_token - * @param fetch - * @returns {Response} + * Delete a note */ export async function remove( doc_id: string | number, note_id: string | number, csrf_token: string, fetch = globalThis.fetch, -): Promise { +) { const endpoint = new URL( `documents/${doc_id}/notes/${note_id}/`, BASE_API_URL, ); - return fetch(endpoint, { + const resp = await fetch(endpoint, { credentials: "include", headers: { "Content-type": "application/json", @@ -170,6 +136,8 @@ export async function remove( }, method: "DELETE", }); + + return getApiResponse(resp); } /** diff --git a/src/lib/api/tests/notes.test.ts b/src/lib/api/tests/notes.test.ts index 939a77224..37bba9d83 100644 --- a/src/lib/api/tests/notes.test.ts +++ b/src/lib/api/tests/notes.test.ts @@ -52,8 +52,14 @@ describe("writing notes", () => { }; }); - const result = await notes.create(document.id, note, "token", mockFetch); + const { data: result, error } = await notes.create( + document.id, + note, + "token", + mockFetch, + ); + expect(error).toBeUndefined(); expect(result).toEqual(note); expect(mockFetch).toBeCalledWith( new URL(`documents/${document.id}/notes/`, BASE_API_URL), @@ -85,7 +91,7 @@ describe("writing notes", () => { const update: Partial = { title: "New title" }; - const result = await notes.update( + const { data: result, error } = await notes.update( document.id, note.id, update, @@ -93,6 +99,7 @@ describe("writing notes", () => { mockFetch, ); + expect(error).toBeUndefined(); expect(result).toMatchObject({ ...note, ...update }); expect(mockFetch).toBeCalledWith( new URL(`documents/${document.id}/notes/${note.id}/`, BASE_API_URL), @@ -114,17 +121,19 @@ describe("writing notes", () => { return { ok: true, status: 204, + async json() {}, }; }); - const resp = await notes.remove( + const { data, error } = await notes.remove( document.id, note.id, csrf_token, mockFetch, ); - expect(resp.status).toStrictEqual(204); + expect(data).toBeUndefined(); + expect(error).toBeUndefined(); expect(mockFetch).toBeCalledWith( new URL(`documents/${document.id}/notes/${note.id}/`, BASE_API_URL), { diff --git a/src/lib/components/forms/EditNote.svelte b/src/lib/components/forms/EditNote.svelte index 62997ee7e..d97b7a50c 100644 --- a/src/lib/components/forms/EditNote.svelte +++ b/src/lib/components/forms/EditNote.svelte @@ -15,7 +15,6 @@ Positioning and generating coordinates should happen outside of this form. import AccessLevel from "../inputs/AccessLevel.svelte"; import Button from "../common/Button.svelte"; - import Card from "../common/Card.svelte"; import Field from "../inputs/Field.svelte"; import Flex from "../common/Flex.svelte"; import Text from "../inputs/Text.svelte"; @@ -36,12 +35,14 @@ Positioning and generating coordinates should happen outside of this form. : new URL("?/createAnnotation", canonical).href; $: page_level = !coords || coords.every((c) => !Boolean(c)); - function onSubmit({ formElement }) { - formElement.disabled = true; - return ({ result }) => { + function onSubmit({ formData, submitter }) { + submitter.disabled = true; + return ({ result, update }) => { + console.log(result); if (result.type === "success") { + // invalidate(`document:${document.id}`); + update(result); dispatch("close"); - invalidate(`document:${document.id}`); } }; } @@ -91,7 +92,8 @@ Positioning and generating coordinates should happen outside of this form. type="submit" mode="danger" formaction={new URL("?/deleteAnnotation", canonical).href} - >{$_("annotate.delete")} + > + {$_("annotate.delete")} {/if} diff --git a/src/lib/utils/api.ts b/src/lib/utils/api.ts index 17bc1ef80..8358004eb 100644 --- a/src/lib/utils/api.ts +++ b/src/lib/utils/api.ts @@ -50,6 +50,12 @@ export async function getApiResponse( } // everything worked + + if (resp.status === 204) { + // deletes return nothing + return {}; + } + try { response.data = await resp.json(); } catch (e) { @@ -57,14 +63,14 @@ export async function getApiResponse( case "SyntaxError": // provide more specific error response.error = { status: 500, - message: "The API returned invalid JSON" - } + message: "The API returned invalid JSON", + }; break; default: // catch-all handling response.error = { status: 500, - message: String(e), - } + message: String(e), + }; } } return response; diff --git a/src/routes/(app)/documents/[id]-[slug]/+page.server.ts b/src/routes/(app)/documents/[id]-[slug]/+page.server.ts index 8d0859e2f..1b99292dd 100644 --- a/src/routes/(app)/documents/[id]-[slug]/+page.server.ts +++ b/src/routes/(app)/documents/[id]-[slug]/+page.server.ts @@ -43,17 +43,24 @@ export const actions = { return m; }, {}); - try { - const document = await edit(id, { data }, csrf_token, fetch); - return { - success: true, - document, - }; - } catch (error) { - console.error(`Data: ${url}`); - console.error(error); - return fail(400); + const { data: document, error } = await edit( + id, + { data }, + csrf_token, + fetch, + ); + + if (error) { + return fail(error.status, { + message: error.message, + error: error.errors, + }); } + + return { + success: true, + document, + }; }, async delete({ cookies, fetch, params }) { @@ -62,19 +69,11 @@ export const actions = { console.info(`Deleting document: ${id}`); - const resp = await destroy(id, csrf_token, fetch).catch((e) => { - console.error(e); - }); + // data will be null on success + const { error } = await destroy(id, csrf_token, fetch); - // probably the API is down - if (!resp) { - return fail(500, { error: "Something went wrong." }); - } - - // something else broke - if (isErrorCode(resp.status)) { - // {"error": "..."} - return fail(resp.status, await resp.json()); + if (error) { + return fail(error.status, { message: error.message }); } return redirect(302, "/documents/"); @@ -87,19 +86,24 @@ export const actions = { const update: Partial = Object.fromEntries(form); + console.log(update); + // noindex is a boolean so needs special treatment - update.noindex = form.get("noindex") === "checked"; - - try { - const document = await edit(id, update, csrf_token, fetch); - return { - success: true, - document, - }; - } catch (error) { - console.error(error); - return fail(400); + update.noindex = form.get("noindex") === "on"; + + const { data: document, error } = await edit(id, update, csrf_token, fetch); + + if (error) { + return fail(error.status, { + message: error.message, + errors: error.errors, + }); } + + return { + success: true, + document, + }; }, async redact({ cookies, fetch, request, params }) { @@ -118,7 +122,6 @@ export const actions = { // something else broke if (isErrorCode(resp.status)) { - // {"error": "..."} return fail(resp.status, await resp.json()); } @@ -145,15 +148,24 @@ export const actions = { y2, }; - try { - const created = await notes.create(params.id, note, csrf_token, fetch); - return { - success: true, - note: created, - }; - } catch (e) { - return fail(400, { error: e.message }); + const { data: created, error } = await notes.create( + params.id, + note, + csrf_token, + fetch, + ); + + if (error) { + return fail(error.status, { + message: error.message, + errors: error.errors, + }); } + + return { + success: true, + note: created, + }; }, async updateAnnotation({ cookies, request, fetch, params }) { @@ -169,21 +181,25 @@ export const actions = { access: form.get("access") as Access, }; - try { - const updated = await notes.update( - params.id, - note_id, - note, - csrf_token, - fetch, - ); - return { - success: true, - note: updated, - }; - } catch (e) { - return fail(400, { error: e.message }); + const { data: updated, error } = await notes.update( + params.id, + note_id, + note, + csrf_token, + fetch, + ); + + if (error) { + return fail(error.status, { + message: error.message, + errors: error.errors, + }); } + + return { + success: true, + note: updated, + }; }, async deleteAnnotation({ cookies, request, fetch, params }) { @@ -194,21 +210,10 @@ export const actions = { if (!note_id) return fail(400, { id: "missing" }); - const resp = await notes - .remove(params.id, note_id, csrf_token, fetch) - .catch((e) => { - console.error(e); - }); + const { error } = await notes.remove(params.id, note_id, csrf_token, fetch); - // probably the API is down - if (!resp) { - return fail(500, { error: "Something went wrong." }); - } - - // something else broke - if (isErrorCode(resp.status)) { - // {"error": "..."} - return fail(resp.status, await resp.json()); + if (error) { + return fail(error.status, { message: error.message }); } return { From 6e6c1e739d470370a782f0adf51329bfa737d2ba Mon Sep 17 00:00:00 2001 From: Chris Amico Date: Tue, 24 Sep 2024 13:13:39 -0400 Subject: [PATCH 06/12] Migrate projects --- src/lib/api/projects.ts | 148 +++++------------- src/lib/api/tests/projects.test.ts | 34 +++- src/lib/components/projects/ProjectPin.svelte | 20 ++- src/routes/(app)/documents/+layout.ts | 3 +- src/routes/(app)/documents/projects/+page.ts | 10 +- .../projects/[id]-[slug]/+page.server.ts | 114 ++++++-------- .../documents/projects/[id]-[slug]/+page.ts | 27 ++-- 7 files changed, 146 insertions(+), 210 deletions(-) diff --git a/src/lib/api/projects.ts b/src/lib/api/projects.ts index 7fdcaefe1..f15883113 100644 --- a/src/lib/api/projects.ts +++ b/src/lib/api/projects.ts @@ -1,77 +1,57 @@ // api methods for projects import type { Page } from "@/api/types"; import type { - APIError, + APIResponse, Document, Project, ProjectMembershipItem, ProjectResults, + ValidationError, } from "./types"; import { APP_URL, BASE_API_URL, CSRF_HEADER_NAME } from "@/config/config.js"; -import { getAll, isErrorCode } from "$lib/utils/api"; +import { getAll, getApiResponse, isErrorCode } from "$lib/utils/api"; /** * Get a single project - * - * @export */ export async function get( id: number, fetch = globalThis.fetch, -): Promise { +): Promise> { const endpoint = new URL(`projects/${id}/`, BASE_API_URL); - const resp = await fetch(endpoint, { credentials: "include" }).catch( - console.error, - ); - - if (!resp) { - throw new Error("API error"); - } + const resp = await fetch(endpoint, { credentials: "include" }); - if (isErrorCode(resp.status)) { - throw new Error(resp.statusText); - } - - return resp.json(); + return getApiResponse(resp); } /** * Get a page of projects - * - * @export */ export async function list( params: Record = {}, fetch = globalThis.fetch, -): Promise { +): Promise> { const endpoint = new URL("projects/", BASE_API_URL); for (const [k, v] of Object.entries(params)) { endpoint.searchParams.set(k, String(v)); } - const resp = await fetch(endpoint, { credentials: "include" }).catch( - console.error, - ); - - if (!resp) { - throw new Error("API error"); - } + const resp = await fetch(endpoint, { credentials: "include" }); - if (isErrorCode(resp.status)) { - throw new Error(resp.statusText); - } - - return resp.json(); + return getApiResponse(resp); } +/** + * Get all projects a user has access to -- owned or shared + */ export async function getForUser( userId: number, query?: string, fetch = globalThis.fetch, -) { +): Promise { const endpoint = new URL("projects/", BASE_API_URL); endpoint.searchParams.set("user", String(userId)); if (query) { @@ -114,7 +94,7 @@ export async function pinProject( pinned = true, csrf_token: string, fetch = globalThis.fetch, -): Promise { +) { const endpoint = new URL(`projects/${id}/`, BASE_API_URL); const options: RequestInit = { credentials: "include", @@ -129,26 +109,15 @@ export async function pinProject( const resp = await fetch(endpoint, { ...options, body: JSON.stringify({ pinned }), - }).catch(console.error); - - if (!resp) { - throw new Error("API error"); - } - - if (isErrorCode(resp.status)) { - throw new Error(resp.statusText); - } + }); - return resp.json(); + return getApiResponse(resp); } // writable methods + /** * Create a new project - * - * @param project - * @param csrf_token - * @param fetch */ export async function create( project: { @@ -159,7 +128,7 @@ export async function create( }, csrf_token: string, fetch = globalThis.fetch, -) { +): Promise> { const endpoint = new URL("projects/", BASE_API_URL); const resp = await fetch(endpoint, { body: JSON.stringify(project), @@ -170,18 +139,9 @@ export async function create( Referer: APP_URL, }, method: "POST", - }).catch(console.error); - - if (!resp) { - throw new Error("API unavailable"); - } - - if (isErrorCode(resp.status)) { - const { data } = await resp.json(); - throw new Error(data); - } + }); - return resp.json(); + return getApiResponse(resp); } export async function edit( @@ -189,7 +149,7 @@ export async function edit( data: Partial, csrf_token: string, fetch = globalThis.fetch, -) { +): Promise> { const endpoint = new URL(`projects/${project_id}/`, BASE_API_URL); const resp = await fetch(endpoint, { @@ -201,26 +161,13 @@ export async function edit( Referer: APP_URL, }, method: "PATCH", - }).catch(console.error); - - if (!resp) { - throw new Error("API unavailable"); - } - - if (isErrorCode(resp.status)) { - const { data } = await resp.json(); - throw new Error(data); - } + }); - return resp.json(); + return getApiResponse(resp); } /** * Delete a project. There is no undo. - * - * @param project_id - * @param csrf_token - * @param fetch */ export async function destroy( project_id: number, @@ -229,7 +176,7 @@ export async function destroy( ) { const endpoint = new URL(`projects/${project_id}/`, BASE_API_URL); - return fetch(endpoint, { + const resp = await fetch(endpoint, { credentials: "include", headers: { "Content-type": "application/json", @@ -238,6 +185,8 @@ export async function destroy( }, method: "DELETE", }); + + return getApiResponse(resp); } /** @@ -253,7 +202,7 @@ export async function add( documents: (string | number)[], csrf_token: string, fetch = globalThis.fetch, -): Promise { +): Promise> { const endpoint = new URL(`projects/${project_id}/documents/`, BASE_API_URL); const data = documents.map((document) => ({ document })); const resp = await fetch(endpoint, { @@ -265,32 +214,20 @@ export async function add( Referer: APP_URL, }, method: "POST", - }).catch(console.error); - - if (!resp) { - throw new Error("API unavailable"); - } - - // trying out some new error handling - if (isErrorCode(resp.status)) { - return { - error: { - status: resp.status, - message: resp.statusText, - ...(await resp.json()), - }, - }; - } + }); - return resp.json(); + return getApiResponse(resp); } +/** + * Remove documents from a project + */ export async function remove( project_id: number, documents: (string | number)[], csrf_token: string, fetch = globalThis.fetch, -): Promise { +) { const endpoint = new URL(`projects/${project_id}/documents/`, BASE_API_URL); endpoint.searchParams.set("document_id__in", documents.join(",")); @@ -302,24 +239,9 @@ export async function remove( Referer: APP_URL, }, method: "DELETE", - }).catch(console.error); - - if (!resp) { - throw new Error("API unavailable"); - } - - // trying out some new error handling - if (isErrorCode(resp.status)) { - return { - error: { - status: resp.status, - message: resp.statusText, - ...(await resp.json()), - }, - }; - } + }); - return null; + return getApiResponse(resp); } /** diff --git a/src/lib/api/tests/projects.test.ts b/src/lib/api/tests/projects.test.ts index 259db5556..a398e2a89 100644 --- a/src/lib/api/tests/projects.test.ts +++ b/src/lib/api/tests/projects.test.ts @@ -39,13 +39,14 @@ describe("projects.get", () => { vi.restoreAllMocks(); }); it("fetches a single project by ID", async () => { - let res = await projects.get(1, mockFetch); + const { data: res, error } = await projects.get(1, mockFetch); expect(mockFetch).toHaveBeenCalledWith( new URL(`${BASE_API_URL}projects/1/`), { credentials: "include", }, ); + expect(error).toBeUndefined(); expect(res).toBe(project); }); it("throws a 500 error if fetch fails", async () => { @@ -74,11 +75,13 @@ describe("projects.list", () => { vi.restoreAllMocks(); }); it("fetches a list of projects", async () => { - const res = await projects.list({}, mockFetch); + const { data: res, error } = await projects.list({}, mockFetch); + expect(mockFetch).toHaveBeenCalledWith( new URL("projects/", BASE_API_URL), expect.any(Object), ); + expect(error).toBeUndefined(); expect(res).toBe(projectList); }); it("attaches any params to the URL as searchParams", async () => { @@ -255,8 +258,13 @@ describe("project lifecycle", () => { pinned: project.pinned, }; - const created = await projects.create(data, "token", mockFetch); + const { data: created, error } = await projects.create( + data, + "token", + mockFetch, + ); + expect(error).toBeUndefined(); expect(created).toMatchObject(project); expect(mockFetch).toBeCalledWith(new URL("projects/", BASE_API_URL), { body: JSON.stringify(data), @@ -284,8 +292,14 @@ describe("project lifecycle", () => { const update: Partial = { title: "New title" }; - const updated = await projects.edit(project.id, update, "token", mockFetch); + const { data: updated, error } = await projects.edit( + project.id, + update, + "token", + mockFetch, + ); + expect(error).toBeUndefined(); expect(updated).toMatchObject({ ...project, ...update }); expect(mockFetch).toHaveBeenCalledWith( new URL(`projects/${project.id}/`, BASE_API_URL), @@ -310,9 +324,9 @@ describe("project lifecycle", () => { }; }); - const resp = await projects.destroy(project.id, "token", mockFetch); + const { error } = await projects.destroy(project.id, "token", mockFetch); - expect(resp.status).toEqual(204); + expect(error).toBeUndefined(); expect(mockFetch).toHaveBeenCalledWith( new URL(`projects/${project.id}/`, BASE_API_URL), { @@ -359,8 +373,14 @@ describe("manage project documents", () => { }); const ids = documents.results.map((d) => d.document as number); - const docs = await projects.add(1, ids, "token", mockFetch); + const { data: docs, error } = await projects.add( + 1, + ids, + "token", + mockFetch, + ); + expect(error).toBeUndefined(); expect(docs).toMatchObject(documents.results); expect(mockFetch).toBeCalledWith( new URL("projects/1/documents/", BASE_API_URL), diff --git a/src/lib/components/projects/ProjectPin.svelte b/src/lib/components/projects/ProjectPin.svelte index 1c37af20d..63d2531f5 100644 --- a/src/lib/components/projects/ProjectPin.svelte +++ b/src/lib/components/projects/ProjectPin.svelte @@ -30,15 +30,19 @@ e.preventDefault(); const csrf_token = getCsrfToken(); const newPinnedState = !project.pinned; - try { - // optimistic update - project.pinned = newPinnedState; - project = await pinProject(project.id, newPinnedState, csrf_token); - await invalidate(canonicalUrl(project)); - } catch (e) { - // undo optimistic update on error + let error: unknown; + + ({ data: project, error } = await pinProject( + project.id, + newPinnedState, + csrf_token, + )); + + if (error) { project.pinned = !project.pinned; - console.error(e); + console.error(error); + } else { + await invalidate(canonicalUrl(project)); } // now that we've updated, set $pinned diff --git a/src/routes/(app)/documents/+layout.ts b/src/routes/(app)/documents/+layout.ts index 137cbdf06..f902183ba 100644 --- a/src/routes/(app)/documents/+layout.ts +++ b/src/routes/(app)/documents/+layout.ts @@ -6,8 +6,7 @@ export async function load({ fetch, parent }) { const pinnedAddons = getPinnedAddons(fetch).catch(console.error); const pinnedProjects = projects .list({ pinned: true }, fetch) - .then((r) => r.results) - .catch(console.error); + .then((r) => r.data?.results); const breadcrumbs = await breadcrumbTrail(parent, [ // { href: "/documents", title: "Documents" }, // TODO: move document manager to `/documents` route diff --git a/src/routes/(app)/documents/projects/+page.ts b/src/routes/(app)/documents/projects/+page.ts index 3a66b0336..ce36f1c5d 100644 --- a/src/routes/(app)/documents/projects/+page.ts +++ b/src/routes/(app)/documents/projects/+page.ts @@ -26,10 +26,13 @@ export async function load({ url, parent, fetch }) { previous: null, }; + let error: unknown; + // for owned and shared, we just get everything at once if (me && filter === "owned") { projects.results = await getOwned(me.id, query, fetch).catch((e) => { console.error(e); + error = e; return []; }); } @@ -37,19 +40,18 @@ export async function load({ url, parent, fetch }) { if (me && filter === "shared") { projects.results = await getShared(me.id, query, fetch).catch((e) => { console.error(e); + error = e; return []; }); } // for public, we get the first page and paginate if (filter === "public") { - projects = await list(params, fetch).catch((e) => { - console.error(e); - return projects; - }); + ({ data: projects, error } = await list(params, fetch)); } return { + error, list: filter, projects, query, diff --git a/src/routes/(app)/documents/projects/[id]-[slug]/+page.server.ts b/src/routes/(app)/documents/projects/[id]-[slug]/+page.server.ts index 04ff229d7..a8d141e84 100644 --- a/src/routes/(app)/documents/projects/[id]-[slug]/+page.server.ts +++ b/src/routes/(app)/documents/projects/[id]-[slug]/+page.server.ts @@ -11,19 +11,13 @@ export const actions = { const csrf_token = cookies.get(CSRF_COOKIE_NAME); const project_id = +params.id; - const resp = await projects - .destroy(project_id, csrf_token, fetch) - .catch(console.error); + const { error } = await projects.destroy(project_id, csrf_token, fetch); - if (!resp) { - return fail(500, { message: "API error" }); + if (error) { + return fail(error.status, { message: error.message }); } - if (resp.status === 204) { - return redirect(302, "/documents/projects/"); - } - - return fail(resp.status, { error: await resp.json() }); + return redirect(302, "/documents/projects/"); }, async edit({ cookies, request, fetch, params }) { @@ -37,17 +31,18 @@ export const actions = { private: form.get("private") === "on", }; - try { - const updated = await projects.edit( - +params.id, - update, - csrf_token, - fetch, - ); - return { success: true, project: updated }; - } catch (error) { - return fail(400, { error }); + const { data: updated, error } = await projects.edit( + +params.id, + update, + csrf_token, + fetch, + ); + + if (error) { + return fail(error.status, { message: error.message }); } + + return { success: true, project: updated }; }, async users({ request, cookies, params, fetch }) { @@ -58,33 +53,29 @@ export const actions = { const users = form.getAll("user").map(Number); const access = form.getAll("access"); - try { - // batch update - const updated = await Promise.all( - users.map((u, i) => { - const a = access[i]; - if (a === "remove") { - // this returns an empty body, so just fire and forget - collaborators.remove(project_id, u, csrf_token, fetch); - } else { - return collaborators.update( - project_id, - u, - a as ProjectAccess, - csrf_token, - fetch, - ); - } - }), - ); - return { - success: true, - users: updated, - }; - } catch (error) { - console.error(error); - return fail(400, { error: error.toString() }); - } + // batch update + const updated = await Promise.all( + users.map((u, i) => { + const a = access[i]; + if (a === "remove") { + // this returns an empty body, so just fire and forget + collaborators.remove(project_id, u, csrf_token, fetch); + } else { + return collaborators.update( + project_id, + u, + a as ProjectAccess, + csrf_token, + fetch, + ); + } + }), + ); + + return { + success: true, + users: updated, + }; }, async invite({ request, cookies, params, fetch }) { @@ -94,21 +85,20 @@ export const actions = { const email = form.get("email") as string; const access = form.get("access") as ProjectAccess; - try { - const user = await collaborators.add( - +params.id, - { email, access }, - csrf_token, - fetch, - ); - - return { - success: true, - user, - }; - } catch (error) { - console.error(error); - return fail(400); + const { data: user, error } = await collaborators.add( + +params.id, + { email, access }, + csrf_token, + fetch, + ); + + if (error) { + return fail(error.status, { ...error }); } + + return { + success: true, + user, + }; }, } satisfies Actions; diff --git a/src/routes/(app)/documents/projects/[id]-[slug]/+page.ts b/src/routes/(app)/documents/projects/[id]-[slug]/+page.ts index af3e6d814..0d316d993 100644 --- a/src/routes/(app)/documents/projects/[id]-[slug]/+page.ts +++ b/src/routes/(app)/documents/projects/[id]-[slug]/+page.ts @@ -7,25 +7,24 @@ import { search } from "$lib/api/documents"; import { breadcrumbTrail } from "$lib/utils/navigation"; export async function load({ params, url, parent, fetch }) { - const id = parseInt(params.id); + const id = parseInt(params.id, 10); + const [project, users] = await Promise.all([ - projects.get(id, fetch).catch(console.error), - collaborators.list(id, fetch).catch(console.error), - ]).catch((e) => { - return []; - }); - - if (!project) { - error(404, "Project not found"); + projects.get(id, fetch), + collaborators.list(id, fetch), + ]); + + if (project.error) { + error(project.error.status, project.error.message); } - if (project.slug !== params.slug) { - const canonical = projects.canonicalUrl(project); + if (project.data.slug !== params.slug) { + const canonical = projects.canonicalUrl(project.data); redirect(302, canonical); } const breadcrumbs = await breadcrumbTrail(parent, [ - { href: url.pathname, title: project.title }, + { href: url.pathname, title: project.data.title }, ]); const query = url.searchParams.get("q") ?? ""; @@ -33,7 +32,7 @@ export async function load({ params, url, parent, fetch }) { const per_page = +url.searchParams.get("per_page") || DEFAULT_PER_PAGE; const documents = search( query, - { cursor, project: project.id, per_page }, + { cursor, project: project.data.id, per_page }, fetch, ); @@ -41,7 +40,7 @@ export async function load({ params, url, parent, fetch }) { breadcrumbs, documents, query, - project, + project: project.data, users, }; } From 9f29922d6d337d208d7a168704dbda7a8ec0944c Mon Sep 17 00:00:00 2001 From: Chris Amico Date: Tue, 24 Sep 2024 14:21:41 -0400 Subject: [PATCH 07/12] Migrate sections --- src/lib/api/sections.ts | 68 ++++++------------- src/lib/api/tests/sections.test.ts | 10 +-- .../components/forms/EditSectionRow.svelte | 7 +- src/routes/(app)/documents/+layout.ts | 2 +- 4 files changed, 30 insertions(+), 57 deletions(-) diff --git a/src/lib/api/sections.ts b/src/lib/api/sections.ts index 1abe30cf2..25a593713 100644 --- a/src/lib/api/sections.ts +++ b/src/lib/api/sections.ts @@ -1,13 +1,18 @@ -import type { Section, SectionResults } from "./types"; +import type { + APIResponse, + Section, + SectionResults, + ValidationError, +} from "./types"; import { APP_URL, BASE_API_URL, CSRF_HEADER_NAME } from "@/config/config.js"; +import { getApiResponse } from "../utils"; /** * Load sections from a single document from the API * Example: https://api.www.documentcloud.org/api/documents/24028981/sections/ * - * @async - * @export + * @deprecated */ export async function list( doc_id: string | number, @@ -34,14 +39,13 @@ export async function list( * Load a single section from a single document from the API * Example: https://api.www.documentcloud.org/api/documents/24028981/sections/33933/ * - * @async - * @export + * @deprecated */ export async function get( doc_id: string | number, section_id: number, fetch = globalThis.fetch, -): Promise
{ +) { const endpoint = new URL( `documents/${doc_id}/sections/${section_id}.json`, BASE_API_URL, @@ -66,19 +70,13 @@ export async function get( /** * Add a section to a document - * - * @param doc_id Document ID - * @param section data - * @param csrf_token - * @param fetch - * @returns {Section} */ export async function create( doc_id: string | number, section: { page_number: number; title: string }, csrf_token: string, fetch = globalThis.fetch, -): Promise
{ +): Promise> { const endpoint = new URL(`documents/${doc_id}/sections/`, BASE_API_URL); const resp = await fetch(endpoint, { @@ -90,28 +88,13 @@ export async function create( Referer: APP_URL, }, method: "POST", - }).catch(console.error); - - if (!resp) { - throw new Error("API error"); - } - - if (!resp.ok) { - throw new Error(resp.statusText); - } + }); - return resp.json(); + return getApiResponse(resp); } /** * Update a section on a document - * - * @param doc_id Document ID - * @param section_id Section ID - * @param section data - * @param csrf_token - * @param fetch - * @returns {Section} */ export async function update( doc_id: string | number, @@ -119,7 +102,7 @@ export async function update( section: { page_number?: number; title?: string }, csrf_token: string, fetch = globalThis.fetch, -): Promise
{ +): Promise> { const endpoint = new URL( `documents/${doc_id}/sections/${section_id}/`, BASE_API_URL, @@ -134,39 +117,26 @@ export async function update( Referer: APP_URL, }, method: "PATCH", - }).catch(console.error); - - if (!resp) { - throw new Error("API error"); - } - - if (!resp.ok) { - throw new Error(resp.statusText); - } + }); - return resp.json(); + return getApiResponse(resp); } /** * Delete a section from a document. - * - * @param doc_id Document ID - * @param section_id Section ID - * @param csrf_token - * @param fetch */ export async function remove( doc_id: string | number, section_id: string | number, csrf_token: string, fetch = globalThis.fetch, -): Promise { +): Promise> { const endpoint = new URL( `documents/${doc_id}/sections/${section_id}/`, BASE_API_URL, ); - return fetch(endpoint, { + const resp = await fetch(endpoint, { credentials: "include", method: "DELETE", headers: { @@ -175,4 +145,6 @@ export async function remove( Referer: APP_URL, }, }); + + return getApiResponse(resp); } diff --git a/src/lib/api/tests/sections.test.ts b/src/lib/api/tests/sections.test.ts index ac68600d5..5d4d3718b 100644 --- a/src/lib/api/tests/sections.test.ts +++ b/src/lib/api/tests/sections.test.ts @@ -50,13 +50,14 @@ describe("sections: writing", () => { }; }); - const result = await sections.create( + const { data: result, error } = await sections.create( document.id, { page_number: 1, title: "Title" }, "token", mockFetch, ); + expect(error).toBeUndefined(); expect(result).toStrictEqual(section); expect(mockFetch).toBeCalledWith( new URL(`documents/${document.id}/sections/`, BASE_API_URL), @@ -91,7 +92,7 @@ describe("sections: writing", () => { title: "New title", }; - const result = await sections.update( + const { data: result, error } = await sections.update( document.id, section.id, update, @@ -99,6 +100,7 @@ describe("sections: writing", () => { mockFetch, ); + expect(error).toBeUndefined(); expect(result).toMatchObject({ ...section, ...update }); expect(mockFetch).toBeCalledWith( new URL(`documents/${document.id}/sections/${section.id}/`, BASE_API_URL), @@ -124,14 +126,14 @@ describe("sections: writing", () => { }; }); - const resp = await sections.remove( + const { error } = await sections.remove( document.id, section.id, "token", mockFetch, ); - expect(resp.status).toStrictEqual(204); + expect(error).toBeUndefined(); expect(mockFetch).toBeCalledWith( new URL(`documents/${document.id}/sections/${section.id}/`, BASE_API_URL), { diff --git a/src/lib/components/forms/EditSectionRow.svelte b/src/lib/components/forms/EditSectionRow.svelte index 102c2900d..a3b7ab5c0 100644 --- a/src/lib/components/forms/EditSectionRow.svelte +++ b/src/lib/components/forms/EditSectionRow.svelte @@ -2,7 +2,7 @@ One row of the `EditSections.svelte` form, to encapsulate logic. --> From b6a33ab888534ac7aa1f93d88639f1c1a8b9728b Mon Sep 17 00:00:00 2001 From: Chris Amico Date: Tue, 24 Sep 2024 21:40:54 -0400 Subject: [PATCH 09/12] finish it out --- src/lib/api/documents.ts | 18 ++++----- src/lib/api/tests/documents.test.ts | 4 +- .../components/forms/DocumentUpload.svelte | 26 +++++-------- .../components/layouts/DocumentLayout.svelte | 10 ----- .../stories/SidebarLayout.stories.svelte | 4 +- .../add-ons/[owner]/[repo]/+page.server.ts | 18 +++------ .../[owner]/[repo]/[event]/+page.server.ts | 23 ++++++------ src/routes/(app)/documents/+page.server.ts | 37 ++++++++----------- src/routes/(app)/documents/+page.ts | 10 +---- 9 files changed, 56 insertions(+), 94 deletions(-) diff --git a/src/lib/api/documents.ts b/src/lib/api/documents.ts index cfe707105..de962bc39 100644 --- a/src/lib/api/documents.ts +++ b/src/lib/api/documents.ts @@ -184,7 +184,7 @@ export async function create( Referer: APP_URL, }, body: JSON.stringify(documents), - }); + }).catch(console.error); return getApiResponse(resp); } @@ -297,7 +297,7 @@ export async function destroy_many( ids: (string | number)[], csrf_token: string, fetch = globalThis.fetch, -): Promise> { +): Promise> { const endpoint = new URL(`documents/`, BASE_API_URL); endpoint.searchParams.set("id__in", ids.join(",")); @@ -310,7 +310,7 @@ export async function destroy_many( }, }).catch(console.log); - return getApiResponse(resp); + return getApiResponse(resp); } /** @@ -348,22 +348,18 @@ export async function edit( * Bulk edit top-level fields of an array of documents. * Each document *must* have an `id` property. * - * This returns the response directly, since a successful update - * will result in invalidation and refetching data. - * * @param documents * @param csrf_token * @param fetch - * @returns {Promise} */ export async function edit_many( documents: Partial[], csrf_token: string, fetch = globalThis.fetch, -): Promise { +) { const endpoint = new URL("documents/", BASE_API_URL); - return fetch(endpoint, { + const resp = await fetch(endpoint, { credentials: "include", method: "PATCH", headers: { @@ -372,7 +368,9 @@ export async function edit_many( Referer: APP_URL, }, body: JSON.stringify(documents), - }); + }).catch(console.error); + + return getApiResponse(resp); } /** diff --git a/src/lib/api/tests/documents.test.ts b/src/lib/api/tests/documents.test.ts index fda238b98..8d87f964e 100644 --- a/src/lib/api/tests/documents.test.ts +++ b/src/lib/api/tests/documents.test.ts @@ -504,9 +504,9 @@ describe("document write methods", () => { }); const update = docs.results.map((d) => ({ ...d, source: "New source" })); - const resp = await documents.edit_many(update, "token", mockFetch); + const { error } = await documents.edit_many(update, "token", mockFetch); - expect(resp.status).toEqual(200); + expect(error).toBeUndefined(); expect(mockFetch).toHaveBeenCalledWith( new URL("documents/", BASE_API_URL), diff --git a/src/lib/components/forms/DocumentUpload.svelte b/src/lib/components/forms/DocumentUpload.svelte index 4eb4c1793..49910463e 100644 --- a/src/lib/components/forms/DocumentUpload.svelte +++ b/src/lib/components/forms/DocumentUpload.svelte @@ -56,16 +56,11 @@ }; }); - let created: Document[]; - try { - created = await documents.create(docs, csrf_token, fetch); - } catch (err) { - return { - type: "error", - status: 400, - error: err, - }; - } + let { data: created, error } = await documents.create( + docs, + csrf_token, + fetch, + ); // upload const uploads = created.map((d, i) => @@ -89,7 +84,7 @@ ); // todo: i18n - if (process_response.ok) { + if (!process_response.error) { const query = new URLSearchParams([["q", userDocs(user, access)]]); return { type: "redirect", @@ -100,8 +95,8 @@ return { type: "error", - status: process_response.status, - error: await process_response.json(), + status: process_response.error.status, + error: process_response.error.errors, }; } @@ -109,7 +104,6 @@ @@ -46,7 +48,7 @@ - +