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

Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
33 changes: 32 additions & 1 deletion packages/app/obojobo-repository/server/routes/api.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ const publicLibCollectionId = require('../../shared/publicLibCollectionId')

const { levelName, levelNumber, FULL } = require('../../../obojobo-express/server/constants')

const uuid = require('uuid').v4

// List public drafts
router.route('/drafts-public').get((req, res) => {
return Collection.fetchById(publicLibCollectionId)
Expand Down Expand Up @@ -186,10 +188,39 @@ router
}

const oldDraft = await Draft.fetchById(draftId)

// gather all of the node IDs in the document and determine a new ID to replace each with
const idsForChange = {}

// this should never not exist, but just in case
if (oldDraft.nodesById) {
// only generate a replacement for ids that are UUIDs, not custom
const uuidRegex = /[0-9a-f]{8}-[0-9a-f]{4}-[1-5][0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i
oldDraft.nodesById.forEach((node, key) => {
if (key && uuidRegex.test(key)) idsForChange[key] = uuid()
})
}

// now convert the updated document to an object for use
const draftObject = oldDraft.root.toObject()

const newTitle = req.body.title ? req.body.title : draftObject.content.title + ' Copy'
draftObject.content.title = newTitle
const newDraft = await Draft.createWithContent(userId, draftObject)

// convert the object to a JSON string so we can swap out all the old IDs with the new ones
let draftString = JSON.stringify(draftObject)

// 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.

draftString = draftString.replace(new RegExp(oldId, 'g'), newId)
}

const newDraftObject = JSON.parse(draftString)

// const newDraft = await Draft.createWithContent(userId, draftObject)
const newDraft = await Draft.createWithContent(userId, newDraftObject)

const draftMetadata = new DraftsMetadata({
draft_id: newDraft.id,
Expand Down
98 changes: 98 additions & 0 deletions packages/app/obojobo-repository/server/routes/api.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,10 @@ jest.mock('obojobo-express/server/insert_event')
jest.unmock('fs') // need fs working for view rendering
jest.unmock('express') // we'll use supertest + express for this

jest.mock('uuid', () => ({
v4: jest.fn()
}))

let CollectionSummary
let Collection
let DraftSummary
Expand Down Expand Up @@ -59,6 +63,8 @@ const express = require('express')
const bodyParser = require('body-parser')
const app = express()

const uuid = require('uuid').v4

// register express-react-views template engine if not already registered
app.engine('jsx', require('express-react-views-custom').createEngine())

Expand Down Expand Up @@ -589,6 +595,98 @@ describe('repository api route', () => {
})
})

test('post /drafts/:draftId/copy refreshes all node IDs in a document', () => {
expect.hasAssertions()

// have a bit of a document just to make sure substitutions work everywhere
// actual details such as type, etc. don't matter too much here
const mockDraftObject = {
id: 'mockNewDraftId',
content: {
id: 'mockNewDraftContentId',
title: 'mockDraftTitle'
},
children: [
{
id: 'do-not-change-me',
children: [
{ id: '4baa2860-5219-404f-9d99-616ca8f81e41' },
{
id: 'adcc55b7-e412-4f44-b64f-6c317021f812',
reference: {
id: '4baa2860-5219-404f-9d99-616ca8f81e41'
}
},
{ id: '9d992860-3540-5219-8b4a-1e41616ca8f8' }
]
}
]
}

// this is admittedly sort of magical - we happen to know how many times
// this should run based on the document structure we made above
// potentially find a more elegant way of doing this
uuid
.mockReturnValueOnce('00000000-0000-0000-0000-000000000001')
.mockReturnValueOnce('00000000-0000-0000-0000-000000000002')
.mockReturnValueOnce('00000000-0000-0000-0000-000000000003')

// this is also a bit brute force, but it does the job
const expectedNewDraftDocument = {
id: 'mockNewDraftId',
content: {
id: 'mockNewDraftContentId',
title: 'New Draft Title'
},
children: [
{
id: 'do-not-change-me',
children: [
{ id: '00000000-0000-0000-0000-000000000001' },
{
id: '00000000-0000-0000-0000-000000000002',
reference: {
id: '00000000-0000-0000-0000-000000000001'
}
},
{ id: '00000000-0000-0000-0000-000000000003' }
]
}
]
}

const mockDraftRootToObject = jest.fn()
mockDraftRootToObject.mockReturnValueOnce(mockDraftObject)

const mockDraft = {
root: {
toObject: mockDraftRootToObject
},
// this would be handled by the Draft model, but we're mocking that so
// we have to do this ourselves
nodesById: new Map([
['do-not-change-me', {}],
['4baa2860-5219-404f-9d99-616ca8f81e41', {}],
['adcc55b7-e412-4f44-b64f-6c317021f812', {}],
['9d992860-3540-5219-8b4a-1e41616ca8f8', {}]
])
}

DraftPermissions.userHasPermissionToCopy.mockResolvedValueOnce(true)

Draft.fetchById = jest.fn()
Draft.fetchById.mockResolvedValueOnce(mockDraft)

return request(app)
.post('/drafts/mockDraftId/copy')
.send({ visitId: 'mockVisitId', title: 'New Draft Title' })
.then(() => {
expect(uuid).toHaveBeenCalledTimes(3)
// 99 = mock user id
expect(Draft.createWithContent).toHaveBeenCalledWith(99, expectedNewDraftDocument)
})
})

test('post /drafts/:draftId/copy returns the expected response when user can not copy draft', () => {
expect.hasAssertions()

Expand Down