-
Notifications
You must be signed in to change notification settings - Fork 4.2k
remove video transcript enabled flag #17923
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,6 +5,7 @@ | |
| from mock import Mock, patch, ANY | ||
|
|
||
| from django.test.testcases import TestCase | ||
| from django.core.urlresolvers import reverse | ||
| from edxval import api | ||
|
|
||
| from contentstore.tests.utils import CourseTestCase | ||
|
|
@@ -177,53 +178,39 @@ def test_invalid_credentials(self, provider, credentials, expected_error_message | |
|
|
||
|
|
||
| @ddt.ddt | ||
| @patch( | ||
| 'openedx.core.djangoapps.video_config.models.VideoTranscriptEnabledFlag.feature_enabled', | ||
| Mock(return_value=True) | ||
| ) | ||
| class TranscriptDownloadTest(CourseTestCase): | ||
| """ | ||
| Tests for transcript download handler. | ||
| """ | ||
| VIEW_NAME = 'transcript_download_handler' | ||
|
|
||
| def get_url_for_course_key(self, course_id): | ||
| return reverse_course_url(self.VIEW_NAME, course_id) | ||
| @property | ||
| def view_url(self): | ||
| """ | ||
| Returns url for this view | ||
| """ | ||
| return reverse('transcript_download_handler') | ||
|
|
||
| def test_302_with_anonymous_user(self): | ||
| """ | ||
| Verify that redirection happens in case of unauthorized request. | ||
| """ | ||
| self.client.logout() | ||
| transcript_download_url = self.get_url_for_course_key(self.course.id) | ||
| response = self.client.get(transcript_download_url, content_type='application/json') | ||
| response = self.client.get(self.view_url, content_type='application/json') | ||
| self.assertEqual(response.status_code, 302) | ||
|
|
||
| def test_405_with_not_allowed_request_method(self): | ||
| """ | ||
| Verify that 405 is returned in case of not-allowed request methods. | ||
| Allowed request methods include GET. | ||
| """ | ||
| transcript_download_url = self.get_url_for_course_key(self.course.id) | ||
| response = self.client.post(transcript_download_url, content_type='application/json') | ||
| response = self.client.post(self.view_url, content_type='application/json') | ||
| self.assertEqual(response.status_code, 405) | ||
|
|
||
| def test_404_with_feature_disabled(self): | ||
| """ | ||
| Verify that 404 is returned if the corresponding feature is disabled. | ||
| """ | ||
| transcript_download_url = self.get_url_for_course_key(self.course.id) | ||
| with patch('openedx.core.djangoapps.video_config.models.VideoTranscriptEnabledFlag.feature_enabled') as feature: | ||
| feature.return_value = False | ||
| response = self.client.get(transcript_download_url, content_type='application/json') | ||
| self.assertEqual(response.status_code, 404) | ||
|
|
||
| @patch('contentstore.views.transcript_settings.get_video_transcript_data') | ||
| def test_transcript_download_handler(self, mock_get_video_transcript_data): | ||
| """ | ||
| Tests that transcript download handler works as expected. | ||
| """ | ||
| transcript_download_url = self.get_url_for_course_key(self.course.id) | ||
| mock_get_video_transcript_data.return_value = { | ||
| 'content': json.dumps({ | ||
| "start": [10], | ||
|
|
@@ -235,7 +222,7 @@ def test_transcript_download_handler(self, mock_get_video_transcript_data): | |
|
|
||
| # Make request to transcript download handler | ||
| response = self.client.get( | ||
| transcript_download_url, | ||
| self.view_url, | ||
| data={ | ||
| 'edx_video_id': '123', | ||
| 'language_code': 'en' | ||
|
|
@@ -277,66 +264,50 @@ def test_transcript_download_handler_missing_attrs(self, request_payload, expect | |
| Tests that transcript download handler with missing attributes. | ||
| """ | ||
| # Make request to transcript download handler | ||
| transcript_download_url = self.get_url_for_course_key(self.course.id) | ||
| response = self.client.get(transcript_download_url, data=request_payload) | ||
| response = self.client.get(self.view_url, data=request_payload) | ||
| # Assert the response | ||
| self.assertEqual(response.status_code, 400) | ||
| self.assertEqual(json.loads(response.content)['error'], expected_error_message) | ||
|
|
||
|
|
||
| @ddt.ddt | ||
| @patch( | ||
| 'openedx.core.djangoapps.video_config.models.VideoTranscriptEnabledFlag.feature_enabled', | ||
| Mock(return_value=True) | ||
| ) | ||
| class TranscriptUploadTest(CourseTestCase): | ||
| """ | ||
| Tests for transcript upload handler. | ||
| """ | ||
| VIEW_NAME = 'transcript_upload_handler' | ||
|
|
||
| def get_url_for_course_key(self, course_id): | ||
| return reverse_course_url(self.VIEW_NAME, course_id) | ||
| @property | ||
| def view_url(self): | ||
| """ | ||
| Returns url for this view | ||
| """ | ||
| return reverse('transcript_upload_handler') | ||
|
|
||
| def test_302_with_anonymous_user(self): | ||
| """ | ||
| Verify that redirection happens in case of unauthorized request. | ||
| """ | ||
| self.client.logout() | ||
| transcript_upload_url = self.get_url_for_course_key(self.course.id) | ||
| response = self.client.post(transcript_upload_url, content_type='application/json') | ||
| response = self.client.post(self.view_url, content_type='application/json') | ||
| self.assertEqual(response.status_code, 302) | ||
|
|
||
| def test_405_with_not_allowed_request_method(self): | ||
| """ | ||
| Verify that 405 is returned in case of not-allowed request methods. | ||
| Allowed request methods include POST. | ||
| """ | ||
| transcript_upload_url = self.get_url_for_course_key(self.course.id) | ||
| response = self.client.get(transcript_upload_url, content_type='application/json') | ||
| response = self.client.get(self.view_url, content_type='application/json') | ||
| self.assertEqual(response.status_code, 405) | ||
|
|
||
| def test_404_with_feature_disabled(self): | ||
| """ | ||
| Verify that 404 is returned if the corresponding feature is disabled. | ||
| """ | ||
| transcript_upload_url = self.get_url_for_course_key(self.course.id) | ||
| with patch('openedx.core.djangoapps.video_config.models.VideoTranscriptEnabledFlag.feature_enabled') as feature: | ||
| feature.return_value = False | ||
| response = self.client.post(transcript_upload_url, content_type='application/json') | ||
| self.assertEqual(response.status_code, 404) | ||
|
|
||
| @patch('contentstore.views.transcript_settings.create_or_update_video_transcript') | ||
| @patch('contentstore.views.transcript_settings.get_available_transcript_languages', Mock(return_value=['en'])) | ||
| def test_transcript_upload_handler(self, mock_create_or_update_video_transcript): | ||
| """ | ||
| Tests that transcript upload handler works as expected. | ||
| """ | ||
| transcript_upload_url = self.get_url_for_course_key(self.course.id) | ||
| transcript_file_stream = BytesIO('0\n00:00:00,010 --> 00:00:00,100\nПривіт, edX вітає вас.\n\n') | ||
| # Make request to transcript upload handler | ||
| response = self.client.post( | ||
| transcript_upload_url, | ||
| self.view_url, | ||
| { | ||
| 'edx_video_id': '123', | ||
| 'language_code': 'en', | ||
|
|
@@ -395,9 +366,8 @@ def test_transcript_upload_handler_missing_attrs(self, request_payload, expected | |
| """ | ||
| Tests the transcript upload handler when the required attributes are missing. | ||
| """ | ||
| transcript_upload_url = self.get_url_for_course_key(self.course.id) | ||
| # Make request to transcript upload handler | ||
| response = self.client.post(transcript_upload_url, request_payload, format='multipart') | ||
| response = self.client.post(self.view_url, request_payload, format='multipart') | ||
| self.assertEqual(response.status_code, 400) | ||
| self.assertEqual(json.loads(response.content)['error'], expected_error_message) | ||
|
|
||
|
|
@@ -407,14 +377,13 @@ def test_transcript_upload_handler_existing_transcript(self): | |
| Tests that upload handler do not update transcript's language if a transcript | ||
| with the same language already present for an edx_video_id. | ||
| """ | ||
| transcript_upload_url = self.get_url_for_course_key(self.course.id) | ||
| # Make request to transcript upload handler | ||
| request_payload = { | ||
| 'edx_video_id': '1234', | ||
| 'language_code': 'en', | ||
| 'new_language_code': 'es' | ||
| } | ||
| response = self.client.post(transcript_upload_url, request_payload, format='multipart') | ||
| response = self.client.post(self.view_url, request_payload, format='multipart') | ||
| self.assertEqual(response.status_code, 400) | ||
| self.assertEqual( | ||
| json.loads(response.content)['error'], | ||
|
|
@@ -427,10 +396,9 @@ def test_transcript_upload_handler_with_image(self): | |
| Tests the transcript upload handler with an image file. | ||
| """ | ||
| with make_image_file() as image_file: | ||
| transcript_upload_url = self.get_url_for_course_key(self.course.id) | ||
| # Make request to transcript upload handler | ||
| response = self.client.post( | ||
| transcript_upload_url, | ||
| self.view_url, | ||
| { | ||
| 'edx_video_id': '123', | ||
| 'language_code': 'en', | ||
|
|
@@ -451,11 +419,10 @@ def test_transcript_upload_handler_with_invalid_transcript(self): | |
| """ | ||
| Tests the transcript upload handler with an invalid transcript file. | ||
| """ | ||
| transcript_upload_url = self.get_url_for_course_key(self.course.id) | ||
| transcript_file_stream = BytesIO('An invalid transcript SubRip file content') | ||
| # Make request to transcript upload handler | ||
| response = self.client.post( | ||
| transcript_upload_url, | ||
| self.view_url, | ||
| { | ||
| 'edx_video_id': '123', | ||
| 'language_code': 'en', | ||
|
|
@@ -473,11 +440,7 @@ def test_transcript_upload_handler_with_invalid_transcript(self): | |
|
|
||
|
|
||
| @ddt.ddt | ||
| @patch( | ||
| 'openedx.core.djangoapps.video_config.models.VideoTranscriptEnabledFlag.feature_enabled', | ||
| Mock(return_value=True) | ||
| ) | ||
| class TranscriptUploadTest(CourseTestCase): | ||
| class TranscriptDeleteTest(CourseTestCase): | ||
| """ | ||
| Tests for transcript deletion handler. | ||
| """ | ||
|
|
@@ -504,16 +467,6 @@ def test_405_with_not_allowed_request_method(self): | |
| response = self.client.post(transcript_delete_url) | ||
| self.assertEqual(response.status_code, 405) | ||
|
|
||
| def test_404_with_feature_disabled(self): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. name of the class should be renamed from should be removed from the class as well. |
||
| """ | ||
| Verify that 404 is returned if the corresponding feature is disabled. | ||
| """ | ||
| transcript_delete_url = self.get_url_for_course_key(self.course.id, edx_video_id='test_id', language_code='en') | ||
| with patch('openedx.core.djangoapps.video_config.models.VideoTranscriptEnabledFlag.feature_enabled') as feature: | ||
| feature.return_value = False | ||
| response = self.client.delete(transcript_delete_url) | ||
| self.assertEqual(response.status_code, 404) | ||
|
|
||
| def test_404_with_non_staff_user(self): | ||
| """ | ||
| Verify that 404 is returned if the user doesn't have studio write access. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -130,23 +130,18 @@ def transcript_credentials_handler(request, course_key_string): | |
|
|
||
| @login_required | ||
| @require_GET | ||
| def transcript_download_handler(request, course_key_string): | ||
| def transcript_download_handler(request): | ||
| """ | ||
| JSON view handler to download a transcript. | ||
|
|
||
| Arguments: | ||
| request: WSGI request object | ||
| course_key_string: course key | ||
|
|
||
| Returns: | ||
| - A 200 response with SRT transcript file attached. | ||
| - A 400 if there is a validation error. | ||
| - A 404 if there is no such transcript or feature flag is disabled. | ||
| - A 404 if there is no such transcript. | ||
| """ | ||
| course_key = CourseKey.from_string(course_key_string) | ||
| if not VideoTranscriptEnabledFlag.feature_enabled(course_key): | ||
| return HttpResponseNotFound() | ||
|
|
||
| missing = [attr for attr in ['edx_video_id', 'language_code'] if attr not in request.GET] | ||
| if missing: | ||
| return JsonResponse( | ||
|
|
@@ -206,27 +201,20 @@ def validate_transcript_upload_data(data, files): | |
|
|
||
| @login_required | ||
| @require_POST | ||
| def transcript_upload_handler(request, course_key_string): | ||
| def transcript_upload_handler(request): | ||
| """ | ||
| View to upload a transcript file. | ||
|
|
||
| Arguments: | ||
| request: A WSGI request object | ||
| course_key_string: Course key identifying a course | ||
|
|
||
| Transcript file, edx video id and transcript language are required. | ||
| Transcript file should be in SRT(SubRip) format. | ||
|
|
||
| Returns | ||
| - A 400 if any of the validation fails | ||
| - A 404 if the corresponding feature flag is disabled | ||
| - A 200 if transcript has been uploaded successfully | ||
| """ | ||
| # Check whether the feature is available for this course. | ||
| course_key = CourseKey.from_string(course_key_string) | ||
| if not VideoTranscriptEnabledFlag.feature_enabled(course_key): | ||
| return HttpResponseNotFound() | ||
|
|
||
| error = validate_transcript_upload_data(data=request.POST, files=request.FILES) | ||
| if error: | ||
| response = JsonResponse({'error': error}, status=400) | ||
|
|
@@ -276,14 +264,13 @@ def transcript_delete_handler(request, course_key_string, edx_video_id, language | |
| language_code: transcript's language code. | ||
|
|
||
| Returns | ||
| - A 404 if the corresponding feature flag is disabled or user does not have required permisions | ||
| - A 404 if the user does not have required permisions | ||
| - A 200 if transcript is deleted without any error(s) | ||
| """ | ||
| # Check whether the feature is available for this course. | ||
| course_key = CourseKey.from_string(course_key_string) | ||
| video_transcripts_enabled = VideoTranscriptEnabledFlag.feature_enabled(course_key) | ||
| # User needs to have studio write access for this course. | ||
| if not video_transcripts_enabled or not has_studio_write_access(request.user, course_key): | ||
| if not has_studio_write_access(request.user, course_key): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why do we need to check write access here ?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. so that not everyone can delete the transcript |
||
| return HttpResponseNotFound() | ||
|
|
||
| delete_video_transcript(video_id=edx_video_id, language_code=language_code) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also let's make this consistent with other tests (
self.view_url) as we have done for other viewsThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is out of scope of this PR :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NVM, I found that this is required for
has_studio_write_accesspermission used in delete handler.