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
1=== modified file 'lib/lp/code/interfaces/branchtarget.py'
2--- lib/lp/code/interfaces/branchtarget.py 2009-08-04 00:41:49 +0000
3+++ lib/lp/code/interfaces/branchtarget.py 2010-01-05 08:21:19 +0000
4@@ -108,7 +108,7 @@
5
6 collection = Attribute("An IBranchCollection for this target.")
7
8- def assignKarma(person, action_name):
9+ def assignKarma(person, action_name, date_created=None):
10 """Assign karma to the person on the appropriate target."""
11
12 def getBugTask(bug):
13
14=== modified file 'lib/lp/code/model/branchtarget.py'
15--- lib/lp/code/model/branchtarget.py 2009-11-16 22:53:42 +0000
16+++ lib/lp/code/model/branchtarget.py 2010-01-05 08:21:19 +0000
17@@ -115,12 +115,13 @@
18 else:
19 return False
20
21- def assignKarma(self, person, action_name):
22+ def assignKarma(self, person, action_name, date_created=None):
23 """See `IBranchTarget`."""
24- person.assignKarma(
25+ return person.assignKarma(
26 action_name,
27 distribution=self.context.distribution,
28- sourcepackagename=self.context.sourcepackagename)
29+ sourcepackagename=self.context.sourcepackagename,
30+ datecreated=date_created)
31
32 def getBugTask(self, bug):
33 """See `IBranchTarget`."""
34@@ -185,9 +186,10 @@
35 """See `IBranchTarget`."""
36 return False
37
38- def assignKarma(self, person, action_name):
39+ def assignKarma(self, person, action_name, date_created=None):
40 """See `IBranchTarget`."""
41 # Does nothing. No karma for +junk.
42+ return None
43
44 def getBugTask(self, bug):
45 """See `IBranchTarget`."""
46@@ -274,9 +276,10 @@
47 else:
48 return False
49
50- def assignKarma(self, person, action_name):
51+ def assignKarma(self, person, action_name, date_created=None):
52 """See `IBranchTarget`."""
53- person.assignKarma(action_name, product=self.product)
54+ return person.assignKarma(
55+ action_name, product=self.product, datecreated=date_created)
56
57 def getBugTask(self, bug):
58 """See `IBranchTarget`."""
59
60=== modified file 'lib/lp/code/model/revision.py'
61--- lib/lp/code/model/revision.py 2009-12-08 04:03:30 +0000
62+++ lib/lp/code/model/revision.py 2010-01-05 08:21:19 +0000
63@@ -86,10 +86,7 @@
64 """See `IRevision`."""
65 # If we know who the revision author is, give them karma.
66 author = self.revision_author.person
67- if (author is not None and branch.product is not None):
68- # No karma for junk branches as we need a product to link
69- # against.
70- karma = author.assignKarma('revisionadded', branch.product)
71+ if author is not None:
72 # Backdate the karma to the time the revision was created. If the
73 # revision_date on the revision is in future (for whatever weird
74 # reason) we will use the date_created from the revision (which
75@@ -97,9 +94,14 @@
76 # events is both wrong, as the revision has been created (and it
77 # is lying), and a problem with the way the Launchpad code
78 # currently does its karma degradation over time.
79+ karma_date = min(self.revision_date, self.date_created)
80+ karma = branch.target.assignKarma(
81+ author, 'revisionadded', karma_date)
82 if karma is not None:
83- karma.datecreated = min(self.revision_date, self.date_created)
84 self.karma_allocated = True
85+ return karma
86+ else:
87+ return None
88
89 def getBranch(self, allow_private=False, allow_junk=True):
90 """See `IRevision`."""
91@@ -385,9 +387,6 @@
92 from lp.registry.model.person import ValidPersonCache
93
94 store = getUtility(IStoreSelector).get(MAIN_STORE, DEFAULT_FLAVOR)
95-
96- # XXX: Tim Penhey 2008-08-12, bug 244768
97- # Using Not(column == None) rather than column != None.
98 return store.find(
99 Revision,
100 Revision.revision_author == RevisionAuthor.id,
101@@ -397,7 +396,8 @@
102 Select(True,
103 And(BranchRevision.revision == Revision.id,
104 BranchRevision.branch == Branch.id,
105- Not(Branch.product == None)),
106+ Or(Branch.product != None,
107+ Branch.distroseries != None)),
108 (Branch, BranchRevision))))
109
110 @staticmethod
111
112=== modified file 'lib/lp/code/model/tests/test_revision.py'
113--- lib/lp/code/model/tests/test_revision.py 2009-11-17 17:28:10 +0000
114+++ lib/lp/code/model/tests/test_revision.py 2010-01-05 08:21:19 +0000
115@@ -140,8 +140,7 @@
116 def test_junkBranchMovedToProductNeedsKarma(self):
117 # A junk branch that moves to a product needs karma allocated.
118 author = self.factory.makePerson()
119- rev = self.factory.makeRevision(
120- author=author.preferredemail.email)
121+ rev = self.factory.makeRevision(author=author)
122 branch = self.factory.makePersonalBranch()
123 branch.createBranchRevision(1, rev)
124 # Once the branch is connected to the revision, we now specify
125@@ -152,6 +151,20 @@
126 self.assertEqual(
127 [rev], list(RevisionSet.getRevisionsNeedingKarmaAllocated()))
128
129+ def test_junkBranchMovedToPackageNeedsKarma(self):
130+ # A junk branch that moves to a package needs karma allocated.
131+ author = self.factory.makePerson()
132+ rev = self.factory.makeRevision(author=author)
133+ branch = self.factory.makePersonalBranch()
134+ branch.createBranchRevision(1, rev)
135+ # Once the branch is connected to the revision, we now specify
136+ # a product for the branch.
137+ source_package = self.factory.makeSourcePackage()
138+ branch.setTarget(user=branch.owner, source_package=source_package)
139+ # The revision is now identified as needing karma allocated.
140+ self.assertEqual(
141+ [rev], list(RevisionSet.getRevisionsNeedingKarmaAllocated()))
142+
143 def test_newRevisionAuthorLinkNeedsKarma(self):
144 # If Launchpad knows of revisions by a particular author, and later
145 # that authoer registers with launchpad, the revisions need karma
146@@ -178,17 +191,39 @@
147 # is set to be the time that the revision was created.
148 author = self.factory.makePerson()
149 rev = self.factory.makeRevision(
150- author=author.preferredemail.email,
151+ author=author,
152 revision_date=datetime.now(pytz.UTC) + timedelta(days=5))
153 branch = self.factory.makeProductBranch()
154- branch.createBranchRevision(1, rev)
155- # Get the karma event.
156- [karma] = list(Store.of(author).find(
157- Karma,
158- Karma.person == author,
159- Karma.product == branch.product))
160+ karma = rev.allocateKarma(branch)
161 self.assertEqual(karma.datecreated, rev.date_created)
162
163+ def test_allocateKarma_personal_branch(self):
164+ # A personal branch gets no karma event.
165+ author = self.factory.makePerson()
166+ rev = self.factory.makeRevision(author=author)
167+ branch = self.factory.makePersonalBranch()
168+ karma = rev.allocateKarma(branch)
169+ self.assertIs(None, karma)
170+
171+ def test_allocateKarma_package_branch(self):
172+ # A revision on a package branch gets karma.
173+ author = self.factory.makePerson()
174+ rev = self.factory.makeRevision(author=author)
175+ branch = self.factory.makePackageBranch()
176+ karma = rev.allocateKarma(branch)
177+ self.assertEqual(author, karma.person)
178+ self.assertEqual(branch.distribution, karma.distribution)
179+ self.assertEqual(branch.sourcepackagename, karma.sourcepackagename)
180+
181+ def test_allocateKarma_product_branch(self):
182+ # A revision on a product branch gets karma.
183+ author = self.factory.makePerson()
184+ rev = self.factory.makeRevision(author=author)
185+ branch = self.factory.makeProductBranch()
186+ karma = rev.allocateKarma(branch)
187+ self.assertEqual(author, karma.person)
188+ self.assertEqual(branch.product, karma.product)
189+
190
191 class TestRevisionGetBranch(TestCaseWithFactory):
192 """Test the `getBranch` method of the revision."""
193
194=== modified file 'lib/lp/code/scripts/revisionkarma.py'
195--- lib/lp/code/scripts/revisionkarma.py 2009-06-25 04:06:00 +0000
196+++ lib/lp/code/scripts/revisionkarma.py 2010-01-05 08:21:19 +0000
197@@ -39,8 +39,9 @@
198 count = 0
199 revision_set = getUtility(IRevisionSet)
200 # Break into bits.
201- revisions = list(
202- revision_set.getRevisionsNeedingKarmaAllocated()[:100])
203+ result_set = revision_set.getRevisionsNeedingKarmaAllocated()
204+ self.logger.info("%d revisions to update", result_set.count())
205+ revisions = list(result_set[:100])
206 while len(revisions) > 0:
207 for revision in revisions:
208 # Find the appropriate branch, and allocate karma to it.
209
210=== modified file 'lib/lp/registry/interfaces/person.py'
211--- lib/lp/registry/interfaces/person.py 2009-12-09 23:11:31 +0000
212+++ lib/lp/registry/interfaces/person.py 2010-01-05 08:21:19 +0000
213@@ -966,13 +966,16 @@
214 """
215
216 def assignKarma(action_name, product=None, distribution=None,
217- sourcepackagename=None):
218+ sourcepackagename=None, datecreated=None):
219 """Assign karma for the action named <action_name> to this person.
220
221 This karma will be associated with the given product or distribution.
222 If a distribution is given, then product must be None and an optional
223 sourcepackagename may also be given. If a product is given, then
224 distribution and sourcepackagename must be None.
225+
226+ If a datecreated is specified, the karma will be created with that
227+ date. This is how historic karma events can be created.
228 """
229
230 def latestKarma(quantity=25):
231
232=== modified file 'lib/lp/registry/model/person.py'
233--- lib/lp/registry/model/person.py 2009-12-05 18:37:28 +0000
234+++ lib/lp/registry/model/person.py 2010-01-05 08:21:19 +0000
235@@ -1050,7 +1050,7 @@
236 return False
237
238 def assignKarma(self, action_name, product=None, distribution=None,
239- sourcepackagename=None):
240+ sourcepackagename=None, datecreated=None):
241 """See `IPerson`."""
242 # Teams don't get Karma. Inactive accounts don't get Karma.
243 # The system user and janitor, does not get karma.
244@@ -1074,9 +1074,12 @@
245 raise AssertionError(
246 "No KarmaAction found with name '%s'." % action_name)
247
248+ if datecreated is None:
249+ datecreated = UTC_NOW
250 karma = Karma(
251 person=self, action=action, product=product,
252- distribution=distribution, sourcepackagename=sourcepackagename)
253+ distribution=distribution, sourcepackagename=sourcepackagename,
254+ datecreated=datecreated)
255 notify(KarmaAssignedEvent(self, karma))
256 return karma
257
258
259=== modified file 'lib/lp/testing/factory.py'
260--- lib/lp/testing/factory.py 2009-12-12 09:47:05 +0000
261+++ lib/lp/testing/factory.py 2010-01-05 08:21:19 +0000
262@@ -108,7 +108,7 @@
263 from lp.registry.interfaces.mailinglistsubscription import (
264 MailingListAutoSubscribePolicy)
265 from lp.registry.interfaces.person import (
266- IPersonSet, PersonCreationRationale, TeamSubscriptionPolicy)
267+ IPerson, IPersonSet, PersonCreationRationale, TeamSubscriptionPolicy)
268 from lp.registry.interfaces.poll import IPollSet, PollAlgorithm, PollSecrecy
269 from lp.registry.interfaces.product import IProductSet, License
270 from lp.registry.interfaces.productseries import IProductSeries
271@@ -904,6 +904,8 @@
272 """Create a single `Revision`."""
273 if author is None:
274 author = self.getUniqueString('author')
275+ elif IPerson.providedBy(author):
276+ author = author.preferredemail.email
277 if revision_date is None:
278 revision_date = datetime.now(pytz.UTC)
279 if parent_ids is None: