-
Notifications
You must be signed in to change notification settings - Fork 52
Feature/notifications #331
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
Feature/notifications #331
Conversation
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.
Awesome job! Very nice.
I've added few insights about this PR, please take a look.
app/database/schemas.py
Outdated
username: str | ||
email: str | ||
full_name: str | ||
|
||
# TODO: Add language_id field |
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.
Remove the TODO
message_id: int, | ||
session: Session, | ||
) -> Union[Message, None]: | ||
"""Returns an invitation by an id. |
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.
Leave blank line after the summary line.
The closing """
should be in their own line.
See PEP 257 for further details.
app/internal/notification.py
Outdated
MessageStatusEnum.UNREAD, | ||
] | ||
|
||
ARCHIVED = [ |
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 check if things are in
ARCHIVED/
UNREAD_STATUS- prefer
{to create a
set`
app/internal/notification.py
Outdated
func: Union[_is_unread, _is_archived], | ||
) -> List[NOTIFICATION_TYPE]: | ||
"""Filters notifications by "func".""" | ||
return list(filter(func, get_all_notifications(session, user_id))) |
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.
yield from
instead list
to imitate the behavior of real filter
app/internal/notification.py
Outdated
def filter_notifications( | ||
session: Session, | ||
user_id: int, | ||
func: Union[_is_unread, _is_archived], |
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.
_is_unread
and is_archived
are not a type. Use Callable
. See PEP 484 or mypy documentations for more details.
Prefer validating your code using mypy.
app/routers/notification.py
Outdated
get_invitation_by_id, | ||
get_unread_notifications, | ||
get_all_messages, | ||
get_message_by_id, | ||
get_archived_notifications, | ||
raise_wrong_id_error, | ||
is_owner, |
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.
abc
app/routers/notification.py
Outdated
@@ -0,0 +1,188 @@ | |||
from fastapi import APIRouter, Depends, Request, Form |
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.
abc
app/routers/notification.py
Outdated
from sqlalchemy.orm import Session | ||
|
||
from app.database.models import MessageStatusEnum | ||
from app.dependencies import templates, get_db |
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.
abc
@@ -1,30 +1,37 @@ | |||
from typing import Generator, Iterator | |||
from typing import Generator, Iterator, Dict |
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.
ABC
import pytest | ||
from fastapi.testclient import TestClient | ||
from sqlalchemy.orm import Session |
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.
ABC
Notification page:
The notification page binds all the messages and the invitations the user received.
to create a new message use
from app.routers.notification import create_message
.* bonus:
create_message
can get an extra parameter (link
), that turns the message into a link.Possible useses: