SAK-52133 Discussions No email notifications are sent if an attachment is added - Fix

[st-manu]

https://sakaiproject.atlassian.net/browse/SAK-52133

Summary by CodeRabbit

• Bug Fixes

  • More reliable cleanup of temporary attachments via a scheduled background cleanup to prevent leftover files.
  • Improved handling when deleting files or directories: failures now fall back to safe deletion-on-exit and emit clearer diagnostics.

• Chores

  • Enhanced, more verbose logging around attachment save and deletion for better troubleshooting.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

Replaces immediate attachment cleanup with a scheduled TimerTask (10s delay) to remove files and directories; updates deletion methods to log outcomes and use deleteOnExit() when immediate removal fails; adds Timer/TimerTask imports and debug logging.

Changes

Cohort / File(s)	Summary
ForumsEmailService changes msgcntr/messageforums-app/src/java/org/sakaiproject/tool/messageforums/ForumsEmailService.java	Added Timer/TimerTask imports; replaced inline cleanup with a scheduled TimerTask (10s) to delete attachments; updated deleteAttachedFile and directory deletion logic to log success/failure, call deleteOnExit() on failure, and emit debug logs for outcomes.

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title directly references a specific JIRA issue (SAK-52133) and clearly describes the fix: resolving the problem where email notifications are not sent when attachments are added to Discussions.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

---

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

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In
`@msgcntr/messageforums-app/src/java/org/sakaiproject/tool/messageforums/ForumsEmailService.java`:
- Around line 272-290: The code risks StringIndexOutOfBounds when extracting the
directory and misuses deleteOnExit for directories; replace manual substring
logic with File file = new File(buildAttachmentPath(resourceId)); File dir =
file.getParentFile(); if dir is null decide a safe default or throw an
IOException, then create dir via dir.exists()/dir.mkdirs() and only call
file.deleteOnExit() (remove dir.deleteOnExit()) because directories may not be
removed reliably at JVM shutdown — ensure you reference
buildAttachmentPath(...), the local variables filename/file/dir, and the
createNewFile() logic when applying these changes.

🧹 Nitpick comments (1)

msgcntr/messageforums-app/src/java/org/sakaiproject/tool/messageforums/ForumsEmailService.java (1)

251-264: Platform-specific /tmp check and potential filename collisions.

Two concerns with this helper:

1. Line 253: The /tmp check is Unix-specific. On Windows, temp paths like C:\Users\...\Temp won't match, so the fallback to System.getProperty("java.io.tmpdir") won't trigger when it should.

2. Line 263: Stripping all spaces from the path could cause filename collisions when two attachments differ only by spaces (e.g., "my file.pdf" and "myfile.pdf" would map to the same path).

Consider a more robust approach:

Suggested improvement

 private String buildAttachmentPath(String resourceId) {
     String basePath = prefixedPath;
-    if (basePath.startsWith("/tmp")) {
+    String systemTmp = System.getProperty("java.io.tmpdir");
+    if (basePath.equals("/tmp") || basePath.equals(systemTmp)) {
         basePath = System.getProperty("java.io.tmpdir");
     }

     StringBuilder path = new StringBuilder(basePath);
     if (!basePath.endsWith(File.separator)) {
         path.append(File.separator);
     }
     path.append("email_tmp").append(File.separator);
     path.append(resourceId.replace("\\", File.separator).replace("/", File.separator));
-    return path.toString().replace(" ", "");
+    return path.toString().trim();
 }

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 27007d3 and 0b01e57.

📒 Files selected for processing (1)

• msgcntr/messageforums-app/src/java/org/sakaiproject/tool/messageforums/ForumsEmailService.java

🧰 Additional context used

📓 Path-based instructions (1)

**/*.java

📄 CodeRabbit inference engine (.cursor/rules/logging-rule.mdc)

**/*.java: Use SLF4J parameterized logging (logger.info("Value is: {}", value)) instead of string concatenation (logger.info("Value is: " + value))
Log messages and code comments should be in English. Log messages should never be translated.

**/*.java: Java: Never use local variable type inference (var). Always declare explicit types. Yes: Map<String, Integer> counts = new HashMap<>(); No: var counts = new HashMap<String, Integer>();
When proposing Java code, spell out full types in local variable declarations, for loops, and try-with-resources
When editing Java, prefer clarity over brevity; avoid introducing language features that aren't widely used in the repo
Treat any PR or suggestion containing Java var as non-compliant. Recommend replacing with explicit types before merge

**/*.java: Use Java 17 for trunk development (Java 11 was used for Sakai 22 and Sakai 23)
Do not use local variable type inference (var) in Java code. Always declare explicit types (e.g., List<String> names = new ArrayList<>(); not var names = new ArrayList<String>();). Enforced by Checkstyle rule during mvn validate

Files:

• msgcntr/messageforums-app/src/java/org/sakaiproject/tool/messageforums/ForumsEmailService.java

🧠 Learnings (1)

📓 Common learnings

Learnt from: ottenhoff
Repo: sakaiproject/sakai PR: 0
File: :0-0
Timestamp: 2025-10-07T15:11:27.298Z
Learning: In samigo’s Total Scores view (samigo/samigo-app/src/webapp/jsf/evaluation/totalScores.jsp), mailto links were hidden after commit dee05746 (PR `#12312`, SAK-49674) added a render check requiring email.fromEmailAddress to be non-empty; PR `#14154` (SAK-52058) restores visibility by checking only description.email.

Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T16:11:33.008Z
Learning: Commit messages should follow: <issue key> <component> <brief description> (e.g., SAK-12345 Assignments add option x)

⏰ 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

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.}