launchpad-reviewers team mailing list archive

[Graham Binns <graham@xxxxxxxxxxxxx>]

Graham Binns has proposed merging lp:~gmb/launchpad/dont-piss-the-losas-off-bug-596877 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers): code
Related bugs:
  #596877 BugTracker pages should have a button to reschedule all failing watches
  https://bugs.launchpad.net/bugs/596877


A button to reschedule all watches
==================================

This branch adds a button to the BugTracker +edit page which will
allow a user with sufficient privileges to reschedule all the watches on
a given BugTracker[1]. This is useful in situations where a bug tracker
has not been updated for a long period of time, so all the watches are
out of date. In the past we've done this by asking a LOSA to run some
SQL, but since that's a drain on their time and ours, a button seemed
more useful.

One of the problems associated with a large number of out-of-date
watches, however, is that checkwatches attempts to burn through them all
at once. This can lead to us adding a great deal of load to the remote
server, which is generally a Bad Thing and can get Launchpad blocked
from communicating with the upstream bug tracker (at the very least).

In order to obviate this problem, we have in the past used SQL that
would randomise the next_check time for each bug watch within a range of
up to twenty-four hours. This would then take some of the strain off the
remote server since checkwatches wouldn't be trying to update everything
as quickly as possible. It seemed sensible to use this same SQL when
creating the reschedule button. Note that 24 hours is the optimal
maximum time frame in which to schedule watches; anything longer means
that watches remain unchecked whilst other watches go out of date and
need to be checked again, potentially creating further load issues.

Finally, I cleaned up some lint in this branch, but I'm not wholly
certain that the new linter isn't getting some things wrong about our
coding standards.


Changes made
------------

lib/lp/bugs/browser/bugtracker.py
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

 - I've added an exception, UserCannotResetWatchesError, which is raised
   when a user attempts to all watches on a bug tracker without
   permission.
 - I've added a user_can_reset_watches property to BugTrackerEditView.
   This returns True if the user is either a Launchpad Admin or is a
   member of ~launchpad.
 - I've added a method, resetBugTrackerWatches() to BugTrackerEditView.
   This calls BugTracker.resetWatches(). I've made this a separate
   method so that it could be tested properly in view tests rather than
   putting all the implementation in the body of the action.
 - I've added a reschedule_action_condition() method to
   BugTrackerEditView. This returns true if there are watches to
   reschedule and user_can_reset_watches returns True. This is used to
   determine whether the reschedule button shows up on the +edit page.
 - I've added a rescheduleAction() method / action to
   BugTrackerEditView. This calls resetBugTrackerWatches() and adds an
   info notification to the page stating that all watches have been
   updated. Note that it makes no attempt to catch
   UserCannotResetWatchesErrors, since we want these to be dealt with by
   the OOPS machinery as they are indicative of someone trying to craft
   an HTTP POST to reschedule watches.


lib/lp/bugs/browser/tests/test_bugtracker_views.py
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

 - I've added tests to cover the resetBugTrackerWatches() method of
   BugTrackerEditView. The tests are designed to check that
   resetBugTrackerWatches() calls BugTracker.resetWatches() correctly
   and that it also checks that the user is allowed to reset a bug
   tracker's watches.


lib/lp/bugs/configure.zcml
~~~~~~~~~~~~~~~~~~~~~~~~~~

 - I've updated the ZCML and moved BugTracker.resetWatches() out of the
   launchpad.Admin section and into launchpad.Edit (hence the need for
   stricter checking of permissions in
   BugTrackerEditView.resetBugTrackerWatches()). This move was necessary
   to allow Launchpad Developers to reset watches.


lib/lp/bugs/doc/bugtracker.txt
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

 - I've updated the existing tests for BugTracker.resetWatches() to
   cover the new randomisation-over-24-hours code. I was tempted to
   refactor the whole of bugtracker.txt into unittest format, but
   decided against it for reasons of space and sanity.
 - I've fixed a bunch of lint issues with the doctest.


lib/lp/bugs/model/bugtracker.py
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

 - I've updated BugTracker.resetWatches() so that it now sets next_check
   to a random time between now and one day in the future, rather than
   just setting it to now as it used to.
 - I've also fixed a minor typo.


lib/lp/bugs/stories/bugtracker/xx-reschedule-all-watches.txt
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

 - This pagetest covers the Reschedule all watches button which will now
   appear on the BugTracker +edit page when it is viewed by admins or LP
   developers.


lib/lp/testing/factory.py
~~~~~~~~~~~~~~~~~~~~~~~~~

 - I updated makeBugTracker() so that it now accepts name and title
   arguments.


lib/lp/testing/sampledata.py
~~~~~~~~~~~~~~~~~~~~~~~~~~~~

 - I added constants for ADMIN_EMAIL (foo.bar@) and USER_EMAIL(test@).


Test commands
-------------

`bin/test -cvv -t /bugtracker.txt -t xx-reschedule-all-watches \
          -t test_bugtracker_views`

 [1] http://launchpad.net/bugs/596877

-- 
https://code.launchpad.net/~gmb/launchpad/dont-piss-the-losas-off-bug-596877/+merge/30186
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~gmb/launchpad/dont-piss-the-losas-off-bug-596877 into lp:launchpad.

=== modified file 'lib/lp/bugs/browser/bugtracker.py'
--- lib/lp/bugs/browser/bugtracker.py	2010-06-18 00:37:37 +0000
+++ lib/lp/bugs/browser/bugtracker.py	2010-07-17 19:22:56 +0000
@@ -61,6 +61,10 @@
         BugTrackerType.EMAILADDRESS,))
 
 
+class UserCannotResetWatchesError(Exception):
+    """Raised when a user cannot reset a bug tracker's watches."""
+
+
 class BugTrackerSetNavigation(GetitemNavigation):
 
     usedfor = IBugTrackerSet
@@ -371,6 +375,42 @@
     def cancel_url(self):
         return canonical_url(self.context)
 
+    @property
+    def user_can_reset_watches(self):
+        lp_developers = getUtility(ILaunchpadCelebrities).launchpad_developers
+        return (
+            check_permission("launchpad.Admin", self.user) or
+            self.user.inTeam(lp_developers))
+
+    def resetBugTrackerWatches(self):
+        """Call the resetWatches() method of the current context.
+
+        :raises `UserCannotResetWatchesError` if the user isn't an admin
+                or a member of the Launchpad Developers team.
+        """
+        if not self.user_can_reset_watches:
+            raise UserCannotResetWatchesError
+
+        self.context.resetWatches()
+
+    def reschedule_action_condition(self, action):
+        """Return True if the user can see the reschedule action."""
+        return (
+            self.context.watches.count() > 0 and
+            self.user_can_reset_watches)
+
+    @action(
+        'Reschedule all watches', name='reschedule',
+        condition=reschedule_action_condition)
+    def rescheduleAction(self, action, data):
+        """Reschedule all the watches for the bugtracker."""
+        self.resetBugTrackerWatches()
+        self.request.response.addInfoNotification(
+            structured(
+                "All bug watches on %s have been rescheduled." %
+                self.context.title))
+        self.next_url = canonical_url(self.context)
+
 
 class BugTrackerNavigation(Navigation):
 

=== added file 'lib/lp/bugs/browser/tests/test_bugtracker_views.py'
--- lib/lp/bugs/browser/tests/test_bugtracker_views.py	1970-01-01 00:00:00 +0000
+++ lib/lp/bugs/browser/tests/test_bugtracker_views.py	2010-07-17 19:22:56 +0000
@@ -0,0 +1,99 @@
+# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for BugTracker views."""
+
+__metaclass__ = type
+
+import unittest
+
+from datetime import datetime, timedelta
+from pytz import utc
+
+from zope.component import getUtility
+
+from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
+from canonical.testing.layers import DatabaseFunctionalLayer
+
+from lp.bugs.browser.bugtracker import (
+    BugTrackerEditView, UserCannotResetWatchesError)
+from lp.registry.interfaces.person import IPersonSet
+from lp.testing import login, TestCaseWithFactory
+from lp.testing.sampledata import ADMIN_EMAIL, NO_PRIVILEGE_EMAIL
+from lp.testing.views import create_initialized_view
+
+
+class BugTrackerEditViewTestCase(TestCaseWithFactory):
+    """A TestCase for the `BugTrackerEditView`."""
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        super(BugTrackerEditViewTestCase, self).setUp()
+        self.bug_tracker = self.factory.makeBugTracker()
+        for i in range(5):
+            self.factory.makeBugWatch(bugtracker=self.bug_tracker)
+
+        self.now = datetime.now(utc)
+
+    def _assertBugWatchesAreCheckedInTheFuture(self):
+        """Check the dates of all self.bug_tracker.watches.
+
+        Raise an error if:
+         * The next_check dates aren't in the future.
+         * The next_check dates aren't <= 1 day in the future.
+         * The lastcheck dates are not None
+         * The last_error_types are not None.
+        """
+        for watch in self.bug_tracker.watches:
+            self.assertTrue(
+                watch.next_check is not None,
+                "BugWatch next_check time should not be None.")
+            self.assertTrue(
+                watch.next_check >= self.now,
+                "BugWatch next_check time should be in the future.")
+            self.assertTrue(
+                watch.next_check <= self.now + timedelta(days=1),
+                "BugWatch next_check time should be one day or less in "
+                "the future.")
+            self.assertTrue(
+                watch.lastchecked is None,
+                "BugWatch lastchecked should be None.")
+            self.assertTrue(
+                watch.last_error_type is None,
+                "BugWatch last_error_type should be None.")
+
+    def test_unprivileged_user_cant_reset_watches(self):
+        # It isn't possible for a user who isn't an admin or a member of
+        # the Launchpad Developers team to reset the watches for a bug
+        # tracker.
+        login(NO_PRIVILEGE_EMAIL)
+        view = create_initialized_view(self.bug_tracker, '+edit')
+        self.assertRaises(
+            UserCannotResetWatchesError, view.resetBugTrackerWatches)
+
+    def test_admin_can_reset_watches(self):
+        # Launchpad admins can reset the watches on a bugtracker.
+        login(ADMIN_EMAIL)
+        view = create_initialized_view(self.bug_tracker, '+edit')
+        view.resetBugTrackerWatches()
+        self._assertBugWatchesAreCheckedInTheFuture()
+
+    def test_lp_dev_can_reset_watches(self):
+        # Launchpad developers can reset the watches on a bugtracker.
+        login(ADMIN_EMAIL)
+        admin = getUtility(IPersonSet).getByEmail(ADMIN_EMAIL)
+        launchpad_developers = getUtility(
+            ILaunchpadCelebrities).launchpad_developers
+
+        lp_dev = self.factory.makePerson()
+        launchpad_developers.addMember(lp_dev, admin)
+
+        login(lp_dev.preferredemail.email)
+        view = create_initialized_view(self.bug_tracker, '+edit')
+        view.resetBugTrackerWatches()
+        self._assertBugWatchesAreCheckedInTheFuture()
+
+
+def test_suite():
+    return unittest.TestLoader().loadTestsFromName(__name__)

=== modified file 'lib/lp/bugs/configure.zcml'
--- lib/lp/bugs/configure.zcml	2010-07-12 16:32:31 +0000
+++ lib/lp/bugs/configure.zcml	2010-07-17 19:22:56 +0000
@@ -385,7 +385,8 @@
                 attributes="
                     destroySelf
                     ensurePersonForSelf
-                    linkPersonToSelf"
+                    linkPersonToSelf
+                    resetWatches"
                 set_attributes="
                     aliases
                     baseurl
@@ -399,7 +400,6 @@
                     "/>
             <require
                 permission="launchpad.Admin"
-                attributes="resetWatches"
                 set_attributes="active"/>
         </class>
         <adapter

=== modified file 'lib/lp/bugs/doc/bugtracker.txt'
--- lib/lp/bugs/doc/bugtracker.txt	2010-03-26 13:48:53 +0000
+++ lib/lp/bugs/doc/bugtracker.txt	2010-07-17 19:22:56 +0000
@@ -611,10 +611,10 @@
     1 not now
 
 Calling resetWatches() on mozilla_bugzilla will set all its next_check
-times to datetime.now(). For testing purposes, we'll pass it a value to
-use in place of now().
+times. For testing purposes, we'll pass it a value to use in place of
+now().
 
-    >>> mozilla_bugzilla.resetWatches(now=definitely_now)
+    >>> mozilla_bugzilla.resetWatches(new_next_check=definitely_now)
     >>> transaction.commit()
     >>> print_tracker_watch_times(mozilla_bugzilla)
     2 now
@@ -622,7 +622,26 @@
     1 now
     1 now
 
-resetWatches() also clears the last_error_type field of the watches.
+By default, the bug watches next_check times are set to a random time
+between now and one day in the future so as to avoid big lumps of
+watches being checked at once. We can't test for randomness, but we can
+check that the times are roughly in the right ball park.
+
+    >>> mozilla_bugzilla.resetWatches()
+
+    >>> for watch in mozilla_bugzilla.watches:
+    ...     one_day_hence = definitely_now + timedelta(days=1)
+    ...     print (
+    ...         watch.next_check is not None and
+    ...         watch.next_check >= definitely_now and
+    ...         watch.next_check <= one_day_hence)
+    True
+    True
+    True
+    True
+
+resetWatches() also clears the last_error_type and lastchecked fields of
+the watches.
 
     >>> from lp.bugs.interfaces.bugwatch import BugWatchActivityStatus
     >>> for watch in mozilla_bugzilla.watches:
@@ -632,8 +651,8 @@
     >>> mozilla_bugzilla.resetWatches()
     >>> txn.commit()
     >>> for watch in mozilla_bugzilla.watches:
-    ...     print watch.last_error_type
-    None
-    None
-    None
-    None
+    ...     print watch.last_error_type, watch.lastchecked
+    None None
+    None None
+    None None
+    None None

=== modified file 'lib/lp/bugs/model/bugtracker.py'
--- lib/lp/bugs/model/bugtracker.py	2010-04-19 14:47:49 +0000
+++ lib/lp/bugs/model/bugtracker.py	2010-07-17 19:22:56 +0000
@@ -26,7 +26,7 @@
     BoolCol, ForeignKey, OR, SQLMultipleJoin, SQLObjectNotFound, StringCol)
 from sqlobject.sqlbuilder import AND
 
-from storm.expr import Count, Desc, Not
+from storm.expr import Count, Desc, Not, SQL
 from storm.locals import Bool
 from storm.store import Store
 
@@ -482,19 +482,20 @@
 
         return person
 
-    def resetWatches(self, now=None):
+    def resetWatches(self, new_next_check=None):
         """See `IBugTracker`."""
-        if now is None:
-            now = datetime.now(timezone('UTC'))
+        if new_next_check is None:
+            new_next_check = SQL(
+                "now() at time zone 'UTC' + (random() * interval '1 day')")
 
         store = Store.of(self)
-        store.execute(
-            "UPDATE BugWatch SET next_check = %s WHERE bugtracker = %s" %
-            sqlvalues(now, self))
+        store.find(BugWatch, BugWatch.bugtracker == self).set(
+            next_check=new_next_check, lastchecked=None,
+            last_error_type=None)
 
 
 class BugTrackerSet:
-    """Implements IBugTrackerSet for a container or set of BugTracker's,
+    """Implements IBugTrackerSet for a container or set of BugTrackers,
     either the full set in the db, or a subset.
     """
 

=== added file 'lib/lp/bugs/stories/bugtracker/xx-reschedule-all-watches.txt'
--- lib/lp/bugs/stories/bugtracker/xx-reschedule-all-watches.txt	1970-01-01 00:00:00 +0000
+++ lib/lp/bugs/stories/bugtracker/xx-reschedule-all-watches.txt	2010-07-17 19:22:56 +0000
@@ -0,0 +1,94 @@
+Rescheduling watches on a bugtracker
+====================================
+
+It's possible for all the watches on a bug tracker to be rescheduled, in
+much the same way as it's possible to reschedule a single bug watch that
+is failing to update.
+
+    >>> from canonical.launchpad.webapp import canonical_url
+    >>> from lp.testing.sampledata import ADMIN_EMAIL
+    >>> from lp.testing import login, login_person, logout
+
+    >>> login(ADMIN_EMAIL)
+    >>> bug_tracker = factory.makeBugTracker(
+    ...     name='our-bugtracker', title='Our BugTracker')
+    >>> bug_watch = factory.makeBugWatch(bugtracker=bug_tracker)
+    >>> logout()
+
+    >>> bug_tracker_edit_url = (
+    ...     canonical_url(bug_tracker) + '/+edit')
+
+The functionality to do this is available from the bug tracker's +edit
+page. It isn't visible to ordinary users, however.
+
+    >>> user_browser.open(bug_tracker_edit_url)
+    >>> user_browser.getControl('Reschedule all watches')
+    Traceback (most recent call last):
+      ...
+    LookupError: label 'Reschedule all watches'
+
+However, the reschedule button will appear to administrators.
+
+    >>> admin_browser.open(bug_tracker_edit_url)
+    >>> admin_browser.getControl('Reschedule all watches')
+    <SubmitControl...>
+
+It will also appear for non-admin members of the Launchpad Developers
+team.
+
+    >>> from zope.component import getUtility
+    >>> from canonical.launchpad.interfaces.launchpad import (
+    ...     ILaunchpadCelebrities)
+    >>> from lp.registry.interfaces.person import IPersonSet
+
+    >>> login(ADMIN_EMAIL)
+    >>> admin_user = getUtility(IPersonSet).getByEmail(ADMIN_EMAIL)
+    >>> new_lp_developer = factory.makePerson(password='newpassword')
+    >>> launchpad_developers = getUtility(
+    ...     ILaunchpadCelebrities).launchpad_developers
+    >>> dev_added = launchpad_developers.addMember(
+    ...     new_lp_developer, admin_user)
+
+    >>> lp_dev_browser = setupBrowser(
+    ...     auth='Basic %s:newpassword' %
+    ...         new_lp_developer.preferredemail.email)
+    >>> logout()
+
+    >>> lp_dev_browser.open(bug_tracker_edit_url)
+    >>> reschedule_button = lp_dev_browser.getControl(
+    ...     'Reschedule all watches')
+
+Clicking the button will reschedule the watches for the bug tracker for
+checking at some future date.
+
+    >>> reschedule_button.click()
+    >>> print lp_dev_browser.url
+    http://bugs.launchpad.dev/bugs/bugtrackers/our-bugtracker
+
+    >>> for message in find_tags_by_class(
+    ...     lp_dev_browser.contents, 'informational message'):
+    ...     print extract_text(message)
+    All bug watches on Our BugTracker have been rescheduled.
+
+If we look at the bug watch on our bugtracker we can see that it has
+been scheduled for checking at some point in the future.
+
+    >>> from datetime import datetime
+    >>> from pytz import utc
+
+    >>> login(ADMIN_EMAIL)
+    >>> print bug_watch.next_check >= datetime.now(utc)
+    True
+
+Should the bug watch be deleted the reschedule button will no longer
+appear on the bugtracker page.
+
+    >>> bug_watch.destroySelf()
+    >>> logout()
+
+    >>> lp_dev_browser.open(bug_tracker_edit_url)
+    >>> reschedule_button = lp_dev_browser.getControl(
+    ...     'Reschedule all watches')
+    Traceback (most recent call last):
+      ...
+    LookupError: label 'Reschedule all watches'

=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2010-07-16 18:15:44 +0000
+++ lib/lp/testing/factory.py	2010-07-17 19:22:56 +0000
@@ -1189,7 +1189,8 @@
 
         return bug.addTask(owner, target)
 
-    def makeBugTracker(self, base_url=None, bugtrackertype=None):
+    def makeBugTracker(self, base_url=None, bugtrackertype=None, title=None,
+                       name=None):
         """Make a new bug tracker."""
         owner = self.makePerson()
 
@@ -1199,7 +1200,7 @@
             bugtrackertype = BugTrackerType.BUGZILLA
 
         return getUtility(IBugTrackerSet).ensureBugTracker(
-            base_url, owner, bugtrackertype)
+            base_url, owner, bugtrackertype, title=title, name=name)
 
     def makeBugWatch(self, remote_bug=None, bugtracker=None, bug=None,
                      owner=None, bug_task=None):

=== modified file 'lib/lp/testing/sampledata.py'
--- lib/lp/testing/sampledata.py	2010-07-10 14:20:23 +0000
+++ lib/lp/testing/sampledata.py	2010-07-17 19:22:56 +0000
@@ -14,5 +14,7 @@
     ]
 
 
+ADMIN_EMAIL = 'foo.bar@xxxxxxxxxxxxx'
 NO_PRIVILEGE_EMAIL = 'no-priv@xxxxxxxxxxxxx'
+USER_EMAIL = 'test@xxxxxxxxxxxxx'
 VCS_IMPORTS_MEMBER_EMAIL = 'david.allouche@xxxxxxxxxxxxx'