Skip to content
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

[api] Implement ScopedRateThrottle by default #112

Closed
wants to merge 3 commits into from

Conversation

PabloCastellano
Copy link
Contributor

@PabloCastellano PabloCastellano commented May 20, 2020

This PR is part of openwisp/openwisp-firmware-upgrader#48

Note that by using ScopedRateThrottling it requires us to define a default anon class throttle for the unauthenticated API requests.

@PabloCastellano PabloCastellano force-pushed the api-scopedratethrottle branch 2 times, most recently from 19b245c to 614107d Compare May 20, 2020 18:50
Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

I like the idea.

What's the purpose of having the code in /test_project/apps.py? That code is not called from the other modules. I think we should put it in /openwisp_utils/api/apps.py or something like that.

See my other comments below.

tests/test_project/apps.py Show resolved Hide resolved

def add_default_menu_items(self):
menu_setting = 'OPENWISP_DEFAULT_ADMIN_MENU_ITEMS'
items = [
{'model': 'test_project.Shelf'},
]
setattr(settings, menu_setting, items)

def configure_drf_defaults(self):
Copy link
Member

Choose a reason for hiding this comment

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

I'm also thinking that this method could be part of a mixin which we could import in the other modules so we don't need to redefine it each time, we will just add the mixin to the AppConfig class, or alternatively we can define a function and call that function in the modules (instead of using the mixin, which some people don't like, I'm fine with both approaches).

tests/test_project/apps.py Outdated Show resolved Hide resolved
@coveralls
Copy link

coveralls commented May 22, 2020

Coverage Status

Coverage decreased (-4.2%) to 95.238% when pulling 83232f7 on api-scopedratethrottle into ea7f7a6 on master.

Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

@PabloCastellano when the code coverage drops so much, it means the code being added is not being used, which seems to be the case.

Leave this to me, I will improve it.

NoumbissiValere pushed a commit that referenced this pull request Jul 27, 2020
@atb00ker atb00ker deleted the api-scopedratethrottle branch August 9, 2020 15:32
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.

3 participants