Merge lp:~sinzui/launchpad/team-delete-email into lp:launchpad

[Curtis Hovey]

This is my branch to allow teams to delete old email addresses.

    lp:~sinzui/launchpad/team-delete-email
    Diff size: 282
    Launchpad bug: https://bugs.launchpad.net/bugs/250103
    Test command: ./bin/test -vv \
        -t TestTeamContactAddress
        -t doc/person.txt \
        -t person-views \
        -t peoplemerge-views
    Pre-implementation: no one
    Target release: 10.04

Allow teams to delete old email addresses
-----------------------------------------

Launchpad never deletes email addresses. When a team changes its email
address, the old one is hidden and marked valid. When a team is merged, the
old email address is marked new and hidden so that it cannot be reclaimed.
This causes lots of problems because email addresses are really owned/managed
by users and they want to reuse the addresses. LOSAs delete 1 or more email
addresses every week after a user discovers that the email address they know
they removed are still associated with a team.

Launchpad should delete the old address as the UI implies was done.

Rules
-----

    * Delete the old email address when the team sets a new contact address.
      If users want to reuse the address they must reconfirm it.
    * Provide a SQL script to remove all the historic old team email
      addresses.

QA
--

On staging where the db can be examined:
    * Add an email address to a team, then remove it.
    * Verify the team has no email address in the DB.
    * Create a mailing list for a team and set it as the contact addresses,
      then remove it.
    * Verify the team has one email address for the mailing list.
    * Add another email address.
    * Verify the team has two email addresses.

Lint
----

Linting changed files:
  database/schema/pending/delete-unused-team-addresses.sql
  lib/lp/registry/browser/tests/peoplemerge-views.txt
  lib/lp/registry/browser/tests/person-views.txt
  lib/lp/registry/doc/person.txt
  lib/lp/registry/model/person.py
  lib/lp/registry/tests/test_team.py

Test
----

    * lib/lp/registry/browser/tests/peoplemerge-views.txt
      * Updated the delete a team test to explain the only case a team can
        have a NEW email address. Updated the test to use the factory.
    * lib/lp/registry/browser/tests/person-views.txt
      * Updated the visible_email_addresses test to explain that teams may
        have two possible email addresses. Updated the test to use the
        factory.
    * lib/lp/registry/doc/person.txt
      * Updated the setContactAddress() doc not verify the email address
        that ceased to exist.
    * lib/lp/registry/tests/test_team.py
      * Added a new test to verify the conditions that setContactAddress()
        supports.

Implementation
--------------

    * database/schema/pending/delete-unused-team-addresses.sql
      * Added a script that can be run by admins to delete the historic
        data.
    * lib/lp/registry/model/person.py
      * Removed some unused variables that new pyflakes reports are lint.
      * Revised the setContactAddress() method to purge the unused team
        email addresses when the team email address is set.

[Brad Crittenden (bac)]

Hi Curtis,

This change is very nice. I found one typo but everything else looks great.

> === modified file 'lib/lp/registry/browser/tests/peoplemerge-views.txt'
> --- lib/lp/registry/browser/tests/peoplemerge-views.txt 2010-04-12 08:11:18 +0000
> +++ lib/lp/registry/browser/tests/peoplemerge-views.txt 2010-04-19 21:35:31 +0000
> @@ -251,21 +251,23 @@
> >>> print find_tag_by_id(content, 'field.actions.delete')
> None
>
> -The registry experts should be able to delete a team with an
> -validated email address, which will be invisible, since only
> -preferred email addresses are shown for teams.
> +The registry experts can delete a team with an new email address (from

s/an new/a new

> +an import), which will be invisible, since only preferred email addresses are
> +shown for teams.

review: Approve (code)

=== added file 'database/schema/pending/delete-unused-team-addresses.sql'
--- database/schema/pending/delete-unused-team-addresses.sql	1970-01-01 00:00:00 +0000
+++ database/schema/pending/delete-unused-team-addresses.sql	2010-04-20 20:05:34 +0000
@@ -0,0 +1,30 @@
+-- Copyright 2010 Canonical Ltd.  This software is licensed under the
+-- GNU Affero General Public License version 3 (see the file LICENSE).
+
+SET client_min_messages=ERROR;
+
+--SELECT person.name, email, status
+--FROM EmailAddress
+--    JOIN Person ON EmailAddress.person = Person.id
+--        AND Person.teamowner IS NOT NULL
+--WHERE
+--    EmailAddress.status in (1, 2)
+--    AND EmailAddress.email not in (
+--        person.name || '@lists.launchpad.net',
+--        person.name || '@lists.staging.launchpad.net')
+--;
+
+-- Delete all the unused team email addresses that Launchpad claims to have
+-- removed. Status 1 (NEW, meaning merged) or 2 (VALID, meaning not used)
+-- and email not the team's mailing list address.
+DELETE
+FROM EmailAddress
+USING Person
+WHERE
+    EmailAddress.person = Person.id
+    AND Person.teamowner IS NOT NULL
+    AND EmailAddress.status in (1, 2)
+    AND EmailAddress.email not in (
+        person.name || '@lists.launchpad.net',
+        person.name || '@lists.staging.launchpad.net')
+;
 
=== modified file 'lib/lp/registry/browser/tests/peoplemerge-views.txt'
--- lib/lp/registry/browser/tests/peoplemerge-views.txt	2010-04-12 08:11:18 +0000
+++ lib/lp/registry/browser/tests/peoplemerge-views.txt	2010-04-20 20:05:34 +0000
@@ -251,21 +251,23 @@
     >>> print find_tag_by_id(content, 'field.actions.delete')
     None
 
-The registry experts should be able to delete a team with an
-validated email address, which will be invisible, since only
-preferred email addresses are shown for teams.
+The registry experts can delete a team with a new email address (from
+an import), which will be invisible, since only preferred email addresses are
+shown for teams.
 
     >>> from canonical.launchpad.interfaces.emailaddress import (
-    ...     IEmailAddressSet)
+    ...     IEmailAddressSet, EmailAddressStatus)
     >>> login_person(registry_expert)
     >>> admin = getUtility(ILaunchpadCelebrities).admin
     >>> registry_expert.inTeam(admin)
     False
-    >>> deletable_team = factory.makeTeam(email="[e-mail]")
-    >>> deletable_team.setContactAddress(None)
+    >>> deletable_team = factory.makeTeam()
+    >>> email = factory.makeEmail(
+    ...     "[e-mail]", deletable_team,
+    ...     email_status=EmailAddressStatus.NEW)
     >>> for email in getUtility(IEmailAddressSet).getByPerson(deletable_team):
     ...     print email.email, email.status.title
-    [e-mail] Validated Email Address
+    [e-mail] New Email Address
     >>> form = {'field.actions.delete': 'Delete'}
     >>> view = create_initialized_view(deletable_team, '+delete', form=form)
     >>> view.errors
 
=== modified file 'lib/lp/registry/browser/tests/person-views.txt'
--- lib/lp/registry/browser/tests/person-views.txt	2010-03-11 20:54:36 +0000
+++ lib/lp/registry/browser/tests/person-views.txt	2010-04-20 20:05:34 +0000
@@ -207,18 +207,15 @@
     >>> view.visible_email_addresses
     [u'[e-mail]']
 
-It is possible for a team to have more than one valid address, but only
-the preferred address is listed in the visible_email_addresses property.
-
-    >>> from canonical.launchpad.interfaces.emailaddress import (
-    ...     EmailAddressStatus, IEmailAddressSet)
-
-    >>> email_address_set = getUtility(IEmailAddressSet)
-    >>> email_address = email_address_set.new(
-    ...     '[e-mail]',
-    ...     person=ubuntu_team,
-    ...     status=EmailAddressStatus.VALIDATED)
+It is possible for a team to have more than two addresses (from a mailing
+list), but only the preferred address is listed in the visible_email_addresses
+property.
+
+    >>> email_address = factory.makeEmail(
+    ...     '[e-mail]', ubuntu_team)
     >>> ubuntu_team.setContactAddress(email_address)
+    >>> mailing_list = factory.makeMailingList(
+    ...     ubuntu_team, ubuntu_team.teamowner)
     >>> view = create_initialized_view(ubuntu_team, '+index')
     >>> view.visible_email_addresses
     [u'[e-mail]']
@@ -739,10 +736,8 @@
 
 EmailToPersonView will use the contact address when the team has one.
 
-    >>> email_address = email_address_set.new(
-    ...     '[e-mail]',
-    ...     person=landscape_developers,
-    ...     status=EmailAddressStatus.VALIDATED)
+    >>> email_address = factory.makeEmail(
+    ...     '[e-mail]', landscape_developers)
     >>> landscape_developers.setContactAddress(email_address)
 
     >>> view = create_initialized_view(landscape_developers, '+contactuser')
 
=== modified file 'lib/lp/registry/doc/person.txt'
--- lib/lp/registry/doc/person.txt	2010-02-17 14:42:16 +0000
+++ lib/lp/registry/doc/person.txt	2010-04-20 20:05:34 +0000
@@ -188,9 +188,6 @@
 to setContactAddress() to leave a team without a contact address.
 
     >>> shipit_admins.setContactAddress(None)
-    >>> email.status
-    <DBItem EmailAddressStatus.VALIDATED...
-
     >>> print shipit_admins.preferredemail
     None
 
 
=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2010-04-06 21:42:28 +0000
+++ lib/lp/registry/model/person.py	2010-04-20 20:05:34 +0000
@@ -123,8 +123,7 @@
 from lp.registry.interfaces.teammembership import (
     TeamMembershipStatus)
 from lp.registry.interfaces.wikiname import IWikiName, IWikiNameSet
-from canonical.launchpad.webapp.interfaces import (
-    ILaunchBag, IStoreSelector, MASTER_FLAVOR)
+from canonical.launchpad.webapp.interfaces import ILaunchBag
 
 from lp.soyuz.model.archive import Archive
 from lp.registry.model.codeofconduct import SignedCodeOfConduct
@@ -1162,11 +1161,8 @@
                 "You need to specify a reviewer when a team joins another.")
             requester = self
 
-        expired = TeamMembershipStatus.EXPIRED
         proposed = TeamMembershipStatus.PROPOSED
         approved = TeamMembershipStatus.APPROVED
-        declined = TeamMembershipStatus.DECLINED
-        deactivated = TeamMembershipStatus.DEACTIVATED
 
         if team.subscriptionpolicy == TeamSubscriptionPolicy.RESTRICTED:
             raise JoinNotAllowed("This is a restricted team")
@@ -1261,7 +1257,7 @@
                 comment=comment)
             # Accessing the id attribute ensures that the team
             # creation has been flushed to the database.
-            tm_id = tm.id
+            tm.id
             notify(event(person, self))
         else:
             # We can't use tm.setExpirationDate() here because the reviewer
@@ -2058,6 +2054,19 @@
             self._unsetPreferredEmail()
         else:
             self._setPreferredEmail(email)
+        # A team can have up to two addresses, the preferred one and one used
+        # by the team mailing list.
+        if self.mailing_list is not None:
+            mailing_list_email = getUtility(IEmailAddressSet).getByEmail(
+                self.mailing_list.address)
+            mailing_list_email = IMasterObject(mailing_list_email)
+        else:
+            mailing_list_email = None
+        all_addresses = IMasterStore(self).find(
+            EmailAddress, EmailAddress.personID == self.id)
+        for address in all_addresses :
+            if address not in (email, mailing_list_email):
+                address.destroySelf()
 
     def _unsetPreferredEmail(self):
         """Change the preferred email address to VALIDATED."""
@@ -2112,6 +2121,7 @@
 
         email = removeSecurityProxy(email)
         IMasterObject(email).status = EmailAddressStatus.PREFERRED
+        IMasterObject(email).syncUpdate()
 
         # Now we update our cache of the preferredemail.
         self._preferredemail_cached = email
 
=== added file 'lib/lp/registry/tests/test_team.py'
--- lib/lp/registry/tests/test_team.py	1970-01-01 00:00:00 +0000
+++ lib/lp/registry/tests/test_team.py	2010-04-20 20:05:34 +0000
@@ -0,0 +1,86 @@
+# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for PersonSet."""
+
+__metaclass__ = type
+
+import transaction
+from unittest import TestLoader
+
+from zope.component import getUtility
+
+from canonical.launchpad.database.emailaddress import EmailAddress
+from canonical.launchpad.interfaces.emailaddress import IEmailAddressSet
+from canonical.launchpad.interfaces.lpstorm import IMasterStore
+from canonical.testing import DatabaseFunctionalLayer
+
+from lp.testing import TestCaseWithFactory
+
+
+class TestTeamContactAddress(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def getAllEmailAddresses(self):
+        transaction.commit()
+        all_addresses = self.store.find(
+            EmailAddress, EmailAddress.personID == self.team.id)
+        return [address for address in all_addresses.order_by('email')]
+
+    def createMailingListAndGetAddress(self):
+        mailing_list = self.factory.makeMailingList(
+            self.team, self.team.teamowner)
+        return getUtility(IEmailAddressSet).getByEmail(
+                mailing_list.address)
+
+    def setUp(self):
+        super(TestTeamContactAddress, self).setUp()
+        self.team = self.factory.makeTeam(name='alpha')
+        self.address = self.factory.makeEmail('[e-mail]', self.team)
+        self.store = IMasterStore(self.address)
+
+    def test_setContactAddress_from_none(self):
+        self.team.setContactAddress(self.address)
+        self.assertEqual(self.address, self.team.preferredemail)
+        self.assertEqual([self.address], self.getAllEmailAddresses())
+
+    def test_setContactAddress_to_none(self):
+        self.team.setContactAddress(self.address)
+        self.team.setContactAddress(None)
+        self.assertEqual(None, self.team.preferredemail)
+        self.assertEqual([], self.getAllEmailAddresses())
+
+    def test_setContactAddress_to_new_address(self):
+        self.team.setContactAddress(self.address)
+        new_address = self.factory.makeEmail('[e-mail]', self.team)
+        self.team.setContactAddress(new_address)
+        self.assertEqual(new_address, self.team.preferredemail)
+        self.assertEqual([new_address], self.getAllEmailAddresses())
+
+    def test_setContactAddress_to_mailing_list(self):
+        self.team.setContactAddress(self.address)
+        list_address = self.createMailingListAndGetAddress()
+        self.team.setContactAddress(list_address)
+        self.assertEqual(list_address, self.team.preferredemail)
+        self.assertEqual([list_address], self.getAllEmailAddresses())
+
+    def test_setContactAddress_from_mailing_list(self):
+        list_address = self.createMailingListAndGetAddress()
+        self.team.setContactAddress(list_address)
+        new_address = self.factory.makeEmail('[e-mail]', self.team)
+        self.team.setContactAddress(new_address)
+        self.assertEqual(new_address, self.team.preferredemail)
+        self.assertEqual(
+            [list_address, new_address], self.getAllEmailAddresses())
+
+    def test_setContactAddress_from_mailing_list_to_none(self):
+        list_address = self.createMailingListAndGetAddress()
+        self.team.setContactAddress(list_address)
+        self.team.setContactAddress(None)
+        self.assertEqual(None, self.team.preferredemail)
+        self.assertEqual([list_address], self.getAllEmailAddresses())
+
+
+def test_suite():
+    return TestLoader().loadTestsFromName(__name__)