Fix error response handling when requesting VCardsswift-4.0.1

When fetching a vCard, an empty vCard response and an item-not-found error are semantically the same. Changed the code to treat and item-not-found error as an empty vCard in this case. This enables setting your own vCard on servers that do not return an empty vCard for fresh accounts and generally improves UX when fetching others vCards. Test-Information: Added unit tests verifying new behaviour. Tested with Swift against a Prosody IM instance. Without this change you cannot set the vCard on a fresh user. With this patch you can set your own vCard. Change-Id: I5f9adb4c3d6b6a1a320b834be918ab5ab0b52975
author: Tobias Markmann <tm@ayena.de> 2018-03-27 16:32:09 (GMT)
committer: Tobias Markmann <tm@ayena.de> 2018-03-28 16:06:10 (GMT)
commit: 44d588baf1f9324ced16e003d0209bd1e3c546ab (patch)
tree: 6627190d9498f69dc505d678a31ae040003caa96
parent: 5ba3f18ad8efa040d49f36d83ec2e7891a9add9f (diff)
download: swift-swift-4.0.1.zip
swift-swift-4.0.1.tar.bz2
4 files changed, 125 insertions, 17 deletions
diff --git a/Swift/ChangeLog.md b/Swift/ChangeLog.md
index a00f41c..3516022 100644
--- a/Swift/ChangeLog.md
+++ b/Swift/ChangeLog.md
@@ -1,3 +1,7 @@
+4.0.1 (2018-03-28)
+------------------
+- Allow setting vCard on servers that do not return an empty vCard on fresh accounts
+
 4.0 (2018-03-20)
 ----------------
 - New chat theme including a new font
diff --git a/Swiften/ChangeLog.md b/Swiften/ChangeLog.md
index a664341..23d5185 100644
--- a/Swiften/ChangeLog.md
+++ b/Swiften/ChangeLog.md
@@ -1,4 +1,8 @@
-4.0 (2017-03-20)
+4.0.1 (2018-03-28)
+------------------
+- Fix handling errors when fetching own vCard
+
+4.0 (2018-03-20)
 ----------------
 - Moved code-base to C++11
     - Use C++11 threading instead of Boost.Thread library
diff --git a/Swiften/VCards/UnitTest/VCardManagerTest.cpp b/Swiften/VCards/UnitTest/VCardManagerTest.cpp
index 3d5338d..669c3ff 100644
--- a/Swiften/VCards/UnitTest/VCardManagerTest.cpp
+++ b/Swiften/VCards/UnitTest/VCardManagerTest.cpp
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2010-2016 Isode Limited.
+ * Copyright (c) 2010-2018 Isode Limited.
  * All rights reserved.
  * See the COPYING file for more information.
  */
@@ -31,7 +31,17 @@ class VCardManagerTest : public CppUnit::TestFixture {
         CPPUNIT_TEST(testRequest_Error);
         CPPUNIT_TEST(testRequest_VCardAlreadyRequested);
         CPPUNIT_TEST(testRequest_AfterPreviousRequest);
-        CPPUNIT_TEST(testRequestOwnVCard);
+
+        CPPUNIT_TEST(testRequestVCard_ReturnFullVCard);
+        CPPUNIT_TEST(testRequestVCard_ReturnEmptyVCard);
+        CPPUNIT_TEST(testRequestVCard_ReturnItemNotFoundError);
+        CPPUNIT_TEST(testRequestVCard_ReturnFeatureNotImplementedError);
+
+        CPPUNIT_TEST(testRequestOwnVCard_ReturnFullVCard);
+        CPPUNIT_TEST(testRequestOwnVCard_ReturnEmptyVCard);
+        CPPUNIT_TEST(testRequestOwnVCard_ReturnItemNotFoundError);
+        CPPUNIT_TEST(testRequestOwnVCard_ReturnFeatureNotImplementedError);
+
         CPPUNIT_TEST(testCreateSetVCardRequest);
         CPPUNIT_TEST(testCreateSetVCardRequest_Error);
         CPPUNIT_TEST_SUITE_END();
@@ -54,7 +64,7 @@ class VCardManagerTest : public CppUnit::TestFixture {
         }
 
         void testGet_NewVCardRequestsVCard() {
-            std::shared_ptr<VCardManager> testling = createManager();
+            auto testling = createManager();
             VCard::ref result = testling->getVCardAndRequestWhenNeeded(JID("foo@bar.com/baz"));
 
             CPPUNIT_ASSERT(!result);
@@ -63,7 +73,7 @@ class VCardManagerTest : public CppUnit::TestFixture {
         }
 
         void testGet_ExistingVCard() {
-            std::shared_ptr<VCardManager> testling = createManager();
+            auto testling = createManager();
             VCard::ref vcard(new VCard());
             vcard->setFullName("Foo Bar");
             vcardStorage->setVCard(JID("foo@bar.com/baz"), vcard);
@@ -75,7 +85,7 @@ class VCardManagerTest : public CppUnit::TestFixture {
         }
 
         void testRequest_RequestsVCard() {
-            std::shared_ptr<VCardManager> testling = createManager();
+            auto testling = createManager();
             testling->requestVCard(JID("foo@bar.com/baz"));
 
             CPPUNIT_ASSERT_EQUAL(1, static_cast<int>(stanzaChannel->sentStanzas.size()));
@@ -83,7 +93,7 @@ class VCardManagerTest : public CppUnit::TestFixture {
         }
 
         void testRequest_ReceiveEmitsNotification() {
-            std::shared_ptr<VCardManager> testling = createManager();
+            auto testling = createManager();
             testling->requestVCard(JID("foo@bar.com/baz"));
             stanzaChannel->onIQReceived(createVCardResult());
 
@@ -96,7 +106,7 @@ class VCardManagerTest : public CppUnit::TestFixture {
         }
 
         void testRequest_Error() {
-            std::shared_ptr<VCardManager> testling = createManager();
+            auto testling = createManager();
             testling->requestVCard(JID("foo@bar.com/baz"));
             stanzaChannel->onIQReceived(IQ::createError(JID("baz@fum.com/foo"), stanzaChannel->sentStanzas[0]->getTo(), stanzaChannel->sentStanzas[0]->getID()));
 
@@ -105,7 +115,7 @@ class VCardManagerTest : public CppUnit::TestFixture {
         }
 
         void testRequest_VCardAlreadyRequested() {
-            std::shared_ptr<VCardManager> testling = createManager();
+            auto testling = createManager();
             testling->requestVCard(JID("foo@bar.com/baz"));
             VCard::ref result = testling->getVCardAndRequestWhenNeeded(JID("foo@bar.com/baz"));
 
@@ -114,7 +124,7 @@ class VCardManagerTest : public CppUnit::TestFixture {
         }
 
         void testRequest_AfterPreviousRequest() {
-            std::shared_ptr<VCardManager> testling = createManager();
+            auto testling = createManager();
             testling->requestVCard(JID("foo@bar.com/baz"));
             stanzaChannel->onIQReceived(createVCardResult());
             testling->requestVCard(JID("foo@bar.com/baz"));
@@ -123,8 +133,60 @@ class VCardManagerTest : public CppUnit::TestFixture {
             CPPUNIT_ASSERT(stanzaChannel->isRequestAtIndex<VCard>(1, JID("foo@bar.com/baz"), IQ::Get));
         }
 
-        void testRequestOwnVCard() {
-            std::shared_ptr<VCardManager> testling = createManager();
+        void testRequestVCard_ReturnFullVCard() {
+            auto testling = createManager();
+            testling->requestVCard(JID("foo@bar.com/baz"));
+            stanzaChannel->onIQReceived(createVCardResult());
+
+            CPPUNIT_ASSERT_EQUAL(1, static_cast<int>(stanzaChannel->sentStanzas.size()));
+            CPPUNIT_ASSERT(stanzaChannel->isRequestAtIndex<VCard>(0, JID("foo@bar.com/baz"), IQ::Get));
+            CPPUNIT_ASSERT_EQUAL(1, static_cast<int>(changes.size()));
+            CPPUNIT_ASSERT_EQUAL(JID("foo@bar.com/baz"), changes[0].first);
+            CPPUNIT_ASSERT_EQUAL(std::string("Foo Bar"), changes[0].second->getFullName());
+            CPPUNIT_ASSERT_EQUAL(false, changes[0].second->isEmpty());
+        }
+
+        void testRequestVCard_ReturnEmptyVCard() {
+            auto testling = createManager();
+            testling->requestVCard(JID("foo@bar.com/baz"));
+                  stanzaChannel->onIQReceived([&](){
+                    auto vcard = std::make_shared<VCard>();
+                    return IQ::createResult(JID("foo@bar.com/baz"), stanzaChannel->sentStanzas[0]->getTo(), stanzaChannel->sentStanzas[0]->getID(), vcard);
+            }());
+
+            CPPUNIT_ASSERT_EQUAL(1, static_cast<int>(stanzaChannel->sentStanzas.size()));
+            CPPUNIT_ASSERT(stanzaChannel->isRequestAtIndex<VCard>(0, JID("foo@bar.com/baz"), IQ::Get));
+            CPPUNIT_ASSERT_EQUAL(1, static_cast<int>(changes.size()));
+            CPPUNIT_ASSERT_EQUAL(true, changes[0].second->isEmpty());
+        }
+
+        void testRequestVCard_ReturnItemNotFoundError() {
+            auto testling = createManager();
+            testling->requestVCard(JID("foo@bar.com/baz"));
+                  stanzaChannel->onIQReceived([&](){
+                    return IQ::createError(JID("foo@bar.com/baz"), stanzaChannel->sentStanzas[0]->getTo(), stanzaChannel->sentStanzas[0]->getID(), ErrorPayload::ItemNotFound);
+            }());
+
+            CPPUNIT_ASSERT_EQUAL(1, static_cast<int>(stanzaChannel->sentStanzas.size()));
+            CPPUNIT_ASSERT(stanzaChannel->isRequestAtIndex<VCard>(0, JID("foo@bar.com/baz"), IQ::Get));
+            CPPUNIT_ASSERT_EQUAL(1, static_cast<int>(changes.size()));
+            CPPUNIT_ASSERT_EQUAL(true, changes[0].second->isEmpty());
+        }
+
+        void testRequestVCard_ReturnFeatureNotImplementedError() {
+            auto testling = createManager();
+            testling->requestVCard(JID("foo@bar.com/baz"));
+                  stanzaChannel->onIQReceived([&](){
+                    return IQ::createError(JID("foo@bar.com/baz"), stanzaChannel->sentStanzas[0]->getTo(), stanzaChannel->sentStanzas[0]->getID(), ErrorPayload::FeatureNotImplemented);
+            }());
+
+            CPPUNIT_ASSERT_EQUAL(1, static_cast<int>(stanzaChannel->sentStanzas.size()));
+            CPPUNIT_ASSERT(stanzaChannel->isRequestAtIndex<VCard>(0, JID("foo@bar.com/baz"), IQ::Get));
+            CPPUNIT_ASSERT_EQUAL(0, static_cast<int>(changes.size()));
+        }
+
+        void testRequestOwnVCard_ReturnFullVCard() {
+            auto testling = createManager();
             testling->requestVCard(ownJID);
             stanzaChannel->onIQReceived(createOwnVCardResult());
 
@@ -139,8 +201,47 @@ class VCardManagerTest : public CppUnit::TestFixture {
             CPPUNIT_ASSERT_EQUAL(std::string("Myself"), ownChanges[0]->getFullName());
         }
 
+        void testRequestOwnVCard_ReturnEmptyVCard() {
+            auto testling = createManager();
+            testling->requestVCard(ownJID);
+            stanzaChannel->onIQReceived([&](){
+                    auto vcard = std::make_shared<VCard>();
+                    return IQ::createResult(JID(), stanzaChannel->sentStanzas[0]->getTo(), stanzaChannel->sentStanzas[0]->getID(), vcard);
+            }());
+
+            CPPUNIT_ASSERT_EQUAL(1, static_cast<int>(stanzaChannel->sentStanzas.size()));
+            CPPUNIT_ASSERT(stanzaChannel->isRequestAtIndex<VCard>(0, JID(), IQ::Get));
+            CPPUNIT_ASSERT_EQUAL(1, static_cast<int>(changes.size()));
+            CPPUNIT_ASSERT_EQUAL(true, changes[0].second->isEmpty());
+        }
+
+        void testRequestOwnVCard_ReturnItemNotFoundError() {
+            auto testling = createManager();
+            testling->requestVCard(ownJID);
+            stanzaChannel->onIQReceived([&](){
+                    return IQ::createError(JID(), stanzaChannel->sentStanzas[0]->getTo(), stanzaChannel->sentStanzas[0]->getID(), ErrorPayload::ItemNotFound);
+            }());
+
+            CPPUNIT_ASSERT_EQUAL(1, static_cast<int>(stanzaChannel->sentStanzas.size()));
+            CPPUNIT_ASSERT(stanzaChannel->isRequestAtIndex<VCard>(0, JID(), IQ::Get));
+            CPPUNIT_ASSERT_EQUAL(1, static_cast<int>(changes.size()));
+            CPPUNIT_ASSERT_EQUAL(true, changes[0].second->isEmpty());
+        }
+
+        void testRequestOwnVCard_ReturnFeatureNotImplementedError() {
+            auto testling = createManager();
+            testling->requestVCard(ownJID);
+            stanzaChannel->onIQReceived([&](){
+                    return IQ::createError(JID(), stanzaChannel->sentStanzas[0]->getTo(), stanzaChannel->sentStanzas[0]->getID(), ErrorPayload::FeatureNotImplemented);
+            }());
+
+            CPPUNIT_ASSERT_EQUAL(1, static_cast<int>(stanzaChannel->sentStanzas.size()));
+            CPPUNIT_ASSERT(stanzaChannel->isRequestAtIndex<VCard>(0, JID(), IQ::Get));
+            CPPUNIT_ASSERT_EQUAL(0, static_cast<int>(changes.size()));
+        }
+
         void testCreateSetVCardRequest() {
-            std::shared_ptr<VCardManager> testling = createManager();
+            auto testling = createManager();
             VCard::ref vcard = std::make_shared<VCard>();
             vcard->setFullName("New Name");
             SetVCardRequest::ref request = testling->createSetVCardRequest(vcard);
@@ -154,7 +255,7 @@ class VCardManagerTest : public CppUnit::TestFixture {
         }
 
         void testCreateSetVCardRequest_Error() {
-            std::shared_ptr<VCardManager> testling = createManager();
+            auto testling = createManager();
             VCard::ref vcard = std::make_shared<VCard>();
             vcard->setFullName("New Name");
             SetVCardRequest::ref request = testling->createSetVCardRequest(vcard);
diff --git a/Swiften/VCards/VCardManager.cpp b/Swiften/VCards/VCardManager.cpp
index 95b96fa..9423702 100644
--- a/Swiften/VCards/VCardManager.cpp
+++ b/Swiften/VCards/VCardManager.cpp
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2010-2016 Isode Limited.
+ * Copyright (c) 2010-2018 Isode Limited.
  * All rights reserved.
  * See the COPYING file for more information.
  */
@@ -50,10 +50,9 @@ void VCardManager::requestOwnVCard() {
     requestVCard(JID());
 }
 
-
 void VCardManager::handleVCardReceived(const JID& actualJID, VCard::ref vcard, ErrorPayload::ref error) {
     requestedVCards.erase(actualJID);
-    if (!error) {
+    if (!error || (error && error->getCondition() == ErrorPayload::ItemNotFound)) {
         if (!vcard) {
             vcard = VCard::ref(new VCard());
         }
author	Tobias Markmann <tm@ayena.de>	2018-03-27 16:32:09 (GMT)
committer	Tobias Markmann <tm@ayena.de>	2018-03-28 16:06:10 (GMT)
commit	44d588baf1f9324ced16e003d0209bd1e3c546ab (patch)
tree	6627190d9498f69dc505d678a31ae040003caa96
parent	5ba3f18ad8efa040d49f36d83ec2e7891a9add9f (diff)
download	swift-swift-4.0.1.zip swift-swift-4.0.1.tar.bz2