Merge lp:~mikemc/ubuntuone-credentials/report-more-errors into lp:ubuntuone-credentials

[Mike McCracken]

Add error signal for failure cases in Network class - covers connection failures.

Add error signal for failure cases in Network class - covers connection failures.

To test, turn off networking and try logging in via the music login app.
Without this branch, it will spin forever, because the network code never signals the upper layers that it has gotten an error and is giving up.

With the branch you should get a generic 'try again later' error message.

[Diego Sarmentero (diegosarmentero)]

+1

review: Approve

[dobey (dobey)]

+ emit ErrorOccurred(ErrorResponse(0, QString(), LOGIN_FAILED, QString()));

Why are these empty QString() exactly? Wouldn't it be more appropriate to include the message we're shoving to qDebug() here, in ErrorResponse object (I think one of those QString() is supposed to be the actual error message), and have a single qDebug() inside the ErrorResponse ctor (or an extra method on it, which does the qDebug() which we just call after creating?

review: Needs Information

[Mike McCracken (mikemc)]

The QStrings are empty because nothing in the signal handler uses them. The ErrorResponse object looks like it was written assuming that you'd have an HTTP response to fit in there.
So, for errors that happen lower than HTTP, it's an awkward fit.

Improvement based on irc discussion will be to use the ErrorResponse message instead of qDebug()s at each new spot, since they get logged by the first signal handler in the chain

[lp:~mikemc/ubuntuone-credentials/report-more-errors]

42. By Mike McCracken on 2013-07-05

merge add-2fa

43. By Mike McCracken on 2013-07-05

clean up use of ErrorResponse, add one more error signal to avoid ever returning without signaling.

44. By Mike McCracken on 2013-07-11

merge with add-2fa

45. By Mike McCracken on 2013-07-11

merge with add-2fa

[dobey (dobey)]

review: Approve

=== modified file 'libubuntuoneauth/network.cpp'
--- libubuntuoneauth/network.cpp	2013-06-14 22:39:59 +0000
+++ libubuntuoneauth/network.cpp	2013-07-11 23:11:26 +0000
@@ -23,10 +23,12 @@
 
 #include <QStringList>
 
+#include "errormessages.h"
 #include "network.h"
 #include "responses.h"
 #include "requests.h"
 
+#define NO_HTTP_REASON QString("No HTTP error reason")
 
 namespace UbuntuOne {
 
@@ -51,30 +53,35 @@
     /* TODO: see if we really need to do this check, we could just operate
        on a bad value as being an error rather than the extra check? */
     if (!statusAttr.isValid()) {
-        qDebug() << "Invalid status received!";
+        QString errmsg = QString("Invalid status attribute in Network::OnReply");
+        // Use login failed code, which results in generic error message:
+        emit ErrorOccurred(ErrorResponse(0, NO_HTTP_REASON, LOGIN_FAILED, errmsg));
         return;
     }
 
     int httpStatus = statusAttr.toInt();
 
-    qDebug() << "Network::OnReply from " << reply->url();
-    qDebug() << "Network::OnReply status: " << httpStatus;
+    qDebug() << "Network::OnReply from " << reply->url()
+             << " status: " << httpStatus;
     
     QByteArray payload = reply->readAll();
     if (payload.isEmpty()) {
-        qDebug() << "empty payload";
-        return; /* TODO: Do something to signal we're having a bad time. */
+        QString errmsg = QString("Network::OnReply: empty payload, giving up.");
+        emit ErrorOccurred(ErrorResponse(0, NO_HTTP_REASON, LOGIN_FAILED, errmsg));
+        return;
     }
  
     QJsonDocument document = QJsonDocument::fromJson(payload);
-    /* TODO: add logging or raise some type of error signal? */
+
     if (document.isEmpty()) {
-        qDebug() << "oops, received empty document";
+        QString errmsg = QString("Network::OnReply received empty document");
+        emit ErrorOccurred(ErrorResponse(0, NO_HTTP_REASON, LOGIN_FAILED, errmsg));
         return;
     }
 
     if (!document.isObject()) {
-        qDebug() << "uh oh, this isn't good.";
+        QString errmsg = QString("Network::OnReply received invalid QJsonDocument");
+        emit ErrorOccurred(ErrorResponse(0, NO_HTTP_REASON, LOGIN_FAILED, errmsg));
         return;
     }
     
@@ -92,7 +99,8 @@
         QVariant phraseAttr = reply->attribute(
                             QNetworkRequest::HttpReasonPhraseAttribute);
         if (!phraseAttr.isValid()) {
-            qDebug() << "got a bad HTTP reason";
+            QString errmsg = QString("HTTP reason phrase is invalid");
+            emit ErrorOccurred(ErrorResponse(httpStatus, NO_HTTP_REASON, LOGIN_FAILED, errmsg));
             return;
         }
         QString httpReason = phraseAttr.toString();

=== modified file 'libubuntuoneauth/ssoservice.cpp'
--- libubuntuoneauth/ssoservice.cpp	2013-07-11 23:11:26 +0000
+++ libubuntuoneauth/ssoservice.cpp	2013-07-11 23:11:26 +0000
@@ -71,7 +71,7 @@
                 this, SLOT(handleTwoFactorAuthRequired()));
         connect(&(_provider),
                 SIGNAL(ErrorOccurred(const ErrorResponse&)),
-                this, SLOT(errorOcurred(const ErrorResponse&)));
+                this, SLOT(errorOccurred(const ErrorResponse&)));
     }
 
     void SSOService::getCredentials()
@@ -197,7 +197,7 @@
         _keyring->deleteToken();
     }
 
-    void SSOService::errorOcurred(const ErrorResponse& error)
+    void SSOService::errorOccurred(const ErrorResponse& error)
     {
         _tempPassword = "";
         emit requestFailed(error);

=== modified file 'libubuntuoneauth/ssoservice.h'
--- libubuntuoneauth/ssoservice.h	2013-07-11 23:11:26 +0000
+++ libubuntuoneauth/ssoservice.h	2013-07-11 23:11:26 +0000
@@ -63,7 +63,7 @@
             void handleCredentialsNotFound();
             void tokenReceived(const OAuthTokenResponse& token);
             void accountRegistered(const AccountResponse& account);
-            void errorOcurred(const ErrorResponse&);
+            void errorOccurred(const ErrorResponse&);
             void handleTwoFactorAuthRequired();
 
     private: