Merge lp:~dobey/unity-scope-click/require-both into lp:unity-scope-click

[dobey]

Don't specify that only 'rating' as required for reviews.

[Antti Kaijanmäki (kaijanmaki)]

LGTM.

review: Approve

[Paweł Stołowski (stolowski)]

30 + Review existing_review;
31 if (has_reviewed) {

Review will not get initialized here and if has_reviewed is false it may have random id in review.id != 0 check inside createRatingWidget. Please change it to "Review existing_review {}" or initialize it to sane defaults manually.

Otherwise looks good.

review: Needs Fixing

=== modified file 'libclickscope/click/preview.cpp'
--- libclickscope/click/preview.cpp	2016-05-25 16:20:04 +0000
+++ libclickscope/click/preview.cpp	2016-05-31 17:24:38 +0000
@@ -848,6 +848,21 @@
     return future.get();
 }
 
+scopes::PreviewWidget InstalledPreview::createRatingWidget(const click::Review& review) const
+{
+    scopes::PreviewWidget rating("rating", "rating-input");
+
+    if (review.id != 0) {
+        qDebug() << "Review for current user already exists, review id:" << review.id;
+        rating = scopes::PreviewWidget(std::to_string(review.id), "rating-edit"); // pass review id via widget id
+        rating.add_attribute_value("review", scopes::Variant(review.review_text));
+        rating.add_attribute_value("rating", scopes::Variant(review.rating));
+        rating.add_attribute_value("author", scopes::Variant(review.reviewer_name));
+    }
+
+    return rating;
+}
+
 void InstalledPreview::run(unity::scopes::PreviewReplyProxy const& reply)
 {
     const bool force_cache = (metadata.internet_connectivity() == scopes::QueryMetadata::ConnectivityStatus::Disconnected);
@@ -935,21 +950,13 @@
                     scopes::PreviewWidgetList review_input;
                     bool has_reviewed = reviews.size() > 0 && reviews.front().reviewer_username == userid;
 
+                    Review existing_review;
+                    existing_review.id = 0;
                     if (has_reviewed) {
-                        auto existing_review = reviews.front();
+                        existing_review = reviews.front();
                         reviews.pop_front();
-                        qDebug() << "Review for current user already exists, review id:" << existing_review.id;
-                        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 {
-                        scopes::PreviewWidget rating("rating", "rating-input");
-                        rating.add_attribute_value("required", scopes::Variant("rating"));
-                        review_input.push_back(rating);
                     }
+                    review_input.push_back(createRatingWidget(existing_review));
                     cachedWidgets.push(review_input);
                     cachedWidgets.layout.appendToColumn(cachedWidgets.layout.singleColumn.column1, review_input);
                     cachedWidgets.layout.appendToColumn(cachedWidgets.layout.twoColumns.column1, review_input);

=== modified file 'libclickscope/click/preview.h'
--- libclickscope/click/preview.h	2016-05-25 16:20:04 +0000
+++ libclickscope/click/preview.h	2016-05-31 17:24:38 +0000
@@ -261,11 +261,12 @@
 
     void run(unity::scopes::PreviewReplyProxy const& reply) override;
 
-protected:
     void getApplicationUri(const Manifest& manifest, std::function<void(const std::string&)> callback);
     std::string get_consumer_key();
     scopes::PreviewWidgetList createButtons(const std::string& uri,
                                             const click::Manifest& manifest);
+    scopes::PreviewWidget createRatingWidget(const click::Review& review) const;
+
 private:
     scopes::ActionMetadata metadata;
     CachedPreviewWidgets cachedWidgets;

=== modified file 'libclickscope/tests/test_preview.cpp'
--- libclickscope/tests/test_preview.cpp	2016-04-27 14:06:29 +0000
+++ libclickscope/tests/test_preview.cpp	2016-05-31 17:24:38 +0000
@@ -584,6 +584,25 @@
     ASSERT_EQ(get_action_from_widgets(widgets, 0, 1), "uninstall_click");
 }
 
+TEST_F(InstalledPreviewTest, testReviewWidgetIsNew) {
+    click::InstalledPreview preview(result, metadata, client, pay_package, depts);
+    click::Review existing_review;
+    existing_review.id = 0;
+    auto widget = preview.createRatingWidget(existing_review);
+    ASSERT_EQ(widget.widget_type(), "rating-input");
+}
+
+TEST_F(InstalledPreviewTest, testReviewWidgetIsEdit) {
+    click::InstalledPreview preview(result, metadata, client, pay_package, depts);
+    click::Review existing_review;
+    existing_review.id = 123456789;
+    existing_review.rating = 2.625f;
+    existing_review.review_text = "Mediocre at best.";
+    existing_review.reviewer_name = "reviewbot";
+    auto widget = preview.createRatingWidget(existing_review);
+    ASSERT_EQ(widget.widget_type(), "rating-edit");
+}
+
 class FakeCancelPurchasePreview : public click::CancelPurchasePreview  {
 public:
     FakeCancelPurchasePreview(const unity::scopes::Result& result, bool installed)