1580 temat pracy dyplomowej nie traci głosów #1646
Conversation
zapisy/apps/theses/enums.py
Outdated
| @@ -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'] | |||
There was a problem hiding this comment.
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.
zapisy/apps/notifications/signals.py
Outdated
| operation = 'create' | ||
| if 'operation' in kwargs: | ||
| operation = kwargs['operation'] |
There was a problem hiding this comment.
| operation = 'create' | |
| if 'operation' in kwargs: | |
| operation = kwargs['operation'] | |
| operation = kwargs.get('operation', 'create') |
There was a problem hiding this comment.
A tak w ogóle, to jakie są możliwe wartości tego kwargu?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Zmienione na flage is_new
zapisy/apps/notifications/signals.py
Outdated
| if 'notify_only_voted' in kwargs and kwargs['notify_only_voted']: | ||
| 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( |
There was a problem hiding this comment.
Chciałbym, żeby w kodzie po zmianach dalej było dokładnie jedno wywołanie tej funkcji.
zapisy/apps/theses/models.py
Outdated
| @@ -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) | |||
There was a problem hiding this comment.
Czy ta instrukcja ma znaczenie? Dla nowego tematu nie będzie (nietrywialnych) głosów.
There was a problem hiding this comment.
Instrukcja do usuwania głosów usunięta, faktycznie dla nowych tematów nie miała ona sensu
zapisy/apps/theses/models.py
Outdated
| 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 |
There was a problem hiding this comment.
| # in case of new thesis titile send notification to all board members | |
| # in case of new thesis title send notification to all board members |
zapisy/apps/theses/models.py
Outdated
| 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 |
There was a problem hiding this comment.
| # 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 |
zapisy/apps/notifications/signals.py
Outdated
| operation = kwargs['operation'] | ||
|
|
||
| if operation == 'modify': | ||
| if 'notify_only_voted' in kwargs and kwargs['notify_only_voted']: |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Usunąłem zmienna operation i dodałem flage notify_all
| 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 | ||
| self.thesis_votes.filter(vote__in=[ThesisVote.ACCEPTED, ThesisVote.REJECTED]).update(vote=ThesisVote.NONE) |
There was a problem hiding this comment.
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ą.
There was a problem hiding this comment.
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ę.
W przypadku zmiany istotnych pól w formularzu tematu pracy dyplomowej, temat nie tracił głosów. PR uwzględnia usunięcie głosów jeśli status tematu pracy dyplomowej został zmieniony na
BEING_EVALUATED. Dodatkowa zmiana to taka, że jeśli zmieniły się istotne pola w temacie pracy dyplomowej, ale status pozostałBEING_EVALUATEDto wysyłane zostaje powiadomienie tylko do osób, które oddały głos na temat pracy.PR również uwzględnia zmianę treści powiadomień z
W pracy dyplomowej "{title}" pojawiła się możliwość głosowania.
na
W nowym temacie pracy dyplomowej "{title}" pojawiła się możliwość głosowania oraz
W zmodyfikowanym temacie pracy dyplomowej "{title}" pojawiła się możliwość głosowania
Podsumowując mamy następnę powiadomienia: