Skip to content

SAK-52126 LTI improve transfer screen - #14778

Open
ottenhoff wants to merge 1 commit into
sakaiproject:masterfrom
ottenhoff:SAK-52126
Open

SAK-52126 LTI improve transfer screen#14778
ottenhoff wants to merge 1 commit into
sakaiproject:masterfrom
ottenhoff:SAK-52126

Conversation

@ottenhoff

@ottenhoff ottenhoff commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Summary by CodeRabbit

  • New Features

    • Added guidance explaining that tool transfers update existing links without changing their titles.
    • Added clearer timeout messaging when launch requests cannot reach a remote URL.
    • Added validation messaging for transfers to system-wide tools.
  • Bug Fixes

    • Restricted transfer destinations to system-wide tools.
    • Improved post-transfer deployment for stealth tools when appropriate.

@coderabbitai

coderabbitai Bot commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 785ed7de-e517-481c-bc81-fd3dad1179c0

📥 Commits

Reviewing files that changed from the base of the PR and between 6f98754 and fa7e3d9.

📒 Files selected for processing (6)
  • lti/lti-impl/src/bundle/ltiservice.properties
  • lti/lti-impl/src/java/org/sakaiproject/lti/impl/BaseLTIService.java
  • lti/lti-impl/src/java/org/sakaiproject/lti/impl/DBLTIService.java
  • lti/lti-tool/src/bundle/ltitool.properties
  • lti/lti-tool/src/java/org/sakaiproject/lti/tool/LTIAdminTool.java
  • lti/lti-tool/src/webapp/vm/lti_tool_transfer.vm

Walkthrough

LTI tool transfers now reject site-scoped destinations, pass administrator context through transfer handling, and deploy stealth destination tools after successful transfers. The admin UI filters destination choices and displays new localized transfer guidance and timeout messaging.

Changes

LTI transfer service

Layer / File(s) Summary
Transfer validation and stealth deployment
lti/lti-impl/src/java/org/sakaiproject/lti/impl/BaseLTIService.java, lti/lti-impl/src/java/org/sakaiproject/lti/impl/DBLTIService.java, lti/lti-impl/src/bundle/ltiservice.properties
Transfer paths reject site-scoped destination tools, propagate administrator context, and deploy stealth destinations after successful transfers. The new validation message is localized.
Transfer administration UI
lti/lti-tool/src/java/org/sakaiproject/lti/tool/LTIAdminTool.java, lti/lti-tool/src/webapp/vm/lti_tool_transfer.vm, lti/lti-tool/src/bundle/ltitool.properties
Administrator destination options exclude site-scoped tools, and the transfer page displays explanatory text. A remote submission timeout message is added.

Possibly related PRs

  • sakaiproject/sakai#14177: Both changes trigger stealth tool deployment in BaseLTIService, with this PR covering tool-link transfers.

Suggested reviewers: csev

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title is concise and accurately reflects the main change to the LTI transfer screen.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.44.1)
lti/lti-tool/src/java/org/sakaiproject/lti/tool/LTIAdminTool.java

ast-grep timed out on this file


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@adrianfish

Copy link
Copy Markdown
Contributor

@ottenhoff Can we sit on this until #14625 is in? I've replaced the use of sqlService in that PR and spent some significant time, even with Claude's help, doing so :)

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