Respond to CR.

// FREEBIE
8 years ago · b74da07f7e
parent 825503210b
commit b74da07f7e
9 changed files with 121 additions and 73 deletions
--- a/SignalServiceKit/src/Devices/OWSReadReceiptsForSenderMessage.m
+++ b/SignalServiceKit/src/Devices/OWSReadReceiptsForSenderMessage.m
@ -3,7 +3,7 @@
 //

 #import "OWSReadReceiptsForSenderMessage.h"
-#import "NSDate+millisecondTimeStamp.h"
+#import "NSDate+OWS.h"
 #import "OWSReadReceipt.h"
 #import "OWSSignalServiceProtos.pb.h"
 #import "SignalRecipient.h"
@ -56,8 +56,6 @@ NS_ASSUME_NONNULL_BEGIN
        [builder addTimestamp:[messageTimestamp unsignedLongLongValue]];
    }

-    // TODO: addLocalProfileKeyIfNecessary.
-
    return [builder build];
 }

--- a/SignalServiceKit/src/Devices/OWSRecordTranscriptJob.m
+++ b/SignalServiceKit/src/Devices/OWSRecordTranscriptJob.m
@ -104,17 +104,16 @@ NS_ASSUME_NONNULL_BEGIN
    }

    if (outgoingMessage.body.length < 1 && outgoingMessage.attachmentIds.count < 1) {
-        // TODO: Is this safe?
-        DDLogInfo(@"Ignoring message transcript for empty message.");
+        OWSFail(@"Ignoring message transcript for empty message.");
        return;
    }

-    // TODO: Refactor this logic.
+    // TODO: Refactor this logic. Most of it doesn't belong in `OWSMessageSender`.
    [self.messageSender handleMessageSentRemotely:outgoingMessage
                                           sentAt:transcript.expirationStartedAt
                                      transaction:transaction];

-    [self.readReceiptManager outgoingMessageFromLinkedDevice:outgoingMessage transaction:transaction];
+    [self.readReceiptManager updateOutgoingMessageFromLinkedDevice:outgoingMessage transaction:transaction];

    [attachmentsProcessor
        fetchAttachmentsForMessage:outgoingMessage
--- a/SignalServiceKit/src/Messages/Interactions/TSInteraction.h
+++ b/SignalServiceKit/src/Messages/Interactions/TSInteraction.h
@ -26,8 +26,6 @@ NS_ASSUME_NONNULL_BEGIN

 #pragma mark Utility Method

-+ (instancetype)interactionForTimestamp:(uint64_t)timestamp
-                        withTransaction:(YapDatabaseReadWriteTransaction *)transaction;
 + (NSArray<TSInteraction *> *)interactionsWithTimestamp:(uint64_t)timestamp
                                                ofClass:(Class)clazz
                                        withTransaction:(YapDatabaseReadWriteTransaction *)transaction;
--- a/SignalServiceKit/src/Messages/Interactions/TSInteraction.m
+++ b/SignalServiceKit/src/Messages/Interactions/TSInteraction.m
@ -12,28 +12,6 @@ NS_ASSUME_NONNULL_BEGIN

@implementation TSInteraction

-+ (instancetype)interactionForTimestamp:(uint64_t)timestamp
-                        withTransaction:(YapDatabaseReadWriteTransaction *)transaction {
-    OWSAssert(timestamp > 0);
-
-    // Accept any interaction.
-    NSArray<TSInteraction *> *interactions = [self interactionsWithTimestamp:timestamp
-                                                                  withFilter:^(TSInteraction *interaction) {
-                                                                      return YES;
-                                                                  }
-                                                             withTransaction:transaction];
-
-    if (interactions.count < 1) {
-        // TODO: OWSFail()?
-        return nil;
-    }
-    if (interactions.count > 1) {
-        DDLogWarn(@"The database contains %zd colliding timestamps at: %lld.", interactions.count, timestamp);
-    }
-    TSInteraction *lastInteraction = interactions.lastObject;
-    return lastInteraction;
-}
-
 + (NSArray<TSInteraction *> *)interactionsWithTimestamp:(uint64_t)timestamp
                                                ofClass:(Class)clazz
                                        withTransaction:(YapDatabaseReadWriteTransaction *)transaction
@ -42,14 +20,14 @@ NS_ASSUME_NONNULL_BEGIN

    // Accept any interaction.
    return [self interactionsWithTimestamp:timestamp
-                                withFilter:^(TSInteraction *interaction) {
+                                    filter:^(TSInteraction *interaction) {
                                        return [interaction isKindOfClass:clazz];
                                    }
                           withTransaction:transaction];
 }

 + (NSArray<TSInteraction *> *)interactionsWithTimestamp:(uint64_t)timestamp
-                                             withFilter:(BOOL (^_Nonnull)(TSInteraction *))filter
+                                                 filter:(BOOL (^_Nonnull)(TSInteraction *))filter
                                        withTransaction:(YapDatabaseReadWriteTransaction *)transaction
 {
    OWSAssert(timestamp > 0);
--- a/SignalServiceKit/src/Messages/Interactions/TSOutgoingMessage.h
+++ b/SignalServiceKit/src/Messages/Interactions/TSOutgoingMessage.h
@ -176,7 +176,7 @@ typedef NS_ENUM(NSInteger, TSGroupMetaMessage) {
 - (void)updateWithWasSentFromLinkedDeviceWithTransaction:(YapDatabaseReadWriteTransaction *)transaction;
 - (void)updateWithSingleGroupRecipient:(NSString *)singleGroupRecipient
                           transaction:(YapDatabaseReadWriteTransaction *)transaction;
- (void)updateWithReadRecipient:(NSString *)recipientId transaction:(YapDatabaseReadWriteTransaction *)transaction;
+- (void)updateWithReadRecipientId:(NSString *)recipientId transaction:(YapDatabaseReadWriteTransaction *)transaction;

 #pragma mark - Sent Recipients

--- a/SignalServiceKit/src/Messages/Interactions/TSOutgoingMessage.m
+++ b/SignalServiceKit/src/Messages/Interactions/TSOutgoingMessage.m
@ -411,7 +411,7 @@ NSString *const kTSOutgoingMessageSentRecipientAll = @"kTSOutgoingMessageSentRec
    }];
 }

- (void)updateWithReadRecipient:(NSString *)recipientId transaction:(YapDatabaseReadWriteTransaction *)transaction
+- (void)updateWithReadRecipientId:(NSString *)recipientId transaction:(YapDatabaseReadWriteTransaction *)transaction
 {
    OWSAssert(recipientId.length > 0);
    OWSAssert(transaction);
--- a/SignalServiceKit/src/Messages/OWSMessageManager.m
+++ b/SignalServiceKit/src/Messages/OWSMessageManager.m
@ -187,14 +187,24 @@ NS_ASSUME_NONNULL_BEGIN
    OWSAssert(envelope);
    OWSAssert(transaction);

-    TSInteraction *interaction = [TSInteraction interactionForTimestamp:envelope.timestamp withTransaction:transaction];
-    if ([interaction isKindOfClass:[TSOutgoingMessage class]]) {
-        TSOutgoingMessage *outgoingMessage = (TSOutgoingMessage *)interaction;
-        [outgoingMessage updateWithWasDeliveredWithTransaction:transaction];
-    } else {
+    NSArray<TSOutgoingMessage *> *messages
+        = (NSArray<TSOutgoingMessage *> *)[TSInteraction interactionsWithTimestamp:envelope.timestamp
+                                                                           ofClass:[TSOutgoingMessage class]
+                                                                   withTransaction:transaction];
+    if (messages.count < 1) {
        // Desktop currently sends delivery receipts for "unpersisted" messages
        // like group updates, so these errors are expected to a certain extent.
-        DDLogInfo(@"%@ Unexpected message with timestamp: %llu", self.tag, envelope.timestamp);
+        DDLogInfo(@"%@ Missing message for delivery receipt: %llu", self.tag, envelope.timestamp);
+    } else {
+        if (messages.count > 1) {
+            DDLogInfo(@"%@ More than one message (%zd) for delivery receipt: %llu",
+                self.tag,
+                messages.count,
+                envelope.timestamp);
+        }
+        for (TSOutgoingMessage *outgoingMessage in messages) {
+            [outgoingMessage updateWithWasDeliveredWithTransaction:transaction];
+        }
    }
 }

--- a/SignalServiceKit/src/Messages/OWSReadReceiptManager.h
+++ b/SignalServiceKit/src/Messages/OWSReadReceiptManager.h
@ -39,7 +39,7 @@ NS_ASSUME_NONNULL_BEGIN
 - (void)processReadReceiptsFromRecipient:(OWSSignalServiceProtosReceiptMessage *)receiptMessage
                                envelope:(OWSSignalServiceProtosEnvelope *)envelope;

- (void)outgoingMessageFromLinkedDevice:(TSOutgoingMessage *)message
+- (void)updateOutgoingMessageFromLinkedDevice:(TSOutgoingMessage *)message
                                  transaction:(YapDatabaseReadWriteTransaction *)transaction;

 // This method cues this manager:
--- a/SignalServiceKit/src/Messages/OWSReadReceiptManager.m
+++ b/SignalServiceKit/src/Messages/OWSReadReceiptManager.m
@ -17,9 +17,81 @@

 NS_ASSUME_NONNULL_BEGIN

+@interface TSRecipientReadReceipt : TSYapDatabaseObject
+
+@property (nonatomic, readonly) uint64_t timestamp;
+@property (nonatomic, readonly) NSSet<NSString *> *recipientIds;
+
+@end
+
+#pragma mark -
+
+@implementation TSRecipientReadReceipt
+
+- (instancetype)initWithTimestamp:(uint64_t)timestamp
+{
+    OWSAssert(timestamp > 0);
+
+    self = [super initWithUniqueId:[TSRecipientReadReceipt uniqueIdForTimestamp:timestamp]];
+
+    if (self) {
+        _timestamp = timestamp;
+        _recipientIds = [NSSet set];
+    }
+
+    return self;
+}
+
+ (NSString *)uniqueIdForTimestamp:(uint64_t)timestamp
+{
+    return [NSString stringWithFormat:@"%llu", timestamp];
+}
+
+- (void)addRecipientId:(NSString *)recipientId
+{
+    NSMutableSet<NSString *> *recipientIdsCopy = [self.recipientIds mutableCopy];
+    [recipientIdsCopy addObject:recipientId];
+    _recipientIds = [recipientIdsCopy copy];
+}
+
+ (void)addRecipientId:(NSString *)recipientId
+             timestamp:(uint64_t)timestamp
+           transaction:(YapDatabaseReadWriteTransaction *)transaction
+{
+    OWSAssert(transaction);
+
+    TSRecipientReadReceipt *_Nullable recipientReadReceipt =
+        [transaction objectForKey:[self uniqueIdForTimestamp:timestamp] inCollection:[self collection]];
+    if (!recipientReadReceipt) {
+        recipientReadReceipt = [[TSRecipientReadReceipt alloc] initWithTimestamp:timestamp];
+    }
+    [recipientReadReceipt addRecipientId:recipientId];
+    [recipientReadReceipt saveWithTransaction:transaction];
+}
+
+ (nullable NSSet<NSString *> *)recipientIdsForTimestamp:(uint64_t)timestamp
+                                             transaction:(YapDatabaseReadWriteTransaction *)transaction
+{
+    OWSAssert(transaction);
+
+    TSRecipientReadReceipt *_Nullable recipientReadReceipt =
+        [transaction objectForKey:[self uniqueIdForTimestamp:timestamp] inCollection:[self collection]];
+    return recipientReadReceipt.recipientIds;
+}
+
+ (void)removeRecipientIdsForTimestamp:(uint64_t)timestamp transaction:(YapDatabaseReadWriteTransaction *)transaction
+{
+    OWSAssert(transaction);
+
+    [transaction removeObjectForKey:[self uniqueIdForTimestamp:timestamp] inCollection:[self collection]];
+}
+
+@end
+
+#pragma mark -
+
 NSString *const OWSReadReceiptManagerCollection = @"OWSReadReceiptManagerCollection";
 NSString *const OWSReadReceiptManagerAreReadReceiptsEnabled = @"areReadReceiptsEnabled";
-NSString *const OWSRecipientReadReceiptCollection = @"OWSRecipientReadReceiptCollection";

@interface OWSReadReceiptManager ()

@ -37,7 +109,7 @@ NSString *const OWSRecipientReadReceiptCollection = @"OWSRecipientReadReceiptCol
 // we will send to senders.
 //
 // Should only be accessed while synchronized on the OWSReadReceiptManager.
-@property (nonatomic, readonly) NSMutableDictionary<NSString *, NSMutableArray<NSNumber *> *> *toSenderReadReceiptMap;
+@property (nonatomic, readonly) NSMutableDictionary<NSString *, NSMutableSet<NSNumber *> *> *toSenderReadReceiptMap;

 // Should only be accessed while synchronized on the OWSReadReceiptManager.
@property (nonatomic) BOOL isProcessing;
@ -168,16 +240,17 @@ NSString *const OWSRecipientReadReceiptCollection = @"OWSRecipientReadReceiptCol
            });
        }

-        NSArray<OWSReadReceipt *> *readReceiptsToSend = [[self.toLinkedDevicesReadReceiptMap allValues] copy];
+        NSArray<OWSReadReceipt *> *readReceiptsToSend = [self.toLinkedDevicesReadReceiptMap allValues];
        [self.toLinkedDevicesReadReceiptMap removeAllObjects];
        if (self.toSenderReadReceiptMap.count > 0) {
            for (NSString *recipientId in self.toSenderReadReceiptMap) {
-                NSArray<NSNumber *> *timestamps = self.toSenderReadReceiptMap[recipientId];
+                NSSet<NSNumber *> *timestamps = self.toSenderReadReceiptMap[recipientId];
                OWSAssert(timestamps.count > 0);

                TSThread *thread = [TSContactThread getOrCreateThreadWithContactId:recipientId];
                OWSReadReceiptsForSenderMessage *message =
-                    [[OWSReadReceiptsForSenderMessage alloc] initWithThread:thread messageTimestamps:timestamps];
+                    [[OWSReadReceiptsForSenderMessage alloc] initWithThread:thread
+                                                          messageTimestamps:timestamps.allObjects];

                dispatch_async(dispatch_get_main_queue(), ^{
                    [self.messageSender sendMessage:message
@ -274,9 +347,9 @@ NSString *const OWSRecipientReadReceiptCollection = @"OWSRecipientReadReceiptCol

            if ([self areReadReceiptsEnabled]) {
                DDLogVerbose(@"%@ Enqueuing read receipt for sender.", self.tag);
-                NSMutableArray<NSNumber *> *_Nullable timestamps = self.toSenderReadReceiptMap[messageAuthorId];
+                NSMutableSet<NSNumber *> *_Nullable timestamps = self.toSenderReadReceiptMap[messageAuthorId];
                if (!timestamps) {
-                    timestamps = [NSMutableArray new];
+                    timestamps = [NSMutableSet new];
                    self.toSenderReadReceiptMap[messageAuthorId] = timestamps;
                }
                [timestamps addObject:@(message.timestamp)];
@ -320,42 +393,34 @@ NSString *const OWSRecipientReadReceiptCollection = @"OWSRecipientReadReceiptCol
                    // TODO: We might also need to "mark as read by recipient" any older messages
                    // from us in that thread.  Or maybe this state should hang on the thread?
                    for (TSOutgoingMessage *message in messages) {
-                        [message updateWithReadRecipient:recipientId transaction:transaction];
+                        [message updateWithReadRecipientId:recipientId transaction:transaction];
                    }
                } else {
                    // Persist the read receipts so that we can apply them to outgoing messages
                    // that we learn about later through sync messages.
-                    NSString *storageKey = [NSString stringWithFormat:@"%llu", timestamp];
-                    NSSet<NSString *> *recipientIds =
-                        [transaction objectForKey:storageKey inCollection:OWSRecipientReadReceiptCollection];
-                    NSMutableSet<NSString *> *recipientIdsCopy
-                        = (recipientIds ? [recipientIds mutableCopy] : [NSMutableSet new]);
-                    [recipientIdsCopy addObject:recipientId];
-                    [transaction setObject:recipientIdsCopy
-                                    forKey:storageKey
-                              inCollection:OWSRecipientReadReceiptCollection];
+                    [TSRecipientReadReceipt addRecipientId:recipientId timestamp:timestamp transaction:transaction];
                }
            }
        }];
    });
 }

- (void)outgoingMessageFromLinkedDevice:(TSOutgoingMessage *)message
+- (void)updateOutgoingMessageFromLinkedDevice:(TSOutgoingMessage *)message
                                  transaction:(YapDatabaseReadWriteTransaction *)transaction
 {
    OWSAssert(message);
    OWSAssert(transaction);

-    NSString *storageKey = [NSString stringWithFormat:@"%llu", message.timestamp];
-    NSSet<NSString *> *recipientIds =
-        [transaction objectForKey:storageKey inCollection:OWSRecipientReadReceiptCollection];
-    if (recipientIds) {
+    NSSet<NSString *> *_Nullable recipientIds =
+        [TSRecipientReadReceipt recipientIdsForTimestamp:message.timestamp transaction:transaction];
+    if (!recipientIds) {
+        return;
+    }
    OWSAssert(recipientIds.count > 0);
    for (NSString *recipientId in recipientIds) {
-            [message updateWithReadRecipient:recipientId transaction:transaction];
-        }
+        [message updateWithReadRecipientId:recipientId transaction:transaction];
    }
-    [transaction removeObjectForKey:storageKey inCollection:OWSRecipientReadReceiptCollection];
+    [TSRecipientReadReceipt removeRecipientIdsForTimestamp:message.timestamp transaction:transaction];
 }

 #pragma mark - Settings