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

Add Django manage command to create a DRF user Token #5188

Merged
merged 5 commits into from Jul 10, 2017

Conversation

andreagrandi
Copy link
Contributor

@andreagrandi andreagrandi commented May 29, 2017

Description

This pull request add the Django manage command called drf_create_token as documented in the issue #5173

help = 'Create DRF Token for a given user'

def create_user_token(self, username):
user = User.objects.get(username=username)
Copy link
Contributor Author

@andreagrandi andreagrandi May 29, 2017

Choose a reason for hiding this comment

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

I would like to handle the situation when username doesn't exist. Should I let the code raise an exception here and try/except it in the handle() method (maybe displaying the error "ERROR: given username does not exists") or is there a better way to do this?

Copy link
Member

@tomchristie tomchristie May 30, 2017

Choose a reason for hiding this comment

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

Catching it in handle seems reasonable enough.

Copy link
Member

@jpadilla jpadilla Jun 1, 2017

Choose a reason for hiding this comment

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

We should probably use get_user_model() here. I'm also thinking this should make less assumptions regarding querying by username. Perhaps we can leverage the user model's USERNAME_FIELD together with get_by_natural_key(). Similar to how django.contrib.auth.backends.ModelBackend does it.

Copy link
Contributor Author

@andreagrandi andreagrandi Jun 1, 2017

Choose a reason for hiding this comment

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

@jpadilla I will take this advice into consideration, thanks! I'm learning a lot of things :)


def create_user_token(self, username):
user = User.objects.get(username=username)
token = Token.objects.get_or_create(user=user)
Copy link
Contributor Author

@andreagrandi andreagrandi May 29, 2017

Choose a reason for hiding this comment

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

I also would like to handle the situation where the user want to regenerate the Token. Should this be handled as a separate manage.py command, as an additional argument or should the command be interactive and inform the user that the Token already exist and to confirm Y/N they want to create a new Token?

Copy link
Member

@tomchristie tomchristie May 30, 2017

Choose a reason for hiding this comment

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

Not sure. As a command line switch sounds okay on first pass. (?)

try:
token = self.create_user_token(username)
except User.DoesNotExist:
print('Cannot create the Token: user {0} does not exist'.format(
Copy link
Member

@rpkilby rpkilby Jun 1, 2017

Choose a reason for hiding this comment

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

Not entirely sure if it's relevant, but management commands typically use self.stdout.write() instead of print(). eg, https://github.com/django/django/blob/1.11/django/core/management/commands/migrate.py#L141

Copy link
Contributor Author

@andreagrandi andreagrandi Jun 1, 2017

Choose a reason for hiding this comment

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

@rpkilby yep! You are totally right and it's something I just spotted a couple of days ago and I was going to amend it. The officia documentation also suggest to use self.stdout because it's even easier to test. I just haven't had any time yet to complete it. I will try to finish during the weekend. Thanks

@andreagrandi andreagrandi changed the title [WIP] Add Django manage command to create a DRF user Token Add Django manage command to create a DRF user Token Jun 3, 2017
@andreagrandi
Copy link
Contributor Author

andreagrandi commented Jun 13, 2017

Is there any other change I need to do before it can be merged? Thank you so much

@tomchristie tomchristie added this to the 3.6.4 Release milestone Jul 10, 2017
@tomchristie tomchristie merged commit fbb3490 into encode:master Jul 10, 2017
1 check passed
@tomchristie
Copy link
Member

tomchristie commented Jul 10, 2017

Good stuff, thanks @andreagrandi!
Ideally I think we should also update the docs here... http://www.django-rest-framework.org/api-guide/authentication/#tokenauthentication to use the management command, rather than creating the token in the shell. You're welcome to submit that, or else I'd be happy to pick it up (?) - either way's good with me. 😄

@andreagrandi
Copy link
Contributor Author

andreagrandi commented Jul 10, 2017

Woooooow thanks! :) I'm so happy to see this merged!
About the docs, I can create a ticket/issue (what do you prefer? Do you use GitHub issues to assign tickets or do you use something like a Trello board?) and I can assign it to me

@andreagrandi
Copy link
Contributor Author

andreagrandi commented Jul 10, 2017

p.s: maybe we should wait for this master branch to be part of the next release before updating the docs? Otherwise people who install the current latest version and then see the documentation will try to use a command that doesn't exist yet. Or, I can prepare the pull request in the mean time and when you do the next release you remember to merge the documentation fix pr too. As you prefer. Cheers

@xordoquy
Copy link
Collaborator

xordoquy commented Jul 10, 2017

Documentation doesn't update live.
It's part of the release process.

@tomchristie
Copy link
Member

tomchristie commented Jul 10, 2017

Prepping the PR is a good plan.

@andreagrandi
Copy link
Contributor Author

andreagrandi commented Aug 13, 2017

Sorry if it took me so long (I've been a little bit busy lately), here is the PR that adds documentation about this command #5328

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

Successfully merging this pull request may close these issues.

None yet

5 participants