Enable optional support for threading slack posts

[johnsonm]

In general, this is meant to implement the feature described at https://meta.discourse.org/t/optionally-threading-posts-to-parent-topic-in-slack-integration/150759 but also includes some cleanups. A few lines of unused or redundant code are cleaned up, and the slack API tests are cleaned up to more accurately reflect the slack API. Requesting a thread does not return arbitrary messages outside the thread; the tests have been modified to more accurately represent this. ts values are promised to be unique within a channel, so a repetition is changed to follow this contract.

[davidtaylorhq]

I like the design here - just added a few specific comments.

A few other general questions/concerns:

• How does this work for non-slack providers? I don't think we want them to have 'thread' rules, because it won't do anything. Maybe we need a boolean on each Provider implementation?
• I don't see any javascript changes here. How does this look in the admin UI?

[johnsonm]

How does this work for non-slack providers? I don't think we want them to have 'thread' rules, because it won't do anything. Maybe we need a boolean on each Provider implementation?

thread just means "prefer a thread where a threaded context exists" and is otherwise a synonym for watch, so for any non-slack providers it would be equivalent to watch until such time as any other chat platform implements threads and the integration is updated, at which point it would just start preferring threads.

If I instead implemented a whole new options beside filter as discussed on meta, it would have exactly the same semantics of just "prefer threads if possible".

I don't see any javascript changes here. How does this look in the admin UI?

No idea yet. Still draft PR because I haven't done anything more than get unit tests going; draft PR because I was looking for feedback before going further. I also haven't touched the help message that shows up from /discourse help which will also need to be done.

Thank you so much!

[johnsonm]

If I instead implemented a whole new options beside filter as discussed on meta, it would have exactly the same semantics of just "prefer threads if possible".

To implement options would require a lot more admin UI that I'm not even sure what would make sense for it to look like, and would invite more complexity. The more I looked into that, the more I prefer the simple filter option as implemented.

I have tested this successfully on a live site in production use; screen shots on the meta thread. I doubt it's perfect but I think it is usable now.

Thank you so much for the help!

[davidtaylorhq]

This is looking great!

To implement options would require a lot more admin UI that I'm not even sure what would make sense for it to look like, and would invite more complexity. The more I looked into that, the more I prefer the simple filter option as implemented.

Agreed, I like having thread as a filter. My concern is that "thread" appears as a filter for all providers, even if they don't support threading. I understand that it's a synonym for watch, but I think it could still be confusing for people.

Can we hide this filter from the UI for non-slack providers?

[johnsonm]

Can we hide this filter from the UI for non-slack providers?

This is where I admit openly to being a back end engineer. I'm sure all things are possible, but I might need a pointer for how to go about implementing this restriction. I intentionally didn't add it to the help messages for providers other than slack, and had honestly forgotten that there even was a UI in the admin console until you asked about it earlier. Is there an example in the plugin somewhere of restricting UI elements by provider type from which I can learn?

[davidtaylorhq]

I see that you added some thread to the available_filters list in rule.js.es6. I think what we need to do is make that available_filters list dynamic. So rather than

available_filters: [
  ...
],

we would have

@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"
    });
  }

  available.push({
     // Watch
  },
  {
    // Follow
  },
  {
    //Mute
  });

  return available;
}

(not tested, there may be some syntax errors)

Let me know if that approach doesn't work - I'm happy to take a look in more detail and get it sorted.

[johnsonm]

• Good news: I learned how to use yarn prettier
• Better news: I deployed to my live site and I still see my new thread setting with chevron in the slack integration admin UI. I don't have other integrations to test live, but I don't see how it would show threads for them with the explicit guard on the provider.
• Bad news: Travis seems to have failed in what looks to me like a transient way and I don't think I have permissions to ask Travis to kindly try again. @davidtaylorhq is this something you have permissions to re-run? I updated my commit message and ran again, and this time got meaningful error messages and can reproduce qunit errors locally.

[davidtaylorhq]

can reproduce qunit errors locally

Great! At a glance, the error is not directly related to your change. But now that we're depending on channel for a computed property, there are restrictions on how we change its value. We need to track down places where we do rule.channel=value and change it to rule.set('channel', value). I am guessing this is the main one, but it's possible there are more.

[johnsonm]

I looked (thank you git grep) and couldn't find any more, and fixing that one instance gave me qunit tests passing locally.

I had another of the Travis failures that seems random, so I pushed an edit to the commit message to kick off another Travis run. I missed that prettier wanted " instaed of ' the first time around.

[johnsonm]

Tests pass. I have re-deployed on my live site and confirm UI is still working. As far as I know, this is ready to merge. 🎉

[johnsonm]

@davidtaylorhq — github still shows "1 change requested" which seems to be from your overall comment in your initial review, and I can't find a way to mark it responded to, even though both of those points have definitely been addressed in following commits. I don't know whether you need to do something to clear that status.

[davidtaylorhq]

Yeah that is leftover from the previous review. This is all looking great - thanks for all your work. Will merge it in now 😁