Merge lp:~stolowski/unity-scope-click/remove-assertion-and-old-api-fallback into lp:unity-scope-click/devel

[Paweł Stołowski]

Remove fallback to old server API if highlights call fails, this was needed only initially when server was not ready. Ensure we call ad highlights only if there was no network error (check general status, not the http code). Replaced assert with warning and early return.

Remove fallback to old server API if highlights call fails, this was needed only initially when server was not ready. Ensure we call ad highlights only if there was no network error (check general status, not the http code). Replaced assert with warning and early return.

No new tests, since this is existing functionality and this change makes it more robust.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:354
http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-ci/228/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-utopic-amd64-ci/203
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-utopic-armhf-ci/202
        deb: http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-utopic-armhf-ci/202/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-utopic-i386-ci/202

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

review: Approve (continuous-integration)

[dobey (dobey)]

review: Approve

=== modified file 'scope/clickstore/store-query.cpp'
--- scope/clickstore/store-query.cpp	2014-07-21 20:49:18 +0000
+++ scope/clickstore/store-query.cpp	2014-07-22 12:43:58 +0000
@@ -357,11 +357,16 @@
 void click::Query::add_highlights(scopes::SearchReplyProxy const& searchReply, const PackageSet& locallyInstalledApps)
 {
     auto curdep = impl->department_lookup.get_department_info(query().department_id());
-    assert(curdep);
-    auto subdepts = curdep->sub_departments();
+    if (!curdep)
+    {
+        qWarning() << "No department information for current department" << QString::fromStdString(query().department_id());
+        return;
+    }
+
     if (query().department_id() == "") // top-level departments
     {
         unity::scopes::Department::SPtr root;
+        auto subdepts = curdep->sub_departments();
         populate_departments(subdepts, query().department_id(), root);
         push_departments(searchReply, root);
 
@@ -396,6 +401,8 @@
                                       const PackageSet& installedPackages,
                                       const std::string& categoryTemplate)
 {
+    // this assertion is here to ensure unit tests are properly implemented.
+    // this pointer is never null during normal execution.
     assert(searchReply);
 
     run_under_qt([=]()
@@ -437,11 +444,11 @@
             };
 
             // this is the case when we do bootstrap for the first time, or it failed last time
-            if (impl->department_lookup.size() == 0 && !click::Scope::use_old_api())
+            if (impl->department_lookup.size() == 0)
             {
                 qDebug() << "performing bootstrap request";
                 impl->search_operation = impl->index.bootstrap([this, search_cb, searchReply, installedPackages](const DepartmentList& deps, const
-                            HighlightList& highlights, click::Index::Error error, int error_code) {
+                            HighlightList& highlights, click::Index::Error error, int) {
                 if (error == click::Index::Error::NoError)
                 {
                     qDebug() << "bootstrap request completed";
@@ -465,27 +472,25 @@
                 else
                 {
                     qWarning() << "bootstrap request failed";
-                    if (error_code == 405 || error_code == 404) // method not allowed or resource not found
-                    {
-                        qDebug() << "bootstrap not available, using old API";
-                        click::Scope::set_use_old_api();
-                    }
                 }
 
-                if (query().query_string().empty() && !click::Scope::use_old_api() && error_code == 0)
-                {
-                    add_highlights(searchReply, installedPackages);
-                }
-                else
-                {
-                    qDebug() << "starting search of" << QString::fromStdString(query().query_string());
-                    impl->search_operation = impl->index.search(query().query_string(), search_cb);
+                if (error == click::Index::Error::NoError)
+                {
+                    if (query().query_string().empty())
+                    {
+                        add_highlights(searchReply, installedPackages);
+                    }
+                    else
+                    {
+                        qDebug() << "starting search of" << QString::fromStdString(query().query_string());
+                        impl->search_operation = impl->index.search(query().query_string(), search_cb);
+                    }
                 }
             });
         }
         else
         {
-            if (query().query_string().empty() && !click::Scope::use_old_api())
+            if (query().query_string().empty())
             {
                 add_highlights(searchReply, installedPackages);
             }

=== modified file 'scope/clickstore/store-scope.cpp'
--- scope/clickstore/store-scope.cpp	2014-07-18 15:01:41 +0000
+++ scope/clickstore/store-scope.cpp	2014-07-22 12:43:58 +0000
@@ -45,8 +45,6 @@
 #include <logging.h>
 #include <iostream>
 
-bool click::Scope::old_api = false;
-
 click::Scope::Scope()
 {
     nam.reset(new click::network::AccessManager());
@@ -70,16 +68,6 @@
 {
 }
 
-void click::Scope::set_use_old_api()
-{
-    old_api = true;
-}
-
-bool click::Scope::use_old_api()
-{
-    return old_api;
-}
-
 void click::Scope::start(std::string const&, scopes::RegistryProxy const&)
 {
     setlocale(LC_ALL, "");

=== modified file 'scope/clickstore/store-scope.h'
--- scope/clickstore/store-scope.h	2014-07-16 21:49:11 +0000
+++ scope/clickstore/store-scope.h	2014-07-22 12:43:58 +0000
@@ -67,9 +67,6 @@
 
     virtual unity::scopes::ActivationQueryBase::UPtr perform_action(unity::scopes::Result const& result, unity::scopes::ActionMetadata const& metadata, std::string const& widget_id, std::string const& action_id) override;
 
-    static void set_use_old_api();
-    static bool use_old_api();
-
 private:
     QSharedPointer<click::network::AccessManager> nam;
     QSharedPointer<click::web::Client> client;
@@ -80,7 +77,6 @@
     std::shared_ptr<pay::Package> pay_package;
 
     std::string installApplication(unity::scopes::Result const& result);
-    static bool old_api;
 };
 }
 #endif // CLICK_SCOPE_H