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

Prepare package #1

Merged
merged 21 commits into from
Apr 20, 2017
Merged

Prepare package #1

merged 21 commits into from
Apr 20, 2017

Conversation

belugame
Copy link
Collaborator

No description provided.

Martin Bauer added 18 commits April 13, 2017 10:13
Confirms that the right args and kwargs are ignored on initing CurrentUserField

It was confirmed that the following scenario works well:
- make a model in testapp with CurrentUserField
- create initial migration
- create a dummy record
- as sample model modification: add a model field, rename the CurrentUserField-field
- create and run migration for the modification
- look at dummy record, confirm that user reference is still there
@belugame belugame requested a review from zsoldosp April 18, 2017 14:46
Then use it in a project::

import django_currentuser
from django_currentuser.middleware import (
get_current_user, get_current_authenticated_user)
Copy link
Owner

Choose a reason for hiding this comment

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

@belugame I think we should have an example about using the db field too

@@ -30,6 +30,7 @@
}

MIDDLEWARE_CLASSES = (
'django_currentuser.middleware.ThreadLocalUserMiddleware',
Copy link
Owner

Choose a reason for hiding this comment

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

shouldn't TLUM be after the AuthMiddleware, as the later one sets the request.user attribute?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good point, i wonder why it still works then

Copy link
Owner

@zsoldosp zsoldosp Apr 19, 2017

Choose a reason for hiding this comment

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

notes from an offline conversation

so this works because ThreadLocalUserMiddleware.process_request sets the thread-local variable to a callable, essentially creating a closure that has a reference to request, and only when get_current_user is called is request.user evaluated. Thus as long as that method is not called between the two middlewares (TLUM and AuthenticationMiddleware), the result is OK, as we can see it in the tests.

Copy link
Owner

Choose a reason for hiding this comment

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

will be addressed by #2

try:
from unittest.mock import patch
except ImportError:
from mock import patch
Copy link
Owner

Choose a reason for hiding this comment

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

FYI: the reason we had sixmock in the template was so we don't have to do this try/except import mechanism in the tests

class GetCurrentPersistedUserTestCase(TestCase):

def test_if_user_is_none_it_is_none(self):
self.assert_becomes(current_user=None, expected_persisted=None)
Copy link
Owner

Choose a reason for hiding this comment

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

the name persisted is misleading to me here I think of db persistance first, not threadlocal

@belugame belugame merged commit 2164fc1 into master Apr 20, 2017
@belugame belugame deleted the mb-prepare-package branch April 20, 2017 12: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.

2 participants