Optionally threading posts to parent topic in slack integration

Thanks much!

Last night, I implemented and have passing tests all through the stack (though I haven’t yet tested live) my first two ACs, but not yet the transcript flow, using a custom field on Topic and a thread rule. So making good progress toward being able to show code in at least a draft PR.

I also have a separate commit to remove the unused pstore_get from the slack provider. Since that’s the only use of pstore in the whole stack, would you like me to also remove all the pstore_* from app/initializers/discourse_chat.rb in that cleanup commit?

That’s exactly where I started, and it sure would have been easier!

However, at least for my use case (and I’ve heard this from people at other companies, we’re not unique) different channels have different preferences for whether threads should be used. There are channels which are meant to be used with threads (because most people care only about some topics) and channels which are very much anti-thread (because threads bury important information).

I’ve seen two essentially unrelated aspects driving this preference:

  • Membership: Channels with most members having actively used IRC for decades are used to mentally disambiguating intermingled conversational threads in a single channel and don’t want to click into threads to see important content, whereas channels with most members having lived in email expect conversations to be threaded and to click into conversations they care about.
  • Purpose: Channels with an announcement business purpose tend to be aggressively threaded, but channels with research or collaboration purposes are often intentionally not threaded because threads hide information unless you notice them and click into them.

If you’d like to have some common syntax for rule options, I’d think that Rule could be generally expanded so that it has an :options value which, like :tags, is an array. I suppose it might make sense to take a page from command-line shells, so that /discourse [watch|follow|mute] -something -here sets :options to ['something', 'here'] — reserving a leading - to introduce an option. Then it would be /discourse watch my-category-slug #tagged-foo -threaded. I don’t know how much more work that would be over what I already did. Do you have strong feelings one way or the other here? I can see arguments both ways: adding another value to Rule makes it more complicated to write a chat provider; adding another filter value makes another slack-specific feature (like posting a transcript) that makes the interface more complicated for non-slack users. Of course I might be missing something that would make it harder than I think; for example, if a category slug could start with - and it is suddenly ambiguous; the shell uses -- to mean “everything after this is not an argument” but perhaps we could invert it to -- means “everything after this is an option.” If you’d like me to look into this, please give me some direction on syntax that you would find appropriate for specifying rule options. But just adding thread seems simpler to me, so absent specific direction to add a general “options” feature, I’ll wait to make any change in that direction.

[Edit: Changing from a rule filter to an option would definitely complicate the UI which would have to grow to general support for general options and that doesn’t feel like a great choice to me. So now, having looked at UI, I’d prefer to stick with the filter; I think it’s cleaner.]

For the post transcript flow, thanks for the HTML comment idea. For my use case it’s not going to hurt me any; I’m honestly expecting to be the primary user anyway, at least initially. That’s a nice simple hack.

I have a separate idea, not to embed in this PR, that it would be nice to have a mapping from the channel for the message being posted to the category in which the resulting discourse post should be made. For that to happen, I think that the transcript would best change to have an envelope or sidecar, at which point the envelope or sidecar could have both the category and the slack ID. That looks like a substantial amount of learning for me, since I’m still in the “advanced googling the problem” stage of learning Ruby on Rails. :wink:

2 Likes

One other idea for gold-plating this feature:

What if there were another setting, cross_post_to_channel_after_hours (default 48?) or something, where if the time since the last reply is greater than x hours, the post is sent the the thread with the “also send to channel” flag.

Don’t take my suggestion too seriously right now if you just want to get things working without it first (probably a better strategy!). Just food for thought…

Not at all a crazy idea but I’m not planning to implement it, since it would break my main use case and I have no need for it anywhere else. :slight_smile: I totally see that other folks would want it though!

If it were a normal setting rather than a per-channel option, it would be chat_integration_slack_copy_threads_to_channel_after_hours which would default to 0 (don’t send) but you can set it to any value. It is conceptually fairly simple but requires more work because you have to go grab the thread (using the code initially written for transcripts, and I don’t know off the top of my head whether it would require refactoring) in order to make a decision about setting a simple flag.

But if you want to work on it, I hope that what I’m doing would provide a good framework. I would just ask that you not turn it on by default because if that hit in an upgrade and my users were “spammed” by me curating content to discourse, I’d have unhappy users.

1 Like

To simplify things, I think you could just check the date of the previous post in the Discourse topic instead.

(It would behave a bit differently in some cases, but could be a fine place to start)

@david I have a draft PR up that passes unit tests as I’ve written them. I haven’t yet tried running it live against slack so I wouldn’t want to merge it until that’s done. But at least all the unit tests pass for me, and I tried to add meaningful tests for them.

I also put the ID comment at the end instead of the beginning of the post because it felt more likely to survive and less likely to be mangled there, and I tried to be conservative about parsing it out.

Thanks for your work on this plugin!

I fixed my initial linting failures, but now tests I didn’t touch are failing. I’m based off latest master, and tests pass locally. Do you have insight into the failing tests?

4 Likes

I’ve cleaned up the PR a lot but it’s not quite yet ready to go. I have two or three things so far causing me trouble that I don’t yet know how to fix.

  1. I’m trying to use fa-arrow-circle-o-right as my thread icon, and it is showing up empty in the UI on my live site. (I’ve been running su discourse -c 'bundle exec rake assets:precompile' && sv restart unicorn after checking out my branch on my live site to test on the live server.) I’ve added it to plugin.rb as well as referencing it, so I’m lost on what next steps are. Is there a list of fontawesome icons that are approved for use in Discourse? Found lib/svg_sprite/svg_sprite.rb and chevron-right looks great for this use.

  2. Tests are all passing for me locally, but in Travis I’m getting consistent errors that appear unrelated to my changes, and naturally that’s hard for me to investigate or reason about. 13 failures with a 404 instead of some other expected (e.g. 200) in spec/lib/discourse_chat/provider/slack/slack_command_controller_spec.rb Fixed by not cargo-coding isolate_namespace and now I know about rake routes

I’ve successfully posted:

There might be more to clean up, but I think this works.

After this merges, I’ll update Chatroom Integration Plugin (discourse-chat-integration) appropriately.

2 Likes

I keep finding new opportunities to learn here. Now I know how to run yarn prettier, next I get to learn how to update front end tests…

Error: Assertion Failed: You must use set() to set the `channel` property (of <(unknown):ember3806>) to `<(unknown):ember3799>`.

Thanks so much for all your work here @mcdanlj - this is now merged:

4 Likes

I appreciate your very helpful reviews! I updated the official integration wiki post as promised. If you’d prefer to call out the changes in other ways I’m cool with that, no pride of authorship or anything like that. :slight_smile:

3 Likes

Could threading please be enabled within chat-integration Settings for Slack ala

Enable threading of Posts to Slack

You mean, you want to be able to configure the plug-in to refuse to honor the thread option in the integration?

We updated to current 2.6 beta1a tests-pass few days ago, but as far as I can tell no slack x-posts are threaded since the upgrrade. We still see topic posts and replies individually displayed on Slack channels.

Are you saying that you previously had rules that threaded, and with the most recent update you did, it is no longer threading? Or is it that you expected with the update that it would start threading them and it is not doing so?

I’m running discourse 093ee1d80c and discourse-chat-integration da91061 (current up-to-date) and my channel with thread rules is correctly threading responses, only for channels set up with thread rules.

Can you show your configuration for a thread rule? In Admin → Plugins → Chat Integrations you should see rules that say

image

and if you click on the edit pencil you should see

image

If you have rules that instead say:

image

then those rules won’t use threads.

You’ll see above the rationale for this being a per-rule, not per-site, configuration.

When you set up a rule from the /discourse command in slack (or whatever command you chose when you set up the integration), you use thread instead of watch or follow, as documented in Chatroom Integration Plugin (discourse-chat-integration)

2 Likes

Okay, so I need to go through Slack integrations and change every single instance of:
All Posts and Replies < All posts with threaded replies.

So far we are displaying all posts and replies on most channel integration. Will it screw anything up to just display all posts with threaded replies instead moving forward? I also ask because we have a lot of channels for me to re-configure, so best for me to re-assign it properly the first time. :+1:

If I understand your question: No, it won’t “screw anything up” — it’s meant to be safe in that it will never prevent sending a new notification to slack, it’s just that if the integration knows about a thread, it will send it to the thread instead of to the channel. If it’s not aware of a thread context for any reason, then it will send to the channel as configured in the rule.

“All posts with threaded replies” means that:

  • When a new post is added to an existing topic:
    • If a thread ID has been stored for the topic, use it to post a thread
    • If no thread ID has been stored for the topic, after posting the notification to slack, use that new slack post’s thread ID for further posts on the topic. It will start threading from that point on.
  • When a new topic is posted to slack, store its resulting thread ID so that additional posts on that topic are sent to slack as threaded responses.

I summarize that as “act just like watch, except if it knows about a thread to post to, do that instead”

Additionally, when using the “transcript” feature to post slack content as a new topic in Discourse, regardless of any rule settings, it always tries to store the thread ID, so that if a thread rule either already exists or is added in the future, post replies on the new topic in discourse are announced on the appropriate slack thread.

I’m sure your existing rules could be changed with some bin/rails c typing but I don’t want to do that to my live site where I’ve intentionally chosen which channels to thread and which not to, and I’m way too new to ruby to type random ruby as help advice on a forum and expect it not to blow up in your face. :sadpanda: Other than it probably starts with DiscourseChat::Rule.where( I’m not going to be much help. Sorry!

4 Likes

@sunjam by the way, I appreciate the validation that you found this functionality desirable and valuable! (Especially given the irony that I don’t particularly like slack threads myself and did this work for others who find them more valuable than I do!)

I can imagine that it would make sense to add a button to the UI to convert all watch rules to thread rules. However, I don’t actually know enough to do it and wouldn’t use it myself. I’m really a back end developer dabbling in discourse, so I wouldn’t even be a useful reviewer for a PR to add such a button. All I can do is be a feckless cheerleader if someone else wants to implement such functionality. :slight_smile:

1 Like

I found an issue @mcdanlj. When creating a new channel integration: threaded replies does not show up on 2.6 beta1 tests-pass for filters. Once the integration is created, it becomes an option by editing the integration.

I see the same thing now; I hadn’t even noticed the UI for that and I created my rules with slash commands from slack.

To the extent of my limited understanding of front end code, I think this is an artifact of the code @david asked for to hide thread from other integration types:

  @computed("channel.provider")
  available_filters(provider) {
    const available = [];

    if (provider === "slack") {
      available.push({
        id: "thread",
        name: I18n.t("chat_integration.filter.thread"),
        icon: "chevron-right"
      });
    }

I could be wrong.

But I’m really a backend developer and I don’t know how to fix this. I don’t know why the channel.provider would be slack only when editing an existing rule, and not when creating a new one. :grimacing:

I think this should do it:

Let me know if it works for you :slight_smile:

3 Likes

I confirm that it is fixed for me. Thanks @david for cleaning up after what I didn’t know about!

1 Like