From ae82875c5719b689f663f2e3579737a9cc766786 Mon Sep 17 00:00:00 2001 From: Scott Erickson Date: Wed, 24 Aug 2016 15:46:35 -0700 Subject: [PATCH] Refactor post new level version handler, add failed save handling When a new version is created, the latest version is updated, then the new one is made. If making a new one fails (most commonly due to a name conflict), the latest version is left in a broken state. Set up the new middleware to revert changes to latest version in this case, and update the level handler to use the middleware. Also added warning logs if models do not have editableProperties or postEditableProperties set. --- server/commons/database.coffee | 8 +++- server/handlers/level_handler.coffee | 60 +----------------------- server/middleware/versions.coffee | 35 +++++++++----- server/models/Achievement.coffee | 1 + server/models/Campaign.coffee | 1 + server/models/Classroom.coffee | 1 + server/models/CodeLog.coffee | 1 + server/models/Level.coffee | 60 ++++++++++++++++++++++++ server/routes/index.coffee | 7 ++- spec/server/functional/level.spec.coffee | 33 ++++++++++++- 10 files changed, 131 insertions(+), 76 deletions(-) diff --git a/server/commons/database.coffee b/server/commons/database.coffee index 34b006744..218ec51f9 100644 --- a/server/commons/database.coffee +++ b/server/commons/database.coffee @@ -123,10 +123,14 @@ module.exports = if _.isEmpty(req.body) throw new errors.UnprocessableEntity('No input') - props = doc.schema.statics.editableProperties.slice() + if not doc.schema.statics.editableProperties + console.warn 'No editableProperties set for', doc.constructor.modelName + props = (doc.schema.statics.editableProperties or []).slice() if doc.isNew - props = props.concat doc.schema.statics.postEditableProperties + props = props.concat(doc.schema.statics.postEditableProperties or []) + if not doc.schema.statics.postEditableProperties + console.warn 'No postEditableProperties set for', doc.constructor.modelName if doc.schema.uses_coco_permissions and req.user isOwner = doc.getAccessForUserObjectId(req.user._id) is 'owner' diff --git a/server/handlers/level_handler.coffee b/server/handlers/level_handler.coffee index cd5b73dd6..7534c7840 100644 --- a/server/handlers/level_handler.coffee +++ b/server/handlers/level_handler.coffee @@ -16,64 +16,8 @@ Classroom = require '../models/Classroom' LevelHandler = class LevelHandler extends Handler modelClass: Level jsonSchema: require '../../app/schemas/models/level' - editableProperties: [ - 'description' - 'documentation' - 'background' - 'nextLevel' - 'scripts' - 'thangs' - 'systems' - 'victory' - 'name' - 'i18n' - 'icon' - 'goals' - 'type' - 'showsGuide' - 'banner' - 'employerDescription' - 'terrain' - 'i18nCoverage' - 'loadingTip' - 'requiresSubscription' - 'adventurer' - 'practice' - 'shareable' - 'adminOnly' - 'disableSpaces' - 'hidesSubmitUntilRun' - 'hidesPlayButton' - 'hidesRunShortcut' - 'hidesHUD' - 'hidesSay' - 'hidesCodeToolbar' - 'hidesRealTimePlayback' - 'backspaceThrottle' - 'lockDefaultCode' - 'moveRightLoopSnippet' - 'realTimeSpeedFactor' - 'autocompleteFontSizePx' - 'requiredCode' - 'suspectCode' - 'requiredGear' - 'restrictedGear' - 'allowedHeroes' - 'tasks' - 'helpVideos' - 'campaign' - 'campaignIndex' - 'replayable' - 'buildTime' - 'scoreTypes' - 'concepts' - 'picoCTFProblem' - 'practiceThresholdMinutes', - 'primerLanguage' - 'studentPlayInstructions' - ] - - postEditableProperties: ['name'] + editableProperties: Level.editableProperties + postEditableProperties: Level.postEditableProperties getByRelationship: (req, res, args...) -> return @getSession(req, res, args[0]) if args[1] is 'session' diff --git a/server/middleware/versions.coffee b/server/middleware/versions.coffee index cafdf18ed..a569f99be 100644 --- a/server/middleware/versions.coffee +++ b/server/middleware/versions.coffee @@ -9,62 +9,66 @@ mongoose = require 'mongoose' database = require '../commons/database' parse = require '../commons/parse' +# More info on database versioning: https://github.com/codecombat/codecombat/wiki/Versioning + module.exports = postNewVersion: (Model, options={}) -> wrap (req, res) -> + # Find the document which is getting a new version parent = yield database.getDocFromHandle(req, Model) if not parent throw new errors.NotFound('Parent not found.') - # TODO: Figure out a better way to do this + # Check permissions + # TODO: Figure out an encapsulated way to do this; it's more permissions than versioning if options.hasPermissionsOrTranslations permissions = options.hasPermissionsOrTranslations permissions = [permissions] if _.isString(permissions) permissions = ['admin'] if not _.isArray(permissions) hasPermission = _.any(req.user?.hasPermission(permission) for permission in permissions) + if Model.schema.uses_coco_permissions and not hasPermission + hasPermission = parent.hasPermissionsForMethod(req.user, req.method) if not (hasPermission or database.isJustFillingTranslations(req, parent)) throw new errors.Forbidden() + # Create the new version, a clone of the parent with POST data applied doc = database.initDoc(req, Model) ATTRIBUTES_NOT_INHERITED = ['_id', 'version', 'created', 'creator'] doc.set(_.omit(parent.toObject(), ATTRIBUTES_NOT_INHERITED)) - database.assignBody(req, doc, { unsetMissing: true }) - # Get latest version + # Get latest (minor or major) version. This may not be the same document (or same major version) as parent. + latestSelect = 'version index slug' major = req.body.version?.major original = parent.get('original') if _.isNumber(major) q1 = Model.findOne({original: original, 'version.isLatestMinor': true, 'version.major': major}) else q1 = Model.findOne({original: original, 'version.isLatestMajor': true}) - q1.select 'version' + q1.select latestSelect latest = yield q1.exec() + # Handle the case where no version is marked as latest, since making new + # versions is not atomic if not latest - # handle the case where no version is marked as latest, since making new - # versions is not atomic if _.isNumber(major) q2 = Model.findOne({original: original, 'version.major': major}) q2.sort({'version.minor': -1}) else q2 = Model.findOne() q2.sort({'version.major': -1, 'version.minor': -1}) - q2.select 'version' + q2.select(latestSelect) latest = yield q2.exec() if not latest throw new errors.NotFound('Previous version not found.') - # Transfer latest version + # Update the latest version, making it no longer the latest. This includes major = req.body.version?.major version = _.clone(latest.get('version')) wasLatestMajor = version.isLatestMajor version.isLatestMajor = false if _.isNumber(major) version.isLatestMinor = false - - conditions = {_id: latest._id} - - raw = yield Model.update(conditions, {version: version, $unset: {index: 1, slug: 1}}) + raw = yield latest.update({$set: {version: version}, $unset: {index: 1, slug: 1}}) if not raw.nModified console.error('Conditions', conditions) console.error('Doc', doc) @@ -89,7 +93,12 @@ module.exports = doc.set('parent', latest._id) - doc = yield doc.save() + try + doc = yield doc.save() + catch e + # Revert changes to latest doc made earlier, should set everything back to normal + yield latest.update({$set: _.pick(latest.toObject(), 'version', 'index', 'slug')}) + throw e editPath = req.headers['x-current-path'] docLink = "http://codecombat.com#{editPath}" diff --git a/server/models/Achievement.coffee b/server/models/Achievement.coffee index f2190f135..6d4322139 100644 --- a/server/models/Achievement.coffee +++ b/server/models/Achievement.coffee @@ -93,6 +93,7 @@ AchievementSchema.statics.editableProperties = [ 'i18nCoverage' 'hidden' ] +AchievementSchema.statics.postEditableProperties = [] AchievementSchema.statics.jsonSchema = require '../../app/schemas/models/achievement' diff --git a/server/models/Campaign.coffee b/server/models/Campaign.coffee index a5a8f0998..7a4a89217 100644 --- a/server/models/Campaign.coffee +++ b/server/models/Campaign.coffee @@ -56,5 +56,6 @@ CampaignSchema.statics.editableProperties = [ 'adjacentCampaigns' 'levels' ] +CampaignSchema.statics.postEditableProperties = [] module.exports = mongoose.model('campaign', CampaignSchema) diff --git a/server/models/Classroom.coffee b/server/models/Classroom.coffee index ae2495a54..69ed379d1 100644 --- a/server/models/Classroom.coffee +++ b/server/models/Classroom.coffee @@ -23,6 +23,7 @@ ClassroomSchema.statics.editableProperties = [ 'ageRangeMax' 'archived' ] +ClassroomSchema.statics.postEditableProperties = [] ClassroomSchema.statics.generateNewCode = (done) -> tryCode = -> diff --git a/server/models/CodeLog.coffee b/server/models/CodeLog.coffee index b51f10be1..9d542b4ce 100644 --- a/server/models/CodeLog.coffee +++ b/server/models/CodeLog.coffee @@ -28,6 +28,7 @@ CodeLogSchema.statics.editableProperties = [ 'log' 'created' ] +CodeLogSchema.statics.postEditableProperties = [] CodeLogSchema.statics.jsonSchema = require '../../app/schemas/models/codelog.schema' diff --git a/server/models/Level.coffee b/server/models/Level.coffee index 3b7764a07..1467b72e2 100644 --- a/server/models/Level.coffee +++ b/server/models/Level.coffee @@ -45,5 +45,65 @@ LevelSchema.plugin(plugins.TranslationCoveragePlugin) LevelSchema.post 'init', (doc) -> if _.isString(doc.get('nextLevel')) doc.set('nextLevel', undefined) + +LevelSchema.statics.postEditableProperties = ['name'] + +LevelSchema.statics.editableProperties = [ + 'description' + 'documentation' + 'background' + 'nextLevel' + 'scripts' + 'thangs' + 'systems' + 'victory' + 'name' + 'i18n' + 'icon' + 'goals' + 'type' + 'showsGuide' + 'banner' + 'employerDescription' + 'terrain' + 'i18nCoverage' + 'loadingTip' + 'requiresSubscription' + 'adventurer' + 'practice' + 'shareable' + 'adminOnly' + 'disableSpaces' + 'hidesSubmitUntilRun' + 'hidesPlayButton' + 'hidesRunShortcut' + 'hidesHUD' + 'hidesSay' + 'hidesCodeToolbar' + 'hidesRealTimePlayback' + 'backspaceThrottle' + 'lockDefaultCode' + 'moveRightLoopSnippet' + 'realTimeSpeedFactor' + 'autocompleteFontSizePx' + 'requiredCode' + 'suspectCode' + 'requiredGear' + 'restrictedGear' + 'allowedHeroes' + 'tasks' + 'helpVideos' + 'campaign' + 'campaignIndex' + 'replayable' + 'buildTime' + 'scoreTypes' + 'concepts' + 'picoCTFProblem' + 'practiceThresholdMinutes', + 'primerLanguage' + 'studentPlayInstructions' +] + module.exports = Level = mongoose.model('level', LevelSchema) diff --git a/server/routes/index.coffee b/server/routes/index.coffee index 434e3d568..b2ab4ba2e 100644 --- a/server/routes/index.coffee +++ b/server/routes/index.coffee @@ -96,7 +96,11 @@ module.exports.setup = (app) -> app.post('/db/course_instance/:handle/members', mw.auth.checkLoggedIn(), mw.courseInstances.addMembers) app.get('/db/course_instance/:handle/classroom', mw.auth.checkLoggedIn(), mw.courseInstances.fetchClassroom) app.get('/db/course_instance/:handle/course', mw.auth.checkLoggedIn(), mw.courseInstances.fetchCourse) - + + Level = require '../models/Level' + app.post('/db/level/:handle', mw.auth.checkLoggedIn(), mw.versions.postNewVersion(Level, { hasPermissionsOrTranslations: 'artisan' })) # TODO: add /new-version to route like Article has + app.get('/db/level/:handle/session', mw.auth.checkHasUser(), mw.levels.upsertSession) + app.put('/db/user/:handle', mw.users.resetEmailVerifiedFlag) app.delete('/db/user/:handle', mw.users.removeFromClassrooms) app.get('/db/user', mw.users.fetchByGPlusID, mw.users.fetchByFacebookID) @@ -104,7 +108,6 @@ module.exports.setup = (app) -> app.put('/db/user/-/remain-teacher', mw.users.remainTeacher) app.post('/db/user/:userID/request-verify-email', mw.users.sendVerificationEmail) app.post('/db/user/:userID/verify/:verificationCode', mw.users.verifyEmailAddress) # TODO: Finalize URL scheme - app.get('/db/level/:handle/session', mw.auth.checkHasUser(), mw.levels.upsertSession) app.get('/db/user/-/students', mw.auth.checkHasPermission(['admin']), mw.users.getStudents) app.get('/db/user/-/teachers', mw.auth.checkHasPermission(['admin']), mw.users.getTeachers) app.post('/db/user/:handle/signup-with-facebook', mw.users.signupWithFacebook) diff --git a/spec/server/functional/level.spec.coffee b/spec/server/functional/level.spec.coffee index 774956dc0..273468e95 100644 --- a/spec/server/functional/level.spec.coffee +++ b/spec/server/functional/level.spec.coffee @@ -52,9 +52,40 @@ describe 'POST /db/level/:handle', -> url = getURL("/db/level/#{@level.id}") [res, body] = yield request.postAsync({url: url, json: levelJSON}) - expect(res.statusCode).toBe(200) + expect(res.statusCode).toBe(201) + done() + + it 'does not break the target level if a name change would conflict with another level', utils.wrap (done) -> + yield utils.clearModels([Level, User]) + user = yield utils.initUser() + yield utils.loginUser(user) + yield utils.makeLevel({name: 'Taken Name'}) + level = yield utils.makeLevel({name: 'Another Level'}) + json = _.extend({}, level.toObject(), {name: 'Taken Name'}) + [res, body] = yield request.postAsync({url: utils.getURL("/db/level/#{level.id}"), json}) + expect(res.statusCode).toBe(409) + level = yield Level.findById(level.id) + # should be unchanged + expect(level.get('slug')).toBe('another-level') + expect(level.get('version').isLatestMinor).toBe(true) + expect(level.get('version').isLatestMajor).toBe(true) + expect(level.get('index')).toBeDefined() done() + it 'enforces permissions', -> + yield utils.clearModels([Level, User]) + user = yield utils.initUser() + yield utils.loginUser(user) + level = yield utils.makeLevel({description:'Original desc'}) + + otherUser = yield utils.initUser() + yield utils.loginUser(otherUser) + json = _.extend({}, level.toObject(), {description: 'Trollin'}) + [res, body] = yield request.postAsync({url: utils.getURL("/db/level/#{level.id}"), json}) + expect(res.statusCode).toBe(403) + level = yield Level.findById(level.id) + expect(level.get('description')).toBe('Original desc') + done() describe 'GET /db/level/:handle/session', ->