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

refactor/jinja-global-variable #302

Open
wants to merge 17 commits into
base: develop
Choose a base branch
from

Conversation

issac211
Copy link
Contributor

@issac211 issac211 commented Feb 16, 2021

user global variable

Contains an example of how to use a global variable
Once it is possible to see if a user is logged in, this method can be used in the code

Copy link
Contributor

@PureDreamer PureDreamer left a comment

Choose a reason for hiding this comment

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

The idea seem very good, yet I can't see how does it work, could you add documentation?
And if it does work the way you wanted, could you add support for showing log-out only for logged-in users, and login/sign up for not logged-in users?

app/routers/global_variable.py Outdated Show resolved Hide resolved
app/templates/base.html Outdated Show resolved Hide resolved
@codecov-io
Copy link

Codecov Report

Merging #302 (7ae0448) into develop (28e460d) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop     #302   +/-   ##
========================================
  Coverage    98.76%   98.77%           
========================================
  Files           63       64    +1     
  Lines         2843     2863   +20     
========================================
+ Hits          2808     2828   +20     
  Misses          35       35           
Impacted Files Coverage Δ
app/main.py 93.61% <100.00%> (+0.75%) ⬆️
app/routers/global_variable.py 100.00% <100.00%> (ø)

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 28e460d...cb0e0f9. Read the comment docs.

Copy link
Member

@yammesicka yammesicka left a comment

Choose a reason for hiding this comment

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

I find it a bit hard to understand what is the purpose of the feature.
Can you please explain it in a nutshell?

app/main.py Outdated Show resolved Hide resolved
@issac211
Copy link
Contributor Author

I find it a bit hard to understand what is the purpose of the feature.
Can you please explain it in a nutshell?

Upload the connected user to a global variable
So that all routers can access it without placing it in their variables.
I therefore place the global variable outside the function of the router.

The router I added is for illustration purposes, so it's inside a demo folder.

@issac211
Copy link
Contributor Author

#302 (review)
I hope it's clearer now,

in the tests\global_var_testing_routes.py
you can see how the function 'set_global_user_var' can be used in the router.

I changed the code, and used the security module functions to make it more usable in the project.
wen using the 'set_global_user_var' function the template will get the username if the user is logged in

So that this function can be used in routers that still want to identify if the user is connected but do not want to be redirected to the login page if not (This is how it was defined in the security module functions)

@@ -0,0 +1,52 @@
<!DOCTYPE html>
Copy link
Member

Choose a reason for hiding this comment

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

There shouldn't be a file used just for testing in the templates directory.
Check the documentation for functions that allow you to load templates from strings instead.

@@ -0,0 +1,21 @@
from sqlalchemy.orm import Session
Copy link
Member

Choose a reason for hiding this comment

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

I believe this file can also be deleted



async def get_user_for_global_var(db: Session, jwt: str) -> str:
jwt_payload = await get_jwt_token(db, jwt)
Copy link
Member

Choose a reason for hiding this comment

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

This will make the site query the database with every request to the site.
It's nullify the advantages of JWT

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So maybe I could pull the token out of the cookie, and if the token is in the right length (and maybe more tests), I wiil set a global var: 'is_token'.
To verify that there is a logged in user.
Tell me what you think

Copy link
Member

Choose a reason for hiding this comment

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

Great! You can also take the user details from the JWT, not from the database.


from app.dependencies import templates
from app.internal.security.ouath2 import (
Session, get_jwt_token, get_authorization_cookie
Copy link
Member

Choose a reason for hiding this comment

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

Use git precommit hooks to order that right and add a trailing comma

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.

5 participants