launchpad-reviewers team mailing list archive

[Curtis Hovey <curtis.hovey@xxxxxxxxxxxxx>]

Curtis Hovey has proposed merging lp:~sinzui/launchpad/delete-team-2 into lp:launchpad/devel.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #619022 Cannot delete a team with a purged mailing list
  https://bugs.launchpad.net/bugs/619022


This is my branch to allow renamed teams with purged mailing lists to set
their contact address.

    lp:~sinzui/launchpad/delete-team-2
    Diff size: 62
    Launchpad bug:
          https://bugs.launchpad.net/bugs/619022
    Test command: ./bin/test -vv -t TestTeamContactAddress
    Pre-implementation: EdwinGrubbs
    Target release: 10.09


Allow renamed teams with purged mailing lists to set their contact address
--------------------------------------------------------------------------

This team had a list purged, then it was renamed. The team has the old mailing
list address but the mailing list does not. This is expected.
setContactAddress should not assume that it will always have an email address
for a mailing list. It is okay to be None, since the deletion loop does the
right thing, but the MasterStore step is not needed.

There was some discussion about whether the email address should be deleted
when the list is purged. It could be, but we need a reallocation mechanism
if the list is recreated. This issue was caused by a rename, and we still
have many examples where renaming persons breaks other things. We really
want to make rename safe for mailing lists and PPAs. Since renames can happen
via the SQL, we cannot entertain such an enhancement until we have a UI
that supports all the cases.


Rules
-----

    * Check is mailing_list_email is not None before getting the master store.


QA
--

    * Visit https://edge.launchpad.net/~ubuntu-l10n-nds
    * Delete the team (which implicitly sets the address to None).
    * Verify you see the people and teams search page.


Lint
----

Paste Linting changed files:
  lib/lp/registry/model/person.py
  lib/lp/registry/tests/test_team.py


I can clean up this file after the review.
./lib/lp/registry/model/person.py
      66: 'safe_hasattr' imported but unused
      64: E501 line too long (83 characters)
      68: E501 line too long (80 characters)
    1450: W291 trailing whitespace
    1491: W291 trailing whitespace
    1492: E501 line too long (82 characters)
    1522: E301 expected 1 blank line, found 0
    1559: E501 line too long (82 characters)
      64: Line exceeds 78 characters.
      68: Line exceeds 78 characters.
    1450: Line has trailing whitespace.
    1478: Line exceeds 78 characters.
    1491: Line has trailing whitespace.
    1492: Line exceeds 78 characters.
    1559: Line exceeds 78 characters. lint report


Test
----

    * lib/lp/registry/tests/test_team.py
      * Added a test that repeats the events that happened to the problem
        team and verified it can set its contact address.


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

    * lib/lp/registry/model/person.py
      * Added check to see if the address is None before

-- 
__Curtis C. Hovey_________
http://launchpad.net/
-- 
https://code.launchpad.net/~sinzui/launchpad/delete-team-2/+merge/32948
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/delete-team-2 into lp:launchpad/devel.

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2010-08-17 13:53:04 +0000
+++ lib/lp/registry/model/person.py	2010-08-18 03:26:00 +0000
@@ -2179,7 +2179,8 @@
         if self.mailing_list is not None:
             mailing_list_email = getUtility(IEmailAddressSet).getByEmail(
                 self.mailing_list.address)
-            mailing_list_email = IMasterObject(mailing_list_email)
+            if mailing_list_email is not None:
+                mailing_list_email = IMasterObject(mailing_list_email)
         else:
             mailing_list_email = None
         all_addresses = IMasterStore(self).find(

=== modified file 'lib/lp/registry/tests/test_team.py'
--- lib/lp/registry/tests/test_team.py	2010-04-19 21:16:12 +0000
+++ lib/lp/registry/tests/test_team.py	2010-08-18 03:26:00 +0000
@@ -6,7 +6,6 @@
 __metaclass__ = type
 
 import transaction
-from unittest import TestLoader
 
 from zope.component import getUtility
 
@@ -15,7 +14,8 @@
 from canonical.launchpad.interfaces.lpstorm import IMasterStore
 from canonical.testing import DatabaseFunctionalLayer
 
-from lp.testing import TestCaseWithFactory
+from lp.registry.interfaces.mailinglist import MailingListStatus
+from lp.testing import login_person, login_celebrity, TestCaseWithFactory
 
 
 class TestTeamContactAddress(TestCaseWithFactory):
@@ -81,6 +81,22 @@
         self.assertEqual(None, self.team.preferredemail)
         self.assertEqual([list_address], self.getAllEmailAddresses())
 
-
-def test_suite():
-    return TestLoader().loadTestsFromName(__name__)
+    def test_setContactAddress_after_purged_mailing_list_and_rename(self):
+        # This is the rare case where a list is purged for a team rename,
+        # then the contact address is set/unset sometime afterwards.
+        # The old mailing list address belongs the the team, but not the list.
+        # 1. Create then purge a mailing list.
+        list_address = self.createMailingListAndGetAddress()
+        mailing_list = self.team.mailing_list
+        mailing_list.deactivate()
+        mailing_list.transitionToStatus(MailingListStatus.INACTIVE)
+        mailing_list.purge()
+        transaction.commit()
+        # 2. Rename the team.
+        login_celebrity('admin')
+        self.team.name = 'beta'
+        login_person(self.team.teamowner)
+        # 3. Set the contact address.
+        self.team.setContactAddress(None)
+        self.assertEqual(None, self.team.preferredemail)
+        self.assertEqual([], self.getAllEmailAddresses())