Merge lp:~sinzui/launchpad/delete-merged-account into lp:launchpad

[Curtis Hovey]

This is my branch to hide the accounts of merged persons.

    lp:~sinzui/launchpad/delete-merged-account
    Diff size: 119
    Launchpad bug: https://bugs.launchpad.net/bugs/575317
    Test command: ./bin/test -vv -t person-merge
    Pre-implementation: mars. salgado
    Target release: 10.05

Hide the accounts of merged persons
-----------------------------------

Recent changes to SSO require Launchpad to delete the accounts of merged
users.

ISD has its own database now and it is not aware of Launchpad person merges.
The summary of the long conversation is that Lp uses the OpenID identifier
and falls back to email address. Since the merged account has the SSO OpenID
identifier, the wrong account is used. We need to prevent Launchpad from
matching the identifier to an account of a merged person. Launchpad correctly
falls back to email address when there is no account and will update that
account's identifier as needed.

We considered deleting the account, but there are many relationships that are
tied to account. We decided to change the Launchpad account's openid
identifier to one that cannot match any identifier sent from SSO.

Rules
-----

    * Change the account's openid_identifier at the end of Person.merge().
    * Prepare a script to delete delete all merged accounts to
      fix the data.

QA
--

    * On staging, merge an account.
    * Verify it is not in the database afterwards
    * Login using the merged account's email address
    * Verify that you login to the remaining account.

Lint
----

Linting changed files:
  database/schema/pending/update-merged-person-accounts.sql
  lib/lp/registry/doc/person-merge.txt
  lib/lp/registry/model/person.py

Test
----

    * lib/lp/registry/doc/person-merge.txt
      * Updated the test to verify that openid_identifier of the merged
        person is changed.

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

    * database/schema/pending/update-merged-person-accounts.sql
      * Added a script to fix the existing data, 1500+ accounts.
    * lib/lp/registry/model/person.py
      * Updated Person.merge() to change the openid_identifier of the merged
        person.

[Abel Deuring (adeuring)]

Hi Curtis,

a nice branch. I just have a few nitpicks, see below.

> === added file 'database/schema/pending/update-merged-person-accounts.sql'
> --- database/schema/pending/update-merged-person-accounts.sql 1970-01-01 00:00:00 +0000
> +++ database/schema/pending/update-merged-person-accounts.sql 2010-05-07 14:10:52 +0000
> @@ -0,0 +1,24 @@
> +-- 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
> + Account.displayname,
> + Account.openid_identifier,
> + 'merged-' || Account.openid_identifier
> +FROM Account
> + JOIN Person ON Account.id = Person.account
> +WHERE
> + Person.merged IS NOT NULL
> +;

I think we don't need the SELECT above.

> +
> +-- Change the openid_identifier of merged Persons to prevent the merged
> +-- person from being selected during login.
> +UPDATE Account
> +SET openid_identifier = 'merged-' || openid_identifier
> +FROM Person
> +WHERE
> + Account.id = Person.account
> + AND Person.merged IS NOT NULL
> +;
>
> === modified file 'lib/lp/registry/doc/person-merge.txt'
> --- lib/lp/registry/doc/person-merge.txt 2010-02-16 20:36:48 +0000
> +++ lib/lp/registry/doc/person-merge.txt 2010-05-07 14:10:52 +0000
> @@ -161,7 +161,7 @@
> >>> sample.karma == sampleperson_old_karma
> True
>
> -A merged account gets a -merged suffix on its name.
> +A merged person gets a -merged suffix on its name.
>
> >>> from storm.store import Store
> >>> store = Store.of(marilize)
> @@ -186,8 +186,8 @@
> >>> results.get_one()[0]
> u'name12'
>
> -The account that has been merged is flagged. We can use this to eliminate
> -merged accounts from lists etc.
> +The person that has been merged is flagged. We can use this to eliminate
> +merged person from lists etc.

s/person/persons/

review: Approve (code)

=== added file 'database/schema/pending/update-merged-person-accounts.sql'
--- database/schema/pending/update-merged-person-accounts.sql	1970-01-01 00:00:00 +0000
+++ database/schema/pending/update-merged-person-accounts.sql	2010-05-07 20:31:27 +0000
@@ -0,0 +1,24 @@
+-- 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 
+--    Account.displayname,
+--    Account.openid_identifier,
+--    'merged-' || Account.openid_identifier
+--FROM Account
+--    JOIN Person ON Account.id = Person.account
+--WHERE
+--    Person.merged IS NOT NULL
+--;
+
+-- Change the openid_identifier of merged Persons to prevent the merged
+-- person from being selected during login.
+UPDATE Account
+SET openid_identifier = 'merged-' || openid_identifier
+FROM Person
+WHERE
+    Account.id = Person.account
+    AND Person.merged IS NOT NULL
+;

=== modified file 'lib/lp/registry/doc/person-merge.txt'
--- lib/lp/registry/doc/person-merge.txt	2010-02-16 20:36:48 +0000
+++ lib/lp/registry/doc/person-merge.txt	2010-05-07 20:31:27 +0000
@@ -161,7 +161,7 @@
     >>> sample.karma == sampleperson_old_karma
     True
 
-A merged account gets a -merged suffix on its name.
+A merged person gets a -merged suffix on its name.
 
     >>> from storm.store import Store
     >>> store = Store.of(marilize)
@@ -186,8 +186,8 @@
     >>> results.get_one()[0]
     u'name12'
 
-The account that has been merged is flagged. We can use this to eliminate
-merged accounts from lists etc.
+The person that has been merged is flagged. We can use this to eliminate
+merged persons from lists etc.
 
     >>> results = store.execute(
     ...     "SELECT merged FROM Person WHERE name='marilize-merged'")
@@ -199,6 +199,14 @@
     >>> results.get_one()[0] is None
     True
 
+The openid_identifier of the merged account is changed so that if the user
+logs in using that SSO account, Launchpad will select the Person by email
+instead of OpenID. Launchpad will update the OpenID of the remaining account
+if needed.
+
+    >>> print marilize.account.openid_identifier
+    merged-marilize_oid
+
 An email is sent to the user informing him that he should review his email
 and mailing list subscription settings.
 

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2010-04-29 23:06:26 +0000
+++ lib/lp/registry/model/person.py	2010-05-07 20:31:27 +0000
@@ -277,12 +277,6 @@
     mugshot = ForeignKey(
         dbName='mugshot', foreignKey='LibraryFileAlias', default=None)
 
-    # XXX StuartBishop 2008-05-13 bug=237280: The password,
-    # account_status and account_status_comment properties should go. Note
-    # that they override the current strict controls on Account, allowing
-    # access via Person to use the less strict controls on that interface.
-    # Part of the process of removing these methods from Person will be
-    # loosening the permissions on Account or fixing the callsites.
     def _get_password(self):
         # We have to remove the security proxy because the password is
         # needed before we are authenticated. I'm not overly worried because
@@ -3354,7 +3348,7 @@
             # from being merged.
             ('mailinglist', 'team'),
             # I don't think we need to worry about the votecast and vote
-            # tables, because a real human should never have two accounts
+            # tables, because a real human should never have two profiles
             # in Launchpad that are active members of a given team and voted
             # in a given poll. -- GuilhermeSalgado 2005-07-07
             # We also can't afford to change poll results after they are
@@ -3507,7 +3501,7 @@
             UPDATE Person SET merged=%(to_id)d WHERE id=%(from_id)d
             ''' % vars())
 
-        # Append a -merged suffix to the account's name.
+        # Append a -merged suffix to the person's name.
         name = base = "%s-merged" % from_person.name.encode('ascii')
         cur.execute("SELECT id FROM Person WHERE name = %s" % sqlvalues(name))
         i = 1
@@ -3523,6 +3517,16 @@
         # flush its caches.
         store.invalidate()
 
+        # Change the merged Account's OpenID identifier so that it cannot be
+        # used to lookup the merged Person; Launchpad will instead select the
+        # remaining Person based on the email address.
+        if from_person.account is not None:
+            account = IMasterObject(from_person.account)
+            # This works because dashes do not occur in SSO identifiers.
+            merge_identifier = 'merged-%s' % removeSecurityProxy(
+                account).openid_identifier
+            removeSecurityProxy(account).openid_identifier = merge_identifier
+
         # Inform the user of the merge changes.
         if not to_person.isTeam():
             mail_text = get_email_template('person-merged.txt')