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

Update models.py #115

Merged
merged 4 commits into from
Sep 10, 2014
Merged

Update models.py #115

merged 4 commits into from
Sep 10, 2014

Conversation

RossLote
Copy link
Contributor

@RossLote RossLote commented Sep 9, 2014

When using the request middleware, if the user is anonymous an error is raised stating that the 'history_user' attribute must be an instance of User. As this field can be null, get_history_user has been updated to return None if the user is not authenticated. Also made the returning of None explicit.

Fixes #114

When using the request middleware, if the user is anonymous an error is raised stating that the 'history_user' attribute must be an instance of User. As this field can be null, get_history_user has been updated to return None if the user is not authenticated. Also made the returning of None explicit.
@macro1
Copy link
Collaborator

macro1 commented Sep 9, 2014

Needs a test. Would you mind taking a stab at it?

Added test for allowing Anonymous user when using HistoryRequestMiddleware.
@RossLote
Copy link
Contributor Author

RossLote commented Sep 9, 2014

@macro1 That's the test committed now. Just waiting for them all to pass. Looking good so far though.

@macro1
Copy link
Collaborator

macro1 commented Sep 9, 2014

Nice. Unfortunately that test isn't testing for the bug though. If you undo your fix, you will see it still passes. You need to do a 'get' on any page, just to set the request in the local thread variable through the middleware. Make sure you do it inside the context block, so the middleware will have an effect. The other test sets the request in the thread variable as part of logging in.

@RossLote
Copy link
Contributor Author

RossLote commented Sep 9, 2014

@macro1 You mean like that? I've never used django_webtest before.

@macro1
Copy link
Collaborator

macro1 commented Sep 9, 2014

Before the objects.create, so one line higher.

The idea is the middleware needs to be triggered to make the request available before you do the create, which checks the thread variable for the request, and its user. Right now the request is unset, so it defaults to None and passes every time without making the is_authenticated check.

@RossLote
Copy link
Contributor Author

RossLote commented Sep 9, 2014

Oh yeah, silly me. I was too busy trying to find a page that I could do a get request on I over looked the order.

@macro1 macro1 merged commit 0b7379d into jazzband:master Sep 10, 2014
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.

history_user error during usage of simple_history.middleware.HistoryRequestMiddleware
2 participants