Opportunistically preserve threading between Slack instances #529
Conversation
patcon
changed the title
patcon
changed the title
|
Yeeeeeeaahhh! got threading working with pretty minimal additions. Seriously, @42wim, you seem to have designed this pretty well -- so much is possible :)
|
|
Not sure what's up with the avatar showing up in open threads, but only showing default icon in the thread summary bit... i have another bot for which this works a-ok EDIT: retracted. The other bot doesn't exercise this edgecase. |
patcon
changed the title
|
Also, a glitch that existed on mobile now becomes more prominent: it seems that messages created by the bridge cannot be turned into threads on android mobile -- trying to click a bot message results in a toast message saying "Sorry, an error occured. Please try again." Also, when a reply to a user message is made (where the thread-starter is a real user), on entering the thread from that, the bot messages don't show up. (Replies do however, show as present from outside the thread.) Again, this is only on the latest version of android mobile. This issue goes back at least to v1.9.0, as that's what i'm running on my other high-usage production instance. I've confirmed that switching between RTM and web api doesn't seem to create any sorts of different effects. As far as I can tell, this is a Slack bug. I've checked out the conversations API response to compare the two types of messages, and there appears to be nothing unexpectedly different between the message objects In summary:
Other that the seemingly upstream bug, this is ready for review. Perhaps we could put it behind a feature toggle and default to disabled? |
|
Hm. Oddly. Messages in the other channels. So there's some set of message properties that don't cause this error. Will keep investigating |
|
Confirmed that's it's messages without attachments that give the error on mobile. This was masked for me since all our translations have attachments with the original text. Hmm.... EDIT: Creating an attachment with pretext of "." solves the issue with the mobile bug. Unfortunately, the much more subtle "footer" property is stripped away on mobile, and so the bugfix attachment is pretty visible |
patcon
force-pushed
the
529-slack-rethreading
branch
from
b4cb094 to
24f3e42
Compare
patcon
changed the title
|
Sent a support request to feedback@slack.com (will post any reply) |
|
Re: slack bug. heard back from Slack within 12 hours, and they seem very interested in resolving this. I dropped the ball with my lack of reply, but will today send them response on all the very specific followup questions they had |
|
Sorry to bother you with the extra comments again :/ |
patcon
force-pushed
the
529-slack-rethreading
branch
from
dce2bed to
588f04f
Compare
bridge/slack/handlers.go
Outdated
| return true | ||
| } | ||
|
|
||
| botInfo, _ := b.rtm.GetBotInfo(ev.BotID) |
There was a problem hiding this comment.
There's no way to get a list of bots via API, and so we can't have a populateBots like we have for populateUsers. But we can keep them around after we've seen them, and save on all subsequent calls. I'll only work on this if we decide to take this route
|
Ok, I was mistaken with some of my comments (sorry), and ended up taking all your suggestions into account. Let me know if you notice anything else |
|
And just so it's not lost in the shuffle, this commit is a significant one that we might want to cherry-pick into another PR: 588f04f (didn't mean to sneak it in, just wanted to get threading working on mobile before merge) |
|
As said in the other issue, I'm not going to merge 588f04f wrt removing callback_id. |
Co-Authored-By: patcon <patrick.c.connolly@gmail.com>
patcon
force-pushed
the
529-slack-rethreading
branch
from
f4e4595 to
6c72ec6
Compare
|
Done and done :) |
* Opportunistically preserve Slack threading when parent thread in cache. [42wim#529] * Removed slack-specific processing from gateway. * Added docs. * Add option to enable threading, with default to off. * Did cleanup on @42wim's comments. * Update gateway/gateway.go Co-Authored-By: patcon <patrick.c.connolly@gmail.com> * Suggestion from @42wim :) * Suggestions from @42wim. * More suggestions.
Is your feature request related to a problem? Please describe.
Even with clear documentation and encouragement of work-arounds, unthreading of messages between slacks leads to confusion and misunderstanding.
Describe the solution you'd like
Would be great to preserve threading between Slack instances, when possible. Presumably this would be when the parent message is still in cache.
Describe alternatives you've considered
Additional context
Downside: There would be inconsistency: sometimes a channel-level message would have originated as a channel-level message, but sometimes it would be a thread-level message where there wasn't enough detail in cache to rethread. And the oldest replies (not in cache) would be the most common to unthread, which are the ones most likely to cause confusion anyhow,