Merge pull request #48239 from nextcloud/backport/47905/stable29

[stable29] fix(files): Ensure children are removed from folder and not duplicated

apps/files/src/eventbus.d.ts

@@ -3,9 +3,12 @@ import type { Node } from '@nextcloud/files'
 declare module '@nextcloud/event-bus' {
  export interface NextcloudEvents {
  // mapping of 'event name' => 'event type'
- 'files:favorites:removed': Node
  'files:favorites:added': Node
+ 'files:favorites:removed': Node
+ 'files:node:created': Node
+ 'files:node:deleted': Node
  'files:node:renamed': Node
+ 'files:node:updated': Node
  'nextcloud:unified-search.search': { query: string }
  }
 }

apps/files/src/store/files.ts

@@ -44,9 +44,10 @@ const fetchNode = async (node: Node): Promise<Node> => {
 
 export const useFilesStore = function(...args) {
  const store = defineStore('files', {
- state: (): FilesState => ({
+ state: () => ({
  files: {} as FilesStore,
  roots: {} as RootsStore,
+ _initialized: false,
  }),
 
  getters: {

apps/files/src/store/paths.spec.ts

@@ -0,0 +1,130 @@
+/**
+ * SPDX-FileCopyrightText: 2024 Nextcloud GmbH and Nextcloud contributors
+ * SPDX-License-Identifier: AGPL-3.0-or-later
+ */
+
+import { describe, beforeEach, test, expect } from '@jest/globals'
+import { setActivePinia, createPinia } from 'pinia'
+import { usePathsStore } from './paths.ts'
+import { emit } from '@nextcloud/event-bus'
+import { File, Folder } from '@nextcloud/files'
+import { useFilesStore } from './files.ts'
+
+describe('Path store', () => {
+
+ let store: ReturnType<typeof usePathsStore>
+ let files: ReturnType<typeof useFilesStore>
+ let root: Folder & { _children?: string[] }
+
+ beforeEach(() => {
+ setActivePinia(createPinia())
+
+ root = new Folder({ owner: 'test', source: 'http://example.com/remote.php/dav/files/test/', id: 1 })
+ files = useFilesStore()
+ files.setRoot({ service: 'files', root })
+
+ store = usePathsStore()
+ })
+
+ test('Folder is created', () => {
+ // no defined paths
+ expect(store.paths).toEqual({})
+
+ // create the folder
+ const node = new Folder({ owner: 'test', source: 'http://example.com/remote.php/dav/files/test/folder', id: 2 })
+ emit('files:node:created', node)
+
+ // see that the path is added
+ expect(store.paths).toEqual({ files: { [node.path]: node.source } })
+
+ // see that the node is added
+ expect(root._children).toEqual([node.source])
+ })
+
+ test('File is created', () => {
+ // no defined paths
+ expect(store.paths).toEqual({})
+
+ // create the file
+ const node = new File({ owner: 'test', source: 'http://example.com/remote.php/dav/files/test/file.txt', id: 2, mime: 'text/plain' })
+ emit('files:node:created', node)
+
+ // see that there are still no paths
+ expect(store.paths).toEqual({})
+
+ // see that the node is added
+ expect(root._children).toEqual([node.source])
+ })
+
+ test('Existing file is created', () => {
+ // no defined paths
+ expect(store.paths).toEqual({})
+
+ // create the file
+ const node1 = new File({ owner: 'test', source: 'http://example.com/remote.php/dav/files/test/file.txt', id: 2, mime: 'text/plain' })
+ emit('files:node:created', node1)
+
+ // see that there are still no paths
+ expect(store.paths).toEqual({})
+
+ // see that the node is added
+ expect(root._children).toEqual([node1.source])
+
+ // create the same named file again
+ const node2 = new File({ owner: 'test', source: 'http://example.com/remote.php/dav/files/test/file.txt', id: 2, mime: 'text/plain' })
+ emit('files:node:created', node2)
+
+ // see that there are still no paths and the children are not duplicated
+ expect(store.paths).toEqual({})
+ expect(root._children).toEqual([node1.source])
+
+ })
+
+ test('Existing folder is created', () => {
+ // no defined paths
+ expect(store.paths).toEqual({})
+
+ // create the file
+ const node1 = new Folder({ owner: 'test', source: 'http://example.com/remote.php/dav/files/test/folder', id: 2 })
+ emit('files:node:created', node1)
+
+ // see the path is added
+ expect(store.paths).toEqual({ files: { [node1.path]: node1.source } })
+
+ // see that the node is added
+ expect(root._children).toEqual([node1.source])
+
+ // create the same named file again
+ const node2 = new Folder({ owner: 'test', source: 'http://example.com/remote.php/dav/files/test/folder', id: 2 })
+ emit('files:node:created', node2)
+
+ // see that there is still only one paths and the children are not duplicated
+ expect(store.paths).toEqual({ files: { [node1.path]: node1.source } })
+ expect(root._children).toEqual([node1.source])
+ })
+
+ test('Folder is deleted', () => {
+ const node = new Folder({ owner: 'test', source: 'http://example.com/remote.php/dav/files/test/folder', id: 2 })
+ emit('files:node:created', node)
+ // see that the path is added and the children are set-up
+ expect(store.paths).toEqual({ files: { [node.path]: node.source } })
+ expect(root._children).toEqual([node.source])
+
+ emit('files:node:deleted', node)
+ // See the path is removed
+ expect(store.paths).toEqual({ files: {} })
+ // See the child is removed
+ expect(root._children).toEqual([])
+ })
+
+ test('File is deleted', () => {
+ const node = new File({ owner: 'test', source: 'http://example.com/remote.php/dav/files/test/file.txt', id: 2, mime: 'text/plain' })
+ emit('files:node:created', node)
+ // see that the children are set-up
+ expect(root._children).toEqual([node.source])
+
+ emit('files:node:deleted', node)
+ // See the child is removed
+ expect(root._children).toEqual([])
+ })
+})

apps/files/src/store/paths.ts

@@ -19,7 +19,7 @@
  * along with this program. If not, see <http://www.gnu.org/licenses/>.
  *
  */
-import type { FileSource, PathsStore, PathOptions, ServicesState } from '../types'
+import type { FileSource, PathsStore, PathOptions, ServicesState, Service } from '../types'
 import { defineStore } from 'pinia'
 import { FileType, Folder, Node, getNavigation } from '@nextcloud/files'
 import { subscribe } from '@nextcloud/event-bus'
@@ -34,7 +34,8 @@ export const usePathsStore = function(...args) {
  const store = defineStore('paths', {
  state: () => ({
  paths: {} as ServicesState,
- } as PathsStore),
+ _initialized: false,
+ }),
 
  getters: {
  getPath: (state) => {
@@ -58,6 +59,57 @@ export const usePathsStore = function(...args) {
  Vue.set(this.paths[payload.service], payload.path, payload.source)
  },
 
+ deletePath(service: Service, path: string) {
+ // skip if service does not exist
+ if (!this.paths[service]) {
+ return
+ }
+
+ Vue.delete(this.paths[service], path)
+ },
+
+ onDeletedNode(node: Node) {
+ const service = getNavigation()?.active?.id || 'files'
+
+ if (node.type === FileType.Folder) {
+ // Delete the path
+ this.deletePath(
+ service,
+ node.path,
+ )
+ }
+
+ // Remove node from children
+ if (node.dirname === '/') {
+ const root = files.getRoot(service) as Folder & { _children?: string[] }
+ // ensure sources are unique
+ const children = new Set(root._children ?? [])
+ children.delete(node.source)
+ Vue.set(root, '_children', [...children.values()])
+ return
+ }
+
+ if (this.paths[service][node.dirname]) {
+ const parentSource = this.paths[service][node.dirname]
+ const parentFolder = files.getNode(parentSource) as Folder & { _children?: string[] }
+
+ if (!parentFolder) {
+ logger.error('Parent folder not found', { parentSource })
+ return
+ }
+
+ logger.debug('Path exists, removing from children', { parentFolder, node })
+
+ // ensure sources are unique
+ const children = new Set(parentFolder._children ?? [])
+ children.delete(node.source)
+ Vue.set(parentFolder, '_children', [...children.values()])
+ return
+ }
+
+ logger.debug('Parent path does not exists, skipping children update', { node })
+ },
+
  onCreatedNode(node: Node) {
  const service = getNavigation()?.active?.id || 'files'
  if (!node.fileid) {
@@ -77,30 +129,30 @@ export const usePathsStore = function(...args) {
  // Update parent folder children if exists
  // If the folder is the root, get it and update it
  if (node.dirname === '/') {
- const root = files.getRoot(service)
- if (!root._children) {
-  Vue.set(root, '_children', [])
- }
- root._children.push(node.source)
+ const root = files.getRoot(service) as Folder & { _children?: string[] }
+ // ensure sources are unique
+ const children = new Set(root._children ?? [])
+ children.add(node.source)
+ Vue.set(root, '_children', [...children.values()])
  return
  }
 
  // If the folder doesn't exists yet, it will be
  // fetched later and its children updated anyway.
  if (this.paths[service][node.dirname]) {
  const parentSource = this.paths[service][node.dirname]
- const parentFolder = files.getNode(parentSource) as Folder
+ const parentFolder = files.getNode(parentSource) as Folder & { _children?: string[] }
  logger.debug('Path already exists, updating children', { parentFolder, node })
 
  if (!parentFolder) {
  logger.error('Parent folder not found', { parentSource })
  return
  }
 
- if (!parentFolder._children) {
-  Vue.set(parentFolder, '_children', [])
- }
- parentFolder._children.push(node.source)
+ // ensure sources are unique
+ const children = new Set(parentFolder._children ?? [])
+ children.add(node.source)
+ Vue.set(parentFolder, '_children', [...children.values()])
  return
  }
 
@@ -114,7 +166,7 @@ export const usePathsStore = function(...args) {
  if (!pathsStore._initialized) {
  // TODO: watch folders to update paths?
  subscribe('files:node:created', pathsStore.onCreatedNode)
- // subscribe('files:node:deleted', pathsStore.onDeletedNode)
+ subscribe('files:node:deleted', pathsStore.onDeletedNode)
  // subscribe('files:node:moved', pathsStore.onMovedNode)
 
  pathsStore._initialized = true

apps/files/src/views/FilesList.vue

@@ -652,6 +652,7 @@ export default defineComponent({
  }
  sidebarAction.exec(this.currentFolder, this.currentView!, this.currentFolder.path)
  },
+
  toggleGridView() {
  this.userConfigStore.update('grid_view', !this.userConfig.grid_view)
  },

cypress/e2e/files/FilesUtils.ts

@@ -45,6 +45,21 @@ export const triggerInlineActionForFile = (filename: string, actionId: string) =
  getActionsForFile(filename).get(`button[data-cy-files-list-row-action="${CSS.escape(actionId)}"]`).should('exist').click()
 }
 
+export const createFolder = (folderName: string) => {
+ cy.intercept('MKCOL', /\/remote.php\/dav\/files\//).as('createFolder')
+
+ // TODO: replace by proper data-cy selectors
+ cy.get('[data-cy-upload-picker] .action-item__menutoggle').first().click()
+ cy.contains('.upload-picker__menu-entry button', 'New folder').click()
+ cy.get('[data-cy-files-new-node-dialog]').should('be.visible')
+ cy.get('[data-cy-files-new-node-dialog-input]').type(`{selectall}${folderName}`)
+ cy.get('[data-cy-files-new-node-dialog-submit]').click()
+
+ cy.wait('@createFolder')
+
+ getRowForFile(folderName).should('be.visible')
+}
+
 export const moveFile = (fileName: string, dirPath: string) => {
  getRowForFile(fileName).should('be.visible')
  triggerActionForFile(fileName, 'move-copy')

cypress/e2e/files/duplicated-node-regression.cy.ts

@@ -0,0 +1,33 @@
+/**
+ * SPDX-FileCopyrightText: 2024 Nextcloud GmbH and Nextcloud contributors
+ * SPDX-License-Identifier: AGPL-3.0-or-later
+ */
+
+import { createFolder, getRowForFile, triggerActionForFile } from './FilesUtils.ts'
+
+before(() => {
+ cy.createRandomUser()
+ .then((user) => {
+ cy.mkdir(user, '/only once')
+ cy.login(user)
+ cy.visit('/apps/files')
+ })
+})
+
+/**
+ * Regression test for https://github.com/nextcloud/server/issues/47904
+ */
+it('Ensure nodes are not duplicated in the file list', () => {
+ // See the folder
+ getRowForFile('only once').should('be.visible')
+ // Delete the folder
+ cy.intercept('DELETE', '**/remote.php/dav/**').as('deleteFolder')
+ triggerActionForFile('only once', 'delete')
+ cy.wait('@deleteFolder')
+ getRowForFile('only once').should('not.exist')
+ // Create the folder again
+ createFolder('only once')
+ // See folder exists only once
+ getRowForFile('only once')
+ .should('have.length', 1)
+})