From fd15cfb77227d10da0c2acc40ea66f605d966e94 Mon Sep 17 00:00:00 2001 From: Pascal Wengerter Date: Thu, 20 May 2021 14:58:52 +0100 Subject: [PATCH 1/3] Introduce UPSERT_RESOURCE to files mutations --- .../unreleased/bugfix-upsert-resource-in-filestable | 7 +++++++ packages/web-app-files/src/components/AppBar.vue | 8 ++++---- packages/web-app-files/src/store/actions.js | 4 ++-- packages/web-app-files/src/store/mutations.js | 12 ++++-------- packages/web-app-files/src/views/SharedWithMe.vue | 6 +++--- .../web-app-files/src/views/SharedWithOthers.vue | 4 ++-- .../web-app-files/tests/views/SharedWithMe.setup.js | 2 +- 7 files changed, 23 insertions(+), 20 deletions(-) create mode 100644 changelog/unreleased/bugfix-upsert-resource-in-filestable diff --git a/changelog/unreleased/bugfix-upsert-resource-in-filestable b/changelog/unreleased/bugfix-upsert-resource-in-filestable new file mode 100644 index 00000000000..de7b958c6d3 --- /dev/null +++ b/changelog/unreleased/bugfix-upsert-resource-in-filestable @@ -0,0 +1,7 @@ +Bugfix: Upsert resource in filestable + +When uploading an already existing resource in the filestable, we sometimes displayed both +files in the filestable until the page got refreshed. We now check when uploading a file if +it exists in the filestable and replace it there if that is the case. + +https://github.com/owncloud/web/pull/5130 diff --git a/packages/web-app-files/src/components/AppBar.vue b/packages/web-app-files/src/components/AppBar.vue index cd53b97d165..34e799e3aad 100644 --- a/packages/web-app-files/src/components/AppBar.vue +++ b/packages/web-app-files/src/components/AppBar.vue @@ -277,7 +277,7 @@ export default { 'loadIndicators' ]), ...mapActions(['openFile', 'showMessage', 'createModal', 'setModalInputErrorMessage']), - ...mapMutations('Files', ['PUSH_NEW_RESOURCE']), + ...mapMutations('Files', ['UPSERT_RESOURCE']), ...mapMutations(['SET_QUOTA']), showCreateResourceModal(isFolder = true, ext = 'txt', openAction = null) { @@ -338,7 +338,7 @@ export default { } resource = buildResource(resource) - this.PUSH_NEW_RESOURCE(resource) + this.UPSERT_RESOURCE(resource) this.hideModal() if (this.isPersonalRoute) { @@ -427,7 +427,7 @@ export default { resource = buildResource(resource) - this.PUSH_NEW_RESOURCE(resource) + this.UPSERT_RESOURCE(resource) this.hideModal() if (this.isPersonalRoute) { @@ -500,7 +500,7 @@ export default { ) resource = buildResource(resource) - this.PUSH_NEW_RESOURCE(resource) + this.UPSERT_RESOURCE(resource) if (this.isPersonalRoute) { await this.loadIndicators({ diff --git a/packages/web-app-files/src/store/actions.js b/packages/web-app-files/src/store/actions.js index 9259a48a025..af8b3e76a33 100644 --- a/packages/web-app-files/src/store/actions.js +++ b/packages/web-app-files/src/store/actions.js @@ -566,7 +566,7 @@ export default { } resource.preview = previewUrl - commit('UPDATE_RESOURCE', resource) + commit('UPSERT_RESOURCE', resource) continue } catch (ignored) {} @@ -575,7 +575,7 @@ export default { const previewUrl = davUrl + encodePath(resource.path) + '?' + queryString.stringify(query) try { resource.preview = await mediaSource(previewUrl, 'url') - commit('UPDATE_RESOURCE', resource) + commit('UPSERT_RESOURCE', resource) } catch (ignored) {} } } diff --git a/packages/web-app-files/src/store/mutations.js b/packages/web-app-files/src/store/mutations.js index b65ba11792f..c8458da2127 100644 --- a/packages/web-app-files/src/store/mutations.js +++ b/packages/web-app-files/src/store/mutations.js @@ -302,23 +302,19 @@ export default { state.files = files }, - PUSH_NEW_RESOURCE(state, resource) { - const files = [...state.files] - files.push(resource) - state.files = files - }, - SELECT_RESOURCES(state, resources) { state.selected = resources }, - UPDATE_RESOURCE(state, resource) { + UPSERT_RESOURCE(state, resource) { const files = [...state.files] const index = files.findIndex(r => r.id === resource.id) if (index > -1) { files.splice(index, 1, resource) - state.files = files + } else { + files.push(resource) } + state.files = files } } diff --git a/packages/web-app-files/src/views/SharedWithMe.vue b/packages/web-app-files/src/views/SharedWithMe.vue index 951bb118b5c..d387be72638 100644 --- a/packages/web-app-files/src/views/SharedWithMe.vue +++ b/packages/web-app-files/src/views/SharedWithMe.vue @@ -160,7 +160,7 @@ export default { 'LOAD_FILES', 'SELECT_RESOURCES', 'CLEAR_CURRENT_FILES_LIST', - 'UPDATE_RESOURCE' + 'UPSERT_RESOURCE' ]), ...mapMutations(['SET_QUOTA']), @@ -193,7 +193,7 @@ export default { this.configuration.server, this.getToken, this.$client, - this.UPDATE_RESOURCE + this.UPSERT_RESOURCE ) this.LOAD_FILES({ currentFolder: rootFolder, files: resources }) @@ -264,7 +264,7 @@ export default { this.configuration.server, this.getToken ) - this.UPDATE_RESOURCE(sharedResource) + this.UPSERT_RESOURCE(sharedResource) } } catch (error) { this.showMessage({ diff --git a/packages/web-app-files/src/views/SharedWithOthers.vue b/packages/web-app-files/src/views/SharedWithOthers.vue index af4221d617e..6ad8db1e423 100644 --- a/packages/web-app-files/src/views/SharedWithOthers.vue +++ b/packages/web-app-files/src/views/SharedWithOthers.vue @@ -132,7 +132,7 @@ export default { 'LOAD_FILES', 'SELECT_RESOURCES', 'CLEAR_CURRENT_FILES_LIST', - 'UPDATE_RESOURCE' + 'UPSERT_RESOURCE' ]), ...mapMutations(['SET_QUOTA']), @@ -165,7 +165,7 @@ export default { this.configuration.server, this.getToken, this.$client, - this.UPDATE_RESOURCE + this.UPSERT_RESOURCE ) this.LOAD_FILES({ currentFolder: rootFolder, files: resources }) diff --git a/packages/web-app-files/tests/views/SharedWithMe.setup.js b/packages/web-app-files/tests/views/SharedWithMe.setup.js index 76e02116c58..de75bed4a30 100644 --- a/packages/web-app-files/tests/views/SharedWithMe.setup.js +++ b/packages/web-app-files/tests/views/SharedWithMe.setup.js @@ -50,7 +50,7 @@ export const store = createStore(Vuex.Store, { highlightedFile: () => null }, mutations: { - UPDATE_RESOURCE: (state, resource) => { + UPSERT_RESOURCE: (state, resource) => { state.resource = resource } }, From 7b0c3d8f6a818007f0c0f4437b06bc9fefbe87a2 Mon Sep 17 00:00:00 2001 From: Benedikt Kulmann Date: Fri, 11 Jun 2021 10:34:54 +0200 Subject: [PATCH 2/3] Re-introduce UPDATE_RESOURCE for when inserting is not allowed --- packages/web-app-files/src/store/actions.js | 4 +- packages/web-app-files/src/store/mutations.js | 45 +++++++++++++++---- .../web-app-files/src/views/SharedWithMe.vue | 6 +-- .../src/views/SharedWithOthers.vue | 4 +- .../tests/views/SharedWithMe.setup.js | 2 +- 5 files changed, 45 insertions(+), 16 deletions(-) diff --git a/packages/web-app-files/src/store/actions.js b/packages/web-app-files/src/store/actions.js index af8b3e76a33..9259a48a025 100644 --- a/packages/web-app-files/src/store/actions.js +++ b/packages/web-app-files/src/store/actions.js @@ -566,7 +566,7 @@ export default { } resource.preview = previewUrl - commit('UPSERT_RESOURCE', resource) + commit('UPDATE_RESOURCE', resource) continue } catch (ignored) {} @@ -575,7 +575,7 @@ export default { const previewUrl = davUrl + encodePath(resource.path) + '?' + queryString.stringify(query) try { resource.preview = await mediaSource(previewUrl, 'url') - commit('UPSERT_RESOURCE', resource) + commit('UPDATE_RESOURCE', resource) } catch (ignored) {} } } diff --git a/packages/web-app-files/src/store/mutations.js b/packages/web-app-files/src/store/mutations.js index c8458da2127..f3a737c3063 100644 --- a/packages/web-app-files/src/store/mutations.js +++ b/packages/web-app-files/src/store/mutations.js @@ -306,15 +306,44 @@ export default { state.selected = resources }, + /** + * Inserts or updates the given resource, depending on whether or not the resource is already loaded. + * Updating the resource always takes precedence, so that we don't duplicate resources in the store + * accidentally. + * + * @param state Current state of this store module + * @param resource A new or updated resource + */ UPSERT_RESOURCE(state, resource) { - const files = [...state.files] - const index = files.findIndex(r => r.id === resource.id) + $_upsertResource(state, resource, true) + }, - if (index > -1) { - files.splice(index, 1, resource) - } else { - files.push(resource) - } - state.files = files + /** + * Updates the given resource in the store. If the resource doesn't exist in the store, the update + * will be ignored. If you also want to allow inserts, use UPSERT_RESOURCE instead. + * + * @param state Current state of this store module + * @param resource An updated resource + */ + UPDATE_RESOURCE(state, resource) { + $_upsertResource(state, resource, false) + } +} + +// eslint-disable-next-line camelcase +function $_upsertResource(state, resource, allowInsert) { + const files = [...state.files] + const index = files.findIndex(r => r.id === resource.id) + const found = index > -1 + + if (!found && !allowInsert) { + return + } + + if (found) { + files.splice(index, 1, resource) + } else { + files.push(resource) } + state.files = files } diff --git a/packages/web-app-files/src/views/SharedWithMe.vue b/packages/web-app-files/src/views/SharedWithMe.vue index d387be72638..951bb118b5c 100644 --- a/packages/web-app-files/src/views/SharedWithMe.vue +++ b/packages/web-app-files/src/views/SharedWithMe.vue @@ -160,7 +160,7 @@ export default { 'LOAD_FILES', 'SELECT_RESOURCES', 'CLEAR_CURRENT_FILES_LIST', - 'UPSERT_RESOURCE' + 'UPDATE_RESOURCE' ]), ...mapMutations(['SET_QUOTA']), @@ -193,7 +193,7 @@ export default { this.configuration.server, this.getToken, this.$client, - this.UPSERT_RESOURCE + this.UPDATE_RESOURCE ) this.LOAD_FILES({ currentFolder: rootFolder, files: resources }) @@ -264,7 +264,7 @@ export default { this.configuration.server, this.getToken ) - this.UPSERT_RESOURCE(sharedResource) + this.UPDATE_RESOURCE(sharedResource) } } catch (error) { this.showMessage({ diff --git a/packages/web-app-files/src/views/SharedWithOthers.vue b/packages/web-app-files/src/views/SharedWithOthers.vue index 6ad8db1e423..af4221d617e 100644 --- a/packages/web-app-files/src/views/SharedWithOthers.vue +++ b/packages/web-app-files/src/views/SharedWithOthers.vue @@ -132,7 +132,7 @@ export default { 'LOAD_FILES', 'SELECT_RESOURCES', 'CLEAR_CURRENT_FILES_LIST', - 'UPSERT_RESOURCE' + 'UPDATE_RESOURCE' ]), ...mapMutations(['SET_QUOTA']), @@ -165,7 +165,7 @@ export default { this.configuration.server, this.getToken, this.$client, - this.UPSERT_RESOURCE + this.UPDATE_RESOURCE ) this.LOAD_FILES({ currentFolder: rootFolder, files: resources }) diff --git a/packages/web-app-files/tests/views/SharedWithMe.setup.js b/packages/web-app-files/tests/views/SharedWithMe.setup.js index de75bed4a30..76e02116c58 100644 --- a/packages/web-app-files/tests/views/SharedWithMe.setup.js +++ b/packages/web-app-files/tests/views/SharedWithMe.setup.js @@ -50,7 +50,7 @@ export const store = createStore(Vuex.Store, { highlightedFile: () => null }, mutations: { - UPSERT_RESOURCE: (state, resource) => { + UPDATE_RESOURCE: (state, resource) => { state.resource = resource } }, From 9c6c6bbc54c46845cf5cf3ec07c7e34b479e2270 Mon Sep 17 00:00:00 2001 From: Benedikt Kulmann Date: Fri, 11 Jun 2021 18:58:37 +0200 Subject: [PATCH 3/3] Add unit tests for UPSERT_RESOURCE and UPDATE_RESOURCE --- .../tests/store/mutations.spec.js | 61 +++++++++++++++++++ 1 file changed, 61 insertions(+) create mode 100644 packages/web-app-files/tests/store/mutations.spec.js diff --git a/packages/web-app-files/tests/store/mutations.spec.js b/packages/web-app-files/tests/store/mutations.spec.js new file mode 100644 index 00000000000..732b6000046 --- /dev/null +++ b/packages/web-app-files/tests/store/mutations.spec.js @@ -0,0 +1,61 @@ +import mutations from '../../src/store/mutations' +import { cloneDeep } from 'lodash-es' + +const stateFixture = { + files: [ + { + id: 1, + name: 'test1' + }, + { + id: 2, + name: 'test2' + }, + { + id: 5, + name: 'test5' + } + ] +} +let stateMock = {} +const resetState = () => { + stateMock = cloneDeep(stateFixture) +} + +describe('vuex store mutations', () => { + describe('update resources by id', () => { + beforeEach(resetState) + it.each([ + [mutations.UPSERT_RESOURCE.name, mutations.UPSERT_RESOURCE], + [mutations.UPDATE_RESOURCE.name, mutations.UPDATE_RESOURCE] + ])('succeeds using mutation %s', (name, m) => { + expect(stateMock.files).toEqual(stateFixture.files) + expect(stateMock.files[1].name).toBe('test2') + m(stateMock, { + id: 2, + name: 'test2-updated' + }) + expect(stateMock.files[1].name).toBe('test2-updated') + expect(stateMock.files.length).toBe(stateFixture.files.length) + }) + }) + describe('insert resources', () => { + beforeEach(resetState) + it('succeeds using mutation UPSERT_RESOURCE', () => { + expect(stateMock.files).toEqual(stateFixture.files) + mutations.UPSERT_RESOURCE(stateMock, { + id: 3, + name: 'test3-inserted' + }) + expect(stateMock.files).toEqual([...stateFixture.files, { id: 3, name: 'test3-inserted' }]) + }) + it('is ignored using mutation UPDATE_RESOURCE', () => { + expect(stateMock.files).toEqual(stateFixture.files) + mutations.UPDATE_RESOURCE(stateMock, { + id: 3, + name: 'test3-inserted' + }) + expect(stateMock.files).toEqual(stateFixture.files) + }) + }) +})