-
-
Notifications
You must be signed in to change notification settings - Fork 66
Add API for AccessToken #170
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
base: master
Are you sure you want to change the base?
Conversation
clean code
Add API for Authentication
add more tests
mrmn2
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the PR and sorry for the late review. I have added some minor corrections. Additionally the code quality job has failed, so you need to fix the findings of flake8. I also suggest to run black locally so the code is correctly formatted. Otherwise the black step will probably also fail. Take a look at the development docs.
|
@mrmn2 , do you want me to add the UI part of creating/updating/deleting tokens in this PR or a different one? |
| # Create your views here. | ||
|
|
||
|
|
||
| class AccessTokenViewSet(ModelViewSet): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about this some more. If I am not mistaken the ModelViewSet will, amongst others add the create and destroy action. I don't think this is really needed. Creating and destroying tokens should be done via the UI. As the PdfDing is using Django's template engine token creation and deletion can be handled directly by Django instead of the API. Or did I miss something? Sorry that I did only think about this now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I got your point. I'll remove Token Creation/Deletion from API.
- What about token rotation? That should also not be allowed
- For updation, only thing that can be updated is
name.
So for this viewset, only update api is required, with only name that can be updated.
I'll still keep all the serializers, so that when UI part is done, it can be reused/referred
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am very sorry for the late reply. Things will be better from on as I can dedicate more time on PdfDing again. Yes rotation should also only be done via the UI. As you mentioned you can keep the serializers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also you'll need to rebase on the repo"s master branch as I updated pyproject.yaml and poetry.lock which will cause merge conflicts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to regenerate the lock file via poetry lock as the tests will not run otherwise.
AccessTokenmodel