Merge lp:~gmb/launchpad/reschedule-button-qafix-bug-558415 into lp:launchpad/db-devel

[Graham Binns]

Bug watches can now be rescheduled by any user rather than just administrators.

This branch fixes a bug with the orignal fix for bug 558415 whereby the
reschedule button would be shown on a bug watch page but nobody but an
admin could click it without it OOPSing.

To fix this I've added a setNextCheck() method to IBugWatch. This can be
called by any user. The upshot of this is that users have a way of
rescheduling a watch, which we can control, without changing next_check
directly.

== lib/lp/bugs/browser/bugwatch.py ==

 - I've modified the BugWatchActivityPortletView to use
   BugWatch.setNextCheck() as described below.

== lib/lp/bugs/browser/tests/bugwatch-views.txt ==

 - I've moved the tests from this file into doc/bugwatch.txt since the
   logic for determining whether a watch can be updated is now part of
   the watch, not the view.

== lib/lp/bugs/configure.zcml ==

 - I've updated the ZCML as appropriate.

== lib/lp/bugs/doc/bugwatch.txt ==

 - I've moved the test from bugwatch-views.txt here and updated them as
   appropriate.
 - I've added tests for BugWatch.setNextCheck()

== lib/lp/bugs/interfaces/bugwatch.py ==

 - I've added a setNextCheck() method and can_be_rescheduled and
   failed_activity properties to IBugWatch.

== lib/lp/bugs/model/bugwatch.py ==

 - I've moved the implementation of userCanReschedule() here from the
   BugWatchActivityPortletView can called it can_be_rescheduled.
 - I've added implementations of IBugWatch.failed_activity and
   IBugWatch.setNextCheck().

== lib/lp/bugs/stories/bugwatches/xx-edit-bugwatch.txt ==

 - I've updated the test to use the user_browser rather than the admin
   browser. This demonstrates that any user can use the reschedule
   button.

== lib/lp/bugs/templates/bugwatch-portlet-activity.pt ==

 - I've updated the template to use the failed_activity and activity
   properties of the BugWatch rather than using view properties.

[Brad Crittenden (bac)]

Hi Graham this branch looks good. You have one typo of 'wathc'.

Also I know you just copied it from the existing code, but I'd expect to see

return failure_ratio < WATCH_RESCHEDULE_THRESHOLD

So that 6/10 failures would still return true.

review: Approve (code)

[Brad Crittenden (bac)]

Graham I forgot to mention there were some valid lint issues with this branch, like unneeded imports.

[Graham Binns (gmb)]

On 20 April 2010 18:25, Brad Crittenden <email address hidden> wrote:
> Graham I forgot to mention there were some valid lint issues with this branch, like unneeded imports.

Thanks Brad, I'll take care of those now.

--
Graham Binns | PGP Key: EC66FA7D

=== modified file 'lib/lp/bugs/browser/bugwatch.py'
--- lib/lp/bugs/browser/bugwatch.py	2010-04-15 15:14:21 +0000
+++ lib/lp/bugs/browser/bugwatch.py	2010-04-20 20:31:31 +0000
@@ -10,13 +10,10 @@
     'BugWatchEditView',
     'BugWatchView']
 
-from datetime import datetime
-from pytz import utc
 
 from zope.component import getUtility
 from zope.interface import Interface
 
-from canonical.cachedproperty import cachedproperty
 from canonical.database.constants import UTC_NOW
 from canonical.widgets.textwidgets import URIWidget
 
@@ -35,9 +32,6 @@
 from canonical.launchpad.webapp.menu import structured
 
 
-WATCH_RESCHEDULE_THRESHOLD = 0.6
-
-
 class BugWatchSetNavigation(GetitemNavigation):
 
     usedfor = IBugWatchSet
@@ -158,48 +152,15 @@
 
     schema = BugWatchEditForm
 
-    @cachedproperty
-    def total_watch_activity_count(self):
-        return self.context.activity.count()
-
-    @cachedproperty
-    def failed_watch_activity_count(self):
-        failed_activity_count = len([
-            activity for activity in self.context.activity if
-            activity.result not in BUG_WATCH_ACTIVITY_SUCCESS_STATUSES])
-        return failed_activity_count
-
     def userCanReschedule(self, action=None):
         """Return True if the current user can reschedule the bug watch."""
-        if (self.context.next_check is not None and
-            self.context.next_check <= datetime.now(utc)):
-            # If the watch is already scheduled for a time in the past
-            # (or for right now) it can't be rescheduled, since it
-            # should be be checked by the next checkwatches run anyway.
-            return False
-
-        if self.total_watch_activity_count == 0:
-            # Don't show the reschedule button if the watch has never
-            # been checked.
-            return False
-
-        if self.failed_watch_activity_count == 0:
-            # Don't show the reschedule button if the watch has never
-            # failed.
-            return False
-
-        # If the ratio is lower than the reschedule threshold, we
-        # can show the button.
-        failure_ratio = (
-            float(self.failed_watch_activity_count) /
-            self.total_watch_activity_count)
-        return failure_ratio <= WATCH_RESCHEDULE_THRESHOLD
+        return self.context.can_be_rescheduled
 
     @action('Update Now', name='reschedule', condition=userCanReschedule)
     def reschedule_action(self, action, data):
         """Schedule the current bug watch for immediate checking."""
         bugwatch = self.context
-        bugwatch.next_check = UTC_NOW
+        bugwatch.setNextCheck(UTC_NOW)
         self.request.response.addInfoNotification(
             structured(
                 'The <a href="%(url)s">%(bugtracker)s #%(remote_bug)s</a> '

=== modified file 'lib/lp/bugs/browser/tests/bugwatch-views.txt'
--- lib/lp/bugs/browser/tests/bugwatch-views.txt	2010-04-15 15:14:21 +0000
+++ lib/lp/bugs/browser/tests/bugwatch-views.txt	2010-04-20 20:31:31 +0000
@@ -110,58 +110,3 @@
      'result_text': 'Synchronisation succeeded'}
 
 
-Rescheduling a watch
---------------------
-
-The BugWatch activity portlet provides a method, userCanReschedule(),
-which indicates whether or not a user can reschedule a bug watch for
-checking.  For a new bug watch this will be False.
-
-    >>> from pytz import utc
-    >>> from datetime import datetime
-    >>> schedulable_watch = factory.makeBugWatch()
-    >>> schedulable_watch.next_check = None
-    >>> scheduling_view = create_initialized_view(
-    ...     schedulable_watch, '+portlet-activity')
-
-    >>> scheduling_view.userCanReschedule()
-    False
-
-If there's been activity on the watch but it's always been successful,
-userCanReschedule() will return False.
-
-    >>> schedulable_watch.addActivity()
-    >>> scheduling_view.userCanReschedule()
-    False
-
-If the watch's updates have failed less than 60% of the time,
-userCanReschedule() will return True
-
-    >>> scheduling_view = create_initialized_view(
-    ...     schedulable_watch, '+portlet-activity')
-    >>> schedulable_watch.addActivity(
-    ...     result=BugWatchActivityStatus.BUG_NOT_FOUND)
-    >>> scheduling_view.userCanReschedule()
-    True
-
-If the watch is rescheduled, the button will disappear, since the
-next_check time for the watch will be in the past (or in this case is
-now) and therefore it will be checked with the next checkwatches run.
-
-    >>> schedulable_watch.next_check = datetime.now(utc)
-    >>> scheduling_view = create_initialized_view(
-    ...     schedulable_watch, '+portlet-activity')
-    >>> scheduling_view.userCanReschedule()
-    False
-
-However, if the watch has failed more than 60% of the time the button
-will disappear again, since it's assumed that the watch needs attention
-in order for it to be able to work again.
-
-    >>> schedulable_watch.next_check = None
-    >>> schedulable_watch.addActivity(
-    ...     result=BugWatchActivityStatus.BUG_NOT_FOUND)
-    >>> scheduling_view = create_initialized_view(
-    ...     schedulable_watch, '+portlet-activity')
-    >>> scheduling_view.userCanReschedule()
-    False

=== modified file 'lib/lp/bugs/configure.zcml'
--- lib/lp/bugs/configure.zcml	2010-04-14 12:55:44 +0000
+++ lib/lp/bugs/configure.zcml	2010-04-20 20:31:31 +0000
@@ -849,7 +849,9 @@
                     bug
                     bugtasks
                     bugtracker
+                    can_be_rescheduled
                     datecreated
+                    failed_activity
                     getLastErrorMessage
                     hasComment
                     unpushed_comments
@@ -870,7 +872,8 @@
                 permission="launchpad.AnyPerson"
                 attributes="
                     destroySelf
-                    addActivity"
+                    addActivity
+                    setNextCheck"
                 set_attributes="bugtracker remotebug"/>
             <require
                 permission="launchpad.Admin"

=== modified file 'lib/lp/bugs/doc/bugwatch.txt'
--- lib/lp/bugs/doc/bugwatch.txt	2010-03-26 10:39:53 +0000
+++ lib/lp/bugs/doc/bugwatch.txt	2010-04-20 20:31:31 +0000
@@ -513,3 +513,87 @@
     >>> bug.removeWatch(bug_watch, factory.makePerson())
     >>> [bug_watch.remotebug for bug_watch in bug.watches]
     []
+
+
+Checking if a watch can be rescheduled
+--------------------------------------
+
+IBugWatch provides an attribute, can_be_rescheduled, which indicates
+whether or not the watch can be rescheduled. For a new bug watch this
+will be False.
+
+    >>> schedulable_watch = factory.makeBugWatch()
+    >>> schedulable_watch.next_check = None
+    >>> schedulable_watch.can_be_rescheduled
+    False
+
+If there's been activity on the watch but it's always been successful,
+can_be_rescheduled will be False.
+
+    >>> schedulable_watch.addActivity()
+    >>> schedulable_watch.can_be_rescheduled
+    False
+
+If the watch's updates have failed less than 60% of the time,
+can_be_rescheduled will be True
+
+    >>> from lp.bugs.interfaces.bugwatch import BugWatchActivityStatus
+    >>> schedulable_watch.addActivity(
+    ...     result=BugWatchActivityStatus.BUG_NOT_FOUND)
+    >>> schedulable_watch.can_be_rescheduled
+    True
+
+If the watch is rescheduled, can_be_rescheduled will be False, since the
+next_check time for the watch will be in the past (or in this case is
+now) and therefore it will be checked with the next checkwatches run.
+
+    >>> from pytz import utc
+    >>> from datetime import datetime
+    >>> schedulable_watch.next_check = datetime.now(utc)
+    >>> schedulable_watch.can_be_rescheduled
+    False
+
+However, if the watch has failed more than 60% of the time
+can_be_rescheduled will be False, since it's assumed that the watch
+needs attention in order for it to be able to work again.
+
+    >>> schedulable_watch.next_check = None
+    >>> schedulable_watch.addActivity(
+    ...     result=BugWatchActivityStatus.BUG_NOT_FOUND)
+    >>> schedulable_watch.can_be_rescheduled
+    False
+
+
+Rescheduling a watch
+--------------------
+
+The rescheduling of a watch is done via IBugWatch.setNextCheck(). This
+is to ensure that watches are only rescheduled when can_be_rescheduled
+is True (note that the BugWatch Scheduler bypasses setNextCheck() and
+sets next_check directly because it has admin privileges).
+
+The schedulable_watch that we used in the previous test cannot currently
+be rescheduled.
+
+    >>> schedulable_watch.can_be_rescheduled
+    False
+
+Calling setNextCheck() on this watch will cause an Exception,
+BugWatchCannotBeRescheduled, to be raised.
+
+    >>> schedulable_watch.setNextCheck(datetime.now(utc))
+    Traceback (most recent call last):
+      ...
+    BugWatchCannotBeRescheduled...
+
+If we add some activity to the watch, to make its can_be_rescheduled
+property become True, setNextCheck() will succeed.
+
+    >>> schedulable_watch.addActivity()
+    >>> schedulable_watch.can_be_rescheduled
+    True
+
+    >>> next_check = datetime.now(utc)
+    >>> schedulable_watch.setNextCheck(next_check)
+    >>> schedulable_watch.next_check == next_check
+    True

=== modified file 'lib/lp/bugs/interfaces/bugwatch.py'
--- lib/lp/bugs/interfaces/bugwatch.py	2010-04-16 14:50:46 +0000
+++ lib/lp/bugs/interfaces/bugwatch.py	2010-04-20 20:31:31 +0000
@@ -10,6 +10,7 @@
 __all__ = [
     'BUG_WATCH_ACTIVITY_SUCCESS_STATUSES',
     'BugWatchActivityStatus',
+    'BugWatchCannotBeRescheduled',
     'IBugWatch',
     'IBugWatchActivity',
     'IBugWatchSet',
@@ -210,6 +211,10 @@
         Text(title=_('The URL at which to view the remote bug.'),
              readonly=True))
 
+    can_be_rescheduled = Attribute(
+        "A True or False indicator of whether or not this watch can be "
+        "rescheduled.")
+
     def updateImportance(remote_importance, malone_importance):
         """Update the importance of the bug watch and any linked bug task.
 
@@ -250,6 +255,13 @@
     def addActivity(result=None, message=None, oops_id=None):
         """Add an `IBugWatchActivity` record for this BugWatch."""
 
+    def setNextCheck(next_check):
+        """Set the next_check time of the watch.
+
+        :raises: `BugWatchCannotBeRescheduled` if
+                 `IBugWatch.can_be_rescheduled` is False.
+        """
+
 
 # Defined here because of circular imports.
 IBugTracker['watches'].value_type.schema = IBugWatch
@@ -363,3 +375,6 @@
         title=_('OOPS ID'), readonly=True,
         description=_("The OOPS ID associated with this activity."))
 
+
+class BugWatchCannotBeRescheduled(Exception):
+    """The current `IBugWatch` can't be rescheduled."""

=== modified file 'lib/lp/bugs/model/bugwatch.py'
--- lib/lp/bugs/model/bugwatch.py	2010-04-16 14:50:46 +0000
+++ lib/lp/bugs/model/bugwatch.py	2010-04-20 20:31:31 +0000
@@ -12,6 +12,9 @@
 
 import re
 import urllib
+
+from datetime import datetime
+from pytz import utc
 from urlparse import urlunsplit
 
 from zope.event import notify
@@ -44,8 +47,9 @@
 
 from lp.bugs.interfaces.bugtracker import BugTrackerType, IBugTrackerSet
 from lp.bugs.interfaces.bugwatch import (
-    BugWatchActivityStatus, IBugWatch, IBugWatchActivity, IBugWatchSet,
-    NoBugTrackerFound, UnrecognizedBugTrackerURL)
+    BUG_WATCH_ACTIVITY_SUCCESS_STATUSES, BugWatchActivityStatus,
+    BugWatchCannotBeRescheduled, IBugWatch, IBugWatchActivity,
+    IBugWatchSet, NoBugTrackerFound, UnrecognizedBugTrackerURL)
 from lp.bugs.model.bugmessage import BugMessage
 from lp.bugs.model.bugset import BugSetBase
 from lp.bugs.model.bugtask import BugTask
@@ -66,6 +70,9 @@
     }
 
 
+WATCH_RESCHEDULE_THRESHOLD = 0.6
+
+
 class BugWatch(SQLBase):
     """See `IBugWatch`."""
     implements(IBugWatch)
@@ -307,6 +314,52 @@
             BugWatchActivity.bug_watch == self).order_by(
                 Desc('activity_date'))
 
+    @property
+    def can_be_rescheduled(self):
+        """See `IBugWatch`."""
+        if (self.next_check is not None and
+            self.next_check <= datetime.now(utc)):
+            # If the watch is already scheduled for a time in the past
+            # (or for right now) it can't be rescheduled, since it
+            # should be be checked by the next checkwatches run anyway.
+            return False
+
+        if self.activity.is_empty():
+            # Don't show the reschedule button if the watch has never
+            # been checked.
+            return False
+
+        if self.failed_activity.is_empty():
+            # Don't show the reschedule button if the watch has never
+            # failed.
+            return False
+
+        # If the ratio is lower than the reschedule threshold, we
+        # can show the button.
+        failure_ratio = (
+            float(self.failed_activity.count()) /
+            self.activity.count())
+        return failure_ratio <= WATCH_RESCHEDULE_THRESHOLD
+
+    @property
+    def failed_activity(self):
+        store = Store.of(self)
+        success_status_ids = [
+            status.value for status in BUG_WATCH_ACTIVITY_SUCCESS_STATUSES]
+
+        return store.find(
+            BugWatchActivity,
+            BugWatchActivity.bug_watch == self,
+            Not(In(BugWatchActivity.result, success_status_ids))).order_by(
+                Desc('activity_date'))
+
+    def setNextCheck(self, next_check):
+        """See `IBugWatch`."""
+        if not self.can_be_rescheduled:
+            raise BugWatchCannotBeRescheduled()
+
+        self.next_check = next_check
+
 
 class BugWatchSet(BugSetBase):
     """A set for BugWatch"""

=== modified file 'lib/lp/bugs/stories/bugwatches/xx-edit-bugwatch.txt'
--- lib/lp/bugs/stories/bugwatches/xx-edit-bugwatch.txt	2010-04-15 15:14:21 +0000
+++ lib/lp/bugs/stories/bugwatches/xx-edit-bugwatch.txt	2010-04-20 20:31:31 +0000
@@ -140,8 +140,8 @@
     ...     (bug_watch.bug.id, bug_watch.id))
     >>> logout()
 
-    >>> admin_browser.open(watch_url)
-    >>> admin_browser.getControl('Update Now')
+    >>> user_browser.open(watch_url)
+    >>> user_browser.getControl('Update Now')
     Traceback (most recent call last):
       ...
     LookupError: label 'Update Now'
@@ -153,8 +153,8 @@
     >>> bug_watch.addActivity()
     >>> logout()
 
-    >>> admin_browser.open(watch_url)
-    >>> admin_browser.getControl('Update Now')
+    >>> user_browser.open(watch_url)
+    >>> user_browser.getControl('Update Now')
     Traceback (most recent call last):
       ...
     LookupError: label 'Update Now'
@@ -166,11 +166,11 @@
     >>> bug_watch.addActivity(result=BugWatchActivityStatus.BUG_NOT_FOUND)
     >>> logout()
 
-    >>> admin_browser.open(watch_url)
-    >>> reschedule_button = admin_browser.getControl('Update Now')
+    >>> user_browser.open(watch_url)
+    >>> reschedule_button = user_browser.getControl('Update Now')
 
     >>> data_tag = find_tag_by_id(
-    ...     admin_browser.contents, 'bugwatch-next_check')
+    ...     user_browser.contents, 'bugwatch-next_check')
     >>> print extract_text(data_tag.renderContents())
     Next check: Not yet scheduled
 
@@ -180,22 +180,22 @@
     >>> reschedule_button.click()
 
     >>> for message in find_tags_by_class(
-    ...     admin_browser.contents, 'informational message'):
+    ...     user_browser.contents, 'informational message'):
     ...     print extract_text(message)
     The ... bug watch has been scheduled for immediate checking.
 
 Looking at the watch +edit page again, we can see that the watch has
 been scheduled.
 
-    >>> admin_browser.open(watch_url)
+    >>> user_browser.open(watch_url)
     >>> data_tag = find_tag_by_id(
-    ...     admin_browser.contents, 'bugwatch-next_check')
+    ...     user_browser.contents, 'bugwatch-next_check')
     >>> print extract_text(data_tag.renderContents())
     Next check: 2...
 
 The button will no longer be shown on the page.
 
-    >>> reschedule_button = admin_browser.getControl('Update Now')
+    >>> reschedule_button = user_browser.getControl('Update Now')
     Traceback (most recent call last):
       ...
     LookupError: label 'Update Now'

=== modified file 'lib/lp/bugs/templates/bugwatch-portlet-activity.pt'
--- lib/lp/bugs/templates/bugwatch-portlet-activity.pt	2010-04-15 15:14:21 +0000
+++ lib/lp/bugs/templates/bugwatch-portlet-activity.pt	2010-04-20 20:31:31 +0000
@@ -17,10 +17,10 @@
         <div>
           This watch has failed to update at
           <tal:fail-count
-              replace="view/failed_watch_activity_count" />
+              replace="context/failed_activity/count" />
           out of the last
           <tal:fail-count
-              replace="view/total_watch_activity_count" />
+              replace="context/activity/count" />
           attempts.
         </div>
         <div>