Merge lp:~sinzui/launchpad/listify-cached-licences into lp:launchpad

[Curtis Hovey]

Listify the precached commercial licenses.

    Launchpad bug: https://bugs.launchpad.net/bugs/931479
    Pre-implementation: jcsackett

+voucher has always had a performance problem because the saleforce
voucher data is slow to collect, the data is cached. When a voucher is
applied to add the commercial subscription to the projects, the view
issues a next_url signal to itself to reload the data cached by
setUpFields and SetUpWidgets. This breaks because I switched the
Commercial Admin query (not the query used by most users) to use
productset.search which is fast and has cached goodness. next_url is in
conflict with the model cache which does not want to reload the object.

Module zope.app.form.browser.itemswidgets, line 122, in convertTokensToValues
term = self.vocabulary.getTermByToken(token)
Module lp.registry.vocabularies, line 1552, in getTermByToken
for search_result in search_results:
Module lp.services.database.decoratedresultset, line 112, in __iter__
self.pre_iter_hook(results)
Module lp.registry.model.product, line 1602, in eager_load
cache._cached_licenses.append(license.license)
AttributeError: 'tuple' object has no attribute 'append'

--------------------------------------------------------------------

RULES

    * The cache issue was caused more by the vocabulary than by the
      view. The view calls the vocabulary multiple times during field and
      widget setup. getTermByToken() performs a search of search
      was previously called during a iter operation. Search is not needed
      in the case of for commercial admins...the token must be an active
      project name and that is faster to lookup that doing another
      search.
      * Do not search for tokens when the user is a commercial_admin. We
        only care if the token is for an active project.
      * Avoid setting next_url. Setup the fields and widgets again
        without the redeemed voucher.

QA

    * As a commercial admin, apply a voucher on qastaging for any project

LINT

    lib/lp/registry/vocabularies.py
    lib/lp/registry/browser/person.py
    lib/lp/registry/browser/tests/test_commercialsubscription.py
    lib/lp/registry/tests/test_commercialprojects_vocabularies.py
    lib/lp/testing/__init__.py

TEST

    ./bin/test -vvc -t test_commercial -t voucher lp.registry
    ^ This also runs xx-voucher-redemption.txt which was crucial in
    identifing identify the need to regregister the current utility.

IMPLEMENTATION

Update getTermByToken() to just get the product for the token. Removed
a function that obsucured the number of calls to _do_search(). This subtle
change allowed me to redeem vouchers.
    lib/lp/registry/vocabularies.py
    lib/lp/registry/tests/test_commercialprojects_vocabularies.py

I did not need to do this set of changes to unblock my work as a
commercial admin. I see timeouts that I was certain I could fix by
removing the redeemed voucher from the view's cache instead of reloading
the whole view. This was not as simple as I thought it would be because
The fields and wdigets needed to be setup again *without* the submitted
form. I also changed redeemable_vouchers to guarantee a list since the
value could be None or just a voucher. I updated the FakeAdapterMixin
with a help that lets me register a test utility.
    lib/lp/registry/browser/person.py
    lib/lp/registry/browser/tests/test_commercialsubscription.py
    lib/lp/testing/__init__.py

[j.c.sackett (jcsackett)]

Curtis--

This looks good. I have a few little concerns in the diff below, but they're not blockers to approval. I would like to see them address before landing, though.

> === modified file 'lib/lp/registry/browser/person.py'
> --- lib/lp/registry/browser/person.py 2012-02-11 04:45:42 +0000
> +++ lib/lp/registry/browser/person.py 2012-02-14 00:55:20 +0000
> @@ -2139,10 +2141,27 @@
>
> @cachedproperty
> def redeemable_vouchers(self):
> - """Get the redeemable vouchers owned by the user."""
> - vouchers = self.context.getRedeemableCommercialSubscriptionVouchers()
> + """Get the list redeemable vouchers owned by the user."""
> + vouchers = [
> + voucher for voucher in
> + self.context.getRedeemableCommercialSubscriptionVouchers()]
> return vouchers
>
> + def updateRedeemableVouchers(self, voucher):
> + """Remove the voucher from the cached list of redeemable vouchers.
> +
> + Updated the voucher field and widget so that the form can be reused.
> + """
> + vouchers = get_property_cache(self).redeemable_vouchers
> + vouchers.remove(voucher)
> + # Setup the fields and widgets again, but withut the submitted data.
> + self.form_fields = (
> + self.createProjectField() + self.createVoucherField())
> + self.widgets = form.setUpWidgets(
> + self.form_fields.select('project', 'voucher'),
> + self.prefix, self.context, self.request,
> + data=self.initial_values, ignore_request=True)

This is, at heart, a nitpick. Feel free to ignore it if you disagree. It
seems a little weird to name it "updateRedeemableVouchers" when the update is a secondary
effect of removing the voucher to keep the form working. Could we just
call this removeRedeemableVoucher, and note that the update occurs to keep
things in sync?

> @cachedproperty
> def has_commercial_projects(self):
> """Does the user manage one or more commercial project?
> @@ -2181,7 +2200,8 @@
> # Force the page to reload so the just consumed voucher is
> # not displayed again (since the field has already been
> # created).
> - self.next_url = self.request.URL
> + self.updateRedeemableVouchers(voucher)
> + #self.next_url = self.request.URL
> except SalesforceVoucherProxyException, error:
> self.addError(
> _("The voucher could not be redeemed at this time."))

If we're commenting this line out, shouldn't we just delete it?

> === modified file 'lib/lp/registry/browser/tests/test_commercialsubscription.py'
> --- lib/lp/registry/browser/tests/test_commercialsubscription.py 2012-02-09 18:49:14 +0000
> +++ lib/lp/registry/browser/tests/test_commercialsubscription.py 2012-02-14 00:55:20 +0000
> @@ -5,20 +5,113 @@
>
> __metaclass__ = type
>
> +from lp.services.salesforce.interfaces import ISalesforceVoucherProxy
> +from lp.services.salesforce.tests.proxy import TestSalesforceVoucherProxy
> from lp.services.webapp.publisher import canonical_url
> -from lp.testing import TestCaseWithFactory
> +from lp.te...

review: Approve

=== modified file 'lib/lp/registry/browser/person.py'
--- lib/lp/registry/browser/person.py	2012-02-11 04:45:42 +0000
+++ lib/lp/registry/browser/person.py	2012-02-14 15:36:20 +0000
@@ -103,6 +103,7 @@
     queryMultiAdapter,
     )
 from zope.error.interfaces import IErrorReportingUtility
+from zope.formlib import form
 from zope.formlib.form import FormFields
 from zope.interface import (
     classImplements,
@@ -2139,10 +2140,27 @@
 
     @cachedproperty
     def redeemable_vouchers(self):
-        """Get the redeemable vouchers owned by the user."""
-        vouchers = self.context.getRedeemableCommercialSubscriptionVouchers()
+        """Get the list redeemable vouchers owned by the user."""
+        vouchers = [
+            voucher for voucher in
+            self.context.getRedeemableCommercialSubscriptionVouchers()]
         return vouchers
 
+    def removeRedeemableVoucher(self, voucher):
+        """Remove the voucher from the cached list of redeemable vouchers.
+
+        Updated the voucher field and widget so that the form can be reused.
+        """
+        vouchers = get_property_cache(self).redeemable_vouchers
+        vouchers.remove(voucher)
+        # Setup the fields and widgets again, but withut the submitted data.
+        self.form_fields = (
+            self.createProjectField() + self.createVoucherField())
+        self.widgets = form.setUpWidgets(
+            self.form_fields.select('project', 'voucher'),
+            self.prefix, self.context, self.request,
+            data=self.initial_values, ignore_request=True)
+
     @cachedproperty
     def has_commercial_projects(self):
         """Does the user manage one or more commercial project?
@@ -2178,10 +2196,7 @@
                 subscription_months=voucher.term_months)
             self.request.response.addInfoNotification(
                 _("Voucher redeemed successfully"))
-            # Force the page to reload so the just consumed voucher is
-            # not displayed again (since the field has already been
-            # created).
-            self.next_url = self.request.URL
+            self.removeRedeemableVoucher(voucher)
         except SalesforceVoucherProxyException, error:
             self.addError(
                 _("The voucher could not be redeemed at this time."))

=== modified file 'lib/lp/registry/browser/tests/test_commercialsubscription.py'
--- lib/lp/registry/browser/tests/test_commercialsubscription.py	2012-02-09 18:49:14 +0000
+++ lib/lp/registry/browser/tests/test_commercialsubscription.py	2012-02-14 15:36:20 +0000
@@ -5,20 +5,113 @@
 
 __metaclass__ = type
 
+from lp.services.salesforce.interfaces import ISalesforceVoucherProxy
+from lp.services.salesforce.tests.proxy import TestSalesforceVoucherProxy
 from lp.services.webapp.publisher import canonical_url
-from lp.testing import TestCaseWithFactory
+from lp.testing import (
+    FakeAdapterMixin,
+    login_celebrity,
+    login_person,
+    TestCaseWithFactory,
+    )
 from lp.testing.layers import DatabaseFunctionalLayer
 from lp.testing.views import create_initialized_view
 
 
-class PersonVouchersViewTestCase(TestCaseWithFactory):
+class PersonVouchersViewTestCase(FakeAdapterMixin, TestCaseWithFactory):
     layer = DatabaseFunctionalLayer
 
-    def test_init(self):
+    def test_init_without_vouchers_or_projects(self):
+        # Thhe view provides common view properties, but the form is disabled.
         user = self.factory.makePerson()
         self.factory.makeProduct(owner=user)
+        voucher_proxy = TestSalesforceVoucherProxy()
+        self.registerUtility(voucher_proxy, ISalesforceVoucherProxy)
         user_url = canonical_url(user)
         view = create_initialized_view(user, '+vouchers')
         self.assertEqual('Commercial subscription vouchers', view.page_title)
         self.assertEqual(user_url, view.cancel_url)
         self.assertIs(None, view.next_url)
+        self.assertEqual(0, len(view.redeemable_vouchers))
+        self.assertEqual([], view.form_fields)
+
+    def test_init_with_vouchers_and_projects(self):
+        # The fields are setup when the user hase both vouchers and projects.
+        user = self.factory.makePerson()
+        login_person(user)
+        voucher_proxy = TestSalesforceVoucherProxy()
+        voucher_proxy.grantVoucher(
+            user, user, user, 12)
+        self.registerUtility(voucher_proxy, ISalesforceVoucherProxy)
+        self.factory.makeProduct(owner=user)
+        view = create_initialized_view(user, '+vouchers')
+        self.assertEqual(1, len(view.redeemable_vouchers))
+        self.assertEqual(
+            ['project', 'voucher'], [f.__name__ for f in view.form_fields])
+
+    def test_init_with_commercial_admin_with_vouchers(self):
+        # The fields are setup if the commercial admin has vouchers.
+        commercial_admin = login_celebrity('commercial_admin')
+        voucher_proxy = TestSalesforceVoucherProxy()
+        voucher_proxy.grantVoucher(
+            commercial_admin, commercial_admin, commercial_admin, 12)
+        self.registerUtility(voucher_proxy, ISalesforceVoucherProxy)
+        view = create_initialized_view(commercial_admin, '+vouchers')
+        self.assertEqual(1, len(view.redeemable_vouchers))
+        self.assertEqual(
+            ['project', 'voucher'], [f.__name__ for f in view.form_fields])
+
+    def test_redeem_with_commercial_admin(self):
+        # The fields are setup if the commercial admin has vouchers.
+        commercial_admin = login_celebrity('commercial_admin')
+        voucher_proxy = TestSalesforceVoucherProxy()
+        voucher_id = voucher_proxy.grantVoucher(
+            commercial_admin, commercial_admin, commercial_admin, 12)
+        self.registerUtility(voucher_proxy, ISalesforceVoucherProxy)
+        project = self.factory.makeProduct()
+        form = {
+            'field.project': project.name,
+            'field.voucher': voucher_id,
+            'field.actions.redeem': 'Redeem',
+            }
+        view = create_initialized_view(
+            commercial_admin, '+vouchers', form=form)
+        self.assertEqual([], view.errors)
+        self.assertIsNot(None, project.commercial_subscription)
+        self.assertEqual(0, len(view.redeemable_vouchers))
+        self.assertEqual(
+            0, len(view.form_fields['voucher'].field.vocabulary))
+        self.assertEqual(
+            0, len(view.widgets['voucher'].vocabulary))
+
+    def test_redeem_twice_with_commercial_admin(self):
+        # The fields are setup if the commercial admin has vouchers.
+        commercial_admin = login_celebrity('commercial_admin')
+        voucher_proxy = TestSalesforceVoucherProxy()
+        voucher_id_1 = voucher_proxy.grantVoucher(
+            commercial_admin, commercial_admin, commercial_admin, 12)
+        voucher_id_2 = voucher_proxy.grantVoucher(
+            commercial_admin, commercial_admin, commercial_admin, 12)
+        self.registerUtility(voucher_proxy, ISalesforceVoucherProxy)
+        project_1 = self.factory.makeProduct()
+        project_2 = self.factory.makeProduct()
+        form = {
+            'field.project': project_1.name,
+            'field.voucher': voucher_id_1,
+            'field.actions.redeem': 'Redeem',
+            }
+        view = create_initialized_view(
+            commercial_admin, '+vouchers', form=form)
+        self.assertEqual([], view.errors)
+        self.assertIsNot(None, project_1.commercial_subscription)
+        self.assertEqual(1, len(view.redeemable_vouchers))
+        form = {
+            'field.project': project_2.name,
+            'field.voucher': voucher_id_2,
+            'field.actions.redeem': 'Redeem',
+            }
+        view = create_initialized_view(
+            commercial_admin, '+vouchers', form=form)
+        self.assertEqual([], view.errors)
+        self.assertIsNot(None, project_2.commercial_subscription)
+        self.assertEqual(0, len(view.redeemable_vouchers))

=== modified file 'lib/lp/registry/tests/test_commercialprojects_vocabularies.py'
--- lib/lp/registry/tests/test_commercialprojects_vocabularies.py	2012-02-10 21:54:12 +0000
+++ lib/lp/registry/tests/test_commercialprojects_vocabularies.py	2012-02-14 15:36:20 +0000
@@ -116,16 +116,31 @@
         self.assertEqual(
             'Open-widget (expires %s)' % expiration_date, term.title)
 
-    def test_getTermByToken(self):
-        # The term for a token in the vocabulary is returned.
-        token = self.vocab.getTermByToken('open-widget')
-        self.assertEqual(self.maintained_project, token.value)
-
-    def test_getTermByToken_error(self):
+    def test_getTermByToken_user(self):
+        # The term for a token in the vocabulary is returned for maintained
+        # projects.
+        token = self.vocab.getTermByToken('open-widget')
+        self.assertEqual(self.maintained_project, token.value)
+
+    def test_getTermByToken_commercial_admin(self):
+        # The term for a token in the vocabulary is returned for any
+        # active project.
+        login_celebrity('commercial_admin')
+        token = self.vocab.getTermByToken('open-widget')
+        self.assertEqual(self.maintained_project, token.value)
+
+    def test_getTermByToken_error_user(self):
         # A LookupError is raised if the token is not in the vocabulary.
         self.assertRaises(
             LookupError, self.vocab.getTermByToken, 'norwegian-blue-widget')
 
+    def test_getTermByToken_error_commercial_admin(self):
+        # The term for a token in the vocabulary is returned for any
+        # active project.
+        login_celebrity('commercial_admin')
+        self.assertRaises(
+            LookupError, self.vocab.getTermByToken, 'norwegian-blue-widget')
+
     def test_iter(self):
         # The vocabulary can be iterated and the order is by displayname.
         displaynames = [p.value.displayname for p in self.vocab]

=== modified file 'lib/lp/registry/vocabularies.py'
--- lib/lp/registry/vocabularies.py	2012-02-10 21:54:12 +0000
+++ lib/lp/registry/vocabularies.py	2012-02-14 15:36:20 +0000
@@ -1548,10 +1548,15 @@
 
     def getTermByToken(self, token):
         """Return the term for the given token."""
-        search_results = self._doSearch(token)
-        for search_result in search_results:
-            if search_result.name == token:
-                return self.toTerm(search_result)
+        if self.is_commercial_admin:
+            project = self.product_set.getByName(token)
+            if project is not None and project.active:
+                return self.toTerm(project)
+        else:
+            search_results = self._doSearch(token)
+            for search_result in search_results:
+                if search_result.name == token:
+                    return self.toTerm(search_result)
         raise LookupError(token)
 
     def searchForTerms(self, query=None, vocab_filter=None):
@@ -1560,13 +1565,9 @@
         num = results.count()
         return CountableIterator(num, results, self.toTerm)
 
-    def _commercial_projects(self):
-        """Return the list of commercial projects owned by this user."""
-        return self._doSearch()
-
     def __iter__(self):
         """See `IVocabulary`."""
-        for proj in self._commercial_projects():
+        for proj in self._doSearch():
             yield self.toTerm(proj)
 
     def __contains__(self, project):

=== modified file 'lib/lp/testing/__init__.py'
--- lib/lp/testing/__init__.py	2012-02-10 03:56:09 +0000
+++ lib/lp/testing/__init__.py	2012-02-14 15:36:20 +0000
@@ -107,6 +107,7 @@
 from testtools.testcase import ExpectedException as TTExpectedException
 import transaction
 from zope.component import (
+    ComponentLookupError,
     getMultiAdapter,
     getSiteManager,
     getUtility,
@@ -1519,3 +1520,18 @@
 
     def getAdapter(self, for_interfaces, provided_interface, name=None):
         return getMultiAdapter(for_interfaces, provided_interface, name=name)
+
+    def registerUtility(self, component, for_interface, name=''):
+        try:
+            current_commponent = getUtility(for_interface, name=name)
+        except ComponentLookupError:
+            current_commponent = None
+        site_manager = getSiteManager()
+        site_manager.registerUtility(component, for_interface, name)
+        self.addCleanup(
+            site_manager.unregisterUtility, component, for_interface, name)
+        if current_commponent is not None:
+            # Restore the default utility.
+            self.addCleanup(
+                site_manager.registerUtility, current_commponent,
+                for_interface, name)