From 81ec67c2e6d45d02a9734de8077d4e01bfe6bf0b Mon Sep 17 00:00:00 2001 From: Jonas Date: Sat, 24 Feb 2024 13:25:10 +0100 Subject: [PATCH] fix(links): Insert `/f/` format for links to files Get rid of the old `/path/?fileId=` format. It never really worked anyway because the paths were relative to the users' home directory. Instead, insert links to files/folders as full URLs with the `/f/` format. This makes link previews work both in Text and Collectives. Rewrite links in the old format to the new format. But don't touch absolute link (without origin) to the collectives app to not break collectives-specific link handling to other pages. Signed-off-by: Jonas --- cypress/e2e/nodes/Links.spec.js | 15 +++++-- cypress/e2e/workspace.spec.js | 7 +++- src/components/Menu/ActionInsertLink.vue | 6 +-- src/helpers/files.js | 20 ---------- src/helpers/links.js | 46 ++++----------------- src/tests/helpers/links.spec.js | 51 +++++++++++++----------- 6 files changed, 53 insertions(+), 92 deletions(-) diff --git a/cypress/e2e/nodes/Links.spec.js b/cypress/e2e/nodes/Links.spec.js index 248f868d791..4af18b7bb52 100644 --- a/cypress/e2e/nodes/Links.spec.js +++ b/cypress/e2e/nodes/Links.spec.js @@ -2,6 +2,7 @@ import { initUserAndFiles, randUser } from '../../utils/index.js' const user = randUser() const fileName = 'empty.md' +let fileId = null describe('test link marks', function() { before(function() { @@ -18,6 +19,9 @@ describe('test link marks', function() { }, }) cy.openFile(fileName, { force: true }) + cy.getFileId(fileName).then((id) => { + fileId = id + }) }) describe('link bubble', function() { @@ -181,7 +185,7 @@ describe('test link marks', function() { }) return cy.getContent() - .find(`a[href*="${encodeURIComponent(filename)}"]`) + .find(`a[href*="/f/${fileId}"]`) .should('have.text', text === undefined ? filename : text) } @@ -204,9 +208,12 @@ describe('test link marks', function() { }) it('link to directory', () => { cy.createFolder(`${window.__currentDirectory}/dummy folder`) - cy.getFile(fileName).then($el => { - cy.getContent().type(`${text}{selectAll}`) - checkLinkFile('dummy folder', text, true) + cy.getFileId('dummy folder').then((id) => { + fileId = id + cy.getFile(fileName).then($el => { + cy.getContent().type(`${text}{selectAll}`) + checkLinkFile('dummy folder', text, true) + }) }) }) }) diff --git a/cypress/e2e/workspace.spec.js b/cypress/e2e/workspace.spec.js index 453bd62de32..9dc4f48dcf4 100644 --- a/cypress/e2e/workspace.spec.js +++ b/cypress/e2e/workspace.spec.js @@ -158,11 +158,15 @@ describe('Workspace', function() { }) it('relative folder links', function() { + let fileId = null cy.createFolder(`${this.testFolder}/sub-folder`) cy.createFolder(`${this.testFolder}/sub-folder/alpha`) cy.uploadFile('test.md', 'text/markdown', `${this.testFolder}/sub-folder/alpha/test.md`) cy.visitTestFolder() + cy.getFileId('test.md').then((id) => { + fileId = id + }) cy.openWorkspace() .type('link me') @@ -180,8 +184,7 @@ describe('Workspace', function() { cy.getEditor() .find('a') .should('have.attr', 'href') - .and('contains', `dir=/${this.testFolder}/sub-folder/alpha`) - .and('contains', '#relPath=sub-folder/alpha/test.md') + .and('contains', `/f/${fileId})`) cy.getContent() .type('{leftArrow}') diff --git a/src/components/Menu/ActionInsertLink.vue b/src/components/Menu/ActionInsertLink.vue index 2cafe67f016..a26694ff5d4 100644 --- a/src/components/Menu/ActionInsertLink.vue +++ b/src/components/Menu/ActionInsertLink.vue @@ -80,12 +80,12 @@ import { NcActions, NcActionButton, NcActionInput } from '@nextcloud/vue' import { getLinkWithPicker } from '@nextcloud/vue/dist/Components/NcRichText.js' import { FilePickerType, getFilePickerBuilder } from '@nextcloud/dialogs' +import { generateUrl } from '@nextcloud/router' import { getMarkAttributes, isActive } from '@tiptap/core' import { Document, Loading, LinkOff, Web, Shape } from '../icons.js' import { BaseActionEntry } from './BaseActionEntry.js' -import { optimalPath } from '../../helpers/files.js' import { useFileMixin } from '../Editor.provider.js' import { useMenuIDMixin } from './MenuBar.provider.js' @@ -144,9 +144,7 @@ export default { .then((file) => { const client = OC.Files.getClient() client.getFileInfo(file).then((_status, fileInfo) => { - const path = optimalPath(this.relativePath, `${fileInfo.path}/${fileInfo.name}`) - const encodedPath = path.split('/').map(encodeURIComponent).join('/') + (fileInfo.type === 'dir' ? '/' : '') - const href = `${encodedPath}?fileId=${fileInfo.id}` + const href = generateUrl(`/f/${fileInfo.id}`) this.setLink(href, fileInfo.name) this.startPath = fileInfo.path + (fileInfo.type === 'dir' ? `/${fileInfo.name}/` : '') }) diff --git a/src/helpers/files.js b/src/helpers/files.js index a5ca85fddb3..1e6f4760434 100644 --- a/src/helpers/files.js +++ b/src/helpers/files.js @@ -37,25 +37,6 @@ import FilePlusSvg from '@mdi/svg/svg/file-plus.svg' const FILE_ACTION_IDENTIFIER = 'Edit with text app' -const optimalPath = function(from, to) { - const current = from.split('/') - const target = to.split('/') - current.pop() // ignore filename - while (current[0] === target[0]) { - current.shift() - target.shift() - // Handle case where target is the current directory - if (current.length === 0 && target.length === 0) { - return '.' - } - } - const relativePath = current.fill('..').concat(target) - const absolutePath = to.split('/') - return relativePath.length < absolutePath.length - ? relativePath.join('/') - : to -} - const registerFileCreate = () => { const newFileMenuPlugin = { attach(menu) { @@ -260,7 +241,6 @@ export const FilesWorkspaceHeader = new Header({ }) export { - optimalPath, registerFileActionFallback, registerFileCreate, FILE_ACTION_IDENTIFIER, diff --git a/src/helpers/links.js b/src/helpers/links.js index b45ab1ba7e2..cd9830aa96c 100644 --- a/src/helpers/links.js +++ b/src/helpers/links.js @@ -20,34 +20,8 @@ * */ -import { loadState } from '@nextcloud/initial-state' import { generateUrl } from '@nextcloud/router' -const absolutePath = function(base, rel) { - if (!rel) { - return base - } - if (rel[0] === '/') { - return rel - } - base = base.split('/') - rel = rel.split('/') - while (rel[0] === '..' || rel[0] === '.') { - if (rel[0] === '..') { - base.pop() - } - rel.shift() - } - return base.concat(rel).join('/') -} - -const basedir = function(file) { - const end = file.lastIndexOf('/') - return (end > 0) - ? file.slice(0, end) - : file.slice(0, end + 1) // basedir('/toplevel') should return '/' -} - const domHref = function(node, relativePath) { const ref = node.attrs.href if (!ref) { @@ -62,22 +36,16 @@ const domHref = function(node, relativePath) { if (ref.startsWith('#')) { return ref } - // Don't rewrite URL in Collectives context - if (loadState('core', 'active-app') === 'collectives') { + // Don't rewrite links to the collectives app + if (ref.includes('/apps/collectives/')) { return ref } + // Rewrite links with old format from file picker to `/f/` const match = ref.match(/^([^?]*)\?fileId=(\d+)/) if (match) { - const [, relPath, id] = match - const currentDir = basedir(relativePath || OCA.Viewer?.file || '/') - const dir = absolutePath(currentDir, basedir(relPath)) - if (relPath.length > 1 && relPath.endsWith('/')) { - // is directory - return generateUrl(`/apps/files/?dir=${dir}&fileId=${id}`) - } else { - return generateUrl(`/apps/files/?dir=${dir}&openfile=${id}#relPath=${relPath}`) - } + const [, , id] = match + return generateUrl(`/f/${id}`) } return ref } @@ -89,8 +57,8 @@ const parseHref = function(dom) { } const match = ref.match(/\?dir=([^&]*)&openfile=([^&]*)#relPath=([^&]*)/) if (match) { - const [, , id, path] = match - return `${path}?fileId=${id}` + const [, , id] = match + return generateUrl(`/f/${id}`) } return ref } diff --git a/src/tests/helpers/links.spec.js b/src/tests/helpers/links.spec.js index 12d4e614602..d1f980eee68 100644 --- a/src/tests/helpers/links.spec.js +++ b/src/tests/helpers/links.spec.js @@ -1,5 +1,4 @@ import { domHref, parseHref } from '../../helpers/links' -import { loadState } from '@nextcloud/initial-state' global.OCA = { Viewer: { @@ -13,9 +12,6 @@ global.OC = { global._oc_webroot = '' -jest.mock('@nextcloud/initial-state') -loadState.mockImplementation((app, key) => 'files') - describe('Preparing href attributes for the DOM', () => { test('leave empty hrefs alone', () => { @@ -36,24 +32,24 @@ describe('Preparing href attributes for the DOM', () => { .toBe('mailTo:test@mail.example') }) - test('relative link with fileid', () => { + test('relative link with fileid (old format from file picker)', () => { expect(domHref({attrs: {href: 'otherfile?fileId=123'}})) - .toBe('/apps/files/?dir=/Wiki&openfile=123#relPath=otherfile') + .toBe('/f/123') }) - test('relative path with ../', () => { + test('relative path with ../ (old format from file picker)', () => { expect(domHref({attrs: {href: '../other/otherfile?fileId=123'}})) - .toBe('/apps/files/?dir=/other&openfile=123#relPath=../other/otherfile') + .toBe('/f/123') }) - test('absolute path', () => { + test('absolute path (old format from file picker)', () => { expect(domHref({attrs: {href: '/other/otherfile?fileId=123'}})) - .toBe('/apps/files/?dir=/other&openfile=123#relPath=/other/otherfile') + .toBe('/f/123') }) - test('absolute path', () => { + test('absolute path (old format from file picker)', () => { expect(domHref({attrs: {href: '/otherfile?fileId=123'}})) - .toBe('/apps/files/?dir=/&openfile=123#relPath=/otherfile') + .toBe('/f/123') }) }) @@ -74,9 +70,9 @@ describe('Extracting short urls from the DOM', () => { expect(parseHref(domStub())).toBe(undefined) }) - test('relative link with fileid', () => { + test('relative link with fileid (old format from file picker)', () => { expect(parseHref(domStub('?dir=/other&openfile=123#relPath=../other/otherfile'))) - .toBe('../other/otherfile?fileId=123') + .toBe('/f/123') }) }) @@ -101,20 +97,29 @@ describe('Inserting hrefs into the dom and extracting them again', () => { expect(insertAndExtract({})).toBe(undefined) }) - test('default relative link format is unchanged', () => { + test('old relative link format (from file picker) is rewritten', () => { expect(insertAndExtract({href: 'otherfile?fileId=123'})) - .toBe('otherfile?fileId=123') + .toBe('/f/123') }) -}) + test('old relative link format with ../ (from file picker) is rewritten', () => { + expect(insertAndExtract({href: '../otherfile?fileId=123'})) + .toBe('/f/123') + }) -describe('Preparing href attributes for the DOM in Collectives app', () => { - beforeAll(() => { - loadState.mockImplementation((app, key) => 'collectives') + test('old absolute link format (from file picker) is rewritten', () => { + expect(insertAndExtract({href: '/otherfile?fileId=123'})) + .toBe('/f/123') }) - test('relative link with fileid in Collectives', () => { - expect(domHref({attrs: {href: 'otherfile?fileId=123'}})) - .toBe('otherfile?fileId=123') + test('default absolute link format is unchanged', () => { + expect(insertAndExtract({href: '/f/123'})) + .toBe('/f/123') }) + + test('absolute link to collectives page is unchanged', () => { + expect(insertAndExtract({href: '/apps/collectives/page?fileId=123'})) + .toBe('/apps/collectives/page?fileId=123') + }) + })