Unit extensions API v2 for instructors dashboard #37783
Conversation
openedx-webhooks
added
the
open-source-contribution
|
Thanks for the pull request, @holaontiveros! 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. |
mphilbrick211
added
the
mao-onboarding
holaontiveros
force-pushed
the
feature/unit_extensions_v2
branch
from
b4b3387 to
55eef0e
Compare
holaontiveros
changed the title
| cls.chapter = BlockFactory.create( | ||
| parent=cls.course, | ||
| category='chapter', | ||
| display_name='Test Chapter' | ||
| ) | ||
| cls.due_date = datetime(2024, 12, 31, 23, 59, 59, tzinfo=UTC) | ||
| cls.subsection = BlockFactory.create( | ||
| parent=cls.chapter, | ||
| category='sequential', | ||
| display_name='Homework 1', | ||
| due=cls.due_date | ||
| ) | ||
| cls.vertical = BlockFactory.create( | ||
| parent=cls.subsection, | ||
| category='vertical', | ||
| display_name='Test Vertical' | ||
| ) | ||
| cls.problem = BlockFactory.create( | ||
| parent=cls.vertical, | ||
| category='problem', | ||
| display_name='Test Problem' | ||
| ) |
There was a problem hiding this comment.
Unless I missed something, other than the course/course_key, these other objects don't contribute to the tests. It looks like the two tests (test_combined_filters and test_filter_by_block_id) that return a non-empty results list are using mocks to supply the data. (I think you intended for more tests to return data, so I would suggest digging into these further to make sure we're testing what we're expecting).
I think that tests that use data setup in the database would be more valuable than mocking, so I would encourage switching over to that unless we can't avoid it.
| self.assertGreaterEqual(len(results), 0) | ||
|
|
||
| # If there are results, verify the structure | ||
| if results: |
There was a problem hiding this comment.
Do we expect data for this test? When I ran it results was empty, so I think we need more clarity on this test.
| @patch('lms.djangoapps.instructor.views.api_v2.get_units_with_due_date') | ||
| def test_filter_by_email_or_username(self, mock_get_units, mock_get_overrides): | ||
| """ | ||
| Test filtering unit extensions by email or username. |
There was a problem hiding this comment.
In practice, this doesn't reach the part of the code that filters based on username/email, because it hits the conditional
if location not in units_dict:
continuethe conditional evaluates to True and it continues.
| url = f"{self._get_url()}?{urlencode(params)}" | ||
| response = self.client.get(url) |
There was a problem hiding this comment.
I believe you can just pass params as the second argument to client.get.
| return [] | ||
| else: | ||
| # Use the course-wide logic from get_overrides_for_course | ||
| query_data = edx_when_api.get_overrides_for_course(course.id) |
There was a problem hiding this comment.
If get_overrides_for_course had the same return structure as get_overrides_for_block, I feel like we could clean up the duplication in the if/else blocks. Is it worth adding something (maybe a get_overrides_for_course_v2) to edx-when that does that? Since the two blocks are fairly close, are we able to combine them somehow?
There was a problem hiding this comment.
I agree with the suggestion.
We might need an adapter function like this o normalize both responses into the same structure:
def normalize_override_tuple(source, row):
if source == "block":
username, fullname, due_date, email, location = row
elif source == "course":
username, fullname, email, location, due_date = row
else:
raise ValueError("Unknown source")
return {
"username": username,
"full_name": fullname,
"email": email,
"location": location,
"due_date": due_date,
}
and the usage could be:
query_data = edx_when_api.get_overrides_for_block(course.id, unit.location)
extension_data = [normalize_override_tuple("block", row) for row in query_data]
query_data = edx_when_api.get_overrides_for_course(course.id)
extension_data = [normalize_override_tuple("course", row) for row in query_data]
| 'extended_due_date': due_date, | ||
| }) | ||
|
|
||
| except (InvalidKeyError): |
There was a problem hiding this comment.
If find_unit is the only place this error is thrown, I would recommend just wrapping that line. Also, you can just write except InvalidKeyError, since there is only one exception we're catching here.
|
|
||
| return extension_data | ||
|
|
||
| def list(self, request, *args, **kwargs): |
There was a problem hiding this comment.
Perhaps rather than override list, we could just override filter_queryset to return the queryset passed into it.
| permission_name = permissions.VIEW_DASHBOARD | ||
| serializer_class = UnitExtensionSerializer | ||
|
|
||
| @apidocs.schema( |
There was a problem hiding this comment.
It sounds like apidocs.schema_for (docs) might be more appropriate in this case.
| """ | ||
| **Use Cases** | ||
| Retrieve a paginated list of due date extensions for units in a course. |
There was a problem hiding this comment.
It looks like the autogenerated swagger docs pick up the first line of the docstring by default, so this should be the first line rather than **Use Cases**.
| extension_data = [] | ||
| for username, fullname, due_date, email, location in query_data: | ||
| # Apply email_or_username filter if specified | ||
| if email_or_username_filter and email_or_username_filter.lower() not in username.lower(): |
There was a problem hiding this comment.
Was the email filter missed here?
| return [] | ||
| else: | ||
| # Use the course-wide logic from get_overrides_for_course | ||
| query_data = edx_when_api.get_overrides_for_course(course.id) |
There was a problem hiding this comment.
I agree with the suggestion.
We might need an adapter function like this o normalize both responses into the same structure:
def normalize_override_tuple(source, row):
if source == "block":
username, fullname, due_date, email, location = row
elif source == "course":
username, fullname, email, location, due_date = row
else:
raise ValueError("Unknown source")
return {
"username": username,
"full_name": fullname,
"email": email,
"location": location,
"due_date": due_date,
}
and the usage could be:
query_data = edx_when_api.get_overrides_for_block(course.id, unit.location)
extension_data = [normalize_override_tuple("block", row) for row in query_data]
query_data = edx_when_api.get_overrides_for_course(course.id)
extension_data = [normalize_override_tuple("course", row) for row in query_data]
5 participants
Description
Supporting information
Link to other information about the change, such as Jira issues, GitHub issues, or Discourse discussions.
Be sure to check they are publicly readable, or if not, repeat the information here.
Testing instructions
example curl to test, make sure you have the right cookies and update the right IDs and details
a possible valid example of response of the query looks like:
{ "next": null, "previous": null, "count": 2, "num_pages": 1, "current_page": 1, "start": 0, "results": [ { "username": "armando", "full_name": "", "email": "[email protected]", "unit_title": "Subsection", "unit_location": "block-v1:OEXCOM+101+2025_T1+type@sequential+block@c3ccd3d1f91b42dfb2bff8185dba4d3b", "extended_due_date": "2026-12-28T00:00:00Z" }, { "username": "javier", "full_name": "", "email": "[email protected]", "unit_title": "Subsection", "unit_location": "block-v1:OEXCOM+101+2025_T1+type@sequential+block@c3ccd3d1f91b42dfb2bff8185dba4d3b", "extended_due_date": "2025-12-12T12:12:00Z" } ] }Deadline
None
Other information
Include anything else that will help reviewers and consumers understand the change.