Add tags to items by gpetretto · Pull Request #8 · Matgenix/datalab

gpetretto · 2026-07-06T12:16:11Z

Lets users annotate items (samples, cells, starting materials, equipment) with tags, either free-text strings or references to managed tags stored in a new tags collection. Managed tags have an owner: no owner means global, otherwise owned by a user and optionally shared with groups. This PR includes both the core tagging capability and a full management UI.

Backend

New Tag model and tags collection; a HasTags mixin adds a tags field (union of managed references or free-text strings) to all item types.
New /tags routes: list, search, create, edit, delete, and permission updates, with an authoring policy (global tags admin-only, group tags manager-only) and scope-based name uniqueness.
Read-time resolution: tag names/colors are inlined on read and refreshed automatically, so renaming or recoloring a tag propagates everywhere; references to deleted tags are dropped and pulled from items. Only the minimal {type, immutable_id} link is stored.
Text/creator indices for tag search; regenerated JSON schemas.

Frontend

New Tags management page (/tags): a table of managed tags with color swatches, scope/owner, and an editable hint, plus a create/edit modal, color picker (preset palette), and delete with confirmation.
Tagging on items: a tags field on the sample/cell/starting-material/equipment detail pages (searchable multi-select supporting both managed and free-text tags, colored badges with tooltips, click-to-edit).

Access model

Permissions gate which tags a user can select (global + own + group-shared), but displaying tags on an item is unfiltered — the item itself is the access boundary, so a personal tag stays visible to anyone who can see the item.

Tests

Model and server tests for the tag CRUD/search/permissions/resolution, plus Cypress component tests and an e2e test for the management page.

Branch

Based on the bump-pydantic-final-final branch in trunk.

Move back to descriptions in field docstrings and configure this with customised BaseModel More model updates for pydantic v2

Rework ITEMS_FTS_FIELDS lookup

Fixes for routes

Chatblock patch; set default values in block More updates for blocks

More updates for models Fixes for models Simplify item versioning model Remove unecessary by_alias=True

…okups to mongo module

…rve out for plugins

davidwaroquiers

Ok I've done a first pass. Not completely finished but I stopped while going through the routes. I have the feeling indeed that there is some complexity that should be removed. Actually, I am wondering if the concept of changing the sharing of a tag is relevant. With items, you share an item/sample with a group and it's read only. With tags you can also share with a group, but then this group should not have another tag that has the same name that moment (same if you share with just another user).
I am wondering if we could still keep some flexibility, i.e. a user can create a new tag for himself but it's only for himself (maybe an admin could make it global). An admin can create global tags. I am thus wondering if the HasOwner is really the right ownership "traits" here. I'm wondering whether a tag should just have one single creator, who is the single owner of that tag, and then an admin may make it globally accessible (and we could keep just user vs global for now and possibly add other scopes later).
What do you think ?
I have made some other comments here and there. Some may not be relevant.

davidwaroquiers · 2026-07-06T15:33:31Z

+    "EntryReference": {
+      "additionalProperties": true,
+      "description": "A reference to a database entry by ID and type.\n\nCan include additional arbitarary metadata useful for\ninlining the item data.",


Why is this added in equipment.json ? Does not seem related to the tags PR.

because apparently EntryReference was never present in the Equipment before.

davidwaroquiers · 2026-07-06T15:35:12Z

+
+
+class Tag(Entry, HasOwner):
+    """A tag that can be associated to other entities.


Maybe "to other Entry objects" instead of "to other entities" ? What do you think ?

will be done in the simplified version

davidwaroquiers · 2026-07-06T20:02:16Z

+TagRef: TypeAlias = EntryReference | str
+"""A tag on an entry: either a reference to a managed `tags` entry or a free-text string."""


I think this can be removed, it is only used in the HasTags.

davidwaroquiers · 2026-07-08T08:34:45Z

+    elif current_user.is_authenticated:
+        creator_ids = [current_user.person.immutable_id]


Can this happen with the above ?

removed due to simplification of tag ownership

davidwaroquiers · 2026-07-08T08:40:29Z

+    Policy (see plan "Who may create/edit privileged tags"):
+        - Admins may author any tag.
+        - Global tags (empty `creator_ids`) require admin.
+        - Group tags (`group_ids` set) require admin or a manager of *every* named group.
+        - Personal tags are allowed for any authenticated user.


I think it would be good to make a short documentation about the tags and how they are used, defined, scoped, etc ...

Given that we are going for the simplified version maybe a detailed documentation can be postponed?

davidwaroquiers · 2026-07-08T08:43:51Z

+        return (
+            jsonify(
+                status="error",
+                message=f"A tag named {name!r} already exists in this scope.",
+            ),
+            409,  # 409: Conflict
+        )


Maybe do the same as for _authorize_tag_ownership ? i.e. have the _name_conflict_exists method return None when there is no conflict and return the jsonified error + error code when there is a conflict ? (I would probably rename the private helpers also)

_name_conflict_exists is used also in save_tag with slightly different approach. In principle can be done, but _authorize_tag_ownership will be removed and _name_conflict_exists would remain as the only one returning a message+error code. For a check function it seems more common to return a bool

Ok, let's keep it as such

davidwaroquiers · 2026-07-08T08:50:12Z

+@TAGS.route("/search-tags", methods=["GET"])
+@TAGS.route("/search/tags", methods=["GET"])


why two routes ? I see in items it's only search-items, but for users it's both ...

Not sure if there is an additional usage. The implementation was mostly modelled on the other similar entries, like collections, users, files and thus was added here as well. Not sure if it is useful for other routes as well. Should I remove the /search/tags?

no idea ... it's anyway not consistent in other routes right now. I would say let's keep just "/search-tags" for now (it's the same for items, only "search-items")

davidwaroquiers · 2026-07-08T12:39:44Z

+    if "creator_ids" in data:
+        creator_ids = [ObjectId(c) for c in data["creator_ids"]]


Here, i think a user can in principle create a tag for other users without himself being one of the creators. Do we allow this ?

removed in the simplified version.

DianaAliabieva · 2026-07-09T09:43:46Z

+          .catch((error) => {
+            console.error("Fetch error");
+            console.error(error);
+            this.isSearchFetchError = true;


This variable is set here, but it looks like we don't use it anywhere in the template (). Right now, if the search fails, the user just sees an empty list and might think there are no tags. Should we add an error message in the template (for example, in the #no-options slot) so the user knows the request actually failed?"

Good point! I think your suggestion is correct, however all the other similar components have the same flag which is never used. So here it should be decided if keeping this exactly equivalent to the other components or actually using it. What would you say?

Let's add a simple error message to the template for this component. As a next step, I would write everywhere clear error message,
easier to see why it doesn't work

DianaAliabieva · 2026-07-09T09:45:53Z

+        title: "Permission error",
+        message: "Unable to delete the tag: check that you have the appropriate permissions.",
+      });
+      throw error;


Right now, this catch block always shows 'Permission error'. But the error could be something else, like a 404 (tag already deleted) or a 500 server error. Showing 'Permission error' for everything might confuse the user. Can we check the actual error status and show 'Delete failed' if it's not a 403?

Thanks I will update the code to surface the true reason of the failure

DianaAliabieva · 2026-07-09T09:47:16Z

+    async debouncedAsyncSearch(query, loading) {
+      loading(true);
+      clearTimeout(this.debounceTimeout); // reset the timer
+      this.debounceTimeout = setTimeout(async () => {


We are setting a setTimeout here, but we don't clear it if the component is destroyed. We should add a beforeUnmount hook to run clearTimeout(this.debounceTimeout). This will prevent memory leaks and stop the code from running after the component is gone.

I will set the clearTiemout, but worth noticing that again this is basically a copy//paste from other components that suffer from the same issue.

yeah, I see now
maybe it the future it will be fully corrected

DianaAliabieva · 2026-07-09T09:53:37Z

+        const groups = this.isGlobal
+          ? []
+          : this.shareWithGroups.map((g) => ({ immutable_id: g.immutable_id || g._id }));
+        await updateTagPermissions(tagId, creators, groups);


If metadataChanged() and ownershipChanged() are both true, this awaits two separate requests sequentially. If updateTag succeeds but updateTagPermissions then fails (e.g. 409, permission denied), the tag is left half-saved — name/color/description updated, but ownership unchanged — while the user only sees a generic "Tag Update Failed" with no indication that part of the edit actually went through.
Suggest running both in parallel and surfacing which part failed, or documenting this as a known limitation if sequential-with-rollback isn't feasible right now:

async submitEdit() {
const tagId = this.tag.immutable_id;
const tasks = [];
if (this.metadataChanged()) {
tasks.push(updateTag(tagId, {
name: this.name.trim(),
description: this.description || null,
color: this.color || null,
}));
}
if (this.ownershipChanged()) {
const creators = this.isGlobal
? []
: this.selectedCreators.map((u) => ({ immutable_id: u.immutable_id }));
const groups = this.isGlobal
? []
: this.shareWithGroups.map((g) => ({ immutable_id: g.immutable_id || g._id }));
tasks.push(updateTagPermissions(tagId, creators, groups));
}
await Promise.all(tasks);
}

This doesn't fully solve atomicity (still two backend calls), but at least it fails together rather than silently leaving a half-applied edit, and Promise.allSettled could be used instead if partial success needs to be reported to the user.

Thanks for catching this. As mentioned in other comments I will propose a simplified version that removes the handling of different scopes for the tags. This part with multiple sequential checks will not be needed anymore, saving the complication of fixing it.

gpetretto · 2026-07-13T09:31:12Z

Thanks both for the review. I should have replied to all the comments, and I left a couple of questions.

BenjaminCharmes and others added 24 commits June 7, 2026 15:40

Update pydantic version across various files

c169b3e

Refactor server config for pydantic v2

7abcda2

Update traits module to pydantic v2

fd4755e

Refactor model imports to avoid circular deps

77f2345

Rework all models and tasks into pydantic v2 amenable form

358e27c

Move back to descriptions in field docstrings and configure this with customised BaseModel More model updates for pydantic v2

Load FTS fields for MongoDB from model schemas directly

8addb32

Rework ITEMS_FTS_FIELDS lookup

Misc pydantic v2 bumps around code

8a9205f

Update various routes for pydantic v2

f64fba3

Fixes for routes

Update various blocks for pydantic v2

b76e115

Chatblock patch; set default values in block More updates for blocks

More model updates

b44a387

More updates for models Fixes for models Simplify item versioning model Remove unecessary by_alias=True

Minimal test updates

b086e55

Patch optional vs default value for plugin data blocks

eb17464

Simplify error handling of all item routes

d4bf525

Fix exports for pydantic 2

1e0c862

Fix info endpoint for pydantic 2

5c0a8cf

Unpick circular dependencies between export/items routes by moving lo…

c5e6b15

…okups to mongo module

Move datalab_* fields in schema to json_schema_extra and allow ca…

15ce513

…rve out for plugins

Final wave of model_dump/dict changes

e3bceab

Minor tweaks suggested by copilot

d8a52ea

Fix docker build by reordering tasks imports

45183c5

Fix for auth routes

1e89b25

Update plugin config validator for pydantic 2

4ccfa2d

More auth route fixes

4265c79

full tags system

6b222b7

davidwaroquiers reviewed Jul 8, 2026

View reviewed changes

DianaAliabieva reviewed Jul 9, 2026

View reviewed changes

gpetretto force-pushed the gp/pyd2_orig branch from 4265c79 to cd2ac4b Compare July 14, 2026 14:08



		class Tag(Entry, HasOwner):
		"""A tag that can be associated to other entities.

		TagRef: TypeAlias = EntryReference \| str
		"""A tag on an entry: either a reference to a managed `tags` entry or a free-text string."""

		elif current_user.is_authenticated:
		creator_ids = [current_user.person.immutable_id]

		@TAGS.route("/search-tags", methods=["GET"])
		@TAGS.route("/search/tags", methods=["GET"])

		if "creator_ids" in data:
		creator_ids = [ObjectId(c) for c in data["creator_ids"]]

Uh oh!

Conversation

gpetretto commented Jul 6, 2026

Backend

Frontend

Access model

Tests

Branch

Uh oh!

davidwaroquiers left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

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!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gpetretto commented Jul 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

DianaAliabieva Jul 9, 2026 •

edited

Loading