Skip to content

Conversation

@mythmon
Copy link
Contributor

@mythmon mythmon commented Feb 11, 2015

This converts all the rate limiting we have to use two related functions kitsune.sumo.decorators.ratelimit and kitsune.sumo.utils.is_ratelimited. These work largely like their django-ratelimit versions, and in-fact use django-ratelimit themselves, except they are simplified and specialized to our needs. In particular, they respect a permission for bypassing rate limits, so we can have a global switch for this, and they all log when a user is over the rate limit.

The rate limit overages are recorded in kitsune.journal.models.Record, which is loosely based off fjord.journal.models.Record, which @willkg wrote. I modified it a lot, since I don't think we needed everything Fjord needed.

This isn't done yet

  • Make new ratelimit decorator.
  • Make new is_ratelimited function.
  • Add "logging".
  • Add the permission some how (I think this will be useful).
  • Test the permissions
  • Test the logging

f?

@mythmon
Copy link
Contributor Author

mythmon commented Feb 12, 2015

All the boxes above are checked. I think this is ready to go. r?

@rlr
Copy link
Contributor

rlr commented Feb 12, 2015

but that test fails. gah!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yay! so much cleaner

@mythmon
Copy link
Contributor Author

mythmon commented Feb 12, 2015

Oops, I forgot to git add the migration that creates the permission. That is why the test failed.

@mythmon
Copy link
Contributor Author

mythmon commented Feb 12, 2015

🎆

rlr added a commit that referenced this pull request Feb 12, 2015
[Bug 1125536] Convert all rate limiting to a standard helper.
@rlr rlr merged commit 557e748 into mozilla:master Feb 12, 2015
@rlr
Copy link
Contributor

rlr commented Feb 12, 2015

yay 💥

@mythmon mythmon deleted the track-ratelimit-1125536 branch February 12, 2015 21:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants