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

SAK-50820 Assignment / LTI improve archive/merge #13188

Merged
merged 3 commits into from
Jan 18, 2025
Merged

SAK-50820 Assignment / LTI improve archive/merge #13188

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6a7a624
SAK-50820 Support for LTI Assignment typrs archive/merge
csev Jan 14, 2025
9a192c7
WHitespace
csev Jan 17, 2025
8a97533
Review comments - thanks Earle
csev Jan 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 31 additions & 3 deletions assignment/impl/src/java/org/sakaiproject/assignment/impl/AssignmentServiceImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@
import java.util.HashMap;
import java.util.HashSet;
import java.util.Iterator;
import java.util.LinkedHashSet;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
Expand Down Expand Up @@ -146,6 +147,8 @@
import org.sakaiproject.grading.api.CategoryDefinition;
import org.sakaiproject.grading.api.GradebookInformation;
import org.sakaiproject.grading.api.GradingService;
import org.sakaiproject.lti.api.LTIService;
import org.sakaiproject.util.foorm.Foorm;
import org.sakaiproject.messaging.api.Message;
import org.sakaiproject.messaging.api.MessageMedium;
import org.sakaiproject.messaging.api.UserMessagingService;
Expand Down Expand Up @@ -244,6 +247,7 @@ public class AssignmentServiceImpl implements AssignmentService, EntityTransferr
@Setter private UserTimeService userTimeService;
@Setter private TimeSheetService timeSheetService;
@Setter private TagService tagService;
@Setter private LTIService ltiService;

private boolean allowSubmitByInstructor;
private boolean exposeContentReviewErrorsToUI;
Expand Down Expand Up @@ -317,16 +321,25 @@ public String archive(String siteId, Document doc, Stack<Element> stack, String

int assignmentsArchived = 0;
for (Assignment assignment : getAssignmentsForContext(siteId)) {

String xml = assignmentRepository.toXML(assignment);
log.debug(xml);

try {
InputSource in = new InputSource(new StringReader(xml));
Document assignmentDocument = DocumentBuilderFactory.newInstance().newDocumentBuilder().parse(in);
Element assignmentElement = assignmentDocument.getDocumentElement();

if ( assignment.getContentId() != null ) {
Long contentKey = assignment.getContentId().longValue();
Element contentElement = ltiService.archiveContentByKey(assignmentDocument, contentKey, siteId);
if ( contentElement != null ) assignmentElement.appendChild(contentElement);
}

Node assignmentNode = doc.importNode(assignmentElement, true);
element.appendChild(assignmentNode);


// Model answer with optional attachments
AssignmentModelAnswerItem modelAnswer = assignmentSupplementItemService.getModelAnswer(assignment.getId());
if (modelAnswer != null) {
Expand Down Expand Up @@ -435,11 +448,16 @@ public String merge(String siteId, Element root, String archivePath, String from
final Stream<Node> allChildrenNodes = IntStream.range(0, allChildrenNodeList.getLength()).mapToObj(allChildrenNodeList::item);
final List<Element> assignmentElements = allChildrenNodes.filter(node -> node.getNodeType() == Node.ELEMENT_NODE).map(element -> (Element) element).collect(Collectors.toList());

Set<String> assignmentTitles = getAssignmentsForContext(siteId).stream()
.map(Assignment::getTitle) // Extract the title from each Assignment
.collect(Collectors.toCollection(LinkedHashSet::new)); // Collect into a LinkedHashSet

int assignmentsMerged = 0;

for (Element assignmentElement : assignmentElements) {

try {
mergeAssignment(siteId, assignmentElement, results);
mergeAssignment(siteId, assignmentElement, results, assignmentTitles);
assignmentsMerged++;
} catch (Exception e) {
final String error = "could not merge assignment with id: " + assignmentElement.getFirstChild().getFirstChild().getNodeValue();
Expand Down Expand Up @@ -974,7 +992,7 @@ public String getTimeSpent(AssignmentSubmission submission) {
return submission.getSubmitters().stream().findAny().get().getTimeSpent();
}

private Assignment mergeAssignment(final String siteId, final Element element, final StringBuilder results) throws PermissionException {
private Assignment mergeAssignment(final String siteId, final Element element, final StringBuilder results, Set<String> assignmentTitles) throws PermissionException {

if (!allowAddAssignment(siteId)) {
throw new PermissionException(sessionManager.getCurrentSessionUserId(), SECURE_ADD_ASSIGNMENT, AssignmentReferenceReckoner.reckoner().context(siteId).reckon().getReference());
Expand All @@ -988,9 +1006,19 @@ private Assignment mergeAssignment(final String siteId, final Element element, f

// Get an assignment object from the xml
final Assignment assignmentFromXml = assignmentRepository.fromXML(xml);

// Remove duplicates from the import
String assignmentTitle = assignmentFromXml.getTitle();
if ( assignmentTitle == null ) return null;
if ( assignmentTitles.contains(assignmentTitle) ) return null;
csev marked this conversation as resolved.
Show resolved Hide resolved

if (assignmentFromXml != null) {
assignmentFromXml.setId(null);
assignmentFromXml.setContext(siteId);
assignmentFromXml.setContentId(null);

Long contentKey = ltiService.mergeContentFromImport(element, siteId);
if ( contentKey != null ) assignmentFromXml.setContentId(contentKey.intValue());

if (serverConfigurationService.getBoolean(SAK_PROP_ASSIGNMENT_IMPORT_SUBMISSIONS, false)) {
Set<AssignmentSubmission> submissions = assignmentFromXml.getSubmissions();
Expand Down Expand Up @@ -4257,7 +4285,7 @@ public Map<String, String> transferCopyEntities(String fromContext, String toCon
// If there is a LTI launch associated with this copy it over
if ( oAssignment.getContentId() != null ) {
Long contentKey = oAssignment.getContentId().longValue();
Object retval = SakaiLTIUtil.copyLTIContent(contentKey, toContext, fromContext);
Object retval = ltiService.copyLTIContent(contentKey, toContext, fromContext);
if ( retval instanceof Long ) {
nAssignment.setContentId(((Long) retval).intValue());
// If something went wrong, we can't be an LTI submission in the new site
Expand Down
6 changes: 6 additions & 0 deletions assignment/impl/src/test/org/sakaiproject/assignment/impl/AssignmentTestConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@
import org.sakaiproject.event.api.LearningResourceStoreService;
import org.sakaiproject.grading.api.GradingService;
import org.sakaiproject.hibernate.AssignableUUIDGenerator;
import org.sakaiproject.lti.api.LTIService;
import org.sakaiproject.messaging.api.UserMessagingService;
import org.sakaiproject.rubrics.api.RubricsService;
import org.sakaiproject.search.api.SearchIndexBuilder;
Expand Down Expand Up @@ -335,4 +336,9 @@ public TimeSheetService timeSheetService() {
public TagService tagService() {
return mock(TagService.class);
}

@Bean(name = "org.sakaiproject.lti.api.LTIService")
public LTIService ltiService() {
return mock(LTIService.class);
}
}
1 change: 1 addition & 0 deletions assignment/impl/src/webapp/WEB-INF/components.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@
<property name="userTimeService" ref="org.sakaiproject.time.api.UserTimeService"/>
<property name="timeSheetService" ref="org.sakaiproject.timesheet.api.TimeSheetService"/>
<property name="tagService" ref="org.sakaiproject.tags.api.TagService"/>
<property name="ltiService" ref="org.sakaiproject.lti.api.LTIService"/>
</bean>

<bean id="org.sakaiproject.assignment.impl.AssignmentContentProducer"
Expand Down
2 changes: 1 addition & 1 deletion lessonbuilder/tool/src/java/org/sakaiproject/lessonbuildertool/service/BltiEntity.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -438,7 +438,7 @@ public String doImportTool(String launchUrl, String bltiTitle, String strXml, St

// Lets find the right tool to assiociate with
List<Map<String,Object>> tools = ltiService.getTools(null,null,0,0, simplePageBean.getCurrentSiteId());
Map<String, Object> theTool = SakaiLTIUtil.findBestToolMatch(launchUrl, tools);
Map<String, Object> theTool = SakaiLTIUtil.findBestToolMatch(launchUrl, null, tools);

if ( theTool == null ) {
log.debug("Inserting new tool title={} url={}",bltiTitle, launchUrl);
Expand Down
2 changes: 1 addition & 1 deletion ...tool/src/java/org/sakaiproject/lessonbuildertool/service/LessonBuilderEntityProducer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2287,7 +2287,7 @@ private String transferAttachmentFiles(String msgBody, String oldSiteId, String

private String copyLTIContent(Map<String, Object> ltiContent, String siteId, String oldSiteId)
{
Object result = SakaiLTIUtil.copyLTIContent(ltiContent, siteId, oldSiteId);
Object result = ltiService.copyLTIContent(ltiContent, siteId, oldSiteId);
String sakaiId = null;
if ( result == null ) {
return null;
Expand Down
154 changes: 74 additions & 80 deletions lti/docs/IMPORTCC.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,8 @@ Cartridges are a hierarchical "table of contents". Often they are something lik
LTI Links
...

When Lessons sees these top level "folders" it imports them as Sub-Pages and promotes them
to the left navigation. If you want the major chunks of content rendered on the
When Lessons sees these top level "folders" it imports them as Sub-Pages and promotes them
to the left navigation. If you want the major chunks of content rendered on the
left navigation - this is great.

When Tsugi knows it is exporting to Sakai, it actually adds one extra folder level:
Expand All @@ -50,80 +50,13 @@ When Tsugi knows it is exporting to Sakai, it actually adds one extra folder lev
So you only get one left nav link which you can easily hide. All the secondary folders
are still subpages so you can put them anywhere you like (i.e Lessons pages or left nav).

LTI Links While Importing a Cartridge (Old School)
--------------------------------------------------

When you have links in the carridge, they have launch urls but *not* keys and secrets.
So a big part of importing LTI links is associating them with an existing LTI Tool that either
has a key and secret *or* inserting a new tool in the site without a key and secret so that
you can edit the tool and add the key and secret later.

The link matching in the (pre 20.3) versions of Sakai is not as sophisticated as it should be.
It looks for a tool that matches the launch URL exactly with or without the query string
and if it finds a matching tool (globally or within the site) it connects to that existing tool.

This was a sufficient feature before the wide usage of Content Item and Deep Linking where the
URLs all looked the same:

https://mymathlab.co/calculus/lti/?section=1.0
https://mymathlab.co/calculus/lti/?assignment=1.1
https://mymathlab.co/calculus/lti/?section=2.0

All these would map to the same tool. But when you got a cartridge from a site like
www.py4e.com - it had a lot of links that were quite different.

https://www.py4e.com/tools/pythonauto/?assn=1.1
https://www.py4e.com/mod/quiz/?quiz=1.1
https://www.py4e.com/mod/peer-grade/?assn=welcome
https://www.py4e.com/tools/pythonauto/?assn=2.1
https://www.py4e.com/mod/quiz/?quiz=2.0
https://www.py4e.com/mod/peer-grade/?assn=loops

This would be looking for three registered tools of the form:

https://www.py4e.com/tools/pythonauto/
https://www.py4e.com/mod/quiz/
https://www.py4e.com/mod/peer-grade/

At least it ignores the query string and coalesces these into three tools instead of six
or 120 tools that each have to be configured with a key and secret.

If Lessons did not find a tool - it created a tool with an empty key and secret for each of the
query-less URL paths in the cartridge. Lessons even had a nice feature that noticed when there
was no secret and on the first launch it let you set the key and secret for one of the URLs
and all the rest of the links using that URL would work fine. So in this example, you needed
to put the key and secret in three places and any number of LTI links would start working.

If you knew what you were doing in advance and installed those three tools with a key
and secret *before* the impoprt - the links would be properly associated and work instantly
after they were imported. But sadly this did not happen often because it was not altogether
clear which URL should be registered and the fact that more than one registration might be required.

But it worked well enough to get by.

Importing LTI Links on the latest Sakai (> 20.3)
------------------------------------------------

The latest Sakai improves the user experience by addressing two use cases:

* Being able to register one global tool before import and have all links find that tool -
"Improve LTI Tool Matching During Common Cartridge Import" - https://jira.sakaiproject.org/browse/SAK-44763

* Being able to register a tool after an import and transfer the imported links from the "stub tools"
created in the site to a tool that was added after the import and then deleting the "stub tools" and
having everything work (and no icky stub tools). "Add ability to "patch" LTI tool placements after a CC
import with broken links" - https://jira.sakaiproject.org/browse/SAK-44772

You can check to see if these changes are in your version of Sakai. At worst you can play with them
on our nightly servers until you upgrade to the latest verison.

Improving Tool Matching (SAK-44763)
-----------------------------------
LTI Tool Matching
-----------------

As Lessons looks at each LTI link being imported, it does the following matching:

1. Look for a locally registered tool with an exact match
2. Next snip off the query string and check for a local tool
1. Look for a registered tool in the site with an exact match
2. Next snip off the query string and check for a tool in the site
3. Next look for a local tool that has a prefix match that at least includes protocol and host

Then we go through the globally registered tools following the same priority:
Expand All @@ -132,11 +65,12 @@ Then we go through the globally registered tools following the same priority:
5. Next snip off the query string and check for a global tool
6. Next look for a global tool that has a prefix match that at least includes protocol and host

After all that if we can't find a matching tool, we punt and make a new tool without a secret (old behavior).
After all that if we can't find a matching tool, we punt and make a new tool without a secret.

It checks local before global to an override - but in general - the most common case is that there will be
no per-site (i.e. local) tools installed in the site so they will just find globally installed tools.
And if you disallow per-site tool creation by the instructor, then the fact that per-site tools are checked
It checks "in site" before global to an override - but in general - the most common case is
that there will be no per-site (i.e. local) tools installed in the site so they will
just find globally installed tools. And if you disallow per-site tool creation by
the instructor, then the fact that per-site tools are checked
first does not matter.

This means that if we were importing a cartridge with the following links:
Expand All @@ -158,8 +92,8 @@ be created) and all the cartridge links would instantly work.
The matching rule (above) in this case would be the last matching rule - "prefix match that at least
includes protocol and host". But it would match and connect up.

Patching Tool Links Post-Import (SAK-44772)
-------------------------------------------
Patching Tool Links Post-Import
-------------------------------

But if you imported a cartridge with the six links *before* you added the global tool Lessons would not
find any registered tool and so you would end up with three "stub tools" created locally in the site:
Expand All @@ -174,9 +108,69 @@ could also install a new global tool at:
https://www.py4e.com/tsugi/lti/store/

And then find the "stub tools" and transfer all the links from the stub tools to the global tool
and then delete the stub tools. And all the links will work.
and then delete the stub tools.

And since there is now a global tool that has a nice matchable url - future imports from www.py4e.com will
simply auto-associate and work as soon as they are imported.

Importing a CC+Sakai Cartridge
------------------------------

For some versions of Sakai there is an option to do a CC Export and include a Sakai Archive
in the cartridge in the Cartridge. These proprietary Sakai files are ignored by other
LMSs doing cartridge imports because they are marked as "ignore this file".

However, if Sakai is importing this augmented cartridge, it notices these files and switches
to an un-archive instead of a CC import. This leads to a much higher fidelity import.

When LTI links are exported into a CC+Sakai cartridge, there is more information included.
For example, if there is an Assignment with an LTI tool, the following is added to the
XML data:

<sakai-lti-content>
<id>83</id>
<title>Breakout 1.1 window 3</title>
<description>&lt;p&gt;ew&lt;/p&gt;</description>
<newpage>1</newpage>
<custom>submissionend=$ResourceLink.submission.endDateTime
availablestart=$ResourceLink.available.startDateTime
canvas_caliper_url=$Caliper.url
availableend=$ResourceLink.available.endDateTime
submissionstart=$ResourceLink.submission.startDateTime
resourcelink_id_history=$ResourceLink.id.history
context_id_history=$Context.id.history
coursegroup_id=$CourseGroup.id
</custom>
<launch>http://localhost:8888/py4e/mod/breakout/</launch>
<sakai-lti-tool>
<id>1</id>
<title>Py4E 1.1 Store</title>
<launch>http://localhost:8888/py4e/tsugi/lti/store/</launch>
<newpage>2</newpage>
<pl_linkselection>1</pl_linkselection>
<pl_lessonsselection>1</pl_lessonsselection>
<pl_contenteditor>1</pl_contenteditor>
<pl_assessmentselection>1</pl_assessmentselection>
<sendname>1</sendname>
<sendemailaddr>1</sendemailaddr>
<allowoutcomes>1</allowoutcomes>
<allowlineitems>1</allowlineitems>
<lti13>0</lti13>
<sakai_tool_checksum>Q3bPh/gLibW0GYXxSoj8Lub351q4XNfLN6BZVQXFkn4=</sakai_tool_checksum>
</sakai-lti-tool>
</sakai-lti-content>

The export adds a `sakai_tool_checksum` which marks a particular tool installed in a particular
instance of Sakai. If the CC+Sakai is imported into the *same* LMS and the tool is still in
the LMS and available to the site where the import is happening, and *exact* match looks up the
correct tool using this checksum.

If there is not tool that matches the checksum associated with the site, the CC+Sakai import
process falls back to the laucn url matching approach described above.


When a content item is being created the same kind of matching based on launch url is described above.




Loading
Loading