From c65a90e2a73814d09ad8c60adc4a259e90006db7 Mon Sep 17 00:00:00 2001 From: Tobias Markmann Date: Tue, 22 Nov 2016 08:51:50 +0100 Subject: Show MUC PM item-not-found error in MUC PM window Previously, if one would send a MUC PM to a user that has left a room the error response from the MUC would be shown in the MUC room. Now the error will show in the MUC PM window, if the MUC PM full JID has a ChatController. Test-Information: Added unit test verifying new behaviour. Manually verified that the error is shown in the MUC PM window instead of the MUC room window. Change-Id: I1b259d5eee9e22217bbe7e5c09294d2166a77895 diff --git a/Swift/Controllers/Chat/ChatControllerBase.cpp b/Swift/Controllers/Chat/ChatControllerBase.cpp index b339bd0..da9064e 100644 --- a/Swift/Controllers/Chat/ChatControllerBase.cpp +++ b/Swift/Controllers/Chat/ChatControllerBase.cpp @@ -270,7 +270,13 @@ void ChatControllerBase::handleIncomingMessage(std::shared_ptr mes std::string body = optionalBody.get_value_or(""); if (message->isError()) { if (!message->getTo().getResource().empty()) { - std::string errorMessage = str(format(QT_TRANSLATE_NOOP("", "Couldn't send message: %1%")) % getErrorMessage(message->getPayload())); + std::string errorMessage; + if (message->getPayload()->getCondition() == ErrorPayload::ItemNotFound) { + errorMessage = QT_TRANSLATE_NOOP("", "This user could not be found in the room."); + } + else { + errorMessage = str(format(QT_TRANSLATE_NOOP("", "Couldn't send message: %1%")) % getErrorMessage(message->getPayload())); + } chatWindow_->addErrorMessage(chatMessageParser_->parseMessageBody(errorMessage)); } } diff --git a/Swift/Controllers/Chat/ChatsManager.cpp b/Swift/Controllers/Chat/ChatsManager.cpp index b9e2cf4..4f95d71 100644 --- a/Swift/Controllers/Chat/ChatsManager.cpp +++ b/Swift/Controllers/Chat/ChatsManager.cpp @@ -937,8 +937,18 @@ void ChatsManager::handleIncomingMessage(std::shared_ptr incomingMessag return; } - // Try to deliver it to a MUC - if (message->getType() == Message::Groupchat || message->getType() == Message::Error /*|| (isInvite && message->getType() == Message::Normal)*/) { + // Try to deliver MUC errors to a MUC PM window if a suitable window is open. + if (message->getType() == Message::Error) { + auto controller = getChatControllerIfExists(fromJID, messageCausesSessionBinding(message)); + if (controller) { + controller->handleIncomingMessage(event); + return; + } + } + + // Try to deliver it to a MUC. + if (message->getType() == Message::Groupchat || message->getType() == Message::Error) { + // Try to deliver it to a MUC room. std::map::iterator i = mucControllers_.find(fromJID.toBare()); if (i != mucControllers_.end()) { i->second->handleIncomingMessage(event); diff --git a/Swift/Controllers/Chat/UnitTest/ChatsManagerTest.cpp b/Swift/Controllers/Chat/UnitTest/ChatsManagerTest.cpp index a5e68cf..cff54f8 100644 --- a/Swift/Controllers/Chat/UnitTest/ChatsManagerTest.cpp +++ b/Swift/Controllers/Chat/UnitTest/ChatsManagerTest.cpp @@ -81,10 +81,13 @@ class ChatsManagerTest : public CppUnit::TestFixture { CPPUNIT_TEST(testChatControllerPresenceAccessUpdatedOnSubscriptionChangeToFrom); CPPUNIT_TEST(testChatControllerFullJIDBindingOnMessageAndNotReceipt); CPPUNIT_TEST(testChatControllerFullJIDBindingOnTypingAndNotActive); - CPPUNIT_TEST(testChatControllerPMPresenceHandling); CPPUNIT_TEST(testLocalMUCServiceDiscoveryResetOnDisconnect); CPPUNIT_TEST(testPresenceChangeDoesNotReplaceMUCInvite); + // MUC PM Tests + CPPUNIT_TEST(testChatControllerPMPresenceHandling); + CPPUNIT_TEST(testChatControllerMucPmUnavailableErrorHandling); + // Highlighting tests CPPUNIT_TEST(testChatControllerHighlightingNotificationTesting); CPPUNIT_TEST(testChatControllerHighlightingNotificationDeduplicateSounds); @@ -190,7 +193,7 @@ public: void testSecondOpenWindowIncoming() { JID messageJID1("testling@test.com/resource1"); - MockChatWindow* window1 = new MockChatWindow();//mocks_->InterfaceMock(); + MockChatWindow* window1 = new MockChatWindow(); mocks_->ExpectCall(chatWindowFactory_, ChatWindowFactory::createChatWindow).With(messageJID1, uiEventStream_).Return(window1); std::shared_ptr message1(new Message()); @@ -213,7 +216,7 @@ public: void testFirstOpenWindowOutgoing() { std::string messageJIDString("testling@test.com"); - ChatWindow* window = new MockChatWindow();//mocks_->InterfaceMock(); + ChatWindow* window = new MockChatWindow(); mocks_->ExpectCall(chatWindowFactory_, ChatWindowFactory::createChatWindow).With(JID(messageJIDString), uiEventStream_).Return(window); uiEventStream_->send(std::make_shared(JID(messageJIDString))); @@ -224,7 +227,7 @@ public: std::string bareJIDString("testling@test.com"); std::string fullJIDString("testling@test.com/resource1"); - MockChatWindow* window = new MockChatWindow();//mocks_->InterfaceMock(); + MockChatWindow* window = new MockChatWindow(); mocks_->ExpectCall(chatWindowFactory_, ChatWindowFactory::createChatWindow).With(JID(bareJIDString), uiEventStream_).Return(window); uiEventStream_->send(std::make_shared(JID(bareJIDString))); @@ -238,12 +241,12 @@ public: void testSecondWindow() { std::string messageJIDString1("testling1@test.com"); - ChatWindow* window1 = new MockChatWindow();//mocks_->InterfaceMock(); + ChatWindow* window1 = new MockChatWindow(); mocks_->ExpectCall(chatWindowFactory_, ChatWindowFactory::createChatWindow).With(JID(messageJIDString1), uiEventStream_).Return(window1); uiEventStream_->send(std::make_shared(JID(messageJIDString1))); std::string messageJIDString2("testling2@test.com"); - ChatWindow* window2 = new MockChatWindow();//mocks_->InterfaceMock(); + ChatWindow* window2 = new MockChatWindow(); mocks_->ExpectCall(chatWindowFactory_, ChatWindowFactory::createChatWindow).With(JID(messageJIDString2), uiEventStream_).Return(window2); uiEventStream_->send(std::make_shared(JID(messageJIDString2))); @@ -260,7 +263,7 @@ public: std::string fullJIDString1("testling@test.com/resource1"); std::string fullJIDString2("testling@test.com/resource2"); - MockChatWindow* window = new MockChatWindow();//mocks_->InterfaceMock(); + MockChatWindow* window = new MockChatWindow(); mocks_->ExpectCall(chatWindowFactory_, ChatWindowFactory::createChatWindow).With(JID(bareJIDString), uiEventStream_).Return(window); uiEventStream_->send(std::make_shared(JID(bareJIDString))); @@ -297,18 +300,18 @@ public: std::string messageJIDString1("testling@test.com/1"); - ChatWindow* window1 = new MockChatWindow();//mocks_->InterfaceMock(); + ChatWindow* window1 = new MockChatWindow(); mocks_->ExpectCall(chatWindowFactory_, ChatWindowFactory::createChatWindow).With(JID(messageJIDString1), uiEventStream_).Return(window1); uiEventStream_->send(std::make_shared(JID(messageJIDString1))); std::string messageJIDString2("testling@test.com/2"); - ChatWindow* window2 = new MockChatWindow();//mocks_->InterfaceMock(); + ChatWindow* window2 = new MockChatWindow(); mocks_->ExpectCall(chatWindowFactory_, ChatWindowFactory::createChatWindow).With(JID(messageJIDString2), uiEventStream_).Return(window2); uiEventStream_->send(std::make_shared(JID(messageJIDString2))); std::string messageJIDString3("testling@test.com/3"); - ChatWindow* window3 = new MockChatWindow();//mocks_->InterfaceMock(); + ChatWindow* window3 = new MockChatWindow(); mocks_->ExpectCall(chatWindowFactory_, ChatWindowFactory::createChatWindow).With(JID(messageJIDString3), uiEventStream_).Return(window3); uiEventStream_->send(std::make_shared(JID(messageJIDString3))); @@ -331,7 +334,7 @@ public: void testNoDuplicateUnbind() { JID messageJID1("testling@test.com/resource1"); - MockChatWindow* window1 = new MockChatWindow();//mocks_->InterfaceMock(); + MockChatWindow* window1 = new MockChatWindow(); mocks_->ExpectCall(chatWindowFactory_, ChatWindowFactory::createChatWindow).With(messageJID1, uiEventStream_).Return(window1); std::shared_ptr message1(new Message()); @@ -407,7 +410,7 @@ public: void testChatControllerPresenceAccessUpdatedOnAddToRoster() { JID messageJID("testling@test.com/resource1"); - MockChatWindow* window = new MockChatWindow();//mocks_->InterfaceMock(); + MockChatWindow* window = new MockChatWindow(); mocks_->ExpectCall(chatWindowFactory_, ChatWindowFactory::createChatWindow).With(messageJID, uiEventStream_).Return(window); settings_->storeSetting(SettingConstants::REQUEST_DELIVERYRECEIPTS, true); @@ -688,6 +691,50 @@ public: CPPUNIT_ASSERT_EQUAL(std::string("participantA has gone offline."), MockChatWindow::bodyFromMessage(window->lastReplacedMessage_)); } + void testChatControllerMucPmUnavailableErrorHandling() { + auto mucJID = JID("test@rooms.test.com"); + auto participantA = mucJID.withResource("participantA"); + auto participantB = mucJID.withResource("participantB"); + + auto mucWindow = new MockChatWindow(); + mocks_->ExpectCall(chatWindowFactory_, ChatWindowFactory::createChatWindow).With(mucJID, uiEventStream_).Return(mucWindow); + uiEventStream_->send(std::make_shared(mucJID, participantB.getResource())); + CPPUNIT_ASSERT_EQUAL(true, mucWindow->mucType_.is_initialized()); + + auto window = new MockChatWindow(); + mocks_->ExpectCall(chatWindowFactory_, ChatWindowFactory::createChatWindow).With(participantA, uiEventStream_).Return(window); + uiEventStream_->send(std::make_shared(participantA)); + CPPUNIT_ASSERT_EQUAL(false, window->mucType_.is_initialized()); + + Presence::ref presence = Presence::create(); + presence->setFrom(participantA); + presence->setShow(StatusShow::Online); + stanzaChannel_->onPresenceReceived(presence); + CPPUNIT_ASSERT_EQUAL(std::string("participantA has become available."), MockChatWindow::bodyFromMessage(window->lastAddedPresence_)); + + // send message to participantA + auto messageBody = std::string("message body to send"); + window->onSendMessageRequest(messageBody, false); + auto sendMessageStanza = stanzaChannel_->getStanzaAtIndex(2); + CPPUNIT_ASSERT_EQUAL(messageBody, *sendMessageStanza->getBody()); + + // receive reply with error + auto messageErrorReply = std::make_shared(); + messageErrorReply->setID(stanzaChannel_->getNewIQID()); + messageErrorReply->setType(Message::Error); + messageErrorReply->setFrom(participantA); + messageErrorReply->setTo(jid_); + messageErrorReply->addPayload(std::make_shared(ErrorPayload::ItemNotFound, ErrorPayload::Cancel, "Recipient not in room")); + + auto lastMUCWindowErrorMessageBeforeError = MockChatWindow::bodyFromMessage(mucWindow->lastAddedErrorMessage_); + manager_->handleIncomingMessage(messageErrorReply); + + // assert that error is not routed to MUC window + CPPUNIT_ASSERT_EQUAL(lastMUCWindowErrorMessageBeforeError, MockChatWindow::bodyFromMessage(mucWindow->lastAddedErrorMessage_)); + // assert that error is routed to PM + CPPUNIT_ASSERT_EQUAL(std::string("This user could not be found in the room."), MockChatWindow::bodyFromMessage(window->lastAddedErrorMessage_)); + } + void testLocalMUCServiceDiscoveryResetOnDisconnect() { JID ownJID("test@test.com/resource"); JID sender("foo@test.com"); diff --git a/Swift/Controllers/UnitTest/MockChatWindow.h b/Swift/Controllers/UnitTest/MockChatWindow.h index 9b943f0..76e5194 100644 --- a/Swift/Controllers/UnitTest/MockChatWindow.h +++ b/Swift/Controllers/UnitTest/MockChatWindow.h @@ -69,7 +69,11 @@ namespace Swift { virtual void setAvailableSecurityLabels(const std::vector& labels) {labels_ = labels;} virtual void setSecurityLabelsEnabled(bool enabled) {labelsEnabled_ = enabled;} virtual void setUnreadMessageCount(int /*count*/) {} - virtual void convertToMUC(MUCType /*mucType*/) {} + + virtual void convertToMUC(MUCType mucType) { + mucType_ = mucType; + } + virtual void setSecurityLabelsError() {} virtual SecurityLabelsCatalog::Item getSelectedSecurityLabel() {return label_;} virtual void setOnline(bool /*online*/) {} @@ -144,6 +148,7 @@ namespace Swift { SecurityLabelsCatalog::Item label_; Roster* roster_ = nullptr; std::vector> receiptChanges_; + boost::optional mucType_; }; } diff --git a/Swiften/Elements/Message.h b/Swiften/Elements/Message.h index f276ef7..c357cd4 100644 --- a/Swiften/Elements/Message.h +++ b/Swiften/Elements/Message.h @@ -39,10 +39,8 @@ namespace Swift { updatePayload(std::make_shared(subject)); } - // Explicitly convert to bool. In C++11, it would be cleaner to - // compare to nullptr. bool hasSubject() { - return static_cast(getPayload()); + return getPayload() != nullptr; } boost::optional getBody() const { -- cgit v0.10.2-6-g49f6