feat: add drf for studio video page - #33528
Conversation
jesperhodge
left a comment
jesperhodge
left a comment
There was a problem hiding this comment.
The code looks great. I mainly left comments discussing the need for tests, as well as one small desired code structure change. However, I have been only able to test this by making requests via the swagger-ui to the new endpoints. Testing with your course-authoring branch I got TypeError undefined is not iterable (cannot read property Symbol(Symbol.iterator)) Call Stack _iterableToArray node_modules/react-dropzone/dist/es/utils/index.js:34:76.
This may very well be due to my local setup, although on master this error did not occur (but the list of videos was also empty on master.) At any rate it's a frontend problem and not a backend problem.
So I think in general this works but I have not tested it successfully with the frontend branch.
I was able to ascertain that nothing broke locally. However, since the video pipeline locally can't be tested, we'll just need to test this once more on stage before we deploy to prod. But I'm not worried about it.
Please push back if you don't share my thought that we need tests here.
jesperhodge
left a comment
jesperhodge
left a comment
There was a problem hiding this comment.
Great work!! Everything code-wise looks really really good. I just left the one remark that I think would be a good improvement, otherwise it's all ready.
| if not has_studio_read_access(request.user, course_key): | ||
| self.permission_denied(request) | ||
|
|
||
| with modulestore().bulk_operations(course_key): |
There was a problem hiding this comment.
I believe an improvement here that would make the api view very flexible is to get rid of any calls to modulestore, so that the api view here is very easy to change later, since it then doesn't care how the data is provided. I would recommend extracting line 151 to 156 to a function and put that function into video_storage_handlers.
jesperhodge
left a comment
jesperhodge
left a comment
There was a problem hiding this comment.
I'll add an approve here so you don't need a re-review from me when the requested change is done.
|
2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production. |
|
2U Release Notice: This PR has been deployed to the edX production environment. |
1 similar comment
|
2U Release Notice: This PR has been deployed to the edX production environment. |
3 participants
Description
This PR adds a couple DRFs for the Course Authoring video page. No functionality to the Studio video page has been changed. This change impacts "Developers".
Testing instructions
Deadline
None
Other information
course-authoringPR #640 is dependent on this PR.course-authoringPR #640 should be used used for testing step 3 and 4.