diff --git a/cms/djangoapps/contentstore/toggles.py b/cms/djangoapps/contentstore/toggles.py index 28485b15da91..6e4699e97e10 100644 --- a/cms/djangoapps/contentstore/toggles.py +++ b/cms/djangoapps/contentstore/toggles.py @@ -773,3 +773,26 @@ def enable_outline_component_creation(course_key): Returns a boolean if the Add Component in Outline feature is enabled for the given course. """ return ENABLE_OUTLINE_COMPONENT_CREATION.is_enabled(course_key) + + +# .. toggle_name: contentstore.hard_cap_library_content_max_count +# .. toggle_implementation: CourseWaffleFlag +# .. toggle_default: False +# .. toggle_description: When enabled for a course (or org), Studio treats library_content / +# itembank max_count values above LIBRARY_CONTENT_MAX_COUNT_WARNING_THRESHOLD as a +# validation ERROR instead of a WARNING. Publish is still not blocked by the platform +# by default; this strengthens author-facing messaging for orgs that want a hard stance. +# .. toggle_use_cases: open_edx +# .. toggle_creation_date: 2026-08-06 +HARD_CAP_LIBRARY_CONTENT_MAX_COUNT = CourseWaffleFlag( + f'{CONTENTSTORE_NAMESPACE}.hard_cap_library_content_max_count', + __name__, + CONTENTSTORE_LOG_PREFIX, +) + + +def hard_cap_library_content_max_count(course_key): + """ + Return True if large library/item-bank max_count should surface as an ERROR in Studio. + """ + return HARD_CAP_LIBRARY_CONTENT_MAX_COUNT.is_enabled(course_key) diff --git a/cms/envs/common.py b/cms/envs/common.py index d330ad909dfa..eed64fa3245d 100644 --- a/cms/envs/common.py +++ b/cms/envs/common.py @@ -1652,6 +1652,17 @@ ######################## Setting for content libraries ######################## MAX_BLOCKS_PER_CONTENT_LIBRARY = 100_000 +# Studio warning when Randomized Content / Item Bank "Count" (max_count) exceeds this. +# Default 25; publish remains allowed (WARNING). Optional course waffle escalates to ERROR: +# contentstore.hard_cap_library_content_max_count +# .. setting_name: LIBRARY_CONTENT_MAX_COUNT_WARNING_THRESHOLD +# .. setting_default: 25 +# .. setting_description: Threshold for Studio validation when library_content / itembank +# max_count is high enough to risk slow learner loads. +LIBRARY_CONTENT_MAX_COUNT_WARNING_THRESHOLD = 25 +# Optional URL appended to the Studio warning (internal runbook). Empty = omit link text. +LIBRARY_CONTENT_LARGE_MAX_COUNT_HELP_URL = '' + ######################## Organizations ######################## # .. toggle_name: ORGANIZATIONS_AUTOCREATE diff --git a/docs/implementation_plans/assessments-not-loading-performance.md b/docs/implementation_plans/assessments-not-loading-performance.md new file mode 100644 index 000000000000..441d04d3208b --- /dev/null +++ b/docs/implementation_plans/assessments-not-loading-performance.md @@ -0,0 +1,358 @@ +# Implementation Plan: Large Library Content / Assessment Render Performance + +**Status:** Draft +**Branch (A1 in progress):** `mraman-2U/assessments-not-loading` +**Code owner:** Aurora (`lms.djangoapps.courseware`) +**Related incident:** IBM Cybfun mock exam vertical timeout — `render_xblock` ~77s, nginx 504 at 60s, `xb_user_state.get_many` ~36s for 359 block keys + +--- + +## Problem statement + +Large quiz verticals that use **Library Content** / **Item Bank** blocks can attach many candidate CAPA problems in modulestore while showing only a subset per learner (`max_count`). Today: + +1. **`FieldDataCache.add_block_descendents`** walks **`get_children()`** for every block, prefetching user state for **all** modulestore children—not only the learner’s selected subset (`get_child_blocks()`). +2. **`DjangoXBlockUserStateClient.get_many`** loads full JSON state for every requested key and parses it synchronously in the request thread. +3. **`render_xblock`** renders the entire vertical (90+ CAPA blocks) in one synchronous HTTP response, exceeding nginx upstream timeouts. + +The Learning MFE loads units via a **single iframe** to `/xblock/{vertical_id}`; there is no incremental problem loading today. + +--- + +## Goals + +| Goal | Target | +|------|--------| +| Reduce user-state prefetch breadth | `xb_user_state.get_many.blocks_requested` scales with `max_count`, not library size | +| Reduce user-state prefetch latency | p95 `get_many` duration **< 10s** for 90-problem configs (after A1–A3) | +| First meaningful paint | **< 5s** TTFB for large quiz verticals (after Phase 1–2) | +| Avoid new bad configs | Studio warning when `max_count` > threshold | +| No regression | Learner sees correct problem count; selection/analytics unchanged on first visit | + +--- + +## Non-goals (this plan) + +- Rewriting CAPA in React +- Per-problem iframes in Learning MFE (rejected pattern; browser perf) +- Fixing unrelated prod issues (e.g. `translatable_xblocks` 500s) unless they block testing +- edx-exams / special-exam registration (not root cause for graded library quizzes) + +--- + +## Implementation order (phases) + +```text +Phase A1 ──► Phase A2+A3 ──► Phase B1 ──► Phase B2 ──► Phase C +(FieldDataCache) (get_many) (shell API) (MFE lazy) (CMS guardrails) + │ │ │ │ + └──────────────┴──────────────┴──────────────┘ + LMS (edx-platform) frontend-app-learning +``` + +Each phase is independently shippable; later phases depend on earlier ones only for **full** UX improvement, not for correctness of A1. + +--- + +## Phase A1 — Dynamic children in `FieldDataCache` + +**Priority:** P0 — ship first +**Repos:** `edx-platform` only +**MFE changes:** None + +### Summary + +When building the descendant tree for user-state prefetch, use the same rule as `vertical_block.block_has_access_error`: for blocks with `has_dynamic_children()`, iterate **`get_child_blocks()`** instead of **`get_children()`**. + +### Deliverables + +- [ ] Helper `_children_for_field_data_cache(block)` in `lms/djangoapps/courseware/model_data.py` +- [ ] `add_block_descendents` uses helper when recursing +- [ ] Unit tests in `lms/djangoapps/courseware/tests/test_model_data.py` +- [ ] Devstack integration validator: `scripts/field_data_cache_integration/` + +### Key files + +| File | Change | +|------|--------| +| `lms/djangoapps/courseware/model_data.py` | Dynamic-child traversal | +| `lms/djangoapps/courseware/tests/test_model_data.py` | Unit + mock integration | +| `scripts/field_data_cache_integration/README.rst` | Manual validation recipe | + +### Rollout + +- No feature flag required (behavior aligns with render path). +- Optional flag `courseware.field_data_cache.use_dynamic_children` for conservative rollout if needed. +- Monitor Datadog: `xb_user_state.get_many.blocks_requested`, `.duration`, `render_xblock` p95. + +### Acceptance criteria + +- For vertical with library size 10, `max_count=2`: `blocks_requested` ≈ vertical + library_content + 2 problems (+ small overhead), **not** ~10+ problems. +- All existing `test_model_data`, `test_library_content`, `test_block_render` pass. +- Integration script reports **PASS (prefetch breadth)** on fix branch vs **FAIL** on baseline. + +### Estimated effort + +1 sprint (implementation + review + staging validation) + +--- + +## Phase A2 — SQL micro-optimizations for `get_many` + +**Priority:** P1 +**Repos:** `edx-platform` +**Depends on:** A1 merged (measurement baseline) + +### Summary + +Reduce cost per remaining `StudentModule` row fetched after A1 narrows the key set. + +### Deliverables + +- [ ] `.only('state', 'modified', 'module_state_key', 'course_id')` on prefetch query in `_get_student_modules` +- [ ] Single-query path when `len(usage_keys) <= STUDENTMODULE_BULK_QUERY_THRESHOLD` (e.g. 500) instead of always chunking +- [ ] Optional read-replica routing for read-only prefetch contexts (match pattern in `StudentModule.all_submitted_problems_read_only`) +- [ ] Extended metrics: `xb_user_state.get_many.db_ms` vs `parse_ms` (Datadog custom attributes) + +### Key files + +| File | Change | +|------|--------| +| `lms/djangoapps/courseware/user_state_client.py` | Query shaping, timing | +| `lms/djangoapps/courseware/models.py` | Optional read manager | + +### Acceptance criteria + +- Same functional behavior as A1; no change to selection or scores. +- p95 `get_many` duration improves measurably vs A1-only on 90-problem staging course. +- `EXPLAIN` on representative query uses `(student_id, course_id, module_state_key)` index. + +### Estimated effort + +0.5–1 sprint + +--- + +## Phase A3 — JSON parse optimizations + +**Priority:** P1 (can ship with A2) +**Repos:** `edx-platform` + +### Summary + +Reduce CPU time parsing ~121KB+ of CAPA state JSON per request. + +### Deliverables + +- [ ] Use `orjson` (or existing fast JSON path) in `get_many` hot loop +- [ ] Lazy parse in `UserStateCache`: store raw JSON string; parse dict on first field read +- [ ] (Optional) Pass `fields` from `_fields_to_cache` into `get_many` for blocks that do not need full state at render time + +### Key files + +| File | Change | +|------|--------| +| `lms/djangoapps/courseware/user_state_client.py` | Parse path | +| `lms/djangoapps/courseware/model_data.py` | `UserStateCache.cache_fields` | + +### Risks + +- CAPA `student_view` may require full state on first paint—audit before enabling field pruning globally. +- Lazy parse must preserve `set_many` / mutation semantics. + +### Acceptance criteria + +- Unit tests for cache hit/miss and field read after lazy load. +- No increase in `get_many` error rate; problem submission still works. + +### Estimated effort + +0.5–1 sprint (parallel with A2) + +--- + +## Phase B1 — Backend shell render + batch child API + +**Priority:** P2 +**Repos:** `edx-platform` +**Depends on:** A1–A3 recommended (reduces blast radius); not strictly required + +### Summary + +Split monolithic `render_xblock` into: + +1. **Shell response** — vertical chrome + placeholders (`render_mode=shell`) +2. **Batch child API** — render N CAPA blocks per request with `FieldDataCache` depth=0 per child + +Testable with **curl/Postman** before any MFE work. + +### Deliverables + +- [ ] Query param `render_mode=shell|full` on `render_xblock` (default `full`) +- [ ] Threshold: shell mode when descendant CAPA count > `settings.LARGE_VERTICAL_PROBLEM_THRESHOLD` (e.g. 20) and waffle enabled +- [ ] Template fragment `vert_module_lazy.html` + bootstrap JS posting `xblock.lazy.ready` to parent +- [ ] New API: `GET /api/courseware/v1/xblock_children/` with `parent_usage_key`, `child_usage_keys` (max 10), auth same as courseware +- [ ] OpenAPI / internal doc for batch endpoint +- [ ] curl examples in `scripts/field_data_cache_integration/` or API doc + +### Key files + +| File | Change | +|------|--------| +| `lms/djangoapps/courseware/views/views.py` | `render_mode` branch | +| `lms/djangoapps/courseware/block_render.py` | `render_xblock_children()` helper | +| `xmodule/item_bank_block.py` | `student_view_shell()` or branch in `student_view` | +| New DRF view + URLconf | Batch API | + +### Feature flags + +| Flag | Purpose | +|------|---------| +| `courseware.render_xblock.lazy_library_content` | Enable shell mode | +| `courseware.render_xblock.lazy_threshold` | Django setting / Waffle config | + +### Acceptance criteria + +- Shell `render_xblock` TTFB **< 5s** on 90-problem vertical in staging. +- Batch API returns valid CAPA HTML for each requested child key; 403 for keys not in learner’s selected set. +- Full `render_mode=full` unchanged when flag off. + +### Estimated effort + +2 sprints + +--- + +## Phase B2 — Learning MFE lazy load + +**Priority:** P2 +**Repos:** `frontend-app-learning`, `edx-platform` (coordination) +**Depends on:** B1 deployed to staging/prod + +### Summary + +Learning MFE continues single iframe per unit but orchestrates incremental child loading inside the iframe lifecycle. + +### Deliverables + +- [x] Unit iframe URL includes `render_mode=shell` when course metadata indicates large library quiz (new field from course blocks API or heuristic) +- [x] `postMessage` handler for `xblock.lazy.ready` with child usage key list +- [x] Batch fetch client (sequential or max 3 parallel) to `/api/courseware/v1/xblock_children/` +- [x] Skeleton UI + progress (“Loading question 12 of 90”) +- [x] iframe resize after each batch (reuse existing height postMessage) +- [x] E2E test or manual QA checklist + +### Key files (MFE) + +| Area | Change | +|------|--------| +| Courseware container / unit iframe loader | Shell URL, lazy orchestration | +| Course blocks / sequence metadata | Expose `has_large_library_content` or problem count | + +### Feature flag + +- `learning_mfe.enable_lazy_xblock_load` (MFE config + backend waffle) + +### Acceptance criteria + +- IBM-scale vertical loads in Learning MFE without nginx 504. +- Learner can answer and submit problems loaded via batch API. +- No regression on small units (flag off → current behavior). + +### Estimated effort + +2 sprints (cross-team) + +--- + +## Phase C — CMS guardrails + +**Priority:** P3 +**Repos:** `edx-platform` (CMS / xmodule) +**Can ship anytime** after A1; independent of B phases + +### Summary + +Prevent authors from creating new “90 problems in one vertical” configs without acknowledgment. + +### Deliverables + +- [x] `LegacyLibraryContentBlock.validate()` / `ItemBankBlock.validate()` warning when `max_count > 25` (threshold configurable) +- [x] Studio message: recommend splitting verticals or lowering count; link to internal runbook +- [x] (Optional) hard cap for net-new courses via org-level waffle + +### Key files + +| File | Change | +|------|--------| +| `xmodule/library_content_block.py` | Validation message | +| `xmodule/item_bank_block.py` | Same for v2 item bank | + +### Acceptance criteria + +- Saving block in Studio shows warning at threshold; publish still allowed. +- No change to existing published courses until edited. + +### Estimated effort + +0.5 sprint + +--- + +## Testing strategy + +| Layer | What | Where | +|-------|------|--------| +| Unit | Dynamic children, get_many, lazy cache | `test_model_data.py`, `test_user_state_client.py` | +| Integration | Prefetch breadth, events, DB | `scripts/field_data_cache_integration/validate_dynamic_children_prefetch.py` | +| Manual staging | IBM Cybfun or clone course, darfield-like learner | Datadog trace + nginx logs | +| Load | 90-problem vertical concurrent renders | k6 or internal load test (post A1) | +| E2E | MFE lazy load happy path | Phase B2 QA checklist | + +**Important:** HTTP 200 alone is insufficient. Validate `StudentModule` rows, `xb_user_state.get_many` metrics, and tracking events (`edx.librarycontentblock.content.assigned`). + +--- + +## Metrics & dashboards + +Add or watch in Datadog: + +- `xb_user_state.get_many.blocks_requested` +- `xb_user_state.get_many.duration` +- `xb_user_state.get_many.problem.blocks_out` +- `resource:render_xblock` p95 by `course_id` / org +- nginx `upstream_duration` for `/xblock/` 504 rate + +Success: IBM vertical p95 render **< 45s** after A1–A3; **< 5s** TTFB after B1–B2. + +--- + +## Rollout sequence (recommended) + +| Step | Action | Environment | +|------|--------|-------------| +| 1 | Merge A1 + run integration script | Devstack → staging | +| 2 | Deploy A1 to prod; monitor 48h | Prod | +| 3 | Merge A2+A3 | Staging → prod | +| 4 | Deploy B1 behind waffle; curl validate | Staging | +| 5 | Enable B1 for pilot org (IBM) | Prod | +| 6 | Ship B2 MFE for pilot org | Prod | +| 7 | Ship C guardrails | All envs | + +--- + +## Open questions + +1. **Prefetch + selection side effect:** `get_child_blocks()` during prefetch may invoke `selected_children()` on bound blocks—confirm whether A1 alone can fire `assigned` events during cache build; document or defer binding until after cache if needed. +2. **nginx timeout:** Raise `/xblock/` timeout as temporary ops mitigation, or rely solely on perf fixes? +3. **Item Bank v2 vs Legacy Library Content:** Same code paths via `ItemBankMixin`—confirm both in test matrix. +4. **Mobile apps:** Do they use same `/xblock/` iframe path or native CAPA? Scope B2 accordingly. + +--- + +## References + +- Incident trace: `trace_id:6a6b1de50000000084279e774be9f57c` (IBM vertical `8e077c86…`, user 64849306) +- Integration test: [`scripts/field_data_cache_integration/README.rst`](../../scripts/field_data_cache_integration/README.rst) +- Learning MFE unit iframe ADR: [frontend-app-learning ADR-0002](https://github.com/openedx/frontend-app-learning/blob/master/docs/decisions/0002-courseware-page-decisions.md) +- Vertical dynamic-child precedent: `xmodule/vertical_block.py` — `block_has_access_error` diff --git a/docs/implementation_plans/phase-c-studio-warning-sample.rst b/docs/implementation_plans/phase-c-studio-warning-sample.rst new file mode 100644 index 000000000000..78eb83f250b3 --- /dev/null +++ b/docs/implementation_plans/phase-c-studio-warning-sample.rst @@ -0,0 +1,27 @@ +Studio warning sample — Phase C (large library / item bank Count) +================================================================= + +When ``max_count`` (Studio field **Count**) exceeds +``LIBRARY_CONTENT_MAX_COUNT_WARNING_THRESHOLD`` (default **25**), Studio shows: + +**Type:** Warning (publish still allowed) + +**Message text:** + + This block is configured to show 90 problems to each learner. Large counts + in a single unit can cause slow loads or timeouts for learners. Split the + quiz across multiple units or verticals, or lower Count to 25 or fewer. + +**Action button:** Edit the configuration. + +Optional org hard-cap +--------------------- +With course waffle ``contentstore.hard_cap_library_content_max_count`` enabled, +the same case becomes an **Error** summary and appends: + + Your organization requires Count to be at most 25. + +Optional runbook URL +-------------------- +If ``LIBRARY_CONTENT_LARGE_MAX_COUNT_HELP_URL`` is set in Django settings, that +URL is appended to the message body. diff --git a/lms/djangoapps/courseware/block_render.py b/lms/djangoapps/courseware/block_render.py index 4317af8d8c5d..6b200b6504c1 100644 --- a/lms/djangoapps/courseware/block_render.py +++ b/lms/djangoapps/courseware/block_render.py @@ -51,6 +51,7 @@ from xmodule.partitions.partitions_service import PartitionService from xmodule.util.sandboxing import SandboxService from xmodule.services import EventPublishingService, RebindUserService, SettingsService, TeamsConfigurationService +from xmodule.x_module import STUDENT_VIEW from common.djangoapps.static_replace.services import ReplaceURLService from common.djangoapps.static_replace.wrapper import replace_urls_wrapper from lms.djangoapps.courseware.access import get_user_role, has_access @@ -855,10 +856,15 @@ def _get_block_by_usage_key(usage_key): def get_block_by_usage_id(request, course_id, usage_id, disable_staff_debug_info=False, course=None, - will_recheck_access=False): + will_recheck_access=False, field_data_cache_depth=None): """ Gets a block instance based on its `usage_id` in a course, for a given request/user + Arguments: + field_data_cache_depth: Optional depth for FieldDataCache descendant prefetch. + ``None`` prefetches all descendants (default). ``0`` caches only the block + itself. Used by shell-mode render and per-child batch loads. + Returns (instance, tracking_context) """ course_key = CourseKey.from_string(course_id) @@ -870,6 +876,7 @@ def get_block_by_usage_id(request, course_id, usage_id, disable_staff_debug_info course_key, user, block, + depth=field_data_cache_depth, read_only=CrawlersConfig.is_crawler(request), ) instance = get_block_for_descriptor( @@ -1075,3 +1082,179 @@ def append_data_to_webob_response(response, data): response_data.update(data) response.body = json.dumps(response_data).encode('utf-8') return response + + +def estimate_problem_descendant_count(block, depth=4): + """ + Estimate how many problem blocks a learner may see under ``block``. + + For dynamic blocks (library_content / item_bank), prefer ``max_count`` when set. + Otherwise walk static children up to ``depth`` levels. Used to decide whether + shell-mode render is eligible for a vertical. + """ + if depth < 0 or block is None: + return 0 + + has_dynamic = getattr(block, 'has_dynamic_children', None) + if callable(has_dynamic) and has_dynamic(): + max_count = getattr(block, 'max_count', None) + if isinstance(max_count, int) and max_count > 0: + return max_count + if max_count == -1: + return len(getattr(block, 'children', []) or []) + return len(getattr(block, 'children', []) or []) + + block_type = getattr(getattr(block, 'location', None), 'block_type', None) or getattr( + block, 'category', None + ) + if block_type == 'problem': + return 1 + + total = 0 + get_children = getattr(block, 'get_children', None) + if not callable(get_children): + return total + for child in get_children(): + total += estimate_problem_descendant_count(child, depth=depth - 1) + return total + + +def should_use_shell_render(course_key, requested_mode, block): + """ + Return True when render_xblock should use shell mode for ``block``. + + Requires an explicit ``render_mode=shell`` request, the lazy-library waffle + flag, and an estimated problem count above LARGE_VERTICAL_PROBLEM_THRESHOLD. + """ + from lms.djangoapps.courseware.toggles import courseware_lazy_library_content_is_enabled + + if requested_mode != 'shell': + return False + if not courseware_lazy_library_content_is_enabled(course_key): + return False + threshold = getattr(settings, 'LARGE_VERTICAL_PROBLEM_THRESHOLD', 20) + return estimate_problem_descendant_count(block) > threshold + + +def field_data_cache_depth_for_shell(block): + """ + Prefetch depth for shell-mode FieldDataCache. + + Vertical → depth 1 (vertical + library_content, not CAPA children). + Dynamic / leaf parents → depth 0 (parent only). + """ + block_type = getattr(getattr(block, 'location', None), 'block_type', None) + if block_type in ('vertical', 'unit'): + return 1 + return 0 + + +def render_xblock_children(request, parent_usage_key, child_usage_keys, course=None): + """ + Render a bounded list of child XBlocks for the batch children API. + + Validates that each child is in the learner-selected set for dynamic parents + (or in ``get_children()`` for static parents). Each child is loaded with + FieldDataCache depth=0. + + Returns: + dict with keys: + parent_usage_key (str) + results (list of {usage_key, html, resources}) + errors (list of {usage_key, error}) + + Raises: + Http404 / PermissionDenied-style errors via get_course_with_access paths + ValueError for invalid input (caller maps to 400) + """ + from lms.djangoapps.courseware.courses import get_course_with_access + + if not child_usage_keys: + raise ValueError('child_usage_keys is required') + + max_batch = getattr(settings, 'XBLOCK_CHILDREN_BATCH_MAX', 10) + if len(child_usage_keys) > max_batch: + raise ValueError(f'At most {max_batch} child_usage_keys are allowed per request') + + course_key = parent_usage_key.course_key + staff_access = bool(has_access(request.user, 'staff', course_key)) + + if course is None: + course = get_course_with_access(request.user, 'load', course_key, check_if_enrolled=True) + + _, request.user = setup_masquerade(request, course_key, staff_access) + + parent, _ = get_block_by_usage_id( + request, + str(course_key), + str(parent_usage_key), + course=course, + field_data_cache_depth=0, + will_recheck_access=True, + ) + + allowed_keys = _allowed_child_usage_keys(parent) + allowed_key_strs = {str(k) for k in allowed_keys} + results = [] + errors = [] + + for child_key in child_usage_keys: + child_key_str = str(child_key) + if child_key not in allowed_keys and child_key_str not in allowed_key_strs: + errors.append({ + 'usage_key': child_key_str, + 'error': 'forbidden', + 'message': 'Child is not in the learner-selected set for this parent.', + }) + continue + + try: + child, _ = get_block_by_usage_id( + request, + str(course_key), + child_key_str, + course=course, + field_data_cache_depth=0, + will_recheck_access=True, + ) + fragment = child.render(STUDENT_VIEW, context={}) + results.append({ + 'usage_key': child_key_str, + 'html': fragment.content, + 'resources': [ + {'kind': getattr(res, 'kind', None), 'data': getattr(res, 'data', None)} + for res in (fragment.resources or []) + ], + }) + except Http404: + errors.append({ + 'usage_key': child_key_str, + 'error': 'not_found', + 'message': 'Child block not found or access denied.', + }) + except Exception as exc: # pylint: disable=broad-except + log.exception('Failed to render child %s under %s', child_key_str, parent_usage_key) + errors.append({ + 'usage_key': child_key_str, + 'error': 'render_failed', + 'message': str(exc), + }) + + return { + 'parent_usage_key': str(parent_usage_key), + 'results': results, + 'errors': errors, + } + + +def _allowed_child_usage_keys(parent): + """ + Return the set of UsageKeys the current learner may load under ``parent``. + """ + get_child_blocks = getattr(parent, 'get_child_blocks', None) + has_dynamic = getattr(parent, 'has_dynamic_children', None) + if callable(has_dynamic) and has_dynamic() and callable(get_child_blocks): + children = get_child_blocks() + else: + children = parent.get_children() + return {child.location for child in children if child is not None} diff --git a/lms/djangoapps/courseware/model_data.py b/lms/djangoapps/courseware/model_data.py index 59fcc725ed36..80b97d034cad 100644 --- a/lms/djangoapps/courseware/model_data.py +++ b/lms/djangoapps/courseware/model_data.py @@ -333,6 +333,9 @@ def _cache_key_for_kvs_key(self, key): class UserStateCache: """ Cache for Scope.user_state xblock field data. + + Prefetched state may be a :class:`~LazyUserState` (raw JSON) until a field is + read via :meth:`get` / :meth:`has` / :meth:`delete`, which forces a parse. """ def __init__(self, user, course_id): self._cache = defaultdict(dict) @@ -349,12 +352,18 @@ def cache_fields(self, fields, xblocks, aside_types): # pylint: disable=unused- fields (list of str): Field names to cache. xblocks (list of :class:`XBlock`): XBlocks to cache fields for. aside_types (list of str): Aside types to cache fields for. + + Note: + Full-state field pruning via ``get_many(..., fields=...)`` is intentionally + not applied here. CAPA ``student_view`` commonly needs the full student + state on first paint; pruning would require a per-block-type audit. """ block_field_state = self._client.get_many( self.user.username, _all_usage_keys(xblocks, aside_types), ) for user_state in block_field_state: + # LazyUserState from get_many: JSON parse deferred until field access. self._cache[user_state.block_key] = user_state.state def set(self, kvs_key, value): @@ -408,7 +417,17 @@ def set_many(self, kv_dict): log.exception("Saving user state failed for %s", self.user.username) raise KeyValueMultiSaveError([]) # lint-amnesty, pylint: disable=raise-missing-from finally: - self._cache.update(pending_updates) + # Overlay onto existing cache entries (materializing lazy JSON first) + # so unread sibling fields remain available in-request after a partial write. + for cache_key, updates in pending_updates.items(): + existing = self._cache.get(cache_key) + if existing is None: + self._cache[cache_key] = dict(updates) + else: + ensure = getattr(existing, '_ensure_parsed', None) + if callable(ensure): + ensure() + existing.update(updates) def get(self, kvs_key): """ @@ -660,6 +679,29 @@ def _cache_key_for_kvs_key(self, key): return key.field_name +def _children_for_field_data_cache(block): + """ + Return child blocks whose field data should be prefetched for ``block``. + + Dynamic blocks such as library_content / item_bank expose a learner-specific + subset via ``get_child_blocks()``. Prefetching all modulestore children + (``get_children()``) loads user state for blocks that will never render for + the current learner. + + This mirrors ``vertical_block.block_has_access_error``. Note that + ``get_child_blocks()`` may invoke selection on ItemBankMixin-style blocks; + when the learner already has a stable ``selected`` set, that path does not + mutate state or emit assign events. See the assessments-not-loading plan + open question on prefetch-time selection side effects for first visits. + """ + get_child_blocks = getattr(block, 'get_child_blocks', None) + has_dynamic_children = getattr(block, 'has_dynamic_children', None) + if callable(has_dynamic_children) and has_dynamic_children() and callable(get_child_blocks): + return list(get_child_blocks()) + + return list(block.get_children()) + list(block.get_required_block_descriptors()) + + class FieldDataCache: """ A cache of django model objects needed to supply the data @@ -731,7 +773,7 @@ def add_block_descendents(self, block, depth=None, block_filter=lambda block: Tr should be cached """ - def get_child_blocks(block, depth, block_filter): + def collect_descendant_blocks(block, depth, block_filter): """ Return a list of all child blocks down to the specified depth that match the block filter. Includes `block` @@ -749,13 +791,13 @@ def get_child_blocks(block, depth, block_filter): if depth is None or depth > 0: new_depth = depth - 1 if depth is not None else depth - for child in block.get_children() + block.get_required_block_descriptors(): - blocks.extend(get_child_blocks(child, new_depth, block_filter)) + for child in _children_for_field_data_cache(block): + blocks.extend(collect_descendant_blocks(child, new_depth, block_filter)) return blocks with modulestore().bulk_operations(block.location.course_key): - blocks = get_child_blocks(block, depth, block_filter) + blocks = collect_descendant_blocks(block, depth, block_filter) self.add_blocks_to_cache(blocks) diff --git a/lms/djangoapps/courseware/tests/test_lazy_xblock_render.py b/lms/djangoapps/courseware/tests/test_lazy_xblock_render.py new file mode 100644 index 000000000000..d96eba3ba0a4 --- /dev/null +++ b/lms/djangoapps/courseware/tests/test_lazy_xblock_render.py @@ -0,0 +1,68 @@ +""" +Unit tests for Phase B1 shell/batch lazy render helpers. +""" +from unittest.mock import Mock, patch + +from django.test import TestCase, override_settings + +from lms.djangoapps.courseware.block_render import ( + estimate_problem_descendant_count, + field_data_cache_depth_for_shell, + should_use_shell_render, +) +from lms.djangoapps.courseware.toggles import COURSEWARE_LAZY_LIBRARY_CONTENT +from opaque_keys.edx.locator import CourseLocator + + +class TestShellRenderHelpers(TestCase): + """Tests for shell-mode eligibility helpers.""" + + def test_estimate_problem_count_uses_max_count_for_dynamic_blocks(self): + block = Mock() + block.has_dynamic_children.return_value = True + block.max_count = 90 + block.children = [Mock()] * 200 + assert estimate_problem_descendant_count(block) == 90 + + def test_estimate_problem_count_walks_static_vertical(self): + problem = Mock() + problem.has_dynamic_children.return_value = False + problem.location.block_type = 'problem' + problem.get_children.return_value = [] + + html = Mock() + html.has_dynamic_children.return_value = False + html.location.block_type = 'html' + html.get_children.return_value = [] + + vertical = Mock() + vertical.has_dynamic_children.return_value = False + vertical.location.block_type = 'vertical' + vertical.get_children.return_value = [html, problem, problem] + + assert estimate_problem_descendant_count(vertical) == 2 + + def test_field_data_cache_depth_for_shell(self): + vertical = Mock() + vertical.location.block_type = 'vertical' + assert field_data_cache_depth_for_shell(vertical) == 1 + + library = Mock() + library.location.block_type = 'library_content' + assert field_data_cache_depth_for_shell(library) == 0 + + @override_settings(LARGE_VERTICAL_PROBLEM_THRESHOLD=20) + def test_should_use_shell_render_requires_flag_mode_and_threshold(self): + course_key = CourseLocator('org', 'course', 'run') + block = Mock() + block.has_dynamic_children.return_value = True + block.max_count = 90 + + with patch.object(COURSEWARE_LAZY_LIBRARY_CONTENT, 'is_enabled', return_value=False): + assert should_use_shell_render(course_key, 'shell', block) is False + + with patch.object(COURSEWARE_LAZY_LIBRARY_CONTENT, 'is_enabled', return_value=True): + assert should_use_shell_render(course_key, 'full', block) is False + assert should_use_shell_render(course_key, 'shell', block) is True + block.max_count = 5 + assert should_use_shell_render(course_key, 'shell', block) is False diff --git a/lms/djangoapps/courseware/tests/test_model_data.py b/lms/djangoapps/courseware/tests/test_model_data.py index 6c763d57b338..00d38d2ea448 100644 --- a/lms/djangoapps/courseware/tests/test_model_data.py +++ b/lms/djangoapps/courseware/tests/test_model_data.py @@ -13,7 +13,14 @@ from xblock.fields import BlockScope, Scope, ScopeIds from common.djangoapps.student.tests.factories import UserFactory -from lms.djangoapps.courseware.model_data import DjangoKeyValueStore, FieldDataCache, InvalidScopeError +from lms.djangoapps.courseware.model_data import ( + DjangoKeyValueStore, + FieldDataCache, + InvalidScopeError, + UserStateCache, + _children_for_field_data_cache, +) +from lms.djangoapps.courseware.user_state_client import LazyUserState from lms.djangoapps.courseware.models import ( StudentModule, XModuleStudentInfoField, @@ -444,3 +451,221 @@ class TestStudentInfoStorage(OtherUserFailureTestMixin, StorageTestBase, TestCas storage_class = XModuleStudentInfoField other_key_factory = partial(DjangoKeyValueStore.Key, Scope.user_info, 2, 'mock_problem') # user_id=2, not 1 existing_field_name = "existing_field" + + +class TestFieldDataCacheDynamicChildren(TestCase): + """Tests for dynamic-child handling in FieldDataCache descendant prefetch.""" + + def test_children_for_field_data_cache_uses_get_child_blocks(self): + """ + Dynamic blocks should only expose learner-selected children for prefetch. + """ + selected_child = Mock(name='selected_child') + dynamic_block = Mock(name='dynamic_block') + dynamic_block.has_dynamic_children.return_value = True + dynamic_block.get_child_blocks.return_value = [selected_child] + dynamic_block.get_children.side_effect = AssertionError( + 'get_children should not be called for dynamic blocks' + ) + + assert _children_for_field_data_cache(dynamic_block) == [selected_child] + dynamic_block.get_child_blocks.assert_called_once_with() + dynamic_block.get_children.assert_not_called() + + def test_children_for_field_data_cache_uses_get_children_for_static_blocks(self): + """ + Static blocks should continue to prefetch all modulestore children. + """ + static_child = Mock(name='static_child') + required_child = Mock(name='required_child') + static_block = Mock(name='static_block') + static_block.has_dynamic_children.return_value = False + static_block.get_children.return_value = [static_child] + static_block.get_required_block_descriptors.return_value = [required_child] + + assert _children_for_field_data_cache(static_block) == [static_child, required_child] + static_block.get_children.assert_called_once_with() + static_block.get_required_block_descriptors.assert_called_once_with() + + def _configure_leaf_block(self, block, user_state_field): + """Configure a non-dynamic leaf mock used in descendant-walk tests.""" + block.get_children.return_value = [] + block.get_required_block_descriptors.return_value = [] + block.has_dynamic_children.return_value = False + block.fields.values.return_value = [user_state_field] + block.has_score = False + block.location = LOCATION('usage_id') + + @patch('lms.djangoapps.courseware.model_data.modulestore') + def test_add_block_descendents_prefetches_only_selected_dynamic_children(self, mock_modulestore): + """ + add_block_descendents should not walk unselected modulestore children. + """ + mock_modulestore.return_value.bulk_operations.return_value.__enter__ = Mock(return_value=None) + mock_modulestore.return_value.bulk_operations.return_value.__exit__ = Mock(return_value=False) + + user_state_field = mock_field(Scope.user_state, 'state') + + unselected_children = [Mock(name=f'unselected_{index}') for index in range(3)] + selected_children = [Mock(name='selected_0'), Mock(name='selected_1')] + for child in selected_children + unselected_children: + self._configure_leaf_block(child, user_state_field) + + library_content = Mock(name='library_content') + library_content.has_dynamic_children.return_value = True + library_content.get_child_blocks.return_value = selected_children + library_content.get_children.return_value = unselected_children + selected_children + library_content.get_required_block_descriptors.return_value = [] + library_content.fields.values.return_value = [user_state_field] + library_content.has_score = False + library_content.location = LOCATION('library_content') + + vertical = Mock(name='vertical') + vertical.has_dynamic_children.return_value = False + vertical.get_children.return_value = [library_content] + vertical.get_required_block_descriptors.return_value = [] + vertical.fields.values.return_value = [user_state_field] + vertical.has_score = False + vertical.location = LOCATION('vertical') + + user = UserFactory.create(username='dynamic_children_user') + field_data_cache = FieldDataCache([], COURSE_KEY, user) + + cached_blocks = [] + + def capture_cache_fields(fields, blocks, aside_types): # lint-amnesty, pylint: disable=unused-argument + cached_blocks.extend(blocks) + + with patch.object(UserStateCache, 'cache_fields', side_effect=capture_cache_fields): + field_data_cache.add_block_descendents(vertical) + + cached_block_names = {block._mock_name for block in cached_blocks} # pylint: disable=protected-access + assert 'vertical' in cached_block_names + assert 'library_content' in cached_block_names + assert 'selected_0' in cached_block_names + assert 'selected_1' in cached_block_names + assert 'unselected_0' not in cached_block_names + assert 'unselected_1' not in cached_block_names + assert 'unselected_2' not in cached_block_names + library_content.get_child_blocks.assert_called_once_with() + + @patch('lms.djangoapps.courseware.model_data.modulestore') + def test_add_block_descendents_recurses_nested_dynamic_and_static(self, mock_modulestore): + """ + Nested static containers under selected dynamic children should still be walked. + """ + mock_modulestore.return_value.bulk_operations.return_value.__enter__ = Mock(return_value=None) + mock_modulestore.return_value.bulk_operations.return_value.__exit__ = Mock(return_value=False) + + user_state_field = mock_field(Scope.user_state, 'state') + + nested_problem = Mock(name='nested_problem') + self._configure_leaf_block(nested_problem, user_state_field) + + nested_vertical = Mock(name='nested_vertical') + nested_vertical.has_dynamic_children.return_value = False + nested_vertical.get_children.return_value = [nested_problem] + nested_vertical.get_required_block_descriptors.return_value = [] + nested_vertical.fields.values.return_value = [user_state_field] + nested_vertical.has_score = False + nested_vertical.location = LOCATION('nested_vertical') + + unselected_nested = Mock(name='unselected_nested') + self._configure_leaf_block(unselected_nested, user_state_field) + + library_content = Mock(name='library_content') + library_content.has_dynamic_children.return_value = True + library_content.get_child_blocks.return_value = [nested_vertical] + library_content.get_children.return_value = [nested_vertical, unselected_nested] + library_content.get_required_block_descriptors.return_value = [] + library_content.fields.values.return_value = [user_state_field] + library_content.has_score = False + library_content.location = LOCATION('library_content') + + vertical = Mock(name='vertical') + vertical.has_dynamic_children.return_value = False + vertical.get_children.return_value = [library_content] + vertical.get_required_block_descriptors.return_value = [] + vertical.fields.values.return_value = [user_state_field] + vertical.has_score = False + vertical.location = LOCATION('vertical') + + user = UserFactory.create(username='nested_dynamic_children_user') + field_data_cache = FieldDataCache([], COURSE_KEY, user) + cached_blocks = [] + + def capture_cache_fields(fields, blocks, aside_types): # lint-amnesty, pylint: disable=unused-argument + cached_blocks.extend(blocks) + + with patch.object(UserStateCache, 'cache_fields', side_effect=capture_cache_fields): + field_data_cache.add_block_descendents(vertical) + + cached_block_names = {block._mock_name for block in cached_blocks} # pylint: disable=protected-access + assert cached_block_names == { + 'vertical', + 'library_content', + 'nested_vertical', + 'nested_problem', + } + library_content.get_child_blocks.assert_called_once_with() + nested_vertical.get_children.assert_called_once_with() + + +class TestUserStateCacheLazyParse(TestCase): + """Tests for lazy JSON handling inside UserStateCache.""" + databases = set(connections) + + def setUp(self): + super().setUp() + self.user = UserFactory.create(username='lazy_cache_user') + assert self.user.id # ensure persisted + self.usage_key = LOCATION('usage_id') + StudentModuleFactory( + student=self.user, + module_state_key=self.usage_key, + state=json.dumps({'a_field': 'a_value', 'b_field': 'b_value'}), + ) + self.block = mock_block([ + mock_field(Scope.user_state, 'a_field'), + mock_field(Scope.user_state, 'b_field'), + ]) + self.cache = UserStateCache(self.user, COURSE_KEY) + + def test_cache_fields_stores_lazy_state(self): + self.cache.cache_fields( + [mock_field(Scope.user_state, 'a_field')], + [self.block], + [], + ) + stored = self.cache._cache[self.usage_key] # pylint: disable=protected-access + assert isinstance(stored, LazyUserState) + assert not stored.is_parsed + + def test_get_parses_on_field_read(self): + self.cache.cache_fields([], [self.block], []) + stored = self.cache._cache[self.usage_key] # pylint: disable=protected-access + assert not stored.is_parsed + + value = self.cache.get(user_state_key('a_field')) + assert value == 'a_value' + assert stored.is_parsed + assert self.cache.get(user_state_key('b_field')) == 'b_value' + + def test_has_parses_for_membership(self): + self.cache.cache_fields([], [self.block], []) + assert self.cache.has(user_state_key('a_field')) + assert not self.cache.has(user_state_key('missing_field')) + + def test_set_many_overlays_without_dropping_sibling_fields(self): + self.cache.cache_fields([], [self.block], []) + self.cache.set(user_state_key('a_field'), 'new_value') + + assert self.cache.get(user_state_key('a_field')) == 'new_value' + # Sibling field must remain readable after partial write overlay. + assert self.cache.get(user_state_key('b_field')) == 'b_value' + + def test_delete_after_lazy_load(self): + self.cache.cache_fields([], [self.block], []) + self.cache.delete(user_state_key('b_field')) + assert not self.cache.has(user_state_key('b_field')) + assert self.cache.get(user_state_key('a_field')) == 'a_value' diff --git a/lms/djangoapps/courseware/tests/test_user_state_client.py b/lms/djangoapps/courseware/tests/test_user_state_client.py index 690a64b8b8c5..7e069244e95d 100644 --- a/lms/djangoapps/courseware/tests/test_user_state_client.py +++ b/lms/djangoapps/courseware/tests/test_user_state_client.py @@ -8,14 +8,17 @@ from xblock.fields import Scope from datetime import datetime from unittest import TestCase +from unittest.mock import patch from collections import defaultdict from django.db import connections from common.djangoapps.student.tests.factories import UserFactory from lms.djangoapps.courseware.user_state_client import ( DjangoXBlockUserStateClient, + LazyUserState, XBlockUserStateClient, - XBlockUserState + XBlockUserState, + loads_user_state, ) from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase # lint-amnesty, pylint: disable=wrong-import-order @@ -757,3 +760,185 @@ def test_multiple_history_entries(self): 2. Update the test in the other repo to align with the new functionality 3. Remove this override to re-enable the working test """ + + +class TestDjangoUserStateClientQueryShape(ModuleStoreTestCase): + """ + Tests for A2 get_many query shaping: single-query vs chunk fallback and field projection. + """ + # Tell Django to clean out all databases, not just default + databases = set(connections) + + def setUp(self): + super().setUp() + self.user = UserFactory.create() + self.client = DjangoXBlockUserStateClient(user=self.user) + self.course_key = CourseLocator('orgQ', 'courseQ', 'runQ') + + def _usage_key(self, index): + return BlockUsageLocator(self.course_key, 'problem', f'block{index}') + + def test_get_many_uses_single_query_under_threshold(self): + """Moderate key counts should use one StudentModule query, not chunked_filter.""" + keys = [self._usage_key(i) for i in range(3)] + for index, key in enumerate(keys): + self.client.set(self.user.username, key, {'a': index}) + + with self.assertNumQueries(1): + results = list(self.client.get_many(self.user.username, keys)) + + assert len(results) == 3 + by_key = {entry.block_key: entry.state for entry in results} + for index, key in enumerate(keys): + assert by_key[key] == {'a': index} + + def test_get_many_chunks_when_over_threshold(self): + """Large key counts should fall back to multiple chunked queries.""" + threshold = 3 + keys = [self._usage_key(i) for i in range(threshold + 2)] + for key in keys: + self.client.set(self.user.username, key, {'v': 1}) + + with self.settings(STUDENTMODULE_BULK_QUERY_THRESHOLD=threshold): + # Two chunks: [0,1,2] and [3,4] → two SELECT queries + with self.assertNumQueries(2): + results = list(self.client.get_many(self.user.username, keys)) + + assert len(results) == len(keys) + + def test_get_many_field_projection_parity(self): + """Projected .only() query must still return full state and modified.""" + key = self._usage_key(0) + self.client.set(self.user.username, key, {'answer': 42, 'attempts': 1}) + + results = list(self.client.get_many(self.user.username, [key])) + assert len(results) == 1 + assert results[0].state == {'answer': 42, 'attempts': 1} + assert results[0].updated is not None + assert results[0].block_key == key + + def test_get_many_fields_filter_still_works(self): + """API fields= filter remains unchanged with the optimized read path.""" + key = self._usage_key(1) + self.client.set(self.user.username, key, {'keep': True, 'drop': False}) + + results = list(self.client.get_many(self.user.username, [key], fields=['keep'])) + assert len(results) == 1 + assert results[0].state == {'keep': True} + + def test_delete_many_still_persists_with_full_model_load(self): + """ + delete_many must not use the read_only projection path, so saves remain safe. + """ + key = self._usage_key(2) + self.client.set(self.user.username, key, {'x': 1}) + self.client.delete_many(self.user.username, [key]) + assert not list(self.client.get_many(self.user.username, [key])) + + def test_get_many_uses_read_only_student_module_path(self): + """get_many should request the read_only projection; delete_many should not.""" + key = self._usage_key(0) + self.client.set(self.user.username, key, {'x': 1}) + + with patch.object( + self.client, + '_student_module_query', + wraps=self.client._student_module_query, # pylint: disable=protected-access + ) as wrapped: + list(self.client.get_many(self.user.username, [key])) + wrapped.assert_called() + assert wrapped.call_args.kwargs.get('read_only') is True + + wrapped.reset_mock() + self.client.delete_many(self.user.username, [key]) + wrapped.assert_called() + assert not wrapped.call_args.kwargs.get('read_only', False) + + def test_student_module_query_only_projects_get_many_fields(self): + """read_only querysets should SELECT the projected columns, not the full row.""" + keys = [self._usage_key(0)] + read_qs = self.client._student_module_query( # pylint: disable=protected-access + self.user.username, self.course_key, keys, read_only=True, + ) + write_qs = self.client._student_module_query( # pylint: disable=protected-access + self.user.username, self.course_key, keys, read_only=False, + ) + read_sql = str(read_qs.query) + write_sql = str(write_qs.query) + # Projected columns (module_state_key is stored as module_id). + assert 'state' in read_sql + assert 'modified' in read_sql + assert 'module_id' in read_sql + assert 'course_id' in read_sql + # Full-row path should still include columns omitted from the projection. + assert 'grade' in write_sql + assert 'module_type' in write_sql + # read_only path should not pull unused score columns. + assert 'grade' not in read_sql + assert 'module_type' not in read_sql + + +class TestLazyUserState(TestCase): + """Unit tests for LazyUserState and loads_user_state helpers.""" + + def test_loads_user_state_parses_object(self): + assert loads_user_state('{"a": 1, "b": "x"}') == {'a': 1, 'b': 'x'} + + def test_lazy_state_defers_parse_until_field_access(self): + lazy = LazyUserState('{"answer": 42, "attempts": 2}') + assert not lazy.is_parsed + assert lazy['answer'] == 42 + assert lazy.is_parsed + assert lazy['attempts'] == 2 + assert lazy == {'answer': 42, 'attempts': 2} + + def test_lazy_state_has_and_delete_force_parse(self): + lazy = LazyUserState('{"keep": true, "drop": false}') + assert 'keep' in lazy + assert lazy.is_parsed + del lazy['drop'] + assert lazy == {'keep': True} + + +class TestDjangoUserStateClientLazyParse(ModuleStoreTestCase): + """Integration tests for lazy JSON parse on the Django get_many path.""" + databases = set(connections) + + def setUp(self): + super().setUp() + self.user = UserFactory.create() + self.client = DjangoXBlockUserStateClient(user=self.user) + self.course_key = CourseLocator('orgL', 'courseL', 'runL') + + def _usage_key(self, index): + return BlockUsageLocator(self.course_key, 'problem', f'lazy{index}') + + def test_get_many_yields_lazy_state_until_read(self): + key = self._usage_key(0) + self.client.set(self.user.username, key, {'student_answers': {'1': 'A'}, 'score': 1}) + + results = list(self.client.get_many(self.user.username, [key])) + assert len(results) == 1 + state = results[0].state + assert isinstance(state, LazyUserState) + assert not state.is_parsed + assert state['score'] == 1 + assert state.is_parsed + assert state['student_answers'] == {'1': 'A'} + + def test_get_many_with_fields_parses_eagerly(self): + key = self._usage_key(1) + self.client.set(self.user.username, key, {'keep': 1, 'drop': 2}) + + results = list(self.client.get_many(self.user.username, [key], fields=['keep'])) + assert len(results) == 1 + assert results[0].state == {'keep': 1} + assert not isinstance(results[0].state, LazyUserState) + + def test_get_many_skips_empty_deleted_state_without_lazy_wrap(self): + """Deleted state should yield no rows and must not wrap as LazyUserState.""" + key = self._usage_key(2) + self.client.set(self.user.username, key, {'x': 1}) + self.client.delete_many(self.user.username, [key]) + + assert not list(self.client.get_many(self.user.username, [key])) diff --git a/lms/djangoapps/courseware/toggles.py b/lms/djangoapps/courseware/toggles.py index 4d3ba067162d..91a32d527bc4 100644 --- a/lms/djangoapps/courseware/toggles.py +++ b/lms/djangoapps/courseware/toggles.py @@ -181,6 +181,24 @@ f'{WAFFLE_FLAG_NAMESPACE}.unify_site_and_translation_language', __name__ ) +# .. toggle_name: courseware.render_xblock.lazy_library_content +# .. toggle_implementation: CourseWaffleFlag +# .. toggle_default: False +# .. toggle_description: Enable shell-mode render_xblock for large library_content / item_bank +# verticals. When enabled (and problem count exceeds LARGE_VERTICAL_PROBLEM_THRESHOLD), +# render_mode=shell returns placeholders instead of synchronously rendering every CAPA child. +# Children are then loaded via /api/courseware/v1/xblock_children/. +# .. toggle_use_cases: temporary, open_edx +# .. toggle_creation_date: 2026-08-06 +# .. toggle_target_removal_date: None +# .. toggle_warning: Requires Learning MFE (or curl client) support for xblock.lazy.ready / +# batch child loading (Phase B2). Full render_mode=full remains the default when this flag +# is off. +# .. toggle_tickets: assessments-not-loading +COURSEWARE_LAZY_LIBRARY_CONTENT = CourseWaffleFlag( + f'{WAFFLE_FLAG_NAMESPACE}.render_xblock.lazy_library_content', __name__ +) + def course_exit_page_is_active(course_key): return COURSEWARE_MICROFRONTEND_COURSE_EXIT_PAGE.is_enabled(course_key) @@ -210,6 +228,13 @@ def courseware_mfe_search_is_enabled(course_key=None): return COURSEWARE_MICROFRONTEND_SEARCH_ENABLED.is_enabled(course_key) +def courseware_lazy_library_content_is_enabled(course_key=None): + """ + Return whether shell/batch lazy loading for large library content is enabled. + """ + return COURSEWARE_LAZY_LIBRARY_CONTENT.is_enabled(course_key) + + def courseware_disable_navigation_sidebar_blocks_caching(course_key=None): """ Return whether the courseware.disable_navigation_sidebar_blocks_caching flag is on. diff --git a/lms/djangoapps/courseware/user_state_client.py b/lms/djangoapps/courseware/user_state_client.py index b85f20175ce8..901ef1343e4b 100644 --- a/lms/djangoapps/courseware/user_state_client.py +++ b/lms/djangoapps/courseware/user_state_client.py @@ -20,8 +20,36 @@ from edx_django_utils import monitoring as monitoring_utils from xblock.fields import Scope -from lms.djangoapps.courseware.models import BaseStudentModuleHistory, StudentModule +from common.djangoapps.util.query import use_read_replica_if_available +from lms.djangoapps.courseware.models import BaseStudentModuleHistory, StudentModule, chunks +# Prefer the fastest decoder available without adding new hard dependencies. +# orjson is optional; simplejson is already pinned in edx-platform requirements. +try: + import orjson as _fast_json + + def loads_user_state(raw): + """Parse user-state JSON with orjson when installed.""" + if raw is None: + return None + if isinstance(raw, str): + raw = raw.encode('utf-8') + return _fast_json.loads(raw) + +except ImportError: + try: + import simplejson as _fast_json + except ImportError: + import json as _fast_json # lint-amnesty, pylint: disable=wrong-import-order + + def loads_user_state(raw): + """Parse user-state JSON with simplejson/stdlib json.""" + if raw is None: + return None + return _fast_json.loads(raw) + +# Encoder stays on simplejson/stdlib: orjson.dumps returns bytes, while StudentModule.state +# is a text field and existing callers expect str. try: import simplejson as json except ImportError: @@ -30,6 +58,140 @@ log = logging.getLogger(__name__) +# When fetching fewer usage keys than this (per course), issue a single IN query +# instead of going through ChunkingManager.chunked_filter. Matches the default +# chunk size used by ChunkingManager. +STUDENTMODULE_BULK_QUERY_THRESHOLD = 500 + +# Columns needed to build XBlockUserState from StudentModule rows during get_many. +# Omitting grade/max_grade/module_type/etc. reduces row transfer for large prefetches. +STUDENTMODULE_GET_MANY_FIELDS = ( + 'state', + 'modified', + 'module_state_key', + 'course_id', +) + + +def _is_empty_user_state_json(raw): + """ + Return True when ``raw`` is a deleted/empty user-state sentinel. + + Avoids a full JSON parse for the common ``"{}"`` deleted-state marker. + """ + if raw is None: + return True + if not isinstance(raw, str): + return False + stripped = raw.strip() + return stripped in ('', '{}', 'null') + + +class LazyUserState(dict): + """ + Dict-like wrapper that defers JSON parsing until the first content access. + + Used by :meth:`DjangoXBlockUserStateClient.get_many` so FieldDataCache can + prefetch many blocks without paying JSON CPU cost for blocks whose fields + are never read during the request (common after A1 narrows the tree but + some siblings still never render). + + Once parsed, this object behaves as a normal ``dict``. Mutations + (``__setitem__``, ``update``, ``__delitem__``) force a parse first so + set/delete semantics stay consistent with eager dicts. + """ + __slots__ = ('_raw', '_parsed') + + def __init__(self, raw_json): + super().__init__() + # Use object.__setattr__ in case a future slots/dict mix changes behavior. + object.__setattr__(self, '_raw', raw_json) + object.__setattr__(self, '_parsed', False) + + @property + def is_parsed(self): + """Whether the underlying JSON has been decoded.""" + return object.__getattribute__(self, '_parsed') + + def _ensure_parsed(self): + """Decode raw JSON into this dict on first field access.""" + if object.__getattribute__(self, '_parsed'): + return + raw = object.__getattribute__(self, '_raw') + data = loads_user_state(raw) or {} + if data: + super().update(data) + object.__setattr__(self, '_raw', None) + object.__setattr__(self, '_parsed', True) + + def __bool__(self): + # Yielded states are pre-filtered as non-empty; avoid parsing for truthiness. + if not object.__getattribute__(self, '_parsed'): + return True + return super().__len__() > 0 + + def __getitem__(self, key): + self._ensure_parsed() + return super().__getitem__(key) + + def __setitem__(self, key, value): + self._ensure_parsed() + super().__setitem__(key, value) + + def __delitem__(self, key): + self._ensure_parsed() + super().__delitem__(key) + + def __contains__(self, key): + self._ensure_parsed() + return super().__contains__(key) + + def __iter__(self): + self._ensure_parsed() + return super().__iter__() + + def __len__(self): + self._ensure_parsed() + return super().__len__() + + def get(self, key, default=None): + self._ensure_parsed() + return super().get(key, default) + + def keys(self): + self._ensure_parsed() + return super().keys() + + def items(self): + self._ensure_parsed() + return super().items() + + def values(self): + self._ensure_parsed() + return super().values() + + def update(self, *args, **kwargs): + self._ensure_parsed() + return super().update(*args, **kwargs) + + def pop(self, key, *args): + self._ensure_parsed() + return super().pop(key, *args) + + def copy(self): + self._ensure_parsed() + return dict(self) + + def __eq__(self, other): + self._ensure_parsed() + return super().__eq__(other) + + def __repr__(self): + if not object.__getattribute__(self, '_parsed'): + raw = object.__getattribute__(self, '_raw') or '' + return f'{self.__class__.__name__}()' + return f'{self.__class__.__name__}({dict.__repr__(self)})' + class XBlockUserState(namedtuple('_XBlockUserState', ['username', 'block_key', 'state', 'updated', 'scope'])): """ @@ -46,6 +208,8 @@ class XBlockUserState(namedtuple('_XBlockUserState', ['username', 'block_key', ' * ``TYPE``: :class:`str` * ``ALL``: ``None`` state: A dict mapping field names to the values of those fields for this XBlock. + For :meth:`~DjangoXBlockUserStateClient.get_many` without ``fields``, this may be a + :class:`LazyUserState` that parses JSON on first access. updated: A :class:`datetime.datetime`. We guarantee that the fields that were returned in "state" have not been changed since this time (in UTC). @@ -267,27 +431,64 @@ def __init__(self, user=None): """ self.user = user - def _get_student_modules(self, username, block_keys): + def _bulk_query_threshold(self): + """Return the max usage-key count for a single StudentModule IN query.""" + return getattr( + settings, + 'STUDENTMODULE_BULK_QUERY_THRESHOLD', + STUDENTMODULE_BULK_QUERY_THRESHOLD, + ) + + def _student_module_query(self, username, course_key, usage_keys, *, read_only=False): + """ + Build a StudentModule queryset for ``username`` / ``course_key`` / ``usage_keys``. + + When ``read_only`` is True (prefetch / get_many), project only the columns + needed to build :class:`XBlockUserState` and prefer the read replica when + configured. Callers that mutate returned rows (e.g. delete_many) must leave + ``read_only`` False so the full model is loaded for safe saves. + """ + query = StudentModule.objects.filter( + student__username=username, + course_id=course_key, + module_state_key__in=usage_keys, + ) + if read_only: + query = query.only(*STUDENTMODULE_GET_MANY_FIELDS) + query = use_read_replica_if_available(query) + return query + + def _get_student_modules(self, username, block_keys, *, read_only=False): """ Retrieve the :class:`~StudentModule`s for the supplied ``username`` and ``block_keys``. Arguments: username (str): The name of the user to load `StudentModule`s for. block_keys (list of :class:`~UsageKey`): The set of XBlocks to load data for. + read_only (bool): If True, apply field projection and optional read-replica + routing suitable for get_many prefetch. Must be False when callers + will save the returned instances. """ course_key_func = attrgetter('course_key') by_course = itertools.groupby( sorted(block_keys, key=course_key_func), course_key_func, ) + threshold = self._bulk_query_threshold() for course_key, usage_keys in by_course: - query = StudentModule.objects.chunked_filter( - 'module_state_key__in', - usage_keys, - student__username=username, - course_id=course_key, - ) + usage_keys = list(usage_keys) + if len(usage_keys) <= threshold: + query = self._student_module_query( + username, course_key, usage_keys, read_only=read_only, + ) + else: + query = itertools.chain.from_iterable( + self._student_module_query( + username, course_key, chunk, read_only=read_only, + ) + for chunk in chunks(usage_keys, threshold) + ) for student_module in query: usage_key = student_module.module_state_key.map_into_course(student_module.course_id) @@ -364,33 +565,44 @@ def get_many(self, username, block_keys, scope=Scope.user_state, fields=None): # keep track of blocks requested self._nr_stat_accumulate('get_many', 'blocks_requested', len(block_keys)) - modules = self._get_student_modules(username, block_keys) + db_start = time() + # Materialize so DB time is separable from JSON parse time below. + modules = list(self._get_student_modules(username, block_keys, read_only=True)) + db_ms = (time() - db_start) * 1000 # milliseconds + self._nr_stat_accumulate('get_many', 'db_ms', db_ms) + + parse_ms = 0.0 for module, usage_key in modules: - if module.state is None: + if module.state is None or _is_empty_user_state_json(module.state): continue - state = json.loads(module.state) state_length = len(module.state) - # If the state is the empty dict, then it has been deleted, and so - # conformant UserStateClients should treat it as if it doesn't exist. - if state == {}: - continue - - # collect statistics for custom attribute reporting - self._nr_block_stat_increment('get_many', usage_key.block_type, 'blocks_out') - self._nr_block_stat_accumulate('get_many', usage_key.block_type, 'size', state_length) - total_block_count += 1 - - # filter state on fields if fields is not None: + # Field filtering requires a concrete dict. + parse_start = time() + state = loads_user_state(module.state) + parse_ms += (time() - parse_start) * 1000 + if not state: + continue state = { field: state[field] for field in fields if field in state } + else: + # Defer JSON CPU until a field is actually read (UserStateCache / KVS). + state = LazyUserState(module.state) + + # collect statistics for custom attribute reporting + self._nr_block_stat_increment('get_many', usage_key.block_type, 'blocks_out') + self._nr_block_stat_accumulate('get_many', usage_key.block_type, 'size', state_length) + total_block_count += 1 + yield XBlockUserState(username, usage_key, state, module.modified, scope) + self._nr_stat_accumulate('get_many', 'parse_ms', parse_ms) + # The rest of this method exists only to report custom attributes. finish_time = time() duration = (finish_time - evt_time) * 1000 # milliseconds @@ -457,7 +669,7 @@ def set_many(self, username, block_keys_to_state, scope=Scope.user_state): if student_module.state is None: current_state = {} else: - current_state = json.loads(student_module.state) + current_state = loads_user_state(student_module.state) num_fields_before = len(current_state) current_state.update(state) num_fields_after = len(current_state) @@ -517,7 +729,7 @@ def delete_many(self, username, block_keys, scope=Scope.user_state, fields=None) if fields is None: student_module.state = "{}" else: - current_state = json.loads(student_module.state) + current_state = loads_user_state(student_module.state) for field in fields: if field in current_state: del current_state[field] @@ -568,7 +780,7 @@ def get_history(self, username, block_key, scope=Scope.user_state): # If the state is serialized json, then load it if state is not None: - state = json.loads(state) + state = loads_user_state(state) # If the state is empty, then for the purposes of `get_history`, it has been # deleted, and so we list that entry as `None`. @@ -607,7 +819,9 @@ def iter_all_for_block(self, block_key, scope=Scope.user_state): page = p.page(page_number) for sm in page.object_list: - state = json.loads(sm.state) + if _is_empty_user_state_json(sm.state): + continue + state = loads_user_state(sm.state) if state == {}: continue @@ -642,7 +856,9 @@ def iter_all_for_course(self, course_key, block_type=None, scope=Scope.user_stat page = p.page(page_number) for sm in page.object_list: - state = json.loads(sm.state) + if _is_empty_user_state_json(sm.state): + continue + state = loads_user_state(sm.state) if state == {}: continue diff --git a/lms/djangoapps/courseware/views/views.py b/lms/djangoapps/courseware/views/views.py index 01a28de1e441..813921f91e56 100644 --- a/lms/djangoapps/courseware/views/views.py +++ b/lms/djangoapps/courseware/views/views.py @@ -154,7 +154,13 @@ from openedx.features.course_experience.utils import dates_banner_should_display from openedx.features.course_experience.waffle import ENABLE_COURSE_ABOUT_SIDEBAR_HTML -from ..block_render import get_block, get_block_by_usage_id, get_block_for_descriptor +from ..block_render import ( + field_data_cache_depth_for_shell, + get_block, + get_block_by_usage_id, + get_block_for_descriptor, + should_use_shell_render, +) from ..tabs import _get_dynamic_tabs from ..toggles import ( COURSEWARE_OPTIMIZED_RENDER_XBLOCK, @@ -1607,6 +1613,12 @@ def render_xblock(request, usage_key_string, check_if_enrolled=True, disable_sta f"Rendering of the xblock view '{nh3.clean(requested_view)}' is not supported." ) + requested_render_mode = request.GET.get('render_mode', 'full') + if requested_render_mode not in ('full', 'shell'): + return HttpResponseBadRequest( + f"Unsupported render_mode '{nh3.clean(requested_render_mode)}'." + ) + staff_access = bool(has_access(request.user, 'staff', course_key)) is_preview = request.GET.get('preview', '0') == '1' @@ -1642,6 +1654,15 @@ def render_xblock(request, usage_key_string, check_if_enrolled=True, disable_sta request.user, usage_key.course_key, request=request, only_if_mobile_app=True ) + # Decide shell vs full before binding so FieldDataCache depth can stay shallow. + try: + unbound_block = store.get_item(usage_key) + except (ItemNotFoundError, NoPathToItem) as exc: + raise Http404("Block not found.") from exc + use_shell = should_use_shell_render(course_key, requested_render_mode, unbound_block) + set_custom_attribute('render_mode', 'shell' if use_shell else 'full') + cache_depth = field_data_cache_depth_for_shell(unbound_block) if use_shell else None + # get the block, which verifies whether the user has access to the block. recheck_access = request.GET.get('recheck_access') == '1' block, _ = get_block_by_usage_id( @@ -1651,11 +1672,13 @@ def render_xblock(request, usage_key_string, check_if_enrolled=True, disable_sta disable_staff_debug_info=disable_staff_debug_info, course=course, will_recheck_access=recheck_access, + field_data_cache_depth=cache_depth, ) student_view_context = request.GET.dict() student_view_context['show_bookmark_button'] = request.GET.get('show_bookmark_button', '0') == '1' student_view_context['show_title'] = request.GET.get('show_title', '1') == '1' + student_view_context['render_mode'] = 'shell' if use_shell else 'full' is_learning_mfe = is_request_from_learning_mfe(request) # Right now, we only care about this in regards to the Learning MFE because it results diff --git a/lms/envs/common.py b/lms/envs/common.py index a43ec942c361..086b00f498ad 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -3394,6 +3394,17 @@ # Maximum number of rows to fetch in XBlockUserStateClient calls. Adjust for performance USER_STATE_BATCH_SIZE = 5000 +############### Settings for large vertical / library_content lazy render (B1) ##### +# When courseware.render_xblock.lazy_library_content is on and estimated problem +# count exceeds this threshold, render_mode=shell is honored. +LARGE_VERTICAL_PROBLEM_THRESHOLD = 20 +# Max child usage keys accepted by /api/courseware/v1/xblock_children/ +XBLOCK_CHILDREN_BATCH_MAX = 10 + +# Studio authoring guardrail (Phase C); also defined in cms/envs/common.py. +LIBRARY_CONTENT_MAX_COUNT_WARNING_THRESHOLD = 25 +LIBRARY_CONTENT_LARGE_MAX_COUNT_HELP_URL = '' + ############## Plugin Django Apps ######################### from edx_django_utils.plugins import get_plugin_apps, add_plugins # pylint: disable=wrong-import-position,wrong-import-order diff --git a/lms/templates/vert_module_lazy.html b/lms/templates/vert_module_lazy.html new file mode 100644 index 000000000000..e6a19bf8fdaa --- /dev/null +++ b/lms/templates/vert_module_lazy.html @@ -0,0 +1,146 @@ +<%page expression_filter="h"/> + +<%namespace name='static' file='/static_content.html'/> + +<%! +from django.utils.translation import gettext as _ + +from openedx.core.djangolib.js_utils import js_escaped_string +from openedx.core.djangolib.markup import HTML +%> + +%if unit_title and show_title: +

${unit_title}

+% endif + +% if show_bookmark_button: + <%include file='bookmark_button.html' args="bookmark_id=bookmark_id, is_bookmarked=bookmarked"/> +% endif + +
+% for idx, item in enumerate(items): +
+ % if item.get('content'): + ${HTML(item['content'])} + % else: +
+ ${_("Loading question {index} of {total}").format(index=idx + 1, total=len(items))} + +
+ % endif +
+% endfor +
+ +% if reset_button: +
+ +
+% endif + + diff --git a/openedx/core/djangoapps/courseware_api/tests/test_views.py b/openedx/core/djangoapps/courseware_api/tests/test_views.py index 1606d245c01f..bbc04e5a33b4 100644 --- a/openedx/core/djangoapps/courseware_api/tests/test_views.py +++ b/openedx/core/djangoapps/courseware_api/tests/test_views.py @@ -865,3 +865,37 @@ def test_public_course_affects_allow_anonymous(self, mock_check_public_access): response = self.client.get(self.url) assert response.status_code == 200 assert response.data['allow_anonymous'] is True + + +@skip_unless_lms +class XBlockChildrenApiTests(BaseCoursewareTests): + """Validation tests for the Phase B1 batch children API.""" + + def setUp(self): + super().setUp() + CourseEnrollment.enroll(self.user, self.course.id, 'audit') + self.children_url = '/api/courseware/v1/xblock_children/' + + def test_requires_parent_usage_key(self): + response = self.client.get(self.children_url) + assert response.status_code == 400 + assert 'parent_usage_key' in response.data.get('developer_message', '') + + def test_rejects_invalid_parent_key(self): + response = self.client.get(self.children_url, {'parent_usage_key': 'not-a-key'}) + assert response.status_code == 400 + + @override_settings(XBLOCK_CHILDREN_BATCH_MAX=2) + @mock.patch('lms.djangoapps.courseware.block_render.render_xblock_children') + def test_oversized_batch_returns_400(self, mock_render): + mock_render.side_effect = ValueError('At most 2 child_usage_keys are allowed per request') + parent = str(self.course.id.make_usage_key('vertical', 'v1')) + children = ','.join([ + str(self.course.id.make_usage_key('problem', f'p{i}')) + for i in range(3) + ]) + response = self.client.get(self.children_url, { + 'parent_usage_key': parent, + 'child_usage_keys': children, + }) + assert response.status_code == 400 diff --git a/openedx/core/djangoapps/courseware_api/urls.py b/openedx/core/djangoapps/courseware_api/urls.py index 12fdf8cff2b4..8be44b911886 100644 --- a/openedx/core/djangoapps/courseware_api/urls.py +++ b/openedx/core/djangoapps/courseware_api/urls.py @@ -21,6 +21,10 @@ re_path(fr'^celebration/{settings.COURSE_KEY_PATTERN}', views.Celebration.as_view(), name="celebration-api"), + # Phase B1: batch child render for large library_content / item_bank verticals + path('v1/xblock_children/', + views.XBlockChildren.as_view(), + name="courseware-xblock-children"), ] if getattr(settings, 'PROVIDER_STATES_URL', None): diff --git a/openedx/core/djangoapps/courseware_api/views.py b/openedx/core/djangoapps/courseware_api/views.py index ee37835b4841..c790ce438119 100644 --- a/openedx/core/djangoapps/courseware_api/views.py +++ b/openedx/core/djangoapps/courseware_api/views.py @@ -949,3 +949,105 @@ def post(self, request, course_key_string, *args, **kwargs): # lint-amnesty, py return Response(status=201 if created else 200) else: return Response(status=200) # just silently allow it + + +class XBlockChildren(DeveloperErrorViewMixin, APIView): + """ + Batch-render selected child XBlocks under a parent (library_content / item_bank / vertical). + + **Use Cases** + + After a shell-mode ``render_xblock`` response, the Learning MFE (or a curl client) + loads CAPA children in batches via this endpoint. + + **Example Request** + + GET /api/courseware/v1/xblock_children/?parent_usage_key=...&child_usage_keys=key1,key2 + + **Response Values** + + * parent_usage_key: echo of the parent key + * results: list of {usage_key, html, resources} for successfully rendered children + * errors: list of {usage_key, error, message} for forbidden / missing / failed children + + **Returns** + + * 200 on success (individual child failures appear in ``errors``) + * 400 for missing/invalid params or oversized batch + * 401/403 when unauthenticated or lacking course access + * 404 when the parent cannot be loaded + """ + + authentication_classes = ( + JwtAuthentication, + BearerAuthenticationAllowInactiveUser, + SessionAuthenticationAllowInactiveUser, + ) + permission_classes = (IsAuthenticated,) + http_method_names = ['get'] + + def get(self, request, *args, **kwargs): # lint-amnesty, pylint: disable=unused-argument + """Handle a GET request.""" + from django.http import Http404 + from lms.djangoapps.courseware.block_render import render_xblock_children + from lms.djangoapps.courseware.exceptions import CourseAccessRedirect + + parent_key_str = request.query_params.get('parent_usage_key') + child_keys_raw = request.query_params.get('child_usage_keys', '') + if not parent_key_str: + return Response( + {'developer_message': 'parent_usage_key is required'}, + status=status.HTTP_400_BAD_REQUEST, + ) + + try: + parent_usage_key = UsageKey.from_string(parent_key_str) + except InvalidKeyError: + return Response( + {'developer_message': 'Invalid parent_usage_key'}, + status=status.HTTP_400_BAD_REQUEST, + ) + + child_key_strings = [part.strip() for part in child_keys_raw.split(',') if part.strip()] + child_usage_keys = [] + for key_str in child_key_strings: + try: + child_usage_keys.append(UsageKey.from_string(key_str)) + except InvalidKeyError: + return Response( + {'developer_message': f'Invalid child_usage_key: {key_str}'}, + status=status.HTTP_400_BAD_REQUEST, + ) + + try: + course = get_course_with_access( + request.user, 'load', parent_usage_key.course_key, check_if_enrolled=True + ) + except CourseAccessRedirect: + raise NotFound() # lint-amnesty, pylint: disable=raise-missing-from + + try: + payload = render_xblock_children( + request, + parent_usage_key, + child_usage_keys, + course=course, + ) + except ValueError as exc: + return Response( + {'developer_message': str(exc)}, + status=status.HTTP_400_BAD_REQUEST, + ) + except Http404: + raise NotFound() # lint-amnesty, pylint: disable=raise-missing-from + + # If every requested child was forbidden, surface 403 for clearer client handling. + if ( + child_usage_keys + and not payload['results'] + and payload['errors'] + and all(err.get('error') == 'forbidden' for err in payload['errors']) + ): + return Response(payload, status=status.HTTP_403_FORBIDDEN) + + return Response(payload) diff --git a/scripts/field_data_cache_integration/B1_shell_batch_curl.rst b/scripts/field_data_cache_integration/B1_shell_batch_curl.rst new file mode 100644 index 000000000000..c5ac7cbc8693 --- /dev/null +++ b/scripts/field_data_cache_integration/B1_shell_batch_curl.rst @@ -0,0 +1,66 @@ +curl / Postman examples for Phase B1 shell + batch child API. + +Prerequisites +------------- +- LMS running (devstack or staging) +- Waffle flag ``courseware.render_xblock.lazy_library_content`` enabled for the course +- Vertical / library_content with estimated problems > LARGE_VERTICAL_PROBLEM_THRESHOLD (default 20) +- Learner enrolled; session cookie or JWT available + +1) Shell render (placeholders + xblock.lazy.ready) +------------------------------------------------- + +.. code-block:: bash + + # Replace USAGE_KEY with the vertical or library_content usage key. + curl -sS -c cookies.txt -b cookies.txt \\ + -H "Accept: text/html" \\ + "https://LMS_HOST/xblock/USAGE_KEY?view=student_view&render_mode=shell&recheck_access=1" \\ + | tee shell.html + + # Expect: + # - HTTP 200 + # - HTML containing class="vert-mod-lazy" and vert-lazy-placeholder divs + # - Inline script posting {type: "xblock.lazy.ready", child_usage_keys: [...]} + # - Custom attribute render_mode=shell in Datadog / New Relic when instrumented + + # Full mode (default) must still work when flag is off or render_mode omitted: + curl -sS -c cookies.txt -b cookies.txt \\ + "https://LMS_HOST/xblock/USAGE_KEY?view=student_view" + +2) Batch children API +--------------------- + +.. code-block:: bash + + # child_usage_keys from the shell HTML data-usage-key attributes (comma-separated, max 10). + curl -sS -c cookies.txt -b cookies.txt \\ + -H "Accept: application/json" \\ + "https://LMS_HOST/api/courseware/v1/xblock_children/?parent_usage_key=PARENT_KEY&child_usage_keys=CHILD1,CHILD2" + + # Expect JSON: + # { + # "parent_usage_key": "...", + # "results": [{"usage_key": "...", "html": "
...
", "resources": [...]}], + # "errors": [] + # } + + # Forbidden child (not in learner selected set) → errors[].error == "forbidden" + # (HTTP 403 if *all* requested children are forbidden) + + curl -sS -c cookies.txt -b cookies.txt \\ + "https://LMS_HOST/api/courseware/v1/xblock_children/?parent_usage_key=PARENT_KEY&child_usage_keys=UNSELECTED_KEY" + +3) Auth notes +------------- +- Browser session: log in via LMS, reuse cookies (``-c/-b cookies.txt``). +- JWT: ``Authorization: JWT `` (same stack as other /api/courseware/ endpoints). +- Enrollment required (``get_course_with_access``). + +4) Feature flag +--------------- +Enable in Django admin / waffle: + +- Flag: ``courseware.render_xblock.lazy_library_content`` +- Optional setting: ``LARGE_VERTICAL_PROBLEM_THRESHOLD`` (default 20) +- Optional setting: ``XBLOCK_CHILDREN_BATCH_MAX`` (default 10) diff --git a/scripts/field_data_cache_integration/validate_dynamic_children_prefetch.py b/scripts/field_data_cache_integration/validate_dynamic_children_prefetch.py new file mode 100644 index 000000000000..b90f95186368 --- /dev/null +++ b/scripts/field_data_cache_integration/validate_dynamic_children_prefetch.py @@ -0,0 +1,419 @@ +#!/usr/bin/env python +""" +Devstack integration validator for FieldDataCache dynamic-children prefetch (A1). + +Run inside LMS Django context: + + export FDC_TEST_COURSE_ID=... + export FDC_TEST_VERTICAL_KEY=... + export FDC_TEST_LIBRARY_CONTENT_KEY=... + export FDC_TEST_USERNAME=fdc_test_learner + python scripts/field_data_cache_integration/validate_dynamic_children_prefetch.py + +Optional: + export FDC_TEST_PASSWORD=edx + export FDC_TEST_RUN_HTTP=1 + export FDC_TEST_MAX_COUNT=2 # expected max_count from Studio (for assertions) + +See README.rst in this directory for full setup and pass criteria. +""" +from __future__ import annotations + +import json +import os +import sys +from contextlib import contextmanager +from dataclasses import dataclass, field +from typing import Any +from unittest.mock import patch + +# --------------------------------------------------------------------------- +# Django bootstrap when executed as a standalone script from edx-platform root +# --------------------------------------------------------------------------- +if 'django' not in sys.modules: + os.environ.setdefault('DJANGO_SETTINGS_MODULE', 'lms.envs.devstack') + import django # pylint: disable=wrong-import-order + + django.setup() + +from django.contrib.auth import get_user_model +from django.db import connection, reset_queries +from django.test import Client +from opaque_keys.edx.keys import UsageKey +from xmodule.modulestore.django import modulestore + +from lms.djangoapps.courseware.model_data import FieldDataCache +from lms.djangoapps.courseware.models import StudentModule +from lms.djangoapps.courseware.user_state_client import DjangoXBlockUserStateClient +from lms.djangoapps.courseware.block_render import get_block_for_descriptor +from common.djangoapps.student.models import CourseEnrollment + +User = get_user_model() + +REQUIRED_ENV = ( + 'FDC_TEST_COURSE_ID', + 'FDC_TEST_VERTICAL_KEY', + 'FDC_TEST_LIBRARY_CONTENT_KEY', + 'FDC_TEST_USERNAME', +) + + +@dataclass +class TestConfig: + course_id: str + vertical_key: str + library_content_key: str + username: str + password: str = 'edx' + run_http: bool = False + expected_max_count: int = 2 + + +@dataclass +class PrefetchMetrics: + blocks_requested: int = 0 + block_types_requested: dict[str, int] = field(default_factory=dict) + studentmodule_queries: int = 0 + published_events: list[tuple[Any, str, dict]] = field(default_factory=list) + + +def _load_config() -> TestConfig: + missing = [name for name in REQUIRED_ENV if not os.environ.get(name)] + if missing: + print('ERROR: Missing required environment variables:') + for name in missing: + print(f' - {name}') + print('\nSee scripts/field_data_cache_integration/README.rst') + sys.exit(1) + + return TestConfig( + course_id=os.environ['FDC_TEST_COURSE_ID'], + vertical_key=os.environ['FDC_TEST_VERTICAL_KEY'], + library_content_key=os.environ['FDC_TEST_LIBRARY_CONTENT_KEY'], + username=os.environ['FDC_TEST_USERNAME'], + password=os.environ.get('FDC_TEST_PASSWORD', 'edx'), + run_http=os.environ.get('FDC_TEST_RUN_HTTP', '').lower() in ('1', 'true', 'yes'), + expected_max_count=int(os.environ.get('FDC_TEST_MAX_COUNT', '2')), + ) + + +def _usage_key(key_str: str): + return UsageKey.from_string(key_str) + + +def snapshot_library_state(config: TestConfig) -> dict: + """Return StudentModule row snapshot for library_content + all problem rows for learner.""" + user = User.objects.get(username=config.username) + course_key = _usage_key(config.vertical_key).course_key + lc_key = _usage_key(config.library_content_key) + + lc_row = StudentModule.objects.filter( + student=user, + course_id=course_key, + module_state_key=lc_key, + ).first() + + lc_state = None + selected = None + if lc_row and lc_row.state: + lc_state = json.loads(lc_row.state) + selected = lc_state.get('selected') + + problem_rows = StudentModule.objects.filter( + student=user, + course_id=course_key, + module_type='problem', + ).values_list('module_state_key', 'state') + + return { + 'library_content_modified': lc_row.modified.isoformat() if lc_row else None, + 'library_content_state': lc_state, + 'selected': selected, + 'problem_row_count': problem_rows.count(), + 'problem_keys': [str(k) for k, _ in problem_rows], + } + + +def _count_modulestore_children(config: TestConfig) -> dict[str, int]: + """How many children modulestore exposes vs learner-selected subset.""" + store = modulestore() + course_key = _usage_key(config.vertical_key).course_key + lc_block = store.get_item(_usage_key(config.library_content_key)) + + modulestore_child_count = len(lc_block.get_children()) + selected_child_count = None + if hasattr(lc_block, 'get_child_blocks'): + try: + selected_child_count = len(lc_block.get_child_blocks()) + except Exception as exc: # pylint: disable=broad-except + selected_child_count = f'error: {exc}' + + return { + 'modulestore_children': modulestore_child_count, + 'get_child_blocks_count': selected_child_count, + 'max_count': getattr(lc_block, 'max_count', None), + } + + +@contextmanager +def _instrument_prefetch(config: TestConfig): + """Capture get_many breadth, ORM queries, and XBlock publish calls during prefetch.""" + metrics = PrefetchMetrics() + original_get_many = DjangoXBlockUserStateClient.get_many + + def tracking_get_many(self, username, block_keys, scope=None, fields=None): + keys = list(block_keys) + metrics.blocks_requested += len(keys) + for key in keys: + block_type = key.block_type + metrics.block_types_requested[block_type] = ( + metrics.block_types_requested.get(block_type, 0) + 1 + ) + yield from original_get_many(self, username, keys, scope=scope, fields=fields) + + published = [] + + def capture_publish(block, event_type, event_data): + published.append((block, event_type, event_data)) + + user = User.objects.get(username=config.username) + course_key = _usage_key(config.vertical_key).course_key + vertical = modulestore().get_item(_usage_key(config.vertical_key)) + + reset_queries() + old_debug = connection.force_debug_cursor + connection.force_debug_cursor = True + + try: + with patch.object(DjangoXBlockUserStateClient, 'get_many', tracking_get_many): + with patch('xmodule.x_module.XModuleMixin.publish', capture_publish): + with patch('xblock.core.XBlock.publish', capture_publish): + field_data_cache = FieldDataCache.cache_for_block_descendents( + course_key, + user, + vertical, + ) + metrics.studentmodule_queries = sum( + 1 for q in connection.queries if 'courseware_studentmodule' in q['sql'].lower() + ) + metrics.published_events = published + yield metrics, field_data_cache + finally: + connection.force_debug_cursor = old_debug + + +def run_prefetch_phase(config: TestConfig) -> PrefetchMetrics: + print('\n=== Phase 1: Prefetch-only (FieldDataCache.cache_for_block_descendents) ===') + before = snapshot_library_state(config) + print(f"Before — selected: {before['selected']!r}, problem rows: {before['problem_row_count']}") + + child_counts = _count_modulestore_children(config) + print(f"Library modulestore children: {child_counts['modulestore_children']}") + print(f"Library get_child_blocks (if bound): {child_counts['get_child_blocks_count']}") + print(f"Studio max_count: {child_counts['max_count']}") + + with _instrument_prefetch(config) as (metrics, _cache): + pass + + after = snapshot_library_state(config) + print(f"\nPrefetch metrics:") + print(f" get_many blocks_requested: {metrics.blocks_requested}") + print(f" by block_type: {metrics.block_types_requested}") + print(f" StudentModule SQL queries: {metrics.studentmodule_queries}") + assigned = [ + (evt, data) for _block, evt, data in metrics.published_events + if evt and evt.endswith('.assigned') + ] + print(f" publish events (assigned): {len(assigned)}") + for evt, data in assigned: + print(f" - {evt}: {data}") + + print(f"\nAfter — selected: {after['selected']!r}, problem rows: {after['problem_row_count']}") + if before['library_content_modified'] != after['library_content_modified']: + print(' NOTE: library_content StudentModule modified timestamp changed during prefetch.') + if before['selected'] != after['selected']: + print(' NOTE: selected field changed during prefetch (may happen on first visit).') + + # Assertions / guidance + # With the fix, requested keys should scale with max_count, not library size. + modulestore_children = child_counts['modulestore_children'] + expected_with_fix = config.expected_max_count * 3 + 5 # lc + problems + vertical overhead + baseline_threshold = max(modulestore_children + 5, config.expected_max_count * 5) + + if modulestore_children > config.expected_max_count and metrics.blocks_requested >= baseline_threshold: + print( + f"\nFAIL (prefetch breadth): blocks_requested={metrics.blocks_requested} suggests " + f"all modulestore children (~{modulestore_children}) were prefetched." + ) + elif metrics.blocks_requested <= expected_with_fix: + print( + f"\nPASS (prefetch breadth): blocks_requested={metrics.blocks_requested} " + f"(expected ~≤{expected_with_fix} with fix; library has {modulestore_children} candidates)." + ) + else: + print( + f"\nWARN (prefetch breadth): blocks_requested={metrics.blocks_requested} between " + f"fix expectation (~{expected_with_fix}) and full library (~{baseline_threshold}). " + f"Review block_types breakdown." + ) + + if assigned: + print('WARN (events): assigned events fired during prefetch-only phase — review output above.') + else: + print('PASS (events): no assigned events during prefetch-only phase.') + + return metrics + + +def run_bound_render_prefetch(config: TestConfig) -> PrefetchMetrics: + """ + Mirrors production more closely: bind block via get_block_for_descriptor, then + walk descendants the same way block_render does before student_view. + """ + print('\n=== Phase 2: Bound block prefetch (get_block_for_descriptor path) ===') + user = User.objects.get(username=config.username) + course_key = _usage_key(config.vertical_key).course_key + vertical = modulestore().get_item(_usage_key(config.vertical_key)) + + before = snapshot_library_state(config) + metrics = PrefetchMetrics() + original_get_many = DjangoXBlockUserStateClient.get_many + + def tracking_get_many(self, username, block_keys, scope=None, fields=None): + keys = list(block_keys) + metrics.blocks_requested += len(keys) + for key in keys: + block_type = key.block_type + metrics.block_types_requested[block_type] = ( + metrics.block_types_requested.get(block_type, 0) + 1 + ) + yield from original_get_many(self, username, keys, scope=scope, fields=fields) + + published = [] + + def capture_publish(block, event_type, event_data): + published.append((block, event_type, event_data)) + + client = Client() + client.force_login(user) + + reset_queries() + old_debug = connection.force_debug_cursor + connection.force_debug_cursor = True + + try: + with patch.object(DjangoXBlockUserStateClient, 'get_many', tracking_get_many): + field_data_cache = FieldDataCache.cache_for_block_descendents( + course_key, + user, + vertical, + ) + with patch('xmodule.x_module.XModuleMixin.publish', capture_publish): + instance = get_block_for_descriptor( + user, + client.request(), + vertical, + field_data_cache, + course_key, + ) + # Trigger selection/render path (production does this in render_xblock) + if instance is not None: + instance.render('student_view', context={}) + finally: + connection.force_debug_cursor = old_debug + + metrics.studentmodule_queries = sum( + 1 for q in connection.queries if 'courseware_studentmodule' in q['sql'].lower() + ) + metrics.published_events = published + + after = snapshot_library_state(config) + assigned = [evt for _b, evt, _d in metrics.published_events if evt and evt.endswith('.assigned')] + + print(f" get_many blocks_requested: {metrics.blocks_requested}") + print(f" by block_type: {metrics.block_types_requested}") + print(f" assigned events: {len(assigned)}") + print(f" selected after render: {after['selected']!r}") + + selected_len = len(after['selected'] or []) + if selected_len == config.expected_max_count: + print(f'PASS (control): selected length == max_count ({config.expected_max_count})') + else: + print( + f'WARN (control): selected length {selected_len} != expected max_count ' + f'{config.expected_max_count}' + ) + + if assigned: + print('PASS (control): assigned event(s) emitted on first full render (expected).') + else: + print('WARN (control): no assigned events on render — learner may already have selection.') + + return metrics + + +def run_http_smoke(config: TestConfig) -> None: + print('\n=== Phase 3: HTTP smoke (render_xblock) ===') + client = Client() + logged_in = client.login(username=config.username, password=config.password) + if not logged_in: + print('FAIL: Could not log in test user. Set FDC_TEST_PASSWORD.') + return + + url = f"/xblock/{config.vertical_key}" + params = { + 'view': 'student_view', + 'recheck_access': '1', + 'show_bookmark': '0', + 'show_title': '0', + } + response = client.get(url, params) + print(f" GET {url} -> {response.status_code}") + if response.status_code != 200: + print(f' FAIL: body snippet: {response.content[:500]!r}') + return + + # Rough count of CAPA problem wrappers in rendered HTML + problem_divs = response.content.count(b'xblock-student_view-problem') + print(f" problem student_view markers in HTML: {problem_divs}") + if problem_divs >= config.expected_max_count: + print('PASS (HTTP): vertical rendered with expected problem markers.') + else: + print( + f'WARN (HTTP): expected at least {config.expected_max_count} problem markers, ' + f'saw {problem_divs}.' + ) + + +def main() -> None: + config = _load_config() + print('FieldDataCache dynamic-children integration validation') + print(f" course: {config.course_id}") + print(f" vertical: {config.vertical_key}") + print(f" library_content: {config.library_content_key}") + print(f" user: {config.username}") + + user = User.objects.filter(username=config.username).first() + if not user: + print(f'ERROR: user {config.username!r} not found') + sys.exit(1) + + course_key = _usage_key(config.vertical_key).course_key + if not CourseEnrollment.is_enrolled(user, course_key): + print(f'ERROR: {config.username} is not enrolled in {course_key}') + sys.exit(1) + + run_prefetch_phase(config) + run_bound_render_prefetch(config) + + if config.run_http: + run_http_smoke(config) + else: + print('\n(Skipping HTTP phase — set FDC_TEST_RUN_HTTP=1 to enable)') + + print('\nDone. Compare blocks_requested between baseline branch and fix branch.') + print('See scripts/field_data_cache_integration/README.rst for pass criteria.') + + +if __name__ == '__main__': + main() diff --git a/xmodule/item_bank_block.py b/xmodule/item_bank_block.py index b53617e2c8e4..df138f5b2eff 100644 --- a/xmodule/item_bank_block.py +++ b/xmodule/item_bank_block.py @@ -65,7 +65,12 @@ class ItemBankMixin( max_count = Integer( display_name=_("Count"), - help=_("Enter the number of components to display to each student. Set it to -1 to display all components."), + help=_( + "Enter the number of components to display to each student. " + "Set it to -1 to display all components. " + "Very large counts in a single unit can cause slow loads; " + "prefer splitting quizzes across multiple units." + ), default=1, scope=Scope.settings, ) @@ -315,6 +320,10 @@ def student_view(self, context): # lint-amnesty, pylint: disable=missing-functi fragment = Fragment() contents = [] child_context = {} if not context else copy(context) + render_mode = (context or {}).get('render_mode', 'full') + + if render_mode == 'shell': + return self._student_view_shell(context) for child in self._get_selected_child_blocks(): if child is None: @@ -346,6 +355,32 @@ def student_view(self, context): # lint-amnesty, pylint: disable=missing-functi fragment.initialize_js('LibraryContentReset') return fragment + def _student_view_shell(self, context): + """ + Chromeless shell: emit placeholders for selected children without rendering CAPA HTML. + + Used when render_xblock is called with render_mode=shell for large library quizzes. + Posts ``xblock.lazy.ready`` so a parent (Learning MFE) can batch-load children. + """ + fragment = Fragment() + contents = [] + for child in self._get_selected_child_blocks(): + if child is None: + continue + contents.append({ + 'id': str(child.usage_key), + 'content': '', + }) + + fragment.add_content(self.runtime.service(self, 'mako').render_lms_template('vert_module_lazy.html', { + 'items': contents, + 'xblock_context': context, + 'show_bookmark_button': False, + 'parent_usage_key': str(self.location), + 'reset_button': False, + })) + return fragment + def studio_view(self, _context): """ Return the studio view. @@ -442,6 +477,81 @@ def get_selected_event_prefix(cls) -> str: """ raise NotImplementedError + def max_count_warning_threshold(self) -> int: + """Return Studio threshold for large max_count (default 25).""" + return int(getattr(settings, 'LIBRARY_CONTENT_MAX_COUNT_WARNING_THRESHOLD', 25)) + + def is_max_count_hard_capped(self) -> bool: + """ + Return True when org/course waffle escalates large max_count to ERROR. + + Lazy-imports CMS toggles so LMS imports of this module stay safe. + """ + try: + from cms.djangoapps.contentstore.toggles import ( # pylint: disable=import-outside-toplevel + hard_cap_library_content_max_count, + ) + course_key = getattr(self.location, 'course_key', None) + if course_key is None: + return False + return hard_cap_library_content_max_count(course_key) + except Exception: # pylint: disable=broad-except + # CMS not installed / waffle unavailable (e.g. some unit contexts). + return False + + def large_max_count_validation_message(self): + """ + Build StudioValidationMessage when max_count exceeds the performance threshold. + + Returns None when max_count is within limits (including -1 = show all). + """ + threshold = self.max_count_warning_threshold() + # max_count < 0 means "show all"; do not treat as a large explicit Count. + if self.max_count < 0 or self.max_count <= threshold: + return None + + help_url = getattr(settings, 'LIBRARY_CONTENT_LARGE_MAX_COUNT_HELP_URL', '') or '' + text = _( + "This block is configured to show {count} problems to each learner. " + "Large counts in a single unit can cause slow loads or timeouts for learners. " + "Split the quiz across multiple units or verticals, or lower Count to {threshold} or fewer." + ).format(count=self.max_count, threshold=threshold) + if help_url: + text = f"{text} {help_url}" + + if self.is_max_count_hard_capped(): + text = ( + f"{text} " + + _("Your organization requires Count to be at most {threshold}.").format( + threshold=threshold + ) + ) + message_type = StudioValidationMessage.ERROR + else: + message_type = StudioValidationMessage.WARNING + + return StudioValidationMessage( + message_type, + text, + action_class='edit-button', + action_label=_("Edit the configuration."), + ) + + def apply_large_max_count_validation(self, validation): + """ + Attach large-max_count Studio validation when applicable. + + Hard-cap (waffle) overwrites an existing summary with ERROR. + Otherwise only sets a WARNING when validation is still empty so more + specific configuration issues keep precedence. + """ + message = self.large_max_count_validation_message() + if message is None: + return validation + if message.type == StudioValidationMessage.ERROR or validation.empty: + validation.set_summary(message) + return validation + class ItemBankBlock(ItemBankMixin, XBlock): """ @@ -490,6 +600,7 @@ def validate(self): action_label=_("Edit the problem bank configuration.") ) ) + self.apply_large_max_count_validation(validation) return validation def author_view(self, context): diff --git a/xmodule/library_content_block.py b/xmodule/library_content_block.py index 52e33108027c..aeb08a93fdc9 100644 --- a/xmodule/library_content_block.py +++ b/xmodule/library_content_block.py @@ -367,6 +367,7 @@ def validate(self): ) ) + self.apply_large_max_count_validation(validation) return validation def source_library_values(self): diff --git a/xmodule/tests/test_item_bank.py b/xmodule/tests/test_item_bank.py index c27412c8b709..d5b99ad28ba1 100644 --- a/xmodule/tests/test_item_bank.py +++ b/xmodule/tests/test_item_bank.py @@ -167,6 +167,39 @@ def test_max_count_validation(self): assert len(self.item_bank.selected_children()) == 4 assert self.item_bank.validate() + def test_large_max_count_performance_warning(self): + """ + Phase C: warn when Count exceeds the configurable performance threshold. + Existing published content is unchanged until the block is edited/saved and validated. + """ + from django.test import override_settings + from edx_toggles.toggles.testutils import override_waffle_flag + from cms.djangoapps.contentstore.toggles import HARD_CAP_LIBRARY_CONTENT_MAX_COUNT + + # Threshold 2 with max_count=3 and 4 children → performance warning only + with override_settings(LIBRARY_CONTENT_MAX_COUNT_WARNING_THRESHOLD=2): + self.item_bank.max_count = 3 + assert len(self.item_bank.children) >= 3 + assert not (result := self.item_bank.validate()) + assert StudioValidationMessage.WARNING == result.summary.type + assert 'configured to show 3 problems' in result.summary.text + assert 'Split the quiz' in result.summary.text + + # Below threshold → clean + self.item_bank.max_count = 2 + assert self.item_bank.validate() + + # Hard-cap waffle escalates to ERROR + with override_waffle_flag(HARD_CAP_LIBRARY_CONTENT_MAX_COUNT, active=True): + self.item_bank.max_count = 3 + assert not (result := self.item_bank.validate()) + assert StudioValidationMessage.ERROR == result.summary.type + assert 'organization requires' in result.summary.text + + # -1 (show all) does not trigger the large-count guardrail + self.item_bank.max_count = -1 + assert self.item_bank.validate() + @patch( 'xmodule.modulestore.split_mongo.caching_descriptor_system.CachingDescriptorSystem.render', VanillaRuntime.render, diff --git a/xmodule/tests/test_library_content.py b/xmodule/tests/test_library_content.py index 092606142e1d..35f214798b11 100644 --- a/xmodule/tests/test_library_content.py +++ b/xmodule/tests/test_library_content.py @@ -319,6 +319,35 @@ def test_validation_of_course_libraries(self): self._sync_lc_block_from_library(upgrade_to_latest=True) assert self.lc_block.validate() + def test_large_max_count_performance_warning(self): + """ + Phase C: warn when Count exceeds the configurable performance threshold. + """ + from django.test import override_settings + from edx_toggles.toggles.testutils import override_waffle_flag + from cms.djangoapps.contentstore.toggles import HARD_CAP_LIBRARY_CONTENT_MAX_COUNT + + self._sync_lc_block_from_library() + assert len(self.lc_block.children) == 4 + + with override_settings(LIBRARY_CONTENT_MAX_COUNT_WARNING_THRESHOLD=2): + self.lc_block.max_count = 3 + result = self.lc_block.validate() + assert not result + assert StudioValidationMessage.WARNING == result.summary.type + assert 'configured to show 3 problems' in result.summary.text + assert 'Split the quiz' in result.summary.text + + self.lc_block.max_count = 1 + assert self.lc_block.validate() + + with override_waffle_flag(HARD_CAP_LIBRARY_CONTENT_MAX_COUNT, active=True): + self.lc_block.max_count = 3 + result = self.lc_block.validate() + assert not result + assert StudioValidationMessage.ERROR == result.summary.type + assert 'organization requires' in result.summary.text + def _assert_has_only_N_matching_problems(self, result, n): assert result.summary assert StudioValidationMessage.WARNING == result.summary.type