Merge lp:~dobey/unity-scope-click/submit-reviews-backend into lp:unity-scope-click/devel

[dobey]

Add a backend method to submit a review to the server.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:213
http://jenkins.qa.ubuntu.com/job/unity-scope-click-ci/440/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scope-click-trusty-amd64-ci/341
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scope-click-trusty-armhf-ci/338
        deb: http://jenkins.qa.ubuntu.com/job/unity-scope-click-trusty-armhf-ci/338/artifact/work/output/*zip*/output.zip

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

review: Approve (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:214
http://jenkins.qa.ubuntu.com/job/unity-scope-click-ci/443/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scope-click-trusty-amd64-ci/344
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scope-click-trusty-armhf-ci/341
        deb: http://jenkins.qa.ubuntu.com/job/unity-scope-click-trusty-armhf-ci/341/artifact/work/output/*zip*/output.zip

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

review: Approve (continuous-integration)

[Paweł Stołowski (stolowski)]

26 + data << "{"
27 + << " 'package_name': '" << package.package.name << "',"
28 + << " 'version': '" << package.package.version << "',"
29 + << " 'rating': " << rating << ","
30 + << " 'review_text': '" << review_text << "',"
31 + << " 'summary': ''"
32 + << " }";

IMHO constructing json like that is a bad idea... What about proper escaping so that the resulting json is stil valid if review text contains an apostrophe? I think we should be using Json::FastWriter from jsoncpp instead.

Also, question - what is the purpose of the empty 'summary' attribute?

review: Needs Fixing

[dobey (dobey)]

> 26 + data << "{"
> 27 + << " 'package_name': '" << package.package.name << "',"
> 28 + << " 'version': '" << package.package.version << "',"
> 29 + << " 'rating': " << rating << ","
> 30 + << " 'review_text': '" << review_text << "',"
> 31 + << " 'summary': ''"
> 32 + << " }";
>
> IMHO constructing json like that is a bad idea... What about proper escaping
> so that the resulting json is stil valid if review text contains an
> apostrophe? I think we should be using Json::FastWriter from jsoncpp instead.

Is there any good example of how to use that API? I spent some time trying to find how to construct a JSON string in C++, and best I found was to use a string stream.

> Also, question - what is the purpose of the empty 'summary' attribute?

The 'summary' is required by the server, but we have no UI to enter a summary (and summaries don't really make sense), so we just send an empty string for it.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:215
http://jenkins.qa.ubuntu.com/job/unity-scope-click-ci/450/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scope-click-trusty-amd64-ci/351
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scope-click-trusty-armhf-ci/348
        deb: http://jenkins.qa.ubuntu.com/job/unity-scope-click-trusty-armhf-ci/348/artifact/work/output/*zip*/output.zip

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

review: Approve (continuous-integration)

[dobey (dobey)]

> IMHO constructing json like that is a bad idea... What about proper escaping
> so that the resulting json is stil valid if review text contains an
> apostrophe? I think we should be using Json::FastWriter from jsoncpp instead.

Discovered Json::Value to use here. No need for FastWriter AFAICT, just creating a root Value, adding the members, and converting it with .toStyledString() appears to be all we need.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:217
http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-ci/6/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-trusty-amd64-ci/6
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-trusty-armhf-ci/6
        deb: http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-trusty-armhf-ci/6/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-trusty-i386-ci/6

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

review: Approve (continuous-integration)

[Alejandro J. Cura (alecu)]

Branch looks good, but please add some test that checks that the json is being built right, and that both unicode and json special characters are escaped properly.

For this, it might make sense to refactor into a new function the bits that create the json, including the root.toStyledString(), then add a few tests for this function, plus one test that checks that Reviews::submit_review calls it right.

review: Needs Fixing

[Paweł Stołowski (stolowski)]

> > IMHO constructing json like that is a bad idea... What about proper escaping
> > so that the resulting json is stil valid if review text contains an
> > apostrophe? I think we should be using Json::FastWriter from jsoncpp
> instead.
>
> Discovered Json::Value to use here. No need for FastWriter AFAICT, just
> creating a root Value, adding the members, and converting it with
> .toStyledString() appears to be all we need.

Right, but if you take a look at the implementation of .toStyledString(), it just creates StyledWriter that outputs human-friendly JSON (i.e. with line breaks and indentation), whereas FastWriter outputs JSON without any formatting, which saves a little bit of bandwidth ;). Creating a FastWriter and passing it the root value yourself is just one extra line of code - see http://jsoncpp.sourceforge.net/json__value_8cpp_source.html#l01471 - and I think it's worth it; we don't need the data to be pretty-printed.

review: Needs Fixing

[lp:~dobey/unity-scope-click/submit-reviews-backend]

218. By dobey on 2014-04-25

Connect to the finished/error signals on the request, to log the result.

219. By dobey on 2014-04-25

Add a test that ensures we're passing UTf8 properly to the server in JSON.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:219
http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-ci/8/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-trusty-amd64-ci/8
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-trusty-armhf-ci/8
        deb: http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-trusty-armhf-ci/8/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-trusty-i386-ci/8

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

review: Approve (continuous-integration)

[Alejandro J. Cura (alecu)]

Looks good, +1.

ps: small typo here:
+ // NOTE: gmock replaes the \" above as \\\" for HasSubstr(), so we have

review: Approve

[lp:~dobey/unity-scope-click/submit-reviews-backend]

220. By dobey on 2014-04-25

Fix small typo.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:220
http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-ci/11/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-trusty-amd64-ci/11
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-trusty-armhf-ci/11
        deb: http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-trusty-armhf-ci/11/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-trusty-i386-ci/11

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

review: Approve (continuous-integration)

[Paweł Stołowski (stolowski)]

LGTM, +1.

review: Approve

[PS Jenkins bot (ps-jenkins)]

review: Approve (continuous-integration)

=== modified file 'scope/click/reviews.cpp'
--- scope/click/reviews.cpp	2014-04-09 18:49:03 +0000
+++ scope/click/reviews.cpp	2014-04-25 18:38:08 +0000
@@ -33,6 +33,9 @@
 #include <boost/property_tree/json_parser.hpp>
 #include <boost/foreach.hpp>
 
+#include <json/value.h>
+#include <json/writer.h>
+
 #include "reviews.h"
 
 namespace click
@@ -123,6 +126,42 @@
     return click::web::Cancellable(response);
 }
 
+click::web::Cancellable Reviews::submit_review (const PackageDetails& package,
+                                                int rating,
+                                                const std::string& review_text)
+{
+    std::map<std::string, std::string> headers({
+            {click::web::CONTENT_TYPE_HEADER, click::web::CONTENT_TYPE_JSON},
+                });
+    // TODO: Need to get language for the package/review.
+    Json::Value root(Json::ValueType::objectValue);
+    root["package_name"] = package.package.name;
+    root["version"] = package.package.version;
+    root["rating"] = rating;
+    root["review_text"] = review_text;
+    root["arch_tag"] = click::Configuration().get_architecture();
+
+    // NOTE: "summary" is a required field, but we don't have one. Use "".
+    root["summary"] = "";
+    
+    qDebug() << "Rating" << package.package.name.c_str() << rating;
+
+    QSharedPointer<click::web::Response> response = client->call
+        (get_base_url() + click::REVIEWS_API_PATH, "POST", true,
+         headers, Json::FastWriter().write(root), click::web::CallParams());
+
+    QObject::connect(response.data(), &click::web::Response::finished,
+                [=](QString) {
+                   qDebug() << "Review submitted for:" << package.package.name.c_str();
+                });
+    QObject::connect(response.data(), &click::web::Response::error,
+                [=](QString) {
+                    qCritical() << "Network error submitting a reviews for:" << package.package.name.c_str();
+                });
+
+    return click::web::Cancellable(response);
+}
+
 std::string Reviews::get_base_url ()
 {
     const char *env_url = getenv(REVIEWS_BASE_URL_ENVVAR.c_str());

=== modified file 'scope/click/reviews.h'
--- scope/click/reviews.h	2014-04-09 18:08:56 +0000
+++ scope/click/reviews.h	2014-04-25 18:38:08 +0000
@@ -78,6 +78,9 @@
 
     click::web::Cancellable fetch_reviews (const std::string& package_name,
                                            std::function<void(ReviewList, Error)> callback);
+    click::web::Cancellable submit_review (const PackageDetails& package,
+                                           int rating,
+                                           const std::string& review_text);
 
     static std::string get_base_url ();
 };

=== modified file 'scope/click/webclient.cpp'
--- scope/click/webclient.cpp	2014-04-01 13:27:09 +0000
+++ scope/click/webclient.cpp	2014-04-25 18:38:08 +0000
@@ -117,7 +117,7 @@
                    [=, &request](const UbuntuOne::Token& token) {
             QString auth_header = token.signUrl(url.toString(),
                                                        method.c_str());
-            request.setRawHeader(AUTHORIZATION.c_str(), auth_header.toUtf8());
+            request.setRawHeader(AUTHORIZATION_HEADER.c_str(), auth_header.toUtf8());
             doConnect();
         });
         sc.connect(impl->sso.data(), &click::CredentialsService::credentialsNotFound,

=== modified file 'scope/click/webclient.h'
--- scope/click/webclient.h	2014-04-01 13:27:09 +0000
+++ scope/click/webclient.h	2014-04-25 18:38:08 +0000
@@ -49,7 +49,10 @@
 namespace web
 {
 
-const std::string AUTHORIZATION = "Authorization";
+const std::string AUTHORIZATION_HEADER = "Authorization";
+const std::string CONTENT_TYPE_HEADER = "Content-Type";
+
+const std::string CONTENT_TYPE_JSON = "application/json";
 
 class Client;
 

=== modified file 'scope/tests/test_reviews.cpp'
--- scope/tests/test_reviews.cpp	2014-03-25 18:49:16 +0000
+++ scope/tests/test_reviews.cpp	2014-04-25 18:38:08 +0000
@@ -219,6 +219,83 @@
     fetch_reviews_op.cancel();
 }
 
+TEST_F(ReviewsTest, testSubmitReviewIsCancellable)
+{
+    LifetimeHelper<click::network::Reply, MockNetworkReply> reply;
+    auto response = responseForReply(reply.asSharedPtr());
+
+    click::PackageDetails fake_details {
+        {
+            "ar.com.beuno.wheather-touch",
+            "\u1F4A9 Weather",
+            1.99,
+            "http://developer.staging.ubuntu.com/site_media/appmedia/2013/07/weather-icone-6797-64.png",
+            "https://public.apps.staging.ubuntu.com/download/ar.com.beuno/wheather-touch/ar.com.beuno.wheather-touch-0.2",
+            "0.2",
+        },
+        "\u1F4A9 Weather\nA weather application.",
+        "https://public.apps.staging.ubuntu.com/download/ar.com.beuno/wheather-touch/ar.com.beuno.wheather-touch-0.2",
+        3.5,
+        "these, are, key, words",
+        "tos",
+        "Proprietary",
+        "Beuno",
+        "sshot0",
+        {"sshot1", "sshot2"},
+        177582,
+        "0.2",
+        "None"
+    };
+    EXPECT_CALL(*clientPtr, callImpl(_, "POST", true, _, _, _))
+            .Times(1)
+            .WillOnce(Return(response));
+
+    auto submit_op = reviewsPtr->submit_review(fake_details, 3, "blah");
+    EXPECT_CALL(reply.instance, abort()).Times(1);
+    submit_op.cancel();
+}
+
+TEST_F(ReviewsTest, testSubmitReviewUtf8)
+{
+    LifetimeHelper<click::network::Reply, MockNetworkReply> reply;
+    auto response = responseForReply(reply.asSharedPtr());
+
+    click::PackageDetails fake_details {
+        {
+            "com.example.weather",
+            "Weather",
+            1.99,
+            "http://developer.staging.ubuntu.com/site_media/appmedia/2013/07/weather-icone-6797-64.png",
+            "https://public.apps.staging.ubuntu.com/download/com.example/weather/com.example.weather-0.2",
+            "0.2",
+        },
+        "Weather application.",
+        "https://public.apps.staging.ubuntu.com/download/com.example/weather/com.example.weather-0.2",
+        3.5,
+        "these, are, key, words",
+        "tos",
+        "Proprietary",
+        "Beuno",
+        "sshot0",
+        {"sshot1", "sshot2"},
+        177582,
+        "0.2",
+        "None"
+    };
+    std::string review_utf8 = "'\"小海嚴選";
+
+    // NOTE: gmock replaces the \" above as \\\" for HasSubstr(), so we have
+    // to manually copy the string into the expected result.
+    std::string expected_review_text = "\"review_text\":\"'\\\"小海嚴選\"";
+
+    EXPECT_CALL(*clientPtr, callImpl(_, "POST", true, _,
+                                     HasSubstr(expected_review_text), _))
+            .Times(1)
+            .WillOnce(Return(response));
+
+    auto submit_op = reviewsPtr->submit_review(fake_details, 3, review_utf8);
+}
+
 TEST_F(ReviewsTest, testGetBaseUrl)
 {
     const char *value = getenv(click::REVIEWS_BASE_URL_ENVVAR.c_str());

=== modified file 'scope/tests/test_webclient.cpp'
--- scope/tests/test_webclient.cpp	2014-04-01 13:27:09 +0000
+++ scope/tests/test_webclient.cpp	2014-04-25 18:38:08 +0000
@@ -46,7 +46,7 @@
 
 MATCHER_P(IsValidOAuthHeader, refOAuth, "")
 {
-    return arg.hasRawHeader("Authorization") && arg.rawHeader(click::web::AUTHORIZATION.c_str())
+    return arg.hasRawHeader(click::web::AUTHORIZATION_HEADER.c_str()) && arg.rawHeader(click::web::AUTHORIZATION_HEADER.c_str())
         .startsWith("OAuth ");
 }