-
Notifications
You must be signed in to change notification settings - Fork 4.2k
basic tab attach uploads with edx_video_id #18032
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
basic tab attach uploads with edx_video_id #18032
Conversation
|
Your PR has finished running tests. |
a051525 to
aa4face
Compare
|
Your PR has finished running tests. |
aa4face to
eef8f27
Compare
|
Your PR has finished running tests. |
|
jenkins run bokchoy |
|
jenkins run a11y |
|
Your PR has finished running tests. |
mushtaqak
left a comment
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.
Looks good to me 👍
| edx_video_id=non_existant_edx_video_id | ||
| ) | ||
| # Verify the response | ||
| self.assert_response(response, expected_status_code=400, expected_message='Invalid Video ID') |
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.
Nice 👍
| validated_data.update({ | ||
| 'video': video, | ||
| 'edx_video_id': clean_video_id(video.edx_video_id), | ||
| 'edx_video_id': clean_video_id(edx_video_id) or clean_video_id(video.edx_video_id), |
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.
I think we have success scenario test written for edx_video_id coming from video.edx_video_id but not from the request data.
| 'js/views/video/transcripts/utils' | ||
| ], | ||
| function($, Backbone, _, Utils) { | ||
| function($, Backbone, _, TranscriptUtils) { |
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.
This wasn't previously being used ?
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.
yes
eef8f27 to
b2f40cd
Compare
|
Your PR has finished running tests. |
d7b5b6b to
88c2a08
Compare
b2f40cd to
df8cfed
Compare
|
Your PR has finished running tests. |
df8cfed to
629efd8
Compare
|
Your PR has finished running tests. |
|
@mushtaqak feedback addressed plus added jasmine tests |
88c2a08 to
388649b
Compare
|
Your PR has finished running tests. |
388649b to
35611e7
Compare
9556ce2 to
5d218e7
Compare
|
Your PR has finished running tests. |
4ce3382 to
c02955d
Compare
EDUCATOR-2761
EDUCATOR-2752
5d218e7 to
84ab9ca
Compare
|
Your PR has finished running tests. |
EDUCATOR-2761