-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Limit /check_api
Invocations to Once Every 15 Minutes per Server
#32
Limit /check_api
Invocations to Once Every 15 Minutes per Server
#32
Conversation
see fastapi/fastapi#8187 for the discussion.
|
||
DB_PATH = os.path.abspath(os.environ.get("DB_PATH", "./slack-events-bot.db")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved the DB-related calls to the database.py file to simplify testing a bit (so I could create a fixture to clean the DB between tests).
|
||
cur.executescript( | ||
""" | ||
SELECT 'DELETE FROM ' || name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I debated making this an ``autouse=true` fixture, but for now I've elected to keep it so that it needs to be manually included into tests. It'll wipe all of the rows from all tables at the end of a test (the schema is left intact, though).
# | ||
# If we get some response other than RATE_LIMIT_COPY that means rate-limiting is not | ||
# occurring, so this is at least targeting that aspect (though, sloppily). | ||
assert response.content.decode("utf-8") != RATE_LIMIT_COPY |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still working through how to stub out the actual Slack bits in the test suite. It's making things a bit interesting in the meantime. 😅
I'd love to come back and refactor this to make some more explicit assertions once any interaction with Bolt doesn't cause a 401 error to be raised.
Just realized datetime.datetime.now() isn't timezone-aware.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested this on an environment from dev and it works great! The database changes make things a lot cleaner and this is a great feature to have, thank you!
Thank you for reviewing, @oliviasculley! |
Fixes #5
Summary
This change only allows for the
/check_api
command to be run on a server/workspace every 15 minutes. If a user from a workspace that has run this command within 15 minutes runs it again, they'll be greeted with this message:Cooldowns are specific to each workspace, so users from one server aren't subject to a cooldown being enforced against another.
I've also extracted out the instantiation of the SQLite connection(s) from bot.py to database.py to make testing database-dependent functionality a little bit easier.
Screenshots
A record in the
cooldowns
table where each server (or anything whose ID we'd want to put here) has a timestamp denoting when someone from there can run/check_api
again.What end users see if they hit the rate-limit.
Test Plan
/check_api
command in a workspace that your bot is integrated with.cooldowns
table.accessor
field will be the subdomain related to your workspace (<accessor>.slack.com
).resource
field will becheck_api
.expires_at
time will be a timestamp in ISO8601 that should be the time 15 minutes from whenever you executed the/check_api
command (in UTC).expires_at
timestamp to be an hour behind what it's currently set to and commit your change. Then try running the command again (it should work).I think it would also be good to test the other end-user-facing functionality (like
/add_channel
and/remove_channel
just to ensure the DB refactor didn't booger anything up.