Merge into devel : say-no-bpns-alsoaffects : Code : Launchpad itself

Status:

Merged

Approved by:

Steve Kowalik on 2011-06-08

Approved revision:

no longer in the source branch.

Merged at revision:

13191

Proposed branch:

lp:~stevenk/launchpad/say-no-bpns-alsoaffects

Merge into:

lp:launchpad

Diff against target:

156 lines (+88/-34)

4 files modified

lib/lp/bugs/browser/bugalsoaffects.py (+11/-10)
lib/lp/bugs/browser/tests/test_bugalsoaffects.py (+76/-0)
lib/lp/bugs/stories/bug-also-affects/10-bug-requestdistrofix.txt (+0/-20)
lib/lp/registry/model/distribution.py (+1/-4)

To merge this branch:

bzr merge lp:~stevenk/launchpad/say-no-bpns-alsoaffects

High

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Jeroen T. Vermeulen (community)		2011-06-08	Approve on 2011-06-08
Review via email: mp+63810@code.launchpad.net

Commit message

[r=jtv][bug=307620] Stop inviting users to file a bug on Launchpad if the picker for a distro task fails to find any results, and include whether or not Launchpad can search on binary package names as well.

Description of the change

Stop inviting users to file a bug on Launchpad if the picker for a distro task fails to find any results. Include text that suggest that Launchpad does not track binary package names for the distribution.

Drive-by a fix to IDistribution.has_published_binaries to make use of .is_empty().

Revision history for this message

Jeroen T. Vermeulen (jtv) wrote on 2011-06-08:

#

Download full text (11.0 KiB)

Hi Steve,

My apologies for the delay in reviewing this. You can land this as-is if you really want, but there are a few tweaks I'd like you to consider (if not for now then for the future) and a few things that I do think need fixing.

1 === modified file 'lib/lp/bugs/browser/bugalsoaffects.py'
2 --- lib/lp/bugs/browser/bugalsoaffects.py 2011-05-27 21:12:25 +0000
3 +++ lib/lp/bugs/browser/bugalsoaffects.py 2011-06-08 04:03:29 +0000
4 @@ -418,16 +418,16 @@
5 self.widgets['sourcepackagename'].name)
6 if sourcepackagename is None and entered_package:
7 # The entered package doesn't exist.
8 - filebug_url = "%s/+filebug" % canonical_url(
9 - getUtility(ILaunchpadCelebrities).launchpad)
10 - self.setFieldError(
11 - 'sourcepackagename',
12 - structured(
13 - 'There is no package in %s named "%s". If it should'
14 - ' be here, <a href="%s">report this as a bug</a>.',
15 - distribution.displayname,
16 - entered_package,
17 - filebug_url))
18 + binary_tracking = ''
19 + if not distribution.has_published_binaries:
20 + binary_tracking = structured(
21 + ' Launchpad does not track binary package names '
22 + 'in %s.' % distribution.displayname)
23 + error = structured(
24 + 'There is no package in %s named "%s".%s',
25 + distribution.displayname, entered_package,
26 + binary_tracking)
27 + self.setFieldError('sourcepackagename', error)

You're running binary_tracking through structured twice here, and maybe compensating for that by not escaping distribution.displayname. To be honest it's not clear to me how this will work exactly: will the distro name in binary_tracking end up being escaped exactly once? Even if this is correct, please make it clearly correct as well. Otherwise an injection vulnerability could sneak in during future maintenance.

Also, it's traditionally been considered clearer in Launchpad to write alternative initializations of a variable in if/else form, rather than initializing it for both cases once and then conditionally overriding it:

18 + binary_tracking = ''
19 + if not distribution.has_published_binaries:
20 + binary_tracking = structured(
21 + ' Launchpad does not track binary package names '
22 + 'in %s.' % distribution.displayname)

…would be more obvious if written along the lines of…

    if distribution.has_published_binaries:
        binary_tracking = ""
    else:
        binary_tracking = "Whaaaa there's an error!"

Or you could use +=, or even (if you really want to be fancy about the whitespace) gather components of your message up in a list that you then " ".join().

In good news, structured supports keyword args. So you could do something like:

    error = structured(
        'There is no package in %(distro)s named "%(package)s". ' + binary_tracking,
        distro=distribution.displayname, package=entered_package)

N...

Hi Steve,

My apologies for the delay in reviewing this.  You can land this as-is if you really want, but there are a few tweaks I'd like you to consider (if not for now then for the future) and a few things that I do think need fixing.

1	=== modified file 'lib/lp/bugs/browser/bugalsoaffects.py'
2	--- lib/lp/bugs/browser/bugalsoaffects.py	2011-05-27 21:12:25 +0000
3	+++ lib/lp/bugs/browser/bugalsoaffects.py	2011-06-08 04:03:29 +0000
4	@@ -418,16 +418,16 @@
5	             self.widgets['sourcepackagename'].name)
6	         if sourcepackagename is None and entered_package:
7	             # The entered package doesn't exist.
8	-            filebug_url = "%s/+filebug" % canonical_url(
9	-                getUtility(ILaunchpadCelebrities).launchpad)
10	-            self.setFieldError(
11	-                'sourcepackagename',
12	-                structured(
13	-                'There is no package in %s named "%s". If it should'
14	-                ' be here, <a href="%s">report this as a bug</a>.',
15	-                distribution.displayname,
16	-                entered_package,
17	-                filebug_url))
18	+            binary_tracking = ''
19	+            if not distribution.has_published_binaries:
20	+                binary_tracking = structured(
21	+                    ' Launchpad does not track binary package names '
22	+                    'in %s.' % distribution.displayname)
23	+            error = structured(
24	+                'There is no package in %s named "%s".%s',
25	+                distribution.displayname, entered_package,
26	+                binary_tracking)
27	+            self.setFieldError('sourcepackagename', error)

You're running binary_tracking through structured twice here, and maybe compensating for that by not escaping distribution.displayname.  To be honest it's not clear to me how this will work exactly: will the distro name in binary_tracking end up being escaped exactly once?  Even if this is correct, please make it clearly correct as well.  Otherwise an injection vulnerability could sneak in during future maintenance.

Also, it's traditionally been considered clearer in Launchpad to write alternative initializations of a variable in if/else form, rather than initializing it for both cases once and then conditionally overriding it:

18	+            binary_tracking = ''
19	+            if not distribution.has_published_binaries:
20	+                binary_tracking = structured(
21	+                    ' Launchpad does not track binary package names '
22	+                    'in %s.' % distribution.displayname)

…would be more obvious if written along the lines of…

if distribution.has_published_binaries:
        binary_tracking = ""
    else:
        binary_tracking = "Whaaaa there's an error!"

Or you could use +=, or even (if you really want to be fancy about the whitespace) gather components of your message up in a list that you then " ".join().

In good news, structured supports keyword args.  So you could do something like:

error = structured(
        'There is no package in %(distro)s named "%(package)s". ' + binary_tracking,
        distro=distribution.displayname, package=entered_package)

Now you don't need to care exactly which interpolations the message will be needing — they're all there.

On a more high-level note, have you considered creating a function or method for "compose a suitable error message"?  The benefits will become obvious shortly.

32	=== added file 'lib/lp/bugs/browser/tests/test_bugalsoaffects.py'
33	--- lib/lp/bugs/browser/tests/test_bugalsoaffects.py	1970-01-01 00:00:00 +0000
34	+++ lib/lp/bugs/browser/tests/test_bugalsoaffects.py	2011-06-08 04:03:29 +0000

59	+    def setUp(self):
60	+        super(TestBugAlsoAffectsDistribution, self).setUp()
61	+        self.admin = getUtility(IPersonSet).getByEmail(ADMIN_EMAIL)
62	+        self.distribution = self.factory.makeDistribution()
63	+        with person_logged_in(self.admin):
64	+           self.distribution.official_malone = True

It's not generally a good idea to set up test data in setUp(); it's better to use it purely for setting up the operational side of the test environment.  It would cost very little in terms of brevity or speed to have a makeBuggyDistro() methods on the test, for instance, but it would make the tests self-contained.  That really helps in maintenance.  It may even help speed-wise, in that all of setUp is run before each test method regardless of whether everything it does is actually needed.

(Speaking from a long history of nightmare test trauma here, which leads me to a related point: good job removing a piece of doctest in this branch!)

I wouldn't bother with logging in the admin just to set official_malone.  If the factory won't set this for you, I'd just ram it right in with removeSecurityProxy.  Logging in is normally something you do to verify privileges for a particular class of user.

So instead of:

61	+        self.admin = getUtility(IPersonSet).getByEmail(ADMIN_EMAIL)
63	+        with person_logged_in(self.admin):
64	+           self.distribution.official_malone = True

Just do:

removeSecurityProxy(self.distribution).official_malone = True

66	+    def test_bug_alsoaffects_spn_exists(self):
67	+        bug = self.factory.makeBug()
68	+        spn = self.factory.makeSourcePackageName()
69	+        browser = self.getUserBrowser()
70	+        browser.open(canonical_url(bug))
71	+        browser.getLink(url='+distrotask').click()
72	+        browser.getControl('Distribution').value = [self.distribution.name]
73	+        browser.getControl('Source Package Name').value = spn.name
74	+        browser.getControl('Continue').click()
75	+        self.assertEqual([], get_feedback_messages(browser.contents))

It took me a while to figure out what this test was meant to verify!  What you really have here is a happy-path story test.  The name didn't fall into place until I'd read the other test cases; it might be helpful to say something like "user sets also-effects" in the name or a comment, rather than just "spn exists."  The current name sort of implies that if you also want to test handling of a nonexistent distro name, you'd have to repeat this exact test as test_bug_alsoaffects_distro_exists.

77	+    def test_bug_alsoaffects_spn_not_exists_with_published_binaries(self):
78	+        bug = self.factory.makeBug()
79	+        distroseries = self.factory.makeDistroSeries(
80	+            distribution=self.distribution)
81	+        das = self.factory.makeDistroArchSeries(distroseries=distroseries)
82	+        bpph = self.factory.makeBinaryPackagePublishingHistory(
83	+            distroarchseries=das, status=PackagePublishingStatus.PUBLISHED)
84	+        self.assertTrue(self.distribution.has_published_binaries)
85	+        browser = self.getUserBrowser()
86	+        browser.open(canonical_url(bug))
87	+        browser.getLink(url='+distrotask').click()
88	+        browser.getControl('Distribution').value = [self.distribution.name]
89	+        browser.getControl('Source Package Name').value = 'does-not-exist'
90	+        browser.getControl('Continue').click()
91	+        expected = [
92	+            u'There is 1 error.',
93	+            u'There is no package in %s named "does-not-exist".' % (
94	+                self.distribution.displayname)]
95	+        self.assertEqual(expected, get_feedback_messages(browser.contents))

Is this worth testing at such a high level, with the whole click-through story?  Since you're enumerating failure cases here, it might be more sensible to instantiate the view and test its handling of the situation directly.

97	+    def test_bug_alsoaffects_spn_not_exists_with_no_binaries(self):
98	+        bug = self.factory.makeBug()
99	+        browser = self.getUserBrowser()
100	+        browser.open(canonical_url(bug))
101	+        browser.getLink(url='+distrotask').click()
102	+        browser.getControl('Distribution').value = [self.distribution.name]
103	+        browser.getControl('Source Package Name').value = 'does-not-exist'
104	+        browser.getControl('Continue').click()
105	+        expected = [
106	+            u'There is 1 error.',
107	+            u'There is no package in %s named "does-not-exist". Launchpad '
108	+            'does not track binary package names in %s.' % (
109	+                self.distribution.displayname,
110	+                self.distribution.displayname)]
111	+        self.assertEqual(expected, get_feedback_messages(browser.contents))

And this is definitely at too high a level for my liking.  You're really just testing composition of the error string here.  There's no reason to go through a simulated browser for that: if you isolated construction of the error string, you'd only need to test once that whatever string comes out will be posted as an error message.  Details of the error message could then be tested directly and cheaply.

That also means that you can easily test more detail on the error message.  For instance, do the distro displaynames get escaped properly (and exactly once)?

113	=== modified file 'lib/lp/bugs/stories/bug-also-affects/10-bug-requestdistrofix.txt'
114	--- lib/lp/bugs/stories/bug-also-affects/10-bug-requestdistrofix.txt	2009-09-18 15:24:30 +0000
115	+++ lib/lp/bugs/stories/bug-also-affects/10-bug-requestdistrofix.txt	2011-06-08 04:03:29 +0000
116	@@ -177,23 +177,3 @@
117	     <...
118	     ...>pmount (Debian)</a>...
119	     ...
120	-
121	-If we enter a source package which doesn't exist, we get an error
122	-message saying so.
123	-
124	-    >>> browser.getLink(url='+distrotask').click()
125	-    >>> browser.getControl('Distribution').value = ['ubuntu']
126	-    >>> browser.getControl('Source Package Name').value = 'no-such-package'
127	-    >>> browser.getControl('Continue').click()
128	-    >>> browser.url
129	-    'http://bugs.launchpad.dev/debian/+source/pmount/+bug/1/+distrotask'
130	-    >>> print get_feedback_messages(browser.contents)
131	-    [u'There is 1 error.',
132	-     u'There is no package in Ubuntu named "no-such-package". If it should
133	-       be here, report this as a bug.']
134	-
135	-The link in the error page links to the Launchpad filebug page.
136	-
137	-    >>> browser.getLink('report this as a bug').click()
138	-    >>> print browser.title
139	-    Report a bug about Launchpad...
140	
141	=== modified file 'lib/lp/registry/model/distribution.py'
142	--- lib/lp/registry/model/distribution.py	2011-06-07 03:30:25 +0000
143	+++ lib/lp/registry/model/distribution.py	2011-06-08 04:03:29 +0000
144	@@ -1835,10 +1835,7 @@
145	             BinaryPackagePublishingHistory.status ==
146	                 PackagePublishingStatus.PUBLISHED).config(limit=1)
147	 
148	-        # XXX 2009-02-19 Julian
149	-        # Storm is not very useful for bool checking on the results,
150	-        # see: https://bugs.launchpad.net/soyuz/+bug/246200
151	-        return results.any() != None
152	+        return not results.is_empty()
153	 
154	     def sharesTranslationsWithOtherSide(self, person, language,
155	                                         sourcepackage=None,

Thanks for these additional cleanups.  Thoughtful and helpful.  I hate doctests.

Jeroen

review: Approve

Launchpad itself

Merge lp:~stevenk/launchpad/say-no-bpns-alsoaffects into lp:launchpad

Commit message

Description of the change

Preview Diff

Subscribers

 === modified file 'lib/lp/bugs/browser/bugalsoaffects.py'
 --- lib/lp/bugs/browser/bugalsoaffects.py	2011-05-27 21:12:25 +0000
 +++ lib/lp/bugs/browser/bugalsoaffects.py	2011-06-08 07:01:23 +0000
@@ -418,16 +418,17 @@
              self.widgets['sourcepackagename'].name)
          if sourcepackagename is None and entered_package:
              # The entered package doesn't exist.
--            filebug_url = "%s/+filebug" % canonical_url(
--                getUtility(ILaunchpadCelebrities).launchpad)
--            self.setFieldError(
--                'sourcepackagename',
--                structured(
--                'There is no package in %s named "%s". If it should'
--                ' be here, <a href="%s">report this as a bug</a>.',
--                distribution.displayname,
--                entered_package,
--                filebug_url))
++            if distribution.has_published_binaries:
++                binary_tracking = ''
++            else:
++                binary_tracking = structured(
++                    ' Launchpad does not track binary package names '
++                    'in %s.', distribution.displayname)
++            error = structured(
++                'There is no package in %s named "%s".%s',
++                distribution.displayname, entered_package,
++                binary_tracking)
++            self.setFieldError('sourcepackagename', error)
          else:
              try:
                  validate_new_distrotask(
 === added file 'lib/lp/bugs/browser/tests/test_bugalsoaffects.py'
 --- lib/lp/bugs/browser/tests/test_bugalsoaffects.py	1970-01-01 00:00:00 +0000
 +++ lib/lp/bugs/browser/tests/test_bugalsoaffects.py	2011-06-08 07:01:23 +0000
@@ -0,0 +1,76 @@
++# Copyright 2011 Canonical Ltd.  This software is licensed under the
++# GNU Affero General Public License version 3 (see the file LICENSE).
++
++__metaclass__ = type
++
++from zope.security.proxy import removeSecurityProxy
++
++from canonical.launchpad.testing.pages import get_feedback_messages
++from canonical.launchpad.webapp import canonical_url
++from canonical.testing.layers import DatabaseFunctionalLayer
++from lp.soyuz.enums import PackagePublishingStatus
++from lp.testing import (
++    person_logged_in,
++    TestCaseWithFactory,
++    )
++
++
++class TestBugAlsoAffectsDistribution(TestCaseWithFactory):
++
++    layer = DatabaseFunctionalLayer
++
++    def setUp(self):
++        super(TestBugAlsoAffectsDistribution, self).setUp()
++        self.distribution = self.factory.makeDistribution()
++        removeSecurityProxy(self.distribution).official_malone = True
++
++    def test_bug_alsoaffects_spn_exists(self):
++        # If the source package name exists, there is no error.
++        bug = self.factory.makeBug()
++        spn = self.factory.makeSourcePackageName()
++        browser = self.getUserBrowser()
++        browser.open(canonical_url(bug))
++        browser.getLink(url='+distrotask').click()
++        browser.getControl('Distribution').value = [self.distribution.name]
++        browser.getControl('Source Package Name').value = spn.name
++        browser.getControl('Continue').click()
++        self.assertEqual([], get_feedback_messages(browser.contents))
++
++    def test_bug_alsoaffects_spn_not_exists_with_published_binaries(self):
++        # When the distribution has published binaries, we search both
++        # source and binary package names.
++        bug = self.factory.makeBug()
++        distroseries = self.factory.makeDistroSeries(
++            distribution=self.distribution)
++        das = self.factory.makeDistroArchSeries(distroseries=distroseries)
++        bpph = self.factory.makeBinaryPackagePublishingHistory(
++            distroarchseries=das, status=PackagePublishingStatus.PUBLISHED)
++        self.assertTrue(self.distribution.has_published_binaries)
++        browser = self.getUserBrowser()
++        browser.open(canonical_url(bug))
++        browser.getLink(url='+distrotask').click()
++        browser.getControl('Distribution').value = [self.distribution.name]
++        browser.getControl('Source Package Name').value = 'does-not-exist'
++        browser.getControl('Continue').click()
++        expected = [
++            u'There is 1 error.',
++            u'There is no package in %s named "does-not-exist".' % (
++                self.distribution.displayname)]
++        self.assertEqual(expected, get_feedback_messages(browser.contents))
++
++    def test_bug_alsoaffects_spn_not_exists_with_no_binaries(self):
++        # When the distribution has no binary packages published, we can't.
++        bug = self.factory.makeBug()
++        browser = self.getUserBrowser()
++        browser.open(canonical_url(bug))
++        browser.getLink(url='+distrotask').click()
++        browser.getControl('Distribution').value = [self.distribution.name]
++        browser.getControl('Source Package Name').value = 'does-not-exist'
++        browser.getControl('Continue').click()
++        expected = [
++            u'There is 1 error.',
++            u'There is no package in %s named "does-not-exist". Launchpad '
++            'does not track binary package names in %s.' % (
++                self.distribution.displayname,
++                self.distribution.displayname)]
++        self.assertEqual(expected, get_feedback_messages(browser.contents))
 === modified file 'lib/lp/bugs/stories/bug-also-affects/10-bug-requestdistrofix.txt'
 --- lib/lp/bugs/stories/bug-also-affects/10-bug-requestdistrofix.txt	2009-09-18 15:24:30 +0000
 +++ lib/lp/bugs/stories/bug-also-affects/10-bug-requestdistrofix.txt	2011-06-08 07:01:23 +0000
@@ -177,23 +177,3 @@
      <...
      ...>pmount (Debian)</a>...
      ...
--
--If we enter a source package which doesn't exist, we get an error
--message saying so.
--
--    >>> browser.getLink(url='+distrotask').click()
--    >>> browser.getControl('Distribution').value = ['ubuntu']
--    >>> browser.getControl('Source Package Name').value = 'no-such-package'
--    >>> browser.getControl('Continue').click()
--    >>> browser.url
--    'http://bugs.launchpad.dev/debian/+source/pmount/+bug/1/+distrotask'
--    >>> print get_feedback_messages(browser.contents)
--    [u'There is 1 error.',
--     u'There is no package in Ubuntu named "no-such-package". If it should
--       be here, report this as a bug.']
--
--The link in the error page links to the Launchpad filebug page.
--
--    >>> browser.getLink('report this as a bug').click()
--    >>> print browser.title
--    Report a bug about Launchpad...
 === modified file 'lib/lp/registry/model/distribution.py'
 --- lib/lp/registry/model/distribution.py	2011-06-07 03:30:25 +0000
 +++ lib/lp/registry/model/distribution.py	2011-06-08 07:01:23 +0000
@@ -1835,10 +1835,7 @@
              BinaryPackagePublishingHistory.status ==
                  PackagePublishingStatus.PUBLISHED).config(limit=1)
--        # XXX 2009-02-19 Julian
--        # Storm is not very useful for bool checking on the results,
--        # see: https://bugs.launchpad.net/soyuz/+bug/246200
--        return results.any() != None
++        return not results.is_empty()
      def sharesTranslationsWithOtherSide(self, person, language,
                                          sourcepackage=None,