-
-
Notifications
You must be signed in to change notification settings - Fork 928
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
SAK-49826 Lessons LTI Learning App does not follow new page rules #12810
Conversation
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.
@csev - In short, using the alternate test plan in the jira, I'm not seeing a difference in results between master with this patch and master without this patch. I've uploaded to the jira a new screencast (SAK-49826-TestResults-20240821.mp4 - 4 min.) that elaborates what I observe and alludes to some rationales behind my version of a fix (master...hornersa:sakai:SAK-49826) that resolves the otherwise unexpected outcomes cited in the alternate test plan.
@hornersa, I marked this as draft because it is incomplete (see the description). I have about 1/2 of the functionality here - the majority of the code at this point is the back end code finding the data that will inform the UI. |
So @hornersa this is now ready for you to test. |
Sorry for the earlier false start. (I interpreted the github signal 'Request for review' as 'ready for review'.) With the current code, I'm observing a presumed intended difference when the lti_tool.newpage is set to 'Always launch in popup' and 'Never launch in popup', where this value has a determining affect on the appearance (or hiding) of the Display Options section of the Edit Item dialog for a corresponding link on a Lessons page. That said, the following case still fails: the lti_tools.newpage value is set to "Allow popup to be changed" and the instructor adds a new link with "Launch in popup" checked. The outcome is that clicking the link opens the tool within the ShowItem portal instead of launching the tool in a new tab. Furthermore, the 'Dispaly Options' value has "Open in new window", which would be desired in this case though contradicted by the undesired behavior upon clicking the link. Is your intent that the "Launch in popup" checkbox should not display at all for a Lessons placement when lti_tools.newpage value is set to "Allow popup to be changed"-- such that all control is designated to the Lesson > Edit Item > Display Options settings and that the following is always selected by default: "Open in New Lessons tool Page with 'next' and 'back' buttons"? |
@hornersa You are correct that when the tool has "Allow popup to be changed" it is supposed to delegate the choice to the lessons item - and the user should then see the lessons checkbox. And then the code should react to that checkbox. It that does not work it still needs to be fixed. If the tool is marked as "Always launch in popup" in the tool - the instructor should never see the launch target options in the lessons item. If the tool is marked as "Always launch in the iframe", the instructor should see and be able to choose between "inline" and "new page" - but not select new window. Yes this is complex. I tested all those combinations - but perhaps I missed something. Once I have gotten caught up on my other two LTI PR's I will come back and retest this to make sure we get it right. Thanks for your patience. The good news is that I think we agree on how it should work. |
@csev - Understood. Thanks. |
I am going to merge this so it is part of the testing of the great LTI rewrite. |
I reformatted the BltiEntity file and cleaned up old stuff.
This in progress for now. It even has some debug stuff that will be removed.