-
Notifications
You must be signed in to change notification settings - Fork 0
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.
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
PADV-303: LTI launch complete course rendering ADR #4
base: main
Are you sure you want to change the base?
Conversation
Currently, there is no method to render a whole course from an LTI 1.3 | ||
launch, the lti_provider app on the edx-platform, is only able to render | ||
sequences or units from a course using the render_xblock function, this | ||
function can only render parts of the course but not the course has a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
function can only render parts of the course but not the course has a | |
function can only render parts of the course but not the course as a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alexjmpb This line of text doesn't exist anymore on the file
different from a normal user, having a more clear separation of the | ||
LTI launch paths from normal course content learning MFE paths. | ||
|
||
Approach 4: Render using a view on our plugin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I'm talking from the ignorance here, but wouldn't it be better to create an MFE to render the content that we want and then render that MFE in an xblock? I think it's better than creating an MFE to render the whole course, sure if it is possible. What do you think, can this be done?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if I understood your question
There is already a way to render specific content of the course using the render_xblock view on edx-platform, we wouldn't need to create a whole MFE if we just needed to render specific content from the course, but in our case we need to be able to display the whole course and that either implies modifying the learning MFE, create our own MFE or creating our own view to render the content with a navigation menu with the course outline
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes sorry for that, after I wrote the question i thought it better hahaha, thanks!
different from a normal user, having a more clear separation of the | ||
LTI launch paths from normal course content learning MFE paths. | ||
|
||
Approach 4: Render using a view on our plugin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about adding a diagram by specifying the Model-Template-View + Controller?
Co-authored-by: Javier Torres <[email protected]>
4737047
to
ad84d0a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kuipumu: I think this doc is great, and does an excellent job in weighing the tradeoffs of these approaches. I do think that in the long run, an approach that makes use of the existing Learning MFE in some way will be beneficial, since you're always going to have to worry about feature drift if you're maintaining your own navigation code. That being said, in the short-to-medium term, I agree that going with Approach 4 as you folks have here is likely to allow you to develop more quickly and with less friction.
Some questions/suggestions for you to consider:
Is your proposed new course navigation really LTI-specific?
One thing I would like you to consider is whether this is truly an LTI-specific view of a course, or if it represents something else with a broader use case that LTI just happens to make use of. For instance, when the /xblock
endpoint was first imagined, it was seen as a mobile-app rendering specific view, but we quickly realized that it was also useful for the LTI use case, and later for the Learning MFE.
If this does represent a different sort of navigation through the course and has re-use outside of LTI, it might make sense to implement it as multiple plugins that serve to separate navigation from LTI profile checking–even if those plugins are both developed in the same repo for now.
Again, this is a genuine question–I can't think of other use cases for this, but you folks have thought about this a lot more than I have.
Please consider what it would mean to make the Learning MFE integrate this cleanly.
As time goes on, there are going to be more and more things added to the Learning MFE, and the capabilities will drift apart. You might find things you really want to leverage (motivational toasts, upsells, proctoring, etc.) that you need to re-implement in your own navigational space.
I know that the Learning MFE isn't in a place where it's easy to isolate and cleanly plug in your functionality today, but I think it would be very beneficial to get the feedback for what it would take to make it so in the long term. This is not a guarantee that any such proposals would be accepted, but I think it would be worth your time to make a writeup of this after you've delivered your initial implementation of Approach 4.
It's great to see this work being done. Thank you so much for documenting your thoughts so clearly!
---------------- | ||
|
||
.. image:: https://mermaid.ink/img/pako:eNptkstuwjAQRX9l5DX8QBZFiEehYoFKukqyGOKBWHXs1B4LocC_12kITaVmZcXnnrFv0orSShKJODtsKkiXuYH4zNtc7NItaAymrMDRVyDPYA2keNzjmWa5uD9QmE5fbmvUnm6wzB77oDwEg5rJkSw6bkynLkR40XYjGh3OygAZPGqSs4d18cc6ftdnV-2HJwcVeoiSvbMnpeNQ4xlNSYNl9Y9lNbKs262HBrkC1Npefsevx8HX7J04uHjGuuHr0EAxJnvdZgCHEi6KKxsYNoQynhaNhLW1sZNHeNOFYZsdKO4ov2O16wtv0GFNEQS2T5vr5aWtG2vIsC-etW5_RG_ZvCscykpp6WiMxrAPmgsxETW5GpWM37ztwrngimrKRRKXEt1nLnJzjxwGtoerKUXC8XYTERqJTEuF8VepRXLqyrl_A1FtvKc?type=png)](https://mermaid.live/edit#pako:eNptkstuwjAQRX9l5DX8QBZFiEehYoFKukqyGOKBWHXs1B4LocC_12kITaVmZcXnnrFv0orSShKJODtsKkiXuYH4zNtc7NItaAymrMDRVyDPYA2keNzjmWa5uD9QmE5fbmvUnm6wzB77oDwEg5rJkSw6bkynLkR40XYjGh3OygAZPGqSs4d18cc6ftdnV-2HJwcVeoiSvbMnpeNQ4xlNSYNl9Y9lNbKs262HBrkC1Npefsevx8HX7J04uHjGuuHr0EAxJnvdZgCHEi6KKxsYNoQynhaNhLW1sZNHeNOFYZsdKO4ov2O16wtv0GFNEQS2T5vr5aWtG2vIsC-etW5_RG_ZvCscykpp6WiMxrAPmgsxETW5GpWM37ztwrngimrKRRKXEt1nLnJzjxwGtoerKUXC8XYTERqJTEuF8VepRXLqyrl_A1FtvKc | ||
:width: 729 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No change required, but did you know you can put Mermaid markup directly in GitHub these days? Just thought it'd be more convenient for you folks in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!, I didn't know, its the first time I use Mermaid on Github so I wasn't aware of the feature, will consider it next time I use a diagram on Github.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks very well-considered. I appreciate the writeup.
To echo @ormsbee's point, in case you ever changed your mind, I think it would be very useful to have a chromeless version of the Learning MFE available, agnostic to whether the request is coming from an LTI launch. I do worry that the additional iframe layer (Client > LTI launch of MFE > Unit) could be a performance issue for end users, though.
Anyway, the approach you landed on sounds good. Just one question.
4. If the user has permission to load the view, we will call the render_xblock view | ||
to render the requested XBlock to the user alongside a navigation menu with | ||
the course outline. (This could be done by either injecting the XBlock using | ||
the xblock_view unpublished API to inject the HTML fragment or load it with | ||
the render_xblock view has an Iframe, similar to how the MFE renders | ||
course content). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On what level of the course hierarchy do you plan to call render_xblock? The unit, I assume?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @kdmccormick!, I will try to modify or extend what was proposed in approach 3 but with a more agnostic approach that could work for this feature. Right now I'm not sure about any performance issues, the performance issue of the iframe should be similar to the learning MFE courseware since it's the same method the learning MFE uses to render a unit content.
Our plan is to call render_xblock on the unit level and add a template to render the sequence navigation on top, similar to how it's done on the learning MFE, currently, you can call render_xblock on a sequence, but renders a sequence navigation that has a broken "Previous" button, there is a change that, if this could be fixed on the edx-platform side, we could render the content at a sequence level and leverage the rendering of the sequence navigation to render_xblock, I still haven't found what generates this issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our plan is to call render_xblock on the unit level and add a template to render the sequence navigation on top, similar to how it's done on the learning MFE
Perfect, I think that is the right way to go 👍🏻 The fact that you can render_xblock can be called with a sequence at all is a legacy behavior that probably should have been removed when we fully switched over to the Learning MFE. (here's some extra context on why we don't want to directly render xblocks above the unit level, if you're curious).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It wasn't still clear to me if that sequence navigation was intended or not, since this is considered legacy I will not look further into fixing it. This context also will be useful to understanding the role of an XBlock and how we should work with it when we get to add LTI grading or other services. Thank you for sharing this ADR and giving us feedback on this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem!
Thanks @ormsbee, I agree that using the learning MFE could be a better option to maintain stable courseware concerning future changes in the platform, for now, it seems to us that approach 4 is a simpler way to create an initial implementation for the LTI tool provider that can be developed without having to maintain another repository and allow an easy installation and configuration of the plugin. Is your proposed new course navigation really LTI-specific?It doesn't necessarily have to be specific views for LTI, the only thing we need is some views that allow having a course home and courseware with nothing more than the content, to allow a user to easily navigate on an LTI launch of a complete course while having strict control over exposed content on LTI, we also have thought the future implementation to make a launch on a unit or component, in that case, we would use directly render_xblock, as it is implemented in the app lti_provider in the LMS. Right now course launches are our priority. Also, we need to have control over access to these views, in our case we should only be able to allow access to these views if a user is related to an LTI profile, integrating it into the plugin allows us to manage access easily with a middleware that would only allow access to these views and log out the user in case of accessing another view of the LMS (this is to make sure the client is not blocked in case it wants to log in with another account outside the LTI launch). I think that if we implement something that can be used for other features, this should be done in the learning MFE, it seems to me that maintaining a separate plugin only for these views is less practical and is limited only to our use case right now. Within this ADR discussion this idea came up, that maybe if a similar feature to this could be pushed to the learning MFE, we could use these views instead of keeping a separate one for our use case. I don't know what you think about this, I think it could be a different discussion if there is interest in integrating these views for other features. Please consider what it would mean to make the Learning MFE integrate this cleanly.Before deciding to go for approach 4, we thought that if it were to be implemented in the learning MFE approach 3 could be the most practical, add a new path for these views, and add a validation that allows us to detect if the user should or should not have access, for now, it is not completely clear how this access control could be best achieved, if possible I could extend the approach 3 and polish it to have an alternative proposal using the MFE, this could perhaps work as a reference for future features that may need this functionality in the learning MFE for a similar purpose. Thanks for your feedback! I hope that as with the documentation, my answers to your questions/suggestions are clear enough. |
Ticket
https://agile-jira.pearson.com/browse/PADV-303
Description
This PR adds an ADR document where we will address the issue on the ticket PADV-303,
the document contains information on rending a whole course from and LTI course,
these are a description of the multiple approaches to achieve this, its workflow,
and each pros and cons. We will use this ADR to discuss on which approach use to
render the whole course.
Type of Change
rendering a whole course for an LTI launch.