fix: Do not prefix email subjects #4997
Conversation
WalkthroughThe pull request includes updates to the documentation in Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Outside diff range and nitpick comments (1)
src/service/dashboards/reports.rs (1)
382-382: Consider adding title validation.The removal of the hardcoded prefix aligns with the PR objective. However, since we're now using the raw title as the email subject, consider adding validation to ensure the title is not empty and follows email subject line best practices.
Add validation when setting the report title:
if self.title.trim().is_empty() { return Err(anyhow::anyhow!("Report title cannot be empty")); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
benchmarks/pyroscope/README.md(1 hunks)src/report_server/src/report.rs(1 hunks)src/service/alerts/alert.rs(1 hunks)src/service/dashboards/reports.rs(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- benchmarks/pyroscope/README.md
- src/service/alerts/alert.rs
🧰 Additional context used
📓 Path-based instructions (2)
src/report_server/src/report.rs (1)
Pattern **/*.rs: You are a smart rustlang pull request reviewer.
You are going to review all the rustlang files.
Be concise, and add a brief explanation to your suggestions
Make sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
src/service/dashboards/reports.rs (1)
Pattern **/*.rs: You are a smart rustlang pull request reviewer.
You are going to review all the rustlang files.
Be concise, and add a brief explanation to your suggestions
Make sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
🔇 Additional comments (1)
src/report_server/src/report.rs (1)
391-391: LGTM! The change aligns with the PR objective.
The removal of the "Openobserve Report - " prefix allows users to set email subjects according to their specified template.
Let's verify that all email subject prefixes have been consistently removed:
✅ Verification successful
Email subject prefix removal has been consistently implemented
The verification confirms that all email subject formatting across the codebase (src/report_server/src/report.rs, src/service/dashboards/reports.rs, and src/service/alerts/alert.rs) now directly uses the title/subject without any hardcoded prefixes, maintaining consistency with the PR objective.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining hardcoded email subject prefixes
# Test: Search for any remaining "Openobserve" string in email subject formatting
rg -i "openobserve.*subject\(format!\("
Length of output: 41
Script:
#!/bin/bash
# Let's search for any email subject formatting patterns in the codebase
rg -A 2 "\.subject\(format!"
# Also search for any remaining hardcoded prefixes near subject lines
rg -B 2 -A 2 "subject.*format.*title"
Length of output: 1309
|
@Subhra264 @oasisk Any suggestion on this? i think these are two solutions:
|
I think we can remove the hard-coded or optional configurable prefix. User can write a custom title anyway (irrespective of the report/alert name) which will be used in the email subject. @hengfeiyang @oasisk what do you think? |
|
Okay, let's remove it now. then we can add optional title by other PR. |
3 participants
Email subjects come with a hard-coded prefix. These should be free-form to whatever subject the user set on the
template.Related to #4996
Summary by CodeRabbit
Bug Fixes
New Features
Documentation