launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #06358
[Merge] lp:~sinzui/launchpad/listify-cached-licences into lp:launchpad
Curtis Hovey has proposed merging lp:~sinzui/launchpad/listify-cached-licences into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #931479 in Launchpad itself: "Person+vouchers AttributeError 'tuple' object has no attribute 'append'"
https://bugs.launchpad.net/launchpad/+bug/931479
Bug #931819 in Launchpad itself: "Person:+vouchers timeout"
https://bugs.launchpad.net/launchpad/+bug/931819
For more details, see:
https://code.launchpad.net/~sinzui/launchpad/listify-cached-licences/+merge/92893
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
--
https://code.launchpad.net/~sinzui/launchpad/listify-cached-licences/+merge/92893
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/listify-cached-licences into lp:launchpad.
=== 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
@@ -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,
@@ -2117,6 +2118,7 @@
render_context=self.render_context)
return field
+
def createVoucherField(self):
"""Create voucher field.
@@ -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)
+
@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."))
=== 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.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 00:55: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 00:55: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 00:55:20 +0000
@@ -1519,3 +1519,9 @@
def getAdapter(self, for_interfaces, provided_interface, name=None):
return getMultiAdapter(for_interfaces, provided_interface, name=name)
+
+ def registerUtility(self, component, for_interface, name=''):
+ site_manager = getSiteManager()
+ site_manager.registerUtility(component, for_interface, name)
+ self.addCleanup(
+ site_manager.unregisterUtility, component, for_interface, name)
