Skip to content

Commit

Permalink
fix(links): Insert /f/<fileId> format for links to files
Browse files Browse the repository at this point in the history
Get rid of the old `/path/?fileId=<id>` 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/<fileId>` 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 <[email protected]>
  • Loading branch information
mejo- committed Feb 24, 2024
1 parent 9afef9d commit 81ec67c
Show file tree
Hide file tree
Showing 6 changed files with 53 additions and 92 deletions.
15 changes: 11 additions & 4 deletions cypress/e2e/nodes/Links.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand All @@ -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() {
Expand Down Expand Up @@ -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)
}

Expand All @@ -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)
})
})
})
})
Expand Down
7 changes: 5 additions & 2 deletions cypress/e2e/workspace.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Expand All @@ -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}')
Expand Down
6 changes: 2 additions & 4 deletions src/components/Menu/ActionInsertLink.vue
Original file line number Diff line number Diff line change
Expand Up @@ -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'

Expand Down Expand Up @@ -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}/` : '')
})
Expand Down
20 changes: 0 additions & 20 deletions src/helpers/files.js
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -260,7 +241,6 @@ export const FilesWorkspaceHeader = new Header({
})

export {
optimalPath,
registerFileActionFallback,
registerFileCreate,
FILE_ACTION_IDENTIFIER,
Expand Down
46 changes: 7 additions & 39 deletions src/helpers/links.js
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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
}
Expand All @@ -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
}
Expand Down
51 changes: 28 additions & 23 deletions src/tests/helpers/links.spec.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import { domHref, parseHref } from '../../helpers/links'
import { loadState } from '@nextcloud/initial-state'

global.OCA = {
Viewer: {
Expand All @@ -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', () => {
Expand All @@ -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')
})

})
Expand All @@ -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')
})

})
Expand All @@ -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')
})

})

0 comments on commit 81ec67c

Please sign in to comment.