Repository navigation
Copy all fields when creating an item from any existing item type - #2195
jbouquiaux wants to merge 1 commit into
Conversation
`_copy_sample_from_id` only replaced the request with the copied document inside its `samples` / `cells` branches, so copying any other type (custom item types, starting materials, ) created an empty item. The copied document is now always used, constituent lists are merged based on the fields present rather than the item type, and copying between different types is rejected
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2195 +/- ##
==========================================
- Coverage 82.48% 82.48% -0.01%
==========================================
Files 91 91
Lines 8782 8781 -1
==========================================
- Hits 7244 7243 -1
Misses 1538 1538
🚀 New features to boost your workflow:
|
davidwaroquiers
left a comment
davidwaroquiers
left a comment
There was a problem hiding this comment.
Thx for the PR. Sound good to me. Just two questions I put in the code.
| 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 |
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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.
davidwaroquiers
left a comment
davidwaroquiers
left a comment
There was a problem hiding this comment.
Thanks @jbouquiaux for this.
I think I uncovered a "subtle" bug (see in the comments) that was already present before, but got "extended" to more cases here.
Pinged @ml-evs to see what to do there (first if I'm indeed correct with my assessment, and second if we try to solve this here, and if so, how, i.e. which behavior do we want)
Besides that, I would maybe add one or two tests for this.
| if original_collections: | ||
| sample_dict["collections"] = original_collections |
There was a problem hiding this comment.
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 )
This branch has not been deployed
2 participants
Creating an item as a copy of another only used the source item's data for samples and cells. For every other type (custom item types, starting_materials, equipment),
_copy_sample_from_idreturned the request body unchanged, so the new item came out empty.With this PR, the copied document is now always used as the base for the new item, for every type. As before, the provided
item_id,name,dateandcollectionstake precedence.Closes #2193