mirror of
https://github.com/yu-i-i/overleaf-cep.git
synced 2026-11-12 21:23:45 +01:00
[web/document-updater] display preview for accepted/rejected tracked changes (#36240)
* [web] add preview payload to track-change notification types * [doc-updater] pass accepted/rejected change data through to web for previews * [web] snapshot change preview onto accepted/rejected notifications * [web] render resolved change previews instead of accepted/rejected count lines * [web] add unit coverage for accepted/rejected change preview flow * fix: notification carries multiple previews per accept/reject batch; extract shared group helper * fix: one preview per accept/reject action, drop adjacency grouping * fix: move buildResolvedChangePreviews out of the email builder class * fix: type error, make tests more explicit * fix: render resolver below preview * fix: overflow text, alignment * fix: test error * fix: resolved count * [document-updater/web] send sparse change preview instead of full doc lines * fix: store sparse preview, hydrate at render, drop CE gate * fix: advance slice cursor past tracked-delete text in sparse preview * fix: inline buildChangeGroupPreview * fix: changePreviewBuilder -> sparseChangePreview * fix: rm change_contributors * fix: copilot flagged testing issues * fix: hydrate and truncate change preview at write time * fix: avoid materializing full doc text when slicing preview window * fix: update reject preview test to hydrated shape * fix: accepted delete removing context * [document-updater/web] split resolved change previews into one per change cluster * fix: narrow cluster userIds to string[] for typecheck * feat: updating to use zod schema for previews --------- Co-authored-by: Jimmy Domagala-Tang <jimmy.domagala-tang@overleaf.com> GitOrigin-RevId: 0740713c715a61a220180f353e497b27da6a5b0f
This commit is contained in:
Kristina
committed by
Copybot
Copybot
co-authored by
Jimmy Domagala-Tang
Jimmy Domagala-Tang
parent
3e0a54c917
commit
9c005e4e54
@@ -13,6 +13,7 @@ const { extractOriginOrSource } = require('./Utils')
|
||||
const { getTotalSizeOfLines } = require('./Limits')
|
||||
const Settings = require('@overleaf/settings')
|
||||
const RangesTracker = require('@overleaf/ranges-tracker')
|
||||
const { buildSparseChangePreviews } = require('./TrackedChangePreview')
|
||||
const {
|
||||
StringFileData,
|
||||
SetCommentStateOperation,
|
||||
@@ -350,7 +351,6 @@ const DocumentManager = {
|
||||
if (changeIds == null) {
|
||||
changeIds = []
|
||||
}
|
||||
let changeContributors = []
|
||||
|
||||
const {
|
||||
lines,
|
||||
@@ -368,6 +368,19 @@ const DocumentManager = {
|
||||
throw new Errors.OTTypeMismatchError('sharejs-text-ot', 'history-ot')
|
||||
}
|
||||
|
||||
// Snapshot the accepted change objects and lines before applying the
|
||||
// accept, so we can build the email-notification previews from them.
|
||||
const acceptedChanges = (ranges.changes || []).filter(change =>
|
||||
changeIds.includes(change.id)
|
||||
)
|
||||
const changeContributors = acceptedChanges
|
||||
.map(change => change?.metadata?.user_id)
|
||||
.filter(userId => userId)
|
||||
const previews = buildSparseChangePreviews({
|
||||
changes: acceptedChanges,
|
||||
lines,
|
||||
})
|
||||
|
||||
const newRanges = RangesManager.acceptChanges(
|
||||
projectId,
|
||||
docId,
|
||||
@@ -396,20 +409,14 @@ const DocumentManager = {
|
||||
projectHistoryId,
|
||||
})
|
||||
|
||||
if (historyUpdates.length === 0) {
|
||||
return changeContributors
|
||||
if (historyUpdates.length > 0) {
|
||||
await ProjectHistoryRedisManager.promises.queueOps(
|
||||
projectId,
|
||||
...historyUpdates.map(op => JSON.stringify(op))
|
||||
)
|
||||
}
|
||||
|
||||
await ProjectHistoryRedisManager.promises.queueOps(
|
||||
projectId,
|
||||
...historyUpdates.map(op => JSON.stringify(op))
|
||||
)
|
||||
}
|
||||
changeContributors = (ranges.changes || [])
|
||||
.filter(change => changeIds.includes(change.id))
|
||||
.map(change => change?.metadata?.user_id)
|
||||
.filter(userId => userId)
|
||||
return changeContributors
|
||||
return { changeContributors, previews }
|
||||
},
|
||||
|
||||
async updateCommentState(projectId, docId, commentId, userId, resolved) {
|
||||
@@ -716,13 +723,12 @@ const DocumentManager = {
|
||||
|
||||
async acceptChangesWithLock(projectId, docId, changeIds) {
|
||||
const UpdateManager = require('./UpdateManager')
|
||||
const changeContributors = await UpdateManager.promises.lockUpdatesAndDo(
|
||||
return await UpdateManager.promises.lockUpdatesAndDo(
|
||||
DocumentManager.acceptChanges,
|
||||
projectId,
|
||||
docId,
|
||||
changeIds
|
||||
)
|
||||
return changeContributors
|
||||
},
|
||||
|
||||
async updateCommentStateWithLock(
|
||||
|
||||
@@ -513,7 +513,7 @@ async function acceptChanges(req, res) {
|
||||
`accepting ${changeIds.length} changes via http`
|
||||
)
|
||||
const timer = new Metrics.Timer('http.acceptChanges')
|
||||
const changeContributors =
|
||||
const { changeContributors, previews } =
|
||||
await DocumentManager.promises.acceptChangesWithLock(
|
||||
projectId,
|
||||
docId,
|
||||
@@ -525,7 +525,7 @@ async function acceptChanges(req, res) {
|
||||
`accepted ${changeIds.length} changes via http`
|
||||
)
|
||||
|
||||
res.status(200).json({ changeContributors })
|
||||
res.status(200).json({ changeContributors, previews })
|
||||
}
|
||||
|
||||
const commentWithUserSchema = z.object({
|
||||
|
||||
@@ -0,0 +1,249 @@
|
||||
// @ts-check
|
||||
|
||||
/**
|
||||
* Builds sparse previews of an accept/reject tracked-change batch. A batch can
|
||||
* cover changes spread across the whole document, so the changes are first
|
||||
* clustered by proximity: each cluster becomes one preview, with its own
|
||||
* section path, start line and bounded slice of surrounding text. Web
|
||||
* assembles the diff, before/after context and truncation from that.
|
||||
*
|
||||
* The clustering gap must stay in step with MERGE_MAX_GAP in
|
||||
* services/web/modules/notifications/app/src/emails/changeBlock.mjs, which
|
||||
* groups pending (not yet resolved) changes the same way.
|
||||
*
|
||||
* A small amount of logic here (section-header pattern, line lookup) is also
|
||||
* present in services/web/modules/notifications/app/src/emails/previewHelpers.mjs
|
||||
*/
|
||||
|
||||
// Matches LaTeX sectioning commands: command name (1), short title (2), full
|
||||
// title (3). Handles starred variants and one level of nested braces.
|
||||
const SECTION_HEADER_PATTERN =
|
||||
/^\s*\\(part|chapter|section|subsection|subsubsection|paragraph|subparagraph)\*?(?:\[([^\]]*)\])?\{((?:[^{}]|\{[^{}]*\})*)\}/
|
||||
|
||||
const SECTION_LEVELS = {
|
||||
part: 0,
|
||||
chapter: 1,
|
||||
section: 2,
|
||||
subsection: 3,
|
||||
subsubsection: 4,
|
||||
paragraph: 5,
|
||||
subparagraph: 6,
|
||||
}
|
||||
|
||||
const CONTEXT_PADDING_CHARS = 500
|
||||
|
||||
// Maximum gap, in characters, between two changes that still read as one
|
||||
// preview.
|
||||
const MERGE_MAX_GAP = 50
|
||||
|
||||
/**
|
||||
* @typedef {{ id?: string, op: { i?: string, d?: string, p: number }, metadata?: { user_id?: string } }} Change
|
||||
*/
|
||||
|
||||
/**
|
||||
* @typedef {object} SparseChangePreview
|
||||
* @property {string[]} sectionPath
|
||||
* @property {number} startLine
|
||||
* @property {{ i?: string, d?: string, p: number }[]} changes
|
||||
* @property {string} slice
|
||||
* @property {number} sliceStart
|
||||
* @property {string[]} userIds - authors of the changes in this cluster
|
||||
*/
|
||||
|
||||
/**
|
||||
* Length a change occupies in document-content coordinate space. Inserts are
|
||||
* present in the document; deletes are zero-width (their text is no longer
|
||||
* there, so it doesn't shift later offsets). See ranges-tracker/index.cjs.
|
||||
* @param {{ i?: string }} op
|
||||
* @returns {number}
|
||||
*/
|
||||
function contentLength(op) {
|
||||
return op.i?.length || 0
|
||||
}
|
||||
|
||||
/**
|
||||
* Character offset of the first character of each line, so position lookups
|
||||
* don't re-walk the document for every cluster.
|
||||
* @param {string[]} lines
|
||||
* @returns {number[]}
|
||||
*/
|
||||
function buildLineStarts(lines) {
|
||||
const starts = new Array(lines.length)
|
||||
let offset = 0
|
||||
for (let i = 0; i < lines.length; i++) {
|
||||
starts[i] = offset
|
||||
offset += lines[i].length + 1 // +1 for \n
|
||||
}
|
||||
return starts
|
||||
}
|
||||
|
||||
/**
|
||||
* @param {number[]} lineStarts
|
||||
* @param {number} charPosition
|
||||
* @returns {number} 0-based line index
|
||||
*/
|
||||
function charPositionToLineIndex(lineStarts, charPosition) {
|
||||
let low = 0
|
||||
let high = lineStarts.length - 1
|
||||
while (low < high) {
|
||||
const mid = Math.ceil((low + high) / 2)
|
||||
if (lineStarts[mid] > charPosition) {
|
||||
high = mid - 1
|
||||
} else {
|
||||
low = mid
|
||||
}
|
||||
}
|
||||
return low
|
||||
}
|
||||
|
||||
/**
|
||||
* Collects every sectioning header in one pass, so per-cluster section lookup
|
||||
* doesn't rescan the document body.
|
||||
* @param {string[]} lines
|
||||
* @returns {{ lineIndex: number, level: number, title: string }[]}
|
||||
*/
|
||||
function collectSectionHeaders(lines) {
|
||||
const headers = []
|
||||
for (let i = 0; i < lines.length; i++) {
|
||||
const match = lines[i].match(SECTION_HEADER_PATTERN)
|
||||
if (!match) {
|
||||
continue
|
||||
}
|
||||
headers.push({
|
||||
lineIndex: i,
|
||||
level: SECTION_LEVELS[match[1]],
|
||||
title: match[2] || match[3],
|
||||
})
|
||||
}
|
||||
return headers
|
||||
}
|
||||
|
||||
/**
|
||||
* @param {{ lineIndex: number, level: number, title: string }[]} headers
|
||||
* @param {number} lineIndex
|
||||
* @returns {string[]}
|
||||
*/
|
||||
function sectionPathAtLine(headers, lineIndex) {
|
||||
const ancestors = []
|
||||
let maxLevel = Infinity
|
||||
for (let i = headers.length - 1; i >= 0; i--) {
|
||||
const header = headers[i]
|
||||
if (header.lineIndex > lineIndex) {
|
||||
continue
|
||||
}
|
||||
if (header.level >= maxLevel) {
|
||||
continue
|
||||
}
|
||||
ancestors.push(header.title)
|
||||
maxLevel = header.level
|
||||
if (header.level === 0) {
|
||||
break
|
||||
}
|
||||
}
|
||||
return ancestors.reverse()
|
||||
}
|
||||
|
||||
/**
|
||||
* Groups changes that sit close enough together to read as one preview.
|
||||
* @param {Change[]} changes
|
||||
* @returns {Change[][]}
|
||||
*/
|
||||
function clusterChanges(changes) {
|
||||
const sorted = [...changes].sort((a, b) => a.op.p - b.op.p)
|
||||
const clusters = [[sorted[0]]]
|
||||
for (let i = 1; i < sorted.length; i++) {
|
||||
const change = sorted[i]
|
||||
const cluster = clusters[clusters.length - 1]
|
||||
const previous = cluster[cluster.length - 1]
|
||||
const previousEnd = previous.op.p + contentLength(previous.op)
|
||||
if (change.op.p - previousEnd <= MERGE_MAX_GAP) {
|
||||
cluster.push(change)
|
||||
} else {
|
||||
clusters.push([change])
|
||||
}
|
||||
}
|
||||
return clusters
|
||||
}
|
||||
|
||||
/**
|
||||
* Builds the sparse change previews that web hydrates into stored previews,
|
||||
* one per cluster of nearby changes.
|
||||
*
|
||||
* @param {object} opts
|
||||
* @param {Change[]} opts.changes - ranges-format change records
|
||||
* @param {string[]} opts.lines - pre-change doc lines snapshot
|
||||
* @returns {SparseChangePreview[]}
|
||||
*/
|
||||
function buildSparseChangePreviews({ changes, lines }) {
|
||||
if (!changes || !changes.length || !lines || !lines.length) {
|
||||
return []
|
||||
}
|
||||
|
||||
const lineStarts = buildLineStarts(lines)
|
||||
const headers = collectSectionHeaders(lines)
|
||||
|
||||
return clusterChanges(changes).map(cluster => {
|
||||
const firstPos = cluster[0].op.p
|
||||
const lastChange = cluster[cluster.length - 1]
|
||||
const lastEnd = lastChange.op.p + contentLength(lastChange.op)
|
||||
const lineIndex = charPositionToLineIndex(lineStarts, firstPos)
|
||||
|
||||
const windowStart = Math.max(0, firstPos - CONTEXT_PADDING_CHARS)
|
||||
const windowEnd = lastEnd + CONTEXT_PADDING_CHARS
|
||||
|
||||
const userIds = [
|
||||
...new Set(
|
||||
cluster.flatMap(change =>
|
||||
change.metadata?.user_id ? [change.metadata.user_id] : []
|
||||
)
|
||||
),
|
||||
]
|
||||
|
||||
return {
|
||||
sectionPath: sectionPathAtLine(headers, lineIndex),
|
||||
startLine: lineIndex + 1,
|
||||
changes: cluster.map(change => ({
|
||||
i: change.op.i,
|
||||
d: change.op.d,
|
||||
p: change.op.p,
|
||||
})),
|
||||
slice: extractWindow(lines, lineStarts, windowStart, windowEnd),
|
||||
sliceStart: windowStart,
|
||||
userIds,
|
||||
}
|
||||
})
|
||||
}
|
||||
|
||||
/**
|
||||
* Extracts `lines.join('\n').slice(windowStart, windowEnd)` without
|
||||
* materializing the full joined text.
|
||||
* @param {string[]} lines
|
||||
* @param {number[]} lineStarts
|
||||
* @param {number} windowStart
|
||||
* @param {number} windowEnd
|
||||
* @returns {string}
|
||||
*/
|
||||
function extractWindow(lines, lineStarts, windowStart, windowEnd) {
|
||||
let out = ''
|
||||
const firstLine = charPositionToLineIndex(lineStarts, windowStart)
|
||||
for (let i = firstLine; i < lines.length; i++) {
|
||||
const line = lines[i]
|
||||
const lineStart = lineStarts[i]
|
||||
const lineEnd = lineStart + line.length
|
||||
if (lineStart >= windowEnd) {
|
||||
break
|
||||
}
|
||||
if (lineEnd > windowStart) {
|
||||
const from = Math.max(0, windowStart - lineStart)
|
||||
const to = Math.min(line.length, windowEnd - lineStart)
|
||||
out += line.slice(from, to)
|
||||
}
|
||||
const hasTrailingNewline = i < lines.length - 1
|
||||
if (hasTrailingNewline && lineEnd >= windowStart && lineEnd < windowEnd) {
|
||||
out += '\n'
|
||||
}
|
||||
}
|
||||
return out
|
||||
}
|
||||
|
||||
module.exports = { buildSparseChangePreviews }
|
||||
@@ -17,6 +17,7 @@ const WebApiManager = require('./WebApiManager')
|
||||
const Profiler = require('./Profiler')
|
||||
const { isInsert, isDelete, getDocLength, computeDocHash } = require('./Utils')
|
||||
const HistoryOTUpdateManager = require('./HistoryOTUpdateManager')
|
||||
const { buildSparseChangePreviews } = require('./TrackedChangePreview')
|
||||
|
||||
/**
|
||||
* @import { Ranges, Update, HistoryUpdate } from "./types"
|
||||
@@ -214,9 +215,16 @@ const UpdateManager = {
|
||||
// Look up the authors of those rejected changes from the pre-update
|
||||
// ranges so we can notify web below.
|
||||
if (removedChangeIds.length > 0) {
|
||||
const rejectedChangeAuthorIds = (ranges?.changes || [])
|
||||
.filter(change => removedChangeIds.includes(change.id))
|
||||
.map(change => change.metadata.user_id)
|
||||
const rejectedChanges = (ranges?.changes || []).filter(change =>
|
||||
removedChangeIds.includes(change.id)
|
||||
)
|
||||
const rejectedChangeAuthorIds = rejectedChanges.map(
|
||||
change => change.metadata.user_id
|
||||
)
|
||||
const previews = buildSparseChangePreviews({
|
||||
changes: rejectedChanges,
|
||||
lines,
|
||||
})
|
||||
|
||||
// Fire-and-forget without awaiting because
|
||||
// we hold the project lock here, and the result of the
|
||||
@@ -226,7 +234,8 @@ const UpdateManager = {
|
||||
projectId,
|
||||
docId,
|
||||
rejectedChangeAuthorIds,
|
||||
update.meta?.user_id
|
||||
update.meta?.user_id,
|
||||
previews
|
||||
)
|
||||
.catch(err => {
|
||||
logger.warn(
|
||||
|
||||
@@ -9,12 +9,14 @@ const MAX_HTTP_REQUEST_LENGTH = 5000
|
||||
* @param {string} docId
|
||||
* @param {string[]} rejectedChangeAuthorIds
|
||||
* @param {string | undefined} userId
|
||||
* @param {object[]} [previews] sparse change previews from TrackedChangePreview.buildSparseChangePreviews
|
||||
*/
|
||||
async function notifyTrackChangesRejected(
|
||||
projectId,
|
||||
docId,
|
||||
rejectedChangeAuthorIds,
|
||||
userId
|
||||
userId,
|
||||
previews
|
||||
) {
|
||||
const url = new URL(
|
||||
`/project/${projectId}/doc/${docId}/changes/reject`,
|
||||
@@ -23,7 +25,11 @@ async function notifyTrackChangesRejected(
|
||||
|
||||
await fetchNothing(url, {
|
||||
method: 'POST',
|
||||
json: { rejectedChangeAuthorIds, userId },
|
||||
json: {
|
||||
rejectedChangeAuthorIds,
|
||||
userId,
|
||||
previews,
|
||||
},
|
||||
basicAuth: {
|
||||
user: Settings.apis.web.user,
|
||||
password: Settings.apis.web.pass,
|
||||
|
||||
@@ -61,6 +61,7 @@ describe('DocumentManager', function () {
|
||||
maxUnflushedAgeMs: 300 * 1000, // 5 minutes
|
||||
}
|
||||
|
||||
this.buildSparseChangePreviews = sinon.stub().returns([])
|
||||
this.DocumentManager = SandboxedModule.require(modulePath, {
|
||||
requires: {
|
||||
'./RedisManager': this.RedisManager,
|
||||
@@ -73,6 +74,9 @@ describe('DocumentManager', function () {
|
||||
'./HistoryOTUpdateManager': this.HistoryOTUpdateManager,
|
||||
'./RangesManager': this.RangesManager,
|
||||
'./Errors': Errors,
|
||||
'./TrackedChangePreview': {
|
||||
buildSparseChangePreviews: this.buildSparseChangePreviews,
|
||||
},
|
||||
'@overleaf/settings': this.Settings,
|
||||
},
|
||||
})
|
||||
@@ -849,7 +853,7 @@ describe('DocumentManager', function () {
|
||||
})
|
||||
|
||||
it('should return the change contributors', function () {
|
||||
expect(this.result).to.deep.equal(['mock-user-id-0'])
|
||||
expect(this.result.changeContributors).to.deep.equal(['mock-user-id-0'])
|
||||
})
|
||||
})
|
||||
|
||||
@@ -874,7 +878,7 @@ describe('DocumentManager', function () {
|
||||
})
|
||||
|
||||
it('should return the change contributors', function () {
|
||||
expect(this.result).to.deep.equal([
|
||||
expect(this.result.changeContributors).to.deep.equal([
|
||||
'mock-user-id-1',
|
||||
'mock-user-id-2',
|
||||
'mock-user-id-3',
|
||||
@@ -883,6 +887,40 @@ describe('DocumentManager', function () {
|
||||
})
|
||||
})
|
||||
|
||||
describe('preview building', function () {
|
||||
beforeEach(async function () {
|
||||
this.mockPreviews = [
|
||||
{
|
||||
sectionPath: ['Intro'],
|
||||
startLine: 1,
|
||||
changes: [{ i: 'x', p: 0 }],
|
||||
slice: 'x',
|
||||
sliceStart: 0,
|
||||
userIds: ['mock-user-id-0'],
|
||||
},
|
||||
]
|
||||
this.buildSparseChangePreviews.returns(this.mockPreviews)
|
||||
this.result = await this.DocumentManager.promises.acceptChanges(
|
||||
this.project_id,
|
||||
this.doc_id,
|
||||
[this.change_id]
|
||||
)
|
||||
})
|
||||
|
||||
it('should call buildSparseChangePreviews with the accepted changes and doc lines', function () {
|
||||
this.buildSparseChangePreviews.should.have.been.calledWith({
|
||||
changes: [
|
||||
{ id: 'mock-change-id', metadata: { user_id: 'mock-user-id-0' } },
|
||||
],
|
||||
lines: this.lines,
|
||||
})
|
||||
})
|
||||
|
||||
it('should return the previews from the helper', function () {
|
||||
expect(this.result.previews).to.equal(this.mockPreviews)
|
||||
})
|
||||
})
|
||||
|
||||
describe('when the doc is not found', function () {
|
||||
beforeEach(async function () {
|
||||
this.DocumentManager.promises.getDoc = sinon
|
||||
|
||||
@@ -692,9 +692,20 @@ describe('HttpController', function () {
|
||||
describe('successfully with a single change', function () {
|
||||
beforeEach(async function () {
|
||||
this.changeContributors = ['user-id-1', 'user-id-2']
|
||||
this.DocumentManager.promises.acceptChangesWithLock.resolves(
|
||||
this.changeContributors
|
||||
)
|
||||
this.previews = [
|
||||
{
|
||||
sectionPath: [],
|
||||
startLine: 1,
|
||||
changes: [{ i: 'x', p: 0 }],
|
||||
slice: 'x',
|
||||
sliceStart: 0,
|
||||
userIds: ['user-id-1'],
|
||||
},
|
||||
]
|
||||
this.DocumentManager.promises.acceptChangesWithLock.resolves({
|
||||
changeContributors: this.changeContributors,
|
||||
previews: this.previews,
|
||||
})
|
||||
await this.HttpController.acceptChanges(this.req, this.res, this.next)
|
||||
})
|
||||
|
||||
@@ -706,10 +717,11 @@ describe('HttpController', function () {
|
||||
)
|
||||
})
|
||||
|
||||
it('should return a successful 200 with a list of the change contributors', function () {
|
||||
it('should return a successful 200 with the contributors and previews', function () {
|
||||
this.res.status.should.have.been.calledWith(200)
|
||||
this.res.json.should.have.been.calledWith({
|
||||
changeContributors: this.changeContributors,
|
||||
previews: this.previews,
|
||||
})
|
||||
})
|
||||
|
||||
@@ -736,6 +748,10 @@ describe('HttpController', function () {
|
||||
'ddddddddddddddddddddddd4',
|
||||
]
|
||||
this.req.body = { change_ids: this.change_ids }
|
||||
this.DocumentManager.promises.acceptChangesWithLock.resolves({
|
||||
changeContributors: [],
|
||||
previews: [],
|
||||
})
|
||||
await this.HttpController.acceptChanges(this.req, this.res, this.next)
|
||||
})
|
||||
|
||||
|
||||
@@ -0,0 +1,206 @@
|
||||
const { expect } = require('chai')
|
||||
const {
|
||||
buildSparseChangePreviews,
|
||||
} = require('../../../app/js/TrackedChangePreview')
|
||||
|
||||
describe('TrackedChangePreview', function () {
|
||||
describe('buildSparseChangePreviews', function () {
|
||||
it('returns no previews for empty inputs', function () {
|
||||
expect(
|
||||
buildSparseChangePreviews({ changes: [], lines: ['a'] })
|
||||
).to.deep.equal([])
|
||||
expect(
|
||||
buildSparseChangePreviews({ changes: null, lines: ['a'] })
|
||||
).to.deep.equal([])
|
||||
expect(
|
||||
buildSparseChangePreviews({
|
||||
changes: [{ op: { i: 'x', p: 0 } }],
|
||||
lines: null,
|
||||
})
|
||||
).to.deep.equal([])
|
||||
})
|
||||
|
||||
it('finds LaTeX section path from the enclosing header', function () {
|
||||
const lines = [
|
||||
'\\section{Intro}',
|
||||
'Some intro text.',
|
||||
'\\subsection{Details}',
|
||||
'target line here',
|
||||
]
|
||||
const position = lines.slice(0, 3).join('\n').length + 5
|
||||
const [preview] = buildSparseChangePreviews({
|
||||
changes: [{ op: { i: 'x', p: position } }],
|
||||
lines,
|
||||
})
|
||||
expect(preview.sectionPath).to.deep.equal(['Intro', 'Details'])
|
||||
})
|
||||
|
||||
it('returns empty sectionPath when no header precedes the change', function () {
|
||||
const lines = ['plain text', 'more text']
|
||||
const [preview] = buildSparseChangePreviews({
|
||||
changes: [{ op: { i: 'x', p: 5 } }],
|
||||
lines,
|
||||
})
|
||||
expect(preview.sectionPath).to.deep.equal([])
|
||||
})
|
||||
|
||||
it('computes 1-based startLine for the first change', function () {
|
||||
const lines = ['first', 'second', 'third']
|
||||
const [preview] = buildSparseChangePreviews({
|
||||
changes: [{ op: { i: 'x', p: 'first\nsecond\n'.length + 1 } }],
|
||||
lines,
|
||||
})
|
||||
expect(preview.startLine).to.equal(3)
|
||||
})
|
||||
|
||||
it('attributes a position on a line-terminating newline to that line', function () {
|
||||
const lines = ['ab', 'cd']
|
||||
const [preview] = buildSparseChangePreviews({
|
||||
changes: [{ op: { i: 'x', p: 2 } }],
|
||||
lines,
|
||||
})
|
||||
expect(preview.startLine).to.equal(1)
|
||||
})
|
||||
|
||||
it('clamps a position past the end of the document to the last line', function () {
|
||||
const lines = ['ab', 'cd']
|
||||
const [preview] = buildSparseChangePreviews({
|
||||
changes: [{ op: { i: 'x', p: 500 } }],
|
||||
lines,
|
||||
})
|
||||
expect(preview.startLine).to.equal(2)
|
||||
})
|
||||
|
||||
it('projects changes to bare ranges-op shape', function () {
|
||||
const [preview] = buildSparseChangePreviews({
|
||||
changes: [
|
||||
{
|
||||
id: 'c1',
|
||||
op: { i: 'hi', p: 0 },
|
||||
metadata: { user_id: 'u1' },
|
||||
},
|
||||
{
|
||||
id: 'c2',
|
||||
op: { d: 'x', p: 5 },
|
||||
metadata: { user_id: 'u2' },
|
||||
},
|
||||
],
|
||||
lines: ['abcdefghij'],
|
||||
})
|
||||
expect(preview.changes).to.deep.equal([
|
||||
{ i: 'hi', d: undefined, p: 0 },
|
||||
{ i: undefined, d: 'x', p: 5 },
|
||||
])
|
||||
})
|
||||
|
||||
it('slices a bounded window of surrounding doc text', function () {
|
||||
const filler = 'x'.repeat(2000)
|
||||
const lines = [filler, filler, filler]
|
||||
const fullText = lines.join('\n')
|
||||
const changePos = filler.length + 1 + 5
|
||||
const [preview] = buildSparseChangePreviews({
|
||||
changes: [{ op: { i: 'y', p: changePos } }],
|
||||
lines,
|
||||
})
|
||||
// slice should be far shorter than the full doc
|
||||
expect(preview.slice.length).to.be.lessThan(fullText.length)
|
||||
expect(preview.sliceStart).to.be.lessThanOrEqual(changePos)
|
||||
const sliceEndPos = preview.sliceStart + preview.slice.length
|
||||
expect(sliceEndPos).to.be.greaterThanOrEqual(changePos)
|
||||
})
|
||||
|
||||
it('treats a tracked delete as zero-width when sizing the window', function () {
|
||||
const lines = ['Overleaf is and features are listed below.']
|
||||
const deletePos = 'Overleaf is '.length
|
||||
const [preview] = buildSparseChangePreviews({
|
||||
changes: [{ op: { d: 'a great tool ', p: deletePos } }],
|
||||
lines,
|
||||
})
|
||||
// The deleted text is not in `lines`, so the window must not extend by
|
||||
// its length past the deletion point.
|
||||
expect(preview.slice).to.equal(lines[0])
|
||||
expect(preview.sliceStart).to.equal(0)
|
||||
})
|
||||
|
||||
describe('clustering', function () {
|
||||
it('keeps nearby changes in one preview', function () {
|
||||
const lines = ['one two three four five six seven eight nine ten']
|
||||
const previews = buildSparseChangePreviews({
|
||||
changes: [
|
||||
{ op: { i: 'a', p: 0 }, metadata: { user_id: 'u1' } },
|
||||
{ op: { i: 'b', p: 20 }, metadata: { user_id: 'u2' } },
|
||||
],
|
||||
lines,
|
||||
})
|
||||
expect(previews).to.have.length(1)
|
||||
expect(previews[0].changes).to.have.length(2)
|
||||
expect(previews[0].userIds).to.deep.equal(['u1', 'u2'])
|
||||
})
|
||||
|
||||
it('splits distant changes into separate previews', function () {
|
||||
const filler = 'x'.repeat(5000)
|
||||
const lines = ['\\section{Top}', filler, '\\section{Bottom}', filler]
|
||||
const topPos = '\\section{Top}\n'.length + 3
|
||||
const bottomPos =
|
||||
`\\section{Top}\n${filler}\n\\section{Bottom}\n`.length + 3
|
||||
const previews = buildSparseChangePreviews({
|
||||
changes: [
|
||||
{ op: { i: 'a', p: topPos }, metadata: { user_id: 'u1' } },
|
||||
{ op: { i: 'b', p: bottomPos }, metadata: { user_id: 'u2' } },
|
||||
],
|
||||
lines,
|
||||
})
|
||||
expect(previews).to.have.length(2)
|
||||
expect(previews[0].sectionPath).to.deep.equal(['Top'])
|
||||
expect(previews[0].startLine).to.equal(2)
|
||||
expect(previews[0].userIds).to.deep.equal(['u1'])
|
||||
expect(previews[1].sectionPath).to.deep.equal(['Bottom'])
|
||||
expect(previews[1].startLine).to.equal(4)
|
||||
expect(previews[1].userIds).to.deep.equal(['u2'])
|
||||
})
|
||||
|
||||
it('starts the slice on a line boundary without losing the newline', function () {
|
||||
const lines = ['first line', 'second line', 'third line']
|
||||
const secondLineStart = 'first line\n'.length
|
||||
const [preview] = buildSparseChangePreviews({
|
||||
changes: [{ op: { i: 'x', p: secondLineStart } }],
|
||||
lines,
|
||||
})
|
||||
expect(preview.sliceStart).to.equal(0)
|
||||
expect(preview.slice).to.equal(lines.join('\n'))
|
||||
})
|
||||
|
||||
it('bounds each preview slice instead of spanning the whole doc', function () {
|
||||
const filler = 'x'.repeat(5000)
|
||||
const lines = [filler, filler, filler]
|
||||
const lastLineStart = (filler.length + 1) * 2
|
||||
const previews = buildSparseChangePreviews({
|
||||
changes: [
|
||||
{ op: { i: 'a', p: 0 } },
|
||||
{ op: { i: 'b', p: lastLineStart + 10 } },
|
||||
],
|
||||
lines,
|
||||
})
|
||||
expect(previews).to.have.length(2)
|
||||
for (const preview of previews) {
|
||||
expect(preview.slice.length).to.be.lessThan(1200)
|
||||
}
|
||||
})
|
||||
|
||||
it('orders previews by position regardless of input order', function () {
|
||||
const lines = ['a'.repeat(100), 'b'.repeat(100), 'c'.repeat(100)]
|
||||
const previews = buildSparseChangePreviews({
|
||||
changes: [
|
||||
{ op: { i: 'late', p: 250 } },
|
||||
{ op: { i: 'early', p: 10 } },
|
||||
],
|
||||
lines,
|
||||
})
|
||||
expect(previews.map(p => p.changes[0].i)).to.deep.equal([
|
||||
'early',
|
||||
'late',
|
||||
])
|
||||
})
|
||||
})
|
||||
})
|
||||
})
|
||||
@@ -92,6 +92,7 @@ describe('UpdateManager', function () {
|
||||
},
|
||||
}
|
||||
|
||||
this.buildSparseChangePreviews = sinon.stub().returns([])
|
||||
this.UpdateManager = SandboxedModule.require(MODULE_PATH, {
|
||||
requires: {
|
||||
'./ProjectLockManager': this.ProjectLockManager,
|
||||
@@ -107,6 +108,9 @@ describe('UpdateManager', function () {
|
||||
'./WebApiManager': this.WebApiManager,
|
||||
'./Profiler': this.Profiler,
|
||||
'./ProjectHistoryRedisManager': this.ProjectHistoryRedisManager,
|
||||
'./TrackedChangePreview': {
|
||||
buildSparseChangePreviews: this.buildSparseChangePreviews,
|
||||
},
|
||||
},
|
||||
})
|
||||
})
|
||||
@@ -610,6 +614,17 @@ describe('UpdateManager', function () {
|
||||
historyUpdates: this.historyUpdates,
|
||||
removedChangeIds: ['change-1', 'change-2'],
|
||||
})
|
||||
this.mockPreviews = [
|
||||
{
|
||||
sectionPath: ['Intro'],
|
||||
startLine: 1,
|
||||
changes: [{ d: 'x', p: 0 }],
|
||||
slice: 'x',
|
||||
sliceStart: 0,
|
||||
userIds: ['author-1', 'author-2'],
|
||||
},
|
||||
]
|
||||
this.buildSparseChangePreviews.returns(this.mockPreviews)
|
||||
await this.UpdateManager.promises.applyUpdate(
|
||||
this.project_id,
|
||||
this.doc_id,
|
||||
@@ -617,12 +632,23 @@ describe('UpdateManager', function () {
|
||||
)
|
||||
})
|
||||
|
||||
it('should notify web of the rejected tracked changes', function () {
|
||||
it('should call buildSparseChangePreviews with the rejected changes and lines', function () {
|
||||
this.buildSparseChangePreviews.should.have.been.calledWith({
|
||||
changes: [
|
||||
{ id: 'change-1', metadata: { user_id: 'author-1' } },
|
||||
{ id: 'change-2', metadata: { user_id: 'author-2' } },
|
||||
],
|
||||
lines: this.lines,
|
||||
})
|
||||
})
|
||||
|
||||
it('should notify web of the rejected tracked changes with the previews', function () {
|
||||
this.WebApiManager.promises.notifyTrackChangesRejected.should.have.been.calledWith(
|
||||
this.project_id,
|
||||
this.doc_id,
|
||||
this.rejectedChangeAuthorIds,
|
||||
this.updateMeta.user_id
|
||||
this.updateMeta.user_id,
|
||||
this.mockPreviews
|
||||
)
|
||||
})
|
||||
})
|
||||
|
||||
@@ -208,7 +208,7 @@ async function clearProjectState(projectId) {
|
||||
* @param {string} userId
|
||||
*/
|
||||
async function acceptChanges(projectId, docId, changeIds, userId) {
|
||||
const { changeContributors } = await fetchJson(
|
||||
const { changeContributors, previews } = await fetchJson(
|
||||
`${BASE_URL}/project/${projectId}/doc/${docId}/change/accept`,
|
||||
{
|
||||
method: 'POST',
|
||||
@@ -221,7 +221,8 @@ async function acceptChanges(projectId, docId, changeIds, userId) {
|
||||
projectId,
|
||||
docId,
|
||||
userId,
|
||||
changeContributors
|
||||
changeContributors,
|
||||
previews
|
||||
)
|
||||
}
|
||||
|
||||
|
||||
@@ -214,6 +214,21 @@ async function setDocument(req, res) {
|
||||
res.json(result)
|
||||
}
|
||||
|
||||
const changePreview = z.strictObject({
|
||||
sectionPath: z.array(z.string()),
|
||||
startLine: z.number().int().min(1),
|
||||
changes: z.array(
|
||||
z.strictObject({
|
||||
i: z.string().optional(),
|
||||
d: z.string().optional(),
|
||||
p: z.number().int().min(0),
|
||||
})
|
||||
),
|
||||
slice: z.string(),
|
||||
sliceStart: z.number().int().min(0),
|
||||
userIds: z.array(zz.objectId()),
|
||||
})
|
||||
|
||||
const trackChangesRejectedSchema = z.object({
|
||||
params: z.strictObject({
|
||||
Project_id: zz.objectId(),
|
||||
@@ -222,6 +237,7 @@ const trackChangesRejectedSchema = z.object({
|
||||
body: z.strictObject({
|
||||
rejectedChangeAuthorIds: z.array(zz.objectId()),
|
||||
userId: zz.objectId().optional(),
|
||||
previews: z.array(changePreview).optional(),
|
||||
}),
|
||||
})
|
||||
|
||||
@@ -230,13 +246,14 @@ async function trackChangesRejected(req, res) {
|
||||
logOnly: true,
|
||||
})
|
||||
const { Project_id: projectId, doc_id: docId } = params
|
||||
const { rejectedChangeAuthorIds, userId } = body
|
||||
const { rejectedChangeAuthorIds, userId, previews } = body
|
||||
await Modules.promises.hooks.fire(
|
||||
'trackChangesRejected',
|
||||
projectId,
|
||||
docId,
|
||||
userId,
|
||||
rejectedChangeAuthorIds
|
||||
rejectedChangeAuthorIds,
|
||||
previews
|
||||
)
|
||||
res.sendStatus(204)
|
||||
}
|
||||
|
||||
@@ -153,6 +153,37 @@ describe('DocumentController', function () {
|
||||
expect(response.statusCode).to.equal(204)
|
||||
})
|
||||
|
||||
it('should accept the previews document-updater sends and return 204', async function () {
|
||||
const response = await privateApiRequest({
|
||||
method: 'post',
|
||||
url: `/project/${projectId}/doc/${docId}/changes/reject`,
|
||||
json: {
|
||||
rejectedChangeAuthorIds: [owner._id.toString()],
|
||||
userId: owner._id.toString(),
|
||||
// the shape buildSparseChangePreviews emits, one entry per cluster
|
||||
previews: [
|
||||
{
|
||||
sectionPath: ['Introduction'],
|
||||
startLine: 2,
|
||||
changes: [{ i: 'inserted', p: 5 }],
|
||||
slice: 'first line\nsecond line inserted',
|
||||
sliceStart: 0,
|
||||
userIds: [owner._id.toString()],
|
||||
},
|
||||
{
|
||||
sectionPath: [],
|
||||
startLine: 404,
|
||||
changes: [{ d: 'removed', p: 5000 }],
|
||||
slice: 'text around the second cluster',
|
||||
sliceStart: 4900,
|
||||
userIds: [owner._id.toString()],
|
||||
},
|
||||
],
|
||||
},
|
||||
})
|
||||
expect(response.statusCode).to.equal(204)
|
||||
})
|
||||
|
||||
it('should reject a non-ObjectId rejectedChangeAuthorIds entry', async function () {
|
||||
const response = await privateApiRequest({
|
||||
method: 'post',
|
||||
|
||||
@@ -581,6 +581,50 @@ describe('DocumentUpdaterHandler', function () {
|
||||
})
|
||||
|
||||
describe('successfully', function () {
|
||||
beforeEach(async function (ctx) {
|
||||
ctx.previews = [
|
||||
{
|
||||
sectionPath: ['Intro'],
|
||||
startLine: 2,
|
||||
changes: [{ i: 'hello', p: 5 }],
|
||||
slice: 'first line\nsecond line hello',
|
||||
sliceStart: 0,
|
||||
userIds: ['mock-user-id-1'],
|
||||
},
|
||||
]
|
||||
ctx.docUpdaterMock
|
||||
.post(`/project/${ctx.project_id}/doc/${ctx.doc_id}/change/accept`, {
|
||||
change_ids: [ctx.change_id],
|
||||
})
|
||||
.reply(200, {
|
||||
changeContributors: ctx.change_contributors,
|
||||
previews: ctx.previews,
|
||||
})
|
||||
await ctx.handler.promises.acceptChanges(
|
||||
ctx.project_id,
|
||||
ctx.doc_id,
|
||||
[ctx.change_id],
|
||||
ctx.user_id
|
||||
)
|
||||
})
|
||||
|
||||
it('should accept the change in the document updater', function (ctx) {
|
||||
expect(ctx.docUpdaterMock.isDone()).to.be.true
|
||||
})
|
||||
|
||||
it('should fire the changesAccepted hook with contributors and previews', function (ctx) {
|
||||
expect(ctx.modulesHooksFire).to.have.been.calledWith(
|
||||
'changesAccepted',
|
||||
ctx.project_id,
|
||||
ctx.doc_id,
|
||||
ctx.user_id,
|
||||
ctx.change_contributors,
|
||||
ctx.previews
|
||||
)
|
||||
})
|
||||
})
|
||||
|
||||
describe('when the document updater does not return preview data', function () {
|
||||
beforeEach(async function (ctx) {
|
||||
ctx.docUpdaterMock
|
||||
.post(`/project/${ctx.project_id}/doc/${ctx.doc_id}/change/accept`, {
|
||||
@@ -595,17 +639,14 @@ describe('DocumentUpdaterHandler', function () {
|
||||
)
|
||||
})
|
||||
|
||||
it('should accept the change in the document updater', function (ctx) {
|
||||
expect(ctx.docUpdaterMock.isDone()).to.be.true
|
||||
})
|
||||
|
||||
it('should fire the changesAccepted hook with change contributors', function (ctx) {
|
||||
it('should fire the changesAccepted hook with undefined preview data', function (ctx) {
|
||||
expect(ctx.modulesHooksFire).to.have.been.calledWith(
|
||||
'changesAccepted',
|
||||
ctx.project_id,
|
||||
ctx.doc_id,
|
||||
ctx.user_id,
|
||||
ctx.change_contributors
|
||||
ctx.change_contributors,
|
||||
undefined
|
||||
)
|
||||
})
|
||||
})
|
||||
|
||||
@@ -2,11 +2,22 @@
|
||||
* Types for module hook events fired across the application
|
||||
*/
|
||||
|
||||
export type SparseChangePreview = {
|
||||
sectionPath: string[]
|
||||
startLine: number
|
||||
changes: { i?: string; d?: string; p: number }[]
|
||||
slice: string
|
||||
sliceStart: number
|
||||
/** authors of the changes in this cluster */
|
||||
userIds: string[]
|
||||
}
|
||||
|
||||
export type TrackChangesAcceptedEvent = {
|
||||
projectId: string
|
||||
docId: string
|
||||
userId: string
|
||||
changeContributors: string[]
|
||||
previews?: SparseChangePreview[] | null
|
||||
}
|
||||
|
||||
export type TrackChangesRejectedEvent = {
|
||||
@@ -14,6 +25,7 @@ export type TrackChangesRejectedEvent = {
|
||||
docId: string
|
||||
userId: string
|
||||
changeContributors: string[]
|
||||
previews?: SparseChangePreview[] | null
|
||||
}
|
||||
|
||||
export type CommentAddedEvent = {
|
||||
|
||||
Reference in New Issue
Block a user