Merge lp:~paulliu/unity-scope-click/showratings into lp:unity-scope-click

[Ying-Chun Liu]

Provide reviews/ratings from server in previews.

Provide reviews/ratings from server in previews.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:90
http://jenkins.qa.ubuntu.com/job/unity-scope-click-ci/118/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scope-click-trusty-amd64-ci/16
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scope-click-trusty-armhf-ci/16

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

review: Approve (continuous-integration)

[Manuel de la Peña (mandel)]

Can you explain what is the following line doing:

var reviews = yield rnrClient.get_reviews("","any","any","any","any",details.title,"1","helpful");

Is "any" some kind of keyword??? Should we use a constant?

We have the following var names:

node1, node2, gvar1

Can we use names that add some information to the reviewer, I have no context of what the data should be and such names do not help. Maybe something like rootNode, ratingNode etc...?

Should we log something in this exception:

182 + try {
183 + gvar1 = Json.gvariant_deserialize(node1.get_member(node2),null);
184 + } catch (GLib.Error e) {
185 + continue;
186 + }

It might be a good thing to do when debugging and I do not trust the server side not changing things or always working ;)

Why do you use in some cases the var keyword and in others you do not? For example:

143 + Rest.Proxy proxy = new Rest.Proxy(REVIEWS_URL, false);
144 + var call = proxy.new_call();

Could not have used bar in both cases?

In the following code:

257 + try {
258 + parser.load_from_data(testData1);
259 + } catch (GLib.Error e) {
260 + }

It might be a good idea to deal with the exception in case some other developer changes the testData wrongly and has bad results in the tests, what do you think???

All the rest of the code looks great, thx!!!

review: Needs Fixing

[dobey (dobey)]

48 + --pkg rest-0.7 \

I thought we agreed to not use librest, but just use soup for everything?

Also, the test doesn't seem to actually test anything, as I read it. It's fake data in a JSON dict, and you parse it as such. We don't need to write unit tests for json-glib in our code. It's also a bit weird to define a test method inside a fake class, and simply have the actual test method call that method, doing the assertion inside the fake class. Fake classes should really only be used to override methods which require network or dbus access to other daemons, to prevent such access and isolate the tests. Fake data should probably be placed in external files for use in the tests, or at the very least, encoded in the fake-data.vala source file. And the actual test and assertion should happen within the test function itself, not inside a set of methods in another class.

review: Needs Fixing

[dobey (dobey)]

118 + const string REVIEWS_URL = "http://reviews.ubuntu.com/reviews/api/1.0";

Also, this needs to be HTTPS, and should be split up such that the base URL can be changed via environment variable, for testing against staging. See click-webservice.vala for how we do this for the SSO base URL.

[Ying-Chun Liu (paulliu)]

I'll rewrite it to use libsoup because I really cannot make the SSO work with librest.

But maybe we want to use librest for the future at some time as it makes the code cleaner and easier to be maintained?

[Ying-Chun Liu (paulliu)]

the reviews retriviing doesn't need the SSO. So I'll keep the http here.
The submit reviews function will be implemented in another branch which will use https.

[dobey (dobey)]

77 + const string REVIEWS_BASE_URL = "http://reviews.ubuntu.com/reviews/api/1.0";

86 + string from_environ (string env_name, string default_value) {
87 + string env_value = Environment.get_variable (env_name);
88 + return env_value != null ? env_value : default_value;
89 + }
90 +
91 + string get_base_url () {
92 + return from_environ ("U1_REVIEWS_BASE_URL", REVIEWS_BASE_URL);
93 + }

This is still wrong. Why are you not using HTTPS here? Also, the _BASE_URL should only be "https://reviews.ubuntu.com/reviews" with "/api/1.0" added after. One shouldn't include the "/api/1.0" portion in the environment variable. If someone passes /api/2.0 for example, the app wouldn't handle it.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:91
http://jenkins.qa.ubuntu.com/job/unity-scope-click-ci/121/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scope-click-trusty-amd64-ci/19
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scope-click-trusty-armhf-ci/19

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

review: Approve (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:93
http://jenkins.qa.ubuntu.com/job/unity-scope-click-ci/122/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scope-click-trusty-amd64-ci/20
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scope-click-trusty-armhf-ci/20

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

review: Approve (continuous-integration)

[dobey (dobey)]

130 + public async Variant? get_reviews(string packagename, string? language,
131 + string? origin, string? distroseries,
132 + string? version, string appname,
133 + string page, string sort) {

I wonder if we can abstract all these arguments to a new object, or just pass in the AppDetails object here, and get the language internally rather than taking an argument for it. It's better to have an object containing a bunch of args, than to pass many options individually like this.

[lp:~paulliu/unity-scope-click/showratings]

90. By dobey on 2013-12-11

Don't reimplement generated rules for vala.
Take advantage of VALA_CHECK_MODULES to find vala packages.
Split non-compatible mdoules to another PKG_CHECK_MODULES.
Build majority of the code as a noinst lib, which click-scope and tests link to.
Update the .bzrignore for an autools project.
. Fixes: https://bugs.launchpad.net/bugs/1258559.

Approved by Roberto Alsina, Mike McCracken, PS Jenkins bot.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:94
http://jenkins.qa.ubuntu.com/job/unity-scope-click-ci/124/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scope-click-trusty-amd64-ci/22/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scope-click-trusty-armhf-ci/22/console

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

review: Needs Fixing (continuous-integration)

[lp:~paulliu/unity-scope-click/showratings]

91. By Ying-Chun Liu on 2013-12-12

get reviews & ratings from server.

92. By Ying-Chun Liu on 2013-12-13

remvoe server-status as we don't need to use it.

93. By Ying-Chun Liu on 2013-12-16

Add comments.

94. By Ying-Chun Liu on 2013-12-19

Change origin to "click".

95. By Ying-Chun Liu on 2013-12-19

Merge upstream.

96. By Ying-Chun Liu on 2014-01-02

fix indent

97. By Ying-Chun Liu on 2014-01-06

Add some unit tests.
Partly fix the issues in comment 465611

98. By Ying-Chun Liu on 2014-01-07

Add version/framework

99. By Ying-Chun Liu on 2014-01-07

merge trunk

100. By Ying-Chun Liu on 2014-01-07

fix indent.

101. By Ying-Chun Liu on 2014-01-07

fix comment for ReviewFilter.

102. By Ying-Chun Liu on 2014-01-08

fix indent.
Add test for from_environ
fix framework.

103. By Ying-Chun Liu on 2014-01-08

Add 2014 to copyright.

104. By Ying-Chun Liu on 2014-01-09

revert part of the previous commit - change back framework as string list.
Add a TODO to indicate that we should use system's framework.

105. By Ying-Chun Liu on 2014-01-09

use "click" as framework.

Unmerged revisions

=== modified file 'src/Makefile.am'
--- src/Makefile.am	2013-12-11 16:46:03 +0000
+++ src/Makefile.am	2013-12-12 14:18:57 +0000
@@ -43,7 +43,8 @@
 	click-interface.vala \
 	ubuntuone-credentials.vala \
 	non-click-scope.vala \
-	utils.vala
+	utils.vala \
+	rnrclient.vala
 
 click_scope_VALAFLAGS = \
 	--thread \
 
=== modified file 'src/click-scope.vala'
--- src/click-scope.vala	2013-12-10 22:21:24 +0000
+++ src/click-scope.vala	2013-12-12 14:18:57 +0000
@@ -80,6 +80,7 @@
   const string LABEL_COMMENTS = "Comments";
 
   ClickInterface click_if = new ClickInterface ();
+  RNRClient rnrClient = new RNRClient();
 
   public ClickScope ()
   {
@@ -118,7 +119,12 @@
         preview.add_info(new Unity.InfoHint.with_variant(HINT_PUBLISHER, "Publisher", null, new Variant.string(details.publisher)));
         preview.add_info(new Unity.InfoHint.with_variant(HINT_SCREENSHOTS, LABEL_SCREENSHOTS, null, new Variant.strv(details.more_screenshot_urls)));
         preview.add_info(new Unity.InfoHint.with_variant(HINT_KEYWORDS, LABEL_KEYWORDS, null, new Variant.strv(details.keywords)));
-        // TODO: get the proper ratings and reviews from the rnr web service
+        // get the proper ratings and reviews from the rnr web service
+        var reviews = yield rnrClient.get_reviews(details);
+        if (reviews != null) {
+          preview.add_info(new Unity.InfoHint.with_variant(HINT_REVIEWS, LABEL_REVIEWS, null, reviews));
+          debug("Add reviews "+reviews.print(true));
+        }
         return preview;
     } catch (WebserviceError e) {
         debug ("Error calling webservice: %s", e.message);
 
=== added file 'src/rnrclient.vala'
--- src/rnrclient.vala	1970-01-01 00:00:00 +0000
+++ src/rnrclient.vala	2013-12-12 14:18:57 +0000
@@ -0,0 +1,177 @@
+/* -*- tab-width: 4; c-basic-offset: 4; indent-tabs-mode: nil; -*- */
+/*
+ * Copyright (C) 2013 Canonical, Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 3.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+using Json;
+
+class RNRClient : GLib.Object
+{
+    UbuntuoneCredentials u1creds;
+    const string CONSUMER_KEY = "consumer_key";
+    const string CONSUMER_SECRET = "consumer_secret";
+    const string TOKEN = "token";
+    const string TOKEN_SECRET = "token_secret";
+    const string REVIEWS_BASE_URL = "https://reviews.ubuntu.com/reviews";
+
+    internal Soup.SessionAsync http_session;
+
+    construct {
+        http_session = WebClient.get_webclient();
+        u1creds = new UbuntuoneCredentials ();
+    }
+
+    string from_environ (string env_name, string default_value) {
+        string env_value = Environment.get_variable (env_name);
+        return env_value != null ? env_value : default_value;
+    }
+
+    string get_base_url () {
+        return from_environ ("U1_REVIEWS_BASE_URL", REVIEWS_BASE_URL);
+    }
+
+    string get_status_url() {
+        return get_base_url() + "/api/1.0/server-status";
+    }
+
+    string get_review_url() {
+        return get_base_url() + "/api/1.0/reviews/filter/%s/%s/%s/%s/%s%s/page/%s/%s/";
+    }
+
+    public async string server_status() {
+        WebserviceError failure = null;
+        string url = get_status_url();
+        string response="";
+
+        var message = new Soup.Message ("GET", url);
+        http_session.queue_message(message, (http_session, message) => {
+            if (message.status_code != Soup.KnownStatusCode.OK) {
+                var msg = "Web request failed: HTTP %u %s".printf(
+                       message.status_code, message.reason_phrase);
+                failure = new WebserviceError.HTTP_ERROR(msg);
+            } else {
+                message.response_body.flatten ();
+                response = (string) message.response_body.data;
+                debug ("response is %s", response);
+            }
+            server_status.callback();
+        });
+        yield;
+        if (failure != null) {
+            return "no";
+        }
+        return response;
+    }
+
+    public async Variant? get_reviews(AppDetails details) {
+        string[] languages = GLib.Intl.get_language_names();
+        string? language = null;
+        if (languages.length > 0) {
+            language = languages[0];
+        }
+        var filter = new ReviewFilter();
+        filter.language = language;
+        filter.appname = details.title;
+        return yield get_reviews_core(filter);
+    }
+
+    public async Variant? get_reviews_core(ReviewFilter filter) {
+        Variant? ret = null;
+        WebserviceError failure = null;
+        string url = get_review_url().printf(filter.language,
+                                             filter.origin,
+                                             filter.distroseries,
+                                             filter.version,
+                                             filter.packagename,
+                                             filter.appname,
+                                             filter.page,
+                                             filter.sort);
+        string response="";
+
+        debug ("url is %s", url);
+
+        var message = new Soup.Message ("GET", url);
+        http_session.queue_message(message, (http_session, message) => {
+            if (message.status_code != Soup.KnownStatusCode.OK) {
+                var msg = "Web request failed: HTTP %u %s".printf(
+                       message.status_code, message.reason_phrase);
+                failure = new WebserviceError.HTTP_ERROR(msg);
+            } else {
+                message.response_body.flatten ();
+                response = (string) message.response_body.data;
+                debug ("response is %s", response);
+            }
+            get_reviews_core.callback();
+        });
+        yield;
+        if (failure != null) {
+            debug("cannot get reviews error %s", failure.message);
+            return ret;
+        }
+        var parser = new Json.Parser();
+        try {
+            parser.load_from_data(response);
+        } catch (GLib.Error e) {
+            debug ("parse reviews response error %s", e.message);
+            return ret;
+        }
+
+        ret = convertJSONtoVariant(parser);
+        return ret;
+    }
+
+    protected Variant? convertJSONtoVariant(Json.Parser parser) {
+        Variant? ret = null;
+        var root_object = parser.get_root();
+        var builder = new VariantBuilder(new VariantType("av"));
+
+        foreach (var node in root_object.get_array().get_elements()) {
+            var ht = new VariantBuilder(new VariantType("a{sv}"));
+            var node_object = node.get_object();
+            foreach (var node_member in node_object.get_members()) {
+                Variant? gvar_member = null;
+                try {
+                    gvar_member = Json.gvariant_deserialize(node_object.get_member(node_member), null);
+                } catch (GLib.Error e) {
+                    debug ("gvariant deserialize error: %s",e.message);
+                    continue;
+                }
+                if (gvar_member == null) {
+                    continue;
+                }
+                if (gvar_member.is_of_type(VariantType.MAYBE) && gvar_member.get_maybe() == null) {
+                    continue;
+                }
+                ht.add("{sv}", node_member, gvar_member);
+            }
+            Variant dict = ht.end();
+            builder.add("v", dict);
+        }
+
+        ret = builder.end();
+        return ret;
+    }
+}
+
+class ReviewFilter : GLib.Object {
+    public string packagename = "";
+    public string language = "any";
+    public string origin = "any";
+    public string distroseries = "any";
+    public string version = "any";
+    public string appname = "";
+    public string page = "1";
+    public string sort = "helpful";
+}