From 4392c72206b3a7c5f294fc187b2f6904e660890e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?S=C3=A9bastien=20Blin?=
<sebastien.blin@savoirfairelinux.com>
Date: Mon, 27 Nov 2023 13:25:30 -0500
Subject: [PATCH] conversation_module: avoid double conversation for same
contact
If we receive a conversation request for a contact where we
already have a conversation, we can just accept the new one.
GitLab: #924
Change-Id: I9e5d0f7302567ce488c52b45377117f034cd5da5
---
src/jamidht/account_manager.cpp | 30 +++++-----
src/jamidht/account_manager.h | 2 +-
src/jamidht/archive_account_manager.cpp | 4 +-
src/jamidht/archive_account_manager.h | 2 +-
src/jamidht/conversation_module.cpp | 44 ++++++++++----
src/jamidht/jamiaccount.cpp | 3 +-
src/jamidht/jamiaccount.h | 5 ++
.../conversation/conversationRequest.cpp | 59 +++++++++++++++++++
8 files changed, 117 insertions(+), 32 deletions(-)
diff --git a/src/jamidht/account_manager.cpp b/src/jamidht/account_manager.cpp
index 66e28c3749..8074a284d5 100644
--- a/src/jamidht/account_manager.cpp
+++ b/src/jamidht/account_manager.cpp
@@ -272,24 +272,26 @@ AccountManager::announceFromReceipt(const std::string& receipt)
}
void
-AccountManager::startSync(const OnNewDeviceCb& cb, const OnDeviceAnnouncedCb& dcb)
+AccountManager::startSync(const OnNewDeviceCb& cb, const OnDeviceAnnouncedCb& dcb, bool publishPresence)
{
// Put device announcement
if (info_->announce) {
auto h = dht::InfoHash(info_->accountId);
- dht_->put(
- h,
- info_->announce,
- [dcb = std::move(dcb), h](bool ok) {
- if (ok)
- JAMI_DEBUG("device announced at {}", h.toString());
- // We do not care about the status, it's a permanent put, if this fail,
- // this means the DHT is disconnected but the put will be retried when connected.
- if (dcb)
- dcb();
- },
- {},
- true);
+ if (publishPresence) {
+ dht_->put(
+ h,
+ info_->announce,
+ [dcb = std::move(dcb), h](bool ok) {
+ if (ok)
+ JAMI_DEBUG("device announced at {}", h.toString());
+ // We do not care about the status, it's a permanent put, if this fail,
+ // this means the DHT is disconnected but the put will be retried when connected.
+ if (dcb)
+ dcb();
+ },
+ {},
+ true);
+ }
for (const auto& crl : info_->identity.second->issuer->getRevocationLists())
dht_->put(h, crl, dht::DoneCallback {}, {}, true);
dht_->listen<DeviceAnnouncement>(h, [this, cb = std::move(cb)](DeviceAnnouncement&& dev) {
diff --git a/src/jamidht/account_manager.h b/src/jamidht/account_manager.h
index cd8e5afcd3..9703012f43 100644
--- a/src/jamidht/account_manager.h
+++ b/src/jamidht/account_manager.h
@@ -140,7 +140,7 @@ public:
void setDht(const std::shared_ptr<dht::DhtRunner>& dht) { dht_ = dht; }
- virtual void startSync(const OnNewDeviceCb& cb, const OnDeviceAnnouncedCb& dcb);
+ virtual void startSync(const OnNewDeviceCb& cb, const OnDeviceAnnouncedCb& dcb, bool publishPresence = true);
const AccountInfo* getInfo() const { return info_.get(); }
diff --git a/src/jamidht/archive_account_manager.cpp b/src/jamidht/archive_account_manager.cpp
index 0b399f9919..7cf0edb0f4 100644
--- a/src/jamidht/archive_account_manager.cpp
+++ b/src/jamidht/archive_account_manager.cpp
@@ -537,9 +537,9 @@ ArchiveAccountManager::syncDevices()
}
void
-ArchiveAccountManager::startSync(const OnNewDeviceCb& cb, const OnDeviceAnnouncedCb& dcb)
+ArchiveAccountManager::startSync(const OnNewDeviceCb& cb, const OnDeviceAnnouncedCb& dcb, bool publishPresence)
{
- AccountManager::startSync(std::move(cb), std::move(dcb));
+ AccountManager::startSync(std::move(cb), std::move(dcb), publishPresence);
dht_->listen<DeviceSync>(
dht::InfoHash::get("inbox:" + info_->devicePk->getId().toString()),
diff --git a/src/jamidht/archive_account_manager.h b/src/jamidht/archive_account_manager.h
index 4437dc5e9f..55d9b30aea 100644
--- a/src/jamidht/archive_account_manager.h
+++ b/src/jamidht/archive_account_manager.h
@@ -49,7 +49,7 @@ public:
AuthFailureCallback onFailure,
const OnChangeCallback& onChange) override;
- void startSync(const OnNewDeviceCb&, const OnDeviceAnnouncedCb& dcb = {}) override;
+ void startSync(const OnNewDeviceCb&, const OnDeviceAnnouncedCb& dcb = {}, bool publishPresence = true) override;
bool changePassword(const std::string& password_old, const std::string& password_new) override;
diff --git a/src/jamidht/conversation_module.cpp b/src/jamidht/conversation_module.cpp
index 40a043f6bd..704e57a485 100644
--- a/src/jamidht/conversation_module.cpp
+++ b/src/jamidht/conversation_module.cpp
@@ -1573,20 +1573,20 @@ ConversationModule::onTrustRequest(const std::string& uri,
if (getOneToOneConversation(uri) != "") {
// If there is already an active one to one conversation here, it's an active
// contact and the contact will reclone this activeConv, so ignore the request
- JAMI_WARN("Contact is sending a request for a non active conversation. Ignore. They will "
+ JAMI_WARNING("Contact is sending a request for a non active conversation. Ignore. They will "
"clone the old one");
return;
}
if (pimpl_->isAcceptedConversation(conversationId)) {
- JAMI_INFO("[Account %s] Received a request for a conversation "
+ JAMI_DEBUG("[Account {}] Received a request for a conversation "
"already handled. Ignore",
- pimpl_->accountId_.c_str());
+ pimpl_->accountId_);
return;
}
if (pimpl_->getRequest(conversationId) != std::nullopt) {
- JAMI_INFO("[Account %s] Received a request for a conversation "
+ JAMI_DEBUG("[Account {}] Received a request for a conversation "
"already existing. Ignore",
- pimpl_->accountId_.c_str());
+ pimpl_->accountId_);
return;
}
ConversationRequest req;
@@ -1612,26 +1612,44 @@ void
ConversationModule::onConversationRequest(const std::string& from, const Json::Value& value)
{
ConversationRequest req(value);
- JAMI_INFO("[Account %s] Receive a new conversation request for conversation %s from %s",
- pimpl_->accountId_.c_str(),
- req.conversationId.c_str(),
- from.c_str());
+ JAMI_DEBUG("[Account {}] Receive a new conversation request for conversation {} from {}",
+ pimpl_->accountId_,
+ req.conversationId,
+ from);
auto convId = req.conversationId;
req.from = from;
// Already accepted request, do nothing
- if (pimpl_->isAcceptedConversation(convId)) {
+ if (pimpl_->isAcceptedConversation(convId))
return;
- }
if (pimpl_->getRequest(convId) != std::nullopt) {
- JAMI_INFO("[Account %s] Received a request for a conversation already existing. "
+ JAMI_DEBUG("[Account {}] Received a request for a conversation already existing. "
"Ignore",
- pimpl_->accountId_.c_str());
+ pimpl_->accountId_);
return;
}
req.received = std::time(nullptr);
auto reqMap = req.toMap();
auto isOneToOne = req.isOneToOne();
+ std::string oldConv;
+ auto acc = pimpl_->account_.lock();
+ if (acc && isOneToOne) {
+ auto oldConv = getOneToOneConversation(from);
+ if (!oldConv.empty()) {
+ // Already a conversation with the contact.
+ if (oldConv != convId && acc->updateConvForContact(from, oldConv, convId)) {
+ initReplay(oldConv, convId);
+ cloneConversationFrom(convId, from, oldConv);
+ return;
+ }
+ // If there is already an active one to one conversation here, it's an active
+ // contact and the contact will reclone this activeConv, so ignore the request
+ JAMI_WARNING("Contact is sending a request for a non active conversation. Ignore. They will "
+ "clone the old one");
+ return;
+ }
+ }
+
if (pimpl_->addConversationRequest(convId, std::move(req))) {
// Note: no need to sync here because other connected devices should receive
// the same conversation request. Will sync when the conversation will be added
diff --git a/src/jamidht/jamiaccount.cpp b/src/jamidht/jamiaccount.cpp
index 6f64e3ef02..55efcf4dca 100644
--- a/src/jamidht/jamiaccount.cpp
+++ b/src/jamidht/jamiaccount.cpp
@@ -1926,7 +1926,8 @@ JamiAccount::doRegister_()
emitSignal<libjami::ConfigurationSignal::VolatileDetailsChanged>(
accountID_, getVolatileAccountDetails());
}
- });
+ },
+ publishPresence_);
};
setRegistrationState(RegistrationState::TRYING);
diff --git a/src/jamidht/jamiaccount.h b/src/jamidht/jamiaccount.h
index edbb3dc78d..2747ce442b 100644
--- a/src/jamidht/jamiaccount.h
+++ b/src/jamidht/jamiaccount.h
@@ -438,6 +438,10 @@ public:
{
noSha3sumVerification_ = newValue;
}
+
+ void publishPresence(bool newValue) {
+ publishPresence_ = newValue;
+ }
#endif
/**
@@ -868,6 +872,7 @@ private:
std::atomic_bool deviceAnnounced_ {false};
bool noSha3sumVerification_ {false};
+ bool publishPresence_ {true};
std::map<Uri::Scheme, std::unique_ptr<ChannelHandlerInterface>> channelHandlers_ {};
diff --git a/test/unitTest/conversation/conversationRequest.cpp b/test/unitTest/conversation/conversationRequest.cpp
index 984f961d4f..646ddfc7a3 100644
--- a/test/unitTest/conversation/conversationRequest.cpp
+++ b/test/unitTest/conversation/conversationRequest.cpp
@@ -77,6 +77,7 @@ public:
void testCacheRequestFromClient();
void testNeedsSyncingWithForCloning();
void testRemoveContactRemoveTrustRequest();
+ void testAddConversationNoPresenceThenConnects();
std::string aliceId;
std::string bobId;
std::string bob2Id;
@@ -107,6 +108,7 @@ private:
CPPUNIT_TEST(testCacheRequestFromClient);
CPPUNIT_TEST(testNeedsSyncingWithForCloning);
CPPUNIT_TEST(testRemoveContactRemoveTrustRequest);
+ CPPUNIT_TEST(testAddConversationNoPresenceThenConnects);
CPPUNIT_TEST_SUITE_END();
};
@@ -1591,6 +1593,63 @@ ConversationRequestTest::testRemoveContactRemoveTrustRequest()
CPPUNIT_ASSERT(bob2Account->getTrustRequests().size() == 0);
}
+void
+ConversationRequestTest::testAddConversationNoPresenceThenConnects()
+{
+ auto aliceAccount = Manager::instance().getAccount<JamiAccount>(aliceId);
+ auto carlaAccount = Manager::instance().getAccount<JamiAccount>(carlaId);
+ auto carlaUri = carlaAccount->getUsername();
+ auto aliceUri = aliceAccount->getUsername();
+ aliceAccount->trackBuddyPresence(carlaUri, true);
+ carlaAccount->publishPresence(false);
+
+ std::mutex mtx;
+ std::unique_lock<std::mutex> lk {mtx};
+ std::condition_variable cv;
+ std::map<std::string, std::shared_ptr<libjami::CallbackWrapperBase>> confHandlers;
+ std::string convId = "";
+ confHandlers.insert(libjami::exportable_callback<libjami::ConversationSignal::ConversationReady>(
+ [&](const std::string& accountId, const std::string& conversationId) {
+ if (accountId == aliceId) {
+ convId = conversationId;
+ }
+ cv.notify_one();
+ }));
+ auto carlaConnected = false;
+ confHandlers.insert(
+ libjami::exportable_callback<libjami::ConfigurationSignal::VolatileDetailsChanged>(
+ [&](const std::string&, const std::map<std::string, std::string>&) {
+ auto details = carlaAccount->getVolatileAccountDetails();
+ auto daemonStatus = details[libjami::Account::ConfProperties::Registration::STATUS];
+ if (daemonStatus == "REGISTERED") {
+ carlaConnected = true;
+ } else if (daemonStatus == "UNREGISTERED") {
+ carlaConnected = false;
+ }
+ cv.notify_one();
+ }));
+ libjami::registerSignalHandlers(confHandlers);
+ aliceAccount->addContact(carlaUri);
+ cv.wait_for(lk, 5s); // Wait 5 secs for the put to happen
+ Manager::instance().sendRegister(carlaId, true);
+ CPPUNIT_ASSERT(cv.wait_for(lk, 30s, [&]() { return carlaConnected; }));
+ carlaAccount->addContact(aliceUri);
+ cv.wait_for(lk, 5s); // Wait 5 secs for the put to happen
+ carlaAccount->publishPresence(true);
+
+ Manager::instance().sendRegister(carlaId, false);
+ CPPUNIT_ASSERT(cv.wait_for(lk, 30s, [&]() { return !carlaConnected; }));
+ convId.clear();
+ Manager::instance().sendRegister(carlaId, true);
+ CPPUNIT_ASSERT(cv.wait_for(lk, 30s, [&]() { return carlaConnected; }));
+ CPPUNIT_ASSERT(
+ cv.wait_for(lk, 30s, [&]() { return !convId.empty(); }));
+
+ auto carlaDetails = carlaAccount->getContactDetails(aliceUri);
+ auto aliceDetails = aliceAccount->getContactDetails(carlaUri);
+ CPPUNIT_ASSERT(carlaDetails["conversationId"] == aliceDetails["conversationId"] && carlaDetails["conversationId"] == convId);
+}
+
} // namespace test
} // namespace jami
--
GitLab