Merge lp:~thumper/launchpad/revision-karma-fix into lp:launchpad

[Tim Penhey]

Fix the allocation of karma for revisions

[Tim Penhey (thumper)]

The revision allocation script has been broken for a little while. It gets itself into an infinite loop. I have had the LOSAs kill it for now.

The fundamental problem was someone fixing Revision.getBranch to be source package aware. Unfortunately the allocation of karma wasn't, and so as soon as a revision that was in a product branch (which is what the query checked for) but the Revision.getBranch returned a package branch, the script got in a loop.

This branch has the following fixes:
 * factory.makeRevision can now take a Person for the author, and will use that person's email address
 * the revision karma allocation script logs at the start of the run how many revisions it thinks it needs to allocate karma for
 * getting revision needing karma allocated now check for related package branches
 * the actual karma allocation is done by the branch target, and the method returns the karma object

[Michael Hudson-Doyle (mwhudson)]

Generally, I like the changes.

> === modified file 'lib/lp/code/model/revision.py'
> --- lib/lp/code/model/revision.py 2009-12-08 04:03:30 +0000
> +++ lib/lp/code/model/revision.py 2010-01-05 02:10:14 +0000
> @@ -86,10 +86,8 @@
> """See `IRevision`."""
> # If we know who the revision author is, give them karma.
> author = self.revision_author.person
> - if (author is not None and branch.product is not None):
> - # No karma for junk branches as we need a product to link
> - # against.
> - karma = author.assignKarma('revisionadded', branch.product)
> + if (author is not None):

These parens can go now.

> + karma = branch.target.assignKarma(author, 'revisionadded')
> # Backdate the karma to the time the revision was created. If the
> # revision_date on the revision is in future (for whatever weird
> # reason) we will use the date_created from the revision (which
> @@ -98,8 +96,12 @@
> # is lying), and a problem with the way the Launchpad code
> # currently does its karma degradation over time.
> if karma is not None:
> - karma.datecreated = min(self.revision_date, self.date_created)
> + removeSecurityProxy(karma).datecreated = min(
> + self.revision_date, self.date_created)
> self.karma_allocated = True
> + return karma
> + else:
> + return None
>
> def getBranch(self, allow_private=False, allow_junk=True):
> """See `IRevision`."""
> @@ -385,9 +387,6 @@
> from lp.registry.model.person import ValidPersonCache
>
> store = getUtility(IStoreSelector).get(MAIN_STORE, DEFAULT_FLAVOR)
> -
> - # XXX: Tim Penhey 2008-08-12, bug 244768
> - # Using Not(column == None) rather than column != None.
> return store.find(
> Revision,
> Revision.revision_author == RevisionAuthor.id,
> @@ -397,7 +396,8 @@
> Select(True,
> And(BranchRevision.revision == Revision.id,
> BranchRevision.branch == Branch.id,
> - Not(Branch.product == None)),
> + Or(Branch.product != None,
> + Branch.distroseries != None)),
> (Branch, BranchRevision))))
>
> @staticmethod

> === modified file 'lib/lp/code/scripts/revisionkarma.py'
> --- lib/lp/code/scripts/revisionkarma.py 2009-06-25 04:06:00 +0000
> +++ lib/lp/code/scripts/revisionkarma.py 2010-01-05 02:10:14 +0000
> @@ -39,8 +39,9 @@
> count = 0
> revision_set = getUtility(IRevisionSet)
> # Break into bits.
> - revisions = list(
> - revision_set.getRevisionsNeedingKarmaAllocated()[:100])
> + result_set = revision_set.getRevisionsNeedingKarmaAllocated()
> + self.logger.info("%d revisions to update", result_set.count())
> + revisions = list(result_set[:100])
> while len(revisions) > 0:
> for revision in revisions:
> # Find t...

review: Approve

[Jonathan Lange (jml)]

> === modified file 'lib/lp/code/model/revision.py'
> --- lib/lp/code/model/revision.py 2009-12-08 04:03:30 +0000
> +++ lib/lp/code/model/revision.py 2010-01-05 02:10:14 +0000
...
> @@ -98,8 +96,12 @@
> # is lying), and a problem with the way the Launchpad code
> # currently does its karma degradation over time.
> if karma is not None:
> - karma.datecreated = min(self.revision_date, self.date_created)
> + removeSecurityProxy(karma).datecreated = min(
> + self.revision_date, self.date_created)
> self.karma_allocated = True
> + return karma
> + else:
> + return None
>

Like Michael, I generally like the changes. However, I'm pretty sure we should avoid using removeSecurityProxy in model code. If we have no other choice, then we should at least have a comment explaining why.

review: Needs Fixing

=== modified file 'lib/lp/code/interfaces/branchtarget.py'
--- lib/lp/code/interfaces/branchtarget.py	2009-08-04 00:41:49 +0000
+++ lib/lp/code/interfaces/branchtarget.py	2010-01-05 08:21:19 +0000
@@ -108,7 +108,7 @@
 
     collection = Attribute("An IBranchCollection for this target.")
 
-    def assignKarma(person, action_name):
+    def assignKarma(person, action_name, date_created=None):
         """Assign karma to the person on the appropriate target."""
 
     def getBugTask(bug):

=== modified file 'lib/lp/code/model/branchtarget.py'
--- lib/lp/code/model/branchtarget.py	2009-11-16 22:53:42 +0000
+++ lib/lp/code/model/branchtarget.py	2010-01-05 08:21:19 +0000
@@ -115,12 +115,13 @@
         else:
             return False
 
-    def assignKarma(self, person, action_name):
+    def assignKarma(self, person, action_name, date_created=None):
         """See `IBranchTarget`."""
-        person.assignKarma(
+        return person.assignKarma(
             action_name,
             distribution=self.context.distribution,
-            sourcepackagename=self.context.sourcepackagename)
+            sourcepackagename=self.context.sourcepackagename,
+            datecreated=date_created)
 
     def getBugTask(self, bug):
         """See `IBranchTarget`."""
@@ -185,9 +186,10 @@
         """See `IBranchTarget`."""
         return False
 
-    def assignKarma(self, person, action_name):
+    def assignKarma(self, person, action_name, date_created=None):
         """See `IBranchTarget`."""
         # Does nothing. No karma for +junk.
+        return None
 
     def getBugTask(self, bug):
         """See `IBranchTarget`."""
@@ -274,9 +276,10 @@
         else:
             return False
 
-    def assignKarma(self, person, action_name):
+    def assignKarma(self, person, action_name, date_created=None):
         """See `IBranchTarget`."""
-        person.assignKarma(action_name, product=self.product)
+        return person.assignKarma(
+            action_name, product=self.product, datecreated=date_created)
 
     def getBugTask(self, bug):
         """See `IBranchTarget`."""

=== modified file 'lib/lp/code/model/revision.py'
--- lib/lp/code/model/revision.py	2009-12-08 04:03:30 +0000
+++ lib/lp/code/model/revision.py	2010-01-05 08:21:19 +0000
@@ -86,10 +86,7 @@
         """See `IRevision`."""
         # If we know who the revision author is, give them karma.
         author = self.revision_author.person
-        if (author is not None and branch.product is not None):
-            # No karma for junk branches as we need a product to link
-            # against.
-            karma = author.assignKarma('revisionadded', branch.product)
+        if author is not None:
             # Backdate the karma to the time the revision was created.  If the
             # revision_date on the revision is in future (for whatever weird
             # reason) we will use the date_created from the revision (which
@@ -97,9 +94,14 @@
             # events is both wrong, as the revision has been created (and it
             # is lying), and a problem with the way the Launchpad code
             # currently does its karma degradation over time.
+            karma_date = min(self.revision_date, self.date_created)
+            karma = branch.target.assignKarma(
+                author, 'revisionadded', karma_date)
             if karma is not None:
-                karma.datecreated = min(self.revision_date, self.date_created)
                 self.karma_allocated = True
+            return karma
+        else:
+            return None
 
     def getBranch(self, allow_private=False, allow_junk=True):
         """See `IRevision`."""
@@ -385,9 +387,6 @@
         from lp.registry.model.person import ValidPersonCache
 
         store = getUtility(IStoreSelector).get(MAIN_STORE, DEFAULT_FLAVOR)
-
-        # XXX: Tim Penhey 2008-08-12, bug 244768
-        # Using Not(column == None) rather than column != None.
         return store.find(
             Revision,
             Revision.revision_author == RevisionAuthor.id,
@@ -397,7 +396,8 @@
                 Select(True,
                        And(BranchRevision.revision == Revision.id,
                            BranchRevision.branch == Branch.id,
-                           Not(Branch.product == None)),
+                           Or(Branch.product != None,
+                              Branch.distroseries != None)),
                        (Branch, BranchRevision))))
 
     @staticmethod

=== modified file 'lib/lp/code/model/tests/test_revision.py'
--- lib/lp/code/model/tests/test_revision.py	2009-11-17 17:28:10 +0000
+++ lib/lp/code/model/tests/test_revision.py	2010-01-05 08:21:19 +0000
@@ -140,8 +140,7 @@
     def test_junkBranchMovedToProductNeedsKarma(self):
         # A junk branch that moves to a product needs karma allocated.
         author = self.factory.makePerson()
-        rev = self.factory.makeRevision(
-            author=author.preferredemail.email)
+        rev = self.factory.makeRevision(author=author)
         branch = self.factory.makePersonalBranch()
         branch.createBranchRevision(1, rev)
         # Once the branch is connected to the revision, we now specify
@@ -152,6 +151,20 @@
         self.assertEqual(
             [rev], list(RevisionSet.getRevisionsNeedingKarmaAllocated()))
 
+    def test_junkBranchMovedToPackageNeedsKarma(self):
+        # A junk branch that moves to a package needs karma allocated.
+        author = self.factory.makePerson()
+        rev = self.factory.makeRevision(author=author)
+        branch = self.factory.makePersonalBranch()
+        branch.createBranchRevision(1, rev)
+        # Once the branch is connected to the revision, we now specify
+        # a product for the branch.
+        source_package = self.factory.makeSourcePackage()
+        branch.setTarget(user=branch.owner, source_package=source_package)
+        # The revision is now identified as needing karma allocated.
+        self.assertEqual(
+            [rev], list(RevisionSet.getRevisionsNeedingKarmaAllocated()))
+
     def test_newRevisionAuthorLinkNeedsKarma(self):
         # If Launchpad knows of revisions by a particular author, and later
         # that authoer registers with launchpad, the revisions need karma
@@ -178,17 +191,39 @@
         # is set to be the time that the revision was created.
         author = self.factory.makePerson()
         rev = self.factory.makeRevision(
-            author=author.preferredemail.email,
+            author=author,
             revision_date=datetime.now(pytz.UTC) + timedelta(days=5))
         branch = self.factory.makeProductBranch()
-        branch.createBranchRevision(1, rev)
-        # Get the karma event.
-        [karma] = list(Store.of(author).find(
-            Karma,
-            Karma.person == author,
-            Karma.product == branch.product))
+        karma = rev.allocateKarma(branch)
         self.assertEqual(karma.datecreated, rev.date_created)
 
+    def test_allocateKarma_personal_branch(self):
+        # A personal branch gets no karma event.
+        author = self.factory.makePerson()
+        rev = self.factory.makeRevision(author=author)
+        branch = self.factory.makePersonalBranch()
+        karma = rev.allocateKarma(branch)
+        self.assertIs(None, karma)
+
+    def test_allocateKarma_package_branch(self):
+        # A revision on a package branch gets karma.
+        author = self.factory.makePerson()
+        rev = self.factory.makeRevision(author=author)
+        branch = self.factory.makePackageBranch()
+        karma = rev.allocateKarma(branch)
+        self.assertEqual(author, karma.person)
+        self.assertEqual(branch.distribution, karma.distribution)
+        self.assertEqual(branch.sourcepackagename, karma.sourcepackagename)
+
+    def test_allocateKarma_product_branch(self):
+        # A revision on a product branch gets karma.
+        author = self.factory.makePerson()
+        rev = self.factory.makeRevision(author=author)
+        branch = self.factory.makeProductBranch()
+        karma = rev.allocateKarma(branch)
+        self.assertEqual(author, karma.person)
+        self.assertEqual(branch.product, karma.product)
+
 
 class TestRevisionGetBranch(TestCaseWithFactory):
     """Test the `getBranch` method of the revision."""

=== modified file 'lib/lp/code/scripts/revisionkarma.py'
--- lib/lp/code/scripts/revisionkarma.py	2009-06-25 04:06:00 +0000
+++ lib/lp/code/scripts/revisionkarma.py	2010-01-05 08:21:19 +0000
@@ -39,8 +39,9 @@
         count = 0
         revision_set = getUtility(IRevisionSet)
         # Break into bits.
-        revisions = list(
-            revision_set.getRevisionsNeedingKarmaAllocated()[:100])
+        result_set = revision_set.getRevisionsNeedingKarmaAllocated()
+        self.logger.info("%d revisions to update", result_set.count())
+        revisions = list(result_set[:100])
         while len(revisions) > 0:
             for revision in revisions:
                 # Find the appropriate branch, and allocate karma to it.

=== modified file 'lib/lp/registry/interfaces/person.py'
--- lib/lp/registry/interfaces/person.py	2009-12-09 23:11:31 +0000
+++ lib/lp/registry/interfaces/person.py	2010-01-05 08:21:19 +0000
@@ -966,13 +966,16 @@
         """
 
     def assignKarma(action_name, product=None, distribution=None,
-                    sourcepackagename=None):
+                    sourcepackagename=None, datecreated=None):
         """Assign karma for the action named <action_name> to this person.
 
         This karma will be associated with the given product or distribution.
         If a distribution is given, then product must be None and an optional
         sourcepackagename may also be given. If a product is given, then
         distribution and sourcepackagename must be None.
+
+        If a datecreated is specified, the karma will be created with that
+        date.  This is how historic karma events can be created.
         """
 
     def latestKarma(quantity=25):

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2009-12-05 18:37:28 +0000
+++ lib/lp/registry/model/person.py	2010-01-05 08:21:19 +0000
@@ -1050,7 +1050,7 @@
             return False
 
     def assignKarma(self, action_name, product=None, distribution=None,
-                    sourcepackagename=None):
+                    sourcepackagename=None, datecreated=None):
         """See `IPerson`."""
         # Teams don't get Karma. Inactive accounts don't get Karma.
         # The system user and janitor, does not get karma.
@@ -1074,9 +1074,12 @@
             raise AssertionError(
                 "No KarmaAction found with name '%s'." % action_name)
 
+        if datecreated is None:
+            datecreated = UTC_NOW
         karma = Karma(
             person=self, action=action, product=product,
-            distribution=distribution, sourcepackagename=sourcepackagename)
+            distribution=distribution, sourcepackagename=sourcepackagename,
+            datecreated=datecreated)
         notify(KarmaAssignedEvent(self, karma))
         return karma
 

=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2009-12-12 09:47:05 +0000
+++ lib/lp/testing/factory.py	2010-01-05 08:21:19 +0000
@@ -108,7 +108,7 @@
 from lp.registry.interfaces.mailinglistsubscription import (
     MailingListAutoSubscribePolicy)
 from lp.registry.interfaces.person import (
-    IPersonSet, PersonCreationRationale, TeamSubscriptionPolicy)
+    IPerson, IPersonSet, PersonCreationRationale, TeamSubscriptionPolicy)
 from lp.registry.interfaces.poll import IPollSet, PollAlgorithm, PollSecrecy
 from lp.registry.interfaces.product import IProductSet, License
 from lp.registry.interfaces.productseries import IProductSeries
@@ -904,6 +904,8 @@
         """Create a single `Revision`."""
         if author is None:
             author = self.getUniqueString('author')
+        elif IPerson.providedBy(author):
+            author = author.preferredemail.email
         if revision_date is None:
             revision_date = datetime.now(pytz.UTC)
         if parent_ids is None: