Patch fixes

* Partially revert GET /db/:collection/:handle/patches, as it returned limited results for versioned docs
* Fix POST /db/:collection/:handle/patch to:
   * normalize diffs based on latest doc version
   * handle empty deltas
   * not swallow thrown errors due to _.curry
   * Set 'target.original' correctly for versioned collections

scripts/mongodb/migrations/2016-09-16-fix-patch-originals.js

@@ -0,0 +1,24 @@
+// Usage: Paste into mongodb client
+
+VERSIONED_COLLECTIONS = {
+  'article': db.articles,
+  'level': db.levels,
+  'level_component': db.level.components,
+  'level_system': db.level.systems,
+  'thang_type': db.thang.types
+};
+
+db.patches.find({created:{$gt: '2016-09-08'}}).forEach(function(patch) {
+  if (!VERSIONED_COLLECTIONS[patch.target.collection]) {
+    print('skip', patch.target.collection);
+    return;
+  }
+  if (patch.target.original && patch.target.id && patch.target.original.equals(patch.target.id)) {
+    target = VERSIONED_COLLECTIONS[patch.target.collection].findOne({_id: patch.target.original});
+    print('found original', target, target._id, target.original);
+    db.patches.update({_id: patch._id}, {$set: {'target.original': target.original}});
+  }
+  else {
+    print('They are different, they are fine');
+  }
+});

server/commons/database.coffee

@@ -6,6 +6,7 @@ mongooseCache = require 'mongoose-cache'
 errors = require '../commons/errors'
 Promise = require 'bluebird'
 _ = require 'lodash'
+co = require 'co'
 
 module.exports =
   isID: (id) -> _.isString(id) and id.length is 24 and id.match(/[a-f0-9]/gi)?.length is 24
@@ -160,11 +161,7 @@ module.exports =
       throw new errors.UnprocessableEntity('JSON-schema validation failed', { validationErrors: result.errors })
 
 
-  getDocFromHandle: Promise.promisify (req, Model, options, done) ->
-    if _.isFunction(options)
-      done = options
-      options = {}
-
+  getDocFromHandle: co.wrap (req, Model, options={}) ->
     dbq = Model.find()
     handle = req.params.handle
     if not handle
@@ -177,7 +174,11 @@ module.exports =
     if options.select
       dbq.select(options.select)
 
-    dbq.exec(done)
+    doc = yield dbq.exec()
+    if options.getLatest and Model.schema.uses_coco_versions and doc and not doc.get('version.isLatestMajor')
+      original = doc.get('original')
+      doc = yield Model.findOne({original}).sort({ 'version.major': -1, 'version.minor': -1 })
+    return doc
 
 
   hasAccessToDocument: (req, doc, method) ->

server/middleware/patchable.coffee

@@ -21,14 +21,18 @@ module.exports =
     dbq.skip(parse.getSkipFromReq(req))
     dbq.select(parse.getProjectFromReq(req))
 
-    doc = yield database.getDocFromHandle(req, Model, {_id: 1})
-    if not doc
-      throw new errors.NotFound('Patchable document not found')
-      
+    id = req.params.handle
+    if not database.isID(id)
+      # handle slug
+      doc = yield database.getDocFromHandle(req, Model, {_id: 1})
+      if not doc
+        throw new errors.NotFound('Patchable document not found')
+      id = (doc.get('original') or doc.id) + ''
+
     query =
       $or: [
-        {'target.original': doc.id }
-        {'target.original': doc._id }
+        {'target.original': id+''}
+        {'target.original': mongoose.Types.ObjectId(id)}
       ]
     if req.query.status
       query.status = req.query.status
@@ -65,12 +69,16 @@ module.exports =
     res.status(200).send(doc)
 
     
-  postPatch: _.curry wrap (Model, collectionName, req, res) ->
+  postPatch: (Model, collectionName) -> wrap (req, res) ->
     # handle either "POST /db/<collection>/:handle/patch" or "POST /db/patch" with target included in body
+    # Tried currying the function, but it didn't play nice with the generator function.
     if req.params.handle
-      target = yield database.getDocFromHandle(req, Model)
+      target = yield database.getDocFromHandle(req, Model, {getLatest:true})
     else if req.body.target?.id
       target = yield Model.findById(req.body.target.id)
+      if Model.schema.uses_coco_versions and target and not target.get('version.isLatestMajor')
+        original = target.get('original')
+        target = yield Model.findOne({original}).sort({ 'version.major': -1, 'version.minor': -1 })
     if not target
       throw new errors.NotFound('Target not found.')
 
@@ -84,9 +92,13 @@ module.exports =
       return value if value instanceof Date
       return undefined
     )
+    if _.isEmpty(originalDelta)
+      throw new errors.UnprocessableEntity('Delta given is empty.')
     jsondiffpatch.patch(newTargetAttrs, originalDelta)
     normalizedDelta = jsondiffpatch.diff(originalTarget, newTargetAttrs)
     normalizedDelta = _.pick(normalizedDelta, _.keys(originalDelta))
+    if _.isEmpty(normalizedDelta)
+      throw new errors.UnprocessableEntity('Normalized delta is empty.')
 
     # decide whether the patch should be auto-accepted, or left 'pending' for an admin or artisan to review
     reasonNotAutoAccepted = undefined
@@ -120,7 +132,7 @@ module.exports =
       patchTarget = {
         collection: collectionName
         id: target._id
-        original: target._id
+        original: target.get('original')
         version: _.pick(target.get('version'), 'major', 'minor')
       }
     else

server/middleware/patches.coffee

@@ -33,7 +33,7 @@ module.exports.post = wrap (req, res) ->
     throw new errors.UnprocessableEntity("#{collection} is not patchable")
     
   # pass to logic shared with "POST /db/:collection/:handle/patch"
-  yield postPatch(Model, collection, req, res)
+  yield postPatch(Model, collection)(req, res)
 
 
 # Allow patch submitters to withdraw their patches, or admins/artisans to accept/reject others' patches

spec/server/functional/level.spec.coffee

@@ -243,3 +243,63 @@ describe 'POST /db/level/names', ->
     aLevel = levels[2]
     expect(_.find(body, (l) -> l._id is aLevel.id).name).toBe(aLevel.get('name'))
     done()
+
+    
+describe 'POST /db/level/:handle/patch', ->
+  
+  fit 'accepts the patch based on the latest version, not the version given', utils.wrap (done) ->
+    user = yield utils.initUser()
+    yield utils.loginUser(user)
+    level = yield utils.makeLevel()
+    original = level.toObject()
+    changed = _.clone(original)
+    changed.i18n = {'de': {name:'German translation #1'}}
+    delta = jsondiffpatch.diff(original, changed)
+    
+    json = { 
+      delta: delta
+      target: {
+        collection: 'level'
+        id: level.id
+      }
+      commitMessage: 'Server test commit'
+    }
+    url = utils.getURL("/db/level/#{level.id}/patch")
+    [res, body] = yield request.postAsync({url, json})
+    expect(res.body.status).toBe('accepted')
+    
+    changed = _.clone(original)
+    changed.i18n = {'de': {name:'German translation #2'}}
+    delta = jsondiffpatch.diff(original, changed)
+    json.delta = delta
+    
+    [res, body] = yield request.postAsync({url, json})
+    expect(res.body.status).toBe('pending')
+    expect(res.body.target.original).toBe(level.get('original').toString())
+    done()
+    
+  it 'throws an error if there would be no change', utils.wrap (done) ->
+    user = yield utils.initUser()
+    yield utils.loginUser(user)
+    level = yield utils.makeLevel()
+    original = level.toObject()
+    changed = _.clone(original)
+    changed.i18n = {'de': {name:'German translation #1'}}
+    delta = jsondiffpatch.diff(original, changed)
+
+    json = {
+      delta: delta
+      target: {
+        collection: 'level'
+        id: level.id
+      }
+      commitMessage: 'Server test commit'
+    }
+    url = utils.getURL("/db/level/#{level.id}/patch")
+    [res, body] = yield request.postAsync({url, json})
+    expect(res.body.status).toBe('accepted')
+
+    # repeat request
+    [res, body] = yield request.postAsync({url, json})
+    expect(res.statusCode).toBe(422)
+    done()

spec/server/functional/patch.spec.coffee

@@ -47,6 +47,15 @@ describe 'POST /db/patch', ->
     expect(article.get('patches').length).toBe(1)
     done()
 
+  it 'is always based on the latest document', utils.wrap (done) ->
+    @json.delta = {i18n: [{de: {name:'German translation'}}]}
+    [res, body] = yield request.postAsync { @url, @json }
+    expect(res.statusCode).toBe(201)
+    expect(res.body.status).toBe('accepted')
+    [res, body] = yield request.postAsync { @url, @json }
+    expect(res.statusCode).toBe(422) # should be a no-change
+    done()
+
   it 'shows up in patch requests', utils.wrap (done) ->
     [res, body] = yield request.postAsync { @url, @json }
     patchID = res.body._id
@@ -126,6 +135,7 @@ describe 'PUT /db/patch/:handle/status', ->
     [res, body] = yield request.putAsync {@url, json: {status: 'withdrawn'}}
     expect(res.statusCode).toBe(200)
     expect(body.status).toBe('withdrawn')
+    yield new Promise((resolve) -> setTimeout(resolve, 50))
     article = yield Article.findById(@article.id)
     expect(article.get('patches').length).toBe(0)
     done()