refactor: Introduce VideoConfig service, move video sharing methods in it

[farhan]

Ticket: openedx/public-engineering#426

[ttqureshi]

I have tested the video sharing functionality. And this refactoring doesn't have any impact on how the video sharing works.
I enabled the Social Sharing checkbox in the video block configuration settings:

This setting rendered the template context for video sharing buttong as shown below:

I am now looking into the test case failures in this PR.

[kdmccormick]

Functionality and testing both look great 👍🏻

Just some more code nits to work through and then we can get this merged.

Could you re-label the commit as a refactor:? The chore: label is supposed to be for really trivial things, like a patch version bump.

[kdmccormick]

This pattern (empty class with static methods) is a common way to organize functions in java, but it is not idiomatic in python. The module already serves the purpose of organizing the functions. So, please remove the VideoSharingUtils class and declare these as regular functions. If you'd like, you could rename the module to sharing.py -- this makes it clear, even without the class, that all the functions within are related to sharing.

[farhan]

Thanks for pointing out, following pythonic way of doing things make sense, going to implement.

I gave it a try to this pattern as it could bring following benefits:

1. utils.py sounds extensible to me to accommodate other util methods in future.
2. Defined empty class VideoSharingUtils to group the related util methods
3. VideoSharingUtils.get_public_video_url() felt more explicit and self-documenting when importing, making it immediately clear what functionality you're using

[kdmccormick]

The only use of a module named utils.py, in my opinion, is for helper functions which cannot be grouped together in any better way. But, you have a good way to group these together (they are sharing-related), so you may as well make that the name of the module.

VideoSharingUtils.get_public_video_url() felt more explicit and self-documenting when importing

This is fair, and you can still achieve it without the class:

from openedx.core.djangoapps.video_config import sharing as video_sharing

video_sharing.get_public_video_url(...)

[kdmccormick]

by taking an instance of VideoBlock as a parameter, this function is making assumptions about the fields of VideoBlock, which it should not be once the VideoBlock is defined outside of the platform.

could this function take just (usage_key: UsageKey, public_access: bool) instead?

please annotate the return type as well.

[kdmccormick]

let's not special-case the video-config like this. The field-data service is weird, and isn't the best pattern to follow. Check out the user or teams_configuration xblock services to see a more standard way of injecting the service into the runtime.

[kdmccormick]

If this addition is necessary in order to get tests to pass, then this is fine.

But, if tests pass without the service, then please remove it. It's best to keep the test runtime as simple as possible. For example, there's not even a user service in this runtime.

[farhan]

Yes, without this injection tests are failing.

FAILED lms/djangoapps/courseware/tests/test_video_mongo.py::VideoBlockTest::test_get_context - xblock.exceptions.NoSuchServiceError: Service 'video_config' is not available.

Detailed logs

_________________________________________________________________________________ VideoBlockTest.test_get_context _________________________________________________________________________________

self = <lms.djangoapps.courseware.tests.test_video_mongo.VideoBlockTest testMethod=test_get_context>

    def test_get_context(self):
        """"
        Test get_context.
    
        This test is located here and not in xmodule.tests because get_context calls editable_metadata_fields.
        Which, in turn, uses settings.LANGUAGES from django setttings.
        """
        correct_tabs = [
            {
                'name': "Basic",
                'template': "video/transcripts.html",
                'current': True
            },
            {
                'name': 'Advanced',
                'template': 'tabs/metadata-edit-tab.html'
            }
        ]
>       rendered_context = self.block.get_context()

lms/djangoapps/courseware/tests/test_video_mongo.py:1829: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
xmodule/video_block/video_block.py:849: in get_context
    _context = MakoTemplateBlockBase.get_context(self)
xmodule/mako_block.py:56: in get_context
    'editable_metadata_fields': self.editable_metadata_fields
xmodule/video_block/video_block.py:647: in editable_metadata_fields
    editable_fields['public_access']['url'] = self.get_public_video_url()
xmodule/video_block/video_block.py:523: in get_public_video_url
    video_config_service = self.runtime.service(self, 'video_config')
xmodule/x_module.py:1505: in service
    service = super().service(block=block, service_name=service_name)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = <xmodule.tests.TestDescriptorSystem object at 0xffff67fea350>
block = <VideoBlockWithMixins @E3CB license=None, name=None, parent=None, tags=[], display_name='Video', course_edit_method='S...{}, youtube_id_0_75='', youtube_id_1_0='3_yD_cEKoCk', youtube_id_1_25='', youtube_id_1_5='', youtube_is_available=True>
service_name = 'video_config'

    def service(self, block, service_name):
        """Return a service, or None.
    
        Services are objects implementing arbitrary other interfaces.  They are
        requested by agreed-upon names, see [XXX TODO] for a list of possible
        services.  The object returned depends on the service requested.
    
        XBlocks must announce their intention to request services with the
        `XBlock.needs` or `XBlock.wants` decorators.  Use `needs` if you assume
        that the service is available, or `wants` if your code is flexible and
        can accept a None from this method.
    
        Runtimes can override this method if they have different techniques for
        finding and delivering services.
    
        Arguments:
            block (XBlock): this block's class will be examined for service
                decorators.
            service_name (str): the name of the service requested.
    
        Returns:
            An object implementing the requested service, or None.
    
        """
        declaration = block.service_declaration(service_name)
        if declaration is None:
            raise NoSuchServiceError(f"Service {service_name!r} was not requested.")
        service = self._services.get(service_name)
        if service is None and declaration == "need":
>           raise NoSuchServiceError(f"Service {service_name!r} is not available.")
E           xblock.exceptions.NoSuchServiceError: Service 'video_config' is not available.

/mnt/xblock/xblock/runtime.py:1101: NoSuchServiceError

[kdmccormick]

this information is redundant. you can omit it and add a type annotation instead.

[open-craft-grove]

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Configuration information unavailable

[open-craft-grove]

Sandbox deployment failed 💥
Please check the settings and requirements.
Retry deployment by pushing a new commit or updating the requirements/settings in the pull request's description.
📜 Failure Logs
ℹ️ Configuration information unavailable

[open-craft-grove]

Sandbox deployment failed 💥
Please check the settings and requirements.
Retry deployment by pushing a new commit or updating the requirements/settings in the pull request's description.
📜 Failure Logs
ℹ️ Configuration information unavailable

[open-craft-grove]

Sandbox deployment failed 💥
Please check the settings and requirements.
Retry deployment by pushing a new commit or updating the requirements/settings in the pull request's description.
📜 Failure Logs
ℹ️ Configuration information unavailable

[open-craft-grove]

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Configuration information unavailable

[open-craft-grove]

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Configuration information unavailable

[open-craft-grove]

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Configuration information unavailable

[open-craft-grove]

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Configuration information unavailable

[open-craft-grove]

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Configuration information unavailable

[open-craft-grove]

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Configuration information unavailable

[farhan]

@kdmccormick The PR is ready for another review

I’ve aligned the service injection with the standard patterns, but please feel free to share any further guidance if needed.
I’ve also tested the sharing feature on the sandbox with all the recent changes.

[kdmccormick]

LGTM

[open-craft-grove]

Sandbox deployment failed 💥
Please check the settings and requirements.
Retry deployment by pushing a new commit or updating the requirements/settings in the pull request's description.
📜 Failure Logs
ℹ️ Grove Config, Tutor Config, Tutor Requirements

[open-craft-grove]

Sandbox deployment failed 💥
Please check the settings and requirements.
Retry deployment by pushing a new commit or updating the requirements/settings in the pull request's description.
📜 Failure Logs
ℹ️ Grove Config, Tutor Config, Tutor Requirements

[open-craft-grove]

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements