From 4889cb5b323977df64748ff020d0f9a3daec1246 Mon Sep 17 00:00:00 2001 From: William Grant Date: Thu, 25 Aug 2022 13:33:49 +1000 Subject: [PATCH] fix: ReactionList Senders is now an array since we want to handle opengroup reactions separately we no longer need the messagehash and server id for rendering reactions in the UI, ignore reactions using the outdated type --- .../message/reactions/Reaction.tsx | 4 +- ts/components/dialog/ReactListModal.tsx | 4 +- ts/models/conversation.ts | 15 --- ts/receiver/dataMessage.ts | 11 +- ts/receiver/queuedJob.ts | 3 - ts/state/selectors/conversations.ts | 10 ++ .../unit/reactions/ReactionMessage_test.ts | 10 +- ts/types/Reaction.ts | 4 +- ts/util/reactions.ts | 107 +++++++++--------- 9 files changed, 76 insertions(+), 92 deletions(-) diff --git a/ts/components/conversation/message/reactions/Reaction.tsx b/ts/components/conversation/message/reactions/Reaction.tsx index d0d6af1a7..675d24afd 100644 --- a/ts/components/conversation/message/reactions/Reaction.tsx +++ b/ts/components/conversation/message/reactions/Reaction.tsx @@ -69,7 +69,7 @@ export const Reaction = (props: ReactionProps): ReactElement => { handlePopupClick, } = props; const reactionsMap = (reactions && Object.fromEntries(reactions)) || {}; - const senders = reactionsMap[emoji].senders ? Object.keys(reactionsMap[emoji].senders) : []; + const senders = reactionsMap[emoji].senders || []; const count = reactionsMap[emoji].count; const showCount = count !== undefined && (count > 1 || inGroup); @@ -138,7 +138,7 @@ export const Reaction = (props: ReactionProps): ReactElement => { { if (handlePopupReaction) { diff --git a/ts/components/dialog/ReactListModal.tsx b/ts/components/dialog/ReactListModal.tsx index debc9fc70..fc1787297 100644 --- a/ts/components/dialog/ReactListModal.tsx +++ b/ts/components/dialog/ReactListModal.tsx @@ -165,7 +165,7 @@ type Props = { }; const handleSenders = (senders: Array, me: string) => { - let updatedSenders = senders; + let updatedSenders = [...senders]; const blindedMe = updatedSenders.filter(isUsAnySogsFromCache); let meIndex = -1; @@ -217,7 +217,7 @@ export const ReactListModal = (props: Props): ReactElement => { let _senders = reactionsMap && reactionsMap[currentReact] && reactionsMap[currentReact].senders - ? Object.keys(reactionsMap[currentReact].senders) + ? reactionsMap[currentReact].senders : null; if (_senders && !isEqual(senders, _senders)) { diff --git a/ts/models/conversation.ts b/ts/models/conversation.ts index bc14d2ccf..3c1aeaa7e 100644 --- a/ts/models/conversation.ts +++ b/ts/models/conversation.ts @@ -89,12 +89,10 @@ import { addMessagePadding } from '../session/crypto/BufferPadding'; import { getSodiumRenderer } from '../session/crypto'; import { findCachedOurBlindedPubkeyOrLookItUp, - getUsBlindedInThatServer, isUsAnySogsFromCache, } from '../session/apis/open_group_api/sogsv3/knownBlindedkeys'; import { sogsV3FetchPreviewAndSaveIt } from '../session/apis/open_group_api/sogsv3/sogsV3FetchFile'; import { Reaction } from '../types/Reaction'; -import { handleMessageReaction } from '../util/reactions'; export class ConversationModel extends Backbone.Model { public updateLastMessage: () => any; @@ -738,8 +736,6 @@ export class ConversationModel extends Backbone.Model { throw new Error('Only opengroupv2 are supported now'); } - let sender = UserUtils.getOurPubKeyStrFromCache(); - // an OpenGroupV2 message is just a visible message const chatMessageParams: VisibleMessageParams = { body: '', @@ -785,20 +781,9 @@ export class ConversationModel extends Backbone.Model { const openGroup = OpenGroupData.getV2OpenGroupRoom(this.id); const blinded = Boolean(roomHasBlindEnabled(openGroup)); - if (blinded) { - const blindedSender = getUsBlindedInThatServer(this); - if (blindedSender) { - sender = blindedSender; - } - } - - await handleMessageReaction(reaction, sender, true); - // send with blinding if we need to await getMessageQueue().sendToOpenGroupV2(chatMessageOpenGroupV2, roomInfos, blinded, []); return; - } else { - await handleMessageReaction(reaction, sender, false); } const destinationPubkey = new PubKey(destination); diff --git a/ts/receiver/dataMessage.ts b/ts/receiver/dataMessage.ts index e2c8d09a4..362639f40 100644 --- a/ts/receiver/dataMessage.ts +++ b/ts/receiver/dataMessage.ts @@ -318,17 +318,12 @@ async function handleSwarmMessage( void convoToAddMessageTo.queueJob(async () => { // this call has to be made inside the queueJob! - if (!msgModel.get('isPublic') && rawDataMessage.reaction && rawDataMessage.syncTarget) { - await handleMessageReaction( - rawDataMessage.reaction, - msgModel.get('source'), - false, - messageHash - ); + // We handle reaction DataMessages separately + if (!msgModel.get('isPublic') && rawDataMessage.reaction) { + await handleMessageReaction(rawDataMessage.reaction, msgModel.get('source')); confirm(); return; } - const isDuplicate = await isSwarmMessageDuplicate({ source: msgModel.get('source'), sentAt, diff --git a/ts/receiver/queuedJob.ts b/ts/receiver/queuedJob.ts index 443ce6789..03b4a67dc 100644 --- a/ts/receiver/queuedJob.ts +++ b/ts/receiver/queuedJob.ts @@ -16,7 +16,6 @@ import { GoogleChrome } from '../util'; import { appendFetchAvatarAndProfileJob } from './userProfileImageUpdates'; import { ConversationTypeEnum } from '../models/conversationAttributes'; import { getUsBlindedInThatServer } from '../session/apis/open_group_api/sogsv3/knownBlindedkeys'; -import { handleMessageReaction } from '../util/reactions'; import { Action, Reaction } from '../types/Reaction'; function contentTypeSupported(type: string): boolean { @@ -341,8 +340,6 @@ export async function handleMessageJob( ); if (!messageModel.get('isPublic') && regularDataMessage.reaction) { - await handleMessageReaction(regularDataMessage.reaction, source, false, messageHash); - if ( regularDataMessage.reaction.action === Action.REACT && conversation.isPrivate() && diff --git a/ts/state/selectors/conversations.ts b/ts/state/selectors/conversations.ts index e3f0d8081..91e87d3dc 100644 --- a/ts/state/selectors/conversations.ts +++ b/ts/state/selectors/conversations.ts @@ -918,6 +918,16 @@ export const getMessageReactsProps = createSelector(getMessagePropsByMessageId, ]); if (msgProps.reacts) { + // TODO we don't want to render reactions that have 'senders' as an object this is a deprecated type used during development 25/08/2022 + const oldReactions = Object.values(msgProps.reacts).filter( + reaction => !Array.isArray(reaction.senders) + ); + + if (oldReactions.length > 0) { + msgProps.reacts = undefined; + return msgProps; + } + const sortedReacts = Object.entries(msgProps.reacts).sort((a, b) => { return a[1].index < b[1].index ? -1 : a[1].index > b[1].index ? 1 : 0; }); diff --git a/ts/test/session/unit/reactions/ReactionMessage_test.ts b/ts/test/session/unit/reactions/ReactionMessage_test.ts index cf5a1150d..dfd2fd1aa 100644 --- a/ts/test/session/unit/reactions/ReactionMessage_test.ts +++ b/ts/test/session/unit/reactions/ReactionMessage_test.ts @@ -54,9 +54,7 @@ describe('ReactionMessage', () => { // Handling reaction const updatedMessage = await handleMessageReaction( reaction as SignalService.DataMessage.IReaction, - ourNumber, - false, - originalMessage.get('id') + ourNumber ); expect(updatedMessage?.get('reacts'), 'original message should have reacts').to.not.be @@ -65,7 +63,7 @@ describe('ReactionMessage', () => { expect(updatedMessage?.get('reacts')!['😄'], 'reacts should have 😄 key').to.not.be.undefined; // tslint:disable: no-non-null-assertion expect( - Object.keys(updatedMessage!.get('reacts')!['😄'].senders)[0], + updatedMessage!.get('reacts')!['😄'].senders[0], 'sender pubkey should match' ).to.be.equal(ourNumber); expect(updatedMessage!.get('reacts')!['😄'].count, 'count should be 1').to.be.equal(1); @@ -87,9 +85,7 @@ describe('ReactionMessage', () => { // Handling reaction const updatedMessage = await handleMessageReaction( reaction as SignalService.DataMessage.IReaction, - ourNumber, - false, - originalMessage.get('id') + ourNumber ); expect(updatedMessage?.get('reacts'), 'original message reacts should be undefined').to.be diff --git a/ts/types/Reaction.ts b/ts/types/Reaction.ts index 22dbad543..617999541 100644 --- a/ts/types/Reaction.ts +++ b/ts/types/Reaction.ts @@ -123,14 +123,14 @@ export type ReactionList = Record< { count: number; index: number; // relies on reactsIndex in the message model - senders: Record; // + senders: Array; you?: boolean; // whether you are in the senders because sometimes we dont have the full list of senders yet. } >; // used when rendering reactions to guarantee sorted order using the index export type SortedReactionList = Array< - [string, { count: number; index: number; senders: Record; you?: boolean }] + [string, { count: number; index: number; senders: Array; you?: boolean }] >; export interface OpenGroupReaction { diff --git a/ts/util/reactions.ts b/ts/util/reactions.ts index f558af080..c3c0d59cd 100644 --- a/ts/util/reactions.ts +++ b/ts/util/reactions.ts @@ -19,28 +19,23 @@ const latestReactionTimestamps: Array = []; * Retrieves the original message of a reaction */ const getMessageByReaction = async ( - reaction: SignalService.DataMessage.IReaction, - isOpenGroup: boolean + reaction: SignalService.DataMessage.IReaction ): Promise => { let originalMessage = null; const originalMessageId = Number(reaction.id); const originalMessageAuthor = reaction.author; - if (isOpenGroup) { - originalMessage = await Data.getMessageByServerId(originalMessageId); - } else { - const collection = await Data.getMessagesBySentAt(originalMessageId); - originalMessage = collection.find((item: MessageModel) => { - const messageTimestamp = item.get('sent_at'); - const author = item.get('source'); - return Boolean( - messageTimestamp && - messageTimestamp === originalMessageId && - author && - author === originalMessageAuthor - ); - }); - } + const collection = await Data.getMessagesBySentAt(originalMessageId); + originalMessage = collection.find((item: MessageModel) => { + const messageTimestamp = item.get('sent_at'); + const author = item.get('source'); + return Boolean( + messageTimestamp && + messageTimestamp === originalMessageId && + author && + author === originalMessageAuthor + ); + }); if (!originalMessage) { window?.log?.warn(`Cannot find the original reacted message ${originalMessageId}.`); @@ -67,6 +62,7 @@ export const sendMessageReaction = async (messageId: string, emoji: string) => { return; } + // TODO need to add rate limiting to SOGS function const timestamp = Date.now(); latestReactionTimestamps.push(timestamp); @@ -80,21 +76,19 @@ export const sendMessageReaction = async (messageId: string, emoji: string) => { } } - const isOpenGroup = Boolean(found?.get('isPublic')); - const id = (isOpenGroup && found.get('serverId')) || Number(found.get('sent_at')); - const me = - (isOpenGroup && getUsBlindedInThatServer(conversationModel)) || - UserUtils.getOurPubKeyStrFromCache(); + if (found?.get('isPublic')) { + window.log.warn("sendMessageReaction() shouldn't be used in opengroups"); + return; + } + + const id = Number(found.get('sent_at')); + const me = UserUtils.getOurPubKeyStrFromCache(); const author = found.get('source'); let action: Action = Action.REACT; const reacts = found.get('reacts'); - if ( - reacts && - Object.keys(reacts).includes(emoji) && - Object.keys(reacts[emoji].senders).includes(me) - ) { - window.log.info('found matching reaction removing it'); + if (reacts && Object.keys(reacts).includes(emoji) && reacts[emoji].senders.includes(me)) { + window.log.info('Found matching reaction removing it'); action = Action.REMOVE; } else { const reactions = getRecentReactions(); @@ -127,27 +121,32 @@ export const sendMessageReaction = async (messageId: string, emoji: string) => { /** * Handle reactions on the client by updating the state of the source message + * Do not use for Open Groups */ export const handleMessageReaction = async ( reaction: SignalService.DataMessage.IReaction, - sender: string, - isOpenGroup: boolean, - messageId?: string + sender: string ) => { if (!reaction.emoji) { - window?.log?.warn(`There is no emoji for the reaction ${messageId}.`); + window?.log?.warn(`There is no emoji for the reaction ${reaction}.`); return; } - const originalMessage = await getMessageByReaction(reaction, isOpenGroup); + const originalMessage = await getMessageByReaction(reaction); if (!originalMessage) { return; } + if (originalMessage.get('isPublic')) { + window.log.warn("handleMessageReaction() shouldn't be used in opengroups"); + return; + } + const reacts: ReactionList = originalMessage.get('reacts') ?? {}; - reacts[reaction.emoji] = reacts[reaction.emoji] || { count: null, senders: {} }; + reacts[reaction.emoji] = reacts[reaction.emoji] || { count: null, senders: [] }; const details = reacts[reaction.emoji] ?? {}; - const senders = Object.keys(details.senders); + const senders = details.senders; + let count = details.count || 0; window.log.info( `${sender} ${reaction.action === Action.REACT ? 'added' : 'removed'} a ${ @@ -156,33 +155,30 @@ export const handleMessageReaction = async ( ); switch (reaction.action) { - case SignalService.DataMessage.Reaction.Action.REACT: - if (senders.includes(sender) && details.senders[sender] !== '') { - window?.log?.info( - 'Received duplicate message reaction. Dropping it. id:', - details.senders[sender] - ); + case Action.REACT: + if (senders.includes(sender)) { + window.log.warn('Received duplicate reaction message. Ignoring it', reaction, sender); return; } - details.senders[sender] = messageId ?? ''; + details.senders.push(sender); + count += 1; break; - case SignalService.DataMessage.Reaction.Action.REMOVE: + case Action.REMOVE: default: - if (senders.length > 0) { - if (senders.indexOf(sender) >= 0) { - // tslint:disable-next-line: no-dynamic-delete - delete details.senders[sender]; + if (senders && senders.length > 0) { + const sendersIndex = senders.indexOf(sender); + if (sendersIndex >= 0) { + details.senders.splice(sendersIndex, 1); + count -= 1; } } } - const count = Object.keys(details.senders).length; if (count > 0) { reacts[reaction.emoji].count = count; reacts[reaction.emoji].senders = details.senders; - // sorting for open groups convos is handled by SOGS - if (!isOpenGroup && details && details.index === undefined) { + if (details && details.index === undefined) { reacts[reaction.emoji].index = originalMessage.get('reactsIndex') ?? 0; originalMessage.set('reactsIndex', (originalMessage.get('reactsIndex') ?? 0) + 1); } @@ -200,7 +196,7 @@ export const handleMessageReaction = async ( }; /** - * Handle all updates to messages reactions from the SOGS API + * Handles all message reaction updates for opengroups */ export const handleOpenGroupMessageReactions = async ( reactions: OpenGroupReactionList, @@ -212,6 +208,11 @@ export const handleOpenGroupMessageReactions = async ( return; } + if (!originalMessage.get('isPublic')) { + window.log.warn('handleOpenGroupMessageReactions() should only be used in opengroups'); + return; + } + if (isEmpty(reactions)) { if (originalMessage.get('reacts')) { originalMessage.set({ @@ -239,9 +240,9 @@ export const handleOpenGroupMessageReactions = async ( } } - const senders: Record = {}; + const senders: Array = []; reactions[key].reactors.forEach(reactor => { - senders[reactor] = String(serverId); + senders.push(reactor); }); reacts[emoji] = {