Migrate to Slack's Events API - #418
Conversation
…till not connecting to heltour
…it stopped me from testing
lakinwecker
left a comment
lakinwecker
left a comment
There was a problem hiding this comment.
Some small changes to consider.
| "codeclimate-test-reporter": "^0.5.1", | ||
| "dotenv": "^8.2.0", | ||
| "ee-ts": "^1.0.1", | ||
| "f": "^1.4.0", |
There was a problem hiding this comment.
This library was last published 8 years ago, and the github repository no longer exists for it, meaning it's a ripe for attack, do we need it? Is there not another library we can use instead?
There was a problem hiding this comment.
It doesn't look like we use it, we should remove it
| clock: { | ||
| initial: 45, | ||
| increment: 45, | ||
| initial: 1, |
There was a problem hiding this comment.
It doesn't matter too much, but probably these shouldn't change
| @@ -0,0 +1,629 @@ | |||
| [ 0.000000] Linux version 4.15.0-212-generic (buildd@lcy02-amd64-083) (gcc version 7.5.0 (Ubuntu 7.5.0-3ubuntu1~18.04)) #223-Ubuntu SMP Tue May 23 13:09:22 UTC 2023 (Ubuntu 4.15.0-212.223-generic 4.15.18) | |||
There was a problem hiding this comment.
This file should not be added
| "q": "^1.5.1", | ||
| "sequelize": "^5.21.5", | ||
| "sequelize-cli": "5.5.0", | ||
| "sqlite3": "^5.1.7", |
There was a problem hiding this comment.
We don't depend on this in prod, so probably best for us to remove this as well
| }) | ||
| .then(() => | ||
| resolve( | ||
| .then(([subscription, created]) => { |
There was a problem hiding this comment.
The subscription/created parameters here are not necessary, are they?
| // here. | ||
| _.map(listener.middleware, (m) => m(this, message)) | ||
| // SAME FIX HERE | ||
| this.log.info( |
There was a problem hiding this comment.
This fix makes no sense, same as before, do you know why?
| // Find command listeners first | ||
| for (const listener of this.listeners) { | ||
| if ( | ||
| listener.type === 'command' && |
There was a problem hiding this comment.
Minor nit: if you invert the condition in this if, and you use continue instead, you can un-indent the following code and it makes things easier to understand
| ) | ||
| if (options.event === 'member_joined_channel') { | ||
| this.app.event('member_joined_channel', async ({ event }) => { | ||
| // @ts-ignore hope I don't regret this lol |
There was a problem hiding this comment.
Yeah, I hope we don't too
| @@ -0,0 +1,218 @@ | |||
| // ----------------------------------------------------------------------------- | |||
There was a problem hiding this comment.
Is this actually used aside from the tests? If not, let's remove it.
There was a problem hiding this comment.
We can put it back in with a PR that causes it to be used, this sort of refactoring and tests is quite a good step towards what we want, but I don't like having unused code in master
| clock: { | ||
| initial: 30, | ||
| increment: 30, | ||
| initial: 1, |
There was a problem hiding this comment.
Yeah, all of these changes should be ignored. this config is setup to test on the live slack
There was a problem hiding this comment.
You can feel free to make and commit another config that you use for your slack, if you'd like
2 participants
Migrating to Slack's Events API.
The problem this solves:
-Previously, chesster ran on Slack's RTM API
-But, the RTM API will be shut down on April 1, 2025
-So we're migrating chesster and adminSlack to the Events API
Details:
-Updated environment variables in code and README
-Updated chesster.start() to connect using the new API
-Updated various listeners and utils to be compatible with the change
QA:
-Tested for all three leagues for these items:
-Chesster listens in #-scheduling and updates scheduled times accordingly
-Chesster picks up on game starts and results in #-games
-Chesster responds appropriately to its normall array of commands (
source,rating, summon mods etc)-Chesster forwards messages
-Some other stuff I'm probably forgetting
Sticking points:
-
@rating(without a specified username) crashes chesster if the sender of the message isn't in the db-My local heltour was only sending one message at a time to be forwarded, so I don't know for sure if Chesster will forward bulk messages
TODO:
This was done in a fairly intense time crunch because I only found out about the RTM API deprecation in late February. Once things are settled down, I'd like to:
-Abstract chesster stuff in to utils functions, particularly
this.app.event...andthis.message...processing of messages to Chesster-Write unit tests for the utils. Test the logic, not the API