fix(concept-sets): handle duplicate names and disable Update for non-owned sets - #3207
Merged
khairul-syazwan merged 6 commits intoSep 1, 2026
Merged
Conversation
…permission
A researcher-only user could not save a concept set. The save runs a
duplicate-name pre-check first, and that check asked WebAPI through
GET /conceptset/{id}/exists, guarded by
isAnyPermitted(anyOf('read:conceptset','write:conceptset'))
with no isOwner fallback. The 'concept set creator' role holds neither
permission, so the check returned 403 and the save stopped before the create
was ever attempted. The create itself was always permitted, because it is
guarded by isPermitted('create:conceptset'). Atlas3 never asks this question
and works for the same user.
The facade merges two stores in that check. Only the WebAPI half fails, and
the WebAPI store already enforces the same rule: webapi.concept_set carries
the uq_cs_name unique constraint, which rejects a duplicate on write and
reports it as HTTP 409.
Keep the legacy half, which needs no permission, and let the constraint answer
for the WebAPI store. Map the 409 on create and on update to a typed error, so
that a duplicate name reaches the browser as a 409 with a readable message
rather than a 500. Remove the now-unreachable WebAPI probe.
No permission grant is required for a researcher to save a concept set.
…rns 409 The save flow discarded the error and always rendered the generic 'Error creating/updating' text. A duplicate name in the WebAPI store is now reported as a 409 by the facade, so read it and show the same message the pre-check already shows for the legacy store. Every other failure keeps the generic text.
The Concept Sets list shows read-only rows (eye icon) for sets the user does not own, but opening one still showed an active Update button. The drawer re-fetches the set by id, and that get-by-id response carries an unreliable hasWriteAccess flag (false for owned sets, and 500 for a non-owned researcher set). The list row is the reliable source: it already marks non-owned sets with an eye. Pass the list row's writability into the drawer and use it as the authoritative ownership signal. Reset the ownership state when a set opens, so a failed get-by-id cannot leave a stale value. Keep the Update button visible but disabled for sets the user does not own, and enabled for owned sets, matching the read-only affordance already shown in the list.
The legacy mapper always reported hasWriteAccess true, so a shared legacy concept set owned by another user showed an active Update button even though the list marked it read-only. Decode the current user from the token and set legacy hasWriteAccess by ownership (createdBy === current user). When no user can be determined, keep the historical writable default. Covers the legacy half of the ownership fix already applied to the WebAPI store.
khairul-syazwan
requested review from
LSriragavan,
brandantck,
jerome-ng,
maggie-li-yd and
p-hoffmann
as code owners
August 26, 2026 07:06
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes duplicate concept-set handling and prevents updates to non-owned sets.
Changes:
- Maps WebAPI duplicate-name conflicts to HTTP 409.
- Propagates ownership-based write access into the drawer.
- Adds backend coverage for ownership and conflicts.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
TerminologyWithEventListener.tsx |
Forwards write access. |
Terminology.tsx |
Disables editing and handles 409 conflicts. |
ConceptSetsTable.tsx |
Passes row writability. |
ConceptSets.tsx |
Includes writability in drawer events. |
conceptset.service.ts |
Derives legacy ownership and narrows duplicate checks. |
conceptset.service.test.ts |
Tests ownership and conflict mapping. |
routes/conceptset.ts |
Returns typed 409 responses. |
ConceptSetErrors.ts |
Defines the name-conflict error. |
dto/conceptset.ts |
Defines the conflict response schema. |
WebApiConceptSetAPI.ts |
Maps upstream 409 responses to conflicts. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Merged
5 tasks
… change The ConceptSetsTable test asserted onAddEdit was called with only the id. It is now called with the writability flag, so update the three expectations to include the boolean. The /:id/exists route description still said the check used the WebAPI database; it now probes the legacy store only, so reflect that in the schema description.
maggie-li-yd
approved these changes
Aug 31, 2026
khairul-syazwan
enabled auto-merge
September 1, 2026 02:08
khairul-syazwan
deleted the
khairul-syazwan/conceptset-duplicate-name
branch
September 1, 2026 06:12
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A researcher-only user could not save a concept set. The Concept Sets UI called
GET /conceptset/{id}/existsbefore saving, and that route is guarded byread:conceptsetorwrite:conceptset. Theconcept set creatorrole holds neither, so the pre-check returned 403 and the save never ran. Atlas3 never makes this call, which is why it worked there.The fix removes the WebAPI half of the pre-check and lets the database
uq_cs_nameconstraint reject a duplicate as HTTP 409. The 409 is mapped to a typed conflict and shown to the user as the duplicate-name message instead of a generic error.Separately, the Concept Sets drawer showed an active Update button for a set the user does not own. The list already marks these sets read-only with an eye icon. The drawer re-fetches the set by id, and that response carries an unreliable write-access flag. The fix passes the list row's writability into the drawer and disables Update for non-owned sets. It also makes legacy concept sets report ownership, so a shared legacy set owned by another user is read-only too.
Affected modules
plugins/functions/d2e-webapi— facade, conceptset routes/service/errorsplugins/ui/apps/concept-sets— Terminology, ConceptSets, ConceptSetsTableValidation performed
denoland/deno:2.5.6).tsc --noEmit: clean.researcher18(create:conceptset only): create set succeeds; duplicate name shows the "already exists" message (409) instead of the generic error; non-owned set shows Update disabled and name read-only; owned set shows Update enabled.admin(owner): Update enabled on an owned legacy set; a mock shared legacy set owned by another user shows Update disabled.Merge Checklist
developbranch)