launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #19497
[Merge] lp:~wgrant/launchpad/bug-1501134 into lp:launchpad
William Grant has proposed merging lp:~wgrant/launchpad/bug-1501134 into lp:launchpad.
Commit message:
Test and fix +merges and +activereviews for all targets.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #1501134 in Launchpad itself: "+activereviews OOPSing on most targets when empty"
https://bugs.launchpad.net/launchpad/+bug/1501134
For more details, see:
https://code.launchpad.net/~wgrant/launchpad/bug-1501134/+merge/272880
Test and fix +merges and +activereviews for all targets.
The SourcePackage views were totally broken, as IGitCollection doesn't (and can't) love SourcePackage. Most +activereviews views were additionally broken on all contexts except GitRef when they listed no MPs, as few contexts have display_name. And +merges was broken on GitRef for the opposite reason.
Several of the new generic tests duplicate some specific old ones, but they can be cleaned up later.
--
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/bug-1501134 into lp:launchpad.
=== modified file 'lib/lp/code/browser/branchmergeproposallisting.py'
--- lib/lp/code/browser/branchmergeproposallisting.py 2015-09-25 14:19:36 +0000
+++ lib/lp/code/browser/branchmergeproposallisting.py 2015-09-30 09:05:40 +0000
@@ -408,7 +408,7 @@
@property
def no_proposal_message(self):
"""Shown when there is no table to show."""
- return "%s has no active code reviews." % self.context.display_name
+ return "%s has no active code reviews." % self.context.displayname
class BranchActiveReviewsView(ActiveReviewsView):
=== modified file 'lib/lp/code/browser/tests/test_branchmergeproposallisting.py'
--- lib/lp/code/browser/tests/test_branchmergeproposallisting.py 2015-09-25 14:19:36 +0000
+++ lib/lp/code/browser/tests/test_branchmergeproposallisting.py 2015-09-30 09:05:40 +0000
@@ -10,12 +10,16 @@
import pytz
from testtools.content import Content
from testtools.content_type import UTF8_TEXT
-from testtools.matchers import Equals
+from testtools.matchers import (
+ Equals,
+ LessThan,
+ )
import transaction
from zope.component import getUtility
from zope.security.proxy import removeSecurityProxy
from lp.app.enums import InformationType
+from lp.app.interfaces.services import IService
from lp.code.browser.branchmergeproposallisting import (
ActiveReviewsView,
BranchMergeProposalListingItem,
@@ -26,9 +30,12 @@
)
from lp.code.interfaces.gitref import IGitRef
from lp.code.interfaces.gitrepository import IGitRepositorySet
+from lp.registry.enums import SharingPermission
from lp.registry.model.personproduct import PersonProduct
from lp.services.database.sqlbase import flush_database_caches
+from lp.services.webapp.publisher import canonical_url
from lp.testing import (
+ admin_logged_in,
ANONYMOUS,
BrowserTestCase,
login,
@@ -236,52 +243,10 @@
"""Test the vote summary for Git."""
-class TestMerges(BrowserTestCase):
+class TestMergesOnce(BrowserTestCase):
layer = DatabaseFunctionalLayer
- def test_person_product(self):
- """The merges view should be enabled for PersonProduct."""
- personproduct = PersonProduct(
- self.factory.makePerson(), self.factory.makeProduct())
- self.getViewBrowser(personproduct, '+merges', rootsite='code')
-
- def test_DistributionSourcePackage(self):
- """The merges view should be enabled for DistributionSourcePackage."""
- package = self.factory.makeDistributionSourcePackage()
- self.getViewBrowser(package, '+merges', rootsite='code')
-
- def test_query_count_bzr(self):
- product = self.factory.makeProduct()
- target = self.factory.makeBranch(
- product=product, information_type=InformationType.USERDATA)
- for i in range(7):
- source = self.factory.makeBranch(
- product=product, information_type=InformationType.USERDATA)
- self.factory.makeBranchMergeProposal(
- source_branch=removeSecurityProxy(source),
- target_branch=target)
- flush_database_caches()
- with StormStatementRecorder() as recorder:
- self.getViewBrowser(
- product, '+merges', rootsite='code', user=product.owner)
- self.assertThat(recorder, HasQueryCount(Equals(41)))
-
- def test_query_count_git(self):
- product = self.factory.makeProduct()
- [target] = self.factory.makeGitRefs(
- target=product, information_type=InformationType.USERDATA)
- for i in range(7):
- [source] = self.factory.makeGitRefs(
- target=product, information_type=InformationType.USERDATA)
- self.factory.makeBranchMergeProposalForGit(
- source_ref=source, target_ref=target)
- flush_database_caches()
- with StormStatementRecorder() as recorder:
- self.getViewBrowser(
- product, '+merges', rootsite='code', user=product.owner)
- self.assertThat(recorder, HasQueryCount(Equals(38)))
-
def test_productseries_bzr(self):
target = self.factory.makeBranch()
with person_logged_in(target.product.owner):
@@ -302,6 +267,313 @@
self.assertIn(identity, view.contents)
+class BranchMergeProposalListingTestMixin:
+
+ layer = DatabaseFunctionalLayer
+
+ supports_privacy = True
+ supports_git = True
+ supports_bzr = True
+
+ bzr_branch = None
+ git_ref = None
+
+ def makeBzrMergeProposal(self):
+ information_type = (
+ InformationType.USERDATA if self.supports_privacy else None)
+ target = self.bzr_branch
+ if target is None:
+ target = self.factory.makeBranch(
+ target=self.bzr_target, information_type=information_type)
+ source = self.factory.makeBranch(
+ target=self.bzr_target, owner=self.owner,
+ information_type=information_type)
+ return self.factory.makeBranchMergeProposal(
+ source_branch=source, target_branch=target,
+ set_state=BranchMergeProposalStatus.NEEDS_REVIEW)
+
+ def makeGitMergeProposal(self):
+ information_type = (
+ InformationType.USERDATA if self.supports_privacy else None)
+ target = self.git_ref
+ if target is None:
+ [target] = self.factory.makeGitRefs(
+ target=self.git_target, information_type=information_type)
+ [source] = self.factory.makeGitRefs(
+ target=self.git_target, owner=self.owner,
+ information_type=information_type)
+ return self.factory.makeBranchMergeProposalForGit(
+ source_ref=source, target_ref=target,
+ set_state=BranchMergeProposalStatus.NEEDS_REVIEW)
+
+ def test_bzr(self):
+ """The merges view should be enabled for the target."""
+ if not self.supports_bzr:
+ self.skipTest("Context doesn't support Bazaar branches.")
+ with admin_logged_in():
+ bmp = self.makeBzrMergeProposal()
+ url = canonical_url(bmp, force_local_path=True)
+ browser = self.getViewBrowser(
+ self.context, self.view_name, rootsite='code', user=self.user)
+ self.assertIn(url, browser.contents)
+
+ def test_git(self):
+ """The merges view should be enabled for the target."""
+ if not self.supports_git:
+ self.skipTest("Context doesn't support Git repositories.")
+ with admin_logged_in():
+ bmp = self.makeGitMergeProposal()
+ url = canonical_url(bmp, force_local_path=True)
+ browser = self.getViewBrowser(
+ self.context, self.view_name, rootsite='code', user=self.user)
+ self.assertIn(url, browser.contents)
+
+ def test_query_count_bzr(self):
+ if not self.supports_bzr:
+ self.skipTest("Context doesn't support Bazaar branches.")
+ with admin_logged_in():
+ for i in range(7):
+ self.makeBzrMergeProposal()
+ flush_database_caches()
+ with StormStatementRecorder() as recorder:
+ self.getViewBrowser(
+ self.context, self.view_name, rootsite='code', user=self.user)
+ self.assertThat(recorder, HasQueryCount(LessThan(51)))
+
+ def test_query_count_git(self):
+ if not self.supports_git:
+ self.skipTest("Context doesn't support Git repositories.")
+ with admin_logged_in():
+ for i in range(7):
+ self.makeGitMergeProposal()
+ flush_database_caches()
+ with StormStatementRecorder() as recorder:
+ self.getViewBrowser(
+ self.context, self.view_name, rootsite='code', user=self.user)
+ self.assertThat(recorder, HasQueryCount(LessThan(47)))
+
+
+class MergesTestMixin(BranchMergeProposalListingTestMixin):
+
+ view_name = '+merges'
+
+ def test_none(self):
+ """The merges view should be enabled for the target."""
+ browser = self.getViewBrowser(
+ self.context, self.view_name, rootsite='code', user=self.user)
+ self.assertIn("has no merge proposals", browser.contents)
+
+
+class ActiveReviewsTestMixin(BranchMergeProposalListingTestMixin):
+
+ view_name = '+activereviews'
+
+ def test_none(self):
+ """The active reviews view should be enabled for the target."""
+ browser = self.getViewBrowser(
+ self.context, self.view_name, rootsite='code', user=self.user)
+ self.assertIn("has no active code reviews", browser.contents)
+
+
+class ProductContextMixin:
+
+ def setUp(self):
+ super(ProductContextMixin, self).setUp()
+ self.context = self.factory.makeProduct()
+ self.git_target = self.bzr_target = self.context = (
+ self.factory.makeProduct())
+ self.user = self.git_target.owner
+ self.owner = None
+
+
+class ProjectGroupContextMixin:
+
+ def setUp(self):
+ super(ProjectGroupContextMixin, self).setUp()
+ self.context = self.factory.makeProject()
+ self.git_target = self.bzr_target = self.factory.makeProduct(
+ projectgroup=self.context)
+ self.user = self.git_target.owner
+ self.owner = None
+
+
+class DistributionSourcePackageContextMixin:
+
+ # Distribution branches don't have access_policy set.
+ supports_privacy = False
+
+ def setUp(self):
+ super(DistributionSourcePackageContextMixin, self).setUp()
+ self.git_target = self.context = (
+ self.factory.makeDistributionSourcePackage())
+ with admin_logged_in():
+ getUtility(IService, "sharing").sharePillarInformation(
+ self.context.distribution, self.context.distribution.owner,
+ self.context.distribution.owner,
+ {InformationType.USERDATA: SharingPermission.ALL})
+ distroseries = self.factory.makeDistroSeries(
+ distribution=self.context.distribution)
+ self.bzr_target = distroseries.getSourcePackage(
+ self.context.sourcepackagename)
+ self.user = self.context.distribution.owner
+ self.owner = None
+
+
+class SourcePackageContextMixin:
+
+ # Distribution branches don't have access_policy set.
+ supports_privacy = False
+ supports_git = False
+
+ def setUp(self):
+ super(SourcePackageContextMixin, self).setUp()
+ self.bzr_target = self.context = self.factory.makeSourcePackage()
+ self.user = self.context.distribution.owner
+ self.owner = None
+
+
+class PersonContextMixin:
+
+ def setUp(self):
+ super(PersonContextMixin, self).setUp()
+ self.context = self.factory.makePerson()
+ self.bzr_target = self.git_target = self.factory.makeProduct()
+ self.user = self.bzr_target.owner
+ self.owner = self.context
+
+
+class PersonProductContextMixin:
+
+ def setUp(self):
+ super(PersonProductContextMixin, self).setUp()
+ self.context = PersonProduct(
+ self.factory.makePerson(), self.factory.makeProduct())
+ self.bzr_target = self.git_target = self.context.product
+ self.user = self.context.product.owner
+ self.owner = self.context.person
+
+
+class BranchContextMixin:
+
+ supports_git = False
+
+ def setUp(self):
+ super(BranchContextMixin, self).setUp()
+ self.bzr_target = self.factory.makeProduct()
+ self.context = self.bzr_branch = self.factory.makeBranch(
+ target=self.bzr_target)
+ self.user = self.bzr_target.owner
+ self.owner = None
+
+
+class GitRefContextMixin:
+
+ supports_bzr = False
+
+ def setUp(self):
+ super(GitRefContextMixin, self).setUp()
+ self.git_target = self.factory.makeProduct()
+ self.context = self.git_ref = self.factory.makeGitRefs(
+ target=self.git_target)[0]
+ self.user = self.git_target.owner
+ self.owner = None
+
+
+class TestProductMerges(
+ ProductContextMixin, MergesTestMixin, BrowserTestCase):
+
+ pass
+
+
+class TestProjectGroupMerges(
+ ProjectGroupContextMixin, MergesTestMixin, BrowserTestCase):
+
+ pass
+
+
+class TestDistributionSourcePackageMerges(
+ DistributionSourcePackageContextMixin, MergesTestMixin,
+ BrowserTestCase):
+
+ pass
+
+
+class TestSourcePackageMerges(
+ SourcePackageContextMixin, MergesTestMixin, BrowserTestCase):
+
+ pass
+
+
+class TestPersonMerges(PersonContextMixin, MergesTestMixin, BrowserTestCase):
+
+ pass
+
+
+class TestPersonProductMerges(
+ PersonProductContextMixin, MergesTestMixin, BrowserTestCase):
+
+ pass
+
+
+class TestBranchMerges(BranchContextMixin, MergesTestMixin, BrowserTestCase):
+
+ pass
+
+
+class TestGitRefMerges(GitRefContextMixin, MergesTestMixin, BrowserTestCase):
+
+ pass
+
+
+class TestProductActiveReviews(
+ ProductContextMixin, ActiveReviewsTestMixin, BrowserTestCase):
+
+ pass
+
+
+class TestProjectGroupActiveReviews(
+ ProjectGroupContextMixin, ActiveReviewsTestMixin, BrowserTestCase):
+
+ pass
+
+
+class TestDistributionSourcePackageActiveReviews(
+ DistributionSourcePackageContextMixin, ActiveReviewsTestMixin,
+ BrowserTestCase):
+
+ pass
+
+
+class TestSourcePackageActiveReviews(
+ SourcePackageContextMixin, ActiveReviewsTestMixin, BrowserTestCase):
+
+ pass
+
+
+class TestPersonActiveReviews(
+ PersonContextMixin, ActiveReviewsTestMixin, BrowserTestCase):
+
+ pass
+
+
+class TestPersonProductActiveReviews(
+ PersonProductContextMixin, ActiveReviewsTestMixin, BrowserTestCase):
+
+ pass
+
+
+class TestBranchActiveReviews(
+ BranchContextMixin, ActiveReviewsTestMixin, BrowserTestCase):
+
+ pass
+
+
+class TestGitRefActiveReviews(
+ GitRefContextMixin, ActiveReviewsTestMixin, BrowserTestCase):
+
+ pass
+
+
class ActiveReviewGroupsTestMixin:
"""Tests for groupings used for active reviews."""
@@ -536,8 +808,8 @@
"""Test reviews of references in Git repositories."""
-class PersonActiveReviewsPerformanceMixin:
- """Test the performance of the person's active reviews page."""
+class ActiveReviewsPerformanceMixin:
+ """Test the performance of the active reviews page."""
layer = LaunchpadFunctionalLayer
@@ -627,11 +899,11 @@
self.assertThat(recorder2, HasQueryCount(Equals(recorder1.count)))
-class PersonActiveReviewsPerformanceBzr(
- PersonActiveReviewsPerformanceMixin, BzrMixin, TestCaseWithFactory):
- """Test the performance of the person's active reviews page for Bazaar."""
-
-
-class PersonActiveReviewsPerformanceGit(
- PersonActiveReviewsPerformanceMixin, GitMixin, TestCaseWithFactory):
- """Test the performance of the person's active reviews page for Git."""
+class ActiveReviewsPerformanceBzr(
+ ActiveReviewsPerformanceMixin, BzrMixin, TestCaseWithFactory):
+ """Test the performance of the active reviews page for Bazaar."""
+
+
+class ActiveReviewsPerformanceGit(
+ ActiveReviewsPerformanceMixin, GitMixin, TestCaseWithFactory):
+ """Test the performance of the active reviews page for Git."""
=== modified file 'lib/lp/code/interfaces/gitref.py'
--- lib/lp/code/interfaces/gitref.py 2015-09-17 08:52:02 +0000
+++ lib/lp/code/interfaces/gitref.py 2015-09-30 09:05:40 +0000
@@ -107,6 +107,9 @@
title=_("Display name"), required=True, readonly=True,
description=_("Display name of the reference."))
+ displayname = Attribute(
+ "Copy of display_name for IHasMergeProposals views.")
+
commit_message_first_line = TextLine(
title=_("The first line of the commit message."),
required=True, readonly=True)
=== modified file 'lib/lp/code/model/gitref.py'
--- lib/lp/code/model/gitref.py 2015-09-21 09:57:57 +0000
+++ lib/lp/code/model/gitref.py 2015-09-30 09:05:40 +0000
@@ -65,6 +65,9 @@
"""See `IGitRef`."""
return self.identity
+ # For IHasMergeProposals views.
+ displayname = display_name
+
@property
def name(self):
"""See `IGitRef`."""
=== modified file 'lib/lp/code/model/hasbranches.py'
--- lib/lp/code/model/hasbranches.py 2015-05-12 17:30:40 +0000
+++ lib/lp/code/model/hasbranches.py 2015-09-30 09:05:40 +0000
@@ -27,6 +27,7 @@
IAllGitRepositories,
IGitCollection,
)
+from lp.registry.interfaces.sourcepackage import ISourcePackage
from lp.services.database.decoratedresultset import DecoratedResultSet
@@ -66,8 +67,13 @@
collection = collection.visibleByUser(visible_by_user)
return collection.getMergeProposals(status, eager_load=False)
- proposals = _getProposals(IBranchCollection).union(
- _getProposals(IGitCollection))
+ # SourcePackage Bazaar branches are an aberration which was not
+ # replicated for Git, so SourcePackage does not support Git.
+ if ISourcePackage.providedBy(self):
+ proposals = _getProposals(IBranchCollection)
+ else:
+ proposals = _getProposals(IBranchCollection).union(
+ _getProposals(IGitCollection))
if not eager_load:
return proposals
else:
Follow ups
