Skip to content

feat(vue-mri): duplicate an exploration, backend and UI (#3123) - #3314

Open
khairul-syazwan wants to merge 4 commits into
khairul-syazwan/d2e-exploration-bulkfrom
khairul-syazwan/d2e-exploration-duplicate
Open

feat(vue-mri): duplicate an exploration, backend and UI (#3123)#3314
khairul-syazwan wants to merge 4 commits into
khairul-syazwan/d2e-exploration-bulkfrom
khairul-syazwan/d2e-exploration-duplicate

Conversation

@khairul-syazwan

Copy link
Copy Markdown
Collaborator

Closes #3123.

Ninth in the Data Exploration redesign stack. Based on khairul-syazwan/d2e-exploration-bulk (#3313), not develop — review that one first.

Backend and UI in one pull request. #3123 covers the whole feature; there is no separate ticket for the service command.

What this adds

Duplicate on the exploration card's More (⋮) menu. It copies immediately — the ticket is explicit that there is no confirmation dialog — and the copy appears named <original> (Copy), unshared and not materialised.

Half Where
duplicate command, route and validation plugins/functions/bookmark-svc/
fireDuplicateBookmarkQuery src/store/modules/bookmark.ts
Menu entry, guard and handler src/components/ExplorationsPage.vue
Three locale strings src/lib/i18n.ts

The backend command

POST /analytics-svc/api/services/bookmark/:bookmarkId/duplicate.

It composes two functions the service already had: loadSingleBookmark reads the source, _insertBookmark writes the copy. No new database primitive and no schema change.

Reading through loadSingleBookmark is the whole authorisation story. It scopes by userName, so a user cannot duplicate a bookmark they cannot read. There is no second check, deliberately.

The caller supplies the new name, so the (Copy) marker stays in the UI where it can be translated, and the command stays a general-purpose copy.

The UI

The menu entry already existed, hardcoded disabled: true and waiting for the command. This enables it.

Duplicate needs a D2E bookmark to read filters from, so it stays disabled for a materialised-only cohort, which has none, and for an Atlas definition, which has its own /copy. Ownership uses the same guard Rename and Delete already apply.

fireDuplicateBookmarkQuery is its own action rather than another fireBookmarkQuery command: that action builds every URL as ${bookmarkURL}/${bookmarkId || ''}, with nothing after the id, and this route is a sub-path. Adding one there would change the URL shape every other command depends on.

It reloads with loadAll afterwards, the way every other mutation in the module refreshes, rather than inserting the copy optimistically — the list derives from three record types and a synthetic row would not match the server.

Duplicating twice gives two identically named cards. The ticket says the user renames afterwards, so there is no (Copy 2) logic.

Figma

Frame 1810:241322. The frame shows the result, not the menu — its subject is the copy card, "SNRI Users (Copy)": a "Not run yet" chip, a Materialize button in place of the person count, and Last Materialised on / Exploration ID / Description all rendered as -.

That state was already built and shipped in #3262, so the "correction to PR 7.3" the plan carried needed no work. The design does not specify the menu item itself; it follows the entries beside it.

Fixed during review

Three passes found defects — a code review of each half, and a manual pass against a real card.

  • Every request was rejected before reaching the handler. duplicateBookmarkSchema was copied from deleteBookmarkSchema, which requires body.cmd. The route handler does set req.body.cmd, but validate() runs first, so every honest request failed with body.cmd Required. Delete only survives the same schema because its caller goes through the generic fireBookmarkQuery, which happens to put cmd in the body. Found by manual verification. The route tests missed it because they invoke handlers directly and bypass the validate() middleware, so the schema has no coverage at all.
  • A double click produced two copies. There is no confirmation dialog, so the menu click is the side effect. Now the ids with a request in flight are tracked, a repeat is ignored, and the entry is disabled for that card while it runs.
  • loadSingleBookmark masked every error. Its callback argument is optional — loadBookmarks and this command both await it instead — but the catch called it unconditionally, so a missing bookmark and a rejected read both surfaced as callback is not a function. It now rethrows when there is no callback. This improves loadBookmarks too.
  • The success toast was silent to screen readers. It is the only confirmation duplicate gives and it self-dismisses after two seconds, but it was a plain div. It now carries role="status" and aria-live="polite". Rename and delete route through the same toast and gain the same fix.

Known issues, not fixed here

  • The copy is stamped with the caller's cdmConfigId/cdmConfigVersion, not the source's. formatUserArtifactData does not carry those columns and IFormattedBookmark has no fields for them, so the source's real CDM stamp is unreachable from the read path. paConfigId is self-checking, because formatUserArtifactData filters on it and a mismatch makes the source unreadable; there is no equivalent guard on the CDM fields. So after a CDM upgrade a copy could carry filters authored against one version while labelled with another. Fixing it means threading two columns through a shared read path behind every bookmark list call, which is wider than this ticket. insert trusts the client the same way.
  • No visible progress between the click and the toast. The action is quick, and the menu entry disables while it runs, but there is no spinner. This is the same complaint as UX: Saving or deleting cohort is possible, but there is no feedback shown to the user #3209; worth matching whatever that settles on.
  • Duplicate's guard and its success/failure branching have no unit coverage. The logic lives in the component, and the repository rule forbids mounting components to assert behaviour. ExplorationsPage.vue being oversized is tracked separately.

Validation

Reported honestly; this is not a claim of full end-to-end coverage.

Check Result
Unit tests, Node 20, the command CI runs 1058 passed, 3 skipped. 3 new, for the store action
vite build pass
vite build --config vite.config.atlas.ts pass
vite build --config vite.config.atlas-app.ts pass
bookmark-svc tests not run — cannot be run. See below
eslint not run. plugins/ui hoists eslint 7.32, which cannot read the app's flat config
Manual browser pass on trex done, against a real card with filters. Found the schema defect above
no-mistakes gate not run. It does not follow a treehouse worktree and targets develop, not this stack's parent — the same path every PR in this stack has taken

bookmark-svc has no runnable test harness in this repository. There is a jest.config.js but no package.json and no dependencies; no plugins/functions/* service has one. Deno is not installed. The CI workflow calls yarn workspace bookmark run test, and the root defines no workspaces, while its services/cdw-svc path no longer holds this code. The spec added here was written red-then-green against a reconstructed harness and then that harness was removed. It cannot currently be executed by repository tooling, by CI or by a reviewer. Worth its own fix, and worth knowing before trusting the spec as evidence.

Manual verification covered the menu entry enabling, the copy appearing with the right name and card state, and opening the copy to confirm its filters survived_convertBookmarkIFR exists because the payload can return as a Uint8Array, so a copy can list correctly and still have lost its filters. That is the one way this feature fails silently, and the list response does not reveal it.

Add POST /:bookmarkId/duplicate, a `duplicate` command, and
_duplicateBookmark, which composes the two functions the service already has:
loadSingleBookmark to read the source, then _insertBookmark to write the copy.
No new database primitive and no schema change.

Reading through loadSingleBookmark is the whole authorisation story. It scopes
by userName, so a user cannot duplicate a bookmark they cannot read. The caller
supplies the new name, so the "(Copy)" suffix stays in the UI where it can be
translated. The copy is always unshared and never materialised.

Also fix loadSingleBookmark's error path. Its callback argument is optional —
loadBookmarks and this new command both await it instead of passing one — but
the catch block called it unconditionally. Every failure therefore surfaced as
"callback is not a function", so a missing bookmark and a rejected read were
indistinguishable in the logs. It now rethrows when there is no callback.

Note for reviewers: this service has no runnable test harness in the repo. It
has a jest.config.js but no package.json or dependencies, and the workflow that
would run it calls `yarn workspace bookmark run test` with no workspaces
defined and a services/ path that no longer holds this code. The spec added
here was verified against a reconstructed harness, red before green, and
cannot currently be executed by repository tooling.
Enable the Duplicate entry the card's More menu already carried. It copied
nothing before, because the backend command did not exist.

Duplicate needs a D2E bookmark to read its filters from, so the entry stays
disabled for a materialized-only cohort, which has none, and for an Atlas
definition, which has its own copy endpoint. Ownership is the same guard
Rename and Delete already use.

fireDuplicateBookmarkQuery is its own action rather than another
fireBookmarkQuery command. That action builds every URL as
`${bookmarkURL}/${bookmarkId || ''}`, with nothing after the id, and the
duplicate route is a sub-path; adding one there would change the URL shape
every other command depends on. It reloads with loadAll afterwards, the way
every other mutation in the module refreshes, rather than inserting the copy
optimistically — the list derives from three record types and a synthetic row
would not match the server.

There is no confirmation dialog; the ticket is explicit. Duplicating twice
gives two cards with the same name, and the user renames afterwards.

The copy's name and both messages read the locale string's own {0}, so word
order and the marker's position stay translatable.
duplicateBookmarkSchema was copied from deleteBookmarkSchema, which requires
body.cmd. The duplicate route's handler sets req.body.cmd itself, but the
validation middleware runs before the handler, so every honest request was
rejected with "body.cmd Required" before reaching it.

Delete only gets away with the same schema because its caller goes through the
generic fireBookmarkQuery, which puts cmd in the body. The duplicate action
posts to a sub-path that already names the command, so it does not, and should
not have to.

Found by manual verification against a real card. The route tests missed it
because they invoke the handler directly and bypass the validate() middleware,
so the schema is never exercised.
…e toast

Duplicate has no confirmation dialog, so the menu click is the side effect
itself. Nothing stopped a second click from posting again before the first
resolved, which produced two copies. Track the ids with a request in flight and
ignore a repeat, and disable the menu entry for that card while it runs, since
the menu closes on select and reopening it is the realistic second click.
Keyed by id rather than one boolean, so copying two different cards at once
still works. Rename and Delete need no equivalent: their click opens a modal.

Give the success toast role="status" and aria-live="polite". It is the only
confirmation duplicate gives, it dismisses itself after two seconds, and it was
a plain div, so a screen reader announced nothing and the action appeared to do
nothing. Rename and delete route their success through the same toast and gain
the same fix. The failure path already announced through BsAlert.

Also restore the log prefix every other console.error in the page uses.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant