feat: [FC-0070] Create a new Studio view for rendering whole Unit in an iframe by UvgenGen · Pull Request #35587 · openedx/openedx-platform

UvgenGen · 2024-10-03T15:32:24Z

… with xBlocks controls

Description

Created a new Studio view for rendering the whole Unit in an iframe with xBlocks controls for studio MFE.

A new studio page renders all xBlocks in the unit with xBlock controls
Studio page sends postmessages to the host in case of size change and executing control actions

Issue: openedx/platform-roadmap#321

Supporting information

URL example: http://studio.local.edly.io:8001/container_embed/block-v1:test+test+test+type@vertical+block@47ca674b3f0f46d7a8be12d8fcccd232

Result:

Added tests for container_embed_handler. Run pytest /cms/djangoapps/contentstore/views/tests/test_container_page.py for local testing inside LMS container.

Deadline

Other information

openedx-webhooks · 2024-10-03T15:32:27Z

Thanks for the pull request, @UvgenGen!

What's next?

Please work through the following steps to get your changes ready for engineering review:

🔘 Get product approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
- This process (including the steps you'll need to take) is documented here.
If it doesn't, simply proceed with the next step.

🔘 Provide context

To help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:

Dependencies

This PR must be merged before / after / at the same time as ...
Blockers

This PR is waiting for OEP-1234 to be accepted.
Timeline information

This PR must be merged by XX date because ...
Partner information

This is for a course on edx.org.
Supporting documentation
Relevant Open edX discussion forum threads

🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

🔘 Let us know that your PR is ready for review:

Who will review my changes?

This repository is currently maintained by @openedx/wg-maintenance-edx-platform. Tag them in a comment and let them know that your changes are ready for review.

Where can I find more information?

If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:

When can I expect my changes to be merged?

Our goal is to get community contributions seen and reviewed as efficiently as possible.

However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:

The size and impact of the changes that it introduces
The need for product review
Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

GlugovGrGlib

First review round passed, please update description and check review comments

GlugovGrGlib · 2024-10-03T16:09:04Z

+                lastHeight = newHeight;
+                lastWidth = newWidth;
+
+                // Within the learning microfrontend the iframe resizes to match the


As this template is dedicated to be used in the authoring MFE only please rename all comments about learning MFE in it

GlugovGrGlib · 2024-10-03T16:13:09Z

+    }());
+    </script>
+    <script>
+    var beamer_config = {


Where was this copied from?
I'm not sure about its usage, but I think this beamer script should be removed from this CMS view.

This is copied from base.html

Let's remove it from this template

GlugovGrGlib · 2024-10-03T16:38:35Z

            name='course_rerun_handler'),
    re_path(fr'^container/{settings.USAGE_KEY_PATTERN}$', contentstore_views.container_handler,
            name='container_handler'),
+    re_path(fr'^container_iframe/{settings.USAGE_KEY_PATTERN}$', contentstore_views.container_handler_iframe,


container_embed might be a better name for a URL
Also, in case of change please update container_handler_iframe to container_embed_handler

GlugovGrGlib · 2024-10-03T16:39:38Z

+@login_required
+def container_handler_iframe(request, usage_key_string):  # pylint: disable=too-many-statements
+    """
+    The restful handler for container xblock requests.


Please update docstring to match specific purpose of this view

GlugovGrGlib · 2024-10-03T16:42:04Z

+                return HttpResponseBadRequest()
+
+            container_handler_context = get_container_handler_context(request, usage_key, course, xblock)
+            return render_to_response('container_iframe.html', container_handler_context)


Please rename template to container_chromeless.html to outline relation with LMS's courseware_chromeless

GlugovGrGlib · 2024-10-03T16:43:07Z

    'container_handler',
-    'component_handler'
+    'component_handler',
+    'container_handler_iframe'


Do you mind adding a comma after the last element in the list?

comma added

GlugovGrGlib · 2024-10-03T16:54:59Z

        renderAddXBlockComponents: function() {
            var self = this;
-            if (self.options.canEdit) {
+            if (self.options.canEdit && !self.options.isIframePage) {


Can we update parameter isIframePage to isIframeEmbed

GlugovGrGlib · 2024-10-03T16:56:53Z

            TaggingDrawerUtils.openDrawer(taxonomyTagsWidgetUrl, contentId);
        },

-        showMoveXBlockModal: function(event) {


I can't exactly understand why some functions were completely removed from this file?

Some methods were duplicated, so I just removed the extra ones

Strange, can you assume what is the reason for it, have you noticed anything strange in behavior on the legacy Unit page in CMS after the removal of these functions?

I assume these methods were duplicated by mistake during some rebasing/merging because they are equal.

duplicated methods added in: https://github.com/openedx/edx-platform/pull/33263/files#diff-1b7855448b6086c2a313e0b9ecff179e991106aac52648d880f6f9d551e64577R531

peterkulko · 2024-10-08T14:53:31Z

+            if (this.options.isIframeEmbed) {
+                window.parent.postMessage(
+                    {
+                        type: 'duplicateXBlock',
+                        payload: {}
+                    }, document.referrer
+                );
+                return
+            }


[optional]: At this stage we do not expect fully working functionality, but it seems to me that it would be great to provide minimal working functionality. Since now after any action we get the error like Failed to execute 'postMessage' on 'Window': Invalid target origin '' in a call to 'postMessage'. I suggest catching such errors and not blocking the execution of the JS code flow. My proposal applies to all syntactic constructions in this file that are related to postMessage. What are your thoughts?

Suggested change

if (this.options.isIframeEmbed) {

window.parent.postMessage(

{

type: 'duplicateXBlock',

payload: {}

}, document.referrer

);

return

}

try {

if (this.options.isIframeEmbed) {

window.parent.postMessage(

{

type: 'duplicateXBlock',

payload: {}

}, document.referrer

);

return

}

} catch (e) {

console.error(e);

}

peterkulko

👍

peterkulko · 2024-10-09T09:30:53Z

+            var lastHeight = window.parent.offsetHeight;
+            var lastWidth = window.parent.offsetWidth;


[nit]: During rendering of the iframe I noticed that we get errors related to the fact that the window.parent offsetHeight and offsetWidth values are not defined. They are inside the object, for example:

Suggested change

var lastHeight = window.parent.offsetHeight;

var lastWidth = window.parent.offsetWidth;

var lastHeight = window.parent[0].offsetHeight;

var lastWidth = window.parent[0].offsetWidth;

peterkulko · 2024-10-09T09:35:25Z

+                            payload: {}
+                        }, document.referrer
+                    );
+                    return


[nit/inform]: We also decided that in future changes we will switch the processing of logic of different methods using conditions related to types of iframe post messages (for example, for some xblocks we want to show new modal windows, we plan to send the type of post message, for example "newModalWindow").
Let's remove return in all such constructions 💯

peterkulko · 2024-10-09T20:50:25Z

[request/inform]: @GlugovGrGlib @UvgenGen we decided that we want to remove the Paste Component button (and the tooltip with information about the component) from the iframe, since we already have a ready-made functionality for inserting an xblock on the page in which the iframe is embedded.

ihor-romaniuk · 2024-10-15T16:38:56Z

+%button-styles {
+  width: 44px;
+  height: 44px;
+  border-radius: 50%;
+}


[question] Do we need to declare %button-styles and other prepared style blocks in two files: _controls.scss and _course-unit-mfe-iframe.scss?

No, I deleted it from _controls.scss

ihor-romaniuk · 2024-10-15T16:49:22Z

+    padding: 24px;
+    padding-bottom: 12px;


[nit] Should be simplified to one padding rule.
Also, we can stick to general rules for margin/paddings with using $baseline variable (ex: https://github.com/openedx/edx-platform/blob/2bbd8ecd180fd299de2c4ee4bc1542ccb97b50bb/cms/static/sass/_base.scss#L313)

ihor-romaniuk · 2024-10-15T16:54:18Z

+    padding: 24px;
+    padding-top: 12px;


[nit] Should be simplified to one padding rule.

ihor-romaniuk · 2024-10-15T16:55:13Z

+  .wrapper-xblock .header-actions .actions-list .action-item .action-button {
+    color: $black;
+
+    @extend %button-styles;


[nit] Let's declare all extended rules at the top of the block for separation and simple overriding.

Moved at the top

ihor-romaniuk · 2024-10-15T16:56:44Z

+
+  .nav-item {
+    border-bottom: none;
+    color: #212529;


[question] Can we use existing variables or create new variables for colors instead of hardcoding them?

Created some new variables

ihor-romaniuk · 2024-10-15T16:57:33Z

+    }
+  }
+
+  input:not([type="radio"]):not([type="checkbox"]):not([type="submit"]) {


[nit] Can be simplified

input:not([type="radio"], [type="checkbox"], [type="submit"]) {

Good catch! Done

ihor-romaniuk · 2024-10-15T16:59:15Z

+body #openassessment-editor #openassessment_editor_header,
+body #openassessment-editor .openassessment_tab_instructions {
+  background-color: $white;
+}


[nit] Let's include in the block below:

body { #openassessment-editor { #openassessment_editor_header, .openassessment_tab_instructions { background-color: $white; } } ... }

I did some additional refactoring

removed everything !important

removed access to elements via body

ihor-romaniuk · 2024-10-15T17:00:06Z

+// [class*="view-"] .modal-window .editor-with-buttons .xblock-actions {
+//   bottom: -6px;
+// }


[question] Do we need this comments?

ormsbee · 2024-10-16T04:51:23Z

+<svg width="24" height="24" viewBox="0 0 24 24" fill="none" xmlns="http://www.w3.org/2000/svg" role="img" focusable="false" aria-hidden="true">
+  <path d="M3 17.25V21h3.75L17.81 9.94l-3.75-3.75L3 17.25ZM21.41 6.34l-3.75-3.75-2.53 2.54 3.75 3.75 2.53-2.54Z" fill="currentColor"></path>
+</svg>


I'll try to have a real review in the next day, but a quick question: where does this icon come from?

Thanks
This icon from Paragon icons

ormsbee

Some questions and requests. Nothing major. I don't feel like I can meaningfully review any of the styles though.

ormsbee · 2024-10-17T02:13:34Z

+    Returns an HttpResponse with HTML content for the container xBlock.
+    The returned HTML is a chromeless rendering of the xBlock.


Suggested change

Returns an HttpResponse with HTML content for the container xBlock.

The returned HTML is a chromeless rendering of the xBlock.

Returns an HttpResponse with HTML content for the container XBlock.

The returned HTML is a chromeless rendering of the XBlock.

[nit, optional]: The standard spelling is "XBlock".

ormsbee · 2024-10-17T02:14:10Z

+        json: not currently supported
+    """
+
+    from ..utils import get_container_handler_context


[question]: Is this to avoid a circular dependency?

Yes, it is. I added a comment for it.

ormsbee · 2024-10-17T02:23:33Z

+            container_handler_context = get_container_handler_context(request, usage_key, course, xblock)
+            return render_to_response('container_chromeless.html', container_handler_context)
+    else:
+        return HttpResponseBadRequest("Only supports HTML requests")


[request] If you have an error condition that can return early, please do that instead of doing an if-else and indenting the main logic of the function. It makes the primary flow of the code easier to read.

So in this case, something like:

if 'text/html' not in request.META.get('HTTP_ACCEPT', 'text/html'): return HttpResponseBadRequest("Only supports HTML requests") # rest of the function here.

ormsbee · 2024-10-17T02:32:12Z

+
+    from ..utils import get_container_handler_context
+
+    if 'text/html' in request.META.get('HTTP_ACCEPT', 'text/html'):


[question] Are we worried about this getting hit with an http-accept header that only wants JSON? I don't usually see this kind of guard in a view that renders HTML.

I created this function based on container_handler, but I agree that this check can be removed. Removed.

ormsbee · 2024-10-17T02:33:19Z

+        try:
+            usage_key = UsageKey.from_string(usage_key_string)
+        except InvalidKeyError:  # Raise Http404 on invalid 'usage_key_string'
+            raise Http404  # lint-amnesty, pylint: disable=raise-missing-from


[request] An invalid key should be a 400, not a 404, since it's a malformed request.

ormsbee · 2024-10-17T02:33:59Z

+            try:
+                course, xblock, lms_link, preview_lms_link = _get_item_in_course(request, usage_key)
+            except ItemNotFoundError:
+                return HttpResponseBadRequest()


[request] If the UsageKey is well formed but not found, then this looks like the time to return the 404 error.

ormsbee · 2024-10-17T02:37:19Z

+    display: inline-block;
+  }
+
+  #openassessment-editor #oa_rubric_editor_wrapper {


[question] Why do we need to bundle the ORA CSS here, and the Drag & Drop CSS below? This is a question, not a request to change this behavior at this time.

Adding styles for ORA and Drag & Drop to this bundle is an improvement of visual perception for the new interface. We have two approaches for modal windows. The first is based on the react interface (full screen modal windows, for example for the Text component), the second is legacy modal windows. The current bundle provides styles exclusively for legacy modal windows inside the iframe that is embedded on the course unit page inside MFE Authoring. For the original legacy page, changing the styles of modal windows will not be available (their display will not change).
Also based on the fact that these xblocks are enabled by default and the code for these xblocks is outside the platform's code structure, additional styles were added.

ormsbee · 2024-10-17T03:04:59Z

@@ -0,0 +1,279 @@
+## coding=utf-8


[request] Please add some sort of comment to this file to explain what it is, why it exists, and how it's intended to be different from the template file it's based off of.

Added comment with a description

ormsbee · 2024-10-17T03:06:35Z

+            <%static:include path="common/templates/image-modal.underscore" />
+        </script>
+        <link rel="stylesheet" type="text/css" href="${static.url('js/vendor/timepicker/jquery.timepicker.css')}" />
+        % if not settings.STUDIO_FRONTEND_CONTAINER_URL:


[request] Please add a comment explaining why we're doing this.

Added comment

ormsbee · 2024-10-17T03:09:46Z

+            );
+        </%static:webpack>
+    </body>
+    <script type="text/javascript">


[request] There's a lot of boilerplate in this template, along with stuff we probably don't strictly need (like the hotjar piece). That's okay–this isn't intended to be long-lived, and it's a fast way to get up and running. But please use comments to highlight the parts of this template that are specific to the new UI you're building (and not just copied from the older template).

Added comment for specific script block and removed the hotjar piece.

ormsbee · 2024-10-17T17:58:38Z

@UvgenGen: Can you please squash + rebase? The static analysis bit should work after that. I just want to make sure it's not masking any other issues. Do you need me to merge these for you, or does someone on your team have merge rights for edx-platform?

UvgenGen · 2024-10-18T09:06:10Z

@ormsbee PR updated and squashed. I would appreciate your help with merging.

GlugovGrGlib

One last comment to fix before merging it

GlugovGrGlib · 2024-10-18T11:48:32Z



+@require_GET
+@login_required


It seems like we are missing a decorator that is required to use this view in an Iframe. Please add xframe_options_exempt as it is done in LMS: https://github.com/openedx/edx-platform/blob/master/lms/djangoapps/courseware/views/views.py#L1546.

xframe_options_exempt decorator added.

…an iframe

edx-pipeline-bot · 2024-10-18T18:40:08Z

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

edx-pipeline-bot · 2024-10-18T19:00:03Z

2U Release Notice: This PR has been deployed to the edX production environment.

edx-pipeline-bot · 2024-10-18T19:00:03Z

2U Release Notice: This PR has been deployed to the edX production environment.

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Oct 3, 2024

GlugovGrGlib suggested changes Oct 4, 2024

View reviewed changes

UvgenGen force-pushed the sagirov/AXIMST-850 branch 3 times, most recently from 9a29b0e to 69b83f2 Compare October 8, 2024 14:50

UvgenGen requested a review from GlugovGrGlib October 8, 2024 14:50

peterkulko reviewed Oct 8, 2024

View reviewed changes

peterkulko suggested changes Oct 8, 2024

View reviewed changes

UvgenGen force-pushed the sagirov/AXIMST-850 branch from 69b83f2 to b9456e2 Compare October 8, 2024 15:03

peterkulko approved these changes Oct 8, 2024

View reviewed changes

peterkulko reviewed Oct 9, 2024

View reviewed changes

UvgenGen force-pushed the sagirov/AXIMST-850 branch 2 times, most recently from ea1ad59 to c0f065d Compare October 9, 2024 15:15

mphilbrick211 added the FC Relates to an Axim Funded Contribution project label Oct 10, 2024

peterkulko mentioned this pull request Oct 13, 2024

feat: [FC-0070] render iframe with xblocks openedx/frontend-app-authoring#1375

Merged

GlugovGrGlib approved these changes Oct 15, 2024

View reviewed changes

GlugovGrGlib requested a review from ormsbee October 15, 2024 15:15

UvgenGen changed the title ~~DRAFT: feat: [FC-0070] Create a new Studio view for rendering whole Unit in an iframe~~ feat: [FC-0070] Create a new Studio view for rendering whole Unit in an iframe Oct 15, 2024

GlugovGrGlib force-pushed the sagirov/AXIMST-850 branch from c0f065d to 62b5ec6 Compare October 15, 2024 15:21

GlugovGrGlib requested a review from arbrandes October 15, 2024 15:21

ihor-romaniuk reviewed Oct 15, 2024

View reviewed changes

ormsbee reviewed Oct 16, 2024

View reviewed changes

peterkulko requested a review from ihor-romaniuk October 16, 2024 08:21

ihor-romaniuk approved these changes Oct 16, 2024

View reviewed changes

ormsbee requested changes Oct 17, 2024

View reviewed changes

UvgenGen requested a review from ormsbee October 17, 2024 11:24

ormsbee approved these changes Oct 17, 2024

View reviewed changes

UvgenGen force-pushed the sagirov/AXIMST-850 branch from 47d1c02 to 1b09b12 Compare October 18, 2024 08:38

GlugovGrGlib suggested changes Oct 18, 2024

View reviewed changes

feat: [FC-0070] Create a new Studio view for rendering whole Unit in …

9c2d0da

…an iframe

UvgenGen force-pushed the sagirov/AXIMST-850 branch from 1b09b12 to 9c2d0da Compare October 18, 2024 11:58

ormsbee merged commit e4a1e41 into openedx:master Oct 18, 2024

navinkarkera mentioned this pull request Oct 19, 2024

feat: adds Library Content button to Studio Unit page [FC-0062] #35670

Merged

This was referenced Oct 28, 2024

feat: [FC-0070] add message events to the unit page container #35693

Merged

feat: [FC-0070] implement move xblock modal openedx/frontend-app-authoring#1422

Merged

salman2013 mentioned this pull request Aug 13, 2026

feat: remove legacy Django unit editor #38985

Draft

4 tasks

		var lastHeight = window.parent.offsetHeight;
		var lastWidth = window.parent.offsetWidth;

		Returns an HttpResponse with HTML content for the container xBlock.
		The returned HTML is a chromeless rendering of the xBlock.


		from ..utils import get_container_handler_context

		if 'text/html' in request.META.get('HTTP_ACCEPT', 'text/html'):



		@require_GET
		@login_required

Uh oh!

Conversation

UvgenGen commented Oct 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Supporting information

Deadline

Other information

Uh oh!

openedx-webhooks commented Oct 3, 2024

What's next?

🔘 Get product approval

🔘 Provide context

🔘 Get a green build

🔘 Let us know that your PR is ready for review:

Who will review my changes?

Where can I find more information?

When can I expect my changes to be merged?

Uh oh!

GlugovGrGlib left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

UvgenGen Oct 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peterkulko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peterkulko Oct 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

UvgenGen commented Oct 3, 2024 •

edited

Loading

UvgenGen Oct 7, 2024 •

edited

Loading

peterkulko Oct 9, 2024 •

edited

Loading