Merge lp:~dobey/unity-scope-click/review-crash into lp:unity-scope-click

[dobey]

Only try to parse the JSON when we get a 200.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:404
http://jenkins.qa.ubuntu.com/job/unity-scope-click-ci/691/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scope-click-wily-amd64-ci/95/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scope-click-wily-armhf-ci/91/console

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

review: Needs Fixing (continuous-integration)

[Paweł Stołowski (stolowski)]

+ qCritical() << "Status:" << status;

Should use qDebug(), no need to spam our logs with criticals on every reply.

Otherwise looking good.

review: Needs Fixing

[lp:~dobey/unity-scope-click/review-crash]

405. By dobey on 2016-01-27

Remove the leftover debug log message.

[Paweł Stołowski (stolowski)]

Looks good, thanks!

review: Approve

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:405
http://jenkins.qa.ubuntu.com/job/unity-scope-click-ci/697/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scope-click-wily-amd64-ci/101/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scope-click-wily-armhf-ci/97/console

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

review: Needs Fixing (continuous-integration)

=== modified file 'libclickscope/click/reviews.cpp'
--- libclickscope/click/reviews.cpp	2015-11-24 18:23:23 +0000
+++ libclickscope/click/reviews.cpp	2016-01-27 18:26:06 +0000
@@ -135,9 +135,14 @@
 
     QObject::connect(response.data(), &click::web::Response::finished,
                 [=](QString reply) {
+                    auto status = response->get_status_code();
                     click::ReviewList reviews;
-                    reviews = review_list_from_json(reply.toUtf8().constData());
-                    callback(reviews, click::Reviews::Error::NoError);
+                    if (status == 200) {
+                        reviews = review_list_from_json(reply.toUtf8().constData());
+                        callback(reviews, click::Reviews::Error::NoError);
+                    } else {
+                        callback(reviews, click::Reviews::Error::NetworkError);
+                    }
                 });
     QObject::connect(response.data(), &click::web::Response::error,
                 [=](QString) {

=== modified file 'libclickscope/click/webclient.cpp'
--- libclickscope/click/webclient.cpp	2015-11-24 19:14:11 +0000
+++ libclickscope/click/webclient.cpp	2016-01-27 18:26:06 +0000
@@ -183,6 +183,11 @@
     return reply->rawHeader(header.c_str()).toUtf8().data();
 }
 
+int click::web::Response::get_status_code() const
+{
+    return reply->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt();
+}
+
 void click::web::Response::replyFinished()
 {
     auto response = reply->readAll();

=== modified file 'libclickscope/click/webclient.h'
--- libclickscope/click/webclient.h	2015-11-24 19:14:11 +0000
+++ libclickscope/click/webclient.h	2016-01-27 18:26:06 +0000
@@ -81,6 +81,7 @@
 
     virtual bool has_header(const std::string& header) const;
     virtual std::string get_header(const std::string& header) const;
+    virtual int get_status_code() const;
 
 public slots:
     void replyFinished();

=== modified file 'libclickscope/tests/test_reviews.cpp'
--- libclickscope/tests/test_reviews.cpp	2015-11-24 18:23:23 +0000
+++ libclickscope/tests/test_reviews.cpp	2016-01-27 18:26:06 +0000
@@ -200,12 +200,35 @@
     response->replyFinished();
 }
 
+TEST_F(ReviewsTest, testFetchReviewsNot200)
+{
+    LifetimeHelper<click::network::Reply, MockNetworkReply> reply;
+    auto response = responseForReply(reply.asSharedPtr());
+
+    EXPECT_CALL(reply.instance, attribute(_)).WillOnce(Return(QVariant(301)));
+    EXPECT_CALL(reply.instance, readAll())
+            .Times(1)
+            .WillOnce(Return(FAKE_JSON_REVIEWS_RESULT_ONE.c_str()));
+    EXPECT_CALL(*clientPtr, callImpl(_, _, _, _, _, _))
+            .Times(1)
+            .WillOnce(Return(response));
+    click::ReviewList empty_reviews_list;
+    EXPECT_CALL(*this, reviews_callback(empty_reviews_list, _)).Times(1);
+
+    reviewsPtr->fetch_reviews("", [this](click::ReviewList reviews,
+                                         click::Reviews::Error error){
+                                  reviews_callback(reviews, error);
+                              });
+    response->replyFinished();
+}
+
 TEST_F(ReviewsTest, testFetchReviewsEmptyJsonIsParsed)
 {
     LifetimeHelper<click::network::Reply, MockNetworkReply> reply;
     auto response = responseForReply(reply.asSharedPtr());
 
     QByteArray fake_json("[]");
+    EXPECT_CALL(reply.instance, attribute(_)).WillOnce(Return(QVariant(200)));
     EXPECT_CALL(reply.instance, readAll())
             .Times(1)
             .WillOnce(Return(fake_json));
@@ -228,6 +251,7 @@
     auto response = responseForReply(reply.asSharedPtr());
 
     QByteArray fake_json(FAKE_JSON_REVIEWS_RESULT_ONE.c_str());
+    EXPECT_CALL(reply.instance, attribute(_)).WillOnce(Return(QVariant(200)));
     EXPECT_CALL(reply.instance, readAll())
             .Times(1)
             .WillOnce(Return(fake_json));