Skip to content

Commit

Permalink
fix(files): Ensure renaming state is correctly reset
Browse files Browse the repository at this point in the history 
Problem: Is a node is renamed and the new name is out of the current
visible list of nodes the component will be recycled, this means
the props will change, so when the `onRename` functions is about to reset
the state the `this.source` will point to a different node.

To fix this, but also to separate business logic from visual representation,
the logic is moved into the renaming store and the component is only
responsible for rendering.

Signed-off-by: Ferdinand Thiessen <[email protected]>
  • Loading branch information
@susnux @backportbot
susnux authored and backportbot[bot] committed Oct 16, 2024
1 parent 281de60 commit 606dc61
Show file tree
Hide file tree
Showing 3 changed files with 122 additions and 61 deletions.
70 changes: 12 additions & 58 deletions apps/files/src/components/FileEntry/FileEntryName.vue
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,12 +40,9 @@
import type { FileAction, Node } from '@nextcloud/files'
import type { PropType } from 'vue'
import axios, { isAxiosError } from '@nextcloud/axios'
import { showError, showSuccess } from '@nextcloud/dialogs'
import { emit } from '@nextcloud/event-bus'
import { FileType, NodeStatus } from '@nextcloud/files'
import { translate as t } from '@nextcloud/l10n'
import { dirname } from '@nextcloud/paths'
import { defineComponent, inject } from 'vue'
import NcTextField from '@nextcloud/vue/dist/Components/NcTextField.js'
Expand Down Expand Up @@ -244,66 +241,23 @@ export default defineComponent({
}
const oldName = this.source.basename
const oldEncodedSource = this.source.encodedSource
if (oldName === newName) {
this.stopRenaming()
return
}
// Set loading state
this.$set(this.source, 'status', NodeStatus.LOADING)
// Update node
this.source.rename(newName)
logger.debug('Moving file to', { destination: this.source.encodedSource, oldEncodedSource })
try {
await axios({
method: 'MOVE',
url: oldEncodedSource,
headers: {
Destination: this.source.encodedSource,
Overwrite: 'F',
},
})
// Success 🎉
emit('files:node:updated', this.source)
emit('files:node:renamed', this.source)
emit('files:node:moved', {
node: this.source,
oldSource: `${dirname(this.source.source)}/${oldName}`,
})
showSuccess(t('files', 'Renamed "{oldName}" to "{newName}"', { oldName, newName }))
// Reset the renaming store
this.stopRenaming()
this.$nextTick(() => {
const nameContainter = this.$refs.basename as HTMLElement | undefined
nameContainter?.focus()
})
const status = await this.renamingStore.rename()
if (status) {
showSuccess(t('files', 'Renamed "{oldName}" to "{newName}"', { oldName, newName }))
this.$nextTick(() => {
const nameContainter = this.$refs.basename as HTMLElement | undefined
nameContainter?.focus()
})
} else {
// Was cancelled - meaning the renaming state is just reset
}
} catch (error) {
logger.error('Error while renaming file', { error })
// Rename back as it failed
this.source.rename(oldName)
logger.error(error as Error)
showError((error as Error).message)
// And ensure we reset to the renaming state
this.startRenaming()
if (isAxiosError(error)) {
// TODO: 409 means current folder does not exist, redirect ?
if (error?.response?.status === 404) {
showError(t('files', 'Could not rename "{oldName}", it does not exist any more', { oldName }))
return
} else if (error?.response?.status === 412) {
showError(t('files', 'The name "{newName}" is already used in the folder "{dir}". Please choose a different name.', { newName, dir: this.directory }))
return
}
}
// Unknown error
showError(t('files', 'Could not rename "{oldName}"', { oldName }))
} finally {
this.$set(this.source, 'status', undefined)
}
},
Expand Down
83 changes: 81 additions & 2 deletions apps/files/src/store/renaming.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,17 +2,96 @@
* SPDX-FileCopyrightText: 2023 Nextcloud GmbH and Nextcloud contributors
* SPDX-License-Identifier: AGPL-3.0-or-later
*/
import { defineStore } from 'pinia'
import { subscribe } from '@nextcloud/event-bus'
import type { Node } from '@nextcloud/files'
import type { RenamingStore } from '../types'

import axios, { isAxiosError } from '@nextcloud/axios'
import { emit, subscribe } from '@nextcloud/event-bus'
import { NodeStatus } from '@nextcloud/files'
import { t } from '@nextcloud/l10n'
import { basename, dirname } from 'path'
import { defineStore } from 'pinia'
import logger from '../logger'
import Vue from 'vue'

export const useRenamingStore = function(...args) {
const store = defineStore('renaming', {
state: () => ({
renamingNode: undefined,
newName: '',
} as RenamingStore),

actions: {
/**
* Execute the renaming.
* This will rename the node set as `renamingNode` to the configured new name `newName`.
* @return true if success, false if skipped (e.g. new and old name are the same)
* @throws Error if renaming fails, details are set in the error message
*/
async rename(): Promise<boolean> {
if (this.renamingNode === undefined) {
throw new Error('No node is currently being renamed')
}

const newName = this.newName.trim?.() || ''
const oldName = this.renamingNode.basename
const oldEncodedSource = this.renamingNode.encodedSource
if (oldName === newName) {
return false
}

const node = this.renamingNode
Vue.set(node, 'status', NodeStatus.LOADING)

try {
// rename the node
this.renamingNode.rename(newName)
logger.debug('Moving file to', { destination: this.renamingNode.encodedSource, oldEncodedSource })
// create MOVE request
await axios({
method: 'MOVE',
url: oldEncodedSource,
headers: {
Destination: this.renamingNode.encodedSource,
Overwrite: 'F',
},
})

// Success 🎉
emit('files:node:updated', this.renamingNode as Node)
emit('files:node:renamed', this.renamingNode as Node)
emit('files:node:moved', {
node: this.renamingNode as Node,
oldSource: `${dirname(this.renamingNode.source)}/${oldName}`,
})
this.$reset()
return true
} catch (error) {
logger.error('Error while renaming file', { error })
// Rename back as it failed
this.renamingNode.rename(oldName)
if (isAxiosError(error)) {
// TODO: 409 means current folder does not exist, redirect ?
if (error?.response?.status === 404) {
throw new Error(t('files', 'Could not rename "{oldName}", it does not exist any more', { oldName }))
} else if (error?.response?.status === 412) {
throw new Error(t(
'files',
'The name "{newName}" is already used in the folder "{dir}". Please choose a different name.',
{
newName,
dir: basename(this.renamingNode.dirname),
},
))
}
}
// Unknown error
throw new Error(t('files', 'Could not rename "{oldName}"', { oldName }))
} finally {
Vue.set(node, 'status', undefined)
}
},
},
})

const renamingStore = store(...args)
Expand Down
30 changes: 29 additions & 1 deletion cypress/e2e/files/files-renaming.cy.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
*/

import type { User } from '@nextcloud/cypress'
import { getRowForFile, haveValidity, triggerActionForFile } from './FilesUtils'
import { getRowForFile, haveValidity, renameFile, triggerActionForFile } from './FilesUtils'

describe('files: Rename nodes', { testIsolation: true }, () => {
let user: User
Expand Down Expand Up @@ -112,4 +112,32 @@ describe('files: Rename nodes', { testIsolation: true }, () => {
.findByRole('img', { name: 'File is loading' })
.should('not.exist')
})

/**
* This is a regression test of: https://github.com/nextcloud/server/issues/47438
* The issue was that the renaming state was not reset when the new name moved the file out of the view of the current files list
* due to virtual scrolling the renaming state was not changed then by the UI events (as the component was taken out of DOM before any event handling).
*/
it('correctly resets renaming state', () => {
for (let i = 1; i <= 20; i++) {
cy.uploadContent(user, new Blob([]), 'text/plain', `/file${i}.txt`)
}
cy.viewport(1200, 500) // 500px is smaller then 20 * 50 which is the place that the files take up
cy.login(user)
cy.visit('/apps/files')

getRowForFile('file.txt').should('be.visible')
// Z so it is shown last
renameFile('file.txt', 'zzz.txt')
// not visible any longer
getRowForFile('zzz.txt').should('not.be.visible')
// scroll file list to bottom
cy.get('[data-cy-files-list]').scrollTo('bottom')
cy.screenshot()
// The file is no longer in rename state
getRowForFile('zzz.txt')
.should('be.visible')
.findByRole('textbox', { name: 'Filename' })
.should('not.exist')
})
})

0 comments on commit 606dc61

Please sign in to comment.