Skip to content

Commit 876262d

Browse files
susnuxsusnux
authored
Merge pull request #48731 from nextcloud/backport/48425/stable30
[stable30] fix(files): Ensure renaming state is correctly reset
2 parents 7f472cc + f6fe039 commit 876262d

5 files changed

Lines changed: 125 additions & 64 deletions

File tree

‎apps/files/src/components/FileEntry/FileEntryName.vue‎

Lines changed: 12 additions & 58 deletions
Original file line numberDiff line numberDiff line change
@@ -40,12 +40,9 @@
4040
import type { FileAction, Node } from '@nextcloud/files'
4141
import type { PropType } from 'vue'
4242
43-
import axios, { isAxiosError } from '@nextcloud/axios'
4443
import { showError, showSuccess } from '@nextcloud/dialogs'
45-
import { emit } from '@nextcloud/event-bus'
4644
import { FileType, NodeStatus } from '@nextcloud/files'
4745
import { translate as t } from '@nextcloud/l10n'
48-
import { dirname } from '@nextcloud/paths'
4946
import { defineComponent, inject } from 'vue'
5047
5148
import NcTextField from '@nextcloud/vue/dist/Components/NcTextField.js'
@@ -244,66 +241,23 @@ export default defineComponent({
244241
}
245242
246243
const oldName = this.source.basename
247-
const oldEncodedSource = this.source.encodedSource
248-
if (oldName === newName) {
249-
this.stopRenaming()
250-
return
251-
}
252-
253-
// Set loading state
254-
this.$set(this.source, 'status', NodeStatus.LOADING)
255244
256-
// Update node
257-
this.source.rename(newName)
258-
259-
logger.debug('Moving file to', { destination: this.source.encodedSource, oldEncodedSource })
260245
try {
261-
await axios({
262-
method: 'MOVE',
263-
url: oldEncodedSource,
264-
headers: {
265-
Destination: this.source.encodedSource,
266-
Overwrite: 'F',
267-
},
268-
})
269-
270-
// Success 🎉
271-
emit('files:node:updated', this.source)
272-
emit('files:node:renamed', this.source)
273-
emit('files:node:moved', {
274-
node: this.source,
275-
oldSource: `${dirname(this.source.source)}/${oldName}`,
276-
})
277-
showSuccess(t('files', 'Renamed "{oldName}" to "{newName}"', { oldName, newName }))
278-
279-
// Reset the renaming store
280-
this.stopRenaming()
281-
this.$nextTick(() => {
282-
const nameContainter = this.$refs.basename as HTMLElement | undefined
283-
nameContainter?.focus()
284-
})
246+
const status = await this.renamingStore.rename()
247+
if (status) {
248+
showSuccess(t('files', 'Renamed "{oldName}" to "{newName}"', { oldName, newName }))
249+
this.$nextTick(() => {
250+
const nameContainter = this.$refs.basename as HTMLElement | undefined
251+
nameContainter?.focus()
252+
})
253+
} else {
254+
// Was cancelled - meaning the renaming state is just reset
255+
}
285256
} catch (error) {
286-
logger.error('Error while renaming file', { error })
287-
// Rename back as it failed
288-
this.source.rename(oldName)
257+
logger.error(error as Error)
258+
showError((error as Error).message)
289259
// And ensure we reset to the renaming state
290260
this.startRenaming()
291-
292-
if (isAxiosError(error)) {
293-
// TODO: 409 means current folder does not exist, redirect ?
294-
if (error?.response?.status === 404) {
295-
showError(t('files', 'Could not rename "{oldName}", it does not exist any more', { oldName }))
296-
return
297-
} else if (error?.response?.status === 412) {
298-
showError(t('files', 'The name "{newName}" is already used in the folder "{dir}". Please choose a different name.', { newName, dir: this.directory }))
299-
return
300-
}
301-
}
302-
303-
// Unknown error
304-
showError(t('files', 'Could not rename "{oldName}"', { oldName }))
305-
} finally {
306-
this.$set(this.source, 'status', undefined)
307261
}
308262
},
309263

‎apps/files/src/store/renaming.ts‎

Lines changed: 81 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,17 +2,96 @@
22
* SPDX-FileCopyrightText: 2023 Nextcloud GmbH and Nextcloud contributors
33
* SPDX-License-Identifier: AGPL-3.0-or-later
44
*/
5-
import { defineStore } from 'pinia'
6-
import { subscribe } from '@nextcloud/event-bus'
75
import type { Node } from '@nextcloud/files'
86
import type { RenamingStore } from '../types'
97

8+
import axios, { isAxiosError } from '@nextcloud/axios'
9+
import { emit, subscribe } from '@nextcloud/event-bus'
10+
import { NodeStatus } from '@nextcloud/files'
11+
import { t } from '@nextcloud/l10n'
12+
import { basename, dirname } from 'path'
13+
import { defineStore } from 'pinia'
14+
import logger from '../logger'
15+
import Vue from 'vue'
16+
1017
export const useRenamingStore = function(...args) {
1118
const store = defineStore('renaming', {
1219
state: () => ({
1320
renamingNode: undefined,
1421
newName: '',
1522
} as RenamingStore),
23+
24+
actions: {
25+
/**
26+
* Execute the renaming.
27+
* This will rename the node set as `renamingNode` to the configured new name `newName`.
28+
* @return true if success, false if skipped (e.g. new and old name are the same)
29+
* @throws Error if renaming fails, details are set in the error message
30+
*/
31+
async rename(): Promise<boolean> {
32+
if (this.renamingNode === undefined) {
33+
throw new Error('No node is currently being renamed')
34+
}
35+
36+
const newName = this.newName.trim?.() || ''
37+
const oldName = this.renamingNode.basename
38+
const oldEncodedSource = this.renamingNode.encodedSource
39+
if (oldName === newName) {
40+
return false
41+
}
42+
43+
const node = this.renamingNode
44+
Vue.set(node, 'status', NodeStatus.LOADING)
45+
46+
try {
47+
// rename the node
48+
this.renamingNode.rename(newName)
49+
logger.debug('Moving file to', { destination: this.renamingNode.encodedSource, oldEncodedSource })
50+
// create MOVE request
51+
await axios({
52+
method: 'MOVE',
53+
url: oldEncodedSource,
54+
headers: {
55+
Destination: this.renamingNode.encodedSource,
56+
Overwrite: 'F',
57+
},
58+
})
59+
60+
// Success 🎉
61+
emit('files:node:updated', this.renamingNode as Node)
62+
emit('files:node:renamed', this.renamingNode as Node)
63+
emit('files:node:moved', {
64+
node: this.renamingNode as Node,
65+
oldSource: `${dirname(this.renamingNode.source)}/${oldName}`,
66+
})
67+
this.$reset()
68+
return true
69+
} catch (error) {
70+
logger.error('Error while renaming file', { error })
71+
// Rename back as it failed
72+
this.renamingNode.rename(oldName)
73+
if (isAxiosError(error)) {
74+
// TODO: 409 means current folder does not exist, redirect ?
75+
if (error?.response?.status === 404) {
76+
throw new Error(t('files', 'Could not rename "{oldName}", it does not exist any more', { oldName }))
77+
} else if (error?.response?.status === 412) {
78+
throw new Error(t(
79+
'files',
80+
'The name "{newName}" is already used in the folder "{dir}". Please choose a different name.',
81+
{
82+
newName,
83+
dir: basename(this.renamingNode.dirname),
84+
},
85+
))
86+
}
87+
}
88+
// Unknown error
89+
throw new Error(t('files', 'Could not rename "{oldName}"', { oldName }))
90+
} finally {
91+
Vue.set(node, 'status', undefined)
92+
}
93+
},
94+
},
1695
})
1796

1897
const renamingStore = store(...args)

‎cypress/e2e/files/files-renaming.cy.ts‎

Lines changed: 29 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44
*/
55

66
import type { User } from '@nextcloud/cypress'
7-
import { getRowForFile, haveValidity, triggerActionForFile } from './FilesUtils'
7+
import { getRowForFile, haveValidity, renameFile, triggerActionForFile } from './FilesUtils'
88

99
describe('files: Rename nodes', { testIsolation: true }, () => {
1010
let user: User
@@ -112,4 +112,32 @@ describe('files: Rename nodes', { testIsolation: true }, () => {
112112
.findByRole('img', { name: 'File is loading' })
113113
.should('not.exist')
114114
})
115+
116+
/**
117+
* This is a regression test of: https://github.com/nextcloud/server/issues/47438
118+
* 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
119+
* 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).
120+
*/
121+
it('correctly resets renaming state', () => {
122+
for (let i = 1; i <= 20; i++) {
123+
cy.uploadContent(user, new Blob([]), 'text/plain', `/file${i}.txt`)
124+
}
125+
cy.viewport(1200, 500) // 500px is smaller then 20 * 50 which is the place that the files take up
126+
cy.login(user)
127+
cy.visit('/apps/files')
128+
129+
getRowForFile('file.txt').should('be.visible')
130+
// Z so it is shown last
131+
renameFile('file.txt', 'zzz.txt')
132+
// not visible any longer
133+
getRowForFile('zzz.txt').should('not.be.visible')
134+
// scroll file list to bottom
135+
cy.get('[data-cy-files-list]').scrollTo('bottom')
136+
cy.screenshot()
137+
// The file is no longer in rename state
138+
getRowForFile('zzz.txt')
139+
.should('be.visible')
140+
.findByRole('textbox', { name: 'Filename' })
141+
.should('not.exist')
142+
})
115143
})

‎dist/files-main.js‎

Lines changed: 2 additions & 2 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎dist/files-main.js.map‎

Lines changed: 1 addition & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)