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.

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

Allow decoration of functions beyond the admin login #86

Merged
merged 4 commits into from
Jun 26, 2017
Merged

Allow decoration of functions beyond the admin login #86

merged 4 commits into from
Jun 26, 2017

Conversation

MattBlack85
Copy link
Contributor

@kencochrane as discussed in #85

@coveralls
Copy link

coveralls commented Jun 24, 2017 •
edited
Loading

Coverage Status

Coverage decreased (-0.09%) to 95.732% when pulling 5fefdb7 on MattBlack85:api_login into a59cbca on kencochrane:master.

kencochrane
kencochrane reviewed Jun 24, 2017
View reviewed changes
Copy link
Collaborator

@kencochrane kencochrane left a comment

Choose a reason for hiding this comment

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

Looks good, thank you. Only a couple of minor comments/questions

defender/tests.py
@@ -724,6 +729,81 @@ def test_disable_username_lockout(self):
data_out = utils.get_blocked_usernames()
self.assertEqual(data_out, [])

@patch('defender.config.BEHIND_REVERSE_PROXY', True)
@patch('defender.config.FAILURE_LIMIT', 3)
def test_login_blocked_for_non_standard_login_views_without_msg(self):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you add docstrings to the test methods explaining what each test does? Not all tests have them now, and I have been meaning to fix that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, no problem

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thx

defender/middleware.py
LoginView.dispatch = watch_login_method(LoginView.dispatch)
except ImportError: # Django < 1.11
auth_views.login = watch_login(auth_views.login)
auth_views.login = watch_login()(auth_views.login)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is that correct? I'm no expert when it comes to decorators so might be correct, it just looks weird with watch_login()(auth_views.login)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That looks weird but it's correct. The old decorator couldn't accept arguments and had a deep of 2. I had to add one more level in order to accept arguments so the first thing the new decorator expects now are the arguments (or the defaults) while here a function was passed in first instance. Does it make sense?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, thank you.

@kencochrane
Copy link
Collaborator

It looks like coverage dropped a little, and a few more quantified code minor issues were found. Other than that, pretty good, thanks for the PR.

@kencochrane
Copy link
Collaborator

I just merged another PR, it looks like there is a conflict.

@coveralls
Copy link

coveralls commented Jun 25, 2017 •
edited
Loading

Coverage Status

Coverage decreased (-0.2%) to 95.61% when pulling 5fefdb7 on MattBlack85:api_login into a59cbca on kencochrane:master.

@coveralls
Copy link

coveralls commented Jun 25, 2017 •
edited
Loading

Coverage Status

Coverage decreased (-0.09%) to 95.763% when pulling 5f47acd on MattBlack85:api_login into d2b712e on kencochrane:master.

@coveralls
Copy link

coveralls commented Jun 25, 2017 •
edited
Loading

Coverage Status

Coverage decreased (-5.5%) to 90.351% when pulling a4be395 on MattBlack85:api_login into d2b712e on kencochrane:master.

@coveralls
Copy link

coveralls commented Jun 25, 2017 •
edited
Loading

Coverage Status

Coverage decreased (-5.5%) to 90.351% when pulling e18c60f on MattBlack85:api_login into d2b712e on kencochrane:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-5.6%) to 90.265% when pulling 43c1074 on MattBlack85:api_login into d2b712e on kencochrane:master.

@coveralls
Copy link

coveralls commented Jun 25, 2017 •
edited
Loading

Coverage Status

Coverage decreased (-4.7%) to 91.15% when pulling 43c1074 on MattBlack85:api_login into d2b712e on kencochrane:master.

@MattBlack85
Copy link
Contributor Author

MattBlack85 commented Jun 25, 2017 •
edited
Loading

@kencochrane please review again, I added the docstrings but also 3 more commits.
One is excluding test files from coverage which is dropping coverage by 5% but it gives real results now. Second one is something I discover yesterday while I was testing my PR, we weren't testing against django 1.11, never, becasue we call python setup.py develop after calling
pip install -q django=$DJANGO.0 which was installing django>=1.8, django<=1.10; fixed that.
Last one is just adding python 3.6 to tests since it's supported officially from django 1.11

@coveralls
Copy link

coveralls commented Jun 25, 2017 •
edited
Loading

Coverage Status

Coverage decreased (-4.7%) to 91.15% when pulling 947a686 on MattBlack85:api_login into d2b712e on kencochrane:master.

@coveralls
Copy link

coveralls commented Jun 25, 2017 •
edited
Loading

Coverage Status

Coverage decreased (-4.7%) to 91.15% when pulling 2a829e4 on MattBlack85:api_login into d2b712e on kencochrane:master.

kencochrane
kencochrane approved these changes Jun 25, 2017
View reviewed changes
Copy link
Collaborator

@kencochrane kencochrane left a comment

Choose a reason for hiding this comment

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

Awesome, looks good. Thank you!

@kencochrane
Copy link
Collaborator

PR looks good, I'll leave it open for a bit in case anyone else wants to review. If no one else comments, I'll merge.

@MattBlack85
Copy link
Contributor Author

👍

@kencochrane kencochrane merged commit b985d17 into jazzband:master Jun 26, 2017
@kencochrane kencochrane mentioned this pull request Jun 26, 2017
Closed
@manan
Copy link

manan commented Jun 26, 2017

Can I use this yet?

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

@kencochrane kencochrane kencochrane approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@MattBlack85 @coveralls @kencochrane @manan