Merge lp:~charlesk/pay-service/fix-status-lookups into lp:pay-service/15.04

[Charles Kerr]

Fix the libpay cold cache bug.

Fix the libpay cold cache bug.

This has the same intent as lp:~dobey/pay-service/fix-refunds but uses a different approach.

* itemStatus() has a sibling bug; fix it too

* implement both itemStatus() and refundStatus() as calls to getItem(). Since getItem() is already blocking, the new status functions have trivial implementation

* adds 'cold cache' tests

* remove unused itemStatusCache

[lp:~charlesk/pay-service/fix-status-lookups]

150. By Charles Kerr on 2015-10-06

remove unused copypasta from new tests

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:150
http://jenkins.qa.ubuntu.com/job/pay-service-15.04-ci/56/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/pay-service-15.04-vivid-amd64-ci/56
    SUCCESS: http://jenkins.qa.ubuntu.com/job/pay-service-15.04-vivid-armhf-ci/56
        deb: http://jenkins.qa.ubuntu.com/job/pay-service-15.04-vivid-armhf-ci/56/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/pay-service-15.04-ci/56/rebuild

review: Approve (continuous-integration)

[lp:~charlesk/pay-service/fix-status-lookups]

151. By Charles Kerr on 2015-10-06

allow refundable_until as an app property in the store mock's AddStore()

152. By Charles Kerr on 2015-10-06

in libpay package tests' setup, populate the mock service with apps

153. By Charles Kerr on 2015-10-06

add 'cache miss' test for libpay packages' item and refund statuses

154. By Charles Kerr on 2015-10-06

remove unused variables from LibpayPackageTest tests

155. By Charles Kerr on 2015-10-06

fix typo in dbusmock's refundable_until test

156. By Charles Kerr on 2015-10-06

add item status & refund status checks to LibpayPackageTests::PurchaseItem

157. By Charles Kerr on 2015-10-06

re-fix the store mock's refund() test for refundable_until... r155 fixed the typo not the property name

158. By Charles Kerr on 2015-10-06

add item status & refund status checks to LibpayPackageTests::RefundItem

159. By Charles Kerr on 2015-10-06

copyediting: fix comments

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:158
http://jenkins.qa.ubuntu.com/job/pay-service-15.04-ci/57/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/pay-service-15.04-vivid-amd64-ci/57
    SUCCESS: http://jenkins.qa.ubuntu.com/job/pay-service-15.04-vivid-armhf-ci/57
        deb: http://jenkins.qa.ubuntu.com/job/pay-service-15.04-vivid-armhf-ci/57/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/pay-service-15.04-ci/57/rebuild

review: Approve (continuous-integration)

[lp:~charlesk/pay-service/fix-status-lookups]

160. By Charles Kerr on 2015-10-06

remove 'approved_app' test -- app purchases don't have an 'approved' status

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:159
http://jenkins.qa.ubuntu.com/job/pay-service-15.04-ci/58/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/pay-service-15.04-vivid-amd64-ci/58
    SUCCESS: http://jenkins.qa.ubuntu.com/job/pay-service-15.04-vivid-armhf-ci/58
        deb: http://jenkins.qa.ubuntu.com/job/pay-service-15.04-vivid-armhf-ci/58/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/pay-service-15.04-ci/58/rebuild

review: Approve (continuous-integration)

[dobey (dobey)]

review: Approve

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:160
http://jenkins.qa.ubuntu.com/job/pay-service-15.04-ci/59/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/pay-service-15.04-vivid-amd64-ci/59
    SUCCESS: http://jenkins.qa.ubuntu.com/job/pay-service-15.04-vivid-armhf-ci/59
        deb: http://jenkins.qa.ubuntu.com/job/pay-service-15.04-vivid-armhf-ci/59/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/pay-service-15.04-ci/59/rebuild

review: Approve (continuous-integration)

=== modified file 'libpay/internal/item.h'
--- libpay/internal/item.h	2015-10-06 17:18:08 +0000
+++ libpay/internal/item.h	2015-10-06 17:18:08 +0000
@@ -40,6 +40,7 @@
     uint64_t m_purchase_id = 0;
     time_t m_completed_timestamp = 0;
     time_t m_acknowledged_timestamp = 0;
+    time_t m_refundable_until = 0;
 
 public:
 
@@ -63,6 +64,7 @@
     PayPackageItemStatus status() const {return m_status;}
     time_t completed_timestamp() const {return m_completed_timestamp;}
     time_t acknowledged_timestamp() const {return m_acknowledged_timestamp;}
+    time_t refundable_until() const {return m_refundable_until;}
     const std::string& title() const {return m_title;}
     PayItemType type() const {return m_type;}
 
@@ -74,6 +76,7 @@
     void set_status(PayPackageItemStatus val) {m_status = val;}
     void set_completed_timestamp(time_t val) {m_completed_timestamp = val;}
     void set_acknowledged_timestamp(time_t val) {m_acknowledged_timestamp = val;}
+    void set_refundable_until(time_t val) {m_refundable_until = val;}
     void set_title(const std::string& val) {m_title = val;}
     void set_type(PayItemType val) {m_type = val;}
 

=== modified file 'libpay/internal/package.cpp'
--- libpay/internal/package.cpp	2015-10-06 17:18:08 +0000
+++ libpay/internal/package.cpp	2015-10-06 17:18:08 +0000
@@ -34,16 +34,6 @@
     : id(packageid)
     , thread([]{}, [this]{storeProxy.reset();})
 {
-    // when item statuses change, update our internal cache
-    statusChanged.connect([this](const std::string& sku,
-                                 PayPackageItemStatus status,
-                                 uint64_t refundable_until)
-    {
-        g_debug("Updating itemStatusCache for '%s', timeout is: %lld",
-                sku.c_str(), refundable_until);
-        itemStatusCache[sku] = std::make_pair(status, refundable_until);
-    });
-
     /* Fire up a glib thread to create the proxies.
        Block on it here so the proxies are ready before this ctor returns */
     const auto errorStr = thread.executeOnThread<std::string>([this]()
@@ -90,28 +80,21 @@
 PayPackageItemStatus
 Package::itemStatus (const std::string& sku) noexcept
 {
-    try
-    {
-        return itemStatusCache[sku].first;
-    }
-    catch (std::out_of_range& /*range*/)
-    {
-        return PAY_PACKAGE_ITEM_STATUS_UNKNOWN;
-    }
+    const auto item = getItem(sku);
+
+    return item
+        ? item->status()
+        : PAY_PACKAGE_ITEM_STATUS_UNKNOWN;
 }
 
 PayPackageRefundStatus
 Package::refundStatus (const std::string& sku) noexcept
 {
-    try
-    {
-        auto entry = itemStatusCache[sku];
-        return calcRefundStatus(entry.first, entry.second);
-    }
-    catch (std::out_of_range& /*range*/)
-    {
-        return PAY_PACKAGE_REFUND_STATUS_NOT_REFUNDABLE;
-    }
+    const auto item = getItem(sku);
+
+    return item
+        ? calcRefundStatus(item->status(), item->refundable_until())
+        : PAY_PACKAGE_REFUND_STATUS_NOT_REFUNDABLE;
 }
 
 PayPackageRefundStatus
@@ -263,6 +246,10 @@
         {
             item->set_completed_timestamp(g_variant_get_uint64(value));
         }
+        else if (!g_strcmp0(key, "refundable_until"))
+        {
+            item->set_refundable_until(g_variant_get_uint64(value));
+        }
         else if (!g_strcmp0(key, "purchase_id"))
         {
             item->set_purchase_id(g_variant_get_uint64(value));
@@ -546,40 +533,6 @@
     return ok;
 }
 
-template <typename BusProxy,
-          void (*startFunc)(BusProxy*, const gchar*, GCancellable*, GAsyncReadyCallback, gpointer),
-          gboolean (*finishFunc)(BusProxy*, GAsyncResult*, GError**)>
-bool Package::startBase (const std::shared_ptr<BusProxy>& bus_proxy, const std::string& sku) noexcept
-{
-    auto async_ready = [](GObject * obj, GAsyncResult * res, gpointer user_data) -> void
-    {
-        auto prom = static_cast<std::promise<bool>*>(user_data);
-        GError* error = nullptr;
-        finishFunc(reinterpret_cast<BusProxy*>(obj), res, &error);
-        if ((error != nullptr) && !g_error_matches(error, G_IO_ERROR, G_IO_ERROR_CANCELLED))
-        {
-            std::cerr << "Error from service: " << error->message << std::endl;
-        }
-        prom->set_value(error == nullptr);
-        g_clear_error(&error);
-    };
-
-    std::promise<bool> promise;
-    thread.executeOnThread([this, bus_proxy, sku, &async_ready, &promise]()
-    {
-        startFunc(bus_proxy.get(),
-        sku.c_str(),
-        thread.getCancellable().get(), // GCancellable
-        async_ready,
-        &promise);
-    });
-
-    auto future = promise.get_future();
-    future.wait();
-    auto call_succeeded = future.get();
-    return call_succeeded;
-}
-
 } // namespace Internal
 
 } // namespace Pay

=== modified file 'libpay/internal/package.h'
--- libpay/internal/package.h	2015-10-06 17:18:08 +0000
+++ libpay/internal/package.h	2015-10-06 17:18:08 +0000
@@ -50,7 +50,6 @@
     std::map <std::pair<PayPackageRefundObserver, void*>, core::ScopedConnection> refundObservers;
 
     core::Signal<std::string, PayPackageItemStatus, uint64_t> statusChanged;
-    std::map <std::string, std::pair<PayPackageItemStatus, uint64_t>> itemStatusCache;
     void updateStatus(const std::string& sku, PayPackageItemStatus);
 
     GLib::ContextThread thread;
@@ -61,11 +60,6 @@
     template<typename Collection>
     bool removeObserver(Collection& collection, const typename Collection::key_type& key);
 
-    template <typename BusProxy,
-              void (*startFunc)(BusProxy*, const gchar*, GCancellable*, GAsyncReadyCallback, gpointer),
-              gboolean (*finishFunc)(BusProxy*, GAsyncResult*, GError**)>
-    bool startBase (const std::shared_ptr<BusProxy>& bus_proxy, const std::string& sku) noexcept;
-
     template<typename BusProxy,
              gboolean (*finish_func)(BusProxy*, GVariant**, GAsyncResult*, GError**)>
     bool startStoreAction(const std::shared_ptr<BusProxy>& bus_proxy,

=== modified file 'tests/com_canonical_pay_store.py'
--- tests/com_canonical_pay_store.py	2015-10-06 17:18:08 +0000
+++ tests/com_canonical_pay_store.py	2015-10-06 17:18:08 +0000
@@ -62,6 +62,7 @@
         'description': dbus.String('The is a default item'),
         'price': dbus.String('$1'),
         'purchase_id': dbus.UInt64(0.0),
+        'refundable_until': dbus.UInt64(0.0),
         'sku': dbus.String('default_item'),
         'state': dbus.String('available'),
         'type': dbus.String('unlockable'),
@@ -170,7 +171,7 @@
     try:
         item = store.items[sku]
         if (item.bus_properties['state'] == 'purchased' and
-            item.bus_properties['refund_tiemout'] > dbus.UInt64(time.time())):
+            item.bus_properties['refundable_until'] > dbus.UInt64(time.time())):
             del store.items[sku]
             return dbus.Dictionary({
                 'state': 'available',

=== modified file 'tests/libpay-package-tests.cpp'
--- tests/libpay-package-tests.cpp	2015-10-06 17:18:08 +0000
+++ tests/libpay-package-tests.cpp	2015-10-06 17:18:08 +0000
@@ -69,8 +69,33 @@
 
         wait_for_store_service();
 
+        const guint64 now = time(nullptr);
+        const struct {
+            const char * sku;
+            const char * state;
+            guint64 refundable_until;
+        } prefab_apps[] = {
+            { "available_app",       "available", 0 },           // not purchased
+            { "newly_purchased_app", "purchased", now+(60*14) }, // purchased, refund window open
+            { "old_purchased_app",   "purchased", now-(60*30) }  // purchased, refund window closed
+        };
         GVariantBuilder b;
         g_variant_builder_init(&b, G_VARIANT_TYPE("aa{sv}"));
+        for (const auto& app : prefab_apps) {
+            GVariantBuilder app_props;
+            g_variant_builder_init(&app_props, G_VARIANT_TYPE_VARDICT);
+            const struct {
+                const char* key;
+                GVariant* value;
+            } entries[] = {
+                { "sku", g_variant_new_string(app.sku) },
+                { "state", g_variant_new_string(app.state) },
+                { "refundable_until", g_variant_new_uint64(app.refundable_until) }
+            };
+            for (const auto& entry : entries) 
+                g_variant_builder_add(&app_props, "{sv}", entry.key, entry.value);
+            g_variant_builder_add_value(&b, g_variant_builder_end(&app_props));
+        }
         auto props = g_variant_builder_end(&b);
 
         GVariant* args[] = { g_variant_new_string("click-scope"), props };
@@ -135,15 +160,17 @@
 
 TEST_F(LibpayPackageTests, PurchaseItem)
 {
-    GError* error = nullptr;
-    guint callcount = 0;
     auto package = pay_package_new("click-scope");
+    const char* sku {"available_app"};
+
+    // pre-purchase tests
+    EXPECT_EQ(PAY_PACKAGE_ITEM_STATUS_NOT_PURCHASED, pay_package_item_status(package, sku));
+    EXPECT_EQ(PAY_PACKAGE_REFUND_STATUS_NOT_PURCHASED, pay_package_refund_status(package, sku));
 
     // install a status observer
     StatusObserverData data;
     InstallStatusObserver(package, data);
 
-    const char* sku = "item";
     EXPECT_TRUE(pay_package_item_start_purchase(package, sku));
 
     // wait for the call to complete
@@ -156,6 +183,7 @@
     EXPECT_EQ(package, data.package);
     EXPECT_EQ(sku, data.sku);
     EXPECT_EQ(PAY_PACKAGE_ITEM_STATUS_PURCHASED, data.status);
+    EXPECT_EQ(PAY_PACKAGE_ITEM_STATUS_PURCHASED, pay_package_item_status(package, sku));
 
     // cleanup
     pay_package_delete(package);
@@ -163,8 +191,6 @@
 
 TEST_F(LibpayPackageTests, PurchaseItemCancelled)
 {
-    GError* error = nullptr;
-    guint callcount = 0;
     auto package = pay_package_new("click-scope");
 
     // install a status observer
@@ -191,15 +217,17 @@
 
 TEST_F(LibpayPackageTests, RefundItem)
 {
-    GError* error = nullptr;
-    guint callcount = 0;
     auto package = pay_package_new("click-scope");
+    const char* sku {"newly_purchased_app"};
+
+    // pre-refund tests
+    EXPECT_EQ(PAY_PACKAGE_ITEM_STATUS_PURCHASED, pay_package_item_status(package, sku));
+    EXPECT_EQ(PAY_PACKAGE_REFUND_STATUS_REFUNDABLE, pay_package_refund_status(package, sku));
 
     // install a status observer
     StatusObserverData data;
     InstallStatusObserver(package, data);
 
-    const char* sku = "item";
     EXPECT_TRUE(pay_package_item_start_refund(package, sku));
 
     // wait for the call to complete
@@ -212,6 +240,8 @@
     EXPECT_EQ(package, data.package);
     EXPECT_EQ(sku, data.sku);
     EXPECT_EQ(PAY_PACKAGE_ITEM_STATUS_NOT_PURCHASED, data.status);
+    EXPECT_EQ(PAY_PACKAGE_ITEM_STATUS_NOT_PURCHASED, pay_package_item_status(package, sku));
+    EXPECT_EQ(PAY_PACKAGE_REFUND_STATUS_NOT_PURCHASED, pay_package_refund_status(package, sku));
 
     // cleanup
     pay_package_delete(package);
@@ -219,8 +249,6 @@
 
 TEST_F(LibpayPackageTests, VerifyItem)
 {
-    GError* error = nullptr;
-    guint callcount = 0;
     auto package = pay_package_new("click-scope");
 
     // install a status observer
@@ -244,3 +272,27 @@
     // cleanup
     pay_package_delete(package);
 }
+
+TEST_F(LibpayPackageTests, ColdCacheStatus)
+{
+    auto package = pay_package_new("click-scope");
+
+    const struct {
+        const char * sku;
+        PayPackageItemStatus expected_item_status;
+        PayPackageRefundStatus expected_refund_status;
+    } tests[] = {
+        { "available_app",       PAY_PACKAGE_ITEM_STATUS_NOT_PURCHASED,  PAY_PACKAGE_REFUND_STATUS_NOT_PURCHASED },
+        { "newly_purchased_app", PAY_PACKAGE_ITEM_STATUS_PURCHASED,      PAY_PACKAGE_REFUND_STATUS_REFUNDABLE },
+        { "old_purchased_app",   PAY_PACKAGE_ITEM_STATUS_PURCHASED,      PAY_PACKAGE_REFUND_STATUS_NOT_REFUNDABLE }
+    };
+
+    for (const auto& test : tests) {
+        EXPECT_EQ(test.expected_item_status, pay_package_item_status(package, test.sku));
+        EXPECT_EQ(test.expected_refund_status, pay_package_refund_status(package, test.sku));
+    }
+
+    // cleanup
+    pay_package_delete(package);
+}
+