feature/invoicer-merge #21

Open
wikiker wants to merge 13 commits from feature/invoicer-merge into dev
Owner

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).

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).
- Added bank_transaction_list view for listing all bank transactions
- Added record_bank_transactions view for finding matching transactions
- Added merge_bank_transaction view for simple merging
- Added merge_bank_transaction_confirm view for conflict resolution
- Added URL routes for all new views
- Created templates: bank_transaction_list.html, record_bank_transactions.html, merge_bank_transaction_confirm.html
- Features:
  - Filter bank transactions by variable symbol, constant symbol, counter account
  - Match bank transactions to records by variable symbol, amount, or date
  - Handle conflicts when bank transaction already has a record
  - Three options for conflict: delete old, keep both, cancel
- Added Bank Transactions link to nav menu in _base.html
- Added Spojit button to record detail page
- Added missing Q import in views.py
- Added 14 tests for bank transaction views:
  - BankTransactionListViewTest (5 tests)
  - RecordBankTransactionsViewTest (3 tests)
  - MergeBankTransactionViewTest (6 tests)
- All 49 tests pass
- Handle variable symbols with/without dashes
- Handle amount formats (different decimal separators)
- Handle date formats (DD.MM.YYYY and YYYY-MM-DD)
- Add filters_applied flag for template
- More robust matching in record_bank_transactions view
- Handle null record.code in filtering (code can be null=True)
- Add show_all parameter to record_bank_transactions view
- Show all unassociated transactions when no matches found
- Add 'Show all' link in template when no matches
- Simplified merge confirmation template
- Removed separate merge_bank_transaction_confirm template
- Added inline Metro UI dialog in record_bank_transactions template
- Dialog pops up when clicking on already-associated bank transactions
- Updated merge_bank_transaction_confirm view to handle POST from dialog
- Updated test to reflect new behavior (GET redirects, POST processes)
- Deleted new_merge.html
- Previously 'keep' just removed association, now it moves to new record
- Updated test to reflect new behavior
- All 50 tests pass
wikiker changed target branch from main to dev 2026-10-24 11:31:30 +02:00
wikiker changed title from WIP: feature/invoicer-merge to feature/invoicer-merge 2026-10-24 11:36:07 +02:00
wikiker force-pushed feature/invoicer-merge from bacd4bc442 to 907911ba92 2026-10-24 11:40:22 +02:00 Compare
- Removed merge_bank_transaction_confirm URL
- Removed merge_bank_transaction_confirm view function
- Updated merge_bank_transaction view to handle POST with action parameter
- Updated all tests to use merge_bank_transaction URL directly
- All 49 tests pass
Author
Owner

@setnicka omrkl bys prosím?

@setnicka omrkl bys prosím?
wikiker self-assigned this 2026-10-24 12:36:11 +02:00
setnicka left a comment

Dobrý základ 👍

Z původního popisu #17 zatím chybí:

  • filtr na nespárované platby, případně spárované jen se záznamem z importu (bez čísla dokladu), protože to jsou kandidáti na duplicity,
  • sloupec se zprávou pro příjemce (message), u darů a poplatků je v ní často účel platby a teď je vidět jen v adminu,
  • filters_applied se 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?

Dobrý základ 👍 Z původního popisu #17 zatím chybí: - filtr na nespárované platby, případně spárované jen se záznamem z importu (bez čísla dokladu), protože to jsou kandidáti na duplicity, - sloupec se zprávou pro příjemce (`message`), u darů a poplatků je v ní často účel platby a teď je vidět jen v adminu, - `filters_applied` se 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>
Collaborator

Tohle tlačítko by měl vidět jenom ten, kdo má právo tu operaci udělat.

Tohle tlačítko by měl vidět jenom ten, kdo má právo tu operaci udělat.
Author
Owner

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.

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.
Collaborator

Tady by stačilo jenom {% if perms.budget.accountant %}. Celkové vylepšení práv bych pak udělal někdy později.

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 asociaci
Collaborator

Popisek neodpovídá tomu, co akce keep dě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.

Popisek neodpovídá tomu, co akce `keep` dě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.
Author
Owner

Upraveno, přidáno víc komentářů a zpráv

Upraveno, přidáno víc komentářů a zpráv
setnicka marked this conversation as resolved
@ -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áznam
Collaborator

Nemě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.record podle 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.

Nemě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.record` podle 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.
Author
Owner

Mělo by být opraveno

Mělo by být opraveno
Author
Owner

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í.

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í.
setnicka marked this conversation as resolved
@ -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/';
Collaborator

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ř. do data-merge-url). Smazaná route by pak při renderu spadla na NoReverseMatch a chytil by to i test.

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ř. do `data-merge-url`). Smazaná route by pak při renderu spadla na `NoReverseMatch` a chytil by to i test.
setnicka marked this conversation as resolved
@ -449,0 +464,4 @@
counter_account = request.GET.get("counter_account", "").strip()
# Start with all bank transactions
transactions = BankTransaction.objects.all().order_by("-date")
Collaborator

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).

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).
Author
Owner

Ř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.

Ř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.
setnicka marked this conversation as resolved
@ -449,0 +551,4 @@
queries |= Q(date__icontains=dt)
# Get matching transactions, ordered by date
transactions = BankTransaction.objects.filter(queries).order_by("-date")
Collaborator

Tohle matchování jsi reálně zkoušel? Podle mě to moc fungovat nebude:

  1. record.amount je DecimalField(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 v BankTransaction. Podle částky se tak nenajde nic, test prochází jen díky shodě VS.
  2. Pozor na datum: staré CSV importy z Fio mají datum jako 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é.
  3. VS se porovnává s číslem dokladu přes icontains, takže 2026-101 najde i VS 20261010–20261019 a 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 🤷

Tohle matchování jsi reálně zkoušel? Podle mě to moc fungovat nebude: 1. `record.amount` je `DecimalField(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 v `BankTransaction`. Podle částky se tak nenajde nic, test prochází jen díky shodě VS. 2. Pozor na datum: staré CSV importy z Fio mají datum jako `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é. 3. VS se porovnává s číslem dokladu přes `icontains`, takže `2026-101` najde i VS `20261010`–`20261019` a 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 🤷
Author
Owner

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.

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.
Collaborator

Souhlas, založil jsem na to issue #25.

Souhlas, založil jsem na to issue #25.
setnicka marked this conversation as resolved
@ -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 directly
return redirect("budget:record_bank_transactions", pk=record_id)
Collaborator

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á action by měla vracet 400, teď propadne sem a platbu taky spojí.

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á `action` by měla vracet 400, teď propadne sem a platbu taky spojí.
Author
Owner

Mělo by být opraveno

Mělo by být opraveno
setnicka marked this conversation as resolved
setnicka left a comment

Díky za opravy :) Ještě jsem našel prá věcí, hlavně jedno nebezpečí XSS a pak nešikovné vypisování None ve stránce a v hláškách, pokud záznam nemá kód.

Díky za opravy :) Ještě jsem našel prá věcí, hlavně jedno nebezpečí XSS a pak nešikovné vypisování `None` ve 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>">
Collaborator

Tohohle jsem si předtím nevšiml 🙈

data-title-caption vklá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.

Tohohle jsem si předtím nevšiml 🙈 `data-title-caption` vklá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 }}"
Collaborator

Obecně u všech použití .code na 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 Record něco jako label:

# models.py
class Record(models.Model):
    ...
    @property
    def label(self):
        """Identification for UI; records without a real code get their ID and date."""
        if (self.code or "").strip().lower() not in ("", "-", "tbd"):
            return self.code
        d = self.date
        prefix = f"#{self.pk} " if self.pk else ""
        return f"{prefix}bez čísla ({d.day}. {d.month}. {d.year})"

A když se k tomu přidá i __str__, půjde všude psát jen {{ bt.record }}:

    def __str__(self):
        return self.label
Obecně u všech použití `.code` na 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 `Record` něco jako `label`: ```python # models.py class Record(models.Model): ... @property def label(self): """Identification for UI; records without a real code get their ID and date.""" if (self.code or "").strip().lower() not in ("", "-", "tbd"): return self.code d = self.date prefix = f"#{self.pk} " if self.pk else "" return f"{prefix}bez čísla ({d.day}. {d.month}. {d.year})" ``` A když se k tomu přidá i `__str__`, půjde všude psát jen `{{ bt.record }}`: ```python def __str__(self): return self.label ```
@ -449,0 +511,4 @@
transactions = BankTransaction.objects.select_related("record")
show_all = True
context = {
Collaborator

Přidal bych ještě řazení od nejnovějších operací (operation_id by mělo ve Fio bance jenom růst):

transactions = transactions.order_by("-operation_id")
Přidal bych ještě řazení od nejnovějších operací (`operation_id` by mělo ve Fio bance jenom růst): ```python transactions = transactions.order_by("-operation_id") ```
@ -449,0 +573,4 @@
bt.save()
if action == "delete" and old_record is not None:
old_record.delete()
Collaborator

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_count už počítáš, takže bych delete odmítl, pokud other_count > 0.

A navíc, komentář o pár řádků výš mluví o CASCADE, ale FK v databázi je SET_NULL.

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_count` už počítáš, takže bych `delete` odmítl, pokud `other_count > 0`. A navíc, komentář o pár řádků výš mluví o `CASCADE`, ale FK v databázi je `SET_NULL`.
This pull request doesn't have enough approvals yet. 0 of 1 approvals granted.
You are not authorized to merge this pull request.
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin feature/invoicer-merge:feature/invoicer-merge
git switch feature/invoicer-merge
Sign in to join this conversation.
No reviewers
No labels
No milestone
No project
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Reference
wikiker/helenka!21
No description provided.