Merge lp:~wgrant/launchpad/bug-1501134 into lp:launchpad

Proposed by William Grant on 2015-09-30
Status: Merged
Merged at revision: 17780
Proposed branch: lp:~wgrant/launchpad/bug-1501134
Merge into: lp:launchpad
Diff against target: 501 lines (+340/-57)
5 files modified
lib/lp/code/browser/branchmergeproposallisting.py (+1/-1)
lib/lp/code/browser/tests/test_branchmergeproposallisting.py (+325/-54)
lib/lp/code/interfaces/gitref.py (+3/-0)
lib/lp/code/model/gitref.py (+3/-0)
lib/lp/code/model/hasbranches.py (+8/-2)
To merge this branch: bzr merge lp:~wgrant/launchpad/bug-1501134
Reviewer Review Type Date Requested Status
Colin Watson 2015-09-30 Approve on 2015-09-30
Review via email: mp+272880@code.launchpad.net

Commit Message

Test and fix +merges and +activereviews for all targets.

Description of the Change

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.

To post a comment you must log in.
Colin Watson (cjwatson) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/code/browser/branchmergeproposallisting.py'
2--- lib/lp/code/browser/branchmergeproposallisting.py 2015-09-25 14:19:36 +0000
3+++ lib/lp/code/browser/branchmergeproposallisting.py 2015-09-30 10:26:33 +0000
4@@ -408,7 +408,7 @@
5 @property
6 def no_proposal_message(self):
7 """Shown when there is no table to show."""
8- return "%s has no active code reviews." % self.context.display_name
9+ return "%s has no active code reviews." % self.context.displayname
10
11
12 class BranchActiveReviewsView(ActiveReviewsView):
13
14=== modified file 'lib/lp/code/browser/tests/test_branchmergeproposallisting.py'
15--- lib/lp/code/browser/tests/test_branchmergeproposallisting.py 2015-09-25 14:19:36 +0000
16+++ lib/lp/code/browser/tests/test_branchmergeproposallisting.py 2015-09-30 10:26:33 +0000
17@@ -10,12 +10,16 @@
18 import pytz
19 from testtools.content import Content
20 from testtools.content_type import UTF8_TEXT
21-from testtools.matchers import Equals
22+from testtools.matchers import (
23+ Equals,
24+ LessThan,
25+ )
26 import transaction
27 from zope.component import getUtility
28 from zope.security.proxy import removeSecurityProxy
29
30 from lp.app.enums import InformationType
31+from lp.app.interfaces.services import IService
32 from lp.code.browser.branchmergeproposallisting import (
33 ActiveReviewsView,
34 BranchMergeProposalListingItem,
35@@ -26,9 +30,12 @@
36 )
37 from lp.code.interfaces.gitref import IGitRef
38 from lp.code.interfaces.gitrepository import IGitRepositorySet
39+from lp.registry.enums import SharingPermission
40 from lp.registry.model.personproduct import PersonProduct
41 from lp.services.database.sqlbase import flush_database_caches
42+from lp.services.webapp.publisher import canonical_url
43 from lp.testing import (
44+ admin_logged_in,
45 ANONYMOUS,
46 BrowserTestCase,
47 login,
48@@ -236,52 +243,10 @@
49 """Test the vote summary for Git."""
50
51
52-class TestMerges(BrowserTestCase):
53+class TestMergesOnce(BrowserTestCase):
54
55 layer = DatabaseFunctionalLayer
56
57- def test_person_product(self):
58- """The merges view should be enabled for PersonProduct."""
59- personproduct = PersonProduct(
60- self.factory.makePerson(), self.factory.makeProduct())
61- self.getViewBrowser(personproduct, '+merges', rootsite='code')
62-
63- def test_DistributionSourcePackage(self):
64- """The merges view should be enabled for DistributionSourcePackage."""
65- package = self.factory.makeDistributionSourcePackage()
66- self.getViewBrowser(package, '+merges', rootsite='code')
67-
68- def test_query_count_bzr(self):
69- product = self.factory.makeProduct()
70- target = self.factory.makeBranch(
71- product=product, information_type=InformationType.USERDATA)
72- for i in range(7):
73- source = self.factory.makeBranch(
74- product=product, information_type=InformationType.USERDATA)
75- self.factory.makeBranchMergeProposal(
76- source_branch=removeSecurityProxy(source),
77- target_branch=target)
78- flush_database_caches()
79- with StormStatementRecorder() as recorder:
80- self.getViewBrowser(
81- product, '+merges', rootsite='code', user=product.owner)
82- self.assertThat(recorder, HasQueryCount(Equals(41)))
83-
84- def test_query_count_git(self):
85- product = self.factory.makeProduct()
86- [target] = self.factory.makeGitRefs(
87- target=product, information_type=InformationType.USERDATA)
88- for i in range(7):
89- [source] = self.factory.makeGitRefs(
90- target=product, information_type=InformationType.USERDATA)
91- self.factory.makeBranchMergeProposalForGit(
92- source_ref=source, target_ref=target)
93- flush_database_caches()
94- with StormStatementRecorder() as recorder:
95- self.getViewBrowser(
96- product, '+merges', rootsite='code', user=product.owner)
97- self.assertThat(recorder, HasQueryCount(Equals(38)))
98-
99 def test_productseries_bzr(self):
100 target = self.factory.makeBranch()
101 with person_logged_in(target.product.owner):
102@@ -302,6 +267,312 @@
103 self.assertIn(identity, view.contents)
104
105
106+class BranchMergeProposalListingTestMixin:
107+
108+ layer = DatabaseFunctionalLayer
109+
110+ supports_privacy = True
111+ supports_git = True
112+ supports_bzr = True
113+
114+ bzr_branch = None
115+ git_ref = None
116+
117+ def makeBzrMergeProposal(self):
118+ information_type = (
119+ InformationType.USERDATA if self.supports_privacy else None)
120+ target = self.bzr_branch
121+ if target is None:
122+ target = self.factory.makeBranch(
123+ target=self.bzr_target, information_type=information_type)
124+ source = self.factory.makeBranch(
125+ target=self.bzr_target, owner=self.owner,
126+ information_type=information_type)
127+ return self.factory.makeBranchMergeProposal(
128+ source_branch=source, target_branch=target,
129+ set_state=BranchMergeProposalStatus.NEEDS_REVIEW)
130+
131+ def makeGitMergeProposal(self):
132+ information_type = (
133+ InformationType.USERDATA if self.supports_privacy else None)
134+ target = self.git_ref
135+ if target is None:
136+ [target] = self.factory.makeGitRefs(
137+ target=self.git_target, information_type=information_type)
138+ [source] = self.factory.makeGitRefs(
139+ target=self.git_target, owner=self.owner,
140+ information_type=information_type)
141+ return self.factory.makeBranchMergeProposalForGit(
142+ source_ref=source, target_ref=target,
143+ set_state=BranchMergeProposalStatus.NEEDS_REVIEW)
144+
145+ def test_bzr(self):
146+ """The merges view should be enabled for the target."""
147+ if not self.supports_bzr:
148+ self.skipTest("Context doesn't support Bazaar branches.")
149+ with admin_logged_in():
150+ bmp = self.makeBzrMergeProposal()
151+ url = canonical_url(bmp, force_local_path=True)
152+ browser = self.getViewBrowser(
153+ self.context, self.view_name, rootsite='code', user=self.user)
154+ self.assertIn(url, browser.contents)
155+
156+ def test_git(self):
157+ """The merges view should be enabled for the target."""
158+ if not self.supports_git:
159+ self.skipTest("Context doesn't support Git repositories.")
160+ with admin_logged_in():
161+ bmp = self.makeGitMergeProposal()
162+ url = canonical_url(bmp, force_local_path=True)
163+ browser = self.getViewBrowser(
164+ self.context, self.view_name, rootsite='code', user=self.user)
165+ self.assertIn(url, browser.contents)
166+
167+ def test_query_count_bzr(self):
168+ if not self.supports_bzr:
169+ self.skipTest("Context doesn't support Bazaar branches.")
170+ with admin_logged_in():
171+ for i in range(7):
172+ self.makeBzrMergeProposal()
173+ flush_database_caches()
174+ with StormStatementRecorder() as recorder:
175+ self.getViewBrowser(
176+ self.context, self.view_name, rootsite='code', user=self.user)
177+ self.assertThat(recorder, HasQueryCount(LessThan(51)))
178+
179+ def test_query_count_git(self):
180+ if not self.supports_git:
181+ self.skipTest("Context doesn't support Git repositories.")
182+ with admin_logged_in():
183+ for i in range(7):
184+ self.makeGitMergeProposal()
185+ flush_database_caches()
186+ with StormStatementRecorder() as recorder:
187+ self.getViewBrowser(
188+ self.context, self.view_name, rootsite='code', user=self.user)
189+ self.assertThat(recorder, HasQueryCount(LessThan(47)))
190+
191+
192+class MergesTestMixin(BranchMergeProposalListingTestMixin):
193+
194+ view_name = '+merges'
195+
196+ def test_none(self):
197+ """The merges view should be enabled for the target."""
198+ browser = self.getViewBrowser(
199+ self.context, self.view_name, rootsite='code', user=self.user)
200+ self.assertIn("has no merge proposals", browser.contents)
201+
202+
203+class ActiveReviewsTestMixin(BranchMergeProposalListingTestMixin):
204+
205+ view_name = '+activereviews'
206+
207+ def test_none(self):
208+ """The active reviews view should be enabled for the target."""
209+ browser = self.getViewBrowser(
210+ self.context, self.view_name, rootsite='code', user=self.user)
211+ self.assertIn("has no active code reviews", browser.contents)
212+
213+
214+class ProductContextMixin:
215+
216+ def setUp(self):
217+ super(ProductContextMixin, self).setUp()
218+ self.git_target = self.bzr_target = self.context = (
219+ self.factory.makeProduct())
220+ self.user = self.git_target.owner
221+ self.owner = None
222+
223+
224+class ProjectGroupContextMixin:
225+
226+ def setUp(self):
227+ super(ProjectGroupContextMixin, self).setUp()
228+ self.context = self.factory.makeProject()
229+ self.git_target = self.bzr_target = self.factory.makeProduct(
230+ projectgroup=self.context)
231+ self.user = self.git_target.owner
232+ self.owner = None
233+
234+
235+class DistributionSourcePackageContextMixin:
236+
237+ # Distribution branches don't have access_policy set.
238+ supports_privacy = False
239+
240+ def setUp(self):
241+ super(DistributionSourcePackageContextMixin, self).setUp()
242+ self.git_target = self.context = (
243+ self.factory.makeDistributionSourcePackage())
244+ with admin_logged_in():
245+ getUtility(IService, "sharing").sharePillarInformation(
246+ self.context.distribution, self.context.distribution.owner,
247+ self.context.distribution.owner,
248+ {InformationType.USERDATA: SharingPermission.ALL})
249+ distroseries = self.factory.makeDistroSeries(
250+ distribution=self.context.distribution)
251+ self.bzr_target = distroseries.getSourcePackage(
252+ self.context.sourcepackagename)
253+ self.user = self.context.distribution.owner
254+ self.owner = None
255+
256+
257+class SourcePackageContextMixin:
258+
259+ # Distribution branches don't have access_policy set.
260+ supports_privacy = False
261+ supports_git = False
262+
263+ def setUp(self):
264+ super(SourcePackageContextMixin, self).setUp()
265+ self.bzr_target = self.context = self.factory.makeSourcePackage()
266+ self.user = self.context.distribution.owner
267+ self.owner = None
268+
269+
270+class PersonContextMixin:
271+
272+ def setUp(self):
273+ super(PersonContextMixin, self).setUp()
274+ self.context = self.factory.makePerson()
275+ self.bzr_target = self.git_target = self.factory.makeProduct()
276+ self.user = self.bzr_target.owner
277+ self.owner = self.context
278+
279+
280+class PersonProductContextMixin:
281+
282+ def setUp(self):
283+ super(PersonProductContextMixin, self).setUp()
284+ self.context = PersonProduct(
285+ self.factory.makePerson(), self.factory.makeProduct())
286+ self.bzr_target = self.git_target = self.context.product
287+ self.user = self.context.product.owner
288+ self.owner = self.context.person
289+
290+
291+class BranchContextMixin:
292+
293+ supports_git = False
294+
295+ def setUp(self):
296+ super(BranchContextMixin, self).setUp()
297+ self.bzr_target = self.factory.makeProduct()
298+ self.context = self.bzr_branch = self.factory.makeBranch(
299+ target=self.bzr_target)
300+ self.user = self.bzr_target.owner
301+ self.owner = None
302+
303+
304+class GitRefContextMixin:
305+
306+ supports_bzr = False
307+
308+ def setUp(self):
309+ super(GitRefContextMixin, self).setUp()
310+ self.git_target = self.factory.makeProduct()
311+ self.context = self.git_ref = self.factory.makeGitRefs(
312+ target=self.git_target)[0]
313+ self.user = self.git_target.owner
314+ self.owner = None
315+
316+
317+class TestProductMerges(
318+ ProductContextMixin, MergesTestMixin, BrowserTestCase):
319+
320+ pass
321+
322+
323+class TestProjectGroupMerges(
324+ ProjectGroupContextMixin, MergesTestMixin, BrowserTestCase):
325+
326+ pass
327+
328+
329+class TestDistributionSourcePackageMerges(
330+ DistributionSourcePackageContextMixin, MergesTestMixin,
331+ BrowserTestCase):
332+
333+ pass
334+
335+
336+class TestSourcePackageMerges(
337+ SourcePackageContextMixin, MergesTestMixin, BrowserTestCase):
338+
339+ pass
340+
341+
342+class TestPersonMerges(PersonContextMixin, MergesTestMixin, BrowserTestCase):
343+
344+ pass
345+
346+
347+class TestPersonProductMerges(
348+ PersonProductContextMixin, MergesTestMixin, BrowserTestCase):
349+
350+ pass
351+
352+
353+class TestBranchMerges(BranchContextMixin, MergesTestMixin, BrowserTestCase):
354+
355+ pass
356+
357+
358+class TestGitRefMerges(GitRefContextMixin, MergesTestMixin, BrowserTestCase):
359+
360+ pass
361+
362+
363+class TestProductActiveReviews(
364+ ProductContextMixin, ActiveReviewsTestMixin, BrowserTestCase):
365+
366+ pass
367+
368+
369+class TestProjectGroupActiveReviews(
370+ ProjectGroupContextMixin, ActiveReviewsTestMixin, BrowserTestCase):
371+
372+ pass
373+
374+
375+class TestDistributionSourcePackageActiveReviews(
376+ DistributionSourcePackageContextMixin, ActiveReviewsTestMixin,
377+ BrowserTestCase):
378+
379+ pass
380+
381+
382+class TestSourcePackageActiveReviews(
383+ SourcePackageContextMixin, ActiveReviewsTestMixin, BrowserTestCase):
384+
385+ pass
386+
387+
388+class TestPersonActiveReviews(
389+ PersonContextMixin, ActiveReviewsTestMixin, BrowserTestCase):
390+
391+ pass
392+
393+
394+class TestPersonProductActiveReviews(
395+ PersonProductContextMixin, ActiveReviewsTestMixin, BrowserTestCase):
396+
397+ pass
398+
399+
400+class TestBranchActiveReviews(
401+ BranchContextMixin, ActiveReviewsTestMixin, BrowserTestCase):
402+
403+ pass
404+
405+
406+class TestGitRefActiveReviews(
407+ GitRefContextMixin, ActiveReviewsTestMixin, BrowserTestCase):
408+
409+ pass
410+
411+
412 class ActiveReviewGroupsTestMixin:
413 """Tests for groupings used for active reviews."""
414
415@@ -536,8 +807,8 @@
416 """Test reviews of references in Git repositories."""
417
418
419-class PersonActiveReviewsPerformanceMixin:
420- """Test the performance of the person's active reviews page."""
421+class ActiveReviewsPerformanceMixin:
422+ """Test the performance of the active reviews page."""
423
424 layer = LaunchpadFunctionalLayer
425
426@@ -627,11 +898,11 @@
427 self.assertThat(recorder2, HasQueryCount(Equals(recorder1.count)))
428
429
430-class PersonActiveReviewsPerformanceBzr(
431- PersonActiveReviewsPerformanceMixin, BzrMixin, TestCaseWithFactory):
432- """Test the performance of the person's active reviews page for Bazaar."""
433-
434-
435-class PersonActiveReviewsPerformanceGit(
436- PersonActiveReviewsPerformanceMixin, GitMixin, TestCaseWithFactory):
437- """Test the performance of the person's active reviews page for Git."""
438+class ActiveReviewsPerformanceBzr(
439+ ActiveReviewsPerformanceMixin, BzrMixin, TestCaseWithFactory):
440+ """Test the performance of the active reviews page for Bazaar."""
441+
442+
443+class ActiveReviewsPerformanceGit(
444+ ActiveReviewsPerformanceMixin, GitMixin, TestCaseWithFactory):
445+ """Test the performance of the active reviews page for Git."""
446
447=== modified file 'lib/lp/code/interfaces/gitref.py'
448--- lib/lp/code/interfaces/gitref.py 2015-09-17 08:52:02 +0000
449+++ lib/lp/code/interfaces/gitref.py 2015-09-30 10:26:33 +0000
450@@ -107,6 +107,9 @@
451 title=_("Display name"), required=True, readonly=True,
452 description=_("Display name of the reference."))
453
454+ displayname = Attribute(
455+ "Copy of display_name for IHasMergeProposals views.")
456+
457 commit_message_first_line = TextLine(
458 title=_("The first line of the commit message."),
459 required=True, readonly=True)
460
461=== modified file 'lib/lp/code/model/gitref.py'
462--- lib/lp/code/model/gitref.py 2015-09-21 09:57:57 +0000
463+++ lib/lp/code/model/gitref.py 2015-09-30 10:26:33 +0000
464@@ -65,6 +65,9 @@
465 """See `IGitRef`."""
466 return self.identity
467
468+ # For IHasMergeProposals views.
469+ displayname = display_name
470+
471 @property
472 def name(self):
473 """See `IGitRef`."""
474
475=== modified file 'lib/lp/code/model/hasbranches.py'
476--- lib/lp/code/model/hasbranches.py 2015-05-12 17:30:40 +0000
477+++ lib/lp/code/model/hasbranches.py 2015-09-30 10:26:33 +0000
478@@ -27,6 +27,7 @@
479 IAllGitRepositories,
480 IGitCollection,
481 )
482+from lp.registry.interfaces.sourcepackage import ISourcePackage
483 from lp.services.database.decoratedresultset import DecoratedResultSet
484
485
486@@ -66,8 +67,13 @@
487 collection = collection.visibleByUser(visible_by_user)
488 return collection.getMergeProposals(status, eager_load=False)
489
490- proposals = _getProposals(IBranchCollection).union(
491- _getProposals(IGitCollection))
492+ # SourcePackage Bazaar branches are an aberration which was not
493+ # replicated for Git, so SourcePackage does not support Git.
494+ if ISourcePackage.providedBy(self):
495+ proposals = _getProposals(IBranchCollection)
496+ else:
497+ proposals = _getProposals(IBranchCollection).union(
498+ _getProposals(IGitCollection))
499 if not eager_load:
500 return proposals
501 else: