From 1ed84cb520c253ec0a96335f4b591f2a12e4d930 Mon Sep 17 00:00:00 2001 From: Tim Down <158919+timdown@users.noreply.github.com> Date: Thu, 28 Sep 2023 14:42:31 +0100 Subject: [PATCH] Reindex references on deleting or refreshing a .bib file (#14938) * Reindex references on deleting or refreshing a .bib file * Remove rendundant props * Tweak file refresh payload, send refresh response after update to keys, remove some unnecessary returns * Tidy up GitOrigin-RevId: bc0309a54fbfd0eb7d8285032300453d360d6b2f --- .../LinkedFiles/LinkedFilesController.js | 25 ++++++++++- .../References/ReferencesController.js | 7 +++- .../file-tree/components/file-tree-context.js | 2 +- .../contexts/file-tree-actionable.js | 11 ++++- .../file-view/components/file-view-header.tsx | 42 ++++++------------- .../file-view/components/file-view.js | 5 +-- .../js/ide/references/ReferencesManager.js | 12 +++--- .../file-tree/flows/delete-entity.test.js | 10 +++-- .../components/file-view-header.test.js | 39 ++++------------- .../file-view/components/file-view.test.js | 16 ++----- 10 files changed, 77 insertions(+), 92 deletions(-) diff --git a/services/web/app/src/Features/LinkedFiles/LinkedFilesController.js b/services/web/app/src/Features/LinkedFiles/LinkedFilesController.js index 3179be0109..7c1d299eee 100644 --- a/services/web/app/src/Features/LinkedFiles/LinkedFilesController.js +++ b/services/web/app/src/Features/LinkedFiles/LinkedFilesController.js @@ -37,9 +37,12 @@ const { const { OutputFileFetchFailedError, FileTooLargeError, + OError, } = require('../Errors/Errors') const Modules = require('../../infrastructure/Modules') const { plainTextResponse } = require('../../infrastructure/Response') +const ReferencesHandler = require('../References/ReferencesHandler') +const EditorRealTimeController = require('../Editor/EditorRealTimeController') module.exports = LinkedFilesController = { Agents: _.extend( @@ -122,7 +125,7 @@ module.exports = LinkedFilesController = { return res.sendStatus(400) } - return Agent.refreshLinkedFile( + Agent.refreshLinkedFile( projectId, linkedFileData, name, @@ -132,7 +135,25 @@ module.exports = LinkedFilesController = { if (err != null) { return LinkedFilesController.handleError(err, req, res, next) } - return res.json({ new_file_id: newFileId }) + if (req.body.shouldReindexReferences) { + ReferencesHandler.indexAll(projectId, function (error, data) { + if (error) { + OError.tag(error, 'failed to index references', { + projectId, + }) + return next(error) + } + EditorRealTimeController.emitToRoom( + projectId, + 'references:keys:updated', + data.keys, + true + ) + res.json({ new_file_id: newFileId }) + }) + } else { + res.json({ new_file_id: newFileId }) + } } ) } diff --git a/services/web/app/src/Features/References/ReferencesController.js b/services/web/app/src/Features/References/ReferencesController.js index e5fb2f2cf2..04a3a243a1 100644 --- a/services/web/app/src/Features/References/ReferencesController.js +++ b/services/web/app/src/Features/References/ReferencesController.js @@ -39,6 +39,7 @@ module.exports = ReferencesController = { res, projectId, shouldBroadcast, + false, data ) }) @@ -57,12 +58,13 @@ module.exports = ReferencesController = { res, projectId, shouldBroadcast, + true, data ) }) }, - _handleIndexResponse(req, res, projectId, shouldBroadcast, data) { + _handleIndexResponse(req, res, projectId, shouldBroadcast, isAllDocs, data) { if (data == null || data.keys == null) { return res.json({ projectId, keys: [] }) } @@ -70,7 +72,8 @@ module.exports = ReferencesController = { EditorRealTimeController.emitToRoom( projectId, 'references:keys:updated', - data.keys + data.keys, + isAllDocs ) } return res.json(data) diff --git a/services/web/frontend/js/features/file-tree/components/file-tree-context.js b/services/web/frontend/js/features/file-tree/components/file-tree-context.js index db1a748dc7..0d2d69f8ac 100644 --- a/services/web/frontend/js/features/file-tree/components/file-tree-context.js +++ b/services/web/frontend/js/features/file-tree/components/file-tree-context.js @@ -26,7 +26,7 @@ function FileTreeContext({ reindexReferences={reindexReferences} > - + {children} diff --git a/services/web/frontend/js/features/file-tree/contexts/file-tree-actionable.js b/services/web/frontend/js/features/file-tree/contexts/file-tree-actionable.js index f0a66b1488..06b03a12d2 100644 --- a/services/web/frontend/js/features/file-tree/contexts/file-tree-actionable.js +++ b/services/web/frontend/js/features/file-tree/contexts/file-tree-actionable.js @@ -119,7 +119,7 @@ function fileTreeActionableReducer(state, action) { } } -export function FileTreeActionableProvider({ children }) { +export function FileTreeActionableProvider({ reindexReferences, children }) { const { _id: projectId } = useProjectContext(projectContextPropTypes) const { permissionsLevel } = useEditorContext(editorContextPropTypes) @@ -187,9 +187,12 @@ export function FileTreeActionableProvider({ children }) { // deletes entities in series. Tree will be updated via the socket event const finishDeleting = useCallback(() => { dispatch({ type: ACTION_TYPES.DELETING }) + let shouldReindexReferences = false return mapSeries(Array.from(selectedEntityIds), id => { const found = findInTreeOrThrow(fileTreeData, id) + shouldReindexReferences = + shouldReindexReferences || /\.bib$/.test(found.entity.name) return syncDelete(projectId, found.type, found.entity._id).catch( error => { // throw unless 404 @@ -200,13 +203,16 @@ export function FileTreeActionableProvider({ children }) { ) }) .then(() => { + if (shouldReindexReferences) { + reindexReferences() + } dispatch({ type: ACTION_TYPES.CLEAR }) }) .catch(error => { // set an error and allow user to retry dispatch({ type: ACTION_TYPES.ERROR, error }) }) - }, [fileTreeData, projectId, selectedEntityIds]) + }, [fileTreeData, projectId, selectedEntityIds, reindexReferences]) // moves entities. Tree is updated immediately and data are sync'd after. const finishMoving = useCallback( @@ -404,6 +410,7 @@ export function FileTreeActionableProvider({ children }) { } FileTreeActionableProvider.propTypes = { + reindexReferences: PropTypes.func.isRequired, children: PropTypes.oneOfType([ PropTypes.arrayOf(PropTypes.node), PropTypes.node, diff --git a/services/web/frontend/js/features/file-view/components/file-view-header.tsx b/services/web/frontend/js/features/file-view/components/file-view-header.tsx index 55667909d1..59cf2d0904 100644 --- a/services/web/frontend/js/features/file-view/components/file-view-header.tsx +++ b/services/web/frontend/js/features/file-view/components/file-view-header.tsx @@ -12,7 +12,6 @@ import importOverleafModules from '../../../../macros/import-overleaf-module.mac import useAbortController from '../../../shared/hooks/use-abort-controller' import { LinkedFileIcon } from './file-view-icons' import { BinaryFile, hasProvider, LinkedFile } from '../types/binary-file' -import { debugConsole } from '@/utils/debugging' const tprLinkedFileInfo = importOverleafModules('tprLinkedFileInfo') as { import: { LinkedFileInfo: ElementType } @@ -45,13 +44,9 @@ function shortenedUrl(url: string) { type FileViewHeaderProps = { file: BinaryFile - storeReferencesKeys: (keys: string[]) => void } -export default function FileViewHeader({ - file, - storeReferencesKeys, -}: FileViewHeaderProps) { +export default function FileViewHeader({ file }: FileViewHeaderProps) { const { _id: projectId } = useProjectContext({ _id: PropTypes.string.isRequired, }) @@ -92,7 +87,16 @@ export default function FileViewHeader({ setRefreshing(true) // Replacement of the file handled by the file tree window.expectingLinkedFileRefreshedSocketFor = file.name - postJSON(`/project/${projectId}/linked_file/${file.id}/refresh`, { signal }) + const body = { + shouldReindexReferences: + file.linkedFileData?.provider === 'mendeley' || + file.linkedFileData?.provider === 'zotero' || + /\.bib$/.test(file.name), + } + postJSON(`/project/${projectId}/linked_file/${file.id}/refresh`, { + signal, + body, + }) .then(() => { setRefreshing(false) }) @@ -100,29 +104,7 @@ export default function FileViewHeader({ setRefreshing(false) setRefreshError(err.data?.message || err.message) }) - .finally(() => { - if ( - hasProvider(file, 'mendeley') || - hasProvider(file, 'zotero') || - file.name.match(/^.*\.bib$/) - ) { - reindexReferences() - } - }) - - function reindexReferences() { - const opts = { - body: { shouldBroadcast: true }, - } - - postJSON(`/project/${projectId}/references/indexAll`, opts) - .then(response => { - // Later updated by the socket but also updated here for immediate use - storeReferencesKeys(response.keys) - }) - .catch(debugConsole.error) - } - }, [file, projectId, signal, storeReferencesKeys]) + }, [file, projectId, signal]) return (
diff --git a/services/web/frontend/js/features/file-view/components/file-view.js b/services/web/frontend/js/features/file-view/components/file-view.js index d5e9ff1e79..b74ffc5144 100644 --- a/services/web/frontend/js/features/file-view/components/file-view.js +++ b/services/web/frontend/js/features/file-view/components/file-view.js @@ -9,7 +9,7 @@ import Icon from '../../../shared/components/icon' const imageExtensions = ['png', 'jpg', 'jpeg', 'gif'] -export default function FileView({ file, storeReferencesKeys }) { +export default function FileView({ file }) { const [contentLoading, setContentLoading] = useState(true) const [hasError, setHasError] = useState(false) @@ -34,7 +34,7 @@ export default function FileView({ file, storeReferencesKeys }) { const content = ( <> - + {imageExtensions.includes(extension) && ( - this._storeReferencesKeys(keys) + this.ide.socket.on('references:keys:updated', (keys, allDocs) => + this._storeReferencesKeys(keys, allDocs) ) this.indexAllReferences(false) } }) } - _storeReferencesKeys(newKeys) { + _storeReferencesKeys(newKeys, replaceExistingKeys) { const oldKeys = this.$scope.$root._references.keys - const keys = _.union(oldKeys, newKeys) + const keys = replaceExistingKeys ? newKeys : _.union(oldKeys, newKeys) window.dispatchEvent( new CustomEvent('project:references', { detail: keys, @@ -99,7 +99,7 @@ export default ReferencesManager = class ReferencesManager { return this.ide.$http .post(`/project/${this.$scope.project_id}/references/index`, opts) .then(response => { - return this._storeReferencesKeys(response.data.keys) + return this._storeReferencesKeys(response.data.keys, false) }) } @@ -111,7 +111,7 @@ export default ReferencesManager = class ReferencesManager { return this.ide.$http .post(`/project/${this.$scope.project_id}/references/indexAll`, opts) .then(response => { - return this._storeReferencesKeys(response.data.keys) + return this._storeReferencesKeys(response.data.keys, true) }) } } diff --git a/services/web/test/frontend/features/file-tree/flows/delete-entity.test.js b/services/web/test/frontend/features/file-tree/flows/delete-entity.test.js index 75b13ff82a..2d378385ef 100644 --- a/services/web/test/frontend/features/file-tree/flows/delete-entity.test.js +++ b/services/web/test/frontend/features/file-tree/flows/delete-entity.test.js @@ -13,6 +13,7 @@ import FileTreeRoot from '../../../../../frontend/js/features/file-tree/componen describe('FileTree Delete Entity Flow', function () { const onSelect = sinon.stub() const onInit = sinon.stub() + const reindexReferences = sinon.stub() beforeEach(function () { window.metaAttributesCache = new Map() @@ -23,6 +24,7 @@ describe('FileTree Delete Entity Flow', function () { fetchMock.restore() onSelect.reset() onInit.reset() + reindexReferences.reset() cleanUpContext() window.metaAttributesCache = new Map() }) @@ -41,7 +43,7 @@ describe('FileTree Delete Entity Flow', function () { renderWithEditorContext( null} + reindexReferences={reindexReferences} setRefProviderEnabled={() => null} setStartedFreeTrial={() => null} onSelect={onSelect} @@ -94,6 +96,7 @@ describe('FileTree Delete Entity Flow', function () { const [lastFetchPath] = fetchMock.lastCall(fetchMatcher) expect(lastFetchPath).to.equal('/project/123abc/doc/456def') + expect(reindexReferences).not.to.have.been.called }) it('continues delete on 404s', async function () { @@ -220,7 +223,7 @@ describe('FileTree Delete Entity Flow', function () { renderWithEditorContext( null} + reindexReferences={reindexReferences} setRefProviderEnabled={() => null} setStartedFreeTrial={() => null} onSelect={onSelect} @@ -254,7 +257,7 @@ describe('FileTree Delete Entity Flow', function () { fireEvent.click(deleteButton) }) - it('removes all items', async function () { + it('removes all items and reindexes references after deleting .bib file', async function () { const fetchMatcher = /\/project\/\w+\/(doc|file)\/\w+/ fetchMock.delete(fetchMatcher, 204) @@ -289,6 +292,7 @@ describe('FileTree Delete Entity Flow', function () { .map(([url]) => url) expect(firstFetchPath).to.equal('/project/123abc/doc/456def') expect(secondFetchPath).to.equal('/project/123abc/file/789ghi') + expect(reindexReferences).to.have.been.called }) }) diff --git a/services/web/test/frontend/features/file-view/components/file-view-header.test.js b/services/web/test/frontend/features/file-view/components/file-view-header.test.js index 8fbc005028..5751a4edf8 100644 --- a/services/web/test/frontend/features/file-view/components/file-view-header.test.js +++ b/services/web/test/frontend/features/file-view/components/file-view-header.test.js @@ -5,7 +5,6 @@ import { } from '@testing-library/react' import { expect } from 'chai' import fetchMock from 'fetch-mock' -import sinon from 'sinon' import { renderWithEditorContext } from '../../../helpers/render-with-context' import FileViewHeader from '../../../../../frontend/js/features/file-view/components/file-view-header' @@ -55,9 +54,7 @@ describe('', function () { describe('header text', function () { it('Renders the correct text for a file with the url provider', function () { - renderWithEditorContext( - {}} /> - ) + renderWithEditorContext() screen.getByText('Imported from', { exact: false }) screen.getByText('at 3:24 am Wed, 17th Feb 21', { exact: false, @@ -65,9 +62,7 @@ describe('', function () { }) it('Renders the correct text for a file with the project_file provider', function () { - renderWithEditorContext( - {}} /> - ) + renderWithEditorContext() screen.getByText('Imported from', { exact: false }) screen.getByText('Another project', { exact: false }) screen.getByText('/source-entity-path.ext, at 3:24 am Wed, 17th Feb 21', { @@ -99,9 +94,7 @@ describe('', function () { } ) - renderWithEditorContext( - {}} /> - ) + renderWithEditorContext() fireEvent.click(screen.getByRole('button', { name: 'Refresh' })) @@ -119,23 +112,7 @@ describe('', function () { } ) - const reindexResponse = { - projectId: '123abc', - keys: ['reference1', 'reference2', 'reference3', 'reference4'], - } - fetchMock.post( - 'express:/project/:project_id/references/indexAll', - reindexResponse - ) - - const storeReferencesKeys = sinon.stub() - - renderWithEditorContext( - - ) + renderWithEditorContext() fireEvent.click(screen.getByRole('button', { name: 'Refresh' })) @@ -144,15 +121,15 @@ describe('', function () { ) expect(fetchMock.done()).to.be.true - expect(storeReferencesKeys).to.be.calledWith(reindexResponse.keys) + + const lastCallBody = JSON.parse(fetchMock.lastCall()[1].body) + expect(lastCallBody.shouldReindexReferences).to.be.true }) }) describe('The download button', function () { it('exists', function () { - renderWithEditorContext( - {}} /> - ) + renderWithEditorContext() screen.getByText('Download', { exact: false }) }) diff --git a/services/web/test/frontend/features/file-view/components/file-view.test.js b/services/web/test/frontend/features/file-view/components/file-view.test.js index 56770fab32..86fce60c15 100644 --- a/services/web/test/frontend/features/file-view/components/file-view.test.js +++ b/services/web/test/frontend/features/file-view/components/file-view.test.js @@ -45,9 +45,7 @@ describe('', function () { 'Text file content' ) - renderWithEditorContext( - {}} /> - ) + renderWithEditorContext() await waitForElementToBeRemoved(() => screen.getByText('Loading', { exact: false }) @@ -60,9 +58,7 @@ describe('', function () { name: 'example.not-tex', } - renderWithEditorContext( - {}} /> - ) + renderWithEditorContext() await screen.findByText('Sorry, no preview is available', { exact: false, @@ -72,17 +68,13 @@ describe('', function () { describe('for an image file', function () { it('shows a loading indicator while the file is loading', async function () { - renderWithEditorContext( - {}} /> - ) + renderWithEditorContext() screen.getByText('Loading', { exact: false }) }) it('shows messaging if the image could not be loaded', async function () { - renderWithEditorContext( - {}} /> - ) + renderWithEditorContext() // Fake the image request failing as the request is handled by the browser fireEvent.error(screen.getByRole('img'))