Merge lp:~mikemc/unity-scope-click/startdownload-error-handling into lp:unity-scope-click

[Mike McCracken]

- Surface errors in the download and install process via ErrorPreview

- Surface errors in the download and install process via ErrorPreview

Shows user error messages when
- creds are not found
- any error is returned from download manager, including network errors or failed install

Other changes:
- fix tests to account for separate error signals for creds vs. downloading/installing.
- fix a problem in the test matcher for the pkcon command
- use constants instead of strings in some places
- cleanup whitespace

TO TEST:

The download manager tests are currently disabled because they're flaky on ARM.
To run all the tests, which should pass on x86, do:
% GTEST_ALSO_RUN_DISABLED_TESTS=1 U1_DEBUG=1 make test

TO TEST IRL:
using unity-scope-tool or a device, try installing an app with:

- no credentials - you should get a specific error message for this
- no network - you should get a network error or a download/install error, depending on when you cut the net.
- install error - not sure how to trigger this on the device. maybe you could mess with permissions of the click directory temporarily. on desktop, it's easy, since pkcon doesn't work with clicks on desktop, according to the packagekit-plugin-click README:
       http://bazaar.launchpad.net/~click-hackers/click/trunk/view/head:/pk-plugin/README

In each case you should get a serviceable (untranslated, un-designed for wording) error message.

[lp:~mikemc/unity-scope-click/startdownload-error-handling]

153. By Mike McCracken on 2014-02-20

remove unused boost/optional header

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:153
http://jenkins.qa.ubuntu.com/job/unity-scope-click-ci/326/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scope-click-trusty-amd64-ci/227
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scope-click-trusty-armhf-ci/224
        deb: http://jenkins.qa.ubuntu.com/job/unity-scope-click-trusty-armhf-ci/224/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-scope-click-ci/326/rebuild

review: Approve (continuous-integration)

[Diego Sarmentero (diegosarmentero)]

+1

review: Approve

[Alejandro J. Cura (alecu)]

Looks good

review: Approve

=== modified file 'scope/click/download-manager.cpp'
--- scope/click/download-manager.cpp	2014-02-18 11:26:15 +0000
+++ scope/click/download-manager.cpp	2014-02-20 06:27:03 +0000
@@ -215,7 +215,7 @@
 void click::DownloadManager::handleCredentialsNotFound()
 {
     qDebug() << "No credentials were found.";
-    emit clickTokenFetchError(QString("No creds found"));
+    emit credentialsNotFound();
 }
 
 void click::DownloadManager::handleNetworkFinished()
@@ -295,7 +295,8 @@
     // TODO, unimplemented. see https://bugs.launchpad.net/ubuntu-download-manager/+bug/1277814
 }
 
-void click::Downloader::startDownload(std::string url, std::string package_name, const std::function<void (std::string)>& callback)
+void click::Downloader::startDownload(std::string url, std::string package_name,
+                                      const std::function<void (std::pair<std::string, click::InstallError >)>& callback)
 {
     qt::core::world::enter_with_task([this, callback, url, package_name] (qt::core::world::Environment& /*env*/)
     {
@@ -304,16 +305,30 @@
         QObject::connect(&dm, &click::DownloadManager::downloadStarted,
                          [callback](QString downloadId)
                          {
-                             std::cout << "Download started: " << downloadId.toStdString() << std::endl;
-                             callback(downloadId.toUtf8().data());
-                         });
+                             qDebug() << "Download started, id: " << downloadId;
+                             auto ret = std::pair<std::string, click::InstallError>(downloadId.toUtf8().data(),
+                                                                                    click::InstallError::NoError);
+                             callback(ret);
+                         });
+
+        QObject::connect(&dm, &click::DownloadManager::credentialsNotFound,
+                         [callback]()
+                         {
+                             qDebug() << "Credentials not found:";
+                             auto ret = std::pair<std::string, click::InstallError>(std::string(),
+                                                                                    click::InstallError::CredentialsError);
+                             callback(ret);
+                         });
+
         QObject::connect(&dm, &click::DownloadManager::downloadError,
-                         [](QString errorMessage)
+                         [callback](QString errorMessage)
                          {
-                             // TODO: unclear how to handle errors
                              qDebug() << "Error creating download:" << errorMessage;
+                             auto ret = std::pair<std::string, click::InstallError>(errorMessage.toStdString(),
+                                                                                    click::InstallError::DownloadInstallError);
+                             callback(ret);
                          });
-        std::cout << "About to call start download" << std::endl;
+
         dm.startDownload(QString::fromStdString(url),
                           QString::fromStdString(package_name));
     });

=== modified file 'scope/click/download-manager.h'
--- scope/click/download-manager.h	2014-02-18 11:26:15 +0000
+++ scope/click/download-manager.h	2014-02-20 06:27:03 +0000
@@ -67,6 +67,8 @@
     virtual void fetchClickToken(const QString& downloadUrl);
 
 signals:
+
+    void credentialsNotFound();
     void downloadStarted(const QString& downloadObjectPath);
     void downloadError(const QString& errorMessage);
     void clickTokenFetched(const QString& clickToken);
@@ -86,13 +88,15 @@
     QScopedPointer<Private> impl;
 };
 
+enum class InstallError {NoError, CredentialsError, DownloadInstallError};
 
 class Downloader
 {
 public:
     Downloader(const QSharedPointer<click::network::AccessManager>& networkAccessManager);
     void get_download_progress(std::string package_name, const std::function<void (std::string)>& callback);
-    void startDownload(std::string url, std::string package_name, const std::function<void (std::string)>& callback);
+    void startDownload(std::string url, std::string package_name,
+                       const std::function<void (std::pair<std::string, InstallError>)>& callback);
 private:
     QSharedPointer<click::network::AccessManager> networkAccessManager;
 };

=== modified file 'scope/click/preview.cpp'
--- scope/click/preview.cpp	2014-02-20 06:27:03 +0000
+++ scope/click/preview.cpp	2014-02-20 06:27:03 +0000
@@ -41,6 +41,8 @@
 
 #include <QDebug>
 
+#include <boost/optional.hpp>
+
 #include <sstream>
 
 namespace
@@ -125,12 +127,14 @@
     reply->push(widgets);
 }
 
-void buildErrorPreview(scopes::PreviewReplyProxy const& reply)
+void buildErrorPreview(scopes::PreviewReplyProxy const& reply,
+                       const std::string& error_message)
 {
     scopes::PreviewWidgetList widgets;
 
     scopes::PreviewWidget header("hdr", "header");
     header.add_attribute("title", scopes::Variant("Error"));
+    header.add_attribute("subtitle", scopes::Variant(error_message));
     widgets.push_back(header);
 
     scopes::PreviewWidget buttons("buttons", "actions");
@@ -151,6 +155,7 @@
 
     scopes::PreviewWidget header("hdr", "header");
     header.add_attribute("title", scopes::Variant("Login Error"));
+    header.add_attribute("subtitle", scopes::Variant("Please log in to your Ubuntu One account."));
     widgets.push_back(header);
 
     scopes::PreviewWidget buttons("buttons", "actions");
@@ -312,9 +317,6 @@
         case Type::UNINSTALLED:
             buildUninstalledPreview(reply, details);
             break;
-        case Type::ERROR:
-            buildErrorPreview(reply);
-            break;
         case Type::LOGIN:
             buildLoginErrorPreview(reply);
             break;
@@ -328,8 +330,12 @@
             buildPurchasingPreview(reply, details);
             break;
         case Type::DEFAULT:
+        case Type::ERROR:
+            // Don't showPreview() with errors. always use the error string.
         default:
-            buildDefaultPreview(reply, details);
+            qDebug() << "reached default preview type, returning internal error preview";
+            buildErrorPreview(reply,
+                              std::string("Internal Error, please close and try again."));
             break;
     };
 }
@@ -339,6 +345,30 @@
     this->type = type;
 }
 
+
+// ErrorPreview
+
+ErrorPreview::ErrorPreview(const std::string& error_message,
+                           const QSharedPointer<click::Index>& index,
+                           const unity::scopes::Result &result)
+    : Preview(result.uri(), index, result), error_message(error_message)
+{
+    qDebug() << "in ErrorPreview constructor, error_message is " << QString::fromStdString(error_message);
+}
+
+ErrorPreview::~ErrorPreview()
+{
+
+}
+
+void ErrorPreview::run(const unity::scopes::PreviewReplyProxy &reply)
+{
+    buildErrorPreview(reply, error_message);
+}
+
+
+// InstallPreview
+
 InstallPreview::InstallPreview(const std::string &download_url, const QSharedPointer<Index> &index,
                                const unity::scopes::Result &result,
                                const QSharedPointer<click::network::AccessManager> &nam)
@@ -356,12 +386,22 @@
 void InstallPreview::run(const unity::scopes::PreviewReplyProxy &reply)
 {
     qDebug() << "about to call startDownload in run()";
-    downloader->startDownload(download_url, result["name"].get_string(),[this, reply](std::string obj_path) {
-            qDebug() << "got object path: " << QString::fromStdString(obj_path);
-            index->get_details(result["name"].get_string(), [this, reply, obj_path](const click::PackageDetails& details)
-                               {
-                                   buildInstallingPreview(reply, details, obj_path);
-                               });
+    downloader->startDownload(download_url, result["name"].get_string(),[this, reply](std::pair<std::string, click::InstallError> pair) {
+
+            auto error = pair.second;
+
+            if (error == InstallError::NoError) {
+                auto obj_path = pair.first;
+                qDebug() << "got object path: " << QString::fromStdString(obj_path);
+                index->get_details(result["name"].get_string(), [this, reply, obj_path](const click::PackageDetails& details)
+                                   {
+                                       buildInstallingPreview(reply, details, obj_path);
+                                   });
+            } else if (error == InstallError::CredentialsError) { 
+                buildLoginErrorPreview(reply);
+            } else {
+                buildErrorPreview(reply, pair.first);
+            }
         });
     qDebug() << "after startDownload in run()";
 }

=== modified file 'scope/click/preview.h'
--- scope/click/preview.h	2014-02-18 12:44:49 +0000
+++ scope/click/preview.h	2014-02-20 06:27:03 +0000
@@ -108,6 +108,21 @@
                      const click::PackageDetails& details);
 };
 
+class ErrorPreview : public Preview
+{
+protected:
+    std::string error_message;
+public:
+    ErrorPreview(const std::string& error_message,
+                   const QSharedPointer<click::Index>& index,
+                   const unity::scopes::Result& result);
+
+    virtual ~ErrorPreview();
+
+    void run(unity::scopes::PreviewReplyProxy const& reply) override;
+
+};
+
 class InstallPreview : public Preview
 {
 protected:

=== modified file 'scope/click/scope.cpp'
--- scope/click/scope.cpp	2014-02-20 06:27:03 +0000
+++ scope/click/scope.cpp	2014-02-20 06:27:03 +0000
@@ -101,10 +101,16 @@
 
     if (metadata.scope_data().which() != scopes::Variant::Type::Null) {
         auto metadict = metadata.scope_data().get_dict();
-        if (metadict.count("download_completed") != 0) {
+
+        if(metadict.count(click::Preview::Actions::DOWNLOAD_FAILED) != 0) {
+            return scopes::QueryBase::UPtr{new ErrorPreview(std::string("Download or install failed. Please try again."),
+                                                            index, result)};
+
+        } else if (metadict.count(click::Preview::Actions::DOWNLOAD_COMPLETED) != 0) {
             Preview* prev = new Preview(result.uri(), index, result);
             prev->setPreview(click::Preview::Type::INSTALLED);
             return scopes::QueryBase::UPtr{prev};
+
         } else if (metadict.count("action_id") != 0  &&
             metadict.count("download_url") != 0) {
             action_id = metadict["action_id"].get_string();
@@ -132,8 +138,13 @@
         activation->setHint("action_id", unity::scopes::Variant(action_id));
         qDebug() << "returning ShowPreview";
         activation->setStatus(unity::scopes::ActivationResponse::Status::ShowPreview);
+
+    } else if (action_id == click::Preview::Actions::DOWNLOAD_FAILED) {
+        activation->setHint(click::Preview::Actions::DOWNLOAD_FAILED, unity::scopes::Variant(true));
+        activation->setStatus(unity::scopes::ActivationResponse::Status::ShowPreview);
+
     } else if (action_id == click::Preview::Actions::DOWNLOAD_COMPLETED) {
-        activation->setHint("download_completed", unity::scopes::Variant(true));
+        activation->setHint(click::Preview::Actions::DOWNLOAD_COMPLETED, unity::scopes::Variant(true));
         activation->setStatus(unity::scopes::ActivationResponse::Status::ShowPreview);
     }
     return scopes::ActivationBase::UPtr(activation);

=== modified file 'scope/tests/download_manager_tool/download_manager_tool.cpp'
--- scope/tests/download_manager_tool/download_manager_tool.cpp	2014-02-18 07:55:54 +0000
+++ scope/tests/download_manager_tool/download_manager_tool.cpp	2014-02-20 06:27:03 +0000
@@ -35,6 +35,8 @@
 
 #include <iostream>
 
+#include <boost/optional.hpp>
+
 #include <download_manager_tool.h>
 
 DownloadManagerTool::DownloadManagerTool(QObject *parent):
@@ -91,8 +93,13 @@
 
         QObject::connect(&timer, &QTimer::timeout, [&]() {
                 downloader.startDownload(std::string(argv[1]), std::string(argv[2]),
-                                         [&a] (std::string download_id){
-                                             std::cout << " Success, got download ID:" << download_id << std::endl;
+                                         [&a] (std::pair<std::string, click::InstallError> arg){
+                                             auto error = arg.second;
+                                             if (error == click::InstallError::NoError) {
+                                                 std::cout << " Success, got download ID:" << arg.first << std::endl;
+                                             } else {
+                                                 std::cout << " Error:" << arg.first << std::endl;
+                                             }
                                              a.quit();
                                          });
                 

=== modified file 'scope/tests/test_download_manager.cpp'
--- scope/tests/test_download_manager.cpp	2014-02-17 21:32:33 +0000
+++ scope/tests/test_download_manager.cpp	2014-02-20 06:27:03 +0000
@@ -184,11 +184,11 @@
         });
         signalTimer.start(10);
     }
-    
 };
 
 struct DownloadManagerMockClient
 {
+    MOCK_METHOD0(onCredentialsNotFoundEmitted, void());
     MOCK_METHOD1(onClickTokenFetchedEmitted, void(QString clickToken));
     MOCK_METHOD1(onClickTokenFetchErrorEmitted, void(QString errorMessage));
     MOCK_METHOD1(onDownloadStartedEmitted, void(QString id));
@@ -256,6 +256,12 @@
 
     DownloadManagerMockClient mockDownloadManagerClient;
 
+    QObject::connect(&dm, &click::DownloadManager::credentialsNotFound,
+                     [&mockDownloadManagerClient]()
+                     {
+                         mockDownloadManagerClient.onCredentialsNotFoundEmitted();
+                     });
+
     QObject::connect(&dm, &click::DownloadManager::clickTokenFetchError,
                      [&mockDownloadManagerClient](const QString& error)
                      {
@@ -282,12 +288,23 @@
 
     } else {
 
-        EXPECT_CALL(mockDownloadManagerClient, onClickTokenFetchErrorEmitted(_))
-            .Times(1)
-            .WillOnce(
-                InvokeWithoutArgs(
-                    this,
-                    &DISABLED_DownloadManagerCredsNetworkTest::Quit));
+        if (p.credsFound) {
+
+            EXPECT_CALL(mockDownloadManagerClient, onClickTokenFetchErrorEmitted(_))
+                .Times(1)
+                .WillOnce(
+                    InvokeWithoutArgs(
+                        this,
+                        &DISABLED_DownloadManagerCredsNetworkTest::Quit));
+        } else {
+
+            EXPECT_CALL(mockDownloadManagerClient, onCredentialsNotFoundEmitted())
+                .Times(1)
+                .WillOnce(
+                    InvokeWithoutArgs(
+                        this,
+                        &DISABLED_DownloadManagerCredsNetworkTest::Quit));
+        }
 
         EXPECT_CALL(mockDownloadManagerClient, onClickTokenFetchedEmitted(_)).Times(0);
 
@@ -326,12 +343,12 @@
 {
     auto commandList = arg.getMetadata()["post-download-command"].toStringList();
     return arg.getUrl() == TEST_URL
-        && arg.getHash() == "" 
+        && arg.getHash() == ""
         && arg.getAlgorithm() == ""
         && arg.getMetadata()["app_id"] == QVariant(TEST_APP_ID)
         && commandList[0] == "/bin/sh"
         && commandList[1] == "-c"
-        && commandList[3] == "$files"
+        && commandList[3] == "$file"
         && arg.getHeaders()["X-Click-Token"] == TEST_CLICK_TOKEN_VALUE;
 }
 
@@ -385,14 +402,14 @@
                 });
         }
 
-        EXPECT_CALL(*mockSystemDownloadManager, 
+        EXPECT_CALL(*mockSystemDownloadManager,
                     createDownload(DownloadStructIsValid())).Times(1).WillOnce(InvokeWithoutArgs(downloadCreatedSignalFunc));
-    }        
+    }
 
     EXPECT_CALL(*mockCredentialsService, getCredentials())
         .Times(1).WillOnce(InvokeWithoutArgs(clickTokenSignalFunc));
 
-    
+
     DownloadManagerMockClient mockDownloadManagerClient;
 
     QObject::connect(&dm, &click::DownloadManager::downloadError,
@@ -418,7 +435,7 @@
                     &DISABLED_DownloadManagerStartDownloadTest::Quit));
 
         EXPECT_CALL(mockDownloadManagerClient, onDownloadErrorEmitted(_)).Times(0);
-  
+
         // when https://bugs.launchpad.net/ubuntu-download-manager/+bug/1278789
         // is fixed, we can add this assertion:
         // EXPECT_CALL(successfulDownload, start()).Times(1);
@@ -435,7 +452,7 @@
         EXPECT_CALL(mockDownloadManagerClient, onDownloadStartedEmitted(_)).Times(0);
 
     }
-    
+
     QTimer timer;
     timer.setSingleShot(true);
     QObject::connect(&timer, &QTimer::timeout, [&dm]() {