launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/bugtaskeditview-spn-dsp-vocab into lp:launchpad with lp:~cjwatson/launchpad/fix-dsp-vocab-picker as a prerequisite.

Commit message:
Convert BugTaskEditView to use the DistributionSourcePackage picker if the appropriate feature flag is set.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #42298 in Launchpad itself: "package picker lists unpublished (invalid) packages"
  https://bugs.launchpad.net/launchpad/+bug/42298

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/bugtaskeditview-spn-dsp-vocab/+merge/300975

Convert BugTaskEditView to use the DistributionSourcePackage picker if the appropriate feature flag is set.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/bugtaskeditview-spn-dsp-vocab into lp:launchpad.

=== modified file 'lib/lp/bugs/browser/bugtask.py'
--- lib/lp/bugs/browser/bugtask.py	2016-01-26 15:47:37 +0000
+++ lib/lp/bugs/browser/bugtask.py	2016-07-23 10:37:37 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2016 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """IBugTask-related browser views."""
@@ -1073,11 +1073,30 @@
         return self.context.userHasBugSupervisorPrivileges(self.user)
 
 
+class IBugTaskEditForm(IBugTask):
+
+    sourcepackagename = copy_field(
+        IBugTask['sourcepackagename'],
+        vocabularyName='DistributionSourcePackage')
+
+
 class BugTaskEditView(LaunchpadEditFormView, BugTaskBugWatchMixin,
                       BugTaskPrivilegeMixin):
     """The view class used for the task +editstatus page."""
 
-    schema = IBugTask
+    @property
+    def schema(self):
+        """See `LaunchpadFormView`."""
+        if bool(getFeatureFlag('disclosure.dsp_picker.enabled')):
+            return IBugTaskEditForm
+        else:
+            return IBugTask
+
+    @property
+    def adapters(self):
+        """See `LaunchpadFormView`."""
+        return {IBugTaskEditForm: self.context}
+
     milestone_source = None
     user_is_subscribed = None
     edit_form = ViewPageTemplateFile('../templates/bugtask-edit-form.pt')
@@ -1324,8 +1343,12 @@
     def validate(self, data):
         if self.show_sourcepackagename_widget and 'sourcepackagename' in data:
             data['target'] = self.context.distroseries
-            spn = data.get('sourcepackagename')
-            if spn:
+            spn_or_dsp = data.get('sourcepackagename')
+            if spn_or_dsp:
+                if IDistributionSourcePackage.providedBy(spn_or_dsp):
+                    spn = spn_or_dsp.sourcepackagename
+                else:
+                    spn = spn_or_dsp
                 data['target'] = data['target'].getSourcePackage(spn)
             del data['sourcepackagename']
             error_field = 'sourcepackagename'

=== modified file 'lib/lp/bugs/browser/tests/test_bugtask.py'
--- lib/lp/bugs/browser/tests/test_bugtask.py	2016-01-26 15:47:37 +0000
+++ lib/lp/bugs/browser/tests/test_bugtask.py	2016-07-23 10:37:37 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2014 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2016 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -17,6 +17,10 @@
 from pytz import UTC
 import simplejson
 import soupmatchers
+from testscenarios import (
+    load_tests_apply_scenarios,
+    WithScenarios,
+    )
 from testtools.matchers import (
     LessThan,
     Not,
@@ -27,6 +31,7 @@
     getUtility,
     )
 from zope.event import notify
+from zope.formlib.interfaces import ConversionError
 from zope.interface import providedBy
 from zope.security.proxy import removeSecurityProxy
 
@@ -1316,11 +1321,24 @@
             view.form_fields['assignee'].field.vocabularyName)
 
 
-class TestBugTaskEditView(TestCaseWithFactory):
+class TestBugTaskEditView(WithScenarios, TestCaseWithFactory):
     """Test the bug task edit form."""
 
     layer = DatabaseFunctionalLayer
 
+    scenarios = [
+        ("spn_picker", {"features": {}, "allow_binarypackagename": True}),
+        ("dsp_picker", {
+            "features": {u"disclosure.dsp_picker.enabled": u"on"},
+            "allow_binarypackagename": False,
+            }),
+        ]
+
+    def setUp(self):
+        super(TestBugTaskEditView, self).setUp()
+        if self.features:
+            self.useFixture(FeatureFixture(self.features))
+
     def test_retarget_already_exists_error(self):
         user = self.factory.makePerson()
         login_person(user)
@@ -1448,6 +1466,8 @@
     def test_retarget_sourcepackage_to_binary_name(self):
         # The sourcepackagename of a SourcePackage task can be changed
         # to a binarypackagename, which gets mapped back to the source.
+        # (This is not allowed for the DistributionSourcePackage picker,
+        # where the vocabulary takes care of doing an appropriate mapping.)
         ds = self.factory.makeDistroSeries()
         das = self.factory.makeDistroArchSeries(distroseries=ds)
         sp1 = self.factory.makeSourcePackage(distroseries=ds, publish=True)
@@ -1462,15 +1482,23 @@
 
         view = self.createNameChangingViewForSourcePackageTask(
             bug_task, bpr.binarypackagename.name)
-        self.assertEqual([], view.errors)
-        self.assertEqual(sp2, bug_task.target)
-        notifications = view.request.response.notifications
-        self.assertEqual(1, len(notifications))
-        expected = html_escape(
-            "'%s' is a binary package. This bug has been assigned to its "
-            "source package '%s' instead."
-            % (bpr.binarypackagename.name, spn.name))
-        self.assertTrue(notifications.pop().message.startswith(expected))
+        if self.allow_binarypackagename:
+            self.assertEqual([], view.errors)
+            self.assertEqual(sp2, bug_task.target)
+            notifications = view.request.response.notifications
+            self.assertEqual(1, len(notifications))
+            expected = html_escape(
+                "'%s' is a binary package. This bug has been assigned to its "
+                "source package '%s' instead."
+                % (bpr.binarypackagename.name, spn.name))
+            self.assertTrue(notifications.pop().message.startswith(expected))
+        else:
+            self.assertEqual(1, len(view.errors))
+            self.assertIsInstance(view.errors[0], ConversionError)
+            self.assertEqual(
+                "Launchpad doesn't know of any source package named "
+                "'binarypackage-100413' in Distribution-100372.",
+                view.errors[0].error_name)
 
     def test_retarget_sourcepackage_to_distroseries(self):
         # A SourcePackage task can be changed to a DistroSeries one.
@@ -2536,3 +2564,6 @@
             bug.date_last_message = datetime(2001, 1, 1, tzinfo=UTC)
             self.assertEqual(
                 'on 2001-01-01', item.model['last_updated'])
+
+
+load_tests = load_tests_apply_scenarios

=== modified file 'lib/lp/bugs/browser/widgets/bugtask.py'
--- lib/lp/bugs/browser/widgets/bugtask.py	2016-07-23 10:37:37 +0000
+++ lib/lp/bugs/browser/widgets/bugtask.py	2016-07-23 10:37:37 +0000
@@ -520,7 +520,7 @@
             try:
                 self.context.vocabulary.setDistribution(distribution)
                 return self.context.vocabulary.getTermByToken(input).value
-            except NotFoundError:
+            except LookupError:
                 raise ConversionError(
                     "Launchpad doesn't know of any source package named"
                     " '%s' in %s." % (input, distribution.displayname))