Rate Throttling #421
Rate Throttling #421
Conversation
✅ Deploy Preview for activist-org canceled.
|
Thank you for the pull request!The activist team will do our best to address your contribution as soon as we can. The following is a checklist for maintainers to make sure this process goes as well as possible. Feel free to address the points below yourself in further commits if you realize that actions are needed :) If you're not already a member of our public Matrix community, please consider joining! We'd suggest using Element as your Matrix client, and definitely join the General and Development rooms once you're in. It'd be great to have you! Maintainer checklist
|
|
Thanks for this, @sulabhkatila! Will try to get to this review in the coming days 😊 |
backend/content/tests.py
Outdated
| @@ -1,3 +1,40 @@ | |||
| from django.test import TestCase | |||
There was a problem hiding this comment.
Quick note @sulabhkatila: we also need to import override_settings here and in the other files :) I can get this for you post merge! 😊
|
@sulabhkatila, @to-sta and I are looking into this right now. Tests are running within the Docker backend, but we're getting errors on our end. We'll be in touch later :) |
There was a problem hiding this comment.
@andrewtavis i would suggest we only add the additons for the REST_FRAMEWORK dict. in settings.py for now and address the test in #432.
| }, | ||
| "test": { | ||
| "ENGINE": "django.db.backends.postgresql", | ||
| "NAME": "test_" + DATABASE_NAME, | ||
| "USER": DATABASE_USER, | ||
| "PASSWORD": DATABASE_PASSWORD, | ||
| "HOST": DATABASE_HOST, | ||
| "PORT": DATABASE_PORT, | ||
| }, | ||
| } |
There was a problem hiding this comment.
I would say for now, we do not need to explicitly create a test database, since Django does that by default. Here are the paragraphs from the docs:
Tests that require a database (namely, model tests) will not use your “real” (production) database. Separate, blank databases are created for the tests.
...
The default test database names are created by prepending test_ to the value of each NAME in DATABASES.
We might start with the TEST key inside the database. But we will address that in the issue #432.
| @@ -44,6 +44,7 @@ | |||
|
|
|||
| INSTALLED_APPS = [ | |||
| "rest_framework", | |||
| "rest_framework.authtoken", | |||
There was a problem hiding this comment.
Do we need this for user-based throttling? I think we can get rid of the authtoken and address this in a seperate issue.
backend/content/tests.py
Outdated
There was a problem hiding this comment.
I would say we address the tests in #432 rather than implementing them now. @sulabhkatila i have seen you put a lot of work in there 🚀 but there are still issues, that we might be able to address in a more standardize way with a thrid party package like factory_boy. For example when you create an object that has a foreign key you have to create the foreign key object.
There was a problem hiding this comment.
@andrewtavis, @to-sta Thank you so much for your feedback. I will also do more research and see if I can make the needed changes to the tests.
There was a problem hiding this comment.
@sulabhkatila we will be merging your PR, without the tests for now. We will picking up the tests for Rate Throttling in another issue soon. You're welcome to work on it if you want 😃
There was a problem hiding this comment.
Thanks for the contribution here, @sulabhkatila! Appreciate you taking this on and attempting the tests as well :) We'll get to them in another issue, and as @to-sta said you'd be welcome to work on that 😊
Contributor checklist
Issue
Related issue