launchpad-reviewers team mailing list archive

[Curtis Hovey <curtis.hovey@xxxxxxxxxxxxx>]

Curtis Hovey has proposed merging lp:~sinzui/launchpad/deactivate-deactivated-account-0 into lp:launchpad/devel.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #583390 AssertionError deactivating an account
  https://bugs.launchpad.net/bugs/583390


This is my branch to fix the oops caused by users trying to deactivate their
already deactivated account.

    lp:~sinzui/launchpad/deactivate-deactivated-account-0
    Diff size: 85
    Launchpad bug:
          https://bugs.launchpad.net/bugs/583390
    Test command: ./bin/test -vv -t TestPersonDeactivateAccountView
    Pre-implementation: who
    Target release: 10.06


Do not let user try to deactivate their already deactivated account
--------------------------------------------------------------------

This oops happens when users are hacking the URL or using their browser
history to resubmit the form that deactivated their account in a false
hope that it doing it again will remove the page. The view must handle
this situation


Rules
-----

    * The view must verify the person is active before preceding to the
      deactivate step.


QA
--

    * On staging create a user
    * Visit the deactivate user page and open another tab to the page.
    * Deactivate the user
    * In the other tab deactivate the user
    * Verify you see a message explaining that the user is already deactivated.


Lint
----

Linting changed files:
  lib/lp/registry/browser/person.py
  lib/lp/registry/browser/tests/test_person_view.py


Test
----

    * lib/lp/registry/browser/tests/test_person_view.py
      * Added a basic test to verify that the TestPersonDeactivateAccountView
        works.
      * Added a test to show that an error is returned if the user is not
        Active.


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

    * lib/lp/registry/browser/person.py
      * Added a validate method to TestPersonDeactivateAccountView to ensure
        non-active users are not deactivated.
-- 
https://code.launchpad.net/~sinzui/launchpad/deactivate-deactivated-account-0/+merge/32264
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/deactivate-deactivated-account-0 into lp:launchpad/devel.

=== modified file 'lib/lp/registry/browser/person.py'
--- lib/lp/registry/browser/person.py	2010-08-05 22:12:44 +0000
+++ lib/lp/registry/browser/person.py	2010-08-10 20:56:54 +0000
@@ -1400,6 +1400,11 @@
     label = "Deactivate your Launchpad account"
     custom_widget('comment', TextAreaWidget, height=5, width=60)
 
+    def validate(self, data):
+        """See `LaunchpadFormView`."""
+        if self.context.account_status != AccountStatus.ACTIVE:
+            self.addError('This account is already deactivated.')
+
     @action(_("Deactivate My Account"), name="deactivate")
     def deactivate_action(self, action, data):
         self.context.deactivateAccount(data['comment'])
@@ -3600,8 +3605,8 @@
 
     def add_ssh(self):
         sshkey = self.request.form.get('sshkey')
-	try:
-      	    getUtility(ISSHKeySet).new(self.user, sshkey)
+        try:
+            getUtility(ISSHKeySet).new(self.user, sshkey)
         except SSHKeyAdditionError:
             self.error_message = structured('Invalid public key')
         except SSHKeyCompromisedError:

=== modified file 'lib/lp/registry/browser/tests/test_person_view.py'
--- lib/lp/registry/browser/tests/test_person_view.py	2010-08-02 02:13:52 +0000
+++ lib/lp/registry/browser/tests/test_person_view.py	2010-08-10 20:56:54 +0000
@@ -1,15 +1,14 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2010 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
 
-import unittest
-
 import transaction
 from zope.component import getUtility
 
 from canonical.config import config
 from canonical.launchpad.ftests import ANONYMOUS, login
+from canonical.launchpad.interfaces.account import AccountStatus
 from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
 from lp.app.errors import NotFoundError
 from canonical.launchpad.webapp.servers import LaunchpadTestRequest
@@ -548,5 +547,33 @@
                 self.build.id) in html)
 
 
-def test_suite():
-    return unittest.TestLoader().loadTestsFromName(__name__)
+class TestPersonDeactivateAccountView(TestCaseWithFactory):
+    """Tests for the PersonDeactivateAccountView."""
+
+    layer = DatabaseFunctionalLayer
+    form = {
+        'field.comment': 'Gotta go.',
+        'field.actions.deactivate': 'Deactivate My Account',
+        }
+
+    def test_deactivate_user_active(self):
+        user = self.factory.makePerson()
+        login_person(user)
+        view = create_initialized_view(
+            user, '+deactivate-account', form=self.form)
+        self.assertEqual([], view.errors)
+        notifications = view.request.response.notifications
+        self.assertEqual(1, len(notifications))
+        self.assertEqual(
+            'Your account has been deactivated.', notifications[0].message)
+        self.assertEqual(AccountStatus.DEACTIVATED, user.account_status)
+
+    def test_deactivate_user_already_deactivated(self):
+        deactivated_user = self.factory.makePerson()
+        login_person(deactivated_user)
+        deactivated_user.deactivateAccount('going.')
+        view = create_initialized_view(
+            deactivated_user, '+deactivate-account', form=self.form)
+        self.assertEqual(1, len(view.errors))
+        self.assertEqual(
+            'This account is already deactivated.', view.errors[0])