Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: support rendering XBlocks in the Learning MFE [FC-0035] #34161

Closed
wants to merge 1 commit into from
Closed

feat: support rendering XBlocks in the Learning MFE [FC-0035] #34161

wants to merge 1 commit into from

Conversation

Agrendalath
Copy link
Member

@Agrendalath Agrendalath commented Feb 1, 2024 •
edited by arbrandes
Loading

Rendering blocks directly in the MFE was developed for the Library Authoring MFE. The solution was then ported to the Learning MFE. This commit includes backend changes required to retrieve data of non-blockstore XBlocks from LMS.

Please see openedx/frontend-app-learning#1281 for the details and testing instructions.

Other information

@Agrendalath Agrendalath self-assigned this Feb 1, 2024
@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Feb 1, 2024
@openedx-webhooks
Copy link

Thanks for the pull request, @Agrendalath! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

@Agrendalath Agrendalath mentioned this pull request Feb 1, 2024
Closed
@Agrendalath
feat: support rendering XBlocks in the Learning MFE
b13c287 
Rendering blocks directly in the MFE was developed for the Library Authoring
MFE. The solution was then ported to the Learning MFE. This commit includes
backend changes required to retrieve data of non-blockstore XBlocks from LMS.
navinkarkera
navinkarkera approved these changes Feb 6, 2024
View reviewed changes
Copy link
Contributor

@navinkarkera navinkarkera left a comment

Choose a reason for hiding this comment

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

@Agrendalath 👍

@arbrandes arbrandes self-requested a review March 6, 2024 18:29
@arbrandes arbrandes mentioned this pull request Mar 6, 2024
Closed
@itsjeyd itsjeyd requested a review from ormsbee March 22, 2024 09:12
@mphilbrick211
Copy link

Hi @ormsbee and @arbrandes! Just checking in on this. I think we're waiting on this to close the project.

ormsbee
ormsbee requested changes Apr 12, 2024
View reviewed changes
Copy link
Contributor

@ormsbee ormsbee left a comment

Choose a reason for hiding this comment

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

My apologies, but I think there's something fundamental about how this is supposed to work that I'm not understanding–particularly around why we're doing branching based on isinstance checks against BlockUsageLocator, and why it's okay to change the contract for student_view_data.

lms/templates/staff_problem_info.html
@@ -143,7 +143,7 @@ <h2>${_("Submission History Viewer")}</h2>
<script type="text/javascript">
// assumes courseware.html's loaded this method.
$(function () {
setup_debug('${element_id | n, js_escaped_string}',
(typeof setup_debug !== 'undefined') && setup_debug('${element_id | n, js_escaped_string}',
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a comment about why this change is necessary.

openedx/core/djangoapps/xblock/api.py
# The second method is more efficient, but it doesn't work with the DescriptorSystem.
display_name = block.display_name
else:
display_name = get_block_display_name(block)
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't get_block_display_name just check for the display_name attribute with fallback? Why does it break?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, when would usage_id not be a BlockUsageLocator?

openedx/core/djangoapps/xblock/api.py
}

if "index_dictionary" in includes:
data["index_dictionary"] = block.index_dictionary()

if "student_view_data" in includes:
data["student_view_data"] = block.student_view_data() if hasattr(block, 'student_view_data') else None
if isinstance(usage_id, BlockUsageLocator):
data["student_view_data"] = render_block_view(block, 'student_view', None).content
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't the content the HTML rendering? Shouldn't student_view_data always be a JSON serializable dict on the XBlocks that support it?

@itsjeyd itsjeyd added the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Apr 22, 2024
@mphilbrick211
Copy link

Hi @Agrendalath and @ormsbee! Just checking in on this.

@arbrandes
Copy link
Contributor

We've found that the Library Authoring approach to using an iframe per XBlock doesn't give users a good enough UX. See my comment in the PR that was meant to add the same approach to Course Authoring:

openedx/frontend-app-authoring#964 (review)

We're going to need a better plan if we want to stop using the backend to render whole units.

To be clear, this is not to say that this PR is wrong. We might find a frontend-only solution to the performance issues. But we should probably hold off here until we've had that conversation.

@mphilbrick211
Copy link

We've found that the Library Authoring approach to using an iframe per XBlock doesn't give users a good enough UX. See my comment in the PR that was meant to add the same approach to Course Authoring:

openedx/frontend-app-course-authoring#964 (review)

We're going to need a better plan if we want to stop using the backend to render whole units.

To be clear, this is not to say that this PR is wrong. We might find a frontend-only solution to the performance issues. But we should probably hold off here until we've had that conversation.

FYI @ormsbee @Agrendalath

@Agrendalath
Copy link
Member Author

Thank you for sharing it, @arbrandes. In this case, I suppose we should close this and openedx/frontend-app-learning#1281 for now.

cc: @ormsbee, @mphilbrick211

@openedx-webhooks
Copy link

@Agrendalath Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future.

@Agrendalath Agrendalath deleted the agrendalath/xblock_rendering branch May 17, 2024 19:28
@itsjeyd itsjeyd removed the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label May 23, 2024
@GlugovGrGlib GlugovGrGlib mentioned this pull request Jun 6, 2024
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ormsbee ormsbee ormsbee requested changes

@navinkarkera navinkarkera navinkarkera approved these changes

@arbrandes arbrandes Awaiting requested review from arbrandes

Assignees

@Agrendalath Agrendalath

Labels
open-source-contribution PR author is not from Axim or 2U
Projects
Contributions
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@Agrendalath @openedx-webhooks @mphilbrick211 @arbrandes @ormsbee @navinkarkera @itsjeyd