From 0f6b74b1945951c45a7bcad4f8ff2bed1219f3b9 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?S=C3=A9bastien=20Blin?=
<sebastien.blin@savoirfairelinux.com>
Date: Tue, 11 Oct 2022 16:22:23 -0400
Subject: [PATCH] conversation: add support for message edition
For now, only be able to edit messages from our-self and with the
text/plain type
Change-Id: I20c6ed7dad3892f0fa655812242b9ca6e77e69b3
GitLab: #316
---
.../cx.ring.Ring.ConfigurationManager.xml | 9 +-
bin/dbus/dbusconfigurationmanager.cpp | 5 +-
bin/dbus/dbusconfigurationmanager.h | 3 +-
bin/jni/conversation.i | 2 +-
bin/nodejs/conversation.i | 2 +-
configure.ac | 2 +-
meson.build | 2 +-
src/client/conversation_interface.cpp | 12 ++-
src/jami/conversation_interface.h | 3 +-
src/jamidht/conversation_module.cpp | 42 ++++++++
src/jamidht/conversation_module.h | 4 +
src/jamidht/conversationrepository.cpp | 54 ++++++++++-
test/unitTest/conversation/conversation.cpp | 96 +++++++++++++++++++
13 files changed, 218 insertions(+), 18 deletions(-)
diff --git a/bin/dbus/cx.ring.Ring.ConfigurationManager.xml b/bin/dbus/cx.ring.Ring.ConfigurationManager.xml
index 9b3dd77607..469048349e 100644
--- a/bin/dbus/cx.ring.Ring.ConfigurationManager.xml
+++ b/bin/dbus/cx.ring.Ring.ConfigurationManager.xml
@@ -1774,14 +1774,17 @@
</method>
<method name="sendMessage" tp:name-for-bindings="sendMessage">
- <tp:added version="10.0.0"/>
+ <tp:added version="13.6.0"/>
<tp:docstring>
- Add a message to a conversation
+ Add a message to a conversation.
+ Flag: 0 = reply (if commitId is not empty), else single message
+ 1 = edit message
</tp:docstring>
<arg type="s" name="accountId" direction="in"/>
<arg type="s" name="conversationId" direction="in"/>
<arg type="s" name="message" direction="in"/>
- <arg type="s" name="replyTo" direction="in"/>
+ <arg type="s" name="commitId" direction="in"/>
+ <arg type="i" name="flag" direction="in"/>
</method>
<method name="loadConversationMessages" tp:name-for-bindings="loadConversationMessages">
diff --git a/bin/dbus/dbusconfigurationmanager.cpp b/bin/dbus/dbusconfigurationmanager.cpp
index 3effef201e..57466c0c9e 100644
--- a/bin/dbus/dbusconfigurationmanager.cpp
+++ b/bin/dbus/dbusconfigurationmanager.cpp
@@ -921,9 +921,10 @@ void
DBusConfigurationManager::sendMessage(const std::string& accountId,
const std::string& conversationId,
const std::string& message,
- const std::string& replyTo)
+ const std::string& replyTo,
+ const int32_t& flag)
{
- DRing::sendMessage(accountId, conversationId, message, replyTo);
+ DRing::sendMessage(accountId, conversationId, message, replyTo, flag);
}
uint32_t
diff --git a/bin/dbus/dbusconfigurationmanager.h b/bin/dbus/dbusconfigurationmanager.h
index b145487307..f317cd4363 100644
--- a/bin/dbus/dbusconfigurationmanager.h
+++ b/bin/dbus/dbusconfigurationmanager.h
@@ -281,7 +281,8 @@ public:
void sendMessage(const std::string& accountId,
const std::string& conversationId,
const std::string& message,
- const std::string& replyTo);
+ const std::string& replyTo,
+ const int32_t& flag);
uint32_t loadConversationMessages(const std::string& accountId,
const std::string& conversationId,
const std::string& fromMessage,
diff --git a/bin/jni/conversation.i b/bin/jni/conversation.i
index 4437faaf3e..6a628904cf 100644
--- a/bin/jni/conversation.i
+++ b/bin/jni/conversation.i
@@ -61,7 +61,7 @@ namespace DRing {
std::vector<std::map<std::string, std::string>> getConversationMembers(const std::string& accountId, const std::string& conversationId);
// Message send/load
- void sendMessage(const std::string& accountId, const std::string& conversationId, const std::string& message, const std::string& replyTo);
+ void sendMessage(const std::string& accountId, const std::string& conversationId, const std::string& message, const std::string& replyTo, const int32_t& flag);
uint32_t loadConversationMessages(const std::string& accountId, const std::string& conversationId, const std::string& fromMessage, size_t n);
uint32_t loadConversationUntil(const std::string& accountId, const std::string& conversationId, const std::string& fromMessage, const std::string& toMessage);
uint32_t countInteractions(const std::string& accountId, const std::string& conversationId, const std::string& toId, const std::string& fromId, const std::string& authorUri);
diff --git a/bin/nodejs/conversation.i b/bin/nodejs/conversation.i
index f645253f66..ac254bcd26 100644
--- a/bin/nodejs/conversation.i
+++ b/bin/nodejs/conversation.i
@@ -61,7 +61,7 @@ namespace DRing {
std::vector<std::map<std::string, std::string>> getConversationMembers(const std::string& accountId, const std::string& conversationId);
// Message send/load
- void sendMessage(const std::string& accountId, const std::string& conversationId, const std::string& message, const std::string& replyTo);
+ void sendMessage(const std::string& accountId, const std::string& conversationId, const std::string& message, const std::string& replyTo, const int32_t& flag);
uint32_t loadConversationMessages(const std::string& accountId, const std::string& conversationId, const std::string& fromMessage, size_t n);
uint32_t loadConversationUntil(const std::string& accountId, const std::string& conversationId, const std::string& fromMessage, const std::string& toMessage);
uint32_t countInteractions(const std::string& accountId, const std::string& conversationId, const std::string& toId, const std::string& fromId, const std::string& authorUri);
diff --git a/configure.ac b/configure.ac
index 3988e089f5..27215d0446 100644
--- a/configure.ac
+++ b/configure.ac
@@ -2,7 +2,7 @@ dnl Jami - configure.ac
dnl Process this file with autoconf to produce a configure script.
AC_PREREQ([2.69])
-AC_INIT([Jami Daemon],[13.5.0],[jami@gnu.org],[jami])
+AC_INIT([Jami Daemon],[13.6.0],[jami@gnu.org],[jami])
dnl Clear the implicit flags that default to '-g -O2', otherwise they
dnl take precedence over the values we set via the
diff --git a/meson.build b/meson.build
index bd14e1b377..3312907c3c 100644
--- a/meson.build
+++ b/meson.build
@@ -1,5 +1,5 @@
project('jami-daemon', ['c', 'cpp'],
- version: '13.5.0',
+ version: '13.6.0',
license: 'GPL3+',
default_options: ['cpp_std=gnu++17', 'buildtype=debugoptimized'],
meson_version:'>= 0.56'
diff --git a/src/client/conversation_interface.cpp b/src/client/conversation_interface.cpp
index 47607790a7..7312f3f2d9 100644
--- a/src/client/conversation_interface.cpp
+++ b/src/client/conversation_interface.cpp
@@ -160,11 +160,17 @@ void
sendMessage(const std::string& accountId,
const std::string& conversationId,
const std::string& message,
- const std::string& replyTo)
+ const std::string& commitId,
+ const int32_t& flag)
{
if (auto acc = jami::Manager::instance().getAccount<jami::JamiAccount>(accountId))
- if (auto convModule = acc->convModule())
- convModule->sendMessage(conversationId, message, replyTo);
+ if (auto convModule = acc->convModule()) {
+ if (flag == 0 /* Reply or simple commit */) {
+ convModule->sendMessage(conversationId, message, commitId);
+ } else if (flag == 1 /* message edition */) {
+ convModule->editMessage(conversationId, message, commitId);
+ }
+ }
}
uint32_t
diff --git a/src/jami/conversation_interface.h b/src/jami/conversation_interface.h
index bb05c1920a..404b7332c9 100644
--- a/src/jami/conversation_interface.h
+++ b/src/jami/conversation_interface.h
@@ -69,7 +69,8 @@ DRING_PUBLIC std::vector<std::map<std::string, std::string>> getConversationMemb
DRING_PUBLIC void sendMessage(const std::string& accountId,
const std::string& conversationId,
const std::string& message,
- const std::string& replyTo);
+ const std::string& replyTo,
+ const int32_t& flag = 0);
DRING_PUBLIC uint32_t loadConversationMessages(const std::string& accountId,
const std::string& conversationId,
const std::string& fromMessage,
diff --git a/src/jamidht/conversation_module.cpp b/src/jamidht/conversation_module.cpp
index f9850f904a..6bebba879e 100644
--- a/src/jamidht/conversation_module.cpp
+++ b/src/jamidht/conversation_module.cpp
@@ -227,6 +227,10 @@ public:
OnCommitCb&& onCommit = {},
OnDoneCb&& cb = {});
+ void editMessage(const std::string& conversationId,
+ const std::string& newBody,
+ const std::string& editedId);
+
// The following informations are stored on the disk
mutable std::mutex convInfosMtx_; // Note, should be locked after conversationsMtx_ if needed
std::map<std::string, ConvInfo> convInfos_;
@@ -842,6 +846,36 @@ ConversationModule::Impl::sendMessage(const std::string& conversationId,
}
}
+void
+ConversationModule::Impl::editMessage(const std::string& conversationId,
+ const std::string& newBody,
+ const std::string& editedId)
+{
+ // Check that editedId is a valid commit, from ourself and plain/text
+ auto validCommit = false;
+ {
+ std::lock_guard<std::mutex> lk(conversationsMtx_);
+ auto conversation = conversations_.find(conversationId);
+ if (conversation != conversations_.end() && conversation->second) {
+ auto commit = conversation->second->getCommit(editedId);
+ if (commit != std::nullopt) {
+ validCommit = commit->at("author") == username_
+ && commit->at("type") == "text/plain";
+ }
+ }
+ }
+ if (!validCommit) {
+ JAMI_ERROR("Cannot edit commit {:s}", editedId);
+ return;
+ }
+ // Commit message edition
+ Json::Value json;
+ json["body"] = newBody;
+ json["edit"] = editedId;
+ json["type"] = "application/edited-message";
+ sendMessage(conversationId, std::move(json));
+}
+
////////////////////////////////////////////////////////////////
void
@@ -1306,6 +1340,14 @@ ConversationModule::sendMessage(const std::string& conversationId,
std::move(cb));
}
+void
+ConversationModule::editMessage(const std::string& conversationId,
+ const std::string& newBody,
+ const std::string& editedId)
+{
+ pimpl_->editMessage(conversationId, newBody, editedId);
+}
+
void
ConversationModule::addCallHistoryMessage(const std::string& uri, uint64_t duration_ms)
{
diff --git a/src/jamidht/conversation_module.h b/src/jamidht/conversation_module.h
index 83f3fcd153..7dee281aaf 100644
--- a/src/jamidht/conversation_module.h
+++ b/src/jamidht/conversation_module.h
@@ -160,6 +160,10 @@ public:
OnCommitCb&& onCommit = {},
OnDoneCb&& cb = {});
+ void editMessage(const std::string& conversationId,
+ const std::string& newBody,
+ const std::string& editedId);
+
/**
* Add to the related conversation the call history message
* @param uri Peer number
diff --git a/src/jamidht/conversationrepository.cpp b/src/jamidht/conversationrepository.cpp
index b11d9cc19c..f88067231f 100644
--- a/src/jamidht/conversationrepository.cpp
+++ b/src/jamidht/conversationrepository.cpp
@@ -102,6 +102,7 @@ public:
bool checkVote(const std::string& userDevice,
const std::string& commitId,
const std::string& parentId) const;
+ bool checkEdit(const std::string& userDevice, const ConversationCommit& commit) const;
bool isValidUserAtCommit(const std::string& userDevice, const std::string& commitId) const;
bool checkInitialCommit(const std::string& userDevice,
const std::string& commitId,
@@ -178,6 +179,15 @@ public:
return members_;
}
+ std::optional<ConversationCommit> getCommit(const std::string& commitId,
+ bool logIfNotFound = true) const
+ {
+ auto commits = log(commitId, "", 1, logIfNotFound);
+ if (commits.empty())
+ return std::nullopt;
+ return std::move(commits[0]);
+ }
+
bool resolveConflicts(git_index* index, const std::string& other_id);
std::vector<std::string> memberUris(std::string_view filter,
@@ -862,6 +872,36 @@ ConversationRepository::Impl::checkValidUserDiff(const std::string& userDevice,
return true;
}
+bool
+ConversationRepository::Impl::checkEdit(const std::string& userDevice,
+ const ConversationCommit& commit) const
+{
+ auto userUri = uriFromDevice(userDevice);
+ // Check that edited commit is found, for the same author, and editable (plain/text)
+ auto commitMap = convCommitToMap(commit);
+ if (commitMap == std::nullopt) {
+ return false;
+ }
+ auto editedId = commitMap->at("edit");
+ auto editedCommit = getCommit(editedId);
+ if (editedCommit == std::nullopt) {
+ JAMI_ERROR("Commit {:s} not found", editedId);
+ return false;
+ }
+ auto editedCommitMap = convCommitToMap(*editedCommit);
+ if (editedCommitMap == std::nullopt or editedCommitMap->at("author").empty()
+ or editedCommitMap->at("author") != commitMap->at("author")
+ or commitMap->at("author") != userUri) {
+ JAMI_ERROR("Edited commit {:s} got a different author ({:s})", editedId, commit.id);
+ return false;
+ }
+ if (editedCommitMap->at("type") != "text/plain") {
+ JAMI_ERROR("Edited commit {:s} is not text!", editedId);
+ return false;
+ }
+ return true;
+}
+
bool
ConversationRepository::Impl::checkVote(const std::string& userDevice,
const std::string& commitId,
@@ -2630,6 +2670,15 @@ ConversationRepository::Impl::validCommits(
}
return false;
}
+ } else if (type == "application/edited-message") {
+ if (!checkEdit(userDevice, commit)) {
+ JAMI_ERROR("Commit {:s} malformed", commit.id);
+ if (auto shared = account_.lock()) {
+ emitSignal<DRing::ConversationSignal::OnConversationError>(
+ shared->getAccountID(), id_, EVALIDFETCH, "Malformed edit commit");
+ }
+ return false;
+ }
} else {
// Note: accept all mimetype here, as we can have new mimetypes
// Just avoid to add weird files
@@ -2970,10 +3019,7 @@ ConversationRepository::search(const Filter& filter) const
std::optional<ConversationCommit>
ConversationRepository::getCommit(const std::string& commitId, bool logIfNotFound) const
{
- auto commits = logN(commitId, 1, logIfNotFound);
- if (commits.empty())
- return std::nullopt;
- return std::move(commits[0]);
+ return pimpl_->getCommit(commitId, logIfNotFound);
}
std::pair<bool, std::string>
diff --git a/test/unitTest/conversation/conversation.cpp b/test/unitTest/conversation/conversation.cpp
index 1f92807655..c5c7f9995a 100644
--- a/test/unitTest/conversation/conversation.cpp
+++ b/test/unitTest/conversation/conversation.cpp
@@ -116,6 +116,7 @@ private:
void testConversationPreferencesMultiDevices();
void testFixContactDetails();
void testRemoveOneToOneNotInDetails();
+ void testMessageEdition();
CPPUNIT_TEST_SUITE(ConversationTest);
CPPUNIT_TEST(testCreateConversation);
@@ -162,6 +163,7 @@ private:
CPPUNIT_TEST(testConversationPreferencesMultiDevices);
CPPUNIT_TEST(testFixContactDetails);
CPPUNIT_TEST(testRemoveOneToOneNotInDetails);
+ CPPUNIT_TEST(testMessageEdition);
CPPUNIT_TEST_SUITE_END();
};
@@ -3397,6 +3399,100 @@ ConversationTest::testRemoveOneToOneNotInDetails()
CPPUNIT_ASSERT(cv.wait_for(lk, 30s, [&]() { return conversationRemoved; }));
}
+void
+ConversationTest::testMessageEdition()
+{
+ auto aliceAccount = Manager::instance().getAccount<JamiAccount>(aliceId);
+ auto bobAccount = Manager::instance().getAccount<JamiAccount>(bobId);
+ auto bobUri = bobAccount->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;
+ std::vector<std::map<std::string, std::string>> messageBobReceived;
+ bool conversationReady = false, memberMessageGenerated = false;
+ confHandlers.insert(DRing::exportable_callback<DRing::ConversationSignal::MessageReceived>(
+ [&](const std::string& accountId,
+ const std::string& /* conversationId */,
+ std::map<std::string, std::string> message) {
+ if (accountId == bobId) {
+ messageBobReceived.emplace_back(message);
+ JAMI_ERR("@@@ EMPLACE!");
+ } else if (accountId == aliceId && message["type"] == "member") {
+ memberMessageGenerated = true;
+ }
+ cv.notify_one();
+ }));
+ bool requestReceived = false;
+ confHandlers.insert(
+ DRing::exportable_callback<DRing::ConversationSignal::ConversationRequestReceived>(
+ [&](const std::string& /*accountId*/,
+ const std::string& /* conversationId */,
+ std::map<std::string, std::string> /*metadatas*/) {
+ requestReceived = true;
+ cv.notify_one();
+ }));
+ confHandlers.insert(DRing::exportable_callback<DRing::ConversationSignal::ConversationReady>(
+ [&](const std::string& accountId, const std::string& /* conversationId */) {
+ if (accountId == bobId) {
+ conversationReady = true;
+ cv.notify_one();
+ }
+ }));
+ auto errorDetected = false;
+ confHandlers.insert(DRing::exportable_callback<DRing::ConversationSignal::OnConversationError>(
+ [&](const std::string& /* accountId */,
+ const std::string& /* conversationId */,
+ int code,
+ const std::string& /* what */) {
+ if (code == 3)
+ errorDetected = true;
+ cv.notify_one();
+ }));
+ DRing::registerSignalHandlers(confHandlers);
+ auto convId = DRing::startConversation(aliceId);
+ DRing::addConversationMember(aliceId, convId, bobUri);
+ CPPUNIT_ASSERT(cv.wait_for(lk, 30s, [&]() { return requestReceived; }));
+ memberMessageGenerated = false;
+ DRing::acceptConversationRequest(bobId, convId);
+ CPPUNIT_ASSERT(
+ cv.wait_for(lk, 30s, [&]() { return conversationReady && memberMessageGenerated; }));
+ auto msgSize = messageBobReceived.size();
+ JAMI_ERR("@@@@@@@@ %u", msgSize);
+ DRing::sendMessage(aliceId, convId, "hi"s, "");
+ CPPUNIT_ASSERT(cv.wait_for(lk, 30s, [&]() { return messageBobReceived.size() == msgSize + 1; }));
+ msgSize = messageBobReceived.size();
+ auto editedId = messageBobReceived.rbegin()->at("id");
+ DRing::sendMessage(aliceId, convId, "New body"s, editedId, 1);
+ CPPUNIT_ASSERT(cv.wait_for(lk, 10s, [&]() { return messageBobReceived.size() == msgSize + 1; }));
+ CPPUNIT_ASSERT(messageBobReceived.rbegin()->at("edit") == editedId);
+ CPPUNIT_ASSERT(messageBobReceived.rbegin()->at("body") == "New body");
+ // Not an existing message
+ msgSize = messageBobReceived.size();
+ DRing::sendMessage(aliceId, convId, "New body"s, "invalidId", 1);
+ CPPUNIT_ASSERT(
+ !cv.wait_for(lk, 10s, [&]() { return messageBobReceived.size() == msgSize + 1; }));
+ // Invalid author
+ DRing::sendMessage(aliceId, convId, "New body"s, convId, 1);
+ CPPUNIT_ASSERT(
+ !cv.wait_for(lk, 10s, [&]() { return messageBobReceived.size() == msgSize + 1; }));
+ // Add invalid edition
+ Json::Value root;
+ root["type"] = "application/edited-message";
+ root["edit"] = convId;
+ root["body"] = "new";
+ Json::StreamWriterBuilder wbuilder;
+ wbuilder["commentStyle"] = "None";
+ wbuilder["indentation"] = "";
+ auto repoPath = fileutils::get_data_dir() + DIR_SEPARATOR_STR + aliceAccount->getAccountID()
+ + DIR_SEPARATOR_STR + "conversations" + DIR_SEPARATOR_STR + convId;
+ auto message = Json::writeString(wbuilder, root);
+ commitInRepo(repoPath, aliceAccount, message);
+ errorDetected = false;
+ DRing::sendMessage(aliceId, convId, "trigger"s, "");
+ CPPUNIT_ASSERT(cv.wait_for(lk, 30s, [&]() { return errorDetected; }));
+}
+
} // namespace test
} // namespace jami
--
GitLab