From e409c7ca82583490b5153aa0f7bc59a3bcb23d74 Mon Sep 17 00:00:00 2001 From: William Grant Date: Fri, 26 Aug 2022 16:51:48 +1000 Subject: [PATCH 01/11] feat: created a cache for opengroups reactions, we can add and update them --- .../sogsv3/sogsV3MutationCache.ts | 68 +++++++++++++++++++ .../sogsv3/sogsV3SendReaction.ts | 26 +++++++ ts/types/Reaction.ts | 1 + 3 files changed, 95 insertions(+) create mode 100644 ts/session/apis/open_group_api/sogsv3/sogsV3MutationCache.ts diff --git a/ts/session/apis/open_group_api/sogsv3/sogsV3MutationCache.ts b/ts/session/apis/open_group_api/sogsv3/sogsV3MutationCache.ts new file mode 100644 index 000000000..775636108 --- /dev/null +++ b/ts/session/apis/open_group_api/sogsv3/sogsV3MutationCache.ts @@ -0,0 +1,68 @@ +/** + * This is strictly use to resolve conflicts between local state and the opengroup poll updates + * Currently only supports message reactions 26/08/2022 + */ + +import { findIndex } from 'lodash'; + +export enum ChangeType { + REACTIONS = 0, +} + +type ReactionAction = 'ADD' | 'REMOVE'; + +type ReactionChange = { + messageId: number; // will be serverId of the reacted message + emoji: string; + action: ReactionAction; +}; + +export type SogsV3Mutation = { + seqno: number | null; // null until mutating API request returns + server: string; // server address + room: string; // room name + changeType: ChangeType; + metadata: ReactionChange; // For now we only support message reactions +}; + +// we don't want to export this, we want to export functions that manipulate it +const sogsMutationCache: Array = []; + +function verifyEntry(entry: SogsV3Mutation): boolean { + return Boolean( + !entry.server || + !entry.room || + entry.seqno !== null || + entry.metadata.messageId || + entry.metadata.emoji || + entry.metadata.action === 'ADD' || + entry.metadata.action === 'REMOVE' + ); +} + +export function addToMutationCache(entry: SogsV3Mutation) { + if (!verifyEntry(entry)) { + window.log.error('SOGS Mutation Cache: Entry verification failed!'); + } else { + sogsMutationCache.push(entry); + window.log.info('SOGS Mutation Cache: Entry added!', entry); + } +} + +export function updateMutationCache(entry: SogsV3Mutation) { + if (!verifyEntry(entry)) { + window.log.error('SOGS Mutation Cache: Entry verification failed!'); + } else { + const entryIndex = findIndex(sogsMutationCache, entry); + if (entryIndex >= 0) { + sogsMutationCache[entryIndex] = entry; + window.log.info('SOGS Mutation Cache: Entry updated!', entry); + } else { + window.log.error('SOGS Mutation Cache: Updated failed! Cannot find entry'); + } + } +} + +export function removeFromMutationCache() { + // TODO +} diff --git a/ts/session/apis/open_group_api/sogsv3/sogsV3SendReaction.ts b/ts/session/apis/open_group_api/sogsv3/sogsV3SendReaction.ts index 01cae53fe..3796f9cdc 100644 --- a/ts/session/apis/open_group_api/sogsv3/sogsV3SendReaction.ts +++ b/ts/session/apis/open_group_api/sogsv3/sogsV3SendReaction.ts @@ -6,6 +6,12 @@ import { hitRateLimit } from '../../../../util/reactions'; import { OnionSending } from '../../../onions/onionSend'; import { OpenGroupPollingUtils } from '../opengroupV2/OpenGroupPollingUtils'; import { batchGlobalIsSuccess, parseBatchGlobalStatusCode } from './sogsV3BatchPoll'; +import { + addToMutationCache, + ChangeType, + SogsV3Mutation, + updateMutationCache, +} from './sogsV3MutationCache'; export const hasReactionSupport = async (serverId: number): Promise => { const found = await Data.getMessageByServerId(serverId); @@ -57,6 +63,20 @@ export const sendSogsReactionOnionV4 = async ( const method = reaction.action === Action.REACT ? 'PUT' : 'DELETE'; const serverPubkey = allValidRoomInfos[0].serverPublicKey; + const cacheEntry: SogsV3Mutation = { + server: serverUrl, + room: room, + changeType: ChangeType.REACTIONS, + seqno: null, + metadata: { + messageId: reaction.id, + emoji, + action: reaction.action === Action.REACT ? 'ADD' : 'REMOVE', + }, + }; + + addToMutationCache(cacheEntry); + // reaction endpoint requires an empty dict {} const stringifiedBody = null; const result = await OnionSending.sendJsonViaOnionV4ToSogs({ @@ -93,5 +113,11 @@ export const sendSogsReactionOnionV4 = async ( `reaction on ${serverUrl}/${room}` ); const success = Boolean(reaction.action === Action.REACT ? rawMessage.added : rawMessage.removed); + + if (success && rawMessage.seqno) { + cacheEntry.seqno = rawMessage.seqno; + updateMutationCache(cacheEntry); + } + return success; }; diff --git a/ts/types/Reaction.ts b/ts/types/Reaction.ts index d9788743e..cf1b26c75 100644 --- a/ts/types/Reaction.ts +++ b/ts/types/Reaction.ts @@ -145,4 +145,5 @@ export type OpenGroupReactionList = Record; export interface OpenGroupReactionResponse { added?: boolean; removed?: boolean; + seqno: number; } From 469de252cb5b61772bf59fb0de8bf85c8153a6be Mon Sep 17 00:00:00 2001 From: William Grant Date: Mon, 29 Aug 2022 13:38:05 +1000 Subject: [PATCH 02/11] fix: added current emoji to reaction list reactor description --- _locales/en/messages.json | 4 ++-- ts/components/dialog/ReactListModal.tsx | 31 ++++++++++++++++++------- 2 files changed, 24 insertions(+), 11 deletions(-) diff --git a/_locales/en/messages.json b/_locales/en/messages.json index c23feed5e..3f0173299 100644 --- a/_locales/en/messages.json +++ b/_locales/en/messages.json @@ -463,6 +463,6 @@ "reactionPopupTwo": "$name$ & $name2$", "reactionPopupThree": "$name$, $name2$ & $name3$", "reactionPopupMany": "$name$, $name2$, $name3$ &", - "reactionListCountSingular": "And $otherSingular$ has reacted to this message", - "reactionListCountPlural": "And $otherPlural$ have reacted to this message" + "reactionListCountSingular": "And $otherSingular$ has reacted $emoji$ to this message", + "reactionListCountPlural": "And $otherPlural$ have reacted $emoji$ to this message" } diff --git a/ts/components/dialog/ReactListModal.tsx b/ts/components/dialog/ReactListModal.tsx index 1d73740e4..3fb5bdccc 100644 --- a/ts/components/dialog/ReactListModal.tsx +++ b/ts/components/dialog/ReactListModal.tsx @@ -17,6 +17,7 @@ import { nativeEmojiData } from '../../util/emoji'; import { sendMessageReaction, SOGSReactorsFetchCount } from '../../util/reactions'; import { Avatar, AvatarSize } from '../avatar/Avatar'; import { Flex } from '../basic/Flex'; +import { SessionHtmlRenderer } from '../basic/SessionHTMLRenderer'; import { ContactName } from '../conversation/ContactName'; import { MessageReactions } from '../conversation/message/message-content/MessageReactions'; import { SessionIconButton } from '../icon'; @@ -163,18 +164,28 @@ const StyledCountText = styled.p` color: var(--color-text-subtle); text-align: center; margin: 16px auto 0; + + span { + color: var(--color-text); + } `; -const CountText = ({ count }: { count: number }) => { +const CountText = ({ count, emoji }: { count: number; emoji: string }) => { return ( - {count > SOGSReactorsFetchCount + 1 - ? window.i18n('reactionListCountPlural', [ - window.i18n('otherPlural', [String(count - SOGSReactorsFetchCount)]), - ]) - : window.i18n('reactionListCountSingular', [ - window.i18n('otherSingular', [String(count - SOGSReactorsFetchCount)]), - ])} + SOGSReactorsFetchCount + 1 + ? window.i18n('reactionListCountPlural', [ + window.i18n('otherPlural', [String(count - SOGSReactorsFetchCount)]), + emoji, + ]) + : window.i18n('reactionListCountSingular', [ + window.i18n('otherSingular', [String(count - SOGSReactorsFetchCount)]), + emoji, + ]) + } + /> ); }; @@ -355,7 +366,9 @@ export const ReactListModal = (props: Props): ReactElement => { handleClose={handleClose} /> )} - {isPublic && count && count > SOGSReactorsFetchCount && } + {isPublic && currentReact && count && count > SOGSReactorsFetchCount && ( + + )} )} From 5ebd1775c06cadc24ec9d924487b4bef76368a90 Mon Sep 17 00:00:00 2001 From: William Grant Date: Mon, 29 Aug 2022 16:10:47 +1000 Subject: [PATCH 03/11] feat: open group messages are now procesed via the cache cached entries are now added or removed based on the "optimistic" state that we want --- .../apis/open_group_api/sogsv3/sogsApiV3.ts | 4 +- .../sogsv3/sogsV3MutationCache.ts | 89 ++++++++++++++++--- .../sogsv3/sogsV3SendReaction.ts | 12 +-- ts/util/reactions.ts | 6 +- 4 files changed, 88 insertions(+), 23 deletions(-) diff --git a/ts/session/apis/open_group_api/sogsv3/sogsApiV3.ts b/ts/session/apis/open_group_api/sogsv3/sogsApiV3.ts index b4c0eac86..190c7ec15 100644 --- a/ts/session/apis/open_group_api/sogsv3/sogsApiV3.ts +++ b/ts/session/apis/open_group_api/sogsv3/sogsApiV3.ts @@ -34,7 +34,7 @@ import { handleOutboxMessageModel } from '../../../../receiver/dataMessage'; import { ConversationTypeEnum } from '../../../../models/conversationAttributes'; import { createSwarmMessageSentFromUs } from '../../../../models/messageFactory'; import { Data } from '../../../../data/data'; -import { handleOpenGroupMessageReactions } from '../../../../util/reactions'; +import { processMessagesUsingCache } from './sogsV3MutationCache'; /** * Get the convo matching those criteria and make sure it is an opengroup convo, or return null. @@ -312,7 +312,7 @@ const handleMessagesResponseV4 = async ( if (groupConvo && groupConvo.isOpenGroupV2()) { for (const message of messagesWithReactions) { void groupConvo.queueJob(async () => { - await handleOpenGroupMessageReactions(message.reactions, message.id); + await processMessagesUsingCache(serverUrl, roomId, message); }); } } diff --git a/ts/session/apis/open_group_api/sogsv3/sogsV3MutationCache.ts b/ts/session/apis/open_group_api/sogsv3/sogsV3MutationCache.ts index 775636108..9cefe2591 100644 --- a/ts/session/apis/open_group_api/sogsv3/sogsV3MutationCache.ts +++ b/ts/session/apis/open_group_api/sogsv3/sogsV3MutationCache.ts @@ -3,7 +3,9 @@ * Currently only supports message reactions 26/08/2022 */ -import { findIndex } from 'lodash'; +import { filter, findIndex, remove } from 'lodash'; +import { handleOpenGroupMessageReactions } from '../../../../util/reactions'; +import { OpenGroupMessageV4 } from '../opengroupV2/OpenGroupServerPoller'; export enum ChangeType { REACTIONS = 0, @@ -19,8 +21,8 @@ type ReactionChange = { export type SogsV3Mutation = { seqno: number | null; // null until mutating API request returns - server: string; // server address - room: string; // room name + server: string; // serverUrl + room: string; // roomId changeType: ChangeType; metadata: ReactionChange; // For now we only support message reactions }; @@ -42,27 +44,92 @@ function verifyEntry(entry: SogsV3Mutation): boolean { export function addToMutationCache(entry: SogsV3Mutation) { if (!verifyEntry(entry)) { - window.log.error('SOGS Mutation Cache: Entry verification failed!'); + window.log.error('SOGS Mutation Cache: Entry verification on add failed!', entry); } else { sogsMutationCache.push(entry); window.log.info('SOGS Mutation Cache: Entry added!', entry); } } -export function updateMutationCache(entry: SogsV3Mutation) { +export function updateMutationCache(entry: SogsV3Mutation, seqno: number) { if (!verifyEntry(entry)) { - window.log.error('SOGS Mutation Cache: Entry verification failed!'); + window.log.error('SOGS Mutation Cache: Entry verification on update failed!'); } else { const entryIndex = findIndex(sogsMutationCache, entry); if (entryIndex >= 0) { - sogsMutationCache[entryIndex] = entry; - window.log.info('SOGS Mutation Cache: Entry updated!', entry); + const updatedEntry = entry; + updatedEntry.seqno = seqno; + sogsMutationCache[entryIndex] = updatedEntry; + window.log.info('SOGS Mutation Cache: Entry updated!', updatedEntry); } else { - window.log.error('SOGS Mutation Cache: Updated failed! Cannot find entry'); + window.log.error('SOGS Mutation Cache: Updated failed! Cannot find entry', entry); } } } -export function removeFromMutationCache() { - // TODO +export async function processMessagesUsingCache( + server: string, + room: string, + message: OpenGroupMessageV4 +) { + const updatedReactions = message.reactions; + const matches: Array = filter(sogsMutationCache, { server, room }); + + if (matches?.length) { + for (const match of matches) { + if (message.seqno && match.seqno && match.seqno <= message.seqno) { + const removedEntry = remove(sogsMutationCache, match); + window.log.info('SOGS Mutation Cache: Entry ignored and removed!', removedEntry); + } else if (!message.seqno || (message.seqno && match.seqno && match.seqno > message.seqno)) { + for (const reaction of Object.keys(message.reactions)) { + const _matches = filter(sogsMutationCache, { + server, + room, + changeType: ChangeType.REACTIONS, + metadata: { + messageId: message.id, + emoji: reaction, + }, + }); + if (_matches?.length) { + for (const match of _matches) { + switch (match.metadata.action) { + case 'ADD': + updatedReactions[reaction].you = true; + updatedReactions[reaction].count += 1; + window.log.info( + 'SOGS Mutation Cache: Added our reaction based on the cache', + updatedReactions[reaction] + ); + break; + case 'REMOVE': + updatedReactions[reaction].you = false; + updatedReactions[reaction].count -= 1; + window.log.info( + 'SOGS Mutation Cache: Removed our reaction based on the cache', + updatedReactions[reaction] + ); + break; + default: + window.log.warn( + 'SOGS Mutation Cache: Unsupported metadata action in OpenGroupMessageV4', + match + ); + break; + } + } + + const removedMatches = remove(sogsMutationCache, ...matches); + window.log.info( + 'SOGS Mutation Cache: Removed processed entries from cache!', + removedMatches + ); + } + } + } + } + } + + message.reactions = updatedReactions; + await handleOpenGroupMessageReactions(message.reactions, message.id); } diff --git a/ts/session/apis/open_group_api/sogsv3/sogsV3SendReaction.ts b/ts/session/apis/open_group_api/sogsv3/sogsV3SendReaction.ts index 3796f9cdc..f92980891 100644 --- a/ts/session/apis/open_group_api/sogsv3/sogsV3SendReaction.ts +++ b/ts/session/apis/open_group_api/sogsv3/sogsV3SendReaction.ts @@ -36,7 +36,7 @@ export const hasReactionSupport = async (serverId: number): Promise => export const sendSogsReactionOnionV4 = async ( serverUrl: string, - room: string, + room: string, // this is the roomId abortSignal: AbortSignal, reaction: Reaction, blinded: boolean @@ -107,16 +107,10 @@ export const sendSogsReactionOnionV4 = async ( throw new Error('putReaction parsing failed'); } - window.log.info( - `You ${reaction.action === Action.REACT ? 'added' : 'removed'} a`, - reaction.emoji, - `reaction on ${serverUrl}/${room}` - ); const success = Boolean(reaction.action === Action.REACT ? rawMessage.added : rawMessage.removed); - if (success && rawMessage.seqno) { - cacheEntry.seqno = rawMessage.seqno; - updateMutationCache(cacheEntry); + if (success) { + updateMutationCache(cacheEntry, rawMessage.seqno); } return success; diff --git a/ts/util/reactions.ts b/ts/util/reactions.ts index 1d1b50a05..7cf1a759c 100644 --- a/ts/util/reactions.ts +++ b/ts/util/reactions.ts @@ -124,7 +124,11 @@ export const sendMessageReaction = async (messageId: string, emoji: string) => { `You ${action === Action.REACT ? 'added' : 'removed'} a`, emoji, 'reaction for message', - id + id, + found.get('isPublic') && + `on ${conversationModel.toOpenGroupV2().serverUrl}/${ + conversationModel.toOpenGroupV2().roomId + }` ); return reaction; } else { From dde61bb35b91bfffc794e817e6bbc2a0a5b23ab0 Mon Sep 17 00:00:00 2001 From: William Grant Date: Mon, 29 Aug 2022 17:29:10 +1000 Subject: [PATCH 04/11] feat: moderator clear all reactions behaviour now uses the cache --- ts/components/dialog/ReactListModal.tsx | 7 +--- .../sogsv3/sogsV3ClearReaction.ts | 37 ++++++++++++++++++- .../sogsv3/sogsV3MutationCache.ts | 30 ++++++++------- 3 files changed, 54 insertions(+), 20 deletions(-) diff --git a/ts/components/dialog/ReactListModal.tsx b/ts/components/dialog/ReactListModal.tsx index 3fb5bdccc..d833bbff2 100644 --- a/ts/components/dialog/ReactListModal.tsx +++ b/ts/components/dialog/ReactListModal.tsx @@ -3,9 +3,8 @@ import React, { ReactElement, useEffect, useState } from 'react'; import { useDispatch } from 'react-redux'; import styled from 'styled-components'; import { Data } from '../../data/data'; -import { useMessageReactsPropsById } from '../../hooks/useParamSelector'; +import { useMessageReactsPropsById, useWeAreModerator } from '../../hooks/useParamSelector'; import { isUsAnySogsFromCache } from '../../session/apis/open_group_api/sogsv3/knownBlindedkeys'; -import { getConversationController } from '../../session/conversations'; import { UserUtils } from '../../session/utils'; import { updateReactClearAllModal, @@ -285,9 +284,7 @@ export const ReactListModal = (props: Props): ReactElement => { const dispatch = useDispatch(); const { convoId, isPublic } = msgProps; - - const convo = getConversationController().get(convoId); - const weAreModerator = convo.getConversationModelProps().weAreModerator; + const weAreModerator = useWeAreModerator(convoId); const handleSelectedReaction = (emoji: string): boolean => { return currentReact === emoji; diff --git a/ts/session/apis/open_group_api/sogsv3/sogsV3ClearReaction.ts b/ts/session/apis/open_group_api/sogsv3/sogsV3ClearReaction.ts index c8f8c41f2..fefa9537d 100644 --- a/ts/session/apis/open_group_api/sogsv3/sogsV3ClearReaction.ts +++ b/ts/session/apis/open_group_api/sogsv3/sogsV3ClearReaction.ts @@ -1,4 +1,5 @@ import AbortController from 'abort-controller'; +import { OpenGroupReactionResponse } from '../../../../types/Reaction'; import { OpenGroupRequestCommonType } from '../opengroupV2/ApiUtil'; import { batchFirstSubIsSuccess, @@ -6,6 +7,12 @@ import { OpenGroupBatchRow, sogsBatchSend, } from './sogsV3BatchPoll'; +import { + addToMutationCache, + ChangeType, + SogsV3Mutation, + updateMutationCache, +} from './sogsV3MutationCache'; import { hasReactionSupport } from './sogsV3SendReaction'; /** @@ -23,6 +30,20 @@ export const clearSogsReactionByServerId = async ( return false; } + const cacheEntry: SogsV3Mutation = { + server: roomInfos.serverUrl, + room: roomInfos.roomId, + changeType: ChangeType.REACTIONS, + seqno: null, + metadata: { + messageId: serverId, + emoji: reaction, + action: 'CLEAR', + }, + }; + + addToMutationCache(cacheEntry); + const options: Array = [ { type: 'deleteReaction', @@ -37,8 +58,22 @@ export const clearSogsReactionByServerId = async ( 'batch' ); + if (!result) { + throw new Error('Could not deleteReaction, res is invalid'); + } + + const rawMessage = (result.body && (result.body[0].body as OpenGroupReactionResponse)) || null; + if (!rawMessage) { + throw new Error('deleteReaction parsing failed'); + } + try { - return batchGlobalIsSuccess(result) && batchFirstSubIsSuccess(result); + if (batchGlobalIsSuccess(result) && batchFirstSubIsSuccess(result)) { + updateMutationCache(cacheEntry, rawMessage.seqno); + return true; + } else { + return false; + } } catch (e) { window?.log?.error("clearSogsReactionByServerId Can't decode JSON body"); } diff --git a/ts/session/apis/open_group_api/sogsv3/sogsV3MutationCache.ts b/ts/session/apis/open_group_api/sogsv3/sogsV3MutationCache.ts index 9cefe2591..90f5d442d 100644 --- a/ts/session/apis/open_group_api/sogsv3/sogsV3MutationCache.ts +++ b/ts/session/apis/open_group_api/sogsv3/sogsV3MutationCache.ts @@ -11,7 +11,7 @@ export enum ChangeType { REACTIONS = 0, } -type ReactionAction = 'ADD' | 'REMOVE'; +type ReactionAction = 'ADD' | 'REMOVE' | 'CLEAR'; type ReactionChange = { messageId: number; // will be serverId of the reacted message @@ -73,16 +73,19 @@ export async function processMessagesUsingCache( message: OpenGroupMessageV4 ) { const updatedReactions = message.reactions; - const matches: Array = filter(sogsMutationCache, { server, room }); + const roomMatches: Array = filter(sogsMutationCache, { server, room }); - if (matches?.length) { - for (const match of matches) { - if (message.seqno && match.seqno && match.seqno <= message.seqno) { - const removedEntry = remove(sogsMutationCache, match); + if (roomMatches?.length) { + for (const roomMatch of roomMatches) { + if (message.seqno && roomMatch.seqno && roomMatch.seqno <= message.seqno) { + const removedEntry = remove(sogsMutationCache, roomMatch); window.log.info('SOGS Mutation Cache: Entry ignored and removed!', removedEntry); - } else if (!message.seqno || (message.seqno && match.seqno && match.seqno > message.seqno)) { + } else if ( + !message.seqno || + (message.seqno && roomMatch.seqno && roomMatch.seqno > message.seqno) + ) { for (const reaction of Object.keys(message.reactions)) { - const _matches = filter(sogsMutationCache, { + const reactionMatches = filter(sogsMutationCache, { server, room, changeType: ChangeType.REACTIONS, @@ -91,9 +94,9 @@ export async function processMessagesUsingCache( emoji: reaction, }, }); - if (_matches?.length) { - for (const match of _matches) { - switch (match.metadata.action) { + if (reactionMatches?.length) { + for (const reactionMatch of reactionMatches) { + switch (reactionMatch.metadata.action) { case 'ADD': updatedReactions[reaction].you = true; updatedReactions[reaction].count += 1; @@ -113,13 +116,12 @@ export async function processMessagesUsingCache( default: window.log.warn( 'SOGS Mutation Cache: Unsupported metadata action in OpenGroupMessageV4', - match + reactionMatch ); - break; } } - const removedMatches = remove(sogsMutationCache, ...matches); + const removedMatches = remove(sogsMutationCache, ...roomMatches); window.log.info( 'SOGS Mutation Cache: Removed processed entries from cache!', removedMatches From f309bf40f8a3eff620d015771540de3fa3e1c8a3 Mon Sep 17 00:00:00 2001 From: William Grant Date: Mon, 29 Aug 2022 17:44:58 +1000 Subject: [PATCH 05/11] fix: repaired reaction notifications for 1-1s --- ts/receiver/dataMessage.ts | 10 ++ ts/receiver/queuedJob.ts | 198 +++++++++++++++++-------------------- ts/util/reactions.ts | 9 +- 3 files changed, 107 insertions(+), 110 deletions(-) diff --git a/ts/receiver/dataMessage.ts b/ts/receiver/dataMessage.ts index 3c0ccdac0..59c98d431 100644 --- a/ts/receiver/dataMessage.ts +++ b/ts/receiver/dataMessage.ts @@ -22,6 +22,7 @@ import { appendFetchAvatarAndProfileJob } from './userProfileImageUpdates'; import { toLogFormat } from '../types/attachments/Errors'; import { ConversationTypeEnum } from '../models/conversationAttributes'; import { handleMessageReaction } from '../util/reactions'; +import { Action, Reaction } from '../types/Reaction'; function cleanAttachment(attachment: any) { return { @@ -326,6 +327,15 @@ async function handleSwarmMessage( msgModel.get('source'), isUsFromCache(msgModel.get('source')) ); + if ( + convoToAddMessageTo.isPrivate() && + msgModel.get('unread') && + rawDataMessage.reaction.action === Action.REACT + ) { + msgModel.set('reaction', rawDataMessage.reaction as Reaction); + convoToAddMessageTo.throttledNotify(msgModel); + } + confirm(); return; } diff --git a/ts/receiver/queuedJob.ts b/ts/receiver/queuedJob.ts index 03b4a67dc..f1bb88927 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 { Action, Reaction } from '../types/Reaction'; function contentTypeSupported(type: string): boolean { const Chrome = GoogleChrome; @@ -339,116 +338,103 @@ export async function handleMessageJob( ) || messageModel.get('timestamp')} in conversation ${conversation.idForLogging()}` ); - if (!messageModel.get('isPublic') && regularDataMessage.reaction) { - if ( - regularDataMessage.reaction.action === Action.REACT && - conversation.isPrivate() && - messageModel.get('unread') - ) { - messageModel.set('reaction', regularDataMessage.reaction as Reaction); - conversation.throttledNotify(messageModel); - } - - confirm?.(); - } else { - const sendingDeviceConversation = await getConversationController().getOrCreateAndWait( - source, - ConversationTypeEnum.PRIVATE - ); - try { - messageModel.set({ flags: regularDataMessage.flags }); - if (messageModel.isExpirationTimerUpdate()) { - const { expireTimer } = regularDataMessage; - const oldValue = conversation.get('expireTimer'); - if (expireTimer === oldValue) { - confirm?.(); - window?.log?.info( - 'Dropping ExpireTimerUpdate message as we already have the same one set.' - ); - return; - } - await handleExpirationTimerUpdateNoCommit(conversation, messageModel, source, expireTimer); - } else { - // this does not commit to db nor UI unless we need to approve a convo - await handleRegularMessage( - conversation, - sendingDeviceConversation, - messageModel, - regularDataMessage, - source, - messageHash + const sendingDeviceConversation = await getConversationController().getOrCreateAndWait( + source, + ConversationTypeEnum.PRIVATE + ); + try { + messageModel.set({ flags: regularDataMessage.flags }); + if (messageModel.isExpirationTimerUpdate()) { + const { expireTimer } = regularDataMessage; + const oldValue = conversation.get('expireTimer'); + if (expireTimer === oldValue) { + confirm?.(); + window?.log?.info( + 'Dropping ExpireTimerUpdate message as we already have the same one set.' ); + return; } + await handleExpirationTimerUpdateNoCommit(conversation, messageModel, source, expireTimer); + } else { + // this does not commit to db nor UI unless we need to approve a convo + await handleRegularMessage( + conversation, + sendingDeviceConversation, + messageModel, + regularDataMessage, + source, + messageHash + ); + } - // save the message model to the db and it save the messageId generated to our in-memory copy - const id = await messageModel.commit(); - messageModel.set({ id }); - - // Note that this can save the message again, if jobs were queued. We need to - // call it after we have an id for this message, because the jobs refer back - // to their source message. - - const unreadCount = await conversation.getUnreadCount(); - conversation.set({ unreadCount }); - conversation.set({ - active_at: Math.max(conversation.attributes.active_at, messageModel.get('sent_at') || 0), - }); - // this is a throttled call and will only run once every 1 sec at most - conversation.updateLastMessage(); - await conversation.commit(); - - if (conversation.id !== sendingDeviceConversation.id) { - await sendingDeviceConversation.commit(); - } + // save the message model to the db and it save the messageId generated to our in-memory copy + const id = await messageModel.commit(); + messageModel.set({ id }); - void queueAttachmentDownloads(messageModel, conversation); - // Check if we need to update any profile names - // the only profile we don't update with what is coming here is ours, - // as our profile is shared accross our devices with a ConfigurationMessage - if (messageModel.isIncoming() && regularDataMessage.profile) { - void appendFetchAvatarAndProfileJob( - sendingDeviceConversation, - regularDataMessage.profile, - regularDataMessage.profileKey - ); - } + // Note that this can save the message again, if jobs were queued. We need to + // call it after we have an id for this message, because the jobs refer back + // to their source message. - // even with all the warnings, I am very sus about if this is usefull or not - // try { - // // We go to the database here because, between the message save above and - // // the previous line's trigger() call, we might have marked all messages - // // unread in the database. This message might already be read! - // const fetched = await getMessageById(messageModel.get('id')); - - // const previousUnread = messageModel.get('unread'); - - // // Important to update message with latest read state from database - // messageModel.merge(fetched); - - // if (previousUnread !== messageModel.get('unread')) { - // window?.log?.warn( - // 'Caught race condition on new message read state! ' + 'Manually starting timers.' - // ); - // // We call markRead() even though the message is already - // // marked read because we need to start expiration - // // timers, etc. - // await messageModel.markRead(Date.now()); - // } - // } catch (error) { - // window?.log?.warn( - // 'handleMessageJob: Message', - // messageModel.idForLogging(), - // 'was deleted' - // ); - // } - - if (messageModel.get('unread')) { - conversation.throttledNotify(messageModel); - } - confirm?.(); - } catch (error) { - const errorForLog = error && error.stack ? error.stack : error; - window?.log?.error('handleMessageJob', messageModel.idForLogging(), 'error:', errorForLog); + const unreadCount = await conversation.getUnreadCount(); + conversation.set({ unreadCount }); + conversation.set({ + active_at: Math.max(conversation.attributes.active_at, messageModel.get('sent_at') || 0), + }); + // this is a throttled call and will only run once every 1 sec at most + conversation.updateLastMessage(); + await conversation.commit(); + + if (conversation.id !== sendingDeviceConversation.id) { + await sendingDeviceConversation.commit(); + } + + void queueAttachmentDownloads(messageModel, conversation); + // Check if we need to update any profile names + // the only profile we don't update with what is coming here is ours, + // as our profile is shared accross our devices with a ConfigurationMessage + if (messageModel.isIncoming() && regularDataMessage.profile) { + void appendFetchAvatarAndProfileJob( + sendingDeviceConversation, + regularDataMessage.profile, + regularDataMessage.profileKey + ); + } + + // even with all the warnings, I am very sus about if this is usefull or not + // try { + // // We go to the database here because, between the message save above and + // // the previous line's trigger() call, we might have marked all messages + // // unread in the database. This message might already be read! + // const fetched = await getMessageById(messageModel.get('id')); + + // const previousUnread = messageModel.get('unread'); + + // // Important to update message with latest read state from database + // messageModel.merge(fetched); + + // if (previousUnread !== messageModel.get('unread')) { + // window?.log?.warn( + // 'Caught race condition on new message read state! ' + 'Manually starting timers.' + // ); + // // We call markRead() even though the message is already + // // marked read because we need to start expiration + // // timers, etc. + // await messageModel.markRead(Date.now()); + // } + // } catch (error) { + // window?.log?.warn( + // 'handleMessageJob: Message', + // messageModel.idForLogging(), + // 'was deleted' + // ); + // } + + if (messageModel.get('unread')) { + conversation.throttledNotify(messageModel); } + confirm?.(); + } catch (error) { + const errorForLog = error && error.stack ? error.stack : error; + window?.log?.error('handleMessageJob', messageModel.idForLogging(), 'error:', errorForLog); } } diff --git a/ts/util/reactions.ts b/ts/util/reactions.ts index 7cf1a759c..44ad176ea 100644 --- a/ts/util/reactions.ts +++ b/ts/util/reactions.ts @@ -125,10 +125,11 @@ export const sendMessageReaction = async (messageId: string, emoji: string) => { emoji, 'reaction for message', id, - found.get('isPublic') && - `on ${conversationModel.toOpenGroupV2().serverUrl}/${ - conversationModel.toOpenGroupV2().roomId - }` + found.get('isPublic') + ? `on ${conversationModel.toOpenGroupV2().serverUrl}/${ + conversationModel.toOpenGroupV2().roomId + }` + : '' ); return reaction; } else { From b33ea096b4c6a35d55f391260178285f4754a879 Mon Sep 17 00:00:00 2001 From: William Grant Date: Tue, 30 Aug 2022 11:37:20 +1000 Subject: [PATCH 06/11] fix: speed up reaction UI update for opengroups --- ts/models/conversation.ts | 14 ++- ts/receiver/dataMessage.ts | 11 ++- .../sogsv3/sogsV3ClearReaction.ts | 19 +++- .../sogsv3/sogsV3SendReaction.ts | 35 +++++-- .../unit/reactions/ReactionMessage_test.ts | 22 +++-- ts/util/reactions.ts | 97 +++++++++++++------ 6 files changed, 138 insertions(+), 60 deletions(-) diff --git a/ts/models/conversation.ts b/ts/models/conversation.ts index 5719dfcca..698966d67 100644 --- a/ts/models/conversation.ts +++ b/ts/models/conversation.ts @@ -737,7 +737,12 @@ export class ConversationModel extends Backbone.Model { const chatMessagePrivate = new VisibleMessage(chatMessageParams); await getMessageQueue().sendToPubKey(destinationPubkey, chatMessagePrivate); - await handleMessageReaction(reaction, UserUtils.getOurPubKeyStrFromCache(), true); + await handleMessageReaction({ + reaction, + sender: UserUtils.getOurPubKeyStrFromCache(), + you: true, + isOpenGroup: false, + }); return; } @@ -749,7 +754,12 @@ export class ConversationModel extends Backbone.Model { }); // we need the return await so that errors are caught in the catch {} await getMessageQueue().sendToGroup(closedGroupVisibleMessage); - await handleMessageReaction(reaction, UserUtils.getOurPubKeyStrFromCache(), true); + await handleMessageReaction({ + reaction, + sender: UserUtils.getOurPubKeyStrFromCache(), + you: true, + isOpenGroup: false, + }); return; } diff --git a/ts/receiver/dataMessage.ts b/ts/receiver/dataMessage.ts index 59c98d431..a207bc219 100644 --- a/ts/receiver/dataMessage.ts +++ b/ts/receiver/dataMessage.ts @@ -322,11 +322,12 @@ async function handleSwarmMessage( // this call has to be made inside the queueJob! // We handle reaction DataMessages separately if (!msgModel.get('isPublic') && rawDataMessage.reaction) { - await handleMessageReaction( - rawDataMessage.reaction, - msgModel.get('source'), - isUsFromCache(msgModel.get('source')) - ); + await handleMessageReaction({ + reaction: rawDataMessage.reaction, + sender: msgModel.get('source'), + you: isUsFromCache(msgModel.get('source')), + isOpenGroup: false, + }); if ( convoToAddMessageTo.isPrivate() && msgModel.get('unread') && diff --git a/ts/session/apis/open_group_api/sogsv3/sogsV3ClearReaction.ts b/ts/session/apis/open_group_api/sogsv3/sogsV3ClearReaction.ts index fefa9537d..03df53a74 100644 --- a/ts/session/apis/open_group_api/sogsv3/sogsV3ClearReaction.ts +++ b/ts/session/apis/open_group_api/sogsv3/sogsV3ClearReaction.ts @@ -1,5 +1,6 @@ import AbortController from 'abort-controller'; import { OpenGroupReactionResponse } from '../../../../types/Reaction'; +import { handleClearReaction } from '../../../../util/reactions'; import { OpenGroupRequestCommonType } from '../opengroupV2/ApiUtil'; import { batchFirstSubIsSuccess, @@ -25,8 +26,13 @@ export const clearSogsReactionByServerId = async ( serverId: number, roomInfos: OpenGroupRequestCommonType ): Promise => { - const canReact = await hasReactionSupport(serverId); - if (!canReact) { + const { supported, conversation } = await hasReactionSupport(serverId); + if (!supported) { + return false; + } + + if (!conversation) { + window.log.warn(`Conversation for ${reaction} not found in db`); return false; } @@ -44,10 +50,17 @@ export const clearSogsReactionByServerId = async ( addToMutationCache(cacheEntry); + // Since responses can take a long time we immediately update the moderators's UI and if there is a problem it is overwritten by handleOpenGroupMessageReactions later. + await handleClearReaction(serverId, reaction); + const options: Array = [ { type: 'deleteReaction', - deleteReaction: { reaction, messageId: serverId, roomId: roomInfos.roomId }, + deleteReaction: { + reaction, + messageId: serverId, + roomId: roomInfos.roomId, + }, }, ]; const result = await sogsBatchSend( diff --git a/ts/session/apis/open_group_api/sogsv3/sogsV3SendReaction.ts b/ts/session/apis/open_group_api/sogsv3/sogsV3SendReaction.ts index f92980891..1e0a9a3b1 100644 --- a/ts/session/apis/open_group_api/sogsv3/sogsV3SendReaction.ts +++ b/ts/session/apis/open_group_api/sogsv3/sogsV3SendReaction.ts @@ -1,10 +1,13 @@ import { AbortSignal } from 'abort-controller'; import { Data } from '../../../../data/data'; +import { ConversationModel } from '../../../../models/conversation'; import { Action, OpenGroupReactionResponse, Reaction } from '../../../../types/Reaction'; import { getEmojiDataFromNative } from '../../../../util/emoji'; -import { hitRateLimit } from '../../../../util/reactions'; +import { handleMessageReaction, hitRateLimit } from '../../../../util/reactions'; import { OnionSending } from '../../../onions/onionSend'; +import { UserUtils } from '../../../utils'; import { OpenGroupPollingUtils } from '../opengroupV2/OpenGroupPollingUtils'; +import { getUsBlindedInThatServer } from './knownBlindedkeys'; import { batchGlobalIsSuccess, parseBatchGlobalStatusCode } from './sogsV3BatchPoll'; import { addToMutationCache, @@ -13,25 +16,27 @@ import { updateMutationCache, } from './sogsV3MutationCache'; -export const hasReactionSupport = async (serverId: number): Promise => { +export const hasReactionSupport = async ( + serverId: number +): Promise<{ supported: boolean; conversation: ConversationModel | null }> => { const found = await Data.getMessageByServerId(serverId); if (!found) { window.log.warn(`Open Group Message ${serverId} not found in db`); - return false; + return { supported: false, conversation: null }; } const conversationModel = found?.getConversation(); if (!conversationModel) { window.log.warn(`Conversation for ${serverId} not found in db`); - return false; + return { supported: false, conversation: null }; } if (!conversationModel.hasReactions()) { window.log.warn("This open group doesn't have reaction support. Server Message ID", serverId); - return false; + return { supported: false, conversation: null }; } - return true; + return { supported: true, conversation: conversationModel }; }; export const sendSogsReactionOnionV4 = async ( @@ -47,8 +52,8 @@ export const sendSogsReactionOnionV4 = async ( throw new Error(`Could not find sogs pubkey of url:${serverUrl}`); } - const canReact = await hasReactionSupport(reaction.id); - if (!canReact) { + const { supported, conversation } = await hasReactionSupport(reaction.id); + if (!supported) { return false; } @@ -56,6 +61,11 @@ export const sendSogsReactionOnionV4 = async ( return false; } + if (!conversation) { + window.log.warn(`Conversation for ${reaction.id} not found in db`); + return false; + } + // The SOGS endpoint supports any text input so we need to make sure we are sending a valid unicode emoji // for an invalid input we use https://emojipedia.org/frame-with-an-x/ as a replacement since it cannot rendered as an emoji but is valid unicode const emoji = getEmojiDataFromNative(reaction.emoji) ? reaction.emoji : '🖾'; @@ -77,6 +87,15 @@ export const sendSogsReactionOnionV4 = async ( addToMutationCache(cacheEntry); + // Since responses can take a long time we immediately update the sender's UI and if there is a problem it is overwritten by handleOpenGroupMessageReactions later. + const me = UserUtils.getOurPubKeyStrFromCache(); + await handleMessageReaction({ + reaction, + sender: blinded ? getUsBlindedInThatServer(conversation) || me : me, + you: true, + isOpenGroup: true, + }); + // reaction endpoint requires an empty dict {} const stringifiedBody = null; const result = await OnionSending.sendJsonViaOnionV4ToSogs({ diff --git a/ts/test/session/unit/reactions/ReactionMessage_test.ts b/ts/test/session/unit/reactions/ReactionMessage_test.ts index 63bfa3748..d7a7bf127 100644 --- a/ts/test/session/unit/reactions/ReactionMessage_test.ts +++ b/ts/test/session/unit/reactions/ReactionMessage_test.ts @@ -52,11 +52,12 @@ describe('ReactionMessage', () => { expect(reaction?.action, 'action should be 0').to.be.equal(0); // Handling reaction - const updatedMessage = await handleMessageReaction( - reaction as SignalService.DataMessage.IReaction, - ourNumber, - true - ); + const updatedMessage = await handleMessageReaction({ + reaction: reaction as SignalService.DataMessage.IReaction, + sender: ourNumber, + you: true, + isOpenGroup: false, + }); expect(updatedMessage?.get('reacts'), 'original message should have reacts').to.not.be .undefined; @@ -84,11 +85,12 @@ describe('ReactionMessage', () => { expect(reaction?.action, 'action should be 1').to.be.equal(1); // Handling reaction - const updatedMessage = await handleMessageReaction( - reaction as SignalService.DataMessage.IReaction, - ourNumber, - true - ); + const updatedMessage = await handleMessageReaction({ + reaction: reaction as SignalService.DataMessage.IReaction, + sender: ourNumber, + you: true, + isOpenGroup: false, + }); expect(updatedMessage?.get('reacts'), 'original message reacts should be undefined').to.be .undefined; diff --git a/ts/util/reactions.ts b/ts/util/reactions.ts index 44ad176ea..9511c415d 100644 --- a/ts/util/reactions.ts +++ b/ts/util/reactions.ts @@ -37,23 +37,28 @@ export function hitRateLimit(): boolean { * Retrieves the original message of a reaction */ const getMessageByReaction = async ( - reaction: SignalService.DataMessage.IReaction + reaction: SignalService.DataMessage.IReaction, + isOpenGroup: boolean ): Promise => { let originalMessage = null; const originalMessageId = Number(reaction.id); const originalMessageAuthor = reaction.author; - 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 (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 + ); + }); + } if (!originalMessage) { window?.log?.warn(`Cannot find the original reacted message ${originalMessageId}.`); @@ -140,19 +145,25 @@ 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 + * Used in OpenGroups for sending reactions only, not handling responses */ -export const handleMessageReaction = async ( - reaction: SignalService.DataMessage.IReaction, - sender: string, - you: boolean -) => { +export const handleMessageReaction = async ({ + reaction, + sender, + you, + isOpenGroup, +}: { + reaction: SignalService.DataMessage.IReaction; + sender: string; + you: boolean; + isOpenGroup: boolean; +}) => { if (!reaction.emoji) { window?.log?.warn(`There is no emoji for the reaction ${reaction}.`); return; } - const originalMessage = await getMessageByReaction(reaction); + const originalMessage = await getMessageByReaction(reaction, isOpenGroup); if (!originalMessage) { return; } @@ -163,20 +174,15 @@ export const handleMessageReaction = async ( const senders = details.senders; let count = details.count || 0; - if (originalMessage.get('isPublic')) { - window.log.warn("handleMessageReaction() shouldn't be used in opengroups"); - return; - } else { - if (details.you && senders.includes(sender)) { - if (reaction.action === Action.REACT) { - window.log.warn('Received duplicate message for your reaction. Ignoring it'); - return; - } else { - details.you = false; - } + if (details.you && senders.includes(sender)) { + if (reaction.action === Action.REACT) { + window.log.warn('Received duplicate message for your reaction. Ignoring it'); + return; } else { - details.you = you; + details.you = false; } + } else { + details.you = you; } switch (reaction.action) { @@ -230,7 +236,34 @@ export const handleMessageReaction = async ( }; /** - * Handles all message reaction updates for opengroups + * Handles updating the UI when clearing all reactions for a certain emoji + * Only usable by moderators in opengroups and runs on their client + */ +export const handleClearReaction = async (serverId: number, emoji: string) => { + const originalMessage = await Data.getMessageByServerId(serverId); + if (!originalMessage) { + window?.log?.warn(`Cannot find the original reacted message ${serverId}.`); + return; + } + + const reacts: ReactionList | undefined = originalMessage.get('reacts'); + if (reacts) { + // tslint:disable-next-line: no-dynamic-delete + delete reacts[emoji]; + } + + originalMessage.set({ + reacts: !isEmpty(reacts) ? reacts : undefined, + }); + + await originalMessage.commit(); + + window.log.info(`You cleared all ${emoji} reactions on message ${serverId}`); + return originalMessage; +}; + +/** + * Handles all message reaction updates/responses for opengroups */ export const handleOpenGroupMessageReactions = async ( reactions: OpenGroupReactionList, From bbfb55f211631bb863de522b8cb8ceb07f7a4622 Mon Sep 17 00:00:00 2001 From: William Grant Date: Tue, 30 Aug 2022 11:53:35 +1000 Subject: [PATCH 07/11] fix: pr review fixes --- ts/components/dialog/ReactListModal.tsx | 9 +- .../sogsv3/sogsV3MutationCache.ts | 100 ++++++++---------- 2 files changed, 50 insertions(+), 59 deletions(-) diff --git a/ts/components/dialog/ReactListModal.tsx b/ts/components/dialog/ReactListModal.tsx index d833bbff2..cc2257e0a 100644 --- a/ts/components/dialog/ReactListModal.tsx +++ b/ts/components/dialog/ReactListModal.tsx @@ -216,15 +216,17 @@ const handleSenders = (senders: Array, me: string) => { export const ReactListModal = (props: Props): ReactElement => { const { reaction, messageId } = props; + const dispatch = useDispatch(); const [reactions, setReactions] = useState([]); const reactionsMap = (reactions && Object.fromEntries(reactions)) || {}; const [currentReact, setCurrentReact] = useState(''); const [reactAriaLabel, setReactAriaLabel] = useState(); const [count, setCount] = useState(null); const [senders, setSenders] = useState>([]); - const me = UserUtils.getOurPubKeyStrFromCache(); const msgProps = useMessageReactsPropsById(messageId); + const weAreModerator = useWeAreModerator(msgProps?.convoId); + const me = UserUtils.getOurPubKeyStrFromCache(); // tslint:disable: cyclomatic-complexity useEffect(() => { @@ -281,10 +283,7 @@ export const ReactListModal = (props: Props): ReactElement => { return <>; } - const dispatch = useDispatch(); - - const { convoId, isPublic } = msgProps; - const weAreModerator = useWeAreModerator(convoId); + const { isPublic } = msgProps; const handleSelectedReaction = (emoji: string): boolean => { return currentReact === emoji; diff --git a/ts/session/apis/open_group_api/sogsv3/sogsV3MutationCache.ts b/ts/session/apis/open_group_api/sogsv3/sogsV3MutationCache.ts index 90f5d442d..3c90a66b9 100644 --- a/ts/session/apis/open_group_api/sogsv3/sogsV3MutationCache.ts +++ b/ts/session/apis/open_group_api/sogsv3/sogsV3MutationCache.ts @@ -57,10 +57,8 @@ export function updateMutationCache(entry: SogsV3Mutation, seqno: number) { } else { const entryIndex = findIndex(sogsMutationCache, entry); if (entryIndex >= 0) { - const updatedEntry = entry; - updatedEntry.seqno = seqno; - sogsMutationCache[entryIndex] = updatedEntry; - window.log.info('SOGS Mutation Cache: Entry updated!', updatedEntry); + sogsMutationCache[entryIndex].seqno = seqno; + window.log.info('SOGS Mutation Cache: Entry updated!', sogsMutationCache[entryIndex]); } else { window.log.error('SOGS Mutation Cache: Updated failed! Cannot find entry', entry); } @@ -75,63 +73,57 @@ export async function processMessagesUsingCache( const updatedReactions = message.reactions; const roomMatches: Array = filter(sogsMutationCache, { server, room }); - if (roomMatches?.length) { - for (const roomMatch of roomMatches) { - if (message.seqno && roomMatch.seqno && roomMatch.seqno <= message.seqno) { - const removedEntry = remove(sogsMutationCache, roomMatch); - window.log.info('SOGS Mutation Cache: Entry ignored and removed!', removedEntry); - } else if ( - !message.seqno || - (message.seqno && roomMatch.seqno && roomMatch.seqno > message.seqno) - ) { - for (const reaction of Object.keys(message.reactions)) { - const reactionMatches = filter(sogsMutationCache, { - server, - room, - changeType: ChangeType.REACTIONS, - metadata: { - messageId: message.id, - emoji: reaction, - }, - }); - if (reactionMatches?.length) { - for (const reactionMatch of reactionMatches) { - switch (reactionMatch.metadata.action) { - case 'ADD': - updatedReactions[reaction].you = true; - updatedReactions[reaction].count += 1; - window.log.info( - 'SOGS Mutation Cache: Added our reaction based on the cache', - updatedReactions[reaction] - ); - break; - case 'REMOVE': - updatedReactions[reaction].you = false; - updatedReactions[reaction].count -= 1; - window.log.info( - 'SOGS Mutation Cache: Removed our reaction based on the cache', - updatedReactions[reaction] - ); - break; - default: - window.log.warn( - 'SOGS Mutation Cache: Unsupported metadata action in OpenGroupMessageV4', - reactionMatch - ); - } - } + for (const roomMatch of roomMatches) { + if (message.seqno && roomMatch.seqno && roomMatch.seqno <= message.seqno) { + const removedEntry = remove(sogsMutationCache, roomMatch); + window.log.info('SOGS Mutation Cache: Entry ignored and removed!', removedEntry); + } else if ( + !message.seqno || + (message.seqno && roomMatch.seqno && roomMatch.seqno > message.seqno) + ) { + for (const reaction of Object.keys(message.reactions)) { + const reactionMatches = filter(sogsMutationCache, { + server, + room, + changeType: ChangeType.REACTIONS, + metadata: { + messageId: message.id, + emoji: reaction, + }, + }); - const removedMatches = remove(sogsMutationCache, ...roomMatches); - window.log.info( - 'SOGS Mutation Cache: Removed processed entries from cache!', - removedMatches - ); + for (const reactionMatch of reactionMatches) { + switch (reactionMatch.metadata.action) { + case 'ADD': + updatedReactions[reaction].you = true; + updatedReactions[reaction].count += 1; + window.log.info( + 'SOGS Mutation Cache: Added our reaction based on the cache', + updatedReactions[reaction] + ); + break; + case 'REMOVE': + updatedReactions[reaction].you = false; + updatedReactions[reaction].count -= 1; + window.log.info( + 'SOGS Mutation Cache: Removed our reaction based on the cache', + updatedReactions[reaction] + ); + break; + default: + window.log.warn( + 'SOGS Mutation Cache: Unsupported metadata action in OpenGroupMessageV4', + reactionMatch + ); } } } } } + const removedMatches = remove(sogsMutationCache, ...roomMatches); + window.log.info('SOGS Mutation Cache: Removed processed entries from cache!', removedMatches); + message.reactions = updatedReactions; await handleOpenGroupMessageReactions(message.reactions, message.id); } From 3060ffd25ac52e8999a49e0217e0b6d34cb48e93 Mon Sep 17 00:00:00 2001 From: William Grant Date: Tue, 30 Aug 2022 17:18:22 +1000 Subject: [PATCH 08/11] test: added tests for adding and updating sogs cache entries updated idForLogging for opengroups to be more verbose, updated reaction method calls to use exported Reactions object --- .../MessageContentWithStatus.tsx | 4 +- .../message-content/MessageContextMenu.tsx | 4 +- ts/components/dialog/ReactListModal.tsx | 12 +- ts/models/conversation.ts | 9 +- ts/receiver/dataMessage.ts | 4 +- .../open_group_api/sogsv3/sogsV3BatchPoll.ts | 6 +- .../sogsv3/sogsV3ClearReaction.ts | 4 +- .../sogsv3/sogsV3MutationCache.ts | 36 +++-- .../sogsv3/sogsV3SendReaction.ts | 6 +- .../unit/reactions/ReactionMessage_test.ts | 16 +- .../receiver/opengroup/deduplicate_test.ts | 50 +++--- .../session/unit/sending/MessageQueue_test.ts | 2 +- .../session/unit/sogsv3/MutationCache_test.ts | 142 ++++++++++++++++++ ts/test/test-utils/utils/message.ts | 25 ++- ts/test/test-utils/utils/stubbing.ts | 2 +- ts/util/reactions.ts | 25 ++- 16 files changed, 260 insertions(+), 87 deletions(-) create mode 100644 ts/test/session/unit/sogsv3/MutationCache_test.ts diff --git a/ts/components/conversation/message/message-content/MessageContentWithStatus.tsx b/ts/components/conversation/message/message-content/MessageContentWithStatus.tsx index a069e5983..a7e0563a5 100644 --- a/ts/components/conversation/message/message-content/MessageContentWithStatus.tsx +++ b/ts/components/conversation/message/message-content/MessageContentWithStatus.tsx @@ -10,7 +10,7 @@ import { getMessageContentWithStatusesSelectorProps, isMessageSelectionMode, } from '../../../../state/selectors/conversations'; -import { sendMessageReaction } from '../../../../util/reactions'; +import { Reactions } from '../../../../util/reactions'; import { MessageAuthorText } from './MessageAuthorText'; import { MessageContent } from './MessageContent'; @@ -93,7 +93,7 @@ export const MessageContentWithStatuses = (props: Props) => { const [popupReaction, setPopupReaction] = useState(''); const handleMessageReaction = async (emoji: string) => { - await sendMessageReaction(messageId, emoji); + await Reactions.sendMessageReaction(messageId, emoji); }; const handlePopupClick = () => { diff --git a/ts/components/conversation/message/message-content/MessageContextMenu.tsx b/ts/components/conversation/message/message-content/MessageContextMenu.tsx index 8103da28c..cc28174d3 100644 --- a/ts/components/conversation/message/message-content/MessageContextMenu.tsx +++ b/ts/components/conversation/message/message-content/MessageContextMenu.tsx @@ -25,7 +25,7 @@ import { import { StateType } from '../../../../state/reducer'; import { getMessageContextMenuProps } from '../../../../state/selectors/conversations'; import { saveAttachmentToDisk } from '../../../../util/attachmentsUtil'; -import { sendMessageReaction } from '../../../../util/reactions'; +import { Reactions } from '../../../../util/reactions'; import { SessionEmojiPanel, StyledEmojiPanel } from '../../SessionEmojiPanel'; import { MessageReactBar } from './MessageReactBar'; @@ -241,7 +241,7 @@ export const MessageContextMenu = (props: Props) => { const onEmojiClick = async (args: any) => { const emoji = args.native ?? args; onCloseEmoji(); - await sendMessageReaction(messageId, emoji); + await Reactions.sendMessageReaction(messageId, emoji); }; const onEmojiKeyDown = (event: any) => { diff --git a/ts/components/dialog/ReactListModal.tsx b/ts/components/dialog/ReactListModal.tsx index cc2257e0a..4c5ac2375 100644 --- a/ts/components/dialog/ReactListModal.tsx +++ b/ts/components/dialog/ReactListModal.tsx @@ -13,7 +13,7 @@ import { } from '../../state/ducks/modalDialog'; import { SortedReactionList } from '../../types/Reaction'; import { nativeEmojiData } from '../../util/emoji'; -import { sendMessageReaction, SOGSReactorsFetchCount } from '../../util/reactions'; +import { Reactions } from '../../util/reactions'; import { Avatar, AvatarSize } from '../avatar/Avatar'; import { Flex } from '../basic/Flex'; import { SessionHtmlRenderer } from '../basic/SessionHTMLRenderer'; @@ -110,7 +110,7 @@ const ReactionSenders = (props: ReactionSendersProps) => { }; const handleRemoveReaction = async () => { - await sendMessageReaction(messageId, currentReact); + await Reactions.sendMessageReaction(messageId, currentReact); if (senders.length <= 1) { dispatch(updateReactListModal(null)); @@ -174,13 +174,13 @@ const CountText = ({ count, emoji }: { count: number; emoji: string }) => { SOGSReactorsFetchCount + 1 + count > Reactions.SOGSReactorsFetchCount + 1 ? window.i18n('reactionListCountPlural', [ - window.i18n('otherPlural', [String(count - SOGSReactorsFetchCount)]), + window.i18n('otherPlural', [String(count - Reactions.SOGSReactorsFetchCount)]), emoji, ]) : window.i18n('reactionListCountSingular', [ - window.i18n('otherSingular', [String(count - SOGSReactorsFetchCount)]), + window.i18n('otherSingular', [String(count - Reactions.SOGSReactorsFetchCount)]), emoji, ]) } @@ -362,7 +362,7 @@ export const ReactListModal = (props: Props): ReactElement => { handleClose={handleClose} /> )} - {isPublic && currentReact && count && count > SOGSReactorsFetchCount && ( + {isPublic && currentReact && count && count > Reactions.SOGSReactorsFetchCount && ( )} diff --git a/ts/models/conversation.ts b/ts/models/conversation.ts index 698966d67..73d3be41e 100644 --- a/ts/models/conversation.ts +++ b/ts/models/conversation.ts @@ -93,7 +93,7 @@ import { } 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'; +import { Reactions } from '../util/reactions'; export class ConversationModel extends Backbone.Model { public updateLastMessage: () => any; @@ -193,7 +193,8 @@ export class ConversationModel extends Backbone.Model { } if (this.isPublic()) { - return `opengroup(${this.id})`; + const opengroup = this.toOpenGroupV2(); + return `${opengroup.serverUrl}/${opengroup.roomId}`; } return `group(${ed25519Str(this.id)})`; @@ -737,7 +738,7 @@ export class ConversationModel extends Backbone.Model { const chatMessagePrivate = new VisibleMessage(chatMessageParams); await getMessageQueue().sendToPubKey(destinationPubkey, chatMessagePrivate); - await handleMessageReaction({ + await Reactions.handleMessageReaction({ reaction, sender: UserUtils.getOurPubKeyStrFromCache(), you: true, @@ -754,7 +755,7 @@ export class ConversationModel extends Backbone.Model { }); // we need the return await so that errors are caught in the catch {} await getMessageQueue().sendToGroup(closedGroupVisibleMessage); - await handleMessageReaction({ + await Reactions.handleMessageReaction({ reaction, sender: UserUtils.getOurPubKeyStrFromCache(), you: true, diff --git a/ts/receiver/dataMessage.ts b/ts/receiver/dataMessage.ts index a207bc219..3be5c0935 100644 --- a/ts/receiver/dataMessage.ts +++ b/ts/receiver/dataMessage.ts @@ -21,7 +21,7 @@ import { isUsFromCache } from '../session/utils/User'; import { appendFetchAvatarAndProfileJob } from './userProfileImageUpdates'; import { toLogFormat } from '../types/attachments/Errors'; import { ConversationTypeEnum } from '../models/conversationAttributes'; -import { handleMessageReaction } from '../util/reactions'; +import { Reactions } from '../util/reactions'; import { Action, Reaction } from '../types/Reaction'; function cleanAttachment(attachment: any) { @@ -322,7 +322,7 @@ async function handleSwarmMessage( // this call has to be made inside the queueJob! // We handle reaction DataMessages separately if (!msgModel.get('isPublic') && rawDataMessage.reaction) { - await handleMessageReaction({ + await Reactions.handleMessageReaction({ reaction: rawDataMessage.reaction, sender: msgModel.get('source'), you: isUsFromCache(msgModel.get('source')), diff --git a/ts/session/apis/open_group_api/sogsv3/sogsV3BatchPoll.ts b/ts/session/apis/open_group_api/sogsv3/sogsV3BatchPoll.ts index d3492417d..073ea5f11 100644 --- a/ts/session/apis/open_group_api/sogsv3/sogsV3BatchPoll.ts +++ b/ts/session/apis/open_group_api/sogsv3/sogsV3BatchPoll.ts @@ -8,7 +8,7 @@ import { import { addJsonContentTypeToHeaders } from './sogsV3SendMessage'; import { AbortSignal } from 'abort-controller'; import { roomHasBlindEnabled } from './sogsV3Capabilities'; -import { SOGSReactorsFetchCount } from '../../../../util/reactions'; +import { Reactions } from '../../../../util/reactions'; type BatchFetchRequestOptions = { method: 'POST' | 'PUT' | 'GET' | 'DELETE'; @@ -240,8 +240,8 @@ const makeBatchRequestPayload = ( return { method: 'GET', path: isNumber(options.messages.sinceSeqNo) - ? `/room/${options.messages.roomId}/messages/since/${options.messages.sinceSeqNo}?t=r&reactors=${SOGSReactorsFetchCount}` - : `/room/${options.messages.roomId}/messages/recent?reactors=${SOGSReactorsFetchCount}`, + ? `/room/${options.messages.roomId}/messages/since/${options.messages.sinceSeqNo}?t=r&reactors=${Reactions.SOGSReactorsFetchCount}` + : `/room/${options.messages.roomId}/messages/recent?reactors=${Reactions.SOGSReactorsFetchCount}`, }; } break; diff --git a/ts/session/apis/open_group_api/sogsv3/sogsV3ClearReaction.ts b/ts/session/apis/open_group_api/sogsv3/sogsV3ClearReaction.ts index 03df53a74..eb358757e 100644 --- a/ts/session/apis/open_group_api/sogsv3/sogsV3ClearReaction.ts +++ b/ts/session/apis/open_group_api/sogsv3/sogsV3ClearReaction.ts @@ -1,6 +1,6 @@ import AbortController from 'abort-controller'; import { OpenGroupReactionResponse } from '../../../../types/Reaction'; -import { handleClearReaction } from '../../../../util/reactions'; +import { Reactions } from '../../../../util/reactions'; import { OpenGroupRequestCommonType } from '../opengroupV2/ApiUtil'; import { batchFirstSubIsSuccess, @@ -51,7 +51,7 @@ export const clearSogsReactionByServerId = async ( addToMutationCache(cacheEntry); // Since responses can take a long time we immediately update the moderators's UI and if there is a problem it is overwritten by handleOpenGroupMessageReactions later. - await handleClearReaction(serverId, reaction); + await Reactions.handleClearReaction(serverId, reaction); const options: Array = [ { diff --git a/ts/session/apis/open_group_api/sogsv3/sogsV3MutationCache.ts b/ts/session/apis/open_group_api/sogsv3/sogsV3MutationCache.ts index 3c90a66b9..9874830bc 100644 --- a/ts/session/apis/open_group_api/sogsv3/sogsV3MutationCache.ts +++ b/ts/session/apis/open_group_api/sogsv3/sogsV3MutationCache.ts @@ -4,7 +4,7 @@ */ import { filter, findIndex, remove } from 'lodash'; -import { handleOpenGroupMessageReactions } from '../../../../util/reactions'; +import { Reactions } from '../../../../util/reactions'; import { OpenGroupMessageV4 } from '../opengroupV2/OpenGroupServerPoller'; export enum ChangeType { @@ -32,28 +32,35 @@ const sogsMutationCache: Array = []; function verifyEntry(entry: SogsV3Mutation): boolean { return Boolean( - !entry.server || - !entry.room || - entry.seqno !== null || - entry.metadata.messageId || - entry.metadata.emoji || - entry.metadata.action === 'ADD' || - entry.metadata.action === 'REMOVE' + entry.server && + entry.server !== '' && + entry.room && + entry.room !== '' && + entry.changeType === ChangeType.REACTIONS && + entry.metadata.messageId && + entry.metadata.emoji && + entry.metadata.emoji !== '' && + (entry.metadata.action === 'ADD' || + entry.metadata.action === 'REMOVE' || + entry.metadata.action === 'CLEAR') ); } -export function addToMutationCache(entry: SogsV3Mutation) { +// we return the cache for testing +export function addToMutationCache(entry: SogsV3Mutation): Array { if (!verifyEntry(entry)) { window.log.error('SOGS Mutation Cache: Entry verification on add failed!', entry); } else { sogsMutationCache.push(entry); window.log.info('SOGS Mutation Cache: Entry added!', entry); } + return sogsMutationCache; } -export function updateMutationCache(entry: SogsV3Mutation, seqno: number) { +// we return the cache for testing +export function updateMutationCache(entry: SogsV3Mutation, seqno: number): Array { if (!verifyEntry(entry)) { - window.log.error('SOGS Mutation Cache: Entry verification on update failed!'); + window.log.error('SOGS Mutation Cache: Entry verification on update failed!', entry); } else { const entryIndex = findIndex(sogsMutationCache, entry); if (entryIndex >= 0) { @@ -63,13 +70,15 @@ export function updateMutationCache(entry: SogsV3Mutation, seqno: number) { window.log.error('SOGS Mutation Cache: Updated failed! Cannot find entry', entry); } } + return sogsMutationCache; } +// return is for testing purposes only export async function processMessagesUsingCache( server: string, room: string, message: OpenGroupMessageV4 -) { +): Promise { const updatedReactions = message.reactions; const roomMatches: Array = filter(sogsMutationCache, { server, room }); @@ -125,5 +134,6 @@ export async function processMessagesUsingCache( window.log.info('SOGS Mutation Cache: Removed processed entries from cache!', removedMatches); message.reactions = updatedReactions; - await handleOpenGroupMessageReactions(message.reactions, message.id); + await Reactions.handleOpenGroupMessageReactions(message.reactions, message.id); + return message; } diff --git a/ts/session/apis/open_group_api/sogsv3/sogsV3SendReaction.ts b/ts/session/apis/open_group_api/sogsv3/sogsV3SendReaction.ts index 1e0a9a3b1..445219e40 100644 --- a/ts/session/apis/open_group_api/sogsv3/sogsV3SendReaction.ts +++ b/ts/session/apis/open_group_api/sogsv3/sogsV3SendReaction.ts @@ -3,7 +3,7 @@ import { Data } from '../../../../data/data'; import { ConversationModel } from '../../../../models/conversation'; import { Action, OpenGroupReactionResponse, Reaction } from '../../../../types/Reaction'; import { getEmojiDataFromNative } from '../../../../util/emoji'; -import { handleMessageReaction, hitRateLimit } from '../../../../util/reactions'; +import { Reactions } from '../../../../util/reactions'; import { OnionSending } from '../../../onions/onionSend'; import { UserUtils } from '../../../utils'; import { OpenGroupPollingUtils } from '../opengroupV2/OpenGroupPollingUtils'; @@ -57,7 +57,7 @@ export const sendSogsReactionOnionV4 = async ( return false; } - if (hitRateLimit()) { + if (Reactions.hitRateLimit()) { return false; } @@ -89,7 +89,7 @@ export const sendSogsReactionOnionV4 = async ( // Since responses can take a long time we immediately update the sender's UI and if there is a problem it is overwritten by handleOpenGroupMessageReactions later. const me = UserUtils.getOurPubKeyStrFromCache(); - await handleMessageReaction({ + await Reactions.handleMessageReaction({ reaction, sender: blinded ? getUsBlindedInThatServer(conversation) || me : me, you: true, diff --git a/ts/test/session/unit/reactions/ReactionMessage_test.ts b/ts/test/session/unit/reactions/ReactionMessage_test.ts index d7a7bf127..8c9574b38 100644 --- a/ts/test/session/unit/reactions/ReactionMessage_test.ts +++ b/ts/test/session/unit/reactions/ReactionMessage_test.ts @@ -1,6 +1,6 @@ import chai, { expect } from 'chai'; import Sinon, { useFakeTimers } from 'sinon'; -import { handleMessageReaction, sendMessageReaction } from '../../../../util/reactions'; +import { Reactions } from '../../../../util/reactions'; import { Data } from '../../../../data/data'; import * as Storage from '../../../../util/storage'; import { generateFakeIncomingPrivateMessage, stubWindowLog } from '../../../test-utils/utils'; @@ -40,7 +40,7 @@ describe('ReactionMessage', () => { it('can react to a message', async () => { // Send reaction - const reaction = await sendMessageReaction(originalMessage.get('id'), '😄'); + const reaction = await Reactions.sendMessageReaction(originalMessage.get('id'), '😄'); expect(reaction?.id, 'id should match the original message timestamp').to.be.equal( Number(originalMessage.get('sent_at')) @@ -52,7 +52,7 @@ describe('ReactionMessage', () => { expect(reaction?.action, 'action should be 0').to.be.equal(0); // Handling reaction - const updatedMessage = await handleMessageReaction({ + const updatedMessage = await Reactions.handleMessageReaction({ reaction: reaction as SignalService.DataMessage.IReaction, sender: ourNumber, you: true, @@ -73,7 +73,7 @@ describe('ReactionMessage', () => { it('can remove a reaction from a message', async () => { // Send reaction - const reaction = await sendMessageReaction(originalMessage.get('id'), '😄'); + const reaction = await Reactions.sendMessageReaction(originalMessage.get('id'), '😄'); expect(reaction?.id, 'id should match the original message timestamp').to.be.equal( Number(originalMessage.get('sent_at')) @@ -85,7 +85,7 @@ describe('ReactionMessage', () => { expect(reaction?.action, 'action should be 1').to.be.equal(1); // Handling reaction - const updatedMessage = await handleMessageReaction({ + const updatedMessage = await Reactions.handleMessageReaction({ reaction: reaction as SignalService.DataMessage.IReaction, sender: ourNumber, you: true, @@ -100,10 +100,10 @@ describe('ReactionMessage', () => { // we have already sent 2 messages when this test runs for (let i = 0; i < 18; i++) { // Send reaction - await sendMessageReaction(originalMessage.get('id'), '👍'); + await Reactions.sendMessageReaction(originalMessage.get('id'), '👍'); } - let reaction = await sendMessageReaction(originalMessage.get('id'), '👎'); + let reaction = await Reactions.sendMessageReaction(originalMessage.get('id'), '👎'); expect(reaction, 'no reaction should be returned since we are over the rate limit').to.be .undefined; @@ -113,7 +113,7 @@ describe('ReactionMessage', () => { // Wait a miniute for the rate limit to clear clock.tick(1 * 60 * 1000); - reaction = await sendMessageReaction(originalMessage.get('id'), '👋'); + reaction = await Reactions.sendMessageReaction(originalMessage.get('id'), '👋'); expect(reaction?.id, 'id should match the original message timestamp').to.be.equal( Number(originalMessage.get('sent_at')) diff --git a/ts/test/session/unit/receiver/opengroup/deduplicate_test.ts b/ts/test/session/unit/receiver/opengroup/deduplicate_test.ts index 25c67d821..e7f675e44 100644 --- a/ts/test/session/unit/receiver/opengroup/deduplicate_test.ts +++ b/ts/test/session/unit/receiver/opengroup/deduplicate_test.ts @@ -21,9 +21,9 @@ describe('filterDuplicatesFromDbAndIncoming', () => { }); it('no duplicates', async () => { - const msg1 = TestUtils.generateOpenGroupMessageV2(); - const msg2 = TestUtils.generateOpenGroupMessageV2(); - const msg3 = TestUtils.generateOpenGroupMessageV2(); + const msg1 = TestUtils.generateOpenGroupMessageV2({ serverId: 111 }); + const msg2 = TestUtils.generateOpenGroupMessageV2({ serverId: 222 }); + const msg3 = TestUtils.generateOpenGroupMessageV2({ serverId: 333 }); const filtered = await filterDuplicatesFromDbAndIncoming([msg1, msg2, msg3]); expect(filtered.length).to.be.eq(3); expect(filtered[0]).to.be.deep.eq(msg1); @@ -32,11 +32,11 @@ describe('filterDuplicatesFromDbAndIncoming', () => { }); it('two duplicate sender but not the same timestamp', async () => { - const msg1 = TestUtils.generateOpenGroupMessageV2(); - const msg2 = TestUtils.generateOpenGroupMessageV2(); + const msg1 = TestUtils.generateOpenGroupMessageV2({ serverId: 111 }); + const msg2 = TestUtils.generateOpenGroupMessageV2({ serverId: 222 }); msg2.sender = msg1.sender; msg2.sentTimestamp = Date.now() + 2; - const msg3 = TestUtils.generateOpenGroupMessageV2(); + const msg3 = TestUtils.generateOpenGroupMessageV2({ serverId: 333 }); const filtered = await filterDuplicatesFromDbAndIncoming([msg1, msg2, msg3]); expect(filtered.length).to.be.eq(3); expect(filtered[0]).to.be.deep.eq(msg1); @@ -45,10 +45,10 @@ describe('filterDuplicatesFromDbAndIncoming', () => { }); it('two duplicate timestamp but not the same sender', async () => { - const msg1 = TestUtils.generateOpenGroupMessageV2(); - const msg2 = TestUtils.generateOpenGroupMessageV2(); + const msg1 = TestUtils.generateOpenGroupMessageV2({ serverId: 111 }); + const msg2 = TestUtils.generateOpenGroupMessageV2({ serverId: 222 }); msg2.sentTimestamp = msg1.sentTimestamp; - const msg3 = TestUtils.generateOpenGroupMessageV2(); + const msg3 = TestUtils.generateOpenGroupMessageV2({ serverId: 333 }); const filtered = await filterDuplicatesFromDbAndIncoming([msg1, msg2, msg3]); expect(filtered.length).to.be.eq(3); expect(filtered[0]).to.be.deep.eq(msg1); @@ -57,10 +57,10 @@ describe('filterDuplicatesFromDbAndIncoming', () => { }); it('two duplicate timestamp but not the same sender', async () => { - const msg1 = TestUtils.generateOpenGroupMessageV2(); - const msg2 = TestUtils.generateOpenGroupMessageV2(); + const msg1 = TestUtils.generateOpenGroupMessageV2({ serverId: 111 }); + const msg2 = TestUtils.generateOpenGroupMessageV2({ serverId: 222 }); msg2.sentTimestamp = msg1.sentTimestamp; - const msg3 = TestUtils.generateOpenGroupMessageV2(); + const msg3 = TestUtils.generateOpenGroupMessageV2({ serverId: 333 }); const filtered = await filterDuplicatesFromDbAndIncoming([msg1, msg2, msg3]); expect(filtered.length).to.be.eq(3); expect(filtered[0]).to.be.deep.eq(msg1); @@ -69,11 +69,11 @@ describe('filterDuplicatesFromDbAndIncoming', () => { }); it('two duplicates in the same poll ', async () => { - const msg1 = TestUtils.generateOpenGroupMessageV2(); - const msg2 = TestUtils.generateOpenGroupMessageV2(); + const msg1 = TestUtils.generateOpenGroupMessageV2({ serverId: 111 }); + const msg2 = TestUtils.generateOpenGroupMessageV2({ serverId: msg1.serverId! }); msg2.sentTimestamp = msg1.sentTimestamp; msg2.sender = msg1.sender; - const msg3 = TestUtils.generateOpenGroupMessageV2(); + const msg3 = TestUtils.generateOpenGroupMessageV2({ serverId: 333 }); const filtered = await filterDuplicatesFromDbAndIncoming([msg1, msg2, msg3]); expect(filtered.length).to.be.eq(2); expect(filtered[0]).to.be.deep.eq(msg1); @@ -81,24 +81,10 @@ describe('filterDuplicatesFromDbAndIncoming', () => { }); it('three duplicates in the same poll', async () => { - const msg1 = TestUtils.generateOpenGroupMessageV2(); - const msg2 = TestUtils.generateOpenGroupMessageV2(); + const msg1 = TestUtils.generateOpenGroupMessageV2({ serverId: 111 }); + const msg2 = TestUtils.generateOpenGroupMessageV2({ serverId: msg1.serverId! }); - const msg3 = TestUtils.generateOpenGroupMessageV2(); - msg2.sentTimestamp = msg1.sentTimestamp; - msg2.sender = msg1.sender; - msg3.sentTimestamp = msg1.sentTimestamp; - msg3.sender = msg1.sender; - const filtered = await filterDuplicatesFromDbAndIncoming([msg1, msg2, msg3]); - expect(filtered.length).to.be.eq(1); - expect(filtered[0]).to.be.deep.eq(msg1); - }); - - it('three duplicates in the same poll', async () => { - const msg1 = TestUtils.generateOpenGroupMessageV2(); - const msg2 = TestUtils.generateOpenGroupMessageV2(); - - const msg3 = TestUtils.generateOpenGroupMessageV2(); + const msg3 = TestUtils.generateOpenGroupMessageV2({ serverId: msg1.serverId! }); msg2.sentTimestamp = msg1.sentTimestamp; msg2.sender = msg1.sender; msg3.sentTimestamp = msg1.sentTimestamp; diff --git a/ts/test/session/unit/sending/MessageQueue_test.ts b/ts/test/session/unit/sending/MessageQueue_test.ts index 51892a99b..53a3d609b 100644 --- a/ts/test/session/unit/sending/MessageQueue_test.ts +++ b/ts/test/session/unit/sending/MessageQueue_test.ts @@ -215,7 +215,7 @@ describe('MessageQueue', () => { let sendToOpenGroupV2Stub: sinon.SinonStub; beforeEach(() => { sendToOpenGroupV2Stub = Sinon.stub(MessageSender, 'sendToOpenGroupV2').resolves( - TestUtils.generateOpenGroupMessageV2() + TestUtils.generateOpenGroupMessageV2({ serverId: 5125 }) ); }); diff --git a/ts/test/session/unit/sogsv3/MutationCache_test.ts b/ts/test/session/unit/sogsv3/MutationCache_test.ts new file mode 100644 index 000000000..9eb17250d --- /dev/null +++ b/ts/test/session/unit/sogsv3/MutationCache_test.ts @@ -0,0 +1,142 @@ +import { expect } from 'chai'; +import Sinon from 'sinon'; +import { + addToMutationCache, + ChangeType, + SogsV3Mutation, + updateMutationCache, +} from '../../../../session/apis/open_group_api/sogsv3/sogsV3MutationCache'; +import { Action, Reaction } from '../../../../types/Reaction'; +import { TestUtils } from '../../../test-utils'; +import { Reactions } from '../../../../util/reactions'; + +describe('mutationCache', () => { + TestUtils.stubWindowLog(); + + const roomInfos = TestUtils.generateOpenGroupV2RoomInfos(); + const originalMessage = TestUtils.generateOpenGroupMessageV2({ serverId: 111 }); + const reactor1 = TestUtils.generateFakePubKey().key; + const reactor2 = TestUtils.generateFakePubKey().key; + + const reaction: Reaction = { + id: originalMessage.serverId!, + author: originalMessage.sender!, + emoji: '😄', + action: Action.REACT, + }; + const validEntry: SogsV3Mutation = { + server: roomInfos.serverUrl, + room: roomInfos.roomId, + changeType: ChangeType.REACTIONS, + seqno: null, + metadata: { + messageId: originalMessage.serverId!, + emoji: reaction.emoji, + action: 'ADD', + }, + }; + const invalidEntry: SogsV3Mutation = { + server: '', + room: roomInfos.roomId, + changeType: ChangeType.REACTIONS, + seqno: 100, + metadata: { + messageId: originalMessage.serverId!, + emoji: reaction.emoji, + action: 'ADD', + }, + }; + const messageResponse = TestUtils.generateFakeIncomingOpenGroupMessageV4({ + id: originalMessage.serverId!, + seqno: 200, + reactions: { + '😄': { + index: 0, + count: 1, + you: true, + reactors: [originalMessage.sender!], + }, + '❤️': { + index: 1, + count: 2, + you: true, + reactors: [originalMessage.sender!, reactor1], + }, + '😈': { + index: 0, + count: 2, + you: false, + reactors: [reactor1, reactor2], + }, + }, + }); + + beforeEach(async () => { + // stubs + Sinon.stub(Reactions, 'handleOpenGroupMessageReactions').resolves(); + }); + + afterEach(Sinon.restore); + + describe('add entry to cache', () => { + it('add entry to cache that is valid', async () => { + const cacheState = addToMutationCache(validEntry); + expect(cacheState, 'should not empty').to.not.equal([]); + expect(cacheState.length, 'should have one entry').to.be.equal(1); + expect(cacheState[0], 'the entry should match the input').to.be.deep.equal(validEntry); + }); + it('add entry to cache that is invalid and fail', async () => { + const cacheState = addToMutationCache(invalidEntry); + expect(cacheState, 'should not empty').to.not.equal([]); + expect(cacheState.length, 'should have one entry').to.be.equal(1); + }); + }); + + describe('update entry in cache', () => { + it('update entry in cache with a valid source entry', async () => { + const cacheState = updateMutationCache(validEntry, messageResponse.seqno); + expect(cacheState, 'should not empty').to.not.equal([]); + expect(cacheState.length, 'should have one entry').to.be.equal(1); + expect( + cacheState[0].seqno, + 'should have an entry with a matching seqno to the message response' + ).to.be.equal(messageResponse.seqno); + }); + it('update entry in cache with an invalid source entry', async () => { + const cacheState = updateMutationCache(invalidEntry, messageResponse.seqno); + expect(cacheState, 'should not empty').to.not.equal([]); + expect(cacheState.length, 'should have one entry').to.be.equal(1); + expect( + cacheState[0].seqno, + 'should have an entry with a matching seqno to the message response' + ).to.be.equal(messageResponse.seqno); + }); + it('update entry in cache with a valid source entry but its not stored in the cache', async () => { + const notFoundEntry: SogsV3Mutation = { + server: roomInfos.serverUrl, + room: roomInfos.roomId, + changeType: ChangeType.REACTIONS, + seqno: 400, + metadata: { + messageId: originalMessage.serverId!, + emoji: reaction.emoji, + action: 'ADD', + }, + }; + const cacheState = updateMutationCache(notFoundEntry, messageResponse.seqno); + expect(cacheState, 'should not empty').to.not.equal([]); + expect(cacheState.length, 'should have one entry').to.be.equal(1); + expect( + cacheState[0].seqno, + 'should have an entry with a matching seqno to the message response' + ).to.be.equal(messageResponse.seqno); + }); + }); + + describe('process opengroup messages using the cache', () => { + it('processing a message with valid serverUrl, roomId and message should return an updated message', async () => {}); + it('processing a message with valid serverUrl, roomId and invalid message should return undefined', async () => {}); + it('processing a message with valid entries in the cache should remove them if the cached entry seqno number is less than the message seqo', async () => {}); + it('processing a message with valid entries in the cache should calculate the optimistic state if there is no message seqo or the cached entry seqno is larger than the message seqno', async () => {}); + }); +}); diff --git a/ts/test/test-utils/utils/message.ts b/ts/test/test-utils/utils/message.ts index 922cd7865..f0f5b6518 100644 --- a/ts/test/test-utils/utils/message.ts +++ b/ts/test/test-utils/utils/message.ts @@ -7,6 +7,8 @@ import { TestUtils } from '..'; import { OpenGroupRequestCommonType } from '../../../session/apis/open_group_api/opengroupV2/ApiUtil'; import { OpenGroupVisibleMessage } from '../../../session/messages/outgoing/visibleMessage/OpenGroupVisibleMessage'; import { MessageModel } from '../../../models/message'; +import { OpenGroupMessageV4 } from '../../../session/apis/open_group_api/opengroupV2/OpenGroupServerPoller'; +import { OpenGroupReaction } from '../../../types/Reaction'; export function generateVisibleMessage({ identifier, @@ -27,8 +29,9 @@ export function generateVisibleMessage({ }); } -export function generateOpenGroupMessageV2(): OpenGroupMessageV2 { +export function generateOpenGroupMessageV2({ serverId }: { serverId: number }): OpenGroupMessageV2 { return new OpenGroupMessageV2({ + serverId, sentTimestamp: Date.now(), sender: TestUtils.generateFakePubKey().key, base64EncodedData: 'whatever', @@ -62,3 +65,23 @@ export function generateFakeIncomingPrivateMessage(): MessageModel { type: 'incoming', }); } + +export function generateFakeIncomingOpenGroupMessageV4({ + id, + seqno, + reactions, +}: { + seqno: number; + id: number; + reactions?: Record; +}): OpenGroupMessageV4 { + return { + id, // serverId + seqno, + /** base64 */ + signature: 'whatever', + /** timestamp number with decimal */ + posted: Date.now(), + reactions: reactions ?? {}, + }; +} diff --git a/ts/test/test-utils/utils/stubbing.ts b/ts/test/test-utils/utils/stubbing.ts index 7ec27485f..d465d6f9a 100644 --- a/ts/test/test-utils/utils/stubbing.ts +++ b/ts/test/test-utils/utils/stubbing.ts @@ -68,7 +68,7 @@ export function stubWindow(fn: K, value: WindowValue) }; } -export const enableLogRedirect = false; +export const enableLogRedirect = true; export const stubWindowLog = () => { stubWindow('log', { diff --git a/ts/util/reactions.ts b/ts/util/reactions.ts index 9511c415d..e2569068d 100644 --- a/ts/util/reactions.ts +++ b/ts/util/reactions.ts @@ -11,12 +11,12 @@ import { UserUtils } from '../session/utils'; import { Action, OpenGroupReactionList, ReactionList, RecentReactions } from '../types/Reaction'; import { getRecentReactions, saveRecentReations } from '../util/storage'; -export const SOGSReactorsFetchCount = 5; +const SOGSReactorsFetchCount = 5; const rateCountLimit = 20; const rateTimeLimit = 60 * 1000; const latestReactionTimestamps: Array = []; -export function hitRateLimit(): boolean { +function hitRateLimit(): boolean { const timestamp = Date.now(); latestReactionTimestamps.push(timestamp); @@ -71,7 +71,7 @@ const getMessageByReaction = async ( /** * Sends a Reaction Data Message */ -export const sendMessageReaction = async (messageId: string, emoji: string) => { +const sendMessageReaction = async (messageId: string, emoji: string) => { const found = await Data.getMessageById(messageId); if (found) { const conversationModel = found?.getConversation(); @@ -147,7 +147,7 @@ export const sendMessageReaction = async (messageId: string, emoji: string) => { * Handle reactions on the client by updating the state of the source message * Used in OpenGroups for sending reactions only, not handling responses */ -export const handleMessageReaction = async ({ +const handleMessageReaction = async ({ reaction, sender, you, @@ -239,7 +239,7 @@ export const handleMessageReaction = async ({ * Handles updating the UI when clearing all reactions for a certain emoji * Only usable by moderators in opengroups and runs on their client */ -export const handleClearReaction = async (serverId: number, emoji: string) => { +const handleClearReaction = async (serverId: number, emoji: string) => { const originalMessage = await Data.getMessageByServerId(serverId); if (!originalMessage) { window?.log?.warn(`Cannot find the original reacted message ${serverId}.`); @@ -265,7 +265,7 @@ export const handleClearReaction = async (serverId: number, emoji: string) => { /** * Handles all message reaction updates/responses for opengroups */ -export const handleOpenGroupMessageReactions = async ( +const handleOpenGroupMessageReactions = async ( reactions: OpenGroupReactionList, serverId: number ) => { @@ -334,7 +334,7 @@ export const handleOpenGroupMessageReactions = async ( return originalMessage; }; -export const updateRecentReactions = async (reactions: Array, newReaction: string) => { +const updateRecentReactions = async (reactions: Array, newReaction: string) => { window?.log?.info('updating recent reactions with', newReaction); const recentReactions = new RecentReactions(reactions); const foundIndex = recentReactions.items.indexOf(newReaction); @@ -348,3 +348,14 @@ export const updateRecentReactions = async (reactions: Array, newReactio } await saveRecentReations(recentReactions.items); }; + +// exported for testing purposes +export const Reactions = { + SOGSReactorsFetchCount, + hitRateLimit, + sendMessageReaction, + handleMessageReaction, + handleClearReaction, + handleOpenGroupMessageReactions, + updateRecentReactions, +}; From 7c6af173275e8e19c6774968b4abbe356ef9c807 Mon Sep 17 00:00:00 2001 From: William Grant Date: Tue, 30 Aug 2022 17:26:15 +1000 Subject: [PATCH 09/11] fix: dont log removing entries from cache when there are none --- ts/session/apis/open_group_api/sogsv3/sogsV3MutationCache.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/ts/session/apis/open_group_api/sogsv3/sogsV3MutationCache.ts b/ts/session/apis/open_group_api/sogsv3/sogsV3MutationCache.ts index 9874830bc..6790b6e8e 100644 --- a/ts/session/apis/open_group_api/sogsv3/sogsV3MutationCache.ts +++ b/ts/session/apis/open_group_api/sogsv3/sogsV3MutationCache.ts @@ -131,7 +131,9 @@ export async function processMessagesUsingCache( } const removedMatches = remove(sogsMutationCache, ...roomMatches); - window.log.info('SOGS Mutation Cache: Removed processed entries from cache!', removedMatches); + if (removedMatches?.length) { + window.log.info('SOGS Mutation Cache: Removed processed entries from cache!', removedMatches); + } message.reactions = updatedReactions; await Reactions.handleOpenGroupMessageReactions(message.reactions, message.id); From f138ea31b21ae0386ca85683d53943dd2f021a6e Mon Sep 17 00:00:00 2001 From: William Grant Date: Wed, 31 Aug 2022 16:02:15 +1000 Subject: [PATCH 10/11] test: finished writing tests for sogs mutation cache refactored processMessagesWithCachce function --- .../opengroupV2/OpenGroupServerPoller.ts | 5 + .../sogsv3/sogsV3MutationCache.ts | 111 +++-- .../receiver/opengroup/deduplicate_test.ts | 36 +- .../session/unit/sending/MessageQueue_test.ts | 2 +- .../session/unit/sogsv3/MutationCache_test.ts | 397 ++++++++++++++---- ts/test/test-utils/utils/message.ts | 33 +- ts/test/test-utils/utils/stubbing.ts | 2 +- 7 files changed, 415 insertions(+), 171 deletions(-) diff --git a/ts/session/apis/open_group_api/opengroupV2/OpenGroupServerPoller.ts b/ts/session/apis/open_group_api/opengroupV2/OpenGroupServerPoller.ts index 1f34bb390..8c2eae034 100644 --- a/ts/session/apis/open_group_api/opengroupV2/OpenGroupServerPoller.ts +++ b/ts/session/apis/open_group_api/opengroupV2/OpenGroupServerPoller.ts @@ -35,6 +35,11 @@ export type OpenGroupMessageV4 = { reactions: Record; }; +// seqno is not set for SOGS < 1.3.4 +export type OpenGroupReactionMessageV4 = Omit & { + seqno: number | undefined; +}; + const pollForEverythingInterval = DURATION.SECONDS * 10; export const invalidAuthRequiresBlinding = diff --git a/ts/session/apis/open_group_api/sogsv3/sogsV3MutationCache.ts b/ts/session/apis/open_group_api/sogsv3/sogsV3MutationCache.ts index 6790b6e8e..547147e28 100644 --- a/ts/session/apis/open_group_api/sogsv3/sogsV3MutationCache.ts +++ b/ts/session/apis/open_group_api/sogsv3/sogsV3MutationCache.ts @@ -5,7 +5,7 @@ import { filter, findIndex, remove } from 'lodash'; import { Reactions } from '../../../../util/reactions'; -import { OpenGroupMessageV4 } from '../opengroupV2/OpenGroupServerPoller'; +import { OpenGroupReactionMessageV4 } from '../opengroupV2/OpenGroupServerPoller'; export enum ChangeType { REACTIONS = 0, @@ -30,6 +30,11 @@ export type SogsV3Mutation = { // we don't want to export this, we want to export functions that manipulate it const sogsMutationCache: Array = []; +// for testing purposes only +export function getMutationCache() { + return sogsMutationCache; +} + function verifyEntry(entry: SogsV3Mutation): boolean { return Boolean( entry.server && @@ -46,19 +51,16 @@ function verifyEntry(entry: SogsV3Mutation): boolean { ); } -// we return the cache for testing -export function addToMutationCache(entry: SogsV3Mutation): Array { +export function addToMutationCache(entry: SogsV3Mutation) { if (!verifyEntry(entry)) { window.log.error('SOGS Mutation Cache: Entry verification on add failed!', entry); } else { sogsMutationCache.push(entry); window.log.info('SOGS Mutation Cache: Entry added!', entry); } - return sogsMutationCache; } -// we return the cache for testing -export function updateMutationCache(entry: SogsV3Mutation, seqno: number): Array { +export function updateMutationCache(entry: SogsV3Mutation, seqno: number) { if (!verifyEntry(entry)) { window.log.error('SOGS Mutation Cache: Entry verification on update failed!', entry); } else { @@ -70,69 +72,64 @@ export function updateMutationCache(entry: SogsV3Mutation, seqno: number): Array window.log.error('SOGS Mutation Cache: Updated failed! Cannot find entry', entry); } } - return sogsMutationCache; } // return is for testing purposes only export async function processMessagesUsingCache( server: string, room: string, - message: OpenGroupMessageV4 -): Promise { + message: OpenGroupReactionMessageV4 +): Promise { const updatedReactions = message.reactions; - const roomMatches: Array = filter(sogsMutationCache, { server, room }); - for (const roomMatch of roomMatches) { - if (message.seqno && roomMatch.seqno && roomMatch.seqno <= message.seqno) { - const removedEntry = remove(sogsMutationCache, roomMatch); + const roomMatches: Array = filter(sogsMutationCache, { server, room }); + for (let i = 0; i < roomMatches.length; i++) { + const matchSeqno = roomMatches[i].seqno; + if (message.seqno && matchSeqno && matchSeqno <= message.seqno) { + const removedEntry = roomMatches.splice(i, 1)[0]; window.log.info('SOGS Mutation Cache: Entry ignored and removed!', removedEntry); - } else if ( - !message.seqno || - (message.seqno && roomMatch.seqno && roomMatch.seqno > message.seqno) - ) { - for (const reaction of Object.keys(message.reactions)) { - const reactionMatches = filter(sogsMutationCache, { - server, - room, - changeType: ChangeType.REACTIONS, - metadata: { - messageId: message.id, - emoji: reaction, - }, - }); - - for (const reactionMatch of reactionMatches) { - switch (reactionMatch.metadata.action) { - case 'ADD': - updatedReactions[reaction].you = true; - updatedReactions[reaction].count += 1; - window.log.info( - 'SOGS Mutation Cache: Added our reaction based on the cache', - updatedReactions[reaction] - ); - break; - case 'REMOVE': - updatedReactions[reaction].you = false; - updatedReactions[reaction].count -= 1; - window.log.info( - 'SOGS Mutation Cache: Removed our reaction based on the cache', - updatedReactions[reaction] - ); - break; - default: - window.log.warn( - 'SOGS Mutation Cache: Unsupported metadata action in OpenGroupMessageV4', - reactionMatch - ); - } - } - } + remove(sogsMutationCache, removedEntry); } } - const removedMatches = remove(sogsMutationCache, ...roomMatches); - if (removedMatches?.length) { - window.log.info('SOGS Mutation Cache: Removed processed entries from cache!', removedMatches); + for (const reaction of Object.keys(message.reactions)) { + const reactionMatches = filter(roomMatches, { + server, + room, + changeType: ChangeType.REACTIONS, + metadata: { + messageId: message.id, + emoji: reaction, + }, + }); + + for (const reactionMatch of reactionMatches) { + switch (reactionMatch.metadata.action) { + case 'ADD': + updatedReactions[reaction].you = true; + updatedReactions[reaction].count += 1; + window.log.info( + 'SOGS Mutation Cache: Added our reaction based on the cache', + updatedReactions[reaction] + ); + break; + case 'REMOVE': + updatedReactions[reaction].you = false; + updatedReactions[reaction].count -= 1; + window.log.info( + 'SOGS Mutation Cache: Removed our reaction based on the cache', + updatedReactions[reaction] + ); + break; + default: + window.log.warn( + 'SOGS Mutation Cache: Unsupported metadata action in OpenGroupMessageV4', + reactionMatch + ); + } + const removedEntry = remove(sogsMutationCache, reactionMatch); + window.log.info('SOGS Mutation Cache: Entry removed!', removedEntry); + } } message.reactions = updatedReactions; diff --git a/ts/test/session/unit/receiver/opengroup/deduplicate_test.ts b/ts/test/session/unit/receiver/opengroup/deduplicate_test.ts index e7f675e44..0d73db702 100644 --- a/ts/test/session/unit/receiver/opengroup/deduplicate_test.ts +++ b/ts/test/session/unit/receiver/opengroup/deduplicate_test.ts @@ -21,9 +21,9 @@ describe('filterDuplicatesFromDbAndIncoming', () => { }); it('no duplicates', async () => { - const msg1 = TestUtils.generateOpenGroupMessageV2({ serverId: 111 }); - const msg2 = TestUtils.generateOpenGroupMessageV2({ serverId: 222 }); - const msg3 = TestUtils.generateOpenGroupMessageV2({ serverId: 333 }); + const msg1 = TestUtils.generateOpenGroupMessageV2(); + const msg2 = TestUtils.generateOpenGroupMessageV2(); + const msg3 = TestUtils.generateOpenGroupMessageV2(); const filtered = await filterDuplicatesFromDbAndIncoming([msg1, msg2, msg3]); expect(filtered.length).to.be.eq(3); expect(filtered[0]).to.be.deep.eq(msg1); @@ -32,11 +32,11 @@ describe('filterDuplicatesFromDbAndIncoming', () => { }); it('two duplicate sender but not the same timestamp', async () => { - const msg1 = TestUtils.generateOpenGroupMessageV2({ serverId: 111 }); - const msg2 = TestUtils.generateOpenGroupMessageV2({ serverId: 222 }); + const msg1 = TestUtils.generateOpenGroupMessageV2(); + const msg2 = TestUtils.generateOpenGroupMessageV2(); msg2.sender = msg1.sender; msg2.sentTimestamp = Date.now() + 2; - const msg3 = TestUtils.generateOpenGroupMessageV2({ serverId: 333 }); + const msg3 = TestUtils.generateOpenGroupMessageV2(); const filtered = await filterDuplicatesFromDbAndIncoming([msg1, msg2, msg3]); expect(filtered.length).to.be.eq(3); expect(filtered[0]).to.be.deep.eq(msg1); @@ -45,10 +45,10 @@ describe('filterDuplicatesFromDbAndIncoming', () => { }); it('two duplicate timestamp but not the same sender', async () => { - const msg1 = TestUtils.generateOpenGroupMessageV2({ serverId: 111 }); - const msg2 = TestUtils.generateOpenGroupMessageV2({ serverId: 222 }); + const msg1 = TestUtils.generateOpenGroupMessageV2(); + const msg2 = TestUtils.generateOpenGroupMessageV2(); msg2.sentTimestamp = msg1.sentTimestamp; - const msg3 = TestUtils.generateOpenGroupMessageV2({ serverId: 333 }); + const msg3 = TestUtils.generateOpenGroupMessageV2(); const filtered = await filterDuplicatesFromDbAndIncoming([msg1, msg2, msg3]); expect(filtered.length).to.be.eq(3); expect(filtered[0]).to.be.deep.eq(msg1); @@ -57,10 +57,10 @@ describe('filterDuplicatesFromDbAndIncoming', () => { }); it('two duplicate timestamp but not the same sender', async () => { - const msg1 = TestUtils.generateOpenGroupMessageV2({ serverId: 111 }); - const msg2 = TestUtils.generateOpenGroupMessageV2({ serverId: 222 }); + const msg1 = TestUtils.generateOpenGroupMessageV2(); + const msg2 = TestUtils.generateOpenGroupMessageV2(); msg2.sentTimestamp = msg1.sentTimestamp; - const msg3 = TestUtils.generateOpenGroupMessageV2({ serverId: 333 }); + const msg3 = TestUtils.generateOpenGroupMessageV2(); const filtered = await filterDuplicatesFromDbAndIncoming([msg1, msg2, msg3]); expect(filtered.length).to.be.eq(3); expect(filtered[0]).to.be.deep.eq(msg1); @@ -69,11 +69,11 @@ describe('filterDuplicatesFromDbAndIncoming', () => { }); it('two duplicates in the same poll ', async () => { - const msg1 = TestUtils.generateOpenGroupMessageV2({ serverId: 111 }); - const msg2 = TestUtils.generateOpenGroupMessageV2({ serverId: msg1.serverId! }); + const msg1 = TestUtils.generateOpenGroupMessageV2(); + const msg2 = TestUtils.generateOpenGroupMessageV2(); msg2.sentTimestamp = msg1.sentTimestamp; msg2.sender = msg1.sender; - const msg3 = TestUtils.generateOpenGroupMessageV2({ serverId: 333 }); + const msg3 = TestUtils.generateOpenGroupMessageV2(); const filtered = await filterDuplicatesFromDbAndIncoming([msg1, msg2, msg3]); expect(filtered.length).to.be.eq(2); expect(filtered[0]).to.be.deep.eq(msg1); @@ -81,10 +81,10 @@ describe('filterDuplicatesFromDbAndIncoming', () => { }); it('three duplicates in the same poll', async () => { - const msg1 = TestUtils.generateOpenGroupMessageV2({ serverId: 111 }); - const msg2 = TestUtils.generateOpenGroupMessageV2({ serverId: msg1.serverId! }); + const msg1 = TestUtils.generateOpenGroupMessageV2(); + const msg2 = TestUtils.generateOpenGroupMessageV2(); - const msg3 = TestUtils.generateOpenGroupMessageV2({ serverId: msg1.serverId! }); + const msg3 = TestUtils.generateOpenGroupMessageV2(); msg2.sentTimestamp = msg1.sentTimestamp; msg2.sender = msg1.sender; msg3.sentTimestamp = msg1.sentTimestamp; diff --git a/ts/test/session/unit/sending/MessageQueue_test.ts b/ts/test/session/unit/sending/MessageQueue_test.ts index 53a3d609b..51892a99b 100644 --- a/ts/test/session/unit/sending/MessageQueue_test.ts +++ b/ts/test/session/unit/sending/MessageQueue_test.ts @@ -215,7 +215,7 @@ describe('MessageQueue', () => { let sendToOpenGroupV2Stub: sinon.SinonStub; beforeEach(() => { sendToOpenGroupV2Stub = Sinon.stub(MessageSender, 'sendToOpenGroupV2').resolves( - TestUtils.generateOpenGroupMessageV2({ serverId: 5125 }) + TestUtils.generateOpenGroupMessageV2() ); }); diff --git a/ts/test/session/unit/sogsv3/MutationCache_test.ts b/ts/test/session/unit/sogsv3/MutationCache_test.ts index 9eb17250d..eae02332e 100644 --- a/ts/test/session/unit/sogsv3/MutationCache_test.ts +++ b/ts/test/session/unit/sogsv3/MutationCache_test.ts @@ -3,75 +3,29 @@ import Sinon from 'sinon'; import { addToMutationCache, ChangeType, + getMutationCache, + processMessagesUsingCache, SogsV3Mutation, updateMutationCache, } from '../../../../session/apis/open_group_api/sogsv3/sogsV3MutationCache'; -import { Action, Reaction } from '../../../../types/Reaction'; import { TestUtils } from '../../../test-utils'; import { Reactions } from '../../../../util/reactions'; +import { + OpenGroupMessageV4, + OpenGroupReactionMessageV4, +} from '../../../../session/apis/open_group_api/opengroupV2/OpenGroupServerPoller'; +// tslint:disable: chai-vague-errors describe('mutationCache', () => { TestUtils.stubWindowLog(); const roomInfos = TestUtils.generateOpenGroupV2RoomInfos(); - const originalMessage = TestUtils.generateOpenGroupMessageV2({ serverId: 111 }); + const originalMessage = TestUtils.generateOpenGroupMessageV2WithServerId(111); + const originalMessage2 = TestUtils.generateOpenGroupMessageV2WithServerId(112); const reactor1 = TestUtils.generateFakePubKey().key; const reactor2 = TestUtils.generateFakePubKey().key; - const reaction: Reaction = { - id: originalMessage.serverId!, - author: originalMessage.sender!, - emoji: '😄', - action: Action.REACT, - }; - const validEntry: SogsV3Mutation = { - server: roomInfos.serverUrl, - room: roomInfos.roomId, - changeType: ChangeType.REACTIONS, - seqno: null, - metadata: { - messageId: originalMessage.serverId!, - emoji: reaction.emoji, - action: 'ADD', - }, - }; - const invalidEntry: SogsV3Mutation = { - server: '', - room: roomInfos.roomId, - changeType: ChangeType.REACTIONS, - seqno: 100, - metadata: { - messageId: originalMessage.serverId!, - emoji: reaction.emoji, - action: 'ADD', - }, - }; - const messageResponse = TestUtils.generateFakeIncomingOpenGroupMessageV4({ - id: originalMessage.serverId!, - seqno: 200, - reactions: { - '😄': { - index: 0, - count: 1, - you: true, - reactors: [originalMessage.sender!], - }, - '❤️': { - index: 1, - count: 2, - you: true, - reactors: [originalMessage.sender!, reactor1], - }, - '😈': { - index: 0, - count: 2, - you: false, - reactors: [reactor1, reactor2], - }, - }, - }); - - beforeEach(async () => { + beforeEach(() => { // stubs Sinon.stub(Reactions, 'handleOpenGroupMessageReactions').resolves(); }); @@ -79,64 +33,333 @@ describe('mutationCache', () => { afterEach(Sinon.restore); describe('add entry to cache', () => { - it('add entry to cache that is valid', async () => { - const cacheState = addToMutationCache(validEntry); - expect(cacheState, 'should not empty').to.not.equal([]); - expect(cacheState.length, 'should have one entry').to.be.equal(1); - expect(cacheState[0], 'the entry should match the input').to.be.deep.equal(validEntry); + it('add entry to cache that is valid', () => { + const entry: SogsV3Mutation = { + server: roomInfos.serverUrl, + room: roomInfos.roomId, + changeType: ChangeType.REACTIONS, + seqno: null, + metadata: { + messageId: originalMessage.serverId, + emoji: '😄', + action: 'ADD', + }, + }; + addToMutationCache(entry); + const cache = getMutationCache(); + expect(cache, 'should not empty').to.not.equal([]); + expect(cache.length, 'should have one entry').to.be.equal(1); + expect(cache[0], 'the entry should match the input').to.be.deep.equal(entry); }); - it('add entry to cache that is invalid and fail', async () => { - const cacheState = addToMutationCache(invalidEntry); - expect(cacheState, 'should not empty').to.not.equal([]); - expect(cacheState.length, 'should have one entry').to.be.equal(1); + it('add entry to cache that is invalid and fail', () => { + const entry: SogsV3Mutation = { + server: '', // this is invalid + room: roomInfos.roomId, + changeType: ChangeType.REACTIONS, + seqno: 100, + metadata: { + messageId: originalMessage.serverId, + emoji: '😄', + action: 'ADD', + }, + }; + addToMutationCache(entry); + const cache = getMutationCache(); + expect(cache, 'should not empty').to.not.equal([]); + expect(cache.length, 'should have one entry').to.be.equal(1); }); }); describe('update entry in cache', () => { - it('update entry in cache with a valid source entry', async () => { - const cacheState = updateMutationCache(validEntry, messageResponse.seqno); - expect(cacheState, 'should not empty').to.not.equal([]); - expect(cacheState.length, 'should have one entry').to.be.equal(1); + it('update entry in cache with a valid source entry', () => { + const entry: SogsV3Mutation = { + server: roomInfos.serverUrl, + room: roomInfos.roomId, + changeType: ChangeType.REACTIONS, + seqno: null, // mutation before we have received a response + metadata: { + messageId: originalMessage.serverId, + emoji: '😄', + action: 'ADD', + }, + }; + const messageResponse = TestUtils.generateFakeIncomingOpenGroupMessageV4({ + id: originalMessage.serverId, + seqno: 200, + reactions: { + '😄': { + index: 0, + count: 1, + you: false, + reactors: [reactor1], + }, + '❤️': { + index: 1, + count: 2, + you: true, + reactors: [originalMessage.sender, reactor1], + }, + '😈': { + index: 2, + count: 2, + you: true, + reactors: [originalMessage.sender, reactor2], + }, + }, + }) as OpenGroupMessageV4; + updateMutationCache(entry, (messageResponse as OpenGroupMessageV4).seqno); + const cache = getMutationCache(); + expect(cache, 'should not empty').to.not.equal([]); + expect(cache.length, 'should have one entry').to.be.equal(1); expect( - cacheState[0].seqno, + cache[0].seqno, 'should have an entry with a matching seqno to the message response' ).to.be.equal(messageResponse.seqno); }); - it('update entry in cache with an invalid source entry', async () => { - const cacheState = updateMutationCache(invalidEntry, messageResponse.seqno); - expect(cacheState, 'should not empty').to.not.equal([]); - expect(cacheState.length, 'should have one entry').to.be.equal(1); + it('update entry in cache with an invalid source entry', () => { + const messageResponse = TestUtils.generateFakeIncomingOpenGroupMessageV4({ + id: originalMessage.serverId, + seqno: 200, + reactions: { + '😄': { + index: 0, + count: 1, + you: false, + reactors: [reactor1], + }, + '❤️': { + index: 1, + count: 2, + you: true, + reactors: [originalMessage.sender, reactor1], + }, + '😈': { + index: 2, + count: 2, + you: true, + reactors: [originalMessage.sender, reactor2], + }, + }, + }) as OpenGroupMessageV4; + const entry: SogsV3Mutation = { + server: '', + room: roomInfos.roomId, + changeType: ChangeType.REACTIONS, + seqno: 100, + metadata: { + messageId: originalMessage.serverId, + emoji: '😄', + action: 'ADD', + }, + }; + updateMutationCache(entry, (messageResponse as OpenGroupMessageV4).seqno); + const cache = getMutationCache(); + expect(cache, 'should not empty').to.not.equal([]); + expect(cache.length, 'should have one entry').to.be.equal(1); expect( - cacheState[0].seqno, + cache[0].seqno, 'should have an entry with a matching seqno to the message response' ).to.be.equal(messageResponse.seqno); }); - it('update entry in cache with a valid source entry but its not stored in the cache', async () => { - const notFoundEntry: SogsV3Mutation = { + it('update entry in cache with a valid source entry but its not stored in the cache', () => { + const messageResponse = TestUtils.generateFakeIncomingOpenGroupMessageV4({ + id: originalMessage.serverId, + seqno: 200, + reactions: { + '😄': { + index: 0, + count: 1, + you: false, + reactors: [reactor1], + }, + '❤️': { + index: 1, + count: 2, + you: true, + reactors: [originalMessage.sender, reactor1], + }, + '😈': { + index: 2, + count: 2, + you: true, + reactors: [originalMessage.sender, reactor2], + }, + }, + }) as OpenGroupMessageV4; + const entry: SogsV3Mutation = { server: roomInfos.serverUrl, room: roomInfos.roomId, changeType: ChangeType.REACTIONS, seqno: 400, metadata: { - messageId: originalMessage.serverId!, - emoji: reaction.emoji, + messageId: originalMessage.serverId, + emoji: '😄', action: 'ADD', }, }; - const cacheState = updateMutationCache(notFoundEntry, messageResponse.seqno); - expect(cacheState, 'should not empty').to.not.equal([]); - expect(cacheState.length, 'should have one entry').to.be.equal(1); + updateMutationCache(entry, (messageResponse as OpenGroupMessageV4).seqno); + const cache = getMutationCache(); + expect(cache, 'should not empty').to.not.equal([]); + expect(cache.length, 'should have one entry').to.be.equal(1); expect( - cacheState[0].seqno, + cache[0].seqno, 'should have an entry with a matching seqno to the message response' ).to.be.equal(messageResponse.seqno); }); }); describe('process opengroup messages using the cache', () => { - it('processing a message with valid serverUrl, roomId and message should return an updated message', async () => {}); - it('processing a message with valid serverUrl, roomId and invalid message should return undefined', async () => {}); - it('processing a message with valid entries in the cache should remove them if the cached entry seqno number is less than the message seqo', async () => {}); - it('processing a message with valid entries in the cache should calculate the optimistic state if there is no message seqo or the cached entry seqno is larger than the message seqno', async () => {}); + it('processing a message with valid serverUrl, roomId and message should return the same message response', async () => { + const messageResponse = TestUtils.generateFakeIncomingOpenGroupMessageV4({ + id: originalMessage.serverId, + seqno: 200, + reactions: { + '😄': { + index: 0, + count: 1, + you: false, + reactors: [reactor1], + }, + '❤️': { + index: 1, + count: 2, + you: true, + reactors: [originalMessage.sender, reactor1], + }, + '😈': { + index: 2, + count: 2, + you: true, + reactors: [originalMessage.sender, reactor2], + }, + }, + }) as OpenGroupMessageV4; + const message = await processMessagesUsingCache( + roomInfos.serverUrl, + roomInfos.roomId, + messageResponse + ); + const cache = getMutationCache(); + expect(cache, 'cache should be empty').to.be.empty; + expect(message, 'message response should match').to.be.deep.equal(messageResponse); + }); + it('processing a message with valid serverUrl, roomId and message (from SOGS < 1.3.4) should return the same message response', async () => { + const messageResponse = TestUtils.generateFakeIncomingOpenGroupMessageV4({ + id: originalMessage2.serverId, + // in version less than 1.3.4 there is no a seqno set + reactions: { + '🤣': { + index: 0, + count: 3, + you: true, + reactors: [reactor1, reactor2, originalMessage2.sender], + }, + '😈': { + index: 0, + count: 1, + you: false, + reactors: [reactor2], + }, + }, + }) as OpenGroupReactionMessageV4; + const message = await processMessagesUsingCache( + roomInfos.serverUrl, + roomInfos.roomId, + messageResponse + ); + const cache = getMutationCache(); + expect(cache, 'cache should be empty').to.be.empty; + expect(message, 'message response should match').to.be.deep.equal(messageResponse); + }); + it('processing a message with valid entries in the cache should calculate the optimistic state if there is no message seqo or the cached entry seqno is larger than the message seqno', async () => { + const messageResponse = TestUtils.generateFakeIncomingOpenGroupMessageV4({ + id: originalMessage.serverId, + seqno: 200, + reactions: { + '😄': { + index: 0, + count: 1, + you: false, + reactors: [reactor1], + }, + '❤️': { + index: 1, + count: 2, + you: true, + reactors: [originalMessage.sender, reactor1], + }, + '😈': { + index: 2, + count: 2, + you: true, + reactors: [originalMessage.sender, reactor2], + }, + }, + }) as OpenGroupMessageV4; + const entry: SogsV3Mutation = { + server: roomInfos.serverUrl, + room: roomInfos.roomId, + changeType: ChangeType.REACTIONS, + seqno: 100, // less than response messageResponse seqno should be ignored + metadata: { + messageId: originalMessage.serverId, + emoji: '❤️', + action: 'ADD', + }, + }; + const entry2: SogsV3Mutation = { + server: roomInfos.serverUrl, + room: roomInfos.roomId, + changeType: ChangeType.REACTIONS, + seqno: 300, // greater than response messageResponse seqno should be procesed + metadata: { + messageId: originalMessage.serverId, + emoji: '😄', + action: 'ADD', + }, + }; + const entry3: SogsV3Mutation = { + server: roomInfos.serverUrl, + room: roomInfos.roomId, + changeType: ChangeType.REACTIONS, + seqno: 301, //// greater than response messageResponse seqno should be procesed + metadata: { + messageId: originalMessage.serverId, + emoji: '😈', + action: 'REMOVE', + }, + }; + addToMutationCache(entry); + addToMutationCache(entry2); + addToMutationCache(entry3); + + const message = await processMessagesUsingCache( + roomInfos.serverUrl, + roomInfos.roomId, + messageResponse + ); + const cache = getMutationCache(); + expect(cache, 'cache should be empty').to.be.empty; + expect( + message.reactions['❤️'].count, + 'message response reaction count for ❤️ should be unchanged with 2' + ).to.equal(2); + expect( + message.reactions['😄'].count, + 'message response reaction count for 😄 should be 2' + ).to.equal(2); + expect( + message.reactions['😄'].you, + 'message response reaction for 😄 should have you = true' + ).to.equal(true); + expect( + message.reactions['😈'].count, + 'message response reaction count for 😈 should be 1' + ).to.equal(1); + expect( + message.reactions['😈'].you, + 'message response reaction for 😈 should have you = false' + ).to.equal(false); + }); }); }); diff --git a/ts/test/test-utils/utils/message.ts b/ts/test/test-utils/utils/message.ts index f0f5b6518..1f511a8fc 100644 --- a/ts/test/test-utils/utils/message.ts +++ b/ts/test/test-utils/utils/message.ts @@ -7,7 +7,10 @@ import { TestUtils } from '..'; import { OpenGroupRequestCommonType } from '../../../session/apis/open_group_api/opengroupV2/ApiUtil'; import { OpenGroupVisibleMessage } from '../../../session/messages/outgoing/visibleMessage/OpenGroupVisibleMessage'; import { MessageModel } from '../../../models/message'; -import { OpenGroupMessageV4 } from '../../../session/apis/open_group_api/opengroupV2/OpenGroupServerPoller'; +import { + OpenGroupMessageV4, + OpenGroupReactionMessageV4, +} from '../../../session/apis/open_group_api/opengroupV2/OpenGroupServerPoller'; import { OpenGroupReaction } from '../../../types/Reaction'; export function generateVisibleMessage({ @@ -29,15 +32,31 @@ export function generateVisibleMessage({ }); } -export function generateOpenGroupMessageV2({ serverId }: { serverId: number }): OpenGroupMessageV2 { +export function generateOpenGroupMessageV2(): OpenGroupMessageV2 { return new OpenGroupMessageV2({ - serverId, sentTimestamp: Date.now(), sender: TestUtils.generateFakePubKey().key, base64EncodedData: 'whatever', }); } +// this is for test purposes only +type OpenGroupMessageV2WithServerId = Omit & { + sender: string; + serverId: number; +}; + +export function generateOpenGroupMessageV2WithServerId( + serverId: number +): OpenGroupMessageV2WithServerId { + return new OpenGroupMessageV2({ + serverId, + sentTimestamp: Date.now(), + sender: TestUtils.generateFakePubKey().key, + base64EncodedData: 'whatever', + }) as OpenGroupMessageV2WithServerId; +} + export function generateOpenGroupVisibleMessage(): OpenGroupVisibleMessage { return new OpenGroupVisibleMessage({ timestamp: Date.now(), @@ -68,16 +87,16 @@ export function generateFakeIncomingPrivateMessage(): MessageModel { export function generateFakeIncomingOpenGroupMessageV4({ id, - seqno, reactions, + seqno, }: { - seqno: number; id: number; + seqno?: number; reactions?: Record; -}): OpenGroupMessageV4 { +}): OpenGroupMessageV4 | OpenGroupReactionMessageV4 { return { id, // serverId - seqno, + seqno: seqno ?? undefined, /** base64 */ signature: 'whatever', /** timestamp number with decimal */ diff --git a/ts/test/test-utils/utils/stubbing.ts b/ts/test/test-utils/utils/stubbing.ts index d465d6f9a..7ec27485f 100644 --- a/ts/test/test-utils/utils/stubbing.ts +++ b/ts/test/test-utils/utils/stubbing.ts @@ -68,7 +68,7 @@ export function stubWindow(fn: K, value: WindowValue) }; } -export const enableLogRedirect = true; +export const enableLogRedirect = false; export const stubWindowLog = () => { stubWindow('log', { From 58e4b4e8961780784a16bf58de6fea727bc85536 Mon Sep 17 00:00:00 2001 From: William Grant Date: Wed, 31 Aug 2022 16:33:57 +1000 Subject: [PATCH 11/11] fix: make sure to clear a reaction is the count is 0 on an opengroup --- .../sogsv3/sogsV3MutationCache.ts | 3 --- ts/util/reactions.ts | 17 +++++++++++------ 2 files changed, 11 insertions(+), 9 deletions(-) diff --git a/ts/session/apis/open_group_api/sogsv3/sogsV3MutationCache.ts b/ts/session/apis/open_group_api/sogsv3/sogsV3MutationCache.ts index 547147e28..3a780937f 100644 --- a/ts/session/apis/open_group_api/sogsv3/sogsV3MutationCache.ts +++ b/ts/session/apis/open_group_api/sogsv3/sogsV3MutationCache.ts @@ -38,13 +38,10 @@ export function getMutationCache() { function verifyEntry(entry: SogsV3Mutation): boolean { return Boolean( entry.server && - entry.server !== '' && entry.room && - entry.room !== '' && entry.changeType === ChangeType.REACTIONS && entry.metadata.messageId && entry.metadata.emoji && - entry.metadata.emoji !== '' && (entry.metadata.action === 'ADD' || entry.metadata.action === 'REMOVE' || entry.metadata.action === 'CLEAR') diff --git a/ts/util/reactions.ts b/ts/util/reactions.ts index e2569068d..3193ba5d3 100644 --- a/ts/util/reactions.ts +++ b/ts/util/reactions.ts @@ -317,12 +317,17 @@ const handleOpenGroupMessageReactions = async ( senders.push(reactor); }); - reacts[emoji] = { - count: reactions[key].count, - index: reactions[key].index, - senders, - you, - }; + if (reactions[key].count > 0) { + reacts[emoji] = { + count: reactions[key].count, + index: reactions[key].index, + senders, + you, + }; + } else { + // tslint:disable-next-line: no-dynamic-delete + delete reacts[key]; + } }); originalMessage.set({