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'" },