Merge branch 'master' into sk-upgrade-metrics

services/web/.nvmrc

@@ -0,0 +1 @@
+0.10.22

services/web/app/coffee/Features/Analytics/AnalyticsController.coffee

@@ -1,7 +1,15 @@
 AnalyticsManager = require "./AnalyticsManager"
+Errors = require "../Errors/Errors"
+AuthenticationController = require("../Authentication/AuthenticationController")
 
 module.exports = AnalyticsController =
 	recordEvent: (req, res, next) ->
-		AnalyticsManager.recordEvent req.session?.user?._id, req.params.event, req.body, (error) ->
-			return next(error) if error?
-			res.send 204
+		user_id = AuthenticationController.getLoggedInUserId(req) or req.sessionID
+		AnalyticsManager.recordEvent user_id, req.params.event, req.body, (error) ->
+			if error instanceof Errors.ServiceNotConfiguredError
+				# ignore, no-op
+				return res.send(204)
+			else if error?
+				return next(error)
+			else
+				return res.send 204

services/web/app/coffee/Features/Analytics/AnalyticsManager.coffee

@@ -2,6 +2,7 @@ settings = require "settings-sharelatex"
 logger = require "logger-sharelatex"
 _ = require "underscore"
 request = require "request"
+Errors = require '../Errors/Errors'
 
 
 makeRequest = (opts, callback)->
@@ -10,12 +11,20 @@ makeRequest = (opts, callback)->
 		opts.url = "#{settings.apis.analytics.url}#{urlPath}"
 		request opts, callback
 	else
-		callback()
-
+		callback(new Errors.ServiceNotConfiguredError('Analytics service not configured'))
 
 
 module.exports =
 
+	identifyUser: (user_id, old_user_id, callback = (error)->)->
+		opts =
+			body:
+				old_user_id:old_user_id
+			json:true
+			method:"POST"
+			timeout:1000
+			url: "/user/#{user_id}/identify"
+		makeRequest opts, callback
 
 	recordEvent: (user_id, event, segmentation = {}, callback = (error) ->) ->
 		if user_id+"" == settings.smokeTest?.userId+""

services/web/app/coffee/Features/Authentication/AuthenticationController.coffee

@@ -87,6 +87,7 @@ module.exports = AuthenticationController =
 					LoginRateLimiter.recordSuccessfulLogin(email)
 					AuthenticationController._recordSuccessfulLogin(user._id)
 					Analytics.recordEvent(user._id, "user-logged-in", {ip:req.ip})
+					Analytics.identifyUser(user._id, req.sessionID)
 					logger.log email: email, user_id: user._id.toString(), "successful log in"
 					req.session.justLoggedIn = true
 					# capture the request ip for use when creating the session

services/web/app/coffee/Features/Authorization/AuthorizationManager.coffee

@@ -4,6 +4,8 @@ User = require("../../models/User").User
 PrivilegeLevels = require("./PrivilegeLevels")
 PublicAccessLevels = require("./PublicAccessLevels")
 Errors = require("../Errors/Errors")
+ObjectId = require("mongojs").ObjectId
+
 
 module.exports = AuthorizationManager =
 	# Get the privilege level that the user has for the project
@@ -13,6 +15,8 @@ module.exports = AuthorizationManager =
 	#   * becausePublic: true if the access level is only because the project is public.
 	getPrivilegeLevelForProject: (user_id, project_id, callback = (error, privilegeLevel, becausePublic) ->) ->
 		getPublicAccessLevel = () ->
+			if !ObjectId.isValid(project_id)
+				return callback(new Error("invalid project id"))
 			Project.findOne { _id: project_id }, { publicAccesLevel: 1 }, (error, project) ->
 				return callback(error) if error?
 				if !project?

services/web/app/coffee/Features/Blog/BlogController.coffee

@@ -10,7 +10,7 @@ module.exports = BlogController =
 		url = req.url?.toLowerCase()
 		blogUrl = "#{settings.apis.blog.url}#{url}"
 
-		extensionsToProxy = [".png", ".xml", ".jpeg", ".json", ".zip", ".eps", ".gif"]
+		extensionsToProxy = [".png", ".xml", ".jpeg", ".jpg", ".json", ".zip", ".eps", ".gif"]
 
 		shouldProxy = _.find extensionsToProxy, (extension)->
 			url.indexOf(extension) != -1
@@ -42,4 +42,4 @@ module.exports = BlogController =
 		upstream = request.get(originUrl)
 		upstream.on "error", (error) ->
 			logger.error err: error, "blog proxy error"
-		upstream.pipe res
+		upstream.pipe res

services/web/app/coffee/Features/Email/EmailSender.coffee

@@ -72,6 +72,7 @@ module.exports =
 			client.sendMail options, (err, res)->
 				if err?
 					logger.err err:err, "error sending message"
+					err = new Error('Cannot send email')
 				else
 					logger.log "Message sent to #{options.to}"
 				callback(err)

services/web/app/coffee/Features/Errors/Errors.coffee

@@ -5,5 +5,14 @@ NotFoundError = (message) ->
 	return error
 NotFoundError.prototype.__proto__ = Error.prototype
 
+
+ServiceNotConfiguredError = (message) ->
+	error = new Error(message)
+	error.name = "ServiceNotConfiguredError"
+	error.__proto__ = ServiceNotConfiguredError.prototype
+	return error
+
+
 module.exports = Errors =
-	NotFoundError: NotFoundError
+	NotFoundError: NotFoundError
+	ServiceNotConfiguredError: ServiceNotConfiguredError

services/web/app/coffee/Features/FileStore/FileStoreHandler.coffee

@@ -13,6 +13,9 @@ module.exports = FileStoreHandler =
 			if err?
 				logger.err err:err, project_id:project_id, file_id:file_id, fsPath:fsPath, "error stating file"
 				callback(err)
+			if !stat?
+				logger.err project_id:project_id, file_id:file_id, fsPath:fsPath, "stat is not available, can not check file from disk"
+				return callback(new Error("error getting stat, not available"))
 			if !stat.isFile()
 				logger.log project_id:project_id, file_id:file_id, fsPath:fsPath, "tried to upload symlink, not contining"
 				return callback(new Error("can not upload symlink"))

services/web/app/coffee/Features/Project/ProjectApiController.coffee

@@ -4,11 +4,9 @@ logger = require("logger-sharelatex")
 
 module.exports = 
 
-	getProjectDetails : (req, res)->
+	getProjectDetails : (req, res, next)->
 		{project_id} = req.params
 		ProjectDetailsHandler.getDetails project_id, (err, projDetails)->
-			if err?
-				logger.log err:err, project_id:project_id, "something went wrong getting project details"
-				return res.sendStatus 500
+			return next(err) if err?
 			res.json(projDetails)
 

services/web/app/coffee/Features/Project/ProjectDetailsHandler.coffee

@@ -5,6 +5,7 @@ logger = require("logger-sharelatex")
 tpdsUpdateSender = require '../ThirdPartyDataStore/TpdsUpdateSender'
 _ = require("underscore")
 PublicAccessLevels = require("../Authorization/PublicAccessLevels")
+Errors = require("../Errors/Errors")
 
 module.exports = 
 
@@ -13,6 +14,7 @@ module.exports =
 			if err?
 				logger.err err:err, project_id:project_id, "error getting project"
 				return callback(err)
+			return callback(new Errors.NotFoundError("project not found")) if !project?
 			UserGetter.getUser project.owner_ref, (err, user) ->
 				return callback(err) if err?
 				details =

services/web/app/coffee/Features/Project/ProjectDuplicator.coffee

@@ -15,9 +15,11 @@ module.exports = ProjectDuplicator =
 	_copyDocs: (newProject, originalRootDoc, originalFolder, desFolder, docContents, callback)->
 		setRootDoc = _.once (doc_id)->
 			projectEntityHandler.setRootDoc newProject._id, doc_id
-
-		jobs = originalFolder.docs.map (doc)->
+		docs = originalFolder.docs or []
+		jobs = docs.map (doc)->
 			return (cb)->
+				if !doc?._id?
+					return callback()
 				content = docContents[doc._id.toString()]
 				projectEntityHandler.addDocWithProject newProject, desFolder._id, doc.name, content.lines, (err, newDoc)->
 					if err?
@@ -30,7 +32,8 @@ module.exports = ProjectDuplicator =
 		async.series jobs, callback
 
 	_copyFiles: (newProject, originalProject_id, originalFolder, desFolder, callback)->
-		jobs = originalFolder.fileRefs.map (file)->
+		fileRefs = originalFolder.fileRefs or []
+		jobs = fileRefs.map (file)->
 			return (cb)->
 				projectEntityHandler.copyFileFromExistingProjectWithProject newProject, desFolder._id, originalProject_id, file, cb
 		async.parallelLimit jobs, 5, callback
@@ -40,10 +43,14 @@ module.exports = ProjectDuplicator =
 		ProjectGetter.getProject newProject_id, {rootFolder:true, name:true}, (err, newProject)->
 			if err?
 				logger.err project_id:newProject_id, "could not get project"
-				return cb(err)
+				return callback(err)
 
-			jobs = originalFolder.folders.map (childFolder)->
+			folders = originalFolder.folders or []
+
+			jobs = folders.map (childFolder)->
 				return (cb)->
+					if !childFolder?._id?
+						return cb()
 					projectEntityHandler.addFolderWithProject newProject, desFolder?._id, childFolder.name, (err, newFolder)->
 						return cb(err) if err?
 						ProjectDuplicator._copyFolderRecursivly newProject_id, originalProject_id, originalRootDoc, childFolder, newFolder, docContents, cb

services/web/app/coffee/Features/Project/ProjectLocator.coffee

@@ -26,6 +26,8 @@ module.exports = ProjectLocator =
 			element = _.find searchFolder[elementType], (el)-> el?._id+'' == element_id+'' #need to ToString both id's for robustness
 			if !element? && searchFolder.folders? && searchFolder.folders.length != 0
 				_.each searchFolder.folders, (folder, index)->
+					if !folder?
+						return
 					newPath = {}
 					newPath[key] = value for own key,value of path #make a value copy of the string
 					newPath.fileSystem += "/#{folder.name}"

services/web/app/coffee/Features/StaticPages/StaticPagesRouter.coffee

@@ -9,6 +9,7 @@ module.exports =
 
 		webRouter.get '/tos', HomeController.externalPage("tos", "Terms of Service")
 		webRouter.get '/about', HomeController.externalPage("about", "About Us")
+
 		webRouter.get '/security', HomeController.externalPage("security", "Security")
 		webRouter.get '/privacy_policy', HomeController.externalPage("privacy", "Privacy Policy")
 		webRouter.get '/planned_maintenance', HomeController.externalPage("planned_maintenance", "Planned Maintenance")
@@ -20,4 +21,4 @@ module.exports =
 		webRouter.get '/dropbox', HomeController.externalPage("dropbox", "Dropbox and ShareLaTeX")
 
 		webRouter.get '/university', UniversityController.getIndexPage
-		webRouter.get '/university/*', UniversityController.getPage
+		webRouter.get '/university/*', UniversityController.getPage

services/web/app/coffee/Features/Subscription/RecurlyWrapper.coffee

@@ -469,33 +469,39 @@ module.exports = RecurlyWrapper =
 				logger.err err:error,  subscriptionId:subscriptionId, daysUntilExpire:daysUntilExpire,  "error exending trial"
 			callback(error)
 		)
+	
+	listAccountActiveSubscriptions: (account_id, callback = (error, subscriptions) ->) ->
+		RecurlyWrapper.apiRequest {
+			url: "accounts/#{account_id}/subscriptions"
+			qs:
+				state: "active"
+			expect404: true
+		}, (error, response, body) ->
+			return callback(error) if error?
+			if response.statusCode == 404
+				return callback null, []
+			else
+				RecurlyWrapper._parseSubscriptionsXml body, callback
+
+	_parseSubscriptionsXml: (xml, callback) ->
+		RecurlyWrapper._parseXmlAndGetAttribute xml, "subscriptions", callback
 
 	_parseSubscriptionXml: (xml, callback) ->
-		RecurlyWrapper._parseXml xml, (error, data) ->
-			return callback(error) if error?
-			if data? and data.subscription?
-				recurlySubscription = data.subscription
-			else
-				return callback "I don't understand the response from Recurly"
-			callback null, recurlySubscription
+		RecurlyWrapper._parseXmlAndGetAttribute xml, "subscription", callback
 
 	_parseAccountXml: (xml, callback) ->
-		RecurlyWrapper._parseXml xml, (error, data) ->
-			return callback(error) if error?
-			if data? and data.account?
-				account = data.account
-			else
-				return callback "I don't understand the response from Recurly"
-			callback null, account
+		RecurlyWrapper._parseXmlAndGetAttribute xml, "account", callback
 
 	_parseBillingInfoXml: (xml, callback) ->
+		RecurlyWrapper._parseXmlAndGetAttribute xml, "billing_info", callback
+
+	_parseXmlAndGetAttribute: (xml, attribute, callback) ->
 		RecurlyWrapper._parseXml xml, (error, data) ->
 			return callback(error) if error?
-			if data? and data.billing_info?
-				billingInfo = data.billing_info
+			if data? and data[attribute]?
+				return callback null, data[attribute]
 			else
-				return callback "I don't understand the response from Recurly"
-			callback null, billingInfo
+				return callback(new Error("I don't understand the response from Recurly"))
 
 	_parseXml: (xml, callback) ->
 		convertDataTypes = (data) ->

services/web/app/coffee/Features/Subscription/SubscriptionController.coffee

@@ -46,31 +46,39 @@ module.exports = SubscriptionController =
 			if hasSubscription or !plan?
 				res.redirect "/user/subscription"
 			else
-				currency = req.query.currency?.toUpperCase()
-				GeoIpLookup.getCurrencyCode req.query?.ip || req.ip, (err, recomendedCurrency, countryCode)->
-					return next(err) if err?
-					if recomendedCurrency? and !currency?
-						currency = recomendedCurrency
-					RecurlyWrapper.sign {
-						subscription:
-							plan_code : req.query.planCode
-							currency: currency
-						account_code: user._id
-					}, (error, signature) ->
-						return next(error) if error?
-						res.render "subscriptions/new",
-							title      : "subscribe"
-							plan_code: req.query.planCode
-							currency: currency
-							countryCode:countryCode
-							plan:plan
-							showStudentPlan: req.query.ssp
-							recurlyConfig: JSON.stringify
-								currency: currency
-								subdomain: Settings.apis.recurly.subdomain
-							showCouponField: req.query.scf
-							showVatField: req.query.svf
-							couponCode:      req.query.cc or ""
+				# LimitationsManager.userHasSubscription only checks Mongo. Double check with
+				# Recurly as well at this point (we don't do this most places for speed).
+				SubscriptionHandler.validateNoSubscriptionInRecurly user._id, (error, valid) ->
+					return next(error) if error?
+					if !valid
+						res.redirect "/user/subscription"
+						return
+					else
+						currency = req.query.currency?.toUpperCase()
+						GeoIpLookup.getCurrencyCode req.query?.ip || req.ip, (err, recomendedCurrency, countryCode)->
+							return next(err) if err?
+							if recomendedCurrency? and !currency?
+								currency = recomendedCurrency
+							RecurlyWrapper.sign {
+								subscription:
+									plan_code : req.query.planCode
+									currency: currency
+								account_code: user._id
+							}, (error, signature) ->
+								return next(error) if error?
+								res.render "subscriptions/new",
+									title      : "subscribe"
+									plan_code: req.query.planCode
+									currency: currency
+									countryCode:countryCode
+									plan:plan
+									showStudentPlan: req.query.ssp
+									recurlyConfig: JSON.stringify
+										currency: currency
+										subdomain: Settings.apis.recurly.subdomain
+									showCouponField: req.query.scf
+									showVatField: req.query.svf
+									couponCode:      req.query.cc or ""
 
 
 

services/web/app/coffee/Features/Subscription/SubscriptionHandler.coffee

@@ -11,15 +11,28 @@ Analytics = require("../Analytics/AnalyticsManager")
 
 
 module.exports =
+	validateNoSubscriptionInRecurly: (user_id, callback = (error, valid) ->) ->
+		RecurlyWrapper.listAccountActiveSubscriptions user_id, (error, subscriptions = []) ->
+			return callback(error) if error?
+			if subscriptions.length > 0
+				SubscriptionUpdater.syncSubscription subscriptions[0], user_id, (error) ->
+					return callback(error) if error?
+					return callback(null, false)
+			else
+				return callback(null, true)
 
 	createSubscription: (user, subscriptionDetails, recurly_token_id, callback)->
 		self = @
 		clientTokenId = ""
-		RecurlyWrapper.createSubscription user, subscriptionDetails, recurly_token_id, (error, recurlySubscription)->
+		@validateNoSubscriptionInRecurly user._id, (error, valid) ->
 			return callback(error) if error?
-			SubscriptionUpdater.syncSubscription recurlySubscription, user._id, (error) ->
+			if !valid
+				return callback(new Error("user already has subscription in recurly"))
+			RecurlyWrapper.createSubscription user, subscriptionDetails, recurly_token_id, (error, recurlySubscription)->
 				return callback(error) if error?
-				callback()
+				SubscriptionUpdater.syncSubscription recurlySubscription, user._id, (error) ->
+					return callback(error) if error?
+					callback()
 
 	updateSubscription: (user, plan_code, coupon_code, callback)->
 		logger.log user:user, plan_code:plan_code, coupon_code:coupon_code, "updating subscription"

services/web/app/views/project/editor/editor.pug

@@ -19,7 +19,8 @@ div.full-size(
 			'rp-size-mini': (!ui.reviewPanelOpen && reviewPanel.hasEntries),\
 			'rp-size-expanded': ui.reviewPanelOpen,\
 			'rp-layout-left': reviewPanel.layoutToLeft,\
-			'rp-loading-threads': reviewPanel.loadingThreads\
+			'rp-loading-threads': reviewPanel.loadingThreads,\
+			'rp-new-comment-ui': reviewPanel.newAddCommentUI\
 		}"
 	)
 		.loading-panel(ng-show="!editor.sharejs_doc || editor.opening")

services/web/app/views/project/editor/review-panel.pug

@@ -1,10 +1,18 @@
 #review-panel
-	a.rp-track-changes-indicator(
-		href
-		ng-if="editor.wantTrackChanges"
-		ng-click="toggleReviewPanel();"
-		ng-class="{ 'rp-track-changes-indicator-on-dark' : darkTheme }"
-	) !{translate("track_changes_is_on")}
+	.rp-in-editor-widgets
+		a.rp-track-changes-indicator(
+			href
+			ng-if="editor.wantTrackChanges"
+			ng-click="toggleReviewPanel();"
+			ng-class="{ 'rp-track-changes-indicator-on-dark' : darkTheme }"
+		) !{translate("track_changes_is_on")}
+		a.rp-add-comment-btn(
+			href
+			ng-if="reviewPanel.newAddCommentUI && reviewPanel.entries[editor.open_doc_id]['add-comment'] != null"
+			ng-click="addNewComment();"
+		) 
+			i.fa.fa-comment
+			|  #{translate("add_comment")}
 	.review-panel-toolbar
 		resolved-comments-dropdown(
 			class="rp-flex-block"
@@ -314,7 +322,7 @@ script(type='text/ng-template', id='resolvedCommentEntryTemplate')
 script(type='text/ng-template', id='addCommentEntryTemplate')
 	div
 		.rp-entry-callout.rp-entry-callout-add-comment
-		.rp-entry-indicator(
+		.rp-entry-indicator.rp-entry-indicator-add-comment(
 			ng-if="!commentState.adding"
 			ng-click="startNewComment(); onIndicatorClick();"
 			tooltip=translate("add_comment")

services/web/public/coffee/ide/editor/EditorManager.coffee

@@ -121,11 +121,22 @@ define [
 
 		_bindToDocumentEvents: (doc, sharejs_doc) ->
 			sharejs_doc.on "error", (error, meta) =>
-				if error?.message?.match "maxDocLength"
+				if error?.message?
+					message = error.message
+				else if typeof error == "string"
+					message = error
+				else
+					message = ""
+				if message.match "maxDocLength"
 					@ide.showGenericMessageModal(
 						"Document Too Long"
 						"Sorry, this file is too long to be edited manually. Please upload it directly."
 					)
+				else if message.match "too many comments or tracked changes"
+					@ide.showGenericMessageModal(
+						"Too many comments or tracked changes"
+						"Sorry, this file has too many comments or tracked changes. Please try accepting or rejecting some existing changes, or resolving and deleting some comments."
+					)
 				else
 					@ide.socket.disconnect()
 					@ide.reportError(error, meta)

services/web/public/coffee/ide/editor/directives/aceEditor/track-changes/TrackChangesManager.coffee

@@ -73,16 +73,24 @@ define [
 						_scrollTimeout = null
 					, 200
 
+			@_resetCutState()
+			onCut = () => @onCut()
+			onPaste = () => @onPaste()
+
 			bindToAce = () =>
 				@editor.on "changeSelection", onChangeSelection
 				@editor.on "change", onChangeSelection # Selection also moves with updates elsewhere in the document
 				@editor.on "changeSession", onChangeSession
+				@editor.on "cut", onCut
+				@editor.on "paste", onPaste
 				@editor.renderer.on "resize", onResize
 
 			unbindFromAce = () =>
 				@editor.off "changeSelection", onChangeSelection
 				@editor.off "change", onChangeSelection
 				@editor.off "changeSession", onChangeSession
+				@editor.off "cut", onCut
+				@editor.off "paste", onPaste
 				@editor.renderer.off "resize", onResize
 
 			@$scope.$watch "trackChangesEnabled", (enabled) =>
@@ -244,6 +252,49 @@ define [
 					@_onCommentAdded(comment)
 			@broadcastChange()
 
+		_resetCutState: () ->
+			@_cutState = {
+				text: null
+				comments: []
+				docId: null
+			}
+
+		onCut: () ->
+			@_resetCutState()
+			selection = @editor.getSelectionRange()
+			selection_start = @_aceRangeToShareJs(selection.start)
+			selection_end = @_aceRangeToShareJs(selection.end)
+			@_cutState.text = @editor.getSelectedText()
+			@_cutState.docId = @$scope.docId
+			for comment in @rangesTracker.comments
+				comment_start = comment.op.p
+				comment_end = comment_start + comment.op.c.length
+				if selection_start <= comment_start and comment_end <= selection_end
+					@_cutState.comments.push {
+						offset: comment.op.p - selection_start
+						text: comment.op.c
+						comment: comment
+					}
+
+		onPaste: () =>
+			@editor.once "change", (change) =>
+				return if change.action != "insert"
+				pasted_text = change.lines.join("\n")
+				paste_offset = @_aceRangeToShareJs(change.start)
+				# We have to wait until the change has been processed by the range tracker, 
+				# since if we move the ops into place beforehand, they will be moved again
+				# when the changes are processed by the range tracker. This ranges:dirty
+				# event is fired after the doc has applied the changes to the range tracker.
+				@$scope.sharejsDoc.on "ranges:dirty.paste", () =>
+					@$scope.sharejsDoc.off "ranges:dirty.paste" # Doc event emitter uses namespaced events
+					if pasted_text == @_cutState.text and @$scope.docId == @_cutState.docId
+						for {comment, offset, text} in @_cutState.comments
+							op = { c: text, p: paste_offset + offset, t: comment.id }
+							@$scope.sharejsDoc.submitOp op # Resubmitting an existing comment op (by thread id) will move it
+					@_resetCutState()
+					# Check that comments still match text. Will throw error if not.
+					@rangesTracker.validate(@editor.getValue())
+
 		checkMapping: () ->
 			# TODO: reintroduce this check
 			session = @editor.getSession()

services/web/public/coffee/ide/file-tree/FileTreeManager.coffee

@@ -173,6 +173,8 @@ define [
 			@_findEntityByPathInFolder @$scope.rootFolder, path
 
 		_findEntityByPathInFolder: (folder, path) ->
+			if !path? or !folder?
+				return null
 			parts = path.split("/")
 			name = parts.shift()
 			rest = parts.join("/")

services/web/public/coffee/ide/review-panel/RangesTracker.coffee

@@ -1,3 +1,6 @@
+# This file is shared between document-updater and web, so that the server and client share
+# an identical track changes implementation. Do not edit it directly in web or document-updater,
+# instead edit it at https://github.com/sharelatex/ranges-tracker, where it has a suite of tests
 load = () ->
 	class RangesTracker
 		# The purpose of this class is to track a set of inserts and deletes to a document, like
@@ -78,6 +81,13 @@ load = () ->
 			@comments = @comments.filter (c) -> c.id != comment_id
 			@_markAsDirty comment, "comment", "removed"
 		
+		moveCommentId: (comment_id, position, text) ->
+			for comment in @comments
+				if comment.id == comment_id
+					comment.op.p = position
+					comment.op.c = text
+					@_markAsDirty comment, "comment", "moved"
+
 		getChange: (change_id) ->
 			change = null
 			for c in @changes
@@ -90,6 +100,18 @@ load = () ->
 			change = @getChange(change_id)
 			return if !change?
 			@_removeChange(change)
+		
+		validate: (text) ->
+			for change in @changes
+				if change.op.i?
+					content = text.slice(change.op.p, change.op.p + change.op.i.length)
+					if content != change.op.i
+						throw new Error("Change (#{JSON.stringify(change)}) doesn't match text (#{JSON.stringify(content)})")
+			for comment in @comments
+				content = text.slice(comment.op.p, comment.op.p + comment.op.c.length)
+				if content != comment.op.c
+					throw new Error("Comment (#{JSON.stringify(comment)}) doesn't match text (#{JSON.stringify(content)})")
+			return true
 
 		applyOp: (op, metadata = {}) ->
 			metadata.ts ?= new Date()
@@ -110,17 +132,21 @@ load = () ->
 				@applyOp(op, metadata)
 
 		addComment: (op, metadata) ->
-			# TODO: Don't allow overlapping comments?
-			@comments.push comment = {
-				id: op.t or @newId()
-				op: # Copy because we'll modify in place
-					c: op.c
-					p: op.p
-					t: op.t
-				metadata
-			}
-			@_markAsDirty comment, "comment", "added"
-			return comment
+			existing = @getComment(op.t)
+			if existing?
+				@moveCommentId(op.t, op.p, op.c)
+				return existing
+			else
+				@comments.push comment = {
+					id: op.t or @newId()
+					op: # Copy because we'll modify in place
+						c: op.c
+						p: op.p
+						t: op.t
+					metadata
+				}
+				@_markAsDirty comment, "comment", "added"
+				return comment
 		
 		applyInsertToComments: (op) ->
 			for comment in @comments

services/web/public/coffee/ide/review-panel/controllers/ReviewPanelController.coffee

@@ -4,7 +4,7 @@ define [
 	"ide/colors/ColorManager"
 	"ide/review-panel/RangesTracker"
 ], (App, EventEmitter, ColorManager, RangesTracker) ->
-	App.controller "ReviewPanelController", ($scope, $element, ide, $timeout, $http, $modal, event_tracking, localStorage) ->
+	App.controller "ReviewPanelController", ($scope, $element, ide, $timeout, $http, $modal, event_tracking, sixpack, localStorage) ->
 		$reviewPanelEl = $element.find "#review-panel"
 
 		$scope.SubViews =
@@ -27,6 +27,14 @@ define [
 			layoutToLeft: false
 			rendererData: {}
 			loadingThreads: false
+			newAddCommentUI: false # Test new UI for adding comments; remove afterwards.
+
+		$scope.shouldABAddCommentBtn = false
+		if $scope.user.signUpDate >= '2017-03-27'
+			sixpack.participate "add-comment-btn", [ "default", "editor-corner" ], (variation) ->
+				$scope.shouldABAddCommentBtn = true
+				$scope.variationABAddCommentBtn = variation
+				$scope.reviewPanel.newAddCommentUI = (variation == "editor-corner")
 
 		window.addEventListener "beforeunload", () ->
 			collapsedStates = {}
@@ -163,7 +171,11 @@ define [
 
 		$scope.$watch (() ->
 			entries = $scope.reviewPanel.entries[$scope.editor.open_doc_id] or {}
-			Object.keys(entries).length
+			permEntries = {}
+			for entry, entryData of entries
+				if entry != "add-comment" or !$scope.reviewPanel.newAddCommentUI
+					permEntries[entry] = entryData 
+			Object.keys(permEntries).length
 		), (nEntries) ->
 			$scope.reviewPanel.hasEntries = nEntries > 0 and $scope.project.features.trackChangesVisible
 
@@ -323,11 +335,17 @@ define [
 			$scope.$broadcast "change:reject", entry_id
 			event_tracking.sendMB "rp-change-rejected", { view: if $scope.ui.reviewPanelOpen then $scope.reviewPanel.subView else 'mini' }
 		
+		$scope.addNewComment = () ->
+			$scope.$broadcast "comment:start_adding"
+			$scope.toggleReviewPanel()
+
 		$scope.startNewComment = () ->
 			$scope.$broadcast "comment:select_line"
 			$timeout () ->
 				$scope.$broadcast "review-panel:layout"
-		
+			if $scope.shouldABAddCommentBtn and !$scope.ui.reviewPanelOpen
+				sixpack.convert "add-comment-btn"
+
 		$scope.submitNewComment = (content) ->
 			return if !content? or content == ""
 			doc_id = $scope.editor.open_doc_id

services/web/public/coffee/ide/review-panel/directives/addCommentEntry.coffee

@@ -8,13 +8,16 @@ define [
 			onStartNew: "&"
 			onSubmit: "&"
 			onCancel: "&"
-			onIndicatorClick: "&"
+			onIndicatorClick: "&"	
 			layoutToLeft: "="
 		link: (scope, element, attrs) ->
 			scope.state =
 				isAdding: false
 				content: ""
 
+			scope.$on "comment:start_adding", () ->
+				scope.startNewComment()
+
 			scope.startNewComment = () ->
 				scope.state.isAdding = true
 				scope.onStartNew()

services/web/public/js/ace-1.2.5/worker-latex.js

@@ -1931,7 +1931,7 @@ var InterpretTokens = function (TokeniseResult, ErrorReporter) {
                 if (newPos === null) { continue; } else {i = newPos;};
             } else if (seq === "hbox" || seq === "text" || seq === "mbox" || seq === "footnote" || seq === "intertext" || seq === "shortintertext" || seq === "textnormal" || seq === "tag" || seq === "reflectbox" || seq === "textrm") {
                 nextGroupMathMode = false;
-            } else if (seq === "rotatebox" || seq === "scalebox") {
+            } else if (seq === "rotatebox" || seq === "scalebox"  || seq == "feynmandiagram") {
                 newPos = readOptionalGeneric(TokeniseResult, i);
                 if (newPos === null) { /* do nothing */ } else {i = newPos;};
                 newPos = readDefinition(TokeniseResult, i);
@@ -2179,7 +2179,7 @@ var EnvHandler = function (ErrorReporter) {
     this._beginMathMode = function (thisEnv) {
         var currentMathMode = this.getMathMode(); // undefined, null, $, $$, name of mathmode env
         if (currentMathMode) {
-            ErrorFrom(thisEnv, thisEnv.name + " used inside existing math mode " + getName(currentMathMode),
+            ErrorFrom(thisEnv, getName(thisEnv) + " used inside existing math mode " + getName(currentMathMode),
                       {suppressIfEditing:true, errorAtStart: true, mathMode:true});
         };
         thisEnv.mathMode = thisEnv;

services/web/public/stylesheets/app/editor/review-panel.less

@@ -9,13 +9,10 @@
 @rp-border-grey         : #d9d9d9;
 
 @rp-green               : #2c8e30;
-@rp-dim-green           : #cae3cb;
 @rp-green-on-dark		: rgba(37, 107, 41, 0.5);
 @rp-red                 : #c5060b;
-@rp-dim-red             : #f3cdce;
 @rp-yellow              : #f3b111;
 @rp-yellow-on-dark		: rgba(194, 93, 11, 0.5);
-@rp-dim-yellow          : #ffe9b2;
 @rp-grey                : #aaaaaa;
 
 @rp-type-blue           : #6b7797;
@@ -93,7 +90,7 @@
 	}
 
 #review-panel {
-	display: none;
+	display: block;
 	.rp-size-expanded & {
 		display: flex;
 		flex-direction: column;
@@ -101,7 +98,6 @@
 		overflow: visible;
 	}
 	.rp-size-mini & {
-		display: block;
 		width: @review-off-width;
 		z-index: 6;
 	}
@@ -155,7 +151,13 @@
 	}
 
 .rp-entry-list {
+	display: none;
 	width: 100%;
+
+	.rp-size-expanded &,
+	.rp-size-mini & {
+		display: block;
+	}
 	
 	.rp-state-current-file & {
 		position: absolute;
@@ -197,6 +199,10 @@
 		right: 4px;
 		z-index: 1;
 	}
+
+	.rp-new-comment-ui &-add-comment {
+		display: none;
+	}
 }
 
 .rp-entry-wrapper {
@@ -448,6 +454,12 @@
 		display: block;
 		padding: 5px 10px;
 		border-radius: 3px;
+
+		.rp-in-editor-widgets & {
+			white-space: nowrap;
+			border-radius: 0;
+			border-bottom-left-radius: 3px;
+		}
 	}
 
 	.rp-new-comment {
@@ -563,6 +575,10 @@
 			border-color: @rp-yellow;
 		}
 	}
+
+	.rp-size-mini.rp-new-comment-ui &-add-comment {
+		display: none;
+	}
 }
 
 .rp-overview-file-header {
@@ -610,11 +626,12 @@
 }
 
 .rp-nav {
-	display: flex;
+	display: none;
 	flex-shrink: 0;
-	.rp-size-mini & {
-		display: none;
+	.rp-size-expanded & {
+		display: flex;
 	}
+
 	.rp-state-current-file & {
 		position: absolute;
 		bottom: 0;
@@ -748,13 +765,16 @@
 		.rp-loading-threads & {
 			display: none;
 		}
+		z-index: 6 // Appear above text selection
 	}
 
 	.track-changes-comment-marker {
-		background-color: @rp-dim-yellow;
+		background-color: @rp-yellow;
+		opacity: 0.3;
 	}
 	.track-changes-added-marker {
-		background-color: @rp-dim-green;
+		background-color: @rp-green;
+		opacity: 0.3;
 	}
 	.track-changes-deleted-marker {
 		border-left: 2px dotted @rp-red;
@@ -871,43 +891,49 @@
 	}
 }
 
-.rp-track-changes-indicator {
-	display: none;
+.rp-in-editor-widgets {
 	position: absolute;
 	top: 0;
-	right: @review-off-width;
-	padding: 5px 10px;
-	background-color: rgba(240, 240, 240, 0.9);
-	color: @rp-type-blue;
-	text-align: center;
-	border-bottom-left-radius: 3px;
-	font-size: 10px;
+	right: 0;
+	font-size: 11px;
 	z-index: 2;
-	white-space: nowrap;
-
-	&.rp-track-changes-indicator-on-dark {
-		background-color: rgba(88, 88, 88, .8);
-		color: #FFF;
-		
-		&:hover,
-		&:focus {
-			background-color: rgba(88, 88, 88, 1);
-			color: #FFF;
-		}
-	}
-
-	&:hover,
-	&:focus {
-		outline: 0;
-		text-decoration: none;
-		background-color: rgba(240, 240, 240, 1);
-		color: @rp-type-blue;
-	}
-
+	
 	.rp-size-mini & {
-		display: block;
+		right: @review-off-width;
 	}
 }
+	.rp-track-changes-indicator {
+		display: block;
+		padding: 5px 10px;
+		background-color: rgba(240, 240, 240, 0.9);
+		color: @rp-type-blue;
+		text-align: center;
+		border-bottom-left-radius: 3px;
+		white-space: nowrap;
+
+		&.rp-track-changes-indicator-on-dark {
+			background-color: rgba(88, 88, 88, .8);
+			color: #FFF;
+			
+			&:hover,
+			&:focus {
+				background-color: rgba(88, 88, 88, 1);
+				color: #FFF;
+			}
+		}
+
+		&:hover,
+		&:focus {
+			outline: 0;
+			text-decoration: none;
+			background-color: rgba(240, 240, 240, 1);
+			color: @rp-type-blue;
+		}
+
+		.rp-size-expanded & {
+			display: none;
+		}
+	}
 
 // Helper class for elements which aren't treated as flex-items by IE10, e.g:
 // * inline items;

services/web/test/UnitTests/coffee/Analytics/AnalyticsControllerTests.coffee

@@ -0,0 +1,44 @@
+should = require('chai').should()
+SandboxedModule = require('sandboxed-module')
+assert = require('assert')
+path = require('path')
+modulePath = path.join __dirname, '../../../../app/js/Features/Analytics/AnalyticsController'
+sinon = require("sinon")
+expect = require("chai").expect
+
+
+describe 'AnalyticsController', ->
+
+	beforeEach ->
+		@AuthenticationController =
+			getLoggedInUserId: sinon.stub()
+
+		@AnalyticsManager =
+			recordEvent: sinon.stub().callsArgWith(3)
+
+		@req = 
+			params:
+				event:"i_did_something"
+			body:"stuff"
+			sessionID: "sessionIDHere"
+
+		@res =
+			send:->
+		@controller = SandboxedModule.require modulePath, requires:
+			"./AnalyticsManager":@AnalyticsManager
+			"../Authentication/AuthenticationController":@AuthenticationController
+			"logger-sharelatex":
+				log:->
+
+	describe "recordEvent", ->
+
+		it "should use the user_id", (done)->
+			@AuthenticationController.getLoggedInUserId.returns("1234")
+			@controller.recordEvent @req, @res
+			@AnalyticsManager.recordEvent.calledWith("1234", @req.params["event"], @req.body).should.equal true
+			done()
+
+		it "should use the session id", (done)->
+			@controller.recordEvent @req, @res
+			@AnalyticsManager.recordEvent.calledWith(@req.sessionID, @req.params["event"], @req.body).should.equal true
+			done()

services/web/test/UnitTests/coffee/Authentication/AuthenticationControllerTests.coffee

@@ -254,6 +254,8 @@ describe "AuthenticationController", ->
 				@cb = sinon.stub()
 				@LoginRateLimiter.processLoginRequest.callsArgWith(1, null, true)
 				@AuthenticationManager.authenticate = sinon.stub().callsArgWith(2, null, @user)
+				@req.sessionID = Math.random()
+				@AnalyticsManager.identifyUser = sinon.stub()
 				@AuthenticationController.doPassportLogin(@req, @req.body.email, @req.body.password, @cb)
 
 			it "should attempt to authorise the user", ->
@@ -261,6 +263,9 @@ describe "AuthenticationController", ->
 					.calledWith(email: @email.toLowerCase(), @password)
 					.should.equal true
 
+			it "should call identifyUser", ->
+				@AnalyticsManager.identifyUser.calledWith(@user._id, @req.sessionID).should.equal true
+
 			it "should setup the user data in the background", ->
 				@UserHandler.setupLoginData.calledWith(@user).should.equal true
 

services/web/test/UnitTests/coffee/Authorization/AuthorizationManagerTests.coffee

@@ -136,7 +136,20 @@ describe "AuthorizationManager", ->
 			it "should return a NotFoundError", ->
 				@AuthorizationManager.getPrivilegeLevelForProject @user_id, @project_id, (error) ->
 					error.should.be.instanceof Errors.NotFoundError
-	
+
+		describe "when the project id is not valid", ->
+			beforeEach ->
+				@AuthorizationManager.isUserSiteAdmin.withArgs(@user_id).yields(null, false)
+				@CollaboratorsHandler.getMemberIdPrivilegeLevel
+					.withArgs(@user_id, @project_id)
+					.yields(null, "readOnly")
+
+			it "should return a error", (done)->
+				@AuthorizationManager.getPrivilegeLevelForProject undefined, "not project id", (err) =>
+					@Project.findOne.called.should.equal false
+					expect(err).to.exist
+					done()
+
 	describe "canUserReadProject", ->
 		beforeEach ->
 			@AuthorizationManager.getPrivilegeLevelForProject = sinon.stub()

services/web/test/UnitTests/coffee/Email/EmailSenderTests.coffee

@@ -51,17 +51,19 @@ describe "EmailSender", ->
 		it "should set the properties on the email to send", (done)->
 			@sesClient.sendMail.callsArgWith(1)
 
-			@sender.sendEmail @opts, =>
+			@sender.sendEmail @opts, (err) =>
+				expect(err).to.not.exist
 				args = @sesClient.sendMail.args[0][0]
 				args.html.should.equal @opts.html
 				args.to.should.equal @opts.to
 				args.subject.should.equal @opts.subject
 				done()
 
-		it "should return the error", (done)->
+		it "should return a non-specific error", (done)->
 			@sesClient.sendMail.callsArgWith(1, "error")
 			@sender.sendEmail {}, (err)=>
-				err.should.equal "error"
+				err.should.exist
+				err.toString().should.equal 'Error: Cannot send email'
 				done()
 
 

services/web/test/UnitTests/coffee/FileStore/FileStoreHandlerTests.coffee

@@ -96,6 +96,13 @@ describe "FileStoreHandler", ->
 					@fs.createReadStream.called.should.equal false
 					done()
 
+		describe "symlink", ->
+			it "should not read file stat returns nothing", (done)->
+				@fs.lstat = sinon.stub().callsArgWith(1, null, null)
+				@handler.uploadFileFromDisk @project_id, @file_id, @fsPath, =>
+					@fs.createReadStream.called.should.equal false
+					done()
+
 		describe "when upload fails", ->
 			beforeEach ->
 				@writeStream.on = (type, cb) ->

services/web/test/UnitTests/coffee/Project/ProjectApiControllerTests.coffee

@@ -20,6 +20,7 @@ describe 'Project api controller', ->
 			session:
 				destroy:sinon.stub()
 		@res = {}
+		@next = sinon.stub()
 		@projDetails = {name:"something"}
 
 
@@ -34,9 +35,7 @@ describe 'Project api controller', ->
 			@controller.getProjectDetails @req, @res
 
 
-		it "should send a 500 if there is an error", (done)->
+		it "should send a 500 if there is an error", ()->
 			@ProjectDetailsHandler.getDetails.callsArgWith(1, "error")
-			@res.sendStatus = (resCode)=>
-				resCode.should.equal 500
-				done()
-			@controller.getProjectDetails @req, @res
+			@controller.getProjectDetails @req, @res, @next
+			@next.calledWith("error").should.equal true

services/web/test/UnitTests/coffee/Project/ProjectDetailsHandlerTests.coffee

@@ -1,5 +1,6 @@
 should = require('chai').should()
 modulePath = "../../../../app/js/Features/Project/ProjectDetailsHandler"
+Errors = require "../../../../app/js/Features/Errors/Errors"
 SandboxedModule = require('sandboxed-module')
 sinon = require('sinon')
 assert = require("chai").assert
@@ -48,6 +49,13 @@ describe 'ProjectDetailsHandler', ->
 				assert.equal(details.something, undefined)
 				done()
 
+		it "should return an error for a non-existent project", (done)->
+			@ProjectGetter.getProject.callsArg(2, null, null)
+			err = new Errors.NotFoundError("project not found")
+			@handler.getDetails "0123456789012345678901234", (error, details) =>
+				err.should.eql error
+				done()
+
 		it "should return the error", (done)->
 			error = "some error"
 			@ProjectGetter.getProject.callsArgWith(2, error)

services/web/test/UnitTests/coffee/Project/ProjectDuplicatorTests.coffee

@@ -9,7 +9,7 @@ describe 'ProjectDuplicator', ->
 		@level2folder =
 			name: "level2folderName"
 			_id:"level2folderId"
-			docs:[@doc2 = {_id: "doc2_id", name:"level2folderDocName"}]
+			docs:[@doc2 = {_id: "doc2_id", name:"level2folderDocName"}, undefined]
 			folders:[]
 			fileRefs:[{name:"file2", _id:"file2"}]
 		@level1folder =
@@ -17,12 +17,12 @@ describe 'ProjectDuplicator', ->
 			_id:"level1folderId"
 			docs:[@doc1 = {_id: "doc1_id", name:"level1folderDocName"}]
 			folders:[@level2folder]
-			fileRefs:[{name:"file1", _id:"file1"}]
+			fileRefs:[{name:"file1", _id:"file1"}, null]
 		@rootFolder = 
 			name:"rootFolder"
 			_id:"rootFolderId"
 			docs:[@doc0 = {_id: "doc0_id", name:"rootDocHere"}]
-			folders:[@level1folder]
+			folders:[@level1folder, {}]
 			fileRefs:[{name:"file0", _id:"file0"}]
 		@project = 
 			_id: @old_project_id = "this_is_the_old_project_id"
@@ -117,7 +117,7 @@ describe 'ProjectDuplicator', ->
 			@projectOptionsHandler.setCompiler.calledWith(@stubbedNewProject._id, @project.compiler).should.equal true
 			done()
 	
-	it 'should use the same root docccccccc', (done)->
+	it 'should use the same root doc', (done)->
 		@entityHandler.addDocWithProject.callsArgWith(4, null, @rootFolder.docs[0])
 		@duplicator.duplicate @owner, @old_project_id, "", (err, newProject)=>
 			@entityHandler.setRootDoc.calledWith(@stubbedNewProject._id, @rootFolder.docs[0]._id).should.equal true

services/web/test/UnitTests/coffee/Project/ProjectLocatorTests.coffee

@@ -17,7 +17,7 @@ file1 = name:"file1", _id:"dsa9lkdsad"
 subSubFile = name:"subSubFile", _id:"d1d2dk"
 subSubDoc = name:"subdoc.txt", _id:"321dmdwi"
 secondSubFolder = name:"secondSubFolder", _id:"dsa3e23", docs:[subSubDoc], fileRefs:[subSubFile], folders:[]
-subFolder = name:"subFolder", _id:"dsadsa93", folders:[secondSubFolder], docs:[], fileRefs:[]
+subFolder = name:"subFolder", _id:"dsadsa93", folders:[secondSubFolder, null], docs:[], fileRefs:[]
 subFolder1 = name:"subFolder1", _id:"123asdjoij"
 
 rootFolder =

services/web/test/UnitTests/coffee/Subscription/RecurlyWrapperTests.coffee

@@ -1030,3 +1030,35 @@ describe "RecurlyWrapper", ->
 					@call (err, result) =>
 						expect(err).to.be.instanceof Error
 						done()
+
+	describe "listAccountActiveSubscriptions", ->
+		beforeEach ->
+			@user_id = "mock-user-id"
+			@callback = sinon.stub()
+			@RecurlyWrapper.apiRequest = sinon.stub().yields(null, @response = {"mock": "response"}, @body = "<mock body/>")
+			@RecurlyWrapper._parseSubscriptionsXml = sinon.stub().yields(null, @subscriptions = ["mock", "subscriptions"])
+			
+		describe "with an account", ->
+			beforeEach ->
+				@RecurlyWrapper.listAccountActiveSubscriptions @user_id, @callback
+			
+			it "should send a request to Recurly", ->
+				@RecurlyWrapper.apiRequest
+					.calledWith({
+						url: "accounts/#{@user_id}/subscriptions"
+						qs:
+							state: "active"
+						expect404: true
+					})
+					.should.equal true
+
+			it "should return the subscriptions", ->
+				@callback.calledWith(null, @subscriptions).should.equal true
+			
+		describe "without an account", ->
+			beforeEach ->
+				@response.statusCode = 404
+				@RecurlyWrapper.listAccountActiveSubscriptions @user_id, @callback
+
+			it "should return an empty array of subscriptions", ->
+				@callback.calledWith(null, []).should.equal true

services/web/test/UnitTests/coffee/Subscription/SubscriptionControllerTests.coffee

@@ -17,8 +17,7 @@ mockSubscriptions =
 		account:
 			account_code: "user-123"
 
-describe "SubscriptionController sanboxed", ->
-
+describe "SubscriptionController", ->
 	beforeEach ->
 		@user = {email:"tom@yahoo.com", _id: 'one', signUpDate: new Date('2000-10-01')}
 		@activeRecurlySubscription = mockSubscriptions["subscription-123-active"]
@@ -150,6 +149,7 @@ describe "SubscriptionController sanboxed", ->
 	describe "paymentPage", ->
 		beforeEach ->
 			@req.headers = {}
+			@SubscriptionHandler.validateNoSubscriptionInRecurly = sinon.stub().yields(null, true)
 			@GeoIpLookup.getCurrencyCode.callsArgWith(1, null, @stubbedCurrencyCode)
 
 		describe "with a user without a subscription", ->
@@ -209,6 +209,16 @@ describe "SubscriptionController sanboxed", ->
 					opts.currency.should.equal @stubbedCurrencyCode
 					done()
 				@SubscriptionController.paymentPage @req, @res
+		
+		describe "with a recurly subscription already", ->
+			it "should redirect to the subscription dashboard", (done)->
+				@LimitationsManager.userHasSubscription.callsArgWith(1, null, false)
+				@SubscriptionHandler.validateNoSubscriptionInRecurly = sinon.stub().yields(null, false)
+				@res.redirect = (url)=>
+					url.should.equal "/user/subscription"
+					done()
+				@SubscriptionController.paymentPage(@req, @res)
+			
 
 	describe "successful_subscription", ->
 		beforeEach (done) ->

services/web/test/UnitTests/coffee/Subscription/SubscriptionHandlerTests.coffee

@@ -16,7 +16,7 @@ mockRecurlySubscriptions =
 		account:
 			account_code: "user-123"
 			
-describe "Subscription Handler sanboxed", ->
+describe "SubscriptionHandler", ->
 
 	beforeEach ->
 		@Settings = 
@@ -33,7 +33,7 @@ describe "Subscription Handler sanboxed", ->
 		@activeRecurlySubscription = mockRecurlySubscriptions["subscription-123-active"]
 		@User = {}
 		@user =
-			_id: "user_id_here_"
+			_id: @user_id = "user_id_here_"
 		@subscription = 
 			recurlySubscription_id: @activeRecurlySubscription.uuid
 		@RecurlyWrapper = 
@@ -77,23 +77,33 @@ describe "Subscription Handler sanboxed", ->
 
 
 	describe "createSubscription", ->
-		beforeEach (done) ->
+		beforeEach ->
+			@callback = sinon.stub()
 			@subscriptionDetails = 
 				cvv:"123"
 				number:"12345"
 			@recurly_token_id = "45555666"
-			@SubscriptionHandler.createSubscription(@user, @subscriptionDetails, @recurly_token_id, done)
+			@SubscriptionHandler.validateNoSubscriptionInRecurly = sinon.stub().yields(null, true)
 
-		it "should create the subscription with the wrapper", (done)->
-			@RecurlyWrapper.createSubscription.calledWith(@user, @subscriptionDetails, @recurly_token_id).should.equal true
-			done()
+		describe "successfully", ->
+			beforeEach ->
+				@SubscriptionHandler.createSubscription(@user, @subscriptionDetails, @recurly_token_id, @callback)
 
-		it "should sync the subscription to the user", (done)->
-			@SubscriptionUpdater.syncSubscription.calledOnce.should.equal true
-			@SubscriptionUpdater.syncSubscription.args[0][0].should.deep.equal @activeRecurlySubscription
-			@SubscriptionUpdater.syncSubscription.args[0][1].should.deep.equal @user._id
-			done()
+			it "should create the subscription with the wrapper", ->
+				@RecurlyWrapper.createSubscription.calledWith(@user, @subscriptionDetails, @recurly_token_id).should.equal true
 
+			it "should sync the subscription to the user", ->
+				@SubscriptionUpdater.syncSubscription.calledOnce.should.equal true
+				@SubscriptionUpdater.syncSubscription.args[0][0].should.deep.equal @activeRecurlySubscription
+				@SubscriptionUpdater.syncSubscription.args[0][1].should.deep.equal @user._id
+
+		describe "when there is already a subscription in Recurly", ->
+			beforeEach ->
+				@SubscriptionHandler.validateNoSubscriptionInRecurly = sinon.stub().yields(null, false)
+				@SubscriptionHandler.createSubscription(@user, @subscriptionDetails, @recurly_token_id, @callback)
+			
+			it "should return an error", ->
+				@callback.calledWith(new Error("user already has subscription in recurly"))
 
 	describe "updateSubscription", ->
 		describe "with a user with a subscription", ->
@@ -145,8 +155,6 @@ describe "Subscription Handler sanboxed", ->
 				updateOptions = @RecurlyWrapper.updateSubscription.args[0][1]
 				updateOptions.plan_code.should.equal @plan_code
 
-
-
 	describe "cancelSubscription", ->
 		describe "with a user without a subscription", ->
 			beforeEach (done) ->
@@ -210,5 +218,39 @@ describe "Subscription Handler sanboxed", ->
 				@SubscriptionUpdater.syncSubscription.args[0][0].should.deep.equal @activeRecurlySubscription
 				@SubscriptionUpdater.syncSubscription.args[0][1].should.deep.equal @user._id
 
+	describe "validateNoSubscriptionInRecurly", ->
+		beforeEach ->
+			@subscriptions = []
+			@RecurlyWrapper.listAccountActiveSubscriptions = sinon.stub().yields(null, @subscriptions)
+			@SubscriptionUpdater.syncSubscription = sinon.stub().yields()
+			@callback = sinon.stub()
 
+		describe "with no subscription in recurly", ->
+			beforeEach ->
+				@subscriptions.push @subscription = { "mock": "subscription" }
+				@SubscriptionHandler.validateNoSubscriptionInRecurly @user_id, @callback
 
+			it "should call RecurlyWrapper.listAccountActiveSubscriptions with the user id", ->
+				@RecurlyWrapper.listAccountActiveSubscriptions
+					.calledWith(@user_id)
+					.should.equal true
+
+			it "should sync the subscription", ->
+				@SubscriptionUpdater.syncSubscription
+					.calledWith(@subscription, @user_id)
+					.should.equal true
+
+			it "should call the callback with valid == false", ->
+				@callback.calledWith(null, false).should.equal true
+
+		describe "with a subscription in recurly", ->
+			beforeEach ->
+				@SubscriptionHandler.validateNoSubscriptionInRecurly @user_id, @callback
+
+			it "should not sync the subscription", ->
+				@SubscriptionUpdater.syncSubscription
+					.called
+					.should.equal false
+
+			it "should call the callback with valid == true", ->
+				@callback.calledWith(null, true).should.equal true