Launchpad itself

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

  1. revision-karma-fix
  2. Merge into devel
Proposed by Tim Penhey
Status: Merged
Approved by: Tim Penhey
Approved revision: not available
Merged at revision: 10105
Proposed branch: lp:~thumper/launchpad/revision-karma-fix
Merge into: lp:launchpad
Diff against target: 279 lines (+78/-31)
8 files modified
lib/lp/code/interfaces/branchtarget.py (+1/-1)
lib/lp/code/model/branchtarget.py (+9/-6)
lib/lp/code/model/revision.py (+9/-9)
lib/lp/code/model/tests/test_revision.py (+44/-9)
lib/lp/code/scripts/revisionkarma.py (+3/-2)
lib/lp/registry/interfaces/person.py (+4/-1)
lib/lp/registry/model/person.py (+5/-2)
lib/lp/testing/factory.py (+3/-1)
To merge this branch: bzr merge lp:~thumper/launchpad/revision-karma-fix
Reviewer Review Type Date Requested Status
Jonathan Lange (community) Needs Fixing
Michael Hudson-Doyle Approve
Review via email: mp+16825@code.launchpad.net

Commit message

Fix the allocation of karma for revisions

To post a comment you must log in.
Revision history for this message
Tim Penhey (thumper) wrote :

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

Revision history for this message
Michael Hudson-Doyle (mwhudson) wrote :
Download full text (3.6 KiB)

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...

Read more...

review: Approve
Revision history for this message
Jonathan Lange (jml) wrote :

> === 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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
108108
109 collection = Attribute("An IBranchCollection for this target.")109 collection = Attribute("An IBranchCollection for this target.")
110110
111 def assignKarma(person, action_name):111 def assignKarma(person, action_name, date_created=None):
112 """Assign karma to the person on the appropriate target."""112 """Assign karma to the person on the appropriate target."""
113113
114 def getBugTask(bug):114 def getBugTask(bug):
115115
=== 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 @@
115 else:115 else:
116 return False116 return False
117117
118 def assignKarma(self, person, action_name):118 def assignKarma(self, person, action_name, date_created=None):
119 """See `IBranchTarget`."""119 """See `IBranchTarget`."""
120 person.assignKarma(120 return person.assignKarma(
121 action_name,121 action_name,
122 distribution=self.context.distribution,122 distribution=self.context.distribution,
123 sourcepackagename=self.context.sourcepackagename)123 sourcepackagename=self.context.sourcepackagename,
124 datecreated=date_created)
124125
125 def getBugTask(self, bug):126 def getBugTask(self, bug):
126 """See `IBranchTarget`."""127 """See `IBranchTarget`."""
@@ -185,9 +186,10 @@
185 """See `IBranchTarget`."""186 """See `IBranchTarget`."""
186 return False187 return False
187188
188 def assignKarma(self, person, action_name):189 def assignKarma(self, person, action_name, date_created=None):
189 """See `IBranchTarget`."""190 """See `IBranchTarget`."""
190 # Does nothing. No karma for +junk.191 # Does nothing. No karma for +junk.
192 return None
191193
192 def getBugTask(self, bug):194 def getBugTask(self, bug):
193 """See `IBranchTarget`."""195 """See `IBranchTarget`."""
@@ -274,9 +276,10 @@
274 else:276 else:
275 return False277 return False
276278
277 def assignKarma(self, person, action_name):279 def assignKarma(self, person, action_name, date_created=None):
278 """See `IBranchTarget`."""280 """See `IBranchTarget`."""
279 person.assignKarma(action_name, product=self.product)281 return person.assignKarma(
282 action_name, product=self.product, datecreated=date_created)
280283
281 def getBugTask(self, bug):284 def getBugTask(self, bug):
282 """See `IBranchTarget`."""285 """See `IBranchTarget`."""
283286
=== 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 @@
86 """See `IRevision`."""86 """See `IRevision`."""
87 # If we know who the revision author is, give them karma.87 # If we know who the revision author is, give them karma.
88 author = self.revision_author.person88 author = self.revision_author.person
89 if (author is not None and branch.product is not None):89 if author is not None:
90 # No karma for junk branches as we need a product to link
91 # against.
92 karma = author.assignKarma('revisionadded', branch.product)
93 # Backdate the karma to the time the revision was created. If the90 # Backdate the karma to the time the revision was created. If the
94 # revision_date on the revision is in future (for whatever weird91 # revision_date on the revision is in future (for whatever weird
95 # reason) we will use the date_created from the revision (which92 # reason) we will use the date_created from the revision (which
@@ -97,9 +94,14 @@
97 # events is both wrong, as the revision has been created (and it94 # events is both wrong, as the revision has been created (and it
98 # is lying), and a problem with the way the Launchpad code95 # is lying), and a problem with the way the Launchpad code
99 # currently does its karma degradation over time.96 # currently does its karma degradation over time.
97 karma_date = min(self.revision_date, self.date_created)
98 karma = branch.target.assignKarma(
99 author, 'revisionadded', karma_date)
100 if karma is not None:100 if karma is not None:
101 karma.datecreated = min(self.revision_date, self.date_created)
102 self.karma_allocated = True101 self.karma_allocated = True
102 return karma
103 else:
104 return None
103105
104 def getBranch(self, allow_private=False, allow_junk=True):106 def getBranch(self, allow_private=False, allow_junk=True):
105 """See `IRevision`."""107 """See `IRevision`."""
@@ -385,9 +387,6 @@
385 from lp.registry.model.person import ValidPersonCache387 from lp.registry.model.person import ValidPersonCache
386388
387 store = getUtility(IStoreSelector).get(MAIN_STORE, DEFAULT_FLAVOR)389 store = getUtility(IStoreSelector).get(MAIN_STORE, DEFAULT_FLAVOR)
388
389 # XXX: Tim Penhey 2008-08-12, bug 244768
390 # Using Not(column == None) rather than column != None.
391 return store.find(390 return store.find(
392 Revision,391 Revision,
393 Revision.revision_author == RevisionAuthor.id,392 Revision.revision_author == RevisionAuthor.id,
@@ -397,7 +396,8 @@
397 Select(True,396 Select(True,
398 And(BranchRevision.revision == Revision.id,397 And(BranchRevision.revision == Revision.id,
399 BranchRevision.branch == Branch.id,398 BranchRevision.branch == Branch.id,
400 Not(Branch.product == None)),399 Or(Branch.product != None,
400 Branch.distroseries != None)),
401 (Branch, BranchRevision))))401 (Branch, BranchRevision))))
402402
403 @staticmethod403 @staticmethod
404404
=== 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 @@
140 def test_junkBranchMovedToProductNeedsKarma(self):140 def test_junkBranchMovedToProductNeedsKarma(self):
141 # A junk branch that moves to a product needs karma allocated.141 # A junk branch that moves to a product needs karma allocated.
142 author = self.factory.makePerson()142 author = self.factory.makePerson()
143 rev = self.factory.makeRevision(143 rev = self.factory.makeRevision(author=author)
144 author=author.preferredemail.email)
145 branch = self.factory.makePersonalBranch()144 branch = self.factory.makePersonalBranch()
146 branch.createBranchRevision(1, rev)145 branch.createBranchRevision(1, rev)
147 # Once the branch is connected to the revision, we now specify146 # Once the branch is connected to the revision, we now specify
@@ -152,6 +151,20 @@
152 self.assertEqual(151 self.assertEqual(
153 [rev], list(RevisionSet.getRevisionsNeedingKarmaAllocated()))152 [rev], list(RevisionSet.getRevisionsNeedingKarmaAllocated()))
154153
154 def test_junkBranchMovedToPackageNeedsKarma(self):
155 # A junk branch that moves to a package needs karma allocated.
156 author = self.factory.makePerson()
157 rev = self.factory.makeRevision(author=author)
158 branch = self.factory.makePersonalBranch()
159 branch.createBranchRevision(1, rev)
160 # Once the branch is connected to the revision, we now specify
161 # a product for the branch.
162 source_package = self.factory.makeSourcePackage()
163 branch.setTarget(user=branch.owner, source_package=source_package)
164 # The revision is now identified as needing karma allocated.
165 self.assertEqual(
166 [rev], list(RevisionSet.getRevisionsNeedingKarmaAllocated()))
167
155 def test_newRevisionAuthorLinkNeedsKarma(self):168 def test_newRevisionAuthorLinkNeedsKarma(self):
156 # If Launchpad knows of revisions by a particular author, and later169 # If Launchpad knows of revisions by a particular author, and later
157 # that authoer registers with launchpad, the revisions need karma170 # that authoer registers with launchpad, the revisions need karma
@@ -178,17 +191,39 @@
178 # is set to be the time that the revision was created.191 # is set to be the time that the revision was created.
179 author = self.factory.makePerson()192 author = self.factory.makePerson()
180 rev = self.factory.makeRevision(193 rev = self.factory.makeRevision(
181 author=author.preferredemail.email,194 author=author,
182 revision_date=datetime.now(pytz.UTC) + timedelta(days=5))195 revision_date=datetime.now(pytz.UTC) + timedelta(days=5))
183 branch = self.factory.makeProductBranch()196 branch = self.factory.makeProductBranch()
184 branch.createBranchRevision(1, rev)197 karma = rev.allocateKarma(branch)
185 # Get the karma event.
186 [karma] = list(Store.of(author).find(
187 Karma,
188 Karma.person == author,
189 Karma.product == branch.product))
190 self.assertEqual(karma.datecreated, rev.date_created)198 self.assertEqual(karma.datecreated, rev.date_created)
191199
200 def test_allocateKarma_personal_branch(self):
201 # A personal branch gets no karma event.
202 author = self.factory.makePerson()
203 rev = self.factory.makeRevision(author=author)
204 branch = self.factory.makePersonalBranch()
205 karma = rev.allocateKarma(branch)
206 self.assertIs(None, karma)
207
208 def test_allocateKarma_package_branch(self):
209 # A revision on a package branch gets karma.
210 author = self.factory.makePerson()
211 rev = self.factory.makeRevision(author=author)
212 branch = self.factory.makePackageBranch()
213 karma = rev.allocateKarma(branch)
214 self.assertEqual(author, karma.person)
215 self.assertEqual(branch.distribution, karma.distribution)
216 self.assertEqual(branch.sourcepackagename, karma.sourcepackagename)
217
218 def test_allocateKarma_product_branch(self):
219 # A revision on a product branch gets karma.
220 author = self.factory.makePerson()
221 rev = self.factory.makeRevision(author=author)
222 branch = self.factory.makeProductBranch()
223 karma = rev.allocateKarma(branch)
224 self.assertEqual(author, karma.person)
225 self.assertEqual(branch.product, karma.product)
226
192227
193class TestRevisionGetBranch(TestCaseWithFactory):228class TestRevisionGetBranch(TestCaseWithFactory):
194 """Test the `getBranch` method of the revision."""229 """Test the `getBranch` method of the revision."""
195230
=== 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 @@
39 count = 039 count = 0
40 revision_set = getUtility(IRevisionSet)40 revision_set = getUtility(IRevisionSet)
41 # Break into bits.41 # Break into bits.
42 revisions = list(42 result_set = revision_set.getRevisionsNeedingKarmaAllocated()
43 revision_set.getRevisionsNeedingKarmaAllocated()[:100])43 self.logger.info("%d revisions to update", result_set.count())
44 revisions = list(result_set[:100])
44 while len(revisions) > 0:45 while len(revisions) > 0:
45 for revision in revisions:46 for revision in revisions:
46 # Find the appropriate branch, and allocate karma to it.47 # Find the appropriate branch, and allocate karma to it.
4748
=== 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 @@
966 """966 """
967967
968 def assignKarma(action_name, product=None, distribution=None,968 def assignKarma(action_name, product=None, distribution=None,
969 sourcepackagename=None):969 sourcepackagename=None, datecreated=None):
970 """Assign karma for the action named <action_name> to this person.970 """Assign karma for the action named <action_name> to this person.
971971
972 This karma will be associated with the given product or distribution.972 This karma will be associated with the given product or distribution.
973 If a distribution is given, then product must be None and an optional973 If a distribution is given, then product must be None and an optional
974 sourcepackagename may also be given. If a product is given, then974 sourcepackagename may also be given. If a product is given, then
975 distribution and sourcepackagename must be None.975 distribution and sourcepackagename must be None.
976
977 If a datecreated is specified, the karma will be created with that
978 date. This is how historic karma events can be created.
976 """979 """
977980
978 def latestKarma(quantity=25):981 def latestKarma(quantity=25):
979982
=== 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 @@
1050 return False1050 return False
10511051
1052 def assignKarma(self, action_name, product=None, distribution=None,1052 def assignKarma(self, action_name, product=None, distribution=None,
1053 sourcepackagename=None):1053 sourcepackagename=None, datecreated=None):
1054 """See `IPerson`."""1054 """See `IPerson`."""
1055 # Teams don't get Karma. Inactive accounts don't get Karma.1055 # Teams don't get Karma. Inactive accounts don't get Karma.
1056 # The system user and janitor, does not get karma.1056 # The system user and janitor, does not get karma.
@@ -1074,9 +1074,12 @@
1074 raise AssertionError(1074 raise AssertionError(
1075 "No KarmaAction found with name '%s'." % action_name)1075 "No KarmaAction found with name '%s'." % action_name)
10761076
1077 if datecreated is None:
1078 datecreated = UTC_NOW
1077 karma = Karma(1079 karma = Karma(
1078 person=self, action=action, product=product,1080 person=self, action=action, product=product,
1079 distribution=distribution, sourcepackagename=sourcepackagename)1081 distribution=distribution, sourcepackagename=sourcepackagename,
1082 datecreated=datecreated)
1080 notify(KarmaAssignedEvent(self, karma))1083 notify(KarmaAssignedEvent(self, karma))
1081 return karma1084 return karma
10821085
10831086
=== 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 @@
108from lp.registry.interfaces.mailinglistsubscription import (108from lp.registry.interfaces.mailinglistsubscription import (
109 MailingListAutoSubscribePolicy)109 MailingListAutoSubscribePolicy)
110from lp.registry.interfaces.person import (110from lp.registry.interfaces.person import (
111 IPersonSet, PersonCreationRationale, TeamSubscriptionPolicy)111 IPerson, IPersonSet, PersonCreationRationale, TeamSubscriptionPolicy)
112from lp.registry.interfaces.poll import IPollSet, PollAlgorithm, PollSecrecy112from lp.registry.interfaces.poll import IPollSet, PollAlgorithm, PollSecrecy
113from lp.registry.interfaces.product import IProductSet, License113from lp.registry.interfaces.product import IProductSet, License
114from lp.registry.interfaces.productseries import IProductSeries114from lp.registry.interfaces.productseries import IProductSeries
@@ -904,6 +904,8 @@
904 """Create a single `Revision`."""904 """Create a single `Revision`."""
905 if author is None:905 if author is None:
906 author = self.getUniqueString('author')906 author = self.getUniqueString('author')
907 elif IPerson.providedBy(author):
908 author = author.preferredemail.email
907 if revision_date is None:909 if revision_date is None:
908 revision_date = datetime.now(pytz.UTC)910 revision_date = datetime.now(pytz.UTC)
909 if parent_ids is None:911 if parent_ids is None: