Merge pull request #18468 from overleaf/em-docupdater-resolved-comment-ids

Store resolved comment ids in docupdater

GitOrigin-RevId: 69f09ecf69deedbb9a3682f13356533799025ea8
This commit is contained in:
Eric Mc Sween 2024-05-28 07:24:06 -04:00 committed by Copybot
parent a486f7e481
commit 8704e430a4
12 changed files with 258 additions and 99 deletions

View file

@ -17,6 +17,7 @@ const DocumentManager = {
lines,
version,
ranges,
resolvedCommentIds,
pathname,
projectHistoryId,
unflushedTime,
@ -31,6 +32,7 @@ const DocumentManager = {
lines,
version,
ranges,
resolvedCommentIds,
pathname,
projectHistoryId,
historyRangesSupport,
@ -40,6 +42,8 @@ const DocumentManager = {
projectId,
docId,
lines,
ranges,
resolvedCommentIds,
version,
pathname,
projectHistoryId,
@ -53,6 +57,7 @@ const DocumentManager = {
lines,
version,
ranges,
resolvedCommentIds,
pathname,
projectHistoryId,
historyRangesSupport
@ -61,6 +66,7 @@ const DocumentManager = {
lines,
version,
ranges: ranges || {},
resolvedCommentIds,
pathname,
projectHistoryId,
unflushedTime: null,
@ -74,6 +80,7 @@ const DocumentManager = {
ranges,
pathname,
projectHistoryId,
resolvedCommentIds,
unflushedTime,
alreadyLoaded: true,
historyRangesSupport,
@ -291,6 +298,8 @@ const DocumentManager = {
}
if (historyRangesSupport) {
await RedisManager.promises.updateCommentState(docId, commentId, resolved)
await ProjectHistoryRedisManager.promises.queueOps(
projectId,
JSON.stringify({
@ -326,6 +335,7 @@ const DocumentManager = {
)
if (historyRangesSupport) {
await RedisManager.promises.updateCommentState(docId, commentId, false)
await ProjectHistoryRedisManager.promises.queueOps(
projectId,
JSON.stringify({
@ -368,8 +378,14 @@ const DocumentManager = {
async resyncDocContents(projectId, docId, path) {
logger.debug({ projectId, docId, path }, 'start resyncing doc contents')
let { lines, ranges, version, projectHistoryId, historyRangesSupport } =
await RedisManager.promises.getDoc(projectId, docId)
let {
lines,
ranges,
resolvedCommentIds,
version,
projectHistoryId,
historyRangesSupport,
} = await RedisManager.promises.getDoc(projectId, docId)
// To avoid issues where the same docId appears with different paths,
// we use the path from the resyncProjectStructure update. If we used
@ -381,8 +397,14 @@ const DocumentManager = {
{ projectId, docId },
'resyncing doc contents - not found in redis - retrieving from web'
)
;({ lines, ranges, version, projectHistoryId, historyRangesSupport } =
await PersistenceManager.promises.getDoc(projectId, docId, {
;({
lines,
ranges,
resolvedCommentIds,
version,
projectHistoryId,
historyRangesSupport,
} = await PersistenceManager.promises.getDoc(projectId, docId, {
peek: true,
}))
} else {
@ -398,6 +420,7 @@ const DocumentManager = {
docId,
lines,
ranges,
resolvedCommentIds,
version,
// use the path from the resyncProjectStructure update
path,

View file

@ -102,7 +102,8 @@ function getDoc(projectId, docId, options = {}, _callback) {
body.ranges,
body.pathname,
body.projectHistoryId?.toString(),
body.historyRangesSupport || false
body.historyRangesSupport || false,
body.resolvedCommentIds || []
)
} else if (res.statusCode === 404) {
callback(new Errors.NotFoundError(`doc not not found: ${urlPath}`))
@ -188,6 +189,7 @@ module.exports = {
'pathname',
'projectHistoryId',
'historyRangesSupport',
'resolvedCommentIds',
]),
setDoc: promisify(setDoc),
},

View file

@ -135,6 +135,7 @@ const ProjectHistoryRedisManager = {
* @param {string} docId
* @param {string[]} lines
* @param {Ranges} ranges
* @param {string[]} resolvedCommentIds
* @param {number} version
* @param {string} pathname
* @param {boolean} historyRangesSupport
@ -146,6 +147,7 @@ const ProjectHistoryRedisManager = {
docId,
lines,
ranges,
resolvedCommentIds,
version,
pathname,
historyRangesSupport
@ -173,6 +175,7 @@ const ProjectHistoryRedisManager = {
if (historyRangesSupport) {
projectUpdate.resyncDocContent.ranges =
HistoryConversions.toHistoryRanges(ranges)
projectUpdate.resyncDocContent.resolvedCommentIds = resolvedCommentIds
}
const jsonUpdate = JSON.stringify(projectUpdate)

View file

@ -35,6 +35,7 @@ const RedisManager = {
docLines,
version,
ranges,
resolvedCommentIds,
pathname,
projectHistoryId,
historyRangesSupport,
@ -101,6 +102,13 @@ const RedisManager = {
})
if (historyRangesSupport) {
multi.sadd(keys.historyRangesSupport(), docId)
multi.del(keys.resolvedCommentIds({ doc_id: docId }))
if (resolvedCommentIds.length > 0) {
multi.sadd(
keys.resolvedCommentIds({ doc_id: docId }),
...resolvedCommentIds
)
}
}
multi.exec(callback)
}
@ -132,7 +140,8 @@ const RedisManager = {
keys.projectHistoryId({ doc_id: docId }),
keys.unflushedTime({ doc_id: docId }),
keys.lastUpdatedAt({ doc_id: docId }),
keys.lastUpdatedBy({ doc_id: docId })
keys.lastUpdatedBy({ doc_id: docId }),
keys.resolvedCommentIds({ doc_id: docId })
)
multi.exec((error, response) => {
if (error) {
@ -209,6 +218,13 @@ const RedisManager = {
if (error) {
return callback(error)
}
rclient.smembers(
keys.resolvedCommentIds({ doc_id: docId }),
(error, resolvedCommentIds) => {
if (error) {
return callback(error)
}
const historyRangesSupport = result === 1
const timeSpan = timer.done()
@ -221,7 +237,9 @@ const RedisManager = {
}
// record bytes loaded from redis
if (docLines != null) {
metrics.summary('redis.docLines', docLines.length, { status: 'get' })
metrics.summary('redis.docLines', docLines.length, {
status: 'get',
})
}
// check sha1 hash value if present
if (docLines != null && storedHash != null) {
@ -251,7 +269,10 @@ const RedisManager = {
version = parseInt(version || 0, 10)
// check doc is in requested project
if (docProjectId != null && docProjectId !== projectId) {
logger.error({ projectId, docId, docProjectId }, 'doc not in project')
logger.error(
{ projectId, docId, docProjectId },
'doc not in project'
)
return callback(new Errors.NotFoundError('document not found'))
}
@ -272,7 +293,10 @@ const RedisManager = {
unflushedTime,
lastUpdatedAt,
lastUpdatedBy,
historyRangesSupport
historyRangesSupport,
resolvedCommentIds
)
}
)
})
})
@ -513,6 +537,22 @@ const RedisManager = {
rclient.del(keys.unflushedTime({ doc_id: docId }), callback)
},
updateCommentState(docId, commentId, resolved, callback) {
if (resolved) {
rclient.sadd(
keys.resolvedCommentIds({ doc_id: docId }),
commentId,
callback
)
} else {
rclient.srem(
keys.resolvedCommentIds({ doc_id: docId }),
commentId,
callback
)
}
},
getDocIdsInProject(projectId, callback) {
rclient.smembers(keys.docsInProject({ project_id: projectId }), callback)
},
@ -629,6 +669,7 @@ module.exports.promises = promisifyAll(RedisManager, {
'lastUpdatedAt',
'lastUpdatedBy',
'historyRangesSupport',
'resolvedCommentIds',
],
getNextProjectToFlushAndDelete: [
'projectId',

View file

@ -137,6 +137,9 @@ module.exports = {
lastUpdatedAt({ doc_id: docId }) {
return `lastUpdatedAt:{${docId}}`
},
resolvedCommentIds({ doc_id: docId }) {
return `ResolvedCommentIds:{${docId}}`
},
flushAndDeleteQueue() {
return 'DocUpdaterFlushAndDeleteQueue'
},

View file

@ -22,6 +22,7 @@ describe('DocumentManager', function () {
putDocInMemory: sinon.stub().resolves(),
removeDocFromMemory: sinon.stub().resolves(),
renameDoc: sinon.stub().resolves(),
updateCommentState: sinon.stub().resolves(),
updateDocument: sinon.stub().resolves(),
},
}
@ -73,6 +74,7 @@ describe('DocumentManager', function () {
this.lines = ['one', 'two', 'three']
this.version = 42
this.ranges = { comments: 'mock', entries: 'mock' }
this.resolvedCommentIds = ['comment-1']
this.pathname = '/a/b/c.tex'
this.unflushedTime = Date.now()
this.lastUpdatedAt = Date.now()
@ -170,8 +172,7 @@ describe('DocumentManager', function () {
})
it('should write the doc lines to the persistence layer', function () {
this.PersistenceManager.promises.setDoc
.calledWith(
this.PersistenceManager.promises.setDoc.should.have.been.calledWith(
this.project_id,
this.doc_id,
this.lines,
@ -180,7 +181,6 @@ describe('DocumentManager', function () {
this.lastUpdatedAt,
this.lastUpdatedBy
)
.should.equal(true)
})
})
@ -298,6 +298,7 @@ describe('DocumentManager', function () {
lines: this.lines,
version: this.version,
ranges: this.ranges,
resolvedCommentIds: this.resolvedCommentIds,
pathname: this.pathname,
projectHistoryId: this.projectHistoryId,
unflushedTime: this.unflushedTime,
@ -322,6 +323,7 @@ describe('DocumentManager', function () {
lines: this.lines,
version: this.version,
ranges: this.ranges,
resolvedCommentIds: this.resolvedCommentIds,
pathname: this.pathname,
projectHistoryId: this.projectHistoryId,
unflushedTime: this.unflushedTime,
@ -344,6 +346,7 @@ describe('DocumentManager', function () {
lines: this.lines,
version: this.version,
ranges: this.ranges,
resolvedCommentIds: this.resolvedCommentIds,
pathname: this.pathname,
projectHistoryId: this.projectHistoryId,
historyRangesSupport: this.historyRangesSupport,
@ -374,6 +377,7 @@ describe('DocumentManager', function () {
this.lines,
this.version,
this.ranges,
this.resolvedCommentIds,
this.pathname,
this.projectHistoryId,
this.historyRangesSupport
@ -386,6 +390,7 @@ describe('DocumentManager', function () {
lines: this.lines,
version: this.version,
ranges: this.ranges,
resolvedCommentIds: this.resolvedCommentIds,
pathname: this.pathname,
projectHistoryId: this.projectHistoryId,
unflushedTime: null,
@ -409,6 +414,7 @@ describe('DocumentManager', function () {
lines: this.beforeLines,
version: this.version,
ranges: this.ranges,
resolvedCommentIds: this.resolvedCommentIds,
pathname: this.pathname,
projectHistoryId: this.projectHistoryId,
unflushedTime: this.unflushedTime,
@ -428,6 +434,7 @@ describe('DocumentManager', function () {
lines: this.beforeLines,
version: this.version,
ranges: this.ranges,
resolvedCommentIds: this.resolvedCommentIds,
pathname: this.pathname,
projectHistoryId: this.projectHistoryId,
unflushedTime: this.unflushedTime,
@ -746,12 +753,14 @@ describe('DocumentManager', function () {
this.version = 34
this.lines = ['original', 'lines']
this.ranges = { comments: ['one', 'two', 'three'] }
this.resolvedCommentIds = ['comment1']
this.updated_ranges = { comments: ['one', 'three'] }
this.historyRangesSupport = true
this.DocumentManager.promises.getDoc = sinon.stub().resolves({
lines: this.lines,
version: this.version,
ranges: this.ranges,
resolvedCommentIds: this.resolvedCommentIds,
pathname: this.pathname,
projectHistoryId: this.projectHistoryId,
unflushedTime: Date.now() - 1e9,
@ -797,6 +806,14 @@ describe('DocumentManager', function () {
.should.equal(true)
})
it('should unset the comment resolved state', function () {
this.RedisManager.promises.updateCommentState.should.have.been.calledWith(
this.doc_id,
this.comment_id,
false
)
})
it('should queue the delete comment operation', function () {
this.ProjectHistoryRedisManager.promises.queueOps.should.have.been.calledWith(
this.project_id,
@ -985,6 +1002,7 @@ describe('DocumentManager', function () {
lines: this.lines,
version: this.version,
ranges: this.ranges,
resolvedCommentIds: this.resolvedCommentIds,
pathname: this.pathname,
projectHistoryId: this.projectHistoryId,
historyRangesSupport: this.historyRangesSupport,
@ -1010,6 +1028,7 @@ describe('DocumentManager', function () {
this.doc_id,
this.lines,
this.ranges,
this.resolvedCommentIds,
this.version,
this.pathnameFromProjectStructureUpdate,
this.historyRangesSupport
@ -1026,6 +1045,7 @@ describe('DocumentManager', function () {
lines: this.lines,
version: this.version,
ranges: this.ranges,
resolvedCommentIds: this.resolvedCommentIds,
pathname: this.pathname,
projectHistoryId: this.projectHistoryId,
historyRangesSupport: this.historyRangesSupport,
@ -1057,6 +1077,7 @@ describe('DocumentManager', function () {
this.doc_id,
this.lines,
this.ranges,
this.resolvedCommentIds,
this.version,
this.pathnameFromProjectStructureUpdate,
this.historyRangesSupport

View file

@ -180,6 +180,7 @@ describe('ProjectHistoryRedisManager', function () {
this.ranges = {
changes: [{ op: { i: 'ne', p: 1 } }, { op: { d: 'deleted', p: 3 } }],
}
this.resolvedCommentIds = ['comment-1']
this.version = 2
this.pathname = '/path'
@ -207,6 +208,7 @@ describe('ProjectHistoryRedisManager', function () {
this.doc_id,
this.lines,
this.ranges,
this.resolvedCommentIds,
this.version,
this.pathname,
false
@ -240,6 +242,7 @@ describe('ProjectHistoryRedisManager', function () {
this.doc_id,
this.lines,
this.ranges,
this.resolvedCommentIds,
this.version,
this.pathname,
false
@ -261,6 +264,7 @@ describe('ProjectHistoryRedisManager', function () {
content: 'onedeleted\ntwo',
version: this.version,
ranges: this.ranges,
resolvedCommentIds: this.resolvedCommentIds,
},
projectHistoryId: this.projectHistoryId,
path: this.pathname,
@ -274,6 +278,7 @@ describe('ProjectHistoryRedisManager', function () {
this.doc_id,
this.lines,
this.ranges,
this.resolvedCommentIds,
this.version,
this.pathname,
true

View file

@ -66,6 +66,9 @@ describe('RedisManager', function () {
historyRangesSupport() {
return 'HistoryRangesSupport'
},
resolvedCommentIds({ doc_id: docId }) {
return `ResolvedCommentIds:${docId}`
},
},
},
},
@ -112,6 +115,7 @@ describe('RedisManager', function () {
.update(this.jsonlines, 'utf8')
.digest('hex')
this.ranges = { comments: 'mock', entries: 'mock' }
this.resolvedCommentIds = ['comment-1']
this.json_ranges = JSON.stringify(this.ranges)
this.unflushed_time = 12345
this.pathname = '/a/b/c.tex'
@ -131,6 +135,10 @@ describe('RedisManager', function () {
this.rclient.sismember
.withArgs('HistoryRangesSupport', this.docId)
.yields(null, 0)
this.rclient.smembers = sinon.stub()
this.rclient.smembers
.withArgs(`ResolvedCommentIds:${this.docId}`)
.yields(null, this.resolvedCommentIds)
})
describe('successfully', function () {
@ -166,7 +174,8 @@ describe('RedisManager', function () {
this.unflushed_time,
this.lastUpdatedAt,
this.lastUpdatedBy,
this.historyRangesSupport
this.historyRangesSupport,
this.resolvedCommentIds
)
})
@ -274,7 +283,8 @@ describe('RedisManager', function () {
this.unflushed_time,
this.lastUpdatedAt,
this.lastUpdatedBy,
true
true,
this.resolvedCommentIds
)
})
})
@ -771,6 +781,7 @@ describe('RedisManager', function () {
beforeEach(function () {
this.multi.mset = sinon.stub()
this.multi.sadd = sinon.stub()
this.multi.del = sinon.stub()
this.rclient.sadd = sinon.stub().yields()
this.lines = ['one', 'two', 'three', 'これは']
this.version = 42
@ -779,6 +790,7 @@ describe('RedisManager', function () {
.update(JSON.stringify(this.lines), 'utf8')
.digest('hex')
this.ranges = { comments: 'mock', entries: 'mock' }
this.resolvedCommentIds = ['comment-1']
this.pathname = '/a/b/c.tex'
})
@ -790,6 +802,7 @@ describe('RedisManager', function () {
this.lines,
this.version,
this.ranges,
this.resolvedCommentIds,
this.pathname,
this.projectHistoryId,
this.historyRangesSupport,
@ -824,6 +837,12 @@ describe('RedisManager', function () {
it('should not add the document to the HistoryRangesSupport set in Redis', function () {
this.multi.sadd.should.not.have.been.calledWith('HistoryRangesSupport')
})
it('should not store the resolved comments in Redis', function () {
this.multi.sadd.should.not.have.been.calledWith(
`ResolvedCommentIds:${this.docId}`
)
})
})
describe('with empty ranges', function () {
@ -834,6 +853,7 @@ describe('RedisManager', function () {
this.lines,
this.version,
{},
[],
this.pathname,
this.projectHistoryId,
this.historyRangesSupport,
@ -865,6 +885,7 @@ describe('RedisManager', function () {
this.lines,
this.version,
this.ranges,
this.resolvedCommentIds,
this.pathname,
this.projectHistoryId,
this.historyRangesSupport,
@ -898,6 +919,7 @@ describe('RedisManager', function () {
this.lines,
this.version,
this.ranges,
this.resolvedCommentIds,
this.pathname,
this.projectHistoryId,
this.historyRangesSupport,
@ -925,6 +947,7 @@ describe('RedisManager', function () {
this.lines,
this.version,
this.ranges,
this.resolvedCommentIds,
this.pathname,
this.projectHistoryId,
this.historyRangesSupport,
@ -938,6 +961,16 @@ describe('RedisManager', function () {
this.docId
)
})
it('should store the resolved comments in Redis', function () {
this.multi.del.should.have.been.calledWith(
`ResolvedCommentIds:${this.docId}`
)
this.multi.sadd.should.have.been.calledWith(
`ResolvedCommentIds:${this.docId}`,
...this.resolvedCommentIds
)
})
})
})
@ -966,7 +999,8 @@ describe('RedisManager', function () {
`ProjectHistoryId:${this.docId}`,
`UnflushedTime:${this.docId}`,
`lastUpdatedAt:${this.docId}`,
`lastUpdatedBy:${this.docId}`
`lastUpdatedBy:${this.docId}`,
`ResolvedCommentIds:${this.docId}`
)
.should.equal(true)
})

View file

@ -716,8 +716,8 @@ class SyncUpdateExpander {
async queueUpdatesForOutOfSyncComments(update, pathname, persistedComments) {
const expectedContent = update.resyncDocContent.content
const expectedComments = update.resyncDocContent.ranges?.comments ?? []
const resolvedComments = new Set(
update.resyncDocContent.resolvedComments ?? []
const resolvedCommentIds = new Set(
update.resyncDocContent.resolvedCommentIds ?? []
)
const expectedCommentsById = new Map(
expectedComments.map(comment => [comment.id, comment])
@ -743,11 +743,11 @@ class SyncUpdateExpander {
for (const expectedComment of expectedComments) {
const persistedComment = persistedCommentsById.get(expectedComment.id)
const expectedCommentResolved = resolvedCommentIds.has(expectedComment.id)
if (
persistedComment != null &&
commentRangesAreInSync(persistedComment, expectedComment)
) {
const expectedCommentResolved = resolvedComments.has(expectedComment.id)
if (expectedCommentResolved === persistedComment.resolved) {
// Both comments are identical; do nothing
} else {
@ -756,13 +756,19 @@ class SyncUpdateExpander {
pathname,
commentId: expectedComment.id,
resolved: expectedCommentResolved,
meta: {
resync: true,
origin: this.origin,
ts: update.meta.ts,
},
})
}
} else {
const op = { ...expectedComment.op, resolved: expectedCommentResolved }
// New comment or ranges differ
this.expandedUpdates.push({
doc: update.doc,
op: [expectedComment.op],
op: [op],
meta: {
resync: true,
origin: this.origin,

View file

@ -278,7 +278,7 @@ class OperationsBuilder {
this.pushTextOperation()
// Add a comment operation
this.operations.push({
const commentOp = {
pathname: this.pathname,
commentId: op.t,
ranges: [
@ -287,7 +287,11 @@ class OperationsBuilder {
length: op.hlen ?? op.c.length,
},
],
})
}
if ('resolved' in op) {
commentOp.resolved = op.resolved
}
this.operations.push(commentOp)
return
}

View file

@ -81,7 +81,7 @@ export type ResyncDocContentUpdate = {
content: string
version: number
ranges?: Ranges
resolvedComments?: string[]
resolvedCommentIds?: string[]
}
projectHistoryId: string
path: string
@ -129,6 +129,7 @@ export type CommentOp = {
t: string
hpos?: number
hlen?: number
resolved?: boolean
}
export type UpdateWithBlob = {

View file

@ -24,7 +24,7 @@ function docContentSyncUpdate(
doc,
content,
ranges = {},
resolvedComments = []
resolvedCommentIds = []
) {
return {
path: doc.path,
@ -33,7 +33,7 @@ function docContentSyncUpdate(
resyncDocContent: {
content,
ranges,
resolvedComments,
resolvedCommentIds,
},
meta: {
@ -1096,7 +1096,7 @@ describe('SyncManager', function () {
makeComment('comment1', 4, 'quick'),
makeComment('comment2', 10, 'brown'),
]
this.resolvedComments = ['comment2']
this.resolvedCommentIds = ['comment2']
})
it('does nothing if comments have not changed', async function () {
@ -1107,7 +1107,7 @@ describe('SyncManager', function () {
{
comments: this.comments,
},
this.resolvedComments
this.resolvedCommentIds
),
]
const expandedUpdates =
@ -1129,7 +1129,7 @@ describe('SyncManager', function () {
{
comments: this.comments,
},
this.resolvedComments
this.resolvedCommentIds
),
]
const expandedUpdates =
@ -1147,6 +1147,7 @@ describe('SyncManager', function () {
c: 'jumps',
p: 20,
t: 'comment3',
resolved: false,
},
],
meta: {
@ -1171,7 +1172,7 @@ describe('SyncManager', function () {
{
comments: this.comments,
},
this.resolvedComments
this.resolvedCommentIds
),
]
const expandedUpdates =
@ -1205,7 +1206,7 @@ describe('SyncManager', function () {
{
comments: this.comments,
},
this.resolvedComments
this.resolvedCommentIds
),
]
const expandedUpdates =
@ -1223,6 +1224,7 @@ describe('SyncManager', function () {
c: 'fox',
p: 16,
t: 'comment2',
resolved: true,
},
],
meta: {
@ -1239,7 +1241,7 @@ describe('SyncManager', function () {
})
it('sets the resolved state when it differs', async function () {
this.resolvedComments = ['comment1']
this.resolvedCommentIds = ['comment1']
const updates = [
docContentSyncUpdate(
this.persistedDoc,
@ -1247,7 +1249,7 @@ describe('SyncManager', function () {
{
comments: this.comments,
},
this.resolvedComments
this.resolvedCommentIds
),
]
const expandedUpdates =
@ -1262,11 +1264,25 @@ describe('SyncManager', function () {
pathname: this.persistedDoc.path,
commentId: 'comment1',
resolved: true,
meta: {
origin: {
kind: 'history-resync',
},
resync: true,
ts: TIMESTAMP,
},
},
{
pathname: this.persistedDoc.path,
commentId: 'comment2',
resolved: false,
meta: {
origin: {
kind: 'history-resync',
},
resync: true,
ts: TIMESTAMP,
},
},
])
})