Skip to content

Commit

Permalink
On send, pull data from target edit if sending edit
Browse files Browse the repository at this point in the history
  • Loading branch information
@scottnonnenberg-signal
scottnonnenberg-signal committed Nov 17, 2023
1 parent 146b562 commit 48245ee
Show file tree
Hide file tree
Showing 12 changed files with 529 additions and 135 deletions.
335 changes: 258 additions & 77 deletions ts/jobs/helpers/sendNormalMessage.ts
View file

Large diffs are not rendered by default.

16 changes: 9 additions & 7 deletions ts/jobs/helpers/sendReaction.ts
View file
Expand Up @@ -198,10 +198,11 @@ export async function sendReaction(
recipients: allRecipientServiceIds,
timestamp: pendingReaction.timestamp,
});
await ephemeralMessageForReactionSend.sendSyncMessageOnly(
await ephemeralMessageForReactionSend.sendSyncMessageOnly({
dataMessage,
saveErrors
);
saveErrors,
targetTimestamp: pendingReaction.timestamp,
});

didFullySend = true;
successfulConversationIds.add(ourConversationId);
Expand Down Expand Up @@ -289,13 +290,14 @@ export async function sendReaction(
);
}

await ephemeralMessageForReactionSend.send(
handleMessageSend(promise, {
await ephemeralMessageForReactionSend.send({
promise: handleMessageSend(promise, {
messageIds: [messageId],
sendType: 'reaction',
}),
saveErrors
);
saveErrors,
targetTimestamp: pendingReaction.timestamp,
});

// Because message.send swallows and processes errors, we'll await the inner promise
// to get the SendMessageProtoError, which gives us information upstream
Expand Down
9 changes: 5 additions & 4 deletions ts/jobs/helpers/sendStory.ts
View file
Expand Up @@ -365,13 +365,14 @@ export async function sendStory(
// eslint-disable-next-line no-param-reassign
message.doNotSendSyncMessage = true;

const messageSendPromise = message.send(
handleMessageSend(innerPromise, {
const messageSendPromise = message.send({
promise: handleMessageSend(innerPromise, {
messageIds: [message.id],
sendType: 'story',
}),
saveErrors
);
saveErrors,
targetTimestamp: message.get('timestamp'),
});

// Because message.send swallows and processes errors, we'll await the
// inner promise to get the SendMessageProtoError, which gives us
Expand Down
2 changes: 2 additions & 0 deletions ts/model-types.d.ts
View file
Expand Up @@ -113,6 +113,8 @@ export type MessageReactionType = {
isSentByConversationId?: Record<string, boolean>;
};

// Note: when adding to the set of things that can change via edits, sendNormalMessage.ts
// needs more usage of get/setPropForTimestamp.
export type EditHistoryType = {
attachments?: Array<AttachmentType>;
body?: string;
Expand Down
103 changes: 78 additions & 25 deletions ts/models/messages.ts
View file
Expand Up @@ -39,7 +39,6 @@ import type {
} from '../textsecure/Types.d';
import { SendMessageProtoError } from '../textsecure/Errors';
import { getUserLanguages } from '../util/userLanguages';
import { getMessageSentTimestamp } from '../util/getMessageSentTimestamp';
import { copyCdnFields } from '../util/attachments';

import type { ReactionType } from '../types/Reactions';
Expand Down Expand Up @@ -156,6 +155,11 @@ import { getSenderIdentifier } from '../util/getSenderIdentifier';
import { getNotificationDataForMessage } from '../util/getNotificationDataForMessage';
import { getNotificationTextForMessage } from '../util/getNotificationTextForMessage';
import { getMessageAuthorText } from '../util/getMessageAuthorText';
import {
getPropForTimestamp,
setPropForTimestamp,
hasEditBeenSent,
} from '../util/editHelpers';

/* eslint-disable more/no-then */

Expand Down Expand Up @@ -831,18 +835,40 @@ export class MessageModel extends window.Backbone.Model<MessageAttributesType> {
* Change any Pending send state to Failed. Note that this will not mark successful
* sends failed.
*/
public markFailed(): void {
public markFailed(editMessageTimestamp?: number): void {
const now = Date.now();
this.set(
'sendStateByConversationId',
mapValues(this.get('sendStateByConversationId') || {}, sendState =>

const targetTimestamp = editMessageTimestamp || this.get('timestamp');
const sendStateByConversationId = getPropForTimestamp({
log,
message: this,
prop: 'sendStateByConversationId',
targetTimestamp,
});

const newSendStateByConversationId = mapValues(
sendStateByConversationId || {},
sendState =>
sendStateReducer(sendState, {
type: SendActionType.Failed,
updatedAt: now,
})
)
);

setPropForTimestamp({
log,
message: this,
prop: 'sendStateByConversationId',
targetTimestamp,
value: newSendStateByConversationId,
});

// We aren't trying to send this message anymore, so we'll delete these caches
delete this.cachedOutgoingContactData;
delete this.cachedOutgoingPreviewData;
delete this.cachedOutgoingQuoteData;
delete this.cachedOutgoingStickerData;

this.notifyStorySendFailed();
}

Expand Down Expand Up @@ -886,10 +912,15 @@ export class MessageModel extends window.Backbone.Model<MessageAttributesType> {
return errors[0][0];
}

async send(
promise: Promise<CallbackResultType | void | null>,
saveErrors?: (errors: Array<Error>) => void
): Promise<void> {
async send({
promise,
saveErrors,
targetTimestamp,
}: {
promise: Promise<CallbackResultType | void | null>;
saveErrors?: (errors: Array<Error>) => void;
targetTimestamp: number;
}): Promise<void> {
const updateLeftPane =
this.getConversation()?.debouncedUpdateLastMessage ?? noop;

Expand Down Expand Up @@ -926,7 +957,12 @@ export class MessageModel extends window.Backbone.Model<MessageAttributesType> {
}

const sendStateByConversationId = {
...(this.get('sendStateByConversationId') || {}),
...(getPropForTimestamp({
log,
message: this,
prop: 'sendStateByConversationId',
targetTimestamp,
}) || {}),
};

const sendIsNotFinal =
Expand Down Expand Up @@ -970,9 +1006,13 @@ export class MessageModel extends window.Backbone.Model<MessageAttributesType> {
});

// Integrate sends via sealed sender
const latestEditTimestamp = this.get('editMessageTimestamp');
const sendIsLatest =
!latestEditTimestamp || targetTimestamp === latestEditTimestamp;
const previousUnidentifiedDeliveries =
this.get('unidentifiedDeliveries') || [];
const newUnidentifiedDeliveries =
sendIsLatest &&
sendIsFinal &&
'unidentifiedDeliveries' in result.value &&
Array.isArray(result.value.unidentifiedDeliveries)
Expand Down Expand Up @@ -1051,7 +1091,6 @@ export class MessageModel extends window.Backbone.Model<MessageAttributesType> {
}
});

attributesToUpdate.sendStateByConversationId = sendStateByConversationId;
// Only update the expirationStartTimestamp if we don't already have one set
if (!this.get('expirationStartTimestamp')) {
attributesToUpdate.expirationStartTimestamp = sentToAtLeastOneRecipient
Expand All @@ -1073,6 +1112,14 @@ export class MessageModel extends window.Backbone.Model<MessageAttributesType> {
void this.saveErrors(errorsToSave, { skipSave: true });
}

setPropForTimestamp({
log,
message: this,
prop: 'sendStateByConversationId',
targetTimestamp,
value: sendStateByConversationId,
});

if (!this.doNotSave) {
await window.Signal.Data.saveMessage(this.attributes, {
ourAci: window.textsecure.storage.user.getCheckedAci(),
Expand All @@ -1082,13 +1129,14 @@ export class MessageModel extends window.Backbone.Model<MessageAttributesType> {
updateLeftPane();

if (sentToAtLeastOneRecipient && !this.doNotSendSyncMessage) {
promises.push(this.sendSyncMessage());
promises.push(this.sendSyncMessage(targetTimestamp));
}

await Promise.all(promises);

const isTotalSuccess: boolean =
result.success && !this.get('errors')?.length;

if (isTotalSuccess) {
delete this.cachedOutgoingContactData;
delete this.cachedOutgoingPreviewData;
Expand All @@ -1099,10 +1147,15 @@ export class MessageModel extends window.Backbone.Model<MessageAttributesType> {
updateLeftPane();
}

async sendSyncMessageOnly(
dataMessage: Uint8Array,
saveErrors?: (errors: Array<Error>) => void
): Promise<CallbackResultType | void> {
async sendSyncMessageOnly({
targetTimestamp,
dataMessage,
saveErrors,
}: {
targetTimestamp: number;
dataMessage: Uint8Array;
saveErrors?: (errors: Array<Error>) => void;
}): Promise<CallbackResultType | void> {
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
const conv = this.getConversation()!;
this.set({ dataMessage });
Expand All @@ -1115,7 +1168,7 @@ export class MessageModel extends window.Backbone.Model<MessageAttributesType> {
expirationStartTimestamp: Date.now(),
errors: [],
});
const result = await this.sendSyncMessage();
const result = await this.sendSyncMessage(targetTimestamp);
this.set({
// We have to do this afterward, since we didn't have a previous send!
unidentifiedDeliveries:
Expand Down Expand Up @@ -1147,7 +1200,9 @@ export class MessageModel extends window.Backbone.Model<MessageAttributesType> {
}
}

async sendSyncMessage(): Promise<CallbackResultType | void> {
async sendSyncMessage(
targetTimestamp: number
): Promise<CallbackResultType | void> {
const ourConversation =
window.ConversationController.getOurConversationOrThrow();
const sendOptions = await getSendOptions(ourConversation.attributes, {
Expand All @@ -1173,8 +1228,8 @@ export class MessageModel extends window.Backbone.Model<MessageAttributesType> {
if (!dataMessage) {
return;
}
const isEditedMessage = Boolean(this.get('editHistory'));
const isUpdate = Boolean(this.get('synced')) && !isEditedMessage;
const wasEditSent = hasEditBeenSent(this);
const isUpdate = Boolean(this.get('synced')) && !wasEditSent;
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
const conv = this.getConversation()!;

Expand Down Expand Up @@ -1206,9 +1261,7 @@ export class MessageModel extends window.Backbone.Model<MessageAttributesType> {
map(conversationsWithSealedSender, c => c.id)
);

const timestamp = getMessageSentTimestamp(this.attributes, { log });

const encodedContent = isEditedMessage
const encodedContent = wasEditSent
? {
encodedEditMessage: dataMessage,
}
Expand All @@ -1219,7 +1272,7 @@ export class MessageModel extends window.Backbone.Model<MessageAttributesType> {
return handleMessageSend(
messaging.sendSyncMessage({
...encodedContent,
timestamp,
timestamp: targetTimestamp,
destination: conv.get('e164'),
destinationServiceId: conv.getServiceId(),
expirationStartTimestamp:
Expand Down
15 changes: 12 additions & 3 deletions ts/test-electron/models/messages_test.ts
View file
Expand Up @@ -183,7 +183,10 @@ describe('Message', () => {
editMessage: undefined,
});

await message.send(promise);
await message.send({
promise,
targetTimestamp: message.get('timestamp'),
});

const result = message.get('sendStateByConversationId') || {};
assert.hasAllKeys(result, [
Expand All @@ -203,7 +206,10 @@ describe('Message', () => {
const message = createMessage({ type: 'outgoing', source });

const promise = Promise.reject(new Error('foo bar'));
await message.send(promise);
await message.send({
promise,
targetTimestamp: message.get('timestamp'),
});

const errors = message.get('errors') || [];
assert.lengthOf(errors, 1);
Expand All @@ -217,7 +223,10 @@ describe('Message', () => {
errors: [new Error('baz qux')],
};
const promise = Promise.reject(result);
await message.send(promise);
await message.send({
promise,
targetTimestamp: message.get('timestamp'),
});

const errors = message.get('errors') || [];
assert.lengthOf(errors, 1);
Expand Down
8 changes: 4 additions & 4 deletions ts/test-mock/messaging/edit_test.ts
View file
Expand Up @@ -766,8 +766,8 @@ describe('editing', function (this: Mocha.Suite) {
strictAssert(v2.sendStateByConversationId, 'v2 has send state');
assert.strictEqual(
v2.sendStateByConversationId[conversationId].status,
SendStatus.Pending, // TODO (DESKTOP-6176) - this should be Sent!
'send state for v2 message is pending'
SendStatus.Sent,
'send state for v2 message is sent'
);

strictAssert(v3.sendStateByConversationId, 'v3 has send state');
Expand All @@ -780,8 +780,8 @@ describe('editing', function (this: Mocha.Suite) {
strictAssert(v4.sendStateByConversationId, 'v4 has send state');
assert.strictEqual(
v4.sendStateByConversationId[conversationId].status,
SendStatus.Pending, // TODO (DESKTOP-6176) - this should be Sent!
'send state for v4 message is pending'
SendStatus.Sent,
'send state for v4 message is sent'
);

assert.strictEqual(
Expand Down

0 comments on commit 48245ee

Please sign in to comment.