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

@@ -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'

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: [
+  editableProperties: Level.editableProperties
-    'description'
+  postEditableProperties: Level.postEditableProperties
-    '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']
 
   getByRelationship: (req, res, args...) ->
     return @getSession(req, res, args[0]) if args[1] is 'session'

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()
 
-    if not latest
+    # Handle the case where no version is marked as latest, since making new
-      # handle the case where no version is marked as latest, since making new
     # versions is not atomic
+    if not latest
       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
-
+    raw = yield latest.update({$set: {version: version}, $unset: {index: 1, slug: 1}})
-    conditions = {_id: latest._id}
-
-    raw = yield Model.update(conditions, {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)
 
+    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}"

server/models/Achievement.coffee

@@ -93,6 +93,7 @@ AchievementSchema.statics.editableProperties = [
   'i18nCoverage'
   'hidden'
 ]
+AchievementSchema.statics.postEditableProperties = []
 
 AchievementSchema.statics.jsonSchema = require '../../app/schemas/models/achievement'
 

server/models/Campaign.coffee

@@ -56,5 +56,6 @@ CampaignSchema.statics.editableProperties = [
   'adjacentCampaigns'
   'levels'
 ]
+CampaignSchema.statics.postEditableProperties = []
 
 module.exports = mongoose.model('campaign', CampaignSchema)

server/models/Classroom.coffee

@@ -23,6 +23,7 @@ ClassroomSchema.statics.editableProperties = [
   'ageRangeMax'
   'archived'
 ]
+ClassroomSchema.statics.postEditableProperties = []
 
 ClassroomSchema.statics.generateNewCode = (done) ->
   tryCode = ->

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'
 

server/models/Level.coffee

@@ -46,4 +46,64 @@ 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)

server/routes/index.coffee

@@ -97,6 +97,10 @@ module.exports.setup = (app) ->
   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)

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', ->