From f974347dcb7c89c6b5ae12e142dbafcbaf975195 Mon Sep 17 00:00:00 2001 From: Saleem Latif Date: Wed, 31 Aug 2016 11:59:45 +0500 Subject: [PATCH] Update theming to have priority over microsites. --- openedx/core/djangoapps/theming/helpers.py | 32 +++- openedx/core/djangoapps/theming/middleware.py | 5 +- openedx/core/djangoapps/theming/models.py | 23 ++- .../djangoapps/theming/tests/test_helpers.py | 149 +++++++++++++++++- .../theming/tests/test_microsites.py | 51 ++++++ 5 files changed, 248 insertions(+), 12 deletions(-) create mode 100644 openedx/core/djangoapps/theming/tests/test_microsites.py diff --git a/openedx/core/djangoapps/theming/helpers.py b/openedx/core/djangoapps/theming/helpers.py index cb119f1266..c7b241f988 100644 --- a/openedx/core/djangoapps/theming/helpers.py +++ b/openedx/core/djangoapps/theming/helpers.py @@ -11,7 +11,7 @@ from django.contrib.staticfiles.storage import staticfiles_storage from request_cache.middleware import RequestCache from microsite_configuration import microsite - +from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers from logging import getLogger logger = getLogger(__name__) # pylint: disable=invalid-name @@ -21,7 +21,10 @@ def get_template_path(relative_path, **kwargs): """ This is a proxy function to hide microsite_configuration behind comprehensive theming. """ - if microsite.is_request_in_microsite(): + # We need to give priority to theming over microsites + # So, we apply microsite override only if there is no associated site theme + # and associated microsite is present. + if not current_request_has_associated_site_theme() and microsite.is_request_in_microsite(): relative_path = microsite.get_template_path(relative_path, **kwargs) return relative_path @@ -30,7 +33,8 @@ def is_request_in_themed_site(): """ This is a proxy function to hide microsite_configuration behind comprehensive theming. """ - return microsite.is_request_in_microsite() + # We need to give priority to theming/site-configuration over microsites + return configuration_helpers.is_site_configuration_enabled() or microsite.is_request_in_microsite() def get_template(uri): @@ -38,7 +42,10 @@ def get_template(uri): This is a proxy function to hide microsite_configuration behind comprehensive theming. :param uri: uri of the template """ - return microsite.get_template(uri) + # We need to give priority to theming over microsites + # So, we apply microsite template override only when there is no associated theme, + if not current_request_has_associated_site_theme(): + return microsite.get_template(uri) def get_template_path_with_theme(relative_path): @@ -190,6 +197,18 @@ def get_current_theme(): return None +def current_request_has_associated_site_theme(): + """ + True if current request has an associated SiteTheme, False otherwise. + + Returns: + True if current request has an associated SiteTheme, False otherwise + """ + request = get_current_request() + site_theme = getattr(request, 'site_theme', None) + return bool(site_theme and site_theme.id) + + def get_theme_base_dir(theme_dir_name, suppress_error=False): """ Returns absolute path to the directory that contains the given theme. @@ -298,7 +317,12 @@ def is_comprehensive_theming_enabled(): Returns: (bool): True if comprehensive theming is enabled else False """ + # We need to give priority to theming over microsites + if settings.ENABLE_COMPREHENSIVE_THEMING and current_request_has_associated_site_theme(): + return True + # Disable theming for microsites + # Microsite configurations take priority over the default site theme. if microsite.is_request_in_microsite(): return False diff --git a/openedx/core/djangoapps/theming/middleware.py b/openedx/core/djangoapps/theming/middleware.py index 614a355b7d..6bef0adbfc 100644 --- a/openedx/core/djangoapps/theming/middleware.py +++ b/openedx/core/djangoapps/theming/middleware.py @@ -5,7 +5,7 @@ Note: This middleware depends on "django_sites_extensions" app So it must be added to INSTALLED_APPS in django settings files. """ - +from django.conf import settings from openedx.core.djangoapps.theming.models import SiteTheme @@ -18,4 +18,5 @@ class CurrentSiteThemeMiddleware(object): """ fetch Site Theme for the current site and add it to the request object. """ - request.site_theme = SiteTheme.get_theme(request.site) + default_theme = SiteTheme(site=request.site, theme_dir_name=settings.DEFAULT_SITE_THEME) + request.site_theme = SiteTheme.get_theme(request.site, default=default_theme) diff --git a/openedx/core/djangoapps/theming/models.py b/openedx/core/djangoapps/theming/models.py index d6e2e43b10..a3ab9ba1d8 100644 --- a/openedx/core/djangoapps/theming/models.py +++ b/openedx/core/djangoapps/theming/models.py @@ -20,20 +20,33 @@ class SiteTheme(models.Model): return self.theme_dir_name @staticmethod - def get_theme(site): + def get_theme(site, default=None): """ Get SiteTheme object for given site, returns default site theme if it can not find a theme for the given site and `DEFAULT_SITE_THEME` setting has a proper value. Args: site (django.contrib.sites.models.Site): site object related to the current site. + default (openedx.core.djangoapps.models.SiteTheme): site theme object to return in case there is no theme + associated for the given site. Returns: - SiteTheme object for given site or a default site set by `DEFAULT_SITE_THEME` + SiteTheme object for given site or a default site passed in as the argument. """ theme = site.themes.first() + return theme or default - if (not theme) and settings.DEFAULT_SITE_THEME: - theme = SiteTheme(site=site, theme_dir_name=settings.DEFAULT_SITE_THEME) - return theme + @staticmethod + def has_theme(site): + """ + Returns True if given site has an associated site theme in database, returns False otherwise. + Note: DEFAULT_SITE_THEME is not considered as an associated site. + + Args: + site (django.contrib.sites.models.Site): site object related to the current site. + + Returns: + True if given site has an associated site theme in database, returns False otherwise. + """ + return site.themes.exists() diff --git a/openedx/core/djangoapps/theming/tests/test_helpers.py b/openedx/core/djangoapps/theming/tests/test_helpers.py index 9774671233..c7d38c2db3 100644 --- a/openedx/core/djangoapps/theming/tests/test_helpers.py +++ b/openedx/core/djangoapps/theming/tests/test_helpers.py @@ -2,13 +2,14 @@ Test helpers for Comprehensive Theming. """ import unittest -from mock import patch +from mock import patch, Mock from django.test import TestCase, override_settings from django.conf import settings from openedx.core.djangoapps.theming.tests.test_util import with_comprehensive_theme from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers +from openedx.core.djangoapps.theming import helpers as theming_helpers from openedx.core.djangoapps.theming.helpers import get_template_path_with_theme, strip_site_theme_templates_path, \ get_themes, Theme, get_theme_base_dir @@ -54,6 +55,152 @@ class TestHelpers(TestCase): jwt_auth = configuration_helpers.get_value('JWT_AUTH') self.assertEqual(jwt_auth[override_key], override_value) + def test_is_comprehensive_theming_enabled(self): + """ + Tests to make sure the is_comprehensive_theming_enabled function works as expected. + Here are different scenarios that we need to test + + 1. Theming is enabled, there is a SiteTheme record and microsite configuration for the current site. + is_comprehensive_theming_enabled should return True + 2. Theming is enabled, there is no SiteTheme record but there is microsite configuration for the current site. + is_comprehensive_theming_enabled should return False + 3. Theming is enabled, there is neither a SiteTheme record nor microsite configuration for the current site. + is_comprehensive_theming_enabled should return True + 4. Theming is disabled, there is a SiteTheme record and microsite configuration for the current site. + is_comprehensive_theming_enabled should return False + 5. Theming is disabled, there is no SiteTheme record but there is microsite configuration for the current site. + is_comprehensive_theming_enabled should return False + 6. Theming is disabled, there is neither a SiteTheme record nor microsite configuration for the current site. + is_comprehensive_theming_enabled should return False + """ + # Theming is enabled, there is a SiteTheme record and microsite configuration for the current site + with patch( + "openedx.core.djangoapps.theming.helpers.current_request_has_associated_site_theme", + Mock(return_value=True), + ): + with patch( + "openedx.core.djangoapps.theming.helpers.microsite.is_request_in_microsite", + Mock(return_value=True), + ): + self.assertTrue(theming_helpers.is_comprehensive_theming_enabled()) + + # Theming is enabled, there is no SiteTheme record but there is microsite configuration for the current site. + with patch( + "openedx.core.djangoapps.theming.helpers.current_request_has_associated_site_theme", + Mock(return_value=False), + ): + with patch( + "openedx.core.djangoapps.theming.helpers.microsite.is_request_in_microsite", + Mock(return_value=True), + ): + self.assertFalse(theming_helpers.is_comprehensive_theming_enabled()) + + # Theming is enabled, there is neither a SiteTheme record nor microsite configuration for the current site. + with patch( + "openedx.core.djangoapps.theming.helpers.current_request_has_associated_site_theme", + Mock(return_value=False), + ): + with patch( + "openedx.core.djangoapps.theming.helpers.microsite.is_request_in_microsite", + Mock(return_value=False), + ): + self.assertTrue(theming_helpers.is_comprehensive_theming_enabled()) + + with override_settings(ENABLE_COMPREHENSIVE_THEMING=False): + # Theming is disabled, there is a SiteTheme record and microsite configuration for the current site. + with patch( + "openedx.core.djangoapps.theming.helpers.current_request_has_associated_site_theme", + Mock(return_value=True), + ): + with patch( + "openedx.core.djangoapps.theming.helpers.microsite.is_request_in_microsite", + Mock(return_value=True), + ): + self.assertFalse(theming_helpers.is_comprehensive_theming_enabled()) + + # Theming is disabled, there is no SiteTheme record but + # there is microsite configuration for the current site. + with patch( + "openedx.core.djangoapps.theming.helpers.current_request_has_associated_site_theme", + Mock(return_value=False), + ): + with patch( + "openedx.core.djangoapps.theming.helpers.microsite.is_request_in_microsite", + Mock(return_value=True), + ): + self.assertFalse(theming_helpers.is_comprehensive_theming_enabled()) + + # Theming is disabled, there is neither a SiteTheme record nor microsite configuration for the current site. + with patch( + "openedx.core.djangoapps.theming.helpers.current_request_has_associated_site_theme", + Mock(return_value=False), + ): + with patch( + "openedx.core.djangoapps.theming.helpers.microsite.is_request_in_microsite", + Mock(return_value=False), + ): + self.assertFalse(theming_helpers.is_comprehensive_theming_enabled()) + + def test_get_template(self): + """ + Tests to make sure the get_template function works as expected. + """ + # if the current site has associated SiteTheme then get_template should return None + with patch( + "openedx.core.djangoapps.theming.helpers.current_request_has_associated_site_theme", + Mock(return_value=True), + ): + with patch( + "openedx.core.djangoapps.theming.helpers.microsite.is_request_in_microsite", + Mock(return_value=True), + ): + with patch("microsite_configuration.microsite.TEMPLATES_BACKEND") as mock_microsite_backend: + mock_microsite_backend.get_template = Mock(return_value="/microsite/about.html") + self.assertIsNone(theming_helpers.get_template("about.html")) + + # if the current site does not have associated SiteTheme then get_template should return microsite override + with patch( + "openedx.core.djangoapps.theming.helpers.current_request_has_associated_site_theme", + Mock(return_value=False), + ): + with patch( + "openedx.core.djangoapps.theming.helpers.microsite.is_request_in_microsite", + Mock(return_value=True), + ): + with patch("microsite_configuration.microsite.TEMPLATES_BACKEND") as mock_microsite_backend: + mock_microsite_backend.get_template = Mock(return_value="/microsite/about.html") + self.assertEqual(theming_helpers.get_template("about.html"), "/microsite/about.html") + + def test_get_template_path(self): + """ + Tests to make sure the get_template_path function works as expected. + """ + # if the current site has associated SiteTheme then get_template_path should return the argument as is. + with patch( + "openedx.core.djangoapps.theming.helpers.current_request_has_associated_site_theme", + Mock(return_value=True), + ): + with patch( + "openedx.core.djangoapps.theming.helpers.microsite.is_request_in_microsite", + Mock(return_value=True), + ): + with patch("microsite_configuration.microsite.TEMPLATES_BACKEND") as mock_microsite_backend: + mock_microsite_backend.get_template = Mock(return_value="/microsite/about.html") + self.assertEqual(theming_helpers.get_template_path("about.html"), "about.html") + + # if the current site does not have associated SiteTheme then get_template_path should return microsite override + with patch( + "openedx.core.djangoapps.theming.helpers.current_request_has_associated_site_theme", + Mock(return_value=False), + ): + with patch( + "openedx.core.djangoapps.theming.helpers.microsite.is_request_in_microsite", + Mock(return_value=True), + ): + with patch("microsite_configuration.microsite.TEMPLATES_BACKEND") as mock_microsite_backend: + mock_microsite_backend.get_template_path = Mock(return_value="/microsite/about.html") + self.assertEqual(theming_helpers.get_template_path("about.html"), "/microsite/about.html") + @unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Test only valid in lms') class TestHelpersLMS(TestCase): diff --git a/openedx/core/djangoapps/theming/tests/test_microsites.py b/openedx/core/djangoapps/theming/tests/test_microsites.py new file mode 100644 index 0000000000..0f3a79fba1 --- /dev/null +++ b/openedx/core/djangoapps/theming/tests/test_microsites.py @@ -0,0 +1,51 @@ +""" + Tests for microsites and comprehensive themes. +""" +import unittest + +from django.conf import settings +from django.test import TestCase +from django.contrib.sites.models import Site + +from openedx.core.djangoapps.theming.models import SiteTheme + + +@unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Test only valid in lms') +class TestComprehensiveThemeLMS(TestCase): + """ + Test html, sass and static file overrides for comprehensive themes. + """ + def __add_site_theme__(self, domain, theme): + """ + Add a Site and SiteTheme record for the given domain and theme + Args: + domain: domain to which attach the new Site + theme: theme to apply on the new site + """ + site, __ = Site.objects.get_or_create(domain=domain, name=domain) + SiteTheme.objects.get_or_create(site=site, theme_dir_name=theme) + + def test_theme_footer(self): + """ + Test that theme footer is used instead of microsite footer. + """ + # Add SiteTheme with the same domain name as microsite + self.__add_site_theme__(domain=settings.MICROSITE_TEST_HOSTNAME, theme="test-theme") + + # Test that requesting on a host, where both theme and microsite is applied + # theme gets priority over microsite. + resp = self.client.get('/', HTTP_HOST=settings.MICROSITE_TEST_HOSTNAME) + self.assertEqual(resp.status_code, 200) + # This string comes from footer.html of test-theme + self.assertContains(resp, "This is a footer for test-theme.") + + def test_microsite_footer(self): + """ + Test that microsite footer is used instead of default theme footer. + """ + # Test that if theming is enabled but there is no SiteTheme for the current site, then + # DEFAULT_SITE_THEME does not interfere with microsites + resp = self.client.get('/', HTTP_HOST=settings.MICROSITE_TEST_HOSTNAME) + self.assertEqual(resp.status_code, 200) + # This string comes from footer.html of test_site, which is a microsite + self.assertContains(resp, "This is a Test Site footer")