Merge lp:~tiagosh/telephony-service/fix-1482401 into lp:telephony-service

[Tiago Salem Herrmann]

Change existingChat() to existingChannels().

Change existingChat() to existingChannels().
There might be more than on channel for the same recipient, as phone numbers can be slightly different.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1113
http://jenkins.qa.ubuntu.com/job/telephony-service-ci/541/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/telephony-service-vivid-amd64-ci/213
    SUCCESS: http://jenkins.qa.ubuntu.com/job/telephony-service-vivid-armhf-ci/209
        deb: http://jenkins.qa.ubuntu.com/job/telephony-service-vivid-armhf-ci/209/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/telephony-service-vivid-i386-ci/209

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/telephony-service-ci/541/rebuild

review: Approve (continuous-integration)

[Gustavo Pichorim Boiko (boiko)]

Looks good!

review: Approve

=== modified file 'handler/texthandler.cpp'
--- handler/texthandler.cpp	2015-06-05 20:36:01 +0000
+++ handler/texthandler.cpp	2015-08-18 13:23:27 +0000
@@ -93,7 +93,6 @@
                 startChat(recipients, accountId);
             }
         }
-
     }
 }
 
@@ -216,8 +215,8 @@
         return;
     }
 
-    Tp::TextChannelPtr channel = existingChat(recipients, accountId);
-    if (channel.isNull()) {
+    QList<Tp::TextChannelPtr> channels = existingChannels(recipients, accountId);
+    if (channels.isEmpty()) {
         mPendingMMSs[accountId][recipients].append(attachments);
         startChat(recipients, accountId);
         return;
@@ -225,7 +224,7 @@
 
     Tp::MessagePartList message = buildMMS(attachments);
 
-    connect(channel->send(message),
+    connect(channels.last()->send(message),
             SIGNAL(finished(Tp::PendingOperation*)),
             SLOT(onMessageSent(Tp::PendingOperation*)));
 }
@@ -241,8 +240,8 @@
         mPendingSilentMessages[accountId][recipients].append(message);
         return;
     }
-    Tp::TextChannelPtr channel = existingChat(recipients, accountId);
-    if (channel.isNull()) {
+    QList<Tp::TextChannelPtr> channels = existingChannels(recipients, accountId);
+    if (channels.isEmpty()) {
         mPendingSilentMessages[accountId][recipients].append(message);
         startChat(recipients, accountId);
         return;
@@ -258,7 +257,7 @@
     body["content-type"] = QDBusVariant("text/plain");
     fullMessage << header << body;
 
-    connect(channel->send(fullMessage),
+    connect(channels.last()->send(fullMessage),
             SIGNAL(finished(Tp::PendingOperation*)),
             SLOT(onMessageSent(Tp::PendingOperation*)));
 }
@@ -294,33 +293,34 @@
         return;
     }
 
-    Tp::TextChannelPtr channel = existingChat(recipients, account->accountId());
-    if (channel.isNull()) {
+    QList<Tp::TextChannelPtr> channels = existingChannels(recipients, account->accountId());
+    if (channels.isEmpty()) {
         mPendingMessages[account->accountId()][recipients].append(message);
         startChat(recipients, account->accountId());
         return;
     }
 
-    connect(channel->send(message),
+    connect(channels.last()->send(message),
             SIGNAL(finished(Tp::PendingOperation*)),
             SLOT(onMessageSent(Tp::PendingOperation*)));
 }
 
 void TextHandler::acknowledgeMessages(const QStringList &recipients, const QStringList &messageIds, const QString &accountId)
 {
-    Tp::TextChannelPtr channel = existingChat(recipients, accountId);
-    if (channel.isNull()) {
+    QList<Tp::TextChannelPtr> channels = existingChannels(recipients, accountId);
+    if (channels.isEmpty()) {
         return;
     }
 
     QList<Tp::ReceivedMessage> messagesToAck;
-    Q_FOREACH(const Tp::ReceivedMessage &message, channel->messageQueue()) {
-        if (messageIds.contains(message.messageToken())) {
-            messagesToAck.append(message);
+    Q_FOREACH(const Tp::TextChannelPtr &channel, channels) {
+        Q_FOREACH(const Tp::ReceivedMessage &message, channel->messageQueue()) {
+            if (messageIds.contains(message.messageToken())) {
+                messagesToAck.append(message);
+            }
         }
+        channel->acknowledge(messagesToAck);
     }
-
-    channel->acknowledge(messagesToAck);
 }
 
 void TextHandler::onTextChannelAvailable(Tp::TextChannelPtr channel)
@@ -347,36 +347,54 @@
     QMap<QStringList, QStringList> &pendingMessages = mPendingMessages[accountId];
     QMap<QStringList, QStringList>::iterator it = pendingMessages.begin();
     while (it != pendingMessages.end()) {
-        if (existingChat(it.key(), accountId) == channel) {
-            Q_FOREACH(const QString &message, it.value()) {
-                sendMessage(recipients, message, accountId);
+        bool found = false;
+        Q_FOREACH(const Tp::TextChannelPtr &existingChannel, existingChannels(it.key(), accountId)) {
+            if (existingChannel == channel) {
+                Q_FOREACH(const QString &message, it.value()) {
+                    sendMessage(recipients, message, accountId);
+                }
+                it = pendingMessages.erase(it);
+                found = true;
+                break;
             }
-            it = pendingMessages.erase(it);
-        } else {
+        }
+        if (!found) {
             ++it;
         }
     }
     QMap<QStringList, QList<AttachmentList>> &pendingMMSs = mPendingMMSs[accountId];
     QMap<QStringList, QList<AttachmentList>>::iterator it2 = pendingMMSs.begin();
     while (it2 != pendingMMSs.end()) {
-        if (existingChat(it2.key(), accountId) == channel) {
-            Q_FOREACH(const AttachmentList &attachments, it2.value()) {
-                sendMMS(recipients, attachments, accountId);
+        bool found = false;
+        Q_FOREACH(const Tp::TextChannelPtr &existingChannel, existingChannels(it2.key(), accountId)) {
+            if (existingChannel == channel) {
+                Q_FOREACH(const AttachmentList &attachments, it2.value()) {
+                    sendMMS(recipients, attachments, accountId);
+                }
+                it2 = pendingMMSs.erase(it2);
+                found = true;
+                break;
             }
-            it2 = pendingMMSs.erase(it2);
-        } else {
+        }
+        if (!found) {
             ++it2;
         }
     }
     QMap<QStringList, QStringList> &pendingSilentMessages = mPendingSilentMessages[accountId];
     QMap<QStringList, QStringList>::iterator it3 = pendingSilentMessages.begin();
     while (it3 != pendingSilentMessages.end()) {
-        if (existingChat(it3.key(), accountId) == channel) {
-            Q_FOREACH(const QString &message, it3.value()) {
-                sendSilentMessage(recipients, message, accountId);
+        bool found = false;
+        Q_FOREACH(const Tp::TextChannelPtr &existingChannel, existingChannels(it3.key(), accountId)) {
+            if (existingChannel == channel) {
+                Q_FOREACH(const QString &message, it3.value()) {
+                    sendSilentMessage(recipients, message, accountId);
+                }
+                it3 = pendingSilentMessages.erase(it3);
+                found = true;
+                break;
             }
-            it3 = pendingSilentMessages.erase(it3);
-        } else {
+        }
+        if (!found) {
             ++it3;
         }
     }
@@ -396,9 +414,9 @@
     }
 }
 
-Tp::TextChannelPtr TextHandler::existingChat(const QStringList &targetIds, const QString &accountId)
+QList<Tp::TextChannelPtr> TextHandler::existingChannels(const QStringList &targetIds, const QString &accountId)
 {
-    Tp::TextChannelPtr channel;
+    QList<Tp::TextChannelPtr> channels;
 
     Q_FOREACH(const Tp::TextChannelPtr &channel, mChannels) {
         int count = 0;
@@ -415,10 +433,10 @@
             }
         }
         if (count == targetIds.size()) {
-            return channel;
+            channels.append(channel);
         }
     }
-    return channel;
+    return channels;
 }
 
 void TextHandler::onContactsAvailable(Tp::PendingOperation *op)

=== modified file 'handler/texthandler.h'
--- handler/texthandler.h	2015-03-04 18:02:13 +0000
+++ handler/texthandler.h	2015-08-18 13:23:27 +0000
@@ -49,7 +49,7 @@
     void onConnectedChanged();
 
 protected:
-    Tp::TextChannelPtr existingChat(const QStringList &targetIds, const QString &accountId);
+    QList<Tp::TextChannelPtr> existingChannels(const QStringList &targetIds, const QString &accountId);
 
 private:
     explicit TextHandler(QObject *parent = 0);

=== modified file 'tests/handler/HandlerTest.cpp'
--- tests/handler/HandlerTest.cpp	2015-07-23 21:08:40 +0000
+++ tests/handler/HandlerTest.cpp	2015-08-18 13:23:27 +0000
@@ -19,6 +19,7 @@
 #include <QtCore/QObject>
 #include <QtTest/QtTest>
 #include "telepathytest.h"
+#include "chatmanager.h"
 #include "handlercontroller.h"
 #include "mockcontroller.h"
 #include "approver.h"
@@ -40,6 +41,7 @@
     void testCallProperties();
     void testConferenceCall();
     void testSendMessage();
+    void testAcknowledgeMessage();
     void testActiveCallIndicator();
     void testNotApprovedChannels();
 
@@ -283,6 +285,40 @@
     QCOMPARE(messageProperties["Recipients"].value<QStringList>().first(), recipient);
 }
 
+void HandlerTest::testAcknowledgeMessage()
+{
+    // if we register the observer before this test, other tests fail
+    TelepathyHelper::instance()->registerChannelObserver();
+    QString recipient("84376666");
+    QString recipient2("[phone]");
+    QString message("Hello, world!");
+    QSignalSpy messageSentSpy(mMockController, SIGNAL(MessageSent(QString,QVariantMap)));
+
+    // first send a message to a certain number so the handler request one channel
+    HandlerController::instance()->sendMessage(recipient, message, mTpAccount->uniqueIdentifier());
+    TRY_COMPARE(messageSentSpy.count(), 1);
+
+    QSignalSpy messageReceivedSpy(ChatManager::instance(), SIGNAL(messageReceived(QString,QString,QDateTime,QString,bool)));
+
+    // now receive a message from a very similar number so CM creates another
+    // channel and the handler needs to deal with both
+    QVariantMap properties;
+    properties["Sender"] = recipient2;
+    properties["Recipients"] = (QStringList() << recipient2);
+    mMockController->PlaceIncomingMessage(message, properties);
+
+    TRY_COMPARE(messageReceivedSpy.count(), 1);
+    QString receivedMessageId = messageReceivedSpy.first()[3].toString();
+
+    // then acknoledge the message that arrived in the second channel and make sure handler
+    // does the right thing
+    QSignalSpy messageReadSpy(mMockController, SIGNAL(MessageRead(QString)));
+    ChatManager::instance()->acknowledgeMessage(properties["Recipients"].toStringList(), receivedMessageId, mTpAccount->uniqueIdentifier());
+
+    TRY_COMPARE(messageReadSpy.count(), 1);
+    QCOMPARE(messageReadSpy.first()[0].toString(), receivedMessageId);
+}
+
 void HandlerTest::testActiveCallIndicator()
 {
     // start by making sure the property is false by default