feat: add endpoint for course transcript details #454
Conversation
BrandonHBodine
force-pushed
the
edx/bbodine/add-course-transcript-endpoint
branch
5 times, most recently
from
735661c to
61560a8
Compare
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #454 +/- ##
==========================================
+ Coverage 94.40% 94.42% +0.02%
==========================================
Files 28 28
Lines 3001 3051 +50
Branches 168 171 +3
==========================================
+ Hits 2833 2881 +48
- Misses 150 151 +1
- Partials 18 19 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
BrandonHBodine
force-pushed
the
edx/bbodine/add-course-transcript-endpoint
branch
from
45b82f0 to
6d55801
Compare
| def test_successful_response(self): | ||
| """ | ||
| Test succesful response from view | ||
| """ | ||
| with patch( | ||
| 'edxval.views.get_transcript_details_for_course' | ||
| ) as mock_transcript_details: | ||
| # Simulate a return value when the function is called. | ||
| mock_transcript_details.return_value = {} | ||
| course_id = 'course-v1:edx+1+2023_05' | ||
| url = reverse(self.base_url, args=[course_id]) | ||
| response = self.client.get(url) | ||
|
|
||
| # Verify the function was called once with course_id | ||
| mock_transcript_details.assert_called_once_with(course_id) | ||
|
|
||
| self.assertEqual(response.status_code, status.HTTP_200_OK) |
There was a problem hiding this comment.
nit: You should not mock the internal util here. Instead, setup the required data using factories and verify that view is getting the correct data as expected.
There was a problem hiding this comment.
@BrandonHBodine Hi, this was not addressed. I would suggest creating a followup PR to address this.
There was a problem hiding this comment.
@DawoudSheraz is this a requirement to getting this released? You included it as a nit; which typically means optional.
The get_transcript_details_for_course is tested with setup data in edxval/tests/test_api.py. Testing it here seems redundant to me.
Please let me know. I have a deadline to demo our feature that uses this in Staging on Thursday.
There was a problem hiding this comment.
Well, not a requirement for release but important from unit testing point of view.
I agree that get_transcript_details_for_course has been tested in edxval/tests/test_api.py but the view is utilizing that util, so we have to ensure that view is working as expected by adding some test data. It is not redundant. Both are testing different parts of the code.
There was a problem hiding this comment.
Please ensure that the version is bumped in https://github.com/openedx/edx-val/blob/master/edxval/__init__.py#L5. Once merged, the release will be created from https://github.com/openedx/edx-val/releases which pushes the latest version to PyPi. Once the latest version is available on Pypi, it will be updated in edx-platform.
BrandonHBodine
deleted the
edx/bbodine/add-course-transcript-endpoint
branch
@DawoudSheraz Is this a manual process or is it automated somewhere? Do I create a new tag and draft a new release? I walked through that process but noticed that the other releases have assets, zip file and tar, included. |
You would need to draft a new release against master with an appropriate new version. Rest of that is automated. |
Context: I'm working on a project that will give edX an automated path for translating transcripts using AI translations services.
This PR is to add an endpoint; that when passed a course_id will retrieve all it's related videos transcripts so that our service will be able to read the transcript files, translate them, and then update the translated transcripts back to VAL.