Repository navigation
fix: serve all_blocks unfiltered to users who can access all blocks - #515
Conversation
Since the data researcher change (openedx#27935), a requesting user with instructor.research gets all_blocks served as themselves. Global staff also hold instructor.research, so staff callers stopped getting the unfiltered tree and started getting per-user transformers, including a random item bank (library_content) selection in place of the full pool. Serve as the requesting user only when they have instructor.research but can't access all blocks, so data researchers keep the access openedx#27935 added and course staff get the unfiltered tree again. The warehouse course structure export calls this endpoint as a staff service user, so it currently records one random selection of each library container's children and misses completions on the rest (ENT-12225). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🟢 Approval recommended
The permission logic matches the intended behavior and is covered by focused regression tests.
0 open findings
What changed in this PR
Updates all_blocks permission handling so eligible staff receive the unfiltered course tree while data researchers retain their per-user view.
Changes:
- Distinguishes data researchers from users who can access all blocks.
- Adds form tests for global staff and data researchers.
| File | Description |
|---|---|
lms/djangoapps/course_api/blocks/forms.py |
Corrects user selection for all_blocks. |
lms/djangoapps/course_api/blocks/tests/test_forms.py |
Verifies staff and researcher behavior. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
pwnage101
left a comment
pwnage101
left a comment
There was a problem hiding this comment.
Looking for a more robust and semantic implementation that results in only calling can_access_all_blocks() once.
Move the instructor.research check into _verify_all_blocks so that can_access_all_blocks is evaluated once and the order of the checks is explicit: users who can access all blocks get the unfiltered tree, data researchers who can't are served as themselves, and anyone else is denied. Behavior is unchanged from the previous commit. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
brobro10000
left a comment
brobro10000
left a comment
There was a problem hiding this comment.
Consolidated review (AI-assisted reviewer aid, not an approval)
Summary: The reorder is correct and minimal. The set of users allowed through all_blocks is unchanged (instructor.research ∪ staff access). Only the view served to staff-access users changes, which restores the pre-openedx#27935 behavior for global staff. I found no security or logic defects, and CI is green on 4c8e0c4. There are two inline comments: one blocking docstring fix and one deferrable test cleanup.
Needs a decision before merge
-
Accept the global-staff response change. With
user=None,_has_access_to_course(None, …)denies staff access (access.py#L736-L738), soVisibilityTransformerdrops staff-only blocks (visibility.py#L64-L73) and per-user filters are skipped. These in-repo callers change for global staff:- the instructor-dashboard BlockBrowser (client.js#L24)
- remote content-library import (courseware_import.py#L287), which will no longer export staff-only blocks when its OAuth user is global staff
Recommendation: accept it. It matches course staff and the pre-2021 behavior, and the description already discloses it. Worth confirming with the ENT-12225 requester that their consumer doesn't need staff-only blocks.
Non-blocking
2. Upstream. openedx/edx-platform master still has the research-first order (forms.py#L141-L143), so this fix exists only on the fork's release-ulmo. An upstream PR or tracking ticket would keep it from being lost at the next release rebase.
3. Endpoint-level regression test (deferrable). The form test locks in the mechanism (user is None), but nothing asserts the observable outcome. A view test modeled on test_no_user_staff_all_blocks (test_views.py#L274-L278) would close that gap: build an item bank (max_count=1, ≥2 children) and a visible_to_staff_only unit, request as admin_user, and assert that every bank child is present and the staff-only unit is absent. Fixture patterns: test_library_content.py#L29, test_visibility.py#L34-L39.
Process: the open changes-requested review on 343ddfeb asked for the shape that 4c8e0c4 now has, so it needs a re-review to clear. The title still says [WIP].
| Returns None (the unfiltered tree) for users who can access all blocks, and the requesting user for data | ||
| researchers who can't, so they see the blocks as themselves. | ||
| """ | ||
| # Verify all blocks can be accessed for the course. | ||
| if not permissions.can_access_all_blocks(requesting_user, course_key): | ||
| raise PermissionDenied( | ||
| "'{requesting_username}' does not have permission to access all blocks in '{course_key}'.".format( | ||
| requesting_username=requesting_user.username, | ||
| course_key=str(course_key), | ||
| ) | ||
| ) | ||
| # Check this before instructor.research: global staff hold both, and serving them as themselves applies | ||
| # per-user transformers, e.g. a random item bank selection instead of the full pool. |
There was a problem hiding this comment.
BLOCKING (one-line docstring fix)
The docstring and comment describe None as "the unfiltered tree", but VisibilityTransformer still runs for user=None and removes staff-only blocks (api.py#L84-L94, visibility.py#L64-L73, access.py#L736-L738). This function is where the access decision is made, so a reader would assume global staff now see everything, which is the opposite of the trade-off the PR description lays out.
Suggested wording: Returns None (the tree without per-user transformers; only VisibilityTransformer runs, so staff-only blocks are excluded, matching course staff) for users who can access all blocks, …. Optionally fix the same inaccuracy in the get_blocks docstring (api.py#L39-L40).
| self.form_data.pop('username') | ||
| self.assert_error('username', "This field is required unless all_blocks is requested.") | ||
|
|
||
| def test_all_blocks_by_staff_is_unfiltered(self): |
There was a problem hiding this comment.
DEFERRABLE (follow-up/tech debt)
This repeats the setup of test_no_user_all_blocks_true (L110-L116). Adding assert form.cleaned_data['user'] is None there covers global staff. The other group this reorder changes has no test: course staff (not global) who also hold the data-researcher role. They were served as themselves before and now get None. Suggest repurposing this test: add one user to both CourseStaffRole(self.course.id) and CourseDataResearcherRole(self.course.id) (same setup as L141-L147) and assert user is None.
Safe to defer: the change is test-only, the precedence branch is already exercised by the global-staff case, and CI is green.
With user=None, get_blocks runs only VisibilityTransformer, and staff access is denied for a missing user, so blocks visible to staff only are removed. The _verify_all_blocks and get_blocks docstrings described this as the unfiltered tree. Also assert user is None for global staff in the existing all_blocks test, and replace the duplicate global-staff test with one for course staff who also hold the data researcher role, the other group whose response this change affects. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Add view tests over a course with an item bank (max_count=1, two children) and a staff-only unit. Global staff requesting all_blocks get every item bank child, where before the fix they got one random selection. A data researcher still gets their own single selection. Neither sees the staff-only unit, since user=None still runs VisibilityTransformer. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Approving. I accept that global-staff all_blocks callers now lose staff-only blocks (same as course staff). Please confirm with the ENT-12225 consumer and open an upstream openedx ticket. Also, the title still says [WIP]: please drop it once this is final (or say if it is not), so it is clear the PR is ready to merge.
macdiesel
changed the title
Hamzah reviewed and we made Troy's requested change.
Description
BlockListGetForm._clean_requested_userserves anall_blocksrequest as the requesting user whenever that user hasinstructor.research. That branch was added in openedx#27935 so data researchers could useall_blocks. Global staff also holdinstructor.research(instructor/permissions.py:is_staff | HasRolesRule('data_researcher')), so since then global-staff callers get their own per-user view instead of the unfiltered tree. The most visible effect:LibraryContentTransformerdoesn't exempt staff, so an item bank (library_content/itembank) returns one random selection ofmax_countchildren for that user instead of the full pool.This change serves the request as the requesting user only when they have
instructor.researchbut can't access all blocks (permissions.can_access_all_blocks, i.e. course staff):user=None), as before fix: added access for data researcher in courseware openedx/openedx-platform#27935Who this affects: global-staff callers of
all_blocks, such as reporting and analytics integrations that export course structure, and the instructor dashboard's problem browser when the instructor is global staff. Course staff who aren't global staff already get this behavior.Behavior differences for those callers:
user=NoneonlyVisibilityTransformerruns, and it removes them, which is what course staff already seeall_blocksTesting instructions
lms/djangoapps/course_api/blocks/tests/test_forms.py: global staff +all_blocks→userisNone; a data researcher who isn't course staff → served as themselves.test_no_user_staff_all_blocks,test_extra_field_*,test_data_researcher_access) should still pass; the toy course has no staff-only blocks, so the staff responses are unchanged./api/courses/v1/blocks/?course_id=<course>&all_blocks=true&depth=all&requested_fields=children. The item bank should list every child, and repeat requests should return the same children.