fix(db,web): convert current_value_eur from the amendment currency, not signing currency (#245) - #257
Conversation
…ot signing currency (midt-bg#245) contracts.current_value is populated from the latest amendment's value_after, denominated in THAT amendment's own currency — but every EUR conversion of current_value used contracts.currency (the contract's original signing currency), silently re-converting an already-EUR amendment (e.g. one recorded after ЦАИС ЕОП's 2026 BGN->EUR feed switch) and halving the reported value. Add contracts.current_value_currency to track which currency last set current_value, and route every current_value -> EUR conversion (refresh-slice, normalize-raw, precompute, the contract detail query) through it instead of contracts.currency. signing_value/signing_value_eur are unaffected — signing really is denominated in the contract's own currency.
lyubomir-bozhinov
left a comment
There was a problem hiding this comment.
Прегледах стриктно на връх 621eeea, с емпирична проверка. Посоката е правилна и по-трудната част е свършена — нова колона current_value_currency, попълва се от валутата на последния анекс (promotion UPDATE + amendment_winner CTE), и current_value_eur вече конвертира по нея в precompute/refresh-slice/details.ts. Това затваря half-value-то на показаната текуща стойност. Migration-approach-ът (само 0002, не в 0000_init) е коректен — D1 пътят получава колоната през миграцията.
Блокер (accuracy — проверих го емпирично): amount_eur остава наполовина
Фиксът не стига до паричната база. В recalc-а (refresh-slice.sql, recalculated CTE → UPDATE на ~1224) new_amount_eur конвертира trusted_native по currency (валутата на договора), а за 'ok' договор с анекс trusted_native = COALESCE(current_value, signing_value) = current_value — стойността на анекса, деноминирана в current_value_currency. Същият шаблон е и в normalize-raw.sql:390-392.
Възпроизведох recalc-а за случая Hemus (УНП 00044-2024-0047, договор в лева + евро-анекс 104 748 559,44):
new_current_value_eur = 104748559.44 ✓ правилно (по current_value_currency)
new_amount_eur = 53557088.01 ✗ ÷1.95583 — наполовина (по currency)
amount_eur е базата на ВСИЧКИ ролове (authority_totals/company_totals/home_totals сумират amount_eur) и на стойността в списъка /contracts. Значи след фикса страницата на договора ще показва вярната текуща стойност (104,7 млн. €), но сумите по възложител/фирма и редът в списъка остават наполовина (53,5 млн. €) — и се появява видимо противоречие страница↔тотал. Това е точно amount_eur-кракът от #245, който остава неадресиран.
Поправка: изведи current-value крака на amount_eur през current_value_currency също — напр. за не-suspect флаговете amount_eur = COALESCE(new_current_value_eur, new_signing_value_eur), за да преизползва вече правилните per-leg конверсии, вместо да конвертира смесения trusted_native с една валута. signing_value_eur си е коректен (signing наистина е във валутата на договора) — фиксът е асиметричен, точно това е капанът.
Дребно (координация)
0002_current_value_currency.sql дели номер 0002 с 7 други отворени PR-а (#226 / #253 / #193 / #172 / #171 / #170 / #169). Вземи разграничен свободен номер преди merge.
Изисквам промени (блокерът amount_eur); иначе основата е добра и близо.
ydimitrof
left a comment
There was a problem hiding this comment.
Преглед на PR #245 — конвертиране на current_value_eur от валутата на анекса, а не от валутата при подписване
Благодаря за прегледната работа — промяната адресира реален проблем с целостта на данните (след превключването BGN→EUR на ЦАИС ЕОП през 2026 г. current_value може да е в различна валута от валутата при подписване) и е придружена от смислени тестове. По-долу са наблюденията по фази.
Фаза 0 — Скан за сигурност (задължителна, блокираща): ЧИСТО
- Няма хардкоднати тайни (API ключове, пароли, токени). Низовете от типа
eop:annexes:2026-06-02са идентификатори на източник, не URL или креденшъли. - Няма нови/променени URL адреси, няма нови зависимости.
- Няма зловредни шаблони (бекдори, инжекции, обфускация). Целият SQL е статичен; в
details.tsконверсията минава презeurFromNative(...), без конкатенация на вход в заявка → без риск от SQL инжекция. - OWASP: няма нови външни входни точки в този diff; входът остава параметризиран. Няма забележки.
Фаза 1/2 — Функционален и архитектурен преглед
Силни страни
- Логиката е последователна между
precompute.sql,refresh-slice.sql(и двата INSERT блока) иnormalize-raw.sql: навсякъде се използваCOALESCE(NULLIF(current_value_currency, ''), NULLIF(currency, ''), 'BGN'), с еднакъв fallback към валутата при подписване. details.tsповтаря същия fallback (r.current_value_currency || r.contract_currency), така че изчисляването в реално време съвпада с предварително изчислената стойност.- Миграцията добавя NULL-ова колона без default; старите редове стават NULL и се обработват коректно от NULLIF/COALESCE fallback-ите. Съвместимо назад.
Забележки за изясняване (виж инлайн коментарите)
- Целост на данните — съответствие на подредбата (важно): В
normalize-raw.sqlamendment_winnerизбира валута сORDER BY published_at DESC, natural_key DESC, докато финалният UPDATE вrefresh-slice.sqlизползваORDER BY published_at DESC, a.id DESC. Коментарът твърди, чеamendment_winner„огледално" повтаря избора наcurrent_valueотderive-amendments.sql. Ако подредбата/tie-break вderive-amendments.sqlсе различава, избраната валута може да съответства на ДРУГ анекс, различен от този, дал стойността → грешна конверсия. Моля, потвърдете, че и трите места избират точно същия ред при равниpublished_at. - Чужда валута (не BGN/EUR) на анекс (граничен случай): Когато
current_value_currencyе чужда валута, конверсията пада доcurrent_value * fx_rate, ноfx_rateе изчислен за валутата при подписване и към датата на подписване — не за валутата/датата на анекса. За договор, подписан в BGN (fx_rate= NULL), анекс в чужда валута даваcurrent_value_eur = NULL(загуба на данни, но безопасно — без грешно число). В домейна анексите реалистично са BGN/EUR, затова е нисък приоритет, но е добре да се документира като известно ограничение.
Фаза 3 — Тестове, качество, документация
- Новият тест (
#245) е смислен: покрива BGN договор + EUR анекс и проверяваcurrent_value_currency,current_value_eur≈2000и чеsigning_value_eurостава в BGN. Не е „читав" тест. - Липсващо покритие: (а) анекс в чужда валута върху BGN договор (граничния случай по т.2), (б) fallback при празна (
'') валута на анекс, (в) път презdetails.ts. Препоръчвам поне (а) и (б). - Документация: SQL коментарите са отлични и обясняват намерението. Няма API/потребителска документация за актуализация. Няма breaking changes за миграционен наръчник.
Съответствие с CLAUDE.md / гейтове за качество
- Без частична имплементация, без TODO, без дублиран/мъртъв код, последователно именуване, без смесване на отговорности. ОК.
- Гейт за тестове (≥90% / разкриващи дефекти): частично — виж липсващото покритие по-горе.
- Обратимост на миграцията:
ADD COLUMNе обратима само чрезDROP COLUMN(нужен по-нов SQLite); ако проектът изисква down-миграции, добавете такава.
Вердикт: COMMENT — не одобрявам преди изясняване на т.1 (съответствие на подредбата спрямо derive-amendments.sql) и добавяне на тест за граничния случай с чужда валута; сканът за сигурност е чист, не е блокиращ.
refresh-slice.sql's final UPDATE picked current_value and current_value_currency via two independent ORDER BY a.published_at DESC, a.id DESC subqueries, diverging from the published_at DESC, natural_key DESC tie-break used everywhere else (normalize-raw.sql's amendment_winner CTE, derive-amendments.sql). On an equal published_at, the two subqueries could therefore pick different annexes, pairing a value with the wrong currency. Consolidate both columns onto a single amendment_winner CTE so they always come from the same winning row, and document the known current_value_eur NULL-on-foreign-annex limitation in precompute.sql.
Extends the midt-bg#245 current_value_eur test with two cases requested on PR midt-bg#257 review: a USD annex over a BGN contract (asserts current_value_eur stays NULL per the documented known limitation) and an annex with an empty-string currency (asserts the COALESCE(NULLIF(...), contract currency) fallback resolves correctly).
ydimitrof
left a comment
There was a problem hiding this comment.
Преглед на PR #245 — конвертиране на current_value_eur по валутата на анекса, а не по валутата на подписване
ВЕРДИКТ: COMMENT — не блокиращо, но 2–3 въпроса изискват потвърждение преди APPROVE
Фаза 0 — задължителен скан за сигурност: ЧИСТО
- Няма hardcoded тайни (ключове, пароли, токени).
- Няма нови или променени URL адреси; няма промени в зависимости/пакети.
- Няма злонамерени шаблони (backdoor, инжекция на код, обфускация).
- Целият SQL е статичен — няма конкатенация на потребителски вход, следователно няма риск от SQL инжекция. Промяната в
details.tsсамо добавя колона към статична заявка (без параметри от вход). OWASP: няма injection / изтичане на тайни / промени в контрола на достъпа.
Съответствие с тикета
Имплементацията отговаря на описанието: добавена е колона current_value_currency (миграция 0002), която се попълва от печелившия анекс (amendment_winner) и се използва навсякъде при изчислението на current_value_eur вместо валутата на подписване (contracts.currency). Пътищата normalize-raw.sql, refresh-slice.sql и precompute.sql са синхронизирани. Тестовете покриват трите ключови сценария: EUR анекс (без повторно деление на пега), чужда валута върху BGN договор (→ NULL, безопасно), и празна валута (→ fallback към валутата на договора).
Качество и тестове
Тестовете са смислени — проверяват реални числени стойности, а не тривиални условия, и документират известното ограничение (#257). Покритието на новата логика е добро. Спазени са изискванията за именуване и стил; няма мъртъв код, дублиране или частична имплементация.
Установени въпроси (детайли в инлайн коментарите)
precompute.sql: остава тиха грешна конверсия при чужда валута на анекс върху договор в друга чужда валута (fx_rateе за валутата на подписване). Документираният коментар покрива само случаяfx_rate IS NULL(чужд анекс върху не-чужд договор).refresh-slice.sql: смяната на вторичния критерий за подредба отid DESCнаnatural_key DESCможе да промени избранияcurrent_valueпри равниpublished_at— това е промяна на стойност извън обявения (валутен) обхват; да се потвърди като желана и да се фиксира с тест.normalize-raw.sql:current_valueидва отraw_contracts, а валутата — отamendment_winner; да се потвърди, че двете произлизат от един и същ анекс (в противен случай конверсията е с несъответстваща валута).
Оценка
Няма блокиращи проблеми по сигурност или интегритет на данните. Препоръчвам COMMENT: адресирайте т.1–3 (или ги документирайте изрично), след което PR е готов за одобрение.
| WHEN COALESCE(currency,'BGN') = 'BGN' THEN current_value / 1.95583 | ||
| WHEN COALESCE(NULLIF(current_value_currency, ''), NULLIF(currency, ''), 'BGN') = 'EUR' THEN current_value | ||
| WHEN COALESCE(NULLIF(current_value_currency, ''), NULLIF(currency, ''), 'BGN') = 'BGN' THEN current_value / 1.95583 | ||
| WHEN fx_rate IS NOT NULL THEN current_value * fx_rate |
There was a problem hiding this comment.
Тази ELSE клауза използва fx_rate, който е курсът на валутата на ПОДПИСВАНЕ към датата на подписване. Когато current_value_currency е чужда валута, различна от валутата на подписване (напр. договор в USD с анекс в GBP), current_value се умножава по курса на грешна валута → тихо грешна EUR стойност, показана на потребителя. Документираният коментар по-горе покрива само случая fx_rate IS NULL (чужд анекс върху не-чужд договор → NULL, безопасно), но не и foreign-анекс върху foreign-договор с различни валути. Тъй като целта на този PR е именно да предотврати конверсия с курса на грешна валута, моля добавете тест/документация за този случай, или конвертирайте по fx_rate само когато current_value_currency съвпада с валутата, за която е изчислен fx_rate (иначе → NULL).
| SELECT a.unp, a.contract_number, a.value_after, a.currency, | ||
| ROW_NUMBER() OVER ( | ||
| PARTITION BY a.unp, a.contract_number | ||
| ORDER BY a.published_at DESC, a.natural_key DESC |
There was a problem hiding this comment.
Вторичният критерий за подредба се сменя от a.id DESC (старата логика за current_value) на a.natural_key DESC. При равни published_at това може да избере различен анекс и съответно да промени current_value (и current_value_eur) за вече съществуващи договори — промяна на стойност извън обявения (валутен) обхват на PR-а. Разбирам мотива (стойността и валутата да идват от един и същ печеливш ред, за да не се разминат). Моля потвърдете, че тази промяна на избора на стойност е приемлива, и добавете тест, който фиксира поведението при равен published_at (детерминизъм на tie-break). Уверете се също, че таблицата amendments действително има колона natural_key в този контекст.
| -- current_value can be denominated in a DIFFERENT currency than the contract's own (when the | ||
| -- winning amendment_winner row recorded one) — precompute.sql's later current_value_eur pass | ||
| -- uses this column instead of `currency` so that amendment isn't re-converted a second time. | ||
| COALESCE(NULLIF(y.amendment_currency, ''), NULLIF(y.currency, ''), 'BGN') AS current_value_currency, |
There was a problem hiding this comment.
Тук current_value_currency се извлича от amendment_winner (y.amendment_currency), докато самата current_value идва от raw_contracts (y.current_value). Ако тези две произлизат от различни анекси, current_value_eur ще бъде конвертирано с валута, която не съответства на стойността. В precompute.sql/refresh-slice.sql стойността и валутата се четат от един и същ печеливш ред (win_rn = 1), което гарантира съответствие; тук инвариантата зависи от това raw_contracts.current_value вече да отразява стойността на печелившия анекс. Моля потвърдете тази инварианта изрично (коментар/тест) или четете и стойността от amendment_winner за симетрия и устойчивост.
lyubomir-bozhinov
left a comment
There was a problem hiding this comment.
Прегледах двата нови комита (fb3ed1b5 tie-break, a8ff2ccd fallback-тестове). Добри са и current_value_eur вече е коректен — конвертира се по current_value_currency (refresh-slice.sql:1197-1198). Но основният блокер от прегледа ми на 07-20 остава отворен: amount_eur — базата, върху която стъпват ВСИЧКИ обобщения и списъци — все още се конвертира по валутата на договора (currency), не по валутата на анекса (current_value_currency).
Къде точно:
refresh-slice.sql:1211-1212(CTErecalculated):new_amount_eurсмятаtrusted_nativeпрезCOALESCE(currency,'BGN'). Ноtrusted_native(ред 1188) =COALESCE(current_value, signing_value)=current_valueза 'ok' редовете, аcurrent_valueе вcurrent_value_currency, не вcurrency.normalize-raw.sql:390-393: същото —amount_eurконвертираtrusted_nativeпрезx.currency. (Коментарът точно над него дори обяснява, чеcurrent_value_currencyсъществува „so that amendment isn't re-converted" — но се ползва само за current_value_eur, не за amount_eur.)
Емпирично (случая Хемус: договор в лева, евро-анекс value_after = 104 748 559,44):
| поле | стойност | конверсия |
|---|---|---|
trusted_native |
104 748 559,44 | (= current_value, в EUR) |
new_current_value_eur |
104 748 559,44 | по current_value_currency=EUR → коректно ✓ |
new_amount_eur |
53 557 088,01 | по currency=BGN → ÷1,95583 → наполовина ✗ |
Множител на грешката ×0,5113. Т.е. страницата на договора показва вярната текуща стойност (104,7 млн.), но amount_eur (наполовинена) е базата за company/authority/home_totals + листовете → обобщенията подбиват точно тези договори, а на страницата числата си противоречат — точно симптомът от #245, който PR-ът поправя за дисплей-крака, но не и за сумарния.
Фикс (асиметричен, реюзва вече коректните EUR-крака вместо да преконвертира trusted_native):
CASE new_value_flag
WHEN 'value_suspect' THEN proc_est_eur
WHEN 'annex_suspect' THEN COALESCE(new_signing_value_eur, new_current_value_eur)
ELSE COALESCE(new_current_value_eur, new_signing_value_eur)
END AS new_amount_eur(огледално и в normalize-raw.sql). Така сумарната база съвпада с current_value_eur за анексираните в чужда валута.
Свързано (не блокер, провери отделно): display_native/amount за същите редове носи чуждовалутната стойност на анекса под етикета на договорната валута — money(amount, currency) би я показал като „104,7 млн. лв." вместо „€". Основното е amount_eur.
Все още блокирам заради amount_eur. Останалото по PR-а е наред.
normalize-raw.sql and refresh-slice.sql each implement their own ROW_NUMBER() OVER (... ORDER BY published_at DESC, natural_key DESC) tie-break for picking a contract's winning amendment. A human review caught the two drifting (fb3ed1b, midt-bg#257); nothing automated would catch the next drift. Extract each script's live ORDER BY clause and run it against a seeded published_at tie so a future divergence fails loudly.
lyubomir-bozhinov
left a comment
There was a problem hiding this comment.
Проверих на текущия HEAD (728b87ed): новият комит е регресионен тест за tie-break-а на печелившия анекс — не пипа конверсията. new_amount_eur все още се смята по валутата на договора:
- refresh-slice.sql:1211-1212 —
WHEN COALESCE(currency,'BGN')='BGN' THEN trusted_native / 1.95583 - normalize-raw.sql:391-392 — същото, по
x.currency
Нула места ползват current_value_currency за amount_eur. Тъй че блокерът от предишния ми коментар стои непроменен (байт по байт същият код): за договор в лева с евро-анекс от 2026 trusted_native = current_value (в EUR), но се дели на 1,95583 → amount_eur наполовина (доказано: 104 748 559,44 → 53 557 088,01). current_value_eur е коректен; amount_eur — сумарната база за всички обобщения и листове — не е.
Фиксът (реюзвай вече коректните new_current_value_eur/new_signing_value_eur крака по флаг, вместо да преконвертираш trusted_native по currency) и пълното доказателство са в предишния ми коментар. Все още блокирам заради amount_eur.
|
Благодаря ти, @StanislavBG - работата тук е добра и влиза в основата. Ядрото на решението (изборът на печелившия анекс и дедупликацията) го проверих срещу реалната база и възпроизвежда 17 867 от 17 867 реда - правилото, което си кодирал, е точно правилото, произвело данните. Затварям този PR и пренасям кода в нов - #261 - но не защото работата отпада, а по техническа причина, която искам да ти обясня, защото ще ти помогне и за напред. Защо не можахме да допълним този PR директно. Довършихме липсващите части ( Как да форкнеш правилно за следващия път. От страницата на Ако искаш да запазиш текущата си работа: направи истински форк както горе, добави го като remote и бутни клона си там. Авторството ти е запазено - оригиналният ти комит е първи в #261, с твоето име. Благодаря отново и разчитаме на още приноси. |
What changed
Fixes #245 — contracts originally signed in BGN that received a EUR-denominated amendment (e.g. after ЦАИС ЕОП's 2026 feed switch to EUR) showed
current_value_eurat roughly half its true value. The EUR amendment amount was being divided by the BGN peg (1.95583) a second time.Root cause:
contracts.current_valueis set from the latest amendment'svalue_after, denominated in that amendment's own currency — but every EUR conversion ofcurrent_valueusedcontracts.currency(the contract's original signing currency) instead.Fix: added
contracts.current_value_currency(migration0002_current_value_currency.sql) to track which currency last setcurrent_value, and routed everycurrent_value → EURconversion through it instead ofcontracts.currency:scripts/refresh-slice.sql(both O/E code paths + the incremental recalc block, plus theeff_eur/annex_suspectdetection feedingcompany_totals/authority_totals/home_totals)scripts/normalize-raw.sql(full-rebuild parity)scripts/precompute.sqlpackages/db/src/queries/details.ts's EUR fallbacksigning_value/signing_value_eurare unchanged — signing is genuinely denominated in the contract's own currency, so this is an intentionally asymmetric fix.Review round (ydimitrof, 2026-07-20):
refresh-slice.sql's finalUPDATEpickedcurrent_valueandcurrent_value_currencyfrom two independentORDER BY published_at DESC, a.id DESCsubqueries, diverging from thepublished_at DESC, natural_key DESCtie-break used everywhere else (normalize-raw.sql'samendment_winnerCTE,derive-amendments.sql). On an equalpublished_at, the two subqueries could pick different annexes, pairing a value with the wrong currency. Consolidated both columns onto a singleamendment_winnerCTE so they always come from the same winning row (fb3ed1b).current_value_eurNULL-on-foreign-annex limitation directly inprecompute.sql(fb3ed1b): when the winning annex's currency isn't BGN/EUR, there's no FX rate for it as of the amendment date, socurrent_value_eurstays NULL rather than silently using the wrong rate.current_value_eurstays NULL per the documented limitation) and an annex with an empty-string currency (asserts theCOALESCE(NULLIF(...), contract currency)fallback resolves correctly) (a8ff2cc).How tested
Reproduced the exact case from #245 locally: contract
c:e:00044-2024-0047:174654:eik:831646048:1(УНП 00044-2024-0047) —current_value_eurwas53557088.0086715, should be104748559.44(the amendment's own EUR value, confirmed against the amendments table).packages/db/src/refresh-slice.test.tsnow covers three cases for the winning-annex currency handling: BGN contract + EUR-denominated last amendment (equals the amendment's raw EUR value, unconverted;signing_value_eurunaffected), USD annex over a BGN contract (NULLcurrent_value_eur, documented limitation), and an annex with an empty-string currency (falls back to the contract's currency). Verified independently in a clean worktree (not just the authoring run):pnpm --filter @sigma/db test— 28/28 files, 200/200 tests passQuality checks run
@sigma/db) — passNot stacked on #203 (ETL rewrite) or #188 (unrelated quality-index fix) — this targets
maindirectly since it's a data-correctness fix.