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

Proposed by Kyle Nitzsche
Status: Merged
Approved by: Kyle Nitzsche
Approved revision: 170
Merge reported by: Kyle Nitzsche
Merged at revision: not available
Proposed branch: lp://staging/~knitzsche/scope-aggregator/fixes-fallback-bug-1599948
Merge into: lp://staging/scope-aggregator
Diff against target: 109 lines (+21/-14)
3 files modified
CMakeLists.txt (+1/-1)
src/query.cpp (+19/-13)
src/utils.cpp (+1/-0)
To merge this branch: bzr merge lp://staging/~knitzsche/scope-aggregator/fixes-fallback-bug-1599948
Reviewer Review Type Date Requested Status
Zhang Enwei (community) Approve
Penk Chen Pending
Gary.Wang Pending
Review via email: mp+299481@code.staging.launchpad.net

Description of the change

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

To post a comment you must log in.
Revision history for this message
Kyle Nitzsche (knitzsche) wrote :

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

Revision history for this message
Zhang Enwei (zhangew401) wrote :

Look good to me

review: Approve
170. By Kyle Nitzsche

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".

Revision history for this message
Kyle Nitzsche (knitzsche) wrote :

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.

Revision history for this message
Zhang Enwei (zhangew401) wrote :

LGTM

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches