launchpad-reviewers team mailing list archive

[Graham Binns <graham@xxxxxxxxxxxxx>]

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

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