launchpad-reviewers team mailing list archive

[Brad Crittenden <bac@xxxxxxxxxxxxx>]

Brad Crittenden has proposed merging lp:~bac/launchpad/bug-831991 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #831991 in Launchpad itself: "When the Janitor autoconfirms a bug, it should explain why"
  https://bugs.launchpad.net/launchpad/+bug/831991

For more details, see:
https://code.launchpad.net/~bac/launchpad/bug-831991/+merge/75068

= Summary =

When a bug is auto-confirmed by the janitor it should make a comment
as to why so as not to confuse innocent bystanders.

== Proposed fix ==

Simply create a new comment when the status is changed in
'maybeConfirm'.

== Pre-implementation notes ==

Nice enlightning chat with Graham.

== Implementation details ==

As above.

== Tests ==

bin/test --vt test_auto_confirm

== Demo and Q/A ==

Find a new bug you didn't create and select "Affects me". See the bug
change to 'Confirmed'.  Re-load the page and you'll see the bug
comment.

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/bugs/model/tests/test_bugtask.py
  lib/lp/bugs/model/bug.py
  lib/lp/bugs/model/bugtask.py
-- 
https://code.launchpad.net/~bac/launchpad/bug-831991/+merge/75068
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~bac/launchpad/bug-831991 into lp:launchpad.

=== modified file 'lib/lp/bugs/model/bug.py'
--- lib/lp/bugs/model/bug.py	2011-08-30 19:25:53 +0000
+++ lib/lp/bugs/model/bug.py	2011-09-12 20:31:27 +0000
@@ -1845,14 +1845,13 @@
             self.duplicateof = duplicate_of
         except LaunchpadValidationError, validation_error:
             raise InvalidDuplicateValue(validation_error)
-
         if duplicate_of is not None:
             # Update the heat of the master bug and set this bug's heat
             # to 0 (since it's a duplicate, it shouldn't have any heat
             # at all).
             self.setHeat(0, affected_targets=affected_targets)
             # Maybe confirm bug tasks, now that more people might be affected
-            # by this bug.
+            # by this bug from the duplicates.
             duplicate_of.maybeConfirmBugtasks()
         else:
             # Otherwise, recalculate this bug's heat, since it will be 0

=== modified file 'lib/lp/bugs/model/bugtask.py'
--- lib/lp/bugs/model/bugtask.py	2011-09-10 00:03:46 +0000
+++ lib/lp/bugs/model/bugtask.py	2011-09-12 20:31:27 +0000
@@ -843,12 +843,17 @@
             and self._checkAutoconfirmFeatureFlag()
             # END TEMPORARY BIT FOR BUGTASK AUTOCONFIRM FEATURE FLAG.
             ):
-            user = getUtility(ILaunchpadCelebrities).janitor
+            janitor = getUtility(ILaunchpadCelebrities).janitor
             bugtask_before_modification = Snapshot(
                 self, providing=providedBy(self))
-            self.transitionToStatus(BugTaskStatus.CONFIRMED, user)
+            # Create a bug message explaining why the janitor auto-confirmed
+            # the bugtask.
+            msg = ("Status changed to 'Confirmed' because the bug "
+                   "affects multiple users.")
+            self.bug.newMessage(owner=janitor, content=msg)
+            self.transitionToStatus(BugTaskStatus.CONFIRMED, janitor)
             notify(ObjectModifiedEvent(
-                self, bugtask_before_modification, ['status'], user=user))
+                self, bugtask_before_modification, ['status'], user=janitor))
 
     def canTransitionToStatus(self, new_status, user):
         """See `IBugTask`."""
@@ -1146,7 +1151,6 @@
         name in this distribution will have their names updated to
         match. This should only be used by _syncSourcePackages.
         """
-
         if self.target == target:
             return
 

=== modified file 'lib/lp/bugs/model/tests/test_bugtask.py'
--- lib/lp/bugs/model/tests/test_bugtask.py	2011-08-23 01:06:14 +0000
+++ lib/lp/bugs/model/tests/test_bugtask.py	2011-09-12 20:31:27 +0000
@@ -1597,24 +1597,35 @@
     layer = DatabaseFunctionalLayer
 
     def test_auto_confirm(self):
-        # A typical new bugtask auto-confirms.
+        # A typical new bugtask auto-confirms.  Doing so changes the status of
+        # the bug task, creates a status event, and creates a new comment
+        # indicating the reason the Janitor auto-confirmed.
         # When feature flag code is removed, remove the next two lines and
         # dedent the rest.
         with feature_flags():
             set_feature_flag(u'bugs.autoconfirm.enabled_product_names', u'*')
             bug_task = self.factory.makeBugTask()
+            bug = bug_task.bug
             self.assertEqual(BugTaskStatus.NEW, bug_task.status)
+            original_comment_count = bug.messages.count()
             with EventRecorder() as recorder:
                 bug_task.maybeConfirm()
                 self.assertEqual(BugTaskStatus.CONFIRMED, bug_task.status)
-                self.assertEqual(1, len(recorder.events))
-                event = recorder.events[0]
+                self.assertEqual(2, len(recorder.events))
+                msg_event, mod_event = recorder.events
                 self.assertEqual(getUtility(ILaunchpadCelebrities).janitor,
-                                 event.user)
-                self.assertEqual(['status'], event.edited_fields)
+                                 mod_event.user)
+                self.assertEqual(['status'], mod_event.edited_fields)
                 self.assertEqual(BugTaskStatus.NEW,
-                                 event.object_before_modification.status)
-                self.assertEqual(bug_task, event.object)
+                                 mod_event.object_before_modification.status)
+                self.assertEqual(bug_task, mod_event.object)
+                # A new comment is recorded.
+                self.assertEqual(
+                    original_comment_count + 1, bug.messages.count())
+                self.assertEqual(
+                    u"Status changed to 'Confirmed' because the bug affects "
+                    "multiple users.",
+                    bug.messages[-1].text_contents)
 
     def test_do_not_confirm_bugwatch_tasks(self):
         # A bugwatch bugtask does not auto-confirm.