Skip to content
This repository has been archived by the owner on Jun 30, 2022. It is now read-only.

Subteam mentions #64

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

erikogan
Copy link
Contributor

@erikogan erikogan commented Dec 4, 2015

Slack supports links to user-groups/sub-teams, of the form: <!subteam^link|@team_name>. This seems to be the least invasive method to support this in a way that has a chance of supporting similar links in the future.

Thanks!

Put type back as the single character, and add a subtype match, which
does not include the carat.
@jimmycuadra
Copy link
Contributor

@paulhammond Does this seem like a reasonable approach? hubot-slack doesn't seem to do this yet.

@erikogan
Copy link
Contributor Author

erikogan commented Dec 4, 2015

Perhaps the pattern for the subtype should be [^^]+ instead of \w+? Just in case other punctuation ends up in those identifiers.

(Also: Hi, @paulhammond.)

@paulhammond
Copy link
Contributor

Hi!

So this command is documented at https://api.slack.com/docs/formatting:

For paid teams there is an additional command for user groups that follows the format <!subteam^ID|handle>. These indicate a user group message, and should cause a notification to be displayed by the client. User group IDs can be determined from the usergroups.list API endpoint.

I can tell you that the the subteam IDs will always be alphanumeric in this case. So your code will handle the subteam command just fine.

But I think it's worth drawing your attention to the next paragraph in the docs where we say we might be adding new commands in the future (where commands are things starting with a !). I know there's at least one more we're about to document, and that uses a lot of non-letter characters after the ^, and even has more than one ^. All of these new types will include labels (because we need to maintain some level of backwards compatibility with existing mobile clients) so the approach you have will almost work, except for the multiple ^ thing.

Here's a test case that includes most of the edge cases you might need to consider:
<!foo^C12345^{foo}^http://www.example.com/|label>

(Oh and I'm moving onto a different team at Slack in the near future. Jimmy, I'll introduce you to some other people on the platform team who can help you with these kinds of questions from here on)

@jimmycuadra
Copy link
Contributor

Thanks very much, Paul!

@erikogan
Copy link
Contributor Author

erikogan commented Aug 1, 2018

@jimmycuadra I just discovered that we’re pinned to my branch for this PR, and we need some of the newer features. I’m going to resurrect it and attempt to handle @paulhammond’s example case above. I’ll review the updated docs, but is there anything you know I need to watch out for?

EDIT: Looks like the one I should specifically test is dates.

(Also, this should fix #124)

@paulhammond
Copy link
Contributor

Wow, it’s been a while! Yes, the thing we were about to launch that I was referring to above was <!date>, docs for that are at https://api.slack.com/docs/message-formatting#formatting_dates

… commands

like <!date…> commands as well as @paulhammond’s generic example (that was probably hinting at <!date…>)
@erikogan
Copy link
Contributor Author

erikogan commented Aug 1, 2018

@jimmycuadra I opted to swap values in a few conditionals after the regexp replace in favor of making the regexp quite a bit more complex trying to get them right with lookaheads. I think that would likely make the regexp inscrutable. Let me know if you’d prefer it that way.

@erikogan
Copy link
Contributor Author

erikogan commented Aug 1, 2018

Hmm. That spec also failed locally, but was far away from my changes, and I presumed it was my environment. I’ll take a quick look at it to see if I can figure out why it is failing. (I suspect it’s something to do with a newer version of EventMachine)

@erikogan
Copy link
Contributor Author

erikogan commented Aug 1, 2018

The spec fails non-deterministically. rspec --seed 0 ./spec/lita/adapters/slack/rtm_connection_spec.rb:119 will fail about 20% of the time. But it clumps in weird ways, too.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants