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

Added a section in docs about decorators #67

Merged
merged 4 commits into from
Jul 27, 2020

Conversation

sandeshbhusal
Copy link

JWT and authorization can be a tough topic for beginners to break into. While the documentation clearly outlines the starting places, it has left out the portion for decorators and decorating the to-be-protected views.

JWT and authorization can be a tough topic for beginners to break into. While the documentation clearly outlines the starting places, it has left out the portion for decorators and decorating the to-be-protected views.
Copy link
Collaborator

@fitodic fitodic left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! 👍

If you could just apply a minor change or two and we're done. 🙂

docs/index.md Outdated
@@ -97,6 +97,21 @@ Now in order to access protected api urls you must include the `Authorization: B
$ curl -H "Authorization: Bearer <your_token>" http://localhost:8000/protected-url/
```

In addition to adding the Authorization: Bearer in your requests, make sure that you have decorated your views properly. For this, import the JSONWebTokenAuthentication authenticaion class from rest_framework_jwt.authentication
Copy link
Collaborator

Choose a reason for hiding this comment

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

JSONWebTokenAuthentication authenticaion

"authenticaion" is misspelled, and the JSONWebTokenAuthentication can be styled as JSONWebTokenAuthentication

make sure that you have decorated your views properly

I suggest you point out that this only applies to function based views. The class based views include this by overriding the authentication_classes class attribute. Feel free to add an example for the class based views as well.

Copy link
Author

Choose a reason for hiding this comment

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

Sure! I will modify the PR according to your suggestions!

@fitodic
Copy link
Collaborator

fitodic commented Jul 21, 2020

Closes #66

Fixed spelling error in previous PR and added a section about class-based views in the doc.
@sandeshbhusal
Copy link
Author

I just made the changes. Could you review the changes? Thanks!

@codecov-commenter
Copy link

codecov-commenter commented Jul 21, 2020

Codecov Report

Merging #67 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #67   +/-   ##
=======================================
  Coverage   98.12%   98.12%           
=======================================
  Files          19       19           
  Lines         481      481           
  Branches       45       45           
=======================================
  Hits          472      472           
  Misses          7        7           
  Partials        2        2           
Flag Coverage Δ
#codecov 98.12% <ø> (ø)
#dj111 97.65% <ø> (ø)
#dj20 ?
#dj21 ?
#dj22 ?
#dj30 97.08% <ø> (-0.63%) ⬇️
#drf310 ?
#drf311 97.08% <ø> (-0.63%) ⬇️
#drf37 97.65% <ø> (ø)
#drf38 97.65% <ø> (ø)
#drf39 ?
#py27 97.23% <ø> (ø)
#py35 97.23% <ø> (ø)
#py36 97.23% <ø> (ø)
#py37 97.23% <ø> (ø)
#py38 97.08% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 14441fc...e69f184. Read the comment docs.

Copy link
Collaborator

@fitodic fitodic left a comment

Choose a reason for hiding this comment

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

Could you please add a changelog as well? I forgot to mention it the first time.

docs/index.md Show resolved Hide resolved
@fitodic fitodic merged commit 12c08db into Styria-Digital:master Jul 27, 2020
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.

3 participants