From 999125f57254c4148dff78f33049b314425b51f8 Mon Sep 17 00:00:00 2001 From: Andrew Nicolaou Date: Tue, 11 Jun 2019 20:33:28 +0200 Subject: [PATCH 01/49] Converts import script to use public API endpoints The endpoints don't exist yet, but this is a good way to see how the implementation of the data structures differ. --- server/scripts/examples-ml5.js | 334 ++++++++++++++------------------- 1 file changed, 139 insertions(+), 195 deletions(-) diff --git a/server/scripts/examples-ml5.js b/server/scripts/examples-ml5.js index 0721fe5400..a04872ae99 100644 --- a/server/scripts/examples-ml5.js +++ b/server/scripts/examples-ml5.js @@ -1,11 +1,6 @@ import fs from 'fs'; import rp from 'request-promise'; import Q from 'q'; -import mongoose from 'mongoose'; -import objectID from 'bson-objectid'; -import shortid from 'shortid'; -import User from '../models/user'; -import Project from '../models/project'; // TODO: Change branchName if necessary const branchName = 'release'; @@ -14,11 +9,13 @@ const baseUrl = 'https://api.github.com/repos/ml5js/ml5-examples/contents'; const clientId = process.env.GITHUB_ID; const clientSecret = process.env.GITHUB_SECRET; const editorUsername = process.env.ML5_EXAMPLES_USERNAME; +const personalAccessToken = process.env.EDITOR_API_ACCESS_TOKEN; const headers = { 'User-Agent': 'p5js-web-editor/0.0.1' }; -const requestOptions = { +// +const githubRequestOptions = { url: baseUrl, qs: { client_id: clientId, @@ -29,14 +26,15 @@ const requestOptions = { json: true }; -const mongoConnectionString = process.env.MONGO_URL; -mongoose.connect(mongoConnectionString, { - useMongoClient: true -}); -mongoose.connection.on('error', () => { - console.error('MongoDB Connection Error. Please make sure that MongoDB is running.'); - process.exit(1); -}); +const editorRequestOptions = { + url: process.env.P5_API || 'http://localhost:8000/api', + method: 'GET', + headers: { + ...headers, + Authorization: `Basic ${Buffer.from(`${editorUsername}:${personalAccessToken}`).toString('base64')}` + }, + json: true +}; /** * --------------------------------------------------------- @@ -51,12 +49,52 @@ function flatten(list) { return list.reduce((a, b) => a.concat(Array.isArray(b) ? flatten(b) : b), []); } +/** + * Fetch data for a single HTML/JS file, or return + * an url to the file's CDN location + */ +async function fetchFileContent(item) { + const { name } = item; + const file = { url: item.url }; + + // if it is an html or js file + if ( + (file.url != null && name.endsWith('.html')) || + name.endsWith('.js') + ) { + const options = Object.assign({}, githubRequestOptions); + options.url = `${file.url}`; + + if ( + options.url !== undefined || + options.url !== null || + options.url !== '' + ) { + file.content = await rp(options); + // NOTE: remove the URL property if there's content + // Otherwise the p5 editor will try to pull from that url + if (file.content !== null) delete file.url; + } + + return file; + // if it is NOT an html or js file + } + + if (file.url) { + const cdnRef = `https://cdn.jsdelivr.net/gh/ml5js/ml5-examples@${branchName}${file.url.split(branchName)[1]}`; + file.url = cdnRef; + } + + return file; +} + + /** * STEP 1: Get the top level cateogories */ async function getCategories() { try { - const options = Object.assign({}, requestOptions); + const options = Object.assign({}, githubRequestOptions); options.url = `${options.url}/p5js${branchRef}`; const results = await rp(options); @@ -76,13 +114,18 @@ async function getCategoryExamples(sketchRootList) { const output = []; const sketchRootCategories = sketchRootList.map(async (categories) => { // let options = Object.assign({url: `${requestOptions.url}/${categories.path}${branchRef}`}, requestOptions) - const options = Object.assign({}, requestOptions); + const options = Object.assign({}, githubRequestOptions); options.url = `${options.url}${categories.path}${branchRef}`; // console.log(options) const sketchDirs = await rp(options); - const result = flatten(sketchDirs); - return result; + try { + const result = flatten(sketchDirs); + + return result; + } catch (err) { + return []; + } }); const sketchList = await Q.all(sketchRootCategories); @@ -107,7 +150,7 @@ async function traverseSketchTree(parentObject) { return output; } // let options = `https://api.github.com/repos/ml5js/ml5-examples/contents/${sketches.path}${branchRef}` - const options = Object.assign({}, requestOptions); + const options = Object.assign({}, githubRequestOptions); options.url = `${options.url}${parentObject.path}${branchRef}`; output.tree = await rp(options); @@ -124,7 +167,6 @@ async function traverseSketchTree(parentObject) { * @param {*} categoryExamples - all of the categories in an array */ async function traverseSketchTreeAll(categoryExamples) { - // const sketches = categoryExamples.map(async sketch => await traverseSketchTree(sketch)); const sketches = categoryExamples.map(async sketch => traverseSketchTree(sketch)); const result = await Q.all(sketches); @@ -139,37 +181,24 @@ function traverseAndFormat(parentObject) { const parent = Object.assign({}, parentObject); if (!parentObject.tree) { - const newid = objectID().toHexString(); // returns the files return { name: parent.name, - url: parent.download_url, - content: null, - id: newid, - _id: newid, - fileType: 'file' + url: parent.download_url }; } const subdir = parentObject.tree.map((item) => { - const newid = objectID().toHexString(); if (!item.tree) { // returns the files return { name: item.name, - url: item.download_url, - content: null, - id: newid, - _id: newid, - fileType: 'file' + url: item.download_url }; } const feat = { name: item.name, - id: newid, - _id: newid, - fileType: 'folder', children: traverseAndFormat(item) }; return feat; @@ -178,69 +207,27 @@ function traverseAndFormat(parentObject) { } /** - * Traverse the tree and flatten for project.files[] + * Traverse the tree and download all the content, + * transforming into an object keyed by file/directory name * @param {*} projectFileTree */ -function traverseAndFlatten(projectFileTree) { - const r = objectID().toHexString(); - - const projectRoot = { - name: 'root', - id: r, - _id: r, - children: [], - fileType: 'folder' - }; - - let currentParent; - - const output = projectFileTree.reduce( - (result, item, idx) => { - if (idx < projectFileTree.length) { - projectRoot.children.push(item.id); - } - - if (item.fileType === 'file') { - if (item.name === 'sketch.js') { - item.isSelectedFile = true; - } - result.push(item); - } - - // here's where the magic happens *twinkles* - if (item.fileType === 'folder') { - // recursively go down the tree of children - currentParent = traverseAndFlatten(item.children); - // the above will return an array of the children files - // concatenate that with the results - result = result.concat(currentParent); // eslint-disable-line no-param-reassign - // since we want to get the children ids, - // we can map the child ids to the current item - // then push that to our result array to get - // our flat files array. - item.children = item.children.map(child => child.id); - result.push(item); +async function traverseAndDownload(projectFileTree) { + return projectFileTree.reduce( + async (previousPromise, item, idx) => { + const result = await previousPromise; + + if (Array.isArray(item.children)) { + result[item.name] = { + files: await traverseAndDownload(item.children) + }; + } else { + result[item.name] = await fetchFileContent(item); } return result; }, - [projectRoot] + {} ); - - // Kind of hacky way to remove all roots other than the starting one - let counter = 0; - output.forEach((item, idx) => { - if (item.name === 'root') { - if (counter === 0) { - counter += 1; - } else { - output.splice(idx, 1); - counter += 1; - } - } - }); - - return output; } /** @@ -249,16 +236,14 @@ function traverseAndFlatten(projectFileTree) { * @param {*} sketch * @param {*} user */ -function formatSketchForStorage(sketch, user) { - const newProject = new Project({ - _id: shortid.generate(), +async function formatSketchForStorage(sketch, user) { + const newProject = { name: sketch.name, - user: user._id, - files: [] // <== add files to this array as file objects and add _id reference to children of root - }); + files: {} // <== add files to this object + }; let projectFiles = traverseAndFormat(sketch); - projectFiles = traverseAndFlatten(projectFiles); + projectFiles = await traverseAndDownload(projectFiles); newProject.files = projectFiles; return newProject; } @@ -271,68 +256,48 @@ function formatSketchForStorageAll(sketchWithItems, user) { sketchList = sketchList.map(sketch => formatSketchForStorage(sketch, user)); - return sketchList; + return Promise.all(sketchList); } /** - * Get all the content for the relevant files in project.files[] - * @param {*} projectObject + * Fetch a list of all projects from the API */ -async function fetchSketchContent(projectObject) { - const output = Object.assign({}, JSON.parse(JSON.stringify(projectObject))); - - const newFiles = output.files.map(async (item, i) => { - // if it is an html or js file - if ( - (item.fileType === 'file' && item.name.endsWith('.html')) || - item.name.endsWith('.js') - ) { - const options = Object.assign({}, requestOptions); - options.url = `${item.url}`; - - if ( - options.url !== undefined || - options.url !== null || - options.url !== '' - ) { - item.content = await rp(options); - // NOTE: remove the URL property if there's content - // Otherwise the p5 editor will try to pull from that url - if (item.content !== null) delete item.url; - } +async function getProjectsList() { + const options = Object.assign({}, editorRequestOptions); + options.url = `${options.url}/${editorUsername}/sketches`; + const results = await rp(options); - return item; - // if it is NOT an html or js file - } - - if (item.url) { - const cdnRef = `https://cdn.jsdelivr.net/gh/ml5js/ml5-examples@${branchName}${ - item.url.split(branchName)[1] - }`; - item.content = cdnRef; - item.url = cdnRef; - } + return results.sketches; +} - return item; - }); +/** + * Delete a project + */ +async function deleteProject(project) { + const options = Object.assign({}, editorRequestOptions); + options.method = 'DELETE'; + options.url = `${options.url}/sketches/${project.id}`; + const results = await rp(options); - output.files = await Q.all(newFiles); - return output; + return results; } /** - * STEP 5 - * Get all the content for the relevant files in project.files[] for all sketches - * @param {*} formattedSketchList + * Create a new project */ -async function fetchSketchContentAll(formattedSketchList) { - let output = formattedSketchList.slice(0); - - output = output.map(async item => fetchSketchContent(item)); +async function createProject(project) { + try { + const options = Object.assign({}, editorRequestOptions); + options.method = 'POST'; + options.url = `${options.url}/sketches`; + options.body = project; - output = await Q.all(output); + const results = await rp(options); - return output; + return results; + } catch (err) { + throw err; + } } /** @@ -342,43 +307,32 @@ async function fetchSketchContentAll(formattedSketchList) { * @param {*} user */ async function createProjectsInP5User(filledProjectList, user) { - const userProjects = await Project.find({ user: user._id }); - const removeProjects = userProjects.map(async project => Project.remove({ _id: project._id })); - await Q.all(removeProjects); - console.log('deleted old projects!'); - - const newProjects = filledProjectList.map(async (project) => { - const item = new Project(project); - console.log(`saving ${project.name}`); - await item.save(); - }); - await Q.all(newProjects); - console.log(`Projects saved to User: ${editorUsername}!`); -} + const existingProjects = await getProjectsList(); -/** - * STEP 0 - * CHECK if user exists, ifnot create one - * - */ -async function checkP5User() { - const user = await User.findOne({ username: editorUsername }); - - if (!user) { - const ml5user = new User({ - username: editorUsername, - email: process.env.ML5_EXAMPLES_EMAIL, - password: process.env.ML5_EXAMPLES_PASS - }); + console.log('**', existingProjects) + + try { + await Q.all(existingProjects.map(deleteProject)); + console.log('deleted old projects!'); + } catch (error) { + console.log('Problem deleting projects'); + console.log(error); + process.exit(1); + } - await ml5user.save((saveErr) => { - if (saveErr) throw saveErr; - console.log(`Created a user p5${ml5user}`); + try { + const newProjects = filledProjectList.map(async (project) => { + console.log(`saving ${project.name}`); + await createProject(project); }); + await Q.all(newProjects); + console.log(`Projects saved to User: ${editorUsername}!`); + } catch (error) { + console.log('Error saving projects'); + console.log(error); } } - /** * --------------------------------------------------------- * --------------------- main ------------------------------ @@ -394,18 +348,14 @@ async function checkP5User() { * Delete existing and save */ async function make() { - await checkP5User(); - // Get the user - const user = await User.findOne({ - username: editorUsername - }); // Get the categories and their examples const categories = await getCategories(); const categoryExamples = await getCategoryExamples(categories); + const examplesWithResourceTree = await traverseSketchTreeAll(categoryExamples); - const formattedSketchList = formatSketchForStorageAll(examplesWithResourceTree, user); - const filledProjectList = await fetchSketchContentAll(formattedSketchList); - await createProjectsInP5User(filledProjectList, user); + const formattedSketchList = await formatSketchForStorageAll(examplesWithResourceTree); + + await createProjectsInP5User(formattedSketchList); console.log('done!'); process.exit(); } @@ -418,20 +368,14 @@ async function make() { * Format the sketch files to be save to the db * Delete existing and save */ +// eslint-disable-next-line no-unused-vars async function test() { - await checkP5User(); - // Get the user - const user = await User.findOne({ - username: editorUsername - }); - // read from file while testing const examplesWithResourceTree = JSON.parse(fs.readFileSync('./ml5-examplesWithResourceTree.json')); - const formattedSketchList = formatSketchForStorageAll(examplesWithResourceTree, user); + const formattedSketchList = await formatSketchForStorageAll(examplesWithResourceTree); - const filledProjectList = await fetchSketchContentAll(formattedSketchList); - await createProjectsInP5User(filledProjectList, user); + await createProjectsInP5User(formattedSketchList); console.log('done!'); process.exit(); } From e9a1baefc588c76a3cf967c4106957210c0dc59a Mon Sep 17 00:00:00 2001 From: Andrew Nicolaou Date: Wed, 12 Jun 2019 09:42:36 +0200 Subject: [PATCH 02/49] Exposes public API endpoint to fetch user's sketches --- server/controllers/project.controller.js | 80 ++++++++++++++++++++---- server/domain-objects/Project.js | 15 +++++ server/routes/api.routes.js | 25 ++++++++ server/scripts/examples-ml5.js | 10 ++- server/server.js | 4 +- 5 files changed, 119 insertions(+), 15 deletions(-) create mode 100644 server/domain-objects/Project.js create mode 100644 server/routes/api.routes.js diff --git a/server/controllers/project.controller.js b/server/controllers/project.controller.js index 527ac64ab2..651faffcb4 100644 --- a/server/controllers/project.controller.js +++ b/server/controllers/project.controller.js @@ -11,6 +11,7 @@ import User from '../models/user'; import { resolvePathToFile } from '../utils/filePath'; import generateFileSystemSafeName from '../utils/generateFileSystemSafeName'; import { deleteObjectsFromS3, getObjectKey } from './aws.controller'; +import { toApi as toApiProjectObject } from '../domain-objects/Project'; export { default as createProject } from './project.controller/createProject'; @@ -157,8 +158,44 @@ export function getProjectAsset(req, res) { }); } -export function getProjectsForUserName(username) { +class UserNotFoundError extends Error { + constructor(message, extra) { + super(); + if (Error.captureStackTrace) { + Error.captureStackTrace(this, this.constructor); + } + this.name = 'UserNotFoundError'; + this.message = message; + if (extra) this.extra = extra; + } +} + +function getProjectsForUserName(username) { + return new Promise((resolve, reject) => { + User.findOne({ username }, (err, user) => { + if (err) { + reject(err); + return; + } + + if (!user) { + reject(new UserNotFoundError()); + return; + } + + Project.find({ user: user._id }) + .sort('-createdAt') + .select('name files id createdAt updatedAt') + .exec((innerErr, projects) => { + if (innerErr) { + reject(innerErr); + return; + } + resolve(projects); + }); + }); + }); } export function getProjects(req, res) { @@ -175,22 +212,43 @@ export function getProjects(req, res) { export function getProjectsForUser(req, res) { if (req.params.username) { - User.findOne({ username: req.params.username }, (err, user) => { - if (!user) { - res.status(404).json({ message: 'User with that username does not exist.' }); - return; - } - Project.find({ user: user._id }) - .sort('-createdAt') - .select('name files id createdAt updatedAt') - .exec((innerErr, projects) => res.json(projects)); - }); + getProjectsForUserName(req.params.username) + .then(projects => res.json(projects)) + .catch((err) => { + if (err instanceof UserNotFoundError) { + res.status(404).json({ message: 'User with that username does not exist.' }); + } else { + res.status(500).json({ message: 'Error fetching projects' }); + } + }); } else { // could just move this to client side res.json([]); } } +export function apiGetProjectsForUser(req, res) { + if (req.params.username) { + getProjectsForUserName(req.params.username) + .then((projects) => { + const asApiObjects = projects.map(p => toApiProjectObject(p)); + res.json({ sketches: asApiObjects }); + }) + .catch((err) => { + if (err instanceof UserNotFoundError) { + res.status(404).json({ message: 'User with that username does not exist.' }); + } else { + console.error(err); + res.status(500).json({ message: 'Error fetching projects' }); + } + }); + } else { + // could just move this to client side + res.json({ sketches: [] }); + } +} + + export function projectExists(projectId, callback) { Project.findById(projectId, (err, project) => ( project ? callback(true) : callback(false) diff --git a/server/domain-objects/Project.js b/server/domain-objects/Project.js new file mode 100644 index 0000000000..6f2734b4ce --- /dev/null +++ b/server/domain-objects/Project.js @@ -0,0 +1,15 @@ +/** + * This converts between a mongoose Project model + * and the public API Project object properties + * + */ +export function toApi(model) { + return { + id: model.id, + name: model.name, + }; +} + +export function toModel(object) { + +} diff --git a/server/routes/api.routes.js b/server/routes/api.routes.js new file mode 100644 index 0000000000..e1c722f66d --- /dev/null +++ b/server/routes/api.routes.js @@ -0,0 +1,25 @@ +import { Router } from 'express'; +import passport from 'passport'; +import * as ProjectController from '../controllers/project.controller'; + +const router = new Router(); + +router.get( + '/:username/sketches', + passport.authenticate('basic', { session: false }), + ProjectController.apiGetProjectsForUser +); + +// router.post( +// '/:username/sketches', +// passport.authenticate('basic', { session: false }), +// ProjectController.apiCreateProject +// ); + +// router.delete( +// '/:username/sketches/:id', +// passport.authenticate('basic', { session: false }), +// ProjectController.deleteProject +// ); + +export default router; diff --git a/server/scripts/examples-ml5.js b/server/scripts/examples-ml5.js index a04872ae99..6285d93570 100644 --- a/server/scripts/examples-ml5.js +++ b/server/scripts/examples-ml5.js @@ -27,7 +27,7 @@ const githubRequestOptions = { }; const editorRequestOptions = { - url: process.env.P5_API || 'http://localhost:8000/api', + url: `${process.env.P5_API || 'http://localhost:8000/api'}/${editorUsername}`, method: 'GET', headers: { ...headers, @@ -264,7 +264,8 @@ function formatSketchForStorageAll(sketchWithItems, user) { */ async function getProjectsList() { const options = Object.assign({}, editorRequestOptions); - options.url = `${options.url}/${editorUsername}/sketches`; + options.url = `${options.url}/sketches`; + const results = await rp(options); return results.sketches; @@ -277,6 +278,7 @@ async function deleteProject(project) { const options = Object.assign({}, editorRequestOptions); options.method = 'DELETE'; options.url = `${options.url}/sketches/${project.id}`; + const results = await rp(options); return results; @@ -307,9 +309,11 @@ async function createProject(project) { * @param {*} user */ async function createProjectsInP5User(filledProjectList, user) { + console.log('Finding existing projects...'); + const existingProjects = await getProjectsList(); - console.log('**', existingProjects) + console.log(`Will delete ${existingProjects.length} projects`); try { await Q.all(existingProjects.map(deleteProject)); diff --git a/server/server.js b/server/server.js index eadfcb067d..0bae932228 100644 --- a/server/server.js +++ b/server/server.js @@ -16,6 +16,7 @@ import webpackHotMiddleware from 'webpack-hot-middleware'; import config from '../webpack/config.dev'; // Import all required modules +import api from './routes/api.routes'; import users from './routes/user.routes'; import sessions from './routes/session.routes'; import projects from './routes/project.routes'; @@ -51,7 +52,7 @@ const corsOriginsWhitelist = [ // Run Webpack dev server in development mode if (process.env.NODE_ENV === 'development') { const compiler = webpack(config); - app.use(webpackDevMiddleware(compiler, { noInfo: true, publicPath: config[0].output.publicPath })); + app.use(webpackDevMiddleware(compiler, { lazy: false, noInfo: true, publicPath: config[0].output.publicPath })); app.use(webpackHotMiddleware(compiler)); corsOriginsWhitelist.push(/localhost/); @@ -95,6 +96,7 @@ app.use(session({ app.use(passport.initialize()); app.use(passport.session()); +app.use('/api', requestsOfTypeJSON(), api); app.use('/api', requestsOfTypeJSON(), users); app.use('/api', requestsOfTypeJSON(), sessions); app.use('/api', requestsOfTypeJSON(), files); From c45f24f2383266859e8138656b290f5e928507fb Mon Sep 17 00:00:00 2001 From: Andrew Nicolaou Date: Wed, 12 Jun 2019 10:12:02 +0200 Subject: [PATCH 03/49] Implements public API delete endpoint --- server/controllers/project.controller.js | 4 ++-- server/routes/api.routes.js | 12 +++++++----- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/server/controllers/project.controller.js b/server/controllers/project.controller.js index 651faffcb4..f7cd742a91 100644 --- a/server/controllers/project.controller.js +++ b/server/controllers/project.controller.js @@ -102,11 +102,11 @@ function deleteFilesFromS3(files) { export function deleteProject(req, res) { Project.findById(req.params.project_id, (findProjectErr, project) => { if (!project.user.equals(req.user._id)) { - res.status(403).json({ success: false, message: 'Session does not match owner of project.' }); + res.status(403).json({ success: false, message: 'Authenticated user does not match owner of project.' }); return; } deleteFilesFromS3(project.files); - Project.remove({ _id: req.params.project_id }, (removeProjectError) => { + project.remove((removeProjectError) => { if (removeProjectError) { res.status(404).send({ message: 'Project with that id does not exist' }); return; diff --git a/server/routes/api.routes.js b/server/routes/api.routes.js index e1c722f66d..2bf1e7a598 100644 --- a/server/routes/api.routes.js +++ b/server/routes/api.routes.js @@ -16,10 +16,12 @@ router.get( // ProjectController.apiCreateProject // ); -// router.delete( -// '/:username/sketches/:id', -// passport.authenticate('basic', { session: false }), -// ProjectController.deleteProject -// ); +// NOTE: Currently :username will not be checked for ownership +// only the project's owner in the database. +router.delete( + '/:username/sketches/:project_id', + passport.authenticate('basic', { session: false }), + ProjectController.deleteProject +); export default router; From aa6cbe501b08c931eec980f30126798b7cc9074c Mon Sep 17 00:00:00 2001 From: Andrew Nicolaou Date: Mon, 17 Jun 2019 09:45:45 +0200 Subject: [PATCH 04/49] Adds helper to create custom ApplicationError classes --- server/controllers/project.controller.js | 14 ++------- server/utils/createApplicationErrorClass.js | 35 +++++++++++++++++++++ 2 files changed, 37 insertions(+), 12 deletions(-) create mode 100644 server/utils/createApplicationErrorClass.js diff --git a/server/controllers/project.controller.js b/server/controllers/project.controller.js index f7cd742a91..169ca6be3b 100644 --- a/server/controllers/project.controller.js +++ b/server/controllers/project.controller.js @@ -12,8 +12,10 @@ import { resolvePathToFile } from '../utils/filePath'; import generateFileSystemSafeName from '../utils/generateFileSystemSafeName'; import { deleteObjectsFromS3, getObjectKey } from './aws.controller'; import { toApi as toApiProjectObject } from '../domain-objects/Project'; +import createApplicationErrorClass from '../utils/createApplicationErrorClass'; export { default as createProject } from './project.controller/createProject'; +const UserNotFoundError = createApplicationErrorClass('UserNotFoundError'); export function updateProject(req, res) { Project.findById(req.params.project_id, (findProjectErr, project) => { @@ -158,18 +160,6 @@ export function getProjectAsset(req, res) { }); } -class UserNotFoundError extends Error { - constructor(message, extra) { - super(); - if (Error.captureStackTrace) { - Error.captureStackTrace(this, this.constructor); - } - this.name = 'UserNotFoundError'; - this.message = message; - if (extra) this.extra = extra; - } -} - function getProjectsForUserName(username) { return new Promise((resolve, reject) => { User.findOne({ username }, (err, user) => { diff --git a/server/utils/createApplicationErrorClass.js b/server/utils/createApplicationErrorClass.js new file mode 100644 index 0000000000..f7dbc483e2 --- /dev/null +++ b/server/utils/createApplicationErrorClass.js @@ -0,0 +1,35 @@ +/** + * This is the base class for custom errors in + * the application. + */ +export class ApplicationError extends Error { + constructor(message, extra) { + super(); + if (Error.captureStackTrace) { + Error.captureStackTrace(this, this.constructor); + } + this.name = 'ApplicationError'; + this.message = message; + if (extra) this.extra = extra; + } +} + +/** + * Create a new custom error class e.g. + * const UserNotFoundError = createApplicationErrorClass('UserNotFoundError'); + * + * // Later + * throw new UserNotFoundError(`user ${user.name} not found`); + * + */ +export default function createApplicationErrorClass(name) { + const CustomError = class extends ApplicationError { + constructor(...params) { + super(...params); + + this.name = name; + } + }; + + return CustomError; +} From 075f9b6b7900a6709e00178d3f621d4a5192d3c1 Mon Sep 17 00:00:00 2001 From: Andrew Nicolaou Date: Mon, 17 Jun 2019 12:15:26 +0200 Subject: [PATCH 05/49] Adds create project endpoint that understand API's data structure This transforms the nested tree of file data into a mongoose Project model --- server/controllers/project.controller.js | 3 +- .../project.controller/createProject.js | 25 ++ server/domain-objects/Project.js | 85 ++++++ .../domain-objects/__test__/Project.test.js | 244 ++++++++++++++++++ server/routes/api.routes.js | 10 +- server/utils/__mocks__/createId.js | 16 ++ server/utils/createId.js | 8 + 7 files changed, 385 insertions(+), 6 deletions(-) create mode 100644 server/domain-objects/__test__/Project.test.js create mode 100644 server/utils/__mocks__/createId.js create mode 100644 server/utils/createId.js diff --git a/server/controllers/project.controller.js b/server/controllers/project.controller.js index 169ca6be3b..e8a7584dd0 100644 --- a/server/controllers/project.controller.js +++ b/server/controllers/project.controller.js @@ -14,7 +14,8 @@ import { deleteObjectsFromS3, getObjectKey } from './aws.controller'; import { toApi as toApiProjectObject } from '../domain-objects/Project'; import createApplicationErrorClass from '../utils/createApplicationErrorClass'; -export { default as createProject } from './project.controller/createProject'; +export { default as createProject, apiCreateProject } from './project.controller/createProject'; + const UserNotFoundError = createApplicationErrorClass('UserNotFoundError'); export function updateProject(req, res) { diff --git a/server/controllers/project.controller/createProject.js b/server/controllers/project.controller/createProject.js index 2cf34e325e..77ff31bc65 100644 --- a/server/controllers/project.controller/createProject.js +++ b/server/controllers/project.controller/createProject.js @@ -1,4 +1,5 @@ import Project from '../../models/project'; +import { toModel } from '../../domain-objects/Project'; export default function createProject(req, res) { let projectValues = { @@ -30,3 +31,27 @@ export default function createProject(req, res) { .then(populateUserData) .catch(sendFailure); } + +// TODO: What happens if you don't supply any files? +export function apiCreateProject(req, res) { + const params = Object.assign({ user: req.user._id }, req.body); + + // TODO: Error handling to match spec + function sendFailure() { + res.json({ success: false }); + } + + try { + const model = toModel(params); + + model + .save() + .then((newProject) => { + res.json({ ok: true }); + }) + .catch(sendFailure); + } catch (err) { + // TODO: Catch custom err object and return correct status code + res.status(422).json({ error: 'Validation error' }); + } +} diff --git a/server/domain-objects/Project.js b/server/domain-objects/Project.js index 6f2734b4ce..5295a2f951 100644 --- a/server/domain-objects/Project.js +++ b/server/domain-objects/Project.js @@ -1,3 +1,8 @@ +import pick from 'lodash/pick'; +import Project from '../models/project'; +import createId from '../utils/createId'; + +// objectID().toHexString(); /** * This converts between a mongoose Project model * and the public API Project object properties @@ -10,6 +15,86 @@ export function toApi(model) { }; } +/** + * Transforms a tree of files matching the APIs DirectoryContents + * format into the data structure stored in mongodb + * + * - flattens the tree into an array of file/folders + * - each file/folder gets a generated BSON-ID + * - each folder has a `children` array containing the IDs of it's children + */ +function transformFilesInner(files = {}, parentNode) { + const allFiles = []; + + Object.entries(files).forEach(([name, params]) => { + const id = createId(); + const isFolder = params.files != null; + + if (isFolder) { + const folder = { + _id: id, + name, + fileType: 'folder', + children: [] // Initialise an empty folder + }; + + allFiles.push(folder); + + // The recursion will return a list of child files/folders + // It will also push the child's id into `folder.children` + allFiles.push(...transformFilesInner(params.files, folder)); + } else { + const file = { + _id: id, + name, + fileType: 'file' + }; + + if (typeof params.url === 'string') { + file.url = params.url; + } else if (typeof params.content === 'string') { + file.content = params.content; + } else { + throw new Error('url or params must be supplied'); + } + + allFiles.push(file); + } + + // Push this child's ID onto it's parent's list + // of children + if (parentNode != null) { + parentNode.children.push(id); + } + }); + + return allFiles; +} + +export function transformFiles(files = {}) { + const withRoot = { + root: { + files + } + }; + + return transformFilesInner(withRoot); +} + +/** + * This converts between the public API's Project object + * properties and a mongoose Project model + * + */ export function toModel(object) { + let files = []; + + if (typeof object.files === 'object') { + files = transformFiles(object.files); + } + + const projectValues = pick(object, ['user', 'name', 'slug']); + projectValues.files = files; + return new Project(projectValues); } diff --git a/server/domain-objects/__test__/Project.test.js b/server/domain-objects/__test__/Project.test.js new file mode 100644 index 0000000000..c5180d1ffd --- /dev/null +++ b/server/domain-objects/__test__/Project.test.js @@ -0,0 +1,244 @@ +import { transformFiles } from '../Project'; + +jest.mock('../../utils/createId'); + +// TODO: File name validation +// TODO: File extension validation +// +describe('domain-objects/Project', () => { + describe('transformFiles', () => { + beforeEach(() => { + // eslint-disable-next-line global-require + const { resetMockCreateId } = require('../../utils/createId'); + + resetMockCreateId(); + }); + + it('creates an empty root with no data', () => { + const tree = {}; + + expect(transformFiles(tree)).toEqual([{ + _id: '0', + fileType: 'folder', + name: 'root', + children: [] + }]); + }); + + it('converts tree-shaped files into list', () => { + const tree = { + 'index.html': { + content: 'some contents', + } + }; + + expect(transformFiles(tree)).toEqual([ + { + _id: '0', + fileType: 'folder', + name: 'root', + children: ['1'] + }, + { + _id: '1', + content: 'some contents', + fileType: 'file', + name: 'index.html' + } + ]); + }); + + it('uses file url over content', () => { + const tree = { + 'script.js': { + url: 'http://example.net/something.js' + } + }; + + expect(transformFiles(tree)).toEqual([ + { + _id: '0', + fileType: 'folder', + name: 'root', + children: ['1'] + }, + { + _id: '1', + url: 'http://example.net/something.js', + fileType: 'file', + name: 'script.js' + } + ]); + }); + + it('creates folders', () => { + const tree = { + 'a-folder': { + files: {} + }, + }; + + expect(transformFiles(tree)).toEqual([ + { + _id: '0', + fileType: 'folder', + name: 'root', + children: ['1'] + }, + { + _id: '1', + children: [], + fileType: 'folder', + name: 'a-folder' + } + ]); + }); + + it('walks the tree processing files', () => { + const tree = { + 'index.html': { + content: 'some contents', + }, + 'a-folder': { + files: { + 'data.csv': { + content: 'this,is,data' + }, + 'another.txt': { + content: 'blah blah' + } + } + }, + }; + + expect(transformFiles(tree)).toEqual([ + { + _id: '0', + fileType: 'folder', + name: 'root', + children: ['1', '2'] + }, + { + _id: '1', + name: 'index.html', + fileType: 'file', + content: 'some contents' + }, + { + _id: '2', + name: 'a-folder', + fileType: 'folder', + children: ['3', '4'] + }, + { + _id: '3', + name: 'data.csv', + fileType: 'file', + content: 'this,is,data' + }, + { + _id: '4', + name: 'another.txt', + fileType: 'file', + content: 'blah blah' + } + ]); + }); + + it('handles deep nesting', () => { + const tree = { + first: { + files: { + second: { + files: { + third: { + files: { + 'hello.js': { + content: 'world!' + } + } + } + } + } + } + }, + }; + + expect(transformFiles(tree)).toEqual([ + { + _id: '0', + fileType: 'folder', + name: 'root', + children: ['1'] + }, + { + _id: '1', + name: 'first', + fileType: 'folder', + children: ['2'] + }, + { + _id: '2', + name: 'second', + fileType: 'folder', + children: ['3'] + }, + { + _id: '3', + name: 'third', + fileType: 'folder', + children: ['4'] + }, + { + _id: '4', + name: 'hello.js', + fileType: 'file', + content: 'world!' + } + ]); + }); + + + it('allows duplicate names in different folder', () => { + const tree = { + 'index.html': { + content: 'some contents', + }, + 'data': { + files: { + 'index.html': { + content: 'different file' + } + } + }, + }; + + expect(transformFiles(tree)).toEqual([ + { + _id: '0', + fileType: 'folder', + name: 'root', + children: ['1', '2'] + }, + { + _id: '1', + name: 'index.html', + fileType: 'file', + content: 'some contents' + }, + { + _id: '2', + name: 'data', + fileType: 'folder', + children: ['3'] + }, + { + _id: '3', + name: 'index.html', + fileType: 'file', + content: 'different file' + } + ]); + }); + }); +}); diff --git a/server/routes/api.routes.js b/server/routes/api.routes.js index 2bf1e7a598..60be3494cf 100644 --- a/server/routes/api.routes.js +++ b/server/routes/api.routes.js @@ -10,11 +10,11 @@ router.get( ProjectController.apiGetProjectsForUser ); -// router.post( -// '/:username/sketches', -// passport.authenticate('basic', { session: false }), -// ProjectController.apiCreateProject -// ); +router.post( + '/:username/sketches', + passport.authenticate('basic', { session: false }), + ProjectController.apiCreateProject +); // NOTE: Currently :username will not be checked for ownership // only the project's owner in the database. diff --git a/server/utils/__mocks__/createId.js b/server/utils/__mocks__/createId.js new file mode 100644 index 0000000000..919ea25207 --- /dev/null +++ b/server/utils/__mocks__/createId.js @@ -0,0 +1,16 @@ +/** + * Creates an increasing numeric ID + * for testing + */ +let nextId = 0; + +export default function mockCreateId() { + const id = nextId; + nextId += 1; + + return String(id); +} + +export function resetMockCreateId() { + nextId = 0; +} diff --git a/server/utils/createId.js b/server/utils/createId.js new file mode 100644 index 0000000000..65b8290fa2 --- /dev/null +++ b/server/utils/createId.js @@ -0,0 +1,8 @@ +import objectID from 'bson-objectid'; + +/** + * Creates a mongo ID + */ +export default function createId() { + return objectID().toHexString(); +} From cef4dca39144c1142236cb875853ce4f41ac2dea Mon Sep 17 00:00:00 2001 From: Andrew Nicolaou Date: Wed, 19 Jun 2019 12:11:55 +0200 Subject: [PATCH 06/49] Returns '201 Created' to match API spec --- server/controllers/project.controller/createProject.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/controllers/project.controller/createProject.js b/server/controllers/project.controller/createProject.js index 77ff31bc65..f6f56d6318 100644 --- a/server/controllers/project.controller/createProject.js +++ b/server/controllers/project.controller/createProject.js @@ -47,7 +47,7 @@ export function apiCreateProject(req, res) { model .save() .then((newProject) => { - res.json({ ok: true }); + res.status(201).json({ id: newProject.id }); }) .catch(sendFailure); } catch (err) { From 8699c1b8696b1f534fae0a598384f6318873d2bb Mon Sep 17 00:00:00 2001 From: Andrew Nicolaou Date: Wed, 19 Jun 2019 12:12:28 +0200 Subject: [PATCH 07/49] Removes 'CustomError' variable assignment as it shows up in test output --- server/utils/createApplicationErrorClass.js | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/server/utils/createApplicationErrorClass.js b/server/utils/createApplicationErrorClass.js index f7dbc483e2..f5499d85d1 100644 --- a/server/utils/createApplicationErrorClass.js +++ b/server/utils/createApplicationErrorClass.js @@ -3,14 +3,13 @@ * the application. */ export class ApplicationError extends Error { - constructor(message, extra) { + constructor(message) { super(); if (Error.captureStackTrace) { Error.captureStackTrace(this, this.constructor); } this.name = 'ApplicationError'; this.message = message; - if (extra) this.extra = extra; } } @@ -23,13 +22,11 @@ export class ApplicationError extends Error { * */ export default function createApplicationErrorClass(name) { - const CustomError = class extends ApplicationError { + return class extends ApplicationError { constructor(...params) { super(...params); this.name = name; } }; - - return CustomError; } From dbeada53f9a02517e0ddee034ae5b19d934fd780 Mon Sep 17 00:00:00 2001 From: Andrew Nicolaou Date: Wed, 19 Jun 2019 12:12:51 +0200 Subject: [PATCH 08/49] transformFiles will return file validation errors --- server/domain-objects/Project.js | 43 ++++++++++++++----- .../domain-objects/__test__/Project.test.js | 30 ++++++++++++- 2 files changed, 61 insertions(+), 12 deletions(-) diff --git a/server/domain-objects/Project.js b/server/domain-objects/Project.js index 5295a2f951..19f2529740 100644 --- a/server/domain-objects/Project.js +++ b/server/domain-objects/Project.js @@ -1,6 +1,10 @@ import pick from 'lodash/pick'; import Project from '../models/project'; import createId from '../utils/createId'; +import createApplicationErrorClass from '../utils/createApplicationErrorClass'; + +export const FileValidationError = createApplicationErrorClass('FileValidationError'); + // objectID().toHexString(); /** @@ -23,10 +27,11 @@ export function toApi(model) { * - each file/folder gets a generated BSON-ID * - each folder has a `children` array containing the IDs of it's children */ -function transformFilesInner(files = {}, parentNode) { - const allFiles = []; +function transformFilesInner(tree = {}, parentNode) { + const files = []; + const errors = []; - Object.entries(files).forEach(([name, params]) => { + Object.entries(tree).forEach(([name, params]) => { const id = createId(); const isFolder = params.files != null; @@ -38,11 +43,13 @@ function transformFilesInner(files = {}, parentNode) { children: [] // Initialise an empty folder }; - allFiles.push(folder); + files.push(folder); // The recursion will return a list of child files/folders // It will also push the child's id into `folder.children` - allFiles.push(...transformFilesInner(params.files, folder)); + const subFolder = transformFilesInner(params.files, folder); + files.push(...subFolder.files); + errors.push(...subFolder.errors); } else { const file = { _id: id, @@ -55,10 +62,10 @@ function transformFilesInner(files = {}, parentNode) { } else if (typeof params.content === 'string') { file.content = params.content; } else { - throw new Error('url or params must be supplied'); + errors.push({ name, message: 'missing \'url\' or \'content\'' }); } - allFiles.push(file); + files.push(file); } // Push this child's ID onto it's parent's list @@ -68,17 +75,31 @@ function transformFilesInner(files = {}, parentNode) { } }); - return allFiles; + return { files, errors }; } -export function transformFiles(files = {}) { +export function transformFiles(tree = {}) { const withRoot = { root: { - files + files: tree } }; - return transformFilesInner(withRoot); + const { files, errors } = transformFilesInner(withRoot); + + if (errors.length > 0) { + const message = `${errors.length} files failed validation. See error.files for individual errors. + + Errors: + ${errors.map(e => `* ${e.name}: ${e.message}`).join('\n')} +`; + const error = new FileValidationError(message); + error.files = errors; + + throw error; + } + + return files; } /** diff --git a/server/domain-objects/__test__/Project.test.js b/server/domain-objects/__test__/Project.test.js index c5180d1ffd..f9de8b33e2 100644 --- a/server/domain-objects/__test__/Project.test.js +++ b/server/domain-objects/__test__/Project.test.js @@ -1,4 +1,4 @@ -import { transformFiles } from '../Project'; +import { transformFiles, FileValidationError } from '../Project'; jest.mock('../../utils/createId'); @@ -240,5 +240,33 @@ describe('domain-objects/Project', () => { } ]); }); + + it('validates files', () => { + const tree = { + 'index.html': {} // missing `content` + }; + + expect(() => transformFiles(tree)).toThrowError(FileValidationError); + }); + + it('collects all file validation errors', () => { + const tree = { + 'index.html': {}, // missing `content` + 'something.js': {} // also missing `content` + }; + + try { + transformFiles(tree); + + // Should not get here + throw new Error('should have thrown before this point'); + } catch (err) { + expect(err).toBeInstanceOf(FileValidationError); + expect(err.files).toEqual([ + { name: 'index.html', message: 'missing \'url\' or \'content\'' }, + { name: 'something.js', message: 'missing \'url\' or \'content\'' } + ]); + } + }); }); }); From a38527d576e36f72402135286f595531719157b6 Mon Sep 17 00:00:00 2001 From: Andrew Nicolaou Date: Wed, 19 Jun 2019 15:10:22 +0200 Subject: [PATCH 09/49] Tests API project controller --- .../__test__/project.controller.test.js | 120 +++++++++++++++++- .../project.controller/createProject.js | 23 +++- server/domain-objects/Project.js | 5 +- server/models/__mocks__/project.js | 13 ++ 4 files changed, 153 insertions(+), 8 deletions(-) diff --git a/server/controllers/__test__/project.controller.test.js b/server/controllers/__test__/project.controller.test.js index e0852e618e..06538e8b77 100644 --- a/server/controllers/__test__/project.controller.test.js +++ b/server/controllers/__test__/project.controller.test.js @@ -2,9 +2,10 @@ * @jest-environment node */ import { Response } from 'jest-express'; +import sinon from 'sinon'; -import { createMock } from '../../models/project'; -import createProject from '../project.controller/createProject'; +import Project, { createMock, createInstanceMock } from '../../models/project'; +import createProject, { apiCreateProject } from '../project.controller/createProject'; jest.mock('../../models/project'); @@ -152,4 +153,119 @@ describe('project.controller', () => { promise.then(expectations, expectations).catch(expectations); }); }); + + describe('apiCreateProject()', () => { + let ProjectMock; + let ProjectInstanceMock; + + beforeEach(() => { + ProjectMock = createMock(); + ProjectInstanceMock = createInstanceMock(); + }); + + afterEach(() => { + ProjectMock.restore(); + ProjectInstanceMock.restore(); + }); + + it('returns 201 with id of created sketch', (done) => { + const request = { + user: { _id: 'abc123' }, + body: { + name: 'My sketch', + files: {} + } + }; + const response = new Response(); + + const result = { + _id: 'abc123', + id: 'abc123', + name: 'Project name', + serveSecure: false, + files: [] + }; + + ProjectInstanceMock.expects('save') + .resolves(new Project(result)); + + const promise = apiCreateProject(request, response); + + function expectations() { + const doc = response.json.mock.calls[0][0]; + + expect(response.status).toHaveBeenCalledWith(201); + expect(response.json).toHaveBeenCalled(); + + expect(JSON.parse(JSON.stringify(doc))).toMatchObject({ + id: 'abc123' + }); + + done(); + } + + promise.then(expectations, expectations).catch(expectations); + }); + + it('returns validation errors on files input', (done) => { + const request = { + user: {}, + body: { + name: 'My sketch', + files: { + 'index.html': { + // missing content or url + } + } + } + }; + const response = new Response(); + + const promise = apiCreateProject(request, response); + + function expectations() { + const doc = response.json.mock.calls[0][0]; + + const responseBody = JSON.parse(JSON.stringify(doc)); + + expect(response.status).toHaveBeenCalledWith(422); + expect(responseBody.name).toBe('File Validation Failed'); + expect(responseBody.errors.length).toBe(1); + expect(responseBody.errors).toEqual([ + { name: 'index.html', message: 'missing \'url\' or \'content\'' } + ]); + + done(); + } + + promise.then(expectations, expectations).catch(expectations); + }); + + it('rejects file parameters not in object format', (done) => { + const request = { + user: { _id: 'abc123' }, + body: { + name: 'Wriggly worm', + files: [{ name: 'file.js', content: 'var hello = true;' }] + } + }; + const response = new Response(); + + const promise = apiCreateProject(request, response); + + function expectations() { + const doc = response.json.mock.calls[0][0]; + + const responseBody = JSON.parse(JSON.stringify(doc)); + + expect(response.status).toHaveBeenCalledWith(422); + expect(responseBody.name).toBe('File Validation Failed'); + expect(responseBody.message).toBe('\'files\' must be an object'); + + done(); + } + + promise.then(expectations, expectations).catch(expectations); + }); + }); }); diff --git a/server/controllers/project.controller/createProject.js b/server/controllers/project.controller/createProject.js index f6f56d6318..56ae8048ca 100644 --- a/server/controllers/project.controller/createProject.js +++ b/server/controllers/project.controller/createProject.js @@ -1,5 +1,5 @@ import Project from '../../models/project'; -import { toModel } from '../../domain-objects/Project'; +import { toModel, FileValidationError } from '../../domain-objects/Project'; export default function createProject(req, res) { let projectValues = { @@ -36,22 +36,35 @@ export default function createProject(req, res) { export function apiCreateProject(req, res) { const params = Object.assign({ user: req.user._id }, req.body); + function sendValidationErrors(err) { + res.status(422).json({ + name: 'File Validation Failed', + message: err.message, + errors: err.files, + }); + } + // TODO: Error handling to match spec function sendFailure() { - res.json({ success: false }); + res.status(500).json({ success: false }); } try { const model = toModel(params); - model + return model .save() .then((newProject) => { res.status(201).json({ id: newProject.id }); }) .catch(sendFailure); } catch (err) { - // TODO: Catch custom err object and return correct status code - res.status(422).json({ error: 'Validation error' }); + if (err instanceof FileValidationError) { + sendValidationErrors(err); + } else { + sendFailure(); + } + + return Promise.reject(); } } diff --git a/server/domain-objects/Project.js b/server/domain-objects/Project.js index 19f2529740..c9e3a2964a 100644 --- a/server/domain-objects/Project.js +++ b/server/domain-objects/Project.js @@ -1,3 +1,4 @@ +import isPlainObject from 'lodash/isPlainObject'; import pick from 'lodash/pick'; import Project from '../models/project'; import createId from '../utils/createId'; @@ -110,8 +111,10 @@ export function transformFiles(tree = {}) { export function toModel(object) { let files = []; - if (typeof object.files === 'object') { + if (isPlainObject(object.files)) { files = transformFiles(object.files); + } else if (object.files != null) { + throw new FileValidationError('\'files\' must be an object'); } const projectValues = pick(object, ['user', 'name', 'slug']); diff --git a/server/models/__mocks__/project.js b/server/models/__mocks__/project.js index 7260fcfd68..4d9d3d3748 100644 --- a/server/models/__mocks__/project.js +++ b/server/models/__mocks__/project.js @@ -11,6 +11,19 @@ export function createMock() { return sinon.mock(Project); } +// Wraps the Project.prototype i.e. the +// instance methods in a mock so +// Project.save() can be mocked +export function createInstanceMock() { + // See: https://stackoverflow.com/questions/40962960/sinon-mock-of-mongoose-save-method-for-all-future-instances-of-a-model-with-pro + Object.defineProperty(Project.prototype, 'save', { + value: Project.prototype.save, + configurable: true, + }); + + return sinon.mock(Project.prototype); +} + // Re-export the model, it will be // altered by mockingoose whenever // we call methods on the MockConfig From 728c427b4cdacde8cb0d0ebeb2341de9a42aa688 Mon Sep 17 00:00:00 2001 From: Andrew Nicolaou Date: Wed, 19 Jun 2019 15:22:04 +0200 Subject: [PATCH 10/49] Tests toModel() --- .../domain-objects/__test__/Project.test.js | 34 ++++++++++++++++++- 1 file changed, 33 insertions(+), 1 deletion(-) diff --git a/server/domain-objects/__test__/Project.test.js b/server/domain-objects/__test__/Project.test.js index f9de8b33e2..d548f9c9f6 100644 --- a/server/domain-objects/__test__/Project.test.js +++ b/server/domain-objects/__test__/Project.test.js @@ -1,4 +1,4 @@ -import { transformFiles, FileValidationError } from '../Project'; +import { toModel, transformFiles, FileValidationError } from '../Project'; jest.mock('../../utils/createId'); @@ -6,6 +6,38 @@ jest.mock('../../utils/createId'); // TODO: File extension validation // describe('domain-objects/Project', () => { + describe('toModel', () => { + it('filters extra properties', () => { + const params = { + name: 'My sketch', + extraThing: 'oopsie', + }; + + const model = toModel(params); + + expect(model.name).toBe('My sketch'); + expect(model.extraThing).toBeUndefined(); + }); + + it('throws FileValidationError', () => { + const params = { + files: { + 'index.html': {} // missing content or url + } + }; + + expect(() => toModel(params)).toThrowError(FileValidationError); + }); + + it('throws if files is not an object', () => { + const params = { + files: [] + }; + + expect(() => toModel(params)).toThrowError(FileValidationError); + }); + }); + describe('transformFiles', () => { beforeEach(() => { // eslint-disable-next-line global-require From ee6891d5b5b6a8514edcd8cfbe82b2191c378463 Mon Sep 17 00:00:00 2001 From: Andrew Nicolaou Date: Wed, 19 Jun 2019 15:51:24 +0200 Subject: [PATCH 11/49] Creates default files if no root-level .html file is provided --- .../__test__/project.controller.test.js | 1 - server/domain-objects/Project.js | 16 +- .../domain-objects/__test__/Project.test.js | 542 ++++++++++-------- server/domain-objects/createDefaultFiles.js | 48 ++ 4 files changed, 372 insertions(+), 235 deletions(-) create mode 100644 server/domain-objects/createDefaultFiles.js diff --git a/server/controllers/__test__/project.controller.test.js b/server/controllers/__test__/project.controller.test.js index 06538e8b77..2d17bae4cb 100644 --- a/server/controllers/__test__/project.controller.test.js +++ b/server/controllers/__test__/project.controller.test.js @@ -2,7 +2,6 @@ * @jest-environment node */ import { Response } from 'jest-express'; -import sinon from 'sinon'; import Project, { createMock, createInstanceMock } from '../../models/project'; import createProject, { apiCreateProject } from '../project.controller/createProject'; diff --git a/server/domain-objects/Project.js b/server/domain-objects/Project.js index c9e3a2964a..552021e99f 100644 --- a/server/domain-objects/Project.js +++ b/server/domain-objects/Project.js @@ -3,6 +3,7 @@ import pick from 'lodash/pick'; import Project from '../models/project'; import createId from '../utils/createId'; import createApplicationErrorClass from '../utils/createApplicationErrorClass'; +import createDefaultFiles from './createDefaultFiles'; export const FileValidationError = createApplicationErrorClass('FileValidationError'); @@ -103,6 +104,10 @@ export function transformFiles(tree = {}) { return files; } +export function containsRootHtmlFile(tree) { + return Object.keys(tree).find(name => /\.html$/.test(name)) != null; +} + /** * This converts between the public API's Project object * properties and a mongoose Project model @@ -110,10 +115,15 @@ export function transformFiles(tree = {}) { */ export function toModel(object) { let files = []; + let tree = object.files; + + if (isPlainObject(tree)) { + if (!containsRootHtmlFile(tree)) { + tree = Object.assign(createDefaultFiles(), tree); + } - if (isPlainObject(object.files)) { - files = transformFiles(object.files); - } else if (object.files != null) { + files = transformFiles(tree); + } else if (tree != null) { throw new FileValidationError('\'files\' must be an object'); } diff --git a/server/domain-objects/__test__/Project.test.js b/server/domain-objects/__test__/Project.test.js index d548f9c9f6..68a740ad64 100644 --- a/server/domain-objects/__test__/Project.test.js +++ b/server/domain-objects/__test__/Project.test.js @@ -1,4 +1,6 @@ -import { toModel, transformFiles, FileValidationError } from '../Project'; +import find from 'lodash/find'; + +import { containsRootHtmlFile, toModel, transformFiles, FileValidationError } from '../Project'; jest.mock('../../utils/createId'); @@ -6,6 +8,29 @@ jest.mock('../../utils/createId'); // TODO: File extension validation // describe('domain-objects/Project', () => { + describe('containsRootHtmlFile', () => { + it('returns true for at least one root .html', () => { + expect(containsRootHtmlFile({ 'index.html': {} })).toBe(true); + expect(containsRootHtmlFile({ 'another-one.html': {} })).toBe(true); + expect(containsRootHtmlFile({ 'one.html': {}, 'two.html': {} })).toBe(true); + expect(containsRootHtmlFile({ 'one.html': {}, 'sketch.js': {} })).toBe(true); + }); + + it('returns false anything else', () => { + expect(containsRootHtmlFile({ 'sketch.js': {} })).toBe(false); + }); + + it('ignores nested html', () => { + expect(containsRootHtmlFile({ + examples: { + files: { + 'index.html': {} + } + } + })).toBe(false); + }); + }); + describe('toModel', () => { it('filters extra properties', () => { const params = { @@ -36,269 +61,324 @@ describe('domain-objects/Project', () => { expect(() => toModel(params)).toThrowError(FileValidationError); }); - }); - describe('transformFiles', () => { - beforeEach(() => { - // eslint-disable-next-line global-require - const { resetMockCreateId } = require('../../utils/createId'); - - resetMockCreateId(); - }); + it('creates default index.html and dependent files if no root .html is provided', () => { + const params = { + files: {} + }; - it('creates an empty root with no data', () => { - const tree = {}; + const { files } = toModel(params); - expect(transformFiles(tree)).toEqual([{ - _id: '0', - fileType: 'folder', - name: 'root', - children: [] - }]); + expect(files.length).toBe(4); + expect(find(files, { name: 'index.html' })).not.toBeUndefined(); + expect(find(files, { name: 'sketch.js' })).not.toBeUndefined(); + expect(find(files, { name: 'style.css' })).not.toBeUndefined(); }); - it('converts tree-shaped files into list', () => { - const tree = { - 'index.html': { - content: 'some contents', + it('does not create default files if any root .html is provided', () => { + const params = { + files: { + 'example.html': { + content: 'Hello!' + } } }; - expect(transformFiles(tree)).toEqual([ - { - _id: '0', - fileType: 'folder', - name: 'root', - children: ['1'] - }, - { - _id: '1', - content: 'some contents', - fileType: 'file', - name: 'index.html' - } - ]); + const { files } = toModel(params); + + expect(files.length).toBe(2); + expect(find(files, { name: 'example.html' })).not.toBeUndefined(); + expect(find(files, { name: 'index.html' })).toBeUndefined(); + expect(find(files, { name: 'sketch.js' })).toBeUndefined(); + expect(find(files, { name: 'style.css' })).toBeUndefined(); }); - it('uses file url over content', () => { - const tree = { - 'script.js': { - url: 'http://example.net/something.js' + it('does not overwrite default CSS and JS of the same name if provided', () => { + const params = { + files: { + 'sketch.js': { + content: 'const sketch = true;' + }, + 'style.css': { + content: 'body { outline: 10px solid red; }' + } } }; - expect(transformFiles(tree)).toEqual([ - { - _id: '0', - fileType: 'folder', - name: 'root', - children: ['1'] - }, - { - _id: '1', - url: 'http://example.net/something.js', - fileType: 'file', - name: 'script.js' - } - ]); - }); + const { files } = toModel(params); - it('creates folders', () => { - const tree = { - 'a-folder': { - files: {} - }, - }; + expect(files.length).toBe(4); + expect(find(files, { name: 'index.html' })).not.toBeUndefined(); - expect(transformFiles(tree)).toEqual([ - { - _id: '0', - fileType: 'folder', - name: 'root', - children: ['1'] - }, - { - _id: '1', - children: [], - fileType: 'folder', - name: 'a-folder' - } - ]); + const sketchFile = find(files, { name: 'sketch.js' }); + expect(sketchFile.content).toBe('const sketch = true;'); + + const cssFile = find(files, { name: 'style.css' }); + expect(cssFile.content).toBe('body { outline: 10px solid red; }'); }); + }); +}); - it('walks the tree processing files', () => { - const tree = { - 'index.html': { - content: 'some contents', - }, - 'a-folder': { - files: { - 'data.csv': { - content: 'this,is,data' - }, - 'another.txt': { - content: 'blah blah' - } - } - }, - }; +describe('transformFiles', () => { + beforeEach(() => { + // eslint-disable-next-line global-require + const { resetMockCreateId } = require('../../utils/createId'); + + resetMockCreateId(); + }); + + it('creates an empty root with no data', () => { + const tree = {}; + + expect(transformFiles(tree)).toEqual([{ + _id: '0', + fileType: 'folder', + name: 'root', + children: [] + }]); + }); + + it('converts tree-shaped files into list', () => { + const tree = { + 'index.html': { + content: 'some contents', + } + }; + + expect(transformFiles(tree)).toEqual([ + { + _id: '0', + fileType: 'folder', + name: 'root', + children: ['1'] + }, + { + _id: '1', + content: 'some contents', + fileType: 'file', + name: 'index.html' + } + ]); + }); + + it('uses file url over content', () => { + const tree = { + 'script.js': { + url: 'http://example.net/something.js' + } + }; + + expect(transformFiles(tree)).toEqual([ + { + _id: '0', + fileType: 'folder', + name: 'root', + children: ['1'] + }, + { + _id: '1', + url: 'http://example.net/something.js', + fileType: 'file', + name: 'script.js' + } + ]); + }); + + it('creates folders', () => { + const tree = { + 'a-folder': { + files: {} + }, + }; - expect(transformFiles(tree)).toEqual([ - { - _id: '0', - fileType: 'folder', - name: 'root', - children: ['1', '2'] - }, - { - _id: '1', - name: 'index.html', - fileType: 'file', - content: 'some contents' - }, - { - _id: '2', - name: 'a-folder', - fileType: 'folder', - children: ['3', '4'] - }, - { - _id: '3', - name: 'data.csv', - fileType: 'file', - content: 'this,is,data' - }, - { - _id: '4', - name: 'another.txt', - fileType: 'file', - content: 'blah blah' + expect(transformFiles(tree)).toEqual([ + { + _id: '0', + fileType: 'folder', + name: 'root', + children: ['1'] + }, + { + _id: '1', + children: [], + fileType: 'folder', + name: 'a-folder' + } + ]); + }); + + it('walks the tree processing files', () => { + const tree = { + 'index.html': { + content: 'some contents', + }, + 'a-folder': { + files: { + 'data.csv': { + content: 'this,is,data' + }, + 'another.txt': { + content: 'blah blah' + } } - ]); - }); + }, + }; - it('handles deep nesting', () => { - const tree = { - first: { - files: { - second: { - files: { - third: { - files: { - 'hello.js': { - content: 'world!' - } + expect(transformFiles(tree)).toEqual([ + { + _id: '0', + fileType: 'folder', + name: 'root', + children: ['1', '2'] + }, + { + _id: '1', + name: 'index.html', + fileType: 'file', + content: 'some contents' + }, + { + _id: '2', + name: 'a-folder', + fileType: 'folder', + children: ['3', '4'] + }, + { + _id: '3', + name: 'data.csv', + fileType: 'file', + content: 'this,is,data' + }, + { + _id: '4', + name: 'another.txt', + fileType: 'file', + content: 'blah blah' + } + ]); + }); + + it('handles deep nesting', () => { + const tree = { + first: { + files: { + second: { + files: { + third: { + files: { + 'hello.js': { + content: 'world!' } } } } } - }, - }; - - expect(transformFiles(tree)).toEqual([ - { - _id: '0', - fileType: 'folder', - name: 'root', - children: ['1'] - }, - { - _id: '1', - name: 'first', - fileType: 'folder', - children: ['2'] - }, - { - _id: '2', - name: 'second', - fileType: 'folder', - children: ['3'] - }, - { - _id: '3', - name: 'third', - fileType: 'folder', - children: ['4'] - }, - { - _id: '4', - name: 'hello.js', - fileType: 'file', - content: 'world!' } - ]); - }); + }, + }; + expect(transformFiles(tree)).toEqual([ + { + _id: '0', + fileType: 'folder', + name: 'root', + children: ['1'] + }, + { + _id: '1', + name: 'first', + fileType: 'folder', + children: ['2'] + }, + { + _id: '2', + name: 'second', + fileType: 'folder', + children: ['3'] + }, + { + _id: '3', + name: 'third', + fileType: 'folder', + children: ['4'] + }, + { + _id: '4', + name: 'hello.js', + fileType: 'file', + content: 'world!' + } + ]); + }); - it('allows duplicate names in different folder', () => { - const tree = { - 'index.html': { - content: 'some contents', - }, - 'data': { - files: { - 'index.html': { - content: 'different file' - } - } - }, - }; - expect(transformFiles(tree)).toEqual([ - { - _id: '0', - fileType: 'folder', - name: 'root', - children: ['1', '2'] - }, - { - _id: '1', - name: 'index.html', - fileType: 'file', - content: 'some contents' - }, - { - _id: '2', - name: 'data', - fileType: 'folder', - children: ['3'] - }, - { - _id: '3', - name: 'index.html', - fileType: 'file', - content: 'different file' + it('allows duplicate names in different folder', () => { + const tree = { + 'index.html': { + content: 'some contents', + }, + 'data': { + files: { + 'index.html': { + content: 'different file' + } } - ]); - }); + }, + }; - it('validates files', () => { - const tree = { - 'index.html': {} // missing `content` - }; + expect(transformFiles(tree)).toEqual([ + { + _id: '0', + fileType: 'folder', + name: 'root', + children: ['1', '2'] + }, + { + _id: '1', + name: 'index.html', + fileType: 'file', + content: 'some contents' + }, + { + _id: '2', + name: 'data', + fileType: 'folder', + children: ['3'] + }, + { + _id: '3', + name: 'index.html', + fileType: 'file', + content: 'different file' + } + ]); + }); - expect(() => transformFiles(tree)).toThrowError(FileValidationError); - }); + it('validates files', () => { + const tree = { + 'index.html': {} // missing `content` + }; - it('collects all file validation errors', () => { - const tree = { - 'index.html': {}, // missing `content` - 'something.js': {} // also missing `content` - }; + expect(() => transformFiles(tree)).toThrowError(FileValidationError); + }); - try { - transformFiles(tree); - - // Should not get here - throw new Error('should have thrown before this point'); - } catch (err) { - expect(err).toBeInstanceOf(FileValidationError); - expect(err.files).toEqual([ - { name: 'index.html', message: 'missing \'url\' or \'content\'' }, - { name: 'something.js', message: 'missing \'url\' or \'content\'' } - ]); - } - }); + it('collects all file validation errors', () => { + const tree = { + 'index.html': {}, // missing `content` + 'something.js': {} // also missing `content` + }; + + try { + transformFiles(tree); + + // Should not get here + throw new Error('should have thrown before this point'); + } catch (err) { + expect(err).toBeInstanceOf(FileValidationError); + expect(err.files).toEqual([ + { name: 'index.html', message: 'missing \'url\' or \'content\'' }, + { name: 'something.js', message: 'missing \'url\' or \'content\'' } + ]); + } }); }); diff --git a/server/domain-objects/createDefaultFiles.js b/server/domain-objects/createDefaultFiles.js new file mode 100644 index 0000000000..9164c12af3 --- /dev/null +++ b/server/domain-objects/createDefaultFiles.js @@ -0,0 +1,48 @@ +const defaultSketch = `function setup() { + createCanvas(400, 400); +} + +function draw() { + background(220); +}`; + +const defaultHTML = + ` + + + + + + + + + + + + + +`; + +const defaultCSS = + `html, body { + margin: 0; + padding: 0; +} +canvas { + display: block; +} +`; + +export default function createDefaultFiles() { + return { + 'index.html': { + content: defaultHTML + }, + 'style.css': { + content: defaultCSS + }, + 'sketch.js': { + content: defaultSketch + } + }; +} From c2064e949fab2874a82f2a4f0bc9c48c06a87d7a Mon Sep 17 00:00:00 2001 From: Andrew Nicolaou Date: Wed, 19 Jun 2019 18:10:56 +0200 Subject: [PATCH 12/49] Do not auto-generate a slug if it is provided Fixes a bug where the slug was auto-generated using the sketch name, even if a slug property had been provided. --- server/models/project.js | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/server/models/project.js b/server/models/project.js index 8642bc90cc..d21d2390e9 100644 --- a/server/models/project.js +++ b/server/models/project.js @@ -49,7 +49,11 @@ projectSchema.set('toJSON', { projectSchema.pre('save', function generateSlug(next) { const project = this; - project.slug = slugify(project.name, '_'); + + if (!project.slug) { + project.slug = slugify(project.name, '_'); + } + return next(); }); From 0f393ec8d569f41a7b13a5b23c8a6ce2c971b364 Mon Sep 17 00:00:00 2001 From: Andrew Nicolaou Date: Wed, 19 Jun 2019 18:49:56 +0200 Subject: [PATCH 13/49] Validates uniqueness of slugs for projects created by the public API --- .../project.controller/createProject.js | 46 +++++++++++++------ server/domain-objects/Project.js | 1 + server/models/project.js | 31 +++++++++++++ 3 files changed, 63 insertions(+), 15 deletions(-) diff --git a/server/controllers/project.controller/createProject.js b/server/controllers/project.controller/createProject.js index 56ae8048ca..53534cad1d 100644 --- a/server/controllers/project.controller/createProject.js +++ b/server/controllers/project.controller/createProject.js @@ -1,5 +1,5 @@ import Project from '../../models/project'; -import { toModel, FileValidationError } from '../../domain-objects/Project'; +import { toModel, FileValidationError, ProjectValidationError } from '../../domain-objects/Project'; export default function createProject(req, res) { let projectValues = { @@ -36,35 +36,51 @@ export default function createProject(req, res) { export function apiCreateProject(req, res) { const params = Object.assign({ user: req.user._id }, req.body); - function sendValidationErrors(err) { + function sendValidationErrors(err, type) { res.status(422).json({ - name: 'File Validation Failed', + name: `${type} Validation Failed`, message: err.message, errors: err.files, }); } // TODO: Error handling to match spec - function sendFailure() { + function sendFailure(err) { res.status(500).json({ success: false }); } - try { - const model = toModel(params); - - return model - .save() - .then((newProject) => { - res.status(201).json({ id: newProject.id }); - }) - .catch(sendFailure); - } catch (err) { + function handleErrors(err) { if (err instanceof FileValidationError) { - sendValidationErrors(err); + sendValidationErrors(err, 'File'); + } else if (err instanceof ProjectValidationError) { + sendValidationErrors(err, 'Sketch'); } else { sendFailure(); } return Promise.reject(); } + + try { + const model = toModel(params); + + return model.isSlugUnique() + .then(({ isUnique, conflictingIds }) => { + if (isUnique) { + return model.save() + .then((newProject) => { + res.status(201).json({ id: newProject.id }); + }); + } + + const error = new ProjectValidationError(`Slug "${model.slug}" is not unique. Check ${conflictingIds.join(', ')}`); + + throw error; + }) + .catch(handleErrors); + } catch (err) { + handleErrors(err); + + return Promise.reject(err); + } } diff --git a/server/domain-objects/Project.js b/server/domain-objects/Project.js index 552021e99f..86727024d7 100644 --- a/server/domain-objects/Project.js +++ b/server/domain-objects/Project.js @@ -6,6 +6,7 @@ import createApplicationErrorClass from '../utils/createApplicationErrorClass'; import createDefaultFiles from './createDefaultFiles'; export const FileValidationError = createApplicationErrorClass('FileValidationError'); +export const ProjectValidationError = createApplicationErrorClass('ProjectValidationError'); // objectID().toHexString(); diff --git a/server/models/project.js b/server/models/project.js index d21d2390e9..bf8c992eb9 100644 --- a/server/models/project.js +++ b/server/models/project.js @@ -57,4 +57,35 @@ projectSchema.pre('save', function generateSlug(next) { return next(); }); +/** + * Check if slug is unique for this user's projects + */ +projectSchema.methods.isSlugUnique = async function isSlugUnique(cb) { + const project = this; + const hasCallback = typeof cb === 'function'; + + try { + const docsWithSlug = await project.model('Project') + .find({ user: project.user, slug: project.slug }, '_id') + .exec(); + + const result = { + isUnique: docsWithSlug.length === 0, + conflictingIds: docsWithSlug.map(d => d._id) || [] + }; + + if (hasCallback) { + cb(null, result); + } + + return result; + } catch (err) { + if (hasCallback) { + cb(err, null); + } + + throw err; + } +}; + export default mongoose.model('Project', projectSchema); From 5ada62599ec3c976623e3e7f2f782c18a11a782d Mon Sep 17 00:00:00 2001 From: Andrew Nicolaou Date: Thu, 27 Jun 2019 17:48:33 +0200 Subject: [PATCH 14/49] Adds tests for slug uniqueness --- .../__test__/project.controller.test.js | 47 +++++++++++++++++++ .../project.controller/createProject.js | 12 ++--- 2 files changed, 52 insertions(+), 7 deletions(-) diff --git a/server/controllers/__test__/project.controller.test.js b/server/controllers/__test__/project.controller.test.js index 2d17bae4cb..ab14ebeb43 100644 --- a/server/controllers/__test__/project.controller.test.js +++ b/server/controllers/__test__/project.controller.test.js @@ -185,6 +185,9 @@ describe('project.controller', () => { files: [] }; + ProjectInstanceMock.expects('isSlugUnique') + .resolves({ isUnique: true, conflictingIds: [] }); + ProjectInstanceMock.expects('save') .resolves(new Project(result)); @@ -206,6 +209,50 @@ describe('project.controller', () => { promise.then(expectations, expectations).catch(expectations); }); + it('fails if slug is not unique', (done) => { + const request = { + user: { _id: 'abc123' }, + body: { + name: 'My sketch', + slug: 'a-slug', + files: {} + } + }; + const response = new Response(); + + const result = { + _id: 'abc123', + id: 'abc123', + name: 'Project name', + // slug: 'a-slug', + serveSecure: false, + files: [] + }; + + ProjectInstanceMock.expects('isSlugUnique') + .resolves({ isUnique: false, conflictingIds: ['cde123'] }); + + ProjectInstanceMock.expects('save') + .resolves(new Project(result)); + + const promise = apiCreateProject(request, response); + + function expectations() { + const doc = response.json.mock.calls[0][0]; + + expect(response.status).toHaveBeenCalledWith(409); + expect(response.json).toHaveBeenCalled(); + + expect(JSON.parse(JSON.stringify(doc))).toMatchObject({ + message: 'Slug "a-slug" is not unique. Check cde123' + }); + + done(); + } + + promise.then(expectations, expectations).catch(expectations); + }); + it('returns validation errors on files input', (done) => { const request = { user: {}, diff --git a/server/controllers/project.controller/createProject.js b/server/controllers/project.controller/createProject.js index 53534cad1d..df1229f225 100644 --- a/server/controllers/project.controller/createProject.js +++ b/server/controllers/project.controller/createProject.js @@ -36,8 +36,8 @@ export default function createProject(req, res) { export function apiCreateProject(req, res) { const params = Object.assign({ user: req.user._id }, req.body); - function sendValidationErrors(err, type) { - res.status(422).json({ + function sendValidationErrors(err, type, code = 422) { + res.status(code).json({ name: `${type} Validation Failed`, message: err.message, errors: err.files, @@ -51,14 +51,12 @@ export function apiCreateProject(req, res) { function handleErrors(err) { if (err instanceof FileValidationError) { - sendValidationErrors(err, 'File'); + sendValidationErrors(err, 'File', err.code); } else if (err instanceof ProjectValidationError) { - sendValidationErrors(err, 'Sketch'); + sendValidationErrors(err, 'Sketch', err.code); } else { sendFailure(); } - - return Promise.reject(); } try { @@ -74,13 +72,13 @@ export function apiCreateProject(req, res) { } const error = new ProjectValidationError(`Slug "${model.slug}" is not unique. Check ${conflictingIds.join(', ')}`); + error.code = 409; throw error; }) .catch(handleErrors); } catch (err) { handleErrors(err); - return Promise.reject(err); } } From 1549fc85ec5a9ee47834f43b4029667a8ba27edc Mon Sep 17 00:00:00 2001 From: Andrew Nicolaou Date: Thu, 27 Jun 2019 17:48:50 +0200 Subject: [PATCH 15/49] Configures node's Promise implementation for mongoose (fixes warnings) --- jest.setup.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/jest.setup.js b/jest.setup.js index 7c9d2b77a9..da03678506 100644 --- a/jest.setup.js +++ b/jest.setup.js @@ -1,5 +1,8 @@ import { configure } from 'enzyme' import Adapter from 'enzyme-adapter-react-16' import '@babel/polyfill' +import mongoose from 'mongoose' + +mongoose.Promise = global.Promise; configure({ adapter: new Adapter() }) From acdac379c45ba62a2979fcc829b2d4d5a129df90 Mon Sep 17 00:00:00 2001 From: Andrew Nicolaou Date: Thu, 27 Jun 2019 17:51:22 +0200 Subject: [PATCH 16/49] Moves createProject tests to match controller location --- .../__test__/createProject.test.js} | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) rename server/controllers/{__test__/project.controller.test.js => project.controller/__test__/createProject.test.js} (97%) diff --git a/server/controllers/__test__/project.controller.test.js b/server/controllers/project.controller/__test__/createProject.test.js similarity index 97% rename from server/controllers/__test__/project.controller.test.js rename to server/controllers/project.controller/__test__/createProject.test.js index ab14ebeb43..7e180b151b 100644 --- a/server/controllers/__test__/project.controller.test.js +++ b/server/controllers/project.controller/__test__/createProject.test.js @@ -3,10 +3,10 @@ */ import { Response } from 'jest-express'; -import Project, { createMock, createInstanceMock } from '../../models/project'; -import createProject, { apiCreateProject } from '../project.controller/createProject'; +import Project, { createMock, createInstanceMock } from '../../../models/project'; +import createProject, { apiCreateProject } from '../createProject'; -jest.mock('../../models/project'); +jest.mock('../../../models/project'); describe('project.controller', () => { describe('createProject()', () => { From db717b784613f29658e9cd4d00d085e8d118753a Mon Sep 17 00:00:00 2001 From: Andrew Nicolaou Date: Thu, 27 Jun 2019 19:04:22 +0200 Subject: [PATCH 17/49] Adds support for code to ApplicationErrors --- server/utils/createApplicationErrorClass.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/server/utils/createApplicationErrorClass.js b/server/utils/createApplicationErrorClass.js index f5499d85d1..f949866c38 100644 --- a/server/utils/createApplicationErrorClass.js +++ b/server/utils/createApplicationErrorClass.js @@ -3,13 +3,14 @@ * the application. */ export class ApplicationError extends Error { - constructor(message) { + constructor(message, extra = {}) { super(); if (Error.captureStackTrace) { Error.captureStackTrace(this, this.constructor); } this.name = 'ApplicationError'; this.message = message; + this.code = extra.code; } } From c75cdad1860003ad9545d67262f25ccdfb88a2ef Mon Sep 17 00:00:00 2001 From: Andrew Nicolaou Date: Thu, 27 Jun 2019 18:22:04 +0200 Subject: [PATCH 18/49] deleteProject controller tests --- .../controllers/__mocks__/aws.controller.js | 5 + server/controllers/project.controller.js | 33 +---- .../__test__/deleteProject.test.js | 115 ++++++++++++++++++ .../project.controller/deleteProject.js | 57 +++++++++ server/models/__mocks__/project.js | 5 + 5 files changed, 183 insertions(+), 32 deletions(-) create mode 100644 server/controllers/__mocks__/aws.controller.js create mode 100644 server/controllers/project.controller/__test__/deleteProject.test.js create mode 100644 server/controllers/project.controller/deleteProject.js diff --git a/server/controllers/__mocks__/aws.controller.js b/server/controllers/__mocks__/aws.controller.js new file mode 100644 index 0000000000..9e5709c403 --- /dev/null +++ b/server/controllers/__mocks__/aws.controller.js @@ -0,0 +1,5 @@ +export const getObjectKey = jest.mock(); +export const deleteObjectsFromS3 = jest.fn(); +export const signS3 = jest.fn(); +export const copyObjectInS3 = jest.fn(); +export const listObjectsInS3ForUser = jest.fn(); diff --git a/server/controllers/project.controller.js b/server/controllers/project.controller.js index e8a7584dd0..f294f6326c 100644 --- a/server/controllers/project.controller.js +++ b/server/controllers/project.controller.js @@ -2,7 +2,6 @@ import archiver from 'archiver'; import format from 'date-fns/format'; import isUrl from 'is-url'; import jsdom, { serializeDocument } from 'jsdom'; -import isBefore from 'date-fns/is_before'; import isAfter from 'date-fns/is_after'; import request from 'request'; import slugify from 'slugify'; @@ -10,11 +9,11 @@ import Project from '../models/project'; import User from '../models/user'; import { resolvePathToFile } from '../utils/filePath'; import generateFileSystemSafeName from '../utils/generateFileSystemSafeName'; -import { deleteObjectsFromS3, getObjectKey } from './aws.controller'; import { toApi as toApiProjectObject } from '../domain-objects/Project'; import createApplicationErrorClass from '../utils/createApplicationErrorClass'; export { default as createProject, apiCreateProject } from './project.controller/createProject'; +export { default as deleteProject } from './project.controller/deleteProject'; const UserNotFoundError = createApplicationErrorClass('UserNotFoundError'); @@ -88,36 +87,7 @@ export function getProject(req, res) { }); } -function deleteFilesFromS3(files) { - deleteObjectsFromS3(files.filter((file) => { - if (file.url) { - if (!process.env.S3_DATE || ( - process.env.S3_DATE && - isBefore(new Date(process.env.S3_DATE), new Date(file.createdAt)))) { - return true; - } - } - return false; - }) - .map(file => getObjectKey(file.url))); -} -export function deleteProject(req, res) { - Project.findById(req.params.project_id, (findProjectErr, project) => { - if (!project.user.equals(req.user._id)) { - res.status(403).json({ success: false, message: 'Authenticated user does not match owner of project.' }); - return; - } - deleteFilesFromS3(project.files); - project.remove((removeProjectError) => { - if (removeProjectError) { - res.status(404).send({ message: 'Project with that id does not exist' }); - return; - } - res.json({ success: true }); - }); - }); -} export function getProjectsForUserId(userId) { return new Promise((resolve, reject) => { @@ -239,7 +209,6 @@ export function apiGetProjectsForUser(req, res) { } } - export function projectExists(projectId, callback) { Project.findById(projectId, (err, project) => ( project ? callback(true) : callback(false) diff --git a/server/controllers/project.controller/__test__/deleteProject.test.js b/server/controllers/project.controller/__test__/deleteProject.test.js new file mode 100644 index 0000000000..18b93c68f4 --- /dev/null +++ b/server/controllers/project.controller/__test__/deleteProject.test.js @@ -0,0 +1,115 @@ +/** + * @jest-environment node + */ +import { Request, Response } from 'jest-express'; + +import Project, { createMock, createInstanceMock } from '../../../models/project'; +import User from '../../../models/user'; +import deleteProject from '../../project.controller/deleteProject'; +import { deleteObjectsFromS3 } from '../../aws.controller'; + + +jest.mock('../../../models/project'); +jest.mock('../../aws.controller'); + +describe('project.controller', () => { + describe('deleteProject()', () => { + let ProjectMock; + let ProjectInstanceMock; + + beforeEach(() => { + ProjectMock = createMock(); + ProjectInstanceMock = createInstanceMock(); + }); + + afterEach(() => { + ProjectMock.restore(); + ProjectInstanceMock.restore(); + }); + + it('returns 403 if project is not owned by authenticated user', (done) => { + const user = new User(); + const project = new Project(); + project.user = user; + + const request = new Request(); + request.setParams({ project_id: project._id }); + request.user = { _id: 'abc123' }; + + const response = new Response(); + + ProjectMock + .expects('findById') + .resolves(project); + + const promise = deleteProject(request, response); + + function expectations() { + expect(response.status).toHaveBeenCalledWith(403); + expect(response.json).toHaveBeenCalledWith({ success: false, message: 'Authenticated user does not match owner of project' }); + + done(); + } + + promise.then(expectations, expectations).catch(expectations); + }); + + it('returns 404 if project does not exist', (done) => { + const user = new User(); + const project = new Project(); + project.user = user; + + const request = new Request(); + request.setParams({ project_id: project._id }); + request.user = { _id: 'abc123' }; + + const response = new Response(); + + ProjectMock + .expects('findById') + .resolves(null); + + const promise = deleteProject(request, response); + + function expectations() { + expect(response.status).toHaveBeenCalledWith(404); + expect(response.json).toHaveBeenCalledWith({ success: false, message: 'Project with that id does not exist' }); + + done(); + } + + promise.then(expectations, expectations).catch(expectations); + }); + + it('deletes project and dependent files from S3 ', (done) => { + const user = new User(); + const project = new Project(); + project.user = user; + + const request = new Request(); + request.setParams({ project_id: project._id }); + request.user = { _id: user._id }; + + const response = new Response(); + + ProjectMock + .expects('findById') + .resolves(project); + + ProjectInstanceMock.expects('remove') + .yields(); + + const promise = deleteProject(request, response); + + function expectations() { + expect(response.status).toHaveBeenCalledWith(200); + expect(response.json).toHaveBeenCalledWith({ success: true }); + expect(deleteObjectsFromS3).toHaveBeenCalled(); + + done(); + } + + promise.then(expectations, expectations).catch(expectations); + }); + }); +}); diff --git a/server/controllers/project.controller/deleteProject.js b/server/controllers/project.controller/deleteProject.js new file mode 100644 index 0000000000..5a70f543ee --- /dev/null +++ b/server/controllers/project.controller/deleteProject.js @@ -0,0 +1,57 @@ +import isBefore from 'date-fns/is_before'; +import Project from '../../models/project'; +import { deleteObjectsFromS3, getObjectKey } from '../aws.controller'; +import createApplicationErrorClass from '../../utils/createApplicationErrorClass'; + +const ProjectDeletionError = createApplicationErrorClass('ProjectDeletionError'); + +function deleteFilesFromS3(files) { + deleteObjectsFromS3(files.filter((file) => { + if (file.url) { + if (!process.env.S3_DATE || ( + process.env.S3_DATE && + isBefore(new Date(process.env.S3_DATE), new Date(file.createdAt)))) { + return true; + } + } + return false; + }) + .map(file => getObjectKey(file.url))); +} + +export default function deleteProject(req, res) { + function sendFailure(error) { + res.status(error.code).json({ success: false, message: error.message }); + } + + function sendProjectNotFound() { + sendFailure(new ProjectDeletionError('Project with that id does not exist', { code: 404 })); + } + + function handleProjectDeletion(project) { + if (project == null) { + sendProjectNotFound(); + return; + } + + if (!project.user.equals(req.user._id)) { + sendFailure(new ProjectDeletionError('Authenticated user does not match owner of project', { code: 403 })); + return; + } + + deleteFilesFromS3(project.files); + + project.remove((removeProjectError) => { + if (removeProjectError) { + sendProjectNotFound(); + return; + } + + res.status(200).json({ success: true }); + }); + } + + return Project.findById(req.params.project_id) + .then(handleProjectDeletion) + .catch(sendFailure); +} diff --git a/server/models/__mocks__/project.js b/server/models/__mocks__/project.js index 4d9d3d3748..2590046f3c 100644 --- a/server/models/__mocks__/project.js +++ b/server/models/__mocks__/project.js @@ -21,6 +21,11 @@ export function createInstanceMock() { configurable: true, }); + Object.defineProperty(Project.prototype, 'remove', { + value: Project.prototype.remove, + configurable: true, + }); + return sinon.mock(Project.prototype); } From 64e39aaf83ff05fb8dbe5bfcc0028930efd68657 Mon Sep 17 00:00:00 2001 From: Andrew Nicolaou Date: Mon, 1 Jul 2019 19:12:41 +0200 Subject: [PATCH 19/49] getProjectsForUser controller tests - implements tests - update apiKey tests to use new User mocks --- server/controllers/project.controller.js | 73 +------ .../__test__/getProjectsForUser.test.js | 151 +++++++++++++++ .../project.controller/getProjectsForUser.js | 72 +++++++ .../user.controller/__tests__/apiKey.test.js | 178 +++++++++--------- server/controllers/user.controller/apiKey.js | 102 +++++----- server/models/__mocks__/user.js | 43 +++-- 6 files changed, 404 insertions(+), 215 deletions(-) create mode 100644 server/controllers/project.controller/__test__/getProjectsForUser.test.js create mode 100644 server/controllers/project.controller/getProjectsForUser.js diff --git a/server/controllers/project.controller.js b/server/controllers/project.controller.js index f294f6326c..c9cb94e588 100644 --- a/server/controllers/project.controller.js +++ b/server/controllers/project.controller.js @@ -9,13 +9,10 @@ import Project from '../models/project'; import User from '../models/user'; import { resolvePathToFile } from '../utils/filePath'; import generateFileSystemSafeName from '../utils/generateFileSystemSafeName'; -import { toApi as toApiProjectObject } from '../domain-objects/Project'; -import createApplicationErrorClass from '../utils/createApplicationErrorClass'; export { default as createProject, apiCreateProject } from './project.controller/createProject'; export { default as deleteProject } from './project.controller/deleteProject'; - -const UserNotFoundError = createApplicationErrorClass('UserNotFoundError'); +export { default as getProjectsForUser, apiGetProjectsForUser } from './project.controller/getProjectsForUser'; export function updateProject(req, res) { Project.findById(req.params.project_id, (findProjectErr, project) => { @@ -87,8 +84,6 @@ export function getProject(req, res) { }); } - - export function getProjectsForUserId(userId) { return new Promise((resolve, reject) => { Project.find({ user: userId }) @@ -131,34 +126,6 @@ export function getProjectAsset(req, res) { }); } -function getProjectsForUserName(username) { - return new Promise((resolve, reject) => { - User.findOne({ username }, (err, user) => { - if (err) { - reject(err); - return; - } - - if (!user) { - reject(new UserNotFoundError()); - return; - } - - Project.find({ user: user._id }) - .sort('-createdAt') - .select('name files id createdAt updatedAt') - .exec((innerErr, projects) => { - if (innerErr) { - reject(innerErr); - return; - } - - resolve(projects); - }); - }); - }); -} - export function getProjects(req, res) { if (req.user) { getProjectsForUserId(req.user._id) @@ -171,44 +138,6 @@ export function getProjects(req, res) { } } -export function getProjectsForUser(req, res) { - if (req.params.username) { - getProjectsForUserName(req.params.username) - .then(projects => res.json(projects)) - .catch((err) => { - if (err instanceof UserNotFoundError) { - res.status(404).json({ message: 'User with that username does not exist.' }); - } else { - res.status(500).json({ message: 'Error fetching projects' }); - } - }); - } else { - // could just move this to client side - res.json([]); - } -} - -export function apiGetProjectsForUser(req, res) { - if (req.params.username) { - getProjectsForUserName(req.params.username) - .then((projects) => { - const asApiObjects = projects.map(p => toApiProjectObject(p)); - res.json({ sketches: asApiObjects }); - }) - .catch((err) => { - if (err instanceof UserNotFoundError) { - res.status(404).json({ message: 'User with that username does not exist.' }); - } else { - console.error(err); - res.status(500).json({ message: 'Error fetching projects' }); - } - }); - } else { - // could just move this to client side - res.json({ sketches: [] }); - } -} - export function projectExists(projectId, callback) { Project.findById(projectId, (err, project) => ( project ? callback(true) : callback(false) diff --git a/server/controllers/project.controller/__test__/getProjectsForUser.test.js b/server/controllers/project.controller/__test__/getProjectsForUser.test.js new file mode 100644 index 0000000000..05a0cf7166 --- /dev/null +++ b/server/controllers/project.controller/__test__/getProjectsForUser.test.js @@ -0,0 +1,151 @@ +/** + * @jest-environment node + */ +import { Request, Response } from 'jest-express'; + +import { createMock } from '../../../models/user'; +import getProjectsForUser, { apiGetProjectsForUser } from '../../project.controller/getProjectsForUser'; + +jest.mock('../../../models/user'); +jest.mock('../../aws.controller'); + +describe('project.controller', () => { + let UserMock; + + beforeEach(() => { + UserMock = createMock(); + }); + + afterEach(() => { + UserMock.restore(); + }); + + describe('getProjectsForUser()', () => { + it('returns empty array user not supplied as parameter', (done) => { + const request = new Request(); + request.setParams({}); + const response = new Response(); + + const promise = getProjectsForUser(request, response); + + function expectations() { + expect(response.status).toHaveBeenCalledWith(200); + expect(response.json).toHaveBeenCalledWith([]); + + done(); + } + + promise.then(expectations, expectations).catch(expectations); + }); + + it('returns 404 if user does not exist', (done) => { + const request = new Request(); + request.setParams({ username: 'abc123' }); + const response = new Response(); + + UserMock + .expects('findOne') + .withArgs({ username: 'abc123' }) + .yields(null, null); + + const promise = getProjectsForUser(request, response); + + function expectations() { + expect(response.status).toHaveBeenCalledWith(404); + expect(response.json).toHaveBeenCalledWith({ message: 'User with that username does not exist.' }); + + done(); + } + + promise.then(expectations, expectations).catch(expectations); + }); + + it('returns 500 on other errors', (done) => { + const request = new Request(); + request.setParams({ username: 'abc123' }); + const response = new Response(); + + UserMock + .expects('findOne') + .withArgs({ username: 'abc123' }) + .yields(new Error(), null); + + const promise = getProjectsForUser(request, response); + + function expectations() { + expect(response.status).toHaveBeenCalledWith(500); + expect(response.json).toHaveBeenCalledWith({ message: 'Error fetching projects' }); + + done(); + } + + promise.then(expectations, expectations).catch(expectations); + }); + }); + + describe('apiGetProjectsForUser()', () => { + it('returns validation error if user id not provided', (done) => { + const request = new Request(); + request.setParams({}); + const response = new Response(); + + const promise = apiGetProjectsForUser(request, response); + + function expectations() { + expect(response.status).toHaveBeenCalledWith(422); + expect(response.json).toHaveBeenCalledWith({ + message: 'Username not provided' + }); + + done(); + } + + promise.then(expectations, expectations).catch(expectations); + }); + + + it('returns 404 if user does not exist', (done) => { + const request = new Request(); + request.setParams({ username: 'abc123' }); + const response = new Response(); + + UserMock + .expects('findOne') + .withArgs({ username: 'abc123' }) + .yields(null, null); + + const promise = apiGetProjectsForUser(request, response); + + function expectations() { + expect(response.status).toHaveBeenCalledWith(404); + expect(response.json).toHaveBeenCalledWith({ message: 'User with that username does not exist.' }); + + done(); + } + + promise.then(expectations, expectations).catch(expectations); + }); + + it('returns 500 on other errors', (done) => { + const request = new Request(); + request.setParams({ username: 'abc123' }); + const response = new Response(); + + UserMock + .expects('findOne') + .withArgs({ username: 'abc123' }) + .yields(new Error(), null); + + const promise = apiGetProjectsForUser(request, response); + + function expectations() { + expect(response.status).toHaveBeenCalledWith(500); + expect(response.json).toHaveBeenCalledWith({ message: 'Error fetching projects' }); + + done(); + } + + promise.then(expectations, expectations).catch(expectations); + }); + }); +}); diff --git a/server/controllers/project.controller/getProjectsForUser.js b/server/controllers/project.controller/getProjectsForUser.js new file mode 100644 index 0000000000..1d9a0e34ae --- /dev/null +++ b/server/controllers/project.controller/getProjectsForUser.js @@ -0,0 +1,72 @@ +import Project from '../../models/project'; +import User from '../../models/user'; +import { toApi as toApiProjectObject } from '../../domain-objects/Project'; +import createApplicationErrorClass from '../../utils/createApplicationErrorClass'; + +const UserNotFoundError = createApplicationErrorClass('UserNotFoundError'); + +function getProjectsForUserName(username) { + return new Promise((resolve, reject) => { + User.findOne({ username }, (err, user) => { + if (err) { + reject(err); + return; + } + + if (!user) { + reject(new UserNotFoundError()); + return; + } + + Project.find({ user: user._id }) + .sort('-createdAt') + .select('name files id createdAt updatedAt') + .exec((innerErr, projects) => { + if (innerErr) { + reject(innerErr); + return; + } + + resolve(projects); + }); + }); + }); +} + +export default function getProjectsForUser(req, res) { + if (req.params.username) { + return getProjectsForUserName(req.params.username) + .then(projects => res.json(projects)) + .catch((err) => { + if (err instanceof UserNotFoundError) { + res.status(404).json({ message: 'User with that username does not exist.' }); + } else { + res.status(500).json({ message: 'Error fetching projects' }); + } + }); + } + + // could just move this to client side + res.status(200).json([]); + return Promise.resolve(); +} + +export function apiGetProjectsForUser(req, res) { + if (req.params.username) { + return getProjectsForUserName(req.params.username) + .then((projects) => { + const asApiObjects = projects.map(p => toApiProjectObject(p)); + res.json({ sketches: asApiObjects }); + }) + .catch((err) => { + if (err instanceof UserNotFoundError) { + res.status(404).json({ message: 'User with that username does not exist.' }); + } else { + res.status(500).json({ message: 'Error fetching projects' }); + } + }); + } + + res.status(422).json({ message: 'Username not provided' }); + return Promise.resolve(); +} diff --git a/server/controllers/user.controller/__tests__/apiKey.test.js b/server/controllers/user.controller/__tests__/apiKey.test.js index a496373bc8..7e396288b0 100644 --- a/server/controllers/user.controller/__tests__/apiKey.test.js +++ b/server/controllers/user.controller/__tests__/apiKey.test.js @@ -1,73 +1,40 @@ /* @jest-environment node */ import last from 'lodash/last'; +import { Request, Response } from 'jest-express'; + +import User, { createMock, createInstanceMock } from '../../../models/user'; import { createApiKey, removeApiKey } from '../../user.controller/apiKey'; jest.mock('../../../models/user'); -/* - Create a mock object representing an express Response -*/ -const createResponseMock = function createResponseMock(done) { - const json = jest.fn(() => { - if (done) { done(); } - }); - - const status = jest.fn(() => ({ json })); - - return { - status, - json - }; -}; - -/* - Create a mock of the mongoose User model -*/ -const createUserMock = function createUserMock() { - const apiKeys = []; - let nextId = 0; - - apiKeys.push = ({ label, hashedKey }) => { - const id = nextId; - nextId += 1; - const publicFields = { id, label }; - const allFields = { ...publicFields, hashedKey }; - - Object.defineProperty(allFields, 'toObject', { - value: () => publicFields, - enumerable: false - }); - - return Array.prototype.push.call(apiKeys, allFields); - }; - - apiKeys.pull = ({ _id }) => { - const index = apiKeys.findIndex(({ id }) => id === _id); - return apiKeys.splice(index, 1); - }; - - return { - apiKeys, - save: jest.fn(callback => callback()) - }; -}; - -const User = require('../../../models/user').default; - describe('user.controller', () => { + let UserMock; + let UserInstanceMock; + beforeEach(() => { - User.__setFindById(null, null); + UserMock = createMock(); + UserInstanceMock = createInstanceMock(); + }); + + afterEach(() => { + UserMock.restore(); + UserInstanceMock.restore(); }); + describe('createApiKey', () => { it('returns an error if user doesn\'t exist', () => { const request = { user: { id: '1234' } }; - const response = createResponseMock(); + const response = new Response(); + + UserMock + .expects('findById') + .withArgs('1234') + .yields(null, null); createApiKey(request, response); - expect(User.findById.mock.calls[0][0]).toBe('1234'); expect(response.status).toHaveBeenCalledWith(404); expect(response.json).toHaveBeenCalledWith({ error: 'User not found' @@ -75,10 +42,13 @@ describe('user.controller', () => { }); it('returns an error if label not provided', () => { - User.__setFindById(undefined, createUserMock()); - const request = { user: { id: '1234' }, body: {} }; - const response = createResponseMock(); + const response = new Response(); + + UserMock + .expects('findById') + .withArgs('1234') + .yields(null, new User()); createApiKey(request, response); @@ -89,46 +59,59 @@ describe('user.controller', () => { }); it('returns generated API key to the user', (done) => { - let response; + const request = new Request(); + request.setBody({ label: 'my key' }); + request.user = { id: '1234' }; - const request = { - user: { id: '1234' }, - body: { label: 'my key' } - }; + const response = new Response(); + + const user = new User(); + + UserMock + .expects('findById') + .withArgs('1234') + .yields(null, user); - const user = createUserMock(); + UserInstanceMock.expects('save') + .yields(); - const checkExpecations = () => { + function expectations() { const lastKey = last(user.apiKeys); expect(lastKey.label).toBe('my key'); expect(typeof lastKey.hashedKey).toBe('string'); - expect(response.json).toHaveBeenCalledWith({ - apiKeys: [ - { id: 0, label: 'my key', token: lastKey.hashedKey } - ] + const responseData = response.json.mock.calls[0][0]; + + expect(responseData.apiKeys.length).toBe(1); + expect(responseData.apiKeys[0]).toMatchObject({ + label: 'my key', + token: lastKey.hashedKey, + lastUsedAt: undefined, + createdAt: undefined }); done(); - }; + } - response = createResponseMock(checkExpecations); + const promise = createApiKey(request, response); - User.__setFindById(undefined, user); - - createApiKey(request, response); + promise.then(expectations, expectations).catch(expectations); }); }); describe('removeApiKey', () => { it('returns an error if user doesn\'t exist', () => { const request = { user: { id: '1234' } }; - const response = createResponseMock(); + const response = new Response(); + + UserMock + .expects('findById') + .withArgs('1234') + .yields(null, null); removeApiKey(request, response); - expect(User.findById.mock.calls[0][0]).toBe('1234'); expect(response.status).toHaveBeenCalledWith(404); expect(response.json).toHaveBeenCalledWith({ error: 'User not found' @@ -140,10 +123,13 @@ describe('user.controller', () => { user: { id: '1234' }, params: { keyId: 'not-a-real-key' } }; - const response = createResponseMock(); + const response = new Response(); + const user = new User(); - const user = createUserMock(); - User.__setFindById(undefined, user); + UserMock + .expects('findById') + .withArgs('1234') + .yields(null, user); removeApiKey(request, response); @@ -153,27 +139,41 @@ describe('user.controller', () => { }); }); - it.skip('removes key if it exists', () => { + it('removes key if it exists', () => { + const user = new User(); + user.apiKeys.push({ label: 'first key' }); // id 0 + user.apiKeys.push({ label: 'second key' }); // id 1 + + const firstKeyId = user.apiKeys[0]._id.toString(); + const secondKeyId = user.apiKeys[1]._id.toString(); + const request = { user: { id: '1234' }, - params: { keyId: 0 } + params: { keyId: firstKeyId } }; - const response = createResponseMock(); + const response = new Response(); - const user = createUserMock(); + UserMock + .expects('findById') + .withArgs('1234') + .yields(null, user); - user.apiKeys.push({ label: 'first key' }); // id 0 - user.apiKeys.push({ label: 'second key' }); // id 1 - - User.__setFindById(undefined, user); + UserInstanceMock + .expects('save') + .yields(); removeApiKey(request, response); expect(response.status).toHaveBeenCalledWith(200); - expect(response.json).toHaveBeenCalledWith({ - apiKeys: [ - { id: 1, label: 'second key' } - ] + + const responseData = response.json.mock.calls[0][0]; + + expect(responseData.apiKeys.length).toBe(1); + expect(responseData.apiKeys[0]).toMatchObject({ + id: secondKeyId, + label: 'second key', + lastUsedAt: undefined, + createdAt: undefined }); }); }); diff --git a/server/controllers/user.controller/apiKey.js b/server/controllers/user.controller/apiKey.js index eed22e6fb6..5d45b123e2 100644 --- a/server/controllers/user.controller/apiKey.js +++ b/server/controllers/user.controller/apiKey.js @@ -19,67 +19,85 @@ function generateApiKey() { } export function createApiKey(req, res) { - User.findById(req.user.id, async (err, user) => { - if (!user) { - res.status(404).json({ error: 'User not found' }); - return; - } - - if (!req.body.label) { - res.status(400).json({ error: 'Expected field \'label\' was not present in request body' }); - return; + return new Promise((resolve, reject) => { + function sendFailure(code, error) { + res.status(code).json({ error }); + resolve(); } - const keyToBeHashed = await generateApiKey(); - - const addedApiKeyIndex = user.apiKeys.push({ label: req.body.label, hashedKey: keyToBeHashed }); + User.findById(req.user.id, async (err, user) => { + if (!user) { + sendFailure(404, 'User not found'); + return; + } - user.save((saveErr) => { - if (saveErr) { - res.status(500).json({ error: saveErr }); + if (!req.body.label) { + sendFailure(400, 'Expected field \'label\' was not present in request body'); return; } - const apiKeys = user.apiKeys - .map((apiKey, index) => { - const fields = apiKey.toObject(); - const shouldIncludeToken = index === addedApiKeyIndex - 1; + const keyToBeHashed = await generateApiKey(); + + const addedApiKeyIndex = user.apiKeys.push({ label: req.body.label, hashedKey: keyToBeHashed }); + + user.save((saveErr) => { + if (saveErr) { + sendFailure(500, saveErr); + return; + } - return shouldIncludeToken ? - { ...fields, token: keyToBeHashed } : - fields; - }); + const apiKeys = user.apiKeys + .map((apiKey, index) => { + const fields = apiKey.toObject(); + const shouldIncludeToken = index === addedApiKeyIndex - 1; - res.json({ apiKeys }); + return shouldIncludeToken ? + { ...fields, token: keyToBeHashed } : + fields; + }); + + res.json({ apiKeys }); + resolve(); + }); }); }); } export function removeApiKey(req, res) { - User.findById(req.user.id, (err, user) => { - if (err) { - res.status(500).json({ error: err }); - return; - } - if (!user) { - res.status(404).json({ error: 'User not found' }); - return; - } - const keyToDelete = user.apiKeys.find(key => key.id === req.params.keyId); - if (!keyToDelete) { - res.status(404).json({ error: 'Key does not exist for user' }); - return; + return new Promise((resolve, reject) => { + function sendFailure(code, error) { + res.status(code).json({ error }); + resolve(); } - user.apiKeys.pull({ _id: req.params.keyId }); + User.findById(req.user.id, (err, user) => { + if (err) { + sendFailure(500, err); + return; + } + + if (!user) { + sendFailure(404, 'User not found'); + return; + } - user.save((saveErr) => { - if (saveErr) { - res.status(500).json({ error: saveErr }); + const keyToDelete = user.apiKeys.find(key => key.id === req.params.keyId); + if (!keyToDelete) { + sendFailure(404, 'Key does not exist for user'); return; } - res.status(200).json({ apiKeys: user.apiKeys }); + user.apiKeys.pull({ _id: req.params.keyId }); + + user.save((saveErr) => { + if (saveErr) { + sendFailure(500, saveErr); + return; + } + + res.status(200).json({ apiKeys: user.apiKeys }); + resolve(); + }); }); }); } diff --git a/server/models/__mocks__/user.js b/server/models/__mocks__/user.js index 585d8b673c..fcd73f32a1 100644 --- a/server/models/__mocks__/user.js +++ b/server/models/__mocks__/user.js @@ -1,12 +1,31 @@ -let __err = null; -let __user = null; - -export default { - __setFindById(err, user) { - __err = err; - __user = user; - }, - findById: jest.fn(async (id, callback) => { - callback(__err, __user); - }) -}; +import sinon from 'sinon'; +import 'sinon-mongoose'; + +// Import the actual model to be mocked +const User = jest.requireActual('../user').default; + +// Wrap User in a sinon mock +// The returned object is used to configure +// the mocked model's behaviour +export function createMock() { + return sinon.mock(User); +} + +// Wraps the User.prototype i.e. the +// instance methods in a mock so +// User.save() can be mocked +export function createInstanceMock() { + // See: https://stackoverflow.com/questions/40962960/sinon-mock-of-mongoose-save-method-for-all-future-instances-of-a-model-with-pro + Object.defineProperty(User.prototype, 'save', { + value: User.prototype.save, + configurable: true, + }); + + return sinon.mock(User.prototype); +} + + +// Re-export the model, it will be +// altered by mockingoose whenever +// we call methods on the MockConfig +export default User; From 3041b35b7f3d2726fa9e3abaf135a9a9577e4b1f Mon Sep 17 00:00:00 2001 From: Andrew Nicolaou Date: Tue, 2 Jul 2019 10:49:14 +0200 Subject: [PATCH 20/49] Ensure error objects have consistent property names `message` is used as a high-level description of the errors `detail` is optional and has an plain language explanation of the individual errors `errors` is an array of each individual problem from `detail` in a machine-readable format --- .../project.controller/__test__/createProject.test.js | 10 ++++++---- .../project.controller/__test__/deleteProject.test.js | 10 +++++++--- server/controllers/project.controller/createProject.js | 6 +++--- server/controllers/project.controller/deleteProject.js | 4 ++-- server/domain-objects/Project.js | 2 -- 5 files changed, 18 insertions(+), 14 deletions(-) diff --git a/server/controllers/project.controller/__test__/createProject.test.js b/server/controllers/project.controller/__test__/createProject.test.js index 7e180b151b..32873a5295 100644 --- a/server/controllers/project.controller/__test__/createProject.test.js +++ b/server/controllers/project.controller/__test__/createProject.test.js @@ -244,7 +244,8 @@ describe('project.controller', () => { expect(response.json).toHaveBeenCalled(); expect(JSON.parse(JSON.stringify(doc))).toMatchObject({ - message: 'Slug "a-slug" is not unique. Check cde123' + message: 'Sketch Validation Failed', + detail: 'Slug "a-slug" is not unique. Check cde123' }); done(); @@ -275,7 +276,8 @@ describe('project.controller', () => { const responseBody = JSON.parse(JSON.stringify(doc)); expect(response.status).toHaveBeenCalledWith(422); - expect(responseBody.name).toBe('File Validation Failed'); + expect(responseBody.message).toBe('File Validation Failed'); + expect(responseBody.detail).not.toBeNull(); expect(responseBody.errors.length).toBe(1); expect(responseBody.errors).toEqual([ { name: 'index.html', message: 'missing \'url\' or \'content\'' } @@ -305,8 +307,8 @@ describe('project.controller', () => { const responseBody = JSON.parse(JSON.stringify(doc)); expect(response.status).toHaveBeenCalledWith(422); - expect(responseBody.name).toBe('File Validation Failed'); - expect(responseBody.message).toBe('\'files\' must be an object'); + expect(responseBody.message).toBe('File Validation Failed'); + expect(responseBody.detail).toBe('\'files\' must be an object'); done(); } diff --git a/server/controllers/project.controller/__test__/deleteProject.test.js b/server/controllers/project.controller/__test__/deleteProject.test.js index 18b93c68f4..84a6824fe8 100644 --- a/server/controllers/project.controller/__test__/deleteProject.test.js +++ b/server/controllers/project.controller/__test__/deleteProject.test.js @@ -46,7 +46,9 @@ describe('project.controller', () => { function expectations() { expect(response.status).toHaveBeenCalledWith(403); - expect(response.json).toHaveBeenCalledWith({ success: false, message: 'Authenticated user does not match owner of project' }); + expect(response.json).toHaveBeenCalledWith({ + message: 'Authenticated user does not match owner of project' + }); done(); } @@ -73,7 +75,9 @@ describe('project.controller', () => { function expectations() { expect(response.status).toHaveBeenCalledWith(404); - expect(response.json).toHaveBeenCalledWith({ success: false, message: 'Project with that id does not exist' }); + expect(response.json).toHaveBeenCalledWith({ + message: 'Project with that id does not exist' + }); done(); } @@ -103,7 +107,7 @@ describe('project.controller', () => { function expectations() { expect(response.status).toHaveBeenCalledWith(200); - expect(response.json).toHaveBeenCalledWith({ success: true }); + expect(response.json).not.toHaveBeenCalled(); expect(deleteObjectsFromS3).toHaveBeenCalled(); done(); diff --git a/server/controllers/project.controller/createProject.js b/server/controllers/project.controller/createProject.js index df1229f225..144285a529 100644 --- a/server/controllers/project.controller/createProject.js +++ b/server/controllers/project.controller/createProject.js @@ -38,15 +38,15 @@ export function apiCreateProject(req, res) { function sendValidationErrors(err, type, code = 422) { res.status(code).json({ - name: `${type} Validation Failed`, - message: err.message, + message: `${type} Validation Failed`, + detail: err.message, errors: err.files, }); } // TODO: Error handling to match spec function sendFailure(err) { - res.status(500).json({ success: false }); + res.status(500).end(); } function handleErrors(err) { diff --git a/server/controllers/project.controller/deleteProject.js b/server/controllers/project.controller/deleteProject.js index 5a70f543ee..dd980b231f 100644 --- a/server/controllers/project.controller/deleteProject.js +++ b/server/controllers/project.controller/deleteProject.js @@ -21,7 +21,7 @@ function deleteFilesFromS3(files) { export default function deleteProject(req, res) { function sendFailure(error) { - res.status(error.code).json({ success: false, message: error.message }); + res.status(error.code).json({ message: error.message }); } function sendProjectNotFound() { @@ -47,7 +47,7 @@ export default function deleteProject(req, res) { return; } - res.status(200).json({ success: true }); + res.status(200).end(); }); } diff --git a/server/domain-objects/Project.js b/server/domain-objects/Project.js index 86727024d7..7b1908d7d8 100644 --- a/server/domain-objects/Project.js +++ b/server/domain-objects/Project.js @@ -8,8 +8,6 @@ import createDefaultFiles from './createDefaultFiles'; export const FileValidationError = createApplicationErrorClass('FileValidationError'); export const ProjectValidationError = createApplicationErrorClass('ProjectValidationError'); - -// objectID().toHexString(); /** * This converts between a mongoose Project model * and the public API Project object properties From 613f33acb4fa782e273eabee36f00d2a630a075b Mon Sep 17 00:00:00 2001 From: Andrew Nicolaou Date: Thu, 4 Jul 2019 16:01:01 +0200 Subject: [PATCH 21/49] Assert environment variables are provided at script start --- server/scripts/examples-ml5.js | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/server/scripts/examples-ml5.js b/server/scripts/examples-ml5.js index 6285d93570..51e1fe5d5e 100644 --- a/server/scripts/examples-ml5.js +++ b/server/scripts/examples-ml5.js @@ -1,6 +1,7 @@ import fs from 'fs'; import rp from 'request-promise'; import Q from 'q'; +import { ok } from 'assert'; // TODO: Change branchName if necessary const branchName = 'release'; @@ -10,10 +11,17 @@ const clientId = process.env.GITHUB_ID; const clientSecret = process.env.GITHUB_SECRET; const editorUsername = process.env.ML5_EXAMPLES_USERNAME; const personalAccessToken = process.env.EDITOR_API_ACCESS_TOKEN; +const editorApiUrl = process.env.EDITOR_API_URL; const headers = { 'User-Agent': 'p5js-web-editor/0.0.1' }; +ok(clientId, 'GITHUB_ID is required'); +ok(clientSecret, 'GITHUB_SECRET is required'); +ok(editorUsername, 'ML5_EXAMPLES_USERNAME is required'); +ok(personalAccessToken, 'EDITOR_API_ACCESS_TOKEN is required'); +ok(editorApiUrl, 'EDITOR_API_URL is required'); + // const githubRequestOptions = { url: baseUrl, @@ -27,7 +35,7 @@ const githubRequestOptions = { }; const editorRequestOptions = { - url: `${process.env.P5_API || 'http://localhost:8000/api'}/${editorUsername}`, + url: `${editorApiUrl}/${editorUsername}`, method: 'GET', headers: { ...headers, From 57ced9cce897f4b2665f1c5ea68955642607b639 Mon Sep 17 00:00:00 2001 From: Andrew Nicolaou Date: Thu, 4 Jul 2019 16:01:11 +0200 Subject: [PATCH 22/49] Version public API --- server/server.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/server.js b/server/server.js index 0bae932228..6fbdf3ac1f 100644 --- a/server/server.js +++ b/server/server.js @@ -96,7 +96,7 @@ app.use(session({ app.use(passport.initialize()); app.use(passport.session()); -app.use('/api', requestsOfTypeJSON(), api); +app.use('/api/v1', requestsOfTypeJSON(), api); app.use('/api', requestsOfTypeJSON(), users); app.use('/api', requestsOfTypeJSON(), sessions); app.use('/api', requestsOfTypeJSON(), files); From de85fea146853f20b07e8b43e6c766fb95234f01 Mon Sep 17 00:00:00 2001 From: Andrew Nicolaou Date: Sun, 21 Jul 2019 17:56:06 +0200 Subject: [PATCH 23/49] Expect "files" property to always be provided --- .../__test__/createProject.test.js | 28 +++++++++++++++++++ server/domain-objects/Project.js | 2 +- .../domain-objects/__test__/Project.test.js | 1 + 3 files changed, 30 insertions(+), 1 deletion(-) diff --git a/server/controllers/project.controller/__test__/createProject.test.js b/server/controllers/project.controller/__test__/createProject.test.js index 32873a5295..a5bfb02019 100644 --- a/server/controllers/project.controller/__test__/createProject.test.js +++ b/server/controllers/project.controller/__test__/createProject.test.js @@ -315,5 +315,33 @@ describe('project.controller', () => { promise.then(expectations, expectations).catch(expectations); }); + + it('rejects if files object in not provided', (done) => { + const request = { + user: { _id: 'abc123' }, + body: { + name: 'Wriggly worm', + // files: {} is missing + } + }; + const response = new Response(); + + const promise = apiCreateProject(request, response); + + function expectations() { + const doc = response.json.mock.calls[0][0]; + + const responseBody = JSON.parse(JSON.stringify(doc)); + + expect(response.status).toHaveBeenCalledWith(422); + expect(responseBody.message).toBe('File Validation Failed'); + expect(responseBody.detail).toBe('\'files\' must be an object'); + + done(); + } + + promise.then(expectations, expectations).catch(expectations); + }); + }); }); diff --git a/server/domain-objects/Project.js b/server/domain-objects/Project.js index 7b1908d7d8..f0de023f2a 100644 --- a/server/domain-objects/Project.js +++ b/server/domain-objects/Project.js @@ -122,7 +122,7 @@ export function toModel(object) { } files = transformFiles(tree); - } else if (tree != null) { + } else { throw new FileValidationError('\'files\' must be an object'); } diff --git a/server/domain-objects/__test__/Project.test.js b/server/domain-objects/__test__/Project.test.js index 68a740ad64..0f3c9dd979 100644 --- a/server/domain-objects/__test__/Project.test.js +++ b/server/domain-objects/__test__/Project.test.js @@ -36,6 +36,7 @@ describe('domain-objects/Project', () => { const params = { name: 'My sketch', extraThing: 'oopsie', + files: {} }; const model = toModel(params); From fdff64a7a25974b20c6e4137eab8d8d3233dc01e Mon Sep 17 00:00:00 2001 From: Andrew Nicolaou Date: Sun, 21 Jul 2019 18:35:33 +0200 Subject: [PATCH 24/49] Fixes linting error --- .../project.controller/__test__/createProject.test.js | 1 - 1 file changed, 1 deletion(-) diff --git a/server/controllers/project.controller/__test__/createProject.test.js b/server/controllers/project.controller/__test__/createProject.test.js index a5bfb02019..99d388001d 100644 --- a/server/controllers/project.controller/__test__/createProject.test.js +++ b/server/controllers/project.controller/__test__/createProject.test.js @@ -342,6 +342,5 @@ describe('project.controller', () => { promise.then(expectations, expectations).catch(expectations); }); - }); }); From 1bc2719db5fafa1e885ac9a817cd87469026617e Mon Sep 17 00:00:00 2001 From: Andrew Nicolaou Date: Tue, 11 Jun 2019 20:33:28 +0200 Subject: [PATCH 25/49] Converts import script to use public API endpoints The endpoints don't exist yet, but this is a good way to see how the implementation of the data structures differ. --- server/scripts/examples-ml5.js | 334 ++++++++++++++------------------- 1 file changed, 139 insertions(+), 195 deletions(-) diff --git a/server/scripts/examples-ml5.js b/server/scripts/examples-ml5.js index 0721fe5400..a04872ae99 100644 --- a/server/scripts/examples-ml5.js +++ b/server/scripts/examples-ml5.js @@ -1,11 +1,6 @@ import fs from 'fs'; import rp from 'request-promise'; import Q from 'q'; -import mongoose from 'mongoose'; -import objectID from 'bson-objectid'; -import shortid from 'shortid'; -import User from '../models/user'; -import Project from '../models/project'; // TODO: Change branchName if necessary const branchName = 'release'; @@ -14,11 +9,13 @@ const baseUrl = 'https://api.github.com/repos/ml5js/ml5-examples/contents'; const clientId = process.env.GITHUB_ID; const clientSecret = process.env.GITHUB_SECRET; const editorUsername = process.env.ML5_EXAMPLES_USERNAME; +const personalAccessToken = process.env.EDITOR_API_ACCESS_TOKEN; const headers = { 'User-Agent': 'p5js-web-editor/0.0.1' }; -const requestOptions = { +// +const githubRequestOptions = { url: baseUrl, qs: { client_id: clientId, @@ -29,14 +26,15 @@ const requestOptions = { json: true }; -const mongoConnectionString = process.env.MONGO_URL; -mongoose.connect(mongoConnectionString, { - useMongoClient: true -}); -mongoose.connection.on('error', () => { - console.error('MongoDB Connection Error. Please make sure that MongoDB is running.'); - process.exit(1); -}); +const editorRequestOptions = { + url: process.env.P5_API || 'http://localhost:8000/api', + method: 'GET', + headers: { + ...headers, + Authorization: `Basic ${Buffer.from(`${editorUsername}:${personalAccessToken}`).toString('base64')}` + }, + json: true +}; /** * --------------------------------------------------------- @@ -51,12 +49,52 @@ function flatten(list) { return list.reduce((a, b) => a.concat(Array.isArray(b) ? flatten(b) : b), []); } +/** + * Fetch data for a single HTML/JS file, or return + * an url to the file's CDN location + */ +async function fetchFileContent(item) { + const { name } = item; + const file = { url: item.url }; + + // if it is an html or js file + if ( + (file.url != null && name.endsWith('.html')) || + name.endsWith('.js') + ) { + const options = Object.assign({}, githubRequestOptions); + options.url = `${file.url}`; + + if ( + options.url !== undefined || + options.url !== null || + options.url !== '' + ) { + file.content = await rp(options); + // NOTE: remove the URL property if there's content + // Otherwise the p5 editor will try to pull from that url + if (file.content !== null) delete file.url; + } + + return file; + // if it is NOT an html or js file + } + + if (file.url) { + const cdnRef = `https://cdn.jsdelivr.net/gh/ml5js/ml5-examples@${branchName}${file.url.split(branchName)[1]}`; + file.url = cdnRef; + } + + return file; +} + + /** * STEP 1: Get the top level cateogories */ async function getCategories() { try { - const options = Object.assign({}, requestOptions); + const options = Object.assign({}, githubRequestOptions); options.url = `${options.url}/p5js${branchRef}`; const results = await rp(options); @@ -76,13 +114,18 @@ async function getCategoryExamples(sketchRootList) { const output = []; const sketchRootCategories = sketchRootList.map(async (categories) => { // let options = Object.assign({url: `${requestOptions.url}/${categories.path}${branchRef}`}, requestOptions) - const options = Object.assign({}, requestOptions); + const options = Object.assign({}, githubRequestOptions); options.url = `${options.url}${categories.path}${branchRef}`; // console.log(options) const sketchDirs = await rp(options); - const result = flatten(sketchDirs); - return result; + try { + const result = flatten(sketchDirs); + + return result; + } catch (err) { + return []; + } }); const sketchList = await Q.all(sketchRootCategories); @@ -107,7 +150,7 @@ async function traverseSketchTree(parentObject) { return output; } // let options = `https://api.github.com/repos/ml5js/ml5-examples/contents/${sketches.path}${branchRef}` - const options = Object.assign({}, requestOptions); + const options = Object.assign({}, githubRequestOptions); options.url = `${options.url}${parentObject.path}${branchRef}`; output.tree = await rp(options); @@ -124,7 +167,6 @@ async function traverseSketchTree(parentObject) { * @param {*} categoryExamples - all of the categories in an array */ async function traverseSketchTreeAll(categoryExamples) { - // const sketches = categoryExamples.map(async sketch => await traverseSketchTree(sketch)); const sketches = categoryExamples.map(async sketch => traverseSketchTree(sketch)); const result = await Q.all(sketches); @@ -139,37 +181,24 @@ function traverseAndFormat(parentObject) { const parent = Object.assign({}, parentObject); if (!parentObject.tree) { - const newid = objectID().toHexString(); // returns the files return { name: parent.name, - url: parent.download_url, - content: null, - id: newid, - _id: newid, - fileType: 'file' + url: parent.download_url }; } const subdir = parentObject.tree.map((item) => { - const newid = objectID().toHexString(); if (!item.tree) { // returns the files return { name: item.name, - url: item.download_url, - content: null, - id: newid, - _id: newid, - fileType: 'file' + url: item.download_url }; } const feat = { name: item.name, - id: newid, - _id: newid, - fileType: 'folder', children: traverseAndFormat(item) }; return feat; @@ -178,69 +207,27 @@ function traverseAndFormat(parentObject) { } /** - * Traverse the tree and flatten for project.files[] + * Traverse the tree and download all the content, + * transforming into an object keyed by file/directory name * @param {*} projectFileTree */ -function traverseAndFlatten(projectFileTree) { - const r = objectID().toHexString(); - - const projectRoot = { - name: 'root', - id: r, - _id: r, - children: [], - fileType: 'folder' - }; - - let currentParent; - - const output = projectFileTree.reduce( - (result, item, idx) => { - if (idx < projectFileTree.length) { - projectRoot.children.push(item.id); - } - - if (item.fileType === 'file') { - if (item.name === 'sketch.js') { - item.isSelectedFile = true; - } - result.push(item); - } - - // here's where the magic happens *twinkles* - if (item.fileType === 'folder') { - // recursively go down the tree of children - currentParent = traverseAndFlatten(item.children); - // the above will return an array of the children files - // concatenate that with the results - result = result.concat(currentParent); // eslint-disable-line no-param-reassign - // since we want to get the children ids, - // we can map the child ids to the current item - // then push that to our result array to get - // our flat files array. - item.children = item.children.map(child => child.id); - result.push(item); +async function traverseAndDownload(projectFileTree) { + return projectFileTree.reduce( + async (previousPromise, item, idx) => { + const result = await previousPromise; + + if (Array.isArray(item.children)) { + result[item.name] = { + files: await traverseAndDownload(item.children) + }; + } else { + result[item.name] = await fetchFileContent(item); } return result; }, - [projectRoot] + {} ); - - // Kind of hacky way to remove all roots other than the starting one - let counter = 0; - output.forEach((item, idx) => { - if (item.name === 'root') { - if (counter === 0) { - counter += 1; - } else { - output.splice(idx, 1); - counter += 1; - } - } - }); - - return output; } /** @@ -249,16 +236,14 @@ function traverseAndFlatten(projectFileTree) { * @param {*} sketch * @param {*} user */ -function formatSketchForStorage(sketch, user) { - const newProject = new Project({ - _id: shortid.generate(), +async function formatSketchForStorage(sketch, user) { + const newProject = { name: sketch.name, - user: user._id, - files: [] // <== add files to this array as file objects and add _id reference to children of root - }); + files: {} // <== add files to this object + }; let projectFiles = traverseAndFormat(sketch); - projectFiles = traverseAndFlatten(projectFiles); + projectFiles = await traverseAndDownload(projectFiles); newProject.files = projectFiles; return newProject; } @@ -271,68 +256,48 @@ function formatSketchForStorageAll(sketchWithItems, user) { sketchList = sketchList.map(sketch => formatSketchForStorage(sketch, user)); - return sketchList; + return Promise.all(sketchList); } /** - * Get all the content for the relevant files in project.files[] - * @param {*} projectObject + * Fetch a list of all projects from the API */ -async function fetchSketchContent(projectObject) { - const output = Object.assign({}, JSON.parse(JSON.stringify(projectObject))); - - const newFiles = output.files.map(async (item, i) => { - // if it is an html or js file - if ( - (item.fileType === 'file' && item.name.endsWith('.html')) || - item.name.endsWith('.js') - ) { - const options = Object.assign({}, requestOptions); - options.url = `${item.url}`; - - if ( - options.url !== undefined || - options.url !== null || - options.url !== '' - ) { - item.content = await rp(options); - // NOTE: remove the URL property if there's content - // Otherwise the p5 editor will try to pull from that url - if (item.content !== null) delete item.url; - } +async function getProjectsList() { + const options = Object.assign({}, editorRequestOptions); + options.url = `${options.url}/${editorUsername}/sketches`; + const results = await rp(options); - return item; - // if it is NOT an html or js file - } - - if (item.url) { - const cdnRef = `https://cdn.jsdelivr.net/gh/ml5js/ml5-examples@${branchName}${ - item.url.split(branchName)[1] - }`; - item.content = cdnRef; - item.url = cdnRef; - } + return results.sketches; +} - return item; - }); +/** + * Delete a project + */ +async function deleteProject(project) { + const options = Object.assign({}, editorRequestOptions); + options.method = 'DELETE'; + options.url = `${options.url}/sketches/${project.id}`; + const results = await rp(options); - output.files = await Q.all(newFiles); - return output; + return results; } /** - * STEP 5 - * Get all the content for the relevant files in project.files[] for all sketches - * @param {*} formattedSketchList + * Create a new project */ -async function fetchSketchContentAll(formattedSketchList) { - let output = formattedSketchList.slice(0); - - output = output.map(async item => fetchSketchContent(item)); +async function createProject(project) { + try { + const options = Object.assign({}, editorRequestOptions); + options.method = 'POST'; + options.url = `${options.url}/sketches`; + options.body = project; - output = await Q.all(output); + const results = await rp(options); - return output; + return results; + } catch (err) { + throw err; + } } /** @@ -342,43 +307,32 @@ async function fetchSketchContentAll(formattedSketchList) { * @param {*} user */ async function createProjectsInP5User(filledProjectList, user) { - const userProjects = await Project.find({ user: user._id }); - const removeProjects = userProjects.map(async project => Project.remove({ _id: project._id })); - await Q.all(removeProjects); - console.log('deleted old projects!'); - - const newProjects = filledProjectList.map(async (project) => { - const item = new Project(project); - console.log(`saving ${project.name}`); - await item.save(); - }); - await Q.all(newProjects); - console.log(`Projects saved to User: ${editorUsername}!`); -} + const existingProjects = await getProjectsList(); -/** - * STEP 0 - * CHECK if user exists, ifnot create one - * - */ -async function checkP5User() { - const user = await User.findOne({ username: editorUsername }); - - if (!user) { - const ml5user = new User({ - username: editorUsername, - email: process.env.ML5_EXAMPLES_EMAIL, - password: process.env.ML5_EXAMPLES_PASS - }); + console.log('**', existingProjects) + + try { + await Q.all(existingProjects.map(deleteProject)); + console.log('deleted old projects!'); + } catch (error) { + console.log('Problem deleting projects'); + console.log(error); + process.exit(1); + } - await ml5user.save((saveErr) => { - if (saveErr) throw saveErr; - console.log(`Created a user p5${ml5user}`); + try { + const newProjects = filledProjectList.map(async (project) => { + console.log(`saving ${project.name}`); + await createProject(project); }); + await Q.all(newProjects); + console.log(`Projects saved to User: ${editorUsername}!`); + } catch (error) { + console.log('Error saving projects'); + console.log(error); } } - /** * --------------------------------------------------------- * --------------------- main ------------------------------ @@ -394,18 +348,14 @@ async function checkP5User() { * Delete existing and save */ async function make() { - await checkP5User(); - // Get the user - const user = await User.findOne({ - username: editorUsername - }); // Get the categories and their examples const categories = await getCategories(); const categoryExamples = await getCategoryExamples(categories); + const examplesWithResourceTree = await traverseSketchTreeAll(categoryExamples); - const formattedSketchList = formatSketchForStorageAll(examplesWithResourceTree, user); - const filledProjectList = await fetchSketchContentAll(formattedSketchList); - await createProjectsInP5User(filledProjectList, user); + const formattedSketchList = await formatSketchForStorageAll(examplesWithResourceTree); + + await createProjectsInP5User(formattedSketchList); console.log('done!'); process.exit(); } @@ -418,20 +368,14 @@ async function make() { * Format the sketch files to be save to the db * Delete existing and save */ +// eslint-disable-next-line no-unused-vars async function test() { - await checkP5User(); - // Get the user - const user = await User.findOne({ - username: editorUsername - }); - // read from file while testing const examplesWithResourceTree = JSON.parse(fs.readFileSync('./ml5-examplesWithResourceTree.json')); - const formattedSketchList = formatSketchForStorageAll(examplesWithResourceTree, user); + const formattedSketchList = await formatSketchForStorageAll(examplesWithResourceTree); - const filledProjectList = await fetchSketchContentAll(formattedSketchList); - await createProjectsInP5User(filledProjectList, user); + await createProjectsInP5User(formattedSketchList); console.log('done!'); process.exit(); } From ae98c407f555a6c444acfb28ca95fd276d22f1e1 Mon Sep 17 00:00:00 2001 From: Andrew Nicolaou Date: Wed, 12 Jun 2019 09:42:36 +0200 Subject: [PATCH 26/49] Exposes public API endpoint to fetch user's sketches --- server/controllers/project.controller.js | 80 ++++++++++++++++++++---- server/domain-objects/Project.js | 15 +++++ server/routes/api.routes.js | 25 ++++++++ server/scripts/examples-ml5.js | 10 ++- server/server.js | 4 +- 5 files changed, 119 insertions(+), 15 deletions(-) create mode 100644 server/domain-objects/Project.js create mode 100644 server/routes/api.routes.js diff --git a/server/controllers/project.controller.js b/server/controllers/project.controller.js index ae02991865..f063bed18f 100644 --- a/server/controllers/project.controller.js +++ b/server/controllers/project.controller.js @@ -11,6 +11,7 @@ import User from '../models/user'; import { resolvePathToFile } from '../utils/filePath'; import generateFileSystemSafeName from '../utils/generateFileSystemSafeName'; import { deleteObjectsFromS3, getObjectKey } from './aws.controller'; +import { toApi as toApiProjectObject } from '../domain-objects/Project'; export { default as createProject } from './project.controller/createProject'; @@ -157,8 +158,44 @@ export function getProjectAsset(req, res) { }); } -export function getProjectsForUserName(username) { +class UserNotFoundError extends Error { + constructor(message, extra) { + super(); + if (Error.captureStackTrace) { + Error.captureStackTrace(this, this.constructor); + } + this.name = 'UserNotFoundError'; + this.message = message; + if (extra) this.extra = extra; + } +} + +function getProjectsForUserName(username) { + return new Promise((resolve, reject) => { + User.findOne({ username }, (err, user) => { + if (err) { + reject(err); + return; + } + + if (!user) { + reject(new UserNotFoundError()); + return; + } + + Project.find({ user: user._id }) + .sort('-createdAt') + .select('name files id createdAt updatedAt') + .exec((innerErr, projects) => { + if (innerErr) { + reject(innerErr); + return; + } + resolve(projects); + }); + }); + }); } export function getProjects(req, res) { @@ -175,22 +212,43 @@ export function getProjects(req, res) { export function getProjectsForUser(req, res) { if (req.params.username) { - User.findOne({ username: req.params.username }, (err, user) => { - if (!user) { - res.status(404).json({ message: 'User with that username does not exist.' }); - return; - } - Project.find({ user: user._id }) - .sort('-createdAt') - .select('name files id createdAt updatedAt') - .exec((innerErr, projects) => res.json(projects)); - }); + getProjectsForUserName(req.params.username) + .then(projects => res.json(projects)) + .catch((err) => { + if (err instanceof UserNotFoundError) { + res.status(404).json({ message: 'User with that username does not exist.' }); + } else { + res.status(500).json({ message: 'Error fetching projects' }); + } + }); } else { // could just move this to client side res.json([]); } } +export function apiGetProjectsForUser(req, res) { + if (req.params.username) { + getProjectsForUserName(req.params.username) + .then((projects) => { + const asApiObjects = projects.map(p => toApiProjectObject(p)); + res.json({ sketches: asApiObjects }); + }) + .catch((err) => { + if (err instanceof UserNotFoundError) { + res.status(404).json({ message: 'User with that username does not exist.' }); + } else { + console.error(err); + res.status(500).json({ message: 'Error fetching projects' }); + } + }); + } else { + // could just move this to client side + res.json({ sketches: [] }); + } +} + + export function projectExists(projectId, callback) { Project.findById(projectId, (err, project) => ( project ? callback(true) : callback(false) diff --git a/server/domain-objects/Project.js b/server/domain-objects/Project.js new file mode 100644 index 0000000000..6f2734b4ce --- /dev/null +++ b/server/domain-objects/Project.js @@ -0,0 +1,15 @@ +/** + * This converts between a mongoose Project model + * and the public API Project object properties + * + */ +export function toApi(model) { + return { + id: model.id, + name: model.name, + }; +} + +export function toModel(object) { + +} diff --git a/server/routes/api.routes.js b/server/routes/api.routes.js new file mode 100644 index 0000000000..e1c722f66d --- /dev/null +++ b/server/routes/api.routes.js @@ -0,0 +1,25 @@ +import { Router } from 'express'; +import passport from 'passport'; +import * as ProjectController from '../controllers/project.controller'; + +const router = new Router(); + +router.get( + '/:username/sketches', + passport.authenticate('basic', { session: false }), + ProjectController.apiGetProjectsForUser +); + +// router.post( +// '/:username/sketches', +// passport.authenticate('basic', { session: false }), +// ProjectController.apiCreateProject +// ); + +// router.delete( +// '/:username/sketches/:id', +// passport.authenticate('basic', { session: false }), +// ProjectController.deleteProject +// ); + +export default router; diff --git a/server/scripts/examples-ml5.js b/server/scripts/examples-ml5.js index a04872ae99..6285d93570 100644 --- a/server/scripts/examples-ml5.js +++ b/server/scripts/examples-ml5.js @@ -27,7 +27,7 @@ const githubRequestOptions = { }; const editorRequestOptions = { - url: process.env.P5_API || 'http://localhost:8000/api', + url: `${process.env.P5_API || 'http://localhost:8000/api'}/${editorUsername}`, method: 'GET', headers: { ...headers, @@ -264,7 +264,8 @@ function formatSketchForStorageAll(sketchWithItems, user) { */ async function getProjectsList() { const options = Object.assign({}, editorRequestOptions); - options.url = `${options.url}/${editorUsername}/sketches`; + options.url = `${options.url}/sketches`; + const results = await rp(options); return results.sketches; @@ -277,6 +278,7 @@ async function deleteProject(project) { const options = Object.assign({}, editorRequestOptions); options.method = 'DELETE'; options.url = `${options.url}/sketches/${project.id}`; + const results = await rp(options); return results; @@ -307,9 +309,11 @@ async function createProject(project) { * @param {*} user */ async function createProjectsInP5User(filledProjectList, user) { + console.log('Finding existing projects...'); + const existingProjects = await getProjectsList(); - console.log('**', existingProjects) + console.log(`Will delete ${existingProjects.length} projects`); try { await Q.all(existingProjects.map(deleteProject)); diff --git a/server/server.js b/server/server.js index 4483b722bb..bbbe85580d 100644 --- a/server/server.js +++ b/server/server.js @@ -16,6 +16,7 @@ import webpackHotMiddleware from 'webpack-hot-middleware'; import config from '../webpack/config.dev'; // Import all required modules +import api from './routes/api.routes'; import users from './routes/user.routes'; import sessions from './routes/session.routes'; import projects from './routes/project.routes'; @@ -51,7 +52,7 @@ const corsOriginsWhitelist = [ // Run Webpack dev server in development mode if (process.env.NODE_ENV === 'development') { const compiler = webpack(config); - app.use(webpackDevMiddleware(compiler, { noInfo: true, publicPath: config[0].output.publicPath })); + app.use(webpackDevMiddleware(compiler, { lazy: false, noInfo: true, publicPath: config[0].output.publicPath })); app.use(webpackHotMiddleware(compiler)); corsOriginsWhitelist.push(/localhost/); @@ -95,6 +96,7 @@ app.use(session({ app.use(passport.initialize()); app.use(passport.session()); +app.use('/api', requestsOfTypeJSON(), api); app.use('/api', requestsOfTypeJSON(), users); app.use('/api', requestsOfTypeJSON(), sessions); app.use('/api', requestsOfTypeJSON(), files); From 5673bf3e644699e0ab1de62b78ac048a2291285e Mon Sep 17 00:00:00 2001 From: Andrew Nicolaou Date: Wed, 12 Jun 2019 10:12:02 +0200 Subject: [PATCH 27/49] Implements public API delete endpoint --- server/controllers/project.controller.js | 4 ++-- server/routes/api.routes.js | 12 +++++++----- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/server/controllers/project.controller.js b/server/controllers/project.controller.js index f063bed18f..77add1885a 100644 --- a/server/controllers/project.controller.js +++ b/server/controllers/project.controller.js @@ -102,11 +102,11 @@ function deleteFilesFromS3(files) { export function deleteProject(req, res) { Project.findById(req.params.project_id, (findProjectErr, project) => { if (!project.user.equals(req.user._id)) { - res.status(403).json({ success: false, message: 'Session does not match owner of project.' }); + res.status(403).json({ success: false, message: 'Authenticated user does not match owner of project.' }); return; } deleteFilesFromS3(project.files); - Project.remove({ _id: req.params.project_id }, (removeProjectError) => { + project.remove((removeProjectError) => { if (removeProjectError) { res.status(404).send({ message: 'Project with that id does not exist' }); return; diff --git a/server/routes/api.routes.js b/server/routes/api.routes.js index e1c722f66d..2bf1e7a598 100644 --- a/server/routes/api.routes.js +++ b/server/routes/api.routes.js @@ -16,10 +16,12 @@ router.get( // ProjectController.apiCreateProject // ); -// router.delete( -// '/:username/sketches/:id', -// passport.authenticate('basic', { session: false }), -// ProjectController.deleteProject -// ); +// NOTE: Currently :username will not be checked for ownership +// only the project's owner in the database. +router.delete( + '/:username/sketches/:project_id', + passport.authenticate('basic', { session: false }), + ProjectController.deleteProject +); export default router; From 16427ad7ec28d66b68db111b3fc28be9c0d028ed Mon Sep 17 00:00:00 2001 From: Andrew Nicolaou Date: Mon, 17 Jun 2019 09:45:45 +0200 Subject: [PATCH 28/49] Adds helper to create custom ApplicationError classes --- server/controllers/project.controller.js | 14 ++------- server/utils/createApplicationErrorClass.js | 35 +++++++++++++++++++++ 2 files changed, 37 insertions(+), 12 deletions(-) create mode 100644 server/utils/createApplicationErrorClass.js diff --git a/server/controllers/project.controller.js b/server/controllers/project.controller.js index 77add1885a..c6cdeed03b 100644 --- a/server/controllers/project.controller.js +++ b/server/controllers/project.controller.js @@ -12,8 +12,10 @@ import { resolvePathToFile } from '../utils/filePath'; import generateFileSystemSafeName from '../utils/generateFileSystemSafeName'; import { deleteObjectsFromS3, getObjectKey } from './aws.controller'; import { toApi as toApiProjectObject } from '../domain-objects/Project'; +import createApplicationErrorClass from '../utils/createApplicationErrorClass'; export { default as createProject } from './project.controller/createProject'; +const UserNotFoundError = createApplicationErrorClass('UserNotFoundError'); export function updateProject(req, res) { Project.findById(req.params.project_id, (findProjectErr, project) => { @@ -158,18 +160,6 @@ export function getProjectAsset(req, res) { }); } -class UserNotFoundError extends Error { - constructor(message, extra) { - super(); - if (Error.captureStackTrace) { - Error.captureStackTrace(this, this.constructor); - } - this.name = 'UserNotFoundError'; - this.message = message; - if (extra) this.extra = extra; - } -} - function getProjectsForUserName(username) { return new Promise((resolve, reject) => { User.findOne({ username }, (err, user) => { diff --git a/server/utils/createApplicationErrorClass.js b/server/utils/createApplicationErrorClass.js new file mode 100644 index 0000000000..f7dbc483e2 --- /dev/null +++ b/server/utils/createApplicationErrorClass.js @@ -0,0 +1,35 @@ +/** + * This is the base class for custom errors in + * the application. + */ +export class ApplicationError extends Error { + constructor(message, extra) { + super(); + if (Error.captureStackTrace) { + Error.captureStackTrace(this, this.constructor); + } + this.name = 'ApplicationError'; + this.message = message; + if (extra) this.extra = extra; + } +} + +/** + * Create a new custom error class e.g. + * const UserNotFoundError = createApplicationErrorClass('UserNotFoundError'); + * + * // Later + * throw new UserNotFoundError(`user ${user.name} not found`); + * + */ +export default function createApplicationErrorClass(name) { + const CustomError = class extends ApplicationError { + constructor(...params) { + super(...params); + + this.name = name; + } + }; + + return CustomError; +} From 323716080c33d0debfebd06bf85074b89941de13 Mon Sep 17 00:00:00 2001 From: Andrew Nicolaou Date: Mon, 17 Jun 2019 12:15:26 +0200 Subject: [PATCH 29/49] Adds create project endpoint that understand API's data structure This transforms the nested tree of file data into a mongoose Project model --- server/controllers/project.controller.js | 3 +- .../project.controller/createProject.js | 25 ++ server/domain-objects/Project.js | 85 ++++++ .../domain-objects/__test__/Project.test.js | 244 ++++++++++++++++++ server/routes/api.routes.js | 10 +- server/utils/__mocks__/createId.js | 16 ++ server/utils/createId.js | 8 + 7 files changed, 385 insertions(+), 6 deletions(-) create mode 100644 server/domain-objects/__test__/Project.test.js create mode 100644 server/utils/__mocks__/createId.js create mode 100644 server/utils/createId.js diff --git a/server/controllers/project.controller.js b/server/controllers/project.controller.js index c6cdeed03b..046c6ebcb4 100644 --- a/server/controllers/project.controller.js +++ b/server/controllers/project.controller.js @@ -14,7 +14,8 @@ import { deleteObjectsFromS3, getObjectKey } from './aws.controller'; import { toApi as toApiProjectObject } from '../domain-objects/Project'; import createApplicationErrorClass from '../utils/createApplicationErrorClass'; -export { default as createProject } from './project.controller/createProject'; +export { default as createProject, apiCreateProject } from './project.controller/createProject'; + const UserNotFoundError = createApplicationErrorClass('UserNotFoundError'); export function updateProject(req, res) { diff --git a/server/controllers/project.controller/createProject.js b/server/controllers/project.controller/createProject.js index 2cf34e325e..77ff31bc65 100644 --- a/server/controllers/project.controller/createProject.js +++ b/server/controllers/project.controller/createProject.js @@ -1,4 +1,5 @@ import Project from '../../models/project'; +import { toModel } from '../../domain-objects/Project'; export default function createProject(req, res) { let projectValues = { @@ -30,3 +31,27 @@ export default function createProject(req, res) { .then(populateUserData) .catch(sendFailure); } + +// TODO: What happens if you don't supply any files? +export function apiCreateProject(req, res) { + const params = Object.assign({ user: req.user._id }, req.body); + + // TODO: Error handling to match spec + function sendFailure() { + res.json({ success: false }); + } + + try { + const model = toModel(params); + + model + .save() + .then((newProject) => { + res.json({ ok: true }); + }) + .catch(sendFailure); + } catch (err) { + // TODO: Catch custom err object and return correct status code + res.status(422).json({ error: 'Validation error' }); + } +} diff --git a/server/domain-objects/Project.js b/server/domain-objects/Project.js index 6f2734b4ce..5295a2f951 100644 --- a/server/domain-objects/Project.js +++ b/server/domain-objects/Project.js @@ -1,3 +1,8 @@ +import pick from 'lodash/pick'; +import Project from '../models/project'; +import createId from '../utils/createId'; + +// objectID().toHexString(); /** * This converts between a mongoose Project model * and the public API Project object properties @@ -10,6 +15,86 @@ export function toApi(model) { }; } +/** + * Transforms a tree of files matching the APIs DirectoryContents + * format into the data structure stored in mongodb + * + * - flattens the tree into an array of file/folders + * - each file/folder gets a generated BSON-ID + * - each folder has a `children` array containing the IDs of it's children + */ +function transformFilesInner(files = {}, parentNode) { + const allFiles = []; + + Object.entries(files).forEach(([name, params]) => { + const id = createId(); + const isFolder = params.files != null; + + if (isFolder) { + const folder = { + _id: id, + name, + fileType: 'folder', + children: [] // Initialise an empty folder + }; + + allFiles.push(folder); + + // The recursion will return a list of child files/folders + // It will also push the child's id into `folder.children` + allFiles.push(...transformFilesInner(params.files, folder)); + } else { + const file = { + _id: id, + name, + fileType: 'file' + }; + + if (typeof params.url === 'string') { + file.url = params.url; + } else if (typeof params.content === 'string') { + file.content = params.content; + } else { + throw new Error('url or params must be supplied'); + } + + allFiles.push(file); + } + + // Push this child's ID onto it's parent's list + // of children + if (parentNode != null) { + parentNode.children.push(id); + } + }); + + return allFiles; +} + +export function transformFiles(files = {}) { + const withRoot = { + root: { + files + } + }; + + return transformFilesInner(withRoot); +} + +/** + * This converts between the public API's Project object + * properties and a mongoose Project model + * + */ export function toModel(object) { + let files = []; + + if (typeof object.files === 'object') { + files = transformFiles(object.files); + } + + const projectValues = pick(object, ['user', 'name', 'slug']); + projectValues.files = files; + return new Project(projectValues); } diff --git a/server/domain-objects/__test__/Project.test.js b/server/domain-objects/__test__/Project.test.js new file mode 100644 index 0000000000..c5180d1ffd --- /dev/null +++ b/server/domain-objects/__test__/Project.test.js @@ -0,0 +1,244 @@ +import { transformFiles } from '../Project'; + +jest.mock('../../utils/createId'); + +// TODO: File name validation +// TODO: File extension validation +// +describe('domain-objects/Project', () => { + describe('transformFiles', () => { + beforeEach(() => { + // eslint-disable-next-line global-require + const { resetMockCreateId } = require('../../utils/createId'); + + resetMockCreateId(); + }); + + it('creates an empty root with no data', () => { + const tree = {}; + + expect(transformFiles(tree)).toEqual([{ + _id: '0', + fileType: 'folder', + name: 'root', + children: [] + }]); + }); + + it('converts tree-shaped files into list', () => { + const tree = { + 'index.html': { + content: 'some contents', + } + }; + + expect(transformFiles(tree)).toEqual([ + { + _id: '0', + fileType: 'folder', + name: 'root', + children: ['1'] + }, + { + _id: '1', + content: 'some contents', + fileType: 'file', + name: 'index.html' + } + ]); + }); + + it('uses file url over content', () => { + const tree = { + 'script.js': { + url: 'http://example.net/something.js' + } + }; + + expect(transformFiles(tree)).toEqual([ + { + _id: '0', + fileType: 'folder', + name: 'root', + children: ['1'] + }, + { + _id: '1', + url: 'http://example.net/something.js', + fileType: 'file', + name: 'script.js' + } + ]); + }); + + it('creates folders', () => { + const tree = { + 'a-folder': { + files: {} + }, + }; + + expect(transformFiles(tree)).toEqual([ + { + _id: '0', + fileType: 'folder', + name: 'root', + children: ['1'] + }, + { + _id: '1', + children: [], + fileType: 'folder', + name: 'a-folder' + } + ]); + }); + + it('walks the tree processing files', () => { + const tree = { + 'index.html': { + content: 'some contents', + }, + 'a-folder': { + files: { + 'data.csv': { + content: 'this,is,data' + }, + 'another.txt': { + content: 'blah blah' + } + } + }, + }; + + expect(transformFiles(tree)).toEqual([ + { + _id: '0', + fileType: 'folder', + name: 'root', + children: ['1', '2'] + }, + { + _id: '1', + name: 'index.html', + fileType: 'file', + content: 'some contents' + }, + { + _id: '2', + name: 'a-folder', + fileType: 'folder', + children: ['3', '4'] + }, + { + _id: '3', + name: 'data.csv', + fileType: 'file', + content: 'this,is,data' + }, + { + _id: '4', + name: 'another.txt', + fileType: 'file', + content: 'blah blah' + } + ]); + }); + + it('handles deep nesting', () => { + const tree = { + first: { + files: { + second: { + files: { + third: { + files: { + 'hello.js': { + content: 'world!' + } + } + } + } + } + } + }, + }; + + expect(transformFiles(tree)).toEqual([ + { + _id: '0', + fileType: 'folder', + name: 'root', + children: ['1'] + }, + { + _id: '1', + name: 'first', + fileType: 'folder', + children: ['2'] + }, + { + _id: '2', + name: 'second', + fileType: 'folder', + children: ['3'] + }, + { + _id: '3', + name: 'third', + fileType: 'folder', + children: ['4'] + }, + { + _id: '4', + name: 'hello.js', + fileType: 'file', + content: 'world!' + } + ]); + }); + + + it('allows duplicate names in different folder', () => { + const tree = { + 'index.html': { + content: 'some contents', + }, + 'data': { + files: { + 'index.html': { + content: 'different file' + } + } + }, + }; + + expect(transformFiles(tree)).toEqual([ + { + _id: '0', + fileType: 'folder', + name: 'root', + children: ['1', '2'] + }, + { + _id: '1', + name: 'index.html', + fileType: 'file', + content: 'some contents' + }, + { + _id: '2', + name: 'data', + fileType: 'folder', + children: ['3'] + }, + { + _id: '3', + name: 'index.html', + fileType: 'file', + content: 'different file' + } + ]); + }); + }); +}); diff --git a/server/routes/api.routes.js b/server/routes/api.routes.js index 2bf1e7a598..60be3494cf 100644 --- a/server/routes/api.routes.js +++ b/server/routes/api.routes.js @@ -10,11 +10,11 @@ router.get( ProjectController.apiGetProjectsForUser ); -// router.post( -// '/:username/sketches', -// passport.authenticate('basic', { session: false }), -// ProjectController.apiCreateProject -// ); +router.post( + '/:username/sketches', + passport.authenticate('basic', { session: false }), + ProjectController.apiCreateProject +); // NOTE: Currently :username will not be checked for ownership // only the project's owner in the database. diff --git a/server/utils/__mocks__/createId.js b/server/utils/__mocks__/createId.js new file mode 100644 index 0000000000..919ea25207 --- /dev/null +++ b/server/utils/__mocks__/createId.js @@ -0,0 +1,16 @@ +/** + * Creates an increasing numeric ID + * for testing + */ +let nextId = 0; + +export default function mockCreateId() { + const id = nextId; + nextId += 1; + + return String(id); +} + +export function resetMockCreateId() { + nextId = 0; +} diff --git a/server/utils/createId.js b/server/utils/createId.js new file mode 100644 index 0000000000..65b8290fa2 --- /dev/null +++ b/server/utils/createId.js @@ -0,0 +1,8 @@ +import objectID from 'bson-objectid'; + +/** + * Creates a mongo ID + */ +export default function createId() { + return objectID().toHexString(); +} From dc1aad767f0c1506290ac166fb5dd6c8c44b9fbf Mon Sep 17 00:00:00 2001 From: Andrew Nicolaou Date: Wed, 19 Jun 2019 12:11:55 +0200 Subject: [PATCH 30/49] Returns '201 Created' to match API spec --- server/controllers/project.controller/createProject.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/controllers/project.controller/createProject.js b/server/controllers/project.controller/createProject.js index 77ff31bc65..f6f56d6318 100644 --- a/server/controllers/project.controller/createProject.js +++ b/server/controllers/project.controller/createProject.js @@ -47,7 +47,7 @@ export function apiCreateProject(req, res) { model .save() .then((newProject) => { - res.json({ ok: true }); + res.status(201).json({ id: newProject.id }); }) .catch(sendFailure); } catch (err) { From c6a577ffa32beca6aacf48ecd9363b75fc9697aa Mon Sep 17 00:00:00 2001 From: Andrew Nicolaou Date: Wed, 19 Jun 2019 12:12:28 +0200 Subject: [PATCH 31/49] Removes 'CustomError' variable assignment as it shows up in test output --- server/utils/createApplicationErrorClass.js | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/server/utils/createApplicationErrorClass.js b/server/utils/createApplicationErrorClass.js index f7dbc483e2..f5499d85d1 100644 --- a/server/utils/createApplicationErrorClass.js +++ b/server/utils/createApplicationErrorClass.js @@ -3,14 +3,13 @@ * the application. */ export class ApplicationError extends Error { - constructor(message, extra) { + constructor(message) { super(); if (Error.captureStackTrace) { Error.captureStackTrace(this, this.constructor); } this.name = 'ApplicationError'; this.message = message; - if (extra) this.extra = extra; } } @@ -23,13 +22,11 @@ export class ApplicationError extends Error { * */ export default function createApplicationErrorClass(name) { - const CustomError = class extends ApplicationError { + return class extends ApplicationError { constructor(...params) { super(...params); this.name = name; } }; - - return CustomError; } From d1dcb9863564716eda0c59aa8a20d0cb3caf521c Mon Sep 17 00:00:00 2001 From: Andrew Nicolaou Date: Wed, 19 Jun 2019 12:12:51 +0200 Subject: [PATCH 32/49] transformFiles will return file validation errors --- server/domain-objects/Project.js | 43 ++++++++++++++----- .../domain-objects/__test__/Project.test.js | 30 ++++++++++++- 2 files changed, 61 insertions(+), 12 deletions(-) diff --git a/server/domain-objects/Project.js b/server/domain-objects/Project.js index 5295a2f951..19f2529740 100644 --- a/server/domain-objects/Project.js +++ b/server/domain-objects/Project.js @@ -1,6 +1,10 @@ import pick from 'lodash/pick'; import Project from '../models/project'; import createId from '../utils/createId'; +import createApplicationErrorClass from '../utils/createApplicationErrorClass'; + +export const FileValidationError = createApplicationErrorClass('FileValidationError'); + // objectID().toHexString(); /** @@ -23,10 +27,11 @@ export function toApi(model) { * - each file/folder gets a generated BSON-ID * - each folder has a `children` array containing the IDs of it's children */ -function transformFilesInner(files = {}, parentNode) { - const allFiles = []; +function transformFilesInner(tree = {}, parentNode) { + const files = []; + const errors = []; - Object.entries(files).forEach(([name, params]) => { + Object.entries(tree).forEach(([name, params]) => { const id = createId(); const isFolder = params.files != null; @@ -38,11 +43,13 @@ function transformFilesInner(files = {}, parentNode) { children: [] // Initialise an empty folder }; - allFiles.push(folder); + files.push(folder); // The recursion will return a list of child files/folders // It will also push the child's id into `folder.children` - allFiles.push(...transformFilesInner(params.files, folder)); + const subFolder = transformFilesInner(params.files, folder); + files.push(...subFolder.files); + errors.push(...subFolder.errors); } else { const file = { _id: id, @@ -55,10 +62,10 @@ function transformFilesInner(files = {}, parentNode) { } else if (typeof params.content === 'string') { file.content = params.content; } else { - throw new Error('url or params must be supplied'); + errors.push({ name, message: 'missing \'url\' or \'content\'' }); } - allFiles.push(file); + files.push(file); } // Push this child's ID onto it's parent's list @@ -68,17 +75,31 @@ function transformFilesInner(files = {}, parentNode) { } }); - return allFiles; + return { files, errors }; } -export function transformFiles(files = {}) { +export function transformFiles(tree = {}) { const withRoot = { root: { - files + files: tree } }; - return transformFilesInner(withRoot); + const { files, errors } = transformFilesInner(withRoot); + + if (errors.length > 0) { + const message = `${errors.length} files failed validation. See error.files for individual errors. + + Errors: + ${errors.map(e => `* ${e.name}: ${e.message}`).join('\n')} +`; + const error = new FileValidationError(message); + error.files = errors; + + throw error; + } + + return files; } /** diff --git a/server/domain-objects/__test__/Project.test.js b/server/domain-objects/__test__/Project.test.js index c5180d1ffd..f9de8b33e2 100644 --- a/server/domain-objects/__test__/Project.test.js +++ b/server/domain-objects/__test__/Project.test.js @@ -1,4 +1,4 @@ -import { transformFiles } from '../Project'; +import { transformFiles, FileValidationError } from '../Project'; jest.mock('../../utils/createId'); @@ -240,5 +240,33 @@ describe('domain-objects/Project', () => { } ]); }); + + it('validates files', () => { + const tree = { + 'index.html': {} // missing `content` + }; + + expect(() => transformFiles(tree)).toThrowError(FileValidationError); + }); + + it('collects all file validation errors', () => { + const tree = { + 'index.html': {}, // missing `content` + 'something.js': {} // also missing `content` + }; + + try { + transformFiles(tree); + + // Should not get here + throw new Error('should have thrown before this point'); + } catch (err) { + expect(err).toBeInstanceOf(FileValidationError); + expect(err.files).toEqual([ + { name: 'index.html', message: 'missing \'url\' or \'content\'' }, + { name: 'something.js', message: 'missing \'url\' or \'content\'' } + ]); + } + }); }); }); From f851d90f2225309d649cbf065c9e1066f6c5f8b6 Mon Sep 17 00:00:00 2001 From: Andrew Nicolaou Date: Wed, 19 Jun 2019 15:10:22 +0200 Subject: [PATCH 33/49] Tests API project controller --- .../__test__/project.controller.test.js | 120 +++++++++++++++++- .../project.controller/createProject.js | 23 +++- server/domain-objects/Project.js | 5 +- server/models/__mocks__/project.js | 13 ++ 4 files changed, 153 insertions(+), 8 deletions(-) diff --git a/server/controllers/__test__/project.controller.test.js b/server/controllers/__test__/project.controller.test.js index e0852e618e..06538e8b77 100644 --- a/server/controllers/__test__/project.controller.test.js +++ b/server/controllers/__test__/project.controller.test.js @@ -2,9 +2,10 @@ * @jest-environment node */ import { Response } from 'jest-express'; +import sinon from 'sinon'; -import { createMock } from '../../models/project'; -import createProject from '../project.controller/createProject'; +import Project, { createMock, createInstanceMock } from '../../models/project'; +import createProject, { apiCreateProject } from '../project.controller/createProject'; jest.mock('../../models/project'); @@ -152,4 +153,119 @@ describe('project.controller', () => { promise.then(expectations, expectations).catch(expectations); }); }); + + describe('apiCreateProject()', () => { + let ProjectMock; + let ProjectInstanceMock; + + beforeEach(() => { + ProjectMock = createMock(); + ProjectInstanceMock = createInstanceMock(); + }); + + afterEach(() => { + ProjectMock.restore(); + ProjectInstanceMock.restore(); + }); + + it('returns 201 with id of created sketch', (done) => { + const request = { + user: { _id: 'abc123' }, + body: { + name: 'My sketch', + files: {} + } + }; + const response = new Response(); + + const result = { + _id: 'abc123', + id: 'abc123', + name: 'Project name', + serveSecure: false, + files: [] + }; + + ProjectInstanceMock.expects('save') + .resolves(new Project(result)); + + const promise = apiCreateProject(request, response); + + function expectations() { + const doc = response.json.mock.calls[0][0]; + + expect(response.status).toHaveBeenCalledWith(201); + expect(response.json).toHaveBeenCalled(); + + expect(JSON.parse(JSON.stringify(doc))).toMatchObject({ + id: 'abc123' + }); + + done(); + } + + promise.then(expectations, expectations).catch(expectations); + }); + + it('returns validation errors on files input', (done) => { + const request = { + user: {}, + body: { + name: 'My sketch', + files: { + 'index.html': { + // missing content or url + } + } + } + }; + const response = new Response(); + + const promise = apiCreateProject(request, response); + + function expectations() { + const doc = response.json.mock.calls[0][0]; + + const responseBody = JSON.parse(JSON.stringify(doc)); + + expect(response.status).toHaveBeenCalledWith(422); + expect(responseBody.name).toBe('File Validation Failed'); + expect(responseBody.errors.length).toBe(1); + expect(responseBody.errors).toEqual([ + { name: 'index.html', message: 'missing \'url\' or \'content\'' } + ]); + + done(); + } + + promise.then(expectations, expectations).catch(expectations); + }); + + it('rejects file parameters not in object format', (done) => { + const request = { + user: { _id: 'abc123' }, + body: { + name: 'Wriggly worm', + files: [{ name: 'file.js', content: 'var hello = true;' }] + } + }; + const response = new Response(); + + const promise = apiCreateProject(request, response); + + function expectations() { + const doc = response.json.mock.calls[0][0]; + + const responseBody = JSON.parse(JSON.stringify(doc)); + + expect(response.status).toHaveBeenCalledWith(422); + expect(responseBody.name).toBe('File Validation Failed'); + expect(responseBody.message).toBe('\'files\' must be an object'); + + done(); + } + + promise.then(expectations, expectations).catch(expectations); + }); + }); }); diff --git a/server/controllers/project.controller/createProject.js b/server/controllers/project.controller/createProject.js index f6f56d6318..56ae8048ca 100644 --- a/server/controllers/project.controller/createProject.js +++ b/server/controllers/project.controller/createProject.js @@ -1,5 +1,5 @@ import Project from '../../models/project'; -import { toModel } from '../../domain-objects/Project'; +import { toModel, FileValidationError } from '../../domain-objects/Project'; export default function createProject(req, res) { let projectValues = { @@ -36,22 +36,35 @@ export default function createProject(req, res) { export function apiCreateProject(req, res) { const params = Object.assign({ user: req.user._id }, req.body); + function sendValidationErrors(err) { + res.status(422).json({ + name: 'File Validation Failed', + message: err.message, + errors: err.files, + }); + } + // TODO: Error handling to match spec function sendFailure() { - res.json({ success: false }); + res.status(500).json({ success: false }); } try { const model = toModel(params); - model + return model .save() .then((newProject) => { res.status(201).json({ id: newProject.id }); }) .catch(sendFailure); } catch (err) { - // TODO: Catch custom err object and return correct status code - res.status(422).json({ error: 'Validation error' }); + if (err instanceof FileValidationError) { + sendValidationErrors(err); + } else { + sendFailure(); + } + + return Promise.reject(); } } diff --git a/server/domain-objects/Project.js b/server/domain-objects/Project.js index 19f2529740..c9e3a2964a 100644 --- a/server/domain-objects/Project.js +++ b/server/domain-objects/Project.js @@ -1,3 +1,4 @@ +import isPlainObject from 'lodash/isPlainObject'; import pick from 'lodash/pick'; import Project from '../models/project'; import createId from '../utils/createId'; @@ -110,8 +111,10 @@ export function transformFiles(tree = {}) { export function toModel(object) { let files = []; - if (typeof object.files === 'object') { + if (isPlainObject(object.files)) { files = transformFiles(object.files); + } else if (object.files != null) { + throw new FileValidationError('\'files\' must be an object'); } const projectValues = pick(object, ['user', 'name', 'slug']); diff --git a/server/models/__mocks__/project.js b/server/models/__mocks__/project.js index 7260fcfd68..4d9d3d3748 100644 --- a/server/models/__mocks__/project.js +++ b/server/models/__mocks__/project.js @@ -11,6 +11,19 @@ export function createMock() { return sinon.mock(Project); } +// Wraps the Project.prototype i.e. the +// instance methods in a mock so +// Project.save() can be mocked +export function createInstanceMock() { + // See: https://stackoverflow.com/questions/40962960/sinon-mock-of-mongoose-save-method-for-all-future-instances-of-a-model-with-pro + Object.defineProperty(Project.prototype, 'save', { + value: Project.prototype.save, + configurable: true, + }); + + return sinon.mock(Project.prototype); +} + // Re-export the model, it will be // altered by mockingoose whenever // we call methods on the MockConfig From cb767afc88042b617786b59dbcaffe07c4e3bacd Mon Sep 17 00:00:00 2001 From: Andrew Nicolaou Date: Wed, 19 Jun 2019 15:22:04 +0200 Subject: [PATCH 34/49] Tests toModel() --- .../domain-objects/__test__/Project.test.js | 34 ++++++++++++++++++- 1 file changed, 33 insertions(+), 1 deletion(-) diff --git a/server/domain-objects/__test__/Project.test.js b/server/domain-objects/__test__/Project.test.js index f9de8b33e2..d548f9c9f6 100644 --- a/server/domain-objects/__test__/Project.test.js +++ b/server/domain-objects/__test__/Project.test.js @@ -1,4 +1,4 @@ -import { transformFiles, FileValidationError } from '../Project'; +import { toModel, transformFiles, FileValidationError } from '../Project'; jest.mock('../../utils/createId'); @@ -6,6 +6,38 @@ jest.mock('../../utils/createId'); // TODO: File extension validation // describe('domain-objects/Project', () => { + describe('toModel', () => { + it('filters extra properties', () => { + const params = { + name: 'My sketch', + extraThing: 'oopsie', + }; + + const model = toModel(params); + + expect(model.name).toBe('My sketch'); + expect(model.extraThing).toBeUndefined(); + }); + + it('throws FileValidationError', () => { + const params = { + files: { + 'index.html': {} // missing content or url + } + }; + + expect(() => toModel(params)).toThrowError(FileValidationError); + }); + + it('throws if files is not an object', () => { + const params = { + files: [] + }; + + expect(() => toModel(params)).toThrowError(FileValidationError); + }); + }); + describe('transformFiles', () => { beforeEach(() => { // eslint-disable-next-line global-require From d89be672ac025a12696edb5e20dea48cde86a275 Mon Sep 17 00:00:00 2001 From: Andrew Nicolaou Date: Wed, 19 Jun 2019 15:51:24 +0200 Subject: [PATCH 35/49] Creates default files if no root-level .html file is provided --- .../__test__/project.controller.test.js | 1 - server/domain-objects/Project.js | 16 +- .../domain-objects/__test__/Project.test.js | 542 ++++++++++-------- server/domain-objects/createDefaultFiles.js | 48 ++ 4 files changed, 372 insertions(+), 235 deletions(-) create mode 100644 server/domain-objects/createDefaultFiles.js diff --git a/server/controllers/__test__/project.controller.test.js b/server/controllers/__test__/project.controller.test.js index 06538e8b77..2d17bae4cb 100644 --- a/server/controllers/__test__/project.controller.test.js +++ b/server/controllers/__test__/project.controller.test.js @@ -2,7 +2,6 @@ * @jest-environment node */ import { Response } from 'jest-express'; -import sinon from 'sinon'; import Project, { createMock, createInstanceMock } from '../../models/project'; import createProject, { apiCreateProject } from '../project.controller/createProject'; diff --git a/server/domain-objects/Project.js b/server/domain-objects/Project.js index c9e3a2964a..552021e99f 100644 --- a/server/domain-objects/Project.js +++ b/server/domain-objects/Project.js @@ -3,6 +3,7 @@ import pick from 'lodash/pick'; import Project from '../models/project'; import createId from '../utils/createId'; import createApplicationErrorClass from '../utils/createApplicationErrorClass'; +import createDefaultFiles from './createDefaultFiles'; export const FileValidationError = createApplicationErrorClass('FileValidationError'); @@ -103,6 +104,10 @@ export function transformFiles(tree = {}) { return files; } +export function containsRootHtmlFile(tree) { + return Object.keys(tree).find(name => /\.html$/.test(name)) != null; +} + /** * This converts between the public API's Project object * properties and a mongoose Project model @@ -110,10 +115,15 @@ export function transformFiles(tree = {}) { */ export function toModel(object) { let files = []; + let tree = object.files; + + if (isPlainObject(tree)) { + if (!containsRootHtmlFile(tree)) { + tree = Object.assign(createDefaultFiles(), tree); + } - if (isPlainObject(object.files)) { - files = transformFiles(object.files); - } else if (object.files != null) { + files = transformFiles(tree); + } else if (tree != null) { throw new FileValidationError('\'files\' must be an object'); } diff --git a/server/domain-objects/__test__/Project.test.js b/server/domain-objects/__test__/Project.test.js index d548f9c9f6..68a740ad64 100644 --- a/server/domain-objects/__test__/Project.test.js +++ b/server/domain-objects/__test__/Project.test.js @@ -1,4 +1,6 @@ -import { toModel, transformFiles, FileValidationError } from '../Project'; +import find from 'lodash/find'; + +import { containsRootHtmlFile, toModel, transformFiles, FileValidationError } from '../Project'; jest.mock('../../utils/createId'); @@ -6,6 +8,29 @@ jest.mock('../../utils/createId'); // TODO: File extension validation // describe('domain-objects/Project', () => { + describe('containsRootHtmlFile', () => { + it('returns true for at least one root .html', () => { + expect(containsRootHtmlFile({ 'index.html': {} })).toBe(true); + expect(containsRootHtmlFile({ 'another-one.html': {} })).toBe(true); + expect(containsRootHtmlFile({ 'one.html': {}, 'two.html': {} })).toBe(true); + expect(containsRootHtmlFile({ 'one.html': {}, 'sketch.js': {} })).toBe(true); + }); + + it('returns false anything else', () => { + expect(containsRootHtmlFile({ 'sketch.js': {} })).toBe(false); + }); + + it('ignores nested html', () => { + expect(containsRootHtmlFile({ + examples: { + files: { + 'index.html': {} + } + } + })).toBe(false); + }); + }); + describe('toModel', () => { it('filters extra properties', () => { const params = { @@ -36,269 +61,324 @@ describe('domain-objects/Project', () => { expect(() => toModel(params)).toThrowError(FileValidationError); }); - }); - describe('transformFiles', () => { - beforeEach(() => { - // eslint-disable-next-line global-require - const { resetMockCreateId } = require('../../utils/createId'); - - resetMockCreateId(); - }); + it('creates default index.html and dependent files if no root .html is provided', () => { + const params = { + files: {} + }; - it('creates an empty root with no data', () => { - const tree = {}; + const { files } = toModel(params); - expect(transformFiles(tree)).toEqual([{ - _id: '0', - fileType: 'folder', - name: 'root', - children: [] - }]); + expect(files.length).toBe(4); + expect(find(files, { name: 'index.html' })).not.toBeUndefined(); + expect(find(files, { name: 'sketch.js' })).not.toBeUndefined(); + expect(find(files, { name: 'style.css' })).not.toBeUndefined(); }); - it('converts tree-shaped files into list', () => { - const tree = { - 'index.html': { - content: 'some contents', + it('does not create default files if any root .html is provided', () => { + const params = { + files: { + 'example.html': { + content: 'Hello!' + } } }; - expect(transformFiles(tree)).toEqual([ - { - _id: '0', - fileType: 'folder', - name: 'root', - children: ['1'] - }, - { - _id: '1', - content: 'some contents', - fileType: 'file', - name: 'index.html' - } - ]); + const { files } = toModel(params); + + expect(files.length).toBe(2); + expect(find(files, { name: 'example.html' })).not.toBeUndefined(); + expect(find(files, { name: 'index.html' })).toBeUndefined(); + expect(find(files, { name: 'sketch.js' })).toBeUndefined(); + expect(find(files, { name: 'style.css' })).toBeUndefined(); }); - it('uses file url over content', () => { - const tree = { - 'script.js': { - url: 'http://example.net/something.js' + it('does not overwrite default CSS and JS of the same name if provided', () => { + const params = { + files: { + 'sketch.js': { + content: 'const sketch = true;' + }, + 'style.css': { + content: 'body { outline: 10px solid red; }' + } } }; - expect(transformFiles(tree)).toEqual([ - { - _id: '0', - fileType: 'folder', - name: 'root', - children: ['1'] - }, - { - _id: '1', - url: 'http://example.net/something.js', - fileType: 'file', - name: 'script.js' - } - ]); - }); + const { files } = toModel(params); - it('creates folders', () => { - const tree = { - 'a-folder': { - files: {} - }, - }; + expect(files.length).toBe(4); + expect(find(files, { name: 'index.html' })).not.toBeUndefined(); - expect(transformFiles(tree)).toEqual([ - { - _id: '0', - fileType: 'folder', - name: 'root', - children: ['1'] - }, - { - _id: '1', - children: [], - fileType: 'folder', - name: 'a-folder' - } - ]); + const sketchFile = find(files, { name: 'sketch.js' }); + expect(sketchFile.content).toBe('const sketch = true;'); + + const cssFile = find(files, { name: 'style.css' }); + expect(cssFile.content).toBe('body { outline: 10px solid red; }'); }); + }); +}); - it('walks the tree processing files', () => { - const tree = { - 'index.html': { - content: 'some contents', - }, - 'a-folder': { - files: { - 'data.csv': { - content: 'this,is,data' - }, - 'another.txt': { - content: 'blah blah' - } - } - }, - }; +describe('transformFiles', () => { + beforeEach(() => { + // eslint-disable-next-line global-require + const { resetMockCreateId } = require('../../utils/createId'); + + resetMockCreateId(); + }); + + it('creates an empty root with no data', () => { + const tree = {}; + + expect(transformFiles(tree)).toEqual([{ + _id: '0', + fileType: 'folder', + name: 'root', + children: [] + }]); + }); + + it('converts tree-shaped files into list', () => { + const tree = { + 'index.html': { + content: 'some contents', + } + }; + + expect(transformFiles(tree)).toEqual([ + { + _id: '0', + fileType: 'folder', + name: 'root', + children: ['1'] + }, + { + _id: '1', + content: 'some contents', + fileType: 'file', + name: 'index.html' + } + ]); + }); + + it('uses file url over content', () => { + const tree = { + 'script.js': { + url: 'http://example.net/something.js' + } + }; + + expect(transformFiles(tree)).toEqual([ + { + _id: '0', + fileType: 'folder', + name: 'root', + children: ['1'] + }, + { + _id: '1', + url: 'http://example.net/something.js', + fileType: 'file', + name: 'script.js' + } + ]); + }); + + it('creates folders', () => { + const tree = { + 'a-folder': { + files: {} + }, + }; - expect(transformFiles(tree)).toEqual([ - { - _id: '0', - fileType: 'folder', - name: 'root', - children: ['1', '2'] - }, - { - _id: '1', - name: 'index.html', - fileType: 'file', - content: 'some contents' - }, - { - _id: '2', - name: 'a-folder', - fileType: 'folder', - children: ['3', '4'] - }, - { - _id: '3', - name: 'data.csv', - fileType: 'file', - content: 'this,is,data' - }, - { - _id: '4', - name: 'another.txt', - fileType: 'file', - content: 'blah blah' + expect(transformFiles(tree)).toEqual([ + { + _id: '0', + fileType: 'folder', + name: 'root', + children: ['1'] + }, + { + _id: '1', + children: [], + fileType: 'folder', + name: 'a-folder' + } + ]); + }); + + it('walks the tree processing files', () => { + const tree = { + 'index.html': { + content: 'some contents', + }, + 'a-folder': { + files: { + 'data.csv': { + content: 'this,is,data' + }, + 'another.txt': { + content: 'blah blah' + } } - ]); - }); + }, + }; - it('handles deep nesting', () => { - const tree = { - first: { - files: { - second: { - files: { - third: { - files: { - 'hello.js': { - content: 'world!' - } + expect(transformFiles(tree)).toEqual([ + { + _id: '0', + fileType: 'folder', + name: 'root', + children: ['1', '2'] + }, + { + _id: '1', + name: 'index.html', + fileType: 'file', + content: 'some contents' + }, + { + _id: '2', + name: 'a-folder', + fileType: 'folder', + children: ['3', '4'] + }, + { + _id: '3', + name: 'data.csv', + fileType: 'file', + content: 'this,is,data' + }, + { + _id: '4', + name: 'another.txt', + fileType: 'file', + content: 'blah blah' + } + ]); + }); + + it('handles deep nesting', () => { + const tree = { + first: { + files: { + second: { + files: { + third: { + files: { + 'hello.js': { + content: 'world!' } } } } } - }, - }; - - expect(transformFiles(tree)).toEqual([ - { - _id: '0', - fileType: 'folder', - name: 'root', - children: ['1'] - }, - { - _id: '1', - name: 'first', - fileType: 'folder', - children: ['2'] - }, - { - _id: '2', - name: 'second', - fileType: 'folder', - children: ['3'] - }, - { - _id: '3', - name: 'third', - fileType: 'folder', - children: ['4'] - }, - { - _id: '4', - name: 'hello.js', - fileType: 'file', - content: 'world!' } - ]); - }); + }, + }; + expect(transformFiles(tree)).toEqual([ + { + _id: '0', + fileType: 'folder', + name: 'root', + children: ['1'] + }, + { + _id: '1', + name: 'first', + fileType: 'folder', + children: ['2'] + }, + { + _id: '2', + name: 'second', + fileType: 'folder', + children: ['3'] + }, + { + _id: '3', + name: 'third', + fileType: 'folder', + children: ['4'] + }, + { + _id: '4', + name: 'hello.js', + fileType: 'file', + content: 'world!' + } + ]); + }); - it('allows duplicate names in different folder', () => { - const tree = { - 'index.html': { - content: 'some contents', - }, - 'data': { - files: { - 'index.html': { - content: 'different file' - } - } - }, - }; - expect(transformFiles(tree)).toEqual([ - { - _id: '0', - fileType: 'folder', - name: 'root', - children: ['1', '2'] - }, - { - _id: '1', - name: 'index.html', - fileType: 'file', - content: 'some contents' - }, - { - _id: '2', - name: 'data', - fileType: 'folder', - children: ['3'] - }, - { - _id: '3', - name: 'index.html', - fileType: 'file', - content: 'different file' + it('allows duplicate names in different folder', () => { + const tree = { + 'index.html': { + content: 'some contents', + }, + 'data': { + files: { + 'index.html': { + content: 'different file' + } } - ]); - }); + }, + }; - it('validates files', () => { - const tree = { - 'index.html': {} // missing `content` - }; + expect(transformFiles(tree)).toEqual([ + { + _id: '0', + fileType: 'folder', + name: 'root', + children: ['1', '2'] + }, + { + _id: '1', + name: 'index.html', + fileType: 'file', + content: 'some contents' + }, + { + _id: '2', + name: 'data', + fileType: 'folder', + children: ['3'] + }, + { + _id: '3', + name: 'index.html', + fileType: 'file', + content: 'different file' + } + ]); + }); - expect(() => transformFiles(tree)).toThrowError(FileValidationError); - }); + it('validates files', () => { + const tree = { + 'index.html': {} // missing `content` + }; - it('collects all file validation errors', () => { - const tree = { - 'index.html': {}, // missing `content` - 'something.js': {} // also missing `content` - }; + expect(() => transformFiles(tree)).toThrowError(FileValidationError); + }); - try { - transformFiles(tree); - - // Should not get here - throw new Error('should have thrown before this point'); - } catch (err) { - expect(err).toBeInstanceOf(FileValidationError); - expect(err.files).toEqual([ - { name: 'index.html', message: 'missing \'url\' or \'content\'' }, - { name: 'something.js', message: 'missing \'url\' or \'content\'' } - ]); - } - }); + it('collects all file validation errors', () => { + const tree = { + 'index.html': {}, // missing `content` + 'something.js': {} // also missing `content` + }; + + try { + transformFiles(tree); + + // Should not get here + throw new Error('should have thrown before this point'); + } catch (err) { + expect(err).toBeInstanceOf(FileValidationError); + expect(err.files).toEqual([ + { name: 'index.html', message: 'missing \'url\' or \'content\'' }, + { name: 'something.js', message: 'missing \'url\' or \'content\'' } + ]); + } }); }); diff --git a/server/domain-objects/createDefaultFiles.js b/server/domain-objects/createDefaultFiles.js new file mode 100644 index 0000000000..9164c12af3 --- /dev/null +++ b/server/domain-objects/createDefaultFiles.js @@ -0,0 +1,48 @@ +const defaultSketch = `function setup() { + createCanvas(400, 400); +} + +function draw() { + background(220); +}`; + +const defaultHTML = + ` + + + + + + + + + + + + + +`; + +const defaultCSS = + `html, body { + margin: 0; + padding: 0; +} +canvas { + display: block; +} +`; + +export default function createDefaultFiles() { + return { + 'index.html': { + content: defaultHTML + }, + 'style.css': { + content: defaultCSS + }, + 'sketch.js': { + content: defaultSketch + } + }; +} From 558ad5891a44b6b579c27d54752ac662ba8b9189 Mon Sep 17 00:00:00 2001 From: Andrew Nicolaou Date: Wed, 19 Jun 2019 18:10:56 +0200 Subject: [PATCH 36/49] Do not auto-generate a slug if it is provided Fixes a bug where the slug was auto-generated using the sketch name, even if a slug property had been provided. --- server/models/project.js | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/server/models/project.js b/server/models/project.js index 8642bc90cc..d21d2390e9 100644 --- a/server/models/project.js +++ b/server/models/project.js @@ -49,7 +49,11 @@ projectSchema.set('toJSON', { projectSchema.pre('save', function generateSlug(next) { const project = this; - project.slug = slugify(project.name, '_'); + + if (!project.slug) { + project.slug = slugify(project.name, '_'); + } + return next(); }); From 475179c4f97ca292280b5583c00aea489c955839 Mon Sep 17 00:00:00 2001 From: Andrew Nicolaou Date: Wed, 19 Jun 2019 18:49:56 +0200 Subject: [PATCH 37/49] Validates uniqueness of slugs for projects created by the public API --- .../project.controller/createProject.js | 46 +++++++++++++------ server/domain-objects/Project.js | 1 + server/models/project.js | 31 +++++++++++++ 3 files changed, 63 insertions(+), 15 deletions(-) diff --git a/server/controllers/project.controller/createProject.js b/server/controllers/project.controller/createProject.js index 56ae8048ca..53534cad1d 100644 --- a/server/controllers/project.controller/createProject.js +++ b/server/controllers/project.controller/createProject.js @@ -1,5 +1,5 @@ import Project from '../../models/project'; -import { toModel, FileValidationError } from '../../domain-objects/Project'; +import { toModel, FileValidationError, ProjectValidationError } from '../../domain-objects/Project'; export default function createProject(req, res) { let projectValues = { @@ -36,35 +36,51 @@ export default function createProject(req, res) { export function apiCreateProject(req, res) { const params = Object.assign({ user: req.user._id }, req.body); - function sendValidationErrors(err) { + function sendValidationErrors(err, type) { res.status(422).json({ - name: 'File Validation Failed', + name: `${type} Validation Failed`, message: err.message, errors: err.files, }); } // TODO: Error handling to match spec - function sendFailure() { + function sendFailure(err) { res.status(500).json({ success: false }); } - try { - const model = toModel(params); - - return model - .save() - .then((newProject) => { - res.status(201).json({ id: newProject.id }); - }) - .catch(sendFailure); - } catch (err) { + function handleErrors(err) { if (err instanceof FileValidationError) { - sendValidationErrors(err); + sendValidationErrors(err, 'File'); + } else if (err instanceof ProjectValidationError) { + sendValidationErrors(err, 'Sketch'); } else { sendFailure(); } return Promise.reject(); } + + try { + const model = toModel(params); + + return model.isSlugUnique() + .then(({ isUnique, conflictingIds }) => { + if (isUnique) { + return model.save() + .then((newProject) => { + res.status(201).json({ id: newProject.id }); + }); + } + + const error = new ProjectValidationError(`Slug "${model.slug}" is not unique. Check ${conflictingIds.join(', ')}`); + + throw error; + }) + .catch(handleErrors); + } catch (err) { + handleErrors(err); + + return Promise.reject(err); + } } diff --git a/server/domain-objects/Project.js b/server/domain-objects/Project.js index 552021e99f..86727024d7 100644 --- a/server/domain-objects/Project.js +++ b/server/domain-objects/Project.js @@ -6,6 +6,7 @@ import createApplicationErrorClass from '../utils/createApplicationErrorClass'; import createDefaultFiles from './createDefaultFiles'; export const FileValidationError = createApplicationErrorClass('FileValidationError'); +export const ProjectValidationError = createApplicationErrorClass('ProjectValidationError'); // objectID().toHexString(); diff --git a/server/models/project.js b/server/models/project.js index d21d2390e9..bf8c992eb9 100644 --- a/server/models/project.js +++ b/server/models/project.js @@ -57,4 +57,35 @@ projectSchema.pre('save', function generateSlug(next) { return next(); }); +/** + * Check if slug is unique for this user's projects + */ +projectSchema.methods.isSlugUnique = async function isSlugUnique(cb) { + const project = this; + const hasCallback = typeof cb === 'function'; + + try { + const docsWithSlug = await project.model('Project') + .find({ user: project.user, slug: project.slug }, '_id') + .exec(); + + const result = { + isUnique: docsWithSlug.length === 0, + conflictingIds: docsWithSlug.map(d => d._id) || [] + }; + + if (hasCallback) { + cb(null, result); + } + + return result; + } catch (err) { + if (hasCallback) { + cb(err, null); + } + + throw err; + } +}; + export default mongoose.model('Project', projectSchema); From f80ba5c7415daa7546314e67cf81f5408a36a9ba Mon Sep 17 00:00:00 2001 From: Andrew Nicolaou Date: Thu, 27 Jun 2019 17:48:33 +0200 Subject: [PATCH 38/49] Adds tests for slug uniqueness --- .../__test__/project.controller.test.js | 47 +++++++++++++++++++ .../project.controller/createProject.js | 12 ++--- 2 files changed, 52 insertions(+), 7 deletions(-) diff --git a/server/controllers/__test__/project.controller.test.js b/server/controllers/__test__/project.controller.test.js index 2d17bae4cb..ab14ebeb43 100644 --- a/server/controllers/__test__/project.controller.test.js +++ b/server/controllers/__test__/project.controller.test.js @@ -185,6 +185,9 @@ describe('project.controller', () => { files: [] }; + ProjectInstanceMock.expects('isSlugUnique') + .resolves({ isUnique: true, conflictingIds: [] }); + ProjectInstanceMock.expects('save') .resolves(new Project(result)); @@ -206,6 +209,50 @@ describe('project.controller', () => { promise.then(expectations, expectations).catch(expectations); }); + it('fails if slug is not unique', (done) => { + const request = { + user: { _id: 'abc123' }, + body: { + name: 'My sketch', + slug: 'a-slug', + files: {} + } + }; + const response = new Response(); + + const result = { + _id: 'abc123', + id: 'abc123', + name: 'Project name', + // slug: 'a-slug', + serveSecure: false, + files: [] + }; + + ProjectInstanceMock.expects('isSlugUnique') + .resolves({ isUnique: false, conflictingIds: ['cde123'] }); + + ProjectInstanceMock.expects('save') + .resolves(new Project(result)); + + const promise = apiCreateProject(request, response); + + function expectations() { + const doc = response.json.mock.calls[0][0]; + + expect(response.status).toHaveBeenCalledWith(409); + expect(response.json).toHaveBeenCalled(); + + expect(JSON.parse(JSON.stringify(doc))).toMatchObject({ + message: 'Slug "a-slug" is not unique. Check cde123' + }); + + done(); + } + + promise.then(expectations, expectations).catch(expectations); + }); + it('returns validation errors on files input', (done) => { const request = { user: {}, diff --git a/server/controllers/project.controller/createProject.js b/server/controllers/project.controller/createProject.js index 53534cad1d..df1229f225 100644 --- a/server/controllers/project.controller/createProject.js +++ b/server/controllers/project.controller/createProject.js @@ -36,8 +36,8 @@ export default function createProject(req, res) { export function apiCreateProject(req, res) { const params = Object.assign({ user: req.user._id }, req.body); - function sendValidationErrors(err, type) { - res.status(422).json({ + function sendValidationErrors(err, type, code = 422) { + res.status(code).json({ name: `${type} Validation Failed`, message: err.message, errors: err.files, @@ -51,14 +51,12 @@ export function apiCreateProject(req, res) { function handleErrors(err) { if (err instanceof FileValidationError) { - sendValidationErrors(err, 'File'); + sendValidationErrors(err, 'File', err.code); } else if (err instanceof ProjectValidationError) { - sendValidationErrors(err, 'Sketch'); + sendValidationErrors(err, 'Sketch', err.code); } else { sendFailure(); } - - return Promise.reject(); } try { @@ -74,13 +72,13 @@ export function apiCreateProject(req, res) { } const error = new ProjectValidationError(`Slug "${model.slug}" is not unique. Check ${conflictingIds.join(', ')}`); + error.code = 409; throw error; }) .catch(handleErrors); } catch (err) { handleErrors(err); - return Promise.reject(err); } } From ca66043f86792a213d41c3b09084248cd79c7f26 Mon Sep 17 00:00:00 2001 From: Andrew Nicolaou Date: Thu, 27 Jun 2019 17:48:50 +0200 Subject: [PATCH 39/49] Configures node's Promise implementation for mongoose (fixes warnings) --- jest.setup.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/jest.setup.js b/jest.setup.js index 7c9d2b77a9..da03678506 100644 --- a/jest.setup.js +++ b/jest.setup.js @@ -1,5 +1,8 @@ import { configure } from 'enzyme' import Adapter from 'enzyme-adapter-react-16' import '@babel/polyfill' +import mongoose from 'mongoose' + +mongoose.Promise = global.Promise; configure({ adapter: new Adapter() }) From 5bd757d01f56a9a10d5da4fe290a9d67f43b3a30 Mon Sep 17 00:00:00 2001 From: Andrew Nicolaou Date: Thu, 27 Jun 2019 17:51:22 +0200 Subject: [PATCH 40/49] Moves createProject tests to match controller location --- .../__test__/createProject.test.js} | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) rename server/controllers/{__test__/project.controller.test.js => project.controller/__test__/createProject.test.js} (97%) diff --git a/server/controllers/__test__/project.controller.test.js b/server/controllers/project.controller/__test__/createProject.test.js similarity index 97% rename from server/controllers/__test__/project.controller.test.js rename to server/controllers/project.controller/__test__/createProject.test.js index ab14ebeb43..7e180b151b 100644 --- a/server/controllers/__test__/project.controller.test.js +++ b/server/controllers/project.controller/__test__/createProject.test.js @@ -3,10 +3,10 @@ */ import { Response } from 'jest-express'; -import Project, { createMock, createInstanceMock } from '../../models/project'; -import createProject, { apiCreateProject } from '../project.controller/createProject'; +import Project, { createMock, createInstanceMock } from '../../../models/project'; +import createProject, { apiCreateProject } from '../createProject'; -jest.mock('../../models/project'); +jest.mock('../../../models/project'); describe('project.controller', () => { describe('createProject()', () => { From a08603a7df3b23c3a584b87912645ba8f00d931c Mon Sep 17 00:00:00 2001 From: Andrew Nicolaou Date: Thu, 27 Jun 2019 19:04:22 +0200 Subject: [PATCH 41/49] Adds support for code to ApplicationErrors --- server/utils/createApplicationErrorClass.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/server/utils/createApplicationErrorClass.js b/server/utils/createApplicationErrorClass.js index f5499d85d1..f949866c38 100644 --- a/server/utils/createApplicationErrorClass.js +++ b/server/utils/createApplicationErrorClass.js @@ -3,13 +3,14 @@ * the application. */ export class ApplicationError extends Error { - constructor(message) { + constructor(message, extra = {}) { super(); if (Error.captureStackTrace) { Error.captureStackTrace(this, this.constructor); } this.name = 'ApplicationError'; this.message = message; + this.code = extra.code; } } From 8798c6ddd56ae11569a79490755f5ebb511309d5 Mon Sep 17 00:00:00 2001 From: Andrew Nicolaou Date: Thu, 27 Jun 2019 18:22:04 +0200 Subject: [PATCH 42/49] deleteProject controller tests --- .../controllers/__mocks__/aws.controller.js | 5 + server/controllers/project.controller.js | 33 +---- .../__test__/deleteProject.test.js | 115 ++++++++++++++++++ .../project.controller/deleteProject.js | 57 +++++++++ server/models/__mocks__/project.js | 5 + 5 files changed, 183 insertions(+), 32 deletions(-) create mode 100644 server/controllers/__mocks__/aws.controller.js create mode 100644 server/controllers/project.controller/__test__/deleteProject.test.js create mode 100644 server/controllers/project.controller/deleteProject.js diff --git a/server/controllers/__mocks__/aws.controller.js b/server/controllers/__mocks__/aws.controller.js new file mode 100644 index 0000000000..9e5709c403 --- /dev/null +++ b/server/controllers/__mocks__/aws.controller.js @@ -0,0 +1,5 @@ +export const getObjectKey = jest.mock(); +export const deleteObjectsFromS3 = jest.fn(); +export const signS3 = jest.fn(); +export const copyObjectInS3 = jest.fn(); +export const listObjectsInS3ForUser = jest.fn(); diff --git a/server/controllers/project.controller.js b/server/controllers/project.controller.js index 046c6ebcb4..8a302533ce 100644 --- a/server/controllers/project.controller.js +++ b/server/controllers/project.controller.js @@ -2,7 +2,6 @@ import archiver from 'archiver'; import format from 'date-fns/format'; import isUrl from 'is-url'; import jsdom, { serializeDocument } from 'jsdom'; -import isBefore from 'date-fns/is_before'; import isAfter from 'date-fns/is_after'; import request from 'request'; import slugify from 'slugify'; @@ -10,11 +9,11 @@ import Project from '../models/project'; import User from '../models/user'; import { resolvePathToFile } from '../utils/filePath'; import generateFileSystemSafeName from '../utils/generateFileSystemSafeName'; -import { deleteObjectsFromS3, getObjectKey } from './aws.controller'; import { toApi as toApiProjectObject } from '../domain-objects/Project'; import createApplicationErrorClass from '../utils/createApplicationErrorClass'; export { default as createProject, apiCreateProject } from './project.controller/createProject'; +export { default as deleteProject } from './project.controller/deleteProject'; const UserNotFoundError = createApplicationErrorClass('UserNotFoundError'); @@ -88,36 +87,7 @@ export function getProject(req, res) { }); } -function deleteFilesFromS3(files) { - deleteObjectsFromS3(files.filter((file) => { - if (file.url) { - if (!process.env.S3_DATE || ( - process.env.S3_DATE && - isBefore(new Date(process.env.S3_DATE), new Date(file.createdAt)))) { - return true; - } - } - return false; - }) - .map(file => getObjectKey(file.url))); -} -export function deleteProject(req, res) { - Project.findById(req.params.project_id, (findProjectErr, project) => { - if (!project.user.equals(req.user._id)) { - res.status(403).json({ success: false, message: 'Authenticated user does not match owner of project.' }); - return; - } - deleteFilesFromS3(project.files); - project.remove((removeProjectError) => { - if (removeProjectError) { - res.status(404).send({ message: 'Project with that id does not exist' }); - return; - } - res.json({ success: true }); - }); - }); -} export function getProjectsForUserId(userId) { return new Promise((resolve, reject) => { @@ -239,7 +209,6 @@ export function apiGetProjectsForUser(req, res) { } } - export function projectExists(projectId, callback) { Project.findById(projectId, (err, project) => ( project ? callback(true) : callback(false) diff --git a/server/controllers/project.controller/__test__/deleteProject.test.js b/server/controllers/project.controller/__test__/deleteProject.test.js new file mode 100644 index 0000000000..18b93c68f4 --- /dev/null +++ b/server/controllers/project.controller/__test__/deleteProject.test.js @@ -0,0 +1,115 @@ +/** + * @jest-environment node + */ +import { Request, Response } from 'jest-express'; + +import Project, { createMock, createInstanceMock } from '../../../models/project'; +import User from '../../../models/user'; +import deleteProject from '../../project.controller/deleteProject'; +import { deleteObjectsFromS3 } from '../../aws.controller'; + + +jest.mock('../../../models/project'); +jest.mock('../../aws.controller'); + +describe('project.controller', () => { + describe('deleteProject()', () => { + let ProjectMock; + let ProjectInstanceMock; + + beforeEach(() => { + ProjectMock = createMock(); + ProjectInstanceMock = createInstanceMock(); + }); + + afterEach(() => { + ProjectMock.restore(); + ProjectInstanceMock.restore(); + }); + + it('returns 403 if project is not owned by authenticated user', (done) => { + const user = new User(); + const project = new Project(); + project.user = user; + + const request = new Request(); + request.setParams({ project_id: project._id }); + request.user = { _id: 'abc123' }; + + const response = new Response(); + + ProjectMock + .expects('findById') + .resolves(project); + + const promise = deleteProject(request, response); + + function expectations() { + expect(response.status).toHaveBeenCalledWith(403); + expect(response.json).toHaveBeenCalledWith({ success: false, message: 'Authenticated user does not match owner of project' }); + + done(); + } + + promise.then(expectations, expectations).catch(expectations); + }); + + it('returns 404 if project does not exist', (done) => { + const user = new User(); + const project = new Project(); + project.user = user; + + const request = new Request(); + request.setParams({ project_id: project._id }); + request.user = { _id: 'abc123' }; + + const response = new Response(); + + ProjectMock + .expects('findById') + .resolves(null); + + const promise = deleteProject(request, response); + + function expectations() { + expect(response.status).toHaveBeenCalledWith(404); + expect(response.json).toHaveBeenCalledWith({ success: false, message: 'Project with that id does not exist' }); + + done(); + } + + promise.then(expectations, expectations).catch(expectations); + }); + + it('deletes project and dependent files from S3 ', (done) => { + const user = new User(); + const project = new Project(); + project.user = user; + + const request = new Request(); + request.setParams({ project_id: project._id }); + request.user = { _id: user._id }; + + const response = new Response(); + + ProjectMock + .expects('findById') + .resolves(project); + + ProjectInstanceMock.expects('remove') + .yields(); + + const promise = deleteProject(request, response); + + function expectations() { + expect(response.status).toHaveBeenCalledWith(200); + expect(response.json).toHaveBeenCalledWith({ success: true }); + expect(deleteObjectsFromS3).toHaveBeenCalled(); + + done(); + } + + promise.then(expectations, expectations).catch(expectations); + }); + }); +}); diff --git a/server/controllers/project.controller/deleteProject.js b/server/controllers/project.controller/deleteProject.js new file mode 100644 index 0000000000..5a70f543ee --- /dev/null +++ b/server/controllers/project.controller/deleteProject.js @@ -0,0 +1,57 @@ +import isBefore from 'date-fns/is_before'; +import Project from '../../models/project'; +import { deleteObjectsFromS3, getObjectKey } from '../aws.controller'; +import createApplicationErrorClass from '../../utils/createApplicationErrorClass'; + +const ProjectDeletionError = createApplicationErrorClass('ProjectDeletionError'); + +function deleteFilesFromS3(files) { + deleteObjectsFromS3(files.filter((file) => { + if (file.url) { + if (!process.env.S3_DATE || ( + process.env.S3_DATE && + isBefore(new Date(process.env.S3_DATE), new Date(file.createdAt)))) { + return true; + } + } + return false; + }) + .map(file => getObjectKey(file.url))); +} + +export default function deleteProject(req, res) { + function sendFailure(error) { + res.status(error.code).json({ success: false, message: error.message }); + } + + function sendProjectNotFound() { + sendFailure(new ProjectDeletionError('Project with that id does not exist', { code: 404 })); + } + + function handleProjectDeletion(project) { + if (project == null) { + sendProjectNotFound(); + return; + } + + if (!project.user.equals(req.user._id)) { + sendFailure(new ProjectDeletionError('Authenticated user does not match owner of project', { code: 403 })); + return; + } + + deleteFilesFromS3(project.files); + + project.remove((removeProjectError) => { + if (removeProjectError) { + sendProjectNotFound(); + return; + } + + res.status(200).json({ success: true }); + }); + } + + return Project.findById(req.params.project_id) + .then(handleProjectDeletion) + .catch(sendFailure); +} diff --git a/server/models/__mocks__/project.js b/server/models/__mocks__/project.js index 4d9d3d3748..2590046f3c 100644 --- a/server/models/__mocks__/project.js +++ b/server/models/__mocks__/project.js @@ -21,6 +21,11 @@ export function createInstanceMock() { configurable: true, }); + Object.defineProperty(Project.prototype, 'remove', { + value: Project.prototype.remove, + configurable: true, + }); + return sinon.mock(Project.prototype); } From afb30c2865d8dc3cce8aca659fbedaaf36deefe7 Mon Sep 17 00:00:00 2001 From: Andrew Nicolaou Date: Mon, 1 Jul 2019 19:12:41 +0200 Subject: [PATCH 43/49] getProjectsForUser controller tests - implements tests - update apiKey tests to use new User mocks --- server/controllers/project.controller.js | 73 +------ .../__test__/getProjectsForUser.test.js | 151 +++++++++++++++ .../project.controller/getProjectsForUser.js | 72 +++++++ .../user.controller/__tests__/apiKey.test.js | 178 +++++++++--------- server/controllers/user.controller/apiKey.js | 102 +++++----- server/models/__mocks__/user.js | 43 +++-- 6 files changed, 404 insertions(+), 215 deletions(-) create mode 100644 server/controllers/project.controller/__test__/getProjectsForUser.test.js create mode 100644 server/controllers/project.controller/getProjectsForUser.js diff --git a/server/controllers/project.controller.js b/server/controllers/project.controller.js index 8a302533ce..bf59a865da 100644 --- a/server/controllers/project.controller.js +++ b/server/controllers/project.controller.js @@ -9,13 +9,10 @@ import Project from '../models/project'; import User from '../models/user'; import { resolvePathToFile } from '../utils/filePath'; import generateFileSystemSafeName from '../utils/generateFileSystemSafeName'; -import { toApi as toApiProjectObject } from '../domain-objects/Project'; -import createApplicationErrorClass from '../utils/createApplicationErrorClass'; export { default as createProject, apiCreateProject } from './project.controller/createProject'; export { default as deleteProject } from './project.controller/deleteProject'; - -const UserNotFoundError = createApplicationErrorClass('UserNotFoundError'); +export { default as getProjectsForUser, apiGetProjectsForUser } from './project.controller/getProjectsForUser'; export function updateProject(req, res) { Project.findById(req.params.project_id, (findProjectErr, project) => { @@ -87,8 +84,6 @@ export function getProject(req, res) { }); } - - export function getProjectsForUserId(userId) { return new Promise((resolve, reject) => { Project.find({ user: userId }) @@ -131,34 +126,6 @@ export function getProjectAsset(req, res) { }); } -function getProjectsForUserName(username) { - return new Promise((resolve, reject) => { - User.findOne({ username }, (err, user) => { - if (err) { - reject(err); - return; - } - - if (!user) { - reject(new UserNotFoundError()); - return; - } - - Project.find({ user: user._id }) - .sort('-createdAt') - .select('name files id createdAt updatedAt') - .exec((innerErr, projects) => { - if (innerErr) { - reject(innerErr); - return; - } - - resolve(projects); - }); - }); - }); -} - export function getProjects(req, res) { if (req.user) { getProjectsForUserId(req.user._id) @@ -171,44 +138,6 @@ export function getProjects(req, res) { } } -export function getProjectsForUser(req, res) { - if (req.params.username) { - getProjectsForUserName(req.params.username) - .then(projects => res.json(projects)) - .catch((err) => { - if (err instanceof UserNotFoundError) { - res.status(404).json({ message: 'User with that username does not exist.' }); - } else { - res.status(500).json({ message: 'Error fetching projects' }); - } - }); - } else { - // could just move this to client side - res.json([]); - } -} - -export function apiGetProjectsForUser(req, res) { - if (req.params.username) { - getProjectsForUserName(req.params.username) - .then((projects) => { - const asApiObjects = projects.map(p => toApiProjectObject(p)); - res.json({ sketches: asApiObjects }); - }) - .catch((err) => { - if (err instanceof UserNotFoundError) { - res.status(404).json({ message: 'User with that username does not exist.' }); - } else { - console.error(err); - res.status(500).json({ message: 'Error fetching projects' }); - } - }); - } else { - // could just move this to client side - res.json({ sketches: [] }); - } -} - export function projectExists(projectId, callback) { Project.findById(projectId, (err, project) => ( project ? callback(true) : callback(false) diff --git a/server/controllers/project.controller/__test__/getProjectsForUser.test.js b/server/controllers/project.controller/__test__/getProjectsForUser.test.js new file mode 100644 index 0000000000..05a0cf7166 --- /dev/null +++ b/server/controllers/project.controller/__test__/getProjectsForUser.test.js @@ -0,0 +1,151 @@ +/** + * @jest-environment node + */ +import { Request, Response } from 'jest-express'; + +import { createMock } from '../../../models/user'; +import getProjectsForUser, { apiGetProjectsForUser } from '../../project.controller/getProjectsForUser'; + +jest.mock('../../../models/user'); +jest.mock('../../aws.controller'); + +describe('project.controller', () => { + let UserMock; + + beforeEach(() => { + UserMock = createMock(); + }); + + afterEach(() => { + UserMock.restore(); + }); + + describe('getProjectsForUser()', () => { + it('returns empty array user not supplied as parameter', (done) => { + const request = new Request(); + request.setParams({}); + const response = new Response(); + + const promise = getProjectsForUser(request, response); + + function expectations() { + expect(response.status).toHaveBeenCalledWith(200); + expect(response.json).toHaveBeenCalledWith([]); + + done(); + } + + promise.then(expectations, expectations).catch(expectations); + }); + + it('returns 404 if user does not exist', (done) => { + const request = new Request(); + request.setParams({ username: 'abc123' }); + const response = new Response(); + + UserMock + .expects('findOne') + .withArgs({ username: 'abc123' }) + .yields(null, null); + + const promise = getProjectsForUser(request, response); + + function expectations() { + expect(response.status).toHaveBeenCalledWith(404); + expect(response.json).toHaveBeenCalledWith({ message: 'User with that username does not exist.' }); + + done(); + } + + promise.then(expectations, expectations).catch(expectations); + }); + + it('returns 500 on other errors', (done) => { + const request = new Request(); + request.setParams({ username: 'abc123' }); + const response = new Response(); + + UserMock + .expects('findOne') + .withArgs({ username: 'abc123' }) + .yields(new Error(), null); + + const promise = getProjectsForUser(request, response); + + function expectations() { + expect(response.status).toHaveBeenCalledWith(500); + expect(response.json).toHaveBeenCalledWith({ message: 'Error fetching projects' }); + + done(); + } + + promise.then(expectations, expectations).catch(expectations); + }); + }); + + describe('apiGetProjectsForUser()', () => { + it('returns validation error if user id not provided', (done) => { + const request = new Request(); + request.setParams({}); + const response = new Response(); + + const promise = apiGetProjectsForUser(request, response); + + function expectations() { + expect(response.status).toHaveBeenCalledWith(422); + expect(response.json).toHaveBeenCalledWith({ + message: 'Username not provided' + }); + + done(); + } + + promise.then(expectations, expectations).catch(expectations); + }); + + + it('returns 404 if user does not exist', (done) => { + const request = new Request(); + request.setParams({ username: 'abc123' }); + const response = new Response(); + + UserMock + .expects('findOne') + .withArgs({ username: 'abc123' }) + .yields(null, null); + + const promise = apiGetProjectsForUser(request, response); + + function expectations() { + expect(response.status).toHaveBeenCalledWith(404); + expect(response.json).toHaveBeenCalledWith({ message: 'User with that username does not exist.' }); + + done(); + } + + promise.then(expectations, expectations).catch(expectations); + }); + + it('returns 500 on other errors', (done) => { + const request = new Request(); + request.setParams({ username: 'abc123' }); + const response = new Response(); + + UserMock + .expects('findOne') + .withArgs({ username: 'abc123' }) + .yields(new Error(), null); + + const promise = apiGetProjectsForUser(request, response); + + function expectations() { + expect(response.status).toHaveBeenCalledWith(500); + expect(response.json).toHaveBeenCalledWith({ message: 'Error fetching projects' }); + + done(); + } + + promise.then(expectations, expectations).catch(expectations); + }); + }); +}); diff --git a/server/controllers/project.controller/getProjectsForUser.js b/server/controllers/project.controller/getProjectsForUser.js new file mode 100644 index 0000000000..1d9a0e34ae --- /dev/null +++ b/server/controllers/project.controller/getProjectsForUser.js @@ -0,0 +1,72 @@ +import Project from '../../models/project'; +import User from '../../models/user'; +import { toApi as toApiProjectObject } from '../../domain-objects/Project'; +import createApplicationErrorClass from '../../utils/createApplicationErrorClass'; + +const UserNotFoundError = createApplicationErrorClass('UserNotFoundError'); + +function getProjectsForUserName(username) { + return new Promise((resolve, reject) => { + User.findOne({ username }, (err, user) => { + if (err) { + reject(err); + return; + } + + if (!user) { + reject(new UserNotFoundError()); + return; + } + + Project.find({ user: user._id }) + .sort('-createdAt') + .select('name files id createdAt updatedAt') + .exec((innerErr, projects) => { + if (innerErr) { + reject(innerErr); + return; + } + + resolve(projects); + }); + }); + }); +} + +export default function getProjectsForUser(req, res) { + if (req.params.username) { + return getProjectsForUserName(req.params.username) + .then(projects => res.json(projects)) + .catch((err) => { + if (err instanceof UserNotFoundError) { + res.status(404).json({ message: 'User with that username does not exist.' }); + } else { + res.status(500).json({ message: 'Error fetching projects' }); + } + }); + } + + // could just move this to client side + res.status(200).json([]); + return Promise.resolve(); +} + +export function apiGetProjectsForUser(req, res) { + if (req.params.username) { + return getProjectsForUserName(req.params.username) + .then((projects) => { + const asApiObjects = projects.map(p => toApiProjectObject(p)); + res.json({ sketches: asApiObjects }); + }) + .catch((err) => { + if (err instanceof UserNotFoundError) { + res.status(404).json({ message: 'User with that username does not exist.' }); + } else { + res.status(500).json({ message: 'Error fetching projects' }); + } + }); + } + + res.status(422).json({ message: 'Username not provided' }); + return Promise.resolve(); +} diff --git a/server/controllers/user.controller/__tests__/apiKey.test.js b/server/controllers/user.controller/__tests__/apiKey.test.js index a496373bc8..7e396288b0 100644 --- a/server/controllers/user.controller/__tests__/apiKey.test.js +++ b/server/controllers/user.controller/__tests__/apiKey.test.js @@ -1,73 +1,40 @@ /* @jest-environment node */ import last from 'lodash/last'; +import { Request, Response } from 'jest-express'; + +import User, { createMock, createInstanceMock } from '../../../models/user'; import { createApiKey, removeApiKey } from '../../user.controller/apiKey'; jest.mock('../../../models/user'); -/* - Create a mock object representing an express Response -*/ -const createResponseMock = function createResponseMock(done) { - const json = jest.fn(() => { - if (done) { done(); } - }); - - const status = jest.fn(() => ({ json })); - - return { - status, - json - }; -}; - -/* - Create a mock of the mongoose User model -*/ -const createUserMock = function createUserMock() { - const apiKeys = []; - let nextId = 0; - - apiKeys.push = ({ label, hashedKey }) => { - const id = nextId; - nextId += 1; - const publicFields = { id, label }; - const allFields = { ...publicFields, hashedKey }; - - Object.defineProperty(allFields, 'toObject', { - value: () => publicFields, - enumerable: false - }); - - return Array.prototype.push.call(apiKeys, allFields); - }; - - apiKeys.pull = ({ _id }) => { - const index = apiKeys.findIndex(({ id }) => id === _id); - return apiKeys.splice(index, 1); - }; - - return { - apiKeys, - save: jest.fn(callback => callback()) - }; -}; - -const User = require('../../../models/user').default; - describe('user.controller', () => { + let UserMock; + let UserInstanceMock; + beforeEach(() => { - User.__setFindById(null, null); + UserMock = createMock(); + UserInstanceMock = createInstanceMock(); + }); + + afterEach(() => { + UserMock.restore(); + UserInstanceMock.restore(); }); + describe('createApiKey', () => { it('returns an error if user doesn\'t exist', () => { const request = { user: { id: '1234' } }; - const response = createResponseMock(); + const response = new Response(); + + UserMock + .expects('findById') + .withArgs('1234') + .yields(null, null); createApiKey(request, response); - expect(User.findById.mock.calls[0][0]).toBe('1234'); expect(response.status).toHaveBeenCalledWith(404); expect(response.json).toHaveBeenCalledWith({ error: 'User not found' @@ -75,10 +42,13 @@ describe('user.controller', () => { }); it('returns an error if label not provided', () => { - User.__setFindById(undefined, createUserMock()); - const request = { user: { id: '1234' }, body: {} }; - const response = createResponseMock(); + const response = new Response(); + + UserMock + .expects('findById') + .withArgs('1234') + .yields(null, new User()); createApiKey(request, response); @@ -89,46 +59,59 @@ describe('user.controller', () => { }); it('returns generated API key to the user', (done) => { - let response; + const request = new Request(); + request.setBody({ label: 'my key' }); + request.user = { id: '1234' }; - const request = { - user: { id: '1234' }, - body: { label: 'my key' } - }; + const response = new Response(); + + const user = new User(); + + UserMock + .expects('findById') + .withArgs('1234') + .yields(null, user); - const user = createUserMock(); + UserInstanceMock.expects('save') + .yields(); - const checkExpecations = () => { + function expectations() { const lastKey = last(user.apiKeys); expect(lastKey.label).toBe('my key'); expect(typeof lastKey.hashedKey).toBe('string'); - expect(response.json).toHaveBeenCalledWith({ - apiKeys: [ - { id: 0, label: 'my key', token: lastKey.hashedKey } - ] + const responseData = response.json.mock.calls[0][0]; + + expect(responseData.apiKeys.length).toBe(1); + expect(responseData.apiKeys[0]).toMatchObject({ + label: 'my key', + token: lastKey.hashedKey, + lastUsedAt: undefined, + createdAt: undefined }); done(); - }; + } - response = createResponseMock(checkExpecations); + const promise = createApiKey(request, response); - User.__setFindById(undefined, user); - - createApiKey(request, response); + promise.then(expectations, expectations).catch(expectations); }); }); describe('removeApiKey', () => { it('returns an error if user doesn\'t exist', () => { const request = { user: { id: '1234' } }; - const response = createResponseMock(); + const response = new Response(); + + UserMock + .expects('findById') + .withArgs('1234') + .yields(null, null); removeApiKey(request, response); - expect(User.findById.mock.calls[0][0]).toBe('1234'); expect(response.status).toHaveBeenCalledWith(404); expect(response.json).toHaveBeenCalledWith({ error: 'User not found' @@ -140,10 +123,13 @@ describe('user.controller', () => { user: { id: '1234' }, params: { keyId: 'not-a-real-key' } }; - const response = createResponseMock(); + const response = new Response(); + const user = new User(); - const user = createUserMock(); - User.__setFindById(undefined, user); + UserMock + .expects('findById') + .withArgs('1234') + .yields(null, user); removeApiKey(request, response); @@ -153,27 +139,41 @@ describe('user.controller', () => { }); }); - it.skip('removes key if it exists', () => { + it('removes key if it exists', () => { + const user = new User(); + user.apiKeys.push({ label: 'first key' }); // id 0 + user.apiKeys.push({ label: 'second key' }); // id 1 + + const firstKeyId = user.apiKeys[0]._id.toString(); + const secondKeyId = user.apiKeys[1]._id.toString(); + const request = { user: { id: '1234' }, - params: { keyId: 0 } + params: { keyId: firstKeyId } }; - const response = createResponseMock(); + const response = new Response(); - const user = createUserMock(); + UserMock + .expects('findById') + .withArgs('1234') + .yields(null, user); - user.apiKeys.push({ label: 'first key' }); // id 0 - user.apiKeys.push({ label: 'second key' }); // id 1 - - User.__setFindById(undefined, user); + UserInstanceMock + .expects('save') + .yields(); removeApiKey(request, response); expect(response.status).toHaveBeenCalledWith(200); - expect(response.json).toHaveBeenCalledWith({ - apiKeys: [ - { id: 1, label: 'second key' } - ] + + const responseData = response.json.mock.calls[0][0]; + + expect(responseData.apiKeys.length).toBe(1); + expect(responseData.apiKeys[0]).toMatchObject({ + id: secondKeyId, + label: 'second key', + lastUsedAt: undefined, + createdAt: undefined }); }); }); diff --git a/server/controllers/user.controller/apiKey.js b/server/controllers/user.controller/apiKey.js index eed22e6fb6..5d45b123e2 100644 --- a/server/controllers/user.controller/apiKey.js +++ b/server/controllers/user.controller/apiKey.js @@ -19,67 +19,85 @@ function generateApiKey() { } export function createApiKey(req, res) { - User.findById(req.user.id, async (err, user) => { - if (!user) { - res.status(404).json({ error: 'User not found' }); - return; - } - - if (!req.body.label) { - res.status(400).json({ error: 'Expected field \'label\' was not present in request body' }); - return; + return new Promise((resolve, reject) => { + function sendFailure(code, error) { + res.status(code).json({ error }); + resolve(); } - const keyToBeHashed = await generateApiKey(); - - const addedApiKeyIndex = user.apiKeys.push({ label: req.body.label, hashedKey: keyToBeHashed }); + User.findById(req.user.id, async (err, user) => { + if (!user) { + sendFailure(404, 'User not found'); + return; + } - user.save((saveErr) => { - if (saveErr) { - res.status(500).json({ error: saveErr }); + if (!req.body.label) { + sendFailure(400, 'Expected field \'label\' was not present in request body'); return; } - const apiKeys = user.apiKeys - .map((apiKey, index) => { - const fields = apiKey.toObject(); - const shouldIncludeToken = index === addedApiKeyIndex - 1; + const keyToBeHashed = await generateApiKey(); + + const addedApiKeyIndex = user.apiKeys.push({ label: req.body.label, hashedKey: keyToBeHashed }); + + user.save((saveErr) => { + if (saveErr) { + sendFailure(500, saveErr); + return; + } - return shouldIncludeToken ? - { ...fields, token: keyToBeHashed } : - fields; - }); + const apiKeys = user.apiKeys + .map((apiKey, index) => { + const fields = apiKey.toObject(); + const shouldIncludeToken = index === addedApiKeyIndex - 1; - res.json({ apiKeys }); + return shouldIncludeToken ? + { ...fields, token: keyToBeHashed } : + fields; + }); + + res.json({ apiKeys }); + resolve(); + }); }); }); } export function removeApiKey(req, res) { - User.findById(req.user.id, (err, user) => { - if (err) { - res.status(500).json({ error: err }); - return; - } - if (!user) { - res.status(404).json({ error: 'User not found' }); - return; - } - const keyToDelete = user.apiKeys.find(key => key.id === req.params.keyId); - if (!keyToDelete) { - res.status(404).json({ error: 'Key does not exist for user' }); - return; + return new Promise((resolve, reject) => { + function sendFailure(code, error) { + res.status(code).json({ error }); + resolve(); } - user.apiKeys.pull({ _id: req.params.keyId }); + User.findById(req.user.id, (err, user) => { + if (err) { + sendFailure(500, err); + return; + } + + if (!user) { + sendFailure(404, 'User not found'); + return; + } - user.save((saveErr) => { - if (saveErr) { - res.status(500).json({ error: saveErr }); + const keyToDelete = user.apiKeys.find(key => key.id === req.params.keyId); + if (!keyToDelete) { + sendFailure(404, 'Key does not exist for user'); return; } - res.status(200).json({ apiKeys: user.apiKeys }); + user.apiKeys.pull({ _id: req.params.keyId }); + + user.save((saveErr) => { + if (saveErr) { + sendFailure(500, saveErr); + return; + } + + res.status(200).json({ apiKeys: user.apiKeys }); + resolve(); + }); }); }); } diff --git a/server/models/__mocks__/user.js b/server/models/__mocks__/user.js index 585d8b673c..fcd73f32a1 100644 --- a/server/models/__mocks__/user.js +++ b/server/models/__mocks__/user.js @@ -1,12 +1,31 @@ -let __err = null; -let __user = null; - -export default { - __setFindById(err, user) { - __err = err; - __user = user; - }, - findById: jest.fn(async (id, callback) => { - callback(__err, __user); - }) -}; +import sinon from 'sinon'; +import 'sinon-mongoose'; + +// Import the actual model to be mocked +const User = jest.requireActual('../user').default; + +// Wrap User in a sinon mock +// The returned object is used to configure +// the mocked model's behaviour +export function createMock() { + return sinon.mock(User); +} + +// Wraps the User.prototype i.e. the +// instance methods in a mock so +// User.save() can be mocked +export function createInstanceMock() { + // See: https://stackoverflow.com/questions/40962960/sinon-mock-of-mongoose-save-method-for-all-future-instances-of-a-model-with-pro + Object.defineProperty(User.prototype, 'save', { + value: User.prototype.save, + configurable: true, + }); + + return sinon.mock(User.prototype); +} + + +// Re-export the model, it will be +// altered by mockingoose whenever +// we call methods on the MockConfig +export default User; From d3a56211a45d778ec715488d8b7630d9c3f84026 Mon Sep 17 00:00:00 2001 From: Andrew Nicolaou Date: Tue, 2 Jul 2019 10:49:14 +0200 Subject: [PATCH 44/49] Ensure error objects have consistent property names `message` is used as a high-level description of the errors `detail` is optional and has an plain language explanation of the individual errors `errors` is an array of each individual problem from `detail` in a machine-readable format --- .../project.controller/__test__/createProject.test.js | 10 ++++++---- .../project.controller/__test__/deleteProject.test.js | 10 +++++++--- server/controllers/project.controller/createProject.js | 6 +++--- server/controllers/project.controller/deleteProject.js | 4 ++-- server/domain-objects/Project.js | 2 -- 5 files changed, 18 insertions(+), 14 deletions(-) diff --git a/server/controllers/project.controller/__test__/createProject.test.js b/server/controllers/project.controller/__test__/createProject.test.js index 7e180b151b..32873a5295 100644 --- a/server/controllers/project.controller/__test__/createProject.test.js +++ b/server/controllers/project.controller/__test__/createProject.test.js @@ -244,7 +244,8 @@ describe('project.controller', () => { expect(response.json).toHaveBeenCalled(); expect(JSON.parse(JSON.stringify(doc))).toMatchObject({ - message: 'Slug "a-slug" is not unique. Check cde123' + message: 'Sketch Validation Failed', + detail: 'Slug "a-slug" is not unique. Check cde123' }); done(); @@ -275,7 +276,8 @@ describe('project.controller', () => { const responseBody = JSON.parse(JSON.stringify(doc)); expect(response.status).toHaveBeenCalledWith(422); - expect(responseBody.name).toBe('File Validation Failed'); + expect(responseBody.message).toBe('File Validation Failed'); + expect(responseBody.detail).not.toBeNull(); expect(responseBody.errors.length).toBe(1); expect(responseBody.errors).toEqual([ { name: 'index.html', message: 'missing \'url\' or \'content\'' } @@ -305,8 +307,8 @@ describe('project.controller', () => { const responseBody = JSON.parse(JSON.stringify(doc)); expect(response.status).toHaveBeenCalledWith(422); - expect(responseBody.name).toBe('File Validation Failed'); - expect(responseBody.message).toBe('\'files\' must be an object'); + expect(responseBody.message).toBe('File Validation Failed'); + expect(responseBody.detail).toBe('\'files\' must be an object'); done(); } diff --git a/server/controllers/project.controller/__test__/deleteProject.test.js b/server/controllers/project.controller/__test__/deleteProject.test.js index 18b93c68f4..84a6824fe8 100644 --- a/server/controllers/project.controller/__test__/deleteProject.test.js +++ b/server/controllers/project.controller/__test__/deleteProject.test.js @@ -46,7 +46,9 @@ describe('project.controller', () => { function expectations() { expect(response.status).toHaveBeenCalledWith(403); - expect(response.json).toHaveBeenCalledWith({ success: false, message: 'Authenticated user does not match owner of project' }); + expect(response.json).toHaveBeenCalledWith({ + message: 'Authenticated user does not match owner of project' + }); done(); } @@ -73,7 +75,9 @@ describe('project.controller', () => { function expectations() { expect(response.status).toHaveBeenCalledWith(404); - expect(response.json).toHaveBeenCalledWith({ success: false, message: 'Project with that id does not exist' }); + expect(response.json).toHaveBeenCalledWith({ + message: 'Project with that id does not exist' + }); done(); } @@ -103,7 +107,7 @@ describe('project.controller', () => { function expectations() { expect(response.status).toHaveBeenCalledWith(200); - expect(response.json).toHaveBeenCalledWith({ success: true }); + expect(response.json).not.toHaveBeenCalled(); expect(deleteObjectsFromS3).toHaveBeenCalled(); done(); diff --git a/server/controllers/project.controller/createProject.js b/server/controllers/project.controller/createProject.js index df1229f225..144285a529 100644 --- a/server/controllers/project.controller/createProject.js +++ b/server/controllers/project.controller/createProject.js @@ -38,15 +38,15 @@ export function apiCreateProject(req, res) { function sendValidationErrors(err, type, code = 422) { res.status(code).json({ - name: `${type} Validation Failed`, - message: err.message, + message: `${type} Validation Failed`, + detail: err.message, errors: err.files, }); } // TODO: Error handling to match spec function sendFailure(err) { - res.status(500).json({ success: false }); + res.status(500).end(); } function handleErrors(err) { diff --git a/server/controllers/project.controller/deleteProject.js b/server/controllers/project.controller/deleteProject.js index 5a70f543ee..dd980b231f 100644 --- a/server/controllers/project.controller/deleteProject.js +++ b/server/controllers/project.controller/deleteProject.js @@ -21,7 +21,7 @@ function deleteFilesFromS3(files) { export default function deleteProject(req, res) { function sendFailure(error) { - res.status(error.code).json({ success: false, message: error.message }); + res.status(error.code).json({ message: error.message }); } function sendProjectNotFound() { @@ -47,7 +47,7 @@ export default function deleteProject(req, res) { return; } - res.status(200).json({ success: true }); + res.status(200).end(); }); } diff --git a/server/domain-objects/Project.js b/server/domain-objects/Project.js index 86727024d7..7b1908d7d8 100644 --- a/server/domain-objects/Project.js +++ b/server/domain-objects/Project.js @@ -8,8 +8,6 @@ import createDefaultFiles from './createDefaultFiles'; export const FileValidationError = createApplicationErrorClass('FileValidationError'); export const ProjectValidationError = createApplicationErrorClass('ProjectValidationError'); - -// objectID().toHexString(); /** * This converts between a mongoose Project model * and the public API Project object properties From e3c93e5c747526e940d5edf6594f32083fa94347 Mon Sep 17 00:00:00 2001 From: Andrew Nicolaou Date: Thu, 4 Jul 2019 16:01:01 +0200 Subject: [PATCH 45/49] Assert environment variables are provided at script start --- server/scripts/examples-ml5.js | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/server/scripts/examples-ml5.js b/server/scripts/examples-ml5.js index 6285d93570..51e1fe5d5e 100644 --- a/server/scripts/examples-ml5.js +++ b/server/scripts/examples-ml5.js @@ -1,6 +1,7 @@ import fs from 'fs'; import rp from 'request-promise'; import Q from 'q'; +import { ok } from 'assert'; // TODO: Change branchName if necessary const branchName = 'release'; @@ -10,10 +11,17 @@ const clientId = process.env.GITHUB_ID; const clientSecret = process.env.GITHUB_SECRET; const editorUsername = process.env.ML5_EXAMPLES_USERNAME; const personalAccessToken = process.env.EDITOR_API_ACCESS_TOKEN; +const editorApiUrl = process.env.EDITOR_API_URL; const headers = { 'User-Agent': 'p5js-web-editor/0.0.1' }; +ok(clientId, 'GITHUB_ID is required'); +ok(clientSecret, 'GITHUB_SECRET is required'); +ok(editorUsername, 'ML5_EXAMPLES_USERNAME is required'); +ok(personalAccessToken, 'EDITOR_API_ACCESS_TOKEN is required'); +ok(editorApiUrl, 'EDITOR_API_URL is required'); + // const githubRequestOptions = { url: baseUrl, @@ -27,7 +35,7 @@ const githubRequestOptions = { }; const editorRequestOptions = { - url: `${process.env.P5_API || 'http://localhost:8000/api'}/${editorUsername}`, + url: `${editorApiUrl}/${editorUsername}`, method: 'GET', headers: { ...headers, From 2b0a218ab94ea01afe26f7b4e98b4555b13b59b1 Mon Sep 17 00:00:00 2001 From: Andrew Nicolaou Date: Thu, 4 Jul 2019 16:01:11 +0200 Subject: [PATCH 46/49] Version public API --- server/server.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/server.js b/server/server.js index bbbe85580d..af6ea25ab4 100644 --- a/server/server.js +++ b/server/server.js @@ -96,7 +96,7 @@ app.use(session({ app.use(passport.initialize()); app.use(passport.session()); -app.use('/api', requestsOfTypeJSON(), api); +app.use('/api/v1', requestsOfTypeJSON(), api); app.use('/api', requestsOfTypeJSON(), users); app.use('/api', requestsOfTypeJSON(), sessions); app.use('/api', requestsOfTypeJSON(), files); From 218e54dee7d258b31179b4f835a37137fa186321 Mon Sep 17 00:00:00 2001 From: Andrew Nicolaou Date: Sun, 21 Jul 2019 17:56:06 +0200 Subject: [PATCH 47/49] Expect "files" property to always be provided --- .../__test__/createProject.test.js | 28 +++++++++++++++++++ server/domain-objects/Project.js | 2 +- .../domain-objects/__test__/Project.test.js | 1 + 3 files changed, 30 insertions(+), 1 deletion(-) diff --git a/server/controllers/project.controller/__test__/createProject.test.js b/server/controllers/project.controller/__test__/createProject.test.js index 32873a5295..a5bfb02019 100644 --- a/server/controllers/project.controller/__test__/createProject.test.js +++ b/server/controllers/project.controller/__test__/createProject.test.js @@ -315,5 +315,33 @@ describe('project.controller', () => { promise.then(expectations, expectations).catch(expectations); }); + + it('rejects if files object in not provided', (done) => { + const request = { + user: { _id: 'abc123' }, + body: { + name: 'Wriggly worm', + // files: {} is missing + } + }; + const response = new Response(); + + const promise = apiCreateProject(request, response); + + function expectations() { + const doc = response.json.mock.calls[0][0]; + + const responseBody = JSON.parse(JSON.stringify(doc)); + + expect(response.status).toHaveBeenCalledWith(422); + expect(responseBody.message).toBe('File Validation Failed'); + expect(responseBody.detail).toBe('\'files\' must be an object'); + + done(); + } + + promise.then(expectations, expectations).catch(expectations); + }); + }); }); diff --git a/server/domain-objects/Project.js b/server/domain-objects/Project.js index 7b1908d7d8..f0de023f2a 100644 --- a/server/domain-objects/Project.js +++ b/server/domain-objects/Project.js @@ -122,7 +122,7 @@ export function toModel(object) { } files = transformFiles(tree); - } else if (tree != null) { + } else { throw new FileValidationError('\'files\' must be an object'); } diff --git a/server/domain-objects/__test__/Project.test.js b/server/domain-objects/__test__/Project.test.js index 68a740ad64..0f3c9dd979 100644 --- a/server/domain-objects/__test__/Project.test.js +++ b/server/domain-objects/__test__/Project.test.js @@ -36,6 +36,7 @@ describe('domain-objects/Project', () => { const params = { name: 'My sketch', extraThing: 'oopsie', + files: {} }; const model = toModel(params); From 9f06995d338dcaed369c224c4851285e39c0dffd Mon Sep 17 00:00:00 2001 From: Andrew Nicolaou Date: Sun, 21 Jul 2019 18:35:33 +0200 Subject: [PATCH 48/49] Fixes linting error --- .../project.controller/__test__/createProject.test.js | 1 - 1 file changed, 1 deletion(-) diff --git a/server/controllers/project.controller/__test__/createProject.test.js b/server/controllers/project.controller/__test__/createProject.test.js index a5bfb02019..99d388001d 100644 --- a/server/controllers/project.controller/__test__/createProject.test.js +++ b/server/controllers/project.controller/__test__/createProject.test.js @@ -342,6 +342,5 @@ describe('project.controller', () => { promise.then(expectations, expectations).catch(expectations); }); - }); }); From 3c23fee996c8f4577ffd5c2569dd6a953e4605d0 Mon Sep 17 00:00:00 2001 From: Andrew Nicolaou Date: Fri, 30 Aug 2019 15:31:16 +0200 Subject: [PATCH 49/49] Checks that authenticated user has permission to create under this namespace Previously, the project was always created under the authenticated user's namespace, but this not obvious behaviour. --- .../__test__/createProject.test.js | 55 +++++++++++++++++-- .../project.controller/createProject.js | 13 +++++ 2 files changed, 63 insertions(+), 5 deletions(-) diff --git a/server/controllers/project.controller/__test__/createProject.test.js b/server/controllers/project.controller/__test__/createProject.test.js index 99d388001d..68025a0cb5 100644 --- a/server/controllers/project.controller/__test__/createProject.test.js +++ b/server/controllers/project.controller/__test__/createProject.test.js @@ -169,7 +169,8 @@ describe('project.controller', () => { it('returns 201 with id of created sketch', (done) => { const request = { - user: { _id: 'abc123' }, + user: { _id: 'abc123', username: 'alice' }, + params: { username: 'alice' }, body: { name: 'My sketch', files: {} @@ -211,7 +212,8 @@ describe('project.controller', () => { it('fails if slug is not unique', (done) => { const request = { - user: { _id: 'abc123' }, + user: { _id: 'abc123', username: 'alice' }, + params: { username: 'alice' }, body: { name: 'My sketch', slug: 'a-slug', @@ -254,9 +256,50 @@ describe('project.controller', () => { promise.then(expectations, expectations).catch(expectations); }); + it('fails if user does not have permission', (done) => { + const request = { + user: { _id: 'abc123', username: 'alice' }, + params: { + username: 'dana', + }, + body: { + name: 'My sketch', + slug: 'a-slug', + files: {} + } + }; + const response = new Response(); + + const result = { + _id: 'abc123', + id: 'abc123', + name: 'Project name', + serveSecure: false, + files: [] + }; + + ProjectInstanceMock.expects('isSlugUnique') + .resolves({ isUnique: true, conflictingIds: [] }); + + ProjectInstanceMock.expects('save') + .resolves(new Project(result)); + + const promise = apiCreateProject(request, response); + + function expectations() { + expect(response.status).toHaveBeenCalledWith(401); + expect(response.json).toHaveBeenCalled(); + + done(); + } + + promise.then(expectations, expectations).catch(expectations); + }); + it('returns validation errors on files input', (done) => { const request = { - user: {}, + user: { username: 'alice' }, + params: { username: 'alice' }, body: { name: 'My sketch', files: { @@ -291,7 +334,8 @@ describe('project.controller', () => { it('rejects file parameters not in object format', (done) => { const request = { - user: { _id: 'abc123' }, + user: { _id: 'abc123', username: 'alice' }, + params: { username: 'alice' }, body: { name: 'Wriggly worm', files: [{ name: 'file.js', content: 'var hello = true;' }] @@ -318,7 +362,8 @@ describe('project.controller', () => { it('rejects if files object in not provided', (done) => { const request = { - user: { _id: 'abc123' }, + user: { _id: 'abc123', username: 'alice' }, + params: { username: 'alice' }, body: { name: 'Wriggly worm', // files: {} is missing diff --git a/server/controllers/project.controller/createProject.js b/server/controllers/project.controller/createProject.js index 144285a529..e40402682e 100644 --- a/server/controllers/project.controller/createProject.js +++ b/server/controllers/project.controller/createProject.js @@ -59,7 +59,19 @@ export function apiCreateProject(req, res) { } } + function checkUserHasPermission() { + if (req.user.username !== req.params.username) { + console.log('no permission'); + const error = new ProjectValidationError(`'${req.user.username}' does not have permission to create for '${req.params.username}'`); + error.code = 401; + + throw error; + } + } + try { + checkUserHasPermission(); + const model = toModel(params); return model.isSlugUnique() @@ -76,6 +88,7 @@ export function apiCreateProject(req, res) { throw error; }) + .then(checkUserHasPermission) .catch(handleErrors); } catch (err) { handleErrors(err);