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.

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

kuipumu
Copy link
Contributor

@kuipumu kuipumu commented Feb 9, 2023

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

  • Add ADR /docs/decisions/0001-launch-complete-course.rst with discovery on
    rendering a whole course for an LTI launch.

docs/decisions/0001-launch-complete-course.rst Outdated Show resolved Hide resolved
docs/decisions/0001-launch-complete-course.rst Outdated Show resolved Hide resolved
docs/decisions/0001-launch-complete-course.rst Outdated Show resolved Hide resolved
docs/decisions/0001-launch-complete-course.rst Outdated Show resolved Hide resolved
docs/decisions/0001-launch-complete-course.rst Outdated Show resolved Hide resolved
docs/decisions/0001-launch-complete-course.rst Outdated Show resolved Hide resolved
docs/decisions/0001-launch-complete-course.rst Outdated Show resolved Hide resolved
docs/decisions/0001-launch-complete-course.rst Outdated Show resolved Hide resolved
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
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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

Copy link
Contributor Author

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
Copy link
Contributor

@alexjmpb alexjmpb Feb 13, 2023

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?

Copy link
Contributor Author

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

Copy link
Contributor

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
Copy link
Member

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?

alexjmpb
alexjmpb previously approved these changes Feb 15, 2023
docs/decisions/0001-launch-complete-course.rst Outdated Show resolved Hide resolved
docs/decisions/0001-launch-complete-course.rst Outdated Show resolved Hide resolved
docs/decisions/0001-launch-complete-course.rst Outdated Show resolved Hide resolved
anfbermudezme
anfbermudezme previously approved these changes Feb 15, 2023
Co-authored-by: Javier Torres <40271196+Jacatove@users.noreply.github.com>
Copy link

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

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

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.

Copy link
Contributor Author

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.

Copy link

@kdmccormick kdmccormick left a 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.

Comment on lines +263 to +268
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).

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?

Copy link
Contributor Author

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.

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

Copy link
Contributor Author

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!

Choose a reason for hiding this comment

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

No problem!

@kuipumu
Copy link
Contributor Author

kuipumu commented Mar 3, 2023

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.

@kuipumu kuipumu dismissed stale reviews from anfbermudezme and alexjmpb via 80a2738 January 11, 2024 22:38
@kuipumu kuipumu self-assigned this May 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants