Launchpad itself

Merge lp:~rockstar/launchpad/merge-queue-index into lp:launchpad/db-devel

  1. merge-queue-index
  2. Merge into db-devel
Proposed by Paul Hummer
Status: Merged
Approved by: Gavin Panella
Approved revision: no longer in the source branch.
Merged at revision: 9943
Proposed branch: lp:~rockstar/launchpad/merge-queue-index
Merge into: lp:launchpad/db-devel
Diff against target: 433 lines (+302/-3)
11 files modified
lib/lp/code/browser/branch.py (+6/-1)
lib/lp/code/browser/branchmergequeue.py (+79/-0)
lib/lp/code/browser/configure.zcml (+26/-0)
lib/lp/code/browser/tests/test_branchmergequeue.py (+127/-0)
lib/lp/code/model/branchmergequeue.py (+3/-0)
lib/lp/code/templates/branch-pending-merges.pt (+15/-1)
lib/lp/code/templates/branchmergequeue-index.pt (+39/-0)
lib/lp/testing/__init__.py (+2/-0)
lib/lp/testing/factory.py (+1/-1)
setup.py (+2/-0)
versions.cfg (+2/-0)
To merge this branch: bzr merge lp:~rockstar/launchpad/merge-queue-index
Reviewer Review Type Date Requested Status
Gavin Panella (community) Approve
Review via email: mp+39488@code.launchpad.net

Commit message

Add branch merge queue index page. Add soupmatchers library dependency.

Description of the change

This branch adds a (really basic) index page for branch merge queues, as long as a flow for creating a merge queue and linking a branch to it. This branch also adds some more things, including:

 - The addition of james_w's soupmatchers library for HTML page matchers. This makes pagetest-type unit tests that we write in code a little nicer.
 - Fixed a bug in getUserBrowser where the authentication didn't support any passwords but 'test'. I fixed this by setting the password for auth to the sooper sekrit attribute that holds the cleartext password.
 - I also fixed a bug in the factory where the branch merge queues owner and registrant were being specified backwards.

This work is hiding behind a feature flag, and the UI is really raw, but I need to get this (and the next branch) landed so that others can also start working on branch merge queues.

To post a comment you must log in.
Revision history for this message
Gavin Panella (allenap) wrote :

Merge queues are cool. A few pretty minor comments.

[1]

+ def enable_queue_flag(self):
+ getFeatureStore().add(FeatureFlag(
+ scope=u'default', flag=u'code.branchmergequeue',
+ value=u'on', priority=1))

There's a neat context manager in lp.testing that you might like. I
think the flag only needs to be enabled on the branch index page, so
something like this might work:

    from lp.testing import feature_flags, set_feature_flag

    with feature_flags():
        set_feature_flag(u'code.branchmergequeue', u'on')
        browser = self.getUserBrowser(
            canonical_url(branch), user=rockstar)

Okay, that's not actually much clearer. Anyway, it's there.

[2]

I think you should ad a short test to show that the +create-queue link
is not available when the feature flag is not set.

[3]

+ if configuration is None:
+ configuration = unicode(simplejson.dumps({}))

The docstring for unicode says:

  Create a new Unicode object from the given encoded string.
  encoding defaults to the current default string encoding.

So I think this would be more correct as:

  configuration = simplejson.dumps({}).decode('ascii')

Or:

  configuration = simplejson.dumps({}, ensure_ascii=False)

In practice it makes bugger all difference though because I doubt
Launchpad will ever run with a default encoding that isn't a superset
of ASCII.

I should just shut up :)

[4]

+ <h4>Merge Queue</h4>
+ This branch is not managed by a queue.

Maybe a test for this too.

[5]

+ password = naked_user._password_cleartext_cached

This clobbers the password argument. Can you remove the argument? (In
a follow-on branch is fine if you need to get this landed.)

review: Approve
Revision history for this message
Gavin Panella (allenap) wrote :

> [5]
>
> + password = naked_user._password_cleartext_cached
>
> This clobbers the password argument. Can you remove the argument? (In
> a follow-on branch is fine if you need to get this landed.)

Erm, not true, as rockstar just pointed out to me on IRC.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/code/browser/branch.py'
2--- lib/lp/code/browser/branch.py 2010-09-30 00:37:25 +0000
3+++ lib/lp/code/browser/branch.py 2010-11-03 19:19:49 +0000
4@@ -296,7 +296,8 @@
5 links = [
6 'add_subscriber', 'browse_revisions', 'create_recipe', 'link_bug',
7 'link_blueprint', 'register_merge', 'source', 'subscription',
8- 'edit_status', 'edit_import', 'upgrade_branch', 'view_recipes']
9+ 'edit_status', 'edit_import', 'upgrade_branch', 'view_recipes',
10+ 'create_queue']
11
12 @enabled_with_permission('launchpad.Edit')
13 def edit_status(self):
14@@ -388,6 +389,10 @@
15 text = 'Create packaging recipe'
16 return Link('+new-recipe', text, enabled=enabled, icon='add')
17
18+ @enabled_with_permission('launchpad.Edit')
19+ def create_queue(self):
20+ return Link('+create-queue', 'Create a new queue', icon='add')
21+
22
23 class BranchMirrorMixin:
24 """Provide mirror_location property.
25
26=== added file 'lib/lp/code/browser/branchmergequeue.py'
27--- lib/lp/code/browser/branchmergequeue.py 1970-01-01 00:00:00 +0000
28+++ lib/lp/code/browser/branchmergequeue.py 2010-11-03 19:19:49 +0000
29@@ -0,0 +1,79 @@
30+# Copyright 2010 Canonical Ltd. This software is licensed under the
31+# GNU Affero General Public License version 3 (see the file LICENSE).
32+
33+"""SourcePackageRecipe views."""
34+
35+__metaclass__ = type
36+
37+__all__ = [
38+ 'BranchMergeQueueContextMenu',
39+ 'BranchMergeQueueView',
40+ ]
41+
42+from lazr.restful.interface import copy_field
43+from zope.component import getUtility
44+from zope.interface import Interface
45+
46+from canonical.launchpad.webapp import (
47+ action,
48+ canonical_url,
49+ ContextMenu,
50+ LaunchpadFormView,
51+ LaunchpadView,
52+ )
53+from lp.code.interfaces.branchmergequeue import (
54+ IBranchMergeQueue,
55+ IBranchMergeQueueSource,
56+ )
57+
58+
59+class BranchMergeQueueContextMenu(ContextMenu):
60+ """Context menu for sourcepackage recipes."""
61+
62+ usedfor = IBranchMergeQueue
63+
64+ facet = 'branches'
65+
66+ links = ()
67+
68+
69+class BranchMergeQueueView(LaunchpadView):
70+ """Default view of a SourcePackageRecipe."""
71+
72+ @property
73+ def page_title(self):
74+ return "%(queue_name)s queue owned by %(name)s" % {
75+ 'name': self.context.owner.displayname,
76+ 'queue_name': self.context.name}
77+
78+ label = page_title
79+
80+
81+class BranchMergeQueueAddView(LaunchpadFormView):
82+
83+ title = label = 'Create a new branch merge queue'
84+
85+ class schema(Interface):
86+ name = copy_field(IBranchMergeQueue['name'], readonly=False)
87+ owner = copy_field(IBranchMergeQueue['owner'], readonly=False)
88+ description = copy_field(IBranchMergeQueue['description'],
89+ readonly=False)
90+
91+ def initialize(self):
92+ super(BranchMergeQueueAddView, self).initialize()
93+
94+ @property
95+ def initial_values(self):
96+ return {}
97+
98+ @property
99+ def cancel_url(self):
100+ return canonical_url(self.context)
101+
102+ @action('Create Queue', name='create')
103+ def request_action(self, action, data):
104+ merge_queue = getUtility(IBranchMergeQueueSource).new(
105+ data['name'], data['owner'], self.user, data['description'])
106+ self.context.addToQueue(merge_queue)
107+
108+ self.next_url = canonical_url(merge_queue)
109
110=== modified file 'lib/lp/code/browser/configure.zcml'
111--- lib/lp/code/browser/configure.zcml 2010-10-21 02:53:17 +0000
112+++ lib/lp/code/browser/configure.zcml 2010-11-03 19:19:49 +0000
113@@ -1326,4 +1326,30 @@
114 path_expression="string:+merge-queues/${name}"
115 rootsite="code" />
116
117+ <browser:menus
118+ classes="BranchMergeQueueContextMenu"
119+ module="lp.code.browser.branchmergequeue"/>
120+
121+ <facet facet="branches">
122+ <browser:defaultView
123+ for="lp.code.interfaces.branchmergequeue.IBranchMergeQueue"
124+ name="+index"
125+ layer="lp.code.publisher.CodeLayer" />
126+ <browser:page
127+ for="lp.code.interfaces.branchmergequeue.IBranchMergeQueue"
128+ layer="lp.code.publisher.CodeLayer"
129+ class="lp.code.browser.branchmergequeue.BranchMergeQueueView"
130+ name="+index"
131+ template="../templates/branchmergequeue-index.pt"
132+ permission="zope.Public" />
133+ <browser:page
134+ for="lp.code.interfaces.branch.IBranch"
135+ layer="lp.code.publisher.CodeLayer"
136+ class="lp.code.browser.branchmergequeue.BranchMergeQueueAddView"
137+ name="+create-queue"
138+ template="../../app/templates/generic-edit.pt"
139+ permission="launchpad.Edit" />
140+
141+ </facet>
142+
143 </configure>
144
145=== added file 'lib/lp/code/browser/tests/test_branchmergequeue.py'
146--- lib/lp/code/browser/tests/test_branchmergequeue.py 1970-01-01 00:00:00 +0000
147+++ lib/lp/code/browser/tests/test_branchmergequeue.py 2010-11-03 19:19:49 +0000
148@@ -0,0 +1,127 @@
149+# Copyright 2010 Canonical Ltd. This software is licensed under the
150+# GNU Affero General Public License version 3 (see the file LICENSE).
151+
152+"""Tests for the branch merge queue view classes and templates."""
153+
154+from __future__ import with_statement
155+
156+__metaclass__ = type
157+
158+from mechanize import LinkNotFoundError
159+import re
160+
161+import soupmatchers
162+
163+from canonical.launchpad.webapp import canonical_url
164+from canonical.testing.layers import (
165+ DatabaseFunctionalLayer,
166+ )
167+from lp.services.features.model import FeatureFlag, getFeatureStore
168+from lp.testing import (
169+ ANONYMOUS,
170+ BrowserTestCase,
171+ person_logged_in,
172+ )
173+
174+
175+class TestBranchMergeQueue(BrowserTestCase):
176+ """Test the Branch Merge Queue index page."""
177+
178+ layer = DatabaseFunctionalLayer
179+
180+ def enable_queue_flag(self):
181+ getFeatureStore().add(FeatureFlag(
182+ scope=u'default', flag=u'code.branchmergequeue',
183+ value=u'on', priority=1))
184+
185+ def test_index(self):
186+ """Test the index page of a branch merge queue."""
187+ with person_logged_in(ANONYMOUS):
188+ queue = self.factory.makeBranchMergeQueue()
189+ queue_owner = queue.owner.displayname
190+ queue_registrant = queue.registrant.displayname
191+ queue_description = queue.description
192+ queue_url = canonical_url(queue)
193+
194+ branch = self.factory.makeBranch()
195+ branch_name = branch.bzr_identity
196+ with person_logged_in(branch.owner):
197+ branch.addToQueue(queue)
198+
199+ # XXX: rockstar - bug #666979 - The text argument should really ignore
200+ # whitespace, but it currently doesn't. Now I have two problems.
201+ queue_matcher = soupmatchers.HTMLContains(
202+ soupmatchers.Tag(
203+ 'Page title', 'h1',
204+ text=re.compile('\w*%s queue owned by %s\w*' % (
205+ queue.name, queue.owner.displayname))),
206+ soupmatchers.Tag(
207+ 'Description Label', 'dt',
208+ text=re.compile('\w*Description\w*')),
209+ soupmatchers.Tag(
210+ 'Description Value', 'dd',
211+ text=re.compile('\w*%s\w*' % queue.description)),
212+ soupmatchers.Tag(
213+ 'Branch link', 'a',
214+ text=re.compile('\w*%s\w*' % branch.bzr_identity)))
215+
216+ browser = self.getUserBrowser(canonical_url(queue), user=queue.owner)
217+
218+ self.assertThat(browser.contents, queue_matcher)
219+
220+ def test_create(self):
221+ """Test that branch merge queues can be created from a branch."""
222+ self.enable_queue_flag()
223+ with person_logged_in(ANONYMOUS):
224+ rockstar = self.factory.makePerson(name='rockstar')
225+ branch = self.factory.makeBranch(owner=rockstar)
226+ self.factory.makeBranch(product=branch.product)
227+ owner_name = branch.owner.name
228+
229+ browser = self.getUserBrowser(canonical_url(branch), user=rockstar)
230+
231+ # There shouldn't be a merge queue linked here.
232+ noqueue_matcher = soupmatchers.HTMLContains(
233+ soupmatchers.Tag(
234+ 'Not managed', 'div',
235+ text=re.compile(
236+ '\w*This branch is not managed by a queue.\w*')))
237+ self.assertThat(browser.contents, noqueue_matcher)
238+
239+ browser.getLink('Create a new queue').click()
240+
241+ browser.getControl('Name').value = 'libbob-queue'
242+ browser.getControl('Description').value = (
243+ 'This is a queue for the libbob projects.')
244+ browser.getControl('Create Queue').click()
245+
246+ self.assertEqual(
247+ 'http://code.launchpad.dev/~rockstar/+merge-queues/libbob-queue',
248+ browser.url)
249+
250+ def test_create_unauthorized(self):
251+ """Test that queues can't be created by unauthorized users."""
252+ self.enable_queue_flag()
253+ with person_logged_in(ANONYMOUS):
254+ branch = self.factory.makeBranch()
255+ self.factory.makeBranch(product=branch.product)
256+
257+ browser = self.getUserBrowser(canonical_url(branch))
258+ self.assertRaises(
259+ LinkNotFoundError,
260+ browser.getLink,
261+ 'Create a new queue')
262+
263+ def test_create_featureflag(self):
264+ """Test that the feature flag hides the "create" link."""
265+ with person_logged_in(ANONYMOUS):
266+ rockstar = self.factory.makePerson(name='rockstar')
267+ branch = self.factory.makeBranch(owner=rockstar)
268+ self.factory.makeBranch(product=branch.product)
269+ owner_name = branch.owner.name
270+
271+ browser = self.getUserBrowser(canonical_url(branch), user=rockstar)
272+ self.assertRaises(
273+ LinkNotFoundError,
274+ browser.getLink,
275+ 'Create a new queue')
276
277=== modified file 'lib/lp/code/model/branchmergequeue.py'
278--- lib/lp/code/model/branchmergequeue.py 2010-10-22 04:16:34 +0000
279+++ lib/lp/code/model/branchmergequeue.py 2010-11-03 19:19:49 +0000
280@@ -72,6 +72,9 @@
281 """See `IBranchMergeQueueSource`."""
282 store = IMasterStore(BranchMergeQueue)
283
284+ if configuration is None:
285+ configuration = unicode(simplejson.dumps({}))
286+
287 queue = cls()
288 queue.name = name
289 queue.owner = owner
290
291=== modified file 'lib/lp/code/templates/branch-pending-merges.pt'
292--- lib/lp/code/templates/branch-pending-merges.pt 2010-06-10 07:54:59 +0000
293+++ lib/lp/code/templates/branch-pending-merges.pt 2010-11-03 19:19:49 +0000
294@@ -2,7 +2,9 @@
295 xmlns:tal="http://xml.zope.org/namespaces/tal"
296 xmlns:metal="http://xml.zope.org/namespaces/metal"
297 xmlns:i18n="http://xml.zope.org/namespaces/i18n"
298- tal:define="context_menu view/context/menu:context"
299+ tal:define="
300+ context_menu view/context/menu:context;
301+ features request/features"
302 tal:condition="view/show_merge_links">
303
304 <h3>Branch merges</h3>
305@@ -46,6 +48,18 @@
306 tal:condition="link/enabled"
307 tal:replace="structure link/render"
308 />
309+
310+ <div tal:condition="features/code.branchmergequeue">
311+ <div tal:condition="not: context/merge_queue">
312+ <h4>Merge Queue</h4>
313+ This branch is not managed by a queue.
314+ <div
315+ tal:define="link context_menu/create_queue"
316+ tal:condition="link/enabled"
317+ tal:content="structure link/render"
318+ />
319+ </div>
320+ </div>
321 </div>
322
323 </div>
324
325=== added file 'lib/lp/code/templates/branchmergequeue-index.pt'
326--- lib/lp/code/templates/branchmergequeue-index.pt 1970-01-01 00:00:00 +0000
327+++ lib/lp/code/templates/branchmergequeue-index.pt 2010-11-03 19:19:49 +0000
328@@ -0,0 +1,39 @@
329+<html
330+ xmlns="http://www.w3.org/1999/xhtml"
331+ xmlns:tal="http://xml.zope.org/namespaces/tal"
332+ xmlns:metal="http://xml.zope.org/namespaces/metal"
333+ xmlns:i18n="http://xml.zope.org/namespaces/i18n"
334+ metal:use-macro="view/macro:page/main_side"
335+ i18n:domain="launchpad"
336+>
337+
338+<metal:side fill-slot="side">
339+ <div tal:replace="structure context/@@+global-actions" />
340+</metal:side>
341+
342+<tal:registering metal:fill-slot="registering">
343+ Created by
344+ <tal:registrant replace="structure context/registrant/fmt:link" />
345+ on
346+ <tal:created-on replace="structure context/date_created/fmt:date" />
347+</tal:registering>
348+
349+<div metal:fill-slot="main">
350+ <dl>
351+ <dt>
352+ Description
353+ </dt>
354+ <dd tal:content="context/description"></dd>
355+ </dl>
356+ <div tal:condition="context/branches">
357+ The following branches are managed by this queue:
358+ <ul>
359+ <tal:branches repeat="branch context/branches">
360+ <li>
361+ <a tal:content="structure branch/fmt:link" />
362+ </li>
363+ </tal:branches>
364+ </ul>
365+ </div>
366+</div>
367+</html>
368
369=== modified file 'lib/lp/testing/__init__.py'
370--- lib/lp/testing/__init__.py 2010-10-27 14:20:21 +0000
371+++ lib/lp/testing/__init__.py 2010-11-03 19:19:49 +0000
372@@ -561,6 +561,8 @@
373 user = self.factory.makePerson(password=password)
374 naked_user = removeSecurityProxy(user)
375 email = naked_user.preferredemail.email
376+ if hasattr(naked_user, '_password_cleartext_cached'):
377+ password = naked_user._password_cleartext_cached
378 logout()
379 browser = setupBrowser(
380 auth="Basic %s:%s" % (str(email), password))
381
382=== modified file 'lib/lp/testing/factory.py'
383--- lib/lp/testing/factory.py 2010-10-29 19:28:14 +0000
384+++ lib/lp/testing/factory.py 2010-11-03 19:19:49 +0000
385@@ -1128,7 +1128,7 @@
386 self.getUniqueString('key'): self.getUniqueString('value')}))
387
388 queue = getUtility(IBranchMergeQueueSource).new(
389- name, registrant, owner, description, configuration)
390+ name, owner, registrant, description, configuration)
391 return queue
392
393 def enableDefaultStackingForProduct(self, product, branch=None):
394
395=== modified file 'setup.py'
396--- setup.py 2010-09-18 08:00:27 +0000
397+++ setup.py 2010-11-03 19:19:49 +0000
398@@ -25,6 +25,7 @@
399 # used in zcml.
400 install_requires=[
401 'ampoule',
402+ 'BeautifulSoup',
403 'bzr',
404 'chameleon.core',
405 'chameleon.zpt',
406@@ -63,6 +64,7 @@
407 'RestrictedPython',
408 'setproctitle',
409 'setuptools',
410+ 'soupmatchers',
411 'sourcecodegen',
412 'storm',
413 'testtools',
414
415=== modified file 'versions.cfg'
416--- versions.cfg 2010-10-27 04:23:52 +0000
417+++ versions.cfg 2010-11-03 19:19:49 +0000
418@@ -5,6 +5,7 @@
419 # Alphabetical, case-insensitive, please! :-)
420
421 ampoule = 0.2.0
422+BeautifulSoup = 3.1.0.1
423 bzr = 2.2.0
424 chameleon.core = 1.0b35
425 chameleon.zpt = 1.0b17
426@@ -64,6 +65,7 @@
427 simplejson = 2.0.9
428 simplesettings = 0.4
429 SimpleTal = 4.1
430+soupmatchers = 0.1r53
431 sourcecodegen = 0.6.9
432 storm = 0.18
433 testtools = 0.9.6

Subscribers

People subscribed via source and target branches

to status/vote changes: