Repository navigation
Conversation
BulkRoleCache.prefetch has two loops: one over legacy CourseAccessRole rows, which builds and caches a `compat_role` object, and one over openedx-authz compat roles, which iterates with `role` but was adding `compat_role` to the set instead. If the legacy loop never ran for any user, compat_role was unbound and the whole call raised UnboundLocalError. If it did run, the authz loop silently cached the last legacy role instead of the actual authz one. This broke the Gradebook (BulkRoleCache.prefetch is called from bulk_gradebook_view_context) for any course where a listed user had an authz compat role but no legacy CourseAccessRole, and any other caller of prefetch was equally affected. Add it instead of compat_role, and add regression tests covering a user with only an authz role and a mix of legacy/authz roles across different users in the same prefetch call. Fixes openedx-authz#514.
|
Thanks for the pull request, @efortish! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. DetailsWhere can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
|
We hit this on Verawood ( We moved course roles into openedx-authz with
With the one-line change in this PR, both bulk contexts cache the same roles as each user's own |
Description
Fixes [openedx-authz#514]
BulkRoleCache.prefetch()contains two loops: one processes legacyCourseAccessRolerecords and builds acompat_roleobject for each row, while the other processes Open edX AuthZ compatibility roles. However, the AuthZ loop iterates overrolebut mistakenly addscompat_roleto the cache instead.This caused two distinct issues:
UnboundLocalError: If no legacy roles were processed for any user in the batch,compat_rolewas never assigned, causingprefetch()to crash.This bug affected the Gradebook because
bulk_gradebook_view_context()callsBulkRoleCache.prefetch(). As a result, the Gradebook could fail for courses where a listed user had an AuthZ compatibility role but no legacyCourseAccessRole. Any other caller ofprefetch()was susceptible to the same issue.The fix replaces
compat_rolewithrolewhen adding the AuthZ role to the cache, ensuring that the correct role is stored regardless of whether legacy roles were processed.Testing
BulkRoleCachePrefetchTestCasetotest_roles.py, covering two scenarios:UnboundLocalError.ruff,pylint, andlint-importspass.common/djangoapps/studenttest suite: 431 passed, 284 skipped.Deadline
None.