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

Proposed by Ying-Chun Liu
Status: Superseded
Proposed branch: lp:~paulliu/unity-scope-click/showratings
Merge into: lp:unity-scope-click
Diff against target: 221 lines (+186/-2)
3 files modified
src/Makefile.am (+2/-1)
src/click-scope.vala (+7/-1)
src/rnrclient.vala (+177/-0)
To merge this branch: bzr merge lp:~paulliu/unity-scope-click/showratings
Reviewer Review Type Date Requested Status
PS Jenkins bot continuous-integration Needs Fixing
dobey (community) Needs Fixing
Manuel de la Peña (community) Needs Fixing
Review via email: mp+198349@code.launchpad.net

This proposal has been superseded by a proposal from 2013-12-12.

Commit message

Provide reviews/ratings from server in previews.

Description of the change

Provide reviews/ratings from server in previews.

To post a comment you must log in.
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Manuel de la Peña (mandel) wrote :

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) wrote :

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) wrote :

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) wrote :

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) wrote :

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) wrote :

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) wrote :
review: Approve (continuous-integration)
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
dobey (dobey) wrote :

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.

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.

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/Makefile.am'
2--- src/Makefile.am 2013-12-11 16:46:03 +0000
3+++ src/Makefile.am 2013-12-12 14:18:57 +0000
4@@ -43,7 +43,8 @@
5 click-interface.vala \
6 ubuntuone-credentials.vala \
7 non-click-scope.vala \
8- utils.vala
9+ utils.vala \
10+ rnrclient.vala
11
12 click_scope_VALAFLAGS = \
13 --thread \
14
15=== modified file 'src/click-scope.vala'
16--- src/click-scope.vala 2013-12-10 22:21:24 +0000
17+++ src/click-scope.vala 2013-12-12 14:18:57 +0000
18@@ -80,6 +80,7 @@
19 const string LABEL_COMMENTS = "Comments";
20
21 ClickInterface click_if = new ClickInterface ();
22+ RNRClient rnrClient = new RNRClient();
23
24 public ClickScope ()
25 {
26@@ -118,7 +119,12 @@
27 preview.add_info(new Unity.InfoHint.with_variant(HINT_PUBLISHER, "Publisher", null, new Variant.string(details.publisher)));
28 preview.add_info(new Unity.InfoHint.with_variant(HINT_SCREENSHOTS, LABEL_SCREENSHOTS, null, new Variant.strv(details.more_screenshot_urls)));
29 preview.add_info(new Unity.InfoHint.with_variant(HINT_KEYWORDS, LABEL_KEYWORDS, null, new Variant.strv(details.keywords)));
30- // TODO: get the proper ratings and reviews from the rnr web service
31+ // get the proper ratings and reviews from the rnr web service
32+ var reviews = yield rnrClient.get_reviews(details);
33+ if (reviews != null) {
34+ preview.add_info(new Unity.InfoHint.with_variant(HINT_REVIEWS, LABEL_REVIEWS, null, reviews));
35+ debug("Add reviews "+reviews.print(true));
36+ }
37 return preview;
38 } catch (WebserviceError e) {
39 debug ("Error calling webservice: %s", e.message);
40
41=== added file 'src/rnrclient.vala'
42--- src/rnrclient.vala 1970-01-01 00:00:00 +0000
43+++ src/rnrclient.vala 2013-12-12 14:18:57 +0000
44@@ -0,0 +1,177 @@
45+/* -*- tab-width: 4; c-basic-offset: 4; indent-tabs-mode: nil; -*- */
46+/*
47+ * Copyright (C) 2013 Canonical, Ltd.
48+ *
49+ * This program is free software; you can redistribute it and/or modify
50+ * it under the terms of the GNU General Public License as published by
51+ * the Free Software Foundation; version 3.
52+ *
53+ * This program is distributed in the hope that it will be useful,
54+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
55+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
56+ * GNU General Public License for more details.
57+ *
58+ * You should have received a copy of the GNU General Public License
59+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
60+ */
61+
62+using Json;
63+
64+class RNRClient : GLib.Object
65+{
66+ UbuntuoneCredentials u1creds;
67+ const string CONSUMER_KEY = "consumer_key";
68+ const string CONSUMER_SECRET = "consumer_secret";
69+ const string TOKEN = "token";
70+ const string TOKEN_SECRET = "token_secret";
71+ const string REVIEWS_BASE_URL = "https://reviews.ubuntu.com/reviews";
72+
73+ internal Soup.SessionAsync http_session;
74+
75+ construct {
76+ http_session = WebClient.get_webclient();
77+ u1creds = new UbuntuoneCredentials ();
78+ }
79+
80+ string from_environ (string env_name, string default_value) {
81+ string env_value = Environment.get_variable (env_name);
82+ return env_value != null ? env_value : default_value;
83+ }
84+
85+ string get_base_url () {
86+ return from_environ ("U1_REVIEWS_BASE_URL", REVIEWS_BASE_URL);
87+ }
88+
89+ string get_status_url() {
90+ return get_base_url() + "/api/1.0/server-status";
91+ }
92+
93+ string get_review_url() {
94+ return get_base_url() + "/api/1.0/reviews/filter/%s/%s/%s/%s/%s%s/page/%s/%s/";
95+ }
96+
97+ public async string server_status() {
98+ WebserviceError failure = null;
99+ string url = get_status_url();
100+ string response="";
101+
102+ var message = new Soup.Message ("GET", url);
103+ http_session.queue_message(message, (http_session, message) => {
104+ if (message.status_code != Soup.KnownStatusCode.OK) {
105+ var msg = "Web request failed: HTTP %u %s".printf(
106+ message.status_code, message.reason_phrase);
107+ failure = new WebserviceError.HTTP_ERROR(msg);
108+ } else {
109+ message.response_body.flatten ();
110+ response = (string) message.response_body.data;
111+ debug ("response is %s", response);
112+ }
113+ server_status.callback();
114+ });
115+ yield;
116+ if (failure != null) {
117+ return "no";
118+ }
119+ return response;
120+ }
121+
122+ public async Variant? get_reviews(AppDetails details) {
123+ string[] languages = GLib.Intl.get_language_names();
124+ string? language = null;
125+ if (languages.length > 0) {
126+ language = languages[0];
127+ }
128+ var filter = new ReviewFilter();
129+ filter.language = language;
130+ filter.appname = details.title;
131+ return yield get_reviews_core(filter);
132+ }
133+
134+ public async Variant? get_reviews_core(ReviewFilter filter) {
135+ Variant? ret = null;
136+ WebserviceError failure = null;
137+ string url = get_review_url().printf(filter.language,
138+ filter.origin,
139+ filter.distroseries,
140+ filter.version,
141+ filter.packagename,
142+ filter.appname,
143+ filter.page,
144+ filter.sort);
145+ string response="";
146+
147+ debug ("url is %s", url);
148+
149+ var message = new Soup.Message ("GET", url);
150+ http_session.queue_message(message, (http_session, message) => {
151+ if (message.status_code != Soup.KnownStatusCode.OK) {
152+ var msg = "Web request failed: HTTP %u %s".printf(
153+ message.status_code, message.reason_phrase);
154+ failure = new WebserviceError.HTTP_ERROR(msg);
155+ } else {
156+ message.response_body.flatten ();
157+ response = (string) message.response_body.data;
158+ debug ("response is %s", response);
159+ }
160+ get_reviews_core.callback();
161+ });
162+ yield;
163+ if (failure != null) {
164+ debug("cannot get reviews error %s", failure.message);
165+ return ret;
166+ }
167+ var parser = new Json.Parser();
168+ try {
169+ parser.load_from_data(response);
170+ } catch (GLib.Error e) {
171+ debug ("parse reviews response error %s", e.message);
172+ return ret;
173+ }
174+
175+ ret = convertJSONtoVariant(parser);
176+ return ret;
177+ }
178+
179+ protected Variant? convertJSONtoVariant(Json.Parser parser) {
180+ Variant? ret = null;
181+ var root_object = parser.get_root();
182+ var builder = new VariantBuilder(new VariantType("av"));
183+
184+ foreach (var node in root_object.get_array().get_elements()) {
185+ var ht = new VariantBuilder(new VariantType("a{sv}"));
186+ var node_object = node.get_object();
187+ foreach (var node_member in node_object.get_members()) {
188+ Variant? gvar_member = null;
189+ try {
190+ gvar_member = Json.gvariant_deserialize(node_object.get_member(node_member), null);
191+ } catch (GLib.Error e) {
192+ debug ("gvariant deserialize error: %s",e.message);
193+ continue;
194+ }
195+ if (gvar_member == null) {
196+ continue;
197+ }
198+ if (gvar_member.is_of_type(VariantType.MAYBE) && gvar_member.get_maybe() == null) {
199+ continue;
200+ }
201+ ht.add("{sv}", node_member, gvar_member);
202+ }
203+ Variant dict = ht.end();
204+ builder.add("v", dict);
205+ }
206+
207+ ret = builder.end();
208+ return ret;
209+ }
210+}
211+
212+class ReviewFilter : GLib.Object {
213+ public string packagename = "";
214+ public string language = "any";
215+ public string origin = "any";
216+ public string distroseries = "any";
217+ public string version = "any";
218+ public string appname = "";
219+ public string page = "1";
220+ public string sort = "helpful";
221+}

Subscribers

People subscribed via source and target branches

to all changes: