SES1567 - Community message delivery status fix (#1442)

* Initial investigation

* WIP

* Continued work to track down cause of delivery status issue

* Fixes #1438

* Cleanup for PR

* Further cleanup

* Fixed merge conflict

* Addressed PR feedback

---------

Co-authored-by: alansley <aclansley@gmail.com>

app/src/main/java/org/thoughtcrime/securesms/conversation/v2/ConversationActivityV2.kt

@@ -46,7 +46,6 @@ import androidx.lifecycle.Observer
 import androidx.lifecycle.ViewModelProvider
 import androidx.lifecycle.flowWithLifecycle
 import androidx.lifecycle.lifecycleScope
-import androidx.lifecycle.repeatOnLifecycle
 import androidx.loader.app.LoaderManager
 import androidx.loader.content.Loader
 import androidx.recyclerview.widget.LinearLayoutManager
@@ -57,8 +56,6 @@ import kotlinx.coroutines.Dispatchers
 import kotlinx.coroutines.channels.BufferOverflow
 import kotlinx.coroutines.channels.Channel
 import kotlinx.coroutines.flow.collectLatest
-import kotlinx.coroutines.flow.consumeAsFlow
-import kotlinx.coroutines.flow.filter
 import kotlinx.coroutines.flow.receiveAsFlow
 import kotlinx.coroutines.launch
 import kotlinx.coroutines.withContext
@@ -181,8 +178,6 @@ import org.thoughtcrime.securesms.util.push
 import org.thoughtcrime.securesms.util.show
 import org.thoughtcrime.securesms.util.toPx
 import java.lang.ref.WeakReference
-import java.time.Instant
-import java.util.Date
 import java.util.Locale
 import java.util.concurrent.ExecutionException
 import java.util.concurrent.atomic.AtomicBoolean
@@ -193,8 +188,6 @@ import kotlin.math.abs
 import kotlin.math.min
 import kotlin.math.roundToInt
 import kotlin.math.sqrt
-import kotlin.time.Duration
-import kotlin.time.Duration.Companion.milliseconds
 
 private const val TAG = "ConversationActivityV2"
 
@@ -1896,7 +1889,6 @@ class ConversationActivityV2 : PassphraseRequiredActionBarActivity(), InputBarDe
         // If the recipient is a community then we delete the message for everyone
         if (recipient.isCommunityRecipient) {
             val messageCount = 1 // Only used for plurals string
-
             showSessionDialog {
                 title(resources.getQuantityString(R.plurals.ConversationFragment_delete_selected_messages, messageCount, messageCount))
                 text(resources.getQuantityString(R.plurals.ConversationFragment_this_will_permanently_delete_all_n_selected_messages, messageCount, messageCount))

app/src/main/java/org/thoughtcrime/securesms/conversation/v2/ConversationReactionOverlay.kt

@@ -39,7 +39,6 @@ import org.thoughtcrime.securesms.components.menu.ActionItem
 import org.thoughtcrime.securesms.conversation.v2.menus.ConversationMenuItemHelper.userCanBanSelectedUsers
 import org.thoughtcrime.securesms.conversation.v2.menus.ConversationMenuItemHelper.userCanDeleteSelectedItems
 import org.thoughtcrime.securesms.database.MmsSmsDatabase
-import org.thoughtcrime.securesms.database.SessionContactDatabase
 import org.thoughtcrime.securesms.database.model.MediaMmsMessageRecord
 import org.thoughtcrime.securesms.database.model.MessageRecord
 import org.thoughtcrime.securesms.database.model.ReactionRecord

app/src/main/java/org/thoughtcrime/securesms/database/MmsDatabase.kt

@@ -630,6 +630,7 @@ class MmsDatabase(context: Context, databaseHelper: SQLCipherOpenHelper) : Messa
         if (retrieved.isExpirationUpdate) deleteExpirationTimerMessages(threadId, true.takeUnless { retrieved.isGroup })
         val messageId = insertMessageOutbox(retrieved, threadId, false, null, serverTimestamp, runThreadUpdate)
         if (messageId == -1L) {
+            Log.w(TAG, "insertSecureDecryptedMessageOutbox believes the MmsDatabase insertion failed.")
             return Optional.absent()
         }
         markAsSent(messageId, true)

app/src/main/java/org/thoughtcrime/securesms/database/MmsSmsDatabase.java

@@ -30,6 +30,7 @@ import net.zetetic.database.sqlcipher.SQLiteQueryBuilder;
 import org.jetbrains.annotations.NotNull;
 import org.session.libsession.utilities.Address;
 import org.session.libsession.utilities.Util;
+import org.session.libsignal.utilities.Log;
 import org.thoughtcrime.securesms.database.MessagingDatabase.SyncMessageId;
 import org.thoughtcrime.securesms.database.helpers.SQLCipherOpenHelper;
 import org.thoughtcrime.securesms.database.model.MessageRecord;
@@ -115,6 +116,53 @@ public class MmsSmsDatabase extends Database {
     return null;
   }
 
+  public @Nullable MessageRecord getSentMessageFor(long timestamp, String serializedAuthor) {
+    // Early exit if the author is not us
+    boolean isOwnNumber = Util.isOwnNumber(context, serializedAuthor);
+    if (!isOwnNumber) {
+      Log.i(TAG, "Asked to find sent messages but provided author is not us - returning null.");
+      return null;
+    }
+
+    try (Cursor cursor = queryTables(PROJECTION, MmsSmsColumns.NORMALIZED_DATE_SENT + " = " + timestamp, null, null)) {
+      MmsSmsDatabase.Reader reader = readerFor(cursor);
+
+      MessageRecord messageRecord;
+      while ((messageRecord = reader.getNext()) != null) {
+        if (messageRecord.isOutgoing())
+        {
+          return messageRecord;
+        }
+      }
+    }
+    Log.i(TAG, "Could not find any message sent from us at provided timestamp - returning null.");
+    return null;
+  }
+
+  public MessageRecord getLastSentMessageRecordFromSender(long threadId, String serializedAuthor) {
+    // Early exit if the author is not us
+    boolean isOwnNumber = Util.isOwnNumber(context, serializedAuthor);
+    if (!isOwnNumber) {
+      Log.i(TAG, "Asked to find last sent message but provided author is not us - returning null.");
+      return null;
+    }
+
+    String order = MmsSmsColumns.NORMALIZED_DATE_SENT + " DESC";
+    String selection = MmsSmsColumns.THREAD_ID + " = " + threadId;
+
+    // Try everything with resources so that they auto-close on end of scope
+    try (Cursor cursor = queryTables(PROJECTION, selection, order, null)) {
+      try (MmsSmsDatabase.Reader reader = readerFor(cursor)) {
+        MessageRecord messageRecord;
+        while ((messageRecord = reader.getNext()) != null) {
+          if (messageRecord.isOutgoing()) { return messageRecord; }
+        }
+      }
+    }
+    Log.i(TAG, "Could not find last sent message from us in given thread - returning null.");
+    return null;
+  }
+
   public @Nullable MessageRecord getMessageFor(long timestamp, Address author) {
     return getMessageFor(timestamp, author.serialize());
   }
@@ -248,20 +296,27 @@ public class MmsSmsDatabase extends Database {
   }
 
   public long getLastSentMessageFromSender(long threadId, String serializedAuthor) {
-    String order = MmsSmsColumns.NORMALIZED_DATE_SENT + " DESC";
-    String selection = MmsSmsColumns.THREAD_ID + " = " + threadId;
 
+    // Early exit
     boolean isOwnNumber = Util.isOwnNumber(context, serializedAuthor);
+    if (!isOwnNumber) {
+      Log.i(TAG, "Asked to find last sent message but sender isn't us - returning null.");
+      return -1;
+    }
+
+    String order = MmsSmsColumns.NORMALIZED_DATE_SENT + " DESC";
+    String selection = MmsSmsColumns.THREAD_ID + " = " + threadId;
 
     // Try everything with resources so that they auto-close on end of scope
     try (Cursor cursor = queryTables(PROJECTION, selection, order, null)) {
       try (MmsSmsDatabase.Reader reader = readerFor(cursor)) {
         MessageRecord messageRecord;
         while ((messageRecord = reader.getNext()) != null) {
-          if (isOwnNumber && messageRecord.isOutgoing()) { return messageRecord.id; }
+          if (messageRecord.isOutgoing()) { return messageRecord.id; }
         }
       }
     }
+    Log.i(TAG, "Could not find last sent message from us - returning -1.");
     return -1;
   }
 

app/src/main/java/org/thoughtcrime/securesms/database/Storage.kt

@@ -92,7 +92,6 @@ import org.thoughtcrime.securesms.mms.PartAuthority
 import org.thoughtcrime.securesms.util.ConfigurationMessageUtilities
 import org.thoughtcrime.securesms.util.SessionMetaProtocol
 import java.security.MessageDigest
-import kotlin.time.Duration.Companion.days
 import network.loki.messenger.libsession_util.util.Contact as LibSessionContact
 
 private const val TAG = "Storage"
@@ -767,13 +766,36 @@ open class Storage(
 
     override fun markAsSent(timestamp: Long, author: String) {
         val database = DatabaseComponent.get(context).mmsSmsDatabase()
-        val messageRecord = database.getMessageFor(timestamp, author) ?: return
+        val messageRecord = database.getSentMessageFor(timestamp, author)
+        if (messageRecord == null) {
+            Log.w(TAG, "Failed to retrieve local message record in Storage.markAsSent - aborting.")
+            return
+        }
+
         if (messageRecord.isMms) {
-            val mmsDatabase = DatabaseComponent.get(context).mmsDatabase()
-            mmsDatabase.markAsSent(messageRecord.getId(), true)
+            DatabaseComponent.get(context).mmsDatabase().markAsSent(messageRecord.getId(), true)
         } else {
-            val smsDatabase = DatabaseComponent.get(context).smsDatabase()
-            smsDatabase.markAsSent(messageRecord.getId(), true)
+            DatabaseComponent.get(context).smsDatabase().markAsSent(messageRecord.getId(), true)
+        }
+    }
+
+    // Method that marks a message as sent in Communities (only!) - where the server modifies the
+    // message timestamp and as such we cannot use that to identify the local message.
+    override fun markAsSentToCommunity(threadId: Long, messageID: Long) {
+        val database = DatabaseComponent.get(context).mmsSmsDatabase()
+        val message = database.getLastSentMessageRecordFromSender(threadId, TextSecurePreferences.getLocalNumber(context))
+
+        // Ensure we can find the local message..
+        if (message == null) {
+            Log.w(TAG, "Could not find local message in Storage.markAsSentToCommunity - aborting.")
+            return
+        }
+
+        // ..and mark as sent if found.
+        if (message.isMms) {
+            DatabaseComponent.get(context).mmsDatabase().markAsSent(message.getId(), true)
+        } else {
+            DatabaseComponent.get(context).smsDatabase().markAsSent(message.getId(), true)
         }
     }
 
@@ -808,7 +830,11 @@ open class Storage(
 
     override fun markUnidentified(timestamp: Long, author: String) {
         val database = DatabaseComponent.get(context).mmsSmsDatabase()
-        val messageRecord = database.getMessageFor(timestamp, author) ?: return
+        val messageRecord = database.getMessageFor(timestamp, author)
+        if (messageRecord == null) {
+            Log.w(TAG, "Could not identify message with timestamp: $timestamp from author: $author")
+            return
+        }
         if (messageRecord.isMms) {
             val mmsDatabase = DatabaseComponent.get(context).mmsDatabase()
             mmsDatabase.markUnidentified(messageRecord.getId(), true)
@@ -818,6 +844,26 @@ open class Storage(
         }
     }
 
+    // Method that marks a message as unidentified in Communities (only!) - where the server
+    // modifies the message timestamp and as such we cannot use that to identify the local message.
+    override fun markUnidentifiedInCommunity(threadId: Long, messageId: Long) {
+        val database = DatabaseComponent.get(context).mmsSmsDatabase()
+        val message = database.getLastSentMessageRecordFromSender(threadId, TextSecurePreferences.getLocalNumber(context))
+
+        // Check to ensure the message exists
+        if (message == null) {
+            Log.w(TAG, "Could not find local message in Storage.markUnidentifiedInCommunity - aborting.")
+            return
+        }
+
+        // Mark it as unidentified if we found the message successfully
+        if (message.isMms) {
+            DatabaseComponent.get(context).mmsDatabase().markUnidentified(message.getId(), true)
+        } else {
+            DatabaseComponent.get(context).smsDatabase().markUnidentified(message.getId(), true)
+        }
+    }
+
     override fun markAsSentFailed(timestamp: Long, author: String, error: Exception) {
         val database = DatabaseComponent.get(context).mmsSmsDatabase()
         val messageRecord = database.getMessageFor(timestamp, author) ?: return
@@ -971,7 +1017,10 @@ open class Storage(
         val infoMessage = OutgoingGroupMediaMessage(recipient, updateData, groupID, null, sentTimestamp, 0, 0, true, null, listOf(), listOf())
         val mmsDB = DatabaseComponent.get(context).mmsDatabase()
         val mmsSmsDB = DatabaseComponent.get(context).mmsSmsDatabase()
-        if (mmsSmsDB.getMessageFor(sentTimestamp, userPublicKey) != null) return
+        if (mmsSmsDB.getMessageFor(sentTimestamp, userPublicKey) != null) {
+            Log.w(TAG, "Bailing from insertOutgoingInfoMessage because we believe the message has already been sent!")
+            return
+        }
         val infoMessageID = mmsDB.insertMessageOutbox(infoMessage, threadID, false, null, runThreadUpdate = true)
         mmsDB.markAsSent(infoMessageID, true)
     }

app/src/main/java/org/thoughtcrime/securesms/database/model/DisplayRecord.java

@@ -22,7 +22,10 @@ import android.text.SpannableString;
 import androidx.annotation.NonNull;
 
 import org.session.libsession.utilities.recipients.Recipient;
+import org.session.libsignal.utilities.Log;
+import org.thoughtcrime.securesms.database.MmsDatabase;
 import org.thoughtcrime.securesms.database.MmsSmsColumns;
+import org.thoughtcrime.securesms.database.MmsSmsDatabase;
 import org.thoughtcrime.securesms.database.SmsDatabase;
 
 /**
@@ -48,6 +51,9 @@ public abstract class DisplayRecord {
     long dateReceived, long threadId, int deliveryStatus, int deliveryReceiptCount,
     long type, int readReceiptCount)
   {
+    // TODO: This gets hit very, very often and it likely shouldn't - place a Log.d statement in it to see.
+    //Log.d("[ACL]", "Creating a display record with delivery status of: " + deliveryStatus);
+
     this.threadId             = threadId;
     this.recipient            = recipient;
     this.dateSent             = dateSent;
@@ -76,9 +82,7 @@ public abstract class DisplayRecord {
       && deliveryStatus < SmsDatabase.Status.STATUS_PENDING) || deliveryReceiptCount > 0;
   }
 
-  public boolean isSent() {
-    return !isFailed() && !isPending();
-  }
+  public boolean isSent() { return MmsSmsColumns.Types.isSentType(type); }
 
   public boolean isSyncing() {
     return MmsSmsColumns.Types.isSyncingType(type);
@@ -99,9 +103,10 @@ public abstract class DisplayRecord {
   }
 
   public boolean isPending() {
-    return MmsSmsColumns.Types.isPendingMessageType(type)
-      && !MmsSmsColumns.Types.isIdentityVerified(type)
-      && !MmsSmsColumns.Types.isIdentityDefault(type);
+    boolean isPending = MmsSmsColumns.Types.isPendingMessageType(type) &&
+                        !MmsSmsColumns.Types.isIdentityVerified(type)  &&
+                        !MmsSmsColumns.Types.isIdentityDefault(type);
+    return isPending;
   }
 
   public boolean isRead() { return readReceiptCount > 0; }

libsession/src/main/java/org/session/libsession/database/StorageProtocol.kt

@@ -120,7 +120,9 @@ interface StorageProtocol {
     fun markAsSyncing(timestamp: Long, author: String)
     fun markAsSending(timestamp: Long, author: String)
     fun markAsSent(timestamp: Long, author: String)
+    fun markAsSentToCommunity(threadID: Long, messageID: Long)
     fun markUnidentified(timestamp: Long, author: String)
+    fun markUnidentifiedInCommunity(threadID: Long, messageID: Long)
     fun markAsSyncFailed(timestamp: Long, author: String, error: Exception)
     fun markAsSentFailed(timestamp: Long, author: String, error: Exception)
     fun clearErrorMessage(messageID: Long)

libsession/src/main/java/org/session/libsession/messaging/sending_receiving/MessageSender.kt

@@ -39,6 +39,7 @@ import org.session.libsignal.crypto.PushTransportDetails
 import org.session.libsignal.protos.SignalServiceProtos
 import org.session.libsignal.utilities.Base64
 import org.session.libsignal.utilities.IdPrefix
+import org.session.libsignal.utilities.Log
 import org.session.libsignal.utilities.Namespace
 import org.session.libsignal.utilities.defaultRequiresAuth
 import org.session.libsignal.utilities.hasNamespaces
@@ -370,7 +371,7 @@ object MessageSender {
     }
 
     // Result Handling
-    fun handleSuccessfulMessageSend(message: Message, destination: Destination, isSyncMessage: Boolean = false, openGroupSentTimestamp: Long = -1) {
+    private fun handleSuccessfulMessageSend(message: Message, destination: Destination, isSyncMessage: Boolean = false, openGroupSentTimestamp: Long = -1) {
         val storage = MessagingModuleConfiguration.shared.storage
         val userPublicKey = storage.getUserPublicKey()!!
         val timestamp = message.sentTimestamp!!
@@ -392,8 +393,10 @@ object MessageSender {
 
             // in case any errors from previous sends
             storage.clearErrorMessage(messageID)
+
             // Track the open group server message ID
-            if (message.openGroupServerMessageID != null && (destination is Destination.LegacyOpenGroup || destination is Destination.OpenGroup)) {
+            val messageIsAddressedToCommunity = message.openGroupServerMessageID != null && (destination is Destination.LegacyOpenGroup || destination is Destination.OpenGroup)
+            if (messageIsAddressedToCommunity) {
                 val server: String
                 val room: String
                 when (destination) {
@@ -415,9 +418,26 @@ object MessageSender {
                     storage.setOpenGroupServerMessageID(messageID, message.openGroupServerMessageID!!, threadID, !(message as VisibleMessage).isMediaMessage())
                 }
             }
-            // Mark the message as sent
-            storage.markAsSent(timestamp, userPublicKey)
-            storage.markUnidentified(timestamp, userPublicKey)
+
+            // Mark the message as sent.
+            // Note: When sending a message to a community the server modifies the message timestamp
+            // so when we go to look up the message in the local database by timestamp it fails and
+            // we're left with the message delivery status as "Sending" forever! As such, we use a
+            // pair of modified "markAsSentToCommunity" and "markUnidentifiedInCommunity" methods
+            // to retrieve the local message by thread & message ID rather than timestamp when
+            // handling community messages only so we can tick the delivery status over to 'Sent'.
+            // Fixed in: https://optf.atlassian.net/browse/SES-1567
+            if (messageIsAddressedToCommunity)
+            {
+                storage.markAsSentToCommunity(message.threadID!!, message.id!!)
+                storage.markUnidentifiedInCommunity(message.threadID!!, message.id!!)
+            }
+            else
+            {
+                storage.markAsSent(timestamp, userPublicKey)
+                storage.markUnidentified(timestamp, userPublicKey)
+            }
+
             // Start the disappearing messages timer if needed
             SSKEnvironment.shared.messageExpirationManager.maybeStartExpiration(message, startDisappearAfterRead = true)
         } ?: run {