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

SAK-49826 Lessons LTI Learning App does not follow new page rules #12810

Merged
merged 1 commit into from
Sep 7, 2024

Conversation

csev
Copy link
Contributor

@csev csev commented Aug 21, 2024

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.

@csev csev requested a review from hornersa August 21, 2024 00:32
@csev csev marked this pull request as draft August 21, 2024 00:32
Copy link
Contributor

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

@csev
Copy link
Contributor Author

csev commented Aug 22, 2024

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

@csev
Copy link
Contributor Author

csev commented Aug 22, 2024

So @hornersa this is now ready for you to test.

@csev csev marked this pull request as ready for review August 22, 2024 03:56
@hornersa
Copy link
Contributor

hornersa commented Aug 22, 2024

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"?

@ern ern changed the title SAK-49826 Lessons-LTI Learning App Does not follow new page rules SAK-49826 Lessons LTI Learning App does not follow new page rules Aug 26, 2024
@csev
Copy link
Contributor Author

csev commented Sep 5, 2024

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

@hornersa
Copy link
Contributor

hornersa commented Sep 5, 2024

@csev - Understood. Thanks.

@csev csev merged commit a48a136 into sakaiproject:master Sep 7, 2024
5 checks passed
@csev
Copy link
Contributor Author

csev commented Sep 7, 2024

I am going to merge this so it is part of the testing of the great LTI rewrite.

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.

2 participants