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

[st-manu]

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

Summary by CodeRabbit

• Bug Fixes
  • Fixed file-path construction for temporary email attachments: paths now consistently normalize separators and use a stable temp path so attachments are stored and retrieved reliably.

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

[coderabbitai]

Walkthrough

Reworked path construction in ForumsEmailService: temporary attachment paths now built with an "email_tmp" segment and attachment IDs/resourceIds are normalized to forward slashes. Removed intermediate sbPrefixedPath usage and adjusted both send() and getAttachedFile() to use the new filePath construction.

Changes

Cohort / File(s)	Change Summary
Temporary email attachment path handling msgcntr/messageforums-app/src/java/org/sakaiproject/tool/messageforums/ForumsEmailService.java	In send(), replaced reuse of sbPrefixedPath with a new filePath StringBuilder that appends "email_tmp" and the attachmentId with backslashes normalized to /. In getAttachedFile(), normalize resourceId backslashes to / before appending to the prefixed path and removed sbPrefixedPath temporary variable usage.

Suggested reviewers

• ottenhoff
• ern
• jesusmmp

Pre-merge checks and finishing touches

❌ 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%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ 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 addresses the main objective of the pull request - fixing email notifications when attachments are added to discussions, which aligns with the code changes that normalize file paths for attachment handling.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 32c5b2c and eee2b6f.

📒 Files selected for processing (1)

• msgcntr/messageforums-app/src/java/org/sakaiproject/tool/messageforums/ForumsEmailService.java (2 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

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

⏰ 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). (2)

• GitHub Check: maven-build
• GitHub Check: sakai-deploy

---

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

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e1eb097 and 32c5b2c.

📒 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.

[github-actions]

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.

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

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

273-273: Normalizing resourceId improves cross-platform path handling

Replacing backslashes with forward slashes before appending resourceId keeps the temp-file path consistent with how deletion and substring logic treat / as the separator. You might optionally extract this normalization (attachmentId.replace('\\', '/')) into a small helper to avoid duplication between creation and cleanup, but the current change is functionally sound.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 32c5b2c and eee2b6f.

📒 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 (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 (2)

📓 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)

📚 Learning: 2025-10-07T15:11:27.298Z

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.

Applied to files:

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

⏰ 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: maven-build
• GitHub Check: sakai-deploy

🔇 Additional comments (1)

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

255-258: Temp-file deletion path now correctly mirrors creation path

Using a fresh StringBuilder per attachment with prefixedPath + "/email_tmp/" + normalizedId aligns the delete path with getAttachedFile, fixing the earlier mismatch and avoiding accidental mutation of shared state. This should reliably clean up the temp files for all attachments.