Merge lp:~stolowski/unity-scope-click/edit-reviews-15-04 into lp:unity-scope-click/touch-15-04

[Paweł Stołowski]

Support for review editing.

Support for review editing. Changed ReviewList type definition to use std::list instead of vector for better efficiency when moving own review to the front of the list.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:320
http://jenkins.qa.ubuntu.com/job/unity-scope-click-ci/592/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scope-click-wily-amd64-ci/4/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scope-click-wily-armhf-ci/4/console

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

review: Needs Fixing (continuous-integration)

[Alejandro J. Cura (alecu)]

Branch looks very good, just tried it on nexus 4 with devel-proposed.

One style comment: the method named "sort" is not really sorting the reviews, so it should be called "bring_to_front" or something similar.
Please also add a few unit tests for it.

review: Needs Fixing

[lp:~stolowski/unity-scope-click/edit-reviews-15-04]

321. By Paweł Stołowski on 2015-05-15

Renamed sort to bring_to_front and added unit tests for it

[Paweł Stołowski (stolowski)]

> Branch looks very good, just tried it on nexus 4 with devel-proposed.
>
> One style comment: the method named "sort" is not really sorting the reviews,
> so it should be called "bring_to_front" or something similar.
> Please also add a few unit tests for it.

Ok, renamed and added unit test for it.

[Alejandro J. Cura (alecu)]

Looks great, thanks!

review: Approve

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:321
http://jenkins.qa.ubuntu.com/job/unity-scope-click-ci/598/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scope-click-wily-amd64-ci/8/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scope-click-wily-armhf-ci/8/console

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

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:321
http://jenkins.qa.ubuntu.com/job/unity-scope-click-ci/599/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scope-click-wily-amd64-ci/9/console
    None: http://jenkins.qa.ubuntu.com/job/unity-scope-click-wily-armhf-ci/9/console

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

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:321
http://jenkins.qa.ubuntu.com/job/unity-scope-click-ci/600/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scope-click-wily-amd64-ci/10/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scope-click-wily-armhf-ci/10/console

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

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:321
http://jenkins.qa.ubuntu.com/job/unity-scope-click-touch-15-04-ci/1/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scope-click-touch-15-04-vivid-amd64-ci/1/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scope-click-touch-15-04-vivid-armhf-ci/1/console

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

review: Needs Fixing (continuous-integration)

=== modified file 'CMakeLists.txt'
--- CMakeLists.txt	2015-03-10 22:26:39 +0000
+++ CMakeLists.txt	2015-05-15 10:38:40 +0000
@@ -25,7 +25,7 @@
 pkg_check_modules(UNITY_SCOPES REQUIRED libunity-scopes>=0.6.7 libunity-api>=0.1.3)
 add_definitions(${UNITY_SCOPES_CFLAGS} ${UNITY_SCOPES_CFLAGS_OTHER})
 
-pkg_check_modules(UBUNTUONE REQUIRED ubuntuoneauth-2.0)
+pkg_check_modules(UBUNTUONE REQUIRED ubuntuoneauth-2.0>=15.04)
 add_definitions(${UBUNTUONE_CFLAGS} ${UBUNTUONE_CFLAGS_OTHER})
 
 pkg_check_modules(UBUNTU_DOWNLOAD_MANAGER_CLIENT REQUIRED ubuntu-download-manager-client)

=== modified file 'debian/control'
--- debian/control	2015-03-09 17:00:08 +0000
+++ debian/control	2015-05-15 10:38:40 +0000
@@ -15,7 +15,7 @@
                libpay2-dev,
                libubuntu-download-manager-client-dev (>= 0.3+14.10.20140430-0ubuntu1),
                libubuntu-download-manager-common-dev (>= 0.3+14.10.20140430-0ubuntu1),
-               libubuntuoneauth-2.0-dev,
+               libubuntuoneauth-2.0-dev (>= 15.04),
                libunity-api-dev (>= 7.80.7),
                libunity-scopes-dev (>= 0.6.7~),
                libgsettings-qt-dev,

=== modified file 'libclickscope/click/preview.cpp'
--- libclickscope/click/preview.cpp	2015-03-26 18:49:42 +0000
+++ libclickscope/click/preview.cpp	2015-05-15 10:38:40 +0000
@@ -36,6 +36,7 @@
 #include <click/dbus_constants.h>
 #include <click/departments-db.h>
 #include <click/utils.h>
+#include <click/smartconnect.h>
 
 #include <boost/algorithm/string/replace.hpp>
 
@@ -642,21 +643,54 @@
 {
 }
 
+std::string InstalledPreview::get_consumer_key()
+{
+    std::promise<std::string> promise;
+    auto future = promise.get_future();
+    QSharedPointer<click::CredentialsService> sso;
+
+    qt::core::world::enter_with_task([this, &sso, &promise]() {
+            sso.reset(new click::CredentialsService());
+
+            QObject::connect(sso.data(), &click::CredentialsService::credentialsFound,
+                    [&promise, &sso](const UbuntuOne::Token& token) {
+                    qDebug() << "Credentials found";
+                    sso.clear();
+                    promise.set_value(token.consumerKey().toStdString());
+                });
+            QObject::connect(sso.data(), &click::CredentialsService::credentialsNotFound,
+                    [&promise, &sso]() {
+                    qDebug() << "No credentials found";
+                    sso.clear();
+                    promise.set_value("");
+                    });
+
+            sso->getCredentials();
+            qDebug() << "getCredentials finished";
+        });
+    return future.get();
+}
+
 void InstalledPreview::run(unity::scopes::PreviewReplyProxy const& reply)
 {
     // Check if the user is submitting a rating, so we can submit it.
     Review review;
     review.rating = 0;
+    std::string widget_id;
     // We use a try/catch here, as scope_data() can be a dict, but not have
     // the values we need, which will result in an exception thrown. 
     try {
         auto metadict = metadata.scope_data().get_dict();
         review.rating = metadict["rating"].get_int();
         review.review_text = metadict["review"].get_string();
+        widget_id = metadict["widget_id"].get_string();
     } catch(...) {
         // Do nothing as we are not submitting a review.
     }
 
+    auto userid = get_consumer_key();
+
+    //
     // Get the click manifest.
     Manifest manifest;
     std::promise<Manifest> manifest_promise;
@@ -682,35 +716,68 @@
         if (review.rating > 0) {
             std::promise<bool> submit_promise;
             std::future<bool> submit_future = submit_promise.get_future();
-            qt::core::world::enter_with_task([this, review, &submit_promise]() {
+            qt::core::world::enter_with_task([this, review, &submit_promise, widget_id]() mutable {
                     QSharedPointer<click::CredentialsService> sso(new click::CredentialsService());
                     client->setCredentialsService(sso);
-                    submit_operation = reviews->submit_review(review,
+                    if (widget_id == "rating") {
+                        submit_operation = reviews->submit_review(review,
                                                               [&submit_promise](click::Reviews::Error){
                                                                   // TODO: Need to handle errors properly.
                                                                   submit_promise.set_value(true);
                                                               });
-                });
+
+                    } else {
+                        try {
+                            review.id = std::stoul(widget_id);
+                            qDebug() << "Updating review" << review.id << "with '" << QString::fromStdString(review.review_text) << "'";
+                            submit_operation = reviews->edit_review(review,
+                                                                [&submit_promise](click::Reviews::Error){
+                                                                    // TODO: Need to handle errors properly.
+                                                                    submit_promise.set_value(true);
+                                                                });
+                        } catch (const std::exception &e) {
+                            qWarning() << "Failed to update review:" << QString::fromStdString(e.what()) << " review widget:" << QString::fromStdString(widget_id);
+                            submit_promise.set_value(false);
+                        }
+                }
+            });
             submit_future.get();
         }
     }
-    getApplicationUri(manifest, [this, reply, manifest, app_name, &review](const std::string& uri) {
-            populateDetails([this, reply, uri, manifest, app_name, &review](const PackageDetails &details){
+    getApplicationUri(manifest, [this, reply, manifest, app_name, &review, userid](const std::string& uri) {
+            populateDetails([this, reply, uri, manifest, app_name](const PackageDetails &details){
                 store_department(details);
                 pushPackagePreviewWidgets(reply, details, createButtons(uri, manifest));
-
-                if (review.rating == 0 && manifest.removable) {
-                    scopes::PreviewWidgetList review_input;
-                    scopes::PreviewWidget rating("rating", "rating-input");
-                    rating.add_attribute_value("required", scopes::Variant("rating"));
-                    review_input.push_back(rating);
-                    reply->push(review_input);
-                }
             },
-            [this, reply](const ReviewList& reviewlist,
+            [this, reply, &review, manifest, userid](const ReviewList& reviewlist,
                           click::Reviews::Error error) {
+                auto reviews = bring_to_front(reviewlist, userid);
+                scopes::PreviewWidgetList review_input;
+                bool has_reviewed = reviews.size() > 0 && reviews.front().reviewer_username == userid;
+
+                if (has_reviewed) {
+                    auto existing_review = reviews.front();
+                    reviews.pop_front();
+                    qDebug() << "Review for current user already exists, review id:" << existing_review.id;
+                    if (manifest.removable) {
+                        scopes::PreviewWidget rating(std::to_string(existing_review.id), "rating-edit"); // pass review id via widget id
+                        rating.add_attribute_value("required", scopes::Variant("rating"));
+                        rating.add_attribute_value("review", scopes::Variant(existing_review.review_text));
+                        rating.add_attribute_value("rating", scopes::Variant(existing_review.rating));
+                        rating.add_attribute_value("author", scopes::Variant(existing_review.reviewer_name));
+                        review_input.push_back(rating);
+                    }
+                } else {
+                    if (review.rating == 0 && manifest.removable) {
+                        scopes::PreviewWidget rating("rating", "rating-input");
+                        rating.add_attribute_value("required", scopes::Variant("rating"));
+                        review_input.push_back(rating);
+                    }
+                }
+                reply->push(review_input);
+
                 if (error == click::Reviews::Error::NoError) {
-                    reply->push(reviewsWidgets(reviewlist));
+                    reply->push(reviewsWidgets(reviews));
                 } else {
                     qDebug() << "There was an error getting reviews for:" << result["name"].get_string().c_str();
                 }

=== modified file 'libclickscope/click/preview.h'
--- libclickscope/click/preview.h	2015-01-26 15:14:10 +0000
+++ libclickscope/click/preview.h	2015-05-15 10:38:40 +0000
@@ -208,6 +208,7 @@
 
 protected:
     void getApplicationUri(const Manifest& manifest, std::function<void(const std::string&)> callback);
+    std::string get_consumer_key();
 
 private:
     static scopes::PreviewWidgetList createButtons(const std::string& uri,

=== modified file 'libclickscope/click/reviews.cpp'
--- libclickscope/click/reviews.cpp	2014-05-26 14:02:45 +0000
+++ libclickscope/click/reviews.cpp	2015-05-15 10:38:40 +0000
@@ -28,6 +28,7 @@
  */
 
 #include <cstdlib>
+#include <algorithm>
 
 #include <boost/property_tree/ptree.hpp>
 #include <boost/property_tree/json_parser.hpp>
@@ -96,6 +97,25 @@
     return reviews;
 }
 
+ReviewList bring_to_front (const ReviewList& reviews, const std::string& userid)
+{
+    if (userid.size() == 0)
+    {
+        return reviews;
+    }
+    auto new_reviews = reviews;
+    auto it = std::find_if(new_reviews.begin(), new_reviews.end(), [userid](const Review& review) {
+                return review.reviewer_username == userid;
+            });
+    if (it != new_reviews.end() && it != new_reviews.begin()) {
+        // move own review to the front
+        auto const review = *it;
+        new_reviews.erase(it);
+        new_reviews.push_front(review);
+    }
+    return new_reviews;
+}
+
 Reviews::Reviews (const QSharedPointer<click::web::Client>& client)
     : client(client)
 {
@@ -174,6 +194,38 @@
     return click::web::Cancellable(response);
 }
 
+click::web::Cancellable Reviews::edit_review (const Review& review,
+                                                std::function<void(Error)> callback)
+{
+    std::map<std::string, std::string> headers({
+            {click::web::CONTENT_TYPE_HEADER, click::web::CONTENT_TYPE_JSON},
+                });
+    Json::Value root(Json::ValueType::objectValue);
+    root["rating"] = review.rating;
+    root["review_text"] = review.review_text;
+    // NOTE: "summary" is a required field, but we don't have one. Use "".
+    root["summary"] = "Review";
+
+    qDebug() << "Rating" << review.package_name.c_str() << review.rating;
+
+    QSharedPointer<click::web::Response> response = client->call
+        (get_base_url() + click::REVIEWS_API_PATH + std::to_string(review.id) + "/", "PUT", true,
+         headers, Json::FastWriter().write(root), click::web::CallParams());
+
+    QObject::connect(response.data(), &click::web::Response::finished,
+                [=](QString) {
+                   qDebug() << "Review updated for:" << review.package_name.c_str();
+                   callback(Error::NoError);
+                });
+    QObject::connect(response.data(), &click::web::Response::error,
+                [=](QString) {
+                    qCritical() << "Network error updating a review for:" << review.package_name.c_str();
+                    callback(Error::NetworkError);
+                });
+
+    return click::web::Cancellable(response);
+}
+
 std::string Reviews::get_base_url ()
 {
     const char *env_url = getenv(REVIEWS_BASE_URL_ENVVAR.c_str());

=== modified file 'libclickscope/click/reviews.h'
--- libclickscope/click/reviews.h	2015-01-13 22:17:36 +0000
+++ libclickscope/click/reviews.h	2015-05-15 10:38:40 +0000
@@ -32,7 +32,7 @@
 
 #include <functional>
 #include <string>
-#include <vector>
+#include <list>
 
 #include <click/webclient.h>
 
@@ -62,9 +62,10 @@
     std::string reviewer_username;
 };
 
-typedef std::vector<Review> ReviewList;
+typedef std::list<Review> ReviewList;
 
 ReviewList review_list_from_json (const std::string& json);
+ReviewList bring_to_front (const ReviewList& reviews, const std::string& userid);
 
 class Reviews
 {
@@ -80,6 +81,8 @@
                                            std::function<void(ReviewList, Error)> callback);
     click::web::Cancellable submit_review (const Review& review,
                                            std::function<void(Error)> callback);
+    click::web::Cancellable edit_review (const Review& review,
+                                           std::function<void(Error)> callback);
 
     static std::string get_base_url ();
 };

=== modified file 'libclickscope/tests/test_reviews.cpp'
--- libclickscope/tests/test_reviews.cpp	2014-05-26 14:27:31 +0000
+++ libclickscope/tests/test_reviews.cpp	2015-05-15 10:38:40 +0000
@@ -63,6 +63,80 @@
 };
 }
 
+TEST_F(ReviewsTest, bringToFrontUserMatches)
+{
+    click::Review r1;
+    r1.id = 1;
+    r1.reviewer_username = "user1";
+
+    click::Review r2;
+    r2.id = 2;
+    r2.reviewer_username = "user2";
+
+    click::Review r3;
+    r3.id = 3;
+    r3.reviewer_username = "user3";
+
+    click::ReviewList reviews {r1, r2, r3};
+
+    auto newReviews = bring_to_front(reviews, "user2");
+    EXPECT_EQ(newReviews.size(), 3);
+    auto it = newReviews.begin();
+    EXPECT_EQ(2, (*it).id);
+    ++it;
+    EXPECT_EQ(1, (*it).id);
+    ++it;
+    EXPECT_EQ(3, (*it).id);
+}
+
+TEST_F(ReviewsTest, bringToFrontUserMatchesFirstElement)
+{
+    click::Review r1;
+    r1.id = 1;
+    r1.reviewer_username = "user1";
+
+    click::Review r2;
+    r2.id = 2;
+    r2.reviewer_username = "user2";
+
+    click::ReviewList reviews {r1, r2};
+
+    auto newReviews = bring_to_front(reviews, "user1");
+    EXPECT_EQ(newReviews.size(), 2);
+    auto it = newReviews.begin();
+    EXPECT_EQ(1, (*it).id);
+    ++it;
+    EXPECT_EQ(2, (*it).id);
+}
+
+TEST_F(ReviewsTest, bringToFrontEmptyList)
+{
+    click::ReviewList reviews;
+
+    auto newReviews = bring_to_front(reviews, "user1");
+    EXPECT_EQ(newReviews.size(), 0);
+}
+
+TEST_F(ReviewsTest, bringToFrontUserDoesntMatch)
+{
+    click::Review r1;
+    r1.id = 1;
+    r1.reviewer_username = "user1";
+
+    click::Review r2;
+    r2.id = 2;
+    r2.reviewer_username = "user2";
+
+    click::ReviewList reviews {r1, r2};
+
+    auto newReviews = bring_to_front(reviews, "user0");
+    EXPECT_EQ(newReviews.size(), 2);
+    auto it = newReviews.begin();
+    EXPECT_EQ(1, (*it).id);
+    ++it;
+    EXPECT_EQ(2, (*it).id);
+}
+
 TEST_F(ReviewsTest, testFetchReviewsCallsWebservice)
 {
     LifetimeHelper<click::network::Reply, MockNetworkReply> reply;
@@ -314,6 +388,49 @@
     }
 }
 
+TEST_F(ReviewsTest, testEditReviewUrlHasReviewId)
+{
+    LifetimeHelper<click::network::Reply, MockNetworkReply> reply;
+    auto response = responseForReply(reply.asSharedPtr());
+
+    click::Review review;
+    review.id = 1234;
+    review.rating = 4;
+    review.review_text = "A review";
+    review.package_name = "com.example.test";
+    review.package_version = "0.1";
+
+    EXPECT_CALL(*clientPtr, callImpl(HasSubstr("/1234/"), "PUT", true, _, _, _))
+            .Times(1)
+            .WillOnce(Return(response));
+
+    auto submit_op = reviewsPtr->edit_review(review,
+                                               [](click::Reviews::Error){});
+}
+
+TEST_F(ReviewsTest, testEditReviewIsCancellable)
+{
+    LifetimeHelper<click::network::Reply, MockNetworkReply> reply;
+    auto response = responseForReply(reply.asSharedPtr());
+
+    click::Review review;
+    review.id = 1234;
+    review.rating = 4;
+    review.review_text = "A review";
+    review.package_name = "com.example.test";
+    review.package_version = "0.1";
+
+    EXPECT_CALL(*clientPtr, callImpl(_, "PUT", true, _, _, _))
+            .Times(1)
+            .WillOnce(Return(response));
+
+    auto submit_op = reviewsPtr->edit_review(review,
+                                               [](click::Reviews::Error){});
+    EXPECT_CALL(reply.instance, abort()).Times(1);
+    submit_op.cancel();
+}
+
+
 TEST_F(ReviewsTest, testGetBaseUrl)
 {
     const char *value = getenv(click::REVIEWS_BASE_URL_ENVVAR.c_str());

=== modified file 'scope/clickapps/apps-scope.cpp'
--- scope/clickapps/apps-scope.cpp	2014-08-13 14:43:32 +0000
+++ scope/clickapps/apps-scope.cpp	2015-05-15 10:38:40 +0000
@@ -105,7 +105,7 @@
 
 
 unity::scopes::ActivationQueryBase::UPtr click::Scope::perform_action(unity::scopes::Result const& result, unity::scopes::ActionMetadata const& metadata,
-        std::string const& /* widget_id */, std::string const& action_id)
+        std::string const& widget_id, std::string const& action_id)
 {
     if (action_id == click::Preview::Actions::CONFIRM_UNINSTALL) {
         auto response = unity::scopes::ActivationResponse(unity::scopes::ActivationResponse::ShowDash);
@@ -134,6 +134,7 @@
         activation->setHint("review", scopes::Variant(review_text));
         activation->setHint(click::Preview::Actions::RATED,
                             scopes::Variant(true));
+        activation->setHint("widget_id", scopes::Variant(widget_id));
         activation->setStatus(scopes::ActivationResponse::Status::ShowPreview);
     }
     return scopes::ActivationQueryBase::UPtr(activation);