From 8cfb1ed8b04731b9b85a004aaf476ae5d6f6ba69 Mon Sep 17 00:00:00 2001 From: zamanafzal Date: Thu, 1 Oct 2026 11:09:44 +0500 Subject: [PATCH] feat(ol_openedx_feedback): send a visible title only for blocks that render one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit For most block types display_name is an authoring label that never appears on the learner's page ("Wk3 intro copy - REVISED, do not reuse"), so the feedback drawer was naming blocks with text only the author was meant to see. The payload now carries `visibleTitle`, set from a curated map of the types that do draw a title and the field holding it — display_name for most, `title` for openassessment, `block_name` for survey. A type outside the map sends "", which the drawer renders as generic block-type wording. drag-and-drop-v2 is gated on its author-controlled `show_title` toggle. blockDisplayName is still sent, so the submission keeps recording the Studio name for the content team. Each map entry asserts that the type draws that field on the learner's page, so a wrong entry leaks the author's private note — which is the bug the map exists to prevent. It is therefore deliberately not a deployment setting, every entry was verified against the rendering template, and a test fails if a new display_name entry ships uncovered. --- src/ol_openedx_feedback/README.rst | 141 +++++++++++------- .../ol_openedx_feedback/block.py | 5 +- .../ol_openedx_feedback/constants.py | 24 +++ .../ol_openedx_feedback/utils.py | 27 +++- src/ol_openedx_feedback/pyproject.toml | 2 +- src/ol_openedx_feedback/tests/test_aside.py | 28 ++++ src/ol_openedx_feedback/tests/test_utils.py | 121 ++++++++++++++- src/ol_openedx_feedback/tests/utils.py | 1 + uv.lock | 2 +- 9 files changed, 291 insertions(+), 60 deletions(-) diff --git a/src/ol_openedx_feedback/README.rst b/src/ol_openedx_feedback/README.rst index 6c83014ab..2dc4289ae 100644 --- a/src/ol_openedx_feedback/README.rst +++ b/src/ol_openedx_feedback/README.rst @@ -1,27 +1,48 @@ ol-openedx-feedback ################### -An Open edX plugin that adds a per-block "Send feedback" trigger to applicable -leaf blocks in the LMS via an ``XBlockAside``. The trigger is shown only to -authenticated learners (never in Studio author/preview mode and never to -anonymous users). - -When a learner clicks the trigger the aside posts an ``ol-feedback::drawer-open`` -message to its parent window (the Learning MFE) using ``window.parent.postMessage``. -The message payload carries the block context needed to identify the content -being rated: - -- ``courseId`` — the course key -- ``blockUsageKey`` — the block's usage key -- ``blockType`` — the XBlock category (e.g. ``problem``, ``video``) -- ``blockDisplayName`` — the block's display name - -The Learning MFE receives the message and opens the feedback drawer, which -submits the learner's feedback directly to the **mit-learn** service. This -plugin does **not** persist anything in edx-platform and exposes no REST API. +An Open edX plugin that adds a "Send feedback" megaphone to course blocks in the +LMS via an ``XBlockAside``. Clicking it opens the feedback drawer in the Learning +MFE, which submits the learner's feedback to the **mit-learn** service. This +plugin renders the trigger and nothing else: no models, no REST API, nothing +persisted in edx-platform. + +The trigger renders only for authenticated learners — never in Studio +author/preview mode, never for anonymous users. It sits right-aligned below the +block, or docked beside the AskTIM button on blocks that have one. + +Messages +======== + +The aside talks to its parent window (the Learning MFE) with +``window.parent.postMessage``, always addressed to +``settings.LEARNING_MICROFRONTEND_URL`` and never to ``"*"``. + +Sent on click, as ``ol-feedback::drawer-open``: + +- ``payload.courseId`` — the course key +- ``payload.blockUsageKey`` — the block's usage key +- ``payload.blockType`` — the XBlock category (e.g. ``video``, ``problem``) +- ``payload.blockDisplayName`` — the Studio ``display_name``. Recorded on the + submission so the content team can find the component; the drawer doesn't + display it. +- ``payload.visibleTitle`` — the title the learner can see on the page, or ``""`` + when this plugin can't confirm there is one. This is the only name the drawer + displays; see `Visible titles`_. +- ``viaKeyboard`` — true when the click came from the keyboard. The drawer + renders cross-origin and so can't use ``:focus-visible``; it uses this to + decide whether to highlight its heading. + +Received from the MFE, both so keyboard focus can return to the megaphone +(WCAG 2.4.3): + +- ``ol-feedback::drawer-closed`` — the drawer has closed; focus the trigger that + opened it. +- ``ol-feedback::focus-trigger`` — the drawer's "return to block" link; focus the + trigger but leave the drawer open. Version Compatibility -====================== +===================== For this plugin's compatibility with Open edX, see the `Open edX Release Compatibility table <../../docs#open-edx-release-compatibility>`_. @@ -29,44 +50,39 @@ For this plugin's compatibility with Open edX, see the Installation ============ -Install the package into the LMS Python environment: +1. Install the package into the LMS Python environment and restart the LMS: -.. code-block:: bash + .. code-block:: bash - pip install ol-openedx-feedback + pip install ol-openedx-feedback -The plugin registers itself automatically through its entry points — the -``xblock_asides.v1`` aside plus the ``lms.djangoapp`` app config — so no changes -to ``INSTALLED_APPS`` are required. Restart the LMS after installing. (The -trigger is learner-facing only, so the plugin is LMS-only and is not installed -in Studio/CMS.) + The plugin registers itself through its entry points — the ``xblock_asides.v1`` + aside plus the ``lms.djangoapp`` app config — so ``INSTALLED_APPS`` needs no + change. The trigger is learner-facing only, so the plugin is LMS-only and is + not installed in Studio/CMS. -Enable XBlock asides in the LMS admin -------------------------------------- +2. **Enable XBlock asides.** No aside renders until they are switched on. In the + LMS admin, open **XBlock Asides Config** + (``/admin/lms_xblock/xblockasidesconfig/``), add an entry, and check + **Enabled**. Keep the block types you want the trigger on out of **Disabled + blocks** (space-separated; defaults to ``about course_info static_tab``). -XBlock asides must be turned on for any aside (including this one) to render. -In the LMS Django admin, open **XBlock Asides Config** -(``/admin/lms_xblock/xblockasidesconfig/``), add a new entry, and check -**Enabled**. Make sure the block types you want the feedback trigger on are -**not** listed in **Disabled blocks** (the space-separated field defaults to -``about course_info static_tab``). - -Enablement -========== - -Feedback is gated by the ``ol_openedx_feedback.feedback_enabled`` course waffle -flag (default off). Enable it for the desired courses (or globally) to roll out. +3. **Turn on the waffle flag.** The trigger is gated per course by the + ``ol_openedx_feedback.feedback_enabled`` course waffle flag, off by default. + Enable it for the courses you're rolling out to, or globally. Configuration ============= +Both settings below are read from ``ENV_TOKENS`` (e.g. in ``lms.yml``) by the +plugin's ``settings.common`` hook and exposed as Django settings of the same +name. + Excluded block types -------------------- -By default the trigger renders on every leaf block and is suppressed only on -structural containers (``course`` / ``chapter`` / ``sequential`` / ``vertical``). -To additionally exclude one or more block types (for example ``html``), override -the excluded set through ``ENV_TOKENS`` (e.g. in ``lms.yml``): +The trigger renders on every block type except a set of structural containers. +Override the set to exclude more types — for example ``html``: .. code-block:: yaml @@ -77,19 +93,40 @@ the excluded set through ``ENV_TOKENS`` (e.g. in ``lms.yml``): - vertical - html -The plugin reads this value via its ``settings.common`` ``plugin_settings`` hook -and exposes it as the ``OL_OPENEDX_FEEDBACK_EXCLUDED_BLOCK_TYPES`` Django -setting, defaulting to the structural set above when unset. +Unset, it defaults to ``course``, ``chapter``, ``sequential`` and ``vertical``. Text label ---------- -The trigger shows only the megaphone icon by default. To also render the -"Feedback" text label beside it, enable it through ``ENV_TOKENS``: +The trigger is icon-only by default. To render the "Feedback" text label beside +the megaphone: .. code-block:: yaml OL_OPENEDX_FEEDBACK_SHOW_LABEL: true -This is exposed as the ``OL_OPENEDX_FEEDBACK_SHOW_LABEL`` Django setting via the -same ``plugin_settings`` hook, defaulting to ``false`` (icon-only) when unset. +Visible titles +============== + +For most block types ``display_name`` is an authoring label that never appears +on the page ("Wk3 intro copy - REVISED, do not reuse"), so showing it in the +feedback drawer would confuse the learner. The plugin therefore keeps a curated +map of the types that *do* render a title, and the field holding it — +``display_name`` for most, ``title`` for ``openassessment``, ``block_name`` for +``survey``. Types outside the map send an empty ``visibleTitle``, which the +drawer renders as generic block-type wording. + +Two details: + +- A type whose title is author-toggleable (``drag-and-drop-v2``'s ``show_title``) + reports its title only when the toggle is on. +- Keys are usage-key block types (``category``). The extracted + ``xblocks-contrib`` variants register under ``__extracted`` entry points + but keep the plain category, so they need no separate entry. + +**The map is not a setting.** Each entry asserts that the block type draws that +field on the learner's page, and the value goes verbatim into the drawer's +question text — so a wrong entry leaks the author's private note, which is the +bug the map exists to prevent. Changing it means editing +``DEFAULT_VISIBLE_TITLE_FIELDS`` in ``constants.py`` and having the claim +reviewed. Leaving a type out only costs generic wording. diff --git a/src/ol_openedx_feedback/ol_openedx_feedback/block.py b/src/ol_openedx_feedback/ol_openedx_feedback/block.py index 11778c077..3c5d6d5b8 100644 --- a/src/ol_openedx_feedback/ol_openedx_feedback/block.py +++ b/src/ol_openedx_feedback/ol_openedx_feedback/block.py @@ -20,7 +20,7 @@ from ol_openedx_feedback.compat import get_feedback_enabled_flag from ol_openedx_feedback.constants import DEFAULT_SHOW_LABEL -from ol_openedx_feedback.utils import is_aside_applicable_to_block +from ol_openedx_feedback.utils import get_visible_title, is_aside_applicable_to_block log = logging.getLogger(__name__) @@ -79,7 +79,10 @@ def student_view_aside(self, block, context=None): # noqa: ARG002 "courseId": str(block_usage_key.course_key), "blockUsageKey": str(block_usage_key), "blockType": block_type, + # Studio name: recorded on the submission, never shown to + # the learner. Only visibleTitle reaches the panel. "blockDisplayName": block.display_name or "", + "visibleTitle": get_visible_title(block), }, }, ) diff --git a/src/ol_openedx_feedback/ol_openedx_feedback/constants.py b/src/ol_openedx_feedback/ol_openedx_feedback/constants.py index 1d8693c82..b601ceea7 100644 --- a/src/ol_openedx_feedback/ol_openedx_feedback/constants.py +++ b/src/ol_openedx_feedback/ol_openedx_feedback/constants.py @@ -6,5 +6,29 @@ # exclude a content type like ``html``). DEFAULT_EXCLUDED_BLOCK_TYPES = {"course", "chapter", "sequential", "vertical"} +# Block types that render a title the learner can see, mapped to the field +# holding it. An unlisted type sends no title and the panel names the type +# instead. A wrong entry leaks an author-only Studio name, so when in doubt +# leave it out — see "Visible titles" in the README before editing. +DEFAULT_VISIBLE_TITLE_FIELDS = { + "annotatable": "display_name", + "drag-and-drop-v2": "display_name", + "edx_sga": "display_name", + "lti": "display_name", + "lti_consumer": "display_name", + "pdf": "display_name", + "poll": "display_name", + "problem": "display_name", + "staffgradedxblock": "display_name", + "video": "display_name", + "videoalpha": "display_name", + "word_cloud": "display_name", + "openassessment": "title", + "survey": "block_name", +} + +# Types whose title renders only when an author-controlled boolean is on. +TITLE_VISIBILITY_TOGGLES = {"drag-and-drop-v2": "show_title"} + # Show the "Feedback" text label next to the icon. Off by default (icon only). DEFAULT_SHOW_LABEL = False diff --git a/src/ol_openedx_feedback/ol_openedx_feedback/utils.py b/src/ol_openedx_feedback/ol_openedx_feedback/utils.py index c27aa3194..9e2d66215 100644 --- a/src/ol_openedx_feedback/ol_openedx_feedback/utils.py +++ b/src/ol_openedx_feedback/ol_openedx_feedback/utils.py @@ -1,8 +1,13 @@ """Utility helpers for ol_openedx_feedback.""" from django.conf import settings +from django.utils.functional import Promise -from ol_openedx_feedback.constants import DEFAULT_EXCLUDED_BLOCK_TYPES +from ol_openedx_feedback.constants import ( + DEFAULT_EXCLUDED_BLOCK_TYPES, + DEFAULT_VISIBLE_TITLE_FIELDS, + TITLE_VISIBILITY_TOGGLES, +) def get_excluded_block_types(): @@ -24,3 +29,23 @@ def is_aside_applicable_to_block(block): """Feedback applies to every block type except excluded containers.""" block_type = getattr(block, "category", None) return bool(block_type) and block_type not in get_excluded_block_types() + + +def get_visible_title(block): + """Return the title the learner can see on the page, or "" if there is none. + + "" tells the panel to name the block type instead. + """ + block_type = getattr(block, "category", None) + title_field = DEFAULT_VISIBLE_TITLE_FIELDS.get(block_type) + if not title_field: + return "" + + toggle_field = TITLE_VISIBILITY_TOGGLES.get(block_type) + if toggle_field and not getattr(block, toggle_field, False): + return "" + + title = getattr(block, title_field, "") + if not isinstance(title, str | Promise): + return "" + return str(title).strip() diff --git a/src/ol_openedx_feedback/pyproject.toml b/src/ol_openedx_feedback/pyproject.toml index 673ab752b..a7f9d2aa8 100644 --- a/src/ol_openedx_feedback/pyproject.toml +++ b/src/ol_openedx_feedback/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "ol-openedx-feedback" -version = "0.3.0" +version = "0.3.1" description = "An Open edX plugin to collect per-block learner feedback" authors = [{name = "MIT Office of Digital Learning"}] license = "BSD-3-Clause" diff --git a/src/ol_openedx_feedback/tests/test_aside.py b/src/ol_openedx_feedback/tests/test_aside.py index 8598d49c9..c889650dc 100644 --- a/src/ol_openedx_feedback/tests/test_aside.py +++ b/src/ol_openedx_feedback/tests/test_aside.py @@ -66,6 +66,34 @@ def test_trigger_is_not_marked_as_a_dialog_popup(self): assert "aria-haspopup" not in fragment.content assert 'aria-label="' in fragment.content + @skip_unless_lms + def test_payload_sends_visible_title_for_a_block_that_renders_it(self): + """A video draws its name as a heading, so the panel may use it.""" + self.runtime.user_id = 5 + self.runtime.is_author_mode = False + self.video_aside_instance.runtime = self.runtime + + fragment = self.video_aside_instance.student_view_aside(self.video_block) + payload = fragment.json_init_args["drawer_payload"] + + assert payload["visibleTitle"] == "My Video" + assert payload["blockDisplayName"] == "My Video" + + @skip_unless_lms + def test_payload_withholds_studio_name_for_a_block_that_hides_it(self): + """An html block renders only its body, so its Studio name is + author-only: withheld from the panel, still recorded. + """ + self.runtime.user_id = 5 + self.runtime.is_author_mode = False + self.html_aside_instance.runtime = self.runtime + + fragment = self.html_aside_instance.student_view_aside(self.html_block) + payload = fragment.json_init_args["drawer_payload"] + + assert payload["visibleTitle"] == "" + assert payload["blockDisplayName"] == "An HTML Block" + @data( *[ ["video", True, False, True], diff --git a/src/ol_openedx_feedback/tests/test_utils.py b/src/ol_openedx_feedback/tests/test_utils.py index e57ef3564..8a46458df 100644 --- a/src/ol_openedx_feedback/tests/test_utils.py +++ b/src/ol_openedx_feedback/tests/test_utils.py @@ -1,14 +1,19 @@ -"""Tests for block applicability helper.""" +"""Tests for block applicability and visible-title helpers.""" from types import SimpleNamespace import pytest from django.test import override_settings -from ol_openedx_feedback.utils import is_aside_applicable_to_block +from django.utils.translation import gettext_lazy +from ol_openedx_feedback.constants import ( + DEFAULT_VISIBLE_TITLE_FIELDS, + TITLE_VISIBILITY_TOGGLES, +) +from ol_openedx_feedback.utils import get_visible_title, is_aside_applicable_to_block -def _block(category): - return SimpleNamespace(category=category) +def _block(category, **fields): + return SimpleNamespace(category=category, **fields) @pytest.mark.parametrize( @@ -42,3 +47,111 @@ def test_excluded_block_types_setting_override(): """A deployment can exclude an extra block type via the setting.""" assert is_aside_applicable_to_block(_block("html")) is False assert is_aside_applicable_to_block(_block("video")) is True + + +DISPLAY_NAME_CATEGORIES = [ + "annotatable", + "edx_sga", + "lti", + "lti_consumer", + "pdf", + "poll", + "problem", + "staffgradedxblock", + "video", + "videoalpha", + "word_cloud", +] + + +@pytest.mark.parametrize("category", DISPLAY_NAME_CATEGORIES) +def test_visible_title_returned_for_blocks_that_render_it(category): + """Types that draw a heading on the page report their display_name.""" + block = _block(category, display_name="Lecture 1: Limits") + assert get_visible_title(block) == "Lecture 1: Limits" + + +def test_every_curated_display_name_entry_is_exercised(): + """A new map entry that no test covers would ship unverified.""" + curated = { + category + for category, field in DEFAULT_VISIBLE_TITLE_FIELDS.items() + if field == "display_name" and category not in TITLE_VISIBILITY_TOGGLES + } + assert set(DISPLAY_NAME_CATEGORIES) == curated + + +@pytest.mark.parametrize( + "category", + ["html", "poll_question", "scorm", "done", "google-document", "image"], +) +def test_no_visible_title_for_blocks_that_never_render_one(category): + """display_name stays a Studio-only label for these.""" + block = _block(category, display_name="Wk3 intro copy - REVISED, do not reuse") + assert get_visible_title(block) == "" + + +def test_unknown_block_type_has_no_visible_title(): + """An unlisted type is assumed author-only — omitting only costs wording.""" + block = _block("ol_openedx_chat_xblock", display_name="internal note") + assert get_visible_title(block) == "" + + +def test_ora_uses_its_own_title_field_not_display_name(): + """ORA renders `title`; `display_name` can drift and isn't on the page.""" + block = _block( + "openassessment", + title="Peer Review: Essay 2", + display_name="ORA copy v3 DO NOT REUSE", + ) + assert get_visible_title(block) == "Peer Review: Essay 2" + + +def test_survey_uses_block_name_not_display_name(): + """Survey deliberately keeps display_name for Studio and renders block_name.""" + block = _block( + "survey", + block_name="End of Week Check-in", + display_name="survey draft - unused", + ) + assert get_visible_title(block) == "End of Week Check-in" + + +def test_drag_and_drop_title_hidden_when_author_turns_it_off(): + """show_title is author-controlled, so the name isn't always on the page.""" + shown = _block("drag-and-drop-v2", display_name="Sort the steps", show_title=True) + hidden = _block("drag-and-drop-v2", display_name="Sort the steps", show_title=False) + assert get_visible_title(shown) == "Sort the steps" + assert get_visible_title(hidden) == "" + + +def test_missing_toggle_is_treated_as_hidden(): + """A runtime without the toggle field can't confirm it renders — don't leak.""" + block = _block("drag-and-drop-v2", display_name="Sort the steps") + assert get_visible_title(block) == "" + + +def test_lazy_translation_title_is_resolved(): + """XBlock defaults are often lazy proxies, not plain strings.""" + block = _block("problem", display_name=gettext_lazy("Blank Problem")) + assert get_visible_title(block) == "Blank Problem" + + +def test_non_string_title_does_not_break_block_rendering(): + """Most mapped blocks are third-party and can redefine a field in any + release. This runs inside student_view_aside, so it must not raise.""" + assert get_visible_title(_block("problem", display_name=1.0)) == "" + + +def test_discussion_is_not_treated_as_having_a_visible_title(): + """It renders an empty fragment off the LEGACY provider, so the name may + be on no page at all.""" + block = _block("discussion", display_name="Week 3 discussion") + assert get_visible_title(block) == "" + + +def test_blank_and_missing_titles_degrade_to_empty(): + """A missing or whitespace-only name falls back to block-type wording.""" + assert get_visible_title(_block("video", display_name=None)) == "" + assert get_visible_title(_block("video", display_name=" ")) == "" + assert get_visible_title(_block("video")) == "" diff --git a/src/ol_openedx_feedback/tests/utils.py b/src/ol_openedx_feedback/tests/utils.py index 533e07440..4d7c872f4 100644 --- a/src/ol_openedx_feedback/tests/utils.py +++ b/src/ol_openedx_feedback/tests/utils.py @@ -69,6 +69,7 @@ def setUp(self): self.aside_name = "ol_openedx_feedback" self.video_aside_instance = self.create_aside("video") self.problem_aside_instance = self.create_aside("problem") + self.html_aside_instance = self.create_aside("html") def create_aside(self, block_type): """ diff --git a/uv.lock b/uv.lock index 1df325649..f97faf8c0 100644 --- a/uv.lock +++ b/uv.lock @@ -2574,7 +2574,7 @@ requires-dist = [ [[package]] name = "ol-openedx-feedback" -version = "0.3.0" +version = "0.3.1" source = { editable = "src/ol_openedx_feedback" } dependencies = [ { name = "django", version = "5.2.17", source = { registry = "https://pypi.org/simple" }, marker = "python_full_version < '3.12'" },