Skip to content

Throttle username logins #83

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.

Sign up for GitHub

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

Jump to bottom
Merged
meshy merged 6 commits into from
Nov 25, 2014
Merged

Throttle username logins #83

meshy merged 6 commits into from
Nov 25, 2014

Conversation

@mlen108
Copy link
Contributor

@mlen108 mlen108 commented Nov 25, 2014

No description provided.

@mlen108
Copy link
Contributor Author

mlen108 commented Nov 25, 2014

Tests fail for python3.3 - I think this project needs tox..

@coveralls
Copy link

coveralls commented Nov 25, 2014

Coverage Status

Coverage decreased (-0.2%) when pulling 2c78f2a on throttle-expanded into 3a110c9 on master.

LilyFirefly
LilyFirefly reviewed Nov 25, 2014
View reviewed changes
user_management/api/tests/test_throttling.py Outdated
Copy link

@LilyFirefly LilyFirefly Nov 25, 2014

Choose a reason for hiding this comment

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

I think you might need to move this into setUp and use reverse instead of reverse_lazy.

Copy link
Contributor Author

@mlen108 mlen108 Nov 25, 2014

Choose a reason for hiding this comment

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

I think this PR isn't ready for reviews yet.

Copy link

@LilyFirefly LilyFirefly Nov 25, 2014

Choose a reason for hiding this comment

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

I'm just suggesting a solution for your test failures.

Copy link
Contributor Author

@mlen108 mlen108 Nov 25, 2014

Choose a reason for hiding this comment

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

Hmm seems like it's the cause - thanks!

@mlen108
Copy link
Contributor Author

mlen108 commented Nov 25, 2014

@ian-foote @meshy review?

@coveralls
Copy link

coveralls commented Nov 25, 2014

Coverage Status

Coverage decreased (-0.2%) when pulling 30c23e7 on throttle-expanded into 3a110c9 on master.

meshy
meshy reviewed Nov 25, 2014
View reviewed changes
user_management/api/tests/test_throttling.py Outdated
Copy link
Contributor

@meshy meshy Nov 25, 2014

Choose a reason for hiding this comment

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

Could this be explained with a comment/docstring, please?

Copy link

@LilyFirefly LilyFirefly Nov 25, 2014

Choose a reason for hiding this comment

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

👍 docstring.

Copy link
Contributor Author

@mlen108 mlen108 Nov 25, 2014

Choose a reason for hiding this comment

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

Updated

@meshy
Copy link
Contributor

meshy commented Nov 25, 2014

Looks good to me

LilyFirefly
LilyFirefly reviewed Nov 25, 2014
View reviewed changes
user_management/api/tests/test_throttling.py
Copy link

@LilyFirefly LilyFirefly Nov 25, 2014

Choose a reason for hiding this comment

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

I would just use status.HTTP_429_TOO_MANY_REQUESTS directly. I don't think this is any clearer and it requires looking in more places.

@meshy
Copy link
Contributor

meshy commented Nov 25, 2014

Hm... coveralls is reporting that coverage has decreased...

@mlen108
Copy link
Contributor Author

mlen108 commented Nov 25, 2014

@meshy the line it complains about if it's your concern.

@LilyFirefly
Copy link

LilyFirefly commented Nov 25, 2014

Test an authenticated user. If the line isn't hit then it doesn't need to be there.

@mlen108
Copy link
Contributor Author

mlen108 commented Nov 25, 2014

If the line isn't hit then it doesn't need to be there.

It is needed there. Will add additional test then.

@mlen108
Copy link
Contributor Author

mlen108 commented Nov 25, 2014

Can this be merged? The coverage is 100% (well, 99% for Django 1.6 .. )

@meshy
Copy link
Contributor

meshy commented Nov 25, 2014

Why are those lines missed on Dj1.6?

@LilyFirefly
Copy link

LilyFirefly commented Nov 25, 2014

Because django 1.6 doesn't support checks.

@mlen108
Copy link
Contributor Author

mlen108 commented Nov 25, 2014

checks isn't part of Django 1.6 (it was introduced in 1.7 ;-) )

@meshy
Copy link
Contributor

meshy commented Nov 25, 2014

👍

meshy added a commit that referenced this pull request Nov 25, 2014
@meshy
Merge pull request #83 from incuna/throttle-expanded
914ccb5 
Throttle username logins
@meshy meshy merged commit 914ccb5 into master Nov 25, 2014
@meshy meshy deleted the throttle-expanded branch November 25, 2014 17:35
@mlen108
Copy link
Contributor Author

mlen108 commented Nov 25, 2014

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@mlen108 @coveralls @meshy @LilyFirefly