Merge pull request #13358 from edx/saleem-latif/MAYN-207-transition-to-theming

MAYN-207: Migrate WL sites to comprehensive theming for LMS

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
 

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)

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()

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):

openedx/core/djangoapps/theming/tests/test_microsites.py

+"""
+    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")