Merge lp:~jamalta/launchpad/bug-127489 into lp:launchpad
- bug-127489
- Merge into devel
Status: | Superseded |
---|---|
Proposed branch: | lp:~jamalta/launchpad/bug-127489 |
Merge into: | lp:launchpad |
Diff against target: | None lines |
To merge this branch: | bzr merge lp:~jamalta/launchpad/bug-127489 |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Jamal Fanaian (community) | Needs Fixing | ||
Review via email: mp+10753@code.launchpad.net |
This proposal supersedes a proposal from 2009-08-26.
This proposal has been superseded by a proposal from 2009-08-27.
Commit message
Description of the change
Stuart Bishop (stub) wrote : Posted in a previous version of this proposal | # |
Gavin Panella (allenap) wrote : Posted in a previous version of this proposal | # |
Hi Jamal,
Thank you for working on this bug!
As Stuart commented, this will need a pagetest. In fact, there is
already one that is probably broken by this change. Have a look at the
end of lib/lp/
Did you have a mentor for this branch, or a pre-implementation
discussion? Perhaps that might have spotted the need to update the
pagetest. Have a look at https:/
you haven't already. It's the same process as all the Launchpad devs
from Canonical use.
I have a comment about the code, because the cmp argument to sorted()
is deprecated (and I think it's gone entirely in Python 3).
Thanks again!
Gavin.
> === modified file 'lib/lp/
> --- lib/lp/
> +++ lib/lp/
> @@ -2517,7 +2517,10 @@
> categories = set()
> for contrib in self.contributions:
> categories.
> - return sorted(categories, key=attrgetter(
> + sort = {'code': 0, 'bugs': 1, 'blueprints': 2, 'translations': 3,
> + 'answers': 4, 'specs': 5, 'soyuz': 6}
> + return sorted(categories, key=attrgetter(
> + cmp=lambda x,y: cmp(sort[x], sort[y]))
Because cmp is deprecated, I think this would be better as:
return sorted(categories, key=lambda category: sort[category.
>
> @cachedproperty
> def context_
Jamal Fanaian (jamalta) wrote : Posted in a previous version of this proposal | # |
Hi Gavin,
Thanks for your input regarding this bug. I have actually been working on a test for this. I did not have a mentor when I started working on this issue, but went on #launchpad-dev to get some help. Salgado had recommended that I write a unittest to call the view, and make sure the data was returned in the right order, using a LaunchpadObject
> Hi Jamal,
>
> Thank you for working on this bug!
>
> As Stuart commented, this will need a pagetest. In fact, there is
> already one that is probably broken by this change. Have a look at the
> end of lib/lp/
>
> Did you have a mentor for this branch, or a pre-implementation
> discussion? Perhaps that might have spotted the need to update the
> pagetest. Have a look at https:/
> you haven't already. It's the same process as all the Launchpad devs
> from Canonical use.
>
> I have a comment about the code, because the cmp argument to sorted()
> is deprecated (and I think it's gone entirely in Python 3).
>
> Thanks again!
>
> Gavin.
>
>
> > === modified file 'lib/lp/
> > --- lib/lp/
> > +++ lib/lp/
> > @@ -2517,7 +2517,10 @@
> > categories = set()
> > for contrib in self.contributions:
> > categories.
> contrib[
> > - return sorted(categories, key=attrgetter(
> > + sort = {'code': 0, 'bugs': 1, 'blueprints': 2, 'translations': 3,
> > + 'answers': 4, 'specs': 5, 'soyuz': 6}
> > + return sorted(categories, key=attrgetter(
> > + cmp=lambda x,y: cmp(sort[x], sort[y]))
>
> Because cmp is deprecated, I think this would be better as:
>
> return sorted(categories, key=lambda category: sort[category.
>
> >
> > @cachedproperty
> > def context_
Jamal Fanaian (jamalta) wrote : Posted in a previous version of this proposal | # |
I went through the pagetest you referenced, and it will actually not fail. It is also not possible to test my changes with this because there is not enough data. The problem is that the only two icons displayed are bugs and translations. Sorted alphabetically, bugs will obviously always be first, which is the case with the replacement sort.
To test this I need to be able to add a third entry, say, Answers for example. I have this in my local environment (which I should really revert since it's going to break tests in the future), but I don't know how to properly write a pagetest for it because of this limitation. I think this is the reason salgado recommended writing a unittest.
Let me give the unittests another try to see if I can get them done. Thanks for your help so far!
> Hi Gavin,
>
> Thanks for your input regarding this bug. I have actually been working on a
> test for this. I did not have a mentor when I started working on this issue,
> but went on #launchpad-dev to get some help. Salgado had recommended that I
> write a unittest to call the view, and make sure the data was returned in the
> right order, using a LaunchpadObject
> figure out how to do that exactly, and haven't gotten back to it in a few days
> now. I will take a look at the test you mentioned and make sure I fix it.
> Also, I appreciate your comment regarding the depracated cmp() function. I am
> fairly new to Python and was not aware of this. I will fix that shortly as
> well, using your recommendation.
>
> > Hi Jamal,
> >
> > Thank you for working on this bug!
> >
> > As Stuart commented, this will need a pagetest. In fact, there is
> > already one that is probably broken by this change. Have a look at the
> > end of lib/lp/
> >
> > Did you have a mentor for this branch, or a pre-implementation
> > discussion? Perhaps that might have spotted the need to update the
> > pagetest. Have a look at https:/
> > you haven't already. It's the same process as all the Launchpad devs
> > from Canonical use.
> >
> > I have a comment about the code, because the cmp argument to sorted()
> > is deprecated (and I think it's gone entirely in Python 3).
> >
> > Thanks again!
> >
> > Gavin.
> >
> >
> > > === modified file 'lib/lp/
> > > --- lib/lp/
> > > +++ lib/lp/
> > > @@ -2517,7 +2517,10 @@
> > > categories = set()
> > > for contrib in self.contributions:
> > > categories.
> > contrib[
> > > - return sorted(categories, key=attrgetter(
> > > + sort = {'code': 0, 'bugs': 1, 'blueprints': 2, 'translations': 3,
> > > + 'answers': 4, 'specs': 5, 'soyuz': 6}
> > > + return sorted(categories, key=attrgetter(
> > > + cmp=lambda x,y: cmp(sort[x], sort[y]))
> >
> > Because cmp is deprecated, I think this would be better as:
> >
> > return sorted(categories, key=lambda category: sort[categ...
Gavin Panella (allenap) wrote : Posted in a previous version of this proposal | # |
Have a look at using lp.testing.
Guilherme Salgado (salgado) wrote : Posted in a previous version of this proposal | # |
On Fri, 2009-08-07 at 09:19 +0000, Gavin Panella wrote:
> Have a look at using lp.testing.
I don't think so. That list of categories is generated based on the kind
of karma the person has on the KarmaCache table, and that table is
populated by a script. Even though the person would get karma when
factory.
table until we ran the script. We could run the script in the test, but
that'd make the test *a lot* slower.
That's why I suggested a new method on LPObjectFactory to create the
KarmaCache entries directly.
Jamal, I remember you were able to write the new factory method, so you
should be quite close to getting a test for your fix. If you need any
help, just drop by #launchpad-
Cheers,
--
Guilherme Salgado <email address hidden>
Jamal Fanaian (jamalta) wrote : Posted in a previous version of this proposal | # |
I have added a test that makes sure the categories returned by PersonView.
I may not be doing this the best way, so I am requesting a review and hopefully I can chat with someone about it come Monday to figure out what needs to be improved or changed.
Thanks again for all your help and sorry this took so long!
> On Fri, 2009-08-07 at 09:19 +0000, Gavin Panella wrote:
> > Have a look at using lp.testing.
> this to quickly create, say, branches, so that there are code artifacts for
> your user. That will probably be enough to test this code.
>
> I don't think so. That list of categories is generated based on the kind
> of karma the person has on the KarmaCache table, and that table is
> populated by a script. Even though the person would get karma when
> factory.
> table until we ran the script. We could run the script in the test, but
> that'd make the test *a lot* slower.
>
> That's why I suggested a new method on LPObjectFactory to create the
> KarmaCache entries directly.
>
> Jamal, I remember you were able to write the new factory method, so you
> should be quite close to getting a test for your fix. If you need any
> help, just drop by #launchpad-
>
> Cheers,
>
> --
> Guilherme Salgado <email address hidden>
Jamal Fanaian (jamalta) wrote : Posted in a previous version of this proposal | # |
Sorry forgot to mention this on my last comment. I will try to ping someone in #launchpad-reviews come Monday to go over my branch, that way we don't have to go back and forth over email.
Thanks!
> I have added a test that makes sure the categories returned by
> PersonView.
> script to update Karma I am using the test factories and creating the
> KarmaCache records based on how the Karma update script does it.
>
> I may not be doing this the best way, so I am requesting a review and
> hopefully I can chat with someone about it come Monday to figure out what
> needs to be improved or changed.
>
> Thanks again for all your help and sorry this took so long!
>
> > On Fri, 2009-08-07 at 09:19 +0000, Gavin Panella wrote:
> > > Have a look at using lp.testing.
> use
> > this to quickly create, say, branches, so that there are code artifacts for
> > your user. That will probably be enough to test this code.
> >
> > I don't think so. That list of categories is generated based on the kind
> > of karma the person has on the KarmaCache table, and that table is
> > populated by a script. Even though the person would get karma when
> > factory.
> > table until we ran the script. We could run the script in the test, but
> > that'd make the test *a lot* slower.
> >
> > That's why I suggested a new method on LPObjectFactory to create the
> > KarmaCache entries directly.
> >
> > Jamal, I remember you were able to write the new factory method, so you
> > should be quite close to getting a test for your fix. If you need any
> > help, just drop by #launchpad-
> >
> > Cheers,
> >
> > --
> > Guilherme Salgado <email address hidden>
Jamal Fanaian (jamalta) wrote : | # |
After taking to salgado in #launchpad-reviews, he recommended that the makeKarmaCache() be modified so that it creates the total records (with category=NULL), instead of processing all entries in KarmaCache to create the sums.
- 9023. By Jamal Fanaian <jamal@jfvm1>
-
Simplified the karma test by only creating totals of the specified products
- 9024. By Jamal Fanaian <jamal@jfvm1>
-
Cleaned up the code to match the style guide. Modified makeKarmaCache to take category and product objects instead of their ids. Switching back to the launchpad db user after making changes to karma. Removed the raw sql query and replaced it with calls to the karma cache manager.a
- 9025. By Jamal Fanaian <jamal@jfvm1>
-
Person is no longer an instance variable. makeKarmaCache now requires a product be passed. Organized imports and a few other things according to the style guidelines and recommendations from the merge proposal.
- 9026. By Jamal Fanaian <jamal@jfvm1>
-
Removed the statement to switch DB user back to launchpad in makeKarmaCache. Renamed to _makeKarmaCache as it is more of an internal call. Added function docs to explain that the DB user will be switched.
- 9027. By Jamal Fanaian <jamal@jfvm1>
-
replaced the 'blueprints' category with it's correct name 'specs'
Unmerged revisions
- 9027. By Jamal Fanaian <jamal@jfvm1>
-
replaced the 'blueprints' category with it's correct name 'specs'
Preview Diff
1 | === modified file 'lib/lp/registry/browser/person.py' |
2 | --- lib/lp/registry/browser/person.py 2009-07-30 22:35:16 +0000 |
3 | +++ lib/lp/registry/browser/person.py 2009-08-05 19:55:13 +0000 |
4 | @@ -2517,7 +2517,9 @@ |
5 | categories = set() |
6 | for contrib in self.contributions: |
7 | categories.update(category for category in contrib['categories']) |
8 | - return sorted(categories, key=attrgetter('title')) |
9 | + sort = {'code': 0, 'bugs': 1, 'blueprints': 2, 'translations': 3, |
10 | + 'answers': 4, 'specs': 5, 'soyuz': 6} |
11 | + return sorted(categories, key=lambda category: sort[category.name]) |
12 | |
13 | @cachedproperty |
14 | def context_is_probably_a_team(self): |
15 | |
16 | === added file 'lib/lp/registry/browser/tests/test_person_view.py' |
17 | --- lib/lp/registry/browser/tests/test_person_view.py 1970-01-01 00:00:00 +0000 |
18 | +++ lib/lp/registry/browser/tests/test_person_view.py 2009-08-22 23:03:21 +0000 |
19 | @@ -0,0 +1,138 @@ |
20 | +# Copyright 2009 Canonical Ltd. This software is licensed under the |
21 | +# GNU Affero General Public License version 3 (see the file LICENSE). |
22 | + |
23 | +__metaclass__ = type |
24 | + |
25 | +import unittest |
26 | + |
27 | +from canonical.config import config |
28 | +from canonical.launchpad.webapp.servers import LaunchpadTestRequest |
29 | +from canonical.testing import LaunchpadZopelessLayer |
30 | +from canonical.database.sqlbase import cursor |
31 | +from lp.registry.browser.person import PersonView |
32 | +from lp.testing import TestCaseWithFactory |
33 | +from zope.component import getUtility |
34 | +from canonical.launchpad.interfaces import IKarmaCacheManager |
35 | + |
36 | +class TestPerson(TestCaseWithFactory): |
37 | + """Test Person view.""" |
38 | + |
39 | + layer = LaunchpadZopelessLayer |
40 | + |
41 | + def setUp(self): |
42 | + # Create a person |
43 | + TestCaseWithFactory.setUp(self) |
44 | + self.person = self.factory.makePerson() |
45 | + self.view = PersonView(self.person, |
46 | + LaunchpadTestRequest()) |
47 | + |
48 | + def test_karma_category_sort(self): |
49 | + # Add karma to some categories for the user |
50 | + cur = cursor() |
51 | + karmacache = self.makeKarmaCache(person=self.person, category_id=2) |
52 | + karmacache = self.makeKarmaCache(person=self.person, category_id=7) |
53 | + karmacache = self.makeKarmaCache(person=self.person, category_id=8) |
54 | + |
55 | + # Update Karma totals |
56 | + self.updateKarmaTotals() |
57 | + |
58 | + # Get contributed categories for the user |
59 | + categories = self.view.contributed_categories |
60 | + category_names = [] |
61 | + for category in categories: |
62 | + category_names.append(category.name) |
63 | + |
64 | + self.assertEqual(category_names, [u'code', u'bugs', u'answers'], |
65 | + 'Categories are not sorted correctly') |
66 | + |
67 | + def makeKarmaCache(self, person=None, value=10, category_id=2, |
68 | + product_id=5): |
69 | + |
70 | + LaunchpadZopelessLayer.switchDbUser(config.karmacacheupdater.dbuser) |
71 | + karmacache = getUtility(IKarmaCacheManager).new( |
72 | + person_id=person.id, value=value, category_id=category_id, |
73 | + product_id=product_id) |
74 | + LaunchpadZopelessLayer.commit() |
75 | + |
76 | + return karmacache |
77 | + |
78 | + def updateKarmaTotals(self): |
79 | + LaunchpadZopelessLayer.switchDbUser(config.karmacacheupdater.dbuser) |
80 | + |
81 | + cur = cursor() |
82 | + cur.execute("DELETE FROM KarmaCache WHERE category IS NULL") |
83 | + cur.execute(""" |
84 | + DELETE FROM KarmaCache |
85 | + WHERE project IS NOT NULL AND product IS NULL""") |
86 | + cur.execute(""" |
87 | + DELETE FROM KarmaCache |
88 | + WHERE category IS NOT NULL AND project IS NULL AND product IS NULL |
89 | + AND distribution IS NULL AND sourcepackagename IS NULL""") |
90 | + cur.execute("DELETE FROM KarmaCache WHERE karmavalue <= 0") |
91 | + |
92 | + # - All actions with a specific category of a person. |
93 | + cur.execute(""" |
94 | + INSERT INTO KarmaCache |
95 | + (person, category, karmavalue, product, distribution, |
96 | + sourcepackagename, project) |
97 | + SELECT person, category, SUM(karmavalue), NULL, NULL, NULL, NULL |
98 | + FROM KarmaCache |
99 | + WHERE category IS NOT NULL |
100 | + GROUP BY person, category |
101 | + """) |
102 | + |
103 | + # - All actions of a person on a given product. |
104 | + cur.execute(""" |
105 | + INSERT INTO KarmaCache |
106 | + (person, category, karmavalue, product, distribution, |
107 | + sourcepackagename, project) |
108 | + SELECT person, NULL, SUM(karmavalue), product, NULL, NULL, NULL |
109 | + FROM KarmaCache |
110 | + WHERE product IS NOT NULL |
111 | + GROUP BY person, product |
112 | + """) |
113 | + |
114 | + # - All actions of a person on a given distribution. |
115 | + cur.execute(""" |
116 | + INSERT INTO KarmaCache |
117 | + (person, category, karmavalue, product, distribution, |
118 | + sourcepackagename, project) |
119 | + SELECT person, NULL, SUM(karmavalue), NULL, distribution, NULL, NULL |
120 | + FROM KarmaCache |
121 | + WHERE distribution IS NOT NULL |
122 | + GROUP BY person, distribution |
123 | + """) |
124 | + |
125 | + # - All actions of a person on a given project. |
126 | + cur.execute(""" |
127 | + INSERT INTO KarmaCache |
128 | + (person, category, karmavalue, product, distribution, |
129 | + sourcepackagename, project) |
130 | + SELECT person, NULL, SUM(karmavalue), NULL, NULL, NULL, |
131 | + Product.project |
132 | + FROM KarmaCache |
133 | + JOIN Product ON product = Product.id |
134 | + WHERE Product.project IS NOT NULL AND product IS NOT NULL |
135 | + AND category IS NOT NULL |
136 | + GROUP BY person, Product.project |
137 | + """) |
138 | + |
139 | + # - All actions with a specific category of a person on a given project |
140 | + # IMPORTANT: This has to be the latest step; otherwise the rows |
141 | + # inserted here will be included in the calculation of the overall |
142 | + # karma of a person on a given project. |
143 | + cur.execute(""" |
144 | + INSERT INTO KarmaCache |
145 | + (person, category, karmavalue, product, distribution, |
146 | + sourcepackagename, project) |
147 | + SELECT person, category, SUM(karmavalue), NULL, NULL, NULL, |
148 | + Product.project |
149 | + FROM KarmaCache |
150 | + JOIN Product ON product = Product.id |
151 | + WHERE Product.project IS NOT NULL AND product IS NOT NULL |
152 | + AND category IS NOT NULL |
153 | + GROUP BY person, category, Product.project |
154 | + """) |
155 | + |
156 | +def test_suite(): |
157 | + return unittest.TestLoader().loadTestsFromName(__name__) |
This is going to need a page test to catch future regressions.