Merge lp:~julian-edwards/launchpad/rename-account-with-ppas into lp:launchpad

[Julian Edwards]

= Summary =
Allow account renaming when all PPAs are deleted

== Proposed fix ==
Change the account +edit form so that the account can be renamed if:
a) the user has no PPAs
b) the user has a PPA but it has no packages
c) the user had PPAs but they are all deleted

== Pre-implementation notes ==
This was pair-programmed with noodles!

== Implementation details ==
Fairly simple steps:
1. The browser setupWidgets() was changed to disabled/enable the name field
appropriately
2. I added a new TestPersonEditView that unit tests setupWidgets.
3. Fixed the ppa-workflow doctest to remove unit test stuff therein.
4. Changed the activation template so that it has a slightly different warning
message.

== Tests ==
bin/test -cvvt xx-ppa-workflow.txt -t TestPersonEditView

== Demo and Q/A ==
I can Q/A this on dogfood before it lands using some dummy accounts and PPAs.
It's a pretty high-profile bug so I don't want to QA it on edge after it lands
and the world sees it.

[Данило Шеган (danilo)]

"if has_packages" should probably be "if has_packages > 0", and the variable name should better reflect that it's a count of published packages for the person

In test_can_rename_with_deleted_PPA, I assume ppa.delete() sets the status to DELETING, so you should probably test that case separately (since DELETING is still not DELETED :): keep the existing test, but add another one for 'deleting' state if that makes sense.

For the existing 'deleted' test you most likely don't even need to call ppa.delete() since you hard-code the status to DELETED anyway, right? If I am smoking crack, just ignore this comment.

Nice job switching to a unit test!

review: Needs Fixing

[Julian Edwards (julian-edwards)]

Thanks for the review!

> "if has_packages" should probably be "if has_packages > 0", and the
> variable name should better reflect that it's a count of published
> packages for the person

Okay, I changed it.

> In test_can_rename_with_deleted_PPA, I assume ppa.delete() sets the status
> to DELETING, so you should probably test that case separately (since
> DELETING is still not DELETED :): keep the existing test, but add another
> one for 'deleting' state if that makes sense.

Good catch! I've added an additional test for this.

> For the existing 'deleted' test you most likely don't even need to call
> ppa.delete() since you hard-code the status to DELETED anyway, right? If
> I am smoking crack, just ignore this comment.

You're smoking crack :)

Deleting a PPA also sets all its publications to DELETED, which is also
necessary to rename your account. I changed a comment to make that clearer in
the test.

> Nice job switching to a unit test!

Thanks!

Partial diff attached.

Cheers
J

[Данило Шеган (danilo)]

Looks good, thanks for all the fish (and crack).

review: Approve

=== modified file 'lib/lp/registry/browser/person.py'
--- lib/lp/registry/browser/person.py	2010-04-19 03:44:27 +0000
+++ lib/lp/registry/browser/person.py	2010-05-11 12:33:46 +0000
@@ -133,6 +133,7 @@
 from canonical.launchpad.browser.launchpad import get_launchpad_views
 from canonical.launchpad.interfaces.account import IAccount
 from canonical.launchpad.interfaces.account import AccountStatus
+from lp.soyuz.interfaces.archive import ArchiveStatus
 from lp.soyuz.interfaces.archivesubscriber import (
     IArchiveSubscriberSet)
 from canonical.launchpad.interfaces.authtoken import LoginTokenType
@@ -3870,14 +3871,19 @@
 
         When a user has a PPA renames are prohibited.
         """
-        writable = self.context.archive is None
-        if not writable:
+        active_ppas = [
+            ppa for ppa in self.context.ppas
+            if ppa.status in (ArchiveStatus.ACTIVE, ArchiveStatus.DELETING)]
+        num_packages = sum(
+            ppa.getPublishedSources().count() for ppa in active_ppas)
+        if num_packages > 0:
             # This makes the field's widget display (i.e. read) only.
             self.form_fields['name'].for_display = True
         super(PersonEditView, self).setUpWidgets()
-        if not writable:
+        if num_packages > 0:
             self.widgets['name'].hint = _(
-                'This user has a PPA and may not be renamed.')
+                'This user has an active PPA with packages published and '
+                'may not be renamed.')
 
     def validate(self, data):
         """If the name changed, warn the user about the implications."""
 
=== modified file 'lib/lp/registry/browser/tests/test_person_view.py'
--- lib/lp/registry/browser/tests/test_person_view.py	2010-04-12 06:03:57 +0000
+++ lib/lp/registry/browser/tests/test_person_view.py	2010-05-11 12:33:46 +0000
@@ -13,8 +13,10 @@
 from lp.registry.interfaces.karma import IKarmaCacheManager
 from canonical.launchpad.webapp.servers import LaunchpadTestRequest
 from canonical.testing import LaunchpadFunctionalLayer, LaunchpadZopelessLayer
-from lp.registry.browser.person import PersonView
+from lp.registry.browser.person import PersonEditView, PersonView
 from lp.registry.model.karma import KarmaCategory
+from lp.soyuz.tests.test_publishing import SoyuzTestPublisher
+from lp.soyuz.interfaces.archive import ArchiveStatus
 from lp.testing import TestCaseWithFactory, login_person
 
 
@@ -168,5 +170,61 @@
         self.failUnless(person_view.should_show_ppa_section)
 
 
+class TestPersonEditView(TestCaseWithFactory):
+
+    layer = LaunchpadFunctionalLayer
+
+    def setUp(self):
+        TestCaseWithFactory.setUp(self)
+        self.person = self.factory.makePerson()
+        login_person(self.person)
+        self.ppa = self.factory.makeArchive(owner=self.person)
+        self.view = PersonEditView(
+            self.person, LaunchpadTestRequest())
+
+    def test_can_rename_with_empty_PPA(self):
+        # If a PPA exists but has no packages, we can rename the
+        # person.
+        self.view.initialize()
+        self.assertFalse(self.view.form_fields['name'].for_display)
+
+    def _publishPPAPackage(self):
+        stp = SoyuzTestPublisher()
+        stp.setUpDefaultDistroSeries()
+        stp.getPubSource(archive=self.ppa)
+
+    def test_cannot_rename_with_non_empty_PPA(self):
+        # Publish some packages in the PPA and test that we can't rename
+        # the person.
+        self._publishPPAPackage()
+        self.view.initialize()
+        self.assertTrue(self.view.form_fields['name'].for_display)
+        self.assertEqual(
+            self.view.widgets['name'].hint,
+            "This user has an active PPA with packages published and "
+            "may not be renamed.")
+
+    def test_cannot_rename_with_deleting_PPA(self):
+        # When a PPA is in the DELETING state we should not allow
+        # renaming just yet.
+        self._publishPPAPackage()
+        self.view.initialize()
+        self.ppa.delete(self.person)
+        self.assertEqual(self.ppa.status, ArchiveStatus.DELETING)
+        self.assertTrue(self.view.form_fields['name'].for_display)
+
+    def test_can_rename_with_deleted_PPA(self):
+        # Delete a PPA and test that the person can be renamed.
+        self._publishPPAPackage()
+        # Deleting the PPA will remove the publications, which is
+        # necessary for the renaming check.
+        self.ppa.delete(self.person)
+        # Simulate the external script running and finalising the
+        # DELETED status.
+        self.ppa.status = ArchiveStatus.DELETED
+        self.view.initialize()
+        self.assertFalse(self.view.form_fields['name'].for_display)
+
+
 def test_suite():
     return unittest.TestLoader().loadTestsFromName(__name__)
 
=== modified file 'lib/lp/soyuz/stories/ppa/xx-ppa-workflow.txt'
--- lib/lp/soyuz/stories/ppa/xx-ppa-workflow.txt	2010-03-26 17:30:33 +0000
+++ lib/lp/soyuz/stories/ppa/xx-ppa-workflow.txt	2010-05-11 12:33:46 +0000
@@ -46,12 +46,13 @@
 text-area.
 
 There is also a message, near the form actions, in which the user is
-warned about the fact the activating this PPA will block renaming of
-the context launchpad user.
+warned about the fact that if the user has any PPAs with published
+packages then they will not be able to rename their account.
 
     >>> print extract_text(
     ...     first_tag_by_class(sample_browser.contents, 'actions'))
-    Activating this PPA will block future renaming of Sample Person
+    If you have any PPAs with published packages, you will not be able to
+    rename ... until those PPAs are deleted.
     ...
 
 'PPA name' and 'Displayname' are required fields.  For the first activated
@@ -149,25 +150,6 @@
     There is 1 error.
     Required input is missing.
 
-Once the PPA is activated, the Sample user account cannot be renamed
-anymore. Changing the account name affects the PPA repository paths
-and we don't have infrastructure in place to support that yet. See
-more information about the feature in bug #87326.
-
-    >>> sample_browser.open("http://launchpad.dev/~name12/+edit")
-    >>> sample_browser.getControl(name='field.name').value = 'el-duderino'
-    Traceback (most recent call last):
-    ...
-    LookupError: name 'field.name'
-
-    >>> print extract_text(
-    ...     first_tag_by_class(sample_browser.contents, 'form'))
-    Display Name:
-    ...
-    Name: name12
-    This user has a PPA and may not be renamed.
-    ...
-
 
 == Activating Personal Package Archives for Teams ==
 
@@ -215,7 +197,8 @@
 
     >>> print extract_text(
     ...     first_tag_by_class(sample_browser.contents, 'actions'))
-    Activating this PPA will block future renaming of Landscape Developers
+    If you have any PPAs with published packages, you will not be able to
+    rename ... until those PPAs are deleted.
     ...
 
 That understood, the PPA gets activated.
@@ -585,15 +568,6 @@
       A short description of this PPA. URLs are allowed and will be rendered
       as links.
 
-Note that, differently than the time when the first PPA was activated,
-this time there is no warning about the fact that the context renaming
-will be blocked. We assumed that, at this point, this is already a
-known aspect.
-
-    >>> print extract_text(
-    ...     first_tag_by_class(cprov_browser.contents, 'actions'))
-    or Cancel
-
 The 'PPA name' field is not pre-filled and if Celso does not fill it then
 an error is raised.
 
 
=== modified file 'lib/lp/soyuz/templates/archive-activate.pt'
--- lib/lp/soyuz/templates/archive-activate.pt	2009-10-12 11:35:06 +0000
+++ lib/lp/soyuz/templates/archive-activate.pt	2010-05-11 12:33:46 +0000
@@ -41,10 +41,11 @@
     </div>
 
     <div class="actions" metal:fill-slot="buttons">
-      <p tal:condition="not: context/ppas">
+      <p>
         <img src="/@@/warning" />
-        Activating this PPA <b>will block</b> future renaming of
-        <tal:context replace="structure context/fmt:link" />
+        If you have any PPAs with published packages, you will not be able to
+        rename <tal:context replace="structure context/fmt:link" /> until
+        those PPAs are deleted.
       </p>
       <input tal:replace="structure view/actions/field.actions.activate/render" />
       or <a tal:attributes="href context/fmt:url">Cancel</a>