SAK-52083 LTI Stealthed tool deployment removal enhancement #14178
Conversation
WalkthroughThis pull request adds stranded LTI tools handling to the site management tool. The changes detect LTI tools that are deployed in a site but no longer available, display them in the UI with a warning message, and remove them during site save operations. Changes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.39.6)site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/SiteAction.javaThanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/SiteAction.java (2)
4242-4253: Incorrect ‘selected’ flag logic for LTI tools (uses siteId instead of toolId).linkedLtiContents is keyed by LTI tool id, but the code checks containment with ltiSiteId, which will always be wrong and can mislabel selection state in the UI.
Fix by checking ltiToolId:
- String ltiSiteId = StringUtils.trimToNull((String) toolMap.get(LTIService.LTI_SITE_ID)); - toolMap.put("selected", linkedLtiContents.containsKey(ltiSiteId)); + String ltiSiteId = StringUtils.trimToNull((String) toolMap.get(LTIService.LTI_SITE_ID)); + toolMap.put("selected", linkedLtiContents.containsKey(ltiToolId));
12220-12228: Possible NPE when adding new LTI tools (oldLtiTools may be null).oldLtiTools can be null; calling containsKey() will throw NPE.
Guard it:
- if (!oldLtiTools.containsKey(ltiToolId)) + if (oldLtiTools == null || !oldLtiTools.containsKey(ltiToolId))
🧹 Nitpick comments (6)
site-manage/site-manage-tool/tool/src/webapp/vm/sitesetup/toolGroupMultipleDisplay.vm (1)
10-10: Consider moving inline styles to CSS classes.The inline styles (
style="margin: 15px 0;"andstyle="margin-right: 8px;") work but using CSS classes would improve maintainability and consistency with the rest of the codebase.Also applies to: 12-12
site-manage/site-manage-tool/tool/src/bundle/sitesetupgeneric.properties (1)
166-166: Consider enhancing the user message for better clarity.The current message states that tools "are no longer available" but doesn't explain why. Consider adding context such as "These LTI tools were removed from the system configuration" or similar to help users understand the root cause. This would provide better user experience, though the current message is functional.
site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/SiteAction.java (4)
1303-1313: State hygiene: clear stranded list in cleanState().STATE_LTITOOL_STRANDED_LIST is not removed in cleanState(), which can leak stale data across flows.
Add removal:
// lti tools state.removeAttribute(STATE_LTITOOL_EXISTING_SELECTED_LIST); state.removeAttribute(STATE_LTITOOL_SELECTED_LIST); + state.removeAttribute(STATE_LTITOOL_STRANDED_LIST);
4195-4203: Good: early stranded-tool detection with configurable CourseNav filter.The requireCourseNavPlacement toggle mirrors getLtiToolGroup() behavior and keeps UI/state in sync. Consider documenting the property key in sitesetupgeneric.properties comments for admins.
6845-6915: Minor polish in getStrandedLTITools().
- Remove unused variable ‘visible’.
- Consider using the consistent key name "id" (tool.get("id")) if available across DB vendors, or keep LTIService.LTI_ID but add a brief comment why both exist.
No behavior change required.
12319-12324: Optional: avoid redundant site save.saveFeatures() calls commitSite(site) before and after the stranded cleanup and later again after imports and tool settings. You can defer the first commit until after cleanup to reduce persistence churn.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
site-manage/site-manage-tool/tool/src/bundle/sitesetupgeneric.properties(1 hunks)site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/SiteAction.java(8 hunks)site-manage/site-manage-tool/tool/src/webapp/vm/sitesetup/toolGroupMultipleDisplay.vm(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.java
📄 CodeRabbit inference engine (.cursor/rules/logging-rule.mdc)
**/*.java: Use SLF4J parameterized logging (e.g., logger.info("Value is: {}", value)) instead of string concatenation in log statements
Write log messages and code comments in English; never translate log messages
**/*.java: Java: Never use local variable type inference (var). Always declare explicit types.
When proposing or writing Java code, spell out full types in local variable declarations, for-loops, and try-with-resources.
If Java code uses var, replace it with an explicit type.
Review gate: Treat any PR or suggestion containing Java var as non-compliant; require explicit types before merge.Do not use local variable type inference (var) in Java; always declare explicit types
Files:
site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/SiteAction.java
🧠 Learnings (1)
📓 Common learnings
Learnt from: csev
PR: sakaiproject/sakai#14094
File: lti/lti-api/src/java/org/sakaiproject/lti/beans/LtiContentBean.java:1-19
Timestamp: 2025-09-27T15:41:26.287Z
Learning: csev prefers to use Apache 2.0 license headers for new code he writes, even when the project standard is ECL 2.0, as the licenses are compatible.
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: sakai-deploy
- GitHub Check: maven-build
- GitHub Check: maven-build
🔇 Additional comments (3)
site-manage/site-manage-tool/tool/src/webapp/vm/sitesetup/toolGroupMultipleDisplay.vm (1)
8-21: LGTM - Stranded LTI tools warning implemented correctly.The stranded tools alert is well-implemented with proper security measures:
- User content is properly escaped using
$formattedText.escapeHtml()for both title and description- Null safety checks are in place before accessing properties
- Localization is properly handled via
$tlang.getString()- Accessibility is addressed with
role="alert"and semantic HTMLsite-manage/site-manage-tool/tool/src/bundle/sitesetupgeneric.properties (1)
165-166: LGTM - Localization entries are clear and follow conventions.The new localization keys follow the existing naming pattern and provide clear messaging about stranded LTI tools. The message appropriately informs users that the tools will be automatically removed when the site is saved.
site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/SiteAction.java (1)
12319-12419: Stranded LTI cleanup: verify DAO filter safety and consider API-level filtering.Building a raw searchClause string for getContentsDao("lti_content.tool_id = ...") risks tight coupling to DAO internals and potential SQL fragility. Prefer an API method that accepts a toolId filter param (if available) or ensure the DAO safely parameterizes the clause. Also consider paging if >5000 items possible.
Can you confirm getContentsDao(searchClause, …) is parameterized/safe and whether an explicit toolId/siteId filter API exists to avoid raw clauses?
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
3 participants
Summary by CodeRabbit