fix(lti): add NRPS pagination link headers - #670
Conversation
|
Thanks for the pull request, @navinkarkera! 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. |
b5bdb7c to
8e74e84
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #670 +/- ##
==========================================
+ Coverage 97.84% 97.93% +0.09%
==========================================
Files 84 84
Lines 7918 8043 +125
==========================================
+ Hits 7747 7877 +130
+ Misses 171 166 -5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
feanil
left a comment
There was a problem hiding this comment.
@navinkarkera sorry this took so long, I took a look through it and found one ordering issue, otherwise the fix makes sense. We should also bump the version and add a changelog entry before this is ready to merge.
| self.attach_external_user_ids(data) | ||
|
|
||
| # Materialize members preserving dict insertion order. | ||
| members = list(data.values()) |
There was a problem hiding this comment.
This offset pagination isn't stable, because the underlying data has no defined order. compat.get_course_members → the platform's get_course_members (lms/djangoapps/course_api/api.py) builds its result dict by iterating two unordered querysets — CourseEnrollment.get_active_enrollments_in_course (which is .filter(...).select_related(...) with no .order_by()) and then CourseAccessRole.access_roles_in_course, appending role-only users afterward. So list(data.values()) has a DB-arbitrary order that can differ between the request for page 1 and the request for page 2 — meaning a client paging through can silently skip or duplicate members.
Since fixing it upstream alone wouldn't be enough (the role-only users are merged in after enrollments), I'd sort by a stable unique key here before slicing, e.g.:
members = sorted(data.values(), key=lambda m: m['id'])A test that pages through the whole set and asserts the union equals all members (no dupes, no gaps) would lock this in. Marking this as blocking.
There was a problem hiding this comment.
@feanil Nice catch! Fixed it and added a test.
Sort course members by ID before slicing paginated resposes and add regression coverage for changing source insertion order.
|
@feanil Bumped version and updated the changelog. This is ready to be merged. |
* fix(lti): add NRPS pagination link headers * refactor(lti): extract NRPS positive integer parsing * fix: lint issues * fix: unstable NRPS pagination ordering Sort course members by ID before slicing paginated resposes and add regression coverage for changing source insertion order. * chore: bump version and update changelog
Description
The LTI NRPS Context Membership endpoint (
GET /memberships) currently returns all course members in a single response.This PR adds manual pagination following the NRPS v2.0 specification:
limitquery param — maximum members per page. When absent, all members are returned (preserving backward compatibility).pagequery param — 1-indexed page number (default 1). Only consulted whenlimitis present and valid.Link: <url>; rel="next"header — emitted when more members remain after the current page. The URL carries the samelimitand an incrementedpage, and preserves any other query params present on the original request.Invalid/zero/negative
limitsilently falls back to no pagination. Invalid/zero/negativepagedefaults to page 1.Co-authored by: GPT 5.5
Related
limitand does not emitLinkheaders, causing IMS certification failure #669Test instructions
Prerequisites
lti_1p3_enable_nrps = True)1. Get the LTI Configuration ID
Open an LMS Django shell:
Note the
idof a config whose block has NRPS enabled. We'll call this<CONFIG_ID>.2. Create a Bearer Token (dev-only)
In the same shell session:
Copy the printed token. We'll call this
<TOKEN>.3. Test Without Pagination (Baseline)
Expected:
200membersLinkheader in the response4. Test First Page with
limitExpected:
200membersLinkheader present withrel="next"containinglimit=2&page=25. Test Second Page
Parse the
Linkheader URL from step 4 and call it:Expected:
200Linkheader if this is the last page6. Test Preserving Unrelated Params
Expected:
200Linkheader URL containsrole=instructoralongsidelimit=2&page=27. Test Invalid Limit (should return all members)
Expected:
200Linkheader8. Test Invalid Page (should default to page 1)
Expected:
200limit=2(page 1)