Powiadomienie do obserwujących sprawę

[ad-m]

Na modelu Case mamy pole notified_users. W przypadku zmian wokół Letter, Note, Event, a także samych Case powiązanych z daną Case powinniśmy wysłać powiadomienie e-mailowe do użytkowników wskazanych przeznotified_users.

Kod wymaga rozsądnego pokrycia testami.

Obecnie Poradnia ma w tym zakresie zgrabną strukturę kodu, jak dobrze pamiętam. Warto się zainspirować.

[ad-m]

@NameeLesS , masz ochotę?

[NameeLesS]

yhm, mogę spróbować

[ad-m]

Jeżeli chcesz to możemy porozmawiać wieczorem i ci objaśnię jak to wykonać :)

[NameeLesS]

Na razie zrobię research, jeżeli nie będę miał pomysłu jak do tego podejść to napiszę

[ad-m]

Nie ma problemu, abyśmy porozmawiali, dzięki temu będzie to zrobione szybciej i będzie można przejść do kolejnych kroków, ale nie narzucam się.

[NameeLesS]

Dobrze możemy porozmawiać

[NameeLesS]

@ad-m ostatnio mówiłem, że chciałbym, aby to jakie powiadomienie w jakim przypadku wysłać nie było hardcoded w utilsie do wysyłania maili tak jak to jest w poradni https://github.com/watchdogpolska/poradnia/blob/7f58cfe5a8c906b334d58751b6dff505198e5c2e/poradnia/template_mail/utils.py#L78
a ustalane przy danym viewsecie ale ty chyba miałeś jakieś zastrzeżenia co do tego?

[ad-m]

Moim zdaniem optymalnie byłoby, gdyby ViewSets wysyłał wywołanie send na użytkownika z określeniem rodzaju operacji, modelu i innych kontekstowych informacji. Następnie send określi czy wiadomość jest w ogóle wspierana przez system, a jeżeli tak to – jeżeli użytkownik wyraził taką wolę w swoich opcjach konfiguracyjnych (do wprowadzenia później) – wyrenderowana i wysyłana.

W Poradni mieliśmy odrobinę inne podejście, że send jest wywoływane wtedy i tylko wtedy, gdy wiadomość jest wspierana, co powoduje utrudnienie w np. konfiguracji czy użytkownik powinien dostać powiadomienie.

[NameeLesS]

Nie do końca zrozumiałeś moje pytanie, dlatego może wyjaśnię co zamierzam.

Obecnie mam stworzonego mixina SendNotificationsMixin, którego należy dziedziczyć z klasy, która dziedziczy z ViewSet

class EventViewSet(viewsets.ModelViewSet, SendNotificationsMixin):
	notified_users = "case.notified_users"
	#...

mixin ten obecnie sprawdza wprowadzone zmiany i zwraca je w postaci dicta

{
   "name":{
      "added": None,
      "removed": None,
      "changed":{
         "from":"case-0002",
         "to":"case-00022"
      }
   },
   "tags":{
      "added":[
         <Tag:tag-0021>
      ],
      "removed":[
         <Tag:tag-0006>,
         <Tag:tag-0008>
      ],
      "changed": None
   }
}

chciałbym, aby w danym ViewSet dziedziczącym po SendNotificationsMixin znajdował się field template_map który przyjmował by dicta. Dict ten mieściłby w sobie path do template który powinien zostać użyty w momencie danej zmiany na danym fieldzie

class CaseViewSet(viewsets.ModelViewSet, SendNotificationsMixin):
    notified_users = "notified_users"
    template_map = {
	    "put": {
	    	"tags": {
	    		"added": ["template/tag_added1", "template/tag_added2"],
	    		"removed": ["template/tag_removed"],
	    		"multi": ["template/tag_multiple_changes"]
	    	},
	    	"multi": ["template/case_updated"]
	    },
	    "delete": ["template/case_deleted"],
	    "post": ["template/case_created"]
    }

można by było użyć multi do określenia template w przypadku wielu zmian jednocześnie zarówno dla wielu zmian na danym field jak i dla zmian na wielu field. Imo będzie to dość elastyczne rozwiązanie dlatego że będzie można tworzyć zarówno ogólne jak i bardzo szczegółowe powiadomienia, bez większych problemów. Przewidziałem również możliwość dodania własnych triggerów powiadomień.

I tak jak pisałeś ViewSet będzie pobierał userów z pola określonego w notified_users i na każdym z tych userów wykonywał send przekazując path do templateu, rodzaj operacji i inne kontekstowe informacje.

send na modelu User będzie zaś decydował, czy powinien wysłać powiadomienie (czyt. czy dany user życzy sobie takiego powiadomienia) oraz jakiego rodzaju ma to być powiadomienie email/push.

I teraz moje pytanie, czy posiadasz jakieś zastrzeżenia co do tego podejścia? Widzisz coś czego ja nie widzę a co należało by uwzględnić?

[ad-m]

Z mojej perspektywy jest to...przekombinowane.

Po pierwsze, najważniejszymi powiadomienia są aktualizacje. Niewykluczone, że o zmianach nie potrzebujemy wcale powiadomień. Przykładowo GitHub nie powiadamia o zmianie, gdy edytujesz komentarz. Skupmy się na podstawowym rozwiązaniu i go dostarczmy.

Po drugie, trudno mi dostrzec praktyczność pisania tyle szablonów dla każdego pola. Jeżeli już to jeden, który wskazuje na zakres zmian. Przykładowo w stylu BitBucket:

Co sądzisz o tym, aby rozszerzyć model User o get_enabled_notification, który zwraca aktywne powiadomienia dla użytkownika (obecnie sztywne wyliczenie kluczy), wykorzystywać te same klucze do emitowania powiadomień, a identyfikatory szablonów generować?

Budowanie struktury, która podejście do wskazania zakresu zmian w aktualizacji pozostawiłbym na #156 , co pewnie wydarzy się dopiero za pewien czas.

[NameeLesS]

(obecnie sztywne wyliczenie kluczy), wykorzystywać te same klucze do emitowania powiadomień, a identyfikatory szablonów generować? Nie do końca rozumiem co masz na myśli.

[ad-m]

Zrób szkic PR ( https://github.blog/2019-02-14-introducing-draft-pull-requests/ ), spróbuje to przedstawić subtelnymi zmianami.