[visual] Disable figure and table editing when read-only (#15349)

GitOrigin-RevId: ac0f9eef7bf2d88afd05689fa89b11716747b970

libraries/promise-utils/.dockerignore

@@ -0,0 +1 @@
+node_modules/

libraries/promise-utils/.gitignore

@@ -0,0 +1,3 @@
+
+# managed by monorepo$ bin/update_build_scripts
+.npmrc

libraries/promise-utils/.mocharc.json

@@ -0,0 +1,5 @@
+{
+  "ui": "bdd",
+  "recursive": "true",
+  "reporter": "spec"
+}

libraries/promise-utils/.nvmrc

@@ -0,0 +1 @@
+18.18.0

libraries/promise-utils/buildscript.txt

@@ -0,0 +1,10 @@
+promise-utils
+--dependencies=None
+--docker-repos=gcr.io/overleaf-ops
+--env-add=
+--env-pass-through=
+--esmock-loader=False
+--is-library=True
+--node-version=18.18.0
+--public-repo=False
+--script-version=4.4.0

libraries/promise-utils/index.js

@@ -0,0 +1,214 @@
+const { promisify, callbackify } = require('util')
+const pLimit = require('p-limit')
+
+module.exports = {
+  promisify,
+  promisifyAll,
+  promisifyClass,
+  promisifyMultiResult,
+  callbackify,
+  callbackifyAll,
+  callbackifyMultiResult,
+  expressify,
+  promiseMapWithLimit,
+}
+
+/**
+ * Promisify all functions in a module.
+ *
+ * This is meant to be used only when all functions in the module are async
+ * callback-style functions.
+ *
+ * It's very much tailored to our current module structure. In particular, it
+ * binds `this` to the module when calling the function in order not to break
+ * modules that call sibling functions using `this`.
+ *
+ * This will not magically fix all modules. Special cases should be promisified
+ * manually.
+ *
+ * The second argument is a bag of options:
+ *
+ * - without: an array of function names that shouldn't be promisified
+ *
+ * - multiResult: an object whose keys are function names and values are lists
+ *   of parameter names. This is meant for functions that invoke their callbacks
+ *   with more than one result in separate parameters. The promisifed function
+ *   will return these results as a single object, with each result keyed under
+ *   the corresponding parameter name.
+ */
+function promisifyAll(module, opts = {}) {
+  const { without = [], multiResult = {} } = opts
+  const promises = {}
+  for (const propName of Object.getOwnPropertyNames(module)) {
+    if (without.includes(propName)) {
+      continue
+    }
+    const propValue = module[propName]
+    if (typeof propValue !== 'function') {
+      continue
+    }
+    if (multiResult[propName] != null) {
+      promises[propName] = promisifyMultiResult(
+        propValue,
+        multiResult[propName]
+      ).bind(module)
+    } else {
+      promises[propName] = promisify(propValue).bind(module)
+    }
+  }
+  return promises
+}
+
+/**
+ * Promisify all methods in a class.
+ *
+ * Options are the same as for promisifyAll
+ */
+function promisifyClass(cls, opts = {}) {
+  const promisified = class extends cls {}
+  const { without = [], multiResult = {} } = opts
+  for (const propName of Object.getOwnPropertyNames(cls.prototype)) {
+    if (propName === 'constructor' || without.includes(propName)) {
+      continue
+    }
+    const propValue = cls.prototype[propName]
+    if (typeof propValue !== 'function') {
+      continue
+    }
+    if (multiResult[propName] != null) {
+      promisified.prototype[propName] = promisifyMultiResult(
+        propValue,
+        multiResult[propName]
+      )
+    } else {
+      promisified.prototype[propName] = promisify(propValue)
+    }
+  }
+  return promisified
+}
+
+/**
+ * Promisify a function that returns multiple results via additional callback
+ * parameters.
+ *
+ * The promisified function returns the results in a single object whose keys
+ * are the names given in the array `resultNames`.
+ *
+ * Example:
+ *
+ *     function f(callback) {
+ *       return callback(null, 1, 2, 3)
+ *     }
+ *
+ *     const g = promisifyMultiResult(f, ['a', 'b', 'c'])
+ *
+ *     const result = await g()  // returns {a: 1, b: 2, c: 3}
+ */
+function promisifyMultiResult(fn, resultNames) {
+  function promisified(...args) {
+    return new Promise((resolve, reject) => {
+      try {
+        fn.bind(this)(...args, (err, ...results) => {
+          if (err != null) {
+            return reject(err)
+          }
+          const promiseResult = {}
+          for (let i = 0; i < resultNames.length; i++) {
+            promiseResult[resultNames[i]] = results[i]
+          }
+          resolve(promiseResult)
+        })
+      } catch (err) {
+        reject(err)
+      }
+    })
+  }
+  return promisified
+}
+
+/**
+ * Reverse of `promisifyAll`.
+ *
+ * Callbackify all async functions in a module and return them in an object. In
+ * contrast with `promisifyAll`, all other exports from the module are added to
+ * the result.
+ *
+ * This is meant to be used like this:
+ *
+ *     const MyPromisifiedModule = {...}
+ *     module.exports = {
+ *       ...callbackifyAll(MyPromisifiedModule),
+ *       promises: MyPromisifiedModule
+ *     }
+ *
+ * @param {Object} module - The module to callbackify
+ * @param {Object} opts - Options
+ * @param {Object} opts.multiResult - Spec of methods to be callbackified with
+ *                                    callbackifyMultiResult()
+ */
+function callbackifyAll(module, opts = {}) {
+  const { multiResult = {} } = opts
+  const callbacks = {}
+  for (const propName of Object.getOwnPropertyNames(module)) {
+    const propValue = module[propName]
+    if (typeof propValue === 'function') {
+      if (propValue.constructor.name === 'AsyncFunction') {
+        if (multiResult[propName] != null) {
+          callbacks[propName] = callbackifyMultiResult(
+            propValue,
+            multiResult[propName]
+          ).bind(module)
+        } else {
+          callbacks[propName] = callbackify(propValue).bind(module)
+        }
+      } else {
+        callbacks[propName] = propValue.bind(module)
+      }
+    } else {
+      callbacks[propName] = propValue
+    }
+  }
+  return callbacks
+}
+
+/**
+ * Reverse the effect of `promisifyMultiResult`.
+ *
+ * This is meant for providing a temporary backward compatible callback
+ * interface while we migrate to promises.
+ */
+function callbackifyMultiResult(fn, resultNames) {
+  function callbackified(...args) {
+    const [callback] = args.splice(-1)
+    fn(...args)
+      .then(result => {
+        const cbResults = resultNames.map(resultName => result[resultName])
+        callback(null, ...cbResults)
+      })
+      .catch(err => {
+        callback(err)
+      })
+  }
+  return callbackified
+}
+
+/**
+ * Transform an async function into an Express middleware
+ *
+ * Any error will be passed to the error middlewares via `next()`
+ */
+function expressify(fn) {
+  return (req, res, next) => {
+    fn(req, res, next).catch(next)
+  }
+}
+
+/**
+ * Map values in `array` with the async function `fn`
+ *
+ * Limit the number of unresolved promises to `concurrency`.
+ */
+function promiseMapWithLimit(concurrency, array, fn) {
+  const limit = pLimit(concurrency)
+  return Promise.all(array.map(x => limit(() => fn(x))))
+}

libraries/promise-utils/package.json

@@ -0,0 +1,25 @@
+{
+  "name": "@overleaf/promise-utils",
+  "version": "0.1.0",
+  "description": "utilities to help working with promises",
+  "main": "index.js",
+  "scripts": {
+    "test": "npm run lint && npm run format && npm run test:unit",
+    "test:unit": "mocha",
+    "lint": "eslint --max-warnings 0 --format unix .",
+    "lint:fix": "eslint --fix .",
+    "format": "prettier --list-different $PWD/'**/*.js'",
+    "format:fix": "prettier --write $PWD/'**/*.js'",
+    "test:ci": "npm run test:unit"
+  },
+  "author": "Overleaf (https://www.overleaf.com)",
+  "license": "AGPL-3.0-only",
+  "devDependencies": {
+    "chai": "^4.3.10",
+    "chai-as-promised": "^7.1.1",
+    "mocha": "^10.2.0"
+  },
+  "dependencies": {
+    "p-limit": "^2.3.0"
+  }
+}

libraries/promise-utils/test/setup.js

@@ -0,0 +1,4 @@
+const chai = require('chai')
+const chaiAsPromised = require('chai-as-promised')
+
+chai.use(chaiAsPromised)

libraries/promise-utils/test/unit/PromiseUtilsTests.js

@@ -0,0 +1,298 @@
+const { expect } = require('chai')
+const {
+  promisifyAll,
+  promisifyClass,
+  callbackifyMultiResult,
+  callbackifyAll,
+} = require('../..')
+
+describe('promisifyAll', function () {
+  describe('basic functionality', function () {
+    before(function () {
+      this.module = {
+        SOME_CONSTANT: 1,
+        asyncAdd(a, b, callback) {
+          callback(null, a + b)
+        },
+        asyncDouble(x, callback) {
+          this.asyncAdd(x, x, callback)
+        },
+      }
+      this.promisified = promisifyAll(this.module)
+    })
+
+    it('promisifies functions in the module', async function () {
+      const sum = await this.promisified.asyncAdd(29, 33)
+      expect(sum).to.equal(62)
+    })
+
+    it('binds this to the original module', async function () {
+      const sum = await this.promisified.asyncDouble(38)
+      expect(sum).to.equal(76)
+    })
+
+    it('does not copy over non-functions', async function () {
+      expect(this.promisified).not.to.have.property('SOME_CONSTANT')
+    })
+
+    it('does not modify the prototype of the module', async function () {
+      expect(this.promisified.toString()).to.equal('[object Object]')
+    })
+  })
+
+  describe('without option', function () {
+    before(function () {
+      this.module = {
+        asyncAdd(a, b, callback) {
+          callback(null, a + b)
+        },
+        syncAdd(a, b) {
+          return a + b
+        },
+      }
+      this.promisified = promisifyAll(this.module, { without: ['syncAdd'] })
+    })
+
+    it('does not promisify excluded functions', function () {
+      expect(this.promisified.syncAdd).not.to.exist
+    })
+
+    it('promisifies other functions', async function () {
+      const sum = await this.promisified.asyncAdd(12, 89)
+      expect(sum).to.equal(101)
+    })
+  })
+
+  describe('multiResult option', function () {
+    before(function () {
+      this.module = {
+        asyncAdd(a, b, callback) {
+          callback(null, a + b)
+        },
+        asyncArithmetic(a, b, callback) {
+          callback(null, a + b, a * b)
+        },
+      }
+      this.promisified = promisifyAll(this.module, {
+        multiResult: { asyncArithmetic: ['sum', 'product'] },
+      })
+    })
+
+    it('promisifies multi-result functions', async function () {
+      const result = await this.promisified.asyncArithmetic(3, 6)
+      expect(result).to.deep.equal({ sum: 9, product: 18 })
+    })
+
+    it('promisifies other functions normally', async function () {
+      const sum = await this.promisified.asyncAdd(6, 1)
+      expect(sum).to.equal(7)
+    })
+  })
+})
+
+describe('promisifyClass', function () {
+  describe('basic functionality', function () {
+    before(function () {
+      this.Class = class {
+        constructor(a) {
+          this.a = a
+        }
+
+        asyncAdd(b, callback) {
+          callback(null, this.a + b)
+        }
+      }
+      this.Promisified = promisifyClass(this.Class)
+    })
+
+    it('promisifies the class methods', async function () {
+      const adder = new this.Promisified(1)
+      const sum = await adder.asyncAdd(2)
+      expect(sum).to.equal(3)
+    })
+  })
+
+  describe('without option', function () {
+    before(function () {
+      this.Class = class {
+        constructor(a) {
+          this.a = a
+        }
+
+        asyncAdd(b, callback) {
+          callback(null, this.a + b)
+        }
+
+        syncAdd(b) {
+          return this.a + b
+        }
+      }
+      this.Promisified = promisifyClass(this.Class, { without: ['syncAdd'] })
+    })
+
+    it('does not promisify excluded functions', function () {
+      const adder = new this.Promisified(10)
+      const sum = adder.syncAdd(12)
+      expect(sum).to.equal(22)
+    })
+
+    it('promisifies other functions', async function () {
+      const adder = new this.Promisified(23)
+      const sum = await adder.asyncAdd(3)
+      expect(sum).to.equal(26)
+    })
+  })
+
+  describe('multiResult option', function () {
+    before(function () {
+      this.Class = class {
+        constructor(a) {
+          this.a = a
+        }
+
+        asyncAdd(b, callback) {
+          callback(null, this.a + b)
+        }
+
+        asyncArithmetic(b, callback) {
+          callback(null, this.a + b, this.a * b)
+        }
+      }
+      this.Promisified = promisifyClass(this.Class, {
+        multiResult: { asyncArithmetic: ['sum', 'product'] },
+      })
+    })
+
+    it('promisifies multi-result functions', async function () {
+      const adder = new this.Promisified(3)
+      const result = await adder.asyncArithmetic(6)
+      expect(result).to.deep.equal({ sum: 9, product: 18 })
+    })
+
+    it('promisifies other functions normally', async function () {
+      const adder = new this.Promisified(6)
+      const sum = await adder.asyncAdd(1)
+      expect(sum).to.equal(7)
+    })
+  })
+})
+
+describe('callbackifyMultiResult', function () {
+  it('callbackifies a multi-result function', function (done) {
+    async function asyncArithmetic(a, b) {
+      return { sum: a + b, product: a * b }
+    }
+    const callbackified = callbackifyMultiResult(asyncArithmetic, [
+      'sum',
+      'product',
+    ])
+    callbackified(3, 11, (err, sum, product) => {
+      if (err != null) {
+        return done(err)
+      }
+      expect(sum).to.equal(14)
+      expect(product).to.equal(33)
+      done()
+    })
+  })
+
+  it('propagates errors', function (done) {
+    async function asyncBomb() {
+      throw new Error('BOOM!')
+    }
+    const callbackified = callbackifyMultiResult(asyncBomb, [
+      'explosives',
+      'dynamite',
+    ])
+    callbackified(err => {
+      expect(err).to.exist
+      done()
+    })
+  })
+})
+
+describe('callbackifyAll', function () {
+  describe('basic functionality', function () {
+    before(function () {
+      this.module = {
+        SOME_CONSTANT: 1,
+        async asyncAdd(a, b) {
+          return a + b
+        },
+        async asyncDouble(x, callback) {
+          return await this.asyncAdd(x, x)
+        },
+        dashConcat(a, b) {
+          return `${a}-${b}`
+        },
+      }
+      this.callbackified = callbackifyAll(this.module)
+    })
+
+    it('callbackifies async functions in the module', function (done) {
+      this.callbackified.asyncAdd(77, 18, (err, sum) => {
+        if (err) {
+          return done(err)
+        }
+        expect(sum).to.equal(95)
+        done()
+      })
+    })
+
+    it('binds this to the original module', function (done) {
+      this.callbackified.asyncDouble(20, (err, double) => {
+        if (err) {
+          return done(err)
+        }
+        expect(double).to.equal(40)
+        done()
+      })
+    })
+
+    it('copies over regular functions', function () {
+      const s = this.callbackified.dashConcat('ping', 'pong')
+      expect(s).to.equal('ping-pong')
+    })
+
+    it('copies over non-functions', function () {
+      expect(this.callbackified.SOME_CONSTANT).to.equal(1)
+    })
+  })
+
+  describe('multiResult option', function () {
+    before(function () {
+      this.module = {
+        async asyncAdd(a, b) {
+          return a + b
+        },
+        async asyncArithmetic(a, b) {
+          return { sum: a + b, product: a * b }
+        },
+      }
+      this.callbackified = callbackifyAll(this.module, {
+        multiResult: { asyncArithmetic: ['sum', 'product'] },
+      })
+    })
+
+    it('callbackifies multi-result functions', function (done) {
+      this.callbackified.asyncArithmetic(4, 5, (err, sum, product) => {
+        if (err) {
+          return done(err)
+        }
+        expect(sum).to.equal(9)
+        expect(product).to.equal(20)
+        done()
+      })
+    })
+
+    it('callbackifies other functions normally', function (done) {
+      this.callbackified.asyncAdd(77, 18, (err, sum) => {
+        if (err) {
+          return done(err)
+        }
+        expect(sum).to.equal(95)
+        done()
+      })
+    })
+  })
+})

services/web/frontend/js/features/source-editor/components/table-generator/cell.tsx

@@ -202,8 +202,10 @@ export const Cell: FC<{
   )
 
   const onDoubleClick = useCallback(() => {
-    startEditing(rowIndex, columnIndex, cellData.content)
+    if (!view.state.readOnly) {
-  }, [columnIndex, rowIndex, startEditing, cellData.content])
+      startEditing(rowIndex, columnIndex, cellData.content)
+    }
+  }, [columnIndex, rowIndex, startEditing, cellData.content, view])
 
   return (
     // eslint-disable-next-line jsx-a11y/click-events-have-key-events, jsx-a11y/no-noninteractive-element-interactions

services/web/frontend/js/features/source-editor/components/table-generator/table.tsx

@@ -148,6 +148,9 @@ export const Table: FC = () => {
       if (startCompileKeypress(event)) {
         return
       }
+      if (view.state.readOnly) {
+        return
+      }
       const commandKey = isMac ? event.metaKey : event.ctrlKey
       if (event.code === 'Enter' && !event.shiftKey) {
         event.preventDefault()
@@ -270,6 +273,9 @@ export const Table: FC = () => {
 
   useEffect(() => {
     const onPaste = (event: ClipboardEvent) => {
+      if (view.state.readOnly) {
+        return false
+      }
       if (cellData || !selection) {
         // We're editing a cell, so allow browser to insert there
         return false

services/web/frontend/js/features/source-editor/components/table-generator/tabular.tsx

@@ -291,7 +291,7 @@ const TabularWrapper: FC = () => {
   }, [cellData, commitCellData, selection, setSelection, ref, view])
   return (
     <div className="table-generator" ref={ref}>
-      <Toolbar />
+      {!view.state.readOnly && <Toolbar />}
       <Table />
     </div>
   )

services/web/frontend/js/features/source-editor/extensions/visual/atomic-decorations.ts

@@ -64,6 +64,8 @@ import { TabularWidget } from './visual-widgets/tabular'
 import { nextSnippetField, pickedCompletion } from '@codemirror/autocomplete'
 import { skipPreambleWithCursor } from './skip-preamble-cursor'
 import { TableRenderingErrorWidget } from './visual-widgets/table-rendering-error'
+import { GraphicsWidget } from './visual-widgets/graphics'
+import { InlineGraphicsWidget } from './visual-widgets/inline-graphics'
 
 type Options = {
   fileTreeManager: {
@@ -886,9 +888,12 @@ export const atomicDecorations = (options: Options) => {
                   line.text.trim().length === nodeRef.to - nodeRef.from
 
                 if (lineContainsOnlyNode) {
+                  const Widget = state.readOnly
+                    ? GraphicsWidget
+                    : EditableGraphicsWidget
                   decorations.push(
                     Decoration.replace({
-                      widget: new EditableGraphicsWidget(
+                      widget: new Widget(
                         filePath,
                         getPreviewByPath,
                         centered,
@@ -898,9 +903,12 @@ export const atomicDecorations = (options: Options) => {
                     }).range(line.from, line.to)
                   )
                 } else {
+                  const Widget = state.readOnly
+                    ? InlineGraphicsWidget
+                    : EditableInlineGraphicsWidget
                   decorations.push(
                     Decoration.replace({
-                      widget: new EditableInlineGraphicsWidget(
+                      widget: new Widget(
                         filePath,
                         getPreviewByPath,
                         centered,

services/web/test/frontend/features/source-editor/components/codemirror-editor-visual-readonly.spec.tsx

@@ -7,6 +7,23 @@ const Container: FC = ({ children }) => (
   <div style={{ width: 785, height: 785 }}>{children}</div>
 )
 
+const mountEditor = (content: string, ...args: any[]) => {
+  const scope = mockScope(content)
+  scope.permissionsLevel = 'readOnly'
+  scope.editor.showVisual = true
+
+  cy.mount(
+    <Container>
+      <EditorProviders scope={scope} {...args}>
+        <CodemirrorEditor />
+      </EditorProviders>
+    </Container>
+  )
+
+  // wait for the content to be parsed and revealed
+  cy.get('.cm-content').should('have.css', 'opacity', '1')
+}
+
 describe('<CodeMirrorEditor/> in Visual mode with read-only permission', function () {
   beforeEach(function () {
     window.metaAttributesCache.set('ol-preventCompileOnLoad', true)
@@ -19,20 +36,7 @@ describe('<CodeMirrorEditor/> in Visual mode with read-only permission', functio
   })
 
   it('decorates footnote content', function () {
-    const scope = mockScope('Foo \\footnote{Bar.} ')
+    mountEditor('Foo \\footnote{Bar.} ')
-    scope.permissionsLevel = 'readOnly'
-    scope.editor.showVisual = true
-
-    cy.mount(
-      <Container>
-        <EditorProviders scope={scope}>
-          <CodemirrorEditor />
-        </EditorProviders>
-      </Container>
-    )
-
-    // wait for the content to be parsed and revealed
-    cy.get('.cm-content').should('have.css', 'opacity', '1')
 
     // select the footnote, so it expands
     cy.get('.ol-cm-footnote').click()
@@ -41,4 +45,68 @@ describe('<CodeMirrorEditor/> in Visual mode with read-only permission', functio
     cy.get('@first-line').should('contain', 'Foo')
     cy.get('@first-line').should('contain', 'Bar')
   })
+
+  it('does not display the table toolbar', function () {
+    mountEditor('\\begin{tabular}{c}\n    cell\n\\end{tabular}')
+
+    cy.get('.table-generator-floating-toolbar').should('not.exist')
+    cy.get('.table-generator-cell').click()
+    cy.get('.table-generator-floating-toolbar').should('not.exist')
+  })
+
+  it('does not enter a table cell on double-click', function () {
+    mountEditor('\\begin{tabular}{c}\n    cell\n\\end{tabular}')
+
+    cy.get('.table-generator-cell').dblclick()
+    cy.get('.table-generator-cell').get('textarea').should('not.exist')
+  })
+
+  it('does not enter a table cell on Enter', function () {
+    mountEditor('\\begin{tabular}{c}\n    cell\n\\end{tabular}')
+
+    cy.get('.table-generator-cell').trigger('keydown', { key: 'Enter' })
+    cy.get('.table-generator-cell').get('textarea').should('not.exist')
+  })
+
+  it('does not paste into a table cell', function () {
+    mountEditor('\\begin{tabular}{c}\n    cell\n\\end{tabular}\n\n')
+
+    cy.get('.cm-line').last().click()
+    cy.get('.table-generator-cell-render').eq(0).click()
+
+    const clipboardData = new DataTransfer()
+    clipboardData.setData('text/plain', 'bar')
+    cy.get('.table-generator-cell-render')
+      .eq(0)
+      .trigger('paste', { clipboardData })
+
+    cy.get('.cm-content').should('have.text', 'cell')
+  })
+
+  it('does not display the figure edit button', function () {
+    const fileTreeManager = {
+      findEntityById: cy.stub(),
+      findEntityByPath: cy.stub(),
+      getEntityPath: cy.stub(),
+      getRootDocDirname: cy.stub(),
+      getPreviewByPath: cy
+        .stub()
+        .returns({ url: '/images/frog.jpg', extension: 'jpg' }),
+    }
+
+    cy.intercept('/images/frog.jpg', { fixture: 'images/gradient.png' })
+
+    mountEditor(
+      `\\begin{figure}
+\\centering
+\\includegraphics[width=0.5\\linewidth]{frog.jpg}
+\\caption{My caption}
+\\label{fig:my-label}
+\\end{figure}`,
+      { fileTreeManager }
+    )
+
+    cy.get('img.ol-cm-graphics').should('have.length', 1)
+    cy.findByRole('button', { name: 'Edit figure' }).should('not.exist')
+  })
 })