From 391e77fa139c2cad3b85c4ad79a8697af6ce415b Mon Sep 17 00:00:00 2001 From: "Grigorii K. Shartsev" Date: Thu, 27 Jul 2023 21:33:24 +0200 Subject: [PATCH 1/2] fix(conversations): fix duplicating messages Signed-off-by: Grigorii K. Shartsev --- src/store/conversationsStore.js | 98 +++++++++++--------- src/utils/__tests__/objectUtils.spec.js | 118 ------------------------ src/utils/objectUtils.js | 61 ------------ 3 files changed, 55 insertions(+), 222 deletions(-) delete mode 100644 src/utils/__tests__/objectUtils.spec.js delete mode 100644 src/utils/objectUtils.js diff --git a/src/store/conversationsStore.js b/src/store/conversationsStore.js index f3bc8b8a567..36e037354e0 100644 --- a/src/store/conversationsStore.js +++ b/src/store/conversationsStore.js @@ -62,7 +62,6 @@ import { startCallRecording, stopCallRecording, } from '../services/recordingService.js' -import { patchObject } from '../utils/objectUtils.js' const DUMMY_CONVERSATION = { token: '', @@ -85,6 +84,21 @@ const DUMMY_CONVERSATION = { isDummyConversation: true, } +/** + * Emit global event for user status update with the status from a 1-1 conversation + * + * @param {object} conversation - a 1-1 conversation + */ +function emitUserStatusUpdated(conversation) { + emit('user_status:status.updated', { + status: conversation.status, + message: conversation.statusMessage, + icon: conversation.statusIcon, + clearAt: conversation.statusClearAt, + userId: conversation.name, + }) +} + const state = { conversations: { }, @@ -118,16 +132,42 @@ const mutations = { }, /** - * Patch a conversation object: - * - add new properties - * - update existing properties recursively - * - delete properties + * Update stored conversation if a new one has changes * * @param {object} state the state * @param {object} conversation the new conversation object */ - patchConversation(state, conversation) { - patchObject(state.conversations[conversation.token], conversation) + updateConversationIfHasChanged(state, conversation) { + const oldConversation = state.conversations[conversation.token] + + // Update 1-1 conversation, if its status was changed + if (conversation.type === CONVERSATION.TYPE.ONE_TO_ONE + && (oldConversation.status !== conversation.status + || oldConversation.statusMessage !== conversation.statusMessage + || oldConversation.statusIcon !== conversation.statusIcon + || oldConversation.statusClearAt !== conversation.statusClearAt + ) + ) { + emitUserStatusUpdated(conversation) + state.conversations[conversation.token] = conversation + return + } + + // Update conversation if lastActivity updated (e.g. new message came up, call state changed) + if (oldConversation.lastActivity !== conversation.lastActivity) { + state.conversations[conversation.token] = conversation + return + } + + // Check if any property were changed (no properties except status-related supposed to be added or deleted) + for (const key of Object.keys(conversation)) { + // "lastMessage" is the only property with non-primitive (object) value and cannot be compared by === + // If "lastMessage" was actually changed, it is already checked by "lastActivity" + if (key !== 'lastMessage' && oldConversation[key] !== conversation[key]) { + state.conversations[conversation.token] = conversation + return + } + } }, /** @@ -209,7 +249,9 @@ const actions = { * @param {object} conversation the conversation; */ addConversation(context, conversation) { - context.dispatch('emitUserStatusUpdatedWhenChanged', conversation) + if (conversation.type === CONVERSATION.TYPE.ONE_TO_ONE) { + emitUserStatusUpdated(conversation) + } context.commit('addConversation', conversation) @@ -242,43 +284,13 @@ const actions = { }, /** - * Patch a conversation object and emit user status changes when needed + * Update conversation in store according to a new conversation object * * @param {object} context store context * @param {object} newConversation the new conversation object */ - patchConversation(context, newConversation) { - context.dispatch('emitUserStatusUpdatedWhenChanged', newConversation) - - // TODO: We can also check if lastActivity >= modifiedSince and skip the update - context.commit('patchConversation', newConversation) - }, - - /** - * Emits an event when the user status changed in a new conversation data - * - * @param {object} context the store context - * @param {object} conversation the new conversation - */ - emitUserStatusUpdatedWhenChanged(context, conversation) { - if (conversation.type === CONVERSATION.TYPE.ONE_TO_ONE && conversation.status) { - const prevStatus = context.state.conversations[conversation.token] - - // Only emit the event when something actually changed - if (!prevStatus - || prevStatus.status !== conversation.status - || prevStatus.statusMessage !== conversation.statusMessage - || prevStatus.statusIcon !== conversation.statusIcon - || prevStatus.statusClearAt !== conversation.statusClearAt) { - emit('user_status:status.updated', { - status: conversation.status, - message: conversation.statusMessage, - icon: conversation.statusIcon, - clearAt: conversation.statusClearAt, - userId: conversation.name, - }) - } - } + updateConversationIfHasChanged(context, newConversation) { + context.commit('updateConversationIfHasChanged', newConversation) }, /** @@ -297,7 +309,7 @@ const actions = { * Patch conversations: * - Add new conversations * - Remove conversations that are not in the new list - * - Patch existing conversations + * - Update existing conversations * * @param {object} context default store context * @param {object} payload the payload @@ -324,7 +336,7 @@ const actions = { if (currentConversations[token] === undefined) { context.dispatch('addConversation', newConversation) } else { - context.dispatch('patchConversation', newConversation) + context.dispatch('updateConversationIfHasChanged', newConversation) } } }, diff --git a/src/utils/__tests__/objectUtils.spec.js b/src/utils/__tests__/objectUtils.spec.js deleted file mode 100644 index 639d3e3790c..00000000000 --- a/src/utils/__tests__/objectUtils.spec.js +++ /dev/null @@ -1,118 +0,0 @@ -/* - * @copyright Copyright (c) 2023 Grigorii Shartsev - * - * @author Grigorii Shartsev - * - * @license AGPL-3.0-or-later - * - * This program is free software: you can redistribute it and/or modify - * it under the terms of the GNU Affero General Public License as - * published by the Free Software Foundation, either version 3 of the - * License, or (at your option) any later version. - * - * This program is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - * GNU Affero General Public License for more details. - * - * You should have received a copy of the GNU Affero General Public License - * along with this program. If not, see . - */ - -import { isObject, patchObject } from '../objectUtils.js' - -describe('objectUtils', () => { - describe('isObject', () => { - it('should return true for plain object', () => { - expect(isObject({})).toBeTruthy() - }) - - it('should return false for null', () => { - expect(isObject(null)).toBeFalsy() - }) - - it('should return false for Array', () => { - expect(isObject([])).toBeFalsy() - }) - }) - - describe('patchObject', () => { - it('should delete removed properties', () => { - const target = { - a: 1, - b: 2, - toRemove1: 3, - toRemove2: 4, - } - const newObject = { - a: 1, - b: 2, - } - patchObject(target, newObject) - expect(target).toEqual({ a: 1, b: 2 }) - }) - - it('should add new properties', () => { - const target = { - a: 1, - b: 2, - } - const newObject = { - a: 1, - b: 2, - newKey1: 3, - newKey2: 4, - } - patchObject(target, newObject) - expect(target).toEqual({ a: 1, b: 2, newKey1: 3, newKey2: 4 }) - }) - - it('should update existing primitive properties', () => { - const target = { - a: 1, - b: 2, - } - const newObject = { - a: 3, - b: 4, - } - patchObject(target, newObject) - expect(target).toEqual({ a: 3, b: 4 }) - }) - - it('should update existing array properties as primitive properties', () => { - const targetSubArray = [1, 2] - const target = { - a: targetSubArray, - } - const newObjectSubArray = [3, 4] - const newObject = { - a: newObjectSubArray, - } - patchObject(target, newObject) - expect(target).toEqual({ a: [3, 4] }) - expect(target.a).toBe(newObjectSubArray) // Re-assigned, not mutated - }) - - it('should update existing object properties recursively', () => { - const targetSubObject = { - c: 2, - d: 3, - } - const target = { - a: 1, - b: targetSubObject, - } - const newObject = { - a: 1, - b: { - c: 4, - d: 5, - }, - } - patchObject(target, newObject) - expect(target).toEqual({ a: 1, b: { c: 4, d: 5 } }) - expect(target.b).toBe(targetSubObject) // Mutated, not re-assigned - }) - }) -}) diff --git a/src/utils/objectUtils.js b/src/utils/objectUtils.js deleted file mode 100644 index 95508ec649d..00000000000 --- a/src/utils/objectUtils.js +++ /dev/null @@ -1,61 +0,0 @@ -/* - * @copyright Copyright (c) 2023 Grigorii Shartsev - * - * @author Grigorii Shartsev - * - * @license AGPL-3.0-or-later - * - * This program is free software: you can redistribute it and/or modify - * it under the terms of the GNU Affero General Public License as - * published by the Free Software Foundation, either version 3 of the - * License, or (at your option) any later version. - * - * This program is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - * GNU Affero General Public License for more details. - * - * You should have received a copy of the GNU Affero General Public License - * along with this program. If not, see . - */ - -import Vue from 'vue' - -/** - * Check if value is a plain object - * - * @param {any} value value to check - * @return {boolean} true if value is a plain object - */ -export const isObject = (value) => value !== null && typeof value === 'object' && !Array.isArray(value) - -/** - * Apply mutations to object based on new object - * - * @param {object} target target object - * @param {object} newObject new object to get changes from - */ -export function patchObject(target, newObject) { - // Delete removed properties - for (const key of Object.keys(target)) { - if (newObject[key] === undefined) { - Vue.delete(target, key) - } - } - - // Add new properties and update existing ones - for (const [key, newValue] of Object.entries(newObject)) { - const oldValue = target[key] - - if (oldValue === undefined) { - // Add new property - Vue.set(target, key, newValue) - } else if (isObject(oldValue) && isObject(newValue)) { - // This property is an object in both - update recursively - patchObject(oldValue, newValue) - } else { - // Update the property - Vue.set(target, key, newValue) - } - } -} From 5d987fa7c9edf5c440919787084ca93969dbcceb Mon Sep 17 00:00:00 2001 From: "Grigorii K. Shartsev" Date: Thu, 27 Jul 2023 22:36:33 +0200 Subject: [PATCH 2/2] test(conversations): add more conversations fetching tests Signed-off-by: Grigorii K. Shartsev --- src/store/conversationsStore.spec.js | 289 +++++++++++++++++++++++---- 1 file changed, 251 insertions(+), 38 deletions(-) diff --git a/src/store/conversationsStore.spec.js b/src/store/conversationsStore.spec.js index 0ba2c6cb7ca..f76d6768165 100644 --- a/src/store/conversationsStore.spec.js +++ b/src/store/conversationsStore.spec.js @@ -3,6 +3,8 @@ import flushPromises from 'flush-promises' import { cloneDeep } from 'lodash' import Vuex from 'vuex' +import { emit } from '@nextcloud/event-bus' + import { CONVERSATION, WEBINAR, @@ -52,6 +54,8 @@ jest.mock('../services/conversationsService', () => ({ setConversationUnread: jest.fn(), })) +jest.mock('@nextcloud/event-bus') + describe('conversationsStore', () => { const testToken = 'XXTOKENXX' const previousLastMessage = { @@ -229,12 +233,12 @@ describe('conversationsStore', () => { { token: 'one_token', attendeeId: 'attendee-id-1', - lastActivity: 1675209600, // 2023-02-01T00:00:00.000Z + lastActivity: Date.parse('2023-02-01T00:00:00.000Z') / 1000, }, { token: 'another_token', attendeeId: 'attendee-id-2', - lastActivity: 1675209600, // 2023-02-01T00:00:00.000Z + lastActivity: Date.parse('2023-01-01T00:00:00.000Z') / 1000, }, ] @@ -254,77 +258,286 @@ describe('conversationsStore', () => { expect(store.getters.conversationsList).toStrictEqual(testConversations) }) - test('fetches all conversations and remove deleted conversations', async () => { - const testConversations = [ + test('fetches all conversations and add new received conversations', async () => { + const oldConversation = { + token: 'tokenOne', + attendeeId: 'attendee-id-1', + lastActivity: Date.parse('2023-01-01T00:00:00.000Z') / 1000, + } + + // Add initial conversations + store.dispatch('addConversation', oldConversation) + + // Fetch new conversation + const newConversation = { + token: 'tokenTwo', + attendeeId: 'attendee-id-2', + lastActivity: Date.parse('2023-02-01T00:00:00.000Z') / 1000, + } + + const response = { + data: { + ocs: { + data: [{ ...oldConversation }, newConversation], + }, + }, + } + + fetchConversations.mockResolvedValue(response) + + await store.dispatch('fetchConversations', { }) + + expect(fetchConversations).toHaveBeenCalledWith({ }) + // conversationsList is actual to the response + expect(store.getters.conversationsList).toEqual([oldConversation, newConversation]) + // Only old conversation with new activity should be actually replaced with new objects + expect(store.state.conversationsStore.conversations[oldConversation.token]).toStrictEqual(oldConversation) + expect(store.state.conversationsStore.conversations[newConversation.token]).toStrictEqual(newConversation) + }) + + test('fetches all conversations and emit user status update for new 1-1 conversations', async () => { + const oldConversations = [ { token: 'one_token', attendeeId: 'attendee-id-1', - lastActivity: 1675209600, // 2023-02-01T00:00:00.000Z + lastActivity: Date.parse('2023-02-01T00:00:00.000Z') / 1000, + type: CONVERSATION.TYPE.ONE_TO_ONE, + status: 'online', + statusIcon: '🎉', + statusMessage: 'I am the test', + statusClearAt: null, }, + ] + store.dispatch('addConversation', oldConversations[0]) + + const newConversations = [{ + ...oldConversations[0], + }, { + token: 'another_token', + attendeeId: 'attendee-id-2', + lastActivity: Date.parse('2023-01-01T00:00:00.000Z') / 1000, + type: CONVERSATION.TYPE.GROUP, + }, { + name: 'bob', + token: 'new_token', + attendeeId: 'attendee-id-3', + lastActivity: Date.parse('2023-02-01T00:00:00.000Z') / 1000, + type: CONVERSATION.TYPE.ONE_TO_ONE, + status: 'online', + statusIcon: '😃', + statusMessage: 'I am the test 2', + statusClearAt: null, + }] + const response = { + data: { + ocs: { + data: newConversations, + }, + }, + } + fetchConversations.mockResolvedValue(response) + emit.mockClear() + await store.dispatch('fetchConversations', { }) + // Only new conversation emits event + expect(emit).toHaveBeenCalledTimes(1) + expect(emit.mock.calls.at(-1)).toEqual([ + 'user_status:status.updated', { - token: 'another_token', + userId: newConversations[2].name, + status: newConversations[2].status, + icon: newConversations[2].statusIcon, + message: newConversations[2].statusMessage, + clearAt: newConversations[2].statusClearAt, + }, + ]) + }) + + test('fetches all conversations and emit user status update for changed statuses of 1-1 conversations', async () => { + const oldConversations = [ + { + token: 'first_token', + name: 'alice', + attendeeId: 'attendee-id-1', + lastActivity: Date.parse('2023-02-01T00:00:00.000Z') / 1000, + type: CONVERSATION.TYPE.ONE_TO_ONE, + status: 'online', + statusIcon: '🎉', + statusMessage: 'I am the test', + statusClearAt: null, + }, + { + token: 'second_token', + name: 'bob', attendeeId: 'attendee-id-2', - lastActivity: 1675209600, // 2023-02-01T00:00:00.000Z + lastActivity: Date.parse('2023-02-01T00:00:00.000Z') / 1000, + type: CONVERSATION.TYPE.ONE_TO_ONE, + status: 'away', + statusIcon: '🙄', + statusMessage: 'I am the test 2', + statusClearAt: null, }, ] - - // add conversation that should be removed - store.dispatch('addConversation', testConversation) - + store.dispatch('addConversation', oldConversations[0]) + store.dispatch('addConversation', oldConversations[1]) + + const newConversations = [{ + // Not changed + ...oldConversations[0], + }, { + // Updated status + ...oldConversations[1], + status: 'online', + statusIcon: '👀', + statusMessage: 'I am the test 3', + statusClearAt: null, + }, { + token: 'another_token', + attendeeId: 'attendee-id-2', + lastActivity: Date.parse('2023-01-01T00:00:00.000Z') / 1000, + type: CONVERSATION.TYPE.GROUP, + }] const response = { data: { ocs: { - data: testConversations, + data: newConversations, }, }, } - fetchConversations.mockResolvedValue(response) + emit.mockClear() + await store.dispatch('fetchConversations', { }) + // Only new conversation emits event + expect(emit).toHaveBeenCalledTimes(1) + expect(emit.mock.calls.at(-1)).toEqual([ + 'user_status:status.updated', + { + userId: newConversations[1].name, + status: newConversations[1].status, + icon: newConversations[1].statusIcon, + message: newConversations[1].statusMessage, + clearAt: newConversations[1].statusClearAt, + }, + ]) + }) - await store.dispatch('fetchConversations', {}) + test('fetches all conversations and re-set conversations with new lastActivity', async () => { + const oldConversations = [ + { + token: 'one_token', + attendeeId: 'attendee-id-1', + lastActivity: Date.parse('2023-02-01T00:00:00.000Z') / 1000, + }, + { + token: 'another_token', + attendeeId: 'attendee-id-2', + lastActivity: Date.parse('2023-01-01T00:00:00.000Z') / 1000, + }, + ] + store.dispatch('addConversation', oldConversations[0]) + store.dispatch('addConversation', oldConversations[1]) + + const newConversations = [{ + ...oldConversations[0], + }, { + ...oldConversations[1], + lastActivity: oldConversations[1].lastActivity + 1000, + }] + const response = { + data: { + ocs: { + data: newConversations, + }, + }, + } + fetchConversations.mockResolvedValue(response) + await store.dispatch('fetchConversations', { }) - expect(fetchConversations).toHaveBeenCalledWith({}) - expect(store.getters.conversationsList).toStrictEqual(testConversations) + // conversationsList is actual to the response + expect(store.getters.conversationsList).toEqual(newConversations) + // Only old conversation with new activity should be actually replaced with new objects + // Not updated + expect(store.state.conversationsStore.conversations[oldConversations[0].token]).toStrictEqual(newConversations[0]) + // Updated because of new lastActivity + expect(store.state.conversationsStore.conversations[oldConversations[1].token]).toStrictEqual(newConversations[1]) }) - test('fetches all conversations and add new conversations', async () => { - const oldConversation = { - token: 'tokenOne', - attendeeId: 'attendee-id-1', - lastActivity: 1672531200, // 2023-01-01T00:00:00.000Z + test('fetches all conversations and re-set conversations when it has any property changed', async () => { + const oldConversations = [ + { + token: 'one_token', + attendeeId: 'attendee-id-1', + unreadMention: false, + lastActivity: Date.parse('2023-02-01T00:00:00.000Z') / 1000, + }, + { + token: 'another_token', + attendeeId: 'attendee-id-2', + unreadMention: false, + lastActivity: Date.parse('2023-01-01T00:00:00.000Z') / 1000, + }, + ] + store.dispatch('addConversation', oldConversations[0]) + store.dispatch('addConversation', oldConversations[1]) + + const newConversations = [{ + ...oldConversations[0], + }, { + ...oldConversations[1], + unreadMention: true, + }] + const response = { + data: { + ocs: { + data: newConversations, + }, + }, } + fetchConversations.mockResolvedValue(response) + await store.dispatch('fetchConversations', { }) - // Add initial conversations - store.dispatch('addConversation', oldConversation) + // conversationsList is actual to the response + expect(store.getters.conversationsList).toEqual(newConversations) + // Only old conversation with new activity should be actually replaced with new objects + // Not updated + expect(store.state.conversationsStore.conversations[oldConversations[0].token]).toStrictEqual(newConversations[0]) + // Updated because unreadMention change + expect(store.state.conversationsStore.conversations[oldConversations[1].token]).toStrictEqual(newConversations[1]) + }) - // Fetch new conversation - const newConversation = { - token: 'tokenTwo', - attendeeId: 'attendee-id-2', - lastActivity: 1675209600, // 2023-02-01T00:00:00.000Z - } + test('fetches all conversations and remove deleted conversations if without modiviedSince', async () => { + const testConversations = [ + { + token: 'one_token', + attendeeId: 'attendee-id-1', + lastActivity: 1675209600, // 2023-02-01T00:00:00.000Z + }, + { + token: 'another_token', + attendeeId: 'attendee-id-2', + lastActivity: 1675209600, // 2023-02-01T00:00:00.000Z + }, + ] + + // add conversation that should be removed + store.dispatch('addConversation', testConversation) const response = { data: { ocs: { - data: [{ ...oldConversation }, newConversation], + data: testConversations, }, }, } fetchConversations.mockResolvedValue(response) - await store.dispatch('fetchConversations', { }) + await store.dispatch('fetchConversations', {}) - expect(fetchConversations).toHaveBeenCalledWith({ }) - // conversationsList is actual to the response - expect(store.getters.conversationsList).toEqual([oldConversation, newConversation]) - // Only old conversation with new activity should be actually replaced with new objects - expect(store.state.conversationsStore.conversations[oldConversation.token]).toStrictEqual(oldConversation) - expect(store.state.conversationsStore.conversations[newConversation.token]).toStrictEqual(newConversation) + expect(fetchConversations).toHaveBeenCalledWith({}) + expect(store.getters.conversationsList).toStrictEqual(testConversations) }) - test('fetches all conversations and update only new without purging when modifiedSince is present', async () => { + test('fetches all conversations without purging not revieved conversations when modifiedSince is present', async () => { const oldConversation1 = { token: 'tokenOne', attendeeId: 'attendee-id-1',