Merge lp:~bac/launchpad/bug-828572 into lp:launchpad

[Brad Crittenden]

[r=jcsackett][bug=828572] Changes to a bug task immediately (< 1 minute) after marking it incomplete no longer count as a response.

= Summary =

Changes to a bug task immediately (< 1 minute) after marking it
incomplete should not count as a response.

== Proposed fix ==

Put in time interval tweaks in the query.

== Pre-implementation notes ==

None

== Implementation details ==

As above

== Tests ==

bin/test -vvm lp.bugs -t test_incomplete

== Demo and Q/A ==

Mark bug task incomplete. Within a minute add a tag. See that
searches for WITH and WITHOUT RESPONSES work.

= 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/bugtask.py

[j.c.sackett (jcsackett)]

This looks fine.

review: Approve

[Francis J. Lacoste (flacoste)]

Hi Brad,

Thanks for working on this stakeholders' escalated bug.

You didn't list anybody in the Pre-implementation section, did you discuss with anyone if this change would be appropriate?

I fear that solution is inadequate for a few reasons:

* The original bug mention this would also affect expiration, this solution doesn't address that. (Not necessarily a big problem, you could easily file another bug about the expiration aspect.)

* If for some reason the user is available and respond within one minute, the bug will still be considered without response. That might be a corner case, but it's still possible and would result in a bug being filed.

I'd suggest approaching this from another angle: don't update date_last_message for changes that shouldn't be considered for expiration or don't really consider a response. In a way, date_last_message should only be updated for "new messages", not for status, importance, tags, or any other non-message related changes. That should solve both problems.

It would leave all existing bugs that had their date_last_message wrongly set show up in the searches. We might consider a data migration for that (update the date_last_message field based on the date of the... last message for all incomplete bug. We should do that in a garbo task.)

review: Needs Fixing

[Robert Collins (lifeless)]

+1 on flacostes review - I have a branch that makes the two
INCOMPLETE_WITH_RESPONSE and INCOMPLETE_WITHOUT_RESPONSE distinct
enums - so putting slack in the query will definitely not be
sufficient for this bug.

[Brad Crittenden (bac)]

Hi Robert,

Thanks for your comments. I'd like to see that branch you mention. Could you link it to the bug?

Unmerged revisions

13926. By Brad Crittenden on 2011-09-13

Make INCOMPLETE_WITH/OUT_RESPONSE ignore immediate changes

=== modified file 'lib/lp/bugs/model/bugtask.py'
--- lib/lp/bugs/model/bugtask.py	2011-09-12 20:27:32 +0000
+++ lib/lp/bugs/model/bugtask.py	2011-09-13 21:22:25 +0000
@@ -1694,17 +1694,21 @@
                 status_clause = (
                     '(BugTask.status = %s) ' %
                     sqlvalues(BugTaskStatus.INCOMPLETE))
+                # The date_last_message is fudged by one minute for
+                # with_response and without_response to not consider changes
+                # made at about the same time as responses.  See Bug 828572
+                # for more details.
                 if with_response:
                     status_clause += ("""
                         AND (Bug.date_last_message IS NOT NULL
                              AND BugTask.date_incomplete <=
-                                 Bug.date_last_message)
+                                 Bug.date_last_message - interval '1 minute')
                         """)
                 elif without_response:
                     status_clause += ("""
                         AND (Bug.date_last_message IS NULL
                              OR BugTask.date_incomplete >
-                                Bug.date_last_message)
+                                Bug.date_last_message - interval '1 minute')
                         """)
                 else:
                     assert with_response != without_response

=== modified file 'lib/lp/bugs/model/tests/test_bugtask.py'
--- lib/lp/bugs/model/tests/test_bugtask.py	2011-09-12 20:27:32 +0000
+++ lib/lp/bugs/model/tests/test_bugtask.py	2011-09-13 21:22:25 +0000
@@ -32,6 +32,7 @@
     BugTaskImportance,
     BugTaskSearchParams,
     BugTaskStatus,
+    BugTaskStatusSearch,
     IBugTaskSet,
     RESOLVED_BUGTASK_STATUSES,
     UNRESOLVED_BUGTASK_STATUSES,
@@ -1008,6 +1009,68 @@
         result = target.searchTasks(None, created_since=date)
         self.assertEqual([task2], list(result))
 
+    def _setupIncompleteBugTask(self):
+        target = self.makeBugTarget()
+        self.login()
+        task = self.factory.makeBugTask(target=target)
+        bug = task.bug
+        task.transitionToStatus(BugTaskStatus.INCOMPLETE, bug.owner)
+        return target, task, bug
+
+    def test_incomplete_with_response_none_found(self):
+        target, task, bug = self._setupIncompleteBugTask()
+        btsp = BugTaskSearchParams(
+            bug.owner,
+            status=BugTaskStatusSearch.INCOMPLETE_WITH_RESPONSE)
+        result = target.searchTasks(btsp)
+        self.assertEqual([], list(result))
+
+    def test_incomplete_with_response_ignores_immediate_changes(self):
+        target, task, bug = self._setupIncompleteBugTask()
+        bug.tags = 'escalated'
+        btsp = BugTaskSearchParams(
+            bug.owner,
+            status=BugTaskStatusSearch.INCOMPLETE_WITH_RESPONSE)
+        result = target.searchTasks(btsp)
+        self.assertEqual([], list(result))
+
+    def test_incomplete_with_response_added(self):
+        target, task, bug = self._setupIncompleteBugTask()
+        bug.tags = 'escalated'
+        removeSecurityProxy(bug).date_last_message += timedelta(seconds=65)
+        btsp = BugTaskSearchParams(
+            bug.owner,
+            status=BugTaskStatusSearch.INCOMPLETE_WITH_RESPONSE)
+        result = target.searchTasks(btsp)
+        self.assertEqual([task], list(result))
+
+    def test_incomplete_without_response_none_found(self):
+        target, task, bug = self._setupIncompleteBugTask()
+        btsp = BugTaskSearchParams(
+            bug.owner,
+            status=BugTaskStatusSearch.INCOMPLETE_WITHOUT_RESPONSE)
+        result = target.searchTasks(btsp)
+        self.assertEqual([task], list(result))
+
+    def test_incomplete_without_response_ignores_immediate_change(self):
+        target, task, bug = self._setupIncompleteBugTask()
+        bug.tags = 'escalated'
+        btsp = BugTaskSearchParams(
+            bug.owner,
+            status=BugTaskStatusSearch.INCOMPLETE_WITHOUT_RESPONSE)
+        result = target.searchTasks(btsp)
+        self.assertEqual([task], list(result))
+
+    def test_incomplete_without_response_one_added(self):
+        target, task, bug = self._setupIncompleteBugTask()
+        bug.tags = 'escalated'
+        removeSecurityProxy(bug).date_last_message += timedelta(seconds=65)
+        btsp = BugTaskSearchParams(
+            bug.owner,
+            status=BugTaskStatusSearch.INCOMPLETE_WITHOUT_RESPONSE)
+        result = target.searchTasks(btsp)
+        self.assertEqual([], list(result))
+
 
 class BugTaskSetSearchTest(TestCaseWithFactory):