From cdd89b3d543afe712bd2bc96fd1ab79f16f68b9c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?S=C3=A9bastien=20Blin?=
<sebastien.blin@savoirfairelinux.com>
Date: Wed, 21 Jul 2021 17:13:31 -0400
Subject: [PATCH] jamiaccount: remove syncing with uri
A user can remove a contact while a conversation is syncing with
this contact. In this case, the conversation was not deleted
properly. Causing some UI bug when re-adding the contact.
GitLab: #596
Change-Id: I88f2549a02ba67e9030cdadfdfb45f34a0ee06b9
---
src/jamidht/jamiaccount.cpp | 43 ++++++++++++------
test/unitTest/conversation/conversation.cpp | 48 +++++++++++++++++++++
2 files changed, 77 insertions(+), 14 deletions(-)
diff --git a/src/jamidht/jamiaccount.cpp b/src/jamidht/jamiaccount.cpp
index d60053bd37..7ca1dadb65 100644
--- a/src/jamidht/jamiaccount.cpp
+++ b/src/jamidht/jamiaccount.cpp
@@ -3060,35 +3060,50 @@ JamiAccount::addContact(const std::string& uri, bool confirmed)
void
JamiAccount::removeContact(const std::string& uri, bool ban)
{
+ std::map<std::string, ConvInfo> convInfos;
{
std::lock_guard<std::mutex> lock(configurationMutex_);
- if (accountManager_)
+ if (accountManager_) {
accountManager_->removeContact(uri, ban);
- else
+ convInfos = accountManager_->getInfo()->conversations;
+ } else {
JAMI_WARN("[Account %s] removeContact: account not loaded", getAccountID().c_str());
+ return;
+ }
}
// Remove related conversation
auto isSelf = uri == getUsername();
+ bool updateConvInfos = false;
std::vector<std::string> toRm;
{
std::lock_guard<std::mutex> lk(conversationsMtx_);
- for (const auto& [key, conv] : conversations_) {
- try {
- // Note it's important to check getUsername(), else
- // removing self can remove all conversations
- if (conv->mode() == ConversationMode::ONE_TO_ONE) {
- auto initMembers = conv->getInitialMembers();
- if ((isSelf && initMembers.size() == 1)
- || std::find(initMembers.begin(), initMembers.end(), uri)
- != initMembers.end())
- toRm.emplace_back(key);
+ for (auto& [convId, conv] : convInfos) {
+ auto itConv = conversations_.find(convId);
+ if (itConv != conversations_.end() && itConv->second) {
+ try {
+ // Note it's important to check getUsername(), else
+ // removing self can remove all conversations
+ if (itConv->second->mode() == ConversationMode::ONE_TO_ONE) {
+ auto initMembers = itConv->second->getInitialMembers();
+ if ((isSelf && initMembers.size() == 1)
+ || std::find(initMembers.begin(), initMembers.end(), uri)
+ != initMembers.end())
+ toRm.emplace_back(convId);
+ }
+ } catch (const std::exception& e) {
+ JAMI_WARN("%s", e.what());
}
- } catch (const std::exception& e) {
- JAMI_WARN("%s", e.what());
+ } else if (std::find(conv.members.begin(), conv.members.end(), uri)
+ != conv.members.end()) {
+ // It's syncing with uri, mark as removed!
+ conv.removed = std::time(nullptr);
+ updateConvInfos = true;
}
}
}
+ if (updateConvInfos)
+ accountManager_->setConversations(convInfos);
for (const auto& id : toRm) {
// Note, if we ban the device, we don't send the leave cause the other peer will just
// never got the notifications, so just erase the datas
diff --git a/test/unitTest/conversation/conversation.cpp b/test/unitTest/conversation/conversation.cpp
index 3bb5ee97ee..d99685b2dc 100644
--- a/test/unitTest/conversation/conversation.cpp
+++ b/test/unitTest/conversation/conversation.cpp
@@ -155,6 +155,7 @@ private:
void testDoNotLoadIncorrectConversation();
void testSyncingWhileAccepting();
void testGetConversationsMembersWhileSyncing();
+ void testRemoveContactRemoveSyncing();
void testCountInteractions();
CPPUNIT_TEST_SUITE(ConversationTest);
@@ -213,6 +214,7 @@ private:
CPPUNIT_TEST(testDoNotLoadIncorrectConversation);
CPPUNIT_TEST(testSyncingWhileAccepting);
CPPUNIT_TEST(testGetConversationsMembersWhileSyncing);
+ CPPUNIT_TEST(testRemoveContactRemoveSyncing);
CPPUNIT_TEST(testCountInteractions);
CPPUNIT_TEST_SUITE_END();
};
@@ -4501,6 +4503,52 @@ ConversationTest::testGetConversationsMembersWhileSyncing()
!= members.end());
}
+void
+ConversationTest::testRemoveContactRemoveSyncing()
+{
+ auto aliceAccount = Manager::instance().getAccount<JamiAccount>(aliceId);
+ auto bobAccount = Manager::instance().getAccount<JamiAccount>(bobId);
+ auto bobUri = bobAccount->getUsername();
+ auto aliceUri = aliceAccount->getUsername();
+ std::mutex mtx;
+ std::unique_lock<std::mutex> lk {mtx};
+ std::condition_variable cv;
+ std::map<std::string, std::shared_ptr<DRing::CallbackWrapperBase>> confHandlers;
+ bool conversationReady = false, requestReceived = false;
+ std::string convId = "";
+ confHandlers.insert(DRing::exportable_callback<DRing::ConfigurationSignal::IncomingTrustRequest>(
+ [&](const std::string& account_id,
+ const std::string& /*from*/,
+ const std::string& convId,
+ const std::vector<uint8_t>& /*payload*/,
+ time_t /*received*/) {
+ if (account_id == bobId && !convId.empty())
+ requestReceived = true;
+ cv.notify_one();
+ }));
+ confHandlers.insert(DRing::exportable_callback<DRing::ConversationSignal::ConversationReady>(
+ [&](const std::string& accountId, const std::string& conversationId) {
+ if (accountId == aliceId) {
+ convId = conversationId;
+ } else if (accountId == bobId) {
+ conversationReady = true;
+ }
+ cv.notify_one();
+ }));
+ DRing::registerSignalHandlers(confHandlers);
+ aliceAccount->addContact(bobUri);
+ aliceAccount->sendTrustRequest(bobUri, {});
+ CPPUNIT_ASSERT(cv.wait_for(lk, std::chrono::seconds(30), [&]() { return requestReceived; }));
+
+ Manager::instance().sendRegister(aliceId, false); // This avoid to sync immediately
+ CPPUNIT_ASSERT(bobAccount->acceptTrustRequest(aliceUri));
+
+ CPPUNIT_ASSERT(bobAccount->getConversations().size() == 1);
+ bobAccount->removeContact(aliceUri, false);
+
+ CPPUNIT_ASSERT(bobAccount->getConversations().size() == 0);
+}
+
void
ConversationTest::testCountInteractions()
{
--
GitLab