Insert unique GUIDs update #13751
Insert unique GUIDs update #13751
Conversation
- assign new GUIDs for all elements - initial workflow, to be optimized and tested
- now correctly inserts groups and notes - now correctly inserts pinned notes - code refactor
- created a separate region for all the new code related to the Insert functionality
- added 2 new tests for Insert functionality - change old test to account for the ability to insert multiple of the same graph
- removed unused method inside the Insert function
src/DynamoCore/Models/DynamoModel.cs
Outdated
| /// <param name="nodesGuids">Nodes [old,new] GUID values</param> | ||
| /// <param name="notesGuids">Annotations [old,new] GUID values</param> | ||
| /// <param name="connectorsGuids">Connectors [old,new] GUID values</param> | ||
| private void PropagateUpdatesToViewInfo(ExtraWorkspaceViewInfo viewInfo, |
There was a problem hiding this comment.
For this function, it seems we are only replacing Guids, I feel the function maybe can be simplified further. After some deeper look, it seems these elements do not share same interface, we cant really write utility method to avoid repeating code. So good for now.
There was a problem hiding this comment.
I was thinking the same, but they are ever so slightly different to make me not do it. They do share the same base class, but that class does not have the GUID we are after. Also, the GUID is sometimes a Guid, sometimes a string.
There was a problem hiding this comment.
All code is now reworked in favor of a much simpler method that operates directly on the json string, as discussed on Friday. It works great to on my side, but let me know if that's not ideal for whatever reason and I can update as needed.
|
LGTM with couple comments |
src/DynamoCore/Models/DynamoModel.cs
Outdated
|
|
||
| // Assign new annotation models new GUIDs | ||
| // We need two separate runs to first create the new GUIDs and second work with the new GUIDS | ||
| if (notes != null) |
There was a problem hiding this comment.
I find this entire approach a bit complicated...
I am just curious what you think of an approach that modifies the json. I would usually shy away from that approach but I believe you could select all IDS BEFORE deserialization or as jtokens and transform them...
I think it would be much less code.
There was a problem hiding this comment.
Yes! Great call and while I didn't get the idea immediately, I tested it today and it works great and simplifies this whole thing. Committed here.
There was a problem hiding this comment.
Let me know if that doesn't look good to you and I will rework as needed.
There was a problem hiding this comment.
@mjkkirschner Previously I feel the approach was OK also because we have another task to update Guids while Dynamo users save the current workspace as a new one, then a JSON block of the graph is not guaranteed to be there for a simplified approach. But after second thought, if this approach is preferred, maybe we can do the same for save as - touch the JSON instead of in-memory workspace representation.
| Assert.AreEqual(currentCount * 2, updatedCount); | ||
| } | ||
|
|
||
| [Test] |
There was a problem hiding this comment.
do these tests cover ALL your new methods? It would be good to cover each new method you add with a unit test.
There was a problem hiding this comment.
There are 3 things to worry about, the way I see it:
- the Nodes being appropriately inserted, and us being able to get them over and over again
- the Groups working correctly, both nested and non-nested
- the Notes are working correctly, both pinned and not pinned
I think these tests cover the scenarios above. I can try adding unit tests too though?
There was a problem hiding this comment.
I will follow up on save as in a new PR and add test as well
- code rework to massively reduce the logic of updating the Guids of the inserted workspace
- more detail explanation to why the Guid update is necessary
Purpose
This PR addresses an outstanding issue with the Insert functionality introduced a while back. Previously, it wasn't possible for users to insert the same graph or derivatives of the same graph inside the workspace. That was due to the identical GUIDs present in the graphs (attached current state of affairs comparison between 3 derivative dynamo graphs).
Logic to recreate all element GUIDs that make up the workspace prior to inserting them into the current workspace is introduced with this PR to handle these cases.
Side-by-side derivative comparison
Insert multiple of the same
Declarations
Check these if you believe they are true
*.resxfilesRelease Notes
Reviewers
@QilongTang
@mjkkirschner
@Amoursol
FYIs
@sm6srw