Skip to content

Conversation

@farhan
Copy link
Contributor

@farhan farhan commented Nov 14, 2025

Move learning core methods directly used in Video XBlock to video config service

Some ignore imports has been added to setup.cfg to fix these

@farhan farhan force-pushed the farhan/add-waffle-flags branch from 7418e07 to 4df35f9 Compare November 17, 2025 09:30
@farhan farhan force-pushed the farhan/add-learning-core-methods branch from db9a5a1 to 3c7e50b Compare November 17, 2025 10:15
@farhan farhan changed the base branch from farhan/add-waffle-flags to master November 17, 2025 10:22
@farhan farhan changed the base branch from master to farhan/add-waffle-flags November 17, 2025 10:23
Base automatically changed from farhan/add-waffle-flags to master November 17, 2025 12:52
@farhan farhan force-pushed the farhan/add-learning-core-methods branch from 3c7e50b to d361718 Compare November 17, 2025 13:21
@farhan farhan marked this pull request as ready for review November 18, 2025 15:42
@farhan farhan changed the title Add learning core methods to video config service Move learning core methods to video config service Nov 19, 2025
@kdmccormick kdmccormick self-requested a review November 19, 2025 14:11
Copy link
Member

@kdmccormick kdmccormick left a comment

Choose a reason for hiding this comment

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

Libraries are particular to edx-platform, so they should not be part of the service interface. Additionally, adding a static asset is something that is common to all blocks, not just video, so if it were to be part of a service, it would not make sense for the video_config service to have such a method.

Here's a proposal, which would introduce upload_transcript and delete_transcript as the video_config methods, hiding the Libraries details within the edx-platform service implementation: #37657 . Could you review?

@farhan
Copy link
Contributor Author

farhan commented Nov 20, 2025

Libraries are particular to edx-platform, so they should not be part of the service interface. Additionally, adding a static asset is something that is common to all blocks, not just video, so if it were to be part of a service, it would not make sense for the video_config service to have such a method.

Here's a proposal, which would introduce upload_transcript and delete_transcript as the video_config methods, hiding the Libraries details within the edx-platform service implementation: #37657 . Could you review?

Yes, I was also concerned related to these methods. Thanks for giving the next directions. Will review.

@farhan farhan marked this pull request as draft November 20, 2025 09:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants