launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #04673
[Merge] lp:~sinzui/launchpad/code-review-affiliation into lp:launchpad
Curtis Hovey has proposed merging lp:~sinzui/launchpad/code-review-affiliation into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #828826 in Launchpad itself: "OOPS using person picker when reassigning merge proposal review slot"
https://bugs.launchpad.net/launchpad/+bug/828826
For more details, see:
https://code.launchpad.net/~sinzui/launchpad/code-review-affiliation/+merge/72106
IHasAffiliation is not required to use the person picker.
Launchpad bug: https://bugs.launchpad.net/bugs/828826
Pre-implementation: jcsackett
OOPS-2056DQ44 was caused when CodeReviewVoteReference could not be
adapted to IHasAffiliation to lookup badges. badge are not required;
PersonPickerEntrySourceAdapter must not assume that every possibly
target has affiliation.
--------------------------------------------------------------------
RULES
* Update PersonPickerEntrySourceAdapter to only lookup badges if
the context can adapted to IHasAffiliation.
* Add an IHasAffiliation adapter for ICodeReviewVoteReference
because the user probably wants to choose a person affiliated with
the pillar that owns the target branch.
* ADDENDUM Update BranchPillarAffiliation to show that trusted reviewers
are also affiliated.
QA
* locate a merge proposal that has not been reviewed yet.
* Choose to reassign (+reassign) the review.
* Use the Choose link and search for julian
* Verify the picker displays choices.
* Verify that Julian is affiliated and a trusted reviewer.
LINT
lib/lp/app/browser/vocabulary.py
lib/lp/app/browser/tests/test_vocabulary.py
lib/lp/registry/configure.zcml
lib/lp/registry/model/pillaraffiliation.py
lib/lp/registry/tests/test_pillaraffiliation.py
TEST
./bin/test -vv \
-t PersonPickerEntrySourceAdapterTestCase \
-t TestBranchPillarAffiliation \
-t CodeReviewVotePillarAffiliationTestCase
IMPLEMENTATION
Updated PersonPickerEntrySourceAdapter to only attempt to get the badges
for the context if it can be adapted it IHasAffiliation.
lib/lp/app/browser/vocabulary.py
lib/lp/app/browser/tests/test_vocabulary.py
Update BranchPillarAffiliation to include the branch trusted reviewers as
affiliated users.
lib/lp/registry/model/pillaraffiliation.py
lib/lp/registry/tests/test_pillaraffiliation.py
Added CodeReviewVotePillarAffiliation to show code reviewers that are
affiliated with the pillar or branch.
lib/lp/registry/configure.zcml
lib/lp/registry/model/pillaraffiliation.py
lib/lp/registry/tests/test_pillaraffiliation.py
--
https://code.launchpad.net/~sinzui/launchpad/code-review-affiliation/+merge/72106
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/code-review-affiliation into lp:launchpad.
=== modified file 'lib/lp/app/browser/tests/test_vocabulary.py'
--- lib/lp/app/browser/tests/test_vocabulary.py 2011-08-11 00:34:33 +0000
+++ lib/lp/app/browser/tests/test_vocabulary.py 2011-08-18 21:05:53 +0000
@@ -137,6 +137,17 @@
self.assertEqual('/@@/product-badge', entry.badges[2]['url'])
self.assertEqual('Fnord bug supervisor', entry.badges[2]['alt'])
+ def test_PersonPickerEntryAdapter_badges_without_IHasAffiliation(self):
+ # The enhanced person picker handles objects that do not support
+ # IHasAffilliation.
+ person = self.factory.makePerson(email='snarf@xxxxxx', name='snarf')
+ thing = object()
+ [entry] = IPickerEntrySource(person).getPickerEntries(
+ [person], thing, enhanced_picker_enabled=True,
+ picker_expander_enabled=True,
+ personpicker_affiliation_enabled=True)
+ self.assertEqual(None, None)
+
class TestPersonVocabulary:
implements(IHugeVocabulary)
=== modified file 'lib/lp/app/browser/vocabulary.py'
--- lib/lp/app/browser/vocabulary.py 2011-08-11 00:34:33 +0000
+++ lib/lp/app/browser/vocabulary.py 2011-08-18 21:05:53 +0000
@@ -146,11 +146,12 @@
personpicker_affiliation_enabled = kwarg.get(
'personpicker_affiliation_enabled', False)
- if personpicker_affiliation_enabled:
+ affiliated_context = IHasAffiliation(context_object, None)
+ if (affiliated_context is not None
+ and personpicker_affiliation_enabled):
# If a person is affiliated with the associated_object then we
# can display a badge.
- badges = IHasAffiliation(
- context_object).getAffiliationBadges(term_values)
+ badges = affiliated_context.getAffiliationBadges(term_values)
for picker_entry, badges in izip(picker_entries, badges):
picker_entry.badges = []
for badge_info in badges:
=== modified file 'lib/lp/registry/configure.zcml'
--- lib/lp/registry/configure.zcml 2011-08-09 04:09:33 +0000
+++ lib/lp/registry/configure.zcml 2011-08-18 21:05:53 +0000
@@ -915,7 +915,10 @@
for="lp.registry.interfaces.productseries.IProductSeries"
factory="lp.registry.model.pillaraffiliation.ProductSeriesPillarAffiliation"
/>
-
+ <adapter
+ for="lp.code.interfaces.codereviewvote.ICodeReviewVoteReference"
+ factory="lp.registry.model.pillaraffiliation.CodeReviewVotePillarAffiliation"
+ />
<!-- Using
permission="Public" here causes some tests to fail
=== modified file 'lib/lp/registry/model/pillaraffiliation.py'
--- lib/lp/registry/model/pillaraffiliation.py 2011-08-11 00:34:33 +0000
+++ lib/lp/registry/model/pillaraffiliation.py 2011-08-18 21:05:53 +0000
@@ -139,9 +139,32 @@
class BranchPillarAffiliation(BugTaskPillarAffiliation):
"""An affiliation adapter for branches."""
+
def getPillar(self):
return self.context.product or self.context.distribution
+ def getBranch(self):
+ return self.context
+
+ def _getAffiliationDetails(self, person, pillar):
+ super_instance = super(BranchPillarAffiliation, self)
+ result = super_instance._getAffiliationDetails(person, pillar)
+ if self.getBranch().isPersonTrustedReviewer(person):
+ result.append((pillar.displayname, 'trusted reviewer'))
+ return result
+
+
+class CodeReviewVotePillarAffiliation(BranchPillarAffiliation):
+ """An affiliation adapter for CodeReviewVotes."""
+
+ def getPillar(self):
+ """Return the target branch'pillar."""
+ branch = self.getBranch()
+ return branch.product or branch.distribution
+
+ def getBranch(self):
+ return self.context.branch_merge_proposal.target_branch
+
class DistroSeriesPillarAffiliation(PillarAffiliation):
"""An affiliation adapter for distroseries."""
=== modified file 'lib/lp/registry/tests/test_pillaraffiliation.py'
--- lib/lp/registry/tests/test_pillaraffiliation.py 2011-08-11 00:34:33 +0000
+++ lib/lp/registry/tests/test_pillaraffiliation.py 2011-08-18 21:05:53 +0000
@@ -246,17 +246,77 @@
adapter = IHasAffiliation(branch)
self.assertEqual(branch.product, adapter.getPillar())
- def _check_affiliated_with_distro(self, person, target, role):
- distroseries = self.factory.makeDistroSeries(distribution=target)
- sp = self.factory.makeSourcePackage(distroseries=distroseries)
- branch = self.factory.makeBranch(sourcepackage=sp)
- [badges] = IHasAffiliation(branch).getAffiliationBadges([person])
- self.assertEqual(
- ("/@@/distribution-badge", "Pting %s" % role), badges[0])
-
- def _check_affiliated_with_product(self, person, target, role):
- branch = self.factory.makeBranch(product=target)
- [badges] = IHasAffiliation(branch).getAffiliationBadges([person])
+ def test_getBranch(self):
+ # The branch is the context.
+ branch = self.factory.makeBranch()
+ adapter = IHasAffiliation(branch)
+ self.assertEqual(branch, adapter.getBranch())
+
+ def test_branch_trusted_reviewer_affiliation(self):
+ # A person who is the branch's trusted reviewer is affiliated.
+ person = self.factory.makePerson()
+ product = self.factory.makeProduct(name='pting')
+ self._check_affiliated_with_product(
+ person, product, 'trusted reviewer')
+
+ def _check_affiliated_with_distro(self, person, target, role):
+ distroseries = self.factory.makeDistroSeries(distribution=target)
+ sp = self.factory.makeSourcePackage(distroseries=distroseries)
+ branch = self.factory.makeBranch(sourcepackage=sp)
+ [badges] = IHasAffiliation(branch).getAffiliationBadges([person])
+ self.assertEqual(
+ ("/@@/distribution-badge", "Pting %s" % role), badges[0])
+
+ def _check_affiliated_with_product(self, person, target, role):
+ branch = self.factory.makeBranch(product=target)
+ with person_logged_in(branch.owner):
+ branch.reviewer = person
+ [badges] = IHasAffiliation(branch).getAffiliationBadges([person])
+ self.assertEqual(
+ ("/@@/product-badge", "Pting %s" % role), badges[0])
+
+
+class CodeReviewVotePillarAffiliationTestCase(TestBranchPillarAffiliation):
+
+ layer = DatabaseFunctionalLayer
+
+ def makeCodeReviewVote(self, branch):
+ merge_proposal = self.factory.makeBranchMergeProposal(
+ target_branch=branch)
+ reviewer = self.factory.makePerson()
+ with person_logged_in(merge_proposal.registrant):
+ vote = merge_proposal.nominateReviewer(
+ reviewer, merge_proposal.registrant)
+ return vote
+
+ def test_correct_pillar_is_used(self):
+ branch = self.factory.makeBranch()
+ vote = self.makeCodeReviewVote(branch)
+ adapter = IHasAffiliation(vote)
+ self.assertEqual(branch.product, adapter.getPillar())
+
+ def test_getBranch(self):
+ # The code review vote's target branch is the branch.
+ branch = self.factory.makeBranch()
+ vote = self.makeCodeReviewVote(branch)
+ adapter = IHasAffiliation(vote)
+ self.assertEqual(branch, adapter.getBranch())
+
+ def _check_affiliated_with_distro(self, person, target, role):
+ distroseries = self.factory.makeDistroSeries(distribution=target)
+ sp = self.factory.makeSourcePackage(distroseries=distroseries)
+ branch = self.factory.makeBranch(sourcepackage=sp)
+ vote = self.makeCodeReviewVote(branch)
+ [badges] = IHasAffiliation(vote).getAffiliationBadges([person])
+ self.assertEqual(
+ ("/@@/distribution-badge", "Pting %s" % role), badges[0])
+
+ def _check_affiliated_with_product(self, person, target, role):
+ branch = self.factory.makeBranch(product=target)
+ with person_logged_in(branch.owner):
+ branch.reviewer = person
+ vote = self.makeCodeReviewVote(branch)
+ [badges] = IHasAffiliation(vote).getAffiliationBadges([person])
self.assertEqual(
("/@@/product-badge", "Pting %s" % role), badges[0])
@@ -279,7 +339,8 @@
owner=owner, driver=driver, name='pting')
distroseries = self.factory.makeDistroSeries(
registrant=driver, distribution=distribution)
- [badges] = IHasAffiliation(distroseries).getAffiliationBadges([driver])
+ [badges] = IHasAffiliation(
+ distroseries).getAffiliationBadges([driver])
self.assertEqual(
("/@@/distribution-badge", "Pting driver"), badges[0])
@@ -292,7 +353,8 @@
owner=owner, driver=driver, name='pting')
distroseries = self.factory.makeDistroSeries(
registrant=owner, distribution=distribution)
- [badges] = IHasAffiliation(distroseries).getAffiliationBadges([driver])
+ [badges] = IHasAffiliation(
+ distroseries).getAffiliationBadges([driver])
self.assertEqual(
("/@@/distribution-badge", "Pting driver"), badges[0])
