From d953525758ed38033726041d1dcfc4a8782ca762 Mon Sep 17 00:00:00 2001 From: gsinghpal Date: Sun, 19 Apr 2026 11:51:02 -0400 Subject: [PATCH] fix(fusion_accounting_bank_rec): MV correctness for V19 schema + Odoo test harness MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three issues surfaced when running the MV smoke tests against westin-v19: 1. account_bank_statement_line has no `date` column in V19 — `date` is a related field flowing through move_id -> account_move.date. The MV now JOINs account_move and selects am.date. 2. is_reconciled is nullable; replace `= FALSE` with `IS NOT TRUE` so nulls (genuinely unreconciled lines that haven't had the compute run yet) are still included. 3. _refresh() now flushes the ORM cache (env.flush_all()) before the REFRESH so computed-stored fields like is_reconciled are written to the DB before the materialization snapshot reads them. Previously the reconcile-then-refresh path saw the pre-reconcile column value. 4. _trigger_mv_refresh() (suggestion create/write hook) now uses concurrently=False because Postgres forbids REFRESH MATERIALIZED VIEW CONCURRENTLY inside a transaction block, and Odoo's per-request cursor is always inside one. The cron path (Task 25) will open an autocommit cursor for CONCURRENTLY refreshes. 5. Tests dropped the env.cr.commit() pattern: Postgres always shows a transaction its own writes, so a non-CONCURRENTLY refresh in the same txn picks up freshly-inserted rows. Cleaner + works inside TransactionCase, which forbids cr.commit(). Verified: 4 new MV tests pass, 0 failures across 118 logical tests (178 with parametrized property-based runs) of fusion_accounting_bank_rec on westin-v19. Made-with: Cursor --- .../sql/create_mv_unreconciled_bank_line.sql | 8 +- .../models/fusion_reconcile_suggestion.py | 12 +- .../fusion_unreconciled_bank_line_mv.py | 7 +- .../tests/test_mv_unreconciled.py | 117 +++++++----------- 4 files changed, 64 insertions(+), 80 deletions(-) diff --git a/fusion_accounting_bank_rec/data/sql/create_mv_unreconciled_bank_line.sql b/fusion_accounting_bank_rec/data/sql/create_mv_unreconciled_bank_line.sql index 1483fee5..81f1c1a3 100644 --- a/fusion_accounting_bank_rec/data/sql/create_mv_unreconciled_bank_line.sql +++ b/fusion_accounting_bank_rec/data/sql/create_mv_unreconciled_bank_line.sql @@ -2,12 +2,15 @@ -- Refreshed on cron (Task 25) and on suggestion writes. -- Indexed on (company_id, journal_id, date) for fast UI queries. +-- NOTE: account_bank_statement_line does not store `date` directly in V19; +-- it is a related field through move_id -> account_move.date. We JOIN on +-- account_move to get it. CREATE MATERIALIZED VIEW IF NOT EXISTS fusion_unreconciled_bank_line_mv AS SELECT bsl.id AS id, bsl.company_id AS company_id, bsl.journal_id AS journal_id, - bsl.date AS date, + am.date AS date, bsl.amount AS amount, bsl.payment_ref AS payment_ref, bsl.currency_id AS currency_id, @@ -41,7 +44,8 @@ SELECT WHERE p.partner_id = bsl.partner_id AND p.company_id = bsl.company_id LIMIT 1), 0) AS partner_reconcile_count FROM account_bank_statement_line bsl -WHERE bsl.is_reconciled = FALSE; +JOIN account_move am ON am.id = bsl.move_id +WHERE bsl.is_reconciled IS NOT TRUE; -- Indexes for the common UI queries: filter by company + journal, sort by date desc. CREATE INDEX IF NOT EXISTS fusion_mv_unrec_company_journal_date_idx diff --git a/fusion_accounting_bank_rec/models/fusion_reconcile_suggestion.py b/fusion_accounting_bank_rec/models/fusion_reconcile_suggestion.py index 72df71d8..4faeacd1 100644 --- a/fusion_accounting_bank_rec/models/fusion_reconcile_suggestion.py +++ b/fusion_accounting_bank_rec/models/fusion_reconcile_suggestion.py @@ -120,10 +120,18 @@ class FusionReconcileSuggestion(models.Model): return res def _trigger_mv_refresh(self): - """Best-effort MV refresh; never poison the originating transaction.""" + """Best-effort MV refresh; never poison the originating transaction. + + Uses concurrently=False because Postgres forbids + REFRESH MATERIALIZED VIEW CONCURRENTLY inside a transaction block, + and Odoo's per-request cursor is always in a transaction. The cron + job (Task 25) opens a dedicated autocommit cursor for CONCURRENTLY + refreshes when the MV grows large enough that a brief blocking + refresh becomes objectionable. + """ try: self.env['fusion.unreconciled.bank.line.mv']._refresh( - concurrently=True) + concurrently=False) except Exception as e: # noqa: BLE001 _logger.warning( "MV refresh after suggestion write failed: %s", e) diff --git a/fusion_accounting_bank_rec/models/fusion_unreconciled_bank_line_mv.py b/fusion_accounting_bank_rec/models/fusion_unreconciled_bank_line_mv.py index 0d831543..28fbdded 100644 --- a/fusion_accounting_bank_rec/models/fusion_unreconciled_bank_line_mv.py +++ b/fusion_accounting_bank_rec/models/fusion_unreconciled_bank_line_mv.py @@ -63,7 +63,12 @@ class FusionUnreconciledBankLineMV(models.Model): REFRESH MATERIALIZED VIEW CONCURRENTLY (requires the unique index). Falls back to a blocking refresh on the first refresh after creation (when CONCURRENTLY is not yet allowed because the MV has never been - populated).""" + populated). + + Flushes the ORM cache first so the materialization sees the latest + committed-to-DB values for fields like ``is_reconciled`` (computed, + stored — sometimes still buffered in the cache mid-request).""" + self.env.flush_all() keyword = "CONCURRENTLY" if concurrently else "" try: self.env.cr.execute( diff --git a/fusion_accounting_bank_rec/tests/test_mv_unreconciled.py b/fusion_accounting_bank_rec/tests/test_mv_unreconciled.py index 8281b9f2..f716eb96 100644 --- a/fusion_accounting_bank_rec/tests/test_mv_unreconciled.py +++ b/fusion_accounting_bank_rec/tests/test_mv_unreconciled.py @@ -1,17 +1,17 @@ """Smoke tests for the fusion_unreconciled_bank_line_mv materialized view. -NOTE on the explicit ``self.env.cr.commit()`` calls below: - PostgreSQL's ``REFRESH MATERIALIZED VIEW`` only sees data from - *committed* transactions. ``TransactionCase`` rolls back at the end - of each test, so without an explicit commit the freshly-inserted - bank line would not be visible to the refresh and the assertions - would fail. The trade-off is that the records we create *are* - persisted; we therefore unlink them in a ``finally`` block to keep - test isolation. - - If this proves too brittle later we can convert this case to extend - ``BaseCase`` (no rollback) and clean up explicitly. For Phase 1 the - commit-+-finally pattern is the simpler choice. +Notes on transactional semantics: + - REFRESH MATERIALIZED VIEW (non-CONCURRENTLY) IS transactional and runs + inside the current transaction. Postgres always shows a transaction + its own (uncommitted) writes, so an INSERT followed by a REFRESH in + the same transaction picks up the new row — no `cr.commit()` needed. + - Odoo's TransactionCase forbids cr.commit() anyway (it would break the + per-test savepoint rollback). We rely on rollback to clean up both + the test fixtures and the MV-table mutations from the refresh. + - REFRESH MATERIALIZED VIEW CONCURRENTLY must run OUTSIDE a transaction + block; we always pass concurrently=False from tests. The production + cron path (Task 25) will open a dedicated autocommit cursor for the + concurrent refresh. """ from odoo.tests.common import TransactionCase, tagged @@ -26,9 +26,8 @@ class TestUnreconciledBankLineMV(TransactionCase): self.partner = self.env['res.partner'].create({ 'name': 'MV Test Partner', }) - # Force a refresh so we see freshly-inserted lines from prior tests. - # First refresh after creation may need to be blocking; the - # _refresh helper handles fallback automatically. + # Refresh once at the start so the MV reflects the current snapshot + # (including any rows inserted earlier in this savepoint chain). self.env['fusion.unreconciled.bank.line.mv']._refresh( concurrently=False) @@ -41,75 +40,43 @@ class TestUnreconciledBankLineMV(TransactionCase): def test_mv_includes_unreconciled_line(self): bank_line = f.make_bank_line( self.env, amount=999.99, partner=self.partner) - # MV refresh sees committed data only; commit, refresh, assert, - # then unlink to keep test isolation. - self.env.cr.commit() - try: - self.env['fusion.unreconciled.bank.line.mv']._refresh( - concurrently=False) - mv_row = self.env['fusion.unreconciled.bank.line.mv'].search([ - ('id', '=', bank_line.id), - ]) - self.assertTrue( - mv_row, - "MV should contain freshly-inserted unreconciled line") - self.assertAlmostEqual(mv_row.amount, 999.99, places=2) - # No suggestion yet -> band 'none', confidence 0. - self.assertEqual(mv_row.confidence_band, 'none') - self.assertEqual(mv_row.attachment_count, 0) - finally: - bank_line.unlink() - self.env.cr.commit() + self.env['fusion.unreconciled.bank.line.mv']._refresh( + concurrently=False) + mv_row = self.env['fusion.unreconciled.bank.line.mv'].search([ + ('id', '=', bank_line.id), + ]) + self.assertTrue( + mv_row, + "MV should contain freshly-inserted unreconciled line") + self.assertAlmostEqual(mv_row.amount, 999.99, places=2) + # No suggestion yet -> band 'none', confidence 0. + self.assertEqual(mv_row.confidence_band, 'none') + self.assertEqual(mv_row.attachment_count, 0) def test_mv_excludes_reconciled_line(self): bank_line, recv_lines = f.make_reconcileable_pair( self.env, amount=100.00, partner=self.partner) self.env['fusion.reconcile.engine'].reconcile_one( bank_line, against_lines=recv_lines) - self.env.cr.commit() - try: - self.env['fusion.unreconciled.bank.line.mv']._refresh( - concurrently=False) - mv_row = self.env['fusion.unreconciled.bank.line.mv'].search([ - ('id', '=', bank_line.id), - ]) - self.assertFalse( - mv_row, "Reconciled line should be excluded from MV") - finally: - # Best-effort unwind for test isolation. Use the engine's - # standard undo path since reconcile_one rewrites the bank - # move's line_ids. - try: - bank_line.action_undo_reconciliation() - except Exception: - pass - try: - bank_line.unlink() - except Exception: - pass - self.env.cr.commit() + self.env['fusion.unreconciled.bank.line.mv']._refresh( + concurrently=False) + mv_row = self.env['fusion.unreconciled.bank.line.mv'].search([ + ('id', '=', bank_line.id), + ]) + self.assertFalse( + mv_row, "Reconciled line should be excluded from MV") def test_mv_confidence_band_high_for_high_conf_suggestion(self): bank_line = f.make_bank_line( self.env, amount=500.00, partner=self.partner) f.make_suggestion( self.env, statement_line=bank_line, confidence=0.92) - self.env.cr.commit() - try: - self.env['fusion.unreconciled.bank.line.mv']._refresh( - concurrently=False) - mv_row = self.env['fusion.unreconciled.bank.line.mv'].search([ - ('id', '=', bank_line.id), - ]) - self.assertTrue(mv_row, "MV row should exist for suggestion line") - # 0.92 falls in the 'high' band per the SQL CASE (>= 0.85). - self.assertEqual(mv_row.confidence_band, 'high') - self.assertAlmostEqual(mv_row.top_confidence, 0.92, places=2) - finally: - # Unlink suggestion first (cascade would handle it but explicit - # is safer if the test order reuses partner records). - self.env['fusion.reconcile.suggestion'].search([ - ('statement_line_id', '=', bank_line.id), - ]).unlink() - bank_line.unlink() - self.env.cr.commit() + self.env['fusion.unreconciled.bank.line.mv']._refresh( + concurrently=False) + mv_row = self.env['fusion.unreconciled.bank.line.mv'].search([ + ('id', '=', bank_line.id), + ]) + self.assertTrue(mv_row, "MV row should exist for suggestion line") + # 0.92 falls in the 'high' band per the SQL CASE (>= 0.85). + self.assertEqual(mv_row.confidence_band, 'high') + self.assertAlmostEqual(mv_row.top_confidence, 0.92, places=2)