Skip to content

Copy all fields when creating an item from any existing item type #2195

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jbouquiaux wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Copy all fields when creating an item from any existing item type #2195

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 32 additions & 34 deletions pydatalab/src/pydatalab/routes/v0_1/items.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -620,6 +620,16 @@ def search_items():
return jsonify({"status": "success", "items": list(cursor)}), 200


COPYABLE_CONSTITUENT_FIELDS: tuple[str, ...] = (
"synthesis_constituents",
"positive_electrode",
"negative_electrode",
"electrolyte",
)
Comment thread
davidwaroquiers marked this conversation as resolved.
"""Constituent list fields that are merged (rather than overwritten) when an item is
created as a copy of another item."""


def _copy_sample_from_id(sample_dict: dict, copy_from_item_id: str) -> dict:
copied_doc = flask_mongo.db.items.find_one(
{"item_id": copy_from_item_id, **get_default_permissions(user_only=False)}
Expand Down Expand Up @@ -647,47 +657,35 @@ def _copy_sample_from_id(sample_dict: dict, copy_from_item_id: str) -> dict:
copied_doc.pop("blocks", None)
copied_doc.pop("file_ObjectIds", None)

# any provided constituents will be added to the synthesis information table in
# addition to the constituents copied from the copy_from_item_id, avoiding duplicates
if copied_doc["type"] == "samples":
requested_type = sample_dict.get("type")
if requested_type and requested_type != copied_doc["type"]:
raise BadRequest(
f"Request to copy item with id {copy_from_item_id} of type {copied_doc['type']!r} "
f"into a new item of different type {requested_type!r} is not supported."
)

# any provided constituents will be added to the relevant constituent tables in
# addition to the constituents copied from the copy_from_item_id, avoiding duplicates.
# This is keyed on the fields present rather than the item type, so that custom item
# types inheriting these fields (e.g., `Sample` subclasses) are handled too.
for component in COPYABLE_CONSTITUENT_FIELDS:
if not isinstance(copied_doc.get(component), list):
continue
existing_consituent_ids = [
constituent["item"].get("item_id", None)
for constituent in copied_doc["synthesis_constituents"]
constituent["item"].get("item_id", None) for constituent in copied_doc[component]
]
copied_doc["synthesis_constituents"] += [
copied_doc[component] += [
constituent
for constituent in sample_dict.get("synthesis_constituents", [])
if constituent["item"].get("item_id") is None
for constituent in sample_dict.get(component, [])
if constituent["item"].get("item_id", None) is None
or constituent["item"].get("item_id") not in existing_consituent_ids
Comment on lines +677 to 681

@davidwaroquiers davidwaroquiers Oct 5, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So here correct me if I understand it correctly, the new item will take precedence over the previous one right ? I.e. if we copy a Sample with e.g. a given quantity of H2SO4, if we say that we use another quantity of H2SO4 for the new sample, it will take the new quantity, not sum up the two ? (I think that's what I would expect, just to understand :)

@jbouquiaux jbouquiaux Oct 6, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really, as I understand, any constituent with a different item_id (or with no item_id, i.e. free-text) is appended. So the result is "source constituents + the extra ones picked", not a replacement. Also in the creation modal, you cannot choose the quantity, only the constituent.

This PR is not changing the logic, just adding a loop over the constituent fields instead of being split by samples/cells. I agree that this might look a bit surprising in the UI. I'd say that if we want to make it replace, this should be discussed in another issue.

]

original_collections = sample_dict.get("collections", [])
sample_dict = copied_doc

if original_collections:
sample_dict["collections"] = original_collections

elif copied_doc["type"] == "cells":
for component in (
"positive_electrode",
"negative_electrode",
"electrolyte",
):
existing_consituent_ids = [
constituent["item"].get("item_id", None) for constituent in copied_doc[component]
]
copied_doc[component] += [
constituent
for constituent in sample_dict.get(component, [])
if constituent["item"].get("item_id", None) is None
or constituent["item"].get("item_id") not in existing_consituent_ids
]

original_collections = sample_dict.get("collections", [])
sample_dict = copied_doc
original_collections = sample_dict.get("collections", [])
sample_dict = copied_doc

if original_collections:
sample_dict["collections"] = original_collections
if original_collections:
sample_dict["collections"] = original_collections
Comment on lines +687 to +688

@davidwaroquiers davidwaroquiers Oct 9, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not strictly a bug of this PR but it seems incorrect. Collections are fine (I think), but groups and creators aren't. If we do:
1/ let's have two groups A and B
2/ let's create one item (item 1), shared with group A
3/ let's create one item (item 2), using "Copy from existing sample:" selecting item 1 as the copied item, also doing "Share with groups:" with group B.
4/ the new item (item 2) is shared with group A but not group B.

@ml-evs Should we fix this in this PR ? I think it would be good. Actually this bug is in the samples but not in the starting materials (or equipment) on main, but the generalization here "extends" it to starting materials.

One thing to note is: what should be the behavior ?
If we don't put any group in the "(Optional) Share with groups:", it should probably use the groups of the copied item.
If we put a group in "(Optional) Share with groups:", what should it do ? "append" to the groups that exist in the copied item ? or replace it ? (same problem is for the creators)

I think one solution could be that if you select an item to be copied, the "(Optional) Share with groups:" (and the equivalent for creators) is automatically filled with the groups from the copied item, that you can then decide to remove, keep or extend. (something "similar" is done for the name, when you select the copied item, the name automatically changes to COPY OF , not strictly the same but related)

To be noted also that this may change if/when we redesign access control (issue #2203 )


return sample_dict

Expand Down
Loading