launchpad-reviewers team mailing list archive

[Curtis Hovey <curtis.hovey@xxxxxxxxxxxxx>]

Curtis Hovey has proposed merging lp:~sinzui/launchpad/contact-new-maintainer into lp:launchpad.

Commit message:
Send licensing email to new project maintainer.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1062358 in Launchpad itself: "new project licensing emails are sent are not sent to the maintainer"
  https://bugs.launchpad.net/launchpad/+bug/1062358

For more details, see:
https://code.launchpad.net/~sinzui/launchpad/contact-new-maintainer/+merge/128273

Lp is sending licensing emails to the project registrant, but the
registrant may have made someone else the maintainer. The licensing
jobs already use the maintainer, so there is a chance of confusion
when the registrant is not a member of the maintainer team.

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

RULES

    Pre-implementation: abentley
    * Use the project maintainer, not the registrant when sending
      the initial email about licensing issues.
    * The emails already use neutral language to deal with user and teams.


QA

    * Clear the staging inbox so that you can find the email.
    * Visit https://qastaging.launchpad.net/projects/+new
    * Register a project with a proprietary license and set a team as the
      maintainer.
    * Check the staging inbox and verify there is a message addressed
      to the team admins.


LINT

    lib/lp/registry/subscribers.py
    lib/lp/registry/tests/test_subscribers.py


TEST

    ./bin/test -vvc lp.registry.tests.test_subscribers


IMPLEMENTATION

I first updated LicenseNotification to ignore the user and instead use
the product.owner. I then removed the user argument since it was unused.
Since the code does not adapt the event.user, tests do not need to be
logged in.
    lib/lp/registry/subscribers.py
    lib/lp/registry/tests/test_subscribers.py
-- 
https://code.launchpad.net/~sinzui/launchpad/contact-new-maintainer/+merge/128273
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/contact-new-maintainer into lp:launchpad.

=== modified file 'lib/lp/registry/subscribers.py'
--- lib/lp/registry/subscribers.py	2012-06-14 05:18:22 +0000
+++ lib/lp/registry/subscribers.py	2012-10-05 15:09:23 +0000
@@ -14,12 +14,12 @@
 import pytz
 from zope.security.proxy import removeSecurityProxy
 
-from lp.registry.interfaces.person import IPerson
 from lp.registry.interfaces.product import License
 from lp.services.config import config
 from lp.services.mail.helpers import get_email_template
 from lp.services.mail.sendmail import (
     format_address,
+    format_address_for_person,
     simple_sendmail,
     )
 from lp.services.webapp.menu import structured
@@ -32,8 +32,7 @@
 def product_licenses_modified(product, event):
     """Send a notification if licences changed and a licence is special."""
     if LicenseNotification.needs_notification(product):
-        user = IPerson(event.user)
-        notification = LicenseNotification(product, user)
+        notification = LicenseNotification(product)
         notification.send()
         notification.display()
 
@@ -41,9 +40,8 @@
 class LicenseNotification:
     """Send notification about special licences to the user."""
 
-    def __init__(self, product, user):
+    def __init__(self, product):
         self.product = product
-        self.user = user
 
     @staticmethod
     def needs_notification(product):
@@ -85,15 +83,18 @@
         if not self.needs_notification(self.product):
             # The project has a common licence.
             return False
-        user_address = format_address(
-            self.user.displayname, self.user.preferredemail.email)
+        maintainer = self.product.owner
+        if maintainer.is_team:
+            user_address = maintainer.getTeamAdminsEmailAddresses()
+        else:
+            user_address = format_address_for_person(maintainer)
         from_address = format_address(
             "Launchpad", config.canonical.noreply_from_address)
         commercial_address = format_address(
             'Commercial', 'commercial@xxxxxxxxxxxxx')
         substitutions = dict(
-            user_displayname=self.user.displayname,
-            user_name=self.user.name,
+            user_displayname=maintainer.displayname,
+            user_name=maintainer.name,
             product_name=self.product.name,
             product_url=canonical_url(self.product),
             commercial_use_expiration=self.getCommercialUseMessage(),

=== modified file 'lib/lp/registry/tests/test_subscribers.py'
--- lib/lp/registry/tests/test_subscribers.py	2012-10-03 11:20:01 +0000
+++ lib/lp/registry/tests/test_subscribers.py	2012-10-05 15:09:23 +0000
@@ -8,18 +8,25 @@
 from datetime import datetime
 
 import pytz
+from zope.component import getUtility
 from zope.security.proxy import removeSecurityProxy
 
+from lp.registry.interfaces.person import TeamMembershipPolicy
 from lp.registry.interfaces.product import License
 from lp.registry.model.product import LicensesModifiedEvent
 from lp.registry.subscribers import (
     LicenseNotification,
     product_licenses_modified,
     )
+from lp.registry.interfaces.teammembership import (
+    ITeamMembershipSet,
+    TeamMembershipStatus,
+    )
 from lp.services.webapp.publisher import get_current_browser_request
 from lp.testing import (
     login_person,
     logout,
+    person_logged_in,
     TestCaseWithFactory,
     )
 from lp.testing.layers import DatabaseFunctionalLayer
@@ -32,16 +39,13 @@
 
     def test_init(self):
         product = self.factory.makeProduct()
-        login_person(product.owner)
         event = LicensesModifiedEvent(product)
-        self.assertEqual(product.owner, event.user.person)
         self.assertEqual(product, event.object)
         self.assertEqual(product, event.object_before_modification)
         self.assertEqual([], event.edited_fields)
 
     def test_init_with_user(self):
         product = self.factory.makeProduct()
-        login_person(product.owner)
         event = LicensesModifiedEvent(product, user=product.owner)
         self.assertEqual(product.owner, event.user)
 
@@ -53,7 +57,6 @@
     def make_product_event(self, licenses):
         product = self.factory.makeProduct(licenses=licenses)
         pop_notifications()
-        login_person(product.owner)
         event = LicensesModifiedEvent(product, user=product.owner)
         return product, event
 
@@ -128,7 +131,7 @@
     def test_send_known_license(self):
         # A known licence does not generate an email.
         product, user = self.make_product_user([License.GNU_GPL_V2])
-        notification = LicenseNotification(product, user)
+        notification = LicenseNotification(product)
         result = notification.send()
         self.assertIs(False, result)
         self.assertEqual(0, len(pop_notifications()))
@@ -136,7 +139,7 @@
     def test_send_other_dont_know(self):
         # An Other/I don't know licence sends one email.
         product, user = self.make_product_user([License.DONT_KNOW])
-        notification = LicenseNotification(product, user)
+        notification = LicenseNotification(product)
         result = notification.send()
         self.assertIs(True, result)
         self.verify_whiteboard(product)
@@ -147,7 +150,7 @@
     def test_send_other_open_source(self):
         # An Other/Open Source licence sends one email.
         product, user = self.make_product_user([License.OTHER_OPEN_SOURCE])
-        notification = LicenseNotification(product, user)
+        notification = LicenseNotification(product)
         result = notification.send()
         self.assertIs(True, result)
         self.verify_whiteboard(product)
@@ -158,7 +161,7 @@
     def test_send_other_proprietary(self):
         # An Other/Proprietary licence sends one email.
         product, user = self.make_product_user([License.OTHER_PROPRIETARY])
-        notification = LicenseNotification(product, user)
+        notification = LicenseNotification(product)
         result = notification.send()
         self.assertIs(True, result)
         self.verify_whiteboard(product)
@@ -166,6 +169,28 @@
         self.assertEqual(1, len(notifications))
         self.verify_user_email(notifications.pop())
 
+    def test_send_other_proprietary_team_admins(self):
+        # An Other/Proprietary licence sends one email to the team admins.
+        product, user = self.make_product_user([License.OTHER_PROPRIETARY])
+        owner = self.factory.makePerson(email='owner@xxxxxx')
+        team = self.factory.makeTeam(
+            owner=owner, membership_policy=TeamMembershipPolicy.RESTRICTED)
+        admin = self.factory.makePerson(email='admin@xxxxxx')
+        with person_logged_in(owner):
+            team.addMember(admin, owner)
+            membership_set = getUtility(ITeamMembershipSet)
+            tm = membership_set.getByPersonAndTeam(admin, team)
+            tm.setStatus(TeamMembershipStatus.ADMIN, owner)
+        with person_logged_in(product.owner):
+            product.owner = team
+        pop_notifications()
+        notification = LicenseNotification(product)
+        result = notification.send()
+        self.assertIs(True, result)
+        notifications = pop_notifications()
+        self.assertEqual(1, len(notifications))
+        self.assertEqual('admin@xxxxxx,owner@xxxxxx', notifications[0]['To'])
+
     def test_display_no_request(self):
         # If there is no request, there is no reason to show a message in
         # the browser.
@@ -173,7 +198,7 @@
         # Using the proxied product leads to an exeception when
         # notification.display() below is called because the permission
         # checks product require an interaction.
-        notification = LicenseNotification(removeSecurityProxy(product), user)
+        notification = LicenseNotification(removeSecurityProxy(product))
         logout()
         result = notification.display()
         self.assertIs(False, result)
@@ -181,7 +206,7 @@
     def test_display_no_message(self):
         # A notification is not added if there is no message to show.
         product, user = self.make_product_user([License.GNU_GPL_V2])
-        notification = LicenseNotification(product, user)
+        notification = LicenseNotification(product)
         result = notification.display()
         self.assertEqual('', notification.getCommercialUseMessage())
         self.assertIs(False, result)
@@ -189,7 +214,7 @@
     def test_display_has_message(self):
         # A notification is added if there is a message to show.
         product, user = self.make_product_user([License.OTHER_PROPRIETARY])
-        notification = LicenseNotification(product, user)
+        notification = LicenseNotification(product)
         result = notification.display()
         message = notification.getCommercialUseMessage()
         self.assertIs(True, result)
@@ -204,7 +229,7 @@
         # A notification is added if there is a message to show.
         product, user = self.make_product_user([License.OTHER_PROPRIETARY])
         product.displayname = '<b>Look</b>'
-        notification = LicenseNotification(product, user)
+        notification = LicenseNotification(product)
         result = notification.display()
         self.assertIs(True, result)
         request = get_current_browser_request()
@@ -221,33 +246,33 @@
 
     def test_getTemplateName_other_dont_know(self):
         product, user = self.make_product_user([License.DONT_KNOW])
-        notification = LicenseNotification(product, user)
+        notification = LicenseNotification(product)
         self.assertEqual(
             'product-license-dont-know.txt',
             notification.getTemplateName())
 
     def test_getTemplateName_propietary(self):
         product, user = self.make_product_user([License.OTHER_PROPRIETARY])
-        notification = LicenseNotification(product, user)
+        notification = LicenseNotification(product)
         self.assertEqual(
             'product-license-other-proprietary.txt',
             notification.getTemplateName())
 
     def test_getTemplateName_other_open_source(self):
         product, user = self.make_product_user([License.OTHER_OPEN_SOURCE])
-        notification = LicenseNotification(product, user)
+        notification = LicenseNotification(product)
         self.assertEqual(
             'product-license-other-open-source.txt',
             notification.getTemplateName())
 
     def test_getCommercialUseMessage_without_commercial_subscription(self):
         product, user = self.make_product_user([License.MIT])
-        notification = LicenseNotification(product, user)
+        notification = LicenseNotification(product)
         self.assertEqual('', notification.getCommercialUseMessage())
 
     def test_getCommercialUseMessage_with_complimentary_cs(self):
         product, user = self.make_product_user([License.OTHER_PROPRIETARY])
-        notification = LicenseNotification(product, user)
+        notification = LicenseNotification(product)
         message = (
             "Ball's complimentary commercial subscription expires on %s." %
             product.commercial_subscription.date_expires.date().isoformat())
@@ -257,7 +282,7 @@
         product, user = self.make_product_user([License.MIT])
         self.factory.makeCommercialSubscription(product)
         product.licenses = [License.MIT, License.OTHER_PROPRIETARY]
-        notification = LicenseNotification(product, user)
+        notification = LicenseNotification(product)
         message = (
             "Ball's commercial subscription expires on %s." %
             product.commercial_subscription.date_expires.date().isoformat())
@@ -267,7 +292,7 @@
         product, user = self.make_product_user([License.MIT])
         self.factory.makeCommercialSubscription(product, expired=True)
         product.licenses = [License.MIT, License.OTHER_PROPRIETARY]
-        notification = LicenseNotification(product, user)
+        notification = LicenseNotification(product)
         message = (
             "Ball's commercial subscription expired on %s." %
             product.commercial_subscription.date_expires.date().isoformat())