XModule Style Decoupling: Follow-up fixes & refactoring

cms/djangoapps/contentstore/views/preview.py

@@ -24,7 +24,7 @@
 from xmodule.services import SettingsService, TeamsConfigurationService
 from xmodule.studio_editable import has_author_view
 from xmodule.util.sandboxing import SandboxService
-from xmodule.util.xmodule_django import add_webpack_to_fragment
+from xmodule.util.builtin_assets import add_webpack_js_to_fragment
 from xmodule.x_module import AUTHOR_VIEW, PREVIEW_VIEWS, STUDENT_VIEW, XModuleMixin
 from cms.djangoapps.xblock_config.models import StudioConfig
 from cms.djangoapps.contentstore.toggles import individualize_anonymous_user_id, ENABLE_COPY_PASTE_FEATURE
@@ -319,7 +319,7 @@ def _studio_wrap_xblock(xblock, view, frag, context, display_name_only=False):
             'language': getattr(course, 'language', None)
         }
 
-        add_webpack_to_fragment(frag, "js/factories/xblock_validation")
+        add_webpack_js_to_fragment(frag, "js/factories/xblock_validation")
 
         html = render_to_string('studio_xblock_wrapper.html', template_context)
         frag = wrap_fragment(frag, html)

cms/envs/common.py

@@ -1440,7 +1440,6 @@
     'DEFAULT': {
         'BUNDLE_DIR_NAME': 'bundles/',
         'STATS_FILE': os.path.join(STATIC_ROOT, 'webpack-stats.json'),
-        'LOADER_CLASS': 'xmodule.util.xmodule_django.XModuleWebpackLoader',
     },
     'WORKERS': {
         'BUNDLE_DIR_NAME': 'bundles/',

cms/static/js/views/xblock.js

@@ -23,9 +23,10 @@ function($, _, ViewUtils, BaseView, XBlock, HtmlUtils) {
             var self = this,
                 view = this.view,
                 xblockInfo = this.model,
-                xblockUrl = xblockInfo.url();
+                xblockUrl = xblockInfo.url(),
+                querystring = window.location.search; // pass any querystring down to child views
             return $.ajax({
-                url: decodeURIComponent(xblockUrl) + '/' + view,
+                url: decodeURIComponent(xblockUrl) + '/' + view + querystring,
                 type: 'GET',
                 cache: false,
                 headers: {Accept: 'application/json'},

lms/envs/common.py

@@ -2715,7 +2715,6 @@ def _make_locale_paths(settings):  # pylint: disable=missing-function-docstring
     'DEFAULT': {
         'BUNDLE_DIR_NAME': 'bundles/',
         'STATS_FILE': os.path.join(STATIC_ROOT, 'webpack-stats.json'),
-        'LOADER_CLASS': 'xmodule.util.xmodule_django.XModuleWebpackLoader',
     },
     'WORKERS': {
         'BUNDLE_DIR_NAME': 'bundles/',

openedx/core/djangoapps/xblock/runtime/runtime.py

@@ -334,7 +334,7 @@ def render(self, block, view_name, context=None):
             raise PermissionDenied
 
         # We also need to override this method because some XBlocks in the
-        # edx-platform codebase use methods like add_webpack_to_fragment()
+        # edx-platform codebase use methods from builtin_assets.py,
         # which create relative URLs (/static/studio/bundles/webpack-foo.js).
         # We want all resource URLs to be absolute, such as is done when
         # local_resource_url() is used.

openedx/core/lib/xblock_utils/__init__.py

@@ -11,7 +11,6 @@
 import uuid
 
 import markupsafe
-import webpack_loader.utils
 from django.conf import settings
 from django.contrib.auth.models import User  # lint-amnesty, pylint: disable=imported-auth-user
 from django.contrib.staticfiles.storage import staticfiles_storage
@@ -29,7 +28,6 @@
 from common.djangoapps import static_replace
 from common.djangoapps.edxmako.shortcuts import render_to_string
 from xmodule.seq_block import SequenceBlock  # lint-amnesty, pylint: disable=wrong-import-order
-from xmodule.util.xmodule_django import add_webpack_to_fragment  # lint-amnesty, pylint: disable=wrong-import-order
 from xmodule.vertical_block import VerticalBlock  # lint-amnesty, pylint: disable=wrong-import-order
 from xmodule.x_module import (  # lint-amnesty, pylint: disable=wrong-import-order
     PREVIEW_VIEWS,
@@ -116,6 +114,9 @@ def wrap_xblock(
     if view == STUDENT_VIEW and getattr(block, 'HIDDEN', False):
         css_classes.append('is-hidden')
 
+    # TODO: This special case will be removed when we update the SCSS under
+    #       xmodule/assets to use the standard XBlock CSS classes.
+    #       See https://github.com/openedx/edx-platform/issues/32617.
     if getattr(block, 'uses_xmodule_styles_setup', False):
         if view in PREVIEW_VIEWS:
             # The block is acting as an XModule
@@ -444,16 +445,22 @@ def xblock_resource_pkg(block):
     openassessment.xblock.openassessmentblock.OpenAssessmentBlock, the value
     returned is 'openassessment.xblock'.
 
-    XModules are special cased because they're local to this repo and they
-    actually don't share their resource files when compiled out as part of the
-    XBlock asset pipeline. This only covers XBlocks and XModules using the
-    XBlock-style of asset specification. If they use the XModule bundling part
-    of the asset pipeline (xmodule_assets), their assets are compiled through an
-    entirely separate mechanism and put into lms-modules.js/css.
+    Built-in edx-platform XBlocks (defined under ./xmodule/) are special cases.
+    They currently use two different mechanisms to load assets:
+    1. The `builtin_assets` utilities, which let the blocks add JS and CSS
+       compiled completely outside of the XBlock pipeline. Used by HtmlBlock,
+       ProblemBlock, and most other built-in blocks currently. Handling for these
+       assets does not interact with this function.
+    2. The (preferred) standard XBlock runtime resource loading system, used by
+       LibrarySourcedBlock. Handling for these assets *does* interact with this
+       function.
+
+    We hope to migrate to (2) eventually, tracked by:
+    https://github.com/openedx/edx-platform/issues/32618.
     """
-    # XModules are a special case because they map to different dirs for
-    # sub-modules.
     module_name = block.__module__
+    # Special handling for case (2) of the built-in XBlocks because they map to different
+    # dirs for sub-modules.
     if module_name.startswith('xmodule.'):
         return module_name
 

pavelib/assets.py

@@ -170,8 +170,7 @@ def get_theme_sass_dirs(system, theme_dir):
     css_dir = theme_dir / system / "static" / "css"
     certs_sass_dir = theme_dir / system / "static" / "certificates" / "sass"
     certs_css_dir = theme_dir / system / "static" / "certificates" / "css"
-    xmodule_sass_dir = path("xmodule") / "static" / "sass" / system
-    xmodule_lookup_dir = path("xmodule") / "static" / "sass" / "include"
+    builtin_xblock_sass = path("xmodule") / "assets"
 
     dependencies = SASS_LOOKUP_DEPENDENCIES.get(system, [])
     if sass_dir.isdir():
@@ -199,18 +198,6 @@ def get_theme_sass_dirs(system, theme_dir):
             ],
         })
 
-        dirs.append({
-            "sass_source_dir": xmodule_sass_dir,
-            "css_destination_dir": path("common") / "static" / "css" / "xmodule",
-            "lookup_paths": [
-                xmodule_lookup_dir,
-                *dependencies,
-                sass_dir / "partials",
-                system_sass_dir / "partials",
-                system_sass_dir,
-            ],
-        })
-
         # now compile theme sass files for certificate
         if system == 'lms':
             dirs.append({
@@ -222,6 +209,28 @@ def get_theme_sass_dirs(system, theme_dir):
                 ],
             })
 
+    # Now, finally, compile builtin XBlocks' Sass. Themes cannot override these
+    # Sass files directly, but they *can* modify Sass variables which will affect
+    # the output here. We compile all builtin XBlocks' Sass both for LMS and CMS,
+    # not because we expect the output to be different between LMS and CMS, but
+    # because only LMS/CMS-compiled Sass can be themed; common sass is not themed.
+    dirs.append({
+        "sass_source_dir": builtin_xblock_sass,
+        "css_destination_dir": css_dir,
+        "lookup_paths": [
+            # XBlock editor views may need both LMS and CMS partials.
+            # XBlock display views should only need LMS patials.
+            # In order to keep this build script simpler, though, we just
+            # include everything and compile everything at once.
+            theme_dir / "lms" / "static" / "sass" / "partials",
+            theme_dir / "cms" / "static" / "sass" / "partials",
+            path("lms") / "static" / "sass" / "partials",
+            path("cms") / "static" / "sass" / "partials",
+            path("lms") / "static" / "sass",
+            path("cms") / "static" / "sass",
+        ],
+    })
+
     return dirs
 
 
@@ -237,8 +246,7 @@ def get_system_sass_dirs(system):
     dirs = []
     sass_dir = path(system) / "static" / "sass"
     css_dir = path(system) / "static" / "css"
-    xmodule_sass_dir = path("xmodule") / "static" / "sass" / system
-    xmodule_lookup_dir = path("xmodule") / "static" / "sass" / "include"
+    builtin_xblock_sass = path("xmodule") / "assets"
 
     dependencies = SASS_LOOKUP_DEPENDENCIES.get(system, [])
     dirs.append({
@@ -250,17 +258,6 @@ def get_system_sass_dirs(system):
         ],
     })
 
-    dirs.append({
-        "sass_source_dir": xmodule_sass_dir,
-        "css_destination_dir": path("common") / "static" / "css" / "xmodule",
-        "lookup_paths": [
-            xmodule_lookup_dir,
-            *dependencies,
-            sass_dir / "partials",
-            sass_dir,
-        ],
-    })
-
     if system == 'lms':
         dirs.append({
             "sass_source_dir": path(system) / "static" / "certificates" / "sass",
@@ -271,6 +268,18 @@ def get_system_sass_dirs(system):
             ],
         })
 
+    # See builtin_xblock_sass compilation in get_theme_sass_dirs for details.
+    dirs.append({
+        "sass_source_dir": builtin_xblock_sass,
+        "css_destination_dir": css_dir,
+        "lookup_paths": dependencies + [
+            path("lms") / "static" / "sass" / "partials",
+            path("cms") / "static" / "sass" / "partials",
+            path("lms") / "static" / "sass",
+            path("cms") / "static" / "sass",
+        ],
+    })
+
     return dirs
 
 
@@ -577,14 +586,18 @@ def _compile_sass(system, theme, debug, force, timing_info):
             else:
                 sh(f"rm -rf {css_dir}/*.css")
 
+        all_lookup_paths = COMMON_LOOKUP_PATHS + lookup_paths
+        print(f"Compiling Sass: {sass_source_dir} -> {css_dir}")
+        for lookup_path in all_lookup_paths:
+            print(f"    with Sass lookup path: {lookup_path}")
         if dry_run:
             tasks.environment.info("libsass {sass_dir}".format(
                 sass_dir=sass_source_dir,
             ))
         else:
             sass.compile(
                 dirname=(sass_source_dir, css_dir),
-                include_paths=COMMON_LOOKUP_PATHS + lookup_paths,
+                include_paths=all_lookup_paths,
                 source_comments=source_comments,
                 output_style=output_style,
             )

xmodule/README.rst

@@ -6,15 +6,18 @@ The ``xmodule`` folder contains a variety of old-yet-important functionality cor
 * the ModuleStore, edx-platform's "Version 1" learning content storage backend;
 * an XBlock Runtime implementation for ModuleStore-backed content;
 * the "partitions" framework for differentiated XBlock content;
-* implementations for the "stuctural" XBlocks: ``course``, ``chapter``, and ``sequential``; and
-* the implementations of several different content-level XBlocks, such as ``problem`` and ``html``.
+* implementations for the "stuctural" XBlocks: ``course``, ``chapter``, and ``sequential``;
+* the implementations of several different built-in content-level XBlocks, such as ``problem`` and ``html``; and
+* `assets for those built-in XBlocks`_.
+
+.. _assets for those built-in XBlocks: https://github.com/openedx/edx-platform/tree/master/assets
 
 Historical Context
 ******************
 
 "XModule" was the original content framework for edx-platform, which gives the folder its current name.
 XModules rendered specific course run content types to users for both authoring and learning.
-For instance, there was an XModule for Videos, another for HTML snippets, and another for Sequences. 
+For instance, there was an XModule for Videos, another for HTML snippets, and another for Sequences.
 
 XModule was succeeded in ~2013 by the "XBlock" framework, which served the same purpose, but put additional focus into flexibility and modularity.
 XBlock allows new content types to be created by anyone, completely external the edx-platform repository.
@@ -47,7 +50,7 @@ To help with this direction, please **do not add new functionality to this direc
 
 .. _Blockstore: https://github.com/openedx/blockstore/
 .. _edx-platform XBlock runtime: https://github.com/openedx/edx-platform/tree/master/openedx/core/djangoapps/xblock
-.. _openedx-learning: https://github.com/openedx/openedx-learning 
+.. _openedx-learning: https://github.com/openedx/openedx-learning
 .. _xblock-drag-and-drop-v2: https://github.com/openedx/xblock-drag-and-drop-v2
 .. _the forums: https://discuss.openedx.org
 

xmodule/annotatable_block.py

@@ -12,7 +12,7 @@
 from openedx.core.djangolib.markup import HTML, Text
 from xmodule.editing_block import EditingMixin
 from xmodule.raw_block import RawMixin
-from xmodule.util.xmodule_django import add_webpack_to_fragment
+from xmodule.util.builtin_assets import add_webpack_js_to_fragment, add_sass_to_fragment
 from xmodule.xml_block import XmlMixin
 from xmodule.x_module import (
     HTMLSnippet,
@@ -199,7 +199,8 @@ def student_view(self, context):  # lint-amnesty, pylint: disable=unused-argumen
         """
         fragment = Fragment()
         fragment.add_content(self.get_html())
-        add_webpack_to_fragment(fragment, 'AnnotatableBlockPreview')
+        add_sass_to_fragment(fragment, 'AnnotatableBlockDisplay.scss')
+        add_webpack_js_to_fragment(fragment, 'AnnotatableBlockDisplay')
         shim_xmodule_js(fragment, 'Annotatable')
 
         return fragment
@@ -211,6 +212,7 @@ def studio_view(self, _context):
         fragment = Fragment(
             self.runtime.service(self, 'mako').render_template(self.mako_template, self.get_context())
         )
-        add_webpack_to_fragment(fragment, 'AnnotatableBlockStudio')
+        add_sass_to_fragment(fragment, 'AnnotatableBlockEditor.scss')
+        add_webpack_js_to_fragment(fragment, 'AnnotatableBlockEditor')
         shim_xmodule_js(fragment, self.studio_js_module_name)
         return fragment

xmodule/assets/HtmlBlockDisplay.scss

@@ -0,0 +1,6 @@
+.xmodule_display.xmodule_AboutBlock,
+.xmodule_display.xmodule_CourseInfoBlock,
+.xmodule_display.xmodule_HtmlBlock,
+.xmodule_display.xmodule_StaticTabBlock {
+  @import "html/display.scss";
+}

xmodule/assets/HtmlBlockEditor.scss

@@ -0,0 +1,7 @@
+.xmodule_edit.xmodule_AboutBlock,
+.xmodule_edit.xmodule_CourseInfoBlock,
+.xmodule_edit.xmodule_HtmlBlock,
+.xmodule_edit.xmodule_StaticTabBlock {
+  @import "editor/edit.scss";
+  @import "html/edit.scss";
+}