chore: Introduce a new service VideoConfig for the Extracted Video XBlock #37383
Conversation
farhan
force-pushed
the
farhan/extract-video-xblock
branch
from
7858c6f to
55848cb
Compare
farhan
force-pushed
the
farhan/extract-video-xblock
branch
2 times, most recently
from
db7033d to
ea753b3
Compare
farhan
force-pushed
the
farhan/extract-video-xblock
branch
from
726c2ad to
e33585a
Compare
| @@ -0,0 +1,12 @@ | |||
| """ | |||
There was a problem hiding this comment.
What makes this change necessary?
| @@ -0,0 +1,209 @@ | |||
| """ | |||
| Video Configuration Service for XBlock runtime. | |||
There was a problem hiding this comment.
Many of the other XBlock services are defined within the apps that related to them. For example, EnrollmentsService is in the enrollments app, CreditService is defined in the credit app, etc. Could you follow that pattern, and define this service within the video_configuration app? It might allow you to remove some of the method-level imports, which I assume you are using in order to get around importlinter and cyclical imports.
There was a problem hiding this comment.
Yes, video config service suppose to be in this app. Refactored
|
|
||
| organization = get_course_organization(self.course_id) | ||
|
|
||
| from xmodule.video_block.sharing_sites import sharing_sites_info_for_video |
There was a problem hiding this comment.
What do you think about moving sharing_sites into video_configuration app? I imagine the module will need to move at some point because we plan to eventually delete the built-in video_block directory.
There was a problem hiding this comment.
It's a great idea.
Eventually we have to remove all built-in video block code, so moving all video related functionality to video-config app makes sense.
Let's make a list of these steps and start moving the code gradually.
Have created this sub-task story
| extracted to a separate repository. | ||
| """ | ||
|
|
||
| def __init__(self, course_id: Optional[CourseKey] = None): |
There was a problem hiding this comment.
Great to see new type annotations, thank you 👍🏻 Some feedback:
- For
Optional[Foo]types, you can also annotate them usingFoo | None, for exampleCourseKey | None. It's the same effect, but it reads a little nicer and you don't need to importOptional. - Please ensure that every method parameter and return in this module are annotated
- Please add this module to
files =inmypy.iniso that it actually gets checked in CI.
| metadata, status_code = load_metadata_from_youtube(video_id=video_id, request=request) | ||
| return metadata, status_code | ||
|
|
||
| def add_library_static_asset(self, usage_key: UsageKeyV2, filename: str, content: bytes): |
There was a problem hiding this comment.
What's this method here for?
There was a problem hiding this comment.
it's for VideoStudioViewHandlers._studio_transcript_upload method
code lines
| log = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| def get_public_video_url(video_block): |
There was a problem hiding this comment.
Unless I missed something, it seems like all method in this file could simply take the video's usage_key as an argument instead of the block itself. This would make the methods easier to reason about (less input data, less potential for mutation) and easier to test.
There was a problem hiding this comment.
Good point, let me add todo and let's do it while testing the code, it needs to test sharing feature.
| "cache": CacheService(cache), | ||
| 'replace_urls': ReplaceURLService | ||
| 'replace_urls': ReplaceURLService, | ||
| 'video_config': VideoConfigService(course_id=course_id), |
There was a problem hiding this comment.
Could you also refactor the existing video block to use this new service? I think that would make the extraction itself less risky, because the built-in and extracted blocks would share the same service code.
There was a problem hiding this comment.
Yes, will do, once we finalize the service.
Added task here
| for object_tag in object_tags: | ||
| assert object_tag.value in new_upstream_tags | ||
|
|
||
| @skip("Skipping video block test for now, needs to be fixed") |
There was a problem hiding this comment.
I presume you'll un-skip this before merging?
farhan
force-pushed
the
farhan/extract-video-xblock
branch
from
3f395c3 to
fc8ea29
Compare
farhan
force-pushed
the
farhan/extract-video-xblock
branch
from
4ae46e1 to
51d3c39
Compare
farhan
force-pushed
the
farhan/extract-video-xblock
branch
2 times, most recently
from
b2a8394 to
6784151
Compare
farhan
force-pushed
the
farhan/extract-video-xblock
branch
from
015bdef to
881605d
Compare
farhan
force-pushed
the
farhan/extract-video-xblock
branch
from
881605d to
6499a25
Compare
farhan
force-pushed
the
farhan/extract-video-xblock
branch
from
2f0155d to
cba4384
Compare
|
Closing as it has been implemented multiple small PR's |
Ticket: openedx/public-engineering#430
Relevant xblocks-contrib PR