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

1580 temat pracy dyplomowej nie traci głosów #1646

Open
wants to merge 3 commits into
base: master-dev
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 19 additions & 1 deletion zapisy/apps/notifications/signals.py
Original file line number Diff line number Diff line change
Expand Up @@ -169,11 +169,29 @@ def notify_board_members_about_voting(sender: Thesis, **kwargs) -> None:
users = [voter.user for voter in all_voters if voter not in accepting_voters]
target = reverse('theses:selected_thesis', args=[thesis.id])

operation = 'create'
if 'operation' in kwargs:
operation = kwargs['operation']
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
operation = 'create'
if 'operation' in kwargs:
operation = kwargs['operation']
operation = kwargs.get('operation', 'create')

Copy link
Contributor

Choose a reason for hiding this comment

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

A tak w ogóle, to jakie są możliwe wartości tego kwargu?

Copy link
Contributor

@lgpawel lgpawel Feb 20, 2024

Choose a reason for hiding this comment

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

A, on wcale nie jest biblioteczny? To na pewno nie chciałbym wprowadzać rozróżniania pomiędzy dwoma możliwościami przez sprawdzanie, czy jakiś napis jest taki a nie inny. Póki to są faktycznie dwie możliwości, to można to opędzić wartością boolowską (o nazwie np. is_new, jeśli nie wpadniemy na nic zgrabniejszego); gdyby było więcej scenariuszy, to należałoby zadeklarować jakiegoś enum-a.

Ponadto skoro mamy nad nim pełną kontrolę, to zamiast zaszywać tutaj jakieś wartości domyślne, to podawajmy go jawnie wysyłając sygnał – przy trzech wywołaniach nawet nie oszczędzamy napisanych znaków.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Zmienione na flage is_new


if operation == 'modify':
if 'notify_only_voted' in kwargs and kwargs['notify_only_voted']:
Copy link
Contributor

Choose a reason for hiding this comment

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

Tu podobnie, zapewnijmy, żeby ten argument zawsze był w kwargs. No i łatwiej jest nazwać jego negację - notify_all (chyba że bardzo nam przeszkadza mniejsza precyzja opisu wywołania z notify_all=False).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Usunąłem zmienna operation i dodałem flage notify_all

already_voted = [v.owner for v in thesis.thesis_votes.all() if v.vote != ThesisVote.NONE]
users = [voter.user for voter in already_voted]
notify_selected_users(
Copy link
Contributor

Choose a reason for hiding this comment

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

Chciałbym, żeby w kodzie po zmianach dalej było dokładnie jedno wywołanie tej funkcji.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Gotowe

users,
Notification(get_id(), get_time(),
NotificationType.THESIS_VOTING_HAS_BEEN_ACTIVATED, {
'title': thesis.title,
'status': 'zmodyfikowanym',
}, target))
return

notify_selected_users(
users,
Notification(get_id(), get_time(),
NotificationType.THESIS_VOTING_HAS_BEEN_ACTIVATED, {
'title': thesis.title
'title': thesis.title,
'status': 'nowym',
}, target))


Expand Down
4 changes: 2 additions & 2 deletions zapisy/apps/notifications/templates.py
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ class NotificationType(str, Enum):
NotificationType.NEWS_HAS_BEEN_ADDED_HIGH_PRIORITY:
"Dodano nową wiadomość w aktualnościach:\n# {title}\n\n{contents}",
NotificationType.THESIS_VOTING_HAS_BEEN_ACTIVATED:
'W pracy dyplomowej "{title}" pojawiła się możliwość głosowania.',
'W {status} temacie pracy dyplomowej "{title}" pojawiła się możliwość głosowania.',
NotificationType.THESIS_HAS_BEEN_ACCEPTED:
'Praca dyplomowa "{title}" została zaakceptowana przez komisję.',
}
Expand All @@ -59,7 +59,7 @@ class NotificationType(str, Enum):
NotificationType.NEWS_HAS_BEEN_ADDED_HIGH_PRIORITY:
"{title}",
NotificationType.THESIS_VOTING_HAS_BEEN_ACTIVATED:
'W pracy dyplomowej "{title}" pojawiła się możliwość głosowania.',
'W {status} temacie pracy dyplomowej "{title}" pojawiła się możliwość głosowania.',
NotificationType.THESIS_HAS_BEEN_ACCEPTED:
'Praca dyplomowa "{title}" została zaakceptowana.',
}
3 changes: 3 additions & 0 deletions zapisy/apps/theses/enums.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,3 +28,6 @@ class ThesisVote(models.IntegerChoices):
NONE = 1, "brak głosu"
REJECTED = 2, "odrzucona"
ACCEPTED = 3, "zaakceptowana"


SIGNIFICANT_FIELDS = ['title', 'supporting_advisor', 'kind', 'max_number_of_students', 'description']
Copy link
Contributor

Choose a reason for hiding this comment

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

Dobry pomysł żeby bardziej wyeksponować tę listę, ale ona nie pasuje do tego pliku. Zostawiłbym ją jednak w forms.py, w formie takiej właśnie deklaracji zaraz za importami - jeśli kiedyś zaczniemy tego używać w jakichś kolejnych funkcjonalnościach, to wtedy się to przeniesie.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

gotowe

8 changes: 4 additions & 4 deletions zapisy/apps/theses/forms.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
from django.utils import timezone

from apps.common import widgets as common_widgets
from apps.theses.enums import ThesisKind, ThesisStatus, ThesisVote
from apps.theses.enums import ThesisKind, ThesisStatus, ThesisVote, SIGNIFICANT_FIELDS
from apps.theses.models import MAX_THESIS_TITLE_LEN, Remark, Thesis, Vote
from apps.users.models import Employee, Student
from apps.theses.validators import MAX_MAX_ASSIGNED_STUDENTS
Expand Down Expand Up @@ -127,11 +127,11 @@ def save(self, commit=True):
instance.modified = timezone.now()

status = self.status
instance.significant_field_changed = False

if len(set(self.changed_data).intersection([
'title', 'supporting_advisor', 'kind',
'max_number_of_students', 'description'])) > 0:
if len(set(self.changed_data).intersection(SIGNIFICANT_FIELDS)) > 0:
instance.status = ThesisStatus.BEING_EVALUATED.value
instance.significant_field_changed = True
elif status == ThesisStatus.ACCEPTED.value and 'students' in self.data:
instance.status = ThesisStatus.IN_PROGRESS.value
elif status == ThesisStatus.IN_PROGRESS.value and 'students' not in self.data:
Expand Down
15 changes: 14 additions & 1 deletion zapisy/apps/theses/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -107,9 +107,22 @@ def save(self, *args, **kwargs):
"""
old = self.pk and type(self).objects.get(pk=self.pk)
super(Thesis, self).save(*args, **kwargs)
if not old or (old.status != ThesisStatus.BEING_EVALUATED and self.status == ThesisStatus.BEING_EVALUATED):
if not old:
self.thesis_votes.filter(vote__in=[ThesisVote.ACCEPTED, ThesisVote.REJECTED]).update(vote=ThesisVote.NONE)
Copy link
Contributor

Choose a reason for hiding this comment

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

Czy ta instrukcja ma znaczenie? Dla nowego tematu nie będzie (nietrywialnych) głosów.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Instrukcja do usuwania głosów usunięta, faktycznie dla nowych tematów nie miała ona sensu

thesis_voting_activated.send(sender=self.__class__, instance=self)

if old and old.status != ThesisStatus.BEING_EVALUATED and self.status == ThesisStatus.BEING_EVALUATED:
# in case of new thesis titile send notification to all board members
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# in case of new thesis titile send notification to all board members
# in case of new thesis title send notification to all board members

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Gotowe

self.thesis_votes.filter(vote__in=[ThesisVote.ACCEPTED, ThesisVote.REJECTED]).update(vote=ThesisVote.NONE)
Copy link
Contributor

Choose a reason for hiding this comment

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

Zdaje się, że w tej gałęzi nie ma znaczenia kolejność "resetowania" głosów oraz wysłania powiadomienia - a gdyby ją odwrócić, to "resetowanie" można postawić w ogóle za instrukcją warunkową.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Wydaje mi się, że nie możemy wyciągnąć tego poza instrukcję warunkową, bo nie zawsze chcemy resetować głosy przy zapisie modelu. Proszę mnie poprawić jeśli się mylę.

thesis_voting_activated.send(sender=self.__class__, instance=self, operation='modify')
if old and (old.status == ThesisStatus.BEING_EVALUATED and
self.status == ThesisStatus.BEING_EVALUATED and
self.significant_field_changed):
# send notification only to the board memebers who already accepted the thesis
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# send notification only to the board memebers who already accepted the thesis
# send notification only to the board members who already voted on the thesis

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Gotowe

thesis_voting_activated.send(sender=self.__class__, instance=self, operation='modify',
notify_only_voted=True)
self.thesis_votes.filter(vote__in=[ThesisVote.ACCEPTED, ThesisVote.REJECTED]).update(vote=ThesisVote.NONE)

def get_accepted_votes(self):
return len(self.thesis_votes.filter(vote=ThesisVote.ACCEPTED))

Expand Down
Loading