mail channel - changed interval from minutes to hrs #1659
Conversation
WalkthroughThe pull request modifies the Changes
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
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: 1
🧹 Nitpick comments (1)
kairon/events/utility.py (1)
52-52: LGTM! Consider refactoring the duplicated cron expression.The cron expression correctly implements hourly scheduling at minute 0. However, the same expression is duplicated in two places.
Consider extracting the cron expression to a constant or method to avoid duplication:
@staticmethod def schedule_channel_mail_reading(bot: str): from kairon.shared.channels.mail.processor import MailProcessor try: mail_processor = MailProcessor(bot) interval = max(1, int(mail_processor.config.get("interval", 2)) % 24) + cron_expr = f"0 */{interval} * * *" event_id = mail_processor.state.event_id if event_id: KScheduler().update_job(event_id, TASK_TYPE.EVENT, - f"0 */{interval} * * *", + cron_expr, EventClass.mail_channel_read_mails, {"bot": bot, "user": mail_processor.bot_settings.user}) else: event_id = uuid7().hex mail_processor.update_event_id(event_id) KScheduler().add_job(event_id, TASK_TYPE.EVENT, - f"0 */{interval} * * *", + cron_expr, EventClass.mail_channel_read_mails, {"bot": bot, "user": mail_processor.bot_settings.user})Also applies to: 59-59
| interval = max(1, int(mail_processor.config.get("interval", 2)) % 24) | ||
|
|
There was a problem hiding this comment.
💡 Codebase verification
Interval change is safe, but tests need updates
The verification shows that the interval configuration is primarily used in tests, where it's set to 1. These test configurations need to be updated to reflect the change from minutes to hours:
tests/unit_test/channels/mail_scheduler_test.py: Update test configurations that use{"interval": 1}to account for the new hour-based interval.
The change is otherwise safe because:
- No hardcoded production configurations were found in the codebase
- The default value of 2 hours in the code provides a reasonable fallback
- The interval is only used within the mail processor context
🔗 Analysis chain
LGTM! Consider adding migration notes for existing users.
The interval calculation correctly implements the hour-based scheduling with proper bounds (1-24 hours). However, since this changes the interpretation of the interval value from minutes to hours, existing configurations might need updates.
Let's check for existing configurations that might be affected:
Consider adding migration notes in the documentation to guide users on updating their existing interval configurations from minutes to hours.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for interval configurations in the codebase
# Expected: Find any hardcoded interval values or configuration examples
# Search for interval configurations
rg -A 2 '"interval":\s*\d+' --type json
rg -A 2 'interval.*=.*\d+' --type python
Length of output: 115
Script:
#!/bin/bash
# Let's try different search patterns and file extensions
# Search for interval configurations in Python files
rg -A 2 '"interval":\s*\d+' -g '*.py'
rg -A 2 'interval.*=.*\d+' -g '*.py'
# Search for mail processor configurations
rg -A 2 'mail_processor.*config' -g '*.py'
# Search for any mail interval related configurations in yaml/yml files
rg -A 2 'interval:' -g '*.{yaml,yml}'
# Search for test configurations
rg -A 2 'interval' -g '*test*.py'
Length of output: 3532
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
string_to_listmethod for clarity and consistency.