From e393bcf125c529c6730ed9fd147d48cb12802e92 Mon Sep 17 00:00:00 2001 From: Shivam Mishra Date: Wed, 7 Aug 2024 06:43:41 +0530 Subject: [PATCH] fix: Update the logic to handle attachments in a conversation (#9784) When the chat is viewed, a function `fetchAllAttachments` is run to get all attachments for a particular conversation. This function, before updating the record creates the `attachments` property on the `chat` object in the store. If in any case this function fails, the `attachments` property is not created, and when the code reaches the `dashboard/store/modules/conversations/index.js` the error occurs This PR fixes it by ensuring that `SET_ALL_ATTACHMENTS` is always run. And it handles the default case as well --- Sentry Issue: [CHATWOOT-FRONTEND-APP-5Y](https://chatwoot-p3.sentry.io/issues/5459056982/) ``` TypeError: Cannot read properties of undefined (reading 'some') at forEach(./app/javascript/dashboard/store/modules/conversations/index.js:160:31) at Array.forEach() at mutations(./app/javascript/dashboard/store/modules/conversations/index.js:159:27) at handler(./node_modules/vuex/dist/vuex.js:771:7) at forEach(./node_modules/vuex/dist/vuex.js:470:9) at Array.forEach() at fn(./node_modules/vuex/dist/vuex.js:469:13) at Store.prototype._withCommit(./node_modules/vuex/dist/vuex.js:574:5) at Store.prototype.commit(./node_modules/vuex/dist/vuex.js:468:10) at this.commit(./node_modules/vuex/dist/vuex.js:420:21) at call(./app/javascript/dashboard/store/modules/conversations/actions.js:273:7) at tryCatch(./node_modules/videojs-record/dist/videojs.record.js:2868:27) at _invoke(./node_modules/videojs-record/dist/videojs.record.js:3088:32) at prototype[method](./node_modules/videojs-record/dist/videojs.record.js:2921:31) at as(/packs/js/application-cf716bca3c984faeb095.js:4:76) at as(/packs/js/application-cf716bca3c984faeb095.js:4:76) at nrWrapper(/app/accounts/81898/conversations/95:6:17817) ``` --------- Co-authored-by: Sivin Varghese <64252451+iamsivin@users.noreply.github.com> Co-authored-by: Sojan Jose Co-authored-by: Pranav --- .../store/modules/conversations/actions.js | 17 ++++- .../store/modules/conversations/getters.js | 5 +- .../store/modules/conversations/index.js | 67 +++++++++---------- .../specs/conversations/getters.spec.js | 28 +++----- .../specs/conversations/mutations.spec.js | 54 ++++++++------- 5 files changed, 84 insertions(+), 87 deletions(-) diff --git a/app/javascript/dashboard/store/modules/conversations/actions.js b/app/javascript/dashboard/store/modules/conversations/actions.js index 6a8b94e1e..eaf385372 100644 --- a/app/javascript/dashboard/store/modules/conversations/actions.js +++ b/app/javascript/dashboard/store/modules/conversations/actions.js @@ -12,6 +12,7 @@ import { } from './helpers/actionHelpers'; import messageReadActions from './actions/messageReadActions'; import messageTranslateActions from './actions/messageTranslateActions'; +import * as Sentry from '@sentry/browser'; export const hasMessageFailedWithExternalError = pendingMessage => { // This helper is used to check if the message has failed with an external error. @@ -100,14 +101,24 @@ const actions = { }, fetchAllAttachments: async ({ commit }, conversationId) => { + let attachments = null; + try { const { data } = await ConversationApi.getAllAttachments(conversationId); + attachments = data.payload; + } catch (error) { + // in case of error, log the error and continue + Sentry.setContext('Conversation', { + id: conversationId, + }); + Sentry.captureException(error); + } finally { + // we run the commit even if the request fails + // this ensures that the `attachment` variable is always present on chat commit(types.SET_ALL_ATTACHMENTS, { id: conversationId, - data: data.payload, + data: attachments, }); - } catch (error) { - // Handle error } }, diff --git a/app/javascript/dashboard/store/modules/conversations/getters.js b/app/javascript/dashboard/store/modules/conversations/getters.js index 8c2070963..1a905c3c5 100644 --- a/app/javascript/dashboard/store/modules/conversations/getters.js +++ b/app/javascript/dashboard/store/modules/conversations/getters.js @@ -18,9 +18,8 @@ const getters = { ); return selectedChat || {}; }, - getSelectedChatAttachments: (_state, _getters) => { - const selectedChat = _getters.getSelectedChat; - return selectedChat.attachments || []; + getSelectedChatAttachments: ({ selectedChatId, attachments }) => { + return attachments[selectedChatId] || []; }, getChatListFilters: ({ conversationFilters }) => conversationFilters, getLastEmailInSelectedChat: (stage, _getters) => { diff --git a/app/javascript/dashboard/store/modules/conversations/index.js b/app/javascript/dashboard/store/modules/conversations/index.js index 769354ed2..94c125494 100644 --- a/app/javascript/dashboard/store/modules/conversations/index.js +++ b/app/javascript/dashboard/store/modules/conversations/index.js @@ -10,6 +10,7 @@ import { emitter } from 'shared/helpers/mitt'; const state = { allConversations: [], + attachments: {}, listLoadingStatus: true, chatStatusFilter: wootConstants.STATUS_TYPE.OPEN, chatSortFilter: wootConstants.SORT_BY_TYPE.LATEST, @@ -48,7 +49,6 @@ export const mutations = { allMessagesLoaded: existingConversation.allMessagesLoaded, messages: existingConversation.messages, dataFetched: existingConversation.dataFetched, - attachments: existingConversation.attachments, }; } }); @@ -78,10 +78,10 @@ export const mutations = { } }, [types.SET_ALL_ATTACHMENTS](_state, { id, data }) { - const [chat] = _state.allConversations.filter(c => c.id === id); - if (!chat) return; - Vue.set(chat, 'attachments', []); - chat.attachments.push(...data); + const attachments = _state.attachments[id] || []; + + attachments.push(...data); + _state.attachments[id] = [...attachments]; }, [types.SET_MISSING_MESSAGES](_state, { id, data }) { const [chat] = _state.allConversations.filter(c => c.id === id); @@ -144,42 +144,37 @@ export const mutations = { Vue.set(chat, 'muted', false); }, - [types.ADD_CONVERSATION_ATTACHMENTS]({ allConversations }, message) { - const { conversation_id: conversationId } = message; - const [chat] = getSelectedChatConversation({ - allConversations, - selectedChatId: conversationId, + [types.ADD_CONVERSATION_ATTACHMENTS](_state, message) { + // early return if the message has not been sent, or has no attachments + if (message.status !== MESSAGE_STATUS.SENT || !message.attachments.length) { + return; + } + + const id = message.conversation_id; + const existingAttachments = _state.attachments[id] || []; + + const attachmentsToAdd = message.attachments.filter(attachment => { + // if the attachment is not already in the store, add it + // this is to prevent duplicates + return !existingAttachments.some( + existingAttachment => existingAttachment.id === attachment.id + ); }); - if (!chat) return; - - const isMessageSent = - message.status === MESSAGE_STATUS.SENT && message.attachments; - if (isMessageSent) { - message.attachments.forEach(attachment => { - if (!chat.attachments.some(a => a.id === attachment.id)) { - chat.attachments.push(attachment); - } - }); - } + // replace the attachments in the store + _state.attachments[id] = [...existingAttachments, ...attachmentsToAdd]; }, - [types.DELETE_CONVERSATION_ATTACHMENTS]({ allConversations }, message) { - const { conversation_id: conversationId } = message; - const [chat] = getSelectedChatConversation({ - allConversations, - selectedChatId: conversationId, + [types.DELETE_CONVERSATION_ATTACHMENTS](_state, message) { + if (message.status !== MESSAGE_STATUS.SENT) return; + + const { conversation_id: id } = message; + const existingAttachments = _state.attachments[id] || []; + if (!existingAttachments.length) return; + + _state.attachments[id] = existingAttachments.filter(attachment => { + return attachment.message_id !== message.id; }); - - if (!chat) return; - - const isMessageSent = message.status === MESSAGE_STATUS.SENT; - if (isMessageSent) { - const attachmentIndex = chat.attachments.findIndex( - a => a.message_id === message.id - ); - if (attachmentIndex !== -1) chat.attachments.splice(attachmentIndex, 1); - } }, [types.ADD_MESSAGE]({ allConversations, selectedChatId }, message) { diff --git a/app/javascript/dashboard/store/modules/specs/conversations/getters.spec.js b/app/javascript/dashboard/store/modules/specs/conversations/getters.spec.js index 3c3bf79de..cded29329 100644 --- a/app/javascript/dashboard/store/modules/specs/conversations/getters.spec.js +++ b/app/javascript/dashboard/store/modules/specs/conversations/getters.spec.js @@ -245,30 +245,18 @@ describe('#getters', () => { describe('#getSelectedChatAttachments', () => { it('Returns attachments in selected chat', () => { - const state = {}; - const getSelectedChat = { - attachments: [ - { - id: 1, - file_name: 'test1', - }, - { - id: 2, - file_name: 'test2', - }, + const attachments = { + 1: [ + { id: 1, file_name: 'test1' }, + { id: 2, file_name: 'test2' }, ], }; + const selectedChatId = 1; expect( - getters.getSelectedChatAttachments(state, { getSelectedChat }) + getters.getSelectedChatAttachments({ selectedChatId, attachments }) ).toEqual([ - { - id: 1, - file_name: 'test1', - }, - { - id: 2, - file_name: 'test2', - }, + { id: 1, file_name: 'test1' }, + { id: 2, file_name: 'test2' }, ]); }); }); diff --git a/app/javascript/dashboard/store/modules/specs/conversations/mutations.spec.js b/app/javascript/dashboard/store/modules/specs/conversations/mutations.spec.js index de596534c..6340e4de9 100644 --- a/app/javascript/dashboard/store/modules/specs/conversations/mutations.spec.js +++ b/app/javascript/dashboard/store/modules/specs/conversations/mutations.spec.js @@ -313,7 +313,6 @@ describe('#mutations', () => { { id: 1, messages: [{ id: 1, content: 'test' }], - attachments: [{ id: 1, name: 'test1.png' }], dataFetched: true, allMessagesLoaded: true, }, @@ -325,7 +324,6 @@ describe('#mutations', () => { id: 1, name: 'test', messages: [{ id: 1, content: 'updated message' }], - attachments: [{ id: 1, name: 'test.png' }], dataFetched: true, allMessagesLoaded: true, }, @@ -335,7 +333,6 @@ describe('#mutations', () => { id: 1, name: 'test', messages: [{ id: 1, content: 'test' }], - attachments: [{ id: 1, name: 'test1.png' }], dataFetched: true, allMessagesLoaded: true, }, @@ -371,25 +368,28 @@ describe('#mutations', () => { it('set all attachments', () => { const state = { allConversations: [{ id: 1 }], + attachments: {}, }; const data = [{ id: 1, name: 'test' }]; mutations[types.SET_ALL_ATTACHMENTS](state, { id: 1, data }); - expect(state.allConversations[0].attachments).toEqual(data); + expect(state.attachments[1]).toEqual(data); }); it('set attachments key even if the attachments are empty', () => { const state = { allConversations: [{ id: 1 }], + attachments: {}, }; const data = []; mutations[types.SET_ALL_ATTACHMENTS](state, { id: 1, data }); - expect(state.allConversations[0].attachments).toEqual([]); + expect(state.attachments[1]).toEqual([]); }); }); describe('#ADD_CONVERSATION_ATTACHMENTS', () => { it('add conversation attachments', () => { const state = { - allConversations: [{ id: 1, attachments: [] }], + allConversations: [{ id: 1 }], + attachments: {}, }; const message = { conversation_id: 1, @@ -398,19 +398,13 @@ describe('#mutations', () => { }; mutations[types.ADD_CONVERSATION_ATTACHMENTS](state, message); - expect(state.allConversations[0].attachments).toEqual( - message.attachments - ); + expect(state.attachments[1]).toEqual(message.attachments); }); it('should not add duplicate attachments', () => { const state = { - allConversations: [ - { - id: 1, - attachments: [{ id: 1, name: 'existing' }], - }, - ], + allConversations: [{ id: 1 }], + attachments: { 1: [{ id: 1, name: 'existing' }] }, }; const message = { conversation_id: 1, @@ -422,12 +416,12 @@ describe('#mutations', () => { }; mutations[types.ADD_CONVERSATION_ATTACHMENTS](state, message); - expect(state.allConversations[0].attachments).toHaveLength(2); - expect(state.allConversations[0].attachments).toContainEqual({ + expect(state.attachments[1]).toHaveLength(2); + expect(state.attachments[1]).toContainEqual({ id: 1, name: 'existing', }); - expect(state.allConversations[0].attachments).toContainEqual({ + expect(state.attachments[1]).toContainEqual({ id: 2, name: 'new', }); @@ -436,6 +430,9 @@ describe('#mutations', () => { it('should not add attachments if chat not found', () => { const state = { allConversations: [{ id: 1, attachments: [] }], + attachments: { + 1: [], + }, }; const message = { conversation_id: 2, @@ -444,14 +441,17 @@ describe('#mutations', () => { }; mutations[types.ADD_CONVERSATION_ATTACHMENTS](state, message); - expect(state.allConversations[0].attachments).toHaveLength(0); + expect(state.attachments[1]).toHaveLength(0); }); }); describe('#DELETE_CONVERSATION_ATTACHMENTS', () => { it('delete conversation attachments', () => { const state = { - allConversations: [{ id: 1, attachments: [{ id: 1, message_id: 1 }] }], + allConversations: [{ id: 1 }], + attachments: { + 1: [{ id: 1, message_id: 1 }], + }, }; const message = { conversation_id: 1, @@ -460,12 +460,15 @@ describe('#mutations', () => { }; mutations[types.DELETE_CONVERSATION_ATTACHMENTS](state, message); - expect(state.allConversations[0].attachments).toHaveLength(0); + expect(state.attachments[1]).toHaveLength(0); }); it('should not delete attachments for non-matching message id', () => { const state = { - allConversations: [{ id: 1, attachments: [{ id: 1, message_id: 1 }] }], + allConversations: [{ id: 1 }], + attachments: { + 1: [{ id: 1, message_id: 1 }], + }, }; const message = { conversation_id: 1, @@ -474,12 +477,13 @@ describe('#mutations', () => { }; mutations[types.DELETE_CONVERSATION_ATTACHMENTS](state, message); - expect(state.allConversations[0].attachments).toHaveLength(1); + expect(state.attachments[1]).toHaveLength(1); }); it('should not delete attachments if chat not found', () => { const state = { - allConversations: [{ id: 1, attachments: [{ id: 1, message_id: 1 }] }], + allConversations: [{ id: 1 }], + attachments: { 1: [{ id: 1, message_id: 1 }] }, }; const message = { conversation_id: 2, @@ -488,7 +492,7 @@ describe('#mutations', () => { }; mutations[types.DELETE_CONVERSATION_ATTACHMENTS](state, message); - expect(state.allConversations[0].attachments).toHaveLength(1); + expect(state.attachments[1]).toHaveLength(1); }); });