diff --git a/.agents/docs/architecture/auth-and-permissions.md b/.agents/docs/architecture/auth-and-permissions.md index 90b153eec..fc5a017fe 100644 --- a/.agents/docs/architecture/auth-and-permissions.md +++ b/.agents/docs/architecture/auth-and-permissions.md @@ -27,22 +27,22 @@ Routes are URI-versioned (`version: '1'` in `src/main.ts`), so paths are `/v1/.. The full inventory of non-ordinary access declarations, current as of writing: -| Route | Declaration | Why it is safe | -| ---------------------------------------------------------------- | --------------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | -| `POST /v1/auth/login` | `'public'` | `@ThrottleLoginRequest()`; credentials checked in `AuthService.login`. | -| `GET /v1/setup` | `'public'` | Returns only `SetupState` (branding, flags, release, uptime). | -| `POST /v1/setup` | `'public'` | **`SetupService.initApp` drops the whole database.** Its only protection is `if (savedOptions?.isSetup && !isDev) throw new ForbiddenException()` — an initialised production instance refuses, a development instance never does. | -| `DELETE /v1/setup` | `{ action: 'delete', subject: 'all' }` | Also refuses unless `NODE_ENV === 'test'`. | -| `PATCH /v1/setup` | `ADMIN_ONLY` | Only `ADMIN` gets `manage all`. | -| `GET /v1/audit/logs` | `ADMIN_ONLY` | `AuditService.find` is deliberately unscoped; the guard is the whole check. | -| `GET /v1/gateway/healthcheck` | `[]` | Any login token; an instrument token is refused. Module only loads when `GATEWAY_ENABLED`. | -| `POST /v1/groups` | `ADMIN_ONLY` | `ADMIN` alone. `create Group` admitted every `GROUP_MANAGER`: their `manage Group` rule is conditioned on their own groups, and this check sees only the subject type (#1468). | -| `POST /v1/instruments` | `{ action: 'manage', subject: 'Instrument' }` | No base permission level grants `manage Instrument`, so this is `ADMIN`-only in practice. Also `@AcceptsInstrumentToken()`: the only route the playground's minted token reaches. | -| `GET /v1/instruments/series`, `PATCH /v1/instruments/series/:id` | `ADMIN_ONLY` | `ADMIN` alone. `InstrumentsService.findSeriesOverview` lists every group's series, deliberately not narrowed to the caller's groups (an administrator belongs to none), so the guard is what keeps it from a `GROUP_MANAGER`, who holds `read Instrument`. Archiving is likewise administrators' alone; group managers keep deleting their own unused series. | -| `PATCH /v1/users/self-update/:id` | `{ action: 'read', subject: 'User' }` | Deliberately weak; `UsersService.updateSelfById` throws `ForbiddenException` unless `id === currentUser.id`. The controller carries a comment saying so. | -| `PUT /v1/users/:id/permissions` | `ADMIN_ONLY` | `ADMIN` alone. An `update User` grant is one of the things this route hands out, so it must not be enough to reach it, or the holder could grant themselves `manage all`. `$UpdateUserData` no longer carries the field either. | -| `POST /v1/users`, `PATCH /v1/users/:id`, `DELETE /v1/users/:id` | `ADMIN_ONLY` | `ADMIN` alone, whatever `User` action a grant names. Level, groups and password are what the rest of a user's access derives from, so a grantee could otherwise promote themselves, join every group, or log in as an admin whose password they set. `UsersService` also refuses an admin deleting, disabling or demoting their own account, so the last one cannot lock every admin-only route. | -| `GET /v1/summary` | five-element array (`read` on `Instrument`, `InstrumentRecord`, `Session`, `Subject`, `User`) | The only use of the multi-element array form; all five must pass. | +| Route | Declaration | Why it is safe | +| ------------------------------------------------------------------------------------------------------- | --------------------------------------------------------------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `POST /v1/auth/login` | `'public'` | `@ThrottleLoginRequest()`; credentials checked in `AuthService.login`. | +| `GET /v1/setup` | `'public'` | Returns only `SetupState` (branding, flags, release, uptime). | +| `POST /v1/setup` | `'public'` | **`SetupService.initApp` drops the whole database.** Its only protection is `if (savedOptions?.isSetup && !isDev) throw new ForbiddenException()` — an initialised production instance refuses, a development instance never does. | +| `DELETE /v1/setup` | `{ action: 'delete', subject: 'all' }` | Also refuses unless `NODE_ENV === 'test'`. | +| `PATCH /v1/setup` | `ADMIN_ONLY` | Only `ADMIN` gets `manage all`. | +| `GET /v1/audit/logs` | `ADMIN_ONLY` | `AuditService.find` is deliberately unscoped; the guard is the whole check. | +| `GET /v1/gateway/healthcheck` | `[]` | Any login token; an instrument token is refused. Module only loads when `GATEWAY_ENABLED`. | +| `POST /v1/groups` | `ADMIN_ONLY` | `ADMIN` alone. `create Group` admitted every `GROUP_MANAGER`: their `manage Group` rule is conditioned on their own groups, and this check sees only the subject type (#1468). | +| `POST /v1/instruments` | `{ action: 'manage', subject: 'Instrument' }` | No base permission level grants `manage Instrument`, so this is `ADMIN`-only in practice. Also `@AcceptsInstrumentToken()`: the only route the playground's minted token reaches. | +| `GET /v1/instruments/series`, `PATCH /v1/instruments/series/:id` | `ADMIN_ONLY` | `ADMIN` alone. `InstrumentsService.findSeriesOverview` lists every group's series, deliberately not narrowed to the caller's groups (an administrator belongs to none), so the guard is what keeps it from a `GROUP_MANAGER`, who holds `read Instrument`. Archiving is likewise administrators' alone; group managers keep deleting their own unused series. | +| `PATCH /v1/users/self-update/:id` | `{ action: 'read', subject: 'User' }` | Deliberately weak; `UsersService.updateSelfById` throws `ForbiddenException` unless `id === currentUser.id`. The controller carries a comment saying so. | +| `PUT /v1/users/:id/permissions` | `ADMIN_ONLY` | `ADMIN` alone. An `update User` grant is one of the things this route hands out, so it must not be enough to reach it, or the holder could grant themselves `manage all`. `$UpdateUserData` no longer carries the field either. | +| `POST /v1/users`, `PATCH /v1/users/:id`, `PATCH /v1/users/:id/archive`, `PATCH /v1/users/:id/unarchive` | `ADMIN_ONLY` | `ADMIN` alone, whatever `User` action a grant names. Level, groups and password are what the rest of a user's access derives from, so a grantee could otherwise promote themselves, join every group, or log in as an admin whose password they set. `UsersService` also refuses an admin archiving, unarchiving, disabling or demoting their own account, so the last one cannot lock every admin-only route. | +| `GET /v1/summary` | five-element array (`read` on `Instrument`, `InstrumentRecord`, `Session`, `Subject`, `User`) | The only use of the multi-element array form; all five must pass. | Adding a fourth `'public'` route, or a second `[]`, is a security decision — raise it rather than deciding alone. diff --git a/.agents/docs/architecture/instrument-pipeline.md b/.agents/docs/architecture/instrument-pipeline.md index bbcce77ad..6f0c79ae3 100644 --- a/.agents/docs/architecture/instrument-pipeline.md +++ b/.agents/docs/architecture/instrument-pipeline.md @@ -36,7 +36,7 @@ import { createRoot } from '/runtime/v1/react-dom@19.x/client.js'; These resolve differently on each tier and are covered in `.agents/docs/architecture/runtime-and-vendor.md`. Sibling files use ordinary relative imports -(`'./styles.css'`, `'./StroopTask.tsx'`). +(`'./styles.css'`, `'./Component.tsx'`). Three places produce instrument source: diff --git a/.agents/docs/architecture/runtime-and-vendor.md b/.agents/docs/architecture/runtime-and-vendor.md index 53d2a885b..4168d98f2 100644 --- a/.agents/docs/architecture/runtime-and-vendor.md +++ b/.agents/docs/architecture/runtime-and-vendor.md @@ -3,7 +3,7 @@ Six workspaces have `runtime` in the name and they do different jobs. This file tells them apart, explains the three unrelated ways `runtime/v1` is spelled in an import, and documents `vendor/`. -**`runtime/v1/dist` is gitignored and nothing works until it is built.** Nine `tsconfig.json` files +**`runtime/v1/dist` is gitignored and nothing works until it is built.** Eight `tsconfig.json` files map a path into it, so `pnpm lint` fails across most of the repo on a cold checkout. Build it with `pnpm --filter @opendatacapture/runtime-v1 build`, or just `pnpm build` (turbo's `^build` ordering gets there). @@ -59,7 +59,7 @@ path**, which only resolves because they are copied byte-for-byte (see ### `packages/runtime-bundler` Ships TypeScript source directly (`exports: "./src/index.ts"`, `bin` → `src/cli.ts` under tsx). The -CLI reads `runtime.config.js` from the working directory, validates it with `$UserConfigs`, and for +CLI reads `runtime.config.js` from the working directory, validates it with `$Config`, and for each `include` entry resolves `/package.json` through a `createRequire` rooted at the config file. **An entry in `include` must therefore also be a dependency of the workspace holding the config.** @@ -120,7 +120,7 @@ The same artifact is addressed three ways. They are not interchangeable. | Spelling | Resolved by | Declared in | | ----------------------------- | --------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| `/runtime/v1/` | the browser, over HTTP | served by `vite-plugin-runtime` (dev) or the copied `dist/runtime/v1` (prod); type-checked via `paths` in eight `tsconfig.json` files; marked external by `packages/instrument-bundler/src/plugin.ts` | +| `/runtime/v1/` | the browser, over HTTP | served by `vite-plugin-runtime` (dev) or the copied `dist/runtime/v1` (prod); type-checked via `paths` in seven `tsconfig.json` files; marked external by `packages/instrument-bundler/src/plugin.ts` | | `#runtime/v1/*` | Node subpath imports | `apps/api/package.json` `imports` **and** `apps/api/tsconfig.json` `paths` | | `@opendatacapture/runtime-v1` | ordinary package resolution | a `dependencies` entry, used to locate `dist` on disk | @@ -195,6 +195,6 @@ The value of an entry in a wrapper's `exports` decides how it is built. See | A vendor directory name `@` ↔ its `package.json` `name` `__` | wrong output URL; `vendor-pairing.test.ts` cannot find the paired wrapper | | A wrapper's sibling-wrapper dependency ↔ its pinned real peer | duplicate library instances at runtime; caught by `vendor-pairing.test.ts` | | `packages/runtime-core/package.json` `exports` ↔ what its three build stages emit | a subpath resolves to a file that no longer exists | -| The eight `tsconfig.json` files mapping `/runtime/v1/*` | that workspace stops type-checking instrument imports | +| Every `tsconfig.json` mapping `/runtime/v1/*` | that workspace stops type-checking instrument imports | | `jsxImportSource: '/runtime/v1/react@19.x'` in `packages/instrument-library/tsconfig.json` and `apps/playground/src/components/Editor/setup.ts` ↔ the react a bundle is built against, which `packages/instrument-bundler/src/build.ts` derives per instrument | the editor or `tsc` type-checks against a different React than the bundle uses | | `src/index.js` ↔ `src/index.d.ts` in `runtime-internal` and `runtime-meta` | hand-written declarations; nothing checks them against the implementation | diff --git a/.agents/docs/architecture/testing-strategy.md b/.agents/docs/architecture/testing-strategy.md index 6a4768b8c..8be9ddc7e 100644 --- a/.agents/docs/architecture/testing-strategy.md +++ b/.agents/docs/architecture/testing-strategy.md @@ -121,8 +121,8 @@ Touching any of them invalidates the cache for every task in the repo. `coverage.thresholds: { 100: true }`, so the run fails when statements, branches, functions or lines drop below 100%. The scope is its `coverage.include` — `apps/{api,gateway,web}/src` and `packages/*/src`; `runtime/**` and `apps/playground` are outside it — minus `coverage.exclude`, -which lists only non-code: test fixtures and helpers (`__tests__`, `__mocks__`, -`apps/web/src/testing`), the generated `route-tree.ts`, and the bare entrypoints. +which lists only non-code: test fixtures and helpers (`__tests__`, `apps/web/src/testing`), the +generated `route-tree.ts`, and the bare entrypoints. - **A line no test can reach is dead code.** Prove it (the types, every caller, the library's contract) and delete it, or restructure so the impossible state cannot be written — e.g. ts-pattern @@ -186,9 +186,5 @@ Flag these rather than fixing them in passing — each one is load-bearing somew pnpm location; `@opendatacapture/api#db:generate` is therefore declared `cache: false` so a cache hit never restores an empty output set. Changing either half changes what a `db:generate` run produces. -- `packages/instrument-bundler/vitest.config.ts` aliases `/runtime/v1` to - `packages/instrument-bundler/runtime/v1/dist`, a directory that does not exist. It is inert today: - the fixtures under `src/__tests__/repositories/` are read as raw strings and handed to the bundler, - never resolved by vitest. Do not rely on the alias. - Root `test` is `env-cmd vitest` but `test:coverage` is `vitest --coverage` with no `env-cmd`, so the two do not run under the same environment. diff --git a/.agents/docs/packages/libjs.md b/.agents/docs/packages/libjs.md index d17b4703d..9d72e0dab 100644 --- a/.agents/docs/packages/libjs.md +++ b/.agents/docs/packages/libjs.md @@ -2,7 +2,7 @@ A collection of utility functions and types for Node.js and the browser. -**Status in Open Data Capture:** the most widely spread DNP package here — imported in ~40 files across `apps/api`, `apps/web`, `apps/playground`, `packages/demo`, `packages/instrument-utils`, `packages/react-core`, `packages/runtime-bundler`, `packages/schemas`, `packages/serve-instrument`, and `testing`. (`packages/instrument-stubs` and `packages/vite-plugin-runtime` also declare it, with no current imports.) +**Status in Open Data Capture:** the most widely spread DNP package here — imported in ~40 files across `apps/api`, `apps/web`, `apps/playground`, `packages/demo`, `packages/instrument-stubs`, `packages/instrument-utils`, `packages/react-core`, `packages/runtime-bundler`, `packages/schemas`, `packages/serve-instrument`, and `testing`. Single root export; there are no subpaths. diff --git a/.agents/docs/packages/libui.md b/.agents/docs/packages/libui.md index 40e6719e3..57bd60aa2 100644 --- a/.agents/docs/packages/libui.md +++ b/.agents/docs/packages/libui.md @@ -2,7 +2,7 @@ Generic UI components for DNP projects, built with React and Tailwind CSS. -**Status in Open Data Capture:** used extensively — ~194 files, concentrated in `apps/web` (105) and `apps/playground` (52), plus `packages/react-core` (31), `apps/gateway`, `apps/outreach`, and `packages/serve-instrument`. It is the primary source of UI components, hooks, i18n, and theming for every frontend surface. (`storybook` declares it as a dependency but imports it only through the components it renders.) +**Status in Open Data Capture:** used extensively — ~194 files, concentrated in `apps/web` (105) and `apps/playground` (52), plus `packages/react-core` (31), `apps/gateway`, `apps/outreach`, and `packages/serve-instrument`. It is the primary source of UI components, hooks, i18n, and theming for every frontend surface. There is no root `.` export — always import from a subpath. @@ -102,7 +102,7 @@ ls apps/web/node_modules/@douglasneuroinformatics/libui/src/hooks cat apps/web/node_modules/@douglasneuroinformatics/libui/src/utils/index.ts ``` -Also resolvable from `apps/gateway`, `apps/playground`, `apps/outreach`, `packages/react-core`, `packages/serve-instrument`, and `storybook`. +Also resolvable from `apps/gateway`, `apps/playground`, `apps/outreach`, `packages/react-core`, and `packages/serve-instrument`. ## Docs diff --git a/.agents/docs/playbooks/add-e2e-test.md b/.agents/docs/playbooks/add-e2e-test.md index b94606029..9b260e7b2 100644 --- a/.agents/docs/playbooks/add-e2e-test.md +++ b/.agents/docs/playbooks/add-e2e-test.md @@ -32,10 +32,11 @@ Mongo replica set. (`testing/src/pages/auth/login.page.ts` is the example). Read `testing/src/pages/_app/session/start-session.page.ts` for the shape. -4. **Register it in the `pageModels` map in `testing/src/support/fixtures.ts`**, keyed by the exact - route literal — e.g. `'/datahub/$subjectId/table': SubjectDataTablePage`. The map is - `satisfies { [K in RouteTo]?: any }`, so a key that is not a real route fails `tsc`. A page object - missing from the map is unreachable from a spec. +4. **Register it in the `pageModels` map in `testing/src/support/fixtures.ts`** when the spec + navigates to it with `getPageModel`, keyed by the exact route literal — e.g. + `'/datahub/$subjectId/assignments': SubjectAssignmentsPage`. The map is + `satisfies { [K in RouteTo]?: any }`, so a key that is not a real route fails `tsc`. A page the + spec reaches by clicking through the app needs no entry; construct it with `new XPage(page)`. 5. **Write the spec** at `testing/src/specs/.spec.ts`, importing from the fixtures module: @@ -80,6 +81,3 @@ pnpm --filter @opendatacapture/testing test:e2e src/specs/.spec.ts # one pnpm --filter @opendatacapture/testing test:dev # Playwright UI mode pnpm test:e2e # whole suite, from repo root ``` - -Ignore the `test:chrome` script: its `--project='*Desktop Chrome'` matches none of the four project -names and errors immediately. diff --git a/.agents/docs/playbooks/add-env-var.md b/.agents/docs/playbooks/add-env-var.md index 57dac99d7..b70d82180 100644 --- a/.agents/docs/playbooks/add-env-var.md +++ b/.agents/docs/playbooks/add-env-var.md @@ -55,9 +55,11 @@ below in order; each skipped one type-checks and compiles, then produces `undefi `env-cmd -f ../../.env`, which is why only build-time reads need this step. 8. **If the browser needs it, three files in `apps/web` must agree:** `apps/web/.env.public` (the - manifest `@import-meta-env/unplugin` and the `inject`/`start` scripts read — values are substituted - into `dist/index.html` after the build, not baked in), the `ImportMetaEnv` interface in - `apps/web/src/vite-env.d.ts`, and the parsed `config` object in `apps/web/src/config.ts`. + manifest `@import-meta-env/unplugin`, the `start` script and the web Docker image's + `import-meta-env` read — values are substituted into `dist/index.html` after the build, not baked + in), the `ImportMetaEnv` interface in `apps/web/src/vite-env.d.ts`, and the parsed `config` object + in `apps/web/src/config.ts` (unless only the inline script in `apps/web/index.html` reads it, as + `PLAUSIBLE_*` are). 9. **If it must reach the production stack, add it to `docker-compose.yaml`** under the `environment:` list of the service that reads it (`api`, `gateway`, `web`). A bare `- YOUR_KEY` forwards the value diff --git a/.agents/docs/playbooks/add-instrument.md b/.agents/docs/playbooks/add-instrument.md index 37d366f48..37a48d270 100644 --- a/.agents/docs/playbooks/add-instrument.md +++ b/.agents/docs/playbooks/add-instrument.md @@ -17,7 +17,7 @@ advice does not apply here; everything it says about the definition object does. | `kind` in the definition | Directory | Read first | | ------------------------ | -------------------------- | -------------------------------------------------------------- | | `'FORM'` | `src/forms/` (plural) | `src/forms/DNP_HAPPINESS_QUESTIONNAIRE/index.ts` | - | `'INTERACTIVE'` | `src/interactive/` | `src/interactive/DNP_STROOP_TASK/` (multi-file JSX) | + | `'INTERACTIVE'` | `src/interactive/` | `src/interactive/DNP_BREAKOUT_TASK/index.ts` | | `'SERIES'` | `src/series/` | `src/series/DNP_HAPPINESS_QUESTIONNAIRE_WITH_CONSENT/index.ts` | | `'FILE'` | `src/file/` (**singular**) | `src/file/MRI_SCAN_SESSION/index.ts` | @@ -25,7 +25,7 @@ advice does not apply here; everything it says about the definition object does. `src/**/*/index.{js,jsx,ts,tsx}`, so the directory is the unit of work and `index.*` is the only fixed filename. **The directory must be flat**: the CLI then globs `/*` and reads every entry as a file, so a subdirectory fails the build with a `Cannot infer loader` error. Relative imports - need their extension (`'./StroopTask.tsx'`, `'./styles.css'`). + need their extension (`'./styles.css'`, `'./Component.tsx'`). 3. **Import everything from `/runtime/v1/...`**, never from `node_modules` or a workspace package: @@ -68,8 +68,7 @@ advice does not apply here; everything it says about the definition object does. then `await this.instrumentsService.create({ bundle: myNewForm })` inside `init`. **Order in `init` is load-bearing**: a series must be created after every instrument it references (step 5). - Skipping this step compiles, builds, and produces an instrument that is never in a demo instance — - `DNP_STROOP_TASK` is the live example of that. + Skipping this step compiles, builds, and produces an instrument that is never in a demo instance. 7. **Add a unit test and an end-to-end test.** The unit test goes in the top-level `packages/instrument-library/src/__tests__/`, never inside the instrument directory (§Tests in diff --git a/.agents/docs/playbooks/add-web-data-hook.md b/.agents/docs/playbooks/add-web-data-hook.md index 70ebfa868..4cc6404a1 100644 --- a/.agents/docs/playbooks/add-web-data-hook.md +++ b/.agents/docs/playbooks/add-web-data-hook.md @@ -40,7 +40,6 @@ step type-checks and compiles, and fails at runtime or serves the wrong cached d | `meta` key | Use when | | --------------------------------- | --------------------------------------------- | - | `disableRetry` | An idempotent request must not be retried | | `disableDefaultTimeout` | The server may legitimately take >10s (setup) | | `disableDefaultErrorNotification` | The caller shows its own error message | | `disableDefaultAuth` | The request must go out unauthenticated | diff --git a/.agents/docs/playbooks/promote-to-react-core.md b/.agents/docs/playbooks/promote-to-react-core.md index 386c8f8fc..743814565 100644 --- a/.agents/docs/playbooks/promote-to-react-core.md +++ b/.agents/docs/playbooks/promote-to-react-core.md @@ -56,7 +56,7 @@ so nothing that passed lint in `apps/web` fails on style here; what breaks is re becomes an injected component, an `href`, or data passed in: `InstrumentRenderer`'s `NavigationBlocker?: NavigationBlockerComponent`, implemented by `apps/web/src/components/NavigationBlocker.tsx`, is the pattern, and `packages/react-core/AGENTS.md` - carries the rule and its exceptions. Declaring `@tanstack/react-query` here instead would make the + carries the rule. Declaring `@tanstack/react-query` here instead would make the import resolve and the typecheck pass; the throw arrives only at render, which nothing before the Verify block's `pnpm test:e2e` reaches — and only where step 9's second consumer renders it. diff --git a/.agents/docs/playbooks/run-locally.md b/.agents/docs/playbooks/run-locally.md index 771dfd40d..1e9e6a5c0 100644 --- a/.agents/docs/playbooks/run-locally.md +++ b/.agents/docs/playbooks/run-locally.md @@ -32,16 +32,15 @@ that never says the word storage. Step 4 is where you avoid it. Done when `node --version` is v24.15.0 or newer and `pnpm --version` prints. 3. **Generate `.env` — once per clone.** `pnpm generate:env` runs `scripts/generate-env.sh`, which - writes `.env` wholesale from `.env.template`: it fills `PROJECT_ROOT`, mints `SECRET_KEY`, - `GATEWAY_API_KEY`, `STORAGE_ACCESS_KEY` and `STORAGE_SECRET_KEY` with `openssl rand -hex`, and - points `GATEWAY_DATABASE_URL` at a sqlite file under `apps/gateway/data` (creating the - directory). Never re-run it over a working `.env`: it discards every local override and mints a - fresh `SECRET_KEY`, which invalidates live sessions and permanently orphans any stored - instrument-repo credential — `InstrumentReposService` encrypts those under `sha256(SECRET_KEY)`. + writes `.env` wholesale from `.env.template`: it mints `SECRET_KEY`, `GATEWAY_API_KEY`, + `STORAGE_ACCESS_KEY` and `STORAGE_SECRET_KEY` with `openssl rand -hex`, and points + `GATEWAY_DATABASE_URL` at a sqlite file under `apps/gateway/data` (creating the directory). Never + re-run it over a working `.env`: it discards every local override and mints a fresh `SECRET_KEY`, + which invalidates live sessions and permanently orphans any stored instrument-repo credential — + `InstrumentReposService` encrypts those under `sha256(SECRET_KEY)`. Adding one variable to an existing `.env` is `.agents/docs/playbooks/add-env-var.md`. - Done when `.env` exists, `PROJECT_ROOT` is the absolute repo root, and none of the four generated - keys is empty. + Done when `.env` exists and none of the four generated keys is empty. 4. **Set `STORAGE_ENABLED=false` in `.env`** unless an S3-compatible service is listening on `STORAGE_ENDPOINT`. With it false, `StorageModule` provides `null` for the `S3Client`, diff --git a/.agents/docs/workspace-map.md b/.agents/docs/workspace-map.md index 8b2d7572d..96d47c830 100644 --- a/.agents/docs/workspace-map.md +++ b/.agents/docs/workspace-map.md @@ -44,15 +44,15 @@ Nothing depends on an app. All five are leaves. | `instrument-guidelines` | Instrument-authoring guidelines written for an AI agent; ships `AGENTS.md` + a `bin` | no | none | nothing (published for external use) | | `instrument-interpreter` | Evaluates a bundled instrument and validates it against the schemas | no | `instrument-interpreter` | react-core, web | | `instrument-library` | Built-in catalog of ready-made instruments | **yes** — `instrument-bundler` CLI → `dist` | `instrument-library` | api | -| `instrument-stubs` | Minimal stub instruments for tests and examples | no | `instrument-stubs` | instrument-bundler, react-core, schemas, storybook, web | +| `instrument-stubs` | Minimal stub instruments for tests and examples | no | `instrument-stubs` | react-core, schemas, web | | `instrument-utils` | Helpers for working with instrument definitions | no | `instrument-utils` | api, react-core, web | -| `licenses` | SPDX + custom license metadata | no | none | outreach, react-core, runtime-core, runtime-v1, schemas, web | +| `licenses` | SPDX + custom license metadata | no | none | outreach, runtime-core, runtime-v1, schemas, web | | `playground-url` | Encodes/decodes shareable playground URLs and editor file state | `dist` holds the `bin` only; the `.` export is still source | `playground-url` | playground | | `react-core` | React components/hooks shared by more than one frontend | no | `react-core` | gateway, playground, serve-instrument, storybook, web | | `release-info` | Resolves version/branch/commit at build time | no | `release-info` | api, gateway, web | | `runtime-bundler` | Bundles the instrument **runtime**; provides the `runtime-bundler` bin | no | `runtime-bundler` | runtime-v1 | -| `runtime-core` | Public runtime API (`defineInstrument`, i18n, notifications) | **yes** — `tsc -b` → `lib`, esbuild + api-extractor → `dist` | `runtime-core` | api, gateway, instrument-interpreter, instrument-stubs, instrument-utils, outreach, playground, react-core, runtime-v1, schemas, web | -| `runtime-internal` | Internal runtime execution primitives (interactive-task iframe/worker bootstrap) | no | `runtime-internal` | instrument-bundler, instrument-interpreter, react-core, runtime-v1, serve-instrument, subject-utils | +| `runtime-core` | Public runtime API (`defineInstrument`, i18n, notifications) | **yes** — `tsc -b` → `lib`, esbuild + api-extractor → `dist` | `runtime-core` | api, instrument-interpreter, instrument-stubs, instrument-utils, outreach, react-core, runtime-v1, schemas, web | +| `runtime-internal` | Internal runtime execution primitives (interactive-task iframe/worker bootstrap) | no | `runtime-internal` | instrument-bundler, instrument-interpreter, runtime-v1, serve-instrument, subject-utils | | `runtime-meta` | Runtime version list and per-version asset manifest types | no | `runtime-meta` | outreach, serve-instrument, vite-plugin-runtime | | `schemas` | Zod schemas/types shared across tiers, one export per domain | no | `schemas` | api, demo, gateway, instrument-interpreter, instrument-utils, outreach, playground, react-core, release-info, subject-utils, testing, web | | `serve-instrument` | CLI/server for previewing one instrument outside the full app | **yes** — esbuild → `dist/cli.js` (the `bin`) | `serve-instrument` | nothing (published for external use) | diff --git a/.agents/skills/odc-api/SKILL.md b/.agents/skills/odc-api/SKILL.md index 1a9dc529d..e4109f0dd 100644 --- a/.agents/skills/odc-api/SKILL.md +++ b/.agents/skills/odc-api/SKILL.md @@ -23,8 +23,9 @@ in this repo, and the only thing standing in front of it is you reading the `whe `{ ability }: EntityOperationOptions` and its controller forwards `@CurrentUser('ability')`, or it takes `currentUser?: RequestUser` and reads `.ability` off it itself while its controller forwards `@CurrentUser()` — the second shape is `apps/api/src/instruments/instruments.service.ts` and -`apps/api/src/instrument-records/files/files.service.ts`. In `instruments.service.ts` the parameter -is optional, so a call site that omits it compiles and queries unscoped. +`apps/api/src/instrument-records/files/files.service.ts`. In `instruments.service.ts` only +`findBundleById` takes it as optional, because the gateway resolves an assignment's bundle unscoped, +so a call site there that omits it compiles and queries every group. **Unscoped is a decision, not an omission.** `AuditService.find` takes no ability at all, because a manage-all guard is the whole check on `GET /v1/audit/logs`; the inventory of routes that are diff --git a/.agents/skills/odc-debugging/SKILL.md b/.agents/skills/odc-debugging/SKILL.md index 749501281..af9de08f8 100644 --- a/.agents/skills/odc-debugging/SKILL.md +++ b/.agents/skills/odc-debugging/SKILL.md @@ -42,9 +42,8 @@ run outside turbo — a bare `tsc`, `vitest`, storybook — does not. **Some breakage is known, documented, and deliberately left alone.** Before calling anything a new bug, read the `AGENTS.md` of the package that owns it — that is where these are recorded (the always-skipped development block in -`packages/release-info`, `testing`'s `test:chrome` naming a Playwright project that does not exist), with the build- -and test-infrastructure ones in the `Known warts` section of `.agents/docs/architecture/testing-strategy.md`. Name any -you hit in your reply and leave it as it is. +`packages/release-info`), with the build- and test-infrastructure ones in the `Known warts` section of +`.agents/docs/architecture/testing-strategy.md`. Name any you hit in your reply and leave it as it is. ## Quiet and wrong @@ -58,7 +57,7 @@ Read the owner before changing anything: the row tells you where you are, not wh | A new route is reachable by any authenticated user | `@RouteAccess([])` passes because `[].every(...)` is `true` — a handler with no decorator 500s instead of opening | `.agents/skills/odc-api/SKILL.md` | | A route 500s inside the validation pipe the first time it is called | The `$Schema` used as the `@Body()` parameter type arrived through `import type`, which erases the runtime binding | `.agents/skills/odc-api/SKILL.md` | | Two apparently identical schemas reject each other; error maps miss | A bare `zod` import resolves to the vendored v3, a separate registry from `zod/v4` | `packages/schemas/AGENTS.md` | -| An instrument builds and is listed, but no instance has it | `apps/api/src/demo/demo.service.ts` holds one hand-written import per instrument; `DNP_STROOP_TASK` is the live proof | `.agents/skills/odc-instruments/SKILL.md` | +| An instrument builds and is listed, but no instance has it | `apps/api/src/demo/demo.service.ts` holds one hand-written import per instrument | `.agents/skills/odc-instruments/SKILL.md` | | The runtime build logged an error and the command still succeeded | `runtime-bundler`'s CLI catches a bundle failure, prints it, and exits 0 | `packages/runtime-bundler/AGENTS.md` | | A route literal a Playwright spec uses does not type-check | `testing/src/generated/route.d.ts` is git-ignored and rewritten by `gen:routes` from `route-tree.ts` | `.agents/skills/odc-web-route/SKILL.md` | | A spec has no authentication and every fixture is undefined | It imported `test` from `@playwright/test` rather than `../support/fixtures` | `.agents/skills/odc-testing/SKILL.md` | diff --git a/.agents/skills/odc-instruments/SKILL.md b/.agents/skills/odc-instruments/SKILL.md index 3251632a6..4f2adb49f 100644 --- a/.agents/skills/odc-instruments/SKILL.md +++ b/.agents/skills/odc-instruments/SKILL.md @@ -52,12 +52,10 @@ build-time `ResolverError`. The reverse is silent — the package installs and i emitted directory exists under `runtime/v1/dist` before believing the build. **A built instrument is not a loaded one.** The built-in catalog takes one hand-written default import and one -`create` call per instrument in `apps/api/src/demo/demo.service.ts` (`.agents/docs/playbooks/add-instrument.md` -is where that step lives); `src/interactive/DNP_STROOP_TASK` is the standing proof — it builds, `pnpm run available` -lists it, and it is in no demo instance. For an external repository, discovery scans -`lib/{file,forms,interactive,series}` and returns `series` last, because a series is rejected unless the -instruments it references are already stored — so a series whose items live in a _different_ repository still -fails to import (`.agents/docs/architecture/instrument-pipeline.md`). +`create` call per instrument in `apps/api/src/demo/demo.service.ts` (`.agents/docs/playbooks/add-instrument.md` is +where that step lives). For an external repository, discovery scans `lib/{file,forms,interactive,series}` and returns +`series` last, because a series is rejected unless the instruments it references are already stored — so a series +whose items live in a _different_ repository still fails to import (`.agents/docs/architecture/instrument-pipeline.md`). **`/runtime/v1/zod@3.x` is the Zod v3 API.** `vendor/zod@3.x`'s `.` export re-exports `zod/v3`; the v4 API is a separate subpath, `/runtime/v1/zod@3.x/v4`. The repo-wide `no-restricted-imports` ban on bare `zod` diff --git a/.env.template b/.env.template index ac487e703..1b8924cca 100644 --- a/.env.template +++ b/.env.template @@ -30,9 +30,6 @@ RUSTFS_VERSION=latest ## PRODUCTION + DEVELOPMENT ## --------------------------------- -# The absolute path to the root of the project (i.e., the directory where this file is located) -PROJECT_ROOT= - # The value to use for various security purposes. SECRET_KEY= diff --git a/apps/api/Dockerfile b/apps/api/Dockerfile index 2f55755a3..f7cf1a426 100644 --- a/apps/api/Dockerfile +++ b/apps/api/Dockerfile @@ -1,7 +1,6 @@ FROM node:lts-krypton AS base WORKDIR /app ARG RELEASE_VERSION -ENV GATEWAY_DATABASE_URL="file:/dev/null" ENV PNPM_HOME="/pnpm" ENV PATH="$PNPM_HOME:$PNPM_HOME/bin:$PATH" ENV NODE_OPTIONS="--max-old-space-size=8192" @@ -25,7 +24,6 @@ RUN turbo build --filter=@opendatacapture/api # RUN SERVER FROM base AS runner COPY --from=installer /app/apps/api/dist/ /app/dist/ -COPY --from=installer /app/apps/api/public/ /app/public/ COPY --from=installer /app/apps/api/dist/runtime/ /runtime/ RUN echo '{ "type": "module", "imports": { "#runtime/v1/*": "./dist/runtime/v1/*" } }' > package.json diff --git a/apps/api/package.json b/apps/api/package.json index 928d6d390..16aeb95ca 100644 --- a/apps/api/package.json +++ b/apps/api/package.json @@ -44,7 +44,6 @@ "@opendatacapture/subject-utils": "workspace:*", "@prisma/client": "catalog:", "axios": "catalog:", - "express": "^5.0.1", "fastify": "^5.12.5", "hibp": "^15.2.1", "jszip": "^3.10.2", @@ -74,8 +73,5 @@ "development": "../../runtime/v1/dist/*", "default": "./dist/runtime/v1/*" } - }, - "trustedDependencies": [ - "prisma" - ] + } } diff --git a/apps/api/prisma/schema.prisma b/apps/api/prisma/schema.prisma index d2d3c49bf..0340ba29f 100644 --- a/apps/api/prisma/schema.prisma +++ b/apps/api/prisma/schema.prisma @@ -209,18 +209,6 @@ model InstrumentRecordFile { // Instruments -enum InstrumentKind { - FILE - FORM - INTERACTIVE - SERIES -} - -type InstrumentInternal { - name String - edition Float -} - model Instrument { createdAt DateTime @default(now()) @db.Date updatedAt DateTime @updatedAt @db.Date diff --git a/apps/api/public/favicon.ico b/apps/api/public/favicon.ico deleted file mode 100644 index c0cc81787..000000000 Binary files a/apps/api/public/favicon.ico and /dev/null differ diff --git a/apps/api/public/index.html b/apps/api/public/index.html deleted file mode 100644 index d1c44b444..000000000 --- a/apps/api/public/index.html +++ /dev/null @@ -1,14 +0,0 @@ - - - - Open Data Capture API - - - - - - - - - - diff --git a/apps/api/public/styles.css b/apps/api/public/styles.css deleted file mode 100644 index ea1e941c3..000000000 --- a/apps/api/public/styles.css +++ /dev/null @@ -1,4 +0,0 @@ -body { - margin: 0; - padding: 0; -} diff --git a/apps/api/src/auth/ability.factory.ts b/apps/api/src/auth/ability.factory.ts index d54785990..ce7111664 100644 --- a/apps/api/src/auth/ability.factory.ts +++ b/apps/api/src/auth/ability.factory.ts @@ -17,7 +17,7 @@ import type { AppAbility, Permission } from './auth.types'; * fields are the same ones the group manager rules below use. Typed over the schema's list, so a * subject made scopable there fails to compile here until its group field is named. */ -export const GROUP_SCOPED_CONDITIONS = { +const GROUP_SCOPED_CONDITIONS = { Assignment: (groupId) => ({ groupId: { in: [groupId] } }), Group: (groupId) => ({ id: { in: [groupId] } }), InstrumentRecord: (groupId) => ({ groupId: { in: [groupId] } }), diff --git a/apps/api/src/auth/auth.types.ts b/apps/api/src/auth/auth.types.ts index ca9d1d647..2663f77b5 100644 --- a/apps/api/src/auth/auth.types.ts +++ b/apps/api/src/auth/auth.types.ts @@ -14,12 +14,10 @@ type AppSubjects = 'all' | Subjects; type AppSubjectName = Extract; -type AppSubjectModel = Extract; - type AppAbilities = [AppAction, AppSubjects]; type AppAbility = PureAbility; type Permission = RawRuleOf>; -export type { AppAbilities, AppAbility, AppAction, AppSubjectModel, AppSubjectModels, AppSubjectName, Permission }; +export type { AppAbilities, AppAbility, AppAction, AppSubjectModels, AppSubjectName, Permission }; diff --git a/apps/api/src/core/decorators/route-access.decorator.ts b/apps/api/src/core/decorators/route-access.decorator.ts index 7da0c915f..a6f58f774 100644 --- a/apps/api/src/core/decorators/route-access.decorator.ts +++ b/apps/api/src/core/decorators/route-access.decorator.ts @@ -4,15 +4,15 @@ import type { AppAction, AppSubjectName } from '../../auth/auth.types.js'; const ROUTE_ACCESS_METADATA_KEY = 'ODC_ROUTE_ACCESS_TOKEN'; -export type PublicRouteAccess = 'public'; +type PublicRouteAccess = 'public'; + +type ProtectedRouteAccess = ProtectedRoutePermissionSet | ProtectedRoutePermissionSet[]; export type ProtectedRoutePermissionSet = { action: AppAction; subject: AppSubjectName; }; -export type ProtectedRouteAccess = ProtectedRoutePermissionSet | ProtectedRoutePermissionSet[]; - export type RouteAccessType = ProtectedRouteAccess | PublicRouteAccess; /** diff --git a/apps/api/src/gateway/__tests__/gateway.service.test.ts b/apps/api/src/gateway/__tests__/gateway.service.test.ts index 2df683512..6b6a7d435 100644 --- a/apps/api/src/gateway/__tests__/gateway.service.test.ts +++ b/apps/api/src/gateway/__tests__/gateway.service.test.ts @@ -151,16 +151,10 @@ describe('GatewayService', () => { url: 'https://gateway.example.org/assignments/assignment-1' }; - it('should filter the remote assignments by the provided subject', async () => { - axiosRef.get.mockResolvedValueOnce(response(200, [])); - await gatewayService.fetchRemoteAssignments({ subjectId: 'subject-1' }); - expect(axiosRef.get).toHaveBeenCalledWith('/api/assignments', { params: { subjectId: 'subject-1' } }); - }); - - it('should request every remote assignment when no subject is provided', async () => { + it('should request every remote assignment from the gateway', async () => { axiosRef.get.mockResolvedValueOnce(response(200, [])); await gatewayService.fetchRemoteAssignments(); - expect(axiosRef.get).toHaveBeenCalledWith('/api/assignments', { params: { subjectId: undefined } }); + expect(axiosRef.get).toHaveBeenCalledWith('/api/assignments'); }); it('should return the remote assignments with their dates parsed', async () => { diff --git a/apps/api/src/gateway/gateway.service.ts b/apps/api/src/gateway/gateway.service.ts index ecaf04ba6..46d709481 100644 --- a/apps/api/src/gateway/gateway.service.ts +++ b/apps/api/src/gateway/gateway.service.ts @@ -94,12 +94,8 @@ export class GatewayService { return $MutateAssignmentResponseBody.parseAsync(response.data); } - async fetchRemoteAssignments({ subjectId }: { subjectId?: string } = {}): Promise { - const response = await this.httpService.axiosRef.get(`/api/assignments`, { - params: { - subjectId - } - }); + async fetchRemoteAssignments(): Promise { + const response = await this.httpService.axiosRef.get('/api/assignments'); if (response.status !== HttpStatus.OK) { throw new BadGatewayException(`Unexpected Status Code From Gateway: ${response.status}`, { cause: response.statusText diff --git a/apps/api/src/instrument-records/__tests__/instrument-records.service.test.ts b/apps/api/src/instrument-records/__tests__/instrument-records.service.test.ts index 39778fae6..47b823d19 100644 --- a/apps/api/src/instrument-records/__tests__/instrument-records.service.test.ts +++ b/apps/api/src/instrument-records/__tests__/instrument-records.service.test.ts @@ -980,16 +980,6 @@ describe('InstrumentRecordsService', () => { }); }); - describe('exists', () => { - it('should report whether a record matches the given filter', async () => { - instrumentRecordModel.exists.mockResolvedValueOnce(true); - - await expect(instrumentRecordsService.exists({ id: 'record-1' })).resolves.toBe(true); - - expect(instrumentRecordModel.exists).toHaveBeenCalledWith({ id: 'record-1' }); - }); - }); - describe('linearModel', () => { const measures = { score: { kind: 'computed', label: 'Score', value: () => 1 } }; const recordAt = (time: number, computedMeasures: { [key: string]: unknown }) => ({ diff --git a/apps/api/src/instrument-records/instrument-records.service.ts b/apps/api/src/instrument-records/instrument-records.service.ts index 5047187ba..18355776c 100644 --- a/apps/api/src/instrument-records/instrument-records.service.ts +++ b/apps/api/src/instrument-records/instrument-records.service.ts @@ -186,10 +186,6 @@ export class InstrumentRecordsService { return deletedRecord; } - async exists(where: Prisma.InstrumentRecordWhereInput): Promise { - return this.instrumentRecordModel.exists(where); - } - async exportRecords( { groupId }: { groupId?: string } = {}, { ability }: Required> diff --git a/apps/api/src/instrument-repos/__tests__/instrument-repos.controller.test.ts b/apps/api/src/instrument-repos/__tests__/instrument-repos.controller.test.ts index 29b3ba8ec..19d4e4a4d 100644 --- a/apps/api/src/instrument-repos/__tests__/instrument-repos.controller.test.ts +++ b/apps/api/src/instrument-repos/__tests__/instrument-repos.controller.test.ts @@ -45,12 +45,6 @@ describe('InstrumentReposController', () => { expect(service.findAll).toHaveBeenCalledWith({ ability }); }); - it("should scope the lookup to the current user's ability", async () => { - service.findById.mockResolvedValueOnce(repo); - await expect(controller.findById('repo-1', ability)).resolves.toBe(repo); - expect(service.findById).toHaveBeenCalledWith('repo-1', { ability }); - }); - it('should sync the requested repository', async () => { service.sync.mockResolvedValueOnce(repo); await expect(controller.sync('repo-1')).resolves.toBe(repo); @@ -61,7 +55,6 @@ describe('InstrumentReposController', () => { ['create', 'create'], ['deleteById', 'delete'], ['findAll', 'read'], - ['findById', 'read'], ['sync', 'update'] ] as const)('should gate %s on the %s InstrumentRepo permission', (handler, action) => { const reflector = new Reflector(); diff --git a/apps/api/src/instrument-repos/__tests__/instrument-repos.service.test.ts b/apps/api/src/instrument-repos/__tests__/instrument-repos.service.test.ts index 7a3c1f2f1..060227587 100644 --- a/apps/api/src/instrument-repos/__tests__/instrument-repos.service.test.ts +++ b/apps/api/src/instrument-repos/__tests__/instrument-repos.service.test.ts @@ -145,18 +145,6 @@ describe('InstrumentReposService', () => { }); }); - describe('findById', () => { - it('throws a NotFoundException when the repo does not exist', async () => { - instrumentRepoModel.findFirst.mockResolvedValueOnce(null); - await expect(service.findById('missing')).rejects.toBeInstanceOf(NotFoundException); - }); - - it('strips the access token from the returned repo', async () => { - instrumentRepoModel.findFirst.mockResolvedValueOnce({ accessToken: 'encrypted', id: '1', name: 'repo' }); - await expect(service.findById('1')).resolves.not.toHaveProperty('accessToken'); - }); - }); - describe('deleteById', () => { it('throws a NotFoundException when the repo does not exist', async () => { instrumentRepoModel.findFirst.mockResolvedValueOnce(null); diff --git a/apps/api/src/instrument-repos/instrument-repos.controller.ts b/apps/api/src/instrument-repos/instrument-repos.controller.ts index fc1e05e57..f6fb6e7a1 100644 --- a/apps/api/src/instrument-repos/instrument-repos.controller.ts +++ b/apps/api/src/instrument-repos/instrument-repos.controller.ts @@ -32,13 +32,6 @@ export class InstrumentReposController { return this.instrumentReposService.findAll({ ability }); } - @ApiOperation({ summary: 'Get Instrument Repo' }) - @Get(':id') - @RouteAccess({ action: 'read', subject: 'InstrumentRepo' }) - findById(@Param('id') id: string, @CurrentUser('ability') ability?: AppAbility) { - return this.instrumentReposService.findById(id, { ability }); - } - @ApiOperation({ summary: 'Sync Instrument Repo' }) @Post(':id/sync') @RouteAccess({ action: 'update', subject: 'InstrumentRepo' }) diff --git a/apps/api/src/instrument-repos/instrument-repos.service.ts b/apps/api/src/instrument-repos/instrument-repos.service.ts index cf0e8dbc3..9df15a694 100644 --- a/apps/api/src/instrument-repos/instrument-repos.service.ts +++ b/apps/api/src/instrument-repos/instrument-repos.service.ts @@ -105,16 +105,6 @@ export class InstrumentReposService implements OnModuleInit { return repos.map((repo) => this.stripSecrets(repo)); } - async findById(id: string, { ability }: EntityOperationOptions = {}) { - const repo = await this.instrumentRepoModel.findFirst({ - where: { AND: [accessibleQuery(ability, 'read', 'InstrumentRepo')], id } - }); - if (!repo) { - throw new NotFoundException(`Failed to find instrument repo with ID: ${id}`); - } - return this.stripSecrets(repo); - } - async onModuleInit(): Promise { // Self-heal any instruments left orphaned by repositories deleted before this logic existed. try { diff --git a/apps/api/src/instruments/__tests__/instruments.controller.test.ts b/apps/api/src/instruments/__tests__/instruments.controller.test.ts index e5b79883c..4e1a4b948 100644 --- a/apps/api/src/instruments/__tests__/instruments.controller.test.ts +++ b/apps/api/src/instruments/__tests__/instruments.controller.test.ts @@ -153,12 +153,12 @@ describe('InstrumentsController', () => { expect(instrumentsService.findBundleById).toHaveBeenCalledWith('form-1', currentUser, 'group-1'); }); - it('should list instruments of the requested kind within the requested group', async () => { + it('should list instruments of the requested kind for the current user', async () => { const listed = [{ id: 'form-1', internal: { edition: 1, name: 'FORM_A' }, title: 'Form A' }]; instrumentsService.list.mockResolvedValue(listed); - await expect(instrumentsController.list(currentUser, 'group-1', 'FORM')).resolves.toBe(listed); - expect(instrumentsService.list).toHaveBeenCalledWith({ kind: 'FORM' }, currentUser, 'group-1'); + await expect(instrumentsController.list(currentUser, 'FORM')).resolves.toBe(listed); + expect(instrumentsService.list).toHaveBeenCalledWith({ kind: 'FORM' }, currentUser); }); it('should archive on behalf of the current user, so the change is audited under their id', async () => { diff --git a/apps/api/src/instruments/__tests__/instruments.service.test.ts b/apps/api/src/instruments/__tests__/instruments.service.test.ts index fc9737617..f2cdeba38 100644 --- a/apps/api/src/instruments/__tests__/instruments.service.test.ts +++ b/apps/api/src/instruments/__tests__/instruments.service.test.ts @@ -125,6 +125,8 @@ const createRequestUser = (ability: AppAbility, groups: Group[] = []): RequestUs username: 'test-user' }); +const adminUser = createRequestUser(createAppAbility([{ action: 'manage', subject: 'all' }])); + /** The series definition `createSeries` last handed to the bundler. */ const bundledDefinition = (): unknown => { const content = vi.mocked(bundle).mock.lastCall?.[0].inputs[0]?.content; @@ -199,22 +201,29 @@ describe('InstrumentsService', () => { const findSpy = vi.spyOn(instrumentsService, 'find').mockResolvedValue([existingSeries]); const createSpy = vi.spyOn(instrumentsService, 'create'); - const result = await instrumentsService.createSeries({ - details: { title: 'My New Series' }, - groupId: 'group-1', - items: [ - { edition: 1, name: 'FORM_A' }, - { edition: 1, name: 'FORM_B' } - ], - language: 'en' - }); + const result = await instrumentsService.createSeries( + { + details: { title: 'My New Series' }, + groupId: 'group-1', + items: [ + { edition: 1, name: 'FORM_A' }, + { edition: 1, name: 'FORM_B' } + ], + language: 'en' + }, + adminUser + ); expect(result).toEqual({ existingTitle: { en: 'Existing Series', fr: 'Série existante' }, outcome: 'duplicate' }); - expect(findSpy).toHaveBeenNthCalledWith(1, { seriesGroupId: 'group-1' }, {}); - expect(findSpy).toHaveBeenNthCalledWith(2, { kind: 'SERIES', seriesGroupId: 'group-1' }, { ability: undefined }); + expect(findSpy).toHaveBeenNthCalledWith(1, { seriesGroupId: 'group-1' }, { ability: adminUser.ability }); + expect(findSpy).toHaveBeenNthCalledWith( + 2, + { kind: 'SERIES', seriesGroupId: 'group-1' }, + { ability: adminUser.ability } + ); expect(createSpy).not.toHaveBeenCalled(); }); @@ -222,15 +231,18 @@ describe('InstrumentsService', () => { vi.spyOn(instrumentsService, 'find').mockResolvedValue([existingSeries]); const createSpy = vi.spyOn(instrumentsService, 'create').mockResolvedValue({ id: 'reordered-id' } as any); - const result = await instrumentsService.createSeries({ - details: { title: 'Reordered Series' }, - groupId: 'group-1', - items: [ - { edition: 1, name: 'FORM_B' }, - { edition: 1, name: 'FORM_A' } - ], - language: 'en' - }); + const result = await instrumentsService.createSeries( + { + details: { title: 'Reordered Series' }, + groupId: 'group-1', + items: [ + { edition: 1, name: 'FORM_B' }, + { edition: 1, name: 'FORM_A' } + ], + language: 'en' + }, + adminUser + ); expect(createSpy).toHaveBeenCalledWith({ bundle: '__BUNDLE__' }, { seriesGroupId: 'group-1' }); expect(result).toEqual({ instrumentId: 'reordered-id', outcome: 'created' }); @@ -240,16 +252,19 @@ describe('InstrumentsService', () => { vi.spyOn(instrumentsService, 'find').mockResolvedValue([existingSeries]); const createSpy = vi.spyOn(instrumentsService, 'create').mockResolvedValue({ id: 'new-id' } as any); - const result = await instrumentsService.createSeries({ - confirmDuplicate: true, - details: { title: 'My New Series' }, - groupId: 'group-1', - items: [ - { edition: 1, name: 'FORM_A' }, - { edition: 1, name: 'FORM_B' } - ], - language: 'en' - }); + const result = await instrumentsService.createSeries( + { + confirmDuplicate: true, + details: { title: 'My New Series' }, + groupId: 'group-1', + items: [ + { edition: 1, name: 'FORM_A' }, + { edition: 1, name: 'FORM_B' } + ], + language: 'en' + }, + adminUser + ); expect(createSpy).toHaveBeenCalledWith({ bundle: '__BUNDLE__' }, { seriesGroupId: 'group-1' }); expect(result).toEqual({ instrumentId: 'new-id', outcome: 'created' }); @@ -259,15 +274,18 @@ describe('InstrumentsService', () => { vi.spyOn(instrumentsService, 'find').mockResolvedValue([existingSeries]); const createSpy = vi.spyOn(instrumentsService, 'create').mockResolvedValue({ id: 'fresh-id' } as any); - const result = await instrumentsService.createSeries({ - details: { title: 'Totally New' }, - groupId: 'group-1', - items: [ - { edition: 1, name: 'FORM_C' }, - { edition: 1, name: 'FORM_D' } - ], - language: 'en' - }); + const result = await instrumentsService.createSeries( + { + details: { title: 'Totally New' }, + groupId: 'group-1', + items: [ + { edition: 1, name: 'FORM_C' }, + { edition: 1, name: 'FORM_D' } + ], + language: 'en' + }, + adminUser + ); expect(createSpy).toHaveBeenCalledWith({ bundle: '__BUNDLE__' }, { seriesGroupId: 'group-1' }); expect(result).toEqual({ instrumentId: 'fresh-id', outcome: 'created' }); @@ -277,16 +295,19 @@ describe('InstrumentsService', () => { vi.spyOn(instrumentsService, 'find').mockResolvedValue([]); vi.spyOn(instrumentsService, 'create').mockResolvedValue({ id: 'generated-id' } as any); - await instrumentsService.createSeries({ - clientDetails: { instructions: ['Complete the instruments in order.'] }, - details: { description: 'Optional description', title: 'Generated Series' }, - groupId: 'group-1', - items: [ - { edition: 2, name: 'FORM_B' }, - { edition: 1, name: 'FORM_A' } - ], - language: 'en' - }); + await instrumentsService.createSeries( + { + clientDetails: { instructions: ['Complete the instruments in order.'] }, + details: { description: 'Optional description', title: 'Generated Series' }, + groupId: 'group-1', + items: [ + { edition: 2, name: 'FORM_B' }, + { edition: 1, name: 'FORM_A' } + ], + language: 'en' + }, + adminUser + ); expect(bundle).toHaveBeenCalledTimes(1); @@ -346,13 +367,16 @@ describe('InstrumentsService', () => { instrumentModel.exists.mockResolvedValue(false); instrumentModel.findMany.mockResolvedValue([{ id: 'hash:FORM_A-1' }, { id: 'hash:FORM_B-1' }] as any); - const result = await instrumentsService.createSeries({ - confirmDuplicate: true, - details: { title: 'Stored Series' }, - groupId: 'group-1', - items, - language: 'en' - }); + const result = await instrumentsService.createSeries( + { + confirmDuplicate: true, + details: { title: 'Stored Series' }, + groupId: 'group-1', + items, + language: 'en' + }, + adminUser + ); expect(virtualizationService.eval).toHaveBeenCalledWith('__BUNDLE__'); expect(instrumentModel.exists).toHaveBeenCalledWith({ id }); @@ -397,16 +421,19 @@ describe('InstrumentsService', () => { instrumentModel.exists.mockResolvedValue(false); instrumentModel.findMany.mockResolvedValue([{ id: 'hash:FORM_A-1' }, { id: 'hash:FORM_B-1' }] as any); - await instrumentsService.createSeries({ - confirmDuplicate: true, - details: { title: 'Uploaded Items' }, - groupId: 'group-1', - items: [ - { edition: 1, name: 'FORM_A' }, - { edition: 1, name: 'FORM_B' } - ], - language: 'en' - }); + await instrumentsService.createSeries( + { + confirmDuplicate: true, + details: { title: 'Uploaded Items' }, + groupId: 'group-1', + items: [ + { edition: 1, name: 'FORM_A' }, + { edition: 1, name: 'FORM_B' } + ], + language: 'en' + }, + adminUser + ); const [{ where }] = instrumentModel.findMany.mock.calls.at(-1)!; expect(where.AND[0].OR).toContainEqual({ sourceRepoId: { isSet: false } }); @@ -417,15 +444,18 @@ describe('InstrumentsService', () => { const createSpy = vi.spyOn(instrumentsService, 'create'); await expect( - instrumentsService.createSeries({ - details: { title: 'existing series' }, - groupId: 'group-1', - items: [ - { edition: 1, name: 'FORM_X' }, - { edition: 1, name: 'FORM_Y' } - ], - language: 'en' - }) + instrumentsService.createSeries( + { + details: { title: 'existing series' }, + groupId: 'group-1', + items: [ + { edition: 1, name: 'FORM_X' }, + { edition: 1, name: 'FORM_Y' } + ], + language: 'en' + }, + adminUser + ) ).rejects.toThrow(ConflictException); expect(createSpy).not.toHaveBeenCalled(); }); @@ -434,15 +464,18 @@ describe('InstrumentsService', () => { const createSpy = vi.spyOn(instrumentsService, 'create'); await expect( - instrumentsService.createSeries({ - details: { title: ' ' }, - groupId: 'group-1', - items: [ - { edition: 1, name: 'FORM_X' }, - { edition: 1, name: 'FORM_Y' } - ], - language: 'en' - }) + instrumentsService.createSeries( + { + details: { title: ' ' }, + groupId: 'group-1', + items: [ + { edition: 1, name: 'FORM_X' }, + { edition: 1, name: 'FORM_Y' } + ], + language: 'en' + }, + adminUser + ) ).rejects.toThrow(UnprocessableEntityException); expect(createSpy).not.toHaveBeenCalled(); }); @@ -451,15 +484,18 @@ describe('InstrumentsService', () => { vi.spyOn(instrumentsService, 'find').mockResolvedValue([]); vi.spyOn(instrumentsService, 'create').mockResolvedValue({ id: 'created-id' } as any); - await instrumentsService.createSeries({ - details: { title: ' Padded Series ' }, - groupId: 'group-1', - items: [ - { edition: 1, name: 'FORM_X' }, - { edition: 1, name: 'FORM_Y' } - ], - language: 'en' - }); + await instrumentsService.createSeries( + { + details: { title: ' Padded Series ' }, + groupId: 'group-1', + items: [ + { edition: 1, name: 'FORM_X' }, + { edition: 1, name: 'FORM_Y' } + ], + language: 'en' + }, + adminUser + ); expect(bundledDefinition()).toMatchObject({ details: { title: 'Padded Series' } }); }); @@ -490,13 +526,16 @@ describe('InstrumentsService', () => { instrumentModel.findMany.mockResolvedValue([{ id: 'hash:FORM_A-1' }] as any); await expect( - instrumentsService.createSeries({ - confirmDuplicate: true, - details: { title: 'S' }, - groupId: 'group-1', - items, - language: 'en' - }) + instrumentsService.createSeries( + { + confirmDuplicate: true, + details: { title: 'S' }, + groupId: 'group-1', + items, + language: 'en' + }, + adminUser + ) ).rejects.toThrow(UnprocessableEntityException); expect(instrumentModel.create).not.toHaveBeenCalled(); }); @@ -527,13 +566,16 @@ describe('InstrumentsService', () => { instrumentModel.exists.mockResolvedValue(false); instrumentModel.findMany.mockResolvedValue([{ id: 'hash:FORM_A-1' }, { id: 'hash:FORM_B-1' }] as any); - await instrumentsService.createSeries({ - confirmDuplicate: true, - details: { title: 'S' }, - groupId: 'group-1', - items, - language: 'en' - }); + await instrumentsService.createSeries( + { + confirmDuplicate: true, + details: { title: 'S' }, + groupId: 'group-1', + items, + language: 'en' + }, + adminUser + ); expect(instrumentModel.findMany).toHaveBeenCalledWith({ select: { id: true }, @@ -549,15 +591,18 @@ describe('InstrumentsService', () => { groupModel.findFirst.mockResolvedValue(null); await expect( - instrumentsService.createSeries({ - details: { title: 'Inaccessible Group Series' }, - groupId: 'group-2', - items: [ - { edition: 1, name: 'FORM_A' }, - { edition: 1, name: 'FORM_B' } - ], - language: 'en' - }) + instrumentsService.createSeries( + { + details: { title: 'Inaccessible Group Series' }, + groupId: 'group-2', + items: [ + { edition: 1, name: 'FORM_A' }, + { edition: 1, name: 'FORM_B' } + ], + language: 'en' + }, + adminUser + ) ).rejects.toThrow(NotFoundException); expect(instrumentModel.create).not.toHaveBeenCalled(); }); @@ -743,15 +788,33 @@ describe('InstrumentsService', () => { await expect(instrumentsService.findInfo({}, currentUser, 'group-2')).rejects.toThrow(ForbiddenException); expect(instrumentModel.findMany).not.toHaveBeenCalled(); }); + + it('narrows the owned series to a requested group the current user belongs to', async () => { + const ability = createAppAbility([{ action: 'read', subject: 'Instrument' }]); + const currentUser = createRequestUser(ability, [createGroup('group-1'), createGroup('group-2')]); + instrumentModel.findMany.mockResolvedValue([]); + + await instrumentsService.findInfo({}, currentUser, 'group-2'); + + expect(instrumentModel.findMany.mock.lastCall?.[0]).toMatchObject({ + where: { + AND: expect.arrayContaining([ + { + OR: [{ seriesGroupId: null }, { seriesGroupId: { isSet: false } }, { seriesGroupId: { in: ['group-2'] } }] + } + ]) + } + }); + }); }); describe('deleteById', () => { it('throws when the instrument does not exist', async () => { instrumentModel.findFirst.mockResolvedValue(null); - await expect(instrumentsService.deleteById('missing')).rejects.toThrow(NotFoundException); + await expect(instrumentsService.deleteById('missing', adminUser)).rejects.toThrow(NotFoundException); expect(instrumentModel.findFirst).toHaveBeenCalledWith({ where: { - AND: [{}], + AND: [accessibleQuery(adminUser.ability, 'delete', 'Instrument')], id: 'missing' } }); @@ -764,7 +827,7 @@ describe('InstrumentsService', () => { value: { internal: { edition: 1, name: 'FORM_A' }, kind: 'FORM' } } as any); - await expect(instrumentsService.deleteById('scalar')).rejects.toThrow(ForbiddenException); + await expect(instrumentsService.deleteById('scalar', adminUser)).rejects.toThrow(ForbiddenException); expect(instrumentModel.delete).not.toHaveBeenCalled(); }); @@ -777,7 +840,7 @@ describe('InstrumentsService', () => { // Records collected through a series carry it in seriesInstrumentId (never as their instrumentId). instrumentRecordModel.count.mockResolvedValue(3); - await expect(instrumentsService.deleteById('target')).rejects.toThrow(ForbiddenException); + await expect(instrumentsService.deleteById('target', adminUser)).rejects.toThrow(ForbiddenException); expect(instrumentRecordModel.count).toHaveBeenCalledWith({ where: { OR: [{ instrumentId: 'target' }, { seriesInstrumentId: 'target' }] } }); @@ -795,7 +858,7 @@ describe('InstrumentsService', () => { instrumentRecordModel.count.mockResolvedValue(0); assignmentModel.count.mockResolvedValue(1); - await expect(instrumentsService.deleteById('target')).rejects.toThrow(ForbiddenException); + await expect(instrumentsService.deleteById('target', adminUser)).rejects.toThrow(ForbiddenException); expect(assignmentModel.count).toHaveBeenCalledWith({ where: { instrumentId: 'target' } }); expect(instrumentModel.delete).not.toHaveBeenCalled(); }); @@ -809,7 +872,7 @@ describe('InstrumentsService', () => { instrumentRecordModel.count.mockResolvedValue(0); groupModel.findMany.mockResolvedValue([{ accessibleInstrumentIds: ['other', 'target'], id: 'g1' }]); - const result = await instrumentsService.deleteById('target'); + const result = await instrumentsService.deleteById('target', adminUser); expect(groupModel.update).toHaveBeenCalledWith({ data: { accessibleInstrumentIds: { set: ['other'] } }, @@ -828,7 +891,7 @@ describe('InstrumentsService', () => { instrumentRecordModel.count.mockResolvedValue(0); groupModel.findMany.mockResolvedValue([]); - await instrumentsService.deleteById('target'); + await instrumentsService.deleteById('target', adminUser); // Checking the kind populates the cache, so a delete always leaves an entry behind to clean up. expect(instanceCache.has('target')).toBe(false); @@ -841,7 +904,7 @@ describe('InstrumentsService', () => { value: { internal: { edition: 1, name: 'FORM_A' }, kind: 'FORM' } } as any); - await expect(instrumentsService.deleteById('scalar')).rejects.toThrow(ForbiddenException); + await expect(instrumentsService.deleteById('scalar', adminUser)).rejects.toThrow(ForbiddenException); expect(instanceCache.has('scalar')).toBe(true); }); @@ -858,12 +921,12 @@ describe('InstrumentsService', () => { }); it('should return only the latest edition of each instrument by default', async () => { - const result = await instrumentsService.findInfo(); + const result = await instrumentsService.findInfo({}, adminUser); expect(result.map((info) => info.id)).toEqual(['id-2']); }); it('should return every edition when allEditions is set', async () => { - const result = await instrumentsService.findInfo({ allEditions: true }); + const result = await instrumentsService.findInfo({ allEditions: true }, adminUser); expect(result.map((info) => info.id)).toEqual(['id-1', 'id-2']); }); @@ -881,7 +944,7 @@ describe('InstrumentsService', () => { { id: 'shared', seriesGroupId: null, sourceRepoId: null, sourceRepoName: null } ]); - const result = await instrumentsService.findInfo(); + const result = await instrumentsService.findInfo({}, adminUser); expect(instrumentModel.findMany).toHaveBeenCalledWith({ select: { @@ -909,7 +972,7 @@ describe('InstrumentsService', () => { { createdAt, id: 'series-1', seriesGroupId: null, sourceRepoId: null, sourceRepoName: null } ]); - const result = await instrumentsService.findInfo(); + const result = await instrumentsService.findInfo({}, adminUser); expect(result).toMatchObject([{ createdAt, id: 'series-1' }]); }); @@ -920,7 +983,7 @@ describe('InstrumentsService', () => { { createdAt, id: 'id-2', seriesGroupId: null, sourceRepoId: null, sourceRepoName: null } ]); - const result = await instrumentsService.findInfo(); + const result = await instrumentsService.findInfo({}, adminUser); expect(result).toMatchObject([{ createdAt, id: 'id-2' }]); }); @@ -937,7 +1000,7 @@ describe('InstrumentsService', () => { { archivedAt: null, id: 'active', seriesGroupId: null, sourceRepoId: null, sourceRepoName: null } ]); - const result = await instrumentsService.findInfo(); + const result = await instrumentsService.findInfo({}, adminUser); expect(result).toMatchObject([ { archivedAt, id: 'archived' }, @@ -953,7 +1016,7 @@ describe('InstrumentsService', () => { ]); instrumentModel.findMany.mockResolvedValue([]); - const result = await instrumentsService.findInfo(); + const result = await instrumentsService.findInfo({}, adminUser); expect(result).toMatchObject([{ createdAt: null, id: 'series-1' }]); }); @@ -1178,7 +1241,7 @@ describe('InstrumentsService', () => { }); describe('count', () => { - it('should count the instruments matching the query', async () => { + it('should count every instrument', async () => { const find = vi.spyOn(instrumentsService, 'find').mockResolvedValue([existingSeries, formInstance('FORM_A', 1)]); await expect(instrumentsService.count()).resolves.toBe(2); @@ -1286,23 +1349,18 @@ describe('InstrumentsService', () => { }); it('should summarize each instrument by id, internal name and title', async () => { - await expect(instrumentsService.list()).resolves.toEqual([ + await expect(instrumentsService.list({}, adminUser)).resolves.toEqual([ { id: 'hash:FORM_A-1', internal: { edition: 1, name: 'FORM_A' }, title: 'FORM_A' } ]); }); - it('should not narrow the listing to any group when there is no current user', async () => { - await instrumentsService.list(); - expect(find).toHaveBeenCalledWith({}, { ability: undefined }, undefined); - }); - - it('should narrow the listing to a requested group the current user belongs to', async () => { + it("should narrow the listing to the current user's groups", async () => { const ability = createAppAbility([{ action: 'read', subject: 'Instrument' }]); const currentUser = createRequestUser(ability, [createGroup('group-1'), createGroup('group-2')]); - await instrumentsService.list({ kind: 'FORM' }, currentUser, 'group-2'); + await instrumentsService.list({ kind: 'FORM' }, currentUser); - expect(find).toHaveBeenCalledWith({ kind: 'FORM' }, { ability }, ['group-2']); + expect(find).toHaveBeenCalledWith({ kind: 'FORM' }, { ability }, ['group-1', 'group-2']); }); }); @@ -1310,7 +1368,7 @@ describe('InstrumentsService', () => { it('should not query stored metadata when no instrument matches', async () => { vi.spyOn(instrumentsService, 'find').mockResolvedValue([]); - await expect(instrumentsService.findInfo()).resolves.toEqual([]); + await expect(instrumentsService.findInfo({}, adminUser)).resolves.toEqual([]); expect(instrumentModel.findMany).not.toHaveBeenCalled(); }); @@ -1321,7 +1379,7 @@ describe('InstrumentsService', () => { { id: 'hash:FORM_B-1', sourceRepoId: 'repo-2', sourceRepoName: null } ]); - await expect(instrumentsService.findInfo()).resolves.toMatchObject([ + await expect(instrumentsService.findInfo({}, adminUser)).resolves.toMatchObject([ { id: 'hash:FORM_A-1', sourceRepo: { id: 'repo-1', name: 'Clinic Repo' } }, { id: 'hash:FORM_B-1', sourceRepo: { id: 'repo-2', name: null } } ]); @@ -1331,7 +1389,7 @@ describe('InstrumentsService', () => { vi.spyOn(instrumentsService, 'find').mockResolvedValue([formInstance('FORM_A', 2), formInstance('FORM_A', 1)]); instrumentModel.findMany.mockResolvedValue([]); - const result = await instrumentsService.findInfo(); + const result = await instrumentsService.findInfo({}, adminUser); expect(result.map(({ id }) => id)).toEqual(['hash:FORM_A-2']); }); @@ -1344,7 +1402,7 @@ describe('InstrumentsService', () => { ]); instrumentModel.findMany.mockResolvedValue([]); - const result = await instrumentsService.findInfo(); + const result = await instrumentsService.findInfo({}, adminUser); expect(result.find(({ id }) => id === existingSeries.id)).toMatchObject({ seriesItems: [{ id: 'hash:FORM_A-1' }, { id: 'hash:FORM_B-1' }] @@ -1355,7 +1413,7 @@ describe('InstrumentsService', () => { vi.spyOn(instrumentsService, 'find').mockResolvedValue([formInstance('FORM_A', 1), existingSeries]); instrumentModel.findMany.mockResolvedValue([]); - const result = await instrumentsService.findInfo(); + const result = await instrumentsService.findInfo({}, adminUser); expect(result.find(({ id }) => id === existingSeries.id)).toMatchObject({ seriesItems: [{ id: 'hash:FORM_A-1' }] @@ -1394,12 +1452,15 @@ describe('InstrumentsService', () => { it('should tag a multilingual series in each of its languages', async () => { vi.spyOn(instrumentsService, 'find').mockResolvedValue([]); - await instrumentsService.createSeries({ - details: { title: { en: 'Series', fr: 'Série' } }, - groupId: 'group-1', - items, - language: ['en', 'fr'] - }); + await instrumentsService.createSeries( + { + details: { title: { en: 'Series', fr: 'Série' } }, + groupId: 'group-1', + items, + language: ['en', 'fr'] + }, + adminUser + ); expect(bundledDefinition()).toMatchObject({ tags: { en: ['Series'], fr: ['Série'] } }); }); @@ -1407,24 +1468,30 @@ describe('InstrumentsService', () => { it('should trim every language of a multilingual title before storing it', async () => { vi.spyOn(instrumentsService, 'find').mockResolvedValue([]); - await instrumentsService.createSeries({ - details: { title: { en: ' Series ', fr: ' Série ' } }, - groupId: 'group-1', - items, - language: ['en', 'fr'] - }); + await instrumentsService.createSeries( + { + details: { title: { en: ' Series ', fr: ' Série ' } }, + groupId: 'group-1', + items, + language: ['en', 'fr'] + }, + adminUser + ); expect(bundledDefinition()).toMatchObject({ details: { title: { en: 'Series', fr: 'Série' } } }); }); it('should name the language whose title is blank, so the author knows which to fill in', async () => { await expect( - instrumentsService.createSeries({ - details: { title: { en: 'Series', fr: ' ' } }, - groupId: 'group-1', - items, - language: ['en', 'fr'] - }) + instrumentsService.createSeries( + { + details: { title: { en: 'Series', fr: ' ' } }, + groupId: 'group-1', + items, + language: ['en', 'fr'] + }, + adminUser + ) ).rejects.toThrowError(new UnprocessableEntityException("Instrument title cannot be blank for language 'fr'")); }); @@ -1432,19 +1499,20 @@ describe('InstrumentsService', () => { vi.spyOn(instrumentsService, 'find').mockResolvedValue([formInstance('FORM_A', 1)]); await expect( - instrumentsService.createSeries({ details: { title: 'Series' }, groupId: 'group-1', items, language: 'en' }) + instrumentsService.createSeries( + { details: { title: 'Series' }, groupId: 'group-1', items, language: 'en' }, + adminUser + ) ).resolves.toEqual({ instrumentId: 'created-id', outcome: 'created' }); }); }); describe('updateSeriesArchive (audit titles)', () => { - const currentUser = createRequestUser(createAppAbility([{ action: 'manage', subject: 'all' }])); - const archiveSeriesTitled = async (title: unknown) => { instrumentModel.findFirst.mockResolvedValue({ archivedAt: null, bundle: '__BUNDLE__', id: 'target' }); virtualizationService.eval.mockReturnValue(okAsync({ ...existingSeries, details: { title } })); instrumentModel.update.mockResolvedValue({ archivedAt: new Date(), id: 'target' }); - await instrumentsService.updateSeriesArchive('target', { isArchived: true }, currentUser); + await instrumentsService.updateSeriesArchive('target', { isArchived: true }, adminUser); return auditLogger.log.mock.lastCall?.[2].metadata; }; diff --git a/apps/api/src/instruments/instruments.controller.ts b/apps/api/src/instruments/instruments.controller.ts index 01049561b..d37ab02a7 100644 --- a/apps/api/src/instruments/instruments.controller.ts +++ b/apps/api/src/instruments/instruments.controller.ts @@ -85,12 +85,8 @@ export class InstrumentsController { @ApiOperation({ summary: 'List Instruments' }) @Get('list') @RouteAccess({ action: 'read', subject: 'Instrument' }) - async list( - @CurrentUser() currentUser: RequestUser, - @Query('groupId') groupId?: string, - @Query('kind') kind?: InstrumentKind - ) { - return this.instrumentsService.list({ kind }, currentUser, groupId); + async list(@CurrentUser() currentUser: RequestUser, @Query('kind') kind?: InstrumentKind) { + return this.instrumentsService.list({ kind }, currentUser); } @ApiOperation({ summary: 'Archive or Unarchive a Series Instrument' }) diff --git a/apps/api/src/instruments/instruments.service.ts b/apps/api/src/instruments/instruments.service.ts index d0e4c21ea..b288fbd19 100644 --- a/apps/api/src/instruments/instruments.service.ts +++ b/apps/api/src/instruments/instruments.service.ts @@ -83,11 +83,8 @@ export class InstrumentsService { private readonly virtualizationService: VirtualizationService ) {} - async count( - query: InstrumentQuery = {}, - options: EntityOperationOptions = {} - ): Promise { - return (await this.find(query, options)).length; + async count(options: EntityOperationOptions = {}): Promise { + return (await this.find({}, options)).length; } async create( @@ -170,9 +167,9 @@ export class InstrumentsService { */ async createSeries( { clientDetails, confirmDuplicate, details, groupId, items, language }: $CreateSeriesInstrumentData, - currentUser?: RequestUser + currentUser: RequestUser ): Promise { - const options = { ability: currentUser?.ability }; + const options = { ability: currentUser.ability }; const group = await this.groupModel.findFirst({ select: { id: true }, where: { AND: [accessibleQuery(options.ability, 'update', 'Group')], id: groupId } @@ -209,10 +206,10 @@ export class InstrumentsService { * instruments and never have records of their own (records belong to their constituent members). * Scalar instruments are shared platform assets and are never removed through this path. */ - async deleteById(id: string, currentUser?: RequestUser): Promise<{ id: string }> { + async deleteById(id: string, currentUser: RequestUser): Promise<{ id: string }> { const instrument = await this.instrumentModel.findFirst({ where: { - AND: [accessibleQuery(currentUser?.ability, 'delete', 'Instrument')], + AND: [accessibleQuery(currentUser.ability, 'delete', 'Instrument')], id } }); @@ -378,12 +375,12 @@ export class InstrumentsService { } async findInfo( - query: InstrumentInfoQuery = {}, - currentUser?: RequestUser, + query: InstrumentInfoQuery, + currentUser: RequestUser, requestedGroupId?: string ): Promise { - const groupIds = currentUser ? this.resolveGroupIds(currentUser, requestedGroupId) : undefined; - return this.findInfoWithinGroups(query, { ability: currentUser?.ability }, groupIds); + const groupIds = this.resolveGroupIds(currentUser, requestedGroupId); + return this.findInfoWithinGroups(query, { ability: currentUser.ability }, groupIds); } /** @@ -442,13 +439,8 @@ export class InstrumentsService { return instance; } - async list( - query: InstrumentQuery = {}, - currentUser?: RequestUser, - requestedGroupId?: string - ) { - const groupIds = currentUser ? this.resolveGroupIds(currentUser, requestedGroupId) : undefined; - return this.find(query, { ability: currentUser?.ability }, groupIds).then((arr) => { + async list(query: InstrumentQuery, currentUser: RequestUser) { + return this.find(query, { ability: currentUser.ability }, this.resolveGroupIds(currentUser)).then((arr) => { return arr.map((instrument) => ({ id: instrument.id, internal: instrument.internal, diff --git a/apps/api/src/sessions/__tests__/sessions.controller.test.ts b/apps/api/src/sessions/__tests__/sessions.controller.test.ts index 73e75f194..ef9d1fcf1 100644 --- a/apps/api/src/sessions/__tests__/sessions.controller.test.ts +++ b/apps/api/src/sessions/__tests__/sessions.controller.test.ts @@ -43,11 +43,4 @@ describe('SessionsController', () => { await expect(sessionsController.findAllIncludeUsernames(ability, 'group-1')).resolves.toBe(sessions); expect(sessionsService.findAllIncludeUsernames).toHaveBeenCalledWith('group-1', { ability }); }); - - it('should find a session by id within what the caller may read', async () => { - sessionsService.findById.mockResolvedValue({ id: 'session-1' }); - - await expect(sessionsController.findByID('session-1', ability)).resolves.toEqual({ id: 'session-1' }); - expect(sessionsService.findById).toHaveBeenCalledWith('session-1', { ability }); - }); }); diff --git a/apps/api/src/sessions/sessions.controller.ts b/apps/api/src/sessions/sessions.controller.ts index ea98c517a..e69428ddd 100644 --- a/apps/api/src/sessions/sessions.controller.ts +++ b/apps/api/src/sessions/sessions.controller.ts @@ -1,5 +1,5 @@ import { ApiOperation, CurrentUser } from '@douglasneuroinformatics/libnest'; -import { Body, Controller, Get, Param, Post, Query } from '@nestjs/common'; +import { Body, Controller, Get, Post, Query } from '@nestjs/common'; import { $CreateSessionData } from '@opendatacapture/schemas/session'; import type { SessionWithUser } from '@opendatacapture/schemas/session'; import type { Session } from '@prisma/client'; @@ -29,11 +29,4 @@ export class SessionsController { ): Promise { return this.sessionsService.findAllIncludeUsernames(groupId, { ability }); } - - @ApiOperation({ description: 'Find Session by ID' }) - @Get(':id') - @RouteAccess({ action: 'read', subject: 'Session' }) - findByID(@Param('id') id: string, @CurrentUser('ability') ability: AppAbility): Promise { - return this.sessionsService.findById(id, { ability }); - } } diff --git a/apps/api/src/subjects/__tests__/subjects.controller.test.ts b/apps/api/src/subjects/__tests__/subjects.controller.test.ts index 34dd55b81..4da5ec400 100644 --- a/apps/api/src/subjects/__tests__/subjects.controller.test.ts +++ b/apps/api/src/subjects/__tests__/subjects.controller.test.ts @@ -33,13 +33,6 @@ describe('SubjectsController', () => { subjectsService = moduleRef.get(SubjectsService); }); - it('should create the submitted subject', async () => { - subjectsService.create.mockResolvedValue({ id: 'subject-1' }); - - await expect(subjectsController.create({ id: 'subject-1' })).resolves.toEqual({ id: 'subject-1' }); - expect(subjectsService.create).toHaveBeenCalledWith({ id: 'subject-1' }); - }); - it('should delete a subject within what the caller may delete, forcing when asked', async () => { subjectsService.deleteById.mockResolvedValue({ id: 'subject-1' }); diff --git a/apps/api/src/subjects/subjects.controller.ts b/apps/api/src/subjects/subjects.controller.ts index 2b8db2846..ce6227b68 100644 --- a/apps/api/src/subjects/subjects.controller.ts +++ b/apps/api/src/subjects/subjects.controller.ts @@ -1,7 +1,6 @@ import { $BooleanLike } from '@douglasneuroinformatics/libjs'; import { ApiOperation, CurrentUser, ParseSchemaPipe, ValidObjectIdPipe } from '@douglasneuroinformatics/libnest'; -import { Body, Controller, Delete, Get, Param, Post, Query } from '@nestjs/common'; -import { $CreateSubjectData } from '@opendatacapture/schemas/subject'; +import { Controller, Delete, Get, Param, Query } from '@nestjs/common'; import z from 'zod/v4'; import type { AppAbility } from '@/auth/auth.types'; @@ -13,13 +12,6 @@ import { SubjectsService } from './subjects.service'; export class SubjectsController { constructor(private readonly subjectsService: SubjectsService) {} - @ApiOperation({ summary: 'Create Subject' }) - @Post() - @RouteAccess({ action: 'create', subject: 'Subject' }) - create(@Body() subject: $CreateSubjectData) { - return this.subjectsService.create(subject); - } - @ApiOperation({ summary: 'Delete Subject' }) @Delete(':id') @RouteAccess({ action: 'delete', subject: 'Subject' }) diff --git a/apps/api/src/users/__tests__/users.service.test.ts b/apps/api/src/users/__tests__/users.service.test.ts index fb572cfee..78062ad45 100644 --- a/apps/api/src/users/__tests__/users.service.test.ts +++ b/apps/api/src/users/__tests__/users.service.test.ts @@ -145,26 +145,6 @@ describe('UsersService', () => { }); }); - describe('deleteByUsername', () => { - it("should delete the found user's record within the caller's delete scope", async () => { - userModel.findFirst.mockResolvedValue({ id: 'user-1', username: 'jane.doe' }); - userModel.delete.mockResolvedValue({ id: 'user-1' }); - await expect(usersService.deleteByUsername('jane.doe', { ability: admin.ability })).resolves.toEqual({ - id: 'user-1' - }); - expect(userModel.delete.mock.lastCall?.[0].where).toEqual({ - AND: [accessibleQuery(admin.ability, 'delete', 'User')], - id: 'user-1' - }); - }); - - it('should throw when no user has the username, rather than deleting anything', async () => { - userModel.findFirst.mockResolvedValue(null); - await expect(usersService.deleteByUsername('jane.doe')).rejects.toThrow(NotFoundException); - expect(userModel.delete).not.toHaveBeenCalled(); - }); - }); - describe('find', () => { beforeEach(() => { userModel.findMany.mockResolvedValue([{ id: 'user-1' }]); diff --git a/apps/api/src/users/users.service.ts b/apps/api/src/users/users.service.ts index ccb94a575..0df44e821 100644 --- a/apps/api/src/users/users.service.ts +++ b/apps/api/src/users/users.service.ts @@ -127,17 +127,6 @@ export class UsersService { }); } - /** Delete the user with the provided username, otherwise throws */ - async deleteByUsername(username: string, { ability }: EntityOperationOptions = {}) { - const user = await this.findByUsername(username); - return this.userModel.delete({ - omit: { - hashedPassword: true - }, - where: { AND: [accessibleQuery(ability, 'delete', 'User')], id: user.id } - }); - } - async find({ groupId }: { groupId?: string } = {}, { ability }: EntityOperationOptions = {}) { return this.userModel.findMany({ omit: { diff --git a/apps/gateway/AGENTS.md b/apps/gateway/AGENTS.md index bb0dd7d88..c2a8ecc89 100644 --- a/apps/gateway/AGENTS.md +++ b/apps/gateway/AGENTS.md @@ -51,8 +51,7 @@ both extend `BaseServer`, which owns the middleware order: Reordering those lines is a security change. Wrap every async handler in `ah()` (`src/utils/async-handler.ts`); an unwrapped rejection never reaches `errorHandlerMiddleware`. -Throw `HttpException(status, message)`. `src/middleware/not-found.middleware.ts` is defined but -never mounted — an unknown path currently gets Express's built-in 404. +Throw `HttpException(status, message)`. An unknown path gets Express's built-in 404. Auth is not the web app's JWT. Two credentials, both sent as `Authorization: Bearer …`: `config.apiKey` (`GATEWAY_API_KEY`, used by `apps/api`), and a per-assignment token diff --git a/apps/gateway/package.json b/apps/gateway/package.json index c9ef9f2a3..96e9f8ccc 100644 --- a/apps/gateway/package.json +++ b/apps/gateway/package.json @@ -23,21 +23,17 @@ "@douglasneuroinformatics/libcrypto": "catalog:", "@douglasneuroinformatics/libui": "catalog:", "@opendatacapture/react-core": "workspace:*", - "@opendatacapture/runtime-core": "workspace:*", "@opendatacapture/runtime-v1": "workspace:*", "@opendatacapture/schemas": "workspace:*", "@prisma/client": "catalog:", "axios": "catalog:", "compression": "^1.8.2", "express": "^5.2.1", - "lodash-es": "workspace:lodash-es__4.x@*", "pino": "^10.3.1", "pino-http": "^11.0.0", - "pino-pretty": "^13.1.3", "react": "workspace:react__19.x@*", "react-dom": "workspace:react-dom__19.x@*", "sirv": "^3.0.2", - "sqlite3": "^6.0.1", "zod": "workspace:zod__3.x@*" }, "devDependencies": { @@ -52,15 +48,10 @@ "@vitejs/plugin-react-swc": "^4.3.3", "esbuild": "catalog:", "happy-dom": "catalog:", - "nodemon": "catalog:", "prisma": "catalog:", "tailwindcss": "catalog:", "tsx": "catalog:", "type-fest": "workspace:type-fest__4.x@*", "vite": "catalog:" - }, - "trustedDependencies": [ - "prisma", - "sqlite3" - ] + } } diff --git a/apps/gateway/scripts/dev.ts b/apps/gateway/scripts/dev.ts index 81ad10d78..c815a4d2a 100644 --- a/apps/gateway/scripts/dev.ts +++ b/apps/gateway/scripts/dev.ts @@ -3,7 +3,6 @@ /* eslint-disable no-console */ import fs from 'fs'; -import module from 'module'; import path from 'path'; import { getReleaseInfo } from '@opendatacapture/release-info'; @@ -12,8 +11,6 @@ import esbuild from 'esbuild'; const outdir = path.resolve(import.meta.dirname, '../dist'); const tsconfig = path.resolve(import.meta.dirname, '../tsconfig.json'); -const require = module.createRequire(import.meta.url); - if (fs.existsSync(outdir)) { await fs.promises.rm(outdir, { recursive: true }); } @@ -42,20 +39,6 @@ const ctx = await esbuild.context({ outdir, platform: 'node', plugins: [ - { - name: 'raw', - setup(build) { - build.onResolve({ filter: /^.*\?raw$/ }, (args) => { - return { - namespace: 'raw', - path: require.resolve(args.path, { paths: [path.dirname(args.importer)] }) - }; - }); - build.onLoad({ filter: /.*/, namespace: 'raw' }, async (args) => { - return { contents: await fs.promises.readFile(args.path, 'utf-8'), loader: 'text' }; - }); - } - }, { name: 'rebuild', setup(build) { diff --git a/apps/gateway/src/middleware/__tests__/not-found.middleware.test.ts b/apps/gateway/src/middleware/__tests__/not-found.middleware.test.ts deleted file mode 100644 index 1e4c695c5..000000000 --- a/apps/gateway/src/middleware/__tests__/not-found.middleware.test.ts +++ /dev/null @@ -1,26 +0,0 @@ -import type { Request, Response } from 'express'; -import { describe, expect, it, vi } from 'vitest'; - -import { notFoundMiddleware } from '../not-found.middleware'; - -function createResponse() { - const end = vi.fn(); - const set = vi.fn(() => ({ end })); - const status = vi.fn(() => ({ set })); - return { end, response: { status } as unknown as Response, set, status }; -} - -describe('notFoundMiddleware', () => { - it('should respond with a 404 status', () => { - const { response, status } = createResponse(); - notFoundMiddleware({} as Request, response, vi.fn()); - expect(status).toHaveBeenCalledWith(404); - }); - - it('should send an HTML page, so a browser renders the message rather than showing raw text', () => { - const { end, response, set } = createResponse(); - notFoundMiddleware({} as Request, response, vi.fn()); - expect(set).toHaveBeenCalledWith({ 'Content-Type': 'text/html' }); - expect(end).toHaveBeenCalledWith(expect.stringContaining('

404 - Not Found

')); - }); -}); diff --git a/apps/gateway/src/middleware/not-found.middleware.ts b/apps/gateway/src/middleware/not-found.middleware.ts deleted file mode 100644 index 91fa835f7..000000000 --- a/apps/gateway/src/middleware/not-found.middleware.ts +++ /dev/null @@ -1,12 +0,0 @@ -import type { RequestHandler } from 'express'; - -export const notFoundMiddleware: RequestHandler = (_, res) => { - res - .status(404) - .set({ 'Content-Type': 'text/html' }) - .end( - `
-

404 - Not Found

-
` - ); -}; diff --git a/apps/gateway/src/routers/__tests__/api.router.test.ts b/apps/gateway/src/routers/__tests__/api.router.test.ts index 3d83cd546..f8112e6cf 100644 --- a/apps/gateway/src/routers/__tests__/api.router.test.ts +++ b/apps/gateway/src/routers/__tests__/api.router.test.ts @@ -147,17 +147,11 @@ describe('$CreateRemoteAssignmentsData', () => { }); describe('GET /assignments', () => { - it('should filter by the subject named in the query, so the API sees only the assignments of that subject', async () => { + it('should list every assignment, since the API reconciles the whole set on each sync', async () => { prisma.remoteAssignmentModel.findMany.mockResolvedValueOnce([{ id: 'assignment-1', status: 'OUTSTANDING' }]); - const response = await request('GET', '/assignments?subjectId=subject-1'); + const response = await request('GET', '/assignments'); expect(await response.json()).toEqual([{ id: 'assignment-1', status: 'OUTSTANDING' }]); - expect(prisma.remoteAssignmentModel.findMany).toHaveBeenCalledWith({ where: { subjectId: 'subject-1' } }); - }); - - it('should ignore a repeated subject query rather than filtering on an array', async () => { - prisma.remoteAssignmentModel.findMany.mockResolvedValueOnce([]); - await request('GET', '/assignments?subjectId=a&subjectId=b'); - expect(prisma.remoteAssignmentModel.findMany).toHaveBeenCalledWith({ where: { subjectId: undefined } }); + expect(prisma.remoteAssignmentModel.findMany).toHaveBeenCalledWith(); }); }); diff --git a/apps/gateway/src/routers/api.router.ts b/apps/gateway/src/routers/api.router.ts index a6e5bc9ae..fe3a887dc 100644 --- a/apps/gateway/src/routers/api.router.ts +++ b/apps/gateway/src/routers/api.router.ts @@ -24,16 +24,8 @@ const router = Router(); router.get( '/assignments', - ah(async (req, res) => { - let subjectId: string | undefined; - if (typeof req.query.subjectId === 'string') { - subjectId = req.query.subjectId; - } - const assignments = await prisma.remoteAssignmentModel.findMany({ - where: { - subjectId - } - }); + ah(async (_, res) => { + const assignments = await prisma.remoteAssignmentModel.findMany(); return res.status(200).json( assignments.map((assignment) => { return { diff --git a/apps/gateway/src/server/__tests__/server.base.test.ts b/apps/gateway/src/server/__tests__/server.base.test.ts index 66348ee07..d6fb62d63 100644 --- a/apps/gateway/src/server/__tests__/server.base.test.ts +++ b/apps/gateway/src/server/__tests__/server.base.test.ts @@ -89,7 +89,7 @@ class StacktraceFixingServer extends TestServer { const openServers: Server[] = []; async function request(server: BaseServer, path: string, init?: RequestInit) { - const httpServer = server.listen(0); + const httpServer = server.listen(); openServers.push(httpServer); await once(httpServer, 'listening'); const address = httpServer.address(); @@ -213,7 +213,7 @@ describe('BaseServer', () => { }); describe('listen', () => { - it('should listen on the configured port by default and log where it started, so the operator knows where to connect', async () => { + it('should listen on the configured port and log where it started, so the operator knows where to connect', async () => { const logInfo = vi.spyOn(logger, 'info'); const httpServer = new TestServer().listen(); openServers.push(httpServer); diff --git a/apps/gateway/src/server/server.base.ts b/apps/gateway/src/server/server.base.ts index 50fc439fe..bd7ff13bc 100644 --- a/apps/gateway/src/server/server.base.ts +++ b/apps/gateway/src/server/server.base.ts @@ -70,9 +70,9 @@ export abstract class BaseServer { protected fixStacktrace?(err: Error): void; - listen(port = config.port) { - return this.app.listen(port, () => { - logger.info(`Server started at http://localhost:${port}`); + listen() { + return this.app.listen(config.port, () => { + logger.info(`Server started at http://localhost:${config.port}`); }); } diff --git a/apps/gateway/src/vite-env.d.ts b/apps/gateway/src/vite-env.d.ts index 83bd29e5f..b9c0667f3 100644 --- a/apps/gateway/src/vite-env.d.ts +++ b/apps/gateway/src/vite-env.d.ts @@ -16,9 +16,5 @@ declare global { __ROOT_PROPS__: RootProps; } - interface ImportMeta { - readonly env: ImportMetaEnv; - } - const __RELEASE__: ReleaseInfo; } diff --git a/apps/outreach/package.json b/apps/outreach/package.json index 4420e7227..3e39b5de7 100644 --- a/apps/outreach/package.json +++ b/apps/outreach/package.json @@ -35,7 +35,6 @@ "github-slugger": "^2.0.0", "sharp": "^0.34.5", "tailwindcss": "catalog:", - "type-fest": "workspace:type-fest__4.x@*", "typedoc": "0.27.x", "typedoc-plugin-markdown": "4.4.x" } diff --git a/apps/outreach/src/assets/examples/4.1-instruments/index.ts b/apps/outreach/src/assets/examples/4.1-instruments/index.ts deleted file mode 100644 index dfe263cdd..000000000 --- a/apps/outreach/src/assets/examples/4.1-instruments/index.ts +++ /dev/null @@ -1,90 +0,0 @@ -/* eslint-disable */ -// @ts-nocheck - -const formInstrument = { - kind: 'FORM', - language: 'en', - tags: ['Example'], - internal: { - edition: 1, - name: 'HAPPINESS_QUESTIONNAIRE' - }, - content: { - overallHappiness: { - description: 'Please select a number from 1 to 10 (inclusive)', - kind: 'number', - label: 'How happy are you overall?', - max: 10, - min: 1, - variant: 'slider' - } - }, - details: { - description: 'The Happiness Questionnaire is a questionnaire about happiness.', - estimatedDuration: 1, - instructions: ['Please answer the questions based on your current feelings.'], - license: 'Apache-2.0', - title: 'Happiness Questionnaire' - }, - measures: null, - validationSchema: z.object({ - overallHappiness: z.number().int().min(1).max(10) - }) -}; - -const interactiveInstrument = { - kind: 'INTERACTIVE', - language: 'en', - tags: ['EXAMPLE'], - internal: { - edition: 1, - name: 'CLICK_THE_BUTTON_TASK' - }, - content: { - render(done) { - // the timestamp when the render function is first called - const start = Date.now(); - - // create a