feature/invoicer-merge #21
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feature/invoicer-merge"
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?
Vytváříme UI na propojování bankovních transakcí a záznamů. Něco už funguje, ještě to chce trochu doladit. Je snaha o nějaké filtrování relevantních záznamů k propojení, fallback jsou všechny záznamy (v nich by se mělo dát fulltextově vyhledávat).
WIP: feature/invoicer-mergeto feature/invoicer-mergebacd4bc442to907911ba92@setnicka omrkl bys prosím?
Dobrý základ 👍
Z původního popisu #17 zatím chybí:
message), u darů a poplatků je v ní často účel platby a teď je vidět jen v adminu,filters_appliedse v šabloně nepoužívá, slibovaná hláška při prázdném výsledku tam není.Plánuješ párování i ze strany platby (u volné platby „Spárovat“ → výběr záznamu), nebo to necháme na další PR?
@ -17,0 +18,4 @@class="button primary"title="Spojit s bankovní transakcí"><span class="mif-link"></span> Spojit</a>Tohle tlačítko by měl vidět jenom ten, kdo má právo tu operaci udělat.
Autorizaci asi budeme chtít předělat nějak víc od základu, teď to neřeším, je to na separátní issue.
Tady by stačilo jenom
{% if perms.budget.accountant %}. Celkové vylepšení práv bych pak udělal někdy později.@ -0,0 +196,4 @@value="keep"class="button warning"title="Odebere asociaci, obě transakce zůstávají"><span class="mif-minus"></span> Odebrat asociaciPopisek neodpovídá tomu, co akce
keepdělá: platbu neodpojí, ale přesune k tomuto záznamu a starý záznam nechá s plnou částkou bez platby. Když je to záznam z importu, zůstane duplicita v součtech.Navrhuju přejmenovat na „Přesunout platbu, starý záznam ponechat“ a v hlášce po akci odkázat na starý záznam, ať je jasné, že ho má uživatel zkontrolovat.
Upraveno, přidáno víc komentářů a zpráv
@ -0,0 +203,4 @@value="delete"class="button danger"title="Smaže existující záznam a spojí transakci s {{ record.code }}"><span class="mif-bin"></span> Smazat starý záznamNemělo by tohle mít nějakou konfirmaci (javascriptový
confirm())? Je to destruktivní operace, na kterou nechceš kliknout omylem.A pozor na souběh: view maže
bank_transaction.recordpodle stavu v DB v okamžiku odeslání, ne podle toho, co ukazoval dialog. Když platba patří k X a dva uživatelé ji současně přepárovávají k Y, první smaže X a druhý pak smaže samotné Y (včetně dokladu) a platba zůstane bez záznamu.Navíc dva uživatele A a B lze nahradit tím, že si to prostě otevřeš dvakrát ve dvou tabech, to už mi přijde docela pravděpodobné.
Podobné formuláře by vždy měly dělat nějakou kontrolu: stačí, aby dialog posílal ID záznamu, na který ukazoval (
expected_record_id), a view akci odmítl, pokud se mezitím změnil.Mělo by být opraveno
Mělo by snad být opraveno.
Kódu na souběh věřím středně, testovat to jde hůře (testy probíhají skoro atomicky), ale snad je tam trochu zlepšení.
@ -0,0 +226,4 @@window.openMergeConfirm = function (el, recordId, btId) {/* Set form action */document.getElementById('merge-confirm-dialog').action ='/record/' + recordId + '/merge-bank-transaction/' + btId + '/confirm/';Tahle URL po posledním commitu už neexistuje, takže obě tlačítka dialogu končí 404. Ideálně by se neměla skládat v JS, ale přes
{% url 'budget:merge_bank_transaction' record.pk bt.pk %}v šabloně (např. dodata-merge-url). Smazaná route by pak při renderu spadla naNoReverseMatcha chytil by to i test.@ -449,0 +464,4 @@counter_account = request.GET.get("counter_account", "").strip()# Start with all bank transactionstransactions = BankTransaction.objects.all().order_by("-date")Pozor na to, že staré záznamy (importovaná CSV) mají date ve formátu "12.03.2026", takže tohle řazení fungovat nebude.
Možná by stálo za to mít to uložené ne jako text, ale jako opravdové datum (a zkonvertovat starší záznamy).
Navíc: Šablona na každém řádku sahá na
bt.record, takže se pro každou spárovanou platbu pošle extra dotaz. Stačíselect_related("record")tady (a i na dalších řádcích, kde se to vytahuje, tedy 520, 554 a 558).Řazení dle date jsem odstranil všude, máš pravdu.
Views jsem zrychlil, chtělo by to ještě zkontrolovat kód podrobněji a zjistit, kde se nám vyplatí udělat JOIN. Hlavně jsem přidal selected_records k overview, tam to asi dost zpomalovalo.
@ -449,0 +551,4 @@queries |= Q(date__icontains=dt)# Get matching transactions, ordered by datetransactions = BankTransaction.objects.filter(queries).order_by("-date")Tohle matchování jsi reálně zkoušel? Podle mě to moc fungovat nebude:
record.amountjeDecimalField(decimal_places=4), takže z DB vypadne vždy se 4 desetinnými místy (-1234.5600) a to nesedí na žádný formát částky vBankTransaction. Podle částky se tak nenajde nic, test prochází jen díky shodě VS.12.03.2026. To možná můžeme ignorovat, staré záznamy nemusíme řešit. Větší problém je, že Fio účtuje karetní platby (a občas i pozdě zadané převody) až další den, takže bych hledal i o pár dní posunuté.icontains, takže2026-101najde i VS20261010–20261019a kód-najde všechno. Tady by mělo být přesné porovnání.Navíc výsledky nejsou seřazené od nejvíc odpovídající (typicky shoda VS).
Co vzít kandidáty jako platby z nějakého rozsahu data + platby s přesně shodným VS (faktury se často platí až týdny po vystavení) a projít je v Pythonu? Shoda VS přidá body, shoda částky přidá body, rozdíl dnů ubere a pak seřadit 🤷
Máš pravdu, testoval jsem to jen středně. Nechat icontains na VS je aktuálně nejlepší, co umím jednoduše, zbytek jsem odstranil.
Tvůj přístup je super a bude fajn ho přidat, ale spíš ho nechme na separátní PR, aktuálně mě moc nenapadá konkrétní logika.
Souhlas, založil jsem na to issue #25.
@ -449,0 +607,4 @@# Check if bank transaction already has a different record (GET request)if bank_transaction.record and bank_transaction.record.pk != record.pk:# Redirect to the list with show_all if accessed directlyreturn redirect("budget:record_bank_transactions", pk=record_id)Tady se zapisuje do DB na GET (takže to navíc ani nechrání CSRF).
Takovéhle věci vždy jen POSTem s
{% csrf_token %}(malý formulář místo<a>) a view označit@require_POST.A neznámá
actionby měla vracet 400, teď propadne sem a platbu taky spojí.Mělo by být opraveno
Díky za opravy :) Ještě jsem našel prá věcí, hlavně jedno nebezpečí XSS a pak nešikovné vypisování
Noneve stránce a v hláškách, pokud záznam nemá kód.@ -0,0 +24,4 @@<!-- Record Info --><div class="row"><div class="cell-12"><div class="panel" data-role="panel" data-title-caption="Záznam: <a href='{% url 'budget:record_detail' record.pk %}'>{{ record.code }}</a>">Tohohle jsem si předtím nevšiml 🙈
data-title-captionvkládá Metro UI jako HTML (proto funguje i ten odkaz) a escapování z Djanga se tím ztratí (prohlížeč entity v atributu dekóduje dřív, než si je Metro přečte).Číslo může upravit kdokoliv s právem editovat ten záznam, takže je to riziko XSS (ověřil jsem testem, když se do čísla napíše třeba
<script>…</script>, tak se to vykoná). Do caption bych dával jen pevný text a číslo s odkazem vypsal v těle panelu.@ -0,0 +90,4 @@data-confirm-url="{% url 'budget:merge_bank_transaction' record.pk bt.pk %}"data-existing-id="{{ bt.record.pk }}"data-existing-url="{% url 'budget:record_detail' bt.record.pk %}"data-existing-code="{{ bt.record.code }}"Obecně u všech použití
.codena téhle stránce a v hláškách ve view: co když záznam nemá číslo (typicky záznam z importu)? Pak se zobrazuje „Spojit bankovní transakci se záznamem None“, „Opravdu smazat starý záznam None?“ a podobně.Navrhuju přidat k
Recordněco jakolabel:A když se k tomu přidá i
__str__, půjde všude psát jen{{ bt.record }}:@ -449,0 +511,4 @@transactions = BankTransaction.objects.select_related("record")show_all = Truecontext = {Přidal bych ještě řazení od nejnovějších operací (
operation_idby mělo ve Fio bance jenom růst):@ -449,0 +573,4 @@bt.save()if action == "delete" and old_record is not None:old_record.delete()Tady se smaže i záznam, na kterém visí další platby, a uživatel se to dozví až z hlášky po akci.
other_countuž počítáš, takže bychdeleteodmítl, pokudother_count > 0.A navíc, komentář o pár řádků výš mluví o
CASCADE, ale FK v databázi jeSET_NULL.View command line instructions
Checkout
From your project repository, check out a new branch and test the changes.