Launchpad itself

Merge lp://qastaging/~gmb/launchpad/dont-piss-the-losas-off-bug-596877 into lp://qastaging/launchpad/db-devel

dont-piss-the-losas-off-bug-596877
Merge into db-devel

Proposed by Graham Binns on 2010-07-17

Status:

Merged

Merged at revision:

9700

Proposed branch:

lp://qastaging/~gmb/launchpad/dont-piss-the-losas-off-bug-596877

Merge into:

lp://qastaging/launchpad/db-devel

Diff against target:

656 lines (+299/-89)

12 files modified

lib/canonical/launchpad/security.py (+11/-0)
lib/lp/bugs/browser/bugtracker.py (+24/-2)
lib/lp/bugs/browser/tests/test_bugtracker_views.py (+29/-0)
lib/lp/bugs/configure.zcml (+2/-1)
lib/lp/bugs/doc/bugtracker.txt (+10/-60)
lib/lp/bugs/interfaces/bugtracker.py (+5/-4)
lib/lp/bugs/model/bugtracker.py (+13/-8)
lib/lp/bugs/scripts/checkwatches/core.py (+2/-1)
lib/lp/bugs/stories/bugtracker/xx-reschedule-all-watches.txt (+94/-0)
lib/lp/bugs/tests/test_bugtracker.py (+97/-11)
lib/lp/testing/factory.py (+3/-2)
lib/lp/testing/sampledata.py (+9/-0)

To merge this branch:

bzr merge lp://qastaging/~gmb/launchpad/dont-piss-the-losas-off-bug-596877

High

Fix Released

Link a bug report

Reviewer	Date Requested	Status
Gavin Panella (community)	2010-07-17	Approve on 2010-08-25
Robert Collins (community)		Needs Information on 2010-07-20
Review via email: mp+30186@code.qastaging.launchpad.net

Commit message

A button, visible to admins and LP developers, has been added to the BugTracker +edit page to allow all of a tracker's watches to be rescheduled en masse.

Description of the change

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

Revision history for this message

Robert Collins (lifeless) wrote on 2010-07-19:

Shouldn't the permission check be in the model? the LBYP idiom seems likely to miss things...

review: Needs Information

Revision history for this message

Robert Collins (lifeless) wrote on 2010-07-19:

Also as discussed there are no particularly special permission issues here so raising the normal permission denied stuff should be all thats needed.

review: Needs Fixing

Revision history for this message

Graham Binns (gmb) wrote on 2010-07-20:

I've updated the branch per our discussions. Here comes the diff:

from canonical.cachedproperty import cachedproperty
from canonical.database.sqlbase import flush_database_updates
@@ -63,10 +64,6 @@
)

-class UserCannotResetWatchesError(Exception):
- """Raised when a user cannot reset a bug tracker's watches."""
-
-
class BugTrackerSetNavigation(GetitemNavigation):

     usedfor = IBugTrackerSet
@@ -387,11 +384,13 @@
     def resetBugTrackerWatches(self):
         """Call the resetWatches() method of the current context.

- :raises `UserCannotResetWatchesError` if the user isn't an admin
+ :raises `Unauthorized` if the user isn't an admin
                 or a member of the Launchpad Developers team.
         """
         if not self.user_can_reset_watches:
- raise UserCannotResetWatchesError
+ raise Unauthorized(
+ "You don't have permission to reset all the watches for "
+ "this bug tracker.")

self.context.resetWatches()

from zope.component import getUtility
+from zope.security.interfaces import Unauthorized

from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
from canonical.testing.layers import DatabaseFunctionalLayer

from lp.bugs.browser.bugtracker import (
- BugTrackerEditView, UserCannotResetWatchesError)
+ BugTrackerEditView)
from lp.registry.interfaces.person import IPersonSet
from lp.testing import login, TestCaseWithFactory
from lp.testing.sampledata import ADMIN_EMAIL, NO_PRIVILEGE_EMAIL
@@ -70,7 +71,7 @@
         login(NO_PRIVILEGE_EMAIL)
         view = create_initialized_view(self.bug_tracker, '+edit')
         self.assertRaises(
- UserCannotResetWatchesError, view.resetBugTrackerWatches)
+ Unauthorized, view.resetBugTrackerWatches)

def test_admin_can_reset_watches(self):
# Launchpad admins can reset the watches on a bugtracker.

I've updated the branch per our discussions. Here comes the diff:

=== modified file 'lib/lp/bugs/browser/bugtracker.py'
--- lib/lp/bugs/browser/bugtracker.py	2010-07-17 19:19:16 +0000
+++ lib/lp/bugs/browser/bugtracker.py	2010-07-20 16:17:59 +0000
@@ -27,6 +27,7 @@
 from zope.formlib import form
 from zope.schema import Choice
 from zope.schema.vocabulary import SimpleVocabulary
+from zope.security.interfaces import Unauthorized
 
 from canonical.cachedproperty import cachedproperty
 from canonical.database.sqlbase import flush_database_updates
@@ -63,10 +64,6 @@
     )
 
 
-class UserCannotResetWatchesError(Exception):
-    """Raised when a user cannot reset a bug tracker's watches."""
-
-
 class BugTrackerSetNavigation(GetitemNavigation):
 
     usedfor = IBugTrackerSet
@@ -387,11 +384,13 @@
     def resetBugTrackerWatches(self):
         """Call the resetWatches() method of the current context.
 
-        :raises `UserCannotResetWatchesError` if the user isn't an admin
+        :raises `Unauthorized` if the user isn't an admin
                 or a member of the Launchpad Developers team.
         """
         if not self.user_can_reset_watches:
-            raise UserCannotResetWatchesError
+            raise Unauthorized(
+                "You don't have permission to reset all the watches for "
+                "this bug tracker.")
 
         self.context.resetWatches()

=== modified file 'lib/lp/bugs/browser/tests/test_bugtracker_views.py'
--- lib/lp/bugs/browser/tests/test_bugtracker_views.py	2010-07-17 16:59:12 +0000
+++ lib/lp/bugs/browser/tests/test_bugtracker_views.py	2010-07-20 16:17:59 +0000
@@ -11,12 +11,13 @@
 from pytz import utc
 
 from zope.component import getUtility
+from zope.security.interfaces import Unauthorized
 
 from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
 from canonical.testing.layers import DatabaseFunctionalLayer
 
 from lp.bugs.browser.bugtracker import (
-    BugTrackerEditView, UserCannotResetWatchesError)
+    BugTrackerEditView)
 from lp.registry.interfaces.person import IPersonSet
 from lp.testing import login, TestCaseWithFactory
 from lp.testing.sampledata import ADMIN_EMAIL, NO_PRIVILEGE_EMAIL
@@ -70,7 +71,7 @@
         login(NO_PRIVILEGE_EMAIL)
         view = create_initialized_view(self.bug_tracker, '+edit')
         self.assertRaises(
-            UserCannotResetWatchesError, view.resetBugTrackerWatches)
+            Unauthorized, view.resetBugTrackerWatches)
 
     def test_admin_can_reset_watches(self):
         # Launchpad admins can reset the watches on a bugtracker.

Revision history for this message

Robert Collins (lifeless) wrote on 2010-07-20:

Thanks for improving it.

review: Approve

Revision history for this message

Robert Collins (lifeless) wrote on 2010-07-20:

Sorry, I missed that its still in the view. Shouldn't it be in the model?

review: Needs Information

Revision history for this message

Graham Binns (gmb) wrote on 2010-07-23:

Download full text (12.8 KiB)

It should, and I've moved it there (for some reason I had it in my head that we should always check stuff in the view, but that turns out not to be true).

Here's a diff:

- @property
- def user_can_reset_watches(self):
+ def reschedule_action_condition(self, action):
+ """Return True if the user can see the reschedule action."""
         lp_developers = getUtility(ILaunchpadCelebrities).launchpad_developers
- return (
+ user_can_reset_watches = (
             check_permission("launchpad.Admin", self.user) or
             self.user.inTeam(lp_developers))
-
- def resetBugTrackerWatches(self):
- """Call the resetWatches() method of the current context.
-
- :raises `Unauthorized` if the user isn't an admin
- or a member of the Launchpad Developers team.
- """
- if not self.user_can_reset_watches:
- raise Unauthorized(
- "You don't have permission to reset all the watches for "
- "this bug tracker.")
-
- 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)
+ 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.context.resetWatches(user=self.user)
         self.request.response.addInfoNotification(
             structured(
                 "All bug watches on %s have been rescheduled." %

-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 i...

It should, and I've moved it there (for some reason I had it in my head that we should always check stuff in the view, but that turns out not to be true).

Here's a diff:

=== modified file 'lib/lp/bugs/browser/bugtracker.py'
--- lib/lp/bugs/browser/bugtracker.py	2010-07-20 16:17:59 +0000
+++ lib/lp/bugs/browser/bugtracker.py	2010-07-23 11:11:13 +0000
@@ -374,38 +374,22 @@
     def cancel_url(self):
         return canonical_url(self.context)
 
-    @property
-    def user_can_reset_watches(self):
+    def reschedule_action_condition(self, action):
+        """Return True if the user can see the reschedule action."""
         lp_developers = getUtility(ILaunchpadCelebrities).launchpad_developers
-        return (
+        user_can_reset_watches = (
             check_permission("launchpad.Admin", self.user) or
             self.user.inTeam(lp_developers))
-
-    def resetBugTrackerWatches(self):
-        """Call the resetWatches() method of the current context.
-
-        :raises `Unauthorized` if the user isn't an admin
-                or a member of the Launchpad Developers team.
-        """
-        if not self.user_can_reset_watches:
-            raise Unauthorized(
-                "You don't have permission to reset all the watches for "
-                "this bug tracker.")
-
-        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)
+            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.context.resetWatches(user=self.user)
         self.request.response.addInfoNotification(
             structured(
                 "All bug watches on %s have been rescheduled." %

=== modified file 'lib/lp/bugs/browser/tests/test_bugtracker_views.py'
--- lib/lp/bugs/browser/tests/test_bugtracker_views.py	2010-07-20 16:17:59 +0000
+++ lib/lp/bugs/browser/tests/test_bugtracker_views.py	2010-07-23 11:11:13 +0000
@@ -24,77 +24,6 @@
 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(
-            Unauthorized, 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/model/bugtracker.py'
--- lib/lp/bugs/model/bugtracker.py	2010-07-17 19:19:16 +0000
+++ lib/lp/bugs/model/bugtracker.py	2010-07-23 11:11:13 +0000
@@ -21,6 +21,7 @@
 
 from zope.component import getUtility
 from zope.interface import implements
+from zope.security.interfaces import Unauthorized
 
 from sqlobject import (
     BoolCol, ForeignKey, OR, SQLMultipleJoin, SQLObjectNotFound, StringCol)
@@ -482,8 +483,20 @@
 
         return person
 
-    def resetWatches(self, new_next_check=None):
+    def resetWatches(self, user, new_next_check=None):
         """See `IBugTracker`."""
+        celebrities = getUtility(ILaunchpadCelebrities)
+        launchpad_developers = celebrities.launchpad_developers
+        admin_team = celebrities.admin
+        janitor = celebrities.janitor
+
+        if (not user.inTeam(admin_team) and
+            not user.inTeam(launchpad_developers) and
+            user != janitor):
+            raise Unauthorized(
+                "You do not have permission to reset the watches for "
+                "this bug tracker.")
+
         if new_next_check is None:
             new_next_check = SQL(
                 "now() at time zone 'UTC' + (random() * interval '1 day')")

=== modified file 'lib/lp/bugs/scripts/checkwatches/core.py'
--- lib/lp/bugs/scripts/checkwatches/core.py	2010-07-14 09:13:59 +0000
+++ lib/lp/bugs/scripts/checkwatches/core.py	2010-07-23 11:11:13 +0000
@@ -288,7 +288,8 @@
             self.logger.info(
                 "Resetting %s bug watches for bug tracker '%s'" %
                 (bug_tracker.watches.count(), bug_tracker_name))
-            bug_tracker.resetWatches()
+            bug_tracker.resetWatches(
+                user=getUtility(ILaunchpadCelebrities).janitor)
 
         # Loop over the bug watches in batches as specificed by
         # batch_size until there are none left to update.

=== modified file 'lib/lp/bugs/tests/test_bugtracker.py'
--- lib/lp/bugs/tests/test_bugtracker.py	2010-04-12 09:51:59 +0000
+++ lib/lp/bugs/tests/test_bugtracker.py	2010-07-23 11:11:13 +0000
@@ -3,15 +3,14 @@
 
 __metaclass__ = type
 
+import transaction
 import unittest
 from urllib2 import HTTPError, Request
-
 from datetime import datetime, timedelta
-
 from pytz import utc
 
-import transaction
-
+from zope.component import getUtility
+from zope.security.interfaces import Unauthorized
 from zope.security.proxy import removeSecurityProxy
 from zope.testing.doctest import NORMALIZE_WHITESPACE, ELLIPSIS
 from zope.testing.doctestunit import DocTestSuite
@@ -19,21 +18,30 @@
 from lazr.lifecycle.snapshot import Snapshot
 
 from canonical.launchpad.ftests import login_person
+from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
 from canonical.testing import LaunchpadFunctionalLayer
 
 from lp.bugs.externalbugtracker import (
     BugTrackerConnectError, Mantis, MantisLoginHandler)
 from lp.bugs.interfaces.bugtracker import BugTrackerType, IBugTracker
 from lp.bugs.tests.externalbugtracker import Urlib2TransportTestHandler
-from lp.testing import TestCaseWithFactory
-
-
-class TestBugTracker(TestCaseWithFactory):
+from lp.registry.interfaces.person import IPersonSet
+from lp.testing import login, login_person, TestCaseWithFactory
+from lp.testing.sampledata import ADMIN_EMAIL
+
+
+class BugTrackerTestCase(TestCaseWithFactory):
+    """Unit tests for the `BugTracker` class."""
+
     layer = LaunchpadFunctionalLayer
 
     def setUp(self):
-        super(TestBugTracker, self).setUp()
-        transaction.commit()
+        super(BugTrackerTestCase, 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 test_multi_product_constraints_observed(self):
         """BugTrackers for which multi_product=True should return None
@@ -131,6 +139,78 @@
         removeSecurityProxy(bug_message).remote_comment_id = 'brains'
         self.failUnless(bug_tracker.watches_with_unpushed_comments.is_empty())
 
+    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.
+        unprivileged_user = self.factory.makePerson()
+        login_person(unprivileged_user)
+        self.assertRaises(
+            Unauthorized, self.bug_tracker.resetWatches,
+            user=unprivileged_user)
+
+    def test_admin_can_reset_watches(self):
+        # Launchpad admins can reset the watches on a bugtracker.
+        admin_user = getUtility(IPersonSet).getByEmail(ADMIN_EMAIL)
+        login_person(admin_user)
+        self.bug_tracker.resetWatches(user=admin_user)
+        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_person(lp_dev)
+        self.bug_tracker.resetWatches(user=lp_dev)
+        self._assertBugWatchesAreCheckedInTheFuture()
+
+    def test_janitor_can_reset_watches(self):
+        # The Janitor can reset the watches on a bug tracker.
+        janitor = getUtility(ILaunchpadCelebrities).janitor
+        login_person(janitor)
+        self.bug_tracker.resetWatches(user=janitor)
+
+
+class TestMantis(TestCaseWithFactory):
+    """Tests for the Mantis-specific bug tracker code."""
+
+    layer = LaunchpadFunctionalLayer
+
+    def setUp(self):
+        super(TestMantis, self).setUp()
+        transaction.commit()
+
     def test_mantis_login_redirects(self):
         # The Mantis bug tracker needs a special HTTP redirect handler
         # in order to login in. Ensure that redirects to the page with
@@ -220,11 +300,9 @@
 
 
 def test_suite():
-    suite = unittest.TestSuite()
+    suite = unittest.TestLoader().loadTestsFromName(__name__)
     doctest_suite = DocTestSuite(
         'lp.bugs.model.bugtracker',
         optionflags=NORMALIZE_WHITESPACE|ELLIPSIS)
-
-    suite.addTest(unittest.makeSuite(TestBugTracker))
     suite.addTest(doctest_suite)
     return suite

Revision history for this message

Gavin Panella (allenap) wrote on 2010-08-11:

This branch is a really cool addition. I think it can be simplified by
leaning more heavily on Zope security, and there are a few other
things to consider.

I wrote the review notes in the Landscape style before I remembered
that this isn't Landscape. However, I think it's quite readable,
especially since responses need only reference the number given to
each comment, so I'll leave it. I hope you don't mind. Let me know
what you think.

[1]

+ user_can_reset_watches = (
+ check_permission("launchpad.Admin", self.user) or
+ self.user.inTeam(lp_developers))

This would perhaps be better placed on the model, something like
canResetWatches(user).

[2]

+ check_permission("launchpad.Admin", self.user) or

I may well be wrong, but this looks like it checks if the current user
has the launchpad.Admin permission on self.user, which will always be
true.

I think it might be worth defining a new AdminBugTracker authorization
adapter in canonical.launchpad.security, something like:

class AdminBugTracker(AuthorizationBase):
permission = 'launchpad.Admin'
usedfor = IBugTracker

    def checkAuthenticated(self, user):
        return (user.in_admin or
                user.in_launchpad_developers or
                user.in_janitor)

Then the changes to lib/lp/bugs/configure.zcml can be reverted.

[3]

=== added file 'lib/lp/bugs/browser/tests/test_bugtracker_views.py'

There are no tests in this file! :)

[4]

I /think/ it's usual to indent the second and subsequent lines by 4
spaces. Doesn't matter much though.

[5]

+ def resetWatches(self, user, new_next_check=None):
"""See `IBugTracker`."""
- if now is None:
- now = datetime.now(timezone('UTC'))
+ celebrities = getUtility(ILaunchpadCelebrities)
+ launchpad_developers = celebrities.launchpad_developers
+ admin_team = celebrities.admin
+ janitor = celebrities.janitor
+
+ if (not user.inTeam(admin_team) and
+ not user.inTeam(launchpad_developers) and
+ user != janitor):
+ raise Unauthorized(
+ "You do not have permission to reset the watches for "
+ "this bug tracker.")

If you add the security adapter in [2] then the user checking stuff
here can be dropped.

[6]

+ self.request.response.addInfoNotification(
+ structured(
+ "All bug watches on %s have been rescheduled." %
+ self.context.title))

I don't think this should be a structured() string; that would allow
markup/script injection via the bug tracker title.

This branch is a really cool addition. I think it can be simplified by
leaning more heavily on Zope security, and there are a few other
things to consider.

[1]

+        user_can_reset_watches = (
+            check_permission("launchpad.Admin", self.user) or
+            self.user.inTeam(lp_developers))

This would perhaps be better placed on the model, something like
canResetWatches(user).

[2]

+            check_permission("launchpad.Admin", self.user) or

I may well be wrong, but this looks like it checks if the current user
has the launchpad.Admin permission on self.user, which will always be
true.

I think it might be worth defining a new AdminBugTracker authorization
adapter in canonical.launchpad.security, something like:

class AdminBugTracker(AuthorizationBase):
    permission = 'launchpad.Admin'
    usedfor = IBugTracker

def checkAuthenticated(self, user):
        return (user.in_admin or
                user.in_launchpad_developers or
                user.in_janitor)

Then the changes to lib/lp/bugs/configure.zcml can be reverted.

[3]

=== added file 'lib/lp/bugs/browser/tests/test_bugtracker_views.py'

There are no tests in this file! :)

[4]

+        :param new_next_check: If specified, contains the datetime to
+                               which to set the BugWatches' next_check times.
+                               If not specified, the watches' next_check
+                               times will be set to a point between now
+                               and 24 hours hence.

I /think/ it's usual to indent the second and subsequent lines by 4
spaces. Doesn't matter much though.

[5]

+    def resetWatches(self, user, new_next_check=None):
         """See `IBugTracker`."""
-        if now is None:
-            now = datetime.now(timezone('UTC'))
+        celebrities = getUtility(ILaunchpadCelebrities)
+        launchpad_developers = celebrities.launchpad_developers
+        admin_team = celebrities.admin
+        janitor = celebrities.janitor
+
+        if (not user.inTeam(admin_team) and
+            not user.inTeam(launchpad_developers) and
+            user != janitor):
+            raise Unauthorized(
+                "You do not have permission to reset the watches for "
+                "this bug tracker.")

If you add the security adapter in [2] then the user checking stuff
here can be dropped.

[6]

+        self.request.response.addInfoNotification(
+            structured(
+                "All bug watches on %s have been rescheduled." %
+                self.context.title))

I don't think this should be a structured() string; that would allow
markup/script injection via the bug tracker title.

review: Needs Fixing

Revision history for this message

Graham Binns (gmb) wrote on 2010-08-25:

Download full text (5.6 KiB)

Diff of changes:

=== modified file 'lib/canonical/launchpad/security.py'
--- lib/canonical/launchpad/security.py 2010-08-22 18:31:30 +0000
+++ lib/canonical/launchpad/security.py 2010-08-25 15:59:28 +0000
@@ -2561,3 +2561,14 @@
         if parent is None:
             return False
         return check_permission(self.permission, parent)
+
+
+class AdminBugTracker(AuthorizationBase):
+ permission = 'launchpad.Admin'
+ usedfor = IBugTracker
+
+ def checkAuthenticated(self, user):
+ return (
+ user.in_janitor or
+ user.in_admin or
+ user.in_launchpad_developers)

     def reschedule_action_condition(self, action):
         """Return True if the user can see the reschedule action."""
- lp_developers = getUtility(ILaunchpadCelebrities).launchpad_developers
- user_can_reset_watches = (
- check_permission("launchpad.Admin", self.user) or
- self.user.inTeam(lp_developers))
+ user_can_reset_watches = check_permission(
+ "launchpad.Admin", self.context)
         return (
             self.context.watches.count() > 0 and
             user_can_reset_watches)

=== modified file 'lib/lp/bugs/configure.zcml'
--- lib/lp/bugs/configure.zcml 2010-08-25 10:45:59 +0000
+++ lib/lp/bugs/configure.zcml 2010-08-25 15:59:28 +0000
@@ -394,7 +394,7 @@
                     destroySelf
                     ensurePersonForSelf
                     linkPersonToSelf
- resetWatches"
+ "
                 set_attributes="
                     aliases
                     baseurl
@@ -408,6 +408,7 @@
                     "/>
             <require
                 permission="launchpad.Admin"
+ attributes="resetWatches"
                 set_attributes="active"/>
         </class>
         <adapter

- def resetWatches(user, new_next_check=None):
+ def resetWatches(new_next_check=None):
"""Reset the next_check times of this BugTracker's `BugWatch`es.

:param new_next_check: If specified, contains the datetime to
- which to set the BugWatches' next_check times.
- If not specified, the watches' next_check
- times will be set to a point between now
- and 24 hours hence.
+ which to set the BugWatches' next_check times. If not
+ specified, the watches' next_check times will be set to a
+ point between now and 24 hours hence.
"""

=== modified file 'lib/lp/bugs/model/bugtracker.py'
--- lib/lp/bugs/model/bugtracker.py 2010-08-25 10:45:59 +0000
+++ lib/lp/bugs/model/bugt...