-
Notifications
You must be signed in to change notification settings - Fork 52
Feature/calendar privacy #247
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
Changes from all commits
a32cc6a
e29a73e
0455c1e
40008d2
e514f2b
8f01051
36f2480
072d124
48704b0
91451d7
0f14060
d536593
da15328
52bbfab
f436850
b563b02
03e1d0f
d49c8dc
3b69510
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -31,6 +31,7 @@ class User(Base): | |
avatar = Column(String, default="profile.png") | ||
telegram_id = Column(String, unique=True) | ||
is_active = Column(Boolean, default=False) | ||
privacy = Column(String, default="Private", nullable=False) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the only values you expect are "private" and "public", consider using boolean type - True ( for private) and Falsr (for public). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Originally the feature was planned to contain "friends" as a level of privacy. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So maybe use Enum? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As this is not a free text field but rather something from a set of closed options in a dropdown I don't think there is any reason to fear an incorrect value here There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that it's a good practice to add a validation also in backend and in db. I agree that right now it's too much to add another table. Please make sure that it's invalid to add a bad value, even if someone tries :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. where would you suggest doing this? I didn't see that anyone did something similar I think... It can't be in the models file obviously.. and as I mentioned no one can enter values I did not specify in the drropdown field, it's not a free text where anyone can enter what they want, just a set of options. Does that not qualify as verifying that only possible values can be entered? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. so anything? or can I resolve this? :) |
||
is_manager = Column(Boolean, default=False) | ||
language_id = Column(Integer, ForeignKey("languages.id")) | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
from app.dependencies import get_db | ||
from app.database.models import User | ||
# TODO switch to using this when the user system is merged | ||
# from app.internal.security.dependancies import ( | ||
imimouni marked this conversation as resolved.
Show resolved
Hide resolved
|
||
# current_user, CurrentUser) | ||
|
||
from fastapi import Depends | ||
|
||
|
||
# TODO add privacy as an attribute in current user | ||
imimouni marked this conversation as resolved.
Show resolved
Hide resolved
|
||
# in app.internal.security.dependancies | ||
# when user system is merged | ||
def can_show_calendar( | ||
requested_user_username: str, | ||
db: Depends(get_db), | ||
current_user: User | ||
# TODO to be added after user system is merged: | ||
# CurrentUser = Depends(current_user) | ||
) -> bool: | ||
"""Check whether current user can show the requested calendar""" | ||
requested_user = db.query(User).filter( | ||
User.username == requested_user_username | ||
).first() | ||
privacy = current_user.privacy | ||
is_current_user = current_user.username == requested_user.username | ||
if privacy == 'Private' and is_current_user: | ||
return True | ||
|
||
elif privacy == 'Public': | ||
return True | ||
|
||
return False |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -138,6 +138,23 @@ async def update_telegram_id( | |
return RedirectResponse(url=url, status_code=HTTP_302_FOUND) | ||
|
||
|
||
@router.post("/privacy") | ||
async def update_calendar_privacy( | ||
request: Request, | ||
session=Depends(get_db) | ||
): | ||
user = session.query(User).filter_by(id=1).first() | ||
data = await request.form() | ||
new_privacy = data['privacy'] | ||
|
||
# Update database | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you can delete the comment There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This comment appears in all the other forms so I just kept the same structure :) Unless a decision is made to remove the comments from all the file I think this should stay |
||
user.privacy = new_privacy | ||
session.commit() | ||
|
||
url = router.url_path_for("profile") | ||
return RedirectResponse(url=url, status_code=HTTP_302_FOUND) | ||
|
||
|
||
@router.get("/holidays/import") | ||
def import_holidays(request: Request): | ||
return templates.TemplateResponse("import_holidays.html", { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -107,3 +107,7 @@ p { | |
.subtitle { | ||
font-size: 20px; | ||
} | ||
|
||
h2.modal-title { | ||
font-size: 1.25rem; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,43 @@ | ||
from app.internal.calendar_privacy import can_show_calendar | ||
# TODO after user system is merged: | ||
imimouni marked this conversation as resolved.
Show resolved
Hide resolved
|
||
# from app.internal.security.dependancies import CurrentUser | ||
from app.routers.user import create_user | ||
|
||
|
||
def test_can_show_calendar_public(session, user): | ||
user.privacy = "Public" | ||
# TODO to be replaced after user system is merged: | ||
imimouni marked this conversation as resolved.
Show resolved
Hide resolved
|
||
# current_user = CurrentUser(**user.__dict__) | ||
current_user = user | ||
result = can_show_calendar( | ||
requested_user_username='test_username', | ||
db=session, current_user=current_user | ||
) | ||
assert result is True | ||
session.commit() | ||
|
||
|
||
def test_can_show_calendar_private(session, user): | ||
another_user = create_user( | ||
session=session, | ||
username='new_test_username2', | ||
email='new_test.email2@gmail.com', | ||
password='passpar_2', | ||
language_id=1 | ||
) | ||
current_user = user | ||
# TODO to be replaced after user system is merged: | ||
# current_user = CurrentUser(**user.__dict__) | ||
|
||
result_a = can_show_calendar( | ||
requested_user_username='new_test_username2', | ||
db=session, current_user=current_user | ||
) | ||
result_b = can_show_calendar( | ||
requested_user_username='test_username', | ||
db=session, current_user=current_user | ||
) | ||
assert result_a is False | ||
assert result_b is True | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you can write it down as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I got a previous comment saying to do the opposite haha ... let's just stay with this:) |
||
session.delete(another_user) | ||
session.commit() |
Uh oh!
There was an error while loading. Please reload this page.