launchpad-reviewers team mailing list archive

[Curtis Hovey <curtis.hovey@xxxxxxxxxxxxx>]

Curtis Hovey has proposed merging lp:~sinzui/launchpad/retarget-bug-milestone into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #157606 in Launchpad itself: "IntegrityError with unknown milestone when changing bug's project"
  https://bugs.launchpad.net/launchpad/+bug/157606

For more details, see:
https://code.launchpad.net/~sinzui/launchpad/retarget-bug-milestone/+merge/54058

Unset the milestone when non-priviliged users retarget bugs.

    Launchpad bug: https://bugs.launchpad.net/bugs/157606
    Pre-implementation: mnay people on the discussion list
    Test command: ./bin/test -vv -t TestBugTaskEditView

OOPS-1532EC1035 shows a non-privileged user changing the project using
+editstatus. He does not have permission to set the status or the milestone,
the fields are not included. The view is passing the existing milestone
instead of clearing it.

We discussed deleting the bug task and creating a new one to avoid
permission and state issues. This is feature level work though, and there
are still 3 months of critical bugs to fix. This branch just fixes the
immediate issue.

--------------------------------------------------------------------

RULES

    * Always clear the milestone when retargeting a different project
      or distro.


QA

    * https://bugs.qastaging.launchpad.net/landscape/+bug/538206/+editstatus
    * Set the project to Launchpad.
    * Verify the bug is targeted to Launchpad and the milestone is unset.


LINT

    lib/lp/bugs/browser/bugtask.py
    lib/lp/bugs/browser/tests/test_bugtask.py


IMPLEMENTATION

Added a test to verify the milestone block in the updateContextFromData()
method. The block's guard wrongly checked if the milestone was in the field
name, which it will not be if the user is not a driver for the project.
I could not find any tests specific for the block, so I added a test for
the assigning-a-milestone condition too.
    lib/lp/bugs/browser/bugtask.py
    lib/lp/bugs/browser/tests/test_bugtask.py
-- 
https://code.launchpad.net/~sinzui/launchpad/retarget-bug-milestone/+merge/54058
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/retarget-bug-milestone into lp:launchpad.

=== modified file 'lib/lp/bugs/browser/bugtask.py'
--- lib/lp/bugs/browser/bugtask.py	2011-03-15 22:58:07 +0000
+++ lib/lp/bugs/browser/bugtask.py	2011-03-18 19:26:15 +0000
@@ -49,7 +49,6 @@
     timedelta,
     )
 from itertools import (
-    chain,
     groupby,
     )
 from math import (
@@ -118,7 +117,10 @@
     SimpleVocabulary,
     )
 from zope.security.interfaces import Unauthorized
-from zope.security.proxy import isinstance as zope_isinstance
+from zope.security.proxy import (
+    isinstance as zope_isinstance,
+    removeSecurityProxy,
+    )
 from zope.traversing.interfaces import IPathAdapter
 
 from canonical.config import config
@@ -309,8 +311,8 @@
 
     :param for_display: If true, the zeroth comment is given an empty body so
         that it will be filtered by get_visible_comments.
-    :param slice_info: If not None, defines a list of slices of the comments to
-        retrieve.
+    :param slice_info: If not None, defines a list of slices of the comments
+        to retrieve.
     """
     comments = build_comments_from_chunks(bugtask, truncate=truncate,
         slice_info=slice_info)
@@ -655,11 +657,11 @@
             edit_url=canonical_url(self.context, view_name='+edit'))
 
         # XXX 2010-10-05 gmb bug=655597:
-        #     This line of code keeps the view's query count down,
-        #     possibly using witchcraft. It should be rewritten to be
-        #     useful or removed in favour of making other queries more
-        #     efficient. The witchcraft is because the subscribers are accessed
-        #     in the initial page load, so the data is actually used.
+        # This line of code keeps the view's query count down,
+        # possibly using witchcraft. It should be rewritten to be
+        # useful or removed in favour of making other queries more
+        # efficient. The witchcraft is because the subscribers are accessed
+        # in the initial page load, so the data is actually used.
         if self.user is not None:
             list(bug.getSubscribersForPerson(self.user))
 
@@ -815,7 +817,7 @@
                     # There is a gap here, record it.
                     separator = {
                         'date': prev_comment.datecreated,
-                        'num_hidden': comment.index - prev_comment.index
+                        'num_hidden': comment.index - prev_comment.index,
                         }
                     events.insert(index, separator)
                     index += 1
@@ -1421,8 +1423,7 @@
         milestone_cleared = None
         milestone_ignored = False
         if (IUpstreamBugTask.providedBy(bugtask) and
-            (bugtask.product != new_values.get("product")) and
-            'milestone' in field_names):
+            (bugtask.product != new_values.get("product"))):
             # We clear the milestone value if one was already set. We ignore
             # the milestone value if it was currently None, and the user tried
             # to set a milestone value while also changing the product. This
@@ -1432,12 +1433,15 @@
             elif new_values.get('milestone') is not None:
                 milestone_ignored = True
 
-            bugtask.milestone = None
+            # Regardless of the user's permission, the milestone
+            # must be cleared because the milestone is unique to a product.
+            removeSecurityProxy(bugtask).milestone = None
             # Remove the "milestone" field from the list of fields
             # whose changes we want to apply, because we don't want
             # the form machinery to try and set this value back to
             # what it was!
-            del data_to_apply["milestone"]
+            if 'milestone' in data_to_apply:
+                del data_to_apply["milestone"]
 
         # We special case setting assignee and status, because there's
         # a workflow associated with changes to these fields.
@@ -3052,7 +3056,8 @@
             distro_packages))
         distro_series_set = getUtility(IDistroSeriesSet)
         self.target_releases.update(
-            distro_series_set.getCurrentSourceReleases(distro_series_packages))
+            distro_series_set.getCurrentSourceReleases(
+                distro_series_packages))
         ids = set()
         for release_person_ids in map(attrgetter('creatorID', 'maintainerID'),
             self.target_releases.values()):

=== modified file 'lib/lp/bugs/browser/tests/test_bugtask.py'
--- lib/lp/bugs/browser/tests/test_bugtask.py	2011-03-16 00:50:54 +0000
+++ lib/lp/bugs/browser/tests/test_bugtask.py	2011-03-18 19:26:15 +0000
@@ -546,7 +546,7 @@
 
 
 class TestBugTaskEditView(TestCaseWithFactory):
-    """Test the bugs overview page for Project Groups."""
+    """Test the bug task edit form."""
 
     layer = DatabaseFunctionalLayer
 
@@ -582,6 +582,65 @@
             'This bug has already been reported on mouse (ubuntu).',
             view.errors[0])
 
+    def setUpRetargetMilestone(self):
+        """Setup a bugtask with a milestone and a product to retarget to."""
+        first_product = self.factory.makeProduct(name='bunny')
+        with person_logged_in(first_product.owner):
+            first_product.official_malone = True
+            bug = self.factory.makeBug(product=first_product)
+            bug_task = bug.bugtasks[0]
+            milestone = self.factory.makeMilestone(
+                productseries=first_product.development_focus, name='1.0')
+            bug_task.transitionToMilestone(milestone, first_product.owner)
+        second_product = self.factory.makeProduct(name='duck')
+        with person_logged_in(second_product.owner):
+            second_product.official_malone = True
+        return bug_task, second_product
+
+    def test_retarget_product_with_milestone(self):
+        # Milestones are always cleared when retargeting a product bug task.
+        bug_task, second_product = self.setUpRetargetMilestone()
+        user = self.factory.makePerson()
+        login_person(user)
+        form = {
+            'bunny.status': 'In Progress',
+            'bunny.assignee.option': 'bunny.assignee.assign_to_nobody',
+            'bunny.product': 'duck',
+            'bunny.actions.save': 'Save Changes',
+            }
+        view = create_initialized_view(
+            bug_task, name='+editstatus', form=form)
+        self.assertEqual([], view.errors)
+        self.assertEqual(second_product, bug_task.target)
+        self.assertEqual(None, bug_task.milestone)
+        notifications = view.request.response.notifications
+        self.assertEqual(1, len(notifications))
+        expected = ('The Bunny 1.0 milestone setting has been removed')
+        self.assertTrue(notifications.pop().message.startswith(expected))
+
+    def test_retarget_product_and_assign_milestone(self):
+        # Milestones are always cleared when retargeting a product bug task.
+        bug_task, second_product = self.setUpRetargetMilestone()
+        login_person(bug_task.target.owner)
+        milestone_id = bug_task.milestone.id
+        bug_task.transitionToMilestone(None, bug_task.target.owner)
+        form = {
+            'bunny.status': 'In Progress',
+            'bunny.assignee.option': 'bunny.assignee.assign_to_nobody',
+            'bunny.product': 'duck',
+            'bunny.milestone': milestone_id,
+            'bunny.actions.save': 'Save Changes',
+            }
+        view = create_initialized_view(
+            bug_task, name='+editstatus', form=form)
+        self.assertEqual([], view.errors)
+        self.assertEqual(second_product, bug_task.target)
+        self.assertEqual(None, bug_task.milestone)
+        notifications = view.request.response.notifications
+        self.assertEqual(1, len(notifications))
+        expected = ('The milestone setting was ignored')
+        self.assertTrue(notifications.pop().message.startswith(expected))
+
 
 class TestProjectGroupBugs(TestCaseWithFactory):
     """Test the bugs overview page for Project Groups."""