From 4c89c165d89b09694b0bfa37340393bf4aa3f07d Mon Sep 17 00:00:00 2001 From: Morgan Pretty Date: Wed, 23 Feb 2022 17:12:57 +1100 Subject: [PATCH] Fixed a few bugs uncovered by QA Fixed a bug where the ConfigurationMessage was getting generated before the contact state was persisted to the database in the message request flow causing odd behaviours (now generating the ConfigurationMessage within the same transaction) Fixed a bug where sending a message to an existing message request thread once the message requests item has been hidden would show the message requests notification and trigger the section to re-appear on the home screen Fixed a bug where blocked contacts weren't getting excluded from the contacts list in the configuration message --- .../ConversationVC+Interaction.swift | 2 +- Session/Home/HomeVC.swift | 31 ++++++++-------- Session/Meta/AppDelegate.swift | 8 +++-- Session/Notifications/AppNotifications.swift | 3 ++ .../Database/Storage+Contacts.swift | 14 +++++--- .../ConfigurationMessage+Convenience.swift | 24 +++++++++---- .../MessageReceiver+Handling.swift | 36 +++++++++++++------ SessionMessagingKit/Storage.swift | 1 + .../NotificationServiceExtension.swift | 3 ++ 9 files changed, 83 insertions(+), 39 deletions(-) diff --git a/Session/Conversations/ConversationVC+Interaction.swift b/Session/Conversations/ConversationVC+Interaction.swift index c2fcc1d07..33ef7e6dc 100644 --- a/Session/Conversations/ConversationVC+Interaction.swift +++ b/Session/Conversations/ConversationVC+Interaction.swift @@ -1138,7 +1138,7 @@ extension ConversationVC { // Send a sync message with the details of the contact if let appDelegate = UIApplication.shared.delegate as? AppDelegate { - appDelegate.forceSyncConfigurationNowIfNeeded().retainUntilComplete() + appDelegate.forceSyncConfigurationNowIfNeeded(with: transaction).retainUntilComplete() } } } diff --git a/Session/Home/HomeVC.swift b/Session/Home/HomeVC.swift index 9693dc8d7..f8635f167 100644 --- a/Session/Home/HomeVC.swift +++ b/Session/Home/HomeVC.swift @@ -273,24 +273,28 @@ final class HomeVC : BaseVC, UITableViewDataSource, UITableViewDelegate, NewConv // Separate out the changes for new message requests and the inbox (so we can avoid updating for // new messages within an existing message request) - let messageRequestInserts = rowChanges + let messageRequestChanges = rowChanges .compactMap { $0 as? YapDatabaseViewRowChange } - .filter { $0.finalGroup == TSMessageRequestGroup && $0.type == .insert } + .filter { $0.originalGroup == TSMessageRequestGroup || $0.finalGroup == TSMessageRequestGroup } let inboxRowChanges = rowChanges - .filter { ($0 as? YapDatabaseViewRowChange)?.finalGroup != TSMessageRequestGroup } + .compactMap { $0 as? YapDatabaseViewRowChange } + .filter { $0.originalGroup == TSInboxGroup || $0.finalGroup == TSInboxGroup } - guard sectionChanges.count > 0 || inboxRowChanges.count > 0 || messageRequestInserts.count > 0 else { return } + guard sectionChanges.count > 0 || inboxRowChanges.count > 0 || messageRequestChanges.count > 0 else { return } tableView.beginUpdates() // If we need to unhide the message request row and then re-insert it - if !messageRequestInserts.isEmpty && (CurrentAppContext().appUserDefaults()[.hasHiddenMessageRequests] || tableView.numberOfRows(inSection: 0) == 0) { - CurrentAppContext().appUserDefaults()[.hasHiddenMessageRequests] = false - tableView.insertRows(at: [IndexPath(row: 0, section: 0)], with: .automatic) + if !messageRequestChanges.isEmpty { + if tableView.numberOfRows(inSection: 0) == 1 && Int(messageRequestCount) <= 0 { + tableView.deleteRows(at: [IndexPath(row: 0, section: 0)], with: .automatic) + } + else if tableView.numberOfRows(inSection: 0) == 0 && Int(messageRequestCount) > 0 && !CurrentAppContext().appUserDefaults()[.hasHiddenMessageRequests] { + tableView.insertRows(at: [IndexPath(row: 0, section: 0)], with: .automatic) + } } inboxRowChanges.forEach { rowChange in - let rowChange = rowChange as! YapDatabaseViewRowChange let key = rowChange.collectionKey.key threadViewModelCache[key] = nil @@ -310,12 +314,9 @@ final class HomeVC : BaseVC, UITableViewDataSource, UITableViewDelegate, NewConv // an insert as the change won't be defined correctly) if rowChange.originalGroup == TSMessageRequestGroup && rowChange.finalGroup == TSInboxGroup { tableView.insertRows(at: [ rowChange.newIndexPath! ], with: .automatic) - - // If that was the last message request then we need to also remove the message request - // row to prevent a crash - if messageRequestCount == 0 { - tableView.deleteRows(at: [ IndexPath(row: 0, section: 0) ], with: .automatic) - } + } + else if rowChange.originalGroup == TSInboxGroup && rowChange.finalGroup == TSMessageRequestGroup { + tableView.deleteRows(at: [ rowChange.indexPath! ], with: .automatic) } default: break @@ -336,7 +337,7 @@ final class HomeVC : BaseVC, UITableViewDataSource, UITableViewDelegate, NewConv case .move: // Since we are custom handling this specific movement in the above 'updates' call we need // to avoid trying to handle it here - if rowChange.originalGroup == TSMessageRequestGroup && rowChange.finalGroup == TSInboxGroup { + if rowChange.originalGroup == TSMessageRequestGroup || rowChange.finalGroup == TSMessageRequestGroup { return } diff --git a/Session/Meta/AppDelegate.swift b/Session/Meta/AppDelegate.swift index 9a66ad3d3..30525183d 100644 --- a/Session/Meta/AppDelegate.swift +++ b/Session/Meta/AppDelegate.swift @@ -22,9 +22,11 @@ extension AppDelegate { } } - func forceSyncConfigurationNowIfNeeded() -> Promise { - guard Storage.shared.getUser()?.name != nil, - let configurationMessage = ConfigurationMessage.getCurrent() else { return Promise.value(()) } + func forceSyncConfigurationNowIfNeeded(with transaction: YapDatabaseReadWriteTransaction? = nil) -> Promise { + guard Storage.shared.getUser()?.name != nil, let configurationMessage = ConfigurationMessage.getCurrent(with: transaction) else { + return Promise.value(()) + } + let destination = Message.Destination.contact(publicKey: getUserHexEncodedPublicKey()) let (promise, seal) = Promise.pending() Storage.writeSync { transaction in diff --git a/Session/Notifications/AppNotifications.swift b/Session/Notifications/AppNotifications.swift index f76313a11..65223c62d 100644 --- a/Session/Notifications/AppNotifications.swift +++ b/Session/Notifications/AppNotifications.swift @@ -182,6 +182,9 @@ public class NotificationPresenter: NSObject, NotificationsProtocol { guard numMessageRequests == 0 else { return } } else if thread.isMessageRequest() && CurrentAppContext().appUserDefaults()[.hasHiddenMessageRequests] { + // If there are other interactions on this thread already then don't show the notification + if thread.numberOfInteractions() > 1 { return } + CurrentAppContext().appUserDefaults()[.hasHiddenMessageRequests] = false } diff --git a/SessionMessagingKit/Database/Storage+Contacts.swift b/SessionMessagingKit/Database/Storage+Contacts.swift index 3591d4d5e..c1af6e5d6 100644 --- a/SessionMessagingKit/Database/Storage+Contacts.swift +++ b/SessionMessagingKit/Database/Storage+Contacts.swift @@ -55,10 +55,16 @@ extension Storage { @objc public func getAllContacts() -> Set { var result: Set = [] Storage.read { transaction in - transaction.enumerateRows(inCollection: Storage.contactCollection) { _, object, _, _ in - guard let contact = object as? Contact else { return } - result.insert(contact) - } + result = self.getAllContacts(with: transaction) + } + return result + } + + @objc public func getAllContacts(with transaction: YapDatabaseReadTransaction) -> Set { + var result: Set = [] + transaction.enumerateRows(inCollection: Storage.contactCollection) { _, object, _, _ in + guard let contact = object as? Contact else { return } + result.insert(contact) } return result } diff --git a/SessionMessagingKit/Messages/Control Messages/ConfigurationMessage+Convenience.swift b/SessionMessagingKit/Messages/Control Messages/ConfigurationMessage+Convenience.swift index 33e916eb4..6575575da 100644 --- a/SessionMessagingKit/Messages/Control Messages/ConfigurationMessage+Convenience.swift +++ b/SessionMessagingKit/Messages/Control Messages/ConfigurationMessage+Convenience.swift @@ -1,7 +1,7 @@ extension ConfigurationMessage { - public static func getCurrent() -> ConfigurationMessage? { + public static func getCurrent(with transaction: YapDatabaseReadWriteTransaction? = nil) -> ConfigurationMessage? { let storage = Storage.shared guard let user = storage.getUser() else { return nil } @@ -13,7 +13,7 @@ extension ConfigurationMessage { var contacts: Set = [] var contactCount = 0 - Storage.read { transaction in + let populateDataClosure: (YapDatabaseReadTransaction) -> () = { transaction in TSGroupThread.enumerateCollectionObjects(with: transaction) { object, _ in guard let thread = object as? TSGroupThread else { return } @@ -47,7 +47,8 @@ extension ConfigurationMessage { } } - var truncatedContacts = storage.getAllContacts() + let currentUserPublicKey: String = getUserHexEncodedPublicKey() + var truncatedContacts = storage.getAllContacts(with: transaction) if truncatedContacts.count > 200 { truncatedContacts = Set(Array(truncatedContacts)[0..<200]) @@ -57,10 +58,12 @@ extension ConfigurationMessage { let publicKey = contact.sessionID let threadID = TSContactThread.threadID(fromContactSessionID: publicKey) + // Want to sync contacts for visible threads and blocked contacts between devices guard - let thread = TSContactThread.fetch(uniqueId: threadID, transaction: transaction), - thread.shouldBeVisible && - !SSKEnvironment.shared.blockingManager.isRecipientIdBlocked(publicKey) + publicKey != currentUserPublicKey && ( + TSContactThread.fetch(uniqueId: threadID, transaction: transaction)?.shouldBeVisible == true || + SSKEnvironment.shared.blockingManager.isRecipientIdBlocked(publicKey) + ) else { return } @@ -87,6 +90,15 @@ extension ConfigurationMessage { } } + // If we are provided with a transaction then read the data based on the state of the database + // from within the transaction rather than the state in disk + if let transaction: YapDatabaseReadWriteTransaction = transaction { + populateDataClosure(transaction) + } + else { + Storage.read { transaction in populateDataClosure(transaction) } + } + return ConfigurationMessage( displayName: displayName, profilePictureURL: profilePictureURL, diff --git a/SessionMessagingKit/Sending & Receiving/MessageReceiver+Handling.swift b/SessionMessagingKit/Sending & Receiving/MessageReceiver+Handling.swift index 545acfbeb..de65ce5cf 100644 --- a/SessionMessagingKit/Sending & Receiving/MessageReceiver+Handling.swift +++ b/SessionMessagingKit/Sending & Receiving/MessageReceiver+Handling.swift @@ -223,24 +223,40 @@ extension MessageReceiver { if contactInfo.hasDidApproveMe { contact.didApproveMe = contactInfo.didApproveMe } Storage.shared.setContact(contact, using: transaction) - let thread = TSContactThread.getOrCreateThread(withContactSessionID: sessionID, transaction: transaction) - thread.shouldBeVisible = true - thread.save(with: transaction) + + // If the contact is blocked + if contactInfo.hasIsBlocked && contactInfo.isBlocked { + // If this message changed them to the blocked state and there is an existing thread + // associated with them that is a message request thread then delete it (assume + // that the current user had deleted that message request) + if + contactInfo.isBlocked != OWSBlockingManager.shared().isRecipientIdBlocked(sessionID), + let thread: TSContactThread = TSContactThread.getWithContactSessionID(sessionID, transaction: transaction), + thread.isMessageRequest(using: transaction) + { + thread.removeAllThreadInteractions(with: transaction) + thread.remove(with: transaction) + } + } + else { + // Otherwise create and save the thread + let thread = TSContactThread.getOrCreateThread(withContactSessionID: sessionID, transaction: transaction) + thread.shouldBeVisible = true + thread.save(with: transaction) + } } - // Contacts blocked state // FIXME: 'OWSBlockingManager' manages it's own dbConnection and transactions so we have to dispatch this to prevent deadlocks - DispatchQueue.global(qos: .background).async { + DispatchQueue.global().async { for contactInfo in message.contacts { let sessionID = contactInfo.publicKey! - let contact = Contact(sessionID: sessionID) - if contact.isBlocked != OWSBlockingManager.shared().isRecipientIdBlocked(contact.sessionID) { - if contact.isBlocked { - OWSBlockingManager.shared().addBlockedPhoneNumber(contact.sessionID) + if contactInfo.hasIsBlocked && contactInfo.isBlocked != OWSBlockingManager.shared().isRecipientIdBlocked(sessionID) { + if contactInfo.isBlocked { + OWSBlockingManager.shared().addBlockedPhoneNumber(sessionID) } else { - OWSBlockingManager.shared().removeBlockedPhoneNumber(contact.sessionID) + OWSBlockingManager.shared().removeBlockedPhoneNumber(sessionID) } } } diff --git a/SessionMessagingKit/Storage.swift b/SessionMessagingKit/Storage.swift index 4444307d3..127cce708 100644 --- a/SessionMessagingKit/Storage.swift +++ b/SessionMessagingKit/Storage.swift @@ -18,6 +18,7 @@ public protocol SessionMessagingKitStorageProtocol { func getUserED25519KeyPair() -> Box.KeyPair? func getUser() -> Contact? func getAllContacts() -> Set + func getAllContacts(with transaction: YapDatabaseReadTransaction) -> Set // MARK: - Closed Groups diff --git a/SessionNotificationServiceExtension/NotificationServiceExtension.swift b/SessionNotificationServiceExtension/NotificationServiceExtension.swift index 215accbe6..1a227569f 100644 --- a/SessionNotificationServiceExtension/NotificationServiceExtension.swift +++ b/SessionNotificationServiceExtension/NotificationServiceExtension.swift @@ -88,6 +88,9 @@ public final class NotificationServiceExtension : UNNotificationServiceExtension guard numMessageRequests == 0 else { return self.completeSilenty() } } else if thread.isMessageRequest() && CurrentAppContext().appUserDefaults()[.hasHiddenMessageRequests] { + // If there are other interactions on this thread already then don't show the notification + if thread.numberOfInteractions() > 1 { return } + CurrentAppContext().appUserDefaults()[.hasHiddenMessageRequests] = false }