Merge lp:~rvb/maas/maas-longpoll-fix into lp:~maas-committers/maas/trunk

[Raphaël Badin]

Fix proxy to txlongpoll.

This branch fixes the dynamic inclusion of the view that acts as a proxy to a txongpoll server. This view should only be served by MaaS if both settings.LONGPOLL_SERVER_URL and settings.LONGPOLL_URL and defined (i.e. if longpoll is enabled (LONGPOLL_URL) and if the url to a txlongpoll is configured (LONGPOLL_SERVER_URL).

[Jeroen T. Vermeulen (jtv)]

review: Approve

[Francis J. Lacoste (flacoste)]

Hi Raphaël,

Thank you very much for this fix, and the clarification of LONGPOLL_PATH, and the additional tests.

But unless I'm reading things incorrectly, we still don't have any tests that ensure that the view is setup correctly, right? We have tests get get_proxy_longpoll_enabled() is implemented correctly, but no tests that ensure that the proxy view is set up correctly? I assume we have unit tests that the view itself work properly.

[Raphaël Badin (rvb)]

The unit tests for the view itself are there: https://code.launchpad.net/~rvb/maas/maas-add-proxy-test/+merge/97593.

I've extracted the method get_proxy_longpoll_enabled only to test that the proxy view is setup correctly. But the tiny piece of code that sets up the view itself, I've no idea how to properly test that…

=== modified file 'src/maas/demo.py'
--- src/maas/demo.py	2012-03-15 10:27:10 +0000
+++ src/maas/demo.py	2012-03-15 11:30:24 +0000
@@ -29,7 +29,7 @@
 
 # Disable longpoll by default for now. Set it back to 'longpoll/' to
 # enable it.
-LONGPOLL_URL = None
+LONGPOLL_PATH = None
 
 # This should match the setting in /etc/pserv.yaml.
 PSERV_URL = "http://localhost:8001/api"

=== modified file 'src/maas/settings.py'
--- src/maas/settings.py	2012-03-14 18:39:21 +0000
+++ src/maas/settings.py	2012-03-15 11:30:24 +0000
@@ -53,7 +53,7 @@
 
 # The relative path where a proxy to the Longpoll server can be
 # reached.  Longpolling will be disabled in the UI if this is None.
-LONGPOLL_URL = 'longpoll/'
+LONGPOLL_PATH = 'longpoll/'
 
 # Default URL specifying protocol, host, and (if necessary) port where
 # this MaaS can be found.  Configuration can, and probably should,
@@ -65,7 +65,7 @@
     LOGOUT_URL = FORCE_SCRIPT_NAME + LOGOUT_URL
     LOGIN_REDIRECT_URL = FORCE_SCRIPT_NAME + LOGIN_REDIRECT_URL
     LOGIN_URL = FORCE_SCRIPT_NAME + LOGIN_URL
-    LONGPOLL_URL = FORCE_SCRIPT_NAME + LONGPOLL_URL
+    LONGPOLL_PATH = FORCE_SCRIPT_NAME + LONGPOLL_PATH
     DEFAULT_MAAS_URL = urljoin(DEFAULT_MAAS_URL, FORCE_SCRIPT_NAME)
     # ADMIN_MEDIA_PREFIX will be deprecated in Django 1.4.
     # Admin's media will be served using staticfiles instead.

=== modified file 'src/maasserver/templates/maasserver/index.html'
--- src/maasserver/templates/maasserver/index.html	2012-03-13 17:13:01 +0000
+++ src/maasserver/templates/maasserver/index.html	2012-03-15 11:30:24 +0000
@@ -50,10 +50,10 @@
       });
 
       // Start longpoll.
-      {% if longpoll_queue and LONGPOLL_URL %}
+      {% if longpoll_queue and LONGPOLL_PATH %}
         Y.later(0, Y.maas.longpoll, function() {
           var longpollmanager = Y.maas.longpoll.setupLongPollManager(
-            '{{ longpoll_queue }}', '/{{ LONGPOLL_URL }}');
+            '{{ longpoll_queue }}', '/{{ LONGPOLL_PATH }}');
         });
       {% endif %}
     });

=== modified file 'src/maasserver/tests/test_views.py'
--- src/maasserver/tests/test_views.py	2012-03-15 09:09:38 +0000
+++ src/maasserver/tests/test_views.py	2012-03-15 11:30:24 +0000
@@ -33,6 +33,7 @@
     LoggedInTestCase,
     TestCase,
     )
+from maasserver.urls import get_proxy_longpoll_enabled
 from maasserver.views import (
     get_longpoll_context,
     proxy_to_longpoll,
@@ -147,6 +148,24 @@
         self.assertEqual(status_code, response.status_code)
 
 
+class TestGetLongpollenabled(TestCase):
+
+    def test_longpoll_not_included_if_LONGPOLL_SERVER_URL_None(self):
+        self.patch(settings, 'LONGPOLL_PATH', factory.getRandomString())
+        self.patch(settings, 'LONGPOLL_SERVER_URL', None)
+        self.assertFalse(get_proxy_longpoll_enabled())
+
+    def test_longpoll_not_included_if_LONGPOLL_PATH_None(self):
+        self.patch(settings, 'LONGPOLL_PATH', None)
+        self.patch(settings, 'LONGPOLL_SERVER_URL', factory.getRandomString())
+        self.assertFalse(get_proxy_longpoll_enabled())
+
+    def test_longpoll_included_if_LONGPOLL_PATH_and_LONGPOLL_SERVER_URL(self):
+        self.patch(settings, 'LONGPOLL_PATH', factory.getRandomString())
+        self.patch(settings, 'LONGPOLL_SERVER_URL', factory.getRandomString())
+        self.assertTrue(get_proxy_longpoll_enabled())
+
+
 class TestComboLoaderView(TestCase):
     """Test combo loader view."""
 
@@ -190,30 +209,25 @@
 
 class TestUtilities(TestCase):
 
-    def patch_settings(self, name, value):
-        old_value = getattr(settings, name)
-        setattr(settings, name, value)
-        self.addCleanup(setattr, settings, name, old_value)
-
     def test_get_longpoll_context_empty_if_rabbitmq_publish_is_none(self):
         self.patch(settings, 'RABBITMQ_PUBLISH', None)
         self.patch(views, 'messaging', get_messaging())
         self.assertEqual({}, get_longpoll_context())
 
     def test_get_longpoll_context_empty_if_longpoll_url_is_None(self):
-        self.patch(settings, 'LONGPOLL_URL', None)
+        self.patch(settings, 'LONGPOLL_PATH', None)
         self.patch(views, 'messaging', get_messaging())
         self.assertEqual({}, get_longpoll_context())
 
     def test_get_longpoll_context(self):
         longpoll = factory.getRandomString()
-        self.patch(settings, 'LONGPOLL_URL', longpoll)
+        self.patch(settings, 'LONGPOLL_PATH', longpoll)
         self.patch(settings, 'RABBITMQ_PUBLISH', True)
         self.patch(views, 'messaging', get_messaging())
         context = get_longpoll_context()
         self.assertItemsEqual(
-            ['LONGPOLL_URL', 'longpoll_queue'], list(context))
-        self.assertEqual(longpoll, context['LONGPOLL_URL'])
+            ['LONGPOLL_PATH', 'longpoll_queue'], list(context))
+        self.assertEqual(longpoll, context['LONGPOLL_PATH'])
 
 
 class UserPrefsViewTest(LoggedInTestCase):

=== modified file 'src/maasserver/urls.py'
--- src/maasserver/urls.py	2012-03-14 22:52:50 +0000
+++ src/maasserver/urls.py	2012-03-15 11:30:24 +0000
@@ -77,10 +77,22 @@
         r'^nodes/create/$', NodesCreateView.as_view(), name='node-create'),
 )
 
-if django_settings.LONGPOLL_SERVER_URL is not None:
+
+def get_proxy_longpoll_enabled():
+    """Should MaaS act as a proxy to a txlongpoll server?
+
+    This should only be true if longpoll is enabled (LONGPOLL_PATH) and
+    if the url to a txlongpoll is configured (LONGPOLL_SERVER_URL).
+    """
+    return (
+        django_settings.LONGPOLL_SERVER_URL is not None and
+        django_settings.LONGPOLL_PATH is not None)
+
+
+if get_proxy_longpoll_enabled():
     urlpatterns += patterns('maasserver.views',
         url(
-            r'^%s$' % re.escape(django_settings.LONGPOLL_SERVER_URL),
+            r'^%s$' % re.escape(django_settings.LONGPOLL_PATH),
             proxy_to_longpoll, name='proxy-to-longpoll'),
         )
 

=== modified file 'src/maasserver/views.py'
--- src/maasserver/views.py	2012-03-15 09:09:38 +0000
+++ src/maasserver/views.py	2012-03-15 11:30:24 +0000
@@ -71,10 +71,10 @@
 
 
 def get_longpoll_context():
-    if messaging is not None and django_settings.LONGPOLL_URL is not None:
+    if messaging is not None and django_settings.LONGPOLL_PATH is not None:
         return {
             'longpoll_queue': messaging.getQueue().name,
-            'LONGPOLL_URL': django_settings.LONGPOLL_URL,
+            'LONGPOLL_PATH': django_settings.LONGPOLL_PATH,
             }
     else:
         return {}