launchpad-reviewers team mailing list archive

[Curtis Hovey <curtis.hovey@xxxxxxxxxxxxx>]

Curtis Hovey has proposed merging lp:~sinzui/launchpad/delete-conjoined-bugtask-0 into lp:launchpad/devel.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #522599 delete milestone conjoined bug
  https://bugs.launchpad.net/bugs/522599


This is my branch to delete milestone conjoined bugtasks instead of oops.

    lp:~sinzui/launchpad/delete-conjoined-bugtask-0
    Diff size: 74
    Launchpad bug:
          https://bugs.launchpad.net/bugs/522599
    Test command: ./bin/test -vv \
          -t TestMilestoneDeleteView -t registry.*test_yuitests
    Pre-implementation: deryck, jcsackett
    Target release: 10.09


Delete milestone conjoined bugtasks instead of oops
---------------------------------------------------

OOPS-1507A1604, shows that the act of untargeting a bugtask fails when it is
conjoined. This is contrary to the intended rules where the bugtask should be
deleted...removing the insane conjoined situation.


Rules
-----

In discussion with Deryck and jcsackett regarding when a bug is conjoined
and where the oops really takes place. We discovered there are two, not
one, condition where a bugtask must be deleted. All bugtasks targeted to
product series are deleted since the project already has a bugtask, but
in the case of milestones, some of them may also be conjoined.

    * When iterating over the milestone's bug tasks, check if it has a
      conjoined_master and call ``Store.of(bugtask).remove(bugtask)``
      as the productseries delete milestone code does.


QA
--

    * Create a series for a project you control and make it the development
      focus.
    * Create a milestone for the release.
    * Target a bug to the series.
    * Choose the delete the milestone.
    * Verify the milestone is deleted.


Lint
----

Linting changed files:
  lib/lp/registry/browser/__init__.py
  lib/lp/registry/browser/tests/test_milestone.py



Test
----

    * lib/lp/registry/browser/tests/test_milestone.py
      * Added a test that reproduced error seen in the oops and verification
        that the milestone was deleted.


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

    * lib/lp/registry/browser/__init__.py
      * Added a condition to check for a conjoined_master and delete it when
        there is a master.
      * Wrapped long line per lint.
-- 
https://code.launchpad.net/~sinzui/launchpad/delete-conjoined-bugtask-0/+merge/32163
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/delete-conjoined-bugtask-0 into lp:launchpad/devel.

=== modified file 'lib/lp/registry/browser/__init__.py'
--- lib/lp/registry/browser/__init__.py	2010-06-12 03:37:58 +0000
+++ lib/lp/registry/browser/__init__.py	2010-08-10 01:06:06 +0000
@@ -115,10 +115,11 @@
                     var create_milestone_link = Y.get(
                         '.menu-link-create_milestone');
                     create_milestone_link.addClass('js-action');
+                    var milestone_table = Y.lp.registry.milestonetable;
                     var config = {
                         milestone_form_uri: milestone_form_uri,
                         series_uri: series_uri,
-                        next_step: Y.lp.registry.milestonetable.get_milestone_row,
+                        next_step: milestone_table.get_milestone_row,
                         activate_node: create_milestone_link
                         };
                     Y.lp.registry.milestoneoverlay.attach_widget(config);
@@ -216,7 +217,10 @@
         """Delete a milestone and unlink related objects."""
         self._unsubscribe_structure(milestone)
         for bugtask in self._getBugtasks(milestone):
-            bugtask.milestone = None
+            if bugtask.conjoined_master is not None:
+                Store.of(bugtask).remove(bugtask)
+            else:
+                bugtask.milestone = None
         for spec in self._getSpecifications(milestone):
             spec.milestone = None
         self._deleteRelease(milestone.product_release)

=== modified file 'lib/lp/registry/browser/tests/test_milestone.py'
--- lib/lp/registry/browser/tests/test_milestone.py	2010-08-06 20:15:31 +0000
+++ lib/lp/registry/browser/tests/test_milestone.py	2010-08-10 01:06:06 +0000
@@ -7,7 +7,11 @@
 
 import unittest
 
-from lp.testing import ANONYMOUS, login_person, login
+from zope.component import getUtility
+
+from canonical.testing.layers import DatabaseFunctionalLayer
+from lp.bugs.interfaces.bugtask import IBugTaskSet
+from lp.testing import ANONYMOUS, login_person, login, TestCaseWithFactory
 from lp.testing.views import create_initialized_view
 from lp.testing.memcache import MemcacheTestCase
 
@@ -78,5 +82,27 @@
         self.assertEqual(360, view.expire_cache_minutes)
 
 
+class TestMilestoneDeleteView(TestCaseWithFactory):
+    """Test the delete rules applied by the Milestone Delete view."""
+
+    layer = DatabaseFunctionalLayer
+
+    def test_delete_conjoined_bugtask(self):
+        product = self.factory.makeProduct()
+        bug = self.factory.makeBug(product=product)
+        master_bugtask = getUtility(IBugTaskSet).createTask(
+            bug, productseries=product.development_focus, owner=product.owner)
+        milestone = self.factory.makeMilestone(
+            productseries=product.development_focus, name="red")
+        login_person(product.owner)
+        master_bugtask.transitionToMilestone(milestone, product.owner)
+        form = {
+            'field.actions.delete': 'Delete Milestone',
+            }
+        view = create_initialized_view(milestone, '+delete', form=form)
+        self.assertEqual([], view.errors)
+        self.assertEqual(0, len(product.all_milestones))
+
+
 def test_suite():
     return unittest.TestLoader().loadTestsFromName(__name__)