Launchpad itself

Code review comment for lp:~allenap/launchpad/initseries-for-one-night-only-bug-793620

  1. initseries-for-one-night-only-bug-793620
  2. Merge into devel
Revision history for this message
Gavin Panella (allenap) wrote :

Okay, Julian explained it to me. The follow-up is quite large, so I'll
explain what I've done:

- Introduced a new matched, EqualsIgnoringWhitespace. Yes, there are
  other ways to do this, but this seems to be a common test that was
  worth cementing. I suppose DocTestMatches is the closest thing to
  it, but this is simpler to grok.

- Removes DistroSeries.is_initialising. As Julian pointed out, this
  hits the database so would be better as a method...

- Introduce DistroSeries.isInitializing() and .isInitialized(). The
  former replaces .is_initialising and the latter reports if there are
  any published sources in the series' main archive.

- These are then used in the +initseries template.

- I also fixed some lint and imports.

post-review.diff
1=== modified file 'lib/lp/registry/browser/distroseries.py'
2--- lib/lp/registry/browser/distroseries.py 2011-06-07 19:49:53 +0000
3+++ lib/lp/registry/browser/distroseries.py 2011-06-09 12:04:01 +0000
4@@ -654,20 +654,20 @@
5 def show_derivation_form(self):
6 return (
7 self.is_derived_series_feature_enabled and
8- not self.context.is_derived_series and
9- not self.context.is_initialising)
10+ not self.context.isInitializing() and
11+ not self.context.isInitialized())
12
13 @property
14- def show_already_derived_message(self):
15+ def show_already_initialized_message(self):
16 return (
17 self.is_derived_series_feature_enabled and
18- self.context.is_derived_series)
19+ self.context.isInitialized())
20
21 @property
22 def show_already_initializing_message(self):
23 return (
24 self.is_derived_series_feature_enabled and
25- self.context.is_initialising)
26+ self.context.isInitializing())
27
28 @property
29 def next_url(self):
30
31=== modified file 'lib/lp/registry/browser/tests/test_distroseries.py'
32--- lib/lp/registry/browser/tests/test_distroseries.py 2011-06-08 08:22:07 +0000
33+++ lib/lp/registry/browser/tests/test_distroseries.py 2011-06-09 12:05:09 +0000
34@@ -85,7 +85,10 @@
35 with_celebrity_logged_in,
36 )
37 from lp.testing.fakemethod import FakeMethod
38-from lp.testing.matchers import HasQueryCount
39+from lp.testing.matchers import (
40+ EqualsIgnoringWhitespace,
41+ HasQueryCount,
42+ )
43 from lp.testing.views import create_initialized_view
44
45
46@@ -321,7 +324,7 @@
47 view.request.features = get_relevant_feature_controller()
48 html_content = view()
49
50- self.assertTrue(derived_series.is_initialising)
51+ self.assertTrue(derived_series.isInitializing())
52 self.assertThat(html_content, portlet_display)
53
54
55@@ -455,9 +458,10 @@
56
57 def test_form_hidden_when_distroseries_is_initialized(self):
58 # The form is hidden when the feature flag is set but the series has
59- # already been derived.
60+ # already been initialized.
61 distroseries = self.factory.makeDistroSeries()
62- self.factory.makeDistroSeriesParent(derived_series=distroseries)
63+ self.factory.makeSourcePackagePublishingHistory(
64+ distroseries=distroseries, archive=distroseries.main_archive)
65 view = create_initialized_view(distroseries, "+initseries")
66 flags = {u"soyuz.derived_series_ui.enabled": u"true"}
67 with FeatureFixture(flags):
68@@ -466,9 +470,10 @@
69 [], root.cssselect("#initseries-form-container"))
70 # Instead an explanatory message is shown.
71 [message] = root.cssselect("p.error.message")
72- self.assertEqual(
73- u"This series has already been derived.",
74- message.text.strip())
75+ self.assertThat(
76+ message.text.strip(), EqualsIgnoringWhitespace(
77+ u"This series already contains source packages "
78+ u"and cannot be initialized again."))
79
80 def test_form_hidden_when_distroseries_is_being_initialized(self):
81 # The form is hidden when the feature flag is set but the series has
82@@ -484,9 +489,10 @@
83 [], root.cssselect("#initseries-form-container"))
84 # Instead an explanatory message is shown.
85 [message] = root.cssselect("p.error.message")
86- self.assertEqual(
87- u"This series is already being initialized.",
88- message.text.strip())
89+ self.assertThat(
90+ message.text.strip(),
91+ EqualsIgnoringWhitespace(
92+ u"This series is already being initialized."))
93
94
95 class DistroSeriesDifferenceMixin:
96
97=== modified file 'lib/lp/registry/interfaces/distroseries.py'
98--- lib/lp/registry/interfaces/distroseries.py 2011-05-31 15:45:19 +0000
99+++ lib/lp/registry/interfaces/distroseries.py 2011-06-09 12:07:12 +0000
100@@ -213,7 +213,7 @@
101 description=_("The version string for this series.")))
102 distribution = exported(
103 Reference(
104- Interface, # Really IDistribution, see circular import fix below.
105+ Interface, # Really IDistribution, see circular import fix below.
106 title=_("Distribution"), required=True,
107 description=_("The distribution for which this is a series.")))
108 distributionID = Attribute('The distribution ID.')
109@@ -239,16 +239,13 @@
110 is_derived_series = Bool(
111 title=u'Is this series a derived series?', readonly=True,
112 description=(u"Whether or not this series is a derived series."))
113- is_initialising = Bool(
114- title=u'Is this series initialising?', readonly=True,
115- description=(u"Whether or not this series is initialising."))
116 datereleased = exported(
117 Datetime(title=_("Date released")))
118 previous_series = exported(
119 ReferenceChoice(
120 title=_("Parent series"),
121 description=_("The series from which this one was branched."),
122- required=True, schema=Interface, # Really IDistroSeries, see below
123+ required=True, schema=Interface, # Really IDistroSeries, see below
124 vocabulary='DistroSeries'),
125 ("devel", dict(exported_as="previous_series")),
126 ("1.0", dict(exported_as="parent_series")),
127@@ -370,7 +367,7 @@
128
129 main_archive = exported(
130 Reference(
131- Interface, # Really IArchive, see below for circular import fix.
132+ Interface, # Really IArchive, see below for circular import fix.
133 title=_('Distribution Main Archive')))
134
135 supported = exported(
136@@ -418,7 +415,7 @@
137 architectures = exported(
138 CollectionField(
139 title=_("All architectures in this series."),
140- value_type=Reference(schema=Interface), # IDistroArchSeries.
141+ value_type=Reference(schema=Interface), # IDistroArchSeries.
142 readonly=True))
143
144 enabled_architectures = Attribute(
145@@ -848,18 +845,18 @@
146
147 @operation_parameters(
148 parent_series=Reference(
149- schema=Interface, # IDistroSeries
150+ schema=Interface, # IDistroSeries
151 title=_("The parent series to consider."),
152 required=False),
153 difference_type=Choice(
154- vocabulary=DBEnumeratedType, # DistroSeriesDifferenceType
155+ vocabulary=DBEnumeratedType, # DistroSeriesDifferenceType
156 title=_("Only return differences of this type."), required=False),
157 source_package_name_filter=TextLine(
158 title=_("Only return differences for packages matching this "
159 "name."),
160 required=False),
161 status=Choice(
162- vocabulary=DBEnumeratedType, # DistroSeriesDifferenceStatus
163+ vocabulary=DBEnumeratedType, # DistroSeriesDifferenceStatus
164 title=_("Only return differences of this status."),
165 required=False),
166 child_version_higher=Bool(
167@@ -886,6 +883,12 @@
168 child's version is higher than the parent's version.
169 """
170
171+ def isInitializing(self):
172+ """Is this series initializing?"""
173+
174+ def isInitialized(self):
175+ """Has this series been initialized?"""
176+
177
178 class IDistroSeriesEditRestricted(Interface):
179 """IDistroSeries properties which require launchpad.Edit."""
180@@ -1032,7 +1035,7 @@
181
182 class DerivationError(Exception):
183 """Raised when there is a problem deriving a distroseries."""
184- webservice_error(400) # Bad Request
185+ webservice_error(400) # Bad Request
186 _message_prefix = "Error deriving distro series"
187
188
189
190=== modified file 'lib/lp/registry/model/distroseries.py'
191--- lib/lp/registry/model/distroseries.py 2011-05-31 15:45:19 +0000
192+++ lib/lp/registry/model/distroseries.py 2011-06-09 11:58:23 +0000
193@@ -801,13 +801,6 @@
194 return not self.getParentSeries() == []
195
196 @property
197- def is_initialising(self):
198- """See `IDistroSeries`."""
199- return not getUtility(
200- IInitialiseDistroSeriesJobSource).getPendingJobsForDistroseries(
201- self).is_empty()
202-
203- @property
204 def bugtargetname(self):
205 """See IBugTarget."""
206 # XXX mpt 2007-07-10 bugs 113258, 113262:
207@@ -2023,6 +2016,17 @@
208 status=status,
209 child_version_higher=child_version_higher)
210
211+ def isInitializing(self):
212+ """See `IDistroSeries`."""
213+ job_source = getUtility(IInitialiseDistroSeriesJobSource)
214+ pending_jobs = job_source.getPendingJobsForDistroseries(self)
215+ return not pending_jobs.is_empty()
216+
217+ def isInitialized(self):
218+ """See `IDistroSeries`."""
219+ published = self.main_archive.getPublishedSources(distroseries=self)
220+ return not published.is_empty()
221+
222
223 class DistroSeriesSet:
224 implements(IDistroSeriesSet)
225
226=== modified file 'lib/lp/registry/templates/distroseries-index.pt'
227--- lib/lp/registry/templates/distroseries-index.pt 2011-05-24 10:08:33 +0000
228+++ lib/lp/registry/templates/distroseries-index.pt 2011-06-09 12:00:16 +0000
229@@ -68,7 +68,8 @@
230 <tal:derivation
231 tal:condition="request/features/soyuz.derived_series_ui.enabled">
232 <div class="yui-u"
233- tal:condition="python:context.is_derived_series or context.is_initialising">
234+ tal:condition="python: context.is_derived_series or
235+ context.isInitializing()">
236 <div tal:replace="structure context/@@+portlet-derivation" />
237 </div>
238 </tal:derivation>
239
240=== modified file 'lib/lp/registry/templates/distroseries-initialize.pt'
241--- lib/lp/registry/templates/distroseries-initialize.pt 2011-06-07 19:49:53 +0000
242+++ lib/lp/registry/templates/distroseries-initialize.pt 2011-06-08 12:57:58 +0000
243@@ -50,11 +50,12 @@
244 </p>
245 </tal:disabled>
246
247- <tal:already-derived condition="view/show_already_derived_message">
248+ <tal:already-initialized condition="view/show_already_initialized_message">
249 <p class="error message">
250- This series has already been derived.
251+ This series already contains source packages and cannot be
252+ initialized again.
253 </p>
254- </tal:already-derived>
255+ </tal:already-initialized>
256
257 <tal:already-initializing
258 condition="view/show_already_initializing_message">
259
260=== modified file 'lib/lp/registry/templates/distroseries-portlet-derivation.pt'
261--- lib/lp/registry/templates/distroseries-portlet-derivation.pt 2011-05-31 11:55:38 +0000
262+++ lib/lp/registry/templates/distroseries-portlet-derivation.pt 2011-06-09 11:59:46 +0000
263@@ -5,7 +5,7 @@
264 id="series-derivation" class="portlet"
265 tal:define="overview_menu context/menu:overview">
266 <tal:is_derived condition="context/is_derived_series">
267- <tal:is_initialised condition="not: context/is_initialising">
268+ <tal:is_initialised condition="not: context/isInitializing()">
269 <tal:one_parent condition="view/has_unique_parent">
270 <h2>Derived from <tal:name replace="view/unique_parent/displayname"/></h2>
271 </tal:one_parent>
272@@ -61,7 +61,7 @@
273 </tal:diffs>
274 </tal:is_initialised>
275 </tal:is_derived>
276- <tal:is_initialising condition="context/is_initialising">
277+ <tal:is_initialising condition="context/isInitializing">
278 <h2>Series initialisation in progress</h2>
279 This series is initialising.
280 </tal:is_initialising>
281
282=== modified file 'lib/lp/registry/tests/test_distroseries.py'
283--- lib/lp/registry/tests/test_distroseries.py 2011-05-31 15:40:10 +0000
284+++ lib/lp/registry/tests/test_distroseries.py 2011-06-09 11:57:05 +0000
285@@ -222,22 +222,31 @@
286 self.assertEquals(registrant, distroseries.registrant)
287 self.assertNotEqual(distroseries.registrant, distroseries.owner)
288
289- def test_is_initialising(self):
290- # The series is_initialising only if there is an initialisation
291- # job with a pending status attached to this series.
292+ def test_isInitializing(self):
293+ # The series method isInitializing() returns True only if there is an
294+ # initialisation job with a pending status attached to this series.
295 distroseries = self.factory.makeDistroSeries()
296 parent_distroseries = self.factory.makeDistroSeries()
297- self.assertEquals(False, distroseries.is_initialising)
298+ self.assertFalse(distroseries.isInitializing())
299 job_source = getUtility(IInitialiseDistroSeriesJobSource)
300 job = job_source.create(distroseries, [parent_distroseries.id])
301- self.assertEquals(True, distroseries.is_initialising)
302+ self.assertTrue(distroseries.isInitializing())
303 job.start()
304- self.assertEquals(True, distroseries.is_initialising)
305+ self.assertTrue(distroseries.isInitializing())
306 job.queue()
307- self.assertEquals(True, distroseries.is_initialising)
308+ self.assertTrue(distroseries.isInitializing())
309 job.start()
310 job.complete()
311- self.assertEquals(False, distroseries.is_initialising)
312+ self.assertFalse(distroseries.isInitializing())
313+
314+ def test_isInitialized(self):
315+ # The series method isInitialized() returns True once the series has
316+ # been initialized.
317+ distroseries = self.factory.makeDistroSeries()
318+ self.assertFalse(distroseries.isInitialized())
319+ self.factory.makeSourcePackagePublishingHistory(
320+ distroseries=distroseries, archive=distroseries.main_archive)
321+ self.assertTrue(distroseries.isInitialized())
322
323
324 class TestDistroSeriesPackaging(TestCaseWithFactory):
325
326=== modified file 'lib/lp/testing/__init__.py'
327--- lib/lp/testing/__init__.py 2011-05-19 15:15:16 +0000
328+++ lib/lp/testing/__init__.py 2011-06-08 13:19:10 +0000
329@@ -163,7 +163,6 @@
330 )
331 from lp.testing.fixture import ZopeEventHandlerFixture
332 from lp.testing.karma import KarmaRecorder
333-from lp.testing.matchers import Provides
334 from lp.testing.windmill import (
335 constants,
336 lpuser,
337@@ -390,6 +389,7 @@
338
339 def assertProvides(self, obj, interface):
340 """Assert 'obj' correctly provides 'interface'."""
341+ from lp.testing.matchers import Provides
342 self.assertThat(obj, Provides(interface))
343
344 def assertClassImplements(self, cls, interface):
345
346=== modified file 'lib/lp/testing/matchers.py'
347--- lib/lp/testing/matchers.py 2011-03-02 23:54:25 +0000
348+++ lib/lp/testing/matchers.py 2011-06-08 13:33:03 +0000
349@@ -8,6 +8,7 @@
350 'DocTestMatches',
351 'DoesNotCorrectlyProvide',
352 'DoesNotProvide',
353+ 'EqualsIgnoringWhitespace',
354 'HasQueryCount',
355 'IsNotProxied',
356 'IsProxied',
357@@ -20,17 +21,17 @@
358 ]
359
360 from lazr.lifecycle.snapshot import Snapshot
361+from testtools import matchers
362 from testtools.content import Content
363 from testtools.content_type import UTF8_TEXT
364 from testtools.matchers import (
365+ DocTestMatches as OriginalDocTestMatches,
366 Equals,
367- DocTestMatches as OriginalDocTestMatches,
368 LessThan,
369 Matcher,
370 Mismatch,
371 MismatchesAll,
372 )
373-from testtools import matchers
374 from zope.interface.exceptions import (
375 BrokenImplementation,
376 BrokenMethodImplementation,
377@@ -44,6 +45,7 @@
378
379 from canonical.launchpad.webapp import canonical_url
380 from canonical.launchpad.webapp.batching import BatchNavigator
381+from lp.testing import normalize_whitespace
382 from lp.testing._login import person_logged_in
383 from lp.testing._webservice import QueryCollector
384
385@@ -290,7 +292,8 @@
386
387 :param singular: The singular header the batch should be using.
388 :param plural: The plural header the batch should be using.
389- :param batch_size: The batch size that should be configured by default.
390+ :param batch_size: The batch size that should be configured by
391+ default.
392 """
393 self._single = Equals(singular)
394 self._plural = Equals(plural)
395@@ -438,3 +441,21 @@
396 text = widget.findAll(attrs={'class': 'yui3-activator-data-box'})[0]
397 text_matcher = DocTestMatches(extract_text(text))
398 return text_matcher.match(matchee)
399+
400+
401+class EqualsIgnoringWhitespace(Equals):
402+ """Compare equality, ignoring whitespace in strings.
403+
404+ Whitespace in strings is normalized before comparison. All other objected
405+ are compared as they come.
406+ """
407+
408+ def __init__(self, expected):
409+ if isinstance(expected, (str, unicode)):
410+ expected = normalize_whitespace(expected)
411+ super(EqualsIgnoringWhitespace, self).__init__(expected)
412+
413+ def match(self, observed):
414+ if isinstance(observed, (str, unicode)):
415+ observed = normalize_whitespace(observed)
416+ return super(EqualsIgnoringWhitespace, self).match(observed)
417
418=== modified file 'lib/lp/testing/tests/test_matchers.py'
419--- lib/lp/testing/tests/test_matchers.py 2011-02-25 07:15:06 +0000
420+++ lib/lp/testing/tests/test_matchers.py 2011-06-08 13:31:16 +0000
421@@ -29,6 +29,7 @@
422 DoesNotContain,
423 DoesNotCorrectlyProvide,
424 DoesNotProvide,
425+ EqualsIgnoringWhitespace,
426 HasQueryCount,
427 IsNotProxied,
428 IsProxied,
429@@ -274,3 +275,36 @@
430 matcher = Contains("bar")
431 mismatch = matcher.match("foo")
432 self.assertEqual("bar", mismatch.expected)
433+
434+
435+class EqualsIgnoringWhitespaceTests(TestCase):
436+
437+ def test_str(self):
438+ matcher = EqualsIgnoringWhitespace("abc")
439+ self.assertEqual("EqualsIgnoringWhitespace('abc')", str(matcher))
440+
441+ def test_match_str(self):
442+ matcher = EqualsIgnoringWhitespace("one \t two \n three")
443+ self.assertIs(None, matcher.match(" one \r two three "))
444+
445+ def test_mismatch_str(self):
446+ matcher = EqualsIgnoringWhitespace("one \t two \n three")
447+ mismatch = matcher.match(" one \r three ")
448+ self.assertEqual(
449+ "'one two three' != 'one three'",
450+ mismatch.describe())
451+
452+ def test_match_unicode(self):
453+ matcher = EqualsIgnoringWhitespace(u"one \t two \n \u1234 ")
454+ self.assertIs(None, matcher.match(u" one \r two \u1234 "))
455+
456+ def test_mismatch_unicode(self):
457+ matcher = EqualsIgnoringWhitespace(u"one \t two \n \u1234 ")
458+ mismatch = matcher.match(u" one \r \u1234 ")
459+ self.assertEqual(
460+ u"u'one two \\u1234' != u'one \\u1234'",
461+ mismatch.describe())
462+
463+ def test_match_non_string(self):
464+ matcher = EqualsIgnoringWhitespace(1234)
465+ self.assertIs(None, matcher.match(1234))

« Back to merge proposal