Change #1319916 had a related patch set uploaded (by Urbanecm; author: Tomáš Vašíček):

[wikimedia-cz/tracker@master] Implement automated payment system

https://gerrit.wikimedia.org/r/1319916

Posting an automated review of the changes for review/improvements.

AI review of the changes

Review of Gerrit 1319916 — *"Implement automated payment system"* (wikimedia-cz/tracker, PS13) is done. Reviewed against your local checkout at 8e7d0d2a (same Change-Id, identical file list). 33 files, +2919/−68. Currently Verified+2 by jenkins-bot, CR 0, no reviewers assigned, no inline comments.

Caveat up front: tox -e lint and the test suite could not run — PyPI is blocked in the sandbox — so flake8/eslint status is unconfirmed beyond Jenkins' verdict. Also, the attempt recreated and then failed to install .tox/lint, so that directory was removed from your working copy. git status is still clean.

Blockers

1. The existing REST API for expenditures will 500. trackersite/api/serializers.py declares ExpeditureSerializer/ExpeditureAdminSerializer as HyperlinkedModelSerializer with fields = '__all__'. The new payment_info/import_info FKs become HyperlinkedRelatedField(view_name='paymentinfo-detail'/'importinfo-detail'), and neither viewset is registered in api/router.py → ImproperlyConfigured at render time for any expenditure with those fields non-null, i.e. everything created through the new UI. Breaks list *and* detail. Separately, read_only_fields covers only ('accounting_info', 'paid') — payment_type, payment_info, import_info, linked_expenditure are all writable by anyone who can edit the ticket, bypassing the new locking in ExpeditureForm/PaymentService. Clearing import_info re-arms an imported expenditure. No API tests exist, which is why CI is green.

2. Fio API token leaks into logs and cron mail. tracker/fio.py::sync_transactions puts the token in the URL (Fio's design), then catches broad Exception and does logger.exception(f"... {e}") plus report['errors'].append(str(e)). requests exceptions stringify the full URL, so one HTTP error writes the live payment token to the log and to fiosync stdout.

3. process_cofinancing_link deletes the row it then saves. linked_expenditure is a OneToOneField('self', on_delete=CASCADE) set in both directions. In tracker/services.py, expenditure.linked_expenditure.delete() cascades into expenditure itself; the following save(update_fields=['linked_expenditure']) matches zero rows → DatabaseError. Reachable from the admin by switching a co-financing row from "source ticket" to "source account". Untested.

4. Same mutual CASCADE defeats the delete guards. ExpeditureInlineFormSet.clean blocks deleting a paid or internal-transfer row, but deleting the *income* row (allowed while unpaid) cascades into its paired INTERNAL_TRANSFER, which may be imported or paid.

Correctness / risk

5. No timeout= on any Fio HTTP call. Import runs synchronously in the web request — a hung endpoint pins a worker.
6. Payment matching is prefix-ambiguous. expected_msg in message: "WMCZ ticket #1" matches "WMCZ ticket #12". With a matching amount, #12's transaction can mark #1 paid. Anchor it.
7. Silent auto-expiry invites double payment. sync_transactions deletes ImportInfo past due_date, returning rows to "Ready for import" with no trace an order was already sent. The manual re-import path warns about exactly this; the automated one doesn't. Also --expiry defaults to 0 in the command vs expiry_days=1 in the method.
8. Currency lookup silently falls back to CZK. FioPaymentManager.__init__ normalizes FIO_API_TOKENS keys via normalize_account() but leaves FIO_API_CURRENCIES raw, then looks it up with the normalized key — always misses if both settings use bare numbers. normalize_account also hardcodes Fio's 2010.
9. import_expenditures 500s on a missing date — date.fromisoformat(None) raises TypeError, not the caught ValueError.
10. Co-financing source account round-trips through free text — written as f"...from account {x}", recovered via description.split("account "). Also get_source_account() on an INCOME row ignores the selected account.
11. Admin exposes every user's bank accounts. ExpeditureAdminForm doesn't narrow saved_account (the non-admin form does) — dropdown lists all users' account numbers, soft-deleted included.
12. BankAccountDeleteView.delete() is a Django-upgrade landmine — Django 4.0 stopped calling delete(), silently turning soft-delete into hard delete.
13. Global import page is inert — computes ready_expenditures with no POST branch and no form; iterates every unpaid expenditure unpaginated.

Front-end

14. {% trans %} inside a static .js file — static/admin/tracker/ticket/expediture.js renders the literal {% trans "Total" %}. Worked before because the code was inline in the template.
15. Fieldset toggle keyed on translated text — includes('Payment Details') vs legend _('Payment Details (Bank Transfer)'); breaks in any translated admin.
16. Duplicate DOM id expediture_set-wrapper in two templates, plus querySelector(x) || querySelector(x) with the identical selector twice.
17. Totals rendered under the wrong inline — included for prefix expediture_set-2, which is Co-financing; matching by auto-generated prefix breaks if inlines is reordered.
18. Re-import button shows for paid rows — item.get_computed_state.paid resolves to '' (the enum has no .paid), so the condition is always true. Server-side check still rejects it, so UI-only.

Nits

admin.py importing a form mixin from views.py (layering inversion); _generate_xml's local transaction shadowing the django.db import; super(forms.ModelForm, self).save(); mixed raw strings vs PaymentType constants; zip(exps, messages) misattributing errors; undocumented FIO_API_TOKENS/FIO_API_CURRENCIES shape (absent from support/settings.py.template, README, deploy/docs); translated strings interpolated into JS without escapejs; Template MTI from PaymentInfo meaning templates appear in PaymentInfo.objects.all().

Unrelated changes bundled in: the ContentType 'Topic'→'topic' test fixes, weakening test_ticket_timestamps from > to >=, static/ticketform/common.js null-guards, trackertags empty-identifier guards, and the tracker.services.get_request → tracker.utils.get_request move. The move is sensible de-cycling; all would be easier to review separately.

Commit message

Doesn't follow WMF convention: no component: prefix (should be tracker: Implement automated payment system), no Why/What body — instead 70+ squashed subject lines ("Fixes", "fio fix", "Error fix"). Bug: T433769 and Change-Id present, Assisted-by: absent. Attribution is prose; the original author is correctly the commit Author, but trailers would be machine-readable.

Testing

~350 new lines: template validation, CNB mod-11 validator, get_target_account, the get_computed_state machine, process_cofinancing_link (ticket source only), Fio import success + 409, sync matching, revert_import, fiosync output. Reasonable core, but for money-moving code the gaps matter: no view tests at all (nothing asserts the import_expenditures permission is enforced, or that valid_ready_ids rejects an unoffered id); no test for the finding-3 path; no Fio error/expiry-revert tests; no negative matching test (where finding 6's risk lives); _generate_xml output never asserted; ExpeditureForm locking untested.

Recommendation

−1. Findings 1–4 before merge; 5–8 are the most likely to cause a real-money incident. The rest can be follow-ups.

Want me to post this to Gerrit as inline comments plus a −1, or would you rather trim it to the blockers first?

Change #1319940 had a related patch set uploaded (by Urbanecm; author: Urbanecm):

[wikimedia-cz/tracker@master] fix(payment automation): Fix several aspects of the automated payment

https://gerrit.wikimedia.org/r/1319940

Change #1337373 had a related patch set uploaded (by Urbanecm; author: Urbanecm):

[wikimedia-cz/tracker@master] tracker: Claim expenditures before sending them to Fio

https://gerrit.wikimedia.org/r/1337373

Change #1337374 had a related patch set uploaded (by Urbanecm; author: Urbanecm):

[wikimedia-cz/tracker@master] fix(payment automation): Compare the transaction amount as a decimal

https://gerrit.wikimedia.org/r/1337374

Manual review

• The popup after registration (https://tracker2.wikimedia.cz/account/register/details/) still writes into the old bank account field, which we probably want to make read-only (or at least, strongly prioritize the new bank account structure). See T437177.

Change #1337386 had a related patch set uploaded (by Urbanecm; author: Urbanecm):

[wikimedia-cz/tracker@master] api: Do not publish or accept the payment automation fields

https://gerrit.wikimedia.org/r/1337386

An AI review update with the latest changes:

Verdict

Deploy the code, but do not add FIO_API_TOKENS yet. common_settings.py defaults them to {}, so the stack lands inert — process_expenditures bails at "Missing API token" before it ever claims a row, and no money can move. That gives you a safe phase 1. Three things must be fixed before you put real tokens in.

Must fix before tokens go in

1. The Fio token is written to the logs and to the cron mail. sync_transactions puts the token in the URL (Fio's design), then does logger.exception(...{e}) and report['errors'].append(str(e)). fiosync prints every error to stdout. A requests exception stringifies the whole URL. Confirmed:

[PROBE token leak] report errors: ['Account 2000145399/2010: 500 Server Error: ... 
   https://fioapi.fio.cz/v1/rest/periods/SUPERSECRETTOKEN/2026-01-01/2026-01-02/transactions.json']

One 5xx or timeout from Fio puts the live payment token in logs/ and in the cron mail. fio.py:437,498-499.

2. The expiry sweep re-arms orders that Fio already has. This is the most likely route to a double payment. fiosync --expiry defaults to 0, so anything imported but unmatched is reverted the day after its due date, with no trace an order went out. Worse, the *same* sync run does both:

[PROBE expiry] errors: ['... a payment of 1000 matches expenditures 1, 2 equally well. Mark the correct one as paid manually.']
[PROBE expiry] reverted count: 2
[PROBE expiry] e1 import_info after run: None | state: ready

It tells the operator to pair it by hand, then immediately clears both claims and offers them for import again. The claim-before-send work in 1337373 closed the concurrency hole; this reopens it from the other end. fio.py:501-518, fiosync.py:20.

3. Deleting a co-financing income row in the admin destroys its paired transfer, live order and all. linked_expenditure is a mutual CASCADE. ExpeditureInlineFormSet blocks deleting a row that is paid or INTERNAL_TRANSFER — but the INCOME half is neither, so it stays deletable:

[PROBE cascade] transfer still exists: False
[PROBE cascade] orphan ImportInfo rows: 1

The API (perform_destroy) and the public form both block this correctly. Only the admin doesn't, which is where the finance team works. admin.py:30-53.

Should fix before deploy — visible, no money at risk

4. The ticket admin's Total/Accepted is under the wrong section and never updates. id="expediture_set-wrapper" renders twice (confirmed: count: 2) — once around Expenditures in change_form.html, once around Co-financings in expediture.html. expediture.js grabs the first, which has no .total element, so the live sum silently goes nowhere; the server-rendered totals sit under Co-financings. The template also keys off the auto-generated prefix expediture_set-2, which breaks if TicketAdmin.inlines is ever reordered.

5. expediture.js is a static file containing a literal {% trans "Total" %}. It worked when the code was inline in the template. Nothing renders it now.

6. Every staff member sees every user's bank accounts. ExpeditureAdminForm inherits saved_account with queryset=BankAccount.objects.all() and never narrows it, unlike the public form:

[PROBE admin dropdown] options: ['alice-live', 'bob-deleted', 'bob-live']

Soft-deleted accounts included, and the dropdown grows with every user, on every ticket page.

7. A token rotation locks staff out of editing a grant. GrantAdminForm turns source_bank_account into a ChoiceField built from the current FIO_API_TOKENS. An already-stored value that is no longer listed fails validation, so *any* edit to that grant is refused:

[PROBE grant form] errors: {'source_bank_account': ['Select a valid choice. 2000145399/2010 is not one of the available choices.']}

8. FIO_API_CURRENCIES never matches. Token keys get normalized, currency keys don't, but the lookup uses the normalized key — a bare account number silently falls back to CZK (confirmed). Only bites on a non-CZK account. Neither setting is documented in support/settings.py.template or the README, which is how you'd hit it.

9. Payment Details toggle keys on English. h2.textContent.includes('Payment Details') vs the legend _('Payment Details (Bank Transfer)'). Works today only because there's no Czech translation yet; the next translatewiki sync breaks the fieldset toggle.

Follow-ups, not deploy-blocking

BankAccountDeleteView.delete() stops being called on Django 4 — soft delete silently becomes hard delete, which SET_NULLs the account recorded on already-paid expenditures. The global import page still has no POST branch. Expediture.state_str.admin_order_field = 'payment_state' names a field that doesn't exist (dead today — Expediture is only ever an inline). fill_details is re-postable and makes a duplicate BankAccount each time. The co-financing source account still round-trips through free text (description.split("account ")).

What the follow-up patches did fix

The API 500 and the writable automation fields, the delete-then-save in process_cofinancing_link, missing HTTP timeouts, the #1 vs #12 prefix match (now scored, and it refuses to pay when two candidates tie), and the TypeError on a missing date. Plus two problems the earlier review missed: the claim-before-send race, and float-vs-decimal amount comparison. Migrations are clean and additive; existing expenditures land as "Missing data", which is correct — they were never importable.

Change #1337388 had a related patch set uploaded (by Urbanecm; author: Urbanecm):

[wikimedia-cz/tracker@master] tracker: Refuse to delete a co-financing row in the admin

https://gerrit.wikimedia.org/r/1337388

Change #1337389 had a related patch set uploaded (by Urbanecm; author: Urbanecm):

[wikimedia-cz/tracker@master] tracker: Stop the expenditure admin JavaScript from reading English

https://gerrit.wikimedia.org/r/1337389

Change #1337391 had a related patch set uploaded (by Urbanecm; author: Urbanecm):

[wikimedia-cz/tracker@master] i18n: Update django.pot

https://gerrit.wikimedia.org/r/1337391

Change #1337391 merged by jenkins-bot:

[wikimedia-cz/tracker@master] i18n: Update django.pot

https://gerrit.wikimedia.org/r/1337391

Change #1319916 merged by jenkins-bot:

[wikimedia-cz/tracker@master] Implement automated payment system

https://gerrit.wikimedia.org/r/1319916

Change #1319940 merged by jenkins-bot:

[wikimedia-cz/tracker@master] fix(payment automation): Fix several aspects of the automated payment

https://gerrit.wikimedia.org/r/1319940

Change #1337374 merged by jenkins-bot:

[wikimedia-cz/tracker@master] fix(payment automation): Compare the transaction amount as a decimal

https://gerrit.wikimedia.org/r/1337374

Change #1337373 merged by jenkins-bot:

[wikimedia-cz/tracker@master] tracker: Claim expenditures before sending them to Fio

https://gerrit.wikimedia.org/r/1337373

Change #1337386 merged by jenkins-bot:

[wikimedia-cz/tracker@master] api: Do not publish or accept the payment automation fields

https://gerrit.wikimedia.org/r/1337386

Change #1337388 merged by jenkins-bot:

[wikimedia-cz/tracker@master] tracker: Refuse to delete a co-financing row in the admin

https://gerrit.wikimedia.org/r/1337388

Change #1337389 merged by jenkins-bot:

[wikimedia-cz/tracker@master] tracker: Stop the expenditure admin JavaScript from reading English

https://gerrit.wikimedia.org/r/1337389

Change #1344235 had a related patch set uploaded (by Urbanecm; author: Urbanecm):

[wikimedia-cz/tracker@master] tracker: Read the payment reference from the Fio comment

https://gerrit.wikimedia.org/r/1344235

Change #1344235 merged by jenkins-bot:

[wikimedia-cz/tracker@master] tracker: Read the payment reference from the Fio comment

https://gerrit.wikimedia.org/r/1344235

Change #1344256 had a related patch set uploaded (by Urbanecm; author: Urbanecm):

[wikimedia-cz/tracker@master] tracker: Do not show closed tickets for expenditure imports

https://gerrit.wikimedia.org/r/1344256

Change #1344256 merged by jenkins-bot:

[wikimedia-cz/tracker@master] tracker: Do not show closed tickets for expenditure imports

https://gerrit.wikimedia.org/r/1344256