-
-
Notifications
You must be signed in to change notification settings - Fork 984
SAK-51078 LTI13 Fix UpdateLineItem API call losing resourceLinkId #13625
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
Conversation
lti/lti-common/src/java/org/sakaiproject/lti13/LineItemUtil.java
Outdated
Show resolved
Hide resolved
lti/lti-common/src/java/org/sakaiproject/lti13/LineItemUtil.java
Outdated
Show resolved
Hide resolved
lti/lti-common/src/java/org/sakaiproject/lti13/LineItemUtil.java
Outdated
Show resolved
Hide resolved
@@ -2650,6 +2655,7 @@ public static Object handleGradebookLTI13(Site site, Long tool_id, Map<String, | |||
String title; | |||
|
|||
log.debug("siteid: {} tool_id: {} lineitem_key: {} userId: {} scoreObj: {}", site.getId(), tool_id, lineitem_key, userId, scoreObj); | |||
System.out.println("content="+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.
Is this println intended? If so, I recommend folding the content var into the log.debug line above.
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've manually backported this PR to my use case in Sakai 23 and it fixed that bug as is. Since attaching the 23.x patch to the jira a while ago, I've modified a few lines to leverage StringUtil method and made another change to further NPE-proof deriveContentIdFromGradebookExternalId. The suggested changes mostly reflect this.
Finally, I'm not sure if my case exercises the changes introduced with SakaiLTIUtil (or SakaiBLTIUtil in 23.x). Otherwise, this looks good to go to me. I'll await your response before committing to "Approve" for my review.
@@ -120,13 +120,16 @@ public static String constructExternalId(Map<String, Object> content, SakaiLineI | |||
*/ | |||
// TODO: Remember to dream that someday this will be JSON :) | |||
public static String constructExternalId(Long tool_id, Map<String, Object> content, SakaiLineItem lineItem) | |||
{ | |||
Long content_id = (content == null) ? null : ((Integer) content.get(LTIService.LTI_ID)).longValue(); |
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 think this is not going to work on Oracle...
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.
@jesusmmp Could you expand on what would/might break for Oracle?
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.
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.
Ok, thanks. I'll try the cited alternative in testing. If I recall correctly, I originally set the cast to Long but encountered a runtime exception when testing against mariadb.
lti/lti-common/src/java/org/sakaiproject/lti13/LineItemUtil.java
Outdated
Show resolved
Hide resolved
@csev - Once you add the following line to the LineItemUtil.java, I think we should be good to go: |
@@ -120,13 +121,16 @@ public static String constructExternalId(Map<String, Object> content, SakaiLineI | |||
*/ | |||
// TODO: Remember to dream that someday this will be JSON :) | |||
public static String constructExternalId(Long tool_id, Map<String, Object> content, SakaiLineItem lineItem) | |||
{ | |||
Long content_id = (content == null) ? null : LTIUtil.toLongNull(content.get(LTIService.LTI_ID)); |
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 where toLongNull is defined as I can't locate it in LTIUtil.java. Earlier this line used LTI13Util.getLongNull which had compelled the import of that class on line 58.
@hornersa I think we should merge this into master, back port it to 23.x and then if something else comes up do it in a separate JIRA. The round trip for testing just takes too long given that a patch is need to |
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 observe now that the toLongNull method(s) seem to have been introduced with some recent commit. I'll bypass testing these new changes since they don't yet have analogs in 23.x (the only platform where I currently have the infrastructure to test LTI stuff outside our institution's firewall).
Co-authored-by: hornersa <[email protected]>
Co-authored-by: hornersa <[email protected]>
Co-authored-by: hornersa <[email protected]>
Co-authored-by: hornersa <[email protected]>
…3625) Co-authored-by: hornersa <[email protected]> (cherry picked from commit 28a66da)
…3625) Co-authored-by: hornersa <[email protected]> (cherry picked from commit 28a66da) Conflicts: basiclti/basiclti-common/src/java/org/sakaiproject/basiclti/util/SakaiBLTIUtil.java basiclti/basiclti-common/src/java/org/sakaiproject/lti13/LineItemUtil.java
No description provided.