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

BE - Notifications #54

Merged
merged 9 commits into from Apr 14, 2020
Merged

BE - Notifications #54

merged 9 commits into from Apr 14, 2020

Conversation

@gal432
Copy link
Member

gal432 commented Apr 13, 2020

No description provided.

@gal432 gal432 requested a review from yammesicka Apr 13, 2020
lms/lmsdb/models.py Outdated Show resolved Hide resolved
lms/lmsweb/views.py Outdated Show resolved Hide resolved
lms/lmsweb/views.py Outdated Show resolved Hide resolved
lms/lmsnotifications/solution_checked.py Outdated Show resolved Hide resolved
lms/tests/test_auto_solution_solver.py Outdated Show resolved Hide resolved
gal432 added 3 commits Apr 14, 2020
@gal432 gal432 requested a review from yammesicka Apr 14, 2020
gal432 added 2 commits Apr 14, 2020
marked_read = BooleanField(default=False)

@property
def message_parameters_dict(self) -> dict:

This comment has been minimized.

@yammesicka

yammesicka Apr 14, 2020

Member

Avoid verbosity when it isn't needed

@property
def params(self) -> Dict[str, Any]:
cls,
for_user: User,
) -> Iterable['Notification']:
return cls.select().join(User).filter(

This comment has been minimized.

@yammesicka

yammesicka Apr 14, 2020

Member

Why filter instead of where?

This comment has been minimized.

@gal432

gal432 Apr 14, 2020

Author Member

Why not?

This comment has been minimized.

@yammesicka

yammesicka Apr 14, 2020

Member

For the sake of consistency, because where is much more documented and because this is a thing that represent a query (in which you use WHERE clause)

cls,
user: User,
notification_type: str,
message_parameters: dict,

This comment has been minimized.

@yammesicka

yammesicka Apr 14, 2020

Member

Dict[str, Any]

This comment has been minimized.

@gal432

gal432 Apr 14, 2020

Author Member

Why? it can be nested as it want

This comment has been minimized.

@yammesicka

yammesicka Apr 14, 2020

Member

The key should be string, just like in string formatting

lms/lmsdb/models.py Outdated Show resolved Hide resolved
lms/lmstests/public/flake8/services.py Outdated Show resolved Hide resolved
lms/lmsweb/views.py Outdated Show resolved Hide resolved
lms/lmsweb/views.py Outdated Show resolved Hide resolved
lms/lmsweb/views.py Outdated Show resolved Hide resolved
def get_notifications_for_user(for_user: models.User) -> typing.Sequence[dict]:
return tuple(
{
models.Notification.ID_FIELD_NAME:

This comment has been minimized.

@yammesicka

yammesicka Apr 14, 2020

Member

from models import notifications or something

This comment has been minimized.

@gal432

gal432 Apr 14, 2020

Author Member

Why? I think its cleaner, I need more models here - what is the point?

This comment has been minimized.

@yammesicka

yammesicka Apr 14, 2020

Member

It's just hurt the eyes to see many linebreaks and I think it too verbose, but as you wish :)

instance.create_notification(for_user=for_user, **kwargs)


def get_notifications_for_user(for_user: models.User) -> typing.Sequence[dict]:

This comment has been minimized.

@yammesicka

yammesicka Apr 14, 2020

Member

Use Dict

This comment has been minimized.

@gal432

gal432 Apr 14, 2020

Author Member

I didn't understand

This comment has been minimized.

@yammesicka

yammesicka Apr 14, 2020

Member

typing.Sequence[Dict[str, Any]]

@gal432 gal432 merged commit 1f0969d into master Apr 14, 2020
2 checks passed
2 checks passed
LGTM analysis: JavaScript No code changes detected
Details
LGTM analysis: Python No new or fixed alerts
Details
@gal432 gal432 deleted the feature/notifications-35 branch Apr 14, 2020
@gal432 gal432 linked an issue that may be closed by this pull request Apr 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants
You can’t perform that action at this time.