Merge lp:~sinzui/launchpad/merge-karma-1 into lp:launchpad/db-devel

[Curtis Hovey]

This is my branch to fix karma left on merged accounts.

    lp:~sinzui/launchpad/merge-karma-1
    Diff size: 250
    Launchpad bug:
        https://bugs.launchpad.net/bugs/190242
        https://bugs.launchpad.net/bugs/3189
        https://bugs.launchpad.net/bugs/301750
    Test command: ./bin/test -vv -t TestPersonSetMerge
    Pre-implementation: Edwin
    Target release: 10.11

Fix karma left on merged accounts
---------------------------------

Merged profiles appear in the top contributors for a project because the
karma cache was not purged. The links are 404 errors (190242). User often
believe karma was lost by the merge because the karma total cache was not
updated by merge. User think they lost 1000's of karma points (3189). Users
are disappointed to see that the member since date is not updated when
merging an older profile with a younger one. (301750)

Rules
-----

    * Delete the KarmaCache and KarmaTotalCache for the merged profile
    * Add the karmaTotalcache from the merged profile so that the karma
      points look like they are combined. This is not accurate; the
      daily job will recalculate the KarmaTotalCache correctly with 24 hours.
    * Use the MIN/LEAST functions to choose the oldest date of the two
      profiles.

QA
--

On staging. Choose to merge an older profile with lots of karma that
also appears in a project top contributors list
    * Merge the profile.
    * Verify the merged profile does not appear in the top contributors
      list. (The preserved profile will not appear until karma is
      recalculated)
    * Verify that the preserved user has more karma points on its profile
      page.
    * Verify that the preserved user has the older date.

Lint
----

Linting changed files:
  database/schema/security.cfg
  lib/lp/registry/model/person.py
  lib/lp/registry/tests/test_person.py

Test
----

Added tests for the two karma merging conditions and the oldest datecreated
condition.
    * lib/lp/registry/tests/test_person.py

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

Added a method to update/delete KarmaCache and KarmaTotalCache. Added
a method to merge the oldest datecreated.
    * lib/lp/registry/model/person.py

Allow the webapp to delete or update KarmaCache and KarmaTotalCache.
    * database/schema/security.cfg

[Aaron Bentley (abentley)]

Please change "if result" to "if result is None" on line 33. Otherwise, looks good.

review: Approve

[Robert Collins (lifeless)]

On Tue, Nov 2, 2010 at 7:17 AM, Aaron Bentley <email address hidden> wrote:
> Review: Approve
> Please change "if result" to "if result is None" on line 33.  Otherwise, looks good.

That would be a bug. The correct spelling is either
if result:
or
if result is not None and len(result) > 0:

if result:
is faster and logically equivalent - writing it long hand gains nothing.

[Robert Collins (lifeless)]

On Tue, Nov 2, 2010 at 8:11 AM, Robert Collins
<email address hidden> wrote:
> That would be a bug. The correct spelling is either
> if result:
> or
> if result is not None and len(result) > 0:
>
> if result:
> is faster and logically equivalent - writing it long hand gains nothing.

Blah, I didn't mean to be quite so black-and-white there.

I know we have a coding style guideline that said to avoid 'if foo:',
but there were two issues in the review:
- the sense of the test was inverted (from if foo to if is None)
- the change from __nonzero__ to a less complete condition which can
break if the list is empty.

The former one obviously matters most. The __nonzero__ angle I haven't
had the time to debate in our style guide yet, though I do feel
strongly about it - we've had numerous bugs because we *didn't use*
__nonzero__ for tests. We've also had bugs because we did :) - I think
we will benefit by removing the guideline altogether and just using
the most appropriate spelling at any one place.

-Rob

[Aaron Bentley (abentley)]

Sorry, omission of "not" is is a typo that didn't appear in my on-call review. However, writing it long hand is considered more explicit and is recommended by our current style: https://dev.launchpad.net/PythonStyleGuide#Truth%20conditionals a

[Aaron Bentley (abentley)]

IMO, "not None" is more precise and explicit, not less complete. If fetchone returns an empty list, it is violating its contract (http://www.python.org/dev/peps/pep-0249/), and that should be treated as an error, not as though there were no results.

=== modified file 'database/schema/security.cfg'
--- database/schema/security.cfg	2010-10-26 12:53:04 +0000
+++ database/schema/security.cfg	2010-11-01 18:59:50 +0000
@@ -175,9 +175,9 @@
 public.hwvendorname                     = SELECT
 public.incrementaldiff                  = SELECT, INSERT, UPDATE, DELETE
 public.job                              = SELECT, INSERT, UPDATE, DELETE
-public.karmacache                       = SELECT
+public.karmacache                       = SELECT, DELETE
 public.karmacategory                    = SELECT
-public.karmatotalcache                  = SELECT
+public.karmatotalcache                  = SELECT, DELETE, UPDATE
 public.language                         = SELECT
 public.languagepack                     = SELECT, INSERT, UPDATE
 public.launchpadstatistic               = SELECT

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2010-10-29 22:14:33 +0000
+++ lib/lp/registry/model/person.py	2010-11-01 18:59:50 +0000
@@ -3778,7 +3778,7 @@
                         'AND team = %s'
                         % sqlvalues(to_id, team_id))
             result = cur.fetchone()
-            if result:
+            if result is not None:
                 current_status = result[0]
                 # Now we can safely delete from_person's membership record,
                 # because we know to_person has a membership entry for this
@@ -3823,6 +3823,45 @@
                     'DELETE FROM TeamParticipation WHERE person = %s AND '
                     'team = %s' % sqlvalues(from_id, team_id))
 
+    def _mergeKarmaCache(self, cur, from_id, to_id, from_karma):
+        # Merge the karma total cache so the user does not think the karma
+        # was lost.
+        if from_karma > 0:
+            cur.execute('''
+                SELECT karma_total FROM KarmaTotalCache
+                WHERE person = %(to_id)d
+                ''' % vars())
+            result = cur.fetchone()
+            if result is not None:
+                # Add the karma to the remaining user.
+                karma_total = from_karma + result[0]
+                cur.execute('''
+                    UPDATE KarmaTotalCache SET karma_total = %(karma_total)d
+                    WHERE person = %(to_id)d
+                    ''' % vars())
+            else:
+                # Make the existing karma belong to the remaining user.
+                cur.execute('''
+                    UPDATE KarmaTotalCache SET person = %(to_id)d
+                    WHERE person = %(from_id)d
+                    ''' % vars())
+        # Delete the old caches; the daily job will build them later.
+        cur.execute('''
+            DELETE FROM KarmaTotalCache WHERE person = %(from_id)d
+            ''' % vars())
+        cur.execute('''
+            DELETE FROM KarmaCache WHERE person = %(from_id)d
+            ''' % vars())
+
+    def _mergeDateCreated(self, cur, from_id, to_id):
+        cur.execute('''
+            UPDATE Person
+            SET datecreated = (
+                SELECT MIN(datecreated) FROM Person
+                WHERE id in (%(to_id)d, %(from_id)d) LIMIT 1)
+            WHERE id = %(to_id)d
+            ''' % vars())
+
     def merge(self, from_person, to_person):
         """See `IPersonSet`."""
         # Sanity checks
@@ -3861,8 +3900,6 @@
             ('personlanguage', 'person'),
             ('person', 'merged'),
             ('emailaddress', 'person'),
-            ('karmacache', 'person'),
-            ('karmatotalcache', 'person'),
             # Polls are not carried over when merging teams.
             ('poll', 'team'),
             # We can safely ignore the mailinglist table as there's a sanity
@@ -3997,6 +4034,12 @@
         self._mergeWebServiceBan(cur, from_id, to_id)
         skip.append(('webserviceban', 'person'))
 
+        self._mergeKarmaCache(cur, from_id, to_id, from_person.karma)
+        skip.append(('karmacache', 'person'))
+        skip.append(('karmatotalcache', 'person'))
+
+        self._mergeDateCreated(cur, from_id, to_id)
+
         # Sanity check. If we have a reference that participates in a
         # UNIQUE index, it must have already been handled by this point.
         # We can tell this by looking at the skip list.

=== modified file 'lib/lp/registry/tests/test_person.py'
--- lib/lp/registry/tests/test_person.py	2010-10-28 10:18:31 +0000
+++ lib/lp/registry/tests/test_person.py	2010-11-01 18:59:50 +0000
@@ -55,7 +55,10 @@
     PersonVisibility,
     )
 from lp.registry.interfaces.product import IProductSet
-from lp.registry.model.karma import KarmaCategory
+from lp.registry.model.karma import (
+    KarmaCategory,
+    KarmaTotalCache,
+    )
 from lp.registry.model.person import Person
 from lp.registry.model.structuralsubscription import StructuralSubscription
 from lp.services.openid.model.openididentifier import OpenIdIdentifier
@@ -447,7 +450,46 @@
         self.assertEqual(person1, person2)
 
 
-class TestPersonSetMerge(TestCaseWithFactory):
+class KarmaTestMixin:
+    """Helper methods for setting karma."""
+
+    def _makeKarmaCache(self, person, product, category_name_values):
+        """Create a KarmaCache entry with the given arguments.
+
+        In order to create the KarmaCache record we must switch to the DB
+        user 'karma'. This invalidates the objects under test so they
+        must be retrieved again.
+        """
+        transaction.commit()
+        reconnect_stores('karmacacheupdater')
+        total = 0
+        # Insert category total for person and project.
+        for category_name, value in category_name_values:
+            category = KarmaCategory.byName(category_name)
+            self.cache_manager.new(
+                value, person.id, category.id, product_id=product.id)
+            total += value
+        # Insert total cache for person and project.
+        self.cache_manager.new(
+            total, person.id, None, product_id=product.id)
+        transaction.commit()
+        reconnect_stores('launchpad')
+
+    def _makeKarmaTotalCache(self, person, total):
+        """Create a KarmaTotalCache entry.
+
+        In order to create the KarmaTotalCache record we must switch to the DB
+        user 'karma'. This invalidates the objects under test so they
+        must be retrieved again.
+        """
+        transaction.commit()
+        reconnect_stores('karmacacheupdater')
+        KarmaTotalCache(person=person.id, karma_total=total)
+        transaction.commit()
+        reconnect_stores('launchpad')
+
+
+class TestPersonSetMerge(TestCaseWithFactory, KarmaTestMixin):
     """Test cases for PersonSet merge."""
 
     layer = DatabaseFunctionalLayer
@@ -495,6 +537,60 @@
         self.assertIn(duplicate_identifier, merged_identifiers)
         self.assertIn(person_identifier, merged_identifiers)
 
+    def test_karmacache_transferred_to_user_has_no_karma(self):
+        # Verify that the merged user has no KarmaCache entries,
+        # and the karma total was transfered.
+        self.cache_manager = getUtility(IKarmaCacheManager)
+        product = self.factory.makeProduct()
+        duplicate = self.factory.makePerson()
+        self._makeKarmaCache(
+            duplicate, product, [('bugs', 10)])
+        self._makeKarmaTotalCache(duplicate, 15)
+        # The karma changes invalidated duplicate instance.
+        duplicate = self.person_set.get(duplicate.id)
+        person = self.factory.makePerson()
+        self._do_premerge(duplicate, person)
+        login_person(person)
+        self.person_set.merge(duplicate, person)
+        self.assertEqual([], duplicate.karma_category_caches)
+        self.assertEqual(0, duplicate.karma)
+        self.assertEqual(15, person.karma)
+
+    def test_karmacache_transferred_to_user_has_karma(self):
+        # Verify that the merged user has no KarmaCache entries,
+        # and the karma total was summed.
+        self.cache_manager = getUtility(IKarmaCacheManager)
+        product = self.factory.makeProduct()
+        duplicate = self.factory.makePerson()
+        self._makeKarmaCache(
+            duplicate, product, [('bugs', 10)])
+        self._makeKarmaTotalCache(duplicate, 15)
+        person = self.factory.makePerson()
+        self._makeKarmaCache(
+            person, product, [('bugs', 9)])
+        self._makeKarmaTotalCache(person, 13)
+        # The karma changes invalidated duplicate and person instances.
+        duplicate = self.person_set.get(duplicate.id)
+        person = self.person_set.get(person.id)
+        self._do_premerge(duplicate, person)
+        login_person(person)
+        self.person_set.merge(duplicate, person)
+        self.assertEqual([], duplicate.karma_category_caches)
+        self.assertEqual(0, duplicate.karma)
+        self.assertEqual(28, person.karma)
+
+    def test_person_date_created_preserved(self):
+        # Verify that the oldest datecreated is merged.
+        person = self.factory.makePerson()
+        duplicate = self.factory.makePerson()
+        oldest_date = datetime(
+            2005, 11, 25, 0, 0, 0, 0, pytz.timezone('UTC'))
+        removeSecurityProxy(duplicate).datecreated = oldest_date
+        self._do_premerge(duplicate, person)
+        login_person(person)
+        self.person_set.merge(duplicate, person)
+        self.assertEqual(oldest_date, person.datecreated)
+
 
 class TestPersonSetCreateByOpenId(TestCaseWithFactory):
     layer = DatabaseFunctionalLayer
@@ -860,7 +956,7 @@
             assignee=self.user)
 
 
-class TestPersonKarma(TestCaseWithFactory):
+class TestPersonKarma(TestCaseWithFactory, KarmaTestMixin):
 
     layer = DatabaseFunctionalLayer
 
@@ -878,28 +974,6 @@
         self._makeKarmaCache(
             self.person, self.c_product, [('code', 100), (('bugs', 50))])
 
-    def _makeKarmaCache(self, person, product, category_name_values):
-        """Create a KarmaCache entry with the given arguments.
-
-        In order to create the KarmaCache record we must switch to the DB
-        user 'karma'. This requires a commit and invalidates the product
-        instance.
-        """
-        transaction.commit()
-        reconnect_stores('karmacacheupdater')
-        total = 0
-        # Insert category total for person and project.
-        for category_name, value in category_name_values:
-            category = KarmaCategory.byName(category_name)
-            self.cache_manager.new(
-                value, person.id, category.id, product_id=product.id)
-            total += value
-        # Insert total cache for person and project.
-        self.cache_manager.new(
-            total, person.id, None, product_id=product.id)
-        transaction.commit()
-        reconnect_stores('launchpad')
-
     def test__getProjectsWithTheMostKarma_ordering(self):
         # Verify that pillars are ordered by karma.
         results = removeSecurityProxy(