Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add endpoint for course transcript details #454

Merged
merged 4 commits into from
Sep 18, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 45 additions & 0 deletions edxval/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -733,6 +733,51 @@ def get_videos_for_course(course_id, sort_field=None, sort_dir=SortDirection.asc
)


def get_transcript_details_for_course(course_id):
"""
Gets all the transcript for a course and bundles up data.

Args:
course_id (String)

Returns:
(dict): Returns all the edx_video_id's and related transcript details for a course
{
'edx_video_id': {
'lang_code': {
'provider': 'What the provider is',
'content': 'Content of the transcript',
'file_format': 'file format',
'url': 'location of the file',
'name': 'name of the file',
'size': size of the file
}
}
"""
course_transcripts_data = {}

course_videos = CourseVideo.objects.filter(course_id=course_id).select_related('video')
for course_video in course_videos:

edx_video_id = course_video.video.edx_video_id
transcript_data = {}

video_transcripts = VideoTranscript.objects.filter(video=course_video.video)
for video_transcript in video_transcripts:
transcript_data[video_transcript.language_code] = {
'provider': video_transcript.provider,
'content': video_transcript.transcript.file.read(),
'file_format': video_transcript.file_format,
'url': video_transcript.transcript.url,
'name': video_transcript.transcript.name,
'size': video_transcript.transcript.size,
}

course_transcripts_data[edx_video_id] = transcript_data

return course_transcripts_data


def remove_video_for_course(course_id, edx_video_id):
"""
Soft deletes video for particular course.
Expand Down
33 changes: 33 additions & 0 deletions edxval/tests/test_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -2684,6 +2684,12 @@ def setUp(self):
self.v2_transcript1 = video_and_transcripts['transcripts']['de']
self.v2_transcript2 = video_and_transcripts['transcripts']['zh']

# Add the videos to courses
self.course_id1 = 'test-course-1'
self.course_id2 = 'test-course-2'
CourseVideo.objects.create(video=self.video1, course_id=self.course_id1)
CourseVideo.objects.create(video=self.video2, course_id=self.course_id2)

self.temp_dir = mkdtemp()
self.addCleanup(shutil.rmtree, self.temp_dir)

Expand Down Expand Up @@ -3133,6 +3139,33 @@ def test_no_create_transcript_file(self, video_id, language_code):
# Verify no file is created.
self.assertEqual(file_system.listdir(constants.EXPORT_IMPORT_STATIC_DIR), [])

def test_get_transcript_details_for_course(self):
"""
Verify that `get_transcript_details_for_course` api function works as expected.
"""

course_transcript = api.get_transcript_details_for_course(self.course_id1)

self.assertEqual(course_transcript['super-soaker']['en']['provider'], TranscriptProviderType.THREE_PLAY_MEDIA)
self.assertIn('content', course_transcript['super-soaker']['en'])
self.assertEqual(course_transcript['super-soaker']['en']['file_format'], utils.TranscriptFormat.SRT)
self.assertIn('url', course_transcript['super-soaker']['en'])
self.assertIn('name', course_transcript['super-soaker']['en'])
self.assertIn('size', course_transcript['super-soaker']['en'])

self.assertEqual(course_transcript['super-soaker']['fr']['provider'], TranscriptProviderType.CIELO24)
self.assertIn('content', course_transcript['super-soaker']['fr'])
self.assertEqual(course_transcript['super-soaker']['en']['file_format'], utils.TranscriptFormat.SRT)
self.assertIn('url', course_transcript['super-soaker']['fr'])
self.assertIn('name', course_transcript['super-soaker']['fr'])
self.assertIn('size', course_transcript['super-soaker']['fr'])

def test_get_transcript_details_for_course_no_course_videos(self):

course_transcript = api.get_transcript_details_for_course('this-is-not-a-course-id')

self.assertEqual(len(course_transcript), 0)


@ddt
class TranscriptPreferencesTest(TestCase):
Expand Down
31 changes: 31 additions & 0 deletions edxval/tests/test_views.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@


import json
from unittest.mock import patch

from ddt import data, ddt, unpack
from django.urls import reverse
Expand Down Expand Up @@ -1101,3 +1102,33 @@ def test_update_hls_encodes_for_video(self):
self.assertEqual(actual_encoded_video.url, expected_data['encode_data']['url'])
self.assertEqual(actual_encoded_video.file_size, expected_data['encode_data']['file_size'])
self.assertEqual(actual_encoded_video.bitrate, expected_data['encode_data']['bitrate'])


class CourseTranscriptsDetailViewTest(APIAuthTestCase):
"""
CourseTranscriptsDetailView Tests.
"""
def setUp(self):
"""
Tests setup.
"""
self.base_url = 'course-transcripts'
BrandonHBodine marked this conversation as resolved.
Show resolved Hide resolved
super().setUp()

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)
Comment on lines +1113 to +1129
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@BrandonHBodine Hi, this was not addressed. I would suggest creating a followup PR to address this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

3 changes: 3 additions & 0 deletions edxval/urls.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@
path('videos/missing-hls/', views.HLSMissingVideoView.as_view(),
name='hls-missing-video'
),
path('videos/course-transcripts/<str:course_id>/', views.CourseTranscriptsDetailView.as_view(),
name='course-transcripts'
),
path('videos/video-transcripts/create/', views.VideoTranscriptView.as_view(),
name='create-video-transcript'
),
Expand Down
19 changes: 18 additions & 1 deletion edxval/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
from rest_framework.response import Response
from rest_framework.views import APIView

from edxval.api import create_or_update_video_transcript
from edxval.api import create_or_update_video_transcript, get_transcript_details_for_course
from edxval.models import (
LIST_MAX_ITEMS,
CourseVideo,
Expand Down Expand Up @@ -175,6 +175,23 @@
return response


class CourseTranscriptsDetailView(APIView):
"""
A view to get the details for all the course transcripts related to a course_id.
"""
authentication_classes = (JwtAuthentication, SessionAuthentication)

def get(self, _request, course_id):
"""
Returns all transcript data for a course when given a course_id.
"""
if not course_id:
return Response(status=status.HTTP_400_BAD_REQUEST, data={'message': 'course_id param required'})

Check warning on line 189 in edxval/views.py

View check run for this annotation

Codecov / codecov/patch

edxval/views.py#L189

Added line #L189 was not covered by tests

course_data = get_transcript_details_for_course(course_id)
return Response(status=status.HTTP_200_OK, data=course_data)


class VideoStatusView(APIView):
"""
A Video View to update the status of a video.
Expand Down
Loading