odevzdavatko: odesílání emailu řešiteli při změně zpětné vazby #83
Loading…
Reference in a new issue
No description provided.
Delete branch "notifikace-zpetne-vazby"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Toto se rozbíjí, když dojde ke smazání hodnocení v pořadí dříve, než
nějaké hodnocení s neprázdnou zpětnou vazbou, neboť řádky formsetu jsou
přečíslovány a pak špatně spárovány s původními hodnotami, takže se
nesprávně detekuje změna.
Zkoušel jsem na testwebu. Vypadá velmi dobře1. Jdu číst kód.
Jediná chyba, kterou jsem zatím objevil, je, že když org smaže poslední hodnocení k danému řešení (což není normální postup, ale stále to umožňujeme), pak URL adresa vede na stránku, která vyhazuje chybu. (Což samozřejmě není chyba tohoto pullrequestu, ale ale možná by to chtělo vyřešit, než ho mergeneme.) ↩︎
Jo, napadá mě, co když org nepíše žádnou zpětnou vazbu, ale pouze dává body. (Pak to řešiteli nepošle žádný e-mail. Obdobně, když org nejprve přidá zpětnou vazbu a o několik dní později teprve nasází body.)
Asi bych to tak nechal…
Za mě OK :)
@ -0,0 +13,4 @@migrations.AddField(model_name='resitel',name='upozorneni',field=models.BooleanField(default=True, verbose_name='zasílat upozornění na změnu zpětné vazby k řešení emailem'),Za mě spíš
default=False, je to změna chování webu a těch mailů potenciálně může být docela hodně… Ale je to asi otázka spíš mého osobního názoru (věci nemají random měnit chování a vývojáři nemají diktovat uživatelům, co chtějí) než něčeho striktního, klidně mě přehlasujte…Za mě
default=True.Klidně tím způsobem, že v migraci je všechny nastavíme na False a s příštím číslem (a na discordu a v novince) pošleme informaci.
Však taky komentuji jen migraci a ne formulář – myslím, že máme na mysli to samé :-)
Jakože by to bylo
Truepro nové řešitele ale pro již existujícíFalse?Přesně tak :)
Ale podle mě to nejde udělat tím, že v migraci napíš
default=False(to dle mého změní default tabulky, tedy django pořád bude chtít vyrobit další migraci), nebo ne?Je pravda, že pak můžu vyrobit druhou migraci, kterou se to vyřeší…
Tak do tabulky se tak jak tak uloží ten boolean, který přijde ve formuláři, tedy pokud má formulář
initial=True(což iirc má), tak se tadydefaultpoužije jen pro stávající řádky tabulky v rámci migrace (a ano, možná tam někde bude poznamenaný, ale nikdy se nepoužije…).Pokud chceme být hodně explicitní, tak můžeme nastavit
default=Truea pak napsat něco typuResitel.objects.update(notifikace_o_zpetne_vazbe=False), ale to mi přijde zbytečně překomplikované…A můj přístup by byl z
models.pydefaultzrušit úplně a naopak nastavitnull=False. To vynutí, že ta tabulka bude vždycky chtít explicitní nastavení při přidání řádku adefaultv migraci se použije jen pro tu migraci… (Speciálně: teď nevím, jestli staré řádky náhodou v DB nebudou mít NULL místo False, ale čekám, že se ten default spíš projeví.)Zvolil jsem variantu s
default=Truea změnou existujících řádků v rámci migrace právě proto, že mi přijde nejvíce explicitní.Mně se to líbí. LEdo?
LEdo lgtm…
@ -250,6 +250,8 @@ class Resitel(SeminarModelBase):poznamka = models.TextField('neveřejná poznámka', blank=True,help_text='Neveřejná poznámka k řešiteli (plain text)')upozorneni = models.BooleanField('zasílat upozornění na změnu zpětné vazby k řešení emailem', default=True)Ideálně asi pojmenovat jinak, tohle potenciálně není jediné upozornění, které chceme posílat. Navrhuji
upozornovat_na_opravy_reseniAž na ty dvě drobnosti za mě taky OK.
A ještě jeden komentář, spíš do budoucna: Asi budeme někdy chtít mít vyloženě nějaká uživatelská nastavení (posílání notifikací, výchozí filtr v odevzdávátkové tabulce, typ frontendu korekturovátka, atp.) a tohle je asi jedna z věcí, které by tam mohly přijít. Ale asi teď neřešme, kde a jak tohle má být kam navázané…
Pokud to takto vyhovuje a nemáte již jiné připomínky, mohu udělat rebase a při něm squash.
To rozhodně není potřeba (na hezký git si tu rozhodně nehrajeme :) )