feat(vue-mri): bulk selection and bulk actions on Data Exploration (#3127)

plugins/ui/apps/vue-mri-ui-lib/src/components/DeleteExplorationDialog.vue

@@ -36,6 +36,7 @@ import { mapActions, mapGetters, mapMutations } from 'vuex'
 import { D2eButton, D2eDialog } from '@d2e/ui'
 import * as types from '../store/mutation-types'
 import { getBookmarkType } from '../utils/BookmarkUtils'
+import { deleteExploration } from './helpers/deleteExploration'
 
 export default {
   name: 'DeleteExplorationDialog',
@@ -81,21 +82,13 @@ export default {
       const activeBookmark = this.getActiveBookmark
       const bookmarkType = getBookmarkType(bookmarkDisplay)
       const isMaterializedCohort = bookmarkType === 'M'
-      const isD2ECohortDefinition = ['D', 'D+M'].includes(bookmarkType)
-      const isAtlasCohortDefinition = ['A', 'A+M'].includes(bookmarkType)
 
       try {
-        if (isMaterializedCohort) {
-          await this.fireDeleteMaterializedCohortQuery(bookmarkDisplay.cohortDefinition.id)
-        } else if (isAtlasCohortDefinition) {
-          await this.fireDeleteAtlasCohortDefinitionQuery(bookmarkDisplay.atlasCohortDefinition.id)
-        } else if (isD2ECohortDefinition) {
-          await this.fireBookmarkQuery({
-            params: { cmd: 'delete' },
-            method: 'delete',
-            bookmarkId: bookmarkDisplay.bookmark.id,
-          })
-        }
+        await deleteExploration(bookmarkDisplay, {
+          fireBookmarkQuery: this.fireBookmarkQuery,
+          fireDeleteMaterializedCohortQuery: this.fireDeleteMaterializedCohortQuery,
+          fireDeleteAtlasCohortDefinitionQuery: this.fireDeleteAtlasCohortDefinitionQuery,
+        })
 
         await this.fireBookmarkQuery({ method: 'get', params: { cmd: 'loadAll' } })
         this.$emit('update:modelValue', false)

plugins/ui/apps/vue-mri-ui-lib/src/components/helpers/__tests__/bulkDeleteExplorations.test.ts

@@ -0,0 +1,153 @@
+import { describe, expect, it, vi } from 'vitest'
+import { runBulkDelete } from '../bulkDeleteExplorations'
+
+const record = (displayName: string): BookmarkDisplay => ({ displayName } as unknown as BookmarkDisplay)
+
+const makeDeps = () => ({
+  deleteOne: vi.fn().mockResolvedValue(undefined),
+  reload: vi.fn().mockResolvedValue(undefined),
+  clearSelection: vi.fn(),
+  clearActiveBookmarkIfDeleted: vi.fn().mockResolvedValue(undefined),
+  notifyFailure: vi.fn(),
+})
+
+describe('runBulkDelete', () => {
+  it('deletes every target, in order', async () => {
+    const deps = makeDeps()
+    const targets = [record('one'), record('two'), record('three')]
+
+    await runBulkDelete(targets, deps)
+
+    expect(deps.deleteOne).toHaveBeenCalledTimes(3)
+    expect(deps.deleteOne.mock.calls.map(([r]: [BookmarkDisplay]) => r.displayName)).toEqual(['one', 'two', 'three'])
+  })
+
+  it('one failure does not stop the rest', async () => {
+    const deps = makeDeps()
+    deps.deleteOne.mockImplementation(async (r: BookmarkDisplay) => {
+      if (r.displayName === 'two') throw new Error('boom')
+    })
+    const targets = [record('one'), record('two'), record('three')]
+
+    await runBulkDelete(targets, deps)
+
+    expect(deps.deleteOne).toHaveBeenCalledTimes(3)
+  })
+
+  it('the failed names reach the alert, in the order they failed', async () => {
+    const deps = makeDeps()
+    deps.deleteOne.mockImplementation(async (r: BookmarkDisplay) => {
+      if (r.displayName === 'two' || r.displayName === 'three') throw new Error('boom')
+    })
+    const targets = [record('one'), record('two'), record('three')]
+
+    await runBulkDelete(targets, deps)
+
+    expect(deps.notifyFailure).toHaveBeenCalledWith(['two', 'three'])
+  })
+
+  it('does not call notifyFailure when nothing failed', async () => {
+    const deps = makeDeps()
+    const targets = [record('one'), record('two')]
+
+    await runBulkDelete(targets, deps)
+
+    expect(deps.notifyFailure).not.toHaveBeenCalled()
+  })
+
+  it('reloads exactly once, after the loop', async () => {
+    const deps = makeDeps()
+    const callOrder: string[] = []
+    deps.deleteOne.mockImplementation(async () => {
+      callOrder.push('delete')
+    })
+    deps.reload.mockImplementation(async () => {
+      callOrder.push('reload')
+    })
+    const targets = [record('one'), record('two'), record('three')]
+
+    await runBulkDelete(targets, deps)
+
+    expect(deps.reload).toHaveBeenCalledTimes(1)
+    expect(callOrder).toEqual(['delete', 'delete', 'delete', 'reload'])
+  })
+
+  it('reloads exactly once even when every delete fails', async () => {
+    const deps = makeDeps()
+    deps.deleteOne.mockRejectedValue(new Error('boom'))
+    const targets = [record('one'), record('two')]
+
+    await runBulkDelete(targets, deps)
+
+    expect(deps.reload).toHaveBeenCalledTimes(1)
+  })
+
+  it('clears the selection after the run', async () => {
+    const deps = makeDeps()
+    const targets = [record('one')]
+
+    await runBulkDelete(targets, deps)
+
+    expect(deps.clearSelection).toHaveBeenCalledTimes(1)
+  })
+
+  it('clears the selection even when every delete fails', async () => {
+    const deps = makeDeps()
+    deps.deleteOne.mockRejectedValue(new Error('boom'))
+    const targets = [record('one')]
+
+    await runBulkDelete(targets, deps)
+
+    expect(deps.clearSelection).toHaveBeenCalledTimes(1)
+  })
+
+  it('passes every target, and the failed records themselves, to clearActiveBookmarkIfDeleted', async () => {
+    const deps = makeDeps()
+    const targets = [record('one'), record('two')]
+    deps.deleteOne.mockImplementation(async (r: BookmarkDisplay) => {
+      if (r === targets[1]) throw new Error('boom')
+    })
+
+    await runBulkDelete(targets, deps)
+
+    expect(deps.clearActiveBookmarkIfDeleted).toHaveBeenCalledWith(targets, new Set([targets[1]]))
+  })
+
+  it('identifies a failed record by identity, not by display name', async () => {
+    // Two never-materialised records can share a displayName. Tracking the
+    // failures by name would mark the deleted one as failed too, and the
+    // caller would leave the active bookmark pointing at a record that is
+    // already gone.
+    const deps = makeDeps()
+    const deleted = record('Cohort A')
+    const failedOne = record('Cohort A')
+    const targets = [deleted, failedOne]
+    deps.deleteOne.mockImplementation(async (r: BookmarkDisplay) => {
+      if (r === failedOne) throw new Error('boom')
+    })
+
+    await runBulkDelete(targets, deps)
+
+    const [, failed] = deps.clearActiveBookmarkIfDeleted.mock.calls[0] as [BookmarkDisplay[], Set<BookmarkDisplay>]
+    expect(failed.has(failedOne)).toBe(true)
+    expect(failed.has(deleted)).toBe(false)
+  })
+
+  it('does not use Promise.all — deletes are sequential, not concurrent', async () => {
+    const deps = makeDeps()
+    let active = 0
+    let maxActive = 0
+    deps.deleteOne.mockImplementation(async () => {
+      active += 1
+      maxActive = Math.max(maxActive, active)
+      await new Promise(resolve => setTimeout(resolve, 5))
+      active -= 1
+    })
+    const targets = [record('one'), record('two'), record('three')]
+
+    await runBulkDelete(targets, deps)
+
+    // Promise.all would let all three run at once, so maxActive would be 3.
+    expect(maxActive).toBe(1)
+  })
+})

plugins/ui/apps/vue-mri-ui-lib/src/components/helpers/__tests__/deleteExploration.test.ts

@@ -0,0 +1,111 @@
+import { describe, expect, it, vi } from 'vitest'
+import { deleteExploration, type DeleteExplorationDeps } from '../deleteExploration'
+
+const makeDeps = (): DeleteExplorationDeps & {
+  fireBookmarkQuery: ReturnType<typeof vi.fn>
+  fireDeleteMaterializedCohortQuery: ReturnType<typeof vi.fn>
+  fireDeleteAtlasCohortDefinitionQuery: ReturnType<typeof vi.fn>
+} => ({
+  fireBookmarkQuery: vi.fn().mockResolvedValue(undefined),
+  fireDeleteMaterializedCohortQuery: vi.fn().mockResolvedValue(undefined),
+  fireDeleteAtlasCohortDefinitionQuery: vi.fn().mockResolvedValue(undefined),
+})
+
+describe('deleteExploration', () => {
+  it('type M calls fireDeleteMaterializedCohortQuery with the cohort definition id, and nothing else', async () => {
+    const deps = makeDeps()
+    const record = { displayName: 'a materialized cohort', cohortDefinition: { id: 42 } } as unknown as BookmarkDisplay
+
+    await deleteExploration(record, deps)
+
+    expect(deps.fireDeleteMaterializedCohortQuery).toHaveBeenCalledWith(42)
+    expect(deps.fireDeleteAtlasCohortDefinitionQuery).not.toHaveBeenCalled()
+    expect(deps.fireBookmarkQuery).not.toHaveBeenCalled()
+  })
+
+  it('type A calls fireDeleteAtlasCohortDefinitionQuery with the Atlas id', async () => {
+    const deps = makeDeps()
+    const record = {
+      displayName: 'an atlas cohort',
+      atlasCohortDefinition: { id: 'atlas-1' },
+    } as unknown as BookmarkDisplay
+
+    await deleteExploration(record, deps)
+
+    expect(deps.fireDeleteAtlasCohortDefinitionQuery).toHaveBeenCalledWith('atlas-1')
+    expect(deps.fireDeleteMaterializedCohortQuery).not.toHaveBeenCalled()
+    expect(deps.fireBookmarkQuery).not.toHaveBeenCalled()
+  })
+
+  it('type A+M calls fireDeleteAtlasCohortDefinitionQuery with the Atlas id, not the materialized path', async () => {
+    const deps = makeDeps()
+    const record = {
+      displayName: 'a materialized atlas cohort',
+      cohortDefinition: { id: 42 },
+      atlasCohortDefinition: { id: 'atlas-1' },
+    } as unknown as BookmarkDisplay
+
+    await deleteExploration(record, deps)
+
+    expect(deps.fireDeleteAtlasCohortDefinitionQuery).toHaveBeenCalledWith('atlas-1')
+    expect(deps.fireDeleteMaterializedCohortQuery).not.toHaveBeenCalled()
+    expect(deps.fireBookmarkQuery).not.toHaveBeenCalled()
+  })
+
+  it('type D calls fireBookmarkQuery with cmd: delete and the bookmark id', async () => {
+    const deps = makeDeps()
+    const record = { displayName: 'a bookmark', bookmark: { id: 'bmk-1' } } as unknown as BookmarkDisplay
+
+    await deleteExploration(record, deps)
+
+    expect(deps.fireBookmarkQuery).toHaveBeenCalledWith({
+      params: { cmd: 'delete' },
+      method: 'delete',
+      bookmarkId: 'bmk-1',
+    })
+    expect(deps.fireDeleteMaterializedCohortQuery).not.toHaveBeenCalled()
+    expect(deps.fireDeleteAtlasCohortDefinitionQuery).not.toHaveBeenCalled()
+  })
+
+  it('type D+M calls fireBookmarkQuery with cmd: delete and the bookmark id, not the materialized path', async () => {
+    const deps = makeDeps()
+    const record = {
+      displayName: 'a materialized bookmark',
+      cohortDefinition: { id: 42 },
+      bookmark: { id: 'bmk-1' },
+    } as unknown as BookmarkDisplay
+
+    await deleteExploration(record, deps)
+
+    expect(deps.fireBookmarkQuery).toHaveBeenCalledWith({
+      params: { cmd: 'delete' },
+      method: 'delete',
+      bookmarkId: 'bmk-1',
+    })
+    expect(deps.fireDeleteMaterializedCohortQuery).not.toHaveBeenCalled()
+    expect(deps.fireDeleteAtlasCohortDefinitionQuery).not.toHaveBeenCalled()
+  })
+
+  it('a record with no sub-record throws, and calls nothing', async () => {
+    const deps = makeDeps()
+    const record = { displayName: 'nothing here' } as unknown as BookmarkDisplay
+
+    await expect(deleteExploration(record, deps)).rejects.toThrow()
+
+    expect(deps.fireDeleteMaterializedCohortQuery).not.toHaveBeenCalled()
+    expect(deps.fireDeleteAtlasCohortDefinitionQuery).not.toHaveBeenCalled()
+    expect(deps.fireBookmarkQuery).not.toHaveBeenCalled()
+  })
+
+  it('does not reload the list', async () => {
+    const deps = makeDeps()
+    const record = { displayName: 'a bookmark', bookmark: { id: 'bmk-1' } } as unknown as BookmarkDisplay
+
+    await deleteExploration(record, deps)
+
+    const loadAllCalls = deps.fireBookmarkQuery.mock.calls.filter(
+      ([payload]: [{ params?: { cmd?: string } }]) => payload?.params?.cmd === 'loadAll'
+    )
+    expect(loadAllCalls).toHaveLength(0)
+  })
+})

plugins/ui/apps/vue-mri-ui-lib/src/components/helpers/__tests__/explorationList.test.ts

@@ -1,8 +1,32 @@
 import { describe, it, expect } from 'vitest'
-import { filterAndSort, lastUpdatedMs, scoreCard } from '../explorationList'
+import { filterAndSort, lastUpdatedMs, scoreCard, toCardId } from '../explorationList'
 
 const card = (over: Record<string, unknown> = {}) => ({ displayName: 'card', ...over }) as never
 
+describe('toCardId', () => {
+  it('namespaces a bookmark id', () => {
+    expect(toCardId(card({ bookmark: { id: '42' } }))).toBe('bookmark:42')
+  })
+
+  it('namespaces a cohort-definition id', () => {
+    expect(toCardId(card({ cohortDefinition: { id: '7' } }))).toBe('cohort:7')
+  })
+
+  it('namespaces an atlas id', () => {
+    expect(toCardId(card({ atlasCohortDefinition: { id: '9' } }))).toBe('atlas:9')
+  })
+
+  it('falls back to the display name when no id is available', () => {
+    expect(toCardId(card({ displayName: 'unsaved' }))).toBe('name:unsaved')
+  })
+
+  it('prefers the bookmark id over a cohort or atlas id', () => {
+    expect(
+      toCardId(card({ bookmark: { id: '1' }, cohortDefinition: { id: '2' }, atlasCohortDefinition: { id: '3' } })),
+    ).toBe('bookmark:1')
+  })
+})
+
 describe('lastUpdatedMs', () => {
   it('prefers the bookmark dateModified', () => {
     const c = card({