Merge pull request #5412 from nextcloud/fix/link_to_files

fix(links): Insert `/f/<fileId>` format for links to files

cypress/e2e/nodes/Links.spec.js

@@ -181,20 +181,28 @@ describe('test link marks', function() {
 				})
 
 				return cy.getContent()
-					.find(`a[href*="${encodeURIComponent(filename)}"]`)
+					.find(`a[href*="/index.php/f/${fileId}"]`)
 					.should('have.text', text === undefined ? filename : text)
 			}
 
+			let fileId = null
+
 			beforeEach(() => cy.clearContent())
 
 			it('without text', () => {
+				cy.getFileId(fileName).then((id) => {
+					fileId = id
+				})
 				cy.getFile(fileName)
 					.then($el => {
 						checkLinkFile(fileName)
 						cy.get('.modal-name').should('include.text', fileName)
 					})
 			})
 			it('with selected text', () => {
+				cy.getFileId(fileName).then((id) => {
+					fileId = id
+				})
 				cy.getFile(fileName)
 					.then($el => {
 						cy.getContent().type(`${text}{selectAll}`)
@@ -203,7 +211,9 @@ describe('test link marks', function() {
 					})
 			})
 			it('link to directory', () => {
-				cy.createFolder(`${window.__currentDirectory}/dummy folder`)
+				cy.createFolder(`${window.__currentDirectory}/dummy folder`).then((folderId) => {
+					fileId = folderId
+				})
 				cy.getFile(fileName).then($el => {
 					cy.getContent().type(`${text}{selectAll}`)
 					checkLinkFile('dummy folder', text, true)

cypress/e2e/workspace.spec.js

@@ -157,7 +157,7 @@ describe('Workspace', function() {
 			.contains('😀')
 	})
 
-	it('relative folder links', function() {
+	it('file links', function() {
 		cy.createFolder(`${this.testFolder}/sub-folder`)
 		cy.createFolder(`${this.testFolder}/sub-folder/alpha`)
 
@@ -179,9 +179,6 @@ 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')
 
 		cy.getContent()
 			.type('{leftArrow}')

cypress/support/commands.js

@@ -261,13 +261,15 @@ Cypress.Commands.add('createFolder', (target) => {
 
 	return cy.getRequestToken()
 		.then(requesttoken => {
-			return cy.request({
+			cy.request({
 				url: `${rootPath}/${dirPath}`,
 				method: 'MKCOL',
 				auth,
 				headers: {
 					requesttoken,
 				},
+			}).then((response) => {
+				return parseInt(response.headers['oc-fileid'])
 			})
 		})
 })

src/components/Link/LinkBubbleView.vue

@@ -149,9 +149,8 @@ export default {
 		 * NcReferenceList only accepts full URLs with origin.
 		 */
 		sanitizedHref() {
-			return this.href?.startsWith('/')
-				? location.origin + this.href
-				: this.href
+			const url = new URL(this.href, window.location)
+			return url.href
 		},
 
 		title() {

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}/` : '')
 					})

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,

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/<fileId>`
 	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
 }

src/nodes/ParagraphView.vue

@@ -106,9 +106,9 @@ export default {
 			const linkMark = textNode?.marks.find((m) => m.type.name === 'link')
 			const href = linkMark?.attrs?.href
 
-			const PATTERN = /(^)(https?:\/\/)((?:[-A-Z0-9+_]+\.)+[-A-Z]+(?:\/[-A-Z0-9+&@#%?=~_|!:,.;()]*)*)($)/ig
-			if ((new RegExp(PATTERN)).test(href)) {
-				return href
+			if (href) {
+				const url = new URL(href, window.location)
+				return url.href
 			}
 
 			return null

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:[email protected]')
 	})
 
-	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')
+	})
+
 })