Merge lp:~gary-wzl77/scope-aggregator/fix_1490828 into lp:scope-aggregator

[Gary.Wang]

load correct keyword for a scope with multiple keywords when dept changed.
Add link_to_child in keyword struct to make it possible to navigateto child scope.

load correct keyword for a scope with multiple keywords when dept changed.
Add link_to_child in keyword struct to make it possible to navigateto child scope.

[Kyle Nitzsche (knitzsche)]

Gary: Nice work.

A couple comments:

1) lines 30-41: This removes the code in handle_child_results() that skips a scope when it is not in the current department. I understand you added code to do the same thing for *keyword* scopes (lines 193 to 198). Also, the main lambda function that handles each child result *does* filter out results when they are not in the current department in all cases. So even with your MR the user should not see the scope if it is in the wrong department. But, there is no need to process results for scopes that are not in the current department with the lambda function, so the approach of removing lines 30-41 seems to result in unnecessary code execution (unless I have misunderstood something :) So it seems to me that lines 30-41 should not be removed. Thoughts?

2) You have added support for a declared "keyword" to have a link_to_child. I did not add this previously because a "keyword" by definition may include any number of child scopes (including zero). There is no guarantee that any particular child is installed. And there is no single child that should obviously be linked to given the dynamic and unpredictable nature or keyword aggregation. Are you sure we have a use case for this?

review: Needs Information

[lp:~gary-wzl77/scope-aggregator/fix_1490828]

149. By Gary.Wang on 2015-09-06

Add department id checker when using departments.

[Gary.Wang (gary-wzl77)]

1.Indeed, it might cause unnecessary code execution, which is sth I didn't take into account. I've re-submitted and move code[30-41] below scope registry list, since that's more reasonable.
2.Yes, I'm sure. To take a trivial instance, for all child scopes in bollywood aggregator, one of the requirement is there is a need for them to linked to corresponding child scope by clicking the header(child scope name). And as you said, not every child scope should obviously be linked to a given keyword aggregation. In such a case, I think developer could isolate the special one by declaring another keyword or scope json object since link_to_child property in keyword object is applied for all child scopes.

[Gary.Wang (gary-wzl77)]

Regarding question2, There is one issue related on this
e.g. amazon scope has keyword "books", if we simply use keyword as following
...
        {
            "keyword":
            {
                "keyword": "books",
                "link_to_child" : "true"
            }
        }
...

There're some results returned from amazon scope, but these results items have nth to do with "books" keywords. Reason for this is developer didn't handle "books" keyword in source code although scope supports the keyword.

So if we isolate amazon by declaring scope json object with default_query_string property as following
        {
            "scope":
            {
                "id": "com.canonical.scopes.amazon",
                "local_id": "amazon",
                "_category_title": "amazon",
                "default_query_string": "best selling books"
            }
        },
        {
            "keyword":
            {
                "keyword": "books",
                "link_to_child" : "true",
            }
        }

We can see there two amazon scope exists since keyword object is applied for all child scopes.First one is explicit declared amazon scope, second one is amazon scope with "books" keywords. That looks bad.
So two ways to fix the duplicated issue.

1. Fix in child scope(amazon)
2. Fix in scope aggregator with adding new property exclude_scopes
   e.g.
        {
            "scope":
            {
                "id": "com.canonical.scopes.amazon",
                "local_id": "amazon",
                "_category_title": "amazon",
                "default_query_string": "best selling books
            }
        },
        {
            "keyword":
            {
                "keyword": "books",
                "link_to_child" : "true",
                "exclude_scopes" :[
                    {
                        "id": "com.canonical.scopes.amazon"
                    }
                ]
            }
        }

So in this way, we keep the first one and second one will be ignored since it's excluded in keyword scope list. I've added this new property in scope agg and have a quick testing, It works fine.

If we own source code, I prefer 1st option.
In the future if scopes which we don't own source code have the same issue that amazon has. I think the second one is better one. At the same time, I think it's also applied for link_to_child property. If there is no need for one certain scope to navigate to child scope, we can write it as following
   e.g
        {
            "scope":
            {
                "id": "com.canonical.scopes.myscope",
                "local_id": "myscope",
                "link_to_child" : "false"
            }
        },
        {
            "keyword":
            {
                "keyword": "books",
                "link_to_child" : "true",
                "exclude_scopes" :[
                    {
                        "id": "com.canonical.scopes.myscope"
                    }
                ]
            }
        }

I'd like to hear you option on this and will submit my MR if this suggestion looks good to you.
Thanks

[Kyle Nitzsche (knitzsche)]

Hi Gary,

We already have "link_to_child_specified" in "category" json objects. Category objects, like keyword objects, have no obvious scope to link to so the dev can declare one.

So, how about if we add "link_to_child_specified" to category objects *and* add a new key to keyword (and category) objects: "link_to_child_default_query_string".

This way the keyword can link to a specific child *with* a default query string.

Note that "link_to_child_specified" should *only* be used for pre-installed scopes (to ensure the scope really is present). We may want to only add the link to the specified child when the registry reports it is actually installed.

[Gary.Wang (gary-wzl77)]

   Personally, I trend to prefer to add "exclude_scopes" property in keyword object, reasons as following:
   1. if add "link_to_child_default_query_string" property, query_string will be applied for all child scope, If one or *more* child scopes match keyword in keyword json object are not supposed to use "default_query_string"?
      let me take an instance I meet during my development
      e.g. there're 5 book child scopes [amazon, ebay, douban, open library, viva reader], all of them supports "books" keyword
        {
            "keyword":
            {
                "keyword": "books",
                "link_to_child" : "true",
                "default_query_string" : "best selling books"
            }
        }

       * In this case, amazon, ebay, open library works fine, but no results returned for douban, viva reader since both two Chinese child scope only support Chinese querying. However both child scope can give correct results when loading default department without query string just as open as usual.

     2. The main point for adding "exclude_scopes" is we can get rid of one or *more* scopes in keyword object and drop them or isolate them to display in another ways[category, scope] to avoid duplicated display issue for one or *more* scope in scope aggregator.

[Kyle Nitzsche (knitzsche)]

Hi Gary,

Regarding the proposal for keyword category link_to_child_specified and link_to_child_default_query_string:

Link_to_child_query_string would only be passed to the link_to_child scope, not to all scopes included by keyword in the keyword json obkect. (That's why I propose the name: link_to_child_query_string.)

        {
            "keyword":
            {
                "keyword": "books",
                "link_to_child_specified" : "SOMESCOPEID",
                "link_to_child_query_string" : "best selling books" <-- is only passed to link_to_child
            }
        }

Because this approach expands the use of an existing feature (link_to_child_specified), I prefer it, unless I am misunderstanding something. :)

On the topic of exclude_scopes. in your opinion, is this needed if we use my proposal (or is it a choice between the two approaches).

[Gary.Wang (gary-wzl77)]

I think your approach does work for the case with some limitation as following
1.scopes: [amazon, ebay, open_library, douban, viva] ----> all of them support "books" keyword
2.all of them are supposed to link to child scopes.

limitation:
1.amazon gives us items which completely unrelated to "books" keyword since developer didn't deal with "books" keywords in source code.
2.english query string (default_query_string)doesn't work for douban and viva scopes, since these two scope only support Chinese querying.

Could you please write your json code based on my requirement as above ?

Thanks.

[Kyle Nitzsche (knitzsche)]

Hi Gary,

I did not understand that you want *every* keyword child scope to have a link to child.

So, I think we have discussed this enough. :)

You understand my preference to reuse and expand existing features and json keys where possible.

But if that does not meet the particular requirements, then please go ahead and complete the implementation as you have described, including the exclude_scopes key and I will approve after normal review.

cheers,
Kyle

[lp:~gary-wzl77/scope-aggregator/fix_1490828]

150. By Gary.Wang on 2015-09-14

1.Add exclude_list key for keywords json object to make it possible to not
2.Merge from trunk

[Gary.Wang (gary-wzl77)]

Basically, at least for all the scope aggregator I implement "link to child" for every scope is required.
I've submitted latest MR.
Please review.

[Kyle Nitzsche (knitzsche)]

Looks good, thanks.

review: Approve

=== modified file 'README.md'
--- README.md	2015-07-15 03:34:23 +0000
+++ README.md	2015-09-14 03:04:52 +0000
@@ -759,3 +759,30 @@
                     }
                 }
            ]
+
+
+#### `exclude_scopes` key (optional)
+
+Value:  json list of objects.
+
+Suppose you want some scopes which supports one certain keywords not to display results, you can specify scope id in the exclude_scopes and scope aggregator will not displays results of specified scopes. 
+
+Note:This key is only supposed to be declared in a `keywords` json object, which doesn't not use a shared category.
+e.g. amazon supports 'books' keyword, but in most cases it returns something unrelated to 'books' keyword. we can get rid of its results in the way as following.
+     "order":
+      [
+        {
+            "keyword":
+            {
+                "keyword": "books",
+                "link_to_child" : "true",
+                "exclude_scopes" :[
+                    {
+                        "id": "com.canonical.scopes.amazon"
+                    }
+                ]
+            }
+        }
+     ]
+
+

=== modified file 'include/query.h'
--- include/query.h	2015-08-04 19:12:04 +0000
+++ include/query.h	2015-09-14 03:04:52 +0000
@@ -153,6 +153,8 @@
         int cardinality = -1;
         std::string dept_id;
         std::string dept_title_msgid;
+        bool link_to_child = false;
+        std::vector<std::string> exclude_scopes; // id of exclude scopes
     };
     std::vector<std::shared_ptr<keyword>> keywords;// pulled from child_sccopes.json
     std::map<std::string, std::shared_ptr<keyword>> id_keyword_map;//keyword id (the keyword string) to keyword instance

=== modified file 'src/query.cpp'
--- src/query.cpp	2015-09-04 20:12:11 +0000
+++ src/query.cpp	2015-09-14 03:04:52 +0000
@@ -289,27 +289,27 @@
         us::CannedQuery canned_query = query();
         std::string dept_id = canned_query.department_id();
 
+        // skip to next if scope is not registered in the system
+        auto reg_scopes = registry_->list();
+        us::MetadataMap::const_iterator scope_it;
+        scope_it = reg_scopes.find(localId_id_m[local_id]);
+        if (scope_it == reg_scopes.end()) {
+            qWarning () << QString("%1: scope is NOT REGISTERED, skipping: %2").arg(qstr(SCOPE_ID), qstr(localId_id_m[local_id]));
+            continue;
+        }
+
         //TODO: add to docs note that when declaring a scope more than once, all must be in different depts
         if (using_departments)
         {
             if (dept_id == "") //is root dept
             {
                 if(scope->department() != dept_id_of_root)
-                    continue;
+                   continue;
             }
             else if(scope->department() != query().department_id())
                 continue;
         }
-
-        // skip to next if scope is not registered in the system
-        auto reg_scopes = registry_->list();
-        us::MetadataMap::const_iterator scope_it;
-        scope_it = reg_scopes.find(localId_id_m[local_id]);
-        if (scope_it == reg_scopes.end()) {
-            qWarning () << QString("%1: scope is NOT REGISTERED, skipping: %2").arg(qstr(SCOPE_ID), qstr(localId_id_m[local_id]));
-            continue;
-        }
-
+        
         // if scope is not in the currently enabled list, skip it
         bool found_as_enabled = false;
         for (auto cs : current_child_scopes)//FQ ids
@@ -322,12 +322,33 @@
                 }
             }
         }
+
         if (!found_as_enabled)
         {
             qWarning () << QString("%1: scope is NOT ENABLED, skipping: %2").arg(qstr(SCOPE_ID), qstr(localId_id_m[local_id]));
             continue;
         }
 
+        // if scope is in exclude scopes list, skip it
+        bool found_as_exclude = false;
+        for (auto kw : keywords)
+        {
+            if (kw->id == scope->keyword()) {
+                auto iter = std::find(kw->exclude_scopes.begin(), kw->exclude_scopes.end(), scope->id());
+                if (iter != kw->exclude_scopes.end())
+                {
+                    found_as_exclude = true;
+                    qWarning () << QString("==== FOUND AS EXCLUDE:  %1: scope is in exclude scopes of keyword: %2")
+                                   .arg(qstr(scope->id()), qstr(scope->keyword()));
+                }
+            }
+        }
+
+        if (found_as_exclude) {
+            continue;
+        }
+
+
         if (query_string.empty() && scope->only_in_search() == true)
         {
             continue;
@@ -612,13 +633,33 @@
                         }
                         if (!upstream_reply->lookup_category(cat_id))
                         {
-                            scope->set_category(upstream_reply->register_category
-                            (
-                                cat_id,
-                                cat_title,
-                                 "",
-                                us::CategoryRenderer(rdr)
-                            ));
+                           if (scope->category_link_to_child()) 
+                           {
+                               scope->set_category
+                               (
+                                   upstream_reply->register_category
+                                   (
+                                    cat_id,
+                                    cat_title,
+                                    "",
+                                    us::CannedQuery(scope->id(), query_string, scope->child_department()),
+                                    us::CategoryRenderer(rdr)
+                                   ) 
+                               );
+                           }
+                           else
+                           {
+                               scope->set_category
+                               (
+                                   upstream_reply->register_category
+                                   (
+                                    cat_id,
+                                    cat_title,
+                                    "",
+                                    us::CategoryRenderer(rdr)
+                                   )
+                               );
+                           }
                         }
                     }
                     res.set_category(scope->category());
@@ -1049,39 +1090,36 @@
                 {
                     if (!rdr.empty()) // should not ever be empty!
                     {
-                        if (scope->category_link_to_child())
-                        {
-                            if (!upstream_reply->lookup_category(cat_id))
-                            {
-                                scope->set_category
-                                (
-                                    upstream_reply->register_category
-                                    (
-                                        cat_id,
-                                        cat_title,
-                                         "",
-                                        us::CannedQuery(scope->id(), query_string, scope->child_department()),
-                                        us::CategoryRenderer(rdr)
-                                    )
-                                );
-                            }
-                        }
-                        else
-                        {
-                            if (!upstream_reply->lookup_category(cat_id))
-                            {
-                                scope->set_category
-                                (
-                                    upstream_reply->register_category
-                                    (
-                                        cat_id,
-                                        cat_title,
-                                         "",
-                                        us::CategoryRenderer(rdr)
-                                    )
-                                );
-                            }
-                        }
+                        if (!upstream_reply->lookup_category(cat_id))
+                        {
+                            if (scope->category_link_to_child())
+                            {
+                                scope->set_category
+                                (
+                                   upstream_reply->register_category
+                                   (
+                                    cat_id,
+                                    cat_title,
+                                    "",
+                                    us::CannedQuery(scope->id(), query_string, scope->child_department()),
+                                    us::CategoryRenderer(rdr)
+                                   )
+                                );
+                            } 
+                            else
+                            {
+                               scope->set_category
+                               (
+                                   upstream_reply->register_category
+                                   (
+                                    cat_id,
+                                    cat_title,
+                                    "",
+                                    us::CategoryRenderer(rdr)
+                                   )
+                               );
+                           }
+                       }
                     }
                     else
                         qWarning() << QString("Warning: Scope %1 renderer is empty.").arg(qstr(scope->id()));

=== modified file 'src/utils.cpp'
--- src/utils.cpp	2015-08-04 19:12:04 +0000
+++ src/utils.cpp	2015-09-14 03:04:52 +0000
@@ -648,6 +648,23 @@
                 QJsonDocument rdr_d(renderer_o);
                 keyword_catname_search_renderer[category] = sstr(rdr_d.toJson());
             }
+            if (keyword_o.contains("link_to_child"))
+            {
+                if (keyword_o["link_to_child"].toString() == "true")
+                    keyword_.link_to_child = true;
+            }
+            if (keyword_o.contains("exclude_scopes"))
+            {
+                  QJsonArray scope_a = keyword_o["exclude_scopes"].toArray();
+  
+                  for (const auto item : scope_a)
+                  {
+                      QJsonObject scope_o = item.toObject();
+                      std::string id = scope_o["id"].toString().toStdString();
+                      keyword_.exclude_scopes.emplace_back(id);
+                  }
+            }
+            
             std::shared_ptr<keyword> kw_ptr = std::make_shared<keyword>(keyword_);
             keywords.emplace_back(kw_ptr);
             id_keyword_map[keyword_id]  = kw_ptr;
@@ -812,7 +829,6 @@
         AggChildScope keych(keyword_child.id);
         QString time_str = QDateTime::currentDateTimeUtc().toString();
         keych.set_local_id(keyword_child.id + time_str.toStdString());
-        keych.set_category_link_to_child(true);
         keych.set_keyword_scope(true);
         dept d;
         d.id = keyword_child.id;
@@ -837,6 +853,7 @@
             d.keyword = kw;
             keych.set_department(kw);
             keych.set_keyword(kw);
+            keych.set_category_link_to_child(id_keyword_map[kw]->link_to_child);
             qWarning() << "==== ID KW 1: " << qstr(keych.keyword());
 
             bool a_dup = false;
@@ -844,6 +861,12 @@
             if (using_departments)
             {
                 std::string curr_dept_id = query().department_id();
+
+                //make sure we load correct dept id if we specify department property in keyword object
+                if (!curr_dept_id.empty() && keyword_deptId[kw] != curr_dept_id) {
+                    continue;
+                }
+
                 for (std::shared_ptr<child_scope> cs : child_scopes)
                 {
                     if (keyword_child.id != cs->id)