Skip to content

Feat: Add Telegram notifications for drop claims - #18

Open
vurmil wants to merge 5 commits into
rangermix:mainfrom
vurmil:feature/notifications-telegram
Open

Feat: Add Telegram notifications for drop claims#18
vurmil wants to merge 5 commits into
rangermix:mainfrom
vurmil:feature/notifications-telegram

Conversation

@vurmil

@vurmil vurmil commented Dec 13, 2025

Copy link
Copy Markdown
Contributor
  • Add Telegram notification service (telegram_service.py) for sending drop claim alerts
  • Add Telegram settings UI in Settings tab with bot token and chat ID inputs
  • Add /api/settings/test-telegram endpoint to verify Telegram bot connectivity
  • Add Save Settings and Test Connection buttons for Telegram configuration
  • Persist Telegram bot token and chat ID in application settings
  • Add Help tab section with 7-step Telegram bot setup instructions (BotFather workflow)
  • Implement automatic Telegram notifications when drops are claimed
  • Add multilingual support for Telegram settings and instructions in all 19 languages

@rangermix rangermix left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks promising but I'm in the middle of refactoring so need to wait for that. Also I don't really use Telegram so better to have unit tests for the telegram service

@rangermix

Copy link
Copy Markdown
Owner

Review findings against head 67e13cd:

  1. src/web/managers/settings.py:56,147 returns the full telegram_bot_token from /api/settings and includes it in every settings_updated Socket.IO broadcast. Because the web API/socket has no per-client authentication, any client that can reach the miner can recover the bot credential. Please keep the token server-side and return only a configured flag or masked placeholder; updates can accept a replacement token without echoing the stored value.

  2. src/services/message_handlers.py:211-215 ignores the boolean returned by drop.claim() and always sends “Drop Claimed!”. _claim() explicitly returns False for GQL errors/rejected claims, so this produces a false success notification on the exact failure path. Gate the notification on the returned result.

  3. web/static/app.js:1719-1728 indexes every settings <h2> globally, but the Telegram section also contains the existing Mining Benefits heading. headers[2] is therefore Mining Benefits, not Games to Watch, and headers[3] is Games to Watch, not Actions. Applying a non-English locale relabels those sections incorrectly. Please select each section by a stable element/ID instead of positional indexes.

  4. src/services/telegram_service.py:52-59 sends Twitch-provided campaign/game/drop/reward names under Telegram parse_mode="HTML" without escaping them. A normal name containing & or < makes Telegram reject the whole notification (and markup-like text can alter formatting). Escape every dynamic field before composing the HTML message.

  5. The PR head does not pass its own Python suite: python -W error::RuntimeWarning -m pytest tests/ -q reports 3 failures (test_proxy_persistence_trigger, test_update_proxy_setting, and test_settings_manager_networking) because the settings mocks were not updated for the new Telegram fields. This also confirms the Telegram path still lacks the requested unit coverage.

The PR is also currently conflicting with main; the findings above are independent of those merge conflicts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants