From 52f3596e53b85661d6ae2de961620870da6d8ad7 Mon Sep 17 00:00:00 2001
From: James Allen <james@sharelatex.com>
Date: Tue, 16 Apr 2019 11:05:17 +0100
Subject: [PATCH] Review feedback

---
 .../app/coffee/SnapshotManager.coffee         |  7 +++++--
 .../app/coffee/UpdateManager.coffee           | 19 +++++++++++++------
 .../test/acceptance/coffee/RangesTests.coffee |  2 +-
 .../UpdateManager/UpdateManagerTests.coffee   |  1 +
 4 files changed, 20 insertions(+), 9 deletions(-)

diff --git a/services/document-updater/app/coffee/SnapshotManager.coffee b/services/document-updater/app/coffee/SnapshotManager.coffee
index 5a756b34db..86670b648d 100644
--- a/services/document-updater/app/coffee/SnapshotManager.coffee
+++ b/services/document-updater/app/coffee/SnapshotManager.coffee
@@ -1,17 +1,20 @@
 {db, ObjectId} = require "./mongojs"
 
 module.exports = SnapshotManager =
-  recordSnapshot: (project_id, doc_id, version, lines, ranges, callback) ->
+  recordSnapshot: (project_id, doc_id, version, pathname, lines, ranges, callback) ->
     try
       project_id = ObjectId(project_id)
       doc_id = ObjectId(doc_id)
     catch error
       return callback(error)
     db.docSnapshots.insert {
-      project_id, doc_id, version, lines
+      project_id, doc_id, version, lines, pathname,
       ranges: SnapshotManager.jsonRangesToMongo(ranges),
       ts: new Date()
     }, callback
+    # Suggested indexes:
+    #   db.docSnapshots.createIndex({project_id:1, doc_id:1})
+    #   db.docSnapshots.createIndex({ts:1},{expiresAfterSeconds: 30*24*3600)) # expires after 30 days
 
   jsonRangesToMongo: (ranges) ->
     return null if !ranges?
diff --git a/services/document-updater/app/coffee/UpdateManager.coffee b/services/document-updater/app/coffee/UpdateManager.coffee
index 5f9dcf5317..2fe7508fed 100644
--- a/services/document-updater/app/coffee/UpdateManager.coffee
+++ b/services/document-updater/app/coffee/UpdateManager.coffee
@@ -85,17 +85,24 @@ module.exports = UpdateManager =
 					UpdateManager._addProjectHistoryMetadataToOps(appliedOps, pathname, projectHistoryId, lines)
 					profile.log("RangesManager.applyUpdate")
 					return callback(error) if error?
-					if ranges_were_collapsed
-						logger.log {project_id, doc_id, previousVersion, lines, ranges, update}, "update collapsed some ranges, snapshotting previous content"
-						SnapshotManager.recordSnapshot project_id, doc_id, previousVersion, lines, ranges, (error) ->
-							if error?
-								logger.error {err: error, project_id, doc_id, version, lines, ranges}, "error recording snapshot"
 					RedisManager.updateDocument project_id, doc_id, updatedDocLines, version, appliedOps, new_ranges, (error, doc_ops_length, project_ops_length) ->
 						profile.log("RedisManager.updateDocument")
 						return callback(error) if error?
 						HistoryManager.recordAndFlushHistoryOps project_id, doc_id, appliedOps, doc_ops_length, project_ops_length, (error) ->
 							profile.log("recordAndFlushHistoryOps")
-							callback(error)
+							return callback(error) if error?
+							if ranges_were_collapsed
+								logger.log {project_id, doc_id, previousVersion, lines, ranges, update}, "update collapsed some ranges, snapshotting previous content"
+								# Do this last, since it's a mongo call, and so potentially longest running
+								# If it overruns the lock, it's ok, since all of our redis work is done
+								SnapshotManager.recordSnapshot project_id, doc_id, previousVersion, pathname, lines, ranges, (error) ->
+									if error?
+										logger.error {err: error, project_id, doc_id, version, lines, ranges}, "error recording snapshot"
+										return callback(error)
+									else
+										callback()
+							else
+								callback()
 
 	lockUpdatesAndDo: (method, project_id, doc_id, args..., callback) ->
 		profile = new Profiler("lockUpdatesAndDo", {project_id, doc_id})
diff --git a/services/document-updater/test/acceptance/coffee/RangesTests.coffee b/services/document-updater/test/acceptance/coffee/RangesTests.coffee
index c6df55b459..52946f4823 100644
--- a/services/document-updater/test/acceptance/coffee/RangesTests.coffee
+++ b/services/document-updater/test/acceptance/coffee/RangesTests.coffee
@@ -311,7 +311,7 @@ describe "Ranges", ->
 				expect(ranges.changes).to.be.undefined
 				done()
 
-	describe.only "deleting text surrounding a comment", ->
+	describe "deleting text surrounding a comment", ->
 		before (done) ->
 			@project_id = DocUpdaterClient.randomId()
 			@user_id = DocUpdaterClient.randomId()
diff --git a/services/document-updater/test/unit/coffee/UpdateManager/UpdateManagerTests.coffee b/services/document-updater/test/unit/coffee/UpdateManager/UpdateManagerTests.coffee
index 32c305aedd..0fdbbb9728 100644
--- a/services/document-updater/test/unit/coffee/UpdateManager/UpdateManagerTests.coffee
+++ b/services/document-updater/test/unit/coffee/UpdateManager/UpdateManagerTests.coffee
@@ -252,6 +252,7 @@ describe "UpdateManager", ->
 						@project_id,
 						@doc_id,
 						@version,
+						@pathname,
 						@lines,
 						@ranges
 					)