Merge lp:~mwhudson/launchpad/read-only-xmlrpc-bug-403281 into lp:launchpad

[Michael Hudson-Doyle]

Don't add read-only warnings to requests that don't support notifications

Hi there.

This branch fixes bug 403281 which causes loads of oopses during rollouts.

As discussed, I check for failure to adapt by passing a default argument to the attempt to adapt and do nothing if adaption fails. I also remove a try:/except: block that *may* have been a botched attempt to do the same thing.

Cheers,
mwh

[Francis J. Lacoste (flacoste)]

Add comments to the tests and this is good to go!

review: Approve

[Francis J. Lacoste (flacoste)]

Can you land this on db-devel as release-critical now. That will save us some OOPSes during next roll-out :-)

review: Approve (release-critical)

=== modified file 'lib/canonical/launchpad/webapp/publication.py'
--- lib/canonical/launchpad/webapp/publication.py	2010-01-30 06:47:36 +0000
+++ lib/canonical/launchpad/webapp/publication.py	2010-03-04 23:03:29 +0000
@@ -34,7 +34,6 @@
 from zope.app.publication.interfaces import BeforeTraverseEvent
 from zope.app.security.interfaces import IUnauthenticatedPrincipal
 from zope.component import getUtility, queryMultiAdapter
-from zope.component.interfaces import ComponentLookupError
 from zope.error.interfaces import IErrorReportingUtility
 from zope.event import notify
 from zope.interface import implements, providedBy
@@ -210,8 +209,9 @@
     def maybeNotifyReadOnlyMode(self, request):
         """Hook to notify about read-only mode."""
         if is_read_only():
-            try:
-                INotificationResponse(request).addWarningNotification(
+            notification_response = INotificationResponse(request, None)
+            if notification_response is not None:
+                notification_response.addWarningNotification(
                     structured("""
                         Launchpad is undergoing maintenance and is in
                         read-only mode. <i>You cannot make any
@@ -219,8 +219,6 @@
                         href="http://blog.launchpad.net/maintenance">Launchpad
                         Blog</a> for details.
                         """))
-            except ComponentLookupError:
-                pass
 
     def getPrincipal(self, request):
         """Return the authenticated principal for this request.

=== modified file 'lib/canonical/launchpad/webapp/tests/test_publication.py'
--- lib/canonical/launchpad/webapp/tests/test_publication.py	2010-01-27 11:25:20 +0000
+++ lib/canonical/launchpad/webapp/tests/test_publication.py	2010-03-04 23:03:29 +0000
@@ -5,6 +5,7 @@
 
 __metaclass__ = type
 
+import logging
 import sys
 import unittest
 
@@ -33,7 +34,7 @@
     is_browser, LaunchpadBrowserPublication)
 from canonical.launchpad.webapp.servers import (
     LaunchpadTestRequest, WebServicePublication)
-from canonical.testing import DatabaseFunctionalLayer
+from canonical.testing import DatabaseFunctionalLayer, FunctionalLayer
 from lp.testing import TestCase, TestCaseWithFactory
 
 
@@ -61,6 +62,7 @@
         publication.callTraversalHooks(request, obj2)
         self.assertEquals(request.traversed_objects, [obj1])
 
+
 class TestReadOnlyModeSwitches(TestCase):
     # At the beginning of every request (in publication.beforeTraversal()), we
     # check to see if we've changed from/to read-only/read-write and if there
@@ -153,6 +155,37 @@
             master._connection._database.dsn_without_user.strip())
 
 
+class TestReadOnlyNotifications(TestCase):
+    """Tests for `LaunchpadBrowserPublication.maybeNotifyReadOnlyMode`."""
+
+    layer = FunctionalLayer
+
+    def setUp(self):
+        TestCase.setUp(self)
+        touch_read_only_file()
+        self.addCleanup(remove_read_only_file, assert_mode_switch=False)
+
+    def test_notification(self):
+        # In read-only mode, maybeNotifyReadOnlyMode adds a warning that
+        # changes cannot be made to every request that supports notifications.
+        publication = LaunchpadBrowserPublication(None)
+        request = LaunchpadTestRequest()
+        publication.maybeNotifyReadOnlyMode(request)
+        self.assertEqual(1, len(request.notifications))
+        notification = request.notifications[0]
+        self.assertEqual(logging.WARNING, notification.level)
+        self.assertTrue('read-only mode' in notification.message)
+
+    def test_notification_xmlrpc(self):
+        # Even in read-only mode, maybeNotifyReadOnlyMode doesn't try to add a
+        # notification to a request that doesn't support notifications.
+        from canonical.launchpad.webapp.servers import PublicXMLRPCRequest
+        publication = LaunchpadBrowserPublication(None)
+        request = PublicXMLRPCRequest(None, {})
+        # This is just assertNotRaises
+        publication.maybeNotifyReadOnlyMode(request)
+
+
 class TestWebServicePublication(TestCaseWithFactory):
     layer = DatabaseFunctionalLayer