Merge lp:~marcustomlinson/unity-scope-click/lp-1578283 into lp:unity-scope-click

Proposed by Marcus Tomlinson on 2016-05-09
Status: Rejected
Rejected by: Marcus Tomlinson on 2016-05-11
Proposed branch: lp:~marcustomlinson/unity-scope-click/lp-1578283
Merge into: lp:unity-scope-click
Diff against target: 454 lines (+55/-109)
18 files modified
libclickscope/click/download-manager.cpp (+3/-2)
libclickscope/click/pay.cpp (+3/-0)
libclickscope/click/preview.cpp (+4/-0)
libclickscope/click/reviews.cpp (+1/-2)
libclickscope/click/ubuntuone_credentials.cpp (+1/-41)
libclickscope/click/ubuntuone_credentials.h (+2/-3)
libclickscope/click/webclient.cpp (+19/-15)
libclickscope/click/webclient.h (+1/-1)
libclickscope/tests/mock_ubuntuone_credentials.h (+5/-5)
libclickscope/tests/mock_webclient.h (+3/-2)
libclickscope/tests/test_download_manager.cpp (+4/-2)
libclickscope/tests/test_index.cpp (+1/-13)
libclickscope/tests/test_reviews.cpp (+0/-13)
libclickscope/tests/test_webclient.cpp (+8/-4)
scope/clickapps/apps-scope.cpp (+0/-2)
scope/clickapps/apps-scope.h (+0/-1)
scope/clickstore/store-scope.cpp (+0/-2)
scope/clickstore/store-scope.h (+0/-1)
To merge this branch: bzr merge lp:~marcustomlinson/unity-scope-click/lp-1578283
Reviewer Review Type Date Requested Status
Marcus Tomlinson (community) Disapprove on 2016-05-11
Paweł Stołowski 2016-05-09 Approve on 2016-05-10
PS Jenkins bot continuous-integration Needs Fixing on 2016-05-10
Review via email: mp+294135@code.launchpad.net
To post a comment you must log in.
Marcus Tomlinson (marcustomlinson) wrote :

This is in silo 54 at the moment. So far, those who have experienced the blank apps scope have been unable to reproduce with this applied.

I wish I could reproduce the issue myself so that I can better understand why this would fix it :/

Paweł Stołowski (stolowski) wrote :

+1

review: Approve

Unmerged revisions

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'libclickscope/click/download-manager.cpp'
2--- libclickscope/click/download-manager.cpp 2016-04-15 06:43:36 +0000
3+++ libclickscope/click/download-manager.cpp 2016-05-09 17:12:23 +0000
4@@ -154,11 +154,11 @@
5 });
6 QObject::connect(response.data(), &click::web::Response::error,
7 [this, callback, package_name](QString error, int error_code) {
8- qWarning() << QStringLiteral("Network error (%1) fetching click token for:").arg(error_code) << package_name.c_str();
9+ qDebug() << QStringLiteral("Network error (%1) fetching click token for:").arg(error_code) << package_name.c_str();
10 switch(error_code) {
11 case 401:
12 case 403:
13- client->invalidateCredentials();
14+ sso->invalidateCredentials();
15 callback(error.toUtf8().data(), Error::CredentialsError);
16 break;
17 default:
18@@ -172,6 +172,7 @@
19 void DownloadManager::setCredentialsService(const QSharedPointer<click::CredentialsService>& credentialsService)
20 {
21 sso = credentialsService;
22+ client->setCredentialsService(sso);
23 }
24
25 } // namespace click
26
27=== modified file 'libclickscope/click/pay.cpp'
28--- libclickscope/click/pay.cpp 2016-04-01 17:16:05 +0000
29+++ libclickscope/click/pay.cpp 2016-05-09 17:12:23 +0000
30@@ -228,6 +228,9 @@
31
32 click::web::Cancellable Package::get_purchases(std::function<void(const PurchaseSet&)> callback)
33 {
34+ QSharedPointer<click::CredentialsService> sso(new click::CredentialsService());
35+ client->setCredentialsService(sso);
36+
37 QSharedPointer<click::web::Response> response = client->call
38 (get_base_url() + pay::API_ROOT + pay::PURCHASES_API_PATH, "GET", true);
39
40
41=== modified file 'libclickscope/click/preview.cpp'
42--- libclickscope/click/preview.cpp 2016-04-01 17:16:05 +0000
43+++ libclickscope/click/preview.cpp 2016-05-09 17:12:23 +0000
44@@ -734,6 +734,8 @@
45 std::promise<bool> promise;
46 auto future = promise.get_future();
47 run_under_qt([this, reply, &promise]() {
48+ QSharedPointer<click::CredentialsService> sso(new click::CredentialsService());
49+ dm->setCredentialsService(sso);
50 dm->start(download_url, download_sha512, result["name"].get_string(),
51 [this, reply, &promise] (std::string msg, DownloadManager::Error dmerr){
52 switch (dmerr)
53@@ -886,6 +888,8 @@
54 std::promise<bool> submit_promise;
55 std::future<bool> submit_future = submit_promise.get_future();
56 qt::core::world::enter_with_task([this, review, &submit_promise, widget_id]() mutable {
57+ QSharedPointer<click::CredentialsService> sso(new click::CredentialsService());
58+ client->setCredentialsService(sso);
59 if (widget_id == "rating") {
60 submit_operation = reviews->submit_review(review,
61 [&submit_promise](click::Reviews::Error){
62
63=== modified file 'libclickscope/click/reviews.cpp'
64--- libclickscope/click/reviews.cpp 2016-04-01 17:16:05 +0000
65+++ libclickscope/click/reviews.cpp 2016-05-09 17:12:23 +0000
66@@ -131,8 +131,7 @@
67 click::web::CallParams params;
68 params.add(click::REVIEWS_QUERY_ARGNAME, package_name.c_str());
69 QSharedPointer<click::web::Response> response = client->call
70- (get_base_url() + click::REVIEWS_API_PATH, "GET", false,
71- std::map<std::string, std::string>{}, "", params);
72+ (get_base_url() + click::REVIEWS_API_PATH, params);
73
74 QObject::connect(response.data(), &click::web::Response::finished,
75 [=](QString reply) {
76
77=== modified file 'libclickscope/click/ubuntuone_credentials.cpp'
78--- libclickscope/click/ubuntuone_credentials.cpp 2016-04-01 17:16:05 +0000
79+++ libclickscope/click/ubuntuone_credentials.cpp 2016-05-09 17:12:23 +0000
80@@ -1,5 +1,5 @@
81 /*
82- * Copyright (C) 2014-2016 Canonical Ltd.
83+ * Copyright (C) 2014 Canonical Ltd.
84 *
85 * This program is free software: you can redistribute it and/or modify it
86 * under the terms of the GNU General Public License version 3, as published
87@@ -29,9 +29,6 @@
88
89 #include "ubuntuone_credentials.h"
90
91-#include <future>
92-#include <QCoreApplication>
93-
94 namespace u1 = UbuntuOne;
95
96 click::CredentialsService::CredentialsService()
97@@ -50,43 +47,6 @@
98 {
99 }
100
101-UbuntuOne::Token click::CredentialsService::getToken()
102-{
103- if (!_token.isValid()) {
104- std::promise<UbuntuOne::Token> promise;
105- auto future = promise.get_future();
106-
107- auto success = QObject::connect(ssoService.data(),
108- &u1::SSOService::credentialsFound,
109- [this, &promise](const u1::Token& token) {
110- emit credentialsFound(_token);
111- promise.set_value(token);
112- });
113- auto notfound = QObject::connect(ssoService.data(),
114- &u1::SSOService::credentialsNotFound,
115- [this, &promise]() {
116- qWarning() << "No Ubuntu One token found.";
117- emit credentialsNotFound();
118- promise.set_value(u1::Token());
119- });
120-
121- getCredentials();
122-
123- std::future_status status = future.wait_for(std::chrono::milliseconds(0));
124- while (status != std::future_status::ready) {
125- QCoreApplication::processEvents();
126- qDebug() << "Processed some events, waiting to process again.";
127- status = future.wait_for(std::chrono::milliseconds(100));
128- }
129-
130- _token = future.get();
131- QObject::disconnect(success);
132- QObject::disconnect(notfound);
133- }
134-
135- return _token;
136-}
137-
138 void click::CredentialsService::getCredentials()
139 {
140 ssoService->getCredentials();
141
142=== modified file 'libclickscope/click/ubuntuone_credentials.h'
143--- libclickscope/click/ubuntuone_credentials.h 2016-04-01 17:16:05 +0000
144+++ libclickscope/click/ubuntuone_credentials.h 2016-05-09 17:12:23 +0000
145@@ -1,5 +1,5 @@
146 /*
147- * Copyright (C) 2014-2016 Canonical Ltd.
148+ * Copyright (C) 2014 Canonical Ltd.
149 *
150 * This program is free software: you can redistribute it and/or modify it
151 * under the terms of the GNU General Public License version 3, as published
152@@ -47,7 +47,6 @@
153
154 CredentialsService& operator=(const CredentialsService&) = delete;
155
156- virtual UbuntuOne::Token getToken();
157 virtual void getCredentials();
158 virtual void invalidateCredentials();
159
160@@ -58,7 +57,7 @@
161
162 private:
163 QScopedPointer<UbuntuOne::SSOService> ssoService;
164- UbuntuOne::Token _token;
165+
166 }; // CredentialsService
167
168 } // namespace click
169
170=== modified file 'libclickscope/click/webclient.cpp'
171--- libclickscope/click/webclient.cpp 2016-04-01 17:16:05 +0000
172+++ libclickscope/click/webclient.cpp 2016-05-09 17:12:23 +0000
173@@ -82,7 +82,7 @@
174 const std::string& iri,
175 const click::web::CallParams& params)
176 {
177- return call(iri, "GET", true,
178+ return call(iri, "GET", false,
179 std::map<std::string, std::string>(), "", params);
180 }
181
182@@ -132,21 +132,25 @@
183 request->setRawHeader(DEVICE_ID_HEADER.c_str(), deviceId.data());
184
185 if (sign && !impl->sso.isNull()) {
186- auto token = impl->sso->getToken();
187- if (token.isValid()) {
188- QString auth_header = token.signUrl(url.toString(),
189- method.c_str());
190- qDebug() << "Signed URL:" << request->url().toString();
191- request->setRawHeader(AUTHORIZATION_HEADER.c_str(), auth_header.toUtf8());
192- } else {
193- qWarning() << "Signing reuested but returned token is invalid.";
194- }
195-
196- doConnect();
197+ click::utils::SmartConnect sc(responsePtr.data());
198+ sc.connect(impl->sso.data(), &click::CredentialsService::credentialsFound,
199+ [=](const UbuntuOne::Token& token) {
200+ QString auth_header = token.signUrl(url.toString(),
201+ method.c_str());
202+ qDebug() << "Signed URL:" << request->url().toString();
203+ request->setRawHeader(AUTHORIZATION_HEADER.c_str(), auth_header.toUtf8());
204+ impl->sso.clear();
205+ doConnect();
206+ });
207+ sc.connect(impl->sso.data(), &click::CredentialsService::credentialsNotFound,
208+ [=]() {
209+ impl->sso.clear();
210+ qWarning() << "Signing reuested but no credentials found. Using unsigned URL.";
211+ doConnect();
212+ });
213+ // TODO: Need to handle error signal once in CredentialsService.
214+ impl->sso->getCredentials();
215 } else {
216- if (sign && impl->sso.isNull()) {
217- qCritical() << "Unable to sign request without SSO object.";
218- }
219 doConnect();
220 }
221
222
223=== modified file 'libclickscope/click/webclient.h'
224--- libclickscope/click/webclient.h 2016-04-01 17:16:05 +0000
225+++ libclickscope/click/webclient.h 2016-05-09 17:12:23 +0000
226@@ -121,7 +121,7 @@
227 virtual QSharedPointer<Response> call(
228 const std::string& iri,
229 const std::string& method,
230- bool sign = true,
231+ bool sign = false,
232 const std::map<std::string, std::string>& headers = std::map<std::string, std::string>(),
233 const std::string& data = "",
234 const CallParams& params = CallParams());
235
236=== modified file 'libclickscope/tests/mock_ubuntuone_credentials.h'
237--- libclickscope/tests/mock_ubuntuone_credentials.h 2016-04-01 17:16:05 +0000
238+++ libclickscope/tests/mock_ubuntuone_credentials.h 2016-05-09 17:12:23 +0000
239@@ -1,5 +1,5 @@
240 /*
241- * Copyright (C) 2014-2016 Canonical Ltd.
242+ * Copyright (C) 2014 Canonical Ltd.
243 *
244 * This program is free software: you can redistribute it and/or modify it
245 * under the terms of the GNU General Public License version 3, as published
246@@ -27,11 +27,11 @@
247 * files in the program, then also delete it here.
248 */
249
250-#include <token.h>
251
252 class MockCredentialsService : public click::CredentialsService {
253 public:
254- MOCK_METHOD0(getToken, UbuntuOne::Token());
255- MOCK_METHOD0(getCredentials, void());
256- MOCK_METHOD0(invalidateCredentials, void());
257+ MOCK_METHOD0(getCredentials,
258+ void());
259+ MOCK_METHOD0(invalidateCredentials,
260+ void());
261 };
262
263=== modified file 'libclickscope/tests/mock_webclient.h'
264--- libclickscope/tests/mock_webclient.h 2016-04-01 17:16:05 +0000
265+++ libclickscope/tests/mock_webclient.h 2016-05-09 17:12:23 +0000
266@@ -87,13 +87,13 @@
267 QSharedPointer<click::web::Response> call(
268 const std::string& iri,
269 const click::web::CallParams& params=click::web::CallParams()) override {
270- return callImpl(iri, "GET", true,
271+ return callImpl(iri, "GET", false,
272 std::map<std::string, std::string>(), "", params);
273 }
274 QSharedPointer<click::web::Response> call(
275 const std::string& iri,
276 const std::string& method,
277- bool sign = true,
278+ bool sign = false,
279 const std::map<std::string, std::string>& headers = std::map<std::string, std::string>(),
280 const std::string& data = "",
281 const click::web::CallParams& params=click::web::CallParams()) override {
282@@ -102,6 +102,7 @@
283
284 MOCK_METHOD1(has_header, bool(const std::string& header));
285 MOCK_METHOD1(get_header, std::string(const std::string&header));
286+ MOCK_METHOD0(invalidateCredentials, void());
287 };
288
289 }
290
291=== modified file 'libclickscope/tests/test_download_manager.cpp'
292--- libclickscope/tests/test_download_manager.cpp 2016-04-01 17:16:05 +0000
293+++ libclickscope/tests/test_download_manager.cpp 2016-05-09 17:12:23 +0000
294@@ -63,7 +63,6 @@
295
296 virtual void SetUp()
297 {
298- ssoPtr.reset(new MockCredentialsService());
299 namPtr.reset(new MockNetworkAccessManager());
300 clientPtr.reset(new NiceMock<MockClient>(namPtr));
301 clientPtr->setCredentialsService(ssoPtr);
302@@ -161,6 +160,9 @@
303 LifetimeHelper<click::network::Reply, MockNetworkReply> reply;
304 auto response = responseForReply(reply.asSharedPtr());
305
306+ QSharedPointer<MockCredentialsService> sso(new MockCredentialsService());
307+ dmPtr->setCredentialsService(sso);
308+
309 EXPECT_CALL(reply.instance, errorString())
310 .WillOnce(Return(QString("ERROR")));
311 EXPECT_CALL(reply.instance, attribute(_)).WillOnce(Return(QVariant(401)));
312@@ -170,7 +172,7 @@
313 EXPECT_CALL(*clientPtr, callImpl(_, _, _, _, _, _))
314 .Times(1)
315 .WillOnce(Return(response));
316- EXPECT_CALL(*ssoPtr, invalidateCredentials());
317+ EXPECT_CALL(*(sso.data()), invalidateCredentials());
318 EXPECT_CALL(*this, start_callback("ERROR (201)",
319 click::DownloadManager::Error::CredentialsError)).Times(1);
320
321
322=== modified file 'libclickscope/tests/test_index.cpp'
323--- libclickscope/tests/test_index.cpp 2016-04-01 17:16:05 +0000
324+++ libclickscope/tests/test_index.cpp 2016-05-09 17:12:23 +0000
325@@ -1,5 +1,5 @@
326 /*
327- * Copyright (C) 2014-2016 Canonical Ltd.
328+ * Copyright (C) 2014 Canonical Ltd.
329 *
330 * This program is free software: you can redistribute it and/or modify it
331 * under the terms of the GNU General Public License version 3, as published
332@@ -190,18 +190,6 @@
333 indexPtr->departments("departments", [](const click::DepartmentList&, const click::HighlightList&, click::Index::Error, int) {});
334 }
335
336-TEST_F(IndexTest, testDetailsSignsCall)
337-{
338- LifetimeHelper<click::network::Reply, MockNetworkReply> reply;
339- auto response = responseForReply(reply.asSharedPtr());
340-
341- EXPECT_CALL(*clientPtr, callImpl(_, _, true, _, _, _))
342- .Times(1)
343- .WillOnce(Return(response));
344-
345- indexPtr->get_details("fake-app", [](const click::PackageDetails, click::Index::Error) {});
346-}
347-
348 TEST_F(IndexTest, testSearchSendsRightPath)
349 {
350 LifetimeHelper<click::network::Reply, MockNetworkReply> reply;
351
352=== modified file 'libclickscope/tests/test_reviews.cpp'
353--- libclickscope/tests/test_reviews.cpp 2016-04-01 17:16:05 +0000
354+++ libclickscope/tests/test_reviews.cpp 2016-05-09 17:12:23 +0000
355@@ -150,19 +150,6 @@
356 click::Reviews::Error) {});
357 }
358
359-TEST_F(ReviewsTest, testFetchReviewsDoesNotSignCall)
360-{
361- LifetimeHelper<click::network::Reply, MockNetworkReply> reply;
362- auto response = responseForReply(reply.asSharedPtr());
363-
364- EXPECT_CALL(*clientPtr, callImpl(_, _, false, _, _, _))
365- .Times(1)
366- .WillOnce(Return(response));
367-
368- reviewsPtr->fetch_reviews("", [](click::ReviewList,
369- click::Reviews::Error) {});
370-}
371-
372 TEST_F(ReviewsTest, testFetchReviewsSendsQueryAsParam)
373 {
374 LifetimeHelper<click::network::Reply, MockNetworkReply> reply;
375
376=== modified file 'libclickscope/tests/test_webclient.cpp'
377--- libclickscope/tests/test_webclient.cpp 2016-04-01 17:16:05 +0000
378+++ libclickscope/tests/test_webclient.cpp 2016-05-09 17:12:23 +0000
379@@ -221,9 +221,11 @@
380 click::web::Client wc(namPtr);
381 wc.setCredentialsService(ssoPtr);
382
383- EXPECT_CALL(sso, getToken())
384- .WillOnce(Return(UbuntuOne::Token("token_key", "token_secret",
385- "consumer_key", "consumer_secret")));
386+ EXPECT_CALL(sso, getCredentials()).WillOnce(Invoke([&](){
387+ UbuntuOne::Token token("token_key", "token_secret",
388+ "consumer_key", "consumer_secret");
389+ sso.credentialsFound(token);
390+ }));
391 EXPECT_CALL(nam, sendCustomRequest(IsValidOAuthHeader(true), _, _))
392 .Times(1)
393 .WillOnce(Return(replyPtr));
394@@ -269,7 +271,9 @@
395 click::web::Client wc(namPtr);
396 wc.setCredentialsService(ssoPtr);
397
398- EXPECT_CALL(sso, getToken()).WillOnce(Return(UbuntuOne::Token()));
399+ EXPECT_CALL(sso, getCredentials()).WillOnce(Invoke([&]() {
400+ sso.credentialsNotFound();
401+ }));
402 EXPECT_CALL(nam, sendCustomRequest(IsValidOAuthHeader(false), _, _))
403 .Times(1)
404 .WillOnce(Return(replyPtr));
405
406=== modified file 'scope/clickapps/apps-scope.cpp'
407--- scope/clickapps/apps-scope.cpp 2016-04-15 09:24:18 +0000
408+++ scope/clickapps/apps-scope.cpp 2016-05-09 17:12:23 +0000
409@@ -82,8 +82,6 @@
410 static const int zero = 0;
411 auto emptyCb = [this]()
412 {
413- sso.reset(new click::CredentialsService());
414- client->setCredentialsService(sso);
415 dm.reset(Ubuntu::DownloadManager::Manager::createSessionManager());
416 qt_ready_p.set_value();
417 };
418
419=== modified file 'scope/clickapps/apps-scope.h'
420--- scope/clickapps/apps-scope.h 2016-04-15 09:24:18 +0000
421+++ scope/clickapps/apps-scope.h 2016-05-09 17:12:23 +0000
422@@ -75,7 +75,6 @@
423 QSharedPointer<click::Index> index;
424 QSharedPointer<pay::Package> pay_package;
425 QSharedPointer<Ubuntu::DownloadManager::Manager> dm;
426- QSharedPointer<click::CredentialsService> sso;
427 std::shared_ptr<click::DepartmentsDb> depts_db;
428
429 std::string installApplication(unity::scopes::Result const& result);
430
431=== modified file 'scope/clickstore/store-scope.cpp'
432--- scope/clickstore/store-scope.cpp 2016-04-15 09:24:18 +0000
433+++ scope/clickstore/store-scope.cpp 2016-05-09 17:12:23 +0000
434@@ -84,8 +84,6 @@
435 static const int zero = 0;
436 auto emptyCb = [this]()
437 {
438- sso.reset(new click::CredentialsService());
439- client->setCredentialsService(sso);
440 dm.reset(Ubuntu::DownloadManager::Manager::createSessionManager());
441 qt_ready_p.set_value();
442 };
443
444=== modified file 'scope/clickstore/store-scope.h'
445--- scope/clickstore/store-scope.h 2016-04-15 09:24:18 +0000
446+++ scope/clickstore/store-scope.h 2016-05-09 17:12:23 +0000
447@@ -78,7 +78,6 @@
448 QSharedPointer<click::Index> index;
449 QSharedPointer<pay::Package> pay_package;
450 QSharedPointer<Ubuntu::DownloadManager::Manager> dm;
451- QSharedPointer<click::CredentialsService> sso;
452 std::shared_ptr<click::DepartmentLookup> depts;
453 std::shared_ptr<click::HighlightList> highlights;
454 std::shared_ptr<click::DepartmentsDb> depts_db;

Subscribers

People subscribed via source and target branches