Merge lp:~sinzui/launchpad/bug-contacts-2 into lp:launchpad

Proposed by Curtis Hovey
Status: Merged
Approved by: Edwin Grubbs
Approved revision: no longer in the source branch.
Merged at revision: 10950
Proposed branch: lp:~sinzui/launchpad/bug-contacts-2
Merge into: lp:launchpad
Diff against target: 295 lines (+185/-8)
4 files modified
lib/lp/bugs/browser/bugtarget.py (+46/-3)
lib/lp/bugs/browser/configure.zcml (+4/-0)
lib/lp/bugs/browser/tests/test_bugtarget_configure.py (+123/-0)
lib/lp/bugs/browser/tests/test_views.py (+12/-5)
To merge this branch: bzr merge lp:~sinzui/launchpad/bug-contacts-2
Reviewer Review Type Date Requested Status
Edwin Grubbs (community) code Approve
Jelmer Vernooij (community) code* Abstain
Review via email: mp+26093@code.launchpad.net

Description of the change

This is my branch to add bug roles to +configure-bugs.

    lp:~sinzui/launchpad/bug-contacts-2
    Diff size: 295
    Launchpad bug: https://bugs.launchpad.net/bugs/233971
    Test command: ./bin/test -vv \
        -t test_bugtarget_configuration -t lp.bugs.browser.*txt
    Pre-implementation: bac, edwin
    Target release: 10.05

Add bug roles to +configure-bugs
---------------------------------

Add the bug supervisor and security contact roles to +configure-bugs

Rules
-----

    * Compose an interface that provides all the bug configuration fields.
    * Reuse the security contact and bug supervisor validation and
      transition rules.

QA
--

    * Visit https://edge.launchpad.net/launchpad-registry/
    * Chose configure bugs
    * Verify you can set the bug supervisor and security contact roles.

Lint
----

Linting changed files:
  lib/lp/bugs/browser/bugtarget.py
  lib/lp/bugs/browser/configure.zcml
  lib/lp/bugs/browser/tests/test_bugtarget_configure.py
  lib/lp/bugs/browser/tests/test_views.py

Test
----

    * lib/lp/bugs/browser/tests/test_bugtarget_configure.py
      * Added a unittest to verify the form additions.
      * The enable_bug_expiration tests are some what redundant with
        tests in stories. I expect these will be moved or expanded upon
        in a subsequent branch that moves the field into the bugtracker
        widget.

Implementation
--------------

    * lib/lp/bugs/browser/bugtarget.py
      * Composed an interface to represent the form schema.
      * Created an adapter to allow IProduct work with the schema.
      * Extended the existing form to use the composed schema. The form
        also reuses bug supervisor and security contact validation and
        change rules.
    * lib/lp/bugs/browser/configure.zcml
      * Registered the adapter for IProduct.
    * lib/lp/bugs/browser/tests/test_views.py
      * Change the default test layer to DatabaseFunctionalLayer because
        I wanted my tests to run faster.

To post a comment you must log in.
Revision history for this message
Jelmer Vernooij (jelmer) wrote :

This seems right to me, tests all work and style-wise it's fine but I don't think I'm familiar enough with the code to be able to approve this. Michael (my review mentor) is also out at the moment so he can't review me. Can somebody else have a look?

review: Abstain (code*)
Revision history for this message
Edwin Grubbs (edwin-grubbs) wrote :

>=== modified file 'lib/lp/bugs/browser/bugtarget.py'
>--- lib/lp/bugs/browser/bugtarget.py 2010-05-18 22:36:20 +0000
>+++ lib/lp/bugs/browser/bugtarget.py 2010-06-03 22:13:41 +0000
>@@ -99,11 +106,35 @@
> [('yes', True), ('no', False)])
>
>
>-class ProductConfigureBugTrackerView(ProductConfigureBase):
>+class IProductBugConfiguration(Interface):
>+ """A composite schema for editing bug app configuration."""
>+
>+ bug_supervisor = copy_field(
>+ IHasBugSupervisor['bug_supervisor'], readonly=False)
>+ security_contact = copy_field(IHasSecurityContact['security_contact'])
>+ official_malone = copy_field(ILaunchpadUsage['official_malone'])
>+ enable_bug_expiration = copy_field(
>+ ILaunchpadUsage['enable_bug_expiration'])
>+ bugtracker = copy_field(IProduct['bugtracker'])
>+ remote_product = copy_field(IProduct['remote_product'])
>+ bug_reporting_guidelines = copy_field(
>+ IBugTarget['bug_reporting_guidelines'])
>+
>+
>+def product_to_productbugconfiguration(product):
>+ """Adapts an `IProduct` into an `IProductBugConfiguration`."""
>+ alsoProvides(IProduct, IProductBugConfiguration)
>+ return product

Hi Curtis,

This branch looks good. merge-conditional on passing the product object without the security wrapper as the first argument for alsoProvides().

-Edwin

Revision history for this message
Edwin Grubbs (edwin-grubbs) :
review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/bugs/browser/bugtarget.py'
2--- lib/lp/bugs/browser/bugtarget.py 2010-05-18 22:36:20 +0000
3+++ lib/lp/bugs/browser/bugtarget.py 2010-06-04 14:18:26 +0000
4@@ -14,8 +14,11 @@
5 "FileBugAdvancedView",
6 "FileBugGuidedView",
7 "FileBugViewBase",
8+ "IProductBugConfiguration",
9 "OfficialBugTagsManageView",
10+ "ProductConfigureBugTrackerView",
11 "ProjectFileBugGuidedView",
12+ "product_to_productbugconfiguration",
13 ]
14
15 import cgi
16@@ -33,11 +36,14 @@
17 from zope.app.form.interfaces import InputErrors
18 from zope.component import getUtility
19 from zope import formlib
20-from zope.interface import implements
21+from zope.interface import alsoProvides, implements, Interface
22 from zope.publisher.interfaces import NotFound
23 from zope.publisher.interfaces.browser import IBrowserPublisher
24 from zope.schema import Bool, Choice
25 from zope.schema.vocabulary import SimpleVocabulary
26+from zope.security.proxy import removeSecurityProxy
27+
28+from lazr.restful.interface import copy_field
29
30 from canonical.cachedproperty import cachedproperty
31 from canonical.config import config
32@@ -46,6 +52,8 @@
33 from lp.bugs.interfaces.bug import IBug
34 from lp.bugs.interfaces.bugtask import BugTaskSearchParams
35 from lp.bugs.interfaces.bugtracker import IBugTracker
36+from lp.bugs.interfaces.securitycontact import IHasSecurityContact
37+from lp.bugs.browser.bugrole import BugRoleMixin
38 from canonical.launchpad import _
39 from canonical.launchpad.browser.feeds import (
40 BugFeedLink, BugTargetLatestBugsFeedLink, FeedsMixin)
41@@ -99,11 +107,36 @@
42 [('yes', True), ('no', False)])
43
44
45-class ProductConfigureBugTrackerView(ProductConfigureBase):
46+class IProductBugConfiguration(Interface):
47+ """A composite schema for editing bug app configuration."""
48+
49+ bug_supervisor = copy_field(
50+ IHasBugSupervisor['bug_supervisor'], readonly=False)
51+ security_contact = copy_field(IHasSecurityContact['security_contact'])
52+ official_malone = copy_field(ILaunchpadUsage['official_malone'])
53+ enable_bug_expiration = copy_field(
54+ ILaunchpadUsage['enable_bug_expiration'])
55+ bugtracker = copy_field(IProduct['bugtracker'])
56+ remote_product = copy_field(IProduct['remote_product'])
57+ bug_reporting_guidelines = copy_field(
58+ IBugTarget['bug_reporting_guidelines'])
59+
60+
61+def product_to_productbugconfiguration(product):
62+ """Adapts an `IProduct` into an `IProductBugConfiguration`."""
63+ alsoProvides(
64+ removeSecurityProxy(product), IProductBugConfiguration)
65+ return product
66+
67+
68+class ProductConfigureBugTrackerView(BugRoleMixin, ProductConfigureBase):
69 """View class to configure the bug tracker for a project."""
70
71 label = "Configure bug tracker"
72+ schema = IProductBugConfiguration
73 field_names = [
74+ "bug_supervisor",
75+ "security_contact",
76 "bugtracker",
77 "enable_bug_expiration",
78 "remote_product",
79@@ -113,6 +146,8 @@
80
81 def validate(self, data):
82 """Constrain bug expiration to Launchpad Bugs tracker."""
83+ self.validateBugSupervisor(data)
84+ self.validateSecurityContact(data)
85 # enable_bug_expiration is disabled by JavaScript when bugtracker
86 # is not 'In Launchpad'. The constraint is enforced here in case the
87 # JavaScript fails to activate or run. Note that the bugtracker
88@@ -122,6 +157,15 @@
89 if bugtracker is None or IBugTracker.providedBy(bugtracker):
90 data['enable_bug_expiration'] = False
91
92+ @action("Change", name='change')
93+ def change_action(self, action, data):
94+ # bug_supervisor requires a transition method, so it must be
95+ # handled separately and removed for the updateContextFromData
96+ # to work as expected.
97+ self.changeBugSupervisor(data['bug_supervisor'])
98+ del data['bug_supervisor']
99+ self.updateContextFromData(data)
100+
101
102 class FileBugViewBase(LaunchpadFormView):
103 """Base class for views related to filing a bug."""
104@@ -1337,7 +1381,6 @@
105 orderings.append((targetname.lower(), "targetname"))
106 return orderings
107
108-
109 def batchedPatchTasks(self):
110 """Return a BatchNavigator for bug tasks with patch attachments."""
111 orderby = self.request.get("orderby", "-latest_patch_uploaded")
112
113=== modified file 'lib/lp/bugs/browser/configure.zcml'
114--- lib/lp/bugs/browser/configure.zcml 2010-05-21 12:03:56 +0000
115+++ lib/lp/bugs/browser/configure.zcml 2010-06-04 14:18:26 +0000
116@@ -384,6 +384,10 @@
117 facet="bugs"
118 permission="launchpad.Edit"
119 template="../templates/securitycontact-edit.pt"/>
120+ <adapter
121+ for="lp.registry.interfaces.product.IProduct"
122+ provides=".bugtarget.IProductBugConfiguration"
123+ factory=".bugtarget.product_to_productbugconfiguration" />
124 <browser:page
125 for="lp.registry.interfaces.product.IProduct"
126 facet="bugs"
127
128=== added file 'lib/lp/bugs/browser/tests/test_bugtarget_configure.py'
129--- lib/lp/bugs/browser/tests/test_bugtarget_configure.py 1970-01-01 00:00:00 +0000
130+++ lib/lp/bugs/browser/tests/test_bugtarget_configure.py 2010-06-04 14:18:26 +0000
131@@ -0,0 +1,123 @@
132+# Copyright 2010 Canonical Ltd. This software is licensed under the
133+# GNU Affero General Public License version (see the file LICENSE).
134+
135+"""Unit tests for bug configuration views."""
136+
137+__metaclass__ = type
138+
139+import unittest
140+
141+from lp.testing import login_person, TestCaseWithFactory
142+from lp.testing.views import create_initialized_view
143+from canonical.testing import DatabaseFunctionalLayer
144+
145+
146+class TestProductBugConfigurationView(TestCaseWithFactory):
147+
148+ layer = DatabaseFunctionalLayer
149+
150+ def setUp(self):
151+ super(TestProductBugConfigurationView, self).setUp()
152+ self.owner = self.factory.makePerson(name='boing-owner')
153+ self.product = self.factory.makeProduct(
154+ name='boing', owner=self.owner)
155+ login_person(self.owner)
156+
157+ def _makeForm(self):
158+ return {
159+ 'field.bug_supervisor': 'boing-owner',
160+ 'field.security_contact': 'boing-owner',
161+ 'field.bugtracker': 'malone',
162+ 'field.enable_bug_expiration': 'on',
163+ 'field.remote_product': 'sf-boing',
164+ 'field.bug_reporting_guidelines': 'guidelines',
165+ 'field.actions.change': 'Change',
166+ }
167+
168+ def test_view_attributes(self):
169+ view = create_initialized_view(
170+ self.product, name='+configure-bugtracker')
171+ label = 'Configure bug tracker'
172+ self.assertEqual(label, view.label)
173+ fields = [
174+ 'bug_supervisor', 'security_contact', 'bugtracker',
175+ 'enable_bug_expiration', 'remote_product',
176+ 'bug_reporting_guidelines']
177+ self.assertEqual(fields, view.field_names)
178+ self.assertEqual('http://launchpad.dev/boing', view.next_url)
179+ self.assertEqual('http://launchpad.dev/boing', view.cancel_url)
180+
181+ def test_all_data_change(self):
182+ # Verify that the composed interface supports all fields.
183+ # This is a sanity check. The bug_supervisor, security_contact and
184+ # bugtracker field are rigorously tested in their respective tests.
185+ form = self._makeForm()
186+ view = create_initialized_view(
187+ self.product, name='+configure-bugtracker', form=form)
188+ self.assertEqual([], view.errors)
189+ self.assertEqual(self.owner, self.product.bug_supervisor)
190+ self.assertEqual(self.owner, self.product.security_contact)
191+ self.assertTrue(self.product.official_malone)
192+ self.assertTrue(self.product.enable_bug_expiration)
193+ self.assertEqual('sf-boing', self.product.remote_product)
194+ self.assertEqual('guidelines', self.product.bug_reporting_guidelines)
195+
196+ def test_bug_supervisor_invalid(self):
197+ # Verify that invalid bug_supervisor states are reported.
198+ # This is a sanity check. The bug_supervisor is rigorously tested
199+ # in its own test.
200+ other_person = self.factory.makePerson()
201+ form = self._makeForm()
202+ form['field.bug_supervisor'] = other_person.name
203+ view = create_initialized_view(
204+ self.product, name='+configure-bugtracker', form=form)
205+ self.assertEqual(1, len(view.errors))
206+
207+ def test_security_contact_invalid(self):
208+ # Verify that invalid security_contact states are reported.
209+ # This is a sanity check. The security_contact is rigorously tested
210+ # in its own test.
211+ other_person = self.factory.makePerson()
212+ form = self._makeForm()
213+ form['field.security_contact'] = other_person.name
214+ view = create_initialized_view(
215+ self.product, name='+configure-bugtracker', form=form)
216+ self.assertEqual(1, len(view.errors))
217+
218+ def test_enable_bug_expiration_with_launchpad(self):
219+ # Verify that enable_bug_expiration can be True bugs are tracked
220+ # in Launchpad.
221+ form = self._makeForm()
222+ form['field.enable_bug_expiration'] = 'on'
223+ form['field.bugtracker'] = 'malone'
224+ view = create_initialized_view(
225+ self.product, name='+configure-bugtracker', form=form)
226+ self.assertEqual([], view.errors)
227+ self.assertTrue(self.product.enable_bug_expiration)
228+
229+ def test_enable_bug_expiration_with_external_bug_tracker(self):
230+ # Verify that enable_bug_expiration is forced to False when the
231+ # bug tracker is external.
232+ form = self._makeForm()
233+ form['field.enable_bug_expiration'] = 'on'
234+ form['field.bugtracker'] = 'external'
235+ form['field.bugtracker.bugtracker'] = '3'
236+ view = create_initialized_view(
237+ self.product, name='+configure-bugtracker', form=form)
238+ self.assertEqual([], view.errors)
239+ self.assertFalse(self.product.enable_bug_expiration)
240+
241+ def test_enable_bug_expiration_with_no_bug_tracker(self):
242+ # Verify that enable_bug_expiration is forced to False when the
243+ # bug tracker is unknown.
244+ form = self._makeForm()
245+ form['field.enable_bug_expiration'] = 'on'
246+ form['field.bugtracker'] = 'project'
247+ view = create_initialized_view(
248+ self.product, name='+configure-bugtracker', form=form)
249+ self.assertEqual([], view.errors)
250+ self.assertFalse(self.product.enable_bug_expiration)
251+
252+
253+def test_suite():
254+ return unittest.TestLoader().loadTestsFromName(__name__)
255
256=== modified file 'lib/lp/bugs/browser/tests/test_views.py'
257--- lib/lp/bugs/browser/tests/test_views.py 2009-06-25 00:40:31 +0000
258+++ lib/lp/bugs/browser/tests/test_views.py 2010-06-04 14:18:26 +0000
259@@ -11,12 +11,20 @@
260
261 from canonical.launchpad.testing.systemdocs import (
262 LayeredDocFileSuite, setUp, tearDown)
263-from canonical.testing import LaunchpadFunctionalLayer
264+from canonical.testing import (
265+ DatabaseFunctionalLayer, LaunchpadFunctionalLayer)
266
267
268 here = os.path.dirname(os.path.realpath(__file__))
269
270
271+special_test_layer = {
272+ 'bug-views.txt': LaunchpadFunctionalLayer,
273+ 'bugtarget-filebug-views.txt': LaunchpadFunctionalLayer,
274+ 'bugtask-target-link-titles.txt': LaunchpadFunctionalLayer,
275+ }
276+
277+
278 def test_suite():
279 suite = unittest.TestSuite()
280 testsdir = os.path.abspath(here)
281@@ -29,11 +37,10 @@
282 filenames.sort()
283 for filename in filenames:
284 path = filename
285+ layer = special_test_layer.get(path, DatabaseFunctionalLayer)
286 one_test = LayeredDocFileSuite(
287- path, setUp=setUp, tearDown=tearDown,
288- layer=LaunchpadFunctionalLayer,
289- stdout_logging_level=logging.WARNING
290- )
291+ path, setUp=setUp, tearDown=tearDown, layer=layer,
292+ stdout_logging_level=logging.WARNING)
293 suite.addTest(one_test)
294
295 return suite