Merge lp:~wgrant/launchpad/account-status-api into lp:launchpad

[William Grant]

Log account status changes to account_status_comment, revise +reviewaccount to append to the history, and expose Person.account_status, Person.account_status_comment and Person.setAccountStatus on the webservice.

Log account status changes to account_status_comment, revise +reviewaccount to append to the history, and expose Person.account_status, Person.account_status_comment and Person.setAccountStatus on the webservice.

[Colin Watson (cjwatson)]

review: Approve

=== modified file 'lib/lp/app/browser/tests/test_launchpad.py'
--- lib/lp/app/browser/tests/test_launchpad.py	2012-12-20 22:52:31 +0000
+++ lib/lp/app/browser/tests/test_launchpad.py	2015-01-07 00:40:21 +0000
@@ -358,7 +358,7 @@
         name = 'suspended-person'
         person = self.factory.makePerson(name=name)
         login_person(self.admin)
-        removeSecurityProxy(person).account_status = AccountStatus.SUSPENDED
+        person.setAccountStatus(AccountStatus.SUSPENDED, None, 'Go away')
         segment = '~%s' % name
         # Admins can see the suspended user.
         traversed = self.traverse(segment, segment)

=== modified file 'lib/lp/code/model/tests/test_revision.py'
--- lib/lp/code/model/tests/test_revision.py	2013-08-13 06:53:11 +0000
+++ lib/lp/code/model/tests/test_revision.py	2015-01-07 00:40:21 +0000
@@ -127,7 +127,7 @@
         author = self.factory.makePerson()
         rev = self.factory.makeRevision(
             author=author.preferredemail.email)
-        author.account.status = AccountStatus.SUSPENDED
+        author.setAccountStatus(AccountStatus.SUSPENDED, None, 'spammer!')
         branch = self.factory.makeProductBranch()
         branch.createBranchRevision(1, rev)
         self.assertTrue(rev.karma_allocated)

=== modified file 'lib/lp/registry/browser/person.py'
--- lib/lp/registry/browser/person.py	2014-11-24 01:20:26 +0000
+++ lib/lp/registry/browser/person.py	2015-01-07 00:40:21 +0000
@@ -945,8 +945,8 @@
 
 
 class DeactivateAccountSchema(Interface):
-    comment = copy_field(
-        IPerson['account_status_comment'], readonly=False, __name__='comment')
+    comment = Text(
+        title=_("Why are you deactivating your account?"), required=False)
 
 
 class PersonDeactivateAccountView(LaunchpadFormView):
@@ -1176,13 +1176,19 @@
         self.updateContextFromData(data)
 
 
-class PersonAccountAdministerView(LaunchpadEditFormView):
+class IAccountAdministerSchema(Interface):
+
+    status = copy_field(IAccount['status'], required=True, readonly=False)
+    comment = Text(
+        title=_('Status change comment'), required=True, readonly=False)
+
+
+class PersonAccountAdministerView(LaunchpadFormView):
     """Administer an `IAccount` belonging to an `IPerson`."""
-    schema = IAccount
+    schema = IAccountAdministerSchema
     label = "Review person's account"
-    field_names = ['displayname', 'status', 'status_comment']
-    custom_widget(
-        'status_comment', TextAreaWidget, height=5, width=60)
+    field_names = ['status', 'comment']
+    custom_widget('comment', TextAreaWidget, height=5, width=60)
 
     def __init__(self, context, request):
         """See `LaunchpadEditFormView`."""
@@ -1193,6 +1199,10 @@
         self.context = self.person.account
 
     @property
+    def initial_values(self):
+        return {'status': self.context.status}
+
+    @property
     def is_viewing_person(self):
         """Is the view showing an `IPerson`?
 
@@ -1232,20 +1242,20 @@
     @action('Change', name='change')
     def change_action(self, action, data):
         """Update the IAccount."""
-        if (data['status'] == AccountStatus.SUSPENDED
-            and self.context.status != AccountStatus.SUSPENDED):
+        if data['status'] == self.context.status:
+            return
+        if data['status'] == AccountStatus.SUSPENDED:
             # The preferred email address is removed to ensure no email
             # is sent to the user.
             self.person.setPreferredEmail(None)
             self.request.response.addInfoNotification(
-                u'The account "%s" has been suspended.' % (
-                    self.context.displayname))
-        if (data['status'] == AccountStatus.ACTIVE
-            and self.context.status != AccountStatus.ACTIVE):
+                u'The account "%s" has been suspended.'
+                % self.context.displayname)
+        elif data['status'] == AccountStatus.DEACTIVATED:
             self.request.response.addInfoNotification(
-                u'The user is reactivated. He must use the '
-                u'"forgot password" to log in.')
-        self.updateContextFromData(data)
+                u'The account "%s" is now deactivated. The user can log in '
+                u'to reactivate it.' % self.context.displayname)
+        self.context.setStatus(data['status'], self.user, data['comment'])
 
 
 class PersonVouchersView(LaunchpadFormView):

=== modified file 'lib/lp/registry/browser/tests/person-admin-views.txt'
--- lib/lp/registry/browser/tests/person-admin-views.txt	2012-12-05 18:43:04 +0000
+++ lib/lp/registry/browser/tests/person-admin-views.txt	2015-01-07 00:40:21 +0000
@@ -65,7 +65,7 @@
 The +reviewaccount allows admins to see and edit user account information.
 Non-admins cannot access it.
 
-    >>> view = create_initialized_view(user, '+reviewaccount')
+    >>> view = create_view(user, '+reviewaccount')
     >>> check_permission('launchpad.Admin', view)
     False
 
@@ -79,7 +79,7 @@
     >>> print view.errors
     []
     >>> view.field_names
-    ['displayname', 'status', 'status_comment']
+    ['status', 'comment']
     >>> view.label
     "Review person's account"
 
@@ -99,28 +99,24 @@
 The admin can change the user's account information.
 
     >>> form = {
-    ...     'field.displayname': 'The Zaphod Beeblebrox',
     ...     'field.status': 'ACTIVE',
     ...     'field.actions.change': 'Change',
     ...     }
     >>> view = create_initialized_view(user, '+reviewaccount', form=form)
     >>> print view.errors
     []
-    >>> print user.displayname
-    Zaphod Beeblebrox
 
 An admin can suspend a user's account using the +reviewaccount view. When
 an account is suspended, the preferred email address is disabled.
 
     >>> user.account_status
     <DBItem AccountStatus.ACTIVE, ...>
-    >>> print user.account_status_comment
+    >>> print user.account_status_history
     None
 
     >>> form = {
-    ...     'field.displayname': 'Zaphod Beeblebrox',
     ...     'field.status': 'SUSPENDED',
-    ...     'field.status_comment': "Wanted by the galactic police.",
+    ...     'field.comment': "Wanted by the galactic police.",
     ...     'field.actions.change': 'Change',
     ...     }
     >>> view = create_initialized_view(user, '+reviewaccount', form=form)
@@ -129,23 +125,22 @@
     >>> transaction.commit()
     >>> user.account_status
     <DBItem AccountStatus.SUSPENDED, ...>
-    >>> print user.account_status_comment
-    Wanted by the galactic police.
+    >>> user.account_status_history
+    u'... name16: Active -> Suspended:
+    Wanted by the galactic police.\n'
     >>> print user.preferredemail
     None
 
 No one can force account status to an invalid transition:
 
     >>> form = {
-    ...     'field.displayname': 'Zaphod Beeblebrox',
     ...     'field.status': 'ACTIVE',
-    ...     'field.status_comment': "Zaphod's a hoopy frood.",
+    ...     'field.status_history': "Zaphod's a hoopy frood.",
     ...     'field.actions.change': 'Change',
     ...     }
     >>> view = create_initialized_view(user, '+reviewaccount', form=form)
     >>> [e[2] for e in view.errors]
-    [AccountStatusError(u'The status cannot change
-                        from Suspended Launchpad account to Active account')]
+    [AccountStatusError(u'The status cannot change from Suspended to Active')]
 
 
 An admin can deactivate a suspended user's account too. Unlike the act of
@@ -153,9 +148,8 @@
 user must log in to restore the email addresses using the reactivate step.
 
     >>> form = {
-    ...     'field.displayname': 'Zaphod Beeblebrox',
     ...     'field.status': 'DEACTIVATED',
-    ...     'field.status_comment': "Zaphod's a hoopy frood.",
+    ...     'field.comment': "Zaphod's a hoopy frood.",
     ...     'field.actions.change': 'Change',
     ...     }
     >>> view = create_initialized_view(user, '+reviewaccount', form=form)
@@ -163,7 +157,8 @@
     []
     >>> user.account_status
     <DBItem AccountStatus.DEACTIVATED, ...>
-    >>> print user.account_status_comment
-    Zaphod's a hoopy frood.
+    >>> user.account_status_history
+    u"... name16: Active -> Suspended: Wanted by the galactic police.\n...
+    name16: Suspended -> Deactivated: Zaphod's a hoopy frood.\n"
     >>> print user.preferredemail
     None

=== modified file 'lib/lp/registry/browser/tests/test_person_webservice.py'
--- lib/lp/registry/browser/tests/test_person_webservice.py	2014-11-17 22:32:30 +0000
+++ lib/lp/registry/browser/tests/test_person_webservice.py	2015-01-07 00:40:21 +0000
@@ -8,10 +8,16 @@
 from zope.security.management import endInteraction
 from zope.security.proxy import removeSecurityProxy
 
-from lp.registry.interfaces.person import TeamMembershipStatus
+from lp.registry.interfaces.person import (
+    IPersonSet,
+    TeamMembershipStatus,
+    )
 from lp.registry.interfaces.teammembership import ITeamMembershipSet
+from lp.services.identity.interfaces.account import AccountStatus
+from lp.services.webapp.interfaces import OAuthPermission
 from lp.testing import (
     admin_logged_in,
+    api_url,
     launchpadlib_for,
     login,
     logout,
@@ -61,6 +67,62 @@
         self.assertEqual([], emails)
 
 
+class TestPersonAccountStatus(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def test_account_status_history_restricted(self):
+        person = self.factory.makePerson()
+        registrar = self.factory.makePerson(
+            member_of=[getUtility(IPersonSet).getByName('registry')])
+        removeSecurityProxy(person.account).status_history = u'Test'
+        person_url = api_url(person)
+
+        # A normal user cannot read account_status_history. Not even
+        # their own.
+        body = webservice_for_person(
+                person, permission=OAuthPermission.WRITE_PRIVATE).get(
+            person_url, api_version='devel').jsonBody()
+        self.assertEqual('Active', body['account_status'])
+        self.assertEqual(
+            'tag:launchpad.net:2008:redacted', body['account_status_history'])
+
+        # A member of ~registry can see it all.
+        body = webservice_for_person(
+                registrar, permission=OAuthPermission.WRITE_PRIVATE).get(
+            person_url, api_version='devel').jsonBody()
+        self.assertEqual('Active', body['account_status'])
+        self.assertEqual('Test', body['account_status_history'])
+
+    def test_setAccountStatus(self):
+        person = self.factory.makePerson()
+        registrar = self.factory.makePerson(
+            name='registrar',
+            member_of=[getUtility(IPersonSet).getByName('registry')])
+        person_url = api_url(person)
+
+        # A normal user cannot set even their own account status.
+        webservice = webservice_for_person(
+            person, permission=OAuthPermission.WRITE_PRIVATE)
+        response = webservice.named_post(
+            person_url, 'setAccountStatus', status='Suspended',
+            comment='Go away', api_version='devel')
+        self.assertEqual(401, response.status)
+
+        # A member of ~registry can do what they wish.
+        webservice = webservice_for_person(
+            registrar, permission=OAuthPermission.WRITE_PRIVATE)
+        response = webservice.named_post(
+            person_url, 'setAccountStatus', status='Suspended',
+            comment='Go away', api_version='devel')
+        self.assertEqual(200, response.status)
+        with admin_logged_in():
+            self.assertEqual(AccountStatus.SUSPENDED, person.account_status)
+            self.assertEndsWith(
+                person.account_status_history,
+                'registrar: Active -> Suspended: Go away\n')
+
+
 class TestPersonRepresentation(TestCaseWithFactory):
 
     layer = DatabaseFunctionalLayer

=== modified file 'lib/lp/registry/configure.zcml'
--- lib/lp/registry/configure.zcml	2014-11-24 01:20:26 +0000
+++ lib/lp/registry/configure.zcml	2015-01-07 00:40:21 +0000
@@ -990,6 +990,7 @@
                 set_attributes="displayname icon logo visibility"/>
             <require
                 permission="launchpad.Moderate"
+                interface="lp.registry.interfaces.person.IPersonModerateRestricted"
                 set_attributes="name"/>
             <require
                 permission="launchpad.Special"

=== modified file 'lib/lp/registry/doc/person-account.txt'
--- lib/lp/registry/doc/person-account.txt	2013-05-02 00:40:14 +0000
+++ lib/lp/registry/doc/person-account.txt	2015-01-07 00:40:21 +0000
@@ -49,8 +49,8 @@
     True
     >>> matsubara.account.status
     <DBItem AccountStatus.ACTIVE, ...>
-    >>> matsubara.account.status_comment
-    u'test'
+    >>> removeSecurityProxy(matsubara.account).status_history
+    u'...: Unactivated -> Active: test\n'
     >>> removeSecurityProxy(matsubara.preferredemail).email
     u'[e-mail]'
 
@@ -144,11 +144,12 @@
 
 ...an account status of DEACTIVATED...
 
-    >>> foobar.account_status
+    >>> foobar.account.status
     <DBItem AccountStatus.DEACTIVATED...
 
-    >>> foobar.account_status_comment
-    u"I'm a person who doesn't want to be listed as a Launchpad user."
+    >>> removeSecurityProxy(foobar.account).status_history
+    u"... name16: Active -> Deactivated:
+    I'm a person who doesn't want to be listed as a Launchpad user.\n"
 
 ...to have no team memberships...
 
@@ -222,8 +223,11 @@
     >>> foobar.account.status
     <DBItem AccountStatus.ACTIVE...
 
-    >>> foobar.account.status_comment
-    u'User reactivated the account using reset password.'
+    >>> removeSecurityProxy(foobar.account).status_history
+    u"... name16: Active -> Deactivated: I'm a person
+    who doesn't want to be listed as a Launchpad user.\n...:
+    Deactivated -> Active:
+    User reactivated the account using reset password.\n"
 
 The person name is fixed if it was altered when it was deactivated.
 

=== modified file 'lib/lp/registry/doc/person.txt'
--- lib/lp/registry/doc/person.txt	2014-03-11 10:14:20 +0000
+++ lib/lp/registry/doc/person.txt	2015-01-07 00:40:21 +0000
@@ -858,7 +858,8 @@
 
     >>> from lp.services.identity.interfaces.account import AccountStatus
     >>> dave = personset.getByName('justdave')
-    >>> removeSecurityProxy(dave).account_status = AccountStatus.DEACTIVATED
+    >>> removeSecurityProxy(dave).setAccountStatus(
+    ...     AccountStatus.DEACTIVATED, None, 'gbcw')
     >>> transaction.commit()
     >>> list(personset.findPerson('dave'))
     []
@@ -868,7 +869,8 @@
     Dave Miller (justdave): [u'[e-mail]',
                              u'[e-mail]']
 
-    >>> removeSecurityProxy(dave).account_status = AccountStatus.ACTIVE
+    >>> removeSecurityProxy(dave).setAccountStatus(
+    ...     AccountStatus.ACTIVE, None, 'Welcome back')
     >>> flush_database_updates()
     >>> login(ANONYMOUS)
 

=== modified file 'lib/lp/registry/interfaces/person.py'
--- lib/lp/registry/interfaces/person.py	2014-11-28 22:28:40 +0000
+++ lib/lp/registry/interfaces/person.py	2015-01-07 00:40:21 +0000
@@ -27,6 +27,7 @@
     'ITeamContactAddressForm',
     'ITeamReassignment',
     'ImmutableVisibilityError',
+    'NoAccountError',
     'NoSuchPerson',
     'PersonCreationRationale',
     'PersonalStanding',
@@ -572,12 +573,11 @@
         exported_as='is_valid')
     is_team = exported(
         Bool(title=_('Is this object a team?'), readonly=True))
-    account_status = Choice(
-        title=_("The status of this person's account"), required=False,
-        readonly=True, vocabulary=AccountStatus)
-    account_status_comment = Text(
-        title=_("Why are you deactivating your account?"), required=False,
-        readonly=True)
+    account_status = exported(
+        Choice(
+            title=_("The status of this person's account"), required=True,
+            readonly=True, vocabulary=AccountStatus),
+        as_of='devel')
     visibility = exported(
         Choice(title=_("Visibility"),
                description=_(
@@ -1843,9 +1843,29 @@
         """
 
 
+class IPersonModerateRestricted(Interface):
+    """IPerson attributes that require launchpad.Moderate permission."""
+
+    @call_with(user=REQUEST_USER)
+    @operation_parameters(
+        status=copy_field(IAccount['status']),
+        comment=Text(title=_("Status change comment"), required=True))
+    @export_write_operation()
+    @operation_for_version('devel')
+    def setAccountStatus(status, user, comment):
+        """Set the status of this person's account."""
+
+    account_status_history = exported(
+        Text(
+            title=_("Account status history"), required=False,
+            readonly=True),
+        as_of='devel')
+
+
 class IPerson(IPersonPublic, IPersonLimitedView, IPersonViewRestricted,
-              IPersonEditRestricted, IPersonSpecialRestricted, IHasStanding,
-              ISetLocation, IHeadingContext):
+              IPersonEditRestricted, IPersonModerateRestricted,
+              IPersonSpecialRestricted, IHasStanding, ISetLocation,
+              IHeadingContext):
     """A Person."""
     export_as_webservice_entry(plural_name='people')
 
@@ -2486,6 +2506,11 @@
     """A change in team membership visibility is not allowed."""
 
 
+@error_status(httplib.BAD_REQUEST)
+class NoAccountError(Exception):
+    """The person has no account."""
+
+
 class NoSuchPerson(NameLookupFailed):
     """Raised when we try to look up an IPerson that doesn't exist."""
 

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2014-11-17 22:32:30 +0000
+++ lib/lp/registry/model/person.py	2015-01-07 00:40:21 +0000
@@ -189,6 +189,7 @@
     IPersonSet,
     IPersonSettings,
     ITeam,
+    NoAccountError,
     PersonalStanding,
     PersonCreationRationale,
     TeamEmailAddressError,
@@ -560,34 +561,22 @@
     mugshot = ForeignKey(
         dbName='mugshot', foreignKey='LibraryFileAlias', default=None)
 
-    def _get_account_status(self):
-        account = IStore(Account).get(Account, self.accountID)
-        if account is not None:
-            return account.status
+    @property
+    def account_status(self):
+        if self.account is not None:
+            return self.account.status
         else:
             return AccountStatus.NOACCOUNT
 
-    def _set_account_status(self, value):
-        assert self.accountID is not None, 'No account for this Person'
-        self.account.status = value
-
-    # Deprecated - this value has moved to the Account table.
-    # We provide this shim for backwards compatibility.
-    account_status = property(_get_account_status, _set_account_status)
-
-    def _get_account_status_comment(self):
-        account = IStore(Account).get(Account, self.accountID)
-        if account is not None:
-            return account.status_comment
-
-    def _set_account_status_comment(self, value):
-        assert self.accountID is not None, 'No account for this Person'
-        self.account.status_comment = value
-
-    # Deprecated - this value has moved to the Account table.
-    # We provide this shim for backwards compatibility.
-    account_status_comment = property(
-            _get_account_status_comment, _set_account_status_comment)
+    @property
+    def account_status_history(self):
+        if self.account is not None:
+            return self.account.status_history
+
+    def setAccountStatus(self, status, user, comment):
+        if self.is_team or self.account is None:
+            raise NoAccountError()
+        self.account.setStatus(status, user, comment)
 
     teamowner = ForeignKey(
         dbName='teamowner', foreignKey='Person', default=None,
@@ -2207,10 +2196,9 @@
         return errors
 
     def preDeactivate(self, comment):
+        self.account.setStatus(AccountStatus.DEACTIVATED, self, comment)
         for email in self.validatedemails:
             email.status = EmailAddressStatus.NEW
-        self.account_status = AccountStatus.DEACTIVATED
-        self.account_status_comment = comment
         self.preferredemail.status = EmailAddressStatus.NEW
         del get_property_cache(self).preferredemail
 

=== modified file 'lib/lp/registry/stories/person/xx-admin-person-review.txt'
--- lib/lp/registry/stories/person/xx-admin-person-review.txt	2013-01-16 05:07:37 +0000
+++ lib/lp/registry/stories/person/xx-admin-person-review.txt	2015-01-07 00:40:21 +0000
@@ -67,6 +67,7 @@
     OpenID identifiers: salgado_oid
     Email addresses: [e-mail]
     Guessed email addresses: [e-mail]
+    Status history:
 
 The page also contains a link back to the +review page.
 

=== modified file 'lib/lp/registry/templates/person-review.pt'
--- lib/lp/registry/templates/person-review.pt	2013-01-16 05:07:37 +0000
+++ lib/lp/registry/templates/person-review.pt	2015-01-07 00:40:21 +0000
@@ -78,6 +78,12 @@
                   </tal:emails>
                 </td>
               </tr>
+              <tr>
+                <th>Status history:</th>
+                <td tal:content="structure context/status_history/fmt:text-to-html">
+                  2015-01-05 cprov Active -&gt; Suspended: Suspended for evil.
+                </td>
+              </tr>
             </table>
           </tal:review-account>
         </div>

=== modified file 'lib/lp/registry/tests/test_person.py'
--- lib/lp/registry/tests/test_person.py	2014-10-22 17:32:38 +0000
+++ lib/lp/registry/tests/test_person.py	2015-01-07 00:40:21 +0000
@@ -1348,7 +1348,7 @@
         """
         owner = self.factory.makePerson(email='[e-mail]')
         team = self.factory.makeTeam(owner)
-        owner.account.status = AccountStatus.DEACTIVATED
+        owner.setAccountStatus(AccountStatus.DEACTIVATED, None, 'gbcw')
         self.assertContentEqual([], get_recipients(team))
 
     def test_get_recipients_team_with_disabled_member_account(self):
@@ -1357,7 +1357,7 @@
         See <https://bugs.launchpad.net/launchpad/+bug/855150>
         """
         person = self.factory.makePerson(email='[e-mail]')
-        person.account.status = AccountStatus.DEACTIVATED
+        person.setAccountStatus(AccountStatus.DEACTIVATED, None, 'gbcw')
         team = self.factory.makeTeam(members=[person])
         self.assertContentEqual([team.teamowner], get_recipients(team))
 
@@ -1367,7 +1367,7 @@
         See <https://bugs.launchpad.net/launchpad/+bug/855150>
         """
         person = self.factory.makePerson(email='[e-mail]')
-        person.account.status = AccountStatus.DEACTIVATED
+        person.setAccountStatus(AccountStatus.DEACTIVATED, None, 'gbcw')
         team1 = self.factory.makeTeam(members=[person])
         team2 = self.factory.makeTeam(members=[team1])
         self.assertContentEqual(

=== modified file 'lib/lp/registry/tests/test_personset.py'
--- lib/lp/registry/tests/test_personset.py	2014-11-17 23:52:20 +0000
+++ lib/lp/registry/tests/test_personset.py	2015-01-07 00:40:21 +0000
@@ -552,9 +552,10 @@
         self.assertEqual(person, found_person)
         self.assertEqual(AccountStatus.ACTIVE, person.account.status)
         self.assertTrue(db_updated)
-        self.assertEqual(
-            "when purchasing an application via Software Center.",
-            removeSecurityProxy(person.account).status_comment)
+        self.assertEndsWith(
+            removeSecurityProxy(person.account).status_history,
+            ": Deactivated -> Active: "
+            "when purchasing an application via Software Center.\n")
 
     def test_existing_suspended_account(self):
         # An existing suspended account will raise an exception.

=== modified file 'lib/lp/scripts/tests/test_garbo.py'
--- lib/lp/scripts/tests/test_garbo.py	2014-11-06 02:22:57 +0000
+++ lib/lp/scripts/tests/test_garbo.py	2015-01-07 00:40:21 +0000
@@ -843,12 +843,12 @@
                 1)
 
         account = person.account
-        account.status = status
+        account.setStatus(status, None, 'Go away')
         # We flush because a trigger sets the date_status_set and we need to
         # modify it ourselves.
         Store.of(account).flush()
         if interval is not None:
-            account.date_status_set = interval
+            removeSecurityProxy(account).date_status_set = interval
 
         self.runDaily()
 

=== modified file 'lib/lp/services/identity/configure.zcml'
--- lib/lp/services/identity/configure.zcml	2013-01-16 04:51:47 +0000
+++ lib/lp/services/identity/configure.zcml	2015-01-07 00:40:21 +0000
@@ -53,7 +53,7 @@
             interface="lp.services.identity.interfaces.account.IAccountViewRestricted"/>
         <require
             permission="launchpad.Moderate"
-            set_attributes="status date_status_set status_comment"/>
+            interface="lp.services.identity.interfaces.account.IAccountModerateRestricted"/>
         <require
             permission="launchpad.Edit"
             set_attributes="displayname"/>

=== modified file 'lib/lp/services/identity/doc/account.txt'
--- lib/lp/services/identity/doc/account.txt	2012-12-05 16:16:20 +0000
+++ lib/lp/services/identity/doc/account.txt	2015-01-07 00:40:21 +0000
@@ -81,7 +81,7 @@
 Account objects have a useful string representation.
 
     >>> account
-    <Account 'No Privileges Person' (Active account)>
+    <Account 'No Privileges Person' (Active)>
 
 The account has other metadata.
 
@@ -97,7 +97,8 @@
     >>> account.date_status_set >= account.date_created
     True
 
-    >>> print account.status_comment
+    >>> from zope.security.proxy import removeSecurityProxy
+    >>> print removeSecurityProxy(account).status_history
     None
 
     >>> print account.displayname
@@ -110,16 +111,15 @@
 
     >>> original_date_status_set = account.date_status_set
     >>> login('[e-mail]')
-    >>> account.status = AccountStatus.SUSPENDED
+    >>> account.setStatus(AccountStatus.SUSPENDED, None, 'spammer')
 
     # Shouldn't be necessary with Storm!
-    >>> from zope.security.proxy import removeSecurityProxy
     >>> removeSecurityProxy(account).sync()
     >>> account.date_status_set > original_date_status_set
     True
 
-    >>> account.status = AccountStatus.DEACTIVATED
-    >>> account.status = AccountStatus.ACTIVE
+    >>> account.setStatus(AccountStatus.DEACTIVATED, None, 'welcome')
+    >>> account.setStatus(AccountStatus.ACTIVE, None, 'logged in!')
     >>> login('[e-mail]')
 
 An Account has at least one OpenID identifier used to generate the

=== modified file 'lib/lp/services/identity/interfaces/account.py'
--- lib/lp/services/identity/interfaces/account.py	2013-01-16 04:51:47 +0000
+++ lib/lp/services/identity/interfaces/account.py	2015-01-07 00:40:21 +0000
@@ -11,9 +11,9 @@
     'AccountSuspendedError',
     'AccountCreationRationale',
     'IAccount',
+    'IAccountModerateRestricted',
     'IAccountPublic',
     'IAccountSet',
-    'IAccountSpecialRestricted',
     'IAccountViewRestricted',
     'INACTIVE_ACCOUNT_STATUSES',
     ]
@@ -48,25 +48,25 @@
     """The status of an account."""
 
     NOACCOUNT = DBItem(10, """
-        Unactivated account
+        Unactivated
 
         The account has not yet been activated.
         """)
 
     ACTIVE = DBItem(20, """
-        Active account
+        Active
 
         The account is active.
         """)
 
     DEACTIVATED = DBItem(30, """
-        Deactivated account
+        Deactivated
 
         The account has been deactivated by the account's owner.
         """)
 
     SUSPENDED = DBItem(40, """
-        Suspended Launchpad account
+        Suspended
 
         The account has been suspended by a Launchpad admin.
         """)
@@ -253,7 +253,7 @@
 
     status = AccountStatusChoice(
         title=_("The status of this account"), required=True,
-        readonly=False, vocabulary=AccountStatus)
+        readonly=True, vocabulary=AccountStatus)
 
 
 class IAccountViewRestricted(Interface):
@@ -269,12 +269,7 @@
     openid_identifiers = Attribute(_("Linked OpenId Identifiers"))
 
     date_status_set = Datetime(
-        title=_('Date status last modified.'),
-        required=True, readonly=False)
-
-    status_comment = Text(
-        title=_("Why are you deactivating your account?"),
-        required=False, readonly=False)
+        title=_('Date status last modified.'), required=True, readonly=True)
 
     def reactivate(comment):
         """Activate this account.
@@ -285,7 +280,22 @@
         """
 
 
-class IAccount(IAccountPublic, IAccountViewRestricted):
+class IAccountModerateRestricted(Interface):
+
+    status_history = Text(
+        title=_("Account status comments"), required=False, readonly=True)
+
+    def setStatus(status, user, comment):
+        """Change the status of this account.
+
+        :param user: The user performing the action or None.
+        :param comment: An explanation of the change to be logged in
+            status_history.
+        """
+
+
+class IAccount(IAccountPublic, IAccountViewRestricted,
+               IAccountModerateRestricted):
     """Interface describing an `Account`."""
 
 

=== modified file 'lib/lp/services/identity/model/account.py'
--- lib/lp/services/identity/model/account.py	2013-06-20 05:50:00 +0000
+++ lib/lp/services/identity/model/account.py	2015-01-07 00:40:21 +0000
@@ -9,6 +9,8 @@
     'AccountSet',
     ]
 
+import datetime
+
 from sqlobject import StringCol
 from storm.locals import ReferenceSet
 from zope.interface import implements
@@ -54,7 +56,7 @@
     status = AccountStatusEnumCol(
         enum=AccountStatus, default=AccountStatus.NOACCOUNT, notNull=True)
     date_status_set = UtcDateTimeCol(notNull=True, default=UTC_NOW)
-    status_comment = StringCol(dbName='status_comment', default=None)
+    status_history = StringCol(dbName='status_comment', default=None)
 
     openid_identifiers = ReferenceSet(
         "Account.id", OpenIdIdentifier.account_id)
@@ -64,10 +66,27 @@
         return "<%s '%s' (%s)>" % (
             self.__class__.__name__, displayname, self.status)
 
+    def addStatusComment(self, user, comment):
+        """See `IAccountModerateRestricted`."""
+        prefix = datetime.datetime.utcnow().strftime('%Y-%m-%d %H:%M:%S')
+        if user is not None:
+            prefix += ' %s' % user.name
+        old_lines = (
+            self.status_history.splitlines() if self.status_history else [])
+        self.status_history = '\n'.join(
+            old_lines + ['%s: %s' % (prefix, comment), ''])
+
+    def setStatus(self, status, user, comment):
+        """See `IAccountModerateRestricted`."""
+        comment = comment or ''
+        self.addStatusComment(
+            user, '%s -> %s: %s' % (self.status.title, status.title, comment))
+        # date_status_set is maintained by a DB trigger.
+        self.status = status
+
     def reactivate(self, comment):
         """See `IAccountSpecialRestricted`."""
-        self.status = AccountStatus.ACTIVE
-        self.status_comment = comment
+        self.setStatus(AccountStatus.ACTIVE, None, comment)
 
 
 class AccountSet:

=== modified file 'lib/lp/services/identity/tests/test_account.py'
--- lib/lp/services/identity/tests/test_account.py	2012-12-04 23:31:37 +0000
+++ lib/lp/services/identity/tests/test_account.py	2015-01-07 00:40:21 +0000
@@ -25,14 +25,14 @@
     def test_account_repr_ansii(self):
         # Verify that ANSI displayname is ascii safe.
         distro = self.factory.makeAccount(u'\xdc-account')
-        ignore, displayname, status_1, status_2 = repr(distro).rsplit(' ', 3)
+        ignore, displayname, status = repr(distro).rsplit(' ', 2)
         self.assertEqual("'\\xdc-account'", displayname)
-        self.assertEqual('(Active account)>', '%s %s' % (status_1, status_2))
+        self.assertEqual('(Active)>', status)
 
     def test_account_repr_unicode(self):
         # Verify that Unicode displayname is ascii safe.
         distro = self.factory.makeAccount(u'\u0170-account')
-        ignore, displayname, status_1, status_2 = repr(distro).rsplit(' ', 3)
+        ignore, displayname, status = repr(distro).rsplit(' ', 2)
         self.assertEqual("'\\u0170-account'", displayname)
 
     def assertCannotTransition(self, account, statuses):
@@ -40,13 +40,13 @@
             self.assertFalse(
                 IAccount['status'].bind(account).constraint(status))
             self.assertRaises(
-                AccountStatusError, setattr, account, 'status', status)
+                AccountStatusError, account.setStatus, status, None, 'Go away')
 
     def assertCanTransition(self, account, statuses):
         for status in statuses:
             self.assertTrue(
                 IAccount['status'].bind(account).constraint(status))
-        account.status = status
+        account.setStatus(status, None, 'No reason')
         self.assertEqual(status, account.status)
 
     def test_status_from_noaccount(self):
@@ -69,7 +69,7 @@
         # The status may change from DEACTIVATED to ACTIVATED.
         account = self.factory.makeAccount()
         login_celebrity('admin')
-        account.status = AccountStatus.DEACTIVATED
+        account.setStatus(AccountStatus.DEACTIVATED, None, 'gbcw')
         self.assertCannotTransition(
             account, [AccountStatus.NOACCOUNT, AccountStatus.SUSPENDED])
         self.assertCanTransition(account, [AccountStatus.ACTIVE])
@@ -78,7 +78,7 @@
         # The status may change from SUSPENDED to DEACTIVATED.
         account = self.factory.makeAccount()
         login_celebrity('admin')
-        account.status = AccountStatus.SUSPENDED
+        account.setStatus(AccountStatus.SUSPENDED, None, 'spammer!')
         self.assertCannotTransition(
             account, [AccountStatus.NOACCOUNT, AccountStatus.ACTIVE])
         self.assertCanTransition(account, [AccountStatus.DEACTIVATED])

=== modified file 'lib/lp/soyuz/doc/distribution.txt'
--- lib/lp/soyuz/doc/distribution.txt	2013-08-01 00:14:03 +0000
+++ lib/lp/soyuz/doc/distribution.txt	2015-01-07 00:40:21 +0000
@@ -111,14 +111,14 @@
 
     >>> from lp.services.identity.interfaces.account import AccountStatus
     >>> login('[e-mail]')
-    >>> no_priv.account.status = AccountStatus.SUSPENDED
+    >>> no_priv.setAccountStatus(AccountStatus.SUSPENDED, None, 'spammer!')
 
     >>> result = ubuntu.searchPPAs(text=u'priv', show_inactive=True)
     >>> [archive for archive in result]
     []
 
-    >>> no_priv.account.status = AccountStatus.DEACTIVATED
-    >>> no_priv.account.status = AccountStatus.ACTIVE
+    >>> no_priv.setAccountStatus(AccountStatus.DEACTIVATED, None, 'oops')
+    >>> no_priv.setAccountStatus(AccountStatus.ACTIVE, None, 'login')
 
 
 Retrieving only pending-acceptance PPAs