Powiadomienie do obserwujących sprawę #934

NameeLesS · 2021-05-09T20:27:36Z

Widziałem że w cases/views.py jest NotifiedUserViewSet, więc chyba podszedłem do tego nie do końca po twojej myśli #923

for more information, see https://pre-commit.ci

backend-project/small_eod/notifications/views.py

backend-project/small_eod/users/models.py

ad-m · 2021-05-11T23:19:16Z

Czy potrzebujesz pomocy?

NameeLesS · 2021-05-11T23:57:31Z

Sry, ostatnio zajmowałem się czymś innym i dopiero dzisiaj nad tym usiadłem. Wydaje mi się, że na razie wszystko jest dla mnie jasne, chociaż nie do końca rozumiem, dlaczego w tym twoim utilsie do wysyłania maili stworzyłeś tego enuma, tzn. dlaczego nie zrobiłeś czegoś w stylu

TEMPLATE_MAP = { 

	"CASE_CLOSED": MailTemplate.from_prefix("cases/email/case_closed") 

	# ... 

	}

a robiłeś to tak bardzo naokoło, czy to miało jakiś cel?

ad-m · 2021-05-12T00:04:27Z

Jak dla mnie dzięki temu jest skuteczniejsza statyczna kontrola kodu z zewnętrznych (znajdujących się w wielu miejscach poza użytkownikiem) modułów, ale wprowadził to @rwakulszowa w watchdogpolska/poradnia@86b0fac#diff-8c555138c101946f482a4a30648dae6f6ee462daf7a92c3f6739ea401ec8fea3 , więc ewentualnie jego pytaj.

for more information, see https://pre-commit.ci

backend-project/small_eod/notifications/utils.py

backend-project/small_eod/notifications/views.py

ad-m · 2021-06-07T00:11:20Z

backend-project/small_eod/users/tests/test_models.py

+
+    def test_do_not_send_disabled_notification(self):
+        actor = ""
+        action = ""


Proponuje wprowadzić w tym miejscu pewne wartości, które wskaża na niewłączone powiadomienie.

backend-project/small_eod/notifications/views.py

ad-m · 2021-06-07T00:17:12Z

backend-project/small_eod/cases/templates/cases/email/case_created.html

+{% block content %}
+<p>W systemie small_eod pojawiła się nowa sprawa.</p>
+
+<p>


To jest wystarczające na ten moment. Zaznaczam, że pewnie w przyszłości informacje o sprawie prawdopodobnie będziemy chcieli mieć w jakiejś formie stopki, aby było jedno miejsce do klikania. Pozwólby jednak temu działać i wykorzystać przez Agnieszkę.

ad-m · 2021-06-07T00:18:46Z

Jesteśmy całkiem blisko. Drobne komentarze mam, ale przede wszystkim sam muszę uruchomić.

rwakulszowa · 2021-06-16T10:28:20Z

Jak dla mnie dzięki temu jest skuteczniejsza statyczna kontrola kodu z zewnętrznych (znajdujących się w wielu miejscach poza użytkownikiem) modułów, ale wprowadził to @rwakulszowa w watchdogpolska/poradnia@86b0fac#diff-8c555138c101946f482a4a30648dae6f6ee462daf7a92c3f6739ea401ec8fea3 , więc ewentualnie jego pytaj.

Przepraszam, nie zauważyłem wcześniej powiadomienia.

W mocnym skrócie - idea była taka jak opisał Adam, tj. przeniesienie dynamicznych wartości (stringów) na statyczne wartości (enumy). Pomijając jakieś tam kwestie estetyczne, które są względne, z tym podejściem po prostu trudniej o jakieś drobne literówki, IDE lepiej radzą sobie z sugestiami, a każde odniesienie do klucza szablonu maila wymaga od użytkownika użycia typu TemplateKey, co, imo, jest bardziej czytelne niż po prostu wpisanie stringa z palca.

Koniec końców oba podejścia są w stanie zrobić to samo, a wybór to kwestia gustu.

ad-m · 2021-07-12T02:17:15Z

@NameeLesS , czy możesz zrobić rebase?

Poza tym wciąż jest to draft.

NameeLesS · 2021-07-20T00:32:03Z

Sry, próbowałem i nie bardzo potrafię ogarnąć tego rebase

ad-m · 2021-07-20T01:06:43Z

Nie szkodzi. Albo mnie złap na Messengerze wieczorem, albo domerguj tu branch dev, a na zrobię squash & merge. Chciałbym, aby lintery przeszły.

ad-m · 2021-07-26T21:59:23Z

@NameeLesS czy mogę Ci pomóc?

NameeLesS · 2021-07-27T22:59:02Z

Zrobiłem merge rozwiązałem konflikty, przed commitem chciałem wykonać testy aby sprawdzić, czy wszystko jest ok no i wywala mi błąd ModuleNotFoundError: No module named 'MySQLdb
Zgodnie z dokumentacją django zainstalowałem mysqlclient oraz wszelkie paczki, które były zalecane, no i nic to nie dało. Próbowałem rozwiązań ze stackoverflow, no i na razie nic nie pomogło.

ad-m · 2021-07-27T23:28:21Z

Jak instalowałeś te paczki?

Czy wykorzystujesz Dockera? Jeżeli tak, w jaki sposób? Czy możesz pokazać logi, które prowadzą Cię do błędu?

Czy możesz wykonać make build? Jeżeli tak, czy naprawia to sytuacje?

…o dev

for more information, see https://pre-commit.ci

NameeLesS · 2021-07-28T21:11:54Z

make build rozwiązało problem

ad-m

Nie mam żadnych uwag więcej.

Proszę o opinie jeszcze @rwakulszowa , bo może mieć wartościowy wkład.

rwakulszowa

Dodałem parę groszy od siebie.

Część to drobiazgi, część to większe znaki zapytania.
Gdyby coś było niejasne (pisałem na szybko) - daj znać.

backend-project/small_eod/notes/templates/notes/email/note_created.html

rwakulszowa · 2021-07-29T08:13:59Z

backend-project/small_eod/notifications/utils.py

+logger = logging.getLogger(__name__)
+
+
+def make_auto():


Funkcję make_auto można usunąć i zamiast niej użyć enum.auto:
https://docs.python.org/3/library/enum.html#enum.auto

W poradni make_auto dopisano w momencie, gdy używaliśmy starszej wersji pythona. W small_eod używamy pythona 3.7, więc funkcja enum.auto jest już dostępna (od wersji 3.6).

Początkowo chciałem użyć enum.auto ale trochę zmylił mnie komentarz "enum.auto replacement"

rwakulszowa · 2021-07-29T08:16:39Z

backend-project/small_eod/notifications/utils.py

+
+
+class MailTemplate:
+    def __init__(self, txt_path, html_path=None):


Jeśli każde powiadomienie ma wersje txt i html, sugerowałbym usunięcie domyślnej wartości html_path=None i wymuszenie, by użytkownik podał oba argumenty.

W poradni dodano html_path=None na czas migracji z txt do html (pewnie powinniśmy byli usunąć domyślny None, ale nam się zapomniało, lub nadal brakuje paru szablonów html).

rwakulszowa · 2021-07-29T08:22:46Z

backend-project/small_eod/notifications/utils.py

+
+    @classmethod
+    def send(cls, template_key, recipient_list, context=None, from_email=None):
+        template = random.choice(cls.TEMPLATE_MAP[template_key])


Nie do końca rozumiem skąd wziął się tutaj random.choice. Z tego co widzę, każdy klucz w TEMPLATE_MAP ma przypisaną 1-elementową listę, przez co random.choice wybiera losowy element z 1-elementowego zbioru, czyli jedyny element.

Być może czegoś nie rozumiem, ale tę samą logikę można zaimplementować przez zmianę typu wartości TEMPLATE_MAP: zamiast używać 1-elementowych list, używając po prostu jedynego elementu danej listy. W konsekwencji można będzie też usunąć referencje do random.

Był taki pomysł, aby maile były wybierane losowo

Zacznijmy z małym rozwiązaniem. Poza tym takie podejście do losowości wydaje mi się czytelniejsze: https://github.com/watchdogpolska/docker-images/blob/master/schedule-bot/lib/template.htm

rwakulszowa · 2021-07-29T09:42:44Z

backend-project/small_eod/notifications/views.py

+    init_instance = None
+    http_action = None


O ile dobrze rozumiem, te dwa pola są używane tylko wewnątrz SendNotificationMixin i nie powinny być ustawiane przez inne klasy (innymi słowy - trzymają stan mixinu).

W takiej sytuacji sugerowałbym dodanie prefiksu _ (tj. np. _init_instance) przed nazwą zmiennej. W nomenklaturze pythona pola zaczynające się od _ traktowane są jako prywatne. Użytkownikom będzie łatwiej zrozumieć, że nie powinni używać tych pól poza klasą.

(Co do sekcji poniżej nie jestem w 100% przekonany że mam rację (ostatnio mniej pracuję z pythonem), więc tutaj proszę @ad-m o potwierdzenie)

Co więcej, w tym momencie pola te są ustawiane na poziomie klasy, a nie instancji. To oznacza, że każda klasa używająca SendNotificationsMixin będzie nadpisywała wartości tych pól dla każdej innej klasy. Bardziej naturalne wydaje mi się dodanie metody __init__ i tam przypisanie wartości do pól self._init_instance i self._http_action.

rwakulszowa · 2021-07-29T09:58:14Z

backend-project/small_eod/notifications/views.py

+        self.http_action = list(self.action_map.keys())[
+            list(self.action_map.values()).index(self.action)
+        ]


O ile dobrze rozumiem, ten blok wyszukuje w słowniku d klucz k przypisany do wartości v na podstawie v.

Można tę samą logikę napisać odrobinę prościej:
next(k for k, v in self.action_map.items() if v == self.action)

dict.items() zwraca iterator par (klucz, wartość).
Kod wewnątrz nawiasu iteruje po parach, jednocześnie filtrując wartości, zwracając jedynie te k, z którymi sparowany jest v == self.action.
next zwraca pierwszy element utworzonego iteratora.

rwakulszowa · 2021-07-29T10:01:37Z

backend-project/small_eod/notifications/views.py

+    notified_users = None
+    ignored_fields = []


Sugerowałbym delikatne zmiany nazw tych pól:

notified_users -> notified_users_field. Obecna nazwa pasowałaby (moim zdaniem) bardziej do konkretnej listy użytkowników otrzymujących powiadomienie, nie zaś pole w którym taką listę można znaleźć.

ignored_fields -> notification_diff_ignored_fields. Dla czytelności. Pole ignored_fields jest na tyle generyczne, że trudno domyślić się, iż dotyczy tylko powiadomień.

rwakulszowa · 2021-07-29T10:05:23Z

backend-project/small_eod/notifications/views.py

+from rest_framework.views import APIView
+
+
+class SendNotificationsMixin(APIView):


(Pytanie do @ad-m, ponieważ nie jestem w 100% przekonany czy mam rację.)

Czy nasz Mixin powinien dziedziczyć po pełnoprawnej klasie (tj. nie po innym Mixinie), czy powinniśmy pozwolić klasie używającej naszego mixinu na dodanie APIView? W tym momencie nasz SendNotificationMixin nie jest sensu stricto Mixinem, a pełnoprawną klasą, o ile dobrze kojarzę.

A no nie jest mixinem, trudno będzie nie dziedziczyć po ApiView, więc można po prostu zmienić nazwę tej klasy na SendNotificationsView lub NotificationsView

Moim zdaniem należaloby użyć class SendNotificationsMixin. Wówczas powiadomienia są tylko czymś dodawanym do jakiegoś istniejącego widoku, a nie definiują kolejności klas w żadnym widoku. Wydaje mi się, że to wystarczy, aby uznać tę klasę za Mixin.

Oczywiście, z drugiej strony należy zadbać o kolejność klasy, więc zapisać raczej jako:

class CaseViewSet(NotificationsMixin, viewsets.ModelViewSet):

rwakulszowa · 2021-07-29T10:14:18Z

backend-project/small_eod/events/tests/test_views.py

@@ -22,6 +23,12 @@ class EventViewSetTestCase(
    def validate_item(self, item):
        self.assertEqual(item["name"], self.obj.name)

+    def validate_notifications(self, action):
+        mail_types = [_mail.extra_headers["Action"] for _mail in mail.outbox]


nit - _mail -> mail.
Zmienne z przedrostkiem _ używane są w zasadzie tylko gdy nie są używane, lub gdy są polami prywatnymi instancji. Tutaj sugerowałbym zmianę nazwy zmiennej na mail.

Gdy nie są używane lub aby uniknąć konfliktu nazw, ale tutaj rzeczywiście może być po prostu mail

rwakulszowa · 2021-07-29T10:32:29Z

backend-project/small_eod/events/tests/test_views.py

+    def validate_notifications(self, action):
+        mail_types = [_mail.extra_headers["Action"] for _mail in mail.outbox]
+        self.assertEqual(
+            mail_types, [action for _ in self.obj.case.notified_users.all()]


O ile dobrze rozumiem, ta metoda najpierw pobiera wartość Action dla wszystkich wiadomości w outbox, po czym tworzy listę N powtórzeń wartości action, gdzie N = deklarowana liczba odbiorców powiadomienia.

Innymi słowy, metoda sprawdza 2 kwestie:

Liczba powiadomień w outbox jest równa liczbie self.obj.case.notified_users

Każdy element mail_types jest równy action.

Sugerowałbym rozbić tę asercję na dwie oddzielne - moim zdaniem łatwiej będzie wtedy odczytać ewentualne błędy jak i zrozumieć kod.

Co do 2 - prostym sposobem sprawdzenia czy wszystkie elementy danego zbioru mają tę samą wartość jest konwersja na zbiór: self.assertEqual(set(mail_types), { action })

NameeLesS · 2021-08-04T15:29:09Z

backend-project/small_eod/letters/templates/letters/email/letter_updated.html

+{% endblock %}
+
+{% block content %}
+<<<<<<< Updated upstream


O cholera xD Pycharm pokazywał mi że nie mam żadnych konfliktów.

add SendNotificationsMixin

128c37e

boring-cyborg bot added the back-end label May 9, 2021

vercel bot temporarily deployed to Preview May 9, 2021 20:27 Inactive

[pre-commit.ci] auto fixes from pre-commit.com hooks

e259fc1

for more information, see https://pre-commit.ci

vercel bot temporarily deployed to Preview May 9, 2021 20:27 Inactive

ad-m reviewed May 9, 2021

View reviewed changes

backend-project/small_eod/notifications/views.py Outdated Show resolved Hide resolved

ad-m reviewed May 9, 2021

View reviewed changes

backend-project/small_eod/users/models.py Show resolved Hide resolved

ad-m reviewed May 9, 2021

View reviewed changes

backend-project/small_eod/users/models.py Outdated Show resolved Hide resolved

NameeLesS added 2 commits May 14, 2021 02:39

add notify method in User model

83325b9

Merge branch 'dev' of https://github.com/NameeLesS/small_eod into dev

9e4feff

vercel bot temporarily deployed to Preview May 14, 2021 01:10 Inactive

[pre-commit.ci] auto fixes from pre-commit.com hooks

63826d3

for more information, see https://pre-commit.ci

vercel bot temporarily deployed to Preview May 14, 2021 01:10 Inactive

ad-m reviewed May 14, 2021

View reviewed changes

backend-project/small_eod/notifications/utils.py Show resolved Hide resolved

ad-m reviewed May 14, 2021

View reviewed changes

backend-project/small_eod/notifications/utils.py Outdated Show resolved Hide resolved

ad-m reviewed May 14, 2021

View reviewed changes

backend-project/small_eod/notifications/views.py Outdated Show resolved Hide resolved

ad-m reviewed May 14, 2021

View reviewed changes

backend-project/small_eod/notifications/views.py Outdated Show resolved Hide resolved

add tests, notifications content

b825181

vercel bot temporarily deployed to Preview June 6, 2021 22:30 Inactive

ad-m reviewed Jun 7, 2021

View reviewed changes

backend-project/small_eod/notifications/views.py Outdated Show resolved Hide resolved

ad-m reviewed Jun 7, 2021

View reviewed changes

backend-project/small_eod/notifications/views.py Outdated Show resolved Hide resolved

ad-m reviewed Jun 7, 2021

View reviewed changes

add more notification tests and mails content change

c405e21

vercel bot temporarily deployed to Preview July 11, 2021 23:18 Inactive

NameeLesS marked this pull request as ready for review July 20, 2021 00:32

Merge branch 'dev' of https://github.com/watchdogpolska/small_eod int…

6db55e8

…o dev

vercel bot deployed to Preview July 28, 2021 20:45 View deployment

[pre-commit.ci] auto fixes from pre-commit.com hooks

bd6b1ea

for more information, see https://pre-commit.ci

vercel bot temporarily deployed to Preview July 28, 2021 20:47 Inactive

ad-m approved these changes Jul 28, 2021

View reviewed changes

rwakulszowa reviewed Jul 29, 2021

View reviewed changes

Adjust changes according to code review

ab614e7

vercel bot temporarily deployed to Preview August 4, 2021 15:16 Inactive

NameeLesS commented Aug 4, 2021

View reviewed changes

Resolved merge conflicts

b4a7abe

vercel bot temporarily deployed to Preview August 5, 2021 16:24 Inactive

ad-m merged commit c873964 into watchdogpolska:dev Aug 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Powiadomienie do obserwujących sprawę #934

Powiadomienie do obserwujących sprawę #934

NameeLesS commented May 9, 2021

ad-m commented May 11, 2021

NameeLesS commented May 11, 2021 •

edited

Loading

ad-m commented May 12, 2021

ad-m Jun 7, 2021

ad-m Jun 7, 2021

ad-m commented Jun 7, 2021

rwakulszowa commented Jun 16, 2021

ad-m commented Jul 12, 2021

NameeLesS commented Jul 20, 2021

ad-m commented Jul 20, 2021

ad-m commented Jul 26, 2021

NameeLesS commented Jul 27, 2021

ad-m commented Jul 27, 2021

NameeLesS commented Jul 28, 2021

ad-m left a comment

rwakulszowa left a comment

rwakulszowa Jul 29, 2021

NameeLesS Jul 30, 2021

rwakulszowa Jul 29, 2021

rwakulszowa Jul 29, 2021

NameeLesS Jul 29, 2021

ad-m Aug 5, 2021

rwakulszowa Jul 29, 2021

rwakulszowa Jul 29, 2021

rwakulszowa Jul 29, 2021

rwakulszowa Jul 29, 2021

NameeLesS Aug 1, 2021

ad-m Aug 5, 2021

rwakulszowa Jul 29, 2021

NameeLesS Jul 30, 2021

rwakulszowa Jul 29, 2021

NameeLesS Aug 4, 2021



		class MailTemplate:
		def __init__(self, txt_path, html_path=None):

		from rest_framework.views import APIView


		class SendNotificationsMixin(APIView):

Powiadomienie do obserwujących sprawę #934

Powiadomienie do obserwujących sprawę #934

Conversation

NameeLesS commented May 9, 2021

ad-m commented May 11, 2021

NameeLesS commented May 11, 2021 • edited Loading

ad-m commented May 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ad-m commented Jun 7, 2021

rwakulszowa commented Jun 16, 2021

ad-m commented Jul 12, 2021

NameeLesS commented Jul 20, 2021

ad-m commented Jul 20, 2021

ad-m commented Jul 26, 2021

NameeLesS commented Jul 27, 2021

ad-m commented Jul 27, 2021

NameeLesS commented Jul 28, 2021

ad-m left a comment

Choose a reason for hiding this comment

rwakulszowa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NameeLesS commented May 11, 2021 •

edited

Loading