Slack chart replies via Slack's current upload API; a failed command stops its typing indicator

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.

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)
 

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

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

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

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:

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

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'):