Skip to content
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.

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

Reset element IDs in copied modules. #2082

Conversation

FrenjaminBanklin
Copy link
Contributor

Closes #2081.

When making a copy of a module, all element IDs following the UUID format will be replaced. References to the old IDs in button actions etc. will also be updated with the corresponding new ID.

…pattern are replaced with new UUIDs. References to the old node IDs in button actions etc. are also replaced with the corresponding new IDs.
mbusch3
mbusch3 previously approved these changes Jun 9, 2023
Copy link
Contributor

@mbusch3 mbusch3 left a comment

Choose a reason for hiding this comment

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

I don't know enough about the existing code to know if this is a problem or not. But in addition to ids that were changed, in the parent object, this also changes the 'rev', 'draftId', and 'contentId'. Do these reference other objects that need to be saved to the database, or does Draft.createWithContent cover that? The front end seemed to work fine when loading and editing both the original and the copy.

// globally replace each old ID with the equivalent new ID
// this should replace node IDs as well as action trigger references to those node IDs
for (const [oldId, newId] of Object.entries(idsForChange)) {
// this works, but there may be a more efficient way of doing it
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a very elegant solution... Changing keys while retaining values is a pain, while string replacing is simple and relatively fast. The only real concern is making sure you're replacing globally instead of the first instance only, which you've done below AND made sure was included in your test case.

@FrenjaminBanklin
Copy link
Contributor Author

A copied module is a new module, so draftId and contentId being new is expected. I'm not exactly sure where rev is set, but my guess would be that it's also specific to an individual draft or revision so it's probably fine that it also changes.

@FrenjaminBanklin FrenjaminBanklin changed the base branch from dev/31-taconite to dev/32-pigeonite July 10, 2023 14:06
@FrenjaminBanklin FrenjaminBanklin dismissed mbusch3’s stale review July 10, 2023 14:06

The base branch was changed.

@FrenjaminBanklin
Copy link
Contributor Author

This was previously reviewed and everything still appears to be working correctly after modernizing from the current dev branch. I'm going to go ahead and call it good.

@FrenjaminBanklin FrenjaminBanklin merged commit efa2060 into ucfopen:dev/32-pigeonite Jul 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants