-
Notifications
You must be signed in to change notification settings - Fork 6
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
Přepisování údajů dárce #129
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Celkově se mi to líbí. Některé věci jsou lehce jinak než u jiných view, ale to není problém a neznamená to, že je to špatně.
Jen tu chybu s přeuložením je potřeba opravit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Jen pár komentů, když jsem šel kolem.
|
||
@blueprint.get("/override/all") | ||
@login_required | ||
def get_overrides(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Velmi zajímavá implementace. Původní myšlenka byla, že pokud má dárce aktivní override, tak se jen zvýrazní celý řádek v tabulce, ale tohle je zajímavější, i když trošku komplexnejší.
Pozor na to, že stejné zvýraznění je třeba udělat v tabulkách pro přípravu a udělování ocenění.
Konečně jsem se k tomu dokopal :) |
A jsme ready na review? |
Ještě tam musím zakomponovat #133. |
Zkoušela jsem to proklikat, vypadá to dobře, ale možná bych u toho zvýraznění zvolila trochu více kontrastní barvu. Na té šedé je to vcelku špatně vidět. |
Super, teď už ta barva vypadá lépe. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Funkčně se mi to moc líbí. Ke kódu jsem napsal jen pár komentářů. JS moc nemám rád, ale tady plní celkem hezky svou funkci, tak snad se to brzy nepokazí. Jen bych ještě poprosil, zda by šlo dát pár vzorových overrides do testovacích dat.
Jinak už zbývá jen udělat rebase a vyřešit konflikty, tak snad ti nový datatables backend nezpůsobí moc potíží.
- přidána tabulka donors_override - přidán formulář pro obsluhu tabulky Záznamy z tabulky donors_override se zatím neprojeví v donors_overview.
Data z tabulky donors_overview se přepisují daty z tabulky donors_override.
- nešlo vícekrát uložit (přepsat) záznam u jednoho dárce - u sloupců, které měly záznam v donors_override, který byl následně smazán, se záznam v donors_overview nepřepsal na původní hodnotu (mějme např. Jana Nováka, kterého přejmenujeme na Josefa Vomáčku. Když smažeme ve formuláři donors_override křestní jméno a uložíme, zůstane dárce jako Josef Vomáčka a ne Jan Vomáčka, jak jsme chtěli)
Co-authored-by: Lumír 'Frenzy' Balhar <[email protected]>
Přidáno zvýraznování dárců se záznamem v donors_override.
- změněny verze migrací - porovnávání verzí SQLite změněno na porovnávání tuplů Co-authored-by: Lumír 'Frenzy' Balhar <[email protected]>
V HTML byl formulář zachován jako flexbox, ale formuláře pro uložení a smazání výjimky byly sloučeny zpět do jednoho (WebTest si neuměl poradit s formulářovými prvky, které jsou mimo formulář a jsou s ním propojené přes atribut `form`).
Upravena validace formuláře pro přepisování udajů dárce, aby používala NumericValidator.
- funkce DonorsOverrideForm.init_fields nic nevrací - mv DonorsOverrideForm.{_,}get_field_data - funkce DonorsOverrideForm.get_field_data se chová smysluplněji - vrací výslednou hodnotu místo toho, aby ji ukládala - volá se samostatně místo toho, aby se volala automaticky při validaci - byl zkrácen její kod - test pro DonorsOverride získává informace z databáze a ne regexem ze stránky - view donor/get_overrides nevolá zbytečně str() na string - styl pro podbarvení elementu <mark> byl přesunut na sjednocené místo
K čemu by tam byly? Vzoroví dárci tam jsou proto, aby se na nich daly zkoušet různé funkcionality, ale u vzorových overridů žádné takové využití nevidím. |
Když si spustíš instanci pro vývoj, je dobré tam mít všechny funkcionality aktivní a i s nějakými daty, takže v případě problémů hned vidíš, kde co je. Také pro testy se to hodí, i když ty si mohou libovolně přidat a odebrat cokoli je pro daný test třeba. Napadá mě, že to tam vlastně nemáme ani pro poznámky nebo ignorované dárce, takže se na to klidně vykašli a já to tam pak dodělám všechno najednou. Mrknu později ještě jednou na kód, vyzkouším ručně a myslím si, že budeme připraveni na sloučení. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Je to téměř dokonalé! Díky! Našel jsem jen poslední drobnosti. Pokud na ně nemáš čas, klidně napiš a já to PR dokončím. Jinak je to funkčně velmi povedené.
Jako poslední přípravu na merge bych asi sloučil všechny commity do jednoho a udělal rebase na aktuální master.
|
||
def to_dict(self): | ||
result = {} | ||
for field in [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bylo by super to udělat tak, aby se seznam vstupů nemusel udržovat na dvou místech - DonorsOverride model a DonorsOverrideForm.
function highlightOverridenValues(url, columnDefs, onDataReady) { | ||
let overrides = {}; // Will be set by an AJAX request | ||
|
||
for (const column of ["first_name", "last_name", "address", "city", "postal_code", "kod_pojistovny"]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kdyby se tento seznam dal předat jako parametr funkce, dal by se renderovat v šabloně, kde se ta funkce volá a ubylo by další místo, kde je třeba jej udržovat.
Teď o víkendu bych se k tomu mohl dostat. Dej vědět, jestli mám nebo ti to mám nechat. |
Zrovna nejsem doma a nebudu tam ještě asi týden, takže se na to můžeš vrhnout.
-------- Original Message --------
…On Aug 20, 2021, 09:21, frenzymadness wrote:
Teď o víkendu bych se k tomu mohl dostat. Dej vědět, jestli mám nebo ti to mám nechat.
—
You are receiving this because you were assigned.
Reply to this email directly, [view it on GitHub](#129 (comment)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/AGKKSXXNN2ATEQZNW6ZXTHLT5X7BJANCNFSM45PJQESQ).
Triage notifications on the go with GitHub Mobile for [iOS](https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675) or [Android](https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email).
|
Přepínám tento na draft, protože je nahrazen novým PR #162 . Zavřeme je pak společně. |
Přidává možnost přepsat osobní údaje dárce.
Closes #73
Depends on #133
TODO: