launchpad-reviewers team mailing list archive

[Ian Booth <ian.booth@xxxxxxxxxxxxx>]

Ian Booth has proposed merging lp:~wallyworld/launchpad/one-bugtask-for-private-bugs-878605 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #878605 in Launchpad itself: "Do not allow private bugs to have more than one bug task"
  https://bugs.launchpad.net/launchpad/+bug/878605

For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/one-bugtask-for-private-bugs-878605/+merge/79918

== Implementation ==

Private bugs can only have one bug task. So we add a canAddTask() method to Bug and if a new bug task cannot be added:

1. Do not show Also Affects links on the bug index page (includes changing privacy via portlet)
2. Raise a mail processing error with a suitable message if an "bug affects" email command is sent
3. Do not allow a bug to be nominated
4. Raise an CannotAddBugTask exception in the addTask method

== Tests ==

Add new tests:

test_bugs_web_service.TestErrorHandling:
  - test_add_bugtask_to_private_bug_gives_bad_request
test_bug_views.TestAlsoAffectsLinks:
  - test_no_also_affects_links_when_cannot_add_bugtask
  - test_also_affects_links_when_can_add_bugtask
test_commands.AffectsEmailCommandTestCase:
  - test_execute_bug_cannot_add_task
bug.txt:
  - add to existing doc tests for addTask

Update test:
test_bug_views.TestBugSecrecyViews:
  - test_secrecy_view_ajax_render

== Lint ==

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/bugs/configure.zcml
  lib/lp/bugs/browser/tests/test_bug_views.py
  lib/lp/bugs/doc/bug.txt
  lib/lp/bugs/interfaces/bug.py
  lib/lp/bugs/javascript/bugtask_index.js
  lib/lp/bugs/mail/commands.py
  lib/lp/bugs/mail/errortemplates/cannot-add-task.txt
  lib/lp/bugs/mail/tests/test_commands.py
  lib/lp/bugs/model/bug.py
  lib/lp/bugs/templates/bugtasks-and-nominations-table.pt
  lib/lp/bugs/tests/test_bugs_webservice.py

./lib/lp/bugs/doc/bug.txt
     739: want exceeds 78 characters.
./lib/lp/bugs/mail/errortemplates/cannot-add-task.txt
       1: Line exceeds 78 characters.
./lib/lp/bugs/mail/tests/test_commands.py
     285: local variable 'product' is assigned to but never used
-- 
https://code.launchpad.net/~wallyworld/launchpad/one-bugtask-for-private-bugs-878605/+merge/79918
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/one-bugtask-for-private-bugs-878605 into lp:launchpad.

=== modified file 'lib/lp/bugs/browser/bug.py'
--- lib/lp/bugs/browser/bug.py	2011-09-27 13:10:47 +0000
+++ lib/lp/bugs/browser/bug.py	2011-10-20 07:58:53 +0000
@@ -865,11 +865,11 @@
             self._handlePrivacyChanged(user_will_be_subscribed)
         if self.request.is_ajax:
             if private_changed or security_related_changed:
-                return self._getSubscriptionDetails()
+                return self._getResultsData()
             else:
                 return ''
 
-    def _getSubscriptionDetails(self):
+    def _getResultsData(self):
         cache = dict()
         # The subscription details for the current user.
         self.extractBugSubscriptionDetails(self.user, self.context.bug, cache)
@@ -884,6 +884,7 @@
             bug, self.request)
         subscription_data = subscribers_portlet.subscriber_data
         result_data = dict(
+            can_add_bugtask=bug.canAddTask(),
             cache_data=cache,
             subscription_data=subscription_data)
         self.request.response.setHeader('content-type', 'application/json')

=== modified file 'lib/lp/bugs/browser/tests/test_bug_views.py'
--- lib/lp/bugs/browser/tests/test_bug_views.py	2011-09-27 20:47:28 +0000
+++ lib/lp/bugs/browser/tests/test_bug_views.py	2011-10-20 07:58:53 +0000
@@ -49,6 +49,32 @@
         self.assertTrue('href' not in dupe_warning)
 
 
+class TestAlsoAffectsLinks(BrowserTestCase):
+    # Tests the visibility of the Also Affects... links on the bug index view.
+    layer = DatabaseFunctionalLayer
+
+    def test_no_also_affects_links_when_cannot_add_bugtask(self):
+        # The also affects links are not visible if a bug task cannot be
+        # added.
+        owner = self.factory.makePerson()
+        bug = self.factory.makeBug(private=True, owner=owner)
+        with person_logged_in(owner):
+            url = canonical_url(bug, rootsite="bugs")
+            browser = self.getUserBrowser(url, user=owner)
+            also_affects = find_tag_by_id(
+                browser.contents, 'also-affects-links')
+            self.assertEqual('display: none', also_affects['style'])
+
+    def test_also_affects_links_when_can_add_bugtask(self):
+        # The also affects links are visible if a bug task can be added.
+        bug = self.factory.makeBug()
+        url = canonical_url(bug, rootsite="bugs")
+        browser = self.getUserBrowser(url)
+        also_affects = find_tag_by_id(
+            browser.contents, 'also-affects-links')
+        self.assertEqual('display: block', also_affects['style'])
+
+
 class TestEmailObfuscated(BrowserTestCase):
     """Test for obfuscated emails on bug pages."""
 
@@ -299,6 +325,10 @@
         view = self.createInitializedSecrecyView(person, bug, request)
         result_data = simplejson.loads(view.render())
 
+        with person_logged_in(person):
+            self.assertTrue(result_data.has_key('can_add_bugtask'))
+            self.assertEqual(bug.canAddTask(), result_data['can_add_bugtask'])
+
         cache_data = result_data['cache_data']
         self.assertFalse(cache_data['other_subscription_notifications'])
         subscription_data = cache_data['subscription']

=== modified file 'lib/lp/bugs/configure.zcml'
--- lib/lp/bugs/configure.zcml	2011-10-19 08:32:33 +0000
+++ lib/lp/bugs/configure.zcml	2011-10-20 07:58:53 +0000
@@ -803,6 +803,7 @@
                     getAlsoNotifiedSubscribers
                     getBugWatch
                     canBeNominatedFor
+                    canAddTask
                     getNominationFor
                     getNominations
                     date_last_message

=== modified file 'lib/lp/bugs/doc/bug.txt'
--- lib/lp/bugs/doc/bug.txt	2011-09-26 06:30:07 +0000
+++ lib/lp/bugs/doc/bug.txt	2011-10-20 07:58:53 +0000
@@ -726,6 +726,20 @@
     >>> print firefox_bug.default_bugtask.bugtargetdisplayname
     Mozilla Firefox
 
+Private bugs can only have one bug task.
+
+    >>> firefox_bug.setPrivate(True, current_user())
+    True
+    >>> firefox_bug.canAddTask()
+    False
+    >>> target = getUtility(IProductSet).getByName('tomcat')
+    >>> firefox_bug.addTask(owner=foobar, target=target)
+    Traceback (most recent call last):
+    ...
+    CannotAddBugTask: Private bugs cannot be marked as affecting more than one target.
+    >>> firefox_bug.setPrivate(False, current_user())
+    True
+
 Changing bug visibility.
 
     >>> bug_before_modification = Snapshot(firefox_bug, providing=IBug)

=== modified file 'lib/lp/bugs/interfaces/bug.py'
--- lib/lp/bugs/interfaces/bug.py	2011-09-23 13:14:36 +0000
+++ lib/lp/bugs/interfaces/bug.py	2011-10-20 07:58:53 +0000
@@ -8,6 +8,7 @@
 __metaclass__ = type
 
 __all__ = [
+    'CannotAddBugTask',
     'CreateBugParams',
     'CreatedBugWithNoBugTasksError',
     'IBug',
@@ -21,12 +22,15 @@
     'IProjectGroupBugAddForm',
     ]
 
+import httplib
+
 from lazr.enum import DBEnumeratedType
 
 from lazr.lifecycle.snapshot import doNotSnapshot
 from lazr.restful.declarations import (
     accessor_for,
     call_with,
+    error_status,
     export_as_webservice_entry,
     export_factory_operation,
     export_operation_as,
@@ -189,6 +193,11 @@
     """Raised when a bug is created with no bug tasks."""
 
 
+@error_status(httplib.BAD_REQUEST)
+class CannotAddBugTask(Exception):
+    """Raised when a new bug task cannot be added to a bug."""
+
+
 def optional_message_subject_field():
     """A modified message subject field allowing None as a value."""
     subject_field = copy_field(IMessage['subject'])
@@ -635,7 +644,13 @@
     @operation_parameters(target=copy_field(IBugTask['target']))
     @export_factory_operation(IBugTask, [])
     def addTask(owner, target):
-        """Create a new bug task on this bug."""
+        """Create a new bug task on this bug.
+
+        :raises CannotAddBugTask: if the bug task cannot be added to the bug.
+        """
+
+    def canAddTask():
+        """Can a new bug task be added to this bug?"""
 
     def hasBranch(branch):
         """Is this branch linked to this bug?"""

=== modified file 'lib/lp/bugs/javascript/bugtask_index.js'
--- lib/lp/bugs/javascript/bugtask_index.js	2011-10-11 11:58:01 +0000
+++ lib/lp/bugs/javascript/bugtask_index.js	2011-10-20 07:58:53 +0000
@@ -394,6 +394,13 @@
                     }
                     var ns = Y.lp.bugs.bugtask_index.portlets.subscription;
                     ns.update_subscription_status();
+
+                    var affects_links =  Y.one('#also-affects-links');
+                    if (result_data.can_add_bugtask) {
+                        affects_links.setStyle('display', 'block');
+                    } else {
+                        affects_links.setStyle('display', 'none');
+                    }
                 }
                 privacy_spinner.setStyle('display', 'none');
                 privacy_link.setStyle('display', 'inline');

=== modified file 'lib/lp/bugs/mail/commands.py'
--- lib/lp/bugs/mail/commands.py	2011-09-21 13:27:17 +0000
+++ lib/lp/bugs/mail/commands.py	2011-10-20 07:58:53 +0000
@@ -576,6 +576,14 @@
                     error_templates=error_templates),
                 stop_processing=True)
 
+        if not bug.canAddTask():
+            raise EmailProcessingError(
+                get_error_message(
+                    'cannot-add-task.txt',
+                    error_templates=error_templates,
+                    bug_id=bug.id),
+                stop_processing=True)
+
         string_args = list(self.string_args)
         try:
             path = string_args.pop(0)

=== added file 'lib/lp/bugs/mail/errortemplates/cannot-add-task.txt'
--- lib/lp/bugs/mail/errortemplates/cannot-add-task.txt	1970-01-01 00:00:00 +0000
+++ lib/lp/bugs/mail/errortemplates/cannot-add-task.txt	2011-10-20 07:58:53 +0000
@@ -0,0 +1,1 @@
+Bug %(bug_id)s cannot be marked as affecting more than one target because it is private.

=== modified file 'lib/lp/bugs/mail/tests/test_commands.py'
--- lib/lp/bugs/mail/tests/test_commands.py	2011-09-15 18:45:21 +0000
+++ lib/lp/bugs/mail/tests/test_commands.py	2011-10-20 07:58:53 +0000
@@ -280,6 +280,19 @@
         self.assertTrue(IObjectCreatedEvent.providedBy(bugtask_event))
         self.assertTrue(IObjectCreatedEvent.providedBy(bug_event))
 
+    def test_execute_bug_cannot_add_task(self):
+        bug = self.factory.makeBug(private=True)
+        product = self.factory.makeProduct(name='fnord')
+        login_person(bug.bugtasks[0].target.owner)
+        command = AffectsEmailCommand('affects', ['fnord'])
+        error = self.assertRaises(
+            EmailProcessingError, command.execute, bug, None)
+        message = str(error).split('\n')
+        self.assertEqual(
+            ("Bug %s cannot be marked as affecting more than one "
+            "target because it is private." % bug.id),
+            message[0])
+
 
 class BugEmailCommandTestCase(TestCaseWithFactory):
 

=== modified file 'lib/lp/bugs/model/bug.py'
--- lib/lp/bugs/model/bug.py	2011-10-19 20:19:27 +0000
+++ lib/lp/bugs/model/bug.py	2011-10-20 07:58:53 +0000
@@ -131,6 +131,7 @@
 from lp.bugs.enum import BugNotificationLevel
 from lp.bugs.errors import InvalidDuplicateValue
 from lp.bugs.interfaces.bug import (
+    CannotAddBugTask,
     IBug,
     IBugBecameQuestionEvent,
     IBugMute,
@@ -1199,6 +1200,11 @@
 
     def addTask(self, owner, target):
         """See `IBug`."""
+        if not self.canAddTask():
+            raise CannotAddBugTask(
+                ("Private bugs cannot be marked as affecting more than one "
+                "target."))
+
         new_task = getUtility(IBugTaskSet).createTask(self, owner, target)
 
         # When a new task is added the bug's heat becomes relevant to the
@@ -1207,6 +1213,10 @@
 
         return new_task
 
+    def canAddTask(self):
+        """See `IBug`."""
+        return not self.private or len(self.bugtasks) == 0
+
     def addWatch(self, bugtracker, remotebug, owner):
         """See `IBug`."""
         # We shouldn't add duplicate bug watches.
@@ -1530,6 +1540,8 @@
 
     def canBeNominatedFor(self, target):
         """See `IBug`."""
+        if not self.canAddTask():
+            return False
         try:
             self.getNominationFor(target)
         except NotFoundError:

=== modified file 'lib/lp/bugs/templates/bugtasks-and-nominations-table.pt'
--- lib/lp/bugs/templates/bugtasks-and-nominations-table.pt	2011-07-18 09:23:10 +0000
+++ lib/lp/bugs/templates/bugtasks-and-nominations-table.pt	2011-10-20 07:58:53 +0000
@@ -82,7 +82,8 @@
 
 </table>
 
-<div class="actions"
+<div id='also-affects-links' class="actions"
+     tal:attributes="style python:'display: '+(context.canAddTask() and 'block' or 'none')"
      tal:define="current_bugtask view/current_bugtask"
      tal:condition="view/displayAlsoAffectsLinks">
   <tal:also-affects-links

=== modified file 'lib/lp/bugs/tests/test_bugs_webservice.py'
--- lib/lp/bugs/tests/test_bugs_webservice.py	2011-08-28 07:29:11 +0000
+++ lib/lp/bugs/tests/test_bugs_webservice.py	2011-10-20 07:58:53 +0000
@@ -37,6 +37,7 @@
 from lp.testing import (
     api_url,
     launchpadlib_for,
+    login_person,
     TestCaseWithFactory,
     )
 from lp.testing._webservice import QueryCollector
@@ -347,3 +348,15 @@
         lp_bug = launchpad.load(api_url(bug))
         self.assertRaises(
             BadRequest, lp_bug.addTask, target=api_url(product))
+
+    def test_add_bugtask_to_private_bug_gives_bad_request(self):
+        # Test we cannot add a second bug task to a private bug.
+        owner = self.factory.makePerson()
+        bug = self.factory.makeBug(private=True, owner=owner)
+        product = self.factory.makeProduct()
+
+        login_person(owner)
+        launchpad = launchpadlib_for('test', owner)
+        lp_bug = launchpad.load(api_url(bug))
+        self.assertRaises(
+            BadRequest, lp_bug.addTask, target=api_url(product))