From e6bfff708712fa797c50be4102814341d2607cc9 Mon Sep 17 00:00:00 2001 From: joshunrau Date: Mon, 5 Oct 2026 19:15:15 -0400 Subject: [PATCH 01/16] chore: configure knip for every workspace Knip only covered apps/api and apps/web, and most of what it reported elsewhere was noise: web's vite config and the playwright config failed to load without .env, storybook's config dir, astro's docs content, the api's worker thread and test suites, the runtime config and the instrument build entries were all invisible to it. - Load .env and NODE_ENV in the knip script so every config loads. - Ignore vendor/ and report unused entry exports of every internal package; published packages keep their public surface. - Register the entry points knip's plugins cannot see, and mark the production ones with `!` so `knip --production` finds code that only tests reach. - Derive runtime/v1's ignored dependencies from runtime.config.js instead of listing them twice. Co-Authored-By: Claude Opus 5.5 --- knip.ts | 149 +++++++++++++++++++++++++++++++++++++++++++++++++-- package.json | 2 +- 2 files changed, 146 insertions(+), 5 deletions(-) diff --git a/knip.ts b/knip.ts index 7887f6baa..20021f80d 100644 --- a/knip.ts +++ b/knip.ts @@ -1,14 +1,155 @@ import type { KnipConfig } from 'knip'; +import runtimeConfig from './runtime/v1/runtime.config.js'; + const config: KnipConfig = { + ignoreBinaries: ['env-cmd!'], + ignoreIssues: { + // module.exports is the function actions/github-script require()s in .github/workflows + '.github/scripts/*.cjs': ['exports'], + // reached through the /runtime/v1/* tsconfig paths; vendor wrappers resolve their peers from runtime/v1 + 'vendor/**': ['unlisted'] + }, + // instrument-stubs only serves test and Storybook fixtures + ignoreWorkspaces: ['vendor/**', 'packages/instrument-stubs!'], + includeEntryExports: true, workspaces: { + '.': { + entry: ['scripts/increment-version.ts', '.github/scripts/*.cjs'], + ignoreBinaries: ['turbo'], + // the JSDoc type of the arguments actions/github-script injects, not an npm package + ignoreDependencies: ['github-script', '@astrojs/starlight'], + // docs/**/*.mdx is outreach content, compiled through the aliases in its astro.config.ts + paths: { '@/components/*': ['apps/outreach/src/components/*'] }, + project: ['**/*.{cjs,js,ts}', '!.agents/**'] + }, 'apps/api': { - entry: ['src/main.ts', 'libnest.config.ts'], - ignoreDependencies: ['@opendatacapture/runtime-v1', 'prisma-json-types-generator', 'lodash-es', 'ts-pattern'], - project: '**/*.{js,ts}' + entry: [ + 'libnest.config.ts!', + // spawned as a worker thread by path + 'src/instrument-records/export-worker.js!', + // test/app.test.ts imports the suites by reading the directory + 'test/**/*.ts' + ], + ignoreIssues: { + 'libnest.config.ts': ['exports'], + 'test/suites/*.suite.ts': ['exports'] + } + }, + 'apps/gateway': { + // scripts/dev.ts runs behind env-cmd, which knip does not parse past + entry: ['src/main.ts!', 'src/entry-server.tsx!', 'scripts/*.ts'], + // generated into node_modules by `prisma generate` + ignoreDependencies: ['@prisma/generated-client'], + // `render` is imported at runtime from the SSR build output + ignoreIssues: { 'src/entry-server.tsx': ['exports'] } + }, + 'apps/outreach': { + // theme-observer.js is read with fs and starlight.css passed through path.resolve in astro.config.ts + entry: ['../../docs/**/*.mdx!', 'src/scripts/theme-observer.js!', 'src/styles/starlight.css!'], + // resolved through runtime-meta's generateMetadata + ignoreDependencies: ['@opendatacapture/runtime-v1'], + // src/plugins are Astro build integrations, imported only by astro.config.ts + project: ['**/*.{js,mjs,cjs,jsx,ts,tsx,mts,cts,astro,mdx,css}!', '!src/plugins/**!'] + }, + 'apps/playground': { + // preview.html is a second Vite page; the vite plugin only reads index.html + entry: ['src/preview/main.tsx!'], + ignoreIssues: { + // loaded as raw text through import.meta.glob and compiled by the instrument bundler + 'src/instruments/*/*/*/index.{js,jsx,ts,tsx}': ['exports'], + // a port of CodeMirror's vim bindings, treated as third-party (see AGENTS.md) + 'src/vim/**': ['exports', 'types'] + } }, 'apps/web': { - ignoreDependencies: ['lodash-es', 'papaparse', 'ts-pattern'] + project: ['**/*.{ts,tsx,css}!', '!src/testing/**!', '!src/**/__tests__/**!'], + // vite.config.ts passes generatedRouteTree to tanstackRouter() inline; the plugin only reads tsr.config.json + 'tanstack-router': { + entry: ['src/route-tree.ts'] + } + }, + 'packages/instrument-bundler': { + // instruments that src/__tests__/repositories/index.ts reads from disk for the tests to bundle + entry: ['src/__tests__/repositories/*/index.{ts,tsx}'], + // tsc resolves react/jsx-runtime for the JSX in those instruments + ignoreDependencies: ['react'], + ignoreIssues: { + // vendored from parse-imports, treated as third-party (see AGENTS.md) + 'src/parse.ts': ['exports', 'types'] + }, + includeEntryExports: false, + project: ['**/*.{js,ts,tsx}!', '!src/**/__tests__/**!'] + }, + 'packages/instrument-library': { + // each instrument directory is compiled by the instrument-bundler CLI into dist/, which apps/api imports + entry: ['src/{file,forms,interactive,series}/*/index.{js,jsx,ts,tsx}!', 'scripts/*.ts'], + // `build` runs the bundler CLI by path + ignoreDependencies: ['@opendatacapture/instrument-bundler'], + // tsconfig `jsxImportSource`, mapped onto runtime/v1/dist by tsconfig `paths` + ignoreUnresolved: ['/runtime/v1/react@19.x'], + includeEntryExports: false + }, + 'packages/licenses': { + // bundled into runtime/v1, so its whole surface is public API + includeEntryExports: false + }, + 'packages/playground-url': { + entry: ['src/cli.ts!', 'scripts/*.js'], + includeEntryExports: false + }, + 'packages/react-core': { + // tsconfig `paths` maps /runtime/v1/* onto runtime/v1/dist for the types in zodErrorMap.ts + ignoreDependencies: ['@opendatacapture/runtime-v1'] + }, + 'packages/release-info': { + // called only from build configs, which --production does not analyse + includeEntryExports: false + }, + 'packages/runtime-bundler': { + // npm packages that test/e2e.test.ts copies into a temporary node_modules and bundles + ignoreFiles: ['test/fixtures/**'] + }, + 'packages/runtime-core': { + // served to instrument authors through runtime/v1 + entry: ['src/index.ts!', 'src/constants.ts!', 'src/**/__tests__/*.test-d.ts'], + includeEntryExports: false + }, + 'packages/runtime-internal': { + ignoreIssues: { 'src/index.d.ts': ['exports', 'types'] } + }, + 'packages/runtime-meta': { + ignoreIssues: { 'src/index.d.ts': ['exports', 'types'] } + }, + 'packages/serve-instrument': { + // scripts/build.js bundles src/client.tsx into dist/client.js, which the server inlines into each page + entry: ['src/cli.ts!', 'src/client.tsx!', 'scripts/*.js'], + // kept external when the CLI is bundled, so it is imported at runtime + ignoreDependencies: ['esbuild!'], + includeEntryExports: false + }, + 'packages/vite-plugin-runtime': { + // its only consumers are Vite configs, which --production does not analyse + includeEntryExports: false + }, + 'runtime/v1': { + // the runtime-bundler CLI imports it from the working directory + entry: ['runtime.config.js!'], + // runtime-bundler resolves every `include` entry from this package's node_modules + ignoreDependencies: runtimeConfig.include, + includeEntryExports: false + }, + storybook: { + project: ['**/*.{css,js,mdx,ts}'], + storybook: { + config: ['config/main.ts'], + entry: ['config/preview.ts'] + } + }, + testing: { + // generated by scripts/gen-routes.ts + ignoreIssues: { 'src/generated/route.d.ts': ['types'] }, + project: ['**/*.ts'] } } }; diff --git a/package.json b/package.json index 823f694b5..87bb7ec0f 100644 --- a/package.json +++ b/package.json @@ -21,7 +21,7 @@ "force-reinstall": "pnpm clean && pnpm install", "format": "turbo run format", "generate:env": "./scripts/generate-env.sh", - "knip": "knip", + "knip": "NODE_ENV=development env-cmd knip", "lint": "turbo run lint lint:root", "lint:root": "tsc --noEmit && eslint --fix commitlint.config.ts knip.ts scripts vitest.config.ts", "postinstall": "turbo telemetry disable", From b82fe8bdbfc438ed2e7728d5b886c8904706dfa7 Mon Sep 17 00:00:00 2001 From: joshunrau Date: Mon, 5 Oct 2026 20:19:57 -0400 Subject: [PATCH 02/16] refactor(api): remove routes no client calls and other dead code Remove the three routes that neither web, playground, the gateway, odc-cli nor the e2e suite calls: POST /v1/subjects, GET /v1/instrument-repos/:id and GET /v1/sessions/:id, with the service method only one of them used. Also remove service methods only their own tests reached (UsersService.deleteByUsername, InstrumentRecordsService.exists), the subjectId parameter of GatewayService.fetchRemoteAssignments no caller passes, the never-passed groupId query on GET /v1/instruments/list, the optional currentUser on the instrument service methods every caller already passes it to, the unused InstrumentKind and InstrumentInternal Prisma declarations, the unserved public/ directory, and exports used only in their own file. Co-Authored-By: Claude Opus 5.5 --- .../docs/architecture/auth-and-permissions.md | 32 +- .agents/skills/odc-api/SKILL.md | 5 +- apps/api/Dockerfile | 2 - apps/api/prisma/schema.prisma | 12 - apps/api/public/favicon.ico | Bin 15086 -> 0 bytes apps/api/public/index.html | 14 - apps/api/public/styles.css | 4 - apps/api/src/auth/ability.factory.ts | 2 +- apps/api/src/auth/auth.types.ts | 4 +- .../core/decorators/route-access.decorator.ts | 6 +- .../gateway/__tests__/gateway.service.test.ts | 10 +- apps/api/src/gateway/gateway.service.ts | 8 +- .../instrument-records.service.test.ts | 10 - .../instrument-records.service.ts | 4 - .../instrument-repos.controller.test.ts | 7 - .../instrument-repos.service.test.ts | 12 - .../instrument-repos.controller.ts | 7 - .../instrument-repos.service.ts | 10 - .../__tests__/instruments.controller.test.ts | 6 +- .../__tests__/instruments.service.test.ts | 404 ++++++++++-------- .../src/instruments/instruments.controller.ts | 8 +- .../src/instruments/instruments.service.ts | 32 +- .../__tests__/sessions.controller.test.ts | 7 - apps/api/src/sessions/sessions.controller.ts | 9 +- .../__tests__/subjects.controller.test.ts | 7 - apps/api/src/subjects/subjects.controller.ts | 10 +- .../src/users/__tests__/users.service.test.ts | 20 - apps/api/src/users/users.service.ts | 11 - 28 files changed, 283 insertions(+), 380 deletions(-) delete mode 100644 apps/api/public/favicon.ico delete mode 100644 apps/api/public/index.html delete mode 100644 apps/api/public/styles.css 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/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/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/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 c0cc8178743989b2db1c15e5b0b138d18bfb9bd9..0000000000000000000000000000000000000000 GIT binary patch literal 0 HcmV?d00001 literal 15086 zcmeHO33QFu_P?6G*RC28Ln+!yRm~)(mY_nZr>K|mOi?AMzA7P;`DLVrAS4k)A|$3} zl#qxBR}3*vL4-&%nTe<=kzv2zKDpoZ-J5%J6a3rtU(a1@XMe-_&i?IvrhWE4Q7Eb@ zj1=bP3UE`!w$Bub&lL)Vsj2GTM4`yQGZS!r|I^zF#X=Nli8A;^F%HDf#jjZzd-L~= z1>RWTud#rGnTfHznMp@GGm{aZ8FuYW=HUJ(T=xR6^VgsEO109^v-sC>0 z7(_C9jCV(%hYj?oVYtRWuLNw~3)qeUYl#f69J~Cw5M_g)L&HB0p{AtFs8RJYB1V3m z9b?^f<0P}bz;kIvI}KaQ@#?(BH(Q{0ZOxir@12vG$vci_Ce>$}nf#q=ru=!;fZ6I= z_{d{d!$#z%hmCxAexp@h&?K``r_Ob0dRt?iSfw_Z4jrerGZy7gws?A*T28MQu|fg7 ze?!|^fPO_^-32-TikZ>Qc=6JKrn3$%?|&VbDeborlpa1(P}*<9Y0rY*v}B+u*_+oU zTj(Jt#_Z6c4$bLWpIm!4rp4cw(h93)T4UXsqUZFW<7)=d{je$_1Q|>RyEo?m>d7?RrCOM6WDnJ@ERP0Y)`f?>o(7# z0r?DXK|AMvNoNB8scsY2>C%>AbasOk9a#1?h1qwd)uUR`g1#To>@Vu0o$8Qk%#^}j zw;2i@}(@F>xr>gmz}m>UDL1Iio+J(1()YumQ9XkM=t6t&caa#AnQ!@GCsPIflk z&iwtAiR*cspO1PnC~U!OjDhz>ztYBl`yyZ)C&xdtqcQjK{&=o;on)J@8atS-ojOh> zB_+haB50TQE%bXCltNhgB&|G)Tsk-MgZ@- zk`A~Yif1=;=qmam+ue7972V6t7WGSiV!fBp+MmCtyjwRZ|ITg7ziog}=5B5drJXoN zxmVIC{nQD{x^#iEE?%Jh8~rJAiMv!a6%`gztoLHHL%nk20a&jBb5q6--U;^^`mj$2 zcGrGplzrj6x^C$r&c~A$4e3db^6pEI4Dv@0@@RiZ5dD7sEFBKpN|z2N(YeHU%7GpM z(|)99j~}b+c=qTITK~%!F-K_OyN5B*9sT`XU=PuHr=c(Rn_a6Gsq2(q>|VW$oVt8W z8D~z)A6Ix?T3SlQMMa_y3SL|+uAe;2C;wk2(Df^qHNL-m(TY{uVuf)UL=h2CnNR2w4J$ghd#|$jj z2C#b|Xqi6j{M@!}OVfK*4Q zn-ke$-H>Ai_Dm4ZHyY37_q?`vW6mjj_Ef4$gU9=MQN*uvY5mmk^vAus8t-1_Ci}uU zjDbm1AR7Z*Pb|inZ5z#X4cK|FpwkzVbYT44b_GwK=!CO-^$JQ1^r20TKU4b2V>-)J z<)Ih@Q*7vl)){QD=N|U~T`2Fy4V^G{uU<|Ew+0fAV`cDqon@-} zFwA|f#uzBU_ztq00Q}|pPb%=|>%#rw8GLiBA?NnS>4d@f4{h5(7nBLKb)Ma883RYd zw^Gy+H?_R`SaUoEbb`-PuK)M~|M?eKb&mt?LjktqF$SKg>y$1e`{&)rrPUMR6Cd1% zb>f_uuXxU?3T0nBFV+C%CVvW^IhEM%{M*05591U5wB|Uy4(#1Q8?>HlmlN}@W9No+ zI5b#-t>%nBX>(b>IT98^K@O8J7A9AnoF>!KVPBEYI4cU7J%uhO9~3fJf4BbTx@_;` zXl`PJ`QIA&pBd7D+irfZmUKEcO6~iw06BhMmx`Vj3Mwh#HL3E%--a!8q2tlvYTM*q zzDO%Z^(%)RSb0w~PUSm%T$w(^`mqjkEIz?nypZn0htb9_3FbK@Icy6lgVvIA#3i zWZ!?Xgk9qBg`9!A)DU*@9{6W<`-C!qk>`l?lgZE{iq7rXAt?9q1>*i()gSvgzBX3n z)+sqAU=K$uQf+$)Km5>!c$XwEThX z(gE}7Ca0e?_DhCzd0tRJkxO04V^CLe|E3Gg?Ak=c1b9x+XDi?dg`N9GoGMjC-&=;f%^wwsIJS#m3@Y} zsy^zOhx&MZ&^y0!d}40TWfxdJyb_bn`i>7@W*U|=uA8Ed2Ytc_CnAfHL^B9=t zgI-%M33KaPsIN816?&ZEcGa?%kj;IV=Oy0n+nLovyCDA57JE>BRzXZqRTuj-S0G~o z*GsSk+bd1FAreMC33goS0T?g zCh=ZrRK{JZ30|NOycjMy-{->u&fuBgO;p?%T#5U4z+J%Mg%yJ_=i)e^q{viprHZ@A zIbT=hC(RDvF8m$jXmce$2cAqh2Tp+pxC{6>xWL&wg0n&SzGBYjv`j&kAoV#dnDWi<|IPwfL--5;=h8o9j(daqg4cltzF=LfXX8Pk zSd-6!t}(~GKi2PV$a%i?I^@Xf$cY|hM3-&<4%y9-|JV(A`f}Je85_=Wyfg-!2Qh*U zHJREe6mMOR`@Vff)TnhCk)z_{-7U{Jf7uW|EY5eR_6*W_59urg{Kh+=zH7xC`B zDb(%@S~s>0c@Az$E|!g+;f%>%=wOY0`JN#Z(6cjSXMYu??X%fRYrU*m0k4D?#Kf+(%vVq-)b+H51CEyjL*Pafn1Th z;C-meS8XW4f2GKKoZb^lM|VV2K#_DT`UM@2iK1H>*XY!)2y#X}Vqg0r)_n=GM9c=V z&2X)G76Gys4*HzZPaF|5ectsP3Yz&@OY`61;;pIo1Ko^A-?55oNMr%FpTa4hrS=@ce9X7&bpW) z_Qjd!&(Pk0HH2W2_~byyI_%x*O8ZHUcRGWnL1v*`KKC2%af3y?SCW-?Jy)E`NIj&K z9vS2XPaY$`wv0IL%)flZb$orx2*|+|Ui@ z?sHQwiee>2#mKje#kf%Cn;t-3xtJEqkM?xYsQO+`HgZ2M&}S(dxih+Mc-)xGR2#JyM2o;6-{FDFZSY>?m1NTKN_t^bltzQV;E?SzXon`VzGL5)iLl>OLu5^H^nDNqvXqCnA3G$Gtmr1oh+d}>?xG&cRT*l6<4lqoh8&^~54$j^r z2KcDQ68Fc8$~~Y2opdQFk#>457O|_Wlr!Wv={xxx%=+%b*slpP1%`7fOy%srV{y?C z3ra)YYp2&j^yLf4<(;RP6$|KS_+~nn5QEsf4vLSatH%$ExpPkUPqgPg9{Y2p`4`u= zkgwB!;B|xJSDZuSJc@FYAM!?ibouapgE4B(7kU3M2GVFR$|;e#yKPD$8#FbM-hwWqMaIu@uek~ z<5X8sr=ua+l08taHp))}aeJt7@VsQ-;eGfPeQ3L@1MOJuhS=}F$>SU3;Q4H&rmYz} z+ndjmHFq?xE$tp9F$OulYlQEuK_Q^asP8FA<$u)>^C-{NT7Ct`v>&28#~^J{$1BHc zk>7Y5^)v>3jXLLpwt@J(KF4fNU&jfREreZ}e#8WuzV2t>L5ke3~nFu2&K`v#a%QVtEBa~x$ zCllWz?qQ6WM=#XlOo;4IK@-S$@#8oJhb2rw>Nv&QxcQTRQ(DxhiM~7tG!rxd)Tkl| z=y&f-ZCRsk!h+r({vI`Yl;7x9ThJd1cWu8U- zi$Rmx7e~*rppc(C(CX1G$#-N6^7^h>sYl-r18_F%Q>hNfpPD|`m7;6!wo@GD;QhO#TJ=L*hC@I1eKWFPjI3-GPT z4Y7R_16PTH?Bnnrg?+0Bw{Fmbo7V-;yOD!)S9@_@iO=Y?XZcs~{YjS3_}vGXQ|h5a zA)C(>oKK9UQ?Zdag9D0-!nv)DI42fPA+sh+>ksb>r$`uh|GQvdCs8)_&_1z8yYqWG zG2P9|fRB_yem|=Ht!#YD{})mV-)~|JyiPcEX-F5760oKhsn3QLBgdF`GZ#M7a`+7m zxE$|`ewNxF^)$qNFsP9A@E$i%$YOi3U%JbCA#&esu}0xsa64nRw;R6E$TQ-;251c4 z?Sw6_yYy{c5=Zri&0cT-!uJ8fF4_okvx s4?#aO$GZyGcuAkC@UoaE;+}}D<^xrx6ob{O#D$;Zy%LlU6w-J91*T5-umAu6 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: { From 3cb429eebab4994c7bc59bdb6e3623582148f80b Mon Sep 17 00:00:00 2001 From: joshunrau Date: Mon, 5 Oct 2026 20:20:07 -0400 Subject: [PATCH 03/16] refactor(gateway): remove dead code Remove the not-found middleware nothing registers, the subjectId filter on GET /api/assignments that the API stopped sending, the unused port parameter of BaseServer.listen, the raw-import esbuild plugin in the dev script and an unused ImportMeta augmentation. Co-Authored-By: Claude Opus 5.5 --- apps/gateway/AGENTS.md | 3 +-- apps/gateway/scripts/dev.ts | 17 ------------ .../__tests__/not-found.middleware.test.ts | 26 ------------------- .../src/middleware/not-found.middleware.ts | 12 --------- .../src/routers/__tests__/api.router.test.ts | 12 +++------ apps/gateway/src/routers/api.router.ts | 12 ++------- .../src/server/__tests__/server.base.test.ts | 4 +-- apps/gateway/src/server/server.base.ts | 6 ++--- apps/gateway/src/vite-env.d.ts | 4 --- 9 files changed, 11 insertions(+), 85 deletions(-) delete mode 100644 apps/gateway/src/middleware/__tests__/not-found.middleware.test.ts delete mode 100644 apps/gateway/src/middleware/not-found.middleware.ts 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/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; } From ca8350bba2f8a99dbc3ac9974aa5b330e0b578f2 Mon Sep 17 00:00:00 2001 From: joshunrau Date: Mon, 5 Oct 2026 20:20:08 -0400 Subject: [PATCH 04/16] refactor(outreach): remove dead code Remove the unreferenced 4.1 instrument example and InstrumentProperties component, translation keys and namespaces no page reads, props no caller passes (translationMode, meta author/keywords, the generated Feature id), the unread testimonial format field, and options of the typedoc plugin that its only configuration never sets. Co-Authored-By: Claude Opus 5.5 --- .../assets/examples/4.1-instruments/index.ts | 90 ------------------- .../examples/4.1-instruments/styles.css | 6 -- .../src/components/InstrumentProperties.astro | 32 ------- .../outreach/src/components/layout/Head.astro | 6 +- .../src/components/overview/Feature.astro | 4 +- apps/outreach/src/content/config.ts | 1 - .../testimonials/massimiliano-orri.yaml | 1 - .../testimonials/maxime-montembeault.yaml | 1 - .../content/testimonials/simon-ducharme.yaml | 1 - apps/outreach/src/env.d.ts | 5 -- apps/outreach/src/i18n/index.ts | 8 +- apps/outreach/src/i18n/translations/blog.json | 4 - apps/outreach/src/i18n/translations/docs.json | 3 - apps/outreach/src/layouts/Page.astro | 10 +-- apps/outreach/src/lib/instrument-schemas.ts | 4 +- .../plugins/starlight-plugin-typedoc/index.ts | 30 ++----- .../starlight-plugin-typedoc/markdown.ts | 4 - .../starlight-plugin-typedoc/starlight.ts | 21 ++--- .../plugins/starlight-plugin-typedoc/theme.ts | 15 ++-- .../starlight-plugin-typedoc/typedoc.ts | 53 ++++------- 20 files changed, 46 insertions(+), 253 deletions(-) delete mode 100644 apps/outreach/src/assets/examples/4.1-instruments/index.ts delete mode 100644 apps/outreach/src/assets/examples/4.1-instruments/styles.css delete mode 100644 apps/outreach/src/components/InstrumentProperties.astro delete mode 100644 apps/outreach/src/i18n/translations/docs.json 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