diff --git a/README.md b/README.md index 79e7197c..7dff6db2 100644 --- a/README.md +++ b/README.md @@ -467,7 +467,7 @@ All 17 keys, read by your launcher. A change needs a restart. Blank means unset. | `slack_webhook` | | Slack incoming webhook, for outbound reports | | `discord_webhook` | | Discord webhook, for outbound reports | | `telegramBotToken` | | Telegram bot token, for outbound reports | -| `slackBotToken` | | Slack bot token, for replies in Slack | +| `slackBotToken` | | Slack bot token, for replies and chart uploads in Slack | | `telegramOutgoingWebhook` | | Telegram webhook path the bot accepts (interactive mode) | | `telegramOutgoingToken` | | Secret token Telegram must present (interactive mode) | | `slackOutgoingWebhook` | | Slack webhook path the bot accepts (interactive mode) | @@ -698,7 +698,7 @@ Note: The utils folder contains scripts to generate `/etc/nginx/aws_subnets` and With these options set, your bot will auto-subscribe your URL to events the bot sees, when you run `bot.py`. It unregisters and registers the webhook as it starts (and only then, which also wakes up a stale connection); if Telegram refuses, finbot does not start and the log says why. #### Slack -Visit https://api.slack.com/apps/ to create a new Slack app from scratch (if you already created one for a finbot webhook, you can reuse that app). Replies in Slack are text: charts go to Telegram only, since Slack retired the file upload API finbot used. +Visit https://api.slack.com/apps/ to create a new Slack app from scratch (if you already created one for a finbot webhook, you can reuse that app). Charts in replies are uploaded into the channel with Slack's file upload API (`files.getUploadURLExternal`, then `files.completeUploadExternal`, which replaced the retired `files.upload`). Scheduled reports reach Slack through the incoming webhook, which takes text only, so they come without charts. If an upload fails (a missing scope, say), the reply goes out as text and the log says why. * From _Basic Information > Verification Token_, copy _Verification token_ into `finbot.env` as `slackOutgoingToken`. This allows Slack to authenticate with finbot. * In `finbot.env`, put your web server URL (e.g. https://www.example.com:8443/slack) into `slackOutgoingWebhook` * Save `finbot.env` and restart `bot.py` @@ -711,7 +711,8 @@ Visit https://api.slack.com/apps/ to create a new Slack app from scratch (if you * Check the box _App Home > Allow users to send Slash commands and messages from the messages tab_ * Save Changes * Under _OAuth & Permissions_: - * Scroll down to _Scopes > Bot Token Scopes_, and add: `chat:write`. + * Scroll down to _Scopes > Bot Token Scopes_, and add: `chat:write`, `files:write` and `files:read` (Slack's file upload API needs both `files` scopes). + * Adding scopes to an app that is already installed takes effect only after _Reinstall to Workspace_. * Scroll up to _OAuth Tokens for Your Workspace_: * If _Bot User OAuth Token_ is not visible, hit _Install to Workspace > Allow_ * Copy _Bot User OAuth Token_ into `finbot.env` as `slackBotToken`. This allows finbot to authenticate with Slack so it can send replies. diff --git a/lib/webhook.py b/lib/webhook.py index e231d1bc..b9b83a1e 100644 --- a/lib/webhook.py +++ b/lib/webhook.py @@ -190,47 +190,98 @@ def visible(text): kept.append(line) return '\n'.join(kept) + f"\n…and {len(lines) - len(kept)} more in the chart" -def takes_charts(service): - """Whether a reply to `service` can carry a chart image. Only Telegram: Slack retired its upload API (files.upload), and the Slack - and Discord webhooks never took images. Elsewhere a report goes out as text.""" - return service == 'telegram' +MAYBE_SENT = object() # a read timeout on the request that posts a chart: it may have arrived, so no text copy follows -def sendPhoto(chat_id, image_data, caption, service, message_id=None, reply_markup=None): - """Telegram: the chart, with the report as its caption (trimmed to fit). Where a chart cannot go (takes_charts), or Telegram - refuses it, the report is sent as text instead, without the chart's buttons, so it does not vanish with its chart; but not after - a read timeout, when the chart may have arrived after all.""" - if takes_charts(service): - url = webhooks['telegram'] + "sendPhoto?chat_id=" + str(chat_id) - headers = {} - data = { - 'disable_notification': True, - 'chat_id': chat_id, - 'caption': fit_caption(caption), - 'parse_mode': 'HTML', - 'disable_web_page_preview': True, - 'allow_sending_without_reply': True, - 'reply_to_message_id': message_id} - if reply_markup: - data['reply_markup'] = json.dumps(reply_markup) # inline keyboard (Telegram only) - files = {"photo": ('image.png', image_data)} - if hasattr(image_data, 'seek'): - image_data.seek(0) +def takes_charts(service, reply=True): + """Whether a report to `service` can carry a chart image: on Telegram always; on Slack in a reply to a chat (`reply`), uploaded + into its channel with the bot token, but not in a scheduled report, which goes through the incoming webhook and takes text only; + never on Discord. Elsewhere a report goes out as text.""" + return service == 'telegram' or (service == 'slack' and reply and bool(config_slackBotToken)) + +def telegram_photo(chat_id, image_data, caption, message_id=None, reply_markup=None): + """sendPhoto, the report as its caption (trimmed to fit). Telegram's answer, MAYBE_SENT, or None if it was refused.""" + url = webhooks['telegram'] + "sendPhoto?chat_id=" + str(chat_id) + data = { + 'disable_notification': True, + 'chat_id': chat_id, + 'caption': fit_caption(caption), + 'parse_mode': 'HTML', + 'disable_web_page_preview': True, + 'allow_sending_without_reply': True, + 'reply_to_message_id': message_id} + if reply_markup: + data['reply_markup'] = json.dumps(reply_markup) # inline keyboard (Telegram only) + if hasattr(image_data, 'seek'): + image_data.seek(0) + try: + r = requests.post(url, data=data, files={"photo": ('image.png', image_data)}, timeout=config_http_timeout) + except requests.exceptions.ReadTimeout as e: + print("Telegram sendPhoto timed out, the chart may still arrive:", e, file=sys.stderr) + return MAYBE_SENT + except Exception as e: + print("Failure executing request:", url, data, str(e)) + return None + try: + output = r.json() + except ValueError: + output = {} + if r.status_code == 200 and output.get('ok'): + print(r.status_code, "OK telegram sendPhoto", data['caption']) + return output + print(r.status_code, "error telegram sendPhoto", output.get('error_code'), output.get('description') or r.text[:300], file=sys.stderr) + return None + +def slack_api(method, **data): + """POST a Slack Web API method as the bot, form-encoded, and return its answer. Raises RuntimeError with Slack's error if not ok.""" + r = requests.post('https://slack.com/api/' + method, headers={'Authorization': 'Bearer ' + config_slackBotToken}, data=data, timeout=config_http_timeout) + try: + output = r.json() + except ValueError: + output = {} + if not output.get('ok'): + raise RuntimeError(f"Slack {method}: {output.get('error') or r.status_code}") + return output + +def slack_photo(chat_id, image_data, caption, message_id=None): + """Upload the chart into a Slack channel with the report as its message, the way Slack takes files since it retired files.upload: + ask for an upload URL, send the bytes there, then complete the upload into the channel (the bot needs the files:write and + files:read scopes). Slack's answer, MAYBE_SENT, or None if a step failed.""" + if hasattr(image_data, 'seek'): + image_data.seek(0) + data = image_data.read() if hasattr(image_data, 'read') else bytes(image_data) + try: + ticket = slack_api('files.getUploadURLExternal', filename='chart.png', length=len(data)) + r = requests.post(ticket['upload_url'], data=data, timeout=config_http_timeout) + if r.status_code != 200: + raise RuntimeError(f"Slack upload: {r.status_code} {r.text[:200]}") + share = {'files': json.dumps([{'id': ticket['file_id']}]), 'channel_id': chat_id, 'initial_comment': caption} + if message_id: + share['thread_ts'] = message_id try: - r = requests.post(url, data=data, headers=headers, files=files, timeout=config_http_timeout) + output = slack_api('files.completeUploadExternal', **share) except requests.exceptions.ReadTimeout as e: - print("sendPhoto timed out, the chart may still arrive:", e, file=sys.stderr) - return None - except Exception as e: - print("Failure executing request:", url, data, str(e)) - else: - try: - output = r.json() - except ValueError: - output = {} - if r.status_code == 200 and output.get('ok'): - print(r.status_code, f"OK {service} sendPhoto", data['caption']) - return output - print(r.status_code, f"error {service} sendPhoto", output.get('error_code'), output.get('description') or r.text[:300], file=sys.stderr) + print("Slack files.completeUploadExternal timed out, the chart may still arrive:", e, file=sys.stderr) + return MAYBE_SENT + except Exception as e: + print("Slack chart upload failed:", e, file=sys.stderr) + return None + print("OK slack chart upload to", chat_id, file=sys.stderr) + return output + +def sendPhoto(chat_id, image_data, caption, service, message_id=None, reply_markup=None): + """The chart, with the report as its caption: on Telegram, or uploaded into the Slack channel of a reply. Where a chart cannot go + (takes_charts), or the service refuses it, the report is sent as text instead, without the chart's buttons, so it does not vanish + with its chart; but not after a read timeout on the request that posts it, when the chart may have arrived after all.""" + output = None + if service == 'telegram': + output = telegram_photo(chat_id, image_data, caption, message_id, reply_markup) + elif takes_charts(service, reply=bool(chat_id)): + output = slack_photo(chat_id, image_data, caption, message_id) + if output is MAYBE_SENT: + return None + if output: + return output + if takes_charts(service, reply=bool(chat_id)): print("chart not sent, sending the report as text", file=sys.stderr) return payload_wrapper(service, chat_url(service, chat_id) if chat_id else webhooks.get(service), caption.split('\n'), chat_id, message_id) diff --git a/lib/worker.py b/lib/worker.py index f2eba822..01fb0954 100755 --- a/lib/worker.py +++ b/lib/worker.py @@ -17,6 +17,8 @@ import shorts import trades +_request = threading.local() # for the thread handling a chat request: the typing indicators it started (see process_request) + class TypingIndicator: def __init__(self, service, chat_id, max_loops=3, action='typing'): self.service = service @@ -40,6 +42,7 @@ def start(self): return self._thread = threading.Thread(target=self._worker, daemon=True) self._thread.start() + getattr(_request, 'typing', []).append(self) def stop(self): if self.service != 'telegram' or not self._thread: @@ -99,12 +102,23 @@ def process_callback(service, callback_id, chat_id, message_id, data): except Exception as e: webhook.report_error(e, service, chat_id, context='chart') +def stop_typing(): + """Stop the typing indicators the current request started. A command stops its own when it ends normally; one that fails part way + would leave "typing…" showing for up to 20 seconds, or bring it back after its error line.""" + for indicator in getattr(_request, 'typing', ()): + indicator.stop() + _request.typing = [] + def process_request(service, chat_id, user, message, botName, userRealName, message_id): - """Entry point for inbound chat requests: any failure becomes a one-line reply in the originating chat.""" + """Entry point for inbound chat requests: any failure becomes a one-line reply in the originating chat, after the typing stops.""" + _request.typing = [] try: _process_request(service, chat_id, user, message, botName, userRealName, message_id) except Exception as e: + stop_typing() webhook.report_error(e, service, chat_id, context=message.split()[0][:30] if message.split() else None) + finally: + stop_typing() if service == 'telegram': try: charts.ensure_keyboard(chat_id) # keep DM keyboards current after the reply diff --git a/performance.py b/performance.py index 28db8428..d49407cd 100755 --- a/performance.py +++ b/performance.py @@ -62,7 +62,7 @@ def prepare_performance_payload(service, performance, portfolios): period = util.days_english(past_days) message = webhook.bold(f"Performance over {period}", service) payload.insert(0, message) - if chart_rows and webhook.takes_charts(service): + if chart_rows and webhook.takes_charts(service, interactive): subtitle = f"% change {'' if period == 'today' else 'over '}{period}" chart_rows.sort(key=lambda row: row[1], reverse=True) # best first graph = util.rows_chart(chart_rows, "Performance", subtitle, value_fmt=lambda v: util.signed_percent(v, 2)) diff --git a/price.py b/price.py index fd9d55b2..64b90d4c 100755 --- a/price.py +++ b/price.py @@ -63,7 +63,7 @@ def prepare_price_payload(service, market_data, threshold, _rows_out=None): # wishlist items will come here # not a holding (e.g. watchlist): use Yahoo history instead, summarised once in the main section below if specific_stock: - percent, graph = yahoo.price_history(ticker, days, graph=config_graph and webhook.takes_charts(service), graphCache=False) + percent, graph = yahoo.price_history(ticker, days, graph=config_graph and webhook.takes_charts(service, interactive), graphCache=False) if isinstance(percent, str): raise RuntimeError(percent) # the one ticker asked for has no history: reported in one line else: @@ -172,7 +172,7 @@ def prepare_price_payload(service, market_data, threshold, _rows_out=None): payload.insert(0, heading) if top: payload.extend([''] + payload_bottom) - can_chart = _rows_out is None and webhook.takes_charts(service) + can_chart = _rows_out is None and webhook.takes_charts(service, interactive) if top and chart_top and can_chart: def short(ticker): return ticker.split('.')[0].removesuffix('-USD') diff --git a/shorts.py b/shorts.py index f0009645..106a8e10 100755 --- a/shorts.py +++ b/shorts.py @@ -44,7 +44,7 @@ def last_element(e): message = f'Tracked stocks shorted over {threshold}%' message = webhook.bold(message, service) payload.insert(0, message) - if chart_rows and webhook.takes_charts(service): + if chart_rows and webhook.takes_charts(service, interactive): chart_rows.sort(key=lambda row: row[1], reverse=True) graph = util.shorts_chart(chart_rows, threshold) else: diff --git a/tests/common.py b/tests/common.py index 94af8525..cd359da6 100644 --- a/tests/common.py +++ b/tests/common.py @@ -87,10 +87,16 @@ def setUp(self): self.patch(sharesight, 'get_trades', lambda name, pid, days=7: [{ 'id': 1, 'transaction_type': 'BUY', 'symbol': 'T1', 'market': 'NASDAQ', 'portfolio': name, 'transaction_date': time.strftime('%Y-%m-%d'), 'quantity': '10', 'price': '5.0', 'value': '50', 'brokerage_currency_code': 'USD', 'holding_id': 7}]) - typing = [] + self.typing = [] # every typing indicator a test starts; none may still be running when the test ends real_start = worker.TypingIndicator.start - self.patch(worker.TypingIndicator, 'start', lambda indicator: (typing.append(indicator), real_start(indicator))) - self.addCleanup(lambda: [indicator.stop() for indicator in typing]) # a command that failed leaves its indicator running: stop it with the test + self.patch(worker.TypingIndicator, 'start', lambda indicator: (self.typing.append(indicator), real_start(indicator))) + self.addCleanup(self.no_typing_left) + + def no_typing_left(self): + left = [indicator for indicator in self.typing if indicator.is_active()] + for indicator in left: + indicator.stop() + self.assertEqual(left, [], 'a typing indicator was left running') def command(self, text, chat='55'): """Run a chat message through the real command parser and handlers.""" diff --git a/tests/test_edges.py b/tests/test_edges.py index 9ab066ff..d1552b17 100644 --- a/tests/test_edges.py +++ b/tests/test_edges.py @@ -51,6 +51,15 @@ def down(*a, **k): worker.process_request('telegram', '55', '@u', '.performance 7d', BOT, 'U', '1') self.assertEqual(len(self.errors), 1) + def test_a_failed_command_stops_typing_before_saying_why(self): + def down(*a, **k): + raise RuntimeError('Sharesight performance error: boom') + self.patch(sharesight, 'get_portfolios', down) + typing_when_reported = [] + self.patch(webhook, 'report_error', lambda e, *a, **k: typing_when_reported.append([t.is_active() for t in self.typing])) + worker.process_request('telegram', '55', '@u', '.performance 7d', BOT, 'U', '1') + self.assertEqual(typing_when_reported, [[False]]) # its one indicator had stopped when the error line went out + def test_no_holdings_and_empty_watchlist(self): self.patch(util, 'get_holdings_and_watchlist', lambda: []) for cmd in ('.watchlist', '.price', '.earnings'): diff --git a/tests/test_providers.py b/tests/test_providers.py index 33ca5605..d1841889 100644 --- a/tests/test_providers.py +++ b/tests/test_providers.py @@ -456,13 +456,84 @@ def test_guarded_reports_and_exits_nonzero(self): class ChartDelivery(FinbotCase): - """Charts go to Telegram only. A report whose chart cannot be sent goes out as text instead of vanishing.""" + """Charts go to Telegram, and to Slack in replies (uploaded into the channel). A report whose chart cannot be sent goes out as + text instead of vanishing.""" + UPLOAD_URL = 'https://files.slack.com/upload/v1/abc' def setUp(self): super().setUp() self.patch(webhook, 'sendPhoto', REAL_SEND_PHOTO) - def test_slack_replies_are_text_and_no_chart_is_drawn_for_them(self): + def slack_answers(self, *outcomes): + """requests.post for the upload steps: the outcomes in turn (an exception is raised). Returns the calls made.""" + calls = [] + def post(url, **kwargs): + calls.append((url, kwargs)) + outcome = outcomes[len(calls) - 1] + if isinstance(outcome, Exception): + raise outcome + return outcome + self.patch(webhook.requests, 'post', post) + return calls + + def upload_steps(self): + return [Resp({'ok': True, 'upload_url': self.UPLOAD_URL, 'file_id': 'F1'}), Resp(None, text='OK - 3'), Resp({'ok': True, 'files': [{'id': 'F1'}]})] + + def test_where_charts_can_go(self): + self.patch(webhook, 'config_slackBotToken', 'xoxb-test') + cases = (('telegram',), ('telegram', False), ('slack',), ('slack', False), ('discord',)) + self.assertEqual([webhook.takes_charts(*case) for case in cases], [True, True, True, False, False]) # Slack: replies, not scheduled reports + self.patch(webhook, 'config_slackBotToken', False) + self.assertFalse(webhook.takes_charts('slack')) + + def test_slack_replies_upload_their_charts(self): + self.patch(webhook, 'config_slackBotToken', 'xoxb-test') + for cmd, heading in (('.beta', '*Beta over 1.5'), ('.price', '*5.0% at close'), ('.compare T1 T2', '*')): + with self.subTest(cmd=cmd): + calls = self.slack_answers(*self.upload_steps()) + self.sent.clear(); self.errors.clear() + worker.process_request('slack', 'C1', '<@U1>', cmd, BOT, 'U', '1') + self.assertEqual(self.errors, []) + self.assertEqual([url for url, _ in calls], ['https://slack.com/api/files.getUploadURLExternal', self.UPLOAD_URL, + 'https://slack.com/api/files.completeUploadExternal']) + (_, ticket), (_, upload), (_, share) = calls + self.assertEqual(ticket['headers'], {'Authorization': 'Bearer xoxb-test'}) + self.assertTrue(upload['data'].startswith(b'\x89PNG')) # the chart's bytes, as the body + self.assertEqual((ticket['data']['filename'], ticket['data']['length']), ('chart.png', len(upload['data']))) + self.assertEqual((share['data']['channel_id'], json.loads(share['data']['files'])), ('C1', [{'id': 'F1'}])) + self.assertTrue(share['data']['initial_comment'].startswith(heading), share['data']['initial_comment'][:80]) # the report + self.assertNotIn(share['data']['initial_comment'].split('\n'), [payload for _, payload in self.sent]) # not sent again as text + + def test_a_slack_upload_that_fails_goes_out_as_text(self): + self.patch(webhook, 'config_slackBotToken', 'xoxb-test') + good = self.upload_steps() + text = [('text', ['*Heading*', 'line 2'])] + for outcomes, expect in (([Resp({'ok': False, 'error': 'missing_scope'})], text), ([ConnectionError('down')], text), + ([good[0], Resp(None, status=500, text='nope')], text), + ([good[0], good[1], Resp({'ok': False, 'error': 'not_in_channel'})], text), + ([good[0], good[1], webhook.requests.exceptions.ReadTimeout('slow')], []), # it may have been shared after all + (good, [])): + with self.subTest(outcomes=outcomes): + self.slack_answers(*outcomes) + self.sent.clear() + with contextlib.redirect_stderr(io.StringIO()), contextlib.redirect_stdout(io.StringIO()): + webhook.sendPhoto('C1', io.BytesIO(b'png'), '*Heading*\nline 2', 'slack') + self.assertEqual(self.sent, expect) + + def test_scheduled_reports_to_slack_stay_text(self): + self.patch(webhook, 'config_slackBotToken', 'xoxb-test') + webhook.webhooks['slack'] = 'https://hooks.slack.com/services/x' # an incoming webhook cannot upload + sent = [] + self.patch(webhook, 'sendPhoto', lambda chat, image, caption, service, **k: sent.append((service, 'chart'))) + self.patch(webhook, 'payload_wrapper', lambda service, url, payload, *a, **k: sent.append((service, 'text')) or []) + for report in (price.lambda_handler, lambda: performance.lambda_handler(past_days=7), shorts.lambda_handler): + with self.subTest(report=report): + sent.clear() + report() + self.assertEqual(sorted(set(sent)), [('slack', 'text'), ('telegram', 'chart')]) + + def test_slack_without_a_bot_token_gets_text_and_no_chart_is_drawn(self): + self.patch(webhook, 'config_slackBotToken', False) def refuse(*a, **k): raise AssertionError('a chart was drawn for Slack') for name in ('rows_chart', 'pie_chart', 'bar_graph', 'shorts_chart'):