Merge lp:~stolowski/unity-scope-click/edit-reviews-15-04 into lp:unity-scope-click
- edit-reviews-15-04
- Merge into trunk
Proposed by
Paweł Stołowski
Status: | Superseded | ||||
---|---|---|---|---|---|
Proposed branch: | lp:~stolowski/unity-scope-click/edit-reviews-15-04 | ||||
Merge into: | lp:unity-scope-click | ||||
Diff against target: |
374 lines (+187/-20) 8 files modified
CMakeLists.txt (+1/-1) debian/control (+1/-1) libclickscope/click/preview.cpp (+82/-15) libclickscope/click/preview.h (+1/-0) libclickscope/click/reviews.cpp (+52/-0) libclickscope/click/reviews.h (+5/-2) libclickscope/tests/test_reviews.cpp (+43/-0) scope/clickapps/apps-scope.cpp (+2/-1) |
||||
To merge this branch: | bzr merge lp:~stolowski/unity-scope-click/edit-reviews-15-04 | ||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
PS Jenkins bot | continuous-integration | Needs Fixing | |
Unity Team | Pending | ||
Review via email: mp+258788@code.launchpad.net |
This proposal has been superseded by a proposal from 2015-05-12.
Commit message
Support for review editing.
Description of the change
Support for review editing.
To post a comment you must log in.
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote : | # |
review:
Needs Fixing
(continuous-integration)
- 321. By Paweł Stołowski
-
Renamed sort to bring_to_front and added unit tests for it
Unmerged revisions
Preview Diff
[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1 | === modified file 'CMakeLists.txt' |
2 | --- CMakeLists.txt 2015-05-07 16:11:57 +0000 |
3 | +++ CMakeLists.txt 2015-05-11 15:29:02 +0000 |
4 | @@ -27,7 +27,7 @@ |
5 | pkg_check_modules(UNITY_SCOPES REQUIRED libunity-scopes>=0.6.7 libunity-api>=0.1.3) |
6 | add_definitions(${UNITY_SCOPES_CFLAGS} ${UNITY_SCOPES_CFLAGS_OTHER}) |
7 | |
8 | -pkg_check_modules(UBUNTUONE REQUIRED ubuntuoneauth-2.0) |
9 | +pkg_check_modules(UBUNTUONE REQUIRED ubuntuoneauth-2.0>=15.04) |
10 | add_definitions(${UBUNTUONE_CFLAGS} ${UBUNTUONE_CFLAGS_OTHER}) |
11 | |
12 | pkg_check_modules(UBUNTU_DOWNLOAD_MANAGER_CLIENT REQUIRED ubuntu-download-manager-client) |
13 | |
14 | === modified file 'debian/control' |
15 | --- debian/control 2015-05-07 16:11:57 +0000 |
16 | +++ debian/control 2015-05-11 15:29:02 +0000 |
17 | @@ -18,7 +18,7 @@ |
18 | libpay2-dev, |
19 | libubuntu-download-manager-client-dev (>= 0.3+14.10.20140430-0ubuntu1), |
20 | libubuntu-download-manager-common-dev (>= 0.3+14.10.20140430-0ubuntu1), |
21 | - libubuntuoneauth-2.0-dev, |
22 | + libubuntuoneauth-2.0-dev (>= 15.04), |
23 | libunity-api-dev (>= 7.80.7), |
24 | libunity-scopes-dev (>= 0.6.7~), |
25 | libgsettings-qt-dev, |
26 | |
27 | === modified file 'libclickscope/click/preview.cpp' |
28 | --- libclickscope/click/preview.cpp 2015-04-10 14:24:10 +0000 |
29 | +++ libclickscope/click/preview.cpp 2015-05-11 15:29:02 +0000 |
30 | @@ -36,6 +36,7 @@ |
31 | #include <click/dbus_constants.h> |
32 | #include <click/departments-db.h> |
33 | #include <click/utils.h> |
34 | +#include <click/smartconnect.h> |
35 | |
36 | #include <boost/algorithm/string/replace.hpp> |
37 | |
38 | @@ -644,21 +645,54 @@ |
39 | { |
40 | } |
41 | |
42 | +std::string InstalledPreview::get_consumer_key() |
43 | +{ |
44 | + std::promise<std::string> promise; |
45 | + auto future = promise.get_future(); |
46 | + QSharedPointer<click::CredentialsService> sso; |
47 | + |
48 | + qt::core::world::enter_with_task([this, &sso, &promise]() { |
49 | + sso.reset(new click::CredentialsService()); |
50 | + |
51 | + QObject::connect(sso.data(), &click::CredentialsService::credentialsFound, |
52 | + [&promise, &sso](const UbuntuOne::Token& token) { |
53 | + qDebug() << "Credentials found"; |
54 | + sso.clear(); |
55 | + promise.set_value(token.consumerKey().toStdString()); |
56 | + }); |
57 | + QObject::connect(sso.data(), &click::CredentialsService::credentialsNotFound, |
58 | + [&promise, &sso]() { |
59 | + qDebug() << "No credentials found"; |
60 | + sso.clear(); |
61 | + promise.set_value(""); |
62 | + }); |
63 | + |
64 | + sso->getCredentials(); |
65 | + qDebug() << "getCredentials finished"; |
66 | + }); |
67 | + return future.get(); |
68 | +} |
69 | + |
70 | void InstalledPreview::run(unity::scopes::PreviewReplyProxy const& reply) |
71 | { |
72 | // Check if the user is submitting a rating, so we can submit it. |
73 | Review review; |
74 | review.rating = 0; |
75 | + std::string widget_id; |
76 | // We use a try/catch here, as scope_data() can be a dict, but not have |
77 | // the values we need, which will result in an exception thrown. |
78 | try { |
79 | auto metadict = metadata.scope_data().get_dict(); |
80 | review.rating = metadict["rating"].get_int(); |
81 | review.review_text = metadict["review"].get_string(); |
82 | + widget_id = metadict["widget_id"].get_string(); |
83 | } catch(...) { |
84 | // Do nothing as we are not submitting a review. |
85 | } |
86 | |
87 | + auto userid = get_consumer_key(); |
88 | + |
89 | + // |
90 | // Get the click manifest. |
91 | Manifest manifest; |
92 | std::promise<Manifest> manifest_promise; |
93 | @@ -684,35 +718,68 @@ |
94 | if (review.rating > 0) { |
95 | std::promise<bool> submit_promise; |
96 | std::future<bool> submit_future = submit_promise.get_future(); |
97 | - qt::core::world::enter_with_task([this, review, &submit_promise]() { |
98 | + qt::core::world::enter_with_task([this, review, &submit_promise, widget_id]() mutable { |
99 | QSharedPointer<click::CredentialsService> sso(new click::CredentialsService()); |
100 | client->setCredentialsService(sso); |
101 | - submit_operation = reviews->submit_review(review, |
102 | + if (widget_id == "rating") { |
103 | + submit_operation = reviews->submit_review(review, |
104 | [&submit_promise](click::Reviews::Error){ |
105 | // TODO: Need to handle errors properly. |
106 | submit_promise.set_value(true); |
107 | }); |
108 | - }); |
109 | + |
110 | + } else { |
111 | + try { |
112 | + review.id = std::stoul(widget_id); |
113 | + qDebug() << "Updating review" << review.id << "with '" << QString::fromStdString(review.review_text) << "'"; |
114 | + submit_operation = reviews->edit_review(review, |
115 | + [&submit_promise](click::Reviews::Error){ |
116 | + // TODO: Need to handle errors properly. |
117 | + submit_promise.set_value(true); |
118 | + }); |
119 | + } catch (const std::exception &e) { |
120 | + qWarning() << "Failed to update review:" << QString::fromStdString(e.what()) << " review widget:" << QString::fromStdString(widget_id); |
121 | + submit_promise.set_value(false); |
122 | + } |
123 | + } |
124 | + }); |
125 | submit_future.get(); |
126 | } |
127 | } |
128 | - getApplicationUri(manifest, [this, reply, manifest, app_name, &review](const std::string& uri) { |
129 | - populateDetails([this, reply, uri, manifest, app_name, &review](const PackageDetails &details){ |
130 | + getApplicationUri(manifest, [this, reply, manifest, app_name, &review, userid](const std::string& uri) { |
131 | + populateDetails([this, reply, uri, manifest, app_name](const PackageDetails &details){ |
132 | store_department(details); |
133 | pushPackagePreviewWidgets(reply, details, createButtons(uri, manifest)); |
134 | - |
135 | - if (review.rating == 0 && manifest.removable) { |
136 | - scopes::PreviewWidgetList review_input; |
137 | - scopes::PreviewWidget rating("rating", "rating-input"); |
138 | - rating.add_attribute_value("required", scopes::Variant("rating")); |
139 | - review_input.push_back(rating); |
140 | - reply->push(review_input); |
141 | - } |
142 | }, |
143 | - [this, reply](const ReviewList& reviewlist, |
144 | + [this, reply, &review, manifest, userid](const ReviewList& reviewlist, |
145 | click::Reviews::Error error) { |
146 | + auto reviews = sort(reviewlist, userid); |
147 | + scopes::PreviewWidgetList review_input; |
148 | + bool has_reviewed = reviews.size() > 0 && reviews.front().reviewer_username == userid; |
149 | + |
150 | + if (has_reviewed) { |
151 | + auto existing_review = reviews.front(); |
152 | + reviews.pop_front(); |
153 | + qDebug() << "Review for current user already exists, review id:" << existing_review.id; |
154 | + if (manifest.removable) { |
155 | + scopes::PreviewWidget rating(std::to_string(existing_review.id), "rating-edit"); // pass review id via widget id |
156 | + rating.add_attribute_value("required", scopes::Variant("rating")); |
157 | + rating.add_attribute_value("review", scopes::Variant(existing_review.review_text)); |
158 | + rating.add_attribute_value("rating", scopes::Variant(existing_review.rating)); |
159 | + rating.add_attribute_value("author", scopes::Variant(existing_review.reviewer_name)); |
160 | + review_input.push_back(rating); |
161 | + } |
162 | + } else { |
163 | + if (review.rating == 0 && manifest.removable) { |
164 | + scopes::PreviewWidget rating("rating", "rating-input"); |
165 | + rating.add_attribute_value("required", scopes::Variant("rating")); |
166 | + review_input.push_back(rating); |
167 | + } |
168 | + } |
169 | + reply->push(review_input); |
170 | + |
171 | if (error == click::Reviews::Error::NoError) { |
172 | - reply->push(reviewsWidgets(reviewlist)); |
173 | + reply->push(reviewsWidgets(reviews)); |
174 | } else { |
175 | qDebug() << "There was an error getting reviews for:" << result["name"].get_string().c_str(); |
176 | } |
177 | |
178 | === modified file 'libclickscope/click/preview.h' |
179 | --- libclickscope/click/preview.h 2015-04-10 14:24:10 +0000 |
180 | +++ libclickscope/click/preview.h 2015-05-11 15:29:02 +0000 |
181 | @@ -209,6 +209,7 @@ |
182 | |
183 | protected: |
184 | void getApplicationUri(const Manifest& manifest, std::function<void(const std::string&)> callback); |
185 | + std::string get_consumer_key(); |
186 | |
187 | private: |
188 | static scopes::PreviewWidgetList createButtons(const std::string& uri, |
189 | |
190 | === modified file 'libclickscope/click/reviews.cpp' |
191 | --- libclickscope/click/reviews.cpp 2014-05-26 14:02:45 +0000 |
192 | +++ libclickscope/click/reviews.cpp 2015-05-11 15:29:02 +0000 |
193 | @@ -28,6 +28,7 @@ |
194 | */ |
195 | |
196 | #include <cstdlib> |
197 | +#include <algorithm> |
198 | |
199 | #include <boost/property_tree/ptree.hpp> |
200 | #include <boost/property_tree/json_parser.hpp> |
201 | @@ -96,6 +97,25 @@ |
202 | return reviews; |
203 | } |
204 | |
205 | +ReviewList sort (const ReviewList& reviews, const std::string& userid) |
206 | +{ |
207 | + if (userid.size() == 0) |
208 | + { |
209 | + return reviews; |
210 | + } |
211 | + auto new_reviews = reviews; |
212 | + auto it = std::find_if(new_reviews.begin(), new_reviews.end(), [userid](const Review& review) { |
213 | + return review.reviewer_username == userid; |
214 | + }); |
215 | + if (it != new_reviews.end() && it != new_reviews.begin()) { |
216 | + // move own review to the front |
217 | + auto const review = *it; |
218 | + new_reviews.erase(it); |
219 | + new_reviews.push_front(review); |
220 | + } |
221 | + return new_reviews; |
222 | +} |
223 | + |
224 | Reviews::Reviews (const QSharedPointer<click::web::Client>& client) |
225 | : client(client) |
226 | { |
227 | @@ -174,6 +194,38 @@ |
228 | return click::web::Cancellable(response); |
229 | } |
230 | |
231 | +click::web::Cancellable Reviews::edit_review (const Review& review, |
232 | + std::function<void(Error)> callback) |
233 | +{ |
234 | + std::map<std::string, std::string> headers({ |
235 | + {click::web::CONTENT_TYPE_HEADER, click::web::CONTENT_TYPE_JSON}, |
236 | + }); |
237 | + Json::Value root(Json::ValueType::objectValue); |
238 | + root["rating"] = review.rating; |
239 | + root["review_text"] = review.review_text; |
240 | + // NOTE: "summary" is a required field, but we don't have one. Use "". |
241 | + root["summary"] = "Review"; |
242 | + |
243 | + qDebug() << "Rating" << review.package_name.c_str() << review.rating; |
244 | + |
245 | + QSharedPointer<click::web::Response> response = client->call |
246 | + (get_base_url() + click::REVIEWS_API_PATH + std::to_string(review.id) + "/", "PUT", true, |
247 | + headers, Json::FastWriter().write(root), click::web::CallParams()); |
248 | + |
249 | + QObject::connect(response.data(), &click::web::Response::finished, |
250 | + [=](QString) { |
251 | + qDebug() << "Review updated for:" << review.package_name.c_str(); |
252 | + callback(Error::NoError); |
253 | + }); |
254 | + QObject::connect(response.data(), &click::web::Response::error, |
255 | + [=](QString) { |
256 | + qCritical() << "Network error updating a review for:" << review.package_name.c_str(); |
257 | + callback(Error::NetworkError); |
258 | + }); |
259 | + |
260 | + return click::web::Cancellable(response); |
261 | +} |
262 | + |
263 | std::string Reviews::get_base_url () |
264 | { |
265 | const char *env_url = getenv(REVIEWS_BASE_URL_ENVVAR.c_str()); |
266 | |
267 | === modified file 'libclickscope/click/reviews.h' |
268 | --- libclickscope/click/reviews.h 2015-01-13 22:17:36 +0000 |
269 | +++ libclickscope/click/reviews.h 2015-05-11 15:29:02 +0000 |
270 | @@ -32,7 +32,7 @@ |
271 | |
272 | #include <functional> |
273 | #include <string> |
274 | -#include <vector> |
275 | +#include <list> |
276 | |
277 | #include <click/webclient.h> |
278 | |
279 | @@ -62,9 +62,10 @@ |
280 | std::string reviewer_username; |
281 | }; |
282 | |
283 | -typedef std::vector<Review> ReviewList; |
284 | +typedef std::list<Review> ReviewList; |
285 | |
286 | ReviewList review_list_from_json (const std::string& json); |
287 | +ReviewList sort (const ReviewList& reviews, const std::string& userid); |
288 | |
289 | class Reviews |
290 | { |
291 | @@ -80,6 +81,8 @@ |
292 | std::function<void(ReviewList, Error)> callback); |
293 | click::web::Cancellable submit_review (const Review& review, |
294 | std::function<void(Error)> callback); |
295 | + click::web::Cancellable edit_review (const Review& review, |
296 | + std::function<void(Error)> callback); |
297 | |
298 | static std::string get_base_url (); |
299 | }; |
300 | |
301 | === modified file 'libclickscope/tests/test_reviews.cpp' |
302 | --- libclickscope/tests/test_reviews.cpp 2014-05-26 14:27:31 +0000 |
303 | +++ libclickscope/tests/test_reviews.cpp 2015-05-11 15:29:02 +0000 |
304 | @@ -314,6 +314,49 @@ |
305 | } |
306 | } |
307 | |
308 | +TEST_F(ReviewsTest, testEditReviewUrlHasReviewId) |
309 | +{ |
310 | + LifetimeHelper<click::network::Reply, MockNetworkReply> reply; |
311 | + auto response = responseForReply(reply.asSharedPtr()); |
312 | + |
313 | + click::Review review; |
314 | + review.id = 1234; |
315 | + review.rating = 4; |
316 | + review.review_text = "A review"; |
317 | + review.package_name = "com.example.test"; |
318 | + review.package_version = "0.1"; |
319 | + |
320 | + EXPECT_CALL(*clientPtr, callImpl(HasSubstr("/1234/"), "PUT", true, _, _, _)) |
321 | + .Times(1) |
322 | + .WillOnce(Return(response)); |
323 | + |
324 | + auto submit_op = reviewsPtr->edit_review(review, |
325 | + [](click::Reviews::Error){}); |
326 | +} |
327 | + |
328 | +TEST_F(ReviewsTest, testEditReviewIsCancellable) |
329 | +{ |
330 | + LifetimeHelper<click::network::Reply, MockNetworkReply> reply; |
331 | + auto response = responseForReply(reply.asSharedPtr()); |
332 | + |
333 | + click::Review review; |
334 | + review.id = 1234; |
335 | + review.rating = 4; |
336 | + review.review_text = "A review"; |
337 | + review.package_name = "com.example.test"; |
338 | + review.package_version = "0.1"; |
339 | + |
340 | + EXPECT_CALL(*clientPtr, callImpl(_, "PUT", true, _, _, _)) |
341 | + .Times(1) |
342 | + .WillOnce(Return(response)); |
343 | + |
344 | + auto submit_op = reviewsPtr->edit_review(review, |
345 | + [](click::Reviews::Error){}); |
346 | + EXPECT_CALL(reply.instance, abort()).Times(1); |
347 | + submit_op.cancel(); |
348 | +} |
349 | + |
350 | + |
351 | TEST_F(ReviewsTest, testGetBaseUrl) |
352 | { |
353 | const char *value = getenv(click::REVIEWS_BASE_URL_ENVVAR.c_str()); |
354 | |
355 | === modified file 'scope/clickapps/apps-scope.cpp' |
356 | --- scope/clickapps/apps-scope.cpp 2015-04-10 14:24:10 +0000 |
357 | +++ scope/clickapps/apps-scope.cpp 2015-05-11 15:29:02 +0000 |
358 | @@ -105,7 +105,7 @@ |
359 | |
360 | |
361 | unity::scopes::ActivationQueryBase::UPtr click::Scope::perform_action(unity::scopes::Result const& result, unity::scopes::ActionMetadata const& metadata, |
362 | - std::string const& /* widget_id */, std::string const& action_id) |
363 | + std::string const& widget_id, std::string const& action_id) |
364 | { |
365 | if (action_id == click::Preview::Actions::CONFIRM_UNINSTALL) { |
366 | auto response = unity::scopes::ActivationResponse(unity::scopes::ActivationResponse::ShowDash); |
367 | @@ -137,6 +137,7 @@ |
368 | activation->setHint("review", scopes::Variant(review_text)); |
369 | activation->setHint(click::Preview::Actions::RATED, |
370 | scopes::Variant(true)); |
371 | + activation->setHint("widget_id", scopes::Variant(widget_id)); |
372 | activation->setStatus(scopes::ActivationResponse::Status::ShowPreview); |
373 | } |
374 | return scopes::ActivationQueryBase::UPtr(activation); |
FAILED: Continuous integration, rev:320 jenkins. qa.ubuntu. com/job/ unity-scope- click-ci/ 592/ jenkins. qa.ubuntu. com/job/ unity-scope- click-wily- amd64-ci/ 4/console jenkins. qa.ubuntu. com/job/ unity-scope- click-wily- armhf-ci/ 4/console
http://
Executed test runs:
FAILURE: http://
FAILURE: http://
Click here to trigger a rebuild: s-jenkins. ubuntu- ci:8080/ job/unity- scope-click- ci/592/ rebuild
http://