fix: apply user time zone to course start-date filters - #39163
alezconsultant wants to merge 1 commit into
Conversation
|
Thanks for the pull request, @alezconsultant! 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. |
2ed2fd6 to
b45daf9
Compare
tbain
left a comment
There was a problem hiding this comment.
Approved, looks good to me, and, in a roundabout way Codex.
Codex raises the possible concern:
Bug. Invalid date parameters bypass validation when the user has no accessible courses. In course.py:1064-1069, the function returns before calling get_datetime_param, so malformed, bare-date, or timezone-naive values produce 200 with an empty result instead of the required 400. Parse the parameters before the early return and add a no-courses/no-access regression test.
However it feels like returning a 200 with an empty list is correct in the case it calls out. If there are no courses, that supersedes warning the end-user about a malformed date since there aren't any courses to even compare with.
Raw Codex output:
Details
Review verdict: Request changes.P2
- Bug. Invalid date parameters bypass validation when the user has no accessible courses. In
course.py:1064-1069, the function returns before callingget_datetime_param, so malformed, bare-date, or timezone-naive values produce200with an empty result instead of the required400. Parse the parameters before the early return and add a no-courses/no-access regression test.
The inclusive gte/lte filtering and timezone-aware comparisons otherwise satisfy the ticket’s boundary behavior. No security, database, or performance issues found.
Checks: changed files passed py_compile; local full tests and Ruff were unavailable. The PR’s public CI reports all 40 checks successful, and the PR includes AI attribution and manual testing steps.
|
@tbain |
Description
The
start_date_on_or_afterandstart_date_on_or_beforeparams ofGET /api/contentstore/v2/home/coursesnow take a full ISO 8601 datetime with a UTC offset orZ, for example2027-07-01T00:00:00+04:00. The filter compares them toCourseOverview.startas instants.start_date_on_or_afterincludes the instant it names, andstart_date_on_or_beforedoes too.The params used to take a plain
YYYY-MM-DDdate, and the backend treated that date as a UTC day. A user in another time zone who picked a local day could miss a course that Studio shows on that day. For example, a course starting at2027-06-30T22:00:00Zis July 1 in UTC+4, but the old filter only returned it for June 30.With the new format the caller decides where its own calendar day starts and ends. The frontend sends the start of the first picked day and the end of the last picked day, both in the user's offset.
A value without an offset is rejected with a 400 whose message asks for one. That covers a naive datetime such as
2027-07-01T00:00:00, a bare date, and an unparseable string. The backend does not assume UTC for a naive value, because that would reproduce the original bug without any visible error.Changes
get_datetime_paramincms/djangoapps/contentstore/api/views/utils.pyreplacesget_date_param. Nothing else used the old helper.CourseOverview.get_all_coursesfilters withstart__gteandstart__lteon the given datetimes.get_courses_accessible_to_userand theHomePageCoursesViewV2API docs use the new format.There is no model change and no migration.
Breaking change
Clients that send
YYYY-MM-DDnow get a 400. The frontend that builds this filter (the Competency Management course search) must send datetimes with an offset. A literal+in a query string decodes to a space, so it has to be percent-encoded as%2B.Testing instructions
Automated tests, run in the Tutor devstack cms container with
--ds=cms.envs.test:cms/djangoapps/contentstore/rest_api/v2/views/tests/test_home.py: 34 passed.test_start_date_range_compares_instants_using_offsetcreates three courses and asserts that a one-day range sent with a+04:00offset returns different courses than the same wall-clock range sent as UTC. A ddt test asserts a 400 for malformed values, bare dates, and naive datetimes.openedx/core/djangoapps/content/course_overviews/tests/test_course_overviews.py:test_get_all_courses_by_start_date_offset_instantsandtest_get_all_courses_by_start_date_us_timezonecover both sides of UTC, plus a boundary test at the exact instant.cms/djangoapps/contentstore/tests/test_course_listing.py: the params reachget_all_coursesthroughget_courses_accessible_to_user.ruff checkis clean on all changed files.pylintwas not run.Manual check against the devstack Studio, with four courses that start at
2040-01-01T00:00:00Z:ZOther information
Related to openedx/openedx-core#842
🤖 Generated with Claude Code