launchpad-reviewers team mailing list archive

[Curtis Hovey <curtis.hovey@xxxxxxxxxxxxx>]

Curtis Hovey has proposed merging lp:~sinzui/launchpad/apply-commercial-subscriptins-1 into lp:launchpad.

Commit message:
Allow commercial admins to apply commercial subscription vouchers for users.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1087293 in Launchpad itself: "Lp does not explain that a voucher can be applied to a commercial project"
  https://bugs.launchpad.net/launchpad/+bug/1087293

For more details, see:
https://code.launchpad.net/~sinzui/launchpad/apply-commercial-subscriptins-1/+merge/138761

Users are confused that buying a commercial subscription does not apply
it to their commercial project. Commercial Admins often quiz both the
buyer and ask Lp Admins to confirm the user has vouchers that can be
applied to a commercial project.


RULES

    Pre-implementation: czajkowski
    * Allow Commercial Admins to view another user's vouchers and apply them
      to the user's projects.

    Next branch
    * Show a link to the +vouchers page when the user has commercial
      subscription vouchers.
    * Show a link to the user's +voucher page when the user has
      commercial subscription vouchers and the project page says it
      needs a renewed commercial subscription.


QA

    As a CA
    * Visit https://qastaging.launchpad.net/~mrevell/+vouchers
      or https://qastaging.launchpad.net/~bac/+vouchers
    * Verify you can see they have vouchers and that you can apply one.


LINT

    lib/lp/security.py
    lib/lp/registry/browser/configure.zcml
    lib/lp/registry/browser/person.py
    lib/lp/registry/browser/tests/test_commercialsubscription.py


LoC

    I have more than 10,000 lines of credit this week.

TEST

    ./bin/test -vvc -t PersonVouchersViewTestCase -t vouchers lp.registry


IMPLEMENTATION

I added a security checker for IPerson and launchpad.Commercial, then
updated the view to ensure there was no change for users. I then added
tests and made one small adjustment to the view to Verify that commercial
admins could see and apply another user's vouchers. I removed a lot ot
duplication in the viuew tests.
    lib/lp/security.py
    lib/lp/registry/browser/configure.zcml
    lib/lp/registry/browser/person.py
    lib/lp/registry/browser/tests/test_commercialsubscription.py

-- 
https://code.launchpad.net/~sinzui/launchpad/apply-commercial-subscriptins-1/+merge/138761
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/apply-commercial-subscriptins-1 into lp:launchpad.

=== modified file 'lib/lp/registry/browser/configure.zcml'
--- lib/lp/registry/browser/configure.zcml	2012-11-08 03:55:11 +0000
+++ lib/lp/registry/browser/configure.zcml	2012-12-07 15:50:29 +0000
@@ -955,7 +955,7 @@
         <browser:page
             for="lp.registry.interfaces.person.IPerson"
             name="+vouchers"
-            permission="launchpad.Edit"
+            permission="launchpad.Commercial"
             class="lp.registry.browser.person.PersonVouchersView"
             template="../templates/person-vouchers.pt"/>
         <browser:page

=== modified file 'lib/lp/registry/browser/person.py'
--- lib/lp/registry/browser/person.py	2012-11-29 16:17:01 +0000
+++ lib/lp/registry/browser/person.py	2012-12-07 15:50:29 +0000
@@ -177,6 +177,7 @@
 from lp.registry.interfaces.pillar import IPillarNameSet
 from lp.registry.interfaces.poll import IPollSubset
 from lp.registry.interfaces.product import IProduct
+from lp.registry.interfaces.role import IPersonRoles
 from lp.registry.interfaces.ssh import (
     ISSHKeySet,
     SSHKeyAdditionError,
@@ -1330,7 +1331,8 @@
         project so the property is True always.  Otherwise it is true if the
         vocabulary is not empty.
         """
-        if check_permission('launchpad.Commercial', self.context):
+        role = IPersonRoles(self.user)
+        if role.in_commercial_admin or role.in_admin:
             return True
         vocabulary_registry = getVocabularyRegistry()
         vocabulary = vocabulary_registry.get(self.context,

=== modified file 'lib/lp/registry/browser/tests/test_commercialsubscription.py'
--- lib/lp/registry/browser/tests/test_commercialsubscription.py	2012-10-22 02:30:44 +0000
+++ lib/lp/registry/browser/tests/test_commercialsubscription.py	2012-12-07 15:50:29 +0000
@@ -26,114 +26,121 @@
 class PersonVouchersViewTestCase(FakeAdapterMixin, TestCaseWithFactory):
     layer = DatabaseFunctionalLayer
 
+    def makeVouchers(self, user, number, voucher_proxy=None):
+        if voucher_proxy is None:
+            voucher_proxy = TestSalesforceVoucherProxy()
+        self.registerUtility(voucher_proxy, ISalesforceVoucherProxy)
+        vouchers = []
+        for n in xrange(number):
+            vouchers.append(voucher_proxy.grantVoucher(user, user, user, 12))
+        return vouchers
+
     def test_init_without_vouchers_or_projects(self):
         # The 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)
+        self.makeVouchers(user, 0)
         user_url = canonical_url(user)
-        view = create_initialized_view(user, '+vouchers')
+        with person_logged_in(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))
 
+    def assertFields(self, view):
+        self.assertEqual(1, len(view.redeemable_vouchers))
+        self.assertEqual(
+            ['project', 'voucher'], [f.__name__ for f in 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.makeVouchers(user, 1)
         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])
+        self.assertFields(view)
 
     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)
+        self.makeVouchers(commercial_admin, 1)
         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 = {
+        self.assertFields(view)
+
+    def test_with_commercial_admin_for_user_with_vouchers_and_projects(self):
+        # A commercial admin can see another user's vouchers.
+        user = self.factory.makePerson()
+        login_person(user)
+        self.makeVouchers(user, 1)
+        self.factory.makeProduct(owner=user)
+        login_celebrity('commercial_admin')
+        view = create_initialized_view(user, '+vouchers')
+        self.assertFields(view)
+
+    def assertRedeem(self, view, project, remaining=0):
+        self.assertEqual([], view.errors)
+        self.assertIsNot(None, project.commercial_subscription)
+        self.assertEqual(remaining, len(view.redeemable_vouchers))
+        self.assertEqual(
+            remaining, len(view.form_fields['voucher'].field.vocabulary))
+        self.assertEqual(
+            remaining, len(view.widgets['voucher'].vocabulary))
+
+    def makeForm(self, project, voucher_id):
+        return {
             'field.project': project.name,
             'field.voucher': voucher_id,
             'field.actions.redeem': 'Redeem',
             }
+
+    def test_redeem_with_commercial_admin_for_user(self):
+        # A commercial admin can redeem a voucher for a user.
+        project = self.factory.makeProduct()
+        user = project.owner
+        [voucher_id] = self.makeVouchers(user, 1)
+        form = self.makeForm(project, voucher_id)
+        login_celebrity('commercial_admin')
+        view = create_initialized_view(user, '+vouchers', form=form)
+        self.assertRedeem(view, project)
+
+    def test_redeem_with_commercial_admin(self):
+        # The fields are setup if the commercial admin has vouchers.
+        commercial_admin = login_celebrity('commercial_admin')
+        [voucher_id] = self.makeVouchers(commercial_admin, 1)
+        project = self.factory.makeProduct()
+        form = self.makeForm(project, voucher_id)
         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))
+        self.assertRedeem(view, project)
 
     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)
+        voucher_id_1, voucher_id_2 = self.makeVouchers(
+            commercial_admin, 2, voucher_proxy)
         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',
-            }
+        form = self.makeForm(project_1, voucher_id_1)
         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))
+        self.assertRedeem(view, project_1, remaining=1)
         # A job will notify Salesforce of the voucher redemption but here we
         # will do it manually.
         voucher_proxy.redeemVoucher(
             voucher_id_1, commercial_admin, project_1)
 
-        form = {
-            'field.project': project_2.name,
-            'field.voucher': voucher_id_2,
-            'field.actions.redeem': 'Redeem',
-            }
+        form = self.makeForm(project_2, voucher_id_2)
         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))
+        self.assertRedeem(view, project_2)
 
     def test_pending_vouchers_excluded(self):
         # Vouchers pending redemption in Salesforce are not included in choice.
         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)
+        voucher_id_1, voucher_id_2 = self.makeVouchers(commercial_admin, 2)
         project_1 = self.factory.makeProduct()
         self.factory.makeCommercialSubscription(
             project_1, False, 'pending-' + voucher_id_1)
@@ -146,10 +153,7 @@
     def test_redeem_twice_causes_error(self):
         # If a voucher is redeemed twice, the second attempt is rejected.
         commercial_admin = login_celebrity('commercial_admin')
-        voucher_proxy = TestSalesforceVoucherProxy()
-        voucher_id_1 = voucher_proxy.grantVoucher(
-            commercial_admin, commercial_admin, commercial_admin, 12)
-        self.registerUtility(voucher_proxy, ISalesforceVoucherProxy)
+        voucher_id_1, voucher_id_2 = self.makeVouchers(commercial_admin, 2)
         project_1 = self.factory.makeProduct(name='p1')
         project_2 = self.factory.makeProduct(name='p2')
         url = canonical_url(commercial_admin, view_name='+vouchers')

=== modified file 'lib/lp/security.py'
--- lib/lp/security.py	2012-12-04 13:52:02 +0000
+++ lib/lp/security.py	2012-12-07 15:50:29 +0000
@@ -895,6 +895,16 @@
         return False
 
 
+class AdminByCommercialTeamOrAdminsOrPerson(AdminByCommercialTeamOrAdmins):
+    permission = 'launchpad.Commercial'
+    usedfor = IPerson
+
+    def checkAuthenticated(self, user):
+        """Users can manage their commericial data and admins can help."""
+        base = super(AdminByCommercialTeamOrAdminsOrPerson, self)
+        return self.obj.id == user.id or base.checkAuthenticated(user)
+
+
 class EditPersonBySelfOrAdmins(AuthorizationBase):
     permission = 'launchpad.Edit'
     usedfor = IPerson