From 6d576ed901f6ad86942bd7cf90bbd38b1ff3d1e2 Mon Sep 17 00:00:00 2001 From: Josh Perez <60019601+josh-signal@users.noreply.github.com> Date: Thu, 28 Apr 2022 18:06:28 -0400 Subject: [PATCH] Allow multiple reactions to stories --- ts/components/StoryListItem.tsx | 1 - ts/jobs/helpers/sendReaction.ts | 3 +- ts/models/messages.ts | 36 ++++++---- ts/reactions/util.ts | 30 +++++++-- ts/services/storyLoader.ts | 14 +--- ts/state/ducks/stories.ts | 56 +++++----------- ts/state/selectors/stories.ts | 100 ++++++++++++++++++++-------- ts/state/smart/StoryViewer.tsx | 4 +- ts/test-both/reactions/util_test.ts | 86 ++++++++++++++++++------ 9 files changed, 211 insertions(+), 119 deletions(-) diff --git a/ts/components/StoryListItem.tsx b/ts/components/StoryListItem.tsx index cd38955d1..b623df37e 100644 --- a/ts/components/StoryListItem.tsx +++ b/ts/components/StoryListItem.tsx @@ -40,7 +40,6 @@ export type StoryViewType = { isHidden?: boolean; isUnread?: boolean; messageId: string; - selectedReaction?: string; sender: Pick< ConversationType, | 'acceptedMessageRequest' diff --git a/ts/jobs/helpers/sendReaction.ts b/ts/jobs/helpers/sendReaction.ts index bcc3155bf..9d3b5432a 100644 --- a/ts/jobs/helpers/sendReaction.ts +++ b/ts/jobs/helpers/sendReaction.ts @@ -313,7 +313,8 @@ export async function sendReaction( const newReactions = reactionUtil.markOutgoingReactionSent( getReactions(message), pendingReaction, - successfulConversationIds + successfulConversationIds, + message.attributes ); setReactions(message, newReactions); diff --git a/ts/models/messages.ts b/ts/models/messages.ts index a01a31372..ccac8926e 100644 --- a/ts/models/messages.ts +++ b/ts/models/messages.ts @@ -152,6 +152,7 @@ import { shouldDownloadStory } from '../util/shouldDownloadStory'; import { shouldShowStoriesView } from '../state/selectors/stories'; import type { ContactWithHydratedAvatar } from '../textsecure/SendMessage'; import { SeenStatus } from '../MessageSeenStatus'; +import { isNewReactionReplacingPrevious } from '../reactions/util'; /* eslint-disable camelcase */ /* eslint-disable more/no-then */ @@ -233,12 +234,7 @@ export class MessageModel extends window.Backbone.Model { const { storyChanged } = window.reduxActions.stories; if (isStory(this.attributes)) { - const ourConversationId = - window.ConversationController.getOurConversationIdOrThrow(); - const storyData = getStoryDataFromMessageAttributes( - this.attributes, - ourConversationId - ); + const storyData = getStoryDataFromMessageAttributes(this.attributes); if (!storyData) { return; @@ -2892,14 +2888,15 @@ export class MessageModel extends window.Backbone.Model { const reactions = reactionUtil.addOutgoingReaction( this.get('reactions') || [], - newReaction + newReaction, + isStory(this.attributes) ); this.set({ reactions }); } else { const oldReactions = this.get('reactions') || []; let reactions: Array; - const oldReaction = oldReactions.find( - re => re.fromId === reaction.get('fromId') + const oldReaction = oldReactions.find(re => + isNewReactionReplacingPrevious(re, reaction.attributes, this.attributes) ); if (oldReaction) { this.clearNotifications(oldReaction); @@ -2914,12 +2911,20 @@ export class MessageModel extends window.Backbone.Model { if (reaction.get('source') === ReactionSource.FromSync) { reactions = oldReactions.filter( re => - re.fromId !== reaction.get('fromId') || - re.timestamp > reaction.get('timestamp') + !isNewReactionReplacingPrevious( + re, + reaction.attributes, + this.attributes + ) || re.timestamp > reaction.get('timestamp') ); } else { reactions = oldReactions.filter( - re => re.fromId !== reaction.get('fromId') + re => + !isNewReactionReplacingPrevious( + re, + reaction.attributes, + this.attributes + ) ); } this.set({ reactions }); @@ -2948,7 +2953,12 @@ export class MessageModel extends window.Backbone.Model { } reactions = oldReactions.filter( - re => re.fromId !== reaction.get('fromId') + re => + !isNewReactionReplacingPrevious( + re, + reaction.attributes, + this.attributes + ) ); reactions.push(reactionToAdd); this.set({ reactions }); diff --git a/ts/reactions/util.ts b/ts/reactions/util.ts index d741238c2..d16ba9d14 100644 --- a/ts/reactions/util.ts +++ b/ts/reactions/util.ts @@ -2,8 +2,12 @@ // SPDX-License-Identifier: AGPL-3.0-only import { findLastIndex, has, identity, omit, negate } from 'lodash'; -import type { MessageReactionType } from '../model-types.d'; +import type { + MessageAttributesType, + MessageReactionType, +} from '../model-types.d'; import { areObjectEntriesEqual } from '../util/areObjectEntriesEqual'; +import { isStory } from '../state/selectors/message'; const isReactionEqual = ( a: undefined | Readonly, @@ -31,8 +35,13 @@ const isOutgoingReactionCompletelyUnsent = ({ export function addOutgoingReaction( oldReactions: ReadonlyArray, - newReaction: Readonly + newReaction: Readonly, + isStoryMessage = false ): Array { + if (isStoryMessage) { + return [...oldReactions, newReaction]; + } + const pendingOutgoingReactions = new Set( oldReactions.filter(isOutgoingReactionPending) ); @@ -101,6 +110,17 @@ export function* getUnsentConversationIds({ } } +// This function is used when filtering reactions so that we can limit normal +// messages to a single reactions but allow multiple reactions from the same +// sender for stories. +export function isNewReactionReplacingPrevious( + reaction: MessageReactionType, + newReaction: MessageReactionType, + messageAttributes: MessageAttributesType +): boolean { + return !isStory(messageAttributes) && reaction.fromId === newReaction.fromId; +} + export const markOutgoingReactionFailed = ( reactions: Array, reaction: Readonly @@ -116,7 +136,8 @@ export const markOutgoingReactionFailed = ( export const markOutgoingReactionSent = ( reactions: ReadonlyArray, reaction: Readonly, - conversationIdsSentTo: Iterable + conversationIdsSentTo: Iterable, + messageAttributes: MessageAttributesType ): Array => { const result: Array = []; @@ -135,7 +156,8 @@ export const markOutgoingReactionSent = ( if (!isReactionEqual(re, reaction)) { const shouldKeep = !isFullySent ? true - : re.fromId !== reaction.fromId || re.timestamp > reaction.timestamp; + : !isNewReactionReplacingPrevious(re, reaction, messageAttributes) || + re.timestamp > reaction.timestamp; if (shouldKeep) { result.push(re); } diff --git a/ts/services/storyLoader.ts b/ts/services/storyLoader.ts index a6c473568..1783e0084 100644 --- a/ts/services/storyLoader.ts +++ b/ts/services/storyLoader.ts @@ -17,8 +17,7 @@ export async function loadStories(): Promise { } export function getStoryDataFromMessageAttributes( - message: MessageAttributesType, - ourConversationId?: string + message: MessageAttributesType ): StoryDataType | undefined { const { attachments } = message; const unresolvedAttachment = attachments ? attachments[0] : undefined; @@ -33,17 +32,13 @@ export function getStoryDataFromMessageAttributes( ? getAttachmentsForMessage(message) : [unresolvedAttachment]; - const selectedReaction = ( - (message.reactions || []).find(re => re.fromId === ourConversationId) || {} - ).emoji; - return { attachment, messageId: message.id, - selectedReaction, ...pick(message, [ 'conversationId', 'deletedForEveryone', + 'reactions', 'readStatus', 'sendStateByConversationId', 'source', @@ -57,11 +52,8 @@ export function getStoryDataFromMessageAttributes( export function getStoriesForRedux(): Array { strictAssert(storyData, 'storyData has not been loaded'); - const ourConversationId = - window.ConversationController.getOurConversationId(); - const stories = storyData - .map(story => getStoryDataFromMessageAttributes(story, ourConversationId)) + .map(getStoryDataFromMessageAttributes) .filter(isNotNil); storyData = undefined; diff --git a/ts/state/ducks/stories.ts b/ts/state/ducks/stories.ts index 2f32a5e0b..af5699404 100644 --- a/ts/state/ducks/stories.ts +++ b/ts/state/ducks/stories.ts @@ -37,11 +37,11 @@ import { viewedReceiptsJobQueue } from '../../jobs/viewedReceiptsJobQueue'; export type StoryDataType = { attachment?: AttachmentType; messageId: string; - selectedReaction?: string; } & Pick< MessageAttributesType, | 'conversationId' | 'deletedForEveryone' + | 'reactions' | 'readStatus' | 'sendStateByConversationId' | 'source' @@ -65,7 +65,6 @@ export type StoriesStateType = { const LOAD_STORY_REPLIES = 'stories/LOAD_STORY_REPLIES'; const MARK_STORY_READ = 'stories/MARK_STORY_READ'; -const REACT_TO_STORY = 'stories/REACT_TO_STORY'; const REPLY_TO_STORY = 'stories/REPLY_TO_STORY'; export const RESOLVE_ATTACHMENT_URL = 'stories/RESOLVE_ATTACHMENT_URL'; const STORY_CHANGED = 'stories/STORY_CHANGED'; @@ -84,14 +83,6 @@ type MarkStoryReadActionType = { payload: string; }; -type ReactToStoryActionType = { - type: typeof REACT_TO_STORY; - payload: { - messageId: string; - selectedReaction: string; - }; -}; - type ReplyToStoryActionType = { type: typeof REPLY_TO_STORY; payload: MessageAttributesType; @@ -119,7 +110,6 @@ export type StoriesActionType = | MarkStoryReadActionType | MessageChangedActionType | MessageDeletedActionType - | ReactToStoryActionType | ReplyToStoryActionType | ResolveAttachmentUrlActionType | StoryChangedActionType @@ -286,27 +276,24 @@ function queueStoryDownload( function reactToStory( nextReaction: string, - messageId: string, - previousReaction?: string -): ThunkAction { + messageId: string +): ThunkAction { return async dispatch => { try { await enqueueReactionForSend({ messageId, emoji: nextReaction, - remove: nextReaction === previousReaction, - }); - dispatch({ - type: REACT_TO_STORY, - payload: { - messageId, - selectedReaction: nextReaction, - }, + remove: false, }); } catch (error) { log.error('Error enqueuing reaction', error, messageId, nextReaction); showToast(ToastReactionFailed); } + + dispatch({ + type: 'NOOP', + payload: null, + }); }; } @@ -403,8 +390,8 @@ export function reducer( 'conversationId', 'deletedForEveryone', 'messageId', + 'reactions', 'readStatus', - 'selectedReaction', 'sendStateByConversationId', 'source', 'sourceUuid', @@ -424,9 +411,14 @@ export function reducer( !isDownloaded(prevStory.attachment) && isDownloaded(newStory.attachment); const readStatusChanged = prevStory.readStatus !== newStory.readStatus; + const reactionsChanged = + prevStory.reactions?.length !== newStory.reactions?.length; const shouldReplace = - isDownloadingAttachment || hasAttachmentDownloaded || readStatusChanged; + isDownloadingAttachment || + hasAttachmentDownloaded || + readStatusChanged || + reactionsChanged; if (!shouldReplace) { return state; } @@ -448,22 +440,6 @@ export function reducer( }; } - if (action.type === REACT_TO_STORY) { - return { - ...state, - stories: state.stories.map(story => { - if (story.messageId === action.payload.messageId) { - return { - ...story, - selectedReaction: action.payload.selectedReaction, - }; - } - - return story; - }), - }; - } - if (action.type === MARK_STORY_READ) { return { ...state, diff --git a/ts/state/selectors/stories.ts b/ts/state/selectors/stories.ts index 974dbdf36..db3f5c33e 100644 --- a/ts/state/selectors/stories.ts +++ b/ts/state/selectors/stories.ts @@ -5,10 +5,12 @@ import { createSelector } from 'reselect'; import { pick } from 'lodash'; import type { GetConversationByIdType } from './conversations'; +import type { ConversationType } from '../ducks/conversations'; import type { ConversationStoryType, StoryViewType, } from '../../components/StoryListItem'; +import type { MessageReactionType } from '../../model-types.d'; import type { ReplyStateType } from '../../types/Stories'; import type { StateType } from '../reducer'; import type { StoryDataType, StoriesStateType } from '../ducks/stories'; @@ -55,6 +57,35 @@ function sortByRecencyAndUnread( return storyA.timestamp > storyB.timestamp ? -1 : 1; } +function getReactionUniqueId(reaction: MessageReactionType): string { + return `${reaction.fromId}:${reaction.targetAuthorUuid}:${reaction.timestamp}`; +} + +function getAvatarData( + conversation: ConversationType +): Pick< + ConversationType, + | 'acceptedMessageRequest' + | 'avatarPath' + | 'color' + | 'isMe' + | 'name' + | 'profileName' + | 'sharedGroupNames' + | 'title' +> { + return pick(conversation, [ + 'acceptedMessageRequest', + 'avatarPath', + 'color', + 'isMe', + 'name', + 'profileName', + 'sharedGroupNames', + 'title', + ]); +} + function getConversationStory( conversationSelector: GetConversationByIdType, story: StoryDataType, @@ -92,7 +123,6 @@ function getConversationStory( canReply: canReply(story, ourConversationId, conversationSelector), isUnread: story.readStatus === ReadStatus.Unread, messageId: story.messageId, - selectedReaction: story.selectedReaction, sender, timestamp, }; @@ -153,38 +183,56 @@ export const getStoryReplies = createSelector( conversationSelector, contactNameColorSelector, me, - { replyState }: Readonly + { stories, replyState }: Readonly ): ReplyStateType | undefined => { if (!replyState) { return; } + const foundStory = stories.find( + story => story.messageId === replyState.messageId + ); + + const reactions = foundStory + ? (foundStory.reactions || []).map(reaction => { + const conversation = conversationSelector(reaction.fromId); + + return { + ...getAvatarData(conversation), + contactNameColor: contactNameColorSelector( + foundStory.conversationId, + conversation.id + ), + id: getReactionUniqueId(reaction), + reactionEmoji: reaction.emoji, + timestamp: reaction.timestamp, + }; + }) + : []; + + const replies = replyState.replies.map(reply => { + const conversation = + reply.type === 'outgoing' + ? me + : conversationSelector(reply.sourceUuid || reply.source); + + return { + ...getAvatarData(conversation), + ...pick(reply, ['body', 'deletedForEveryone', 'id', 'timestamp']), + contactNameColor: contactNameColorSelector( + reply.conversationId, + conversation.id + ), + }; + }); + + const combined = [...replies, ...reactions].sort((a, b) => + a.timestamp > b.timestamp ? 1 : -1 + ); + return { messageId: replyState.messageId, - replies: replyState.replies.map(reply => { - const conversation = - reply.type === 'outgoing' - ? me - : conversationSelector(reply.sourceUuid || reply.source); - - return { - ...pick(conversation, [ - 'acceptedMessageRequest', - 'avatarPath', - 'color', - 'isMe', - 'name', - 'profileName', - 'sharedGroupNames', - 'title', - ]), - ...pick(reply, ['body', 'deletedForEveryone', 'id', 'timestamp']), - contactNameColor: contactNameColorSelector( - reply.conversationId, - conversation.id - ), - }; - }), + replies: combined, }; } ); diff --git a/ts/state/smart/StoryViewer.tsx b/ts/state/smart/StoryViewer.tsx index 3ebdaae78..e5d63105f 100644 --- a/ts/state/smart/StoryViewer.tsx +++ b/ts/state/smart/StoryViewer.tsx @@ -69,8 +69,8 @@ export function SmartStoryViewer({ onNextUserStories={onNextUserStories} onPrevUserStories={onPrevUserStories} onReactToStory={async (emoji, story) => { - const { messageId, selectedReaction: previousReaction } = story; - storiesActions.reactToStory(emoji, messageId, previousReaction); + const { messageId } = story; + storiesActions.reactToStory(emoji, messageId); }} onReplyToStory={(message, mentions, timestamp, story) => { storiesActions.replyToStory( diff --git a/ts/test-both/reactions/util_test.ts b/ts/test-both/reactions/util_test.ts index f9d9fc2b0..dd57e15e6 100644 --- a/ts/test-both/reactions/util_test.ts +++ b/ts/test-both/reactions/util_test.ts @@ -4,7 +4,10 @@ import { assert } from 'chai'; import { v4 as uuid } from 'uuid'; import { omit } from 'lodash'; -import type { MessageReactionType } from '../../model-types.d'; +import type { + MessageAttributesType, + MessageReactionType, +} from '../../model-types.d'; import { isEmpty } from '../../util/iterables'; import { @@ -48,6 +51,18 @@ describe('reaction utilities', () => { const newReactions = addOutgoingReaction(oldReactions, reaction); assert.deepStrictEqual(newReactions, [oldReactions[1], reaction]); }); + + it('does not remove any pending reactions if its a story', () => { + const oldReactions = [ + { ...rxn('😭', { isPending: true }), timestamp: 3 }, + { ...rxn('💬'), fromId: uuid() }, + { ...rxn('🥀', { isPending: true }), timestamp: 1 }, + { ...rxn('🌹', { isPending: true }), timestamp: 2 }, + ]; + const reaction = rxn('😀'); + const newReactions = addOutgoingReaction(oldReactions, reaction, true); + assert.deepStrictEqual(newReactions, [...oldReactions, reaction]); + }); }); describe('getNewestPendingOutgoingReaction', () => { @@ -199,21 +214,36 @@ describe('reaction utilities', () => { const reactions = [star, none, { ...rxn('🔕'), timestamp: 1 }]; + function getMessage(): MessageAttributesType { + const now = Date.now(); + return { + conversationId: uuid(), + id: uuid(), + received_at: now, + sent_at: now, + timestamp: now, + type: 'incoming', + }; + } + it("does nothing if the reaction isn't in the list", () => { const result = markOutgoingReactionSent( reactions, rxn('🥀', { isPending: true }), - [uuid()] + [uuid()], + getMessage() ); assert.deepStrictEqual(result, reactions); }); it('updates reactions to be partially sent', () => { [star, none].forEach(reaction => { - const result = markOutgoingReactionSent(reactions, reaction, [ - uuid1, - uuid2, - ]); + const result = markOutgoingReactionSent( + reactions, + reaction, + [uuid1, uuid2], + getMessage() + ); assert.deepStrictEqual( result.find(re => re.emoji === reaction.emoji) ?.isSentByConversationId, @@ -227,11 +257,12 @@ describe('reaction utilities', () => { }); it('removes sent state if a reaction with emoji is fully sent', () => { - const result = markOutgoingReactionSent(reactions, star, [ - uuid1, - uuid2, - uuid3, - ]); + const result = markOutgoingReactionSent( + reactions, + star, + [uuid1, uuid2, uuid3], + getMessage() + ); const newReaction = result.find(re => re.emoji === '⭐️'); assert.isDefined(newReaction); @@ -239,11 +270,12 @@ describe('reaction utilities', () => { }); it('removes a fully-sent reaction removal', () => { - const result = markOutgoingReactionSent(reactions, none, [ - uuid1, - uuid2, - uuid3, - ]); + const result = markOutgoingReactionSent( + reactions, + none, + [uuid1, uuid2, uuid3], + getMessage() + ); assert( result.every(({ emoji }) => typeof emoji === 'string'), @@ -252,13 +284,25 @@ describe('reaction utilities', () => { }); it('removes older reactions of mine', () => { - const result = markOutgoingReactionSent(reactions, star, [ - uuid1, - uuid2, - uuid3, - ]); + const result = markOutgoingReactionSent( + reactions, + star, + [uuid1, uuid2, uuid3], + getMessage() + ); assert.isUndefined(result.find(re => re.emoji === '🔕')); }); + + it('does not remove my older reactions if they are on a story', () => { + const result = markOutgoingReactionSent( + reactions, + star, + [uuid1, uuid2, uuid3], + { ...getMessage(), type: 'story' } + ); + + assert.isDefined(result.find(re => re.emoji === '🔕')); + }); }); });