Slack Event API Support v2 #89
Conversation
- abstracted out a BaseSlackHandler class which the SlackEventHandler & SlackHookHandler extend. - fix bug with room linking - enable slack msg transformations in SlackEventHandler - provide better file bridging support from slack -> matrix
Half-Shot
force-pushed
the
hs/perissology/slack-events
branch
2 times, most recently
from
4c537ce to
1529a6f
Compare
Half-Shot
force-pushed
the
hs/perissology/slack-events
branch
from
1529a6f to
73814f9
Compare
|
Unassigning @matrix-org/bridge-team while there are still bugs present. |
|
Bugs were fixed in matrix-org/matrix-spec-proposals#93 because they were slightly unrelated, this just needs a review now. |
There was a problem hiding this comment.
This looks largely okay. I'm assuming it has generally been tested, although there's two specific scenarios I'd like tested before this is merged (as mentioned oob):
- Starting with a bridge that doesn't have this PR, bridge a room. Then upgrade. Does the bridge still work for that room?
- When using this PR, does the provisioning API support using the old bridging method?
README.md
Outdated
| You will also need to determine the "channel ID" that Slack uses to identify | ||
| the channel, which can be found in the url `https://XXX.slack.com/messages/<channel id>/`. | ||
|
|
||
| 2. Issue a ``link`` command in the administration control room with these |
There was a problem hiding this comment.
if we're going to duplicate list item numbers, can we use 1. please?
lib/AdminCommands.js
Outdated
| @@ -131,20 +131,31 @@ adminCommands.link = new AdminCommand({ | |||
| required: true, | |||
| }, | |||
| webhook_url: { | |||
| description: "Slack webhook URL", | |||
| description: "Slack webhook URL. Used with slack outgoing hooks integration", | |||
lib/AdminCommands.js
Outdated
| aliases: ['u'], | ||
| }, | ||
| slack_bot_token: { | ||
| description: "Slack bot user token. Used with slack bot user & Events api", |
lib/BaseSlackHandler.js
Outdated
| const CHANNEL_ID_REGEX = /<#(\w+)\|?\w*?>/g; | ||
| const CHANNEL_ID_REGEX_FIRST = /<#(\w+)\|?\w*?>/; | ||
|
|
||
| // (if this is an emote msg, the format is <@ID|nick>, but in normal msgs it's just <@ID> |
There was a problem hiding this comment.
the comment references 'this' which makes no sense up here
lib/BaseSlackHandler.js
Outdated
| message.text = message.text.replace(CHANNEL_ID_REGEX_FIRST, "#" + name); | ||
| iteration++; | ||
| }).catch((err) => { | ||
| console.log("Caught error " + err); |
There was a problem hiding this comment.
also helpful error messages would be awesome :)
lib/BaseSlackHandler.js
Outdated
| } | ||
| iteration++; | ||
| }).catch((err) => { | ||
| console.log("Caught error " + err); |
There was a problem hiding this comment.
can we also get a more descriptive error message on this?
lib/BridgedRoom.js
Outdated
| if (message.text) { | ||
| var text = substitutions.slackToMatrix( | ||
| message.text | ||
| ); |
There was a problem hiding this comment.
surely this isn't hitting a line length limit if line 286 can be half a kilometre long
Tested both today and they work flawlessly, I was kinda surprised 😆 |
This is a follow-on from matrix-org/matrix-spec-proposals#66 with a few changes to tidy it up a bit. We are also now targetting the develop branch as these changes are experimental.
Goes without saying that @perissology did a huge amount of work in this and credit should go to them for the actual implementation.