Skip to content

fix: apply library_content transformer to all ItemBankMixin xblocks #37830

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
MaferMazu wants to merge 3 commits into
base: master
Choose a base branch
from
+69 −25
Open

fix: apply library_content transformer to all ItemBankMixin xblocks #37830

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8eb4234
fix: apply library_content transformer to all ItemBankMixin xblocks
MaferMazu Dec 31, 2025
cca46cf
fix: update the write version and add fallback for old cache
MaferMazu Jan 2, 2026
68a362d
fix: use load_class because it is already cached
MaferMazu Feb 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
94 changes: 69 additions & 25 deletions lms/djangoapps/course_blocks/transformers/library_content.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,20 +1,24 @@
"""
Content Library Transformer.
Item Bank Transformer.

Transformers for handling item bank blocks (library_content, itembank, etc.)
that use ItemBankMixin for randomized content selection.
"""


import json
import logging

from eventtracking import tracker
from xblock.core import XBlock

from common.djangoapps.track import contexts
from lms.djangoapps.courseware.models import StudentModule
from openedx.core.djangoapps.content.block_structure.transformer import (
BlockStructureTransformer,
FilteringTransformerMixin
)
from xmodule.library_content_block import LegacyLibraryContentBlock # lint-amnesty, pylint: disable=wrong-import-order
from xmodule.item_bank_block import ItemBankMixin # lint-amnesty, pylint: disable=wrong-import-order
from xmodule.modulestore.django import modulestore # lint-amnesty, pylint: disable=wrong-import-order

from ..utils import get_student_module_as_dict
Expand All @@ -25,12 +29,15 @@
class ContentLibraryTransformer(FilteringTransformerMixin, BlockStructureTransformer):
"""
A transformer that manipulates the block structure by removing all
blocks within a library_content block to which a user should not
have access.
blocks within item bank blocks (library_content, itembank, etc.)
to which a user should not have access.

This transformer works with any XBlock that inherits from ItemBankMixin,
filtering children based on the selection logic defined by each block type.

Staff users are not to be exempted from library content pathways.
Staff users are not to be exempted from item bank pathways.
"""
WRITE_VERSION = 1
WRITE_VERSION = 2
READ_VERSION = 1

@classmethod
Expand Down Expand Up @@ -61,22 +68,33 @@ def summarize_block(usage_key):
"original_usage_version": str(orig_version) if orig_version else None,
}

# For each block check if block is library_content.
# If library_content add children array to content_library_children field
# For each block check if block uses ItemBankMixin (e.g., library_content, itembank).
# Mark these blocks and collect analytics data for their children.
for block_key in block_structure.topological_traversal(
filter_func=lambda block_key: block_key.block_type == 'library_content',
yield_descendants_of_unyielded=True,
):
xblock = block_structure.get_xblock(block_key)
for child_key in xblock.children:
summary = summarize_block(child_key)
block_structure.set_transformer_block_field(child_key, cls, 'block_analytics_summary', summary)
block_class = XBlock.load_class(block_key.block_type)
is_item_bank = block_class and issubclass(block_class, ItemBankMixin)

if is_item_bank:
block_structure.set_transformer_block_field(block_key, cls, 'is_item_bank_block', True)
xblock = block_structure.get_xblock(block_key)
for child_key in xblock.children:
summary = summarize_block(child_key)
block_structure.set_transformer_block_field(child_key, cls, 'block_analytics_summary', summary)

def transform_block_filters(self, usage_info, block_structure):
all_library_children = set()
all_selected_children = set()
for block_key in block_structure:
if block_key.block_type != 'library_content':
# Check if this block was marked as an ItemBankMixin block during collect
is_item_bank = block_structure.get_transformer_block_field(block_key, self, 'is_item_bank_block')
# Fallback for old cache that doesn't have the 'is_item_bank_block' field
if is_item_bank is None:
block_class = XBlock.load_class(block_key.block_type)
is_item_bank = block_class and issubclass(block_class, ItemBankMixin)
Comment on lines 92 to 95
Copy link
Contributor

@bradenmacdonald bradenmacdonald Jan 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this just temporary code? Do we need it at all? Or is this why READ_VERSION is left at 1? If so, for how long do we need to keep this code in here? (Same question below)

Copy link
Contributor Author

@MaferMazu MaferMazu Jan 27, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it is temporary code. I added to keep read_version at 1 (so we can use the old cache if available). If a course has an old cache, that section will execute. I added it to avoid changing the read_version to 2, which, in my understanding, forces the platform to discard all the xblock structure cache (and can cause issues in large instances).

Copy link
Contributor Author

@MaferMazu MaferMazu Feb 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

About how long: I found that the cache is regenerated by default every 24h, so if someone migrates to a version with this change in ~1 or 2 weeks, it should be safe to remove the fallback code. In the case of openedx I was thinking of 1 release. I added that fallback code to avoid discarding the entire xblock structure cache at once.

References:


if not is_item_bank:
continue
library_children = block_structure.get_children(block_key)
if library_children:
Expand All @@ -98,7 +116,12 @@ def transform_block_filters(self, usage_info, block_structure):

# Update selected
previous_count = len(selected)
block_keys = LegacyLibraryContentBlock.make_selection(selected, library_children, max_count)
# Get the cached block class to call make_selection
block_class = XBlock.load_class(block_key.block_type)
if not block_class:
logger.error('Failed to load block class for %s', block_key)
continue
block_keys = block_class.make_selection(selected, library_children, max_count)
selected = block_keys['selected']

# Save back any changes
Expand Down Expand Up @@ -128,7 +151,7 @@ def check_child_removal(block_key):
"""
Return True if selected block should be removed.

Block is removed if it is part of library_content, but has
Block is removed if it is a child of an item bank block, but has
not been selected for current user.
"""
if block_key not in all_library_children:
Expand Down Expand Up @@ -156,6 +179,12 @@ def format_block_keys(keys):
json_result.append(info)
return json_result

# Get the cached block class to call publish_selected_children_events
block_class = XBlock.load_class(location.block_type)
if not block_class:
logger.error('Failed to load block class for publishing events: %s', location)
return

def publish_event(event_name, result, **kwargs):
"""
Helper function to publish an event for analytics purposes
Expand All @@ -170,11 +199,12 @@ def publish_event(event_name, result, **kwargs):
context = contexts.course_context_from_course_id(location.course_key)
if user_id:
context['user_id'] = user_id
full_event_name = f"edx.librarycontentblock.content.{event_name}"
event_prefix = block_class.get_selected_event_prefix()
full_event_name = f"{event_prefix}.{event_name}"
with tracker.get_tracker().context(full_event_name, context):
tracker.emit(full_event_name, event_data)

LegacyLibraryContentBlock.publish_selected_children_events(
block_class.publish_selected_children_events(
block_keys,
format_block_keys,
publish_event,
Expand All @@ -184,12 +214,14 @@ def publish_event(event_name, result, **kwargs):
class ContentLibraryOrderTransformer(BlockStructureTransformer):
"""
A transformer that manipulates the block structure by modifying the order of the
selected blocks within a library_content block to match the order of the selections
made by the ContentLibraryTransformer or the corresponding XBlock. So this transformer
requires the selections for the randomized content block to be already
made either by the ContentLibraryTransformer or the XBlock.
selected blocks within item bank blocks (library_content, itembank, etc.)
to match the order of the selections made by the ContentLibraryTransformer or the
corresponding XBlock. This transformer requires the selections for the item bank block
to be already made either by the ContentLibraryTransformer or the XBlock.

This transformer works with any XBlock that inherits from ItemBankMixin.

Staff users are *not* exempted from library content pathways.
Staff users are *not* exempted from item bank pathways.
"""
WRITE_VERSION = 1
READ_VERSION = 1
Expand Down Expand Up @@ -217,7 +249,19 @@ def transform(self, usage_info, block_structure):
to match the order of the selections made and stored in the XBlock 'selected' field.
"""
for block_key in block_structure:
if block_key.block_type != 'library_content':
# Try to read from cache (collected by ContentLibraryTransformer)
is_item_bank = block_structure.get_transformer_block_field(
block_key,
ContentLibraryTransformer,
'is_item_bank_block'
)

# Fallback for old cache that doesn't have the 'is_item_bank_block' field
if is_item_bank is None:
block_class = XBlock.load_class(block_key.block_type)
is_item_bank = block_class and issubclass(block_class, ItemBankMixin)
Comment on lines 258 to 262
Copy link
Contributor

@bradenmacdonald bradenmacdonald Jan 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question about this [temporary?] code.


if not is_item_bank:
continue

library_children = block_structure.get_children(block_key)
Expand All @@ -228,7 +272,7 @@ def transform(self, usage_info, block_structure):
current_selected_blocks = {item[1] for item in state_dict.get('selected', [])}

# As the selections should have already been made by the ContentLibraryTransformer,
# the current children of the library_content block should be the same as the stored
# the current children of the item bank block should be the same as the stored
# selections. If they aren't, some other transformer that ran before this transformer
# has modified those blocks (for example, content gating may have affected this). So do not
# transform the order in that case.
Expand Down
Loading