Merge pull request DouglasNeuroInformatics#1519 from DouglasNeuroInformatics/fix/demo-mode-accessible-instruments

fix(api): connect uploaded instruments when a group is created

Author: joshunrau

apps/api/src/groups/__tests__/groups.service.spec.ts

@@ -42,13 +42,15 @@ describe('GroupsService', () => {
       expect(groupModel.create.mock.lastCall?.[0]).toMatchObject({ data: { name: 'Test Group' } });
     });
 
-    it('should connect only shared non-repo instruments', async () => {
+    it('should connect only shared non-repo instruments, matching an absent field as well as a null one, so an uploaded instrument is not skipped', async () => {
       instrumentModel.findMany.mockResolvedValueOnce([{ id: 'manual-1' }, { id: 'manual-2' }]);
       await groupsService.create({ name: 'Test Group', type: 'CLINICAL' });
       expect(instrumentModel.findMany).toHaveBeenCalledWith({
         where: {
-          OR: [{ seriesGroupId: null }, { seriesGroupId: { isSet: false } }],
-          sourceRepoId: null
+          AND: [
+            { OR: [{ seriesGroupId: null }, { seriesGroupId: { isSet: false } }] },
+            { OR: [{ sourceRepoId: null }, { sourceRepoId: { isSet: false } }] }
+          ]
         }
       });
       expect(groupModel.create.mock.lastCall?.[0]).toMatchObject({

apps/api/src/groups/groups.service.ts

@@ -25,10 +25,16 @@ export class GroupsService {
     }
     // Connect only instruments that did not come from an instrument repository. Repo-sourced
     // instruments are opt-in: a group manager must select them manually after a repo is assigned.
+    //
+    // Both fields need the `isSet` fallback: `InstrumentsService.create` writes neither, so on an
+    // instrument that was never imported from a repository the key is absent rather than null, and
+    // Prisma's `null` filter does not match an absent key.
     const nonRepoInstruments = await this.instrumentModel.findMany({
       where: {
-        OR: [{ seriesGroupId: null }, { seriesGroupId: { isSet: false } }],
-        sourceRepoId: null
+        AND: [
+          { OR: [{ seriesGroupId: null }, { seriesGroupId: { isSet: false } }] },
+          { OR: [{ sourceRepoId: null }, { sourceRepoId: { isSet: false } }] }
+        ]
       }
     });
     return this.groupModel.create({

testing/src/specs/group-manage.spec.ts

@@ -2,6 +2,25 @@ import { GroupManagePage } from '../pages/_app/group/manage.page';
 import { expect, test } from '../support/fixtures';
 
 test.describe('group manage', () => {
+  test('should give a newly created group access to every uploaded instrument, so a manager has something to administer', async ({
+    adminToken,
+    apiRequestContext,
+    uniqueId
+  }) => {
+    // Deliberately not the `api.createGroup` fixture: that helper PATCHes the accessible instruments
+    // in after creating the group, so it passes whether or not creation connected anything itself.
+    // An uploaded instrument has no `sourceRepoId` key at all, and the selection query has to match
+    // that as well as an explicit null.
+    const response = await apiRequestContext.post('/api/v1/groups', {
+      data: { name: `RawGroup${uniqueId}`, type: 'CLINICAL' },
+      headers: { Authorization: `Bearer ${adminToken}` }
+    });
+    expect(response.status()).toBe(201);
+
+    const group = (await response.json()) as { accessibleInstrumentIds: string[] };
+    expect(group.accessibleInstrumentIds.length).toBeGreaterThan(0);
+  });
+
   test('should let a group manager preview an accessible instrument', async ({ getPageModel }) => {
     const groupManagePage = await getPageModel('/group/manage');
 
@@ -31,8 +50,11 @@ test.describe('group manage', () => {
     await expect(page).toHaveURL('/group/manage');
 
     const groupManagePage = new GroupManagePage(page);
-    // A freshly created group has every non-repo instrument accessible already, so uncheck one to
-    // produce an observable, reversible change.
+    // `api.createGroup` grants access to every instrument, so uncheck one to produce an observable,
+    // reversible change. Assert it starts checked first: `uncheck()` is a no-op on an already-
+    // unchecked box, so without this the rest of the test would still pass if the precondition
+    // silently stopped holding.
+    await expect(groupManagePage.instrumentCheckbox('Happiness Questionnaire')).toBeChecked();
     await groupManagePage.instrumentCheckbox('Happiness Questionnaire').uncheck();
     await groupManagePage.subjectIdDisplayLengthInput.fill('6');
     await groupManagePage.submitButton.click();