feat: [FC-0070] Create a new Studio view for rendering whole Unit in an iframe #35587

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

cms/templates/container_iframe.html

+                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

cms/templates/container_iframe.html

+    }());
+    </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

cms/urls.py

@@ -123,6 +123,8 @@
            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

cms/djangoapps/contentstore/views/component.py

+@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

cms/djangoapps/contentstore/views/component.py

+                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

cms/djangoapps/contentstore/views/component.py

@@ -35,7 +35,8 @@

 __all__ = [
    '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

cms/static/js/views/pages/container.js

@@ -207,7 +207,7 @@ function($, _, Backbone, gettext, BasePage,

        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

cms/static/js/views/pages/container.js

            const taxonomyTagsWidgetUrl = this.model.get('taxonomy_tags_widget_url');
            const contentId = this.findXBlockElement(event.target).data('locator');

            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

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

cms/static/js/views/pages/container.js

+            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);

}

PKulkoRaccoonGang

👍

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

cms/templates/container_chromeless.html

+            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;

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

cms/static/js/views/pages/container.js

+                            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 💯

PKulkoRaccoonGang · 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

cms/static/sass/elements/_controls.scss

+%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

cms/static/sass/course-unit-mfe-iframe-bundle.scss

+    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:

edx-platform/cms/static/sass/_base.scss

Line 313 in 2bbd8ec

padding: ($baseline/4) ($baseline/2) ($baseline/3) ($baseline/2);

)

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

cms/static/sass/course-unit-mfe-iframe-bundle.scss

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


[nit] Should be simplified to one padding rule.

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

cms/static/sass/course-unit-mfe-iframe-bundle.scss

+  .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

cms/static/sass/course-unit-mfe-iframe-bundle.scss

+
+  .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

cms/static/sass/course-unit-mfe-iframe-bundle.scss

+    }
+  }
+
+  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

cms/static/sass/course-unit-mfe-iframe-bundle.scss

+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

cms/static/sass/course-unit-mfe-iframe-bundle.scss

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


[question] Do we need this comments?

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

cms/static/images/pencil-icon.svg

+<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

cms/djangoapps/contentstore/views/component.py

+    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

cms/djangoapps/contentstore/views/component.py

+        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

cms/djangoapps/contentstore/views/component.py

+            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

cms/djangoapps/contentstore/views/component.py

+
+    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

cms/djangoapps/contentstore/views/component.py

+        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

cms/djangoapps/contentstore/views/component.py

+            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

cms/static/sass/course-unit-mfe-iframe-bundle.scss

+    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

cms/templates/container_chromeless.html

@@ -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

cms/templates/container_chromeless.html

+            <%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

cms/templates/container_chromeless.html

+            );
+        </%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

cms/djangoapps/contentstore/views/component.py

@@ -141,6 +142,35 @@ def container_handler(request, usage_key_string):  # pylint: disable=too-many-st
        return HttpResponseBadRequest("Only supports HTML requests")


+@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

PKulkoRaccoonGang reviewed Oct 8, 2024

View reviewed changes

PKulkoRaccoonGang 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

PKulkoRaccoonGang approved these changes Oct 8, 2024

View reviewed changes

PKulkoRaccoonGang 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

PKulkoRaccoonGang 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

PKulkoRaccoonGang 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
49 checks passed

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

		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'):

feat: [FC-0070] Create a new Studio view for rendering whole Unit in an iframe #35587

feat: [FC-0070] Create a new Studio view for rendering whole Unit in an iframe #35587

Conversation

UvgenGen commented Oct 3, 2024 • edited Loading

Description

Supporting information

Deadline

Other information

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?

GlugovGrGlib left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

UvgenGen Oct 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PKulkoRaccoonGang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PKulkoRaccoonGang Oct 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PKulkoRaccoonGang commented Oct 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ormsbee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

UvgenGen commented Oct 3, 2024 •

edited

Loading

UvgenGen Oct 7, 2024 •

edited

Loading

PKulkoRaccoonGang Oct 9, 2024 •

edited

Loading

UvgenGen Oct 17, 2024 •

edited

Loading