Merge lp:~jamesh/online-accounts-api/all-services into lp:online-accounts-api

[James Henstridge]

Expose all serviceIds for each account rather than ignoring all but the first.

Expose all serviceIds for each account rather than ignoring all but the first.

[lp:~jamesh/online-accounts-api/all-services]

26. By James Henstridge on 2016-07-12

Update symbols file.

27. By James Henstridge on 2016-07-12

Update multiple services test to properly exercise the "any serviceId"
case.

[Alberto Mardegan (mardy)]

Unfortunately this won't work: the D-Bus interface is built around the assumption that an application can only use a single service for each account.
I'll comment on the linked bug.

review: Disapprove

[Alberto Mardegan (mardy)]

Let's try this.

review: Approve

=== modified file 'debian/libonline-accounts-qt1.symbols'
--- debian/libonline-accounts-qt1.symbols	2016-02-25 10:28:39 +0000
+++ debian/libonline-accounts-qt1.symbols	2016-07-12 10:24:12 +0000
@@ -98,6 +98,7 @@
  _ZN14OnlineAccounts7Manager17availableAccountsERK7QString@Base 0.1+16.04.20160212-0ubuntu1
  _ZN14OnlineAccounts7Manager5readyEv@Base 0.1+16.04.20160212-0ubuntu1
  _ZN14OnlineAccounts7Manager7accountEj@Base 0.1+16.04.20160212-0ubuntu1
+ _ZN14OnlineAccounts7Manager7accountEjRK7QString@Base 0replaceme
  _ZN14OnlineAccounts7ManagerC1ERK7QStringP7QObject@Base 0.1+16.04.20160212-0ubuntu1
  _ZN14OnlineAccounts7ManagerC2ERK7QStringP7QObject@Base 0.1+16.04.20160212-0ubuntu1
  _ZN14OnlineAccounts7ManagerD0Ev@Base 0.1+16.04.20160212-0ubuntu1

=== modified file 'src/lib/OnlineAccounts/manager.cpp'
--- src/lib/OnlineAccounts/manager.cpp	2015-08-28 14:25:23 +0000
+++ src/lib/OnlineAccounts/manager.cpp	2016-07-12 10:24:12 +0000
@@ -83,10 +83,11 @@
 Account *ManagerPrivate::ensureAccount(const AccountInfo &info)
 {
     if (Q_UNLIKELY(info.id() == 0)) return 0;
+    if (Q_UNLIKELY(info.service().isEmpty())) return 0;
 
-    QHash<AccountId,AccountData>::iterator i = m_accounts.find(info.id());
+    auto i = m_accounts.find({info.id(), info.service()});
     if (i == m_accounts.end()) {
-        i = m_accounts.insert(info.id(), AccountData(info));
+        i = m_accounts.insert({info.id(), info.service()}, AccountData(info));
     }
 
     AccountData &accountData = i.value();
@@ -129,7 +130,7 @@
     } else {
         QList<AccountInfo> accountInfos = reply.argumentAt<0>();
         Q_FOREACH(const AccountInfo &info, accountInfos) {
-            m_accounts.insert(info.id(), AccountData(info));
+            m_accounts.insert({info.id(), info.service()}, AccountData(info));
         }
     }
     m_getAccountsCall->deleteLater();
@@ -153,7 +154,7 @@
          * we remove it from our list so that we won't return it to the client
          * anymore.
          */
-        m_accounts.remove(info.id());
+        m_accounts.remove({info.id(), service});
     }
 
     /* No need to handle the Update change type: ensureAccount already updates
@@ -191,8 +192,7 @@
     Q_D(Manager);
 
     QList<Account*> result;
-    for (QHash<AccountId,AccountData>::iterator i = d->m_accounts.begin();
-         i != d->m_accounts.end(); i++) {
+    for (auto i = d->m_accounts.begin(); i != d->m_accounts.end(); i++) {
         AccountData &accountData = i.value();
         if (!service.isEmpty() && accountData.info.service() != service) continue;
 
@@ -209,10 +209,25 @@
 
 Account *Manager::account(AccountId accountId)
 {
+    return account(accountId, QString());
+}
+
+Account *Manager::account(AccountId accountId, const QString &serviceId)
+{
     Q_D(Manager);
 
-    QHash<AccountId,AccountData>::iterator i = d->m_accounts.find(accountId);
-    if (Q_UNLIKELY(i == d->m_accounts.end())) return 0;
+    decltype(d->m_accounts)::iterator i;
+    if (serviceId.isEmpty()) {
+        // Any non-empty service ID will sort after the empty string
+        i = d->m_accounts.lowerBound({accountId, QString()});
+        if (Q_UNLIKELY(i == d->m_accounts.end() ||
+                       i.key().first != accountId)) {
+            return 0;
+        }
+    } else {
+        i = d->m_accounts.find({accountId, serviceId});
+        if (Q_UNLIKELY(i == d->m_accounts.end())) return 0;
+    }
 
     AccountData &accountData = i.value();
     if (!accountData.account) {

=== modified file 'src/lib/OnlineAccounts/manager.h'
--- src/lib/OnlineAccounts/manager.h	2015-03-04 10:01:32 +0000
+++ src/lib/OnlineAccounts/manager.h	2016-07-12 10:24:12 +0000
@@ -48,6 +48,7 @@
 
     QList<Account*> availableAccounts(const QString &service = QString());
     Account *account(AccountId accountId);
+    Account *account(AccountId accountId, const QString &service);
 
     PendingCall requestAccess(const QString &service,
                               const AuthenticationData &authData);

=== modified file 'src/lib/OnlineAccounts/manager_p.h'
--- src/lib/OnlineAccounts/manager_p.h	2015-03-04 10:01:32 +0000
+++ src/lib/OnlineAccounts/manager_p.h	2016-07-12 10:24:12 +0000
@@ -23,8 +23,9 @@
 
 #include "manager.h"
 
-#include <QHash>
+#include <QMap>
 #include <QObject>
+#include <QPair>
 #include <QPointer>
 #include "account_info.h"
 #include "dbus_interface.h"
@@ -68,7 +69,7 @@
     QString m_applicationId;
     DBusInterface m_daemon;
     QDBusPendingCallWatcher *m_getAccountsCall;
-    QHash<AccountId,AccountData> m_accounts;
+    QMap<QPair<AccountId,QString>,AccountData> m_accounts;
     mutable Manager *q_ptr;
 };
 

=== modified file 'tests/lib/OnlineAccounts/functional_tests/functional_tests.cpp'
--- tests/lib/OnlineAccounts/functional_tests/functional_tests.cpp	2016-02-11 09:00:13 +0000
+++ tests/lib/OnlineAccounts/functional_tests/functional_tests.cpp	2016-07-12 10:24:12 +0000
@@ -87,6 +87,7 @@
     void testAccountData_data();
     void testAccountData();
     void testAccountChanges();
+    void testMultipleServices();
     void testPendingCallWatcher();
     void testAuthentication();
     void testAuthenticationErrors_data();
@@ -204,13 +205,13 @@
         "";
 
     QTest::newRow("access granted") <<
-        "ret = ((1, {'displayName': 'Bob'}),{'AccessToken':'GoOn'})" <<
+        "ret = ((1, {'displayName': 'Bob', 'serviceId': 'my-service'}),{'AccessToken':'GoOn'})" <<
         true <<
         int(0) <<
         "Bob" <<
         "GoOn";
     QTest::newRow("access granted, no wait") <<
-        "ret = ((1, {'displayName': 'Bob'}),{'AccessToken':'GoOn'})" <<
+        "ret = ((1, {'displayName': 'Bob', 'serviceId': 'my-service'}),{'AccessToken':'GoOn'})" <<
         false <<
         int(0) <<
         "Bob" <<
@@ -429,6 +430,58 @@
     QVERIFY(!account->isValid());
 }
 
+void FunctionalTests::testMultipleServices()
+{
+    addMockedMethod("GetAccounts", "a{sv}", "a(ua{sv})",
+                    "ret = ["
+                    "(1, {"
+                    "  'displayName': 'One',"
+                    "  'serviceId': 'service2',"
+                    "  'authMethod': 1,"
+                    "}),"
+                    "(2, {"
+                    "  'displayName': 'Two',"
+                    "  'serviceId': 'service1',"
+                    "  'authMethod': 1,"
+                    "}),"
+                    "(2, {"
+                    "  'displayName': 'Two',"
+                    "  'serviceId': 'service2',"
+                    "  'authMethod': 1,"
+                    "}),"
+                    "(4, {"
+                    "  'displayName': 'Three',"
+                    "  'serviceId': 'service1',"
+                    "  'authMethod': 1,"
+                    "}),"
+                    "]");
+    OnlineAccounts::Manager manager("my-app");
+    manager.waitForReady();
+
+    // All account services are available, including both from account #2
+    auto accounts = manager.availableAccounts();
+    QCOMPARE(accounts.count(), 4);
+
+    // Picks the first known service for the given account ID.
+    OnlineAccounts::Account *account = manager.account(2);
+    QVERIFY(account);
+    QVERIFY(account->isValid());
+    QCOMPARE(account->id(), OnlineAccounts::AccountId(2));
+    QCOMPARE(account->displayName(), QString("Two"));
+    QCOMPARE(account->serviceId(), QString("service1"));
+
+    account = manager.account(2, "service2");
+    QVERIFY(account);
+    QCOMPARE(account->id(), OnlineAccounts::AccountId(2));
+    QCOMPARE(account->serviceId(), QString("service2"));
+
+    account = manager.account(2, "service3");
+    QVERIFY(account == nullptr);
+
+    account = manager.account(3);
+    QVERIFY(account == nullptr);
+}
+
 void FunctionalTests::testAuthentication()
 {
     addMockedMethod("GetAccounts", "a{sv}", "a(ua{sv})",

=== modified file 'tests/lib/qml_module/tst_qml_module.cpp'
--- tests/lib/qml_module/tst_qml_module.cpp	2016-01-18 12:23:07 +0000
+++ tests/lib/qml_module/tst_qml_module.cpp	2016-07-12 10:24:12 +0000
@@ -383,7 +383,7 @@
     authenticationData.clear();
     authenticationData.insert("AccessToken", "GoOn");
     QTest::newRow("access granted") <<
-        "ret = ((1, {'displayName': 'Bob'}),{'AccessToken':'GoOn'})" <<
+        "ret = ((1, {'displayName': 'Bob', 'serviceId': 'bar'}),{'AccessToken':'GoOn'})" <<
         accessReply <<
         authenticationData;
 }