Fixed OT behaviour of client when ops are rejected

src/client/client.coffee

@@ -52,9 +52,12 @@ Doc = (connection, @name, @version, @type, snapshot) ->
 	# Some recent ops, incase submitOp is called with an old op version number.
 	serverOps = {}
 
-	# Listeners for the document changing
-	listeners = []
-
+	# Transform a server op by a client op, and vice versa.
+	xf = @type['transformX'] or (client, server) =>
+		client_ = @type['transform'] client, server, 'left'
+		server_ = @type['transform'] server, client, 'right'
+		return [client_, server_]
+
 	tryFlushPendingOp = =>
 		if inflightOp == null && pendingOp != null
 			# Rotate null -> pending -> inflight, 
@@ -65,6 +68,9 @@ Doc = (connection, @name, @version, @type, snapshot) ->
 			pendingCallbacks = []
 
 			connection.send {'doc':@name, 'op':inflightOp, 'v':@version}, (response, error) =>
+				oldInflightOp = inflightOp
+				inflightOp = null
+
 				if error
 					# This will happen if the server rejects edits from the client.
 					# We'll send the error message to the user and roll back the change.
@@ -74,27 +80,24 @@ Doc = (connection, @name, @version, @type, snapshot) ->
 
 					if type['invert']
 
-						undo = @type['invert'] inflightOp
-						undo = @type['transform'] undo, pendingOp, 'left' if pendingOp
+						undo = @type['invert'] oldInflightOp
 
-						#console.warn "if #{JSON.stringify(inflightOp)} undo #{JSON.stringify(undo)} pending #{JSON.stringify(pendingOp)}"
-						#console.warn "snapshot", JSON.stringify(@snapshot)
+						if pendingOp
+							[pendingOp, undo] = xf pendingOp, undo
 
 						# Now we have to transform the undo operation by any server ops & pending ops
 						setSnapshot @type['apply'](@snapshot, undo)
 					else
 						throw new Error "Op apply failed (#{response['error']}) and the OT type does not define an invert function."
 
 					callback(null, error) for callback in inflightCallbacks
-					#throw new Error(response['error'])
 				else
 					throw new Error('Invalid version from server') unless response['v'] == @version
 
-					serverOps[@version] = inflightOp
+					serverOps[@version] = oldInflightOp
 					@version++
-					callback(inflightOp, null) for callback in inflightCallbacks
+					callback(oldInflightOp, null) for callback in inflightCallbacks
 
-				inflightOp = null
 				tryFlushPendingOp()
 
 	# Internal - do not call directly.
@@ -115,12 +118,6 @@ Doc = (connection, @name, @version, @type, snapshot) ->
 		op = msg['op']
 		serverOps[@version] = op
 
-		# Transform a server op by a client op, and vice versa.
-		xf = @type['transformX'] or (client, server) =>
-			client_ = @type['transform'] client, server, 'left'
-			server_ = @type['transform'] server, client, 'right'
-			return [client_, server_]
-
 		docOp = op
 		if inflightOp != null
 			[inflightOp, docOp] = xf inflightOp, docOp

test/client.coffee

@@ -8,7 +8,7 @@ types = require '../src/types'
 nativeclient = require '../src/client'
 webclient = require './helpers/webclient'
 
-makePassPart = require('./helpers').makePassPart
+{makePassPart, expectCalls} = require('./helpers')
 
 genTests = (client) ->
 	testCase
@@ -125,6 +125,15 @@ genTests = (client) ->
 				test.strictEqual doc.snapshot, 'hi'
 				# ... but the version tracks the server version, so thats still 0.
 				test.strictEqual doc.version, 0
+
+		'submit an op to a document using the API works': (test) ->
+			@c.open @name, 'text', (doc, error) =>
+				doc.insert 'hi', 0, =>
+					test.strictEqual doc.snapshot, 'hi'
+					test.strictEqual doc.getText(), 'hi'
+					@model.getSnapshot @name, ({snapshot}) ->
+						test.strictEqual snapshot, 'hi'
+						test.done()
 
 		'submitting an op while another op is inflight works': (test) ->
 			@c.open @name, 'text', (doc, error) =>
@@ -330,10 +339,7 @@ genTests = (client) ->
 
 		"Can't submit an op if auth rejects you": (test) ->
 			@auth = (client, action) ->
-				if action.name == 'submit op' and action.op[0].i == 'hi'
-					action.reject()
-				else
-					action.accept()
+				if action.name == 'submit op' then action.reject() else action.accept()
 
 			@c.open @name, 'text', (doc, error) =>
 				doc.insert 'hi', 0, (op, error) =>
@@ -342,10 +348,44 @@ genTests = (client) ->
 					# Also need to test that ops sent afterwards get sent correctly.
 					# because that behaviour IS CURRENTLY BROKEN
 
-					@model.getSnapshot @name, (data) ->
-						test.strictEqual data.snapshot, ''
+					@model.getSnapshot @name, ({snapshot}) ->
+						test.strictEqual snapshot, ''
 						test.done()
 
+		'If auth rejects your op, other transforms work correctly': (test) ->
+			# This should probably have a randomized tester as well.
+			@auth = (client, action) ->
+				if action.name == 'submit op' and action.op[0].d == 'cC'
+					action.reject()
+				else
+					action.accept()
+
+			@c.open @name, 'text', (doc, error) =>
+				doc.insert 'abcCBA', 0, =>
+					e = expectCalls 3, =>
+						# The b's are successfully deleted, the ds are added by the server and the
+						# op to delete the cs is denied.
+						@model.getSnapshot @name, ({snapshot}) ->
+							test.deepEqual snapshot, 'acdDCA'
+							test.done()
+
+					doc.del 2, 2, (op, error) => # Delete the 'cC', so the document becomes 'abBA'
+						# This op is denied by the auth code
+						test.strictEqual error, 'forbidden'
+						e()
+
+					test.strictEqual doc.getText(), 'abBA'
+					doc.flush()
+
+					# Simultaneously, we'll apply another op locally:
+					doc.del 2, 1, -> # Delete the 'bB'
+						e()
+					test.strictEqual doc.getText(), 'aA'
+
+					# ... and yet another op on the server. (Remember, the server hasn't seen either op yet.)
+					@model.applyOp @name, {op:[{i:'dD', p:3}], v:1, meta:{}}, =>
+						@model.getSnapshot @name, e
+
 		'Text API is advertised': (test) ->
 			@c.open @name, 'text', (doc, error) ->
 				test.strictEqual doc.provides?.text, true
@@ -396,6 +436,5 @@ genTests = (client) ->
 	#			test.strictEqual doc.created, true
 	#			test.done()
 
-
 exports.native = genTests nativeclient
 exports.webclient = genTests webclient

test/helpers/misc.coffee

@@ -36,15 +36,19 @@ exports.transformLists = (type, serverOps, clientOps) ->
 # Compose a list of ops together
 exports.composeList = (type, ops) -> ops.reduce type.compose
 
-# Returns a function that calls test.done() after it has been called n times
-exports.makePassPart = (test, n) ->
+# Wait for the function to be called a given number of times, then call the callback.
+exports.expectCalls = expectCalls = (n, callback) ->
 	remaining = n
 	->
 		remaining--
 		if remaining == 0
-			test.done()
+			callback()
 		else if remaining < 0
-			throw new Error "passPart() called more than #{n} times"
+			throw new Error "expectCalls called more than #{n} times"
+
+# Returns a function that calls test.done() after it has been called n times
+exports.makePassPart = (test, n) ->
+	expectCalls n, -> test.done()
 
 # Callback will be called after all the ops have been applied, with the
 # resultant snapshot. Callback format is callback(error, snapshot)

test/integration.coffee

@@ -119,3 +119,5 @@ module.exports = testCase
 
 			testSome()
 
+	# TODO: Add a randomized tester which also randomly denies ops.
+