Merge lp:~dobey/ubuntuone-credentials/fix-cancel into lp:ubuntuone-credentials

[dobey]

Don't call sync so much.
Don't call resetUI on success or cancel, to avoid popping the keyboard.
Set the displayName of the account internally.
Do not set the display name, or enable or sync the account, in the qml.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:110
http://jenkins.qa.ubuntu.com/job/ubuntuone-credentials-ci/101/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntuone-credentials-utopic-amd64-ci/17
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntuone-credentials-utopic-armhf-ci/16
        deb: http://jenkins.qa.ubuntu.com/job/ubuntuone-credentials-utopic-armhf-ci/16/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntuone-credentials-utopic-i386-ci/10

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

review: Approve (continuous-integration)

[Alejandro J. Cura (alecu)]

Looks fine, seems to work better on the phone.

review: Approve

[Alejandro J. Cura (alecu)]

review: Approve

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:112
http://jenkins.qa.ubuntu.com/job/ubuntuone-credentials-ci/103/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntuone-credentials-utopic-amd64-ci/19
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntuone-credentials-utopic-armhf-ci/18
        deb: http://jenkins.qa.ubuntu.com/job/ubuntuone-credentials-utopic-armhf-ci/18/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntuone-credentials-utopic-i386-ci/12

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

review: Approve (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:113
http://jenkins.qa.ubuntu.com/job/ubuntuone-credentials-ci/104/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntuone-credentials-utopic-amd64-ci/20
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntuone-credentials-utopic-armhf-ci/19
        deb: http://jenkins.qa.ubuntu.com/job/ubuntuone-credentials-utopic-armhf-ci/19/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntuone-credentials-utopic-i386-ci/13

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

review: Approve (continuous-integration)

[Alejandro J. Cura (alecu)]

After testing on the device, I can confirm that this branch fixes the cancel button, and fixes the creation of empty accounts. Those are very good and awesome news. Great work!

Unfortunately, this branch also breaks the login flow, so I think we cannot land it as is. We'll find someone to take a look at this next week while dobey is on vacations.

review: Needs Fixing

[Alberto Mardegan (mardy)]

I found out what's wrong when the plugin is called from an application (such as the dash): the sync() method is called too early.
It should be called only once, but only when the plugin has finished writing stuff into the account. While this branch improves things by reducing the number of times sync() is being called, it doesn't call sync after _account->setCredentialsId(id) has been called. This results in the account being created with a "credentialsId" field set to 0, and therefore there's no link between the account and the SignOn::Identity objects.

review: Needs Fixing

[dobey (dobey)]

> I found out what's wrong when the plugin is called from an application (such
> as the dash): the sync() method is called too early.
> It should be called only once, but only when the plugin has finished writing
> stuff into the account. While this branch improves things by reducing the
> number of times sync() is being called, it doesn't call sync after
> _account->setCredentialsId(id) has been called. This results in the account
> being created with a "credentialsId" field set to 0, and therefore there's no
> link between the account and the SignOn::Identity objects.

Actually, sync() is being called at the end, and only once, already in this branch. The call to sync() is made in NewAccount.qml in the handleSuccess() function, after the display name is updated. Adding another sync() call back to keyring.cpp in handleCredentialsStored() is resulting again in the multiple broken accounts being created, when testing on my phone.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:116
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~dobey/ubuntuone-credentials/fix-cancel/+merge/230540/+edit-commit-message

http://jenkins.qa.ubuntu.com/job/ubuntuone-credentials-ci/107/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntuone-credentials-utopic-amd64-ci/23
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntuone-credentials-utopic-armhf-ci/22
        deb: http://jenkins.qa.ubuntu.com/job/ubuntuone-credentials-utopic-armhf-ci/22/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntuone-credentials-utopic-i386-ci/16

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

review: Needs Fixing (continuous-integration)

[Alejandro J. Cura (alecu)]

I've just tested the debs from this branch on the device, works wonderfully.
- login and register from Online Accounts works
- login and register when trying to install an app works
- login and register when trying to purchase an app in staging works (but after this, completing the purchase is blocked by LP: #1355173)

The only issue is that U1 accounts now always show up as "grayed out", but it doesn't seem like a problem attributable to this branch.

big +1, great work.

review: Approve

[lp:~dobey/ubuntuone-credentials/fix-cancel]

117. By dobey on 2014-08-13

Enable the main account first.
Loop through all services and enable them when adding the credentials.

[Alberto Mardegan (mardy)]

I didn't test it, but code-wise looks OK.

review: Approve

=== modified file 'debian/libubuntuoneauth-2.0-0.symbols'
--- debian/libubuntuoneauth-2.0-0.symbols	2014-01-16 17:57:14 +0000
+++ debian/libubuntuoneauth-2.0-0.symbols	2014-08-13 16:11:15 +0000
@@ -111,6 +111,7 @@
  (c++)"UbuntuOne::Token::~Token()@Base" 13.08
  (c++)"UbuntuOne::Token::~Token()@Base" 13.08
  (c++)"UbuntuOne::Keyring::storeToken(UbuntuOne::Token)@Base" 13.08
+ (c++)"UbuntuOne::Keyring::storeToken(UbuntuOne::Token, QString const&)@Base" 14.04+14.10.20140812
  (c++)"UbuntuOne::Keyring::tokenFound(UbuntuOne::Token const&)@Base" 13.08
  (c++)"UbuntuOne::Keyring::deleteToken()@Base" 13.08
  (c++)"UbuntuOne::Keyring::handleError(SignOn::Error const&)@Base" 13.08

=== modified file 'libubuntuoneauth/keyring.cpp'
--- libubuntuoneauth/keyring.cpp	2014-04-15 15:29:51 +0000
+++ libubuntuoneauth/keyring.cpp	2014-08-13 16:11:15 +0000
@@ -1,5 +1,5 @@
 /*
- * Copyright 2013 Canonical Ltd.
+ * Copyright 2013-2014 Canonical Ltd.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of version 3 of the GNU Lesser General Public
@@ -35,6 +35,7 @@
         : QObject(parent),
           _manager()
     {
+        _account = nullptr;
     }
 
     void Keyring::handleError(const SignOn::Error &error)
@@ -103,67 +104,51 @@
     {
         QString _acctName("ubuntuone");
         AccountIdList _ids = _manager.accountList(_acctName);
-        Account *account;
-
-        if (_ids.length() > 0) {
-            if (_ids.length() > 1) {
-                qDebug() << "handleCredentialsStored(): Found '" << _ids.length() << "' accounts. Using first.";
-            }
-            account = _manager.account(_ids[0]);
-            qDebug() << "handleCredentialsStored(): Using Ubuntu One account '" << _ids[0] << "'.";
-            account->selectService();
-            account->setCredentialsId(id);
-
-            ServiceList services = account->services(_acctName);
-            if (services.length() > 0) {
-                account->selectService(services[0]);
-            } else {
-                QString errMsg("Unable to enable 'ubuntuone' service.");
-                emit keyringError(errMsg);
-            }
-            account->setEnabled(true);
-            account->sync();
-            emit tokenStored();
-            return;
+
+        _account->selectService();
+        _account->setCredentialsId(id);
+
+        for (auto service: _account->services()) {
+            _account->selectService(service);
+            _account->setEnabled(true);
         }
-        emit keyringError(QStringLiteral("Could not sync credentials ID."));
+        _account->sync();
+        emit tokenStored();
     }
 
     void Keyring::storeToken(Token token)
     {
+        storeToken(token, "");
+    }
+
+    void Keyring::storeToken(Token token, const QString& displayName)
+    {
         QString _acctName("ubuntuone");
         AccountIdList _ids = _manager.accountList(_acctName);
         Identity *identity = NULL;
-        Account *account = NULL;
 
         if (_ids.length() > 0) {
             if (_ids.length() > 1) {
                 qDebug() << "storeToken(): Found '" << _ids.length() << "' accounts. Using first.";
             }
-            account = _manager.account(_ids[0]);
+            _account = _manager.account(_ids[0]);
             qDebug() << "storeToken(): Using Ubuntu One account '" << _ids[0] << "'.";
         } else {
             qDebug() << "in storeToken(): no accounts found in accountList, creating new";
-            account = _manager.createAccount(_acctName);
-
-            ServiceList services = account->services(_acctName);
-            if (services.length() > 0) {
-                account->selectService(services[0]);
-            } else {
-                QString errMsg("Unable to enable 'ubuntuone' service.");
-                emit keyringError(errMsg);
-            }
-
-            account->setEnabled(true);
-            account->sync();
-        }
-
-        if(account->credentialsId() == 0){
-            qDebug() << "storeToken() : creating new Identity for account " << account->id() ;
+            _account = _manager.createAccount(_acctName);
+        }
+        _account->setEnabled(true);
+
+        if (!displayName.isEmpty()) {
+            _account->setDisplayName(displayName);
+        }
+
+        if(_account->credentialsId() == 0) {
+            qDebug() << "storeToken() : creating new Identity for account " << _account->id() ;
             identity = Identity::newIdentity();
-        }else{
+        } else {
             qDebug() << "storeToken(): identity found.";
-            identity = Identity::existingIdentity(account->credentialsId());
+            identity = Identity::existingIdentity(_account->credentialsId());
         }
 
         Q_ASSERT(identity != NULL);

=== modified file 'libubuntuoneauth/keyring.h'
--- libubuntuoneauth/keyring.h	2013-08-20 21:15:53 +0000
+++ libubuntuoneauth/keyring.h	2014-08-13 16:11:15 +0000
@@ -1,5 +1,5 @@
 /*
- * Copyright 2013 Canonical Ltd.
+ * Copyright 2013-2014 Canonical Ltd.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of version 3 of the GNU Lesser General Public
@@ -18,6 +18,7 @@
 #ifndef _U1_KEYRING_H_
 #define _U1_KEYRING_H_
 
+#include <Accounts/Account>
 #include <Accounts/Manager>
 #include <SignOn/Identity>
 
@@ -36,6 +37,7 @@
 
         void findToken();
         void storeToken(Token token);
+        void storeToken(Token token, const QString& displayName);
         void deleteToken();
 
     Q_SIGNALS:
@@ -54,6 +56,7 @@
 
     private:
         Accounts::Manager _manager;
+        Accounts::Account *_account;
     };
 
 } /* namespace UbuntuOne */

=== modified file 'libubuntuoneauth/ssoservice.cpp'
--- libubuntuoneauth/ssoservice.cpp	2014-07-09 20:22:51 +0000
+++ libubuntuoneauth/ssoservice.cpp	2014-08-13 16:11:15 +0000
@@ -146,7 +146,7 @@
     {
         Token realToken = Token(token.token_key(), token.token_secret(),
                                 token.consumer_key(), token.consumer_secret());
-        _keyring->storeToken(realToken);
+        _keyring->storeToken(realToken, _tempEmail);
     }
 
     void SSOService::accountPinged(QNetworkReply*)

=== modified file 'online-accounts-provider/NewAccount.qml'
--- online-accounts-provider/NewAccount.qml	2014-08-01 19:53:01 +0000
+++ online-accounts-provider/NewAccount.qml	2014-08-13 16:11:15 +0000
@@ -41,8 +41,6 @@
 
     Component.onCompleted: {
         resetUI();
-        enableAccount();
-        __account.sync()
     }
 
     Label {
@@ -181,7 +179,6 @@
         console.debug("Removing account ID: " + account.accountId);
         account.removed.connect(finished);
         account.remove(Account.RemoveCredentials);
-        resetUI();
     }
 
     /* processForm uses a timer to delay calling u1credservice, which can
@@ -222,11 +219,7 @@
     function handleSuccess() {
         loadingOverlay.visible = false;
         errorLabel.visible = false;
-        account.updateDisplayName(emailTextField.text);
-        account.updateEnabled(true);
-        account.synced.connect(main.finished);
-        account.sync();
-        resetUI();
+        main.finished();
     }
 
     function showTwoFactorUI() {
@@ -273,15 +266,4 @@
         includeDisabled: true
         account: __account.objectHandle
     }
-
-    function enableAccount() {
-        for (var i = 0; i < accountServices.count; i++) {
-            var accountServiceHandle = accountServices.get(i, "accountService")
-            var accountService = accountServiceComponent.createObject(null,
-                                     { "objectHandle": accountServiceHandle })
-            accountService.updateServiceEnabled(true)
-            accountService.destroy(1000)
-        }
-    }
-
 }