Merge lp:~mardy/ubuntuone-credentials/use-provided-account-1563007 into lp:ubuntuone-credentials

[Alberto Mardegan]

Use account object provided by online-accounts-ui

Without this change, the creation of UbuntuOne accounts gets reported as a failure, because the value of "account.accountId" never gets changed, since the UbuntuOne::Keyring is creating a different account.
This change also fixes the account creation in System Settings: before, once a new account was created the view switched back to the list of account providers; now, it properly switches back to the list of accounts.

Use account object provided by online-accounts-ui

Without this change, the creation of UbuntuOne accounts gets reported as a failure, because the value of "account.accountId" never gets changed, since the UbuntuOne::Keyring is creating a different account.
This change also fixes the account creation in System Settings: before, once a new account was created the view switched back to the list of account providers; now, it properly switches back to the list of accounts.

[dobey (dobey)]

I do not understand what this is meant to fix exactly, or why adding API is a reasonable solution for it. We have been using the account object provided by OA for a long time now, so this can't possibly simply be doing that.

AFAICT, this is a rushed workaround for a problem that is not well understood.

review: Needs Information

[lp:~mardy/ubuntuone-credentials/use-provided-account-1563007]

234. By Alberto Mardegan on 2016-03-30

Update bug number

[Alberto Mardegan (mardy)]

While I initially claimed that this change would fix bug 1563007 (which it doesn't), the MP description is correct.
I've now filed a different bug to better explain what this change fixes, and updated the changelog accordingly.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:234
http://jenkins.qa.ubuntu.com/job/ubuntuone-credentials-ci/142/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntuone-credentials-wily-amd64-ci/19/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntuone-credentials-wily-armhf-ci/19
        deb: http://jenkins.qa.ubuntu.com/job/ubuntuone-credentials-wily-armhf-ci/19/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntuone-credentials-wily-i386-ci/19

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntuone-credentials-ci/142/rebuild

review: Needs Fixing (continuous-integration)

[dobey (dobey)]

As per discussion on the bug, this seems like perhaps a bug in ussoa instead, when there are existing accounts, as the problem doesn't occur when U1 is the first account being added, only when there are other accounts already existing.

Let's try to figure out what actually changed or is going on, before moving any further here, as I'd rather avoid adding some obtuse API to work around an issue that exists elsewhere.

review: Needs Information

[Alberto Mardegan (mardy)]

On 20/04/2016 20:45, Rodney Dawes wrote:
> Review: Needs Information
>
> Let's try to figure out what actually changed or is going on, before moving any further here, as I'd rather avoid adding some obtuse API to work around an issue that exists elsewhere.

I've investigated this, and I'm not sure we have a bug, after all.
When the Online Accounts page is opened from the System Settings, we
bind to the accountsModel.count property to decide whether to show the
NoAccountsPage (the one with the explanatory text and the list of
accounts providers) or the AccountsPage (which lists your existing
accounts).

It's a property binding, meaning that if an account is created (even by
an external app) while you are in the NoAccountsPage, the view will
switch to the AccountsPage; and conversely, if you are in the
AccountsPage and all accounts get deleted (even by an external app), the
view will switch to the NoAccountsPage. I think that this is the correct
behaviour.

So, back to our issue: when you create the U1 account as your first
account, the account creation won't be considered successful by ussoa
(due to this bug about another account object being modified), but you
are still writing the account to the database, so the account model is
getting its "count" property increased to 1 and the view will switch.

So, this is not really an issue, and we should not worry about it.

Now, I understand that you don't like changing the public API, so here's
my suggestions:

1) HACK: we don't add a public API, and the QML plugin will set the
account as a dynamic QObject property on the SsoService:

    ssoService->setProperty("_u1_account", account);

and the keyring will fetch it from the ssoService with

    account = ssoService->property("_u1_account");

It's a hack, but it's not that bad since it's contained in a single
project and it can go away when we switch to #2 below.

2) Once we have the U1 signon authentication plugin, we can modify the
account plugin not to talk to libubuntuoneauth at all, and instead
create the account directly using the OA QML APIs, like the OAuth plugin
is doing.

[dobey (dobey)]

I wonder what broke this then. We previously did a fair bit of work to use the existing account provided by OA, to fix a previous issue similar to this. It was working fine for quite some time, so I wonder what broke, because we haven't changed anything in u1-credentials that would be related to this, for quite a while now.

[unity-api-1-bot (unity-api-1-bot)]

FAILED: Continuous integration, rev:234
https://jenkins.canonical.com/unity-api-1/job/lp-ubuntuone-credentials-ci/4/
Executed test runs:
    FAILURE: https://jenkins.canonical.com/unity-api-1/job/build/87/console
    FAILURE: https://jenkins.canonical.com/unity-api-1/job/build-0-fetch/94/console

Click here to trigger a rebuild:
https://jenkins.canonical.com/unity-api-1/job/lp-ubuntuone-credentials-ci/4/rebuild

review: Needs Fixing (continuous-integration)

[Natalia Bidart (nataliabidart)]

Started a massive cleanup of old MPs, closing this given its age, please update and re-open if still valid.

Unmerged revisions

234. By Alberto Mardegan on 2016-03-30

Update bug number

233. By Alberto Mardegan on 2016-03-30

Use account object provided by OA

=== modified file 'debian/changelog'
--- debian/changelog	2016-03-23 19:31:42 +0000
+++ debian/changelog	2016-03-30 13:47:05 +0000
@@ -1,3 +1,10 @@
+ubuntuone-credentials (15.11+16.04.20160330) vivid; urgency=medium
+
+  * debian/libubuntuoneauth-2.0-0.symbols: update to released version.
+  * Use account object provided by OA (LP: #1563867)
+
+ -- Alberto Mardegan <[e-mail]>  Wed, 30 Mar 2016 12:33:36 +0300
+
 ubuntuone-credentials (15.11+16.04.20160323) xenial; urgency=medium
 
   [ CI Train Bot ]

=== modified file 'debian/libubuntuoneauth-2.0-0.symbols'
--- debian/libubuntuoneauth-2.0-0.symbols	2015-12-07 21:38:01 +0000
+++ debian/libubuntuoneauth-2.0-0.symbols	2016-03-30 13:47:05 +0000
@@ -15,6 +15,8 @@
  (c++)"UbuntuOne::AuthLogger::~AuthLogger()@Base" 13.08
  (c++)"UbuntuOne::SSOService::qt_metacall(QMetaObject::Call, int, void**)@Base" 13.08
  (c++)"UbuntuOne::SSOService::qt_metacast(char const*)@Base" 13.08
+ (c++)"UbuntuOne::SSOService::setAccount(QObject*)@Base" 15.11+16.04.20160330
+ (c++)"UbuntuOne::SSOService::account() const@Base" 15.11+16.04.20160330
  (c++)"UbuntuOne::SSOService::registerUser(QString, QString, QString)@Base" 13.08
  (c++)"UbuntuOne::SSOService::accountPinged(QNetworkReply*)@Base" 13.08
  (c++)"UbuntuOne::SSOService::errorOccurred(UbuntuOne::ErrorResponse const&)@Base" 13.08
@@ -116,6 +118,8 @@
  (c++)"UbuntuOne::Token::updated() const@Base" 14.04+14.10.20140818
  (c++)"UbuntuOne::Token::getServerTimestamp() const@Base" 15.11+16.04.20151207.1
  (c++)"UbuntuOne::Token::addOAuthTimestamp(QString) const@Base" 15.11+16.04.20151207.1
+ (c++)"UbuntuOne::Keyring::setAccount(QObject*)@Base" 15.11+16.04.20160330
+ (c++)"UbuntuOne::Keyring::account() const@Base" 15.11+16.04.20160330
  (c++)"UbuntuOne::Keyring::storeToken(UbuntuOne::Token)@Base" 13.08
  (c++)"UbuntuOne::Keyring::storeToken(UbuntuOne::Token, QString const&)@Base" 14.04+14.10.20140802
  (c++)"UbuntuOne::Keyring::tokenFound(UbuntuOne::Token const&)@Base" 13.08

=== modified file 'libubuntuoneauth/keyring.cpp'
--- libubuntuoneauth/keyring.cpp	2015-03-12 13:13:46 +0000
+++ libubuntuoneauth/keyring.cpp	2016-03-30 13:47:05 +0000
@@ -38,6 +38,16 @@
         _account = nullptr;
     }
 
+    void Keyring::setAccount(QObject *account)
+    {
+        _account = qobject_cast<Accounts::Account*>(account);
+    }
+
+    QObject *Keyring::account() const
+    {
+        return _account;
+    }
+
     void Keyring::handleError(const SignOn::Error &error)
     {
         qCritical() << "Error:" << error.message();
@@ -103,7 +113,6 @@
     void Keyring::handleCredentialsStored(const quint32 id)
     {
         QString _acctName("ubuntuone");
-        AccountIdList _ids = _manager.accountList(_acctName);
 
         _account->selectService();
         _account->setCredentialsId(id);
@@ -133,6 +142,8 @@
             }
             _account = _manager.account(_ids[0]);
             qDebug() << "storeToken(): Using Ubuntu One account '" << _ids[0] << "'.";
+        } else if (_account) {
+            qDebug() << "storeToken(): using provided account";
         } else {
             qDebug() << "in storeToken(): no accounts found in accountList, creating new";
             _account = _manager.createAccount(_acctName);

=== modified file 'libubuntuoneauth/keyring.h'
--- libubuntuoneauth/keyring.h	2015-01-16 22:09:15 +0000
+++ libubuntuoneauth/keyring.h	2016-03-30 13:47:05 +0000
@@ -35,6 +35,9 @@
     public:
         explicit Keyring(QObject *parent=NULL);
 
+        void setAccount(QObject *account);
+        QObject *account() const;
+
         void findToken();
         void storeToken(Token token);
         void storeToken(Token token, const QString& displayName);

=== modified file 'libubuntuoneauth/ssoservice.cpp'
--- libubuntuoneauth/ssoservice.cpp	2015-01-15 21:12:35 +0000
+++ libubuntuoneauth/ssoservice.cpp	2016-03-30 13:47:05 +0000
@@ -77,6 +77,16 @@
                 this, SLOT(errorOccurred(const ErrorResponse&)));
     }
 
+    void SSOService::setAccount(QObject *account)
+    {
+        _keyring->setAccount(account);
+    }
+
+    QObject *SSOService::account() const
+    {
+        return _keyring->account();
+    }
+
     void SSOService::getCredentials()
     {
         _keyring->findToken();

=== modified file 'libubuntuoneauth/ssoservice.h'
--- libubuntuoneauth/ssoservice.h	2015-01-15 21:12:35 +0000
+++ libubuntuoneauth/ssoservice.h	2016-03-30 13:47:05 +0000
@@ -38,6 +38,9 @@
     public:
         explicit SSOService(QObject *parent = 0);
 
+        void setAccount(QObject *account);
+        QObject *account() const;
+
         void invalidateCredentials();
         void getCredentials();
         void login(QString email, QString password, QString twoFactorCode = QString());

=== modified file 'online-accounts-provider/NewAccount.qml'
--- online-accounts-provider/NewAccount.qml	2016-03-21 14:48:41 +0000
+++ online-accounts-provider/NewAccount.qml	2016-03-30 13:47:05 +0000
@@ -117,6 +117,7 @@
 
     UbuntuOneCredentialsService {
         id: u1credservice
+        account: __account.objectHandle
 
         onLoginOrRegisterSuccess: {
             handleSuccess();
@@ -252,17 +253,4 @@
             errorLabel.visible = false;
         }
     }
-
-    Component {
-        id: accountServiceComponent
-        AccountService {
-            autoSync: false
-        }
-    }
-
-    AccountServiceModel {
-        id: accountServices
-        includeDisabled: true
-        account: __account.objectHandle
-    }
 }

=== modified file 'qml-credentials-service/ubuntuone_credentials_service.cpp'
--- qml-credentials-service/ubuntuone_credentials_service.cpp	2013-11-05 14:53:04 +0000
+++ qml-credentials-service/ubuntuone_credentials_service.cpp	2016-03-30 13:47:05 +0000
@@ -26,6 +26,16 @@
 {
 }
 
+void UbuntuOneCredentialsService::setAccount(QObject *account)
+{
+    _service.setAccount(account);
+    Q_EMIT accountChanged();
+}
+
+QObject *UbuntuOneCredentialsService::account() const
+{
+    return _service.account();
+}
 
 // public API (Q_INVOKABLE)
 

=== modified file 'qml-credentials-service/ubuntuone_credentials_service.h'
--- qml-credentials-service/ubuntuone_credentials_service.h	2013-07-12 17:53:20 +0000
+++ qml-credentials-service/ubuntuone_credentials_service.h	2016-03-30 13:47:05 +0000
@@ -19,11 +19,15 @@
 {
     Q_OBJECT
     Q_DISABLE_COPY(UbuntuOneCredentialsService)
+    Q_PROPERTY(QObject *account READ account WRITE setAccount NOTIFY accountChanged)
 
 public:
     UbuntuOneCredentialsService(QQuickItem *parent = 0);
     ~UbuntuOneCredentialsService();
 
+    void setAccount(QObject *account);
+    QObject *account() const;
+
     Q_INVOKABLE void checkCredentials();
     Q_INVOKABLE void invalidateCredentials();
     Q_INVOKABLE void login(QString email, QString password, QString twoFactorCode = QString());
@@ -31,6 +35,7 @@
     Q_INVOKABLE void signUrl(QString url, QString method, bool asQuery = false);
 
 signals:
+    void accountChanged();
     void credentialsFound();
     void credentialsNotFound();
     void credentialsDeleted();