Add series instrument creation and remote assignment flow by thomasbeaudry · Pull Request #1392 · DouglasNeuroInformatics/OpenDataCapture

thomasbeaudry · 2026-07-06T04:51:40Z

Includes the create-series-instrument feature (bundler, schemas, renderer, web hooks/routes, e2e coverage) and a fix for the group-save 500: filter deleted instrument ids server-side before the relation set, and prune them from the cached group after an instrument delete.

Something I noticed is that the datahub doesn't track whether an insturment is administered standalone or within a series. It may be useful to track this because I could imagine the results of a form could vary depending on the other forms they take at the same time? Also the datahub records the form date, but no the form adminsitration time.

Do you want me to add those 2 things to the data hub? If we determine that is it important ot track the series in the DB, then i would disallow deletiion of a series if it has been adminsitered to a participant for reproducibility reasons.

Includes the create-series-instrument feature (bundler, schemas, renderer, web hooks/routes, e2e coverage) and a fix for the group-save 500: filter deleted instrument ids server-side before the relation set, and prune them from the cached group after an instrument delete. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

The ./constants subpath export had its `types` condition pointing at the source `./src/constants.ts` instead of the built declaration. A source .ts is not a valid target for the `types` condition, so a fresh CI build failed to resolve `@opendatacapture/runtime-core` exports, cascading into module-not-found and implicit-any errors in the instrument-bundler fixtures. Local builds passed only because a stale runtime/v1/dist masked it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

gdevenyi · 2026-07-06T18:59:46Z

Do you want me to add those 2 things to the data hub? If we determine that is it important ot track the series in the DB, then i would disallow deletiion of a series if it has been adminsitered to a participant for reproducibility reasons.

We need to review the "metadata" model of instruments in general to make sure we're capturing all we should capture.

Copilot

Pull request overview

Adds a “series instrument” feature that lets group managers assemble an ordered set of existing scalar instruments into a new SERIES instrument, preview it in the web UI, assign it to groups (including expanding underlying scalar IDs), and delete series instruments when they have no collected records. Also hardens group updates against stale/deleted instrument IDs to prevent 500s.

Changes:

Add schemas + API endpoints/service logic for creating SERIES instruments server-side (bundled/validated like normal instruments) and deleting instruments with record-count protection.
Update web group-manage UI to create/preview/delete series instruments and expand selected series IDs into underlying scalar IDs on save.
Prevent group-save failures by filtering deleted/nonexistent instrument IDs server-side and pruning deleted IDs client-side.

Reviewed changes

Copilot reviewed 17 out of 18 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
packages/schemas/src/instrument/instrument.base.ts	Extends instrument info schema (seriesItems/internal.name) and adds create-series DTO schema.
packages/react-core/src/components/InstrumentRenderer/SeriesInstrumentRenderer.tsx	Threads submit button label into series form rendering.
packages/react-core/src/components/InstrumentRenderer/ScalarInstrumentRenderer.tsx	Threads submit button label into scalar form rendering.
packages/react-core/src/components/InstrumentRenderer/InstrumentRenderer.tsx	Adds submitButtonLabel prop to top-level renderer API.
packages/react-core/src/components/FormContent/FormContent.tsx	Adds submitButtonLabel prop and forwards to libui Form.
apps/web/src/store/types.ts	Adds auth-slice API for pruning deleted instrument IDs.
apps/web/src/store/slices/auth.slice.ts	Implements pruneDeletedInstrument to keep cached group state consistent.
apps/web/src/routes/_app/group/manage.tsx	Adds series create/preview/delete UI and expands series selections to include scalar IDs.
apps/web/src/hooks/useDeleteInstrumentMutation.ts	New mutation hook to delete instruments + invalidate instrument-info + prune cache.
apps/web/src/hooks/useCreateSeriesInstrumentMutation.ts	New mutation hook to create series instruments with duplicate-confirmation flow.
apps/api/src/instruments/instruments.service.ts	Implements createSeries, deleteById, seriesItems projection in findInfo, and series-id logic update.
apps/api/src/instruments/instruments.controller.ts	Adds POST /instruments/series and DELETE /instruments/:id endpoints.
apps/api/src/instruments/dto/create-series-instrument.dto.ts	Adds validated DTO for create-series input.
apps/api/src/instruments/tests/instruments.service.spec.ts	Adds unit tests for createSeries/id generation/deleteById behavior.
apps/api/src/groups/groups.service.ts	Filters nonexistent instrument IDs before setting group instrument relations.
apps/api/src/groups/tests/groups.service.spec.ts	Adds test for dropping deleted instrument IDs before relation set.
apps/api/src/auth/ability.factory.ts	Grants group managers create/delete abilities for instruments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

thomasbeaudry · 2026-07-06T20:53:34Z

        ability.can('read', 'Instrument');
+        // Group managers may assemble series instruments on the fly and remove ones they created
+        // (the service enforces that an instrument with collected records cannot be deleted).
+        ability.can('create', 'Instrument');
+        ability.can('delete', 'Instrument');


Addressed on the current branch: the delete ability is now scoped — ability.can('delete', 'Instrument', { sourceRepoId: null }) (ability.factory.ts:33). accessibleQuery therefore no longer returns {}, so a group manager can only delete non-repo (manually-uploaded) instruments; repo-sourced instruments shared across groups are excluded. The service-side record-count guard remains as defense-in-depth.

Why do group managers need to be able to delete instruments in the first place? This contradicts the original design of the platform.

Addressed in the latest push. The rationale for keeping it: a series instrument isn't a shared platform asset — it's an on-the-fly bundle of existing instruments that a group manager assembles, and records are only ever collected against the constituent scalars, never the series itself. So deleting one never orphans data or touches a real instrument. The ability is now scoped — ability.can('delete', 'Instrument', { sourceRepoId: null }) — so repo-sourced instruments can never be deleted, and the service additionally rejects deletion of anything that isn't a series (unit-tested). If you'd still rather group managers not have this at all, I can drop the delete path entirely.

thomasbeaudry · 2026-07-06T20:53:35Z

+    const instrument = await this.instrumentModel.findFirst({
+      where: { AND: [accessibleQuery(ability, 'delete', 'Instrument')], id }
+    });
+    if (!instrument) {
+      throw new NotFoundException(`Failed to find instrument with ID: ${id}`);
+    }
+    const recordCount = await this.instrumentRecordModel.count({ where: { instrumentId: id } });


deleteById gates on accessibleQuery(ability, 'delete', 'Instrument'), and the delete ability now carries { sourceRepoId: null } (ability.factory.ts:33), so the generated Prisma where already excludes repo-sourced instruments — a repo instrument yields no match and returns NotFound. Happy to also add an explicit sourceRepoId === null assertion in the service as belt-and-suspenders (independent of the ability config) if you'd like it guaranteed at the service layer too.

the ability should be scoped to only series instruments and this should be explicitly tested.

Done in the latest push — deleteById now rejects anything that isn't a series (isSeriesInstrument check → ForbiddenException), with an explicit unit test ('refuses to delete a non-series (scalar) instrument'). One caveat on scoping the ability to series: kind isn't a database column (it lives inside the evaluated bundle), so the CASL/Prisma layer can only scope on real fields — the ability is scoped to { sourceRepoId: null } and the series-only rule is enforced (and tested) at the service layer.

thomasbeaudry · 2026-07-06T20:53:37Z

+        internal: instrument.internal ?? null,
        kind: instrument.kind,
+        seriesItems: instrument.seriesItems,
        source,


Addressed: source is now derived from repoId, not the repo name — const source = repoId ? { kind: 'repo', name: repoName ?? 'Unknown' } : { kind: 'manual' } (manage.tsx:876). A legacy repo instrument with a null name is therefore still treated as repo-sourced (so it doesn't get the manual/delete affordance), with an "Unknown" fallback label.

Following up — correcting my earlier reply: the committed code was still deriving provenance from the repo name. Now fixed: source is keyed on the repo id, with a localized 'Unknown repository' fallback label, so a legacy repo instrument with a null name is still treated as repo-sourced and never gets the manual/delete affordance.

joshunrau · 2026-07-06T20:11:45Z

Do you want me to add those 2 things to the data hub? If we determine that is it important ot track the series in the DB, then i would disallow deletiion of a series if it has been adminsitered to a participant for reproducibility reasons.

We need to review the "metadata" model of instruments in general to make sure we're capturing all we should capture.

The data hub does not track anything. The data hub is just displaying the results from the database. We record both the exact time that an instrument was completed as well as the mode of administration (remote, in-person, retrospective). Both of these should be available when you download the results as a CSV.

I agree that we should display the mode of capture. I did not add exact time to the table to avoid cluttering it. If you would prefer, we could add something to display the time, I just didn't see the use case.

gdevenyi · 2026-07-06T20:36:22Z

If you would prefer, we could add something to display the time, I just didn't see the use case.

I'd think I'd like to discuss how we can make customiziable columns in the viewer so that data can be available if wanted. Seperate issue to this (and to the broader metadata audit)

thomasbeaudry · 2026-07-06T20:41:31Z

I'd think I'd like to discuss how we can make customiziable columns in the viewer so that data can be available if wanted. Seperate issue to this (and to the broader metadata audit)

Yes i was thinking the exact same thing.

@joshunrau also I downloaded the happiness questionaire csv and I don't see the mode of administration:

Also, is the series that was used to record the data being recorded in the DB? This would be linked to this PR if we want ot track it.

joshunrau

this is terrible, it needs major changes

joshunrau · 2026-07-07T18:43:31Z

        ability.can('read', 'Instrument');
+        // Group managers may assemble series instruments on the fly and remove ones they created
+        // (the service enforces that an instrument with collected records cannot be deleted).
+        ability.can('create', 'Instrument');
+        ability.can('delete', 'Instrument');


Why do group managers need to be able to delete instruments in the first place? This contradicts the original design of the platform.

joshunrau · 2026-07-07T22:34:40Z

+  bundle: vi.fn(() => Promise.resolve('__BUNDLE__'))
+}));
+
+// A multilingual series instance as returned by `find`, containing two forms.


add proper type annotation

@joshunrau a series instrument isn't really an instrument, it's a bundled list of instruments. I see no harm in letting the people who create them delete them, They could even belong to speccific groups instead of every user as well

Done — existingSeries is now typed as WithID<SeriesInstrument> (no as any), which also forced the fixture to carry the full details/license/tags shape.

joshunrau · 2026-07-07T22:58:47Z

+import type { ScalarInstrumentInternal } from '@opendatacapture/runtime-core';
+import { $CreateSeriesInstrumentData } from '@opendatacapture/schemas/instrument';
+
+@ValidationSchema($CreateSeriesInstrumentData)


no need for dto anymore. just import the $CreateSeriesInstrumentData any use that as annotation. make sure not to use type-only import

Done — the DTO class is deleted. $CreateSeriesInstrumentData is exported as both the schema value and its inferred type, and the controller uses it directly as the @Body() annotation (value import, not type-only) — same pattern as $SelfUpdateUserData in the users controller.

joshunrau · 2026-07-07T23:00:52Z

+  instructions?: string;
+  items: ScalarInstrumentInternal[];
+  title: string;
+}


Why are you not using the actual series instrument definition? Right now, you are missing the multilingual aspects and potential other things. Use the same data structure as the instrument type, omitting things that are not stored/computed

Reworked — the request schema is now composed from the master instrument schemas: details picked from $InstrumentDetails (multilingual title + optional description), clientDetails.instructions from $ClientInstrumentDetails, plus language: $InstrumentLanguage. Only fields that are stored verbatim are accepted; everything computed server-side (id, content, kind) is omitted. The generated definition is explicitly typed Omit<SeriesInstrument, 'id'> so any drift from the instrument type fails to compile.

joshunrau · 2026-07-07T23:02:06Z

+    const instrument = await this.instrumentModel.findFirst({
+      where: { AND: [accessibleQuery(ability, 'delete', 'Instrument')], id }
+    });
+    if (!instrument) {
+      throw new NotFoundException(`Failed to find instrument with ID: ${id}`);
+    }
+    const recordCount = await this.instrumentRecordModel.count({ where: { instrumentId: id } });


the ability should be scoped to only series instruments and this should be explicitly tested.

joshunrau · 2026-07-07T23:12:13Z

    logout: () => {
      window.location.reload();
+    },
+    pruneDeletedInstrument: (instrumentId) => {


this is a terrible design. I understand why it might be necessary, but consider how you can change implementation details to make a clean approach

Removed. The underlying need (don't re-send a deleted id on the next group save) is handled locally in the manage route: a deletedIds set in component state, filtered at submit time. No global store involvement.

joshunrau · 2026-07-07T23:12:52Z

  /** @deprecated */
  options?: InterpretOptions;
  subject?: SubjectDisplayInfo;
+  submitButtonLabel?: string;


Changed — submitButtonLabel is now a LocalizedText ({ [L in Language]?: string }) resolved with t() inside FormContent, so the shared component can no longer be handed a pre-translated single-language string.

joshunrau · 2026-07-07T23:12:57Z

  NavigationBlocker?: NavigationBlockerComponent;
  onSubmit: InstrumentSubmitHandler;
  subject?: SubjectDisplayInfo;
+  submitButtonLabel?: string;


Same fix as on ScalarInstrumentRenderer — the prop is LocalizedText on all three renderers and translated at the leaf with t().

joshunrau · 2026-07-07T23:14:24Z

+  // The ordered list of scalar instruments (by name + edition) that make up the series.
+  items: z.array($ScalarInstrumentInternal).min(2),
+  // The unique display name entered by the group manager.
+  title: z.string().min(1)


this should be your source of truth and should come based on the master schema

Done — $CreateSeriesInstrumentData is now composed from the master schemas ($InstrumentDetails, $ClientInstrumentDetails, $InstrumentLanguage, $ScalarInstrumentInternal) rather than redeclaring its own shapes.

joshunrau · 2026-07-07T23:15:33Z

+      })
+      .array()
+      .optional(),
    sourceRepo: z


we should be using a discriminated union here for series vs scalar

Done — InstrumentInfo is now a discriminated union on kind: scalar variants carry a required internal, the series variant carries required seriesItems. findInfo builds the proper variant and consumers narrow on kind instead of optional-chaining.

Backend - Derive $CreateSeriesInstrumentData from the master instrument details/clientDetails schemas (multilingual title/description/instructions) and take language from the client instead of hardcoding en/fr and duplicating text. - Remove the create-series DTO; the controller annotates the body with the $CreateSeriesInstrumentData schema value directly. - Share a discriminated-union result contract (CreateSeriesInstrumentResult) between the service, controller and web hook (outcome: 'created' | 'duplicate'). - Make InstrumentInfo a discriminated union over kind (scalar carries `internal`, series carries `seriesItems`); build the correct variant in findInfo and resolve series items from the full scalar set even under a kind filter. - deleteById: restrict deletion to series instruments (series never collect records) and drop the record-count guard; scope the group-manager delete ability to non-repo instruments. Add an explicit test that deleting a non-series is refused. - Handle multilingual titles throughout (uniqueness search now spans all instruments, duplicate detection returns the stored title); rename getFormSetKey -> getSeriesItemSetKey and give the generated definition an explicit SeriesInstrument type. Frontend - Rewrite useCreateSeriesInstrumentMutation to reuse the shared request/result types. - Remove pruneDeletedInstrument from the auth store; track deleted ids as local manage state and filter them from the save payload. - Fix instrument provenance to derive from the repo id (not name) so legacy repo instruments are never treated as manually-uploaded. - Make the shared submitButtonLabel prop a localizable value resolved with t(). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

thomasbeaudry requested a review from joshunrau as a code owner July 6, 2026 04:51

gdevenyi requested a review from Copilot July 6, 2026 18:59

Copilot started reviewing on behalf of gdevenyi July 6, 2026 18:59 View session

Copilot AI reviewed Jul 6, 2026

View reviewed changes

joshunrau requested changes Jul 7, 2026

View reviewed changes

Uh oh!

Conversation

thomasbeaudry commented Jul 6, 2026

Uh oh!

gdevenyi commented Jul 6, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joshunrau commented Jul 6, 2026

Uh oh!

gdevenyi commented Jul 6, 2026

Uh oh!

thomasbeaudry commented Jul 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joshunrau left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

thomasbeaudry commented Jul 6, 2026 •

edited

Loading