[BD-14] [WIP] [SE-2936] Modernize Video Block editor for Blockstore runtime.

[Kelketek]

This PR creates a modernized video block editor for use with the new library authoring MFE. It is currently incomplete, and is here to serve the purpose of getting arch review and feedback before continuing. It contains several comments in places where it is unclear what the best path forward is.

Screenshots:

As mentioned, CSS is not working, so it looks wonky:

Merge deadline: None, yet.

Testing instructions:

1. Check this branch out, and use this frontend branch: [WIP] Enable video block in editor. openedx-unsupported/frontend-app-library-authoring#13
2. Add a video block to a library.
3. Edit the video block.
4. Play around with the settings and save them.

Author notes and concerns:

1. The 'Transitional Service' will be removed, as it has been determined that although preserving studio_view as the editing view would be a laudable achievement and would avoid changing the API, the amount of effort involved and the number of considerations required for backwards compatibility are just too great for the case of XModules and a great deal of shimming work which need not be done with properly isolated XBlocks is being done to allow compatibility with old blocks. The new view will be edit_view.
2. It's not clear what the gettext hack included would do in a real environment or if it's even helpful.
3. edX's version of React had to be upgraded to support the features used in these new components. I've tried to avoid upgrading much else.
4. It may be possible to include the MFE libraries but I have not yet attempted it and think it's unlikely to work.
5. The version of Paragon in the platform probably can't be upgraded without changing code in several other places, though I haven't tried.
6. It's unclear how to best handle CSS for this task. Following the instructions in the README of the installed version of Paragon does not work.
7. File uploads-- transcripts and handouts-- are not yet supported. Adding them in seemed a bit premature with the number of open questions so far, since they seem to be a project in their own right.

Reviewers

• @arbrandes
• @bradenmacdonald
• @kdmccormick

[openedx-webhooks]

Thanks for the pull request, @Kelketek! I've created BLENDED-608 to keep track of it in Jira. More details are on the BD-14 project page.

This is currently a draft pull request. When it is ready for our review and all tests are green, click "Ready for Review", or remove "WIP" from the title, as appropriate.

[kdmccormick]

Linking this back to TNL-7592

[bradenmacdonald]

edX's version of React had to be upgraded to support the features used in these new components. I've tried to avoid upgrading much else. ...
The version of Paragon in the platform probably can't be upgraded without changing code in several other places, though I haven't tried.

I think a long-term goal is to make XBlocks more independent from the platform. So: can we just specify a version of React and Paragon to use just for this XBlock (it's in an iframe after all, no chance of conflicts), and either load it from a CDN or use webpack with tree shaking, terser etc. to produce a tiny little video-edit.js runtime file that includes all its own dependencies? Even without tree shaking, react is 4.5kB and react-dom is 33.2kB. The code benefits of isolation will exceed the (one time, cached) extra download of a bit more JS IMHO.

Another way to say this: think of the video block UI as a tiny microfrontend (a nanofrontend?), so it should have its own package.json and choose its own dependencies (which should be very few, probably just Paragon which I assume will in turn install React etc.)

[bradenmacdonald]

File uploads-- transcripts and handouts-- are not yet supported. Adding them in seemed a bit premature with the number of open questions so far, since they seem to be a project in their own right.

We haven't yet built out an API for an XBlock['s editing UI] to list or upload its own files, but there is a solid set of python and REST APIs for dealing with file uploads for an XBlock. This is what we use in the LabXchange video editor to allow uploading transcript files. It's also working in Ramshackle, so in Ramshackle you can try uploading a transcript file to a video XBlock's file list, then edit the OLX to set the transcript to /static/the-filename.srt and you'll see the transcript file works. Maybe it's working in the new libraries frontend too - I haven't tried it, but I see the "Assets" tab is there :)

Conceptually, I was thinking of adding a "static asset service" to the XBlock runtime which exposes these same APIs, so an XBlock can use APIs like self.runtime.service(self, "static-assets").list_files() to list all the files associated with that XBlock. You don't necessarily need an "upload file" API exposed, as transcript (and other) files can be uploaded via the "Assets" tab shown in your screenshot, and then the XBlock's editor can just show a "Transcript: (select...)" widget which lists any uploaded .srt files and lets the user pick one.

[bradenmacdonald]

edit_view is a better name than studio_view anyways, and reflects that this editing UI is no longer tied to Studio and can now run independently of Studio's UI.

[bradenmacdonald]

This would be good to put in an ADR, or inline in the code but the new runtime avoids using VAL and explicitly won't use this edx_video_id field: https://github.com/edx/edx-platform/blob/657da20c6e75f2c2e6a8c6c6d3833f5aa45deb0e/common/lib/xmodule/xmodule/video_module/video_module.py#L625-L626

See openedx-unsupported/blockstore#62 for context.

Basically: in the new runtime, videos and transcripts should be stored in blockstore as static asset files linked to the video block, rather than in VAL. To simplify transcript handling, .srt is the only supported format.

---

However, we may eventually need to add some backwards compatible support for this field, so perhaps just comment it out with this explanation or make it read-only rather than removing it completely.

[bradenmacdonald]

XBlock handlers are exempt from CSRF and exempt from CORS ; they use another method for security. As a result, you shouldn't need the Cookies dependency nor to worry about CORS, CSRF, or Cookies.

Note that if the handler throws an exception, it may return a response that doesn't include the CORS exempt header, so you can sometimes see CORS errors which are really just masking some other error - check the LMS logs to debug.

[bradenmacdonald]

The gettext global in Studio only loads translations defined in Studio, so it won't work for (third party) XBlocks, which have their own strings. We would need something like the existing ModuleI18nService which loads translation files from the XBlock package, not the platform. A long-term option could be to expose similar (or the same) functionality through the runtime frontend. For now, a workaround can be to use `self.runtime.service('i18n') to translate all the strings on the backend and then push them to the frontend. But I'm not sure how performant/cacheable that is.

[bradenmacdonald]

I know you're not using this anymore, but for the record, yes:

https://github.com/edx/edx-platform/blob/657da20c6e75f2c2e6a8c6c6d3833f5aa45deb0e/openedx/core/djangoapps/xblock/runtime/runtime.py#L80-L82

or

https://github.com/edx/edx-platform/blob/657da20c6e75f2c2e6a8c6c6d3833f5aa45deb0e/openedx/core/djangoapps/xblock/runtime/runtime.py#L218-L236

or

https://github.com/edx/edx-platform/blob/657da20c6e75f2c2e6a8c6c6d3833f5aa45deb0e/openedx/core/djangoapps/xblock/runtime/runtime.py#L411-L420

depending on how costly the service is to initialize and whether it's scoped to one XBlock or not.

[arbrandes]

@Kelketek, I don't have a lot to add to @bradenmacdonald's arch review. Just a couple of comments:

• Big +1 to having this block's edit_view (which @kdmccormick suggests be renamed to author_view_modernized, and I counter-suggested author_view_generic - see TNL-7592), and later the problem block's, be as isolated as possible from Studio's own JS dependencies.

• It is indeed possible to use the Assets tab in the MFE.

I haven't gotten around to testing the PRs, yet, though. Will try to do so tomorrow.

[kdmccormick]

Thanks for the PR @Kelketek. I have three immediate concerns with this:

(1) Dependency isolation

As others have already said, we need to isolate the JS dependencies. We can't absorb this risk of upgrading React for all of edx-platform into this project. It seems like @bradenmacdonald already has some ideas to address this.

Alternatively, would putting this in a new repo (think frontend-component-video-block-editor, or even a more general frontend-xblocks) be a reasonable solution, or would that introduce more complexity than it's worth? I honestly do not know what the mechanics of trying to serve the build of an NPM package as an XBlock view would be like.

(2) frontend-platform

Because we're coding this into edx-platform and sharing dependencies, we can't use the standard auth/i18n/logging/etc. functionality that frontend-platform provides. This isn't a blocker, as we do have other React code in edx-platform that doesn't use frontend-platform, but if solving for (1) would allow us to install and use frontend-platform without too much pain, then let's do that.

(3) Naming of the old and new views

The only reason I'm balking at edit_view (which is otherwise a good name) is that, to me, it implies that this is a new view we'll want on all blocks, with the end-goal being the deprecation of studio_view as an XBlock view. Is that your intent @bradenmacdonald ? I'm not necessarily against it, but you are essentially proposing a change to the XBlock API, which we'd want to communicate more broadly.

My understanding has been that this is a temporary workaround to the fact that studio_view for these blocks is broken outside of legacy Studio, so we'd have this alternative view for as long as the old studio_view needs to be around (but no longer than that). In this case, I imagine we'd want a more transient-sounding name like studio_view_new (or _modernized, the specific suffix isn't that important to me).

Or, to flip it around, what about calling the old view studio_view_legacy, and calling this new view studio_view? If legacy Studio is truly the only client of the old view, then wouldn't preserving backwards compatibility be as simple as making legacy Studio request studio_view_legacy for these two blocks? Or am I missing something here?

[kdmccormick]

FYing @davidjoy for the comment above this one, as a lot of that came from a conversation with him :)

[Kelketek]

@kdmccormick I can't speak for @bradenmacdonald , but I'm getting the impression we'd want to move everyone over to the edit_view and consider it the preferred standard long-term, while providing better tools to the community for them to do so (Maybe including some react components in the SDK and creating some NPM library that has them?)

I do think having a separate repo is best-- it orients the code around better isolation, and provides a potential jumping point for making a library from it later. The trouble is on the deployment side. A hacky, but simple way to do it would be to have a Python package that runs npm build upon install and then exposes an interface for the block to pull assets for the fragment. An alternative might be doing something with git submodules, or else modifying config to separately clone and build the project as needed.

I think the Python package method may be best for this problem. I don't expect that this repo would be a permanent solution-- it'd be a transitionary measure until we figure out our general pattern for these 'nano-frontends'. And if it isn't, we can then go about changing the install method to something more standard.

As a bonus, we could then have the package be dependently built. For environments not using the new Video block frontend, they won't have to spend deployment time building a project they won't use, or risking a communication issue with NPM that might cause a deployment to fail. The XBlock would need to gracefully handle not having the package installed.

After talking with Adolfo and reviewing the comments here, I'm planning to create a few new tickets on our side to track the different subtasks this feedback has created. I'm going to make this branch a feature branch and will make PRs against it as we progress.

[Kelketek]

One more question, @kdmccormick @bradenmacdonald -- The block has several Youtube ID fields that are used for providing different playback speeds. Do we need to support these in the new frontend? I imagine that there aren't too many browsers out there we'll be supporting that won't be able to play HTML5 video properly.

[bradenmacdonald]

@Kelketek @kdmccormick

Dependency isolation

I believe there has long been interest in moving the XBlocks like video out of edx-platform but that's probably more work than we want to take on for this particular project just now.

One thing to note it that the Video XBlock JS code doesn't change very often, and I see no reason to build it at the same time as the rest of the platform JS; perhaps we can just have a Makefile (or use an npm script) that will locally cache the XBlock's dependencies as needed and build a minified .js file. So, if someone wants to edit the JS associated with this XBlock, they can just run npm watch in the video_block directory, make edits as needed, and commit in the end the updated .js source files and the video_edit.min.js output file (obviously not committing any intermediate files like node_modules). In other words, if you think of this as a project that can/should be moved to an external repo, why not treat it like an npm package and include the built version in the repo, ready to use?

The only reason I'm balking at edit_view (which is otherwise a good name) is that, to me, it implies that this is a new view we'll want on all blocks, with the end-goal being the deprecation of studio_view as an XBlock view. Is that your intent @bradenmacdonald ? I'm not necessarily against it, but you are essentially proposing a change to the XBlock API, which we'd want to communicate more broadly.

That was my intent, yes, but you're absolutely right that it's a broader change we'd need to shop around more first.

what about calling the old view studio_view_legacy, and calling this new view studio_view? If legacy Studio is truly the only client of the old view, then wouldn't preserving backwards compatibility be as simple as making legacy Studio request studio_view_legacy for these two blocks?

It's not just these two blocks unfortuntely; there are a number of (first and third party) XBlocks that make assumptions about the surrounding frontend runtime environment of the studio_view. The "API" for how XBlocks render and use studio_view has always been very fuzzy, and I'm pretty sure I've seen XBlocks rely on functionality like tabs in the studio edit dialog that depend on Studio's existing JS+CSS.

The block has several Youtube ID fields that are used for providing different playback speeds. Do we need to support these in the new frontend?

I don't think we need them anymore, no.

[bradenmacdonald]

Clarifying my comment about moving things to external repos: I think if anything is moved to a separate repo, it should be the video XBlock as a whole, including any "nano-frontend" code for its student and studio views. The new studio/edit view is too small to be an external repo on its own imho.

And, it doesn't have to actually be in a separate repo; but you can try to isolate it as though it's in a separate repo, as an intermediate step. Define its own dependencies in its own package.json file, including frontend-platform, don't import or use any tooling from the rest of edx-platform, etc.

[arbrandes]

@bradenmacdonald, @Kelketek

try to isolate it as though it's in a separate repo, as an intermediate step

We discussed this during the BD-14 weekly meeting, and the short of it is:

1. It is fine to do the isolation as Braden describes, in the block itself while still in edx-platform. No need for a separate repo. (Not even studio-frontend.) No-one thinks adding additional deployment pipelines is a good idea at the moment.

2. Wouldn't a simpler alternative be to hack up a "chromeless" version of author_view that can be consumed by the MFE (like the one that frontend-app-learning uses for student_view)? That way we could: a) avoid the problem entirely, while b) not having to jump through hoops to figure out what to wrap around the view on the MFE side.

@bradenmacdonald, on the off chance you've investigated (or just thought about) option 2 in the past, can you comment on it?

[bradenmacdonald]

@arbrandes

Wouldn't a simpler alternative be to hack up a "chromeless" version of author_view that can be consumed by the MFE (like the one that frontend-app-learning uses for student_view)? That way we could: a) avoid the problem entirely, while b) not having to jump through hoops to figure out what to wrap around the view on the MFE side.

I don't see how that solves the problem, but I don't really understand what you're suggesting. The chromeless view still includes all the Studio JS and CSS, so we'd have the problem of React version conflicts and lack of isolation. We'd also have things like jQuery that we don't need.

What I'm expecting is there is an edit_view that gets rendered in a responsive iframe and includes all its own JS+CSS dependencies. This would look identical to a chromeless vie being rendered in a responsive iframe, except the chromeless view would include conflicting JS+CSS files from Studio.

But I feel like I didn't really understand your suggestion...

[arbrandes]

@bradenmacdonald

But I feel like I didn't really understand your suggestion...

For the record, it wasn't mine: during the meeting, the point that frontend-app-learning can get away with the chromeless view was raised - and why can't we do the same for author_view. I did not have a good answer, hence why I'm asking you. :)

we'd have the problem of React version conflicts and lack of isolation

If it's running in an iframe in the MFE, we wouldn't - or would we? Or is the problem with the sandbox, and communicating with the parent of the iframe via wrap.js?

To be clear, what's being suggested here is for author_view to (continue to) be rendered by edx-platform, unaltered, except for the machinations required for rendering it chromeless with all dependencies included. This would require no alterations no any blocks, including this one, and would guarantee that any and all blocks' author_views would continue to work anywhere. We wouldn't have to include dependencies in wrap.js, either.

[kdmccormick]

@bradenmacdonald The chromeless-xblock-for-studio-view idea is mine, but @arbrandes articulates it well here.

My thinking is as simple as "the chromeless-xblock endpoint works great for the student_view in frontend-app-learning, how hard would it be to make a parallel chromeless-xblock for the studio_view?" I am happy to hear why this logic is flawed, though.

Stepping back, as much as I (and all the other devs on the Teaching & Learning team) would like to redo these blocks' views to not be so coupled to edx-platform, I'm nervous about the impact that it would have on the timeline of BD-14. That's why I've asked @arbrandes to size up how many blocks he thinks we'd need to modernize. If it's just two blocks, then sure, go ahead and rewrite the views in React! If it's many more than that, though, then we need another option -- hence, me bringing up chromeless-xblock.

[bradenmacdonald]

@kdmccormick @arbrandes

That's why I've asked @arbrandes to size up how many blocks he thinks we'd need to modernize. If it's just two blocks, then sure, go ahead and rewrite the views in React! If it's many more than that, though, then we need another option -- hence, me bringing up chromeless-xblock.

Good point. I think many third party XBlocks should work fine, perhaps with minor changes, but for the built-in blocks with an XModule heritage, they likely won't work at all. That said, it's worth testing them to see how much work is needed.

For example, I tested drag and drop v2 in Ramshackle, and it worked fine with no changes. Any XBlock (like LTI) which is based on my StudioEditableMixin should also work out of the box, or with some minor fixes to that mixin.

I know problem, video, and html won't work without changes, but I think that covering those (and the third party ones mostly working, or needing only minor adaptations) covers 90% or more.

what's being suggested here is for author_view to (continue to) be rendered by edx-platform, unaltered

I think what you're missing is that Studio's frontend code for author_view is not something that can be rendered chromelessly; it appears in a modal and relies on a tangled web of spaghetti code to render the editor widget in the modal, interact with the surrounding Studio UI, and save changes. If it already existed as an independent view that was capable of appearing on its own in an iframe, you're right, it'd be great. But unless there's been some refactoring that I'm not aware of, I think it would be a lot of work to make that happen (especially without breaking anything about how author_view currently works).

For example, the code to open the studio_view modal relies on the author_view already being rendered in Studio on the current page.

[arbrandes]

@bradenmacdonald

it appears in a modal and relies on a tangled web of spaghetti code to render the editor widget in the modal, interact with the surrounding Studio UI, and save changes. (...) I think it would be a lot of work to make [chromeless] happen

This is in essence what I figured to be the case, but did not know for sure. You explained it much better, in any case. Thanks! :)

@kdmccormick, I'll still scout out a list of the built-in blocks whose author_views are known to not work in the new MFE, but do work in legacy libraries. As I noted earlier today, legacy libraries don't work with all XBlocks either, so the list won't be huge.

[davidjoy]

I want to call attention to some work that @muselesscreator has been doing here for the ORA XBlock, around building it's frontend with webpack.

https://github.com/muselesscreator/edx-webpack-xblock
https://github.com/muselesscreator/webpack_xblock_utils
openedx/edx-ora2#1512

While still in draft form, the goal here is to get modern JS tooling into the edx-ora2 repository in support of bringing it closer to our MFEs and other modern frontend repos.

I'd like to see us avoid using python tooling to build JavaScript assets - as a high-level best practice, we should keep the build technology aligned with the thing being built/as standard and vanilla as possible.

For the video XBlock, I'm wondering if there's an opportunity here to at least pull the editor-facing frontend of the XBlock out of the monolith and build it via a webpack/frontend-build-based config, rather than leaving it in edx-platform where it will have to share dependencies with the monolith. I'm curious about opinions on that split as an incremental step toward pulling it out completely. (Note that the above repos use a custom webpack config, but I think ultimately we can fold that into frontend-build too to be shared across XBlocks)

The XBlock's frontend build script would provide compiled assets that can then be used in edx-platform's asset pipeline as they are today, again, as an incremental step. (This is admittedly a somewhat hand-wavy statement on my part). It sounds as if the video XBlock's tight coupling with edx-platform is causing us pain to begin with; I'm not sure who might be interested in helping to see this work through to pull the rest of the XBlock out, but I don't expect the BD-14 team really wants to take all that on as part of this project.

Another small side point - there are parts of frontend-platform that might be helpful for an XBlock, and parts that probably won't function being loaded from edx-platform yet. The 'auth' code will be useful for making JWT-based API requests to the XBlock's backend (I'm assuming those endpoints are JWT-enabled?), but logging, analytics, and i18n will all most likely be broken/non-functional for various reasons.

Again, I think there's virtue here in starting to bring XBlock frontends closer to our latest-and-greatest frontend libraries, but the work for that may not really be something we want to bite off in this project. Admittedly, I'm only tangentially involved!

[edx-status-bot]

Your PR has finished running tests. The following contexts failed:

• jenkins/python-3.8/a11y
• jenkins/a11y
• jenkins/js
• jenkins/python-3.8/js
• jenkins/python-3.8/quality
• jenkins/quality

[kdmccormick]

I'm going to close this as we get ready to pause BD-14 development for now. Thanks for your work on it so far @Kelketek. Your PoC and learnings here will be useful when we figure out how to tackle the editor issue in Milestone 3.