SAK-52133 Discussions No email notifications are sent if an attachment is added #14220
Conversation
WalkthroughModified path construction for temporary email attachments in ForumsEmailService, changing from slash-formatted prefix ("/email_tmp/") to plain "email_tmp". Affects attachment file location and deletion logic. ResourceId handling updated to normalize separators. Changes
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. 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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
msgcntr/messageforums-app/src/java/org/sakaiproject/tool/messageforums/ForumsEmailService.java(2 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.
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: maven-build
- GitHub Check: sakai-deploy
- GitHub Check: maven-build
🔇 Additional comments (1)
msgcntr/messageforums-app/src/java/org/sakaiproject/tool/messageforums/ForumsEmailService.java (1)
273-274: Backslash normalization improves cross-platform compatibility.The
.replace('\\', '/')addition properly normalizes Windows-style path separators in resourceId, ensuring consistent forward-slash paths across platforms.
| sbPrefixedPath.append("email_tmp"); | ||
| a = (Attachment) iter.next(); | ||
| deleteAttachedFile(sbPrefixedPath.append( | ||
| a.getAttachmentId()).toString()); |
There was a problem hiding this comment.
Critical: Path mismatch prevents attachment cleanup.
The path construction here is now inconsistent with getAttachedFile at line 273. Files are created at prefixedPath/email_tmp/resourceId but this deletion attempts prefixedPathemail_tmpattachmentId (missing separators), causing:
- Temporary files are never deleted (resource leak)
- Disk space accumulates over time
- Potential system issues as temp directory fills
Apply this diff to restore consistent path construction:
- sbPrefixedPath.append("email_tmp");
+ sbPrefixedPath.append("/email_tmp/");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| sbPrefixedPath.append("email_tmp"); | |
| a = (Attachment) iter.next(); | |
| deleteAttachedFile(sbPrefixedPath.append( | |
| a.getAttachmentId()).toString()); | |
| sbPrefixedPath.append("/email_tmp/"); | |
| a = (Attachment) iter.next(); | |
| deleteAttachedFile(sbPrefixedPath.append( | |
| a.getAttachmentId()).toString()); |
🤖 Prompt for AI Agents
In
msgcntr/messageforums-app/src/java/org/sakaiproject/tool/messageforums/ForumsEmailService.java
around lines 256 to 259, the deletion path is built incorrectly by appending
"email_tmp" and attachmentId to the same StringBuilder without path separators
and by mutating sbPrefixedPath used elsewhere; change the deletion logic to
construct the path consistently with getAttachedFile (prefixedPath +
File.separator + "email_tmp" + File.separator + resourceId/attachmentId) and do
not reuse/mutate the original sbPrefixedPath (create a new StringBuilder or use
String.join/Paths.get) so the final string matches the created file path and
temporary files are actually deleted.
|
This PR has been automatically marked as stale due to 10 days of inactivity. It will be closed in 4 days unless there is new activity. |
2 participants
https://sakaiproject.atlassian.net/browse/SAK-52133
Summary by CodeRabbit