launchpad-reviewers team mailing list archive

[Graham Binns <graham@xxxxxxxxxxxxx>]

Graham Binns has proposed merging lp:~gmb/launchpad/conjoined-oops-bug-106338 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #106338 in Launchpad itself: "Editing a bug targeted to a release crashes if you directly edit the untargeted task "
  https://bugs.launchpad.net/launchpad/+bug/106338

For more details, see:
https://code.launchpad.net/~gmb/launchpad/conjoined-oops-bug-106338/+merge/62456

This branch stops ConjoinedBugTaskEditErrors from being raised when someone tries to edit the slave task in a pair of conjoined bug tasks.

I've taken the simplest approach here, which is, when encountering someone trying to change an attribute on a slave task, to pass that alteration to the master task. The change will then propagate down to the slave task.

I've added unit tests to cover the change and have removed some doctest that no longer applies.
-- 
https://code.launchpad.net/~gmb/launchpad/conjoined-oops-bug-106338/+merge/62456
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~gmb/launchpad/conjoined-oops-bug-106338 into lp:launchpad.

=== modified file 'lib/lp/bugs/doc/bugtask.txt'
--- lib/lp/bugs/doc/bugtask.txt	2011-03-28 20:54:50 +0000
+++ lib/lp/bugs/doc/bugtask.txt	2011-05-26 10:43:29 +0000
@@ -648,22 +648,6 @@
     >>> print current_series_netapplet_task.sourcepackagename.name
     pmount
 
-The generic tasks are not directly editable.
-
-    >>> generic_netapplet_task.transitionToStatus(
-    ...     BugTaskStatus.INVALID, getUtility(ILaunchBag).user)
-    Traceback (most recent call last):
-      ...
-    ConjoinedBugTaskEditError: This task cannot be edited directly, it
-                               should be edited through its conjoined_master.
-
-    >>> generic_alsa_utils_task.transitionToAssignee(launchbag.user)
-    Traceback (most recent call last):
-      ...
-    ConjoinedBugTaskEditError: This task cannot be edited directly, it
-                               should be edited through its conjoined_master.
-
-
 A conjoined relationship can be broken, though. If the development
 task (i.e the conjoined master) is Won't Fix, it means that the bug is
 deferred to the next series. In this case the development task should

=== modified file 'lib/lp/bugs/model/bugtask.py'
--- lib/lp/bugs/model/bugtask.py	2011-05-04 01:31:58 +0000
+++ lib/lp/bugs/model/bugtask.py	2011-05-26 10:43:29 +0000
@@ -431,10 +431,14 @@
     if self.bug is None:
         return value
 
+    # If this is a conjoined slave then call setattr on the master.
+    # Effectively this means that making a change to the slave will
+    # actually make the change to the master (which will then be passed
+    # down to the slave, of course). This helps to prevent OOPSes when
+    # people try to update the conjoined slave via the API.
     if self._isConjoinedBugTask():
-        raise ConjoinedBugTaskEditError(
-            "This task cannot be edited directly, it should be"
-            " edited through its conjoined_master.")
+        setattr(self.conjoined_master, attr, value)
+
     # The conjoined slave is updated before the master one because,
     # for distro tasks, conjoined_slave does a comparison on
     # sourcepackagename, and the sourcepackagenames will not match

=== modified file 'lib/lp/bugs/model/tests/test_bugtask.py'
--- lib/lp/bugs/model/tests/test_bugtask.py	2011-04-11 12:52:27 +0000
+++ lib/lp/bugs/model/tests/test_bugtask.py	2011-05-26 10:43:29 +0000
@@ -1384,3 +1384,68 @@
         self.assertEqual(person.displayname, result['person_name'])
         self.assertEqual(
             bug.default_bugtask.pillar.displayname, result['pillar_name'])
+
+
+class TestConjoinedBugTasks(TestCaseWithFactory):
+    """Tests for conjoined bug task functionality."""
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        super(TestConjoinedBugTasks, self).setUp()
+        self.owner = self.factory.makePerson()
+        self.distro = self.factory.makeDistribution(
+            name="eggs", owner=self.owner, bug_supervisor=self.owner)
+        distro_release = self.factory.makeDistroRelease(
+            distribution=self.distro, registrant=self.owner)
+        source_package = self.factory.makeSourcePackage(
+            sourcepackagename="spam", distroseries=distro_release)
+        bug = self.factory.makeBug(
+            distribution=self.distro,
+            sourcepackagename=source_package.sourcepackagename,
+            owner=self.owner)
+        with person_logged_in(self.owner):
+            nomination = bug.addNomination(self.owner, distro_release)
+            nomination.approve(self.owner)
+            self.generic_task, self.series_task = bug.bugtasks
+            self.assertEqual(
+                self.generic_task, self.series_task.conjoined_slave)
+
+    def test_editing_generic_status_reflects_upon_conjoined_master(self):
+        # If a change is made to the status of a conjoined slave
+        # (generic) task, that change is reflected upon the conjoined
+        # master.
+        with person_logged_in(self.owner):
+            self.generic_task.transitionToStatus(
+                BugTaskStatus.CONFIRMED, self.owner)
+            self.assertEqual(
+                BugTaskStatus.CONFIRMED, self.series_task.status)
+
+    def test_editing_generic_importance_reflects_upon_conjoined_master(self):
+        # If a change is made to the importance of a conjoined slave
+        # (generic) task, that change is reflected upon the conjoined
+        # master.
+        with person_logged_in(self.owner):
+            self.generic_task.transitionToImportance(
+                BugTaskImportance.HIGH, self.owner)
+            self.assertEqual(
+                BugTaskImportance.HIGH, self.series_task.importance)
+
+    def test_editing_generic_assignee_reflects_upon_conjoined_master(self):
+        # If a change is made to the assignee of a conjoined slave
+        # (generic) task, that change is reflected upon the conjoined
+        # master.
+        with person_logged_in(self.owner):
+            self.generic_task.transitionToAssignee(self.owner)
+            self.assertEqual(
+                self.owner, self.series_task.assignee)
+
+    def test_editing_generic_package_reflects_upon_conjoined_master(self):
+        # If a change is made to the source package of a conjoined slave
+        # (generic) task, that change is reflected upon the conjoined
+        # master.
+        source_package_name = self.factory.makeSourcePackageName("ham")
+        with person_logged_in(self.owner):
+            self.generic_task.sourcepackagename = source_package_name
+            self.assertEqual(
+                source_package_name, self.series_task.sourcepackagename)

=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2011-05-24 12:32:35 +0000
+++ lib/lp/testing/factory.py	2011-05-26 10:43:29 +0000
@@ -1684,8 +1684,9 @@
         if milestone is not None:
             bugtask.transitionToMilestone(milestone, milestone.target.owner)
         if series is not None:
-            task = bug.addTask(owner, series)
-            task.transitionToStatus(status, owner)
+            with person_logged_in(owner):
+                task = bug.addTask(owner, series)
+                task.transitionToStatus(status, owner)
 
         return bug