Merge lp:~knitzsche/scope-aggregator/fixes-fallback-bug-1599948 into lp:scope-aggregator

[Kyle Nitzsche]

Fixes the fallback mechanism through which the aggregator scope
can attempt to find, for example, art in other result fields, for
example mascot, or emblem. There was an exception when a result
did not contain the primary field because in some cases the code
tried to access a field that did not exist.

This problem causes exceptions when News scope aggregates El Pais
(which sometimes lacks expected attributes) and Yahoo, which now
always lacks "art", "mascot" and "emblem" -- see:
https://bugs.launchpad.net/news-scope/+bug/1600026

[Kyle Nitzsche (knitzsche)]

FYI: I am adding googletest unit tests to scope-aggregator and I will definitely add tests for fallback mechanism.

[Zhang Enwei (zhangew401)]

Look good to me

review: Approve

[lp:~knitzsche/scope-aggregator/fixes-fallback-bug-1599948]

170. By Kyle Nitzsche on 2016-07-12

When testing photos scope I found an issue that this commit fixes.

Please review. The key change is line 1192.

DETAILS

The issue I noticed when testing this MP in photos-scope is that the art for
Douban LOGIN result did not display in photos agg scope.

I found that the fallback code (check_result_fallbacks()) was not called
for Duoban in the login case.

The general case I found is: For scopes that are declared in child_scopes.json
using the "scope" key, the check_results_callbacks() method was not called.

(Please note that fallbacks code/feature is only currently implemented
for scopes that use a renderer defined in common_templates.)

Teseting. Modify photos scope child_scopes.json to only this for Duoban:

        "scope": {
            "_category_title": "My Douban Photos",
            "id": "com.canonical.scopes.douban_doubanalbum",
            "local_id": "douban",
            "result_category_id_to_common_template": [
              {
                "result_category_id": "login",
                "common_template": "not_logged_in"
              }
            ],
            "renderer_common_id": "normal_renderer"
        }

This ^ means:
 * if Douban result category is "login": use common_template "not logged_in".
    This template uses a fallback:
            "id": "not_logged_in",
            "fallbacks":
            [
                {
                    "key": "mascot",
                    "fields":
                    [
                        {"field": "mascot"},
                        {"field": "art"}
                    ]
                }
            ],
    [...]
    Result is that Douban's login result which provides "art" is converted to
    "mascot".
* A result with any other category uses "normal_renderer". This does not use a
  fallback and expects the result to contain 'art".

[Kyle Nitzsche (knitzsche)]

Note that the douban login art previously did not display in photos agg, so this is not a regression caused by the MP. Good time to fix it though.

[Zhang Enwei (zhangew401)]

LGTM

review: Approve

=== modified file 'CMakeLists.txt'
--- CMakeLists.txt	2016-06-29 10:29:37 +0000
+++ CMakeLists.txt	2016-07-12 17:15:04 +0000
@@ -1,4 +1,4 @@
-set(VERSION "4.8")
+set(VERSION "4.9")
 
 # Supress qDebug() output
 ADD_DEFINITIONS( -DQT_NO_DEBUG_OUTPUT )
 
=== modified file 'src/query.cpp'
--- src/query.cpp	2016-06-29 10:29:37 +0000
+++ src/query.cpp	2016-07-12 17:15:04 +0000
@@ -392,15 +392,19 @@
         const auto & map = *common_templates_fallbacks[common_id];
         for(auto iter = map.begin(); iter != map.end(); ++iter)
         {
-            qDebug() << "==== check FALLB first: " << qstr(iter->first);
+            qDebug() << "==== check FALLB field to populate: " << qstr(iter->first);
+            if (res.contains(iter->first))
+            {
+                qDebug() << "=== check FALLB. res has field: " << qstr(iter->first);
+                return;
+            }
             for (const auto & field : iter->second)
             {
-                qDebug() << "==== check FALLB field: " << qstr(field);
-                if (res[iter->first].get_string().empty())
+                qDebug() << "==== check FALLB field exists: " << qstr(field);
+                if (res.contains(field))
                 {
-                    qDebug() << "==== check FALLB first is empty: " << qstr(res[iter->first].get_string());
-                    if (res.contains(field))
-                        res[iter->first] = res[field];
+                    res[iter->first] = res[field].get_string();
+                    return;
                 }
             }
         }
@@ -557,7 +561,8 @@
 
             // save bits of incoming result
             std::string inc_res_cat_id = res.category()->id();
-            us::Category::SCPtr res_cat = res.category();
+            qDebug() <<"=== incoming result cat rdr: " << qstr(res.category()->renderer_template().data());
+            qDebug() <<"=== incoming result: " << qstr(us::Variant(res.serialize()).serialize_json());
             std::string inc_res_cat_title = res.category()->title();
 
             qDebug() << "==== RESULT scope: " << qstr(scope->id());
@@ -1175,14 +1180,16 @@
                     // if needed, get renderer from result_category_to_common_template
                     if (common_templates.find(child_scopes_m[scope->local_id()]->result_category_id_to_common_template[res.category()->id()]) != common_templates.end())
                     {
-                        qDebug() << "==== DECLARED. category to renderer";
+                        qDebug() << "==== DECLARED. current category:" << qstr(res.category()->id());
                         std::string common_template_id = child_scopes_m[scope->local_id()]->result_category_id_to_common_template[res.category()->id()];
+                        qDebug() << "==== DECLARED. using common_template_id: " << qstr(common_template_id);
                         std::string cat_id = common_template_id + ":" + scope->local_id() + cat_title;
 
                         // if the common category has not yet been registered, register it
                         if (!upstream_reply->lookup_category(cat_id))
                         {
                             std::string template_to_use = common_templates[child_scopes_m[scope->local_id()]->result_category_id_to_common_template[res.category()->id()]];
+                            check_result_fallbacks(res, common_template_id);
                             if (scope->category_link_to_child())
                             {
                                 if (!upstream_reply->lookup_category(cat_id))
@@ -1287,7 +1294,7 @@
     for (size_t i = 0; i < replies.size(); ++i) {
         us::SearchMetadata metadata(search_metadata());
         std::string locID = scopes_m_int_localId[i];
-        qDebug() << "==== locID: " << QString::fromStdString(locID);
+        qDebug() << "==== subsearch locID: " << qstr(locID);
         auto scope = scopes_m[locID];
 
         if (scope->keyword_scope())
@@ -1317,9 +1324,7 @@
             metadata.set_cardinality( scope->cardinality() );
         }
 
-
-        qDebug() << "===- LOCAL_ID: " << qstr(scope->local_id());
-        metadata.set_hint(scope->local_id(), us::Variant("valueMeansNothing"));
+        metadata.set_hint(scope->id(), us::Variant("valueMeansNothing"));
 
         // dispatch subsearches differently depending on whether is keyword or declared scope
         qDebug() << "==== QUERY STRING: " << QString::fromStdString(query_string);
@@ -1403,7 +1408,8 @@
                         adj_qry,
                         scope->child_department(),
                         us::FilterState(),
-                        metadata, replies[i]
+                        metadata,
+                        replies[i]
                     );
                     break;
                 }
 
=== modified file 'src/utils.cpp'
--- src/utils.cpp	2016-06-29 10:29:37 +0000
+++ src/utils.cpp	2016-07-12 17:15:04 +0000
@@ -61,6 +61,7 @@
 {
     if (root.contains(QStringLiteral("cardinality_settings")))
     {
+        setting_cardinalities.clear();
         QJsonArray card_a = root[QStringLiteral("cardinality_settings")].toArray();
         for (const auto & item : card_a)
         {