From 1f068eb0609b1fa2b6dd31f11383ea2fa18f65da 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 | 8 ++-- src/components/Menu/ActionInsertLink.vue | 7 ++-- src/helpers/links.js | 21 ++++------ src/tests/helpers/links.spec.js | 51 +++++++++++++----------- 4 files changed, 44 insertions(+), 43 deletions(-) diff --git a/cypress/e2e/nodes/Links.spec.js b/cypress/e2e/nodes/Links.spec.js index 248f868d791..7ea24f0f2ae 100644 --- a/cypress/e2e/nodes/Links.spec.js +++ b/cypress/e2e/nodes/Links.spec.js @@ -180,9 +180,11 @@ describe('test link marks', function() { cy.contains('button', isFolder ? 'Choose' : `Choose ${filename}`).click() }) - return cy.getContent() - .find(`a[href*="${encodeURIComponent(filename)}"]`) - .should('have.text', text === undefined ? filename : text) + cy.getFileId(filename).then((fileId) => { + return cy.getContent() + .find(`a[href*="/f/${fileId}"]`) + .should('have.text', text === undefined ? filename : text) + }) } beforeEach(() => cy.clearContent()) diff --git a/src/components/Menu/ActionInsertLink.vue b/src/components/Menu/ActionInsertLink.vue index 2cafe67f016..8d4fa6c011c 100644 --- a/src/components/Menu/ActionInsertLink.vue +++ b/src/components/Menu/ActionInsertLink.vue @@ -80,6 +80,7 @@ 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' @@ -144,10 +145,8 @@ 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}` - this.setLink(href, fileInfo.name) + const url = new URL(generateUrl(`/f/${fileInfo.id}`), window.location) + this.setLink(url.href, fileInfo.name) this.startPath = fileInfo.path + (fileInfo.type === 'dir' ? `/${fileInfo.name}/` : '') }) this.menuOpen = false diff --git a/src/helpers/links.js b/src/helpers/links.js index b45ab1ba7e2..210ef7aeb5a 100644 --- a/src/helpers/links.js +++ b/src/helpers/links.js @@ -20,7 +20,6 @@ * */ -import { loadState } from '@nextcloud/initial-state' import { generateUrl } from '@nextcloud/router' const absolutePath = function(base, rel) { @@ -62,22 +61,17 @@ 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 + const url = new URL(generateUrl(`/f/${id}`), window.location) + return url.href } return ref } @@ -90,7 +84,8 @@ const parseHref = function(dom) { const match = ref.match(/\?dir=([^&]*)&openfile=([^&]*)#relPath=([^&]*)/) if (match) { const [, , id, path] = match - return `${path}?fileId=${id}` + const url = new URL(generateUrl(`/f/${id}`), window.location) + return url.href } return ref } diff --git a/src/tests/helpers/links.spec.js b/src/tests/helpers/links.spec.js index 12d4e614602..3f5400ad284 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('http://localhost/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('http://localhost/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('http://localhost/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('http://localhost/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('http://localhost/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('http://localhost/f/123') }) -}) + test('old relative link format with ../ (from file picker) is rewritten', () => { + expect(insertAndExtract({href: '../otherfile?fileId=123'})) + .toBe('http://localhost/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('http://localhost/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: 'http://localhost/f/123'})) + .toBe('http://localhost/f/123') }) + + test('absolute link to collectives page is unchanged', () => { + expect(insertAndExtract({href: '/apps/collectives/page?fileId=123'})) + .toBe('/apps/collectives/page?fileId=123') + }) + })