launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #01757
[Merge] lp:~sinzui/launchpad/merge-karma-1 into lp:launchpad
Curtis Hovey has proposed merging lp:~sinzui/launchpad/merge-karma-1 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
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
--
https://code.launchpad.net/~sinzui/launchpad/merge-karma-1/+merge/39763
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/merge-karma-1 into lp:launchpad.
=== modified file 'database/schema/security.cfg'
--- database/schema/security.cfg 2010-10-26 12:53:04 +0000
+++ database/schema/security.cfg 2010-11-01 17:34:45 +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 17:34:45 +0000
@@ -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:
+ # 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 17:34:45 +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(
